Re: [PATCH 3/4] domain_conf: Introduce <mtu/> to <interface/>

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 01/25/2017 03:46 PM, Laine Stump wrote:
> 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@xxxxxxxxxx>
>> ---
>>   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/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).

I think we are already doing that: virNetDevTapCreateInBridgePort()
calls virNetDevSetMTUFromDevice() which sets MTU on the new tap
interface from the one that bridge has.

> 
>>   </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.

True and usually I require the xml2xml test to be in the same patch as
the conf/ changes. However, this is an exception: because in the post
parse callback I check whether there's an implementation available for
given interface type. Since there is no implementation available in this
patch, <mtu size=''/> is not allowed and xml2xml test would just error out.

> 
> ACK, if you move the xml2xml test case to this patch.
> 

Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux