Re: [Qemu-block] [PATCH 2/3] block: skip implicit nodes in snapshots, blockjobs

[Kevin Wolf]

Am 01.08.2017 um 15:49 hat Manos Pitsidianakis geschrieben:
> Implicit filter nodes added at the top of nodes can interfere with block
> jobs. This is not a problem when they are added by other jobs since
> adding another job will issue a QERR_DEVICE_IN_USE, but it can happen in
> the next commit which introduces an implicitly created throttle filter
> node below BlockBackend, which we want to be skipped during automatic
> operations on the graph since the user does not necessarily know about
> their existence.
> 
> Signed-off-by: Manos Pitsidianakis <address@hidden>
> ---
>  block.c                   | 17 +++++++++++++++++
>  blockdev.c                | 12 ++++++++++++
>  include/block/block_int.h |  2 ++
>  3 files changed, 31 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 886a457ab0..9ebdba28b0 100644
> --- a/block.c
> +++ b/block.c
> @@ -4947,3 +4947,20 @@ bool bdrv_can_store_new_dirty_bitmap(BlockDriverState 
> *bs, const char *name,
>  
>      return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
>  }
> +
> +/* Get first non-implicit node down a bs chain. */
> +BlockDriverState *bdrv_get_first_non_implicit(BlockDriverState *bs)
> +{
> +    if (!bs) {
> +        return NULL;
> +    }
> +
> +    if (!bs->implicit) {
> +        return bs;
> +    }
> +
> +    /* at this point bs is implicit and must have a child */
> +    assert(bs->file);
> +
> +    return bdrv_get_first_non_implicit(bs->file->bs);
> +}

mirror_top/commit_top use bs->backing instead of bs->file, so this
assertion would fail for them.

It's probably better to assert that the implicit node has only one child
and then use that child, whatever it may be.

I'd also prefer the function to work iteratively rather than recursively
because chains can be rather long. (I know that we recurse in many
places anyway, so it's not a strong argument, but I think it would also
look a bit nicer.)

Kevin