Am 27.05.25 um 11:34 schrieb Michael Köppl:
> 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().
As we had discussed off-list, that check should even go in vdisk_free()
itself, not delete_mountpoint_volume(). And regarding the parsing, that
was an oversight on my part because the parameter is confusingly named
$volume, but it's already a volume ID. We already had discussed this too ;)
> 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
Yes, the $volume variable there is also just confusingly named, as it's
also already a volume ID.
> 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.
We don't need to adapt the signature of delete_mountpoint_volume(),
except renaming the argument $volume => $volid would be worthwhile.
> 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.
No, I didn't mean those.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
