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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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.

> 
> 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:

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?

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.

> +			    return $ostream;
> +			}
> +		    };
>  		}
>  
>  	    } elsif ($h eq $error) {