| Message ID | 20220531182741.1784-1-u9012063@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] datapath-windows: Detect truncate action. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 5/31/22 20:27, William Tu wrote: > Truncate action is not supported in Windows datapath, but > currently it incorrectly reports to ovs-vswitchd as supported blow > "system@ovs-system: Datapath supports truncate action". > The patches detects it and returns not support. > > Cc: Alin-Gabriel Serdean <aserdean@ovn.org> > Cc: Wilson Peng <pweisong@vmware.com> > Signed-off-by: William Tu <u9012063@gmail.com> > --- > datapath-windows/ovsext/Actions.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/datapath-windows/ovsext/Actions.c b/datapath-windows/ovsext/Actions.c > index 0f7f78932..3a5e71ee9 100644 > --- a/datapath-windows/ovsext/Actions.c > +++ b/datapath-windows/ovsext/Actions.c > @@ -2502,6 +2502,11 @@ OvsDoExecuteActions(POVS_SWITCH_CONTEXT switchContext, > } > break; > } > + case OVS_ACTION_ATTR_TRUNC: > + status = NDIS_STATUS_NOT_SUPPORTED; > + dropReason = L"OVS-truncate action not supported"; > + goto dropit; > + break; Hmm. I don't know much about windows datapath, but shouldn't this just be a behavior of the 'default' case? i.e.: default: status = NDIS_STATUS_NOT_SUPPORTED; dropReason = L"Unknown action"; // or generate the string // dynamically with a number? break; Current code seems to skip all the unknown actions. That doesn't seem correct. Best regards, Ilya Maximets.
On Tue, May 31, 2022 at 12:06 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 5/31/22 20:27, William Tu wrote: > > Truncate action is not supported in Windows datapath, but > > currently it incorrectly reports to ovs-vswitchd as supported blow > > "system@ovs-system: Datapath supports truncate action". > > The patches detects it and returns not support. > > > > Cc: Alin-Gabriel Serdean <aserdean@ovn.org> > > Cc: Wilson Peng <pweisong@vmware.com> > > Signed-off-by: William Tu <u9012063@gmail.com> > > --- > > datapath-windows/ovsext/Actions.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/datapath-windows/ovsext/Actions.c b/datapath-windows/ovsext/Actions.c > > index 0f7f78932..3a5e71ee9 100644 > > --- a/datapath-windows/ovsext/Actions.c > > +++ b/datapath-windows/ovsext/Actions.c > > @@ -2502,6 +2502,11 @@ OvsDoExecuteActions(POVS_SWITCH_CONTEXT switchContext, > > } > > break; > > } > > + case OVS_ACTION_ATTR_TRUNC: > > + status = NDIS_STATUS_NOT_SUPPORTED; > > + dropReason = L"OVS-truncate action not supported"; > > + goto dropit; > > + break; > > Hmm. I don't know much about windows datapath, but shouldn't > this just be a behavior of the 'default' case? i.e.: > > default: > status = NDIS_STATUS_NOT_SUPPORTED; > dropReason = L"Unknown action"; // or generate the string > // dynamically with a number? > break; > > Current code seems to skip all the unknown actions. > That doesn't seem correct. Thanks for taking a look and you're right. Current code simply skips if the action is not supported, and there is no warning/error message logged at all. Ideally ovs-vswitchd should detect the right action supported in datapath, and generate/translate the corresponding datapath flow from openflow flow. I will work on the next patch to drop the unsupported action. William
diff --git a/datapath-windows/ovsext/Actions.c b/datapath-windows/ovsext/Actions.c index 0f7f78932..3a5e71ee9 100644 --- a/datapath-windows/ovsext/Actions.c +++ b/datapath-windows/ovsext/Actions.c @@ -2502,6 +2502,11 @@ OvsDoExecuteActions(POVS_SWITCH_CONTEXT switchContext, } break; } + case OVS_ACTION_ATTR_TRUNC: + status = NDIS_STATUS_NOT_SUPPORTED; + dropReason = L"OVS-truncate action not supported"; + goto dropit; + break; default: status = NDIS_STATUS_NOT_SUPPORTED; break;
Truncate action is not supported in Windows datapath, but currently it incorrectly reports to ovs-vswitchd as supported blow "system@ovs-system: Datapath supports truncate action". The patches detects it and returns not support. Cc: Alin-Gabriel Serdean <aserdean@ovn.org> Cc: Wilson Peng <pweisong@vmware.com> Signed-off-by: William Tu <u9012063@gmail.com> --- datapath-windows/ovsext/Actions.c | 5 +++++ 1 file changed, 5 insertions(+)