[pve-devel] [PATCH installer 2/4] sys: command: allow terminating the process early from log subroutine
Christoph Heiss
c.heiss at proxmox.com
Mon Feb 12 13:06:36 CET 2024
Thanks for the review!
On Mon, Feb 12, 2024 at 11:58:27AM +0100, Thomas Lamprecht wrote:
> Am 09/02/2024 um 11:55 schrieb Christoph Heiss:
> > This is done in a entirely backwards-compatible way, i.e. existing
> > usages don't need any modification.
>
> can you actually describe here that you do so by checking the return
> value of the stdout parser closure, can be done in a short sentence
> and surely doesn't hurt.
I'll add some more documentation for this in v2.
>
> >
> > Signed-off-by: Christoph Heiss <c.heiss at proxmox.com>
> > ---
> > Proxmox/Sys/Command.pm | 9 ++++++++-
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/Proxmox/Sys/Command.pm b/Proxmox/Sys/Command.pm
> > index c3e24b3..bf67b27 100644
> > --- a/Proxmox/Sys/Command.pm
> > +++ b/Proxmox/Sys/Command.pm
> > @@ -114,7 +114,14 @@ sub run_command {
> > $logout .= $buf;
> > while ($logout =~ s/^([^\010\r\n]*)(\r|\n|(\010)+|\r\n)//s) {
> > my $line = $1;
> > - $func->($line) if $func;
> > + if ($func) {
> > + my $ret = $func->($line);
> > + if (defined($ret) && $ret == 1) {
>
> maybe we can define a constant for this, so that we have
> descriptive name that better conveys the meaning of this special
> return value, e.g., something like:
Sounds good, I will add that.
>
> use constant RET_TIMED_OUT => 1;
>
> (no hard feelings on that name, just used the first thing that came to
> my mind)
>
>
> > + kill('KILL', $pid);
> > + waitpid($pid, 0);
>
> FWIW, this can block forever too though?
It /could/ probably happen if the process e.g. somehow enters D-state
after reading the previous line of output, good catch.
>
> How about factoring out the kill+waiting in it's own sub (could be
> also one for waitpid with timeout and combined kill+waitpid that reuses
> the first one), that does a TERM first, checks with WNOHANG if the
> target PID exited and if not, sends the KILL and loops until a timeout
> (say, /throws dice/, 5s) expired.
>
> That way no D-state, or whatever else can block the process' death,
> is hanging up the installer.
That seems like the /really/ bulletproof way.
This was more a proof-of-concept-fix anyway in the beginning, so a bit
of refactoring on that thing is warranted after all, I guess.
>
> > + return $ostream;
> > + }
> > + };
> > }
> >
> > } elsif ($h eq $error) {
>
More information about the pve-devel
mailing list