[pve-devel] [PATCH cluster] config_change: correctly propagate $@ with nested locks

[Thomas Lamprecht]

On 7/16/19 3:30 PM, Fabian Grünbichler wrote:
> On Tue, Jul 16, 2019 at 03:16:35PM +0200, Thomas Lamprecht wrote:
>> On 7/16/19 2:18 PM, Fabian Grünbichler wrote:
>>> +	    die $@ if !defined($res) && defined($@);
>>
>> Do you think the !defined($res) is important here? For a perl "exception"
>> the $@ is normally enough, and we could just do a
>> $@ = undef;
>>
>> before the cfs_lock_file and omit the defined-ness check on $res. Would
>> guard a bit more against changing behavior (al thought such a change should
>> not happen without close looks at the users).
> 
> I did a quick grep on other call sites, and we usually just check $@. I

Yes, that's why it stuck to me as a bit odd.

> don't think we need to reset $@ since cfs_lock_* either returns
> something and $@ explicitly set to undef, or it returns undef with $@
> explicitly set to an error message.
> 
>> Also, you now do not return $res anymore, any reason for that? It probably
>> even works, as in the normal case the  "my $res = .." is the last executed
>> statement and thus gets indirectly returned (perldoc 'perlsub'), but
>> 1. the docs are a bit unclear how the last statement is defined
>>    (is it really the last executed one even for post-if)
>> 2. Even if 1. would be defined clearly, I do not want such magic behaviour
>>    especially for locking helpers ^^
> 
> nope, that was just oversight. both call sites don't care about the
> returned value ;)

Yes, but if one branch now returns the value but the other not (or
depending on perl version) then this makes for nice to debug bugs :D

> I'll send a fixed-up v2 shortly. I think there is also at least one
> wrong call in PVE::API2::Ceph and PVE::API2::Ceph::MON that I will also
> take a closer look at..

Thanks!