No oxford comma in the subject? :) On 06/23/2016 10:36 AM, Kevin Wolf wrote: > I am relatively confident to say that everything that should use a > BlockBackend, does so by now. Almost all users create their own anonymous > BlockBackend internally and use that. The user configures the BB only > indirectly using the configuration methods of the user that the BB belongs to. > > There is one exception, which are guest devices. There the user is expected to > manually set up a BlockBackend and pass it to the device. This requires that > users understand the difference between node and BlockBackends and manage both > kinds of objects. This is a rather nasty interface. > > My goal is that we allow a user (management tool) to ignore that BlockBackends > exist as separate entities in qemu. Ideally we could fully make them an > implementation detail, but I'm not sure to which degree we can do that for > compatibility reasons. But what I'm pretty sure we can do is provide > interfaces > that address everything using either node names or (qdev) device names, so > that > you don't have to manage BlockBackends if you don't want to. > > This involves several steps, and for most of them this series contains an > example patch that shows what this could look like: > > 1. Accept node-name in -device drive=... and create an internal anonymous BB > for devices configured this way. This is the way to create devices that > completely avoid legacy interfaces using the BB name. > > 2. Update all QMP commands touching block devices. There are two kinds of > them, > concerning either the guest device (which the BlockBackend is logically > part > of, even though it's not implemented this way) or the actual backend > (BlockDriverState/node level) > > * Device level commands: Accept a guest device ID instead of BB name to > identify the BlockBackend the command works on. As device IDs and BB > names > don't share a single namespace, we'll need a new QMP argument for this. > > * Node level commands: We need to complete the conversion that makes > commands accept node names instead of BlockBackend names. In some places > we intentionally allow only BlockBackends because we don't know if the > command works in other places than the root. This is okay, but we can > accept node names anyway. We just need to check that the node is a root > node as expected. > > 3. Remove all BlockBackend options from blockdev-add. This has already > happened > partially (e.g. WCE flag), but at least id, rerror, werror are still there. > This is a very incompatible change, but we declared blockdev-add > experimental, so I think it's acceptable. > > 4. Add BlockBackend options as qdev properties to all block devices. > > 5. Add a way on the command line to create block nodes that have a node-name > and don't have a BlockBackend. blockdev-add already supports this (and > after > implementing 3. it will be the only mode supported by blockdev-add), but we > can't do this on the command line yet. You always get a BB with -drive. > > This might finally become the -blockdev we were talking about at the very > beginning of the block layer generalisation work. > > So this is my plan. It's pretty radical, but I think we really must do > something about our interfaces. Having nodes, BlockBackends and guest devices > to manage is just too much and doesn't really make sense. Making BlockBackends > visible in the external API essentially only as aliases for either node names > or guest devices (and that only for compatibility, not when using > blockdev-add/ > -blockdev) feels to me like the right thing to do. > > But of course I'm aware that there probably isn't a clear right or wrong, and > that I might be missing important details, so this needs to be discussed in > advance before I go and implement the full thing instead of just small example > patches. > > So please let me know what you guys think about this plan. > > Kevin Wolf (7): > block/qdev: Allow node name for drive properties > block: Add blk_by_dev() > qdev-monitor: Factor out find_device_state() > qdev-monitor: Add blk_by_qdev_id() > block: Accept device model name for blockdev-open/close-tray > block: Accept node-name for block-stream > block/qdev: Allow configuring WCE with qdev properties > > block/block-backend.c | 19 +++++++++ > blockdev.c | 92 > ++++++++++++++++++++++++++++------------ > hw/block/block.c | 16 +++++++ > hw/block/nvme.c | 1 + > hw/block/virtio-blk.c | 1 + > hw/core/qdev-properties-system.c | 18 +++++++- > hw/ide/qdev.c | 1 + > hw/scsi/scsi-disk.c | 1 + > hw/usb/dev-storage.c | 1 + > include/hw/block/block.h | 5 ++- > include/sysemu/block-backend.h | 2 + > qapi/block-core.json | 16 ++++--- > qdev-monitor.c | 34 +++++++++++++-- > qmp-commands.hx | 14 +++--- > 14 files changed, 178 insertions(+), 43 deletions(-) >
1-4: Reviewed-by: John Snow <js...@redhat.com> 5: Looks good, pending discussion on the right thing to name "ID", but the patch itself looks perfectly cromulent. 6: Causes only a minor regression in 030 due to different error class names, but R-B otherwise. 7: No opinion. Looks sane mechanically but I don't know enough about core block properties to have a meaningful opinion. "ACK." For non-RFC, some new iotests would be good. Thanks, --js