[v3,1/6] block/qdev: Allow node name for drive properties

Commit Message

Kevin Wolf July 6, 2016, 2:45 p.m. UTC

If a node name instead of a BlockBackend name is specified as the driver
for a guest device, an anonymous BlockBackend is created now.

usb-storage uses a hack where it forwards its BlockBackend as a property
to another device that it internally creates. This hack must be updated
so that it doesn't drop its original BB before it can be passed to the
other device. This used to work because we always had the monitor
reference around, but with node-names the device reference is the only
one now.

Signed-off-by: Kevin Wolf <[e-mail]>
---
 hw/core/qdev-properties-system.c | 37 ++++++++++++++++++++++++++++++++-----
 hw/usb/dev-storage.c             |  5 ++++-
 2 files changed, 36 insertions(+), 6 deletions(-)

Comments

Max Reitz July 8, 2016, 3:15 p.m. UTC | #1

On 06.07.2016 16:45, 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.
> 
> usb-storage uses a hack where it forwards its BlockBackend as a property
> to another device that it internally creates. This hack must be updated
> so that it doesn't drop its original BB before it can be passed to the
> other device. This used to work because we always had the monitor
> reference around, but with node-names the device reference is the only
> one now.
> 
> Signed-off-by: Kevin Wolf <[e-mail]>
> ---
>  hw/core/qdev-properties-system.c | 37 ++++++++++++++++++++++++++++++++-----
>  hw/usb/dev-storage.c             |  5 ++++-
>  2 files changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index df38b8a..615c191 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c

[...]

> @@ -362,8 +378,19 @@ PropertyInfo qdev_prop_vlan = {
>  void qdev_prop_set_drive(DeviceState *dev, const char *name,
>                           BlockBackend *value, Error **errp)
>  {
> -    object_property_set_str(OBJECT(dev), value ? blk_name(value) : "",
> -                            name, errp);
> +    const char *ref = NULL;

This should be "", otherwise strlen() called by qstring_from_str()
called by object_property_set_str() will probably segfault.

With that fixed:

Reviewed-by: Max Reitz <[e-mail]>

> +
> +    if (value) {
> +        ref = blk_name(value);
> +        if (!*ref) {
> +            BlockDriverState *bs = blk_bs(value);
> +            if (bs) {
> +                ref = bdrv_get_node_name(bs);
> +            }
> +        }
> +    }
> +
> +    object_property_set_str(OBJECT(dev), ref, name, errp);
>  }
>  
>  void qdev_prop_set_chr(DeviceState *dev, const char *name,

Patch

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index df38b8a..615c191 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -72,12 +72,21 @@  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;
+        goto fail;
     }
     if (blk_attach_dev(blk, dev) < 0) {
         DriveInfo *dinfo = blk_legacy_dinfo(blk);
@@ -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)
@@ -127,7 +143,7 @@  static void set_drive(Object *obj, Visitor *v, const char *name, void *opaque,
 
 PropertyInfo qdev_prop_drive = {
     .name  = "str",
-    .description = "ID of a drive to use as a backend",
+    .description = "Node name or ID of a block device to use as a backend",
     .get   = get_drive,
     .set   = set_drive,
     .release = release_drive,
@@ -362,8 +378,19 @@  PropertyInfo qdev_prop_vlan = {
 void qdev_prop_set_drive(DeviceState *dev, const char *name,
                          BlockBackend *value, Error **errp)
 {
-    object_property_set_str(OBJECT(dev), value ? blk_name(value) : "",
-                            name, errp);
+    const char *ref = NULL;
+
+    if (value) {
+        ref = blk_name(value);
+        if (!*ref) {
+            BlockDriverState *bs = blk_bs(value);
+            if (bs) {
+                ref = bdrv_get_node_name(bs);
+            }
+        }
+    }
+
+    object_property_set_str(OBJECT(dev), ref, name, errp);
 }
 
 void qdev_prop_set_chr(DeviceState *dev, const char *name,
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 4d605b8..78038a2 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -609,10 +609,12 @@  static void usb_msd_realize_storage(USBDevice *dev, Error **errp)
      * a SCSI bus that can serve only a single device, which it
      * creates automatically.  But first it needs to detach from its
      * blockdev, or else scsi_bus_legacy_add_drive() dies when it
-     * attaches again.
+     * attaches again. We also need to take another reference so that
+     * blk_detach_dev() doesn't free blk while we still need it.
      *
      * The hack is probably a bad idea.
      */
+    blk_ref(blk);
     blk_detach_dev(blk, &s->dev.qdev);
     s->conf.blk = NULL;
 
@@ -623,6 +625,7 @@  static void usb_msd_realize_storage(USBDevice *dev, Error **errp)
     scsi_dev = scsi_bus_legacy_add_drive(&s->bus, blk, 0, !!s->removable,
                                          s->conf.bootindex, dev->serial,
                                          &err);
+    blk_unref(blk);
     if (!scsi_dev) {
         error_propagate(errp, err);
         return;