On 02/05/2015 07:52 AM, Ján Tomko wrote:

It is not used for other network interface models.

A little light here... Something along the lines of only supporting backend vhost attribute for virtio adapters and quietly dropping it for any other definition...

https://bugzilla.redhat.com/show_bug.cgi?id=1147195 --- src/conf/domain_conf.c | 6 +++- .../qemuxml2argv-tap-vhost-incorrect.xml | 39 ++++++++++++++++++++++ .../qemuxml2xmlout-tap-vhost-incorrect.xml | 38 +++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 4 files changed, 83 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-tap-vhost-incorrect.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-tap-vhost-incorrect.xml

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4251b13..da3764f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7369,6 +7369,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, char *vhostuser_path = NULL; char *vhostuser_type = NULL; char *trustGuestRxFilters = NULL; + char *vhost_path = NULL; virNWFilterHashTablePtr filterparams = NULL; virDomainActualNetDefPtr actual = NULL; xmlNodePtr oldnode = ctxt->node; @@ -7551,7 +7552,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, VIR_FREE(tmp);

if ((tmp = virXMLPropString(cur, "vhost"))) - def->backend.vhost = virFileSanitizePath(tmp); + vhost_path = virFileSanitizePath(tmp); VIR_FREE(tmp);

Coverity got grumpy here - it believes "vhost" can be found twice probably because there's other code (just above here) which fills in vhostuser_* values, but first checks (!vhostuser_* &&...) To keep Coverity quiet, adding a "!vhost_path &&" prior to the tmp = does work. Why this doesn't get flagged for def->backend.tap nor the former code and some other allocations, I'm not quite sure. Another option is to not use vhost_path and just do the following after decoding def->model (prior to the if loop where you used vhost_path): if (def->backend.vhost && STRNEQ_NULLABLE(def->model, "virtio")) VIR_FREE(def->backend.vhost); ACK - regardless of which method you choose. John

} } @@ -7992,6 +7993,8 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, } def->driver.virtio.guest.ufo = val; } + def->backend.vhost = vhost_path; + vhost_path = NULL; }

def->linkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DEFAULT; @@ -8061,6 +8064,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, VIR_FREE(addrtype); VIR_FREE(trustGuestRxFilters); VIR_FREE(ips); + VIR_FREE(vhost_path); virNWFilterHashTableFree(filterparams);

return def; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-tap-vhost-incorrect.xml b/tests/qemuxml2argvdata/qemuxml2argv-tap-vhost-incorrect.xml new file mode 100644 index 0000000..2cf312f --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-tap-vhost-incorrect.xml @@ -0,0 +1,39 @@ +<domain type='qemu'> + <name>test</name> + <uuid>bba65c0e-c049-934f-b6aa-4e2c0582acdf</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-0.13'>hvm</type> + <boot dev='cdrom'/> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='usb' index='0'/> + <controller type='virtio-serial' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> + </controller> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <interface type='user'> + <mac address='52:54:00:e5:48:58'/> + <model type='definitely-not-virtio'/> + <driver name='vhost' queues='5'/> + <backend tap='/dev/null' vhost='/dev/zero'/> + </interface> + <serial type='pty'> + <target port='0'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + </console> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-tap-vhost-incorrect.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-tap-vhost-incorrect.xml new file mode 100644 index 0000000..266cbf0 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-tap-vhost-incorrect.xml @@ -0,0 +1,38 @@ +<domain type='qemu'> + <name>test</name> + <uuid>bba65c0e-c049-934f-b6aa-4e2c0582acdf</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-0.13'>hvm</type> + <boot dev='cdrom'/> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='usb' index='0'/> + <controller type='virtio-serial' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> + </controller> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <interface type='user'> + <mac address='52:54:00:e5:48:58'/> + <model type='definitely-not-virtio'/> + <backend tap='/dev/null'/> + </interface> + <serial type='pty'> + <target port='0'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + </console> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index a0a1cab..d3dfd9e 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -417,6 +417,7 @@ mymain(void) DO_TEST("bios-nvram");

DO_TEST("tap-vhost"); + DO_TEST_DIFFERENT("tap-vhost-incorrect"); DO_TEST("shmem"); DO_TEST("smbios");

On 02/05/2015 07:52 AM, Ján Tomko wrote:

It is only usable for NETWORK and BRIDGE type interfaces. Error out when trying to start a domain where the custom tap device path is specified for interfaces of other types, or when the daemon is not privileged.

Note that this cannot be checked at definition time, because the comparison is against actual type.

https://bugzilla.redhat.com/show_bug.cgi?id=1147195 --- src/qemu/qemu_command.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)

Note: Something I forgot for [1/2] The <backend> is not well described in formatdomain.html.in - perhaps take the opportunity to indicate that <backend> attribute "vhost" is only valid for virtio network interfaces. Do you think it's reasonable to list the default of value "/dev/vhost-net" Then of course for this one - formatdomain.html.in - the <backend> attribute "tap" is only usable for NETWORK and BRIDGE type interfaces. Furthermore, custom "tap" devices are not supported in session mode. Do you think it's reasonable to list the default value "/dev/net/tun"?

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3b6eddc..06a59d0 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -299,8 +299,14 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); const char *tunpath = "/dev/net/tun";

- if (net->backend.tap) + if (net->backend.tap) { tunpath = net->backend.tap; + if (!cfg->privileged) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("cannot use custom tap device in session mode")); + goto cleanup; + } + }

This seems reasonable, wasn't part of the bz, but still should be documented... ACK - with the doc changes... John

if (!(brname = virDomainNetGetActualBridgeName(net))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing bridge name")); @@ -7721,6 +7727,15 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, return -1; }

+ if (net->backend.tap && + !(actualType == VIR_DOMAIN_NET_TYPE_NETWORK || + actualType == VIR_DOMAIN_NET_TYPE_BRIDGE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Custom tap device path is not supported for: %s"), + virDomainNetTypeToString(actualType)); + return -1; + } + if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { tapfdSize = net->driver.virtio.queues;