Re: [pve-devel] [PATCH container v6 1/4] fix #3711: lxc: print warning if storage for mounted volume does not exist anymore

From: "Michael Köppl" <m.koeppl@proxmox.com>
To: Thomas Lamprecht <t.lamprecht@proxmox.com>,
	Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH container v6 1/4] fix #3711: lxc: print warning if storage for mounted volume does not exist anymore
Date: Tue, 27 May 2025 09:37:31 +0200	[thread overview]
Message-ID: <b23c3fb6-8e8c-4ae1-81f9-9155b535506f@proxmox.com> (raw)
In-Reply-To: <92f78ba0-44bb-41e9-b43b-9e4b0b47390b@proxmox.com>

On 5/22/25 08:08, Thomas Lamprecht wrote:
> 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.

I opted for wrapping the delete_mountpoint_volume in an eval in this
case, so the comment wasn't necessary anymore, but I'll keep that in
mind. I definitely understand the need for succinct comments. Thanks for
the feedback! Also, I won't be sending patches with crypto code anytime
soon, I promise ;)

> 
>>> +	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