On Thu 18 Mar 2021 03:25:07 PM CET, Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> wrote: >> static int bdrv_reopen_prepare(BDRVReopenState *reopen_state, >> BlockReopenQueue *queue, >> - Transaction *set_backings_tran, Error >> **errp); >> + Transaction *tran, Error **errp); > > I'd not call it just "tran" to not interfere with transaction > actions. Of course, reopen should be finally refactored to work > cleanly on Transaction API, but that is not done yet. And here we pass > a transaction pointer only to keep children modification.. So, let's > make it change_child_tran, or something like this.
The name change looks good to me. >> + } else if (bdrv_recurse_has_child(new_child_bs, bs)) { >> + error_setg(errp, "Making '%s' a %s of '%s' would create a >> cycle", >> + str, parse_file ? "file" : "backing file", > > maybe s/"file"/"file child"/ Ok. >> default: >> - /* 'backing' does not allow any other data type */ >> + /* The options QDict has been flattened, so 'backing' and 'file' >> + * do not allow any other data type here. */ > > checkpatch should complain that you didn't fix style of the comment... I actually don't like to use the proposed style for 2-line comments in many cases. I think it makes sense for big comment blocks but adds noise for shorter comments. >> + } else { >> + /* >> + * 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 */ > > Probably we can assert bs->backing, as otherwise backing option should > be unsupported [preexisting, not about this patch] Yes, I see that this was added in commit 1d42f48c3a, maybe Max has good reasons to keep it this way? >> if (bdrv_is_backing_chain_frozen(overlay_bs, >> - child_bs(overlay_bs->backing), >> errp)) >> + bdrv_filter_or_cow_bs(overlay_bs), >> + errp)) >> { >> return -EPERM; >> } I just realized that this part is probably not ok if you want to change bs->file on a node that is not a filter, because this would check bs->backing->frozen and not bs->file->frozen. >> + if (parse_file) { >> + /* Store the old file bs, we'll need to refresh its permissions >> */ >> + reopen_state->old_file_bs = bs->file->bs; >> + >> + /* And finally replace the child */ >> + bdrv_replace_child(bs->file, new_child_bs, tran); > > I think that actually, we need also to update inherits_from and do > refresh_limits like in bdrv_set_backing_noperm(). Yes, I think you're right. > Probably, bdrv_replace_child should do it. Probably not (there are > still a lot of things to refactor in block.c :).. > > Hm. Also, using blockdev-reopen probably means that we are in a >blockdev word, so we should not care about inherits_from here. But with blockdev-reopen we do update inherits_from for backing files, don't we? > Also, you don't create reopen_state->replace_file_bs, like for > backing.. On bdrv_reopen_comnmit replace_backing_bs is used to remove > corresponding options.. Shouldn't we do the same with file options? I think you're right. >> - self.reopen(opts, {'file': 'not-found'}, "Cannot change the option >> 'file'") >> - self.reopen(opts, {'file': ''}, "Cannot change the option 'file'") >> + self.reopen(opts, {'file': 'not-found'}, "Cannot find device='' nor >> node-name='not-found'") > > Interesting that error-message say about device='', not 'not-found'... That's because 'file' refers to a node name. Thanks for reviewing, Berto