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

For some reason your email client escaped incorrectly Daniel's email:

  "Daniel P. Berrangé\" <berrange@redhat.com>

Which makes my email client very unhappy (Thunderbird):

There are non-ASCII characters in the local part of the recipient
address "Daniel P. Berrangé <berrange@redhat.com>,
qemu-block@nongnu.org, qemu-arm@nongnu.org, qemu-ppc@nongnu.org,
qemu-s390x@nongnu.org"@d06av23.portsmouth.uk.ibm.com. This is not yet
supported. Please change this address and try again.

Neither is MTA:

An error occurred while sending mail. The mail server responded:
5.1.2 The recipient address <qemu-s390x@nongnu.org@d06av22.portsmouth.uk.ibm
5.1.2 .com> is not a valid RFC-5321 address. r28sm2465872wrr.94 - gsmtp.
 Please check the message recipient
""qemu-s390x@nongnu.org"@d06av22.portsmouth.uk.ibm.com" and try again.

On Tue, 17 Sep 2019 13:00:37 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> For some reason your email client escaped incorrectly Daniel's email:
> 
>   "Daniel P. Berrangé\" <berrange@redhat.com>
> 

The client is stgit's "stg mail" command, and indeed it generates this:

 "Daniel P. =?utf-8?q?Berrang=C3=A9=22?= <berrange@redhat.com>
                                   ^^^
                                double-quote

and if I turn the 'é' into a 'e':

 "Daniel P. Berrange" <berrange@redhat.com>

This looks like a bug in "stg mail"...

> Which makes my email client very unhappy (Thunderbird):
> 
> There are non-ASCII characters in the local part of the recipient
> address "Daniel P. Berrangé <berrange@redhat.com>,
> qemu-block@nongnu.org, qemu-arm@nongnu.org, qemu-ppc@nongnu.org,
> qemu-s390x@nongnu.org"@d06av23.portsmouth.uk.ibm.com. This is not yet
> supported. Please change this address and try again.
> 
> Neither is MTA:
> 
> An error occurred while sending mail. The mail server responded:
> 5.1.2 The recipient address <qemu-s390x@nongnu.org@d06av22.portsmouth.uk.ibm
> 5.1.2 .com> is not a valid RFC-5321 address. r28sm2465872wrr.94 - gsmtp.
>  Please check the message recipient
> ""qemu-s390x@nongnu.org"@d06av22.portsmouth.uk.ibm.com" and try again.

Drat... this may prevent recipients starting with Daniel from receiving
the mails... ie, the sub-lists IIUC. Maybe not worth reposting for that.
Hopefully, Daniel may catch up on qemu-devel.

On Tue, Sep 17, 2019 at 01:45:29PM +0200, Greg Kurz wrote:
> On Tue, 17 Sep 2019 13:00:37 +0200
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
> > For some reason your email client escaped incorrectly Daniel's email:
> > 
> >   "Daniel P. Berrangé\" <berrange@redhat.com>
> > 
> 
> The client is stgit's "stg mail" command, and indeed it generates this:
> 
>  "Daniel P. =?utf-8?q?Berrang=C3=A9=22?= <berrange@redhat.com>
>                                    ^^^
>                                 double-quote
> 
> and if I turn the 'é' into a 'e':
> 
>  "Daniel P. Berrange" <berrange@redhat.com>
> 
> This looks like a bug in "stg mail"...

Happy to see that my name is detecting bugs :-)

I should add an emoji to it to really flush out the edge cases...

> > Which makes my email client very unhappy (Thunderbird):
> > 
> > There are non-ASCII characters in the local part of the recipient
> > address "Daniel P. Berrangé <berrange@redhat.com>,
> > qemu-block@nongnu.org, qemu-arm@nongnu.org, qemu-ppc@nongnu.org,
> > qemu-s390x@nongnu.org"@d06av23.portsmouth.uk.ibm.com. This is not yet
> > supported. Please change this address and try again.
> > 
> > Neither is MTA:
> > 
> > An error occurred while sending mail. The mail server responded:
> > 5.1.2 The recipient address <qemu-s390x@nongnu.org@d06av22.portsmouth.uk.ibm
> > 5.1.2 .com> is not a valid RFC-5321 address. r28sm2465872wrr.94 - gsmtp.
> >  Please check the message recipient
> > ""qemu-s390x@nongnu.org"@d06av22.portsmouth.uk.ibm.com" and try again.
> 
> Drat... this may prevent recipients starting with Daniel from receiving
> the mails... ie, the sub-lists IIUC. Maybe not worth reposting for that.

Yeah, I wouldn't bother reposting just for that.

> Hopefully, Daniel may catch up on qemu-devel.

Indeed.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|