[Qemu-devel] [PATCH 0/4] Miscellaneous block fixes

[Qemu-devel] [PATCH 0/4] Miscellaneous block fixes

[Markus Armbruster]

More random crap I ran into while iterating on BlockBackends.

Markus Armbruster (4):
  blockdev: Disentangle BlockDriverState and DriveInfo creation
  block: Keep DriveInfo alive until BlockDriverState dies
  qemu-nbd: Destroy the BlockDriverState properly
  block: Improve message for device name clashing with node name

 block.c                    |  5 ++++-
 blockdev.c                 | 56 +++++++++++++++++++++++++---------------------
 include/sysemu/blockdev.h  |  1 +
 qemu-nbd.c                 |  2 +-
 stubs/Makefile.objs        |  1 +
 stubs/blockdev.c           | 12 ++++++++++
 tests/qemu-iotests/087.out |  2 +-
 7 files changed, 51 insertions(+), 28 deletions(-)
 create mode 100644 stubs/blockdev.c

-- 
1.9.3

[Qemu-devel] [PATCH 1/4] blockdev: Disentangle BlockDriverState and DriveInfo creation

[Markus Armbruster]

blockdev_init() mixes up BlockDriverState and DriveInfo initialization
Finish the BlockDriverState job before starting to mess with
DriveInfo.  Easier on the eyes.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c | 43 +++++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index b361fbb..5ec4635 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -301,6 +301,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
     int ro = 0;
     int bdrv_flags = 0;
     int on_read_error, on_write_error;
+    BlockDriverState *bs;
     DriveInfo *dinfo;
     ThrottleConfig cfg;
     int snapshot = 0;
@@ -456,26 +457,27 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
     }
 
     /* init */
+    bs = bdrv_new(qemu_opts_id(opts), errp);
+    if (!bs) {
+        goto early_err;
+    }
+    bs->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
+    bs->read_only = ro;
+    bs->detect_zeroes = detect_zeroes;
+
+    bdrv_set_on_error(bs, on_read_error, on_write_error);
+
+    /* disk I/O throttling */
+    if (throttle_enabled(&cfg)) {
+        bdrv_io_limits_enable(bs);
+        bdrv_set_io_limits(bs, &cfg);
+    }
+
     dinfo = g_malloc0(sizeof(*dinfo));
     dinfo->id = g_strdup(qemu_opts_id(opts));
-    dinfo->bdrv = bdrv_new(dinfo->id, &error);
-    if (error) {
-        error_propagate(errp, error);
-        goto bdrv_new_err;
-    }
-    dinfo->bdrv->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
-    dinfo->bdrv->read_only = ro;
-    dinfo->bdrv->detect_zeroes = detect_zeroes;
+    dinfo->bdrv = bs;
     QTAILQ_INSERT_TAIL(&drives, dinfo, next);
 
-    bdrv_set_on_error(dinfo->bdrv, on_read_error, on_write_error);
-
-    /* disk I/O throttling */
-    if (throttle_enabled(&cfg)) {
-        bdrv_io_limits_enable(dinfo->bdrv);
-        bdrv_set_io_limits(dinfo->bdrv, &cfg);
-    }
-
     if (!file || !*file) {
         if (has_driver_specific_opts) {
             file = NULL;
@@ -502,7 +504,8 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
     bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
 
     QINCREF(bs_opts);
-    ret = bdrv_open(&dinfo->bdrv, file, NULL, bs_opts, bdrv_flags, drv, &error);
+    ret = bdrv_open(&bs, file, NULL, bs_opts, bdrv_flags, drv, &error);
+    assert(bs == dinfo->bdrv);
 
     if (ret < 0) {
         error_setg(errp, "could not open disk image %s: %s",
@@ -511,8 +514,9 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
         goto err;
     }
 
-    if (bdrv_key_required(dinfo->bdrv))
+    if (bdrv_key_required(bs)) {
         autostart = 0;
+    }
 
     QDECREF(bs_opts);
     qemu_opts_del(opts);
@@ -520,9 +524,8 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
     return dinfo;
 
 err:
-    bdrv_unref(dinfo->bdrv);
+    bdrv_unref(bs);
     QTAILQ_REMOVE(&drives, dinfo, next);
-bdrv_new_err:
     g_free(dinfo->id);
     g_free(dinfo);
 early_err:
-- 
1.9.3

[Qemu-devel] [PATCH 2/4] block: Keep DriveInfo alive until BlockDriverState dies

[Markus Armbruster]

If the BDS's refcnt > 0, drive_del() destroys the DriveInfo, but not
the BDS.  This can happen in three places:

* Device model destruction during unplug: blockdev_auto_del()

* Xen IDE unplug: pci_piix3_xen_ide_unplug()

* drive_del command when no device model is attached: do_drive_del()

The other callers of drive_del are on error paths where refcnt == 1.

If the user somehow manages to plug in a device model using a BDS that
has gone through drive_del(), the legacy configuration passed in
DriveInfo doesn't reach the device model, and automatic deletion on
unplug doesn't work.  Worse, some device models such as scsi-disk
crash when DriveInfo doesn't exist.

This is theoretical; I didn't research an actual reproducer.

Fix by keeping DriveInfo alive until its BDS dies.

This affects qemu_drive_opts: now you can't reuse the same ID for new
drive options until the BDS dies.  Before, you could, but since the
code always attempts to create a BDS with the same ID next, the
enclosing operation "create a new drive" failed anyway.  Different
error path, same result.

Unfortunately, the fix involves use of blockdev.c stuff from block.c,
which is a layering violation.  Fortunately, my forthcoming
BlockBackend work will get rid of it again.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block.c                   |  2 ++
 blockdev.c                | 13 ++++++++-----
 include/sysemu/blockdev.h |  1 +
 stubs/Makefile.objs       |  1 +
 stubs/blockdev.c          | 12 ++++++++++++
 5 files changed, 24 insertions(+), 5 deletions(-)
 create mode 100644 stubs/blockdev.c

diff --git a/block.c b/block.c
index d06dd51..6faf36f 100644
--- a/block.c
+++ b/block.c
@@ -29,6 +29,7 @@
 #include "qemu/module.h"
 #include "qapi/qmp/qjson.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/blockdev.h"    /* FIXME layering violation */
 #include "qemu/notify.h"
 #include "block/coroutine.h"
 #include "block/qapi.h"
@@ -2110,6 +2111,7 @@ static void bdrv_delete(BlockDriverState *bs)
     /* remove from list, if necessary */
     bdrv_make_anon(bs);
 
+    drive_info_del(drive_get_by_blockdev(bs));
     g_free(bs);
 }
 
diff --git a/blockdev.c b/blockdev.c
index 5ec4635..450f95c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -216,11 +216,17 @@ static void bdrv_format_print(void *opaque, const char *name)
 
 void drive_del(DriveInfo *dinfo)
 {
+    bdrv_unref(dinfo->bdrv);
+}
+
+void drive_info_del(DriveInfo *dinfo)
+{
+    if (!dinfo) {
+        return;
+    }
     if (dinfo->opts) {
         qemu_opts_del(dinfo->opts);
     }
-
-    bdrv_unref(dinfo->bdrv);
     g_free(dinfo->id);
     QTAILQ_REMOVE(&drives, dinfo, next);
     g_free(dinfo->serial);
@@ -525,9 +531,6 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
 
 err:
     bdrv_unref(bs);
-    QTAILQ_REMOVE(&drives, dinfo, next);
-    g_free(dinfo->id);
-    g_free(dinfo);
 early_err:
     qemu_opts_del(opts);
 err_no_opts:
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 23a5d10..abec381 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -56,6 +56,7 @@ QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
                     const char *optstr);
 DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type);
 void drive_del(DriveInfo *dinfo);
+void drive_info_del(DriveInfo *dinfo);
 
 /* device-hotplug */
 
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 5e347d0..c0b1f6a 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -1,5 +1,6 @@
 stub-obj-y += arch-query-cpu-def.o
 stub-obj-y += bdrv-commit-all.o
+stub-obj-y += blockdev.o
 stub-obj-y += chr-baum-init.o
 stub-obj-y += chr-msmouse.o
 stub-obj-y += chr-testdev.o
diff --git a/stubs/blockdev.c b/stubs/blockdev.c
new file mode 100644
index 0000000..5d0a79c
--- /dev/null
+++ b/stubs/blockdev.c
@@ -0,0 +1,12 @@
+#include <assert.h>
+#include "sysemu/blockdev.h"
+
+DriveInfo *drive_get_by_blockdev(BlockDriverState *bs)
+{
+    return NULL;
+}
+
+void drive_info_del(DriveInfo *dinfo)
+{
+    assert(!dinfo);
+}
-- 
1.9.3

[Qemu-devel] [PATCH 3/4] qemu-nbd: Destroy the BlockDriverState properly

[Markus Armbruster]

Match the bdrv_new() with a bdrv_unref(), just to be tidy.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qemu-nbd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 9bc152e..f3cf8a2 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -769,7 +769,7 @@ int main(int argc, char **argv)
         }
     } while (state != TERMINATED);
 
-    bdrv_close(bs);
+    bdrv_unref(bs);
     if (sockpath) {
         unlink(sockpath);
     }
-- 
1.9.3

[Qemu-devel] [PATCH 4/4] block: Improve message for device name clashing with node name

[Markus Armbruster]

Suggested-by: Benoit Canet <benoit.canet@nodalink.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block.c                    | 3 ++-
 tests/qemu-iotests/087.out | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 6faf36f..02ea90f 100644
--- a/block.c
+++ b/block.c
@@ -347,7 +347,8 @@ BlockDriverState *bdrv_new(const char *device_name, Error **errp)
         return NULL;
     }
     if (bdrv_find_node(device_name)) {
-        error_setg(errp, "Device with node-name '%s' already exists",
+        error_setg(errp,
+                   "Device name '%s' conflicts with an existing node name",
                    device_name);
         return NULL;
     }
diff --git a/tests/qemu-iotests/087.out b/tests/qemu-iotests/087.out
index 7fbee3f..75a54e0 100644
--- a/tests/qemu-iotests/087.out
+++ b/tests/qemu-iotests/087.out
@@ -20,7 +20,7 @@ QMP_VERSION
 {"return": {}}
 {"return": {}}
 {"error": {"class": "GenericError", "desc": "Device with id 'disk' already exists"}}
-{"error": {"class": "GenericError", "desc": "Device with node-name 'test-node' already exists"}}
+{"error": {"class": "GenericError", "desc": "Device name 'test-node' conflicts with an existing node name"}}
 main-loop: WARNING: I/O thread spun for 1000 iterations
 {"error": {"class": "GenericError", "desc": "could not open disk image disk2: node-name=disk is conflicting with a device id"}}
 {"error": {"class": "GenericError", "desc": "could not open disk image disk2: Duplicate node name"}}
-- 
1.9.3

Re: [Qemu-devel] [PATCH 3/4] qemu-nbd: Destroy the BlockDriverState properly

[Paolo Bonzini]

Il 12/09/2014 21:26, Markus Armbruster ha scritto:
> Match the bdrv_new() with a bdrv_unref(), just to be tidy.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qemu-nbd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 9bc152e..f3cf8a2 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -769,7 +769,7 @@ int main(int argc, char **argv)
>          }
>      } while (state != TERMINATED);
>  
> -    bdrv_close(bs);
> +    bdrv_unref(bs);
>      if (sockpath) {
>          unlink(sockpath);
>      }
> 

Applying to nbd-next, thanks.

Paolo

Re: [Qemu-devel] [PATCH 4/4] block: Improve message for device name clashing with node name

[Eric Blake]

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

On 09/12/2014 01:26 PM, Markus Armbruster wrote:
> Suggested-by: Benoit Canet <benoit.canet@nodalink.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block.c                    | 3 ++-
>  tests/qemu-iotests/087.out | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH 1/4] blockdev: Disentangle BlockDriverState and DriveInfo creation

[Max Reitz]

On 12.09.2014 21:26, Markus Armbruster wrote:
> blockdev_init() mixes up BlockDriverState and DriveInfo initialization
> Finish the BlockDriverState job before starting to mess with
> DriveInfo.  Easier on the eyes.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   blockdev.c | 43 +++++++++++++++++++++++--------------------
>   1 file changed, 23 insertions(+), 20 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index b361fbb..5ec4635 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -301,6 +301,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>       int ro = 0;
>       int bdrv_flags = 0;
>       int on_read_error, on_write_error;
> +    BlockDriverState *bs;
>       DriveInfo *dinfo;
>       ThrottleConfig cfg;
>       int snapshot = 0;
> @@ -456,26 +457,27 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>       }
>   
>       /* init */
> +    bs = bdrv_new(qemu_opts_id(opts), errp);
> +    if (!bs) {
> +        goto early_err;
> +    }
> +    bs->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
> +    bs->read_only = ro;
> +    bs->detect_zeroes = detect_zeroes;
> +
> +    bdrv_set_on_error(bs, on_read_error, on_write_error);
> +
> +    /* disk I/O throttling */
> +    if (throttle_enabled(&cfg)) {
> +        bdrv_io_limits_enable(bs);
> +        bdrv_set_io_limits(bs, &cfg);
> +    }
> +
>       dinfo = g_malloc0(sizeof(*dinfo));

Could've changed this to g_new0 in the process, but you're the expert 
for that, so I'll leave it up to you. ;-)

>       dinfo->id = g_strdup(qemu_opts_id(opts));
> -    dinfo->bdrv = bdrv_new(dinfo->id, &error);
> -    if (error) {
> -        error_propagate(errp, error);
> -        goto bdrv_new_err;
> -    }
> -    dinfo->bdrv->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
> -    dinfo->bdrv->read_only = ro;
> -    dinfo->bdrv->detect_zeroes = detect_zeroes;
> +    dinfo->bdrv = bs;
>       QTAILQ_INSERT_TAIL(&drives, dinfo, next);
>   
> -    bdrv_set_on_error(dinfo->bdrv, on_read_error, on_write_error);
> -
> -    /* disk I/O throttling */
> -    if (throttle_enabled(&cfg)) {
> -        bdrv_io_limits_enable(dinfo->bdrv);
> -        bdrv_set_io_limits(dinfo->bdrv, &cfg);
> -    }
> -
>       if (!file || !*file) {
>           if (has_driver_specific_opts) {
>               file = NULL;
> @@ -502,7 +504,8 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>       bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
>   
>       QINCREF(bs_opts);
> -    ret = bdrv_open(&dinfo->bdrv, file, NULL, bs_opts, bdrv_flags, drv, &error);
> +    ret = bdrv_open(&bs, file, NULL, bs_opts, bdrv_flags, drv, &error);
> +    assert(bs == dinfo->bdrv);

Well, this is guaranteed by bdrv_open(), but of course better having too 
many assertions than too few.

With or without g_new0:

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH 4/4] block: Improve message for device name clashing with node name

[Benoît Canet]

The Friday 12 Sep 2014 à 21:26:24 (+0200), Markus Armbruster wrote :
> Suggested-by: Benoit Canet <benoit.canet@nodalink.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block.c                    | 3 ++-
>  tests/qemu-iotests/087.out | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 6faf36f..02ea90f 100644
> --- a/block.c
> +++ b/block.c
> @@ -347,7 +347,8 @@ BlockDriverState *bdrv_new(const char *device_name, Error **errp)
>          return NULL;
>      }
>      if (bdrv_find_node(device_name)) {
> -        error_setg(errp, "Device with node-name '%s' already exists",
> +        error_setg(errp,
> +                   "Device name '%s' conflicts with an existing node name",
>                     device_name);
>          return NULL;
>      }
> diff --git a/tests/qemu-iotests/087.out b/tests/qemu-iotests/087.out
> index 7fbee3f..75a54e0 100644
> --- a/tests/qemu-iotests/087.out
> +++ b/tests/qemu-iotests/087.out
> @@ -20,7 +20,7 @@ QMP_VERSION
>  {"return": {}}
>  {"return": {}}
>  {"error": {"class": "GenericError", "desc": "Device with id 'disk' already exists"}}
> -{"error": {"class": "GenericError", "desc": "Device with node-name 'test-node' already exists"}}
> +{"error": {"class": "GenericError", "desc": "Device name 'test-node' conflicts with an existing node name"}}
>  main-loop: WARNING: I/O thread spun for 1000 iterations
>  {"error": {"class": "GenericError", "desc": "could not open disk image disk2: node-name=disk is conflicting with a device id"}}
>  {"error": {"class": "GenericError", "desc": "could not open disk image disk2: Duplicate node name"}}
> -- 
> 1.9.3
> 

Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>

Re: [Qemu-devel] [PATCH 2/4] block: Keep DriveInfo alive until BlockDriverState dies

[Max Reitz]

On 12.09.2014 21:26, Markus Armbruster wrote:
> If the BDS's refcnt > 0, drive_del() destroys the DriveInfo, but not
> the BDS.  This can happen in three places:
>
> * Device model destruction during unplug: blockdev_auto_del()
>
> * Xen IDE unplug: pci_piix3_xen_ide_unplug()
>
> * drive_del command when no device model is attached: do_drive_del()
>
> The other callers of drive_del are on error paths where refcnt == 1.
>
> If the user somehow manages to plug in a device model using a BDS that
> has gone through drive_del(), the legacy configuration passed in
> DriveInfo doesn't reach the device model, and automatic deletion on
> unplug doesn't work.  Worse, some device models such as scsi-disk
> crash when DriveInfo doesn't exist.
>
> This is theoretical; I didn't research an actual reproducer.
>
> Fix by keeping DriveInfo alive until its BDS dies.
>
> This affects qemu_drive_opts: now you can't reuse the same ID for new
> drive options until the BDS dies.  Before, you could, but since the
> code always attempts to create a BDS with the same ID next, the
> enclosing operation "create a new drive" failed anyway.  Different
> error path, same result.
>
> Unfortunately, the fix involves use of blockdev.c stuff from block.c,
> which is a layering violation.  Fortunately, my forthcoming
> BlockBackend work will get rid of it again.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   block.c                   |  2 ++
>   blockdev.c                | 13 ++++++++-----
>   include/sysemu/blockdev.h |  1 +
>   stubs/Makefile.objs       |  1 +
>   stubs/blockdev.c          | 12 ++++++++++++
>   5 files changed, 24 insertions(+), 5 deletions(-)
>   create mode 100644 stubs/blockdev.c

Seems reasonable.

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH 2/4] block: Keep DriveInfo alive until BlockDriverState dies

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> If the BDS's refcnt > 0, drive_del() destroys the DriveInfo, but not
> the BDS.  This can happen in three places:
>
> * Device model destruction during unplug: blockdev_auto_del()
>
> * Xen IDE unplug: pci_piix3_xen_ide_unplug()
>
> * drive_del command when no device model is attached: do_drive_del()
>
> The other callers of drive_del are on error paths where refcnt == 1.
>
> If the user somehow manages to plug in a device model using a BDS that
> has gone through drive_del(), the legacy configuration passed in
> DriveInfo doesn't reach the device model, and automatic deletion on
> unplug doesn't work.  Worse, some device models such as scsi-disk
> crash when DriveInfo doesn't exist.
>
> This is theoretical; I didn't research an actual reproducer.

Broken when we replaced DriveInfo reference counting by BDS reference
counting in commit a94a3fa..fa510eb.

Kevin, would you mind inserting that into the commit message?

> Fix by keeping DriveInfo alive until its BDS dies.
>
> This affects qemu_drive_opts: now you can't reuse the same ID for new
> drive options until the BDS dies.  Before, you could, but since the
> code always attempts to create a BDS with the same ID next, the
> enclosing operation "create a new drive" failed anyway.  Different
> error path, same result.
>
> Unfortunately, the fix involves use of blockdev.c stuff from block.c,
> which is a layering violation.  Fortunately, my forthcoming
> BlockBackend work will get rid of it again.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
[...]

Re: [Qemu-devel] [PATCH 1/4] blockdev: Disentangle BlockDriverState and DriveInfo creation

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 12.09.2014 21:26, Markus Armbruster wrote:
>> blockdev_init() mixes up BlockDriverState and DriveInfo initialization
>> Finish the BlockDriverState job before starting to mess with
>> DriveInfo.  Easier on the eyes.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   blockdev.c | 43 +++++++++++++++++++++++--------------------
>>   1 file changed, 23 insertions(+), 20 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index b361fbb..5ec4635 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -301,6 +301,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>>       int ro = 0;
>>       int bdrv_flags = 0;
>>       int on_read_error, on_write_error;
>> +    BlockDriverState *bs;
>>       DriveInfo *dinfo;
>>       ThrottleConfig cfg;
>>       int snapshot = 0;
>> @@ -456,26 +457,27 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>>       }
>>         /* init */
>> +    bs = bdrv_new(qemu_opts_id(opts), errp);
>> +    if (!bs) {
>> +        goto early_err;
>> +    }
>> +    bs->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
>> +    bs->read_only = ro;
>> +    bs->detect_zeroes = detect_zeroes;
>> +
>> +    bdrv_set_on_error(bs, on_read_error, on_write_error);
>> +
>> +    /* disk I/O throttling */
>> +    if (throttle_enabled(&cfg)) {
>> +        bdrv_io_limits_enable(bs);
>> +        bdrv_set_io_limits(bs, &cfg);
>> +    }
>> +
>>       dinfo = g_malloc0(sizeof(*dinfo));
>
> Could've changed this to g_new0 in the process, but you're the expert
> for that, so I'll leave it up to you. ;-)

When I made block use g_new() & friends, I only converted patterns like
p = g_malloc(sizeof(T)), not patterns like p = g_malloc(sizeof(*p)).

In the former case, p = g_new(T) is a clear improvement, because now the
compiler checks T matches typeof(*p).

In the latter case, we trade some visible obviousness for type safety.
Matter of taste.

If we agree to prefer type safety in block land, I'll gladly do the
conversion work.

>>       dinfo->id = g_strdup(qemu_opts_id(opts));
>> -    dinfo->bdrv = bdrv_new(dinfo->id, &error);
>> -    if (error) {
>> -        error_propagate(errp, error);
>> -        goto bdrv_new_err;
>> -    }
>> -    dinfo->bdrv->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
>> -    dinfo->bdrv->read_only = ro;
>> -    dinfo->bdrv->detect_zeroes = detect_zeroes;
>> +    dinfo->bdrv = bs;
>>       QTAILQ_INSERT_TAIL(&drives, dinfo, next);
>>   -    bdrv_set_on_error(dinfo->bdrv, on_read_error,
>> on_write_error);
>> -
>> -    /* disk I/O throttling */
>> -    if (throttle_enabled(&cfg)) {
>> -        bdrv_io_limits_enable(dinfo->bdrv);
>> -        bdrv_set_io_limits(dinfo->bdrv, &cfg);
>> -    }
>> -
>>       if (!file || !*file) {
>>           if (has_driver_specific_opts) {
>>               file = NULL;
>> @@ -502,7 +504,8 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>>       bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
>>         QINCREF(bs_opts);
>> -    ret = bdrv_open(&dinfo->bdrv, file, NULL, bs_opts, bdrv_flags, drv, &error);
>> +    ret = bdrv_open(&bs, file, NULL, bs_opts, bdrv_flags, drv, &error);
>> +    assert(bs == dinfo->bdrv);
>
> Well, this is guaranteed by bdrv_open(), but of course better having
> too many assertions than too few.

Indeed.  bdrv_open() is in another file, and its function comment
doesn't quite spell out this part of its contract.

Assertions do double-duty: they check assumptions are valid, and they
remind the reader of assumptions.  The second part can be useful even
when the first part is trivial.

> With or without g_new0:
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH 1/4] blockdev: Disentangle BlockDriverState and DriveInfo creation

[Benoît Canet]

The Friday 12 Sep 2014 à 21:26:21 (+0200), Markus Armbruster wrote :
> blockdev_init() mixes up BlockDriverState and DriveInfo initialization
> Finish the BlockDriverState job before starting to mess with
> DriveInfo.  Easier on the eyes.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  blockdev.c | 43 +++++++++++++++++++++++--------------------
>  1 file changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index b361fbb..5ec4635 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -301,6 +301,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>      int ro = 0;
>      int bdrv_flags = 0;
>      int on_read_error, on_write_error;
> +    BlockDriverState *bs;
>      DriveInfo *dinfo;
>      ThrottleConfig cfg;
>      int snapshot = 0;
> @@ -456,26 +457,27 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>      }
>  
>      /* init */
> +    bs = bdrv_new(qemu_opts_id(opts), errp);
> +    if (!bs) {
> +        goto early_err;
> +    }
> +    bs->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
> +    bs->read_only = ro;
> +    bs->detect_zeroes = detect_zeroes;
> +
> +    bdrv_set_on_error(bs, on_read_error, on_write_error);
> +
> +    /* disk I/O throttling */
> +    if (throttle_enabled(&cfg)) {
> +        bdrv_io_limits_enable(bs);
> +        bdrv_set_io_limits(bs, &cfg);
> +    }
> +
>      dinfo = g_malloc0(sizeof(*dinfo));
>      dinfo->id = g_strdup(qemu_opts_id(opts));
> -    dinfo->bdrv = bdrv_new(dinfo->id, &error);
> -    if (error) {
> -        error_propagate(errp, error);
> -        goto bdrv_new_err;
> -    }
> -    dinfo->bdrv->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
> -    dinfo->bdrv->read_only = ro;
> -    dinfo->bdrv->detect_zeroes = detect_zeroes;
> +    dinfo->bdrv = bs;
>      QTAILQ_INSERT_TAIL(&drives, dinfo, next);
>  
> -    bdrv_set_on_error(dinfo->bdrv, on_read_error, on_write_error);
> -
> -    /* disk I/O throttling */
> -    if (throttle_enabled(&cfg)) {
> -        bdrv_io_limits_enable(dinfo->bdrv);
> -        bdrv_set_io_limits(dinfo->bdrv, &cfg);
> -    }
> -
>      if (!file || !*file) {
>          if (has_driver_specific_opts) {
>              file = NULL;
> @@ -502,7 +504,8 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>      bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
>  
>      QINCREF(bs_opts);
> -    ret = bdrv_open(&dinfo->bdrv, file, NULL, bs_opts, bdrv_flags, drv, &error);
> +    ret = bdrv_open(&bs, file, NULL, bs_opts, bdrv_flags, drv, &error);
> +    assert(bs == dinfo->bdrv);
>  
>      if (ret < 0) {
>          error_setg(errp, "could not open disk image %s: %s",
> @@ -511,8 +514,9 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>          goto err;
>      }
>  
> -    if (bdrv_key_required(dinfo->bdrv))
> +    if (bdrv_key_required(bs)) {
>          autostart = 0;
> +    }
>  
>      QDECREF(bs_opts);
>      qemu_opts_del(opts);
> @@ -520,9 +524,8 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>      return dinfo;
>  
>  err:
> -    bdrv_unref(dinfo->bdrv);

> +    bdrv_unref(bs);
I would have moved this.

>      QTAILQ_REMOVE(&drives, dinfo, next);
> -bdrv_new_err:
>      g_free(dinfo->id);
>      g_free(dinfo);

To Here.

No functional difference but it would reflect it's goto chain role:
being in the reverse order of the allocations.

>  early_err:
> -- 
> 1.9.3

Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>
> 

Re: [Qemu-devel] [PATCH 2/4] block: Keep DriveInfo alive until BlockDriverState dies

[Benoît Canet]

The Friday 12 Sep 2014 à 21:26:22 (+0200), Markus Armbruster wrote :
> If the BDS's refcnt > 0, drive_del() destroys the DriveInfo, but not
> the BDS.  This can happen in three places:
> 
> * Device model destruction during unplug: blockdev_auto_del()
> 
> * Xen IDE unplug: pci_piix3_xen_ide_unplug()
> 
> * drive_del command when no device model is attached: do_drive_del()
> 
> The other callers of drive_del are on error paths where refcnt == 1.
> 
> If the user somehow manages to plug in a device model using a BDS that
> has gone through drive_del(), the legacy configuration passed in
> DriveInfo doesn't reach the device model, and automatic deletion on
> unplug doesn't work.  Worse, some device models such as scsi-disk
> crash when DriveInfo doesn't exist.
> 
> This is theoretical; I didn't research an actual reproducer.
> 
> Fix by keeping DriveInfo alive until its BDS dies.
> 
> This affects qemu_drive_opts: now you can't reuse the same ID for new
> drive options until the BDS dies.  Before, you could, but since the
> code always attempts to create a BDS with the same ID next, the
> enclosing operation "create a new drive" failed anyway.  Different
> error path, same result.
> 
> Unfortunately, the fix involves use of blockdev.c stuff from block.c,
> which is a layering violation.  Fortunately, my forthcoming
> BlockBackend work will get rid of it again.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block.c                   |  2 ++
>  blockdev.c                | 13 ++++++++-----
>  include/sysemu/blockdev.h |  1 +
>  stubs/Makefile.objs       |  1 +
>  stubs/blockdev.c          | 12 ++++++++++++
>  5 files changed, 24 insertions(+), 5 deletions(-)
>  create mode 100644 stubs/blockdev.c
> 
> diff --git a/block.c b/block.c
> index d06dd51..6faf36f 100644
> --- a/block.c
> +++ b/block.c
> @@ -29,6 +29,7 @@
>  #include "qemu/module.h"
>  #include "qapi/qmp/qjson.h"
>  #include "sysemu/sysemu.h"
> +#include "sysemu/blockdev.h"    /* FIXME layering violation */
>  #include "qemu/notify.h"
>  #include "block/coroutine.h"
>  #include "block/qapi.h"
> @@ -2110,6 +2111,7 @@ static void bdrv_delete(BlockDriverState *bs)
>      /* remove from list, if necessary */
>      bdrv_make_anon(bs);
>  
> +    drive_info_del(drive_get_by_blockdev(bs));
>      g_free(bs);
>  }
>  
> diff --git a/blockdev.c b/blockdev.c
> index 5ec4635..450f95c 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -216,11 +216,17 @@ static void bdrv_format_print(void *opaque, const char *name)
>  
>  void drive_del(DriveInfo *dinfo)
>  {
> +    bdrv_unref(dinfo->bdrv);
> +}
> +
> +void drive_info_del(DriveInfo *dinfo)
> +{
> +    if (!dinfo) {
> +        return;
> +    }
>      if (dinfo->opts) {
>          qemu_opts_del(dinfo->opts);
>      }
> -
> -    bdrv_unref(dinfo->bdrv);
>      g_free(dinfo->id);
>      QTAILQ_REMOVE(&drives, dinfo, next);
>      g_free(dinfo->serial);
> @@ -525,9 +531,6 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>  
>  err:
>      bdrv_unref(bs);
> -    QTAILQ_REMOVE(&drives, dinfo, next);
> -    g_free(dinfo->id);
> -    g_free(dinfo);
>  early_err:
>      qemu_opts_del(opts);
>  err_no_opts:
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index 23a5d10..abec381 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -56,6 +56,7 @@ QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
>                      const char *optstr);
>  DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type);
>  void drive_del(DriveInfo *dinfo);
> +void drive_info_del(DriveInfo *dinfo);
>  
>  /* device-hotplug */
>  
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 5e347d0..c0b1f6a 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -1,5 +1,6 @@
>  stub-obj-y += arch-query-cpu-def.o
>  stub-obj-y += bdrv-commit-all.o
> +stub-obj-y += blockdev.o
>  stub-obj-y += chr-baum-init.o
>  stub-obj-y += chr-msmouse.o
>  stub-obj-y += chr-testdev.o
> diff --git a/stubs/blockdev.c b/stubs/blockdev.c
> new file mode 100644
> index 0000000..5d0a79c
> --- /dev/null
> +++ b/stubs/blockdev.c
> @@ -0,0 +1,12 @@
> +#include <assert.h>
> +#include "sysemu/blockdev.h"
> +
> +DriveInfo *drive_get_by_blockdev(BlockDriverState *bs)
> +{
> +    return NULL;
> +}
> +
> +void drive_info_del(DriveInfo *dinfo)
> +{
> +    assert(!dinfo);
> +}
> -- 
> 1.9.3
>


Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>

Re: [Qemu-devel] [PATCH 1/4] blockdev: Disentangle BlockDriverState and DriveInfo creation

[Markus Armbruster]

Benoît Canet <benoit.canet@irqsave.net> writes:

> The Friday 12 Sep 2014 à 21:26:21 (+0200), Markus Armbruster wrote :
>> blockdev_init() mixes up BlockDriverState and DriveInfo initialization
>> Finish the BlockDriverState job before starting to mess with
>> DriveInfo.  Easier on the eyes.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  blockdev.c | 43 +++++++++++++++++++++++--------------------
>>  1 file changed, 23 insertions(+), 20 deletions(-)
>> 
>> diff --git a/blockdev.c b/blockdev.c
>> index b361fbb..5ec4635 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -301,6 +301,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>>      int ro = 0;
>>      int bdrv_flags = 0;
>>      int on_read_error, on_write_error;
>> +    BlockDriverState *bs;
>>      DriveInfo *dinfo;
>>      ThrottleConfig cfg;
>>      int snapshot = 0;
>> @@ -456,26 +457,27 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>>      }
>>  
>>      /* init */
>> +    bs = bdrv_new(qemu_opts_id(opts), errp);
>> +    if (!bs) {
>> +        goto early_err;
>> +    }
>> +    bs->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
>> +    bs->read_only = ro;
>> +    bs->detect_zeroes = detect_zeroes;
>> +
>> +    bdrv_set_on_error(bs, on_read_error, on_write_error);
>> +
>> +    /* disk I/O throttling */
>> +    if (throttle_enabled(&cfg)) {
>> +        bdrv_io_limits_enable(bs);
>> +        bdrv_set_io_limits(bs, &cfg);
>> +    }
>> +
>>      dinfo = g_malloc0(sizeof(*dinfo));
>>      dinfo->id = g_strdup(qemu_opts_id(opts));
>> -    dinfo->bdrv = bdrv_new(dinfo->id, &error);
>> -    if (error) {
>> -        error_propagate(errp, error);
>> -        goto bdrv_new_err;
>> -    }
>> -    dinfo->bdrv->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
>> -    dinfo->bdrv->read_only = ro;
>> -    dinfo->bdrv->detect_zeroes = detect_zeroes;
>> +    dinfo->bdrv = bs;
>>      QTAILQ_INSERT_TAIL(&drives, dinfo, next);
>>  
>> -    bdrv_set_on_error(dinfo->bdrv, on_read_error, on_write_error);
>> -
>> -    /* disk I/O throttling */
>> -    if (throttle_enabled(&cfg)) {
>> -        bdrv_io_limits_enable(dinfo->bdrv);
>> -        bdrv_set_io_limits(dinfo->bdrv, &cfg);
>> -    }
>> -
>>      if (!file || !*file) {
>>          if (has_driver_specific_opts) {
>>              file = NULL;
>> @@ -502,7 +504,8 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>>      bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
>>  
>>      QINCREF(bs_opts);
>> -    ret = bdrv_open(&dinfo->bdrv, file, NULL, bs_opts, bdrv_flags, drv, &error);
>> +    ret = bdrv_open(&bs, file, NULL, bs_opts, bdrv_flags, drv, &error);
>> +    assert(bs == dinfo->bdrv);
>>  
>>      if (ret < 0) {
>>          error_setg(errp, "could not open disk image %s: %s",
>> @@ -511,8 +514,9 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>>          goto err;
>>      }
>>  
>> -    if (bdrv_key_required(dinfo->bdrv))
>> +    if (bdrv_key_required(bs)) {
>>          autostart = 0;
>> +    }
>>  
>>      QDECREF(bs_opts);
>>      qemu_opts_del(opts);
>> @@ -520,9 +524,8 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>>      return dinfo;
>>  
>>  err:
>> -    bdrv_unref(dinfo->bdrv);
>
>> +    bdrv_unref(bs);
> I would have moved this.
>
>>      QTAILQ_REMOVE(&drives, dinfo, next);
>> -bdrv_new_err:
>>      g_free(dinfo->id);
>>      g_free(dinfo);
>
> To Here.
>
> No functional difference but it would reflect it's goto chain role:
> being in the reverse order of the allocations.

You're right.

In the BlockBackend series, this becomes just

    err:
        blk_unref(blk);
    early_err:

so the disorder is just temporary.  I'll change it anyway if I have to
respin for some other reason.

>>  early_err:
>> -- 
>> 1.9.3
>
> Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>

Thanks!

Re: [Qemu-devel] [PATCH 0/4] Miscellaneous block fixes

[Kevin Wolf]

Am 12.09.2014 um 21:26 hat Markus Armbruster geschrieben:
> More random crap I ran into while iterating on BlockBackends.
> 
> Markus Armbruster (4):
>   blockdev: Disentangle BlockDriverState and DriveInfo creation
>   block: Keep DriveInfo alive until BlockDriverState dies
>   qemu-nbd: Destroy the BlockDriverState properly
>   block: Improve message for device name clashing with node name

Thanks, applied all to the block branch.

Kevin