[PATCH] util: convert char pointers to use g_autofree

[Barrett J Schonefeld]

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 at redhat.com> 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 at utexas.edu>
> >>
> >> 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 :-)
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20201120/4744355e/attachment-0001.htm>