Hi Sorin, Thank you for the patch.
Comments inlined. Alin. > -----Mesaj original----- > De la: dev [mailto:dev-boun...@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 <svintu...@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 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
