Re: [PATCH v2 00/16] Crazy shit around -global (pardon my french)
John Snow writes: > On 6/22/20 5:42 AM, Markus Armbruster wrote: >> There are three ways to configure backends: >> >> * -nic, -serial, -drive, ... (onboard devices) >> >> * Set the property with -device, or, if you feel masochistic, with >> -set device (pluggable devices) >> >> * Set the property with -global (both) >> >> The trouble is -global is terrible. >> >> It gets applied in object_new(), which can't fail. We treat failure >> to apply -global as fatal error, except when hot-plugging, where we >> treat it as warning *boggle*. I'm not addressing that today. >> >> Some code falls apart when you use both -global and the other way. >> >> To make life more interesting, we gave -drive two roles: with >> interface type other than none, it's for configuring onboard devices, >> and with interface type none, it's for defining backends for use with >> -device and such. Since we neglect to require interface type none for >> the latter, you can use one -drive in both roles. This confuses the >> code about as much as you, dear reader, probably are by now. >> >> Because this still isn't interesting enough, there's yet another way >> to configure backends, just for floppies: set the floppy controller's >> property. Goes back to the time when floppy wasn't a separate device, >> and involves some Bad Magic. Now -global can interact with itself! >> >> Digging through all this took me an embarrassing amount of time. >> Hair, too. >> >> My patches reject some the silliest uses outright, and deprecate some >> not so silly ones that have replacements. >> >> Apply on top of my "[PATCH v2 00/58] qdev: Rework how we plug into the >> parent bus". >> > > Oof. Thank you for your work in fixing our darkest corners. I sincerely > appreciate it. > > The qdev tree ordering problems don't cause any issues for migration, do > they? This series should only change device configuration, not device state or its encoding in the migration stream. I'm not sure what you mean by "qdev tree ordering problems". Ist it commit e8c9e65816 'qom: Make "info qom-tree" show children sorted'? > (I see you already sent a PR, so whatever!) A question that might avoid a later migration debugging session is *never* "whatever"!
Re: [PATCH 04/46] macio: Tidy up error handling in macio_newworld_realize()
On Wed, Jun 24, 2020 at 06:43:02PM +0200, Markus Armbruster wrote:
> macio_newworld_realize() effectively ignores ns->gpio realization
> errors, leaking the Error object. Fortunately, macio_gpio_realize()
> can't actually fail. Tidy up.
>
> Cc: Mark Cave-Ayland
> Cc: David Gibson
> Signed-off-by: Markus Armbruster
Acked-by: David Gibson
> ---
> hw/misc/macio/macio.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
> index 7cfe357cc4..bedf10e77b 100644
> --- a/hw/misc/macio/macio.c
> +++ b/hw/misc/macio/macio.c
> @@ -329,7 +329,9 @@ static void macio_newworld_realize(PCIDevice *d, Error
> **errp)
> _abort);
> memory_region_add_subregion(>bar, 0x50,
> sysbus_mmio_get_region(sysbus_dev, 0));
> -qdev_realize(DEVICE(>gpio), BUS(>macio_bus), );
> +if (!qdev_realize(DEVICE(>gpio), BUS(>macio_bus), errp)) {
> +return;
> +}
>
> /* PMU */
> object_initialize_child(OBJECT(s), "pmu", >pmu, TYPE_VIA_PMU);
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
[PULL 03/19] acpi: move aml builder code for floppy device
From: Gerd Hoffmann
DSDT change: isa device order changes in case MI1 (ipmi) is present.
Signed-off-by: Gerd Hoffmann
Reviewed-by: Igor Mammedov
Message-Id: <20200619091905.21676-4-kra...@redhat.com>
Reviewed-by: Michael S. Tsirkin
Signed-off-by: Michael S. Tsirkin
---
hw/block/fdc.c | 83
hw/i386/acpi-build.c | 83
stubs/cmos.c | 7
stubs/Makefile.objs | 1 +
4 files changed, 91 insertions(+), 83 deletions(-)
create mode 100644 stubs/cmos.c
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index be0674e4aa..5244df6f91 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -32,6 +32,8 @@
#include "qapi/error.h"
#include "qemu/error-report.h"
#include "qemu/timer.h"
+#include "hw/i386/pc.h"
+#include "hw/acpi/aml-build.h"
#include "hw/irq.h"
#include "hw/isa/isa.h"
#include "hw/qdev-properties.h"
@@ -2768,6 +2770,85 @@ void isa_fdc_get_drive_max_chs(FloppyDriveType type,
(*maxc)--;
}
+static Aml *build_fdinfo_aml(int idx, FloppyDriveType type)
+{
+Aml *dev, *fdi;
+uint8_t maxc, maxh, maxs;
+
+isa_fdc_get_drive_max_chs(type, , , );
+
+dev = aml_device("FLP%c", 'A' + idx);
+
+aml_append(dev, aml_name_decl("_ADR", aml_int(idx)));
+
+fdi = aml_package(16);
+aml_append(fdi, aml_int(idx)); /* Drive Number */
+aml_append(fdi,
+aml_int(cmos_get_fd_drive_type(type))); /* Device Type */
+/*
+ * the values below are the limits of the drive, and are thus independent
+ * of the inserted media
+ */
+aml_append(fdi, aml_int(maxc)); /* Maximum Cylinder Number */
+aml_append(fdi, aml_int(maxs)); /* Maximum Sector Number */
+aml_append(fdi, aml_int(maxh)); /* Maximum Head Number */
+/*
+ * SeaBIOS returns the below values for int 0x13 func 0x08 regardless of
+ * the drive type, so shall we
+ */
+aml_append(fdi, aml_int(0xAF)); /* disk_specify_1 */
+aml_append(fdi, aml_int(0x02)); /* disk_specify_2 */
+aml_append(fdi, aml_int(0x25)); /* disk_motor_wait */
+aml_append(fdi, aml_int(0x02)); /* disk_sector_siz */
+aml_append(fdi, aml_int(0x12)); /* disk_eot */
+aml_append(fdi, aml_int(0x1B)); /* disk_rw_gap */
+aml_append(fdi, aml_int(0xFF)); /* disk_dtl */
+aml_append(fdi, aml_int(0x6C)); /* disk_formt_gap */
+aml_append(fdi, aml_int(0xF6)); /* disk_fill */
+aml_append(fdi, aml_int(0x0F)); /* disk_head_sttl */
+aml_append(fdi, aml_int(0x08)); /* disk_motor_strt */
+
+aml_append(dev, aml_name_decl("_FDI", fdi));
+return dev;
+}
+
+static void fdc_isa_build_aml(ISADevice *isadev, Aml *scope)
+{
+Aml *dev;
+Aml *crs;
+int i;
+
+#define ACPI_FDE_MAX_FD 4
+uint32_t fde_buf[5] = {
+0, 0, 0, 0, /* presence of floppy drives #0 - #3 */
+cpu_to_le32(2) /* tape presence (2 == never present) */
+};
+
+crs = aml_resource_template();
+aml_append(crs, aml_io(AML_DECODE16, 0x03F2, 0x03F2, 0x00, 0x04));
+aml_append(crs, aml_io(AML_DECODE16, 0x03F7, 0x03F7, 0x00, 0x01));
+aml_append(crs, aml_irq_no_flags(6));
+aml_append(crs,
+aml_dma(AML_COMPATIBILITY, AML_NOTBUSMASTER, AML_TRANSFER8, 2));
+
+dev = aml_device("FDC0");
+aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0700")));
+aml_append(dev, aml_name_decl("_CRS", crs));
+
+for (i = 0; i < MIN(MAX_FD, ACPI_FDE_MAX_FD); i++) {
+FloppyDriveType type = isa_fdc_get_drive_type(isadev, i);
+
+if (type < FLOPPY_DRIVE_TYPE_NONE) {
+fde_buf[i] = cpu_to_le32(1); /* drive present */
+aml_append(dev, build_fdinfo_aml(i, type));
+}
+}
+aml_append(dev, aml_name_decl("_FDE",
+ aml_buffer(sizeof(fde_buf), (uint8_t *)fde_buf)));
+
+aml_append(scope, dev);
+}
+
static const VMStateDescription vmstate_isa_fdc ={
.name = "fdc",
.version_id = 2,
@@ -2801,11 +2882,13 @@ static Property isa_fdc_properties[] = {
static void isabus_fdc_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
+ISADeviceClass *isa = ISA_DEVICE_CLASS(klass);
dc->realize = isabus_fdc_realize;
dc->fw_name = "fdc";
dc->reset = fdctrl_external_reset_isa;
dc->vmsd = _isa_fdc;
+isa->build_aml = fdc_isa_build_aml;
device_class_set_props(dc, isa_fdc_properties);
set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
}
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 900f786d08..45297d9a90 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -938,85 +938,6 @@ static void build_hpet_aml(Aml *table)
aml_append(table, scope);
}
-static Aml *build_fdinfo_aml(int idx, FloppyDriveType type)
-{
-Aml *dev, *fdi;
-uint8_t maxc, maxh, maxs;
-
-isa_fdc_get_drive_max_chs(type, , , );
-
-dev = aml_device("FLP%c", 'A' + idx);
-
-aml_append(dev, aml_name_decl("_ADR", aml_int(idx)));
-
-fdi
[PULL 05/19] floppy: move cmos_get_fd_drive_type() from pc
From: Gerd Hoffmann
Signed-off-by: Gerd Hoffmann
Reviewed-by: Philippe Mathieu-Daudé
Acked-by: John Snow
Message-Id: <20200619091905.21676-6-kra...@redhat.com>
Reviewed-by: Michael S. Tsirkin
Signed-off-by: Michael S. Tsirkin
---
include/hw/block/fdc.h | 1 +
include/hw/i386/pc.h | 1 -
hw/block/fdc.c | 26 +-
hw/i386/pc.c | 25 -
4 files changed, 26 insertions(+), 27 deletions(-)
diff --git a/include/hw/block/fdc.h b/include/hw/block/fdc.h
index 5d71cf9722..479cebc0a3 100644
--- a/include/hw/block/fdc.h
+++ b/include/hw/block/fdc.h
@@ -16,5 +16,6 @@ void sun4m_fdctrl_init(qemu_irq irq, hwaddr io_base,
DriveInfo **fds, qemu_irq *fdc_tc);
FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i);
+int cmos_get_fd_drive_type(FloppyDriveType fd0);
#endif
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index e6135c34d6..dce1273c7d 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -178,7 +178,6 @@ typedef void (*cpu_set_smm_t)(int smm, void *arg);
void pc_i8259_create(ISABus *isa_bus, qemu_irq *i8259_irqs);
ISADevice *pc_find_fdc0(void);
-int cmos_get_fd_drive_type(FloppyDriveType fd0);
/* port92.c */
#define PORT92_A20_LINE "a20"
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index f1da83f08e..4f0921298b 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -32,7 +32,6 @@
#include "qapi/error.h"
#include "qemu/error-report.h"
#include "qemu/timer.h"
-#include "hw/i386/pc.h"
#include "hw/acpi/aml-build.h"
#include "hw/irq.h"
#include "hw/isa/isa.h"
@@ -2812,6 +2811,31 @@ static Aml *build_fdinfo_aml(int idx, FloppyDriveType
type)
return dev;
}
+int cmos_get_fd_drive_type(FloppyDriveType fd0)
+{
+int val;
+
+switch (fd0) {
+case FLOPPY_DRIVE_TYPE_144:
+/* 1.44 Mb 3"5 drive */
+val = 4;
+break;
+case FLOPPY_DRIVE_TYPE_288:
+/* 2.88 Mb 3"5 drive */
+val = 5;
+break;
+case FLOPPY_DRIVE_TYPE_120:
+/* 1.2 Mb 5"5 drive */
+val = 2;
+break;
+case FLOPPY_DRIVE_TYPE_NONE:
+default:
+val = 0;
+break;
+}
+return val;
+}
+
static void fdc_isa_build_aml(ISADevice *isadev, Aml *scope)
{
Aml *dev;
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index d103b8c0ab..e78e32b41b 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -386,31 +386,6 @@ static uint64_t ioportF0_read(void *opaque, hwaddr addr,
unsigned size)
#define REG_EQUIPMENT_BYTE 0x14
-int cmos_get_fd_drive_type(FloppyDriveType fd0)
-{
-int val;
-
-switch (fd0) {
-case FLOPPY_DRIVE_TYPE_144:
-/* 1.44 Mb 3"5 drive */
-val = 4;
-break;
-case FLOPPY_DRIVE_TYPE_288:
-/* 2.88 Mb 3"5 drive */
-val = 5;
-break;
-case FLOPPY_DRIVE_TYPE_120:
-/* 1.2 Mb 5"5 drive */
-val = 2;
-break;
-case FLOPPY_DRIVE_TYPE_NONE:
-default:
-val = 0;
-break;
-}
-return val;
-}
-
static void cmos_init_hd(ISADevice *s, int type_ofs, int info_ofs,
int16_t cylinders, int8_t heads, int8_t sectors)
{
--
MST
[PULL 04/19] floppy: make isa_fdc_get_drive_max_chs static
From: Gerd Hoffmann
acpi aml generator needs this, but it is in floppy code now
so we can make the function static.
Signed-off-by: Gerd Hoffmann
Reviewed-by: Igor Mammedov
Reviewed-by: Philippe Mathieu-Daudé
Acked-by: John Snow
Message-Id: <20200619091905.21676-5-kra...@redhat.com>
Reviewed-by: Michael S. Tsirkin
Signed-off-by: Michael S. Tsirkin
---
include/hw/block/fdc.h | 2 --
hw/block/fdc.c | 4 ++--
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/include/hw/block/fdc.h b/include/hw/block/fdc.h
index c15ff4c623..5d71cf9722 100644
--- a/include/hw/block/fdc.h
+++ b/include/hw/block/fdc.h
@@ -16,7 +16,5 @@ void sun4m_fdctrl_init(qemu_irq irq, hwaddr io_base,
DriveInfo **fds, qemu_irq *fdc_tc);
FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i);
-void isa_fdc_get_drive_max_chs(FloppyDriveType type,
- uint8_t *maxc, uint8_t *maxh, uint8_t *maxs);
#endif
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 5244df6f91..f1da83f08e 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2747,8 +2747,8 @@ FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc,
int i)
return isa->state.drives[i].drive;
}
-void isa_fdc_get_drive_max_chs(FloppyDriveType type,
- uint8_t *maxc, uint8_t *maxh, uint8_t *maxs)
+static void isa_fdc_get_drive_max_chs(FloppyDriveType type, uint8_t *maxc,
+ uint8_t *maxh, uint8_t *maxs)
{
const FDFormat *fdf;
--
MST
Re: [PATCH 05/46] virtio-crypto-pci: Tidy up virtio_crypto_pci_realize()
On 6/24/20 11:43 AM, Markus Armbruster wrote:
virtio_crypto_pci_realize() continues after realization of its
"virtio-crypto-device" fails. Only an object_property_set_link()
follows; looks harmless to me. Tidy up anyway: return after failure,
just like virtio_rng_pci_realize() does.
Cc: "Gonglei (Arei)"
Cc: Michael S. Tsirkin
Signed-off-by: Markus Armbruster
---
hw/virtio/virtio-crypto-pci.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Reviewed-by: Eric Blake
diff --git a/hw/virtio/virtio-crypto-pci.c b/hw/virtio/virtio-crypto-pci.c
index 72be531c95..0755722288 100644
--- a/hw/virtio/virtio-crypto-pci.c
+++ b/hw/virtio/virtio-crypto-pci.c
@@ -54,7 +54,9 @@ static void virtio_crypto_pci_realize(VirtIOPCIProxy
*vpci_dev, Error **errp)
}
virtio_pci_force_virtio_1(vpci_dev);
-qdev_realize(vdev, BUS(_dev->bus), errp);
+if (!qdev_realize(vdev, BUS(_dev->bus), errp)) {
+return;
+}
object_property_set_link(OBJECT(vcrypto),
OBJECT(vcrypto->vdev.conf.cryptodev), "cryptodev",
NULL);
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 46/46] hmp: Ignore Error objects where the return value suffices
On 6/24/20 11:43 AM, Markus Armbruster wrote: qdev_print_props() receives and throws away Error objects just to check for object_property_get_str() and object_property_print() failure. Unnecessary, both return suitable values, so use those instead. Signed-off-by: Markus Armbruster --- qdev-monitor.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 04/46] macio: Tidy up error handling in macio_newworld_realize()
On 6/24/20 11:43 AM, Markus Armbruster wrote:
macio_newworld_realize() effectively ignores ns->gpio realization
errors, leaking the Error object. Fortunately, macio_gpio_realize()
can't actually fail. Tidy up.
Cc: Mark Cave-Ayland
Cc: David Gibson
Signed-off-by: Markus Armbruster
---
hw/misc/macio/macio.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Reviewed-by: Eric Blake
diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index 7cfe357cc4..bedf10e77b 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -329,7 +329,9 @@ static void macio_newworld_realize(PCIDevice *d, Error
**errp)
_abort);
memory_region_add_subregion(>bar, 0x50,
sysbus_mmio_get_region(sysbus_dev, 0));
-qdev_realize(DEVICE(>gpio), BUS(>macio_bus), );
+if (!qdev_realize(DEVICE(>gpio), BUS(>macio_bus), errp)) {
+return;
+}
/* PMU */
object_initialize_child(OBJECT(s), "pmu", >pmu, TYPE_VIA_PMU);
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 45/46] qdev: Ignore Error objects where the return value suffices
On 6/24/20 11:43 AM, Markus Armbruster wrote: Signed-off-by: Markus Armbruster --- hw/core/qdev-properties.c | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 44/46] qemu-img: Ignore Error objects where the return value suffices
On 6/24/20 11:43 AM, Markus Armbruster wrote: Signed-off-by: Markus Armbruster --- qemu-img.c | 8 ++-- 1 file changed, 2 insertions(+), 6 deletions(-) Reviewed-by: Eric Blake Another case where you already wrote the followup patch for something I spotted earlier in the series ;) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 43/46] qdev: Smooth error checking manually
On 6/24/20 11:43 AM, Markus Armbruster wrote:
When foo(..., ) is followed by error_propagate(errp, err), we can
often just as well do foo(..., errp). The previous commit did that
for simple cases with Coccinelle. Do it for one more manually.
Signed-off-by: Markus Armbruster
---
hw/block/fdc.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
@@ -2566,11 +2566,9 @@ static void fdctrl_connect_drives(FDCtrl *fdctrl,
DeviceState *fdc_dev,
blk_ref(blk);
blk_detach_dev(blk, fdc_dev);
fdctrl->qdev_for_drives[i].blk = NULL;
-qdev_prop_set_drive_err(dev, "drive", blk, _err);
+ok = qdev_prop_set_drive_err(dev, "drive", blk, errp);
blk_unref(blk);
Perhaps some glib g_auto* magic could make this even easier (to mark a
variable to be blk_unref'd when it goes out of scope). But for now,
your pattern is fine.
-
-if (local_err) {
-error_propagate(errp, local_err);
+if (!ok) {
return;
}
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 42/46] qdev: Smooth error checking with Coccinelle
On 6/24/20 11:43 AM, Markus Armbruster wrote:
The previous commit enables conversion of
qdev_prop_set_drive_err(..., );
if (err) {
...
}
to
if (!qdev_prop_set_drive_err(..., errp)) {
...
}
Eliminate error_propagate() that are now unnecessary. Delete @err
that are now unused. Tidy up line breaks and whitespace.
Signed-off-by: Markus Armbruster
---
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 41/46] qdev: Make functions taking Error ** return bool, not void
On 6/24/20 11:43 AM, Markus Armbruster wrote: See recent commit "error: Document Error API usage rules" for rationale. Signed-off-by: Markus Armbruster --- Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 40/46] qom: Make functions taking Error ** return bool, not 0/-1
On 6/24/20 11:43 AM, Markus Armbruster wrote: Just for consistency. Also fix the example in object_set_props()'s documentation. Signed-off-by: Markus Armbruster --- include/qom/object.h | 28 +++- qom/object.c | 14 +++--- 2 files changed, 18 insertions(+), 24 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 38/46] qom: Smooth error checking with Coccinelle
On 6/24/20 11:43 AM, Markus Armbruster wrote:
The previous commit enables conversion of
foo(..., );
if (err) {
...
}
to
if (!foo(..., errp)) {
...
}
for QOM functions that now return true / false on success / error.
Coccinelle script:
Signed-off-by: Markus Armbruster
---
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 39/46] qom: Smooth error checking manually
On 6/24/20 11:43 AM, Markus Armbruster wrote: When foo(..., ) is followed by error_propagate(errp, err), we can often just as well do foo(..., errp). The previous commit did that for simple cases with Coccinelle. Do it for a few more manually. Signed-off-by: Markus Armbruster --- Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 37/46] qom: Make functions taking Error ** return bool, not void
On 6/24/20 11:43 AM, Markus Armbruster wrote: See recent commit "error: Document Error API usage rules" for rationale. Signed-off-by: Markus Armbruster --- @@ -524,25 +527,29 @@ void object_initialize(void *data, size_t size, const char *typename) object_initialize_with_type(data, size, type); } -void object_initialize_child_with_props(Object *parentobj, +bool object_initialize_child_with_props(Object *parentobj, const char *propname, void *childobj, size_t size, const char *type, Error **errp, ...) Is it worth tweaking indentation while here? Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 36/46] qom: Put name parameter before value / visitor parameter
On 6/24/20 11:43 AM, Markus Armbruster wrote:
The object_property_set_FOO() setters take property name and value in
an unusual order:
void object_property_set_FOO(Object *obj, FOO_TYPE value,
const char *name, Error **errp)
Having to pass value before name feels grating. Swap them.
Same for object_property_set(), object_property_get(), and
object_property_parse().
Convert callers with this Coccinelle script:
@@
identifier fun = {object_property_get, object_property_parse,
object_property_set_str, object_property_set_link, object_property_set_bool,
object_property_set_int, object_property_set_uint, object_property_set,
object_property_set_qobject};
expression obj, v, name, errp;
@@
-fun(obj, v, name, errp)
+fun(obj, name, v, errp)
Chokes on hw/arm/musicpal.c's lcd_refresh() with the unhelpful error
message "no position information". Convert that one manually.
Fails to convert hw/arm/armsse.c, because Coccinelle gets confused by
ARMSSE being used both as typedef and function-like macro there.
Convert manually.
Fails to convert hw/rx/rx-gdbsim.c, because Coccinelle gets confused
by RXCPU being used both as typedef and function-like macro there.
Convert manually. Convert that one manually. The other files using
RXCPU that way don't need conversion.
Signed-off-by: Markus Armbruster
---
136 files changed, 702 insertions(+), 729 deletions(-)
Big but mechanical.
Reviewed-by: Eric Blake
This one might be a semantic conflict magnet with patches written in the
meantime; I guess the trick is to check that 'git grep' finds as many
calls to any of the functions listed as conversions.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 35/46] qom: Use return values to check for error where that's simpler
On 6/24/20 11:43 AM, Markus Armbruster wrote: When using the Error object to check for error, we need to receive it into a local variable, then propagate() it to @errp. Using the return value permits allows receiving it straight to @errp. Signed-off-by: Markus Armbruster --- qom/object.c | 10 -- 1 file changed, 4 insertions(+), 6 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 34/46] qom: Don't handle impossible object_property_get_link() failure
On 6/24/20 11:43 AM, Markus Armbruster wrote: Don't handle object_property_get_link() failure that can't happen unless the programmer screwed up, pass _abort. Signed-off-by: Markus Armbruster --- Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 32/46] qom: Rename qdev_get_type() to object_get_type()
On 6/24/20 11:43 AM, Markus Armbruster wrote: Commit 2f262e06f0 lifted qdev_get_type() from qdev to object without renaming it accordingly. Do that now. Signed-off-by: Markus Armbruster --- qom/object.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 33/46] qom: Crash more nicely on object_property_get_link() failure
On 6/24/20 11:43 AM, Markus Armbruster wrote:
Pass _abort instead of NULL where the returned value is
dereferenced or asserted to be non-null.
Signed-off-by: Markus Armbruster
---
@@ -63,8 +64,8 @@ hwaddr platform_bus_get_mmio_addr(PlatformBusDevice *pbus,
SysBusDevice *sbdev,
return -1;
}
-parent_mr = object_property_get_link(OBJECT(sbdev_mr), "container", NULL);
-
+parent_mr = object_property_get_link(OBJECT(sbdev_mr), "container",
+ _abort);
assert(parent_mr);
Do we still need to keep the assert?
+++ b/hw/ppc/spapr_pci_nvlink2.c
@@ -141,9 +141,10 @@ static void spapr_phb_pci_collect_nvgpu(PCIBus *bus,
PCIDevice *pdev,
if (tgt) {
Error *local_err = NULL;
SpaprPhbPciNvGpuConfig *nvgpus = opaque;
-Object *mr_gpu = object_property_get_link(po, "nvlink2-mr[0]", NULL);
+Object *mr_gpu = object_property_get_link(po, "nvlink2-mr[0]",
+ _abort);
Object *mr_npu = object_property_get_link(po, "nvlink2-atsd-mr[0]",
- NULL);
+ _abort);
g_assert(mr_gpu || mr_npu);
Likewise.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 31/46] qom: Use error_reportf_err() instead of g_printerr() in examples
On 6/24/20 11:43 AM, Markus Armbruster wrote: Signed-off-by: Markus Armbruster --- include/qom/object.h | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 29/46] acpi: Avoid unnecessary error_propagate() after error_setg()
On 6/24/20 11:43 AM, Markus Armbruster wrote: The commit before previous enables another round of the transformation from recent commit "error: Avoid unnecessary error_propagate() after error_setg()". Signed-off-by: Markus Armbruster --- hw/acpi/core.c | 15 ++- 1 file changed, 6 insertions(+), 9 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 28/46] block/parallels: Simplify parallels_open() after previous commit
On 6/24/20 11:43 AM, Markus Armbruster wrote: Signed-off-by: Markus Armbruster --- block/parallels.c | 7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 27/46] qapi: Purge error_propagate() from QAPI core
On 6/24/20 11:43 AM, Markus Armbruster wrote: Signed-off-by: Markus Armbruster --- qapi/qapi-visit-core.c | 40 +++- 1 file changed, 19 insertions(+), 21 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 26/46] qapi: Smooth another visitor error checking pattern
On 6/24/20 11:43 AM, Markus Armbruster wrote:
Convert
visit_type_FOO(v, ..., , );
...
if (err) {
...
}
to
visit_type_FOO(v, ..., , errp);
...
if (!ptr) {
...
}
for functions that set @ptr to non-null / null on success / error.
Eliminate error_propagate() that are now unnecessary. Delete @err
that are now unused.
Signed-off-by: Markus Armbruster
---
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 25/46] qapi: Smooth visitor error checking in generated code
On 6/24/20 11:43 AM, Markus Armbruster wrote: Use visitor functions' return values to check for failure. Eliminate error_propagate() that are now unnecessary. Delete @err that are now unused. Signed-off-by: Markus Armbruster --- docs/devel/qapi-code-gen.txt | 60 ++-- scripts/qapi/commands.py | 22 ++--- scripts/qapi/visit.py| 57 ++ 3 files changed, 55 insertions(+), 84 deletions(-) Aha - you resolved some of my comments in 22/46. Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 24/46] qapi: Smooth error checking manually
On 6/24/20 11:43 AM, Markus Armbruster wrote: When foo(..., ) is followed by error_propagate(errp, err), we can often just as well do foo(..., errp). The previous commit did that for simple cases with Coccinelle. Do it for a few more manually. Signed-off-by: Markus Armbruster --- Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 23/46] qapi: Smooth error checking with Coccinelle
On 6/24/20 11:43 AM, Markus Armbruster wrote:
The previous commit enables conversion of
visit_foo(..., );
if (err) {
...
}
to
if (!visit_foo(..., errp)) {
...
}
for visitor functions that now return true / false on success / error.
Coccinelle script:
@@
identifier fun =~
"check_list|input_type_enum|lv_start_struct|lv_type_bool|lv_type_int64|lv_type_str|lv_type_uint64|output_type_enum|parse_type_bool|parse_type_int64|parse_type_null|parse_type_number|parse_type_size|parse_type_str|parse_type_uint64|print_type_bool|print_type_int64|print_type_null|print_type_number|print_type_size|print_type_str|print_type_uint64|qapi_clone_start_alternate|qapi_clone_start_list|qapi_clone_start_struct|qapi_clone_type_bool|qapi_clone_type_int64|qapi_clone_type_null|qapi_clone_type_number|qapi_clone_type_str|qapi_clone_type_uint64|qapi_dealloc_start_list|qapi_dealloc_start_struct|qapi_dealloc_type_anything|qapi_dealloc_type_bool|qapi_dealloc_type_int64|qapi_dealloc_type_null|qapi_dealloc_type_number|qapi_dealloc_type_str|qapi_dealloc_type_uint64|qobject_input_check_list|qobject_input_check_struct|qobject_input_start_alternate|qobject_input_start_list|qobject_input_start_struct|qobject_input_type_any|qobject_input_type_bool|qobject_input_type_bool_keyval|qobject_input_type_int64|qobject_input_type_int64_keyval|qobject_input_type_null|qobject_input_type_number|qobject_input_type_number_keyval|qobject_input_type_size_keyval|qobject_input_type_str|qobject_input_type_str_keyval|qobject_input_type_uint64|qobject_input_type_uint64_keyval|qobject_output_start_list|qobject_output_start_struct|qobject_output_type_any|qobject_output_type_bool|qobject_output_type_int64|qobject_output_type_null|qobject_output_type_number|qobject_output_type_str|qobject_output_type_uint64|start_list|visit_check_list|visit_check_struct|visit_start_alternate|visit_start_list|visit_start_struct|visit_type_.*";
Long line. Does coccinelle understand \-newline wrapping?
expression list args, args2;
typedef Error;
Error *err;
identifier errp;
@@
- fun(args, , args2);
- if (err) {
+ if (!fun(args, errp, args2)) {
... when != err
- error_propagate(errp, err);
...
}
@@
identifier fun =~
"check_list|input_type_enum|lv_start_struct|lv_type_bool|lv_type_int64|lv_type_str|lv_type_uint64|output_type_enum|parse_type_bool|parse_type_int64|parse_type_null|parse_type_number|parse_type_size|parse_type_str|parse_type_uint64|print_type_bool|print_type_int64|print_type_null|print_type_number|print_type_size|print_type_str|print_type_uint64|qapi_clone_start_alternate|qapi_clone_start_list|qapi_clone_start_struct|qapi_clone_type_bool|qapi_clone_type_int64|qapi_clone_type_null|qapi_clone_type_number|qapi_clone_type_str|qapi_clone_type_uint64|qapi_dealloc_start_list|qapi_dealloc_start_struct|qapi_dealloc_type_anything|qapi_dealloc_type_bool|qapi_dealloc_type_int64|qapi_dealloc_type_null|qapi_dealloc_type_number|qapi_dealloc_type_str|qapi_dealloc_type_uint64|qobject_input_check_list|qobject_input_check_struct|qobject_input_start_alternate|qobject_input_start_list|qobject_input_start_struct|qobject_input_type_any|qobject_input_type_bool|qobject_input_type_bool_keyval|qobject_input_type_int64|qobject_input_type_int64_keyval|qobject_input_type_null|qobject_input_type_number|qobject_input_type_number_keyval|qobject_input_type_size_keyval|qobject_input_type_str|qobject_input_type_str_keyval|qobject_input_type_uint64|qobject_input_type_uint64_keyval|qobject_output_start_list|qobject_output_start_struct|qobject_output_type_any|qobject_output_type_bool|qobject_output_type_int64|qobject_output_type_null|qobject_output_type_number|qobject_output_type_str|qobject_output_type_uint64|start_list|visit_check_list|visit_check_struct|visit_start_alternate|visit_start_list|visit_start_struct|visit_type_.*";
The same list twice. Is there a way to write it only once, then refer
to it by reference in the two halves of the script?
46 files changed, 95 insertions(+), 331 deletions(-)
Nice to see the size reduction.
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 22/46] qapi: Make visitor functions taking Error ** return bool, not void
On 6/24/20 11:43 AM, Markus Armbruster wrote:
See recent commit "error: Document Error API usage rules" for
rationale.
Signed-off-by: Markus Armbruster
---
docs/devel/qapi-code-gen.txt | 51 +--
include/qapi/clone-visitor.h | 8 +-
include/qapi/visitor-impl.h | 26 +++---
include/qapi/visitor.h| 102 -
audio/audio_legacy.c | 15 ++--
qapi/opts-visitor.c | 58 +++-
qapi/qapi-clone-visitor.c | 33 ---
qapi/qapi-dealloc-visitor.c | 27 --
qapi/qapi-visit-core.c| 165 ++
qapi/qobject-input-visitor.c | 109 +-
qapi/qobject-output-visitor.c | 27 --
qapi/string-input-visitor.c | 62 +++--
qapi/string-output-visitor.c | 32 ---
scripts/qapi/visit.py | 58 +---
14 files changed, 435 insertions(+), 338 deletions(-)
Hefty, but I don't see a sane way to split it further.
diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index a7794ef658..9bfc57063c 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
-void visit_type_UserDefOne_members(Visitor *v, UserDefOne *obj, Error
**errp)
+bool visit_type_UserDefOne_members(Visitor *v, UserDefOne *obj, Error
**errp)
{
Error *err = NULL;
-visit_type_int(v, "integer", >integer, );
-if (err) {
-goto out;
+if (!visit_type_int(v, "integer", >integer, errp)) {
+return false;
}
if (visit_optional(v, "string", >has_string)) {
-visit_type_str(v, "string", >string, );
-if (err) {
-goto out;
+if (!visit_type_str(v, "string", >string, errp)) {
+return false;
}
}
Is this worth compressing two 'if's into one:
if (visit_optional(...) &&
!visit_type_str(...)) {
return false;
}
-
-out:
error_propagate(errp, err);
+return !err;
Now that 'err' is never anything but NULL, why aren't you dropping the
error_propagate() and merely using 'return true;'?
}
-void visit_type_UserDefOne(Visitor *v, const char *name, UserDefOne **obj, Error **errp)
+bool visit_type_UserDefOne(Visitor *v, const char *name, UserDefOne **obj,
Error **errp)
{
Error *err = NULL;
-visit_start_struct(v, name, (void **)obj, sizeof(UserDefOne), );
-if (err) {
-goto out;
+if (!visit_start_struct(v, name, (void **)obj, sizeof(UserDefOne),
errp)) {
+return false;
}
if (!*obj) {
/* incomplete */
@@ -1461,19 +1457,18 @@ Example:
Adding context:
assert(visit_is_dealloc(v));
goto out_obj;
}
visit_type_UserDefOne_members(v, *obj, );
if (err) {
goto out_obj;
Should this be:
if (!visit_type_UserDefOne_members(v, *obj, )) {
goto out_obj;
}
visit_check_struct(v, );
out_obj:
visit_end_struct(v, (void **)obj);
if (err && visit_is_input(v)) {
qapi_free_UserDefOne(*obj);
*obj = NULL;
}
-out:
error_propagate(errp, err);
+return !err;
Here, err is still used by out_obj:, so this one is fine.
}
-void visit_type_UserDefOneList(Visitor *v, const char *name, UserDefOneList **obj, Error **errp)
+bool visit_type_UserDefOneList(Visitor *v, const char *name,
UserDefOneList **obj, Error **errp)
{
Error *err = NULL;
UserDefOneList *tail;
size_t size = sizeof(**obj);
-visit_start_list(v, name, (GenericList **)obj, size, );
-if (err) {
-goto out;
+if (!visit_start_list(v, name, (GenericList **)obj, size, errp)) {
+return false;
}
for (tail = *obj; tail;
@@ -1492,21 +1487,19 @@ Example:
Adding context:
tail = (UserDefOneList *)visit_next_list(v, (GenericList *)tail,
size)) {
visit_type_UserDefOne(v, NULL, >value, );
if (err) {
break;
}
Should this be:
if (visit_type_UserDefOne(...)) {
break;
}
if (!err) {
visit_check_list(v, );
}
visit_end_list(v, (void **)obj);
if (err && visit_is_input(v)) {
qapi_free_UserDefOneList(*obj);
*obj = NULL;
}
-out:
error_propagate(errp, err);
+return !err;
}
Again, err is still used, so this one is fine.
-void visit_type_q_obj_my_command_arg_members(Visitor *v, q_obj_my_command_arg *obj, Error **errp)
+bool visit_type_q_obj_my_command_arg_members(Visitor *v,
q_obj_my_command_arg *obj, Error **errp)
{
Error *err = NULL;
-visit_type_UserDefOneList(v, "arg1", >arg1, );
-if (err)
Re: [PATCH 30/46] s390x/pci: Fix harmless mistake in zpci's property fid's setter
On 6/24/20 12:43 PM, Markus Armbruster wrote:
s390_pci_set_fid() sets zpci->fid_defined to true even when
visit_type_uint32() failed. Reproducer: "-device zpci,fid=junk".
Harmless in practice, because qdev_device_add() then fails, throwing
away @zpci. Fix it anyway.
Cc: Matthew Rosato
Cc: Cornelia Huck
Signed-off-by: Markus Armbruster
---
hw/s390x/s390-pci-bus.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index be8535304e..2e0eab1c69 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -1265,7 +1265,9 @@ static void s390_pci_set_fid(Object *obj, Visitor *v,
const char *name,
return;
}
-visit_type_uint32(v, name, ptr, errp);
+if (!visit_type_uint32(v, name, ptr, errp)) {
+return;
+}
zpci->fid_defined = true;
}
Assuming no major overhaul of patch #22:
Reviewed-by: Matthew Rosato
Re: [PATCH 21/46] hmp: Eliminate a variable in hmp_migrate_set_parameter()
On 6/24/20 11:43 AM, Markus Armbruster wrote: Signed-off-by: Markus Armbruster --- monitor/hmp-cmds.c | 8 ++-- 1 file changed, 2 insertions(+), 6 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 20/46] block: Avoid error accumulation in bdrv_img_create()
On 6/24/20 11:43 AM, Markus Armbruster wrote: When creating an image fails because the format doesn't support option "backing_file" or "backing_fmt", bdrv_img_create() first has qemu_opt_set() put a generic error into @local_err, then puts the real error into @errp with error_setg(), and then propagates the former to the latter, which throws away the generic error. A bit complicated, but works. Hmm - may interact with my series to deprecate -b without -F. We'll deal with the fallout based on whatever lands first. Not that qemu_opt_set() returns a useful value, we can simply ignore s/Not/Now/ the generic error instead. Signed-off-by: Markus Armbruster --- block.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) Aha - you fixed 2 of the 4 cases that I noticed in 17/46. Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 19/46] block: Avoid unnecessary error_propagate() after error_setg()
On 6/24/20 11:43 AM, Markus Armbruster wrote: The previous commit enables another round of the transformation from recent commit "error: Avoid unnecessary error_propagate() after error_setg()". Signed-off-by: Markus Armbruster --- block/quorum.c | 16 +++- block/replication.c | 12 +--- block/vxhs.c| 10 -- 3 files changed, 16 insertions(+), 22 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 18/46] qemu-option: Smooth error checking manually
On 6/24/20 11:43 AM, Markus Armbruster wrote:
When foo(..., ) is followed by error_propagate(errp, err), we can
often just as well do foo(..., errp). The previous commit did that
for simple cases with Coccinelle. Do it for a few more manually.
Signed-off-by: Markus Armbruster
---
block.c | 2 +-
block/gluster.c | 8
block/parallels.c | 2 +-
block/quorum.c | 2 +-
block/replication.c | 2 +-
block/vxhs.c| 4 ++--
hw/net/virtio-net.c | 4 ++--
7 files changed, 12 insertions(+), 12 deletions(-)
+++ b/block/gluster.c
@@ -523,7 +523,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster
*gconf,
/* create opts info from runtime_json_opts list */
opts = qemu_opts_create(_json_opts, NULL, 0, _abort);
-if (!qemu_opts_absorb_qdict(opts, options, _err)) {
+if (!qemu_opts_absorb_qdict(opts, options, errp)) {
goto out;
}
This function also has a few error_setg(_err) that could be
cleaned up to error_setg(errp); but the ones that use
error_append_hint() immediately after (and thus the
error_propagate(errp, local_err) in the out: label) still have to
remain, until we have Vladimir's macro in place.
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 17/46] qemu-option: Smooth error checking with Coccinelle
On 6/24/20 11:43 AM, Markus Armbruster wrote:
The previous commit enables conversion of
foo(..., );
if (err) {
...
}
to
if (!foo(..., )) {
...
}
for QemuOpts functions that now return true / false on success /
error. Coccinelle script:
Eliminate error_propagate() that are now unnecessary. Delete @err
that are now unused. Tidy up line breaks and whitespace.
Signed-off-by: Markus Armbruster
---
32 files changed, 70 insertions(+), 192 deletions(-)
Another decent chunk of cleanups.
diff --git a/block.c b/block.c
index 30a72bc4c2..77e85f13db 100644
--- a/block.c
+++ b/block.c
@@ -6091,8 +6086,8 @@ void bdrv_img_create(const char *filename, const char
*fmt,
}
if (base_filename) {
-qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename, _err);
-if (local_err) {
+if (!qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename,
+ _err)) {
error_setg(errp, "Backing file not supported for file format
'%s'",
fmt);
Pre-existing - it is odd that we collect a message into local_err, then
write something else into errp; the out: label does
error_propagate(errp, local_err) which ensures there is no leak but
discards the original err. You could pass NULL instead. But as it is
pre-existing, passing NULL should be a separate patch.
goto out;
@@ -6100,8 +6095,7 @@ void bdrv_img_create(const char *filename, const char
*fmt,
}
if (base_fmt) {
-qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt, _err);
-if (local_err) {
+if (!qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt, _err)) {
error_setg(errp, "Backing file format not supported for file "
"format '%s'", fmt);
Ditto.
+++ b/qemu-img.c
@@ -467,8 +467,8 @@ static int add_old_style_options(const char *fmt, QemuOpts
*opts,
Error *err = NULL;
if (base_filename) {
-qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename, );
-if (err) {
+if (!qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename,
+ )) {
error_report("Backing file not supported for file format '%s'",
fmt);
error_free(err);
@@ -476,8 +476,7 @@ static int add_old_style_options(const char *fmt, QemuOpts
*opts,
}
}
if (base_fmt) {
-qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt, );
-if (err) {
+if (!qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt, )) {
error_report("Backing file format not supported for file "
"format '%s'", fmt);
error_free(err);
Ditto.
But the conversion here is sane.
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 16/46] qemu-option: Make functions taking Error ** return bool, not void
On 6/24/20 11:43 AM, Markus Armbruster wrote:
See recent commit "error: Document Error API usage rules" for
rationale.
Signed-off-by: Markus Armbruster
---
include/qemu/option.h | 16
blockdev.c| 5 ++-
util/qemu-option.c| 92 +--
3 files changed, 64 insertions(+), 49 deletions(-)
-static void qemu_opts_from_qdict_entry(QemuOpts *opts,
+static bool qemu_opts_from_qdict_entry(QemuOpts *opts,
const QDictEntry *entry,
Error **errp)
{
const char *key = qdict_entry_key(entry);
QObject *obj = qdict_entry_value(entry);
-char buf[32], *tmp = NULL;
+char buf[32];
+g_autofree char *tmp = NULL;
A bit fancier than a straight mechanical conversion here, but it works.
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 15/46] qemu-option: Tidy up opt_set() not to free arguments on failure
On 6/24/20 11:43 AM, Markus Armbruster wrote: opt_set() frees its argument @value on failure. Slightly unclean; functions ideally do nothing on failure. To tidy this up, move opt_create() from opt_set() into its callers, along with the cleanup. Signed-off-by: Markus Armbruster --- util/qemu-option.c | 33 ++--- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/util/qemu-option.c b/util/qemu-option.c index 3cdf0c0800..14946e81f2 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -519,36 +519,39 @@ static QemuOpt *opt_create(QemuOpts *opts, const char *name, char *value, return opt; } Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 14/46] qemu-option: Factor out helper opt_create()
On 6/24/20 11:43 AM, Markus Armbruster wrote: There is just one use so far. The next commit will add more. Signed-off-by: Markus Armbruster --- util/qemu-option.c | 27 ++- 1 file changed, 18 insertions(+), 9 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v10 1/9] error: auto propagated local_err
On Wed, 24 Jun 2020 18:53:05 +0200 Markus Armbruster wrote: > Greg Kurz writes: > > > On Mon, 15 Jun 2020 07:21:03 +0200 > > Markus Armbruster wrote: > > > >> Greg Kurz writes: > >> > >> > On Tue, 17 Mar 2020 18:16:17 +0300 > >> > Vladimir Sementsov-Ogievskiy wrote: > >> > > >> >> Introduce a new ERRP_AUTO_PROPAGATE macro, to be used at start of > >> >> functions with an errp OUT parameter. > >> >> > >> >> It has three goals: > >> >> > >> >> 1. Fix issue with error_fatal and error_prepend/error_append_hint: user > >> >> can't see this additional information, because exit() happens in > >> >> error_setg earlier than information is added. [Reported by Greg Kurz] > >> >> > >> > > >> > I have more of these coming and I'd really like to use > >> > ERRP_AUTO_PROPAGATE. > >> > > >> > It seems we have a consensus on the macro itself but this series is gated > >> > by the conversion of the existing code base. > >> > > >> > What about merging this patch separately so that people can start using > >> > it at least ? > >> > >> Please give me a few more days to finish the work I feel should go in > >> before the conversion. With any luck, Vladimir can then rebase / > >> recreate the conversion easily, and you can finally use the macro for > >> your own work. > >> > > > > Sure. Thanks. > > Just posted "[PATCH 00/46] Less clumsy error checking". The sheer size > of the thing and the length of its dependency chain explains why it took > me so long. I feel bad about delaying you all the same. Apologies! > No problem. This series of yours is impressive. Putting an end to the highjacking of the Error ** argument is really a beneficial move. > I hope we can converge quickly enough to get Vladimir's work on top > ready in time for the soft freeze. > I'll find some cycles for reviewing. Cheers, -- Greg
Re: [PATCH v5 0/7] Introduce 'yank' oob qmp command to recover from hanging qemu
On Tue, 23 Jun 2020 16:42:30 +0200 Lukas Straub wrote: > Hello Everyone, > In many cases, if qemu has a network connection (qmp, migration, chardev, > etc.) > to some other server and that server dies or hangs, qemu hangs too. > These patches introduce the new 'yank' out-of-band qmp command to recover from > these kinds of hangs. The different subsystems register callbacks which get > executed with the yank command. For example the callback can shutdown() a > socket. This is intended for the colo use-case, but it can be used for other > things too of course. > > Regards, > Lukas Straub > > v5: > -move yank.c to util/ > -move yank.h to include/qemu/ > -add license to yank.h > -use const char* > -nbd: use atomic_store_release and atomic_load_aqcuire > -io-channel: ensure thread-safety and document it > -add myself as maintainer for yank > > v4: > -fix build errors... > > v3: > -don't touch softmmu/vl.c, use __contructor__ attribute instead (Paolo > Bonzini) > -fix build errors > -rewrite migration patch so it actually passes all tests > > v2: > -don't touch io/ code anymore > -always register yank functions > -'yank' now takes a list of instances to yank > -'query-yank' returns a list of yankable instances > > Lukas Straub (7): > Introduce yank feature > block/nbd.c: Add yank feature > chardev/char-socket.c: Add yank feature > migration: Add yank feature > io/channel-tls.c: make qio_channel_tls_shutdown thread-safe > io: Document thread-safety of qio_channel_shutdown > MAINTAINERS: Add myself as maintainer for yank feature > > MAINTAINERS | 13 +++ > block/nbd.c | 101 --- > chardev/char-socket.c | 24 + > include/io/channel.h | 2 + > include/qemu/yank.h | 79 +++ > io/channel-tls.c | 6 +- > migration/channel.c | 12 +++ > migration/migration.c | 18 +++- > migration/multifd.c | 10 ++ > migration/qemu-file-channel.c | 6 ++ > migration/savevm.c| 2 + > qapi/misc.json| 45 + > tests/Makefile.include| 2 +- > util/Makefile.objs| 1 + > util/yank.c | 179 ++ > 15 files changed, 459 insertions(+), 41 deletions(-) > create mode 100644 include/qemu/yank.h > create mode 100644 util/yank.c > > -- > 2.20.1 Forgot to cc Stefan Hajnoczi... pgpRMUVO8N5L_.pgp Description: OpenPGP digital signature
Re: [PATCH 13/46] qemu-option: Simplify around find_default_by_name()
On 6/24/20 11:43 AM, Markus Armbruster wrote:
Signed-off-by: Markus Armbruster
---
util/qemu-option.c | 13 -
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/util/qemu-option.c b/util/qemu-option.c
index ddcf3072c5..d9293814b4 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -286,11 +286,9 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name)
opt = qemu_opt_find(opts, name);
if (!opt) {
def_val = find_default_by_name(opts, name);
-if (def_val) {
-return def_val;
-}
+return def_val;
}
-return opt ? opt->str : NULL;
+return opt->str;
}
You could go with even fewer lines and variables by inverting the logic:
if (opt) {
return opt->str;
}
return find_default_by_name(opts, name);
void qemu_opt_iter_init(QemuOptsIter *iter, QemuOpts *opts, const char *name)
@@ -320,7 +318,7 @@ char *qemu_opt_get_del(QemuOpts *opts, const char *name)
{
QemuOpt *opt;
const char *def_val;
-char *str = NULL;
+char *str;
if (opts == NULL) {
return NULL;
@@ -329,10 +327,7 @@ char *qemu_opt_get_del(QemuOpts *opts, const char *name)
opt = qemu_opt_find(opts, name);
if (!opt) {
def_val = find_default_by_name(opts, name);
-if (def_val) {
-str = g_strdup(def_val);
-}
-return str;
+return g_strdup(def_val);
Similarly, you could drop def_val with:
return g_strdup(find_default_by_name(opts, name));
Either way,
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 12/46] qemu-option: Factor out helper find_default_by_name()
On 6/24/20 11:43 AM, Markus Armbruster wrote: Signed-off-by: Markus Armbruster --- util/qemu-option.c | 47 ++ 1 file changed, 27 insertions(+), 20 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 11/46] qemu-option: Make uses of find_desc_by_name() more similar
On 6/24/20 11:43 AM, Markus Armbruster wrote: This is to make the next commit easier to review. Signed-off-by: Markus Armbruster --- util/qemu-option.c | 32 ++-- 1 file changed, 18 insertions(+), 14 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 10/46] qemu-option: Check return value instead of @err where convenient
On 6/24/20 11:43 AM, Markus Armbruster wrote:
Convert uses like
opts = qemu_opts_create(..., );
if (err) {
...
}
to
opts = qemu_opts_create(..., );
if (!opts) {
...
}
Eliminate error_propagate() that are now unnecessary. Delete @err
that are now unused.
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 09/46] error: Avoid error_propagate() after migrate_add_blocker()
On 6/24/20 11:43 AM, Markus Armbruster wrote: When migrate_add_blocker(blocker, ) is followed by error_propagate(errp, err), we can often just as well do migrate_add_blocker(..., errp). Do that with this Coccinelle script: Double-check @err is not used afterwards. Dereferencing it would be use after free, but checking whether it's null would be legitimate. Signed-off-by: Markus Armbruster --- Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 08/46] error: Avoid unnecessary error_propagate() after error_setg()
On 6/24/20 11:43 AM, Markus Armbruster wrote:
Replace
error_setg(, ...);
error_propagate(errp, err);
by
error_setg(errp, ...);
Related pattern:
Nice explanation.
Bonus: the elimination of gotos will make later patches in this series
easier to review.
Candidates for conversion tracked down with this Coccinelle script:
@@
identifier err, errp;
expression list args;
@@
-error_setg(, args);
+error_setg(errp, args);
... when != err
error_propagate(errp, err);
Signed-off-by: Markus Armbruster
---
backends/cryptodev.c| 11 +++---
backends/hostmem-file.c | 19 +++---
backends/hostmem-memfd.c| 15
backends/hostmem.c | 27 ++
block/throttle-groups.c | 22 +--
hw/hyperv/vmbus.c | 5 +--
hw/i386/pc.c| 35 ++
hw/mem/nvdimm.c | 17 -
hw/mem/pc-dimm.c| 14 +++
hw/misc/aspeed_sdmc.c | 3 +-
hw/ppc/rs6000_mc.c | 9 ++---
hw/ppc/spapr.c | 73 -
hw/ppc/spapr_pci.c | 14 +++
hw/s390x/ipl.c | 23 +---
hw/s390x/sclp.c | 12 ++
hw/xen/xen_pt_config_init.c | 3 +-
iothread.c | 12 +++---
net/colo-compare.c | 20 --
net/dump.c | 10 ++---
net/filter-buffer.c | 10 ++---
qga/commands-win32.c| 16 +++-
21 files changed, 151 insertions(+), 219 deletions(-)
A bit bigger, and starts to be too complex to ask Coccinelle to directly
fix it (but at least using it for identification is nice). But the
patch is still manageable, and hopefully not too many instances creep
back in during the meantime while waiting for this series to land.
@@ -140,7 +138,6 @@ static void file_memory_backend_set_pmem(Object *o, bool
value, Error **errp)
HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o);
if (host_memory_backend_mr_inited(backend)) {
-
error_setg(errp, "cannot change property 'pmem' of %s.",
object_get_typename(o));
return;
Unrelated cleanup. Does it belong in a different patch?
@@ -148,13 +145,9 @@ static void file_memory_backend_set_pmem(Object *o, bool
value, Error **errp)
#ifndef CONFIG_LIBPMEM
if (value) {
-Error *local_err = NULL;
-
-error_setg(_err,
- "Lack of libpmem support while setting the 'pmem=on'"
+error_setg(errp, "Lack of libpmem support while setting the 'pmem=on'"
" of %s. We can't ensure data persistence.",
Pre-existing - doesn't follow our usual error message content
conventions regarding trailing '.'.
+++ b/hw/ppc/spapr_pci.c
@@ -1517,15 +1517,16 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
*/
if (plugged_dev->hotplugged && bus->devices[PCI_DEVFN(slotnr, 0)] &&
PCI_FUNC(pdev->devfn) != 0) {
-error_setg(_err, "PCI: slot %d function 0 already ocuppied by
%s,"
+error_setg(errp, "PCI: slot %d function 0 already ocuppied by %s,"
" additional functions can no longer be exposed to guest.",
Another one. Also, s/ocuppied/occupied/ while touching it.
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 07/46] error: Avoid more error_propagate() when error is not used here
On 6/24/20 11:43 AM, Markus Armbruster wrote:
When all we do with an Error we receive into a local variable is
propagating to somewhere else, we can just as well receive it there
right away. The previous commit did that for simple cases with
Coccinelle. Do it for a few more manually.
Signed-off-by: Markus Armbruster
---
blockdev.c | 5 +
hw/core/numa.c | 44 ++--
qdev-monitor.c | 11 ---
3 files changed, 19 insertions(+), 41 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index b66863c42a..73736a4eaf 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1009,13 +1009,10 @@ DriveInfo *drive_new(QemuOpts *all_opts,
BlockInterfaceType block_default_type,
}
/* Actual block device init: Functionality shared with blockdev-add */
-blk = blockdev_init(filename, bs_opts, _err);
+blk = blockdev_init(filename, bs_opts, errp);
bs_opts = NULL;
if (!blk) {
-error_propagate(errp, local_err);
goto fail;
-} else {
-assert(!local_err);
Loses an assertion that blockdev_init() doesn't mis-use errp, but I
think the goal of your cleanup work has been to make it easier to prove
any function follows the rules, so the assertion doesn't add much at
this point.
+++ b/qdev-monitor.c
@@ -600,7 +600,6 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
const char *driver, *path;
DeviceState *dev = NULL;
BusState *bus = NULL;
-Error *err = NULL;
bool hide;
driver = qemu_opt_get(opts, "driver");
@@ -655,15 +654,14 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
dev = qdev_new(driver);
/* Check whether the hotplug is allowed by the machine */
-if (qdev_hotplug && !qdev_hotplug_allowed(dev, )) {
+if (qdev_hotplug && !qdev_hotplug_allowed(dev, errp)) {
/* Error must be set in the machine hook */
-assert(err);
Another such case.
goto err_del_dev;
}
if (!bus && qdev_hotplug && !qdev_get_machine_hotplug_handler(dev)) {
/* No bus, no machine hotplug handler --> device is not hotpluggable
*/
-error_setg(, "Device '%s' can not be hotplugged on this machine",
+error_setg(errp, "Device '%s' can not be hotplugged on this machine",
Should we s/can not/cannot/ while touching this?
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 06/46] error: Avoid error_propagate() when error is not used here
On 6/24/20 11:43 AM, Markus Armbruster wrote: When all we do with an Error we receive into a local variable is propagating to somewhere else, we can just as well receive it there right away. Coccinelle script: This seems to be a recurring cleanup (witness commit 06592d7e, c0e90679, 6b62d961). In fact, shouldn't you just update that script with your enhancements here, and then run it directly, instead of embedding your tweaks in the commit message? @@ identifier fun, err, errp; expression list args; @@ -fun(args, ); +fun(args, errp); ... when != err when strict -error_propagate(errp, err); What does the 'when strict' accomplish? The existing coccinelle script uses 'when != errp', which may be enough to address... The first two rules are prone to fail with "error_propagate(...) ... reachable by inconsistent control-flow paths". Script without them re-run where that happens. ...the control-flow failures you hit? Manually double-check @err is not used afterwards. Dereferencing it would be use after free, but checking whether it's null would be legitimate. One such change to qbus_realize() backed out. Suboptimal line breaks tweaked manually. Signed-off-by: Markus Armbruster --- 22 files changed, 31 insertions(+), 73 deletions(-) At any rate, it's small enough to ensure all the changes remaining are still valid. Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 03/46] qdev: Smooth error checking of qdev_realize() & friends
On 6/24/20 11:43 AM, Markus Armbruster wrote:
Convert
foo(..., );
if (err) {
...
}
to
if (!foo(..., )) {
...
}
for qdev_realize(), qdev_realize_and_unref(), qbus_realize() and their
wrappers isa_realize_and_unref(), pci_realize_and_unref(),
sysbus_realize(), sysbus_realize_and_unref(), usb_realize_and_unref().
Coccinelle script:
Automated patching is so much easier than manual :)
Fails to convert hw/arm/armsse.c, because Coccinelle gets confused by
ARMSSE being used both as typedef and function-like macro there.
Convert manually.
Eliminate error_propagate() that are now unnecessary. Delete @err
that are now unused. Clean up whitespace.
Well, so there's still some manual stuff. But that's okay; hopefully we
don't have too many stragglers reintroduced via pending patches.
Signed-off-by: Markus Armbruster
---
65 files changed, 248 insertions(+), 768 deletions(-)
Quite a big trim. But I didn't spot any obvious problems.
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 02/46] error: Document Error API usage rules
On 6/24/20 11:43 AM, Markus Armbruster wrote: This merely codifies existing practice, with one exception: the rule advising against returning void, where existing practice is mixed. When the Error API was created, we adopted the (unwritten) rule to return void when the function returns no useful value on success, unlike GError, which recommends to return true on success and false on error then. Not only is this more verbose, it also creates an Error object even when @errp is null, _abort or _fatal. People got tired of the additional boilerplate, and started to ignore the unwritten rule. The result is confusion among developers about the preferred usage. The written rule will hopefully reduce the confusion. The remainder of this series will update a substantial amount of code to honor the rule. Signed-off-by: Markus Armbruster --- include/qapi/error.h | 26 ++ 1 file changed, 26 insertions(+) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 01/46] error: Improve examples in error.h's big comment
On 6/24/20 11:42 AM, Markus Armbruster wrote: Show errp instead of where is actually unusual. Add a missing declaration. Add a second error pileup example. Signed-off-by: Markus Armbruster --- include/qapi/error.h | 19 +++ 1 file changed, 15 insertions(+), 4 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 00/46] Less clumsy error checking
On 24/06/20 18:42, Markus Armbruster wrote: > When the Error API was created, we adopted the (unwritten) rule to > return void when the function returns no useful value on success, > unlike GError, which recommends to return true on success and false on > error then. I was actually never aware of the GError rule, but I, for one, welcome our new bool overlords. Paolo
Re: [PATCH v10 1/9] error: auto propagated local_err
Greg Kurz writes: > On Mon, 15 Jun 2020 07:21:03 +0200 > Markus Armbruster wrote: > >> Greg Kurz writes: >> >> > On Tue, 17 Mar 2020 18:16:17 +0300 >> > Vladimir Sementsov-Ogievskiy wrote: >> > >> >> Introduce a new ERRP_AUTO_PROPAGATE macro, to be used at start of >> >> functions with an errp OUT parameter. >> >> >> >> It has three goals: >> >> >> >> 1. Fix issue with error_fatal and error_prepend/error_append_hint: user >> >> can't see this additional information, because exit() happens in >> >> error_setg earlier than information is added. [Reported by Greg Kurz] >> >> >> > >> > I have more of these coming and I'd really like to use ERRP_AUTO_PROPAGATE. >> > >> > It seems we have a consensus on the macro itself but this series is gated >> > by the conversion of the existing code base. >> > >> > What about merging this patch separately so that people can start using >> > it at least ? >> >> Please give me a few more days to finish the work I feel should go in >> before the conversion. With any luck, Vladimir can then rebase / >> recreate the conversion easily, and you can finally use the macro for >> your own work. >> > > Sure. Thanks. Just posted "[PATCH 00/46] Less clumsy error checking". The sheer size of the thing and the length of its dependency chain explains why it took me so long. I feel bad about delaying you all the same. Apologies! I hope we can converge quickly enough to get Vladimir's work on top ready in time for the soft freeze.
[PATCH 46/46] hmp: Ignore Error objects where the return value suffices
qdev_print_props() receives and throws away Error objects just to
check for object_property_get_str() and object_property_print()
failure. Unnecessary, both return suitable values, so use those
instead.
Signed-off-by: Markus Armbruster
---
qdev-monitor.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 2db38b18af..7ebb97cf0d 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -695,22 +695,22 @@ static void qdev_print_props(Monitor *mon, DeviceState
*dev, Property *props,
if (!props)
return;
for (; props->name; props++) {
-Error *err = NULL;
char *value;
char *legacy_name = g_strdup_printf("legacy-%s", props->name);
+
if (object_property_get_type(OBJECT(dev), legacy_name, NULL)) {
-value = object_property_get_str(OBJECT(dev), legacy_name, );
+value = object_property_get_str(OBJECT(dev), legacy_name, NULL);
} else {
-value = object_property_print(OBJECT(dev), props->name, true,
);
+value = object_property_print(OBJECT(dev), props->name, true,
+ NULL);
}
g_free(legacy_name);
-if (err) {
-error_free(err);
+if (!value) {
continue;
}
qdev_printf("%s = %s\n", props->name,
-value && *value ? value : "");
+*value ? value : "");
g_free(value);
}
}
--
2.26.2
[PATCH 42/46] qdev: Smooth error checking with Coccinelle
The previous commit enables conversion of
qdev_prop_set_drive_err(..., );
if (err) {
...
}
to
if (!qdev_prop_set_drive_err(..., errp)) {
...
}
Coccinelle script:
@@
identifier fun = qdev_prop_set_drive_err;
expression list args, args2;
typedef Error;
Error *err;
identifier errp;
@@
- fun(args, , args2);
- if (err) {
+ if (!fun(args, errp, args2)) {
... when != err
- error_propagate(errp, err);
...
}
@@
identifier fun = qdev_prop_set_drive_err;
expression list args, args2;
typedef Error;
Error *err;
@@
- fun(args, , args2);
- if (err) {
+ if (!fun(args, , args2)) {
...
}
Eliminate error_propagate() that are now unnecessary. Delete @err
that are now unused. Tidy up line breaks and whitespace.
Signed-off-by: Markus Armbruster
---
hw/scsi/scsi-bus.c | 5 +
hw/sd/sd.c | 3 +--
hw/sd/ssi-sd.c | 5 ++---
3 files changed, 4 insertions(+), 9 deletions(-)
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index b937df16bc..df65cc2223 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -248,7 +248,6 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus,
BlockBackend *blk,
const char *driver;
char *name;
DeviceState *dev;
-Error *err = NULL;
DriveInfo *dinfo;
if (blk_is_sg(blk)) {
@@ -277,9 +276,7 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus,
BlockBackend *blk,
if (serial && object_property_find(OBJECT(dev), "serial", NULL)) {
qdev_prop_set_string(dev, "serial", serial);
}
-qdev_prop_set_drive_err(dev, "drive", blk, );
-if (err) {
-error_propagate(errp, err);
+if (!qdev_prop_set_drive_err(dev, "drive", blk, errp)) {
object_unparent(OBJECT(dev));
return NULL;
}
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 97a9d32964..5137168d66 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -706,8 +706,7 @@ SDState *sd_init(BlockBackend *blk, bool is_spi)
obj = object_new(TYPE_SD_CARD);
dev = DEVICE(obj);
-qdev_prop_set_drive_err(dev, "drive", blk, );
-if (err) {
+if (!qdev_prop_set_drive_err(dev, "drive", blk, )) {
error_reportf_err(err, "sd_init failed: ");
return NULL;
}
diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index 4d91f603fa..e0fb9f3093 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -254,9 +254,8 @@ static void ssi_sd_realize(SSISlave *d, Error **errp)
dinfo = drive_get_next(IF_SD);
carddev = qdev_new(TYPE_SD_CARD);
if (dinfo) {
-qdev_prop_set_drive_err(carddev, "drive", blk_by_legacy_dinfo(dinfo),
-);
-if (err) {
+if (!qdev_prop_set_drive_err(carddev, "drive",
+ blk_by_legacy_dinfo(dinfo), )) {
goto fail;
}
}
--
2.26.2
[PATCH 21/46] hmp: Eliminate a variable in hmp_migrate_set_parameter()
Signed-off-by: Markus Armbruster
---
monitor/hmp-cmds.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 2b0b58a336..d7810cb564 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1247,7 +1247,6 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict
*qdict)
MigrateSetParameters *p = g_new0(MigrateSetParameters, 1);
uint64_t valuebw = 0;
uint64_t cache_size;
-MultiFDCompression compress_type;
Error *err = NULL;
int val, ret;
@@ -1343,11 +1342,8 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict
*qdict)
break;
case MIGRATION_PARAMETER_MULTIFD_COMPRESSION:
p->has_multifd_compression = true;
-visit_type_MultiFDCompression(v, param, _type, );
-if (err) {
-break;
-}
-p->multifd_compression = compress_type;
+visit_type_MultiFDCompression(v, param, >multifd_compression,
+ );
break;
case MIGRATION_PARAMETER_MULTIFD_ZLIB_LEVEL:
p->has_multifd_zlib_level = true;
--
2.26.2
[PATCH 38/46] qom: Smooth error checking with Coccinelle
The previous commit enables conversion of
foo(..., );
if (err) {
...
}
to
if (!foo(..., errp)) {
...
}
for QOM functions that now return true / false on success / error.
Coccinelle script:
@@
identifier fun = {object_apply_global_props,
object_initialize_child_with_props, object_initialize_child_with_propsv,
object_property_get, object_property_get_bool, object_property_parse,
object_property_set, object_property_set_bool, object_property_set_int,
object_property_set_link, object_property_set_qobject, object_property_set_str,
object_property_set_uint, object_set_props, object_set_propv,
user_creatable_add_dict, user_creatable_complete, user_creatable_del};
expression list args, args2;
typedef Error;
Error *err;
identifier errp;
@@
- fun(args, , args2);
- if (err) {
+ if (!fun(args, errp, args2)) {
... when != err
- error_propagate(errp, err);
...
}
@@
identifier fun = {object_apply_global_props,
object_initialize_child_with_props, object_initialize_child_with_propsv,
object_property_get, object_property_get_bool, object_property_parse,
object_property_set, object_property_set_bool, object_property_set_int,
object_property_set_link, object_property_set_qobject, object_property_set_str,
object_property_set_uint, object_set_props, object_set_propv,
user_creatable_add_dict, user_creatable_complete, user_creatable_del};
expression list args, args2;
typedef Error;
Error *err;
@@
- fun(args, , args2);
- if (err) {
+ if (!fun(args, , args2)) {
...
}
Fails to convert hw/arm/armsse.c, because Coccinelle gets confused by
ARMSSE being used both as typedef and function-like macro there.
Convert manually.
Eliminate error_propagate() that are now unnecessary. Delete @err
that are now unused. Tidy up line breaks and whitespace.
Signed-off-by: Markus Armbruster
---
hw/arm/armsse.c | 64 +---
hw/arm/armv7m.c | 25 +-
hw/arm/aspeed_ast2600.c | 6 ++--
hw/arm/aspeed_soc.c | 6 ++--
hw/arm/bcm2835_peripherals.c | 6 ++--
hw/arm/bcm2836.c | 15 -
hw/arm/cubieboard.c | 11 +++
hw/arm/digic.c | 6 ++--
hw/arm/nrf51_soc.c | 10 ++
hw/arm/stm32f405_soc.c | 8 ++---
hw/arm/xlnx-zynqmp.c | 18 --
hw/block/xen-block.c | 15 -
hw/core/qdev.c | 8 +
hw/ppc/pnv_psi.c | 6 ++--
hw/s390x/s390-pci-bus.c | 3 +-
hw/s390x/s390-virtio-ccw.c | 3 +-
hw/scsi/scsi-bus.c | 4 +--
hw/sd/aspeed_sdhci.c | 11 ++-
hw/sd/ssi-sd.c | 3 +-
hw/virtio/virtio-rng.c | 6 +---
qdev-monitor.c | 5 +--
qom/object.c | 19 +++
qom/object_interfaces.c | 6 ++--
softmmu/vl.c | 7 +---
target/arm/monitor.c | 3 +-
target/i386/cpu.c| 11 +++
target/s390x/cpu_models.c| 3 +-
27 files changed, 94 insertions(+), 194 deletions(-)
diff --git a/hw/arm/armsse.c b/hw/arm/armsse.c
index 8af599cb13..3f12b7130b 100644
--- a/hw/arm/armsse.c
+++ b/hw/arm/armsse.c
@@ -535,23 +535,18 @@ static void armsse_realize(DeviceState *dev, Error **errp)
* later if necessary.
*/
if (extract32(info->cpuwait_rst, i, 1)) {
-object_property_set_bool(cpuobj, "start-powered-off", true, );
-if (err) {
-error_propagate(errp, err);
+if (!object_property_set_bool(cpuobj, "start-powered-off", true,
+ errp)) {
return;
}
}
if (!s->cpu_fpu[i]) {
-object_property_set_bool(cpuobj, "vfp", false, );
-if (err) {
-error_propagate(errp, err);
+if (!object_property_set_bool(cpuobj, "vfp", false, errp)) {
return;
}
}
if (!s->cpu_dsp[i]) {
-object_property_set_bool(cpuobj, "dsp", false, );
-if (err) {
-error_propagate(errp, err);
+if (!object_property_set_bool(cpuobj, "dsp", false, errp)) {
return;
}
}
@@ -603,10 +598,8 @@ static void armsse_realize(DeviceState *dev, Error **errp)
DeviceState *devs = DEVICE(splitter);
int cpunum;
-object_property_set_int(splitter, "num-lines", info->num_cpus,
-);
-if (err) {
-error_propagate(errp, err);
+if (!object_property_set_int(splitter, "num-lines",
+ info->num_cpus, errp)) {
return;
}
[PATCH 41/46] qdev: Make functions taking Error ** return bool, not void
See recent commit "error: Document Error API usage rules" for
rationale.
Signed-off-by: Markus Armbruster
---
include/hw/qdev-properties.h | 4 ++--
hw/core/qdev-properties-system.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 49c6cd2460..f12ab9e6bc 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -236,8 +236,8 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
/*
* Set properties between creation and realization.
*/
-void qdev_prop_set_drive_err(DeviceState *dev, const char *name,
- BlockBackend *value, Error **errp);
+bool qdev_prop_set_drive_err(DeviceState *dev, const char *name,
+ BlockBackend *value, Error **errp);
/*
* Set properties between creation and realization.
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index abd851656f..3e4f16fc21 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -413,7 +413,7 @@ const PropertyInfo qdev_prop_audiodev = {
.set = set_audiodev,
};
-void qdev_prop_set_drive_err(DeviceState *dev, const char *name,
+bool qdev_prop_set_drive_err(DeviceState *dev, const char *name,
BlockBackend *value, Error **errp)
{
const char *ref = "";
@@ -428,7 +428,7 @@ void qdev_prop_set_drive_err(DeviceState *dev, const char
*name,
}
}
-object_property_set_str(OBJECT(dev), name, ref, errp);
+return object_property_set_str(OBJECT(dev), name, ref, errp);
}
void qdev_prop_set_drive(DeviceState *dev, const char *name,
--
2.26.2
[PATCH 34/46] qom: Don't handle impossible object_property_get_link() failure
Don't handle object_property_get_link() failure that can't happen
unless the programmer screwed up, pass _abort.
Signed-off-by: Markus Armbruster
---
hw/arm/bcm2835_peripherals.c | 7 +--
hw/arm/bcm2836.c | 7 +--
hw/display/bcm2835_fb.c | 8 +---
hw/dma/bcm2835_dma.c | 9 +
hw/gpio/bcm2835_gpio.c | 15 ++-
hw/intc/nios2_iic.c | 8 +---
hw/misc/bcm2835_mbox.c | 9 +
hw/misc/bcm2835_property.c | 17 ++---
hw/usb/hcd-dwc2.c| 9 +
9 files changed, 11 insertions(+), 78 deletions(-)
diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
index 8313410ffe..3c40bda91e 100644
--- a/hw/arm/bcm2835_peripherals.c
+++ b/hw/arm/bcm2835_peripherals.c
@@ -134,12 +134,7 @@ static void bcm2835_peripherals_realize(DeviceState *dev,
Error **errp)
uint64_t ram_size, vcram_size;
int n;
-obj = object_property_get_link(OBJECT(dev), "ram", );
-if (obj == NULL) {
-error_setg(errp, "%s: required ram link not found: %s",
- __func__, error_get_pretty(err));
-return;
-}
+obj = object_property_get_link(OBJECT(dev), "ram", _abort);
ram = MEMORY_REGION(obj);
ram_size = memory_region_size(ram);
diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
index 51d156f0c5..35281df8c3 100644
--- a/hw/arm/bcm2836.c
+++ b/hw/arm/bcm2836.c
@@ -77,12 +77,7 @@ static void bcm2836_realize(DeviceState *dev, Error **errp)
/* common peripherals from bcm2835 */
-obj = object_property_get_link(OBJECT(dev), "ram", );
-if (obj == NULL) {
-error_setg(errp, "%s: required ram link not found: %s",
- __func__, error_get_pretty(err));
-return;
-}
+obj = object_property_get_link(OBJECT(dev), "ram", _abort);
object_property_add_const_link(OBJECT(>peripherals), "ram", obj);
diff --git a/hw/display/bcm2835_fb.c b/hw/display/bcm2835_fb.c
index c6263808a2..c4bfed2740 100644
--- a/hw/display/bcm2835_fb.c
+++ b/hw/display/bcm2835_fb.c
@@ -401,7 +401,6 @@ static void bcm2835_fb_reset(DeviceState *dev)
static void bcm2835_fb_realize(DeviceState *dev, Error **errp)
{
BCM2835FBState *s = BCM2835_FB(dev);
-Error *err = NULL;
Object *obj;
if (s->vcram_base == 0) {
@@ -409,12 +408,7 @@ static void bcm2835_fb_realize(DeviceState *dev, Error
**errp)
return;
}
-obj = object_property_get_link(OBJECT(dev), "dma-mr", );
-if (obj == NULL) {
-error_setg(errp, "%s: required dma-mr link not found: %s",
- __func__, error_get_pretty(err));
-return;
-}
+obj = object_property_get_link(OBJECT(dev), "dma-mr", _abort);
/* Fill in the parts of initial_config that are not set by QOM properties
*/
s->initial_config.xres_virtual = s->initial_config.xres;
diff --git a/hw/dma/bcm2835_dma.c b/hw/dma/bcm2835_dma.c
index 4cd9dab745..eb0002a2b9 100644
--- a/hw/dma/bcm2835_dma.c
+++ b/hw/dma/bcm2835_dma.c
@@ -376,16 +376,9 @@ static void bcm2835_dma_reset(DeviceState *dev)
static void bcm2835_dma_realize(DeviceState *dev, Error **errp)
{
BCM2835DMAState *s = BCM2835_DMA(dev);
-Error *err = NULL;
Object *obj;
-obj = object_property_get_link(OBJECT(dev), "dma-mr", );
-if (obj == NULL) {
-error_setg(errp, "%s: required dma-mr link not found: %s",
- __func__, error_get_pretty(err));
-return;
-}
-
+obj = object_property_get_link(OBJECT(dev), "dma-mr", _abort);
s->dma_mr = MEMORY_REGION(obj);
address_space_init(>dma_as, s->dma_mr, TYPE_BCM2835_DMA "-memory");
diff --git a/hw/gpio/bcm2835_gpio.c b/hw/gpio/bcm2835_gpio.c
index 91ce3d10cc..abdddbc67c 100644
--- a/hw/gpio/bcm2835_gpio.c
+++ b/hw/gpio/bcm2835_gpio.c
@@ -312,22 +312,11 @@ static void bcm2835_gpio_realize(DeviceState *dev, Error
**errp)
{
BCM2835GpioState *s = BCM2835_GPIO(dev);
Object *obj;
-Error *err = NULL;
-obj = object_property_get_link(OBJECT(dev), "sdbus-sdhci", );
-if (obj == NULL) {
-error_setg(errp, "%s: required sdhci link not found: %s",
-__func__, error_get_pretty(err));
-return;
-}
+obj = object_property_get_link(OBJECT(dev), "sdbus-sdhci", _abort);
s->sdbus_sdhci = SD_BUS(obj);
-obj = object_property_get_link(OBJECT(dev), "sdbus-sdhost", );
-if (obj == NULL) {
-error_setg(errp, "%s: required sdhost link not found: %s",
-__func__, error_get_pretty(err));
-return;
-}
+obj = object_property_get_link(OBJECT(dev), "sdbus-sdhost", _abort);
s->sdbus_sdhost = SD_BUS(obj);
}
diff --git a/hw/intc/nios2_iic.c b/hw/intc/nios2_iic.c
index 3a5d86c2a4..1a5df8c89a 100644
--- a/hw/intc/nios2_iic.c
+++ b/hw/intc/nios2_iic.c
@@ -66,14 +66,8 @@ static void altera_iic_init(Object *obj)
static void altera_iic_realize(DeviceState *dev, Error **errp)
{
struct
[PATCH 16/46] qemu-option: Make functions taking Error ** return bool, not void
See recent commit "error: Document Error API usage rules" for
rationale.
Signed-off-by: Markus Armbruster
---
include/qemu/option.h | 16
blockdev.c| 5 ++-
util/qemu-option.c| 92 +--
3 files changed, 64 insertions(+), 49 deletions(-)
diff --git a/include/qemu/option.h b/include/qemu/option.h
index eb4097889d..2d77b10f90 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -30,7 +30,7 @@
const char *get_opt_value(const char *p, char **value);
-void parse_option_size(const char *name, const char *value,
+bool parse_option_size(const char *name, const char *value,
uint64_t *ret, Error **errp);
bool has_help_option(const char *param);
@@ -80,11 +80,11 @@ uint64_t qemu_opt_get_number_del(QemuOpts *opts, const char
*name,
uint64_t qemu_opt_get_size_del(QemuOpts *opts, const char *name,
uint64_t defval);
int qemu_opt_unset(QemuOpts *opts, const char *name);
-void qemu_opt_set(QemuOpts *opts, const char *name, const char *value,
+bool qemu_opt_set(QemuOpts *opts, const char *name, const char *value,
Error **errp);
-void qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val,
+bool qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val,
Error **errp);
-void qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val,
+bool qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val,
Error **errp);
typedef int (*qemu_opt_loopfunc)(void *opaque,
const char *name, const char *value,
@@ -106,13 +106,13 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char
*id,
int fail_if_exists, Error **errp);
void qemu_opts_reset(QemuOptsList *list);
void qemu_opts_loc_restore(QemuOpts *opts);
-void qemu_opts_set(QemuOptsList *list, const char *id,
+bool qemu_opts_set(QemuOptsList *list, const char *id,
const char *name, const char *value, Error **errp);
const char *qemu_opts_id(QemuOpts *opts);
void qemu_opts_set_id(QemuOpts *opts, char *id);
void qemu_opts_del(QemuOpts *opts);
-void qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc, Error **errp);
-void qemu_opts_do_parse(QemuOpts *opts, const char *params,
+bool qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc, Error **errp);
+bool qemu_opts_do_parse(QemuOpts *opts, const char *params,
const char *firstname, Error **errp);
QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const char *params,
bool permit_abbrev);
@@ -125,7 +125,7 @@ QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const
QDict *qdict,
QDict *qemu_opts_to_qdict_filtered(QemuOpts *opts, QDict *qdict,
QemuOptsList *list, bool del);
QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict);
-void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp);
+bool qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp);
typedef int (*qemu_opts_loopfunc)(void *opaque, QemuOpts *opts, Error **errp);
int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func,
diff --git a/blockdev.c b/blockdev.c
index 481f36c543..1987625aba 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -705,7 +705,7 @@ BlockDriverState *bdrv_next_monitor_owned(BlockDriverState
*bs)
: QTAILQ_FIRST(_bdrv_states);
}
-static void qemu_opt_rename(QemuOpts *opts, const char *from, const char *to,
+static bool qemu_opt_rename(QemuOpts *opts, const char *from, const char *to,
Error **errp)
{
const char *value;
@@ -715,7 +715,7 @@ static void qemu_opt_rename(QemuOpts *opts, const char
*from, const char *to,
if (qemu_opt_find(opts, to)) {
error_setg(errp, "'%s' and its alias '%s' can't be used at the "
"same time", to, from);
-return;
+return false;
}
}
@@ -724,6 +724,7 @@ static void qemu_opt_rename(QemuOpts *opts, const char
*from, const char *to,
qemu_opt_set(opts, to, value, _abort);
qemu_opt_unset(opts, from);
}
+return true;
}
QemuOptsList qemu_legacy_drive_opts = {
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 14946e81f2..ff1e703b52 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -96,7 +96,7 @@ const char *get_opt_value(const char *p, char **value)
return offset;
}
-static void parse_option_bool(const char *name, const char *value, bool *ret,
+static bool parse_option_bool(const char *name, const char *value, bool *ret,
Error **errp)
{
if (!strcmp(value, "on")) {
@@ -106,10 +106,12 @@ static void parse_option_bool(const char *name, const
char *value, bool *ret,
} else {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
[PATCH 45/46] qdev: Ignore Error objects where the return value suffices
Signed-off-by: Markus Armbruster
---
hw/core/qdev-properties.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index e1ad147339..8eb4283a56 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -653,7 +653,6 @@ static void set_pci_devfn(Object *obj, Visitor *v, const
char *name,
Property *prop = opaque;
int32_t value, *ptr = qdev_get_prop_ptr(dev, prop);
unsigned int slot, fn, n;
-Error *local_err = NULL;
char *str;
if (dev->realized) {
@@ -661,9 +660,7 @@ static void set_pci_devfn(Object *obj, Visitor *v, const
char *name,
return;
}
-if (!visit_type_str(v, name, , _err)) {
-error_free(local_err);
-local_err = NULL;
+if (!visit_type_str(v, name, , NULL)) {
if (!visit_type_int32(v, name, , errp)) {
return;
}
--
2.26.2
[PATCH 43/46] qdev: Smooth error checking manually
When foo(..., ) is followed by error_propagate(errp, err), we can
often just as well do foo(..., errp). The previous commit did that
for simple cases with Coccinelle. Do it for one more manually.
Signed-off-by: Markus Armbruster
---
hw/block/fdc.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index fe0ae2d146..a0670c0aa0 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2527,7 +2527,7 @@ static void fdctrl_connect_drives(FDCtrl *fdctrl,
DeviceState *fdc_dev,
FDrive *drive;
DeviceState *dev;
BlockBackend *blk;
-Error *local_err = NULL;
+bool ok;
const char *fdc_name, *drive_suffix;
for (i = 0; i < MAX_FD; i++) {
@@ -2566,11 +2566,9 @@ static void fdctrl_connect_drives(FDCtrl *fdctrl,
DeviceState *fdc_dev,
blk_ref(blk);
blk_detach_dev(blk, fdc_dev);
fdctrl->qdev_for_drives[i].blk = NULL;
-qdev_prop_set_drive_err(dev, "drive", blk, _err);
+ok = qdev_prop_set_drive_err(dev, "drive", blk, errp);
blk_unref(blk);
-
-if (local_err) {
-error_propagate(errp, local_err);
+if (!ok) {
return;
}
--
2.26.2
[PATCH 35/46] qom: Use return values to check for error where that's simpler
When using the Error object to check for error, we need to receive it
into a local variable, then propagate() it to @errp.
Using the return value permits allows receiving it straight to @errp.
Signed-off-by: Markus Armbruster
---
qom/object.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/qom/object.c b/qom/object.c
index f6e9f0e413..326a8de91e 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -549,8 +549,7 @@ void object_initialize_child_with_propsv(Object *parentobj,
object_initialize(childobj, size, type);
obj = OBJECT(childobj);
-object_set_propv(obj, _err, vargs);
-if (local_err) {
+if (object_set_propv(obj, errp, vargs) < 0) {
goto out;
}
@@ -743,7 +742,7 @@ Object *object_new_with_propv(const char *typename,
}
obj = object_new_with_type(klass->type);
-if (object_set_propv(obj, _err, vargs) < 0) {
+if (object_set_propv(obj, errp, vargs) < 0) {
goto error;
}
@@ -1771,12 +1770,11 @@ static void object_set_link_property(Object *obj,
Visitor *v,
}
if (strcmp(path, "") != 0) {
-new_target = object_resolve_link(obj, name, path, _err);
+new_target = object_resolve_link(obj, name, path, errp);
}
g_free(path);
-if (local_err) {
-error_propagate(errp, local_err);
+if (!new_target) {
return;
}
--
2.26.2
[PATCH 22/46] qapi: Make visitor functions taking Error ** return bool, not void
See recent commit "error: Document Error API usage rules" for
rationale.
Signed-off-by: Markus Armbruster
---
docs/devel/qapi-code-gen.txt | 51 +--
include/qapi/clone-visitor.h | 8 +-
include/qapi/visitor-impl.h | 26 +++---
include/qapi/visitor.h| 102 -
audio/audio_legacy.c | 15 ++--
qapi/opts-visitor.c | 58 +++-
qapi/qapi-clone-visitor.c | 33 ---
qapi/qapi-dealloc-visitor.c | 27 --
qapi/qapi-visit-core.c| 165 ++
qapi/qobject-input-visitor.c | 109 +-
qapi/qobject-output-visitor.c | 27 --
qapi/string-input-visitor.c | 62 +++--
qapi/string-output-visitor.c | 32 ---
scripts/qapi/visit.py | 58 +---
14 files changed, 435 insertions(+), 338 deletions(-)
diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index a7794ef658..9bfc57063c 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -1408,42 +1408,38 @@ Example:
#include "example-qapi-types.h"
-void visit_type_UserDefOne_members(Visitor *v, UserDefOne *obj, Error
**errp);
-void visit_type_UserDefOne(Visitor *v, const char *name, UserDefOne **obj,
Error **errp);
-void visit_type_UserDefOneList(Visitor *v, const char *name,
UserDefOneList **obj, Error **errp);
+bool visit_type_UserDefOne_members(Visitor *v, UserDefOne *obj, Error
**errp);
+bool visit_type_UserDefOne(Visitor *v, const char *name, UserDefOne **obj,
Error **errp);
+bool visit_type_UserDefOneList(Visitor *v, const char *name,
UserDefOneList **obj, Error **errp);
-void visit_type_q_obj_my_command_arg_members(Visitor *v,
q_obj_my_command_arg *obj, Error **errp);
+bool visit_type_q_obj_my_command_arg_members(Visitor *v,
q_obj_my_command_arg *obj, Error **errp);
#endif /* EXAMPLE_QAPI_VISIT_H */
$ cat qapi-generated/example-qapi-visit.c
[Uninteresting stuff omitted...]
-void visit_type_UserDefOne_members(Visitor *v, UserDefOne *obj, Error
**errp)
+bool visit_type_UserDefOne_members(Visitor *v, UserDefOne *obj, Error
**errp)
{
Error *err = NULL;
-visit_type_int(v, "integer", >integer, );
-if (err) {
-goto out;
+if (!visit_type_int(v, "integer", >integer, errp)) {
+return false;
}
if (visit_optional(v, "string", >has_string)) {
-visit_type_str(v, "string", >string, );
-if (err) {
-goto out;
+if (!visit_type_str(v, "string", >string, errp)) {
+return false;
}
}
-
-out:
error_propagate(errp, err);
+return !err;
}
-void visit_type_UserDefOne(Visitor *v, const char *name, UserDefOne **obj,
Error **errp)
+bool visit_type_UserDefOne(Visitor *v, const char *name, UserDefOne **obj,
Error **errp)
{
Error *err = NULL;
-visit_start_struct(v, name, (void **)obj, sizeof(UserDefOne), );
-if (err) {
-goto out;
+if (!visit_start_struct(v, name, (void **)obj, sizeof(UserDefOne),
errp)) {
+return false;
}
if (!*obj) {
/* incomplete */
@@ -1461,19 +1457,18 @@ Example:
qapi_free_UserDefOne(*obj);
*obj = NULL;
}
-out:
error_propagate(errp, err);
+return !err;
}
-void visit_type_UserDefOneList(Visitor *v, const char *name,
UserDefOneList **obj, Error **errp)
+bool visit_type_UserDefOneList(Visitor *v, const char *name,
UserDefOneList **obj, Error **errp)
{
Error *err = NULL;
UserDefOneList *tail;
size_t size = sizeof(**obj);
-visit_start_list(v, name, (GenericList **)obj, size, );
-if (err) {
-goto out;
+if (!visit_start_list(v, name, (GenericList **)obj, size, errp)) {
+return false;
}
for (tail = *obj; tail;
@@ -1492,21 +1487,19 @@ Example:
qapi_free_UserDefOneList(*obj);
*obj = NULL;
}
-out:
error_propagate(errp, err);
+return !err;
}
-void visit_type_q_obj_my_command_arg_members(Visitor *v,
q_obj_my_command_arg *obj, Error **errp)
+bool visit_type_q_obj_my_command_arg_members(Visitor *v,
q_obj_my_command_arg *obj, Error **errp)
{
Error *err = NULL;
-visit_type_UserDefOneList(v, "arg1", >arg1, );
-if (err) {
-goto out;
+if (!visit_type_UserDefOneList(v, "arg1", >arg1, errp)) {
+return false;
}
-
-out:
error_propagate(errp, err);
+return !err;
}
[Uninteresting stuff omitted...]
diff --git a/include/qapi/clone-visitor.h b/include/qapi/clone-visitor.h
index 5b665ee38c..adf9a788e2 100644
--- a/include/qapi/clone-visitor.h
+++ b/include/qapi/clone-visitor.h
@@
[PATCH 40/46] qom: Make functions taking Error ** return bool, not 0/-1
Just for consistency. Also fix the example in object_set_props()'s
documentation.
Signed-off-by: Markus Armbruster
---
include/qom/object.h | 28 +++-
qom/object.c | 14 +++---
2 files changed, 18 insertions(+), 24 deletions(-)
diff --git a/include/qom/object.h b/include/qom/object.h
index b2d2558245..d74ede4eac 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -729,15 +729,13 @@ void object_apply_compat_props(Object *obj);
* Error *err = NULL;
* Object *obj = ...get / create object...;
*
- * obj = object_set_props(obj,
- * ,
- * "share", "yes",
- * "mem-path", "/dev/shm/somefile",
- * "prealloc", "yes",
- * "size", "1048576",
- * NULL);
- *
- * if (!obj) {
+ * if (!object_set_props(obj,
+ * ,
+ * "share", "yes",
+ * "mem-path", "/dev/shm/somefile",
+ * "prealloc", "yes",
+ * "size", "1048576",
+ * NULL)) {
* error_reportf_err(err, "Cannot set properties: ");
* }
*
@@ -746,11 +744,9 @@ void object_apply_compat_props(Object *obj);
* The returned object will have one stable reference maintained
* for as long as it is present in the object hierarchy.
*
- * Returns: -1 on error, 0 on success
+ * Returns: %true on success, %false on error.
*/
-int object_set_props(Object *obj,
- Error **errp,
- ...) QEMU_SENTINEL;
+bool object_set_props(Object *obj, Error **errp, ...) QEMU_SENTINEL;
/**
* object_set_propv:
@@ -760,11 +756,9 @@ int object_set_props(Object *obj,
*
* See object_set_props() for documentation.
*
- * Returns: -1 on error, 0 on success
+ * Returns: %true on success, %false on error.
*/
-int object_set_propv(Object *obj,
- Error **errp,
- va_list vargs);
+bool object_set_propv(Object *obj, Error **errp, va_list vargs);
/**
* object_initialize:
diff --git a/qom/object.c b/qom/object.c
index 684540a09f..9b479621e4 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -554,7 +554,7 @@ bool object_initialize_child_with_propsv(Object *parentobj,
object_initialize(childobj, size, type);
obj = OBJECT(childobj);
-if (object_set_propv(obj, errp, vargs) < 0) {
+if (!object_set_propv(obj, errp, vargs)) {
goto out;
}
@@ -746,7 +746,7 @@ Object *object_new_with_propv(const char *typename,
}
obj = object_new_with_type(klass->type);
-if (object_set_propv(obj, errp, vargs) < 0) {
+if (!object_set_propv(obj, errp, vargs)) {
goto error;
}
@@ -773,12 +773,12 @@ Object *object_new_with_propv(const char *typename,
}
-int object_set_props(Object *obj,
+bool object_set_props(Object *obj,
Error **errp,
...)
{
va_list vargs;
-int ret;
+bool ret;
va_start(vargs, errp);
ret = object_set_propv(obj, errp, vargs);
@@ -788,7 +788,7 @@ int object_set_props(Object *obj,
}
-int object_set_propv(Object *obj,
+bool object_set_propv(Object *obj,
Error **errp,
va_list vargs)
{
@@ -800,12 +800,12 @@ int object_set_propv(Object *obj,
g_assert(value != NULL);
if (!object_property_parse(obj, propname, value, errp)) {
-return -1;
+return false;
}
propname = va_arg(vargs, char *);
}
-return 0;
+return true;
}
--
2.26.2
[PATCH 23/46] qapi: Smooth error checking with Coccinelle
The previous commit enables conversion of
visit_foo(..., );
if (err) {
...
}
to
if (!visit_foo(..., errp)) {
...
}
for visitor functions that now return true / false on success / error.
Coccinelle script:
@@
identifier fun =~
"check_list|input_type_enum|lv_start_struct|lv_type_bool|lv_type_int64|lv_type_str|lv_type_uint64|output_type_enum|parse_type_bool|parse_type_int64|parse_type_null|parse_type_number|parse_type_size|parse_type_str|parse_type_uint64|print_type_bool|print_type_int64|print_type_null|print_type_number|print_type_size|print_type_str|print_type_uint64|qapi_clone_start_alternate|qapi_clone_start_list|qapi_clone_start_struct|qapi_clone_type_bool|qapi_clone_type_int64|qapi_clone_type_null|qapi_clone_type_number|qapi_clone_type_str|qapi_clone_type_uint64|qapi_dealloc_start_list|qapi_dealloc_start_struct|qapi_dealloc_type_anything|qapi_dealloc_type_bool|qapi_dealloc_type_int64|qapi_dealloc_type_null|qapi_dealloc_type_number|qapi_dealloc_type_str|qapi_dealloc_type_uint64|qobject_input_check_list|qobject_input_check_struct|qobject_input_start_alternate|qobject_input_start_list|qobject_input_start_struct|qobject_input_type_any|qobject_input_type_bool|qobject_input_type_bool_keyval|qobject_input_type_int64|qobject_input_type_int64_keyval|qobject_input_type_null|qobject_input_type_number|qobject_input_type_number_keyval|qobject_input_type_size_keyval|qobject_input_type_str|qobject_input_type_str_keyval|qobject_input_type_uint64|qobject_input_type_uint64_keyval|qobject_output_start_list|qobject_output_start_struct|qobject_output_type_any|qobject_output_type_bool|qobject_output_type_int64|qobject_output_type_null|qobject_output_type_number|qobject_output_type_str|qobject_output_type_uint64|start_list|visit_check_list|visit_check_struct|visit_start_alternate|visit_start_list|visit_start_struct|visit_type_.*";
expression list args, args2;
typedef Error;
Error *err;
identifier errp;
@@
- fun(args, , args2);
- if (err) {
+ if (!fun(args, errp, args2)) {
... when != err
- error_propagate(errp, err);
...
}
@@
identifier fun =~
"check_list|input_type_enum|lv_start_struct|lv_type_bool|lv_type_int64|lv_type_str|lv_type_uint64|output_type_enum|parse_type_bool|parse_type_int64|parse_type_null|parse_type_number|parse_type_size|parse_type_str|parse_type_uint64|print_type_bool|print_type_int64|print_type_null|print_type_number|print_type_size|print_type_str|print_type_uint64|qapi_clone_start_alternate|qapi_clone_start_list|qapi_clone_start_struct|qapi_clone_type_bool|qapi_clone_type_int64|qapi_clone_type_null|qapi_clone_type_number|qapi_clone_type_str|qapi_clone_type_uint64|qapi_dealloc_start_list|qapi_dealloc_start_struct|qapi_dealloc_type_anything|qapi_dealloc_type_bool|qapi_dealloc_type_int64|qapi_dealloc_type_null|qapi_dealloc_type_number|qapi_dealloc_type_str|qapi_dealloc_type_uint64|qobject_input_check_list|qobject_input_check_struct|qobject_input_start_alternate|qobject_input_start_list|qobject_input_start_struct|qobject_input_type_any|qobject_input_type_bool|qobject_input_type_bool_keyval|qobject_input_type_int64|qobject_input_type_int64_keyval|qobject_input_type_null|qobject_input_type_number|qobject_input_type_number_keyval|qobject_input_type_size_keyval|qobject_input_type_str|qobject_input_type_str_keyval|qobject_input_type_uint64|qobject_input_type_uint64_keyval|qobject_output_start_list|qobject_output_start_struct|qobject_output_type_any|qobject_output_type_bool|qobject_output_type_int64|qobject_output_type_null|qobject_output_type_number|qobject_output_type_str|qobject_output_type_uint64|start_list|visit_check_list|visit_check_struct|visit_start_alternate|visit_start_list|visit_start_struct|visit_type_.*";
expression list args, args2;
typedef Error;
Error *err;
@@
- fun(args, , args2);
- if (err) {
+ if (!fun(args, , args2)) {
...
}
Eliminate error_propagate() that are now unnecessary. Delete @err
that are now unused. Tidy up line breaks and whitespace.
Signed-off-by: Markus Armbruster
---
accel/kvm/kvm-all.c | 5 +--
accel/tcg/tcg-all.c | 5 +--
backends/cryptodev.c | 5 +--
backends/hostmem-file.c | 5 +--
backends/hostmem-memfd.c | 5 +--
backends/hostmem.c | 10 ++
block/blkdebug.c | 5 +--
block/nbd.c | 5 +--
block/sheepdog.c | 5 +--
block/throttle-groups.c | 8 ++---
bootdevice.c | 3 +-
hw/block/xen-block.c | 5 +--
hw/core/machine.c| 5 +--
hw/core/qdev-properties-system.c | 20 +++
hw/core/qdev-properties.c| 58 +++-
hw/cpu/core.c| 10 ++
hw/gpio/aspeed_gpio.c| 5 +--
hw/i386/pc.c
[PATCH 11/46] qemu-option: Make uses of find_desc_by_name() more similar
This is to make the next commit easier to review.
Signed-off-by: Markus Armbruster
---
util/qemu-option.c | 32 ++--
1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 6119f971a4..9941005c91 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -270,6 +270,7 @@ static void qemu_opt_del_all(QemuOpts *opts, const char
*name)
const char *qemu_opt_get(QemuOpts *opts, const char *name)
{
QemuOpt *opt;
+const QemuOptDesc *desc;
if (opts == NULL) {
return NULL;
@@ -277,7 +278,7 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name)
opt = qemu_opt_find(opts, name);
if (!opt) {
-const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
+desc = find_desc_by_name(opts->list->desc, name);
if (desc && desc->def_value_str) {
return desc->def_value_str;
}
@@ -348,6 +349,7 @@ static bool qemu_opt_get_bool_helper(QemuOpts *opts, const
char *name,
bool defval, bool del)
{
QemuOpt *opt;
+const QemuOptDesc *desc;
bool ret = defval;
if (opts == NULL) {
@@ -356,7 +358,7 @@ static bool qemu_opt_get_bool_helper(QemuOpts *opts, const
char *name,
opt = qemu_opt_find(opts, name);
if (opt == NULL) {
-const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
+desc = find_desc_by_name(opts->list->desc, name);
if (desc && desc->def_value_str) {
parse_option_bool(name, desc->def_value_str, , _abort);
}
@@ -384,6 +386,7 @@ static uint64_t qemu_opt_get_number_helper(QemuOpts *opts,
const char *name,
uint64_t defval, bool del)
{
QemuOpt *opt;
+const QemuOptDesc *desc;
uint64_t ret = defval;
if (opts == NULL) {
@@ -392,7 +395,7 @@ static uint64_t qemu_opt_get_number_helper(QemuOpts *opts,
const char *name,
opt = qemu_opt_find(opts, name);
if (opt == NULL) {
-const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
+desc = find_desc_by_name(opts->list->desc, name);
if (desc && desc->def_value_str) {
parse_option_number(name, desc->def_value_str, , _abort);
}
@@ -421,6 +424,7 @@ static uint64_t qemu_opt_get_size_helper(QemuOpts *opts,
const char *name,
uint64_t defval, bool del)
{
QemuOpt *opt;
+const QemuOptDesc *desc;
uint64_t ret = defval;
if (opts == NULL) {
@@ -429,7 +433,7 @@ static uint64_t qemu_opt_get_size_helper(QemuOpts *opts,
const char *name,
opt = qemu_opt_find(opts, name);
if (opt == NULL) {
-const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
+desc = find_desc_by_name(opts->list->desc, name);
if (desc && desc->def_value_str) {
parse_option_size(name, desc->def_value_str, , _abort);
}
@@ -540,18 +544,18 @@ void qemu_opt_set_bool(QemuOpts *opts, const char *name,
bool val,
Error **errp)
{
QemuOpt *opt;
-const QemuOptDesc *desc = opts->list->desc;
+const QemuOptDesc *desc;
-opt = g_malloc0(sizeof(*opt));
-opt->desc = find_desc_by_name(desc, name);
-if (!opt->desc && !opts_accepts_any(opts)) {
+desc = find_desc_by_name(opts->list->desc, name);
+if (!desc && !opts_accepts_any(opts)) {
error_setg(errp, QERR_INVALID_PARAMETER, name);
-g_free(opt);
return;
}
+opt = g_malloc0(sizeof(*opt));
opt->name = g_strdup(name);
opt->opts = opts;
+opt->desc = desc;
opt->value.boolean = !!val;
opt->str = g_strdup(val ? "on" : "off");
QTAILQ_INSERT_TAIL(>head, opt, next);
@@ -561,18 +565,18 @@ void qemu_opt_set_number(QemuOpts *opts, const char
*name, int64_t val,
Error **errp)
{
QemuOpt *opt;
-const QemuOptDesc *desc = opts->list->desc;
+const QemuOptDesc *desc;
-opt = g_malloc0(sizeof(*opt));
-opt->desc = find_desc_by_name(desc, name);
-if (!opt->desc && !opts_accepts_any(opts)) {
+desc = find_desc_by_name(opts->list->desc, name);
+if (!desc && !opts_accepts_any(opts)) {
error_setg(errp, QERR_INVALID_PARAMETER, name);
-g_free(opt);
return;
}
+opt = g_malloc0(sizeof(*opt));
opt->name = g_strdup(name);
opt->opts = opts;
+opt->desc = desc;
opt->value.uint = val;
opt->str = g_strdup_printf("%" PRId64, val);
QTAILQ_INSERT_TAIL(>head, opt, next);
--
2.26.2
[PATCH 17/46] qemu-option: Smooth error checking with Coccinelle
The previous commit enables conversion of
foo(..., );
if (err) {
...
}
to
if (!foo(..., )) {
...
}
for QemuOpts functions that now return true / false on success /
error. Coccinelle script:
@@
identifier fun = {opts_do_parse, parse_option_bool, parse_option_number,
parse_option_size, qemu_opt_parse, qemu_opt_rename, qemu_opt_set,
qemu_opt_set_bool, qemu_opt_set_number, qemu_opts_absorb_qdict,
qemu_opts_do_parse, qemu_opts_from_qdict_entry, qemu_opts_set,
qemu_opts_validate};
expression list args, args2;
typedef Error;
Error *err;
identifier errp;
@@
- fun(args, , args2);
- if (err) {
+ if (!fun(args, errp, args2)) {
... when != err
- error_propagate(errp, err);
...
}
@@
identifier fun = {opts_do_parse, parse_option_bool, parse_option_number,
parse_option_size, qemu_opt_parse, qemu_opt_rename, qemu_opt_set,
qemu_opt_set_bool, qemu_opt_set_number, qemu_opts_absorb_qdict,
qemu_opts_do_parse, qemu_opts_from_qdict_entry, qemu_opts_set,
qemu_opts_validate};
expression list args, args2;
typedef Error;
Error *err;
@@
- fun(args, , args2);
- if (err) {
+ if (!fun(args, , args2)) {
...
}
Eliminate error_propagate() that are now unnecessary. Delete @err
that are now unused. Tidy up line breaks and whitespace.
Signed-off-by: Markus Armbruster
---
block.c | 18 ++
block/blkdebug.c | 4 +---
block/blklogwrites.c | 4 +---
block/blkverify.c | 4 +---
block/crypto.c| 5 +
block/curl.c | 5 +
block/file-posix.c| 8 ++--
block/file-win32.c| 8 ++--
block/gluster.c | 17 +
block/iscsi.c | 4 +---
block/nbd.c | 5 +
block/parallels.c | 3 +--
block/qcow2.c | 4 +---
block/quorum.c| 3 +--
block/raw-format.c| 5 +
block/replication.c | 3 +--
block/sheepdog.c | 5 +
block/ssh.c | 4 +---
block/throttle.c | 5 +
block/vpc.c | 4 +---
block/vvfat.c | 5 +
block/vxhs.c | 6 ++
blockdev.c| 15 ---
chardev/char.c| 6 ++
contrib/ivshmem-server/main.c | 4 ++--
hw/net/virtio-net.c | 5 ++---
hw/smbios/smbios.c| 33 -
qapi/string-input-visitor.c | 5 +
qemu-img.c| 19 +++
tpm.c | 5 +
util/qemu-config.c| 15 ---
util/qemu-option.c| 26 +-
32 files changed, 70 insertions(+), 192 deletions(-)
diff --git a/block.c b/block.c
index 30a72bc4c2..77e85f13db 100644
--- a/block.c
+++ b/block.c
@@ -1629,9 +1629,7 @@ static int bdrv_open_common(BlockDriverState *bs,
BlockBackend *file,
assert(options != NULL && bs->options != options);
opts = qemu_opts_create(_runtime_opts, NULL, 0, _abort);
-qemu_opts_absorb_qdict(opts, options, _err);
-if (local_err) {
-error_propagate(errp, local_err);
+if (!qemu_opts_absorb_qdict(opts, options, errp)) {
ret = -EINVAL;
goto fail_opts;
}
@@ -4091,9 +4089,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
BlockReopenQueue *queue,
/* Process generic block layer options */
opts = qemu_opts_create(_runtime_opts, NULL, 0, _abort);
-qemu_opts_absorb_qdict(opts, reopen_state->options, _err);
-if (local_err) {
-error_propagate(errp, local_err);
+if (!qemu_opts_absorb_qdict(opts, reopen_state->options, errp)) {
ret = -EINVAL;
goto error;
}
@@ -6077,8 +6073,7 @@ void bdrv_img_create(const char *filename, const char
*fmt,
/* Parse -o options */
if (options) {
-qemu_opts_do_parse(opts, options, NULL, _err);
-if (local_err) {
+if (!qemu_opts_do_parse(opts, options, NULL, _err)) {
goto out;
}
}
@@ -6091,8 +6086,8 @@ void bdrv_img_create(const char *filename, const char
*fmt,
}
if (base_filename) {
-qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename, _err);
-if (local_err) {
+if (!qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename,
+ _err)) {
error_setg(errp, "Backing file not supported for file format '%s'",
fmt);
goto out;
@@ -6100,8 +6095,7 @@ void bdrv_img_create(const char *filename, const char
*fmt,
}
if (base_fmt) {
-qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt, _err);
-if (local_err) {
+if
[PATCH 29/46] acpi: Avoid unnecessary error_propagate() after error_setg()
The commit before previous enables another round of the transformation
from recent commit "error: Avoid unnecessary error_propagate() after
error_setg()".
Signed-off-by: Markus Armbruster
---
hw/acpi/core.c | 15 ++-
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index 91ae66b806..f6d9ec4f13 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -239,7 +239,6 @@ static void acpi_table_install(const char unsigned *blob,
size_t bloblen,
void acpi_table_add(const QemuOpts *opts, Error **errp)
{
AcpiTableOptions *hdrs = NULL;
-Error *err = NULL;
char **pathnames = NULL;
char **cur;
size_t bloblen = 0;
@@ -257,13 +256,13 @@ void acpi_table_add(const QemuOpts *opts, Error **errp)
goto out;
}
if (hdrs->has_file == hdrs->has_data) {
-error_setg(, "'-acpitable' requires one of 'data' or 'file'");
+error_setg(errp, "'-acpitable' requires one of 'data' or 'file'");
goto out;
}
pathnames = g_strsplit(hdrs->has_file ? hdrs->file : hdrs->data, ":", 0);
if (pathnames == NULL || pathnames[0] == NULL) {
-error_setg(, "'-acpitable' requires at least one pathname");
+error_setg(errp, "'-acpitable' requires at least one pathname");
goto out;
}
@@ -272,7 +271,7 @@ void acpi_table_add(const QemuOpts *opts, Error **errp)
int fd = open(*cur, O_RDONLY | O_BINARY);
if (fd < 0) {
-error_setg(, "can't open file %s: %s", *cur, strerror(errno));
+error_setg(errp, "can't open file %s: %s", *cur, strerror(errno));
goto out;
}
@@ -288,8 +287,8 @@ void acpi_table_add(const QemuOpts *opts, Error **errp)
memcpy(blob + bloblen, data, r);
bloblen += r;
} else if (errno != EINTR) {
-error_setg(, "can't read file %s: %s",
- *cur, strerror(errno));
+error_setg(errp, "can't read file %s: %s", *cur,
+ strerror(errno));
close(fd);
goto out;
}
@@ -298,14 +297,12 @@ void acpi_table_add(const QemuOpts *opts, Error **errp)
close(fd);
}
-acpi_table_install(blob, bloblen, hdrs->has_file, hdrs, );
+acpi_table_install(blob, bloblen, hdrs->has_file, hdrs, errp);
out:
g_free(blob);
g_strfreev(pathnames);
qapi_free_AcpiTableOptions(hdrs);
-
-error_propagate(errp, err);
}
unsigned acpi_table_len(void *current)
--
2.26.2
[PATCH 19/46] block: Avoid unnecessary error_propagate() after error_setg()
The previous commit enables another round of the transformation from
recent commit "error: Avoid unnecessary error_propagate() after
error_setg()".
Signed-off-by: Markus Armbruster
---
block/quorum.c | 16 +++-
block/replication.c | 12 +---
block/vxhs.c| 10 --
3 files changed, 16 insertions(+), 22 deletions(-)
diff --git a/block/quorum.c b/block/quorum.c
index 9ed20e1998..6df9449fc2 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -910,13 +910,12 @@ static int quorum_open(BlockDriverState *bs, QDict
*options, int flags,
/* count how many different children are present */
s->num_children = qdict_array_entries(options, "children.");
if (s->num_children < 0) {
-error_setg(_err, "Option children is not a valid array");
+error_setg(errp, "Option children is not a valid array");
ret = -EINVAL;
goto exit;
}
if (s->num_children < 1) {
-error_setg(_err,
- "Number of provided children must be 1 or more");
+error_setg(errp, "Number of provided children must be 1 or more");
ret = -EINVAL;
goto exit;
}
@@ -929,7 +928,7 @@ static int quorum_open(BlockDriverState *bs, QDict
*options, int flags,
s->threshold = qemu_opt_get_number(opts, QUORUM_OPT_VOTE_THRESHOLD, 0);
/* and validate it against s->num_children */
-ret = quorum_valid_threshold(s->threshold, s->num_children, _err);
+ret = quorum_valid_threshold(s->threshold, s->num_children, errp);
if (ret < 0) {
goto exit;
}
@@ -942,7 +941,7 @@ static int quorum_open(BlockDriverState *bs, QDict
*options, int flags,
-EINVAL, NULL);
}
if (ret < 0) {
-error_setg(_err, "Please set read-pattern as fifo or quorum");
+error_setg(errp, "Please set read-pattern as fifo or quorum");
goto exit;
}
s->read_pattern = ret;
@@ -950,7 +949,7 @@ static int quorum_open(BlockDriverState *bs, QDict
*options, int flags,
if (s->read_pattern == QUORUM_READ_PATTERN_QUORUM) {
s->is_blkverify = qemu_opt_get_bool(opts, QUORUM_OPT_BLKVERIFY, false);
if (s->is_blkverify && (s->num_children != 2 || s->threshold != 2)) {
-error_setg(_err, "blkverify=on can only be set if there are "
+error_setg(errp, "blkverify=on can only be set if there are "
"exactly two files and vote-threshold is 2");
ret = -EINVAL;
goto exit;
@@ -959,7 +958,7 @@ static int quorum_open(BlockDriverState *bs, QDict
*options, int flags,
s->rewrite_corrupted = qemu_opt_get_bool(opts, QUORUM_OPT_REWRITE,
false);
if (s->rewrite_corrupted && s->is_blkverify) {
-error_setg(_err,
+error_setg(errp,
"rewrite-corrupted=on cannot be used with
blkverify=on");
ret = -EINVAL;
goto exit;
@@ -979,6 +978,7 @@ static int quorum_open(BlockDriverState *bs, QDict
*options, int flags,
_of_bds, BDRV_CHILD_DATA, false,
_err);
if (local_err) {
+error_propagate(errp, local_err);
ret = -EINVAL;
goto close_exit;
}
@@ -1004,8 +1004,6 @@ close_exit:
g_free(opened);
exit:
qemu_opts_del(opts);
-/* propagate error */
-error_propagate(errp, local_err);
return ret;
}
diff --git a/block/replication.c b/block/replication.c
index 7f4ab357a4..0c70215784 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -85,7 +85,6 @@ static int replication_open(BlockDriverState *bs, QDict
*options,
{
int ret;
BDRVReplicationState *s = bs->opaque;
-Error *local_err = NULL;
QemuOpts *opts = NULL;
const char *mode;
const char *top_id;
@@ -105,7 +104,7 @@ static int replication_open(BlockDriverState *bs, QDict
*options,
mode = qemu_opt_get(opts, REPLICATION_MODE);
if (!mode) {
-error_setg(_err, "Missing the option mode");
+error_setg(errp, "Missing the option mode");
goto fail;
}
@@ -113,7 +112,8 @@ static int replication_open(BlockDriverState *bs, QDict
*options,
s->mode = REPLICATION_MODE_PRIMARY;
top_id = qemu_opt_get(opts, REPLICATION_TOP_ID);
if (top_id) {
-error_setg(_err, "The primary side does not support option
top-id");
+error_setg(errp,
+ "The primary side does not support option top-id");
goto fail;
}
} else if (!strcmp(mode, "secondary")) {
@@ -121,11 +121,11 @@ static int replication_open(BlockDriverState *bs, QDict
*options,
top_id = qemu_opt_get(opts, REPLICATION_TOP_ID);
s->top_id = g_strdup(top_id);
if (!s->top_id) {
-error_setg(_err, "Missing the option top-id");
+
[PATCH 00/46] Less clumsy error checking
When the Error API was created, we adopted the (unwritten) rule to
return void when the function returns no useful value on success,
unlike GError, which recommends to return true on success and false on
error then.
When a function returns a distinct error value, say false, a checked
call that passes the error up looks like
if (!frobnicate(..., errp)) {
handle the error...
}
When it returns void, we need
Error *err = NULL;
frobnicate(..., );
if (err) {
handle the error...
error_propagate(errp, err);
}
Not only is this more verbose, it also creates an Error object even
when @errp is null, _abort or _fatal.
People got tired of the additional boilerplate, and started to ignore
the unwritten rule. The result is confusion among developers about
the preferred usage.
This series adopts the GError rule (in writing), and updates a
substantial amount of code to honor the rule. Cuts the number of
error_propagate() calls nearly by half. The diffstat speaks for
itself.
Based on my "[PATCH v2 00/25] Error handling fixes & cleanups". Also
available from my public repository https://repo.or.cz/qemu/armbru.git
on branch error-smooth.
Based-on: <20200624083737.3086768-1-arm...@redhat.com>
Markus Armbruster (46):
error: Improve examples in error.h's big comment
error: Document Error API usage rules
qdev: Smooth error checking of qdev_realize() & friends
macio: Tidy up error handling in macio_newworld_realize()
virtio-crypto-pci: Tidy up virtio_crypto_pci_realize()
error: Avoid error_propagate() when error is not used here
error: Avoid more error_propagate() when error is not used here
error: Avoid unnecessary error_propagate() after error_setg()
error: Avoid error_propagate() after migrate_add_blocker()
qemu-option: Check return value instead of @err where convenient
qemu-option: Make uses of find_desc_by_name() more similar
qemu-option: Factor out helper find_default_by_name()
qemu-option: Simplify around find_default_by_name()
qemu-option: Factor out helper opt_create()
qemu-option: Tidy up opt_set() not to free arguments on failure
qemu-option: Make functions taking Error ** return bool, not void
qemu-option: Smooth error checking with Coccinelle
qemu-option: Smooth error checking manually
block: Avoid unnecessary error_propagate() after error_setg()
block: Avoid error accumulation in bdrv_img_create()
hmp: Eliminate a variable in hmp_migrate_set_parameter()
qapi: Make visitor functions taking Error ** return bool, not void
qapi: Smooth error checking with Coccinelle
qapi: Smooth error checking manually
qapi: Smooth visitor error checking in generated code
qapi: Smooth another visitor error checking pattern
qapi: Purge error_propagate() from QAPI core
block/parallels: Simplify parallels_open() after previous commit
acpi: Avoid unnecessary error_propagate() after error_setg()
s390x/pci: Fix harmless mistake in zpci's property fid's setter
qom: Use error_reportf_err() instead of g_printerr() in examples
qom: Rename qdev_get_type() to object_get_type()
qom: Crash more nicely on object_property_get_link() failure
qom: Don't handle impossible object_property_get_link() failure
qom: Use return values to check for error where that's simpler
qom: Put name parameter before value / visitor parameter
qom: Make functions taking Error ** return bool, not void
qom: Smooth error checking with Coccinelle
qom: Smooth error checking manually
qom: Make functions taking Error ** return bool, not 0/-1
qdev: Make functions taking Error ** return bool, not void
qdev: Smooth error checking with Coccinelle
qdev: Smooth error checking manually
qemu-img: Ignore Error objects where the return value suffices
qdev: Ignore Error objects where the return value suffices
hmp: Ignore Error objects where the return value suffices
docs/devel/qapi-code-gen.txt | 103 -
include/hw/audio/pcspk.h | 2 +-
include/hw/qdev-properties.h | 4 +-
include/qapi/clone-visitor.h | 8 +-
include/qapi/error.h | 45 +++-
include/qapi/visitor-impl.h | 26 +--
include/qapi/visitor.h | 102 +
include/qemu/option.h| 16 +-
include/qom/object.h | 105 +
include/qom/object_interfaces.h | 12 +-
include/qom/qom-qobject.h| 9 +-
accel/kvm/kvm-all.c | 55 +++--
accel/tcg/tcg-all.c | 5 +-
audio/audio_legacy.c | 15 +-
backends/cryptodev-vhost-user.c | 3 +-
backends/cryptodev.c | 16 +-
backends/hostmem-file.c | 22 +-
backends/hostmem-memfd.c | 18 +-
backends/hostmem.c | 33 ++-
backends/rng.c | 2 +-
block.c |
[PATCH 32/46] qom: Rename qdev_get_type() to object_get_type()
Commit 2f262e06f0 lifted qdev_get_type() from qdev to object without
renaming it accordingly. Do that now.
Signed-off-by: Markus Armbruster
---
qom/object.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/qom/object.c b/qom/object.c
index b8aac074c2..f6e9f0e413 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -2359,7 +2359,7 @@ object_class_property_add_tm(ObjectClass *klass, const
char *name,
NULL, NULL, prop);
}
-static char *qdev_get_type(Object *obj, Error **errp)
+static char *object_get_type(Object *obj, Error **errp)
{
return g_strdup(object_get_typename(obj));
}
@@ -2702,7 +2702,7 @@ void object_class_property_set_description(ObjectClass
*klass,
static void object_class_init(ObjectClass *klass, void *data)
{
-object_class_property_add_str(klass, "type", qdev_get_type,
+object_class_property_add_str(klass, "type", object_get_type,
NULL);
}
--
2.26.2
[PATCH 37/46] qom: Make functions taking Error ** return bool, not void
See recent commit "error: Document Error API usage rules" for rationale. Signed-off-by: Markus Armbruster --- include/qom/object.h| 42 - include/qom/object_interfaces.h | 12 +++-- include/qom/qom-qobject.h | 4 +- qom/object.c| 84 + qom/object_interfaces.c | 21 + qom/qom-qobject.c | 6 ++- 6 files changed, 113 insertions(+), 56 deletions(-) diff --git a/include/qom/object.h b/include/qom/object.h index 7ef9c8d0cc..b2d2558245 100644 --- a/include/qom/object.h +++ b/include/qom/object.h @@ -703,7 +703,7 @@ Object *object_new_with_propv(const char *typename, Error **errp, va_list vargs); -void object_apply_global_props(Object *obj, const GPtrArray *props, +bool object_apply_global_props(Object *obj, const GPtrArray *props, Error **errp); void object_set_machine_compat_props(GPtrArray *compat_props); void object_set_accelerator_compat_props(GPtrArray *compat_props); @@ -798,8 +798,10 @@ void object_initialize(void *obj, size_t size, const char *typename); * strings. The propname of %NULL indicates the end of the property list. * If the object implements the user creatable interface, the object will * be marked complete once all the properties have been processed. + * + * Returns: %true on success, %false on failure. */ -void object_initialize_child_with_props(Object *parentobj, +bool object_initialize_child_with_props(Object *parentobj, const char *propname, void *childobj, size_t size, const char *type, Error **errp, ...) QEMU_SENTINEL; @@ -815,8 +817,10 @@ void object_initialize_child_with_props(Object *parentobj, * @vargs: list of property names and values * * See object_initialize_child() for documentation. + * + * Returns: %true on success, %false on failure. */ -void object_initialize_child_with_propsv(Object *parentobj, +bool object_initialize_child_with_propsv(Object *parentobj, const char *propname, void *childobj, size_t size, const char *type, Error **errp, va_list vargs); @@ -1197,8 +1201,10 @@ void object_unparent(Object *obj); * @errp: returns an error if this function fails * * Reads a property from a object. + * + * Returns: %true on success, %false on failure. */ -void object_property_get(Object *obj, const char *name, Visitor *v, +bool object_property_get(Object *obj, const char *name, Visitor *v, Error **errp); /** @@ -1208,8 +1214,10 @@ void object_property_get(Object *obj, const char *name, Visitor *v, * @errp: returns an error if this function fails * * Writes a string value to a property. + * + * Returns: %true on success, %false on failure. */ -void object_property_set_str(Object *obj, +bool object_property_set_str(Object *obj, const char *name, const char *value, Error **errp); @@ -1238,8 +1246,9 @@ char *object_property_get_str(Object *obj, const char *name, * OBJ_PROP_LINK_STRONG bit, the old target object is * unreferenced, and a reference is added to the new target object. * + * Returns: %true on success, %false on failure. */ -void object_property_set_link(Object *obj, const char *name, Object *value, +bool object_property_set_link(Object *obj, const char *name, Object *value, Error **errp); /** @@ -1262,8 +1271,10 @@ Object *object_property_get_link(Object *obj, const char *name, * @errp: returns an error if this function fails * * Writes a bool value to a property. + * + * Returns: %true on success, %false on failure. */ -void object_property_set_bool(Object *obj, const char *name, bool value, +bool object_property_set_bool(Object *obj, const char *name, bool value, Error **errp); /** @@ -1285,8 +1296,10 @@ bool object_property_get_bool(Object *obj, const char *name, * @errp: returns an error if this function fails * * Writes an integer value to a property. + * + * Returns: %true on success, %false on failure. */ -void object_property_set_int(Object *obj, const char *name, int64_t value, +bool object_property_set_int(Object *obj, const char *name, int64_t value, Error **errp); /** @@ -1308,8 +1321,10 @@ int64_t object_property_get_int(Object *obj, const char *name, * @errp: returns an error if this function fails * * Writes an unsigned integer value to a property. + * + * Returns: %true on success, %false on failure. */ -void object_property_set_uint(Object *obj, const char *name, uint64_t value, +bool object_property_set_uint(Object *obj, const char *name, uint64_t value, Error **errp);
[PATCH 18/46] qemu-option: Smooth error checking manually
When foo(..., ) is followed by error_propagate(errp, err), we can
often just as well do foo(..., errp). The previous commit did that
for simple cases with Coccinelle. Do it for a few more manually.
Signed-off-by: Markus Armbruster
---
block.c | 2 +-
block/gluster.c | 8
block/parallels.c | 2 +-
block/quorum.c | 2 +-
block/replication.c | 2 +-
block/vxhs.c| 4 ++--
hw/net/virtio-net.c | 4 ++--
7 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/block.c b/block.c
index 77e85f13db..93a5fbf60a 100644
--- a/block.c
+++ b/block.c
@@ -6073,7 +6073,7 @@ void bdrv_img_create(const char *filename, const char
*fmt,
/* Parse -o options */
if (options) {
-if (!qemu_opts_do_parse(opts, options, NULL, _err)) {
+if (!qemu_opts_do_parse(opts, options, NULL, errp)) {
goto out;
}
}
diff --git a/block/gluster.c b/block/gluster.c
index c620880f27..4f1448e2bc 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -523,7 +523,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster
*gconf,
/* create opts info from runtime_json_opts list */
opts = qemu_opts_create(_json_opts, NULL, 0, _abort);
-if (!qemu_opts_absorb_qdict(opts, options, _err)) {
+if (!qemu_opts_absorb_qdict(opts, options, errp)) {
goto out;
}
@@ -554,7 +554,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster
*gconf,
/* create opts info from runtime_type_opts list */
opts = qemu_opts_create(_type_opts, NULL, 0, _abort);
-if (!qemu_opts_absorb_qdict(opts, backing_options, _err)) {
+if (!qemu_opts_absorb_qdict(opts, backing_options, errp)) {
goto out;
}
@@ -584,7 +584,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster
*gconf,
if (gsconf->type == SOCKET_ADDRESS_TYPE_INET) {
/* create opts info from runtime_inet_opts list */
opts = qemu_opts_create(_inet_opts, NULL, 0, _abort);
-if (!qemu_opts_absorb_qdict(opts, backing_options, _err)) {
+if (!qemu_opts_absorb_qdict(opts, backing_options, errp)) {
goto out;
}
@@ -632,7 +632,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster
*gconf,
} else {
/* create opts info from runtime_unix_opts list */
opts = qemu_opts_create(_unix_opts, NULL, 0, _abort);
-if (!qemu_opts_absorb_qdict(opts, backing_options, _err)) {
+if (!qemu_opts_absorb_qdict(opts, backing_options, errp)) {
goto out;
}
diff --git a/block/parallels.c b/block/parallels.c
index ef0d92d05c..0397f3894f 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -828,7 +828,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
goto fail_options;
}
-if (!qemu_opts_absorb_qdict(opts, options, _err)) {
+if (!qemu_opts_absorb_qdict(opts, options, errp)) {
goto fail_options;
}
diff --git a/block/quorum.c b/block/quorum.c
index beb3b6dbcc..9ed20e1998 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -922,7 +922,7 @@ static int quorum_open(BlockDriverState *bs, QDict
*options, int flags,
}
opts = qemu_opts_create(_runtime_opts, NULL, 0, _abort);
-if (!qemu_opts_absorb_qdict(opts, options, _err)) {
+if (!qemu_opts_absorb_qdict(opts, options, errp)) {
ret = -EINVAL;
goto exit;
}
diff --git a/block/replication.c b/block/replication.c
index 00a50b095e..7f4ab357a4 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -99,7 +99,7 @@ static int replication_open(BlockDriverState *bs, QDict
*options,
ret = -EINVAL;
opts = qemu_opts_create(_runtime_opts, NULL, 0, _abort);
-if (!qemu_opts_absorb_qdict(opts, options, _err)) {
+if (!qemu_opts_absorb_qdict(opts, options, errp)) {
goto fail;
}
diff --git a/block/vxhs.c b/block/vxhs.c
index 237df4f185..20513a43f4 100644
--- a/block/vxhs.c
+++ b/block/vxhs.c
@@ -318,7 +318,7 @@ static int vxhs_open(BlockDriverState *bs, QDict *options,
opts = qemu_opts_create(_opts, NULL, 0, _abort);
tcp_opts = qemu_opts_create(_tcp_opts, NULL, 0, _abort);
-if (!qemu_opts_absorb_qdict(opts, options, _err)) {
+if (!qemu_opts_absorb_qdict(opts, options, errp)) {
ret = -EINVAL;
goto out;
}
@@ -345,7 +345,7 @@ static int vxhs_open(BlockDriverState *bs, QDict *options,
/* get the 'server.' arguments */
qdict_extract_subqdict(options, _options, VXHS_OPT_SERVER".");
-if (!qemu_opts_absorb_qdict(tcp_opts, backing_options, _err)) {
+if (!qemu_opts_absorb_qdict(tcp_opts, backing_options, errp)) {
ret = -EINVAL;
goto out;
}
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 04b012e487..7502ff0b1e 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3119,8 +3119,8 @@ static bool
[PATCH 26/46] qapi: Smooth another visitor error checking pattern
Convert
visit_type_FOO(v, ..., , );
...
if (err) {
...
}
to
visit_type_FOO(v, ..., , errp);
...
if (!ptr) {
...
}
for functions that set @ptr to non-null / null on success / error.
Eliminate error_propagate() that are now unnecessary. Delete @err
that are now unused.
Signed-off-by: Markus Armbruster
---
block/nfs.c | 7 ++-
block/parallels.c| 7 ++-
block/qcow.c | 7 ++-
block/qcow2.c| 7 ++-
block/qed.c | 7 ++-
block/rbd.c | 7 ++-
block/sheepdog.c | 6 ++
block/ssh.c | 7 ++-
block/vdi.c | 7 ++-
block/vhdx.c | 7 ++-
block/vpc.c | 7 ++-
hw/acpi/core.c | 4 ++--
hw/block/xen-block.c | 6 ++
hw/core/numa.c | 7 +++
monitor/monitor.c| 21 +++--
15 files changed, 36 insertions(+), 78 deletions(-)
diff --git a/block/nfs.c b/block/nfs.c
index b1718d125a..61a249a9fc 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -563,18 +563,15 @@ static BlockdevOptionsNfs
*nfs_options_qdict_to_qapi(QDict *options,
BlockdevOptionsNfs *opts = NULL;
Visitor *v;
const QDictEntry *e;
-Error *local_err = NULL;
v = qobject_input_visitor_new_flat_confused(options, errp);
if (!v) {
return NULL;
}
-visit_type_BlockdevOptionsNfs(v, NULL, , _err);
+visit_type_BlockdevOptionsNfs(v, NULL, , errp);
visit_free(v);
-
-if (local_err) {
-error_propagate(errp, local_err);
+if (!opts) {
return NULL;
}
diff --git a/block/parallels.c b/block/parallels.c
index 0397f3894f..9e85ab995e 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -625,7 +625,6 @@ static int coroutine_fn
parallels_co_create_opts(BlockDriver *drv,
Error **errp)
{
BlockdevCreateOptions *create_options = NULL;
-Error *local_err = NULL;
BlockDriverState *bs = NULL;
QDict *qdict;
Visitor *v;
@@ -668,11 +667,9 @@ static int coroutine_fn
parallels_co_create_opts(BlockDriver *drv,
goto done;
}
-visit_type_BlockdevCreateOptions(v, NULL, _options, _err);
+visit_type_BlockdevCreateOptions(v, NULL, _options, errp);
visit_free(v);
-
-if (local_err) {
-error_propagate(errp, local_err);
+if (!create_options) {
ret = -EINVAL;
goto done;
}
diff --git a/block/qcow.c b/block/qcow.c
index eefa3b63da..1e134f3445 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -941,7 +941,6 @@ static int coroutine_fn qcow_co_create_opts(BlockDriver
*drv,
QDict *qdict;
Visitor *v;
const char *val;
-Error *local_err = NULL;
int ret;
static const QDictRenames opt_renames[] = {
@@ -993,11 +992,9 @@ static int coroutine_fn qcow_co_create_opts(BlockDriver
*drv,
goto fail;
}
-visit_type_BlockdevCreateOptions(v, NULL, _options, _err);
+visit_type_BlockdevCreateOptions(v, NULL, _options, errp);
visit_free(v);
-
-if (local_err) {
-error_propagate(errp, local_err);
+if (!create_options) {
ret = -EINVAL;
goto fail;
}
diff --git a/block/qcow2.c b/block/qcow2.c
index a9137a535b..9ed2396c88 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3683,7 +3683,6 @@ static int coroutine_fn qcow2_co_create_opts(BlockDriver
*drv,
Visitor *v;
BlockDriverState *bs = NULL;
BlockDriverState *data_bs = NULL;
-Error *local_err = NULL;
const char *val;
int ret;
@@ -3779,11 +3778,9 @@ static int coroutine_fn qcow2_co_create_opts(BlockDriver
*drv,
goto finish;
}
-visit_type_BlockdevCreateOptions(v, NULL, _options, _err);
+visit_type_BlockdevCreateOptions(v, NULL, _options, errp);
visit_free(v);
-
-if (local_err) {
-error_propagate(errp, local_err);
+if (!create_options) {
ret = -EINVAL;
goto finish;
}
diff --git a/block/qed.c b/block/qed.c
index e369fd360a..7fa7f880f6 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -729,7 +729,6 @@ static int coroutine_fn bdrv_qed_co_create_opts(BlockDriver
*drv,
QDict *qdict;
Visitor *v;
BlockDriverState *bs = NULL;
-Error *local_err = NULL;
int ret;
static const QDictRenames opt_renames[] = {
@@ -771,11 +770,9 @@ static int coroutine_fn
bdrv_qed_co_create_opts(BlockDriver *drv,
goto fail;
}
-visit_type_BlockdevCreateOptions(v, NULL, _options, _err);
+visit_type_BlockdevCreateOptions(v, NULL, _options, errp);
visit_free(v);
-
-if (local_err) {
-error_propagate(errp, local_err);
+if (!create_options) {
ret = -EINVAL;
goto fail;
}
diff --git a/block/rbd.c b/block/rbd.c
index 617553b022..688074c64b 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -681,7 +681,6 @@ static int qemu_rbd_convert_options(QDict *options,
BlockdevOptionsRbd **opts,
[PATCH 28/46] block/parallels: Simplify parallels_open() after previous commit
Signed-off-by: Markus Armbruster
---
block/parallels.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 9e85ab995e..3c22dfdc9d 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -839,6 +839,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
_err);
g_free(buf);
if (local_err != NULL) {
+error_propagate(errp, local_err);
goto fail_options;
}
@@ -868,15 +869,11 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
fail_format:
error_setg(errp, "Image not in Parallels format");
+fail_options:
ret = -EINVAL;
fail:
qemu_vfree(s->header);
return ret;
-
-fail_options:
-error_propagate(errp, local_err);
-ret = -EINVAL;
-goto fail;
}
--
2.26.2
[PATCH 44/46] qemu-img: Ignore Error objects where the return value suffices
Signed-off-by: Markus Armbruster
---
qemu-img.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 27bf94e7ae..c11bfe0268 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -464,22 +464,18 @@ static int add_old_style_options(const char *fmt,
QemuOpts *opts,
const char *base_filename,
const char *base_fmt)
{
-Error *err = NULL;
-
if (base_filename) {
if (!qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename,
- )) {
+ NULL)) {
error_report("Backing file not supported for file format '%s'",
fmt);
-error_free(err);
return -1;
}
}
if (base_fmt) {
-if (!qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt, )) {
+if (!qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt, NULL)) {
error_report("Backing file format not supported for file "
"format '%s'", fmt);
-error_free(err);
return -1;
}
}
--
2.26.2
[PATCH 39/46] qom: Smooth error checking manually
When foo(..., ) is followed by error_propagate(errp, err), we can
often just as well do foo(..., errp). The previous commit did that
for simple cases with Coccinelle. Do it for a few more manually.
Signed-off-by: Markus Armbruster
---
hw/core/bus.c | 6 +-
hw/s390x/s390-virtio-ccw.c | 15 +++
qom/object.c | 9 ++---
qom/qom-qobject.c | 5 +
target/i386/cpu.c | 19 +--
5 files changed, 16 insertions(+), 38 deletions(-)
diff --git a/hw/core/bus.c b/hw/core/bus.c
index 00d1d31762..6b987b6946 100644
--- a/hw/core/bus.c
+++ b/hw/core/bus.c
@@ -166,11 +166,7 @@ BusState *qbus_create(const char *typename, DeviceState
*parent, const char *nam
bool qbus_realize(BusState *bus, Error **errp)
{
-Error *err = NULL;
-
-object_property_set_bool(OBJECT(bus), "realized", true, );
-error_propagate(errp, err);
-return !err;
+return object_property_set_bool(OBJECT(bus), "realized", true, errp);
}
void qbus_unrealize(BusState *bus)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 62af349c31..877ea2af9d 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -69,20 +69,19 @@ static S390CPU *s390x_new_cpu(const char *typename,
uint32_t core_id,
Error **errp)
{
S390CPU *cpu = S390_CPU(object_new(typename));
-Error *err = NULL;
+S390CPU *ret = NULL;
-if (!object_property_set_int(OBJECT(cpu), "core-id", core_id, )) {
+if (!object_property_set_int(OBJECT(cpu), "core-id", core_id, errp)) {
goto out;
}
-qdev_realize(DEVICE(cpu), NULL, );
+if (!qdev_realize(DEVICE(cpu), NULL, errp)) {
+goto out;
+}
+ret = cpu;
out:
object_unref(OBJECT(cpu));
-if (err) {
-error_propagate(errp, err);
-cpu = NULL;
-}
-return cpu;
+return ret;
}
static void s390_init_cpus(MachineState *machine)
diff --git a/qom/object.c b/qom/object.c
index eb814e41be..684540a09f 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -547,7 +547,6 @@ bool object_initialize_child_with_propsv(Object *parentobj,
void *childobj, size_t size, const char *type,
Error **errp, va_list vargs)
{
-Error *local_err = NULL;
bool ok = false;
Object *obj;
UserCreatable *uc;
@@ -563,7 +562,7 @@ bool object_initialize_child_with_propsv(Object *parentobj,
uc = (UserCreatable *)object_dynamic_cast(obj, TYPE_USER_CREATABLE);
if (uc) {
-if (!user_creatable_complete(uc, _err)) {
+if (!user_creatable_complete(uc, errp)) {
object_unparent(obj);
goto out;
}
@@ -581,8 +580,6 @@ out:
* the reference taken by object_property_add_child().
*/
object_unref(obj);
-
-error_propagate(errp, local_err);
return ok;
}
@@ -735,7 +732,6 @@ Object *object_new_with_propv(const char *typename,
{
Object *obj;
ObjectClass *klass;
-Error *local_err = NULL;
UserCreatable *uc;
klass = object_class_by_name(typename);
@@ -760,7 +756,7 @@ Object *object_new_with_propv(const char *typename,
uc = (UserCreatable *)object_dynamic_cast(obj, TYPE_USER_CREATABLE);
if (uc) {
-if (!user_creatable_complete(uc, _err)) {
+if (!user_creatable_complete(uc, errp)) {
if (id != NULL) {
object_unparent(obj);
}
@@ -772,7 +768,6 @@ Object *object_new_with_propv(const char *typename,
return obj;
error:
-error_propagate(errp, local_err);
object_unref(obj);
return NULL;
}
diff --git a/qom/qom-qobject.c b/qom/qom-qobject.c
index 62ac5e07ac..21ce22de94 100644
--- a/qom/qom-qobject.c
+++ b/qom/qom-qobject.c
@@ -34,15 +34,12 @@ QObject *object_property_get_qobject(Object *obj, const
char *name,
Error **errp)
{
QObject *ret = NULL;
-Error *local_err = NULL;
Visitor *v;
v = qobject_output_visitor_new();
-object_property_get(obj, name, v, _err);
-if (!local_err) {
+if (object_property_get(obj, name, v, errp)) {
visit_complete(v, );
}
-error_propagate(errp, local_err);
visit_free(v);
return ret;
}
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 26ba40656d..b7c9f0fa72 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5251,16 +5251,13 @@ static void x86_cpu_to_dict_full(X86CPU *cpu, QDict
*props)
static void object_apply_props(Object *obj, QDict *props, Error **errp)
{
const QDictEntry *prop;
-Error *err = NULL;
for (prop = qdict_first(props); prop; prop = qdict_next(props, prop)) {
if (!object_property_set_qobject(obj, qdict_entry_key(prop),
- qdict_entry_value(prop), )) {
+ qdict_entry_value(prop), errp)) {
break;
}
}
[PATCH 20/46] block: Avoid error accumulation in bdrv_img_create()
When creating an image fails because the format doesn't support option
"backing_file" or "backing_fmt", bdrv_img_create() first has
qemu_opt_set() put a generic error into @local_err, then puts the real
error into @errp with error_setg(), and then propagates the former to
the latter, which throws away the generic error. A bit complicated,
but works.
Not that qemu_opt_set() returns a useful value, we can simply ignore
the generic error instead.
Signed-off-by: Markus Armbruster
---
block.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block.c b/block.c
index 93a5fbf60a..2dcf9afd61 100644
--- a/block.c
+++ b/block.c
@@ -6087,7 +6087,7 @@ void bdrv_img_create(const char *filename, const char
*fmt,
if (base_filename) {
if (!qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename,
- _err)) {
+ NULL)) {
error_setg(errp, "Backing file not supported for file format '%s'",
fmt);
goto out;
@@ -6095,7 +6095,7 @@ void bdrv_img_create(const char *filename, const char
*fmt,
}
if (base_fmt) {
-if (!qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt, _err)) {
+if (!qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt, NULL)) {
error_setg(errp, "Backing file format not supported for file "
"format '%s'", fmt);
goto out;
--
2.26.2
[PATCH 30/46] s390x/pci: Fix harmless mistake in zpci's property fid's setter
s390_pci_set_fid() sets zpci->fid_defined to true even when
visit_type_uint32() failed. Reproducer: "-device zpci,fid=junk".
Harmless in practice, because qdev_device_add() then fails, throwing
away @zpci. Fix it anyway.
Cc: Matthew Rosato
Cc: Cornelia Huck
Signed-off-by: Markus Armbruster
---
hw/s390x/s390-pci-bus.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index be8535304e..2e0eab1c69 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -1265,7 +1265,9 @@ static void s390_pci_set_fid(Object *obj, Visitor *v,
const char *name,
return;
}
-visit_type_uint32(v, name, ptr, errp);
+if (!visit_type_uint32(v, name, ptr, errp)) {
+return;
+}
zpci->fid_defined = true;
}
--
2.26.2
[PATCH 25/46] qapi: Smooth visitor error checking in generated code
Use visitor functions' return values to check for failure. Eliminate
error_propagate() that are now unnecessary. Delete @err that are now
unused.
Signed-off-by: Markus Armbruster
---
docs/devel/qapi-code-gen.txt | 60 ++--
scripts/qapi/commands.py | 22 ++---
scripts/qapi/visit.py| 57 ++
3 files changed, 55 insertions(+), 84 deletions(-)
diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index 9bfc57063c..69eede6c28 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -1420,8 +1420,6 @@ Example:
bool visit_type_UserDefOne_members(Visitor *v, UserDefOne *obj, Error
**errp)
{
-Error *err = NULL;
-
if (!visit_type_int(v, "integer", >integer, errp)) {
return false;
}
@@ -1430,13 +1428,12 @@ Example:
return false;
}
}
-error_propagate(errp, err);
-return !err;
+return true;
}
bool visit_type_UserDefOne(Visitor *v, const char *name, UserDefOne **obj,
Error **errp)
{
-Error *err = NULL;
+bool ok = false;
if (!visit_start_struct(v, name, (void **)obj, sizeof(UserDefOne),
errp)) {
return false;
@@ -1446,24 +1443,22 @@ Example:
assert(visit_is_dealloc(v));
goto out_obj;
}
-visit_type_UserDefOne_members(v, *obj, );
-if (err) {
+if (!visit_type_UserDefOne_members(v, *obj, errp)) {
goto out_obj;
}
-visit_check_struct(v, );
+ok = visit_check_struct(v, errp);
out_obj:
visit_end_struct(v, (void **)obj);
-if (err && visit_is_input(v)) {
+if (!ok && visit_is_input(v)) {
qapi_free_UserDefOne(*obj);
*obj = NULL;
}
-error_propagate(errp, err);
-return !err;
+return ok;
}
bool visit_type_UserDefOneList(Visitor *v, const char *name,
UserDefOneList **obj, Error **errp)
{
-Error *err = NULL;
+bool ok = false;
UserDefOneList *tail;
size_t size = sizeof(**obj);
@@ -1473,33 +1468,27 @@ Example:
for (tail = *obj; tail;
tail = (UserDefOneList *)visit_next_list(v, (GenericList *)tail,
size)) {
-visit_type_UserDefOne(v, NULL, >value, );
-if (err) {
-break;
+if (!visit_type_UserDefOne(v, NULL, >value, errp)) {
+goto out_obj;
}
}
-if (!err) {
-visit_check_list(v, );
-}
+ok = visit_check_list(v, errp);
+out_obj:
visit_end_list(v, (void **)obj);
-if (err && visit_is_input(v)) {
+if (!ok && visit_is_input(v)) {
qapi_free_UserDefOneList(*obj);
*obj = NULL;
}
-error_propagate(errp, err);
-return !err;
+return ok;
}
bool visit_type_q_obj_my_command_arg_members(Visitor *v,
q_obj_my_command_arg *obj, Error **errp)
{
-Error *err = NULL;
-
if (!visit_type_UserDefOneList(v, "arg1", >arg1, errp)) {
return false;
}
-error_propagate(errp, err);
-return !err;
+return true;
}
[Uninteresting stuff omitted...]
@@ -1554,15 +1543,12 @@ Example:
static void qmp_marshal_output_UserDefOne(UserDefOne *ret_in, QObject
**ret_out, Error **errp)
{
-Error *err = NULL;
Visitor *v;
v = qobject_output_visitor_new(ret_out);
-visit_type_UserDefOne(v, "unused", _in, );
-if (!err) {
+if (visit_type_UserDefOne(v, "unused", _in, errp)) {
visit_complete(v, ret_out);
}
-error_propagate(errp, err);
visit_free(v);
v = qapi_dealloc_visitor_new();
visit_type_UserDefOne(v, "unused", _in, NULL);
@@ -1572,33 +1558,32 @@ Example:
void qmp_marshal_my_command(QDict *args, QObject **ret, Error **errp)
{
Error *err = NULL;
+bool ok = false;
Visitor *v;
UserDefOne *retval;
q_obj_my_command_arg arg = {0};
v = qobject_input_visitor_new(QOBJECT(args));
-visit_start_struct(v, NULL, NULL, 0, );
-if (err) {
+if (!visit_start_struct(v, NULL, NULL, 0, errp)) {
goto out;
}
-visit_type_q_obj_my_command_arg_members(v, , );
-if (!err) {
-visit_check_struct(v, );
+if (visit_type_q_obj_my_command_arg_members(v, , errp)) {
+ok = visit_check_struct(v, errp);
}
visit_end_struct(v, NULL);
-if (err) {
+if (!ok) {
goto out;
}
retval = qmp_my_command(arg.arg1, );
+error_propagate(errp, err);
if (err) {
goto out;
}
-qmp_marshal_output_UserDefOne(retval,
[PATCH 24/46] qapi: Smooth error checking manually
When foo(..., ) is followed by error_propagate(errp, err), we can
often just as well do foo(..., errp). The previous commit did that
for simple cases with Coccinelle. Do it for a few more manually.
Signed-off-by: Markus Armbruster
---
accel/kvm/kvm-all.c | 50 ++-
block/throttle-groups.c | 5 ++--
bootdevice.c | 4 ++--
hw/core/qdev-properties.c | 12 +-
hw/ide/qdev.c | 4 ++--
hw/mem/nvdimm.c | 9 +++
hw/net/ne2000-isa.c | 4 ++--
hw/usb/dev-storage.c | 4 ++--
net/net.c | 8 ++-
qom/object.c | 30 +++
10 files changed, 59 insertions(+), 71 deletions(-)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index be02b8e07a..0b921cd24c 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -3119,37 +3119,33 @@ static void kvm_set_kernel_irqchip(Object *obj, Visitor
*v,
const char *name, void *opaque,
Error **errp)
{
-Error *err = NULL;
KVMState *s = KVM_STATE(obj);
OnOffSplit mode;
-visit_type_OnOffSplit(v, name, , );
-if (err) {
-error_propagate(errp, err);
+if (!visit_type_OnOffSplit(v, name, , errp)) {
return;
-} else {
-switch (mode) {
-case ON_OFF_SPLIT_ON:
-s->kernel_irqchip_allowed = true;
-s->kernel_irqchip_required = true;
-s->kernel_irqchip_split = ON_OFF_AUTO_OFF;
-break;
-case ON_OFF_SPLIT_OFF:
-s->kernel_irqchip_allowed = false;
-s->kernel_irqchip_required = false;
-s->kernel_irqchip_split = ON_OFF_AUTO_OFF;
-break;
-case ON_OFF_SPLIT_SPLIT:
-s->kernel_irqchip_allowed = true;
-s->kernel_irqchip_required = true;
-s->kernel_irqchip_split = ON_OFF_AUTO_ON;
-break;
-default:
-/* The value was checked in visit_type_OnOffSplit() above. If
- * we get here, then something is wrong in QEMU.
- */
-abort();
-}
+}
+switch (mode) {
+case ON_OFF_SPLIT_ON:
+s->kernel_irqchip_allowed = true;
+s->kernel_irqchip_required = true;
+s->kernel_irqchip_split = ON_OFF_AUTO_OFF;
+break;
+case ON_OFF_SPLIT_OFF:
+s->kernel_irqchip_allowed = false;
+s->kernel_irqchip_required = false;
+s->kernel_irqchip_split = ON_OFF_AUTO_OFF;
+break;
+case ON_OFF_SPLIT_SPLIT:
+s->kernel_irqchip_allowed = true;
+s->kernel_irqchip_required = true;
+s->kernel_irqchip_split = ON_OFF_AUTO_ON;
+break;
+default:
+/* The value was checked in visit_type_OnOffSplit() above. If
+ * we get here, then something is wrong in QEMU.
+ */
+abort();
}
}
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index dba2bb6451..03a53c89ea 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -889,8 +889,8 @@ static void throttle_group_set_limits(Object *obj, Visitor
*v,
ThrottleLimits *argp;
Error *local_err = NULL;
-if (!visit_type_ThrottleLimits(v, name, , _err)) {
-goto ret;
+if (!visit_type_ThrottleLimits(v, name, , errp)) {
+return;
}
qemu_mutex_lock(>lock);
throttle_get_config(>ts, );
@@ -902,7 +902,6 @@ static void throttle_group_set_limits(Object *obj, Visitor
*v,
unlock:
qemu_mutex_unlock(>lock);
-ret:
qapi_free_ThrottleLimits(argp);
error_propagate(errp, local_err);
return;
diff --git a/bootdevice.c b/bootdevice.c
index fb09d3c668..769f40c77d 100644
--- a/bootdevice.c
+++ b/bootdevice.c
@@ -297,8 +297,8 @@ static void device_set_bootindex(Object *obj, Visitor *v,
const char *name,
int32_t boot_index;
Error *local_err = NULL;
-if (!visit_type_int32(v, name, _index, _err)) {
-goto out;
+if (!visit_type_int32(v, name, _index, errp)) {
+return;
}
/* check whether bootindex is present in fw_boot_order list */
check_boot_index(boot_index, _err);
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index fa88a8885c..11e7d27ccc 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -664,15 +664,15 @@ static void set_pci_devfn(Object *obj, Visitor *v, const
char *name,
if (!visit_type_str(v, name, , _err)) {
error_free(local_err);
local_err = NULL;
-visit_type_int32(v, name, , _err);
-if (local_err) {
-error_propagate(errp, local_err);
-} else if (value < -1 || value > 255) {
+if (!visit_type_int32(v, name, , errp)) {
+return;
+}
+if (value < -1 || value > 255) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
name ? name : "null", "pci_devfn");
-}
[PATCH 33/46] qom: Crash more nicely on object_property_get_link() failure
Pass _abort instead of NULL where the returned value is
dereferenced or asserted to be non-null.
Signed-off-by: Markus Armbruster
---
hw/core/platform-bus.c | 5 +++--
hw/ppc/spapr_drc.c | 3 ++-
hw/ppc/spapr_hcall.c | 3 ++-
hw/ppc/spapr_pci_nvlink2.c | 8 +---
ui/vnc.c | 2 +-
5 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/hw/core/platform-bus.c b/hw/core/platform-bus.c
index d494e5cec1..b700ff45fe 100644
--- a/hw/core/platform-bus.c
+++ b/hw/core/platform-bus.c
@@ -22,6 +22,7 @@
#include "qemu/osdep.h"
#include "hw/platform-bus.h"
#include "hw/qdev-properties.h"
+#include "qapi/error.h"
#include "qemu/error-report.h"
#include "qemu/module.h"
@@ -63,8 +64,8 @@ hwaddr platform_bus_get_mmio_addr(PlatformBusDevice *pbus,
SysBusDevice *sbdev,
return -1;
}
-parent_mr = object_property_get_link(OBJECT(sbdev_mr), "container", NULL);
-
+parent_mr = object_property_get_link(OBJECT(sbdev_mr), "container",
+ _abort);
assert(parent_mr);
if (parent_mr != pbus_mr_obj) {
/* MMIO region is not mapped on platform bus */
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index c8e8ba2ee8..43d12bc33a 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -867,7 +867,8 @@ int spapr_dt_drc(void *fdt, int offset, Object *owner,
uint32_t drc_type_mask)
continue;
}
-obj = object_property_get_link(root_container, prop->name, NULL);
+obj = object_property_get_link(root_container, prop->name,
+ _abort);
drc = SPAPR_DR_CONNECTOR(obj);
drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 0f54988f2e..c1d01228c6 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1655,7 +1655,8 @@ static void
spapr_handle_transient_dev_before_cas(SpaprMachineState *spapr)
continue;
}
drc = SPAPR_DR_CONNECTOR(object_property_get_link(drc_container,
- prop->name, NULL));
+ prop->name,
+ _abort));
if (spapr_drc_transient(drc)) {
spapr_drc_reset(drc);
diff --git a/hw/ppc/spapr_pci_nvlink2.c b/hw/ppc/spapr_pci_nvlink2.c
index 8332d5694e..e4e09a93e6 100644
--- a/hw/ppc/spapr_pci_nvlink2.c
+++ b/hw/ppc/spapr_pci_nvlink2.c
@@ -141,9 +141,10 @@ static void spapr_phb_pci_collect_nvgpu(PCIBus *bus,
PCIDevice *pdev,
if (tgt) {
Error *local_err = NULL;
SpaprPhbPciNvGpuConfig *nvgpus = opaque;
-Object *mr_gpu = object_property_get_link(po, "nvlink2-mr[0]", NULL);
+Object *mr_gpu = object_property_get_link(po, "nvlink2-mr[0]",
+ _abort);
Object *mr_npu = object_property_get_link(po, "nvlink2-atsd-mr[0]",
- NULL);
+ _abort);
g_assert(mr_gpu || mr_npu);
if (mr_gpu) {
@@ -358,7 +359,8 @@ void spapr_phb_nvgpu_ram_populate_dt(SpaprPhbState *sphb,
void *fdt)
for (i = 0; i < sphb->nvgpus->num; ++i) {
SpaprPhbPciNvGpuSlot *nvslot = >nvgpus->slots[i];
Object *nv_mrobj = object_property_get_link(OBJECT(nvslot->gpdev),
-"nvlink2-mr[0]", NULL);
+"nvlink2-mr[0]",
+_abort);
uint32_t associativity[] = {
cpu_to_be32(0x4),
SPAPR_GPU_NUMA_ID,
diff --git a/ui/vnc.c b/ui/vnc.c
index 527ad25124..f006aa1afd 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -568,7 +568,7 @@ VncInfo2List *qmp_query_vnc_servers(Error **errp)
>vencrypt, >has_vencrypt);
if (vd->dcl.con) {
dev = DEVICE(object_property_get_link(OBJECT(vd->dcl.con),
- "device", NULL));
+ "device", _abort));
info->has_display = true;
info->display = g_strdup(dev->id);
}
--
2.26.2
[PATCH 05/46] virtio-crypto-pci: Tidy up virtio_crypto_pci_realize()
virtio_crypto_pci_realize() continues after realization of its
"virtio-crypto-device" fails. Only an object_property_set_link()
follows; looks harmless to me. Tidy up anyway: return after failure,
just like virtio_rng_pci_realize() does.
Cc: "Gonglei (Arei)"
Cc: Michael S. Tsirkin
Signed-off-by: Markus Armbruster
---
hw/virtio/virtio-crypto-pci.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/virtio/virtio-crypto-pci.c b/hw/virtio/virtio-crypto-pci.c
index 72be531c95..0755722288 100644
--- a/hw/virtio/virtio-crypto-pci.c
+++ b/hw/virtio/virtio-crypto-pci.c
@@ -54,7 +54,9 @@ static void virtio_crypto_pci_realize(VirtIOPCIProxy
*vpci_dev, Error **errp)
}
virtio_pci_force_virtio_1(vpci_dev);
-qdev_realize(vdev, BUS(_dev->bus), errp);
+if (!qdev_realize(vdev, BUS(_dev->bus), errp)) {
+return;
+}
object_property_set_link(OBJECT(vcrypto),
OBJECT(vcrypto->vdev.conf.cryptodev), "cryptodev",
NULL);
--
2.26.2
[PATCH 03/46] qdev: Smooth error checking of qdev_realize() & friends
Convert
foo(..., );
if (err) {
...
}
to
if (!foo(..., )) {
...
}
for qdev_realize(), qdev_realize_and_unref(), qbus_realize() and their
wrappers isa_realize_and_unref(), pci_realize_and_unref(),
sysbus_realize(), sysbus_realize_and_unref(), usb_realize_and_unref().
Coccinelle script:
@@
identifier fun = {isa_realize_and_unref, pci_realize_and_unref,
qbus_realize, qdev_realize, qdev_realize_and_unref, sysbus_realize,
sysbus_realize_and_unref, usb_realize_and_unref};
expression list args, args2;
typedef Error;
Error *err;
identifier errp;
@@
- fun(args, , args2);
- if (err) {
+ if (!fun(args, errp, args2)) {
... when != err
- error_propagate(errp, err);
...
}
@@
identifier fun = {isa_realize_and_unref, pci_realize_and_unref,
qbus_realize, qdev_realize, qdev_realize_and_unref, sysbus_realize,
sysbus_realize_and_unref, usb_realize_and_unref};
expression list args, args2;
typedef Error;
Error *err;
@@
- fun(args, , args2);
- if (err) {
+ if (!fun(args, , args2)) {
...
}
Fails to convert hw/arm/armsse.c, because Coccinelle gets confused by
ARMSSE being used both as typedef and function-like macro there.
Convert manually.
Eliminate error_propagate() that are now unnecessary. Delete @err
that are now unused. Clean up whitespace.
Signed-off-by: Markus Armbruster
---
hw/arm/allwinner-a10.c | 21 ++-
hw/arm/armsse.c | 104
hw/arm/armv7m.c | 12 +---
hw/arm/aspeed_ast2600.c | 68 ++---
hw/arm/aspeed_soc.c | 60 +-
hw/arm/bcm2835_peripherals.c| 60 +-
hw/arm/bcm2836.c| 12 +---
hw/arm/cubieboard.c | 3 +-
hw/arm/digic.c | 12 +---
hw/arm/digic_boards.c | 3 +-
hw/arm/fsl-imx25.c | 44 --
hw/arm/fsl-imx31.c | 32 +++---
hw/arm/fsl-imx6.c | 48 ---
hw/arm/msf2-soc.c | 21 ++-
hw/arm/nrf51_soc.c | 24 ++--
hw/arm/stm32f205_soc.c | 29 +++--
hw/arm/stm32f405_soc.c | 32 +++---
hw/arm/xlnx-zynqmp.c| 61 +--
hw/block/fdc.c | 4 +-
hw/block/xen-block.c| 3 +-
hw/char/serial-pci-multi.c | 5 +-
hw/char/serial-pci.c| 5 +-
hw/char/serial.c| 10 +--
hw/core/cpu.c | 3 +-
hw/cpu/a15mpcore.c | 5 +-
hw/cpu/a9mpcore.c | 21 ++-
hw/cpu/arm11mpcore.c| 17 ++
hw/cpu/realview_mpcore.c| 9 +--
hw/display/virtio-gpu-pci.c | 6 +-
hw/display/virtio-vga.c | 5 +-
hw/intc/armv7m_nvic.c | 9 +--
hw/intc/pnv_xive.c | 8 +--
hw/intc/realview_gic.c | 5 +-
hw/intc/spapr_xive.c| 8 +--
hw/intc/xics.c | 5 +-
hw/intc/xive.c | 3 +-
hw/isa/piix4.c | 5 +-
hw/microblaze/xlnx-zynqmp-pmu.c | 9 +--
hw/mips/cps.c | 17 ++
hw/misc/macio/cuda.c| 5 +-
hw/misc/macio/macio.c | 25 ++--
hw/misc/macio/pmu.c | 5 +-
hw/pci-host/pnv_phb3.c | 13 +---
hw/pci-host/pnv_phb4.c | 5 +-
hw/pci-host/pnv_phb4_pec.c | 5 +-
hw/ppc/e500.c | 5 +-
hw/ppc/pnv.c| 53
hw/ppc/pnv_core.c | 4 +-
hw/ppc/pnv_psi.c| 9 +--
hw/ppc/spapr_cpu_core.c | 3 +-
hw/ppc/spapr_irq.c | 5 +-
hw/riscv/opentitan.c| 9 +--
hw/riscv/sifive_e.c | 6 +-
hw/riscv/sifive_u.c | 5 +-
hw/s390x/event-facility.c | 13 ++--
hw/s390x/s390-pci-bus.c | 3 +-
hw/s390x/sclp.c | 3 +-
hw/s390x/virtio-ccw-crypto.c| 5 +-
hw/s390x/virtio-ccw-rng.c | 5 +-
hw/scsi/scsi-bus.c | 4 +-
hw/sd/aspeed_sdhci.c| 4 +-
hw/sd/ssi-sd.c | 3 +-
hw/usb/bus.c| 3 +-
hw/virtio/virtio-rng-pci.c | 5 +-
qdev-monitor.c | 3 +-
65 files changed, 248 insertions(+), 768 deletions(-)
diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
index 52e0d83760..3e45aa4141 100644
--- a/hw/arm/allwinner-a10.c
+++ b/hw/arm/allwinner-a10.c
@@ -72,17 +72,12 @@ static void aw_a10_realize(DeviceState *dev, Error **errp)
{
AwA10State *s = AW_A10(dev);
SysBusDevice *sysbusdev;
-Error *err = NULL;
-qdev_realize(DEVICE(>cpu), NULL, );
-if (err != NULL) {
-error_propagate(errp, err);
+if (!qdev_realize(DEVICE(>cpu), NULL, errp)) {
[PATCH 31/46] qom: Use error_reportf_err() instead of g_printerr() in examples
Signed-off-by: Markus Armbruster
---
include/qom/object.h | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/include/qom/object.h b/include/qom/object.h
index 94a61ccc3f..b70edd8cd9 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -671,8 +671,7 @@ Object *object_new(const char *typename);
* NULL);
*
* if (!obj) {
- * g_printerr("Cannot create memory backend: %s\n",
- *error_get_pretty(err));
+ * error_reportf_err(err, "Cannot create memory backend: ");
* }
*
*
@@ -739,8 +738,7 @@ void object_apply_compat_props(Object *obj);
* NULL);
*
* if (!obj) {
- * g_printerr("Cannot set properties: %s\n",
- *error_get_pretty(err));
+ * error_reportf_err(err, "Cannot set properties: ");
* }
*
*
--
2.26.2
[PATCH 27/46] qapi: Purge error_propagate() from QAPI core
Signed-off-by: Markus Armbruster
---
qapi/qapi-visit-core.c | 40 +++-
1 file changed, 19 insertions(+), 21 deletions(-)
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 5a9c47aabf..7e5f40e7f0 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -39,19 +39,18 @@ void visit_free(Visitor *v)
bool visit_start_struct(Visitor *v, const char *name, void **obj,
size_t size, Error **errp)
{
-Error *err = NULL;
+bool ok;
trace_visit_start_struct(v, name, obj, size);
if (obj) {
assert(size);
assert(!(v->type & VISITOR_OUTPUT) || *obj);
}
-v->start_struct(v, name, obj, size, );
+ok = v->start_struct(v, name, obj, size, errp);
if (obj && (v->type & VISITOR_INPUT)) {
-assert(!err != !*obj);
+assert(ok != !*obj);
}
-error_propagate(errp, err);
-return !err;
+return ok;
}
bool visit_check_struct(Visitor *v, Error **errp)
@@ -69,16 +68,15 @@ void visit_end_struct(Visitor *v, void **obj)
bool visit_start_list(Visitor *v, const char *name, GenericList **list,
size_t size, Error **errp)
{
-Error *err = NULL;
+bool ok;
assert(!list || size >= sizeof(GenericList));
trace_visit_start_list(v, name, list, size);
-v->start_list(v, name, list, size, );
+ok = v->start_list(v, name, list, size, errp);
if (list && (v->type & VISITOR_INPUT)) {
-assert(!(err && *list));
+assert(ok || !*list);
}
-error_propagate(errp, err);
-return !err;
+return ok;
}
GenericList *visit_next_list(Visitor *v, GenericList *tail, size_t size)
@@ -104,19 +102,20 @@ bool visit_start_alternate(Visitor *v, const char *name,
GenericAlternate **obj, size_t size,
Error **errp)
{
-Error *err = NULL;
+bool ok;
assert(obj && size >= sizeof(GenericAlternate));
assert(!(v->type & VISITOR_OUTPUT) || *obj);
trace_visit_start_alternate(v, name, obj, size);
-if (v->start_alternate) {
-v->start_alternate(v, name, obj, size, );
+if (!v->start_alternate) {
+assert(!(v->type & VISITOR_INPUT));
+return true;
}
+ok = v->start_alternate(v, name, obj, size, errp);
if (v->type & VISITOR_INPUT) {
-assert(v->start_alternate && !err != !*obj);
+assert(ok != !*obj);
}
-error_propagate(errp, err);
-return !err;
+return ok;
}
void visit_end_alternate(Visitor *v, void **obj)
@@ -309,7 +308,7 @@ bool visit_type_bool(Visitor *v, const char *name, bool
*obj, Error **errp)
bool visit_type_str(Visitor *v, const char *name, char **obj, Error **errp)
{
-Error *err = NULL;
+bool ok;
assert(obj);
/* TODO: Fix callers to not pass NULL when they mean "", so that we
@@ -317,12 +316,11 @@ bool visit_type_str(Visitor *v, const char *name, char
**obj, Error **errp)
assert(!(v->type & VISITOR_OUTPUT) || *obj);
*/
trace_visit_type_str(v, name, obj);
-v->type_str(v, name, obj, );
+ok = v->type_str(v, name, obj, errp);
if (v->type & VISITOR_INPUT) {
-assert(!err != !*obj);
+assert(ok != !*obj);
}
-error_propagate(errp, err);
-return !err;
+return ok;
}
bool visit_type_number(Visitor *v, const char *name, double *obj,
--
2.26.2
[PATCH 08/46] error: Avoid unnecessary error_propagate() after error_setg()
Replace
error_setg(, ...);
error_propagate(errp, err);
by
error_setg(errp, ...);
Related pattern:
if (...) {
error_setg(, ...);
goto out;
}
...
out:
error_propagate(errp, err);
return;
When all paths to label out are that way, replace by
if (...) {
error_setg(errp, ...);
return;
}
and delete the label along with the error_propagate().
When we have at most one other path that actually needs to propagate,
and maybe one at the end that where propagation is unnecessary, e.g.
foo(..., );
if (err) {
goto out;
}
...
bar(..., );
out:
error_propagate(errp, err);
return;
move the error_propagate() to where it's needed, like
if (...) {
foo(..., );
error_propagate(errp, err);
return;
}
...
bar(..., errp);
return;
and transform the error_setg() as above.
Bonus: the elimination of gotos will make later patches in this series
easier to review.
Candidates for conversion tracked down with this Coccinelle script:
@@
identifier err, errp;
expression list args;
@@
-error_setg(, args);
+error_setg(errp, args);
... when != err
error_propagate(errp, err);
Signed-off-by: Markus Armbruster
---
backends/cryptodev.c| 11 +++---
backends/hostmem-file.c | 19 +++---
backends/hostmem-memfd.c| 15
backends/hostmem.c | 27 ++
block/throttle-groups.c | 22 +--
hw/hyperv/vmbus.c | 5 +--
hw/i386/pc.c| 35 ++
hw/mem/nvdimm.c | 17 -
hw/mem/pc-dimm.c| 14 +++
hw/misc/aspeed_sdmc.c | 3 +-
hw/ppc/rs6000_mc.c | 9 ++---
hw/ppc/spapr.c | 73 -
hw/ppc/spapr_pci.c | 14 +++
hw/s390x/ipl.c | 23 +---
hw/s390x/sclp.c | 12 ++
hw/xen/xen_pt_config_init.c | 3 +-
iothread.c | 12 +++---
net/colo-compare.c | 20 --
net/dump.c | 10 ++---
net/filter-buffer.c | 10 ++---
qga/commands-win32.c| 16 +++-
21 files changed, 151 insertions(+), 219 deletions(-)
diff --git a/backends/cryptodev.c b/backends/cryptodev.c
index a3841c4e41..8645f297e3 100644
--- a/backends/cryptodev.c
+++ b/backends/cryptodev.c
@@ -159,16 +159,15 @@ cryptodev_backend_set_queues(Object *obj, Visitor *v,
const char *name,
visit_type_uint32(v, name, , _err);
if (local_err) {
-goto out;
+error_propagate(errp, local_err);
+return;
}
if (!value) {
-error_setg(_err, "Property '%s.%s' doesn't take value '%"
- PRIu32 "'", object_get_typename(obj), name, value);
-goto out;
+error_setg(errp, "Property '%s.%s' doesn't take value '%" PRIu32 "'",
+ object_get_typename(obj), name, value);
+return;
}
backend->conf.peers.queues = value;
-out:
-error_propagate(errp, local_err);
}
static void
diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
index cdabb412e6..098c8e6e64 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -114,19 +114,17 @@ static void file_memory_backend_set_align(Object *o,
Visitor *v,
uint64_t val;
if (host_memory_backend_mr_inited(backend)) {
-error_setg(_err, "cannot change property '%s' of %s",
- name, object_get_typename(o));
-goto out;
+error_setg(errp, "cannot change property '%s' of %s", name,
+ object_get_typename(o));
+return;
}
visit_type_size(v, name, , _err);
if (local_err) {
-goto out;
+error_propagate(errp, local_err);
+return;
}
fb->align = val;
-
- out:
-error_propagate(errp, local_err);
}
static bool file_memory_backend_get_pmem(Object *o, Error **errp)
@@ -140,7 +138,6 @@ static void file_memory_backend_set_pmem(Object *o, bool
value, Error **errp)
HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o);
if (host_memory_backend_mr_inited(backend)) {
-
error_setg(errp, "cannot change property 'pmem' of %s.",
object_get_typename(o));
return;
@@ -148,13 +145,9 @@ static void file_memory_backend_set_pmem(Object *o, bool
value, Error **errp)
#ifndef CONFIG_LIBPMEM
if (value) {
-Error *local_err = NULL;
-
-error_setg(_err,
- "Lack of libpmem support while setting the 'pmem=on'"
+error_setg(errp, "Lack of libpmem support while setting the 'pmem=on'"
" of %s. We can't ensure data persistence.",
object_get_typename(o));
-error_propagate(errp, local_err);
return;
}
#endif
diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
index 1b5e4bfe0d..9582c7f8fc 100644
---
[PATCH 13/46] qemu-option: Simplify around find_default_by_name()
Signed-off-by: Markus Armbruster
---
util/qemu-option.c | 13 -
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/util/qemu-option.c b/util/qemu-option.c
index ddcf3072c5..d9293814b4 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -286,11 +286,9 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name)
opt = qemu_opt_find(opts, name);
if (!opt) {
def_val = find_default_by_name(opts, name);
-if (def_val) {
-return def_val;
-}
+return def_val;
}
-return opt ? opt->str : NULL;
+return opt->str;
}
void qemu_opt_iter_init(QemuOptsIter *iter, QemuOpts *opts, const char *name)
@@ -320,7 +318,7 @@ char *qemu_opt_get_del(QemuOpts *opts, const char *name)
{
QemuOpt *opt;
const char *def_val;
-char *str = NULL;
+char *str;
if (opts == NULL) {
return NULL;
@@ -329,10 +327,7 @@ char *qemu_opt_get_del(QemuOpts *opts, const char *name)
opt = qemu_opt_find(opts, name);
if (!opt) {
def_val = find_default_by_name(opts, name);
-if (def_val) {
-str = g_strdup(def_val);
-}
-return str;
+return g_strdup(def_val);
}
str = opt->str;
opt->str = NULL;
--
2.26.2
[PATCH 07/46] error: Avoid more error_propagate() when error is not used here
When all we do with an Error we receive into a local variable is
propagating to somewhere else, we can just as well receive it there
right away. The previous commit did that for simple cases with
Coccinelle. Do it for a few more manually.
Signed-off-by: Markus Armbruster
---
blockdev.c | 5 +
hw/core/numa.c | 44 ++--
qdev-monitor.c | 11 ---
3 files changed, 19 insertions(+), 41 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index b66863c42a..73736a4eaf 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1009,13 +1009,10 @@ DriveInfo *drive_new(QemuOpts *all_opts,
BlockInterfaceType block_default_type,
}
/* Actual block device init: Functionality shared with blockdev-add */
-blk = blockdev_init(filename, bs_opts, _err);
+blk = blockdev_init(filename, bs_opts, errp);
bs_opts = NULL;
if (!blk) {
-error_propagate(errp, local_err);
goto fail;
-} else {
-assert(!local_err);
}
/* Create legacy DriveInfo */
diff --git a/hw/core/numa.c b/hw/core/numa.c
index 5f81900f88..aa8c6be210 100644
--- a/hw/core/numa.c
+++ b/hw/core/numa.c
@@ -449,40 +449,33 @@ void parse_numa_hmat_cache(MachineState *ms,
NumaHmatCacheOptions *node,
void set_numa_options(MachineState *ms, NumaOptions *object, Error **errp)
{
-Error *err = NULL;
-
if (!ms->numa_state) {
error_setg(errp, "NUMA is not supported by this machine-type");
-goto end;
+return;
}
switch (object->type) {
case NUMA_OPTIONS_TYPE_NODE:
-parse_numa_node(ms, >u.node, );
-if (err) {
-goto end;
-}
+parse_numa_node(ms, >u.node, errp);
break;
case NUMA_OPTIONS_TYPE_DIST:
-parse_numa_distance(ms, >u.dist, );
-if (err) {
-goto end;
-}
+parse_numa_distance(ms, >u.dist, errp);
break;
case NUMA_OPTIONS_TYPE_CPU:
if (!object->u.cpu.has_node_id) {
-error_setg(, "Missing mandatory node-id property");
-goto end;
+error_setg(errp, "Missing mandatory node-id property");
+return;
}
if (!ms->numa_state->nodes[object->u.cpu.node_id].present) {
-error_setg(, "Invalid node-id=%" PRId64 ", NUMA node must be "
-"defined with -numa node,nodeid=ID before it's used with "
-"-numa cpu,node-id=ID", object->u.cpu.node_id);
-goto end;
+error_setg(errp, "Invalid node-id=%" PRId64 ", NUMA node must be "
+ "defined with -numa node,nodeid=ID before it's used
with "
+ "-numa cpu,node-id=ID", object->u.cpu.node_id);
+return;
}
-machine_set_cpu_numa_node(ms, qapi_NumaCpuOptions_base(>u.cpu),
- );
+machine_set_cpu_numa_node(ms,
+ qapi_NumaCpuOptions_base(>u.cpu),
+ errp);
break;
case NUMA_OPTIONS_TYPE_HMAT_LB:
if (!ms->numa_state->hmat_enabled) {
@@ -492,10 +485,7 @@ void set_numa_options(MachineState *ms, NumaOptions
*object, Error **errp)
return;
}
-parse_numa_hmat_lb(ms->numa_state, >u.hmat_lb, );
-if (err) {
-goto end;
-}
+parse_numa_hmat_lb(ms->numa_state, >u.hmat_lb, errp);
break;
case NUMA_OPTIONS_TYPE_HMAT_CACHE:
if (!ms->numa_state->hmat_enabled) {
@@ -505,17 +495,11 @@ void set_numa_options(MachineState *ms, NumaOptions
*object, Error **errp)
return;
}
-parse_numa_hmat_cache(ms, >u.hmat_cache, );
-if (err) {
-goto end;
-}
+parse_numa_hmat_cache(ms, >u.hmat_cache, errp);
break;
default:
abort();
}
-
-end:
-error_propagate(errp, err);
}
static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
diff --git a/qdev-monitor.c b/qdev-monitor.c
index e38030429b..40c34bb9cf 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -600,7 +600,6 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
const char *driver, *path;
DeviceState *dev = NULL;
BusState *bus = NULL;
-Error *err = NULL;
bool hide;
driver = qemu_opt_get(opts, "driver");
@@ -655,15 +654,14 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
dev = qdev_new(driver);
/* Check whether the hotplug is allowed by the machine */
-if (qdev_hotplug && !qdev_hotplug_allowed(dev, )) {
+if (qdev_hotplug && !qdev_hotplug_allowed(dev, errp)) {
/* Error must be set in the machine hook */
-assert(err);
goto err_del_dev;
}
if (!bus && qdev_hotplug && !qdev_get_machine_hotplug_handler(dev)) {
/* No bus, no machine hotplug handler --> device is not hotpluggable */
-error_setg(, "Device '%s' can not be
[PATCH 15/46] qemu-option: Tidy up opt_set() not to free arguments on failure
opt_set() frees its argument @value on failure. Slightly unclean;
functions ideally do nothing on failure.
To tidy this up, move opt_create() from opt_set() into its callers,
along with the cleanup.
Signed-off-by: Markus Armbruster
---
util/qemu-option.c | 33 ++---
1 file changed, 18 insertions(+), 15 deletions(-)
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 3cdf0c0800..14946e81f2 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -519,36 +519,39 @@ static QemuOpt *opt_create(QemuOpts *opts, const char
*name, char *value,
return opt;
}
-static void opt_set(QemuOpts *opts, const char *name, char *value,
-bool prepend, bool *help_wanted, Error **errp)
+static bool opt_set(QemuOpts *opts, QemuOpt *opt, bool *help_wanted,
+Error **errp)
{
-QemuOpt *opt;
const QemuOptDesc *desc;
Error *local_err = NULL;
-desc = find_desc_by_name(opts->list->desc, name);
+desc = find_desc_by_name(opts->list->desc, opt->name);
if (!desc && !opts_accepts_any(opts)) {
-g_free(value);
-error_setg(errp, QERR_INVALID_PARAMETER, name);
-if (help_wanted && is_help_option(name)) {
+error_setg(errp, QERR_INVALID_PARAMETER, opt->name);
+if (help_wanted && is_help_option(opt->name)) {
*help_wanted = true;
}
-return;
+return false;
}
-opt = opt_create(opts, name, value, prepend);
opt->desc = desc;
qemu_opt_parse(opt, _err);
if (local_err) {
error_propagate(errp, local_err);
-qemu_opt_del(opt);
+return false;
}
+
+return true;
}
void qemu_opt_set(QemuOpts *opts, const char *name, const char *value,
Error **errp)
{
-opt_set(opts, name, g_strdup(value), false, NULL, errp);
+QemuOpt *opt = opt_create(opts, name, g_strdup(value), false);
+
+if (!opt_set(opts, opt, NULL, errp)) {
+qemu_opt_del(opt);
+}
}
void qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val,
@@ -820,9 +823,9 @@ static void opts_do_parse(QemuOpts *opts, const char
*params,
const char *firstname, bool prepend,
bool *help_wanted, Error **errp)
{
-Error *local_err = NULL;
char *option, *value;
const char *p;
+QemuOpt *opt;
for (p = params; *p;) {
p = get_opt_name_value(p, firstname, , );
@@ -834,10 +837,10 @@ static void opts_do_parse(QemuOpts *opts, const char
*params,
continue;
}
-opt_set(opts, option, value, prepend, help_wanted, _err);
+opt = opt_create(opts, option, value, prepend);
g_free(option);
-if (local_err) {
-error_propagate(errp, local_err);
+if (!opt_set(opts, opt, help_wanted, errp)) {
+qemu_opt_del(opt);
return;
}
}
--
2.26.2
[PATCH 14/46] qemu-option: Factor out helper opt_create()
There is just one use so far. The next commit will add more.
Signed-off-by: Markus Armbruster
---
util/qemu-option.c | 27 ++-
1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/util/qemu-option.c b/util/qemu-option.c
index d9293814b4..3cdf0c0800 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -502,6 +502,23 @@ int qemu_opt_unset(QemuOpts *opts, const char *name)
}
}
+static QemuOpt *opt_create(QemuOpts *opts, const char *name, char *value,
+ bool prepend)
+{
+QemuOpt *opt = g_malloc0(sizeof(*opt));
+
+opt->name = g_strdup(name);
+opt->str = value;
+opt->opts = opts;
+if (prepend) {
+QTAILQ_INSERT_HEAD(>head, opt, next);
+} else {
+QTAILQ_INSERT_TAIL(>head, opt, next);
+}
+
+return opt;
+}
+
static void opt_set(QemuOpts *opts, const char *name, char *value,
bool prepend, bool *help_wanted, Error **errp)
{
@@ -519,16 +536,8 @@ static void opt_set(QemuOpts *opts, const char *name, char
*value,
return;
}
-opt = g_malloc0(sizeof(*opt));
-opt->name = g_strdup(name);
-opt->opts = opts;
-if (prepend) {
-QTAILQ_INSERT_HEAD(>head, opt, next);
-} else {
-QTAILQ_INSERT_TAIL(>head, opt, next);
-}
+opt = opt_create(opts, name, value, prepend);
opt->desc = desc;
-opt->str = value;
qemu_opt_parse(opt, _err);
if (local_err) {
error_propagate(errp, local_err);
--
2.26.2
[PATCH 04/46] macio: Tidy up error handling in macio_newworld_realize()
macio_newworld_realize() effectively ignores ns->gpio realization
errors, leaking the Error object. Fortunately, macio_gpio_realize()
can't actually fail. Tidy up.
Cc: Mark Cave-Ayland
Cc: David Gibson
Signed-off-by: Markus Armbruster
---
hw/misc/macio/macio.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index 7cfe357cc4..bedf10e77b 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -329,7 +329,9 @@ static void macio_newworld_realize(PCIDevice *d, Error
**errp)
_abort);
memory_region_add_subregion(>bar, 0x50,
sysbus_mmio_get_region(sysbus_dev, 0));
-qdev_realize(DEVICE(>gpio), BUS(>macio_bus), );
+if (!qdev_realize(DEVICE(>gpio), BUS(>macio_bus), errp)) {
+return;
+}
/* PMU */
object_initialize_child(OBJECT(s), "pmu", >pmu, TYPE_VIA_PMU);
--
2.26.2
[PATCH 06/46] error: Avoid error_propagate() when error is not used here
When all we do with an Error we receive into a local variable is
propagating to somewhere else, we can just as well receive it there
right away. Coccinelle script:
@@
identifier fun, err, errp;
expression list args;
@@
-fun(args, );
+fun(args, errp);
... when != err
when strict
-error_propagate(errp, err);
@@
identifier fun, err, errp;
expression list args;
expression ret;
@@
-ret = fun(args, );
+ret = fun(args, errp);
... when != err
when strict
-error_propagate(errp, err);
@@
identifier fun, err, errp;
expression list args;
expression ret;
@@
-ret = fun(args, );
+ret = fun(args, errp);
... when != err
when strict
if (
(
ret
|
!ret
|
ret == 0
|
ret != 0
|
ret < 0
|
ret != NULL
|
ret == NULL
)
)
{
... when != err
when strict
-error_propagate(errp, err);
...
}
@@
identifier fun, err, errp;
expression list args;
@@
if (
(
-fun(args, )
+fun(args, errp)
|
-!fun(args, )
+!fun(args, errp)
|
-fun(args, ) == 0
+fun(args, errp) == 0
|
-fun(args, ) != 0
+fun(args, errp) != 0
|
-fun(args, ) < 0
+fun(args, errp) < 0
|
-fun(args, ) == NULL
+fun(args, errp) == NULL
|
-fun(args, ) != NULL
+fun(args, errp) != NULL
)
)
{
... when != err;
when strict
-error_propagate(errp, err);
...
}
The first two rules are prone to fail with "error_propagate(...)
... reachable by inconsistent control-flow paths". Script without
them re-run where that happens.
Manually double-check @err is not used afterwards. Dereferencing it
would be use after free, but checking whether it's null would be
legitimate. One such change to qbus_realize() backed out.
Suboptimal line breaks tweaked manually.
Signed-off-by: Markus Armbruster
---
backends/cryptodev-vhost-user.c | 3 +--
block.c | 3 +--
block/file-posix.c | 8 ++--
block/parallels.c | 3 +--
block/qcow.c| 3 +--
block/qcow2.c | 10 +++---
block/qed.c | 3 +--
block/replication.c | 4 +---
block/vhdx.c| 3 +--
block/vmdk.c| 7 ++-
block/vpc.c | 3 +--
blockdev.c | 10 +++---
dump/dump.c | 7 ++-
hw/intc/xics_kvm.c | 3 +--
hw/s390x/s390-pci-bus.c | 4 +---
hw/usb/bus.c| 4 +---
hw/vfio/pci.c | 10 +++---
iothread.c | 3 +--
qdev-monitor.c | 3 +--
qga/commands-win32.c| 3 +--
util/main-loop.c| 4 +---
util/qemu-option.c | 3 +--
22 files changed, 31 insertions(+), 73 deletions(-)
diff --git a/backends/cryptodev-vhost-user.c b/backends/cryptodev-vhost-user.c
index 8b8cbc4223..dbe5a8aae6 100644
--- a/backends/cryptodev-vhost-user.c
+++ b/backends/cryptodev-vhost-user.c
@@ -209,8 +209,7 @@ static void cryptodev_vhost_user_init(
backend->conf.peers.ccs[i] = cc;
if (i == 0) {
-if (!qemu_chr_fe_init(>chr, chr, _err)) {
-error_propagate(errp, local_err);
+if (!qemu_chr_fe_init(>chr, chr, errp)) {
return;
}
}
diff --git a/block.c b/block.c
index 6dbcb7e083..30a72bc4c2 100644
--- a/block.c
+++ b/block.c
@@ -5680,10 +5680,9 @@ static void coroutine_fn
bdrv_co_invalidate_cache(BlockDriverState *bs,
if (bs->open_flags & BDRV_O_INACTIVE) {
bs->open_flags &= ~BDRV_O_INACTIVE;
bdrv_get_cumulative_perm(bs, , _perm);
-ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL,
_err);
+ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, errp);
if (ret < 0) {
bs->open_flags |= BDRV_O_INACTIVE;
-error_propagate(errp, local_err);
return;
}
bdrv_set_perm(bs, perm, shared_perm);
diff --git a/block/file-posix.c b/block/file-posix.c
index 3ab8f5a0fa..2294bf5d00 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -3336,7 +3336,6 @@ static int hdev_open(BlockDriverState *bs, QDict
*options, int flags,
Error **errp)
{
BDRVRawState *s = bs->opaque;
-Error *local_err = NULL;
int ret;
#if defined(__APPLE__) && defined(__MACH__)
@@ -3401,9 +3400,8 @@
[PATCH 12/46] qemu-option: Factor out helper find_default_by_name()
Signed-off-by: Markus Armbruster
---
util/qemu-option.c | 47 ++
1 file changed, 27 insertions(+), 20 deletions(-)
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 9941005c91..ddcf3072c5 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -142,6 +142,13 @@ static const QemuOptDesc *find_desc_by_name(const
QemuOptDesc *desc,
return NULL;
}
+static const char *find_default_by_name(QemuOpts *opts, const char *name)
+{
+const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
+
+return desc ? desc->def_value_str : NULL;
+}
+
void parse_option_size(const char *name, const char *value,
uint64_t *ret, Error **errp)
{
@@ -270,7 +277,7 @@ static void qemu_opt_del_all(QemuOpts *opts, const char
*name)
const char *qemu_opt_get(QemuOpts *opts, const char *name)
{
QemuOpt *opt;
-const QemuOptDesc *desc;
+const char *def_val;
if (opts == NULL) {
return NULL;
@@ -278,9 +285,9 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name)
opt = qemu_opt_find(opts, name);
if (!opt) {
-desc = find_desc_by_name(opts->list->desc, name);
-if (desc && desc->def_value_str) {
-return desc->def_value_str;
+def_val = find_default_by_name(opts, name);
+if (def_val) {
+return def_val;
}
}
return opt ? opt->str : NULL;
@@ -312,7 +319,7 @@ const char *qemu_opt_iter_next(QemuOptsIter *iter)
char *qemu_opt_get_del(QemuOpts *opts, const char *name)
{
QemuOpt *opt;
-const QemuOptDesc *desc;
+const char *def_val;
char *str = NULL;
if (opts == NULL) {
@@ -321,9 +328,9 @@ char *qemu_opt_get_del(QemuOpts *opts, const char *name)
opt = qemu_opt_find(opts, name);
if (!opt) {
-desc = find_desc_by_name(opts->list->desc, name);
-if (desc && desc->def_value_str) {
-str = g_strdup(desc->def_value_str);
+def_val = find_default_by_name(opts, name);
+if (def_val) {
+str = g_strdup(def_val);
}
return str;
}
@@ -349,7 +356,7 @@ static bool qemu_opt_get_bool_helper(QemuOpts *opts, const
char *name,
bool defval, bool del)
{
QemuOpt *opt;
-const QemuOptDesc *desc;
+const char *def_val;
bool ret = defval;
if (opts == NULL) {
@@ -358,9 +365,9 @@ static bool qemu_opt_get_bool_helper(QemuOpts *opts, const
char *name,
opt = qemu_opt_find(opts, name);
if (opt == NULL) {
-desc = find_desc_by_name(opts->list->desc, name);
-if (desc && desc->def_value_str) {
-parse_option_bool(name, desc->def_value_str, , _abort);
+def_val = find_default_by_name(opts, name);
+if (def_val) {
+parse_option_bool(name, def_val, , _abort);
}
return ret;
}
@@ -386,7 +393,7 @@ static uint64_t qemu_opt_get_number_helper(QemuOpts *opts,
const char *name,
uint64_t defval, bool del)
{
QemuOpt *opt;
-const QemuOptDesc *desc;
+const char *def_val;
uint64_t ret = defval;
if (opts == NULL) {
@@ -395,9 +402,9 @@ static uint64_t qemu_opt_get_number_helper(QemuOpts *opts,
const char *name,
opt = qemu_opt_find(opts, name);
if (opt == NULL) {
-desc = find_desc_by_name(opts->list->desc, name);
-if (desc && desc->def_value_str) {
-parse_option_number(name, desc->def_value_str, , _abort);
+def_val = find_default_by_name(opts, name);
+if (def_val) {
+parse_option_number(name, def_val, , _abort);
}
return ret;
}
@@ -424,7 +431,7 @@ static uint64_t qemu_opt_get_size_helper(QemuOpts *opts,
const char *name,
uint64_t defval, bool del)
{
QemuOpt *opt;
-const QemuOptDesc *desc;
+const char *def_val;
uint64_t ret = defval;
if (opts == NULL) {
@@ -433,9 +440,9 @@ static uint64_t qemu_opt_get_size_helper(QemuOpts *opts,
const char *name,
opt = qemu_opt_find(opts, name);
if (opt == NULL) {
-desc = find_desc_by_name(opts->list->desc, name);
-if (desc && desc->def_value_str) {
-parse_option_size(name, desc->def_value_str, , _abort);
+def_val = find_default_by_name(opts, name);
+if (def_val) {
+parse_option_size(name, def_val, , _abort);
}
return ret;
}
--
2.26.2
[PATCH 09/46] error: Avoid error_propagate() after migrate_add_blocker()
When migrate_add_blocker(blocker, ) is followed by
error_propagate(errp, err), we can often just as well do
migrate_add_blocker(..., errp).
Do that with this Coccinelle script:
@@
expression blocker, err, errp;
expression ret;
@@
-ret = migrate_add_blocker(blocker, );
-if (err) {
+ret = migrate_add_blocker(blocker, errp);
+if (ret < 0) {
... when != err;
-error_propagate(errp, err);
...
}
@@
expression blocker, err, errp;
@@
-migrate_add_blocker(blocker, );
-if (err) {
+if (migrate_add_blocker(blocker, errp) < 0) {
... when != err;
-error_propagate(errp, err);
...
}
Double-check @err is not used afterwards. Dereferencing it would be
use after free, but checking whether it's null would be legitimate.
Signed-off-by: Markus Armbruster
---
block/parallels.c| 5 ++---
block/qcow.c | 6 ++
block/vdi.c | 6 ++
block/vhdx.c | 5 ++---
block/vmdk.c | 6 ++
block/vpc.c | 5 ++---
block/vvfat.c| 5 ++---
hw/display/virtio-gpu-base.c | 5 +
hw/intc/arm_gic_kvm.c| 4 +---
hw/intc/arm_gicv3_its_kvm.c | 4 +---
hw/intc/arm_gicv3_kvm.c | 4 +---
hw/misc/ivshmem.c| 4 +---
hw/scsi/vhost-scsi.c | 4 +---
13 files changed, 20 insertions(+), 43 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index a84ec6a518..860dbb80a2 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -862,9 +862,8 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
error_setg(>migration_blocker, "The Parallels format used by node '%s' "
"does not support live migration",
bdrv_get_device_or_node_name(bs));
-ret = migrate_add_blocker(s->migration_blocker, _err);
-if (local_err) {
-error_propagate(errp, local_err);
+ret = migrate_add_blocker(s->migration_blocker, errp);
+if (ret < 0) {
error_free(s->migration_blocker);
goto fail;
}
diff --git a/block/qcow.c b/block/qcow.c
index dca2a1fe7d..eefa3b63da 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -121,7 +121,6 @@ static int qcow_open(BlockDriverState *bs, QDict *options,
int flags,
unsigned int len, i, shift;
int ret;
QCowHeader header;
-Error *local_err = NULL;
QCryptoBlockOpenOptions *crypto_opts = NULL;
unsigned int cflags = 0;
QDict *encryptopts = NULL;
@@ -314,9 +313,8 @@ static int qcow_open(BlockDriverState *bs, QDict *options,
int flags,
error_setg(>migration_blocker, "The qcow format used by node '%s' "
"does not support live migration",
bdrv_get_device_or_node_name(bs));
-ret = migrate_add_blocker(s->migration_blocker, _err);
-if (local_err) {
-error_propagate(errp, local_err);
+ret = migrate_add_blocker(s->migration_blocker, errp);
+if (ret < 0) {
error_free(s->migration_blocker);
goto fail;
}
diff --git a/block/vdi.c b/block/vdi.c
index 2f506a01ba..5fca67f52d 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -375,7 +375,6 @@ static int vdi_open(BlockDriverState *bs, QDict *options,
int flags,
VdiHeader header;
size_t bmap_size;
int ret;
-Error *local_err = NULL;
QemuUUID uuid_link, uuid_parent;
bs->file = bdrv_open_child(NULL, options, "file", bs, _of_bds,
@@ -496,9 +495,8 @@ static int vdi_open(BlockDriverState *bs, QDict *options,
int flags,
error_setg(>migration_blocker, "The vdi format used by node '%s' "
"does not support live migration",
bdrv_get_device_or_node_name(bs));
-ret = migrate_add_blocker(s->migration_blocker, _err);
-if (local_err) {
-error_propagate(errp, local_err);
+ret = migrate_add_blocker(s->migration_blocker, errp);
+if (ret < 0) {
error_free(s->migration_blocker);
goto fail_free_bmap;
}
diff --git a/block/vhdx.c b/block/vhdx.c
index ac5a9094c4..b9ef4ff074 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1089,9 +1089,8 @@ static int vhdx_open(BlockDriverState *bs, QDict
*options, int flags,
error_setg(>migration_blocker, "The vhdx format used by node '%s' "
"does not support live migration",
bdrv_get_device_or_node_name(bs));
-ret = migrate_add_blocker(s->migration_blocker, _err);
-if (local_err) {
-error_propagate(errp, local_err);
+ret = migrate_add_blocker(s->migration_blocker, errp);
+if (ret < 0) {
error_free(s->migration_blocker);
goto fail;
}
diff --git a/block/vmdk.c b/block/vmdk.c
index 9a09193f3b..28cec50f38 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1263,7 +1263,6 @@ static int vmdk_open(BlockDriverState *bs, QDict
*options, int flags,
int ret;
BDRVVmdkState *s
[PATCH 01/46] error: Improve examples in error.h's big comment
Show errp instead of where is actually unusual. Add a
missing declaration. Add a second error pileup example.
Signed-off-by: Markus Armbruster
---
include/qapi/error.h | 19 +++
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/include/qapi/error.h b/include/qapi/error.h
index ad5b6e896d..1a5ea25e12 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -16,15 +16,15 @@
* Error reporting system loosely patterned after Glib's GError.
*
* Create an error:
- * error_setg(, "situation normal, all fouled up");
+ * error_setg(errp, "situation normal, all fouled up");
*
* Create an error and add additional explanation:
- * error_setg(, "invalid quark");
- * error_append_hint(, "Valid quarks are up, down, strange, "
+ * error_setg(errp, "invalid quark");
+ * error_append_hint(errp, "Valid quarks are up, down, strange, "
* "charm, top, bottom.\n");
*
* Do *not* contract this to
- * error_setg(, "invalid quark\n"
+ * error_setg(errp, "invalid quark\n" // WRONG!
*"Valid quarks are up, down, strange, charm, top, bottom.");
*
* Report an error to the current monitor if we have one, else stderr:
@@ -108,12 +108,23 @@
* }
*
* Do *not* "optimize" this to
+ * Error *err = NULL;
* foo(arg, );
* bar(arg, ); // WRONG!
* if (err) {
* handle the error...
* }
* because this may pass a non-null err to bar().
+ *
+ * Likewise, do *not*
+ * Error *err = NULL;
+ * if (cond1) {
+ * error_setg(err, ...);
+ * }
+ * if (cond2) {
+ * error_setg(err, ...); // WRONG!
+ * }
+ * because this may pass a non-null err to error_setg().
*/
#ifndef ERROR_H
--
2.26.2
[PATCH 10/46] qemu-option: Check return value instead of @err where convenient
Convert uses like
opts = qemu_opts_create(..., );
if (err) {
...
}
to
opts = qemu_opts_create(..., );
if (!opts) {
...
}
Eliminate error_propagate() that are now unnecessary. Delete @err
that are now unused.
Signed-off-by: Markus Armbruster
---
block/parallels.c | 4 ++--
blockdev.c | 5 ++---
qdev-monitor.c | 6 ++
util/qemu-config.c | 10 --
util/qemu-option.c | 12
5 files changed, 14 insertions(+), 23 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 860dbb80a2..b15c9ac28d 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -823,8 +823,8 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
}
}
-opts = qemu_opts_create(_runtime_opts, NULL, 0, _err);
-if (local_err != NULL) {
+opts = qemu_opts_create(_runtime_opts, NULL, 0, errp);
+if (!opts) {
goto fail_options;
}
diff --git a/blockdev.c b/blockdev.c
index 73736a4eaf..481f36c543 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -504,9 +504,8 @@ static BlockBackend *blockdev_init(const char *file, QDict
*bs_opts,
/* Check common options by copying from bs_opts to opts, all other options
* stay in bs_opts for processing by bdrv_open(). */
id = qdict_get_try_str(bs_opts, "id");
-opts = qemu_opts_create(_common_drive_opts, id, 1, );
-if (error) {
-error_propagate(errp, error);
+opts = qemu_opts_create(_common_drive_opts, id, 1, errp);
+if (!opts) {
goto err_no_opts;
}
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 40c34bb9cf..3143dd2afb 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -792,13 +792,11 @@ void hmp_info_qdm(Monitor *mon, const QDict *qdict)
void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
{
-Error *local_err = NULL;
QemuOpts *opts;
DeviceState *dev;
-opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, _err);
-if (local_err) {
-error_propagate(errp, local_err);
+opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, errp);
+if (!opts) {
return;
}
if (!monitor_cur_is_qmp() && qdev_device_help(opts)) {
diff --git a/util/qemu-config.c b/util/qemu-config.c
index 772f5a219e..c0d0e9b8ef 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -493,9 +493,8 @@ static void config_parse_qdict_section(QDict *options,
QemuOptsList *opts,
goto out;
}
-subopts = qemu_opts_create(opts, NULL, 0, _err);
-if (local_err) {
-error_propagate(errp, local_err);
+subopts = qemu_opts_create(opts, NULL, 0, errp);
+if (!subopts) {
goto out;
}
@@ -538,10 +537,9 @@ static void config_parse_qdict_section(QDict *options,
QemuOptsList *opts,
}
opt_name = g_strdup_printf("%s.%u", opts->name, i++);
-subopts = qemu_opts_create(opts, opt_name, 1, _err);
+subopts = qemu_opts_create(opts, opt_name, 1, errp);
g_free(opt_name);
-if (local_err) {
-error_propagate(errp, local_err);
+if (!subopts) {
goto out;
}
diff --git a/util/qemu-option.c b/util/qemu-option.c
index fd667ea523..6119f971a4 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -670,11 +670,9 @@ void qemu_opts_set(QemuOptsList *list, const char *id,
const char *name, const char *value, Error **errp)
{
QemuOpts *opts;
-Error *local_err = NULL;
-opts = qemu_opts_create(list, id, 1, _err);
-if (local_err) {
-error_propagate(errp, local_err);
+opts = qemu_opts_create(list, id, 1, errp);
+if (!opts) {
return;
}
qemu_opt_set(opts, name, value, errp);
@@ -1011,10 +1009,8 @@ QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const
QDict *qdict,
QemuOpts *opts;
const QDictEntry *entry;
-opts = qemu_opts_create(list, qdict_get_try_str(qdict, "id"), 1,
-_err);
-if (local_err) {
-error_propagate(errp, local_err);
+opts = qemu_opts_create(list, qdict_get_try_str(qdict, "id"), 1, errp);
+if (!opts) {
return NULL;
}
--
2.26.2
