5:28 a.m.
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(a)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