[ovs-dev] [PATCH] datapath-windows: Append flow attribute key

Sairam Venugopal vsairam at vmware.com
Mon Sep 14 18:03:34 UTC 2015


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%vsairam at 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: <96312149FA4A1241A02BE13F0E6936BA at 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" <aserdean at 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 at 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 <aserdean at 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 at 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




More information about the dev mailing list