Re: [PATCH] libxl: wait for QEMU exit before restarting a domain

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2026-11-04 01:46, Jiaqing Zhao wrote:
> The PID-based device model destruction path returns immediately after
> sending SIGHUP, before QEMU has necessarily exited. When an HVM guest
> reboots with VNC enabled, the new QEMU process may fail to bind the VNC
> port because the old process still holds it.
>
> Wait for QEMU to exit in a child process after singaling it for at most
"signaling"

> 60 seconds. If waiting fails or times out, log the error and retain the
> existing domain destruction behaviour.
60 seconds seems like a lot.  Did you measure the time to see how long 
it takes?  It's an upper bound, but maybe 10 seconds would be better?
> Signed-off-by: Jiaqing Zhao <Zhao.Jiaqing@xxxxxxx>
> @@ -3547,12 +3564,28 @@ void libxl__destroy_device_model(libxl__egc *egc,
>       LOGD(DEBUG, domid, "Didn't find dm UID; destroying by pid");
>   
>       path = GCSPRINTF("/local/domain/%d/image/device-model-pid", domid);
> -    rc = kill_device_model(gc, path);
> -
> +    rc = kill_device_model(gc, path, &dm_pid);
If you just read the pid directly here, you don't need to change 
kill_device_model().  But your approach is only a single read, so maybe 
it's better.  I'm undecided, but maybe someone else has an opinion.
>       if (rc) {
>           ACCUMULATE_RC(ddms->rc);
>           LOGD(ERROR, domid, "Killing device model pid from path %s", path);
> +        goto out;
> +    }
> +
> +    LOGD(DEBUG, domid, "Waiting for device model [%d] to exit", dm_pid);
> +    reaper_pid = libxl__ev_child_fork(gc, &ddms->destroyer,
> +                                      kill_device_model_uid_cb);
The reaper_pid handling below is duplicated from
`if (dm_kill_uid_str) {`

Put the above pid code in an else, and use a single implementation of 
the reaper_pid handling.
> +    if (reaper_pid < 0) {
> +        rc = ERROR_FAIL;
> +        ACCUMULATE_RC(ddms->rc);
> +        LOGD(ERROR, domid, "Unable to fork device model exit waiter");
> +        goto out;
>       }
> +    if (!reaper_pid) {
> +        rc = wait_device_model_pid_child(gc, domid, dm_pid);
Though this would need an additional conditional.

Thanks,
Jason

> +        assert(rc <= 0 && rc >= -125);
> +        _exit(-rc);
> +    }
> +    return;
>   
>   out:
>       /*