19.09.2019 18:50, Daniel P. Berrangé wrote: > On Thu, Sep 19, 2019 at 10:24:20AM -0500, Eric Blake wrote: >> On 9/19/19 9:49 AM, Daniel P. Berrangé wrote: >> >>>> ALWAYS using MAKE_ERRP_SAFE() on entry to any function that has an Error >>>> **errp parameter is dirt-simple to explain. It has no performance >>>> penalty if the user passed in a normal error or error_abort (the cost of >>>> an 'if' hidden in the macro is probably negligible compared to >>>> everything else we do), and has no semantic penalty if the user passed >>>> in NULL or error_fatal (we now get the behavior we want with less >>>> boilerplate). >>>> >>>> Having to think 'does this method require me to use MAKE_ERRP_SAFE, or >>>> can I omit it?' does not provide the same simplicity. >>> >>> The flipside is that MAKE_ERRP_SAFE hides a bunch of logic, so you don't >>> really know what its doing without looking at it, and this is QEMU >>> custom concept so one more thing to learn for new contributors. >>> >>> While I think it is a nice trick, personally I think we would be better >>> off if we simply used a code pattern which does not require de-referencing >>> 'errp' at all, aside from exceptional cases. IOW, no added macro in 95% >>> of all our methods using Error **errp. >> >> If 100% of our callsites use the macro, then new contributors will >> quickly learn by observation alone that the macro usage must be >> important on any new function taking Error **errp, whether or not they >> actually read the macro to see what it does. If only 5% of our >> callsites use the macro, it's harder to argue that a new user will pick >> up on the nuances by observation alone (presumably, our docs would also >> spell it out, but we know that not everyone reads those...). > > To get some slightly less made-up stats, I did some searching: > > - 4408 methods with an 'errp' parameter declared > > git grep 'Error \*\*errp'| wc -l > > - 696 methods with an 'Error *local' declared (what other names > do we use for this?) > > git grep 'Error \*local' | wc -l > > - 1243 methods with an 'errp' parameter which have void > return value (fuzzy number - my matching is quite crude) > > git grep 'Error \*\*errp'| grep -E '(^| )void' | wc -l > > - 11 methods using error_append_hint with a local_err > > git grep append_hint | grep local | wc -l
why do you count only with local? Greg's series is here to bring local to all functions with append_hint: # git grep append_hint | wc -l 85 (still, some functions may append_hint several times, so the number should be less) and file list is big: # git grep -l append_hint block.c block/backup.c block/dirty-bitmap.c block/file-posix.c block/gluster.c block/qcow.c block/qcow2.c block/vhdx-log.c block/vpc.c chardev/spice.c hw/9pfs/9p-local.c hw/9pfs/9p-proxy.c hw/arm/msf2-soc.c hw/arm/virt.c hw/audio/intel-hda.c hw/intc/arm_gicv3_kvm.c hw/misc/msf2-sysreg.c hw/pci-bridge/pci_bridge_dev.c hw/pci-bridge/pcie_root_port.c hw/ppc/mac_newworld.c hw/ppc/spapr.c hw/ppc/spapr_irq.c hw/ppc/spapr_pci.c hw/rdma/vmw/pvrdma_main.c hw/s390x/s390-ccw.c hw/scsi/megasas.c hw/scsi/mptsas.c hw/scsi/scsi-disk.c hw/scsi/scsi-generic.c hw/usb/ccid-card-emulated.c hw/usb/hcd-xhci.c hw/vfio/common.c hw/vfio/pci-quirks.c hw/vfio/pci.c hw/virtio/virtio-pci.c hw/xen/xen_pt.c hw/xen/xen_pt_config_init.c include/qapi/error.h migration/migration.c nbd/client.c nbd/server.c qdev-monitor.c target/arm/cpu64.c target/ppc/kvm.c util/error.c util/qemu-option.c util/qemu-sockets.c Also, conversion to use macro everywhere may be done (seems so) by coccinelle script. But conversion which you mean, only by hand I think. Converting 1243 methods by hand is a huge task.. I think there are three consistent ways: 1. Use macro everywhere 2. Drop error_append_hint 3. Drop error_fatal > > > This suggests to me, that if we used the 'return 0 / return -1' convention > to indicate failure for the methods which currently return void, we could > potentially only have 11 cases where a local error object is genuinely > needed. > > If my numbers are at all accurate, I still believe we're better off > changing the return type and eliminating essentially all use of local > error variables, as void funcs/local error objects appear to be the > minority coding pattern. > > >>> There are lots of neat things we could do with auto-cleanup functions we >>> I think we need to be wary of hiding too much cleverness behind macros >>> when doing so overall. >> >> The benefit of getting rid of the 'Error *local_err = NULL; ... >> error_propagate()' boilerplate is worth the cleverness, in my opinion, >> but especially if also accompanied by CI coverage that we abide by our >> new rules. > > At least we're both wanting to eliminate the local error + propagation. > The difference is whether we're genuinely eliminating it, or just hiding > it from view via auto-cleanup & macro magic > > Regards, > Daniel > -- Best regards, Vladimir
