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(-)

-- 
2.23.0


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

> 
> 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 honestly don't have an option about which method is better. I agree
that splitting one function per patch is easier for review. I don't
mind the effort into splitting this work either.

So, what I'll do then is to apply your reviews in this same format, then
I'll experiment with how much individual patches this would generate.
If it's too extreme (I have no idea how much functions I touched overall
in this file) then I'll resubmit in the old format. If it's a feasible
number I'll send it one per function. Perhaps a hybrid - similar functions
that got changed in the same way can go in the same patch. I'll experiment.

There are more cleanups to be made in the future. As soon as we can get down
to a feasible format for them, better.


Thanks,


DHB


> 
> - Cole
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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.

Having said that, it still makes sense to honour our rules and have one
semantic chnage per commit.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list