A recent post unveiled a potential problem when passing errp to error_append_hint():
https://patchwork.ozlabs.org/patch/1162171/ The issue is that error_append_hint() may end up not being called at all if errp is equal to &error_fatal or &error_abort. It turns out that the only way to ensure error_append_hint() can do its job regardless of how the error is handled is to use a local error object and to propagate it to the caller. As suggested by Markus Armbruster, this series does: (1) update error_append_hint()'s documentation in <qapi/error.h> to spell this out (2) fix other misuses of error_append_hint() in the tree The following command was used to detect candidates for (2), which were then manually inspected. It is possible that many of them never exhibit the unwanted behaviour in practice because errp happens to never be equal to &error_fatal or &error_abort. It isn't trivial to assess though, and this is fragile and can be easily broken if the code gets rearranged. Also, passing errp directly is a shortcut that doesn't bring much, neither for clarity, nor for performance that we don't really care for on an error path). Better safe than sorry, so fix them all. I tried to group the changes that are supposed to go through the same tree together. $ git grep -n error_append_hint\(errp | cut -d: -f 1-2 block/backup.c:608 block/dirty-bitmap.c:256 block/file-posix.c:393 block/file-posix.c:854 block/file-posix.c:2282 block/gluster.c:480 block/gluster.c:483 block/gluster.c:489 block/gluster.c:702 block/gluster.c:711 block/qcow.c:162 block/qcow.c:195 block/qcow2.c:1363 block/vhdx-log.c:811 block/vpc.c:1033 => Patch for block chardev/spice.c:284 => Patch for chardev hw/9pfs/9p-local.c:1474 hw/9pfs/9p-proxy.c:1119 Already posted https://patchwork.ozlabs.org/patch/1162171/ hw/arm/msf2-soc.c:131 hw/arm/virt.c:1808 hw/arm/virt.c:1836 hw/intc/arm_gicv3_kvm.c:815 hw/misc/msf2-sysreg.c:135 => Patch for arm hw/pci-bridge/pcie_root_port.c:76 hw/pci-bridge/pcie_root_port.c:90 => Patch for pci hw/ppc/mac_newworld.c:622 hw/ppc/spapr.c:4341 hw/ppc/spapr_pci.c:1874 hw/ppc/spapr_pci.c:1876 hw/ppc/spapr_pci.c:1878 => Patch for ppc hw/rdma/vmw/pvrdma_main.c:670 => Patch for pvrdma hw/s390x/s390-ccw.c:63 => Patch for s390 hw/scsi/scsi-disk.c:2624 hw/scsi/scsi-generic.c:679 => Patch for scsi hw/usb/ccid-card-emulated.c:516 => Patch for usb hw/vfio/common.c:1473 hw/vfio/common.c:1536 hw/vfio/pci.c:2532 hw/vfio/pci.c:2718 => Patch for vfio hw/virtio/virtio-pci.c:1548 hw/virtio/virtio-pci.c:1742 => Patch for virtio migration/migration.c:988 migration/migration.c:997 => Patch for migration nbd/client.c:230 nbd/server.c:1627 => Patch for nbd qdev-monitor.c:334 qdev-monitor.c:337 qdev-monitor.c:340 qdev-monitor.c:348 qdev-monitor.c:351 qdev-monitor.c:354 qdev-monitor.c:358 No misuse here as these aren't preceded by a call to error_setg() or error_propagate() that could terminate QEMU. target/ppc/kvm.c:246 target/ppc/kvm.c:2089 target/ppc/kvm.c:2092 => Patch for kvm util/qemu-option.c:160 util/qemu-option.c:669 => Patch for option util/qemu-sockets.c:886 util/qemu-sockets.c:956 => Patch for sockets As a bonus, also teach checkpatch to detect that. Since the convention of using the errp naming seems to be strictly followed, the check is just to detect "error_append_hint(errp" and emit a warning, for the sake of simplicity. -- Greg --- Greg Kurz (17): error: Update error_append_hint()'s documentation block: Pass local error object pointer to error_append_hint() char/spice: Pass local error object pointer to error_append_hint() ppc: Pass local error object pointer to error_append_hint() arm: Pass local error object pointer to error_append_hint() vfio: Pass local error object pointer to error_append_hint() virtio-pci: Pass local error object pointer to error_append_hint() pcie_root_port: Pass local error object pointer to error_append_hint() hw/rdma: Fix missing conversion to rdma_error_report() s390x/css: Pass local error object pointer to error_append_hint() scsi: Pass local error object pointer to error_append_hint() migration: Pass local error object pointer to error_append_hint() nbd: Pass local error object pointer to error_append_hint() ccid-card-emul: Pass local error object pointer to error_append_hint() option: Pass local error object pointer to error_append_hint() socket: Pass local error object pointer to error_append_hint() checkpatch: Warn when errp is passed to error_append_hint() block/backup.c | 7 +++++-- block/dirty-bitmap.c | 7 +++++-- block/file-posix.c | 20 +++++++++++++------- block/gluster.c | 23 +++++++++++++++-------- block/qcow.c | 10 ++++++---- block/qcow2.c | 7 +++++-- block/vhdx-log.c | 7 +++++-- block/vpc.c | 7 +++++-- chardev/spice.c | 6 ++++-- hw/arm/msf2-soc.c | 5 +++-- hw/arm/virt.c | 14 ++++++++++---- hw/intc/arm_gicv3_kvm.c | 5 +++-- hw/misc/msf2-sysreg.c | 6 ++++-- hw/pci-bridge/pcie_root_port.c | 11 +++++++---- hw/ppc/mac_newworld.c | 7 +++++-- hw/ppc/spapr.c | 7 +++++-- hw/ppc/spapr_pci.c | 9 +++++---- hw/rdma/vmw/pvrdma_main.c | 2 +- hw/s390x/s390-ccw.c | 6 ++++-- hw/scsi/scsi-disk.c | 7 +++++-- hw/scsi/scsi-generic.c | 7 +++++-- hw/usb/ccid-card-emulated.c | 7 +++++-- hw/vfio/common.c | 14 ++++++++++---- hw/vfio/pci.c | 12 ++++++++---- hw/virtio/virtio-pci.c | 14 ++++++++++---- include/qapi/error.h | 11 ++++++++++- migration/migration.c | 14 ++++++++++---- nbd/client.c | 24 +++++++++++++----------- nbd/server.c | 7 +++++-- scripts/checkpatch.pl | 4 ++++ target/ppc/kvm.c | 13 +++++++++---- util/qemu-option.c | 14 ++++++++++---- util/qemu-sockets.c | 14 ++++++++++---- 33 files changed, 224 insertions(+), 104 deletions(-)
