[PATCH] util: convert char pointers to use g_autofree

Laine Stump laine at redhat.com
Fri Nov 20 20:54:41 UTC 2020


On 11/20/20 11:43 AM, Barrett J Schonefeld wrote:
> 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.

One suggestion on how to more easily split one patch into multiple 
patches (keeping in mind there's probably a much cleaner way of doing 
the same thing; this is just how I've evolved to do it):

1) make a new branch "X-v2" based off the branch "X" that has this 
current patch

2) "git reset HEAD^" on the new branch - this will remove the last 
commit from git, but leave the working copies of the file unchanged.

3) use a tool like "git meld" to interactively go through all the 
changes (hunks) you've made in this single commit, *un*doing the ones 
that aren't related to basic g_autofree conversion.

4) git add / git commit -m"convert to g_autofree"

5) "git meld X" to compare the original commit on the *old* branch to 
the tip of the new branch, interactively re-applying all the hunks that 
you had just removed.

5) git add / git commit -m"remove unnecessary cleanup labels and return 
variables"

> 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.

A couple of qualifiers:

a) changing the return values to constants will of course happen as a 
part of the "cleanup label removal" patch (item (3)), not on its own.

b) a recent patch from jtomko reminded me of two occasions when you 
*would* want a separate patch for the g_autofree changed in a single 
function all by itself:

   ii) if that change fixes a bug (which would usually be a memory leak),

and/or

   iii) if it requires any change in the logic of the function beyond
        simply adding "g_autofree virBlahPtr blah = NULL;" and
        removing the VIR_FREE(blah); at the end of the scope.

Both of these would warrant extra explanation in the commit log, and 
that's easier to follow if it's isolated.





More information about the libvir-list mailing list