[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[libvirt] [PATCH 1/1] Consolidating contributor guidelines



* 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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]