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

[Stefano Stabellini]

On Tue, 5 Jun 2012, Markus Armbruster wrote:
> 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 <address@hidden>
> ---
>  hw/xen_devconfig.c |    6 +++---
>  hw/xen_disk.c      |    5 +++--
>  2 files changed, 6 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..c73b71e 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"
> @@ -554,6 +553,7 @@ static int blk_init(struct XenDevice *xendev)
>  {
>      struct XenBlkDev *blkdev = container_of(xendev, struct XenBlkDev, 
> xendev);
>      int index, qflags, info = 0;
> +    char fmt_name[128];
>  
>      /* read xenstore entries */
>      if (blkdev->params == NULL) {
> @@ -634,9 +634,10 @@ static int blk_init(struct XenDevice *xendev)
>      blkdev->file_blk  = BLOCK_SIZE;
>      blkdev->file_size = bdrv_getlength(blkdev->bs);
>      if (blkdev->file_size < 0) {
> +        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;
>      }

You might as well move fmt_name here because it is only used if
blkdev->file_size < 0.

Apart from this minor nitpick, both patches are OK.