14.06.2019 19:02, Max Reitz wrote: > On 14.06.19 16:31, Vladimir Sementsov-Ogievskiy wrote: >> 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. > > Perhaps. The op blockers are in this weird state anyway. I don’t think > we even need them any more, because the permissions were intended to > replace them (they were originally called “fine-grained op blockers”, I > seem to remember). > > I dare not touch them. > >>>>> /* 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) > > Because it’s needed. (Just not in the current form, but that’s what 37 > is for.) > >> 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? > > I don’t quite see what you mean. There is no “overlay” in this function.
Hmm, sorry, I kept in mind overlay from the next patch.. > >> 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.. > > In theory yes, but that isn’t an option for qemu-img commit, for example. And if something doesn't work with qemu-img, users should use qemu process in stopped state. And I'd prefer to deprecate most of qemu-img :) Actually we in Virtuozzo already go this way for some things. QMP interface is a lot more useful than qemu-img, and it's always simpler to maintain and develop one thing than two. > >> And I'd prefer to deprecate filename based interfaces at all. > > Me too. > > https://lists.nongnu.org/archive/html/qemu-devel/2014-09/msg04878.html > > :-/ > Really sad.. -- Best regards, Vladimir