[ovs-dev] [PATCH] datapath-windows: Remove Hyper-V port name / Do not set the NDIS port state

Eitan Eliahu eliahue at vmware.com
Wed Nov 12 17:08:40 UTC 2014


Thanks for the review Nithin.
We should not set the NDIS state to any state which is not what NDIS maintains. If the state sent by NDIS is not Teardown the OVS port state should not be Teardown (should probably be in error state or whatever sent by NDIS). Forwarding should be done according to the connection state of the port. In any case we should not have a maintain "NDIS port state" and set it by the driver.
Eitan

-----Original Message-----
From: Nithin Raju 
Sent: Wednesday, November 12, 2014 6:20 AM
To: Eitan Eliahu
Cc: <dev at openvswitch.org>
Subject: Re: [ovs-dev] [PATCH] datapath-windows: Remove Hyper-V port name / Do not set the NDIS port state

On Nov 12, 2014, at 2:03 AM, Eitan Eliahu <eliahue at vmware.com> wrote:

> [1] The NDIS port state should always reflect the port state maintained by
>    NDIS so it should never be directly updated.
> [2] Remove the "port name" field as we use the "friendly name" instead.
> 
> Signed-off-by: Eitan Eliahu <eliahue at vmware.com>

Eitan,
Nuking "port name" is fine. However, I don't understand what is the practical use of port state change. Perhaps a change like this suffices?

-        /* add assertion here
-         */
+        ASSERT(portParam->PortState == NdisSwitchPortStateTeardown);
-        vport->portState = NdisSwitchPortStateTeardown;
+        vport->portState = portParam->PortState;
        vport->ovsState = OVS_STATE_PORT_TEAR_DOWN;

One thing to note is that we rely a lot of vport->ovsState while processing packets. In OvsAddPorts() in Actions.c, we decide to forward packets to an NDIS port only if it in in the CONNECTED state. If we fail to update the state here to OVS_STATE_PORT_TEAR_DOWN, we might end up forwarding packets to a vport in !CONNECTED state, and that can lead to bug checks.

thanks,
-- Nithin




More information about the dev mailing list