14.06.2019 16:50, Max Reitz wrote: > On 14.06.19 15:26, Vladimir Sementsov-Ogievskiy wrote: >> 13.06.2019 1:09, Max Reitz wrote: >>> Use child access functions when iterating through backing chains so >>> filters do not break the chain. >>> >>> Signed-off-by: Max Reitz <mre...@redhat.com> >>> --- >>> block.c | 40 ++++++++++++++++++++++++++++------------ >>> 1 file changed, 28 insertions(+), 12 deletions(-) >>> >>> diff --git a/block.c b/block.c >>> index 11f37983d9..505b3e9a01 100644 >>> --- a/block.c >>> +++ b/block.c > > [...] > >>> @@ -4273,11 +4274,18 @@ int bdrv_change_backing_file(BlockDriverState *bs, >>> BlockDriverState *bdrv_find_overlay(BlockDriverState *active, >>> BlockDriverState *bs) >>> { >>> - while (active && bs != backing_bs(active)) { >>> - active = backing_bs(active); >>> + bs = bdrv_skip_rw_filters(bs); >>> + active = bdrv_skip_rw_filters(active); >>> + >>> + while (active) { >>> + BlockDriverState *next = bdrv_backing_chain_next(active); >>> + if (bs == next) { >>> + return active; >>> + } >>> + active = next; >>> } >>> >>> - return active; >>> + return NULL; >>> } >> >> Semantics changed for this function. >> It is used in two places >> 1. from bdrv_find_base wtih @bs=NULL, it should be unchanged, as I hope we >> will never have >> filter node as a bottom of some valid chain >> >> 2. from qmp_block_commit, only to check op-blocker... hmmm. I really don't >> understand, >> why do we check BLOCK_OP_TYPE_COMMIT_TARGET on top_bs overlay.. top_bs >> overlay is out of the job, >> what is this check for? > > There is a loop before this check which checks that the same blocker is > not set on any nodes between top and base (both inclusive). I guess > non-active commit checks the node above @top, too, because its backing > file will change.
So in this case frozen chain works better. > >>> /* Given a BDS, searches for the base layer. */ > > [...] > >>> @@ -5149,7 +5165,7 @@ BlockDriverState >>> *bdrv_find_backing_image(BlockDriverState *bs, >>> char *backing_file_full_ret; >>> >>> if (strcmp(backing_file, curr_bs->backing_file) == 0) { >> >> hmm, interesting, what bs->backing_file now means? It's strange enough to >> store such field on >> bds, when we have backing link anyway.. > > Patch 37 has you covered. :-) > Hmm, if it has removed this field, but it doesn't) So, we finished with some object, called "overlay", but it is not an overlay of bs, it's overlay of first non-implicit filtered node in bs backing chain, it may be found by bdrv_find_overlay() helper (which is almost unused and my be safely dropped), and filename of this "overlay" is stored in bs->backing_file string variable, keeping in mind that bs->backing is pointer to backing child of bs which is completely another thing? Oh, no, everything related to filename-based backing chain logic is not for me o_O. If something doesn't work with filename-based logic users should use node-names.. And I'd prefer to deprecate filename based interfaces at all. -- Best regards, Vladimir