qemu_monitor was already returning -1 and setting errno to EINVAL on any attempt to send an fd without a unix socket, but this was a silent failure in the case of qemuDomainAttachHostPciDevice. Meanwhile, qemuDomainAttachNetDevice was doing some sanity checking for a better error message; it's better to consolidate that to a central point in the API. * src/qemu/qemu_hotplug.c (qemuDomainAttachNetDevice): Move sanity checking... * src/qemu/qemu_monitor.c (qemuMonitorSendFileHandle): ...into central location. Suggested by Chris Wright. --- v2: new patch Note that this changes the behavior of qemuDomainAttachHostPciDevice even though that function is not directly patched, but hopefully for the better. src/qemu/qemu_hotplug.c | 16 ---------------- src/qemu/qemu_monitor.c | 13 +++++++++++++ 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e4ba526..20d94e4 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -568,28 +568,12 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE || net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { - if (priv->monConfig->type != VIR_DOMAIN_CHR_TYPE_UNIX) { - qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("network device type '%s' cannot be attached: " - "qemu is not using a unix socket monitor"), - virDomainNetTypeToString(net->type)); - return -1; - } - if ((tapfd = qemuNetworkIfaceConnect(vm->def, conn, driver, net, qemuCaps)) < 0) return -1; if (qemuOpenVhostNet(vm->def, net, qemuCaps, &vhostfd) < 0) goto cleanup; } else if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { - if (priv->monConfig->type != VIR_DOMAIN_CHR_TYPE_UNIX) { - qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("network device type '%s' cannot be attached: " - "qemu is not using a unix socket monitor"), - virDomainNetTypeToString(net->type)); - return -1; - } - if ((tapfd = qemuPhysIfaceConnect(vm->def, conn, driver, net, qemuCaps, VIR_VM_OP_CREATE)) < 0) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index da38096..fb875fc 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1776,6 +1776,19 @@ int qemuMonitorSendFileHandle(qemuMonitorPtr mon, return -1; } + if (fd < 0) { + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("fd must be valid")); + return -1; + } + + if (!mon->hasSendFD) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("qemu is not using a unix socket monitor, " + "cannot send fd %s"), fdname); + return -1; + } + if (mon->json) ret = qemuMonitorJSONSendFileHandle(mon, fdname, fd); else

This is also a bug fix - on the error path, qemu_hotplug would leave the configfd file leaked into qemu. At least the next attempt to hotplug a PCI device would reuse the same fdname, and when the qemu getfd monitor command gets a new fd by the same name as an earlier one, it closes the earlier one, so there is no risk of qemu running out of fds. * src/qemu/qemu_monitor.h (qemuMonitorAddDeviceWithFd): New prototype. * src/qemu/qemu_monitor.c (qemuMonitorAddDevice): Move guts... (qemuMonitorAddDeviceWithFd): ...to new function, and add support for fd passing. * src/qemu/qemu_hotplug.c (qemuDomainAttachHostPciDevice): Use it to simplify code. --- v2: simplify, due to error handling improvement in patch 1 src/qemu/qemu_hotplug.c | 11 ++--------- src/qemu/qemu_monitor.c | 24 +++++++++++++++++++++--- src/qemu/qemu_monitor.h | 5 +++++ 3 files changed, 28 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 20d94e4..87fbb9a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -820,14 +820,6 @@ int qemuDomainAttachHostPciDevice(struct qemud_driver *driver, virReportOOMError(); goto error; } - - qemuDomainObjEnterMonitorWithDriver(driver, vm); - if (qemuMonitorSendFileHandle(priv->mon, configfd_name, - configfd) < 0) { - qemuDomainObjExitMonitorWithDriver(driver, vm); - goto error; - } - qemuDomainObjExitMonitorWithDriver(driver, vm); } } @@ -842,7 +834,8 @@ int qemuDomainAttachHostPciDevice(struct qemud_driver *driver, goto error; qemuDomainObjEnterMonitorWithDriver(driver, vm); - ret = qemuMonitorAddDevice(priv->mon, devstr); + ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr, + configfd, configfd_name); qemuDomainObjExitMonitorWithDriver(driver, vm); } else { virDomainDevicePCIAddress guestAddr; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index fb875fc..074b0b2 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2029,10 +2029,13 @@ int qemuMonitorDelDevice(qemuMonitorPtr mon, } -int qemuMonitorAddDevice(qemuMonitorPtr mon, - const char *devicestr) +int qemuMonitorAddDeviceWithFd(qemuMonitorPtr mon, + const char *devicestr, + int fd, + const char *fdname) { - VIR_DEBUG("mon=%p device=%s", mon, devicestr); + VIR_DEBUG("mon=%p device=%s fd=%d fdname=%s", mon, devicestr, fd, + NULLSTR(fdname)); int ret; if (!mon) { @@ -2041,13 +2044,28 @@ int qemuMonitorAddDevice(qemuMonitorPtr mon, return -1; } + if (fd >= 0 && qemuMonitorSendFileHandle(mon, fdname, fd) < 0) + return -1; + if (mon->json) ret = qemuMonitorJSONAddDevice(mon, devicestr); else ret = qemuMonitorTextAddDevice(mon, devicestr); + + if (ret < 0 && fd >= 0) { + if (qemuMonitorCloseFileHandle(mon, fdname) < 0) + VIR_WARN("failed to close device handle '%s'", fdname); + } + return ret; } +int qemuMonitorAddDevice(qemuMonitorPtr mon, + const char *devicestr) +{ + return qemuMonitorAddDeviceWithFd(mon, devicestr, -1, NULL); +} + int qemuMonitorAddDrive(qemuMonitorPtr mon, const char *drivestr) { diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 7bea083..a20ff8e 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -390,6 +390,11 @@ int qemuMonitorGetAllPCIAddresses(qemuMonitorPtr mon, int qemuMonitorAddDevice(qemuMonitorPtr mon, const char *devicestr); +int qemuMonitorAddDeviceWithFd(qemuMonitorPtr mon, + const char *devicestr, + int fd, + const char *fdname); + int qemuMonitorDelDevice(qemuMonitorPtr mon, const char *devalias);

With only a single caller to these two monitor commands, I didn't need to wrap a new WithFds version, but just change the command itself. * src/qemu/qemu_monitor.h (qemuMonitorAddNetdev) (qemuMonitorAddHostNetwork): Add parameters. * src/qemu/qemu_monitor.c (qemuMonitorAddNetdev) (qemuMonitorAddHostNetwork): Add support for fd passing. * src/qemu/qemu_hotplug.c (qemuDomainAttachNetDevice): Use it to simplify code. --- v2: new patch src/qemu/qemu_hotplug.c | 57 ++++++--------------------------------------- src/qemu/qemu_monitor.c | 59 +++++++++++++++++++++++++++++++++++++++------- src/qemu/qemu_monitor.h | 8 ++++- 3 files changed, 64 insertions(+), 60 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 87fbb9a..9d7435c 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -611,63 +611,39 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, if (tapfd != -1) { if (virAsprintf(&tapfd_name, "fd-%s", net->info.alias) < 0) goto no_memory; - - qemuDomainObjEnterMonitorWithDriver(driver, vm); - if (qemuMonitorSendFileHandle(priv->mon, tapfd_name, tapfd) < 0) { - qemuDomainObjExitMonitorWithDriver(driver, vm); - goto cleanup; - } - qemuDomainObjExitMonitorWithDriver(driver, vm); - - if (!virDomainObjIsActive(vm)) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("guest unexpectedly quit")); - goto cleanup; - } } if (vhostfd != -1) { if (virAsprintf(&vhostfd_name, "vhostfd-%s", net->info.alias) < 0) goto no_memory; - - qemuDomainObjEnterMonitorWithDriver(driver, vm); - if (qemuMonitorSendFileHandle(priv->mon, vhostfd_name, vhostfd) < 0) { - qemuDomainObjExitMonitorWithDriver(driver, vm); - goto try_tapfd_close; - } - qemuDomainObjExitMonitorWithDriver(driver, vm); - - if (!virDomainObjIsActive(vm)) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("guest unexpectedly quit")); - goto cleanup; - } } if (qemuCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { if (!(netstr = qemuBuildHostNetStr(net, ',', -1, tapfd_name, vhostfd_name))) - goto try_tapfd_close; + goto cleanup; } else { if (!(netstr = qemuBuildHostNetStr(net, ' ', vlan, tapfd_name, vhostfd_name))) - goto try_tapfd_close; + goto cleanup; } qemuDomainObjEnterMonitorWithDriver(driver, vm); if (qemuCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { - if (qemuMonitorAddNetdev(priv->mon, netstr) < 0) { + if (qemuMonitorAddNetdev(priv->mon, netstr, tapfd, tapfd_name, + vhostfd, vhostfd_name) < 0) { qemuDomainObjExitMonitorWithDriver(driver, vm); qemuAuditNet(vm, NULL, net, "attach", false); - goto try_tapfd_close; + goto cleanup; } } else { - if (qemuMonitorAddHostNetwork(priv->mon, netstr) < 0) { + if (qemuMonitorAddHostNetwork(priv->mon, netstr, tapfd, tapfd_name, + vhostfd, vhostfd_name) < 0) { qemuDomainObjExitMonitorWithDriver(driver, vm); qemuAuditNet(vm, NULL, net, "attach", false); - goto try_tapfd_close; + goto cleanup; } } qemuDomainObjExitMonitorWithDriver(driver, vm); @@ -765,23 +741,6 @@ try_remove: } goto cleanup; -try_tapfd_close: - if (!virDomainObjIsActive(vm)) - goto cleanup; - - if (tapfd_name || vhostfd_name) { - qemuDomainObjEnterMonitorWithDriver(driver, vm); - if (tapfd_name && - qemuMonitorCloseFileHandle(priv->mon, tapfd_name) < 0) - VIR_WARN("Failed to close tapfd with '%s'", tapfd_name); - if (vhostfd_name && - qemuMonitorCloseFileHandle(priv->mon, vhostfd_name) < 0) - VIR_WARN("Failed to close vhostfd with '%s'", vhostfd_name); - qemuDomainObjExitMonitorWithDriver(driver, vm); - } - - goto cleanup; - no_memory: virReportOOMError(); goto cleanup; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 074b0b2..65bebe3 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1819,11 +1819,15 @@ int qemuMonitorCloseFileHandle(qemuMonitorPtr mon, int qemuMonitorAddHostNetwork(qemuMonitorPtr mon, - const char *netstr) + const char *netstr, + int tapfd, const char *tapfd_name, + int vhostfd, const char *vhostfd_name) { - int ret; - VIR_DEBUG("mon=%p netstr=%s", - mon, netstr); + int ret = -1; + VIR_DEBUG("mon=%p netstr=%s tapfd=%d tapfd_name=%s " + "vhostfd=%d vhostfd_name=%s", + mon, netstr, tapfd, NULLSTR(tapfd_name), + vhostfd, NULLSTR(vhostfd_name)); if (!mon) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", @@ -1831,10 +1835,27 @@ int qemuMonitorAddHostNetwork(qemuMonitorPtr mon, return -1; } + if (tapfd >= 0 && qemuMonitorSendFileHandle(mon, tapfd_name, tapfd) < 0) + return -1; + if (vhostfd >= 0 && + qemuMonitorSendFileHandle(mon, vhostfd_name, vhostfd) < 0) { + vhostfd = -1; + goto cleanup; + } + if (mon->json) ret = qemuMonitorJSONAddHostNetwork(mon, netstr); else ret = qemuMonitorTextAddHostNetwork(mon, netstr); + +cleanup: + if (ret < 0) { + if (tapfd >= 0 && qemuMonitorCloseFileHandle(mon, tapfd_name) < 0) + VIR_WARN("failed to close device handle '%s'", tapfd_name); + if (vhostfd >= 0 && qemuMonitorCloseFileHandle(mon, vhostfd_name) < 0) + VIR_WARN("failed to close device handle '%s'", vhostfd_name); + } + return ret; } @@ -1862,11 +1883,15 @@ int qemuMonitorRemoveHostNetwork(qemuMonitorPtr mon, int qemuMonitorAddNetdev(qemuMonitorPtr mon, - const char *netdevstr) + const char *netdevstr, + int tapfd, const char *tapfd_name, + int vhostfd, const char *vhostfd_name) { - int ret; - VIR_DEBUG("mon=%p netdevstr=%s", - mon, netdevstr); + int ret = -1; + VIR_DEBUG("mon=%p netdevstr=%s tapfd=%d tapfd_name=%s " + "vhostfd=%d vhostfd_name=%s", + mon, netdevstr, tapfd, NULLSTR(tapfd_name), + vhostfd, NULLSTR(vhostfd_name)); if (!mon) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", @@ -1874,14 +1899,30 @@ int qemuMonitorAddNetdev(qemuMonitorPtr mon, return -1; } + if (tapfd >= 0 && qemuMonitorSendFileHandle(mon, tapfd_name, tapfd) < 0) + return -1; + if (vhostfd >= 0 && + qemuMonitorSendFileHandle(mon, vhostfd_name, vhostfd) < 0) { + vhostfd = -1; + goto cleanup; + } + if (mon->json) ret = qemuMonitorJSONAddNetdev(mon, netdevstr); else ret = qemuMonitorTextAddNetdev(mon, netdevstr); + +cleanup: + if (ret < 0) { + if (tapfd >= 0 && qemuMonitorCloseFileHandle(mon, tapfd_name) < 0) + VIR_WARN("failed to close device handle '%s'", tapfd_name); + if (vhostfd >= 0 && qemuMonitorCloseFileHandle(mon, vhostfd_name) < 0) + VIR_WARN("failed to close device handle '%s'", vhostfd_name); + } + return ret; } - int qemuMonitorRemoveNetdev(qemuMonitorPtr mon, const char *alias) { diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index a20ff8e..e933af1 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -352,14 +352,18 @@ int qemuMonitorCloseFileHandle(qemuMonitorPtr mon, * sendable item here */ int qemuMonitorAddHostNetwork(qemuMonitorPtr mon, - const char *netstr); + const char *netstr, + int tapfd, const char *tapfd_name, + int vhostfd, const char *vhostfd_name); int qemuMonitorRemoveHostNetwork(qemuMonitorPtr mon, int vlan, const char *netname); int qemuMonitorAddNetdev(qemuMonitorPtr mon, - const char *netdevstr); + const char *netdevstr, + int tapfd, const char *tapfd_name, + int vhostfd, const char *vhostfd_name); int qemuMonitorRemoveNetdev(qemuMonitorPtr mon, const char *alias);