diff mbox

[ovs-dev,2/3] ofp-parse: Allow ofctl flow monitor filtering on field existence.

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

Commit Message

Jesse Gross Sept. 1, 2015, 2:57 a.m. UTC 
It is supposed to be possible to allow ovs-ofctl to filter flows
it is monitoring based on a match string. However, the parser will
reject expressions that match only on a field's existence (such as
Geneve options). This relaxes the restriction to bring it in line
with matches supported by other commands.

Signed-off-by: Jesse Gross <jesse@nicira.com>
---
 lib/ofp-parse.c | 70 +++++++++++++++++++++++++++++----------------------------
 1 file changed, 36 insertions(+), 34 deletions(-)

Comments

Ben Pfaff Sept. 8, 2015, 8:54 p.m. UTC | #1 
On Mon, Aug 31, 2015 at 07:57:05PM -0700, Jesse Gross wrote:
> It is supposed to be possible to allow ovs-ofctl to filter flows
> it is monitoring based on a match string. However, the parser will
> reject expressions that match only on a field's existence (such as
> Geneve options). This relaxes the restriction to bring it in line
> with matches supported by other commands.
> 
> Signed-off-by: Jesse Gross <jesse@nicira.com>

Might be another possible place to use ofputil_parse_key_value().

Is this ability to match on the existence of a field documented
anywhere?  If not, it should be.

Acked-by: Ben Pfaff <blp@nicira.com>
Jesse Gross Sept. 9, 2015, 4:50 p.m. UTC | #2 
On Tue, Sep 8, 2015 at 1:54 PM, Ben Pfaff <blp@nicira.com> wrote:
> On Mon, Aug 31, 2015 at 07:57:05PM -0700, Jesse Gross wrote:
>> It is supposed to be possible to allow ovs-ofctl to filter flows
>> it is monitoring based on a match string. However, the parser will
>> reject expressions that match only on a field's existence (such as
>> Geneve options). This relaxes the restriction to bring it in line
>> with matches supported by other commands.
>>
>> Signed-off-by: Jesse Gross <jesse@nicira.com>
>
> Might be another possible place to use ofputil_parse_key_value().

I changed it here too (actually, I folded it into the previous patch).

> Is this ability to match on the existence of a field documented
> anywhere?  If not, it should be.

Yes, it was added to the ovs-ofctl man page when support was added:

       tun_metadataidx[=value[/mask]]
              Matches  value  either  exactly  or with optional mask in tunnel
              metadata field number idx (numbered from 0 to 63).  The  act  of
              specifying  a  field  implies  a  match on the existence of that
              field in the packet in addition to the masked value. As a short‐
              hand,  it  is  possible to specify only the field name to simply
              match on an option being present.
diff mbox

Patch

diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index a6190ed..855d732 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -221,6 +221,12 @@  parse_field(const struct mf_field *mf, const char *s, struct match *match,
     union mf_value value, mask;
     char *error;
 
+    if (!s) {
+        /* If there's no string, we're just trying to match on the
+         * existence of the field, so use a no-op value. */
+        s = "0/0";
+    }
+
     error = mf_parse(mf, s, &value, &mask);
     if (!error) {
         *usable_protocols &= mf_set(mf, &value, &mask, match);
@@ -372,11 +378,6 @@  parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string,
             value = field;
 
             if (mf_from_name(name)) {
-                if (!value) {
-                    /* If there's no value, we're just trying to match on the
-                     * existence of the field, so use a no-op value. */
-                     value = "0/0";
-                }
                 error = parse_field(mf_from_name(name), value, &fm->match,
                                     usable_protocols);
             } else {
@@ -774,7 +775,7 @@  parse_flow_monitor_request__(struct ofputil_flow_monitor_request *fmr,
 {
     static atomic_count id = ATOMIC_COUNT_INIT(0);
     char *save_ptr = NULL;
-    char *name;
+    char *field;
 
     fmr->id = atomic_count_inc(&id);
 
@@ -784,52 +785,53 @@  parse_flow_monitor_request__(struct ofputil_flow_monitor_request *fmr,
     fmr->table_id = 0xff;
     match_init_catchall(&fmr->match);
 
-    for (name = strtok_r(string, "=, \t\r\n", &save_ptr); name;
-         name = strtok_r(NULL, "=, \t\r\n", &save_ptr)) {
+    for (field = strtok_r(string, ", \t\r\n", &save_ptr); field;
+         field = strtok_r(NULL, ", \t\r\n", &save_ptr)) {
         const struct protocol *p;
 
-        if (!strcmp(name, "!initial")) {
+        if (!strcmp(field, "!initial")) {
             fmr->flags &= ~NXFMF_INITIAL;
-        } else if (!strcmp(name, "!add")) {
+        } else if (!strcmp(field, "!add")) {
             fmr->flags &= ~NXFMF_ADD;
-        } else if (!strcmp(name, "!delete")) {
+        } else if (!strcmp(field, "!delete")) {
             fmr->flags &= ~NXFMF_DELETE;
-        } else if (!strcmp(name, "!modify")) {
+        } else if (!strcmp(field, "!modify")) {
             fmr->flags &= ~NXFMF_MODIFY;
-        } else if (!strcmp(name, "!actions")) {
+        } else if (!strcmp(field, "!actions")) {
             fmr->flags &= ~NXFMF_ACTIONS;
-        } else if (!strcmp(name, "!own")) {
+        } else if (!strcmp(field, "!own")) {
             fmr->flags &= ~NXFMF_OWN;
-        } else if (parse_protocol(name, &p)) {
+        } else if (parse_protocol(field, &p)) {
             match_set_dl_type(&fmr->match, htons(p->dl_type));
             if (p->nw_proto) {
                 match_set_nw_proto(&fmr->match, p->nw_proto);
             }
         } else {
-            char *value;
-
-            value = strtok_r(NULL, ", \t\r\n", &save_ptr);
-            if (!value) {
-                return xasprintf("%s: field %s missing value", str_, name);
-            }
+            char *name, *value;
+            char *error = NULL;
 
-            if (!strcmp(name, "table")) {
-                char *error = str_to_u8(value, "table", &fmr->table_id);
-                if (error) {
-                    return error;
-                }
-            } else if (!strcmp(name, "out_port")) {
-                fmr->out_port = u16_to_ofp(atoi(value));
-            } else if (mf_from_name(name)) {
-                char *error;
+            name = strsep(&field, "=");
+            value = field;
 
+            if (mf_from_name(name)) {
                 error = parse_field(mf_from_name(name), value, &fmr->match,
                                     usable_protocols);
-                if (error) {
-                    return error;
-                }
             } else {
-                return xasprintf("%s: unknown keyword %s", str_, name);
+                if (!value) {
+                    return xasprintf("%s: field %s missing value", str_, name);
+                }
+
+                if (!strcmp(name, "table")) {
+                    error = str_to_u8(value, "table", &fmr->table_id);
+                } else if (!strcmp(name, "out_port")) {
+                    fmr->out_port = u16_to_ofp(atoi(value));
+                } else {
+                    return xasprintf("%s: unknown keyword %s", str_, name);
+                }
+            }
+
+            if (error) {
+                return error;
             }
         }
     }