|
From: | Max Reitz |
Subject: | Re: [Qemu-devel] [PATCH v3 04/14] block/xen: Use blk_new_open() in blk_connect() |
Date: | Mon, 02 Feb 2015 14:41:10 -0500 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 |
On 2015-02-02 at 13:27, Kevin Wolf wrote:
Am 26.01.2015 um 16:00 hat Max Reitz geschrieben:Signed-off-by: Max Reitz <address@hidden> --- 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.
I was glad enough that I made it out alive after digging just a bit into the code. I'm fully in favor of leaving this as-is and adding a note to the commit message.
Thank you for reviewing! Max
+ 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
[Prev in Thread] | Current Thread | [Next in Thread] |