qemu-devel
[Top][All Lists]
Advanced

[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: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v4 2/6] block: Allow changing bs->file on reopen
Date: Wed, 24 Mar 2021 18:08:38 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1

24.03.2021 15:25, Alberto Garcia wrote:
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?

We do.. But as I understand resent Kevin's explanation on my "[PATCH RFC 0/3] block: 
drop inherits_from", inherits_from exists to support pre-blockdev era..

Anyway, better to support it and don't care, and drop all inherits_from logic 
at some bright future point.


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



--
Best regards,
Vladimir



reply via email to

[Prev in Thread] Current Thread [Next in Thread]