[ovs-dev] [PATCH 2/2] datapath-windows: Accept MPLS feature probe.

Sorin Vinturis svinturis at cloudbasesolutions.com
Tue Jan 19 16:06:48 UTC 2016


Hi Alin,

Thanks for reviewing this patch. Please see my answers inline.

-Sorin

-----Original Message-----
From: Alin Serdean 
Sent: Tuesday, 19 January, 2016 12:29
To: Sorin Vinturis; dev at openvswitch.org
Subject: RE: [PATCH 2/2] datapath-windows: Accept MPLS feature probe.

Hi Sorin,

Thank you for the patch.

Comments inlined.

Alin.

> -----Mesaj original-----
> De la: dev [mailto:dev-bounces at openvswitch.org] În numele Sorin 
> Vinturis
> Trimis: Monday, December 21, 2015 9:12 PM
> Către: dev at openvswitch.org
> Subiect: [ovs-dev] [PATCH 2/2] datapath-windows: Accept MPLS feature 
> probe.
> 
> Currently all feature probe messages sent from userspace are 
> suppressed by the OVS extension.
> 
> This patch changes the current behaviour to allow feature probe for MPLS.
> 
> Signed-off-by: Sorin Vinturis <svinturis at cloudbasesolutions.com>
> ---
>  datapath-windows/ovsext/Flow.c                 | 136 ++++++++++++++++---------
>  datapath-windows/ovsext/Mpls.h                 |  24 +++++
>  datapath-windows/ovsext/Netlink/NetlinkError.h |   3 +
>  3 files changed, 116 insertions(+), 47 deletions(-)
> 
> diff --git a/datapath-windows/ovsext/Flow.c b/datapath- 
> windows/ovsext/Flow.c index 99a89e6..a0f91c6 100644
> --- a/datapath-windows/ovsext/Flow.c
> +++ b/datapath-windows/ovsext/Flow.c
> @@ -46,7 +46,9 @@ static VOID DeleteAllFlows(OVS_DATAPATH *datapath); 
> static NTSTATUS AddFlow(OVS_DATAPATH *datapath, OvsFlow *flow); static 
> VOID FreeFlow(OvsFlow *flow);  static VOID __inline 
> *GetStartAddrNBL(const NET_BUFFER_LIST *_pNB); -static NTSTATUS 
> _MapNlToFlowPut(POVS_MESSAGE msgIn, PNL_ATTR keyAttr,
> +static NTSTATUS _MapNlToFlowPut(POVS_MESSAGE msgIn,
> +                                PNL_ATTR* keyAttrs,
> +                                PNL_ATTR* tunnelAttrs,
>                                  PNL_ATTR actionAttr,
>                                  PNL_ATTR flowAttrClear,
>                                  OvsFlowPut *mappedFlow); @@ -86,6 
> +88,7 @@ static NTSTATUS _MapFlowMplsKeyToNlKey(PNL_BUFFER nlBuf,  
> static NTSTATUS OvsDoDumpFlows(OvsFlowDumpInput *dumpInput,
>                                 OvsFlowDumpOutput *dumpOutput,
>                                 UINT32 *replyLen);
> +static NTSTATUS OvsProbeSupportedFeature(PNL_ATTR *keyAttr);
> 
> 
>  #define OVS_FLOW_TABLE_SIZE 2048
> @@ -256,8 +259,12 @@
> OvsFlowNlCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
>      PNL_MSG_HDR nlMsgHdr = &(msgIn->nlMsg);
>      PGENL_MSG_HDR genlMsgHdr = &(msgIn->genlMsg);
>      POVS_HDR ovsHdr = &(msgIn->ovsHdr);
> -    PNL_ATTR flowAttrs[__OVS_FLOW_ATTR_MAX];
> +    PNL_ATTR flowAttrs[__OVS_FLOW_ATTR_MAX] = { NULL };
> +    PNL_ATTR keyAttrs[__OVS_KEY_ATTR_MAX] = { NULL };
> +    PNL_ATTR tunnelAttrs[__OVS_TUNNEL_KEY_ATTR_MAX] = { NULL };
>      UINT32 attrOffset = NLMSG_HDRLEN + GENL_HDRLEN + OVS_HDRLEN;
> +    UINT32 keyAttrOffset = 0;
> +    UINT32 tunnelKeyAttrOffset = 0;
>      OvsFlowPut mappedFlow;
>      OvsFlowStats stats;
>      struct ovs_flow_stats replyStats; @@ -312,14 +319,52 @@ 
> OvsFlowNlCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
>         goto done;
>      }
> 
> -    if (flowAttrs[OVS_FLOW_ATTR_PROBE]) {
> -        OVS_LOG_ERROR("Attribute OVS_FLOW_ATTR_PROBE not
> supported");
> +    keyAttrOffset = (UINT32)((PCHAR)flowAttrs[OVS_FLOW_ATTR_KEY] -
> +                    (PCHAR)nlMsgHdr);
> +
> +    /* Get flow keys attributes */
> +    if ((NlAttrParseNested(nlMsgHdr, keyAttrOffset,
> +        NlAttrLen(flowAttrs[OVS_FLOW_ATTR_KEY]),
> +        nlFlowKeyPolicy, ARRAY_SIZE(nlFlowKeyPolicy),
> +        keyAttrs, ARRAY_SIZE(keyAttrs)))
> +        != TRUE) {
> +        OVS_LOG_ERROR("Key Attr Parsing failed for msg: %p",
> +                      nlMsgHdr);
> +        rc = STATUS_INVALID_PARAMETER;
>          goto done;
>      }
> 
> -    if ((rc = _MapNlToFlowPut(msgIn, flowAttrs[OVS_FLOW_ATTR_KEY],
> -        flowAttrs[OVS_FLOW_ATTR_ACTIONS],
> flowAttrs[OVS_FLOW_ATTR_CLEAR],
> -         &mappedFlow))
> +    if (flowAttrs[OVS_FLOW_ATTR_PROBE]) {
> +        rc = OvsProbeSupportedFeature(keyAttrs);
> +        if (rc != STATUS_SUCCESS) {
> +            nlError = NlMapStatusToNlErr(rc);
> +            goto done;
> +        }
> +    }
> +
> +    if (keyAttrs[OVS_KEY_ATTR_TUNNEL]) {
> +        tunnelKeyAttrOffset = (UINT32)((PCHAR)
> +            (keyAttrs[OVS_KEY_ATTR_TUNNEL])
> +            - (PCHAR)nlMsgHdr);
> +
> +        /* Get tunnel keys attributes */
> +        if ((NlAttrParseNested(nlMsgHdr, tunnelKeyAttrOffset,
> +            NlAttrLen(keyAttrs[OVS_KEY_ATTR_TUNNEL]),
> +            nlFlowTunnelKeyPolicy,
> +            ARRAY_SIZE(nlFlowTunnelKeyPolicy),
> +            tunnelAttrs, ARRAY_SIZE(tunnelAttrs)))
> +            != TRUE) {
> +            OVS_LOG_ERROR("Tunnel key Attr Parsing failed for msg: %p",
> +                          nlMsgHdr);
> +            rc = STATUS_INVALID_PARAMETER;
> +            goto done;
> +        }
> +    }
> +
> +    if ((rc = _MapNlToFlowPut(msgIn, keyAttrs, tunnelAttrs,
> +                              flowAttrs[OVS_FLOW_ATTR_ACTIONS],
> +                              flowAttrs[OVS_FLOW_ATTR_CLEAR],
> +                              &mappedFlow))
[Alin Gabriel Serdean: ] I am not in favor of modifying MapNlToFlowPut, please leave it intact and just add the logic for what you need (OvsProbeSupportedFeature)
[SV]: I have made these changes to avoid double parsing of the NL buffer to get the key attributes, but I can restore them and add the NL buffer parsing in OvsProbeSupportedFeature.

>          != STATUS_SUCCESS) {
>          OVS_LOG_ERROR("Conversion to OvsFlowPut failed");
>          goto done;
> @@ -1233,51 +1278,14 @@ done:
>   *----------------------------------------------------------------------------
>   */
>  static NTSTATUS
> -_MapNlToFlowPut(POVS_MESSAGE msgIn, PNL_ATTR keyAttr,
> +_MapNlToFlowPut(POVS_MESSAGE msgIn, PNL_ATTR* keyAttrs,
> PNL_ATTR*
> +tunnelAttrs,
>                  PNL_ATTR actionAttr, PNL_ATTR flowAttrClear,
>                  OvsFlowPut *mappedFlow)  {
>      NTSTATUS rc = STATUS_SUCCESS;
> -    PNL_MSG_HDR nlMsgHdr = &(msgIn->nlMsg);
>      PGENL_MSG_HDR genlMsgHdr = &(msgIn->genlMsg);
>      POVS_HDR ovsHdr = &(msgIn->ovsHdr);
> 
> -    UINT32 keyAttrOffset = (UINT32)((PCHAR)keyAttr - (PCHAR)nlMsgHdr);
> -    UINT32 tunnelKeyAttrOffset;
> -
> -    PNL_ATTR keyAttrs[__OVS_KEY_ATTR_MAX] = {NULL};
> -    PNL_ATTR tunnelAttrs[__OVS_TUNNEL_KEY_ATTR_MAX] = {NULL};
> -
> -    /* Get flow keys attributes */
> -    if ((NlAttrParseNested(nlMsgHdr, keyAttrOffset, NlAttrLen(keyAttr),
> -                           nlFlowKeyPolicy, ARRAY_SIZE(nlFlowKeyPolicy),
> -                           keyAttrs, ARRAY_SIZE(keyAttrs)))
> -                           != TRUE) {
> -        OVS_LOG_ERROR("Key Attr Parsing failed for msg: %p",
> -                       nlMsgHdr);
> -        rc = STATUS_INVALID_PARAMETER;
> -        goto done;
> -    }
> -
> -    if (keyAttrs[OVS_KEY_ATTR_TUNNEL]) {
> -        tunnelKeyAttrOffset = (UINT32)((PCHAR)
> -                              (keyAttrs[OVS_KEY_ATTR_TUNNEL])
> -                              - (PCHAR)nlMsgHdr);
> -
> -        /* Get tunnel keys attributes */
> -        if ((NlAttrParseNested(nlMsgHdr, tunnelKeyAttrOffset,
> -                               NlAttrLen(keyAttrs[OVS_KEY_ATTR_TUNNEL]),
> -                               nlFlowTunnelKeyPolicy,
> -                               ARRAY_SIZE(nlFlowTunnelKeyPolicy),
> -                               tunnelAttrs, ARRAY_SIZE(tunnelAttrs)))
> -                               != TRUE) {
> -            OVS_LOG_ERROR("Tunnel key Attr Parsing failed for msg: %p",
> -                           nlMsgHdr);
> -            rc = STATUS_INVALID_PARAMETER;
> -            goto done;
> -        }
> -    }
> -
>      _MapKeyAttrToFlowPut(keyAttrs, tunnelAttrs,
>                           &(mappedFlow->key));
> 
> @@ -1290,9 +1298,8 @@ _MapNlToFlowPut(POVS_MESSAGE msgIn, PNL_ATTR 
> keyAttr,
>      mappedFlow->dpNo = ovsHdr->dp_ifindex;
> 
>      _MapNlToFlowPutFlags(genlMsgHdr, flowAttrClear,
> -                                mappedFlow);
> +                         mappedFlow);
> 
> -done:
>      return rc;
>  }
> 
> @@ -2520,4 +2527,39 @@ OvsTunKeyAttrSize(void)
>           + NlAttrTotalSize(2);   /* OVS_TUNNEL_KEY_ATTR_TP_DST */
>  }
> 
> +/*
> +
> +*--------------------------------------------------------------------
> +--
> +------
> + *  OvsProbeSupportedFeature --
> + *    Verifies if the probed feature is supported.
> + *
> + * Results:
> + *   STATUS_SUCCESS if the probed feature is supported.
> +
> +*--------------------------------------------------------------------
> +--
> +------
> + */
> +static NTSTATUS
> +OvsProbeSupportedFeature(PNL_ATTR *keyAttrs) {
> +    NTSTATUS status = STATUS_SUCCESS;
> +
> +    if (keyAttrs[OVS_KEY_ATTR_MPLS] &&
> +        keyAttrs[OVS_KEY_ATTR_ETHERTYPE]) {
> +        ovs_be16 ethType =
> + NlAttrGetU16(keyAttrs[OVS_KEY_ATTR_ETHERTYPE]);
> +
> +        if (OvsEthertypeIsMpls(ethType)) {
> +            if (!OvsCountMplsLabels(keyAttrs[OVS_KEY_ATTR_MPLS])) {
> +                OVS_LOG_ERROR("Maximum supported MPLS labels exceeded.");
> +                status = STATUS_INVALID_MESSAGE;
> +            }
> +        } else {
> +            OVS_LOG_ERROR("Wrong ethertype for MPLS attribute.");
> +            status = STATUS_INVALID_PARAMETER;
> +        }
> +    } else {
> +        OVS_LOG_ERROR("Probed feature not supported.");
> +        status = STATUS_NOT_SUPPORTED;
> +    }
> +
> +    return status;
> +}
[Alin Gabriel Serdean: ] Since at the moment we do not have a dry run of the actions. Can you change it into a switch for future use? 
Regarding the returned value, I think in all cases it should be STATUS_INVALID_PARAMETER.
[SV]: Here the use of a switch is not possible because the key attributes are different for each probe message and there is no a common key attributes value that can be used to identify the probe message.

> +
>  #pragma warning( pop )
> diff --git a/datapath-windows/ovsext/Mpls.h b/datapath- 
> windows/ovsext/Mpls.h index 3508233..4b5c30c 100644
> --- a/datapath-windows/ovsext/Mpls.h
> +++ b/datapath-windows/ovsext/Mpls.h
> @@ -60,4 +60,28 @@ OvsEthertypeIsMpls(ovs_be16 ethertype)
>             ethertype == htons(ETH_TYPE_MPLS_MCAST);  }
> 
> +/* Returns the number of MPLS LSEs present in 'a'
> + *
> + * Counts 'flow''s MPLS label stack entries (LESs) stopping at the 
> +first
> + * entry that has the bottom of stack (BOS) bit set. If no such entry 
> +exists,
> + * then zero is returned, meaning that the maximum number of 
> +supported
> + * MPLS LSEs exceeded.
> + */
> +__inline UINT32
> +OvsCountMplsLabels(PNL_ATTR a)
> +{
> +    const MPLSHdr *mpls = NlAttrGet(a);
> +    UINT32 count = 0;
> +    BOOLEAN bos = FALSE;
> +
> +    for (count = 0; count < FLOW_MAX_MPLS_LABELS; count++) {
> +        if ((mpls + count)->lse & htonl(MPLS_BOS_MASK)) {
> +            bos = TRUE;
> +            break;
> +        }
> +    }
> +
> +    return bos ? ++count : 0;
> +}
> +
>  #endif /* __MPLS_H_ */
> diff --git a/datapath-windows/ovsext/Netlink/NetlinkError.h 
> b/datapath- windows/ovsext/Netlink/NetlinkError.h
> index eefa89e..36115c8 100644
> --- a/datapath-windows/ovsext/Netlink/NetlinkError.h
> +++ b/datapath-windows/ovsext/Netlink/NetlinkError.h
> @@ -229,6 +229,9 @@ NlMapStatusToNlErr(NTSTATUS status)
>      case STATUS_OBJECT_NAME_EXISTS:
>        ret = NL_ERROR_EXIST;
>        break;
> +    case STATUS_INVALID_MESSAGE:
> +      ret = NL_ERROR_BADMSG;
> +      break;
>      default:
>        ret = NL_ERROR_OTHER;
>        break;
> --
> 1.9.0.msysgit.0
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev


More information about the dev mailing list