Post patches in unified diff format. A command similar to this should work:
-- diff -urp libvirt.orig/ libvirt.modified/ > libvirt-myfeature.patch+
+ diff -urp libvirt.orig/ libvirt.modified/ > libvirt-myfeature.patch +
or:
-- git diff > libvirt-myfeature.patch+
+ git diff > libvirt-myfeature.patch +
Run the automated tests on your code before submitting any changes. In particular, configure with compile warnings set to -Werror:
-- ./configure --enable-compile-warnings=error+
+ ./configure --enable-compile-warnings=error +
and run the tests:
-+make check make syntax-check - make -C tests valgrind+ make -C tests valgrind +
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: +
-++VIR_TEST_DEBUG=1 make check (or) - VIR_TEST_DEBUG=2 make check-- Also, individual tests can be run from inside the 'tests/' - directory, like: -
-- ./qemuxml2xmltest+ VIR_TEST_DEBUG=2 make check +
+ Also, individual tests can be run from inside the 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 expand -i, since some leading TABs can get through.
Usually they're in macro definitions or strings, and should be converted
anyhow.
@@ -125,18 +130,20 @@
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 the else 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;
+ 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
+ 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:
-
+
error: 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)
+
@@ -691,7 +719,7 @@
configure with
- --enable-compile-warnings=error
+ --enable-compile-warnings=error
which adds -Werror to compile flags, so no warnings get missed