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.gruenbich...@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..
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
