[pve-devel] [RFC PATCH 1/1] Add run_or_get_killed utility

[Wolfgang Bumiller]

On Mon, Jun 19, 2017 at 11:21:08AM +0200, Emmanuel Kasper wrote:
> This runs subroutine in a forked process
> and kills it after a timeout
> ---
>  src/PVE/Tools.pm | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
> index da7da5d..0bf94ae 100644
> --- a/src/PVE/Tools.pm
> +++ b/src/PVE/Tools.pm
> @@ -841,6 +841,49 @@ sub next_spice_port {
>      return next_unused_port(61000, 61099, $family, $address);
>  }
>  
> +# sigkill a $sub running in a fork if it can't write a pipe after $timeout
> +# the $sub has to return a single scalar
> +sub run_or_get_killed {

The name itself should also convey the fact that the code will run in a
fork. It's important because it adds some restrictions like the fact
that modifications to globals (or even function-locals outside the inner
code's scope) will not be propagated to the parent process, and exit()
won't affect the parent.

Maybe 'run_fork_with_timeout' would be a better fit.

> +    my ($sub, $timeout) = @_;

The existing 'run_with_timeout' function has the parameters the other
way round. It's also easier to read calls with inline `sub { code }`
expressions if the timeout comes first since it may otherwise be buried
further down together with the closing braces.

> +
> +    my $res;
> +
> +    my $pipe = IO::Pipe->new();
> +    my $child = fork();

Before doing this you should suspend previous alarms, like
run_with_timeout() does right at the beginning. The child does not
inherit timers, but the parent might get interrupted before getting to
reading the result or reaping the child process, resulting in zombies.
(Eg. the timer could trigger the moment right after the fork() syscall)
(Don't forget to restore the alarm at the end.)

> +    if (!defined($child)) {
> +	warn "fork failed: $!\n";

Why not just die? This is an actual exception after all.

> +	return $res;
> +    }
> +
> +    if (!$child) {
> +	$pipe->writer();
> +	eval {
> +	    $res = $sub->();
> +	    print {$pipe} "$res";
> +	    $pipe->close();
> +	};
> +	if (my $err = $@) {
> +	    warn $err;

I think a nicer interface would be adding a second pipe for exceptions
here. That way the parent process can re-throw the exact error message
and the original caller of the function can use the usual eval{} if($@)
blocks even around run_fork_with_timeout() calls.
The _exit() code then tells the parent whether to expect an error from
the pipe or a result to return.

> +	    POSIX::_exit(1);
> +	}
> +	POSIX::_exit(0);
> +    }
> +
> +    $pipe->reader();
> +
> +    my $readvalues = sub {
> +	$res = (<$pipe> =~ /^(.*)$/)[0];
> +    };
> +    eval {
> +	run_with_timeout($timeout, $readvalues);
> +    };
> +    warn $@ if $@;
> +    $pipe->close();
> +    kill('KILL', $child);
> +    waitpid($child, 0);

This may still get interrupted and leave a zombie. It's unlikely, but
probably still better to loop here and remember if we were interrupted.

> +    return $res;
> +}
> +
>  # NOTE: NFS syscall can't be interrupted, so alarm does
>  # not work to provide timeouts.
>  # from 'man nfs': "Only SIGKILL can interrupt a pending NFS operation"
> -- 
> 2.11.0