[Qemu-devel] [RFC] Using BlockdevRef in the block layer

[Qemu-devel] [RFC] Using BlockdevRef in the block layer

[Max Reitz]

Hi everyone,

Quite recently, I sent a series to qemu-devel which allowed it to use 
blkdebug and blkverify in some kind of a “native” QMP mode; that is, 
there was no need to parse the filename anymore, instead everything 
could be passed through the blockdev options.

The image filename could easily be specified through the filename 
itself; however, the raw image for blkverify had to be specified through 
some special option, which I just used “x-raw” for. Kevin correctly 
commented that this is a pretty ugly way to solve the issue and instead, 
both the image to be verified and the raw image should be specified 
through a BlockdevRef.

When trying to implement this, I hit the problem that BlockdevRef allows 
you to reference an existing block device; however, this seems currently 
unimplemented. This is further hindered by the fact how this reference 
is done: If you want to give a file to some driver such as qcow2 (or 
blkdebug) which references an existing block device, the option dict 
should look something like this:

{
'driver': 'blkdebug',
'id': 'drive0-debug',
'file': 'drive0'
}

So, as you can see, the “file” value now is simply a string (this is 
what distinguishes a reference from a "real" file, in which case it 
would be a dict). The problem is that blockdev_init() already uses this 
case for just specifying a filename without any further options.

My current solution is to ignore the “file” value in case 
blockdev_init() is called from qmp_blockdev_add(), but this doesn't 
solve the general legacy issue. So, did I miss anything or is 
referencing an existing block device really not supported so far and the 
only meaning to the "file" option containing a pure string is specifying 
a filename?

Second, if specifying a reference to an existing device should really be 
supported, bdrv_open() should ideally not call bdrv_file_open() anymore, 
but a function bdrv_find_ref() instead which resolves a BlockdevRef 
structure (for simplicity, it appears to be easier to use a QDict 
equivalent to a BlockdevRef instead of the latter itself (since that 
results in many effectively redundant conversions to and from those 
representations)). However, bdrv_file_open() supports parsing protocol 
filenames, which bdrv_find_ref() would not. As a result, it is probably 
best to call bdrv_find_ref() from bdrv_file_open() instead and leave 
bdrv_open() generally the way it is right now – yes, this is a question. 
;-) (“Do you agree?”)

Third, I planned to implement the blkdebug and blkverify QMP interface 
by just making them BlockdevOptionsGenericFormat (with the addition of a 
"test" BlockdevRef for blkverify). This will give them a “file” 
automatically. However, this makes them “drivers for the protocol level” 
(or however this is properly called), i.e., they need to specify 
bdrv_open() instead of bdrv_file_open() to work. But blkdebug and 
blkverify are their own protocols with the current interface: Making 
them require an underlying file will break the current interface with 
the filename specifying the required options. To resolve this, I added 
two new “interfaces”, blkdebug-qmp and blkverify-qmp, which reference 
the same functions as blkdebug and blkverify, respectively, however, 
they offer bdrv_open() instead of bdrv_file_open(). These new block 
drivers will thus not support the current interface, but they will be 
properly supported through the QMP interface.

In retrospect, I thought to have changed a lot more, but this is still 
more than I actually thought would be required to adapt blkdebug and 
blkverify to the “standard“ QMP interface in the first place, so I 
considered it worth asking for comments before sending a rather large 
patch series. ;-)


Kind regards,

Max

Re: [Qemu-devel] [RFC] Using BlockdevRef in the block layer

[Max Reitz]

On 05.12.2013 18:41, Max Reitz wrote:
> […]
>
> Second, if specifying a reference to an existing device should really 
> be supported, bdrv_open() should ideally not call bdrv_file_open() 
> anymore, but a function bdrv_find_ref() instead which resolves a 
> BlockdevRef structure (for simplicity, it appears to be easier to use 
> a QDict equivalent to a BlockdevRef instead of the latter itself 
> (since that results in many effectively redundant conversions to and 
> from those representations)). However, bdrv_file_open() supports 
> parsing protocol filenames, which bdrv_find_ref() would not. As a 
> result, it is probably best to call bdrv_find_ref() from 
> bdrv_file_open() instead and leave bdrv_open() generally the way it is 
> right now – yes, this is a question. ;-) (“Do you agree?”)

I noticed only just now that the current design does not seem to allow 
nesting of files (i.e., driver=blkdebug-qmp, file.driver=qcow2, 
file.file.driver=file). Perhaps I do have to call bdrv_find_ref() in 
bdrv_open() and only resort to bdrv_file_open() if a filename that must 
be parsed was given...?

Max

Re: [Qemu-devel] [RFC] Using BlockdevRef in the block layer

[Max Reitz]

On 05.12.2013 19:35, Max Reitz wrote:
> On 05.12.2013 18:41, Max Reitz wrote:
>> […]
>>
>> Second, if specifying a reference to an existing device should really 
>> be supported, bdrv_open() should ideally not call bdrv_file_open() 
>> anymore, but a function bdrv_find_ref() instead which resolves a 
>> BlockdevRef structure (for simplicity, it appears to be easier to use 
>> a QDict equivalent to a BlockdevRef instead of the latter itself 
>> (since that results in many effectively redundant conversions to and 
>> from those representations)). However, bdrv_file_open() supports 
>> parsing protocol filenames, which bdrv_find_ref() would not. As a 
>> result, it is probably best to call bdrv_find_ref() from 
>> bdrv_file_open() instead and leave bdrv_open() generally the way it 
>> is right now – yes, this is a question. ;-) (“Do you agree?”)
>
> I noticed only just now that the current design does not seem to allow 
> nesting of files (i.e., driver=blkdebug-qmp, file.driver=qcow2, 
> file.file.driver=file). Perhaps I do have to call bdrv_find_ref() in 
> bdrv_open() and only resort to bdrv_file_open() if a filename that 
> must be parsed was given...?

Okay, I removed bdrv_find_ref() in favor of letting bdrv_file_open() 
handle everything. If a reference to an existing block device is given 
(as an additional parameter, since the QDict cannot handle this because 
of the fact that a blockdev reference will replace the QDict instead of 
being part of it), it is used. Otherwise, if the block driver found does 
not support bdrv_file_open(), it will recursively call bdrv_open() 
instead of bdrv_open_common(). Concept-wise, this is perhaps a pretty 
ugly solution; but code-wise, it looks pretty well, especially 
considering that this does not change the current code much in terms of 
LoC changed (and, more importantly, it just works; at least apparently, 
so far).

On the other hand, as far as I know, the current “file” concept is not 
determined to stay anyway, is it? At some point, we will have to 
implement something to actually chain multiple block drivers. Allowing 
this through “file” is as far as the current concept goes and there is 
no way of getting around recursion.

The only question to me here is whether it is a good idea to allow calls 
to bdrv_file_open() on block drivers which don't actually support that 
function; but I don't know how to solve this problem in any other way, 
and I personally don't want to put too much effort into fixing the 
current model if we want to have a new one as soon as possible anyway.

Max

Re: [Qemu-devel] [RFC] Using BlockdevRef in the block layer

[Kevin Wolf]

Am 05.12.2013 um 19:35 hat Max Reitz geschrieben:
> On 05.12.2013 18:41, Max Reitz wrote:
> >[…]
> >
> >Second, if specifying a reference to an existing device should
> >really be supported, bdrv_open() should ideally not call
> >bdrv_file_open() anymore, but a function bdrv_find_ref() instead
> >which resolves a BlockdevRef structure (for simplicity, it appears
> >to be easier to use a QDict equivalent to a BlockdevRef instead of
> >the latter itself (since that results in many effectively
> >redundant conversions to and from those representations)).
> >However, bdrv_file_open() supports parsing protocol filenames,
> >which bdrv_find_ref() would not. As a result, it is probably best
> >to call bdrv_find_ref() from bdrv_file_open() instead and leave
> >bdrv_open() generally the way it is right now – yes, this is a
> >question. ;-) (“Do you agree?”)
> 
> I noticed only just now that the current design does not seem to
> allow nesting of files (i.e., driver=blkdebug-qmp,
> file.driver=qcow2, file.file.driver=file). Perhaps I do have to call
> bdrv_find_ref() in bdrv_open() and only resort to bdrv_file_open()
> if a filename that must be parsed was given...?

This is supposed to work. If it doesn't work today, it is a bug or
missing implementation rather than a design decision.

Kevin

Re: [Qemu-devel] [RFC] Using BlockdevRef in the block layer

[Max Reitz]

On 06.12.2013 11:45, Kevin Wolf wrote:
> Am 05.12.2013 um 19:35 hat Max Reitz geschrieben:
>> On 05.12.2013 18:41, Max Reitz wrote:
>>> […]
>>>
>>> Second, if specifying a reference to an existing device should
>>> really be supported, bdrv_open() should ideally not call
>>> bdrv_file_open() anymore, but a function bdrv_find_ref() instead
>>> which resolves a BlockdevRef structure (for simplicity, it appears
>>> to be easier to use a QDict equivalent to a BlockdevRef instead of
>>> the latter itself (since that results in many effectively
>>> redundant conversions to and from those representations)).
>>> However, bdrv_file_open() supports parsing protocol filenames,
>>> which bdrv_find_ref() would not. As a result, it is probably best
>>> to call bdrv_find_ref() from bdrv_file_open() instead and leave
>>> bdrv_open() generally the way it is right now – yes, this is a
>>> question. ;-) (“Do you agree?”)
>> I noticed only just now that the current design does not seem to
>> allow nesting of files (i.e., driver=blkdebug-qmp,
>> file.driver=qcow2, file.file.driver=file). Perhaps I do have to call
>> bdrv_find_ref() in bdrv_open() and only resort to bdrv_file_open()
>> if a filename that must be parsed was given...?
> This is supposed to work. If it doesn't work today, it is a bug or
> missing implementation rather than a design decision.

The top level bdrv_open() will call bdrv_file_open() with {driver=qcow2, 
file.driver=file}. This in turn calls bdrv_open_common() with file=NULL 
which will fail, since qcow2 expects a file to be present 
(drv->bdrv_file_open == NULL && file == NULL). At least, that is what 
happened for me when I tried this.

Max

Re: [Qemu-devel] [RFC] Using BlockdevRef in the block layer

[Kevin Wolf]

Am 05.12.2013 um 18:41 hat Max Reitz geschrieben:
> When trying to implement this, I hit the problem that BlockdevRef
> allows you to reference an existing block device; however, this
> seems currently unimplemented. This is further hindered by the fact
> how this reference is done: If you want to give a file to some
> driver such as qcow2 (or blkdebug) which references an existing
> block device, the option dict should look something like this:
> 
> {
> 'driver': 'blkdebug',
> 'id': 'drive0-debug',
> 'file': 'drive0'
> }
> 
> So, as you can see, the “file” value now is simply a string (this is
> what distinguishes a reference from a "real" file, in which case it
> would be a dict). The problem is that blockdev_init() already uses
> this case for just specifying a filename without any further
> options.

Yes, -drive uses 'file' for the filename, but blockdev-add doesn't.
Inside bdrv_open(), there is a pretty clear distinction between the two:
The legacy filename option of -drive is passed as a separate parameter
and never as an options QDict entry. If the option comes from QMP,
though, it will be in the QDict.

/me checks the source code...

Okay, that's not true today, even if it comes from QMP we treat it like
the legacy option. We need to move the parsing of the 'file' option from
blockdev_init() to drive_init() to make it work this way. It's the right
thing to do anyway.

> My current solution is to ignore the “file” value in case
> blockdev_init() is called from qmp_blockdev_add(), but this doesn't
> solve the general legacy issue. So, did I miss anything or is
> referencing an existing block device really not supported so far and
> the only meaning to the "file" option containing a pure string is
> specifying a filename?

It's not supported so far, but we want to have it.

Ideally legacy option handling would be moved to drive_init() by
conversion to the new data structures. This might not be entirely
trivial with file names, though, so I think for now changing things
within block_open() and friends is okay.

> Second, if specifying a reference to an existing device should
> really be supported, bdrv_open() should ideally not call
> bdrv_file_open() anymore, but a function bdrv_find_ref() instead
> which resolves a BlockdevRef structure (for simplicity, it appears
> to be easier to use a QDict equivalent to a BlockdevRef instead of
> the latter itself (since that results in many effectively redundant
> conversions to and from those representations)).

Agreed. Not sure about the function name (perhaps bdrv_open_ref is
clearer?), but otherwise I like this design; perhaps the reason is that
I suggested it myself earlier. ;-)

> However,
> bdrv_file_open() supports parsing protocol filenames, which
> bdrv_find_ref() would not. As a result, it is probably best to call
> bdrv_find_ref() from bdrv_file_open() instead and leave bdrv_open()
> generally the way it is right now – yes, this is a question. ;-)
> (“Do you agree?”)

Perhaps what we need to do is to call bdrv_file_open() for the legacy
case (filename passed as a separate parameter to bdrv_open()), and call
bdrv_find_ref() when we have a 'file' QDict entry?

> Third, I planned to implement the blkdebug and blkverify QMP
> interface by just making them BlockdevOptionsGenericFormat (with the
> addition of a "test" BlockdevRef for blkverify). This will give them
> a “file” automatically. However, this makes them “drivers for the
> protocol level” (or however this is properly called), i.e., they
> need to specify bdrv_open() instead of bdrv_file_open() to work. But
> blkdebug and blkverify are their own protocols with the current
> interface: Making them require an underlying file will break the
> current interface with the filename specifying the required options.
> To resolve this, I added two new “interfaces”, blkdebug-qmp and
> blkverify-qmp, which reference the same functions as blkdebug and
> blkverify, respectively, however, they offer bdrv_open() instead of
> bdrv_file_open(). These new block drivers will thus not support the
> current interface, but they will be properly supported through the
> QMP interface.

Hm... This is ugly.

I'm not entirely sure I understand why we can't keep using
bdrv_file_open() when inheriting from BlockdevOptionsGenericFormat.
Granted, we wouldn't get bs->file automatially opened by
bdrv_open_common() - but we don't today, and calling bdrv_file_open()
manually from blkdebug_open() works just fine.

Kevin

Re: [Qemu-devel] [RFC] Using BlockdevRef in the block layer

[Max Reitz]

On 06.12.2013 11:43, Kevin Wolf wrote:
> Am 05.12.2013 um 18:41 hat Max Reitz geschrieben:
>> When trying to implement this, I hit the problem that BlockdevRef
>> allows you to reference an existing block device; however, this
>> seems currently unimplemented. This is further hindered by the fact
>> how this reference is done: If you want to give a file to some
>> driver such as qcow2 (or blkdebug) which references an existing
>> block device, the option dict should look something like this:
>>
>> {
>> 'driver': 'blkdebug',
>> 'id': 'drive0-debug',
>> 'file': 'drive0'
>> }
>>
>> So, as you can see, the “file” value now is simply a string (this is
>> what distinguishes a reference from a "real" file, in which case it
>> would be a dict). The problem is that blockdev_init() already uses
>> this case for just specifying a filename without any further
>> options.
> Yes, -drive uses 'file' for the filename, but blockdev-add doesn't.
> Inside bdrv_open(), there is a pretty clear distinction between the two:
> The legacy filename option of -drive is passed as a separate parameter
> and never as an options QDict entry. If the option comes from QMP,
> though, it will be in the QDict.
>
> /me checks the source code...
>
> Okay, that's not true today, even if it comes from QMP we treat it like
> the legacy option. We need to move the parsing of the 'file' option from
> blockdev_init() to drive_init() to make it work this way. It's the right
> thing to do anyway.
>
>> My current solution is to ignore the “file” value in case
>> blockdev_init() is called from qmp_blockdev_add(), but this doesn't
>> solve the general legacy issue. So, did I miss anything or is
>> referencing an existing block device really not supported so far and
>> the only meaning to the "file" option containing a pure string is
>> specifying a filename?
> It's not supported so far, but we want to have it.
>
> Ideally legacy option handling would be moved to drive_init() by
> conversion to the new data structures. This might not be entirely
> trivial with file names, though, so I think for now changing things
> within block_open() and friends is okay.

Okay, this makes sense. I'll try my best. ;-)

>> Second, if specifying a reference to an existing device should
>> really be supported, bdrv_open() should ideally not call
>> bdrv_file_open() anymore, but a function bdrv_find_ref() instead
>> which resolves a BlockdevRef structure (for simplicity, it appears
>> to be easier to use a QDict equivalent to a BlockdevRef instead of
>> the latter itself (since that results in many effectively redundant
>> conversions to and from those representations)).
> Agreed. Not sure about the function name (perhaps bdrv_open_ref is
> clearer?), but otherwise I like this design; perhaps the reason is that
> I suggested it myself earlier. ;-)
>
>> However,
>> bdrv_file_open() supports parsing protocol filenames, which
>> bdrv_find_ref() would not. As a result, it is probably best to call
>> bdrv_find_ref() from bdrv_file_open() instead and leave bdrv_open()
>> generally the way it is right now – yes, this is a question. ;-)
>> (“Do you agree?”)
> Perhaps what we need to do is to call bdrv_file_open() for the legacy
> case (filename passed as a separate parameter to bdrv_open()), and call
> bdrv_find_ref() when we have a 'file' QDict entry?

Yes, that is what I referred to in my reply to the original RFC. 
However, it seems easier to just do everything in bdrv_file_open().

>> Third, I planned to implement the blkdebug and blkverify QMP
>> interface by just making them BlockdevOptionsGenericFormat (with the
>> addition of a "test" BlockdevRef for blkverify). This will give them
>> a “file” automatically. However, this makes them “drivers for the
>> protocol level” (or however this is properly called), i.e., they
>> need to specify bdrv_open() instead of bdrv_file_open() to work. But
>> blkdebug and blkverify are their own protocols with the current
>> interface: Making them require an underlying file will break the
>> current interface with the filename specifying the required options.
>> To resolve this, I added two new “interfaces”, blkdebug-qmp and
>> blkverify-qmp, which reference the same functions as blkdebug and
>> blkverify, respectively, however, they offer bdrv_open() instead of
>> bdrv_file_open(). These new block drivers will thus not support the
>> current interface, but they will be properly supported through the
>> QMP interface.
> Hm... This is ugly.
>
> I'm not entirely sure I understand why we can't keep using
> bdrv_file_open() when inheriting from BlockdevOptionsGenericFormat.
> Granted, we wouldn't get bs->file automatially opened by
> bdrv_open_common() - but we don't today, and calling bdrv_file_open()
> manually from blkdebug_open() works just fine.

Yes, we wouldn't get it opened automatically. I liked it being opened 
automatically. ;-)

Okay, then I'll have to make it do without.

Max

Re: [Qemu-devel] [RFC] Using BlockdevRef in the block layer

[Max Reitz]

On 06.12.2013 12:00, Max Reitz wrote:
> On 06.12.2013 11:43, Kevin Wolf wrote:
>> Am 05.12.2013 um 18:41 hat Max Reitz geschrieben:
>>> When trying to implement this, I hit the problem that BlockdevRef
>>> allows you to reference an existing block device; however, this
>>> seems currently unimplemented. This is further hindered by the fact
>>> how this reference is done: If you want to give a file to some
>>> driver such as qcow2 (or blkdebug) which references an existing
>>> block device, the option dict should look something like this:
>>>
>>> {
>>> 'driver': 'blkdebug',
>>> 'id': 'drive0-debug',
>>> 'file': 'drive0'
>>> }
>>>
>>> So, as you can see, the “file” value now is simply a string (this is
>>> what distinguishes a reference from a "real" file, in which case it
>>> would be a dict). The problem is that blockdev_init() already uses
>>> this case for just specifying a filename without any further
>>> options.
>> Yes, -drive uses 'file' for the filename, but blockdev-add doesn't.
>> Inside bdrv_open(), there is a pretty clear distinction between the two:
>> The legacy filename option of -drive is passed as a separate parameter
>> and never as an options QDict entry. If the option comes from QMP,
>> though, it will be in the QDict.
>>
>> /me checks the source code...
>>
>> Okay, that's not true today, even if it comes from QMP we treat it like
>> the legacy option. We need to move the parsing of the 'file' option from
>> blockdev_init() to drive_init() to make it work this way. It's the right
>> thing to do anyway.
>>
>>> My current solution is to ignore the “file” value in case
>>> blockdev_init() is called from qmp_blockdev_add(), but this doesn't
>>> solve the general legacy issue. So, did I miss anything or is
>>> referencing an existing block device really not supported so far and
>>> the only meaning to the "file" option containing a pure string is
>>> specifying a filename?
>> It's not supported so far, but we want to have it.
>>
>> Ideally legacy option handling would be moved to drive_init() by
>> conversion to the new data structures. This might not be entirely
>> trivial with file names, though, so I think for now changing things
>> within block_open() and friends is okay.
>
> Okay, this makes sense. I'll try my best. ;-)
>
>>> Second, if specifying a reference to an existing device should
>>> really be supported, bdrv_open() should ideally not call
>>> bdrv_file_open() anymore, but a function bdrv_find_ref() instead
>>> which resolves a BlockdevRef structure (for simplicity, it appears
>>> to be easier to use a QDict equivalent to a BlockdevRef instead of
>>> the latter itself (since that results in many effectively redundant
>>> conversions to and from those representations)).
>> Agreed. Not sure about the function name (perhaps bdrv_open_ref is
>> clearer?), but otherwise I like this design; perhaps the reason is that
>> I suggested it myself earlier. ;-)
>>
>>> However,
>>> bdrv_file_open() supports parsing protocol filenames, which
>>> bdrv_find_ref() would not. As a result, it is probably best to call
>>> bdrv_find_ref() from bdrv_file_open() instead and leave bdrv_open()
>>> generally the way it is right now – yes, this is a question. ;-)
>>> (“Do you agree?”)
>> Perhaps what we need to do is to call bdrv_file_open() for the legacy
>> case (filename passed as a separate parameter to bdrv_open()), and call
>> bdrv_find_ref() when we have a 'file' QDict entry?
>
> Yes, that is what I referred to in my reply to the original RFC. 
> However, it seems easier to just do everything in bdrv_file_open().
>
>>> Third, I planned to implement the blkdebug and blkverify QMP
>>> interface by just making them BlockdevOptionsGenericFormat (with the
>>> addition of a "test" BlockdevRef for blkverify). This will give them
>>> a “file” automatically. However, this makes them “drivers for the
>>> protocol level” (or however this is properly called), i.e., they
>>> need to specify bdrv_open() instead of bdrv_file_open() to work. But
>>> blkdebug and blkverify are their own protocols with the current
>>> interface: Making them require an underlying file will break the
>>> current interface with the filename specifying the required options.
>>> To resolve this, I added two new “interfaces”, blkdebug-qmp and
>>> blkverify-qmp, which reference the same functions as blkdebug and
>>> blkverify, respectively, however, they offer bdrv_open() instead of
>>> bdrv_file_open(). These new block drivers will thus not support the
>>> current interface, but they will be properly supported through the
>>> QMP interface.
>> Hm... This is ugly.
>>
>> I'm not entirely sure I understand why we can't keep using
>> bdrv_file_open() when inheriting from BlockdevOptionsGenericFormat.
>> Granted, we wouldn't get bs->file automatially opened by
>> bdrv_open_common() - but we don't today, and calling bdrv_file_open()
>> manually from blkdebug_open() works just fine.
>
> Yes, we wouldn't get it opened automatically. I liked it being opened 
> automatically. ;-)

Ah, and what's worse, bdrv_file_open() attempts a bdrv_swap() which is 
bound to fail. That means, we either have to disable bdrv_swap() on some 
protocols – or we just don't call it "file" (i.e., don't let 
BlockdevOptionsBlkdebug be based on BlockdevOptionsGenericFormat). I'd 
prefer the second option, since then I don't even have to care about 
nesting "file"s anymore. ;-)

Max

Re: [Qemu-devel] [RFC] Using BlockdevRef in the block layer

[Max Reitz]

On 06.12.2013 12:14, Max Reitz wrote:
> On 06.12.2013 12:00, Max Reitz wrote:
>> On 06.12.2013 11:43, Kevin Wolf wrote:
>>> Am 05.12.2013 um 18:41 hat Max Reitz geschrieben:
>>>> When trying to implement this, I hit the problem that BlockdevRef
>>>> allows you to reference an existing block device; however, this
>>>> seems currently unimplemented. This is further hindered by the fact
>>>> how this reference is done: If you want to give a file to some
>>>> driver such as qcow2 (or blkdebug) which references an existing
>>>> block device, the option dict should look something like this:
>>>>
>>>> {
>>>> 'driver': 'blkdebug',
>>>> 'id': 'drive0-debug',
>>>> 'file': 'drive0'
>>>> }
>>>>
>>>> So, as you can see, the “file” value now is simply a string (this is
>>>> what distinguishes a reference from a "real" file, in which case it
>>>> would be a dict). The problem is that blockdev_init() already uses
>>>> this case for just specifying a filename without any further
>>>> options.
>>> Yes, -drive uses 'file' for the filename, but blockdev-add doesn't.
>>> Inside bdrv_open(), there is a pretty clear distinction between the 
>>> two:
>>> The legacy filename option of -drive is passed as a separate parameter
>>> and never as an options QDict entry. If the option comes from QMP,
>>> though, it will be in the QDict.
>>>
>>> /me checks the source code...
>>>
>>> Okay, that's not true today, even if it comes from QMP we treat it like
>>> the legacy option. We need to move the parsing of the 'file' option 
>>> from
>>> blockdev_init() to drive_init() to make it work this way. It's the 
>>> right
>>> thing to do anyway.
>>>
>>>> My current solution is to ignore the “file” value in case
>>>> blockdev_init() is called from qmp_blockdev_add(), but this doesn't
>>>> solve the general legacy issue. So, did I miss anything or is
>>>> referencing an existing block device really not supported so far and
>>>> the only meaning to the "file" option containing a pure string is
>>>> specifying a filename?
>>> It's not supported so far, but we want to have it.
>>>
>>> Ideally legacy option handling would be moved to drive_init() by
>>> conversion to the new data structures. This might not be entirely
>>> trivial with file names, though, so I think for now changing things
>>> within block_open() and friends is okay.
>>
>> Okay, this makes sense. I'll try my best. ;-)
>>
>>>> Second, if specifying a reference to an existing device should
>>>> really be supported, bdrv_open() should ideally not call
>>>> bdrv_file_open() anymore, but a function bdrv_find_ref() instead
>>>> which resolves a BlockdevRef structure (for simplicity, it appears
>>>> to be easier to use a QDict equivalent to a BlockdevRef instead of
>>>> the latter itself (since that results in many effectively redundant
>>>> conversions to and from those representations)).
>>> Agreed. Not sure about the function name (perhaps bdrv_open_ref is
>>> clearer?), but otherwise I like this design; perhaps the reason is that
>>> I suggested it myself earlier. ;-)
>>>
>>>> However,
>>>> bdrv_file_open() supports parsing protocol filenames, which
>>>> bdrv_find_ref() would not. As a result, it is probably best to call
>>>> bdrv_find_ref() from bdrv_file_open() instead and leave bdrv_open()
>>>> generally the way it is right now – yes, this is a question. ;-)
>>>> (“Do you agree?”)
>>> Perhaps what we need to do is to call bdrv_file_open() for the legacy
>>> case (filename passed as a separate parameter to bdrv_open()), and call
>>> bdrv_find_ref() when we have a 'file' QDict entry?
>>
>> Yes, that is what I referred to in my reply to the original RFC. 
>> However, it seems easier to just do everything in bdrv_file_open().
>>
>>>> Third, I planned to implement the blkdebug and blkverify QMP
>>>> interface by just making them BlockdevOptionsGenericFormat (with the
>>>> addition of a "test" BlockdevRef for blkverify). This will give them
>>>> a “file” automatically. However, this makes them “drivers for the
>>>> protocol level” (or however this is properly called), i.e., they
>>>> need to specify bdrv_open() instead of bdrv_file_open() to work. But
>>>> blkdebug and blkverify are their own protocols with the current
>>>> interface: Making them require an underlying file will break the
>>>> current interface with the filename specifying the required options.
>>>> To resolve this, I added two new “interfaces”, blkdebug-qmp and
>>>> blkverify-qmp, which reference the same functions as blkdebug and
>>>> blkverify, respectively, however, they offer bdrv_open() instead of
>>>> bdrv_file_open(). These new block drivers will thus not support the
>>>> current interface, but they will be properly supported through the
>>>> QMP interface.
>>> Hm... This is ugly.
>>>
>>> I'm not entirely sure I understand why we can't keep using
>>> bdrv_file_open() when inheriting from BlockdevOptionsGenericFormat.
>>> Granted, we wouldn't get bs->file automatially opened by
>>> bdrv_open_common() - but we don't today, and calling bdrv_file_open()
>>> manually from blkdebug_open() works just fine.
>>
>> Yes, we wouldn't get it opened automatically. I liked it being opened 
>> automatically. ;-)
>
> Ah, and what's worse, bdrv_file_open() attempts a bdrv_swap() which is 
> bound to fail. That means, we either have to disable bdrv_swap() on 
> some protocols – or we just don't call it "file" (i.e., don't let 
> BlockdevOptionsBlkdebug be based on BlockdevOptionsGenericFormat). I'd 
> prefer the second option, since then I don't even have to care about 
> nesting "file"s anymore. ;-)

Er, make that bdrv_open_common() instead of bdrv_file_open(), obviously.

Max