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

[Fabian Grünbichler]

On Tue, Jul 16, 2019 at 03:16:35PM +0200, Thomas Lamprecht wrote:
> On 7/16/19 2:18 PM, Fabian Grünbichler wrote:
> > PVE::Cluster::cfs_lock_file sets $@ and returns undef for all errors,
> > including when $code dies. PVE::Tools::lock_file runs $code inside an
> > eval as well, so just setting $@ is not enough when nesting these two
> > types of locks.
> > 
> > re-die with the inner error to actually propagate error messages and
> > fail instead of proceeding. this triggered (probably among other cases)
> > when attempting to join an existing cluster without specifying all
> > needed links.
> > 
> > Signed-off-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
> > ---
> >  data/PVE/API2/ClusterConfig.pm | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/data/PVE/API2/ClusterConfig.pm b/data/PVE/API2/ClusterConfig.pm
> > index 6d23056..767bda9 100644
> > --- a/data/PVE/API2/ClusterConfig.pm
> > +++ b/data/PVE/API2/ClusterConfig.pm
> > @@ -172,7 +172,10 @@ my $config_change_lock = sub {
> >  	PVE::Cluster::cfs_update(1);
> >  	my $members = PVE::Cluster::get_members();
> >  	if (scalar(keys %$members) > 1) {
> > -	    return PVE::Cluster::cfs_lock_file('corosync.conf', 10, $code);
> > +	    my $res = PVE::Cluster::cfs_lock_file('corosync.conf', 10, $code);
> > +	    # cfs_lock_file only sets $@
> > +	    # lock_file does not propagate $@ unless we die here
> > +	    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
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 ;)

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