diff mbox

[1/9] error: New convenience function error_report_err()

Message ID 1423586055-4932-2-git-send-email-armbru@redhat.com
State New
Headers show

Commit Message

Markus Armbruster Feb. 10, 2015, 4:34 p.m. UTC
I've typed error_report("%s", error_get_pretty(ERR)) too many times
already, and I've fixed too many instances of qerror_report_err(ERR)
to error_report("%s", error_get_pretty(ERR)) as well.  Capture the
pattern in a convenience function.

Since it's almost invariably followed by error_free(), stuff that into
the convenience function as well.

Put it to use with this Coccinelle semantic patch:

    @@
    expression E;
    @@
    -    error_report("%s", error_get_pretty(E));
    -    error_free(E);
    +    error_report_err(E);
    @@
    expression E, S;
    @@
    -    error_report("%s", error_get_pretty(E));
    +    error_report_err(E);
    (
         exit(S);
    |
         abort();
    )

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 arch_init.c              |  3 +--
 block/sheepdog.c         | 39 +++++++++++++--------------------------
 blockdev.c               | 12 ++++--------
 hw/arm/exynos4210.c      |  4 ++--
 hw/arm/highbank.c        |  4 ++--
 hw/arm/integratorcp.c    |  4 ++--
 hw/arm/realview.c        |  6 +++---
 hw/arm/versatilepb.c     |  4 ++--
 hw/arm/vexpress.c        |  2 +-
 hw/arm/xilinx_zynq.c     |  8 ++++----
 hw/block/virtio-blk.c    |  3 +--
 hw/char/serial.c         |  6 ++----
 hw/i386/kvm/pci-assign.c |  9 +++------
 hw/i386/pc.c             |  5 ++---
 hw/i386/smbios.c         | 14 +++++++-------
 hw/ide/qdev.c            |  3 +--
 hw/pci/pci-hotplug-old.c |  3 +--
 hw/pci/pci.c             |  3 +--
 hw/usb/dev-network.c     |  3 +--
 hw/usb/host-libusb.c     |  3 +--
 hw/usb/redirect.c        |  3 +--
 include/qapi/error.h     |  5 +++++
 qemu-char.c              |  3 +--
 qemu-img.c               |  6 ++----
 qemu-io.c                |  3 +--
 qemu-nbd.c               | 12 ++++--------
 qom/cpu.c                |  3 +--
 target-i386/cpu.c        |  3 +--
 target-sparc/cpu.c       |  3 +--
 util/error.c             | 14 ++++++++++----
 util/qemu-config.c       |  9 +++------
 vl.c                     | 12 +++++-------
 32 files changed, 89 insertions(+), 125 deletions(-)

Comments

Paolo Bonzini Feb. 10, 2015, 4:44 p.m. UTC | #1
On 10/02/2015 17:34, Markus Armbruster wrote:
> I've typed error_report("%s", error_get_pretty(ERR)) too many times
> already, and I've fixed too many instances of qerror_report_err(ERR)
> to error_report("%s", error_get_pretty(ERR)) as well.  Capture the
> pattern in a convenience function.
> 
> Since it's almost invariably followed by error_free(), stuff that into
> the convenience function as well.
> 
> Put it to use with this Coccinelle semantic patch:

For your "no good deed goes unpunished" record, can you prepare a Wiki
page on how to use Coccinelle?  My attempts always saw it confused by
qemu/queue.h macros.

Paolo
Eric Blake Feb. 10, 2015, 5:20 p.m. UTC | #2
On 02/10/2015 09:34 AM, Markus Armbruster wrote:
> I've typed error_report("%s", error_get_pretty(ERR)) too many times
> already, and I've fixed too many instances of qerror_report_err(ERR)
> to error_report("%s", error_get_pretty(ERR)) as well.  Capture the
> pattern in a convenience function.
> 
> Since it's almost invariably followed by error_free(), stuff that into
> the convenience function as well.
> 

> @@ -2234,8 +2225,7 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>  
>      ret = do_sd_create(s, &new_vid, 1, &local_err);
>      if (ret < 0) {
> -        error_report("%s", error_get_pretty(local_err));;
> -        error_free(local_err);
> +        error_report_err(local_err);
>          error_report("failed to create inode for snapshot. %s",
>                       strerror(errno));

Pre-existing bug, so maybe worth a separate patch.  This looks fishy:
are we guaranteed that errno is unchanged by error_report_err()?  On the
surface, error_vreport() and friends do NOT try to preserve errno; maybe
your new function should guarantee that errno is not clobbered? (in
libvirt, we've explicitly made error-reporting convenience functions
document that they do not clobber errno, because it is much easier for
callers to report an error then return an errno value without having to
save errno locally)

> @@ -152,6 +152,12 @@ const char *error_get_pretty(Error *err)
>      return err->msg;
>  }
>  
> +void error_report_err(Error *err)
> +{
> +    error_report("%s", error_get_pretty(err));
> +    error_free(err);
> +}
> +

If it were me, I'd split this patch in two, one that introduces the new
function (with no clients), and the other which is a strict Coccinelle
touchup to use it, so that readers don't have to hunt for the meat of
the change.
Eduardo Habkost Feb. 10, 2015, 10:04 p.m. UTC | #3
On Tue, Feb 10, 2015 at 05:44:59PM +0100, Paolo Bonzini wrote:
> 
> 
> On 10/02/2015 17:34, Markus Armbruster wrote:
> > I've typed error_report("%s", error_get_pretty(ERR)) too many times
> > already, and I've fixed too many instances of qerror_report_err(ERR)
> > to error_report("%s", error_get_pretty(ERR)) as well.  Capture the
> > pattern in a convenience function.
> > 
> > Since it's almost invariably followed by error_free(), stuff that into
> > the convenience function as well.
> > 
> > Put it to use with this Coccinelle semantic patch:
> 
> For your "no good deed goes unpunished" record, can you prepare a Wiki
> page on how to use Coccinelle?  My attempts always saw it confused by
> qemu/queue.h macros.

I assume you are asking about how to use/apply a semantic patch, not
about writing one[1]. I do this:

$ cat /tmp/error_report.spatch 
@@
expression E;
@@
-    error_report("%s", error_get_pretty(E));
-    error_free(E);
+    error_report_err(E);
@@
expression E, S;
@@
-    error_report("%s", error_get_pretty(E));
+    error_report_err(E);
(
     exit(S);
|
     abort();
)
$ spatch --sp-file /tmp/error_report.spatch $(git grep -l error_report) > /tmp/error_report.patch
init_defs_builtins: /usr/share/coccinelle/standard.h
HANDLING: arch_init.c audio/spiceaudio.c audio/wavcapture.c balloon.c block.c block/archipelago.c block/dmg.c block/iscsi.c block/nfs.c block/qcow2-snapshot.c block/qcow2.c block/rbd.c block/sheepdog.c block/ssh.c block/vhdx-log.c block/vmdk.c blockdev.c bootdevice.c device_tree.c exec.c hw/9pfs/virtio-9p-proxy.c hw/9pfs/virtio-9p.c hw/arm/armv7m.c hw/arm/cubieboard.c hw/arm/digic_boards.c hw/arm/exynos4210.c hw/arm/highbank.c hw/arm/integratorcp.c hw/arm/realview.c hw/arm/strongarm.c hw/arm/versatilepb.c hw/arm/vexpress.c hw/arm/virt.c hw/arm/xilinx_zynq.c hw/audio/milkymist-ac97.c hw/block/onenand.c hw/block/tc58128.c hw/block/virtio-blk.c hw/char/lm32_uart.c hw/char/milkymist-uart.c hw/char/sclpconsole-lm.c hw/char/sclpconsole.c hw/char/serial-pci.c hw/char/serial.c hw/char/virtio-serial-bus.c hw/core/machine.c hw/core/qdev-properties-system.c hw/core/qdev-properties.c hw/core/qdev.c hw/display/cg3.c hw/display/cirrus_vga.c hw/display/g364fb.c hw/display/milkymist-tmu2.c hw/display/milkymist-vgafb.c hw/display/qxl.c hw/display/tcx.c hw/i386/acpi-build.c hw/i386/kvm/pci-assign.c hw/i386/pc.c hw/i386/pc_piix.c hw/i386/pc_q35.c hw/i386/pc_sysfw.c hw/i386/smbios.c hw/ide/ahci.c hw/ide/core.c hw/ide/pci.c hw/ide/qdev.c hw/input/milkymist-softusb.c hw/intc/s390_flic.c hw/intc/s390_flic_kvm.c hw/intc/xics.c hw/intc/xics_kvm.c hw/isa/pc87312.c hw/microblaze/boot.c hw/mips/mips_fulong2e.c hw/mips/mips_jazz.c hw/mips/mips_malta.c hw/mips/mips_mipssim.c hw/misc/ivshmem.c hw/misc/milkymist-hpdmc.c hw/misc/milkymist-pfpu.c hw/net/milkymist-minimac2.c hw/net/vhost_net.c hw/net/virtio-net.c hw/nvram/fw_cfg.c hw/pci/pci-hotplug-old.c hw/pci/pci.c hw/pci/slotid_cap.c hw/ppc/e500.c hw/ppc/mpc8544ds.c hw/ppc/ppc.c hw/ppc/ppc405_boards.c hw/ppc/spapr.c hw/ppc/spapr_pci.c hw/ppc/virtex_ml507.c hw/s390x/s390-pci-inst.c hw/s390x/virtio-ccw.c hw/scsi/scsi-bus.c hw/scsi/vhost-scsi.c hw/scsi/virtio-scsi.c hw/sd/milkymist-memcard.c hw/sh4/shix.c hw/sparc/sun4m.c hw/timer/lm32_timer.c hw/timer/milkymist-sysctl.c hw/tpm/tpm_passthrough.c hw/tricore/tricore_testboard.c hw/usb/bus.c hw/usb/ccid-card-passthru.c hw/usb/dev-bluetooth.c hw/usb/dev-network.c hw/usb/dev-serial.c hw/usb/dev-smartcard-reader.c hw/usb/dev-storage.c hw/usb/dev-uas.c hw/usb/hcd-ehci.c hw/usb/host-libusb.c hw/usb/redirect.c hw/vfio/common.c hw/vfio/pci.c hw/virtio/dataplane/vring.c hw/virtio/vhost-backend.c hw/virtio/vhost-user.c hw/virtio/virtio-pci.c hw/virtio/virtio.c hw/xtensa/sim.c hw/xtensa/xtfpga.c include/qapi/qmp/qerror.h include/qemu/error-report.h migration/block.c migration/migration.c migration/qemu-file-buf.c migration/rdma.c migration/tcp.c migration/unix.c migration/vmstate.c monitor.c net/dump.c net/l2tpv3.c net/net.c net/netmap.c net/slirp.c net/socket.c net/tap-bsd.c net/tap-linux.c net/tap-solaris.c net/tap-win32.c net/tap.c net/vhost-user.c numa.c qdev-monitor.c qemu-char.c qemu-img.c qemu-io-cmds.c qemu-io.c qemu-nbd.c qmp.c qobject/qerror.c qom/cpu.c savevm.c scripts/qapi-commands.py slirp/misc.c target-i386/cpu.c target-mips/kvm.c target-ppc/translate_init.c target-s390x/cpu.c target-s390x/kvm.c target-sparc/cpu.c tests/test-aio.c tests/test-thread-pool.c tests/test-throttle.c tpm.c trace/control.c ui/input.c ui/spice-core.c ui/vnc.c util/error.c util/osdep.c util/qemu-config.c util/qemu-error.c util/qemu-option.c vl.c
[...]
Note: processing took    59.7s: arch_init.c audio/spiceaudio.c audio/wavcapture.c balloon.c block.c block/archipelago.c block/dmg.c block/iscsi.c block/nfs.c block/qcow2-snapshot.c block/qcow2.c block/rbd.c block/sheepdog.c block/ssh.c block/vhdx-log.c block/vmdk.c blockdev.c bootdevice.c device_tree.c exec.c hw/9pfs/virtio-9p-proxy.c hw/9pfs/virtio-9p.c hw/arm/armv7m.c hw/arm/cubieboard.c hw/arm/digic_boards.c hw/arm/exynos4210.c hw/arm/highbank.c hw/arm/integratorcp.c hw/arm/realview.c hw/arm/strongarm.c hw/arm/versatilepb.c hw/arm/vexpress.c hw/arm/virt.c hw/arm/xilinx_zynq.c hw/audio/milkymist-ac97.c hw/block/onenand.c hw/block/tc58128.c hw/block/virtio-blk.c hw/char/lm32_uart.c hw/char/milkymist-uart.c hw/char/sclpconsole-lm.c hw/char/sclpconsole.c hw/char/serial-pci.c hw/char/serial.c hw/char/virtio-serial-bus.c hw/core/machine.c hw/core/qdev-properties-system.c hw/core/qdev-properties.c hw/core/qdev.c hw/display/cg3.c hw/display/cirrus_vga.c hw/display/g364fb.c hw/display/milkymist-tmu2.c hw/display/milkymist-vgafb.c hw/display/qxl.c hw/display/tcx.c hw/i386/acpi-build.c hw/i386/kvm/pci-assign.c hw/i386/pc.c hw/i386/pc_piix.c hw/i386/pc_q35.c hw/i386/pc_sysfw.c hw/i386/smbios.c hw/ide/ahci.c hw/ide/core.c hw/ide/pci.c hw/ide/qdev.c hw/input/milkymist-softusb.c hw/intc/s390_flic.c hw/intc/s390_flic_kvm.c hw/intc/xics.c hw/intc/xics_kvm.c hw/isa/pc87312.c hw/microblaze/boot.c hw/mips/mips_fulong2e.c hw/mips/mips_jazz.c hw/mips/mips_malta.c hw/mips/mips_mipssim.c hw/misc/ivshmem.c hw/misc/milkymist-hpdmc.c hw/misc/milkymist-pfpu.c hw/net/milkymist-minimac2.c hw/net/vhost_net.c hw/net/virtio-net.c hw/nvram/fw_cfg.c hw/pci/pci-hotplug-old.c hw/pci/pci.c hw/pci/slotid_cap.c hw/ppc/e500.c hw/ppc/mpc8544ds.c hw/ppc/ppc.c hw/ppc/ppc405_boards.c hw/ppc/spapr.c hw/ppc/spapr_pci.c hw/ppc/virtex_ml507.c hw/s390x/s390-pci-inst.c hw/s390x/virtio-ccw.c hw/scsi/scsi-bus.c hw/scsi/vhost-scsi.c hw/scsi/virtio-scsi.c hw/sd/milkymist-memcard.c hw/sh4/shix.c hw/sparc/sun4m.c hw/timer/lm32_timer.c hw/timer/milkymist-sysctl.c hw/tpm/tpm_passthrough.c hw/tricore/tricore_testboard.c hw/usb/bus.c hw/usb/ccid-card-passthru.c hw/usb/dev-bluetooth.c hw/usb/dev-network.c hw/usb/dev-serial.c hw/usb/dev-smartcard-reader.c hw/usb/dev-storage.c hw/usb/dev-uas.c hw/usb/hcd-ehci.c hw/usb/host-libusb.c hw/usb/redirect.c hw/vfio/common.c hw/vfio/pci.c hw/virtio/dataplane/vring.c hw/virtio/vhost-backend.c hw/virtio/vhost-user.c hw/virtio/virtio-pci.c hw/virtio/virtio.c hw/xtensa/sim.c hw/xtensa/xtfpga.c include/qapi/qmp/qerror.h include/qemu/error-report.h migration/block.c migration/migration.c migration/qemu-file-buf.c migration/rdma.c migration/tcp.c migration/unix.c migration/vmstate.c monitor.c net/dump.c net/l2tpv3.c net/net.c net/netmap.c net/slirp.c net/socket.c net/tap-bsd.c net/tap-linux.c net/tap-solaris.c net/tap-win32.c net/tap.c net/vhost-user.c numa.c qdev-monitor.c qemu-char.c qemu-img.c qemu-io-cmds.c qemu-io.c qemu-nbd.c qmp.c qobject/qerror.c qom/cpu.c savevm.c scripts/qapi-commands.py slirp/misc.c target-i386/cpu.c target-mips/kvm.c target-ppc/translate_init.c target-s390x/cpu.c target-s390x/kvm.c target-sparc/cpu.c tests/test-aio.c tests/test-thread-pool.c tests/test-throttle.c tpm.c trace/control.c ui/input.c ui/spice-core.c ui/vnc.c util/error.c util/osdep.c util/qemu-config.c util/qemu-error.c util/qemu-option.c vl.c
$ diffstat /tmp/error_report.patch 
 cocci-output-11381-08fbd5-arch_init.c       |  600 +++---
 cocci-output-11381-0f7074-vexpress.c        |  314 +--
 cocci-output-11381-162164-realview.c        |  212 +-
 cocci-output-11381-219b63-host-libusb.c     |  899 ++++-----
 cocci-output-11381-2417f9-sheepdog.c        | 1559 ++++++++--------
 cocci-output-11381-260c6a-blockdev.c        | 1736 +++++++++---------
 cocci-output-11381-2c3742-redirect.c        | 1426 +++++++--------
 cocci-output-11381-2eec3c-qemu-char.c       | 2616 +++++++++++++---------------
 cocci-output-11381-2f6baa-pci-assign.c      | 1077 +++++------
 cocci-output-11381-353382-virtio-blk.c      |  467 ++--
 cocci-output-11381-35d629-exynos4210.c      |  108 -
 cocci-output-11381-427134-qemu-config.c     |  267 +-
 cocci-output-11381-4a0b4a-smbios.c          |  759 ++++----
 cocci-output-11381-50f054-versatilepb.c     |  210 +-
 cocci-output-11381-5f8ab0-vl.c              | 2020 ++++++++++-----------
 cocci-output-11381-66581d-qemu-io.c         |  197 +-
 cocci-output-11381-69640d-highbank.c        |  199 +-
 cocci-output-11381-6f68f2-cpu.c             | 2419 ++++++++++++-------------
 cocci-output-11381-714f3f-dev-network.c     |  900 ++++-----
 cocci-output-11381-85678e-error.c           |  115 -
 cocci-output-11381-8c3a4d-serial.c          |  640 +++---
 cocci-output-11381-96591b-integratorcp.c    |  284 +--
 cocci-output-11381-a7fe63-cpu.c             |  231 +-
 cocci-output-11381-adb67b-qdev.c            |  219 +-
 cocci-output-11381-affb2c-qemu-img.c        | 1739 +++++++++---------
 cocci-output-11381-b34641-qemu-nbd.c        |  337 +--
 cocci-output-11381-b71e7c-cpu.c             | 1001 +++++-----
 cocci-output-11381-b9a744-pc.c              | 1031 +++++------
 cocci-output-11381-bddd78-pci-hotplug-old.c |  125 -
 cocci-output-11381-fa9822-xilinx_zynq.c     |  134 -
 cocci-output-11381-fcda25-pci.c             | 1436 +++++++--------
 31 files changed, 12602 insertions(+), 12675 deletions(-)
$ 

(Don't ask me why the weird filenames. The patch applies cleanly if I
use "patch -p0")


[1] If you are talking about writing them, I think the best way to learn
    it is to read some examples[2]. If you want something slightly more
    detailed, there's the tutorial[3].

    Now, if you want additional details, you can read the
    specification[4], learn everything about the SmPL grammar and be
    completely confused about its semantics. I don't see the language
    semantics explained anywhere except on the incomplete tutorial and
    list of examples. :(

[2] http://coccinelle.lip6.fr/rules/
[3] http://coccinelle.lip6.fr/papers/ols07-padioleau.pdf
[4] http://coccinelle.lip6.fr/docs/index.html
Kevin Wolf Feb. 11, 2015, 10:04 a.m. UTC | #4
Am 10.02.2015 um 18:20 hat Eric Blake geschrieben:
> On 02/10/2015 09:34 AM, Markus Armbruster wrote:
> > I've typed error_report("%s", error_get_pretty(ERR)) too many times
> > already, and I've fixed too many instances of qerror_report_err(ERR)
> > to error_report("%s", error_get_pretty(ERR)) as well.  Capture the
> > pattern in a convenience function.
> > 
> > Since it's almost invariably followed by error_free(), stuff that into
> > the convenience function as well.
> > 
> 
> > @@ -2234,8 +2225,7 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
> >  
> >      ret = do_sd_create(s, &new_vid, 1, &local_err);
> >      if (ret < 0) {
> > -        error_report("%s", error_get_pretty(local_err));;
> > -        error_free(local_err);
> > +        error_report_err(local_err);
> >          error_report("failed to create inode for snapshot. %s",
> >                       strerror(errno));
> 
> Pre-existing bug, so maybe worth a separate patch.  This looks fishy:
> are we guaranteed that errno is unchanged by error_report_err()?

errno doesn't seem to contain anything meaningful here in the first
place, so I think that line should simply be removed.

> On the
> surface, error_vreport() and friends do NOT try to preserve errno; maybe
> your new function should guarantee that errno is not clobbered? (in
> libvirt, we've explicitly made error-reporting convenience functions
> document that they do not clobber errno, because it is much easier for
> callers to report an error then return an errno value without having to
> save errno locally)

Isn't something going wrong if you report an error and pass it on at the
same time? I always thought that the error reporting should be at the
end of the error handling code.

> > @@ -152,6 +152,12 @@ const char *error_get_pretty(Error *err)
> >      return err->msg;
> >  }
> >  
> > +void error_report_err(Error *err)
> > +{
> > +    error_report("%s", error_get_pretty(err));
> > +    error_free(err);
> > +}
> > +
> 
> If it were me, I'd split this patch in two, one that introduces the new
> function (with no clients), and the other which is a strict Coccinelle
> touchup to use it, so that readers don't have to hunt for the meat of
> the change.

Yes, I thought the same.

Kevin
Paolo Bonzini Feb. 11, 2015, 10:17 a.m. UTC | #5
On 10/02/2015 23:04, Eduardo Habkost wrote:
> $ spatch --sp-file /tmp/error_report.spatch $(git grep -l error_report) > /tmp/error_report.patch

Does it work without specifying any include path?

I tried this a while ago:

@@
identifier s, fld;
@@
  struct s {
    ...
-   QEMUTimer *fld;
+   QEMUTimer fld;
    ...
  }
@@
expression E;
identifier fld;
@@
- E->fld = timer_new_ms(
+ timer_init_ms(&E->fld,
    ...
  );


(The patch is incomplete, but shows the problem).  "spatch --sp-file
timer.cocci hw/arm/pxa2xx.c" misses the first replacement for struct
PXA2xxRTCState, but "spatch --sp-file timer.cocci hmp.c" does it right
in struct MigrationStatus.

Paolo
Markus Armbruster Feb. 11, 2015, 12:24 p.m. UTC | #6
Kevin Wolf <kwolf@redhat.com> writes:

> Am 10.02.2015 um 18:20 hat Eric Blake geschrieben:
>> On 02/10/2015 09:34 AM, Markus Armbruster wrote:
>> > I've typed error_report("%s", error_get_pretty(ERR)) too many times
>> > already, and I've fixed too many instances of qerror_report_err(ERR)
>> > to error_report("%s", error_get_pretty(ERR)) as well.  Capture the
>> > pattern in a convenience function.
>> > 
>> > Since it's almost invariably followed by error_free(), stuff that into
>> > the convenience function as well.
>> > 
>> 
>> > @@ -2234,8 +2225,7 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>> >  
>> >      ret = do_sd_create(s, &new_vid, 1, &local_err);
>> >      if (ret < 0) {
>> > -        error_report("%s", error_get_pretty(local_err));;
>> > -        error_free(local_err);
>> > +        error_report_err(local_err);
>> >          error_report("failed to create inode for snapshot. %s",
>> >                       strerror(errno));
>> 
>> Pre-existing bug, so maybe worth a separate patch.  This looks fishy:
>> are we guaranteed that errno is unchanged by error_report_err()?
>
> errno doesn't seem to contain anything meaningful here in the first
> place, so I think that line should simply be removed.

Either that, or combine the messages like this:

    error_report("failed to create inode for snapshot: %s",
                 error_get_pretty(local_err));

Two error_report() in a row are suspect in general.

Aside: do_sd_create() can't decide what to return on failure, negative
value without meaning, or a negative error number.

>> On the
>> surface, error_vreport() and friends do NOT try to preserve errno; maybe
>> your new function should guarantee that errno is not clobbered? (in
>> libvirt, we've explicitly made error-reporting convenience functions
>> document that they do not clobber errno, because it is much easier for
>> callers to report an error then return an errno value without having to
>> save errno locally)
>
> Isn't something going wrong if you report an error and pass it on at the
> same time? I always thought that the error reporting should be at the
> end of the error handling code.

I guess that depends on the conventions used by the code.  In QEMU,
error reporting is generally an error information sink, i.e. no detailed
information on the reported error is passed further up.

>> > @@ -152,6 +152,12 @@ const char *error_get_pretty(Error *err)
>> >      return err->msg;
>> >  }
>> >  
>> > +void error_report_err(Error *err)
>> > +{
>> > +    error_report("%s", error_get_pretty(err));
>> > +    error_free(err);
>> > +}
>> > +
>> 
>> If it were me, I'd split this patch in two, one that introduces the new
>> function (with no clients), and the other which is a strict Coccinelle
>> touchup to use it, so that readers don't have to hunt for the meat of
>> the change.
>
> Yes, I thought the same.

I can do that.
Markus Armbruster Feb. 11, 2015, 12:47 p.m. UTC | #7
Paolo Bonzini <pbonzini@redhat.com> writes:

> On 10/02/2015 17:34, Markus Armbruster wrote:
>> I've typed error_report("%s", error_get_pretty(ERR)) too many times
>> already, and I've fixed too many instances of qerror_report_err(ERR)
>> to error_report("%s", error_get_pretty(ERR)) as well.  Capture the
>> pattern in a convenience function.
>> 
>> Since it's almost invariably followed by error_free(), stuff that into
>> the convenience function as well.
>> 
>> Put it to use with this Coccinelle semantic patch:
>
> For your "no good deed goes unpunished" record, can you prepare a Wiki
> page on how to use Coccinelle?  My attempts always saw it confused by
> qemu/queue.h macros.

Same here, just "sometimes" instead of "always".  I once tried to use
Coccinelle's macro hints to unconfuse it.  Frustrating failure.

It's clearly a poweful tool, but working with it makes me feel like an
ogre trying to use pliers.  Sometimes I manage to pull a whole raft of
splinters with it in one go, and then I'm one proud ogre.  At other
times, it completely resists my attempts to tell it what I want done.

I'm at least as confused about it as Eduardo.
Eduardo Habkost Feb. 11, 2015, 1:13 p.m. UTC | #8
On Wed, Feb 11, 2015 at 11:17:40AM +0100, Paolo Bonzini wrote:
> 
> 
> On 10/02/2015 23:04, Eduardo Habkost wrote:
> > $ spatch --sp-file /tmp/error_report.spatch $(git grep -l error_report) > /tmp/error_report.patch
> 
> Does it work without specifying any include path?

I didn't even know it had include path options. I don't know what it would use
it for (I guess there are some use cases where parsing the include files would
be useful, but I am not aware of them).

> 
> I tried this a while ago:
> 
> @@
> identifier s, fld;
> @@
>   struct s {
>     ...
> -   QEMUTimer *fld;
> +   QEMUTimer fld;
>     ...
>   }
> @@
> expression E;
> identifier fld;
> @@
> - E->fld = timer_new_ms(
> + timer_init_ms(&E->fld,
>     ...
>   );
> 
> 
> (The patch is incomplete, but shows the problem).  "spatch --sp-file
> timer.cocci hw/arm/pxa2xx.c" misses the first replacement for struct
> PXA2xxRTCState, but "spatch --sp-file timer.cocci hmp.c" does it right
> in struct MigrationStatus.

That's because your rule doesn't match the PXA2xxRTCState declaration, that is
written as "typedef struct { ... } PXA2xxRTCState", and hence doesn't match the
"struct s { ... }" pattern.

I don't know how to write a rule that matches both (the "\( a \| b \)"
format seems to work only inside statement lists). As a good
plier-wielding ogre, I would give up and simply write two rules:

@@
identifier fld;
@@
 struct {
    ...
-   QEMUTimer *fld;
+   QEMUTimer fld;
    ...
 }

@@
identifier s, fld;
@@
 struct s {
    ...
-   QEMUTimer *fld;
+   QEMUTimer fld;
    ...
 }
@@
expression E;
identifier fld;
@@
- E->fld = timer_new_ms(
+ timer_init_ms(&E->fld,
    ...
  );
diff mbox

Patch

diff --git a/arch_init.c b/arch_init.c
index 89c8fa4..8414726 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -1093,8 +1093,7 @@  static int ram_load(QEMUFile *f, void *opaque, int version_id)
 
                             ret = qemu_ram_resize(block->offset, length, &local_err);
                             if (local_err) {
-                                error_report("%s", error_get_pretty(local_err));
-                                error_free(local_err);
+                                error_report_err(local_err);
                             }
                         }
                         break;
diff --git a/block/sheepdog.c b/block/sheepdog.c
index be3176f..6a4a3bd 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -726,8 +726,7 @@  static coroutine_fn void reconnect_to_sdog(void *opaque)
         s->fd = get_sheep_fd(s, &local_err);
         if (s->fd < 0) {
             DPRINTF("Wait for connection to be established\n");
-            error_report("%s", error_get_pretty(local_err));
-            error_free(local_err);
+            error_report_err(local_err);
             co_aio_sleep_ns(bdrv_get_aio_context(s->bs), QEMU_CLOCK_REALTIME,
                             1000000000ULL);
         }
@@ -1283,8 +1282,7 @@  static int reload_inode(BDRVSheepdogState *s, uint32_t snapid, const char *tag)
 
     fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
-        error_report("%s", error_get_pretty(local_err));;
-        error_free(local_err);
+        error_report_err(local_err);
         return -EIO;
     }
 
@@ -1292,8 +1290,7 @@  static int reload_inode(BDRVSheepdogState *s, uint32_t snapid, const char *tag)
 
     ret = find_vdi_name(s, s->name, snapid, tag, &vid, false, &local_err);
     if (ret) {
-        error_report("%s", error_get_pretty(local_err));;
-        error_free(local_err);
+        error_report_err(local_err);
         goto out;
     }
 
@@ -1785,8 +1782,7 @@  static void sd_close(BlockDriverState *bs)
 
     fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
-        error_report("%s", error_get_pretty(local_err));;
-        error_free(local_err);
+        error_report_err(local_err);
         return;
     }
 
@@ -1838,8 +1834,7 @@  static int sd_truncate(BlockDriverState *bs, int64_t offset)
 
     fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
-        error_report("%s", error_get_pretty(local_err));;
-        error_free(local_err);
+        error_report_err(local_err);
         return fd;
     }
 
@@ -1912,8 +1907,7 @@  static bool sd_delete(BDRVSheepdogState *s)
 
     fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
-        error_report("%s", error_get_pretty(local_err));;
-        error_free(local_err);
+        error_report_err(local_err);
         return false;
     }
 
@@ -1960,8 +1954,7 @@  static int sd_create_branch(BDRVSheepdogState *s)
     deleted = sd_delete(s);
     ret = do_sd_create(s, &vid, !deleted, &local_err);
     if (ret) {
-        error_report("%s", error_get_pretty(local_err));;
-        error_free(local_err);
+        error_report_err(local_err);
         goto out;
     }
 
@@ -1969,8 +1962,7 @@  static int sd_create_branch(BDRVSheepdogState *s)
 
     fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
-        error_report("%s", error_get_pretty(local_err));;
-        error_free(local_err);
+        error_report_err(local_err);
         ret = fd;
         goto out;
     }
@@ -2218,8 +2210,7 @@  static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     /* refresh inode. */
     fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
-        error_report("%s", error_get_pretty(local_err));;
-        error_free(local_err);
+        error_report_err(local_err);
         ret = fd;
         goto cleanup;
     }
@@ -2234,8 +2225,7 @@  static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
 
     ret = do_sd_create(s, &new_vid, 1, &local_err);
     if (ret < 0) {
-        error_report("%s", error_get_pretty(local_err));;
-        error_free(local_err);
+        error_report_err(local_err);
         error_report("failed to create inode for snapshot. %s",
                      strerror(errno));
         goto cleanup;
@@ -2336,8 +2326,7 @@  static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
 
     fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
-        error_report("%s", error_get_pretty(local_err));;
-        error_free(local_err);
+        error_report_err(local_err);
         ret = fd;
         goto out;
     }
@@ -2366,8 +2355,7 @@  static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
 
     fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
-        error_report("%s", error_get_pretty(local_err));;
-        error_free(local_err);
+        error_report_err(local_err);
         ret = fd;
         goto out;
     }
@@ -2429,8 +2417,7 @@  static int do_load_save_vmstate(BDRVSheepdogState *s, uint8_t *data,
 
     fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
-        error_report("%s", error_get_pretty(local_err));;
-        error_free(local_err);
+        error_report_err(local_err);
         return fd;
     }
 
diff --git a/blockdev.c b/blockdev.c
index 7d34960..5437955 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -728,8 +728,7 @@  DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
         qemu_opt_rename(all_opts, opt_renames[i].from, opt_renames[i].to,
                         &local_err);
         if (local_err) {
-            error_report("%s", error_get_pretty(local_err));
-            error_free(local_err);
+            error_report_err(local_err);
             return NULL;
         }
     }
@@ -767,8 +766,7 @@  DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
                                    &error_abort);
     qemu_opts_absorb_qdict(legacy_opts, bs_opts, &local_err);
     if (local_err) {
-        error_report("%s", error_get_pretty(local_err));
-        error_free(local_err);
+        error_report_err(local_err);
         goto fail;
     }
 
@@ -983,8 +981,7 @@  DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
     bs_opts = NULL;
     if (!blk) {
         if (local_err) {
-            error_report("%s", error_get_pretty(local_err));
-            error_free(local_err);
+            error_report_err(local_err);
         }
         goto fail;
     } else {
@@ -1978,8 +1975,7 @@  int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
     aio_context_acquire(aio_context);
 
     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, &local_err)) {
-        error_report("%s", error_get_pretty(local_err));
-        error_free(local_err);
+        error_report_err(local_err);
         aio_context_release(aio_context);
         return -1;
     }
diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
index 97dafca..c55fab8 100644
--- a/hw/arm/exynos4210.c
+++ b/hw/arm/exynos4210.c
@@ -158,7 +158,7 @@  Exynos4210State *exynos4210_init(MemoryRegion *system_mem,
         if (object_property_find(cpuobj, "has_el3", NULL)) {
             object_property_set_bool(cpuobj, false, "has_el3", &err);
             if (err) {
-                error_report("%s", error_get_pretty(err));
+                error_report_err(err);
                 exit(1);
             }
         }
@@ -168,7 +168,7 @@  Exynos4210State *exynos4210_init(MemoryRegion *system_mem,
                                 "reset-cbar", &error_abort);
         object_property_set_bool(cpuobj, true, "realized", &err);
         if (err) {
-            error_report("%s", error_get_pretty(err));
+            error_report_err(err);
             exit(1);
         }
     }
diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index f67570a..a92cdc3 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -248,7 +248,7 @@  static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
         if (object_property_find(cpuobj, "has_el3", NULL)) {
             object_property_set_bool(cpuobj, false, "has_el3", &err);
             if (err) {
-                error_report("%s", error_get_pretty(err));
+                error_report_err(err);
                 exit(1);
             }
         }
@@ -259,7 +259,7 @@  static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
         }
         object_property_set_bool(cpuobj, true, "realized", &err);
         if (err) {
-            error_report("%s", error_get_pretty(err));
+            error_report_err(err);
             exit(1);
         }
         cpu_irq[n] = qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_IRQ);
diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
index 8c48b68..949ae1e 100644
--- a/hw/arm/integratorcp.c
+++ b/hw/arm/integratorcp.c
@@ -500,14 +500,14 @@  static void integratorcp_init(MachineState *machine)
     if (object_property_find(cpuobj, "has_el3", NULL)) {
         object_property_set_bool(cpuobj, false, "has_el3", &err);
         if (err) {
-            error_report("%s", error_get_pretty(err));
+            error_report_err(err);
             exit(1);
         }
     }
 
     object_property_set_bool(cpuobj, true, "realized", &err);
     if (err) {
-        error_report("%s", error_get_pretty(err));
+        error_report_err(err);
         exit(1);
     }
 
diff --git a/hw/arm/realview.c b/hw/arm/realview.c
index 50cb93d..ef2788d 100644
--- a/hw/arm/realview.c
+++ b/hw/arm/realview.c
@@ -108,7 +108,7 @@  static void realview_init(MachineState *machine,
         if (object_property_find(cpuobj, "has_el3", NULL)) {
             object_property_set_bool(cpuobj, false, "has_el3", &err);
             if (err) {
-                error_report("%s", error_get_pretty(err));
+                error_report_err(err);
                 exit(1);
             }
         }
@@ -116,14 +116,14 @@  static void realview_init(MachineState *machine,
         if (is_pb && is_mpcore) {
             object_property_set_int(cpuobj, periphbase, "reset-cbar", &err);
             if (err) {
-                error_report("%s", error_get_pretty(err));
+                error_report_err(err);
                 exit(1);
             }
         }
 
         object_property_set_bool(cpuobj, true, "realized", &err);
         if (err) {
-            error_report("%s", error_get_pretty(err));
+            error_report_err(err);
             exit(1);
         }
 
diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
index b1dae77..624fdb0 100644
--- a/hw/arm/versatilepb.c
+++ b/hw/arm/versatilepb.c
@@ -213,14 +213,14 @@  static void versatile_init(MachineState *machine, int board_id)
     if (object_property_find(cpuobj, "has_el3", NULL)) {
         object_property_set_bool(cpuobj, false, "has_el3", &err);
         if (err) {
-            error_report("%s", error_get_pretty(err));
+            error_report_err(err);
             exit(1);
         }
     }
 
     object_property_set_bool(cpuobj, true, "realized", &err);
     if (err) {
-        error_report("%s", error_get_pretty(err));
+        error_report_err(err);
         exit(1);
     }
 
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index 84415c8..5933454 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -223,7 +223,7 @@  static void init_cpus(const char *cpu_model, const char *privdev,
         }
         object_property_set_bool(cpuobj, true, "realized", &err);
         if (err) {
-            error_report("%s", error_get_pretty(err));
+            error_report_err(err);
             exit(1);
         }
     }
diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 06e6e24..5c37521 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -133,25 +133,25 @@  static void zynq_init(MachineState *machine)
     if (object_property_find(OBJECT(cpu), "has_el3", NULL)) {
         object_property_set_bool(OBJECT(cpu), false, "has_el3", &err);
         if (err) {
-            error_report("%s", error_get_pretty(err));
+            error_report_err(err);
             exit(1);
         }
     }
 
     object_property_set_int(OBJECT(cpu), ZYNQ_BOARD_MIDR, "midr", &err);
     if (err) {
-        error_report("%s", error_get_pretty(err));
+        error_report_err(err);
         exit(1);
     }
 
     object_property_set_int(OBJECT(cpu), MPCORE_PERIPHBASE, "reset-cbar", &err);
     if (err) {
-        error_report("%s", error_get_pretty(err));
+        error_report_err(err);
         exit(1);
     }
     object_property_set_bool(OBJECT(cpu), true, "realized", &err);
     if (err) {
-        error_report("%s", error_get_pretty(err));
+        error_report_err(err);
         exit(1);
     }
 
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 1a8a176..cb71772 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -858,8 +858,7 @@  static void virtio_blk_migration_state_changed(Notifier *notifier, void *data)
         virtio_blk_data_plane_create(VIRTIO_DEVICE(s), &s->conf,
                                      &s->dataplane, &err);
         if (err != NULL) {
-            error_report("%s", error_get_pretty(err));
-            error_free(err);
+            error_report_err(err);
         }
     }
 }
diff --git a/hw/char/serial.c b/hw/char/serial.c
index 0491897..55011cf 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -906,8 +906,7 @@  SerialState *serial_init(int base, qemu_irq irq, int baudbase,
     s->chr = chr;
     serial_realize_core(s, &err);
     if (err != NULL) {
-        error_report("%s", error_get_pretty(err));
-        error_free(err);
+        error_report_err(err);
         exit(1);
     }
 
@@ -970,8 +969,7 @@  SerialState *serial_mm_init(MemoryRegion *address_space,
 
     serial_realize_core(s, &err);
     if (err != NULL) {
-        error_report("%s", error_get_pretty(err));
-        error_free(err);
+        error_report_err(err);
         exit(1);
     }
     vmstate_register(NULL, base, &vmstate_serial, s);
diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index bb206da..29ce2c4 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -953,8 +953,7 @@  static void assigned_dev_update_irq_routing(PCIDevice *dev)
 
     r = assign_intx(assigned_dev, &err);
     if (r < 0) {
-        error_report("%s", error_get_pretty(err));
-        error_free(err);
+        error_report_err(err);
         err = NULL;
         qdev_unplug(&dev->qdev, &err);
         assert(!err);
@@ -1010,8 +1009,7 @@  static void assigned_dev_update_msi(PCIDevice *pci_dev)
 
         assign_intx(assigned_dev, &local_err);
         if (local_err) {
-            error_report("%s", error_get_pretty(local_err));
-            error_free(local_err);
+            error_report_err(local_err);
         }
     }
 }
@@ -1158,8 +1156,7 @@  static void assigned_dev_update_msix(PCIDevice *pci_dev)
 
         assign_intx(assigned_dev, &local_err);
         if (local_err) {
-            error_report("%s", error_get_pretty(local_err));
-            error_free(local_err);
+            error_report_err(local_err);
         }
     }
 }
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c7af6aa..05008cb 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -414,7 +414,7 @@  void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
 
     set_boot_dev(s, boot_device, &local_err);
     if (local_err) {
-        error_report("%s", error_get_pretty(local_err));
+        error_report_err(local_err);
         exit(1);
     }
 
@@ -1040,8 +1040,7 @@  void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
         cpu = pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i),
                          icc_bridge, &error);
         if (error) {
-            error_report("%s", error_get_pretty(error));
-            error_free(error);
+            error_report_err(error);
             exit(1);
         }
     }
diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index 12d2137..f2e9ab6 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -908,7 +908,7 @@  void smbios_entry_add(QemuOpts *opts)
 
         qemu_opts_validate(opts, qemu_smbios_file_opts, &local_err);
         if (local_err) {
-            error_report("%s", error_get_pretty(local_err));
+            error_report_err(local_err);
             exit(1);
         }
 
@@ -994,7 +994,7 @@  void smbios_entry_add(QemuOpts *opts)
         case 0:
             qemu_opts_validate(opts, qemu_smbios_type0_opts, &local_err);
             if (local_err) {
-                error_report("%s", error_get_pretty(local_err));
+                error_report_err(local_err);
                 exit(1);
             }
             save_opt(&type0.vendor, opts, "vendor");
@@ -1014,7 +1014,7 @@  void smbios_entry_add(QemuOpts *opts)
         case 1:
             qemu_opts_validate(opts, qemu_smbios_type1_opts, &local_err);
             if (local_err) {
-                error_report("%s", error_get_pretty(local_err));
+                error_report_err(local_err);
                 exit(1);
             }
             save_opt(&type1.manufacturer, opts, "manufacturer");
@@ -1036,7 +1036,7 @@  void smbios_entry_add(QemuOpts *opts)
         case 2:
             qemu_opts_validate(opts, qemu_smbios_type2_opts, &local_err);
             if (local_err) {
-                error_report("%s", error_get_pretty(local_err));
+                error_report_err(local_err);
                 exit(1);
             }
             save_opt(&type2.manufacturer, opts, "manufacturer");
@@ -1049,7 +1049,7 @@  void smbios_entry_add(QemuOpts *opts)
         case 3:
             qemu_opts_validate(opts, qemu_smbios_type3_opts, &local_err);
             if (local_err) {
-                error_report("%s", error_get_pretty(local_err));
+                error_report_err(local_err);
                 exit(1);
             }
             save_opt(&type3.manufacturer, opts, "manufacturer");
@@ -1061,7 +1061,7 @@  void smbios_entry_add(QemuOpts *opts)
         case 4:
             qemu_opts_validate(opts, qemu_smbios_type4_opts, &local_err);
             if (local_err) {
-                error_report("%s", error_get_pretty(local_err));
+                error_report_err(local_err);
                 exit(1);
             }
             save_opt(&type4.sock_pfx, opts, "sock_pfx");
@@ -1074,7 +1074,7 @@  void smbios_entry_add(QemuOpts *opts)
         case 17:
             qemu_opts_validate(opts, qemu_smbios_type17_opts, &local_err);
             if (local_err) {
-                error_report("%s", error_get_pretty(local_err));
+                error_report_err(local_err);
                 exit(1);
             }
             save_opt(&type17.loc_pfx, opts, "loc_pfx");
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 1ebb58d..b4103fa 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -172,8 +172,7 @@  static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
     if (kind != IDE_CD) {
         blkconf_geometry(&dev->conf, &dev->chs_trans, 65536, 16, 255, &err);
         if (err) {
-            error_report("%s", error_get_pretty(err));
-            error_free(err);
+            error_report_err(err);
             return -1;
         }
     }
diff --git a/hw/pci/pci-hotplug-old.c b/hw/pci/pci-hotplug-old.c
index 0c09c72..d07db25 100644
--- a/hw/pci/pci-hotplug-old.c
+++ b/hw/pci/pci-hotplug-old.c
@@ -132,8 +132,7 @@  static int scsi_hot_add(Monitor *mon, DeviceState *adapter,
                                         dinfo->unit, false, -1, NULL,
                                         &local_err);
     if (!scsidev) {
-        error_report("%s", error_get_pretty(local_err));
-        error_free(local_err);
+        error_report_err(local_err);
         return -1;
     }
     dinfo->unit = scsidev->id;
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index d508930..31b222d 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2038,8 +2038,7 @@  int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
     ret = pci_add_capability2(pdev, cap_id, offset, size, &local_err);
     if (local_err) {
         assert(ret < 0);
-        error_report("%s", error_get_pretty(local_err));
-        error_free(local_err);
+        error_report_err(local_err);
     } else {
         /* success implies a positive offset in config space */
         assert(ret > 0);
diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
index 5b95d5c..c49fde8 100644
--- a/hw/usb/dev-network.c
+++ b/hw/usb/dev-network.c
@@ -1399,8 +1399,7 @@  static USBDevice *usb_net_init(USBBus *bus, const char *cmdline)
 
     idx = net_client_init(opts, 0, &local_err);
     if (local_err) {
-        error_report("%s", error_get_pretty(local_err));
-        error_free(local_err);
+        error_report_err(local_err);
         return NULL;
     }
 
diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index cff4f7c..38f7c72 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -878,8 +878,7 @@  static int usb_host_open(USBHostDevice *s, libusb_device *dev)
 
     usb_device_attach(udev, &local_err);
     if (local_err) {
-        error_report("%s", error_get_pretty(local_err));
-        error_free(local_err);
+        error_report_err(local_err);
         goto fail;
     }
 
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 962d3f5..2416de8 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -1273,8 +1273,7 @@  static void usbredir_do_attach(void *opaque)
 
     usb_device_attach(&dev->dev, &local_err);
     if (local_err) {
-        error_report("%s", error_get_pretty(local_err));
-        error_free(local_err);
+        error_report_err(local_err);
         WARNING("rejecting device due to speed mismatch\n");
         usbredir_reject_device(dev);
     }
diff --git a/include/qapi/error.h b/include/qapi/error.h
index d712089..f44c451 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -83,6 +83,11 @@  Error *error_copy(const Error *err);
 const char *error_get_pretty(Error *err);
 
 /**
+ * Convenience function to error_report() and free an error object.
+ */
+void error_report_err(Error *);
+
+/**
  * Propagate an error to an indirect pointer to an error.  This function will
  * always transfer ownership of the error reference and handles the case where
  * dst_err is NULL correctly.  Errors after the first are discarded.
diff --git a/qemu-char.c b/qemu-char.c
index 98d4342..f901ed1 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3753,8 +3753,7 @@  CharDriverState *qemu_chr_new(const char *label, const char *filename, void (*in
 
     chr = qemu_chr_new_from_opts(opts, init, &err);
     if (err) {
-        error_report("%s", error_get_pretty(err));
-        error_free(err);
+        error_report_err(err);
     }
     if (chr && qemu_opt_get_bool(opts, "mux", 0)) {
         qemu_chr_fe_claim_no_fail(chr);
diff --git a/qemu-img.c b/qemu-img.c
index e148af8..97d04bd 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2004,8 +2004,7 @@  static ImageInfoList *collect_image_info_list(const char *filename,
 
         bdrv_query_image_info(bs, &info, &err);
         if (err) {
-            error_report("%s", error_get_pretty(err));
-            error_free(err);
+            error_report_err(err);
             blk_unref(blk);
             goto err;
         }
@@ -3053,8 +3052,7 @@  int main(int argc, char **argv)
     qemu_init_exec_dir(argv[0]);
 
     if (qemu_init_main_loop(&local_error)) {
-        error_report("%s", error_get_pretty(local_error));
-        error_free(local_error);
+        error_report_err(local_error);
         exit(EXIT_FAILURE);
     }
 
diff --git a/qemu-io.c b/qemu-io.c
index 91a445a..d7a32e4 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -464,8 +464,7 @@  int main(int argc, char **argv)
     }
 
     if (qemu_init_main_loop(&local_error)) {
-        error_report("%s", error_get_pretty(local_error));
-        error_free(local_error);
+        error_report_err(local_error);
         exit(1);
     }
 
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 4d8df08..2ba2d4c 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -228,8 +228,7 @@  static int tcp_socket_incoming(const char *address, uint16_t port)
     int fd = inet_listen(address_and_port, NULL, 0, SOCK_STREAM, 0, &local_err);
 
     if (local_err != NULL) {
-        error_report("%s", error_get_pretty(local_err));
-        error_free(local_err);
+        error_report_err(local_err);
     }
     return fd;
 }
@@ -240,8 +239,7 @@  static int unix_socket_incoming(const char *path)
     int fd = unix_listen(path, NULL, 0, &local_err);
 
     if (local_err != NULL) {
-        error_report("%s", error_get_pretty(local_err));
-        error_free(local_err);
+        error_report_err(local_err);
     }
     return fd;
 }
@@ -252,8 +250,7 @@  static int unix_socket_outgoing(const char *path)
     int fd = unix_connect(path, &local_err);
 
     if (local_err != NULL) {
-        error_report("%s", error_get_pretty(local_err));
-        error_free(local_err);
+        error_report_err(local_err);
     }
     return fd;
 }
@@ -681,8 +678,7 @@  int main(int argc, char **argv)
     }
 
     if (qemu_init_main_loop(&local_err)) {
-        error_report("%s", error_get_pretty(local_err));
-        error_free(local_err);
+        error_report_err(local_err);
         exit(EXIT_FAILURE);
     }
     bdrv_init();
diff --git a/qom/cpu.c b/qom/cpu.c
index 9c68fa4..970377e 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -71,8 +71,7 @@  CPUState *cpu_generic_init(const char *typename, const char *cpu_model)
 
 out:
     if (err != NULL) {
-        error_report("%s", error_get_pretty(err));
-        error_free(err);
+        error_report_err(err);
         object_unref(OBJECT(cpu));
         return NULL;
     }
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3a9b32e..d543e2b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2163,8 +2163,7 @@  X86CPU *cpu_x86_init(const char *cpu_model)
 
 out:
     if (error) {
-        error_report("%s", error_get_pretty(error));
-        error_free(error);
+        error_report_err(error);
         if (cpu != NULL) {
             object_unref(OBJECT(cpu));
             cpu = NULL;
diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c
index aa7626c..a952097 100644
--- a/target-sparc/cpu.c
+++ b/target-sparc/cpu.c
@@ -111,8 +111,7 @@  static int cpu_sparc_register(SPARCCPU *cpu, const char *cpu_model)
     cc->parse_features(CPU(cpu), featurestr, &err);
     g_free(s);
     if (err) {
-        error_report("%s", error_get_pretty(err));
-        error_free(err);
+        error_report_err(err);
         return -1;
     }
 
diff --git a/util/error.c b/util/error.c
index 2ace0d8..14f4351 100644
--- a/util/error.c
+++ b/util/error.c
@@ -41,7 +41,7 @@  void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
     err->err_class = err_class;
 
     if (errp == &error_abort) {
-        error_report("%s", error_get_pretty(err));
+        error_report_err(err);
         abort();
     }
 
@@ -77,7 +77,7 @@  void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
     err->err_class = err_class;
 
     if (errp == &error_abort) {
-        error_report("%s", error_get_pretty(err));
+        error_report_err(err);
         abort();
     }
 
@@ -122,7 +122,7 @@  void error_set_win32(Error **errp, int win32_err, ErrorClass err_class,
     err->err_class = err_class;
 
     if (errp == &error_abort) {
-        error_report("%s", error_get_pretty(err));
+        error_report_err(err);
         abort();
     }
 
@@ -152,6 +152,12 @@  const char *error_get_pretty(Error *err)
     return err->msg;
 }
 
+void error_report_err(Error *err)
+{
+    error_report("%s", error_get_pretty(err));
+    error_free(err);
+}
+
 void error_free(Error *err)
 {
     if (err) {
@@ -163,7 +169,7 @@  void error_free(Error *err)
 void error_propagate(Error **dst_errp, Error *local_err)
 {
     if (local_err && dst_errp == &error_abort) {
-        error_report("%s", error_get_pretty(local_err));
+        error_report_err(local_err);
         abort();
     } else if (dst_errp && !*dst_errp) {
         *dst_errp = local_err;
diff --git a/util/qemu-config.c b/util/qemu-config.c
index ba375c0..b13efe2 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -32,8 +32,7 @@  QemuOptsList *qemu_find_opts(const char *group)
 
     ret = find_list(vm_config_groups, group, &local_err);
     if (local_err) {
-        error_report("%s", error_get_pretty(local_err));
-        error_free(local_err);
+        error_report_err(local_err);
     }
 
     return ret;
@@ -314,8 +313,7 @@  int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
             /* group with id */
             list = find_list(lists, group, &local_err);
             if (local_err) {
-                error_report("%s", error_get_pretty(local_err));
-                error_free(local_err);
+                error_report_err(local_err);
                 goto out;
             }
             opts = qemu_opts_create(list, id, 1, NULL);
@@ -325,8 +323,7 @@  int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
             /* group without id */
             list = find_list(lists, group, &local_err);
             if (local_err) {
-                error_report("%s", error_get_pretty(local_err));
-                error_free(local_err);
+                error_report_err(local_err);
                 goto out;
             }
             opts = qemu_opts_create(list, NULL, 0, &error_abort);
diff --git a/vl.c b/vl.c
index 8c8f142..bfdf935 100644
--- a/vl.c
+++ b/vl.c
@@ -2137,8 +2137,7 @@  static int chardev_init_func(QemuOpts *opts, void *opaque)
 
     qemu_chr_new_from_opts(opts, NULL, &local_err);
     if (local_err) {
-        error_report("%s", error_get_pretty(local_err));
-        error_free(local_err);
+        error_report_err(local_err);
         return -1;
     }
     return 0;
@@ -3766,7 +3765,7 @@  int main(int argc, char **argv, char **envp)
     os_daemonize();
 
     if (qemu_init_main_loop(&main_loop_err)) {
-        error_report("%s", error_get_pretty(main_loop_err));
+        error_report_err(main_loop_err);
         exit(1);
     }
 
@@ -4033,8 +4032,7 @@  int main(int argc, char **argv, char **envp)
         Error *local_err = NULL;
         qtest_init(qtest_chrdev, qtest_log, &local_err);
         if (local_err) {
-            error_report("%s", error_get_pretty(local_err));
-            error_free(local_err);
+            error_report_err(local_err);
             exit(1);
         }
     }
@@ -4056,7 +4054,7 @@  int main(int argc, char **argv, char **envp)
         if (order) {
             validate_bootdevices(order, &local_err);
             if (local_err) {
-                error_report("%s", error_get_pretty(local_err));
+                error_report_err(local_err);
                 exit(1);
             }
             boot_order = order;
@@ -4066,7 +4064,7 @@  int main(int argc, char **argv, char **envp)
         if (once) {
             validate_bootdevices(once, &local_err);
             if (local_err) {
-                error_report("%s", error_get_pretty(local_err));
+                error_report_err(local_err);
                 exit(1);
             }
             normal_boot_order = g_strdup(boot_order);