[libvirt PATCH 03/10] docs: Add coding-style.rst

This part represents the biggest chunk of the existing hacking.rst, and despite that its utility is very limited because 'make syntax-check' already guarantees most of the rules are followed over time. Until the glorious day we finally codify our coding style completely into a configuration for a tool such as clang-format and thus no longer need a plain English description of it, move this part to a separate page. Signed-off-by: Andrea Bolognani <abologna(a)redhat.com&gt; --- build-aux/syntax-check.mk | 2 +- docs/{hacking.rst => coding-style.rst} | 391 +---------- docs/hacking.rst | 914 ------------------------- 3 files changed, 4 insertions(+), 1303 deletions(-) copy docs/{hacking.rst => coding-style.rst} (64%) diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index 6ffea7afb9..8bee1bbbf1 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -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\.rst|tools/virt-login-shell\.c)$$ + ^(build-aux/syntax-check\.mk|docs/coding-style\.rst|tools/virt-login-shell\.c)$$ exclude_file_name_regexp--sc_prohibit_strtol = ^examples/.*$$ diff --git a/docs/hacking.rst b/docs/coding-style.rst similarity index 64% copy from docs/hacking.rst copy to docs/coding-style.rst index 4067b282a3..04baf9473c 100644 --- a/docs/hacking.rst +++ b/docs/coding-style.rst @@ -1,361 +1,9 @@ -====================== -Contributor guidelines -====================== +============ +Coding style +============ .. 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(a)redhat.com master - - For a single patch you can omit ``--cover-letter``, but a - series of two or more patches needs a cover letter. - - Note that the ``git send-email`` subcommand may not be in the - main git package and using it may require installation of a - separate package, for example the "git-email" package in Fedora - and Debian. If this is your first time using - ``git send-email``, you might need to configure it to point it - to your SMTP server with something like: - - :: - - git config --global sendemail.smtpServer stmp.youremailprovider.net - - If you get tired of typing ``--to=libvir-list(a)redhat.com`` all - the time, you can configure that to be automatically handled as - well: - - :: - - git config sendemail.to libvir-list(a)redhat.com - - As a rule, patches should be sent to the mailing list only: all - developers are subscribed to libvir-list and read it regularly, - so **please don't CC individual developers** unless they've - explicitly asked you to. - - Avoid using mail clients for sending patches, as most of them - will mangle the messages in some way, making them unusable for - our purposes. Gmail and other Web-based mail clients are - particularly bad at this. - - If everything went well, your patch should show up on the - `libvir-list - archives <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-project.... - -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 ================== @@ -1269,36 +917,3 @@ git): 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 diff --git a/docs/hacking.rst b/docs/hacking.rst index 4067b282a3..356dbc506b 100644 --- a/docs/hacking.rst +++ b/docs/hacking.rst @@ -356,920 +356,6 @@ include: - `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 | -+-------------------------------+------------------------------------------------------------+ - -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 ============================ -- 2.25.1