Hi Ben, Thanks for the review. a. My bad on the Signed off by, will take care of it in V2. b. Sure, I will add the test case as well. c. I will update the ovn-sb.xml as well. d. Yes, openflow does not restrict the usage of registers for connection tracker, I think probably because of lack of use case thus far, it was missing in OVN.
Thanks again for review, V2 will have all the comments addressed. Regards, Ankur -----Original Message----- From: Ben Pfaff <b...@ovn.org> Sent: Tuesday, February 5, 2019 1:30 PM To: Ankur Sharma <ankur.sha...@nutanix.com> Cc: ovs-dev@openvswitch.org Subject: Re: [ovs-dev] [RFC PATCH v1 2/3] OVN ACL: Allow ct_mark and ct_label values to be set from register as well 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
