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

[Thomas Lamprecht]

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

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

>  	} else {
>  	    return $code->();
>  	}
>