From d39620e36770c8baeacff74152a812208fbc70f5 Mon Sep 17 00:00:00 2001
From: Matthias Bolte Post patches in unified diff format. A command similar to this
should work:
or:
Run the automated tests on your code before submitting any changes.
In particular, configure with compile warnings set to -Werror:
and run the tests:
The latter test checks for memory leaks.
- If you encounter any failing tests, the VIR_TEST_DEBUG
- environment variable may provide extra information to debug
- the failures. Larger values of VIR_TEST_DEBUG may provide
- larger amounts of information:
-
+ If you encounter any failing tests, the VIR_TEST_DEBUG
+ environment variable may provide extra information to debug
+ the failures. Larger values of VIR_TEST_DEBUG may provide
+ larger amounts of information:
+
- Also, individual tests can be run from inside the 'tests/'
- directory, like:
-
+ Also, individual tests can be run from inside the
General tips for contributing patches
-
- diff -urp libvirt.orig/ libvirt.modified/ > libvirt-myfeature.patch
+
+ diff -urp libvirt.orig/ libvirt.modified/ > libvirt-myfeature.patch
+
- git diff > libvirt-myfeature.patch
+
+ git diff > libvirt-myfeature.patch
+
- ./configure --enable-compile-warnings=error
+
+ ./configure --enable-compile-warnings=error
+
+
make check
make syntax-check
- make -C tests valgrind
+ make -C tests valgrind
+
+
+
VIR_TEST_DEBUG=1 make check (or)
- VIR_TEST_DEBUG=2 make check
-
- ./qemuxml2xmltest
+ VIR_TEST_DEBUG=2 make check
+tests/
+ directory, like:
+
+ ./qemuxml2xmltest
+
+;;; When editing C sources in libvirt, use this style. (defun libvirt-c-mode () "C mode with adjusted defaults for use with libvirt." @@ -105,7 +110,7 @@ around operators and keywords: -+indent-libvirt() { indent -bad -bap -bbb -bli4 -br -ce -brs -cs -i4 -l75 -lc75 \ @@ -116,7 +121,7 @@Note that sometimes you'll have to post-process that output further, by - piping it through "expand -i", since some leading TABs can get through. + piping it through
@@ -125,18 +130,20 @@expand -i
, since some leading TABs can get through. Usually they're in macro definitions or strings, and should be converted anyhow.Curly braces
- Omit the curly braces around an "if", "while", "for" etc. body only + Omit the curly braces around an
if
,while
, +for
etc. body only when that body occupies a single line. In every other case we require the braces. This ensures that it is trivially easy to identify a - single-*statement* loop: each has only one *line* in its body. + single-statement loop: each has only one line in its body.Omitting braces with a single-line body is fine:
-+while (expr) // one-line body -> omitting curly braces is ok - single_line_stmt ();+ single_line_stmt(); +However, the moment your loop/if/else body extends onto a second @@ -146,26 +153,29 @@ it is already a multi-statement loop:
-+while (true) // BAD! multi-line body with no braces /* comment... */ - single_line_stmt ();+ single_line_stmt(); +Do this instead:
-+while (true) { // Always put braces around a multi-line body. /* comment... */ - single_line_stmt (); - }+ single_line_stmt(); + } +There is one exception: when the second body line is not at the same indentation level as the first body line:
-+if (expr) - die ("a diagnostic that would make this line" - " extend past the 80-column limit"));+ die("a diagnostic that would make this line" + " extend past the 80-column limit")); +It is safe to omit the braces in the code above, since the @@ -177,40 +187,44 @@ To reiterate, don't do this:
-+if (expr) // BAD: no braces around... while (expr_2) { // ... a multi-line body ... - }+ } +Do this, instead:
-+if (expr) { while (expr_2) { ... } - }+ } +However, there is one exception in the other direction, when even a one-line block should have braces. That occurs when that one-line, - brace-less block is an "else" block, and the corresponding "then" block - *does* use braces. In that case, either put braces around the "else" - block, or negate the "if"-condition and swap the bodies, putting the + brace-less block is an
-else
block, and the corresponding +then
block does use braces. In that case, either + put braces around theelse
block, or negate the +if
-condition and swap the bodies, putting the one-line block first and making the longer, multi-line block be the - "else" block. +else
block.+if (expr) { ... ... } else - x = y; // BAD: braceless "else" with braced "then"+ x = y; // BAD: braceless "else" with braced "then" +This is preferred, especially when the multi-line body is more than a @@ -219,43 +233,45 @@ after the more involved block:
-+if (!expr) x = y; // putting the smaller block first is more readable else { ... ... - }+ } +If you'd rather not negate the condition, then at least add braces:
-+if (expr) { ... ... } else { x = y; - }+ } +Preprocessor
For variadic macros, stick with C99 syntax:
-+#define vshPrint(_ctl, ...) fprintf(stdout, __VA_ARGS__) -+Use parenthesis when checking if a macro is defined, and use indentation to track nesting:
-+#if defined(HAVE_POSIX_FALLOCATE) && !defined(HAVE_FALLOCATE) # define fallocate(a,ignored,b,c) posix_fallocate(a,b,c) #endif -+C types
@@ -266,45 +282,51 @@Scalars
int
or long
, odds are
+ good that there's a better type.size_t
(use
+ ssize_t
only if required).off_t
.off_t
.unsigned int
;
(on all but oddball embedded systems, you can assume that that
type is at least four bytes wide).bool
type
+ and use the corresponding true
and false
macros.
+ It's ok to include <stdbool.h>, since libvirt's use of gnulib ensures
that it exists and is usable.int32_t
, uint32_t
,
+ uint64_t
, etc.bool
is good for readability, it comes with
+ minor caveats:
bool
in places where the type size must be constant across
all systems, like public interfaces and on-the-wire protocols. Note
- that it would be possible (albeit wasteful) to use "bool" in libvirt's
- logical wire protocol, since XDR maps that to its lower-level bool_t
- type, which *is* fixed-size.bool
in libvirt's
+ logical wire protocol, since XDR maps that to its lower-level bool_t
+ type, which is fixed-size.true
,
+ since a value with a logical non-false value need not be 1
.
+ I.e., don't write if (seen == true) ...
. Rather,
+ write if (seen)...
.
Of course, take all of the above with a grain of salt. If you're about
- to use some system interface that requires a type like size_t, pid_t or
- off_t, use matching types for any corresponding variables.
+ to use some system interface that requires a type like size_t
,
+ pid_t
or off_t
, use matching types for any
+ corresponding variables.
- Also, if you try to use e.g., "unsigned int" as a type, and that
+ Also, if you try to use e.g., unsigned int
as a type, and that
conflicts with the signedness of a related variable, sometimes
- it's best just to use the *wrong* type, if "pulling the thread"
+ it's best just to use the wrong type, if pulling the thread
and fixing all related variables would be too invasive.
- Ensure that all of your pointers are "const-correct".
+ Ensure that all of your pointers are const-correct.
Unless a pointer is used to modify the pointed-to storage,
- give it the "const" attribute. That way, the reader knows
+ give it the const
attribute. That way, the reader knows
up-front that this is a read-only pointer. Perhaps more
importantly, if we're diligent about this, when you see a non-const
pointer, you're guaranteed that it is used to modify the storage
@@ -336,57 +358,57 @@
eg to allocate a single object:
- +e.g. to allocate a single object:
- virDomainPtr domain; + virDomainPtr domain; - if (VIR_ALLOC(domain) < 0) { - virReportOOMError(); - return NULL; - } -
eg to allocate an array of objects
+ if (VIR_ALLOC(domain) < 0) { + virReportOOMError(); + return NULL; + } + +e.g. to allocate an array of objects
- virDomainPtr domains; - int ndomains = 10; - - if (VIR_ALLOC_N(domains, ndomains) < 0) { - virReportOOMError(); - return NULL; - } -
eg to allocate an array of object pointers
+ if (VIR_ALLOC_N(domains, ndomains) < 0) { + virReportOOMError(); + return NULL; + } + +e.g. to allocate an array of object pointers
- virDomainPtr *domains; - int ndomains = 10; + virDomainPtr *domains; + int ndomains = 10; - if (VIR_ALLOC_N(domains, ndomains) < 0) { - virReportOOMError(); - return NULL; - } -
eg to re-allocate the array of domains to be longer
+ if (VIR_ALLOC_N(domains, ndomains) < 0) { + virReportOOMError(); + return NULL; + } + +e.g. to re-allocate the array of domains to be longer
- ndomains = 20 - - if (VIR_REALLOC_N(domains, ndomains) < 0) { - virReportOOMError(); - return NULL; - } -
eg to free the domain
+ if (VIR_REALLOC_N(domains, ndomains) < 0) { + virReportOOMError(); + return NULL; + } + +e.g. to free the domain
- VIR_FREE(domain); -
eg close a file descriptor
- +e.g. close a file descriptor
- if (VIR_CLOSE(fd) < 0) { - virReportSystemError(errno, _("failed to close file")); - } -
eg close a file descriptor in an error path, without losing - the previous errno value
+ the previouserrno
value
- VIR_FORCE_CLOSE(fd); -
For strict equality:
-- STREQ(a,b) - STRNEQ(a,b) ++ STREQ(a,b) + STRNEQ(a,b)
For case insensitive equality:
-- STRCASEEQ(a,b) - STRCASENEQ(a,b) ++ STRCASEEQ(a,b) + STRCASENEQ(a,b)
For strict equality of a substring:
- -- STREQLEN(a,b,n) - STRNEQLEN(a,b,n) ++ STREQLEN(a,b,n) + STRNEQLEN(a,b,n)
For case insensitive equality of a substring:
- -- STRCASEEQLEN(a,b,n) - STRCASENEQLEN(a,b,n) ++ STRCASEEQLEN(a,b,n) + STRCASENEQLEN(a,b,n)
For strict equality of a prefix:
- -- STRPREFIX(a,b) ++ STRPREFIX(a,b)
virStrncpy(char *dest, const char *src, size_t n, size_t destbytes)+ +
+ virStrncpy(char *dest, const char *src, size_t n, size_t destbytes) +
The first three arguments have the same meaning as for strncpy; namely the destination, source, and number of bytes to copy, @@ -481,8 +504,9 @@ trailing \0 is appended.
-virStrcpy(char *dest, const char *src, size_t destbytes)- +
+ virStrcpy(char *dest, const char *src, size_t destbytes) +
Use this variant if you know you want to copy the entire src string into dest. Note that this is a macro, so arguments could @@ -490,11 +514,12 @@ virStrncpy(dest, src, strlen(src), destbytes)
-virStrcpyStatic(char *dest, const char *src)- +
+ virStrcpyStatic(char *dest, const char *src) +
Use this variant if you know you want to copy the entire src - string into dest *and* you know that your destination string is + string into dest and you know that your destination string is a static string (i.e. that sizeof(dest) returns something meaningful). Note that this is a macro, so arguments could be evaluated more than once. This is equivalent to @@ -511,9 +536,10 @@
eg typical usage is as follows:
-+char * - somefunction(...) { + somefunction(...) + { virBuffer buf = VIR_BUFFER_INITIALIZER; ... @@ -545,7 +571,7 @@ *.c source files: -+/* * Copyright notice * .... @@ -561,7 +587,7 @@ #include <limits.h> #if HAVE_NUMACTL Some system includes aren't supported - # include <numa.h> everywhere so need these #if guards. + # include <numa.h> everywhere so need these #if guards. #endif #include "internal.h" Include this first, after system includes. @@ -569,13 +595,14 @@ #include "util.h" Any libvirt internal header files. #include "buf.h" - static myInternalFunc () The actual code. + static int + myInternalFunc() The actual code. { - ... + ...- Of particular note: *DO NOT* include libvirt/libvirt.h or + Of particular note: Do not include libvirt/libvirt.h or libvirt/virterror.h. It is included by "internal.h" already and there are some special reasons why you cannot include these files explicitly. @@ -591,9 +618,9 @@ the one for virAsprintf, in util.h:
-- int virAsprintf(char **strp, const char *fmt, ...) - ATTRIBUTE_FORMAT(printf, 2, 3); ++ int virAsprintf(char **strp, const char *fmt, ...) + ATTRIBUTE_FORMAT(printf, 2, 3);@@ -654,7 +681,7 @@ Although libvirt does not encourage the Linux kernel wind/unwind style of multiple labels, there's a good general discussion of the issue archived at - KernelTrap + KernelTrap
@@ -662,11 +689,12 @@ makes sense:
-+@@ -691,7 +719,7 @@ configure witherror: A path only taken upon return with an error code cleanup: A path taken upon return with success code + optional error no_memory: A path only taken upon return with an OOM error code - retry: If needing to jump upwards (eg retry on EINTR)+ retry: If needing to jump upwards (eg retry on EINTR) +- --enable-compile-warnings=error + --enable-compile-warnings=errorwhich adds -Werror to compile flags, so no warnings get missed -- GitLab