Paolo Bonzini <pbonz...@redhat.com> writes:
> This enables some simplification of vl.c via error_fatal.
>
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> ---
> block/blkdebug.c | 3 +--
> include/qemu/config-file.h | 4 ++--
> softmmu/vl.c | 30 ++++++++++++------------------
> util/qemu-config.c | 20 ++++++++++----------
> 4 files changed, 25 insertions(+), 32 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 7d26fe3816..da6f4690b7 100644
> --- a/include/qemu/config-file.h
> +++ b/include/qemu/config-file.h
> @@ -10,9 +10,9 @@ void qemu_add_opts(QemuOptsList *list);
> void qemu_add_drive_opts(QemuOptsList *list);
> int qemu_global_option(const char *str);
>
> -int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname);
> +int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname,
> Error **errp);
Long line.
>
> -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 d34307bf11..d991919155 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2056,17 +2056,20 @@ 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)
> {
> int ret;
> + Error *local_err = NULL;
> 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, &local_err);
> + if (ret < 0) {
> + if (ret == -ENOENT) {
> + error_free(local_err);
> + } else {
> + error_propagate(errp, local_err);
> + }
> }
> -
> - return 0;
> }
Please use ERRP_GUARD() in new code:
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, errp);
if (ret == -ENOENT) {
error_free(*errp);
*errp = NULL;
}
}
>
> static int qemu_set_option(const char *str)
> @@ -2622,9 +2625,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 */
> @@ -3312,15 +3313,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;
More than just code simplifcation: you're deleting an extra error
message. Test case:
$ qemu-system-x86_64 -readconfig .
qemu: error reading file
qemu: -readconfig .: read config .: Invalid argument
Pretty bad. With your patch applied:
qemu-system-x86_64: error reading file
Worse :)
I actually expected this to come out as
qemu-system-x86_64: -readconfig .: error reading file
That would be an improvement.
The reason for the bad location information is here:
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;
QemuOptsList *list = NULL;
Error *local_err = NULL;
QemuOpts *opts = NULL;
int res = -EINVAL, lno = 0;
int count = 0;
loc_push_none(&loc);
while (fgets(line, sizeof(line), fp) != NULL) {
If the very first fgets() fails setting @fp's error indicator, ...
loc_set_file(fname, ++lno);
[...]
}
if (ferror(fp)) {
... we get here with error location "none", and ...
error_setg(errp, "error reading file");
... use it to report the error (remember, @errp is &error_fatal).
Independently, error_setg_errno() would be nice. Assuming we're willing
to rely on errno being useful after fgets().
goto out;
}
res = count;
out:
loc_pop(&loc);
return res;
}
Always, always, always test the error messages.
> 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 a4a1324c68..d0629f4960 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -308,7 +308,7 @@ void qemu_add_opts(QemuOptsList *list)
> }
>
> /* 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;
> @@ -333,7 +333,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);
> @@ -344,7 +344,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);
> @@ -356,20 +356,19 @@ 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");
*Ächz* Not your patch's fault.
> goto out;
> }
> if (ferror(fp)) {
> - error_report("error reading file");
> + error_setg(errp, "error reading file");
> goto out;
> }
> res = count;
> @@ -378,16 +377,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;
> }
