[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 3/3] block: use bdrv_lookup_bs() over blk_by_nam
From: |
Jeff Cody |
Subject: |
Re: [Qemu-block] [PATCH 3/3] block: use bdrv_lookup_bs() over blk_by_name() for BDS only results |
Date: |
Wed, 14 Oct 2015 14:13:03 -0400 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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 <address@hidden>
> > ---
> > 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
>