* [Qemu-devel] [PATCH 1/9] vl: Report failure to sandbox at most once
2015-05-28 12:21 [Qemu-devel] [PATCH 0/9] Miscellaneous error reporting improvements Markus Armbruster
@ 2015-05-28 12:21 ` Markus Armbruster
2015-05-28 14:24 ` Eric Blake
2015-05-28 12:21 ` [Qemu-devel] [PATCH 2/9] vl: Print -device help " Markus Armbruster
` (8 subsequent siblings)
9 siblings, 1 reply; 31+ messages in thread
From: Markus Armbruster @ 2015-05-28 12:21 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini
It's reported once per -sandbox on. Stop on the first failure, like
we do for other options.
Not fixed: "-sandox on -sandbox off" should leave the sandbox off. It
doesn't.
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 15bccc4..07a890f 100644
--- a/vl.c
+++ b/vl.c
@@ -3786,7 +3786,7 @@ int main(int argc, char **argv, char **envp)
exit(1);
}
- if (qemu_opts_foreach(qemu_find_opts("sandbox"), parse_sandbox, NULL, 0)) {
+ if (qemu_opts_foreach(qemu_find_opts("sandbox"), parse_sandbox, NULL, 1)) {
exit(1);
}
--
1.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PATCH 2/9] vl: Print -device help at most once
2015-05-28 12:21 [Qemu-devel] [PATCH 0/9] Miscellaneous error reporting improvements Markus Armbruster
2015-05-28 12:21 ` [Qemu-devel] [PATCH 1/9] vl: Report failure to sandbox at most once Markus Armbruster
@ 2015-05-28 12:21 ` Markus Armbruster
2015-05-28 14:47 ` Eric Blake
2015-05-28 12:21 ` [Qemu-devel] [PATCH 3/9] vl: Fail right after first bad -object Markus Armbruster
` (7 subsequent siblings)
9 siblings, 1 reply; 31+ messages in thread
From: Markus Armbruster @ 2015-05-28 12:21 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini
We print it once for each -device help. Not helpful. Stop after the
first one.
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 07a890f..a31e7a5 100644
--- a/vl.c
+++ b/vl.c
@@ -4044,7 +4044,7 @@ int main(int argc, char **argv, char **envp)
exit(1);
}
- if (qemu_opts_foreach(qemu_find_opts("device"), device_help_func, NULL, 0)
+ if (qemu_opts_foreach(qemu_find_opts("device"), device_help_func, NULL, 1)
!= 0) {
exit(0);
}
--
1.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PATCH 3/9] vl: Fail right after first bad -object
2015-05-28 12:21 [Qemu-devel] [PATCH 0/9] Miscellaneous error reporting improvements Markus Armbruster
2015-05-28 12:21 ` [Qemu-devel] [PATCH 1/9] vl: Report failure to sandbox at most once Markus Armbruster
2015-05-28 12:21 ` [Qemu-devel] [PATCH 2/9] vl: Print -device help " Markus Armbruster
@ 2015-05-28 12:21 ` Markus Armbruster
2015-05-28 14:52 ` Eric Blake
2015-05-28 12:21 ` [Qemu-devel] [PATCH 4/9] QemuOpts: Drop qemu_opts_foreach() parameter abort_on_failure Markus Armbruster
` (6 subsequent siblings)
9 siblings, 1 reply; 31+ messages in thread
From: Markus Armbruster @ 2015-05-28 12:21 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini
Failure to create a -object is a fatal error. However, we delay the
actual exit until all -object are processed. On the one hand, this
permits detection of genuine additional errors. On the other hand, it
can muddy the waters with uninteresting additional errors, e.g. when a
later -object tries to reference a prior one that failed.
We generally stop right on the first bad option, so do that for
-object as well.
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 a31e7a5..1553c9e 100644
--- a/vl.c
+++ b/vl.c
@@ -4050,7 +4050,7 @@ int main(int argc, char **argv, char **envp)
}
if (qemu_opts_foreach(qemu_find_opts("object"),
- object_create, NULL, 0) != 0) {
+ object_create, NULL, 1) != 0) {
exit(1);
}
--
1.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH 3/9] vl: Fail right after first bad -object
2015-05-28 12:21 ` [Qemu-devel] [PATCH 3/9] vl: Fail right after first bad -object Markus Armbruster
@ 2015-05-28 14:52 ` Eric Blake
2015-06-02 8:41 ` Markus Armbruster
0 siblings, 1 reply; 31+ messages in thread
From: Eric Blake @ 2015-05-28 14:52 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: pbonzini
[-- Attachment #1: Type: text/plain, Size: 890 bytes --]
On 05/28/2015 06:21 AM, Markus Armbruster wrote:
> Failure to create a -object is a fatal error. However, we delay the
s/a -object/an -object/ ? (depends on whether you pronounce '-object' as
"object" or "dash-object")
> actual exit until all -object are processed. On the one hand, this
> permits detection of genuine additional errors. On the other hand, it
> can muddy the waters with uninteresting additional errors, e.g. when a
> later -object tries to reference a prior one that failed.
>
> We generally stop right on the first bad option, so do that for
> -object as well.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> vl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PATCH 3/9] vl: Fail right after first bad -object
2015-05-28 14:52 ` Eric Blake
@ 2015-06-02 8:41 ` Markus Armbruster
0 siblings, 0 replies; 31+ messages in thread
From: Markus Armbruster @ 2015-06-02 8:41 UTC (permalink / raw)
To: Eric Blake; +Cc: pbonzini, qemu-devel
Eric Blake <eblake@redhat.com> writes:
> On 05/28/2015 06:21 AM, Markus Armbruster wrote:
>> Failure to create a -object is a fatal error. However, we delay the
>
> s/a -object/an -object/ ? (depends on whether you pronounce '-object' as
> "object" or "dash-object")
I'll use "to create an object with -object".
>> actual exit until all -object are processed. On the one hand, this
>> permits detection of genuine additional errors. On the other hand, it
>> can muddy the waters with uninteresting additional errors, e.g. when a
>> later -object tries to reference a prior one that failed.
>>
>> We generally stop right on the first bad option, so do that for
>> -object as well.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> vl.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
Thanks!
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PATCH 4/9] QemuOpts: Drop qemu_opts_foreach() parameter abort_on_failure
2015-05-28 12:21 [Qemu-devel] [PATCH 0/9] Miscellaneous error reporting improvements Markus Armbruster
` (2 preceding siblings ...)
2015-05-28 12:21 ` [Qemu-devel] [PATCH 3/9] vl: Fail right after first bad -object Markus Armbruster
@ 2015-05-28 12:21 ` Markus Armbruster
2015-05-28 15:10 ` Eric Blake
2015-05-28 12:21 ` [Qemu-devel] [PATCH 5/9] QemuOpts: Convert qemu_opts_foreach() to Error Markus Armbruster
` (5 subsequent siblings)
9 siblings, 1 reply; 31+ messages in thread
From: Markus Armbruster @ 2015-05-28 12:21 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini
When the argument is non-zero, qemus_opt_foreach() stops on callback
returning non-zero, and returns that value.
When the argument is zero, it doesn't stop, and returns the bit-wise
inclusive or of all the return values. Funky :)
The callers that pass zero could just as well pass one, because their
callbacks can't return anything but zero:
* qemu_add_globals()'s callback qdev_add_one_global()
* qemu_config_write()'s callback config_write_opts()
* main()'s callbacks default_driver_check(), drive_enable_snapshot(),
vnc_init_func()
Drop the parameter, and always stop.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
block/blkdebug.c | 4 ++--
hw/core/qdev-properties-system.c | 2 +-
include/qemu/option.h | 4 ++--
net/net.c | 5 +++--
net/vhost-user.c | 2 +-
numa.c | 3 +--
tpm.c | 3 +--
util/qemu-config.c | 2 +-
util/qemu-option.c | 21 ++++++++++++++-------
vl.c | 35 ++++++++++++++++++-----------------
10 files changed, 44 insertions(+), 37 deletions(-)
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 3c30edb..58f5105 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -320,7 +320,7 @@ static int read_config(BDRVBlkdebugState *s, const char *filename,
d.s = s;
d.action = ACTION_INJECT_ERROR;
d.errp = &local_err;
- qemu_opts_foreach(&inject_error_opts, add_rule, &d, 1);
+ qemu_opts_foreach(&inject_error_opts, add_rule, &d);
if (local_err) {
error_propagate(errp, local_err);
ret = -EINVAL;
@@ -328,7 +328,7 @@ static int read_config(BDRVBlkdebugState *s, const char *filename,
}
d.action = ACTION_SET_STATE;
- qemu_opts_foreach(&set_state_opts, add_rule, &d, 1);
+ qemu_opts_foreach(&set_state_opts, add_rule, &d);
if (local_err) {
error_propagate(errp, local_err);
ret = -EINVAL;
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index c413226..93daeb0 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -404,5 +404,5 @@ static int qdev_add_one_global(QemuOpts *opts, void *opaque)
void qemu_add_globals(void)
{
- qemu_opts_foreach(qemu_find_opts("global"), qdev_add_one_global, NULL, 0);
+ qemu_opts_foreach(qemu_find_opts("global"), qdev_add_one_global, NULL);
}
diff --git a/include/qemu/option.h b/include/qemu/option.h
index f88b545..2edf58f 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -126,9 +126,9 @@ QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict);
void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp);
typedef int (*qemu_opts_loopfunc)(QemuOpts *opts, void *opaque);
+int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func,
+ void *opaque);
void qemu_opts_print(QemuOpts *opts, const char *sep);
-int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque,
- int abort_on_failure);
void qemu_opts_print_help(QemuOptsList *list);
void qemu_opts_free(QemuOptsList *list);
QemuOptsList *qemu_opts_append(QemuOptsList *dst, QemuOptsList *list);
diff --git a/net/net.c b/net/net.c
index db6be12..011de59 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1373,10 +1373,11 @@ int net_init_clients(void)
QTAILQ_INIT(&net_clients);
- if (qemu_opts_foreach(qemu_find_opts("netdev"), net_init_netdev, NULL, 1) == -1)
+ if (qemu_opts_foreach(qemu_find_opts("netdev"), net_init_netdev, NULL)) {
return -1;
+ }
- if (qemu_opts_foreach(net, net_init_client, NULL, 1) == -1) {
+ if (qemu_opts_foreach(net, net_init_client, NULL)) {
return -1;
}
diff --git a/net/vhost-user.c b/net/vhost-user.c
index 11899c5..3d127e7 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -240,7 +240,7 @@ int net_init_vhost_user(const NetClientOptions *opts, const char *name,
/* verify net frontend */
if (qemu_opts_foreach(qemu_find_opts("device"), net_vhost_check_net,
- (char *)name, true) == -1) {
+ (char *)name)) {
return -1;
}
diff --git a/numa.c b/numa.c
index c975fb2..360a595 100644
--- a/numa.c
+++ b/numa.c
@@ -216,8 +216,7 @@ void parse_numa_opts(MachineClass *mc)
{
int i;
- if (qemu_opts_foreach(qemu_find_opts("numa"), parse_numa,
- NULL, 1) != 0) {
+ if (qemu_opts_foreach(qemu_find_opts("numa"), parse_numa, NULL)) {
exit(1);
}
diff --git a/tpm.c b/tpm.c
index 963b7ee..bca3b3a 100644
--- a/tpm.c
+++ b/tpm.c
@@ -207,8 +207,7 @@ void tpm_cleanup(void)
*/
int tpm_init(void)
{
- if (qemu_opts_foreach(qemu_find_opts("tpmdev"),
- tpm_init_tpmdev, NULL, 1) != 0) {
+ if (qemu_opts_foreach(qemu_find_opts("tpmdev"), tpm_init_tpmdev, NULL)) {
return -1;
}
diff --git a/util/qemu-config.c b/util/qemu-config.c
index 30d6dcf..b38927a 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -367,7 +367,7 @@ void qemu_config_write(FILE *fp)
fprintf(fp, "# qemu config file\n\n");
for (i = 0; lists[i] != NULL; i++) {
data.list = lists[i];
- qemu_opts_foreach(data.list, config_write_opts, &data, 0);
+ qemu_opts_foreach(data.list, config_write_opts, &data);
}
}
diff --git a/util/qemu-option.c b/util/qemu-option.c
index fda4e5f..7672aae 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -1046,22 +1046,29 @@ void qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc, Error **errp)
}
}
-int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque,
- int abort_on_failure)
+/**
+ * For each member of @list, call @func(member, @opaque).
+ * Call it with the current location temporarily set to the member's.
+ * When @func() returns non-zero, break the loop and return that value.
+ * Return zero when the loop completes.
+ */
+int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func,
+ void *opaque)
{
Location loc;
QemuOpts *opts;
- int rc = 0;
+ int rc;
loc_push_none(&loc);
QTAILQ_FOREACH(opts, &list->head, next) {
loc_restore(&opts->loc);
- rc |= func(opts, opaque);
- if (abort_on_failure && rc != 0)
- break;
+ rc = func(opts, opaque);
+ if (rc) {
+ return rc;
+ }
}
loc_pop(&loc);
- return rc;
+ return 0;
}
static size_t count_opts_list(QemuOptsList *list)
diff --git a/vl.c b/vl.c
index 1553c9e..dccccbc 100644
--- a/vl.c
+++ b/vl.c
@@ -3786,20 +3786,20 @@ int main(int argc, char **argv, char **envp)
exit(1);
}
- if (qemu_opts_foreach(qemu_find_opts("sandbox"), parse_sandbox, NULL, 1)) {
+ if (qemu_opts_foreach(qemu_find_opts("sandbox"), parse_sandbox, NULL)) {
exit(1);
}
- if (qemu_opts_foreach(qemu_find_opts("name"), parse_name, NULL, 1)) {
+ if (qemu_opts_foreach(qemu_find_opts("name"), parse_name, NULL)) {
exit(1);
}
#ifndef _WIN32
- if (qemu_opts_foreach(qemu_find_opts("add-fd"), parse_add_fd, NULL, 1)) {
+ if (qemu_opts_foreach(qemu_find_opts("add-fd"), parse_add_fd, NULL)) {
exit(1);
}
- if (qemu_opts_foreach(qemu_find_opts("add-fd"), cleanup_add_fd, NULL, 1)) {
+ if (qemu_opts_foreach(qemu_find_opts("add-fd"), cleanup_add_fd, NULL)) {
exit(1);
}
#endif
@@ -3892,8 +3892,8 @@ int main(int argc, char **argv, char **envp)
machine_class->default_machine_opts, 0);
}
- qemu_opts_foreach(qemu_find_opts("device"), default_driver_check, NULL, 0);
- qemu_opts_foreach(qemu_find_opts("global"), default_driver_check, NULL, 0);
+ qemu_opts_foreach(qemu_find_opts("device"), default_driver_check, NULL);
+ qemu_opts_foreach(qemu_find_opts("global"), default_driver_check, NULL);
if (!vga_model && !default_vga) {
vga_interface_type = VGA_DEVICE;
@@ -4031,10 +4031,12 @@ int main(int argc, char **argv, char **envp)
socket_init();
- if (qemu_opts_foreach(qemu_find_opts("chardev"), chardev_init_func, NULL, 1) != 0)
+ if (qemu_opts_foreach(qemu_find_opts("chardev"), chardev_init_func, NULL)) {
exit(1);
+ }
+
#ifdef CONFIG_VIRTFS
- if (qemu_opts_foreach(qemu_find_opts("fsdev"), fsdev_init_func, NULL, 1) != 0) {
+ if (qemu_opts_foreach(qemu_find_opts("fsdev"), fsdev_init_func, NULL)) {
exit(1);
}
#endif
@@ -4044,13 +4046,11 @@ int main(int argc, char **argv, char **envp)
exit(1);
}
- if (qemu_opts_foreach(qemu_find_opts("device"), device_help_func, NULL, 1)
- != 0) {
+ if (qemu_opts_foreach(qemu_find_opts("device"), device_help_func, NULL)) {
exit(0);
}
- if (qemu_opts_foreach(qemu_find_opts("object"),
- object_create, NULL, 1) != 0) {
+ if (qemu_opts_foreach(qemu_find_opts("object"), object_create, NULL)) {
exit(1);
}
@@ -4184,9 +4184,9 @@ int main(int argc, char **argv, char **envp)
/* open the virtual block devices */
if (snapshot)
- qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot, NULL, 0);
+ qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot, NULL);
if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
- &machine_class->block_default_type, 1) != 0) {
+ &machine_class->block_default_type)) {
exit(1);
}
@@ -4197,7 +4197,7 @@ int main(int argc, char **argv, char **envp)
parse_numa_opts(machine_class);
- if (qemu_opts_foreach(qemu_find_opts("mon"), mon_init_func, NULL, 1) != 0) {
+ if (qemu_opts_foreach(qemu_find_opts("mon"), mon_init_func, NULL)) {
exit(1);
}
@@ -4263,8 +4263,9 @@ int main(int argc, char **argv, char **envp)
}
/* init generic devices */
- if (qemu_opts_foreach(qemu_find_opts("device"), device_init_func, NULL, 1) != 0)
+ if (qemu_opts_foreach(qemu_find_opts("device"), device_init_func, NULL)) {
exit(1);
+ }
/* Did we create any drives that we failed to create a device for? */
drive_check_orphaned();
@@ -4316,7 +4317,7 @@ int main(int argc, char **argv, char **envp)
#ifdef CONFIG_VNC
/* init remote displays */
- qemu_opts_foreach(qemu_find_opts("vnc"), vnc_init_func, NULL, 0);
+ qemu_opts_foreach(qemu_find_opts("vnc"), vnc_init_func, NULL);
if (show_vnc_port) {
printf("VNC server running on `%s'\n",
vnc_display_local_addr("default"));
--
1.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH 4/9] QemuOpts: Drop qemu_opts_foreach() parameter abort_on_failure
2015-05-28 12:21 ` [Qemu-devel] [PATCH 4/9] QemuOpts: Drop qemu_opts_foreach() parameter abort_on_failure Markus Armbruster
@ 2015-05-28 15:10 ` Eric Blake
2015-06-02 8:42 ` Markus Armbruster
0 siblings, 1 reply; 31+ messages in thread
From: Eric Blake @ 2015-05-28 15:10 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: pbonzini
[-- Attachment #1: Type: text/plain, Size: 2081 bytes --]
On 05/28/2015 06:21 AM, Markus Armbruster wrote:
> When the argument is non-zero, qemus_opt_foreach() stops on callback
s/qemus_opt/qemu_opts/
> returning non-zero, and returns that value.
>
> When the argument is zero, it doesn't stop, and returns the bit-wise
> inclusive or of all the return values. Funky :)
>
> The callers that pass zero could just as well pass one, because their
> callbacks can't return anything but zero:
>
> * qemu_add_globals()'s callback qdev_add_one_global()
>
> * qemu_config_write()'s callback config_write_opts()
>
> * main()'s callbacks default_driver_check(), drive_enable_snapshot(),
> vnc_init_func()
>
> Drop the parameter, and always stop.
I had a hunch you might be getting to this point, just from reading the
earlier patches :)
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> block/blkdebug.c | 4 ++--
> hw/core/qdev-properties-system.c | 2 +-
> include/qemu/option.h | 4 ++--
> net/net.c | 5 +++--
> net/vhost-user.c | 2 +-
> numa.c | 3 +--
> tpm.c | 3 +--
> util/qemu-config.c | 2 +-
> util/qemu-option.c | 21 ++++++++++++++-------
> vl.c | 35 ++++++++++++++++++-----------------
> 10 files changed, 44 insertions(+), 37 deletions(-)
>
> +++ b/net/vhost-user.c
> @@ -240,7 +240,7 @@ int net_init_vhost_user(const NetClientOptions *opts, const char *name,
>
> /* verify net frontend */
> if (qemu_opts_foreach(qemu_find_opts("device"), net_vhost_check_net,
> - (char *)name, true) == -1) {
> + (char *)name)) {
Also evidence that we weren't clear on 'int' vs. 'bool' on the
parameter's usage, so getting rid of it is indeed an improvement.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH 4/9] QemuOpts: Drop qemu_opts_foreach() parameter abort_on_failure
2015-05-28 15:10 ` Eric Blake
@ 2015-06-02 8:42 ` Markus Armbruster
0 siblings, 0 replies; 31+ messages in thread
From: Markus Armbruster @ 2015-06-02 8:42 UTC (permalink / raw)
To: Eric Blake; +Cc: pbonzini, qemu-devel
Eric Blake <eblake@redhat.com> writes:
> On 05/28/2015 06:21 AM, Markus Armbruster wrote:
>> When the argument is non-zero, qemus_opt_foreach() stops on callback
>
> s/qemus_opt/qemu_opts/
Wonder what got my fingers into *that* tangle. Fixing...
>> returning non-zero, and returns that value.
>>
>> When the argument is zero, it doesn't stop, and returns the bit-wise
>> inclusive or of all the return values. Funky :)
>>
>> The callers that pass zero could just as well pass one, because their
>> callbacks can't return anything but zero:
>>
>> * qemu_add_globals()'s callback qdev_add_one_global()
>>
>> * qemu_config_write()'s callback config_write_opts()
>>
>> * main()'s callbacks default_driver_check(), drive_enable_snapshot(),
>> vnc_init_func()
>>
>> Drop the parameter, and always stop.
>
> I had a hunch you might be getting to this point, just from reading the
> earlier patches :)
I take that as a good sign :)
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> block/blkdebug.c | 4 ++--
>> hw/core/qdev-properties-system.c | 2 +-
>> include/qemu/option.h | 4 ++--
>> net/net.c | 5 +++--
>> net/vhost-user.c | 2 +-
>> numa.c | 3 +--
>> tpm.c | 3 +--
>> util/qemu-config.c | 2 +-
>> util/qemu-option.c | 21 ++++++++++++++-------
>> vl.c | 35 ++++++++++++++++++-----------------
>> 10 files changed, 44 insertions(+), 37 deletions(-)
>>
>
>> +++ b/net/vhost-user.c
>> @@ -240,7 +240,7 @@ int net_init_vhost_user(const NetClientOptions *opts, const char *name,
>>
>> /* verify net frontend */
>> if (qemu_opts_foreach(qemu_find_opts("device"), net_vhost_check_net,
>> - (char *)name, true) == -1) {
>> + (char *)name)) {
>
> Also evidence that we weren't clear on 'int' vs. 'bool' on the
> parameter's usage, so getting rid of it is indeed an improvement.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
Thanks!
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PATCH 5/9] QemuOpts: Convert qemu_opts_foreach() to Error
2015-05-28 12:21 [Qemu-devel] [PATCH 0/9] Miscellaneous error reporting improvements Markus Armbruster
` (3 preceding siblings ...)
2015-05-28 12:21 ` [Qemu-devel] [PATCH 4/9] QemuOpts: Drop qemu_opts_foreach() parameter abort_on_failure Markus Armbruster
@ 2015-05-28 12:21 ` Markus Armbruster
2015-05-28 16:18 ` Eric Blake
2015-05-28 12:21 ` [Qemu-devel] [PATCH 6/9] blkdebug: Simplify passing of Error through qemu_opts_foreach() Markus Armbruster
` (4 subsequent siblings)
9 siblings, 1 reply; 31+ messages in thread
From: Markus Armbruster @ 2015-05-28 12:21 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini
Retain the function value for now, to permit selective conversion of
its callers.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
block/blkdebug.c | 6 ++--
hw/core/qdev-properties-system.c | 5 +--
include/qemu/option.h | 4 +--
include/ui/console.h | 2 +-
net/net.c | 9 ++---
net/vhost-user.c | 4 +--
numa.c | 5 +--
tpm.c | 6 ++--
ui/vnc.c | 2 +-
util/qemu-config.c | 4 +--
util/qemu-option.c | 7 ++--
vl.c | 72 ++++++++++++++++++++++++----------------
12 files changed, 72 insertions(+), 54 deletions(-)
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 58f5105..50ef1fc 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -219,7 +219,7 @@ struct add_rule_data {
Error **errp;
};
-static int add_rule(QemuOpts *opts, void *opaque)
+static int add_rule(void *opaque, QemuOpts *opts, Error **errp)
{
struct add_rule_data *d = opaque;
BDRVBlkdebugState *s = d->s;
@@ -320,7 +320,7 @@ static int read_config(BDRVBlkdebugState *s, const char *filename,
d.s = s;
d.action = ACTION_INJECT_ERROR;
d.errp = &local_err;
- qemu_opts_foreach(&inject_error_opts, add_rule, &d);
+ qemu_opts_foreach(&inject_error_opts, add_rule, &d, &error_abort);
if (local_err) {
error_propagate(errp, local_err);
ret = -EINVAL;
@@ -328,7 +328,7 @@ static int read_config(BDRVBlkdebugState *s, const char *filename,
}
d.action = ACTION_SET_STATE;
- qemu_opts_foreach(&set_state_opts, add_rule, &d);
+ qemu_opts_foreach(&set_state_opts, add_rule, &d, &error_abort);
if (local_err) {
error_propagate(errp, local_err);
ret = -EINVAL;
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 93daeb0..8be85bb 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -389,7 +389,7 @@ void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd)
nd->instantiated = 1;
}
-static int qdev_add_one_global(QemuOpts *opts, void *opaque)
+static int qdev_add_one_global(void *opaque, QemuOpts *opts, Error **errp)
{
GlobalProperty *g;
@@ -404,5 +404,6 @@ static int qdev_add_one_global(QemuOpts *opts, void *opaque)
void qemu_add_globals(void)
{
- qemu_opts_foreach(qemu_find_opts("global"), qdev_add_one_global, NULL);
+ qemu_opts_foreach(qemu_find_opts("global"),
+ qdev_add_one_global, NULL, &error_abort);
}
diff --git a/include/qemu/option.h b/include/qemu/option.h
index 2edf58f..a3850b2 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -125,9 +125,9 @@ QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict);
void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp);
-typedef int (*qemu_opts_loopfunc)(QemuOpts *opts, void *opaque);
+typedef int (*qemu_opts_loopfunc)(void *opaque, QemuOpts *opts, Error **errp);
int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func,
- void *opaque);
+ void *opaque, Error **errp);
void qemu_opts_print(QemuOpts *opts, const char *sep);
void qemu_opts_print_help(QemuOptsList *list);
void qemu_opts_free(QemuOptsList *list);
diff --git a/include/ui/console.h b/include/ui/console.h
index e8b3a9e..fd889c7 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -375,7 +375,7 @@ char *vnc_display_local_addr(const char *id);
int vnc_display_password(const char *id, const char *password);
int vnc_display_pw_expire(const char *id, time_t expires);
QemuOpts *vnc_parse_func(const char *str);
-int vnc_init_func(QemuOpts *opts, void *opaque);
+int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp);
#else
static inline int vnc_display_password(const char *id, const char *password)
{
diff --git a/net/net.c b/net/net.c
index 011de59..90398ec 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1329,7 +1329,7 @@ void net_check_clients(void)
}
}
-static int net_init_client(QemuOpts *opts, void *dummy)
+static int net_init_client(void *dummy, QemuOpts *opts, Error **errp)
{
Error *local_err = NULL;
@@ -1342,7 +1342,7 @@ static int net_init_client(QemuOpts *opts, void *dummy)
return 0;
}
-static int net_init_netdev(QemuOpts *opts, void *dummy)
+static int net_init_netdev(void *dummy, QemuOpts *opts, Error **errp)
{
Error *local_err = NULL;
int ret;
@@ -1373,11 +1373,12 @@ int net_init_clients(void)
QTAILQ_INIT(&net_clients);
- if (qemu_opts_foreach(qemu_find_opts("netdev"), net_init_netdev, NULL)) {
+ if (qemu_opts_foreach(qemu_find_opts("netdev"),
+ net_init_netdev, NULL, &error_abort)) {
return -1;
}
- if (qemu_opts_foreach(net, net_init_client, NULL)) {
+ if (qemu_opts_foreach(net, net_init_client, NULL, &error_abort)) {
return -1;
}
diff --git a/net/vhost-user.c b/net/vhost-user.c
index 3d127e7..545f203 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -200,7 +200,7 @@ static CharDriverState *net_vhost_parse_chardev(const NetdevVhostUserOptions *op
return chr;
}
-static int net_vhost_check_net(QemuOpts *opts, void *opaque)
+static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp)
{
const char *name = opaque;
const char *driver, *netdev;
@@ -240,7 +240,7 @@ int net_init_vhost_user(const NetClientOptions *opts, const char *name,
/* verify net frontend */
if (qemu_opts_foreach(qemu_find_opts("device"), net_vhost_check_net,
- (char *)name)) {
+ (char *)name, &error_abort)) {
return -1;
}
diff --git a/numa.c b/numa.c
index 360a595..bd35509 100644
--- a/numa.c
+++ b/numa.c
@@ -125,7 +125,7 @@ static void numa_node_parse(NumaNodeOptions *node, QemuOpts *opts, Error **errp)
max_numa_nodeid = MAX(max_numa_nodeid, nodenr + 1);
}
-static int parse_numa(QemuOpts *opts, void *opaque)
+static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
{
NumaOptions *object = NULL;
Error *err = NULL;
@@ -216,7 +216,8 @@ void parse_numa_opts(MachineClass *mc)
{
int i;
- if (qemu_opts_foreach(qemu_find_opts("numa"), parse_numa, NULL)) {
+ if (qemu_opts_foreach(qemu_find_opts("numa"),
+ parse_numa, NULL, &error_abort)) {
exit(1);
}
diff --git a/tpm.c b/tpm.c
index bca3b3a..87f5405 100644
--- a/tpm.c
+++ b/tpm.c
@@ -182,7 +182,7 @@ static int configure_tpm(QemuOpts *opts)
return 0;
}
-static int tpm_init_tpmdev(QemuOpts *opts, void *dummy)
+static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, Error **errp)
{
return configure_tpm(opts);
}
@@ -207,12 +207,12 @@ void tpm_cleanup(void)
*/
int tpm_init(void)
{
- if (qemu_opts_foreach(qemu_find_opts("tpmdev"), tpm_init_tpmdev, NULL)) {
+ if (qemu_opts_foreach(qemu_find_opts("tpmdev"),
+ tpm_init_tpmdev, NULL, &error_abort)) {
return -1;
}
atexit(tpm_cleanup);
-
return 0;
}
diff --git a/ui/vnc.c b/ui/vnc.c
index 1013ea5..0c6b5e3 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3770,7 +3770,7 @@ QemuOpts *vnc_parse_func(const char *str)
return opts;
}
-int vnc_init_func(QemuOpts *opts, void *opaque)
+int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
{
Error *local_err = NULL;
char *id = (char *)qemu_opts_id(opts);
diff --git a/util/qemu-config.c b/util/qemu-config.c
index b38927a..281c47d 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -343,7 +343,7 @@ static int config_write_opt(const char *name, const char *value, void *opaque)
return 0;
}
-static int config_write_opts(QemuOpts *opts, void *opaque)
+static int config_write_opts(void *opaque, QemuOpts *opts, Error **errp)
{
struct ConfigWriteData *data = opaque;
const char *id = qemu_opts_id(opts);
@@ -367,7 +367,7 @@ void qemu_config_write(FILE *fp)
fprintf(fp, "# qemu config file\n\n");
for (i = 0; lists[i] != NULL; i++) {
data.list = lists[i];
- qemu_opts_foreach(data.list, config_write_opts, &data);
+ qemu_opts_foreach(data.list, config_write_opts, &data, &error_abort);
}
}
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 7672aae..b56aac8 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -1047,13 +1047,14 @@ void qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc, Error **errp)
}
/**
- * For each member of @list, call @func(member, @opaque).
+ * For each member of @list, call @func(@opaque, member, @errp).
* Call it with the current location temporarily set to the member's.
+ * @func() may store an Error through @errp, but must return non-zero then.
* When @func() returns non-zero, break the loop and return that value.
* Return zero when the loop completes.
*/
int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func,
- void *opaque)
+ void *opaque, Error **errp)
{
Location loc;
QemuOpts *opts;
@@ -1062,7 +1063,7 @@ int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func,
loc_push_none(&loc);
QTAILQ_FOREACH(opts, &list->head, next) {
loc_restore(&opts->loc);
- rc = func(opts, opaque);
+ rc = func(opaque, opts, errp);
if (rc) {
return rc;
}
diff --git a/vl.c b/vl.c
index dccccbc..84a19a9 100644
--- a/vl.c
+++ b/vl.c
@@ -514,7 +514,7 @@ static void res_free(void)
}
}
-static int default_driver_check(QemuOpts *opts, void *opaque)
+static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
{
const char *driver = qemu_opt_get(opts, "driver");
int i;
@@ -960,7 +960,7 @@ static int bt_parse(const char *opt)
return 1;
}
-static int parse_sandbox(QemuOpts *opts, void *opaque)
+static int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp)
{
/* FIXME: change this to true for 1.3 */
if (qemu_opt_get_bool(opts, "enable", false)) {
@@ -980,7 +980,7 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
return 0;
}
-static int parse_name(QemuOpts *opts, void *opaque)
+static int parse_name(void *opaque, QemuOpts *opts, Error **errp)
{
const char *proc_name;
@@ -1008,7 +1008,7 @@ bool usb_enabled(void)
}
#ifndef _WIN32
-static int parse_add_fd(QemuOpts *opts, void *opaque)
+static int parse_add_fd(void *opaque, QemuOpts *opts, Error **errp)
{
int fd, dupfd, flags;
int64_t fdset_id;
@@ -1070,7 +1070,7 @@ static int parse_add_fd(QemuOpts *opts, void *opaque)
return 0;
}
-static int cleanup_add_fd(QemuOpts *opts, void *opaque)
+static int cleanup_add_fd(void *opaque, QemuOpts *opts, Error **errp)
{
int fd;
@@ -1091,14 +1091,14 @@ static int cleanup_add_fd(QemuOpts *opts, void *opaque)
#define MTD_OPTS ""
#define SD_OPTS ""
-static int drive_init_func(QemuOpts *opts, void *opaque)
+static int drive_init_func(void *opaque, QemuOpts *opts, Error **errp)
{
BlockInterfaceType *block_default_type = opaque;
return drive_new(opts, *block_default_type) == NULL;
}
-static int drive_enable_snapshot(QemuOpts *opts, void *opaque)
+static int drive_enable_snapshot(void *opaque, QemuOpts *opts, Error **errp)
{
if (qemu_opt_get(opts, "snapshot") == NULL) {
qemu_opt_set(opts, "snapshot", "on", &error_abort);
@@ -1118,7 +1118,7 @@ static void default_drive(int enable, int snapshot, BlockInterfaceType type,
opts = drive_add(type, index, NULL, optstr);
if (snapshot) {
- drive_enable_snapshot(opts, NULL);
+ drive_enable_snapshot(NULL, opts, &error_abort);
}
dinfo = drive_new(opts, type);
@@ -2128,12 +2128,12 @@ char *qemu_find_file(int type, const char *name)
return NULL;
}
-static int device_help_func(QemuOpts *opts, void *opaque)
+static int device_help_func(void *opaque, QemuOpts *opts, Error **errp)
{
return qdev_device_help(opts);
}
-static int device_init_func(QemuOpts *opts, void *opaque)
+static int device_init_func(void *opaque, QemuOpts *opts, Error **errp)
{
DeviceState *dev;
@@ -2144,7 +2144,7 @@ static int device_init_func(QemuOpts *opts, void *opaque)
return 0;
}
-static int chardev_init_func(QemuOpts *opts, void *opaque)
+static int chardev_init_func(void *opaque, QemuOpts *opts, Error **errp)
{
Error *local_err = NULL;
@@ -2157,7 +2157,7 @@ static int chardev_init_func(QemuOpts *opts, void *opaque)
}
#ifdef CONFIG_VIRTFS
-static int fsdev_init_func(QemuOpts *opts, void *opaque)
+static int fsdev_init_func(void *opaque, QemuOpts *opts, Error **errp)
{
int ret;
ret = qemu_fsdev_add(opts);
@@ -2166,7 +2166,7 @@ static int fsdev_init_func(QemuOpts *opts, void *opaque)
}
#endif
-static int mon_init_func(QemuOpts *opts, void *opaque)
+static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp)
{
CharDriverState *chr;
const char *chardev;
@@ -2601,7 +2601,7 @@ static int machine_set_property(const char *name, const char *value,
return 0;
}
-static int object_create(QemuOpts *opts, void *opaque)
+static int object_create(void *opaque, QemuOpts *opts, Error **errp)
{
Error *err = NULL;
char *type = NULL;
@@ -3786,20 +3786,24 @@ int main(int argc, char **argv, char **envp)
exit(1);
}
- if (qemu_opts_foreach(qemu_find_opts("sandbox"), parse_sandbox, NULL)) {
+ if (qemu_opts_foreach(qemu_find_opts("sandbox"),
+ parse_sandbox, NULL, &error_abort)) {
exit(1);
}
- if (qemu_opts_foreach(qemu_find_opts("name"), parse_name, NULL)) {
+ if (qemu_opts_foreach(qemu_find_opts("name"),
+ parse_name, NULL, &error_abort)) {
exit(1);
}
#ifndef _WIN32
- if (qemu_opts_foreach(qemu_find_opts("add-fd"), parse_add_fd, NULL)) {
+ if (qemu_opts_foreach(qemu_find_opts("add-fd"),
+ parse_add_fd, NULL, &error_abort)) {
exit(1);
}
- if (qemu_opts_foreach(qemu_find_opts("add-fd"), cleanup_add_fd, NULL)) {
+ if (qemu_opts_foreach(qemu_find_opts("add-fd"),
+ cleanup_add_fd, NULL, &error_abort)) {
exit(1);
}
#endif
@@ -3892,8 +3896,10 @@ int main(int argc, char **argv, char **envp)
machine_class->default_machine_opts, 0);
}
- qemu_opts_foreach(qemu_find_opts("device"), default_driver_check, NULL);
- qemu_opts_foreach(qemu_find_opts("global"), default_driver_check, NULL);
+ qemu_opts_foreach(qemu_find_opts("device"),
+ default_driver_check, NULL, &error_abort);
+ qemu_opts_foreach(qemu_find_opts("global"),
+ default_driver_check, NULL, &error_abort);
if (!vga_model && !default_vga) {
vga_interface_type = VGA_DEVICE;
@@ -4031,12 +4037,14 @@ int main(int argc, char **argv, char **envp)
socket_init();
- if (qemu_opts_foreach(qemu_find_opts("chardev"), chardev_init_func, NULL)) {
+ if (qemu_opts_foreach(qemu_find_opts("chardev"),
+ chardev_init_func, NULL, &error_abort)) {
exit(1);
}
#ifdef CONFIG_VIRTFS
- if (qemu_opts_foreach(qemu_find_opts("fsdev"), fsdev_init_func, NULL)) {
+ if (qemu_opts_foreach(qemu_find_opts("fsdev"),
+ fsdev_init_func, NULL, &error_abort)) {
exit(1);
}
#endif
@@ -4046,11 +4054,13 @@ int main(int argc, char **argv, char **envp)
exit(1);
}
- if (qemu_opts_foreach(qemu_find_opts("device"), device_help_func, NULL)) {
+ if (qemu_opts_foreach(qemu_find_opts("device"),
+ device_help_func, NULL, &error_abort)) {
exit(0);
}
- if (qemu_opts_foreach(qemu_find_opts("object"), object_create, NULL)) {
+ if (qemu_opts_foreach(qemu_find_opts("object"),
+ object_create, NULL, &error_abort)) {
exit(1);
}
@@ -4184,9 +4194,10 @@ int main(int argc, char **argv, char **envp)
/* open the virtual block devices */
if (snapshot)
- qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot, NULL);
+ qemu_opts_foreach(qemu_find_opts("drive"),
+ drive_enable_snapshot, NULL, &error_abort);
if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
- &machine_class->block_default_type)) {
+ &machine_class->block_default_type, &error_abort)) {
exit(1);
}
@@ -4197,7 +4208,8 @@ int main(int argc, char **argv, char **envp)
parse_numa_opts(machine_class);
- if (qemu_opts_foreach(qemu_find_opts("mon"), mon_init_func, NULL)) {
+ if (qemu_opts_foreach(qemu_find_opts("mon"),
+ mon_init_func, NULL, &error_abort)) {
exit(1);
}
@@ -4263,7 +4275,8 @@ int main(int argc, char **argv, char **envp)
}
/* init generic devices */
- if (qemu_opts_foreach(qemu_find_opts("device"), device_init_func, NULL)) {
+ if (qemu_opts_foreach(qemu_find_opts("device"),
+ device_init_func, NULL, &error_abort)) {
exit(1);
}
@@ -4317,7 +4330,8 @@ int main(int argc, char **argv, char **envp)
#ifdef CONFIG_VNC
/* init remote displays */
- qemu_opts_foreach(qemu_find_opts("vnc"), vnc_init_func, NULL);
+ qemu_opts_foreach(qemu_find_opts("vnc"),
+ vnc_init_func, NULL, &error_abort);
if (show_vnc_port) {
printf("VNC server running on `%s'\n",
vnc_display_local_addr("default"));
--
1.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH 5/9] QemuOpts: Convert qemu_opts_foreach() to Error
2015-05-28 12:21 ` [Qemu-devel] [PATCH 5/9] QemuOpts: Convert qemu_opts_foreach() to Error Markus Armbruster
@ 2015-05-28 16:18 ` Eric Blake
2015-06-02 11:33 ` Markus Armbruster
0 siblings, 1 reply; 31+ messages in thread
From: Eric Blake @ 2015-05-28 16:18 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: pbonzini
[-- Attachment #1: Type: text/plain, Size: 3313 bytes --]
On 05/28/2015 06:21 AM, Markus Armbruster wrote:
> Retain the function value for now, to permit selective conversion of
> its callers.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> block/blkdebug.c | 6 ++--
> hw/core/qdev-properties-system.c | 5 +--
> include/qemu/option.h | 4 +--
> include/ui/console.h | 2 +-
> net/net.c | 9 ++---
> net/vhost-user.c | 4 +--
> numa.c | 5 +--
> tpm.c | 6 ++--
> ui/vnc.c | 2 +-
> util/qemu-config.c | 4 +--
> util/qemu-option.c | 7 ++--
> vl.c | 72 ++++++++++++++++++++++++----------------
> 12 files changed, 72 insertions(+), 54 deletions(-)
>
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 58f5105..50ef1fc 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -219,7 +219,7 @@ struct add_rule_data {
> Error **errp;
> };
>
> -static int add_rule(QemuOpts *opts, void *opaque)
> +static int add_rule(void *opaque, QemuOpts *opts, Error **errp)
> +++ b/include/qemu/option.h
> @@ -125,9 +125,9 @@ QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
> QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict);
> void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp);
>
> -typedef int (*qemu_opts_loopfunc)(QemuOpts *opts, void *opaque);
> +typedef int (*qemu_opts_loopfunc)(void *opaque, QemuOpts *opts, Error **errp);
Might be nice to justify in the commit message why swapping the
arguments of the callback made sense. But doesn't affect code
correctness, so:
Reviewed-by: Eric Blake <eblake@redhat.com>
> +++ b/util/qemu-option.c
> @@ -1047,13 +1047,14 @@ void qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc, Error **errp)
> }
>
> /**
> - * For each member of @list, call @func(member, @opaque).
> + * For each member of @list, call @func(@opaque, member, @errp).
> * Call it with the current location temporarily set to the member's.
> + * @func() may store an Error through @errp, but must return non-zero then.
> * When @func() returns non-zero, break the loop and return that value.
> * Return zero when the loop completes.
> */
> int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func,
> - void *opaque)
> + void *opaque, Error **errp)
> {
> Location loc;
> QemuOpts *opts;
> @@ -1062,7 +1063,7 @@ int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func,
> loc_push_none(&loc);
> QTAILQ_FOREACH(opts, &list->head, next) {
> loc_restore(&opts->loc);
> - rc = func(opts, opaque);
> + rc = func(opaque, opts, errp);
> if (rc) {
> return rc;
> }
Do you want to enforce that if errp is set, that rc is non-zero, to
match your contract? Perhaps by assert(!*errp) at this point? But if
the return value goes away later in the series in favor of only using
errp, it may be a moot question.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH 5/9] QemuOpts: Convert qemu_opts_foreach() to Error
2015-05-28 16:18 ` Eric Blake
@ 2015-06-02 11:33 ` Markus Armbruster
2015-06-02 12:34 ` Eric Blake
0 siblings, 1 reply; 31+ messages in thread
From: Markus Armbruster @ 2015-06-02 11:33 UTC (permalink / raw)
To: Eric Blake; +Cc: pbonzini, qemu-devel
Eric Blake <eblake@redhat.com> writes:
> On 05/28/2015 06:21 AM, Markus Armbruster wrote:
>> Retain the function value for now, to permit selective conversion of
>> its callers.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> block/blkdebug.c | 6 ++--
>> hw/core/qdev-properties-system.c | 5 +--
>> include/qemu/option.h | 4 +--
>> include/ui/console.h | 2 +-
>> net/net.c | 9 ++---
>> net/vhost-user.c | 4 +--
>> numa.c | 5 +--
>> tpm.c | 6 ++--
>> ui/vnc.c | 2 +-
>> util/qemu-config.c | 4 +--
>> util/qemu-option.c | 7 ++--
>> vl.c | 72 ++++++++++++++++++++++++----------------
>> 12 files changed, 72 insertions(+), 54 deletions(-)
>>
>
>> diff --git a/block/blkdebug.c b/block/blkdebug.c
>> index 58f5105..50ef1fc 100644
>> --- a/block/blkdebug.c
>> +++ b/block/blkdebug.c
>> @@ -219,7 +219,7 @@ struct add_rule_data {
>> Error **errp;
>> };
>>
>> -static int add_rule(QemuOpts *opts, void *opaque)
>> +static int add_rule(void *opaque, QemuOpts *opts, Error **errp)
>
>> +++ b/include/qemu/option.h
>> @@ -125,9 +125,9 @@ QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
>> QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict);
>> void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp);
>>
>> -typedef int (*qemu_opts_loopfunc)(QemuOpts *opts, void *opaque);
>> +typedef int (*qemu_opts_loopfunc)(void *opaque, QemuOpts *opts, Error **errp);
>
> Might be nice to justify in the commit message why swapping the
> arguments of the callback made sense. But doesn't affect code
> correctness, so:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
Not sure what to write. Not even quite sure why I like it better this
way, only that I do. Could be because it puts the object we're
modifying (when we modify any of the argument objects) on the left.
>> +++ b/util/qemu-option.c
>> @@ -1047,13 +1047,14 @@ void qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc, Error **errp)
>> }
>>
>> /**
>> - * For each member of @list, call @func(member, @opaque).
>> + * For each member of @list, call @func(@opaque, member, @errp).
>> * Call it with the current location temporarily set to the member's.
>> + * @func() may store an Error through @errp, but must return non-zero then.
>> * When @func() returns non-zero, break the loop and return that value.
>> * Return zero when the loop completes.
>> */
>> int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func,
>> - void *opaque)
>> + void *opaque, Error **errp)
>> {
>> Location loc;
>> QemuOpts *opts;
>> @@ -1062,7 +1063,7 @@ int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func,
>> loc_push_none(&loc);
>> QTAILQ_FOREACH(opts, &list->head, next) {
>> loc_restore(&opts->loc);
>> - rc = func(opts, opaque);
>> + rc = func(opaque, opts, errp);
>> if (rc) {
>> return rc;
>> }
>
> Do you want to enforce that if errp is set, that rc is non-zero, to
> match your contract? Perhaps by assert(!*errp) at this point? But if
> the return value goes away later in the series in favor of only using
> errp, it may be a moot question.
assert(!*errp) is incorrect, because callers may pass a null errp to
ignore errors. Could do
if (rc) {
return rc;
}
assert(!errp || !*errp);
Catches misbehaving func() only when caller passes non-null errp. To
catcht it always, we'd need to use Error *err here, and
error_propagate(errp, err). I doubt it's worth the trouble.
What do you think?
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH 5/9] QemuOpts: Convert qemu_opts_foreach() to Error
2015-06-02 11:33 ` Markus Armbruster
@ 2015-06-02 12:34 ` Eric Blake
2015-06-02 14:13 ` Paolo Bonzini
0 siblings, 1 reply; 31+ messages in thread
From: Eric Blake @ 2015-06-02 12:34 UTC (permalink / raw)
To: Markus Armbruster; +Cc: pbonzini, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2971 bytes --]
On 06/02/2015 05:33 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> On 05/28/2015 06:21 AM, Markus Armbruster wrote:
>>> Retain the function value for now, to permit selective conversion of
>>> its callers.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>> -typedef int (*qemu_opts_loopfunc)(QemuOpts *opts, void *opaque);
>>> +typedef int (*qemu_opts_loopfunc)(void *opaque, QemuOpts *opts, Error **errp);
>>
>> Might be nice to justify in the commit message why swapping the
>> arguments of the callback made sense. But doesn't affect code
>> correctness, so:
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> Not sure what to write. Not even quite sure why I like it better this
> way, only that I do. Could be because it puts the object we're
> modifying (when we modify any of the argument objects) on the left.
That's indeed weak, but it's more justification than nothing, so I'll
buy it.
>>> int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func,
>>> - void *opaque)
>>> + void *opaque, Error **errp)
>>> {
>>> Location loc;
>>> QemuOpts *opts;
>>> @@ -1062,7 +1063,7 @@ int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func,
>>> loc_push_none(&loc);
>>> QTAILQ_FOREACH(opts, &list->head, next) {
>>> loc_restore(&opts->loc);
>>> - rc = func(opts, opaque);
>>> + rc = func(opaque, opts, errp);
>>> if (rc) {
>>> return rc;
>>> }
>>
>> Do you want to enforce that if errp is set, that rc is non-zero, to
>> match your contract? Perhaps by assert(!*errp) at this point? But if
>> the return value goes away later in the series in favor of only using
>> errp, it may be a moot question.
>
> assert(!*errp) is incorrect, because callers may pass a null errp to
> ignore errors. Could do
>
> if (rc) {
> return rc;
> }
> assert(!errp || !*errp);
>
> Catches misbehaving func() only when caller passes non-null errp. To
> catcht it always, we'd need to use Error *err here, and
> error_propagate(errp, err). I doubt it's worth the trouble.
Or, you could do:
if (!errp) {
errp = &error_abort;
}
...
if (rc) {
return rc;
}
assert(!*errp);
and not have to use error_propagate() - either the caller is tracking
errors, or the caller passed NULL because they are tracking return value
and we can assume that their callback will not raise errors (even on
negative returns). But that's a slightly different contract.
You're right that the possibility of NULL makes it not as trivial as I
first thought, so I'm starting to waffle on whether enforcing the
contract is worth the extra lines of code. Anyone else with an opinion?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH 5/9] QemuOpts: Convert qemu_opts_foreach() to Error
2015-06-02 12:34 ` Eric Blake
@ 2015-06-02 14:13 ` Paolo Bonzini
0 siblings, 0 replies; 31+ messages in thread
From: Paolo Bonzini @ 2015-06-02 14:13 UTC (permalink / raw)
To: Eric Blake, Markus Armbruster; +Cc: qemu-devel
On 02/06/2015 14:34, Eric Blake wrote:
> Or, you could do:
>
> if (!errp) { errp = &error_abort; } ... if (rc) { return rc; }
> assert(!*errp);
>
> and not have to use error_propagate() - either the caller is
> tracking errors, or the caller passed NULL because they are
> tracking return value and we can assume that their callback will
> not raise errors (even on negative returns). But that's a slightly
> different contract.
>
> You're right that the possibility of NULL makes it not as trivial
> as I first thought, so I'm starting to waffle on whether enforcing
> the contract is worth the extra lines of code. Anyone else with an
> opinion?
I think the simplest code is the best. :) Our error propagation
at last works surprisingly well, but can already be pretty heavy on
boilerplate...
Paolo
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PATCH 6/9] blkdebug: Simplify passing of Error through qemu_opts_foreach()
2015-05-28 12:21 [Qemu-devel] [PATCH 0/9] Miscellaneous error reporting improvements Markus Armbruster
` (4 preceding siblings ...)
2015-05-28 12:21 ` [Qemu-devel] [PATCH 5/9] QemuOpts: Convert qemu_opts_foreach() to Error Markus Armbruster
@ 2015-05-28 12:21 ` Markus Armbruster
2015-05-28 17:15 ` Eric Blake
2015-05-28 12:21 ` [Qemu-devel] [PATCH 7/9] QemuOpts: Drop qemu_opt_foreach() parameter abort_on_failure Markus Armbruster
` (3 subsequent siblings)
9 siblings, 1 reply; 31+ messages in thread
From: Markus Armbruster @ 2015-05-28 12:21 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, pbonzini, qemu-block
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: qemu-block@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
block/blkdebug.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 50ef1fc..1e92607 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -216,7 +216,6 @@ static int get_event_by_name(const char *name, BlkDebugEvent *event)
struct add_rule_data {
BDRVBlkdebugState *s;
int action;
- Error **errp;
};
static int add_rule(void *opaque, QemuOpts *opts, Error **errp)
@@ -230,10 +229,10 @@ static int add_rule(void *opaque, QemuOpts *opts, Error **errp)
/* Find the right event for the rule */
event_name = qemu_opt_get(opts, "event");
if (!event_name) {
- error_setg(d->errp, "Missing event name for rule");
+ error_setg(errp, "Missing event name for rule");
return -1;
} else if (get_event_by_name(event_name, &event) < 0) {
- error_setg(d->errp, "Invalid event name \"%s\"", event_name);
+ error_setg(errp, "Invalid event name \"%s\"", event_name);
return -1;
}
@@ -319,8 +318,7 @@ static int read_config(BDRVBlkdebugState *s, const char *filename,
d.s = s;
d.action = ACTION_INJECT_ERROR;
- d.errp = &local_err;
- qemu_opts_foreach(&inject_error_opts, add_rule, &d, &error_abort);
+ qemu_opts_foreach(&inject_error_opts, add_rule, &d, &local_err);
if (local_err) {
error_propagate(errp, local_err);
ret = -EINVAL;
@@ -328,7 +326,7 @@ static int read_config(BDRVBlkdebugState *s, const char *filename,
}
d.action = ACTION_SET_STATE;
- qemu_opts_foreach(&set_state_opts, add_rule, &d, &error_abort);
+ qemu_opts_foreach(&set_state_opts, add_rule, &d, &local_err);
if (local_err) {
error_propagate(errp, local_err);
ret = -EINVAL;
--
1.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PATCH 7/9] QemuOpts: Drop qemu_opt_foreach() parameter abort_on_failure
2015-05-28 12:21 [Qemu-devel] [PATCH 0/9] Miscellaneous error reporting improvements Markus Armbruster
` (5 preceding siblings ...)
2015-05-28 12:21 ` [Qemu-devel] [PATCH 6/9] blkdebug: Simplify passing of Error through qemu_opts_foreach() Markus Armbruster
@ 2015-05-28 12:21 ` Markus Armbruster
2015-05-28 18:57 ` Eric Blake
2015-05-28 12:21 ` [Qemu-devel] [PATCH 8/9] QemuOpts: Convert qemu_opt_foreach() to Error Markus Armbruster
` (2 subsequent siblings)
9 siblings, 1 reply; 31+ messages in thread
From: Markus Armbruster @ 2015-05-28 12:21 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini
When the argument is non-zero, qemu_opt_foreach() stops on callback
returning non-zero, and returns that value.
When the argument is zero, it doesn't stop, and returns the callback's
value from the last iteration.
The two callers that pass zero could just as well pass one:
* qemu_spice_init()'s callback add_channel() either returns zero or
exit()s.
* config_write_opts()'s callback config_write_opt() always returns
zero.
Drop the parameter, and always stop.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
include/qemu/option.h | 3 +--
net/vhost-user.c | 2 +-
qdev-monitor.c | 2 +-
ui/spice-core.c | 2 +-
util/qemu-config.c | 2 +-
util/qemu-option.c | 17 +++++++++++------
vl.c | 4 ++--
7 files changed, 18 insertions(+), 14 deletions(-)
diff --git a/include/qemu/option.h b/include/qemu/option.h
index a3850b2..a3cf4c1 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -101,8 +101,7 @@ void qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val,
void qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val,
Error **errp);
typedef int (*qemu_opt_loopfunc)(const char *name, const char *value, void *opaque);
-int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
- int abort_on_failure);
+int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque);
QemuOpts *qemu_opts_find(QemuOptsList *list, const char *id);
QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
diff --git a/net/vhost-user.c b/net/vhost-user.c
index 545f203..dae0965 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -185,7 +185,7 @@ static CharDriverState *net_vhost_parse_chardev(const NetdevVhostUserOptions *op
/* inspect chardev opts */
memset(&props, 0, sizeof(props));
- if (qemu_opt_foreach(chr->opts, net_vhost_chardev_opts, &props, true) != 0) {
+ if (qemu_opt_foreach(chr->opts, net_vhost_chardev_opts, &props)) {
return NULL;
}
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 1d87f57..da2cbc9 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -564,7 +564,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
}
/* set properties */
- if (qemu_opt_foreach(opts, set_property, dev, 1) != 0) {
+ if (qemu_opt_foreach(opts, set_property, dev)) {
object_unparent(OBJECT(dev));
object_unref(OBJECT(dev));
return NULL;
diff --git a/ui/spice-core.c b/ui/spice-core.c
index f00e074..1338d86 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -782,7 +782,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, 0);
+ qemu_opt_foreach(opts, add_channel, &tls_port);
spice_server_set_name(spice_server, qemu_name);
spice_server_set_uuid(spice_server, qemu_uuid);
diff --git a/util/qemu-config.c b/util/qemu-config.c
index 281c47d..f5965ab 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -353,7 +353,7 @@ static int config_write_opts(void *opaque, QemuOpts *opts, Error **errp)
} else {
fprintf(data->fp, "[%s]\n", data->list->name);
}
- qemu_opt_foreach(opts, config_write_opt, data, 0);
+ qemu_opt_foreach(opts, config_write_opt, data);
fprintf(data->fp, "\n");
return 0;
}
diff --git a/util/qemu-option.c b/util/qemu-option.c
index b56aac8..e1f28fc 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -596,18 +596,23 @@ void qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val,
QTAILQ_INSERT_TAIL(&opts->head, opt, next);
}
-int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
- int abort_on_failure)
+/**
+ * For each member of @opts, call @func(name, value, @opaque).
+ * When @func() returns non-zero, break the loop and return that value.
+ * Return zero when the loop completes.
+ */
+int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque)
{
QemuOpt *opt;
- int rc = 0;
+ int rc;
QTAILQ_FOREACH(opt, &opts->head, next) {
rc = func(opt->name, opt->str, opaque);
- if (abort_on_failure && rc != 0)
- break;
+ if (rc) {
+ return rc;
+ }
}
- return rc;
+ return 0;
}
QemuOpts *qemu_opts_find(QemuOptsList *list, const char *id)
diff --git a/vl.c b/vl.c
index 84a19a9..5b51e6d 100644
--- a/vl.c
+++ b/vl.c
@@ -4065,8 +4065,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,
- 1) < 0) {
+ if (qemu_opt_foreach(machine_opts, machine_set_property,
+ current_machine)) {
object_unref(OBJECT(current_machine));
exit(1);
}
--
1.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH 7/9] QemuOpts: Drop qemu_opt_foreach() parameter abort_on_failure
2015-05-28 12:21 ` [Qemu-devel] [PATCH 7/9] QemuOpts: Drop qemu_opt_foreach() parameter abort_on_failure Markus Armbruster
@ 2015-05-28 18:57 ` Eric Blake
0 siblings, 0 replies; 31+ messages in thread
From: Eric Blake @ 2015-05-28 18:57 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: pbonzini
[-- Attachment #1: Type: text/plain, Size: 1236 bytes --]
On 05/28/2015 06:21 AM, Markus Armbruster wrote:
> When the argument is non-zero, qemu_opt_foreach() stops on callback
> returning non-zero, and returns that value.
>
> When the argument is zero, it doesn't stop, and returns the callback's
> value from the last iteration.
>
> The two callers that pass zero could just as well pass one:
>
> * qemu_spice_init()'s callback add_channel() either returns zero or
> exit()s.
>
> * config_write_opts()'s callback config_write_opt() always returns
> zero.
>
> Drop the parameter, and always stop.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> +++ b/net/vhost-user.c
> @@ -185,7 +185,7 @@ static CharDriverState *net_vhost_parse_chardev(const NetdevVhostUserOptions *op
>
> /* inspect chardev opts */
> memset(&props, 0, sizeof(props));
> - if (qemu_opt_foreach(chr->opts, net_vhost_chardev_opts, &props, true) != 0) {
> + if (qemu_opt_foreach(chr->opts, net_vhost_chardev_opts, &props)) {
Another case of confusion on 'int' vs. 'bool' gone. Good riddance!
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PATCH 8/9] QemuOpts: Convert qemu_opt_foreach() to Error
2015-05-28 12:21 [Qemu-devel] [PATCH 0/9] Miscellaneous error reporting improvements Markus Armbruster
` (6 preceding siblings ...)
2015-05-28 12:21 ` [Qemu-devel] [PATCH 7/9] QemuOpts: Drop qemu_opt_foreach() parameter abort_on_failure Markus Armbruster
@ 2015-05-28 12:21 ` Markus Armbruster
2015-05-28 19:07 ` Eric Blake
2015-05-28 12:21 ` [Qemu-devel] [PATCH 9/9] vhost-user: Improve -netdev/netdev_add/-net/... error reporting Markus Armbruster
2015-05-29 8:51 ` [Qemu-devel] [PATCH 0/9] Miscellaneous error reporting improvements Kevin Wolf
9 siblings, 1 reply; 31+ messages in thread
From: Markus Armbruster @ 2015-05-28 12:21 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini
Retain the function value for now, to permit selective conversion of
its callers.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
include/qemu/option.h | 7 +++++--
net/vhost-user.c | 8 +++++---
qdev-monitor.c | 5 +++--
ui/spice-core.c | 5 +++--
util/qemu-config.c | 5 +++--
util/qemu-option.c | 8 +++++---
vl.c | 9 +++++----
7 files changed, 29 insertions(+), 18 deletions(-)
diff --git a/include/qemu/option.h b/include/qemu/option.h
index a3cf4c1..ac0e43b 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -100,8 +100,11 @@ void qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val,
Error **errp);
void qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val,
Error **errp);
-typedef int (*qemu_opt_loopfunc)(const char *name, const char *value, void *opaque);
-int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque);
+typedef int (*qemu_opt_loopfunc)(void *opaque,
+ const char *name, const char *value,
+ Error **errp);
+int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
+ Error **errp);
QemuOpts *qemu_opts_find(QemuOptsList *list, const char *id);
QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
diff --git a/net/vhost-user.c b/net/vhost-user.c
index dae0965..2effe29 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -153,8 +153,9 @@ static int net_vhost_user_init(NetClientState *peer, const char *device,
return 0;
}
-static int net_vhost_chardev_opts(const char *name, const char *value,
- void *opaque)
+static int net_vhost_chardev_opts(void *opaque,
+ const char *name, const char *value,
+ Error **errp)
{
VhostUserChardevProps *props = opaque;
@@ -185,7 +186,8 @@ static CharDriverState *net_vhost_parse_chardev(const NetdevVhostUserOptions *op
/* inspect chardev opts */
memset(&props, 0, sizeof(props));
- if (qemu_opt_foreach(chr->opts, net_vhost_chardev_opts, &props)) {
+ if (qemu_opt_foreach(chr->opts, net_vhost_chardev_opts, &props,
+ &error_abort)) {
return NULL;
}
diff --git a/qdev-monitor.c b/qdev-monitor.c
index da2cbc9..93cda97 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -143,7 +143,8 @@ static void qdev_print_devinfos(bool show_no_user)
g_slist_free(list);
}
-static int set_property(const char *name, const char *value, void *opaque)
+static int set_property(void *opaque, const char *name, const char *value,
+ Error **errp)
{
Object *obj = opaque;
Error *err = NULL;
@@ -564,7 +565,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
}
/* set properties */
- if (qemu_opt_foreach(opts, set_property, dev)) {
+ if (qemu_opt_foreach(opts, set_property, dev, &error_abort)) {
object_unparent(OBJECT(dev));
object_unref(OBJECT(dev));
return NULL;
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 1338d86..2d95241 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -583,7 +583,8 @@ int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
return ret;
}
-static int add_channel(const char *name, const char *value, void *opaque)
+static int add_channel(void *opaque, const char *name, const char *value,
+ Error **errp)
{
int security = 0;
int rc;
@@ -782,7 +783,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);
+ qemu_opt_foreach(opts, add_channel, &tls_port, &error_abort);
spice_server_set_name(spice_server, qemu_name);
spice_server_set_uuid(spice_server, qemu_uuid);
diff --git a/util/qemu-config.c b/util/qemu-config.c
index f5965ab..cf53d1d 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -335,7 +335,8 @@ struct ConfigWriteData {
FILE *fp;
};
-static int config_write_opt(const char *name, const char *value, void *opaque)
+static int config_write_opt(void *opaque, const char *name, const char *value,
+ Error **errp)
{
struct ConfigWriteData *data = opaque;
@@ -353,7 +354,7 @@ static int config_write_opts(void *opaque, QemuOpts *opts, Error **errp)
} else {
fprintf(data->fp, "[%s]\n", data->list->name);
}
- qemu_opt_foreach(opts, config_write_opt, data);
+ qemu_opt_foreach(opts, config_write_opt, data, &error_abort);
fprintf(data->fp, "\n");
return 0;
}
diff --git a/util/qemu-option.c b/util/qemu-option.c
index e1f28fc..1b1a338 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -597,17 +597,19 @@ void qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val,
}
/**
- * For each member of @opts, call @func(name, value, @opaque).
+ * For each member of @opts, call @func(@opaque, name, value, @errp).
+ * @func() may store an Error through @errp, but must return non-zero then.
* When @func() returns non-zero, break the loop and return that value.
* Return zero when the loop completes.
*/
-int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque)
+int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
+ Error **errp)
{
QemuOpt *opt;
int rc;
QTAILQ_FOREACH(opt, &opts->head, next) {
- rc = func(opt->name, opt->str, opaque);
+ rc = func(opaque, opt->name, opt->str, errp);
if (rc) {
return rc;
}
diff --git a/vl.c b/vl.c
index 5b51e6d..30b55a0 100644
--- a/vl.c
+++ b/vl.c
@@ -2571,8 +2571,9 @@ static void free_and_trace(gpointer mem)
free(mem);
}
-static int machine_set_property(const char *name, const char *value,
- void *opaque)
+static int machine_set_property(void *opaque,
+ const char *name, const char *value,
+ Error **errp)
{
Object *obj = OBJECT(opaque);
Error *local_err = NULL;
@@ -4065,8 +4066,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)) {
+ if (qemu_opt_foreach(machine_opts, machine_set_property, current_machine,
+ &error_abort)) {
object_unref(OBJECT(current_machine));
exit(1);
}
--
1.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH 8/9] QemuOpts: Convert qemu_opt_foreach() to Error
2015-05-28 12:21 ` [Qemu-devel] [PATCH 8/9] QemuOpts: Convert qemu_opt_foreach() to Error Markus Armbruster
@ 2015-05-28 19:07 ` Eric Blake
0 siblings, 0 replies; 31+ messages in thread
From: Eric Blake @ 2015-05-28 19:07 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: pbonzini
[-- Attachment #1: Type: text/plain, Size: 2685 bytes --]
On 05/28/2015 06:21 AM, Markus Armbruster wrote:
> Retain the function value for now, to permit selective conversion of
> its callers.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> include/qemu/option.h | 7 +++++--
> net/vhost-user.c | 8 +++++---
> qdev-monitor.c | 5 +++--
> ui/spice-core.c | 5 +++--
> util/qemu-config.c | 5 +++--
> util/qemu-option.c | 8 +++++---
> vl.c | 9 +++++----
> 7 files changed, 29 insertions(+), 18 deletions(-)
>
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index a3cf4c1..ac0e43b 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -100,8 +100,11 @@ void qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val,
> Error **errp);
> void qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val,
> Error **errp);
> -typedef int (*qemu_opt_loopfunc)(const char *name, const char *value, void *opaque);
> -int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque);
> +typedef int (*qemu_opt_loopfunc)(void *opaque,
> + const char *name, const char *value,
> + Error **errp);
Again, justification for reordering callback parameter ordering might be
nice to mention in the commit message, but the code itself is correct, so:
Reviewed-by: Eric Blake <eblake@redhat.com>
> +++ b/util/qemu-option.c
> @@ -597,17 +597,19 @@ void qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val,
> }
>
> /**
> - * For each member of @opts, call @func(name, value, @opaque).
> + * For each member of @opts, call @func(@opaque, name, value, @errp).
> + * @func() may store an Error through @errp, but must return non-zero then.
> * When @func() returns non-zero, break the loop and return that value.
> * Return zero when the loop completes.
> */
> -int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque)
> +int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
> + Error **errp)
> {
> QemuOpt *opt;
> int rc;
>
> QTAILQ_FOREACH(opt, &opts->head, next) {
> - rc = func(opt->name, opt->str, opaque);
> + rc = func(opaque, opt->name, opt->str, errp);
> if (rc) {
> return rc;
> }
As in my earlier review, wouldn't it be better to add:
assert(!*errp)
at this point, to ensure we meet the contract?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PATCH 9/9] vhost-user: Improve -netdev/netdev_add/-net/... error reporting
2015-05-28 12:21 [Qemu-devel] [PATCH 0/9] Miscellaneous error reporting improvements Markus Armbruster
` (7 preceding siblings ...)
2015-05-28 12:21 ` [Qemu-devel] [PATCH 8/9] QemuOpts: Convert qemu_opt_foreach() to Error Markus Armbruster
@ 2015-05-28 12:21 ` Markus Armbruster
2015-05-28 19:20 ` Eric Blake
2015-06-02 16:32 ` Stefan Hajnoczi
2015-05-29 8:51 ` [Qemu-devel] [PATCH 0/9] Miscellaneous error reporting improvements Kevin Wolf
9 siblings, 2 replies; 31+ messages in thread
From: Markus Armbruster @ 2015-05-28 12:21 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, Jason Wang, Stefan Hajnoczi
When -netdev vhost-user fails, it first reports a specific error, then
one or more generic ones, like this:
$ qemu-system-x86_64 -netdev vhost-user,id=foo,chardev=xxx
qemu-system-x86_64: -netdev vhost-user,id=foo,chardev=xxx: chardev "xxx" not found
qemu-system-x86_64: -netdev vhost-user,id=foo,chardev=xxx: No suitable chardev found
qemu-system-x86_64: -netdev vhost-user,id=foo,chardev=xxx: Device 'vhost-user' could not be initialized
With the command line, the messages go to stderr. In HMP, they go to
the monitor. In QMP, the last one becomes the error reply, and the
others go to stderr.
Convert net_init_vhost_user() and its helpers to Error. This
suppresses the unwanted unspecific error messages, and makes the
specific error the QMP error reply.
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
net/vhost-user.c | 27 ++++++++++++---------------
1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/net/vhost-user.c b/net/vhost-user.c
index 2effe29..49d340e 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -166,34 +166,34 @@ static int net_vhost_chardev_opts(void *opaque,
} else if (strcmp(name, "server") == 0) {
props->is_server = true;
} else {
- error_report("vhost-user does not support a chardev"
- " with the following option:\n %s = %s",
- name, value);
+ error_setg(errp,
+ "vhost-user does not support a chardev with option %s=%s",
+ name, value);
return -1;
}
return 0;
}
-static CharDriverState *net_vhost_parse_chardev(const NetdevVhostUserOptions *opts)
+static CharDriverState *net_vhost_parse_chardev(
+ const NetdevVhostUserOptions *opts, Error **errp)
{
CharDriverState *chr = qemu_chr_find(opts->chardev);
VhostUserChardevProps props;
if (chr == NULL) {
- error_report("chardev \"%s\" not found", opts->chardev);
+ error_setg(errp, "chardev \"%s\" not found", opts->chardev);
return NULL;
}
/* inspect chardev opts */
memset(&props, 0, sizeof(props));
- if (qemu_opt_foreach(chr->opts, net_vhost_chardev_opts, &props,
- &error_abort)) {
+ if (qemu_opt_foreach(chr->opts, net_vhost_chardev_opts, &props, errp)) {
return NULL;
}
if (!props.is_socket || !props.is_unix) {
- error_report("chardev \"%s\" is not a unix socket",
- opts->chardev);
+ error_setg(errp, "chardev \"%s\" is not a unix socket",
+ opts->chardev);
return NULL;
}
@@ -217,7 +217,7 @@ static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp)
if (strcmp(netdev, name) == 0 &&
strncmp(driver, virtio_name, strlen(virtio_name)) != 0) {
- error_report("vhost-user requires frontend driver virtio-net-*");
+ error_setg(errp, "vhost-user requires frontend driver virtio-net-*");
return -1;
}
@@ -227,25 +227,22 @@ static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp)
int net_init_vhost_user(const NetClientOptions *opts, const char *name,
NetClientState *peer, Error **errp)
{
- /* FIXME error_setg(errp, ...) on failure */
const NetdevVhostUserOptions *vhost_user_opts;
CharDriverState *chr;
assert(opts->kind == NET_CLIENT_OPTIONS_KIND_VHOST_USER);
vhost_user_opts = opts->vhost_user;
- chr = net_vhost_parse_chardev(vhost_user_opts);
+ chr = net_vhost_parse_chardev(vhost_user_opts, errp);
if (!chr) {
- error_report("No suitable chardev found");
return -1;
}
/* verify net frontend */
if (qemu_opts_foreach(qemu_find_opts("device"), net_vhost_check_net,
- (char *)name, &error_abort)) {
+ (char *)name, errp)) {
return -1;
}
-
return net_vhost_user_init(peer, "vhost_user", name, chr);
}
--
1.9.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH 9/9] vhost-user: Improve -netdev/netdev_add/-net/... error reporting
2015-05-28 12:21 ` [Qemu-devel] [PATCH 9/9] vhost-user: Improve -netdev/netdev_add/-net/... error reporting Markus Armbruster
@ 2015-05-28 19:20 ` Eric Blake
2015-06-02 16:32 ` Stefan Hajnoczi
1 sibling, 0 replies; 31+ messages in thread
From: Eric Blake @ 2015-05-28 19:20 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: pbonzini, Jason Wang, Stefan Hajnoczi
[-- Attachment #1: Type: text/plain, Size: 1297 bytes --]
On 05/28/2015 06:21 AM, Markus Armbruster wrote:
> When -netdev vhost-user fails, it first reports a specific error, then
> one or more generic ones, like this:
>
> $ qemu-system-x86_64 -netdev vhost-user,id=foo,chardev=xxx
> qemu-system-x86_64: -netdev vhost-user,id=foo,chardev=xxx: chardev "xxx" not found
> qemu-system-x86_64: -netdev vhost-user,id=foo,chardev=xxx: No suitable chardev found
> qemu-system-x86_64: -netdev vhost-user,id=foo,chardev=xxx: Device 'vhost-user' could not be initialized
>
> With the command line, the messages go to stderr. In HMP, they go to
> the monitor. In QMP, the last one becomes the error reply, and the
> others go to stderr.
>
> Convert net_init_vhost_user() and its helpers to Error. This
> suppresses the unwanted unspecific error messages, and makes the
> specific error the QMP error reply.
>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> net/vhost-user.c | 27 ++++++++++++---------------
> 1 file changed, 12 insertions(+), 15 deletions(-)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PATCH 9/9] vhost-user: Improve -netdev/netdev_add/-net/... error reporting
2015-05-28 12:21 ` [Qemu-devel] [PATCH 9/9] vhost-user: Improve -netdev/netdev_add/-net/... error reporting Markus Armbruster
2015-05-28 19:20 ` Eric Blake
@ 2015-06-02 16:32 ` Stefan Hajnoczi
1 sibling, 0 replies; 31+ messages in thread
From: Stefan Hajnoczi @ 2015-06-02 16:32 UTC (permalink / raw)
To: Markus Armbruster; +Cc: pbonzini, Jason Wang, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1207 bytes --]
On Thu, May 28, 2015 at 02:21:35PM +0200, Markus Armbruster wrote:
> When -netdev vhost-user fails, it first reports a specific error, then
> one or more generic ones, like this:
>
> $ qemu-system-x86_64 -netdev vhost-user,id=foo,chardev=xxx
> qemu-system-x86_64: -netdev vhost-user,id=foo,chardev=xxx: chardev "xxx" not found
> qemu-system-x86_64: -netdev vhost-user,id=foo,chardev=xxx: No suitable chardev found
> qemu-system-x86_64: -netdev vhost-user,id=foo,chardev=xxx: Device 'vhost-user' could not be initialized
>
> With the command line, the messages go to stderr. In HMP, they go to
> the monitor. In QMP, the last one becomes the error reply, and the
> others go to stderr.
>
> Convert net_init_vhost_user() and its helpers to Error. This
> suppresses the unwanted unspecific error messages, and makes the
> specific error the QMP error reply.
>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> net/vhost-user.c | 27 ++++++++++++---------------
> 1 file changed, 12 insertions(+), 15 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PATCH 0/9] Miscellaneous error reporting improvements
2015-05-28 12:21 [Qemu-devel] [PATCH 0/9] Miscellaneous error reporting improvements Markus Armbruster
` (8 preceding siblings ...)
2015-05-28 12:21 ` [Qemu-devel] [PATCH 9/9] vhost-user: Improve -netdev/netdev_add/-net/... error reporting Markus Armbruster
@ 2015-05-29 8:51 ` Kevin Wolf
2015-05-29 11:22 ` Markus Armbruster
9 siblings, 1 reply; 31+ messages in thread
From: Kevin Wolf @ 2015-05-29 8:51 UTC (permalink / raw)
To: Markus Armbruster; +Cc: pbonzini, qemu-devel
Am 28.05.2015 um 14:21 hat Markus Armbruster geschrieben:
> Touches vl.c, which gives me pretext to ask Paolo: would you be
> willing to take this through your tree? Or should I take it through
> mine?
After this series we have an ugly half-converted state where
qemu_opts_foreach() has both a return code and an Error object,
and it's not generally true that an error is set for a failing
return code.
The most confusing part about this is that you have &error_abort almost
everywhere, but the function doesn't actually abort on error, but rather
returns a negative error code and leaves errp alone.
If you don't want to complete the conversion, can we add it to that wiki
page with the list half-done conversions at least?
Anyway, my opinion is only authoritative for the block layer, and that
part looks good, so for the blkdebug part:
Acked-by: Kevin Wolf <kwolf@redhat.com>
PS: I would have preferred to be CCed on the whole series instead of
just a single patch. That one patch wasn't the only one that touched
blkdebug and it also wasn't quite enough to understand the context. So
CC on one patch is enough to make me aware of the series, but CC on the
whole series would also make it convenient to review it.
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH 0/9] Miscellaneous error reporting improvements
2015-05-29 8:51 ` [Qemu-devel] [PATCH 0/9] Miscellaneous error reporting improvements Kevin Wolf
@ 2015-05-29 11:22 ` Markus Armbruster
2015-05-29 12:42 ` Kevin Wolf
2015-05-29 14:00 ` Eric Blake
0 siblings, 2 replies; 31+ messages in thread
From: Markus Armbruster @ 2015-05-29 11:22 UTC (permalink / raw)
To: Kevin Wolf; +Cc: pbonzini, qemu-devel
Kevin Wolf <kwolf@redhat.com> writes:
> Am 28.05.2015 um 14:21 hat Markus Armbruster geschrieben:
>> Touches vl.c, which gives me pretext to ask Paolo: would you be
>> willing to take this through your tree? Or should I take it through
>> mine?
>
> After this series we have an ugly half-converted state where
> qemu_opts_foreach() has both a return code and an Error object,
> and it's not generally true that an error is set for a failing
> return code.
>
> The most confusing part about this is that you have &error_abort almost
> everywhere, but the function doesn't actually abort on error, but rather
> returns a negative error code and leaves errp alone.
True. The function contract spells it out, which hopefully reduces the
confusion somewhat.
Would you find NULL less confusing than &error_abort?
> If you don't want to complete the conversion, can we add it to that wiki
> page with the list half-done conversions at least?
Can do.
"That wiki page" = http://wiki.qemu.org/CodeTransitions
> Anyway, my opinion is only authoritative for the block layer, and that
> part looks good, so for the blkdebug part:
>
> Acked-by: Kevin Wolf <kwolf@redhat.com>
Thanks!
> PS: I would have preferred to be CCed on the whole series instead of
> just a single patch. That one patch wasn't the only one that touched
> blkdebug and it also wasn't quite enough to understand the context. So
> CC on one patch is enough to make me aware of the series, but CC on the
> whole series would also make it convenient to review it.
I'll try to remember your preference next time.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PATCH 0/9] Miscellaneous error reporting improvements
2015-05-29 11:22 ` Markus Armbruster
@ 2015-05-29 12:42 ` Kevin Wolf
2015-05-29 14:00 ` Eric Blake
1 sibling, 0 replies; 31+ messages in thread
From: Kevin Wolf @ 2015-05-29 12:42 UTC (permalink / raw)
To: Markus Armbruster; +Cc: pbonzini, qemu-devel
Am 29.05.2015 um 13:22 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > Am 28.05.2015 um 14:21 hat Markus Armbruster geschrieben:
> >> Touches vl.c, which gives me pretext to ask Paolo: would you be
> >> willing to take this through your tree? Or should I take it through
> >> mine?
> >
> > After this series we have an ugly half-converted state where
> > qemu_opts_foreach() has both a return code and an Error object,
> > and it's not generally true that an error is set for a failing
> > return code.
> >
> > The most confusing part about this is that you have &error_abort almost
> > everywhere, but the function doesn't actually abort on error, but rather
> > returns a negative error code and leaves errp alone.
>
> True. The function contract spells it out, which hopefully reduces the
> confusion somewhat.
>
> Would you find NULL less confusing than &error_abort?
That might be better, yes. At least it doesn't imply that no errors can
happen, just that we don't handle them. In places that actually do
handle errors using the return code even though passing a NULL errp, that
is obvious and doesn't lead to assumptions about the called function.
> > If you don't want to complete the conversion, can we add it to that wiki
> > page with the list half-done conversions at least?
>
> Can do.
>
> "That wiki page" = http://wiki.qemu.org/CodeTransitions
Yes, that's the one I meant. Thanks.
Kevin
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PATCH 0/9] Miscellaneous error reporting improvements
2015-05-29 11:22 ` Markus Armbruster
2015-05-29 12:42 ` Kevin Wolf
@ 2015-05-29 14:00 ` Eric Blake
2015-06-02 11:51 ` Markus Armbruster
1 sibling, 1 reply; 31+ messages in thread
From: Eric Blake @ 2015-05-29 14:00 UTC (permalink / raw)
To: Markus Armbruster, Kevin Wolf; +Cc: pbonzini, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1319 bytes --]
On 05/29/2015 05:22 AM, Markus Armbruster wrote:
> Kevin Wolf <kwolf@redhat.com> writes:
>
>> Am 28.05.2015 um 14:21 hat Markus Armbruster geschrieben:
>>> Touches vl.c, which gives me pretext to ask Paolo: would you be
>>> willing to take this through your tree? Or should I take it through
>>> mine?
>>
>> After this series we have an ugly half-converted state where
>> qemu_opts_foreach() has both a return code and an Error object,
>> and it's not generally true that an error is set for a failing
>> return code.
>>
>> The most confusing part about this is that you have &error_abort almost
>> everywhere, but the function doesn't actually abort on error, but rather
>> returns a negative error code and leaves errp alone.
>
> True. The function contract spells it out, which hopefully reduces the
> confusion somewhat.
Except that you don't enforce the contract; I suggested adding
assert(!*errp) at the right place in the two conversions.
>
> Would you find NULL less confusing than &error_abort?
NULL says to ignore errors, &error_abort says to diagnose errors as
programming bugs. If we know we aren't going to have an error, I prefer
diagnosing coding bugs.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PATCH 0/9] Miscellaneous error reporting improvements
2015-05-29 14:00 ` Eric Blake
@ 2015-06-02 11:51 ` Markus Armbruster
2015-06-02 12:51 ` Eric Blake
0 siblings, 1 reply; 31+ messages in thread
From: Markus Armbruster @ 2015-06-02 11:51 UTC (permalink / raw)
To: Eric Blake; +Cc: Kevin Wolf, pbonzini, qemu-devel
Eric Blake <eblake@redhat.com> writes:
> On 05/29/2015 05:22 AM, Markus Armbruster wrote:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>>> Am 28.05.2015 um 14:21 hat Markus Armbruster geschrieben:
>>>> Touches vl.c, which gives me pretext to ask Paolo: would you be
>>>> willing to take this through your tree? Or should I take it through
>>>> mine?
>>>
>>> After this series we have an ugly half-converted state where
>>> qemu_opts_foreach() has both a return code and an Error object,
>>> and it's not generally true that an error is set for a failing
>>> return code.
>>>
>>> The most confusing part about this is that you have &error_abort almost
>>> everywhere, but the function doesn't actually abort on error, but rather
>>> returns a negative error code and leaves errp alone.
>>
>> True. The function contract spells it out, which hopefully reduces the
>> confusion somewhat.
>
> Except that you don't enforce the contract; I suggested adding
> assert(!*errp) at the right place in the two conversions.
>
>>
>> Would you find NULL less confusing than &error_abort?
>
> NULL says to ignore errors, &error_abort says to diagnose errors as
> programming bugs. If we know we aren't going to have an error, I prefer
> diagnosing coding bugs.
You prefer &error_abort, Kevin prefers NULL, so I need to figure out
what I prefer to break the tie :)
I think we can agree on these two rules on Error ** arguments:
R1: When caller doesn't care whether the callee sets an error, it should
pass NULL.
R2: When a caller relies on the callee not setting an error, it should
pass &error_abort.
R1 applies, R2 does not, thus we should pass NULL.
The case for &error_abort requires a third rule:
Proposed R3: When a caller knows that the callee won't set an error, it
may pass &error_abort to document this knowledge even when it doesn't
actually rely on it (thus R2 doesn't apply). This is an exception to
R1.
To keep things simple, I lean towards rejecting R3 and passing NULL.
Opinions?
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PATCH 0/9] Miscellaneous error reporting improvements
2015-06-02 11:51 ` Markus Armbruster
@ 2015-06-02 12:51 ` Eric Blake
0 siblings, 0 replies; 31+ messages in thread
From: Eric Blake @ 2015-06-02 12:51 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Kevin Wolf, pbonzini, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2111 bytes --]
On 06/02/2015 05:51 AM, Markus Armbruster wrote:
>>>> The most confusing part about this is that you have &error_abort almost
>>>> everywhere, but the function doesn't actually abort on error, but rather
>>>> returns a negative error code and leaves errp alone.
>>>
>>> True. The function contract spells it out, which hopefully reduces the
>>> confusion somewhat.
>>
>> Except that you don't enforce the contract; I suggested adding
>> assert(!*errp) at the right place in the two conversions.
>>
>>>
>>> Would you find NULL less confusing than &error_abort?
>>
>> NULL says to ignore errors, &error_abort says to diagnose errors as
>> programming bugs. If we know we aren't going to have an error, I prefer
>> diagnosing coding bugs.
>
> You prefer &error_abort, Kevin prefers NULL, so I need to figure out
> what I prefer to break the tie :)
>
> I think we can agree on these two rules on Error ** arguments:
>
> R1: When caller doesn't care whether the callee sets an error, it should
> pass NULL.
>
> R2: When a caller relies on the callee not setting an error, it should
> pass &error_abort.
Yes, these two rules cover the current state of the art.
>
> R1 applies, R2 does not, thus we should pass NULL.
>
> The case for &error_abort requires a third rule:
>
> Proposed R3: When a caller knows that the callee won't set an error, it
> may pass &error_abort to document this knowledge even when it doesn't
> actually rely on it (thus R2 doesn't apply). This is an exception to
> R1.
Or, as I explored in another message, if the caller passes NULL, but we
then turn it to &error_abort locally, to enforce that the callback does
not set an error for either success or failure.
>
> To keep things simple, I lean towards rejecting R3 and passing NULL.
>
> Opinions?
At this point I'm leaning towards simplicity - pass NULL, and not worth
modifying the contract (passing NULL does not need to get transformed
into error_abort).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread