[pve-devel] [PATCH cluster v2 1/3] cfs_lock: swap checks for specific errors with $got_lock

[Thomas Lamprecht]

We checked if a specific error was set or, respectively, not set to
know if we got the lock or not.
The check if we may unlock again was negated and thus could lead to
problems, in specific - rather unlikely - cases.

Add a $got_lock variable which only gets set when we really got the
lock, this is easier to track for humans reading the code and should
cope better against code changes, e.g., it does not breaks just if an
error message typo got corrected a few lines above.

While refactoring for the new variable, set the $noerr parameter of
check_cfs_quorum() as we do not want to die here.
---

changes v1 -> v2:
* added missing else branch for when we get the lock instantly

 data/PVE/Cluster.pm | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/data/PVE/Cluster.pm b/data/PVE/Cluster.pm
index 70ce250..f0708a0 100644
--- a/data/PVE/Cluster.pm
+++ b/data/PVE/Cluster.pm
@@ -861,6 +861,7 @@ my $cfs_lock = sub {
     my ($lockid, $timeout, $code, @param) = @_;
 
     my $res;
+    my $got_lock = 0;
 
     # this timeout is for aquire the lock
     $timeout = 10 if !$timeout;
@@ -887,11 +888,14 @@ my $cfs_lock = sub {
 		if (!(mkdir $filename)) {
 		    (utime 0, 0, $filename); # cfs unlock request
 		} else {
+		    $got_lock = 1;
 		    print STDERR " OK\n";
 		    last;
 		}
 		sleep(1);
 	    }
+	} else {
+	    $got_lock = 1;
 	}
 
 	# fixed command timeout: cfs locks have a timeout of 120
@@ -910,14 +914,9 @@ my $cfs_lock = sub {
 
     alarm(0);
 
-    if ($err && ($err eq "got lock request timeout\n") &&
-	!check_cfs_quorum()){
-	$err = "$msg: no quorum!\n";
-    }
+    $err = "$msg: no quorum!\n" if !$got_lock && !check_cfs_quorum(1);
 
-    if (!$err || $err !~ /^got lock timeout -/) {
-	rmdir $filename; # cfs unlock
-    }
+    rmdir $filename if $got_lock; # if we held the lock always unlock again
 
     if ($err) {
         $@ = $err;
-- 
2.11.0