On 04/02/2014 11:29 AM, Michael S. Tsirkin wrote: > On Wed, Apr 02, 2014 at 11:25:32AM -0400, Vlad Yasevich wrote: >> On 04/02/2014 11:03 AM, Michael S. Tsirkin wrote: >>> On Wed, Apr 02, 2014 at 10:57:08AM -0400, Vlad Yasevich wrote: >>>> On 04/02/2014 06:46 AM, Yan Vugenfirer wrote: >>>>> >>>>> On Apr 2, 2014, at 11:51 AM, Michael S. Tsirkin <m...@redhat.com> wrote: >>>>> >>>>>> On Thu, Nov 21, 2013 at 09:05:51PM -0500, Vlad Yasevich wrote: >>>>>>> When a link change occurs on a backend (like tap), we currently do >>>>>>> not propage such change to the nic. As a result, when someone turns >>>>>>> off a link on a tap device, for instance, then a guest doesn't see >>>>>>> that change and continues to try to send traffic or run DHCP even >>>>>>> though the lower-layer is disconnected. This is OK when the network >>>>>>> is set up as a HUB since the the guest may be connected to other HUB >>>>>>> ports too, but when it's set up as a netdev, it makes thinkgs worse. >>>>>>> >>>>>>> The patch addresses this by setting the peers link down only when the >>>>>>> peer is not a HUBPORT device. With this patch, in the following config >>>>>>> -netdev tap,id=net0 -device e1000,mac=XXXXX,netdev=net0 >>>>>>> when net0 link is turned off, the guest e1000 shows lower-layer link >>>>>>> down. This allows guests to boot much faster in such configurations. >>>>>>> With windows guest, it also allows the network to recover properly >>>>>>> since windows will not configure the link-local IPv4 address, and >>>>>>> when the link is turned on, the proper address address is configured. >>>>>>> >>>>>>> Signed-off-by: Vlad Yasevich <vyase...@redhat.com> >>>>>>> --- >>>>>>> net/net.c | 26 +++++++++++++++++--------- >>>>>>> 1 file changed, 17 insertions(+), 9 deletions(-) >>>>>>> >>>>>>> diff --git a/net/net.c b/net/net.c >>>>>>> index 0a88e68..8a084bf 100644 >>>>>>> --- a/net/net.c >>>>>>> +++ b/net/net.c >>>>>>> @@ -1065,15 +1065,23 @@ void qmp_set_link(const char *name, bool up, >>>>>>> Error **errp) >>>>>>> nc->info->link_status_changed(nc); >>>>>>> } >>>>>>> >>>>>>> - /* Notify peer. Don't update peer link status: this makes it >>>>>>> possible to >>>>>>> - * disconnect from host network without notifying the guest. >>>>>>> - * FIXME: is disconnected link status change operation useful? >>>>>>> - * >>>>>>> - * Current behaviour is compatible with qemu vlans where there >>>>>>> could be >>>>>>> - * multiple clients that can still communicate with each other in >>>>>>> - * disconnected mode. For now maintain this compatibility. */ >>>>>> >>>>>> Hmm this went in without much discussion. >>>>>> >>>>>> But before this change went in, it was possible to control link state on >>>>>> NIC >>>>>> and on link separately, without guest noticing. >>>>>> >>>>>> Why did we decide to drop this functionality? >>>>> >>>>> Not sure there was a real need for the patch. >>>>> >>>>> On other hand Windows guest will not receive IP address from DHCP server >>>>> if you start VM with tap link down and then set it to up without toggling >>>>> link state of the guest device as well. >>>> >>>> Same for a linux guest. It a fault scenario. So we either handle >>>> it by propagating the link event, or we forbid it. Doing neither >>>> allows for a bad state in common configuration. >>> >>> It's how networking works. >>> If I turn off my router at home I can't get dhcp either. >> >> And you device link will show that it has not carrier. > > It doesn't as long as the switch it's connected to is up. > > >>> >>> We had a way to disable networking at link or at >>> the router, then removed the ability to turn it >>> off at the router and I'm asking why. >> >> We didn't remove the ability to turn it off at router/switch. >> We just now propagate carrier loss correctly. >> >> In fact, if you have a configuration where the VM is connected >> to a hub in qemu, turning off the link on the 'tap' connected >> to the same hub doesn't not change guest state. > > So why is it a good idea to make -netdev inconsistent > with this?
Because -netdev has a 1-1 relationship with -device. You could think of it a physical wire that connects the nic to the network. If something happens to the wire, the nic should notice it. > It doesn't seem to add anything useful. > It might fix things for users who turned off link > at the wrong place by mistake but I doubt it's > a common scenario. I don't think link state management in general is a common scenario, but we support it. I think this makes the behavior better and improves the vm experience. > > Where end users getting it wrong and complaining? Yes, I can provide you with a list reported bugs if you wish. -vlad > > >>> >>>> This patch chose to propagate the event under certain configuration. >>>> >>>> -vlad >>> >>> So don't do this then. >>> If you want guest to know that link is down, >>> put it down on guest side. >>> >>> It's that simple. >>> >> >> Sorry, but that's not always possible. The host administrator >> may not always be vm administrator and without this patch vm >> administrator has no idea what happened to his network. >> >> -vlad >>> >>> >>> >>>>> >>>>> Yan. >>>>> >>>>>> >>>>>> >>>>>> >>>>>>> - if (nc->peer && nc->peer->info->link_status_changed) { >>>>>>> - nc->peer->info->link_status_changed(nc->peer); >>>>>>> + if (nc->peer) { >>>>>>> + /* Change peer link only if the peer is NIC and then notify >>>>>>> peer. >>>>>>> + * If the peer is a HUBPORT or a backend, we do not change the >>>>>>> + * link status. >>>>>>> + * >>>>>>> + * This behavior is compatible with qemu vlans where there >>>>>>> could be >>>>>>> + * multiple clients that can still communicate with each other >>>>>>> in >>>>>>> + * disconnected mode. For now maintain this compatibility. >>>>>>> + */ >>>>>>> + if (nc->peer->info->type == NET_CLIENT_OPTIONS_KIND_NIC) { >>>>>>> + for (i = 0; i < queues; i++) { >>>>>>> + ncs[i]->peer->link_down = !up; >>>>>>> + } >>>>>>> + } >>>>>>> + if (nc->peer->info->link_status_changed) { >>>>>>> + nc->peer->info->link_status_changed(nc->peer); >>>>>>> + } >>>>>>> } >>>>>>> } >>>>>>> >>>>>>> -- >>>>>>> 1.8.4.2 >>>>>>> >>>>>> >>>>>