[ovs-dev,2/2] ofp-actions: Make parse_reg_load() accept set_field syntax.
diff mbox

Message ID 1443214186-50469-2-git-send-email-joestringer@nicira.com
State Superseded
Headers show

Commit Message

Joe Stringer Sept. 25, 2015, 8:49 p.m. UTC
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 <joestringer@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.
---
 NEWS                  |  3 +++
 lib/ofp-actions.c     | 57 +++++++++++++++++++++++++++++++++++++--------------
 tests/ofproto-dpif.at |  6 +++---
 3 files changed, 48 insertions(+), 18 deletions(-)

Patch
diff mbox

diff --git a/NEWS b/NEWS
index ca22c8e..ef0348b 100644
--- a/NEWS
+++ b/NEWS
@@ -21,6 +21,9 @@  Post-v2.4.0
      targets to run a new system testsuite.  These tests can be run inside
      a Vagrant box.  See INSTALL.md for details
    - Dropped support for GRE64 tunnel.
+   - ovs-ofctl syntax for the "reg_load" action now validates inputs to
+     ensure they are of the format that the field accepts. Simply loading
+     hexadecimal values into any field will no longer work.
 
 
 v2.4.0 - 20 Aug 2015
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index d33e429..153c712 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -2571,36 +2571,63 @@  error:
 }
 
 static char * OVS_WARN_UNUSED_RESULT
-parse_reg_load(char *arg, struct ofpbuf *ofpacts)
+parse_reg_load__(char *arg, struct ofpbuf *ofpacts)
 {
     struct ofpact_set_field *sf = ofpact_put_reg_load(ofpacts);
-    const char *full_arg = arg;
-    uint64_t value = strtoull(arg, (char **) &arg, 0);
+    union mf_value immediate, mask;
+    const struct mf_field *mf;
+    char *key, *value, *delim;
     struct mf_subfield dst;
-    char *error;
+    char *error = NULL;
 
-    if (strncmp(arg, "->", 2)) {
-        return xasprintf("%s: missing `->' following value", full_arg);
+    error = set_field_split_str(arg, &key, &value, &delim);
+    if (error) {
+        return error;
     }
-    arg += 2;
-    error = mf_parse_subfield(&dst, arg);
+
+    error = mf_parse_subfield(&dst, key);
     if (error) {
         return error;
     }
+    delim[0] = '\0';
+    mf = dst.field;
 
-    if (dst.n_bits < 64 && (value >> dst.n_bits) != 0) {
-        return xasprintf("%s: value %"PRIu64" does not fit into %d bits",
-                         full_arg, value, dst.n_bits);
+    error = set_field_parse__(mf, key, value, &immediate, &mask);
+    if (error) {
+        return error;
     }
 
-    sf->field = dst.field;
+    if (!bitwise_is_all_zeros(&immediate, mf->n_bytes, dst.n_bits,
+                              mf->n_bytes * 8 - dst.n_bits)) {
+        struct ds ds;
+
+        ds_init(&ds);
+        mf_format(mf, &immediate, &mask, &ds);
+        error = xasprintf("%s: value %s does not fit into %d bits",
+                          arg, ds_cstr(&ds), dst.n_bits);
+        ds_destroy(&ds);
+        return error;
+    }
+
+    sf->field = mf;
     memset(&sf->value, 0, sizeof sf->value);
-    bitwise_put(value, &sf->value, dst.field->n_bytes, dst.ofs, dst.n_bits);
-    bitwise_put(UINT64_MAX, &sf->mask,
-                dst.field->n_bytes, dst.ofs, dst.n_bits);
+    bitwise_copy(&immediate, mf->n_bytes, 0, &sf->value, mf->n_bytes,
+                 dst.ofs, dst.n_bits);
+    bitwise_copy(&mask, mf->n_bytes, 0, &sf->mask, mf->n_bytes,
+                 dst.ofs, dst.n_bits);
+
     return NULL;
 }
 
+static char * OVS_WARN_UNUSED_RESULT
+parse_reg_load(char *arg, struct ofpbuf *ofpacts)
+{
+    char *copy = xstrdup(arg);
+    char *error = parse_reg_load__(copy, ofpacts);
+    free(copy);
+    return error;
+}
+
 static void
 format_SET_FIELD(const struct ofpact_set_field *a, struct ds *s)
 {
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 9609d2d..c211a50 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -1292,7 +1292,7 @@  cookie=0x7 table=5 in_port=84 actions=load:5->NXM_NX_REG4[[]],load:6->NXM_NX_TUN
 cookie=0x8 table=6 in_port=85 actions=mod_tp_src:85,controller,resubmit(86,7)
 cookie=0x9 table=7 in_port=86 actions=mod_tp_dst:86,controller,controller
 cookie=0xa dl_src=40:44:44:44:44:41 actions=mod_vlan_vid:99,mod_vlan_pcp:1,controller
-cookie=0xd dl_src=80:88:88:88:88:88 arp actions=load:2->OXM_OF_ARP_OP[[]],controller,load:0xc0a88001->OXM_OF_ARP_SPA[[]],controller,load:0x404444444441->OXM_OF_ARP_THA[[]],load:0x01010101->OXM_OF_ARP_SPA[[]],load:0x02020202->OXM_OF_ARP_TPA[[]],controller
+cookie=0xd dl_src=80:88:88:88:88:88 arp actions=load:2->OXM_OF_ARP_OP[[]],controller,load:192.168.128.1->OXM_OF_ARP_SPA[[]],controller,load:40:44:44:44:44:41->OXM_OF_ARP_THA[[]],load:1.1.1.1->OXM_OF_ARP_SPA[[]],load:2.2.2.2->OXM_OF_ARP_TPA[[]],controller
 ])
 AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
 
@@ -1564,7 +1564,7 @@  cookie=0xd dl_src=60:66:66:66:66:66 actions=pop_mpls:0x0800,controller
 cookie=0xc dl_src=70:77:77:77:77:77 actions=push_mpls:0x8848,load:1000->OXM_OF_MPLS_LABEL[[]],load:7->OXM_OF_MPLS_TC[[]],controller
 
 cookie=0xd dl_src=60:66:66:66:00:01 actions=pop_mpls:0x0800,dec_ttl,controller
-cookie=0xd dl_src=60:66:66:66:00:02 actions=pop_mpls:0x0800,load:0xa000001->OXM_OF_IPV4_DST[[]],controller
+cookie=0xd dl_src=60:66:66:66:00:02 actions=pop_mpls:0x0800,load:10.0.0.1->OXM_OF_IPV4_DST[[]],controller
 cookie=0xd dl_src=60:66:66:66:00:03 actions=pop_mpls:0x0800,move:OXM_OF_IPV4_DST[[]]->OXM_OF_IPV4_SRC[[]],controller
 cookie=0xd dl_src=60:66:66:66:00:04 actions=pop_mpls:0x0800,push:OXM_OF_IPV4_DST[[]],pop:OXM_OF_IPV4_SRC[[]],controller
 cookie=0xd dl_src=60:66:66:66:00:05 actions=pop_mpls:0x0800,multipath(eth_src,50,modulo_n,256,0,OXM_OF_IPV4_SRC[[0..7]]),controller
@@ -3020,7 +3020,7 @@  OVS_VSWITCHD_START
 ADD_OF_PORTS([br0], [1], [2])
 
 ovs-vsctl -- set Interface p2 type=dummy options:pcap=p2.pcap
-ovs-ofctl add-flow br0 'in_port=1,arp actions=load:2->OXM_OF_ARP_OP[[]],2,load:0xc0a88001->OXM_OF_ARP_SPA[[]],2,load:0x404444444441->OXM_OF_ARP_THA[[]],2'
+ovs-ofctl add-flow br0 'in_port=1,arp actions=load:2->OXM_OF_ARP_OP[[]],2,load:192.168.128.1->OXM_OF_ARP_SPA[[]],2,load:40:44:44:44:44:41->OXM_OF_ARP_THA[[]],2'
 
 # Input some packets that should follow the arp modification slow-path.
 for i in 1 2 3; do