On Fri, Jan 11, 2019 at 12:16:38AM +0000, Ankur Sharma wrote: > OVN allows only an integer (or masked integer) to be assigned to > ct_mark and ct_label. > > This patch, enhances the parser code to allow ct_mark and ct_label > to be assigned from 32 bit registers (MFF_REG0 - MFF_REG15) and 128 > bit registers (MFF_XXREG0 - MFF_XXREG3) respectively. > > signed-off-by: Ankur Sharma <ankur.sha...@nutanix.com>
Thanks for the patch! Please capitalize "Signed-off-by" <-- just like that. When we add new support for OVN actions, we also add tests to the "ovn -- action parsing" test in ovn.at. Please add a test for a correctly formed action as well as at least one negative form that exercises each of the new error messages. Please also document the new form of the action in ovn-sb.xml. The error messages don't seem all that user friendly. Make sure that they clearly point out the problem. (That's probably easier when you add the tests.) The new forms break the level of abstraction that OVN fields are supposed to provide, that is, please use OVN field names rather than OpenFlow ones. I don't know why only registers are allowed. I don't believe that such a restriction exists at the OpenFlow level. I also don't know why only full, aligned registers are allowed. I don't think that restriction exists at the OpenFlow level either. It looks like the indentation is wrong here in encode_CT_COMMIT(). Thanks, Ben. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
