Re: [PATCH v1 00/59] trivial unneeded labels cleanup

[Corey Minyard <minyard@acm.org>]

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