[ovs-dev] [PATCH v3] ovs:group: support to insert bucket with weight value for select type

[solomon]

After creating a group with hash select type，then  we need to insert a new bucket with weight，
But，it fails. Commands are as following:

# ovs-ofctl  -O OpenFlow15 add-group br0 "group_id=10, type=select, selection_method=hash,fields=tcp_src, bucket=bucket_id=10,weight:99,actions=output:1, bucket=bucket_id=20,weight:199,actions=output:1 "

# ovs-ofctl -O OpenFlow15 insert-buckets br0 "group_id=10,type=select command_bucket_id=last,bucket=bucket_id=3,weight=100,actions=output:1"
ovs-ofctl: type is not needed

# ovs-ofctl -O OpenFlow15 insert-buckets br0 "group_id=10 command_bucket_id=last,bucket=bucket_id=3,weight=100,actions=output:1"
ovs-ofctl: Only select groups can have bucket weights.


This patch can help us. However, for other types that are not select, the check of the parameters
is not strict, but it does not affect their function, because other types do not use this weight parameter.


v2-->v3:
1. add testcase.

v1-->v2:
1. fix warning from 0-day robot.


Signed-off-by: solomon <liwei.solomon at gmail.com>
---
 lib/ofp-group.c  | 17 +++++++++++------
 tests/ofproto.at | 14 ++++++++++++++
 2 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/lib/ofp-group.c b/lib/ofp-group.c
index c9e95ad4c..d9eba735d 100644
--- a/lib/ofp-group.c
+++ b/lib/ofp-group.c
@@ -1043,7 +1043,8 @@ parse_ofp_group_mod_str__(struct ofputil_group_mod *gm, int command,
         }
         ovs_list_push_back(&gm->buckets, &bucket->list_node);
 
-        if (gm->type != OFPGT11_SELECT && bucket->weight) {
+        if (gm->command != OFPGC15_INSERT_BUCKET
+            && gm->type != OFPGT11_SELECT && bucket->weight) {
             error = xstrdup("Only select groups can have bucket weights.");
             goto out;
         }
@@ -1176,7 +1177,8 @@ ofputil_put_ofp11_bucket(const struct ofputil_bucket *bucket,
 static void
 ofputil_put_ofp15_bucket(const struct ofputil_bucket *bucket,
                          uint32_t bucket_id, enum ofp11_group_type group_type,
-                         struct ofpbuf *openflow, enum ofp_version ofp_version)
+                         struct ofpbuf *openflow, enum ofp_version ofp_version,
+                         int group_command)
 {
     struct ofp15_bucket *ob;
     size_t start, actions_start, actions_len;
@@ -1189,7 +1191,8 @@ ofputil_put_ofp15_bucket(const struct ofputil_bucket *bucket,
                                  openflow, ofp_version);
     actions_len = openflow->size - actions_start;
 
-    if (group_type == OFPGT11_SELECT) {
+    if (group_type == OFPGT11_SELECT
+        || group_command == OFPGC15_INSERT_BUCKET) {
         ofpprop_put_u16(openflow, OFPGBPT15_WEIGHT, bucket->weight);
     }
     if (bucket->watch_port != OFPP_ANY) {
@@ -1266,7 +1269,7 @@ ofputil_append_ofp15_group_desc_reply(const struct ofputil_group_desc *gds,
     start_buckets = reply->size;
     LIST_FOR_EACH (bucket, list_node, buckets) {
         ofputil_put_ofp15_bucket(bucket, bucket->bucket_id,
-                                 gds->type, reply, version);
+                                 gds->type, reply, version, -1);
     }
     ogds = ofpbuf_at_assert(reply, start_ogds, sizeof *ogds);
     ogds->type = gds->type;
@@ -2066,7 +2069,8 @@ ofputil_encode_ofp15_group_mod(enum ofp_version ofp_version,
             bucket_id = bucket->bucket_id;
         }
 
-        ofputil_put_ofp15_bucket(bucket, bucket_id, gm->type, b, ofp_version);
+        ofputil_put_ofp15_bucket(bucket, bucket_id, gm->type, b, ofp_version,
+                                 gm->command);
     }
     ogm = ofpbuf_at_assert(b, start_ogm, sizeof *ogm);
     ogm->command = htons(gm->command != OFPGC11_ADD_OR_MOD || group_existed < 0
@@ -2251,7 +2255,8 @@ ofputil_check_group_mod(const struct ofputil_group_mod *gm)
 
     struct ofputil_bucket *bucket;
     LIST_FOR_EACH (bucket, list_node, &gm->buckets) {
-        if (bucket->weight && gm->type != OFPGT11_SELECT) {
+        if (bucket->weight && gm->type != OFPGT11_SELECT
+            && gm->command != OFPGC15_INSERT_BUCKET) {
             return OFPERR_OFPGMFC_INVALID_GROUP;
         }
 
diff --git a/tests/ofproto.at b/tests/ofproto.at
index 9819bc577..7a6598010 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -678,6 +678,20 @@ AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn insert-buckets br0 group_id=1234,comman
 AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn insert-buckets br0 group_id=1234,command_bucket_id=first,bucket=bucket_id:0,actions=output:0,bucket=bucket_id:1,actions=output:1], [1], [],
   [ovs-ofctl: insert-bucket needs OpenFlow 1.5 or later ('-O OpenFlow15')
 ])
+
+#Verify insert-buckets command to insert bucket with weight value for select group.
+AT_CHECK([ovs-ofctl -O OpenFlow15 --strict del-groups br0 group_id=1234])
+AT_DATA([groups.txt], [dnl
+group_id=1234,type=select,selection_method=hash,bucket=bucket_id=1,weight:100,output:11
+])
+AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn add-groups br0 groups.txt])
+AT_CHECK([ovs-ofctl -O OpenFlow15 insert-buckets br0 group_id=1234,command_bucket_id=last,bucket=bucket_id=2,weight=100,actions=output:11])
+AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn dump-groups br0], [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:1,weight:100,actions=output:11,bucket=bucket_id:2,weigh    t:100,actions=output:11
+])
+
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
-- 
2.11.0