| Message ID | 1450725123-4158-3-git-send-email-svinturis@cloudbasesolutions.com |
|---|---|
| State | Not Applicable |
| Headers | show |
Hi Sorin, Thank you for the patch. Comments inlined. Alin. > -----Mesaj original----- > De la: dev [mailto:dev-bounces@openvswitch.org] În numele Sorin Vinturis > Trimis: Monday, December 21, 2015 9:12 PM > Către: dev@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@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) > != 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. > + > #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@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
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@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@openvswitch.org] În numele Sorin > Vinturis > Trimis: Monday, December 21, 2015 9:12 PM > Către: dev@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@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@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
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)) != 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; +} + #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;
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@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(-)