Re: [libvirt] [PATCH] qemu: fix ethernet network type ip/route assign
On 08/24/2016 02:32 PM, Laine Stump wrote:
On 08/24/2016 12:09 PM, Vasiliy Tolstov wrote:
> IP and routes assigenment incorrectly placed on device stop.
> This is fixing it, also change device state according to xml.
> Note that as i know in linux routes can't be created on device that does
> not up.
>
> Signed-off-by: Vasiliy Tolstov <v.tolstov(a)selfip.ru>
> ---
> src/qemu/qemu_interface.c | 23 ++++++++++++++++++-----
> 1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
> index e637d21fb77a..f80feff2d545 100644
> --- a/src/qemu/qemu_interface.c
> +++ b/src/qemu/qemu_interface.c
> @@ -108,8 +108,25 @@ qemuInterfaceStartDevice(virDomainNetDefPtr net)
> break;
> }
> - case VIR_DOMAIN_NET_TYPE_USER:
> case VIR_DOMAIN_NET_TYPE_ETHERNET:
> + switch (dev->linkstate) {
> + case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP:
> + case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DEFAULT:
> + if ((ret = virNetDevSetOnline(dev->ifname, true)) < 0)
> + goto cleanup;
> + break;
> +
> + case VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN:
> + if ((ret = virNetDevSetOnline(dev->ifname, false)) < 0)
> + goto cleanup;
> + break;
> + }
The Online/Offline handling of the tap device for ethernet devices
should be identical to that used for network/bridge network devices.
If something is necessary (and it may be), it should be in a separate
patch.
More on this - aside from code that you added in your patch 9717d6
"autocreate tap device for ethernet network type", the linkstate of a
device is only used to send qemu commands controlling the linkstate of
the *guest* device, but never to set the online status of the host side
of the tap device (several of us missed this during the review of
multiple versions of patches leading to 9717d6; heck, I didn't even
realize it when looking at this new patch until I went back and read
through all the code again)
This (plus a bit of investigation about when tap devices for bridge
networks are set online) leads to the following:
1) The above code is erroneous. It shouldn't be there at all.
2) The code in qemuDomainChangeNetLinkState() that modifies the online
status of the tap device when the interface is type='ethernet' also
should not be there.
3) The reason the tap interface isn't IFF_UP is that
virNetDevTapCreate() (called from qemuInterfaceEthernetConnect() in the
same patch 9717d6) *ignores the VIR_NETDEV_TAP_CREATE_IFUP flag* (and it
is documented this way). That's never been noticed before because the
only previous call to virNetDevTapCreate() is from
virNetDevTapCreateInBridgePort(), which sets IFF_UP on the tap device
itself after calling virNetDevTapCreate().
My assumption is that it was done this way possibly because the tap
device needed to be offline during some part of the initialization (or
possibly because that's just the way some function was split up during a
refactoring or something).
Anyway, I think there are 3 patches needed:
1) The part of your patch that just moves the virNetDevIPInfoAddToDev()
call.
2) A patch to remove the virNetDevSetOnline() from
qemuDomainChangeNetLinkState()
3) A patch to call virNetDevSetOnline() after calling virNetDevTapCreate()
I just modified your patch, created the other two patches, and sent the
resulting patch series to the list.
If you can try it out and verify that it works for you (it works for
me), then I'll push it in the morning ( US EST) so it's in before the
freeze.