Patchwork [07/13] blockdev: Means to destroy blockdev only if made with drive_init()

login
register
mail settings
Submitter Markus Armbruster
Date June 2, 2010, 4:55 p.m.
Message ID <1275497729-13120-8-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/54397/
State New
Headers show

Comments

Markus Armbruster - June 2, 2010, 4:55 p.m.
All drives are still made that way.  They get destroyed along with
their device.  That's inappropriate for the alternative way to make
blockdevs that will appear later in this series.  These won't have a
DriveInfo.

blockdev_detach() destroys the blockdev only if it has a DriveInfo.

blockdev_attach() does nothing for now.  It'll be fleshed out later.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c |   35 +++++++++++++++++++++++++++++++++++
 blockdev.h |    7 +++++++
 2 files changed, 42 insertions(+), 0 deletions(-)
Kevin Wolf - June 10, 2010, 2:19 p.m.
Am 02.06.2010 18:55, schrieb Markus Armbruster:
> All drives are still made that way.  They get destroyed along with
> their device.  That's inappropriate for the alternative way to make
> blockdevs that will appear later in this series.  These won't have a
> DriveInfo.
> 
> blockdev_detach() destroys the blockdev only if it has a DriveInfo.
> 
> blockdev_attach() does nothing for now.  It'll be fleshed out later.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  blockdev.c |   35 +++++++++++++++++++++++++++++++++++
>  blockdev.h |    7 +++++++
>  2 files changed, 42 insertions(+), 0 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index ace74e4..f90d4fc 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1,8 +1,12 @@
>  /*
>   * QEMU host block devices
>   *
> + * Copyright (C) 2010 Red Hat Inc.
>   * Copyright (c) 2003-2008 Fabrice Bellard
>   *
> + * Authors:
> + *  Markus Armbruster <armbru@redhat.com>,
> + *
>   * This work is licensed under the terms of the GNU GPL, version 2 or
>   * later.  See the COPYING file in the top-level directory.
>   */
> @@ -17,6 +21,37 @@
>  
>  static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives);
>  
> +static int blockdev_del_dinfo(BlockDriverState *bs)
> +{
> +    DriveInfo *dinfo, *next_dinfo;
> +    int res = 0;
> +
> +    QTAILQ_FOREACH_SAFE(dinfo, &drives, next, next_dinfo) {
> +        if (dinfo->bdrv == bs) {
> +            qemu_opts_del(dinfo->opts);
> +            QTAILQ_REMOVE(&drives, dinfo, next);
> +            qemu_free(dinfo);
> +            res = 1;
> +        }
> +    }
> +
> +    return res;

Can it happen that a BlockDriverState belongs to multiple DriveInfos? If
no, why not returning in the loop? Wouldn't need a FOREACH_SAFE then, too.

It's not worth respinning because of this one, but there were more
comments and I think you'll send a v2 for the actual -blockdev option
anyway once we have decided how to do it.

I have applied patches 1 to 6 now, and I think I could safely go on
until patch 9 if the minor improvements that were mentioned in comments
are made. I'd ignore patch 10 to 13 for now.

Is this what you would have expected or should I handle anything in a
different way?

Kevin
Markus Armbruster - June 10, 2010, 4 p.m.
Kevin Wolf <kwolf@redhat.com> writes:

> Am 02.06.2010 18:55, schrieb Markus Armbruster:
>> All drives are still made that way.  They get destroyed along with
>> their device.  That's inappropriate for the alternative way to make
>> blockdevs that will appear later in this series.  These won't have a
>> DriveInfo.
>> 
>> blockdev_detach() destroys the blockdev only if it has a DriveInfo.
>> 
>> blockdev_attach() does nothing for now.  It'll be fleshed out later.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  blockdev.c |   35 +++++++++++++++++++++++++++++++++++
>>  blockdev.h |    7 +++++++
>>  2 files changed, 42 insertions(+), 0 deletions(-)
>> 
>> diff --git a/blockdev.c b/blockdev.c
>> index ace74e4..f90d4fc 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -1,8 +1,12 @@
>>  /*
>>   * QEMU host block devices
>>   *
>> + * Copyright (C) 2010 Red Hat Inc.
>>   * Copyright (c) 2003-2008 Fabrice Bellard
>>   *
>> + * Authors:
>> + *  Markus Armbruster <armbru@redhat.com>,
>> + *
>>   * This work is licensed under the terms of the GNU GPL, version 2 or
>>   * later.  See the COPYING file in the top-level directory.
>>   */
>> @@ -17,6 +21,37 @@
>>  
>>  static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives);
>>  
>> +static int blockdev_del_dinfo(BlockDriverState *bs)
>> +{
>> +    DriveInfo *dinfo, *next_dinfo;
>> +    int res = 0;
>> +
>> +    QTAILQ_FOREACH_SAFE(dinfo, &drives, next, next_dinfo) {
>> +        if (dinfo->bdrv == bs) {
>> +            qemu_opts_del(dinfo->opts);
>> +            QTAILQ_REMOVE(&drives, dinfo, next);
>> +            qemu_free(dinfo);
>> +            res = 1;
>> +        }
>> +    }
>> +
>> +    return res;
>
> Can it happen that a BlockDriverState belongs to multiple DriveInfos? If
> no, why not returning in the loop? Wouldn't need a FOREACH_SAFE then, too.

No, that shouldn't happen.  Defensive coding, I don't want to leave
dinfos with dangling dinfo->bdrv around.  Maybe I should put an
assert(!res) before the qemu_opts_del().  Or just forget about it, and
simplify like you suggest.

> It's not worth respinning because of this one, but there were more
> comments and I think you'll send a v2 for the actual -blockdev option
> anyway once we have decided how to do it.
>
> I have applied patches 1 to 6 now, and I think I could safely go on
> until patch 9 if the minor improvements that were mentioned in comments
> are made. I'd ignore patch 10 to 13 for now.
>
> Is this what you would have expected or should I handle anything in a
> different way?

No, that suits me fine.  I definitely need to respin from part 8 on
(commit message too terse).

Patch

diff --git a/blockdev.c b/blockdev.c
index ace74e4..f90d4fc 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1,8 +1,12 @@ 
 /*
  * QEMU host block devices
  *
+ * Copyright (C) 2010 Red Hat Inc.
  * Copyright (c) 2003-2008 Fabrice Bellard
  *
+ * Authors:
+ *  Markus Armbruster <armbru@redhat.com>,
+ *
  * This work is licensed under the terms of the GNU GPL, version 2 or
  * later.  See the COPYING file in the top-level directory.
  */
@@ -17,6 +21,37 @@ 
 
 static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives);
 
+static int blockdev_del_dinfo(BlockDriverState *bs)
+{
+    DriveInfo *dinfo, *next_dinfo;
+    int res = 0;
+
+    QTAILQ_FOREACH_SAFE(dinfo, &drives, next, next_dinfo) {
+        if (dinfo->bdrv == bs) {
+            qemu_opts_del(dinfo->opts);
+            QTAILQ_REMOVE(&drives, dinfo, next);
+            qemu_free(dinfo);
+            res = 1;
+        }
+    }
+
+    return res;
+}
+
+int blockdev_attach(BlockDriverState *bs, DeviceState *qdev)
+{
+    return 0;
+}
+
+void blockdev_detach(BlockDriverState *bs, DeviceState *qdev)
+{
+    /* Backward compatibility: auto-destroy block device made with
+     * drive_init() when its guest device detaches */
+    if (blockdev_del_dinfo(bs)) {
+        bdrv_delete(bs);
+    }
+}
+
 QemuOpts *drive_add(const char *file, const char *fmt, ...)
 {
     va_list ap;
diff --git a/blockdev.h b/blockdev.h
index 23ea576..8607086 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -1,8 +1,12 @@ 
 /*
  * QEMU host block devices
  *
+ * Copyright (C) 2010 Red Hat Inc.
  * Copyright (c) 2003-2008 Fabrice Bellard
  *
+ * Authors:
+ *  Markus Armbruster <armbru@redhat.com>,
+ *
  * This work is licensed under the terms of the GNU GPL, version 2 or
  * later.  See the COPYING file in the top-level directory.
  */
@@ -13,6 +17,9 @@ 
 #include "block.h"
 #include "qemu-queue.h"
 
+int blockdev_attach(BlockDriverState *, DeviceState *);
+void blockdev_detach(BlockDriverState *, DeviceState *);
+
 typedef enum {
     IF_NONE,
     IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,