Re: [pve-devel] [PATCH installer 2/4] sys: command: allow terminating the process early from log subroutine

[Christoph Heiss <c.heiss@proxmox.com>]

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@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) {
>