mirror of
https://gitlab.com/libvirt/libvirt.git
synced 2024-08-30 12:31:16 +00:00
Signed-off-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
1558 lines
59 KiB
XML
1558 lines
59 KiB
XML
<?xml version="1.0" encoding="UTF-8"?>
|
|
<!DOCTYPE html>
|
|
<html xmlns="http://www.w3.org/1999/xhtml">
|
|
<body>
|
|
<h1>Contributor guidelines</h1>
|
|
|
|
<ul id="toc"></ul>
|
|
|
|
<h2><a id="patches">General tips for contributing patches</a></h2>
|
|
<ol>
|
|
<li>
|
|
<p>Discuss any large changes on the mailing list first. Post patches
|
|
early and listen to feedback.</p>
|
|
</li>
|
|
|
|
<li>
|
|
<p>Official upstream repository is kept in git
|
|
(<code>https://libvirt.org/git/libvirt.git</code>) and is browsable
|
|
along with other libvirt-related repositories
|
|
(e.g. libvirt-python) <a href="https://libvirt.org/git/">online</a>.</p>
|
|
</li>
|
|
|
|
<li>
|
|
<p>Patches to translations are maintained via
|
|
the <a href="https://fedora.zanata.org/">zanata project</a>.
|
|
If you want to fix a translation in a .po file, join the
|
|
appropriate language team. The libvirt release process
|
|
automatically pulls the latest version of each translation
|
|
file from zanata.</p>
|
|
</li>
|
|
|
|
<li><p>The simplest way to send patches is to use the
|
|
<a href="https://github.com/stefanha/git-publish"><code>git-publish</code></a>
|
|
tool. All libvirt-related repositories contain a config file that
|
|
tells git-publish to use the correct mailing list and subject prefix.</p>
|
|
<p>Alternatively, you may send patches using <code>git send-email</code>.</p>
|
|
<p>Also, for code motion patches, you may find that <code>git
|
|
diff --patience</code> provides an easier-to-read patch.
|
|
However, the usual workflow of libvirt developer is:</p>
|
|
<pre>
|
|
git checkout master
|
|
git pull
|
|
git checkout -t origin -b workbranch
|
|
Hack, committing any changes along the way
|
|
</pre>
|
|
<p>More hints on compiling can be
|
|
found <a href="compiling.html">here</a>. When you want to
|
|
post your patches:</p>
|
|
<pre>
|
|
git pull --rebase
|
|
(fix any conflicts)
|
|
git send-email --cover-letter --no-chain-reply-to --annotate \
|
|
--confirm=always --to=libvir-list@redhat.com master
|
|
</pre>
|
|
<p>For a single patch you can omit
|
|
<code>--cover-letter</code>, but a series of two or more
|
|
patches needs a cover letter.</p>
|
|
<p>Note that the <code>git send-email</code> subcommand may not
|
|
be in the main git package and using it may require installation
|
|
of a separate package, for example the "git-email" package in
|
|
Fedora and Debian. If this is your first time using
|
|
<code>git send-email</code>, you might need to configure it to
|
|
point it to your SMTP server with something like:</p>
|
|
<pre>
|
|
git config --global sendemail.smtpServer stmp.youremailprovider.net
|
|
</pre>
|
|
<p>If you get tired of typing
|
|
<code>--to=libvir-list@redhat.com</code> all the time, you can
|
|
configure that to be automatically handled as well:</p>
|
|
<pre>
|
|
git config sendemail.to libvir-list@redhat.com
|
|
</pre>
|
|
<p>As a rule, patches should be sent to the mailing list only: all
|
|
developers are subscribed to libvir-list and read it regularly, so
|
|
<strong>please don't CC individual developers</strong> unless
|
|
they've explicitly asked you to.</p>
|
|
<p>Avoid using mail clients for sending patches, as most of them
|
|
will mangle the messages in some way, making them unusable for our
|
|
purposes. Gmail and other Web-based mail clients are particularly
|
|
bad at this.</p>
|
|
<p>If everything went well, your patch should show up on the
|
|
<a href="https://www.redhat.com/archives/libvir-list/">libvir-list
|
|
archives</a> in a matter of minutes; if you still can't find it on
|
|
there after an hour or so, you should double-check your setup.
|
|
<strong>Note that, if you are not already a subscriber, your very
|
|
first post to the mailing list will be
|
|
subject to moderation</strong>, and it's not uncommon for that to
|
|
take around a day.</p>
|
|
<p>Please follow this as close as you can, especially the rebase and
|
|
<code>git send-email</code> part, as it makes life easier for other
|
|
developers to review your patch set.</p>
|
|
<p>One should avoid sending patches as attachments,
|
|
but rather send them in email body along with commit message. If a
|
|
developer is sending another version of the patch (e.g. to address
|
|
review comments), they are advised to note differences to previous
|
|
versions after the <code>---</code> line in the patch so that it helps
|
|
reviewers but doesn't become part of git history. Moreover, such patch
|
|
needs to be prefixed correctly with
|
|
<code>--subject-prefix=PATCHv2</code> appended to <code>git
|
|
send-email</code> (substitute <code>v2</code> with the correct
|
|
version if needed though).</p>
|
|
</li>
|
|
|
|
<li><p>In your commit message, make the summary line reasonably
|
|
short (60 characters is typical), followed by a blank line,
|
|
followed by any longer description of why your patch makes
|
|
sense. If the patch fixes a regression, and you know what
|
|
commit introduced the problem, mentioning that is useful.
|
|
If the patch resolves a bugzilla report, mentioning the URL
|
|
of the bug number is useful; but also summarize the issue
|
|
rather than making all readers follow the link. You can use
|
|
'git shortlog -30' to get an idea of typical summary lines.
|
|
</p>
|
|
</li>
|
|
|
|
<li><p>Contributors to libvirt projects <strong>must</strong>
|
|
assert that they are in compliance with the
|
|
<a href="https://developercertificate.org/">Developer
|
|
Certificate of Origin 1.1</a>. This is achieved by adding
|
|
a "Signed-off-by" line containing the contributor's name
|
|
and e-mail to every commit message. The presence
|
|
of this line attests that the contributor has read the
|
|
above lined DCO and agrees with its statements.
|
|
</p></li>
|
|
|
|
<li><p>Split large changes into a series of smaller patches,
|
|
self-contained if possible, with an explanation of each patch
|
|
and an explanation of how the sequence of patches fits
|
|
together. Moreover, please keep in mind that it's required to
|
|
be able to compile cleanly (<b>including</b> <code>make
|
|
check</code> and <code>make syntax-check</code>) after each
|
|
patch. A feature does not have to work until the end of a
|
|
series, but intermediate patches must compile and not cause
|
|
test-suite failures (this is to preserve the usefulness
|
|
of <code>git bisect</code>, among other things).</p>
|
|
</li>
|
|
|
|
<li>
|
|
<p>Make sure your patches apply against libvirt GIT. Developers
|
|
only follow GIT and don't care much about released versions.</p>
|
|
</li>
|
|
|
|
<li><p>Run the automated tests on your code before submitting any changes.
|
|
That is:
|
|
</p>
|
|
<pre>
|
|
make check
|
|
make syntax-check
|
|
make -C tests valgrind
|
|
</pre>
|
|
<p><a href="http://valgrind.org/">Valgrind</a> is a test that checks
|
|
for memory management issues, such as leaks or use of uninitialized
|
|
variables.
|
|
</p>
|
|
|
|
<p>
|
|
Some tests are skipped by default in a development environment,
|
|
based on the time they take in comparison to the likelihood
|
|
that those tests will turn up problems during incremental builds.
|
|
These tests default to being run when building from a
|
|
tarball or with the configure option --enable-expensive-tests;
|
|
you can also force a one-time toggle of these tests by
|
|
setting VIR_TEST_EXPENSIVE to 0 or 1 at make time, as in:
|
|
</p>
|
|
<pre>
|
|
make check VIR_TEST_EXPENSIVE=1
|
|
</pre>
|
|
<p>
|
|
If you encounter any failing tests, the VIR_TEST_DEBUG
|
|
environment variable may provide extra information to debug
|
|
the failures. Larger values of VIR_TEST_DEBUG may provide
|
|
larger amounts of information:
|
|
</p>
|
|
|
|
<pre>
|
|
VIR_TEST_DEBUG=1 make check (or)
|
|
VIR_TEST_DEBUG=2 make check
|
|
</pre>
|
|
|
|
<p>
|
|
When debugging failures during development, it is possible
|
|
to focus in on just the failing subtests by using
|
|
VIR_TEST_RANGE. I.e. to run all tests from 3 to 20 with the
|
|
exception of tests 6 and 16, use:
|
|
</p>
|
|
|
|
<pre>
|
|
VIR_TEST_DEBUG=1 VIR_TEST_RANGE=3-5,7-20,^16 ./run tests/qemuxml2argvtest
|
|
</pre>
|
|
|
|
<p>
|
|
Also, individual tests can be run from inside the <code>tests/</code>
|
|
directory, like:
|
|
</p>
|
|
<pre>
|
|
./qemuxml2xmltest
|
|
</pre>
|
|
|
|
<p>
|
|
If you are adding new test cases, or making changes that alter
|
|
existing test output, you can use the environment variable
|
|
VIR_TEST_REGENERATE_OUTPUT to quickly update the saved test data.
|
|
Of course you still need to review the changes VERY CAREFULLY to
|
|
ensure they are correct.
|
|
</p>
|
|
<pre>
|
|
VIR_TEST_REGENERATE_OUTPUT=1 ./qemuxml2argvtest
|
|
</pre>
|
|
|
|
<p>There is also a <code>./run</code> script at the top level,
|
|
to make it easier to run programs that have not yet been
|
|
installed, as well as to wrap invocations of various tests
|
|
under gdb or Valgrind.
|
|
</p>
|
|
|
|
<p>When running our test suite it may happen that the test result is
|
|
nondeterministic because of the test suite relying on a particular file
|
|
in the system being accessible or having some specific value. To catch
|
|
this kind of errors, the test suite has a module for that prints any
|
|
path touched that fulfils constraints described above
|
|
into a file. To enable it just set
|
|
<code>VIR_TEST_FILE_ACCESS</code> environment variable.
|
|
Then <code>VIR_TEST_FILE_ACCESS_OUTPUT</code> environment
|
|
variable can alter location where the file is stored.</p>
|
|
<pre>
|
|
VIR_TEST_FILE_ACCESS=1 VIR_TEST_FILE_ACCESS_OUTPUT="/tmp/file_access.txt" ./qemuxml2argvtest
|
|
</pre>
|
|
|
|
</li>
|
|
<li><p>The Valgrind test should produce similar output to
|
|
<code>make check</code>. If the output has traces within libvirt
|
|
API's, then investigation is required in order to determine the
|
|
cause of the issue. Output such as the following indicates some
|
|
sort of leak:
|
|
</p>
|
|
<pre>
|
|
==5414== 4 bytes in 1 blocks are definitely lost in loss record 3 of 89
|
|
==5414== at 0x4A0881C: malloc (vg_replace_malloc.c:270)
|
|
==5414== by 0x34DE0AAB85: xmlStrndup (in /usr/lib64/libxml2.so.2.7.8)
|
|
==5414== by 0x4CC97A6: virDomainVideoDefParseXML (domain_conf.c:7410)
|
|
==5414== by 0x4CD581D: virDomainDefParseXML (domain_conf.c:10188)
|
|
==5414== by 0x4CD8C73: virDomainDefParseNode (domain_conf.c:10640)
|
|
==5414== by 0x4CD8DDB: virDomainDefParse (domain_conf.c:10590)
|
|
==5414== by 0x41CB1D: testCompareXMLToArgvHelper (qemuxml2argvtest.c:100)
|
|
==5414== by 0x41E20F: virtTestRun (testutils.c:161)
|
|
==5414== by 0x41C7CB: mymain (qemuxml2argvtest.c:866)
|
|
==5414== by 0x41E84A: virtTestMain (testutils.c:723)
|
|
==5414== by 0x34D9021734: (below main) (in /usr/lib64/libc-2.15.so)
|
|
</pre>
|
|
<p>In this example, the <code>virDomainDefParseXML()</code> had
|
|
an error path where the <code>virDomainVideoDefPtr video</code>
|
|
pointer was not properly disposed. By simply adding a
|
|
<code>virDomainVideoDefFree(video);</code> in the error path,
|
|
the issue was resolved.
|
|
</p>
|
|
|
|
<p>Another common mistake is calling a printing function, such as
|
|
<code>VIR_DEBUG()</code> without initializing a variable to be
|
|
printed. The following example involved a call which could return
|
|
an error, but not set variables passed by reference to the call.
|
|
The solution was to initialize the variables prior to the call.
|
|
</p>
|
|
<pre>
|
|
==4749== Use of uninitialised value of size 8
|
|
==4749== at 0x34D904650B: _itoa_word (in /usr/lib64/libc-2.15.so)
|
|
==4749== by 0x34D9049118: vfprintf (in /usr/lib64/libc-2.15.so)
|
|
==4749== by 0x34D9108F60: __vasprintf_chk (in /usr/lib64/libc-2.15.so)
|
|
==4749== by 0x4CAEEF7: virVasprintf (stdio2.h:199)
|
|
==4749== by 0x4C8A55E: virLogVMessage (virlog.c:814)
|
|
==4749== by 0x4C8AA96: virLogMessage (virlog.c:751)
|
|
==4749== by 0x4DA0056: virNetTLSContextCheckCertKeyUsage (virnettlscontext.c:225)
|
|
==4749== by 0x4DA06DB: virNetTLSContextCheckCert (virnettlscontext.c:439)
|
|
==4749== by 0x4DA1620: virNetTLSContextNew (virnettlscontext.c:562)
|
|
==4749== by 0x4DA26FC: virNetTLSContextNewServer (virnettlscontext.c:927)
|
|
==4749== by 0x409C39: testTLSContextInit (virnettlscontexttest.c:467)
|
|
==4749== by 0x40AB8F: virtTestRun (testutils.c:161)
|
|
</pre>
|
|
<p>Valgrind will also find some false positives or code paths
|
|
which cannot be resolved by making changes to the libvirt code.
|
|
For these paths, it is possible to add a filter to avoid the
|
|
errors. For example:
|
|
</p>
|
|
<pre>
|
|
==4643== 7 bytes in 1 blocks are possibly lost in loss record 4 of 20
|
|
==4643== at 0x4A0881C: malloc (vg_replace_malloc.c:270)
|
|
==4643== by 0x34D90853F1: strdup (in /usr/lib64/libc-2.15.so)
|
|
==4643== by 0x34EEC2C08A: ??? (in /usr/lib64/libnl.so.1.1)
|
|
==4643== by 0x34EEC15B81: ??? (in /usr/lib64/libnl.so.1.1)
|
|
==4643== by 0x34D8C0EE15: call_init.part.0 (in /usr/lib64/ld-2.15.so)
|
|
==4643== by 0x34D8C0EECF: _dl_init (in /usr/lib64/ld-2.15.so)
|
|
==4643== by 0x34D8C01569: ??? (in /usr/lib64/ld-2.15.so)
|
|
|
|
</pre>
|
|
<p>In this instance, it is acceptable to modify the
|
|
<code>tests/.valgrind.supp</code> file in order to add a
|
|
suppression filter. The filter should be unique enough to
|
|
not suppress real leaks, but it should be generic enough to
|
|
cover multiple code paths. The format of the entry can be
|
|
found in the documentation found at the
|
|
<a href="http://valgrind.org/">Valgrind home page</a>.
|
|
The following trace was added to <code>tests/.valgrind.supp</code>
|
|
in order to suppress the warning:
|
|
</p>
|
|
<pre>
|
|
{
|
|
dlInitMemoryLeak1
|
|
Memcheck:Leak
|
|
fun:?alloc
|
|
...
|
|
fun:call_init.part.0
|
|
fun:_dl_init
|
|
...
|
|
obj:*/lib*/ld-2.*so*
|
|
}
|
|
</pre>
|
|
</li>
|
|
|
|
<li>
|
|
<p>Update tests and/or documentation, particularly if you are adding
|
|
a new feature or changing the output of a program.</p>
|
|
</li>
|
|
|
|
<li>
|
|
<p>Don't forget to update the <a href="news.html">release notes</a>
|
|
by changing <code>docs/news.xml</code> if your changes are
|
|
significant. All user-visible changes, such as adding new XML elements
|
|
or fixing all but the most obscure bugs, must be (briefly) described
|
|
in a release notes entry; changes that are only relevant to other
|
|
libvirt developers, such as code refactoring, don't belong in the
|
|
release notes. Note that <code>docs/news.xml</code> should be updated
|
|
in its own commit not to get in the way of backports.</p>
|
|
</li>
|
|
</ol>
|
|
|
|
<p>
|
|
There is more on this subject, including lots of links to background
|
|
reading on the subject, on
|
|
<a href="http://people.redhat.com/rjones/how-to-supply-code-to-open-source-projects/">
|
|
Richard Jones' guide to working with open source projects</a>.
|
|
</p>
|
|
|
|
<h2><a id="lang">Language Usage</a></h2>
|
|
|
|
<p>
|
|
The libvirt repository makes use of a large number of programming
|
|
languages. It is anticipated that in the future libvirt will adopt
|
|
use of other new languages. To reduce the overall burden on developers,
|
|
there is thus a general desire to phase out usage of some of the
|
|
existing languages.
|
|
</p>
|
|
|
|
<p>
|
|
The preferred languages at this time are:
|
|
</p>
|
|
|
|
<ul>
|
|
<li>C - for the main libvirt codebase. Dialect supported by
|
|
GCC/CLang only.</li>
|
|
<li>Python - for supporting build scripts / tools. Code must
|
|
run with both version 2.7 and 3.x at this time.</li>
|
|
</ul>
|
|
|
|
<p>
|
|
Languages that should not be used for any new contributions:
|
|
</p>
|
|
|
|
<ul>
|
|
<li>Perl - build scripts must be written in Python instead.</li>
|
|
<li>Shell - build scripts must be written in Python instead.</li>
|
|
</ul>
|
|
|
|
<h2><a id="tooling">Tooling</a></h2>
|
|
|
|
<p>
|
|
libvirt includes support for some useful development tools right in its
|
|
source repository, meaning users will be able to take advantage of them
|
|
without little or no configuration. Examples include:
|
|
</p>
|
|
|
|
<ul>
|
|
<li>
|
|
<a href="https://github.com/jeaye/color_coded">color_coded</a>,
|
|
a vim plugin for libclang-powered semantic syntax highlighting;
|
|
</li>
|
|
|
|
<li>
|
|
<a href="http://valloric.github.io/YouCompleteMe/">YouCompleteMe</a>,
|
|
a vim plugin for libclang-powered semantic code completion.
|
|
</li>
|
|
</ul>
|
|
|
|
<h2><a id="naming">Naming conventions</a></h2>
|
|
|
|
<p>
|
|
When reading libvirt code, a number of different naming conventions will
|
|
be evident due to various changes in thinking over the course of the
|
|
project's lifetime. The conventions documented below should be followed
|
|
when creating any entirely new files in libvirt. When working on existing
|
|
files, while it is desirable to apply these conventions, keeping a
|
|
consistent style with existing code in that particular file is generally
|
|
more important. The overall guiding principal is that every file, enum,
|
|
struct, function, macro and typedef name must have a 'vir' or 'VIR' prefix.
|
|
All local scope variable names are exempt, and global variables are exempt,
|
|
unless exported in a header file.
|
|
</p>
|
|
|
|
<dl>
|
|
<dt>File names</dt>
|
|
<dd>
|
|
<p>
|
|
File naming varies depending on the subdirectory. The preferred
|
|
style is to have a 'vir' prefix, followed by a name which matches
|
|
the name of the functions / objects inside the file. For example,
|
|
a file containing an object 'virHashtable' is stored in files
|
|
'virhashtable.c' and 'virhashtable.h'. Sometimes, methods which
|
|
would otherwise be declared 'static' need to be exported for use
|
|
by a test suite. For this purpose a second header file should be
|
|
added with a suffix of 'priv', e.g. 'virhashtablepriv.h'. Use of
|
|
underscores in file names is discouraged when using the 'vir'
|
|
prefix style. The 'vir' prefix naming applies to src/util,
|
|
src/rpc and tests/ directories. Most other directories do not
|
|
follow this convention.
|
|
</p>
|
|
</dd>
|
|
<dt>Enum type & field names</dt>
|
|
<dd>
|
|
<p>
|
|
All enums should have a 'vir' prefix in their typedef name,
|
|
and each following word should have its first letter in
|
|
uppercase. The enum name should match the typedef name with
|
|
a leading underscore. The enum member names should be in all
|
|
uppercase, and use an underscore to separate each word. The
|
|
enum member name prefix should match the enum typedef name.
|
|
</p>
|
|
<pre>
|
|
typedef enum _virSocketType virSocketType;
|
|
enum _virSocketType {
|
|
VIR_SOCKET_TYPE_IPV4,
|
|
VIR_SOCKET_TYPE_IPV6,
|
|
};</pre>
|
|
</dd>
|
|
<dt>Struct type names</dt>
|
|
<dd>
|
|
<p>
|
|
All structs should have a 'vir' prefix in their typedef name,
|
|
and each following word should have its first letter in
|
|
uppercase. The struct name should be the same as the typedef
|
|
name with a leading underscore. A second typedef should be
|
|
given for a pointer to the struct with a 'Ptr' suffix.
|
|
</p>
|
|
<pre>
|
|
typedef struct _virHashTable virHashTable;
|
|
typedef virHashTable *virHashTablePtr;
|
|
struct _virHashTable {
|
|
...
|
|
};</pre>
|
|
</dd>
|
|
<dt>Function names</dt>
|
|
<dd>
|
|
<p>
|
|
All functions should have a 'vir' prefix in their name,
|
|
followed by one or more words with first letter of each
|
|
word capitalized. Underscores should not be used in function
|
|
names. If the function is operating on an object, then the
|
|
function name prefix should match the object typedef name,
|
|
otherwise it should match the filename. Following this
|
|
comes the verb / action name, and finally an optional
|
|
subject name. For example, given an object 'virHashTable',
|
|
all functions should have a name 'virHashTable$VERB' or
|
|
'virHashTable$VERB$SUBJECT", e.g. 'virHashTableLookup'
|
|
or 'virHashTableGetValue'.
|
|
</p>
|
|
</dd>
|
|
<dt>Macro names</dt>
|
|
<dd>
|
|
<p>
|
|
All macros should have a "VIR" prefix in their name, followed
|
|
by one or more uppercase words separated by underscores. The
|
|
macro argument names should be in lowercase. Aside from having
|
|
a "VIR" prefix there are no common practices for the rest of
|
|
the macro name.
|
|
</p>
|
|
</dd>
|
|
</dl>
|
|
|
|
<h2><a id="indent">Code indentation</a></h2>
|
|
<p>
|
|
Libvirt's C source code generally adheres to some basic code-formatting
|
|
conventions. The existing code base is not totally consistent on this
|
|
front, but we do prefer that contributed code be formatted similarly.
|
|
In short, use spaces-not-TABs for indentation, use 4 spaces for each
|
|
indentation level, and other than that, follow the K&R style.
|
|
</p>
|
|
|
|
<p>
|
|
If you use Emacs, the project includes a file .dir-locals.el
|
|
that sets up the preferred indentation. If you use vim,
|
|
append the following to your ~/.vimrc file:
|
|
</p>
|
|
<pre>
|
|
set nocompatible
|
|
filetype on
|
|
set autoindent
|
|
set smartindent
|
|
set cindent
|
|
set tabstop=8
|
|
set shiftwidth=4
|
|
set expandtab
|
|
set cinoptions=(0,:0,l1,t0,L3
|
|
filetype plugin indent on
|
|
au FileType make setlocal noexpandtab
|
|
au BufRead,BufNewFile *.am setlocal noexpandtab
|
|
match ErrorMsg /\s\+$\| \+\ze\t/
|
|
</pre>
|
|
<p>
|
|
Or if you don't want to mess your ~/.vimrc up, you can save the above
|
|
into a file called .lvimrc (not .vimrc) located at the root of libvirt
|
|
source, then install a vim script from
|
|
http://www.vim.org/scripts/script.php?script_id=1408,
|
|
which will load the .lvimrc only when you edit libvirt code.
|
|
</p>
|
|
|
|
<h2><a id="formatting">Code formatting (especially for new code)</a></h2>
|
|
|
|
<p>
|
|
With new code, we can be even more strict.
|
|
Please apply the following function (using GNU indent) to any new code.
|
|
Note that this also gives you an idea of the type of spacing we prefer
|
|
around operators and keywords:
|
|
</p>
|
|
|
|
<pre>
|
|
indent-libvirt()
|
|
{
|
|
indent -bad -bap -bbb -bli4 -br -ce -brs -cs -i4 -l75 -lc75 \
|
|
-sbi4 -psl -saf -sai -saw -sbi4 -ss -sc -cdw -cli4 -npcs -nbc \
|
|
--no-tabs "$@"
|
|
}
|
|
</pre>
|
|
|
|
<p>
|
|
Note that sometimes you'll have to post-process that output further, by
|
|
piping it through <code>expand -i</code>, since some leading TABs can get through.
|
|
Usually they're in macro definitions or strings, and should be converted
|
|
anyhow.
|
|
</p>
|
|
|
|
<p>
|
|
Libvirt requires a C99 compiler for various reasons. However,
|
|
most of the code base prefers to stick to C89 syntax unless
|
|
there is a compelling reason otherwise. For example, it is
|
|
preferable to use <code>/* */</code> comments rather
|
|
than <code>//</code>. Also, when declaring local variables, the
|
|
prevailing style has been to declare them at the beginning of a
|
|
scope, rather than immediately before use.
|
|
</p>
|
|
|
|
|
|
<h2><a id="bracket_spacing">Bracket spacing</a></h2>
|
|
|
|
<p>
|
|
The keywords <code>if</code>, <code>for</code>, <code>while</code>,
|
|
and <code>switch</code> must have a single space following them
|
|
before the opening bracket. E.g.
|
|
</p>
|
|
<pre>
|
|
if(foo) // Bad
|
|
if (foo) // Good
|
|
</pre>
|
|
|
|
<p>
|
|
Function implementations must <strong>not</strong> have any whitespace
|
|
between the function name and the opening bracket. E.g.
|
|
</p>
|
|
<pre>
|
|
int foo (int wizz) // Bad
|
|
int foo(int wizz) // Good
|
|
</pre>
|
|
|
|
<p>
|
|
Function calls must <strong>not</strong> have any whitespace
|
|
between the function name and the opening bracket. E.g.
|
|
</p>
|
|
<pre>
|
|
bar = foo (wizz); // Bad
|
|
bar = foo(wizz); // Good
|
|
</pre>
|
|
|
|
<p>
|
|
Function typedefs must <strong>not</strong> have any whitespace
|
|
between the closing bracket of the function name and opening
|
|
bracket of the arg list. E.g.
|
|
</p>
|
|
<pre>
|
|
typedef int (*foo) (int wizz); // Bad
|
|
typedef int (*foo)(int wizz); // Good
|
|
</pre>
|
|
|
|
<p>
|
|
There must not be any whitespace immediately following any
|
|
opening bracket, or immediately prior to any closing bracket. E.g.
|
|
</p>
|
|
<pre>
|
|
int foo( int wizz ); // Bad
|
|
int foo(int wizz); // Good
|
|
</pre>
|
|
|
|
<h2><a id="comma">Commas</a></h2>
|
|
|
|
<p>
|
|
Commas should always be followed by a space or end of line, and
|
|
never have leading space; this is enforced during 'make
|
|
syntax-check'.
|
|
</p>
|
|
<pre>
|
|
call(a,b ,c);// Bad
|
|
call(a, b, c); // Good
|
|
</pre>
|
|
|
|
<p>
|
|
When declaring an enum or using a struct initializer that
|
|
occupies more than one line, use a trailing comma. That way,
|
|
future edits to extend the list only have to add a line, rather
|
|
than modify an existing line to add the intermediate comma. Any
|
|
sentinel enumerator value with a name ending in _LAST is exempt,
|
|
since you would extend such an enum before the _LAST element.
|
|
Another reason to favor trailing commas is that it requires less
|
|
effort to produce via code generators. Note that the syntax
|
|
checker is unable to enforce a style of trailing commas, so
|
|
there are counterexamples in existing code which do not use it;
|
|
also, while C99 allows trailing commas, remember that JSON and
|
|
XDR do not.
|
|
</p>
|
|
<pre>
|
|
enum {
|
|
VALUE_ONE,
|
|
VALUE_TWO // Bad
|
|
};
|
|
enum {
|
|
VALUE_THREE,
|
|
VALUE_FOUR, // Good
|
|
};
|
|
</pre>
|
|
|
|
<h2><a id="semicolon">Semicolons</a></h2>
|
|
|
|
<p>
|
|
Semicolons should never have a space beforehand. Inside the
|
|
condition of a <code>for</code> loop, there should always be a
|
|
space or line break after each semicolon, except for the special
|
|
case of an infinite loop (although more infinite loops
|
|
use <code>while</code>). While not enforced, loop counters
|
|
generally use post-increment.
|
|
</p>
|
|
<pre>
|
|
for (i = 0 ;i < limit ; ++i) { // Bad
|
|
for (i = 0; i < limit; i++) { // Good
|
|
for (;;) { // ok
|
|
while (1) { // Better
|
|
</pre>
|
|
<p>
|
|
Empty loop bodies are better represented with curly braces and a
|
|
comment, although use of a semicolon is not currently rejected.
|
|
</p>
|
|
<pre>
|
|
while ((rc = waitpid(pid, &st, 0) == -1) &&
|
|
errno == EINTR); // ok
|
|
while ((rc = waitpid(pid, &st, 0) == -1) &&
|
|
errno == EINTR) { // Better
|
|
/* nothing */
|
|
}
|
|
</pre>
|
|
|
|
<h2><a id="curly_braces">Curly braces</a></h2>
|
|
|
|
<p>
|
|
Omit the curly braces around an <code>if</code>, <code>while</code>,
|
|
<code>for</code> etc. body only when both that body and the condition
|
|
itself occupy a single line. In every other case we require
|
|
the braces. This ensures that it is trivially easy to identify a
|
|
single-<i>statement</i> loop: each has only one <i>line</i> in its body.
|
|
</p>
|
|
|
|
<pre>
|
|
while (expr) // single line body; {} is forbidden
|
|
single_line_stmt();
|
|
</pre>
|
|
|
|
<pre>
|
|
while (expr(arg1,
|
|
arg2)) // indentation makes it obvious it is single line,
|
|
single_line_stmt(); // {} is optional (not enforced either way)
|
|
</pre>
|
|
|
|
<pre>
|
|
while (expr1 &&
|
|
expr2) { // multi-line, at same indentation, {} required
|
|
single_line_stmt();
|
|
}
|
|
</pre>
|
|
|
|
<p>
|
|
However, the moment your loop/if/else body extends on to a second
|
|
line, for whatever reason (even if it's just an added comment), then
|
|
you should add braces. Otherwise, it would be too easy to insert a
|
|
statement just before that comment (without adding braces), thinking
|
|
it is already a multi-statement loop:
|
|
</p>
|
|
|
|
<pre>
|
|
while (true) // BAD! multi-line body with no braces
|
|
/* comment... */
|
|
single_line_stmt();
|
|
</pre>
|
|
<p>
|
|
Do this instead:
|
|
</p>
|
|
<pre>
|
|
while (true) { // Always put braces around a multi-line body.
|
|
/* comment... */
|
|
single_line_stmt();
|
|
}
|
|
</pre>
|
|
<p>
|
|
There is one exception: when the second body line is not at the same
|
|
indentation level as the first body line:
|
|
</p>
|
|
<pre>
|
|
if (expr)
|
|
die("a diagnostic that would make this line"
|
|
" extend past the 80-column limit"));
|
|
</pre>
|
|
|
|
<p>
|
|
It is safe to omit the braces in the code above, since the
|
|
further-indented second body line makes it obvious that this is still
|
|
a single-statement body.
|
|
</p>
|
|
|
|
<p>
|
|
To reiterate, don't do this:
|
|
</p>
|
|
|
|
<pre>
|
|
if (expr) // BAD: no braces around...
|
|
while (expr_2) { // ... a multi-line body
|
|
...
|
|
}
|
|
</pre>
|
|
|
|
<p>
|
|
Do this, instead:
|
|
</p>
|
|
|
|
<pre>
|
|
if (expr) {
|
|
while (expr_2) {
|
|
...
|
|
}
|
|
}
|
|
</pre>
|
|
|
|
<p>
|
|
However, there is one exception in the other direction, when even a
|
|
one-line block should have braces. That occurs when that one-line,
|
|
brace-less block is an <code>if</code> or <code>else</code>
|
|
block, and the counterpart block <b>does</b> use braces. In
|
|
that case, put braces around both blocks. Also, if
|
|
the <code>else</code> block is much shorter than
|
|
the <code>if</code> block, consider negating the
|
|
<code>if</code>-condition and swapping the bodies, putting the
|
|
short block first and making the longer, multi-line block be the
|
|
<code>else</code> block.
|
|
</p>
|
|
|
|
<pre>
|
|
if (expr) {
|
|
...
|
|
...
|
|
}
|
|
else
|
|
x = y; // BAD: braceless "else" with braced "then",
|
|
// and short block last
|
|
|
|
if (expr)
|
|
x = y; // BAD: braceless "if" with braced "else"
|
|
else {
|
|
...
|
|
...
|
|
}
|
|
</pre>
|
|
|
|
<p>
|
|
Keeping braces consistent and putting the short block first is
|
|
preferred, especially when the multi-line body is more than a
|
|
few lines long, because it is easier to read and grasp the semantics of
|
|
an if-then-else block when the simpler block occurs first, rather than
|
|
after the more involved block:
|
|
</p>
|
|
|
|
<pre>
|
|
if (!expr) {
|
|
x = y; // putting the smaller block first is more readable
|
|
} else {
|
|
...
|
|
...
|
|
}
|
|
</pre>
|
|
|
|
<p>
|
|
But if negating a complex condition is too ugly, then at least
|
|
add braces:
|
|
</p>
|
|
|
|
<pre>
|
|
if (complex expr not worth negating) {
|
|
...
|
|
...
|
|
} else {
|
|
x = y;
|
|
}
|
|
</pre>
|
|
|
|
<p>Use hanging braces for compound statements: the opening brace
|
|
of a compound statement should be on the same line as the
|
|
condition being tested. Only top-level function bodies, nested
|
|
scopes, and compound structure declarations should ever have {
|
|
on a line by itself.
|
|
</p>
|
|
|
|
<pre>
|
|
void
|
|
foo(int a, int b)
|
|
{ // correct - function body
|
|
int 2d[][] = {
|
|
{ // correct - complex initialization
|
|
1, 2,
|
|
},
|
|
};
|
|
if (a)
|
|
{ // BAD: compound brace on its own line
|
|
do_stuff();
|
|
}
|
|
{ // correct - nested scope
|
|
int tmp;
|
|
if (a < b) { // correct - hanging brace
|
|
tmp = b;
|
|
b = a;
|
|
a = tmp;
|
|
}
|
|
}
|
|
}
|
|
</pre>
|
|
|
|
<h2><a id="conditions">Conditional expressions</a></h2>
|
|
<p>For readability reasons new code should avoid shortening comparisons
|
|
to 0 for numeric types. Boolean and pointer comparisions may be
|
|
shortened. All long forms are okay:
|
|
</p>
|
|
<pre>
|
|
virFooPtr foos = NULL;
|
|
size nfoos = 0;
|
|
bool hasFoos = false;
|
|
|
|
GOOD:
|
|
if (!foos)
|
|
if (!hasFoos)
|
|
if (nfoos == 0)
|
|
if (foos == NULL)
|
|
if (hasFoos == true)
|
|
|
|
BAD:
|
|
if (!nfoos)
|
|
if (nfoos)
|
|
</pre>
|
|
<p>New code should avoid the ternary operator as much as possible.
|
|
Specifically it must never span more than one line or nest:
|
|
</p>
|
|
<pre>
|
|
BAD:
|
|
char *foo = baz ?
|
|
virDoSomethingReallyComplex(driver, vm, something, baz->foo) :
|
|
NULL;
|
|
|
|
char *foo = bar ? bar->baz ? bar->baz->foo : "nobaz" : "nobar";
|
|
</pre>
|
|
|
|
<h2><a id="preprocessor">Preprocessor</a></h2>
|
|
|
|
<p>Macros defined with an ALL_CAPS name should generally be
|
|
assumed to be unsafe with regards to arguments with side-effects
|
|
(that is, MAX(a++, b--) might increment a or decrement b too
|
|
many or too few times). Exceptions to this rule are explicitly
|
|
documented for macros in viralloc.h and virstring.h.
|
|
</p>
|
|
|
|
<p>
|
|
For variadic macros, stick with C99 syntax:
|
|
</p>
|
|
<pre>
|
|
#define vshPrint(_ctl, ...) fprintf(stdout, __VA_ARGS__)
|
|
</pre>
|
|
|
|
<p>Use parenthesis when checking if a macro is defined, and use
|
|
indentation to track nesting:
|
|
</p>
|
|
<pre>
|
|
#if defined(HAVE_POSIX_FALLOCATE) && !defined(HAVE_FALLOCATE)
|
|
# define fallocate(a, ignored, b, c) posix_fallocate(a, b, c)
|
|
#endif
|
|
</pre>
|
|
|
|
<h2><a id="types">C types</a></h2>
|
|
|
|
<p>
|
|
Use the right type.
|
|
</p>
|
|
|
|
<h3>Scalars</h3>
|
|
|
|
<ul>
|
|
<li>If you're using <code>int</code> or <code>long</code>, odds are
|
|
good that there's a better type.</li>
|
|
<li>If a variable is counting something, be sure to declare it with an
|
|
unsigned type.</li>
|
|
<li>If it's memory-size-related, use <code>size_t</code> (use
|
|
<code>ssize_t</code> only if required).</li>
|
|
<li>If it's file-size related, use uintmax_t, or maybe <code>off_t</code>.</li>
|
|
<li>If it's file-offset related (i.e., signed), use <code>off_t</code>.</li>
|
|
<li>If it's just counting small numbers use <code>unsigned int</code>;
|
|
(on all but oddball embedded systems, you can assume that that
|
|
type is at least four bytes wide).</li>
|
|
<li>If a variable has boolean semantics, give it the <code>bool</code> type
|
|
and use the corresponding <code>true</code> and <code>false</code> macros.
|
|
It's ok to include <stdbool.h>, since libvirt's use of gnulib ensures
|
|
that it exists and is usable.</li>
|
|
<li>In the unusual event that you require a specific width, use a
|
|
standard type like <code>int32_t</code>, <code>uint32_t</code>,
|
|
<code>uint64_t</code>, etc.</li>
|
|
<li>While using <code>bool</code> is good for readability, it comes with
|
|
minor caveats:
|
|
<ul>
|
|
<li>Don't use <code>bool</code> in places where the type size must be constant across
|
|
all systems, like public interfaces and on-the-wire protocols. Note
|
|
that it would be possible (albeit wasteful) to use <code>bool</code> in libvirt's
|
|
logical wire protocol, since XDR maps that to its lower-level <code>bool_t</code>
|
|
type, which <b>is</b> fixed-size.</li>
|
|
<li>Don't compare a bool variable against the literal, <code>true</code>,
|
|
since a value with a logical non-false value need not be <code>1</code>.
|
|
I.e., don't write <code>if (seen == true) ...</code>. Rather,
|
|
write <code>if (seen)...</code>.</li>
|
|
</ul>
|
|
</li>
|
|
</ul>
|
|
|
|
<p>
|
|
Of course, take all of the above with a grain of salt. If you're about
|
|
to use some system interface that requires a type like <code>size_t</code>,
|
|
<code>pid_t</code> or <code>off_t</code>, use matching types for any
|
|
corresponding variables.
|
|
</p>
|
|
|
|
<p>
|
|
Also, if you try to use e.g., <code>unsigned int</code> as a type, and that
|
|
conflicts with the signedness of a related variable, sometimes
|
|
it's best just to use the <b>wrong</b> type, if <i>pulling the thread</i>
|
|
and fixing all related variables would be too invasive.
|
|
</p>
|
|
|
|
<p>
|
|
Finally, while using descriptive types is important, be careful not to
|
|
go overboard. If whatever you're doing causes warnings, or requires
|
|
casts, then reconsider or ask for help.
|
|
</p>
|
|
|
|
<h3>Pointers</h3>
|
|
|
|
<p>
|
|
Ensure that all of your pointers are <i>const-correct</i>.
|
|
Unless a pointer is used to modify the pointed-to storage,
|
|
give it the <code>const</code> attribute. That way, the reader knows
|
|
up-front that this is a read-only pointer. Perhaps more
|
|
importantly, if we're diligent about this, when you see a non-const
|
|
pointer, you're guaranteed that it is used to modify the storage
|
|
it points to, or it is aliased to another pointer that is.
|
|
</p>
|
|
|
|
<h2><a id="attribute_annotations">Attribute annotations</a></h2>
|
|
<p>
|
|
Use the following annotations to help the compiler and/or static
|
|
analysis tools understand the code better:
|
|
</p>
|
|
|
|
<table class="top_table">
|
|
<tr><th>Macro</th><th>Meaning</th></tr>
|
|
<tr><td><code>ATTRIBUTE_NONNULL</code></td><td>passing NULL for this parameter is not allowed</td></tr>
|
|
<tr><td><code>ATTRIBUTE_PACKED</code></td><td>force a structure to be packed</td></tr>
|
|
<tr><td><code>G_GNUC_FALLTHROUGH</code></td><td>allow code reuse by multiple switch cases</td></tr>
|
|
<tr><td><code>G_GNUC_NO_INLINE</code></td><td>the function is mocked in the test suite</td></tr>
|
|
<tr><td><code>G_GNUC_NORETURN</code></td><td>the function never returns</td></tr>
|
|
<tr><td><code>G_GNUC_NULL_TERMINATED</code></td><td>last parameter must be NULL</td></tr>
|
|
<tr><td><code>G_GNUC_PRINTF</code></td><td>validate that the formatting string matches parameters</td></tr>
|
|
<tr><td><code>G_GNUC_UNUSED</code></td><td>parameter is unused in this implementation of the function</td></tr>
|
|
<tr><td><code>G_GNUC_WARN_UNUSED_RESULT</code></td><td>the return value must be checked</td></tr>
|
|
</table>
|
|
|
|
<h2><a id="glib">Adoption of GLib APIs</a></h2>
|
|
|
|
<p>
|
|
Libvirt has adopted use of the
|
|
<a href="https://developer.gnome.org/glib/stable/">GLib library</a>.
|
|
Due to libvirt's long history of development, there are many APIs
|
|
in libvirt, for which GLib provides an alternative solution. The
|
|
general rule to follow is that the standard GLib solution will be
|
|
preferred over historical libvirt APIs. Existing code will be
|
|
ported over to use GLib APIs over time, but new code should use
|
|
the GLib APIs straight away where possible.
|
|
</p>
|
|
|
|
<p>
|
|
The following is a list of libvirt APIs that should no longer be
|
|
used in new code, and their suggested GLib replacements:
|
|
</p>
|
|
|
|
<dl>
|
|
<dt><code>VIR_ALLOC</code>, <code>VIR_REALLOC</code>,
|
|
<code>VIR_RESIZE_N</code>, <code>VIR_EXPAND_N</code>,
|
|
<code>VIR_SHRINK_N</code>, <code>VIR_FREE</code>,
|
|
<code>VIR_APPEND_ELEMENT</code>, <code>VIR_INSERT_ELEMENT</code>,
|
|
<code>VIR_DELETE_ELEMENT</code></dt>
|
|
<dd>Prefer the GLib APIs <code>g_new0</code>/<code>g_renew</code>/
|
|
<code>g_free</code> in most cases. There should rarely be a need
|
|
to use <code>g_malloc</code>/<code>g_realloc</code>.
|
|
Instead of using plain C arrays, it is preferrable to use
|
|
one of the GLib types, <code>GArray</code>, <code>GPtrArray</code>
|
|
or <code>GByteArray</code>. These
|
|
all use a struct to track the array memory and size together
|
|
and efficiently resize. <strong>NEVER MIX</strong> use of the
|
|
classic libvirt memory allocation APIs and GLib APIs within
|
|
a single method. Keep the style consistent, converting existing
|
|
code to GLib style in a separate, prior commit.</dd>
|
|
|
|
<dt><code>virStrerror</code></dt>
|
|
<dd>The GLib <code>g_strerror()</code> function should be used instead,
|
|
which has a simpler calling convention as an added benefit.</dd>
|
|
|
|
<table class="top_table">
|
|
<tr><th>deprecated version</th><th>GLib version</th><th>Notes</th></tr>
|
|
<tr><td><code>VIR_ALLOC(var)</code></td><td><code>g_new0(var_t, 1)</code></td>
|
|
<td>the type needs to be passed explicitly</td></tr>
|
|
<tr><td><code>VIR_ALLOC_N</code></td><td><code>g_new0(var_t, n)</code></td><td></td></tr>
|
|
<tr><td><code>VIR_REALLOC_N</code></td><td><code>g_renew(var_t, ptr, n)</code></td>
|
|
<td>the newly added memory is not zeroed</td></tr>
|
|
<tr><td><code>VIR_EXPAND_N</code></td><td><code>g_renew(var_t, ptr, n)</code></td>
|
|
<td>zero the new memory manually or use an array type</td></tr>
|
|
<tr><td><code>VIR_SHRINK_N</code></td><td><code>g_renew(var_t, ptr, n)</code></td>
|
|
<td></td></tr>
|
|
<tr><td><code>VIR_APPEND_ELEMENT</code></td><td><code>g_array_append_val</code></td>
|
|
<td><code>g_ptr_array_add</code> or <code>g_byte_array_append</code></td></tr>
|
|
<tr><td><code>VIR_INSERT_ELEMENT</code></td><td><code>g_array_insert_val</code></td>
|
|
<td><code>g_ptr_array_insert</code></td></tr>
|
|
<tr><td><code>VIR_DELETE_ELEMENT</code></td><td><code>g_array_remove_index</code></td>
|
|
<td><code>g_ptr_array_remove_index</code> or <code>g_byte_array_remove_index</code></td></tr>
|
|
<tr><td><code>VIR_FREE</code></td><td><code>g_free</code></td>
|
|
<td><code>g_free</code> does not zero the pointer</td></tr>
|
|
</table>
|
|
|
|
<p>String allocation macros and functions:</p>
|
|
<table class="top_table">
|
|
<tr><th>deprecated version</th><th>GLib version</th><th>Notes</th></tr>
|
|
<tr><td><code>virAsprintf</code></td><td><code>g_strdup_printf</code></td><td></td></tr>
|
|
<tr><td><code>virVasprintf</code></td><td><code>g_strdup_vprint</code></td>
|
|
<td>use <code>g_vasprintf</code> if you really need to know the returned length</td></tr>
|
|
<tr><td><code>virStrerror</code></td><td><code>g_strerror</code></td>
|
|
<td>the error strings are cached globally so no need to free it</td></tr>
|
|
</table>
|
|
</dl>
|
|
|
|
<p>
|
|
The following libvirt APIs have been deleted already:
|
|
</p>
|
|
<dl>
|
|
<dt><code>VIR_AUTOPTR</code>, <code>VIR_AUTOCLEAN</code>, <code>VIR_AUTOFREE</code></dt>
|
|
<dd>The GLib macros <code>g_autoptr</code>, <code>g_auto</code> and
|
|
<code>g_autofree</code> must be used
|
|
instead in all new code. In existing code, the GLib macros must
|
|
never be mixed with libvirt macros within a method, nor should
|
|
they be mixed with <code>VIR_FREE</code>. If introducing GLib macros to an
|
|
existing method, any use of libvirt macros must be converted
|
|
in an independent commit.
|
|
</dd>
|
|
|
|
<dt><code>VIR_DEFINE_AUTOPTR_FUNC</code>, <code>VIR_DEFINE_AUTOCLEAN_FUNC</code></dt>
|
|
<dd>The GLib macros <code>G_DEFINE_AUTOPTR_CLEANUP_FUNC</code> and
|
|
<code>G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC</code> must be used in all
|
|
new code. Existing code should be converted to the
|
|
new macros where relevant. It is permissible to use
|
|
<code>g_autoptr</code>, <code>g_auto</code> on an object whose cleanup function
|
|
is declared with the libvirt macros and vice-versa.
|
|
</dd>
|
|
|
|
<dt><code>VIR_AUTOUNREF</code></dt>
|
|
<dd>The GLib macros <code>g_autoptr</code> and <code>G_DEFINE_AUTOPTR_CLEANUP_FUNC</code>
|
|
should be used to manage autoclean of virObject classes.
|
|
This matches usage with GObject classes.</dd>
|
|
|
|
<dt><code>VIR_STRDUP</code>, <code>VIR_STRNDUP</code></dt>
|
|
<dd>Prefer the GLib APIs <code>g_strdup</code> and <code>g_strndup</code>.</dd>
|
|
</dl>
|
|
<table class="top_table">
|
|
<tr><th>deleted version</th><th>GLib version</th><th>Notes</th></tr>
|
|
<tr><td><code>VIR_AUTOPTR</code></td><td><code>g_autoptr</code></td><td></td></tr>
|
|
<tr><td><code>VIR_AUTOCLEAN</code></td><td><code>g_auto</code></td><td></td></tr>
|
|
<tr><td><code>VIR_AUTOFREE</code></td><td><code>g_autofree</code></td><td>The GLib version does not use parentheses</td></tr>
|
|
<tr><td><code>VIR_AUTOUNREF</code></td><td><code>g_autoptr</code></td><td>The cleanup function needs to be defined</td></tr>
|
|
<tr><td><code>VIR_DEFINE_AUTOPTR_FUNC</code></td><td><code>G_DEFINE_AUTOPTR_CLEANUP_FUNC</code></td><td></td></tr>
|
|
<tr><td><code>VIR_DEFINE_AUTOCLEAN_FUNC</code></td><td><code>G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC</code></td><td></td></tr>
|
|
<tr><td><code>VIR_STEAL_PTR</code></td><td><code>g_steal_pointer</code></td>
|
|
<td><code>a = f(&b)</code> instead of <code>f(a, b)</code></td></tr>
|
|
<tr><td><code>VIR_RETURN_PTR</code></td><td><code>return g_steal_pointer</code></td><td></td></tr>
|
|
<tr><td><code>ARRAY_CARDINALITY</code></td><td><code>G_N_ELEMENTS</code></td><td></td></tr>
|
|
<tr><td><code>ATTRIBUTE_FALLTHROUGH</code></td><td><code>G_GNUC_FALLTHROUGH</code></td><td></td></tr>
|
|
<tr><td><code>ATTRIBUTE_FMT_PRINTF</code></td><td><code>G_GNUC_PRINTF</code></td><td></td></tr>
|
|
<tr><td><code>ATTRIBUTE_NOINLINE</code></td><td><code>G_GNUC_NO_INLINE</code></td><td></td></tr>
|
|
<tr><td><code>ATTRIBUTE_NORETURN</code></td><td><code>G_GNUC_NORETURN</code></td><td></td></tr>
|
|
<tr><td><code>ATTRIBUTE_RETURN_CHECK</code></td><td><code>G_GNUC_WARN_UNUSED_RESULT</code></td><td></td></tr>
|
|
<tr><td><code>ATTRIBUTE_SENTINEL</code></td><td><code>G_GNUC_NULL_TERMINATED</code></td><td></td></tr>
|
|
<tr><td><code>ATTRIBUTE_UNUSED</code></td><td><code>G_GNUC_UNUSED</code></td><td></td></tr>
|
|
<tr><td><code>VIR_STRDUP</code></td><td><code>g_strdup</code></td><td></td></tr>
|
|
<tr><td><code>VIR_STRNDUP</code></td><td><code>g_strndup</code></td><td></td></tr>
|
|
</table>
|
|
|
|
|
|
<h2><a id="file_handling">File handling</a></h2>
|
|
|
|
<p>
|
|
Usage of the <code>fdopen()</code>, <code>close()</code>, <code>fclose()</code>
|
|
APIs is deprecated in libvirt code base to help avoiding double-closing of files
|
|
or file descriptors, which is particularly dangerous in a multi-threaded
|
|
application. Instead of these APIs, use the macros from virfile.h
|
|
</p>
|
|
|
|
<ul>
|
|
<li><p>Open a file from a file descriptor:</p>
|
|
|
|
<pre>
|
|
if ((file = VIR_FDOPEN(fd, "r")) == NULL) {
|
|
virReportSystemError(errno, "%s",
|
|
_("failed to open file from file descriptor"));
|
|
return -1;
|
|
}
|
|
/* fd is now invalid; only access the file using file variable */
|
|
</pre></li>
|
|
|
|
<li><p>Close a file descriptor:</p>
|
|
<pre>
|
|
if (VIR_CLOSE(fd) < 0) {
|
|
virReportSystemError(errno, "%s", _("failed to close file"));
|
|
}
|
|
</pre></li>
|
|
|
|
<li><p>Close a file:</p>
|
|
|
|
<pre>
|
|
if (VIR_FCLOSE(file) < 0) {
|
|
virReportSystemError(errno, "%s", _("failed to close file"));
|
|
}
|
|
</pre></li>
|
|
|
|
<li><p>Close a file or file descriptor in an error path, without losing
|
|
the previous <code>errno</code> value:</p>
|
|
|
|
<pre>
|
|
VIR_FORCE_CLOSE(fd);
|
|
VIR_FORCE_FCLOSE(file);
|
|
</pre>
|
|
</li>
|
|
</ul>
|
|
|
|
<h2><a id="string_comparision">String comparisons</a></h2>
|
|
|
|
<p>
|
|
Do not use the strcmp, strncmp, etc functions directly. Instead use
|
|
one of the following semantically named macros
|
|
</p>
|
|
|
|
<ul>
|
|
<li><p>For strict equality:</p>
|
|
<pre>
|
|
STREQ(a,b)
|
|
STRNEQ(a,b)
|
|
</pre>
|
|
</li>
|
|
|
|
<li><p>For case insensitive equality:</p>
|
|
<pre>
|
|
STRCASEEQ(a,b)
|
|
STRCASENEQ(a,b)
|
|
</pre>
|
|
</li>
|
|
|
|
<li><p>For strict equality of a substring:</p>
|
|
<pre>
|
|
STREQLEN(a,b,n)
|
|
STRNEQLEN(a,b,n)
|
|
</pre>
|
|
</li>
|
|
|
|
<li><p>For case insensitive equality of a substring:</p>
|
|
<pre>
|
|
STRCASEEQLEN(a,b,n)
|
|
STRCASENEQLEN(a,b,n)
|
|
</pre>
|
|
</li>
|
|
|
|
<li><p>For strict equality of a prefix:</p>
|
|
<pre>
|
|
STRPREFIX(a,b)
|
|
</pre>
|
|
</li>
|
|
<li><p>To avoid having to check if a or b are NULL:</p>
|
|
<pre>
|
|
STREQ_NULLABLE(a, b)
|
|
STRNEQ_NULLABLE(a, b)
|
|
</pre>
|
|
</li>
|
|
</ul>
|
|
|
|
|
|
<h2><a id="string_copying">String copying</a></h2>
|
|
|
|
<p>
|
|
Do not use the strncpy function. According to the man page, it
|
|
does <b>not</b> guarantee a NULL-terminated buffer, which makes
|
|
it extremely dangerous to use. Instead, use one of the replacement
|
|
functions provided by libvirt:
|
|
</p>
|
|
|
|
<pre>
|
|
virStrncpy(char *dest, const char *src, size_t n, size_t destbytes)
|
|
</pre>
|
|
<p>
|
|
The first two arguments have the same meaning as for strncpy,
|
|
namely the destination and source of the copy operation. Unlike
|
|
strncpy, the function will always copy exactly the number of bytes
|
|
requested and make sure the destination is NULL-terminated, as the
|
|
source is required to be; sanity checks are performed to ensure the
|
|
size of the destination, as specified by the last argument, is
|
|
sufficient for the operation to succeed. On success, 0 is returned;
|
|
on failure, a value <0 is returned instead.
|
|
</p>
|
|
|
|
<pre>
|
|
virStrcpy(char *dest, const char *src, size_t destbytes)
|
|
</pre>
|
|
<p>
|
|
Use this variant if you know you want to copy the entire src
|
|
string into dest.
|
|
</p>
|
|
|
|
<pre>
|
|
virStrcpyStatic(char *dest, const char *src)
|
|
</pre>
|
|
<p>
|
|
Use this variant if you know you want to copy the entire src
|
|
string into dest <b>and</b> you know that your destination string is
|
|
a static string (i.e. that sizeof(dest) returns something
|
|
meaningful). Note that this is a macro, so arguments could be
|
|
evaluated more than once.
|
|
</p>
|
|
|
|
<pre>
|
|
dst = g_strdup(src);
|
|
dst = g_strndup(src, n);
|
|
</pre>
|
|
<p>
|
|
You should avoid using strdup or strndup directly as they do not handle
|
|
out-of-memory errors, and do not allow a NULL source.
|
|
Use <code>g_strdup</code> and <code>g_strndup</code> from GLib which
|
|
abort on OOM and handle NULL source by returning NULL.
|
|
</p>
|
|
|
|
<h2><a id="strbuf">Variable length string buffer</a></h2>
|
|
|
|
<p>
|
|
If there is a need for complex string concatenations, avoid using
|
|
the usual sequence of malloc/strcpy/strcat/snprintf functions and
|
|
make use of either the
|
|
<a href="https://developer.gnome.org/glib/stable/glib-Strings.html">GString</a>
|
|
type from GLib or the virBuffer API.
|
|
If formatting XML or QEMU command line is needed, use the virBuffer
|
|
API described in virbuffer.h, since it has helper functions for those.
|
|
</p>
|
|
|
|
<p>Typical usage is as follows:</p>
|
|
|
|
<pre>
|
|
char *
|
|
somefunction(...)
|
|
{
|
|
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
|
|
|
|
...
|
|
|
|
virBufferAddLit(&buf, "<domain>\n");
|
|
virBufferAsprintf(&buf, " <memory>%d</memory>\n", memory);
|
|
if (some_error)
|
|
return NULL; /* g_auto will free the memory used so far */
|
|
...
|
|
virBufferAddLit(&buf, "</domain>\n");
|
|
|
|
...
|
|
|
|
if (virBufferCheckError(&buf) < 0)
|
|
return NULL;
|
|
|
|
return virBufferContentAndReset(&buf);
|
|
}
|
|
</pre>
|
|
|
|
|
|
<h2><a id="includes">Include files</a></h2>
|
|
|
|
<p>
|
|
There are now quite a large number of include files, both libvirt
|
|
internal and external, and system includes. To manage all this
|
|
complexity it's best to stick to the following general plan for all
|
|
*.c source files:
|
|
</p>
|
|
|
|
<pre>
|
|
/*
|
|
* Copyright notice
|
|
* ....
|
|
* ....
|
|
* ....
|
|
*
|
|
*/
|
|
|
|
#include <config.h> Must come first in every file.
|
|
|
|
#include <stdio.h> Any system includes you need.
|
|
#include <string.h>
|
|
#include <limits.h>
|
|
|
|
#if WITH_NUMACTL Some system includes aren't supported
|
|
# include <numa.h> everywhere so need these #if guards.
|
|
#endif
|
|
|
|
#include "internal.h" Include this first, after system includes.
|
|
|
|
#include "util.h" Any libvirt internal header files.
|
|
#include "buf.h"
|
|
|
|
static int
|
|
myInternalFunc() The actual code.
|
|
{
|
|
...
|
|
</pre>
|
|
|
|
<p>
|
|
Of particular note: <b>Do not</b> include libvirt/libvirt.h,
|
|
libvirt/virterror.h, libvirt/libvirt-qemu.h, or libvirt/libvirt-lxc.h.
|
|
They are included by "internal.h" already and there are some special reasons
|
|
why you cannot include these files explicitly. One of the special cases,
|
|
"libvirt/libvirt.h" is included prior to "internal.h" in "remote_protocol.x",
|
|
to avoid exposing *_LAST enum elements.
|
|
</p>
|
|
|
|
|
|
<h2><a id="printf">Printf-style functions</a></h2>
|
|
|
|
<p>
|
|
Whenever you add a new printf-style function, i.e., one with a format
|
|
string argument and following "..." in its prototype, be sure to use
|
|
gcc's printf attribute directive in the prototype. For example, here's
|
|
the one for virCommandAddEnvFormat in vircommand.h:
|
|
</p>
|
|
|
|
<pre>
|
|
void virCommandAddEnvFormat(virCommandPtr cmd, const char *format, ...)
|
|
G_GNUC_PRINTF(2, 3);
|
|
</pre>
|
|
|
|
<p>
|
|
This makes it so gcc's -Wformat and -Wformat-security options can do
|
|
their jobs and cross-check format strings with the number and types
|
|
of arguments.
|
|
</p>
|
|
|
|
<p>
|
|
When printing to a string, consider using GString or virBuffer for
|
|
incremental allocations, g_strdup_printf for a one-shot allocation,
|
|
and g_snprintf for fixed-width buffers. Only use g_sprintf,
|
|
if you can prove the buffer won't overflow.
|
|
</p>
|
|
|
|
<h2><a id="errors">Error message format</a></h2>
|
|
|
|
<p>
|
|
Error messages visible to the user should be short and descriptive. All
|
|
error messages are translated using gettext and thus must be wrapped in
|
|
<code>_()</code> macro. To simplify the translation work, the error message
|
|
must not be concatenated from various parts. To simplify searching for
|
|
the error message in the code the strings should not be broken even
|
|
if they result into a line longer than 80 columns and any formatting
|
|
modifier should be enclosed by quotes or other obvious separator.
|
|
If a string used with <code>%s</code> can be NULL the NULLSTR macro must
|
|
be used.
|
|
</p>
|
|
|
|
<pre>
|
|
GOOD: virReportError(VIR_ERR_INTERNAL_ERROR,
|
|
_("Failed to connect to remote host '%s'"), hostname)
|
|
|
|
BAD: virReportError(VIR_ERR_INTERNAL_ERROR,
|
|
_("Failed to %s to remote host '%s'"),
|
|
"connect", hostname);
|
|
|
|
BAD: virReportError(VIR_ERR_INTERNAL_ERROR,
|
|
_("Failed to connect "
|
|
"to remote host '%s'),
|
|
hostname);
|
|
</pre>
|
|
|
|
<h2><a id="goto">Use of goto</a></h2>
|
|
|
|
<p>
|
|
The use of goto is not forbidden, and goto is widely used
|
|
throughout libvirt. While the uncontrolled use of goto will
|
|
quickly lead to unmaintainable code, there is a place for it in
|
|
well structured code where its use increases readability and
|
|
maintainability. In general, if goto is used for error
|
|
recovery, it's likely to be ok, otherwise, be cautious or avoid
|
|
it all together.
|
|
</p>
|
|
|
|
<p>
|
|
The typical use of goto is to jump to cleanup code in the case
|
|
of a long list of actions, any of which may fail and cause the
|
|
entire operation to fail. In this case, a function will have a
|
|
single label at the end of the function. It's almost always ok
|
|
to use this style. In particular, if the cleanup code only
|
|
involves free'ing memory, then having multiple labels is
|
|
overkill. g_free() and most of the functions named XXXFree() in
|
|
libvirt is required to handle NULL as its arg. This does not
|
|
apply to libvirt's public APIs. Thus you can
|
|
safely call free on all the variables even if they were not yet
|
|
allocated (yes they have to have been initialized to NULL).
|
|
This is much simpler and clearer than having multiple labels.
|
|
Note that most of libvirt's type declarations can be marked with
|
|
either <code>g_autofree</code> or <code>g_autoptr</code> which uses
|
|
the compiler's <code>__attribute__((cleanup))</code> that calls
|
|
the appropriate free function when the variable goes out of scope.
|
|
</p>
|
|
|
|
<p>
|
|
There are a couple of signs that a particular use of goto is not
|
|
ok:
|
|
</p>
|
|
|
|
<ul>
|
|
<li>You're using multiple labels. If you find yourself using
|
|
multiple labels, you're strongly encouraged to rework your code
|
|
to eliminate all but one of them.</li>
|
|
<li>The goto jumps back up to a point above the current line of
|
|
code being executed. Please use some combination of looping
|
|
constructs to re-execute code instead; it's almost certainly
|
|
going to be more understandable by others. One well-known
|
|
exception to this rule is restarting an i/o operation following
|
|
EINTR.</li>
|
|
<li>The goto jumps down to an arbitrary place in the middle of a
|
|
function followed by further potentially failing calls. You
|
|
should almost certainly be using a conditional and a block
|
|
instead of a goto. Perhaps some of your function's logic would
|
|
be better pulled out into a helper function.</li>
|
|
</ul>
|
|
|
|
<p>
|
|
Although libvirt does not encourage the Linux kernel wind/unwind
|
|
style of multiple labels, there's a good general discussion of
|
|
the issue archived at
|
|
<a href="http://kerneltrap.org/node/553/2131">KernelTrap</a>
|
|
</p>
|
|
|
|
<p>
|
|
When using goto, please use one of these standard labels if it
|
|
makes sense:
|
|
</p>
|
|
|
|
<pre>
|
|
error: A path only taken upon return with an error code
|
|
cleanup: A path taken upon return with success code + optional error
|
|
no_memory: A path only taken upon return with an OOM error code
|
|
retry: If needing to jump upwards (e.g., retry on EINTR)
|
|
</pre>
|
|
|
|
<p>
|
|
Top-level labels should be indented by one space (putting them on
|
|
the beginning of the line confuses function context detection in git):
|
|
</p>
|
|
|
|
<pre>
|
|
int foo()
|
|
{
|
|
/* ... do stuff ... */
|
|
cleanup:
|
|
/* ... do other stuff ... */
|
|
}
|
|
</pre>
|
|
|
|
|
|
|
|
<h2><a id="committers">Libvirt committer guidelines</a></h2>
|
|
|
|
<p>
|
|
The AUTHORS files indicates the list of people with commit access right
|
|
who can actually merge the patches.
|
|
</p>
|
|
|
|
<p>
|
|
The general rule for committing a patch is to make sure
|
|
it has been reviewed
|
|
properly in the mailing-list first, usually if a couple of people gave an
|
|
ACK or +1 to a patch and nobody raised an objection on the list it should
|
|
be good to go. If the patch touches a part of the code where you're not
|
|
the main maintainer, or where you do not have a very clear idea of
|
|
how things work, it's better
|
|
to wait for a more authoritative feedback though. Before committing, please
|
|
also rebuild locally, run 'make check syntax-check', and make sure you
|
|
don't raise errors.
|
|
</p>
|
|
|
|
<p>
|
|
An exception to 'review and approval on the list first' is fixing failures
|
|
to build:
|
|
</p>
|
|
<ul>
|
|
<li>if a recently committed patch breaks compilation on a platform
|
|
or for a given driver, then it's fine to commit a minimal fix
|
|
directly without getting the review feedback first</li>
|
|
<li>if make check or make syntax-check breaks, if there is
|
|
an obvious fix, it's fine to commit immediately.
|
|
The patch should still be sent to the list (or tell what the fix was if
|
|
trivial), and 'make check syntax-check' should pass too, before committing
|
|
anything</li>
|
|
<li>
|
|
fixes for documentation and code comments can be managed
|
|
in the same way, but still make sure they get reviewed if non-trivial.
|
|
</li>
|
|
<li>(ir)regular pulls from other repositories or automated updates, such
|
|
as the .gnulib submodule updates, pulling in new translations or updating
|
|
the container images for the CI system
|
|
</li>
|
|
</ul>
|
|
</body>
|
|
</html>
|