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

[Max Reitz]

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 <address@hidden>
> > ---
> >   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