Re: [Qemu-devel] [PATCH 00/50] blockdev: BlockBackend and media

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2648 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]