Re: [PATCH v7 02/47] block: Add chain helper functions

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

25.06.2020 18:21, Max Reitz wrote:
> Add some helper functions for skipping filters in a chain of block
> nodes.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   include/block/block_int.h |  3 +++
>   block.c                   | 55 +++++++++++++++++++++++++++++++++++++++
>   2 files changed, 58 insertions(+)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index bb3457c5e8..5da793bfc3 100644


This patch raises two questions:

1. How to treat filters at the end of the backing chain?

- child-access function will return no filter child for such nodes, it's correct of course
- filer skipping functions will return this filter.. How much is it correct - I don't know.


Consider a chain

top --- backing ---> filter-with-no-child

if bdrv_backing_chain_next(top) returns NULL, it's incorrect, because
top actually have backing, and on read it will read from it for
unallocated clusters (and this should crash). So, probably, returning
filter as a backing-chain-next is a valid thing to do. Or we should
assert that we are not in such situation (which may crash more often
than trying to really read from nonexistent child).

so, returning NULL, may even less correct than returning a filter..


2. How to tread nodes with drv=NULL, but with filter child (with BDRV_CHILD_FILTERED role).
- child-access functions returns no filtered child for such nodes
- filter skipping functions will stop on it..

=======

Isn't it better to drop drv->is_filter at all? And call filter nodes with a bs->file or bs->backing
child in BDRV_CHILD_FILTERED role? This automatically closes the two questions:

- node without a child in BDRV_CHILD_FILTERED is automatically non-filter. So, filter driver is responsible for having such child.
- node without a drv may still be a filter if it have BDRV_CHILD_FILTERED.. Still, not very useful.

Anyway, is_filter and BDRV_CHILD_FILTERED are in contradiction, and it seems good to get rid of is_filter. But I may miss something.

[..]

> +
> +static BlockDriverState *bdrv_do_skip_filters(BlockDriverState *bs,
> +                                              bool stop_on_explicit_filter)
> +{
> +    BdrvChild *c;
> +
> +    if (!bs) {
> +        return NULL;
> +    }
> +
> +    while (!(stop_on_explicit_filter && !bs->implicit)) {
> +        c = bdrv_filter_child(bs);
> +        if (!c) {
> +            break;
> +        }
> +        bs = c->bs;
> +    }
> +    /*
> +     * Note that this treats nodes with bs->drv == NULL as not being
> +     * filters (bs->drv == NULL should be replaced by something else
> +     * anyway).
> +     * The advantage of this behavior is that this function will thus
> +     * always return a non-NULL value (given a non-NULL @bs).

I don't see, how it is follows from first sentence? We can skip nodes
with a child of BDRV_CHILD_FILTERED and drv=NULL as well, and still return
non-NULL bs at the end...

Didn't you mean "treat nodes without filter child as not being filters, even if they have drv->is_filter == true"? This is a real reason for the second sentence.

... and the disadvantage is that we may return filter node, which may be not expected by caller ...

> +     */
> +
> +    return bs;
> +}


I think, I can live with it as is for now. If we don't drop is_filter, I think it worth documenting these corner cases somewhere (may be near .is_filter definition).

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir