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 <ankursha...@vmware.com> Thanks. Regards, Ankur ________________________________________ From: dev <dev-boun...@openvswitch.org> on behalf of Eitan Eliahu <elia...@vmware.com> Sent: Friday, August 29, 2014 8:42 AM To: Samuel Ghinet; dev@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-boun...@openvswitch.org] On Behalf Of Samuel Ghinet Sent: Friday, August 29, 2014 3:58 AM To: dev@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 <sghi...@cloudbasesolutions.com> Regards, Sam ________________________________________ Date: Fri, 29 Aug 2014 01:15:21 -0700 From: Nithin Raju <nit...@vmware.com> To: dev@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-nit...@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 <nit...@vmware.com> Acked-by: Ankur Sharma <ankursha...@vmware.com> Acked-by: Samuel Ghinet <sghi...@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@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@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 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
