10:41 a.m.
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
10:41 a.m.
New subject: [libvirt] [PATCH 1/9] qemuDomainAttachNetDevice: Explicitly list allowed types for hotplug
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(a)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
5:47 p.m.
New subject: [libvirt] [PATCH 1/9] qemuDomainAttachNetDevice: Explicitly list allowed types for hotplug
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(a)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
9:13 a.m.
New subject: [libvirt] [PATCH 1/9] qemuDomainAttachNetDevice: Explicitly list allowed types for hotplug
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(a)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
10:41 a.m.
New subject: [libvirt] [PATCH 2/9] qemuDomain{Attach, Remove}NetDevice: Prefer qemuDomainSupportsNetdev
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(a)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
11:52 p.m.
New subject: [libvirt] [PATCH 2/9] qemuDomain{Attach, Remove}NetDevice: Prefer qemuDomainSupportsNetdev
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(a)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;
5:36 a.m.
New subject: [libvirt] [PATCH 2/9] qemuDomain{Attach, Remove}NetDevice: Prefer qemuDomainSupportsNetdev
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
5:48 p.m.
New subject: [libvirt] [PATCH 2/9] qemuDomain{Attach, Remove}NetDevice: Prefer qemuDomainSupportsNetdev
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(a)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;
10:41 a.m.
New subject: [libvirt] [PATCH 3/9] qemuBuildChrChardevStr: Introduce @nowait argument
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(a)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
5:50 p.m.
New subject: [libvirt] [PATCH 3/9] qemuBuildChrChardevStr: Introduce @nowait argument
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(a)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
9:13 a.m.
New subject: [libvirt] [PATCH 3/9] qemuBuildChrChardevStr: Introduce @nowait argument
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(a)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
10:41 a.m.
New subject: [libvirt] [PATCH 4/9] qemuBuildVhostuserCommandLine: Reuse qemuBuildChrChardevStr
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(a)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
5:52 p.m.
New subject: [libvirt] [PATCH 4/9] qemuBuildVhostuserCommandLine: Reuse qemuBuildChrChardevStr
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(a)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);
10:41 a.m.
New subject: [libvirt] [PATCH 5/9] qemuBuildHostNetStr: Realign
Signed-off-by: Michal Privoznik <mprivozn(a)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
5:52 p.m.
New subject: [libvirt] [PATCH 5/9] qemuBuildHostNetStr: Realign
On 08/16/2016 11:41 AM, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_command.c | 64 ++++++++++++++++++++++++-------------------------
1 file changed, 32 insertions(+), 32 deletions(-)
ACK
John
10:41 a.m.
New subject: [libvirt] [PATCH 6/9] qemuBuildHostNetStr: Explicitly enumerate net types
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(a)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
5:56 p.m.
New subject: [libvirt] [PATCH 6/9] qemuBuildHostNetStr: Explicitly enumerate net types
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(a)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) {
10:41 a.m.
New subject: [libvirt] [PATCH 7/9] qemuBuildVhostuserCommandLine: Unify -netdev creation
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(a)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
5:56 p.m.
New subject: [libvirt] [PATCH 7/9] qemuBuildVhostuserCommandLine: Unify -netdev creation
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(a)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
10:41 a.m.
New subject: [libvirt] [PATCH 8/9] qemuBuildHostNetStr: Support VIR_DOMAIN_NET_TYPE_VHOSTUSER
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(a)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
5:58 p.m.
New subject: [libvirt] [PATCH 8/9] qemuBuildHostNetStr: Support VIR_DOMAIN_NET_TYPE_VHOSTUSER
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(a)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
10:41 a.m.
New subject: [libvirt] [PATCH 9/9] qemu_hotplug: Support interface type of vhost-user hotplug
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(a)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
6:02 p.m.
New subject: [libvirt] [PATCH 9/9] qemu_hotplug: Support interface type of vhost-user hotplug
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(a)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;
}
7:36 p.m.
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(a)redhat.com>
To: libvir-list(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
12:54 a.m.
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
3017
days inactive
3055
days old
25 comments
4 participants
participants (4)
-
John Ferlan -
Laine Stump -
Luyao Huang -
Michal Privoznik