Re: [pve-devel] [PATCH container v5 2/4] lxc: get volid through parse_volume()

[Daniel Kral <d.kral@proxmox.com>]

On 5/13/25 10:03, Michael Köppl wrote:
> The value in $conf->{opt} is not necessarily a volume ID. To ensure that
> a valid volume ID is used, it is retrieved by calling parse_volume().

The patch summary and/or description should contain more information 
about the whereabouts, e.g. for here:

config: apply_pending: get unused volid through parse_volume()

nit: If there are no functional changes here, it's always nice to state 
that in the commit message, so it's clear from the start ;).

> 
> Co-authored-by: Stefan Hrdlicka
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
>   src/PVE/LXC/Config.pm | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
> index 0740e8c..7f76b34 100644
> --- a/src/PVE/LXC/Config.pm
> +++ b/src/PVE/LXC/Config.pm
> @@ -1557,15 +1557,16 @@ 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});
> +
>   	    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+)$/) {
>   		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);

I was a little confused why $mp->{volume} needs to be passed here but at 
other places (like is_volume_in_use for mp* above) $conf->{$opt} is fine.

AFAICS that is because valid_volume_keys(...) for the LXC config defines 
rootfs and mp* as valid volume keys, but not unused*, which is then used 
by foreach_volume(...) in the is_volume_in_use(...) helper, where it can 
be found to be used.

"Coincidentally" $conf->{$opt} and $mp->{volume} have the same value 
here, so this patch doesn't change the behavior here AFAICS, but a 
$class->parse_volume(...) makes it more future proof if anything changes 
here, because that helper is aware of unused and will correctly parse it 
with $parse_ct_mountpoint_full->($class, $unused_desc, ...).

It would be great if this was better documented here, but that could be 
a follow-up as it isn't really related to the series itself.

>   	    } 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