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 <mpriv...@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
