* Added a section on the appropriate and inappropriate uses of goto to the HACKING document and website. * Added a few sections that were part of HACKING but not in hacking.html * Removed contents of HACKING file and replaced them with a link to the libvirt.org hacking page. --- HACKING | 395 +------------------------------------------------- docs/hacking.html.in | 125 ++++++++++++++++- 2 files changed, 125 insertions(+), 395 deletions(-) diff --git a/HACKING b/HACKING index be8725d..e892b60 100644 --- a/HACKING +++ b/HACKING @@ -1,394 +1 @@ - Libvirt contributor guidelines - ============================== - - -General tips for contributing patches -===================================== - -(1) Discuss any large changes on the mailing list first. Post patches -early and listen to feedback. - -(2) Post patches in unified diff format. A command similar to this -should work: - - diff -urp libvirt.orig/ libvirt.modified/ > libvirt-myfeature.patch - -or: - - git diff > libvirt-myfeature.patch - -(3) Split large changes into a series of smaller patches, self-contained -if possible, with an explanation of each patch and an explanation of how -the sequence of patches fits together. - -(4) Make sure your patches apply against libvirt GIT. Developers -only follow GIT and don't care much about released versions. - -(5) 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 - -and run the tests: - - make check - make syntax-check - 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: - - 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 - -(6) Update tests and/or documentation, particularly if you are adding -a new feature or changing the output of a program. - - -There is more on this subject, including lots of links to background -reading on the subject, on this page: - - http://et.redhat.com/~rjones/how-to-supply-code-to-open-source-projects/ - - - -Code indentation -================ -Libvirt's C source code generally adheres to some basic code-formatting -conventions. The existing code base is not totally consistent on this -front, but we do prefer that contributed code be formatted similarly. -In short, use spaces-not-TABs for indentation, use 4 spaces for each -indentation level, and other than that, follow the K&R style. - -If you use Emacs, add the following to one of one of your start-up files -(e.g., ~/.emacs), to help ensure that you get indentation right: - - ;;; When editing C sources in libvirt, use this style. - (defun libvirt-c-mode () - "C mode with adjusted defaults for use with libvirt." - (interactive) - (c-set-style "K&R") - (setq indent-tabs-mode nil) ; indent using spaces, not TABs - (setq c-indent-level 4) - (setq c-basic-offset 4)) - (add-hook 'c-mode-hook - '(lambda () (if (string-match "/libvirt" (buffer-file-name)) - (libvirt-c-mode)))) - -Code formatting (especially for new code) -========================================= -With new code, we can be even more strict. -Please apply the following function (using GNU indent) to any new code. -Note that this also gives you an idea of the type of spacing we prefer -around operators and keywords: - - indent-libvirt() - { - indent -bad -bap -bbb -bli4 -br -ce -brs -cs -i4 -l75 -lc75 \ - -sbi4 -psl -saf -sai -saw -sbi4 -ss -sc -cdw -cli4 -npcs -nbc \ - --no-tabs "$@" - } - -Note that sometimes you'll have to postprocess that output further, by -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. - - -C types -======= -Use the right type. - -Scalars -------- -If you're using "int" or "long", odds are good that there's a better type. -If a variable is counting something, be sure to declare it with an -unsigned type. -If it's memory-size-related, use size_t (use ssize_t only if required). -If it's file-size related, use uintmax_t, or maybe off_t. -If it's file-offset related (i.e., signed), use off_t. -If it's just counting small numbers use "unsigned int"; -(on all but oddball embedded systems, you can assume that that -type is at least four bytes wide). -If a variable has boolean semantics, give it the "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. -In the unusual event that you require a specific width, use a -standard type like int32_t, uint32_t, uint64_t, etc. - -While using "bool" is good for readability, it comes with minor caveats: - - Don't use "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. - - Don't compare a bool variable against the literal, "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. - -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" -and fixing all related variables would be too invasive. - -Finally, while using descriptive types is important, be careful not to -go overboard. If whatever you're doing causes warnings, or requires -casts, then reconsider or ask for help. - -Pointers --------- -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 -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 -it points to, or it is aliased to another pointer that is. - - -Low level memory management -=========================== - -Use of the malloc/free/realloc/calloc APIs is deprecated in the libvirt -codebase, because they encourage a number of serious coding bugs and do -not enable compile time verification of checks for NULL. Instead of these -routines, use the macros from memory.h - - - eg to allocate a single object: - - virDomainPtr domain; - - if (VIR_ALLOC(domain) < 0) { - virReportOOMError(); - return NULL; - } - - - - eg 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 - - 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 - - ndomains = 20 - - if (VIR_REALLOC_N(domains, ndomains) < 0) { - virReportOOMError(); - return NULL; - } - - - eg to free the domain - - VIR_FREE(domain); - - - -String comparisons -================== - -Do not use the strcmp, strncmp, etc functions directly. Instead use -one of the following semantically named macros - - - For strict equality: - - STREQ(a,b) - STRNEQ(a,b) - - - For case insensitive equality: - STRCASEEQ(a,b) - STRCASENEQ(a,b) - - - For strict equality of a substring: - - STREQLEN(a,b,n) - STRNEQLEN(a,b,n) - - - For case insensitive equality of a substring: - - STRCASEEQLEN(a,b,n) - STRCASENEQLEN(a,b,n) - - - For strict equality of a prefix: - - STRPREFIX(a,b) - - - -String copying -============== - -Do not use the strncpy function. According to the man page, it does -*not* guarantee a NULL-terminated buffer, which makes it extremely dangerous -to use. Instead, use one of the functionally equivalent functions: - - - 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, respectively. The last - argument is the number of bytes available in the destination string; if a - copy of the source string (including a \0) will not fit into the - destination, no bytes are copied and the routine returns NULL. - Otherwise, n bytes from the source are copied into the destination and a - trailing \0 is appended. - - - 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 be - evaluated more than once. This is equivalent to - virStrncpy(dest, src, strlen(src), destbytes) - - - 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 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 virStrncpy(dest, src, strlen(src), sizeof(dest)). - - - -Variable length string buffer -============================= - -If there is a need for complex string concatenations, avoid using -the usual sequence of malloc/strcpy/strcat/snprintf functions and -make use of the virBuffer API described in buf.h - -eg typical usage is as follows: - - char * - somefunction(...) { - virBuffer buf = VIR_BUFFER_INITIALIZER; - - ... - - virBufferAddLit(&buf, "<domain>\n"); - virBufferVSprint(&buf, " <memory>%d</memory>\n", memory); - ... - virBufferAddLit(&buf, "</domain>\n"); - - ... - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return NULL; - } - - return virBufferContentAndReset(&buf); - } - - -Include files -============= - -There are now quite a large number of include files, both libvirt -internal and external, and system includes. To manage all this -complexity it's best to stick to the following general plan for all -*.c source files: - - /* - * Copyright notice - * .... - * .... - * .... - * - */ - - #include <config.h> Must come first in every file. - - #include <stdio.h> Any system includes you need. - #include <string.h> - #include <limits.h> - - #if HAVE_NUMACTL Some system includes aren't supported - #include <numa.h> everywhere so need these #if defences. - #endif - - #include "internal.h" Include this first, after system includes. - - #include "util.h" Any libvirt internal header files. - #include "buf.h" - - static myInternalFunc () The actual code. - { - ... - -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. - - -Printf-style functions -====================== - -Whenever you add a new printf-style function, i.e., one with a format -string argument and following "..." in its prototype, be sure to use -gcc's printf attribute directive in the prototype. For example, here's -the one for virAsprintf, in util.h: - - int virAsprintf(char **strp, const char *fmt, ...) - ATTRIBUTE_FMT_PRINTF(2, 3); - -This makes it so gcc's -Wformat and -Wformat-security options can do -their jobs and cross-check format strings with the number and types -of arguments. - - - - Libvirt commiters guidelines - ============================ - -The AUTHORS files indicates the list of people with commit acces right -who can actually merge the patches. - -The general rule for commiting patches is to make sure it has been reviewed -properly in the mailing-list first, usually if a couple of persons gave an -ACK or +1 to a patch and nobody raised an objection on the list it should -be good to go. If the patch touches a part of the code where you're not the -main maintainer or not have a very clear idea of how things work, it's better -to wait for a more authoritative feedback though. Before commiting please -also rebuild locally and run 'make check syntax-check' and make sure they -don't raise error. Try to look for warnings too for example configure with - --enable-compile-warnings=error -which adds -Werror to compile flags, so no warnings get missed - -Exceptions to that 'review and approval on the list first' is fixing failures -to build: - - if a recently commited patch breaks compilation on a platform - or for a given driver then it's fine to commit a minimal fix - directly without getting the review feedback first - - similary if make check or make syntax-check breaks, if there is - an obvious fix, it's fine to commit immediately -The patch should still be sent to the list (or tell what the fix was if -trivial) and 'make check syntax-check' should pass too before commiting -anything -Similary fixes for documentation and code comments can be managed -in the same way, but still make sure they get reviewed if non-trivial. +Please see http://libvirt.org/hacking.html for contributor guidelines. diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 788a49b..2016b28 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -43,6 +43,23 @@ The latter test checks for memory leaks. </p> + <p> + 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: + </p> + + <pre> + VIR_TEST_DEBUG=1 make check (or) + VIR_TEST_DEBUG=2 make check</pre> + <p> + Also, individual tests can be run from inside the 'tests/' + directory, like: + </p> + <pre> + ./qemuxml2xmltest</pre> + <li>Update tests and/or documentation, particularly if you are adding a new feature or changing the output of a program.</li> </ol> @@ -241,7 +258,7 @@ - <h2><a name="string">String comparisons</a></h2> + <h2><a name="string_comparision">String comparisons</a></h2> <p> Do not use the strcmp, strncmp, etc functions directly. Instead use @@ -288,6 +305,46 @@ </ul> + <h2><a name="string_copying">String copying</a></h2> + + <p> + Do not use the strncpy function. According to the man page, it + does <b>not</b> guarantee a NULL-terminated buffer, which makes + it extremely dangerous to use. Instead, use one of the + functionally equivalent functions: + </p> + <pre>virStrncpy(char *dest, const char *src, size_t n, size_t destbytes)</pre> + <p> + The first three arguments have the same meaning as for strncpy; + namely the destination, source, and number of bytes to copy, + respectively. The last argument is the number of bytes + available in the destination string; if a copy of the source + string (including a \0) will not fit into the destination, no + bytes are copied and the routine returns NULL. Otherwise, n + bytes from the source are copied into the destination and a + trailing \0 is appended. + </p> + + <pre>virStrcpy(char *dest, const char *src, size_t destbytes)</pre> + + <p> + 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 + be evaluated more than once. This is equivalent to + virStrncpy(dest, src, strlen(src), destbytes) + </p> + + <pre>virStrcpyStatic(char *dest, const char *src)</pre> + + <p> + Use this variant if you know you want to copy the entire src + 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 + virStrncpy(dest, src, strlen(src), sizeof(dest)). + </p> + <h2><a name="strbuf">Variable length string buffer</a></h2> <p> @@ -389,6 +446,72 @@ of arguments. </p> + <h2><a name="goto">Use of goto</a></h2> + + <p> + The use of goto is not forbidden, and goto is widely used + throughout libvirt. While the uncontrolled use of goto will + quickly lead to unmaintainable code, there is a place for it in + well structured code where its use increases readability and + maintainability. In general, if goto is used for error + recovery, it's likely to be ok, otherwise, be cautious or avoid + it all together. + </p> + + <p> + The typical use of goto is to jump to cleanup code in the case + of a long list of actions, any of which may fail and cause the + entire operation to fail. In this case, a function will have a + single label at the end of the function. It's almost always ok + to use this style. In particular, if the cleanup code only + involves free'ing memory, then having multiple labels is + overkill. VIR_FREE() and every function named XXXFree() in + libvirt is required to handle NULL as its arg. Thus you can + safely call free on all the variables even if they were not yet + allocated (yes they have to have been initialized to NULL). + This is much simpler and clearer than having multiple labels. + </p> + + <p> + There are a couple of signs that a particular use of goto is not + ok: + </p> + + <ul> + <li>You're using multiple labels. If you find yourself using + multiple labels, you're strongly encouraged to rework your code + to eliminate all but one of them.</li> + <li>The goto jumps back up to a point above the current line of + code being executed. Please use some combination of looping + constructs to re-execute code instead; it's almost certainly + going to be more understandable by others. One well-known + exception to this rule is restarting an i/o operation following + EINTR.</li> + <li>The goto jumps down to an arbitrary place in the middle of a + function followed by further potentially failing calls. You + should almost certainly be using a conditional and a block + instead of a goto. Perhaps some of your function's logic would + be better pulled out into a helper function.</li> + </ul> + + <p> + 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 + <a href=http://kerneltrap.org/node/553/2131>KernelTrap</a> + </p> + + <p> + When using goto, please use one of these standard labels if it + makes sense: + </p> + + <pre> + 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)</pre> + <h2><a name="committers">Libvirt commiters guidelines</a></h2> -- 1.6.5.5