[PATCH common 1/2] cmd: run: do not cancel the caller's alarm

[Kefu Chai <k.chai@proxmox.com>]

run() calls alarm(0) when done, so a caller's pending alarm is lost,
or pushed back if run had a timeout of its own. The cfs lock relies on
such an alarm: on shared LVM, 'vgs' and 'lvcreate' run under the
storage lock, and after 'vgs' nothing limits 'lvcreate' anymore.

Only touch the alarm if run has its own timeout, and afterwards give
the caller's alarm back what is left of it.

Signed-off-by: Kefu Chai <k.chai@proxmox.com>
---
 src/PVE/Cmd.pm   | 16 ++++++++++++----
 test/cmd-test.pl | 28 ++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/src/PVE/Cmd.pm b/src/PVE/Cmd.pm
index 27c9193..a6bb17d 100644
--- a/src/PVE/Cmd.pm
+++ b/src/PVE/Cmd.pm
@@ -100,6 +100,7 @@ sub run($cmd, %param) {
     my $laststderr;
     my $timeout;
     my $oldtimeout;
+    my $armed_at;
     my $pid;
     my $exitcode = -1;
 
@@ -195,7 +196,10 @@ sub run($cmd, %param) {
 
         local $SIG{ALRM} = sub { die "got timeout\n"; }
             if $timeout;
-        $oldtimeout = alarm($timeout) if $timeout;
+        if ($timeout) {
+            $oldtimeout = alarm($timeout);
+            $armed_at = Time::HiRes::time();
+        }
 
         &$afterfork() if $afterfork;
 
@@ -295,12 +299,12 @@ sub run($cmd, %param) {
             }
         }
 
-        alarm(0);
+        alarm(0) if $timeout; # while our handler is still in place
     };
 
     my $err = $@;
 
-    alarm(0);
+    alarm(0) if $armed_at;
 
     if ($errmsg && $laststderr) {
         &$errfunc(undef); # flush laststderr
@@ -308,7 +312,11 @@ sub run($cmd, %param) {
 
     umask($old_umask) if defined($old_umask);
 
-    alarm($oldtimeout) if $oldtimeout;
+    # give the caller's alarm back what is left of it, a lock may depend on it
+    if ($oldtimeout) {
+        my $left = $oldtimeout - (Time::HiRes::time() - $armed_at);
+        alarm($left > 0 ? $left : 0.001);
+    }
 
     if ($err) {
         if ($pid && ($err eq "got timeout\n")) {
diff --git a/test/cmd-test.pl b/test/cmd-test.pl
index c2f7ae7..191f9f5 100755
--- a/test/cmd-test.pl
+++ b/test/cmd-test.pl
@@ -11,6 +11,7 @@ use lib '../src';
 use PVE::Cmd;
 
 use Test::More;
+use Time::HiRes ();
 
 # shell_quote: quote a single argument for the shell
 #
@@ -67,6 +68,33 @@ ok($@, 'run dies on a failing command without noerr');
 
 is(PVE::Cmd::run_command(['true']), 0, 'run_command works as a wrapper for run');
 
+# run must leave a pending alarm of its caller alone, like the time limit of a lock
+#
+# name  - how run is called
+# cmd   - command to run, sleeps for a second by default
+# param - parameters for run
+my $caller_alarm_tests = [
+    { name => 'without a timeout', param => {} },
+    { name => 'with a timeout', param => { timeout => 5 } },
+    {
+        name => 'with a timeout on a command that cannot start',
+        cmd => ['/nonexistent/command'],
+        param => { timeout => 5 },
+    },
+];
+for my $test ($caller_alarm_tests->@*) {
+    my $fired;
+    local $SIG{ALRM} = sub { $fired = Time::HiRes::time() };
+    my $start = Time::HiRes::time();
+    Time::HiRes::alarm(2);
+    eval { PVE::Cmd::run($test->{cmd} // ['sleep', '1'], $test->{param}->%*) };
+    Time::HiRes::sleep(0.1) until $fired || Time::HiRes::time() - $start > 5;
+    Time::HiRes::alarm(0);
+    ok($fired, "the caller's alarm still fires after run $test->{name}");
+    my $after = defined($fired) ? $fired - $start : 99;
+    cmp_ok($after, '<', 2.5, "run $test->{name} does not delay the caller's alarm");
+}
+
 # TODO:
 # - cover pipe_socket (needs a socket peer)
 # - cover timeouts and the errfunc/logfunc callbacks
-- 
2.47.3