Re: [PATCH container v9 05/10] allow pending mount point changes if storage is gone

["Michael Köppl" <m.koeppl@proxmox.com>]

On Tue Jun 30, 2026 at 3:41 PM CEST, Fiona Ebner wrote:

[snip]

>>  src/PVE/LXC/Config.pm | 43 ++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 34 insertions(+), 9 deletions(-)
>> 
>> diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
>> index c8098538..a3dd3a1a 100644
>> --- a/src/PVE/LXC/Config.pm
>> +++ b/src/PVE/LXC/Config.pm
>
> Missing import for PVE::RESTEnvironment.

Ack, will fix in v10. Thanks!

>
>> @@ -1663,8 +1663,14 @@ sub vmconfig_hotplug_pending {
>>                  # pass
>>              } elsif ($opt =~ m/^unused(\d+)$/) {
>>                  my $volid = $class->parse_volume($opt, $conf->{$opt})->{volume};
>> -                PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $volid)
>> -                    if !$class->is_volume_in_use($conf, $volid, 1, 1);
>> +                my ($storeid) = PVE::Storage::parse_volume_id($volid);
>> +                if (PVE::Storage::storage_config($storecfg, $storeid, 1)) {
>> +                    PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $volid)
>> +                        if !$class->is_volume_in_use($conf, $volid, 1, 1);
>> +                } else {
>> +                    PVE::RESTEnvironment::log_warn(
>> +                        "storage '$storeid' no longer exists, unused volume '$volid' will be removed");
>> +                }
>
> Style nit: should we add a helper for readability/to reduce bloat and
> repetition?
>
> I'm thinking about something like
>
> with_checked_volid($opt, $conf->{opt}, sub {
>     my ($volid) = @_;
>     PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $volid)
>         if !$class->is_volume_in_use($conf, $volid, 1, 1);
> });
>
> where the helper does the parsing of the volume and volid, logs the
> warning and returns if the storage does not exist and calls the
> passed-in sub if it does exist.

Sounds good, I'll add a helper. Thanks for the suggestion. Though I'd go
with something like:

```
sub with_checked_volid {
    my ($storecfg, $volid, $callback) = @_;

    my ($storeid) = PVE::Storage::parse_volume_id($volid);
    if (PVE::Storage::storage_config($storecfg, $storeid, 1)) {
        callback->();
    } else {
        PVE::RESTEnvironment::log_warn(
            "storage '$storeid' no longer exists, volume '$volid' will be removed");
    }
}
```

I'd keep the parsing of the volume outside the helper. The call sites
don't use the result of parse_volume uniformly. It seems easier to me
using the same helper across all call sites by passing in an already
resolved volid as the common denominator. What do you think?