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

[Max Reitz <mreitz@redhat.com>]

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