It doesn't make sense to track both the source and the generated file in the repository, so drop the latter. --- HACKING | 1008 --------------------------------------------------------------- 1 file changed, 1008 deletions(-) delete mode 100644 HACKING diff --git a/HACKING b/HACKING deleted file mode 100644 index e308568..0000000 --- a/HACKING +++ /dev/null @@ -1,1008 +0,0 @@ --*- buffer-read-only: t -*- vi: set ro: -DO NOT EDIT THIS FILE! IT IS GENERATED AUTOMATICALLY -from docs/hacking.html.in! - - - - 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) Official upstream repository is kept in git ("git://libvirt.org/libvirt.git") -and is browsable along with other libvirt-related repositories (e.g. -libvirt-python) online <http://libvirt.org/git/>;. - -(3) Patches to translations are maintained via the zanata project -<https://fedora.zanata.org/>. If you want to fix a translation in a .po file, -join the appropriate language team. The libvirt release process automatically -pulls the latest version of each translation file from zanata. - -(4) Post patches in unified diff format, with git rename detection enabled. You -need a one-time setup of: - - git config diff.renames true - -After that, a command similar to this should work: - - diff -urp libvirt.orig/ libvirt.modified/ > libvirt-myfeature.patch - -or: - - git diff > libvirt-myfeature.patch - -Also, for code motion patches, you may find that "git diff --patience" -provides an easier-to-read patch. However, the usual workflow of libvirt -developer is: - - git checkout master - git pull - git checkout -t origin -b workbranch - Hack, committing any changes along the way - -More hints on compiling can be found here <compiling.html>. When you want to -post your patches: - - git pull --rebase - (fix any conflicts) - git send-email --cover-letter --no-chain-reply-to --annotate \ - --to=libvir-list(a)redhat.com master - -(Note that the "git send-email" subcommand may not be in the main git package -and using it may require installation of a separate package, for example the -"git-email" package in Fedora.) For a single patch you can omit -"--cover-letter", but a series of two or more patches needs a cover letter. If -you get tired of typing &quot;--to=libvir-list(a)redhat.com&quot; designation you can set -it in git config: - - git config sendemail.to libvir-list(a)redhat.com - -Please follow this as close as you can, especially the rebase and git -send-email part, as it makes life easier for other developers to review your -patch set. One should avoid sending patches as attachments, but rather send -them in email body along with commit message. If a developer is sending -another version of the patch (e.g. to address review comments), they are -advised to note differences to previous versions after the "---" line in the -patch so that it helps reviewers but doesn't become part of git history. -Moreover, such patch needs to be prefixed correctly with -"--subject-prefix=PATCHv2" appended to "git send-email" (substitute "v2" with -the correct version if needed though). - - - -(5) In your commit message, make the summary line reasonably short (60 characters -is typical), followed by a blank line, followed by any longer description of -why your patch makes sense. If the patch fixes a regression, and you know what -commit introduced the problem, mentioning that is useful. If the patch -resolves a bugzilla report, mentioning the URL of the bug number is useful; -but also summarize the issue rather than making all readers follow the link. -You can use 'git shortlog -30' to get an idea of typical summary lines. -Libvirt does not currently attach any meaning to Signed-off-by: lines, so it -is up to you if you want to include or omit them in the commit message. - - - -(6) 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. Moreover, please keep in mind that it's -required to be able to compile cleanly (*including* "make check" and "make -syntax-check") after each patch. A feature does not have to work until the end -of a series, but intermediate patches must compile and not cause test-suite -failures (this is to preserve the usefulness of "git bisect", among other -things). - - - -(7) Make sure your patches apply against libvirt GIT. Developers only follow GIT -and don't care much about released versions. - -(8) Run the automated tests on your code before submitting any changes. In -particular, configure with compile warnings set to -Werror. This is done -automatically for a git checkout; from a tarball, use: - - ./configure --enable-werror - -and run the tests: - - make check - make syntax-check - make -C tests valgrind - -Valgrind <http://valgrind.org/> is a test that checks for memory management -issues, such as leaks or use of uninitialized variables. - -Some tests are skipped by default in a development environment, based on the -time they take in comparison to the likelihood that those tests will turn up -problems during incremental builds. These tests default to being run when -building from a tarball or with the configure option --enable-expensive-tests; -you can also force a one-time toggle of these tests by setting -VIR_TEST_EXPENSIVE to 0 or 1 at make time, as in: - - make check VIR_TEST_EXPENSIVE=1 - -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 - -When debugging failures during development, it is possible to focus in on just -the failing subtests by using TESTS and VIR_TEST_RANGE: - - make check VIR_TEST_DEBUG=1 VIR_TEST_RANGE=3-5 TESTS=qemuxml2argvtest - -Also, individual tests can be run from inside the "tests/" directory, like: - - ./qemuxml2xmltest - -If you are adding new test cases, or making changes that alter existing test -output, you can use the environment variable VIR_TEST_REGENERATE_OUTPUT to -quickly update the saved test data. Of course you still need to review the -changes VERY CAREFULLY to ensure they are correct. - - VIR_TEST_REGENERATE_OUTPUT=1 ./qemuxml2argvtest - -There is also a "./run" script at the top level, to make it easier to run -programs that have not yet been installed, as well as to wrap invocations of -various tests under gdb or Valgrind. - - - -(9) The Valgrind test should produce similar output to "make check". If the output -has traces within libvirt API's, then investigation is required in order to -determine the cause of the issue. Output such as the following indicates some -sort of leak: - -==5414== 4 bytes in 1 blocks are definitely lost in loss record 3 of 89 -==5414== at 0x4A0881C: malloc (vg_replace_malloc.c:270) -==5414== by 0x34DE0AAB85: xmlStrndup (in /usr/lib64/libxml2.so.2.7.8) -==5414== by 0x4CC97A6: virDomainVideoDefParseXML (domain_conf.c:7410) -==5414== by 0x4CD581D: virDomainDefParseXML (domain_conf.c:10188) -==5414== by 0x4CD8C73: virDomainDefParseNode (domain_conf.c:10640) -==5414== by 0x4CD8DDB: virDomainDefParse (domain_conf.c:10590) -==5414== by 0x41CB1D: testCompareXMLToArgvHelper (qemuxml2argvtest.c:100) -==5414== by 0x41E20F: virtTestRun (testutils.c:161) -==5414== by 0x41C7CB: mymain (qemuxml2argvtest.c:866) -==5414== by 0x41E84A: virtTestMain (testutils.c:723) -==5414== by 0x34D9021734: (below main) (in /usr/lib64/libc-2.15.so) - -In this example, the "virDomainDefParseXML()" had an error path where the -"virDomainVideoDefPtr video" pointer was not properly disposed. By simply -adding a "virDomainVideoDefFree(video);" in the error path, the issue was -resolved. - -Another common mistake is calling a printing function, such as "VIR_DEBUG()" -without initializing a variable to be printed. The following example involved -a call which could return an error, but not set variables passed by reference -to the call. The solution was to initialize the variables prior to the call. - -==4749== Use of uninitialised value of size 8 -==4749== at 0x34D904650B: _itoa_word (in /usr/lib64/libc-2.15.so) -==4749== by 0x34D9049118: vfprintf (in /usr/lib64/libc-2.15.so) -==4749== by 0x34D9108F60: __vasprintf_chk (in /usr/lib64/libc-2.15.so) -==4749== by 0x4CAEEF7: virVasprintf (stdio2.h:199) -==4749== by 0x4C8A55E: virLogVMessage (virlog.c:814) -==4749== by 0x4C8AA96: virLogMessage (virlog.c:751) -==4749== by 0x4DA0056: virNetTLSContextCheckCertKeyUsage (virnettlscontext.c:225) -==4749== by 0x4DA06DB: virNetTLSContextCheckCert (virnettlscontext.c:439) -==4749== by 0x4DA1620: virNetTLSContextNew (virnettlscontext.c:562) -==4749== by 0x4DA26FC: virNetTLSContextNewServer (virnettlscontext.c:927) -==4749== by 0x409C39: testTLSContextInit (virnettlscontexttest.c:467) -==4749== by 0x40AB8F: virtTestRun (testutils.c:161) - -Valgrind will also find some false positives or code paths which cannot be -resolved by making changes to the libvirt code. For these paths, it is -possible to add a filter to avoid the errors. For example: - -==4643== 7 bytes in 1 blocks are possibly lost in loss record 4 of 20 -==4643== at 0x4A0881C: malloc (vg_replace_malloc.c:270) -==4643== by 0x34D90853F1: strdup (in /usr/lib64/libc-2.15.so) -==4643== by 0x34EEC2C08A: ??? (in /usr/lib64/libnl.so.1.1) -==4643== by 0x34EEC15B81: ??? (in /usr/lib64/libnl.so.1.1) -==4643== by 0x34D8C0EE15: call_init.part.0 (in /usr/lib64/ld-2.15.so) -==4643== by 0x34D8C0EECF: _dl_init (in /usr/lib64/ld-2.15.so) -==4643== by 0x34D8C01569: ??? (in /usr/lib64/ld-2.15.so) - - -In this instance, it is acceptable to modify the "tests/.valgrind.supp" file -in order to add a suppression filter. The filter should be unique enough to -not suppress real leaks, but it should be generic enough to cover multiple -code paths. The format of the entry can be found in the documentation found at -the Valgrind home page <http://valgrind.org/>;. The following trace was added -to "tests/.valgrind.supp" in order to suppress the warning: - -{ - dlInitMemoryLeak1 - Memcheck:Leak - fun:?alloc - ... - fun:call_init.part.0 - fun:_dl_init - ... - obj:*/lib*/ld-2.*so* -} - - - -(10) 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 Richard Jones' guide to working with open source projects -<http://people.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, the project includes a file .dir-locals.el that sets up the -preferred indentation. If you use vim, append the following to your ~/.vimrc -file: - - set nocompatible - filetype on - set autoindent - set smartindent - set cindent - set tabstop=8 - set shiftwidth=4 - set expandtab - set cinoptions=(0,:0,l1,t0,L3 - filetype plugin indent on - au FileType make setlocal noexpandtab - au BufRead,BufNewFile *.am setlocal noexpandtab - match ErrorMsg /\s\+$\| \+\ze\t/ - -Or if you don't want to mess your ~/.vimrc up, you can save the above into a -file called .lvimrc (not .vimrc) located at the root of libvirt source, then -install a vim script from -http://www.vim.org/scripts/script.php?script_id=1408, which will load the -.lvimrc only when you edit libvirt code. - - -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 post-process 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. - -Libvirt requires a C99 compiler for various reasons. However, most of the code -base prefers to stick to C89 syntax unless there is a compelling reason -otherwise. For example, it is preferable to use "/* */" comments rather than -"//". Also, when declaring local variables, the prevailing style has been to -declare them at the beginning of a scope, rather than immediately before use. - - -Bracket spacing -=============== -The keywords "if", "for", "while", and "switch" must have a single space -following them before the opening bracket. E.g. - - if(foo) // Bad - if (foo) // Good - -Function implementations mustnothave any whitespace between the function name and the opening bracket. E.g. - - int foo (int wizz) // Bad - int foo(int wizz) // Good - -Function calls mustnothave any whitespace between the function name and the opening bracket. E.g. - - bar = foo (wizz); // Bad - bar = foo(wizz); // Good - -Function typedefs mustnothave any whitespace between the closing bracket of the function name and -opening bracket of the arg list. E.g. - - typedef int (*foo) (int wizz); // Bad - typedef int (*foo)(int wizz); // Good - -There must not be any whitespace immediately following any opening bracket, or -immediately prior to any closing bracket. E.g. - - int foo( int wizz ); // Bad - int foo(int wizz); // Good - - -Commas -====== -Commas should always be followed by a space or end of line, and never have -leading space; this is enforced during 'make syntax-check'. - - call(a,b ,c);// Bad - call(a, b, c); // Good - -When declaring an enum or using a struct initializer that occupies more than -one line, use a trailing comma. That way, future edits to extend the list only -have to add a line, rather than modify an existing line to add the -intermediate comma. Any sentinel enumerator value with a name ending in _LAST -is exempt, since you would extend such an enum before the _LAST element. -Another reason to favor trailing commas is that it requires less effort to -produce via code generators. Note that the syntax checker is unable to enforce -a style of trailing commas, so there are counterexamples in existing code -which do not use it; also, while C99 allows trailing commas, remember that -JSON and XDR do not. - - enum { - VALUE_ONE, - VALUE_TWO // Bad - }; - enum { - VALUE_THREE, - VALUE_FOUR, // Good - }; - - -Semicolons -========== -Semicolons should never have a space beforehand. Inside the condition of a -"for" loop, there should always be a space or line break after each semicolon, -except for the special case of an infinite loop (although more infinite loops -use "while"). While not enforced, loop counters generally use post-increment. - - for (i = 0 ;i < limit ; ++i) { // Bad - for (i = 0; i < limit; i++) { // Good - for (;;) { // ok - while (1) { // Better - -Empty loop bodies are better represented with curly braces and a comment, -although use of a semicolon is not currently rejected. - - while ((rc = waitpid(pid, &st, 0) == -1) && - errno == EINTR); // ok - while ((rc = waitpid(pid, &st, 0) == -1) && - errno == EINTR) { // Better - /* nothing */ - } - - -Curly braces -============ -Omit the curly braces around an "if", "while", "for" etc. body only when both -that body and the condition itself occupy 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. - - while (expr) // single line body; {} is forbidden - single_line_stmt(); - - while (expr(arg1, - arg2)) // indentation makes it obvious it is single line, - single_line_stmt(); // {} is optional (not enforced either way) - - while (expr1 && - expr2) { // multi-line, at same indentation, {} required - single_line_stmt(); - } - -However, the moment your loop/if/else body extends on to a second line, for -whatever reason (even if it's just an added comment), then you should add -braces. Otherwise, it would be too easy to insert a statement just before that -comment (without adding braces), thinking it is already a multi-statement loop: - - while (true) // BAD! multi-line body with no braces - /* comment... */ - single_line_stmt(); - -Do this instead: - - while (true) { // Always put braces around a multi-line body. - /* comment... */ - 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")); - -It is safe to omit the braces in the code above, since the further-indented -second body line makes it obvious that this is still a single-statement body. - -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 "if" or "else" block, and the counterpart block *does* use braces. In that -case, put braces around both blocks. Also, if the "else" block is much shorter -than the "if" block, consider negating the "if"-condition and swapping the -bodies, putting the short block first and making the longer, multi-line block -be the "else" block. - - if (expr) { - ... - ... - } - else - x = y; // BAD: braceless "else" with braced "then", - // and short block last - - if (expr) - x = y; // BAD: braceless "if" with braced "else" - else { - ... - ... - } - -Keeping braces consistent and putting the short block first is preferred, -especially when the multi-line body is more than a few lines long, because it -is easier to read and grasp the semantics of an if-then-else block when the -simpler block occurs first, rather than after the more involved block: - - if (!expr) { - x = y; // putting the smaller block first is more readable - } else { - ... - ... - } - -But if negating a complex condition is too ugly, then at least add braces: - - if (complex expr not worth negating) { - ... - ... - } else { - x = y; - } - -Use hanging braces for compound statements: the opening brace of a compound -statement should be on the same line as the condition being tested. Only -top-level function bodies, nested scopes, and compound structure declarations -should ever have { on a line by itself. - - void - foo(int a, int b) - { // correct - function body - int 2d[][] = { - { // correct - complex initialization - 1, 2, - }, - }; - if (a) - { // BAD: compound brace on its own line - do_stuff(); - } - { // correct - nested scope - int tmp; - if (a < b) { // correct - hanging brace - tmp = b; - b = a; - a = tmp; - } - } - } - - -Preprocessor -============ -Macros defined with an ALL_CAPS name should generally be assumed to be unsafe -with regards to arguments with side-effects (that is, MAX(a++, b--) might -increment a or decrement b too many or too few times). Exceptions to this rule -are explicitly documented for macros in viralloc.h and virstring.h. - -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. - -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 viralloc.h. - -- To allocate a single object: - - virDomainPtr domain; - - if (VIR_ALLOC(domain) < 0) - return NULL; - - - -- To allocate an array of objects: - - virDomainPtr domains; - size_t ndomains = 10; - - if (VIR_ALLOC_N(domains, ndomains) < 0) - return NULL; - - - -- To allocate an array of object pointers: - - virDomainPtr *domains; - size_t ndomains = 10; - - if (VIR_ALLOC_N(domains, ndomains) < 0) - return NULL; - - - -- To re-allocate the array of domains to be 1 element longer (however, note that -repeatedly expanding an array by 1 scales quadratically, so this is -recommended only for smaller arrays): - - virDomainPtr domains; - size_t ndomains = 0; - - if (VIR_EXPAND_N(domains, ndomains, 1) < 0) - return NULL; - domains[ndomains - 1] = domain; - - - -- To ensure an array has room to hold at least one more element (this approach -scales better, but requires tracking allocation separately from usage) - - virDomainPtr domains; - size_t ndomains = 0; - size_t ndomains_max = 0; - - if (VIR_RESIZE_N(domains, ndomains_max, ndomains, 1) < 0) - return NULL; - domains[ndomains++] = domain; - - - -- To trim an array of domains from its allocated size down to the actual used -size: - - virDomainPtr domains; - size_t ndomains = x; - size_t ndomains_max = y; - - VIR_SHRINK_N(domains, ndomains_max, ndomains_max - ndomains); - - - -- To free an array of domains: - - virDomainPtr domains; - size_t ndomains = x; - size_t ndomains_max = y; - size_t i; - - for (i = 0; i < ndomains; i++) - VIR_FREE(domains[i]); - VIR_FREE(domains); - ndomains_max = ndomains = 0; - - - - - - -File handling -============= -Usage of the "fdopen()", "close()", "fclose()" APIs is deprecated in libvirt -code base to help avoiding double-closing of files or file descriptors, which -is particularly dangerous in a multi-threaded application. Instead of these -APIs, use the macros from virfile.h - -- Open a file from a file descriptor: - - if ((file = VIR_FDOPEN(fd, "r")) == NULL) { - virReportSystemError(errno, "%s", - _("failed to open file from file descriptor")); - return -1; - } - /* fd is now invalid; only access the file using file variable */ - - - -- Close a file descriptor: - - if (VIR_CLOSE(fd) < 0) { - virReportSystemError(errno, "%s", _("failed to close file")); - } - - - -- Close a file: - - if (VIR_FCLOSE(file) < 0) { - virReportSystemError(errno, "%s", _("failed to close file")); - } - - - -- Close a file or file descriptor in an error path, without losing the previous -"errno" value: - - VIR_FORCE_CLOSE(fd); - VIR_FORCE_FCLOSE(file); - - - - - - -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) - - - -- To avoid having to check if a or b are NULL: - - STREQ_NULLABLE(a, b) - STRNEQ_NULLABLE(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)). - - VIR_STRDUP(char *dst, const char *src); - VIR_STRNDUP(char *dst, const char *src, size_t n); - -You should avoid using strdup or strndup directly as they do not report -out-of-memory error, and do not allow a NULL source. Use VIR_STRDUP or -VIR_STRNDUP macros instead, which return 0 for NULL source, 1 for successful -copy, and -1 for allocation failure with the error already reported. In very -specific cases, when you don't want to report the out-of-memory error, you can -use VIR_STRDUP_QUIET or VIR_STRNDUP_QUIET, but such usage is very rare and -usually considered a flaw. - - -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 virbuffer.h - -Typical usage is as follows: - - char * - somefunction(...) - { - virBuffer buf = VIR_BUFFER_INITIALIZER; - - ... - - virBufferAddLit(&buf, "<domain>\n"); - virBufferAsprintf(&buf, " <memory>%d</memory>\n", memory); - ... - virBufferAddLit(&buf, "</domain>\n"); - - ... - - if (virBufferCheckError(&buf) < 0) - 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 WITH_NUMACTL Some system includes aren't supported - # include <numa.h> everywhere so need these #if guards. - #endif - - #include "internal.h" Include this first, after system includes. - - #include "util.h" Any libvirt internal header files. - #include "buf.h" - - static int - myInternalFunc() The actual code. - { - ... - -Of particular note: *Do not* include libvirt/libvirt.h, libvirt/virterror.h, -libvirt/libvirt-qemu.h, or libvirt/libvirt-lxc.h. They are included by -"internal.h" already and there are some special reasons why you cannot include -these files explicitly. One of the special cases, "libvirt/libvirt.h" is -included prior to "internal.h" in "remote_protocol.x", to avoid exposing -*_LAST enum elements. - - -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_FORMAT(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. - -When printing to a string, consider using virBuffer for incremental -allocations, virAsprintf for a one-shot allocation, and snprintf for -fixed-width buffers. Do not use sprintf, even if you can prove the buffer -won't overflow, since gnulib does not provide the same portability guarantees -for sprintf as it does for snprintf. - - -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. In general, if goto is used for error -recovery, it's likely to be ok, otherwise, be cautious or avoid it all -together. - -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. - -There are a couple of signs that a particular use of goto is not ok: - -- 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. - -- 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. - -- 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. - - - -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 <http://kerneltrap.org/node/553/2131> - -When using goto, please use one of these standard labels if it 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 (e.g., retry on EINTR) - -Top-level labels should be indented by one space (putting them on the -beginning of the line confuses function context detection in git): - -int foo() -{ - /* ... do stuff ... */ - cleanup: - /* ... do other stuff ... */ -} - - -Libvirt committer guidelines -============================ -The AUTHORS files indicates the list of people with commit access right who -can actually merge the patches. - -The general rule for committing a patch is to make sure it has been reviewed -properly in the mailing-list first, usually if a couple of people 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 where you do not 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 - - --enable-compile-warnings=error - -which adds -Werror to compile flags, so no warnings get missed - -An exception to 'review and approval on the list first' is fixing failures to -build: - -- 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 - -- 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 committing anything - -- fixes for documentation and code comments can be managed in the same way, but -still make sure they get reviewed if non-trivial. -- 2.4.3

This ensures it will be built as part of the release process and included in the distribution archive. Don't generate it during syntax-check. --- Makefile.am | 1 + cfg.mk | 3 +-- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile.am b/Makefile.am index 47865f7..8f928f4 100644 --- a/Makefile.am +++ b/Makefile.am @@ -47,6 +47,7 @@ EXTRA_DIST = \ cfg.mk \ run.in \ AUTHORS.in \ + HACKING \ $(XML_EXAMPLES) pkgconfigdir = $(libdir)/pkgconfig diff --git a/cfg.mk b/cfg.mk index 93dadcd..a56272e 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1066,9 +1066,8 @@ _autogen: $(srcdir)/autogen.sh ./config.status -# regenerate HACKING as part of the syntax-check ifneq ($(_gl-Makefile),) -syntax-check: HACKING bracket-spacing-check +syntax-check: bracket-spacing-check endif bracket-spacing-check: -- 2.4.3