Re: [pve-devel] [PATCH container v6 2/4] config: apply_pending: get unused volid through parse_volume()

From: Fiona Ebner <f.ebner@proxmox.com>
To: "Proxmox VE development discussion" <pve-devel@lists.proxmox.com>,
	"Michael Köppl" <m.koeppl@proxmox.com>
Subject: Re: [pve-devel] [PATCH container v6 2/4] config: apply_pending: get unused volid through parse_volume()
Date: Tue, 20 May 2025 15:45:28 +0200	[thread overview]
Message-ID: <de3a2d79-e610-4c47-9439-b740401b7842@proxmox.com> (raw)
In-Reply-To: <20250520090818.44881-3-m.koeppl@proxmox.com>

Am 20.05.25 um 11:08 schrieb Michael Köppl:
> The volume ID returned by parse_volume() is used here as opposed to
> $conf->{$opt} used for regular mount points because $conf->{$opt} is not
> necessarily a valid volume ID in this case. This distinction is
> important because is_volume_in_use does internally not consider
> 'unused*' a valid volume key and does not check it. parse_volume knows
> about 'unused*' and returns a valid volume ID.

The key is not passed to is_volume_in_use(), just the value. The
rationale is imprecise in that regard.

Hmm, what can the value for 'unusedN' be other than a valid volume ID?
Do you have an example? I'd rather avoid adding entries where it is not
a volume ID to 'unusedN' in the first place.

> No functional changes intended.

Yes there is, as you want to fix handling 'unusedN' entries. Why make
the change otherwise?

> 
> Originally-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
>  src/PVE/LXC/Config.pm | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
> index 0740e8c..a7c1023 100644
> --- a/src/PVE/LXC/Config.pm
> +++ b/src/PVE/LXC/Config.pm
> @@ -1557,15 +1557,20 @@ sub vmconfig_apply_pending {
>      foreach my $opt (sort keys %$pending_delete_hash) {
>  	next if $selection && !$selection->{$opt};
>  	eval {
> +	    my $mp = $class->parse_volume($opt, $conf->{$opt});

You don't know that $opt is for a volume at this stage, it can be
anything, e.g. 'net0'.

> +
>  	    if ($opt =~ m/^mp(\d+)$/) {
> -		my $mp = $class->parse_volume($opt, $conf->{$opt});
>  		if ($mp->{type} eq 'volume') {
>  		    $class->add_unused_volume($conf, $mp->{volume})
>  			if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);
>  		}
>  	    } elsif ($opt =~ m/^unused(\d+)$/) {
> +		# $mp->{volume} is used for is_volume_in_use() because parse_volume()
> +		# knows about 'unused*' and will return a valid volume ID whereas
> +		# $conf->{$opt} is not guaranteed to contain a valid volume ID in this
> +		# case.
>  		PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $conf->{$opt})
> -		    if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);
> +		    if !$class->is_volume_in_use($conf, $mp->{volume}, 1, 1);
>  	    } elsif ($opt =~ m/^net(\d+)$/) {
>  		if ($have_sdn) {
>  		    my $net = $class->parse_lxc_network($conf->{$opt});

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel