Re: [PATCH v4 2/6] block: Allow changing bs->file on reopen

[Alberto Garcia]

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