[libvirt] [PATCH v2 00/12] Multiple TX queue support
Just a rebased version of [1]. Kernel and subsequently QEMU learned multiple transmit queues a while ago. The feature has a nice advantage, it allows a single guest to transmit multiple flows of network data using multiple CPUs simultaneously which increase traffic bandwidth. A lot. The documentation how to use this is available at [2] or [3]. 1: https://www.redhat.com/archives/libvir-list/2013-April/msg01548.html 2: https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/tree/Documen... 3: http://git.qemu.org/?p=qemu.git;a=blob;f=qemu-options.hx;hb=HEAD#l1363 Michal Privoznik (12): Introduce /domain/devices/interface/driver/@queues attribute qemu: Move interface cmd line construction into a separate function qemu: Make qemuMonitorAddHostNetwork to pass multiple FDs qemu: Make qemuMonitorAddHostNetwork to pass multiple FDs qemu: Adapt command line generation to multiqueue net util: Learn virNetDevTapCreate multiqueue network qemu: Allow multiple vhost-net openings qemu: Rework qemuNetworkIfaceConnect qemu: Adapt qemuDomainAttachNetDevice to multiqueue net qemu: Change qemuOpenVhostNet return value qemu: Adapt qemuBuildInterfaceCommandLine to to multiqueue net qemu: Enable multiqueue network docs/formatdomain.html.in | 11 +- docs/schemas/domaincommon.rng | 5 + src/conf/domain_conf.c | 15 + src/conf/domain_conf.h | 1 + src/network/bridge_driver.c | 2 +- src/qemu/qemu_command.c | 453 +++++++++++++-------- src/qemu/qemu_command.h | 13 +- src/qemu/qemu_domain.c | 27 +- src/qemu/qemu_hotplug.c | 99 +++-- src/qemu/qemu_monitor.c | 78 ++-- src/qemu/qemu_monitor.h | 8 +- src/uml/uml_conf.c | 5 +- src/util/virnetdevtap.c | 115 +++--- src/util/virnetdevtap.h | 2 + tests/qemuxml2argvdata/qemuxml2argv-event_idx.xml | 2 +- .../qemuxml2argv-net-virtio-device.xml | 2 +- .../qemuxml2argvdata/qemuxml2argv-vhost_queues.xml | 51 +++ tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml | 2 +- tests/qemuxml2xmltest.c | 1 + 19 files changed, 586 insertions(+), 306 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-vhost_queues.xml -- 1.8.2.1
This attribute is going to represent number of queues for multique vhost network interface. This commit implements XML extension part of the feature and add one test as well. For now, we can only do xml2xml test as qemu command line generation code is not adapted yet. --- docs/formatdomain.html.in | 11 ++++- docs/schemas/domaincommon.rng | 5 +++ src/conf/domain_conf.c | 15 +++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_domain.c | 27 +++++++----- tests/qemuxml2argvdata/qemuxml2argv-event_idx.xml | 2 +- .../qemuxml2argv-net-virtio-device.xml | 2 +- .../qemuxml2argvdata/qemuxml2argv-vhost_queues.xml | 51 ++++++++++++++++++++++ tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml | 2 +- tests/qemuxml2xmltest.c | 1 + 10 files changed, 103 insertions(+), 14 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-vhost_queues.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 9ade507..68cd3d4 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3186,7 +3186,7 @@ qemu-kvm -net nic,model=? /dev/null <source network='default'/> <target dev='vnet1'/> <model type='virtio'/> - <b><driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off'/></b> + <b><driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5'/></b> </interface> </devices> ...</pre> @@ -3280,6 +3280,15 @@ qemu-kvm -net nic,model=? /dev/null <b>In general you should leave this option alone, unless you are very certain you know what you are doing.</b> </dd> + <dt><code>queues</code></dt> + <dd> + The optional <code>queues</code> attribute controls number of + queues for <a href="http://www.linux-kvm.org/page/Multiqueue">M + ultiqueue virtio-net</a> feature. Long story short, in case of a + virtio net device, multiple queues can be created so each queue is + handled by different processor resulting in much higher throughput. + <span class="since">Since 1.0.5 (QEMU and KVM only)</span> + </dd> </dl> <h5><a name="elementsNICSTargetOverride">Overriding the target element</a></h5> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 8004428..d671491 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1990,6 +1990,11 @@ </attribute> </optional> <optional> + <attribute name='queues'> + <ref name="positiveInteger"/> + </attribute> + </optional> + <optional> <attribute name="txmode"> <choice> <value>iothread</value> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 862b997..429f0ed 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5935,6 +5935,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, char *txmode = NULL; char *ioeventfd = NULL; char *event_idx = NULL; + char *queues = NULL; char *filter = NULL; char *internal = NULL; char *devaddr = NULL; @@ -6046,6 +6047,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, txmode = virXMLPropString(cur, "txmode"); ioeventfd = virXMLPropString(cur, "ioeventfd"); event_idx = virXMLPropString(cur, "event_idx"); + queues = virXMLPropString(cur, "queues"); } else if (xmlStrEqual(cur->name, BAD_CAST "filterref")) { if (filter) { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -6336,6 +6338,16 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, } def->driver.virtio.event_idx = idx; } + if (queues) { + unsigned int q; + if (virStrToLong_ui(queues, NULL, 10, &q) < 0) { + virReportError(VIR_ERR_XML_DETAIL, + _("'queues' attribute must be unsigned integer: %s"), + queues); + goto error; + } + def->driver.virtio.queues = q; + } } def->linkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DEFAULT; @@ -6389,6 +6401,7 @@ cleanup: VIR_FREE(txmode); VIR_FREE(ioeventfd); VIR_FREE(event_idx); + VIR_FREE(queues); VIR_FREE(filter); VIR_FREE(type); VIR_FREE(internal); @@ -14354,6 +14367,8 @@ virDomainNetDefFormat(virBufferPtr buf, virBufferAsprintf(buf, " event_idx='%s'", virDomainVirtioEventIdxTypeToString(def->driver.virtio.event_idx)); } + if (def->driver.virtio.queues) + virBufferAsprintf(buf, " queues='%u'", def->driver.virtio.queues); virBufferAddLit(buf, "/>\n"); } } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a9d3410..0a30406 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -923,6 +923,7 @@ struct _virDomainNetDef { enum virDomainNetVirtioTxModeType txmode; enum virDomainIoEventFd ioeventfd; enum virDomainVirtioEventIdx event_idx; + unsigned int queues; /* Multiqueue virtio-net */ } virtio; } driver; union { diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 97a8307..ce22afc 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -727,17 +727,24 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, virQEMUDriverPtr driver = opaque; virQEMUDriverConfigPtr cfg = NULL; - if (dev->type == VIR_DOMAIN_DEVICE_NET && - dev->data.net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV && - !dev->data.net->model) { - if (def->os.arch == VIR_ARCH_S390 || - def->os.arch == VIR_ARCH_S390X) - dev->data.net->model = strdup("virtio"); - else - dev->data.net->model = strdup("rtl8139"); + if (dev->type == VIR_DOMAIN_DEVICE_NET) { + if (dev->data.net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV && + !dev->data.net->model) { + if (def->os.arch == VIR_ARCH_S390 || + def->os.arch == VIR_ARCH_S390X) + dev->data.net->model = strdup("virtio"); + else + dev->data.net->model = strdup("rtl8139"); - if (!dev->data.net->model) - goto no_memory; + if (!dev->data.net->model) + goto no_memory; + } + + if (STREQ(dev->data.net->model, "virtio") && + dev->data.net->driver.virtio.queues == 0) { + /* default number of queues for multiqueue NIC */ + dev->data.net->driver.virtio.queues = 1; + } } /* set default disk types and drivers */ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-event_idx.xml b/tests/qemuxml2argvdata/qemuxml2argv-event_idx.xml index b3b7e89..44ce184 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-event_idx.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-event_idx.xml @@ -38,7 +38,7 @@ <interface type='user'> <mac address='52:54:00:e5:48:58'/> <model type='virtio'/> - <driver name='vhost' event_idx='off'/> + <driver name='vhost' event_idx='off' queues='1'/> </interface> <serial type='pty'> <target port='0'/> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-device.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-device.xml index 1782831..04f3471 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-device.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-device.xml @@ -25,7 +25,7 @@ <interface type='user'> <mac address='00:11:22:33:44:55'/> <model type='virtio'/> - <driver txmode='iothread'/> + <driver txmode='iothread' queues='1'/> </interface> <memballoon model='virtio'/> </devices> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-vhost_queues.xml b/tests/qemuxml2argvdata/qemuxml2argv-vhost_queues.xml new file mode 100644 index 0000000..76f84f6 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-vhost_queues.xml @@ -0,0 +1,51 @@ +<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> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2' event_idx='on'/> + <source file='/var/lib/libvirt/images/f14.img'/> + <target dev='vda' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </disk> + <disk type='file' device='cdrom'> + <driver name='qemu' type='raw'/> + <source file='/var/lib/libvirt/Fedora-14-x86_64-Live-KDE.iso'/> + <target dev='hdc' bus='ide'/> + <readonly/> + <address type='drive' controller='0' bus='1' target='0' unit='0'/> + </disk> + <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='virtio'/> + <driver name='vhost' queues='5'/> + </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/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml index 077ca92..2717439 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml @@ -37,7 +37,7 @@ <interface type='user'> <mac address='52:54:00:e5:48:58'/> <model type='virtio'/> - <driver name='vhost' event_idx='off'/> + <driver name='vhost' event_idx='off' queues='1'/> </interface> <serial type='pty'> <target port='0'/> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 492ac60..c06c189 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -244,6 +244,7 @@ mymain(void) DO_TEST("smp"); DO_TEST("lease"); DO_TEST("event_idx"); + DO_TEST("vhost_queues"); DO_TEST("virtio-lun"); DO_TEST("usb-redir"); -- 1.8.2.1
On 05/13/2013 01:22 PM, Michal Privoznik wrote:
This attribute is going to represent number of queues for multique vhost network interface. This commit implements XML extension part of the feature and add one test as well. For now, we can only do xml2xml test as qemu command line generation code is not adapted yet. --- docs/formatdomain.html.in | 11 ++++- docs/schemas/domaincommon.rng | 5 +++ src/conf/domain_conf.c | 15 +++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_domain.c | 27 +++++++----- tests/qemuxml2argvdata/qemuxml2argv-event_idx.xml | 2 +- .../qemuxml2argv-net-virtio-device.xml | 2 +- .../qemuxml2argvdata/qemuxml2argv-vhost_queues.xml | 51 ++++++++++++++++++++++ tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml | 2 +- tests/qemuxml2xmltest.c | 1 + 10 files changed, 103 insertions(+), 14 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-vhost_queues.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 9ade507..68cd3d4 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3186,7 +3186,7 @@ qemu-kvm -net nic,model=? /dev/null <source network='default'/> <target dev='vnet1'/> <model type='virtio'/> - <b><driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off'/></b> + <b><driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5'/></b> </interface> </devices> ...</pre> @@ -3280,6 +3280,15 @@ qemu-kvm -net nic,model=? /dev/null <b>In general you should leave this option alone, unless you are very certain you know what you are doing.</b> </dd> + <dt><code>queues</code></dt> + <dd> + The optional <code>queues</code> attribute controls number of + queues for <a href="http://www.linux-kvm.org/page/Multiqueue">M + ultiqueue virtio-net</a> feature. Long story short, in case of a
"Long story short" is a bit too informal for documentation. Maybe instead you could say something like: If the interface has <model type='virtio'/>, multiple packet processing queues can be created; each queue will potentially be handled by a different processor, resulting in much higher throughput.
+ virtio net device, multiple queues can be created so each queue is + handled by different processor resulting in much higher throughput. + <span class="since">Since 1.0.5 (QEMU and KVM only)</span>
s/1.0.5/1.0.6/
+ </dd> </dl>
<h5><a name="elementsNICSTargetOverride">Overriding the target element</a></h5> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 8004428..d671491 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1990,6 +1990,11 @@ </attribute> </optional> <optional> + <attribute name='queues'> + <ref name="positiveInteger"/>
Should a lower limit be put on it in the RNG? (does qemu have a documented limit?)
+ </attribute> + </optional> + <optional> <attribute name="txmode"> <choice> <value>iothread</value> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 862b997..429f0ed 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5935,6 +5935,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, char *txmode = NULL; char *ioeventfd = NULL; char *event_idx = NULL; + char *queues = NULL; char *filter = NULL; char *internal = NULL; char *devaddr = NULL; @@ -6046,6 +6047,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, txmode = virXMLPropString(cur, "txmode"); ioeventfd = virXMLPropString(cur, "ioeventfd"); event_idx = virXMLPropString(cur, "event_idx"); + queues = virXMLPropString(cur, "queues"); } else if (xmlStrEqual(cur->name, BAD_CAST "filterref")) { if (filter) { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -6336,6 +6338,16 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, } def->driver.virtio.event_idx = idx; } + if (queues) { + unsigned int q; + if (virStrToLong_ui(queues, NULL, 10, &q) < 0) { + virReportError(VIR_ERR_XML_DETAIL, + _("'queues' attribute must be unsigned integer: %s"),
How about "Invalid queues value %s, must be a positive number" or "must be 1 - [some value]"?
+ queues); + goto error; + } + def->driver.virtio.queues = q; + } }
def->linkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DEFAULT; @@ -6389,6 +6401,7 @@ cleanup: VIR_FREE(txmode); VIR_FREE(ioeventfd); VIR_FREE(event_idx); + VIR_FREE(queues); VIR_FREE(filter); VIR_FREE(type); VIR_FREE(internal); @@ -14354,6 +14367,8 @@ virDomainNetDefFormat(virBufferPtr buf, virBufferAsprintf(buf, " event_idx='%s'", virDomainVirtioEventIdxTypeToString(def->driver.virtio.event_idx)); } + if (def->driver.virtio.queues) + virBufferAsprintf(buf, " queues='%u'", def->driver.virtio.queues); virBufferAddLit(buf, "/>\n"); } } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a9d3410..0a30406 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -923,6 +923,7 @@ struct _virDomainNetDef { enum virDomainNetVirtioTxModeType txmode; enum virDomainIoEventFd ioeventfd; enum virDomainVirtioEventIdx event_idx; + unsigned int queues; /* Multiqueue virtio-net */ } virtio; } driver; union { diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 97a8307..ce22afc 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -727,17 +727,24 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, virQEMUDriverPtr driver = opaque; virQEMUDriverConfigPtr cfg = NULL;
- if (dev->type == VIR_DOMAIN_DEVICE_NET && - dev->data.net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV && - !dev->data.net->model) { - if (def->os.arch == VIR_ARCH_S390 || - def->os.arch == VIR_ARCH_S390X) - dev->data.net->model = strdup("virtio"); - else - dev->data.net->model = strdup("rtl8139"); + if (dev->type == VIR_DOMAIN_DEVICE_NET) { + if (dev->data.net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV && + !dev->data.net->model) { + if (def->os.arch == VIR_ARCH_S390 || + def->os.arch == VIR_ARCH_S390X) + dev->data.net->model = strdup("virtio"); + else + dev->data.net->model = strdup("rtl8139");
- if (!dev->data.net->model) - goto no_memory; + if (!dev->data.net->model) + goto no_memory; + } + + if (STREQ(dev->data.net->model, "virtio") && + dev->data.net->driver.virtio.queues == 0) { + /* default number of queues for multiqueue NIC */ + dev->data.net->driver.virtio.queues = 1; + }
Rather than relying on some code in an out-of-the-way post-parse callback to set this to 1, I think it would be better to just interpret 0 as 1 where the value is used. Aside from removing the mystery for someone casually reading the code who doesn't know about the post-parse function, it would also allow us to use "0" as a sentinel value which means "don't emit" during formatting. This is one of those attributes which I think *shouldn't* have its default value auto-filled (as long as migrating from one machine to another with the number of queues changing from source to dest doesn't create a problem, that is).
}
/* set default disk types and drivers */ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-event_idx.xml b/tests/qemuxml2argvdata/qemuxml2argv-event_idx.xml index b3b7e89..44ce184 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-event_idx.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-event_idx.xml @@ -38,7 +38,7 @@ <interface type='user'> <mac address='52:54:00:e5:48:58'/> <model type='virtio'/> - <driver name='vhost' event_idx='off'/> + <driver name='vhost' event_idx='off' queues='1'/>
... and you would then avoid all of these gratuitous changes to xml test data.
</interface> <serial type='pty'> <target port='0'/> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-device.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-device.xml index 1782831..04f3471 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-device.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-device.xml @@ -25,7 +25,7 @@ <interface type='user'> <mac address='00:11:22:33:44:55'/> <model type='virtio'/> - <driver txmode='iothread'/> + <driver txmode='iothread' queues='1'/> </interface> <memballoon model='virtio'/> </devices> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-vhost_queues.xml b/tests/qemuxml2argvdata/qemuxml2argv-vhost_queues.xml new file mode 100644 index 0000000..76f84f6 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-vhost_queues.xml @@ -0,0 +1,51 @@ +<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> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2' event_idx='on'/> + <source file='/var/lib/libvirt/images/f14.img'/> + <target dev='vda' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </disk> + <disk type='file' device='cdrom'> + <driver name='qemu' type='raw'/> + <source file='/var/lib/libvirt/Fedora-14-x86_64-Live-KDE.iso'/> + <target dev='hdc' bus='ide'/> + <readonly/> + <address type='drive' controller='0' bus='1' target='0' unit='0'/> + </disk> + <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='virtio'/> + <driver name='vhost' queues='5'/> + </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/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml index 077ca92..2717439 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml @@ -37,7 +37,7 @@ <interface type='user'> <mac address='52:54:00:e5:48:58'/> <model type='virtio'/> - <driver name='vhost' event_idx='off'/> + <driver name='vhost' event_idx='off' queues='1'/> </interface> <serial type='pty'> <target port='0'/> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 492ac60..c06c189 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -244,6 +244,7 @@ mymain(void) DO_TEST("smp"); DO_TEST("lease"); DO_TEST("event_idx"); + DO_TEST("vhost_queues"); DO_TEST("virtio-lun");
DO_TEST("usb-redir");
On 13.05.2013 22:38, Laine Stump wrote:
On 05/13/2013 01:22 PM, Michal Privoznik wrote:
This attribute is going to represent number of queues for multique vhost network interface. This commit implements XML extension part of the feature and add one test as well. For now, we can only do xml2xml test as qemu command line generation code is not adapted yet. --- docs/formatdomain.html.in | 11 ++++- docs/schemas/domaincommon.rng | 5 +++ src/conf/domain_conf.c | 15 +++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_domain.c | 27 +++++++----- tests/qemuxml2argvdata/qemuxml2argv-event_idx.xml | 2 +- .../qemuxml2argv-net-virtio-device.xml | 2 +- .../qemuxml2argvdata/qemuxml2argv-vhost_queues.xml | 51 ++++++++++++++++++++++ tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml | 2 +- tests/qemuxml2xmltest.c | 1 + 10 files changed, 103 insertions(+), 14 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-vhost_queues.xml
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 8004428..d671491 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1990,6 +1990,11 @@ </attribute> </optional> <optional> + <attribute name='queues'> + <ref name="positiveInteger"/>
Should a lower limit be put on it in the RNG? (does qemu have a documented limit?)
I don't think so. QEMU doesn't have anything documented, but they are using uint16_t to store the max_queues internally. However, when setting it in virtio_net_device_init they use: n->max_queues = MAX(n->nic_conf.queues, 1); where n->nic_conf.queues is int32_t. Maybe we should ask on the qemu list.
+ </attribute> + </optional> + <optional> <attribute name="txmode"> <choice> <value>iothread</value>
On 14/05/13 17:33, Michal Privoznik wrote:
On 13.05.2013 22:38, Laine Stump wrote:
On 05/13/2013 01:22 PM, Michal Privoznik wrote:
This attribute is going to represent number of queues for multique vhost network interface. This commit implements XML extension part of the feature and add one test as well. For now, we can only do xml2xml test as qemu command line generation code is not adapted yet. --- docs/formatdomain.html.in | 11 ++++- docs/schemas/domaincommon.rng | 5 +++ src/conf/domain_conf.c | 15 +++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_domain.c | 27 +++++++----- tests/qemuxml2argvdata/qemuxml2argv-event_idx.xml | 2 +- .../qemuxml2argv-net-virtio-device.xml | 2 +- .../qemuxml2argvdata/qemuxml2argv-vhost_queues.xml | 51 ++++++++++++++++++++++ tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml | 2 +- tests/qemuxml2xmltest.c | 1 + 10 files changed, 103 insertions(+), 14 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-vhost_queues.xml
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 8004428..d671491 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1990,6 +1990,11 @@ </attribute> </optional> <optional> + <attribute name='queues'> + <ref name="positiveInteger"/>
Should a lower limit be put on it in the RNG? (does qemu have a documented limit?) I don't think so. QEMU doesn't have anything documented, but they are using uint16_t to store the max_queues internally. However, when setting it in virtio_net_device_init they use:
n->max_queues = MAX(n->nic_conf.queues, 1);
where n->nic_conf.queues is int32_t. Maybe we should ask on the qemu list.
This means we want qemu to either document the limits or expose them in some manner. See commit log of 0d70656afded421e434a03c4af4d004a0fbcde47, I was annoyed about to find out the limits of the various qemu device. Osier
On 14.05.2013 11:33, Michal Privoznik wrote:
On 13.05.2013 22:38, Laine Stump wrote:
On 05/13/2013 01:22 PM, Michal Privoznik wrote:
This attribute is going to represent number of queues for multique vhost network interface. This commit implements XML extension part of the feature and add one test as well. For now, we can only do xml2xml test as qemu command line generation code is not adapted yet. --- docs/formatdomain.html.in | 11 ++++- docs/schemas/domaincommon.rng | 5 +++ src/conf/domain_conf.c | 15 +++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_domain.c | 27 +++++++----- tests/qemuxml2argvdata/qemuxml2argv-event_idx.xml | 2 +- .../qemuxml2argv-net-virtio-device.xml | 2 +- .../qemuxml2argvdata/qemuxml2argv-vhost_queues.xml | 51 ++++++++++++++++++++++ tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml | 2 +- tests/qemuxml2xmltest.c | 1 + 10 files changed, 103 insertions(+), 14 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-vhost_queues.xml
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 8004428..d671491 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1990,6 +1990,11 @@ </attribute> </optional> <optional> + <attribute name='queues'> + <ref name="positiveInteger"/>
Should a lower limit be put on it in the RNG? (does qemu have a documented limit?)
I don't think so. QEMU doesn't have anything documented, but they are using uint16_t to store the max_queues internally. However, when setting it in virtio_net_device_init they use:
n->max_queues = MAX(n->nic_conf.queues, 1);
where n->nic_conf.queues is int32_t. Maybe we should ask on the qemu list.
D'oh! So I've just asked a kernel guy privately and he says, kernel has the limit of 8 queues. But anyway, do we need to enforce this limit or should we let kernel to report an error instead? What if: we introduce the limit, but kernel lifts the limit someday? Users are effectively forced to update the libvirt, and we are forced to keep an eye on kernel if the limit has changed or not (any volunteer?). Therefore I vote for keeping this part as is. I don't think putting another check point is desired. Michal
On 05/14/2013 06:10 AM, Michal Privoznik wrote:
On 14.05.2013 11:33, Michal Privoznik wrote:
On 13.05.2013 22:38, Laine Stump wrote:
On 05/13/2013 01:22 PM, Michal Privoznik wrote:
This attribute is going to represent number of queues for multique vhost network interface. This commit implements XML extension part of the feature and add one test as well. For now, we can only do xml2xml test as qemu command line generation code is not adapted yet. --- docs/formatdomain.html.in | 11 ++++- docs/schemas/domaincommon.rng | 5 +++ src/conf/domain_conf.c | 15 +++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_domain.c | 27 +++++++----- tests/qemuxml2argvdata/qemuxml2argv-event_idx.xml | 2 +- .../qemuxml2argv-net-virtio-device.xml | 2 +- .../qemuxml2argvdata/qemuxml2argv-vhost_queues.xml | 51 ++++++++++++++++++++++ tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml | 2 +- tests/qemuxml2xmltest.c | 1 + 10 files changed, 103 insertions(+), 14 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-vhost_queues.xml
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 8004428..d671491 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1990,6 +1990,11 @@ </attribute> </optional> <optional> + <attribute name='queues'> + <ref name="positiveInteger"/>
Should a lower limit be put on it in the RNG? (does qemu have a documented limit?) I don't think so. QEMU doesn't have anything documented, but they are using uint16_t to store the max_queues internally. However, when setting it in virtio_net_device_init they use:
n->max_queues = MAX(n->nic_conf.queues, 1);
where n->nic_conf.queues is int32_t. Maybe we should ask on the qemu list.
D'oh! So I've just asked a kernel guy privately and he says, kernel has the limit of 8 queues. But anyway, do we need to enforce this limit or should we let kernel to report an error instead? What if: we introduce the limit, but kernel lifts the limit someday? Users are effectively forced to update the libvirt, and we are forced to keep an eye on kernel if the limit has changed or not (any volunteer?). Therefore I vote for keeping this part as is. I don't think putting another check point is desired.
If someone enters "queues='2345987234'" will the kernel report the failure when we attempt to open the 9th fd? Or would be be allowed to open fds until some more catastrophic failure happened (i.e. we reach the open fds limit for libvirtd)? If the former, then it's probably okay to not impose any artificial limit, and let the error be exposed organically. If not, then we may need to impose some limit that is so high it would never be reached, but still low enough that reaching it wouldn't put us into a DoS situation.
On 14.05.2013 13:59, Laine Stump wrote:
On 05/14/2013 06:10 AM, Michal Privoznik wrote:
On 14.05.2013 11:33, Michal Privoznik wrote:
On 13.05.2013 22:38, Laine Stump wrote:
On 05/13/2013 01:22 PM, Michal Privoznik wrote:
This attribute is going to represent number of queues for multique vhost network interface. This commit implements XML extension part of the feature and add one test as well. For now, we can only do xml2xml test as qemu command line generation code is not adapted yet. --- docs/formatdomain.html.in | 11 ++++- docs/schemas/domaincommon.rng | 5 +++ src/conf/domain_conf.c | 15 +++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_domain.c | 27 +++++++----- tests/qemuxml2argvdata/qemuxml2argv-event_idx.xml | 2 +- .../qemuxml2argv-net-virtio-device.xml | 2 +- .../qemuxml2argvdata/qemuxml2argv-vhost_queues.xml | 51 ++++++++++++++++++++++ tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml | 2 +- tests/qemuxml2xmltest.c | 1 + 10 files changed, 103 insertions(+), 14 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-vhost_queues.xml
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 8004428..d671491 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1990,6 +1990,11 @@ </attribute> </optional> <optional> + <attribute name='queues'> + <ref name="positiveInteger"/>
Should a lower limit be put on it in the RNG? (does qemu have a documented limit?) I don't think so. QEMU doesn't have anything documented, but they are using uint16_t to store the max_queues internally. However, when setting it in virtio_net_device_init they use:
n->max_queues = MAX(n->nic_conf.queues, 1);
where n->nic_conf.queues is int32_t. Maybe we should ask on the qemu list.
D'oh! So I've just asked a kernel guy privately and he says, kernel has the limit of 8 queues. But anyway, do we need to enforce this limit or should we let kernel to report an error instead? What if: we introduce the limit, but kernel lifts the limit someday? Users are effectively forced to update the libvirt, and we are forced to keep an eye on kernel if the limit has changed or not (any volunteer?). Therefore I vote for keeping this part as is. I don't think putting another check point is desired.
If someone enters "queues='2345987234'" will the kernel report the failure when we attempt to open the 9th fd? Or would be be allowed to open fds until some more catastrophic failure happened (i.e. we reach the open fds limit for libvirtd)? If the former, then it's probably okay to not impose any artificial limit, and let the error be exposed organically. If not, then we may need to impose some limit that is so high it would never be reached, but still low enough that reaching it wouldn't put us into a DoS situation.
The former one. Kernel shouts: 2013-05-14 10:04:44.370+0000: 12723: error : virNetDevTapCreate:202 : Unable to create tap device vnet0: Argument list too long So I think we're safe here. Michal
Currently, we have one huge function to construct qemu command line. This is very ineffective esp. if there's a fault somewhere. --- src/qemu/qemu_command.c | 228 +++++++++++++++++++++++++----------------------- 1 file changed, 121 insertions(+), 107 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index eddc263..7775471 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6356,6 +6356,121 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg, return 0; } +static int +qemuBuildInterfaceCommandLine(virCommandPtr cmd, + virQEMUDriverPtr driver, + virConnectPtr conn, + virDomainDefPtr def, + virDomainNetDefPtr net, + virQEMUCapsPtr qemuCaps, + int vlan, + int bootindex, + enum virNetDevVPortProfileOp vmop) +{ + int ret = -1; + int tapfd = -1; + char *nic = NULL, *host = NULL; + char *tapfdName = NULL; + char *vhostfdName = NULL; + int actualType = virDomainNetGetActualType(net); + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + + if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + /* NET_TYPE_HOSTDEV devices are really hostdev devices, so + * their commandlines are constructed with other hostdevs. + */ + ret = 0; + goto cleanup; + } + + if (!bootindex) + bootindex = net->info.bootIndex; + + if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || + actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { + tapfd = qemuNetworkIfaceConnect(def, conn, driver, net, qemuCaps); + if (tapfd < 0) + goto cleanup; + } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { + tapfd = qemuPhysIfaceConnect(def, driver, net, qemuCaps, vmop); + if (tapfd < 0) + goto cleanup; + } + + if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || + actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || + actualType == VIR_DOMAIN_NET_TYPE_ETHERNET || + actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { + /* Attempt to use vhost-net mode for these types of + network device */ + int vhostfd; + + if (qemuOpenVhostNet(def, net, qemuCaps, &vhostfd) < 0) + goto cleanup; + if (vhostfd >= 0) { + virCommandTransferFD(cmd, vhostfd); + + if (virAsprintf(&vhostfdName, "%d", vhostfd) < 0) { + virReportOOMError(); + goto cleanup; + } + } + } + + if (tapfd >= 0) { + if (virAsprintf(&tapfdName, "%d", tapfd) < 0) { + virReportOOMError(); + goto cleanup; + } + virCommandTransferFD(cmd, tapfd); + } + + /* Possible combinations: + * + * 1. Old way: -net nic,model=e1000,vlan=1 -net tap,vlan=1 + * 2. Semi-new: -device e1000,vlan=1 -net tap,vlan=1 + * 3. Best way: -netdev type=tap,id=netdev1 -device e1000,id=netdev1 + * + * NB, no support for -netdev without use of -device + */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { + if (!(host = qemuBuildHostNetStr(net, driver, + ',', vlan, tapfdName, + vhostfdName))) + goto cleanup; + virCommandAddArgList(cmd, "-netdev", host, NULL); + } + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { + if (!(nic = qemuBuildNicDevStr(net, vlan, bootindex, qemuCaps))) + goto cleanup; + virCommandAddArgList(cmd, "-device", nic, NULL); + } else { + if (!(nic = qemuBuildNicStr(net, "nic,", vlan))) + goto cleanup; + virCommandAddArgList(cmd, "-net", nic, NULL); + } + if (!(virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))) { + if (!(host = qemuBuildHostNetStr(net, driver, + ',', vlan, tapfdName, + vhostfdName))) + goto cleanup; + virCommandAddArgList(cmd, "-net", host, NULL); + } + + ret = 0; +cleanup: + if (ret < 0) + virDomainConfNWFilterTeardown(net); + VIR_FREE(nic); + VIR_FREE(host); + VIR_FREE(tapfdName); + VIR_FREE(vhostfdName); + virObjectUnref(cfg); + return ret; +} + /* * Constructs a argv suitable for launching qemu with config defined * for a given virtual machine. @@ -7269,118 +7384,17 @@ qemuBuildCommandLine(virConnectPtr conn, for (i = 0 ; i < def->nnets ; i++) { virDomainNetDefPtr net = def->nets[i]; - char *nic, *host; - char tapfd_name[50] = ""; - char vhostfd_name[50] = ""; - int vlan; - int bootindex = bootNet; - int actualType = virDomainNetGetActualType(net); - - if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { - /* NET_TYPE_HOSTDEV devices are really hostdev devices, so - * their commandlines are constructed with other hostdevs. - */ - continue; - } - - bootNet = 0; - if (!bootindex) - bootindex = net->info.bootIndex; - + int vlan = i; /* VLANs are not used with -netdev, so don't record them */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) vlan = -1; - else - vlan = i; - if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || - actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { - int tapfd = qemuNetworkIfaceConnect(def, conn, driver, net, - qemuCaps); - if (tapfd < 0) - goto error; - - last_good_net = i; - virCommandTransferFD(cmd, tapfd); - - if (snprintf(tapfd_name, sizeof(tapfd_name), "%d", - tapfd) >= sizeof(tapfd_name)) - goto no_memory; - } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { - int tapfd = qemuPhysIfaceConnect(def, driver, net, - qemuCaps, vmop); - if (tapfd < 0) - goto error; - - last_good_net = i; - virCommandTransferFD(cmd, tapfd); - - if (snprintf(tapfd_name, sizeof(tapfd_name), "%d", - tapfd) >= sizeof(tapfd_name)) - goto no_memory; - } - - if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || - actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || - actualType == VIR_DOMAIN_NET_TYPE_ETHERNET || - actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { - /* Attempt to use vhost-net mode for these types of - network device */ - int vhostfd; - - if (qemuOpenVhostNet(def, net, qemuCaps, &vhostfd) < 0) - goto error; - if (vhostfd >= 0) { - virCommandTransferFD(cmd, vhostfd); - - if (snprintf(vhostfd_name, sizeof(vhostfd_name), "%d", - vhostfd) >= sizeof(vhostfd_name)) - goto no_memory; - } - } - /* Possible combinations: - * - * 1. Old way: -net nic,model=e1000,vlan=1 -net tap,vlan=1 - * 2. Semi-new: -device e1000,vlan=1 -net tap,vlan=1 - * 3. Best way: -netdev type=tap,id=netdev1 -device e1000,id=netdev1 - * - * NB, no support for -netdev without use of -device - */ - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { - virCommandAddArg(cmd, "-netdev"); - if (!(host = qemuBuildHostNetStr(net, driver, - ',', vlan, tapfd_name, - vhostfd_name))) - goto error; - virCommandAddArg(cmd, host); - VIR_FREE(host); - } - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { - virCommandAddArg(cmd, "-device"); - nic = qemuBuildNicDevStr(net, vlan, bootindex, qemuCaps); - if (!nic) - goto error; - virCommandAddArg(cmd, nic); - VIR_FREE(nic); - } else { - virCommandAddArg(cmd, "-net"); - if (!(nic = qemuBuildNicStr(net, "nic,", vlan))) - goto error; - virCommandAddArg(cmd, nic); - VIR_FREE(nic); - } - if (!(virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))) { - virCommandAddArg(cmd, "-net"); - if (!(host = qemuBuildHostNetStr(net, driver, - ',', vlan, tapfd_name, - vhostfd_name))) - goto error; - virCommandAddArg(cmd, host); - VIR_FREE(host); - } + if (qemuBuildInterfaceCommandLine(cmd, driver, conn, def, net, + qemuCaps, vlan, bootNet, vmop) < 0) + goto error; + last_good_net = i; + bootNet = 0; } } -- 1.8.2.1
On 05/13/2013 01:22 PM, Michal Privoznik wrote:
Currently, we have one huge function to construct qemu command line. This is very ineffective esp. if there's a fault somewhere. --- src/qemu/qemu_command.c | 228 +++++++++++++++++++++++++----------------------- 1 file changed, 121 insertions(+), 107 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index eddc263..7775471 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6356,6 +6356,121 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg, return 0; }
+static int +qemuBuildInterfaceCommandLine(virCommandPtr cmd, + virQEMUDriverPtr driver, + virConnectPtr conn, + virDomainDefPtr def, + virDomainNetDefPtr net, + virQEMUCapsPtr qemuCaps, + int vlan, + int bootindex, + enum virNetDevVPortProfileOp vmop) +{ + int ret = -1; + int tapfd = -1; + char *nic = NULL, *host = NULL; + char *tapfdName = NULL; + char *vhostfdName = NULL; + int actualType = virDomainNetGetActualType(net); + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
This made me take a look at how often/where virQEMUDriverGetConfig is called. Although it does it for a very short time, virQEMUDriverGetConfig does lock the entire driver, but this is unnecessary because the caller already has a reference to the config object. Unless there's some unwritten rule against it (and I don't think there is, because there are other qemuBuild*CommandLine() functions that do it), I think we should be passing the driver config pointer into subordinate functions, rather than going to the time/trouble to lock the driver just to get another reference to the object when we know that we already have a reference in the same thread. (it's not as important here since this is just control plane operations, but every time you grab a lock, all the caches on all the CPU cores are flushed.)
+ + if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + /* NET_TYPE_HOSTDEV devices are really hostdev devices, so + * their commandlines are constructed with other hostdevs. + */ + ret = 0; + goto cleanup; + }
If you did the above check prior to virQEMUDriverGetConfig() (or send cfg in as an arg), you could just do an immediate "return 0;"
+ + if (!bootindex) + bootindex = net->info.bootIndex; + + if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || + actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { + tapfd = qemuNetworkIfaceConnect(def, conn, driver, net, qemuCaps); + if (tapfd < 0) + goto cleanup; + } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { + tapfd = qemuPhysIfaceConnect(def, driver, net, qemuCaps, vmop); + if (tapfd < 0) + goto cleanup; + } + + if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || + actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || + actualType == VIR_DOMAIN_NET_TYPE_ETHERNET || + actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { + /* Attempt to use vhost-net mode for these types of + network device */ + int vhostfd; + + if (qemuOpenVhostNet(def, net, qemuCaps, &vhostfd) < 0) + goto cleanup; + if (vhostfd >= 0) { + virCommandTransferFD(cmd, vhostfd); + + if (virAsprintf(&vhostfdName, "%d", vhostfd) < 0) {
That blank line before the if() looks odd.
+ virReportOOMError(); + goto cleanup; + } + } + } + + if (tapfd >= 0) { + if (virAsprintf(&tapfdName, "%d", tapfd) < 0) { + virReportOOMError(); + goto cleanup; + } + virCommandTransferFD(cmd, tapfd);
I know that, practically speaking, it makes no difference, but up above you call virCommandTransferFD, then virAsprintf, and here you do it in the opposite order. The OCD in me wants to see them both do it in the same order :-)
+ } + + /* Possible combinations: + * + * 1. Old way: -net nic,model=e1000,vlan=1 -net tap,vlan=1 + * 2. Semi-new: -device e1000,vlan=1 -net tap,vlan=1 + * 3. Best way: -netdev type=tap,id=netdev1 -device e1000,id=netdev1 + * + * NB, no support for -netdev without use of -device + */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { + if (!(host = qemuBuildHostNetStr(net, driver, + ',', vlan, tapfdName, + vhostfdName))) + goto cleanup; + virCommandAddArgList(cmd, "-netdev", host, NULL); + } + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { + if (!(nic = qemuBuildNicDevStr(net, vlan, bootindex, qemuCaps))) + goto cleanup; + virCommandAddArgList(cmd, "-device", nic, NULL); + } else { + if (!(nic = qemuBuildNicStr(net, "nic,", vlan))) + goto cleanup; + virCommandAddArgList(cmd, "-net", nic, NULL); + }
+ if (!(virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))) { + if (!(host = qemuBuildHostNetStr(net, driver, + ',', vlan, tapfdName, + vhostfdName))) + goto cleanup; + virCommandAddArgList(cmd, "-net", host, NULL); + }
This set if if() clauses seems more complex than it needs to be, but you're just copying existing code, so we can leave it as is.
+ + ret = 0; +cleanup: + if (ret < 0) + virDomainConfNWFilterTeardown(net); + VIR_FREE(nic); + VIR_FREE(host); + VIR_FREE(tapfdName); + VIR_FREE(vhostfdName); + virObjectUnref(cfg); + return ret; +} + /* * Constructs a argv suitable for launching qemu with config defined * for a given virtual machine. @@ -7269,118 +7384,17 @@ qemuBuildCommandLine(virConnectPtr conn,
for (i = 0 ; i < def->nnets ; i++) { virDomainNetDefPtr net = def->nets[i]; - char *nic, *host; - char tapfd_name[50] = ""; - char vhostfd_name[50] = ""; - int vlan; - int bootindex = bootNet; - int actualType = virDomainNetGetActualType(net); - - if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { - /* NET_TYPE_HOSTDEV devices are really hostdev devices, so - * their commandlines are constructed with other hostdevs. - */ - continue; - } - - bootNet = 0; - if (!bootindex) - bootindex = net->info.bootIndex; - + int vlan = i; /* VLANs are not used with -netdev, so don't record them */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) vlan = -1; - else - vlan = i;
- if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || - actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { - int tapfd = qemuNetworkIfaceConnect(def, conn, driver, net, - qemuCaps); - if (tapfd < 0) - goto error; - - last_good_net = i; - virCommandTransferFD(cmd, tapfd); - - if (snprintf(tapfd_name, sizeof(tapfd_name), "%d", - tapfd) >= sizeof(tapfd_name)) - goto no_memory; - } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { - int tapfd = qemuPhysIfaceConnect(def, driver, net, - qemuCaps, vmop); - if (tapfd < 0) - goto error; - - last_good_net = i; - virCommandTransferFD(cmd, tapfd); - - if (snprintf(tapfd_name, sizeof(tapfd_name), "%d", - tapfd) >= sizeof(tapfd_name)) - goto no_memory; - } - - if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || - actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || - actualType == VIR_DOMAIN_NET_TYPE_ETHERNET || - actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { - /* Attempt to use vhost-net mode for these types of - network device */ - int vhostfd; - - if (qemuOpenVhostNet(def, net, qemuCaps, &vhostfd) < 0) - goto error; - if (vhostfd >= 0) { - virCommandTransferFD(cmd, vhostfd); - - if (snprintf(vhostfd_name, sizeof(vhostfd_name), "%d", - vhostfd) >= sizeof(vhostfd_name)) - goto no_memory; - } - } - /* Possible combinations: - * - * 1. Old way: -net nic,model=e1000,vlan=1 -net tap,vlan=1 - * 2. Semi-new: -device e1000,vlan=1 -net tap,vlan=1 - * 3. Best way: -netdev type=tap,id=netdev1 -device e1000,id=netdev1 - * - * NB, no support for -netdev without use of -device - */ - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { - virCommandAddArg(cmd, "-netdev"); - if (!(host = qemuBuildHostNetStr(net, driver, - ',', vlan, tapfd_name, - vhostfd_name))) - goto error; - virCommandAddArg(cmd, host); - VIR_FREE(host); - } - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { - virCommandAddArg(cmd, "-device"); - nic = qemuBuildNicDevStr(net, vlan, bootindex, qemuCaps); - if (!nic) - goto error; - virCommandAddArg(cmd, nic); - VIR_FREE(nic); - } else { - virCommandAddArg(cmd, "-net"); - if (!(nic = qemuBuildNicStr(net, "nic,", vlan))) - goto error; - virCommandAddArg(cmd, nic); - VIR_FREE(nic); - } - if (!(virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))) { - virCommandAddArg(cmd, "-net"); - if (!(host = qemuBuildHostNetStr(net, driver, - ',', vlan, tapfd_name, - vhostfd_name))) - goto error; - virCommandAddArg(cmd, host); - VIR_FREE(host); - } + if (qemuBuildInterfaceCommandLine(cmd, driver, conn, def, net, + qemuCaps, vlan, bootNet, vmop) < 0) + goto error; + last_good_net = i; + bootNet = 0; } }
ACK if you get rid of virQEMUDriverGetConfig and pass cfg as a parameter instead.
Currently, only one tapfd and one vhostfd could be passed. However, multiqueue network requires several FDs to be passed to qemu so we must adapt out monitor handling functions to cope with that. --- src/qemu/qemu_hotplug.c | 7 +++++-- src/qemu/qemu_monitor.c | 39 +++++++++++++++++++++++---------------- src/qemu/qemu_monitor.h | 4 ++-- 3 files changed, 30 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index d037c9d..b04f3bb 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -813,8 +813,11 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, qemuDomainObjEnterMonitor(driver, vm); if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { - if (qemuMonitorAddNetdev(priv->mon, netstr, tapfd, tapfd_name, - vhostfd, vhostfd_name) < 0) { + if (qemuMonitorAddNetdev(priv->mon, netstr, + &tapfd, &tapfd_name, + tapfd_name ? 1 : 0, + &vhostfd, &vhostfd_name, + vhostfd_name ? 1 : 0) < 0) { qemuDomainObjExitMonitor(driver, vm); virDomainAuditNet(vm, NULL, net, "attach", false); goto cleanup; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 4deb2d6..c3f35b5 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2527,14 +2527,16 @@ int qemuMonitorRemoveHostNetwork(qemuMonitorPtr mon, int qemuMonitorAddNetdev(qemuMonitorPtr mon, const char *netdevstr, - int tapfd, const char *tapfd_name, - int vhostfd, const char *vhostfd_name) + int *tapfd, char **tapfdName, int tapfdSize, + int *vhostfd, char **vhostfdName, int vhostfdSize) { int ret = -1; - VIR_DEBUG("mon=%p netdevstr=%s tapfd=%d tapfd_name=%s " - "vhostfd=%d vhostfd_name=%s", - mon, netdevstr, tapfd, NULLSTR(tapfd_name), - vhostfd, NULLSTR(vhostfd_name)); + int i = 0, j = 0; + + VIR_DEBUG("mon=%p netdevstr=%s tapfd=%p tapfdName=%p tapfdSize=%d" + "vhostfd=%p vhostfdName=%p vhostfdSize=%d", + mon, netdevstr, tapfd, tapfdName, tapfdSize, + vhostfd, vhostfdName, tapfdSize); if (!mon) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -2542,12 +2544,13 @@ int qemuMonitorAddNetdev(qemuMonitorPtr mon, return -1; } - if (tapfd >= 0 && qemuMonitorSendFileHandle(mon, tapfd_name, tapfd) < 0) - return -1; - if (vhostfd >= 0 && - qemuMonitorSendFileHandle(mon, vhostfd_name, vhostfd) < 0) { - vhostfd = -1; - goto cleanup; + for (i = 0; i < tapfdSize; i++) { + if (qemuMonitorSendFileHandle(mon, tapfdName[i], tapfd[i]) < 0) + goto cleanup; + } + for (j = 0; j < vhostfdSize; j++) { + if (qemuMonitorSendFileHandle(mon, vhostfdName[j], vhostfd[j]) < 0) + goto cleanup; } if (mon->json) @@ -2557,10 +2560,14 @@ int qemuMonitorAddNetdev(qemuMonitorPtr mon, cleanup: if (ret < 0) { - if (tapfd >= 0 && qemuMonitorCloseFileHandle(mon, tapfd_name) < 0) - VIR_WARN("failed to close device handle '%s'", tapfd_name); - if (vhostfd >= 0 && qemuMonitorCloseFileHandle(mon, vhostfd_name) < 0) - VIR_WARN("failed to close device handle '%s'", vhostfd_name); + while (i--) { + if (qemuMonitorCloseFileHandle(mon, tapfdName[i]) < 0) + VIR_WARN("failed to close device handle '%s'", tapfdName[i]); + } + while (j--) { + if (qemuMonitorCloseFileHandle(mon, vhostfdName[j]) < 0) + VIR_WARN("failed to close device handle '%s'", vhostfdName[j]); + } } return ret; diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index f39f009..580e42b 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -506,8 +506,8 @@ int qemuMonitorRemoveHostNetwork(qemuMonitorPtr mon, int qemuMonitorAddNetdev(qemuMonitorPtr mon, const char *netdevstr, - int tapfd, const char *tapfd_name, - int vhostfd, const char *vhostfd_name); + int *tapfd, char **tapfdName, int tapfdSize, + int *vhostfd, char **vhostfdName, int vhostfdSize); int qemuMonitorRemoveNetdev(qemuMonitorPtr mon, const char *alias); -- 1.8.2.1
On 05/13/2013 01:23 PM, Michal Privoznik wrote:
Currently, only one tapfd and one vhostfd could be passed. However, multiqueue network requires several FDs to be passed to qemu so we must adapt out monitor handling functions to cope with that. --- src/qemu/qemu_hotplug.c | 7 +++++-- src/qemu/qemu_monitor.c | 39 +++++++++++++++++++++++---------------- src/qemu/qemu_monitor.h | 4 ++-- 3 files changed, 30 insertions(+), 20 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index d037c9d..b04f3bb 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -813,8 +813,11 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, qemuDomainObjEnterMonitor(driver, vm); if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { - if (qemuMonitorAddNetdev(priv->mon, netstr, tapfd, tapfd_name, - vhostfd, vhostfd_name) < 0) { + if (qemuMonitorAddNetdev(priv->mon, netstr, + &tapfd, &tapfd_name, + tapfd_name ? 1 : 0, + &vhostfd, &vhostfd_name, + vhostfd_name ? 1 : 0) < 0) {
That looks a bit obtuse, but of course it's all going to be corrected in another patch or two...
qemuDomainObjExitMonitor(driver, vm); virDomainAuditNet(vm, NULL, net, "attach", false); goto cleanup; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 4deb2d6..c3f35b5 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2527,14 +2527,16 @@ int qemuMonitorRemoveHostNetwork(qemuMonitorPtr mon,
int qemuMonitorAddNetdev(qemuMonitorPtr mon, const char *netdevstr, - int tapfd, const char *tapfd_name, - int vhostfd, const char *vhostfd_name) + int *tapfd, char **tapfdName, int tapfdSize, + int *vhostfd, char **vhostfdName, int vhostfdSize) { int ret = -1; - VIR_DEBUG("mon=%p netdevstr=%s tapfd=%d tapfd_name=%s " - "vhostfd=%d vhostfd_name=%s", - mon, netdevstr, tapfd, NULLSTR(tapfd_name), - vhostfd, NULLSTR(vhostfd_name)); + int i = 0, j = 0; + + VIR_DEBUG("mon=%p netdevstr=%s tapfd=%p tapfdName=%p tapfdSize=%d" + "vhostfd=%p vhostfdName=%p vhostfdSize=%d", + mon, netdevstr, tapfd, tapfdName, tapfdSize, + vhostfd, vhostfdName, tapfdSize);
The unfortunate thing here is that the debug log no longer displays the actual fd, even in the common case of there being a single queue. On the other hand, I've *never* gotten any use out of any of the "top of the function" debug log messages anyway :-)
if (!mon) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -2542,12 +2544,13 @@ int qemuMonitorAddNetdev(qemuMonitorPtr mon, return -1; }
- if (tapfd >= 0 && qemuMonitorSendFileHandle(mon, tapfd_name, tapfd) < 0) - return -1; - if (vhostfd >= 0 && - qemuMonitorSendFileHandle(mon, vhostfd_name, vhostfd) < 0) { - vhostfd = -1; - goto cleanup; + for (i = 0; i < tapfdSize; i++) { + if (qemuMonitorSendFileHandle(mon, tapfdName[i], tapfd[i]) < 0) + goto cleanup; + } + for (j = 0; j < vhostfdSize; j++) { + if (qemuMonitorSendFileHandle(mon, vhostfdName[j], vhostfd[j]) < 0) + goto cleanup; }
if (mon->json) @@ -2557,10 +2560,14 @@ int qemuMonitorAddNetdev(qemuMonitorPtr mon,
cleanup: if (ret < 0) { - if (tapfd >= 0 && qemuMonitorCloseFileHandle(mon, tapfd_name) < 0) - VIR_WARN("failed to close device handle '%s'", tapfd_name); - if (vhostfd >= 0 && qemuMonitorCloseFileHandle(mon, vhostfd_name) < 0) - VIR_WARN("failed to close device handle '%s'", vhostfd_name); + while (i--) { + if (qemuMonitorCloseFileHandle(mon, tapfdName[i]) < 0) + VIR_WARN("failed to close device handle '%s'", tapfdName[i]); + } + while (j--) { + if (qemuMonitorCloseFileHandle(mon, vhostfdName[j]) < 0) + VIR_WARN("failed to close device handle '%s'", vhostfdName[j]); + } }
return ret; diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index f39f009..580e42b 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -506,8 +506,8 @@ int qemuMonitorRemoveHostNetwork(qemuMonitorPtr mon,
int qemuMonitorAddNetdev(qemuMonitorPtr mon, const char *netdevstr, - int tapfd, const char *tapfd_name, - int vhostfd, const char *vhostfd_name); + int *tapfd, char **tapfdName, int tapfdSize, + int *vhostfd, char **vhostfdName, int vhostfdSize);
int qemuMonitorRemoveNetdev(qemuMonitorPtr mon, const char *alias);
ACK.
Currently, only one tapfd and one vhostfd could be passed. However, multiqueue network requires several FDs to be passed to qemu so we must adapt out monitor handling functions to cope with that. --- src/qemu/qemu_hotplug.c | 7 +++++-- src/qemu/qemu_monitor.c | 39 +++++++++++++++++++++++---------------- src/qemu/qemu_monitor.h | 4 ++-- 3 files changed, 30 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b04f3bb..41431b3 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -823,8 +823,11 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, goto cleanup; } } else { - if (qemuMonitorAddHostNetwork(priv->mon, netstr, tapfd, tapfd_name, - vhostfd, vhostfd_name) < 0) { + if (qemuMonitorAddHostNetwork(priv->mon, netstr, + &tapfd, &tapfd_name, + tapfd_name ? 1 : 0, + &vhostfd, &vhostfd_name, + vhostfd_name ? 1 : 0) < 0) { qemuDomainObjExitMonitor(driver, vm); virDomainAuditNet(vm, NULL, net, "attach", false); goto cleanup; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index c3f35b5..905801a 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2461,14 +2461,16 @@ cleanup: int qemuMonitorAddHostNetwork(qemuMonitorPtr mon, const char *netstr, - int tapfd, const char *tapfd_name, - int vhostfd, const char *vhostfd_name) + int *tapfd, char **tapfdName, int tapfdSize, + int *vhostfd, char **vhostfdName, int vhostfdSize) { int ret = -1; - VIR_DEBUG("mon=%p netstr=%s tapfd=%d tapfd_name=%s " - "vhostfd=%d vhostfd_name=%s", - mon, netstr, tapfd, NULLSTR(tapfd_name), - vhostfd, NULLSTR(vhostfd_name)); + int i = 0, j = 0; + + VIR_DEBUG("mon=%p netstr=%s tapfd=%p tapfdName=%p tapfdSize=%d " + "vhostfd=%p vhostfdName=%p vhostfdSize=%d", + mon, netstr, tapfd, tapfdName, tapfdSize, + vhostfd, vhostfdName, vhostfdSize); if (!mon) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -2476,12 +2478,13 @@ int qemuMonitorAddHostNetwork(qemuMonitorPtr mon, return -1; } - if (tapfd >= 0 && qemuMonitorSendFileHandle(mon, tapfd_name, tapfd) < 0) - return -1; - if (vhostfd >= 0 && - qemuMonitorSendFileHandle(mon, vhostfd_name, vhostfd) < 0) { - vhostfd = -1; - goto cleanup; + for (i = 0; i < tapfdSize; i++) { + if (qemuMonitorSendFileHandle(mon, tapfdName[i], tapfd[i]) < 0) + goto cleanup; + } + for (j = 0; j < vhostfdSize; j++) { + if (qemuMonitorSendFileHandle(mon, vhostfdName[j], vhostfd[j]) < 0) + goto cleanup; } if (mon->json) @@ -2492,10 +2495,14 @@ int qemuMonitorAddHostNetwork(qemuMonitorPtr mon, cleanup: if (ret < 0) { - if (tapfd >= 0 && qemuMonitorCloseFileHandle(mon, tapfd_name) < 0) - VIR_WARN("failed to close device handle '%s'", tapfd_name); - if (vhostfd >= 0 && qemuMonitorCloseFileHandle(mon, vhostfd_name) < 0) - VIR_WARN("failed to close device handle '%s'", vhostfd_name); + while (i--) { + if (qemuMonitorCloseFileHandle(mon, tapfdName[i]) < 0) + VIR_WARN("failed to close device handle '%s'", tapfdName[i]); + } + while (j--) { + if (qemuMonitorCloseFileHandle(mon, vhostfdName[j]) < 0) + VIR_WARN("failed to close device handle '%s'", vhostfdName[j]); + } } return ret; diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 580e42b..45257a4 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -497,8 +497,8 @@ int qemuMonitorRemoveFd(qemuMonitorPtr mon, int fdset, int fd); */ int qemuMonitorAddHostNetwork(qemuMonitorPtr mon, const char *netstr, - int tapfd, const char *tapfd_name, - int vhostfd, const char *vhostfd_name); + int *tapfd, char **tapfdName, int tapfdSize, + int *vhostfd, char **vhostfdName, int vhostfdSize); int qemuMonitorRemoveHostNetwork(qemuMonitorPtr mon, int vlan, -- 1.8.2.1
On 05/13/2013 01:23 PM, Michal Privoznik wrote:
Currently, only one tapfd and one vhostfd could be passed. However, multiqueue network requires several FDs to be passed to qemu so we must adapt out monitor handling functions to cope with that.
This is the same patch as 03/12. How did you manage that?
--- src/qemu/qemu_hotplug.c | 7 +++++-- src/qemu/qemu_monitor.c | 39 +++++++++++++++++++++++---------------- src/qemu/qemu_monitor.h | 4 ++-- 3 files changed, 30 insertions(+), 20 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b04f3bb..41431b3 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -823,8 +823,11 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, goto cleanup; } } else { - if (qemuMonitorAddHostNetwork(priv->mon, netstr, tapfd, tapfd_name, - vhostfd, vhostfd_name) < 0) { + if (qemuMonitorAddHostNetwork(priv->mon, netstr, + &tapfd, &tapfd_name, + tapfd_name ? 1 : 0, + &vhostfd, &vhostfd_name, + vhostfd_name ? 1 : 0) < 0) { qemuDomainObjExitMonitor(driver, vm); virDomainAuditNet(vm, NULL, net, "attach", false); goto cleanup; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index c3f35b5..905801a 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2461,14 +2461,16 @@ cleanup:
int qemuMonitorAddHostNetwork(qemuMonitorPtr mon, const char *netstr, - int tapfd, const char *tapfd_name, - int vhostfd, const char *vhostfd_name) + int *tapfd, char **tapfdName, int tapfdSize, + int *vhostfd, char **vhostfdName, int vhostfdSize) { int ret = -1; - VIR_DEBUG("mon=%p netstr=%s tapfd=%d tapfd_name=%s " - "vhostfd=%d vhostfd_name=%s", - mon, netstr, tapfd, NULLSTR(tapfd_name), - vhostfd, NULLSTR(vhostfd_name)); + int i = 0, j = 0; + + VIR_DEBUG("mon=%p netstr=%s tapfd=%p tapfdName=%p tapfdSize=%d " + "vhostfd=%p vhostfdName=%p vhostfdSize=%d", + mon, netstr, tapfd, tapfdName, tapfdSize, + vhostfd, vhostfdName, vhostfdSize);
if (!mon) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -2476,12 +2478,13 @@ int qemuMonitorAddHostNetwork(qemuMonitorPtr mon, return -1; }
- if (tapfd >= 0 && qemuMonitorSendFileHandle(mon, tapfd_name, tapfd) < 0) - return -1; - if (vhostfd >= 0 && - qemuMonitorSendFileHandle(mon, vhostfd_name, vhostfd) < 0) { - vhostfd = -1; - goto cleanup; + for (i = 0; i < tapfdSize; i++) { + if (qemuMonitorSendFileHandle(mon, tapfdName[i], tapfd[i]) < 0) + goto cleanup; + } + for (j = 0; j < vhostfdSize; j++) { + if (qemuMonitorSendFileHandle(mon, vhostfdName[j], vhostfd[j]) < 0) + goto cleanup; }
if (mon->json) @@ -2492,10 +2495,14 @@ int qemuMonitorAddHostNetwork(qemuMonitorPtr mon,
cleanup: if (ret < 0) { - if (tapfd >= 0 && qemuMonitorCloseFileHandle(mon, tapfd_name) < 0) - VIR_WARN("failed to close device handle '%s'", tapfd_name); - if (vhostfd >= 0 && qemuMonitorCloseFileHandle(mon, vhostfd_name) < 0) - VIR_WARN("failed to close device handle '%s'", vhostfd_name); + while (i--) { + if (qemuMonitorCloseFileHandle(mon, tapfdName[i]) < 0) + VIR_WARN("failed to close device handle '%s'", tapfdName[i]); + } + while (j--) { + if (qemuMonitorCloseFileHandle(mon, vhostfdName[j]) < 0) + VIR_WARN("failed to close device handle '%s'", vhostfdName[j]); + } }
return ret; diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 580e42b..45257a4 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -497,8 +497,8 @@ int qemuMonitorRemoveFd(qemuMonitorPtr mon, int fdset, int fd); */ int qemuMonitorAddHostNetwork(qemuMonitorPtr mon, const char *netstr, - int tapfd, const char *tapfd_name, - int vhostfd, const char *vhostfd_name); + int *tapfd, char **tapfdName, int tapfdSize, + int *vhostfd, char **vhostfdName, int vhostfdSize);
int qemuMonitorRemoveHostNetwork(qemuMonitorPtr mon, int vlan,
The qemuBuildHostNetStr() function which is responsible for generating command line for a network interface needs to be aware of multiqueue network interface as we are required to use: - fd=%d in case of one FD - fds=%d:%d:%d:...:%d in case of multiple FDs These two approaches can't be mixed. Same applies for vhost FDs. --- src/qemu/qemu_command.c | 46 +++++++++++++++++++++++++++++++++++++--------- src/qemu/qemu_command.h | 6 ++++-- src/qemu/qemu_hotplug.c | 10 ++++++---- 3 files changed, 47 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7775471..ec061d1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4091,13 +4091,16 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, virQEMUDriverPtr driver, char type_sep, int vlan, - const char *tapfd, - const char *vhostfd) + char **tapfd, + int tapfdSize, + char **vhostfd, + int vhostfdSize) { bool is_tap = false; virBuffer buf = VIR_BUFFER_INITIALIZER; enum virDomainNetType netType = virDomainNetGetActualType(net); virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + int i; if (net->script && netType != VIR_DOMAIN_NET_TYPE_ETHERNET) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -4116,7 +4119,19 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_NETWORK: case VIR_DOMAIN_NET_TYPE_DIRECT: - virBufferAsprintf(&buf, "tap%cfd=%s", type_sep, tapfd); + virBufferAsprintf(&buf, "tap%c", type_sep); + /* 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]); + } else { + virBufferAddLit(&buf, "fds="); + for (i = 0; i < tapfdSize; i++) { + if (i) + virBufferAddChar(&buf, ':'); + virBufferAdd(&buf, tapfd[i], -1); + } + } type_sep = ','; is_tap = true; break; @@ -4176,8 +4191,19 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, } if (is_tap) { - if (vhostfd && *vhostfd) - virBufferAsprintf(&buf, ",vhost=on,vhostfd=%s", vhostfd); + if (vhostfdSize) { + virBufferAddLit(&buf, ",vhost=on,"); + if (vhostfdSize == 1) { + virBufferAsprintf(&buf, "vhostfd=%s", vhostfd[0]); + } else { + virBufferAddLit(&buf, "vhostfds="); + for (i = 0; i < vhostfdSize; i++) { + if (i) + virBufferAddChar(&buf, ':'); + virBufferAdd(&buf, vhostfd[i], -1); + } + } + } if (net->tune.sndbuf_specified) virBufferAsprintf(&buf, ",sndbuf=%lu", net->tune.sndbuf); } @@ -6436,8 +6462,9 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { if (!(host = qemuBuildHostNetStr(net, driver, - ',', vlan, tapfdName, - vhostfdName))) + ',', vlan, + &tapfdName, tapfdName ? 1 : 0, + &vhostfdName, vhostfdName ? 1 : 0))) goto cleanup; virCommandAddArgList(cmd, "-netdev", host, NULL); } @@ -6453,8 +6480,9 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, if (!(virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))) { if (!(host = qemuBuildHostNetStr(net, driver, - ',', vlan, tapfdName, - vhostfdName))) + ',', vlan, + &tapfdName, tapfdName ? 1 : 0, + &vhostfdName, vhostfdName ? 1 : 0))) goto cleanup; virCommandAddArgList(cmd, "-net", host, NULL); } diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index ba42bb9..1068b4d 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -73,8 +73,10 @@ char * qemuBuildHostNetStr(virDomainNetDefPtr net, virQEMUDriverPtr driver, char type_sep, int vlan, - const char *tapfd, - const char *vhostfd); + char **tapfd, + int tapfdSize, + char **vhostfd, + int vhostfdSize); /* Legacy, pre device support */ char * qemuBuildNicStr(virDomainNetDefPtr net, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 41431b3..0a1845a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -800,13 +800,15 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (!(netstr = qemuBuildHostNetStr(net, driver, - ',', -1, tapfd_name, - vhostfd_name))) + ',', -1, + &tapfd_name, tapfd_name ? 1 : 0, + &vhostfd_name, vhostfd_name ? 1 : 0))) goto cleanup; } else { if (!(netstr = qemuBuildHostNetStr(net, driver, - ' ', vlan, tapfd_name, - vhostfd_name))) + ' ', vlan, + &tapfd_name, tapfd_name ? 1 : 0, + &vhostfd_name, vhostfd_name ? 1 : 0))) goto cleanup; } -- 1.8.2.1
On 05/13/2013 01:23 PM, Michal Privoznik wrote:
The qemuBuildHostNetStr() function which is responsible for generating command line for a network interface needs to be aware of multiqueue network interface as we are required to use: - fd=%d in case of one FD - fds=%d:%d:%d:...:%d in case of multiple FDs
These two approaches can't be mixed. Same applies for vhost FDs. --- src/qemu/qemu_command.c | 46 +++++++++++++++++++++++++++++++++++++--------- src/qemu/qemu_command.h | 6 ++++-- src/qemu/qemu_hotplug.c | 10 ++++++---- 3 files changed, 47 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7775471..ec061d1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4091,13 +4091,16 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, virQEMUDriverPtr driver, char type_sep, int vlan, - const char *tapfd, - const char *vhostfd) + char **tapfd, + int tapfdSize, + char **vhostfd, + int vhostfdSize) { bool is_tap = false; virBuffer buf = VIR_BUFFER_INITIALIZER; enum virDomainNetType netType = virDomainNetGetActualType(net); virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + int i;
if (net->script && netType != VIR_DOMAIN_NET_TYPE_ETHERNET) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -4116,7 +4119,19 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_NETWORK: case VIR_DOMAIN_NET_TYPE_DIRECT: - virBufferAsprintf(&buf, "tap%cfd=%s", type_sep, tapfd); + virBufferAsprintf(&buf, "tap%c", type_sep); + /* 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]); + } else { + virBufferAddLit(&buf, "fds="); + for (i = 0; i < tapfdSize; i++) { + if (i) + virBufferAddChar(&buf, ':'); + virBufferAdd(&buf, tapfd[i], -1); + } + } type_sep = ','; is_tap = true; break; @@ -4176,8 +4191,19 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, }
if (is_tap) { - if (vhostfd && *vhostfd) - virBufferAsprintf(&buf, ",vhost=on,vhostfd=%s", vhostfd); + if (vhostfdSize) { + virBufferAddLit(&buf, ",vhost=on,"); + if (vhostfdSize == 1) { + virBufferAsprintf(&buf, "vhostfd=%s", vhostfd[0]); + } else { + virBufferAddLit(&buf, "vhostfds="); + for (i = 0; i < vhostfdSize; i++) { + if (i) + virBufferAddChar(&buf, ':'); + virBufferAdd(&buf, vhostfd[i], -1); + } + } + } if (net->tune.sndbuf_specified) virBufferAsprintf(&buf, ",sndbuf=%lu", net->tune.sndbuf); } @@ -6436,8 +6462,9 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { if (!(host = qemuBuildHostNetStr(net, driver, - ',', vlan, tapfdName, - vhostfdName))) + ',', vlan, + &tapfdName, tapfdName ? 1 : 0, + &vhostfdName, vhostfdName ? 1 : 0))) goto cleanup; virCommandAddArgList(cmd, "-netdev", host, NULL); } @@ -6453,8 +6480,9 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, if (!(virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))) { if (!(host = qemuBuildHostNetStr(net, driver, - ',', vlan, tapfdName, - vhostfdName))) + ',', vlan, + &tapfdName, tapfdName ? 1 : 0, + &vhostfdName, vhostfdName ? 1 : 0))) goto cleanup; virCommandAddArgList(cmd, "-net", host, NULL); } diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index ba42bb9..1068b4d 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -73,8 +73,10 @@ char * qemuBuildHostNetStr(virDomainNetDefPtr net, virQEMUDriverPtr driver, char type_sep, int vlan, - const char *tapfd, - const char *vhostfd); + char **tapfd, + int tapfdSize, + char **vhostfd, + int vhostfdSize);
/* Legacy, pre device support */ char * qemuBuildNicStr(virDomainNetDefPtr net, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 41431b3..0a1845a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -800,13 +800,15 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (!(netstr = qemuBuildHostNetStr(net, driver, - ',', -1, tapfd_name, - vhostfd_name))) + ',', -1, + &tapfd_name, tapfd_name ? 1 : 0, + &vhostfd_name, vhostfd_name ? 1 : 0))) goto cleanup; } else { if (!(netstr = qemuBuildHostNetStr(net, driver, - ' ', vlan, tapfd_name, - vhostfd_name))) + ' ', vlan, + &tapfd_name, tapfd_name ? 1 : 0, + &vhostfd_name, vhostfd_name ? 1 : 0))) goto cleanup; }
ACK
Currently, the function knows how to open a TAP device for a single time. However, in case of multiqueue network we need to open it multiple times. Moreover, when doing TUNSETIFF ioctl, the IFF_MULTI_QUEUE flag shall be requested. This commit changes a behaviour slightly as well. Till now it was possible to pass NULL as an @fd address by which caller didn't care about returned FD. While this was used only in UML driver, the appropriate UML code snippets has been changed as well. --- src/network/bridge_driver.c | 2 +- src/qemu/qemu_command.c | 2 +- src/uml/uml_conf.c | 5 +- src/util/virnetdevtap.c | 115 ++++++++++++++++++++++++-------------------- src/util/virnetdevtap.h | 2 + 5 files changed, 72 insertions(+), 54 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 99c1316..f81e925 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2444,7 +2444,7 @@ networkStartNetworkVirtual(struct network_driver *driver, /* Keep tun fd open and interface up to allow for IPv6 DAD to happen */ if (virNetDevTapCreateInBridgePort(network->def->bridge, &macTapIfName, &network->def->mac, - NULL, &tapfd, NULL, NULL, + NULL, &tapfd, 1, NULL, NULL, VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE | VIR_NETDEV_TAP_CREATE_IFUP | VIR_NETDEV_TAP_CREATE_PERSIST) < 0) { diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ec061d1..e4bb2b7 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -355,7 +355,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, if (cfg->privileged) err = virNetDevTapCreateInBridgePort(brname, &net->ifname, &net->mac, - def->uuid, &tapfd, + def->uuid, &tapfd, 1, virDomainNetGetActualVirtPortProfile(net), virDomainNetGetActualVlan(net), tap_create_flags); diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c index 52b705c..3fda7e4 100644 --- a/src/uml/uml_conf.c +++ b/src/uml/uml_conf.c @@ -109,6 +109,7 @@ umlConnectTapDevice(virConnectPtr conn, const char *bridge) { bool template_ifname = false; + int tapfd; if (!net->ifname || STRPREFIX(net->ifname, VIR_NET_GENERATED_PREFIX) || @@ -121,7 +122,7 @@ umlConnectTapDevice(virConnectPtr conn, } if (virNetDevTapCreateInBridgePort(bridge, &net->ifname, &net->mac, - vm->uuid, NULL, + vm->uuid, &tapfd, 1, virDomainNetGetActualVirtPortProfile(net), virDomainNetGetActualVlan(net), VIR_NETDEV_TAP_CREATE_IFUP | @@ -139,9 +140,11 @@ umlConnectTapDevice(virConnectPtr conn, } } + VIR_FORCE_CLOSE(tapfd); return 0; error: + VIR_FORCE_CLOSE(tapfd); return -1; } diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index 75599db..ee839e3 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -140,7 +140,8 @@ virNetDevProbeVnetHdr(int tapfd) /** * virNetDevTapCreate: * @ifname: the interface name - * @tapfd: file descriptor return value for the new tap device + * @tapfds: file descriptor return value for the new tap device + * @tapfdSize: size of @tapfd * @flags: OR of virNetDevTapCreateFlags. Only one flag is recognized: * * VIR_NETDEV_TAP_CREATE_VNET_HDR @@ -148,76 +149,85 @@ virNetDevProbeVnetHdr(int tapfd) * VIR_NETDEV_TAP_CREATE_PERSIST * - The device will persist after the file descriptor is closed * - * Creates a tap interface. - * If the @tapfd parameter is supplied, the open tap device file descriptor - * will be returned, otherwise the TAP device will be closed. The caller must - * use virNetDevTapDelete to remove a persistent TAP device when it is no - * longer needed. + * Creates a tap interface. The caller must use virNetDevTapDelete to + * remove a persistent TAP device when it is no longer needed. In case + * @tapfdSize is greater than one, multiqueue extension is requested + * from kernel. * * Returns 0 in case of success or -1 on failure. */ int virNetDevTapCreate(char **ifname, int *tapfd, + int tapfdSize, unsigned int flags) { - int fd; + int i; struct ifreq ifr; int ret = -1; - if ((fd = open("/dev/net/tun", O_RDWR)) < 0) { - virReportSystemError(errno, "%s", - _("Unable to open /dev/net/tun, is tun module loaded?")); - return -1; - } - memset(&ifr, 0, sizeof(ifr)); + for (i = 0; i < tapfdSize; i++) { + int fd; - ifr.ifr_flags = IFF_TAP|IFF_NO_PI; + if ((fd = open("/dev/net/tun", O_RDWR)) < 0) { + virReportSystemError(errno, "%s", + _("Unable to open /dev/net/tun, is tun module loaded?")); + goto cleanup; + } + + memset(&ifr, 0, sizeof(ifr)); + + ifr.ifr_flags = IFF_TAP | IFF_NO_PI; + /* If tapfdSize is greater than one, request multiqueue */ + if (tapfdSize > 1) + ifr.ifr_flags |= IFF_MULTI_QUEUE; # ifdef IFF_VNET_HDR - if ((flags & VIR_NETDEV_TAP_CREATE_VNET_HDR) && - virNetDevProbeVnetHdr(fd)) - ifr.ifr_flags |= IFF_VNET_HDR; + if ((flags & VIR_NETDEV_TAP_CREATE_VNET_HDR) && + virNetDevProbeVnetHdr(fd)) + ifr.ifr_flags |= IFF_VNET_HDR; # endif - if (virStrcpyStatic(ifr.ifr_name, *ifname) == NULL) { - virReportSystemError(ERANGE, - _("Network interface name '%s' is too long"), - *ifname); - goto cleanup; + if (virStrcpyStatic(ifr.ifr_name, *ifname) == NULL) { + virReportSystemError(ERANGE, + _("Network interface name '%s' is too long"), + *ifname); + goto cleanup; - } + } - if (ioctl(fd, TUNSETIFF, &ifr) < 0) { - virReportSystemError(errno, - _("Unable to create tap device %s"), - NULLSTR(*ifname)); - goto cleanup; - } + if (ioctl(fd, TUNSETIFF, &ifr) < 0) { + virReportSystemError(errno, + _("Unable to create tap device %s"), + NULLSTR(*ifname)); + goto cleanup; + } - if ((flags & VIR_NETDEV_TAP_CREATE_PERSIST) && - (errno = ioctl(fd, TUNSETPERSIST, 1))) { - virReportSystemError(errno, - _("Unable to set tap device %s to persistent"), - NULLSTR(*ifname)); - goto cleanup; - } + if (i == 0) { + /* In case we are looping more than once, set other + * TAPs to have the same name */ + VIR_FREE(*ifname); + if (ifr.ifr_name && VIR_STRDUP(*ifname, ifr.ifr_name) < 0) + goto cleanup; + } - VIR_FREE(*ifname); - if (!(*ifname = strdup(ifr.ifr_name))) { - virReportOOMError(); - goto cleanup; + if ((flags & VIR_NETDEV_TAP_CREATE_PERSIST) && + (errno = ioctl(fd, TUNSETPERSIST, 1))) { + virReportSystemError(errno, + _("Unable to set tap device %s to persistent"), + NULLSTR(*ifname)); + goto cleanup; + } + tapfd[i] = fd; } - if (tapfd) - *tapfd = fd; - else - VIR_FORCE_CLOSE(fd); ret = 0; cleanup: - if (ret < 0) - VIR_FORCE_CLOSE(fd); + if (ret < 0) { + for (i = 0; i < tapfdSize; i++) + VIR_FORCE_CLOSE(tapfd[i]); + } return ret; } @@ -266,6 +276,7 @@ cleanup: #else /* ! TUNSETIFF */ int virNetDevTapCreate(char **ifname ATTRIBUTE_UNUSED, int *tapfd ATTRIBUTE_UNUSED, + int tapfdSize ATTRIBUTE_UNUSED, unsigned int flags ATTRIBUTE_UNUSED) { virReportSystemError(ENOSYS, "%s", @@ -286,7 +297,8 @@ int virNetDevTapDelete(const char *ifname ATTRIBUTE_UNUSED) * @brname: the bridge name * @ifname: the interface name (or name template) * @macaddr: desired MAC address - * @tapfd: file descriptor return value for the new tap device + * @tapfd: array of file descriptor return value for the new tap device + * @tapfdSize: size of @tapfd * @virtPortProfile: bridge/port specific configuration * @flags: OR of virNetDevTapCreateFlags: @@ -314,6 +326,7 @@ int virNetDevTapCreateInBridgePort(const char *brname, const virMacAddrPtr macaddr, const unsigned char *vmuuid, int *tapfd, + int tapfdSize, virNetDevVPortProfilePtr virtPortProfile, virNetDevVlanPtr virtVlan, unsigned int flags) @@ -321,7 +334,7 @@ int virNetDevTapCreateInBridgePort(const char *brname, virMacAddr tapmac; char macaddrstr[VIR_MAC_STRING_BUFLEN]; - if (virNetDevTapCreate(ifname, tapfd, flags) < 0) + if (virNetDevTapCreate(ifname, tapfd, tapfdSize, flags) < 0) return -1; /* We need to set the interface MAC before adding it @@ -372,9 +385,9 @@ int virNetDevTapCreateInBridgePort(const char *brname, return 0; - error: - if (tapfd) - VIR_FORCE_CLOSE(*tapfd); +error: + while (tapfdSize) + VIR_FORCE_CLOSE(tapfd[--tapfdSize]); return -1; } diff --git a/src/util/virnetdevtap.h b/src/util/virnetdevtap.h index 6bfc80c..cb6c284 100644 --- a/src/util/virnetdevtap.h +++ b/src/util/virnetdevtap.h @@ -29,6 +29,7 @@ int virNetDevTapCreate(char **ifname, int *tapfd, + int tapfdSize, unsigned int flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; @@ -55,6 +56,7 @@ int virNetDevTapCreateInBridgePort(const char *brname, const virMacAddrPtr macaddr, const unsigned char *vmuuid, int *tapfd, + int tapfdSize, virNetDevVPortProfilePtr virtPortProfile, virNetDevVlanPtr virtVlan, unsigned int flags) -- 1.8.2.1
On 05/13/2013 01:23 PM, Michal Privoznik wrote:
Currently, the function knows how to open a TAP device for a single time. However, in case of multiqueue network we need to open it multiple times. Moreover, when doing TUNSETIFF ioctl, the IFF_MULTI_QUEUE flag shall be requested. This commit changes a behaviour slightly as well. Till now it was possible to pass NULL as an @fd address by which caller didn't care about returned FD. While this was used only in UML driver, the appropriate UML code snippets has been changed as well. --- src/network/bridge_driver.c | 2 +- src/qemu/qemu_command.c | 2 +- src/uml/uml_conf.c | 5 +- src/util/virnetdevtap.c | 115 ++++++++++++++++++++++++-------------------- src/util/virnetdevtap.h | 2 + 5 files changed, 72 insertions(+), 54 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 99c1316..f81e925 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2444,7 +2444,7 @@ networkStartNetworkVirtual(struct network_driver *driver, /* Keep tun fd open and interface up to allow for IPv6 DAD to happen */ if (virNetDevTapCreateInBridgePort(network->def->bridge, &macTapIfName, &network->def->mac, - NULL, &tapfd, NULL, NULL, + NULL, &tapfd, 1, NULL, NULL, VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE | VIR_NETDEV_TAP_CREATE_IFUP | VIR_NETDEV_TAP_CREATE_PERSIST) < 0) { diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ec061d1..e4bb2b7 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -355,7 +355,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
if (cfg->privileged) err = virNetDevTapCreateInBridgePort(brname, &net->ifname, &net->mac, - def->uuid, &tapfd, + def->uuid, &tapfd, 1, virDomainNetGetActualVirtPortProfile(net), virDomainNetGetActualVlan(net), tap_create_flags); diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c index 52b705c..3fda7e4 100644 --- a/src/uml/uml_conf.c +++ b/src/uml/uml_conf.c @@ -109,6 +109,7 @@ umlConnectTapDevice(virConnectPtr conn, const char *bridge) { bool template_ifname = false; + int tapfd;
if (!net->ifname || STRPREFIX(net->ifname, VIR_NET_GENERATED_PREFIX) || @@ -121,7 +122,7 @@ umlConnectTapDevice(virConnectPtr conn, }
if (virNetDevTapCreateInBridgePort(bridge, &net->ifname, &net->mac, - vm->uuid, NULL, + vm->uuid, &tapfd, 1, virDomainNetGetActualVirtPortProfile(net), virDomainNetGetActualVlan(net), VIR_NETDEV_TAP_CREATE_IFUP | @@ -139,9 +140,11 @@ umlConnectTapDevice(virConnectPtr conn, } }
+ VIR_FORCE_CLOSE(tapfd); return 0;
error: + VIR_FORCE_CLOSE(tapfd); return -1; }
diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index 75599db..ee839e3 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -140,7 +140,8 @@ virNetDevProbeVnetHdr(int tapfd) /** * virNetDevTapCreate: * @ifname: the interface name - * @tapfd: file descriptor return value for the new tap device + * @tapfds: file descriptor return value for the new tap device + * @tapfdSize: size of @tapfd * @flags: OR of virNetDevTapCreateFlags. Only one flag is recognized: * * VIR_NETDEV_TAP_CREATE_VNET_HDR @@ -148,76 +149,85 @@ virNetDevProbeVnetHdr(int tapfd) * VIR_NETDEV_TAP_CREATE_PERSIST * - The device will persist after the file descriptor is closed * - * Creates a tap interface. - * If the @tapfd parameter is supplied, the open tap device file descriptor - * will be returned, otherwise the TAP device will be closed. The caller must - * use virNetDevTapDelete to remove a persistent TAP device when it is no - * longer needed. + * Creates a tap interface. The caller must use virNetDevTapDelete to + * remove a persistent TAP device when it is no longer needed. In case + * @tapfdSize is greater than one, multiqueue extension is requested + * from kernel. * * Returns 0 in case of success or -1 on failure. */ int virNetDevTapCreate(char **ifname, int *tapfd, + int tapfdSize, unsigned int flags) { - int fd; + int i; struct ifreq ifr; int ret = -1;
- if ((fd = open("/dev/net/tun", O_RDWR)) < 0) { - virReportSystemError(errno, "%s", - _("Unable to open /dev/net/tun, is tun module loaded?")); - return -1; - } - memset(&ifr, 0, sizeof(ifr)); + for (i = 0; i < tapfdSize; i++) { + int fd;
- ifr.ifr_flags = IFF_TAP|IFF_NO_PI; + if ((fd = open("/dev/net/tun", O_RDWR)) < 0) { + virReportSystemError(errno, "%s", + _("Unable to open /dev/net/tun, is tun module loaded?")); + goto cleanup; + }
If there is a failure anywhere between here and the next time we get to the top of the loop, we will leak fd.
+ + memset(&ifr, 0, sizeof(ifr)); + + ifr.ifr_flags = IFF_TAP | IFF_NO_PI; + /* If tapfdSize is greater than one, request multiqueue */ + if (tapfdSize > 1) + ifr.ifr_flags |= IFF_MULTI_QUEUE;
# ifdef IFF_VNET_HDR - if ((flags & VIR_NETDEV_TAP_CREATE_VNET_HDR) && - virNetDevProbeVnetHdr(fd)) - ifr.ifr_flags |= IFF_VNET_HDR; + if ((flags & VIR_NETDEV_TAP_CREATE_VNET_HDR) && + virNetDevProbeVnetHdr(fd)) + ifr.ifr_flags |= IFF_VNET_HDR; # endif
- if (virStrcpyStatic(ifr.ifr_name, *ifname) == NULL) { - virReportSystemError(ERANGE, - _("Network interface name '%s' is too long"), - *ifname); - goto cleanup; + if (virStrcpyStatic(ifr.ifr_name, *ifname) == NULL) { + virReportSystemError(ERANGE, + _("Network interface name '%s' is too long"), + *ifname); + goto cleanup;
- } + }
- if (ioctl(fd, TUNSETIFF, &ifr) < 0) { - virReportSystemError(errno, - _("Unable to create tap device %s"), - NULLSTR(*ifname)); - goto cleanup; - } + if (ioctl(fd, TUNSETIFF, &ifr) < 0) { + virReportSystemError(errno, + _("Unable to create tap device %s"), + NULLSTR(*ifname)); + goto cleanup; + }
- if ((flags & VIR_NETDEV_TAP_CREATE_PERSIST) && - (errno = ioctl(fd, TUNSETPERSIST, 1))) { - virReportSystemError(errno, - _("Unable to set tap device %s to persistent"), - NULLSTR(*ifname)); - goto cleanup; - } + if (i == 0) { + /* In case we are looping more than once, set other + * TAPs to have the same name */ + VIR_FREE(*ifname); + if (ifr.ifr_name && VIR_STRDUP(*ifname, ifr.ifr_name) < 0) + goto cleanup; + }
- VIR_FREE(*ifname); - if (!(*ifname = strdup(ifr.ifr_name))) { - virReportOOMError(); - goto cleanup; + if ((flags & VIR_NETDEV_TAP_CREATE_PERSIST) && + (errno = ioctl(fd, TUNSETPERSIST, 1))) { + virReportSystemError(errno, + _("Unable to set tap device %s to persistent"), + NULLSTR(*ifname)); + goto cleanup; + } + tapfd[i] = fd; } - if (tapfd) - *tapfd = fd; - else - VIR_FORCE_CLOSE(fd);
ret = 0;
cleanup: - if (ret < 0) - VIR_FORCE_CLOSE(fd); + if (ret < 0) { + for (i = 0; i < tapfdSize; i++) + VIR_FORCE_CLOSE(tapfd[i]); + }
If there is a failure when only some of the items in tapfd have been filled in, this could be very catastrophic (depending on what happens to be in the positions that haven't been set yet). You should only close the ones that have valid data in them.
return ret; } @@ -266,6 +276,7 @@ cleanup: #else /* ! TUNSETIFF */ int virNetDevTapCreate(char **ifname ATTRIBUTE_UNUSED, int *tapfd ATTRIBUTE_UNUSED, + int tapfdSize ATTRIBUTE_UNUSED, unsigned int flags ATTRIBUTE_UNUSED) { virReportSystemError(ENOSYS, "%s", @@ -286,7 +297,8 @@ int virNetDevTapDelete(const char *ifname ATTRIBUTE_UNUSED) * @brname: the bridge name * @ifname: the interface name (or name template) * @macaddr: desired MAC address - * @tapfd: file descriptor return value for the new tap device + * @tapfd: array of file descriptor return value for the new tap device + * @tapfdSize: size of @tapfd * @virtPortProfile: bridge/port specific configuration * @flags: OR of virNetDevTapCreateFlags:
@@ -314,6 +326,7 @@ int virNetDevTapCreateInBridgePort(const char *brname, const virMacAddrPtr macaddr, const unsigned char *vmuuid, int *tapfd, + int tapfdSize, virNetDevVPortProfilePtr virtPortProfile, virNetDevVlanPtr virtVlan, unsigned int flags) @@ -321,7 +334,7 @@ int virNetDevTapCreateInBridgePort(const char *brname, virMacAddr tapmac; char macaddrstr[VIR_MAC_STRING_BUFLEN];
- if (virNetDevTapCreate(ifname, tapfd, flags) < 0) + if (virNetDevTapCreate(ifname, tapfd, tapfdSize, flags) < 0) return -1;
/* We need to set the interface MAC before adding it @@ -372,9 +385,9 @@ int virNetDevTapCreateInBridgePort(const char *brname,
return 0;
- error: - if (tapfd) - VIR_FORCE_CLOSE(*tapfd); +error: + while (tapfdSize) + VIR_FORCE_CLOSE(tapfd[--tapfdSize]);
return -1; } diff --git a/src/util/virnetdevtap.h b/src/util/virnetdevtap.h index 6bfc80c..cb6c284 100644 --- a/src/util/virnetdevtap.h +++ b/src/util/virnetdevtap.h @@ -29,6 +29,7 @@
int virNetDevTapCreate(char **ifname, int *tapfd, + int tapfdSize, unsigned int flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
@@ -55,6 +56,7 @@ int virNetDevTapCreateInBridgePort(const char *brname, const virMacAddrPtr macaddr, const unsigned char *vmuuid, int *tapfd, + int tapfdSize, virNetDevVPortProfilePtr virtPortProfile, virNetDevVlanPtr virtVlan, unsigned int flags)
Otherwise looks okay.
With multiqueue network feature, we are advised to pass multiple vhost-net FDs as well. The ratio should be 1:1. Therefore we must alter the qemuOpenVhostNet function to allow that. --- src/qemu/qemu_command.c | 40 ++++++++++++++++++++++++++-------------- src/qemu/qemu_command.h | 3 ++- src/qemu/qemu_hotplug.c | 6 +++--- 3 files changed, 31 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e4bb2b7..13e3488 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -409,9 +409,13 @@ int qemuOpenVhostNet(virDomainDefPtr def, virDomainNetDefPtr net, virQEMUCapsPtr qemuCaps, - int *vhostfd) + int *vhostfd, + int vhostfdSize) { - *vhostfd = -1; /* assume we won't use vhost */ + int i; + + for (i = 0; i < vhostfdSize; i++) + vhostfd[i] = -1; /* assume we won't use vhost */ /* If the config says explicitly to not use vhost, return now */ if (net->driver.virtio.name == VIR_DOMAIN_NET_BACKEND_TYPE_QEMU) { @@ -444,20 +448,28 @@ qemuOpenVhostNet(virDomainDefPtr def, return 0; } - *vhostfd = open("/dev/vhost-net", O_RDWR); - virDomainAuditNetDevice(def, net, "/dev/vhost-net", *vhostfd >= 0); + for (i = 0; i < vhostfdSize; i++) { + vhostfd[i] = open("/dev/vhost-net", O_RDWR); + virDomainAuditNetDevice(def, net, "/dev/vhost-net", vhostfd[i] >= 0); - /* If the config says explicitly to use vhost and we couldn't open it, - * report an error. - */ - if ((*vhostfd < 0) && - (net->driver.virtio.name == VIR_DOMAIN_NET_BACKEND_TYPE_VHOST)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - "%s", _("vhost-net was requested for an interface, " - "but is unavailable")); - return -1; + /* If the config says explicitly to use vhost and we couldn't open it, + * report an error. + */ + if (vhostfd[i] < 0 && + net->driver.virtio.name == VIR_DOMAIN_NET_BACKEND_TYPE_VHOST) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("vhost-net was requested for an interface, " + "but is unavailable")); + goto error; + } } return 0; + +error: + for (i = 0; i < vhostfdSize; i++) + VIR_FORCE_CLOSE(vhostfd[i]); + + return -1; } int @@ -6431,7 +6443,7 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, network device */ int vhostfd; - if (qemuOpenVhostNet(def, net, qemuCaps, &vhostfd) < 0) + if (qemuOpenVhostNet(def, net, qemuCaps, &vhostfd, 1) < 0) goto cleanup; if (vhostfd >= 0) { virCommandTransferFD(cmd, vhostfd); diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 1068b4d..d9e620a 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -167,7 +167,8 @@ int qemuPhysIfaceConnect(virDomainDefPtr def, int qemuOpenVhostNet(virDomainDefPtr def, virDomainNetDefPtr net, virQEMUCapsPtr qemuCaps, - int *vhostfd); + int *vhostfd, + int vhostfdSize); int qemuNetworkPrepareDevices(virDomainDefPtr def); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 0a1845a..9ca1e43 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -738,7 +738,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, priv->qemuCaps)) < 0) goto cleanup; iface_connected = true; - if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd) < 0) + if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd, 1) < 0) goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { if ((tapfd = qemuPhysIfaceConnect(vm->def, driver, net, @@ -746,10 +746,10 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, VIR_NETDEV_VPORT_PROFILE_OP_CREATE)) < 0) goto cleanup; iface_connected = true; - if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd) < 0) + if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd, 1) < 0) goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET) { - if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd) < 0) + if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd, 1) < 0) goto cleanup; } -- 1.8.2.1
On 05/13/2013 01:23 PM, Michal Privoznik wrote:
With multiqueue network feature, we are advised to pass multiple vhost-net FDs as well. The ratio should be 1:1. Therefore we must alter the qemuOpenVhostNet function to allow that. --- src/qemu/qemu_command.c | 40 ++++++++++++++++++++++++++-------------- src/qemu/qemu_command.h | 3 ++- src/qemu/qemu_hotplug.c | 6 +++--- 3 files changed, 31 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e4bb2b7..13e3488 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -409,9 +409,13 @@ int qemuOpenVhostNet(virDomainDefPtr def, virDomainNetDefPtr net, virQEMUCapsPtr qemuCaps, - int *vhostfd) + int *vhostfd, + int vhostfdSize) { - *vhostfd = -1; /* assume we won't use vhost */ + int i; + + for (i = 0; i < vhostfdSize; i++) + vhostfd[i] = -1; /* assume we won't use vhost */
Would it maybe be cleaner for the caller if the args had int *vhostfdSize, and this function set it to 0 if it determined we didn't want to use vhostnet?
/* If the config says explicitly to not use vhost, return now */ if (net->driver.virtio.name == VIR_DOMAIN_NET_BACKEND_TYPE_QEMU) { @@ -444,20 +448,28 @@ qemuOpenVhostNet(virDomainDefPtr def, return 0; }
- *vhostfd = open("/dev/vhost-net", O_RDWR); - virDomainAuditNetDevice(def, net, "/dev/vhost-net", *vhostfd >= 0); + for (i = 0; i < vhostfdSize; i++) { + vhostfd[i] = open("/dev/vhost-net", O_RDWR); + virDomainAuditNetDevice(def, net, "/dev/vhost-net", vhostfd[i] >= 0);
- /* If the config says explicitly to use vhost and we couldn't open it, - * report an error. - */ - if ((*vhostfd < 0) && - (net->driver.virtio.name == VIR_DOMAIN_NET_BACKEND_TYPE_VHOST)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - "%s", _("vhost-net was requested for an interface, " - "but is unavailable")); - return -1; + /* If the config says explicitly to use vhost and we couldn't open it, + * report an error. + */ + if (vhostfd[i] < 0 && + net->driver.virtio.name == VIR_DOMAIN_NET_BACKEND_TYPE_VHOST) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("vhost-net was requested for an interface, " + "but is unavailable")); + goto error; + } } return 0; + +error: + for (i = 0; i < vhostfdSize; i++) + VIR_FORCE_CLOSE(vhostfd[i]);
This time you're saved by the fact that you've initialized vhostfd to all -1, but I still would prefer if you instead made this a while (i--) { } loop so that you never even look at items in the array that aren't valid fds. (and if you change it so that vhostfdSize can be changed to 0, you can then avoid initializing vhostfd[].)
+ + return -1; }
int @@ -6431,7 +6443,7 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, network device */ int vhostfd;
- if (qemuOpenVhostNet(def, net, qemuCaps, &vhostfd) < 0) + if (qemuOpenVhostNet(def, net, qemuCaps, &vhostfd, 1) < 0) goto cleanup; if (vhostfd >= 0) { virCommandTransferFD(cmd, vhostfd); diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 1068b4d..d9e620a 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -167,7 +167,8 @@ int qemuPhysIfaceConnect(virDomainDefPtr def, int qemuOpenVhostNet(virDomainDefPtr def, virDomainNetDefPtr net, virQEMUCapsPtr qemuCaps, - int *vhostfd); + int *vhostfd, + int vhostfdSize);
int qemuNetworkPrepareDevices(virDomainDefPtr def);
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 0a1845a..9ca1e43 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -738,7 +738,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, priv->qemuCaps)) < 0) goto cleanup; iface_connected = true; - if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd) < 0) + if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd, 1) < 0) goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { if ((tapfd = qemuPhysIfaceConnect(vm->def, driver, net, @@ -746,10 +746,10 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, VIR_NETDEV_VPORT_PROFILE_OP_CREATE)) < 0) goto cleanup; iface_connected = true; - if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd) < 0) + if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd, 1) < 0) goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET) { - if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd) < 0) + if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd, 1) < 0) goto cleanup; }
For future work it's better, if tapfd is passed as pointer. Moreover, we need to be able to return multiple values now. --- src/qemu/qemu_command.c | 89 ++++++++++++++++++++++++++----------------------- src/qemu/qemu_command.h | 4 ++- src/qemu/qemu_hotplug.c | 4 +-- 3 files changed, 53 insertions(+), 44 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 13e3488..3193c0f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -281,11 +281,12 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, virConnectPtr conn, virQEMUDriverPtr driver, virDomainNetDefPtr net, - virQEMUCapsPtr qemuCaps) + virQEMUCapsPtr qemuCaps, + int *tapfd, + int tapfdSize) { char *brname = NULL; - int err; - int tapfd = -1; + int ret = -1; unsigned int tap_create_flags = VIR_NETDEV_TAP_CREATE_IFUP; bool template_ifname = false; int actualType = virDomainNetGetActualType(net); @@ -297,7 +298,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, virNetworkPtr network = virNetworkLookupByName(conn, net->data.network.name); if (!network) - return -1; + return ret; active = virNetworkIsActive(network); if (active != 1) { @@ -322,18 +323,18 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, virFreeError(errobj); if (fail) - return -1; + return ret; } else if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { if (!(brname = strdup(virDomainNetGetActualBridgeName(net)))) { virReportOOMError(); - return -1; + return ret; } } else { virReportError(VIR_ERR_INTERNAL_ERROR, _("Network type %d is not supported"), virDomainNetGetActualType(net)); - return -1; + return ret; } if (!net->ifname || @@ -353,55 +354,61 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR; } - if (cfg->privileged) - err = virNetDevTapCreateInBridgePort(brname, &net->ifname, &net->mac, - def->uuid, &tapfd, 1, - virDomainNetGetActualVirtPortProfile(net), - virDomainNetGetActualVlan(net), - tap_create_flags); - else - err = qemuCreateInBridgePortWithHelper(cfg, brname, + if (cfg->privileged) { + if (virNetDevTapCreateInBridgePort(brname, &net->ifname, &net->mac, + def->uuid, tapfd, tapfdSize, + virDomainNetGetActualVirtPortProfile(net), + virDomainNetGetActualVlan(net), + tap_create_flags) < 0) { + virDomainAuditNetDevice(def, net, "/dev/net/tun", false); + goto cleanup; + } + } else { + if (qemuCreateInBridgePortWithHelper(cfg, brname, &net->ifname, - &tapfd, tap_create_flags); - - virDomainAuditNetDevice(def, net, "/dev/net/tun", tapfd >= 0); - if (err < 0) { - if (template_ifname) - VIR_FREE(net->ifname); - tapfd = -1; - } - - if (cfg->macFilter) { - if ((err = networkAllowMacOnPort(driver, net->ifname, &net->mac))) { - virReportSystemError(err, - _("failed to add ebtables rule to allow MAC address on '%s'"), - net->ifname); + tapfd, tap_create_flags) < 0) { + virDomainAuditNetDevice(def, net, "/dev/net/tun", false); + goto cleanup; } } - if (tapfd >= 0 && - virNetDevBandwidthSet(net->ifname, + virDomainAuditNetDevice(def, net, "/dev/net/tun", true); + + if (cfg->macFilter && + (ret = networkAllowMacOnPort(driver, net->ifname, &net->mac)) < 0) { + virReportSystemError(ret, + _("failed to add ebtables rule " + "to allow MAC address on '%s'"), + net->ifname); + } + + if (virNetDevBandwidthSet(net->ifname, virDomainNetGetActualBandwidth(net), false) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot set bandwidth limits on %s"), net->ifname); - VIR_FORCE_CLOSE(tapfd); goto cleanup; } - if (tapfd >= 0) { - if ((net->filter) && (net->ifname)) { - if (virDomainConfNWFilterInstantiate(conn, def->uuid, net) < 0) - VIR_FORCE_CLOSE(tapfd); - } - } + if (net->filter && net->ifname && + virDomainConfNWFilterInstantiate(conn, def->uuid, net) < 0) + goto cleanup; + + + ret = 0; cleanup: + if (ret < 0) { + while (tapfdSize--) + VIR_FORCE_CLOSE(tapfd[tapfdSize]); + if (template_ifname) + VIR_FREE(net->ifname); + } VIR_FREE(brname); virObjectUnref(cfg); - return tapfd; + return ret; } @@ -6426,8 +6433,8 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { - tapfd = qemuNetworkIfaceConnect(def, conn, driver, net, qemuCaps); - if (tapfd < 0) + if (qemuNetworkIfaceConnect(def, conn, driver, net, + qemuCaps, &tapfd, 1) < 0) goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { tapfd = qemuPhysIfaceConnect(def, driver, net, qemuCaps, vmop); diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index d9e620a..fecf7c3 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -155,7 +155,9 @@ int qemuNetworkIfaceConnect(virDomainDefPtr def, virConnectPtr conn, virQEMUDriverPtr driver, virDomainNetDefPtr net, - virQEMUCapsPtr qemuCaps) + virQEMUCapsPtr qemuCaps, + int *tapfd, + int tapfdSize) ATTRIBUTE_NONNULL(2); int qemuPhysIfaceConnect(virDomainDefPtr def, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 9ca1e43..e0d8a89 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -734,8 +734,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || actualType == VIR_DOMAIN_NET_TYPE_NETWORK) { - if ((tapfd = qemuNetworkIfaceConnect(vm->def, conn, driver, net, - priv->qemuCaps)) < 0) + if (qemuNetworkIfaceConnect(vm->def, conn, driver, net, + priv->qemuCaps, &tapfd, 1) < 0) goto cleanup; iface_connected = true; if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd, 1) < 0) -- 1.8.2.1
--- src/qemu/qemu_hotplug.c | 96 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 63 insertions(+), 33 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e0d8a89..256c54f 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -685,10 +685,12 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, virDomainNetDefPtr net) { qemuDomainObjPrivatePtr priv = vm->privateData; - char *tapfd_name = NULL; - int tapfd = -1; - char *vhostfd_name = NULL; - int vhostfd = -1; + char **tapfdName = NULL; + int *tapfd = NULL; + int tapfdSize = 0; + char **vhostfdName = NULL; + int *vhostfd = NULL; + int vhostfdSize = 0; char *nicstr = NULL; char *netstr = NULL; virNetDevVPortProfilePtr vport = NULL; @@ -699,6 +701,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, bool iface_connected = false; int actualType; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + int i; /* preallocate new slot for device */ if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets+1) < 0) { @@ -734,22 +737,37 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || actualType == VIR_DOMAIN_NET_TYPE_NETWORK) { + if (VIR_ALLOC(tapfd) < 0 || VIR_ALLOC(vhostfd) < 0) { + virReportOOMError(); + goto cleanup; + } + tapfdSize = vhostfdSize = 1; if (qemuNetworkIfaceConnect(vm->def, conn, driver, net, - priv->qemuCaps, &tapfd, 1) < 0) + priv->qemuCaps, tapfd, tapfdSize) < 0) goto cleanup; iface_connected = true; - if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd, 1) < 0) + if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd, vhostfdSize) < 0) goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { - if ((tapfd = qemuPhysIfaceConnect(vm->def, driver, net, - priv->qemuCaps, - VIR_NETDEV_VPORT_PROFILE_OP_CREATE)) < 0) + if (VIR_ALLOC(tapfd) < 0 || VIR_ALLOC(vhostfd) < 0) { + virReportOOMError(); + goto cleanup; + } + tapfdSize = vhostfdSize = 1; + if ((tapfd[0] = qemuPhysIfaceConnect(vm->def, driver, net, + priv->qemuCaps, + VIR_NETDEV_VPORT_PROFILE_OP_CREATE)) < 0) goto cleanup; iface_connected = true; - if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd, 1) < 0) + if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd, vhostfdSize) < 0) goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET) { - if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd, 1) < 0) + if (VIR_ALLOC(vhostfd) < 0) { + virReportOOMError(); + goto cleanup; + } + vhostfdSize = 1; + if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd, vhostfdSize) < 0) goto cleanup; } @@ -787,13 +805,19 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, } } - if (tapfd != -1) { - if (virAsprintf(&tapfd_name, "fd-%s", net->info.alias) < 0) + if (VIR_ALLOC_N(tapfdName, tapfdSize) < 0 || + VIR_ALLOC_N(vhostfdName, vhostfdSize) < 0) { + virReportOOMError(); + goto cleanup; + } + + for (i = 0; i < tapfdSize; i++) { + if (virAsprintf(&tapfdName[i], "fd-%s%d", net->info.alias, i) < 0) goto no_memory; } - if (vhostfd != -1) { - if (virAsprintf(&vhostfd_name, "vhostfd-%s", net->info.alias) < 0) + for (i = 0; i < vhostfdSize; i++) { + if (virAsprintf(&vhostfdName[i], "vhostfd-%s%d", net->info.alias, i) < 0) goto no_memory; } @@ -801,14 +825,14 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (!(netstr = qemuBuildHostNetStr(net, driver, ',', -1, - &tapfd_name, tapfd_name ? 1 : 0, - &vhostfd_name, vhostfd_name ? 1 : 0))) + tapfdName, tapfdSize, + vhostfdName, vhostfdSize))) goto cleanup; } else { if (!(netstr = qemuBuildHostNetStr(net, driver, ' ', vlan, - &tapfd_name, tapfd_name ? 1 : 0, - &vhostfd_name, vhostfd_name ? 1 : 0))) + tapfdName, tapfdSize, + vhostfdName, vhostfdSize))) goto cleanup; } @@ -816,20 +840,16 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuMonitorAddNetdev(priv->mon, netstr, - &tapfd, &tapfd_name, - tapfd_name ? 1 : 0, - &vhostfd, &vhostfd_name, - vhostfd_name ? 1 : 0) < 0) { + tapfd, tapfdName, tapfdSize, + vhostfd, vhostfdName, vhostfdSize) < 0) { qemuDomainObjExitMonitor(driver, vm); virDomainAuditNet(vm, NULL, net, "attach", false); goto cleanup; } } else { if (qemuMonitorAddHostNetwork(priv->mon, netstr, - &tapfd, &tapfd_name, - tapfd_name ? 1 : 0, - &vhostfd, &vhostfd_name, - vhostfd_name ? 1 : 0) < 0) { + tapfd, tapfdName, tapfdSize, + vhostfd, vhostfdName, vhostfdSize) < 0) { qemuDomainObjExitMonitor(driver, vm); virDomainAuditNet(vm, NULL, net, "attach", false); goto cleanup; @@ -837,8 +857,10 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, } qemuDomainObjExitMonitor(driver, vm); - VIR_FORCE_CLOSE(tapfd); - VIR_FORCE_CLOSE(vhostfd); + for (i = 0; i < tapfdSize; i++) + VIR_FORCE_CLOSE(tapfd[i]); + for (i = 0; i < vhostfdSize; i++) + VIR_FORCE_CLOSE(vhostfd[i]); if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -934,10 +956,18 @@ cleanup: VIR_FREE(nicstr); VIR_FREE(netstr); - VIR_FREE(tapfd_name); - VIR_FORCE_CLOSE(tapfd); - VIR_FREE(vhostfd_name); - VIR_FORCE_CLOSE(vhostfd); + for (i = 0; i < tapfdSize; i++) { + VIR_FORCE_CLOSE(tapfd[i]); + VIR_FREE(tapfdName[i]); + } + VIR_FREE(tapfd); + VIR_FREE(tapfdName); + for (i = 0; i < vhostfdSize; i++) { + VIR_FORCE_CLOSE(vhostfd[i]); + VIR_FREE(vhostfdName[i]); + } + VIR_FREE(vhostfd); + VIR_FREE(vhostfdName); virObjectUnref(cfg); return ret; -- 1.8.2.1
Currently, need for use of vhost-net is signalized by returning zero, and setting passed FD to a value different to negative one. However, when using multiple vhost-net devices, it is not so easy so we should use return value for that. --- src/qemu/qemu_command.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3193c0f..d466725 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -412,6 +412,19 @@ cleanup: } +/** + * qemuOpenVhostNet: + * @def: domain definition + * @net: network definition + * @qemuCaps: qemu binary capabilities + * @vhostfd: array of opened vhost-net device + * @vhostfdSize: size of @vhostfd array + * + * Open vhost-net, multiple times - if request. + * Returns: 1 if no vhost-net is required for @net type + * 0 on success + * -1 on failure + */ int qemuOpenVhostNet(virDomainDefPtr def, virDomainNetDefPtr net, @@ -425,9 +438,8 @@ qemuOpenVhostNet(virDomainDefPtr def, vhostfd[i] = -1; /* assume we won't use vhost */ /* If the config says explicitly to not use vhost, return now */ - if (net->driver.virtio.name == VIR_DOMAIN_NET_BACKEND_TYPE_QEMU) { - return 0; - } + if (net->driver.virtio.name == VIR_DOMAIN_NET_BACKEND_TYPE_QEMU) + return 1; /* If qemu doesn't support vhost-net mode (including the -netdev command * option), don't try to open the device. @@ -441,7 +453,7 @@ qemuOpenVhostNet(virDomainDefPtr def, "this QEMU binary")); return -1; } - return 0; + return 1; } /* If the nic model isn't virtio, don't try to open. */ @@ -452,7 +464,7 @@ qemuOpenVhostNet(virDomainDefPtr def, "virtio network interfaces")); return -1; } - return 0; + return 1; } for (i = 0; i < vhostfdSize; i++) { -- 1.8.2.1
--- src/qemu/qemu_command.c | 85 ++++++++++++++++++++++++++++++++++++------------- 1 file changed, 62 insertions(+), 23 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d466725..cdca2db 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6425,12 +6425,16 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, enum virNetDevVPortProfileOp vmop) { int ret = -1; - int tapfd = -1; char *nic = NULL, *host = NULL; - char *tapfdName = NULL; - char *vhostfdName = NULL; + int *tapfd = NULL; + int tapfdSize = 0; + int *vhostfd = NULL; + int vhostfdSize = 0; + char **tapfdName = NULL; + char **vhostfdName = NULL; int actualType = virDomainNetGetActualType(net); virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + int i; if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { /* NET_TYPE_HOSTDEV devices are really hostdev devices, so @@ -6445,12 +6449,24 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { + if (VIR_ALLOC(tapfd) < 0 || VIR_ALLOC(tapfdName) < 0) { + virReportOOMError(); + goto cleanup; + } + + tapfdSize = 1; if (qemuNetworkIfaceConnect(def, conn, driver, net, - qemuCaps, &tapfd, 1) < 0) + qemuCaps, tapfd, tapfdSize) < 0) goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { - tapfd = qemuPhysIfaceConnect(def, driver, net, qemuCaps, vmop); - if (tapfd < 0) + if (VIR_ALLOC(tapfd) < 0 || VIR_ALLOC(tapfdName) < 0) { + virReportOOMError(); + goto cleanup; + } + tapfdSize = 1; + tapfd[0] = qemuPhysIfaceConnect(def, driver, net, + qemuCaps, vmop); + if (tapfd[0] < 0) goto cleanup; } @@ -6458,30 +6474,41 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || actualType == VIR_DOMAIN_NET_TYPE_ETHERNET || actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { + int useVhost; /* Attempt to use vhost-net mode for these types of network device */ - int vhostfd; + if (VIR_ALLOC(vhostfd) < 0 || VIR_ALLOC(vhostfdName)) { + virReportOOMError(); + goto cleanup; + } + vhostfdSize = 1; + + useVhost = qemuOpenVhostNet(def, net, qemuCaps, vhostfd, vhostfdSize); + if (useVhost < 0) + goto cleanup; + if (useVhost > 0) + vhostfdSize = 0; + } - if (qemuOpenVhostNet(def, net, qemuCaps, &vhostfd, 1) < 0) + for (i = 0; i < tapfdSize; i++) { + if (virAsprintf(&tapfdName[i], "%d", tapfd[i]) < 0) { + virReportOOMError(); goto cleanup; - if (vhostfd >= 0) { - virCommandTransferFD(cmd, vhostfd); + } + virCommandTransferFD(cmd, tapfd[i]); + } + + for (i = 0; i < vhostfdSize; i++) { + if (vhostfd[i] >= 0) { + virCommandTransferFD(cmd, vhostfd[i]); - if (virAsprintf(&vhostfdName, "%d", vhostfd) < 0) { + if (virAsprintf(&vhostfdName[i], "%d", vhostfd[i]) < 0) { virReportOOMError(); goto cleanup; } } } - if (tapfd >= 0) { - if (virAsprintf(&tapfdName, "%d", tapfd) < 0) { - virReportOOMError(); - goto cleanup; - } - virCommandTransferFD(cmd, tapfd); - } - /* Possible combinations: * * 1. Old way: -net nic,model=e1000,vlan=1 -net tap,vlan=1 @@ -6494,8 +6521,8 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { if (!(host = qemuBuildHostNetStr(net, driver, ',', vlan, - &tapfdName, tapfdName ? 1 : 0, - &vhostfdName, vhostfdName ? 1 : 0))) + tapfdName, tapfdSize, + vhostfdName, vhostfdSize))) goto cleanup; virCommandAddArgList(cmd, "-netdev", host, NULL); } @@ -6512,8 +6539,8 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))) { if (!(host = qemuBuildHostNetStr(net, driver, ',', vlan, - &tapfdName, tapfdName ? 1 : 0, - &vhostfdName, vhostfdName ? 1 : 0))) + tapfdName, tapfdSize, + vhostfdName, vhostfdSize))) goto cleanup; virCommandAddArgList(cmd, "-net", host, NULL); } @@ -6522,6 +6549,18 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, cleanup: if (ret < 0) virDomainConfNWFilterTeardown(net); + for (i = 0; i < tapfdSize; i++) { + if (ret < 0) + VIR_FORCE_CLOSE(tapfd[i]); + VIR_FREE(tapfdName[i]); + } + for (i = 0; i < vhostfdSize; i++) { + if (ret < 0) + VIR_FORCE_CLOSE(vhostfd[i]); + VIR_FREE(vhostfdName[i]); + } + VIR_FREE(tapfd); + VIR_FREE(vhostfd); VIR_FREE(nic); VIR_FREE(host); VIR_FREE(tapfdName); -- 1.8.2.1
--- src/qemu/qemu_command.c | 11 +++++++---- src/qemu/qemu_hotplug.c | 5 +++-- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index cdca2db..c6ae820 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6449,12 +6449,14 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { - if (VIR_ALLOC(tapfd) < 0 || VIR_ALLOC(tapfdName) < 0) { + if (VIR_ALLOC_N(tapfd, net->driver.virtio.queues) < 0 || + VIR_ALLOC_N(tapfdName, net->driver.virtio.queues) < 0) { virReportOOMError(); goto cleanup; } - tapfdSize = 1; + tapfdSize = net->driver.virtio.queues; + if (qemuNetworkIfaceConnect(def, conn, driver, net, qemuCaps, tapfd, tapfdSize) < 0) goto cleanup; @@ -6477,11 +6479,12 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, int useVhost; /* Attempt to use vhost-net mode for these types of network device */ - if (VIR_ALLOC(vhostfd) < 0 || VIR_ALLOC(vhostfdName)) { + if (VIR_ALLOC_N(vhostfd, net->driver.virtio.queues) < 0 || + VIR_ALLOC_N(vhostfdName, net->driver.virtio.queues)) { virReportOOMError(); goto cleanup; } - vhostfdSize = 1; + vhostfdSize = net->driver.virtio.queues; useVhost = qemuOpenVhostNet(def, net, qemuCaps, vhostfd, vhostfdSize); if (useVhost < 0) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 256c54f..59fd645 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -737,11 +737,12 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || actualType == VIR_DOMAIN_NET_TYPE_NETWORK) { - if (VIR_ALLOC(tapfd) < 0 || VIR_ALLOC(vhostfd) < 0) { + if (VIR_ALLOC_N(tapfd, net->driver.virtio.queues) < 0 || + VIR_ALLOC_N(vhostfd, net->driver.virtio.queues) < 0) { virReportOOMError(); goto cleanup; } - tapfdSize = vhostfdSize = 1; + tapfdSize = vhostfdSize = net->driver.virtio.queues; if (qemuNetworkIfaceConnect(vm->def, conn, driver, net, priv->qemuCaps, tapfd, tapfdSize) < 0) goto cleanup; -- 1.8.2.1
participants (3)
-
Laine Stump -
Michal Privoznik -
Osier Yang