Re: [Qemu-devel] [PATCH v2 7/7] blkverify: Don't require protocol filename

[Max Reitz <mreitz@redhat.com>]

On 25.11.2013 15:40, Kevin Wolf wrote:
> Am 22.11.2013 um 17:10 hat Max Reitz geschrieben:
>> If the filename is not prefixed by "blkverify:" in
>> blkverify_parse_filename(), the blkverify driver was not selected
>> through that protocol prefix, but by an explicit command line option
>> (like file.driver=blkverify). Contrary to the current reaction, this is
>> not really a problem; the whole filename just has to be stored (in the
>> x-image option) and the user has to manually specify the x-raw option.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/blkverify.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/blkverify.c b/block/blkverify.c
>> index 3c63528..bdbdd68 100644
>> --- a/block/blkverify.c
>> +++ b/block/blkverify.c
>> @@ -78,7 +78,9 @@ static void blkverify_parse_filename(const char *filename, QDict *options,
>>   
>>       /* Parse the blkverify: prefix */
>>       if (!strstart(filename, "blkverify:", &filename)) {
>> -        error_setg(errp, "File name string must start with 'blkverify:'");
>> +        /* There was no prefix; therefore, all options have to be already
>> +           present in the QDict (except for the filename) */
>> +        qdict_put(options, "x-image", qstring_from_str(filename));
>>           return;
>>       }
> We don't want users to specify x-raw options, that's why it starts with
> "x-" in the first place. So I'm not sure if this patch is a useful
> intermediate step to make.

Well, it's the last patch of the series, so we could just leave it out 
for the time being. ;-)

> What we want to allow in the end is something like this:
>
>      { "execute": "blockdev-add", "options": {
>          { "driver": "blkverify",
>            "image": {
>                "driver": "qcow2",
>                "file": ... },
>            "raw:" {
>                "driver": "raw",
>                "file": ... } } }
>
> Where "image" and "raw" are both of the BlockdevRef union type in QAPI,
> i.e. there could also be a string that references an existing block
> device.
>
> We'll probably want a function that takes a BlockdevRef and returns a
> BlockDriverState; either by bdrv_open() on a new one, or by bdrv_ref()
> on an existing one.

Hm, yes, that seems far better.

> Fam already has some code to achieve this in his BlockOp blockers
> series, though not yet in a reusable way. I guess this series is the
> good reason to actually request something reusable and then make use of
> it here.
>
> I guess you two just need to coordinate who's going to implement it (Fam
> by default, I'd assume?)

Hm, are you referring to patch 5/7? I don't see right now how we could 
use that code for BlockdevRef… I'd agree we could probably use such code 
(using BlockdevRef) there instead, but not really the other way round. 
So, I think I could try to implement that function myself first.

Max