On Sat, Oct 19, 2019 at 12:36:37AM +0200, Ján Tomko wrote: s/Note that/Note that, if you are not already a subscriber,/ Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Document the preferred alternatives to existing libvirt macros for memory allocation. These cannot be deleted just yet because converting them will require a lot of work. Signed-off-by: Ján Tomko <jtomko(a)redhat.com&gt; --- docs/hacking.html.in | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/docs/hacking.html.in b/docs/hacking.html.in index ad0f595897..f39f6433d4 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -1050,6 +1050,27 @@ BAD: <dt><code>virStrerror</code></dt> <dd>The GLib <code>g_strerror()</code> function should be used instead, which has a simpler calling convention as an added benefit.</dd> + + <table class="top_table"> + <tr><th>deprecated version</th><th>GLib version</th><th>Notes</th></tr> + <tr><td><code>VIR_ALLOC(var)</code></td><td><code>g_new(var_t, 1)</code></td> + <td>yes, you need to tell it the type explicitly</td></tr> + <tr><td><code>VIR_ALLOC_N</code></td><td><code>g_new0(var_t, n)</code></td><td></td></tr> + <tr><td><code>VIR_REALLOC_N</code></td><td><code>g_renew(var_t, ptr, n)</code></td> + <td>the newly added memory is not zeroed</td></tr> + <tr><td><code>VIR_EXPAND_N</code></td><td><code>g_renew(var_t, ptr, n)</code></td> + <td>zero the new memory manually or use an array type</td></tr> + <tr><td><code>VIR_SHRINK_N</code></td><td>no alternative</td> + <td></td></tr> + <tr><td><code>VIR_APPEND_ELEMENT</code></td><td><code>g_array_append_val</code></td> + <td><code>g_ptr_array_add</code> or <code>g_byte_array_append</code></td></tr> + <tr><td><code>VIR_INSERT_ELEMENT</code></td><td><code>g_array_insert_val</code></td> + <td><code>g_ptr_array_insert</code></td></tr> + <tr><td><code>VIR_DELETE_ELEMENT</code></td><td><code>g_array_remove_index</code></td> + <td><code>g_ptr_array_remove_index</code> or <code>g_byte_array_remove_index</code></td></tr> + <tr><td><code>VIR_FREE</code></td><td><code>g_free</code></td> + <td><code>g_free</code> does not zero the pointer</td></tr> + </table> </dl> <p> -- 2.21.0

Prefer G_GNUC_PRINTF. Also, pick another example than virAsprintf since it may get removed in the future. Signed-off-by: Ján Tomko <jtomko(a)redhat.com&gt; --- docs/hacking.html.in | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 384da96d60..8c68cd1c3c 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -1378,12 +1378,12 @@ BAD: 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 one for virCommandAddEnvFormat in vircommand.h: </p> <pre> - int virAsprintf(char **strp, const char *fmt, ...) - ATTRIBUTE_FORMAT(printf, 2, 3); + void virCommandAddEnvFormat(virCommandPtr cmd, const char *format, ...) + G_GNUC_PRINTF(2, 3); </pre> <p> -- 2.21.0

Replace reference to VIR_FREE with g_free and mention the use of g_auto cleanup attributes that eliminate most of label use. Signed-off-by: Ján Tomko <jtomko(a)redhat.com&gt; --- docs/hacking.html.in | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 8c68cd1c3c..7f608657b3 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -1446,11 +1446,16 @@ BAD: 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 + overkill. g_free() and most of the functions named XXXFree() in + libvirt is required to handle NULL as its arg. This does not + apply to libvirt's public APIs. Thus you can safely call free on all the variables even if they were not yet allocated (yes they have to have been initialized to NULL). This is much simpler and clearer than having multiple labels. + Note that most of libvirt's type declarations can be marked with + either <code>g_autofree</code> or <code>g_autoptr</code> which uses + the compiler's <code>__attribute__((cleanup))</code> that calls + the appropriate free function when the variable goes out of scope. </p> <p> -- 2.21.0

Mention the possibility of relaxing the rule in the future. Signed-off-by: Ján Tomko <jtomko(a)redhat.com&gt; --- docs/hacking.html.in | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 7f608657b3..20127d6ece 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -1500,8 +1500,10 @@ BAD: </pre> <p> - Top-level labels should be indented by one space (putting them on - the beginning of the line confuses function context detection in git): + Top-level labels are indented by one space, since no indentation + used to confuse function context detection in git. This rule may + be re-evaluated in the future since git handles it correctly now + and not all code formatting tools support this setting: </p> <pre> -- 2.21.0