[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] config: qemu_config_parse() return number o
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] config: qemu_config_parse() return number of config groups |
Date: |
Mon, 27 Feb 2017 16:05:00 -0300 |
User-agent: |
Mutt/1.7.1 (2016-10-04) |
On Mon, Feb 27, 2017 at 06:59:51PM +0100, Markus Armbruster wrote:
> Uh, I totally forgot about this series. My apologies...
>
> Eduardo Habkost <address@hidden> writes:
>
> > Change qemu_config_parse() to return the number of config groups
> > in success and -EINVAL on error. This will allow callers of
> > qemu_config_parse() to check if something was really loaded from
> > the config file.
> >
> > All existing callers of qemu_config_parse() and
> > qemu_read_config_file() only check if the return value was
> > negative, so the change shouldn't affect them.
>
> Two of them:
>
> * read_config() maps negative value to -EINVAL. Callers:
>
> - blkdebug_open() passes it on. As a .bdrv_file_open() method, it's
> supposed to return -errno on failure. Good.
>
> * qemu_read_config_file() maps non-zero value to -EINVAL. Callers:
>
> - qemu_read_default_config_file() maps -EINVAL to zero. WTF?
qemu_read_config_file() maps -ENOENT to zero, not -EINVAL. It
will ignore the error only if the default config file doesn't
exist (if fopen() sets errno to ENOENT).
>
> - main() passes sterror(EINVAL) to error_report(). Good.
>
> Also: qemu_config_parse() reports errors with error_report(). Let's
> have another look at its callers:
>
> * read_config() has an Error ** parameter. Bad. Care to convert the
> sucker to Error?
I can do it in a separate patch, unless you believe it must be
done by this patch patch in a single step.
>
> * qemu_read_config_file() doesn't report errors. Callers:
>
> - qemu_read_default_config_file() doesn't report errors. Its called
> by main(), and ...
This means fopen() errors won't get reported. Good catch. I will
send a separate fix.
>
> - main() reports with error_report(). Good.
>
[...]
>
> I think this mapping to -EINVAL is also superfluous now:
>
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 6117ce5..fbefa9e 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -252,7 +252,6 @@ static int read_config(BDRVBlkdebugState *s, const char
> *filename,
> ret = qemu_config_parse(f, config_groups, filename);
> if (ret < 0) {
> error_setg(errp, "Could not parse blkdebug config file");
> - ret = -EINVAL;
> goto fail;
Good catch. I will squash it into the patch.
--
Eduardo