diff mbox

[ovs-dev,3/3] tun-metadata: Provide error messages during auto-allocation.

Message ID 1441076226-60939-3-git-send-email-jesse@nicira.com
State Accepted
Headers show

Commit Message

Jesse Gross Sept. 1, 2015, 2:57 a.m. UTC 
In cases where we don't have a map of tunnel metadata options (such
as with ovs-ofctl) we dynamically allocate them as part of the match.
However, dynamic allocation brings the possibility of errors such as
duplicate entries or running out of space. Up until now, anything that
would cause an error was silently ignored. Since that is not very user
friendly, this adds a mechanism for reporting these types of errors.

Signed-off-by: Jesse Gross <jesse@nicira.com>
---
 lib/meta-flow.c    | 52 ++++++++++++++++++++++++++++++++++++++--------------
 lib/meta-flow.h    |  6 +++---
 lib/nx-match.c     | 10 +++++++++-
 lib/ofp-parse.c    |  2 +-
 lib/tun-metadata.c | 35 ++++++++++++++++++++++++++++-------
 lib/tun-metadata.h |  9 +++++----
 6 files changed, 84 insertions(+), 30 deletions(-)

Comments

Ben Pfaff Sept. 8, 2015, 9:11 p.m. UTC | #1 
On Mon, Aug 31, 2015 at 07:57:06PM -0700, Jesse Gross wrote:
> In cases where we don't have a map of tunnel metadata options (such
> as with ovs-ofctl) we dynamically allocate them as part of the match.
> However, dynamic allocation brings the possibility of errors such as
> duplicate entries or running out of space. Up until now, anything that
> would cause an error was silently ignored. Since that is not very user
> friendly, this adds a mechanism for reporting these types of errors.
> 
> Signed-off-by: Jesse Gross <jesse@nicira.com>

Some of the functions that take an err_str parameter don't set it to
NULL initially, e.g. at least tun_metadata_set_match(),
metadata_loc_from_match().

Is it possible to add some tests that provoke these messages?

Acked-by: Ben Pfaff <blp@nicira.com>
Jesse Gross Sept. 9, 2015, 4:51 p.m. UTC | #2 
On Tue, Sep 8, 2015 at 2:11 PM, Ben Pfaff <blp@nicira.com> wrote:
> On Mon, Aug 31, 2015 at 07:57:06PM -0700, Jesse Gross wrote:
>> In cases where we don't have a map of tunnel metadata options (such
>> as with ovs-ofctl) we dynamically allocate them as part of the match.
>> However, dynamic allocation brings the possibility of errors such as
>> duplicate entries or running out of space. Up until now, anything that
>> would cause an error was silently ignored. Since that is not very user
>> friendly, this adds a mechanism for reporting these types of errors.
>>
>> Signed-off-by: Jesse Gross <jesse@nicira.com>
>
> Some of the functions that take an err_str parameter don't set it to
> NULL initially, e.g. at least tun_metadata_set_match(),
> metadata_loc_from_match().

This was semi-intentional the sense that their calling functions
already do this. However, it's probably not the most robust thing to
do, so I added an explicit initialization.

> Is it possible to add some tests that provoke these messages?

It's a good idea, I added a couple of tests.

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

Thanks, I changed the pieces that I mentioned and applied this series to master.
diff mbox

Patch

diff --git a/lib/meta-flow.c b/lib/meta-flow.c
index e2e61f8..224ba53 100644
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
@@ -796,11 +796,19 @@  mf_get_value(const struct mf_field *mf, const struct flow *flow,
 
 /* Makes 'match' match field 'mf' exactly, with the value matched taken from
  * 'value'.  The caller is responsible for ensuring that 'match' meets 'mf''s
- * prerequisites. */
+ * prerequisites.
+ *
+ * If non-NULL, 'err_str' returns a malloc'ed string describing any errors
+ * with the request or NULL if there is no error. The caller is reponsible
+ * for freeing the string. */
 void
 mf_set_value(const struct mf_field *mf,
-             const union mf_value *value, struct match *match)
+             const union mf_value *value, struct match *match, char **err_str)
 {
+    if (err_str) {
+        *err_str = NULL;
+    }
+
     switch (mf->id) {
     case MFF_DP_HASH:
         match_set_dp_hash(match, ntohl(value->be32));
@@ -836,7 +844,7 @@  mf_set_value(const struct mf_field *mf,
         match_set_tun_ttl(match, value->u8);
         break;
     CASE_MFF_TUN_METADATA:
-        tun_metadata_set_match(mf, value, NULL, match);
+        tun_metadata_set_match(mf, value, NULL, match, err_str);
         break;
 
     case MFF_METADATA:
@@ -1364,10 +1372,18 @@  mf_is_set(const struct mf_field *mf, const struct flow *flow)
 /* Makes 'match' wildcard field 'mf'.
  *
  * The caller is responsible for ensuring that 'match' meets 'mf''s
- * prerequisites. */
+ * prerequisites.
+ *
+ * If non-NULL, 'err_str' returns a malloc'ed string describing any errors
+ * with the request or NULL if there is no error. The caller is reponsible
+ * for freeing the string. */
 void
-mf_set_wild(const struct mf_field *mf, struct match *match)
+mf_set_wild(const struct mf_field *mf, struct match *match, char **err_str)
 {
+    if (err_str) {
+        *err_str = NULL;
+    }
+
     switch (mf->id) {
     case MFF_DP_HASH:
         match->flow.dp_hash = 0;
@@ -1406,7 +1422,7 @@  mf_set_wild(const struct mf_field *mf, struct match *match)
         match_set_tun_ttl_masked(match, 0, 0);
         break;
     CASE_MFF_TUN_METADATA:
-        tun_metadata_set_match(mf, NULL, NULL, match);
+        tun_metadata_set_match(mf, NULL, NULL, match, err_str);
         break;
 
     case MFF_METADATA:
@@ -1595,22 +1611,30 @@  mf_set_wild(const struct mf_field *mf, struct match *match)
  * call is equivalent to mf_set_wild(mf, match).
  *
  * 'mask' must be a valid mask for 'mf' (see mf_is_mask_valid()).  The caller
- * is responsible for ensuring that 'match' meets 'mf''s prerequisites. */
+ * is responsible for ensuring that 'match' meets 'mf''s prerequisites.
+ *
+ * If non-NULL, 'err_str' returns a malloc'ed string describing any errors
+ * with the request or NULL if there is no error. The caller is reponsible
+ * for freeing the string.*/
 enum ofputil_protocol
 mf_set(const struct mf_field *mf,
        const union mf_value *value, const union mf_value *mask,
-       struct match *match)
+       struct match *match, char **err_str)
 {
     if (!mask || is_all_ones(mask, mf->n_bytes)) {
-        mf_set_value(mf, value, match);
+        mf_set_value(mf, value, match, err_str);
         return mf->usable_protocols_exact;
     } else if (is_all_zeros(mask, mf->n_bytes) && !mf_is_tun_metadata(mf)) {
         /* Tunnel metadata matches on the existence of the field itself, so
          * it still needs to be encoded even if the value is wildcarded. */
-        mf_set_wild(mf, match);
+        mf_set_wild(mf, match, err_str);
         return OFPUTIL_P_ANY;
     }
 
+    if (err_str) {
+        *err_str = NULL;
+    }
+
     switch (mf->id) {
     case MFF_RECIRC_ID:
     case MFF_CONJ_ID:
@@ -1665,7 +1689,7 @@  mf_set(const struct mf_field *mf,
         match_set_tun_tos_masked(match, value->u8, mask->u8);
         break;
     CASE_MFF_TUN_METADATA:
-        tun_metadata_set_match(mf, value, mask, match);
+        tun_metadata_set_match(mf, value, mask, match, err_str);
         break;
 
     case MFF_METADATA:
@@ -1731,7 +1755,7 @@  mf_set(const struct mf_field *mf,
 
     case MFF_IPV6_LABEL:
         if ((mask->be32 & htonl(IPV6_LABEL_MASK)) == htonl(IPV6_LABEL_MASK)) {
-            mf_set_value(mf, value, match);
+            mf_set_value(mf, value, match, err_str);
         } else {
             match_set_ipv6_label_masked(match, value->be32, mask->be32);
         }
@@ -2316,7 +2340,7 @@  mf_write_subfield(const struct mf_subfield *sf, const union mf_subvalue *x,
     mf_get(field, match, &value, &mask);
     bitwise_copy(x, sizeof *x, 0, &value, field->n_bytes, sf->ofs, sf->n_bits);
     bitwise_one (                 &mask,  field->n_bytes, sf->ofs, sf->n_bits);
-    mf_set(field, &value, &mask, match);
+    mf_set(field, &value, &mask, match, NULL);
 }
 
 /* 'v' and 'm' correspond to values of 'field'.  This function copies them into
@@ -2332,7 +2356,7 @@  mf_mask_subfield(const struct mf_field *field,
     mf_get(field, match, &value, &mask);
     bitwise_copy(v, sizeof *v, 0, &value, field->n_bytes, 0, field->n_bits);
     bitwise_copy(m, sizeof *m, 0, &mask,  field->n_bytes, 0, field->n_bits);
-    mf_set(field, &value, &mask, match);
+    mf_set(field, &value, &mask, match, NULL);
 }
 
 /* Initializes 'x' to the value of 'sf' within 'flow'.  'sf' must be valid for
diff --git a/lib/meta-flow.h b/lib/meta-flow.h
index cfc6263..02272ef 100644
--- a/lib/meta-flow.h
+++ b/lib/meta-flow.h
@@ -1844,7 +1844,7 @@  bool mf_is_value_valid(const struct mf_field *, const union mf_value *value);
 void mf_get_value(const struct mf_field *, const struct flow *,
                   union mf_value *value);
 void mf_set_value(const struct mf_field *, const union mf_value *value,
-                  struct match *);
+                  struct match *, char **err_str);
 void mf_set_flow_value(const struct mf_field *, const union mf_value *value,
                        struct flow *);
 void mf_set_flow_value_masked(const struct mf_field *,
@@ -1864,9 +1864,9 @@  void mf_get(const struct mf_field *, const struct match *,
 enum ofputil_protocol mf_set(const struct mf_field *,
                              const union mf_value *value,
                              const union mf_value *mask,
-                             struct match *);
+                             struct match *, char **err_str);
 
-void mf_set_wild(const struct mf_field *, struct match *);
+void mf_set_wild(const struct mf_field *, struct match *, char **err_str);
 
 /* Subfields. */
 void mf_write_subfield_flow(const struct mf_subfield *,
diff --git a/lib/nx-match.c b/lib/nx-match.c
index 84283b3..eef2c54 100644
--- a/lib/nx-match.c
+++ b/lib/nx-match.c
@@ -511,7 +511,15 @@  nx_pull_raw(const uint8_t *p, unsigned int match_len, bool strict,
         } else if (!mf_is_all_wild(field, &match->wc)) {
             error = OFPERR_OFPBMC_DUP_FIELD;
         } else {
-            mf_set(field, &value, &mask, match);
+            char *err_str;
+
+            mf_set(field, &value, &mask, match, &err_str);
+            if (err_str) {
+                VLOG_DBG_RL(&rl, "error parsing OXM at offset %"PRIdPTR" "
+                           "within match (%s)", pos - p, err_str);
+                free(err_str);
+                return OFPERR_OFPBMC_BAD_VALUE;
+            }
         }
 
         if (error) {
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 855d732..e8b831d 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -229,7 +229,7 @@  parse_field(const struct mf_field *mf, const char *s, struct match *match,
 
     error = mf_parse(mf, s, &value, &mask);
     if (!error) {
-        *usable_protocols &= mf_set(mf, &value, &mask, match);
+        *usable_protocols &= mf_set(mf, &value, &mask, match, &error);
     }
     return error;
 }
diff --git a/lib/tun-metadata.c b/lib/tun-metadata.c
index 0a4090d..3c66eba 100644
--- a/lib/tun-metadata.c
+++ b/lib/tun-metadata.c
@@ -280,7 +280,8 @@  tun_metadata_write(struct flow_tnl *tnl,
 
 static const struct tun_metadata_loc *
 metadata_loc_from_match(struct tun_table *map, struct match *match,
-                        unsigned int idx, unsigned int field_len, bool masked)
+                        const char *name, unsigned int idx,
+                        unsigned int field_len, bool masked, char **err_str)
 {
     ovs_assert(idx < TUN_METADATA_NUM_OPTS);
 
@@ -292,8 +293,22 @@  metadata_loc_from_match(struct tun_table *map, struct match *match,
         }
     }
 
-    if (match->tun_md.alloc_offset + field_len >= TUN_METADATA_TOT_OPT_SIZE ||
-        ULLONG_GET(match->wc.masks.tunnel.metadata.present.map, idx)) {
+    if (match->tun_md.alloc_offset + field_len > TUN_METADATA_TOT_OPT_SIZE) {
+        if (err_str) {
+            *err_str = xasprintf("field %s exceeds maximum size for tunnel "
+                                 "metadata (used %d, max %d)", name,
+                                 match->tun_md.alloc_offset + field_len,
+                                 TUN_METADATA_TOT_OPT_SIZE);
+        }
+
+        return NULL;
+    }
+
+    if (ULLONG_GET(match->wc.masks.tunnel.metadata.present.map, idx)) {
+        if (err_str) {
+            *err_str = xasprintf("field %s set multiple times", name);
+        }
+
         return NULL;
     }
 
@@ -322,10 +337,15 @@  metadata_loc_from_match(struct tun_table *map, struct match *match,
  * value.
  *
  * 'mask' may be NULL; if so, then 'mf' is made exact-match.
+ *
+ * If non-NULL, 'err_str' returns a malloc'ed string describing any errors
+ * with the request or NULL if there is no error. The caller is reponsible
+ * for freeing the string.
  */
 void
 tun_metadata_set_match(const struct mf_field *mf, const union mf_value *value,
-                       const union mf_value *mask, struct match *match)
+                       const union mf_value *mask, struct match *match,
+                       char **err_str)
 {
     struct tun_table *map = ovsrcu_get(struct tun_table *, &metadata_tab);
     const struct tun_metadata_loc *loc;
@@ -338,7 +358,8 @@  tun_metadata_set_match(const struct mf_field *mf, const union mf_value *value,
     ovs_assert(!(match->flow.tunnel.flags & FLOW_TNL_F_UDPIF));
 
     field_len = mf_field_len(mf, value, mask, &is_masked);
-    loc = metadata_loc_from_match(map, match, idx, field_len, is_masked);
+    loc = metadata_loc_from_match(map, match, mf->name, idx, field_len,
+                                  is_masked, err_str);
     if (!loc) {
         return;
     }
@@ -424,8 +445,8 @@  tun_metadata_get_fmd(const struct flow_tnl *tnl, struct match *flow_metadata)
         const struct tun_metadata_loc *old_loc = &flow.metadata.tab->entries[i].loc;
         const struct tun_metadata_loc *new_loc;
 
-        new_loc = metadata_loc_from_match(NULL, flow_metadata, i,
-                                          old_loc->len, false);
+        new_loc = metadata_loc_from_match(NULL, flow_metadata, NULL, i,
+                                          old_loc->len, false, NULL);
 
         memcpy_from_metadata(opts.tun_metadata, &flow.metadata, old_loc);
         memcpy_to_metadata(&flow_metadata->flow.tunnel.metadata,
diff --git a/lib/tun-metadata.h b/lib/tun-metadata.h
index 85652ad..624c881 100644
--- a/lib/tun-metadata.h
+++ b/lib/tun-metadata.h
@@ -77,8 +77,8 @@  BUILD_ASSERT_DECL(sizeof(((struct tun_metadata *)0)->present.map) * 8 >=
  * linked list of these blocks. */
 struct tun_metadata_loc_chain {
     struct tun_metadata_loc_chain *next;
-    uint8_t offset;       /* In bytes, from start of 'opts', multiple of 4.  */
-    uint8_t len;          /* In bytes, multiple of 4. */
+    int offset;       /* In bytes, from start of 'opts', multiple of 4.  */
+    int len;          /* In bytes, multiple of 4. */
 };
 
 struct tun_metadata_loc {
@@ -102,7 +102,7 @@  struct tun_metadata_match_entry {
  * allocated memory because the address space is never fragmented. */
 struct tun_metadata_allocation {
     struct tun_metadata_match_entry entry[TUN_METADATA_NUM_OPTS];
-    uint8_t alloc_offset;       /* Byte offset into 'opts', multiple of 4.  */
+    int alloc_offset;           /* Byte offset into 'opts', multiple of 4.  */
     bool valid;                 /* Set to true after any allocation occurs. */
 };
 
@@ -117,7 +117,8 @@  void tun_metadata_write(struct flow_tnl *,
                         const struct mf_field *, const union mf_value *);
 void tun_metadata_set_match(const struct mf_field *,
                             const union mf_value *value,
-                            const union mf_value *mask, struct match *);
+                            const union mf_value *mask, struct match *,
+                            char **err_str);
 void tun_metadata_get_fmd(const struct flow_tnl *, struct match *flow_metadata);
 
 int tun_metadata_from_geneve_nlattr(const struct nlattr *attr,