hi Alin,
Thanks for the patch.
From my reading, it seems that you are attempting to add support in the kernel
datapath to not handle the UFID attribute gracefully, even though there’s no
support for it in the kernel datapath. It would be useful to add a comment in
the code to this effect that UFID is not actually supported.
Also, IIRC, the userspace code probes the kernel datapath figure out if UFID is
supported by adding a flow and seeing if we get a INVALID error code. I see
that, you are checking if the KEY attribute is present, and if not, you are
returning NL_ERROR_INVAL. This is fine. I was wondering if you got a chance to
see if nl_transact() in userspace is able to handle this error code and
propagate it up to the dpif-netlink.c code?
Also, do you know if there’s a possibility of KEY != NULL && UFID != NULL? If
this is the case, then we should return NL_ERROR_INVAL since userspace should
not assume that kernel datapath was able to handle UFID != NULL.
I had a comment in OvsFlowNlCmdHandler() as indicated below.
Looks good otherwise. I should be able to ACK the next version.
> /* For Parsing nested OVS_FLOW_ATTR_KEY attributes.
> @@ -308,6 +310,11 @@ OvsFlowNlCmdHandler(POVS_USER_PARAMS_CONTEXT
> usrParamsCtx,
> goto done;
> }
>
> + if (nlAttrs[OVS_FLOW_ATTR_KEY] == NULL) {
> + nlError = NL_ERROR_INVAL;
> + goto done;
> + }
Looking at the code, it seems that a combination of genlMsgHdr->cmd ==
OVS_FLOW_CMD_DEL && !(nlAttrs[OVS_FLOW_ATTR_KEY]), will result in flow flush. I
guess this logic needs to be updated, to not flush the flows if cmd ==
OVS_FLOW_CMD_DEL, and nlAttrs[OVS_FLOW_ATTR_KEY] == NULL &&
nlAttrs[OVS_FLOW_ATTR_UFID] != NULL.
/* FLOW_DEL command w/o any key input is a flush case. */
if ((genlMsgHdr->cmd == OVS_FLOW_CMD_DEL) &&
(!(nlAttrs[OVS_FLOW_ATTR_KEY]) &&
!(nlAttrs[OVS_FLOW_ATTR_UFID])) {
}
thanks,
-- Nithin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
