2010/11/11 Eric Blake <eblake@xxxxxxxxxx>:
> On 11/11/2010 01:39 PM, Matthias Bolte wrote:
>> Tweak pre tags in docs/hacking.html.in to achieve proper
>> indentation of their plaintext representation.
>>
>> Also use more b/i/code tags in docs/hacking.html.in.
>> ---
>> ÂHACKING Â Â Â Â Â Â Â| Â602 +++++++++++++++++++++++++++++---------------------
>
> A lot of this diff is whitespace; the rest looks like it is pulling in
> missed bits. ÂOverall, it looks pretty decent; and it's certainly more
> maintainable!
>
>> ÂMakefile.am     Â|  Â8 +
>> Âdocs/hacking.html.in | Â351 ++++++++++++++++--------------
>
> Would you mind splitting this into two patches? ÂOne that cleans up just
> hacking.html.in, and the rest that covers going from a clean source file
> into the new generated HACKING file.
I did that now.
>> +++ b/Makefile.am
>> @@ -48,6 +48,8 @@ EXTRA_DIST = \
>> Âpkgconfigdir = $(libdir)/pkgconfig
>> Âpkgconfig_DATA = libvirt.pc
>>
>> +all: NEWS HACKING
>
> Hmm. ÂThis means that everyone will attempt to build NEWS and HACKING,
> even if they don't have the prerequisite tools.
I changed that to:
syntax-check: $(top_srcdir)/HACKING
>>     $(XSLTPROC) --nonet $(top_srcdir)/docs/news.xsl    \
>> @@ -56,6 +58,12 @@ NEWS: $(top_srcdir)/docs/news.xsl $(top_srcdir)/docs/news.html.in
>> Â Â Â Â Â| perl -pe 's/[ \t]+$$//' Â Â Â Â Â Â Â Â Â Â Â Â Â Â\
>> Â Â Â Â Â> $@-t && mv $@-t $@ ; fi );
>>
>> +HACKING: $(top_srcdir)/docs/hacking1.xsl $(top_srcdir)/docs/hacking2.xsl \
> If HACKING stays in git, then it must also reside in $(srcdir) rather
> than $(builddir), which means this rule needs a bit of tweaking to be
> VPATH-friendly.
I think it should stay in git and changed the rule to
$(top_srcdir)/HACKING: ...
I also tested it in a VPATH build and it works as it should.
v2 is attached.
Matthias
From dcd3513dd5d31cf074bc19e1ade9e2485cf4bd5e Mon Sep 17 00:00:00 2001
From: Matthias Bolte <matthias.bolte@xxxxxxxxxxxxxx>
Date: Fri, 12 Nov 2010 15:33:00 +0100
Subject: [PATCH] docs: Prepare hacking.html.in to generate HACKING from it
Tweak pre tags to achieve proper indentation of their
plaintext representation. Also use more b/i/code tags.
---
docs/hacking.html.in | 351 +++++++++++++++++++++++++++-----------------------
1 files changed, 189 insertions(+), 162 deletions(-)
diff --git a/docs/hacking.html.in b/docs/hacking.html.in
index bd8b443..88db01a 100644
--- a/docs/hacking.html.in
+++ b/docs/hacking.html.in
@@ -5,20 +5,21 @@
<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>
+<pre>
+ diff -urp libvirt.orig/ libvirt.modified/ > libvirt-myfeature.patch
+</pre>
<p>
or:
</p>
- <pre>
- git diff > libvirt-myfeature.patch</pre>
+<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
@@ -27,35 +28,39 @@
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>
+<pre>
+ ./configure --enable-compile-warnings=error
+</pre>
<p>
and run the tests:
</p>
- <pre>
+<pre>
make check
make syntax-check
- make -C tests valgrind</pre>
+ 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>
+ <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>
+<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 'tests/'
- directory, like:
- </p>
- <pre>
- ./qemuxml2xmltest</pre>
+ 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
@@ -82,7 +87,7 @@
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>
+<pre>
;;; When editing C sources in libvirt, use this style.
(defun libvirt-c-mode ()
"C mode with adjusted defaults for use with libvirt."
@@ -105,7 +110,7 @@
around operators and keywords:
</p>
- <pre>
+<pre>
indent-libvirt()
{
indent -bad -bap -bbb -bli4 -br -ce -brs -cs -i4 -l75 -lc75 \
@@ -116,7 +121,7 @@
<p>
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.
+ 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>
@@ -125,18 +130,20 @@
<h2><a name="curly_braces">Curly braces</a></h2>
<p>
- Omit the curly braces around an "if", "while", "for" etc. body only
+ 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-*statement* loop: each has only one *line* in its body.
+ 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>
+<pre>
while (expr) // one-line body -> omitting curly braces is ok
- single_line_stmt ();</pre>
+ single_line_stmt();
+</pre>
<p>
However, the moment your loop/if/else body extends onto a second
@@ -146,26 +153,29 @@
it is already a multi-statement loop:
</p>
- <pre>
+<pre>
while (true) // BAD! multi-line body with no braces
/* comment... */
- single_line_stmt ();</pre>
+ single_line_stmt();
+</pre>
<p>
Do this instead:
</p>
- <pre>
+<pre>
while (true) { // Always put braces around a multi-line body.
/* comment... */
- single_line_stmt ();
- }</pre>
+ 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>
+<pre>
if (expr)
- die ("a diagnostic that would make this line"
- " extend past the 80-column limit"));</pre>
+ 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
@@ -177,40 +187,44 @@
To reiterate, don't do this:
</p>
- <pre>
+<pre>
if (expr) // BAD: no braces around...
while (expr_2) { // ... a multi-line body
...
- }</pre>
+ }
+</pre>
<p>
Do this, instead:
</p>
- <pre>
+<pre>
if (expr) {
while (expr_2) {
...
}
- }</pre>
+ }
+</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 "else" block, and the corresponding "then" block
- *does* use braces. In that case, either put braces around the "else"
- block, or negate the "if"-condition and swap the bodies, putting the
+ brace-less block is an <code>else</code> block, and the corresponding
+ <code>then</code> block <b>does</b> use braces. In that case, either
+ put braces around the <code>else</code> block, or negate the
+ <code>if</code>-condition and swap the bodies, putting the
one-line block first and making the longer, multi-line block be the
- "else" block.
+ <code>else</code> block.
</p>
- <pre>
+<pre>
if (expr) {
...
...
}
else
- x = y; // BAD: braceless "else" with braced "then"</pre>
+ x = y; // BAD: braceless "else" with braced "then"
+</pre>
<p>
This is preferred, especially when the multi-line body is more than a
@@ -219,43 +233,45 @@
after the more involved block:
</p>
- <pre>
+<pre>
if (!expr)
x = y; // putting the smaller block first is more readable
else {
...
...
- }</pre>
+ }
+</pre>
<p>
If you'd rather not negate the condition, then at least add braces:
</p>
- <pre>
+<pre>
if (expr) {
...
...
} else {
x = y;
- }</pre>
+ }
+</pre>
<h2><a href="types">Preprocessor</a></h2>
<p>
For variadic macros, stick with C99 syntax:
</p>
- <pre>
+<pre>
#define vshPrint(_ctl, ...) fprintf(stdout, __VA_ARGS__)
- </pre>
+</pre>
<p>Use parenthesis when checking if a macro is defined, and use
indentation to track nesting:
</p>
- <pre>
+<pre>
#if defined(HAVE_POSIX_FALLOCATE) && !defined(HAVE_FALLOCATE)
# define fallocate(a,ignored,b,c) posix_fallocate(a,b,c)
#endif
- </pre>
+</pre>
<h2><a href="types">C types</a></h2>
@@ -266,45 +282,51 @@
<h3>Scalars</h3>
<ul>
- <li>If you're using "int" or "long", odds are good that there's a better type.</li>
+ <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 size_t (use ssize_t only if required).</li>
- <li>If it's file-size related, use uintmax_t, or maybe off_t.</li>
- <li>If it's file-offset related (i.e., signed), use off_t.</li>
- <li>If it's just counting small numbers use "unsigned int";
+ <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 "bool" type
- and use the corresponding "true" and "false" macros. It's ok
- to include <stdbool.h>, since libvirt's use of gnulib ensures
+ <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 int32_t, uint32_t, uint64_t, etc.</li>
- <li>While using "bool" is good for readability, it comes with minor caveats:
+ 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 "bool" in places where the type size must be constant across
+ <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 "bool" in libvirt's
- logical wire protocol, since XDR maps that to its lower-level bool_t
- type, which *is* fixed-size.</li>
- <li>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)...".</li>
+ 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 size_t, pid_t or
- off_t, use matching types for any corresponding variables.
+ 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., "unsigned int" as a type, and that
+ 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 *wrong* type, if "pulling the thread"
+ 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>
@@ -317,9 +339,9 @@
<h3>Pointers</h3>
<p>
- Ensure that all of your pointers are "const-correct".
+ 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 "const" attribute. That way, the reader knows
+ 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
@@ -336,57 +358,57 @@
</p>
<ul>
- <li><p>eg to allocate a single object:</p>
-
+ <li><p>e.g. to allocate a single object:</p>
<pre>
- virDomainPtr domain;
+ virDomainPtr domain;
- if (VIR_ALLOC(domain) < 0) {
- virReportOOMError();
- return NULL;
- }
-</pre></li>
-
- <li><p>eg to allocate an array of objects</p>
+ if (VIR_ALLOC(domain) < 0) {
+ virReportOOMError();
+ return NULL;
+ }
+</pre>
+ </li>
+ <li><p>e.g. to allocate an array of objects</p>
<pre>
- virDomainPtr domains;
- int ndomains = 10;
-
- if (VIR_ALLOC_N(domains, ndomains) < 0) {
- virReportOOMError();
- return NULL;
- }
-</pre></li>
+ virDomainPtr domains;
+ int ndomains = 10;
- <li><p>eg to allocate an array of object pointers</p>
+ if (VIR_ALLOC_N(domains, ndomains) < 0) {
+ virReportOOMError();
+ return NULL;
+ }
+</pre>
+ </li>
+ <li><p>e.g. to allocate an array of object pointers</p>
<pre>
- virDomainPtr *domains;
- int ndomains = 10;
+ virDomainPtr *domains;
+ int ndomains = 10;
- if (VIR_ALLOC_N(domains, ndomains) < 0) {
- virReportOOMError();
- return NULL;
- }
-</pre></li>
-
- <li><p>eg to re-allocate the array of domains to be longer</p>
+ if (VIR_ALLOC_N(domains, ndomains) < 0) {
+ virReportOOMError();
+ return NULL;
+ }
+</pre>
+ </li>
+ <li><p>e.g. to re-allocate the array of domains to be longer</p>
<pre>
- ndomains = 20
-
- if (VIR_REALLOC_N(domains, ndomains) < 0) {
- virReportOOMError();
- return NULL;
- }
-</pre></li>
+ ndomains = 20
- <li><p>eg to free the domain</p>
+ if (VIR_REALLOC_N(domains, ndomains) < 0) {
+ virReportOOMError();
+ return NULL;
+ }
+</pre>
+ </li>
+ <li><p>e.g. to free the domain</p>
<pre>
- VIR_FREE(domain);
-</pre></li>
+ VIR_FREE(domain);
+</pre>
+ </li>
</ul>
<h2><a name="file_handling">File handling</a></h2>
@@ -398,20 +420,21 @@
</p>
<ul>
- <li><p>eg close a file descriptor</p>
-
+ <li><p>e.g. close a file descriptor</p>
<pre>
- if (VIR_CLOSE(fd) < 0) {
- virReportSystemError(errno, _("failed to close file"));
- }
-</pre></li>
+ if (VIR_CLOSE(fd) < 0) {
+ virReportSystemError(errno, _("failed to close file"));
+ }
+</pre>
+ </li>
<li><p>eg close a file descriptor in an error path, without losing
the previous errno value</p>
<pre>
- VIR_FORCE_CLOSE(fd);
-</pre></li>
+ VIR_FORCE_CLOSE(fd);
+</pre>
+ </li>
</ul>
<h2><a name="string_comparision">String comparisons</a></h2>
@@ -423,39 +446,36 @@
<ul>
<li><p>For strict equality:</p>
- <pre>
- STREQ(a,b)
- STRNEQ(a,b)
+<pre>
+ STREQ(a,b)
+ STRNEQ(a,b)
</pre>
</li>
<li><p>For case insensitive equality:</p>
- <pre>
- STRCASEEQ(a,b)
- STRCASENEQ(a,b)
+<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>
+ 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>
+ STRCASEEQLEN(a,b,n)
+ STRCASENEQLEN(a,b,n)
</pre>
</li>
<li><p>For strict equality of a prefix:</p>
-
- <pre>
- STRPREFIX(a,b)
+<pre>
+ STRPREFIX(a,b)
</pre>
</li>
</ul>
@@ -469,7 +489,10 @@
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>
+
+<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,
@@ -481,8 +504,9 @@
trailing \0 is appended.
</p>
- <pre>virStrcpy(char *dest, const char *src, size_t destbytes)</pre>
-
+<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
@@ -490,11 +514,12 @@
virStrncpy(dest, src, strlen(src), destbytes)
</p>
- <pre>virStrcpyStatic(char *dest, const char *src)</pre>
-
+<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 *and* you know that your destination string is
+ 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
@@ -511,9 +536,10 @@
<p>eg typical usage is as follows:</p>
- <pre>
+<pre>
char *
- somefunction(...) {
+ somefunction(...)
+ {
virBuffer buf = VIR_BUFFER_INITIALIZER;
...
@@ -545,7 +571,7 @@
*.c source files:
</p>
- <pre>
+<pre>
/*
* Copyright notice
* ....
@@ -561,7 +587,7 @@
#include <limits.h>
#if HAVE_NUMACTL Some system includes aren't supported
- # include <numa.h> everywhere so need these #if guards.
+ # include <numa.h> everywhere so need these #if guards.
#endif
#include "internal.h" Include this first, after system includes.
@@ -569,13 +595,13 @@
#include "util.h" Any libvirt internal header files.
#include "buf.h"
- static myInternalFunc () The actual code.
+ static myInternalFunc() The actual code.
{
- ...
+ ...
</pre>
<p>
- Of particular note: *DO NOT* include libvirt/libvirt.h or
+ 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.
@@ -591,9 +617,9 @@
the one for virAsprintf, in util.h:
</p>
- <pre>
- int virAsprintf(char **strp, const char *fmt, ...)
- ATTRIBUTE_FORMAT(printf, 2, 3);
+<pre>
+ int virAsprintf(char **strp, const char *fmt, ...)
+ ATTRIBUTE_FORMAT(printf, 2, 3);
</pre>
<p>
@@ -654,7 +680,7 @@
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>
+ <a href="http://kerneltrap.org/node/553/2131";>KernelTrap</a>
</p>
<p>
@@ -662,11 +688,12 @@
makes sense:
</p>
- <pre>
+<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 (eg retry on EINTR)</pre>
+ retry: If needing to jump upwards (eg retry on EINTR)
+</pre>
@@ -691,7 +718,7 @@
configure with
</p>
<pre>
- --enable-compile-warnings=error
+ --enable-compile-warnings=error
</pre>
<p>
which adds -Werror to compile flags, so no warnings get missed
--
1.7.0.4
From 92dd2632553c011b2511772346b138c2eab627d8 Mon Sep 17 00:00:00 2001
From: Matthias Bolte <matthias.bolte@xxxxxxxxxxxxxx>
Date: Fri, 12 Nov 2010 15:36:53 +0100
Subject: [PATCH] Generate HACKING from docs/hacking.html.in
---
HACKING | 599 +++++++++++++++++++++++++++++----------------------
Makefile.am | 9 +
docs/hacking1.xsl | 28 +++
docs/hacking2.xsl | 146 +++++++++++++
docs/wrapstring.xsl | 56 +++++
5 files changed, 581 insertions(+), 257 deletions(-)
create mode 100644 docs/hacking1.xsl
create mode 100644 docs/hacking2.xsl
create mode 100644 docs/wrapstring.xsl
diff --git a/HACKING b/HACKING
index a9a9b49..cf0f382 100644
--- a/HACKING
+++ b/HACKING
@@ -1,18 +1,19 @@
-*- buffer-read-only: t -*- vi: set ro:
DO NOT EDIT THIS FILE! IT IS GENERATED AUTOMATICALLY!
- Libvirt contributor guidelines
- ==============================
+
+
+ Contributor guidelines
+ ======================
+
General tips for contributing patches
=====================================
+(1) Discuss any large changes on the mailing list first. Post patches early and
+listen to feedback.
-(1) Discuss any large changes on the mailing list first. Post patches
-early and listen to feedback.
-
-(2) Post patches in unified diff format. A command similar to this
-should work:
+(2) Post patches in unified diff format. A command similar to this should work:
diff -urp libvirt.orig/ libvirt.modified/ > libvirt-myfeature.patch
@@ -20,15 +21,15 @@ or:
git diff > libvirt-myfeature.patch
-(3) 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.
+(3) 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.
-(4) Make sure your patches apply against libvirt GIT. Developers
-only follow GIT and don't care much about released versions.
+(4) Make sure your patches apply against libvirt GIT. Developers only follow GIT
+and don't care much about released versions.
-(5) Run the automated tests on your code before submitting any changes.
-In particular, configure with compile warnings set to -Werror:
+(5) Run the automated tests on your code before submitting any changes. In
+particular, configure with compile warnings set to -Werror:
./configure --enable-compile-warnings=error
@@ -47,28 +48,29 @@ VIR_TEST_DEBUG may provide larger amounts of information:
VIR_TEST_DEBUG=1 make check (or)
VIR_TEST_DEBUG=2 make check
-Also, individual tests can be run from inside the 'tests/' directory, like:
+Also, individual tests can be run from inside the "tests/" directory, like:
./qemuxml2xmltest
-(6) Update tests and/or documentation, particularly if you are adding
-a new feature or changing the output of a program.
+(6) Update tests and/or documentation, particularly if you are adding a new
+feature or changing the output of a program.
-There is more on this subject, including lots of links to background
-reading on the subject, on this page:
- http://et.redhat.com/~rjones/how-to-supply-code-to-open-source-projects/
+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://et.redhat.com/~rjones/how-to-supply-code-to-open-source-projects/
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.
+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, add the following to one of one of your start-up files
(e.g., ~/.emacs), to help ensure that you get indentation right:
@@ -82,15 +84,15 @@ If you use Emacs, add the following to one of one of your start-up files
(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))))
+ '(lambda () (if (string-match "/libvirt" (buffer-file-name))
+ (libvirt-c-mode))))
+
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:
+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()
{
@@ -99,66 +101,63 @@ around operators and keywords:
--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.
+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.
Curly braces
============
Omit the curly braces around an "if", "while", "for" 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-*statement* loop: each
-has only one *line* in its body.
+ensures that it is trivially easy to identify a single-'statement' loop: each
+has only one 'line' in its body.
Omitting braces with a single-line body is fine:
- while (expr) // one-line body -> omitting curly braces is ok
- single_line_stmt ();
+ while (expr) // one-line body -> omitting curly braces is ok
+ single_line_stmt();
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:
+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 ();
+ 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 ();
- }
+ 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"));
+ 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
- ...
- }
+ if (expr) // BAD: no braces around...
+ while (expr_2) { // ... a multi-line body
+ ...
+ }
Do this, instead:
- if (expr) {
- while (expr_2) {
- ...
- }
- }
+ 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
@@ -167,47 +166,47 @@ case, either put braces around the "else" block, or negate the "if"-condition
and swap the bodies, putting the one-line block first and making the longer,
multi-line block be the "else" block.
- if (expr) {
- ...
- ...
- }
- else
- x = y; // BAD: braceless "else" with braced "then"
+ if (expr) {
+ ...
+ ...
+ }
+ else
+ x = y; // BAD: braceless "else" with braced "then"
This 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
+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 {
- ...
- ...
- }
+ if (!expr)
+ x = y; // putting the smaller block first is more readable
+ else {
+ ...
+ ...
+ }
If you'd rather not negate the condition, then at least add braces:
- if (expr) {
- ...
- ...
- } else {
- x = y;
- }
+ if (expr) {
+ ...
+ ...
+ } else {
+ x = y;
+ }
Preprocessor
============
For variadic macros, stick with C99 syntax:
-#define vshPrint(_ctl, ...) fprintf(stdout, __VA_ARGS__)
+ #define vshPrint(_ctl, ...) fprintf(stdout, __VA_ARGS__)
-Use parenthesis when checking if a macro is defined, and use
-indentation to track nesting:
+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
+ #if defined(HAVE_POSIX_FALLOCATE) && !defined(HAVE_FALLOCATE)
+ # define fallocate(a,ignored,b,c) posix_fallocate(a,b,c)
+ #endif
C types
@@ -216,199 +215,235 @@ 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. It's ok
-to include <stdbool.h>, since libvirt's use of gnulib ensures
-that it exists and is usable.
-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.
+- 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. It's ok to include <stdbool.h>, since
+libvirt's use of gnulib ensures that it exists and is usable.
+
+- 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.
+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.
Low level memory management
===========================
-
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
+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
- - eg to allocate a single object:
+- e.g. to allocate a single object:
- virDomainPtr domain;
+ virDomainPtr domain;
+
+ if (VIR_ALLOC(domain) < 0) {
+ virReportOOMError();
+ return NULL;
+ }
- if (VIR_ALLOC(domain) < 0) {
- virReportOOMError();
- return NULL;
- }
- - eg to allocate an array of objects
+- e.g. to allocate an array of objects
- virDomainPtr domains;
- int ndomains = 10;
+ virDomainPtr domains;
+ int ndomains = 10;
+
+ if (VIR_ALLOC_N(domains, ndomains) < 0) {
+ virReportOOMError();
+ return NULL;
+ }
- if (VIR_ALLOC_N(domains, ndomains) < 0) {
- virReportOOMError();
- return NULL;
- }
- - eg to allocate an array of object pointers
- virDomainPtr *domains;
- int ndomains = 10;
+- e.g. to allocate an array of object pointers
- if (VIR_ALLOC_N(domains, ndomains) < 0) {
- virReportOOMError();
- return NULL;
- }
+ virDomainPtr *domains;
+ int ndomains = 10;
- - eg to re-allocate the array of domains to be longer
+ if (VIR_ALLOC_N(domains, ndomains) < 0) {
+ virReportOOMError();
+ return NULL;
+ }
+
+
+
+- e.g. to re-allocate the array of domains to be longer
+
+ ndomains = 20
+
+ if (VIR_REALLOC_N(domains, ndomains) < 0) {
+ virReportOOMError();
+ return NULL;
+ }
+
+
+
+- e.g. to free the domain
+
+ VIR_FREE(domain);
- ndomains = 20
- if (VIR_REALLOC_N(domains, ndomains) < 0) {
- virReportOOMError();
- return NULL;
- }
- - eg to free the domain
- VIR_FREE(domain);
File handling
=============
-
Use of the close() API is deprecated in libvirt code base to help avoiding
double-closing of a file descriptor. Instead of this API, use the macro from
files.h
- - eg close a file descriptor
+- e.g. close a file descriptor
+
+ if (VIR_CLOSE(fd) < 0) {
+ virReportSystemError(errno, _("failed to close file"));
+ }
+
+
+
+- eg close a file descriptor in an error path, without losing the previous errno
+value
+
+ VIR_FORCE_CLOSE(fd);
+
+
- if (VIR_CLOSE(fd) < 0) {
- virReportSystemError(errno, _("failed to close file"));
- }
- - eg close a file descriptor in an error path, without losing the previous
- errno value
- VIR_FORCE_CLOSE(fd);
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)
+
+
-Do not use the strcmp, strncmp, etc functions directly. Instead use
-one of the following semantically named macros
+- For strict equality of a substring:
- - For strict equality:
+ STREQLEN(a,b,n)
+ STRNEQLEN(a,b,n)
- STREQ(a,b)
- STRNEQ(a,b)
- - For case insensitive equality:
- STRCASEEQ(a,b)
- STRCASENEQ(a,b)
- - For strict equality of a substring:
+- For case insensitive equality of a substring:
- STREQLEN(a,b,n)
- STRNEQLEN(a,b,n)
+ STRCASEEQLEN(a,b,n)
+ STRCASENEQLEN(a,b,n)
- - For case insensitive equality of a substring:
- STRCASEEQLEN(a,b,n)
- STRCASENEQLEN(a,b,n)
- - For strict equality of a prefix:
+- For strict equality of a prefix:
+
+ STRPREFIX(a,b)
+
+
- STRPREFIX(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 functionally equivalent functions:
-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 functionally equivalent functions:
+ virStrncpy(char *dest, const char *src, size_t n, size_t destbytes)
- - virStrncpy(char *dest, const char *src, size_t n, size_t destbytes)
- 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.
+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.
- - 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. Note that this is a macro, so arguments could be
- evaluated more than once. This is equivalent to
- virStrncpy(dest, src, strlen(src), destbytes)
+ virStrcpy(char *dest, const char *src, size_t destbytes)
- - 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. This is
- equivalent to virStrncpy(dest, src, strlen(src), sizeof(dest)).
+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)
+ 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. This is equivalent to
+virStrncpy(dest, src, strlen(src), sizeof(dest)).
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 the virBuffer API described in buf.h
+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
eg typical usage is as follows:
char *
- somefunction(...) {
+ somefunction(...)
+ {
virBuffer buf = VIR_BUFFER_INITIALIZER;
...
@@ -432,11 +467,9 @@ eg typical usage is as follows:
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:
+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
@@ -461,59 +494,111 @@ complexity it's best to stick to the following general plan for all
#include "util.h" Any libvirt internal header files.
#include "buf.h"
- static myInternalFunc () The actual code.
+ static myInternalFunc() The actual code.
{
- ...
+ ...
-Of particular note: *DO NOT* 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.
+Of particular note: *Do not* 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.
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
+virAsprintf, in util.h:
+
+ int virAsprintf(char **strp, const char *fmt, ...)
+ ATTRIBUTE_FORMAT(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.
+
+
+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.
-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:
+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.
- int virAsprintf(char **strp, const char *fmt, ...)
- ATTRIBUTE_FMT_PRINTF(2, 3);
+There are a couple of signs that a particular use of goto is not ok:
-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.
+- 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.
- Libvirt committer guidelines
- ============================
-The AUTHORS files indicates the list of people with commit access right
-who can actually merge the patches.
+
+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 (eg retry on EINTR)
+
+
+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 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 and run 'make check syntax-check' and make sure they
-don't raise error. Try to look for warnings too for example configure with
- --enable-compile-warnings=error
+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
+
+ --enable-compile-warnings=error
+
which adds -Werror to compile flags, so no warnings get missed
-Exceptions to that '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
- - similarly, 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
-Similar fixes for documentation and code comments can be managed
-in the same way, but still make sure they get reviewed if non-trivial.
+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.
diff --git a/Makefile.am b/Makefile.am
index d05b7da..1a23229 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -56,6 +56,15 @@ NEWS: $(top_srcdir)/docs/news.xsl $(top_srcdir)/docs/news.html.in
| perl -pe 's/[ \t]+$$//' \
> $@-t && mv $@-t $@ ; fi );
+syntax-check: $(top_srcdir)/HACKING
+
+$(top_srcdir)/HACKING: $(top_srcdir)/docs/hacking1.xsl $(top_srcdir)/docs/hacking2.xsl \
+ $(top_srcdir)/docs/wrapstring.xsl $(top_srcdir)/docs/hacking.html.in
+ -@(if [ -x $(XSLTPROC) ] ; then \
+ $(XSLTPROC) --nonet $(top_srcdir)/docs/hacking1.xsl $(top_srcdir)/docs/hacking.html.in | \
+ $(XSLTPROC) --nonet $(top_srcdir)/docs/hacking2.xsl - \
+ | perl -0777 -pe 's/\n\n+$$/\n/' \
+ > $@-t && mv $@-t $@ ; fi );
rpm: clean
@(unset CDPATH ; $(MAKE) dist && rpmbuild -ta $(distdir).tar.gz)
diff --git a/docs/hacking1.xsl b/docs/hacking1.xsl
new file mode 100644
index 0000000..982fa8d
--- /dev/null
+++ b/docs/hacking1.xsl
@@ -0,0 +1,28 @@
+<?xml version="1.0"?>
+<xsl:stylesheet version="1.0" xmlns:xsl="http://www.w3.org/1999/XSL/Transform";>
+
+<xsl:output method="xml" encoding="UTF-8" indent="no"/>
+
+
+
+<xsl:template match="/">
+ <xsl:apply-templates/>
+</xsl:template>
+
+
+
+<xsl:template match="@*|node()">
+ <xsl:copy>
+ <xsl:apply-templates select="@*|node()"/>
+ </xsl:copy>
+</xsl:template>
+
+
+
+<!-- resolve b/i/code tags in a first pass, because they interfere with line
+ wrapping in the second pass -->
+<xsl:template match="b">*<xsl:apply-templates/>*</xsl:template>
+<xsl:template match="i">'<xsl:apply-templates/>'</xsl:template>
+<xsl:template match="code">"<xsl:apply-templates/>"</xsl:template>
+
+</xsl:stylesheet>
diff --git a/docs/hacking2.xsl b/docs/hacking2.xsl
new file mode 100644
index 0000000..89e777b
--- /dev/null
+++ b/docs/hacking2.xsl
@@ -0,0 +1,146 @@
+<?xml version="1.0"?>
+<xsl:stylesheet version="1.0" xmlns:xsl="http://www.w3.org/1999/XSL/Transform";>
+
+<xsl:import href="wrapstring.xsl"/>
+
+<xsl:output method="text" encoding="UTF-8" indent="no"/>
+
+<xsl:strip-space elements="*"/>
+
+
+
+<xsl:variable name="newline">
+<xsl:text>
+</xsl:text>
+</xsl:variable>
+
+
+
+<xsl:template match="/">
+<xsl:text>-*- buffer-read-only: t -*- vi: set ro:
+DO NOT EDIT THIS FILE! IT IS GENERATED AUTOMATICALLY!
+
+
+
+</xsl:text>
+<xsl:apply-templates/>
+</xsl:template>
+
+
+
+<!-- title -->
+<xsl:template match="h1">
+<xsl:text> </xsl:text>
+<xsl:value-of select="normalize-space(.)"/>
+<xsl:text>
+ </xsl:text>======================
+</xsl:template>
+
+
+
+<!-- output the current text node underlined -->
+<xsl:template name="underline">
+ <xsl:param name="text" select="normalize-space(.)"/>
+ <xsl:param name="text-length" select="string-length($text)"/>
+ <xsl:param name="char" select="'='"/>
+ <xsl:param name="line" select="$char"/>
+ <xsl:choose>
+ <xsl:when test="$text-length > 1">
+ <xsl:call-template name="underline">
+ <xsl:with-param name="text" select="$text"/>
+ <xsl:with-param name="text-length" select="$text-length - 1"/>
+ <xsl:with-param name="char" select="$char"/>
+ <xsl:with-param name="line" select="concat($line,$char)"/>
+ </xsl:call-template>
+ </xsl:when>
+ <xsl:otherwise>
+<xsl:value-of select="$text"/>
+<xsl:value-of select="$newline"/>
+<xsl:value-of select="$line"/>
+<xsl:value-of select="$newline"/>
+ </xsl:otherwise>
+ </xsl:choose>
+</xsl:template>
+
+
+
+<xsl:template match="h2">
+<xsl:value-of select="$newline"/>
+<xsl:call-template name="underline"/>
+</xsl:template>
+
+
+
+<xsl:template match="h3">
+<xsl:call-template name="underline">
+<xsl:with-param name="char" select="'-'"/>
+</xsl:call-template>
+</xsl:template>
+
+
+
+<!-- output text line wrapped at 80 chars -->
+<xsl:template match="text()">
+<xsl:call-template name="wrap-string">
+<xsl:with-param name="str" select="normalize-space(.)"/>
+<xsl:with-param name="wrap-col" select="80"/>
+<xsl:with-param name="break-mark" select="$newline"/>
+</xsl:call-template>
+</xsl:template>
+
+
+
+<xsl:template match="ol|ul|p">
+<xsl:apply-templates/><xsl:value-of select="$newline"/><xsl:value-of select="$newline"/>
+</xsl:template>
+
+
+
+<xsl:template match="ol/li">
+<xsl:choose>
+<xsl:when test=".//node()[position()=last()]/self::pre">(<xsl:value-of select="position()"/>) <xsl:apply-templates/>
+</xsl:when>
+<!-- only append two newlines when the last element isn't a pre element -->
+<xsl:otherwise>(<xsl:value-of select="position()"/>) <xsl:apply-templates/><xsl:value-of select="$newline"/><xsl:value-of select="$newline"/>
+</xsl:otherwise>
+</xsl:choose>
+</xsl:template>
+
+
+
+<xsl:template match="ul/li">- <xsl:apply-templates/><xsl:value-of select="$newline"/><xsl:value-of select="$newline"/>
+</xsl:template>
+
+
+
+<xsl:template match="li/ul/li">-- <xsl:apply-templates/><xsl:value-of select="$newline"/><xsl:value-of select="$newline"/>
+</xsl:template>
+
+
+
+<!-- add newline before nested <ul> -->
+<xsl:template match="li/ul"><xsl:value-of select="$newline"/><xsl:value-of select="$newline"/><xsl:apply-templates/>
+</xsl:template>
+
+
+
+<xsl:template match="pre">
+<xsl:choose>
+<xsl:when test="starts-with(.,'
')"><xsl:value-of select="substring(.,2)"/><xsl:value-of select="$newline"/>
+</xsl:when>
+<xsl:otherwise>
+<xsl:value-of select="."/><xsl:value-of select="$newline"/>
+</xsl:otherwise>
+</xsl:choose>
+</xsl:template>
+
+
+
+<xsl:template match="a">
+<xsl:value-of select="$newline"/><xsl:value-of select="$newline"/>
+<xsl:text> </xsl:text><xsl:apply-templates/>
+<xsl:value-of select="$newline"/>
+<xsl:text> </xsl:text><xsl:value-of select="@href"/>
+</xsl:template>
+
+</xsl:stylesheet>
diff --git a/docs/wrapstring.xsl b/docs/wrapstring.xsl
new file mode 100644
index 0000000..b468a70
--- /dev/null
+++ b/docs/wrapstring.xsl
@@ -0,0 +1,56 @@
+<?xml version="1.0"?>
+<xsl:stylesheet version="1.0" xmlns:xsl="http://www.w3.org/1999/XSL/Transform";>
+
+<!-- based on http://plasmasturm.org/log/xslwordwrap/ -->
+<!-- Copyright 2010 Aristotle Pagaltzis; under the MIT licence -->
+<!-- http://www.opensource.org/licenses/mit-license.php -->
+<xsl:template name="wrap-string">
+ <xsl:param name="str" />
+ <xsl:param name="wrap-col" />
+ <xsl:param name="break-mark" />
+ <xsl:param name="pos" select="0" />
+ <xsl:choose>
+ <xsl:when test="contains( $str, ' ' )">
+ <xsl:variable name="first-word" select="substring-before( $str, ' ' )" />
+ <xsl:variable name="pos-now" select="$pos + 1 + string-length( $first-word )" />
+ <xsl:choose>
+ <xsl:when test="$pos > 0 and $pos-now >= $wrap-col">
+ <xsl:copy-of select="$break-mark" />
+ <xsl:call-template name="wrap-string">
+ <xsl:with-param name="str" select="$str" />
+ <xsl:with-param name="wrap-col" select="$wrap-col" />
+ <xsl:with-param name="break-mark" select="$break-mark" />
+ <xsl:with-param name="pos" select="0" />
+ </xsl:call-template>
+ </xsl:when>
+ <xsl:otherwise>
+ <xsl:if test="$pos > 0">
+ <xsl:text> </xsl:text>
+ </xsl:if>
+ <xsl:value-of select="$first-word" />
+ <xsl:call-template name="wrap-string">
+ <xsl:with-param name="str" select="substring-after( $str, ' ' )" />
+ <xsl:with-param name="wrap-col" select="$wrap-col" />
+ <xsl:with-param name="break-mark" select="$break-mark" />
+ <xsl:with-param name="pos" select="$pos-now" />
+ </xsl:call-template>
+ </xsl:otherwise>
+ </xsl:choose>
+ </xsl:when>
+ <xsl:otherwise>
+ <xsl:choose>
+ <xsl:when test="$pos + string-length( $str ) >= $wrap-col">
+ <xsl:copy-of select="$break-mark" />
+ </xsl:when>
+ <xsl:otherwise>
+ <xsl:if test="$pos > 0">
+ <xsl:text> </xsl:text>
+ </xsl:if>
+ </xsl:otherwise>
+ </xsl:choose>
+ <xsl:value-of select="$str" />
+ </xsl:otherwise>
+ </xsl:choose>
+</xsl:template>
+
+</xsl:stylesheet>
--
1.7.0.4
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list