Re: [Qemu-block] [RFC PATCH 1/7] block/qdev: Allow node name for drive properties

[Kevin Wolf]

Am 24.06.2016 um 19:35 hat Eric Blake geschrieben:
> On 06/23/2016 08:36 AM, Kevin Wolf wrote:
> > If a node name instead of a BlockBackend name is specified as the driver
> > for a guest device, an anonymous BlockBackend is created now.
> > 
> > Signed-off-by: Kevin Wolf <address@hidden>
> > ---
> >  hw/core/qdev-properties-system.c | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/core/qdev-properties-system.c 
> > b/hw/core/qdev-properties-system.c
> > index df38b8a..c5cc9cf 100644
> > --- a/hw/core/qdev-properties-system.c
> > +++ b/hw/core/qdev-properties-system.c
> > @@ -72,9 +72,18 @@ static void parse_drive(DeviceState *dev, const char 
> > *str, void **ptr,
> >                          const char *propname, Error **errp)
> >  {
> >      BlockBackend *blk;
> > +    bool blk_created = false;
> >  
> >      blk = blk_by_name(str);
> >      if (!blk) {
> > +        BlockDriverState *bs = bdrv_lookup_bs(NULL, str, NULL);
> 
> So BB name takes priority, but if one is not found, you try a BDS lookup
> (node name) and create an anonymous BB to match.  Seems okay.

Well, there's no real priority here because BBs and BDSes share the same
namespace, so it can only be one or the other. I just can't pass both to
bdrv_lookup_bs() because I need the BB and not just its root BDS if it's
a BB name, so I have to check one after the other.

> > +        if (bs) {
> > +            blk = blk_new();
> > +            blk_insert_bs(blk, bs);
> > +            blk_created = true;
> > +        }
> > +    }
> > +    if (!blk) {
> >          error_setg(errp, "Property '%s.%s' can't find value '%s'",
> >                     object_get_typename(OBJECT(dev)), propname, str);
> >          return;
> 
> This return is safe, but looks a bit odd...
> 
> > @@ -91,9 +100,16 @@ static void parse_drive(DeviceState *dev, const char 
> > *str, void **ptr,
> >              error_setg(errp, "Drive '%s' is already in use by another 
> > device",
> >                         str);
> >          }
> > -        return;
> > +        goto fail;
> 
> ...given that you had to convert this return to a goto.

Hm, okay. I think it's pretty common to have early error paths return
directly and later ones use goto. But blk_unref(NULL) is allowed, so in
theory I could change that.

Kevin

> >      }
> > +
> >      *ptr = blk;
> > +
> > +fail:
> > +    if (blk_created) {
> > +        /* If we need to keep a reference, blk_attach_dev() took it */
> > +        blk_unref(blk);
> > +    }
> >  }
> >  
> >  static void release_drive(Object *obj, const char *name, void *opaque)

pgpfhSiw26SEq.pgp
Description: PGP signature