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

[Qemu-devel] [PATCH v2 for-2.4 0/3] 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.

Version 2 reduces scope, because it turns out that it is harder than
we thought to identify whether a use of an if=something drive is the
auto-plugging or a manual wiring up to a device. So all we do here
is improve the error messages for two situations which were already
errors but with rather cryptic messages:

    (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:
      qemu-system-x86_64: -device ide-hd,drive=foo: Property 'ide-hd.drive'
      can't take value 'foo', it's in use
    
    Now:
      qemu-system-x86_64: -device ide-hd,drive=foo: Drive 'foo' is already in
      use because it has been automatically connected to another device (did
      you need 'if=none' in the drive options?)
    
    (2) Drive specified to be manually connected in two different ways:
    
      qemu-system-x86_64 -nodefaults -display none \
       -drive if=none,file=tmp.qcow2,id=foo -device ide-hd,drive=foo \
       -device ide-hd,drive=foo
    
    Previously:
      qemu-system-x86_64: -device ide-hd,drive=foo: Property 'ide-hd.drive'
      can't take value 'foo', it's in use
    
    Now:
      qemu-system-x86_64: -device ide-hd,drive=foo: Drive 'foo' is already in
      use by another device

(We'll also produce message 1 in the oddball case where the user creates
a drive if=something-not-handled-by-machine and then wires it up manually
to two different devices; in this case their command line is doubly broken
and if they use if=none as suggested by message 1 they'll then get message 2
and can fix their own double-usage...)

Changes v1->v2:
 * drop "warn if an if=<something> drive was also connected manually" patch
 * change implementation of "improve error message" patch


Peter Maydell (3):
  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

 hw/arm/virt.c                    |  2 ++
 hw/core/qdev-properties-system.c | 42 +++++++++++++++++++++++++++-------------
 2 files changed, 31 insertions(+), 13 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v2 for-2.4 1/3] 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 v2 for-2.4 2/3] 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.

We rephrase the "drive conflict but not for an if=something" error as
well to keep the wording in line.

The two cases that change 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:
  qemu-system-x86_64: -device ide-hd,drive=foo: Property 'ide-hd.drive'
  can't take value 'foo', it's in use

Now:
  qemu-system-x86_64: -device ide-hd,drive=foo: Drive 'foo' is already in
  use because it has been automatically connected to another device (did
  you need 'if=none' in the drive options?)

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

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

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

Now:
  qemu-system-x86_64: -device ide-hd,drive=foo: Drive 'foo' is already in
  use by another device

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

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 56954b4..820cd14 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -78,8 +78,17 @@ 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->type != IF_NONE) {
+            error_setg(errp, "Drive '%s' is already in use because "
+                       "it has been automatically connected to another "
+                       "device (did you need 'if=none' in the drive options?)",
+                       str);
+        } else {
+            error_setg(errp, "Drive '%s' is already in use by another device",
+                       str);
+        }
         return;
     }
     *ptr = blk;
-- 
1.9.1

[Qemu-devel] [PATCH v2 for-2.4 3/3] 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.

Note that this change means that some command lines which used
to work (by accident) will stop working. Where a drive was connected
manually to a device but without 'if=none' being specified, we
used to treat this as an IDE drive, which we would then not autoplug
because the board doesn't support IDE. Now we will treat it as a
virtio disk and autoplug it, which means the attempt to use the
drive manually will fail:
  qemu-system-arm: -drive file=img.qcow2,id=foo: Drive 'foo' is already
  in use because it has been automatically connected to another device
  (did you need 'if=none' in the drive options?)
The command line will be changed to include 'if=none', as the
error message suggests.

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 f1e85c8..7e643ba 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -966,6 +966,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 v2 for-2.4 3/3] hw/arm/virt: Make block devices default to virtio

[Markus Armbruster]

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

> 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.

It'll create a virtio-blk device with non-removable medium, won't it?

> Note that this change means that some command lines which used
> to work (by accident) will stop working. Where a drive was connected
> manually to a device but without 'if=none' being specified, we
> used to treat this as an IDE drive, which we would then not autoplug
> because the board doesn't support IDE. Now we will treat it as a
> virtio disk and autoplug it, which means the attempt to use the
> drive manually will fail:
>   qemu-system-arm: -drive file=img.qcow2,id=foo: Drive 'foo' is already
>   in use because it has been automatically connected to another device
>   (did you need 'if=none' in the drive options?)
> The command line will be changed to include 'if=none', as the

will have to be changed

> error message suggests.
>
> 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 f1e85c8..7e643ba 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -966,6 +966,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 = {

Re: [Qemu-devel] [PATCH v2 for-2.4 3/3] hw/arm/virt: Make block devices default to virtio

[Peter Maydell]

On 25 June 2015 at 08:40, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> 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.
>
> It'll create a virtio-blk device with non-removable medium, won't it?

Yes, I think so. Mostly I cared that -cdrom won't make qemu die with a
confusing error.

(Without no_cdrom, qemu dies even if you don't say -cdrom, because
of the default empty drive.)

>> The command line will be changed to include 'if=none', as the
>
> will have to be changed

Yes.

-- PMM

Re: [Qemu-devel] [PATCH v2 for-2.4 3/3] hw/arm/virt: Make block devices default to virtio

[Markus Armbruster]

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

> On 25 June 2015 at 08:40, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> 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.
>>
>> It'll create a virtio-blk device with non-removable medium, won't it?
>
> Yes, I think so. Mostly I cared that -cdrom won't make qemu die with a
> confusing error.
>
> (Without no_cdrom, qemu dies even if you don't say -cdrom, because
> of the default empty drive.)

Instead of "will still work", I'd like to see something like "will
succeed, but silently create a device with non-removable medium, which
is probably not what you want, but the best we can do now".

>>> The command line will be changed to include 'if=none', as the
>>
>> will have to be changed
>
> Yes.
>
> -- PMM

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

[Markus Armbruster]

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

> 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.
>
> Version 2 reduces scope, because it turns out that it is harder than
> we thought to identify whether a use of an if=something drive is the
> auto-plugging or a manual wiring up to a device. So all we do here
> is improve the error messages for two situations which were already
> errors but with rather cryptic messages:
>
>     (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:
>       qemu-system-x86_64: -device ide-hd,drive=foo: Property 'ide-hd.drive'
>       can't take value 'foo', it's in use
>     
>     Now:
>       qemu-system-x86_64: -device ide-hd,drive=foo: Drive 'foo' is already in
>       use because it has been automatically connected to another device (did
>       you need 'if=none' in the drive options?)
>     
>     (2) Drive specified to be manually connected in two different ways:
>     
>       qemu-system-x86_64 -nodefaults -display none \
>        -drive if=none,file=tmp.qcow2,id=foo -device ide-hd,drive=foo \
>        -device ide-hd,drive=foo
>     
>     Previously:
>       qemu-system-x86_64: -device ide-hd,drive=foo: Property 'ide-hd.drive'
>       can't take value 'foo', it's in use
>     
>     Now:
>       qemu-system-x86_64: -device ide-hd,drive=foo: Drive 'foo' is already in
>       use by another device
>
> (We'll also produce message 1 in the oddball case where the user creates
> a drive if=something-not-handled-by-machine and then wires it up manually
> to two different devices; in this case their command line is doubly broken
> and if they use if=none as suggested by message 1 they'll then get message 2
> and can fix their own double-usage...)

With the commit message of PATCH 3 amended, series
Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Peter Maydell]

On 25 June 2015 at 10:26, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> 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.

> With the commit message of PATCH 3 amended, series
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks. Do you want to take this through a block tree or are you happy
for me to put it through target-arm.next?

-- PMM

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

[Markus Armbruster]

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

> On 25 June 2015 at 10:26, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> 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.
>
>> With the commit message of PATCH 3 amended, series
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> Thanks. Do you want to take this through a block tree or are you happy
> for me to put it through target-arm.next?

I'm fine with target-arm.  Copying Stefan in the unlikely case he's not.

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3111 bytes --]

On Tue, Jun 23, 2015 at 03:01:44PM +0100, Peter Maydell 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.
> 
> Version 2 reduces scope, because it turns out that it is harder than
> we thought to identify whether a use of an if=something drive is the
> auto-plugging or a manual wiring up to a device. So all we do here
> is improve the error messages for two situations which were already
> errors but with rather cryptic messages:
> 
>     (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:
>       qemu-system-x86_64: -device ide-hd,drive=foo: Property 'ide-hd.drive'
>       can't take value 'foo', it's in use
>     
>     Now:
>       qemu-system-x86_64: -device ide-hd,drive=foo: Drive 'foo' is already in
>       use because it has been automatically connected to another device (did
>       you need 'if=none' in the drive options?)
>     
>     (2) Drive specified to be manually connected in two different ways:
>     
>       qemu-system-x86_64 -nodefaults -display none \
>        -drive if=none,file=tmp.qcow2,id=foo -device ide-hd,drive=foo \
>        -device ide-hd,drive=foo
>     
>     Previously:
>       qemu-system-x86_64: -device ide-hd,drive=foo: Property 'ide-hd.drive'
>       can't take value 'foo', it's in use
>     
>     Now:
>       qemu-system-x86_64: -device ide-hd,drive=foo: Drive 'foo' is already in
>       use by another device
> 
> (We'll also produce message 1 in the oddball case where the user creates
> a drive if=something-not-handled-by-machine and then wires it up manually
> to two different devices; in this case their command line is doubly broken
> and if they use if=none as suggested by message 1 they'll then get message 2
> and can fix their own double-usage...)
> 
> Changes v1->v2:
>  * drop "warn if an if=<something> drive was also connected manually" patch
>  * change implementation of "improve error message" patch
> 
> 
> Peter Maydell (3):
>   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
> 
>  hw/arm/virt.c                    |  2 ++
>  hw/core/qdev-properties-system.c | 42 +++++++++++++++++++++++++++-------------
>  2 files changed, 31 insertions(+), 13 deletions(-)
> 
> -- 
> 1.9.1
> 
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1168 bytes --]

On Thu, Jun 25, 2015 at 12:49:31PM +0200, Markus Armbruster wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
> > On 25 June 2015 at 10:26, Markus Armbruster <armbru@redhat.com> wrote:
> >> Peter Maydell <peter.maydell@linaro.org> writes:
> >>
> >>> 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.
> >
> >> With the commit message of PATCH 3 amended, series
> >> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> >
> > Thanks. Do you want to take this through a block tree or are you happy
> > for me to put it through target-arm.next?
> 
> I'm fine with target-arm.  Copying Stefan in the unlikely case he's not.

Thanks, I'm happy with this series.  Please take it through the arm
tree.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]