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

[Cole Robinson]

On 12/20/19 3:54 PM, Michal Prívozník wrote:
> On 12/20/19 7: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.
>>
>> The first method is more tedious, but it's easier for reviewers, and
>> good patches can go in first while patches with issues can be kicked out
>> for v2. But it could be thousands of patches judging by the 3000+
>> cleanup labels we have in the code base, which sounds extreme.
>>
>> I think in general you will find the list more receptive to series of
>> small per function patches though. Optimizing for ease of review will
>> always give things a better chance of getting committed in my
>> experience. This is just recommendation for future series though, I'll
>> review this one as is
> 
> I think the sooner we get this over with the better (i.e. less rebase
> conflicts). It's like tearing off a patch (bandage?) - it hurts less if
> you do it all at once.
> 

Yes I agree. Dropping the ALLOC < 0 bits shouldn't be done
incrementally. It should be a largely mechanical change, everything
under the 'if' is already dead code.

- Cole