[libvirt] [PATCH 0/9] Couple of vhost-user fixes and cleanups
While working on this, I've discovered a qemu crasher [1], so there are not tested properly. If you have pointers how to test vhost-user without need to install openvswitch, I'm all ears. 1: http://lists.nongnu.org/archive/html/qemu-devel/2016-08/msg02933.html Michal Privoznik (9): qemuDomainAttachNetDevice: Explicitly list allowed types for hotplug qemuDomain{Attach,Remove}NetDevice: Prefer qemuDomainSupportsNetdev qemuBuildChrChardevStr: Introduce @nowait argument qemuBuildVhostuserCommandLine: Reuse qemuBuildChrChardevStr qemuBuildHostNetStr: Realign qemuBuildHostNetStr: Explicitly enumerate net types qemuBuildVhostuserCommandLine: Unify -netdev creation qemuBuildHostNetStr: Support VIR_DOMAIN_NET_TYPE_VHOSTUSER qemu_hotplug: Support interface type of vhost-user hotplug src/qemu/qemu_command.c | 169 ++++++++++++--------- src/qemu/qemu_hotplug.c | 83 ++++++++-- .../qemuxml2argv-net-vhostuser-multiq.args | 6 +- .../qemuxml2argv-net-vhostuser.args | 4 +- 4 files changed, 176 insertions(+), 86 deletions(-) -- 2.8.4
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 | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 00e4a75..d1acdd9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -947,6 +947,30 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, actualType = virDomainNetGetActualType(net); + + switch ((virDomainNetType) actualType) { + case VIR_DOMAIN_NET_TYPE_NETWORK: + case VIR_DOMAIN_NET_TYPE_ETHERNET: + case VIR_DOMAIN_NET_TYPE_DIRECT: + case VIR_DOMAIN_NET_TYPE_BRIDGE: + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + /* These types are supported. */ + 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; + } + 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 -- 2.8.4
On 08/16/2016 11:41 AM, Michal Privoznik wrote:
Instead of blindly claim support for hot-plugging of every
claiming
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 | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
Part of me thinks - sure ACK this, but the other part says why not convert the "if - elseif - elseif" a few lines below to be essentially this switch? Then I wonder if Coverity would pick up on the HOSTDEV specific if and "flag" the switch case HOSTDEV: and complain? What would be the drawback to the switch option? John
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 00e4a75..d1acdd9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -947,6 +947,30 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
actualType = virDomainNetGetActualType(net);
+ + switch ((virDomainNetType) actualType) { + case VIR_DOMAIN_NET_TYPE_NETWORK: + case VIR_DOMAIN_NET_TYPE_ETHERNET: + case VIR_DOMAIN_NET_TYPE_DIRECT: + case VIR_DOMAIN_NET_TYPE_BRIDGE: + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + /* These types are supported. */ + 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; + } + 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
On 23.09.2016 00:47, John Ferlan wrote:
On 08/16/2016 11:41 AM, Michal Privoznik wrote:
Instead of blindly claim support for hot-plugging of every
claiming
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 | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
Part of me thinks - sure ACK this, but the other part says why not convert the "if - elseif - elseif" a few lines below to be essentially this switch?
Then I wonder if Coverity would pick up on the HOSTDEV specific if and "flag" the switch case HOSTDEV: and complain?
What would be the drawback to the switch option?
Good point, this could work. I'm gonna try this. Michal
Depending on domain OS type, and interface address type we might not want to use -netdev even though qemu has the capability. We should use more advanced check implemented in qemuDomainSupportsNetdev() function. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index d1acdd9..feb1f44 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1108,7 +1108,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, releaseaddr = true; - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) { + if (qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) { vlan = -1; } else { vlan = qemuDomainNetVLAN(net); @@ -1134,7 +1134,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, goto cleanup; } - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) { + if (qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) { if (!(netstr = qemuBuildHostNetStr(net, driver, ',', -1, tapfdName, tapfdSize, @@ -1149,7 +1149,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, } qemuDomainObjEnterMonitor(driver, vm); - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) { + if (qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) { if (qemuMonitorAddNetdev(priv->mon, netstr, tapfd, tapfdName, tapfdSize, vhostfd, vhostfdName, vhostfdSize) < 0) { @@ -1195,7 +1195,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, } else { qemuDomainObjEnterMonitor(driver, vm); - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) { + if (qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) { if (qemuMonitorSetLink(priv->mon, net->info.alias, VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN) < 0) { ignore_value(qemuDomainObjExitMonitor(driver, vm)); virDomainAuditNet(vm, NULL, net, "attach", false); @@ -1278,7 +1278,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, goto cleanup; if (vlan < 0) { - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) { + if (qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) { char *netdev_name; if (virAsprintf(&netdev_name, "host%s", net->info.alias) < 0) goto cleanup; @@ -3326,7 +3326,7 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, goto cleanup; qemuDomainObjEnterMonitor(driver, vm); - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) { + if (qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) { if (qemuMonitorRemoveNetdev(priv->mon, hostnet_name) < 0) { if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; -- 2.8.4
On 08/16/2016 11:41 AM, Michal Privoznik wrote:
Depending on domain OS type, and interface address type we might not want to use -netdev even though qemu has the capability. We should use more advanced check implemented in qemuDomainSupportsNetdev() function.
So the effect of using qemuDomainSupportsNetdev rather than a direct check of the capability is that aarch64 network devices that are using neither PCI nor virtio-mmio connections would get a different commandline. Is there a particular situation where this makes a difference, or did it just "look like the right thing to do"? I ask this because the more I look at qemuDomainSupportsNetdev() and qemuDomainSupportsNicdev(), the more I wonder if they're still accomplishing what was originally intended (or if what was originally intended is still necessary). qemuDomainSupportsNicdev() (called by qemuDomainSupportsNetdev()) says that its raison d'etre is "non-virtio ARM nics require legacy -net nic" But that's not really what's checked for; instead, it's checking if the network device has a virtio-mmio *or PCI* address - the result of this test for both virtio-net-pci and for emulated NICs (do they even work on aarch64? (e1000 for example - the qemu-system-aarch64 binary says it's compiled in, but has it actually been tested, or was it just "not taken out"?)) is the same, so virtio and emulated NICs will both get the netdev-type commandline. For that matter, by the time the post-parse is done, any interface device that hasn't had a manually-specified <address> element is going to have either type='pci' or type='virtio-mmio' (is an address type of virtio-mmio a valid thing for a non-virtio emulated device? I wouldn't think so, but I'm just going on common sense, not on any actual knowledge). And as far as I can see, the only network device that can even use any address type other than virtio-mmio or PCI is "usb-net", which requires <address type='usb' .../>. (I looked back through the git history a bit, and that function was added prior to support for PCI on ARM (when support for virtio-mmio addressing was added - see commits 54a77c and 4fa172), and it looks like it was intended to force the use of -net rather than -netdev when virtio-mmio addressing isn't used. But before virtio-mmio and support for PCI addresses, how were these devices connected in the guest? And now is it even a useful thing at all, or is it obsolete? Cole?
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index d1acdd9..feb1f44 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1108,7 +1108,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
releaseaddr = true;
- if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) { + if (qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) { vlan = -1; } else { vlan = qemuDomainNetVLAN(net); @@ -1134,7 +1134,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, goto cleanup; }
- if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) { + if (qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) { if (!(netstr = qemuBuildHostNetStr(net, driver, ',', -1, tapfdName, tapfdSize, @@ -1149,7 +1149,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, }
qemuDomainObjEnterMonitor(driver, vm); - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) { + if (qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) { if (qemuMonitorAddNetdev(priv->mon, netstr, tapfd, tapfdName, tapfdSize, vhostfd, vhostfdName, vhostfdSize) < 0) { @@ -1195,7 +1195,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, } else { qemuDomainObjEnterMonitor(driver, vm);
- if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) { + if (qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) { if (qemuMonitorSetLink(priv->mon, net->info.alias, VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN) < 0) { ignore_value(qemuDomainObjExitMonitor(driver, vm)); virDomainAuditNet(vm, NULL, net, "attach", false); @@ -1278,7 +1278,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, goto cleanup;
if (vlan < 0) { - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) { + if (qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) { char *netdev_name; if (virAsprintf(&netdev_name, "host%s", net->info.alias) < 0) goto cleanup; @@ -3326,7 +3326,7 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, goto cleanup;
qemuDomainObjEnterMonitor(driver, vm); - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) { + if (qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) { if (qemuMonitorRemoveNetdev(priv->mon, hostnet_name) < 0) { if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup;
On 29.08.2016 06:52, Laine Stump wrote:
On 08/16/2016 11:41 AM, Michal Privoznik wrote:
Depending on domain OS type, and interface address type we might not want to use -netdev even though qemu has the capability. We should use more advanced check implemented in qemuDomainSupportsNetdev() function.
So the effect of using qemuDomainSupportsNetdev rather than a direct check of the capability is that aarch64 network devices that are using neither PCI nor virtio-mmio connections would get a different commandline. Is there a particular situation where this makes a difference, or did it just "look like the right thing to do"?
The latter one. When working on later patches from this patchset I've spotted this inconsistency. I'm okay with dropping this one if it causes confusion though. Michal
On 08/16/2016 11:41 AM, Michal Privoznik wrote:
Depending on domain OS type, and interface address type we might not want to use -netdev even though qemu has the capability. We should use more advanced check implemented in qemuDomainSupportsNetdev() function.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
Seems to me part of the goal here is to follow the decision points that qemu_command.c would make in qemuBuildInterfaceCommandLine, but that logic isn't all that clear either. If it helps, I'm in favor of the change - although I have a couple of concerns (and per usual I left some thoughts along the way)... it's a weak ACK, but let's see what you/Laine think too John
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index d1acdd9..feb1f44 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1108,7 +1108,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
releaseaddr = true;
- if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) { + if (qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) {
same check as qemuBuildNetCommandLine, so this looks right.
vlan = -1; } else { vlan = qemuDomainNetVLAN(net); @@ -1134,7 +1134,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, goto cleanup; }
- if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) { + if (qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) { if (!(netstr = qemuBuildHostNetStr(net, driver, ',', -1,
FWIW: That -1 is also 'vlan' value *if* qemuDomainSupportsNetdev is used and true. If you follow the qemu_command logic, it too would pass -1 in the vlan to qemuBuildInterfaceCommandLine. So "theoretically" we're still on par with qemu_command processing... The else portion of the hotplug code doesn't seem to match the qemu_command code's !qemuDomainSupportsNetdev and qemuBuildHostNetStr call though (' ' vs. ','). I know - separate issue, but I figured I'd point it out at least.
tapfdName, tapfdSize, @@ -1149,7 +1149,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, }
qemuDomainObjEnterMonitor(driver, vm); - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) { + if (qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) {
This particular check/usage isn't as clear to me at least with respect to comparing to qemu_command processing of "-device" and hotplug usage of netdev_add. As for the else condition, it's qemu_command usage of "-net" and (text only) hotplug usage of host_net_add. As long as those are "equals", then this check seems to pass muster too... At least w/r/t how qemu_command does things.
if (qemuMonitorAddNetdev(priv->mon, netstr, tapfd, tapfdName, tapfdSize, vhostfd, vhostfdName, vhostfdSize) < 0) { @@ -1195,7 +1195,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, } else { qemuDomainObjEnterMonitor(driver, vm);
- if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) { + if (qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) {
This would seemingly be OK
if (qemuMonitorSetLink(priv->mon, net->info.alias, VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN) < 0) { ignore_value(qemuDomainObjExitMonitor(driver, vm)); virDomainAuditNet(vm, NULL, net, "attach", false); @@ -1278,7 +1278,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, goto cleanup;
if (vlan < 0) { - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) { + if (qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) {
This would match, error code...
char *netdev_name; if (virAsprintf(&netdev_name, "host%s", net->info.alias) < 0) goto cleanup; @@ -3326,7 +3326,7 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, goto cleanup;
qemuDomainObjEnterMonitor(driver, vm); - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) { + if (qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) {
and this would be the corollary to our Attach which seems OK.
if (qemuMonitorRemoveNetdev(priv->mon, hostnet_name) < 0) { if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup;
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 | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ebedaef..c419ecf 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4867,7 +4867,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; @@ -4940,19 +4941,20 @@ 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); break; case VIR_DOMAIN_CHR_TYPE_UNIX: 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: @@ -5276,7 +5278,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); @@ -5434,7 +5436,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; } @@ -8376,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; } @@ -8490,7 +8492,7 @@ qemuBuildShmemBackendStr(virLogManagerPtr logManager, devstr = qemuBuildChrChardevStr(logManager, cmd, cfg, def, &shmem->server.chr, - shmem->info.alias, qemuCaps); + shmem->info.alias, qemuCaps, true); return devstr; } @@ -8568,7 +8570,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 +8609,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 +8655,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 +8678,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 +8721,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 +8735,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 +8864,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 08/16/2016 11:41 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 | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-)
This one had some merge conflicts with some changes I made - not difficult to handle thankfully... Of course it caused me to notice some bad indention in my code too <sigh> These changes look fine to me though, with one question... Considering my changes were to add the ability to use TLS for a TCP chardev via 'tls-creds' - how does that play with your next change to add a 'vhost-user' chardev option? OK beyond what will be some very obvious issues with usage of NULL parameters. IOW: Does one of these vhost-user chardevs "support" that "tls-creds" option? John
On 23.09.2016 00:50, John Ferlan wrote:
On 08/16/2016 11:41 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 | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-)
This one had some merge conflicts with some changes I made - not difficult to handle thankfully... Of course it caused me to notice some bad indention in my code too <sigh>
These changes look fine to me though, with one question...
Considering my changes were to add the ability to use TLS for a TCP chardev via 'tls-creds' - how does that play with your next change to add a 'vhost-user' chardev option? OK beyond what will be some very obvious issues with usage of NULL parameters.
Yeah, I might need to rework this.
IOW: Does one of these vhost-user chardevs "support" that "tls-creds" option?
Maybe. But I don't think it's used - the chardev sockets for vhost-user are just a control interface and are used just locally. Therefore I think TLS is not used here by anybody. Michal
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 c419ecf..59fcb4c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7848,7 +7848,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; @@ -7861,9 +7861,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: @@ -7881,7 +7882,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; } @@ -7899,7 +7900,8 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd, } virCommandAddArg(cmd, "-chardev"); - virCommandAddArgBuffer(cmd, &chardev_buf); + virCommandAddArg(cmd, chardev); + VIR_FREE(chardev); virCommandAddArg(cmd, "-netdev"); virCommandAddArgBuffer(cmd, &netdev_buf); @@ -7917,7 +7919,7 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd, return 0; error: - virBufferFreeAndReset(&chardev_buf); + VIR_FREE(chardev); virBufferFreeAndReset(&netdev_buf); VIR_FREE(nic); -- 2.8.4
On 08/16/2016 11:41 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(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c419ecf..59fcb4c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7848,7 +7848,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; @@ -7861,9 +7861,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,
logManager == NULL? cmd == NULL? cfg == NULL? if (dev->logfile) code will surely have problems (as will the TLS code) While true your narrow usage could avoid the TLS code that's going to dereference cfg without doing the obligatory "cfg &&", I'm still not clear how it would avoid the dev->logfile code. Shouldn't this support that logManager anyway? If not, then perhaps it needs to be more explicit. IOW: Extra checks. John
+ net->data.vhostuser, + net->info.alias, qemuCaps, false))) + goto error; break;
case VIR_DOMAIN_CHR_TYPE_NULL: @@ -7881,7 +7882,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; }
@@ -7899,7 +7900,8 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd, }
virCommandAddArg(cmd, "-chardev"); - virCommandAddArgBuffer(cmd, &chardev_buf); + virCommandAddArg(cmd, chardev); + VIR_FREE(chardev);
virCommandAddArg(cmd, "-netdev"); virCommandAddArgBuffer(cmd, &netdev_buf); @@ -7917,7 +7919,7 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd, return 0;
error: - virBufferFreeAndReset(&chardev_buf); + VIR_FREE(chardev); virBufferFreeAndReset(&netdev_buf); VIR_FREE(nic);
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 64 ++++++++++++++++++++++++------------------------- 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 59fcb4c..e6b3d9d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3598,11 +3598,11 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, } switch (netType) { - /* - * If type='bridge', and we're running as privileged user - * or -netdev bridge is not supported then it will fall - * through, -net tap,fd - */ + /* + * If type='bridge', and we're running as privileged user + * or -netdev bridge is not supported then it will fall + * through, -net tap,fd + */ case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_NETWORK: case VIR_DOMAIN_NET_TYPE_DIRECT: @@ -3625,39 +3625,39 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, break; case VIR_DOMAIN_NET_TYPE_CLIENT: - virBufferAsprintf(&buf, "socket%cconnect=%s:%d", - type_sep, - net->data.socket.address, - net->data.socket.port); - type_sep = ','; - break; + virBufferAsprintf(&buf, "socket%cconnect=%s:%d", + type_sep, + net->data.socket.address, + net->data.socket.port); + type_sep = ','; + break; case VIR_DOMAIN_NET_TYPE_SERVER: - virBufferAsprintf(&buf, "socket%clisten=%s:%d", - type_sep, - net->data.socket.address ? net->data.socket.address - : "", - net->data.socket.port); - type_sep = ','; - break; + virBufferAsprintf(&buf, "socket%clisten=%s:%d", + type_sep, + net->data.socket.address ? net->data.socket.address + : "", + net->data.socket.port); + type_sep = ','; + break; case VIR_DOMAIN_NET_TYPE_MCAST: - virBufferAsprintf(&buf, "socket%cmcast=%s:%d", - type_sep, - net->data.socket.address, - net->data.socket.port); - type_sep = ','; - break; + virBufferAsprintf(&buf, "socket%cmcast=%s:%d", + type_sep, + net->data.socket.address, + net->data.socket.port); + type_sep = ','; + break; case VIR_DOMAIN_NET_TYPE_UDP: - virBufferAsprintf(&buf, "socket%cudp=%s:%d,localaddr=%s:%d", - type_sep, - net->data.socket.address, - net->data.socket.port, - net->data.socket.localaddr, - net->data.socket.localport); - type_sep = ','; - break; + virBufferAsprintf(&buf, "socket%cudp=%s:%d,localaddr=%s:%d", + type_sep, + net->data.socket.address, + net->data.socket.port, + net->data.socket.localaddr, + net->data.socket.localport); + type_sep = ','; + break; case VIR_DOMAIN_NET_TYPE_USER: default: -- 2.8.4
On 08/16/2016 11:41 AM, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 64 ++++++++++++++++++++++++------------------------- 1 file changed, 32 insertions(+), 32 deletions(-)
ACK 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 e6b3d9d..12f3a6b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3660,9 +3660,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 08/16/2016 11:41 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(-)
I agree in principal, although this function is called by both qemu_command and qemu_hotplug...
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e6b3d9d..12f3a6b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3660,9 +3660,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. */
True for the hotplug code - it diverts the HOSTDEV call to qemuDomainAttachHostDevice... The qemu_command code would not call here and perhaps even could be considered an error. The problem with returning NULL here is if it *ever* happened, you'd get that failed for some reason error.
+ virObjectUnref(cfg); + return NULL; + + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: + /* Unsupported yet. */
So, could this too produce meaningless code? Although not called, so I suppose no big deal. (thinking while typing)...
+ break; + + case VIR_DOMAIN_NET_TYPE_LAST:
Similarly meaningless code, but would also be an error ostensibly wouldn't it? Why not move the HOSTDEV down here (with that same comment)... Generate a real error (invalid type=%d), do the Unref and return NULL, then call it a good. John
+ break; }
if (vlan >= 0) {
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 12f3a6b..34594b3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7898,7 +7898,7 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd, 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 08/16/2016 11:41 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(-)
Seems reasonable... and if you say so ;-) 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 | 43 +++++++++++++--------- .../qemuxml2argv-net-vhostuser-multiq.args | 6 +-- .../qemuxml2argv-net-vhostuser.args | 4 +- 3 files changed, 31 insertions(+), 22 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 34594b3..e3dc34a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3670,7 +3670,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: @@ -7854,14 +7860,15 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg, } static int -qemuBuildVhostuserCommandLine(virCommandPtr cmd, +qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver, + virCommandPtr cmd, virDomainDefPtr def, virDomainNetDefPtr net, virQEMUCapsPtr qemuCaps, unsigned int bootindex) { char *chardev = NULL; - virBuffer netdev_buf = VIR_BUFFER_INITIALIZER; + char *netdev = NULL; unsigned int queues = net->driver.virtio.queues; char *nic = NULL; @@ -7898,25 +7905,27 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd, 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))) { @@ -7931,8 +7940,8 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd, return 0; error: + VIR_FREE(netdev); VIR_FREE(chardev); - virBufferFreeAndReset(&netdev_buf); VIR_FREE(nic); return -1; @@ -7969,7 +7978,7 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, bootindex = net->info.bootIndex; if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) - return qemuBuildVhostuserCommandLine(cmd, def, net, qemuCaps, bootindex); + return qemuBuildVhostuserCommandLine(driver, cmd, def, net, qemuCaps, bootindex); if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { /* NET_TYPE_HOSTDEV devices are really hostdev devices, so 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 08/16/2016 11:41 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 | 43 +++++++++++++--------- .../qemuxml2argv-net-vhostuser-multiq.args | 6 +-- .../qemuxml2argv-net-vhostuser.args | 4 +- 3 files changed, 31 insertions(+), 22 deletions(-)
Seems reasonable and at least the NULL parameters being sent are checked in qemuBuildHostNetStr before being used. ACK John
https://bugzilla.redhat.com/show_bug.cgi?id=1366108 So far, the hotplug of vhost-user "worked" by pure chance. Well, qemu would error on our commands, but nevertheless. Now that we have everything prepare, We should support hotplugging og vhost-user. Firstly, we need to plug in chardev (through which qemu talks to OpenVSwitch), then netdev and only after that we can plug in the virtio-net-pci device. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 49 +++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 45 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index feb1f44..0bcb411 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -933,6 +933,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virDomainCCWAddressSetPtr ccwaddrs = NULL; size_t i; + char *charDevAlias = NULL; /* preallocate new slot for device */ if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets + 1) < 0) @@ -954,11 +955,11 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, case VIR_DOMAIN_NET_TYPE_DIRECT: case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_HOSTDEV: + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: /* These types are supported. */ 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: @@ -1148,6 +1149,26 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, goto cleanup; } + if (actualType == 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; + + qemuDomainObjEnterMonitor(driver, vm); + if (qemuMonitorAttachCharDev(priv->mon, charDevAlias, net->data.vhostuser) < 0) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + virDomainAuditNet(vm, NULL, net, "attach", false); + goto cleanup; + } + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + } + qemuDomainObjEnterMonitor(driver, vm); if (qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) { if (qemuMonitorAddNetdev(priv->mon, netstr, @@ -1268,6 +1289,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, } VIR_FREE(vhostfd); VIR_FREE(vhostfdName); + VIR_FREE(charDevAlias); virObjectUnref(cfg); virDomainCCWAddressSetFree(ccwaddrs); @@ -1283,6 +1305,9 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, if (virAsprintf(&netdev_name, "host%s", net->info.alias) < 0) goto cleanup; qemuDomainObjEnterMonitor(driver, vm); + if (charDevAlias && + qemuMonitorDetachCharDev(priv->mon, charDevAlias) < 0) + VIR_WARN("Failed to remove associated chardev %s", charDevAlias); if (qemuMonitorRemoveNetdev(priv->mon, netdev_name) < 0) VIR_WARN("Failed to remove network backend for netdev %s", netdev_name); @@ -3309,10 +3334,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)); @@ -3322,9 +3349,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 (qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) { if (qemuMonitorRemoveNetdev(priv->mon, hostnet_name) < 0) { @@ -3347,6 +3376,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; @@ -3371,7 +3411,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), @@ -3397,6 +3437,7 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, cleanup: virObjectUnref(cfg); + VIR_FREE(charDevAlias); VIR_FREE(hostnet_name); return ret; } -- 2.8.4
On 08/16/2016 11:41 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1366108
So far, the hotplug of vhost-user "worked" by pure chance. Well, qemu would error on our commands, but nevertheless. Now that we have everything prepare, We should support hotplugging og
prepared, we ... of
vhost-user. Firstly, we need to plug in chardev (through which qemu talks to OpenVSwitch), then netdev and only after that we can plug in the virtio-net-pci device.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 49 +++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 45 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index feb1f44..0bcb411 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -933,6 +933,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virDomainCCWAddressSetPtr ccwaddrs = NULL; size_t i; + char *charDevAlias = NULL;
/* preallocate new slot for device */ if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets + 1) < 0) @@ -954,11 +955,11 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, case VIR_DOMAIN_NET_TYPE_DIRECT: case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_HOSTDEV: + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: /* These types are supported. */ 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: @@ -1148,6 +1149,26 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, goto cleanup; }
+ if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) { + if (!qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Netdev support unavailable"));
"%s", on previous line "vhost-user hot-plug not support by this version of qemu"
+ goto cleanup; + } + + if (virAsprintf(&charDevAlias, "char%s", net->info.alias) < 0) + goto cleanup; + + qemuDomainObjEnterMonitor(driver, vm); + if (qemuMonitorAttachCharDev(priv->mon, charDevAlias, net->data.vhostuser) < 0) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + virDomainAuditNet(vm, NULL, net, "attach", false); + goto cleanup; + } + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + } + qemuDomainObjEnterMonitor(driver, vm); if (qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) { if (qemuMonitorAddNetdev(priv->mon, netstr, @@ -1268,6 +1289,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, } VIR_FREE(vhostfd); VIR_FREE(vhostfdName); + VIR_FREE(charDevAlias); virObjectUnref(cfg); virDomainCCWAddressSetFree(ccwaddrs);
@@ -1283,6 +1305,9 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, if (virAsprintf(&netdev_name, "host%s", net->info.alias) < 0) goto cleanup; qemuDomainObjEnterMonitor(driver, vm); + if (charDevAlias && + qemuMonitorDetachCharDev(priv->mon, charDevAlias) < 0) + VIR_WARN("Failed to remove associated chardev %s", charDevAlias); if (qemuMonitorRemoveNetdev(priv->mon, netdev_name) < 0) VIR_WARN("Failed to remove network backend for netdev %s", netdev_name); @@ -3309,10 +3334,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)); @@ -3322,9 +3349,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)
Part of me wonders if this should be || (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER && virAsprintf(&hostnet_name, "host%s", net->info.alias) < 0) Since it's only necessary for vhost-user, but it does seem excessive. Then again no more excessive than allocating something we don't use. IDC either way, but I'd be remiss if I didn't point it out.
goto cleanup;
+ qemuDomainObjEnterMonitor(driver, vm); if (qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) { if (qemuMonitorRemoveNetdev(priv->mon, hostnet_name) < 0) { @@ -3347,6 +3376,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. + */ + } + } +
The attach order is: 1. Build 'netstr' 2. If vhost-user, attach char dev 3. Add netdev/hostdev using netstr But your detach order is 1. Remove netdev/hostdev 2. Remove vhost-user chardev See anything wrong with that dependency-wise? Look at the failure code for Attach - it'll remove CharDev before Netdev So the question is - if you did this in the right order, then would that messy situation not exist? John
if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup;
@@ -3371,7 +3411,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), @@ -3397,6 +3437,7 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
cleanup: virObjectUnref(cfg); + VIR_FREE(charDevAlias); VIR_FREE(hostnet_name); return ret; }
Hi Michal, I remember i have used vhost-user-bridge to test vhost-user before. And you can find it in qemu source code tests/vhost-user-bridge.c, but i am not sure if you can avoid this crash with vhost-user-bridge :) BR, Luyao ----- Original Message ----- From: "Michal Privoznik" <mprivozn@redhat.com> To: libvir-list@redhat.com Sent: Tuesday, August 16, 2016 11:41:13 PM Subject: [libvirt] [PATCH 0/9] Couple of vhost-user fixes and cleanups While working on this, I've discovered a qemu crasher [1], so there are not tested properly. If you have pointers how to test vhost-user without need to install openvswitch, I'm all ears. 1: http://lists.nongnu.org/archive/html/qemu-devel/2016-08/msg02933.html Michal Privoznik (9): qemuDomainAttachNetDevice: Explicitly list allowed types for hotplug qemuDomain{Attach,Remove}NetDevice: Prefer qemuDomainSupportsNetdev qemuBuildChrChardevStr: Introduce @nowait argument qemuBuildVhostuserCommandLine: Reuse qemuBuildChrChardevStr qemuBuildHostNetStr: Realign qemuBuildHostNetStr: Explicitly enumerate net types qemuBuildVhostuserCommandLine: Unify -netdev creation qemuBuildHostNetStr: Support VIR_DOMAIN_NET_TYPE_VHOSTUSER qemu_hotplug: Support interface type of vhost-user hotplug src/qemu/qemu_command.c | 169 ++++++++++++--------- src/qemu/qemu_hotplug.c | 83 ++++++++-- .../qemuxml2argv-net-vhostuser-multiq.args | 6 +- .../qemuxml2argv-net-vhostuser.args | 4 +- 4 files changed, 176 insertions(+), 86 deletions(-) -- 2.8.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 17.08.2016 02:36, Luyao Huang wrote:
Hi Michal,
I remember i have used vhost-user-bridge to test vhost-user before. And you can find it in qemu source code tests/vhost-user-bridge.c, but i am not sure if you can avoid this crash with vhost-user-bridge :)
Hey, that's is awesome! Exactly what I was hoping for. Thank you very much. Michal
participants (4)
-
John Ferlan -
Laine Stump -
Luyao Huang -
Michal Privoznik