On Tue, Aug 14, 2018 at 12:13 PM Bjoern Walk <bwalk@linux.ibm.com> wrote:
Christian Ehrhardt <christian.ehrhardt@canonical.com> [2018-08-14, 11:27AM +0200]:
> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> index ecea27a2d4..46360cc051 100644
> --- a/src/util/virprocess.c
> +++ b/src/util/virprocess.c
> @@ -341,15 +341,19 @@ int virProcessKill(pid_t pid, int sig)
>   * Returns 0 if it was killed gracefully, 1 if it
>   * was killed forcibly, -1 if it is still alive,
>   * or another error occurred.
> + *
> + * Callers can proide an extra delay to wait longer
> + * than the default.
>   */
>  int
> -virProcessKillPainfully(pid_t pid, bool force)
> +virProcessKillPainfullyDelay(pid_t pid, bool force, unsigned int extradelay)
>  {
>      size_t i;
>      int ret = -1;
> +    unsigned int delay = 75 + (extradelay*5);
>      const char *signame = "TERM";

> -    VIR_DEBUG("vpid=%lld force=%d", (long long)pid, force);
> +    VIR_DEBUG("vpid=%lld force=%d delay=%u", (long long)pid, force, pid);
                                                                       ^
This probably should be delay, right?

Absolutely correct, bad old copy-pasta from the beginning of this change.
Thanks for spotting it.

Example is with 12 Devices passed through and force, which means with both patches applied:
- raise base value to wait after kill by 30 seconds (force is set) - second patch
- request 12*2 seconds extra for the devices - this patch
That should be 200+(12*2*5)=320 and that matches what I see int he log:

2018-08-16 13:04:51.999+0000: 61243: debug : virProcessKillPainfullyDelay:359 : vpid=60939 force=1 delay=320

I'm not pushing a V2 just for the change, but I have it queued with the fixed variable.
Any other feedback on this or the sibling patch?
Especially since those aren't my usual apparmor changes I'd really want some more reviews/acks before considering a push.

--
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd