qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Max Reitz <mreitz@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>
Cc: Fam Zheng <famz@redhat.com>,
	qemu-devel@nongnu.org, Stefan Hajnoczi <stefanha@redhat.com>
Subject: Re: [Qemu-devel] [PATCH v2 7/7] blkverify: Don't require protocol filename
Date: Mon, 25 Nov 2013 21:09:09 +0100	[thread overview]
Message-ID: <5293AE65.505@redhat.com> (raw)
In-Reply-To: <20131125144011.GJ3009@dhcp-200-207.str.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

      reply	other threads:[~2013-11-25 20:08 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-11-22 16:10 [Qemu-devel] [PATCH v2 0/7] blkdebug/blkverify: Allow command-line configuration Max Reitz
2013-11-22 16:10 ` [Qemu-devel] [PATCH v2 1/7] blkdebug: Use errp for read_config() Max Reitz
2013-11-25 13:40   ` Kevin Wolf
2013-11-25 19:42     ` Max Reitz
2013-11-22 16:10 ` [Qemu-devel] [PATCH v2 2/7] blkdebug: Don't require sophisticated filename Max Reitz
2013-11-22 16:10 ` [Qemu-devel] [PATCH v2 3/7] qdict: Add qdict_array_split() Max Reitz
2013-11-22 16:10 ` [Qemu-devel] [PATCH v2 4/7] qemu-option: Add qemu_config_parse_qdict() Max Reitz
2013-11-22 16:10 ` [Qemu-devel] [PATCH v2 5/7] blkdebug: Always call read_config() Max Reitz
2013-11-22 16:10 ` [Qemu-devel] [PATCH v2 6/7] blkdebug: Use command-line in read_config() Max Reitz
2013-11-22 16:10 ` [Qemu-devel] [PATCH v2 7/7] blkverify: Don't require protocol filename Max Reitz
2013-11-25 14:40   ` Kevin Wolf
2013-11-25 20:09     ` Max Reitz [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5293AE65.505@redhat.com \
    --to=mreitz@redhat.com \
    --cc=famz@redhat.com \
    --cc=kwolf@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=stefanha@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).