From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33877) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGByU-0002Aj-K6 for qemu-devel@nongnu.org; Tue, 27 Jan 2015 14:38:59 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YGByQ-0008FU-04 for qemu-devel@nongnu.org; Tue, 27 Jan 2015 14:38:58 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39935) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGByP-0008Ey-Os for qemu-devel@nongnu.org; Tue, 27 Jan 2015 14:38:53 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t0RJcp8v006651 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Tue, 27 Jan 2015 14:38:52 -0500 Message-ID: <54C7E949.2030006@redhat.com> Date: Tue, 27 Jan 2015 14:38:49 -0500 From: Max Reitz MIME-Version: 1.0 References: <1422288204-29271-1-git-send-email-mreitz@redhat.com> <54C7E766.3080005@redhat.com> In-Reply-To: <54C7E766.3080005@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 00/50] blockdev: BlockBackend and media List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: Kevin Wolf , Fam Zheng , Jeff Cody , Markus Armbruster , Stefan Hajnoczi , john@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.