From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:54244)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <mreitz@redhat.com>) id 1YIMs4-0003I3-Rw
	for qemu-devel@nongnu.org; Mon, 02 Feb 2015 14:41:25 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <mreitz@redhat.com>) id 1YIMrz-0006W3-Qq
	for qemu-devel@nongnu.org; Mon, 02 Feb 2015 14:41:20 -0500
Received: from mx1.redhat.com ([209.132.183.28]:57100)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <mreitz@redhat.com>) id 1YIMrz-0006Vx-JX
	for qemu-devel@nongnu.org; Mon, 02 Feb 2015 14:41:15 -0500
Message-ID: <54CFD2D6.1000101@redhat.com>
Date: Mon, 02 Feb 2015 14:41:10 -0500
From: Max Reitz <mreitz@redhat.com>
MIME-Version: 1.0
References: <1422284444-12529-1-git-send-email-mreitz@redhat.com>
	<1422284444-12529-5-git-send-email-mreitz@redhat.com>
	<20150202182751.GB19586@noname.redhat.com>
In-Reply-To: <20150202182751.GB19586@noname.redhat.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v3 04/14] block/xen: Use blk_new_open() in
	blk_connect()
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Kevin Wolf <kwolf@redhat.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>, qemu-devel@nongnu.org, Stefan Hajnoczi <stefanha@redhat.com>, Markus Armbruster <armbru@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