[PATCH 0/5] qemu: respond to NETDEV_VHOST_USER_DISCONNECTED event by starting a new passt process
This brings behavior of passt+vhostuser in line with trad-passt - if the passt process is somehow terminated, libvirt will start a new passt process that qemu will connect to. You'll notice no change in behavior if your QEMU binary doesn't have the patches that add NETDEV_VHOST_USER_DISCONNECTED to qemu, but also nothing extra will fail. The requisite QEMU patches are upstream in commit 02fd9f8aeeb1 ("net: vhost-user: add QAPI events to report connection state"), which will be in upstream qemu-10.0.0. Resolves: https://issues.redhat.com/browse/RHEL-80169 Laine Stump (5): qemu: remove nonsensical sanity check in processNetdevStreamDisconnectedEvent() qemu: make processNetDevStreamDisconnectedEvent() reusable qemu: respond to NETDEV_VHOST_USER_DISCONNECTED event qemu: put vhost-user code that's special for passt in a helper function qemu: make passt+vhostuser reconnect behave identically to passt+user src/qemu/qemu_domain.c | 1 + src/qemu/qemu_domain.h | 1 + src/qemu/qemu_driver.c | 55 ++++++++++++------- src/qemu/qemu_hotplug.c | 7 +-- src/qemu/qemu_monitor.c | 11 ++++ src/qemu/qemu_monitor.h | 6 ++ src/qemu/qemu_monitor_json.c | 16 ++++++ src/qemu/qemu_passt.c | 25 +++++++++ src/qemu/qemu_passt.h | 3 + src/qemu/qemu_process.c | 27 ++++++--- .../net-vhostuser-passt.x86_64-latest.args | 6 +- ...rder-domain-subelements.x86_64-latest.args | 2 +- 12 files changed, 122 insertions(+), 38 deletions(-) -- 2.49.0
By definition QEMU will never send a NETDEV_STREAM_DISCONNECTED event if it doesn't support the reconnect option for a stream netdev (and even if, by some comedy of errors, it did send NETDEV_STREAM_DISCONNECTED in that case, our response to the event doesn't request anything at all of QEMU (much less something that would fail if QEMU didn't understand NETDEV_STREAM_DISCONNETED) - it just starts a new passt process to replace the one that has just been terminated, so we don't need to check the QEMU capabilities for QEMU_CAPS_NETDEV_STREAM_RECONNECT. Signed-off-by: Laine Stump <laine@redhat.com> --- src/qemu/qemu_driver.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6ce949dd07..411cf79317 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3612,7 +3612,6 @@ processNetdevStreamDisconnectedEvent(virDomainObj *vm, { virDomainDeviceDef dev; virDomainNetDef *def; - virQEMUCaps *qemuCaps = QEMU_DOMAIN_PRIVATE(vm)->qemuCaps; const char *devAlias = STRSKIP(netdevId, "host"); /* The event sends us the "netdev-id", but we don't store the @@ -3658,12 +3657,6 @@ processNetdevStreamDisconnectedEvent(virDomainObj *vm, goto endjob; } - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV_STREAM_RECONNECT)) { - VIR_WARN("ignore NETDEV_STREAM_DISCONNECTED event for passt network device %s in domain %s - QEMU binary does not support reconnect", - def->info.alias, vm->def->name); - goto endjob; - } - /* handle the event - restart the passt process with its original * parameters */ -- 2.49.0
On Thu, Apr 10, 2025 at 02:58:26 -0400, Laine Stump wrote:
By definition QEMU will never send a NETDEV_STREAM_DISCONNECTED event if it doesn't support the reconnect option for a stream netdev (and
Hmm, nested parenthesis and with a closing one missing :-) I suggest just s/ (a/. A/
even if, by some comedy of errors, it did send NETDEV_STREAM_DISCONNECTED in that case, our response to the event doesn't request anything at all of QEMU (much less something that would fail if QEMU didn't understand NETDEV_STREAM_DISCONNETED) - it just starts a new passt process to replace the one that has just been terminated, so we don't need to check the QEMU capabilities for QEMU_CAPS_NETDEV_STREAM_RECONNECT.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/qemu/qemu_driver.c | 7 ------- 1 file changed, 7 deletions(-)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
We will be adding a new event whose response will be *exactly* the same as the response to NETDEV_STREAM_DISCONNECTED. Rather than doing a copy-paste of the complete function that does the processing, turn that function into something more generic that takes the name of the event as an arg (the event name is only used in log messages). Signed-off-by: Laine Stump <laine@redhat.com> --- src/qemu/qemu_driver.c | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 411cf79317..f4b4d01d6c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3607,8 +3607,9 @@ processDeviceDeletedEvent(virQEMUDriver *driver, static void -processNetdevStreamDisconnectedEvent(virDomainObj *vm, - const char *netdevId) +processNetdevDisconnectedEvent(virDomainObj *vm, + const char *netdevId, + const char *eventName) { virDomainDeviceDef dev; virDomainNetDef *def; @@ -3623,13 +3624,13 @@ processNetdevStreamDisconnectedEvent(virDomainObj *vm, */ if (!devAlias) { - VIR_WARN("Received NETDEV_STREAM_DISCONNECTED event for unrecognized netdev %s from domain %p %s", - netdevId, vm, vm->def->name); + VIR_WARN("Received %s event for unrecognized netdev %s from domain %p %s", + eventName, netdevId, vm, vm->def->name); return; } - VIR_DEBUG("Received NETDEV_STREAM_DISCONNECTED event for device %s from domain %p %s", - devAlias, vm, vm->def->name); + VIR_DEBUG("Received %s event for device %s from domain %p %s", + eventName, devAlias, vm, vm->def->name); if (virDomainObjBeginJob(vm, VIR_JOB_QUERY) < 0) return; @@ -3640,28 +3641,28 @@ processNetdevStreamDisconnectedEvent(virDomainObj *vm, } if (virDomainDefFindDevice(vm->def, devAlias, &dev, true) < 0) { - VIR_WARN("NETDEV_STREAM_DISCONNECTED event received for non-existent device %s in domain %s", - devAlias, vm->def->name); + VIR_WARN("%s event received for non-existent device %s in domain %s", + eventName, devAlias, vm->def->name); goto endjob; } if (dev.type != VIR_DOMAIN_DEVICE_NET) { - VIR_WARN("NETDEV_STREAM_DISCONNECTED event received for non-network device %s in domain %s", - devAlias, vm->def->name); + VIR_WARN("%s event received for non-network device %s in domain %s", + eventName, devAlias, vm->def->name); goto endjob; } def = dev.data.net; if (def->backend.type != VIR_DOMAIN_NET_BACKEND_PASST) { - VIR_DEBUG("ignore NETDEV_STREAM_DISCONNECTED event for non-passt network device %s in domain %s", - def->info.alias, vm->def->name); + VIR_DEBUG("ignore %s event for non-passt network device %s in domain %s", + eventName, def->info.alias, vm->def->name); goto endjob; } /* handle the event - restart the passt process with its original * parameters */ - VIR_DEBUG("process NETDEV_STREAM_DISCONNECTED event for network device %s in domain %s", - def->info.alias, vm->def->name); + VIR_DEBUG("process %s event for network device %s in domain %s", + eventName, def->info.alias, vm->def->name); if (qemuPasstStart(vm, def) < 0) goto endjob; @@ -3671,6 +3672,14 @@ processNetdevStreamDisconnectedEvent(virDomainObj *vm, } +static void +processNetdevStreamDisconnectedEvent(virDomainObj *vm, + const char *netdevId) +{ + processNetdevDisconnectedEvent(vm, netdevId, "NETDEV_STREAM_DISCONNECTED"); +} + + static void processNicRxFilterChangedEvent(virQEMUDriver *driver, virDomainObj *vm, -- 2.49.0
On Thu, Apr 10, 2025 at 02:58:27 -0400, Laine Stump wrote:
We will be adding a new event whose response will be *exactly* the same as the response to NETDEV_STREAM_DISCONNECTED. Rather than doing a copy-paste of the complete function that does the processing, turn that function into something more generic that takes the name of the event as an arg (the event name is only used in log messages).
Signed-off-by: Laine Stump <laine@redhat.com> --- src/qemu/qemu_driver.c | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
This response to this event is identical to NETDEV_STREAM_DISCONNECTED (start a new passt process to replace the one that just disappeared - see commitf62ce81b8a5), except that the new passt process will have "--vhost-user" appended to the commandline. Fortunately that difference is already handled based on the virDomainNetDef contents, so we can, in fact, respond to the new event in exactly the same manner. Signed-off-by: Laine Stump <laine@redhat.com> --- src/qemu/qemu_domain.c | 1 + src/qemu/qemu_domain.h | 1 + src/qemu/qemu_driver.c | 11 +++++++++++ src/qemu/qemu_monitor.c | 11 +++++++++++ src/qemu/qemu_monitor.h | 6 ++++++ src/qemu/qemu_monitor_json.c | 16 ++++++++++++++++ src/qemu/qemu_process.c | 18 ++++++++++++++++++ 7 files changed, 64 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c3ca4b3040..d4bcc581b0 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -10087,6 +10087,7 @@ qemuProcessEventFree(struct qemuProcessEvent *event) case QEMU_PROCESS_EVENT_WATCHDOG: case QEMU_PROCESS_EVENT_DEVICE_DELETED: case QEMU_PROCESS_EVENT_NETDEV_STREAM_DISCONNECTED: + case QEMU_PROCESS_EVENT_NETDEV_VHOST_USER_DISCONNECTED: case QEMU_PROCESS_EVENT_NIC_RX_FILTER_CHANGED: case QEMU_PROCESS_EVENT_SERIAL_CHANGED: case QEMU_PROCESS_EVENT_GUEST_CRASHLOADED: diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 70e1fb187f..787f1464aa 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -469,6 +469,7 @@ typedef enum { QEMU_PROCESS_EVENT_GUESTPANIC, QEMU_PROCESS_EVENT_DEVICE_DELETED, QEMU_PROCESS_EVENT_NETDEV_STREAM_DISCONNECTED, + QEMU_PROCESS_EVENT_NETDEV_VHOST_USER_DISCONNECTED, QEMU_PROCESS_EVENT_NIC_RX_FILTER_CHANGED, QEMU_PROCESS_EVENT_SERIAL_CHANGED, QEMU_PROCESS_EVENT_JOB_STATUS_CHANGE, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f4b4d01d6c..ef3f1d3763 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3680,6 +3680,14 @@ processNetdevStreamDisconnectedEvent(virDomainObj *vm, } +static void +processNetdevVhostUserDisconnectedEvent(virDomainObj *vm, + const char *netdevId) +{ + processNetdevDisconnectedEvent(vm, netdevId, "NETDEV_VHOST_USER_DISCONNECTED"); +} + + static void processNicRxFilterChangedEvent(virQEMUDriver *driver, virDomainObj *vm, @@ -4078,6 +4086,9 @@ static void qemuProcessEventHandler(void *data, void *opaque) case QEMU_PROCESS_EVENT_NETDEV_STREAM_DISCONNECTED: processNetdevStreamDisconnectedEvent(vm, processEvent->data); break; + case QEMU_PROCESS_EVENT_NETDEV_VHOST_USER_DISCONNECTED: + processNetdevVhostUserDisconnectedEvent(vm, processEvent->data); + break; case QEMU_PROCESS_EVENT_NIC_RX_FILTER_CHANGED: processNicRxFilterChangedEvent(driver, vm, processEvent->data); break; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 8262e3a8b2..981975cdd2 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1264,6 +1264,17 @@ qemuMonitorEmitNetdevStreamDisconnected(qemuMonitor *mon, } +void +qemuMonitorEmitNetdevVhostUserDisconnected(qemuMonitor *mon, + const char *devAlias) +{ + VIR_DEBUG("mon=%p", mon); + + QEMU_MONITOR_CALLBACK(mon, domainNetdevVhostUserDisconnected, + mon->vm, devAlias); +} + + void qemuMonitorEmitSerialChange(qemuMonitor *mon, const char *devAlias, diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 4acf50a166..8d49ada114 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -255,6 +255,9 @@ typedef void (*qemuMonitorDomainDeviceUnplugErrCallback)(qemuMonitor *mon, typedef void (*qemuMonitorDomainNetdevStreamDisconnectedCallback)(qemuMonitor *mon, virDomainObj *vm, const char *devAlias); +typedef void (*qemuMonitorDomainNetdevVhostUserDisconnectedCallback)(qemuMonitor *mon, + virDomainObj *vm, + const char *devAlias); typedef void (*qemuMonitorDomainNicRxFilterChangedCallback)(qemuMonitor *mon, virDomainObj *vm, const char *devAlias); @@ -403,6 +406,7 @@ struct _qemuMonitorCallbacks { qemuMonitorDomainMemoryDeviceSizeChange domainMemoryDeviceSizeChange; qemuMonitorDomainDeviceUnplugErrCallback domainDeviceUnplugError; qemuMonitorDomainNetdevStreamDisconnectedCallback domainNetdevStreamDisconnected; + qemuMonitorDomainNetdevVhostUserDisconnectedCallback domainNetdevVhostUserDisconnected; }; qemuMonitor *qemuMonitorOpen(virDomainObj *vm, @@ -490,6 +494,8 @@ void qemuMonitorEmitDeviceUnplugErr(qemuMonitor *mon, const char *devAlias); void qemuMonitorEmitNetdevStreamDisconnected(qemuMonitor *mon, const char *devAlias); +void qemuMonitorEmitNetdevVhostUserDisconnected(qemuMonitor *mon, + const char *devAlias); void qemuMonitorEmitNicRxFilterChanged(qemuMonitor *mon, const char *devAlias); void qemuMonitorEmitSerialChange(qemuMonitor *mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 6fa301577d..dc2eaace96 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -85,6 +85,7 @@ static void qemuMonitorJSONHandleMemoryFailure(qemuMonitor *mon, virJSONValue *d static void qemuMonitorJSONHandleMemoryDeviceSizeChange(qemuMonitor *mon, virJSONValue *data); static void qemuMonitorJSONHandleDeviceUnplugErr(qemuMonitor *mon, virJSONValue *data); static void qemuMonitorJSONHandleNetdevStreamDisconnected(qemuMonitor *mon, virJSONValue *data); +static void qemuMonitorJSONHandleNetdevVhostUserDisconnected(qemuMonitor *mon, virJSONValue *data); typedef struct { const char *type; @@ -108,6 +109,7 @@ static qemuEventHandler eventHandlers[] = { { "MIGRATION", qemuMonitorJSONHandleMigrationStatus, }, { "MIGRATION_PASS", qemuMonitorJSONHandleMigrationPass, }, { "NETDEV_STREAM_DISCONNECTED", qemuMonitorJSONHandleNetdevStreamDisconnected, }, + { "NETDEV_VHOST_USER_DISCONNECTED", qemuMonitorJSONHandleNetdevVhostUserDisconnected, }, { "NIC_RX_FILTER_CHANGED", qemuMonitorJSONHandleNicRxFilterChanged, }, { "PR_MANAGER_STATUS_CHANGED", qemuMonitorJSONHandlePRManagerStatusChanged, }, { "RDMA_GID_STATUS_CHANGED", qemuMonitorJSONHandleRdmaGidStatusChanged, }, @@ -1044,6 +1046,20 @@ qemuMonitorJSONHandleNetdevStreamDisconnected(qemuMonitor *mon, virJSONValue *da } +static void +qemuMonitorJSONHandleNetdevVhostUserDisconnected(qemuMonitor *mon, virJSONValue *data) +{ + const char *name; + + if (!(name = virJSONValueObjectGetString(data, "netdev-id"))) { + VIR_WARN("missing device in NETDEV_VHOST_USER_DISCONNECTED event"); + return; + } + + qemuMonitorEmitNetdevVhostUserDisconnected(mon, name); +} + + static void qemuMonitorJSONHandleNicRxFilterChanged(qemuMonitor *mon, virJSONValue *data) { diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 015a98d035..ae5349fd30 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1402,6 +1402,23 @@ qemuProcessHandleNetdevStreamDisconnected(qemuMonitor *mon G_GNUC_UNUSED, } +static void +qemuProcessHandleNetdevVhostUserDisconnected(qemuMonitor *mon G_GNUC_UNUSED, + virDomainObj *vm, + const char *devAlias) +{ + virObjectLock(vm); + + VIR_DEBUG("Device %s Netdev vhost-user Disconnected in domain %p %s", + devAlias, vm, vm->def->name); + + qemuProcessEventSubmit(vm, QEMU_PROCESS_EVENT_NETDEV_VHOST_USER_DISCONNECTED, + 0, 0, g_strdup(devAlias)); + + virObjectUnlock(vm); +} + + static void qemuProcessHandleNicRxFilterChanged(qemuMonitor *mon G_GNUC_UNUSED, virDomainObj *vm, @@ -1848,6 +1865,7 @@ static qemuMonitorCallbacks monitorCallbacks = { .domainMemoryDeviceSizeChange = qemuProcessHandleMemoryDeviceSizeChange, .domainDeviceUnplugError = qemuProcessHandleDeviceUnplugErr, .domainNetdevStreamDisconnected = qemuProcessHandleNetdevStreamDisconnected, + .domainNetdevVhostUserDisconnected = qemuProcessHandleNetdevVhostUserDisconnected, }; static void -- 2.49.0
On Thu, Apr 10, 2025 at 02:58:28 -0400, Laine Stump wrote:
This response to this event is identical to NETDEV_STREAM_DISCONNECTED (start a new passt process to replace the one that just disappeared - see commitf62ce81b8a5), except that the new passt process will have "--vhost-user" appended to the commandline. Fortunately that difference is already handled based on the virDomainNetDef contents, so we can, in fact, respond to the new event in exactly the same manner.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/qemu/qemu_domain.c | 1 + src/qemu/qemu_domain.h | 1 + src/qemu/qemu_driver.c | 11 +++++++++++ src/qemu/qemu_monitor.c | 11 +++++++++++ src/qemu/qemu_monitor.h | 6 ++++++ src/qemu/qemu_monitor_json.c | 16 ++++++++++++++++ src/qemu/qemu_process.c | 18 ++++++++++++++++++ 7 files changed, 64 insertions(+)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Rather than duplicating these two lines of chr device object setup for hotplug and domain start, put them in a helper function that's called from both places. That way when we need to setup *more* stuff specific to passt+vhostuser, we can just add it in that one place. Signed-off-by: Laine Stump <laine@redhat.com> --- src/qemu/qemu_hotplug.c | 7 +------ src/qemu/qemu_passt.c | 19 +++++++++++++++++++ src/qemu/qemu_passt.h | 3 +++ src/qemu/qemu_process.c | 9 ++------- 4 files changed, 25 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 9977662a2c..4562a75d2a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1268,12 +1268,7 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver, if (net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST) { - /* vhostuser needs socket path in this location, and when - * backend is passt, the path is derived from other info, - * not taken from config. - */ - g_free(net->data.vhostuser->data.nix.path); - net->data.vhostuser->data.nix.path = qemuPasstCreateSocketPath(vm, net); + qemuPasstPrepareVhostUser(vm, net); if (qemuPasstStart(vm, net) < 0) goto cleanup; diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c index b9616d1c63..bc495eca1e 100644 --- a/src/qemu/qemu_passt.c +++ b/src/qemu/qemu_passt.c @@ -165,6 +165,25 @@ qemuPasstSetupCgroup(virDomainObj *vm, } +void +qemuPasstPrepareVhostUser(virDomainObj *vm, + virDomainNetDef *net) +{ + /* There are some options on the QEMU commandline for a vhost-user + * chr device that are normally configurable, but when it is passt + * speaking to the vhost-user device those things are + * derived/fixed. This function, which is called prior to + * generating the QEMU commandline, sets thos derived/fixed things + * in the chr device object. + */ + + /* The socket path is not user-configurable for passt - it is + * derived from other info + */ + g_free(net->data.vhostuser->data.nix.path); + net->data.vhostuser->data.nix.path = qemuPasstCreateSocketPath(vm, net); +} + int qemuPasstStart(virDomainObj *vm, virDomainNetDef *net) diff --git a/src/qemu/qemu_passt.h b/src/qemu/qemu_passt.h index e0b9aaac8d..ea545ccf38 100644 --- a/src/qemu/qemu_passt.h +++ b/src/qemu/qemu_passt.h @@ -37,5 +37,8 @@ int qemuPasstSetupCgroup(virDomainObj *vm, virDomainNetDef *net, virCgroup *cgroup); +void qemuPasstPrepareVhostUser(virDomainObj *vm, + virDomainNetDef *net); + char *qemuPasstCreateSocketPath(virDomainObj *vm, virDomainNetDef *net); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ae5349fd30..ba54da62ba 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6022,13 +6022,8 @@ qemuProcessPrepareDomainNetwork(virDomainObj *vm) case VIR_DOMAIN_NET_TYPE_VHOSTUSER: if (net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST) { - /* when using the passt backend, the path of the - * unix socket is always derived from other info - * *not* manually given in the config, but all the - * vhostuser code looks for it there. - */ - g_free(net->data.vhostuser->data.nix.path); - net->data.vhostuser->data.nix.path = qemuPasstCreateSocketPath(vm, net); + /* some extra setup of internal data for passt vhostuser mode */ + qemuPasstPrepareVhostUser(vm, net); } break; -- 2.49.0
On Thu, Apr 10, 2025 at 02:58:29 -0400, Laine Stump wrote:
Rather than duplicating these two lines of chr device object setup for hotplug and domain start, put them in a helper function that's called from both places. That way when we need to setup *more* stuff specific to passt+vhostuser, we can just add it in that one place.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/qemu/qemu_hotplug.c | 7 +------ src/qemu/qemu_passt.c | 19 +++++++++++++++++++ src/qemu/qemu_passt.h | 3 +++ src/qemu/qemu_process.c | 9 ++------- 4 files changed, 25 insertions(+), 13 deletions(-)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
When "original passt" support was added, we decided that we always wanted to reconnect (i.e. restart the passt process) if it was somehow terminated. Generic vhost-user only turns on reconnect if specified in the config, but there is no reason to require this if the other end of the vhost-user socket is a passt process - we know what has happened and what we want to do; no reason to make the default configuration "do the *wrong* thing". Resolves: https://issues.redhat.com/browse/RHEL-80169 Signed-off-by: Laine Stump <laine@redhat.com> --- src/qemu/qemu_passt.c | 6 ++++++ .../qemuxmlconfdata/net-vhostuser-passt.x86_64-latest.args | 6 +++--- .../schema-reorder-domain-subelements.x86_64-latest.args | 2 +- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c index bc495eca1e..018630a5de 100644 --- a/src/qemu/qemu_passt.c +++ b/src/qemu/qemu_passt.c @@ -182,6 +182,12 @@ qemuPasstPrepareVhostUser(virDomainObj *vm, */ g_free(net->data.vhostuser->data.nix.path); net->data.vhostuser->data.nix.path = qemuPasstCreateSocketPath(vm, net); + + /* reconnect is always enabled, with timeout always at 5 seconds, when + * using passt + */ + net->data.vhostuser->data.nix.reconnect.enabled = VIR_TRISTATE_BOOL_YES; + net->data.vhostuser->data.nix.reconnect.timeout = 5; } int diff --git a/tests/qemuxmlconfdata/net-vhostuser-passt.x86_64-latest.args b/tests/qemuxmlconfdata/net-vhostuser-passt.x86_64-latest.args index 7c030d7067..afbbe188cf 100644 --- a/tests/qemuxmlconfdata/net-vhostuser-passt.x86_64-latest.args +++ b/tests/qemuxmlconfdata/net-vhostuser-passt.x86_64-latest.args @@ -28,13 +28,13 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \ -boot strict=on \ -blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1","node-name":"libvirt-1-storage","read-only":false}' \ -device '{"driver":"ide-hd","bus":"ide.0","unit":0,"drive":"libvirt-1-storage","id":"ide0-0-0","bootindex":1}' \ --chardev socket,id=charnet0,path=/var/run/libvirt/qemu/passt/-1-QEMUGuest1-net0.socket \ +-chardev socket,id=charnet0,path=/var/run/libvirt/qemu/passt/-1-QEMUGuest1-net0.socket,reconnect-ms=5000 \ -netdev '{"type":"vhost-user","chardev":"charnet0","id":"hostnet0"}' \ -device '{"driver":"virtio-net-pci","netdev":"hostnet0","id":"net0","mac":"00:11:22:33:44:55","bus":"pci.0","addr":"0x2"}' \ --chardev socket,id=charnet1,path=/var/run/libvirt/qemu/passt/-1-QEMUGuest1-net1.socket \ +-chardev socket,id=charnet1,path=/var/run/libvirt/qemu/passt/-1-QEMUGuest1-net1.socket,reconnect-ms=5000 \ -netdev '{"type":"vhost-user","chardev":"charnet1","id":"hostnet1"}' \ -device '{"driver":"virtio-net-pci","netdev":"hostnet1","id":"net1","mac":"00:11:22:33:44:11","bus":"pci.0","addr":"0x3"}' \ --chardev socket,id=charnet2,path=/var/run/libvirt/qemu/passt/-1-QEMUGuest1-net2.socket \ +-chardev socket,id=charnet2,path=/var/run/libvirt/qemu/passt/-1-QEMUGuest1-net2.socket,reconnect-ms=5000 \ -netdev '{"type":"vhost-user","chardev":"charnet2","id":"hostnet2"}' \ -device '{"driver":"virtio-net-pci","netdev":"hostnet2","id":"net2","mac":"00:11:22:33:44:11","bus":"pci.0","addr":"0x4"}' \ -audiodev '{"id":"audio1","driver":"none"}' \ diff --git a/tests/qemuxmlconfdata/schema-reorder-domain-subelements.x86_64-latest.args b/tests/qemuxmlconfdata/schema-reorder-domain-subelements.x86_64-latest.args index d27dd77d9f..76df9c30b0 100644 --- a/tests/qemuxmlconfdata/schema-reorder-domain-subelements.x86_64-latest.args +++ b/tests/qemuxmlconfdata/schema-reorder-domain-subelements.x86_64-latest.args @@ -29,7 +29,7 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-passtvhostuu/.config \ -device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \ -blockdev '{"driver":"file","filename":"/home/laine/libvirt/images/fedora-34.img","node-name":"libvirt-1-storage","read-only":false}' \ -device '{"driver":"virtio-blk-pci","bus":"pci.0","multifunction":true,"addr":"0x3","drive":"libvirt-1-storage","id":"virtio-disk0","bootindex":1}' \ --chardev socket,id=charnet0,path=/var/run/libvirt/qemu/passt/-1-passtvhostuu-net0.socket \ +-chardev socket,id=charnet0,path=/var/run/libvirt/qemu/passt/-1-passtvhostuu-net0.socket,reconnect-ms=5000 \ -netdev '{"type":"vhost-user","chardev":"charnet0","id":"hostnet0"}' \ -device '{"driver":"virtio-net-pci","netdev":"hostnet0","id":"net0","mac":"52:54:00:21:de:6c","bus":"pci.0","addr":"0x2"}' \ -chardev pty,id=charserial0 \ -- 2.49.0
On Thu, Apr 10, 2025 at 02:58:30 -0400, Laine Stump wrote:
When "original passt" support was added, we decided that we always wanted to reconnect (i.e. restart the passt process) if it was somehow terminated. Generic vhost-user only turns on reconnect if specified in the config, but there is no reason to require this if the other end of the vhost-user socket is a passt process - we know what has happened and what we want to do; no reason to make the default configuration "do the *wrong* thing".
Resolves: https://issues.redhat.com/browse/RHEL-80169
Signed-off-by: Laine Stump <laine@redhat.com> --- src/qemu/qemu_passt.c | 6 ++++++ .../qemuxmlconfdata/net-vhostuser-passt.x86_64-latest.args | 6 +++--- .../schema-reorder-domain-subelements.x86_64-latest.args | 2 +- 3 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c index bc495eca1e..018630a5de 100644 --- a/src/qemu/qemu_passt.c +++ b/src/qemu/qemu_passt.c @@ -182,6 +182,12 @@ qemuPasstPrepareVhostUser(virDomainObj *vm, */ g_free(net->data.vhostuser->data.nix.path); net->data.vhostuser->data.nix.path = qemuPasstCreateSocketPath(vm, net); + + /* reconnect is always enabled, with timeout always at 5 seconds, when + * using passt + */ + net->data.vhostuser->data.nix.reconnect.enabled = VIR_TRISTATE_BOOL_YES; + net->data.vhostuser->data.nix.reconnect.timeout = 5;
Hmm, shouldn't this be reflected in the XML? That is, I would expect a post-parse function to set these defaults. And the current code overrides user specified settings (either disabling or setting a different timeout), which doesn't is not right. Actually, looking at our documentation the <reconnect> it seems we don't even support setting reconnect for vhost-user with passt backend. If that's the case, the code is fine as is except for the following nit, which I'd like to see addressed anyway... And I suggest relacing the 5 seconds timeout with a macro that would also be used in qemuPasstAddNetProps. Jirka
On 4/10/25 2:55 AM, Jiri Denemark wrote:
On Thu, Apr 10, 2025 at 02:58:30 -0400, Laine Stump wrote:
When "original passt" support was added, we decided that we always wanted to reconnect (i.e. restart the passt process) if it was somehow terminated. Generic vhost-user only turns on reconnect if specified in the config, but there is no reason to require this if the other end of the vhost-user socket is a passt process - we know what has happened and what we want to do; no reason to make the default configuration "do the *wrong* thing".
Resolves: https://issues.redhat.com/browse/RHEL-80169
Signed-off-by: Laine Stump <laine@redhat.com> --- src/qemu/qemu_passt.c | 6 ++++++ .../qemuxmlconfdata/net-vhostuser-passt.x86_64-latest.args | 6 +++--- .../schema-reorder-domain-subelements.x86_64-latest.args | 2 +- 3 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c index bc495eca1e..018630a5de 100644 --- a/src/qemu/qemu_passt.c +++ b/src/qemu/qemu_passt.c @@ -182,6 +182,12 @@ qemuPasstPrepareVhostUser(virDomainObj *vm, */ g_free(net->data.vhostuser->data.nix.path); net->data.vhostuser->data.nix.path = qemuPasstCreateSocketPath(vm, net); + + /* reconnect is always enabled, with timeout always at 5 seconds, when + * using passt + */ + net->data.vhostuser->data.nix.reconnect.enabled = VIR_TRISTATE_BOOL_YES; + net->data.vhostuser->data.nix.reconnect.timeout = 5;
Hmm, shouldn't this be reflected in the XML?
Nope. Actually I thought about this when giving the commit messages a final look before I sent last night/morning - just as with the socket path for the associated character device is hardcoded when vhostuser is being used for passt, the reconnect timeout for the character device is hardcoded, so it's not just a "default", it is the only possible value. I'll reflect that in the commit log. The entire net->data.vhostuser part of the object isn't configurable when vhostuser is being used for passt, the corresponding attributes in <source> aren't allowed in the input XML, and won't be output when the object is formatted.
That is, I would expect a post-parse function to set these defaults.
And the current code overrides user specified settings (either disabling or setting a different timeout), which doesn't is not right.
Except that the user can't set it :-)
Actually, looking at our documentation the <reconnect> it seems we don't even support setting reconnect for vhost-user with passt backend.
Correct. This is the same behavior as for non-vhostuser passt, where we decided that there was no downside to having reconnect always enabled and no reasonable way to explain how or why a user should modify the timeout, so made it always on and always 5 second timeout.
If that's the case, the code is fine as is except for the following nit, which I'd like to see addressed anyway...
And I suggest relacing the 5 seconds timeout with a macro that would also be used in qemuPasstAddNetProps.
Sure, I can do that. Thanks for the quick review!
On Thu, Apr 10, 2025 at 11:45:15 -0400, Laine Stump wrote:
On 4/10/25 2:55 AM, Jiri Denemark wrote:
On Thu, Apr 10, 2025 at 02:58:30 -0400, Laine Stump wrote:
When "original passt" support was added, we decided that we always wanted to reconnect (i.e. restart the passt process) if it was somehow terminated. Generic vhost-user only turns on reconnect if specified in the config, but there is no reason to require this if the other end of the vhost-user socket is a passt process - we know what has happened and what we want to do; no reason to make the default configuration "do the *wrong* thing".
Resolves: https://issues.redhat.com/browse/RHEL-80169
Signed-off-by: Laine Stump <laine@redhat.com> --- src/qemu/qemu_passt.c | 6 ++++++ .../qemuxmlconfdata/net-vhostuser-passt.x86_64-latest.args | 6 +++--- .../schema-reorder-domain-subelements.x86_64-latest.args | 2 +- 3 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c index bc495eca1e..018630a5de 100644 --- a/src/qemu/qemu_passt.c +++ b/src/qemu/qemu_passt.c @@ -182,6 +182,12 @@ qemuPasstPrepareVhostUser(virDomainObj *vm, */ g_free(net->data.vhostuser->data.nix.path); net->data.vhostuser->data.nix.path = qemuPasstCreateSocketPath(vm, net); + + /* reconnect is always enabled, with timeout always at 5 seconds, when + * using passt + */ + net->data.vhostuser->data.nix.reconnect.enabled = VIR_TRISTATE_BOOL_YES; + net->data.vhostuser->data.nix.reconnect.timeout = 5; ... Actually, looking at our documentation the <reconnect> it seems we don't even support setting reconnect for vhost-user with passt backend.
Correct. This is the same behavior as for non-vhostuser passt, where we decided that there was no downside to having reconnect always enabled and no reasonable way to explain how or why a user should modify the timeout, so made it always on and always 5 second timeout.
Great.
If that's the case, the code is fine as is except for the following nit, which I'd like to see addressed anyway...
And I suggest relacing the 5 seconds timeout with a macro that would also be used in qemuPasstAddNetProps.
Sure, I can do that.
Thanks for the quick review!
With this small change Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
On 4/10/25 08:58, Laine Stump via Devel wrote:
This brings behavior of passt+vhostuser in line with trad-passt - if the passt process is somehow terminated, libvirt will start a new passt process that qemu will connect to.
You'll notice no change in behavior if your QEMU binary doesn't have the patches that add NETDEV_VHOST_USER_DISCONNECTED to qemu, but also nothing extra will fail. The requisite QEMU patches are upstream in commit 02fd9f8aeeb1 ("net: vhost-user: add QAPI events to report connection state"), which will be in upstream qemu-10.0.0.
Resolves: https://issues.redhat.com/browse/RHEL-80169
Laine Stump (5): qemu: remove nonsensical sanity check in processNetdevStreamDisconnectedEvent() qemu: make processNetDevStreamDisconnectedEvent() reusable qemu: respond to NETDEV_VHOST_USER_DISCONNECTED event qemu: put vhost-user code that's special for passt in a helper function qemu: make passt+vhostuser reconnect behave identically to passt+user
src/qemu/qemu_domain.c | 1 + src/qemu/qemu_domain.h | 1 + src/qemu/qemu_driver.c | 55 ++++++++++++------- src/qemu/qemu_hotplug.c | 7 +-- src/qemu/qemu_monitor.c | 11 ++++ src/qemu/qemu_monitor.h | 6 ++ src/qemu/qemu_monitor_json.c | 16 ++++++ src/qemu/qemu_passt.c | 25 +++++++++ src/qemu/qemu_passt.h | 3 + src/qemu/qemu_process.c | 27 ++++++--- .../net-vhostuser-passt.x86_64-latest.args | 6 +- ...rder-domain-subelements.x86_64-latest.args | 2 +- 12 files changed, 122 insertions(+), 38 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (4)
-
Jiri Denemark -
Jiří Denemark
-
Laine Stump -
Michal Prívozník