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

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Wed, Oct 21, 2026 at 07:01:25PM +0800, Kefu Chai wrote:
> 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;

There is technically a race here where at *this* point the outer alarm
can trigger.

I suggest placing

    $oldtimeout = alarm(0) if $timeout;

*above* the `local $SIG{ALRM}` line above first, so that if a timeout is
created, we first disable the alarm, then establish the signal handler,
*then* the new alarm (without re-assigning to $oldtimeout again).


> -        $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;

This now means an external timer can interrupt `$errfunc` which it could
not previously.

I think it makes more sense to `eval` the `$errfunc` call and do
something like:

    if ($err eq "got timeout\n") {
        warn $errfunc_error;
    } else {
        $err = "$errfunc_error\n$err";
    }

then…

>  
>      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);
> +    }

…then move this hunk to after the kill/wait code below, replacing its
`die`s with appending the error similar to the above code, so the
external timer also cannot prevent the kill/wait.

This might then also make the 2nd patch unnecessary?

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

--