* Added a section on the appropriate and inappropriate uses of goto to the HACKING document and website. --- HACKING | 32 ++++++++++++++++++++++++++++++++ docs/hacking.html.in | 41 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 0 deletions(-) diff --git a/HACKING b/HACKING index be8725d..db99630 100644 --- a/HACKING +++ b/HACKING @@ -362,6 +362,38 @@ their jobs and cross-check format strings with the number and types of arguments. +Use of goto +=========== + +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. + +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 funcion. 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. + +There are a couple of signs that a particular use of goto is not ok. +The first is the use of multiple labels. If you find yourself using +multiple labels, you're strongly encouraged to rework your code to +eliminate all but one of them. The second is jumping back up, 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. + +Although libvirt does not encourage the Linux kernel wind/unwind style +of multiple labels, there's a good general discussion of the issue at: + +http://kerneltrap.org/node/553/2131 + Libvirt commiters guidelines ============================ diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 788a49b..e10d2dd 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -389,6 +389,47 @@ 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. + </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 funcion. 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. The first is the use of multiple labels. If you find + yourself using multiple labels, you're strongly encouraged to + rework your code to eliminate all but one of them. The second + is jumping back up, 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. + </p> + + <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> <h2><a name="committers">Libvirt commiters guidelines</a></h2> -- 1.6.5.5

On Fri, Mar 05, 2010 at 12:25:19PM -0500, David Allan wrote: Thanks, it would be good if we documented a consistent naming scheme for labels too. 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) There are plenty of violations of these 4 names, but they are the most common, so we might want to convert others to use these names, or avoid goto if it wasn't appropriate. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

* docs/hacking.html.in (committers): Fix spelling and grammar. --- Per the very text I'm editing, this counts as trivial :) docs/hacking.html.in | 26 +++++++++++++++----------- 1 files changed, 15 insertions(+), 11 deletions(-) diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 2016b28..15029ac 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -514,38 +514,42 @@ - <h2><a name="committers">Libvirt commiters guidelines</a></h2> + <h2><a name="committers">Libvirt committers guidelines</a></h2> <p> - The AUTHORS files indicates the list of people with commit acces right + The AUTHORS files indicates the list of people with commit access right who can actually merge the patches. </p> <p> - The general rule for commiting patches is to make sure it has been reviewed + The general rule for committing 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 + main maintainer, or where you donot have a very clear idea of + how things work, it's better + to wait for a more authoritative feedback though. Before committing, please + also rebuild locally, run 'make check syntax-check', and make sure you + don't raise errors. Try to look for warnings too; for example, + configure with +<pre> --enable-compile-warnings=error +</pre> which adds -Werror to compile flags, so no warnings get missed </p> <p> - Exceptions to that 'review and approval on the list first' is fixing failures + An exception to 'review and approval on the list first' is fixing failures to build: </p> <ul> - <li>if a recently commited patch breaks compilation on a platform - or for a given driver then it's fine to commit a minimal fix + <li>if a recently committed 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</li> <li>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 + trivial), and 'make check syntax-check' should pass too, before committing anything</li> <li> fixes for documentation and code comments can be managed -- 1.6.6.1

* doc/hacking.html.in (preprocessor): New section to document recently-discussed style issues. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- This patch assumes that we are willing to mass-update preprocessor indentation via Jim's cppi tool. HACKING | 14 ++++++++++++++ docs/hacking.html.in | 21 ++++++++++++++++++++- 2 files changed, 34 insertions(+), 1 deletions(-) diff --git a/HACKING b/HACKING index be8725d..4bc7b52 100644 --- a/HACKING +++ b/HACKING @@ -102,6 +102,20 @@ Usually they're in macro definitions or strings, and should be converted anyhow. +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 ======= Use the right type. diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 15029ac..8771c54 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -124,6 +124,25 @@ </p> + <h2><a href="types">Preprocessor</a></h2> + + <p> + For variadic macros, stick with C99 syntax: + <pre> + #define vshPrint(_ctl, ...) fprintf(stdout, __VA_ARGS__) + </pre> + </p> + + <p>Use parenthesis when checking if a macro is defined, and use + indentation to track nesting: + + <pre> + #if defined(HAVE_POSIX_FALLOCATE) &amp;&amp; !defined(HAVE_FALLOCATE) + # define fallocate(a,ignored,b,c) posix_fallocate(a,b,c) + #endif + </pre> + </p> + <h2><a href="types">C types</a></h2> <p> @@ -405,7 +424,7 @@ #include &lt;limits.h&gt; #if HAVE_NUMACTL Some system includes aren't supported - #include &lt;numa.h&gt; everywhere so need these #if defences. + # include &lt;numa.h&gt; everywhere so need these #if defences. #endif #include "internal.h" Include this first, after system includes. -- 1.6.6.1