[ovs-dev] [PATCH] datapath-windows - fix crash when internal port is removed

Eitan Eliahu eliahue at vmware.com
Fri Aug 1 00:22:22 UTC 2014


Thanks for the review Nithin.
1. ok.
2. ok.
3. If clearing the context from will be  removed from OvsCompleteNBLForwardingCtx then we need to call OvsClearTunCtx() on all error paths of this function. Apply it only to this path does not buy anything here.
4. This state is caused by misconfiguration. This should be blocked in user mode so I don't see a lot of meaning for incrementing a counter.

A better approach would be just to fail the driver Attach callback but I would defer it for later. This change is just to prevent the BSOD.

Eitan

-----Original Message-----
From: Nithin Raju 
Sent: Thursday, July 31, 2014 5:01 PM
To: Eitan Eliahu
Cc: <dev at openvswitch.org>
Subject: Re: [ovs-dev] [PATCH] datapath-windows - fix crash when internal port is removed


On Aug 1, 2014, at 12:17 AM, Eitan Eliahu <eliahue at vmware.com>
 wrote:

> BSOD while setting AllowManagementOS on $false #13
> https://urldefense.proofpoint.com/v1/url?u=https://github.com/openvswitch/ovs/issues/13&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=ubrOpIWavCMqX4l4j1LEVpTfDj%2FD5Qyn8KCoJIBGvzo%3D%0A&m=nJEdnLEDoAOVUJoqVm4ZcqCMbQj8d7Tyr5bgPJbq7Qs%3D%0A&s=924730f29356b7532a8ebc6c80a2a68ef41cb31cf81fad50bdf450683db9e796
> 
> Signed-off-by: Eitan Eliahu <eliahue at vmware.com>
> Reported-by: Alin Serdean <aserdean at cloudbasesolutions.com>

Thanks for doing this.

I had the following comments.
1. Pls. remove the URL from the commit message. It is getting translated into something unfriendly. You can mention the Issue ID #13. You can also explain the issue if you wish.
2. The drop string can be "OVS-Dropped since internal port is absent.", to keep it consistent with the other strings.
3. Please also clear the tunnel context (tunnelTxNic at the least) using OvsClearTunCtx() before the function returns. The general pattern is that the lower layer function "consumes" the packet and the context it is supposed to consume. Currently OvsCompleteNBLForwardingCtx() is clearing the context, but I think we should remove that code, in the future.
4. It would be great if a counter could be added similar to ovsActions.failedEncap to indicate this issue. We'll figure out how to export all these counters to userspace as we go. A log message is also OK.

thanks,
Nithin




More information about the dev mailing list