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 <[email protected]> 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 <[email protected]> > >> > >> 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 :-) > >
