* [Qemu-devel] [PATCH v2 01/35] error: Fix use of error_prepend() with &error_fatal, &error_abort
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
@ 2018-10-15 11:52 ` Markus Armbruster
2018-10-15 18:49 ` Eric Blake
2018-10-15 22:52 ` Philippe Mathieu-Daudé
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 02/35] Use error_fatal to simplify obvious fatal errors (again) Markus Armbruster
` (33 subsequent siblings)
34 siblings, 2 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:52 UTC (permalink / raw)
To: qemu-devel; +Cc: Fei Li
>From include/qapi/error.h:
* Pass an existing error to the caller with the message modified:
* error_propagate(errp, err);
* error_prepend(errp, "Could not frobnicate '%s': ", name);
Fei Li pointed out that doing error_propagate() first doesn't work
well when @errp is &error_fatal or &error_abort: the error_prepend()
is never reached.
Since I doubt fixing the documentation will stop people from getting
it wrong, introduce error_propagate_prepend(), in the hope that it
lures people away from using its constituents in the wrong order.
Update the instructions in error.h accordingly.
Convert existing error_prepend() next to error_propagate to
error_propagate_prepend(). If any of these get reached with
&error_fatal or &error_abort, the error messages improve. I didn't
check whether that's the case anywhere.
Cc: Fei Li <fli@suse.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
block.c | 6 +++---
block/qcow2.c | 4 ++--
block/qed.c | 4 ++--
hw/9pfs/9p-local.c | 4 ++--
hw/intc/xics.c | 15 +++++++++------
hw/ppc/pnv_core.c | 4 ++--
hw/ppc/spapr_pci.c | 7 +++----
hw/timer/aspeed_timer.c | 3 +--
hw/usb/bus.c | 5 +++--
hw/vfio/pci.c | 3 +--
include/qapi/error.h | 14 ++++++++++++++
migration/migration.c | 12 ++++++------
util/error.c | 13 +++++++++++++
13 files changed, 61 insertions(+), 33 deletions(-)
diff --git a/block.c b/block.c
index 7710b399a3..5d51419d21 100644
--- a/block.c
+++ b/block.c
@@ -4697,9 +4697,9 @@ bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp)
assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
if (!QLIST_EMPTY(&bs->op_blockers[op])) {
blocker = QLIST_FIRST(&bs->op_blockers[op]);
- error_propagate(errp, error_copy(blocker->reason));
- error_prepend(errp, "Node '%s' is busy: ",
- bdrv_get_device_or_node_name(bs));
+ error_propagate_prepend(errp, error_copy(blocker->reason),
+ "Node '%s' is busy: ",
+ bdrv_get_device_or_node_name(bs));
return true;
}
return false;
diff --git a/block/qcow2.c b/block/qcow2.c
index 7277feda13..4f8d2fa7bd 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2208,8 +2208,8 @@ static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
qemu_co_mutex_unlock(&s->lock);
qobject_unref(options);
if (local_err) {
- error_propagate(errp, local_err);
- error_prepend(errp, "Could not reopen qcow2 layer: ");
+ error_propagate_prepend(errp, local_err,
+ "Could not reopen qcow2 layer: ");
bs->drv = NULL;
return;
} else if (ret < 0) {
diff --git a/block/qed.c b/block/qed.c
index 689ea9d4d5..9377c0b7ad 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1606,8 +1606,8 @@ static void coroutine_fn bdrv_qed_co_invalidate_cache(BlockDriverState *bs,
ret = bdrv_qed_do_open(bs, NULL, bs->open_flags, &local_err);
qemu_co_mutex_unlock(&s->table_lock);
if (local_err) {
- error_propagate(errp, local_err);
- error_prepend(errp, "Could not reopen qed layer: ");
+ error_propagate_prepend(errp, local_err,
+ "Could not reopen qed layer: ");
return;
} else if (ret < 0) {
error_setg_errno(errp, -ret, "Could not reopen qed layer");
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index c30f4f26bd..08e673a79c 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1509,8 +1509,8 @@ static int local_parse_opts(QemuOpts *opts, FsDriverEntry *fse, Error **errp)
fsdev_throttle_parse_opts(opts, &fse->fst, &local_err);
if (local_err) {
- error_propagate(errp, local_err);
- error_prepend(errp, "invalid throttle configuration: ");
+ error_propagate_prepend(errp, local_err,
+ "invalid throttle configuration: ");
return -1;
}
diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index c90c893228..406efee064 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -320,8 +320,9 @@ static void icp_realize(DeviceState *dev, Error **errp)
obj = object_property_get_link(OBJECT(dev), ICP_PROP_XICS, &err);
if (!obj) {
- error_propagate(errp, err);
- error_prepend(errp, "required link '" ICP_PROP_XICS "' not found: ");
+ error_propagate_prepend(errp, err,
+ "required link '" ICP_PROP_XICS
+ "' not found: ");
return;
}
@@ -329,8 +330,9 @@ static void icp_realize(DeviceState *dev, Error **errp)
obj = object_property_get_link(OBJECT(dev), ICP_PROP_CPU, &err);
if (!obj) {
- error_propagate(errp, err);
- error_prepend(errp, "required link '" ICP_PROP_CPU "' not found: ");
+ error_propagate_prepend(errp, err,
+ "required link '" ICP_PROP_CPU
+ "' not found: ");
return;
}
@@ -624,8 +626,9 @@ static void ics_base_realize(DeviceState *dev, Error **errp)
obj = object_property_get_link(OBJECT(dev), ICS_PROP_XICS, &err);
if (!obj) {
- error_propagate(errp, err);
- error_prepend(errp, "required link '" ICS_PROP_XICS "' not found: ");
+ error_propagate_prepend(errp, err,
+ "required link '" ICS_PROP_XICS
+ "' not found: ");
return;
}
ics->xics = XICS_FABRIC(obj);
diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index 9750464bf4..ad1bcc7990 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -148,8 +148,8 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
chip = object_property_get_link(OBJECT(dev), "chip", &local_err);
if (!chip) {
- error_propagate(errp, local_err);
- error_prepend(errp, "required link 'chip' not found: ");
+ error_propagate_prepend(errp, local_err,
+ "required link 'chip' not found: ");
return;
}
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index c2271e6ed4..58afa46204 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1724,16 +1724,15 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
if (smc->legacy_irq_allocation) {
irq = spapr_irq_findone(spapr, &local_err);
if (local_err) {
- error_propagate(errp, local_err);
- error_prepend(errp, "can't allocate LSIs: ");
+ error_propagate_prepend(errp, local_err,
+ "can't allocate LSIs: ");
return;
}
}
spapr_irq_claim(spapr, irq, true, &local_err);
if (local_err) {
- error_propagate(errp, local_err);
- error_prepend(errp, "can't allocate LSIs: ");
+ error_propagate_prepend(errp, local_err, "can't allocate LSIs: ");
return;
}
diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
index 54b400b94a..5c786e5128 100644
--- a/hw/timer/aspeed_timer.c
+++ b/hw/timer/aspeed_timer.c
@@ -454,8 +454,7 @@ static void aspeed_timer_realize(DeviceState *dev, Error **errp)
obj = object_property_get_link(OBJECT(dev), "scu", &err);
if (!obj) {
- error_propagate(errp, err);
- error_prepend(errp, "required link 'scu' not found: ");
+ error_propagate_prepend(errp, err, "required link 'scu' not found: ");
return;
}
s->scu = ASPEED_SCU(obj);
diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index 11f7720d71..bf796d67e6 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -340,8 +340,9 @@ static USBDevice *usb_try_create_simple(USBBus *bus, const char *name,
}
object_property_set_bool(OBJECT(dev), true, "realized", &err);
if (err) {
- error_propagate(errp, err);
- error_prepend(errp, "Failed to initialize USB device '%s': ", name);
+ error_propagate_prepend(errp, err,
+ "Failed to initialize USB device '%s': ",
+ name);
return NULL;
}
return dev;
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 866f0deeb7..8c95fc648a 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1280,8 +1280,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
if (ret == -ENOTSUP) {
return 0;
}
- error_prepend(&err, "msi_init failed: ");
- error_propagate(errp, err);
+ error_propagate_prepend(errp, err, "msi_init failed: ");
return ret;
}
vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 : 0);
diff --git a/include/qapi/error.h b/include/qapi/error.h
index bcb86a79f5..51b63dd4b5 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -52,8 +52,12 @@
* where Error **errp is a parameter, by convention the last one.
*
* Pass an existing error to the caller with the message modified:
+ * error_propagate_prepend(errp, err);
+ *
+ * Avoid
* error_propagate(errp, err);
* error_prepend(errp, "Could not frobnicate '%s': ", name);
+ * because this fails to prepend when @errp is &error_fatal.
*
* Create a new error and pass it to the caller:
* error_setg(errp, "situation normal, all fouled up");
@@ -215,6 +219,16 @@ void error_setg_win32_internal(Error **errp,
*/
void error_propagate(Error **dst_errp, Error *local_err);
+
+/*
+ * Propagate error object (if any) with some text prepended.
+ * Behaves like
+ * error_prepend(&local_err, fmt, ...);
+ * error_propagate(dst_errp, local_err);
+ */
+void error_propagate_prepend(Error **dst_errp, Error *local_err,
+ const char *fmt, ...);
+
/*
* Prepend some text to @errp's human-readable error message.
* The text is made by formatting @fmt, @ap like vprintf().
diff --git a/migration/migration.c b/migration/migration.c
index d6ae879dc8..f0f299a773 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1546,9 +1546,9 @@ static GSList *migration_blockers;
int migrate_add_blocker(Error *reason, Error **errp)
{
if (migrate_get_current()->only_migratable) {
- error_propagate(errp, error_copy(reason));
- error_prepend(errp, "disallowing migration blocker "
- "(--only_migratable) for: ");
+ error_propagate_prepend(errp, error_copy(reason),
+ "disallowing migration blocker "
+ "(--only_migratable) for: ");
return -EACCES;
}
@@ -1557,9 +1557,9 @@ int migrate_add_blocker(Error *reason, Error **errp)
return 0;
}
- error_propagate(errp, error_copy(reason));
- error_prepend(errp, "disallowing migration blocker (migration in "
- "progress) for: ");
+ error_propagate_prepend(errp, error_copy(reason),
+ "disallowing migration blocker "
+ "(migration in progress) for: ");
return -EBUSY;
}
diff --git a/util/error.c b/util/error.c
index 3efdd69162..b5ccbd8eac 100644
--- a/util/error.c
+++ b/util/error.c
@@ -292,3 +292,16 @@ void error_propagate(Error **dst_errp, Error *local_err)
error_free(local_err);
}
}
+
+void error_propagate_prepend(Error **dst_errp, Error *err,
+ const char *fmt, ...)
+{
+ va_list ap;
+
+ if (dst_errp && !*dst_errp) {
+ va_start(ap, fmt);
+ error_vprepend(&err, fmt, ap);
+ va_end(ap);
+ } /* else error is being ignored, don't bother with prepending */
+ error_propagate(dst_errp, err);
+}
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [Qemu-devel] [PATCH v2 01/35] error: Fix use of error_prepend() with &error_fatal, &error_abort
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 01/35] error: Fix use of error_prepend() with &error_fatal, &error_abort Markus Armbruster
@ 2018-10-15 18:49 ` Eric Blake
2018-10-15 22:52 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 50+ messages in thread
From: Eric Blake @ 2018-10-15 18:49 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: Fei Li
On 10/15/18 6:52 AM, Markus Armbruster wrote:
>>From include/qapi/error.h:
>
> * Pass an existing error to the caller with the message modified:
> * error_propagate(errp, err);
> * error_prepend(errp, "Could not frobnicate '%s': ", name);
>
> Fei Li pointed out that doing error_propagate() first doesn't work
> well when @errp is &error_fatal or &error_abort: the error_prepend()
> is never reached.
>
> Since I doubt fixing the documentation will stop people from getting
> it wrong, introduce error_propagate_prepend(), in the hope that it
> lures people away from using its constituents in the wrong order.
> Update the instructions in error.h accordingly.
>
> Convert existing error_prepend() next to error_propagate to
> error_propagate_prepend(). If any of these get reached with
> &error_fatal or &error_abort, the error messages improve. I didn't
> check whether that's the case anywhere.
>
> Cc: Fei Li <fli@suse.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v2 01/35] error: Fix use of error_prepend() with &error_fatal, &error_abort
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 01/35] error: Fix use of error_prepend() with &error_fatal, &error_abort Markus Armbruster
2018-10-15 18:49 ` Eric Blake
@ 2018-10-15 22:52 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 50+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-15 22:52 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: Fei Li
On 15/10/2018 13:52, Markus Armbruster wrote:
> From include/qapi/error.h:
>
> * Pass an existing error to the caller with the message modified:
> * error_propagate(errp, err);
> * error_prepend(errp, "Could not frobnicate '%s': ", name);
>
> Fei Li pointed out that doing error_propagate() first doesn't work
> well when @errp is &error_fatal or &error_abort: the error_prepend()
> is never reached.
>
> Since I doubt fixing the documentation will stop people from getting
> it wrong, introduce error_propagate_prepend(), in the hope that it
> lures people away from using its constituents in the wrong order.
> Update the instructions in error.h accordingly.
>
> Convert existing error_prepend() next to error_propagate to
> error_propagate_prepend(). If any of these get reached with
> &error_fatal or &error_abort, the error messages improve. I didn't
> check whether that's the case anywhere.
>
> Cc: Fei Li <fli@suse.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> block.c | 6 +++---
> block/qcow2.c | 4 ++--
> block/qed.c | 4 ++--
> hw/9pfs/9p-local.c | 4 ++--
> hw/intc/xics.c | 15 +++++++++------
> hw/ppc/pnv_core.c | 4 ++--
> hw/ppc/spapr_pci.c | 7 +++----
> hw/timer/aspeed_timer.c | 3 +--
> hw/usb/bus.c | 5 +++--
> hw/vfio/pci.c | 3 +--
> include/qapi/error.h | 14 ++++++++++++++
> migration/migration.c | 12 ++++++------
> util/error.c | 13 +++++++++++++
> 13 files changed, 61 insertions(+), 33 deletions(-)
>
> diff --git a/block.c b/block.c
> index 7710b399a3..5d51419d21 100644
> --- a/block.c
> +++ b/block.c
> @@ -4697,9 +4697,9 @@ bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp)
> assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
> if (!QLIST_EMPTY(&bs->op_blockers[op])) {
> blocker = QLIST_FIRST(&bs->op_blockers[op]);
> - error_propagate(errp, error_copy(blocker->reason));
> - error_prepend(errp, "Node '%s' is busy: ",
> - bdrv_get_device_or_node_name(bs));
> + error_propagate_prepend(errp, error_copy(blocker->reason),
> + "Node '%s' is busy: ",
> + bdrv_get_device_or_node_name(bs));
> return true;
> }
> return false;
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 7277feda13..4f8d2fa7bd 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2208,8 +2208,8 @@ static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
> qemu_co_mutex_unlock(&s->lock);
> qobject_unref(options);
> if (local_err) {
> - error_propagate(errp, local_err);
> - error_prepend(errp, "Could not reopen qcow2 layer: ");
> + error_propagate_prepend(errp, local_err,
> + "Could not reopen qcow2 layer: ");
> bs->drv = NULL;
> return;
> } else if (ret < 0) {
> diff --git a/block/qed.c b/block/qed.c
> index 689ea9d4d5..9377c0b7ad 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -1606,8 +1606,8 @@ static void coroutine_fn bdrv_qed_co_invalidate_cache(BlockDriverState *bs,
> ret = bdrv_qed_do_open(bs, NULL, bs->open_flags, &local_err);
> qemu_co_mutex_unlock(&s->table_lock);
> if (local_err) {
> - error_propagate(errp, local_err);
> - error_prepend(errp, "Could not reopen qed layer: ");
> + error_propagate_prepend(errp, local_err,
> + "Could not reopen qed layer: ");
> return;
> } else if (ret < 0) {
> error_setg_errno(errp, -ret, "Could not reopen qed layer");
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index c30f4f26bd..08e673a79c 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -1509,8 +1509,8 @@ static int local_parse_opts(QemuOpts *opts, FsDriverEntry *fse, Error **errp)
>
> fsdev_throttle_parse_opts(opts, &fse->fst, &local_err);
> if (local_err) {
> - error_propagate(errp, local_err);
> - error_prepend(errp, "invalid throttle configuration: ");
> + error_propagate_prepend(errp, local_err,
> + "invalid throttle configuration: ");
> return -1;
> }
>
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index c90c893228..406efee064 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -320,8 +320,9 @@ static void icp_realize(DeviceState *dev, Error **errp)
>
> obj = object_property_get_link(OBJECT(dev), ICP_PROP_XICS, &err);
> if (!obj) {
> - error_propagate(errp, err);
> - error_prepend(errp, "required link '" ICP_PROP_XICS "' not found: ");
> + error_propagate_prepend(errp, err,
> + "required link '" ICP_PROP_XICS
> + "' not found: ");
> return;
> }
>
> @@ -329,8 +330,9 @@ static void icp_realize(DeviceState *dev, Error **errp)
>
> obj = object_property_get_link(OBJECT(dev), ICP_PROP_CPU, &err);
> if (!obj) {
> - error_propagate(errp, err);
> - error_prepend(errp, "required link '" ICP_PROP_CPU "' not found: ");
> + error_propagate_prepend(errp, err,
> + "required link '" ICP_PROP_CPU
> + "' not found: ");
> return;
> }
>
> @@ -624,8 +626,9 @@ static void ics_base_realize(DeviceState *dev, Error **errp)
>
> obj = object_property_get_link(OBJECT(dev), ICS_PROP_XICS, &err);
> if (!obj) {
> - error_propagate(errp, err);
> - error_prepend(errp, "required link '" ICS_PROP_XICS "' not found: ");
> + error_propagate_prepend(errp, err,
> + "required link '" ICS_PROP_XICS
> + "' not found: ");
> return;
> }
> ics->xics = XICS_FABRIC(obj);
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index 9750464bf4..ad1bcc7990 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -148,8 +148,8 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>
> chip = object_property_get_link(OBJECT(dev), "chip", &local_err);
> if (!chip) {
> - error_propagate(errp, local_err);
> - error_prepend(errp, "required link 'chip' not found: ");
> + error_propagate_prepend(errp, local_err,
> + "required link 'chip' not found: ");
> return;
> }
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index c2271e6ed4..58afa46204 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1724,16 +1724,15 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> if (smc->legacy_irq_allocation) {
> irq = spapr_irq_findone(spapr, &local_err);
> if (local_err) {
> - error_propagate(errp, local_err);
> - error_prepend(errp, "can't allocate LSIs: ");
> + error_propagate_prepend(errp, local_err,
> + "can't allocate LSIs: ");
> return;
> }
> }
>
> spapr_irq_claim(spapr, irq, true, &local_err);
> if (local_err) {
> - error_propagate(errp, local_err);
> - error_prepend(errp, "can't allocate LSIs: ");
> + error_propagate_prepend(errp, local_err, "can't allocate LSIs: ");
> return;
> }
>
> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
> index 54b400b94a..5c786e5128 100644
> --- a/hw/timer/aspeed_timer.c
> +++ b/hw/timer/aspeed_timer.c
> @@ -454,8 +454,7 @@ static void aspeed_timer_realize(DeviceState *dev, Error **errp)
>
> obj = object_property_get_link(OBJECT(dev), "scu", &err);
> if (!obj) {
> - error_propagate(errp, err);
> - error_prepend(errp, "required link 'scu' not found: ");
> + error_propagate_prepend(errp, err, "required link 'scu' not found: ");
> return;
> }
> s->scu = ASPEED_SCU(obj);
> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> index 11f7720d71..bf796d67e6 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -340,8 +340,9 @@ static USBDevice *usb_try_create_simple(USBBus *bus, const char *name,
> }
> object_property_set_bool(OBJECT(dev), true, "realized", &err);
> if (err) {
> - error_propagate(errp, err);
> - error_prepend(errp, "Failed to initialize USB device '%s': ", name);
> + error_propagate_prepend(errp, err,
> + "Failed to initialize USB device '%s': ",
> + name);
> return NULL;
> }
> return dev;
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 866f0deeb7..8c95fc648a 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1280,8 +1280,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
> if (ret == -ENOTSUP) {
> return 0;
> }
> - error_prepend(&err, "msi_init failed: ");
> - error_propagate(errp, err);
> + error_propagate_prepend(errp, err, "msi_init failed: ");
> return ret;
> }
> vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 : 0);
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index bcb86a79f5..51b63dd4b5 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -52,8 +52,12 @@
> * where Error **errp is a parameter, by convention the last one.
> *
> * Pass an existing error to the caller with the message modified:
> + * error_propagate_prepend(errp, err);
> + *
> + * Avoid
> * error_propagate(errp, err);
> * error_prepend(errp, "Could not frobnicate '%s': ", name);
> + * because this fails to prepend when @errp is &error_fatal.
> *
> * Create a new error and pass it to the caller:
> * error_setg(errp, "situation normal, all fouled up");
> @@ -215,6 +219,16 @@ void error_setg_win32_internal(Error **errp,
> */
> void error_propagate(Error **dst_errp, Error *local_err);
>
> +
> +/*
> + * Propagate error object (if any) with some text prepended.
> + * Behaves like
> + * error_prepend(&local_err, fmt, ...);
> + * error_propagate(dst_errp, local_err);
> + */
> +void error_propagate_prepend(Error **dst_errp, Error *local_err,
> + const char *fmt, ...);
> +
> /*
> * Prepend some text to @errp's human-readable error message.
> * The text is made by formatting @fmt, @ap like vprintf().
> diff --git a/migration/migration.c b/migration/migration.c
> index d6ae879dc8..f0f299a773 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1546,9 +1546,9 @@ static GSList *migration_blockers;
> int migrate_add_blocker(Error *reason, Error **errp)
> {
> if (migrate_get_current()->only_migratable) {
> - error_propagate(errp, error_copy(reason));
> - error_prepend(errp, "disallowing migration blocker "
> - "(--only_migratable) for: ");
> + error_propagate_prepend(errp, error_copy(reason),
> + "disallowing migration blocker "
> + "(--only_migratable) for: ");
> return -EACCES;
> }
>
> @@ -1557,9 +1557,9 @@ int migrate_add_blocker(Error *reason, Error **errp)
> return 0;
> }
>
> - error_propagate(errp, error_copy(reason));
> - error_prepend(errp, "disallowing migration blocker (migration in "
> - "progress) for: ");
> + error_propagate_prepend(errp, error_copy(reason),
> + "disallowing migration blocker "
> + "(migration in progress) for: ");
> return -EBUSY;
> }
>
> diff --git a/util/error.c b/util/error.c
> index 3efdd69162..b5ccbd8eac 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -292,3 +292,16 @@ void error_propagate(Error **dst_errp, Error *local_err)
> error_free(local_err);
> }
> }
> +
> +void error_propagate_prepend(Error **dst_errp, Error *err,
> + const char *fmt, ...)
> +{
> + va_list ap;
> +
> + if (dst_errp && !*dst_errp) {
> + va_start(ap, fmt);
> + error_vprepend(&err, fmt, ap);
> + va_end(ap);
> + } /* else error is being ignored, don't bother with prepending */
> + error_propagate(dst_errp, err);
> +}
>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 50+ messages in thread
* [Qemu-devel] [PATCH v2 02/35] Use error_fatal to simplify obvious fatal errors (again)
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 01/35] error: Fix use of error_prepend() with &error_fatal, &error_abort Markus Armbruster
@ 2018-10-15 11:52 ` Markus Armbruster
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 03/35] block: Use warn_report() & friends to report warnings Markus Armbruster
` (32 subsequent siblings)
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:52 UTC (permalink / raw)
To: qemu-devel; +Cc: David Gibson, Alexander Graf, Eric Blake, Paolo Bonzini
Add a slight improvement of the Coccinelle semantic patch from commit
007b06578ab, and use it to clean up. It leaves dead Error * variables
behind, cleaned up manually.
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Alexander Graf <agraf@suse.de>
Cc: Eric Blake <eblake@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Acked-by: David Gibson <david@gibson.dropbear.id.au>
---
hw/intc/xics_kvm.c | 7 +------
qemu-nbd.c | 6 +-----
scripts/coccinelle/use-error_fatal.cocci | 20 ++++++++++++++++++++
vl.c | 7 +------
4 files changed, 23 insertions(+), 17 deletions(-)
create mode 100644 scripts/coccinelle/use-error_fatal.cocci
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index 30c3769a20..e8fa9a53ae 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -198,17 +198,12 @@ static void ics_get_kvm_state(ICSState *ics)
{
uint64_t state;
int i;
- Error *local_err = NULL;
for (i = 0; i < ics->nr_irqs; i++) {
ICSIRQState *irq = &ics->irqs[i];
kvm_device_access(kernel_xics_fd, KVM_DEV_XICS_GRP_SOURCES,
- i + ics->offset, &state, false, &local_err);
- if (local_err) {
- error_report_err(local_err);
- exit(1);
- }
+ i + ics->offset, &state, false, &error_fatal);
irq->server = state & KVM_XICS_DESTINATION_MASK;
irq->saved_priority = (state >> KVM_XICS_PRIORITY_SHIFT)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index e76fe3082a..7874bc973c 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -1002,11 +1002,7 @@ int main(int argc, char **argv)
}
exp = nbd_export_new(bs, dev_offset, fd_size, nbdflags, nbd_export_closed,
- writethrough, NULL, &local_err);
- if (!exp) {
- error_report_err(local_err);
- exit(EXIT_FAILURE);
- }
+ writethrough, NULL, &error_fatal);
nbd_export_set_name(exp, export_name);
nbd_export_set_description(exp, export_description);
diff --git a/scripts/coccinelle/use-error_fatal.cocci b/scripts/coccinelle/use-error_fatal.cocci
new file mode 100644
index 0000000000..10fff0aec4
--- /dev/null
+++ b/scripts/coccinelle/use-error_fatal.cocci
@@ -0,0 +1,20 @@
+@@
+type T;
+identifier FUN, RET;
+expression list ARGS;
+expression ERR, EC, FAIL;
+@@
+(
+- T RET = FUN(ARGS, &ERR);
++ T RET = FUN(ARGS, &error_fatal);
+|
+- RET = FUN(ARGS, &ERR);
++ RET = FUN(ARGS, &error_fatal);
+|
+- FUN(ARGS, &ERR);
++ FUN(ARGS, &error_fatal);
+)
+- if (FAIL) {
+- error_report_err(ERR);
+- exit(EC);
+- }
diff --git a/vl.c b/vl.c
index 4e25c78bff..ff50199e64 100644
--- a/vl.c
+++ b/vl.c
@@ -2002,15 +2002,10 @@ static void select_vgahw(const char *p)
static void parse_display_qapi(const char *optarg)
{
- Error *err = NULL;
DisplayOptions *opts;
Visitor *v;
- v = qobject_input_visitor_new_str(optarg, "type", &err);
- if (!v) {
- error_report_err(err);
- exit(1);
- }
+ v = qobject_input_visitor_new_str(optarg, "type", &error_fatal);
visit_type_DisplayOptions(v, NULL, &opts, &error_fatal);
QAPI_CLONE_MEMBERS(DisplayOptions, &dpy, opts);
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* [Qemu-devel] [PATCH v2 03/35] block: Use warn_report() & friends to report warnings
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 01/35] error: Fix use of error_prepend() with &error_fatal, &error_abort Markus Armbruster
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 02/35] Use error_fatal to simplify obvious fatal errors (again) Markus Armbruster
@ 2018-10-15 11:52 ` Markus Armbruster
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 04/35] cpus hw target: " Markus Armbruster
` (31 subsequent siblings)
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:52 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Ronnie Sahlberg, Paolo Bonzini, Peter Lieven,
Hitoshi Mitake, Liu Yuan
Calling error_report() in a function that takes an Error ** argument
is suspicious. Convert a few that are actually warnings to
warn_report().
While there, split warnings consisting of multiple sentences to
conform to conventions spelled out in warn_report()'s contract, and
improve a rather useless warning in sheepdog.c.
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Peter Lieven <pl@kamp.de>
Cc: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp>
Cc: Liu Yuan <namei.unix@gmail.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/bochs.c | 8 ++++----
block/cloop.c | 8 ++++----
block/dmg.c | 8 ++++----
block/iscsi.c | 2 +-
block/rbd.c | 12 ++++++------
block/sheepdog.c | 2 +-
block/vvfat.c | 8 ++++----
7 files changed, 24 insertions(+), 24 deletions(-)
diff --git a/block/bochs.c b/block/bochs.c
index 50c630047b..36c1b45bd2 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -112,10 +112,10 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
}
if (!bdrv_is_read_only(bs)) {
- error_report("Opening bochs images without an explicit read-only=on "
- "option is deprecated. Future versions will refuse to "
- "open the image instead of automatically marking the "
- "image read-only.");
+ warn_report("Opening bochs images without an explicit read-only=on "
+ "option is deprecated");
+ error_printf("Future versions may refuse to open the image "
+ "instead of automatically marking it read-only.\n");
ret = bdrv_set_read_only(bs, true, errp); /* no write support yet */
if (ret < 0) {
return ret;
diff --git a/block/cloop.c b/block/cloop.c
index 2be68987bd..a558e67cb0 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -74,10 +74,10 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
}
if (!bdrv_is_read_only(bs)) {
- error_report("Opening cloop images without an explicit read-only=on "
- "option is deprecated. Future versions will refuse to "
- "open the image instead of automatically marking the "
- "image read-only.");
+ warn_report("Opening cloop images without an explicit read-only=on "
+ "option is deprecated");
+ error_printf("Future versions may refuse to open the image "
+ "instead of automatically marking it read-only.\n");
ret = bdrv_set_read_only(bs, true, errp);
if (ret < 0) {
return ret;
diff --git a/block/dmg.c b/block/dmg.c
index c9b3c519c4..9fb814460d 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -420,10 +420,10 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
}
if (!bdrv_is_read_only(bs)) {
- error_report("Opening dmg images without an explicit read-only=on "
- "option is deprecated. Future versions will refuse to "
- "open the image instead of automatically marking the "
- "image read-only.");
+ warn_report("Opening dmg images without an explicit read-only=on "
+ "option is deprecated");
+ error_printf("Future versions may refuse to open the image "
+ "instead of automatically marking it read-only.\n");
ret = bdrv_set_read_only(bs, true, errp);
if (ret < 0) {
return ret;
diff --git a/block/iscsi.c b/block/iscsi.c
index bb69faf34a..73998c2860 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1844,7 +1844,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
iscsi_set_timeout(iscsi, timeout);
#else
if (timeout) {
- error_report("iSCSI: ignoring timeout value for libiscsi <1.15.0");
+ warn_report("iSCSI: ignoring timeout value for libiscsi <1.15.0");
}
#endif
diff --git a/block/rbd.c b/block/rbd.c
index 014c68d629..6e26bac170 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -750,8 +750,8 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
/* Take care whenever deciding to actually deprecate; once this ability
* is removed, we will not be able to open any images with legacy-styled
* backing image strings. */
- error_report("RBD options encoded in the filename as keyvalue pairs "
- "is deprecated");
+ warn_report("RBD options encoded in the filename as keyvalue pairs "
+ "is deprecated");
}
/* Remove the processed options from the QDict (the visitor processes
@@ -781,10 +781,10 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
* leave as-is */
if (s->snap != NULL) {
if (!bdrv_is_read_only(bs)) {
- error_report("Opening rbd snapshots without an explicit "
- "read-only=on option is deprecated. Future versions "
- "will refuse to open the image instead of "
- "automatically marking the image read-only.");
+ warn_report("Opening rbd snapshots without an explicit "
+ "read-only=on option is deprecated");
+ error_printf("Future versions may refuse to open the image "
+ "instead of automatically marking it read-only.\n");
r = bdrv_set_read_only(bs, true, &local_err);
if (r < 0) {
error_propagate(errp, local_err);
diff --git a/block/sheepdog.c b/block/sheepdog.c
index b229a664d9..0125df9d49 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -572,7 +572,7 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
if (s->addr->type == SOCKET_ADDRESS_TYPE_INET && fd >= 0) {
int ret = socket_set_nodelay(fd);
if (ret < 0) {
- error_report("%s", strerror(errno));
+ warn_report("can't set TCP_NODELAY: %s", strerror(errno));
}
}
diff --git a/block/vvfat.c b/block/vvfat.c
index fc41841a5c..8f3f9e9a93 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1263,10 +1263,10 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
goto fail;
}
} else if (!bdrv_is_read_only(bs)) {
- error_report("Opening non-rw vvfat images without an explicit "
- "read-only=on option is deprecated. Future versions "
- "will refuse to open the image instead of "
- "automatically marking the image read-only.");
+ warn_report("Opening non-rw vvfat images without an explicit "
+ "read-only=on option is deprecated");
+ error_printf("Future versions may refuse to open the image "
+ "instead of automatically marking it read-only.\n");
/* read only is the default for safety */
ret = bdrv_set_read_only(bs, true, &local_err);
if (ret < 0) {
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* [Qemu-devel] [PATCH v2 04/35] cpus hw target: Use warn_report() & friends to report warnings
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (2 preceding siblings ...)
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 03/35] block: Use warn_report() & friends to report warnings Markus Armbruster
@ 2018-10-15 11:52 ` Markus Armbruster
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 05/35] vfio: " Markus Armbruster
` (30 subsequent siblings)
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:52 UTC (permalink / raw)
To: qemu-devel
Cc: Alex Bennée, Mark Cave-Ayland, Alex Williamson, Fam Zheng,
Wei Huang, David Gibson
Calling error_report() in a function that takes an Error ** argument
is suspicious. Convert a few that are actually warnings to
warn_report().
While there, split a warning consisting of multiple sentences to
conform to conventions spelled out in warn_report()'s contract.
Cc: Alex Bennée <alex.bennee@linaro.org>
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Fam Zheng <famz@redhat.com>
Cc: Wei Huang <wei@redhat.com>
Cc: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Acked-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
cpus.c | 8 ++++----
hw/display/cg3.c | 2 +-
hw/display/tcx.c | 2 +-
hw/misc/ivshmem.c | 4 ++--
hw/net/virtio-net.c | 8 ++++----
hw/virtio/virtio-pci.c | 4 ++--
target/i386/cpu.c | 17 +++++++++--------
target/ppc/translate_init.inc.c | 4 ++--
8 files changed, 25 insertions(+), 24 deletions(-)
diff --git a/cpus.c b/cpus.c
index 361678e459..7804071872 100644
--- a/cpus.c
+++ b/cpus.c
@@ -211,12 +211,12 @@ void qemu_tcg_configure(QemuOpts *opts, Error **errp)
error_setg(errp, "No MTTCG when icount is enabled");
} else {
#ifndef TARGET_SUPPORTS_MTTCG
- error_report("Guest not yet converted to MTTCG - "
- "you may get unexpected results");
+ warn_report("Guest not yet converted to MTTCG - "
+ "you may get unexpected results");
#endif
if (!check_tcg_memory_orders_compatible()) {
- error_report("Guest expects a stronger memory ordering "
- "than the host provides");
+ warn_report("Guest expects a stronger memory ordering "
+ "than the host provides");
error_printf("This may cause strange/hard to debug errors\n");
}
mttcg_enabled = true;
diff --git a/hw/display/cg3.c b/hw/display/cg3.c
index 1c199ab369..e50d97e48c 100644
--- a/hw/display/cg3.c
+++ b/hw/display/cg3.c
@@ -307,7 +307,7 @@ static void cg3_realizefn(DeviceState *dev, Error **errp)
ret = load_image_mr(fcode_filename, &s->rom);
g_free(fcode_filename);
if (ret < 0 || ret > FCODE_MAX_ROM_SIZE) {
- error_report("cg3: could not load prom '%s'", CG3_ROM_FILE);
+ warn_report("cg3: could not load prom '%s'", CG3_ROM_FILE);
}
}
diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index b2786ee8d0..66f2459226 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -823,7 +823,7 @@ static void tcx_realizefn(DeviceState *dev, Error **errp)
ret = load_image_mr(fcode_filename, &s->rom);
g_free(fcode_filename);
if (ret < 0 || ret > FCODE_MAX_ROM_SIZE) {
- error_report("tcx: could not load prom '%s'", TCX_ROM_FILE);
+ warn_report("tcx: could not load prom '%s'", TCX_ROM_FILE);
}
}
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 8cb17b9dd4..f88910e55c 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -1288,8 +1288,8 @@ static void ivshmem_realize(PCIDevice *dev, Error **errp)
IVShmemState *s = IVSHMEM_COMMON(dev);
if (!qtest_enabled()) {
- error_report("ivshmem is deprecated, please use ivshmem-plain"
- " or ivshmem-doorbell instead");
+ warn_report("ivshmem is deprecated, please use ivshmem-plain"
+ " or ivshmem-doorbell instead");
}
if (qemu_chr_fe_backend_connected(&s->server_chr) + !!s->shmobj != 1) {
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 4bdd5b8532..385b1a03e9 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -2020,10 +2020,10 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
if (n->net_conf.tx && strcmp(n->net_conf.tx, "timer")
&& strcmp(n->net_conf.tx, "bh")) {
- error_report("virtio-net: "
- "Unknown option tx=%s, valid options: \"timer\" \"bh\"",
- n->net_conf.tx);
- error_report("Defaulting to \"bh\"");
+ warn_report("virtio-net: "
+ "Unknown option tx=%s, valid options: \"timer\" \"bh\"",
+ n->net_conf.tx);
+ error_printf("Defaulting to \"bh\"");
}
n->net_conf.tx_queue_size = MIN(virtio_net_max_tx_queue_size(n),
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 3a01fe90f0..a954799267 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1683,8 +1683,8 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
if (err) {
/* Notice when a system that supports MSIx can't initialize it */
if (err != -ENOTSUP) {
- error_report("unable to init msix vectors to %" PRIu32,
- proxy->nvectors);
+ warn_report("unable to init msix vectors to %" PRIu32,
+ proxy->nvectors);
}
proxy->nvectors = 0;
}
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index c88876dfe3..9d4217afba 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5123,14 +5123,15 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
* NOTE: the following code has to follow qemu_init_vcpu(). Otherwise
* cs->nr_threads hasn't be populated yet and the checking is incorrect.
*/
- if (IS_AMD_CPU(env) &&
- !(env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
- cs->nr_threads > 1 && !ht_warned) {
- error_report("This family of AMD CPU doesn't support "
- "hyperthreading(%d). Please configure -smp "
- "options properly or try enabling topoext feature.",
- cs->nr_threads);
- ht_warned = true;
+ if (IS_AMD_CPU(env) &&
+ !(env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
+ cs->nr_threads > 1 && !ht_warned) {
+ warn_report("This family of AMD CPU doesn't support "
+ "hyperthreading(%d)",
+ cs->nr_threads);
+ error_printf("Please configure -smp options properly"
+ " or try enabling topoext feature.\n");
+ ht_warned = true;
}
x86_cpu_apic_realize(cpu, &local_err);
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index 263e63cb03..ee9432eb15 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -8381,8 +8381,8 @@ static void getset_compat_deprecated(Object *obj, Visitor *v, const char *name,
QNull *null = NULL;
if (!qtest_enabled()) {
- error_report("CPU 'compat' property is deprecated and has no effect; "
- "use max-cpu-compat machine property instead");
+ warn_report("CPU 'compat' property is deprecated and has no effect; "
+ "use max-cpu-compat machine property instead");
}
visit_type_null(v, name, &null, NULL);
qobject_unref(null);
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* [Qemu-devel] [PATCH v2 05/35] vfio: Use warn_report() & friends to report warnings
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (3 preceding siblings ...)
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 04/35] cpus hw target: " Markus Armbruster
@ 2018-10-15 11:52 ` Markus Armbruster
2018-10-15 18:53 ` Eric Blake
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 06/35] vfio: Clean up error reporting after previous commit Markus Armbruster
` (29 subsequent siblings)
34 siblings, 1 reply; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:52 UTC (permalink / raw)
To: qemu-devel; +Cc: Alex Williamson
The vfio code reports warnings like
error_report(WARN_PREFIX "Could not frobnicate", DEV-NAME);
where WARN_PREFIX is defined so the message comes out as
vfio warning: DEV-NAME: Could not frobnicate
This usage predates the introduction of warn_report() & friends in
commit 97f40301f1d. It's time to convert to that interface. Since
these functions already prefix the message with "warning: ", replace
WARN_PREFIX by VFIO_MSG_PREFIX, so the messages come out like
warning: vfio DEV-NAME: Could not frobnicate
The next commit will replace ERR_PREFIX.
Cc: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Acked-by: Alex Williamson <alex.williamson@redhat.com>
---
hw/vfio/pci.c | 14 +++++++-------
hw/vfio/platform.c | 4 ++--
include/hw/vfio/vfio-common.h | 2 +-
3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 8c95fc648a..79ea45e1f3 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -249,7 +249,7 @@ static void vfio_intx_update(PCIDevice *pdev)
vfio_intx_enable_kvm(vdev, &err);
if (err) {
- error_reportf_err(err, WARN_PREFIX, vdev->vbasedev.name);
+ warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
}
/* Re-enable the interrupt in cased we missed an EOI */
@@ -314,7 +314,7 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
vfio_intx_enable_kvm(vdev, &err);
if (err) {
- error_reportf_err(err, WARN_PREFIX, vdev->vbasedev.name);
+ warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
}
vdev->interrupt = VFIO_INT_INTx;
@@ -1554,7 +1554,7 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
&err);
if (ret < 0) {
if (ret == -ENOTSUP) {
- error_report_err(err);
+ warn_report_err(err);
return 0;
}
@@ -2587,9 +2587,9 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
} else if (irq_info.count == 1) {
vdev->pci_aer = true;
} else {
- error_report(WARN_PREFIX
- "Could not enable error recovery for the device",
- vbasedev->name);
+ warn_report(VFIO_MSG_PREFIX
+ "Could not enable error recovery for the device",
+ vbasedev->name);
}
}
@@ -2714,7 +2714,7 @@ static void vfio_req_notifier_handler(void *opaque)
qdev_unplug(&vdev->pdev.qdev, &err);
if (err) {
- error_reportf_err(err, WARN_PREFIX, vdev->vbasedev.name);
+ warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
}
}
diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index 57c4a0ee2b..c1aecac43c 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -657,8 +657,8 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp)
for (i = 0; i < vbasedev->num_regions; i++) {
if (vfio_region_mmap(vdev->regions[i])) {
- error_report("%s mmap unsupported. Performance may be slow",
- memory_region_name(vdev->regions[i]->mem));
+ warn_report("%s mmap unsupported, performance may be slow",
+ memory_region_name(vdev->regions[i]->mem));
}
sysbus_init_mmio(sbdev, vdev->regions[i]->mem);
}
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 6be9a93f61..c788e68bf8 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -31,7 +31,7 @@
#endif
#define ERR_PREFIX "vfio error: %s: "
-#define WARN_PREFIX "vfio warning: %s: "
+#define VFIO_MSG_PREFIX "vfio %s: "
enum {
VFIO_DEVICE_TYPE_PCI = 0,
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [Qemu-devel] [PATCH v2 05/35] vfio: Use warn_report() & friends to report warnings
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 05/35] vfio: " Markus Armbruster
@ 2018-10-15 18:53 ` Eric Blake
0 siblings, 0 replies; 50+ messages in thread
From: Eric Blake @ 2018-10-15 18:53 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: Alex Williamson
On 10/15/18 6:52 AM, Markus Armbruster wrote:
> The vfio code reports warnings like
>
> error_report(WARN_PREFIX "Could not frobnicate", DEV-NAME);
>
> where WARN_PREFIX is defined so the message comes out as
>
> vfio warning: DEV-NAME: Could not frobnicate
>
> This usage predates the introduction of warn_report() & friends in
> commit 97f40301f1d. It's time to convert to that interface. Since
> these functions already prefix the message with "warning: ", replace
> WARN_PREFIX by VFIO_MSG_PREFIX, so the messages come out like
>
> warning: vfio DEV-NAME: Could not frobnicate
>
> The next commit will replace ERR_PREFIX.
>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Acked-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> hw/vfio/pci.c | 14 +++++++-------
> hw/vfio/platform.c | 4 ++--
> include/hw/vfio/vfio-common.h | 2 +-
> 3 files changed, 10 insertions(+), 10 deletions(-)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 50+ messages in thread
* [Qemu-devel] [PATCH v2 06/35] vfio: Clean up error reporting after previous commit
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (4 preceding siblings ...)
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 05/35] vfio: " Markus Armbruster
@ 2018-10-15 11:52 ` Markus Armbruster
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 07/35] char: Use error_printf() to print help and such Markus Armbruster
` (28 subsequent siblings)
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:52 UTC (permalink / raw)
To: qemu-devel; +Cc: Alex Williamson
The previous commit changed vfio's warning messages from
vfio warning: DEV-NAME: Could not frobnicate
to
warning: vfio DEV-NAME: Could not frobnicate
To match this change, change error messages from
vfio error: DEV-NAME: On fire
to
vfio DEV-NAME: On fire
Note the loss of "error". If we think marking error messages that way
is a good idea, we should mark *all* error messages, i.e. make
error_report() print it.
Cc: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Acked-by: Alex Williamson <alex.williamson@redhat.com>
---
hw/vfio/pci-quirks.c | 4 ++--
hw/vfio/pci.c | 8 ++++----
hw/vfio/platform.c | 2 +-
include/hw/vfio/vfio-common.h | 1 -
4 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
index 481fd08df7..eae31c74d6 100644
--- a/hw/vfio/pci-quirks.c
+++ b/hw/vfio/pci-quirks.c
@@ -1670,7 +1670,7 @@ static void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
* but also no point in us enabling VGA if disabled in hardware.
*/
if (!(gmch & 0x2) && !vdev->vga && vfio_populate_vga(vdev, &err)) {
- error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
+ error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
error_report("IGD device %s failed to enable VGA access, "
"legacy mode disabled", vdev->vbasedev.name);
goto out;
@@ -1696,7 +1696,7 @@ static void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
ret = vfio_pci_igd_opregion_init(vdev, opregion, &err);
if (ret) {
error_append_hint(&err, "IGD legacy mode disabled\n");
- error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
+ error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
goto out;
}
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 79ea45e1f3..0338e08819 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -742,7 +742,7 @@ static void vfio_msi_disable_common(VFIOPCIDevice *vdev)
vfio_intx_enable(vdev, &err);
if (err) {
- error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
+ error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
}
}
@@ -2193,7 +2193,7 @@ static void vfio_pci_post_reset(VFIOPCIDevice *vdev)
vfio_intx_enable(vdev, &err);
if (err) {
- error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
+ error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
}
for (nr = 0; nr < PCI_NUM_REGIONS - 1; ++nr) {
@@ -2827,7 +2827,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
if (stat(vdev->vbasedev.sysfsdev, &st) < 0) {
error_setg_errno(errp, errno, "no such host device");
- error_prepend(errp, ERR_PREFIX, vdev->vbasedev.sysfsdev);
+ error_prepend(errp, VFIO_MSG_PREFIX, vdev->vbasedev.sysfsdev);
return;
}
@@ -3078,7 +3078,7 @@ out_teardown:
vfio_teardown_msi(vdev);
vfio_bars_exit(vdev);
error:
- error_prepend(errp, ERR_PREFIX, vdev->vbasedev.name);
+ error_prepend(errp, VFIO_MSG_PREFIX, vdev->vbasedev.name);
}
static void vfio_instance_finalize(Object *obj)
diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index c1aecac43c..61852f711b 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -668,7 +668,7 @@ out:
}
if (vdev->vbasedev.name) {
- error_prepend(errp, ERR_PREFIX, vdev->vbasedev.name);
+ error_prepend(errp, VFIO_MSG_PREFIX, vdev->vbasedev.name);
} else {
error_prepend(errp, "vfio error: ");
}
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index c788e68bf8..28483fce7c 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -30,7 +30,6 @@
#include <linux/vfio.h>
#endif
-#define ERR_PREFIX "vfio error: %s: "
#define VFIO_MSG_PREFIX "vfio %s: "
enum {
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* [Qemu-devel] [PATCH v2 07/35] char: Use error_printf() to print help and such
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (5 preceding siblings ...)
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 06/35] vfio: Clean up error reporting after previous commit Markus Armbruster
@ 2018-10-15 11:52 ` Markus Armbruster
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 08/35] 9pfs: Fix CLI parsing crash on error Markus Armbruster
` (27 subsequent siblings)
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:52 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Marc-André Lureau
Calling error_report() in a function that takes an Error ** argument
is suspicious. Convert a few that are actually help and such to
error_printf().
Improves output of -chardev help from
qemu-system-x86_64: -chardev help: Available chardev backend types:
serial
...
to
Available chardev backend types:
serial
...
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
chardev/char-pty.c | 2 +-
chardev/char.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index e8d9a53476..f681d637c1 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -259,7 +259,7 @@ static void char_pty_open(Chardev *chr,
qemu_set_nonblock(master_fd);
chr->filename = g_strdup_printf("pty:%s", pty_name);
- error_report("char device redirected to %s (label %s)",
+ error_printf("char device redirected to %s (label %s)\n",
pty_name, chr->label);
s = PTY_CHARDEV(chr);
diff --git a/chardev/char.c b/chardev/char.c
index e115166995..7f07a1bfbd 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -634,7 +634,7 @@ Chardev *qemu_chr_new_from_opts(QemuOpts *opts, Error **errp)
chardev_name_foreach(help_string_append, str);
- error_report("Available chardev backend types: %s", str->str);
+ error_printf("Available chardev backend types: %s\n", str->str);
g_string_free(str, true);
return NULL;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* [Qemu-devel] [PATCH v2 08/35] 9pfs: Fix CLI parsing crash on error
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (6 preceding siblings ...)
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 07/35] char: Use error_printf() to print help and such Markus Armbruster
@ 2018-10-15 11:52 ` Markus Armbruster
2018-10-15 19:00 ` Eric Blake
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 09/35] pc: Fix machine property nvdimm-persistence error handling Markus Armbruster
` (26 subsequent siblings)
34 siblings, 1 reply; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:52 UTC (permalink / raw)
To: qemu-devel; +Cc: Greg Kurz
Calling error_report() in a function that takes an Error ** argument
is suspicious. 9p-handle.c's handle_parse_opts() does that, and then
fails without setting an error. Wrong. Its caller crashes when it
tries to report the error:
$ qemu-system-x86_64 -nodefaults -fsdev id=foo,fsdriver=handle
qemu-system-x86_64: -fsdev id=foo,fsdriver=handle: warning: handle backend is deprecated
qemu-system-x86_64: -fsdev id=foo,fsdriver=handle: fsdev: No path specified
Segmentation fault (core dumped)
Screwed up when commit 91cda4e8f37 (v2.12.0) converted the function to
Error. Fix by calling error_setg() instead of error_report().
Fixes: 91cda4e8f372602795e3a2f4bd2e3adaf9f82255
Cc: Greg Kurz <groug@kaod.org>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Acked-by: Greg Kurz <groug@kaod.org>
---
hw/9pfs/9p-handle.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/hw/9pfs/9p-handle.c b/hw/9pfs/9p-handle.c
index f3641dbe4a..3465b1ef30 100644
--- a/hw/9pfs/9p-handle.c
+++ b/hw/9pfs/9p-handle.c
@@ -19,6 +19,7 @@
#include <grp.h>
#include <sys/socket.h>
#include <sys/un.h>
+#include "qapi/error.h"
#include "qemu/xattr.h"
#include "qemu/cutils.h"
#include "qemu/error-report.h"
@@ -655,12 +656,13 @@ static int handle_parse_opts(QemuOpts *opts, FsDriverEntry *fse, Error **errp)
warn_report("handle backend is deprecated");
if (sec_model) {
- error_report("Invalid argument security_model specified with handle fsdriver");
+ error_setg(errp,
+ "Invalid argument security_model specified with handle fsdriver");
return -1;
}
if (!path) {
- error_report("fsdev: No path specified");
+ error_setg(errp, "fsdev: No path specified");
return -1;
}
fse->path = g_strdup(path);
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [Qemu-devel] [PATCH v2 08/35] 9pfs: Fix CLI parsing crash on error
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 08/35] 9pfs: Fix CLI parsing crash on error Markus Armbruster
@ 2018-10-15 19:00 ` Eric Blake
0 siblings, 0 replies; 50+ messages in thread
From: Eric Blake @ 2018-10-15 19:00 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: Greg Kurz
On 10/15/18 6:52 AM, Markus Armbruster wrote:
> Calling error_report() in a function that takes an Error ** argument
> is suspicious. 9p-handle.c's handle_parse_opts() does that, and then
> fails without setting an error. Wrong. Its caller crashes when it
> tries to report the error:
>
> $ qemu-system-x86_64 -nodefaults -fsdev id=foo,fsdriver=handle
> qemu-system-x86_64: -fsdev id=foo,fsdriver=handle: warning: handle backend is deprecated
> qemu-system-x86_64: -fsdev id=foo,fsdriver=handle: fsdev: No path specified
> Segmentation fault (core dumped)
>
> Screwed up when commit 91cda4e8f37 (v2.12.0) converted the function to
> Error. Fix by calling error_setg() instead of error_report().
>
> Fixes: 91cda4e8f372602795e3a2f4bd2e3adaf9f82255
> Cc: Greg Kurz <groug@kaod.org>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Acked-by: Greg Kurz <groug@kaod.org>
> ---
> hw/9pfs/9p-handle.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 50+ messages in thread
* [Qemu-devel] [PATCH v2 09/35] pc: Fix machine property nvdimm-persistence error handling
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (7 preceding siblings ...)
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 08/35] 9pfs: Fix CLI parsing crash on error Markus Armbruster
@ 2018-10-15 11:52 ` Markus Armbruster
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 10/35] ioapic: Fix error handling in realize() Markus Armbruster
` (25 subsequent siblings)
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:52 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael S. Tsirkin
Calling error_report() in a function that takes an Error ** argument
is suspicious. pc.c's pc_machine_set_nvdimm_persistence() does that,
and then exit()s. Wrong. Attempting to set machine property
nvdimm-persistence to a bad value instantly kills the VM:
$ qemu-system-x86_64 -nodefaults -S -display none -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 3}, "package": "v3.0.0-837-gc5e4e49258"}, "capabilities": []}}
{"execute": "qmp_capabilities"}
{"return": {}}
{"execute": "qom-set", "arguments": {"path": "/machine", "property": "nvdimm-persistence", "value": "instadeath"}}
-machine nvdimm-persistence=instadeath: unsupported option
$ echo $?
1
Broken when commit 11c39b5cd96 (v3.0.0) replaced error_propagate();
return by error_report(); exit() instead of error_setg(); return. Fix
that.
Fixes: 11c39b5cd966ddc067a1ca0c5392ec9b666c45b7
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
hw/i386/pc.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index cd5029c149..eab8572f2a 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2209,8 +2209,9 @@ static void pc_machine_set_nvdimm_persistence(Object *obj, const char *value,
else if (strcmp(value, "mem-ctrl") == 0)
nvdimm_state->persistence = 2;
else {
- error_report("-machine nvdimm-persistence=%s: unsupported option", value);
- exit(EXIT_FAILURE);
+ error_setg(errp, "-machine nvdimm-persistence=%s: unsupported option",
+ value);
+ return;
}
g_free(nvdimm_state->persistence_string);
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* [Qemu-devel] [PATCH v2 10/35] ioapic: Fix error handling in realize()
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (8 preceding siblings ...)
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 09/35] pc: Fix machine property nvdimm-persistence error handling Markus Armbruster
@ 2018-10-15 11:52 ` Markus Armbruster
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 11/35] smbios: Clean up error handling in smbios_add() Markus Armbruster
` (24 subsequent siblings)
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:52 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Xu
Calling error_report() in a function that takes an Error ** argument
is suspicious. ioapic_realize() does that, and then exit()s.
Currently mostly harmless, as the device cannot be hot-plugged.
Fixes: 20fd4b7b6d9282fe0cb83601f1821f31bd257458
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
hw/intc/ioapic.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
index b6896ac4ce..4e529729b4 100644
--- a/hw/intc/ioapic.c
+++ b/hw/intc/ioapic.c
@@ -21,7 +21,7 @@
*/
#include "qemu/osdep.h"
-#include "qemu/error-report.h"
+#include "qapi/error.h"
#include "monitor/monitor.h"
#include "hw/hw.h"
#include "hw/i386/pc.h"
@@ -393,9 +393,9 @@ static void ioapic_realize(DeviceState *dev, Error **errp)
IOAPICCommonState *s = IOAPIC_COMMON(dev);
if (s->version != 0x11 && s->version != 0x20) {
- error_report("IOAPIC only supports version 0x11 or 0x20 "
- "(default: 0x%x).", IOAPIC_VER_DEF);
- exit(1);
+ error_setg(errp, "IOAPIC only supports version 0x11 or 0x20 "
+ "(default: 0x%x).", IOAPIC_VER_DEF);
+ return;
}
memory_region_init_io(&s->io_memory, OBJECT(s), &ioapic_io_ops, s,
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* [Qemu-devel] [PATCH v2 11/35] smbios: Clean up error handling in smbios_add()
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (9 preceding siblings ...)
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 10/35] ioapic: Fix error handling in realize() Markus Armbruster
@ 2018-10-15 11:52 ` Markus Armbruster
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 12/35] migration: Fix !replay_can_snapshot() error handling Markus Armbruster
` (23 subsequent siblings)
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:52 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini
Calling error_report() in a function that takes an Error ** argument
is suspicious. smbios_entry_add() does that, and then exit()s. It
also passes &error_fatal to qemu_opts_validate(). Both wrong, but
currently harmless, as its only caller passes &error_fatal. Messed up
in commit 1007a37e208. Clean it up.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/smbios/smbios.c | 90 +++++++++++++++++++++++++++++++---------------
1 file changed, 62 insertions(+), 28 deletions(-)
diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index a27e54b2fa..920939454e 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -950,6 +950,7 @@ static void save_opt_list(size_t *ndest, const char ***dest,
void smbios_entry_add(QemuOpts *opts, Error **errp)
{
+ Error *err = NULL;
const char *val;
assert(!smbios_immutable);
@@ -960,12 +961,16 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
int size;
struct smbios_table *table; /* legacy mode only */
- qemu_opts_validate(opts, qemu_smbios_file_opts, &error_fatal);
+ qemu_opts_validate(opts, qemu_smbios_file_opts, &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
size = get_image_size(val);
if (size == -1 || size < sizeof(struct smbios_structure_header)) {
- error_report("Cannot read SMBIOS file %s", val);
- exit(1);
+ error_setg(errp, "Cannot read SMBIOS file %s", val);
+ return;
}
/*
@@ -978,14 +983,15 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
smbios_tables_len);
if (load_image(val, (uint8_t *)header) != size) {
- error_report("Failed to load SMBIOS file %s", val);
- exit(1);
+ error_setg(errp, "Failed to load SMBIOS file %s", val);
+ return;
}
if (test_bit(header->type, have_fields_bitmap)) {
- error_report("can't load type %d struct, fields already specified!",
- header->type);
- exit(1);
+ error_setg(errp,
+ "can't load type %d struct, fields already specified!",
+ header->type);
+ return;
}
set_bit(header->type, have_binfile_bitmap);
@@ -1030,19 +1036,23 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
unsigned long type = strtoul(val, NULL, 0);
if (type > SMBIOS_MAX_TYPE) {
- error_report("out of range!");
- exit(1);
+ error_setg(errp, "out of range!");
+ return;
}
if (test_bit(type, have_binfile_bitmap)) {
- error_report("can't add fields, binary file already loaded!");
- exit(1);
+ error_setg(errp, "can't add fields, binary file already loaded!");
+ return;
}
set_bit(type, have_fields_bitmap);
switch (type) {
case 0:
- qemu_opts_validate(opts, qemu_smbios_type0_opts, &error_fatal);
+ qemu_opts_validate(opts, qemu_smbios_type0_opts, &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
save_opt(&type0.vendor, opts, "vendor");
save_opt(&type0.version, opts, "version");
save_opt(&type0.date, opts, "date");
@@ -1051,14 +1061,18 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
val = qemu_opt_get(opts, "release");
if (val) {
if (sscanf(val, "%hhu.%hhu", &type0.major, &type0.minor) != 2) {
- error_report("Invalid release");
- exit(1);
+ error_setg(errp, "Invalid release");
+ return;
}
type0.have_major_minor = true;
}
return;
case 1:
- qemu_opts_validate(opts, qemu_smbios_type1_opts, &error_fatal);
+ qemu_opts_validate(opts, qemu_smbios_type1_opts, &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
save_opt(&type1.manufacturer, opts, "manufacturer");
save_opt(&type1.product, opts, "product");
save_opt(&type1.version, opts, "version");
@@ -1069,14 +1083,18 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
val = qemu_opt_get(opts, "uuid");
if (val) {
if (qemu_uuid_parse(val, &qemu_uuid) != 0) {
- error_report("Invalid UUID");
- exit(1);
+ error_setg(errp, "Invalid UUID");
+ return;
}
qemu_uuid_set = true;
}
return;
case 2:
- qemu_opts_validate(opts, qemu_smbios_type2_opts, &error_fatal);
+ qemu_opts_validate(opts, qemu_smbios_type2_opts, &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
save_opt(&type2.manufacturer, opts, "manufacturer");
save_opt(&type2.product, opts, "product");
save_opt(&type2.version, opts, "version");
@@ -1085,7 +1103,11 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
save_opt(&type2.location, opts, "location");
return;
case 3:
- qemu_opts_validate(opts, qemu_smbios_type3_opts, &error_fatal);
+ qemu_opts_validate(opts, qemu_smbios_type3_opts, &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
save_opt(&type3.manufacturer, opts, "manufacturer");
save_opt(&type3.version, opts, "version");
save_opt(&type3.serial, opts, "serial");
@@ -1093,7 +1115,11 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
save_opt(&type3.sku, opts, "sku");
return;
case 4:
- qemu_opts_validate(opts, qemu_smbios_type4_opts, &error_fatal);
+ qemu_opts_validate(opts, qemu_smbios_type4_opts, &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
save_opt(&type4.sock_pfx, opts, "sock_pfx");
save_opt(&type4.manufacturer, opts, "manufacturer");
save_opt(&type4.version, opts, "version");
@@ -1102,11 +1128,19 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
save_opt(&type4.part, opts, "part");
return;
case 11:
- qemu_opts_validate(opts, qemu_smbios_type11_opts, &error_fatal);
+ qemu_opts_validate(opts, qemu_smbios_type11_opts, &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
save_opt_list(&type11.nvalues, &type11.values, opts, "value");
return;
case 17:
- qemu_opts_validate(opts, qemu_smbios_type17_opts, &error_fatal);
+ qemu_opts_validate(opts, qemu_smbios_type17_opts, &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
save_opt(&type17.loc_pfx, opts, "loc_pfx");
save_opt(&type17.bank, opts, "bank");
save_opt(&type17.manufacturer, opts, "manufacturer");
@@ -1116,12 +1150,12 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
type17.speed = qemu_opt_get_number(opts, "speed", 0);
return;
default:
- error_report("Don't know how to build fields for SMBIOS type %ld",
- type);
- exit(1);
+ error_setg(errp,
+ "Don't know how to build fields for SMBIOS type %ld",
+ type);
+ return;
}
}
- error_report("Must specify type= or file=");
- exit(1);
+ error_setg(errp, "Must specify type= or file=");
}
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* [Qemu-devel] [PATCH v2 12/35] migration: Fix !replay_can_snapshot() error handling
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (10 preceding siblings ...)
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 11/35] smbios: Clean up error handling in smbios_add() Markus Armbruster
@ 2018-10-15 11:52 ` Markus Armbruster
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 13/35] l2tpv3: Improve -netdev/netdev_add/-net/... error reporting Markus Armbruster
` (22 subsequent siblings)
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:52 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini
Calling error_report() in a function that takes an Error ** argument
is suspicious. save_snapshot() and load_snapshot() do that, and then
fail without setting an error. Wrong. The HMP commands survive this
unscathed, since hmp_handle_error() does nothing when no error has
been set. Callers main() (on behalf of -loadvm) and
replay_vmstate_init() crash, but I'm not sure the error is possible
there.
Screwed up when commit 377b21ccea1 (v2.12.0) added incorrect error
handling right next to correct examples. Fix by calling error_setg()
instead of error_report().
Fixes: 377b21ccea1755a8b0dae822c29567c58dda6939
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
migration/savevm.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/migration/savevm.c b/migration/savevm.c
index 2d10e45582..5f8eb38676 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2414,8 +2414,8 @@ int save_snapshot(const char *name, Error **errp)
AioContext *aio_context;
if (!replay_can_snapshot()) {
- error_report("Record/replay does not allow making snapshot "
- "right now. Try once more later.");
+ error_setg(errp, "Record/replay does not allow making snapshot "
+ "right now. Try once more later.");
return ret;
}
@@ -2611,8 +2611,8 @@ int load_snapshot(const char *name, Error **errp)
MigrationIncomingState *mis = migration_incoming_get_current();
if (!replay_can_snapshot()) {
- error_report("Record/replay does not allow loading snapshot "
- "right now. Try once more later.");
+ error_setg(errp, "Record/replay does not allow loading snapshot "
+ "right now. Try once more later.");
return -EINVAL;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* [Qemu-devel] [PATCH v2 13/35] l2tpv3: Improve -netdev/netdev_add/-net/... error reporting
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (11 preceding siblings ...)
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 12/35] migration: Fix !replay_can_snapshot() error handling Markus Armbruster
@ 2018-10-15 11:52 ` Markus Armbruster
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 14/35] net/socket: Fix invalid socket type error handling Markus Armbruster
` (21 subsequent siblings)
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:52 UTC (permalink / raw)
To: qemu-devel; +Cc: Jason Wang
When -netdev l2tpv3 fails, it first reports a specific error, then a
generic one, like this:
$ qemu-system-x86_64 -netdev l2tpv3,id=foo,src=,dst=,txsession=1
qemu-system-x86_64: -netdev l2tpv3,id=foo,src=,dst=,txsession=1: l2tpv3_open : could not resolve src, errno = Name or service not known
qemu-system-x86_64: Device 'l2tpv3' could not be initialized
With the command line, the messages go to stderr. In HMP, they go to
the monitor. In QMP, the second one becomes the error reply, and the
first one goes to stderr.
Convert net_init_tap() to Error. This suppresses the unwanted second
message, and makes the specific error the QMP error reply.
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
net/l2tpv3.c | 25 ++++++++++++-------------
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/net/l2tpv3.c b/net/l2tpv3.c
index 6745b78990..81db24dc8c 100644
--- a/net/l2tpv3.c
+++ b/net/l2tpv3.c
@@ -28,6 +28,7 @@
#include <netdb.h>
#include "net/net.h"
#include "clients.h"
+#include "qapi/error.h"
#include "qemu-common.h"
#include "qemu/error-report.h"
#include "qemu/option.h"
@@ -528,7 +529,6 @@ int net_init_l2tpv3(const Netdev *netdev,
const char *name,
NetClientState *peer, Error **errp)
{
- /* FIXME error_setg(errp, ...) on failure */
const NetdevL2TPv3Options *l2tpv3;
NetL2TPV3State *s;
NetClientState *nc;
@@ -555,7 +555,7 @@ int net_init_l2tpv3(const Netdev *netdev,
}
if ((l2tpv3->has_offset) && (l2tpv3->offset > 256)) {
- error_report("l2tpv3_open : offset must be less than 256 bytes");
+ error_setg(errp, "offset must be less than 256 bytes");
goto outerr;
}
@@ -563,6 +563,8 @@ int net_init_l2tpv3(const Netdev *netdev,
if (l2tpv3->has_rxcookie && l2tpv3->has_txcookie) {
s->cookie = true;
} else {
+ error_setg(errp,
+ "require both 'rxcookie' and 'txcookie' or neither");
goto outerr;
}
} else {
@@ -578,7 +580,7 @@ int net_init_l2tpv3(const Netdev *netdev,
if (l2tpv3->has_udp && l2tpv3->udp) {
s->udp = true;
if (!(l2tpv3->has_srcport && l2tpv3->has_dstport)) {
- error_report("l2tpv3_open : need both src and dst port for udp");
+ error_setg(errp, "need both src and dst port for udp");
goto outerr;
} else {
srcport = l2tpv3->srcport;
@@ -639,20 +641,19 @@ int net_init_l2tpv3(const Netdev *netdev,
gairet = getaddrinfo(l2tpv3->src, srcport, &hints, &result);
if ((gairet != 0) || (result == NULL)) {
- error_report(
- "l2tpv3_open : could not resolve src, errno = %s",
- gai_strerror(gairet)
- );
+ error_setg(errp, "could not resolve src, errno = %s",
+ gai_strerror(gairet));
goto outerr;
}
fd = socket(result->ai_family, result->ai_socktype, result->ai_protocol);
if (fd == -1) {
fd = -errno;
- error_report("l2tpv3_open : socket creation failed, errno = %d", -fd);
+ error_setg(errp, "socket creation failed, errno = %d",
+ -fd);
goto outerr;
}
if (bind(fd, (struct sockaddr *) result->ai_addr, result->ai_addrlen)) {
- error_report("l2tpv3_open : could not bind socket err=%i", errno);
+ error_setg(errp, "could not bind socket err=%i", errno);
goto outerr;
}
if (result) {
@@ -677,10 +678,8 @@ int net_init_l2tpv3(const Netdev *netdev,
result = NULL;
gairet = getaddrinfo(l2tpv3->dst, dstport, &hints, &result);
if ((gairet != 0) || (result == NULL)) {
- error_report(
- "l2tpv3_open : could not resolve dst, error = %s",
- gai_strerror(gairet)
- );
+ error_setg(errp, "could not resolve dst, error = %s",
+ gai_strerror(gairet));
goto outerr;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* [Qemu-devel] [PATCH v2 14/35] net/socket: Fix invalid socket type error handling
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (12 preceding siblings ...)
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 13/35] l2tpv3: Improve -netdev/netdev_add/-net/... error reporting Markus Armbruster
@ 2018-10-15 11:52 ` Markus Armbruster
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 15/35] numa: Fix QMP command set-numa-node " Markus Armbruster
` (20 subsequent siblings)
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:52 UTC (permalink / raw)
To: qemu-devel; +Cc: Jason Wang
Calling error_report() in a function that takes an Error ** argument
is suspicious. net_socket_fd_init() does that, and then fails without
setting an error. Wrong. I didn't analyze how exactly this can
break. A caller that reports the error on failure would crash.
Broken when commit c37f0bb1d0d (v2.11.0) converted the function to
Error. Fix by calling error_setg() instead of error_report().
Fixes: c37f0bb1d0d24e3a6b5f4659bb305913dcb798a6
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
net/socket.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/socket.c b/net/socket.c
index 6917fbcbf5..90ef3517be 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -453,8 +453,8 @@ static NetSocketState *net_socket_fd_init(NetClientState *peer,
case SOCK_STREAM:
return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
default:
- error_report("socket type=%d for fd=%d must be either"
- " SOCK_DGRAM or SOCK_STREAM", so_type, fd);
+ error_setg(errp, "socket type=%d for fd=%d must be either"
+ " SOCK_DGRAM or SOCK_STREAM", so_type, fd);
closesocket(fd);
}
return NULL;
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* [Qemu-devel] [PATCH v2 15/35] numa: Fix QMP command set-numa-node error handling
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (13 preceding siblings ...)
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 14/35] net/socket: Fix invalid socket type error handling Markus Armbruster
@ 2018-10-15 11:52 ` Markus Armbruster
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 16/35] xen/pt: Fix incomplete conversion to realize() Markus Armbruster
` (19 subsequent siblings)
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:52 UTC (permalink / raw)
To: qemu-devel; +Cc: Igor Mammedov
Calling error_report() in a function that takes an Error ** argument
is suspicious. parse_numa_node() does that, and then exit()s. It
also passes &error_fatal to machine_set_cpu_numa_node(). Both wrong.
Attempting to configure numa when the machine doesn't support it kills
the VM:
$ qemu-system-x86_64 -nodefaults -S -display none -M none -preconfig -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 3}, "package": "v3.0.0-837-gc5e4e49258"}, "capabilities": []}}
{"execute": "qmp_capabilities"}
{"return": {}}
{"execute": "set-numa-node", "arguments": {"type": "node"}}
NUMA is not supported by this machine-type
$ echo $?
1
Messed up when commit 64c2a8f6d3f and 7c88e65d9e9 (v2.10.0) added
incorrect error handling right next to correct examples. Latent bug
until commit f3be67812c2 (v3.0.0) made it accessible via QMP. Fairly
harmless in practice, because it's limited to RUN_STATE_PRECONFIG.
The fix is obvious: replace error_report(); exit() by error_setg();
return.
This affects parse_numa_node()'s other caller
numa_complete_configuration(): since it ignores errors, the "NUMA is
not supported by this machine-type" is now ignored, too. But that
error is as unexpected there as any other. Change it to abort on
error instead.
Fixes: f3be67812c226162f86ce92634bd913714445420
Cc: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
numa.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/numa.c b/numa.c
index 81542d4ebb..1d7c49ad43 100644
--- a/numa.c
+++ b/numa.c
@@ -60,6 +60,7 @@ NodeInfo numa_info[MAX_NODES];
static void parse_numa_node(MachineState *ms, NumaNodeOptions *node,
Error **errp)
{
+ Error *err = NULL;
uint16_t nodenr;
uint16List *cpus = NULL;
MachineClass *mc = MACHINE_GET_CLASS(ms);
@@ -82,8 +83,8 @@ static void parse_numa_node(MachineState *ms, NumaNodeOptions *node,
}
if (!mc->cpu_index_to_instance_props || !mc->get_default_cpu_node_id) {
- error_report("NUMA is not supported by this machine-type");
- exit(1);
+ error_setg(errp, "NUMA is not supported by this machine-type");
+ return;
}
for (cpus = node->cpus; cpus; cpus = cpus->next) {
CpuInstanceProperties props;
@@ -97,7 +98,11 @@ static void parse_numa_node(MachineState *ms, NumaNodeOptions *node,
props = mc->cpu_index_to_instance_props(ms, cpus->value);
props.node_id = nodenr;
props.has_node_id = true;
- machine_set_cpu_numa_node(ms, &props, &error_fatal);
+ machine_set_cpu_numa_node(ms, &props, &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
}
if (node->has_mem && node->has_memdev) {
@@ -367,7 +372,7 @@ void numa_complete_configuration(MachineState *ms)
if (ms->ram_slots > 0 && nb_numa_nodes == 0 &&
mc->auto_enable_numa_with_memhp) {
NumaNodeOptions node = { };
- parse_numa_node(ms, &node, NULL);
+ parse_numa_node(ms, &node, &error_abort);
}
assert(max_numa_nodeid <= MAX_NODES);
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* [Qemu-devel] [PATCH v2 16/35] xen/pt: Fix incomplete conversion to realize()
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (14 preceding siblings ...)
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 15/35] numa: Fix QMP command set-numa-node " Markus Armbruster
@ 2018-10-15 11:52 ` Markus Armbruster
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 17/35] seccomp: Clean up error reporting in parse_sandbox() Markus Armbruster
` (18 subsequent siblings)
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:52 UTC (permalink / raw)
To: qemu-devel; +Cc: Stefano Stabellini, Anthony Perard
The conversion of "xen-pci-passthrough" to realize() (commit
5a11d0f7549, v2.6.0) neglected to convert the xen_pt_config_init()
error path. If xen_pt_config_init() fails, xen_pt_realize() reports
the error, then returns success without completing its job. I don't
know the exact impact, but it can't be good.
Belatedly convert the error path.
Fixes: 5a11d0f7549e24a10e178a9dc8ff5e698031d9a6
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
---
hw/xen/xen_pt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index e5a6eff44f..f1f3a3727c 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -830,7 +830,7 @@ static void xen_pt_realize(PCIDevice *d, Error **errp)
xen_pt_config_init(s, &err);
if (err) {
error_append_hint(&err, "PCI Config space initialisation failed");
- error_report_err(err);
+ error_propagate(errp, err);
rc = -1;
goto err_out;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* [Qemu-devel] [PATCH v2 17/35] seccomp: Clean up error reporting in parse_sandbox()
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (15 preceding siblings ...)
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 16/35] xen/pt: Fix incomplete conversion to realize() Markus Armbruster
@ 2018-10-15 11:52 ` Markus Armbruster
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 18/35] vl: Clean up error reporting in parse_add_fd() Markus Armbruster
` (17 subsequent siblings)
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:52 UTC (permalink / raw)
To: qemu-devel; +Cc: Eduardo Otubo
Calling error_report() in a function that takes an Error ** argument
is suspicious. parse_sandbox() does that, and then fails without
setting an error. Its caller main(), via qemu_opts_foreach(), is fine
with it, but clean it up anyway.
Cc: Eduardo Otubo <otubo@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Acked-by: Eduardo Otubo <otubo@redhat.com>
---
qemu-seccomp.c | 18 +++++++++---------
vl.c | 4 ++--
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/qemu-seccomp.c b/qemu-seccomp.c
index 1baa5c69ed..5c73e6ad05 100644
--- a/qemu-seccomp.c
+++ b/qemu-seccomp.c
@@ -12,11 +12,12 @@
* Contributions after 2012-01-13 are licensed under the terms of the
* GNU GPL, version 2 or (at your option) any later version.
*/
+
#include "qemu/osdep.h"
+#include "qapi/error.h"
#include "qemu/config-file.h"
#include "qemu/option.h"
#include "qemu/module.h"
-#include "qemu/error-report.h"
#include <sys/prctl.h>
#include <seccomp.h>
#include "sysemu/seccomp.h"
@@ -190,7 +191,7 @@ int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp)
* to provide a little bit of consistency for
* the command line */
} else {
- error_report("invalid argument for obsolete");
+ error_setg(errp, "invalid argument for obsolete");
return -1;
}
}
@@ -205,14 +206,13 @@ int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp)
/* calling prctl directly because we're
* not sure if host has CAP_SYS_ADMIN set*/
if (prctl(PR_SET_NO_NEW_PRIVS, 1)) {
- error_report("failed to set no_new_privs "
- "aborting");
+ error_setg(errp, "failed to set no_new_privs aborting");
return -1;
}
} else if (g_str_equal(value, "allow")) {
/* default value */
} else {
- error_report("invalid argument for elevateprivileges");
+ error_setg(errp, "invalid argument for elevateprivileges");
return -1;
}
}
@@ -224,7 +224,7 @@ int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp)
} else if (g_str_equal(value, "allow")) {
/* default value */
} else {
- error_report("invalid argument for spawn");
+ error_setg(errp, "invalid argument for spawn");
return -1;
}
}
@@ -236,14 +236,14 @@ int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp)
} else if (g_str_equal(value, "allow")) {
/* default value */
} else {
- error_report("invalid argument for resourcecontrol");
+ error_setg(errp, "invalid argument for resourcecontrol");
return -1;
}
}
if (seccomp_start(seccomp_opts) < 0) {
- error_report("failed to install seccomp syscall filter "
- "in the kernel");
+ error_setg(errp, "failed to install seccomp syscall filter "
+ "in the kernel");
return -1;
}
}
diff --git a/vl.c b/vl.c
index ff50199e64..643121e5d4 100644
--- a/vl.c
+++ b/vl.c
@@ -3972,8 +3972,8 @@ int main(int argc, char **argv, char **envp)
#ifdef CONFIG_SECCOMP
olist = qemu_find_opts_err("sandbox", NULL);
- if (olist && qemu_opts_foreach(olist, parse_sandbox, NULL, NULL)) {
- exit(1);
+ if (olist) {
+ qemu_opts_foreach(olist, parse_sandbox, NULL, &error_fatal);
}
#endif
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* [Qemu-devel] [PATCH v2 18/35] vl: Clean up error reporting in parse_add_fd()
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (16 preceding siblings ...)
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 17/35] seccomp: Clean up error reporting in parse_sandbox() Markus Armbruster
@ 2018-10-15 11:52 ` Markus Armbruster
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 19/35] qom: Clean up error reporting in user_creatable_add_opts_foreach() Markus Armbruster
` (16 subsequent siblings)
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:52 UTC (permalink / raw)
To: qemu-devel
Calling error_report() in a function that takes an Error ** argument
is suspicious. parse_add_fd() does that, and then fails without
setting an error. Its caller main(), via qemu_opts_foreach(), is fine
with it, but clean it up anyway.
Also change call of cleanup_add_fd(), which can't fail, for symmetry.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
vl.c | 22 +++++++++-------------
1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/vl.c b/vl.c
index 643121e5d4..2ef066ad61 100644
--- a/vl.c
+++ b/vl.c
@@ -1059,12 +1059,12 @@ static int parse_add_fd(void *opaque, QemuOpts *opts, Error **errp)
fd_opaque = qemu_opt_get(opts, "opaque");
if (fd < 0) {
- error_report("fd option is required and must be non-negative");
+ error_setg(errp, "fd option is required and must be non-negative");
return -1;
}
if (fd <= STDERR_FILENO) {
- error_report("fd cannot be a standard I/O stream");
+ error_setg(errp, "fd cannot be a standard I/O stream");
return -1;
}
@@ -1074,12 +1074,12 @@ static int parse_add_fd(void *opaque, QemuOpts *opts, Error **errp)
*/
flags = fcntl(fd, F_GETFD);
if (flags == -1 || (flags & FD_CLOEXEC)) {
- error_report("fd is not valid or already in use");
+ error_setg(errp, "fd is not valid or already in use");
return -1;
}
if (fdset_id < 0) {
- error_report("set option is required and must be non-negative");
+ error_setg(errp, "set option is required and must be non-negative");
return -1;
}
@@ -1092,7 +1092,7 @@ static int parse_add_fd(void *opaque, QemuOpts *opts, Error **errp)
}
#endif
if (dupfd == -1) {
- error_report("error duplicating fd: %s", strerror(errno));
+ error_setg(errp, "error duplicating fd: %s", strerror(errno));
return -1;
}
@@ -3983,15 +3983,11 @@ int main(int argc, char **argv, char **envp)
}
#ifndef _WIN32
- if (qemu_opts_foreach(qemu_find_opts("add-fd"),
- parse_add_fd, NULL, NULL)) {
- exit(1);
- }
+ qemu_opts_foreach(qemu_find_opts("add-fd"),
+ parse_add_fd, NULL, &error_fatal);
- if (qemu_opts_foreach(qemu_find_opts("add-fd"),
- cleanup_add_fd, NULL, NULL)) {
- exit(1);
- }
+ qemu_opts_foreach(qemu_find_opts("add-fd"),
+ cleanup_add_fd, NULL, &error_fatal);
#endif
current_machine = MACHINE(object_new(object_class_get_name(
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* [Qemu-devel] [PATCH v2 19/35] qom: Clean up error reporting in user_creatable_add_opts_foreach()
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (17 preceding siblings ...)
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 18/35] vl: Clean up error reporting in parse_add_fd() Markus Armbruster
@ 2018-10-15 11:52 ` Markus Armbruster
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 20/35] vl: Clean up error reporting in chardev_init_func() Markus Armbruster
` (15 subsequent siblings)
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:52 UTC (permalink / raw)
To: qemu-devel
Calling error_report() in a function that takes an Error ** argument
is suspicious. user_creatable_add_opts_foreach() does that, and then
fails without setting an error. Its caller main(), via
qemu_opts_foreach(), is fine with it, but clean it up anyway.
Cc: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
qemu-io.c | 8 +++-----
qemu-nbd.c | 8 +++-----
qom/object_interfaces.c | 4 +---
vl.c | 16 ++++++----------
4 files changed, 13 insertions(+), 23 deletions(-)
diff --git a/qemu-io.c b/qemu-io.c
index 13829f5e21..6df7731af4 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -620,11 +620,9 @@ int main(int argc, char **argv)
exit(1);
}
- if (qemu_opts_foreach(&qemu_object_opts,
- user_creatable_add_opts_foreach,
- NULL, NULL)) {
- exit(1);
- }
+ qemu_opts_foreach(&qemu_object_opts,
+ user_creatable_add_opts_foreach,
+ NULL, &error_fatal);
if (!trace_init_backends()) {
exit(1);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 7874bc973c..ca7109652e 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -766,11 +766,9 @@ int main(int argc, char **argv)
exit(EXIT_FAILURE);
}
- if (qemu_opts_foreach(&qemu_object_opts,
- user_creatable_add_opts_foreach,
- NULL, NULL)) {
- exit(EXIT_FAILURE);
- }
+ qemu_opts_foreach(&qemu_object_opts,
+ user_creatable_add_opts_foreach,
+ NULL, &error_fatal);
if (!trace_init_backends()) {
exit(1);
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 941fd63afd..97b79b48bb 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -143,7 +143,6 @@ int user_creatable_add_opts_foreach(void *opaque, QemuOpts *opts, Error **errp)
{
bool (*type_opt_predicate)(const char *, QemuOpts *) = opaque;
Object *obj = NULL;
- Error *err = NULL;
const char *type;
type = qemu_opt_get(opts, "qom-type");
@@ -152,9 +151,8 @@ int user_creatable_add_opts_foreach(void *opaque, QemuOpts *opts, Error **errp)
return 0;
}
- obj = user_creatable_add_opts(opts, &err);
+ obj = user_creatable_add_opts(opts, errp);
if (!obj) {
- error_report_err(err);
return -1;
}
object_unref(obj);
diff --git a/vl.c b/vl.c
index 2ef066ad61..bb18f2d995 100644
--- a/vl.c
+++ b/vl.c
@@ -4228,11 +4228,9 @@ int main(int argc, char **argv, char **envp)
page_size_init();
socket_init();
- if (qemu_opts_foreach(qemu_find_opts("object"),
- user_creatable_add_opts_foreach,
- object_create_initial, NULL)) {
- exit(1);
- }
+ qemu_opts_foreach(qemu_find_opts("object"),
+ user_creatable_add_opts_foreach,
+ object_create_initial, &error_fatal);
if (qemu_opts_foreach(qemu_find_opts("chardev"),
chardev_init_func, NULL, NULL)) {
@@ -4363,11 +4361,9 @@ int main(int argc, char **argv, char **envp)
exit(1);
}
- if (qemu_opts_foreach(qemu_find_opts("object"),
- user_creatable_add_opts_foreach,
- object_create_delayed, NULL)) {
- exit(1);
- }
+ qemu_opts_foreach(qemu_find_opts("object"),
+ user_creatable_add_opts_foreach,
+ object_create_delayed, &error_fatal);
if (tpm_init() < 0) {
exit(1);
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* [Qemu-devel] [PATCH v2 20/35] vl: Clean up error reporting in chardev_init_func()
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (18 preceding siblings ...)
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 19/35] qom: Clean up error reporting in user_creatable_add_opts_foreach() Markus Armbruster
@ 2018-10-15 11:52 ` Markus Armbruster
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 21/35] vl: Clean up error reporting in machine_set_property() Markus Armbruster
` (14 subsequent siblings)
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:52 UTC (permalink / raw)
To: qemu-devel
Calling error_report() in a function that takes an Error ** argument
is suspicious. chardev_init_func() does that, and then fails without
setting an error. Its caller main(), via qemu_opts_foreach(), is fine
with it, but clean it up anyway.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
vl.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/vl.c b/vl.c
index bb18f2d995..aa03192019 100644
--- a/vl.c
+++ b/vl.c
@@ -2239,7 +2239,7 @@ static int chardev_init_func(void *opaque, QemuOpts *opts, Error **errp)
if (!qemu_chr_new_from_opts(opts, &local_err)) {
if (local_err) {
- error_report_err(local_err);
+ error_propagate(errp, local_err);
return -1;
}
exit(0);
@@ -4232,10 +4232,8 @@ int main(int argc, char **argv, char **envp)
user_creatable_add_opts_foreach,
object_create_initial, &error_fatal);
- if (qemu_opts_foreach(qemu_find_opts("chardev"),
- chardev_init_func, NULL, NULL)) {
- exit(1);
- }
+ qemu_opts_foreach(qemu_find_opts("chardev"),
+ chardev_init_func, NULL, &error_fatal);
#ifdef CONFIG_VIRTFS
if (qemu_opts_foreach(qemu_find_opts("fsdev"),
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* [Qemu-devel] [PATCH v2 21/35] vl: Clean up error reporting in machine_set_property()
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (19 preceding siblings ...)
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 20/35] vl: Clean up error reporting in chardev_init_func() Markus Armbruster
@ 2018-10-15 11:52 ` Markus Armbruster
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 22/35] vl: Clean up error reporting in mon_init_func() Markus Armbruster
` (13 subsequent siblings)
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:52 UTC (permalink / raw)
To: qemu-devel
Calling error_report() in a function that takes an Error ** argument
is suspicious. machine_set_property() does that, and then fails without
setting an error. Its caller main(), via qemu_opts_foreach(), is fine
with it, but clean it up anyway.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
vl.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/vl.c b/vl.c
index aa03192019..c3c17106bf 100644
--- a/vl.c
+++ b/vl.c
@@ -2676,7 +2676,7 @@ static int machine_set_property(void *opaque,
g_free(qom_name);
if (local_err) {
- error_report_err(local_err);
+ error_propagate(errp, local_err);
return -1;
}
@@ -4248,11 +4248,8 @@ int main(int argc, char **argv, char **envp)
}
machine_opts = qemu_get_machine_opts();
- if (qemu_opt_foreach(machine_opts, machine_set_property, current_machine,
- NULL)) {
- object_unref(OBJECT(current_machine));
- exit(1);
- }
+ qemu_opt_foreach(machine_opts, machine_set_property, current_machine,
+ &error_fatal);
configure_accelerator(current_machine);
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* [Qemu-devel] [PATCH v2 22/35] vl: Clean up error reporting in mon_init_func()
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (20 preceding siblings ...)
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 21/35] vl: Clean up error reporting in machine_set_property() Markus Armbruster
@ 2018-10-15 11:52 ` Markus Armbruster
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 23/35] vl: Clean up error reporting in parse_fw_cfg() Markus Armbruster
` (12 subsequent siblings)
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:52 UTC (permalink / raw)
To: qemu-devel
Calling error_report() in a function that takes an Error ** argument
is suspicious. mon_init_func() does that, and then fails without
setting an error. Its caller main(), via qemu_opts_foreach(), is fine
with it, but clean it up anyway.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
vl.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/vl.c b/vl.c
index c3c17106bf..cd8e8a6586 100644
--- a/vl.c
+++ b/vl.c
@@ -2270,8 +2270,8 @@ static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp)
} else if (strcmp(mode, "control") == 0) {
flags = MONITOR_USE_CONTROL;
} else {
- error_report("unknown monitor mode \"%s\"", mode);
- exit(1);
+ error_setg(errp, "unknown monitor mode \"%s\"", mode);
+ return -1;
}
if (qemu_opt_get_bool(opts, "pretty", 0))
@@ -2285,8 +2285,8 @@ static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp)
chardev = qemu_opt_get(opts, "chardev");
chr = qemu_chr_find(chardev);
if (chr == NULL) {
- error_report("chardev \"%s\" not found", chardev);
- exit(1);
+ error_setg(errp, "chardev \"%s\" not found", chardev);
+ return -1;
}
monitor_init(chr, flags);
@@ -4412,10 +4412,8 @@ int main(int argc, char **argv, char **envp)
default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
- if (qemu_opts_foreach(qemu_find_opts("mon"),
- mon_init_func, NULL, NULL)) {
- exit(1);
- }
+ qemu_opts_foreach(qemu_find_opts("mon"),
+ mon_init_func, NULL, &error_fatal);
if (foreach_device_config(DEV_SERIAL, serial_parse) < 0)
exit(1);
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* [Qemu-devel] [PATCH v2 23/35] vl: Clean up error reporting in parse_fw_cfg()
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (21 preceding siblings ...)
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 22/35] vl: Clean up error reporting in mon_init_func() Markus Armbruster
@ 2018-10-15 11:52 ` Markus Armbruster
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 24/35] vl: Clean up error reporting in device_init_func() Markus Armbruster
` (11 subsequent siblings)
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:52 UTC (permalink / raw)
To: qemu-devel
Calling error_report() in a function that takes an Error ** argument
is suspicious. parse_fw_cfg() does that, and then fails without
setting an error. Its caller main(), via qemu_opts_foreach(), is fine
with it, but clean it up anyway.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
vl.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/vl.c b/vl.c
index cd8e8a6586..c5bc54f72b 100644
--- a/vl.c
+++ b/vl.c
@@ -2174,7 +2174,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp)
FWCfgState *fw_cfg = (FWCfgState *) opaque;
if (fw_cfg == NULL) {
- error_report("fw_cfg device not available");
+ error_setg(errp, "fw_cfg device not available");
return -1;
}
name = qemu_opt_get(opts, "name");
@@ -2183,15 +2183,16 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp)
/* we need name and either a file or the content string */
if (!(nonempty_str(name) && (nonempty_str(file) || nonempty_str(str)))) {
- error_report("invalid argument(s)");
+ error_setg(errp, "invalid argument(s)");
return -1;
}
if (nonempty_str(file) && nonempty_str(str)) {
- error_report("file and string are mutually exclusive");
+ error_setg(errp, "file and string are mutually exclusive");
return -1;
}
if (strlen(name) > FW_CFG_MAX_FILE_PATH - 1) {
- error_report("name too long (max. %d char)", FW_CFG_MAX_FILE_PATH - 1);
+ error_setg(errp, "name too long (max. %d char)",
+ FW_CFG_MAX_FILE_PATH - 1);
return -1;
}
if (strncmp(name, "opt/", 4) != 0) {
@@ -2203,7 +2204,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp)
buf = g_memdup(str, size);
} else {
if (!g_file_get_contents(file, &buf, &size, NULL)) {
- error_report("can't load %s", file);
+ error_setg(errp, "can't load %s", file);
return -1;
}
}
@@ -4476,10 +4477,8 @@ int main(int argc, char **argv, char **envp)
hax_sync_vcpus();
}
- if (qemu_opts_foreach(qemu_find_opts("fw_cfg"),
- parse_fw_cfg, fw_cfg_find(), NULL) != 0) {
- exit(1);
- }
+ qemu_opts_foreach(qemu_find_opts("fw_cfg"),
+ parse_fw_cfg, fw_cfg_find(), &error_fatal);
/* init USB devices */
if (machine_usb(current_machine)) {
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* [Qemu-devel] [PATCH v2 24/35] vl: Clean up error reporting in device_init_func()
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (22 preceding siblings ...)
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 23/35] vl: Clean up error reporting in parse_fw_cfg() Markus Armbruster
@ 2018-10-15 11:52 ` Markus Armbruster
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 25/35] ui/keymaps: Fix handling of erroneous include files Markus Armbruster
` (10 subsequent siblings)
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:52 UTC (permalink / raw)
To: qemu-devel
Calling error_report() in a function that takes an Error ** argument
is suspicious. device_init_func() does that, and then fails without
setting an error. Its caller main(), via qemu_opts_foreach(), is fine
with it, but clean it up anyway.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
vl.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/vl.c b/vl.c
index c5bc54f72b..c053117028 100644
--- a/vl.c
+++ b/vl.c
@@ -2222,12 +2222,10 @@ static int device_help_func(void *opaque, QemuOpts *opts, Error **errp)
static int device_init_func(void *opaque, QemuOpts *opts, Error **errp)
{
- Error *err = NULL;
DeviceState *dev;
- dev = qdev_device_add(opts, &err);
+ dev = qdev_device_add(opts, errp);
if (!dev) {
- error_report_err(err);
return -1;
}
object_unref(OBJECT(dev));
@@ -4491,10 +4489,8 @@ int main(int argc, char **argv, char **envp)
/* init generic devices */
rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE);
- if (qemu_opts_foreach(qemu_find_opts("device"),
- device_init_func, NULL, NULL)) {
- exit(1);
- }
+ qemu_opts_foreach(qemu_find_opts("device"),
+ device_init_func, NULL, &error_fatal);
cpu_synchronize_all_post_init();
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* [Qemu-devel] [PATCH v2 25/35] ui/keymaps: Fix handling of erroneous include files
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (23 preceding siblings ...)
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 24/35] vl: Clean up error reporting in device_init_func() Markus Armbruster
@ 2018-10-15 11:52 ` Markus Armbruster
2018-10-15 11:53 ` [Qemu-devel] [PATCH v2 26/35] ui: Convert vnc_display_init(), init_keyboard_layout() to Error Markus Armbruster
` (9 subsequent siblings)
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:52 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann
While errors in the keyboard layout named with -k are fatal, errors in
included files are reported, but otherwise ignored:
$ cat worst
include bad
include worse
$ ls -l bad worse
ls: cannot access 'bad': No such file or directory
ls: cannot access 'worse': No such file or directory
$ qemu-system-x86_64 -nodefaults -S -monitor stdio -display vnc=:0 -k bad
QEMU 3.0.50 monitor - type 'help' for more information
(qemu) Could not read keymap file: 'bad'
$ qemu-system-x86_64 -nodefaults -S -monitor stdio -display vnc=:0 -k worst
QEMU 3.0.50 monitor - type 'help' for more information
(qemu) Could not read keymap file: 'bad'
Could not read keymap file: 'worse'
Fix that.
Note that parse_keyboard_layout() allocates the keymap, except when
it's parsing an include file. To keep error handling simple, move the
memory management to its caller init_keyboard_layout().
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
ui/keymaps.c | 35 +++++++++++++++++++++++------------
1 file changed, 23 insertions(+), 12 deletions(-)
diff --git a/ui/keymaps.c b/ui/keymaps.c
index 43fe604724..b05fb028dc 100644
--- a/ui/keymaps.c
+++ b/ui/keymaps.c
@@ -79,10 +79,11 @@ static void add_keysym(char *line, int keysym, int keycode, kbd_layout_t *k)
trace_keymap_add(keysym, keycode, line);
}
-static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
- const char *language,
- kbd_layout_t *k)
+static int parse_keyboard_layout(kbd_layout_t *k,
+ const name2keysym_t *table,
+ const char *language)
{
+ int ret;
FILE *f;
char * filename;
char line[1024];
@@ -95,12 +96,7 @@ static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
g_free(filename);
if (!f) {
fprintf(stderr, "Could not read keymap file: '%s'\n", language);
- return NULL;
- }
-
- if (!k) {
- k = g_new0(kbd_layout_t, 1);
- k->hash = g_hash_table_new(NULL, NULL);
+ return -1;
}
for(;;) {
@@ -118,7 +114,10 @@ static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
continue;
}
if (!strncmp(line, "include ", 8)) {
- parse_keyboard_layout(table, line + 8, k);
+ if (parse_keyboard_layout(k, table, line + 8) < 0) {
+ ret = -1;
+ goto out;
+ }
} else {
int offset = 0;
while (line[offset] != 0 &&
@@ -164,15 +163,27 @@ static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
}
}
}
+
+ ret = 0;
+out:
fclose(f);
- return k;
+ return ret;
}
kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
const char *language)
{
- return parse_keyboard_layout(table, language, NULL);
+ kbd_layout_t *k;
+
+ k = g_new0(kbd_layout_t, 1);
+ k->hash = g_hash_table_new(NULL, NULL);
+ if (parse_keyboard_layout(k, table, language) < 0) {
+ g_hash_table_unref(k->hash);
+ g_free(k);
+ return NULL;
+ }
+ return k;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* [Qemu-devel] [PATCH v2 26/35] ui: Convert vnc_display_init(), init_keyboard_layout() to Error
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (24 preceding siblings ...)
2018-10-15 11:52 ` [Qemu-devel] [PATCH v2 25/35] ui/keymaps: Fix handling of erroneous include files Markus Armbruster
@ 2018-10-15 11:53 ` Markus Armbruster
2018-10-15 22:42 ` Philippe Mathieu-Daudé
2018-10-15 11:53 ` [Qemu-devel] [PATCH v2 27/35] vnc: Clean up error reporting in vnc_init_func() Markus Armbruster
` (8 subsequent siblings)
34 siblings, 1 reply; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Fei Li, Gerd Hoffmann
From: Fei Li <fli@suse.com>
Signed-off-by: Fei Li <fli@suse.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
include/ui/console.h | 2 +-
ui/curses.c | 6 +++---
ui/keymaps.c | 11 ++++++-----
ui/keymaps.h | 2 +-
ui/sdl.c | 6 +++---
ui/vnc.c | 15 ++++++++++-----
6 files changed, 24 insertions(+), 18 deletions(-)
diff --git a/include/ui/console.h b/include/ui/console.h
index fb969caf70..c17803c530 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -453,7 +453,7 @@ void qemu_display_early_init(DisplayOptions *opts);
void qemu_display_init(DisplayState *ds, DisplayOptions *opts);
/* vnc.c */
-void vnc_display_init(const char *id);
+void vnc_display_init(const char *id, Error **errp);
void vnc_display_open(const char *id, Error **errp);
void vnc_display_add_client(const char *id, int csock, bool skipauth);
int vnc_display_password(const char *id, const char *password);
diff --git a/ui/curses.c b/ui/curses.c
index 59d819fd4d..f4e7a12f74 100644
--- a/ui/curses.c
+++ b/ui/curses.c
@@ -28,6 +28,7 @@
#include <termios.h>
#endif
+#include "qapi/error.h"
#include "qemu-common.h"
#include "ui/console.h"
#include "ui/input.h"
@@ -421,9 +422,8 @@ static void curses_keyboard_setup(void)
keyboard_layout = "en-us";
#endif
if(keyboard_layout) {
- kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
- if (!kbd_layout)
- exit(1);
+ kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout,
+ &error_fatal);
}
}
diff --git a/ui/keymaps.c b/ui/keymaps.c
index b05fb028dc..085889b555 100644
--- a/ui/keymaps.c
+++ b/ui/keymaps.c
@@ -27,6 +27,7 @@
#include "sysemu/sysemu.h"
#include "trace.h"
#include "qemu/error-report.h"
+#include "qapi/error.h"
struct keysym2code {
uint32_t count;
@@ -81,7 +82,7 @@ static void add_keysym(char *line, int keysym, int keycode, kbd_layout_t *k)
static int parse_keyboard_layout(kbd_layout_t *k,
const name2keysym_t *table,
- const char *language)
+ const char *language, Error **errp)
{
int ret;
FILE *f;
@@ -95,7 +96,7 @@ static int parse_keyboard_layout(kbd_layout_t *k,
f = filename ? fopen(filename, "r") : NULL;
g_free(filename);
if (!f) {
- fprintf(stderr, "Could not read keymap file: '%s'\n", language);
+ error_setg(errp, "could not read keymap file: '%s'", language);
return -1;
}
@@ -114,7 +115,7 @@ static int parse_keyboard_layout(kbd_layout_t *k,
continue;
}
if (!strncmp(line, "include ", 8)) {
- if (parse_keyboard_layout(k, table, line + 8) < 0) {
+ if (parse_keyboard_layout(k, table, line + 8, errp) < 0) {
ret = -1;
goto out;
}
@@ -172,13 +173,13 @@ out:
kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
- const char *language)
+ const char *language, Error **errp)
{
kbd_layout_t *k;
k = g_new0(kbd_layout_t, 1);
k->hash = g_hash_table_new(NULL, NULL);
- if (parse_keyboard_layout(k, table, language) < 0) {
+ if (parse_keyboard_layout(k, table, language, errp) < 0) {
g_hash_table_unref(k->hash);
g_free(k);
return NULL;
diff --git a/ui/keymaps.h b/ui/keymaps.h
index 0693588225..98213a4191 100644
--- a/ui/keymaps.h
+++ b/ui/keymaps.h
@@ -53,7 +53,7 @@ typedef struct {
typedef struct kbd_layout_t kbd_layout_t;
kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
- const char *language);
+ const char *language, Error **errp);
int keysym2scancode(kbd_layout_t *k, int keysym,
bool shift, bool altgr, bool ctrl);
int keycode_is_keypad(kbd_layout_t *k, int keycode);
diff --git a/ui/sdl.c b/ui/sdl.c
index a5fd503c25..190b16f575 100644
--- a/ui/sdl.c
+++ b/ui/sdl.c
@@ -29,6 +29,7 @@
#include <SDL.h>
#include <SDL_syswm.h>
+#include "qapi/error.h"
#include "qemu-common.h"
#include "qemu/cutils.h"
#include "ui/console.h"
@@ -917,9 +918,8 @@ static void sdl1_display_init(DisplayState *ds, DisplayOptions *o)
keyboard_layout = "en-us";
#endif
if(keyboard_layout) {
- kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
- if (!kbd_layout)
- exit(1);
+ kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout,
+ &error_fatal);
}
g_printerr("Running QEMU with SDL 1.2 is deprecated, and will be removed\n"
diff --git a/ui/vnc.c b/ui/vnc.c
index cf221c83cc..98e3d3b1d8 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3205,7 +3205,7 @@ static const DisplayChangeListenerOps dcl_ops = {
.dpy_cursor_define = vnc_dpy_cursor_define,
};
-void vnc_display_init(const char *id)
+void vnc_display_init(const char *id, Error **errp)
{
VncDisplay *vd;
@@ -3222,13 +3222,14 @@ void vnc_display_init(const char *id)
if (keyboard_layout) {
trace_vnc_key_map_init(keyboard_layout);
- vd->kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
+ vd->kbd_layout = init_keyboard_layout(name2keysym,
+ keyboard_layout, errp);
} else {
- vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us");
+ vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us", errp);
}
if (!vd->kbd_layout) {
- exit(1);
+ return;
}
vd->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;
@@ -4079,7 +4080,11 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
char *id = (char *)qemu_opts_id(opts);
assert(id);
- vnc_display_init(id);
+ vnc_display_init(id, &local_err);
+ if (local_err) {
+ error_report_err(local_err);
+ exit(1);
+ }
vnc_display_open(id, &local_err);
if (local_err != NULL) {
error_reportf_err(local_err, "Failed to start VNC server: ");
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [Qemu-devel] [PATCH v2 26/35] ui: Convert vnc_display_init(), init_keyboard_layout() to Error
2018-10-15 11:53 ` [Qemu-devel] [PATCH v2 26/35] ui: Convert vnc_display_init(), init_keyboard_layout() to Error Markus Armbruster
@ 2018-10-15 22:42 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 50+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-15 22:42 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: Fei Li, Gerd Hoffmann
On 15/10/2018 13:53, Markus Armbruster wrote:
> From: Fei Li <fli@suse.com>
>
> Signed-off-by: Fei Li <fli@suse.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> include/ui/console.h | 2 +-
> ui/curses.c | 6 +++---
> ui/keymaps.c | 11 ++++++-----
> ui/keymaps.h | 2 +-
> ui/sdl.c | 6 +++---
> ui/vnc.c | 15 ++++++++++-----
> 6 files changed, 24 insertions(+), 18 deletions(-)
>
> diff --git a/include/ui/console.h b/include/ui/console.h
> index fb969caf70..c17803c530 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -453,7 +453,7 @@ void qemu_display_early_init(DisplayOptions *opts);
> void qemu_display_init(DisplayState *ds, DisplayOptions *opts);
>
> /* vnc.c */
> -void vnc_display_init(const char *id);
> +void vnc_display_init(const char *id, Error **errp);
> void vnc_display_open(const char *id, Error **errp);
> void vnc_display_add_client(const char *id, int csock, bool skipauth);
> int vnc_display_password(const char *id, const char *password);
> diff --git a/ui/curses.c b/ui/curses.c
> index 59d819fd4d..f4e7a12f74 100644
> --- a/ui/curses.c
> +++ b/ui/curses.c
> @@ -28,6 +28,7 @@
> #include <termios.h>
> #endif
>
> +#include "qapi/error.h"
> #include "qemu-common.h"
> #include "ui/console.h"
> #include "ui/input.h"
> @@ -421,9 +422,8 @@ static void curses_keyboard_setup(void)
> keyboard_layout = "en-us";
> #endif
> if(keyboard_layout) {
> - kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
> - if (!kbd_layout)
> - exit(1);
> + kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout,
> + &error_fatal);
> }
> }
>
> diff --git a/ui/keymaps.c b/ui/keymaps.c
> index b05fb028dc..085889b555 100644
> --- a/ui/keymaps.c
> +++ b/ui/keymaps.c
> @@ -27,6 +27,7 @@
> #include "sysemu/sysemu.h"
> #include "trace.h"
> #include "qemu/error-report.h"
> +#include "qapi/error.h"
>
> struct keysym2code {
> uint32_t count;
> @@ -81,7 +82,7 @@ static void add_keysym(char *line, int keysym, int keycode, kbd_layout_t *k)
>
> static int parse_keyboard_layout(kbd_layout_t *k,
> const name2keysym_t *table,
> - const char *language)
> + const char *language, Error **errp)
> {
> int ret;
> FILE *f;
> @@ -95,7 +96,7 @@ static int parse_keyboard_layout(kbd_layout_t *k,
> f = filename ? fopen(filename, "r") : NULL;
> g_free(filename);
> if (!f) {
> - fprintf(stderr, "Could not read keymap file: '%s'\n", language);
> + error_setg(errp, "could not read keymap file: '%s'", language);
> return -1;
> }
>
> @@ -114,7 +115,7 @@ static int parse_keyboard_layout(kbd_layout_t *k,
> continue;
> }
> if (!strncmp(line, "include ", 8)) {
> - if (parse_keyboard_layout(k, table, line + 8) < 0) {
> + if (parse_keyboard_layout(k, table, line + 8, errp) < 0) {
> ret = -1;
> goto out;
> }
> @@ -172,13 +173,13 @@ out:
>
>
> kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
> - const char *language)
> + const char *language, Error **errp)
> {
> kbd_layout_t *k;
>
> k = g_new0(kbd_layout_t, 1);
> k->hash = g_hash_table_new(NULL, NULL);
> - if (parse_keyboard_layout(k, table, language) < 0) {
> + if (parse_keyboard_layout(k, table, language, errp) < 0) {
> g_hash_table_unref(k->hash);
> g_free(k);
> return NULL;
> diff --git a/ui/keymaps.h b/ui/keymaps.h
> index 0693588225..98213a4191 100644
> --- a/ui/keymaps.h
> +++ b/ui/keymaps.h
> @@ -53,7 +53,7 @@ typedef struct {
> typedef struct kbd_layout_t kbd_layout_t;
>
> kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
> - const char *language);
> + const char *language, Error **errp);
> int keysym2scancode(kbd_layout_t *k, int keysym,
> bool shift, bool altgr, bool ctrl);
> int keycode_is_keypad(kbd_layout_t *k, int keycode);
> diff --git a/ui/sdl.c b/ui/sdl.c
> index a5fd503c25..190b16f575 100644
> --- a/ui/sdl.c
> +++ b/ui/sdl.c
> @@ -29,6 +29,7 @@
> #include <SDL.h>
> #include <SDL_syswm.h>
>
> +#include "qapi/error.h"
> #include "qemu-common.h"
> #include "qemu/cutils.h"
> #include "ui/console.h"
> @@ -917,9 +918,8 @@ static void sdl1_display_init(DisplayState *ds, DisplayOptions *o)
> keyboard_layout = "en-us";
> #endif
> if(keyboard_layout) {
> - kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
> - if (!kbd_layout)
> - exit(1);
> + kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout,
> + &error_fatal);
> }
>
> g_printerr("Running QEMU with SDL 1.2 is deprecated, and will be removed\n"
> diff --git a/ui/vnc.c b/ui/vnc.c
> index cf221c83cc..98e3d3b1d8 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3205,7 +3205,7 @@ static const DisplayChangeListenerOps dcl_ops = {
> .dpy_cursor_define = vnc_dpy_cursor_define,
> };
>
> -void vnc_display_init(const char *id)
> +void vnc_display_init(const char *id, Error **errp)
> {
> VncDisplay *vd;
>
> @@ -3222,13 +3222,14 @@ void vnc_display_init(const char *id)
>
> if (keyboard_layout) {
> trace_vnc_key_map_init(keyboard_layout);
> - vd->kbd_layout = init_keyboard_layout(name2keysym, keyboard_layout);
> + vd->kbd_layout = init_keyboard_layout(name2keysym,
> + keyboard_layout, errp);
> } else {
> - vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us");
> + vd->kbd_layout = init_keyboard_layout(name2keysym, "en-us", errp);
> }
>
> if (!vd->kbd_layout) {
> - exit(1);
> + return;
> }
>
> vd->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;
> @@ -4079,7 +4080,11 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
> char *id = (char *)qemu_opts_id(opts);
>
> assert(id);
> - vnc_display_init(id);
> + vnc_display_init(id, &local_err);
> + if (local_err) {
> + error_report_err(local_err);
> + exit(1);
> + }
> vnc_display_open(id, &local_err);
> if (local_err != NULL) {
> error_reportf_err(local_err, "Failed to start VNC server: ");
>
^ permalink raw reply [flat|nested] 50+ messages in thread
* [Qemu-devel] [PATCH v2 27/35] vnc: Clean up error reporting in vnc_init_func()
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (25 preceding siblings ...)
2018-10-15 11:53 ` [Qemu-devel] [PATCH v2 26/35] ui: Convert vnc_display_init(), init_keyboard_layout() to Error Markus Armbruster
@ 2018-10-15 11:53 ` Markus Armbruster
2018-10-15 12:51 ` Fei Li
2018-10-15 22:41 ` Philippe Mathieu-Daudé
2018-10-15 11:53 ` [Qemu-devel] [PATCH v2 28/35] numa: Clean up error reporting in parse_numa() Markus Armbruster
` (7 subsequent siblings)
34 siblings, 2 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann
Calling error_report() in a function that takes an Error ** argument
is suspicious. vnc_init_func() does that, and then fails without
setting an error. Its caller main(), via qemu_opts_foreach(), is fine
with it, but clean it up anyway.
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
ui/vnc.c | 9 +++++----
vl.c | 2 +-
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/ui/vnc.c b/ui/vnc.c
index 98e3d3b1d8..fcd2744d52 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -4082,13 +4082,14 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
assert(id);
vnc_display_init(id, &local_err);
if (local_err) {
- error_report_err(local_err);
- exit(1);
+ error_propagate(errp, local_err);
+ return -1;
}
vnc_display_open(id, &local_err);
if (local_err != NULL) {
- error_reportf_err(local_err, "Failed to start VNC server: ");
- exit(1);
+ error_propagate_prepend(errp, local_err,
+ "Failed to start VNC server: ");
+ return -1;
}
return 0;
}
diff --git a/vl.c b/vl.c
index c053117028..8e0006d49c 100644
--- a/vl.c
+++ b/vl.c
@@ -4526,7 +4526,7 @@ int main(int argc, char **argv, char **envp)
/* init remote displays */
#ifdef CONFIG_VNC
qemu_opts_foreach(qemu_find_opts("vnc"),
- vnc_init_func, NULL, NULL);
+ vnc_init_func, NULL, &error_fatal);
#endif
if (using_spice) {
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [Qemu-devel] [PATCH v2 27/35] vnc: Clean up error reporting in vnc_init_func()
2018-10-15 11:53 ` [Qemu-devel] [PATCH v2 27/35] vnc: Clean up error reporting in vnc_init_func() Markus Armbruster
@ 2018-10-15 12:51 ` Fei Li
2018-10-16 4:08 ` Markus Armbruster
2018-10-15 22:41 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 50+ messages in thread
From: Fei Li @ 2018-10-15 12:51 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: Gerd Hoffmann
On 10/15/2018 07:53 PM, Markus Armbruster wrote:
> Calling error_report() in a function that takes an Error ** argument
> is suspicious. vnc_init_func() does that, and then fails without
> setting an error. Its caller main(), via qemu_opts_foreach(), is fine
> with it, but clean it up anyway.
>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> ui/vnc.c | 9 +++++----
> vl.c | 2 +-
> 2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 98e3d3b1d8..fcd2744d52 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -4082,13 +4082,14 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
> assert(id);
> vnc_display_init(id, &local_err);
> if (local_err) {
> - error_report_err(local_err);
> - exit(1);
> + error_propagate(errp, local_err);
Shall we use error_propagate(errp, local_err, ("Failed to init VNC
server: ");
like vnc_display_open does?
If yes, I guess this error message should be added from last patch [26/35].
If not, please omit. :)
Have a nice day
Fei
> + return -1;
> }
> vnc_display_open(id, &local_err);
> if (local_err != NULL) {
> - error_reportf_err(local_err, "Failed to start VNC server: ");
> - exit(1);
> + error_propagate_prepend(errp, local_err,
> + "Failed to start VNC server: ");
> + return -1;
> }
> return 0;
> }
> diff --git a/vl.c b/vl.c
> index c053117028..8e0006d49c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4526,7 +4526,7 @@ int main(int argc, char **argv, char **envp)
> /* init remote displays */
> #ifdef CONFIG_VNC
> qemu_opts_foreach(qemu_find_opts("vnc"),
> - vnc_init_func, NULL, NULL);
> + vnc_init_func, NULL, &error_fatal);
> #endif
>
> if (using_spice) {
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [Qemu-devel] [PATCH v2 27/35] vnc: Clean up error reporting in vnc_init_func()
2018-10-15 12:51 ` Fei Li
@ 2018-10-16 4:08 ` Markus Armbruster
2018-10-16 6:52 ` Gerd Hoffmann
0 siblings, 1 reply; 50+ messages in thread
From: Markus Armbruster @ 2018-10-16 4:08 UTC (permalink / raw)
To: Fei Li; +Cc: qemu-devel, Gerd Hoffmann
Fei Li <fli@suse.com> writes:
> On 10/15/2018 07:53 PM, Markus Armbruster wrote:
>> Calling error_report() in a function that takes an Error ** argument
>> is suspicious. vnc_init_func() does that, and then fails without
>> setting an error. Its caller main(), via qemu_opts_foreach(), is fine
>> with it, but clean it up anyway.
>>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> ui/vnc.c | 9 +++++----
>> vl.c | 2 +-
>> 2 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/ui/vnc.c b/ui/vnc.c
>> index 98e3d3b1d8..fcd2744d52 100644
>> --- a/ui/vnc.c
>> +++ b/ui/vnc.c
>> @@ -4082,13 +4082,14 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
>> assert(id);
>> vnc_display_init(id, &local_err);
>> if (local_err) {
>> - error_report_err(local_err);
>> - exit(1);
>> + error_propagate(errp, local_err);
> Shall we use error_propagate(errp, local_err, ("Failed to init VNC
> server: ");
> like vnc_display_open does?
I don't know.
The error reporting is somewhat poor around here. Consider:
$ qemu-system-x86_64 -display vnc=:0,share=nope
qemu-system-x86_64: -display vnc=:0,share=nope: Failed to start VNC server: unknown vnc share= option
The "Failed to start VNC server: " prefix doesn't really add value.
$ qemu-system-x86_64 -display vnc=:0 -k bad
qemu-system-x86_64: -display vnc=:0: could not read keymap file: 'bad'
The error message refers to -display instead of -k. I doubt adding the
prefix would improve it.
Gerd, what do you think?
> If yes, I guess this error message should be added from last patch [26/35].
Yes.
> If not, please omit. :)
>
> Have a nice day
> Fei
>> + return -1;
>> }
>> vnc_display_open(id, &local_err);
>> if (local_err != NULL) {
>> - error_reportf_err(local_err, "Failed to start VNC server: ");
>> - exit(1);
>> + error_propagate_prepend(errp, local_err,
>> + "Failed to start VNC server: ");
>> + return -1;
>> }
>> return 0;
>> }
>> diff --git a/vl.c b/vl.c
>> index c053117028..8e0006d49c 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -4526,7 +4526,7 @@ int main(int argc, char **argv, char **envp)
>> /* init remote displays */
>> #ifdef CONFIG_VNC
>> qemu_opts_foreach(qemu_find_opts("vnc"),
>> - vnc_init_func, NULL, NULL);
>> + vnc_init_func, NULL, &error_fatal);
>> #endif
>> if (using_spice) {
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [Qemu-devel] [PATCH v2 27/35] vnc: Clean up error reporting in vnc_init_func()
2018-10-16 4:08 ` Markus Armbruster
@ 2018-10-16 6:52 ` Gerd Hoffmann
2018-10-16 11:21 ` Markus Armbruster
0 siblings, 1 reply; 50+ messages in thread
From: Gerd Hoffmann @ 2018-10-16 6:52 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Fei Li, qemu-devel
> >> + error_propagate(errp, local_err);
> > Shall we use error_propagate(errp, local_err, ("Failed to init VNC
> > server: ");
> > like vnc_display_open does?
>
> I don't know.
>
> The error reporting is somewhat poor around here. Consider:
>
> $ qemu-system-x86_64 -display vnc=:0,share=nope
> qemu-system-x86_64: -display vnc=:0,share=nope: Failed to start VNC server: unknown vnc share= option
>
> The "Failed to start VNC server: " prefix doesn't really add value.
Indeed.
> Gerd, what do you think?
I'd keep the messages short.
cheers,
Gerd
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [Qemu-devel] [PATCH v2 27/35] vnc: Clean up error reporting in vnc_init_func()
2018-10-16 6:52 ` Gerd Hoffmann
@ 2018-10-16 11:21 ` Markus Armbruster
0 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-16 11:21 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Markus Armbruster, Fei Li, qemu-devel
Gerd Hoffmann <kraxel@redhat.com> writes:
>> >> + error_propagate(errp, local_err);
>> > Shall we use error_propagate(errp, local_err, ("Failed to init VNC
>> > server: ");
>> > like vnc_display_open does?
>>
>> I don't know.
>>
>> The error reporting is somewhat poor around here. Consider:
>>
>> $ qemu-system-x86_64 -display vnc=:0,share=nope
>> qemu-system-x86_64: -display vnc=:0,share=nope: Failed to start VNC server: unknown vnc share= option
>>
>> The "Failed to start VNC server: " prefix doesn't really add value.
>
> Indeed.
>
>> Gerd, what do you think?
>
> I'd keep the messages short.
Okay, I'll delete the prefix. Thanks!
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v2 27/35] vnc: Clean up error reporting in vnc_init_func()
2018-10-15 11:53 ` [Qemu-devel] [PATCH v2 27/35] vnc: Clean up error reporting in vnc_init_func() Markus Armbruster
2018-10-15 12:51 ` Fei Li
@ 2018-10-15 22:41 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 50+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-15 22:41 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: Gerd Hoffmann
On 15/10/2018 13:53, Markus Armbruster wrote:
> Calling error_report() in a function that takes an Error ** argument
> is suspicious. vnc_init_func() does that, and then fails without
> setting an error. Its caller main(), via qemu_opts_foreach(), is fine
> with it, but clean it up anyway.
>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> ui/vnc.c | 9 +++++----
> vl.c | 2 +-
> 2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 98e3d3b1d8..fcd2744d52 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -4082,13 +4082,14 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
> assert(id);
> vnc_display_init(id, &local_err);
> if (local_err) {
> - error_report_err(local_err);
> - exit(1);
> + error_propagate(errp, local_err);
> + return -1;
> }
> vnc_display_open(id, &local_err);
> if (local_err != NULL) {
> - error_reportf_err(local_err, "Failed to start VNC server: ");
> - exit(1);
> + error_propagate_prepend(errp, local_err,
> + "Failed to start VNC server: ");
> + return -1;
> }
> return 0;
> }
> diff --git a/vl.c b/vl.c
> index c053117028..8e0006d49c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4526,7 +4526,7 @@ int main(int argc, char **argv, char **envp)
> /* init remote displays */
> #ifdef CONFIG_VNC
> qemu_opts_foreach(qemu_find_opts("vnc"),
> - vnc_init_func, NULL, NULL);
> + vnc_init_func, NULL, &error_fatal);
> #endif
>
> if (using_spice) {
>
^ permalink raw reply [flat|nested] 50+ messages in thread
* [Qemu-devel] [PATCH v2 28/35] numa: Clean up error reporting in parse_numa()
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (26 preceding siblings ...)
2018-10-15 11:53 ` [Qemu-devel] [PATCH v2 27/35] vnc: Clean up error reporting in vnc_init_func() Markus Armbruster
@ 2018-10-15 11:53 ` Markus Armbruster
2018-10-15 11:53 ` [Qemu-devel] [PATCH v2 29/35] tpm: Clean up error reporting in tpm_init_tpmdev() Markus Armbruster
` (6 subsequent siblings)
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Eduardo Habkost
Calling error_report() in a function that takes an Error ** argument
is suspicious. parse_numa() does that, and then fails without setting
an error. Its caller main(), via qemu_opts_foreach(), is fine with
it, but clean it up anyway.
While there, give parse_numa() internal linkage.
Cc: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
include/sysemu/numa.h | 1 -
numa.c | 8 +++-----
2 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
index 7a0ae751aa..21713b7e2f 100644
--- a/include/sysemu/numa.h
+++ b/include/sysemu/numa.h
@@ -22,7 +22,6 @@ struct NumaNodeMem {
};
extern NodeInfo numa_info[MAX_NODES];
-int parse_numa(void *opaque, QemuOpts *opts, Error **errp);
void parse_numa_opts(MachineState *ms);
void numa_complete_configuration(MachineState *ms);
void query_numa_node_mem(NumaNodeMem node_mem[]);
diff --git a/numa.c b/numa.c
index 1d7c49ad43..50ec016013 100644
--- a/numa.c
+++ b/numa.c
@@ -215,7 +215,7 @@ end:
error_propagate(errp, err);
}
-int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
+static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
{
NumaOptions *object = NULL;
MachineState *ms = MACHINE(opaque);
@@ -239,7 +239,7 @@ int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
end:
qapi_free_NumaOptions(object);
if (err) {
- error_report_err(err);
+ error_propagate(errp, err);
return -1;
}
@@ -444,9 +444,7 @@ void numa_complete_configuration(MachineState *ms)
void parse_numa_opts(MachineState *ms)
{
- if (qemu_opts_foreach(qemu_find_opts("numa"), parse_numa, ms, NULL)) {
- exit(1);
- }
+ qemu_opts_foreach(qemu_find_opts("numa"), parse_numa, ms, &error_fatal);
}
void qmp_set_numa_node(NumaOptions *cmd, Error **errp)
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* [Qemu-devel] [PATCH v2 29/35] tpm: Clean up error reporting in tpm_init_tpmdev()
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (27 preceding siblings ...)
2018-10-15 11:53 ` [Qemu-devel] [PATCH v2 28/35] numa: Clean up error reporting in parse_numa() Markus Armbruster
@ 2018-10-15 11:53 ` Markus Armbruster
2018-10-15 11:53 ` [Qemu-devel] [PATCH v2 30/35] spice: Clean up error reporting in add_channel() Markus Armbruster
` (5 subsequent siblings)
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Stefan Berger
Calling error_report() in a function that takes an Error ** argument
is suspicious. tpm_init_tpmdev() does that, and then fails without
setting an error. Its caller main(), via tpm_init() and
qemu_opts_foreach(), is fine with it, but clean it up anyway.
Cc: Stefan Berger <stefanb@linux.vnet.ibm.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
---
include/sysemu/tpm.h | 2 +-
stubs/tpm.c | 3 +--
tpm.c | 22 +++++++++-------------
vl.c | 4 +---
4 files changed, 12 insertions(+), 19 deletions(-)
diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
index 9ae1ab6da3..17a97ed77a 100644
--- a/include/sysemu/tpm.h
+++ b/include/sysemu/tpm.h
@@ -16,7 +16,7 @@
#include "qom/object.h"
int tpm_config_parse(QemuOptsList *opts_list, const char *optarg);
-int tpm_init(void);
+void tpm_init(void);
void tpm_cleanup(void);
typedef enum TPMVersion {
diff --git a/stubs/tpm.c b/stubs/tpm.c
index 6729bc8517..80939cd3db 100644
--- a/stubs/tpm.c
+++ b/stubs/tpm.c
@@ -9,9 +9,8 @@
#include "qapi/qapi-commands-tpm.h"
#include "sysemu/tpm.h"
-int tpm_init(void)
+void tpm_init(void)
{
- return 0;
}
void tpm_cleanup(void)
diff --git a/tpm.c b/tpm.c
index 93031723ad..9c9e20bbb7 100644
--- a/tpm.c
+++ b/tpm.c
@@ -89,19 +89,19 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, Error **errp)
int i;
if (!QLIST_EMPTY(&tpm_backends)) {
- error_report("Only one TPM is allowed.");
+ error_setg(errp, "Only one TPM is allowed.");
return 1;
}
id = qemu_opts_id(opts);
if (id == NULL) {
- error_report(QERR_MISSING_PARAMETER, "id");
+ error_setg(errp, QERR_MISSING_PARAMETER, "id");
return 1;
}
value = qemu_opt_get(opts, "type");
if (!value) {
- error_report(QERR_MISSING_PARAMETER, "type");
+ error_setg(errp, QERR_MISSING_PARAMETER, "type");
tpm_display_backend_drivers();
return 1;
}
@@ -109,8 +109,8 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, Error **errp)
i = qapi_enum_parse(&TpmType_lookup, value, -1, NULL);
be = i >= 0 ? tpm_be_find_by_type(i) : NULL;
if (be == NULL) {
- error_report(QERR_INVALID_PARAMETER_VALUE,
- "type", "a TPM backend type");
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type",
+ "a TPM backend type");
tpm_display_backend_drivers();
return 1;
}
@@ -118,7 +118,7 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, Error **errp)
/* validate backend specific opts */
qemu_opts_validate(opts, be->opts, &local_err);
if (local_err) {
- error_report_err(local_err);
+ error_propagate(errp, local_err);
return 1;
}
@@ -151,14 +151,10 @@ void tpm_cleanup(void)
* Initialize the TPM. Process the tpmdev command line options describing the
* TPM backend.
*/
-int tpm_init(void)
+void tpm_init(void)
{
- if (qemu_opts_foreach(qemu_find_opts("tpmdev"),
- tpm_init_tpmdev, NULL, NULL)) {
- return -1;
- }
-
- return 0;
+ qemu_opts_foreach(qemu_find_opts("tpmdev"),
+ tpm_init_tpmdev, NULL, &error_fatal);
}
/*
diff --git a/vl.c b/vl.c
index 8e0006d49c..abfe991ed6 100644
--- a/vl.c
+++ b/vl.c
@@ -4359,9 +4359,7 @@ int main(int argc, char **argv, char **envp)
user_creatable_add_opts_foreach,
object_create_delayed, &error_fatal);
- if (tpm_init() < 0) {
- exit(1);
- }
+ tpm_init();
/* init the bluetooth world */
if (foreach_device_config(DEV_BT, bt_parse))
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* [Qemu-devel] [PATCH v2 30/35] spice: Clean up error reporting in add_channel()
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (28 preceding siblings ...)
2018-10-15 11:53 ` [Qemu-devel] [PATCH v2 29/35] tpm: Clean up error reporting in tpm_init_tpmdev() Markus Armbruster
@ 2018-10-15 11:53 ` Markus Armbruster
2018-10-15 11:53 ` [Qemu-devel] [PATCH v2 31/35] fsdev: Clean up error reporting in qemu_fsdev_add() Markus Armbruster
` (4 subsequent siblings)
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann
Calling error_report() in a function that takes an Error ** argument
is suspicious. add_channel() does that, and then exit()s. Its caller
main(), via qemu_opts_foreach(), is fine with it, but clean it up
anyway.
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
ui/spice-core.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/ui/spice-core.c b/ui/spice-core.c
index a4fbbc3898..ebaae24643 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -597,9 +597,9 @@ static int add_channel(void *opaque, const char *name, const char *value,
if (strcmp(name, "tls-channel") == 0) {
int *tls_port = opaque;
if (!*tls_port) {
- error_report("spice: tried to setup tls-channel"
- " without specifying a TLS port");
- exit(1);
+ error_setg(errp, "spice: tried to setup tls-channel"
+ " without specifying a TLS port");
+ return -1;
}
security = SPICE_CHANNEL_SECURITY_SSL;
}
@@ -615,8 +615,9 @@ static int add_channel(void *opaque, const char *name, const char *value,
rc = spice_server_set_channel_security(spice_server, value, security);
}
if (rc != 0) {
- error_report("spice: failed to set channel security for %s", value);
- exit(1);
+ error_setg(errp, "spice: failed to set channel security for %s",
+ value);
+ return -1;
}
return 0;
}
@@ -787,7 +788,7 @@ void qemu_spice_init(void)
spice_server_set_playback_compression
(spice_server, qemu_opt_get_bool(opts, "playback-compression", 1));
- qemu_opt_foreach(opts, add_channel, &tls_port, NULL);
+ qemu_opt_foreach(opts, add_channel, &tls_port, &error_fatal);
spice_server_set_name(spice_server, qemu_name);
spice_server_set_uuid(spice_server, (unsigned char *)&qemu_uuid);
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* [Qemu-devel] [PATCH v2 31/35] fsdev: Clean up error reporting in qemu_fsdev_add()
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (29 preceding siblings ...)
2018-10-15 11:53 ` [Qemu-devel] [PATCH v2 30/35] spice: Clean up error reporting in add_channel() Markus Armbruster
@ 2018-10-15 11:53 ` Markus Armbruster
2018-10-15 11:53 ` [Qemu-devel] [PATCH v2 32/35] vl: Assert drive_new() does not fail in default_drive() Markus Armbruster
` (3 subsequent siblings)
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Greg Kurz
Calling error_report() from within a function that takes an Error **
argument is suspicious. qemu_fsdev_add() does that, and its caller
fsdev_init_func() then fails without setting an error. Its caller
main(), via qemu_opts_foreach(), is fine with it, but clean it up
anyway.
Cc: Greg Kurz <groug@kaod.org>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Acked-by: Greg Kurz <groug@kaod.org>
---
fsdev/qemu-fsdev-dummy.c | 2 +-
fsdev/qemu-fsdev.c | 12 +++++-------
fsdev/qemu-fsdev.h | 2 +-
hw/9pfs/xen-9p-backend.c | 7 ++++++-
vl.c | 8 +++-----
5 files changed, 16 insertions(+), 15 deletions(-)
diff --git a/fsdev/qemu-fsdev-dummy.c b/fsdev/qemu-fsdev-dummy.c
index 6dc0fbc4c4..489cd29081 100644
--- a/fsdev/qemu-fsdev-dummy.c
+++ b/fsdev/qemu-fsdev-dummy.c
@@ -15,7 +15,7 @@
#include "qemu/config-file.h"
#include "qemu/module.h"
-int qemu_fsdev_add(QemuOpts *opts)
+int qemu_fsdev_add(QemuOpts *opts, Error **errp)
{
return 0;
}
diff --git a/fsdev/qemu-fsdev.c b/fsdev/qemu-fsdev.c
index 8a4afbffbd..7a3b87cc9e 100644
--- a/fsdev/qemu-fsdev.c
+++ b/fsdev/qemu-fsdev.c
@@ -30,7 +30,7 @@ static FsDriverTable FsDrivers[] = {
{ .name = "proxy", .ops = &proxy_ops},
};
-int qemu_fsdev_add(QemuOpts *opts)
+int qemu_fsdev_add(QemuOpts *opts, Error **errp)
{
int i;
struct FsDriverListEntry *fsle;
@@ -38,10 +38,9 @@ int qemu_fsdev_add(QemuOpts *opts)
const char *fsdriver = qemu_opt_get(opts, "fsdriver");
const char *writeout = qemu_opt_get(opts, "writeout");
bool ro = qemu_opt_get_bool(opts, "readonly", 0);
- Error *local_err = NULL;
if (!fsdev_id) {
- error_report("fsdev: No id specified");
+ error_setg(errp, "fsdev: No id specified");
return -1;
}
@@ -53,11 +52,11 @@ int qemu_fsdev_add(QemuOpts *opts)
}
if (i == ARRAY_SIZE(FsDrivers)) {
- error_report("fsdev: fsdriver %s not found", fsdriver);
+ error_setg(errp, "fsdev: fsdriver %s not found", fsdriver);
return -1;
}
} else {
- error_report("fsdev: No fsdriver specified");
+ error_setg(errp, "fsdev: No fsdriver specified");
return -1;
}
@@ -76,8 +75,7 @@ int qemu_fsdev_add(QemuOpts *opts)
}
if (fsle->fse.ops->parse_opts) {
- if (fsle->fse.ops->parse_opts(opts, &fsle->fse, &local_err)) {
- error_report_err(local_err);
+ if (fsle->fse.ops->parse_opts(opts, &fsle->fse, errp)) {
g_free(fsle->fse.fsdev_id);
g_free(fsle);
return -1;
diff --git a/fsdev/qemu-fsdev.h b/fsdev/qemu-fsdev.h
index 65e4b1cfab..d9716b4144 100644
--- a/fsdev/qemu-fsdev.h
+++ b/fsdev/qemu-fsdev.h
@@ -38,7 +38,7 @@ typedef struct FsDriverListEntry {
QTAILQ_ENTRY(FsDriverListEntry) next;
} FsDriverListEntry;
-int qemu_fsdev_add(QemuOpts *opts);
+int qemu_fsdev_add(QemuOpts *opts, Error **errp);
FsDriverEntry *get_fsdev_fsentry(char *id);
extern FileOperations local_ops;
extern FileOperations handle_ops;
diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index 6026780f95..3f54a21c76 100644
--- a/hw/9pfs/xen-9p-backend.c
+++ b/hw/9pfs/xen-9p-backend.c
@@ -14,6 +14,7 @@
#include "hw/9pfs/9p.h"
#include "hw/xen/xen_backend.h"
#include "hw/9pfs/xen-9pfs.h"
+#include "qapi/error.h"
#include "qemu/config-file.h"
#include "qemu/option.h"
#include "fsdev/qemu-fsdev.h"
@@ -355,6 +356,7 @@ static int xen_9pfs_free(struct XenDevice *xendev)
static int xen_9pfs_connect(struct XenDevice *xendev)
{
+ Error *err = NULL;
int i;
Xen9pfsDev *xen_9pdev = container_of(xendev, Xen9pfsDev, xendev);
V9fsState *s = &xen_9pdev->state;
@@ -452,7 +454,10 @@ static int xen_9pfs_connect(struct XenDevice *xendev)
qemu_opt_set(fsdev, "path", xen_9pdev->path, NULL);
qemu_opt_set(fsdev, "security_model", xen_9pdev->security_model, NULL);
qemu_opts_set_id(fsdev, s->fsconf.fsdev_id);
- qemu_fsdev_add(fsdev);
+ qemu_fsdev_add(fsdev, &err);
+ if (err) {
+ error_report_err(err);
+ }
v9fs_device_realize_common(s, &xen_9p_transport, NULL);
return 0;
diff --git a/vl.c b/vl.c
index abfe991ed6..2a8ef05aef 100644
--- a/vl.c
+++ b/vl.c
@@ -2249,7 +2249,7 @@ static int chardev_init_func(void *opaque, QemuOpts *opts, Error **errp)
#ifdef CONFIG_VIRTFS
static int fsdev_init_func(void *opaque, QemuOpts *opts, Error **errp)
{
- return qemu_fsdev_add(opts);
+ return qemu_fsdev_add(opts, errp);
}
#endif
@@ -4235,10 +4235,8 @@ int main(int argc, char **argv, char **envp)
chardev_init_func, NULL, &error_fatal);
#ifdef CONFIG_VIRTFS
- if (qemu_opts_foreach(qemu_find_opts("fsdev"),
- fsdev_init_func, NULL, NULL)) {
- exit(1);
- }
+ qemu_opts_foreach(qemu_find_opts("fsdev"),
+ fsdev_init_func, NULL, &error_fatal);
#endif
if (qemu_opts_foreach(qemu_find_opts("device"),
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* [Qemu-devel] [PATCH v2 32/35] vl: Assert drive_new() does not fail in default_drive()
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (30 preceding siblings ...)
2018-10-15 11:53 ` [Qemu-devel] [PATCH v2 31/35] fsdev: Clean up error reporting in qemu_fsdev_add() Markus Armbruster
@ 2018-10-15 11:53 ` Markus Armbruster
2018-10-15 11:53 ` [Qemu-devel] [PATCH v2 33/35] blockdev: Convert drive_new() to Error Markus Armbruster
` (2 subsequent siblings)
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Max Reitz
If creating (empty) default drives fails, it's a bug. Therefore,
assert() is more appropriate than exit(1).
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
vl.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/vl.c b/vl.c
index 2a8ef05aef..65366b961e 100644
--- a/vl.c
+++ b/vl.c
@@ -1156,9 +1156,7 @@ static void default_drive(int enable, int snapshot, BlockInterfaceType type,
}
dinfo = drive_new(opts, type);
- if (!dinfo) {
- exit(1);
- }
+ assert(dinfo);
dinfo->is_default = true;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* [Qemu-devel] [PATCH v2 33/35] blockdev: Convert drive_new() to Error
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (31 preceding siblings ...)
2018-10-15 11:53 ` [Qemu-devel] [PATCH v2 32/35] vl: Assert drive_new() does not fail in default_drive() Markus Armbruster
@ 2018-10-15 11:53 ` Markus Armbruster
2018-10-15 14:48 ` Max Reitz
2018-10-15 11:53 ` [Qemu-devel] [PATCH v2 34/35] vl: Fix exit status for -drive format=help Markus Armbruster
2018-10-15 11:53 ` [Qemu-devel] [PATCH v2 35/35] vl: Simplify call of parse_name() Markus Armbruster
34 siblings, 1 reply; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Max Reitz
Calling error_report() from within a function that takes an Error **
argument is suspicious. drive_new() calls error_report() even though
it can run within drive_init_func(), which takes an Error ** argument.
drive_init_func()'s caller main(), via qemu_opts_foreach(), is fine
with it, but clean it up anyway:
* Convert drive_new() to Error
* Update add_init_drive() to report the error received from
drive_new()
* Make main() pass &error_fatal through qemu_opts_foreach(),
drive_init_func() to drive_new()
* Make default_drive() pass &error_abort through qemu_opts_foreach(),
drive_init_func() to drive_new()
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
blockdev.c | 27 ++++++++++++++-------------
device-hotplug.c | 5 ++++-
include/sysemu/blockdev.h | 3 ++-
vl.c | 8 ++++----
4 files changed, 24 insertions(+), 19 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index a8755bd908..574adbcb7f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -759,7 +759,8 @@ QemuOptsList qemu_legacy_drive_opts = {
},
};
-DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
+DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type,
+ Error **errp)
{
const char *value;
BlockBackend *blk;
@@ -808,7 +809,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
qemu_opt_rename(all_opts, opt_renames[i].from, opt_renames[i].to,
&local_err);
if (local_err) {
- error_report_err(local_err);
+ error_propagate(errp, local_err);
return NULL;
}
}
@@ -819,7 +820,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
bool writethrough;
if (bdrv_parse_cache_mode(value, &flags, &writethrough) != 0) {
- error_report("invalid cache option");
+ error_setg(errp, "invalid cache option");
return NULL;
}
@@ -847,7 +848,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
&error_abort);
qemu_opts_absorb_qdict(legacy_opts, bs_opts, &local_err);
if (local_err) {
- error_report_err(local_err);
+ error_propagate(errp, local_err);
goto fail;
}
@@ -860,7 +861,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
media = MEDIA_CDROM;
read_only = true;
} else {
- error_report("'%s' invalid media", value);
+ error_setg(errp, "'%s' invalid media", value);
goto fail;
}
}
@@ -885,7 +886,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
type++) {
}
if (type == IF_COUNT) {
- error_report("unsupported bus type '%s'", value);
+ error_setg(errp, "unsupported bus type '%s'", value);
goto fail;
}
} else {
@@ -902,7 +903,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
if (index != -1) {
if (bus_id != 0 || unit_id != -1) {
- error_report("index cannot be used with bus and unit");
+ error_setg(errp, "index cannot be used with bus and unit");
goto fail;
}
bus_id = drive_index_to_bus_id(type, index);
@@ -921,13 +922,13 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
}
if (max_devs && unit_id >= max_devs) {
- error_report("unit %d too big (max is %d)", unit_id, max_devs - 1);
+ error_setg(errp, "unit %d too big (max is %d)", unit_id, max_devs - 1);
goto fail;
}
if (drive_get(type, bus_id, unit_id) != NULL) {
- error_report("drive with bus=%d, unit=%d (index=%d) exists",
- bus_id, unit_id, index);
+ error_setg(errp, "drive with bus=%d, unit=%d (index=%d) exists",
+ bus_id, unit_id, index);
goto fail;
}
@@ -970,7 +971,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
if (werror != NULL) {
if (type != IF_IDE && type != IF_SCSI && type != IF_VIRTIO &&
type != IF_NONE) {
- error_report("werror is not supported by this bus type");
+ error_setg(errp, "werror is not supported by this bus type");
goto fail;
}
qdict_put_str(bs_opts, "werror", werror);
@@ -980,7 +981,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
if (rerror != NULL) {
if (type != IF_IDE && type != IF_VIRTIO && type != IF_SCSI &&
type != IF_NONE) {
- error_report("rerror is not supported by this bus type");
+ error_setg(errp, "rerror is not supported by this bus type");
goto fail;
}
qdict_put_str(bs_opts, "rerror", rerror);
@@ -991,7 +992,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
bs_opts = NULL;
if (!blk) {
if (local_err) {
- error_report_err(local_err);
+ error_propagate(errp, local_err);
}
goto fail;
} else {
diff --git a/device-hotplug.c b/device-hotplug.c
index cd427e2c76..6090d5f1e9 100644
--- a/device-hotplug.c
+++ b/device-hotplug.c
@@ -28,6 +28,7 @@
#include "sysemu/block-backend.h"
#include "sysemu/blockdev.h"
#include "qapi/qmp/qdict.h"
+#include "qapi/error.h"
#include "qemu/config-file.h"
#include "qemu/option.h"
#include "sysemu/sysemu.h"
@@ -36,6 +37,7 @@
static DriveInfo *add_init_drive(const char *optstr)
{
+ Error *err = NULL;
DriveInfo *dinfo;
QemuOpts *opts;
MachineClass *mc;
@@ -45,8 +47,9 @@ static DriveInfo *add_init_drive(const char *optstr)
return NULL;
mc = MACHINE_GET_CLASS(current_machine);
- dinfo = drive_new(opts, mc->block_default_type);
+ dinfo = drive_new(opts, mc->block_default_type, &err);
if (!dinfo) {
+ error_report_err(err);
qemu_opts_del(opts);
return NULL;
}
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 24954b94e0..d34c4920dc 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -54,7 +54,8 @@ DriveInfo *drive_get_next(BlockInterfaceType type);
QemuOpts *drive_def(const char *optstr);
QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
const char *optstr);
-DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type);
+DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type,
+ Error **errp);
/* device-hotplug */
diff --git a/vl.c b/vl.c
index 65366b961e..22beca29d1 100644
--- a/vl.c
+++ b/vl.c
@@ -1129,7 +1129,7 @@ static int drive_init_func(void *opaque, QemuOpts *opts, Error **errp)
{
BlockInterfaceType *block_default_type = opaque;
- return drive_new(opts, *block_default_type) == NULL;
+ return drive_new(opts, *block_default_type, errp) == NULL;
}
static int drive_enable_snapshot(void *opaque, QemuOpts *opts, Error **errp)
@@ -1155,8 +1155,7 @@ static void default_drive(int enable, int snapshot, BlockInterfaceType type,
drive_enable_snapshot(NULL, opts, NULL);
}
- dinfo = drive_new(opts, type);
- assert(dinfo);
+ dinfo = drive_new(opts, type, &error_abort);
dinfo->is_default = true;
}
@@ -4396,7 +4395,8 @@ int main(int argc, char **argv, char **envp)
NULL, NULL);
}
if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
- &machine_class->block_default_type, NULL)) {
+ &machine_class->block_default_type, &error_fatal)) {
+ /* We printed help */
exit(1);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [Qemu-devel] [PATCH v2 33/35] blockdev: Convert drive_new() to Error
2018-10-15 11:53 ` [Qemu-devel] [PATCH v2 33/35] blockdev: Convert drive_new() to Error Markus Armbruster
@ 2018-10-15 14:48 ` Max Reitz
2018-10-15 18:54 ` Eric Blake
2018-10-15 22:38 ` Philippe Mathieu-Daudé
0 siblings, 2 replies; 50+ messages in thread
From: Max Reitz @ 2018-10-15 14:48 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: Kevin Wolf
[-- Attachment #1: Type: text/plain, Size: 1766 bytes --]
On 15.10.18 13:53, Markus Armbruster wrote:
> Calling error_report() from within a function that takes an Error **
> argument is suspicious. drive_new() calls error_report() even though
> it can run within drive_init_func(), which takes an Error ** argument.
> drive_init_func()'s caller main(), via qemu_opts_foreach(), is fine
> with it, but clean it up anyway:
>
> * Convert drive_new() to Error
>
> * Update add_init_drive() to report the error received from
> drive_new()
>
> * Make main() pass &error_fatal through qemu_opts_foreach(),
> drive_init_func() to drive_new()
>
> * Make default_drive() pass &error_abort through qemu_opts_foreach(),
> drive_init_func() to drive_new()
>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> blockdev.c | 27 ++++++++++++++-------------
> device-hotplug.c | 5 ++++-
> include/sysemu/blockdev.h | 3 ++-
> vl.c | 8 ++++----
> 4 files changed, 24 insertions(+), 19 deletions(-)
[...]
> diff --git a/vl.c b/vl.c
> index 65366b961e..22beca29d1 100644
> --- a/vl.c
> +++ b/vl.c
[...]
> @@ -4396,7 +4395,8 @@ int main(int argc, char **argv, char **envp)
> NULL, NULL);
> }
> if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
> - &machine_class->block_default_type, NULL)) {
> + &machine_class->block_default_type, &error_fatal)) {
> + /* We printed help */
> exit(1);
> }
I thought you wanted it to become an exit(0)? I don't care either way,
though, so:
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [Qemu-devel] [PATCH v2 33/35] blockdev: Convert drive_new() to Error
2018-10-15 14:48 ` Max Reitz
@ 2018-10-15 18:54 ` Eric Blake
2018-10-15 22:38 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 50+ messages in thread
From: Eric Blake @ 2018-10-15 18:54 UTC (permalink / raw)
To: Max Reitz, Markus Armbruster, qemu-devel; +Cc: Kevin Wolf
On 10/15/18 9:48 AM, Max Reitz wrote:
> On 15.10.18 13:53, Markus Armbruster wrote:
>> Calling error_report() from within a function that takes an Error **
>> argument is suspicious. drive_new() calls error_report() even though
>> it can run within drive_init_func(), which takes an Error ** argument.
>> drive_init_func()'s caller main(), via qemu_opts_foreach(), is fine
>> with it, but clean it up anyway:
>>
>> @@ -4396,7 +4395,8 @@ int main(int argc, char **argv, char **envp)
>> NULL, NULL);
>> }
>> if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
>> - &machine_class->block_default_type, NULL)) {
>> + &machine_class->block_default_type, &error_fatal)) {
>> + /* We printed help */
>> exit(1);
>> }
>
> I thought you wanted it to become an exit(0)? I don't care either way,
> though, so:
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
I _do_ care. Printing help isn't an error, so it shouldn't result in a
non-zero exit status.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [Qemu-devel] [PATCH v2 33/35] blockdev: Convert drive_new() to Error
2018-10-15 14:48 ` Max Reitz
2018-10-15 18:54 ` Eric Blake
@ 2018-10-15 22:38 ` Philippe Mathieu-Daudé
2018-10-16 4:09 ` Markus Armbruster
1 sibling, 1 reply; 50+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-15 22:38 UTC (permalink / raw)
To: Max Reitz, Markus Armbruster, qemu-devel; +Cc: Kevin Wolf
On 15/10/2018 16:48, Max Reitz wrote:
> On 15.10.18 13:53, Markus Armbruster wrote:
>> Calling error_report() from within a function that takes an Error **
>> argument is suspicious. drive_new() calls error_report() even though
>> it can run within drive_init_func(), which takes an Error ** argument.
>> drive_init_func()'s caller main(), via qemu_opts_foreach(), is fine
>> with it, but clean it up anyway:
>>
>> * Convert drive_new() to Error
>>
>> * Update add_init_drive() to report the error received from
>> drive_new()
>>
>> * Make main() pass &error_fatal through qemu_opts_foreach(),
>> drive_init_func() to drive_new()
>>
>> * Make default_drive() pass &error_abort through qemu_opts_foreach(),
>> drive_init_func() to drive_new()
>>
>> Cc: Kevin Wolf <kwolf@redhat.com>
>> Cc: Max Reitz <mreitz@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> blockdev.c | 27 ++++++++++++++-------------
>> device-hotplug.c | 5 ++++-
>> include/sysemu/blockdev.h | 3 ++-
>> vl.c | 8 ++++----
>> 4 files changed, 24 insertions(+), 19 deletions(-)
>
> [...]
>
>> diff --git a/vl.c b/vl.c
>> index 65366b961e..22beca29d1 100644
>> --- a/vl.c
>> +++ b/vl.c
>
> [...]
>> @@ -4396,7 +4395,8 @@ int main(int argc, char **argv, char **envp)
>> NULL, NULL);
>> }
>> if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
>> - &machine_class->block_default_type, NULL)) {
>> + &machine_class->block_default_type, &error_fatal)) {
>> + /* We printed help */
>> exit(1);
>> }
>
> I thought you wanted it to become an exit(0)? I don't care either way,
Markus did it in the next patch ;)
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> though, so:
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [Qemu-devel] [PATCH v2 33/35] blockdev: Convert drive_new() to Error
2018-10-15 22:38 ` Philippe Mathieu-Daudé
@ 2018-10-16 4:09 ` Markus Armbruster
0 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-16 4:09 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Max Reitz, qemu-devel, Kevin Wolf
Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> On 15/10/2018 16:48, Max Reitz wrote:
>> On 15.10.18 13:53, Markus Armbruster wrote:
>>> Calling error_report() from within a function that takes an Error **
>>> argument is suspicious. drive_new() calls error_report() even though
>>> it can run within drive_init_func(), which takes an Error ** argument.
>>> drive_init_func()'s caller main(), via qemu_opts_foreach(), is fine
>>> with it, but clean it up anyway:
>>>
>>> * Convert drive_new() to Error
>>>
>>> * Update add_init_drive() to report the error received from
>>> drive_new()
>>>
>>> * Make main() pass &error_fatal through qemu_opts_foreach(),
>>> drive_init_func() to drive_new()
>>>
>>> * Make default_drive() pass &error_abort through qemu_opts_foreach(),
>>> drive_init_func() to drive_new()
>>>
>>> Cc: Kevin Wolf <kwolf@redhat.com>
>>> Cc: Max Reitz <mreitz@redhat.com>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>> blockdev.c | 27 ++++++++++++++-------------
>>> device-hotplug.c | 5 ++++-
>>> include/sysemu/blockdev.h | 3 ++-
>>> vl.c | 8 ++++----
>>> 4 files changed, 24 insertions(+), 19 deletions(-)
>>
>> [...]
>>
>>> diff --git a/vl.c b/vl.c
>>> index 65366b961e..22beca29d1 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>
>> [...]
>>> @@ -4396,7 +4395,8 @@ int main(int argc, char **argv, char **envp)
>>> NULL, NULL);
>>> }
>>> if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
>>> - &machine_class->block_default_type, NULL)) {
>>> + &machine_class->block_default_type, &error_fatal)) {
>>> + /* We printed help */
>>> exit(1);
>>> }
>>
>> I thought you wanted it to become an exit(0)? I don't care either way,
>
> Markus did it in the next patch ;)
I noticed I was about to start a commit message paragraph with "Also",
and split the patch :)
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Thanks!
^ permalink raw reply [flat|nested] 50+ messages in thread
* [Qemu-devel] [PATCH v2 34/35] vl: Fix exit status for -drive format=help
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (32 preceding siblings ...)
2018-10-15 11:53 ` [Qemu-devel] [PATCH v2 33/35] blockdev: Convert drive_new() to Error Markus Armbruster
@ 2018-10-15 11:53 ` Markus Armbruster
2018-10-15 11:53 ` [Qemu-devel] [PATCH v2 35/35] vl: Simplify call of parse_name() Markus Armbruster
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:53 UTC (permalink / raw)
To: qemu-devel
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
vl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/vl.c b/vl.c
index 22beca29d1..89520d8007 100644
--- a/vl.c
+++ b/vl.c
@@ -4397,7 +4397,7 @@ int main(int argc, char **argv, char **envp)
if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
&machine_class->block_default_type, &error_fatal)) {
/* We printed help */
- exit(1);
+ exit(0);
}
default_drive(default_cdrom, snapshot, machine_class->block_default_type, 2,
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread* [Qemu-devel] [PATCH v2 35/35] vl: Simplify call of parse_name()
2018-10-15 11:52 [Qemu-devel] [PATCH v2 00/35] Replace some unwise uses of error_report() & friends Markus Armbruster
` (33 preceding siblings ...)
2018-10-15 11:53 ` [Qemu-devel] [PATCH v2 34/35] vl: Fix exit status for -drive format=help Markus Armbruster
@ 2018-10-15 11:53 ` Markus Armbruster
34 siblings, 0 replies; 50+ messages in thread
From: Markus Armbruster @ 2018-10-15 11:53 UTC (permalink / raw)
To: qemu-devel
main() checks for parse_name() failure even though it can't actually
fail. That's okay. Simplify it to check by passing &error_fatal,
like the other users of qemu_opts_foreach().
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
vl.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/vl.c b/vl.c
index 89520d8007..01d71371c2 100644
--- a/vl.c
+++ b/vl.c
@@ -3973,10 +3973,8 @@ int main(int argc, char **argv, char **envp)
}
#endif
- if (qemu_opts_foreach(qemu_find_opts("name"),
- parse_name, NULL, NULL)) {
- exit(1);
- }
+ qemu_opts_foreach(qemu_find_opts("name"),
+ parse_name, NULL, &error_fatal);
#ifndef _WIN32
qemu_opts_foreach(qemu_find_opts("add-fd"),
--
2.17.1
^ permalink raw reply related [flat|nested] 50+ messages in thread