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

[Jiaqing Zhao <Zhao.Jiaqing@xxxxxxx>]

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

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