[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 00/50] blockdev: BlockBackend and media
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH 00/50] blockdev: BlockBackend and media |
Date: |
Tue, 27 Jan 2015 12:30:46 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 |
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
- [Qemu-devel] [PATCH 38/50] blockdev: Add blockdev-open-tray, (continued)
- [Qemu-devel] [PATCH 38/50] blockdev: Add blockdev-open-tray, Max Reitz, 2015/01/26
- [Qemu-devel] [PATCH 40/50] blockdev: Add blockdev-remove-medium, Max Reitz, 2015/01/26
- [Qemu-devel] [PATCH 49/50] iotests: More options for VM.add_drive(), Max Reitz, 2015/01/26
- [Qemu-devel] [PATCH 42/50] blockdev: Implement eject with basic operations, Max Reitz, 2015/01/26
- [Qemu-devel] [PATCH 45/50] qmp: Introduce blockdev-change-medium, Max Reitz, 2015/01/26
- [Qemu-devel] [PATCH 41/50] blockdev: Add blockdev-insert-medium, Max Reitz, 2015/01/26
- [Qemu-devel] [PATCH 46/50] hmp: Use blockdev-change-medium for change command, Max Reitz, 2015/01/26
- [Qemu-devel] [PATCH 47/50] blockdev: Add read-only option to blockdev-change-medium, Max Reitz, 2015/01/26
- [Qemu-devel] [PATCH 50/50] iotests: Add test for change-related QMP commands, Max Reitz, 2015/01/26
- [Qemu-devel] [PATCH 48/50] hmp: Add read-only option to change command, Max Reitz, 2015/01/26
- Re: [Qemu-devel] [PATCH 00/50] blockdev: BlockBackend and media,
Eric Blake <=
- Re: [Qemu-devel] [PATCH 00/50] blockdev: BlockBackend and media, Max Reitz, 2015/01/27