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

[Jiaqing Zhao <Zhao.Jiaqing@amd.com>]

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