[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