| Message ID | 1338900668-6093-3-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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!
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
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 :-)
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.
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
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.
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
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.
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; }
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(-)