[libvirt] [PATCH 4/4] qemu: Implement mtu on interface

[Michal Privoznik]

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 at 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