[libvirt] [PATCH 0/4] qemuBuildHostNetStr cleanups
This is not a blurb. Ján Tomko (4): qemuBuildHostNetStr: use type_sep earlier qemuBuildHostNetStr: do not start options with a comma qemuBuildHostNetStr: move stray VIR_DOMAIN_NET_TYPE_INTERNAL qemuBuildHostNetStr: remove dead code src/qemu/qemu_command.c | 49 +++++++++++++++++++++---------------------------- 1 file changed, 21 insertions(+), 28 deletions(-) -- 2.7.3
When hotplugging networks with ancient QEMUs not supporting QEMU_CAPS_NETDEV, we use space instead of a comma as the separator between the network type and other options. Except for "user", all the network types pass other options and use up the first separator by the time we get to the section that adds the alias (or vlan for QEMUs without CAPS_NETDEV). Since the alias/vlan is mandatory, convert all preceding code to add the separator at the end, removing the need to rewrite type_sep for all types but NET_TYPE_USER. --- src/qemu/qemu_command.c | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 21fd85c..eba7ba9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3674,7 +3674,7 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, /* for one tapfd 'fd=' shall be used, * for more than one 'fds=' is the right choice */ if (tapfdSize == 1) { - virBufferAsprintf(&buf, "fd=%s", tapfd[0]); + virBufferAsprintf(&buf, "fd=%s,", tapfd[0]); } else { virBufferAddLit(&buf, "fds="); for (i = 0; i < tapfdSize; i++) { @@ -3682,49 +3682,45 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, virBufferAddChar(&buf, ':'); virBufferAdd(&buf, tapfd[i], -1); } + virBufferAddChar(&buf, ','); } - type_sep = ','; is_tap = true; break; case VIR_DOMAIN_NET_TYPE_CLIENT: - virBufferAsprintf(&buf, "socket%cconnect=%s:%d", + 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", + 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", + 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", + 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: case VIR_DOMAIN_NET_TYPE_INTERNAL: - virBufferAddLit(&buf, "user"); + virBufferAsprintf(&buf, "user%c", type_sep); break; case VIR_DOMAIN_NET_TYPE_HOSTDEV: @@ -3733,12 +3729,11 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, return NULL; case VIR_DOMAIN_NET_TYPE_VHOSTUSER: - virBufferAsprintf(&buf, "vhost-user%cchardev=char%s", + virBufferAsprintf(&buf, "vhost-user%cchardev=char%s,", type_sep, net->info.alias); - type_sep = ','; if (net->driver.virtio.queues > 1) - virBufferAsprintf(&buf, ",queues=%u", + virBufferAsprintf(&buf, "queues=%u,", net->driver.virtio.queues); break; @@ -3747,13 +3742,12 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, } if (vlan >= 0) { - virBufferAsprintf(&buf, "%cvlan=%d", type_sep, vlan); + virBufferAsprintf(&buf, "vlan=%d", vlan); if (net->info.alias) virBufferAsprintf(&buf, ",name=host%s", net->info.alias); } else { - virBufferAsprintf(&buf, "%cid=host%s", - type_sep, net->info.alias); + virBufferAsprintf(&buf, "id=host%s", net->info.alias); } if (is_tap) { -- 2.7.3
On Fri, Oct 14, 2016 at 04:32:15PM +0200, Ján Tomko wrote:
When hotplugging networks with ancient QEMUs not supporting QEMU_CAPS_NETDEV, we use space instead of a comma as the separator between the network type and other options.
Except for "user", all the network types pass other options and use up the first separator by the time we get to the section that adds the alias (or vlan for QEMUs without CAPS_NETDEV).
Since the alias/vlan is mandatory, convert all preceding code to add the separator at the end, removing the need to rewrite type_sep for all types but NET_TYPE_USER. ---
ACK Pavel
Put the comma at the end and trim it later for consistency. --- src/qemu/qemu_command.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index eba7ba9..80ebe51 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3742,19 +3742,18 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, } if (vlan >= 0) { - virBufferAsprintf(&buf, "vlan=%d", vlan); + virBufferAsprintf(&buf, "vlan=%d,", vlan); if (net->info.alias) - virBufferAsprintf(&buf, ",name=host%s", - net->info.alias); + virBufferAsprintf(&buf, "name=host%s,", net->info.alias); } else { - virBufferAsprintf(&buf, "id=host%s", net->info.alias); + virBufferAsprintf(&buf, "id=host%s,", net->info.alias); } if (is_tap) { if (vhostfdSize) { - virBufferAddLit(&buf, ",vhost=on,"); + virBufferAddLit(&buf, "vhost=on,"); if (vhostfdSize == 1) { - virBufferAsprintf(&buf, "vhostfd=%s", vhostfd[0]); + virBufferAsprintf(&buf, "vhostfd=%s,", vhostfd[0]); } else { virBufferAddLit(&buf, "vhostfds="); for (i = 0; i < vhostfdSize; i++) { @@ -3762,14 +3761,16 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, virBufferAddChar(&buf, ':'); virBufferAdd(&buf, vhostfd[i], -1); } + virBufferAddChar(&buf, ','); } } if (net->tune.sndbuf_specified) - virBufferAsprintf(&buf, ",sndbuf=%lu", net->tune.sndbuf); + virBufferAsprintf(&buf, "sndbuf=%lu,", net->tune.sndbuf); } virObjectUnref(cfg); + virBufferTrim(&buf, ",", -1); if (virBufferCheckError(&buf) < 0) return NULL; -- 2.7.3
This network type is only used by the vbox driver and it does not make sense to group it with VIR_DOMAIN_NET_TYPE_USER. Introduced by commit 1dcbef8. --- src/qemu/qemu_command.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 80ebe51..19ee652 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3719,7 +3719,6 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, break; case VIR_DOMAIN_NET_TYPE_USER: - case VIR_DOMAIN_NET_TYPE_INTERNAL: virBufferAsprintf(&buf, "user%c", type_sep); break; @@ -3737,6 +3736,8 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, net->driver.virtio.queues); break; + case VIR_DOMAIN_NET_TYPE_INTERNAL: + /* Not supported by QEMU driver */ case VIR_DOMAIN_NET_TYPE_LAST: break; } -- 2.7.3
On Fri, Oct 14, 2016 at 04:32:17PM +0200, Ján Tomko wrote:
This network type is only used by the vbox driver and it does not make sense to group it with VIR_DOMAIN_NET_TYPE_USER.
Introduced by commit 1dcbef8. --- src/qemu/qemu_command.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 80ebe51..19ee652 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3719,7 +3719,6 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, break;
case VIR_DOMAIN_NET_TYPE_USER: - case VIR_DOMAIN_NET_TYPE_INTERNAL: virBufferAsprintf(&buf, "user%c", type_sep); break;
@@ -3737,6 +3736,8 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, net->driver.virtio.queues); break;
+ case VIR_DOMAIN_NET_TYPE_INTERNAL: + /* Not supported by QEMU driver */ case VIR_DOMAIN_NET_TYPE_LAST: break; }
This one is tricky. The net type *internal* was introduced for vbox driver, but there was no check in other drivers to refuse this type. Before commit 1dcbef8 this code handled all not listed cases the same way as net type *user* and that goes back before the *internal* type was introduced. So basically for qemu driver we've handled *internal* as *user* since it was introduced in libvirt. Personaly I don't like that fact and IMO we can add a code to ${driver}DomainDefValidate() to refuse that type except vbox driver. The argument to do it is that the net type *internal* is not documented so there is a chance that it's not used at all for other drivers except vbox. This patch resolves in this error while starting a guest: error: internal error: process exited while connecting to monitor: 2016-10-21T12:28:46.841029Z qemu-system-x86_64: -netdev id=hostnet1: Parameter 'type' is missing Pavel
This function is never called for VIR_DOMAIN_NET_TYPE_HOSTDEV, and the dead code comment agrees. Introduced by commit 1dcbef8a. --- src/qemu/qemu_command.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 19ee652..adeb906 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3722,11 +3722,6 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, virBufferAsprintf(&buf, "user%c", type_sep); 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: virBufferAsprintf(&buf, "vhost-user%cchardev=char%s,", type_sep, @@ -3736,6 +3731,8 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, net->driver.virtio.queues); break; + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + /* Should have been handled earlier via PCI/USB hotplug code. */ case VIR_DOMAIN_NET_TYPE_INTERNAL: /* Not supported by QEMU driver */ case VIR_DOMAIN_NET_TYPE_LAST: -- 2.7.3
On Fri, Oct 14, 2016 at 04:32:18PM +0200, Ján Tomko wrote:
This function is never called for VIR_DOMAIN_NET_TYPE_HOSTDEV, and the dead code comment agrees.
Introduced by commit 1dcbef8a. ---
ACK Pavel
participants (2)
-
Ján Tomko -
Pavel Hrdina