Message ID | 1385136658-16347-2-git-send-email-mreitz@redhat.com |
---|---|
State | New |
Headers | show |
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. > { > 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? 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 > @@ -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. Kevin
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
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) { 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; } @@ -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; } }
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(-)