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