On 01/26/2015 09:02 AM, Max Reitz wrote: > This series reworks a lot regarding BlockBackend and media. It is > essentially a v3 to the "blockdev: Add blockdev-change-medium with > read-only option" series (which is in fact a part of this series), but > of course does a lot more. >
I'm not done reviewing yet, but making some comments here as I go. > > This series depends on v3 (or any later version) of my series > 'block: Remove "growable", add blk_new_open()'. So I reviewed that first. > > > - Patches 1 and 2 make it possible to use blockdev-add without creating > a BlockBackend. This is necessary because the QMP command introduced > in patch 42 (blockdev-insert-medium) will insert a BDS tree (a medium) > into a BlockBackend (a drive). Creating a BlockBackend for such a BDS > tree would both be a hassle and a waste, so this makes it possible to > omit creating a BB. Basically, making blockdev-add be capable of creating a detached BDS tree for later attachment to some device or as a branch in an even larger BDS tree. Seems reasonable enough; I still haven't made up my mind if we need some form of introspection to know if this usage is supported (but since later in the series you are adding new commands that depend on this, that may be a good enough witness). > - Patch 6 makes blk_is_inserted() return true only if there is a BDS > tree; furthermore, blk_is_available() is added which additionally > checks whether the guest device tray is closed. > > - Patches 7 and 8 are some kind of follow-up to patch 6; they make > bdrv_is_inserted() actually useful (unless I'm not mistaken; maybe I > am and they make bdrv_is_inserted() actually useless). Does it even make sense to have a quorum that is mixed between a CDROM passthrough drive (where medium can be removed) and on-disk/network locations (where medium is unchangeable)? But your idea makes sense - a tree is inserted if all of its parts are also inserted, and removing any one medium anywhere in the BDS tree makes it pointless to use all other parts of the tree that feed the same BB. Do we need some sort of operation blocker that prevents eject on a BDS that feeds a higher-level node of a BDS tree? Or if you want to read that paragraph differently: I reviewed the patches for coding bugs and found none, but I'm fuzzy enough on design details that I'd appreciate a second review from an interested party to make sure I'm not overlooking a design flaw in your change. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
