It has been enabled by default for over three years now: commit 5d2a30d7d8777319c745804f040fa405d02169ce Author: Junio C Hamano <gitster@pobox.com> CommitDate: 2016-04-03 10:29:22 -0700 Merge branch 'mm/diff-renames-default' https://github.com/git/git/commit/5d2a30d7d8777319c745804f040fa405d02169ce Signed-off-by: Ján Tomko <jtomko@redhat.com> --- docs/hacking.html.in | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 981b782d31..1da2cbbd9f 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -29,11 +29,7 @@ file from zanata.</p> </li> - <li><p>Post patches using <code>git send-email</code>, with git - rename detection enabled. You need a one-time setup of:</p> -<pre> - git config diff.renames true -</pre> + <li><p>Post 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> -- 2.21.0

This tool takes care of many of the tedious parts of submitting a patch. Mention it first, above the "manual" way using git send-email. Signed-off-by: Ján Tomko <jtomko@redhat.com> Cc: Stefan Hajnoczi <stefanha@redhat.com> --- docs/hacking.html.in | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 1da2cbbd9f..42c8596abe 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -29,7 +29,11 @@ file from zanata.</p> </li> - <li><p>Post patches using <code>git send-email</code>.</p> + <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> -- 2.21.0

Namely: * holding up the first-time patch submissions for moderation, which might cause first-time submitters to question the process * not CC-ing individual developers Signed-off-by: Ján Tomko <jtomko@redhat.com> --- docs/hacking.html.in | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 42c8596abe..71a6ccc18b 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -72,8 +72,8 @@ </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 - please don't CC individual developers unless they've explicitly - asked you to.</p> + <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 @@ -81,9 +81,10 @@ <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. Note - that your very first post to the mailing list will be subject to - moderation, and it's not uncommon for that to take around a day.</p> + there after an hour or so, you should double-check your setup. + <strong>Note that 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> -- 2.21.0

Our HACKING file is clear about requiring submission from a git checkout, which automatically enables -Werror. Remove the mentions of explicitly enabling it to alleviate the collective cognitive encumbrance. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- docs/hacking.html.in | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 71a6ccc18b..d8f748626e 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -140,14 +140,7 @@ </li> <li><p>Run the automated tests on your code before submitting any changes. - In particular, configure with compile warnings set to - -Werror. This is done automatically for a git checkout; from a - tarball, use:</p> -<pre> - ./configure --enable-werror -</pre> - <p> - and run the tests: + That is: </p> <pre> make check @@ -1473,14 +1466,7 @@ int foo() 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 + don't raise errors. </p> <p> -- 2.21.0

Mention a more complex example. Invoke the test without 'make' since the mentioned example does not seem to be working anymore. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- docs/hacking.html.in | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/hacking.html.in b/docs/hacking.html.in index d8f748626e..1e16813868 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -178,12 +178,13 @@ <p> When debugging failures during development, it is possible - to focus in on just the failing subtests by using TESTS and - VIR_TEST_RANGE: + 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> - make check VIR_TEST_DEBUG=1 VIR_TEST_RANGE=3-5 TESTS=qemuxml2argvtest + VIR_TEST_DEBUG=1 VIR_TEST_RANGE=3-5,7-20,^16 ./run tests/qemuxml2argvtest </pre> <p> -- 2.21.0

Mention the various ATTRIBUTE* macros and ARRAY_CARDINALITY that were removed earlier. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- docs/hacking.html.in | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 1e16813868..b270aa69e3 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -1072,6 +1072,14 @@ BAD: <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> </table> -- 2.21.0

Mention all the __atribute__ annotations we use to make the compiler and/or the static analysis tools understand the code better. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- docs/hacking.html.in | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/docs/hacking.html.in b/docs/hacking.html.in index b270aa69e3..ad0f595897 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -984,6 +984,25 @@ BAD: 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> -- 2.21.0

Document the preferred alternatives to existing libvirt macros for allocating strings. These cannot be deleted just yet because converting them will require a lot of work. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- docs/hacking.html.in | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/docs/hacking.html.in b/docs/hacking.html.in index f39f6433d4..d4f1c2baad 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -1071,6 +1071,18 @@ BAD: <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>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> + <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> -- 2.21.0

Recommend g_str(n)dup instead of VIR_STRDUP. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- docs/hacking.html.in | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/docs/hacking.html.in b/docs/hacking.html.in index d4f1c2baad..d6a4f04ad0 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -1274,18 +1274,14 @@ BAD: </p> <pre> - VIR_STRDUP(char *dst, const char *src); - VIR_STRNDUP(char *dst, const char *src, size_t n); + dst = g_strcpy(src); + dst = g_strncpy(src, n) </pre> <p> - You should avoid using strdup or strndup directly as they do not report - out-of-memory error, and do not allow a NULL source. Use - VIR_STRDUP or VIR_STRNDUP macros instead, which return 0 for - NULL source, 1 for successful copy, and -1 for allocation - failure with the error already reported. In very - specific cases, when you don't want to report the out-of-memory error, you - can use VIR_STRDUP_QUIET or VIR_STRNDUP_QUIET, but such usage is very rare - and usually considered a flaw. + 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_strcpy</code> and <code>g_strncpy</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> -- 2.21.0

Recommend GString for generic strings and virBuffer for strings that need helpers for other uses, like XML or command line formatting. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- docs/hacking.html.in | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/docs/hacking.html.in b/docs/hacking.html.in index d6a4f04ad0..384da96d60 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -1289,7 +1289,11 @@ BAD: <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 virbuffer.h + make use of either the + <a href="https://developer.gnome.org/glib/stable/glib-Strings.html">GString</a> + type from GLib. + 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> @@ -1298,12 +1302,14 @@ BAD: char * somefunction(...) { - virBuffer buf = VIR_BUFFER_INITIALIZER; + 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"); @@ -1387,12 +1393,10 @@ BAD: </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. + 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> -- 2.21.0

Include the 'semi-automatic' updates in the list of patches pushed at maintainers' discretion to match current practice. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- docs/hacking.html.in | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 20127d6ece..3c8cceaec0 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -1554,6 +1554,10 @@ int foo() 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> -- 2.21.0

s/verca/versa/ Signed-off-by: Ján Tomko <jtomko@redhat.com> --- docs/hacking.html.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 3c8cceaec0..73d137ab1d 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -1105,7 +1105,7 @@ BAD: 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-verca. + is declared with the libvirt macros and vice-versa. </dd> <dt><code>VIR_AUTOUNREF</code></dt> -- 2.21.0