diff options
| author |   Daniel P. Berrange <berrange@redhat.com> | 2009-06-29 11:09:17 +0000 |
|---|---|---|
| committer | Daniel P. Berrange <berrange@redhat.com> | 2009-06-29 11:09:17 +0000 |
| commit | 2c359dd609186b56bdaeb04d831b70992dcd6e3b (patch) | |
| tree | 11d29180b6d9fcb8697f4197c450b05603c8652c /docs/hacking.html.in | |
| parent | Fix crash in QEMU driver with bad capabilities data (diff) | |
| download | libvirt-2c359dd609186b56bdaeb04d831b70992dcd6e3b.tar.gz libvirt-2c359dd609186b56bdaeb04d831b70992dcd6e3b.tar.bz2 libvirt-2c359dd609186b56bdaeb04d831b70992dcd6e3b.zip | |
Add HACKING doc to the website
Diffstat (limited to 'docs/hacking.html.in')
| -rw-r--r-- | docs/hacking.html.in | 432 |
1 files changed, 432 insertions, 0 deletions
diff --git a/docs/hacking.html.in b/docs/hacking.html.in new file mode 100644 index 000000000..bc2f8f0a4 --- /dev/null +++ b/docs/hacking.html.in @@ -0,0 +1,432 @@ +<html> + <body> + <h1>Contributor guidelines</h1> + + <ul id="toc"></ul> + + <h2><a name="patches">General tips for contributing patches</a></h2> + + <ol> + <li>Discuss any large changes on the mailing list first. Post patches + early and listen to feedback.</li> + + <li><p>Post patches in unified diff format. A command similar to this + should work:</p> + <pre> + diff -urp libvirt.orig/ libvirt.modified/ > libvirt-myfeature.patch +</pre> + <p> + or: + </p> + <pre> + cvs diff -up > libvirt-myfeature.patch +</pre></li> + <li>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.</li> + <li>Make sure your patches apply against libvirt CVS. Developers + only follow CVS and don't care much about released versions.</li> + <li><p>Run the automated tests on your code before submitting any changes. + In particular, configure with compile warnings set to -Werror:</p> + <pre> + ./configure --enable-compile-warnings=error +</pre> + <p> + and run the tests: + </p> + <pre> + make check + make syntax-check + make -C tests valgrind +</pre> + <p> + The latter test checks for memory leaks. + </p> + + <li>Update tests and/or documentation, particularly if you are adding + a new feature or changing the output of a program.</li> + </ol> + + <p> + There is more on this subject, including lots of links to background + reading on the subject, on + <a href="http://et.redhat.com/~rjones/how-to-supply-code-to-open-source-projects/"> + Richard Jones' guide to working with open source projects</a> + </p> + + + <h2><a name="indent">Code indentation</a></h2> + <p> + 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. + </p> + <p> + 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: + </p> + <pre> + ;;; 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)))) +</pre> + + <h2><a name="formatting">Code formatting (especially for new code)</a></h2> + + <p> + 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: + </p> + + <pre> + 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 "$@" + } +</pre> + + <p> + 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. + </p> + + + <h2><a href="types">C types</a></h2> + + <p> + Use the right type. + </p> + + <h3>Scalars</h3> + + <ul> + <li>If you're using "int" or "long", odds are good that there's a better type.</li> + <li>If a variable is counting something, be sure to declare it with an + unsigned type.</li> + <li>If it's memory-size-related, use size_t (use ssize_t only if required).</li> + <li>If it's file-size related, use uintmax_t, or maybe off_t.</li> + <li>If it's file-offset related (i.e., signed), use off_t.</li> + <li>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).</li> + <li>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.</li> + <li>In the unusual event that you require a specific width, use a + standard type like int32_t, uint32_t, uint64_t, etc.</li> + <li>While using "bool" is good for readability, it comes with minor caveats: + <ul> + <li>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.</li> + <li>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)...".</li> + </ul> + </li> + </ul> + + <p> + 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. + </p> + + <p> + 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. + </p> + + <p> + 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. + </p> + + <h3>Pointers</h3> + + <p> + 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. + </p> + + <h2><a name="memalloc">Low level memory management</a></h2> + + <p> + 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 + </p> + + <ul> + <li><p>eg to allocate a single object:</p> + +<pre> + virDomainPtr domain; + + if (VIR_ALLOC(domain) < 0) { + __virRaiseError(VIR_ERROR_NO_MEMORY) + return NULL; + } +</pre></li> + + <li><p>eg to allocate an array of objects</p> + +<pre> + virDomainPtr domains; + int ndomains = 10; + + if (VIR_ALLOC_N(domains, ndomains) < 0) { + __virRaiseError(VIR_ERROR_NO_MEMORY) + return NULL; + } +</pre></li> + + <li><p>eg to allocate an array of object pointers</p> + +<pre> + virDomainPtr *domains; + int ndomains = 10; + + if (VIR_ALLOC_N(domains, ndomains) < 0) { + __virRaiseError(VIR_ERROR_NO_MEMORY) + return NULL; + } +</pre></li> + + <li><p>eg to re-allocate the array of domains to be longer</p> + +<pre> + ndomains = 20 + + if (VIR_REALLOC_N(domains, ndomains) < 0) { + __virRaiseError(VIR_ERROR_NO_MEMORY) + return NULL; + } +</pre></li> + + <li><p>eg to free the domain</p> + +<pre> + VIR_FREE(domain); +</pre></li> + </ul> + + + + <h2><a name="string">String comparisons</a></h2> + + <p> + Do not use the strcmp, strncmp, etc functions directly. Instead use + one of the following semantically named macros + </p> + + <ul> + <li><p>For strict equality:</p> + <pre> + STREQ(a,b) + STRNEQ(a,b) +</pre> + </li> + + <li><p>For case sensitive equality:</p> + <pre> + STRCASEEQ(a,b) + STRCASENEQ(a,b) +</pre> + </li> + + <li><p>For strict equality of a substring:</p> + + <pre> + STREQLEN(a,b,n) + STRNEQLEN(a,b,n) +</pre> + </li> + + <li><p>For case sensitive equality of a substring:</p> + + <pre> + STRCASEEQLEN(a,b,n) + STRCASENEQLEN(a,b,n) +</pre> + </li> + + <li><p>For strict equality of a prefix:</p> + + <pre> + STRPREFIX(a,b) +</pre> + </li> + </ul> + + + <h2><a name="strbuf">Variable length string buffer</a></h2> + + <p> + 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 + </p> + + <p>eg typical usage is as follows:</p> + + <pre> + 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)) { + __virRaiseError(...); + return NULL; + } + + return virBufferContentAndReset(&buf); + } +</pre> + + + <h2><a name="includes">Include files</a></h2> + + <p> + 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: + </p> + + <pre> + /* + * 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. + { + ... +</pre> + + <p> + 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. + </p> + + + <h2><a name="printf">Printf-style functions</a></h2> + + <p> + 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: + </p> + + <pre> + int virAsprintf(char **strp, const char *fmt, ...) + ATTRIBUTE_FORMAT(printf, 2, 3); +</pre> + + <p> + 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. + </p> + + + + <h2><a name="committers">Libvirt commiters guidelines</a></h2> + + <p> + The AUTHORS files indicates the list of people with commit acces right + who can actually merge the patches. + </p> + + <p> + 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 + </p> + + <p> + Exceptions to that '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 + directly without getting the review feedback first</li> + <li>if make check or make syntax-chek 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</li> + <li> + fixes for documentation and code comments can be managed + in the same way, but still make sure they get reviewed if non-trivial. + </li> + </ul> + </body> +</html> |