| Message ID | 1422387983-32153-18-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 01/27/2015 12:45 PM, Max Reitz wrote: > blk_by_name() may return a BlockBackend for which blk_bs() returns NULL. > In this case, an error should be returned (instead of just returning > NULL without modifying *errp). > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block.c | 5 +++++ > 1 file changed, 5 insertions(+) > Reviewed-by: Eric Blake <eblake@redhat.com>
On 01/27/2015 02:45 PM, Max Reitz wrote: > blk_by_name() may return a BlockBackend for which blk_bs() returns NULL. > In this case, an error should be returned (instead of just returning > NULL without modifying *errp). > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/block.c b/block.c > index 9a0a510..b7e631c 100644 > --- a/block.c > +++ b/block.c > @@ -3718,6 +3718,11 @@ BlockDriverState *bdrv_lookup_bs(const char *device, > blk = blk_by_name(device); > > if (blk) { > + if (!blk_bs(blk)) { > + error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device); > + return NULL; > + } > + > return blk_bs(blk); > } > } > Do we have a consensus on The One True And Proper Way to report errors? I know Markus usually campaigns against error_set() in favor of error_setg().
On 2015-01-28 at 13:31, John Snow wrote: > > > On 01/27/2015 02:45 PM, Max Reitz wrote: >> blk_by_name() may return a BlockBackend for which blk_bs() returns NULL. >> In this case, an error should be returned (instead of just returning >> NULL without modifying *errp). >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/block.c b/block.c >> index 9a0a510..b7e631c 100644 >> --- a/block.c >> +++ b/block.c >> @@ -3718,6 +3718,11 @@ BlockDriverState *bdrv_lookup_bs(const char >> *device, >> blk = blk_by_name(device); >> >> if (blk) { >> + if (!blk_bs(blk)) { >> + error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device); >> + return NULL; >> + } >> + >> return blk_bs(blk); >> } >> } >> > > Do we have a consensus on The One True And Proper Way to report > errors? I know Markus usually campaigns against error_set() in favor > of error_setg(). The wiki says to use error_setg(). However, QERR_DEVICE_HAS_NO_MEDIUM is at least a generic error (ERROR_CLASS_GENERIC_ERROR) and I somehow like using a macro for such commonplace errors more. I don't know the real problem with error_set() with macro, though. I can only imagine that if we use macros, people may start to rely on the human-readable error string whereas they should not. If I would not use a macro here, I would've written my own error message which might have differed from QERR_DEVICE_HAS_NO_MEDIUM and thus increased diversity. But I don't feel like that's enough of a reason... I'm most probably just missing something about the evilness of error_set() with macros. Max
diff --git a/block.c b/block.c index 9a0a510..b7e631c 100644 --- a/block.c +++ b/block.c @@ -3718,6 +3718,11 @@ BlockDriverState *bdrv_lookup_bs(const char *device, blk = blk_by_name(device); if (blk) { + if (!blk_bs(blk)) { + error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device); + return NULL; + } + return blk_bs(blk); } }
blk_by_name() may return a BlockBackend for which blk_bs() returns NULL. In this case, an error should be returned (instead of just returning NULL without modifying *errp). Signed-off-by: Max Reitz <mreitz@redhat.com> --- block.c | 5 +++++ 1 file changed, 5 insertions(+)