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