Re: [Qemu-devel] [PATCH v3 04/14] block/xen: Use blk_new_open() in blk_connect()

[Kevin Wolf <kwolf@redhat.com>]

Am 26.01.2015 um 16:00 hat Max Reitz geschrieben:
> Signed-off-by: Max Reitz <mreitz@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