The conversion has been performed by using pandoc as a first pass, and then tweaking the result manually until it looked satisfactory. Signed-off-by: Andrea Bolognani <abologna@xxxxxxxxxx> --- build-aux/syntax-check.mk | 4 +- docs/hacking.html.in | 1555 ------------------------------------- docs/hacking.rst | 1400 +++++++++++++++++++++++++++++++++ 3 files changed, 1402 insertions(+), 1557 deletions(-) delete mode 100644 docs/hacking.html.in create mode 100644 docs/hacking.rst diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index c7c938ac92..6ffea7afb9 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -2040,7 +2040,7 @@ exclude_file_name_regexp--sc_prohibit_canonicalize_file_name = \ ^(build-aux/syntax-check\.mk|tests/virfilemock\.c)$$ exclude_file_name_regexp--sc_prohibit_raw_allocation = \ - ^(docs/hacking\.html\.in|src/util/viralloc\.[ch]|examples/.*|tests/(securityselinuxhelper|(vircgroup|nss)mock|commandhelper)\.c|tools/wireshark/src/packet-libvirt\.c|tools/nss/libvirt_nss(_leases|_macs)?\.c|build-aux/useless-if-before-free)$$ + ^(docs/hacking\.rst|src/util/viralloc\.[ch]|examples/.*|tests/(securityselinuxhelper|(vircgroup|nss)mock|commandhelper)\.c|tools/wireshark/src/packet-libvirt\.c|tools/nss/libvirt_nss(_leases|_macs)?\.c|build-aux/useless-if-before-free)$$ exclude_file_name_regexp--sc_prohibit_readlink = \ ^src/(util/virutil|lxc/lxc_container)\.c$$ @@ -2048,7 +2048,7 @@ exclude_file_name_regexp--sc_prohibit_readlink = \ exclude_file_name_regexp--sc_prohibit_setuid = ^src/util/virutil\.c|tools/virt-login-shell\.c$$ exclude_file_name_regexp--sc_prohibit_snprintf = \ - ^(build-aux/syntax-check\.mk|docs/hacking\.html\.in|tools/virt-login-shell\.c)$$ + ^(build-aux/syntax-check\.mk|docs/hacking\.rst|tools/virt-login-shell\.c)$$ exclude_file_name_regexp--sc_prohibit_strtol = ^examples/.*$$ diff --git a/docs/hacking.html.in b/docs/hacking.html.in deleted file mode 100644 index 1756e84fc4..0000000000 --- a/docs/hacking.html.in +++ /dev/null @@ -1,1555 +0,0 @@ -<?xml version="1.0" encoding="UTF-8"?> -<!DOCTYPE html> -<html xmlns="http://www.w3.org/1999/xhtml";> - <body> - <h1>Contributor guidelines</h1> - - <ul id="toc"></ul> - - <h2><a id="patches">General tips for contributing patches</a></h2> - <ol> - <li> - <p>Discuss any large changes on the mailing list first. Post patches - early and listen to feedback.</p> - </li> - - <li> - <p>Official upstream repository is kept in git - (<code>https://libvirt.org/git/libvirt.git</code>) and is browsable - along with other libvirt-related repositories - (e.g. libvirt-python) <a href="https://libvirt.org/git/";>online</a>.</p> - </li> - - <li> - <p>Patches to translations are maintained via - the <a href="https://fedora.zanata.org/";>zanata project</a>. - 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.</p> - </li> - - <li><p>The simplest way to send patches is to use the - <a href="https://github.com/stefanha/git-publish";><code>git-publish</code></a> - tool. All libvirt-related repositories contain a config file that - tells git-publish to use the correct mailing list and subject prefix.</p> - <p>Alternatively, you may send patches using <code>git send-email</code>.</p> - <p>Also, for code motion patches, you may find that <code>git - diff --patience</code> provides an easier-to-read patch. - However, the usual workflow of libvirt developer is:</p> -<pre> - git checkout master - git pull - git checkout -t origin -b workbranch - Hack, committing any changes along the way -</pre> - <p>More hints on compiling can be - found <a href="compiling.html">here</a>. When you want to - post your patches:</p> -<pre> - git pull --rebase - (fix any conflicts) - git send-email --cover-letter --no-chain-reply-to --annotate \ - --confirm=always --to=libvir-list@xxxxxxxxxx master -</pre> - <p>For a single patch you can omit - <code>--cover-letter</code>, but a series of two or more - patches needs a cover letter.</p> - <p>Note that the <code>git send-email</code> 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 - <code>git send-email</code>, you might need to configure it to - point it to your SMTP server with something like:</p> -<pre> - git config --global sendemail.smtpServer stmp.youremailprovider.net -</pre> - <p>If you get tired of typing - <code>--to=libvir-list@xxxxxxxxxx</code> all the time, you can - configure that to be automatically handled as well:</p> -<pre> - git config sendemail.to libvir-list@xxxxxxxxxx -</pre> - <p>As a rule, patches should be sent to the mailing list only: all - developers are subscribed to libvir-list and read it regularly, so - <strong>please don't CC individual developers</strong> unless - they've explicitly asked you to.</p> - <p>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.</p> - <p>If everything went well, your patch should show up on the - <a href="https://www.redhat.com/archives/libvir-list/";>libvir-list - archives</a> 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. - <strong>Note that, if you are not already a subscriber, your very - first post to the mailing list will be - subject to moderation</strong>, and it's not uncommon for that to - take around a day.</p> - <p>Please follow this as close as you can, especially the rebase and - <code>git send-email</code> part, as it makes life easier for other - developers to review your patch set.</p> - <p>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 <code>---</code> 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 - <code>--subject-prefix=PATCHv2</code> appended to <code>git - send-email</code> (substitute <code>v2</code> with the correct - version if needed though).</p> - </li> - - <li><p>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. - </p> - </li> - - <li><p>Contributors to libvirt projects <strong>must</strong> - assert that they are in compliance with the - <a href="https://developercertificate.org/";>Developer - Certificate of Origin 1.1</a>. This is achieved by adding - a "Signed-off-by" line containing the contributor's name - and e-mail to every commit message. The presence - of this line attests that the contributor has read the - above lined DCO and agrees with its statements. - </p></li> - - <li><p>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 (<b>including</b> <code>make - check</code> and <code>make syntax-check</code>) 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 <code>git bisect</code>, among other things).</p> - </li> - - <li> - <p>Make sure your patches apply against libvirt GIT. Developers - only follow GIT and don't care much about released versions.</p> - </li> - - <li><p>Run the automated tests on your code before submitting any changes. - That is: - </p> -<pre> - make check - make syntax-check - make -C tests valgrind -</pre> - <p><a href="http://valgrind.org/";>Valgrind</a> is a test that checks - for memory management issues, such as leaks or use of uninitialized - variables. - </p> - - <p> - 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: - </p> -<pre> - make check VIR_TEST_EXPENSIVE=1 -</pre> - <p> - If you encounter any failing tests, the VIR_TEST_DEBUG - environment variable may provide extra information to debug - the failures. Larger values of VIR_TEST_DEBUG may provide - larger amounts of information: - </p> - -<pre> - VIR_TEST_DEBUG=1 make check (or) - VIR_TEST_DEBUG=2 make check -</pre> - - <p> - When debugging failures during development, it is possible - to focus in on just the failing subtests by using - VIR_TEST_RANGE. I.e. to run all tests from 3 to 20 with the - exception of tests 6 and 16, use: - </p> - -<pre> - VIR_TEST_DEBUG=1 VIR_TEST_RANGE=3-5,7-20,^16 ./run tests/qemuxml2argvtest -</pre> - - <p> - Also, individual tests can be run from inside the <code>tests/</code> - directory, like: - </p> -<pre> - ./qemuxml2xmltest -</pre> - - <p> - 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. - </p> -<pre> - VIR_TEST_REGENERATE_OUTPUT=1 ./qemuxml2argvtest -</pre> - - <p>There is also a <code>./run</code> 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. - </p> - - <p>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 - <code>VIR_TEST_FILE_ACCESS</code> environment variable. - Then <code>VIR_TEST_FILE_ACCESS_OUTPUT</code> environment - variable can alter location where the file is stored.</p> -<pre> - VIR_TEST_FILE_ACCESS=1 VIR_TEST_FILE_ACCESS_OUTPUT="/tmp/file_access.txt" ./qemuxml2argvtest -</pre> - - </li> - <li><p>The Valgrind test should produce similar output to - <code>make check</code>. 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: - </p> -<pre> -==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) -</pre> - <p>In this example, the <code>virDomainDefParseXML()</code> had - an error path where the <code>virDomainVideoDefPtr video</code> - pointer was not properly disposed. By simply adding a - <code>virDomainVideoDefFree(video);</code> in the error path, - the issue was resolved. - </p> - - <p>Another common mistake is calling a printing function, such as - <code>VIR_DEBUG()</code> 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. - </p> -<pre> -==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) -</pre> - <p>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: - </p> -<pre> -==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) - -</pre> - <p>In this instance, it is acceptable to modify the - <code>tests/.valgrind.supp</code> 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 - <a href="http://valgrind.org/";>Valgrind home page</a>. - The following trace was added to <code>tests/.valgrind.supp</code> - in order to suppress the warning: - </p> -<pre> -{ - dlInitMemoryLeak1 - Memcheck:Leak - fun:?alloc - ... - fun:call_init.part.0 - fun:_dl_init - ... - obj:*/lib*/ld-2.*so* -} -</pre> - </li> - - <li> - <p>Update tests and/or documentation, particularly if you are adding - a new feature or changing the output of a program.</p> - </li> - - <li> - <p>Don't forget to update the <a href="news.html">release notes</a> - by changing <code>docs/news.xml</code> 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 <code>docs/news.xml</code> should be updated - in its own commit not to get in the way of backports.</p> - </li> - </ol> - - <p> - There is more on this subject, including lots of links to background - reading on the subject, on - <a href="http://people.redhat.com/rjones/how-to-supply-code-to-open-source-projects/";> - Richard Jones' guide to working with open source projects</a>. - </p> - - <h2><a id="lang">Language Usage</a></h2> - - <p> - The libvirt repository makes use of a large number of programming - languages. It is anticipated that in the future libvirt will adopt - use of other new languages. To reduce the overall burden on developers, - there is thus a general desire to phase out usage of some of the - existing languages. - </p> - - <p> - The preferred languages at this time are: - </p> - - <ul> - <li>C - for the main libvirt codebase. Dialect supported by - GCC/CLang only.</li> - <li>Python - for supporting build scripts / tools. Code must - run with both version 2.7 and 3.x at this time.</li> - </ul> - - <p> - Languages that should not be used for any new contributions: - </p> - - <ul> - <li>Perl - build scripts must be written in Python instead.</li> - <li>Shell - build scripts must be written in Python instead.</li> - </ul> - - <h2><a id="tooling">Tooling</a></h2> - - <p> - 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: - </p> - - <ul> - <li> - <a href="https://github.com/jeaye/color_coded";>color_coded</a>, - a vim plugin for libclang-powered semantic syntax highlighting; - </li> - - <li> - <a href="http://valloric.github.io/YouCompleteMe/";>YouCompleteMe</a>, - a vim plugin for libclang-powered semantic code completion. - </li> - </ul> - - <h2><a id="naming">Naming conventions</a></h2> - - <p> - 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. - </p> - - <dl> - <dt>File names</dt> - <dd> - <p> - 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. - </p> - </dd> - <dt>Enum type & field names</dt> - <dd> - <p> - 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. - </p> - <pre> - typedef enum _virSocketType virSocketType; - enum _virSocketType { - VIR_SOCKET_TYPE_IPV4, - VIR_SOCKET_TYPE_IPV6, - };</pre> - </dd> - <dt>Struct type names</dt> - <dd> - <p> - 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. - </p> - <pre> - typedef struct _virHashTable virHashTable; - typedef virHashTable *virHashTablePtr; - struct _virHashTable { - ... - };</pre> - </dd> - <dt>Function names</dt> - <dd> - <p> - 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'. - </p> - </dd> - <dt>Macro names</dt> - <dd> - <p> - 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. - </p> - </dd> - </dl> - - <h2><a id="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, 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: - </p> -<pre> - 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/ -</pre> - <p> - 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. - </p> - - <h2><a id="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 post-process that output further, by - piping it through <code>expand -i</code>, since some leading TABs can get through. - Usually they're in macro definitions or strings, and should be converted - anyhow. - </p> - - <p> - 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 <code>/* */</code> comments rather - than <code>//</code>. Also, when declaring local variables, the - prevailing style has been to declare them at the beginning of a - scope, rather than immediately before use. - </p> - - - <h2><a id="bracket_spacing">Bracket spacing</a></h2> - - <p> - The keywords <code>if</code>, <code>for</code>, <code>while</code>, - and <code>switch</code> must have a single space following them - before the opening bracket. E.g. - </p> - <pre> - if(foo) // Bad - if (foo) // Good -</pre> - - <p> - Function implementations must <strong>not</strong> have any whitespace - between the function name and the opening bracket. E.g. - </p> - <pre> - int foo (int wizz) // Bad - int foo(int wizz) // Good -</pre> - - <p> - Function calls must <strong>not</strong> have any whitespace - between the function name and the opening bracket. E.g. - </p> - <pre> - bar = foo (wizz); // Bad - bar = foo(wizz); // Good -</pre> - - <p> - Function typedefs must <strong>not</strong> have any whitespace - between the closing bracket of the function name and opening - bracket of the arg list. E.g. - </p> - <pre> - typedef int (*foo) (int wizz); // Bad - typedef int (*foo)(int wizz); // Good -</pre> - - <p> - There must not be any whitespace immediately following any - opening bracket, or immediately prior to any closing bracket. E.g. - </p> - <pre> - int foo( int wizz ); // Bad - int foo(int wizz); // Good -</pre> - - <h2><a id="comma">Commas</a></h2> - - <p> - Commas should always be followed by a space or end of line, and - never have leading space; this is enforced during 'make - syntax-check'. - </p> - <pre> - call(a,b ,c);// Bad - call(a, b, c); // Good -</pre> - - <p> - 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. - </p> - <pre> - enum { - VALUE_ONE, - VALUE_TWO // Bad - }; - enum { - VALUE_THREE, - VALUE_FOUR, // Good - }; -</pre> - - <h2><a id="semicolon">Semicolons</a></h2> - - <p> - Semicolons should never have a space beforehand. Inside the - condition of a <code>for</code> 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 <code>while</code>). While not enforced, loop counters - generally use post-increment. - </p> - <pre> - for (i = 0 ;i < limit ; ++i) { // Bad - for (i = 0; i < limit; i++) { // Good - for (;;) { // ok - while (1) { // Better -</pre> - <p> - Empty loop bodies are better represented with curly braces and a - comment, although use of a semicolon is not currently rejected. - </p> - <pre> - while ((rc = waitpid(pid, &st, 0) == -1) && - errno == EINTR); // ok - while ((rc = waitpid(pid, &st, 0) == -1) && - errno == EINTR) { // Better - /* nothing */ - } -</pre> - - <h2><a id="curly_braces">Curly braces</a></h2> - - <p> - Omit the curly braces around an <code>if</code>, <code>while</code>, - <code>for</code> 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-<i>statement</i> loop: each has only one <i>line</i> in its body. - </p> - -<pre> - while (expr) // single line body; {} is forbidden - single_line_stmt(); -</pre> - -<pre> - while (expr(arg1, - arg2)) // indentation makes it obvious it is single line, - single_line_stmt(); // {} is optional (not enforced either way) -</pre> - -<pre> - while (expr1 && - expr2) { // multi-line, at same indentation, {} required - single_line_stmt(); - } -</pre> - - <p> - 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: - </p> - -<pre> - while (true) // BAD! multi-line body with no braces - /* comment... */ - single_line_stmt(); -</pre> - <p> - Do this instead: - </p> -<pre> - while (true) { // Always put braces around a multi-line body. - /* comment... */ - single_line_stmt(); - } -</pre> - <p> - There is one exception: when the second body line is not at the same - indentation level as the first body line: - </p> -<pre> - if (expr) - die("a diagnostic that would make this line" - " extend past the 80-column limit")); -</pre> - - <p> - 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. - </p> - - <p> - To reiterate, don't do this: - </p> - -<pre> - if (expr) // BAD: no braces around... - while (expr_2) { // ... a multi-line body - ... - } -</pre> - - <p> - Do this, instead: - </p> - -<pre> - if (expr) { - while (expr_2) { - ... - } - } -</pre> - - <p> - 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 <code>if</code> or <code>else</code> - block, and the counterpart block <b>does</b> use braces. In - that case, put braces around both blocks. Also, if - the <code>else</code> block is much shorter than - the <code>if</code> block, consider negating the - <code>if</code>-condition and swapping the bodies, putting the - short block first and making the longer, multi-line block be the - <code>else</code> block. - </p> - -<pre> - 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 { - ... - ... - } -</pre> - - <p> - 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: - </p> - -<pre> - if (!expr) { - x = y; // putting the smaller block first is more readable - } else { - ... - ... - } -</pre> - - <p> - But if negating a complex condition is too ugly, then at least - add braces: - </p> - -<pre> - if (complex expr not worth negating) { - ... - ... - } else { - x = y; - } -</pre> - - <p>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. - </p> - -<pre> - 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; - } - } - } -</pre> - - <h2><a id="conditions">Conditional expressions</a></h2> - <p>For readability reasons new code should avoid shortening comparisons - to 0 for numeric types. Boolean and pointer comparisions may be - shortened. All long forms are okay: - </p> -<pre> - virFooPtr foos = NULL; - size nfoos = 0; - bool hasFoos = false; - -GOOD: - if (!foos) - if (!hasFoos) - if (nfoos == 0) - if (foos == NULL) - if (hasFoos == true) - -BAD: - if (!nfoos) - if (nfoos) -</pre> - <p>New code should avoid the ternary operator as much as possible. - Specifically it must never span more than one line or nest: - </p> -<pre> -BAD: - char *foo = baz ? - virDoSomethingReallyComplex(driver, vm, something, baz->foo) : - NULL; - - char *foo = bar ? bar->baz ? bar->baz->foo : "nobaz" : "nobar"; -</pre> - - <h2><a id="preprocessor">Preprocessor</a></h2> - - <p>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. - </p> - - <p> - For variadic macros, stick with C99 syntax: - </p> -<pre> - #define vshPrint(_ctl, ...) fprintf(stdout, __VA_ARGS__) -</pre> - - <p>Use parenthesis when checking if a macro is defined, and use - indentation to track nesting: - </p> -<pre> - #if defined(HAVE_POSIX_FALLOCATE) && !defined(HAVE_FALLOCATE) - # define fallocate(a, ignored, b, c) posix_fallocate(a, b, c) - #endif -</pre> - - <h2><a id="types">C types</a></h2> - - <p> - Use the right type. - </p> - - <h3>Scalars</h3> - - <ul> - <li>If you're using <code>int</code> or <code>long</code>, 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 <code>size_t</code> (use - <code>ssize_t</code> only if required).</li> - <li>If it's file-size related, use uintmax_t, or maybe <code>off_t</code>.</li> - <li>If it's file-offset related (i.e., signed), use <code>off_t</code>.</li> - <li>If it's just counting small numbers use <code>unsigned int</code>; - (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 <code>bool</code> type - and use the corresponding <code>true</code> and <code>false</code> macros. - </li> - <li>In the unusual event that you require a specific width, use a - standard type like <code>int32_t</code>, <code>uint32_t</code>, - <code>uint64_t</code>, etc.</li> - <li>While using <code>bool</code> is good for readability, it comes with - minor caveats: - <ul> - <li>Don't use <code>bool</code> 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 <code>bool</code> in libvirt's - logical wire protocol, since XDR maps that to its lower-level <code>bool_t</code> - type, which <b>is</b> fixed-size.</li> - <li>Don't compare a bool variable against the literal, <code>true</code>, - since a value with a logical non-false value need not be <code>1</code>. - I.e., don't write <code>if (seen == true) ...</code>. Rather, - write <code>if (seen)...</code>.</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 <code>size_t</code>, - <code>pid_t</code> or <code>off_t</code>, use matching types for any - corresponding variables. - </p> - - <p> - Also, if you try to use e.g., <code>unsigned int</code> as a type, and that - conflicts with the signedness of a related variable, sometimes - it's best just to use the <b>wrong</b> type, if <i>pulling the thread</i> - 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 <i>const-correct</i>. - Unless a pointer is used to modify the pointed-to storage, - give it the <code>const</code> 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 id="attribute_annotations">Attribute annotations</a></h2> - <p> - Use the following annotations to help the compiler and/or static - analysis tools understand the code better: - </p> - - <table class="top_table"> - <tr><th>Macro</th><th>Meaning</th></tr> - <tr><td><code>ATTRIBUTE_NONNULL</code></td><td>passing NULL for this parameter is not allowed</td></tr> - <tr><td><code>ATTRIBUTE_PACKED</code></td><td>force a structure to be packed</td></tr> - <tr><td><code>G_GNUC_FALLTHROUGH</code></td><td>allow code reuse by multiple switch cases</td></tr> - <tr><td><code>G_GNUC_NO_INLINE</code></td><td>the function is mocked in the test suite</td></tr> - <tr><td><code>G_GNUC_NORETURN</code></td><td>the function never returns</td></tr> - <tr><td><code>G_GNUC_NULL_TERMINATED</code></td><td>last parameter must be NULL</td></tr> - <tr><td><code>G_GNUC_PRINTF</code></td><td>validate that the formatting string matches parameters</td></tr> - <tr><td><code>G_GNUC_UNUSED</code></td><td>parameter is unused in this implementation of the function</td></tr> - <tr><td><code>G_GNUC_WARN_UNUSED_RESULT</code></td><td>the return value must be checked</td></tr> - </table> - - <h2><a id="glib">Adoption of GLib APIs</a></h2> - - <p> - Libvirt has adopted use of the - <a href="https://developer.gnome.org/glib/stable/";>GLib library</a>. - Due to libvirt's long history of development, there are many APIs - in libvirt, for which GLib provides an alternative solution. The - general rule to follow is that the standard GLib solution will be - preferred over historical libvirt APIs. Existing code will be - ported over to use GLib APIs over time, but new code should use - the GLib APIs straight away where possible. - </p> - - <p> - The following is a list of libvirt APIs that should no longer be - used in new code, and their suggested GLib replacements: - </p> - - <dl> - <dt><code>VIR_ALLOC</code>, <code>VIR_REALLOC</code>, - <code>VIR_RESIZE_N</code>, <code>VIR_EXPAND_N</code>, - <code>VIR_SHRINK_N</code>, <code>VIR_FREE</code>, - <code>VIR_APPEND_ELEMENT</code>, <code>VIR_INSERT_ELEMENT</code>, - <code>VIR_DELETE_ELEMENT</code></dt> - <dd>Prefer the GLib APIs <code>g_new0</code>/<code>g_renew</code>/ - <code>g_free</code> in most cases. There should rarely be a need - to use <code>g_malloc</code>/<code>g_realloc</code>. - Instead of using plain C arrays, it is preferrable to use - one of the GLib types, <code>GArray</code>, <code>GPtrArray</code> - or <code>GByteArray</code>. These - all use a struct to track the array memory and size together - and efficiently resize. <strong>NEVER MIX</strong> use of the - classic libvirt memory allocation APIs and GLib APIs within - a single method. Keep the style consistent, converting existing - code to GLib style in a separate, prior commit.</dd> - - <dt><code>virStrerror</code></dt> - <dd>The GLib <code>g_strerror()</code> function should be used instead, - which has a simpler calling convention as an added benefit.</dd> - - <table class="top_table"> - <tr><th>deprecated version</th><th>GLib version</th><th>Notes</th></tr> - <tr><td><code>VIR_ALLOC(var)</code></td><td><code>g_new0(var_t, 1)</code></td> - <td>the type needs to be passed explicitly</td></tr> - <tr><td><code>VIR_ALLOC_N</code></td><td><code>g_new0(var_t, n)</code></td><td></td></tr> - <tr><td><code>VIR_REALLOC_N</code></td><td><code>g_renew(var_t, ptr, n)</code></td> - <td>the newly added memory is not zeroed</td></tr> - <tr><td><code>VIR_EXPAND_N</code></td><td><code>g_renew(var_t, ptr, n)</code></td> - <td>zero the new memory manually or use an array type</td></tr> - <tr><td><code>VIR_SHRINK_N</code></td><td><code>g_renew(var_t, ptr, n)</code></td> - <td></td></tr> - <tr><td><code>VIR_APPEND_ELEMENT</code></td><td><code>g_array_append_val</code></td> - <td><code>g_ptr_array_add</code> or <code>g_byte_array_append</code></td></tr> - <tr><td><code>VIR_INSERT_ELEMENT</code></td><td><code>g_array_insert_val</code></td> - <td><code>g_ptr_array_insert</code></td></tr> - <tr><td><code>VIR_DELETE_ELEMENT</code></td><td><code>g_array_remove_index</code></td> - <td><code>g_ptr_array_remove_index</code> or <code>g_byte_array_remove_index</code></td></tr> - <tr><td><code>VIR_FREE</code></td><td><code>g_free</code></td> - <td><code>g_free</code> does not zero the pointer</td></tr> - </table> - - <p>String allocation macros and functions:</p> - <table class="top_table"> - <tr><th>deprecated version</th><th>GLib version</th><th>Notes</th></tr> - <tr><td><code>virAsprintf</code></td><td><code>g_strdup_printf</code></td><td></td></tr> - <tr><td><code>virVasprintf</code></td><td><code>g_strdup_vprint</code></td> - <td>use <code>g_vasprintf</code> if you really need to know the returned length</td></tr> - </table> - </dl> - - <p> - The following libvirt APIs have been deleted already: - </p> - <dl> - <dt><code>VIR_AUTOPTR</code>, <code>VIR_AUTOCLEAN</code>, <code>VIR_AUTOFREE</code></dt> - <dd>The GLib macros <code>g_autoptr</code>, <code>g_auto</code> and - <code>g_autofree</code> must be used - instead in all new code. In existing code, the GLib macros must - never be mixed with libvirt macros within a method, nor should - they be mixed with <code>VIR_FREE</code>. If introducing GLib macros to an - existing method, any use of libvirt macros must be converted - in an independent commit. - </dd> - - <dt><code>VIR_DEFINE_AUTOPTR_FUNC</code>, <code>VIR_DEFINE_AUTOCLEAN_FUNC</code></dt> - <dd>The GLib macros <code>G_DEFINE_AUTOPTR_CLEANUP_FUNC</code> and - <code>G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC</code> must be used in all - new code. Existing code should be converted to the - new macros where relevant. It is permissible to use - <code>g_autoptr</code>, <code>g_auto</code> on an object whose cleanup function - is declared with the libvirt macros and vice-versa. - </dd> - - <dt><code>VIR_AUTOUNREF</code></dt> - <dd>The GLib macros <code>g_autoptr</code> and <code>G_DEFINE_AUTOPTR_CLEANUP_FUNC</code> - should be used to manage autoclean of virObject classes. - This matches usage with GObject classes.</dd> - - <dt><code>VIR_STRDUP</code>, <code>VIR_STRNDUP</code></dt> - <dd>Prefer the GLib APIs <code>g_strdup</code> and <code>g_strndup</code>.</dd> - </dl> - <table class="top_table"> - <tr><th>deleted version</th><th>GLib version</th><th>Notes</th></tr> - <tr><td><code>VIR_AUTOPTR</code></td><td><code>g_autoptr</code></td><td></td></tr> - <tr><td><code>VIR_AUTOCLEAN</code></td><td><code>g_auto</code></td><td></td></tr> - <tr><td><code>VIR_AUTOFREE</code></td><td><code>g_autofree</code></td><td>The GLib version does not use parentheses</td></tr> - <tr><td><code>VIR_AUTOUNREF</code></td><td><code>g_autoptr</code></td><td>The cleanup function needs to be defined</td></tr> - <tr><td><code>VIR_DEFINE_AUTOPTR_FUNC</code></td><td><code>G_DEFINE_AUTOPTR_CLEANUP_FUNC</code></td><td></td></tr> - <tr><td><code>VIR_DEFINE_AUTOCLEAN_FUNC</code></td><td><code>G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC</code></td><td></td></tr> - <tr><td><code>VIR_STEAL_PTR</code></td><td><code>g_steal_pointer</code></td> - <td><code>a = f(&b)</code> instead of <code>f(a, b)</code></td></tr> - <tr><td><code>VIR_RETURN_PTR</code></td><td><code>return g_steal_pointer</code></td><td></td></tr> - <tr><td><code>ARRAY_CARDINALITY</code></td><td><code>G_N_ELEMENTS</code></td><td></td></tr> - <tr><td><code>ATTRIBUTE_FALLTHROUGH</code></td><td><code>G_GNUC_FALLTHROUGH</code></td><td></td></tr> - <tr><td><code>ATTRIBUTE_FMT_PRINTF</code></td><td><code>G_GNUC_PRINTF</code></td><td></td></tr> - <tr><td><code>ATTRIBUTE_NOINLINE</code></td><td><code>G_GNUC_NO_INLINE</code></td><td></td></tr> - <tr><td><code>ATTRIBUTE_NORETURN</code></td><td><code>G_GNUC_NORETURN</code></td><td></td></tr> - <tr><td><code>ATTRIBUTE_RETURN_CHECK</code></td><td><code>G_GNUC_WARN_UNUSED_RESULT</code></td><td></td></tr> - <tr><td><code>ATTRIBUTE_SENTINEL</code></td><td><code>G_GNUC_NULL_TERMINATED</code></td><td></td></tr> - <tr><td><code>ATTRIBUTE_UNUSED</code></td><td><code>G_GNUC_UNUSED</code></td><td></td></tr> - <tr><td><code>VIR_STRDUP</code></td><td><code>g_strdup</code></td><td></td></tr> - <tr><td><code>VIR_STRNDUP</code></td><td><code>g_strndup</code></td><td></td></tr> - <tr><td><code>virStrerror</code></td><td><code>g_strerror</code></td><td></td></tr> - </table> - - - <h2><a id="file_handling">File handling</a></h2> - - <p> - Usage of the <code>fdopen()</code>, <code>close()</code>, <code>fclose()</code> - 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 - </p> - - <ul> - <li><p>Open a file from a file descriptor:</p> - -<pre> - 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 */ -</pre></li> - - <li><p>Close a file descriptor:</p> -<pre> - if (VIR_CLOSE(fd) < 0) { - virReportSystemError(errno, "%s", _("failed to close file")); - } -</pre></li> - - <li><p>Close a file:</p> - -<pre> - if (VIR_FCLOSE(file) < 0) { - virReportSystemError(errno, "%s", _("failed to close file")); - } -</pre></li> - - <li><p>Close a file or file descriptor in an error path, without losing - the previous <code>errno</code> value:</p> - -<pre> - VIR_FORCE_CLOSE(fd); - VIR_FORCE_FCLOSE(file); -</pre> - </li> - </ul> - - <h2><a id="string_comparision">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 insensitive 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 insensitive 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> - <li><p>To avoid having to check if a or b are NULL:</p> -<pre> - STREQ_NULLABLE(a, b) - STRNEQ_NULLABLE(a, b) -</pre> - </li> - </ul> - - - <h2><a id="string_copying">String copying</a></h2> - - <p> - Do not use the strncpy function. According to the man page, it - does <b>not</b> guarantee a NULL-terminated buffer, which makes - it extremely dangerous to use. Instead, use one of the replacement - functions provided by libvirt: - </p> - -<pre> - virStrncpy(char *dest, const char *src, size_t n, size_t destbytes) -</pre> - <p> - The first two arguments have the same meaning as for strncpy, - namely the destination and source of the copy operation. Unlike - strncpy, the function will always copy exactly the number of bytes - requested and make sure the destination is NULL-terminated, as the - source is required to be; sanity checks are performed to ensure the - size of the destination, as specified by the last argument, is - sufficient for the operation to succeed. On success, 0 is returned; - on failure, a value <0 is returned instead. - </p> - -<pre> - virStrcpy(char *dest, const char *src, size_t destbytes) -</pre> - <p> - Use this variant if you know you want to copy the entire src - string into dest. - </p> - -<pre> - virStrcpyStatic(char *dest, const char *src) -</pre> - <p> - Use this variant if you know you want to copy the entire src - string into dest <b>and</b> 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. - </p> - -<pre> - dst = g_strdup(src); - dst = g_strndup(src, n); -</pre> - <p> - You should avoid using strdup or strndup directly as they do not handle - out-of-memory errors, and do not allow a NULL source. - Use <code>g_strdup</code> and <code>g_strndup</code> from GLib which - abort on OOM and handle NULL source by returning NULL. - </p> - - <h2><a id="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 either the - <a href="https://developer.gnome.org/glib/stable/glib-Strings.html";>GString</a> - type from GLib or the virBuffer API. - If formatting XML or QEMU command line is needed, use the virBuffer - API described in virbuffer.h, since it has helper functions for those. - </p> - - <p>Typical usage is as follows:</p> - -<pre> - char * - somefunction(...) - { - g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; - - ... - - virBufferAddLit(&buf, "<domain>\n"); - virBufferAsprintf(&buf, " <memory>%d</memory>\n", memory); - if (some_error) - return NULL; /* g_auto will free the memory used so far */ - ... - virBufferAddLit(&buf, "</domain>\n"); - - ... - - if (virBufferCheckError(&buf) < 0) - return NULL; - - return virBufferContentAndReset(&buf); - } -</pre> - - - <h2><a id="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 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. - { - ... -</pre> - - <p> - Of particular note: <b>Do not</b> 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. - </p> - - - <h2><a id="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 virCommandAddEnvFormat in vircommand.h: - </p> - -<pre> - void virCommandAddEnvFormat(virCommandPtr cmd, const char *format, ...) - G_GNUC_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> - - <p> - When printing to a string, consider using GString or virBuffer for - incremental allocations, g_strdup_printf for a one-shot allocation, - and g_snprintf for fixed-width buffers. Only use g_sprintf, - if you can prove the buffer won't overflow. - </p> - - <h2><a id="errors">Error message format</a></h2> - - <p> - Error messages visible to the user should be short and descriptive. All - error messages are translated using gettext and thus must be wrapped in - <code>_()</code> macro. To simplify the translation work, the error message - must not be concatenated from various parts. To simplify searching for - the error message in the code the strings should not be broken even - if they result into a line longer than 80 columns and any formatting - modifier should be enclosed by quotes or other obvious separator. - If a string used with <code>%s</code> can be NULL the NULLSTR macro must - be used. - </p> - -<pre> - GOOD: virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to connect to remote host '%s'"), hostname) - - BAD: virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to %s to remote host '%s'"), - "connect", hostname); - - BAD: virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to connect " - "to remote host '%s'), - hostname); -</pre> - - <h2><a id="goto">Use of goto</a></h2> - - <p> - The use of goto is not forbidden, and goto is widely used - throughout libvirt. While the uncontrolled use of goto will - quickly lead to unmaintainable code, there is a place for it in - well structured code where its use increases readability and - maintainability. In general, if goto is used for error - recovery, it's likely to be ok, otherwise, be cautious or avoid - it all together. - </p> - - <p> - The typical use of goto is to jump to cleanup code in the case - of a long list of actions, any of which may fail and cause the - entire operation to fail. In this case, a function will have a - single label at the end of the function. It's almost always ok - to use this style. In particular, if the cleanup code only - involves free'ing memory, then having multiple labels is - overkill. g_free() and most of the functions named XXXFree() in - libvirt is required to handle NULL as its arg. This does not - apply to libvirt's public APIs. 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. - Note that most of libvirt's type declarations can be marked with - either <code>g_autofree</code> or <code>g_autoptr</code> which uses - the compiler's <code>__attribute__((cleanup))</code> that calls - the appropriate free function when the variable goes out of scope. - </p> - - <p> - There are a couple of signs that a particular use of goto is not - ok: - </p> - - <ul> - <li>You're using multiple labels. If you find yourself using - multiple labels, you're strongly encouraged to rework your code - to eliminate all but one of them.</li> - <li>The goto jumps back up to a point above the current line of - code being executed. Please use some combination of looping - constructs to re-execute code instead; it's almost certainly - going to be more understandable by others. One well-known - exception to this rule is restarting an i/o operation following - EINTR.</li> - <li>The goto jumps down to an arbitrary place in the middle of a - function followed by further potentially failing calls. You - should almost certainly be using a conditional and a block - instead of a goto. Perhaps some of your function's logic would - be better pulled out into a helper function.</li> - </ul> - - <p> - Although libvirt does not encourage the Linux kernel wind/unwind - style of multiple labels, there's a good general discussion of - the issue archived at - <a href="http://kerneltrap.org/node/553/2131";>KernelTrap</a> - </p> - - <p> - When using goto, please use one of these standard labels if it - makes sense: - </p> - -<pre> - error: A path only taken upon return with an error code - cleanup: A path taken upon return with success code + optional error - no_memory: A path only taken upon return with an OOM error code - retry: If needing to jump upwards (e.g., retry on EINTR) -</pre> - - <p> - Top-level labels should be indented by one space (putting them on - the beginning of the line confuses function context detection in git): - </p> - -<pre> -int foo() -{ - /* ... do stuff ... */ - cleanup: - /* ... do other stuff ... */ -} -</pre> - - - - <h2><a id="committers">Libvirt committer guidelines</a></h2> - - <p> - The AUTHORS files indicates the list of people with commit access right - who can actually merge the patches. - </p> - - <p> - 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. - </p> - - <p> - An exception to 'review and approval on the list first' is fixing failures - to build: - </p> - <ul> - <li>if a recently committed patch breaks compilation on a platform - or for a given driver, then it's fine to commit a minimal fix - directly without getting the review feedback first</li> - <li>if make check or make syntax-check breaks, if there is - an obvious fix, it's fine to commit immediately. - The patch should still be sent to the list (or tell what the fix was if - trivial), and 'make check syntax-check' should pass too, before committing - 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> - <li>(ir)regular pulls from other repositories or automated updates, such - as the keycodemap submodule updates, pulling in new translations or updating - the container images for the CI system - </li> - </ul> - </body> -</html> diff --git a/docs/hacking.rst b/docs/hacking.rst new file mode 100644 index 0000000000..ac02e9ab73 --- /dev/null +++ b/docs/hacking.rst @@ -0,0 +1,1400 @@ +====================== +Contributor guidelines +====================== + +.. contents:: + +General tips for contributing patches +===================================== + +#. Discuss any large changes on the mailing list first. Post + patches early and listen to feedback. + +#. Official upstream repository is kept in git + (``https://libvirt.org/git/libvirt.git``) and is browsable + along with other libvirt-related repositories (e.g. + libvirt-python) `online <https://libvirt.org/git/>`__. + +#. 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. + +#. The simplest way to send patches is to use the + `git-publish <https://github.com/stefanha/git-publish>`__ + tool. All libvirt-related repositories contain a config file + that tells git-publish to use the correct mailing list and + subject prefix. + + Alternatively, you may send patches using ``git send-email``. + + 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 \ + --confirm=always --to=libvir-list@xxxxxxxxxx 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@xxxxxxxxxx`` all + the time, you can configure that to be automatically handled as + well: + + :: + + git config sendemail.to libvir-list@xxxxxxxxxx + + 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 <https://www.redhat.com/archives/libvir-list/>`__ 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, if + you are not already a subscriber, 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). + +#. 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. + +#. Contributors to libvirt projects **must** assert that they are + in compliance with the `Developer Certificate of Origin + 1.1 <https://developercertificate.org/>`__. This is achieved by + adding a "Signed-off-by" line containing the contributor's name + and e-mail to every commit message. The presence of this line + attests that the contributor has read the above lined DCO and + agrees with its statements. + +#. 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). + +#. Make sure your patches apply against libvirt GIT. Developers + only follow GIT and don't care much about released versions. + +#. Run the automated tests on your code before submitting any + changes. That is: + + :: + + 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 VIR_TEST_RANGE. + I.e. to run all tests from 3 to 20 with the exception of tests + 6 and 16, use: + + :: + + VIR_TEST_DEBUG=1 VIR_TEST_RANGE=3-5,7-20,^16 ./run 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 + +#. 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* + } + +#. Update tests and/or documentation, particularly if you are + adding a new feature or changing the output of a program. + +#. Don't forget to update the `release notes <news.html>`__ 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 <http://people.redhat.com/rjones/how-to-supply-code-to-open-source-projects/>`__. + +Language Usage +============== + +The libvirt repository makes use of a large number of programming +languages. It is anticipated that in the future libvirt will adopt +use of other new languages. To reduce the overall burden on +developers, there is thus a general desire to phase out usage of +some of the existing languages. + +The preferred languages at this time are: + +- C - for the main libvirt codebase. Dialect supported by + GCC/CLang only. +- Python - for supporting build scripts / tools. Code must run + with both version 2.7 and 3.x at this time. + +Languages that should not be used for any new contributions: + +- Perl - build scripts must be written in Python instead. +- Shell - build scripts must be written in Python instead. + +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 <https://github.com/jeaye/color_coded>`__, a vim + plugin for libclang-powered semantic syntax highlighting; +- `YouCompleteMe <http://valloric.github.io/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 must **not** have any whitespace between +the function name and the opening bracket. E.g. + +:: + + int foo (int wizz) // Bad + int foo(int wizz) // Good + +Function calls must **not** have any whitespace between the +function name and the opening bracket. E.g. + +:: + + bar = foo (wizz); // Bad + bar = foo(wizz); // Good + +Function typedefs must **not** have 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; + } + } + } + +Conditional expressions +----------------------- + +For readability reasons new code should avoid shortening +comparisons to 0 for numeric types. Boolean and pointer +comparisions may be shortened. All long forms are okay: + +:: + + virFooPtr foos = NULL; + size nfoos = 0; + bool hasFoos = false; + + GOOD: + if (!foos) + if (!hasFoos) + if (nfoos == 0) + if (foos == NULL) + if (hasFoos == true) + + BAD: + if (!nfoos) + if (nfoos) + +New code should avoid the ternary operator as much as possible. +Specifically it must never span more than one line or nest: + +:: + + BAD: + char *foo = baz ? + virDoSomethingReallyComplex(driver, vm, something, baz->foo) : + NULL; + + char *foo = bar ? bar->baz ? bar->baz->foo : "nobaz" : "nobar"; + +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. +- 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. + +Attribute annotations +--------------------- + +Use the following annotations to help the compiler and/or static +analysis tools understand the code better: + ++-------------------------------+------------------------------------------------------------+ +| Macro | Meaning | ++===============================+============================================================+ +| ``ATTRIBUTE_NONNULL`` | passing NULL for this parameter is not allowed | ++-------------------------------+------------------------------------------------------------+ +| ``ATTRIBUTE_PACKED`` | force a structure to be packed | ++-------------------------------+------------------------------------------------------------+ +| ``G_GNUC_FALLTHROUGH`` | allow code reuse by multiple switch cases | ++-------------------------------+------------------------------------------------------------+ +| ``G_GNUC_NO_INLINE`` | the function is mocked in the test suite | ++-------------------------------+------------------------------------------------------------+ +| ``G_GNUC_NORETURN`` | the function never returns | ++-------------------------------+------------------------------------------------------------+ +| ``G_GNUC_NULL_TERMINATED`` | last parameter must be NULL | ++-------------------------------+------------------------------------------------------------+ +| ``G_GNUC_PRINTF`` | validate that the formatting string matches parameters | ++-------------------------------+------------------------------------------------------------+ +| ``G_GNUC_UNUSED`` | parameter is unused in this implementation of the function | ++-------------------------------+------------------------------------------------------------+ +| ``G_GNUC_WARN_UNUSED_RESULT`` | the return value must be checked | ++-------------------------------+------------------------------------------------------------+ + +Adoption of GLib APIs +--------------------- + +Libvirt has adopted use of the `GLib +library <https://developer.gnome.org/glib/stable/>`__. Due to +libvirt's long history of development, there are many APIs in +libvirt, for which GLib provides an alternative solution. The +general rule to follow is that the standard GLib solution will be +preferred over historical libvirt APIs. Existing code will be +ported over to use GLib APIs over time, but new code should use +the GLib APIs straight away where possible. + +The following is a list of libvirt APIs that should no longer be +used in new code, and their suggested GLib replacements: + +``VIR_ALLOC``, ``VIR_REALLOC``, ``VIR_RESIZE_N``, ``VIR_EXPAND_N``, ``VIR_SHRINK_N``, ``VIR_FREE``, ``VIR_APPEND_ELEMENT``, ``VIR_INSERT_ELEMENT``, ``VIR_DELETE_ELEMENT`` + Prefer the GLib APIs ``g_new0``/``g_renew``/ ``g_free`` in most + cases. There should rarely be a need to use + ``g_malloc``/``g_realloc``. Instead of using plain C arrays, it + is preferrable to use one of the GLib types, ``GArray``, + ``GPtrArray`` or ``GByteArray``. These all use a struct to + track the array memory and size together and efficiently + resize. **NEVER MIX** use of the classic libvirt memory + allocation APIs and GLib APIs within a single method. Keep the + style consistent, converting existing code to GLib style in a + separate, prior commit. +``virStrerror`` + The GLib ``g_strerror()`` function should be used instead, + which has a simpler calling convention as an added benefit. + +The following libvirt APIs have been deleted already: + +``VIR_AUTOPTR``, ``VIR_AUTOCLEAN``, ``VIR_AUTOFREE`` + The GLib macros ``g_autoptr``, ``g_auto`` and ``g_autofree`` + must be used instead in all new code. In existing code, the + GLib macros must never be mixed with libvirt macros within a + method, nor should they be mixed with ``VIR_FREE``. If + introducing GLib macros to an existing method, any use of + libvirt macros must be converted in an independent commit. +``VIR_DEFINE_AUTOPTR_FUNC``, ``VIR_DEFINE_AUTOCLEAN_FUNC`` + The GLib macros ``G_DEFINE_AUTOPTR_CLEANUP_FUNC`` and + ``G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC`` must be used in all new + code. Existing code should be converted to the new macros where + relevant. It is permissible to use ``g_autoptr``, ``g_auto`` on + an object whose cleanup function is declared with the libvirt + macros and vice-versa. +``VIR_AUTOUNREF`` + The GLib macros ``g_autoptr`` and + ``G_DEFINE_AUTOPTR_CLEANUP_FUNC`` should be used to manage + autoclean of virObject classes. This matches usage with GObject + classes. +``VIR_STRDUP``, ``VIR_STRNDUP`` + Prefer the GLib APIs ``g_strdup`` and ``g_strndup``. + ++-------------------------------+--------------------------------------+-------------------------------------------+ +| deleted version | GLib version | Notes | ++===============================+======================================+===========================================+ +| ``VIR_AUTOPTR`` | ``g_autoptr`` | | ++-------------------------------+--------------------------------------+-------------------------------------------+ +| ``VIR_AUTOCLEAN`` | ``g_auto`` | | ++-------------------------------+--------------------------------------+-------------------------------------------+ +| ``VIR_AUTOFREE`` | ``g_autofree`` | The GLib version does not use parentheses | ++-------------------------------+--------------------------------------+-------------------------------------------+ +| ``VIR_AUTOUNREF`` | ``g_autoptr`` | The cleanup function needs to be defined | ++-------------------------------+--------------------------------------+-------------------------------------------+ +| ``VIR_DEFINE_AUTOPTR_FUNC`` | ``G_DEFINE_AUTOPTR_CLEANUP_FUNC`` | | ++-------------------------------+--------------------------------------+-------------------------------------------+ +| ``VIR_DEFINE_AUTOCLEAN_FUNC`` | ``G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC`` | | ++-------------------------------+--------------------------------------+-------------------------------------------+ +| ``VIR_STEAL_PTR`` | ``g_steal_pointer`` | ``a = f(&b)`` instead of ``f(a, b)`` | ++-------------------------------+--------------------------------------+-------------------------------------------+ +| ``VIR_RETURN_PTR`` | ``return g_steal_pointer`` | | ++-------------------------------+--------------------------------------+-------------------------------------------+ +| ``ARRAY_CARDINALITY`` | ``G_N_ELEMENTS`` | | ++-------------------------------+--------------------------------------+-------------------------------------------+ +| ``ATTRIBUTE_FALLTHROUGH`` | ``G_GNUC_FALLTHROUGH`` | | ++-------------------------------+--------------------------------------+-------------------------------------------+ +| ``ATTRIBUTE_FMT_PRINTF`` | ``G_GNUC_PRINTF`` | | ++-------------------------------+--------------------------------------+-------------------------------------------+ +| ``ATTRIBUTE_NOINLINE`` | ``G_GNUC_NO_INLINE`` | | ++-------------------------------+--------------------------------------+-------------------------------------------+ +| ``ATTRIBUTE_NORETURN`` | ``G_GNUC_NORETURN`` | | ++-------------------------------+--------------------------------------+-------------------------------------------+ +| ``ATTRIBUTE_RETURN_CHECK`` | ``G_GNUC_WARN_UNUSED_RESULT`` | | ++-------------------------------+--------------------------------------+-------------------------------------------+ +| ``ATTRIBUTE_SENTINEL`` | ``G_GNUC_NULL_TERMINATED`` | | ++-------------------------------+--------------------------------------+-------------------------------------------+ +| ``ATTRIBUTE_UNUSED`` | ``G_GNUC_UNUSED`` | | ++-------------------------------+--------------------------------------+-------------------------------------------+ +| ``VIR_STRDUP`` | ``g_strdup`` | | ++-------------------------------+--------------------------------------+-------------------------------------------+ +| ``VIR_STRNDUP`` | ``g_strndup`` | | ++-------------------------------+--------------------------------------+-------------------------------------------+ +| ``virStrerror`` | ``g_strerror`` | | ++-------------------------------+--------------------------------------+-------------------------------------------+ + +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 replacement +functions provided by libvirt: + +:: + + virStrncpy(char *dest, const char *src, size_t n, size_t destbytes) + +The first two arguments have the same meaning as for strncpy, +namely the destination and source of the copy operation. Unlike +strncpy, the function will always copy exactly the number of bytes +requested and make sure the destination is NULL-terminated, as the +source is required to be; sanity checks are performed to ensure +the size of the destination, as specified by the last argument, is +sufficient for the operation to succeed. On success, 0 is +returned; on failure, a value <0 is returned instead. + +:: + + 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. + +:: + + 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. + +:: + + dst = g_strdup(src); + dst = g_strndup(src, n); + +You should avoid using strdup or strndup directly as they do not +handle out-of-memory errors, and do not allow a NULL source. Use +``g_strdup`` and ``g_strndup`` from GLib which abort on OOM and +handle NULL source by returning NULL. + +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 either the +`GString <https://developer.gnome.org/glib/stable/glib-Strings.html>`__ +type from GLib or the virBuffer API. If formatting XML or QEMU +command line is needed, use the virBuffer API described in +virbuffer.h, since it has helper functions for those. + +Typical usage is as follows: + +:: + + char * + somefunction(...) + { + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; + + ... + + virBufferAddLit(&buf, "<domain>\n"); + + ... + + if (some_error) + return NULL; /* g_auto will free the memory used so far */ + + ... + + 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 virCommandAddEnvFormat in +vircommand.h: + +:: + + void virCommandAddEnvFormat(virCommandPtr cmd, const char *format, ...) + G_GNUC_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 GString or virBuffer for +incremental allocations, g_strdup_printf for a one-shot +allocation, and g_snprintf for fixed-width buffers. Only use +g_sprintf, if you can prove the buffer won't overflow. + +Error message format +-------------------- + +Error messages visible to the user should be short and +descriptive. All error messages are translated using gettext and +thus must be wrapped in ``_()`` macro. To simplify the translation +work, the error message must not be concatenated from various +parts. To simplify searching for the error message in the code the +strings should not be broken even if they result into a line +longer than 80 columns and any formatting modifier should be +enclosed by quotes or other obvious separator. If a string used +with ``%s`` can be NULL the NULLSTR macro must be used. + +:: + + GOOD: virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to connect to remote host '%s'"), hostname) + + BAD: virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to %s to remote host '%s'"), + "connect", hostname); + + BAD: virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to connect " + "to remote host '%s'), + hostname); + +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. g_free() +and most of the functions named XXXFree() in libvirt is required +to handle NULL as its arg. This does not apply to libvirt's public +APIs. 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. Note that most of libvirt's type declarations can +be marked with either ``g_autofree`` or ``g_autoptr`` which uses +the compiler's ``__attribute__((cleanup))`` that calls the +appropriate free function when the variable goes out of scope. + +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. + +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. +- (ir)regular pulls from other repositories or automated updates, + such as the keycodemap submodule updates, pulling in new + translations or updating the container images for the CI system -- 2.25.1
