| Message ID | 1466692592-9551-2-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <kwolf@redhat.com> > --- > 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. > + 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. > } > + > *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) >
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 <kwolf@redhat.com> > > --- > > 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)
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); + 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; @@ -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; } + *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)
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 <kwolf@redhat.com> --- hw/core/qdev-properties-system.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)