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

[Roger Pau Monné <roger.pau@xxxxxxx>]

On Wed, Nov 04, 2026 at 02:46:28PM +0800, 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
> 60 seconds. If waiting fails or times out, log the error and retain the
> existing domain destruction behaviour.
> 
> Signed-off-by: Jiaqing Zhao <Zhao.Jiaqing@xxxxxxx>
> ---
> Cc: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> Cc: Juergen Gross <jgross@xxxxxxxx>
> Cc: Roger Pau Monné <roger@xxxxxxxxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Penny Zheng <penny.zheng@xxxxxxx>
> ---
>  tools/libs/light/libxl_aoutils.c  |  8 ++++--
>  tools/libs/light/libxl_dm.c       | 45 ++++++++++++++++++++++++++-----
>  tools/libs/light/libxl_domain.c   |  2 +-
>  tools/libs/light/libxl_internal.h |  3 ++-
>  4 files changed, 48 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/libs/light/libxl_aoutils.c 
> b/tools/libs/light/libxl_aoutils.c
> index c2d42e7cac..977a2705a8 100644
> --- a/tools/libs/light/libxl_aoutils.c
> +++ b/tools/libs/light/libxl_aoutils.c
> @@ -628,10 +628,11 @@ void libxl__kill(libxl__gc *gc, pid_t pid, int sig, 
> const char *what)
>  
>  /* Generic function to signal (HUP) a pid stored in xenstore */
>  int libxl__kill_xs_path(libxl__gc *gc, const char *xs_path_pid,
> -                        const char *what)
> +                        const char *what, pid_t *pid_r)
>  {
>      const char *xs_pid;
> -    int ret, pid;
> +    int ret;
> +    pid_t pid;
>  
>      ret = libxl__xs_read_checked(gc, XBT_NULL, xs_path_pid, &xs_pid);
>      if (ret || !xs_pid) {
> @@ -640,6 +641,9 @@ int libxl__kill_xs_path(libxl__gc *gc, const char 
> *xs_path_pid,
>          goto out;
>      }
>      pid = atoi(xs_pid);
> +    if (pid_r) {
> +        *pid_r = pid;
> +    }
>  
>      ret = kill(pid, SIGHUP);
>      if (ret < 0 && errno == ESRCH) {
> diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
> index 511ec76a65..ac1db9fe6a 100644
> --- a/tools/libs/light/libxl_dm.c
> +++ b/tools/libs/light/libxl_dm.c
> @@ -2859,7 +2859,7 @@ static void device_model_probe_spawn_outcome(libxl__egc 
> *egc,
>  
>      /* Ensure our QEMU command line probe is killed. */
>      rc = libxl__kill_xs_path(gc, dmss->spawn.pidpath,
> -                             "qemu command-line probe");
> +                             "qemu command-line probe", NULL);
>      if (rc) {
>          LOGD(WARN, dmss->guest_domid,
>               "Killing qemu command-line probe pid from path %s",
> @@ -3420,9 +3420,10 @@ out:
>  }
>  
>  /* Generic function to signal a Qemu instance to exit */
> -static int kill_device_model(libxl__gc *gc, const char *xs_path_pid)
> +static int kill_device_model(libxl__gc *gc, const char *xs_path_pid,
> +                             pid_t *pid_r)
>  {
> -    return libxl__kill_xs_path(gc, xs_path_pid, "Device Model");
> +    return libxl__kill_xs_path(gc, xs_path_pid, "Device Model", pid_r);
>  }
>  
>  /* Helper to destroy a Qdisk backend */
> @@ -3433,7 +3434,7 @@ int libxl__destroy_qemu_xenpv_backend(libxl__gc *gc, 
> uint32_t domid)
>  
>      pid_path = GCSPRINTF("libxl/%u/qdisk-backend-pid", domid);
>  
> -    rc = kill_device_model(gc, pid_path);
> +    rc = kill_device_model(gc, pid_path, NULL);
>      if (rc)
>          goto out;
>  
> @@ -3454,6 +3455,21 @@ static void kill_device_model_uid_cb(libxl__egc *egc,
>                                       libxl__ev_child *destroyer,
>                                       pid_t pid, int status);
>  
> +/* Run in a child so waiting for QEMU does not block the libxl event loop. */
> +static int wait_device_model_pid_child(libxl__gc *gc, uint32_t domid, pid_t 
> pid)
> +{
> +    unsigned int i;
> +
> +    for (i = 0; i < LIBXL_DEVICE_MODEL_STOP_TIMEOUT * 10; i++) {
> +        if (kill(pid, 0) && errno == ESRCH)

Isn't this theoretically racy?  How can you guarantee the PID hasn't
been reused, and thus you end up killing an unexpected process on
further loops?

I see you hook this up into the events interface by spawning a process
that does the spinning, have you looked whether it's possible to get
PID state changes as notification on a file descriptor?  That would be
more natural to hook into the events interface, and would avoid the
race mentioned above AFAICT.

Thanks, Roger.