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

[Max Reitz <mreitz@redhat.com>]

On 2015-01-27 at 14:30, Eric Blake wrote:
> 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).

Well, one form of introspection is just assuming that this usage is 
supported and if it doesn't work (you're trying to execute blockdev-add 
without an ID and it fails), fall back to the mem-leaking variant (just 
use blockdev-add with a superfluous BlockBackend) and remember that 
support is missing.

>> - 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)?

Hm, good question.

> 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?

I'd be completely fine with setting up an eject blocker for Quorum 
children, it does make sense to me.

Max

> 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.