* [PATCH v2] qemu-config: add error propagation to qemu_config_parse
@ 2021-02-26 17:08 Paolo Bonzini
2021-02-26 17:46 ` Philippe Mathieu-Daudé
2021-03-03 13:48 ` Markus Armbruster
0 siblings, 2 replies; 4+ messages in thread
From: Paolo Bonzini @ 2021-02-26 17:08 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru
This enables some simplification of vl.c via error_fatal, and improves
error messages. Before:
Before:
$ ./qemu-system-x86_64 -readconfig .
qemu-system-x86_64: error reading file
qemu-system-x86_64: -readconfig .: read config .: Invalid argument
$ /usr/libexec/qemu-kvm -readconfig foo
qemu-kvm: -readconfig foo: read config foo: No such file or directory
After:
$ ./qemu-system-x86_64 -readconfig .
qemu-system-x86_64: -readconfig .: error reading file: Is a directory
$ ./qemu-system-x86_64 -readconfig foo
qemu-system-x86_64: -readconfig foo: Cannot read config file foo: No such file or directory
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/blkdebug.c | 3 +--
include/qemu/config-file.h | 5 +++--
softmmu/vl.c | 29 +++++++++++------------------
util/qemu-config.c | 23 ++++++++++++-----------
4 files changed, 27 insertions(+), 33 deletions(-)
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 5fe6172da9..7eaa8a28bf 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -279,9 +279,8 @@ static int read_config(BDRVBlkdebugState *s, const char *filename,
return -errno;
}
- ret = qemu_config_parse(f, config_groups, filename);
+ ret = qemu_config_parse(f, config_groups, filename, errp);
if (ret < 0) {
- error_setg(errp, "Could not parse blkdebug config file");
goto fail;
}
}
diff --git a/include/qemu/config-file.h b/include/qemu/config-file.h
index 29226107bd..8d3e53ae4d 100644
--- a/include/qemu/config-file.h
+++ b/include/qemu/config-file.h
@@ -11,9 +11,10 @@ void qemu_add_drive_opts(QemuOptsList *list);
int qemu_global_option(const char *str);
void qemu_config_write(FILE *fp);
-int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname);
+int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname,
+ Error **errp);
-int qemu_read_config_file(const char *filename);
+int qemu_read_config_file(const char *filename, Error **errp);
/* Parse QDict options as a replacement for a config file (allowing multiple
enumerated (0..(n-1)) configuration "sections") */
diff --git a/softmmu/vl.c b/softmmu/vl.c
index e67f91dd37..e80859c40d 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2062,17 +2062,19 @@ static int global_init_func(void *opaque, QemuOpts *opts, Error **errp)
return 0;
}
-static int qemu_read_default_config_file(void)
+static void qemu_read_default_config_file(Error **errp)
{
+ ERRP_GUARD();
int ret;
g_autofree char *file = get_relocated_path(CONFIG_QEMU_CONFDIR "/qemu.conf");
- ret = qemu_read_config_file(file);
- if (ret < 0 && ret != -ENOENT) {
- return ret;
+ ret = qemu_read_config_file(file, errp);
+ if (ret < 0) {
+ if (ret == -ENOENT) {
+ error_free(*errp);
+ *errp = NULL;
+ }
}
-
- return 0;
}
static int qemu_set_option(const char *str)
@@ -2640,9 +2642,7 @@ void qemu_init(int argc, char **argv, char **envp)
}
if (userconfig) {
- if (qemu_read_default_config_file() < 0) {
- exit(1);
- }
+ qemu_read_default_config_file(&error_fatal);
}
/* second pass of option parsing */
@@ -3330,15 +3330,8 @@ void qemu_init(int argc, char **argv, char **envp)
qemu_plugin_opt_parse(optarg, &plugin_list);
break;
case QEMU_OPTION_readconfig:
- {
- int ret = qemu_read_config_file(optarg);
- if (ret < 0) {
- error_report("read config %s: %s", optarg,
- strerror(-ret));
- exit(1);
- }
- break;
- }
+ qemu_read_config_file(optarg, &error_fatal);
+ break;
case QEMU_OPTION_spice:
olist = qemu_find_opts_err("spice", NULL);
if (!olist) {
diff --git a/util/qemu-config.c b/util/qemu-config.c
index e2a700b284..af89df109d 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -350,7 +350,7 @@ void qemu_config_write(FILE *fp)
}
/* Returns number of config groups on success, -errno on error */
-int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
+int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname, Error **errp)
{
char line[1024], group[64], id[64], arg[64], value[1024];
Location loc;
@@ -375,7 +375,7 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
/* group with id */
list = find_list(lists, group, &local_err);
if (local_err) {
- error_report_err(local_err);
+ error_propagate(errp, local_err);
goto out;
}
opts = qemu_opts_create(list, id, 1, NULL);
@@ -386,7 +386,7 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
/* group without id */
list = find_list(lists, group, &local_err);
if (local_err) {
- error_report_err(local_err);
+ error_propagate(errp, local_err);
goto out;
}
opts = qemu_opts_create(list, NULL, 0, &error_abort);
@@ -398,21 +398,21 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
sscanf(line, " %63s = \"\"", arg) == 1) {
/* arg = value */
if (opts == NULL) {
- error_report("no group defined");
+ error_setg(errp, "no group defined");
goto out;
}
- if (!qemu_opt_set(opts, arg, value, &local_err)) {
- error_report_err(local_err);
+ if (!qemu_opt_set(opts, arg, value, errp)) {
goto out;
}
continue;
}
- error_report("parse error");
+ error_setg(errp, "parse error");
goto out;
}
if (ferror(fp)) {
- error_report("error reading file");
- goto out;
+ loc_pop(&loc);
+ error_setg_errno(errp, errno, "error reading file");
+ return res;
}
res = count;
out:
@@ -420,16 +420,17 @@ out:
return res;
}
-int qemu_read_config_file(const char *filename)
+int qemu_read_config_file(const char *filename, Error **errp)
{
FILE *f = fopen(filename, "r");
int ret;
if (f == NULL) {
+ error_setg_errno(errp, errno, "Cannot read config file %s", filename);
return -errno;
}
- ret = qemu_config_parse(f, vm_config_groups, filename);
+ ret = qemu_config_parse(f, vm_config_groups, filename, errp);
fclose(f);
return ret;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] qemu-config: add error propagation to qemu_config_parse
2021-02-26 17:08 [PATCH v2] qemu-config: add error propagation to qemu_config_parse Paolo Bonzini
@ 2021-02-26 17:46 ` Philippe Mathieu-Daudé
2021-03-03 13:48 ` Markus Armbruster
1 sibling, 0 replies; 4+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-02-26 17:46 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: armbru
On 2/26/21 6:08 PM, Paolo Bonzini wrote:
> This enables some simplification of vl.c via error_fatal, and improves
> error messages. Before:
>
> Before:
> $ ./qemu-system-x86_64 -readconfig .
> qemu-system-x86_64: error reading file
> qemu-system-x86_64: -readconfig .: read config .: Invalid argument
> $ /usr/libexec/qemu-kvm -readconfig foo
> qemu-kvm: -readconfig foo: read config foo: No such file or directory
>
> After:
>
> $ ./qemu-system-x86_64 -readconfig .
> qemu-system-x86_64: -readconfig .: error reading file: Is a directory
> $ ./qemu-system-x86_64 -readconfig foo
> qemu-system-x86_64: -readconfig foo: Cannot read config file foo: No such file or directory
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/blkdebug.c | 3 +--
> include/qemu/config-file.h | 5 +++--
> softmmu/vl.c | 29 +++++++++++------------------
> util/qemu-config.c | 23 ++++++++++++-----------
> 4 files changed, 27 insertions(+), 33 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] qemu-config: add error propagation to qemu_config_parse
2021-02-26 17:08 [PATCH v2] qemu-config: add error propagation to qemu_config_parse Paolo Bonzini
2021-02-26 17:46 ` Philippe Mathieu-Daudé
@ 2021-03-03 13:48 ` Markus Armbruster
2021-03-03 13:55 ` Paolo Bonzini
1 sibling, 1 reply; 4+ messages in thread
From: Markus Armbruster @ 2021-03-03 13:48 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
Paolo Bonzini <pbonzini@redhat.com> writes:
> This enables some simplification of vl.c via error_fatal, and improves
> error messages. Before:
>
> Before:
> $ ./qemu-system-x86_64 -readconfig .
> qemu-system-x86_64: error reading file
> qemu-system-x86_64: -readconfig .: read config .: Invalid argument
> $ /usr/libexec/qemu-kvm -readconfig foo
> qemu-kvm: -readconfig foo: read config foo: No such file or directory
>
> After:
>
> $ ./qemu-system-x86_64 -readconfig .
> qemu-system-x86_64: -readconfig .: error reading file: Is a directory
> $ ./qemu-system-x86_64 -readconfig foo
> qemu-system-x86_64: -readconfig foo: Cannot read config file foo: No such file or directory
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Lovely :)
> ---
> block/blkdebug.c | 3 +--
> include/qemu/config-file.h | 5 +++--
> softmmu/vl.c | 29 +++++++++++------------------
> util/qemu-config.c | 23 ++++++++++++-----------
> 4 files changed, 27 insertions(+), 33 deletions(-)
>
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 5fe6172da9..7eaa8a28bf 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -279,9 +279,8 @@ static int read_config(BDRVBlkdebugState *s, const char *filename,
> return -errno;
> }
>
> - ret = qemu_config_parse(f, config_groups, filename);
> + ret = qemu_config_parse(f, config_groups, filename, errp);
> if (ret < 0) {
> - error_setg(errp, "Could not parse blkdebug config file");
> goto fail;
> }
> }
> diff --git a/include/qemu/config-file.h b/include/qemu/config-file.h
> index 29226107bd..8d3e53ae4d 100644
> --- a/include/qemu/config-file.h
> +++ b/include/qemu/config-file.h
> @@ -11,9 +11,10 @@ void qemu_add_drive_opts(QemuOptsList *list);
> int qemu_global_option(const char *str);
>
> void qemu_config_write(FILE *fp);
> -int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname);
> +int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname,
> + Error **errp);
>
> -int qemu_read_config_file(const char *filename);
> +int qemu_read_config_file(const char *filename, Error **errp);
>
> /* Parse QDict options as a replacement for a config file (allowing multiple
> enumerated (0..(n-1)) configuration "sections") */
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index e67f91dd37..e80859c40d 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2062,17 +2062,19 @@ static int global_init_func(void *opaque, QemuOpts *opts, Error **errp)
> return 0;
> }
>
> -static int qemu_read_default_config_file(void)
> +static void qemu_read_default_config_file(Error **errp)
> {
> + ERRP_GUARD();
> int ret;
> g_autofree char *file = get_relocated_path(CONFIG_QEMU_CONFDIR "/qemu.conf");
>
> - ret = qemu_read_config_file(file);
> - if (ret < 0 && ret != -ENOENT) {
> - return ret;
> + ret = qemu_read_config_file(file, errp);
> + if (ret < 0) {
> + if (ret == -ENOENT) {
> + error_free(*errp);
> + *errp = NULL;
> + }
> }
> -
> - return 0;
> }
This change to return void goes against advice in error.h:
* - Whenever practical, also return a value that indicates success /
* failure. This can make the error checking more concise, and can
* avoid useless error object creation and destruction. Note that
* we still have many functions returning void. We recommend
* • bool-valued functions return true on success / false on failure,
* • pointer-valued functions return non-null / null pointer, and
* • integer-valued functions return non-negative / negative.
Tolerable. The only caller wouldn't profit from a return value (it
passes &error_fatal), and more callers seem quite unlikely.
>
> static int qemu_set_option(const char *str)
> @@ -2640,9 +2642,7 @@ void qemu_init(int argc, char **argv, char **envp)
> }
>
> if (userconfig) {
> - if (qemu_read_default_config_file() < 0) {
> - exit(1);
> - }
> + qemu_read_default_config_file(&error_fatal);
> }
>
> /* second pass of option parsing */
> @@ -3330,15 +3330,8 @@ void qemu_init(int argc, char **argv, char **envp)
> qemu_plugin_opt_parse(optarg, &plugin_list);
> break;
> case QEMU_OPTION_readconfig:
> - {
> - int ret = qemu_read_config_file(optarg);
> - if (ret < 0) {
> - error_report("read config %s: %s", optarg,
> - strerror(-ret));
> - exit(1);
> - }
> - break;
> - }
> + qemu_read_config_file(optarg, &error_fatal);
> + break;
> case QEMU_OPTION_spice:
> olist = qemu_find_opts_err("spice", NULL);
> if (!olist) {
> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index e2a700b284..af89df109d 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -350,7 +350,7 @@ void qemu_config_write(FILE *fp)
> }
>
> /* Returns number of config groups on success, -errno on error */
> -int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
> +int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname, Error **errp)
> {
> char line[1024], group[64], id[64], arg[64], value[1024];
> Location loc;
> @@ -375,7 +375,7 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
> /* group with id */
> list = find_list(lists, group, &local_err);
> if (local_err) {
> - error_report_err(local_err);
> + error_propagate(errp, local_err);
> goto out;
> }
Please avoid error_propagate() where possible:
list = find_list(lists, group, errp);
if (!list) {
goto out;
}
> opts = qemu_opts_create(list, id, 1, NULL);
> @@ -386,7 +386,7 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
> /* group without id */
> list = find_list(lists, group, &local_err);
> if (local_err) {
> - error_report_err(local_err);
> + error_propagate(errp, local_err);
> goto out;
> }
Likewise.
> opts = qemu_opts_create(list, NULL, 0, &error_abort);
> @@ -398,21 +398,21 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
> sscanf(line, " %63s = \"\"", arg) == 1) {
> /* arg = value */
> if (opts == NULL) {
> - error_report("no group defined");
> + error_setg(errp, "no group defined");
> goto out;
> }
> - if (!qemu_opt_set(opts, arg, value, &local_err)) {
> - error_report_err(local_err);
> + if (!qemu_opt_set(opts, arg, value, errp)) {
> goto out;
> }
> continue;
> }
> - error_report("parse error");
> + error_setg(errp, "parse error");
> goto out;
> }
> if (ferror(fp)) {
> - error_report("error reading file");
> - goto out;
> + loc_pop(&loc);
> + error_setg_errno(errp, errno, "error reading file");
> + return res;
> }
> res = count;
> out:
> @@ -420,16 +420,17 @@ out:
> return res;
> }
>
> -int qemu_read_config_file(const char *filename)
> +int qemu_read_config_file(const char *filename, Error **errp)
> {
> FILE *f = fopen(filename, "r");
> int ret;
>
> if (f == NULL) {
> + error_setg_errno(errp, errno, "Cannot read config file %s", filename);
> return -errno;
> }
>
> - ret = qemu_config_parse(f, vm_config_groups, filename);
> + ret = qemu_config_parse(f, vm_config_groups, filename, errp);
> fclose(f);
> return ret;
> }
Not avoiding error_propagate() isn't wrong, so
Reviewed-by: Markus Armbruster <armbru@redhat.com>
But please consider avoiding it anyway.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] qemu-config: add error propagation to qemu_config_parse
2021-03-03 13:48 ` Markus Armbruster
@ 2021-03-03 13:55 ` Paolo Bonzini
0 siblings, 0 replies; 4+ messages in thread
From: Paolo Bonzini @ 2021-03-03 13:55 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
On 03/03/21 14:48, Markus Armbruster wrote:
> Reviewed-by: Markus Armbruster<armbru@redhat.com>
>
> But please consider avoiding it anyway.
It does make for a smaller patch, so I'll use that as an excuse instead
of me being old school. I'll clean it up in a separate patch.
Paolo
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-03-03 13:59 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-02-26 17:08 [PATCH v2] qemu-config: add error propagation to qemu_config_parse Paolo Bonzini
2021-02-26 17:46 ` Philippe Mathieu-Daudé
2021-03-03 13:48 ` Markus Armbruster
2021-03-03 13:55 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).