Re: [qemu-s390x] [RFC] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

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