Re: [Qemu-devel] [PATCH v2 1/7] blkdebug: Use errp for read_config()

[Max Reitz <mreitz@redhat.com>]

On 25.11.2013 14:40, Kevin Wolf wrote:
> Am 22.11.2013 um 17:10 hat Max Reitz geschrieben:
>> Use an Error variable in the read_config() function.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/blkdebug.c | 12 ++++++++----
>>   1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/block/blkdebug.c b/block/blkdebug.c
>> index 16d2b91..9dfa712 100644
>> --- a/block/blkdebug.c
>> +++ b/block/blkdebug.c
>> @@ -271,7 +271,7 @@ static void remove_rule(BlkdebugRule *rule)
>>       g_free(rule);
>>   }
>>   
>> -static int read_config(BDRVBlkdebugState *s, const char *filename)
>> +static int read_config(BDRVBlkdebugState *s, const char *filename, Error **errp)
> For a complete conversion, this should become a void function.

I wanted to do this at first, but blkdebug_open needs to return -errno 
again; in the hunk below, -errno from fopen is returned. We could just 
return some dummy value in blkdebug_open again, but I thought it better 
to pass the correct value.

>>   {
>>       FILE *f;
>>       int ret;
>> @@ -279,11 +279,16 @@ static int read_config(BDRVBlkdebugState *s, const char *filename)
>>   
>>       f = fopen(filename, "r");
>>       if (f == NULL) {
>> +        error_setg_errno(errp, errno, "Could not read blkdebug config file "
>> +                         "'%s'", filename);
>>           return -errno;
>>       }
>>   
>>       ret = qemu_config_parse(f, config_groups, filename);
>>       if (ret < 0) {
>> +        error_setg(errp, "Could not parse blkdebug config file '%s'",
>> +                   filename);
>> +        ret = -EINVAL;
>>           goto fail;
>>       }
> Any reason for adding the file name here without mentioning it in the
> commit message?

No, not really. I don't know why I added this, the original 
error_setg_errno() in blkdebug_open didn't have it, so…

> I'm not completely sure here, but the information is probably redundant;
> the caller already knows what the config file was. On the command line,
> this will lead to error messages such as:
>
> qemu: -drive file=blkdebug:/path/to/blkdebug.cfg:foo.qcow2: Could not
> read blkdebug config file '/path/to/blkdebug.cfg': No such file or
> directory

Personally, I like such redundant information, but I guess I still need 
to learn that qemu sometimes differs from my preferences. ;-)

>> @@ -370,9 +375,8 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
>>       /* Read rules from config file */
>>       config = qemu_opt_get(opts, "config");
>>       if (config) {
>> -        ret = read_config(s, config);
>> -        if (ret < 0) {
>> -            error_setg_errno(errp, -ret, "Could not read blkdebug config file");
>> +        ret = read_config(s, config, errp);
>> +        if (ret) {
>>               goto fail;
>>           }
>>       }
> Hm, I see. You actually need to return -errno for bdrv_open(). Then
> let's just check if we really want to add the file name in the message.

If you don't see the point, I do not either. The original 
error_setg_errno() didn't have it, so we should leave it that way.

Max