On 29 September 2015 at 23:35, Ben Pfaff <b...@nicira.com> wrote: > On Tue, Sep 29, 2015 at 01:40:25PM -0700, Joe Stringer wrote: >> Commit 7eb4b1f1d "ofp-actions: Support OF1.5 (draft) masked Set-Field, >> merge with reg_load." purportedly merged the set_field and reg_load >> fields, including the commandline parsing to use the metafield syntax >> like "load:192.168.0.1->OXM_OF_IPV4_SRC". However, the actual reg_load >> parsing code was not updated. >> >> This patch takes advantage of the newly refactored parse_set_field() >> helpers to make parse_reg_load() conform to the documentation, which >> allows more thorough validation of inputs, and allows reg_load to parse >> fields that are wider than 64 bits, as well as masked fields. >> >> Note that this disallows syntax that was previously allowed, such as: >> load:0x0a000001->OXM_OF_IPV4_SRC >> >> Signed-off-by: Joe Stringer <joestrin...@nicira.com> >> --- >> >> Note that v2.4.0 includes the aforementioned commit; we should consider >> either backporting this fix to branch-2.4, or reverting the >> documentation modification on branch-2.4 to be consistent with the >> previous syntax. > > I'm having a little trouble following here. Can you give an example of > a "load" or a "set_field" for which (before this commit) the > documentation and the implementation disagree?
Hmm, reviewing it looks like I'm mistaken. I saw the original description removed in the patch but didn't notice that the new text still described the original syntax. > Also I think that this commit leads to an inconsistency between parsing > and formatting. For example, take a look at ofproto-dpif.at line 1537, > which is part of the output of "ovs-ofctl dump-flows": > > cookie=0xd, n_packets=3, n_bytes=180, arp,dl_src=80:88:88:88:88:88 > actions=load:0x2->NXM_OF_ARP_OP[[]],CONTROLLER:65535,load:0xc0a88001->NXM_OF_ARP_SPA[[]],CONTROLLER:65535,load:0x404444444441->NXM_NX_ARP_THA[[]],load:0x1010101->NXM_OF_ARP_SPA[[]],load:0x2020202->NXM_OF_ARP_TPA[[]],CONTROLLER:65535 > > It contains (after m4 processing) "load:0xc0a88001->NXM_OF_ARP_SPA[]", > which "ovs-ofctl add-flow" now rejects: > > $ ovs-ofctl add-flow br0 "actions=load:0xc0a88001->NXM_OF_ARP_SPA[]" > ovs-ofctl: 0xc0a88001: invalid IP address > > I'm pretty nervous about rejecting syntax that's been valid for a long > time. Fair enough, I can drop this approach, so these issues should not be problematic. I think the main change I was looking for in this series is to support >64bit values by switching the value parsing to use parse_int_string() instead of strtoull() directly, using the mf field's expected length to figure out how long the string is allowed to be. The other matter was that there seems to be a discrepancy between the error checking code in set_field vs reg_load, for instance checking whether the field is read-only, or passing it through mf_is_value_valid(). Again, that doesn't require the current approach to address. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
