[libvirt] [PATCH v4 0/4] qemu_process: Glib sponsored cleanup

Cole Robinson crobinso at redhat.com
Fri Dec 20 19:42:09 UTC 2019


On 12/20/19 2:31 PM, Daniel Henrique Barboza wrote:
> 
> 
> On 12/20/19 3:57 PM, Cole Robinson wrote:
>> On 12/19/19 4:09 PM, Daniel Henrique Barboza wrote:
>>> This series got buried a few months ago. Let's go onward unto
>>> the 20s with no one left behind.
>>>
>>> changes from version 3 [1]:
>>> - rebased to master at commit 330b556829
>>> - removed former patch 4. The 'g_strdup_printf' change was
>>> made along the road
>>> - new patch 4: I am sending this one in separate to patch 3
>>> because this unneeded label was already in the code, being
>>> unrelated to the changes of this series. The maintainer is
>>> welcome to squash it into patch 3.
>>>
>>> [1]
>>> https://www.redhat.com/archives/libvir-list/2019-October/msg01080.html
>>>
>>> Daniel Henrique Barboza (4):
>>>    qemu_process.c: use g_autofree
>>>    qemu_process.c: use g_autoptr()
>>>    qemu_process.c: remove cleanup labels after g_auto*() changes
>>>    qemu_process.c: remove 'cleanup' label from
>>>      qemuProcessCreatePretendCmd()
>>>
>>>   src/qemu/qemu_process.c | 429 +++++++++++++++-------------------------
>>>   1 file changed, 157 insertions(+), 272 deletions(-)
>>>
>>
>> Patch 3 and 4 look fine, some comments on the first two.
>>
>> I can't really decide what is the best way to approach cleanups like
>> this. Should patches be split by function, and do all cleanups in one
>> pass, or do one type of cleanup, but over a larger surface per file?
>> Like you have done here.
> 
> This cleanup strategy was proposed by Jano [1] a few months ago in another
> cleanup [2]. This is what he proposed:
> 
> ----
> These patches would look much better split by:
> * individual functions (in case you do rework multiple things at once)
> * individual changes, i.e.
>  * g_autofree for scalars
>  * g_autoptr for pointers and unref
>  * possible removal of cleanup labels
> 
> Especially splitting the goto -> return change makes the patches
> much more easier to read, since it makes it obvious that you don't
> change the exit points of the function while adding the autofree
> attributes.
> ----
> 
> He followed up with:
> 
> ----
>> Do you mean sending an individual patch for any function that might
>> have, say, 2+ changes in it? For example, if the same function
>> was changed to use g_autoptr and g_autofree and perhaps
>> that causes a label to be dropped, this can be an individual patch?
> 
> Yes, but if you convert a lot of functions, that would result in a lot
> of patches.
> 
> Sending one patch per function is more viable for the cases where you
> need to refactor it in order to add some functionality later (see
> Jirka's series for example). For mass conversion for the sake of
> conversion, one patch per change is better.
> ----
> 
> These cleanups are cleanups for the sake of cleanups, thus I followed
> up with this model of one type of change across all the file per patch.
> 
> 
> [1] https://www.redhat.com/archives/libvir-list/2019-October/msg00985.html
> [2] https://www.redhat.com/archives/libvir-list/2019-October/msg00918.html
> 

Ah I didn't realize (or forgot). I'll defer to Jano on this one, he's
done far more reviews than me. So I suggest sticking with his method

Thanks,
Cole




More information about the libvir-list mailing list