diff --git a/HACKING b/HACKING deleted file mode 100644 index 1d9f3f1625341714a3a4d393db1bdfab95055c8c..0000000000000000000000000000000000000000 --- a/HACKING +++ /dev/null @@ -1,1112 +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 . - -(3) Patches to translations are maintained via the zanata project -. 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 using "git send-email", with git rename detection enabled. You -need a one-time setup of: - - git config diff.renames true - -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 . When you want to -post your patches: - - git pull --rebase - (fix any conflicts) - git send-email --cover-letter --no-chain-reply-to --annotate \ - --confirm=always --to=libvir-list@redhat.com master - -For a single patch you can omit "--cover-letter", but a series of two or more -patches needs a cover letter. - -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 and Debian. If this is your first time using -"git send-email", you might need to configure it to point it to your SMTP -server with something like: - - git config --global sendemail.smtpServer stmp.youremailprovider.net - -If you get tired of typing "--to=libvir-list@redhat.com" all the time, you can -configure that to be automatically handled as well: - - git config sendemail.to libvir-list@redhat.com - -As a rule, patches should be sent to the mailing list only: all developers are -subscribed to libvir-list and read it regularly, so please don't CC individual -developers unless they've explicitly asked you to. - -Avoid using mail clients for sending patches, as most of them will mangle the -messages in some way, making them unusable for our purposes. Gmail and other -Web-based mail clients are particularly bad at this. - -If everything went well, your patch should show up on the libvir-list archives - in a matter of minutes; if you -still can't find it on there after an hour or so, you should double-check your -setup. Note that your very first post to the mailing list will be subject to -moderation, and it's not uncommon for that to take around a day. - -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 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. - -When running our test suite it may happen that the test result is -nondeterministic because of the test suite relying on a particular file in the -system being accessible or having some specific value. To catch this kind of -errors, the test suite has a module for that prints any path touched that -fulfils constraints described above into a file. To enable it just set -"VIR_TEST_FILE_ACCESS" environment variable. Then -"VIR_TEST_FILE_ACCESS_OUTPUT" environment variable can alter location where -the file is stored. - - VIR_TEST_FILE_ACCESS=1 VIR_TEST_FILE_ACCESS_OUTPUT="/tmp/file_access.txt" ./qemuxml2argvtest - -(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 . 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. - -(11) Don't forget to update the release notes by changing -"docs/news.xml" if your changes are significant. All user-visible changes, -such as adding new XML elements or fixing all but the most obscure bugs, must -be (briefly) described in a release notes entry; changes that are only -relevant to other libvirt developers, such as code refactoring, don't belong -in the release notes. Note that "docs/news.xml" should be updated in its own -commit not to get in the way of backports. - -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 -. - - -Tooling -======= -libvirt includes support for some useful development tools right in its source -repository, meaning users will be able to take advantage of them without -little or no configuration. Examples include: - -- color_coded , a vim plugin for -libclang-powered semantic syntax highlighting; - -- YouCompleteMe , a vim plugin for -libclang-powered semantic code completion. - - -Naming conventions -================== -When reading libvirt code, a number of different naming conventions will be -evident due to various changes in thinking over the course of the project's -lifetime. The conventions documented below should be followed when creating -any entirely new files in libvirt. When working on existing files, while it is -desirable to apply these conventions, keeping a consistent style with existing -code in that particular file is generally more important. The overall guiding -principal is that every file, enum, struct, function, macro and typedef name -must have a 'vir' or 'VIR' prefix. All local scope variable names are exempt, -and global variables are exempt, unless exported in a header file. - -*File names* - -File naming varies depending on the subdirectory. The preferred style is to -have a 'vir' prefix, followed by a name which matches the name of the -functions / objects inside the file. For example, a file containing an object -'virHashtable' is stored in files 'virhashtable.c' and 'virhashtable.h'. -Sometimes, methods which would otherwise be declared 'static' need to be -exported for use by a test suite. For this purpose a second header file should -be added with a suffix of 'priv', e.g. 'virhashtablepriv.h'. Use of -underscores in file names is discouraged when using the 'vir' prefix style. -The 'vir' prefix naming applies to src/util, src/rpc and tests/ directories. -Most other directories do not follow this convention. - - - -*Enum type & field names* - -All enums should have a 'vir' prefix in their typedef name, and each following -word should have its first letter in uppercase. The enum name should match the -typedef name with a leading underscore. The enum member names should be in all -uppercase, and use an underscore to separate each word. The enum member name -prefix should match the enum typedef name. - - typedef enum _virSocketType virSocketType; - enum _virSocketType { - VIR_SOCKET_TYPE_IPV4, - VIR_SOCKET_TYPE_IPV6, - }; - - -*Struct type names* - -All structs should have a 'vir' prefix in their typedef name, and each -following word should have its first letter in uppercase. The struct name -should be the same as the typedef name with a leading underscore. A second -typedef should be given for a pointer to the struct with a 'Ptr' suffix. - - typedef struct _virHashTable virHashTable; - typedef virHashTable *virHashTablePtr; - struct _virHashTable { - ... - }; - - -*Function names* - -All functions should have a 'vir' prefix in their name, followed by one or -more words with first letter of each word capitalized. Underscores should not -be used in function names. If the function is operating on an object, then the -function name prefix should match the object typedef name, otherwise it should -match the filename. Following this comes the verb / action name, and finally -an optional subject name. For example, given an object 'virHashTable', all -functions should have a name 'virHashTable$VERB' or -'virHashTable$VERB$SUBJECT", e.g. 'virHashTableLookup' or -'virHashTableGetValue'. - - - -*Macro names* - -All macros should have a "VIR" prefix in their name, followed by one or more -uppercase words separated by underscores. The macro argument names should be -in lowercase. Aside from having a "VIR" prefix there are no common practices -for the rest of the macro name. - - - - -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 , 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, "\n"); - virBufferAsprintf(&buf, " %d\n", memory); - ... - virBufferAddLit(&buf, "\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 Must come first in every file. - - #include Any system includes you need. - #include - #include - - #if WITH_NUMACTL Some system includes aren't supported - # include 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 - -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. diff --git a/Makefile.am b/Makefile.am index db991ba3b3b1070144d05c6bc49bee4606151f33..d042e9a31f84cf94bec8e2dc79b4c500f31adc1f 100644 --- a/Makefile.am +++ b/Makefile.am @@ -66,17 +66,6 @@ EXTRA_DIST += \ $(srcdir)/docs/news-ascii.xsl \ $(srcdir)/docs/reformat-news.py -$(top_srcdir)/HACKING: $(top_srcdir)/docs/hacking1.xsl \ - $(top_srcdir)/docs/hacking2.xsl \ - $(top_srcdir)/docs/wrapstring.xsl \ - $(top_srcdir)/docs/hacking.html.in - $(AM_V_GEN)if [ -x $(XSLTPROC) ] ; then \ - $(XSLTPROC) --nonet $(top_srcdir)/docs/hacking1.xsl \ - $(top_srcdir)/docs/hacking.html.in | \ - $(XSLTPROC) --nonet $(top_srcdir)/docs/hacking2.xsl - \ - | perl -0777 -pe 's/\n\n+$$/\n/' \ - > $@-t && mv $@-t $@ ; fi; - rpm: clean @(unset CDPATH ; $(MAKE) dist && rpmbuild -ta $(distdir).tar.xz) diff --git a/README-hacking b/README-hacking index 4e02fd854c4cdbdd40bfc2e29382342419645aae..165d6d59ccadec814b3596f44108ae56305b5894 100644 --- a/README-hacking +++ b/README-hacking @@ -2,7 +2,8 @@ These notes intend to help people working on the checked-out sources. These requirements do not apply when building from a distribution tarball. -See also HACKING for more detailed libvirt contribution guidelines. +See also docs/hacking.html (after building libvirt using the information +included in this file) for more detailed contribution guidelines. * Requirements diff --git a/README.md b/README.md index bfa8f6e94aa65b71633b0d1343d540ff4fdb079d..5fed0fd874b4c465fe740ef879921a446db71686 100644 --- a/README.md +++ b/README.md @@ -64,8 +64,7 @@ Contributing The libvirt project welcomes contributions in many ways. For most components the best way to contribute is to send patches to the primary development -mailing list. Further guidance on this can be found in the `HACKING` file -or on the website: +mailing list. Further guidance on this can be found on the website: [https://libvirt.org/contribute.html](https://libvirt.org/contribute.html) diff --git a/cfg.mk b/cfg.mk index 26d70edee05511667df429f6331312fb061750c0..56cb14bd94eccc36c3e46eb856ec063db67b9d0a 100644 --- a/cfg.mk +++ b/cfg.mk @@ -91,7 +91,7 @@ endif # Files that should never cause syntax check failures. VC_LIST_ALWAYS_EXCLUDE_REGEX = \ - (^(HACKING|docs/(news(-[0-9]*)?\.html\.in|.*\.patch))|\.(po|fig|gif|ico|png))$$ + (^(docs/(news(-[0-9]*)?\.html\.in|.*\.patch))|\.(po|fig|gif|ico|png))$$ # Functions like free() that are no-ops on NULL arguments. useless_free_options = \ @@ -910,12 +910,11 @@ sc_curly_braces_style: '^\s*(?!([a-zA-Z_]*for_?each[a-zA-Z_]*) ?\()([_a-zA-Z0-9]+( [_a-zA-Z0-9]+)* ?\()?(\*?[_a-zA-Z0-9]+(,? \*?[_a-zA-Z0-9\[\]]+)+|void)\) ?\{' \ $$files; then \ echo '$(ME): Non-K&R style used for curly braces around' \ - 'function body, see HACKING' 1>&2; exit 1; \ + 'function body' 1>&2; exit 1; \ fi; \ if $(GREP) -A1 -En ' ((if|for|while|switch) \(|(else|do)\b)[^{]*$$'\ $$files | $(GREP) '^[^ ]*- *{'; then \ - echo '$(ME): Use hanging braces for compound statements,' \ - 'see HACKING' 1>&2; exit 1; \ + echo '$(ME): Use hanging braces for compound statements' 1>&2; exit 1; \ fi sc_prohibit_windows_special_chars_in_filename: @@ -1067,9 +1066,8 @@ _autogen: _autogen_error: $(srcdir)/autogen.sh --dry-run -# regenerate HACKING as part of the syntax-check ifneq ($(_gl-Makefile),) -syntax-check: $(top_srcdir)/HACKING spacing-check test-wrap-argv \ +syntax-check: spacing-check test-wrap-argv \ prohibit-duplicate-header mock-noinline endif @@ -1081,8 +1079,7 @@ prohibit-duplicate-header: spacing-check: $(AM_V_GEN)files=`$(VC_LIST) | grep '\.c$$'`; \ $(PERL) $(top_srcdir)/build-aux/check-spacing.pl $$files || \ - { echo '$(ME): incorrect formatting, see HACKING for rules' 1>&2; \ - exit 1; } + { echo '$(ME): incorrect formatting' 1>&2; exit 1; } mock-noinline: $(AM_V_GEN)files=`$(VC_LIST) | grep '\.[ch]$$'`; \ diff --git a/docs/Makefile.am b/docs/Makefile.am index 7a10a501fb7274d57def46c0fa9cac05241a7f1b..e32758f4aa36f6b7e30f0dd84331b35cfda243e0 100644 --- a/docs/Makefile.am +++ b/docs/Makefile.am @@ -172,7 +172,7 @@ schema_DATA = $(wildcard $(srcdir)/schemas/*.rng) EXTRA_DIST= \ apibuild.py genaclperms.pl \ site.xsl subsite.xsl newapi.xsl page.xsl \ - hacking1.xsl hacking2.xsl wrapstring.xsl \ + wrapstring.xsl \ $(dot_html) $(dot_html_in) $(gif) $(apihtml) $(apipng) \ $(devhelphtml) $(devhelppng) $(devhelpcss) $(devhelpxsl) \ $(xml) $(qemu_xml) $(lxc_xml) $(admin_xml) $(fig) $(png) $(css) \ diff --git a/docs/hacking1.xsl b/docs/hacking1.xsl deleted file mode 100644 index e70b45d94d2364e0fcb07cd906bf1e5ad40f7f66..0000000000000000000000000000000000000000 --- a/docs/hacking1.xsl +++ /dev/null @@ -1,40 +0,0 @@ - - - - - - - - - - - - - - - - - - - - - - -** -'' -"" - - - - - - < - > - - - - - diff --git a/docs/hacking2.xsl b/docs/hacking2.xsl deleted file mode 100644 index 7e5ac828f32c0a65074af3b4b647a5c1b7552c41..0000000000000000000000000000000000000000 --- a/docs/hacking2.xsl +++ /dev/null @@ -1,140 +0,0 @@ - - - - - - - - - - - - - - - - - - - --*- buffer-read-only: t -*- vi: set ro: -DO NOT EDIT THIS FILE! IT IS GENERATED AUTOMATICALLY -from docs/hacking.html.in! - - - - - - - - - - - - - - - ====================== - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -() - - - - -- - - - - --- - - -** - - - - - - - - - - - - - - - - - - - - - - -