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

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

[Markus Armbruster]

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

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      |    5 +++--
 2 files changed, 9 insertions(+), 9 deletions(-)

-- 
1.7.6.5

[Qemu-devel] [PATCH 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 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      |    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;
     }
 
-- 
1.7.6.5

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

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

[Markus Armbruster]

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!

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

[Peter Maydell]

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

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

[Stefano Stabellini]

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 :-)

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

[Markus Armbruster]

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.

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

[Peter Maydell]

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

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

[Markus Armbruster]

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.

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

[Peter Maydell]

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

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

[Markus Armbruster]

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.