From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 751BA1FF09B for ; Mon, 09 Nov 2026 09:00:13 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id D80912164D; Mon, 09 Nov 2026 09:00:07 +0100 (CET) From: Kefu Chai To: pve-devel@lists.proxmox.com Subject: [PATCH common v2 2/2] cmd: run: kill the command when run dies Date: Mon, 9 Nov 2026 15:59:38 +0800 Message-ID: <20261109075938.1737717-3-k.chai@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20261109075938.1737717-1-k.chai@proxmox.com> References: <20261109075938.1737717-1-k.chai@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1794211189017 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.225 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: BFR5WX2SMXV5MTMQ6LH7UFV3TU2PM7Z2 X-Message-ID-Hash: BFR5WX2SMXV5MTMQ6LH7UFV3TU2PM7Z2 X-MailFrom: k.chai@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: w.bumiller@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: If a signal handler of the caller dies while the command runs, run() leaves the command running. So 'pvesm alloc' on an RBD pool whose placement groups never become active fails with 'storage-'-locked command timed out - aborting and releases the lock, while 'rbd create' keeps running. Kill and reap the command after the eval if it was not reaped yet, which replaces the kill and waitpid calls on the error paths, and drop the evals around the callbacks that only rethrew. A killed 'rbd create' may still leave a partial image behind. Signed-off-by: Kefu Chai --- src/PVE/Cmd.pm | 58 ++++++++++++++++++------------------------------ test/cmd-test.pl | 40 ++++++++++++++++++++++++++++++++- 2 files changed, 60 insertions(+), 38 deletions(-) diff --git a/src/PVE/Cmd.pm b/src/PVE/Cmd.pm index 4b470a1..edff112 100644 --- a/src/PVE/Cmd.pm +++ b/src/PVE/Cmd.pm @@ -102,6 +102,7 @@ sub run($cmd, %param) { my $oldtimeout; my $armed_at; my $pid; + my $reaped; my $exitcode = -1; my $outfunc; @@ -226,51 +227,30 @@ sub run($cmd, %param) { foreach my $h (@handles) { my $buf = ''; my $count = sysread($h, $buf, 4096); - if (!defined($count)) { - my $err = $!; - kill(9, $pid); - waitpid($pid, 0); - die $err; - } + die $! if !defined($count); $select->remove($h) if !$count; if ($h eq $reader) { if ($outfunc || $logfunc) { - eval { - while ($buf =~ s/^([^\010\r\n]*)(?:\n|(?:\010)+|\r\n?)//) { - my $line = $outlog . $1; - $outlog = ''; - &$outfunc($line) if $outfunc; - &$logfunc($line) if $logfunc; - } - $outlog .= $buf; - }; - my $err = $@; - if ($err) { - kill(9, $pid); - waitpid($pid, 0); - die $err; + while ($buf =~ s/^([^\010\r\n]*)(?:\n|(?:\010)+|\r\n?)//) { + my $line = $outlog . $1; + $outlog = ''; + &$outfunc($line) if $outfunc; + &$logfunc($line) if $logfunc; } + $outlog .= $buf; } elsif (!$quiet) { print $buf; *STDOUT->flush(); } } elsif ($h eq $error) { if ($errfunc || $logfunc) { - eval { - while ($buf =~ s/^([^\010\r\n]*)(?:\n|(?:\010)+|\r\n?)//) { - my $line = $errlog . $1; - $errlog = ''; - &$errfunc($line) if $errfunc; - &$logfunc($line) if $logfunc; - } - $errlog .= $buf; - }; - my $err = $@; - if ($err) { - kill(9, $pid); - waitpid($pid, 0); - die $err; + while ($buf =~ s/^([^\010\r\n]*)(?:\n|(?:\010)+|\r\n?)//) { + my $line = $errlog . $1; + $errlog = ''; + &$errfunc($line) if $errfunc; + &$logfunc($line) if $logfunc; } + $errlog .= $buf; } elsif (!$quiet) { print STDERR $buf; *STDERR->flush(); @@ -285,7 +265,7 @@ sub run($cmd, %param) { &$errfunc($errlog) if $errfunc && $errlog; &$logfunc($errlog) if $logfunc && $errlog; - waitpid($pid, 0); + $reaped = waitpid($pid, 0); # same statement, so no signal handler runs in between if ($? == -1) { die "failed to execute\n"; @@ -309,6 +289,12 @@ sub run($cmd, %param) { alarm(0) if $armed_at; + # whatever made us die, also a signal handler of the caller, the command must not outlive us + if ($pid && !$reaped) { + kill(9, $pid); + waitpid($pid, 0); + } + if ($errmsg && $laststderr) { # the caller's alarm can fire during the flush, and the callback can # die on its own, so do not let it replace the error we already have @@ -334,8 +320,6 @@ sub run($cmd, %param) { if ($err) { if ($pid && ($err eq "got timeout\n")) { - kill(9, $pid); - waitpid($pid, 0); die "command '$cmdstr' failed: $err"; } diff --git a/test/cmd-test.pl b/test/cmd-test.pl index 518b369..86df8eb 100755 --- a/test/cmd-test.pl +++ b/test/cmd-test.pl @@ -10,6 +10,7 @@ use lib '../src'; use PVE::Cmd; +use POSIX (); use Test::More; use Time::HiRes (); @@ -112,9 +113,46 @@ for my $test ($caller_alarm_tests->@*) { like(join('', @warned), qr/boom/, 'run warns about the dying errfunc'); } +# run must not leave its command behind when it dies, whatever makes it die +# +# name - when run dies +# param - parameters for run +# alarm - seconds until the caller's alarm handler dies +# err - expected error +my $reap_tests = [ + { name => 'its own timeout fires', param => { timeout => 1 }, err => qr/got timeout/ }, + { + name => 'outfunc dies', + param => { outfunc => sub { die "outfunc failed\n" } }, + err => qr/outfunc failed/, + }, + { name => "the caller's alarm handler dies", alarm => 1, err => qr/caller timed out/ }, +]; +for my $test ($reap_tests->@*) { + my $pid; + my %param = ($test->{param} // {})->%*; + my $outfunc = delete($param{outfunc}) // sub { }; + eval { + local $SIG{ALRM} = sub { die "caller timed out\n" }; + alarm($test->{alarm}) if $test->{alarm}; + PVE::Cmd::run( + ['sh', '-c', 'echo $$; exec sleep 30'], + outfunc => sub { $pid //= $_[0]; $outfunc->(@_) }, + %param, + ); + }; + my $err = $@; + alarm(0); + like($err, $test->{err}, "run dies when $test->{name}"); + ok( + waitpid($pid, POSIX::WNOHANG) == -1 && !kill(0, $pid), + "run reaps the command when $test->{name}", + ); +} + # TODO: # - cover pipe_socket (needs a socket peer) -# - cover timeouts and the errfunc/logfunc callbacks +# - cover the errfunc/logfunc callbacks # - more tests done_testing(); -- 2.47.3