From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40])
	by lore.proxmox.com (Postfix) with ESMTPS id 2AF161FF13E
	for <inbox@lore.proxmox.com>; Wed, 01 Jul 2026 14:16:13 +0200 (CEST)
Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1])
	by gate001.proxmox.com (Proxmox) with ESMTP id 610872138E;
	Wed, 01 Jul 2026 14:16:12 +0200 (CEST)
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Wed, 01 Jul 2026 14:15:39 +0200
Message-Id: <DJN86S1O37OM.27Q9EN0ISR3KS@proxmox.com>
Subject: Re: [PATCH container v9 05/10] allow pending mount point changes if
 storage is gone
From: =?utf-8?q?Michael_K=C3=B6ppl?= <m.koeppl@proxmox.com>
To: "Fiona Ebner" <f.ebner@proxmox.com>, =?utf-8?q?Michael_K=C3=B6ppl?=
 <m.koeppl@proxmox.com>, <pve-devel@lists.proxmox.com>
X-Mailer: aerc 0.21.0
References: <20260629140439.184878-1-m.koeppl@proxmox.com>
 <20260629140439.184878-6-m.koeppl@proxmox.com>
 <576f756c-9c81-4058-9b41-ed81a1a66a07@proxmox.com>
In-Reply-To: <576f756c-9c81-4058-9b41-ed81a1a66a07@proxmox.com>
X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2
X-Bm-Transport-Timestamp: 1782908134774
X-SPAM-LEVEL: Spam detection results:  0
	DMARC_MISSING             0.1 Missing DMARC policy
	KAM_DMARC_STATUS         0.01 Test Rule for DKIM or SPF Failure with Strict
 Alignment (newer systems)
	SPF_HELO_NONE           0.001 SPF: HELO does not publish an SPF Record
	SPF_PASS               -0.001 SPF: sender matches SPF record
Message-ID-Hash: PIZT3ZPZ2TFNU3GHM2DK6DUTJDZSHKUO
X-Message-ID-Hash: PIZT3ZPZ2TFNU3GHM2DK6DUTJDZSHKUO
X-MailFrom: m.koeppl@proxmox.com
X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop;
 banned-address; emergency; member-moderation; nonmember-moderation;
 administrivia; implicit-dest; max-recipients; max-size; news-moderation;
 no-subject; digests; suspicious-header
X-Mailman-Version: 3.3.10
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Owner: <mailto:pve-devel-owner@lists.proxmox.com>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Subscribe: <mailto:pve-devel-join@lists.proxmox.com>
List-Unsubscribe: <mailto:pve-devel-leave@lists.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(-)
>>=20
>> 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 =3D~ m/^unused(\d+)$/) {
>>                  my $volid =3D $class->parse_volume($opt, $conf->{$opt})=
->{volume};
>> -                PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $v=
olid)
>> -                    if !$class->is_volume_in_use($conf, $volid, 1, 1);
>> +                my ($storeid) =3D 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 vo=
lume '$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) =3D @_;
>     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) =3D @_;

    my ($storeid) =3D 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 r=
emoved");
    }
}
```

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?