[Qemu-devel] [PATCH 0/9] Clean up around error_get_pretty(), qerror_report_err()

[Qemu-devel] [PATCH 0/9] Clean up around error_get_pretty(), qerror_report_err()

[Markus Armbruster]

Tree-wide cleanup, but the tree-wide changes are mechanical.  If
nobody objects, I'll route it through my own tree.

Depends on my "[PATCH] vhost-scsi: Improve error reporting for invalid
vhostfd".

PATCH 01 is responsible for the bulk of the diffstat.  I could split
off one or more of block, arm, x86+pci, scsi, usb, char, net for
maintainers to pick up after the main patch got in.  Cc'ing a few
maintainers who might prefer that, to give them a chance to ask for
it.

PATCH 02 gets rid of an unnecessary use of qerror_report_err().  This
is the patch that causes the dependency mentioned above.

The remaining patches clean up inappropriate uses of
qerror_report_err().

Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Andreas Färber <afaerber@suse.de>

Markus Armbruster (9):
  error: New convenience function error_report_err()
  monitor: Clean up around monitor_handle_fd_param()
  monitor: Avoid qerror_report_err() outside QMP command handlers
  net: Avoid qerror_report_err() outside QMP command handlers
  numa: Avoid qerror_report_err() outside QMP command handlers
  tpm: Avoid qerror_report_err() outside QMP command handlers
  vl: Avoid qerror_report_err() outside QMP command handlers
  qemu-img: Avoid qerror_report_err() outside QMP command handlers
  qemu-char: Avoid qerror_report_err() outside QMP command handlers

 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  | 14 +++++---------
 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/scsi/vhost-scsi.c      |  2 +-
 hw/usb/dev-network.c      |  3 +--
 hw/usb/host-libusb.c      |  3 +--
 hw/usb/redirect.c         |  3 +--
 include/monitor/monitor.h |  3 +--
 include/qapi/error.h      |  5 +++++
 monitor.c                 | 18 ++----------------
 net/net.c                 |  9 +++------
 net/socket.c              |  4 +++-
 net/tap.c                 | 11 ++++++++---
 numa.c                    |  8 +++-----
 qemu-char.c               |  6 ++----
 qemu-img.c                |  9 +++------
 qemu-io.c                 |  3 +--
 qemu-nbd.c                | 12 ++++--------
 qom/cpu.c                 |  3 +--
 target-i386/cpu.c         |  3 +--
 target-sparc/cpu.c        |  3 +--
 tpm.c                     |  6 ++----
 util/error.c              | 14 ++++++++++----
 util/qemu-config.c        |  9 +++------
 vl.c                      | 18 +++++++-----------
 40 files changed, 118 insertions(+), 174 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/9] error: New convenience function error_report_err()

[Markus Armbruster]

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

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);
-- 
1.9.3

[Qemu-devel] [PATCH 2/9] monitor: Clean up around monitor_handle_fd_param()

[Markus Armbruster]

monitor_handle_fd_param() is a wrapper around
monitor_handle_fd_param2() that feeds errors to qerror_report_err()
instead of returning them.  qerror_report_err() is inappropriate in
many contexts.  monitor_handle_fd_param() looks simpler than
monitor_handle_fd_param2(), which tempts use.  Remove the temptation:
drop the wrapper and open-code the (trivial) error handling instead.

Replace the open-coded qerror_report_err() by error_report_err() in
places that already use error_report().  Turns out that's everywhere.

While there, rename monitor_handle_fd_param2() to monitor_fd_param().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/i386/kvm/pci-assign.c  |  5 ++---
 hw/scsi/vhost-scsi.c      |  2 +-
 include/monitor/monitor.h |  3 +--
 monitor.c                 | 15 +--------------
 net/socket.c              |  4 +++-
 net/tap.c                 | 11 ++++++++---
 6 files changed, 16 insertions(+), 24 deletions(-)

diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index 29ce2c4..bd92c69 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -552,9 +552,8 @@ static void get_real_device(AssignedDevice *pci_dev, Error **errp)
     snprintf(name, sizeof(name), "%sconfig", dir);
 
     if (pci_dev->configfd_name && *pci_dev->configfd_name) {
-        dev->config_fd = monitor_handle_fd_param2(cur_mon,
-                                                  pci_dev->configfd_name,
-                                                  &local_err);
+        dev->config_fd = monitor_fd_param(cur_mon, pci_dev->configfd_name,
+                                          &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
             return;
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 567f350..484f4a8 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -214,7 +214,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
     }
 
     if (vs->conf.vhostfd) {
-        vhostfd = monitor_handle_fd_param2(cur_mon, vs->conf.vhostfd, &err);
+        vhostfd = monitor_fd_param(cur_mon, vs->conf.vhostfd, &err);
         if (vhostfd == -1) {
             error_setg(errp, "vhost-scsi: unable to parse vhostfd: %s",
                        error_get_pretty(err));
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 47606d0..1c06bed 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -34,8 +34,7 @@ int monitor_read_block_device_key(Monitor *mon, const char *device,
                                   void *opaque);
 
 int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp);
-int monitor_handle_fd_param(Monitor *mon, const char *fdname);
-int monitor_handle_fd_param2(Monitor *mon, const char *fdname, Error **errp);
+int monitor_fd_param(Monitor *mon, const char *fdname, Error **errp);
 
 void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
     GCC_FMT_ATTR(2, 0);
diff --git a/monitor.c b/monitor.c
index c3cc060..ac2a4ab 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2570,20 +2570,7 @@ void monitor_fdset_dup_fd_remove(int dup_fd)
     monitor_fdset_dup_fd_find_remove(dup_fd, true);
 }
 
-int monitor_handle_fd_param(Monitor *mon, const char *fdname)
-{
-    int fd;
-    Error *local_err = NULL;
-
-    fd = monitor_handle_fd_param2(mon, fdname, &local_err);
-    if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
-    }
-    return fd;
-}
-
-int monitor_handle_fd_param2(Monitor *mon, const char *fdname, Error **errp)
+int monitor_fd_param(Monitor *mon, const char *fdname, Error **errp)
 {
     int fd;
     Error *local_err = NULL;
diff --git a/net/socket.c b/net/socket.c
index 68a93cd..c30e03f 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -695,6 +695,7 @@ static int net_socket_udp_init(NetClientState *peer,
 int net_init_socket(const NetClientOptions *opts, const char *name,
                     NetClientState *peer)
 {
+    Error *err = NULL;
     const NetdevSocketOptions *sock;
 
     assert(opts->kind == NET_CLIENT_OPTIONS_KIND_SOCKET);
@@ -715,8 +716,9 @@ int net_init_socket(const NetClientOptions *opts, const char *name,
     if (sock->has_fd) {
         int fd;
 
-        fd = monitor_handle_fd_param(cur_mon, sock->fd);
+        fd = monitor_fd_param(cur_mon, sock->fd, &err);
         if (fd == -1) {
+            error_report_err(err);
             return -1;
         }
         qemu_set_nonblock(fd);
diff --git a/net/tap.c b/net/tap.c
index 1fe0edf..968df46 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -605,6 +605,7 @@ static int net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
                             const char *downscript, const char *vhostfdname,
                             int vnet_hdr, int fd)
 {
+    Error *err = NULL;
     TAPState *s;
     int vhostfd;
 
@@ -643,8 +644,9 @@ static int net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
         options.force = tap->has_vhostforce && tap->vhostforce;
 
         if (tap->has_vhostfd || tap->has_vhostfds) {
-            vhostfd = monitor_handle_fd_param(cur_mon, vhostfdname);
+            vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
             if (vhostfd == -1) {
+                error_report_err(err);
                 return -1;
             }
         } else {
@@ -704,6 +706,7 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
     /* for the no-fd, no-helper case */
     const char *script = NULL; /* suppress wrong "uninit'd use" gcc warning */
     const char *downscript = NULL;
+    Error *err = NULL;
     const char *vhostfdname;
     char ifname[128];
 
@@ -729,8 +732,9 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
             return -1;
         }
 
-        fd = monitor_handle_fd_param(cur_mon, tap->fd);
+        fd = monitor_fd_param(cur_mon, tap->fd, &err);
         if (fd == -1) {
+            error_report_err(err);
             return -1;
         }
 
@@ -768,8 +772,9 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
         }
 
         for (i = 0; i < nfds; i++) {
-            fd = monitor_handle_fd_param(cur_mon, fds[i]);
+            fd = monitor_fd_param(cur_mon, fds[i], &err);
             if (fd == -1) {
+                error_report_err(err);
                 return -1;
             }
 
-- 
1.9.3

[Qemu-devel] [PATCH 3/9] monitor: Avoid qerror_report_err() outside QMP command handlers

[Markus Armbruster]

qerror_report_err() is a transitional interface to help with
converting existing monitor commands to QMP.  It should not be used
elsewhere.  Replace by error_report_err() in HMP command handler
hmp_trace_event().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/monitor.c b/monitor.c
index ac2a4ab..b109188 100644
--- a/monitor.c
+++ b/monitor.c
@@ -889,8 +889,7 @@ static void do_trace_event_set_state(Monitor *mon, const QDict *qdict)
 
     qmp_trace_event_set_state(tp_name, new_state, true, true, &local_err);
     if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_report_err(local_err);
     }
 }
 
-- 
1.9.3

[Qemu-devel] [PATCH 4/9] net: Avoid qerror_report_err() outside QMP command handlers

[Markus Armbruster]

qerror_report_err() is a transitional interface to help with
converting existing monitor commands to QMP.  It should not be used
elsewhere.  Replace by error_report_err() in HMP command handler
hmp_host_net_add() and initial startup helpers net_init_client(),
net_init_netdev().  Keep it in QMP command handler qmp_netdev_add().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 net/net.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/net/net.c b/net/net.c
index 74e651e..ec6e581 100644
--- a/net/net.c
+++ b/net/net.c
@@ -974,8 +974,7 @@ void net_host_device_add(Monitor *mon, const QDict *qdict)
 
     net_client_init(opts, 0, &local_err);
     if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_report_err(local_err);
         monitor_printf(mon, "adding host network device %s failed\n", device);
     }
 }
@@ -1270,8 +1269,7 @@ static int net_init_client(QemuOpts *opts, void *dummy)
 
     net_client_init(opts, 0, &local_err);
     if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_report_err(local_err);
         return -1;
     }
 
@@ -1285,8 +1283,7 @@ static int net_init_netdev(QemuOpts *opts, void *dummy)
 
     ret = net_client_init(opts, 1, &local_err);
     if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_report_err(local_err);
         return -1;
     }
 
-- 
1.9.3

[Qemu-devel] [PATCH 5/9] numa: Avoid qerror_report_err() outside QMP command handlers

[Markus Armbruster]

qerror_report_err() is a transitional interface to help with
converting existing monitor commands to QMP.  It should not be used
elsewhere.  Replace by error_report_err() in initial startup helper
numa_init_func() and board setup helper
memory_region_allocate_system_memory().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 numa.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/numa.c b/numa.c
index afd2866..f8fc377 100644
--- a/numa.c
+++ b/numa.c
@@ -146,8 +146,7 @@ int numa_init_func(QemuOpts *opts, void *opaque)
     return 0;
 
 error:
-    qerror_report_err(err);
-    error_free(err);
+    error_report_err(err);
 
     if (object) {
         QapiDeallocVisitor *dv = qapi_dealloc_visitor_new();
@@ -262,8 +261,7 @@ static void allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner,
          * regular RAM allocation.
          */
         if (err) {
-            qerror_report_err(err);
-            error_free(err);
+            error_report_err(err);
             memory_region_init_ram(mr, owner, name, ram_size, &error_abort);
         }
 #else
@@ -298,7 +296,7 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
         }
         MemoryRegion *seg = host_memory_backend_get_memory(backend, &local_err);
         if (local_err) {
-            qerror_report_err(local_err);
+            error_report_err(local_err);
             exit(1);
         }
 
-- 
1.9.3

[Qemu-devel] [PATCH 6/9] tpm: Avoid qerror_report_err() outside QMP command handlers

[Markus Armbruster]

qerror_report_err() is a transitional interface to help with
converting existing monitor commands to QMP.  It should not be used
elsewhere.  Replace by error_report_err() in initial startup helper
configure_tpm().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tpm.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tpm.c b/tpm.c
index c371023..4ffd9b9 100644
--- a/tpm.c
+++ b/tpm.c
@@ -162,8 +162,7 @@ static int configure_tpm(QemuOpts *opts)
     /* validate backend specific opts */
     qemu_opts_validate(opts, be->opts, &local_err);
     if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_report_err(local_err);
         return 1;
     }
 
@@ -174,8 +173,7 @@ static int configure_tpm(QemuOpts *opts)
 
     tpm_backend_open(drv, &local_err);
     if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_report_err(local_err);
         return 1;
     }
 
-- 
1.9.3

[Qemu-devel] [PATCH 7/9] vl: Avoid qerror_report_err() outside QMP command handlers

[Markus Armbruster]

qerror_report_err() is a transitional interface to help with
converting existing monitor commands to QMP.  It should not be used
elsewhere.  Replace by error_report_err() in initial startup helpers
machine_set_property() and object_create().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 vl.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/vl.c b/vl.c
index bfdf935..8544bc9 100644
--- a/vl.c
+++ b/vl.c
@@ -2582,8 +2582,7 @@ static int machine_set_property(const char *name, const char *value,
     g_free(qom_name);
 
     if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_report_err(local_err);
         return -1;
     }
 
@@ -2636,8 +2635,7 @@ out:
     g_free(type);
     g_free(dummy);
     if (err) {
-        qerror_report_err(err);
-        error_free(err);
+        error_report_err(err);
         return -1;
     }
     return 0;
-- 
1.9.3

[Qemu-devel] [PATCH 8/9] qemu-img: Avoid qerror_report_err() outside QMP command handlers

[Markus Armbruster]

qerror_report_err() is a transitional interface to help with
converting existing monitor commands to QMP.  It should not be used
elsewhere.  Replace by error_report_err().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qemu-img.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 97d04bd..25b1369 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -889,8 +889,7 @@ done:
     blk_unref(blk);
 
     if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_report_err(local_err);
         return 1;
     }
 
-- 
1.9.3

[Qemu-devel] [PATCH 9/9] qemu-char: Avoid qerror_report_err() outside QMP command handlers

[Markus Armbruster]

qerror_report_err() is a transitional interface to help with
converting existing monitor commands to QMP.  It should not be used
elsewhere.  Replace by error_report_err() in legacy chardev parser
qemu_chr_parse_compat().  Legacy chardev syntax is not to be used in
QMP.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qemu-char.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index f901ed1..8159462 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3306,8 +3306,7 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
 
     opts = qemu_opts_create(qemu_find_opts("chardev"), label, 1, &local_err);
     if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_report_err(local_err);
         return NULL;
     }
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH 1/9] error: New convenience function error_report_err()

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH 1/9] error: New convenience function error_report_err()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1961 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/9] monitor: Clean up around monitor_handle_fd_param()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1211 bytes --]

On 02/10/2015 09:34 AM, Markus Armbruster wrote:
> monitor_handle_fd_param() is a wrapper around
> monitor_handle_fd_param2() that feeds errors to qerror_report_err()
> instead of returning them.  qerror_report_err() is inappropriate in
> many contexts.  monitor_handle_fd_param() looks simpler than
> monitor_handle_fd_param2(), which tempts use.  Remove the temptation:
> drop the wrapper and open-code the (trivial) error handling instead.
> 
> Replace the open-coded qerror_report_err() by error_report_err() in
> places that already use error_report().  Turns out that's everywhere.
> 
> While there, rename monitor_handle_fd_param2() to monitor_fd_param().
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/i386/kvm/pci-assign.c  |  5 ++---
>  hw/scsi/vhost-scsi.c      |  2 +-
>  include/monitor/monitor.h |  3 +--
>  monitor.c                 | 15 +--------------
>  net/socket.c              |  4 +++-
>  net/tap.c                 | 11 ++++++++---
>  6 files changed, 16 insertions(+), 24 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/9] error: New convenience function error_report_err()

[Eduardo Habkost]

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

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/9] error: New convenience function error_report_err()

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 2335 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH 1/9] error: New convenience function error_report_err()

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH 1/9] error: New convenience function error_report_err()

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH 1/9] error: New convenience function error_report_err()

[Markus Armbruster]

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.

[Qemu-devel] Coccinelle semantic patches (was Re: [PATCH 1/9] error: New convenience function error_report_err())

[Eduardo Habkost]

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,
    ...
  );


-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/9] Clean up around error_get_pretty(), qerror_report_err()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1251 bytes --]

On 02/10/2015 09:34 AM, Markus Armbruster wrote:
> Tree-wide cleanup, but the tree-wide changes are mechanical.  If
> nobody objects, I'll route it through my own tree.
> 
> Depends on my "[PATCH] vhost-scsi: Improve error reporting for invalid
> vhostfd".
> 
> PATCH 01 is responsible for the bulk of the diffstat.  I could split
> off one or more of block, arm, x86+pci, scsi, usb, char, net for
> maintainers to pick up after the main patch got in.  Cc'ing a few
> maintainers who might prefer that, to give them a chance to ask for
> it.
> 
> PATCH 02 gets rid of an unnecessary use of qerror_report_err().  This
> is the patch that causes the dependency mentioned above.
> 
> The remaining patches clean up inappropriate uses of
> qerror_report_err().
> 
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Andreas Färber <afaerber@suse.de>

for patches 3-9:
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]