[ovs-dev] [PATCH 9/9 v2] datapath-windows: refactor code to setup dump start state

Ankur Sharma ankursharma at vmware.com
Fri Aug 29 19:11:41 UTC 2014


Hi Sam,

I have talked to ben and he is fine with the approach of handling the review comment in another patch in the same series. But yes ideally we should try to keep the review comment fix in the same patch.

As i can see nithin has handled most of your comments. If you think something critical has been missed then please feel free to let us know and we will take care of it after this checkin.

I have submitted a v3 of the patch which has following changes:
a. Rebasing patch 1/1
b. Trailing whitespace in 4/4
c. Removing your name from Acked-by.

I am fine with the current set of changes and hence they stay as Acked-by: Ankur Sharma <ankursharma at vmware.com>

Thanks.

Regards,
Ankur
________________________________________
From: dev <dev-bounces at openvswitch.org> on behalf of Eitan Eliahu <eliahue at vmware.com>
Sent: Friday, August 29, 2014 8:42 AM
To: Samuel Ghinet; dev at openvswitch.org; Nithin Raju
Subject: Re: [ovs-dev] [PATCH 9/9 v2] datapath-windows: refactor code to setup dump start state

Hi Sam,
I was wondering if we could go ahead and acknowledge Nithin's change. I know you have some reservations but this work should be considered as a bring up which effort to enable other developers to continue with their own tasks. The current situation is that we have a circular dependency and given the fact that Nithin is on a long PTO makes things worse. Unless you see fundamental issues please consider approving the code. Certainly, this code is not written on a stone and can be improved as early as possible.
Thank you,
Eitan

-----Original Message-----
From: dev [mailto:dev-bounces at openvswitch.org] On Behalf Of Samuel Ghinet
Sent: Friday, August 29, 2014 3:58 AM
To: dev at openvswitch.org; Nithin Raju
Subject: Re: [ovs-dev] [PATCH 9/9 v2] datapath-windows: refactor code to setup dump start state

Nithin,

I had expected you modify the original patch with my suggestions, not add a new patch on top of it, by which to refactor the original patch.
So this patch is: Not acked-by: Samuel Ghinet <sghinet at cloudbasesolutions.com>

Regards,
Sam
________________________________________
Date: Fri, 29 Aug 2014 01:15:21 -0700
From: Nithin Raju <nithin at vmware.com>
To: dev at openvswitch.org
Subject: [ovs-dev] [PATCH 9/9 v2] datapath-windows: refactor code to
        setup   dump start state
Message-ID: <1409300121-13568-9-git-send-email-nithin at vmware.com>

Per review comment, in this patch we refactor the code to create a
OvsSetupDumpStart() which can be leveraged by dump functions in the future. I have not refactored the code that continues the dump operation primarily since it is not final yet. Once the netlink set APIs are in place, we can refactor that too.

Signed-off-by: Nithin Raju <nithin at vmware.com>
Acked-by: Ankur Sharma <ankursharma at vmware.com>
Acked-by: Samuel Ghinet <sghinet at cloudbasesolutions.com>
---
 datapath-windows/ovsext/Datapath.c |   66 ++++++++++++++++++++++--------------
 1 files changed, 40 insertions(+), 26 deletions(-)

diff --git a/datapath-windows/ovsext/Datapath.c b/datapath-windows/ovsext/Datapath.c
index 943e6f9..4a17914 100644
--- a/datapath-windows/ovsext/Datapath.c
+++ b/datapath-windows/ovsext/Datapath.c
@@ -110,8 +110,11 @@ static NTSTATUS OvsGetDpCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,

 /* Netlink control family: this is a Windows specific family. */  NETLINK_CMD nlControlFamilyCmdOps[] = {
-    { OVS_CTRL_CMD_WIN_GET_PID, OvsGetPidCmdHandler,
-      OVS_TRANSACTION_DEV_OP, FALSE }
+    { .cmd             = OVS_CTRL_CMD_WIN_GET_PID,
+      .handler         = OvsGetPidCmdHandler,
+      .supportedDevOp  = OVS_TRANSACTION_DEV_OP,
+      .validateDpIndex = FALSE
+    }
 };

 NETLINK_FAMILY nlControlFamilyOps = {
@@ -125,8 +128,11 @@ NETLINK_FAMILY nlControlFamilyOps = {

 /* Netlink datapath family. */
 NETLINK_CMD nlDatapathFamilyCmdOps[] = {
-    { OVS_DP_CMD_GET, OvsGetDpCmdHandler,
-      OVS_WRITE_DEV_OP | OVS_READ_DEV_OP, FALSE }
+    { .cmd             = OVS_DP_CMD_GET,
+      .handler         = OvsGetDpCmdHandler,
+      .supportedDevOp  = OVS_WRITE_DEV_OP | OVS_READ_DEV_OP,
+      .validateDpIndex = FALSE
+    }
 };

 NETLINK_FAMILY nlDatapathFamilyOps = {
@@ -182,6 +188,7 @@ static NTSTATUS ValidateNetlinkCmd(UINT32 devOp,  static NTSTATUS InvokeNetlinkCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
                                         NETLINK_FAMILY *nlFamilyOps,
                                         UINT32 *replyLen);
+static NTSTATUS OvsSetupDumpStart(POVS_USER_PARAMS_CONTEXT
+usrParamsCtx);


 /* Handles to the device object for communication with userspace. */ @@ -828,28 +835,8 @@ OvsGetDpCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
         (POVS_OPEN_INSTANCE)usrParamsCtx->ovsInstance;

     if (usrParamsCtx->devOp == OVS_WRITE_DEV_OP) {
-        NTSTATUS status;
-
-        /* input buffer has been validated while validating write dev op. */
-        ASSERT(msgIn != NULL && usrParamsCtx->inputLength >= sizeof *msgIn);
-
-        /* A write operation that does not indicate dump start is invalid. */
-        if ((msgIn->nlMsg.nlmsgFlags & NLM_F_DUMP) != NLM_F_DUMP) {
-            return STATUS_INVALID_PARAMETER;
-        }
-        /* XXX: Handle other NLM_F_* flags in the future. */
-
-        /*
-         * This operation should be setting up the dump state. If there's any
-         * previous state, clear it up so as to set it up afresh.
-         */
-        if (instance->dumpState.ovsMsg != NULL) {
-            FreeUserDumpState(instance);
-        }
-        status = InitUserDumpState(instance, msgIn);
-        if (status != STATUS_SUCCESS) {
-            return STATUS_NO_MEMORY;
-        }
+        *replyLen = 0;
+        OvsSetupDumpStart(usrParamsCtx);
     } else {
         ASSERT(usrParamsCtx->devOp == OVS_READ_DEV_OP);

@@ -943,6 +930,33 @@ OvsGetDpCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
     return STATUS_SUCCESS;
 }

+static NTSTATUS
+OvsSetupDumpStart(POVS_USER_PARAMS_CONTEXT usrParamsCtx) {
+    POVS_MESSAGE msgIn = (POVS_MESSAGE)usrParamsCtx->inputBuffer;
+    POVS_OPEN_INSTANCE instance =
+        (POVS_OPEN_INSTANCE)usrParamsCtx->ovsInstance;
+
+    /* input buffer has been validated while validating write dev op. */
+    ASSERT(msgIn != NULL && usrParamsCtx->inputLength >= sizeof
+ *msgIn);
+
+    /* A write operation that does not indicate dump start is invalid. */
+    if ((msgIn->nlMsg.nlmsgFlags & NLM_F_DUMP) != NLM_F_DUMP) {
+        return STATUS_INVALID_PARAMETER;
+    }
+    /* XXX: Handle other NLM_F_* flags in the future. */
+
+    /*
+     * This operation should be setting up the dump state. If there's any
+     * previous state, clear it up so as to set it up afresh.
+     */
+    if (instance->dumpState.ovsMsg != NULL) {
+        FreeUserDumpState(instance);
+    }
+
+    return InitUserDumpState(instance, msgIn); }
+
 /*
  * --------------------------------------------------------------------------
  *  Utility function to map the output buffer in an IRP. The buffer is assumed
--
1.7.4.1
_______________________________________________
dev mailing list
dev at openvswitch.org
https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=yTvML8OxA42Jb6ViHe7fUXbvPVOYDPVq87w43doxtlY%3D%0A&m=S3km2ZHKUwhDT8Gkzz4TE72ctA5%2F31S%2FN2BeQj7VCAM%3D%0A&s=302b179c09ef127a5fd1367bfb0a59d5010d5bdaf5c72055eb870bbcfd65c4c8
_______________________________________________
dev mailing list
dev at openvswitch.org
https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=f6EhnZ0ORGZNt5QbYmRaOxfWfx%2Bqd3KEiPf3%2FYaollU%3D%0A&m=zR10nrovKi4g0h%2BDslK0Prnc1ZQcqLz5w2JEHYA6iTQ%3D%0A&s=a72fe9cf45c7a073d5cdc72bd635b525e74a0d7992e4ccc3108e876f7d82356d



More information about the dev mailing list