On Thu 18 Mar 2021 03:25:07 PM CET, Vladimir Sementsov-Ogievskiy
<vsementsov@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