14.06.2019 18:52, Max Reitz wrote: > On 14.06.19 15:42, Vladimir Sementsov-Ogievskiy wrote: >> 13.06.2019 1:09, Max Reitz wrote: >>> Reopening a node's backing child needs a bit of special handling because >>> the "backing" child has different defaults than all other children >>> (among other things). Adding filter support here is a bit more >>> difficult than just using the child access functions. In fact, we often >>> have to directly use bs->backing because these functions are about the >>> "backing" child (which may or may not be the COW backing file). >>> >>> Signed-off-by: Max Reitz <mre...@redhat.com> >>> --- >>> block.c | 36 +++++++++++++++++++++++++++++------- >>> 1 file changed, 29 insertions(+), 7 deletions(-) >>> >>> diff --git a/block.c b/block.c >>> index 505b3e9a01..db2759c10d 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -3542,17 +3542,39 @@ static int >>> bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, >>> } >>> } >>> >>> + /* >>> + * Ensure that @bs can really handle backing files, because we are >>> + * about to give it one (or swap the existing one) >>> + */ >>> + if (bs->drv->is_filter) { >>> + /* Filters always have a file or a backing child */ >>> + if (!bs->backing) { >>> + error_setg(errp, "'%s' is a %s filter node that does not >>> support a " >>> + "backing child", bs->node_name, >>> bs->drv->format_name); >>> + return -EINVAL; >>> + } >>> + } else if (!bs->drv->supports_backing) { >>> + error_setg(errp, "Driver '%s' of node '%s' does not support >>> backing " >>> + "files", bs->drv->format_name, bs->node_name); >>> + return -EINVAL; >>> + } >> >> hmm, shouldn't we have these checks for overlay_bs? > > I think this is correct here because this is the only node the user has > control over, so this is the only one we can reasonably complain about. > > And I do think it is reasonable to complain about. > >>> + >>> /* >>> * Find the "actual" backing file by skipping all links that point >>> * to an implicit node, if any (e.g. a commit filter node). >>> + * We cannot use any of the bdrv_skip_*() functions here because >>> + * those return the first explicit node, while we are looking for >>> + * its overlay here. >>> */ >>> overlay_bs = bs; >>> - while (backing_bs(overlay_bs) && backing_bs(overlay_bs)->implicit) { >>> - overlay_bs = backing_bs(overlay_bs); >>> + while (bdrv_filtered_bs(overlay_bs) && >>> + bdrv_filtered_bs(overlay_bs)->implicit) >>> + { >>> + overlay_bs = bdrv_filtered_bs(overlay_bs); >>> } >> >> here, overlay_bs may be some filter with file child .. >> >>> >>> /* If we want to replace the backing file we need some extra checks >>> */ >>> - if (new_backing_bs != backing_bs(overlay_bs)) { >>> + if (new_backing_bs != bdrv_filtered_bs(overlay_bs)) { >>> /* Check for implicit nodes between bs and its backing file */ >>> if (bs != overlay_bs) { >>> error_setg(errp, "Cannot change backing link if '%s' has " >>> @@ -3560,8 +3582,8 @@ static int bdrv_reopen_parse_backing(BDRVReopenState >>> *reopen_state, >>> return -EPERM; >>> } >>> /* Check if the backing link that we want to replace is frozen */ >>> - if (bdrv_is_backing_chain_frozen(overlay_bs, >>> backing_bs(overlay_bs), >>> - errp)) { >>> + if (bdrv_is_backing_chain_frozen(overlay_bs, >>> + child_bs(overlay_bs->backing), >>> errp)) { >> >> .. and here we are doing wrong thing, as it don't have backing child >> >> Aha, you use the fact that we now don't have implicit filters with file >> child. Then, should >> we add an assertion for this? > > No, that wasn’t my intention. The real reason is that all of this is a > mess. > > Here is the full context: > >> overlay_bs = bs; >> while (bdrv_filtered_bs(overlay_bs) && >> bdrv_filtered_bs(overlay_bs)->implicit) >> { >> overlay_bs = bdrv_filtered_bs(overlay_bs); >> } >> >> /* If we want to replace the backing file we need some extra checks */ >> if (new_backing_bs != bdrv_filtered_bs(overlay_bs)) { >> /* Check for implicit nodes between bs and its backing file */ >> if (bs != overlay_bs) { >> error_setg(errp, "Cannot change backing link if '%s' has " >> "an implicit backing file", bs->node_name); >> return -EPERM; >> } >> /* Check if the backing link that we want to replace is frozen */ >> if (bdrv_is_backing_chain_frozen(overlay_bs, >> child_bs(overlay_bs->backing), >> errp)) { >> return -EPERM; >> } > > Note the “Check for implicit nodes” thing. If we get to the frozen > check, we have already confirmed that overlay_bs == bs, so we then know > that overlay_bs->backing works. > > I can add an additional comment to make that more clear. It took myself > quite a bit of digging to figure that out again...
Aha, I see it. Comment would be good. > > (The reason for the loop is that we want to be able to recognize when > the user tries to not change the backing file. In that case, we don’t > have to do anything, but because the user doesn’t know about implicit > nodes, we have to skip them in order to check whether the user actually > doesn’t want to change anything.) > > Max > >>> return -EPERM; >>> } >>> reopen_state->replace_backing_bs = true; >>> @@ -3712,7 +3734,7 @@ int bdrv_reopen_prepare(BDRVReopenState >>> *reopen_state, BlockReopenQueue *queue, >>> * its metadata. Otherwise the 'backing' option can be omitted. >>> */ >>> if (drv->supports_backing && reopen_state->backing_missing && >>> - (backing_bs(reopen_state->bs) || >>> reopen_state->bs->backing_file[0])) { >>> + (reopen_state->bs->backing || reopen_state->bs->backing_file[0])) { >>> error_setg(errp, "backing is missing for '%s'", >>> reopen_state->bs->node_name); >>> ret = -EINVAL; >>> @@ -3857,7 +3879,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state) >>> * from bdrv_set_backing_hd()) has the new values. >>> */ >>> if (reopen_state->replace_backing_bs) { >>> - BlockDriverState *old_backing_bs = backing_bs(bs); >>> + BlockDriverState *old_backing_bs = child_bs(bs->backing); >>> assert(!old_backing_bs || !old_backing_bs->implicit); >>> /* Abort the permission update on the backing bs we're detaching >>> */ >>> if (old_backing_bs) { >>> >> >> > > -- Best regards, Vladimir