From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9])
by lore.proxmox.com (Postfix) with ESMTPS id D2F9D1FF16F
for <inbox@lore.proxmox.com>; Tue, 27 May 2025 11:34:38 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 9847DFF25;
Tue, 27 May 2025 11:34:50 +0200 (CEST)
Message-ID: <8f515019-1d9d-4760-a027-4b7210d7f37d@proxmox.com>
Date: Tue, 27 May 2025 11:34:17 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
To: Fiona Ebner <f.ebner@proxmox.com>,
Proxmox VE development discussion <pve-devel@lists.proxmox.com>
References: <20250520090818.44881-1-m.koeppl@proxmox.com>
<20250520090818.44881-4-m.koeppl@proxmox.com>
<3cb1c0a2-19d2-4fdc-a8a7-b4a2835d423b@proxmox.com>
From: =?UTF-8?Q?Michael_K=C3=B6ppl?= <m.koeppl@proxmox.com>
Content-Language: en-US
In-Reply-To: <3cb1c0a2-19d2-4fdc-a8a7-b4a2835d423b@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.004 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
Subject: Re: [pve-devel] [PATCH container v6 3/4] fix #3711: lxc: allow
removing unused mp if storage no longer exists
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>
On 5/20/25 16:03, Fiona Ebner wrote:
>> @@ -1558,13 +1564,17 @@ sub vmconfig_apply_pending {
>> next if $selection && !$selection->{$opt};
>> eval {
>> my $mp = $class->parse_volume($opt, $conf->{$opt});
>> + my ($storeid, undef) = PVE::Storage::parse_volume_id($mp->{volume});
>>
>> if ($opt =~ m/^mp(\d+)$/) {
>> 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+)$/) {
>> + } elsif (
>> + $opt =~ m/^unused(\d+)$/
>> + && PVE::Storage::storage_config($storecfg, $storeid, 1)
>> + ) {
>> # $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
>
> Can we put the parsing/check in delete_mountpoint_volume() itself
> instead? And maybe print an informational message if the storage didn't
> exist anymore. That would also cover the caller in patch 1/4, although
> we still might want to use the eval+print there for other kinds of errors.
I think moving the check if the storage exists inside definitely makes
sense here. Thanks for the suggestion. I adapted the
delete_mountpoint_volume() for a v7, but opted to keep the parsing of
the volume ID in vmconfig_apply_pending() and
vmconfig_hotplug_pending(). destroy_lxc_container() uses
foreach_volume_full() to iterate over all its mountpoints, which uses
parse_volume() internally. So the $volume input used there is already
what we want, as opposed to the $conf and $opt arguments used by the
other 2 callers. Moving the parsing into delete_mountpoint_volume()
would require changing other parts of the implementation (to get the
required inputs for all 3 callers) for little benefit. One way to avoid
that would be a signature such as
my ($storage_cfg, $vmid, $volid, $conf, $opt) = @_;
and using $conf->{$opt} to call parse_volume() if $volid is undef. But I
think that's not very transparent to the caller.
Also, I don't know if you meant the is_volume_in_use() checks as well,
but I think keeping those where they are makes it very obvious from the
code that the delete only happens if the volume is not in use.
Would love your input on this, maybe I'm just missing something.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel