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.