Re: [Qemu-devel] [Qemu-block] [PATCH 3/3] block: use bdrv_lookup_bs() over blk_by_name() for BDS only results

[Jeff Cody <jcody@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 <jcody@redhat.com>
> > ---
> >  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
>