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:[email protected]] On Behalf Of Samuel Ghinet Sent: Friday, August 29, 2014 3:58 AM To: [email protected]; 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 <[email protected]> Regards, Sam ________________________________________ Date: Fri, 29 Aug 2014 01:15:21 -0700 From: Nithin Raju <[email protected]> To: [email protected] Subject: [ovs-dev] [PATCH 9/9 v2] datapath-windows: refactor code to setup dump start state Message-ID: <[email protected]> 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 <[email protected]> Acked-by: Ankur Sharma <[email protected]> Acked-by: Samuel Ghinet <[email protected]> --- 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 [email protected] 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 [email protected] http://openvswitch.org/mailman/listinfo/dev
