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 9B02E1FF0C0 for ; Wed, 21 Oct 2026 13:01:52 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id D1FC221432; Wed, 21 Oct 2026 13:01:41 +0200 (CEST) From: Kefu Chai To: pve-devel@lists.proxmox.com Subject: [PATCH common 2/2] cmd: run: kill the command when run dies Date: Wed, 21 Oct 2026 19:01:26 +0800 Message-ID: <20261021110126.4024539-3-k.chai@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20261021110126.4024539-1-k.chai@proxmox.com> References: <20261021110126.4024539-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: 1792580492064 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.298 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: T67GC2Y3GM2UFZJMIRM53C2AXYBZTJTJ X-Message-ID-Hash: T67GC2Y3GM2UFZJMIRM53C2AXYBZTJTJ 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 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 a6bb17d..581bb93 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; @@ -223,51 +224,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(); @@ -282,7 +262,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"; @@ -306,6 +286,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) { &$errfunc(undef); # flush laststderr } @@ -320,8 +306,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 191f9f5..1dbb0cc 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 (); @@ -95,9 +96,46 @@ for my $test ($caller_alarm_tests->@*) { cmp_ok($after, '<', 2.5, "run $test->{name} does not delay the caller's alarm"); } +# 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