[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 2/6] block: Allow changing bs->file on reopen
From: |
Alberto Garcia |
Subject: |
Re: [PATCH v4 2/6] block: Allow changing bs->file on reopen |
Date: |
Wed, 24 Mar 2021 13:25:54 +0100 |
User-agent: |
Notmuch/0.18.2 (http://notmuchmail.org) Emacs/24.4.1 (i586-pc-linux-gnu) |
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