Message ID | 87bmtna50o.fsf@dusky.pond.sub.org |
---|---|
State | New |
Headers | show |
On Mon, Feb 27, 2017 at 06:59:51PM +0100, Markus Armbruster wrote: > Uh, I totally forgot about this series. My apologies... > > Eduardo Habkost <ehabkost@redhat.com> 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 Habkost <ehabkost@redhat.com> writes: > On Mon, Feb 27, 2017 at 06:59:51PM +0100, Markus Armbruster wrote: >> Uh, I totally forgot about this series. My apologies... >> >> Eduardo Habkost <ehabkost@redhat.com> 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). Crosseyed reviewer syndrome... >> - 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. It *should* be a separate patch. >> >> * 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. Looking forward to v2 :)
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; } }