On Tue, May 01, 2012 at 06:14:52PM +0900, Simon Horman wrote: > This code, which leverages the existing NXM implementation, > adds parsing and serialisation of OXM matches. Test cases > have also been provided. > > This patch only implements parsing and serialisation of OXM fields that > are already handled by NXM. > > It should be noted that in OXM ports are 32bit whereas in NXM they > are 16 bit. This has been handled as a special case as all other field > widths are the same in both OXM and NXM. > > This patch does not address differences in wildcarding between OXM and NXM. > It is planned that liberal wildcarding policy dictated by either OXM or > NXM will be implemented. > > This patch also does not address any (subtle?) differences between > OXM and NXM treatment of specific fields. It is envisages that his > can be handled by subsequent patches. > > Signed-off-by: Simon Horman <[email protected]>
I've been sitting on this patch for a couple of days, sorry about that Simon. I have a couple of thoughts about it. They're the sorts of thoughts where ideally I would have tried implementing them myself instead of asking you to do it. I apologize that I didn't; time is short this week. First, I find myself wondering whether there is value in passing an indication of OXM vs. NXM into functions that parse [ON]XM. The header values are, intentionally, non-overlapping. I think that we could do OK with supporting both formats in any context where one or the other is allowed. Functions that produce [ON]XM obviously do need to know which one to produce. It's really sad that the special case for in_port leaked through into mf_set(_value). I had thought/assumed that it could be localized into nx_pull_match__(). Do you see a way to do that? The existence of ofputil_native_match_format_is_oxm() seems wrong to me. The ofputil_protocol enum is intended to answer all flow format questions. In this case, I see that ofputil_native_match_format_is_oxm() has only two callers. I'd argue that each of these existing callers should hard-code that it is using NXM, because each of these callers is in the context of a Nicira extension that uses NXM. (But, further, if we get drop the "is this OXM?" parameters from functions for parsing [ON]XM, as I suggested earlier, then both callers disappear anyway.) As a minor style point, I'm not sure I like using a bool to distinguish OXM and NXM. It could be more readable to use an enum, something like XFF_OXM and XFF_NXM (xff = extensible flow format). _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
