[ovs-dev,PATCHv4,02/11] ofp-actions: Extend reg_load parsing to >64bits.
diff mbox

Message ID 1443820578-9287-3-git-send-email-joestringer@nicira.com
State Accepted
Headers show

Commit Message

Joe Stringer Oct. 2, 2015, 9:16 p.m. UTC
Previously, reg_load would only understand 64-bit values passed to it.
This patch extends the parsing to handle larger fields, if they are
specified in hexadecimal. Internally they are stored as a single action,
but they are converted into multiple 64-bit modifications when
re-serialised.

Signed-off-by: Joe Stringer <joestringer@nicira.com>
---
v4: Reduce change to allowing >64bit values to be specified in hex.
---
 lib/ofp-actions.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

Comments

Ben Pfaff Oct. 5, 2015, 10:27 p.m. UTC | #1
On Fri, Oct 02, 2015 at 02:16:09PM -0700, Joe Stringer wrote:
> Previously, reg_load would only understand 64-bit values passed to it.
> This patch extends the parsing to handle larger fields, if they are
> specified in hexadecimal. Internally they are stored as a single action,
> but they are converted into multiple 64-bit modifications when
> re-serialised.
> 
> Signed-off-by: Joe Stringer <joestringer@nicira.com>
> ---
> v4: Reduce change to allowing >64bit values to be specified in hex.

Would you mind adding a test that exercises a >64 bit example, at least
to demonstrate that it is parsed and formatted correctly?

Acked-by: Ben Pfaff <blp@nicira.com>
Joe Stringer Oct. 6, 2015, 1:34 a.m. UTC | #2
On 5 October 2015 at 15:27, Ben Pfaff <blp@nicira.com> wrote:
> On Fri, Oct 02, 2015 at 02:16:09PM -0700, Joe Stringer wrote:
>> Previously, reg_load would only understand 64-bit values passed to it.
>> This patch extends the parsing to handle larger fields, if they are
>> specified in hexadecimal. Internally they are stored as a single action,
>> but they are converted into multiple 64-bit modifications when
>> re-serialised.
>>
>> Signed-off-by: Joe Stringer <joestringer@nicira.com>
>> ---
>> v4: Reduce change to allowing >64bit values to be specified in hex.
>
> Would you mind adding a test that exercises a >64 bit example, at least
> to demonstrate that it is parsed and formatted correctly?

Sure. I was using a test that is added later with the ct_label
support, but IPv6 should be able to be handled in a similar manner.

> Acked-by: Ben Pfaff <blp@nicira.com>

Thanks!

Patch
diff mbox

diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 6c9e1cb..0b22ce1 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -2571,10 +2571,9 @@  static char * OVS_WARN_UNUSED_RESULT
 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);
     struct mf_subfield dst;
     char *key, *value_str;
+    union mf_value value;
     char *error;
 
     error = set_field_split_str(arg, &key, &value_str, NULL);
@@ -2587,16 +2586,29 @@  parse_reg_load(char *arg, struct ofpbuf *ofpacts)
         return error;
     }
 
-    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);
+    if (parse_int_string(value_str, (uint8_t *)&value, dst.field->n_bytes,
+                         &key)) {
+        return xasprintf("%s: cannot parse integer value", arg);
+    }
+
+    if (!bitwise_is_all_zeros(&value, dst.field->n_bytes, dst.n_bits,
+                              dst.field->n_bytes * 8 - dst.n_bits)) {
+        struct ds ds;
+
+        ds_init(&ds);
+        mf_format(dst.field, &value, NULL, &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 = dst.field;
     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(&value, dst.field->n_bytes, 0, &sf->value,
+                 dst.field->n_bytes, dst.ofs, dst.n_bits);
+    bitwise_one(&sf->mask, dst.field->n_bytes, dst.ofs, dst.n_bits);
+
     return NULL;
 }