Hey Alin,
Thanks for the patch.
Please find my comments inline.
I have not worked on this code for sometime, so may miss something here.
Can you please also let us know the unit testing that you did? Although change
is minor, but editing FLOW_KEY* related code may end up breaking some
functionality.
Thanks.
Regards,
Ankur
________________________________________
From: dev <dev-boun...@openvswitch.org> on behalf of Alin Serdean
<aserd...@cloudbasesolutions.com>
Sent: Wednesday, February 4, 2015 6:19 AM
To: dev@openvswitch.org
Subject: [ovs-dev] [PATCH v2] datapath-windows: accommodate to UFID changes
Current flow commands: new, set, get, del need to respond with a NETLINK
error in the case OVS_FLOW_ATTR_KEY is missing.
OVS_FLOW_ATTR_KEY is now an optional attribute.
Also add OVS_FLOW_ATTR_UFID attribute to the kernel for further use.
v2 Remove redundant return code
Acked-by: Eitan Eliahu <eliahue at vmware.com>
Signed-off-by: Alin Gabriel Serdean <aserd...@cloudbasesolutions.com>
---
datapath-windows/ovsext/Flow.c | 25 ++++++++++++++++++++++---
1 file changed, 22 insertions(+), 3 deletions(-)
diff --git a/datapath-windows/ovsext/Flow.c b/datapath-windows/ovsext/Flow.c
index d3de8cc..c5954d8 100644
--- a/datapath-windows/ovsext/Flow.c
+++ b/datapath-windows/ovsext/Flow.c
@@ -95,7 +95,7 @@ static NTSTATUS OvsDoDumpFlows(OvsFlowDumpInput *dumpInput,
/* For Parsing attributes in FLOW_* commands */
const NL_POLICY nlFlowPolicy[] = {
- [OVS_FLOW_ATTR_KEY] = {.type = NL_A_NESTED, .optional = FALSE},
+ [OVS_FLOW_ATTR_KEY] = {.type = NL_A_NESTED, .optional = TRUE},
[ANKUR]: Will it not affect the flush case, where we will be getting a DEL
operation without FLOW_KEY?
[OVS_FLOW_ATTR_MASK] = {.type = NL_A_NESTED, .optional = TRUE},
[OVS_FLOW_ATTR_ACTIONS] = {.type = NL_A_NESTED, .optional = TRUE},
[OVS_FLOW_ATTR_STATS] = {.type = NL_A_UNSPEC,
@@ -103,7 +103,9 @@ const NL_POLICY nlFlowPolicy[] = {
.maxLen = sizeof(struct ovs_flow_stats),
.optional = TRUE},
[OVS_FLOW_ATTR_TCP_FLAGS] = {NL_A_U8, .optional = TRUE},
- [OVS_FLOW_ATTR_USED] = {NL_A_U64, .optional = TRUE}
+ [OVS_FLOW_ATTR_USED] = {NL_A_U64, .optional = TRUE},
+ [OVS_FLOW_ATTR_UFID] = { .type = NL_A_UNSPEC, .optional = TRUE,
+ .minLen = sizeof(OvsU128) }
};
[ANKUR]: As nithin correctly, mentioned what we are trying to do with
OVS_FLOW_ATTR_UFID is not clear? Do we not want to support it or do we want it
to be a NO OP? If we want it to be a NO OP then is it going to create some
functionality issue (since we'll end up returning success to user space while
kernel did not do anything)
/* For Parsing nested OVS_FLOW_ATTR_KEY attributes.
@@ -308,6 +310,11 @@ OvsFlowNlCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
goto done;
}
+ if (nlAttrs[OVS_FLOW_ATTR_KEY] == NULL) {
+ nlError = NL_ERROR_INVAL;
+ goto done;
+ }
+
[ANKUR]: Thanks for adding the check. As nithin mentioned, please confirm that
this error code is handled in userspace.
if ((rc = _MapNlToFlowPut(msgIn, nlAttrs[OVS_FLOW_ATTR_KEY],
nlAttrs[OVS_FLOW_ATTR_ACTIONS], nlAttrs[OVS_FLOW_ATTR_CLEAR],
&mappedFlow))
@@ -420,7 +427,7 @@ _FlowNlGetCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
PNL_MSG_HDR nlMsgOutHdr = NULL;
UINT32 attrOffset = NLMSG_HDRLEN + GENL_HDRLEN + OVS_HDRLEN;
PNL_ATTR nlAttrs[__OVS_FLOW_ATTR_MAX];
-
+ NL_ERROR nlError = NL_ERROR_SUCCESS;
OvsFlowGetInput getInput;
OvsFlowGetOutput getOutput;
NL_BUFFER nlBuf;
@@ -455,6 +462,11 @@ _FlowNlGetCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
goto done;
}
+ if (nlAttrs[OVS_FLOW_ATTR_KEY] == NULL) {
+ nlError = NL_ERROR_INVAL;
+ goto done;
+ }
+
[ANKUR]: Thanks for adding this. Same comment as previous check.
keyAttrOffset = (UINT32)((PCHAR) nlAttrs[OVS_FLOW_ATTR_KEY] -
(PCHAR)nlMsgHdr);
@@ -532,6 +544,13 @@ _FlowNlGetCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
*replyLen += NlMsgSize(nlMsgOutHdr);
done:
+ if (nlError != NL_ERROR_SUCCESS) {
+ POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR)
+ usrParamsCtx->outputBuffer;
+ NlBuildErrorMsg(msgIn, msgError, nlError);
+ *replyLen = msgError->nlMsg.nlmsgLen;
+ }
+
return rc;
[ANKUR]: Caller of this function is handling the error case. Is there a reason
that you added it here. My bad if i am missing something.
}
--
1.9.5.msysgit.0
_______________________________________________
dev mailing list
dev@openvswitch.org
https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailman_listinfo_dev&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=RDZsrBxhAiOewDSD-jiF-W03FqHevF2o7haW6eQzmtA&m=p6keu4ALm2BI9PBvGYFt7d_QZOjcdyd7DGsma9v2CjE&s=ii5o_QQckVciSbMVP6CrD8smqvKA7zJZJgrOehuCR6Q&e=
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
