Hello,

This is the type of cleanup I've contributed to Libvirt
during the last year. Figured QEMU also deserves the same
care.

The idea here is remove unneeded labels. By 'unneeded' I
mean labels that does nothing but a 'return' call. One
common case is something like this:

if ()
    goto cleanup;
[...]
 cleanup:
    return 0;

This code can be simplified to:

if ()
    return 0;


Which is cleaner and requires less brain cycles to wonder
whether the 'cleanup' label does anything special, such
as a heap memory cleanup.

Another common case uses a variable to set a return value,
generally an error, then return:

if () {
    ret = -ENOENT;
    goto out;
}
[..]
 out:
    return ret;

Likewise, it is clearer to just 'return -ENOENT' instead of
jumping to a label. There are other cases being handled in
these patches, but these are the most common.

There are still a handful of unneeded labels hanging around after
this series. There are cases in which the label name is
part of the code semantics and I judged not worth removing.
search_chunk() in block/dmg.c has an example of an unneeded
'err' label that I decided to not remove.

No functional change was made. If any of these patches changes
existing behavior it is unintended and an error from my
part.



Daniel Henrique Barboza (59):
  spapr.c: remove 'out' label in spapr_dt_cas_updates()
  ppc440_bamboo.c: remove label from bamboo_load_device_tree()
  kvm-all.c: remove unneeded labels
  paaudio.c: remove unneeded labels
  ram.c: remove unneeded labels
  mips-semi.c: remove 'uhi_done' label in helper_do_semihosting()
  unicore32/softmmu.c: remove 'do_fault' label in get_phys_addr_ucv2()
  chardev/char-mux.c: remove 'send_char' label
  chardev/char-pipe.c: remove 'fail' label in win_chr_pipe_init()
  chardev/char-win.c: remove 'fail' label in win_chr_serial_init()
  exec.c: remove 'err' label in ram_block_discard_range()
  virtfs-proxy-helper.c: remove 'err_out' label in setugid()
  block/vdi.c: remove 'fail' label in vdi_open()
  block/file-posix.c: remove unneeded labels
  block/blkreplay.c: remove unneeded 'fail' label in blkreplay_open()
  block/gluster.c: remove unneeded 'exit' label
  block/dmg.c: remove unneeded 'fail' label in dmg_read_mish_block()
  qcow2-refcount.c: remove unneeded 'fail' label in
    qcow2_refcount_init()
  block/ssh.c: remove unneeded labels
  block/vpc.c: remove unneeded 'fail' label in create_dynamic_disk()
  block/backup.c remove unneeded 'out' label in backup_run()
  block/vmdk.c: remove unneeded labels
  block/vxhs.c: remove unneeded 'out' label in vxhs_iio_callback()
  block/vhdx-log.c: remove unneeded labels
  block/vhdx.c: remove unneeded 'exit' labels
  block/replication.c: remove unneeded label in replication_co_writev
  crypto/block-luks.c: remove unneeded label in
    qcrypto_block_luks_find_key
  qga/commands-win32.c: remove 'out' label in get_pci_info
  cryptodev-vhost.c: remove unneeded 'err' label in
    cryptodev_vhost_start
  util/module.c: remove unneeded label in module_load_file()
  util/aio-posix.c: remove unneeded 'out' label in aio_epoll
  qemu-img.c: remove 'out4' label in img_compare
  ipmi/ipmi_bmc_sim.c: remove unneeded labels
  ipmi/ipmi_bt.c: remove unneeded label in ipmi_bt_handle_event
  ipmi_bmc_extern.c: remove unneeded label in
    ipmi_bmc_extern_handle_command
  ipmi/ipmi_kcs.c: remove unneeded label in ipmi_kcs_handle_event
  s390x/event-facility.c: remove unneeded labels
  s390x/sclp.c: remove unneeded label in sclp_service_call()
  usb/dev-mtp.c: remove unneeded label in write_retry()
  hsb/hcd-ehci.c: remove unneeded labels
  intc/s390_flic_kvm.c: remove unneeded label in kvm_flic_load()
  i386/intel_iommu.c: remove unneeded labels
  i386/amd_iommu.c: remove unneeded label in amdvi_int_remap_msi()
  9p-local.c: remove unneeded label in local_unlinkat_common()
  9pfs/9p.c: remove unneeded labels
  alpha/typhoon.c: remove unneeded label in typhoon_translate_iommu()
  pvrdma_main.c: remove unneeded labels
  pvrdma_dev_ring.c: remove unneeded label in pvrdma_ring_init()
  rdma/rdma_rm.c: remove unneeded label in rdma_rm_alloc_pd()
  rdma/rdma_backend.c: remove unneeded label in rdma_backend_init()
  virtio/vhost.c: remove unneeded labels
  net/vhost_net.c: remove unneeded labels
  net/net_tx_pkt.c: remove unneeded label in net_tx_pkt_get_gso_type()
  ivshmem-server/main.c: remove unneeded label in main()
  linux-user/flatload.c: remove unused 'out' label
  linux-user/signal.c: remove unneeded label in do_sigaltstack()
  linux-user/syscall.c: fix trailing whitespaces and style
  linux-user/syscall.c: remove unneeded labels
  linux-user/vm86.c: remove unneeded label in do_vm86()

 accel/kvm/kvm-all.c           | 30 +++++-------
 audio/paaudio.c               | 10 +---
 backends/cryptodev-vhost.c    |  4 +-
 block/backup.c                |  6 +--
 block/blkreplay.c             |  8 +---
 block/dmg.c                   | 10 +---
 block/file-posix.c            | 10 ++--
 block/gluster.c               |  3 +-
 block/qcow2-refcount.c        |  7 +--
 block/replication.c           |  9 ++--
 block/ssh.c                   | 61 ++++++++-----------------
 block/vdi.c                   | 40 ++++++----------
 block/vhdx-log.c              | 86 +++++++++++++----------------------
 block/vhdx.c                  | 10 ++--
 block/vmdk.c                  | 37 ++++++---------
 block/vpc.c                   | 12 ++---
 block/vxhs.c                  |  4 +-
 chardev/char-mux.c            |  3 +-
 chardev/char-pipe.c           | 13 ++----
 chardev/char-win.c            | 19 ++++----
 contrib/ivshmem-server/main.c |  9 ++--
 crypto/block-luks.c           |  3 +-
 exec.c                        | 13 +++---
 fsdev/virtfs-proxy-helper.c   |  4 +-
 hw/9pfs/9p-local.c            | 12 ++---
 hw/9pfs/9p.c                  |  9 ++--
 hw/alpha/typhoon.c            | 18 ++++----
 hw/i386/amd_iommu.c           | 13 ++----
 hw/i386/intel_iommu.c         |  8 ++--
 hw/intc/s390_flic_kvm.c       | 10 ++--
 hw/ipmi/ipmi_bmc_extern.c     |  5 +-
 hw/ipmi/ipmi_bmc_sim.c        |  9 +---
 hw/ipmi/ipmi_bt.c             |  8 ++--
 hw/ipmi/ipmi_kcs.c            |  4 +-
 hw/net/net_tx_pkt.c           | 11 ++---
 hw/net/vhost_net.c            |  7 ++-
 hw/ppc/ppc440_bamboo.c        |  8 +---
 hw/ppc/spapr.c                |  9 ++--
 hw/rdma/rdma_backend.c        |  4 +-
 hw/rdma/rdma_rm.c             | 11 ++---
 hw/rdma/vmw/pvrdma_dev_ring.c |  7 +--
 hw/rdma/vmw/pvrdma_main.c     | 10 ++--
 hw/s390x/event-facility.c     | 21 +++------
 hw/s390x/sclp.c               | 16 ++-----
 hw/usb/dev-mtp.c              | 13 ++----
 hw/usb/hcd-ehci.c             | 32 ++++---------
 hw/virtio/vhost.c             | 11 ++---
 linux-user/flatload.c         |  1 -
 linux-user/signal.c           | 20 +++-----
 linux-user/syscall.c          | 54 ++++++++++------------
 linux-user/vm86.c             |  7 +--
 migration/ram.c               | 18 ++------
 qemu-img.c                    |  7 +--
 qga/commands-win32.c          | 17 ++++---
 target/mips/mips-semi.c       | 15 +++---
 target/unicore32/softmmu.c    | 23 +++-------
 util/aio-posix.c              |  3 +-
 util/module.c                 | 11 ++---
 58 files changed, 293 insertions(+), 550 deletions(-)

-- 
2.24.1

On Mon, Jan 06, 2020 at 03:23:26PM -0300, Daniel Henrique Barboza wrote:
> Hello,
> 
> This is the type of cleanup I've contributed to Libvirt
> during the last year. Figured QEMU also deserves the same
> care.
> 
> The idea here is remove unneeded labels. By 'unneeded' I
> mean labels that does nothing but a 'return' call. One
> common case is something like this:
> 
> if ()
>     goto cleanup;
> [...]
>  cleanup:
>     return 0;
> 
> This code can be simplified to:
> 
> if ()
>     return 0;
> 
> 
> Which is cleaner and requires less brain cycles to wonder
> whether the 'cleanup' label does anything special, such
> as a heap memory cleanup.

I would disagree with this analysis.  To me, I often wonder
when I have to add cleanup code to a routine whether there is
some hidden return in the middle of the function.  That's a lot
harder to spot than just looking for the cleanup label at the
end of the function to see what it does.  For non-trivial
functions I prefer to have one point of return at the end
(and maybe some minor checks with returns right at the beginning).
I'm not adamant about this, just my opinion.

But if we are going to fix things like this, it might be best to add
them to the coding style document and checkpatch script.  Otherwise
these sorts of things will just continue.

-corey

> 
> Another common case uses a variable to set a return value,
> generally an error, then return:
> 
> if () {
>     ret = -ENOENT;
>     goto out;
> }
> [..]
>  out:
>     return ret;
> 
> Likewise, it is clearer to just 'return -ENOENT' instead of
> jumping to a label. There are other cases being handled in
> these patches, but these are the most common.
> 
> There are still a handful of unneeded labels hanging around after
> this series. There are cases in which the label name is
> part of the code semantics and I judged not worth removing.
> search_chunk() in block/dmg.c has an example of an unneeded
> 'err' label that I decided to not remove.
> 
> No functional change was made. If any of these patches changes
> existing behavior it is unintended and an error from my
> part.
> 
> 
> 
> Daniel Henrique Barboza (59):
>   spapr.c: remove 'out' label in spapr_dt_cas_updates()
>   ppc440_bamboo.c: remove label from bamboo_load_device_tree()
>   kvm-all.c: remove unneeded labels
>   paaudio.c: remove unneeded labels
>   ram.c: remove unneeded labels
>   mips-semi.c: remove 'uhi_done' label in helper_do_semihosting()
>   unicore32/softmmu.c: remove 'do_fault' label in get_phys_addr_ucv2()
>   chardev/char-mux.c: remove 'send_char' label
>   chardev/char-pipe.c: remove 'fail' label in win_chr_pipe_init()
>   chardev/char-win.c: remove 'fail' label in win_chr_serial_init()
>   exec.c: remove 'err' label in ram_block_discard_range()
>   virtfs-proxy-helper.c: remove 'err_out' label in setugid()
>   block/vdi.c: remove 'fail' label in vdi_open()
>   block/file-posix.c: remove unneeded labels
>   block/blkreplay.c: remove unneeded 'fail' label in blkreplay_open()
>   block/gluster.c: remove unneeded 'exit' label
>   block/dmg.c: remove unneeded 'fail' label in dmg_read_mish_block()
>   qcow2-refcount.c: remove unneeded 'fail' label in
>     qcow2_refcount_init()
>   block/ssh.c: remove unneeded labels
>   block/vpc.c: remove unneeded 'fail' label in create_dynamic_disk()
>   block/backup.c remove unneeded 'out' label in backup_run()
>   block/vmdk.c: remove unneeded labels
>   block/vxhs.c: remove unneeded 'out' label in vxhs_iio_callback()
>   block/vhdx-log.c: remove unneeded labels
>   block/vhdx.c: remove unneeded 'exit' labels
>   block/replication.c: remove unneeded label in replication_co_writev
>   crypto/block-luks.c: remove unneeded label in
>     qcrypto_block_luks_find_key
>   qga/commands-win32.c: remove 'out' label in get_pci_info
>   cryptodev-vhost.c: remove unneeded 'err' label in
>     cryptodev_vhost_start
>   util/module.c: remove unneeded label in module_load_file()
>   util/aio-posix.c: remove unneeded 'out' label in aio_epoll
>   qemu-img.c: remove 'out4' label in img_compare
>   ipmi/ipmi_bmc_sim.c: remove unneeded labels
>   ipmi/ipmi_bt.c: remove unneeded label in ipmi_bt_handle_event
>   ipmi_bmc_extern.c: remove unneeded label in
>     ipmi_bmc_extern_handle_command
>   ipmi/ipmi_kcs.c: remove unneeded label in ipmi_kcs_handle_event
>   s390x/event-facility.c: remove unneeded labels
>   s390x/sclp.c: remove unneeded label in sclp_service_call()
>   usb/dev-mtp.c: remove unneeded label in write_retry()
>   hsb/hcd-ehci.c: remove unneeded labels
>   intc/s390_flic_kvm.c: remove unneeded label in kvm_flic_load()
>   i386/intel_iommu.c: remove unneeded labels
>   i386/amd_iommu.c: remove unneeded label in amdvi_int_remap_msi()
>   9p-local.c: remove unneeded label in local_unlinkat_common()
>   9pfs/9p.c: remove unneeded labels
>   alpha/typhoon.c: remove unneeded label in typhoon_translate_iommu()
>   pvrdma_main.c: remove unneeded labels
>   pvrdma_dev_ring.c: remove unneeded label in pvrdma_ring_init()
>   rdma/rdma_rm.c: remove unneeded label in rdma_rm_alloc_pd()
>   rdma/rdma_backend.c: remove unneeded label in rdma_backend_init()
>   virtio/vhost.c: remove unneeded labels
>   net/vhost_net.c: remove unneeded labels
>   net/net_tx_pkt.c: remove unneeded label in net_tx_pkt_get_gso_type()
>   ivshmem-server/main.c: remove unneeded label in main()
>   linux-user/flatload.c: remove unused 'out' label
>   linux-user/signal.c: remove unneeded label in do_sigaltstack()
>   linux-user/syscall.c: fix trailing whitespaces and style
>   linux-user/syscall.c: remove unneeded labels
>   linux-user/vm86.c: remove unneeded label in do_vm86()
> 
>  accel/kvm/kvm-all.c           | 30 +++++-------
>  audio/paaudio.c               | 10 +---
>  backends/cryptodev-vhost.c    |  4 +-
>  block/backup.c                |  6 +--
>  block/blkreplay.c             |  8 +---
>  block/dmg.c                   | 10 +---
>  block/file-posix.c            | 10 ++--
>  block/gluster.c               |  3 +-
>  block/qcow2-refcount.c        |  7 +--
>  block/replication.c           |  9 ++--
>  block/ssh.c                   | 61 ++++++++-----------------
>  block/vdi.c                   | 40 ++++++----------
>  block/vhdx-log.c              | 86 +++++++++++++----------------------
>  block/vhdx.c                  | 10 ++--
>  block/vmdk.c                  | 37 ++++++---------
>  block/vpc.c                   | 12 ++---
>  block/vxhs.c                  |  4 +-
>  chardev/char-mux.c            |  3 +-
>  chardev/char-pipe.c           | 13 ++----
>  chardev/char-win.c            | 19 ++++----
>  contrib/ivshmem-server/main.c |  9 ++--
>  crypto/block-luks.c           |  3 +-
>  exec.c                        | 13 +++---
>  fsdev/virtfs-proxy-helper.c   |  4 +-
>  hw/9pfs/9p-local.c            | 12 ++---
>  hw/9pfs/9p.c                  |  9 ++--
>  hw/alpha/typhoon.c            | 18 ++++----
>  hw/i386/amd_iommu.c           | 13 ++----
>  hw/i386/intel_iommu.c         |  8 ++--
>  hw/intc/s390_flic_kvm.c       | 10 ++--
>  hw/ipmi/ipmi_bmc_extern.c     |  5 +-
>  hw/ipmi/ipmi_bmc_sim.c        |  9 +---
>  hw/ipmi/ipmi_bt.c             |  8 ++--
>  hw/ipmi/ipmi_kcs.c            |  4 +-
>  hw/net/net_tx_pkt.c           | 11 ++---
>  hw/net/vhost_net.c            |  7 ++-
>  hw/ppc/ppc440_bamboo.c        |  8 +---
>  hw/ppc/spapr.c                |  9 ++--
>  hw/rdma/rdma_backend.c        |  4 +-
>  hw/rdma/rdma_rm.c             | 11 ++---
>  hw/rdma/vmw/pvrdma_dev_ring.c |  7 +--
>  hw/rdma/vmw/pvrdma_main.c     | 10 ++--
>  hw/s390x/event-facility.c     | 21 +++------
>  hw/s390x/sclp.c               | 16 ++-----
>  hw/usb/dev-mtp.c              | 13 ++----
>  hw/usb/hcd-ehci.c             | 32 ++++---------
>  hw/virtio/vhost.c             | 11 ++---
>  linux-user/flatload.c         |  1 -
>  linux-user/signal.c           | 20 +++-----
>  linux-user/syscall.c          | 54 ++++++++++------------
>  linux-user/vm86.c             |  7 +--
>  migration/ram.c               | 18 ++------
>  qemu-img.c                    |  7 +--
>  qga/commands-win32.c          | 17 ++++---
>  target/mips/mips-semi.c       | 15 +++---
>  target/unicore32/softmmu.c    | 23 +++-------
>  util/aio-posix.c              |  3 +-
>  util/module.c                 | 11 ++---
>  58 files changed, 293 insertions(+), 550 deletions(-)
> 
> -- 
> 2.24.1
> 
> 

On 1/6/20 4:54 PM, Corey Minyard wrote:
> On Mon, Jan 06, 2020 at 03:23:26PM -0300, Daniel Henrique Barboza wrote:
>> Hello,
[...]
>>
>> Which is cleaner and requires less brain cycles to wonder
>> whether the 'cleanup' label does anything special, such
>> as a heap memory cleanup.
> 
> I would disagree with this analysis.  To me, I often wonder
> when I have to add cleanup code to a routine whether there is
> some hidden return in the middle of the function.  That's a lot
> harder to spot than just looking for the cleanup label at the
> end of the function to see what it does.  For non-trivial
> functions I prefer to have one point of return at the end
> (and maybe some minor checks with returns right at the beginning).
> I'm not adamant about this, just my opinion.

I agree that what I'm doing here isn't a one rule fits all situation. This
is why I didn't purge all the 'unused' labels I found. The criteria used to
remove/spare labels will differ from person to person (although I believe that
cases such as patch 15 isn't too much of a debate), thus I'd rather leave to
each maintainer to accept/deny the changes based on the context of the code.

And for this same reason I don't think that a checkpatch rule is necessary.
The review process can take care of these situations and allow 'good unneeded
labels' to be in the code.


Thanks,


DHB



> 
> But if we are going to fix things like this, it might be best to add
> them to the coding style document and checkpatch script.  Otherwise
> these sorts of things will just continue.
> 
> -corey
> 

Am 06.01.2020 um 21:35 hat Daniel Henrique Barboza geschrieben:
> 
> 
> On 1/6/20 4:54 PM, Corey Minyard wrote:
> > On Mon, Jan 06, 2020 at 03:23:26PM -0300, Daniel Henrique Barboza wrote:
> > > Hello,
> [...]
> > > 
> > > Which is cleaner and requires less brain cycles to wonder
> > > whether the 'cleanup' label does anything special, such
> > > as a heap memory cleanup.
> > 
> > I would disagree with this analysis.  To me, I often wonder
> > when I have to add cleanup code to a routine whether there is
> > some hidden return in the middle of the function.  That's a lot
> > harder to spot than just looking for the cleanup label at the
> > end of the function to see what it does.  For non-trivial
> > functions I prefer to have one point of return at the end
> > (and maybe some minor checks with returns right at the beginning).
> > I'm not adamant about this, just my opinion.

It depends on the case, but yes, I had similar thoughts, at least when
we're talking about non-trivial parts of a function. (Very short
functions of just some initial checks returning directly are usually
fine.)

> I agree that what I'm doing here isn't a one rule fits all situation. This
> is why I didn't purge all the 'unused' labels I found. The criteria used to
> remove/spare labels will differ from person to person (although I believe that
> cases such as patch 15 isn't too much of a debate), thus I'd rather leave to
> each maintainer to accept/deny the changes based on the context of the code.

So what is your plan for getting the series merged? Should maintainers
just picks patches from the series, or do you want to collect Acked-by
tags and then merge it through a single tree? If the latter, which one?

Kevin

On 1/7/20 7:16 AM, Kevin Wolf wrote:
> Am 06.01.2020 um 21:35 hat Daniel Henrique Barboza geschrieben:
>> On 1/6/20 4:54 PM, Corey Minyard wrote:
>>> On Mon, Jan 06, 2020 at 03:23:26PM -0300, Daniel Henrique Barboza wrote:
>>>> Hello,
>> [...]
>>>>
>>>> Which is cleaner and requires less brain cycles to wonder
>>>> whether the 'cleanup' label does anything special, such
>>>> as a heap memory cleanup.
>>>
>>> I would disagree with this analysis.  To me, I often wonder
>>> when I have to add cleanup code to a routine whether there is
>>> some hidden return in the middle of the function.  That's a lot
>>> harder to spot than just looking for the cleanup label at the
>>> end of the function to see what it does.  For non-trivial
>>> functions I prefer to have one point of return at the end
>>> (and maybe some minor checks with returns right at the beginning).
>>> I'm not adamant about this, just my opinion.
> 
> It depends on the case, but yes, I had similar thoughts, at least when
> we're talking about non-trivial parts of a function. (Very short
> functions of just some initial checks returning directly are usually
> fine.)

 From a debugging point of view, and when adding trace-events, it is 
easier to have a single function exit path.

In various functions modified by your patches, we can split big 
functions in smaller ones and avoid the goto label.

On 1/7/20 4:06 AM, Philippe Mathieu-Daudé wrote:
> On 1/7/20 7:16 AM, Kevin Wolf wrote:
>> Am 06.01.2020 um 21:35 hat Daniel Henrique Barboza geschrieben:
>>> On 1/6/20 4:54 PM, Corey Minyard wrote:
>>>> On Mon, Jan 06, 2020 at 03:23:26PM -0300, Daniel Henrique Barboza wrote:
>>>>> Hello,
>>> [...]
>>>>>
>>>>> Which is cleaner and requires less brain cycles to wonder
>>>>> whether the 'cleanup' label does anything special, such
>>>>> as a heap memory cleanup.
>>>>
>>>> I would disagree with this analysis.  To me, I often wonder
>>>> when I have to add cleanup code to a routine whether there is
>>>> some hidden return in the middle of the function.  That's a lot
>>>> harder to spot than just looking for the cleanup label at the
>>>> end of the function to see what it does.  For non-trivial
>>>> functions I prefer to have one point of return at the end
>>>> (and maybe some minor checks with returns right at the beginning).
>>>> I'm not adamant about this, just my opinion.
>>
>> It depends on the case, but yes, I had similar thoughts, at least when
>> we're talking about non-trivial parts of a function. (Very short
>> functions of just some initial checks returning directly are usually
>> fine.)
> 
>  From a debugging point of view, and when adding trace-events, it is easier to have a single function exit path.
> 
> In various functions modified by your patches, we can split big functions in smaller ones and avoid the goto label.
> 

Splitting and refactoring big funcitons wasn't the initial idea of this
work, but I see no problems in doing it. Care to point out where did you
see such cases?


Thanks,


DHB

On 1/7/20 3:16 AM, Kevin Wolf wrote:
> Am 06.01.2020 um 21:35 hat Daniel Henrique Barboza geschrieben:
>>

[...]

> 
> So what is your plan for getting the series merged? Should maintainers
> just picks patches from the series, or do you want to collect Acked-by
> tags and then merge it through a single tree? If the latter, which one?

The idea I had in mind was for each maintainer to pick up the patches and push
through their own trees, like David did in patches 1 and 2.

 From qemu-trivial archives I notice that Laurent creates qemu-trivial pull
requests often, thus I believe this might also be an option. If we decide
to go this route I'll ask David to remove patches 1 and 2 from his ppc
tree to avoid unnecessary conflicts.



Thanks,


DHB


> 
> Kevin
> 

Am 07.01.2020 um 12:52 hat Daniel Henrique Barboza geschrieben:
> > So what is your plan for getting the series merged? Should maintainers
> > just picks patches from the series, or do you want to collect Acked-by
> > tags and then merge it through a single tree? If the latter, which one?
> 
> The idea I had in mind was for each maintainer to pick up the patches and push
> through their own trees, like David did in patches 1 and 2.

Okay, thanks.

> From qemu-trivial archives I notice that Laurent creates qemu-trivial pull
> requests often, thus I believe this might also be an option. If we decide
> to go this route I'll ask David to remove patches 1 and 2 from his ppc
> tree to avoid unnecessary conflicts.

Two trees picking up the same patch doesn't result in a conflict, git
can resolve this just fine. It only gets problematic if different
versions of a patch are picked up.

Kevin

Corey Minyard <minyard@acm.org> writes:

> On Mon, Jan 06, 2020 at 03:23:26PM -0300, Daniel Henrique Barboza wrote:
>> Hello,
>> 
>> This is the type of cleanup I've contributed to Libvirt
>> during the last year. Figured QEMU also deserves the same
>> care.
>> 
>> The idea here is remove unneeded labels. By 'unneeded' I
>> mean labels that does nothing but a 'return' call. One
>> common case is something like this:
>> 
>> if ()
>>     goto cleanup;
>> [...]
>>  cleanup:
>>     return 0;
>> 
>> This code can be simplified to:
>> 
>> if ()
>>     return 0;
>> 
>> 
>> Which is cleaner and requires less brain cycles to wonder
>> whether the 'cleanup' label does anything special, such
>> as a heap memory cleanup.
>
> I would disagree with this analysis.  To me, I often wonder
> when I have to add cleanup code to a routine whether there is
> some hidden return in the middle of the function.  That's a lot
> harder to spot than just looking for the cleanup label at the
> end of the function to see what it does.

A cleanup label does not preclude existence of return.  You have to
check for return anyway.

>                                           For non-trivial
> functions I prefer to have one point of return at the end
> (and maybe some minor checks with returns right at the beginning).
> I'm not adamant about this, just my opinion.

I'm in Daniel's camp: if there's no need for cleanup, return without
ado.

> But if we are going to fix things like this, it might be best to add
> them to the coding style document and checkpatch script.  Otherwise
> these sorts of things will just continue.

Maybe.

On 06.01.20 19:23, Daniel Henrique Barboza wrote:
> Hello,
> 
> This is the type of cleanup I've contributed to Libvirt
> during the last year. Figured QEMU also deserves the same
> care.
> 
> The idea here is remove unneeded labels. By 'unneeded' I
> mean labels that does nothing but a 'return' call. One
> common case is something like this:
> 
> if ()
>     goto cleanup;
> [...]
>  cleanup:
>     return 0;
> 
> This code can be simplified to:
> 
> if ()
>     return 0;
> 
> 
> Which is cleaner and requires less brain cycles to wonder
> whether the 'cleanup' label does anything special, such
> as a heap memory cleanup.

For me, it doesn’t require any brain cycles, because I generally just
assume the cleanup label will do the right thing.  OTOH, a return
statement may make me invest some some brain cycles, because maybe there
is something to be cleaned up and it isn’t done here.

> Another common case uses a variable to set a return value,
> generally an error, then return:
> 
> if () {
>     ret = -ENOENT;
>     goto out;
> }
> [..]
>  out:
>     return ret;
> 
> Likewise, it is clearer to just 'return -ENOENT' instead of
> jumping to a label. There are other cases being handled in
> these patches, but these are the most common.

I find it clearer from the perspective of “less LoC”, but I find it less
clear from the perspective of “Is this the right way to clean up?”.

Even on patch 15 (which you say isn’t too much of a debate), I don’t
find the change to make things any clearer.  Just less verbose.

I suppose none of this would matter if we used __attribute__((cleanup))
everywhere and simply never had to clean up anything manually.  But as
long as we don’t and require cleanup paths in many places, I disagree
that they require more brain cycles than plain return statements.

Max

On 1/7/20 6:43 AM, Max Reitz wrote:
> On 06.01.20 19:23, Daniel Henrique Barboza wrote:

[...]

  
> For me, it doesn’t require any brain cycles, because I generally just
> assume the cleanup label will do the right thing.  OTOH, a return
> statement may make me invest some some brain cycles, because maybe there
> is something to be cleaned up and it isn’t done here.
> 
>> Another common case uses a variable to set a return value,
>> generally an error, then return:
>>
>> if () {
>>      ret = -ENOENT;
>>      goto out;
>> }
>> [..]
>>   out:
>>      return ret;
>>
>> Likewise, it is clearer to just 'return -ENOENT' instead of
>> jumping to a label. There are other cases being handled in
>> these patches, but these are the most common.
> 
> I find it clearer from the perspective of “less LoC”, but I find it less
> clear from the perspective of “Is this the right way to clean up?”.
> 
> Even on patch 15 (which you say isn’t too much of a debate), I don’t
> find the change to make things any clearer.  Just less verbose.

Fair enough. As I said in the cover, all this patches makes no functional
changes, just a clean up aiming for less LoC (and more clarity, at least
in my opinion).

I am aware all the good sides of keeping the code as is, such as being
easier to debug (although I would argue that an explicit trace_event
call is better than keeping verbose code 'just in case'), or goto usage
to keep just one return statement per function.

I am also aware that the existing QEMU code base has a mesh of styles.
What I'm proposing here isn't a 'my way is better' case by any means,
but it's not something unprecedented in the existing code base either.
Since there's no QEMU code guideline imposing that a function should
have only one 'return' statement regardless of how many 'goto' calls
are needed, or a guideline that discourages 'goto' calls regardless of
how many 'return' calls are needed, in the end it's a matter of seeing
what fits the function/code better. In the maintainers opinion, of
course.


Thanks,


DHB


> 
> I suppose none of this would matter if we used __attribute__((cleanup))
> everywhere and simply never had to clean up anything manually.  But as
> long as we don’t and require cleanup paths in many places, I disagree
> that they require more brain cycles than plain return statements.
> 
> Max
> 

On 1/6/20 12:23 PM, Daniel Henrique Barboza wrote:
> Hello,
> 
> This is the type of cleanup I've contributed to Libvirt
> during the last year. Figured QEMU also deserves the same
> care.
> 
> The idea here is remove unneeded labels. By 'unneeded' I
> mean labels that does nothing but a 'return' call. One
> common case is something like this:
> 
> if ()
>      goto cleanup;
> [...]
>   cleanup:
>      return 0;
> 
> This code can be simplified to:
> 
> if ()
>      return 0;
> 
> 

How much of this work is done manually, and how much via Coccinelle?


>   qga/commands-win32.c          | 17 ++++---
>   target/mips/mips-semi.c       | 15 +++---
>   target/unicore32/softmmu.c    | 23 +++-------
>   util/aio-posix.c              |  3 +-
>   util/module.c                 | 11 ++---

Hmm, no change to scripts/coccinelle, so presumably all manual :(

If we have a Coccinelle script that performs this transformation, we are 
more likely to be able to catch all places in the tree that would 
benefit (rather than relying on grep calls or other manual inspection), 
and more importantly, we can repeat the effort periodically to fix 
future additions that don't comply with the preferred style as well as 
backport the patch by rerunning the Coccinelle script with less worry of 
changed context.  Automated cleanups are always going to be easier to 
swallow (even if the diffstat is larger) than manual ad hoc cleanups.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 1/10/20 4:05 PM, Eric Blake wrote:
> On 1/6/20 12:23 PM, Daniel Henrique Barboza wrote:
>> Hello,
>>
>> This is the type of cleanup I've contributed to Libvirt
>> during the last year. Figured QEMU also deserves the same
>> care.
>>
>> The idea here is remove unneeded labels. By 'unneeded' I
>> mean labels that does nothing but a 'return' call. One
>> common case is something like this:
>>
>> if ()
>>      goto cleanup;
>> [...]
>>   cleanup:
>>      return 0;
>>
>> This code can be simplified to:
>>
>> if ()
>>      return 0;
>>
>>
> 
> How much of this work is done manually, and how much via Coccinelle?


I'm still getting along with Coccinelle. I'm able to do simple matches but
couldn't make it work to match this scenario. I didn't invest too much
time on it though.


> 
> 
>>   qga/commands-win32.c          | 17 ++++---
>>   target/mips/mips-semi.c       | 15 +++---
>>   target/unicore32/softmmu.c    | 23 +++-------
>>   util/aio-posix.c              |  3 +-
>>   util/module.c                 | 11 ++---
> 
> Hmm, no change to scripts/coccinelle, so presumably all manual :(

I used pcregrep:

$ pcregrep --exclude-dir=build --exclude-dir=docs --exclude-dir=scripts -r -n -M ':\n*\s*return' . > clean_labels

(note: there was a lot more of --exclude-dir in the original command, unfortunately I
didn't record it)

Then I filtered in the output file the cases where the regexp matched "switch"
labels and any other false positives like strings in comments. It's not manual
inspection, but it was crude indeed.



> 
> If we have a Coccinelle script that performs this transformation, we are more likely to be able to catch all places in the tree that would benefit (rather than relying on grep calls or other manual inspection), and more importantly, we can repeat the effort periodically to fix future additions that don't comply with the preferred style as well as backport the patch by rerunning the Coccinelle script with less worry of changed context.  Automated cleanups are always going to be easier to swallow (even if the diffstat is larger) than manual ad hoc cleanups.

I am not aware of how QEMU handles Coccinelle, if it is imposed via
./scripts/checkpatch.pl or something that it is ran from time to time to
suggest changes. But if it's desirable, I can see if I can cook a Coccinelle
script (or anything a bit more sophisticated than what I did)  to flag these
cases I attempted to handle with this series.


Thanks,


DHB


>