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

[Max Reitz <mreitz@redhat.com>]

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

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