From 68dfc25b62e5edc2939bcae791a35fddfecb5d20 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Thu, 15 Oct 2015 09:46:21 -0700 Subject: [PATCH] ofp-parse: Fix parsing, formatting of multiple fields in NTR extension. Until now, the only way to specify multiple fields in the "fields" parameter for the Netronome groups extension, was to specify "fields" more than once, e.g. fields=eth_dst,fields=ip_dst However, this wasn't documented and the code in ofp-print didn't use it, generating output that couldn't be parsed. This commit fixes the situation by introducing a more straightforward syntax, e.g. fields(eth_dst,ip_dst), documents it, and adjusts ofp-print code to use it when there is more than one field (it retains the previous format for backward compatibility when there is exactly one field) Signed-off-by: Ben Pfaff Acked-by: Simon Horman --- lib/ofp-parse.c | 25 ++++++------------------- lib/ofp-print.c | 22 ++++++++++------------ tests/ofp-print.at | 2 +- tests/ofproto.at | 4 ++-- utilities/ovs-ofctl.8.in | 6 +++++- 5 files changed, 24 insertions(+), 35 deletions(-) diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c index 843765650..214cc36a8 100644 --- a/lib/ofp-parse.c +++ b/lib/ofp-parse.c @@ -1228,24 +1228,20 @@ static char * OVS_WARN_UNUSED_RESULT parse_select_group_field(char *s, struct field_array *fa, enum ofputil_protocol *usable_protocols) { - char *save_ptr = NULL; - char *name; + char *name, *value_str; - for (name = strtok_r(s, "=, \t\r\n", &save_ptr); name; - name = strtok_r(NULL, "=, \t\r\n", &save_ptr)) { + while (ofputil_parse_key_value(&s, &name, &value_str)) { const struct mf_field *mf = mf_from_name(name); if (mf) { char *error; - const char *value_str; union mf_value value; if (bitmap_is_set(fa->used.bm, mf->id)) { return xasprintf("%s: duplicate field", name); } - value_str = strtok_r(NULL, ", \t\r\n", &save_ptr); - if (value_str) { + if (*value_str) { error = mf_parse_value(mf, value_str, &value); if (error) { return error; @@ -1297,10 +1293,8 @@ parse_ofp_group_mod_str__(struct ofputil_group_mod *gm, uint16_t command, F_COMMAND_BUCKET_ID = 1 << 2, F_COMMAND_BUCKET_ID_ALL = 1 << 3, } fields; - char *save_ptr = NULL; bool had_type = false; bool had_command_bucket_id = false; - char *name; struct ofputil_bucket *bucket; char *error = NULL; @@ -1358,16 +1352,9 @@ parse_ofp_group_mod_str__(struct ofputil_group_mod *gm, uint16_t command, } /* Parse everything before the buckets. */ - for (name = strtok_r(string, "=, \t\r\n", &save_ptr); name; - name = strtok_r(NULL, "=, \t\r\n", &save_ptr)) { - char *value; - - value = strtok_r(NULL, ", \t\r\n", &save_ptr); - if (!value) { - error = xasprintf("field %s missing value", name); - goto out; - } - + char *pos = string; + char *name, *value; + while (ofputil_parse_key_value(&pos, &name, &value)) { if (!strcmp(name, "command_bucket_id")) { if (!(fields & F_COMMAND_BUCKET_ID)) { error = xstrdup("command bucket id is not needed"); diff --git a/lib/ofp-print.c b/lib/ofp-print.c index d0c94cec3..240ba8452 100644 --- a/lib/ofp-print.c +++ b/lib/ofp-print.c @@ -2352,22 +2352,20 @@ ofp_print_group(struct ds *s, uint32_t group_id, uint8_t type, } if (props->selection_method[0]) { - size_t mark, start; - - ds_put_format(s, ",selection_method=%s,", props->selection_method); + ds_put_format(s, ",selection_method=%s", props->selection_method); if (props->selection_method_param) { - ds_put_format(s, "selection_method_param=%"PRIu64",", + ds_put_format(s, ",selection_method_param=%"PRIu64, props->selection_method_param); } - /* Allow rewinding to immediately before the trailing ',' */ - mark = s->length - 1; - - ds_put_cstr(s, "fields="); - start = s->length; - oxm_format_field_array(s, &props->fields); - if (s->length == start) { - ds_truncate(s, mark); + size_t n = bitmap_count1(props->fields.used.bm, MFF_N_IDS); + if (n == 1) { + ds_put_cstr(s, ",fields="); + oxm_format_field_array(s, &props->fields); + } else if (n > 1) { + ds_put_cstr(s, ",fields("); + oxm_format_field_array(s, &props->fields); + ds_put_char(s, ')'); } } diff --git a/tests/ofp-print.at b/tests/ofp-print.at index 25cf950c6..fce7671ff 100644 --- a/tests/ofp-print.at +++ b/tests/ofp-print.at @@ -2043,7 +2043,7 @@ ff ff 00 3b 00 00 15 40 00 00 00 01 00 00 00 00 \ 14 01 ff 00 00 00 00 00 \ "], [0], [dnl OFPST_GROUP_DESC reply (OF1.5) (xid=0x2): - group_id=8192,type=select,selection_method=hash,fields=ip_dst=255.255.255.0,nw_proto,tcp_src,bucket=bucket_id:0,weight:100,watch_port:1,actions=output:1,bucket=bucket_id:1,weight:200,watch_port:2,actions=output:2,bucket=bucket_id:2,weight:200,watch_port:3,actions=output:3 + group_id=8192,type=select,selection_method=hash,fields(ip_dst=255.255.255.0,nw_proto,tcp_src),bucket=bucket_id:0,weight:100,watch_port:1,actions=output:1,bucket=bucket_id:1,weight:200,watch_port:2,actions=output:2,bucket=bucket_id:2,weight:200,watch_port:3,actions=output:3 ]) AT_CLEANUP diff --git a/tests/ofproto.at b/tests/ofproto.at index 732c1a93f..634735b86 100644 --- a/tests/ofproto.at +++ b/tests/ofproto.at @@ -344,14 +344,14 @@ AT_SETUP([ofproto - del group (OpenFlow 1.5)]) OVS_VSWITCHD_START AT_DATA([groups.txt], [dnl group_id=1233,type=select,selection_method=hash,bucket=output:10,bucket=output:11 -group_id=1234,type=select,selection_method=hash,bucket=output:10,bucket=output:11 +group_id=1234,type=select,selection_method=hash,fields(eth_dst,ip_dst,tcp_dst),bucket=output:10,bucket=output:11 group_id=1235,type=all,bucket=actions=output:12,bucket=bucket_id:0,actions=output:10,bucket=bucket_id:1,actions=output:11 ]) AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn add-groups br0 groups.txt]) AT_CHECK([ovs-ofctl -F OXM-OpenFlow15 -O OpenFlow15 -vwarn dump-groups br0 1234], [0], [stdout]) AT_CHECK([STRIP_XIDS stdout], [0], [dnl OFPST_GROUP_DESC reply (OF1.5): - group_id=1234,type=select,selection_method=hash,bucket=bucket_id:0,actions=output:10,bucket=bucket_id:1,actions=output:11 + group_id=1234,type=select,selection_method=hash,fields(eth_dst,ip_dst,tcp_dst),bucket=bucket_id:0,actions=output:10,bucket=bucket_id:1,actions=output:11 ]) AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn del-groups br0 group_id=1234]) AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn dump-groups br0], [0], [stdout]) diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in index a6087f611..dc81e7edf 100644 --- a/utilities/ovs-ofctl.8.in +++ b/utilities/ovs-ofctl.8.in @@ -2596,6 +2596,9 @@ This field is optional for \fBadd\-group\fR, \fBadd\-groups\fR and \fBmod\-group\fR commands on groups of type \fBselect\fR. Prohibited otherwise. The default value is the empty string. .IP +Other than the empty string, \fBhash\fR is currently the only defined +selection method. +.IP This option will use a Netronome OpenFlow extension which is only supported when using Open vSwitch 2.4 and later with OpenFlow 1.5 and later. @@ -2609,7 +2612,8 @@ Prohibited otherwise. The default value is zero. This option will use a Netronome OpenFlow extension which is only supported when using Open vSwitch 2.4 and later with OpenFlow 1.5 and later. -.IP \fBfields\fR=\fIparam\fR +.IP \fBfields\fR=\fIfield\fR +.IQ \fBfields(\fIfield\fR[\fB=\fImask\fR]\fR...\fB)\fR The field parameters to selection method selected by the \fBselection_method\fR field. The syntax is described in \fBFlow Syntax\fR with the additional restrictions that if a value is provided it is -- 2.20.1