[ovs-dev] ofproto: Handle OpenFlow version mismatch for requestforward with groups.

diff --git a/include/openvswitch/ofp-group.h b/include/openvswitch/ofp-group.h
index 10790eb020bd..cd7af0ebff9c 100644
--- a/include/openvswitch/ofp-group.h
+++ b/include/openvswitch/ofp-group.h
@@ -103,7 +103,9 @@  struct ofputil_group_mod {
 
 void ofputil_uninit_group_mod(struct ofputil_group_mod *gm);
 struct ofpbuf *ofputil_encode_group_mod(enum ofp_version ofp_version,
-                                        const struct ofputil_group_mod *gm);
+                                        const struct ofputil_group_mod *gm,
+                                        const struct ovs_list *new_buckets,
+                                        int group_existed);
 
 enum ofperr ofputil_decode_group_mod(const struct ofp_header *,
                                      struct ofputil_group_mod *);
diff --git a/include/openvswitch/ofp-monitor.h b/include/openvswitch/ofp-monitor.h
index 3edaeb53a16c..1bfcf92386c1 100644
--- a/include/openvswitch/ofp-monitor.h
+++ b/include/openvswitch/ofp-monitor.h
@@ -125,7 +125,20 @@  struct ofputil_requestforward {
         };
 
         /* reason == OFPRFR_GROUP_MOD. */
-        struct ofputil_group_mod *group_mod;
+        struct {
+            struct ofputil_group_mod *group_mod;
+
+            /* If nonnull, points to the full set of new buckets that resulted
+             * from a OFPGC15_INSERT_BUCKET or OFPGC15_REMOVE_BUCKET command.
+             * Needed to translate such group_mods into OpenFlow 1.1-1.4
+             * OFPGC11_MODIFY. */
+            const struct ovs_list *new_buckets;
+
+            /* If nonnegative, specifies whether the group existed before the
+             * command was executed.  Needed to translate OVS's nonstandard
+             * OFPGC11_ADD_OR_MOD into a standard command. */
+            int group_existed;
+        };
     };
 };
 
diff --git a/lib/ofp-bundle.c b/lib/ofp-bundle.c
index 0e9b46b2f224..1f65fe54cacf 100644
--- a/lib/ofp-bundle.c
+++ b/lib/ofp-bundle.c
@@ -65,7 +65,7 @@  ofputil_encode_bundle_msgs(const struct ofputil_bundle_msg *bms,
             request = ofputil_encode_flow_mod(&bms[i].fm, protocol);
             break;
         case OFPTYPE_GROUP_MOD:
-            request = ofputil_encode_group_mod(version, &bms[i].gm);
+            request = ofputil_encode_group_mod(version, &bms[i].gm, NULL, -1);
             break;
         case OFPTYPE_PACKET_OUT:
             request = ofputil_encode_packet_out(&bms[i].po, protocol);
diff --git a/lib/ofp-group.c b/lib/ofp-group.c
index 62cab725efb5..c9e95ad4ca26 100644
--- a/lib/ofp-group.c
+++ b/lib/ofp-group.c
@@ -1916,9 +1916,68 @@  ofputil_uninit_group_mod(struct ofputil_group_mod *gm)
     ofputil_group_properties_destroy(&gm->props);
 }
 
+static void
+bad_group_cmd(enum ofp15_group_mod_command cmd)
+{
+    const char *opt_version;
+    const char *version;
+    const char *cmd_str;
+
+    switch (cmd) {
+    case OFPGC15_ADD:
+    case OFPGC15_MODIFY:
+    case OFPGC15_ADD_OR_MOD:
+    case OFPGC15_DELETE:
+        version = "1.1";
+        opt_version = "11";
+        break;
+
+    case OFPGC15_INSERT_BUCKET:
+    case OFPGC15_REMOVE_BUCKET:
+        version = "1.5";
+        opt_version = "15";
+        break;
+
+    default:
+        OVS_NOT_REACHED();
+    }
+
+    switch (cmd) {
+    case OFPGC15_ADD:
+        cmd_str = "add-group";
+        break;
+
+    case OFPGC15_MODIFY:
+    case OFPGC15_ADD_OR_MOD:
+        cmd_str = "mod-group";
+        break;
+
+    case OFPGC15_DELETE:
+        cmd_str = "del-group";
+        break;
+
+    case OFPGC15_INSERT_BUCKET:
+        cmd_str = "insert-bucket";
+        break;
+
+    case OFPGC15_REMOVE_BUCKET:
+        cmd_str = "remove-bucket";
+        break;
+
+    default:
+        OVS_NOT_REACHED();
+    }
+
+    ovs_fatal(0, "%s needs OpenFlow %s or later (\'-O OpenFlow%s\')",
+              cmd_str, version, opt_version);
+
+}
+
 static struct ofpbuf *
 ofputil_encode_ofp11_group_mod(enum ofp_version ofp_version,
-                               const struct ofputil_group_mod *gm)
+                               const struct ofputil_group_mod *gm,
+                               const struct ovs_list *new_buckets,
+                               int group_existed)
 {
     struct ofpbuf *b;
     struct ofp11_group_mod *ogm;
@@ -1929,11 +1988,32 @@  ofputil_encode_ofp11_group_mod(enum ofp_version ofp_version,
     start_ogm = b->size;
     ofpbuf_put_zeros(b, sizeof *ogm);
 
-    LIST_FOR_EACH (bucket, list_node, &gm->buckets) {
+    uint16_t command = gm->command;
+    const struct ovs_list *buckets = &gm->buckets;
+    switch (gm->command) {
+    case OFPGC15_INSERT_BUCKET:
+    case OFPGC15_REMOVE_BUCKET:
+        if (!new_buckets) {
+            bad_group_cmd(gm->command);
+        }
+        command = OFPGC11_MODIFY;
+        buckets = new_buckets;
+        break;
+
+    case OFPGC11_ADD_OR_MOD:
+        if (group_existed >= 0) {
+            command = group_existed ? OFPGC11_MODIFY : OFPGC11_ADD;
+        }
+        break;
+
+    default:
+        break;
+    }
+    LIST_FOR_EACH (bucket, list_node, buckets) {
         ofputil_put_ofp11_bucket(bucket, b, ofp_version);
     }
     ogm = ofpbuf_at_assert(b, start_ogm, sizeof *ogm);
-    ogm->command = htons(gm->command);
+    ogm->command = htons(command);
     ogm->type = gm->type;
     ogm->group_id = htonl(gm->group_id);
 
@@ -1942,7 +2022,8 @@  ofputil_encode_ofp11_group_mod(enum ofp_version ofp_version,
 
 static struct ofpbuf *
 ofputil_encode_ofp15_group_mod(enum ofp_version ofp_version,
-                               const struct ofputil_group_mod *gm)
+                               const struct ofputil_group_mod *gm,
+                               int group_existed)
 {
     struct ofpbuf *b;
     struct ofp15_group_mod *ogm;
@@ -1988,7 +2069,9 @@  ofputil_encode_ofp15_group_mod(enum ofp_version ofp_version,
         ofputil_put_ofp15_bucket(bucket, bucket_id, gm->type, b, ofp_version);
     }
     ogm = ofpbuf_at_assert(b, start_ogm, sizeof *ogm);
-    ogm->command = htons(gm->command);
+    ogm->command = htons(gm->command != OFPGC11_ADD_OR_MOD || group_existed < 0
+                         ? gm->command
+                         : group_existed ? OFPGC11_MODIFY : OFPGC11_ADD);
     ogm->type = gm->type;
     ogm->group_id = htonl(gm->group_id);
     ogm->command_bucket_id = htonl(gm->command_bucket_id);
@@ -2003,68 +2086,24 @@  ofputil_encode_ofp15_group_mod(enum ofp_version ofp_version,
     return b;
 }
 
-static void
-bad_group_cmd(enum ofp15_group_mod_command cmd)
-{
-    const char *opt_version;
-    const char *version;
-    const char *cmd_str;
-
-    switch (cmd) {
-    case OFPGC15_ADD:
-    case OFPGC15_MODIFY:
-    case OFPGC15_ADD_OR_MOD:
-    case OFPGC15_DELETE:
-        version = "1.1";
-        opt_version = "11";
-        break;
-
-    case OFPGC15_INSERT_BUCKET:
-    case OFPGC15_REMOVE_BUCKET:
-        version = "1.5";
-        opt_version = "15";
-        break;
-
-    default:
-        OVS_NOT_REACHED();
-    }
-
-    switch (cmd) {
-    case OFPGC15_ADD:
-        cmd_str = "add-group";
-        break;
-
-    case OFPGC15_MODIFY:
-    case OFPGC15_ADD_OR_MOD:
-        cmd_str = "mod-group";
-        break;
-
-    case OFPGC15_DELETE:
-        cmd_str = "del-group";
-        break;
-
-    case OFPGC15_INSERT_BUCKET:
-        cmd_str = "insert-bucket";
-        break;
-
-    case OFPGC15_REMOVE_BUCKET:
-        cmd_str = "remove-bucket";
-        break;
-
-    default:
-        OVS_NOT_REACHED();
-    }
-
-    ovs_fatal(0, "%s needs OpenFlow %s or later (\'-O OpenFlow%s\')",
-              cmd_str, version, opt_version);
-
-}
-
 /* Converts abstract group mod 'gm' into a message for OpenFlow version
- * 'ofp_version' and returns the message. */
+ * 'ofp_version' and returns the message.
+ *
+ * If 'new_buckets' is nonnull, it should point to the full set of new buckets
+ * that resulted from a OFPGC15_INSERT_BUCKET or OFPGC15_REMOVE_BUCKET command.
+ * It is needed to translate such group_mods into OpenFlow 1.1-1.4
+ * OFPGC11_MODIFY.  If it is null but needed for translation, then encoding the
+ * group_mod will print an error on stderr and exit().
+ *
+ * If 'group_existed' is nonnegative, it should specify whether the group
+ * existed before the command was executed.  If it is nonnegative, then it is
+ * used to translate OVS's nonstandard OFPGC11_ADD_OR_MOD into a standard
+ * command.  If it is negative, then OFPGC11_ADD_OR_MOD will be left as is. */
 struct ofpbuf *
 ofputil_encode_group_mod(enum ofp_version ofp_version,
-                         const struct ofputil_group_mod *gm)
+                         const struct ofputil_group_mod *gm,
+                         const struct ovs_list *new_buckets,
+                         int group_existed)
 {
 
     switch (ofp_version) {
@@ -2072,15 +2111,13 @@  ofputil_encode_group_mod(enum ofp_version ofp_version,
     case OFP12_VERSION:
     case OFP13_VERSION:
     case OFP14_VERSION:
-        if (gm->command > OFPGC11_DELETE && gm->command != OFPGC11_ADD_OR_MOD) {
-            bad_group_cmd(gm->command);
-        }
-        return ofputil_encode_ofp11_group_mod(ofp_version, gm);
+        return ofputil_encode_ofp11_group_mod(ofp_version, gm,
+                                              new_buckets, group_existed);
 
     case OFP10_VERSION:
     case OFP15_VERSION:
     case OFP16_VERSION:
-        return ofputil_encode_ofp15_group_mod(ofp_version, gm);
+        return ofputil_encode_ofp15_group_mod(ofp_version, gm, group_existed);
 
     default:
         OVS_NOT_REACHED();
diff --git a/lib/ofp-monitor.c b/lib/ofp-monitor.c
index 3e02834a26a0..d1853d928b50 100644
--- a/lib/ofp-monitor.c
+++ b/lib/ofp-monitor.c
@@ -796,7 +796,8 @@  ofputil_encode_requestforward(const struct ofputil_requestforward *rf,
 
     switch (rf->reason) {
     case OFPRFR_GROUP_MOD:
-        inner = ofputil_encode_group_mod(ofp_version, rf->group_mod);
+        inner = ofputil_encode_group_mod(ofp_version, rf->group_mod,
+                                         rf->new_buckets, rf->group_existed);
         break;
 
     case OFPRFR_METER_MOD:
@@ -829,6 +830,9 @@  enum ofperr
 ofputil_decode_requestforward(const struct ofp_header *outer,
                               struct ofputil_requestforward *rf)
 {
+    rf->new_buckets = NULL;
+    rf->group_existed = -1;
+
     struct ofpbuf b = ofpbuf_const_initializer(outer, ntohs(outer->length));
 
     /* Skip past outer message. */
@@ -920,6 +924,7 @@  ofputil_destroy_requestforward(struct ofputil_requestforward *rf)
     case OFPRFR_GROUP_MOD:
         ofputil_uninit_group_mod(rf->group_mod);
         free(rf->group_mod);
+        /* 'rf' does not own rf->new_buckets. */
         break;
 
     case OFPRFR_METER_MOD:
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 9552a585d096..a8cc4751f8c9 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -7393,10 +7393,13 @@  ofproto_group_mod_finish(struct ofproto *ofproto,
     remove_groups_postponed(&ogm->old_groups);
 
     if (req) {
-        struct ofputil_requestforward rf;
-        rf.xid = req->request->xid;
-        rf.reason = OFPRFR_GROUP_MOD;
-        rf.group_mod = &ogm->gm;
+        struct ofputil_requestforward rf = {
+            .xid = req->request->xid,
+            .reason = OFPRFR_GROUP_MOD,
+            .group_mod = &ogm->gm,
+            .new_buckets = new_group ? &new_group->buckets : NULL,
+            .group_existed = group_collection_n(&ogm->old_groups) > 0,
+        };
         connmgr_send_requestforward(ofproto->connmgr, req->ofconn, &rf);
     }
 }
diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index 96c57f143843..f5b53195d984 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -777,7 +777,7 @@  add_flow_mod(struct ofputil_flow_mod *fm, struct ovs_list *msgs)
 static struct ofpbuf *
 encode_group_mod(const struct ofputil_group_mod *gm)
 {
-    return ofputil_encode_group_mod(OFP13_VERSION, gm);
+    return ofputil_encode_group_mod(OFP13_VERSION, gm, NULL, -1);
 }
 
 static void
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index c018bd48fcfb..82bc0cad6861 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -2956,7 +2956,8 @@  bundle_group_mod__(const char *remote, struct ofputil_group_mod *gms,
 
     for (i = 0; i < n_gms; i++) {
         struct ofputil_group_mod *gm = &gms[i];
-        struct ofpbuf *request = ofputil_encode_group_mod(version, gm);
+        struct ofpbuf *request = ofputil_encode_group_mod(version, gm,
+                                                          NULL, -1);
 
         ovs_list_push_back(&requests, &request->list_node);
         ofputil_uninit_group_mod(gm);
@@ -2989,7 +2990,7 @@  ofctl_group_mod__(const char *remote, struct ofputil_group_mod *gms,
 
     for (i = 0; i < n_gms; i++) {
         gm = &gms[i];
-        request = ofputil_encode_group_mod(version, gm);
+        request = ofputil_encode_group_mod(version, gm, NULL, -1);
         transact_noreply(vconn, request);
         ofputil_uninit_group_mod(gm);
     }