Eduardo Habkost <ehabk...@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 <ehabk...@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 :)
