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 <mre...@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
signature.asc
Description: OpenPGP digital signature
