[libvirt] [PATCH v2 00/14] Couple of vhost-user fixes and cleanups
v2 of: https://www.redhat.com/archives/libvir-list/2016-August/msg00806.html The first patches are mostly code movement and cleanups. diff to v1: - Hopefully all John's review suggestions are worked in now Michal Privoznik (14): virDomainNetDefParseXML: Realign virDomainNetGetActualType: Return type is virDomainNetType qemuBuildInterfaceCommandLine: Move hostdev handling a bit further qemuBuildInterfaceCommandLine: Move vhostuser handling a bit further qemuBuildInterfaceCommandLine: Move from if-else forest to switch qemuDomainAttachNetDevice: Move hostdev handling a bit further qemuDomainAttachNetDevice: Explicitly list allowed types for hotplug qemuBuildHostNetStr: Explicitly enumerate net types qemuBuildChrChardevStr: Introduce @nowait argument qemuBuildVhostuserCommandLine: Reuse qemuBuildChrChardevStr qemuBuildInterfaceCommandLine: Pass proper args qemuBuildVhostuserCommandLine: Unify -netdev creation qemuBuildHostNetStr: Support VIR_DOMAIN_NET_TYPE_VHOSTUSER qemu_hotplug: Support interface type of vhost-user hotplug src/bhyve/bhyve_command.c | 2 +- src/bhyve/bhyve_process.c | 2 +- src/conf/domain_conf.c | 16 +- src/conf/domain_conf.h | 2 +- src/libxl/libxl_domain.c | 2 +- src/libxl/libxl_driver.c | 2 +- src/lxc/lxc_driver.c | 9 +- src/qemu/qemu_command.c | 182 +++++++++++++-------- src/qemu/qemu_hotplug.c | 130 +++++++++++---- src/qemu/qemu_process.c | 13 +- .../qemuxml2argv-net-hostdev-fail.xml | 39 +++++ .../qemuxml2argv-net-vhostuser-fail.xml | 36 ++++ .../qemuxml2argv-net-vhostuser-multiq.args | 6 +- .../qemuxml2argv-net-vhostuser.args | 4 +- tests/qemuxml2argvtest.c | 7 + 15 files changed, 334 insertions(+), 118 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-hostdev-fail.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-fail.xml -- 2.8.4
There are couple of formatting issues. No functional change though. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f562323..8f04e6f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9170,8 +9170,8 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, ifname = virXMLPropString(cur, "dev"); if (ifname && (flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && - (STRPREFIX(ifname, VIR_NET_GENERATED_PREFIX) || - (prefix && STRPREFIX(ifname, prefix)))) { + (STRPREFIX(ifname, VIR_NET_GENERATED_PREFIX) || + (prefix && STRPREFIX(ifname, prefix)))) { /* An auto-generated target name, blank it out */ VIR_FREE(ifname); } @@ -9732,9 +9732,9 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, filter = NULL; def->filterparams = filterparams; filterparams = NULL; - break; + break; default: - break; + break; } } -- 2.8.4
On 10/06/2016 09:38 AM, Michal Privoznik wrote:
There are couple of formatting issues. No functional change though.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
ACK John
This function for some weird reason returns integer instead of virDomainNetType type. It is important to return the correct type so that we know what values we can expect. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/bhyve/bhyve_command.c | 2 +- src/bhyve/bhyve_process.c | 2 +- src/conf/domain_conf.c | 8 ++++---- src/conf/domain_conf.h | 2 +- src/libxl/libxl_domain.c | 2 +- src/libxl/libxl_driver.c | 2 +- src/lxc/lxc_driver.c | 9 +++++++-- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_hotplug.c | 4 ++-- src/qemu/qemu_process.c | 13 ++++++++++++- 10 files changed, 31 insertions(+), 15 deletions(-) diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c index 9ad3f9b..55ad950 100644 --- a/src/bhyve/bhyve_command.c +++ b/src/bhyve/bhyve_command.c @@ -52,7 +52,7 @@ bhyveBuildNetArgStr(const virDomainDef *def, char macaddr[VIR_MAC_STRING_BUFLEN]; char *realifname = NULL; char *brname = NULL; - int actualType = virDomainNetGetActualType(net); + virDomainNetType actualType = virDomainNetGetActualType(net); if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { if (VIR_STRDUP(brname, virDomainNetGetActualBridgeName(net)) < 0) diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c index 6db070f..9d80976 100644 --- a/src/bhyve/bhyve_process.c +++ b/src/bhyve/bhyve_process.c @@ -77,7 +77,7 @@ bhyveNetCleanup(virDomainObjPtr vm) for (i = 0; i < vm->def->nnets; i++) { virDomainNetDefPtr net = vm->def->nets[i]; - int actualType = virDomainNetGetActualType(net); + virDomainNetType actualType = virDomainNetGetActualType(net); if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { if (net->ifname) { diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8f04e6f..9343770 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -20804,7 +20804,7 @@ virDomainActualNetDefContentsFormat(virBufferPtr buf, bool inSubelement, unsigned int flags) { - int actualType = virDomainNetGetActualType(def); + virDomainNetType actualType = virDomainNetGetActualType(def); if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { if (virDomainHostdevDefFormatSubsys(buf, virDomainNetGetActualHostdev(def), @@ -20884,7 +20884,7 @@ virDomainActualNetDefFormat(virBufferPtr buf, virDomainNetDefPtr def, unsigned int flags) { - unsigned int type; + virDomainNetType type; const char *typeStr; if (!def) @@ -21039,7 +21039,7 @@ virDomainNetDefFormat(virBufferPtr buf, char *prefix, unsigned int flags) { - unsigned int actualType = virDomainNetGetActualType(def); + virDomainNetType actualType = virDomainNetGetActualType(def); bool publicActual = false; int sourceLines = 0; const char *typeStr; @@ -24781,7 +24781,7 @@ virDomainStateReasonFromString(virDomainState state, const char *reason) * otherwise return the value from the NetDef. */ -int +virDomainNetType virDomainNetGetActualType(virDomainNetDefPtr iface) { if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a70bc21..95bcf4f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2814,7 +2814,7 @@ int virDomainGraphicsListenAppendSocket(virDomainGraphicsDefPtr def, const char *socket) ATTRIBUTE_NONNULL(1); -int virDomainNetGetActualType(virDomainNetDefPtr iface); +virDomainNetType virDomainNetGetActualType(virDomainNetDefPtr iface); const char *virDomainNetGetActualBridgeName(virDomainNetDefPtr iface); int virDomainNetGetActualBridgeMACTableManager(virDomainNetDefPtr iface); const char *virDomainNetGetActualDirectDev(virDomainNetDefPtr iface); diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index db2c1dc..3df04cf 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -937,7 +937,7 @@ libxlNetworkPrepareDevices(virDomainDefPtr def) for (i = 0; i < def->nnets; i++) { virDomainNetDefPtr net = def->nets[i]; - int actualType; + virDomainNetType actualType; /* If appropriate, grab a physical device from the configured * network's pool of devices, or resolve bridge device name diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index b66cb1f..f87f549 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3328,7 +3328,7 @@ libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver, virDomainNetDefPtr net) { libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); - int actualType; + virDomainNetType actualType; libxl_device_nic nic; int ret = -1; char mac[VIR_MAC_STRING_BUFLEN]; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index a9e664c..91f67f2 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -3943,7 +3943,7 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn, { virLXCDomainObjPrivatePtr priv = vm->privateData; int ret = -1; - int actualType; + virDomainNetType actualType; virNetDevBandwidthPtr actualBandwidth; char *veth = NULL; @@ -4030,6 +4030,10 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn, case VIR_DOMAIN_NET_TYPE_DIRECT: ignore_value(virNetDevMacVLanDelete(veth)); break; + + default: + /* no-op */ + break; } } @@ -4430,7 +4434,8 @@ static int lxcDomainDetachDeviceNetLive(virDomainObjPtr vm, virDomainDeviceDefPtr dev) { - int detachidx, actualType, ret = -1; + int detachidx, ret = -1; + virDomainNetType actualType; virDomainNetDefPtr detach = NULL; virNetDevVPortProfilePtr vport = NULL; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 578ff8b..2aebf8a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7910,7 +7910,7 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, size_t vhostfdSize = 0; char **tapfdName = NULL; char **vhostfdName = NULL; - int actualType = virDomainNetGetActualType(net); + virDomainNetType actualType = virDomainNetGetActualType(net); virQEMUDriverConfigPtr cfg = NULL; virNetDevBandwidthPtr actualBandwidth; size_t i; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 72dd93b..bb8ea0d 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -927,7 +927,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, int vlan; bool releaseaddr = false; bool iface_connected = false; - int actualType; + virDomainNetType actualType; virNetDevBandwidthPtr actualBandwidth; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virDomainCCWAddressSetPtr ccwaddrs = NULL; @@ -2373,7 +2373,7 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, virDomainNetDefPtr newdev = dev->data.net; virDomainNetDefPtr *devslot = NULL; virDomainNetDefPtr olddev; - int oldType, newType; + virDomainNetType oldType, newType; bool needReconnect = false; bool needBridgeChange = false; bool needFilterChange = false; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a4bc082..aa6c36f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4628,7 +4628,7 @@ qemuProcessNetworkPrepareDevices(virDomainDefPtr def) for (i = 0; i < def->nnets; i++) { virDomainNetDefPtr net = def->nets[i]; - int actualType; + virDomainNetType actualType; /* If appropriate, grab a physical device from the configured * network's pool of devices, or resolve bridge device name @@ -6038,6 +6038,17 @@ void qemuProcessStop(virQEMUDriverPtr driver, ignore_value(virNetDevTapDelete(net->ifname, net->backend.tap)); #endif break; + case VIR_DOMAIN_NET_TYPE_USER: + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: + case VIR_DOMAIN_NET_TYPE_SERVER: + case VIR_DOMAIN_NET_TYPE_CLIENT: + case VIR_DOMAIN_NET_TYPE_MCAST: + case VIR_DOMAIN_NET_TYPE_INTERNAL: + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + case VIR_DOMAIN_NET_TYPE_UDP: + case VIR_DOMAIN_NET_TYPE_LAST: + /* No special cleanup procedure for these types. */ + break; } /* release the physical device (or any other resources used by * this interface in the network driver -- 2.8.4
On 10/06/2016 09:38 AM, Michal Privoznik wrote:
This function for some weird reason returns integer instead of virDomainNetType type. It is important to return the correct type so that we know what values we can expect.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/bhyve/bhyve_command.c | 2 +- src/bhyve/bhyve_process.c | 2 +- src/conf/domain_conf.c | 8 ++++---- src/conf/domain_conf.h | 2 +- src/libxl/libxl_domain.c | 2 +- src/libxl/libxl_driver.c | 2 +- src/lxc/lxc_driver.c | 9 +++++++-- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_hotplug.c | 4 ++-- src/qemu/qemu_process.c | 13 ++++++++++++- 10 files changed, 31 insertions(+), 15 deletions(-)
diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c index 9ad3f9b..55ad950 100644 --- a/src/bhyve/bhyve_command.c +++ b/src/bhyve/bhyve_command.c @@ -52,7 +52,7 @@ bhyveBuildNetArgStr(const virDomainDef *def, char macaddr[VIR_MAC_STRING_BUFLEN]; char *realifname = NULL; char *brname = NULL; - int actualType = virDomainNetGetActualType(net); + virDomainNetType actualType = virDomainNetGetActualType(net);
if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { if (VIR_STRDUP(brname, virDomainNetGetActualBridgeName(net)) < 0) diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c index 6db070f..9d80976 100644 --- a/src/bhyve/bhyve_process.c +++ b/src/bhyve/bhyve_process.c @@ -77,7 +77,7 @@ bhyveNetCleanup(virDomainObjPtr vm)
for (i = 0; i < vm->def->nnets; i++) { virDomainNetDefPtr net = vm->def->nets[i]; - int actualType = virDomainNetGetActualType(net); + virDomainNetType actualType = virDomainNetGetActualType(net);
if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { if (net->ifname) { diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8f04e6f..9343770 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -20804,7 +20804,7 @@ virDomainActualNetDefContentsFormat(virBufferPtr buf, bool inSubelement, unsigned int flags) { - int actualType = virDomainNetGetActualType(def); + virDomainNetType actualType = virDomainNetGetActualType(def);
if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { if (virDomainHostdevDefFormatSubsys(buf, virDomainNetGetActualHostdev(def), @@ -20884,7 +20884,7 @@ virDomainActualNetDefFormat(virBufferPtr buf, virDomainNetDefPtr def, unsigned int flags) { - unsigned int type; + virDomainNetType type; const char *typeStr;
if (!def) @@ -21039,7 +21039,7 @@ virDomainNetDefFormat(virBufferPtr buf, char *prefix, unsigned int flags) { - unsigned int actualType = virDomainNetGetActualType(def); + virDomainNetType actualType = virDomainNetGetActualType(def); bool publicActual = false; int sourceLines = 0; const char *typeStr; @@ -24781,7 +24781,7 @@ virDomainStateReasonFromString(virDomainState state, const char *reason) * otherwise return the value from the NetDef. */
-int +virDomainNetType virDomainNetGetActualType(virDomainNetDefPtr iface) { if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a70bc21..95bcf4f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2814,7 +2814,7 @@ int virDomainGraphicsListenAppendSocket(virDomainGraphicsDefPtr def, const char *socket) ATTRIBUTE_NONNULL(1);
-int virDomainNetGetActualType(virDomainNetDefPtr iface); +virDomainNetType virDomainNetGetActualType(virDomainNetDefPtr iface); const char *virDomainNetGetActualBridgeName(virDomainNetDefPtr iface); int virDomainNetGetActualBridgeMACTableManager(virDomainNetDefPtr iface); const char *virDomainNetGetActualDirectDev(virDomainNetDefPtr iface); diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index db2c1dc..3df04cf 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -937,7 +937,7 @@ libxlNetworkPrepareDevices(virDomainDefPtr def)
for (i = 0; i < def->nnets; i++) { virDomainNetDefPtr net = def->nets[i]; - int actualType; + virDomainNetType actualType;
/* If appropriate, grab a physical device from the configured * network's pool of devices, or resolve bridge device name diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index b66cb1f..f87f549 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3328,7 +3328,7 @@ libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver, virDomainNetDefPtr net) { libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); - int actualType; + virDomainNetType actualType; libxl_device_nic nic; int ret = -1; char mac[VIR_MAC_STRING_BUFLEN]; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index a9e664c..91f67f2 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -3943,7 +3943,7 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn, { virLXCDomainObjPrivatePtr priv = vm->privateData; int ret = -1; - int actualType; + virDomainNetType actualType; virNetDevBandwidthPtr actualBandwidth; char *veth = NULL;
@@ -4030,6 +4030,10 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn, case VIR_DOMAIN_NET_TYPE_DIRECT: ignore_value(virNetDevMacVLanDelete(veth)); break; + + default: + /* no-op */ + break;
Naturally this draws the attention of the [dead_error_condition] for Coverity. Oh and if each of the other cases are added instead of using default, then there's multiple dead error conditions.... It just doesn't make sense <sigh>. Even stranger still if I replace the actualType with just using virDomainNetGetActualType(net) directly, then the Coverity warnings go away. So there's something about assigning the variable that causes Coverity to stick it's nose where it doesn't belong... <double sigh> IDC if you keep things as is, I'll just add a false positive in my local build... Not that it matters (or is related), but the switch/case before this one has no reason to go to cleanup - each condition could just return -1. The only reason to go to cleanup is if "veth" exists and the only way for that to happen is if the previous cases are successful.
} }
@@ -4430,7 +4434,8 @@ static int lxcDomainDetachDeviceNetLive(virDomainObjPtr vm, virDomainDeviceDefPtr dev) { - int detachidx, actualType, ret = -1; + int detachidx, ret = -1; + virDomainNetType actualType; virDomainNetDefPtr detach = NULL; virNetDevVPortProfilePtr vport = NULL;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 578ff8b..2aebf8a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7910,7 +7910,7 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, size_t vhostfdSize = 0; char **tapfdName = NULL; char **vhostfdName = NULL; - int actualType = virDomainNetGetActualType(net); + virDomainNetType actualType = virDomainNetGetActualType(net); virQEMUDriverConfigPtr cfg = NULL; virNetDevBandwidthPtr actualBandwidth; size_t i; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 72dd93b..bb8ea0d 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -927,7 +927,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, int vlan; bool releaseaddr = false; bool iface_connected = false; - int actualType; + virDomainNetType actualType; virNetDevBandwidthPtr actualBandwidth; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virDomainCCWAddressSetPtr ccwaddrs = NULL; @@ -2373,7 +2373,7 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, virDomainNetDefPtr newdev = dev->data.net; virDomainNetDefPtr *devslot = NULL; virDomainNetDefPtr olddev; - int oldType, newType; + virDomainNetType oldType, newType; bool needReconnect = false; bool needBridgeChange = false; bool needFilterChange = false; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a4bc082..aa6c36f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4628,7 +4628,7 @@ qemuProcessNetworkPrepareDevices(virDomainDefPtr def)
for (i = 0; i < def->nnets; i++) { virDomainNetDefPtr net = def->nets[i]; - int actualType; + virDomainNetType actualType;
/* If appropriate, grab a physical device from the configured * network's pool of devices, or resolve bridge device name @@ -6038,6 +6038,17 @@ void qemuProcessStop(virQEMUDriverPtr driver, ignore_value(virNetDevTapDelete(net->ifname, net->backend.tap)); #endif break; + case VIR_DOMAIN_NET_TYPE_USER: + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: + case VIR_DOMAIN_NET_TYPE_SERVER: + case VIR_DOMAIN_NET_TYPE_CLIENT: + case VIR_DOMAIN_NET_TYPE_MCAST: + case VIR_DOMAIN_NET_TYPE_INTERNAL: + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + case VIR_DOMAIN_NET_TYPE_UDP: + case VIR_DOMAIN_NET_TYPE_LAST: + /* No special cleanup procedure for these types. */ + break;
Ironically Coverity doesn't complain on this one. There's no rhyme or reason... This of course is where/why I got the idea to not use the local variable. ACK for the changes John
} /* release the physical device (or any other resources used by * this interface in the network driver
The idea is to have function that does some checking of the arguments at its beginning and then have one big switch for all the interface types it supports. Each one of them generating the corresponding part of the command line. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 13 ++++---- .../qemuxml2argv-net-hostdev-fail.xml | 39 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 +++ 3 files changed, 49 insertions(+), 7 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-hostdev-fail.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2aebf8a..0da2add 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7922,13 +7922,6 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) return qemuBuildVhostuserCommandLine(cmd, def, net, qemuCaps, bootindex); - if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { - /* NET_TYPE_HOSTDEV devices are really hostdev devices, so - * their commandlines are constructed with other hostdevs. - */ - return 0; - } - /* Currently nothing besides TAP devices supports multiqueue. */ if (net->driver.virtio.queues > 0 && !(actualType == VIR_DOMAIN_NET_TYPE_NETWORK || @@ -8008,6 +8001,12 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, if (qemuInterfaceEthernetConnect(def, driver, net, tapfd, tapfdSize) < 0) goto cleanup; + } else if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + /* NET_TYPE_HOSTDEV devices are really hostdev devices, so + * their commandlines are constructed with other hostdevs. + */ + ret = 0; + goto cleanup; } /* For types whose implementations use a netdev on the host, add diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-hostdev-fail.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-hostdev-fail.xml new file mode 100644 index 0000000..7807d79 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-hostdev-fail.xml @@ -0,0 +1,39 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <interface type='hostdev' managed='yes'> + <mac address='00:11:22:33:44:55'/> + <source> + <address type='pci' domain='0x0000' bus='0x03' slot='0x07' function='0x1'/> + </source> + <model type='virtio'/> + <filterref filter='myfilter'/> + <backend tap='/dev/mytap'/> + <driver queues='4'/> + </interface> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 4b9ecb8..442ab31 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1104,6 +1104,10 @@ mymain(void) QEMU_CAPS_DEVICE_VFIO_PCI, QEMU_CAPS_HOST_PCI_MULTIDOMAIN); DO_TEST_FAILURE("net-hostdev-vfio-multidomain", QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_DEVICE_VFIO_PCI); + DO_TEST_FAILURE("net-hostdev-fail", + QEMU_CAPS_NODEFCONFIG, + QEMU_CAPS_DEVICE_VFIO_PCI); + DO_TEST("serial-vc", NONE); DO_TEST("serial-pty", NONE); -- 2.8.4
On 10/06/2016 09:38 AM, Michal Privoznik wrote:
The idea is to have function that does some checking of the arguments at its beginning and then have one big switch for all the interface types it supports. Each one of them generating the corresponding part of the command line.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 13 ++++---- .../qemuxml2argv-net-hostdev-fail.xml | 39 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 +++ 3 files changed, 49 insertions(+), 7 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-hostdev-fail.xml
Is it safe to assume that previously if the XML had "<filterref filter='myfilter'/>, <backend tap='/dev/mytap'/>, and/or "<driver queues='4'/>" that it would be ignored, but now there will be an error at command line build time? Hopefully we get no complaints ;-) ACK John
On 13.10.2016 22:58, John Ferlan wrote:
On 10/06/2016 09:38 AM, Michal Privoznik wrote:
The idea is to have function that does some checking of the arguments at its beginning and then have one big switch for all the interface types it supports. Each one of them generating the corresponding part of the command line.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 13 ++++---- .../qemuxml2argv-net-hostdev-fail.xml | 39 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 +++ 3 files changed, 49 insertions(+), 7 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-hostdev-fail.xml
Is it safe to assume that previously if the XML had "<filterref filter='myfilter'/>, <backend tap='/dev/mytap'/>, and/or "<driver queues='4'/>" that it would be ignored, but now there will be an error at command line build time?
Yeah. I mean, we were lying to users before letting them think we set one of those things you mentioned. We should error out.
Hopefully we get no complaints ;-)
Well, if we do, we can at least tell them sorry for fooling you :-) Michal
The idea is to have function that does some checking of the arguments at its beginning and then have one big switch for all the interface types it supports. Each one of them generating the corresponding part of the command line. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 9 +++--- .../qemuxml2argv-net-vhostuser-fail.xml | 36 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 3 ++ 3 files changed, 44 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-fail.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0da2add..5ca82ec 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7919,15 +7919,13 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, if (!bootindex) bootindex = net->info.bootIndex; - if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) - return qemuBuildVhostuserCommandLine(cmd, def, net, qemuCaps, bootindex); - /* Currently nothing besides TAP devices supports multiqueue. */ if (net->driver.virtio.queues > 0 && !(actualType == VIR_DOMAIN_NET_TYPE_NETWORK || actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || actualType == VIR_DOMAIN_NET_TYPE_DIRECT || - actualType == VIR_DOMAIN_NET_TYPE_ETHERNET)) { + actualType == VIR_DOMAIN_NET_TYPE_ETHERNET || + actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Multiqueue network is not supported for: %s"), virDomainNetTypeToString(actualType)); @@ -8007,6 +8005,9 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, */ ret = 0; goto cleanup; + } else if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) { + ret = qemuBuildVhostuserCommandLine(cmd, def, net, qemuCaps, bootindex); + goto cleanup; } /* For types whose implementations use a netdev on the host, add diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-fail.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-fail.xml new file mode 100644 index 0000000..e9fe14f --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-fail.xml @@ -0,0 +1,36 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <interface type='vhostuser'> + <mac address='52:54:00:ee:96:6b'/> + <source type='unix' path='/tmp/vhost0.sock' mode='server'/> + <model type='virtio'/> + <filterref filter='myfilter'/> + <backend tap='/dev/mytap'/> + <driver queues='4'/> + </interface> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 442ab31..8deb651 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1066,6 +1066,9 @@ mymain(void) DO_TEST("net-vhostuser-multiq", QEMU_CAPS_NETDEV, QEMU_CAPS_VHOSTUSER_MULTIQUEUE); DO_TEST_FAILURE("net-vhostuser-multiq", QEMU_CAPS_NETDEV); + DO_TEST_FAILURE("net-vhostuser-fail", + QEMU_CAPS_NETDEV, + QEMU_CAPS_VHOSTUSER_MULTIQUEUE); DO_TEST("net-user", NONE); DO_TEST("net-virtio", NONE); DO_TEST("net-virtio-device", -- 2.8.4
On 10/06/2016 09:38 AM, Michal Privoznik wrote:
The idea is to have function that does some checking of the arguments at its beginning and then have one big switch for all the interface types it supports. Each one of them generating the corresponding part of the command line.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 9 +++--- .../qemuxml2argv-net-vhostuser-fail.xml | 36 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 3 ++ 3 files changed, 44 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-fail.xml
Similarly to patch 3, if a vhost-user entry contains 'filterref' or 'backend tap', then bulding will fail. Although queues is Ok. BTW: I just realized cfg isn't even used in this function other than the GetConfig call and Unref call. ACK, John
On 13.10.2016 22:58, John Ferlan wrote:
On 10/06/2016 09:38 AM, Michal Privoznik wrote:
The idea is to have function that does some checking of the arguments at its beginning and then have one big switch for all the interface types it supports. Each one of them generating the corresponding part of the command line.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 9 +++--- .../qemuxml2argv-net-vhostuser-fail.xml | 36 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 3 ++ 3 files changed, 44 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-fail.xml
Similarly to patch 3, if a vhost-user entry contains 'filterref' or 'backend tap', then bulding will fail. Although queues is Ok.
BTW: I just realized cfg isn't even used in this function other than the GetConfig call and Unref call.
Yes, I've noticed that too and I'm fixing it somewhere in a later patch. Michal
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5ca82ec..cc3b24f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7956,8 +7956,9 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, cfg = virQEMUDriverGetConfig(driver); - if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || - actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { + switch (actualType) { + case VIR_DOMAIN_NET_TYPE_NETWORK: + case VIR_DOMAIN_NET_TYPE_BRIDGE: tapfdSize = net->driver.virtio.queues; if (!tapfdSize) tapfdSize = 1; @@ -7971,7 +7972,9 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, if (qemuInterfaceBridgeConnect(def, driver, net, tapfd, &tapfdSize) < 0) goto cleanup; - } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { + break; + + case VIR_DOMAIN_NET_TYPE_DIRECT: tapfdSize = net->driver.virtio.queues; if (!tapfdSize) tapfdSize = 1; @@ -7985,7 +7988,9 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, if (qemuInterfaceDirectConnect(def, driver, net, tapfd, tapfdSize, vmop) < 0) goto cleanup; - } else if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET) { + break; + + case VIR_DOMAIN_NET_TYPE_ETHERNET: tapfdSize = net->driver.virtio.queues; if (!tapfdSize) tapfdSize = 1; @@ -7997,17 +8002,32 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, memset(tapfd, -1, tapfdSize * sizeof(tapfd[0])); if (qemuInterfaceEthernetConnect(def, driver, net, - tapfd, tapfdSize) < 0) + tapfd, tapfdSize) < 0) goto cleanup; - } else if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + break; + + case VIR_DOMAIN_NET_TYPE_HOSTDEV: /* NET_TYPE_HOSTDEV devices are really hostdev devices, so * their commandlines are constructed with other hostdevs. */ ret = 0; goto cleanup; - } else if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) { + break; + + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: ret = qemuBuildVhostuserCommandLine(cmd, def, net, qemuCaps, bootindex); goto cleanup; + break; + + case VIR_DOMAIN_NET_TYPE_USER: + case VIR_DOMAIN_NET_TYPE_SERVER: + case VIR_DOMAIN_NET_TYPE_CLIENT: + case VIR_DOMAIN_NET_TYPE_MCAST: + case VIR_DOMAIN_NET_TYPE_INTERNAL: + case VIR_DOMAIN_NET_TYPE_UDP: + case VIR_DOMAIN_NET_TYPE_LAST: + /* nada */ + break; } /* For types whose implementations use a netdev on the host, add -- 2.8.4
On 10/06/2016 09:38 AM, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-)
ACK John
The idea is to have function that does some checking at its beginning and then have one big switch for all the interface types it supports. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index bb8ea0d..95bbb35 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -946,20 +946,6 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, actualType = virDomainNetGetActualType(net); - if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { - /* This is really a "smart hostdev", so it should be attached - * as a hostdev (the hostdev code will reach over into the - * netdev-specific code as appropriate), then also added to - * the nets list (see cleanup:) if successful. - * - * qemuDomainAttachHostDevice uses a connection to resolve - * a SCSI hostdev secret, which is not this case, so pass NULL. - */ - ret = qemuDomainAttachHostDevice(NULL, driver, vm, - virDomainNetGetActualHostdev(net)); - goto cleanup; - } - /* Currently only TAP/macvtap devices supports multiqueue. */ if (net->driver.virtio.queues > 0 && !(actualType == VIR_DOMAIN_NET_TYPE_NETWORK || @@ -1037,6 +1023,18 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, if (qemuInterfaceOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd, &vhostfdSize) < 0) goto cleanup; + } else if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + /* This is really a "smart hostdev", so it should be attached + * as a hostdev (the hostdev code will reach over into the + * netdev-specific code as appropriate), then also added to + * the nets list (see cleanup:) if successful. + * + * qemuDomainAttachHostDevice uses a connection to resolve + * a SCSI hostdev secret, which is not this case, so pass NULL. + */ + ret = qemuDomainAttachHostDevice(NULL, driver, vm, + virDomainNetGetActualHostdev(net)); + goto cleanup; } /* Set device online immediately */ -- 2.8.4
On 10/06/2016 09:38 AM, Michal Privoznik wrote:
The idea is to have function that does some checking at its beginning and then have one big switch for all the interface types it supports.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-)
Similar to patch 3, we'll now fail if filterref and queues exist in the to be attached device's XML, but it doesn't check the backend tap. Not a problem with this patch per se, but perhaps something that could be put on the virtual todo list (make the checks consistent between building command line and hotplug and of course do so by adding some helper function rather than duplicating the checks which are prone to issues)... ACK for what's here though John
Instead of blindly claim support for hot-plugging of every interface type out there we should copy approach we have for device types: white listing supported types and explicitly error out on unsupported ones. For instance, trying to hotplug vhostuser interface results in nothing usable from guest currently. vhostuser typed interfaces require additional work on our side. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 95bbb35..1b2a075 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -970,8 +970,9 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, return -1; } - if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || - actualType == VIR_DOMAIN_NET_TYPE_NETWORK) { + switch (actualType) { + case VIR_DOMAIN_NET_TYPE_BRIDGE: + case VIR_DOMAIN_NET_TYPE_NETWORK: tapfdSize = vhostfdSize = net->driver.virtio.queues; if (!tapfdSize) tapfdSize = vhostfdSize = 1; @@ -988,7 +989,9 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, if (qemuInterfaceOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd, &vhostfdSize) < 0) goto cleanup; - } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { + break; + + case VIR_DOMAIN_NET_TYPE_DIRECT: tapfdSize = vhostfdSize = net->driver.virtio.queues; if (!tapfdSize) tapfdSize = vhostfdSize = 1; @@ -1006,7 +1009,9 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, if (qemuInterfaceOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd, &vhostfdSize) < 0) goto cleanup; - } else if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET) { + break; + + case VIR_DOMAIN_NET_TYPE_ETHERNET: tapfdSize = vhostfdSize = net->driver.virtio.queues; if (!tapfdSize) tapfdSize = vhostfdSize = 1; @@ -1017,13 +1022,15 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, goto cleanup; memset(vhostfd, -1, sizeof(*vhostfd) * vhostfdSize); if (qemuInterfaceEthernetConnect(vm->def, driver, net, - tapfd, tapfdSize) < 0) + tapfd, tapfdSize) < 0) goto cleanup; iface_connected = true; if (qemuInterfaceOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd, &vhostfdSize) < 0) goto cleanup; - } else if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + break; + + case VIR_DOMAIN_NET_TYPE_HOSTDEV: /* This is really a "smart hostdev", so it should be attached * as a hostdev (the hostdev code will reach over into the * netdev-specific code as appropriate), then also added to @@ -1035,6 +1042,20 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, ret = qemuDomainAttachHostDevice(NULL, driver, vm, virDomainNetGetActualHostdev(net)); goto cleanup; + break; + + case VIR_DOMAIN_NET_TYPE_USER: + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: + case VIR_DOMAIN_NET_TYPE_SERVER: + case VIR_DOMAIN_NET_TYPE_CLIENT: + case VIR_DOMAIN_NET_TYPE_MCAST: + case VIR_DOMAIN_NET_TYPE_INTERNAL: + case VIR_DOMAIN_NET_TYPE_UDP: + case VIR_DOMAIN_NET_TYPE_LAST: + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("hotplug of interface type of %s is not implemented yet"), + virDomainNetTypeToString(actualType)); + goto cleanup; } /* Set device online immediately */ -- 2.8.4
On 10/06/2016 09:38 AM, Michal Privoznik wrote:
Instead of blindly claim support for hot-plugging of every interface type out there we should copy approach we have for device types: white listing supported types and explicitly error out on unsupported ones. For instance, trying to hotplug vhostuser interface results in nothing usable from guest currently. vhostuser typed interfaces require additional work on our side.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-)
Should have just kept quiet in the v1 review about a switch ;-) FWIW: If someone tries to add a vhostuser it will fail before the unsupported message because the "if (net->driver.virtio.queues > 0 && ..." doesn't have VHOSTUSER in the list like it does for command line. I'm OK with adding that in here now or as a separate patch... ACK for what's here. John
We tend to prevent using 'default' in switches. And it is for a good reason - control may end up in paths we wouldn't want for new values. In this specific case, if qemuBuildHostNetStr is called over VIR_DOMAIN_NET_TYPE_VHOSTUSER it would produce meaningless output. Fortunately, there no such call yet. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index cc3b24f..4a67d80 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3711,9 +3711,21 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, break; case VIR_DOMAIN_NET_TYPE_USER: - default: + case VIR_DOMAIN_NET_TYPE_INTERNAL: virBufferAddLit(&buf, "user"); break; + + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + /* Should have been handled earlier via PCI/USB hotplug code. */ + virObjectUnref(cfg); + return NULL; + + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: + /* Unsupported yet. */ + break; + + case VIR_DOMAIN_NET_TYPE_LAST: + break; } if (vlan >= 0) { -- 2.8.4
On 10/06/2016 09:38 AM, Michal Privoznik wrote:
We tend to prevent using 'default' in switches. And it is for a good reason - control may end up in paths we wouldn't want for new values. In this specific case, if qemuBuildHostNetStr is called over VIR_DOMAIN_NET_TYPE_VHOSTUSER it would produce meaningless output. Fortunately, there no such call yet.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
Doesn't seem anything changed from the original patch (which is fine). Yet another function that doesn't use/need the 'cfg' variable... ACK to what's here, I'd support removing cfg as well John
This alone makes not much sense. But the aim is to reuse this function in qemuBuildVhostuserCommandLine() where 'nowait' is not supported for vhost-user devices. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4a67d80..71067ee 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4934,7 +4934,8 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, const virDomainDef *def, const virDomainChrSourceDef *dev, const char *alias, - virQEMUCapsPtr qemuCaps) + virQEMUCapsPtr qemuCaps, + bool nowait) { virBuffer buf = VIR_BUFFER_INITIALIZER; bool telnet; @@ -5007,12 +5008,14 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, case VIR_DOMAIN_CHR_TYPE_TCP: telnet = dev->data.tcp.protocol == VIR_DOMAIN_CHR_TCP_PROTOCOL_TELNET; virBufferAsprintf(&buf, - "socket,id=char%s,host=%s,port=%s%s%s", + "socket,id=char%s,host=%s,port=%s%s", alias, dev->data.tcp.host, dev->data.tcp.service, - telnet ? ",telnet" : "", - dev->data.tcp.listen ? ",server,nowait" : ""); + telnet ? ",telnet" : ""); + + if (dev->data.tcp.listen) + virBufferAdd(&buf, nowait ? ",server,nowait" : ",server", -1); if (cfg->chardevTLS) { char *objalias = NULL; @@ -5034,7 +5037,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, virBufferAsprintf(&buf, "socket,id=char%s,path=", alias); virQEMUBuildBufferEscapeComma(&buf, dev->data.nix.path); if (dev->data.nix.listen) - virBufferAddLit(&buf, ",server,nowait"); + virBufferAdd(&buf, nowait ? ",server,nowait" : ",server", -1); break; case VIR_DOMAIN_CHR_TYPE_SPICEVMC: @@ -5358,7 +5361,7 @@ qemuBuildMonitorCommandLine(virLogManagerPtr logManager, if (!(chrdev = qemuBuildChrChardevStr(logManager, cmd, cfg, def, monitor_chr, "monitor", - qemuCaps))) + qemuCaps, true))) return -1; virCommandAddArg(cmd, "-chardev"); virCommandAddArg(cmd, chrdev); @@ -5516,7 +5519,7 @@ qemuBuildRNGBackendChrdevStr(virLogManagerPtr logManager, case VIR_DOMAIN_RNG_BACKEND_EGD: if (!(*chr = qemuBuildChrChardevStr(logManager, cmd, cfg, def, rng->source.chardev, - rng->info.alias, qemuCaps))) + rng->info.alias, qemuCaps, true))) return -1; } @@ -8375,7 +8378,7 @@ qemuBuildSmartcardCommandLine(virLogManagerPtr logManager, if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, cfg, def, &smartcard->data.passthru, smartcard->info.alias, - qemuCaps))) { + qemuCaps, true))) { virBufferFreeAndReset(&opt); return -1; } @@ -8464,7 +8467,7 @@ qemuBuildShmemBackendChrStr(virLogManagerPtr logManager, devstr = qemuBuildChrChardevStr(logManager, cmd, cfg, def, &shmem->server.chr, - shmem->info.alias, qemuCaps); + shmem->info.alias, qemuCaps, true); return devstr; } @@ -8568,7 +8571,7 @@ qemuBuildSerialCommandLine(virLogManagerPtr logManager, if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, cfg, def, &serial->source, serial->info.alias, - qemuCaps))) + qemuCaps, true))) return -1; virCommandAddArg(cmd, "-chardev"); virCommandAddArg(cmd, devstr); @@ -8607,7 +8610,7 @@ qemuBuildParallelsCommandLine(virLogManagerPtr logManager, if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, cfg, def, ¶llel->source, parallel->info.alias, - qemuCaps))) + qemuCaps, true))) return -1; virCommandAddArg(cmd, "-chardev"); virCommandAddArg(cmd, devstr); @@ -8653,7 +8656,7 @@ qemuBuildChannelsCommandLine(virLogManagerPtr logManager, if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, cfg, def, &channel->source, channel->info.alias, - qemuCaps))) + qemuCaps, true))) return -1; virCommandAddArg(cmd, "-chardev"); virCommandAddArg(cmd, devstr); @@ -8676,7 +8679,7 @@ qemuBuildChannelsCommandLine(virLogManagerPtr logManager, if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, cfg, def, &channel->source, channel->info.alias, - qemuCaps))) + qemuCaps, true))) return -1; virCommandAddArg(cmd, "-chardev"); virCommandAddArg(cmd, devstr); @@ -8719,7 +8722,7 @@ qemuBuildConsoleCommandLine(virLogManagerPtr logManager, if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, cfg, def, &console->source, console->info.alias, - qemuCaps))) + qemuCaps, true))) return -1; virCommandAddArg(cmd, "-chardev"); virCommandAddArg(cmd, devstr); @@ -8733,7 +8736,7 @@ qemuBuildConsoleCommandLine(virLogManagerPtr logManager, if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, cfg, def, &console->source, console->info.alias, - qemuCaps))) + qemuCaps, true))) return -1; virCommandAddArg(cmd, "-chardev"); virCommandAddArg(cmd, devstr); @@ -8862,7 +8865,7 @@ qemuBuildRedirdevCommandLine(virLogManagerPtr logManager, if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, cfg, def, &redirdev->source.chr, redirdev->info.alias, - qemuCaps))) { + qemuCaps, true))) { return -1; } -- 2.8.4
On 10/06/2016 09:38 AM, Michal Privoznik wrote:
This alone makes not much sense. But the aim is to reuse this function in qemuBuildVhostuserCommandLine() where 'nowait' is not supported for vhost-user devices.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-)
ACK John
There's no need to reinvent the wheel here. We already have a function to format virDomainChrSourceDefPtr. It's called qemuBuildChrChardevStr(). Use that instead of some dummy virBufferAsprintf(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 71067ee..246ffe0 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7828,7 +7828,7 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd, virQEMUCapsPtr qemuCaps, unsigned int bootindex) { - virBuffer chardev_buf = VIR_BUFFER_INITIALIZER; + char *chardev = NULL; virBuffer netdev_buf = VIR_BUFFER_INITIALIZER; unsigned int queues = net->driver.virtio.queues; char *nic = NULL; @@ -7841,9 +7841,10 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd, switch ((virDomainChrType) net->data.vhostuser->type) { case VIR_DOMAIN_CHR_TYPE_UNIX: - virBufferAsprintf(&chardev_buf, "socket,id=char%s,path=%s%s", - net->info.alias, net->data.vhostuser->data.nix.path, - net->data.vhostuser->data.nix.listen ? ",server" : ""); + if (!(chardev = qemuBuildChrChardevStr(NULL, NULL, NULL, def, + net->data.vhostuser, + net->info.alias, qemuCaps, false))) + goto error; break; case VIR_DOMAIN_CHR_TYPE_NULL: @@ -7861,7 +7862,7 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd, case VIR_DOMAIN_CHR_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("vhost-user type '%s' not supported"), - virDomainChrTypeToString(net->data.vhostuser->type)); + virDomainChrTypeToString(net->data.vhostuser->type)); goto error; } @@ -7879,7 +7880,8 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd, } virCommandAddArg(cmd, "-chardev"); - virCommandAddArgBuffer(cmd, &chardev_buf); + virCommandAddArg(cmd, chardev); + VIR_FREE(chardev); virCommandAddArg(cmd, "-netdev"); virCommandAddArgBuffer(cmd, &netdev_buf); @@ -7897,7 +7899,7 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd, return 0; error: - virBufferFreeAndReset(&chardev_buf); + VIR_FREE(chardev); virBufferFreeAndReset(&netdev_buf); VIR_FREE(nic); -- 2.8.4
On 10/06/2016 09:38 AM, Michal Privoznik wrote:
There's no need to reinvent the wheel here. We already have a function to format virDomainChrSourceDefPtr. It's called qemuBuildChrChardevStr(). Use that instead of some dummy virBufferAsprintf().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)
If someone bisected to here, they'd have issues because patch 11 needs to be merged here... ACK if merged with patch 11 John
Instead of various NULL arguments, we can pass proper qemu driver config, log manager, and virCommand. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 246ffe0..895bb23 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7822,12 +7822,15 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg, } static int -qemuBuildVhostuserCommandLine(virCommandPtr cmd, +qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver, + virLogManagerPtr logManager, + virCommandPtr cmd, virDomainDefPtr def, virDomainNetDefPtr net, virQEMUCapsPtr qemuCaps, unsigned int bootindex) { + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); char *chardev = NULL; virBuffer netdev_buf = VIR_BUFFER_INITIALIZER; unsigned int queues = net->driver.virtio.queues; @@ -7841,7 +7844,7 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd, switch ((virDomainChrType) net->data.vhostuser->type) { case VIR_DOMAIN_CHR_TYPE_UNIX: - if (!(chardev = qemuBuildChrChardevStr(NULL, NULL, NULL, def, + if (!(chardev = qemuBuildChrChardevStr(logManager, cmd, cfg, def, net->data.vhostuser, net->info.alias, qemuCaps, false))) goto error; @@ -7895,10 +7898,12 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd, virCommandAddArgList(cmd, "-device", nic, NULL); VIR_FREE(nic); + virObjectUnref(cfg); return 0; error: + virObjectUnref(cfg); VIR_FREE(chardev); virBufferFreeAndReset(&netdev_buf); VIR_FREE(nic); @@ -7907,8 +7912,9 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd, } static int -qemuBuildInterfaceCommandLine(virCommandPtr cmd, - virQEMUDriverPtr driver, +qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, + virLogManagerPtr logManager, + virCommandPtr cmd, virDomainDefPtr def, virDomainNetDefPtr net, virQEMUCapsPtr qemuCaps, @@ -7928,7 +7934,6 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, char **tapfdName = NULL; char **vhostfdName = NULL; virDomainNetType actualType = virDomainNetGetActualType(net); - virQEMUDriverConfigPtr cfg = NULL; virNetDevBandwidthPtr actualBandwidth; size_t i; @@ -7971,8 +7976,6 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, return -1; } - cfg = virQEMUDriverGetConfig(driver); - switch (actualType) { case VIR_DOMAIN_NET_TYPE_NETWORK: case VIR_DOMAIN_NET_TYPE_BRIDGE: @@ -8032,7 +8035,8 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, break; case VIR_DOMAIN_NET_TYPE_VHOSTUSER: - ret = qemuBuildVhostuserCommandLine(cmd, def, net, qemuCaps, bootindex); + ret = qemuBuildVhostuserCommandLine(driver, logManager, cmd, def, + net, qemuCaps, bootindex); goto cleanup; break; @@ -8205,7 +8209,6 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, VIR_FREE(host); VIR_FREE(tapfdName); VIR_FREE(vhostfdName); - virObjectUnref(cfg); return ret; } @@ -8215,8 +8218,9 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, * API domainSetSecurityTapFDLabel that doesn't use the const format. */ static int -qemuBuildNetCommandLine(virCommandPtr cmd, - virQEMUDriverPtr driver, +qemuBuildNetCommandLine(virQEMUDriverPtr driver, + virLogManagerPtr logManager, + virCommandPtr cmd, virDomainDefPtr def, virQEMUCapsPtr qemuCaps, virNetDevVPortProfileOp vmop, @@ -8254,7 +8258,7 @@ qemuBuildNetCommandLine(virCommandPtr cmd, else vlan = i; - if (qemuBuildInterfaceCommandLine(cmd, driver, def, net, + if (qemuBuildInterfaceCommandLine(driver, logManager, cmd, def, net, qemuCaps, vlan, bootNet, vmop, standalone, nnicindexes, nicindexes) < 0) @@ -9481,7 +9485,8 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, if (qemuBuildFSDevCommandLine(cmd, def, qemuCaps) < 0) goto error; - if (qemuBuildNetCommandLine(cmd, driver, def, qemuCaps, vmop, standalone, + if (qemuBuildNetCommandLine(driver, logManager, cmd, def, + qemuCaps, vmop, standalone, nnicindexes, nicindexes, &bootHostdevNet) < 0) goto error; -- 2.8.4
On 10/06/2016 09:38 AM, Michal Privoznik wrote:
Instead of various NULL arguments, we can pass proper qemu driver config, log manager, and virCommand.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-)
This needs to be merged with patch 10 see note below ACK w/ the merged patches - let's avoid a round of ATTRIBUTE_NONNULL John
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 246ffe0..895bb23 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7822,12 +7822,15 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg, }
static int -qemuBuildVhostuserCommandLine(virCommandPtr cmd, +qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver, + virLogManagerPtr logManager, + virCommandPtr cmd, virDomainDefPtr def, virDomainNetDefPtr net, virQEMUCapsPtr qemuCaps, unsigned int bootindex) { + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); char *chardev = NULL; virBuffer netdev_buf = VIR_BUFFER_INITIALIZER; unsigned int queues = net->driver.virtio.queues; @@ -7841,7 +7844,7 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd,
switch ((virDomainChrType) net->data.vhostuser->type) { case VIR_DOMAIN_CHR_TYPE_UNIX: - if (!(chardev = qemuBuildChrChardevStr(NULL, NULL, NULL, def, + if (!(chardev = qemuBuildChrChardevStr(logManager, cmd, cfg, def, net->data.vhostuser, net->info.alias, qemuCaps, false))) goto error; @@ -7895,10 +7898,12 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd,
virCommandAddArgList(cmd, "-device", nic, NULL); VIR_FREE(nic); + virObjectUnref(cfg);
return 0;
error: + virObjectUnref(cfg); VIR_FREE(chardev); virBufferFreeAndReset(&netdev_buf); VIR_FREE(nic); @@ -7907,8 +7912,9 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd, }
static int -qemuBuildInterfaceCommandLine(virCommandPtr cmd, - virQEMUDriverPtr driver, +qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, + virLogManagerPtr logManager, + virCommandPtr cmd, virDomainDefPtr def, virDomainNetDefPtr net, virQEMUCapsPtr qemuCaps, @@ -7928,7 +7934,6 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, char **tapfdName = NULL; char **vhostfdName = NULL; virDomainNetType actualType = virDomainNetGetActualType(net); - virQEMUDriverConfigPtr cfg = NULL;
NOTE: Ironically I had suggested removing cfg in patch 4. You can either do it then or now, it doesn't matter!
virNetDevBandwidthPtr actualBandwidth; size_t i;
@@ -7971,8 +7976,6 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, return -1; }
- cfg = virQEMUDriverGetConfig(driver); - switch (actualType) { case VIR_DOMAIN_NET_TYPE_NETWORK: case VIR_DOMAIN_NET_TYPE_BRIDGE: @@ -8032,7 +8035,8 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, break;
case VIR_DOMAIN_NET_TYPE_VHOSTUSER: - ret = qemuBuildVhostuserCommandLine(cmd, def, net, qemuCaps, bootindex); + ret = qemuBuildVhostuserCommandLine(driver, logManager, cmd, def, + net, qemuCaps, bootindex); goto cleanup; break;
@@ -8205,7 +8209,6 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, VIR_FREE(host); VIR_FREE(tapfdName); VIR_FREE(vhostfdName); - virObjectUnref(cfg); return ret; }
@@ -8215,8 +8218,9 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, * API domainSetSecurityTapFDLabel that doesn't use the const format. */ static int -qemuBuildNetCommandLine(virCommandPtr cmd, - virQEMUDriverPtr driver, +qemuBuildNetCommandLine(virQEMUDriverPtr driver, + virLogManagerPtr logManager, + virCommandPtr cmd, virDomainDefPtr def, virQEMUCapsPtr qemuCaps, virNetDevVPortProfileOp vmop, @@ -8254,7 +8258,7 @@ qemuBuildNetCommandLine(virCommandPtr cmd, else vlan = i;
- if (qemuBuildInterfaceCommandLine(cmd, driver, def, net, + if (qemuBuildInterfaceCommandLine(driver, logManager, cmd, def, net, qemuCaps, vlan, bootNet, vmop, standalone, nnicindexes, nicindexes) < 0) @@ -9481,7 +9485,8 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, if (qemuBuildFSDevCommandLine(cmd, def, qemuCaps) < 0) goto error;
- if (qemuBuildNetCommandLine(cmd, driver, def, qemuCaps, vmop, standalone, + if (qemuBuildNetCommandLine(driver, logManager, cmd, def, + qemuCaps, vmop, standalone, nnicindexes, nicindexes, &bootHostdevNet) < 0) goto error;
Currently, what we do for vhost-user network is generate the following part of command line: -netdev type=vhost-user,id=hostnet0,chardev=charnet0 There's no need for 'type=' it is the default. Drop it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 2 +- tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-multiq.args | 6 +++--- tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.args | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 895bb23..e8d4d8d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7869,7 +7869,7 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver, goto error; } - virBufferAsprintf(&netdev_buf, "type=vhost-user,id=host%s,chardev=char%s", + virBufferAsprintf(&netdev_buf, "vhost-user,id=host%s,chardev=char%s", net->info.alias, net->info.alias); if (queues > 1) { diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-multiq.args b/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-multiq.args index bab15ad..4360e5e 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-multiq.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-multiq.args @@ -20,17 +20,17 @@ QEMU_AUDIO_DRV=none \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ -chardev socket,id=charnet0,path=/tmp/vhost0.sock,server \ --netdev type=vhost-user,id=hostnet0,chardev=charnet0 \ +-netdev vhost-user,id=hostnet0,chardev=charnet0 \ -device virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:ee:96:6b,bus=pci.0,\ addr=0x3 \ -chardev socket,id=charnet1,path=/tmp/vhost1.sock \ --netdev type=vhost-user,id=hostnet1,chardev=charnet1 \ +-netdev vhost-user,id=hostnet1,chardev=charnet1 \ -device virtio-net-pci,netdev=hostnet1,id=net1,mac=52:54:00:ee:96:6c,bus=pci.0,\ addr=0x4 \ -netdev socket,listen=:2015,id=hostnet2 \ -device rtl8139,netdev=hostnet2,id=net2,mac=52:54:00:95:db:c0,bus=pci.0,\ addr=0x5 \ -chardev socket,id=charnet3,path=/tmp/vhost2.sock \ --netdev type=vhost-user,id=hostnet3,chardev=charnet3,queues=4 \ +-netdev vhost-user,id=hostnet3,chardev=charnet3,queues=4 \ -device virtio-net-pci,mq=on,vectors=10,netdev=hostnet3,id=net3,\ mac=52:54:00:ee:96:6d,bus=pci.0,addr=0x6 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.args b/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.args index ce8d669..47c1d84 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.args @@ -20,11 +20,11 @@ QEMU_AUDIO_DRV=none \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ -chardev socket,id=charnet0,path=/tmp/vhost0.sock,server \ --netdev type=vhost-user,id=hostnet0,chardev=charnet0 \ +-netdev vhost-user,id=hostnet0,chardev=charnet0 \ -device virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:ee:96:6b,bus=pci.0,\ addr=0x3 \ -chardev socket,id=charnet1,path=/tmp/vhost1.sock \ --netdev type=vhost-user,id=hostnet1,chardev=charnet1 \ +-netdev vhost-user,id=hostnet1,chardev=charnet1 \ -device virtio-net-pci,netdev=hostnet1,id=net1,mac=52:54:00:ee:96:6c,bus=pci.0,\ addr=0x4 \ -netdev socket,listen=:2015,id=hostnet2 \ -- 2.8.4
On 10/06/2016 09:38 AM, Michal Privoznik wrote:
Currently, what we do for vhost-user network is generate the following part of command line:
-netdev type=vhost-user,id=hostnet0,chardev=charnet0
There's no need for 'type=' it is the default. Drop it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 2 +- tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-multiq.args | 6 +++--- tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.args | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-)
Still an ACK, John
https://bugzilla.redhat.com/show_bug.cgi?id=1366505 So far, this function lacked support for VIR_DOMAIN_NET_TYPE_VHOSTUSER leaving callers to hack around the problem by constructing the command line on their own. This is not ideal as it blocks hot plug support. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 38 +++++++++++++--------- .../qemuxml2argv-net-vhostuser-multiq.args | 6 ++-- .../qemuxml2argv-net-vhostuser.args | 4 +-- 3 files changed, 28 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e8d4d8d..95e40de 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3721,7 +3721,13 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, return NULL; case VIR_DOMAIN_NET_TYPE_VHOSTUSER: - /* Unsupported yet. */ + virBufferAsprintf(&buf, "vhost-user%cchardev=char%s", + type_sep, + net->info.alias); + type_sep = ','; + if (net->driver.virtio.queues > 1) + virBufferAsprintf(&buf, ",queues=%u", + net->driver.virtio.queues); break; case VIR_DOMAIN_NET_TYPE_LAST: @@ -7832,7 +7838,7 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver, { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); char *chardev = NULL; - virBuffer netdev_buf = VIR_BUFFER_INITIALIZER; + char *netdev = NULL; unsigned int queues = net->driver.virtio.queues; char *nic = NULL; @@ -7869,25 +7875,27 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver, goto error; } - virBufferAsprintf(&netdev_buf, "vhost-user,id=host%s,chardev=char%s", - net->info.alias, net->info.alias); - - if (queues > 1) { - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VHOSTUSER_MULTIQUEUE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("multi-queue is not supported for vhost-user " - "with this QEMU binary")); - goto error; - } - virBufferAsprintf(&netdev_buf, ",queues=%u", queues); + if (queues > 1 && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VHOSTUSER_MULTIQUEUE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("multi-queue is not supported for vhost-user " + "with this QEMU binary")); + goto error; } + if (!(netdev = qemuBuildHostNetStr(net, driver, + ',', -1, + NULL, 0, NULL, 0))) + goto error; + + virCommandAddArg(cmd, "-chardev"); virCommandAddArg(cmd, chardev); VIR_FREE(chardev); virCommandAddArg(cmd, "-netdev"); - virCommandAddArgBuffer(cmd, &netdev_buf); + virCommandAddArg(cmd, netdev); + VIR_FREE(netdev); if (!(nic = qemuBuildNicDevStr(def, net, -1, bootindex, queues, qemuCaps))) { @@ -7904,8 +7912,8 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver, error: virObjectUnref(cfg); + VIR_FREE(netdev); VIR_FREE(chardev); - virBufferFreeAndReset(&netdev_buf); VIR_FREE(nic); return -1; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-multiq.args b/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-multiq.args index 4360e5e..59929c1 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-multiq.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-multiq.args @@ -20,17 +20,17 @@ QEMU_AUDIO_DRV=none \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ -chardev socket,id=charnet0,path=/tmp/vhost0.sock,server \ --netdev vhost-user,id=hostnet0,chardev=charnet0 \ +-netdev vhost-user,chardev=charnet0,id=hostnet0 \ -device virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:ee:96:6b,bus=pci.0,\ addr=0x3 \ -chardev socket,id=charnet1,path=/tmp/vhost1.sock \ --netdev vhost-user,id=hostnet1,chardev=charnet1 \ +-netdev vhost-user,chardev=charnet1,id=hostnet1 \ -device virtio-net-pci,netdev=hostnet1,id=net1,mac=52:54:00:ee:96:6c,bus=pci.0,\ addr=0x4 \ -netdev socket,listen=:2015,id=hostnet2 \ -device rtl8139,netdev=hostnet2,id=net2,mac=52:54:00:95:db:c0,bus=pci.0,\ addr=0x5 \ -chardev socket,id=charnet3,path=/tmp/vhost2.sock \ --netdev vhost-user,id=hostnet3,chardev=charnet3,queues=4 \ +-netdev vhost-user,chardev=charnet3,queues=4,id=hostnet3 \ -device virtio-net-pci,mq=on,vectors=10,netdev=hostnet3,id=net3,\ mac=52:54:00:ee:96:6d,bus=pci.0,addr=0x6 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.args b/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.args index 47c1d84..e15d735 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.args @@ -20,11 +20,11 @@ QEMU_AUDIO_DRV=none \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ -chardev socket,id=charnet0,path=/tmp/vhost0.sock,server \ --netdev vhost-user,id=hostnet0,chardev=charnet0 \ +-netdev vhost-user,chardev=charnet0,id=hostnet0 \ -device virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:ee:96:6b,bus=pci.0,\ addr=0x3 \ -chardev socket,id=charnet1,path=/tmp/vhost1.sock \ --netdev vhost-user,id=hostnet1,chardev=charnet1 \ +-netdev vhost-user,chardev=charnet1,id=hostnet1 \ -device virtio-net-pci,netdev=hostnet1,id=net1,mac=52:54:00:ee:96:6c,bus=pci.0,\ addr=0x4 \ -netdev socket,listen=:2015,id=hostnet2 \ -- 2.8.4
On 10/06/2016 09:38 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1366505
So far, this function lacked support for VIR_DOMAIN_NET_TYPE_VHOSTUSER leaving callers to hack around the problem by constructing the command line on their own. This is not ideal as it blocks hot plug support.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 38 +++++++++++++--------- .../qemuxml2argv-net-vhostuser-multiq.args | 6 ++-- .../qemuxml2argv-net-vhostuser.args | 4 +-- 3 files changed, 28 insertions(+), 20 deletions(-)
ACK still applies, John
https://bugzilla.redhat.com/show_bug.cgi?id=1366108 There are couple of things that needs to be done in order to allow vhost-user hotplug. Firstly, vhost-user requires a chardev which is connected to vhost-user bridge and through which qemu communicates with the bridge (no acutal guest traffic is sent through there, just some metadata). In order to generate proper chardev alias, we must assign device alias way sooner. Then, because we are plugging the chardev first, we need to do the proper undo if something fails - that is remove netdev too. We don't want anything to be left over in case attach fails at some point. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 71 +++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 60 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1b2a075..14af4e1 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -932,6 +932,10 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virDomainCCWAddressSetPtr ccwaddrs = NULL; size_t i; + char *charDevAlias = NULL; + bool charDevPlugged = false; + bool netdevPlugged = false; + bool hostPlugged = false; /* preallocate new slot for device */ if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets + 1) < 0) @@ -970,6 +974,9 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, return -1; } + if (qemuAssignDeviceNetAlias(vm->def, net, -1) < 0) + goto cleanup; + switch (actualType) { case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_NETWORK: @@ -1044,8 +1051,18 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, goto cleanup; break; - case VIR_DOMAIN_NET_TYPE_USER: case VIR_DOMAIN_NET_TYPE_VHOSTUSER: + if (!qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Netdev support unavailable")); + goto cleanup; + } + + if (virAsprintf(&charDevAlias, "char%s", net->info.alias) < 0) + goto cleanup; + break; + + case VIR_DOMAIN_NET_TYPE_USER: case VIR_DOMAIN_NET_TYPE_SERVER: case VIR_DOMAIN_NET_TYPE_CLIENT: case VIR_DOMAIN_NET_TYPE_MCAST: @@ -1081,9 +1098,6 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, goto cleanup; } - if (qemuAssignDeviceNetAlias(vm->def, net, -1) < 0) - goto cleanup; - if (qemuDomainMachineIsS390CCW(vm->def) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW)) { net->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW; @@ -1143,23 +1157,36 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, } qemuDomainObjEnterMonitor(driver, vm); + + if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) { + if (qemuMonitorAttachCharDev(priv->mon, charDevAlias, net->data.vhostuser) < 0) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + virDomainAuditNet(vm, NULL, net, "attach", false); + goto cleanup; + } + charDevPlugged = true; + } + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) { if (qemuMonitorAddNetdev(priv->mon, netstr, tapfd, tapfdName, tapfdSize, vhostfd, vhostfdName, vhostfdSize) < 0) { ignore_value(qemuDomainObjExitMonitor(driver, vm)); virDomainAuditNet(vm, NULL, net, "attach", false); - goto cleanup; + goto try_remove; } + netdevPlugged = true; } else { if (qemuMonitorAddHostNetwork(priv->mon, netstr, tapfd, tapfdName, tapfdSize, vhostfd, vhostfdName, vhostfdSize) < 0) { ignore_value(qemuDomainObjExitMonitor(driver, vm)); virDomainAuditNet(vm, NULL, net, "attach", false); - goto cleanup; + goto try_remove; } + hostPlugged = true; } + if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; @@ -1262,6 +1289,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, } VIR_FREE(vhostfd); VIR_FREE(vhostfdName); + VIR_FREE(charDevAlias); virObjectUnref(cfg); virDomainCCWAddressSetFree(ccwaddrs); @@ -1277,7 +1305,11 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, if (virAsprintf(&netdev_name, "host%s", net->info.alias) < 0) goto cleanup; qemuDomainObjEnterMonitor(driver, vm); - if (qemuMonitorRemoveNetdev(priv->mon, netdev_name) < 0) + if (charDevPlugged && + qemuMonitorDetachCharDev(priv->mon, charDevAlias) < 0) + VIR_WARN("Failed to remove associated chardev %s", charDevAlias); + if (netdevPlugged && + qemuMonitorRemoveNetdev(priv->mon, netdev_name) < 0) VIR_WARN("Failed to remove network backend for netdev %s", netdev_name); ignore_value(qemuDomainObjExitMonitor(driver, vm)); @@ -1290,7 +1322,8 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, if (virAsprintf(&hostnet_name, "host%s", net->info.alias) < 0) goto cleanup; qemuDomainObjEnterMonitor(driver, vm); - if (qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name) < 0) + if (hostPlugged && + qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name) < 0) VIR_WARN("Failed to remove network backend for vlan %d, net %s", vlan, hostnet_name); ignore_value(qemuDomainObjExitMonitor(driver, vm)); @@ -3320,10 +3353,12 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, virNetDevVPortProfilePtr vport; virObjectEventPtr event; char *hostnet_name = NULL; + char *charDevAlias = NULL; size_t i; int ret = -1; + int actualType = virDomainNetGetActualType(net); - if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { /* this function handles all hostdev and netdev cleanup */ ret = qemuDomainRemoveHostDevice(driver, vm, virDomainNetGetActualHostdev(net)); @@ -3333,9 +3368,11 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, VIR_DEBUG("Removing network interface %s from domain %p %s", net->info.alias, vm, vm->def->name); - if (virAsprintf(&hostnet_name, "host%s", net->info.alias) < 0) + if (virAsprintf(&hostnet_name, "host%s", net->info.alias) < 0 || + virAsprintf(&charDevAlias, "char%s", net->info.alias) < 0) goto cleanup; + qemuDomainObjEnterMonitor(driver, vm); if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) { if (qemuMonitorRemoveNetdev(priv->mon, hostnet_name) < 0) { @@ -3358,6 +3395,17 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, goto cleanup; } } + + if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) { + /* vhostuser has a chardev too */ + if (qemuMonitorDetachCharDev(priv->mon, charDevAlias) < 0) { + /* well, this is a messy situation. Guest visible PCI device has + * been removed, netdev too but chardev not. The best seems to be + * to just ignore the error and carry on. + */ + } + } + if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; @@ -3382,7 +3430,7 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, &net->mac)); } - if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT) { + if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { ignore_value(virNetDevMacVLanDeleteWithVPortProfile( net->ifname, &net->mac, virDomainNetGetActualDirectDev(net), @@ -3408,6 +3456,7 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, cleanup: virObjectUnref(cfg); + VIR_FREE(charDevAlias); VIR_FREE(hostnet_name); return ret; } -- 2.8.4
On 10/06/2016 09:39 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1366108
There are couple of things that needs to be done in order to
s/needs/need/
allow vhost-user hotplug. Firstly, vhost-user requires a chardev which is connected to vhost-user bridge and through which qemu communicates with the bridge (no acutal guest traffic is sent
s/acutal/actual/
through there, just some metadata). In order to generate proper chardev alias, we must assign device alias way sooner.
Then, because we are plugging the chardev first, we need to do the proper undo if something fails - that is remove netdev too. We don't want anything to be left over in case attach fails at some point.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 71 +++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 60 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1b2a075..14af4e1 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -932,6 +932,10 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virDomainCCWAddressSetPtr ccwaddrs = NULL; size_t i; + char *charDevAlias = NULL; + bool charDevPlugged = false; + bool netdevPlugged = false; + bool hostPlugged = false;
/* preallocate new slot for device */ if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets + 1) < 0) @@ -970,6 +974,9 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, return -1; }
+ if (qemuAssignDeviceNetAlias(vm->def, net, -1) < 0) + goto cleanup; + switch (actualType) { case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_NETWORK: @@ -1044,8 +1051,18 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, goto cleanup; break;
- case VIR_DOMAIN_NET_TYPE_USER: case VIR_DOMAIN_NET_TYPE_VHOSTUSER: + if (!qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Netdev support unavailable")); + goto cleanup; + } + + if (virAsprintf(&charDevAlias, "char%s", net->info.alias) < 0) + goto cleanup; + break; + + case VIR_DOMAIN_NET_TYPE_USER: case VIR_DOMAIN_NET_TYPE_SERVER: case VIR_DOMAIN_NET_TYPE_CLIENT: case VIR_DOMAIN_NET_TYPE_MCAST: @@ -1081,9 +1098,6 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, goto cleanup; }
- if (qemuAssignDeviceNetAlias(vm->def, net, -1) < 0) - goto cleanup; - if (qemuDomainMachineIsS390CCW(vm->def) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW)) { net->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW; @@ -1143,23 +1157,36 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, }
qemuDomainObjEnterMonitor(driver, vm); + + if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) { + if (qemuMonitorAttachCharDev(priv->mon, charDevAlias, net->data.vhostuser) < 0) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + virDomainAuditNet(vm, NULL, net, "attach", false); + goto cleanup; + } + charDevPlugged = true; + } + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) { if (qemuMonitorAddNetdev(priv->mon, netstr, tapfd, tapfdName, tapfdSize, vhostfd, vhostfdName, vhostfdSize) < 0) { ignore_value(qemuDomainObjExitMonitor(driver, vm)); virDomainAuditNet(vm, NULL, net, "attach", false); - goto cleanup; + goto try_remove; } + netdevPlugged = true; } else { if (qemuMonitorAddHostNetwork(priv->mon, netstr, tapfd, tapfdName, tapfdSize, vhostfd, vhostfdName, vhostfdSize) < 0) { ignore_value(qemuDomainObjExitMonitor(driver, vm)); virDomainAuditNet(vm, NULL, net, "attach", false); - goto cleanup; + goto try_remove; } + hostPlugged = true; } + if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup;
@@ -1262,6 +1289,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, } VIR_FREE(vhostfd); VIR_FREE(vhostfdName); + VIR_FREE(charDevAlias); virObjectUnref(cfg); virDomainCCWAddressSetFree(ccwaddrs);
@@ -1277,7 +1305,11 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, if (virAsprintf(&netdev_name, "host%s", net->info.alias) < 0) goto cleanup; qemuDomainObjEnterMonitor(driver, vm); - if (qemuMonitorRemoveNetdev(priv->mon, netdev_name) < 0) + if (charDevPlugged && + qemuMonitorDetachCharDev(priv->mon, charDevAlias) < 0)
Adding this wasn't within the NETDEV specific code, so removing it shouldn't be either...
+ VIR_WARN("Failed to remove associated chardev %s", charDevAlias); + if (netdevPlugged && + qemuMonitorRemoveNetdev(priv->mon, netdev_name) < 0) VIR_WARN("Failed to remove network backend for netdev %s", netdev_name); ignore_value(qemuDomainObjExitMonitor(driver, vm)); @@ -1290,7 +1322,8 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, if (virAsprintf(&hostnet_name, "host%s", net->info.alias) < 0) goto cleanup; qemuDomainObjEnterMonitor(driver, vm); - if (qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name) < 0) + if (hostPlugged && + qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name) < 0) VIR_WARN("Failed to remove network backend for vlan %d, net %s", vlan, hostnet_name); ignore_value(qemuDomainObjExitMonitor(driver, vm));
BTW: I know it's existing, but I think the whole think can be simplified... The only way to have any of those booleans defined to true is if something was success, so why not (starting at vlan < 0): char *alias = NULL; <== this would be at the top... and the VIR_FREE(alias) would be in cleanup: if (virAsprintf(&alias, "host%s", net->info.alias) < 0) goto cleanup; qemuDomainObjEnterMonitor(driver, vm); if (netdevPlugged && ...) if (hostPlugged && ...) -> The error messages could be made generic - add a vlan=%d - who really cares if it shows up -1, then at least you know which path it was). if (charDevPlugged && ...) ignore_value(qemuDomainObjExitMonitor(driver, vm)); goto cleanup; and yes the move of chardevPlugged was intentional... it was added, so remove it last. and the VIR_WARN("Unable to remove network backend"); can go away...
@@ -3320,10 +3353,12 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, virNetDevVPortProfilePtr vport; virObjectEventPtr event; char *hostnet_name = NULL; + char *charDevAlias = NULL; size_t i; int ret = -1; + int actualType = virDomainNetGetActualType(net);
- if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { /* this function handles all hostdev and netdev cleanup */ ret = qemuDomainRemoveHostDevice(driver, vm, virDomainNetGetActualHostdev(net)); @@ -3333,9 +3368,11 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, VIR_DEBUG("Removing network interface %s from domain %p %s", net->info.alias, vm, vm->def->name);
- if (virAsprintf(&hostnet_name, "host%s", net->info.alias) < 0) + if (virAsprintf(&hostnet_name, "host%s", net->info.alias) < 0 || + virAsprintf(&charDevAlias, "char%s", net->info.alias) < 0) goto cleanup;
+ qemuDomainObjEnterMonitor(driver, vm); if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) { if (qemuMonitorRemoveNetdev(priv->mon, hostnet_name) < 0) { @@ -3358,6 +3395,17 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, goto cleanup; } } + + if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) { + /* vhostuser has a chardev too */ + if (qemuMonitorDetachCharDev(priv->mon, charDevAlias) < 0) { + /* well, this is a messy situation. Guest visible PCI device has + * been removed, netdev too but chardev not. The best seems to be + * to just ignore the error and carry on. + */
I'd say - keep the comment, but just make this an ignore_value(); encased call (qemuDomainRemoveDiskDevice does so for objAlias and encAlias) ACK - although I would prefer that the try_remove code in qemuDomainAttachNetDevice is cleaned up, it's not "contingent" on the ACK. The changes to the commit message and just going with the ignore_value() would be. John
+ } + } + if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup;
@@ -3382,7 +3430,7 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, &net->mac)); }
- if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT) { + if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { ignore_value(virNetDevMacVLanDeleteWithVPortProfile( net->ifname, &net->mac, virDomainNetGetActualDirectDev(net), @@ -3408,6 +3456,7 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
cleanup: virObjectUnref(cfg); + VIR_FREE(charDevAlias); VIR_FREE(hostnet_name); return ret; }
On 06.10.2016 21:38, Michal Privoznik wrote:
v2 of:
https://www.redhat.com/archives/libvir-list/2016-August/msg00806.html
The first patches are mostly code movement and cleanups.
diff to v1: - Hopefully all John's review suggestions are worked in now
Michal Privoznik (14): virDomainNetDefParseXML: Realign virDomainNetGetActualType: Return type is virDomainNetType qemuBuildInterfaceCommandLine: Move hostdev handling a bit further qemuBuildInterfaceCommandLine: Move vhostuser handling a bit further qemuBuildInterfaceCommandLine: Move from if-else forest to switch qemuDomainAttachNetDevice: Move hostdev handling a bit further qemuDomainAttachNetDevice: Explicitly list allowed types for hotplug qemuBuildHostNetStr: Explicitly enumerate net types qemuBuildChrChardevStr: Introduce @nowait argument qemuBuildVhostuserCommandLine: Reuse qemuBuildChrChardevStr qemuBuildInterfaceCommandLine: Pass proper args qemuBuildVhostuserCommandLine: Unify -netdev creation qemuBuildHostNetStr: Support VIR_DOMAIN_NET_TYPE_VHOSTUSER qemu_hotplug: Support interface type of vhost-user hotplug
src/bhyve/bhyve_command.c | 2 +- src/bhyve/bhyve_process.c | 2 +- src/conf/domain_conf.c | 16 +- src/conf/domain_conf.h | 2 +- src/libxl/libxl_domain.c | 2 +- src/libxl/libxl_driver.c | 2 +- src/lxc/lxc_driver.c | 9 +- src/qemu/qemu_command.c | 182 +++++++++++++-------- src/qemu/qemu_hotplug.c | 130 +++++++++++---- src/qemu/qemu_process.c | 13 +- .../qemuxml2argv-net-hostdev-fail.xml | 39 +++++ .../qemuxml2argv-net-vhostuser-fail.xml | 36 ++++ .../qemuxml2argv-net-vhostuser-multiq.args | 6 +- .../qemuxml2argv-net-vhostuser.args | 4 +- tests/qemuxml2argvtest.c | 7 + 15 files changed, 334 insertions(+), 118 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-hostdev-fail.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser-fail.xml
I've merged patches 10 a 11 as requested, and pushed these. Thank you John for the review! Michal
participants (2)
-
John Ferlan -
Michal Privoznik