[Qemu-devel] [PATCH v2 0/2] xen: Clean up BlockDriverState use

[Qemu-devel] [PATCH v2 0/2] xen: Clean up BlockDriverState use

[Markus Armbruster]

Compile tested only.  Stefano, please give it a whirl.

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

Markus Armbruster (2):
  xen: Don't change -drive if=xen device name during machine init
  xen: Don't peek behind the BlockDriverState abstraction

 hw/xen_devconfig.c |   13 ++++++-------
 hw/xen_disk.c      |    6 ++++--
 2 files changed, 10 insertions(+), 9 deletions(-)

-- 
1.7.6.5

[Qemu-devel] [PATCH v2 1/2] 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 v2 2/2] 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      |    6 ++++--
 2 files changed, 7 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 07594bc..52131cb 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"
@@ -634,9 +633,12 @@ static int blk_init(struct XenDevice *xendev)
     blkdev->file_blk  = BLOCK_SIZE;
     blkdev->file_size = bdrv_getlength(blkdev->bs);
     if (blkdev->file_size < 0) {
+        char fmt_name[128];
+
+        bdrv_get_format(blkdev->bs, fmt_name, sizeof(fmt_name));
         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 : "-");
+                      fmt_name[0] ? fmt_name : "-");
         blkdev->file_size = 0;
     }
 
-- 
1.7.6.5

Re: [Qemu-devel] [PATCH v2 0/2] xen: Clean up BlockDriverState use

[Stefano Stabellini]

On Wed, 6 Jun 2012, Markus Armbruster wrote:
> Compile tested only.  Stefano, please give it a whirl.
> 
> v2: Moved a declaration to an inner block on Stefano's request.
> 
> Markus Armbruster (2):
>   xen: Don't change -drive if=xen device name during machine init
>   xen: Don't peek behind the BlockDriverState abstraction
> 
>  hw/xen_devconfig.c |   13 ++++++-------
>  hw/xen_disk.c      |    6 ++++--
>  2 files changed, 10 insertions(+), 9 deletions(-)

I am OK with this series, however how do you intend to address Peter's
comments?