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

[Max Reitz]

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.

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

signature.asc
Description: OpenPGP digital signature