Am 26.01.2015 um 16:00 hat Max Reitz geschrieben: > Signed-off-by: Max Reitz <mre...@redhat.com> > --- > hw/block/xen_disk.c | 28 ++++++++++++---------------- > 1 file changed, 12 insertions(+), 16 deletions(-) > > diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c > index 21842a0..1b0257c 100644 > --- a/hw/block/xen_disk.c > +++ b/hw/block/xen_disk.c > @@ -40,6 +40,8 @@ > #include "xen_blkif.h" > #include "sysemu/blockdev.h" > #include "sysemu/block-backend.h" > +#include "qapi/qmp/qdict.h" > +#include "qapi/qmp/qstring.h" > > /* ------------------------------------------------------------- */ > > @@ -897,30 +899,24 @@ static int blk_connect(struct XenDevice *xendev) > blkdev->dinfo = drive_get(IF_XEN, 0, index); > if (!blkdev->dinfo) { > Error *local_err = NULL; > - BlockBackend *blk; > - BlockDriver *drv; > - BlockDriverState *bs; > + QDict *options = NULL; > > - /* setup via xenbus -> create new block driver instance */ > - xen_be_printf(&blkdev->xendev, 2, "create new bdrv (xenbus > setup)\n"); > - blk = blk_new_with_bs(blkdev->dev, NULL); > - if (!blk) { > - return -1; > + if (strcmp(blkdev->fileproto, "<unset>")) {
xen_disk's usage of the string "<unset>" to mark configurations where no driver is specific is quite ugly. I think it's possible for users to pass this string as the driver name, and we would end up probing the driver instead of returning an error. Which was actually how any invalid driver name was handled before this patch: bdrv_find_whitelisted_format() would return NULL, and instead of erroring out, NULL would be passed to bdrv_open(), which probed the driver then. This patch improves the situation: Now any value that is not "<unset>" is passed as the "driver" option, so invalid drivers will produce an error now. Should be mentioned in the commit message. However, if the user passes "<unset>", that still means probing. Ugly. Not sure why blkdev->fileproto == NULL wasn't used to specify that no driver was given. We could change that in a patch before this one if we wanted to clean it up. Or we could just feel reassured that xen_disk is horrible code, that this patch already fixes most of it and leave it alone. > + options = qdict_new(); > + qdict_put_obj(options, "driver", > + QOBJECT(qstring_from_str(blkdev->fileproto))); > } > - blkdev->blk = blk; > > - bs = blk_bs(blk); > - drv = bdrv_find_whitelisted_format(blkdev->fileproto, readonly); > - if (bdrv_open(&bs, blkdev->filename, NULL, NULL, qflags, > - drv, &local_err) != 0) { > + /* setup via xenbus -> create new block driver instance */ > + xen_be_printf(&blkdev->xendev, 2, "create new bdrv (xenbus > setup)\n"); > + blkdev->blk = blk_new_open(blkdev->dev, blkdev->filename, NULL, > options, > + qflags, &local_err); > + if (!blkdev->blk) { > xen_be_printf(&blkdev->xendev, 0, "error: %s\n", > error_get_pretty(local_err)); > error_free(local_err); > - blk_unref(blk); > - blkdev->blk = NULL; > return -1; > } > - assert(bs == blk_bs(blk)); > } else { > /* setup via qemu cmdline -> already setup for us */ > xen_be_printf(&blkdev->xendev, 2, "get configured bdrv (cmdline > setup)\n"); Kevin