From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47080) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZmQYA-0005H7-9f for qemu-devel@nongnu.org; Wed, 14 Oct 2015 14:13:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZmQY6-000800-Cx for qemu-devel@nongnu.org; Wed, 14 Oct 2015 14:13:18 -0400 Date: Wed, 14 Oct 2015 14:13:03 -0400 From: Jeff Cody Message-ID: <20151014181303.GA9447@localhost.localdomain> References: <561E996E.4040608@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <561E996E.4040608@redhat.com> Subject: Re: [Qemu-devel] [Qemu-block] [PATCH 3/3] block: use bdrv_lookup_bs() over blk_by_name() for BDS only results List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: kwolf@redhat.com, quintela@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org, armbru@redhat.com On Wed, Oct 14, 2015 at 08:05:34PM +0200, Max Reitz wrote: > On 14.10.2015 15:16, Jeff Cody wrote: > > To find a BlockDriverState interface, it can be done via blk_by_name(), > > bdrv_find_node(), and bdrv_lookup_bs(). The latter can take the place > > of the other two, in the instances where we are only concerned with the > > BlockDriverState. > > > > In much of the usage of blk_by_name(), we can simplify the code by > > calling bdrv_lookup_bs() instead of blk_by_name(), when what we need is > > just the BDS, and not the BB. > > > > Signed-off-by: Jeff Cody > > --- > > blockdev.c | 84 ++++++++++++++++++++----------------------------------- > > migration/block.c | 6 ++-- > > 2 files changed, 32 insertions(+), 58 deletions(-) > > Again, if this series is based on Berto's blockdev-snapshot series, it > should be based on my BB series as well. But with that series applied, > this patch has some (non-trivial) rebase conflicts on it. > > Also, it has a fundamental conflict with that series: If a BB can be > found by bdrv_lookup_bs() but it doesn't have a BDS, it will return NULL > and set *errp accordingly ("No medium in device"). However, this patch > discards that error message and just keeps the one of the former > blk_by_name() caller, which is generally "Device not found". That is > wrong, however, if there is simply no medium in the device. > Good point. We can just drop this patch, then - maybe at some point, we can have a consolidated API to obtain a BDS (if having such a thing is even all that important), that fits well with the design goal of your series. I think the first two patches are probably worth keeping, however. > In some cases, that doesn't matter (since we just assume that if there > is a BB, it will have a BDS, like for hmp_commit), but it most probably > does matter for all the places which conflict with my BB series, the > reason being that they generally conflict because I added a > blk_is_available() check. > > Note that this is a "design conflict", too: bdrv_lookup_bs() will return > NULL only if blk_bs() is NULL. blk_is_available() may return false even > if there is a BDS (if the BDS is unavailable, e.g. one of the BDSs in > the tree not being inserted or the guest device's tray is open). But if > you already have a BDS (because you used bdrv_lookup_bs()), calling > blk_is_available() on bs->blk afterwards looks kind of strange... > > Max >