[libvirt PATCH 03/10] docs: Add coding-style.rst
Andrea Bolognani
abologna at redhat.com
Mon Apr 6 16:20:03 UTC 2020
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 at redhat.com>
---
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 at 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 at redhat.com`` all
- the time, you can configure that to be automatically handled as
- well:
-
- ::
-
- git config sendemail.to libvir-list at 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-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
==================
@@ -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
More information about the libvir-list
mailing list