The ASSERT there is because we expect that NDIS will send Teardown status. But
, the extension cannot control it and should not overwrite it.
It would better to not assume specific state set by NDIS. Since OvsAddPorts
adds NDIS/Hyper-V ports we should examine the NDIS port state rather the OVS
port state.
Does it make sense?
-----Original Message-----
From: Nithin Raju
Sent: Wednesday, November 12, 2014 10:46 AM
To: Eitan Eliahu
Cc: <dev@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 9:08 AM, Eitan Eliahu <elia...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
