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? 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. Where end users getting it wrong and complaining? > > > >> 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 > >>>>> > >>>> > >>>