<div dir="ltr"><div>I appreciate the feedback on this patch!</div><div><br></div><div>I will work on splitting this into multiple patches. I believe this will involve redoing much of the work because I will need to split this patch (a single commit) into many commits. Hence, I'd like to get some confirmation on how I should approach the patch.</div><div><br></div><div>I plan to:</div><div><br></div><div>1. Address the feedback on returning `-errno`, `0`, `-1`, etc. directly instead of setting the local variable, `ret`, and returning `ret`.</div><div>2. Submit a patch per file with only the g_autofree changes.</div><div>3. Submit a patch per file that removes the cleanup sections.</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Nov 19, 2020 at 9:57 AM Laine Stump <<a href="mailto:laine@redhat.com">laine@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 11/19/20 6:46 AM, Tim Wiederhake wrote:<br>
> On Wed, 2020-11-18 at 16:47 -0600, Ryan Gahagan wrote:<br>
>> From: Barrett Schonefeld <<a href="mailto:bschoney@utexas.edu" target="_blank">bschoney@utexas.edu</a>><br>
>><br>
>> additional conversions to the GLib API in src/util per issue #11.<br>
>><br>
>> [...]<br>
<br>
>>       return ret;<br>
>>   }<br>
>>   <br>
> I believe you can remove "cleanup" and "ret" as well. More instances<br>
> below.<br>
<br>
<br>
The method recommended to me by a few reviewers was to make *only* the <br>
conversions to g_autofree in one patch, then have a separate patch that <br>
short-circuits the "goto cleanup"s and removes the cleanup: label from <br>
those functions that now have an "empty" cleanup. This makes review more <br>
mechanical, and thus easier to verify during review.<br>
<br>
<br>
><br>
>> diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c<br>
>> index 9030f9218a..93bc4a129f 100644<br>
>> --- a/src/util/virdnsmasq.c<br>
>> +++ b/src/util/virdnsmasq.c<br>
>> @@ -164,7 +164,7 @@ addnhostsWrite(const char *path,<br>
>>                  dnsmasqAddnHost *hosts,<br>
>>                  unsigned int nhosts)<br>
>>   {<br>
>> -    char *tmp;<br>
>> +    g_autofree char *tmp = NULL;<br>
>>       FILE *f;<br>
>>       bool istmp = true;<br>
>>       size_t i, j;<br>
>> @@ -180,7 +180,7 @@ addnhostsWrite(const char *path,<br>
>>           istmp = false;<br>
>>           if (!(f = fopen(path, "w"))) {<br>
>>               rc = -errno;<br>
>> -            goto cleanup;<br>
>> +            return rc;<br>
> Why not "return -errno;"? More instances below.<br>
<br>
<br>
Yeah, that's another that would be done in the 2nd "remove cleanup: <br>
labels" patch - usually you end up with all the returns just directly <br>
returning some value (usually 0 or -1, but it could be something like <br>
-errno or some other variable), and that very often makes the variable <br>
"ret" (or sometimes, as in this case, "rc") unused, so it would be <br>
removed as a part of the same patch.<br>
<br>
<br>
><br>
> You also might want to split the patch up, e.g. go function by<br>
> function, to create self-contained changes.<br>
<br>
<br>
My opinion is that for a mechanical change like this, a separate patch <br>
for each function is overkill. And I'm on the fence about even having a <br>
separate patch for each file (it depends on how many chunks there are <br>
and how similar/simple the chunks are). Having several identical changes <br>
in one patch makes it easier to scan through all the chunks without <br>
needing to hop from one message to the next (and then either give a <br>
blanket R-b: to the series, or else reply to every single one of the <br>
tiny patches).<br>
<br>
<br>
(The downside to including too many pieces of too many files in one <br>
patch is that if some other unrelated future patch needs to be <br>
backported to a distro's stable maintenance branch of libvirt, and that <br>
patch created a merge conflict if the patch you made hadn't also been <br>
backported to the branch, then the maintainer would either need to <br>
backport one large patch rather than one small patch. In the case of <br>
g_autofree conversion patches )and other similar simple changes), I'd <br>
say backporting the larger patch would create any extra problems, but <br>
then I don't backport a lot of patches to downstream branches :-)<br>
<br>
</blockquote></div>