[Qemu-devel] [PATCH 0/4] block: Improve warnings for doubly-connected drives

[Qemu-devel] [PATCH 0/4] block: Improve warnings for doubly-connected drives

[Peter Maydell]

This patchset attempts to improve the warning and error messages for
bad user command lines that attempt to connect a drive up to two
devices. The motivation here is patch #4, which changes the default
interface for the virt board to virtio. That will break some existing
command lines which forgot to specify if=none, and so I would like
us to at least diagnose that user error in a helpful way that points
the user towards adding the missing if=none.

The patchset improves some error messages, and makes some previously
undiagnosed mistakes into warnings. The changes (with sample x86
command lines to provoke them) are:

(1) Drive specified as to be auto-connected and also manually connected
(and the board does handle this if= type):

  qemu-system-x86_64 -nodefaults -display none -drive if=scsi,file=tmp.qcow2,id=foo -device ide-hd,drive=foo

Previously: an error:
  qemu-system-x86_64: -device ide-hd,drive=foo: Property 'ide-hd.drive'
  can't take value 'foo', it's in use

Now: a better error:
  qemu-system-x86_64: -device ide-hd,drive=foo: Property 'ide-hd.drive'
  can't be set to drive ID 'foo'; that drive has been automatically
  connected to another device. Use if=none if you do not want that
  automatic connection.

(2) As 1, but the board does not handle this if= type:

Previously: not diagnosed at all

Now: a warning:
  Warning: automatic connection of this drive requested (because if=sd
  was specified) but it was also connected manually to a device:
  id=foo,file=tmp.qcow2,if=sd,bus=0,unit=0
  (If you don't want this drive auto-connected, use if=none.)

[This means we now will always warn one way or another about drives which
have an if= auto-connect specified but which the board didn't pick up: either
they're also manually connected and get this warning, or they're not manually
connected, and get the orphan-drive warning. If the if= was due to the
board default rather than the user typing it specifically, the error message
text is slightly different to reflect that.]

(3) Drive specified to be manually connected in two different ways:

  qemu-system-x86_64 -nodefaults -display none -drive if=sd,file=tmp.qcow2,id=foo -device ide-hd,drive=foo -device ide-hd,drive=foo

Previously: an error:
  qemu-system-x86_64: -device ide-hd,drive=foo: Property 'ide-hd.drive'
  can't take value 'foo', it's in use

Now: a better error:
  qemu-system-x86_64: -device ide-hd,drive=foo: Property 'ide-hd.drive'
  can't be set to drive ID 'foo'; that drive has already been connected
  to another device.



In order to detect when a drive was auto-connected, we need to set a
flag in the DriveInfo when this happens.  we do this by assuming that
all calls to blk_by_legacy_dinfo() imply that we're about to assign
the drive to a device.  This is a slightly ugly place to make the
test, but simpler than trying to locate and change every place in the
code that does automatic drive handling, and the worst case is that
we might print out a spurious warning.


I include patch #4 as the motivation/context but in fact it doesn't
depend on the first 3, so if you want to take the first 3 via
block and have me put the 4th one in target-arm that's OK.

thanks
-- PMM


Peter Maydell (4):
  block: Warn if an if=<something> drive was also connected manually
  qdev-properties-system: Change set_pointer's parse callback to use
    Error
  qdev-properties-system: Improve error message for drive assignment
    conflict
  hw/arm/virt: Make block devices default to virtio

 block/block-backend.c            |  4 ++++
 blockdev.c                       | 39 ++++++++++++++++++++++++++++++++++
 hw/arm/virt.c                    |  2 ++
 hw/core/qdev-properties-system.c | 45 ++++++++++++++++++++++++++++------------
 include/sysemu/blockdev.h        |  2 ++
 5 files changed, 79 insertions(+), 13 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/4] block: Warn if an if=<something> drive was also connected manually

[Peter Maydell]

Improve the diagnosis of command line errors where the user requested
an automatic connection of a drive (via if=<something>, or by not
setting if= and using the board-default-if). We already fail this
case if the board actually handles if=<something>, but if the board
did not auto-connect the drive we should at least warn about the
problem, since the most likely reason is a forgotten if=none, and
this command line might become an error if the board starts handling
this if= type in future.

To do this we need to identify when a drive is automatically
connected by the board; we do this by assuming that all calls
to blk_by_legacy_dinfo() imply that we're about to assign the
drive to a device. This is a slightly ugly place to make the
test, but simpler than trying to locate and change every place
in the code that does automatic drive handling, and the worst
case is that we might print out a spurious warning.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 block/block-backend.c     |  4 ++++
 blockdev.c                | 39 +++++++++++++++++++++++++++++++++++++++
 include/sysemu/blockdev.h |  2 ++
 3 files changed, 45 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 93e46f3..a45c207 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -260,6 +260,9 @@ DriveInfo *blk_set_legacy_dinfo(BlockBackend *blk, DriveInfo *dinfo)
 /*
  * Return the BlockBackend with DriveInfo @dinfo.
  * It must exist.
+ * For the purposes of providing helpful error messages, we assume
+ * that any call to this function implies that the drive is going
+ * to be automatically claimed by the board model.
  */
 BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo)
 {
@@ -267,6 +270,7 @@ BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo)
 
     QTAILQ_FOREACH(blk, &blk_backends, link) {
         if (blk->legacy_dinfo == dinfo) {
+            dinfo->auto_claimed = true;
             return blk;
         }
     }
diff --git a/blockdev.c b/blockdev.c
index de94a8b..97a56b9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -230,6 +230,32 @@ bool drive_check_orphaned(void)
                     dinfo->bus, dinfo->unit);
             rs = true;
         }
+        if (blk_get_attached_dev(blk) && dinfo->type != IF_NONE &&
+            !dinfo->auto_claimed) {
+            /* This drive is attached to something, but it was specified
+             * with if=<something> and it's not attached because it was
+             * automatically claimed by the board code because of the if=
+             * specification. The user probably forgot an if=none.
+             */
+            fprintf(stderr,
+                    "Warning: automatic connection of this drive requested ");
+            if (dinfo->type_is_board_default) {
+                fprintf(stderr, "(because if= was not specified and this "
+                        "machine defaults to if=%s) ",
+                        if_name[dinfo->type]);
+            } else {
+                fprintf(stderr, "(because if=%s was specified) ",
+                        if_name[dinfo->type]);
+            }
+            fprintf(stderr,
+                    "but it was also connected manually to a device: "
+                    "id=%s,file=%s,if=%s,bus=%d,unit=%d\n"
+                    "(If you don't want this drive auto-connected, "
+                    "use if=none.)\n",
+                    blk_name(blk), blk_bs(blk)->filename, if_name[dinfo->type],
+                    dinfo->bus, dinfo->unit);
+            rs = true;
+        }
     }
 
     return rs;
@@ -683,6 +709,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
     const char *werror, *rerror;
     bool read_only = false;
     bool copy_on_read;
+    bool type_is_board_default = false;
     const char *serial;
     const char *filename;
     Error *local_err = NULL;
@@ -808,6 +835,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
         }
     } else {
         type = block_default_type;
+        type_is_board_default = true;
     }
 
     /* Geometry */
@@ -994,6 +1022,17 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
     dinfo->devaddr = devaddr;
     dinfo->serial = g_strdup(serial);
 
+    if (type == IF_VIRTIO) {
+        /* We created the automatic device earlier. For other types this
+         * will be set to true at the point where the drive is claimed
+         * by the IDE/SCSI/etc bus, when that code calls blk_by_legacy_dinfo()
+         * to find the block backend from the drive.
+         */
+        dinfo->auto_claimed = true;
+    }
+
+    dinfo->type_is_board_default = type_is_board_default;
+
     blk_set_legacy_dinfo(blk, dinfo);
 
     switch(type) {
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 3104150..f9c44e2 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -36,6 +36,8 @@ struct DriveInfo {
     int unit;
     int auto_del;               /* see blockdev_mark_auto_del() */
     bool is_default;            /* Added by default_drive() ?  */
+    bool auto_claimed;          /* Automatically claimed by board model? */
+    bool type_is_board_default; /* type is from board default, not user 'if=' */
     int media_cd;
     int cyls, heads, secs, trans;
     QemuOpts *opts;
-- 
1.9.1

[Qemu-devel] [PATCH 2/4] qdev-properties-system: Change set_pointer's parse callback to use Error

[Peter Maydell]

Instead of having set_pointer() call a parse callback which returns
an error number that we then convert to an Error string with
error_set_from_qdev_prop_error(), make the parse callback take an
Error** and set the error itself. This will allow parse routines
to provide more helpful error messages than the generic ones.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/core/qdev-properties-system.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 0309fe5..56954b4 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -35,15 +35,15 @@ static void get_pointer(Object *obj, Visitor *v, Property *prop,
 }
 
 static void set_pointer(Object *obj, Visitor *v, Property *prop,
-                        int (*parse)(DeviceState *dev, const char *str,
-                                     void **ptr),
+                        void (*parse)(DeviceState *dev, const char *str,
+                                      void **ptr, const char *propname,
+                                      Error **errp),
                         const char *name, Error **errp)
 {
     DeviceState *dev = DEVICE(obj);
     Error *local_err = NULL;
     void **ptr = qdev_get_prop_ptr(dev, prop);
     char *str;
-    int ret;
 
     if (dev->realized) {
         qdev_prop_set_after_realize(dev, name, errp);
@@ -60,26 +60,29 @@ static void set_pointer(Object *obj, Visitor *v, Property *prop,
         *ptr = NULL;
         return;
     }
-    ret = parse(dev, str, ptr);
-    error_set_from_qdev_prop_error(errp, ret, dev, prop, str);
+    parse(dev, str, ptr, prop->name, errp);
     g_free(str);
 }
 
 /* --- drive --- */
 
-static int parse_drive(DeviceState *dev, const char *str, void **ptr)
+static void parse_drive(DeviceState *dev, const char *str, void **ptr,
+                        const char *propname, Error **errp)
 {
     BlockBackend *blk;
 
     blk = blk_by_name(str);
     if (!blk) {
-        return -ENOENT;
+        error_setg(errp, "Property '%s.%s' can't find value '%s'",
+                   object_get_typename(OBJECT(dev)), propname, str);
+        return;
     }
     if (blk_attach_dev(blk, dev) < 0) {
-        return -EEXIST;
+        error_setg(errp, "Property '%s.%s' can't take value '%s', it's in use",
+                  object_get_typename(OBJECT(dev)), propname, str);
+        return;
     }
     *ptr = blk;
-    return 0;
 }
 
 static void release_drive(Object *obj, const char *name, void *opaque)
@@ -121,17 +124,21 @@ PropertyInfo qdev_prop_drive = {
 
 /* --- character device --- */
 
-static int parse_chr(DeviceState *dev, const char *str, void **ptr)
+static void parse_chr(DeviceState *dev, const char *str, void **ptr,
+                      const char *propname, Error **errp)
 {
     CharDriverState *chr = qemu_chr_find(str);
     if (chr == NULL) {
-        return -ENOENT;
+        error_setg(errp, "Property '%s.%s' can't find value '%s'",
+                   object_get_typename(OBJECT(dev)), propname, str);
+        return;
     }
     if (qemu_chr_fe_claim(chr) != 0) {
-        return -EEXIST;
+        error_setg(errp, "Property '%s.%s' can't take value '%s', it's in use",
+                  object_get_typename(OBJECT(dev)), propname, str);
+        return;
     }
     *ptr = chr;
-    return 0;
 }
 
 static void release_chr(Object *obj, const char *name, void *opaque)
-- 
1.9.1

[Qemu-devel] [PATCH 3/4] qdev-properties-system: Improve error message for drive assignment conflict

[Peter Maydell]

If the user forgot if=none on their drive specification they're likely
to get an error message because the drive is assigned once automatically
by QEMU and once by the manual id=/drive= user command line specification.
Improve the error message produced in this case to explicitly guide the
user towards if=none.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/core/qdev-properties-system.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 56954b4..bde9e12 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -78,8 +78,20 @@ static void parse_drive(DeviceState *dev, const char *str, void **ptr,
         return;
     }
     if (blk_attach_dev(blk, dev) < 0) {
-        error_setg(errp, "Property '%s.%s' can't take value '%s', it's in use",
-                  object_get_typename(OBJECT(dev)), propname, str);
+        DriveInfo *dinfo = blk_legacy_dinfo(blk);
+
+        if (dinfo->auto_claimed) {
+            error_setg(errp, "Property '%s.%s' can't be set to drive ID '%s'; "
+                       "that drive has been automatically connected to another "
+                       "device. Use if=none if you do not want that automatic "
+                       "connection.",
+                       object_get_typename(OBJECT(dev)), propname, str);
+        } else {
+            error_setg(errp, "Property '%s.%s' can't be set to drive ID '%s'; "
+                       "that drive has already been connected to another "
+                       "device.",
+                       object_get_typename(OBJECT(dev)), propname, str);
+        }
         return;
     }
     *ptr = blk;
-- 
1.9.1

[Qemu-devel] [PATCH 4/4] hw/arm/virt: Make block devices default to virtio

[Peter Maydell]

Now we have virtio-pci, we can make the virt board's default block
device type be IF_VIRTIO. This allows users to use simplified
command lines that don't have to explicitly create virtio-pci-blk
devices; the -hda &c very short options now also work.

This means we also need to set no_cdrom to avoid getting a
default cdrom device -- this is needed because the virtio-blk
device will fail if it is connected to a block backend with
no media, which is what the default cdrom device typically is.
Providing a cdrom with media via -cdrom will still work.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/virt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 1b1cc71..c42ca32 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -961,6 +961,8 @@ static void virt_class_init(ObjectClass *oc, void *data)
     mc->init = machvirt_init;
     mc->max_cpus = 8;
     mc->has_dynamic_sysbus = true;
+    mc->block_default_type = IF_VIRTIO;
+    mc->no_cdrom = 1;
 }
 
 static const TypeInfo machvirt_info = {
-- 
1.9.1

Re: [Qemu-devel] [PATCH 0/4] block: Improve warnings for doubly-connected drives

[Peter Maydell]

Ping?

thanks
-- PMM

On 12 June 2015 at 14:26, Peter Maydell <peter.maydell@linaro.org> wrote:
> This patchset attempts to improve the warning and error messages for
> bad user command lines that attempt to connect a drive up to two
> devices. The motivation here is patch #4, which changes the default
> interface for the virt board to virtio. That will break some existing
> command lines which forgot to specify if=none, and so I would like
> us to at least diagnose that user error in a helpful way that points
> the user towards adding the missing if=none.
>
> The patchset improves some error messages, and makes some previously
> undiagnosed mistakes into warnings. The changes (with sample x86
> command lines to provoke them) are:
>
> (1) Drive specified as to be auto-connected and also manually connected
> (and the board does handle this if= type):
>
>   qemu-system-x86_64 -nodefaults -display none -drive if=scsi,file=tmp.qcow2,id=foo -device ide-hd,drive=foo
>
> Previously: an error:
>   qemu-system-x86_64: -device ide-hd,drive=foo: Property 'ide-hd.drive'
>   can't take value 'foo', it's in use
>
> Now: a better error:
>   qemu-system-x86_64: -device ide-hd,drive=foo: Property 'ide-hd.drive'
>   can't be set to drive ID 'foo'; that drive has been automatically
>   connected to another device. Use if=none if you do not want that
>   automatic connection.
>
> (2) As 1, but the board does not handle this if= type:
>
> Previously: not diagnosed at all
>
> Now: a warning:
>   Warning: automatic connection of this drive requested (because if=sd
>   was specified) but it was also connected manually to a device:
>   id=foo,file=tmp.qcow2,if=sd,bus=0,unit=0
>   (If you don't want this drive auto-connected, use if=none.)
>
> [This means we now will always warn one way or another about drives which
> have an if= auto-connect specified but which the board didn't pick up: either
> they're also manually connected and get this warning, or they're not manually
> connected, and get the orphan-drive warning. If the if= was due to the
> board default rather than the user typing it specifically, the error message
> text is slightly different to reflect that.]
>
> (3) Drive specified to be manually connected in two different ways:
>
>   qemu-system-x86_64 -nodefaults -display none -drive if=sd,file=tmp.qcow2,id=foo -device ide-hd,drive=foo -device ide-hd,drive=foo
>
> Previously: an error:
>   qemu-system-x86_64: -device ide-hd,drive=foo: Property 'ide-hd.drive'
>   can't take value 'foo', it's in use
>
> Now: a better error:
>   qemu-system-x86_64: -device ide-hd,drive=foo: Property 'ide-hd.drive'
>   can't be set to drive ID 'foo'; that drive has already been connected
>   to another device.
>
>
>
> In order to detect when a drive was auto-connected, we need to set a
> flag in the DriveInfo when this happens.  we do this by assuming that
> all calls to blk_by_legacy_dinfo() imply that we're about to assign
> the drive to a device.  This is a slightly ugly place to make the
> test, but simpler than trying to locate and change every place in the
> code that does automatic drive handling, and the worst case is that
> we might print out a spurious warning.
>
>
> I include patch #4 as the motivation/context but in fact it doesn't
> depend on the first 3, so if you want to take the first 3 via
> block and have me put the 4th one in target-arm that's OK.
>
> thanks
> -- PMM
>
>
> Peter Maydell (4):
>   block: Warn if an if=<something> drive was also connected manually
>   qdev-properties-system: Change set_pointer's parse callback to use
>     Error
>   qdev-properties-system: Improve error message for drive assignment
>     conflict
>   hw/arm/virt: Make block devices default to virtio
>
>  block/block-backend.c            |  4 ++++
>  blockdev.c                       | 39 ++++++++++++++++++++++++++++++++++
>  hw/arm/virt.c                    |  2 ++
>  hw/core/qdev-properties-system.c | 45 ++++++++++++++++++++++++++++------------
>  include/sysemu/blockdev.h        |  2 ++
>  5 files changed, 79 insertions(+), 13 deletions(-)

Re: [Qemu-devel] [PATCH 3/4] qdev-properties-system: Improve error message for drive assignment conflict

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> If the user forgot if=none on their drive specification they're likely
> to get an error message because the drive is assigned once automatically
> by QEMU and once by the manual id=/drive= user command line specification.
> Improve the error message produced in this case to explicitly guide the
> user towards if=none.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/core/qdev-properties-system.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index 56954b4..bde9e12 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -78,8 +78,20 @@ static void parse_drive(DeviceState *dev, const char *str, void **ptr,
>          return;
>      }
>      if (blk_attach_dev(blk, dev) < 0) {
> -        error_setg(errp, "Property '%s.%s' can't take value '%s', it's in use",
> -                  object_get_typename(OBJECT(dev)), propname, str);
> +        DriveInfo *dinfo = blk_legacy_dinfo(blk);
> +
> +        if (dinfo->auto_claimed) {
> +            error_setg(errp, "Property '%s.%s' can't be set to drive ID '%s'; "
> +                       "that drive has been automatically connected to another "
> +                       "device. Use if=none if you do not want that automatic "
> +                       "connection.",
> +                       object_get_typename(OBJECT(dev)), propname, str);
> +        } else {
> +            error_setg(errp, "Property '%s.%s' can't be set to drive ID '%s'; "
> +                       "that drive has already been connected to another "
> +                       "device.",
> +                       object_get_typename(OBJECT(dev)), propname, str);
> +        }
>          return;
>      }
>      *ptr = blk;

We generally do not end error messages with a period.

The message for auto_claimed drives is of the form

    LOCATION: This went wrong.  Advice on how to fix it.

All in one awfully long line.  A friendler format is

    LOCATION: This went wrong
    Advice on how to fix it.

Unfortunately, the Error API doesn't support that.

Easy with error_report():

    error_report("This went wrong");
    error_printf("Advice on how to fix it.");

With soon-to-be-gone qerror_report():

    qerror_report(ERROR_CLASS_GENERIC_ERROR, "This went wrong");
    error_printf_unless_qmp("Advice on how to fix it.");

I think we should just bite the bullet and extend Error to support
additional helpful information for humans.  See also
http://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg02482.html
and commit 7216ae3 "qemu-option: Disable two helpful messages that got
broken recently".

Re: [Qemu-devel] [PATCH 2/4] qdev-properties-system: Change set_pointer's parse callback to use Error

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> Instead of having set_pointer() call a parse callback which returns
> an error number that we then convert to an Error string with
> error_set_from_qdev_prop_error(), make the parse callback take an
> Error** and set the error itself. This will allow parse routines
> to provide more helpful error messages than the generic ones.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/core/qdev-properties-system.c | 33 ++++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index 0309fe5..56954b4 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -35,15 +35,15 @@ static void get_pointer(Object *obj, Visitor *v, Property *prop,
>  }
>  
>  static void set_pointer(Object *obj, Visitor *v, Property *prop,
> -                        int (*parse)(DeviceState *dev, const char *str,
> -                                     void **ptr),
> +                        void (*parse)(DeviceState *dev, const char *str,
> +                                      void **ptr, const char *propname,
> +                                      Error **errp),
>                          const char *name, Error **errp)
>  {
>      DeviceState *dev = DEVICE(obj);
>      Error *local_err = NULL;
>      void **ptr = qdev_get_prop_ptr(dev, prop);
>      char *str;
> -    int ret;
>  
>      if (dev->realized) {
>          qdev_prop_set_after_realize(dev, name, errp);
> @@ -60,26 +60,29 @@ static void set_pointer(Object *obj, Visitor *v, Property *prop,
>          *ptr = NULL;
>          return;
>      }
> -    ret = parse(dev, str, ptr);
> -    error_set_from_qdev_prop_error(errp, ret, dev, prop, str);
> +    parse(dev, str, ptr, prop->name, errp);
>      g_free(str);
>  }
>  
>  /* --- drive --- */
>  
> -static int parse_drive(DeviceState *dev, const char *str, void **ptr)
> +static void parse_drive(DeviceState *dev, const char *str, void **ptr,
> +                        const char *propname, Error **errp)
>  {
>      BlockBackend *blk;
>  
>      blk = blk_by_name(str);
>      if (!blk) {
> -        return -ENOENT;
> +        error_setg(errp, "Property '%s.%s' can't find value '%s'",
> +                   object_get_typename(OBJECT(dev)), propname, str);
> +        return;
>      }
>      if (blk_attach_dev(blk, dev) < 0) {
> -        return -EEXIST;
> +        error_setg(errp, "Property '%s.%s' can't take value '%s', it's in use",
> +                  object_get_typename(OBJECT(dev)), propname, str);
> +        return;
>      }
>      *ptr = blk;
> -    return 0;
>  }
>  
>  static void release_drive(Object *obj, const char *name, void *opaque)
> @@ -121,17 +124,21 @@ PropertyInfo qdev_prop_drive = {
>  
>  /* --- character device --- */
>  
> -static int parse_chr(DeviceState *dev, const char *str, void **ptr)
> +static void parse_chr(DeviceState *dev, const char *str, void **ptr,
> +                      const char *propname, Error **errp)
>  {
>      CharDriverState *chr = qemu_chr_find(str);
>      if (chr == NULL) {
> -        return -ENOENT;
> +        error_setg(errp, "Property '%s.%s' can't find value '%s'",
> +                   object_get_typename(OBJECT(dev)), propname, str);
> +        return;
>      }
>      if (qemu_chr_fe_claim(chr) != 0) {
> -        return -EEXIST;
> +        error_setg(errp, "Property '%s.%s' can't take value '%s', it's in use",
> +                  object_get_typename(OBJECT(dev)), propname, str);
> +        return;
>      }
>      *ptr = chr;
> -    return 0;
>  }
>  
>  static void release_chr(Object *obj, const char *name, void *opaque)

The error messages are faithfully copied from
error_set_from_qdev_prop_error().  The next patch will improve
parse_drive()'s messages, and that's why you got this patch.

The next patch takes special care of the if=T, T!=none case.  That case
only exists for drives, because only for drives did we mix up old-style
(one option to configure front- and backend) and new-style (one option
to configure backend, -device to configure frontend) configuration.

It also changes the if=none case, from

    Property 'TYPE.PROP' can't take value 'VAL', it's in use

to

    "Property 'TYPE.PROP' can't be set to drive ID 'VAL'; that drive has already been connected to another device."

More explicit.  Rather long, though.

If this change is deemed an improvement for drive=, then it surely is
one for the other backend references, isn't it?  These are parse_chr()
for chardev= (shown above), and set_netdev() for netdev= (not shown).

    Property 'TYPE.PROP' can't take value 'VAL', it's in use

vs.

    Property 'TYPE.PROP' can't take value 'VAL'; that chardev has already been connected to another device.

Same as above, except s/drive/chardev/.  Likewise for netdev.

Other users of error_set_from_qdev_prop_error() remain: set_vlan(),
set_mac(), set_pci_devfn(), set_pci_host_devaddr().  Okay.  I figure the
error message duplication could be avoided by calling
error_set_from_qdev_prop_error() here.  No biggie either way.

Re: [Qemu-devel] [PATCH 1/4] block: Warn if an if=<something> drive was also connected manually

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> Improve the diagnosis of command line errors where the user requested
> an automatic connection of a drive (via if=<something>, or by not
> setting if= and using the board-default-if). We already fail this
> case if the board actually handles if=<something>, but if the board
> did not auto-connect the drive we should at least warn about the
> problem, since the most likely reason is a forgotten if=none, and
> this command line might become an error if the board starts handling
> this if= type in future.
>
> To do this we need to identify when a drive is automatically
> connected by the board; we do this by assuming that all calls
> to blk_by_legacy_dinfo() imply that we're about to assign the
> drive to a device. This is a slightly ugly place to make the
> test, but simpler than trying to locate and change every place
> in the code that does automatic drive handling, and the worst
> case is that we might print out a spurious warning.

Copying what I wrote on an earlier iteration of this idea:

Adding it to blk_by_legacy_dinfo() is sound when it's called exactly for
the block backends the board claims.  We need to show:

(A) It's called for all block backends the board claims

    Plausible enough, because:

    * Boards claim only drives defined with interface types other than
      IF_NONE.

    * Boards find these drives with drive_get() or its wrappers.  They
      all return DriveInfo.

    * To actually connect a frontend, they need to find the DriveInfo's
      BlockBackend, with blk_by_legacy_dinfo().

(B) It's not called for any block backend the board doesn't claim

    Counter-example: hmp_drive_add().  However, that can only run later,
    so we can ignore it.  Still, not exactly inspiring confidence.

What about this instead:

1. When -device creation connects a qdev_prop_drive property to a
backend, fail when the backend has a DriveInfo and the DriveInfo has
type != IF_NONE.  Note: the connection is made in parse_drive().

2. This breaks -drive if=virtio and possibly more.  That's because for
if=virtio, we create input for -device creation that asks to connect
this IF_VIRTIO drive.  To unbreak it, mark the DriveInfo when we create
such input, and make parse_drive() accept backends so marked.  To find
the places that need to mark, examine users of option group "device".  A
quick, sloppy grep shows a bit over a dozen candidates.  Not too bad.

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  block/block-backend.c     |  4 ++++
>  blockdev.c                | 39 +++++++++++++++++++++++++++++++++++++++
>  include/sysemu/blockdev.h |  2 ++
>  3 files changed, 45 insertions(+)
>
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 93e46f3..a45c207 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -260,6 +260,9 @@ DriveInfo *blk_set_legacy_dinfo(BlockBackend *blk, DriveInfo *dinfo)
>  /*
>   * Return the BlockBackend with DriveInfo @dinfo.
>   * It must exist.
> + * For the purposes of providing helpful error messages, we assume
> + * that any call to this function implies that the drive is going
> + * to be automatically claimed by the board model.

As explained above, this is a problematic assumption.  If we decice to
rely on it anyway, we need a scarier comment here, and a /* This
violates the assumption ..., but that's okay, because ... */ next to
calls that violate the assumption, like hmp_drive_add() does.

Can we find a way to check for not-okay violations with assert()?

>   */
>  BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo)
>  {
> @@ -267,6 +270,7 @@ BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo)
>  
>      QTAILQ_FOREACH(blk, &blk_backends, link) {
>          if (blk->legacy_dinfo == dinfo) {
> +            dinfo->auto_claimed = true;
>              return blk;
>          }
>      }
> diff --git a/blockdev.c b/blockdev.c
> index de94a8b..97a56b9 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -230,6 +230,32 @@ bool drive_check_orphaned(void)
>                      dinfo->bus, dinfo->unit);
>              rs = true;
>          }
> +        if (blk_get_attached_dev(blk) && dinfo->type != IF_NONE &&
> +            !dinfo->auto_claimed) {
> +            /* This drive is attached to something, but it was specified
> +             * with if=<something> and it's not attached because it was
> +             * automatically claimed by the board code because of the if=
> +             * specification. The user probably forgot an if=none.
> +             */
> +            fprintf(stderr,
> +                    "Warning: automatic connection of this drive requested ");
> +            if (dinfo->type_is_board_default) {
> +                fprintf(stderr, "(because if= was not specified and this "
> +                        "machine defaults to if=%s) ",
> +                        if_name[dinfo->type]);

In my opinion, a board that specifies a default interface type it
doesn't support is simply broken, and should be fixed.

Even if we fix that, we could still reach this case when the board
claims only *some* of the drives for its default type.  Example:

    $ qemu-system-x86_64 -nodefaults -S -display none -drive if=floppy,file=tmp.qcow2,index=99
    Warning: Orphaned drive without device: id=floppy99,file=tmp.qcow2,if=floppy,bus=0,unit=99

Compare:

    $ qemu-system-x86_64 -nodefaults -S -display none -drive if=ide,file=tmp.qcow2,index=99
    qemu-system-x86_64: Too many IDE buses defined (50 > 2)

Crap shot.

If we have boards that don't claim *any* interface type, we need to give
them a .block_default_type that rejects -drive without an explicit if=.

> +            } else {
> +                fprintf(stderr, "(because if=%s was specified) ",
> +                        if_name[dinfo->type]);
> +            }
> +            fprintf(stderr,
> +                    "but it was also connected manually to a device: "
> +                    "id=%s,file=%s,if=%s,bus=%d,unit=%d\n"
> +                    "(If you don't want this drive auto-connected, "
> +                    "use if=none.)\n",
> +                    blk_name(blk), blk_bs(blk)->filename, if_name[dinfo->type],
> +                    dinfo->bus, dinfo->unit);

Doesn't point the user to the offending -device.  If we detected the
problem right when we connect -device drive=, in parse_drive(), we'd get
that for free.

> +            rs = true;
> +        }
>      }
>  
>      return rs;
> @@ -683,6 +709,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
>      const char *werror, *rerror;
>      bool read_only = false;
>      bool copy_on_read;
> +    bool type_is_board_default = false;
>      const char *serial;
>      const char *filename;
>      Error *local_err = NULL;
> @@ -808,6 +835,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
>          }
>      } else {
>          type = block_default_type;
> +        type_is_board_default = true;
>      }
>  
>      /* Geometry */
> @@ -994,6 +1022,17 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
>      dinfo->devaddr = devaddr;
>      dinfo->serial = g_strdup(serial);
>  
> +    if (type == IF_VIRTIO) {
> +        /* We created the automatic device earlier. For other types this
> +         * will be set to true at the point where the drive is claimed
> +         * by the IDE/SCSI/etc bus, when that code calls blk_by_legacy_dinfo()
> +         * to find the block backend from the drive.
> +         */
> +        dinfo->auto_claimed = true;
> +    }
> +
> +    dinfo->type_is_board_default = type_is_board_default;
> +
>      blk_set_legacy_dinfo(blk, dinfo);
>  
>      switch(type) {
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index 3104150..f9c44e2 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -36,6 +36,8 @@ struct DriveInfo {
>      int unit;
>      int auto_del;               /* see blockdev_mark_auto_del() */
>      bool is_default;            /* Added by default_drive() ?  */
> +    bool auto_claimed;          /* Automatically claimed by board model? */
> +    bool type_is_board_default; /* type is from board default, not user 'if=' */
>      int media_cd;
>      int cyls, heads, secs, trans;
>      QemuOpts *opts;

Re: [Qemu-devel] [PATCH 2/4] qdev-properties-system: Change set_pointer's parse callback to use Error

[Peter Maydell]

On 22 June 2015 at 10:39, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> Instead of having set_pointer() call a parse callback which returns
>> an error number that we then convert to an Error string with
>> error_set_from_qdev_prop_error(), make the parse callback take an
>> Error** and set the error itself. This will allow parse routines
>> to provide more helpful error messages than the generic ones.

> The error messages are faithfully copied from
> error_set_from_qdev_prop_error().  The next patch will improve
> parse_drive()'s messages, and that's why you got this patch.

That's right.

> The next patch takes special care of the if=T, T!=none case.  That case
> only exists for drives, because only for drives did we mix up old-style
> (one option to configure front- and backend) and new-style (one option
> to configure backend, -device to configure frontend) configuration.
>
> It also changes the if=none case, from
>
>     Property 'TYPE.PROP' can't take value 'VAL', it's in use
>
> to
>
>     "Property 'TYPE.PROP' can't be set to drive ID 'VAL'; that drive has already been connected to another device."
>
> More explicit.  Rather long, though.

Do you have a proposed better phrasing? In general I feel that the problem
with the current error message is that:
 (1) it is phrased from the point of view of QEMU's internal implementation
 (2) it is extremely terse

So the lengthiness is in my opinion an improvement. Better not to
leave the user in the dark about why we just refused to run...

> If this change is deemed an improvement for drive=, then it surely is
> one for the other backend references, isn't it?  These are parse_chr()
> for chardev= (shown above), and set_netdev() for netdev= (not shown).
>
>     Property 'TYPE.PROP' can't take value 'VAL', it's in use
>
> vs.
>
>     Property 'TYPE.PROP' can't take value 'VAL'; that chardev has already been connected to another device.
>
> Same as above, except s/drive/chardev/.  Likewise for netdev.
>
> Other users of error_set_from_qdev_prop_error() remain: set_vlan(),
> set_mac(), set_pci_devfn(), set_pci_host_devaddr().  Okay.  I figure the
> error message duplication could be avoided by calling
> error_set_from_qdev_prop_error() here.  No biggie either way.

Yes. My feeling is that error_set_from_qdev_prop_error() should
probably be removed entirely; but with 2.4 looming I didn't want
to bulk this patchset out with that change.

-- PMM

Re: [Qemu-devel] [PATCH 3/4] qdev-properties-system: Improve error message for drive assignment conflict

[Peter Maydell]

On 22 June 2015 at 10:12, Markus Armbruster <armbru@redhat.com> wrote:
> I think we should just bite the bullet and extend Error to support
> additional helpful information for humans.

...I thought all of the string was already just helpful information
for humans? I certainly hope we aren't expecting anybody to parse it...

-- PMM

Re: [Qemu-devel] [PATCH 3/4] qdev-properties-system: Improve error message for drive assignment conflict

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 22 June 2015 at 10:12, Markus Armbruster <armbru@redhat.com> wrote:
>> We generally do not end error messages with a period.
>>
>> The message for auto_claimed drives is of the form
>>
>>     LOCATION: This went wrong.  Advice on how to fix it.
>>
>> All in one awfully long line.  A friendler format is
>>
>>     LOCATION: This went wrong
>>     Advice on how to fix it.
>>
>> Unfortunately, the Error API doesn't support that.
[...]
>> I think we should just bite the bullet and extend Error to support
>> additional helpful information for humans.
>
> ...I thought all of the string was already just helpful information
> for humans? I certainly hope we aren't expecting anybody to parse it...

My point is

    qemu-system-arm: -device virtio-blk-pci,drive=foo: Property 'virtio-blk-device can't be set to drive ID 'foo'; that drive has been automatically connected to another device. Use if=none if you do not want that automatic connection.

is considerable less readable than something like

    qemu-system-arm: -device virtio-blk-pci,drive=foo: Property 'virtio-blk-device can't take value 'foo', it's in use
    The drive with ID 'foo' has been automatically connected to another device.
    Use if=none if you do not want that automatic connection.

Error messages should be short and to the point.  We actually enforce a
"no newline in error messages" policy there.

Some errors can use a more verbose explanation, or hints on how to avoid
the error, or hints on how to find out more on what exactly went wrong".
Such explanations should be sensibly formatted, not crammed into the
same line as the error message.

Whether the additional explanation should also be transmitted over QMP
is open for debate.

Note: my example is just for demonstrating layout and style.  No claim
to perfection for the actual text.

Re: [Qemu-devel] [PATCH 2/4] qdev-properties-system: Change set_pointer's parse callback to use Error

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 22 June 2015 at 10:39, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> Instead of having set_pointer() call a parse callback which returns
>>> an error number that we then convert to an Error string with
>>> error_set_from_qdev_prop_error(), make the parse callback take an
>>> Error** and set the error itself. This will allow parse routines
>>> to provide more helpful error messages than the generic ones.
>
>> The error messages are faithfully copied from
>> error_set_from_qdev_prop_error().  The next patch will improve
>> parse_drive()'s messages, and that's why you got this patch.
>
> That's right.
>
>> The next patch takes special care of the if=T, T!=none case.  That case
>> only exists for drives, because only for drives did we mix up old-style
>> (one option to configure front- and backend) and new-style (one option
>> to configure backend, -device to configure frontend) configuration.
>>
>> It also changes the if=none case, from
>>
>>     Property 'TYPE.PROP' can't take value 'VAL', it's in use
>>
>> to
>>
>>     "Property 'TYPE.PROP' can't be set to drive ID 'VAL'; that drive
>> has already been connected to another device."
>>
>> More explicit.  Rather long, though.
>
> Do you have a proposed better phrasing? In general I feel that the problem
> with the current error message is that:
>  (1) it is phrased from the point of view of QEMU's internal implementation
>  (2) it is extremely terse
>
> So the lengthiness is in my opinion an improvement. Better not to
> leave the user in the dark about why we just refused to run...

Perhaps

    Drive VAL is already used by another device

Relies on the fact that the context is either blatantly obvious (monitor
command), or the actual message makes it obvious, e.g.

    qemu-system-arm: -device virtio-blk-pci,drive=foo: Drive foo is already used by another device

[...]

Re: [Qemu-devel] [PATCH 1/4] block: Warn if an if=<something> drive was also connected manually

[Peter Maydell]

On 22 June 2015 at 10:59, Markus Armbruster <armbru@redhat.com> wrote:
> What about this instead:
>
> 1. When -device creation connects a qdev_prop_drive property to a
> backend, fail when the backend has a DriveInfo and the DriveInfo has
> type != IF_NONE.  Note: the connection is made in parse_drive().

So, the reason I didn't do this is that it breaks some options
that currently work (the ones where the board doesn't pick up
the device and so there's no conflict). I preferred to make those
"continue to function, but warn", but if we're happy to make this
a hard error we could go this way.

> 2. This breaks -drive if=virtio and possibly more.  That's because for
> if=virtio, we create input for -device creation that asks to connect
> this IF_VIRTIO drive.  To unbreak it, mark the DriveInfo when we create
> such input, and make parse_drive() accept backends so marked.  To find
> the places that need to mark, examine users of option group "device".  A
> quick, sloppy grep shows a bit over a dozen candidates.  Not too bad.

How do we then tell the difference between parse_drive() being called
for the auto-created virtio device, and it then later being called as
part of connecting the drive to a manually created device?

My grep (which was for 'qemu_find_opts.*device' -- is that right?)
suggests that in fact the only case we need to care about is the
one where we auto-create the virtio device (the other grep matches
are not relevant).

> In my opinion, a board that specifies a default interface type it
> doesn't support is simply broken, and should be fixed.

I'm inclined to agree, but I bet we have a lot. It doesn't help
that the default if the machine doesn't specify anything is "IDE",
not "you can't use a default interface"...

> Even if we fix that, we could still reach this case when the board
> claims only *some* of the drives for its default type.  Example:
>
>     $ qemu-system-x86_64 -nodefaults -S -display none -drive if=floppy,file=tmp.qcow2,index=99
>     Warning: Orphaned drive without device: id=floppy99,file=tmp.qcow2,if=floppy,bus=0,unit=99
>
> Compare:
>
>     $ qemu-system-x86_64 -nodefaults -S -display none -drive if=ide,file=tmp.qcow2,index=99
>     qemu-system-x86_64: Too many IDE buses defined (50 > 2)
>
> Crap shot.
>
> If we have boards that don't claim *any* interface type, we need to give
> them a .block_default_type that rejects -drive without an explicit if=.

We have several of these boards, yes. (for example, hw/arm/cubieboard.c)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/4] block: Warn if an if=<something> drive was also connected manually

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 22 June 2015 at 10:59, Markus Armbruster <armbru@redhat.com> wrote:
>> What about this instead:
>>
>> 1. When -device creation connects a qdev_prop_drive property to a
>> backend, fail when the backend has a DriveInfo and the DriveInfo has
>> type != IF_NONE.  Note: the connection is made in parse_drive().
>
> So, the reason I didn't do this is that it breaks some options
> that currently work (the ones where the board doesn't pick up
> the device and so there's no conflict). I preferred to make those
> "continue to function, but warn", but if we're happy to make this
> a hard error we could go this way.

I think we could make it a warning in parse_drive(), too:

    blk = blk_by_name(str);
    if (!blk) {
        // no such backend, bail out
    }
    if (blk_attach_dev(blk, dev) < 0) {
        // backend already attached, bail out
    }
    dinfo = blk_legacy_dinfo(blk);
    if (dinfo && dinfo->type != IF_NONE) {
        // warn, but carry on
    }

>> 2. This breaks -drive if=virtio and possibly more.  That's because for
>> if=virtio, we create input for -device creation that asks to connect
>> this IF_VIRTIO drive.  To unbreak it, mark the DriveInfo when we create
>> such input, and make parse_drive() accept backends so marked.  To find
>> the places that need to mark, examine users of option group "device".  A
>> quick, sloppy grep shows a bit over a dozen candidates.  Not too bad.
>
> How do we then tell the difference between parse_drive() being called
> for the auto-created virtio device, and it then later being called as
> part of connecting the drive to a manually created device?

Good point.  We need a way to determine whether we're running on behalf
of the QemuOpts created for this DriveInfo.

So make the DriveInfo mark a QemuOpts *device_opts, non-null only when
drive_new() creates device options.

Now parse_drive() has to check whether dinfo->device_opts matches the
qdev_device_add()'s opts argument.  Fortunately, qdev_device_add()
stores it in dev->opts, so this could do:

    dinfo = blk_legacy_dinfo(blk);
    if (dinfo && dinfo->type != IF_NONE && dinfo->device_opts != dev->opts) {
        // warn, but carry on
    }

> My grep (which was for 'qemu_find_opts.*device' -- is that right?)
> suggests that in fact the only case we need to care about is the
> one where we auto-create the virtio device (the other grep matches
> are not relevant).

I'm not aware of another one.

>> In my opinion, a board that specifies a default interface type it
>> doesn't support is simply broken, and should be fixed.
>
> I'm inclined to agree, but I bet we have a lot. It doesn't help
> that the default if the machine doesn't specify anything is "IDE",
> not "you can't use a default interface"...

Easy enough to change:

    typedef enum {
        IF_DEFAULT = -1,            /* for use with drive_add() only */
        /*
         * IF_NONE must be zero, because we want QEMUMachine member
         * block_default_type to default-initialize to IF_NONE
         */
        IF_NONE,
        IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
        IF_COUNT
    } BlockInterfaceType;

Then special-case IF_NONE in drive_new():

    value = qemu_opt_get(legacy_opts, "if");
    if (value) {
        ...
    } else if (block_default_type == IF_NONE) {
        error_report("bla, bla");
        goto fail;
    } else {
        type = block_default_type;
    }

If you want to support .block_default_type = IF_NONE, you need to create
a new special member for this purpose instead.

Naturally, we then have to find all boards that actually claim their IDE
drives and fix them to .block_default_type = IF_IDE.  Shouldn't be hard.

>> Even if we fix that, we could still reach this case when the board
>> claims only *some* of the drives for its default type.  Example:
>>
>>     $ qemu-system-x86_64 -nodefaults -S -display none -drive
>> if=floppy,file=tmp.qcow2,index=99
>>     Warning: Orphaned drive without device:
>> id=floppy99,file=tmp.qcow2,if=floppy,bus=0,unit=99
>>
>> Compare:
>>
>>     $ qemu-system-x86_64 -nodefaults -S -display none -drive
>> if=ide,file=tmp.qcow2,index=99
>>     qemu-system-x86_64: Too many IDE buses defined (50 > 2)
>>
>> Crap shot.
>>
>> If we have boards that don't claim *any* interface type, we need to give
>> them a .block_default_type that rejects -drive without an explicit if=.
>
> We have several of these boards, yes. (for example, hw/arm/cubieboard.c)
>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH 1/4] block: Warn if an if=<something> drive was also connected manually

[Peter Maydell]

On 22 June 2015 at 10:59, Markus Armbruster <armbru@redhat.com> wrote:
> What about this instead:
>
> 1. When -device creation connects a qdev_prop_drive property to a
> backend, fail when the backend has a DriveInfo and the DriveInfo has
> type != IF_NONE.  Note: the connection is made in parse_drive().
>
> 2. This breaks -drive if=virtio and possibly more.  That's because for
> if=virtio, we create input for -device creation that asks to connect
> this IF_VIRTIO drive.  To unbreak it, mark the DriveInfo when we create
> such input, and make parse_drive() accept backends so marked.  To find
> the places that need to mark, examine users of option group "device".  A
> quick, sloppy grep shows a bit over a dozen candidates.  Not too bad.

It looks like we missed a bunch of places that need changing, because
we don't only go through parse_drive() for connections made by user
-device creation and places where the code has added something to the
"device" option group. For instance, if=scsi auto-plugin for x86 pc will
end up calling qdev_prop_set_drive() from scsi_bus_legacy_add_drive(),
which then goes through parse_drive().

There are 17 places that call qdev_prop_set_drive(); we could change
them all, but that's the sort of invasive change I was hoping to avoid
with my initial blk_by_legacy_dinfo change...
(I think currently all callers of qdev_prop_set_drive{,_nofail}()
are using it to connect legacy drives, except for hw/usb/dev-storage.c,
which is calling drive_new() to create an if=none drive and then
wiring it up to the device with qdev_prop_set_drive(). This feels
like a fragile assumption for the future, though.)

thanks
-- PMM