From: Alberto Garcia <berto@igalia.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
qemu-devel@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>,
qemu-block@nongnu.org, Max Reitz <mreitz@redhat.com>
Subject: Re: [PATCH v4 2/6] block: Allow changing bs->file on reopen
Date: Wed, 24 Mar 2021 13:25:54 +0100 [thread overview]
Message-ID: <w515z1gviul.fsf@maestria.local.igalia.com> (raw)
In-Reply-To: <1a4ca40e-5924-4bef-d8b7-6a6b3eac32f5@virtuozzo.com>
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
next prev parent reply other threads:[~2021-03-24 12:27 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-17 17:15 [PATCH v4 0/6] Allow changing bs->file on reopen Alberto Garcia
2021-03-17 17:15 ` [PATCH v4 1/6] block: Add bdrv_reopen_queue_free() Alberto Garcia
2021-03-18 13:32 ` Vladimir Sementsov-Ogievskiy
2021-03-17 17:15 ` [PATCH v4 2/6] block: Allow changing bs->file on reopen Alberto Garcia
2021-03-18 14:25 ` Vladimir Sementsov-Ogievskiy
2021-03-24 12:25 ` Alberto Garcia [this message]
2021-03-24 15:08 ` Vladimir Sementsov-Ogievskiy
2021-05-05 13:58 ` Kevin Wolf
2021-05-07 7:11 ` Vladimir Sementsov-Ogievskiy
2021-05-07 14:09 ` Kevin Wolf
2021-05-10 9:26 ` Vladimir Sementsov-Ogievskiy
2021-03-17 17:15 ` [PATCH v4 3/6] iotests: Test replacing files with x-blockdev-reopen Alberto Garcia
2021-05-05 15:57 ` Kevin Wolf
2021-03-17 17:15 ` [PATCH v4 4/6] block: Support multiple reopening " Alberto Garcia
2021-03-18 14:45 ` Vladimir Sementsov-Ogievskiy
2021-05-05 16:18 ` Kevin Wolf
2021-05-06 9:21 ` Vladimir Sementsov-Ogievskiy
2021-03-17 17:15 ` [PATCH v4 5/6] iotests: Test reopening multiple devices at the same time Alberto Garcia
2021-03-17 17:15 ` [PATCH v4 6/6] block: Make blockdev-reopen stable API Alberto Garcia
2021-05-14 15:53 ` [PATCH v4 0/6] Allow changing bs->file on reopen Vladimir Sementsov-Ogievskiy
2021-06-09 15:53 ` Kevin Wolf
2021-06-09 16:40 ` Vladimir Sementsov-Ogievskiy
2021-06-10 12:10 ` Vladimir Sementsov-Ogievskiy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=w515z1gviul.fsf@maestria.local.igalia.com \
--to=berto@igalia.com \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=vsementsov@virtuozzo.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).