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

[Nithin Raju]

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

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

This is not what I am suggesting. I understand that you think that setting 'vport->portState' to a hardcoded value of 'NdisSwitchPortStateTeardown' in the teardown OID handler is a bad idea, and instead you want to copy the value from 'portParam->PortState'.  Sure, I don't have any objection to it.

What I had comments was about the way the patch was written. To quote the patch:
> +        vport->portState = portParam->PortState;
I am fine with this.

> +        if (portParam->PortState == NdisSwitchPortStateTeardown) {
> +            vport->ovsState = OVS_STATE_PORT_TEAR_DOWN;

In the highly unlikely scenario that 'portParam->PortState' != NdisSwitchPortStateTeardown, we won't set 'vport->ovsState' to OVS_STATE_PORT_TEAR_DOWN. If we don't do this, then OvsAddPorts() might end up adding this vport to the list of destination ports while processing a packet, and that can lead to hitting a bug check if 'portParam->PortState' != NdisSwitchPortStateCreated.

> +        }
> +        else {
> +            /* Expect Teardown state to be set */
> +            ASSERT(portParam->PortState == NdisSwitchPortStateTeardown);
> +        }

I don't really get this point of this ASSERT().

Instead, what I suggested is to write the patch in what I thought was a simpler fashion:
> -        /* add assertion here
> -         */
> +        ASSERT(portParam->PortState == NdisSwitchPortStateTeardown);
> -        vport->portState = NdisSwitchPortStateTeardown;
> +        vport->portState = portParam->PortState;
>        vport->ovsState = OVS_STATE_PORT_TEAR_DOWN;


You might have a comment here saying, we are hardcoding 'vport->ovsState' to OVS_STATE_PORT_TEAR_DOWN. But, this is better than leaving it in OVS_STATE_PORT_CONNECTED and hitting a bugcheck if a packet gets forwarded to this port.

Stepping back a bit, can you practically reproduce a scenario where we get a OID_SWITCH_PORT_TEARDOWN call but the 'portParam->PortState' != NdisSwitchPortStateTeardown?

thanks,
-- Nithin