Thread-Topic: [ovs-dev] [PATCH] datapath-windows: Append flow attribute key
Thread-Index: AQHQ7xepL84xz91rMEqut1rnlan04A==
Date: Mon, 14 Sep 2015 18:03:29 +0000
Message-ID: <d21c58f9.d12%vsai...@vmware.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
x-ms-exchange-transport-fromentityheader: Hosted
x-originating-ip: [10.113.160.246]
Content-Type: text/plain; charset="iso-8859-1"
Content-ID: <96312149fa4a1241a02be13f0e693...@pa-exch1.vmware.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
I have added my comment inline.
- Sairam Venugopal
On 9/10/15, 4:15 PM, "Alin Serdean" <aserd...@cloudbasesolutions.com>
wrote:
>Currently when running the vswitch daemon we get a lot of messages of the
>form:
>2015-09-10T23:04:21Z|07255|dpif(revalidator11)|WARN|system@ovs-system:
>failed
>to flow_del (Invalid argument).
>
>The userspace expects after sending a delete flow command, to receive the
>flow
>key of the deleted flow.
>
>Currently we only send back the statiscs. This patch appends back the
>flow key
>attribute for to the response buffer for the flow commands new, modify and
>delete.
>
>This patch also responds to the userspace with ENOENT in the case the flow
>was not modified, deleted, created or retrieved.
>
>Also incorporate some refactors.
>
>Signed-off-by: Alin Gabriel Serdean <aserd...@cloudbasesolutions.com>
>---
>This patch is also intended for branch 2.4
>---
> datapath-windows/ovsext/Flow.c | 65
>+++++++++++++++++++++----------
> datapath-windows/ovsext/Netlink/Netlink.c | 4 +-
> datapath-windows/ovsext/Netlink/Netlink.h | 4 +-
> 3 files changed, 49 insertions(+), 24 deletions(-)
>
>diff --git a/datapath-windows/ovsext/Flow.c
>b/datapath-windows/ovsext/Flow.c
>index 58bdd42..e71f332 100644
>--- a/datapath-windows/ovsext/Flow.c
>+++ b/datapath-windows/ovsext/Flow.c
>@@ -312,7 +312,6 @@ OvsFlowNlCmdHandler(POVS_USER_PARAMS_CONTEXT
>usrParamsCtx,
>=20
> if (flowAttrs[OVS_FLOW_ATTR_PROBE]) {
> OVS_LOG_ERROR("Attribute OVS_FLOW_ATTR_PROBE not supported");
>- nlError =3D NL_ERROR_NOENT;
> goto done;
> }
>=20
>@@ -328,6 +327,11 @@ OvsFlowNlCmdHandler(POVS_USER_PARAMS_CONTEXT
>usrParamsCtx,
> &stats);
> if (rc !=3D STATUS_SUCCESS) {
> OVS_LOG_ERROR("OvsPutFlowIoctl failed.");
>+ /*
>+ * Report back to the userspace the flow could not be modified,
>+ * created or deleted
>+ */
>+ nlError =3D NL_ERROR_NOENT;
> goto done;
> }
>=20
>@@ -350,6 +354,15 @@ OvsFlowNlCmdHandler(POVS_USER_PARAMS_CONTEXT
>usrParamsCtx,
> rc =3D STATUS_SUCCESS;
> }
>=20
>+ /* Append OVS_FLOW_ATTR_KEY attribute. This is need i.e. for flow
>delete*/
>+ if (!NlMsgPutNested(&nlBuf, OVS_FLOW_ATTR_KEY,
>+ NlAttrData(flowAttrs[OVS_FLOW_ATTR_KEY]),
>+ NlAttrGetSize(flowAttrs[OVS_FLOW_ATTR_KEY]))) {
>+ OVS_LOG_ERROR("Adding OVS_FLOW_ATTR_KEY attribute failed.");
>+ rc =3D STATUS_INVALID_BUFFER_SIZE;
>+ goto done;
>+ }
>+
> /* Append OVS_FLOW_ATTR_STATS attribute */
> if (!NlMsgPutTailUnspec(&nlBuf, OVS_FLOW_ATTR_STATS,
> (PCHAR)(&replyStats), sizeof(replyStats))) {
>@@ -365,7 +378,7 @@ done:
>=20
> if (nlError !=3D NL_ERROR_SUCCESS) {
> POVS_MESSAGE_ERROR msgError =3D (POVS_MESSAGE_ERROR)
>- usrParamsCtx->outputBuffer;
>+ usrParamsCtx->outputBuffer;
> NlBuildErrorMsg(msgIn, msgError, nlError);
> *replyLen =3D msgError->nlMsg.nlmsgLen;
> rc =3D STATUS_SUCCESS;
>@@ -385,29 +398,14 @@ OvsFlowNlGetCmdHandler(POVS_USER_PARAMS_CONTEXT
>usrParamsCtx,
> UINT32 *replyLen)
> {
> NTSTATUS status =3D STATUS_SUCCESS;
>- NL_ERROR nlError =3D NL_ERROR_SUCCESS;
>- POVS_MESSAGE msgIn =3D (POVS_MESSAGE)usrParamsCtx->inputBuffer;
>=20
> if (usrParamsCtx->devOp =3D=3D OVS_TRANSACTION_DEV_OP) {
> status =3D _FlowNlGetCmdHandler(usrParamsCtx, replyLen);
>-
>- /* No trasanctional errors as of now.
>- * If we have something then we need to convert rc to
>- * nlError. */
>- if ((nlError !=3D NL_ERROR_SUCCESS) &&
>- (usrParamsCtx->outputBuffer)) {
>- POVS_MESSAGE_ERROR msgError =3D (POVS_MESSAGE_ERROR)
>- usrParamsCtx->outputBuffer;
>- NlBuildErrorMsg(msgIn, msgError, nlError);
>- *replyLen =3D msgError->nlMsg.nlmsgLen;
>- status =3D STATUS_SUCCESS;
>- goto done;
>- }
>- } else {
>+ }
>+ else {
> status =3D _FlowNlDumpCmdHandler(usrParamsCtx, replyLen);
> }
>=20
>-done:
> return status;
> }
>=20
>@@ -442,6 +440,7 @@ _FlowNlGetCmdHandler(POVS_USER_PARAMS_CONTEXT
>usrParamsCtx,
> UINT32 keyAttrOffset =3D 0;
> UINT32 tunnelKeyAttrOffset =3D 0;
> BOOLEAN ok;
>+ NL_ERROR nlError =3D NL_ERROR_SUCCESS;
>=20
> if (usrParamsCtx->inputLength > usrParamsCtx->outputLength) {
> /* Should not be the case.
>@@ -510,6 +509,10 @@ _FlowNlGetCmdHandler(POVS_USER_PARAMS_CONTEXT
>usrParamsCtx,
> rc =3D OvsGetFlowIoctl(&getInput, &getOutput);
> if (rc !=3D STATUS_SUCCESS) {
> OVS_LOG_ERROR("OvsGetFlowIoctl failed.");
>+ /*
>+ * Report back to the userspace the flow could not be found
>+ */
>+ nlError =3D NL_ERROR_NOENT;
> goto done;
> }
>=20
>@@ -543,6 +546,14 @@ _FlowNlGetCmdHandler(POVS_USER_PARAMS_CONTEXT
>usrParamsCtx,
> *replyLen +=3D NlMsgSize(nlMsgOutHdr);
>=20
> done:
>+ if (nlError !=3D NL_ERROR_SUCCESS) {
>+ POVS_MESSAGE_ERROR msgError =3D (POVS_MESSAGE_ERROR)
>+ usrParamsCtx->outputBuffer;
>+ NlBuildErrorMsg(msgIn, msgError, nlError);
>+ *replyLen =3D msgError->nlMsg.nlmsgLen;
>+ rc =3D STATUS_SUCCESS;
>+ }
>+
> return rc;
> }
>=20
>@@ -558,9 +569,10 @@ _FlowNlDumpCmdHandler(POVS_USER_PARAMS_CONTEXT
>usrParamsCtx,
> {
> NTSTATUS rc =3D STATUS_SUCCESS;
> UINT32 temp =3D 0; /* To keep compiler happy for calling
>OvsDoDumpFlows */
>-
>+ NL_ERROR nlError =3D NL_ERROR_SUCCESS;
> POVS_OPEN_INSTANCE instance =3D (POVS_OPEN_INSTANCE)
> (usrParamsCtx->ovsInstance);
>+ POVS_MESSAGE msgIn =3D instance->dumpState.ovsMsg;
>=20
> if (usrParamsCtx->devOp =3D=3D OVS_WRITE_DEV_OP) {
> /* Dump Start */
>@@ -568,7 +580,6 @@ _FlowNlDumpCmdHandler(POVS_USER_PARAMS_CONTEXT
>usrParamsCtx,
> goto done;
> }
>=20
>- POVS_MESSAGE msgIn =3D instance->dumpState.ovsMsg;
> PNL_MSG_HDR nlMsgHdr =3D &(msgIn->nlMsg);
> PGENL_MSG_HDR genlMsgHdr =3D &(msgIn->genlMsg);
> POVS_HDR ovsHdr =3D &(msgIn->ovsHdr);
>@@ -600,6 +611,10 @@ _FlowNlDumpCmdHandler(POVS_USER_PARAMS_CONTEXT
>usrParamsCtx,
> rc =3D OvsDoDumpFlows(&dumpInput, &dumpOutput, &temp);
> if (rc !=3D STATUS_SUCCESS) {
> OVS_LOG_ERROR("OvsDoDumpFlows failed with rc: %d", rc);
>+ /*
>+ * Report back to the userspace the flows could not be found
>+ */
>+ nlError =3D NL_ERROR_NOENT;
> break;
> }
>=20
>@@ -669,6 +684,14 @@ _FlowNlDumpCmdHandler(POVS_USER_PARAMS_CONTEXT
>usrParamsCtx,
> } while(TRUE);
>=20
> done:
>+ if (nlError !=3D NL_ERROR_SUCCESS) {
>+ POVS_MESSAGE_ERROR msgError =3D (POVS_MESSAGE_ERROR)
>+ usrParamsCtx->outputBuffer;
>+ NlBuildErrorMsg(msgIn, msgError, nlError);
>+ *replyLen =3D msgError->nlMsg.nlmsgLen;
Sai: Should rc be set to STATUS_SUCCESS? Isn=B9t this a failure condition
that will affect the calling function?
>+ rc =3D STATUS_SUCCESS;
>+ }
>+
> return rc;
> }
>=20
>diff --git a/datapath-windows/ovsext/Netlink/Netlink.c
>b/datapath-windows/ovsext/Netlink/Netlink.c
>index a66fb38..e2312da 100644
>--- a/datapath-windows/ovsext/Netlink/Netlink.c
>+++ b/datapath-windows/ovsext/Netlink/Netlink.c
>@@ -560,7 +560,7 @@ NlMsgEndNested(PNL_BUFFER buf, UINT32 offset)
> * Refer nl_msg_put_nested for more details.
> *=20
>--------------------------------------------------------------------------
> */
>-VOID
>+BOOLEAN
> NlMsgPutNested(PNL_BUFFER buf, UINT16 type,
> const PVOID data, UINT32 size)
> {
>@@ -574,6 +574,8 @@ NlMsgPutNested(PNL_BUFFER buf, UINT16 type,
> ASSERT(ret);
>=20
> NlMsgEndNested(buf, offset);
>+
>+ return ret;
> }
>=20
> /* Accessing netlink message payload */
>diff --git a/datapath-windows/ovsext/Netlink/Netlink.h
>b/datapath-windows/ovsext/Netlink/Netlink.h
>index a520ccf..d270737 100644
>--- a/datapath-windows/ovsext/Netlink/Netlink.h
>+++ b/datapath-windows/ovsext/Netlink/Netlink.h
>@@ -203,8 +203,8 @@ BOOLEAN NlMsgPutHeadU64(PNL_BUFFER buf, UINT16 type,
>UINT64 value);
> BOOLEAN NlMsgPutHeadString(PNL_BUFFER buf, UINT16 type, PCHAR value);
> UINT32 NlMsgStartNested(PNL_BUFFER buf, UINT16 type);
> VOID NlMsgEndNested(PNL_BUFFER buf, UINT32 offset);
>-VOID NlMsgPutNested(PNL_BUFFER buf, UINT16 type,
>- const PVOID data, UINT32 size);
>+BOOLEAN NlMsgPutNested(PNL_BUFFER buf, UINT16 type,
>+ const PVOID data, UINT32 size);
>=20
> /* These variants are convenient for iterating nested attributes. */
> #define NL_NESTED_FOR_EACH(ITER, LEFT, A) \
>--=20
>1.9.5.msysgit.0
>_______________________________________________
>dev mailing list
>dev@openvswitch.org
>https://urldefense.proofpoint.com/v2/url?u=3Dhttp-3A__openvswitch.org_mail=
ma
>n_listinfo_dev&d=3DBQIGaQ&c=3DSqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&=
r=3DDc
>ruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ&m=3D6RH6tvn1Q1aHdnXHx1osausgXjLk=
1M
>PcdL8htuL8dsE&s=3D0wmTX0Pml41QX4qI5d3EkkpMgNXbJMgVfe5cdty6ccQ&e=3D=20
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
