datapath-windows: Append flow attribute key

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 <aserdean@cloudbasesolutions.com>
Acked-by: Sorin Vinturis <svinturis@cloudbasesolutions.com>
Acked-by: Sairam Venugopal <vsairam@vmware.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>

3 files changed, 46 insertions, 22 deletions

diff --git a/datapath-windows/ovsext/Flow.c b/datapath-windows/ovsext/Flow.c
index 58bdd42cc..32cb0865d 100644
--- a/datapath-windows/ovsext/Flow.c
+++ b/datapath-windows/ovsext/Flow.c
@@ -312,7 +312,6 @@ OvsFlowNlCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
 
     if (flowAttrs[OVS_FLOW_ATTR_PROBE]) {
         OVS_LOG_ERROR("Attribute OVS_FLOW_ATTR_PROBE not supported");
-        nlError = NL_ERROR_NOENT;
         goto done;
     }
 
@@ -328,6 +327,11 @@ OvsFlowNlCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
                          &stats);
     if (rc != STATUS_SUCCESS) {
         OVS_LOG_ERROR("OvsPutFlowIoctl failed.");
+        /*
+         * Report back to the userspace the flow could not be modified,
+         * created or deleted
+         */
+        nlError = NL_ERROR_NOENT;
         goto done;
     }
 
@@ -350,6 +354,15 @@ OvsFlowNlCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
         rc = STATUS_SUCCESS;
     }
 
+    /* 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 = STATUS_INVALID_BUFFER_SIZE;
+        goto done;
+    }
+
     /* Append OVS_FLOW_ATTR_STATS attribute */
     if (!NlMsgPutTailUnspec(&nlBuf, OVS_FLOW_ATTR_STATS,
         (PCHAR)(&replyStats), sizeof(replyStats))) {
@@ -385,29 +398,13 @@ OvsFlowNlGetCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
                        UINT32 *replyLen)
 {
     NTSTATUS status = STATUS_SUCCESS;
-    NL_ERROR nlError = NL_ERROR_SUCCESS;
-    POVS_MESSAGE msgIn = (POVS_MESSAGE)usrParamsCtx->inputBuffer;
 
     if (usrParamsCtx->devOp == OVS_TRANSACTION_DEV_OP) {
         status = _FlowNlGetCmdHandler(usrParamsCtx, replyLen);
-
-        /* No trasanctional errors as of now.
-         * If we have something then we need to convert rc to
-         * nlError. */
-        if ((nlError != NL_ERROR_SUCCESS) &&
-            (usrParamsCtx->outputBuffer)) {
-            POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR)
-                                           usrParamsCtx->outputBuffer;
-            NlBuildErrorMsg(msgIn, msgError, nlError);
-            *replyLen = msgError->nlMsg.nlmsgLen;
-            status = STATUS_SUCCESS;
-            goto done;
-        }
     } else {
         status = _FlowNlDumpCmdHandler(usrParamsCtx, replyLen);
     }
 
-done:
     return status;
 }
 
@@ -442,6 +439,7 @@ _FlowNlGetCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
     UINT32 keyAttrOffset = 0;
     UINT32 tunnelKeyAttrOffset = 0;
     BOOLEAN ok;
+    NL_ERROR nlError = NL_ERROR_SUCCESS;
 
     if (usrParamsCtx->inputLength > usrParamsCtx->outputLength) {
         /* Should not be the case.
@@ -510,6 +508,10 @@ _FlowNlGetCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
     rc = OvsGetFlowIoctl(&getInput, &getOutput);
     if (rc != STATUS_SUCCESS) {
         OVS_LOG_ERROR("OvsGetFlowIoctl failed.");
+        /*
+         * Report back to the userspace the flow could not be found
+         */
+        nlError = NL_ERROR_NOENT;
         goto done;
     }
 
@@ -543,6 +545,14 @@ _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;
+        rc = STATUS_SUCCESS;
+    }
+
     return rc;
 }
 
@@ -558,9 +568,10 @@ _FlowNlDumpCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
 {
     NTSTATUS rc = STATUS_SUCCESS;
     UINT32  temp = 0;   /* To keep compiler happy for calling OvsDoDumpFlows */
-
+    NL_ERROR nlError = NL_ERROR_SUCCESS;
     POVS_OPEN_INSTANCE instance = (POVS_OPEN_INSTANCE)
                                   (usrParamsCtx->ovsInstance);
+    POVS_MESSAGE msgIn = instance->dumpState.ovsMsg;
 
     if (usrParamsCtx->devOp == OVS_WRITE_DEV_OP) {
         /* Dump Start */
@@ -568,7 +579,6 @@ _FlowNlDumpCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
         goto done;
     }
 
-    POVS_MESSAGE msgIn = instance->dumpState.ovsMsg;
     PNL_MSG_HDR nlMsgHdr = &(msgIn->nlMsg);
     PGENL_MSG_HDR genlMsgHdr = &(msgIn->genlMsg);
     POVS_HDR ovsHdr = &(msgIn->ovsHdr);
@@ -600,6 +610,10 @@ _FlowNlDumpCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
         rc = OvsDoDumpFlows(&dumpInput, &dumpOutput, &temp);
         if (rc != STATUS_SUCCESS) {
             OVS_LOG_ERROR("OvsDoDumpFlows failed with rc: %d", rc);
+            /*
+             * Report back to the userspace the flows could not be found
+             */
+            nlError = NL_ERROR_NOENT;
             break;
         }
 
@@ -669,6 +683,14 @@ _FlowNlDumpCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
     } while(TRUE);
 
 done:
+    if (nlError != NL_ERROR_SUCCESS) {
+        POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR)
+                                      usrParamsCtx->outputBuffer;
+        NlBuildErrorMsg(msgIn, msgError, nlError);
+        *replyLen = msgError->nlMsg.nlmsgLen;
+        rc = STATUS_SUCCESS;
+    }
+
     return rc;
 }
 
diff --git a/datapath-windows/ovsext/Netlink/Netlink.c b/datapath-windows/ovsext/Netlink/Netlink.c
index a66fb3833..e2312dab3 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.
  * --------------------------------------------------------------------------
  */
-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);
 
     NlMsgEndNested(buf, offset);
+
+    return ret;
 }
 
 /* Accessing netlink message payload */
diff --git a/datapath-windows/ovsext/Netlink/Netlink.h b/datapath-windows/ovsext/Netlink/Netlink.h
index a520ccf57..d27073731 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);
 
 /* These variants are convenient for iterating nested attributes. */
 #define NL_NESTED_FOR_EACH(ITER, LEFT, A)                               \