[ovs-dev] [PATCH v3 4/4] datapath-windows: OVS_PACKET_CMD_EXECUTE handler.

[Nithin Raju]

hi Ankur,
Change looks good but for the comments. I'll wait for the next version to ACK the change.

On Oct 15, 2014, at 10:32 AM, Ankur Sharma <ankursharma at vmware.com>
 wrote:

> In this patch we have implemented the handler for OVS_PACKET_CMD_EXECUTE command.
> 
> Signed-off-by: Ankur Sharma <ankursharma at vmware.com>
> Acked-by: Eitan Eliahu <eliahue at vmware.com>
> ---
> datapath-windows/ovsext/User.c | 111 +++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 108 insertions(+), 3 deletions(-)
> 
> diff --git a/datapath-windows/ovsext/User.c b/datapath-windows/ovsext/User.c
> index 7c68495..4f7c997 100644
> --- a/datapath-windows/ovsext/User.c
> +++ b/datapath-windows/ovsext/User.c
> @@ -46,6 +46,9 @@ extern PNDIS_SPIN_LOCK gOvsCtrlLock;
> extern POVS_SWITCH_CONTEXT gOvsSwitchContext;
> OVS_USER_STATS ovsUserStats;
> 
> +static VOID _MapNlAttrToOvsPktExec(PNL_ATTR *nlAttrs, PNL_ATTR *keyAttrs,
> +                                   OvsPacketExecute  *execute);
> +extern NL_POLICY nlFlowKeyPolicy[];
> 
> NTSTATUS
> OvsUserInit()
> @@ -310,12 +313,114 @@ NTSTATUS
> OvsNlExecuteCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
>                        UINT32 *replyLen)
> {
> -    NTSTATUS rc = STATUS_SUCCESS;
> +    NTSTATUS status = STATUS_SUCCESS;
> +    POVS_MESSAGE msgIn = (POVS_MESSAGE)usrParamsCtx->inputBuffer;
> +    POVS_MESSAGE msgOut = (POVS_MESSAGE)usrParamsCtx->outputBuffer;
> +    PNL_MSG_HDR nlMsgHdr = &(msgIn->nlMsg);
> +    PGENL_MSG_HDR genlMsgHdr = &(msgIn->genlMsg);
> +    POVS_HDR ovsHdr = &(msgIn->ovsHdr);

Just to keep in mind for next time, there's already an 'ovsMsg' in userParamsCtx. So, we can always use 'usrParamsCtx->ovsMsg.nlMsg' etc.

> +
> +    PNL_ATTR nlAttrs[__OVS_PACKET_ATTR_MAX];
> +    PNL_ATTR keyAttrs[__OVS_KEY_ATTR_MAX] = {NULL};
> +
> +    UINT32 attrOffset = NLMSG_HDRLEN + GENL_HDRLEN + OVS_HDRLEN;
> +    UINT32 keyAttrOffset = 0;
> +    OvsPacketExecute execute;
> +    NL_ERROR nlError = NL_ERROR_SUCCESS;
> +    NL_BUFFER nlBuf;
> 
> -    UNREFERENCED_PARAMETER(usrParamsCtx);
>     UNREFERENCED_PARAMETER(replyLen);

replyLen is used. YOu can get rid of the UNREFERENCED_PARAMETER()

> 
> -    return rc;
> +    static const NL_POLICY nlPktExecPolicy[] = {
> +        [OVS_PACKET_ATTR_PACKET] = {.type = NL_A_UNSPEC, .optional = FALSE},
> +        [OVS_PACKET_ATTR_KEY] = {.type = NL_A_UNSPEC, .optional = FALSE},

If key is mandatory, we can avoid recalculating the key in OvsExecuteDpIoctl() where we call OvsExtractFlow(). If you are not planning to fix this right now, can you pls. add an XXX comment, and we can take care of it later?


> +        [OVS_PACKET_ATTR_ACTIONS] = {.type = NL_A_UNSPEC, .optional = FALSE},
> +        [OVS_PACKET_ATTR_USERDATA] = {.type = NL_A_UNSPEC, .optional = TRUE},
> +        [OVS_PACKET_ATTR_EGRESS_TUN_KEY] = {.type = NL_A_UNSPEC,
> +                                            .optional = TRUE}
> +    };
> +
> +    RtlZeroMemory(&execute, sizeof(OvsPacketExecute));
> +
> +    /* Get all the top level Flow attributes */
> +    if ((NlAttrParse(nlMsgHdr, attrOffset, NlMsgAttrsLen(nlMsgHdr),
> +                     nlPktExecPolicy, nlAttrs, ARRAY_SIZE(nlAttrs)))
> +                     != TRUE) {
> +        OVS_LOG_ERROR("Attr Parsing failed for msg: %p",
> +                       nlMsgHdr);
> +        status = STATUS_UNSUCCESSFUL;
> +        goto done;
> +    }
> +
> +    keyAttrOffset = (UINT32)((PCHAR)nlAttrs[OVS_PACKET_ATTR_KEY] -
> +                    (PCHAR)nlMsgHdr);
> +
> +    /* Get flow keys attributes */
> +    if ((NlAttrParseNested(nlMsgHdr, keyAttrOffset,
> +                           NlAttrLen(nlAttrs[OVS_PACKET_ATTR_KEY]),
> +                           nlFlowKeyPolicy, keyAttrs,
> +                           ARRAY_SIZE(keyAttrs))) != TRUE) {
> +        OVS_LOG_ERROR("Key Attr Parsing failed for msg: %p", nlMsgHdr);
> +        status = STATUS_UNSUCCESSFUL;
> +        goto done;
> +    }
> +
> +    execute.dpNo = ovsHdr->dp_ifindex;
> +
> +    _MapNlAttrToOvsPktExec(nlAttrs, keyAttrs, &execute);
> +
> +    status = OvsExecuteDpIoctl(&execute);
> +
> +    if (status == STATUS_SUCCESS) {
> +        NlBufInit(&nlBuf, usrParamsCtx->outputBuffer,
> +                  usrParamsCtx->outputLength);
> +
> +        /* Prepare nl Msg headers */
> +        status = NlFillOvsMsg(&nlBuf, nlMsgHdr->nlmsgType, 0,
> +                 nlMsgHdr->nlmsgSeq, nlMsgHdr->nlmsgPid,
> +                 genlMsgHdr->cmd, OVS_PACKET_VERSION,
> +                 ovsHdr->dp_ifindex);
> +
> +        if (status == STATUS_SUCCESS) {
> +            *replyLen = msgOut->nlMsg.nlmsgLen;
> +        }
> +    }

Currently, OvsExecuteDpIoctl() -> ActionsExecute() does not return any error for actions that are not supported. I am not sure if this is the right things to do. Eg. for the check_masked_set_actions(), we'd end up returning success even though we did not execute that actions correctly.

So, we should change ActionsExecute() to return failure for unsupported actions. But, here we need to decide if we should return ioctl failure or transactional error. Can you pls. check that, and change the code appropriately? From a cursory look, it looks like we should returning a transactional error.

The change to return failure in ActionsExecute() for unsupported actions can be done ina subsequent change.

thanks,
-- Nithin