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

[Jiaqing Zhao <Zhao.Jiaqing@xxxxxxx>]

On 2026-11-06 17:17, Roger Pau Monné wrote:
> 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.

This destruction path already identifies QEMU by PID read from xenstore
before sending SIGHUP, and SIGHUP is only sent once. The wait loop only
checks if the process exists without sending any signal by kill(pid, 0).

PID reuse is possible, although I would expect it to be very unlikely to
happen. Even it happens, it could therefore cause xl to wait for an
unrelated process until timeout, but would not cause the loop to kill it.

I agree that fd-based exit notifications would be cleaner. However, pidfd
is only available on Linux, FreeBSD compatibility need to be considered.
For this patch, I would prefer to retain the existing PID-based approach
and treat the wait as best effort, with notification considered separately.