I would really like to discourage this practice of - "Lets commit this code and we will take your comments in separate reviews". I just replied with a similar comment to one of Samuel's review where he took care of comments in a separate review. This does not make for a high quality code that we are all aspire for.
Also, please wait for the reviewers to add acknowledgement. I wrote about this earlier on the mailing list - http://openvswitch.org/pipermail/dev/2014-August/044488.html. Saurabh > -----Original Message----- > From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Eitan Eliahu > 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/mailm > an/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=yTvML8Ox > A42Jb6ViHe7fUXbvPVOYDPVq87w43doxtlY%3D%0A&m=S3km2ZHKUwhDT8 > Gkzz4TE72ctA5%2F31S%2FN2BeQj7VCAM%3D%0A&s=302b179c09ef127a5fd > 1367bfb0a59d5010d5bdaf5c72055eb870bbcfd65c4c8 > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailm > an/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=pEkjsHfytv > HEWufeZPpgqSOJMdMjuZPbesVsNhCUc0E%3D%0A&m=yd%2BN2WTqENdX > 8qjtplqwKn2cXzWdHvOVN5NFbaGdImo%3D%0A&s=5eadff38ec5bf17f60df42 > 563376572629256785dad009bbf102d05ef01e842b _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
