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

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

[Jiaqing Zhao]

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@amd.com>
---
Cc: Anthony PERARD <anthony.perard@vates.tech>
Cc: Juergen Gross <jgross@suse.com>
Cc: Roger Pau Monné <roger@xenproject.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Penny Zheng <penny.zheng@amd.com>
---
 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)
+            return 0;
+        usleep(100000);
+    }
+
+    LOGD(ERROR, domid, "Timed out waiting for device model [%d] to exit", pid);
+    return ERROR_TIMEDOUT;
+}
+
 /*
  * If we have a uid, we shouldn't kill by pid.  This is because a
  * hostile QEMU might have exited, in which case the pid we have may
@@ -3477,6 +3493,7 @@ void libxl__destroy_device_model(libxl__egc *egc,
     int domid = ddms->domid;
     char *path;
     const char *dm_kill_uid_str = NULL;
+    pid_t dm_pid;
     int reaper_pid;
 
     ddms->rc = 0;
@@ -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 (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);
+    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);
+        assert(rc <= 0 && rc >= -125);
+        _exit(-rc);
+    }
+    return;
 
 out:
     /*
diff --git a/tools/libs/light/libxl_domain.c b/tools/libs/light/libxl_domain.c
index 37fcd92871..6e332cfed7 100644
--- a/tools/libs/light/libxl_domain.c
+++ b/tools/libs/light/libxl_domain.c
@@ -1270,7 +1270,7 @@ static void dm_destroy_cb(libxl__egc *egc,
         libxl__stubdomain_is_linux_running(gc, target_domid)) {
         char *path = GCSPRINTF("/local/domain/%d/image/qmp-proxy-pid", domid);
 
-        libxl__kill_xs_path(gc, path, "QMP Proxy");
+        libxl__kill_xs_path(gc, path, "QMP Proxy", NULL);
         /* qmp-proxy for stubdom registers target_domid's QMP sockets. */
         libxl__qmp_cleanup(gc, target_domid);
     }
diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index b65e0064b9..f4fde8009e 100644
--- a/tools/libs/light/libxl_internal.h
+++ b/tools/libs/light/libxl_internal.h
@@ -97,6 +97,7 @@
 /* QEMU may be slow to load and start due to a bug in Linux where the I/O
  * subsystem sometime produce high latency under load. */
 #define LIBXL_DEVICE_MODEL_START_TIMEOUT 60
+#define LIBXL_DEVICE_MODEL_STOP_TIMEOUT 60
 #define LIBXL_DEVICE_MODEL_SAVE_FILE XEN_LIB_DIR "/qemu-save" /* .$domid */
 #define LIBXL_DEVICE_MODEL_RESTORE_FILE XEN_LIB_DIR "/qemu-resume" /* .$domid */
 #define LIBXL_QMP_CMD_TIMEOUT 10
@@ -2659,7 +2660,7 @@ bool libxl__async_exec_inuse(const libxl__async_exec_state *aes);
 _hidden void libxl__kill(libxl__gc *gc, pid_t pid, int sig, const char *what);
 /* kill SIGHUP a pid stored in xenstore */
 _hidden int libxl__kill_xs_path(libxl__gc *gc, const char *xs_path_pid,
-                                const char *what);
+                                const char *what, pid_t *pid_r);
 
 /*----- device addition/removal -----*/
 
-- 
2.53.0

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

[Jason Andryuk]

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@amd.com>

> @@ -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:
>       /*

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

[Jiaqing Zhao]

On 2026-11-05 01:33, Jason Andryuk wrote:
> 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?

Typically it costs less than 3s. Most time it only takes 1s. Will change
to 10s in v2.

>> Signed-off-by: Jiaqing Zhao <Zhao.Jiaqing@amd.com>
> 
>> @@ -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.

Yes keeping old kill_device_model() requires an extra libxl__xs_read_checked()
to read the pid. Let's wait for more comments.

>>       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

Sure, will do in v2.

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

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

[Jan Beulich]

On 04.11.2026 18:33, Jason Andryuk wrote:
> 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?

Is 10s enough for it to time out upon disconnecting pass-through devices?
I'm seeing this happen routinely, albeit with an older version of qemu.

Jan

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

[Jiaqing Zhao]

On 2026-11-05 16:13, Jan Beulich wrote:
> On 04.11.2026 18:33, Jason Andryuk wrote:
>> 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?
> 
> Is 10s enough for it to time out upon disconnecting pass-through devices?
> I'm seeing this happen routinely, albeit with an older version of qemu.
> 
> Jan

I checked the code, passthorugh devices are removed via QMP, with
pci_remove_qmp_query_cb(), before sending SIGHUP to terminate QEMU.

Passing through a PCI SD card controller on my system took ~1.2s from
`xl destory` to sending SIGHUP to QEMU, and QEMU then took ~50ms to exit.
So 10s is pretty enough here I think.

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

[Jan Beulich]

On 10.11.2026 10:10, Jiaqing Zhao wrote:
> On 2026-11-05 16:13, Jan Beulich wrote:
>> On 04.11.2026 18:33, Jason Andryuk wrote:
>>> 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?
>>
>> Is 10s enough for it to time out upon disconnecting pass-through devices?
>> I'm seeing this happen routinely, albeit with an older version of qemu.
> 
> I checked the code, passthorugh devices are removed via QMP, with
> pci_remove_qmp_query_cb(), before sending SIGHUP to terminate QEMU.
> 
> Passing through a PCI SD card controller on my system took ~1.2s from
> `xl destory` to sending SIGHUP to QEMU, and QEMU then took ~50ms to exit.
> So 10s is pretty enough here I think.

Yet that wasn't my question. What you describe is the situation where qemu
processes things normally. Whereas I've asked about qemu hitting some sort
of timeout in the process. I haven't looked into this in any detail (and
hence I don't know what exact timeout it is that is coming into play
there), I just know that I've been observing such quite often when shutting
down a guest with an SR-IOV NIC attached.

Jan

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

[Roger Pau Monné]

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@amd.com>
> ---
> Cc: Anthony PERARD <anthony.perard@vates.tech>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Roger Pau Monné <roger@xenproject.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Penny Zheng <penny.zheng@amd.com>
> ---
>  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.

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

[Jiaqing Zhao]

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@amd.com>
>> ---
>> Cc: Anthony PERARD <anthony.perard@vates.tech>
>> Cc: Juergen Gross <jgross@suse.com>
>> Cc: Roger Pau Monné <roger@xenproject.com>
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Penny Zheng <penny.zheng@amd.com>
>> ---
>>  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.