Re: [Qemu-devel] [PATCH RESEND 17/50] block: Respect empty BB in bdrv_lookup_bs()

[Max Reitz]

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