Re: [Qemu-devel] [PATCH] net: Fix link state inter-dependencies between NIC and backend

[Michael S. Tsirkin]

On Wed, Apr 01, 2015 at 07:55:38PM -0400, Vladislav Yasevich wrote:
> Commit 02d38fcb2caa4454cf4ed728d5908c3cc9ba47be
>     net: Update netdev peer on link change
> 
> introduced a link state dependency between the backend
> netdev and the nic.  If the admin turned off the link
> on the backend, the nic link state was set to down because
> the link had no access to the network at all.  This created
> some inconsitet behavior for someone who wanted to play
> around the links states.
>  1) Turning off the nic link and then turning on the backend
>     link (even if it was already on) would turn on the nic link
>     again.
>  2) Turning off the backend link and then turning on the nic
>     link would turn on the link in the VM, but would not change
>     the backend state resulting in a broken/unreachable network.
> 
> This patch attempts to correct these behaviors.  The patch treats
> the nic-backend relationship as two ends of a wire.  Each end tracks
> the administratively set link state in addition to actual link
> state.  Thus is is possible to set link down at each end effectively
> emulating plugging/unplugging the wire at either end.  The patch
> continues to preserve the old behavior where setting just
> nic side down does NOT impact the backend.
> 
> Signed-off-by: Vladislav Yasevich <address@hidden>

I never understood the point of
02d38fcb2caa4454cf4ed728d5908c3cc9ba47be.

If you want to tell guest link is down,
just set the nic link down.

How about we just revert it?

Then one can change link state on each end
independently, with no notification,
and no state to track.

> ---
>  include/net/net.h |  1 +
>  net/net.c         | 25 +++++++++++++++++++++++--
>  roms/ipxe         |  2 +-
>  roms/openbios     |  2 +-
>  roms/seabios      |  2 +-
>  5 files changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/include/net/net.h b/include/net/net.h
> index 50ffcb9..8d04861 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -78,6 +78,7 @@ typedef struct NetClientInfo {
>  struct NetClientState {
>      NetClientInfo *info;
>      int link_down;
> +    int admin_link_down;
>      QTAILQ_ENTRY(NetClientState) next;
>      NetClientState *peer;
>      NetQueue *incoming_queue;
> diff --git a/net/net.c b/net/net.c
> index 0be084d..be8de61 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1158,6 +1158,26 @@ void qmp_set_link(const char *name, bool up, Error 
> **errp)
>      }
>      nc = ncs[0];
>  
> +    /* Always set the administratively requested link state */
> +    nc->admin_link_down = !up;
> +
> +    /* If link status will not change, don't bother doing the work */
> +    if (nc->link_down == !up) {
> +        return;
> +    }
> +
> +    /* If this is a NIC device and the user is trying to turn the link on,
> +     * look at our peer link state first.
> +     * If the peer link state is down, then the peer was unpluged and
> +     * turning on the link state on this NIC will result in a broken network.
> +     * Report an error to the user.
> +     */
> +    if (nc->info->type == NET_CLIENT_OPTIONS_KIND_NIC && up &&
> +        nc->peer && nc->peer->link_down) {
> +        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, name);
> +        return;
> +    }
> +
>      for (i = 0; i < queues; i++) {
>          ncs[i]->link_down = !up;
>      }
> @@ -1166,8 +1186,9 @@ void qmp_set_link(const char *name, bool up, Error 
> **errp)
>          nc->info->link_status_changed(nc);
>      }
>  
> -    if (nc->peer) {
> -        /* Change peer link only if the peer is NIC and then notify peer.
> +    if (nc->peer && !nc->peer->admin_link_down) {
> +        /* Change peer link only if the peer is a NIC that has not been 
> forced
> +         * down and then notify peer.
>           * If the peer is a HUBPORT or a backend, we do not change the
>           * link status.
>           *
> diff --git a/roms/ipxe b/roms/ipxe
> index 35c5379..5de37e1 160000
> --- a/roms/ipxe
> +++ b/roms/ipxe
> @@ -1 +1 @@
> -Subproject commit 35c5379760aa1fea5e38f7a78b090f92bb7813ee
> +Subproject commit 5de37e124fd21c8f918f3fe26fc33a764709ead4
> diff --git a/roms/openbios b/roms/openbios
> index 5d3db90..d9e38ba 160000
> --- a/roms/openbios
> +++ b/roms/openbios
> @@ -1 +1 @@
> -Subproject commit 5d3db901435ef5a114c9d89461dd0f6d1ef1d44f
> +Subproject commit d9e38ba2ffd2d2cdfa840ea9bc7dd4a64472f2c7
> diff --git a/roms/seabios b/roms/seabios
> index 4adadbd..67d1fbe 160000
> --- a/roms/seabios
> +++ b/roms/seabios
> @@ -1 +1 @@
> -Subproject commit 4adadbde6904807de2e990c0af839ad0cc977806
> +Subproject commit 67d1fbef0f630e1e823f137d1bae7fa5790bcf4e
> -- 
> 1.9.3