On 01/24/2017 10:40 AM, Michal Privoznik wrote:

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatnetwork.html.in | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index 291dcea12..8638df96e 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -76,7 +76,7 @@ be used to set that attribute of the same name for each domain interface connected to this network (<span class="since">since 1.2.10</span>). See - the <a href="formatdomain.html#elementSNICS">Network + the <a href="formatdomain.html#elementsNICS">Network interfaces</a> section of the domain XML documentation for more details. Note that an explicit setting of this attribute in a portgroup or the individual domain interface will @@ -765,7 +765,7 @@ set that attribute of the same name for each domain interface using this portgroup (<span class="since">since 1.2.10</span>). See - the <a href="formatdomain.html#elementSNICS">Network + the <a href="formatdomain.html#elementsNICS">Network interfaces</a> section of the domain XML documentation for more details. Note that an explicit setting of this attribute in the portgroup overrides the network-wide setting, and an explicit

ACK

On 01/24/2017 10:40 AM, Michal Privoznik wrote:

We use @ret to hold the actual return value of the function we are currently in. To hold a return value of a function called we use different variables: @rv, @rc, etc. Honour this naming scheme in virDomainNetDefParseXML too.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b9092bdde..26bb0fdd0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9343,7 +9343,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, virNWFilterHashTablePtr filterparams = NULL; virDomainActualNetDefPtr actual = NULL; xmlNodePtr oldnode = ctxt->node; - int ret, val; + int rv, val;

if (VIR_ALLOC(def) < 0) return NULL; @@ -10041,10 +10041,10 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, } }

- ret = virXPathULong("string(./tune/sndbuf)", ctxt, &def->tune.sndbuf); - if (ret >= 0) { + rv = virXPathULong("string(./tune/sndbuf)", ctxt, &def->tune.sndbuf); + if (rv >= 0) { def->tune.sndbuf_specified = true; - } else if (ret == -2) { + } else if (rv == -2) { virReportError(VIR_ERR_XML_ERROR, "%s", _("sndbuf must be a positive integer")); goto error;

I guess you're doing this because you don't like the fact that it's called ret but we're not actually returning it? Yeah, I'll buy that <ObamaWithBeerThumbsUp/>. ACK.

On 01/24/2017 10:40 AM, Michal Privoznik wrote:

So far we allow to set MTU for libvirt networks. However, not all domain interfaces have to be plugged into a libvirt network and even if they are, they might want to have a different MTU (e.g. for testing purposes).

... although setting an MTU larger than a bridge's current MTU will result in the tap device MTU being lowered back to the bridge's MTU anyway, and setting an MTU smaller than the bridge's current MTU will result in the bridge's MTU being clamped to that new lower value. (Still, I think it's reasonable to expect someone someday will want to do that for some reason, so might as well allow it...)

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.html.in | 19 ++++++++ docs/schemas/domaincommon.rng | 3 ++ docs/schemas/networkcommon.rng | 9 ++++ src/conf/domain_conf.c | 10 +++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_domain.c | 29 ++++++++++++ src/qemu/qemu_domain.h | 2 + tests/qemuxml2argvdata/qemuxml2argv-net-mtu.xml | 60 +++++++++++++++++++++++++ 8 files changed, 133 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-mtu.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 3f7f87524..1bbece0e5 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -5215,6 +5215,25 @@ qemu-kvm -net nic,model=? /dev/null <span class="since">Since 0.9.5</span> </p>

+ <h5><a name="mtu">MTU configuration</a></h5> +<pre> +... +<devices> + <interface type='network'> + <source network='default'/> + <target dev='vnet0'/> + <b><mtu size='1500'/></b> + </interface> +</devices> +...</pre> + + <p> + This element provides means of setting MTU of the virtual network link. + Currently there is just one attribute <code>size</code> which accepts a + non-negative integer which specifies the MTU size for the interface. + <span class="since">Since 3.1.0</span> + </p>

This collection of words has an elevated level of elucidating with the verbiage and so on. It's all true though. :-)

+ <h5><a name="ipconfig">IP configuration</a></h5> <pre> ... diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 4d76315b0..cc6e0d0c0 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2465,6 +2465,9 @@ <empty/> </element> </optional> + <optional> + <ref name="mtu"/> + </optional> <optional> <element name="target"> <attribute name="dev"> diff --git a/docs/schemas/networkcommon.rng b/docs/schemas/networkcommon.rng index a334b83e3..26995556d 100644 --- a/docs/schemas/networkcommon.rng +++ b/docs/schemas/networkcommon.rng @@ -260,10 +260,19 @@ </optional> </element> </define> + <define name="macTableManager"> <choice> <value>kernel</value> <value>libvirt</value> </choice> </define> + + <define name="mtu"> + <element name="mtu"> + <attribute name="size"> + <ref name="unsignedShort"/> + </attribute> + </element> + </define>

Well, if you're going to add this, then I should use it in my patches, or I should do it and you use it in yours. Our relative timezones (and the fact that I haven't respun my patches yet) dictate that you should push first (anyway, I'm realizing I'll need to add an extra patch to mine for another issue - retrieving the MTU from the network in the case that it's not specified in the domain's interface element directly, similar to what we do with vlan tags).

</grammar> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 26bb0fdd0..a2b72cb9c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10050,6 +10050,12 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; }

+ if (virXPathUInt("string(./mtu/@size)", ctxt, &def->mtu) < -1) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("malformed mtu size")); + goto error; + } + cleanup: ctxt->node = oldnode; VIR_FREE(macaddr); @@ -21769,6 +21775,10 @@ virDomainNetDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<link state='%s'/>\n", virDomainNetInterfaceLinkStateTypeToString(def->linkstate)); } + + if (def->mtu) + virBufferAsprintf(buf, "<mtu size='%u'/>\n", def->mtu); + if (virDomainDeviceInfoFormat(buf, &def->info, flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT | VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM) < 0) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 78a3db4e1..4d830c51d 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1009,6 +1009,7 @@ struct _virDomainNetDef { virNetDevVlan vlan; int trustGuestRxFilters; /* enum virTristateBool */ int linkstate; + unsigned int mtu; };

/* Used for prefix of ifname of any network name generated dynamically diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2ed45ab17..26ca89930 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2868,6 +2868,14 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, _("rx_queue_size has to be a power of two")); goto cleanup; } + + if (net->mtu && + !qemuDomainNetSupportsMTU(net->type)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("setting MTU on interface type %s is not supported yet"), + virDomainNetTypeToString(net->type)); + goto cleanup; + } }

ret = 0; @@ -6529,6 +6537,27 @@ qemuDomainSupportsNetdev(virDomainDefPtr def, return virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV); }

+bool +qemuDomainNetSupportsMTU(virDomainNetType type) +{ + switch (type) { + case VIR_DOMAIN_NET_TYPE_USER: + case VIR_DOMAIN_NET_TYPE_ETHERNET: + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: + case VIR_DOMAIN_NET_TYPE_SERVER: + case VIR_DOMAIN_NET_TYPE_CLIENT: + case VIR_DOMAIN_NET_TYPE_MCAST: + case VIR_DOMAIN_NET_TYPE_NETWORK: + case VIR_DOMAIN_NET_TYPE_BRIDGE: + case VIR_DOMAIN_NET_TYPE_INTERNAL: + case VIR_DOMAIN_NET_TYPE_DIRECT: + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + case VIR_DOMAIN_NET_TYPE_UDP: + case VIR_DOMAIN_NET_TYPE_LAST: + break; + } + return false; +}

int qemuDomainNetVLAN(virDomainNetDefPtr def) diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 88b586972..041149167 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -708,6 +708,8 @@ bool qemuDomainSupportsNetdev(virDomainDefPtr def, virQEMUCapsPtr qemuCaps, virDomainNetDefPtr net);

+bool qemuDomainNetSupportsMTU(virDomainNetType type); + int qemuDomainNetVLAN(virDomainNetDefPtr def);

int qemuDomainSetPrivatePaths(virQEMUDriverPtr driver, diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-mtu.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-mtu.xml new file mode 100644 index 000000000..606322463 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-mtu.xml

You added this test case, but didn't reference it in qemuxml2xmltest.c (or add the corresponding file in qemuxml2xmloutdata). Ah, I see you did add it in patch 4/4, but I think the xml2xml test should be added in the same patch that the conf changes are made.

@@ -0,0 +1,60 @@ +<domain type='qemu'> + <name>test</name> + <uuid>15d091de-0181-456b-9554-e4382dc1f1ab</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='network'> + <source network='default'/> + <mac address='52:54:00:e5:48:58'/> + <model type='virtio'/> + <mtu size='1500'/> + </interface> + <interface type='network'> + <source network='default'/> + <mac address='52:54:00:e5:48:59'/> + <model type='virtio'/> + <mtu size='9000'/> + </interface> + <serial type='pty'> + <target port='0'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + </console> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'/> + </devices> +</domain>

Long test cases with lots of extra stuff already tested elsewhere make jtomko cry :-P. ACK, if you move the xml2xml test case to this patch.

On 01/25/2017 04:16 PM, Laine Stump wrote:

On 01/24/2017 10:40 AM, Michal Privoznik wrote:

...

Not only we should set the MTU on the host end of the device but also let qemu know what MTU did we set.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 14 +++++ src/qemu/qemu_domain.c | 7 ++- .../qemuxml2xmloutdata/qemuxml2xmlout-net-mtu.xml | 68 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 +

You added the test case for xml2xml in this patch, when it should have been added in the previous patch, and you didn't add the test case to qemuxml2argv.

Well, that's because we don't have a mock network driver for our qemu tests. Also, another problem is we do a resource allocation while building the command line. Had we a better separation, adding qemuxml2argv test case would be much simpler (and there would be no excuse for not doing so).

...

6 files changed, 90 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-net-mtu.xml

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 1e1b53b22..3247d2567 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -356,6 +356,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "drive-iotune-group", "query-cpu-model-expansion", /* 245 */ + "virtio-net.host_mtu", ); @@ -1642,6 +1643,7 @@ static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioNet[] = { { "tx", QEMU_CAPS_VIRTIO_TX_ALG }, { "event_idx", QEMU_CAPS_VIRTIO_NET_EVENT_IDX }, { "rx_queue_size", QEMU_CAPS_VIRTIO_NET_RX_QUEUE_SIZE }, + { "host_mtu", QEMU_CAPS_VIRTIO_NET_HOST_MTU },

I think if the enum says "VIRTIO_NET_HOST_MTU" then the string should be virtio_net_host_mtu, to eliminate possible ambiguity just in case a later version of qemu adds the host_mtu option to, e.g. the e1000e driver or something.

The string, our string that will be under <qemuCaps/> does say virtio-net.host_mtu. See the chunk above. This is how qemu named its parameter on the cmd line.

...

}; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioSCSI[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index b5ad95e46..95bb67d44 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -392,6 +392,7 @@ typedef enum { /* 245 */ QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION, /* qmp query-cpu-model-expansion */ + QEMU_CAPS_VIRTIO_NET_HOST_MTU, /* virtio-net-*.host_mtu */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d459f8e3e..6d6587235 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3678,6 +3678,16 @@ qemuBuildNicDevStr(virDomainDefPtr def, } virBufferAsprintf(&buf, ",rx_queue_size=%u", net->driver.virtio.rx_queue_size); } + + if (usingVirtio && net->mtu) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_HOST_MTU)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("setting MTU is not supported with this QEMU binary")); + goto error; + } + virBufferAsprintf(&buf, ",host_mtu=%u", net->mtu); + } + if (vlan == -1) virBufferAsprintf(&buf, ",netdev=host%s", net->info.alias); else @@ -8251,6 +8261,10 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, } } + if (net->mtu && + virNetDevSetMTU(net->ifname, net->mtu) < 0) + goto cleanup; + if ((actualType == VIR_DOMAIN_NET_TYPE_NETWORK || actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || actualType == VIR_DOMAIN_NET_TYPE_ETHERNET || diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 26ca89930..c6ce09021 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6541,14 +6541,15 @@ bool qemuDomainNetSupportsMTU(virDomainNetType type) { switch (type) { - case VIR_DOMAIN_NET_TYPE_USER: + case VIR_DOMAIN_NET_TYPE_NETWORK: + case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_ETHERNET: case VIR_DOMAIN_NET_TYPE_VHOSTUSER: + return true; + case VIR_DOMAIN_NET_TYPE_USER:

Hmm. Maybe this function was causing a validation failure when you tried to add the xml2xml test in the previous patch. So I guess it's okay to not add the xml2xml test until this patch (my preference would be to not add the extra validation until this patch, so that the previous one could have useful testing as a part of the same patch, but I suppose the end result is the same)

Yes. My reasoning when adding XML that looks untested in the previous patch was that in fact it is being tested. By our virschematest - so at least the RNG is tested.

Also, you didn't update news.xml :-) (either in this patch or the previous patch)

Ah, very good point.

ACK with the test added to xml2argv and the capabilities string changed to be more specific.

Fixed the news.xml and pushed. Thank you