mirror of
https://gitlab.com/libvirt/libvirt.git
synced 2024-12-27 07:22:07 +03:00
68ea80cfdd
We already have virAsprintf, so picking a similar name helps for seeing a similar purpose. Furthermore, the prefix V before printf generally implies 'va_list', even though this variant was '...', and the old name got in the way of adding a new va_list version. global rename performed with: $ git grep -l virBufferVSprintf \ | xargs -L1 sed -i 's/virBufferVSprintf/virBufferAsprintf/g' then revert the changes in ChangeLog-old.
866 lines
26 KiB
HTML
866 lines
26 KiB
HTML
<html>
|
|
<body>
|
|
<h1>Contributor guidelines</h1>
|
|
|
|
<ul id="toc"></ul>
|
|
|
|
<h2><a name="patches">General tips for contributing patches</a></h2>
|
|
<ol>
|
|
<li>Discuss any large changes on the mailing list first. Post patches
|
|
early and listen to feedback.</li>
|
|
|
|
<li><p>Post patches in unified diff format. A command similar to this
|
|
should work:</p>
|
|
<pre>
|
|
diff -urp libvirt.orig/ libvirt.modified/ > libvirt-myfeature.patch
|
|
</pre>
|
|
<p>
|
|
or:
|
|
</p>
|
|
<pre>
|
|
git diff > libvirt-myfeature.patch
|
|
</pre>
|
|
</li>
|
|
<li>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.</li>
|
|
<li>Make sure your patches apply against libvirt GIT. Developers
|
|
only follow GIT and don't care much about released versions.</li>
|
|
<li><p>Run the automated tests on your code before submitting any changes.
|
|
In particular, configure with compile warnings set to -Werror:</p>
|
|
<pre>
|
|
./configure --enable-compile-warnings=error
|
|
</pre>
|
|
<p>
|
|
and run the tests:
|
|
</p>
|
|
<pre>
|
|
make check
|
|
make syntax-check
|
|
make -C tests valgrind
|
|
</pre>
|
|
<p>
|
|
The latter test checks for memory leaks.
|
|
</p>
|
|
|
|
<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>
|
|
Also, individual tests can be run from inside the <code>tests/</code>
|
|
directory, like:
|
|
</p>
|
|
<pre>
|
|
./qemuxml2xmltest
|
|
</pre>
|
|
|
|
</li>
|
|
<li>Update tests and/or documentation, particularly if you are adding
|
|
a new feature or changing the output of a program.</li>
|
|
</ol>
|
|
|
|
<p>
|
|
There is more on this subject, including lots of links to background
|
|
reading on the subject, on
|
|
<a href="http://et.redhat.com/~rjones/how-to-supply-code-to-open-source-projects/">
|
|
Richard Jones' guide to working with open source projects</a>
|
|
</p>
|
|
|
|
|
|
<h2><a name="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, add the following to one of one of your start-up files
|
|
(e.g., ~/.emacs), to help ensure that you get indentation right:
|
|
</p>
|
|
<pre>
|
|
;;; When editing C sources in libvirt, use this style.
|
|
(defun libvirt-c-mode ()
|
|
"C mode with adjusted defaults for use with libvirt."
|
|
(interactive)
|
|
(c-set-style "K&R")
|
|
(setq indent-tabs-mode nil) ; indent using spaces, not TABs
|
|
(setq c-indent-level 4)
|
|
(setq c-basic-offset 4))
|
|
(add-hook 'c-mode-hook
|
|
'(lambda () (if (string-match "/libvirt" (buffer-file-name))
|
|
(libvirt-c-mode))))
|
|
</pre>
|
|
|
|
<p>
|
|
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
|
|
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 name="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 name="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 that body occupies 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>
|
|
<p>
|
|
Omitting braces with a single-line body is fine:
|
|
</p>
|
|
|
|
<pre>
|
|
while (expr) // one-line body -> omitting curly braces is ok
|
|
single_line_stmt();
|
|
</pre>
|
|
|
|
<p>
|
|
However, the moment your loop/if/else body extends onto 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>
|
|
|
|
<h2><a href="types">Preprocessor</a></h2>
|
|
|
|
<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 href="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 name="memalloc">Low level memory management</a></h2>
|
|
|
|
<p>
|
|
Use of the malloc/free/realloc/calloc APIs is deprecated in the libvirt
|
|
codebase, because they encourage a number of serious coding bugs and do
|
|
not enable compile time verification of checks for NULL. Instead of these
|
|
routines, use the macros from memory.h.
|
|
</p>
|
|
|
|
<ul>
|
|
<li><p>To allocate a single object:</p>
|
|
|
|
<pre>
|
|
virDomainPtr domain;
|
|
|
|
if (VIR_ALLOC(domain) < 0) {
|
|
virReportOOMError();
|
|
return NULL;
|
|
}
|
|
</pre>
|
|
</li>
|
|
|
|
<li><p>To allocate an array of objects:</p>
|
|
<pre>
|
|
virDomainPtr domains;
|
|
size_t ndomains = 10;
|
|
|
|
if (VIR_ALLOC_N(domains, ndomains) < 0) {
|
|
virReportOOMError();
|
|
return NULL;
|
|
}
|
|
</pre>
|
|
</li>
|
|
|
|
<li><p>To allocate an array of object pointers:</p>
|
|
<pre>
|
|
virDomainPtr *domains;
|
|
size_t ndomains = 10;
|
|
|
|
if (VIR_ALLOC_N(domains, ndomains) < 0) {
|
|
virReportOOMError();
|
|
return NULL;
|
|
}
|
|
</pre>
|
|
</li>
|
|
|
|
<li><p>To re-allocate the array of domains to be 1 element
|
|
longer (however, note that repeatedly expanding an array by 1
|
|
scales quadratically, so this is recommended only for smaller
|
|
arrays):</p>
|
|
<pre>
|
|
virDomainPtr domains;
|
|
size_t ndomains = 0;
|
|
|
|
if (VIR_EXPAND_N(domains, ndomains, 1) < 0) {
|
|
virReportOOMError();
|
|
return NULL;
|
|
}
|
|
domains[ndomains - 1] = domain;
|
|
</pre></li>
|
|
|
|
<li><p>To ensure an array has room to hold at least one more
|
|
element (this approach scales better, but requires tracking
|
|
allocation separately from usage)</p>
|
|
|
|
<pre>
|
|
virDomainPtr domains;
|
|
size_t ndomains = 0;
|
|
size_t ndomains_max = 0;
|
|
|
|
if (VIR_RESIZE_N(domains, ndomains_max, ndomains, 1) < 0) {
|
|
virReportOOMError();
|
|
return NULL;
|
|
}
|
|
domains[ndomains++] = domain;
|
|
</pre>
|
|
</li>
|
|
|
|
<li><p>To trim an array of domains from its allocated size down
|
|
to the actual used size:</p>
|
|
|
|
<pre>
|
|
virDomainPtr domains;
|
|
size_t ndomains = x;
|
|
size_t ndomains_max = y;
|
|
|
|
VIR_SHRINK_N(domains, ndomains_max, ndomains_max - ndomains);
|
|
</pre></li>
|
|
|
|
<li><p>To free an array of domains:</p>
|
|
<pre>
|
|
virDomainPtr domains;
|
|
size_t ndomains = x;
|
|
size_t ndomains_max = y;
|
|
size_t i;
|
|
|
|
for (i = 0; i < ndomains; i++)
|
|
VIR_FREE(domains[i]);
|
|
VIR_FREE(domains);
|
|
ndomains_max = ndomains = 0;
|
|
</pre>
|
|
</li>
|
|
</ul>
|
|
|
|
<h2><a name="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 particulary dangerous in a multi-threaded
|
|
applications. Instead of these APIs, use the macros from files.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 name="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>
|
|
</ul>
|
|
|
|
|
|
<h2><a name="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
|
|
functionally equivalent functions:
|
|
</p>
|
|
|
|
<pre>
|
|
virStrncpy(char *dest, const char *src, size_t n, size_t destbytes)
|
|
</pre>
|
|
<p>
|
|
The first three arguments have the same meaning as for strncpy;
|
|
namely the destination, source, and number of bytes to copy,
|
|
respectively. The last argument is the number of bytes
|
|
available in the destination string; if a copy of the source
|
|
string (including a \0) will not fit into the destination, no
|
|
bytes are copied and the routine returns NULL. Otherwise, n
|
|
bytes from the source are copied into the destination and a
|
|
trailing \0 is appended.
|
|
</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. Note that this is a macro, so arguments could
|
|
be evaluated more than once. This is equivalent to
|
|
virStrncpy(dest, src, strlen(src), destbytes)
|
|
</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. This is equivalent to
|
|
virStrncpy(dest, src, strlen(src), sizeof(dest)).
|
|
</p>
|
|
|
|
<h2><a name="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 the virBuffer API described in buf.h
|
|
</p>
|
|
|
|
<p>Typical usage is as follows:</p>
|
|
|
|
<pre>
|
|
char *
|
|
somefunction(...)
|
|
{
|
|
virBuffer buf = VIR_BUFFER_INITIALIZER;
|
|
|
|
...
|
|
|
|
virBufferAddLit(&buf, "<domain>\n");
|
|
virBufferAsprintf(&buf, " <memory>%d</memory>\n", memory);
|
|
...
|
|
virBufferAddLit(&buf, "</domain>\n");
|
|
|
|
...
|
|
|
|
if (virBufferError(&buf)) {
|
|
virBufferFreeAndReset(&buf);
|
|
virReportOOMError();
|
|
return NULL;
|
|
}
|
|
|
|
return virBufferContentAndReset(&buf);
|
|
}
|
|
</pre>
|
|
|
|
|
|
<h2><a name="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 HAVE_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 or
|
|
libvirt/virterror.h. It is included by "internal.h" already and there
|
|
are some special reasons why you cannot include these files
|
|
explicitly.
|
|
</p>
|
|
|
|
|
|
<h2><a name="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 virAsprintf, in util.h:
|
|
</p>
|
|
|
|
<pre>
|
|
int virAsprintf(char **strp, const char *fmt, ...)
|
|
ATTRIBUTE_FORMAT(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 virBuffer for
|
|
incremental allocations, virAsprintf for a one-shot allocation,
|
|
and snprintf for fixed-width buffers. Do not use sprintf, even
|
|
if you can prove the buffer won't overflow, since gnulib does
|
|
not provide the same portability guarantees for sprintf as it
|
|
does for snprintf.
|
|
</p>
|
|
|
|
<h2><a name="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. VIR_FREE() and every function named XXXFree() in
|
|
libvirt is required to handle NULL as its arg. Thus you can
|
|
safely call free on all the variables even if they were not yet
|
|
allocated (yes they have to have been initialized to NULL).
|
|
This is much simpler and clearer than having multiple labels.
|
|
</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>
|
|
|
|
|
|
|
|
<h2><a name="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. Try to look for warnings too; for example,
|
|
configure with
|
|
</p>
|
|
<pre>
|
|
--enable-compile-warnings=error
|
|
</pre>
|
|
<p>
|
|
which adds -Werror to compile flags, so no warnings get missed
|
|
</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>
|
|
</ul>
|
|
</body>
|
|
</html>
|