[ovs-dev,v2] : ofproto: Add relaxed group_mod command ADD_OR_MOD

This patch adds support for a new Group Mod command OFPGC_ADD_OR_MOD to OVS
for all OpenFlow versions that support groups (OF11 and higher). The new
ADD_OR_MOD creates a group that does not yet exist (like ADD) and modifies
an existing group (like MODIFY).

Rational: In OpenFlow 1.x the Group Mod commands OFPGC_ADD and OFPGC_MODIFY
have strict semantics: ADD fails if the group exists, while MODIFY fails if
the group does not exist. This requires a controller to exactly know the
state of the switch when programming a group in order not run the risk of
getting an OFP Error message in response. This is hard to achieve and
maintain at all times in view of possible switch and controller restarts or
other connection losses between switch and controller.

Due to the un-acknowledged nature of the Group Mod message programming groups
safely and efficiently at the same time is virtually impossible as the
controller has to either query the existence of the group prior to each Group
Mod message or to insert a Barrier Request/Reply after every group to be sure
that no Error can be received at a later stage and require a complicated
roll-back of any dependent actions taken between the failed Group Mod and the
Error.

In the ovs-ofctl command line the ADD_OR_MOD command is made available through
the new option --may-create in the mod-group command:

$ ovs-ofctl -Oopenflow13 del-groups br-int group_id=100

$ ovs-ofctl -Oopenflow13 mod-group br-int group_id=100,type=indirect,bucket=actions=2
OFPT_ERROR (OF1.3) (xid=0x2): OFPGMFC_UNKNOWN_GROUP
OFPT_GROUP_MOD (OF1.3) (xid=0x2):
 MOD group_id=100,type=indirect,bucket=actions=output:2

$ ovs-ofctl -Oopenflow13 --may-create mod-group br-int group_id=100,type=indirect,bucket=actions=2

$ ovs-ofctl -Oopenflow13 dump-groups br-int
OFPST_GROUP_DESC reply (OF1.3) (xid=0x2):
 group_id=100,type=indirect,bucket=actions=output:2

$ ovs-ofctl -Oopenflow13 --may-create mod-group br-int group_id=100,type=indirect,bucket=actions=3

$ ovs-ofctl -Oopenflow13 dump-groups br-int
OFPST_GROUP_DESC reply (OF1.3) (xid=0x2):
 group_id=100,type=indirect,bucket=actions=output:3
 

Patch v2:
- Replaced new ovs-ofctl write-group command with --may-create option for mod-group
- Updated ovs-ofctl --help message
- Added a test for the new command option
- Updated documentation

Signed-off-by: Jan Scheurich <jan.scheurich at web.de>

To aid review this patch is available at:
    tree: https://github.com/JScheurich/openvswitch
    branch: dev/group_add_modify
    tag: group_add_modify_v2

---

 NEWS                            |  3 +++
 include/openflow/openflow-1.1.h |  1 +
 include/openflow/openflow-1.5.h |  1 +
 lib/ofp-parse.c                 |  4 ++++
 lib/ofp-print.c                 |  4 ++++
 lib/ofp-util.c                  |  7 ++++++-
 ofproto/ofproto.c               | 25 +++++++++++++++++++++++++
 tests/ofproto.at                | 26 ++++++++++++++++++++++++++
 utilities/ovs-ofctl.8.in        | 12 +++++++++---
 utilities/ovs-ofctl.c           | 14 +++++++++++++-
 10 files changed, 92 insertions(+), 5 deletions(-)

I have noticed in patchwork that this patch has been set to "Not Applicable"
http://patchwork.ozlabs.org/patch/630624/

What does that mean? Is there anything I did wrong? How should I fix this?

I checked today and the patch still applies on master except for the NEWS file.
I can submit a new version that fixes this.

Thanks, Jan

> -----Original Message-----
> From: Jan Scheurich [mailto:jan.scheurich@web.de]
> Sent: Monday, 06 June, 2016 00:31
> To: dev@openvswitch.org
> Cc: jan.scheurich@web.de
> Subject: [PATCH v2]: ofproto: Add relaxed group_mod command
> ADD_OR_MOD
> 
> This patch adds support for a new Group Mod command
> OFPGC_ADD_OR_MOD to OVS for all OpenFlow versions that support groups
> (OF11 and higher). The new ADD_OR_MOD creates a group that does not yet
> exist (like ADD) and modifies an existing group (like MODIFY).
> 
> Rational: In OpenFlow 1.x the Group Mod commands OFPGC_ADD and
> OFPGC_MODIFY have strict semantics: ADD fails if the group exists, while
> MODIFY fails if the group does not exist. This requires a controller to exactly
> know the state of the switch when programming a group in order not run the
> risk of getting an OFP Error message in response. This is hard to achieve and
> maintain at all times in view of possible switch and controller restarts or other
> connection losses between switch and controller.
> 
> Due to the un-acknowledged nature of the Group Mod message
> programming groups safely and efficiently at the same time is virtually
> impossible as the controller has to either query the existence of the group
> prior to each Group Mod message or to insert a Barrier Request/Reply after
> every group to be sure that no Error can be received at a later stage and
> require a complicated roll-back of any dependent actions taken between the
> failed Group Mod and the Error.
> 
> In the ovs-ofctl command line the ADD_OR_MOD command is made
> available through the new option --may-create in the mod-group command:
> 
> $ ovs-ofctl -Oopenflow13 del-groups br-int group_id=100
> 
> $ ovs-ofctl -Oopenflow13 mod-group br-int
> group_id=100,type=indirect,bucket=actions=2
> OFPT_ERROR (OF1.3) (xid=0x2): OFPGMFC_UNKNOWN_GROUP
> OFPT_GROUP_MOD (OF1.3) (xid=0x2):
>  MOD group_id=100,type=indirect,bucket=actions=output:2
> 
> $ ovs-ofctl -Oopenflow13 --may-create mod-group br-int
> group_id=100,type=indirect,bucket=actions=2
> 
> $ ovs-ofctl -Oopenflow13 dump-groups br-int OFPST_GROUP_DESC reply
> (OF1.3) (xid=0x2):
>  group_id=100,type=indirect,bucket=actions=output:2
> 
> $ ovs-ofctl -Oopenflow13 --may-create mod-group br-int
> group_id=100,type=indirect,bucket=actions=3
> 
> $ ovs-ofctl -Oopenflow13 dump-groups br-int OFPST_GROUP_DESC reply
> (OF1.3) (xid=0x2):
>  group_id=100,type=indirect,bucket=actions=output:3
> 
> 
> Patch v2:
> - Replaced new ovs-ofctl write-group command with --may-create option for
> mod-group
> - Updated ovs-ofctl --help message
> - Added a test for the new command option
> - Updated documentation
> 
> Signed-off-by: Jan Scheurich <jan.scheurich at web.de>
> 
> To aid review this patch is available at:
>     tree: https://github.com/JScheurich/openvswitch
>     branch: dev/group_add_modify
>     tag: group_add_modify_v2
> 
> ---
> 
>  NEWS                            |  3 +++
>  include/openflow/openflow-1.1.h |  1 +
>  include/openflow/openflow-1.5.h |  1 +
>  lib/ofp-parse.c                 |  4 ++++
>  lib/ofp-print.c                 |  4 ++++
>  lib/ofp-util.c                  |  7 ++++++-
>  ofproto/ofproto.c               | 25 +++++++++++++++++++++++++
>  tests/ofproto.at                | 26 ++++++++++++++++++++++++++
>  utilities/ovs-ofctl.8.in        | 12 +++++++++---
>  utilities/ovs-ofctl.c           | 14 +++++++++++++-
>  10 files changed, 92 insertions(+), 5 deletions(-)
> 
> 
> diff --git a/NEWS b/NEWS
> index ba201cf..74bda47 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -15,10 +15,13 @@ Post-v2.5.0
>         now implemented.  Only flow mod and port mod messages are
> supported
>         in bundles.
>       * New OpenFlow extension NXM_NX_MPLS_TTL to provide access to
> MPLS TTL.
> +     * New command OFPGC_ADD_OR_MOD for OFPT_GROUP_MOD
> message that adds a
> +       new group or modifies an existing groups
>     - ovs-ofctl:
>       * queue-get-config command now allows a queue ID to be specified.
>       * '--bundle' option can now be used with OpenFlow 1.3.
>       * New option "--color" to produce colorized output for some commands.
> +     * '--may-create' option to use OFPGC_ADD_OR_MOD in mod-group
> command.
>     - DPDK:
>       * New option "n_rxq" for PMD interfaces.
>         Old 'other_config:n-dpdk-rxqs' is no longer supported.
> diff --git a/include/openflow/openflow-1.1.h b/include/openflow/openflow-
> 1.1.h index 805f222..de28475 100644
> --- a/include/openflow/openflow-1.1.h
> +++ b/include/openflow/openflow-1.1.h
> @@ -172,6 +172,7 @@ enum ofp11_group_mod_command {
>      OFPGC11_ADD,          /* New group. */
>      OFPGC11_MODIFY,       /* Modify all matching groups. */
>      OFPGC11_DELETE,       /* Delete all matching groups. */
> +    OFPGC11_ADD_OR_MOD = 0x8000, /* Create new or modify existing
> + group. */
>  };
> 
>  /* OpenFlow 1.1 specific capabilities supported by the datapath (struct diff --
> git a/include/openflow/openflow-1.5.h b/include/openflow/openflow-1.5.h
> index 0c478d1..3649e6c 100644
> --- a/include/openflow/openflow-1.5.h
> +++ b/include/openflow/openflow-1.5.h
> @@ -59,6 +59,7 @@ enum ofp15_group_mod_command {
>      /* OFPGCXX_YYY = 4, */    /* Reserved for future use. */
>      OFPGC15_REMOVE_BUCKET = 5,/* Remove all action buckets or any
> specific
>                                   action bucket from matching group */
> +    OFPGC15_ADD_OR_MOD = 0x8000, /* Create new or modify existing
> + group. */
>  };
> 
>  /* Group bucket property types.  */
> diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c index 71c5cdf..4af6d9b 100644
> --- a/lib/ofp-parse.c
> +++ b/lib/ofp-parse.c
> @@ -1380,6 +1380,10 @@ parse_ofp_group_mod_str__(struct
> ofputil_group_mod *gm, uint16_t
>          fields = F_GROUP_TYPE | F_BUCKETS;
>          break;
> 
> +    case OFPGC11_ADD_OR_MOD:
> +        fields = F_GROUP_TYPE | F_BUCKETS;
> +        break;
> +
>      case OFPGC15_INSERT_BUCKET:
>          fields = F_BUCKETS | F_COMMAND_BUCKET_ID;
>          *usable_protocols &= OFPUTIL_P_OF15_UP; diff --git a/lib/ofp-print.c
> b/lib/ofp-print.c index b21d76f..ce2eecb 100644
> --- a/lib/ofp-print.c
> +++ b/lib/ofp-print.c
> @@ -2673,6 +2673,10 @@ ofp_print_group_mod__(struct ds *s, enum
> ofp_version ofp_version,
>          ds_put_cstr(s, "MOD");
>          break;
> 
> +    case OFPGC11_ADD_OR_MOD:
> +        ds_put_cstr(s, "ADD_OR_MOD");
> +        break;
> +
>      case OFPGC11_DELETE:
>          ds_put_cstr(s, "DEL");
>          break;
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c index 2c6fb1f..83685ca 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -8787,6 +8787,7 @@ parse_group_prop_ntr_selection_method(struct
> ofpbuf *payload,
>      switch (group_cmd) {
>      case OFPGC15_ADD:
>      case OFPGC15_MODIFY:
> +    case OFPGC15_ADD_OR_MOD:
>          break;
>      case OFPGC15_DELETE:
>      case OFPGC15_INSERT_BUCKET:
> @@ -9115,6 +9116,7 @@ bad_group_cmd(enum
> ofp15_group_mod_command cmd)
>      switch (cmd) {
>      case OFPGC15_ADD:
>      case OFPGC15_MODIFY:
> +    case OFPGC15_ADD_OR_MOD:
>      case OFPGC15_DELETE:
>          version = "1.1";
>          opt_version = "11";
> @@ -9136,6 +9138,7 @@ bad_group_cmd(enum
> ofp15_group_mod_command cmd)
>          break;
> 
>      case OFPGC15_MODIFY:
> +    case OFPGC15_ADD_OR_MOD:
>          cmd_str = "mod-group";
>          break;
> 
> @@ -9175,7 +9178,7 @@ ofputil_encode_group_mod(enum ofp_version
> ofp_version,
>      case OFP12_VERSION:
>      case OFP13_VERSION:
>      case OFP14_VERSION:
> -        if (gm->command > OFPGC11_DELETE) {
> +        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); @@ -
> 9246,6 +9249,7 @@ ofputil_pull_ofp15_group_mod(struct ofpbuf *msg,
> enum ofp_version
> 
>      case OFPGC11_ADD:
>      case OFPGC11_MODIFY:
> +    case OFPGC11_ADD_OR_MOD:
>      case OFPGC11_DELETE:
>      default:
>          if (gm->command_bucket_id == OFPG15_BUCKET_ALL) { @@ -9324,6
> +9328,7 @@ ofputil_decode_group_mod(const struct ofp_header *oh,
>      switch (gm->command) {
>      case OFPGC11_ADD:
>      case OFPGC11_MODIFY:
> +    case OFPGC11_ADD_OR_MOD:
>      case OFPGC11_DELETE:
>      case OFPGC15_INSERT_BUCKET:
>          break;
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 835a397..6321756
> 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -6634,6 +6634,27 @@ out:
>      return error;
>  }
> 
> +/* Implements the OFPGC11_ADD_OR_MOD command which creates the
> group
> +when it does not
> + * exist yet and modifies it otherwise */ static enum ofperr
> +add_or_modify_group(struct ofproto *ofproto, const struct
> +ofputil_group_mod *gm) {
> +    struct ofgroup *ofgroup;
> +    enum ofperr error;
> +    bool exists;
> +
> +    ovs_rwlock_rdlock(&ofproto->groups_rwlock);
> +    exists = ofproto_group_lookup__(ofproto, gm->group_id, &ofgroup);
> +    ovs_rwlock_unlock(&ofproto->groups_rwlock);
> +
> +    if (!exists) {
> +        error = add_group(ofproto, gm);
> +    } else {
> +        error = modify_group(ofproto, gm);
> +    }
> +    return error;
> +}
> +
>  static void
>  delete_group__(struct ofproto *ofproto, struct ofgroup *ofgroup)
>      OVS_RELEASES(ofproto->groups_rwlock)
> @@ -6723,6 +6744,10 @@ handle_group_mod(struct ofconn *ofconn, const
> struct ofp_header *
>          error = modify_group(ofproto, &gm);
>          break;
> 
> +    case OFPGC11_ADD_OR_MOD:
> +        error = add_or_modify_group(ofproto, &gm);
> +        break;
> +
>      case OFPGC11_DELETE:
>          delete_group(ofproto, gm.group_id);
>          error = 0;
> diff --git a/tests/ofproto.at b/tests/ofproto.at index f8c0d07..57ca50a 100644
> --- a/tests/ofproto.at
> +++ b/tests/ofproto.at
> @@ -393,6 +393,32 @@ AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn add-
> group br0 'group_id=12  OVS_VSWITCHD_STOP  AT_CLEANUP
> 
> +AT_SETUP([ofproto - group_mod with mod and add_or_mod command])
> +OVS_VSWITCHD_START dnl Check that mod-group for non-existing group
> +fails without --may-create AT_DATA([stderr], [dnl OFPT_ERROR (OF1.3)
> +(xid=0x2): OFPGMFC_UNKNOWN_GROUP OFPT_GROUP_MOD (OF1.3)
> (xid=0x2):
> + MOD group_id=1234,type=indirect,bucket=actions=output:2
> +])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn mod-group br0
> +'group_id=1234,type=indirect,buc dnl Check that mod-group for
> +non-existing group succeeds with --may-create AT_CHECK([ovs-ofctl -O
> +OpenFlow13 -vwarn --may-create mod-group br0 'group_id=1234,type
> +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn dump-groups br0], [0],
> +[stdout]) AT_CHECK([strip_xids < stdout], [0], [dnl OFPST_GROUP_DESC
> reply (OF1.3):
> + group_id=1234,type=indirect,bucket=actions=output:2
> +])
> +dnl Check that mod-group for existing group succeeds with --may-create
> +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn --may-create mod-group br0
> +'group_id=1234,type AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn
> +dump-groups br0], [0], [stdout]) AT_CHECK([strip_xids < stdout], [0],
> +[dnl OFPST_GROUP_DESC reply (OF1.3):
> + group_id=1234,type=indirect,bucket=actions=output:3
> +])
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
>  dnl This is really bare-bones.
>  dnl It at least checks request and reply serialization and deserialization.
>  dnl Actions definition listed in both supported formats (w/ actions=) diff --git
> a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in old mode 100644 new mode
> 100755 index e2e26f7..843480c
> --- a/utilities/ovs-ofctl.8.in
> +++ b/utilities/ovs-ofctl.8.in
> @@ -411,10 +411,10 @@ zero or more groups in the same syntax, one per
> line.
>  .IQ "\fBadd\-groups \fIswitch file\fR"
>  Add each group entry to \fIswitch\fR's tables.
>  .
> -.IP "\fBmod\-group \fIswitch group\fR"
> -.IQ "\fBmod\-group \fIswitch \fB\- < \fIfile\fR"
> +.IP "[\fB\-\-may\-create\fR] \fBmod\-group \fIswitch group\fR"
> +.IQ "[\fB\-\-may\-create\fR] \fBmod\-group \fIswitch \fB\- < \fIfile\fR"
>  Modify the action buckets in entries from \fIswitch\fR's tables for -each
> group entry.
> +each group entry. Optionally create non-existing group entries.
>  .
>  .IP "\fBdel\-groups \fIswitch\fR"
>  .IQ "\fBdel\-groups \fIswitch \fR[\fIgroup\fR]"
> @@ -2911,8 +2911,14 @@ Bundles require OpenFlow 1.4 or higher.  An
> explicit \fB-O  OpenFlow14\fR option is not needed, but you may need to
> enable  OpenFlow 1.4 support for OVS by setting the OVSDB \fIprotocols\fR
> column in the \fIbridge\fR table.
> +.
>  .RE
>  .
> +.IP "\fB\-\-may\-create\fR"
> +A mod-group command creates a group if it doesn't exist yet. This uses
> +an Open vSwitch extension to OpenFlow and only works with Open vSwitch
> +2.6 and later.
> +.
>  .so lib/ofp-version.man
>  .
>  .IP "\fB\-F \fIformat\fR[\fB,\fIformat\fR...]"
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c index a8116d9..5b90a82
> 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -84,6 +84,10 @@ static bool enable_color;
>   */
>  static bool strict;
> 
> +/* --may-create: A mod-group command creates a group that does not yet
> exist.
> + */
> +static bool may_create = false;
> +
>  /* --readd: If true, on replace-flows, re-add even flows that have not
> changed
>   * (to reset flow counters). */
>  static bool readd;
> @@ -175,6 +179,7 @@ parse_options(int argc, char *argv[])
>          OPT_UNIXCTL,
>          OPT_BUNDLE,
>          OPT_COLOR,
> +        OPT_MAY_CREATE,
>          DAEMON_OPTION_ENUMS,
>          OFP_VERSION_OPTION_ENUMS,
>          VLOG_OPTION_ENUMS
> @@ -194,6 +199,7 @@ parse_options(int argc, char *argv[])
>          {"option", no_argument, NULL, 'o'},
>          {"bundle", no_argument, NULL, OPT_BUNDLE},
>          {"color", optional_argument, NULL, OPT_COLOR},
> +        {"may-create", no_argument, NULL, OPT_MAY_CREATE},
>          DAEMON_LONG_OPTIONS,
>          OFP_VERSION_LONG_OPTIONS,
>          VLOG_LONG_OPTIONS,
> @@ -319,6 +325,10 @@ parse_options(int argc, char *argv[])
>              }
>          break;
> 
> +        case OPT_MAY_CREATE:
> +            may_create = true;
> +            break;
> +
>          DAEMON_OPTION_HANDLERS
>          OFP_VERSION_OPTION_HANDLERS
>          VLOG_OPTION_HANDLERS
> @@ -440,6 +450,7 @@ usage(void)
>      vlog_usage();
>      printf("\nOther options:\n"
>             "  --strict                    use strict match for flow commands\n"
> +           "  --may-create                mod-group creates a non-existing group\n"
>             "  --readd                     replace flows that haven't changed\n"
>             "  -F, --flow-format=FORMAT    force particular flow format\n"
>             "  -P, --packet-in-format=FRMT force particular packet in format\n"
> @@ -2520,7 +2531,8 @@ ofctl_add_groups(struct ovs_cmdl_context *ctx)
> static void  ofctl_mod_group(struct ovs_cmdl_context *ctx)  {
> -    ofctl_group_mod(ctx->argc, ctx->argv, OFPGC11_MODIFY);
> +    ofctl_group_mod(ctx->argc, ctx->argv,
> +                    may_create ? OFPGC11_ADD_OR_MOD : OFPGC11_MODIFY);
>  }
> 
>  static void

In this case it means that the patch literally fails to apply to the
current tree.  Ryan requested a rebase:
        http://openvswitch.org/pipermail/dev/2016-June/073435.html

On Tue, Jun 28, 2016 at 02:18:30PM +0000, Jan Scheurich wrote:
> I have noticed in patchwork that this patch has been set to "Not Applicable"
> http://patchwork.ozlabs.org/patch/630624/
> 
> What does that mean? Is there anything I did wrong? How should I fix this?
> 
> I checked today and the patch still applies on master except for the NEWS file.
> I can submit a new version that fixes this.
> 
> Thanks, Jan
> 
> > -----Original Message-----
> > From: Jan Scheurich [mailto:jan.scheurich@web.de]
> > Sent: Monday, 06 June, 2016 00:31
> > To: dev@openvswitch.org
> > Cc: jan.scheurich@web.de
> > Subject: [PATCH v2]: ofproto: Add relaxed group_mod command
> > ADD_OR_MOD
> > 
> > This patch adds support for a new Group Mod command
> > OFPGC_ADD_OR_MOD to OVS for all OpenFlow versions that support groups
> > (OF11 and higher). The new ADD_OR_MOD creates a group that does not yet
> > exist (like ADD) and modifies an existing group (like MODIFY).
> > 
> > Rational: In OpenFlow 1.x the Group Mod commands OFPGC_ADD and
> > OFPGC_MODIFY have strict semantics: ADD fails if the group exists, while
> > MODIFY fails if the group does not exist. This requires a controller to exactly
> > know the state of the switch when programming a group in order not run the
> > risk of getting an OFP Error message in response. This is hard to achieve and
> > maintain at all times in view of possible switch and controller restarts or other
> > connection losses between switch and controller.
> > 
> > Due to the un-acknowledged nature of the Group Mod message
> > programming groups safely and efficiently at the same time is virtually
> > impossible as the controller has to either query the existence of the group
> > prior to each Group Mod message or to insert a Barrier Request/Reply after
> > every group to be sure that no Error can be received at a later stage and
> > require a complicated roll-back of any dependent actions taken between the
> > failed Group Mod and the Error.
> > 
> > In the ovs-ofctl command line the ADD_OR_MOD command is made
> > available through the new option --may-create in the mod-group command:
> > 
> > $ ovs-ofctl -Oopenflow13 del-groups br-int group_id=100
> > 
> > $ ovs-ofctl -Oopenflow13 mod-group br-int
> > group_id=100,type=indirect,bucket=actions=2
> > OFPT_ERROR (OF1.3) (xid=0x2): OFPGMFC_UNKNOWN_GROUP
> > OFPT_GROUP_MOD (OF1.3) (xid=0x2):
> >  MOD group_id=100,type=indirect,bucket=actions=output:2
> > 
> > $ ovs-ofctl -Oopenflow13 --may-create mod-group br-int
> > group_id=100,type=indirect,bucket=actions=2
> > 
> > $ ovs-ofctl -Oopenflow13 dump-groups br-int OFPST_GROUP_DESC reply
> > (OF1.3) (xid=0x2):
> >  group_id=100,type=indirect,bucket=actions=output:2
> > 
> > $ ovs-ofctl -Oopenflow13 --may-create mod-group br-int
> > group_id=100,type=indirect,bucket=actions=3
> > 
> > $ ovs-ofctl -Oopenflow13 dump-groups br-int OFPST_GROUP_DESC reply
> > (OF1.3) (xid=0x2):
> >  group_id=100,type=indirect,bucket=actions=output:3
> > 
> > 
> > Patch v2:
> > - Replaced new ovs-ofctl write-group command with --may-create option for
> > mod-group
> > - Updated ovs-ofctl --help message
> > - Added a test for the new command option
> > - Updated documentation
> > 
> > Signed-off-by: Jan Scheurich <jan.scheurich at web.de>
> > 
> > To aid review this patch is available at:
> >     tree: https://github.com/JScheurich/openvswitch
> >     branch: dev/group_add_modify
> >     tag: group_add_modify_v2
> > 
> > ---
> > 
> >  NEWS                            |  3 +++
> >  include/openflow/openflow-1.1.h |  1 +
> >  include/openflow/openflow-1.5.h |  1 +
> >  lib/ofp-parse.c                 |  4 ++++
> >  lib/ofp-print.c                 |  4 ++++
> >  lib/ofp-util.c                  |  7 ++++++-
> >  ofproto/ofproto.c               | 25 +++++++++++++++++++++++++
> >  tests/ofproto.at                | 26 ++++++++++++++++++++++++++
> >  utilities/ovs-ofctl.8.in        | 12 +++++++++---
> >  utilities/ovs-ofctl.c           | 14 +++++++++++++-
> >  10 files changed, 92 insertions(+), 5 deletions(-)
> > 
> > 
> > diff --git a/NEWS b/NEWS
> > index ba201cf..74bda47 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -15,10 +15,13 @@ Post-v2.5.0
> >         now implemented.  Only flow mod and port mod messages are
> > supported
> >         in bundles.
> >       * New OpenFlow extension NXM_NX_MPLS_TTL to provide access to
> > MPLS TTL.
> > +     * New command OFPGC_ADD_OR_MOD for OFPT_GROUP_MOD
> > message that adds a
> > +       new group or modifies an existing groups
> >     - ovs-ofctl:
> >       * queue-get-config command now allows a queue ID to be specified.
> >       * '--bundle' option can now be used with OpenFlow 1.3.
> >       * New option "--color" to produce colorized output for some commands.
> > +     * '--may-create' option to use OFPGC_ADD_OR_MOD in mod-group
> > command.
> >     - DPDK:
> >       * New option "n_rxq" for PMD interfaces.
> >         Old 'other_config:n-dpdk-rxqs' is no longer supported.
> > diff --git a/include/openflow/openflow-1.1.h b/include/openflow/openflow-
> > 1.1.h index 805f222..de28475 100644
> > --- a/include/openflow/openflow-1.1.h
> > +++ b/include/openflow/openflow-1.1.h
> > @@ -172,6 +172,7 @@ enum ofp11_group_mod_command {
> >      OFPGC11_ADD,          /* New group. */
> >      OFPGC11_MODIFY,       /* Modify all matching groups. */
> >      OFPGC11_DELETE,       /* Delete all matching groups. */
> > +    OFPGC11_ADD_OR_MOD = 0x8000, /* Create new or modify existing
> > + group. */
> >  };
> > 
> >  /* OpenFlow 1.1 specific capabilities supported by the datapath (struct diff --
> > git a/include/openflow/openflow-1.5.h b/include/openflow/openflow-1.5.h
> > index 0c478d1..3649e6c 100644
> > --- a/include/openflow/openflow-1.5.h
> > +++ b/include/openflow/openflow-1.5.h
> > @@ -59,6 +59,7 @@ enum ofp15_group_mod_command {
> >      /* OFPGCXX_YYY = 4, */    /* Reserved for future use. */
> >      OFPGC15_REMOVE_BUCKET = 5,/* Remove all action buckets or any
> > specific
> >                                   action bucket from matching group */
> > +    OFPGC15_ADD_OR_MOD = 0x8000, /* Create new or modify existing
> > + group. */
> >  };
> > 
> >  /* Group bucket property types.  */
> > diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c index 71c5cdf..4af6d9b 100644
> > --- a/lib/ofp-parse.c
> > +++ b/lib/ofp-parse.c
> > @@ -1380,6 +1380,10 @@ parse_ofp_group_mod_str__(struct
> > ofputil_group_mod *gm, uint16_t
> >          fields = F_GROUP_TYPE | F_BUCKETS;
> >          break;
> > 
> > +    case OFPGC11_ADD_OR_MOD:
> > +        fields = F_GROUP_TYPE | F_BUCKETS;
> > +        break;
> > +
> >      case OFPGC15_INSERT_BUCKET:
> >          fields = F_BUCKETS | F_COMMAND_BUCKET_ID;
> >          *usable_protocols &= OFPUTIL_P_OF15_UP; diff --git a/lib/ofp-print.c
> > b/lib/ofp-print.c index b21d76f..ce2eecb 100644
> > --- a/lib/ofp-print.c
> > +++ b/lib/ofp-print.c
> > @@ -2673,6 +2673,10 @@ ofp_print_group_mod__(struct ds *s, enum
> > ofp_version ofp_version,
> >          ds_put_cstr(s, "MOD");
> >          break;
> > 
> > +    case OFPGC11_ADD_OR_MOD:
> > +        ds_put_cstr(s, "ADD_OR_MOD");
> > +        break;
> > +
> >      case OFPGC11_DELETE:
> >          ds_put_cstr(s, "DEL");
> >          break;
> > diff --git a/lib/ofp-util.c b/lib/ofp-util.c index 2c6fb1f..83685ca 100644
> > --- a/lib/ofp-util.c
> > +++ b/lib/ofp-util.c
> > @@ -8787,6 +8787,7 @@ parse_group_prop_ntr_selection_method(struct
> > ofpbuf *payload,
> >      switch (group_cmd) {
> >      case OFPGC15_ADD:
> >      case OFPGC15_MODIFY:
> > +    case OFPGC15_ADD_OR_MOD:
> >          break;
> >      case OFPGC15_DELETE:
> >      case OFPGC15_INSERT_BUCKET:
> > @@ -9115,6 +9116,7 @@ bad_group_cmd(enum
> > ofp15_group_mod_command cmd)
> >      switch (cmd) {
> >      case OFPGC15_ADD:
> >      case OFPGC15_MODIFY:
> > +    case OFPGC15_ADD_OR_MOD:
> >      case OFPGC15_DELETE:
> >          version = "1.1";
> >          opt_version = "11";
> > @@ -9136,6 +9138,7 @@ bad_group_cmd(enum
> > ofp15_group_mod_command cmd)
> >          break;
> > 
> >      case OFPGC15_MODIFY:
> > +    case OFPGC15_ADD_OR_MOD:
> >          cmd_str = "mod-group";
> >          break;
> > 
> > @@ -9175,7 +9178,7 @@ ofputil_encode_group_mod(enum ofp_version
> > ofp_version,
> >      case OFP12_VERSION:
> >      case OFP13_VERSION:
> >      case OFP14_VERSION:
> > -        if (gm->command > OFPGC11_DELETE) {
> > +        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); @@ -
> > 9246,6 +9249,7 @@ ofputil_pull_ofp15_group_mod(struct ofpbuf *msg,
> > enum ofp_version
> > 
> >      case OFPGC11_ADD:
> >      case OFPGC11_MODIFY:
> > +    case OFPGC11_ADD_OR_MOD:
> >      case OFPGC11_DELETE:
> >      default:
> >          if (gm->command_bucket_id == OFPG15_BUCKET_ALL) { @@ -9324,6
> > +9328,7 @@ ofputil_decode_group_mod(const struct ofp_header *oh,
> >      switch (gm->command) {
> >      case OFPGC11_ADD:
> >      case OFPGC11_MODIFY:
> > +    case OFPGC11_ADD_OR_MOD:
> >      case OFPGC11_DELETE:
> >      case OFPGC15_INSERT_BUCKET:
> >          break;
> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 835a397..6321756
> > 100644
> > --- a/ofproto/ofproto.c
> > +++ b/ofproto/ofproto.c
> > @@ -6634,6 +6634,27 @@ out:
> >      return error;
> >  }
> > 
> > +/* Implements the OFPGC11_ADD_OR_MOD command which creates the
> > group
> > +when it does not
> > + * exist yet and modifies it otherwise */ static enum ofperr
> > +add_or_modify_group(struct ofproto *ofproto, const struct
> > +ofputil_group_mod *gm) {
> > +    struct ofgroup *ofgroup;
> > +    enum ofperr error;
> > +    bool exists;
> > +
> > +    ovs_rwlock_rdlock(&ofproto->groups_rwlock);
> > +    exists = ofproto_group_lookup__(ofproto, gm->group_id, &ofgroup);
> > +    ovs_rwlock_unlock(&ofproto->groups_rwlock);
> > +
> > +    if (!exists) {
> > +        error = add_group(ofproto, gm);
> > +    } else {
> > +        error = modify_group(ofproto, gm);
> > +    }
> > +    return error;
> > +}
> > +
> >  static void
> >  delete_group__(struct ofproto *ofproto, struct ofgroup *ofgroup)
> >      OVS_RELEASES(ofproto->groups_rwlock)
> > @@ -6723,6 +6744,10 @@ handle_group_mod(struct ofconn *ofconn, const
> > struct ofp_header *
> >          error = modify_group(ofproto, &gm);
> >          break;
> > 
> > +    case OFPGC11_ADD_OR_MOD:
> > +        error = add_or_modify_group(ofproto, &gm);
> > +        break;
> > +
> >      case OFPGC11_DELETE:
> >          delete_group(ofproto, gm.group_id);
> >          error = 0;
> > diff --git a/tests/ofproto.at b/tests/ofproto.at index f8c0d07..57ca50a 100644
> > --- a/tests/ofproto.at
> > +++ b/tests/ofproto.at
> > @@ -393,6 +393,32 @@ AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn add-
> > group br0 'group_id=12  OVS_VSWITCHD_STOP  AT_CLEANUP
> > 
> > +AT_SETUP([ofproto - group_mod with mod and add_or_mod command])
> > +OVS_VSWITCHD_START dnl Check that mod-group for non-existing group
> > +fails without --may-create AT_DATA([stderr], [dnl OFPT_ERROR (OF1.3)
> > +(xid=0x2): OFPGMFC_UNKNOWN_GROUP OFPT_GROUP_MOD (OF1.3)
> > (xid=0x2):
> > + MOD group_id=1234,type=indirect,bucket=actions=output:2
> > +])
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn mod-group br0
> > +'group_id=1234,type=indirect,buc dnl Check that mod-group for
> > +non-existing group succeeds with --may-create AT_CHECK([ovs-ofctl -O
> > +OpenFlow13 -vwarn --may-create mod-group br0 'group_id=1234,type
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn dump-groups br0], [0],
> > +[stdout]) AT_CHECK([strip_xids < stdout], [0], [dnl OFPST_GROUP_DESC
> > reply (OF1.3):
> > + group_id=1234,type=indirect,bucket=actions=output:2
> > +])
> > +dnl Check that mod-group for existing group succeeds with --may-create
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn --may-create mod-group br0
> > +'group_id=1234,type AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn
> > +dump-groups br0], [0], [stdout]) AT_CHECK([strip_xids < stdout], [0],
> > +[dnl OFPST_GROUP_DESC reply (OF1.3):
> > + group_id=1234,type=indirect,bucket=actions=output:3
> > +])
> > +OVS_VSWITCHD_STOP
> > +AT_CLEANUP
> > +
> >  dnl This is really bare-bones.
> >  dnl It at least checks request and reply serialization and deserialization.
> >  dnl Actions definition listed in both supported formats (w/ actions=) diff --git
> > a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in old mode 100644 new mode
> > 100755 index e2e26f7..843480c
> > --- a/utilities/ovs-ofctl.8.in
> > +++ b/utilities/ovs-ofctl.8.in
> > @@ -411,10 +411,10 @@ zero or more groups in the same syntax, one per
> > line.
> >  .IQ "\fBadd\-groups \fIswitch file\fR"
> >  Add each group entry to \fIswitch\fR's tables.
> >  .
> > -.IP "\fBmod\-group \fIswitch group\fR"
> > -.IQ "\fBmod\-group \fIswitch \fB\- < \fIfile\fR"
> > +.IP "[\fB\-\-may\-create\fR] \fBmod\-group \fIswitch group\fR"
> > +.IQ "[\fB\-\-may\-create\fR] \fBmod\-group \fIswitch \fB\- < \fIfile\fR"
> >  Modify the action buckets in entries from \fIswitch\fR's tables for -each
> > group entry.
> > +each group entry. Optionally create non-existing group entries.
> >  .
> >  .IP "\fBdel\-groups \fIswitch\fR"
> >  .IQ "\fBdel\-groups \fIswitch \fR[\fIgroup\fR]"
> > @@ -2911,8 +2911,14 @@ Bundles require OpenFlow 1.4 or higher.  An
> > explicit \fB-O  OpenFlow14\fR option is not needed, but you may need to
> > enable  OpenFlow 1.4 support for OVS by setting the OVSDB \fIprotocols\fR
> > column in the \fIbridge\fR table.
> > +.
> >  .RE
> >  .
> > +.IP "\fB\-\-may\-create\fR"
> > +A mod-group command creates a group if it doesn't exist yet. This uses
> > +an Open vSwitch extension to OpenFlow and only works with Open vSwitch
> > +2.6 and later.
> > +.
> >  .so lib/ofp-version.man
> >  .
> >  .IP "\fB\-F \fIformat\fR[\fB,\fIformat\fR...]"
> > diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c index a8116d9..5b90a82
> > 100644
> > --- a/utilities/ovs-ofctl.c
> > +++ b/utilities/ovs-ofctl.c
> > @@ -84,6 +84,10 @@ static bool enable_color;
> >   */
> >  static bool strict;
> > 
> > +/* --may-create: A mod-group command creates a group that does not yet
> > exist.
> > + */
> > +static bool may_create = false;
> > +
> >  /* --readd: If true, on replace-flows, re-add even flows that have not
> > changed
> >   * (to reset flow counters). */
> >  static bool readd;
> > @@ -175,6 +179,7 @@ parse_options(int argc, char *argv[])
> >          OPT_UNIXCTL,
> >          OPT_BUNDLE,
> >          OPT_COLOR,
> > +        OPT_MAY_CREATE,
> >          DAEMON_OPTION_ENUMS,
> >          OFP_VERSION_OPTION_ENUMS,
> >          VLOG_OPTION_ENUMS
> > @@ -194,6 +199,7 @@ parse_options(int argc, char *argv[])
> >          {"option", no_argument, NULL, 'o'},
> >          {"bundle", no_argument, NULL, OPT_BUNDLE},
> >          {"color", optional_argument, NULL, OPT_COLOR},
> > +        {"may-create", no_argument, NULL, OPT_MAY_CREATE},
> >          DAEMON_LONG_OPTIONS,
> >          OFP_VERSION_LONG_OPTIONS,
> >          VLOG_LONG_OPTIONS,
> > @@ -319,6 +325,10 @@ parse_options(int argc, char *argv[])
> >              }
> >          break;
> > 
> > +        case OPT_MAY_CREATE:
> > +            may_create = true;
> > +            break;
> > +
> >          DAEMON_OPTION_HANDLERS
> >          OFP_VERSION_OPTION_HANDLERS
> >          VLOG_OPTION_HANDLERS
> > @@ -440,6 +450,7 @@ usage(void)
> >      vlog_usage();
> >      printf("\nOther options:\n"
> >             "  --strict                    use strict match for flow commands\n"
> > +           "  --may-create                mod-group creates a non-existing group\n"
> >             "  --readd                     replace flows that haven't changed\n"
> >             "  -F, --flow-format=FORMAT    force particular flow format\n"
> >             "  -P, --packet-in-format=FRMT force particular packet in format\n"
> > @@ -2520,7 +2531,8 @@ ofctl_add_groups(struct ovs_cmdl_context *ctx)
> > static void  ofctl_mod_group(struct ovs_cmdl_context *ctx)  {
> > -    ofctl_group_mod(ctx->argc, ctx->argv, OFPGC11_MODIFY);
> > +    ofctl_group_mod(ctx->argc, ctx->argv,
> > +                    may_create ? OFPGC11_ADD_OR_MOD : OFPGC11_MODIFY);
> >  }
> > 
> >  static void
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev