Re: [PATCH common 1/2] cmd: run: do not cancel the caller's alarm

[Kefu Chai <k.chai@proxmox.com>]

Hi Wolfgang,

thanks for the review!

> There is technically a race here where at *this* point the outer alarm
> can trigger.
>
> I suggest placing
>
>     $oldtimeout = alarm(0) if $timeout;
>
> *above* the `local $SIG{ALRM}` line above first, so that if a timeout is
> created, we first disable the alarm, then establish the signal handler,
> *then* the new alarm (without re-assigning to `$oldtimeout` again).

Good catch, the v2 does exactly that.

[snip]

> This now means an external timer can interrupt `$errfunc` which it could
> not previously.
>
> I think it makes more sense to `eval` the `$errfunc` call and do
> something like:
>
>     if ($err eq "got timeout\n") {
>         warn $errfunc_error;
>     } else {
>         $err = "$errfunc_error\n$err";
>     }

Done like that. The new test has an errfunc that dies on the flushed
line: run still reports its timeout and only warns the error.

[snip]

> ...then move this hunk to after the kill/wait code below, replacing
> its `die`s with appending the error similar to the above code, so
> the external timer also cannot prevent the kill/wait.
>
> This might then also make the 2nd patch unnecessary?

The series already has that order: the second patch kills and reaps
the command right after the eval, before the flush and before the
caller's alarm is re-armed. The dies on the error paths come after
the reap, so they cannot skip it either.

But I don't think the second patch becomes redundant, because that
unconditional kill/wait is what it adds in the first place. In the
first patch alone, the kills sit on the timeout, sysread and callback
paths. When the caller's alarm handler is what dies while the command
runs, the 'rbd create' case, nothing kills the command, however the
first patch is ordered.

Happy to fold the two into one patch if that reads better. I kept
them apart because they fix two different things.

One window stays open on purpose: without a timeout of its own, run
leaves the caller's alarm armed during the kill/wait too, so its
handler can still die between the eval and the kill. Disarming it
there would leave nothing to break out of the waitpid when a child is
stuck in uninterruptible sleep, and I prefer keeping the caller's
watchdog working over guaranteeing the reap.

I'll send the v2 with both changes and their tests right after this.