[Qemu-devel] [PATCH v3 0/3] xen: Clean up BlockDriverState use

[Qemu-devel] [PATCH v3 0/3] xen: Clean up BlockDriverState use

[Markus Armbruster]

Xen compile-tested only.  Stefano, please give it a whirl.

v3: Replaced bdrv_get_format() on Peter's request

v2: Moved a declaration to an inner block on Stefano's request.

Markus Armbruster (3):
  block: Replace bdrv_get_format() by bdrv_get_format_name()
  xen: Don't change -drive if=xen device name during machine init
  xen: Don't peek behind the BlockDriverState abstraction

 block.c            |   11 ++++-------
 block.h            |    2 +-
 hw/xen_devconfig.c |   13 ++++++-------
 hw/xen_disk.c      |    3 +--
 qemu-img.c         |    5 ++---
 5 files changed, 14 insertions(+), 20 deletions(-)

-- 
1.7.6.5

[Qemu-devel] [PATCH v3 1/3] block: Replace bdrv_get_format() by bdrv_get_format_name()

[Markus Armbruster]

So callers don't need to know anything about maximum name length.
Returning a pointer is safe, because the name string lives as long as
the block driver it names, and block drivers don't die.

Requested by Peter Maydell.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block.c    |   11 ++++-------
 block.h    |    2 +-
 qemu-img.c |    5 ++---
 3 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/block.c b/block.c
index 7547051..7c04948 100644
--- a/block.c
+++ b/block.c
@@ -1032,7 +1032,8 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
      * swapping bs_new and bs_top contents. */
     tmp.backing_hd = bs_new;
     pstrcpy(tmp.backing_file, sizeof(tmp.backing_file), bs_top->filename);
-    bdrv_get_format(bs_top, tmp.backing_format, sizeof(tmp.backing_format));
+    pstrcpy(tmp.backing_format, sizeof(tmp.backing_format),
+            bs_top->drv ? bs_top->drv->format_name : "");
 
     /* swap contents of the fixed new bs and the current top */
     *bs_new = *bs_top;
@@ -2413,13 +2414,9 @@ int bdrv_set_key(BlockDriverState *bs, const char *key)
     return ret;
 }
 
-void bdrv_get_format(BlockDriverState *bs, char *buf, int buf_size)
+const char *bdrv_get_format_name(BlockDriverState *bs)
 {
-    if (!bs->drv) {
-        buf[0] = '\0';
-    } else {
-        pstrcpy(buf, buf_size, bs->drv->format_name);
-    }
+    return bs->drv ? bs->drv->format_name : NULL;
 }
 
 void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
diff --git a/block.h b/block.h
index 7408acc..3b28276 100644
--- a/block.h
+++ b/block.h
@@ -284,7 +284,7 @@ int bdrv_is_inserted(BlockDriverState *bs);
 int bdrv_media_changed(BlockDriverState *bs);
 void bdrv_lock_medium(BlockDriverState *bs, bool locked);
 void bdrv_eject(BlockDriverState *bs, bool eject_flag);
-void bdrv_get_format(BlockDriverState *bs, char *buf, int buf_size);
+const char *bdrv_get_format_name(BlockDriverState *bs);
 BlockDriverState *bdrv_find(const char *name);
 BlockDriverState *bdrv_next(BlockDriverState *bs);
 void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs),
diff --git a/qemu-img.c b/qemu-img.c
index c8a70ff..fda8a35 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1078,7 +1078,7 @@ static int img_info(int argc, char **argv)
     int c;
     const char *filename, *fmt;
     BlockDriverState *bs;
-    char fmt_name[128], size_buf[128], dsize_buf[128];
+    char size_buf[128], dsize_buf[128];
     uint64_t total_sectors;
     int64_t allocated_size;
     char backing_filename[1024];
@@ -1110,7 +1110,6 @@ static int img_info(int argc, char **argv)
     if (!bs) {
         return 1;
     }
-    bdrv_get_format(bs, fmt_name, sizeof(fmt_name));
     bdrv_get_geometry(bs, &total_sectors);
     get_human_readable_size(size_buf, sizeof(size_buf), total_sectors * 512);
     allocated_size = bdrv_get_allocated_file_size(bs);
@@ -1124,7 +1123,7 @@ static int img_info(int argc, char **argv)
            "file format: %s\n"
            "virtual size: %s (%" PRId64 " bytes)\n"
            "disk size: %s\n",
-           filename, fmt_name, size_buf,
+           filename, bdrv_get_format_name(bs), size_buf,
            (total_sectors * 512),
            dsize_buf);
     if (bdrv_is_encrypted(bs)) {
-- 
1.7.6.5

[Qemu-devel] [PATCH v3 2/3] xen: Don't change -drive if=xen device name during machine init

[Markus Armbruster]

A "top" BlockDriverState has a non-empty device_name.  If the user
doesn't specify one with -drive parameter id, the system supplies a
default name.

xen_config_dev_blk() changes this name, during machine initialization.
Naughty.  Don't do that.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/xen_devconfig.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/xen_devconfig.c b/hw/xen_devconfig.c
index 41accbb..7b7b0a2 100644
--- a/hw/xen_devconfig.c
+++ b/hw/xen_devconfig.c
@@ -94,16 +94,15 @@ static int xen_config_dev_all(char *fe, char *be)
 
 int xen_config_dev_blk(DriveInfo *disk)
 {
-    char fe[256], be[256];
+    char fe[256], be[256], device_name[32];
     int vdev = 202 * 256 + 16 * disk->unit;
     int cdrom = disk->media_cd;
     const char *devtype = cdrom ? "cdrom" : "disk";
     const char *mode    = cdrom ? "r"     : "w";
 
-    snprintf(disk->bdrv->device_name, sizeof(disk->bdrv->device_name),
-	     "xvd%c", 'a' + disk->unit);
+    snprintf(device_name, sizeof(device_name), "xvd%c", 'a' + disk->unit);
     xen_be_printf(NULL, 1, "config disk %d [%s]: %s\n",
-                  disk->unit, disk->bdrv->device_name, disk->bdrv->filename);
+                  disk->unit, device_name, disk->bdrv->filename);
     xen_config_dev_dirs("vbd", "qdisk", vdev, fe, be, sizeof(fe));
 
     /* frontend */
@@ -111,7 +110,7 @@ int xen_config_dev_blk(DriveInfo *disk)
     xenstore_write_str(fe, "device-type",     devtype);
 
     /* backend */
-    xenstore_write_str(be, "dev",             disk->bdrv->device_name);
+    xenstore_write_str(be, "dev",             device_name);
     xenstore_write_str(be, "type",            "file");
     xenstore_write_str(be, "params",          disk->bdrv->filename);
     xenstore_write_str(be, "mode",            mode);
-- 
1.7.6.5

[Qemu-devel] [PATCH v3 3/3] xen: Don't peek behind the BlockDriverState abstraction

[Markus Armbruster]

First offender is xen_config_dev_blk()'s use of disk->bdrv->filename.
Get the filename from disk->opts instead.  Same result, except for
snapshots: there, we now get the filename specified by the user
instead of the name of the temporary image created by bdrv_open().
Should be an improvement.

Second offender is blk_init()'s use of blkdev->bs->drv->format_name.
Simply use the appropriate interface to get the format name.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/xen_devconfig.c |    6 +++---
 hw/xen_disk.c      |    3 +--
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/xen_devconfig.c b/hw/xen_devconfig.c
index 7b7b0a2..0928613 100644
--- a/hw/xen_devconfig.c
+++ b/hw/xen_devconfig.c
@@ -1,6 +1,5 @@
 #include "xen_backend.h"
 #include "blockdev.h"
-#include "block_int.h" /* XXX */
 
 /* ------------------------------------------------------------- */
 
@@ -99,10 +98,11 @@ int xen_config_dev_blk(DriveInfo *disk)
     int cdrom = disk->media_cd;
     const char *devtype = cdrom ? "cdrom" : "disk";
     const char *mode    = cdrom ? "r"     : "w";
+    const char *filename = qemu_opt_get(disk->opts, "file");
 
     snprintf(device_name, sizeof(device_name), "xvd%c", 'a' + disk->unit);
     xen_be_printf(NULL, 1, "config disk %d [%s]: %s\n",
-                  disk->unit, device_name, disk->bdrv->filename);
+                  disk->unit, device_name, filename);
     xen_config_dev_dirs("vbd", "qdisk", vdev, fe, be, sizeof(fe));
 
     /* frontend */
@@ -112,7 +112,7 @@ int xen_config_dev_blk(DriveInfo *disk)
     /* backend */
     xenstore_write_str(be, "dev",             device_name);
     xenstore_write_str(be, "type",            "file");
-    xenstore_write_str(be, "params",          disk->bdrv->filename);
+    xenstore_write_str(be, "params",          filename);
     xenstore_write_str(be, "mode",            mode);
 
     /* common stuff */
diff --git a/hw/xen_disk.c b/hw/xen_disk.c
index de7e8a4..fb68ed9 100644
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -40,7 +40,6 @@
 #include <xen/io/xenbus.h>
 
 #include "hw.h"
-#include "block_int.h"
 #include "qemu-char.h"
 #include "xen_blkif.h"
 #include "xen_backend.h"
@@ -650,7 +649,7 @@ static int blk_init(struct XenDevice *xendev)
     if (blkdev->file_size < 0) {
         xen_be_printf(&blkdev->xendev, 1, "bdrv_getlength: %d (%s) | drv %s\n",
                       (int)blkdev->file_size, strerror(-blkdev->file_size),
-                      blkdev->bs->drv ? blkdev->bs->drv->format_name : "-");
+                      bdrv_get_format_name(blkdev->bs) ?: "-");
         blkdev->file_size = 0;
     }
 
-- 
1.7.6.5

Re: [Qemu-devel] [PATCH v3 0/3] xen: Clean up BlockDriverState use

[Stefano Stabellini]

On Wed, 13 Jun 2012, Markus Armbruster wrote:
> Xen compile-tested only.  Stefano, please give it a whirl.
> 
> v3: Replaced bdrv_get_format() on Peter's request
> 
> v2: Moved a declaration to an inner block on Stefano's request.
> 
> Markus Armbruster (3):
>   block: Replace bdrv_get_format() by bdrv_get_format_name()
>   xen: Don't change -drive if=xen device name during machine init
>   xen: Don't peek behind the BlockDriverState abstraction
> 
>  block.c            |   11 ++++-------
>  block.h            |    2 +-
>  hw/xen_devconfig.c |   13 ++++++-------
>  hw/xen_disk.c      |    3 +--
>  qemu-img.c         |    5 ++---
>  5 files changed, 14 insertions(+), 20 deletions(-)


You can stick my acked-by on patch 2/3 and 3/3.
Given that patch 1/3 is a generic block patch, I think that this series
should go through Kevin.

Re: [Qemu-devel] [PATCH v3 0/3] xen: Clean up BlockDriverState use

[Markus Armbruster]

Stefano Stabellini <stefano.stabellini@eu.citrix.com> writes:

> On Wed, 13 Jun 2012, Markus Armbruster wrote:
>> Xen compile-tested only.  Stefano, please give it a whirl.
>> 
>> v3: Replaced bdrv_get_format() on Peter's request
>> 
>> v2: Moved a declaration to an inner block on Stefano's request.
>> 
>> Markus Armbruster (3):
>>   block: Replace bdrv_get_format() by bdrv_get_format_name()
>>   xen: Don't change -drive if=xen device name during machine init
>>   xen: Don't peek behind the BlockDriverState abstraction
>> 
>>  block.c            |   11 ++++-------
>>  block.h            |    2 +-
>>  hw/xen_devconfig.c |   13 ++++++-------
>>  hw/xen_disk.c      |    3 +--
>>  qemu-img.c         |    5 ++---
>>  5 files changed, 14 insertions(+), 20 deletions(-)
>
>
> You can stick my acked-by on patch 2/3 and 3/3.
> Given that patch 1/3 is a generic block patch, I think that this series
> should go through Kevin.

Makes sense.  Thanks for your help!

Re: [Qemu-devel] [PATCH v3 0/3] xen: Clean up BlockDriverState use

[Kevin Wolf]

Am 13.06.2012 13:03, schrieb Stefano Stabellini:
> On Wed, 13 Jun 2012, Markus Armbruster wrote:
>> Xen compile-tested only.  Stefano, please give it a whirl.
>>
>> v3: Replaced bdrv_get_format() on Peter's request
>>
>> v2: Moved a declaration to an inner block on Stefano's request.
>>
>> Markus Armbruster (3):
>>   block: Replace bdrv_get_format() by bdrv_get_format_name()
>>   xen: Don't change -drive if=xen device name during machine init
>>   xen: Don't peek behind the BlockDriverState abstraction
>>
>>  block.c            |   11 ++++-------
>>  block.h            |    2 +-
>>  hw/xen_devconfig.c |   13 ++++++-------
>>  hw/xen_disk.c      |    3 +--
>>  qemu-img.c         |    5 ++---
>>  5 files changed, 14 insertions(+), 20 deletions(-)
> 
> 
> You can stick my acked-by on patch 2/3 and 3/3.
> Given that patch 1/3 is a generic block patch, I think that this series
> should go through Kevin.

Thanks, applied all to the block branch.

Kevin