qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH 2/2] xen: Don't peek behind the BlockDriverState abstraction
Date: Wed, 06 Jun 2012 13:39:51 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux)

Stefano Stabellini <address@hidden> writes:

> 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.

Matter of taste, and you're the maintainer.  Want me to respin?

> Apart from this minor nitpick, both patches are OK.

Thanks!



reply via email to

[Prev in Thread] Current Thread [Next in Thread]