From: "Thomas Ellmenreich" <t.ellmenreich@proxmox.com>
To: "Filip Schauer" <f.schauer@proxmox.com>, <pve-devel@lists.proxmox.com>
Subject: Re: [RFC pve-container 1/1] mountpoint backup: allow changing backup flag at runtime
Date: Fri, 04 Sep 2026 12:11:19 +0200 [thread overview]
Message-ID: <DL6G8ZLSYZOV.KY6LJB3ZIHLP@proxmox.com> (raw)
In-Reply-To: <26b4609b-3645-4087-bb98-4eda32a7d177@proxmox.com>
On Mon Aug 31, 2026 at 12:36 PM CEST, Filip Schauer wrote:
[snip]
>> +# Checks whether the changed properties can be applied at runtime. It only
>> +# returns true if all changes can be applied in this way. If even one
>> +# property cannot be applied, returns false to ensure changes are applied
>> +# as a unit.
>> +my $mountpoint_config_fast_plug_safe = sub {
>> + my ($new, $old) = @_;
>> + my @fast_plug_safe_keys = ("backup");
>> +
>> + for my $key (uniq(keys %$new, keys %$old)) {
>> + next if first { $_ eq $key } @fast_plug_safe_keys;
>> + return 0 if ($new->{$key} // '') ne ($old->{$key} // '');
>
> From testing I noticed that when toggling `backup` on a mount point
> specifying a custom `idmap`, the `backup` property is still marked as
> pending.
> The reason seems to be that this is comparing array references. Even if
> the arrays both hold the same data, the references still differ.
Ack, thanks for this, stupid mistake.
>
>> + }
>> +
>> + 1;
>> +};
>> +
>> sub apply_pending_mountpoint {
>> my ($class, $vmid, $conf, $opt, $storecfg, $running) = @_;
>>
>> my $mp = $class->parse_volume($opt, $conf->{pending}->{$opt});
>> my $old = $conf->{$opt};
>> +
>> + if (exists($conf->{$opt}) && $running) {
>> + if ($mountpoint_config_fast_plug_safe->($mp, $class->parse_volume($opt, $old))) {
>> + return;
>> + }
>> + die "skip\n"; # don't try to hotplug over existing mp
>
> Doesn't this make the later `die "skip\n" if $running && defined($old);`
> unreachable?
Good catch, yes. I think it will be unreachable, although I'm not sure what to
do with the open #TODO, especially since I don't quite understand what the
exact meaning of "changing" is in this context.
```perl
if ($mp->{type} eq 'volume' && $mp->{volume} =~ $PVE::LXC::NEW_DISK_RE) {
# more code
} else {
die "skip\n" if $running && defined($old); # TODO: "changing" mount points?
# more code
}
```
[snip]
prev parent reply other threads:[~2026-09-04 10:11 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-08-27 12:26 [RFC pve-container 1/1] mountpoint backup: allow changing backup flag at runtime Thomas Ellmenreich
2026-08-31 10:36 ` Filip Schauer
2026-09-04 10:11 ` Thomas Ellmenreich [this message]
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=DL6G8ZLSYZOV.KY6LJB3ZIHLP@proxmox.com \
--to=t.ellmenreich@proxmox.com \
--cc=f.schauer@proxmox.com \
--cc=pve-devel@lists.proxmox.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.