[libvirt] [PATCH v2] qemu: fix ip/route assigning on ethernet device
After investigation, i found that ips and routes incorrectly assigning on device deletion. Also after libvirt create tap device it does not up. Vasiliy Tolstov (1): qemu: fix ethernet network type ip/route assign src/qemu/qemu_interface.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) -- 2.7.4
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@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; + } + if (virNetDevIPInfoAddToDev(net->ifname, &net->hostIP) < 0) + goto cleanup; + + break; + + case VIR_DOMAIN_NET_TYPE_USER: case VIR_DOMAIN_NET_TYPE_VHOSTUSER: case VIR_DOMAIN_NET_TYPE_SERVER: case VIR_DOMAIN_NET_TYPE_CLIENT: @@ -197,10 +214,6 @@ qemuInterfaceStopDevice(virDomainNetDefPtr net) } case VIR_DOMAIN_NET_TYPE_ETHERNET: - if (virNetDevIPInfoAddToDev(net->ifname, &net->hostIP) < 0) - goto cleanup; - break; - case VIR_DOMAIN_NET_TYPE_USER: case VIR_DOMAIN_NET_TYPE_VHOSTUSER: case VIR_DOMAIN_NET_TYPE_SERVER: -- 2.7.4
2016-08-24 19:09 GMT+03:00 Vasiliy Tolstov <v.tolstov@selfip.ru>:
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.
Forget please, incorrect version switch (dev->linkstate) -> switch (net->linkstate) { -- Vasiliy Tolstov, e-mail: v.tolstov@selfip.ru
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@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. (Note that macvtap devices are set online in this function because they're artificially kept offline when the macvtap device is created --> this is necessary because the macvtap and guest interface use the same MAC address, and setting it online early could cause unwanted traffic with that MAC address, thus prematurely directing traffic for the MAC address to this host. See this BZ for more details: https://bugzilla.redhat.com/show_bug.cgi?id=1113474
+ if (virNetDevIPInfoAddToDev(net->ifname, &net->hostIP) < 0) + goto cleanup;
ACK to moving this.
+ + break; + + case VIR_DOMAIN_NET_TYPE_USER: case VIR_DOMAIN_NET_TYPE_VHOSTUSER: case VIR_DOMAIN_NET_TYPE_SERVER: case VIR_DOMAIN_NET_TYPE_CLIENT: @@ -197,10 +214,6 @@ qemuInterfaceStopDevice(virDomainNetDefPtr net) }
case VIR_DOMAIN_NET_TYPE_ETHERNET: - if (virNetDevIPInfoAddToDev(net->ifname, &net->hostIP) < 0) - goto cleanup; - break; - case VIR_DOMAIN_NET_TYPE_USER: case VIR_DOMAIN_NET_TYPE_VHOSTUSER: case VIR_DOMAIN_NET_TYPE_SERVER:
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@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.
25 Авг 2016 г. 9:00 пользователь "Laine Stump" <laine@laine.org> написал:
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@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. Big thanks! I'm test it today.
25 Авг 2016 г. 9:00 пользователь "Laine Stump" <laine@laine.org> написал:
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@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. May be I miss something but in you patch you always up tap device, but if I create domain with link down in XML this not right. What do you think?
On 08/25/2016 02:59 AM, Vasiliy Tolstov wrote:
May be I miss something but in you patch you always up tap device, but if I create domain with link down in XML this not right. What do you think?
Until now, linkstate has always been used to control the online status of the emulated NIC in the guest, and the tap device has always been IFF_UP (and for a good reason. See my reply to you in the thread for my patches for more details...)
2016-08-25 18:17 GMT+03:00 Laine Stump <laine@laine.org>:
Until now, linkstate has always been used to control the online status of the emulated NIC in the guest, and the tap device has always been IFF_UP (and for a good reason. See my reply to you in the thread for my patches for more details...)
Yes, i commented with proposed solution to get that i need =) -- Vasiliy Tolstov, e-mail: v.tolstov@selfip.ru
participants (2)
-
Laine Stump -
Vasiliy Tolstov