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?