Re: [Qemu-devel] [PATCH] net: Update netdev peer on link change

[Michael S. Tsirkin]

On Wed, Apr 02, 2014 at 01:46:14PM +0300, Yan Vugenfirer wrote:
> 
> On Apr 2, 2014, at 11:51 AM, Michael S. Tsirkin <address@hidden> 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 <address@hidden>
> >> ---
> >> 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.
> 
> Yan. 

So why not toggle the link of the guest device?

> > 
> > 
> > 
> >> -    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
> >> 
> >