Am 20.05.25 um 15:33 schrieb Fiona Ebner:
> Am 20.05.25 um 11:08 schrieb Michael Köppl:
>> An explicit check for the existence of the storage is added to print a
>> warning and continue with the removal of the container without deleting
>> the mount point in case the storage does not exist anymore. For other
>> errors, the function should still die.
>>
>> Originally-by: Stefan Hrdlicka <s.hrdli...@proxmox.com>
>
> Nit: Ideally, you also describe the changes to the original patch here.
> For how this is usually done, see e.g.
> https://git.proxmox.com/?p=pve-container.git;a=commit;h=ee81952f4fc8faf01ed4eda5b8962d1a82d5425d
>
>> Signed-off-by: Michael Köppl <m.koe...@proxmox.com>
>> ---
>> src/PVE/LXC.pm | 12 +++++++++++-
>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
>> index 2b9f0cf..6a1ce92 100644
>> --- a/src/PVE/LXC.pm
>> +++ b/src/PVE/LXC.pm
>> @@ -953,7 +953,17 @@ sub destroy_lxc_container {
>> return if $volids->{$volume};
>> $volids->{$volume} = 1;
>>
>> - delete_mountpoint_volume($storage_cfg, $vmid, $volume);
>> + # explicitly check if storage still exists to avoid failing during
>> + # deletion of the mountpoint volume. instead, only a warning is
>> + # printed and destroying the container continues.
nit: in general: use the full width for comments (at least 80cc, 100c is
totally fine
too).
But most of the comment reads as description for what happens, which is
relatively
obvious from reading the code here, e.g. a "log_warn" call isn't exactly
complex, but
rather telling on its own already.
While comments can really help, they mostly do when they state the things that
are
not already obvious from reading the code in the local context already, like,
e.g.,
"distant" effects or assumptions, or if it really is complex and there is not a
good way to simplify the code.
If one want's a comment here it probably would be enough to write something
like:
# storages can be removed while volumes still exist, check that for better UX.
Note that your single comment is not a problem on it's own, but having a lot of
these makes reading code harder and as especially long comments describing the
code itself, and not the reasons, why's and other such rationale, tend to get
outdated fast, making it even more confusing to read.
That doesn't mean no comments though, but if, then please lets favor succinct
comments focusing on background, one or maybe two lines should be enough for
most
code that benefits from having one. Exceptions naturally exist, e.g., if you
write
some crypto code (please don't, as that's even hard to get right for field
experts
with dozens of years of good experience, but just as example) then having more
comment than code would even be expected.
>> + my ($storeid) = PVE::Storage::parse_volume_id($volume);
>> + eval { PVE::Storage::storage_config($storage_cfg, $storeid) };
>> + my $err = $@;
>> + PVE::RESTEnvironment::log_warn("failed to delete $volume, $err") if
>> $err;
>> +
>> + if (!$err) {
>> + delete_mountpoint_volume($storage_cfg, $vmid, $volume);
>> + }
>
> Can we instead just surround the delete_mountpoint_volume() call itself
> with an eval + printing warning? That also catches other situations
> where deletion fails and is simpler.
Yeah, that would be nicer. As in
eval {
foo();
bar();
}
# ... error handling
The bar method won't be called if foo dies.
>
>> };
>> PVE::LXC::Config->foreach_volume_full($conf, {include_unused => 1},
>> $remove_volume);
>>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
