Patchwork [2/2] xen: Don't peek behind the BlockDriverState abstraction

login
register
mail settings
Submitter Markus Armbruster
Date June 5, 2012, 12:51 p.m.
Message ID <1338900668-6093-3-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/163070/
State New
Headers show

Comments

Markus Armbruster - June 5, 2012, 12:51 p.m.
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      |    5 +++--
 2 files changed, 6 insertions(+), 5 deletions(-)
Stefano Stabellini - June 5, 2012, 4:59 p.m.
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 <armbru@redhat.com>
> ---
>  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.
Markus Armbruster - June 6, 2012, 11:39 a.m.
Stefano Stabellini <stefano.stabellini@eu.citrix.com> 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 <armbru@redhat.com>
>> ---
>>  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!
Peter Maydell - June 6, 2012, 11:52 a.m.
On 5 June 2012 13:51, Markus Armbruster <armbru@redhat.com> wrote:
> @@ -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];

Fixed length array with a hardcoded magic number size ?
If the block layer guarantees that format names are going to be
less than 128 bytes it ought to provide a suitable #define for
people to set array sizes to...

-- PMM
Stefano Stabellini - June 6, 2012, 12:23 p.m.
On Wed, 6 Jun 2012, Markus Armbruster wrote:
> Stefano Stabellini <stefano.stabellini@eu.citrix.com> 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 <armbru@redhat.com>
> >> ---
> >>  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?
 
Yes, please :-)
Markus Armbruster - June 6, 2012, 12:55 p.m.
Peter Maydell <peter.maydell@linaro.org> writes:

> On 5 June 2012 13:51, Markus Armbruster <armbru@redhat.com> wrote:
>> @@ -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];
>
> Fixed length array with a hardcoded magic number size ?
> If the block layer guarantees that format names are going to be
> less than 128 bytes it ought to provide a suitable #define for
> people to set array sizes to...

Maybe it should, but it doesn't.  Does it really matter in this
particular case?  If somebody insists on giving his driver a name longer
than 127 characters, we'll silently log it truncated, that's all.
Peter Maydell - June 7, 2012, 12:07 a.m.
On 6 June 2012 13:55, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On 5 June 2012 13:51, Markus Armbruster <armbru@redhat.com> wrote:
>>> @@ -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];
>>
>> Fixed length array with a hardcoded magic number size ?
>> If the block layer guarantees that format names are going to be
>> less than 128 bytes it ought to provide a suitable #define for
>> people to set array sizes to...
>
> Maybe it should, but it doesn't.  Does it really matter in this
> particular case?  If somebody insists on giving his driver a name longer
> than 127 characters, we'll silently log it truncated, that's all.

I think it matters in the general case, yours is just the first
usage of this API which has caught my attention. We should fix
the API before adding more uses of it (at the moment it seems to
be only used in two places).

Alternatively, we could have the function return a const char* rather
than taking a buffer to be filled in.

-- PMM
Markus Armbruster - June 7, 2012, 8:13 a.m.
Peter Maydell <peter.maydell@linaro.org> writes:

> On 6 June 2012 13:55, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> On 5 June 2012 13:51, Markus Armbruster <armbru@redhat.com> wrote:
>>>> @@ -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];
>>>
>>> Fixed length array with a hardcoded magic number size ?
>>> If the block layer guarantees that format names are going to be
>>> less than 128 bytes it ought to provide a suitable #define for
>>> people to set array sizes to...
>>
>> Maybe it should, but it doesn't.  Does it really matter in this
>> particular case?  If somebody insists on giving his driver a name longer
>> than 127 characters, we'll silently log it truncated, that's all.
>
> I think it matters in the general case, yours is just the first
> usage of this API which has caught my attention. We should fix
> the API before adding more uses of it (at the moment it seems to
> be only used in two places).

What kind of fix do you have in mind?

> Alternatively, we could have the function return a const char* rather
> than taking a buffer to be filled in.

Trades the theoretical string truncation problem for a theoretical
dangling pointer problem.
Peter Maydell - June 7, 2012, 12:49 p.m.
On 7 June 2012 09:13, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> I think it matters in the general case, yours is just the first
>> usage of this API which has caught my attention. We should fix
>> the API before adding more uses of it (at the moment it seems to
>> be only used in two places).
>
> What kind of fix do you have in mind?

Option 1: the function should guarantee that it won't ever
use more than X bytes of buffer, and provide a #define that
corresponds to that maximum length.

Option 2: this: vv

>> Alternatively, we could have the function return a const char* rather
>> than taking a buffer to be filled in.
>
> Trades the theoretical string truncation problem for a theoretical
> dangling pointer problem.

Yes, you'd need to come up with some reasonable lifecycle
management if you took this option.

-- PMM
Markus Armbruster - June 13, 2012, 7:35 a.m.
Peter Maydell <peter.maydell@linaro.org> writes:

> On 7 June 2012 09:13, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>> I think it matters in the general case, yours is just the first
>>> usage of this API which has caught my attention. We should fix
>>> the API before adding more uses of it (at the moment it seems to
>>> be only used in two places).
>>
>> What kind of fix do you have in mind?
>
> Option 1: the function should guarantee that it won't ever
> use more than X bytes of buffer, and provide a #define that
> corresponds to that maximum length.
>
> Option 2: this: vv
>
>>> Alternatively, we could have the function return a const char* rather
>>> than taking a buffer to be filled in.
>>
>> Trades the theoretical string truncation problem for a theoretical
>> dangling pointer problem.
>
> Yes, you'd need to come up with some reasonable lifecycle
> management if you took this option.

Actually, the lifecycle is trivial, because it's a *driver* name, and
drivers never go away.  Taking option 2.

Patch

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;
     }