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

Saurabh Shah ssaurabh at vmware.com
Fri Aug 29 19:14:11 UTC 2014


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-bounces at openvswitch.org] On Behalf Of Eitan Eliahu
> 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/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 at 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



More information about the dev mailing list