From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 016FF1FF0DF for ; Thu, 05 Nov 2026 14:21:06 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id CB2E4215B9; Thu, 05 Nov 2026 14:21:01 +0100 (CET) Date: Thu, 5 Nov 2026 14:20:55 +0100 From: Wolfgang Bumiller To: Kefu Chai Subject: Re: [PATCH common 1/2] cmd: run: do not cancel the caller's alarm Message-ID: References: <20261021110126.4024539-1-k.chai@proxmox.com> <20261021110126.4024539-2-k.chai@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20261021110126.4024539-2-k.chai@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1793884856017 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.385 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: PUMIZOQ5TREV5OQYB3UZR2WLELRBDBFE X-Message-ID-Hash: PUMIZOQ5TREV5OQYB3UZR2WLELRBDBFE X-MailFrom: w.bumiller@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pve-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 > --- > 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 > > > > > --