On 13.07.20 12:18, Vladimir Sementsov-Ogievskiy wrote:
> 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?

It was my understanding that this configuration would be impossible.

> - 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

How would it be possible to have filter without a 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).

drv=NULL is a bug on its own that should be fixed...  (The idea we had
at some point was to introduce a special driver that just always returns
-EIO on everything, and to replace corrupt qcow2 nodes by that.  Or,
well, we might just return -EIO from the qcow2 driver, actually, if we
never use drv=NULL anywhere else.)

In any case, drv=NULL is an edge case that I think never has anything to
do with filters.

> - 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...

My idea was that nodes with bs->drv == NULL might not even have
children.  If we treated them like filters and skipped through them, we
would have to return NULL if there is no child.

> 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.

Hm.  I implicitly always assume that filters always have a filter child,
so I tend to not even question that part.

Max