Message ID | 20200106182425.20312-1-danielhb413@gmail.com |
---|---|
Headers | show |
Series | trivial unneeded labels cleanup | expand |
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 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 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
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 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.
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 >
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.