ofproto: Keep inserting buckets into a group from changing group type.

The "insert buckets" and "delete buckets" operations on a group should not
change the group's type or properties, but the implementation did this by
mistake.  This fixes the problem.

Reported-by: shivani dommeti <shivani.dommeti@gmail.com>
Tested-by: shivani dommeti <shivani.dommeti@gmail.com>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2017-December/045830.html
Signed-off-by: Ben Pfaff <blp@ovn.org>

diff --git a/AUTHORS.rst b/AUTHORS.rst
index 446a4392587819747ee12b1a7944cf61d1327eb6..e17daf078eb6f13d567f7c74f8d24a82b3b33bde 100644 (file)
--- a/AUTHORS.rst
+++ b/AUTHORS.rst
@@ -589,6 +589,7 @@ likunyun                        kunyunli@hotmail.com
 meishengxin                     meishengxin@huawei.com
 neeraj mehta                    mehtaneeraj07@gmail.com
 rahim entezari                  rahim.entezari@gmail.com
+shivani dommeti                 shivani.dommeti@gmail.com
 weizj                           34965317@qq.com
 俊 赵                           zhaojun12@outlook.com
 冯全树(Crab)                    fqs888@126.com

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 82c2bb27d3484442b92c122845da9175ccf1a67b..84eb18e901edd6a519c4c506cddcbfd0859b2248 100644 (file)
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -6905,6 +6905,13 @@ handle_queue_get_config_request(struct ofconn *ofconn,
     return error;
 }
 
+/* Allocates, initializes, and constructs a new group in 'ofproto', obtaining
+ * all the attributes for it from 'gm', and stores a pointer to it in
+ * '*ofgroup'.  Makes the new group visible from the flow table starting from
+ * 'version'.
+ *
+ * Returns 0 if successful, otherwise an error code.  If there is an error then
+ * '*ofgroup' is indeterminate upon return. */
 static enum ofperr
 init_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm,
            ovs_version_t version, struct ofgroup **ofgroup)
@@ -7113,6 +7120,16 @@ modify_group_start(struct ofproto *ofproto, struct ofproto_group_mod *ogm)
         return OFPERR_OFPGMFC_UNKNOWN_GROUP;
     }
 
+    /* Inserting or deleting a bucket should not change the group's type or
+     * properties, so change the group mod so that these aspects match the old
+     * group.  (See EXT-570.)  */
+    if (ogm->gm.command == OFPGC15_INSERT_BUCKET ||
+        ogm->gm.command == OFPGC15_REMOVE_BUCKET) {
+        ogm->gm.type = old_group->type;
+        ofputil_group_properties_destroy(&ogm->gm.props);
+        ofputil_group_properties_copy(&ogm->gm.props, &old_group->props);
+    }
+
     if (old_group->type != ogm->gm.type
         && (ofproto->n_groups[ogm->gm.type]
             >= ofproto->ogf.max_groups[ogm->gm.type])) {

diff --git a/tests/ofproto.at b/tests/ofproto.at
index c19a3d10daed8ea4b1ccd05aa1b7379d69c6b778..74a30cbefe3bf063e727527b6847f6d836e99dbc 100644 (file)
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -621,7 +621,32 @@ OFPST_GROUP_DESC reply (OF1.5):
  group_id=1234,type=all,bucket=bucket_id:0,actions=output:0,bucket=bucket_id:1,actions=output:1,bucket=bucket_id:10,actions=output:10,bucket=bucket_id:11,actions=output:11,bucket=bucket_id:12,actions=output:12,bucket=bucket_id:13,actions=output:13,bucket=bucket_id:14,actions=output:14,bucket=bucket_id:15,actions=output:15,bucket=bucket_id:20,actions=output:20,bucket=bucket_id:21,actions=output:21
 ])
 
+# Delete groups.
+AT_CHECK([ovs-ofctl -O OpenFlow15 del-groups br0])
+
+# Add "fast_failover" group, then insert a bucket into it and make
+# sure that the type of the group doesn't change.  (There was a bug
+# that caused this to happen.)
+AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn add-group br0 group_id=1234,type=ff])
+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=ff
+])
+AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn insert-buckets br0 group_id=1234,command_bucket_id=first,bucket=bucket_id:20,actions=output:20,bucket=bucket_id:21,actions=output:21])
+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=ff,bucket=bucket_id:20,actions=output:20,bucket=bucket_id:21,actions=output:21
+])
+
 # Negative tests.
+AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn insert-buckets br0 group_id=1234,command_bucket_id=123,type=indirect,bucket=bucket_id:0,actions=output:0,bucket=bucket_id:1,actions=output:1], [1], [],
+  [ovs-ofctl: type is not needed
+])
+AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn insert-buckets br0 group_id=1234,command_bucket_id=123,selection_method=dp_hash,type=indirect,bucket=bucket_id:0,actions=output:0,bucket=bucket_id:1,actions=output:1], [1], [],
+  [ovs-ofctl: selection method is not needed
+])
 AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn insert-buckets br0 group_id=1234,command_bucket_id=0xffffff01,bucket=bucket_id:0,actions=output:0,bucket=bucket_id:1,actions=output:1], [1], [],
   [ovs-ofctl: invalid command bucket id 4294967041
 ])