Re: [pve-devel] [PATCH qemu-server 1/1] fix 1734: clone VM: if deactivation fails demote error to warning

[Fiona Ebner <f.ebner@proxmox.com>]

Am 06.03.24 um 12:37 schrieb Friedrich Weber:
> Thanks for tackling this! Can confirm this patch demotes the error to a
> warning and lets the qmclone task succeed (with a warning). GUI shows
> "Warnings: 1" and task log contains:
> 
> can't deactivate LV '/dev/foobar/vm-100-disk-0':   Logical volume
> foobar/vm-100-disk-0 in use.
> WARN: volume deactivation failed: foobar:vm-100-disk-0 at
> /usr/share/perl5/PVE/Storage.pm line 1246.
> 

Sounds like there is a missing newline after the error message in
PVE/Storage.pm. That's why Perl prints the file/line info.


>> @@ -3820,7 +3821,13 @@ __PACKAGE__->register_method({
>>  
>>  		if ($target) {
>>  		    # always deactivate volumes - avoid lvm LVs to be active on several nodes
>> -		    PVE::Storage::deactivate_volumes($storecfg, $vollist, $snapname) if !$running;
>> +		    eval {
>> +			PVE::Storage::deactivate_volumes($storecfg, $vollist, $snapname) if !$running;
>> +		    };
>> +		    my $err = $@;
>> +		    if ($err) {
>> +			log_warn("$err\n");
>> +		    }
> 
> I think the extra \n adds an unnecessary newline here, which looks a bit
> weird in the task log (though I'm not sure why the `chomp` in `log_warn`
> doesn't remove the newline).
> 
> While at it, I think the four lines can be shortened to
> 
>> log_warn($@) if $@;
> 
> Though that might be too terse -- someone with more Perl experience than
> me should judge that :)
> 

It's fine if the error is only used for printing and this comes
immediately after the eval.

In cases, you do something else with the error it can still be
if (my $err = $@) {
}