[ovs-dev,v3,2/9] ovs-vsctl: Add conntrack zone commands.

From: William Tu <u9012063@gmail.com>

The patch adds commands creating/deleting/listing conntrack zone
timeout policies:
  $ ovs-vsctl {add,del,list}-zone-tp dp zone=zone_id ...

Signed-off-by: William Tu <u9012063@gmail.com>
---
 tests/ovs-vsctl.at       |  34 ++++++++-
 utilities/ovs-vsctl.8.in |  26 +++++++
 utilities/ovs-vsctl.c    | 194 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 252 insertions(+), 2 deletions(-)

Thanks for the patch

Thanks for the fixups; mostly minor comments inline.

On Mon, Aug 12, 2019 at 5:53 PM Yi-Hung Wei <yihung.wei@gmail.com> wrote:

> From: William Tu <u9012063@gmail.com>
>
> The patch adds commands creating/deleting/listing conntrack zone
> timeout policies:
>   $ ovs-vsctl {add,del,list}-zone-tp dp zone=zone_id ...
>
> Signed-off-by: William Tu <u9012063@gmail.com>
> ---
>  tests/ovs-vsctl.at       |  34 ++++++++-
>  utilities/ovs-vsctl.8.in |  26 +++++++
>  utilities/ovs-vsctl.c    | 194
> +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 252 insertions(+), 2 deletions(-)
>
> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> index 46fa3c5b1a33..df15fb6901a0 100644
> --- a/tests/ovs-vsctl.at
> +++ b/tests/ovs-vsctl.at
> @@ -805,6 +805,20 @@ AT_CHECK(
>    [RUN_OVS_VSCTL([--if-exists remove netflow x targets '"1.2.3.4:567
> "'])])
>  AT_CHECK(
>    [RUN_OVS_VSCTL([--if-exists clear netflow x targets])])
> +
> +AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath datapath_version=0 --
> set Open_vSwitch . datapaths:"netdev"=@m])], [0], [stdout])
> +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1
> icmp_reply=2])])
> +AT_CHECK([RUN_OVS_VSCTL([--may-exist add-zone-tp netdev zone=1
> icmp_first=1 icmp_reply=2])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:1, Timeout
> Policies: icmp_first=1 icmp_reply=2
> +])
> +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2
> icmp_reply=3])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:1, Timeout
> Policies: icmp_first=1 icmp_reply=2
> +Zone:2, Timeout Policies: icmp_first=2 icmp_reply=3
> +])
> +AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=1])])
> +AT_CHECK([RUN_OVS_VSCTL([--if-exists del-zone-tp netdev zone=1])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout
> Policies: icmp_first=2 icmp_reply=3
> +])
>  OVS_VSCTL_CLEANUP
>  AT_CLEANUP
>
> @@ -890,10 +904,10 @@ AT_CHECK([RUN_OVS_VSCTL([set bridge br0
> flood_vlans=-1])],
>  AT_CHECK([RUN_OVS_VSCTL([set bridge br0 flood_vlans=4096])],
>    [1], [], [ovs-vsctl: constraint violation: 4096 is not in the valid
> range 0 to 4095 (inclusive)
>  ])
> -AT_CHECK([RUN_OVS_VSCTL([set c br1 'connection-mode=xyz'])],
> +AT_CHECK([RUN_OVS_VSCTL([set controller br1 'connection-mode=xyz'])],
>    [1], [], [[ovs-vsctl: constraint violation: xyz is not one of the
> allowed values ([in-band, out-of-band])
>  ]])
> -AT_CHECK([RUN_OVS_VSCTL([set c br1 connection-mode:x=y])],
> +AT_CHECK([RUN_OVS_VSCTL([set controller br1 connection-mode:x=y])],
>    [1], [], [ovs-vsctl: cannot specify key to set for non-map column
> connection_mode
>  ])
>  AT_CHECK([RUN_OVS_VSCTL([add bridge br1 datapath_id x y])],
> @@ -929,6 +943,22 @@ AT_CHECK([RUN_OVS_VSCTL([remove bridge br1
> flood-vlans true])],
>  AT_CHECK([RUN_OVS_VSCTL([clear bridge br1 name])],
>    [1], [], [ovs-vsctl: cannot modify read-only column name in table Bridge
>  ])
> +
> +AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath datapath_version=0 --
> set Open_vSwitch . datapaths:"netdev"=@m])], [0], [stdout])
>

If I execute

AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath datapath_version=0 --
set Open_vSwitch . datapaths:"netdevvv"=@m])], [0], [stdout])

it works, but there is no such datapath type 'netdevvv'

I think it would be better to enforce an enum here as well thru the schema,
as I mentioned in V2, since this handles errors better.
This is actually the same idea as enforcing enum for timeout keys that was
done for V3 to block bad timeout keys like "foo_bar=3"
I commented patch 1 anyways.


> +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdevxx zone=1 icmp_first=1
> icmp_reply=2])],
> +  [1], [], [ovs-vsctl: datapath netdevxx does not exist
> +])
> +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2
> icmp_reply=3])])
> +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2
> icmp_reply=3])],
> +  [1], [], [ovs-vsctl: zone id 2 already exists
> +])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout
> Policies: icmp_first=2 icmp_reply=3
> +])
> +AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=11])],
> +  [1], [], [ovs-vsctl: zone id 11 does not exist
> +])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout
> Policies: icmp_first=2 icmp_reply=3
> +])
>  OVS_VSCTL_CLEANUP
>  AT_CLEANUP
>
> diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
> index 7c09df79bd29..5b9883ae1c3d 100644
> --- a/utilities/ovs-vsctl.8.in
> +++ b/utilities/ovs-vsctl.8.in
> @@ -353,6 +353,32 @@ list.
>  Prints the name of the bridge that contains \fIiface\fR on standard
>  output.
>  .
> +.SS "Conntrack Zone Commands"
> +These commands query and modify datapath CT zones and Timeout Policies.
> +.
> +.IP "[\fB\-\-may\-exist\fR] \fBadd\-zone\-tp \fIdatapath
> \fBzone=\fIzone_id \fIpolicies\fR"
> +Creates a conntrack zone timeout policy with \fIzone_id\fR in
> +\fIdatapath\fR.  The \fIpolicies\fR consist of \fIkey\fB=\fIvalue\fR
> +pairs, separated by spaces.  For example, \fBicmp_first=30
> +icmp_reply=60\fR specifies a 30-second timeout policy for the first ICMP
> +packet and a 60-second policy for ICMP reply packet.  See the
> +\fBCT_Timeout_Policy\fR table in \fBovs-vswitchd.conf.db\fR(5) for the
> +supported keys.
> +.IP
> +Without \fB\-\-may\-exist\fR, attempting to add a \fIzone_id\fR that
> +already exists is an error.  With \fB\-\-may\-exist\fR,
> +this command does nothing if \fIzone_id\fR is already created\fR.
> +.
> +.IP "[\fB\-\-if\-exists\fR] \fBdel\-zone\-tp \fIdatapath
> \fBzone=\fIzone_id\fR"
> +Delete the timeout policy associated with \fIzone_id\fR from
> \fIdatapath\fR.
> +.IP
> +Without \fB\-\-if\-exists\fR, attempting to delete a zone that
> +does not exist is an error.  With \fB\-\-if\-exists\fR, attempting to
> +delete a zone that does not exist has no effect.
> +.
> +.IP "\fBlist\-zone\-tp \fIdatapath\fR"
> +Prints the timeout policies of all zones in \fIdatapath\fR.
> +.
>  .SS "OpenFlow Controller Connectivity"
>  .
>  \fBovs\-vswitchd\fR can perform all configured bridging and switching
> diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> index 4948137efe8c..7419f723804c 100644
> --- a/utilities/ovs-vsctl.c
> +++ b/utilities/ovs-vsctl.c
> @@ -40,6 +40,7 @@
>  #include "ovsdb-idl.h"
>  #include "openvswitch/poll-loop.h"
>  #include "process.h"
> +#include "simap.h"
>  #include "stream.h"
>  #include "stream-ssl.h"
>  #include "smap.h"
> @@ -49,6 +50,7 @@
>  #include "table.h"
>  #include "timeval.h"
>  #include "util.h"
> +#include "openvswitch/ofp-parse.h"
>

include ordering is not right


>  #include "openvswitch/vconn.h"
>  #include "openvswitch/vlog.h"
>
> @@ -1153,6 +1155,191 @@ cmd_emer_reset(struct ctl_context *ctx)
>      vsctl_context_invalidate_cache(ctx);
>  }
>
> +static struct ovsrec_datapath *
> +find_datapath(struct vsctl_context *vsctl_ctx, const char *dp_name)
> +{
> +    const struct ovsrec_open_vswitch *ovs = vsctl_ctx->ovs;
> +    int i;
>

remove



> +
> +    for (i = 0; i < ovs->n_datapaths; i++) {
>

+    for (int i = 0; i < ovs->n_datapaths; i++) {


> +        if (!strcmp(ovs->key_datapaths[i], dp_name)) {
> +            return ovs->value_datapaths[i];
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static struct ovsrec_ct_zone *
> +find_ct_zone(struct ovsrec_datapath *dp, const int64_t zone_id)
> +{
> +    int i;
>

remove


> +
> +    for (i = 0; i < dp->n_ct_zones; i++) {


+    for (int i = 0; i < dp->n_ct_zones; i++) {


> +        if (dp->key_ct_zones[i] == zone_id) {
> +            return dp->value_ct_zones[i];
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static struct ovsrec_ct_timeout_policy *
> +create_timeout_policy(struct ctl_context *ctx, char **argv, int n_tps)
>

recommend checking for n_tps of 0 and bailing out early
Could also check in the caller cmd_add_zone_tp().


> +{
> +    const struct ovsrec_ct_timeout_policy_table *tp_table;
> +    const struct ovsrec_ct_timeout_policy *row;
> +    struct ovsrec_ct_timeout_policy *tp = NULL;
>

I think the above 3 declarations could be moved after 'done:' label.

+    struct simap new_tp = SIMAP_INITIALIZER(&new_tp);
> +
> +    char **policies = xzalloc(sizeof *policies * n_tps);
> +    const char **key_timeouts = xmalloc(sizeof *key_timeouts * n_tps);
> +    int64_t *value_timeouts = xmalloc(sizeof *value_timeouts * n_tps);
> +
> +    /* Parse timeout arguments. */
> +    for (int i = 0; i < n_tps; i++) {
> +        policies[i] = xstrdup(argv[i]);
> +
> +        char *key, *value;
> +        char *policy = policies[i];
> +        if (!ofputil_parse_key_value(&policy, &key, &value)) {
> +            goto done;
> +        }
> +        key_timeouts[i] = key;
> +        value_timeouts[i] = atoi(value);
> +        simap_put(&new_tp, key, (unsigned int)value_timeouts[i]);
> +    }
> +
> +done:
> +    tp_table = ovsrec_ct_timeout_policy_table_get(ctx->idl);
> +    OVSREC_CT_TIMEOUT_POLICY_TABLE_FOR_EACH (row, tp_table) {
> +        struct simap s = SIMAP_INITIALIZER(&s);
> +
> +        /* Convert to simap. */
> +        for (int i = 0; i < row->n_timeouts; i++) {
> +            simap_put(&s, row->key_timeouts[i], row->value_timeouts[i]);
> +        }
> +
> +        if (simap_equal(&s, &new_tp)) {
> +            tp = CONST_CAST(struct ovsrec_ct_timeout_policy *, row);
> +            simap_destroy(&s);
> +            break;
> +        }
> +        simap_destroy(&s);
> +    }
> +
> +    if (!tp) {
> +        tp = ovsrec_ct_timeout_policy_insert(ctx->txn);
> +        ovsrec_ct_timeout_policy_set_timeouts(tp, key_timeouts,
> +                                              (const int64_t
> *)value_timeouts,
> +                                              n_tps);
> +    }
> +
> +    for (int i = 0; i < n_tps; i++) {
> +        free(policies[i]);
> +    }
> +    free(policies);
> +    simap_destroy(&new_tp);
> +    free(key_timeouts);
> +    free(value_timeouts);
> +    return tp;
> +}
> +
> +static void
> +cmd_add_zone_tp(struct ctl_context *ctx)
> +{
> +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> +    struct ovsrec_ct_timeout_policy *tp;
> +    int64_t zone_id;
> +
> +    const char *dp_name = ctx->argv[1];
> +    ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
> +    bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
> +
> +    struct ovsrec_datapath *dp = find_datapath(vsctl_ctx, dp_name);
> +    if (!dp) {
> +        ctl_fatal("datapath %s does not exist", dp_name);
> +    }
> +
> +    int n_tps = ctx->argc - 3;
> +    struct ovsrec_ct_zone *zone = find_ct_zone(dp, zone_id);
> +
> +    if (zone && !may_exist) {
> +        ctl_fatal("zone id %"PRIu64" already exists", zone_id);
> +    }
> +
> +    tp = create_timeout_policy(ctx, &ctx->argv[3], n_tps);
> +    if (zone) {
> +        ovsrec_ct_zone_set_timeout_policy(zone, tp);
> +    } else {
> +        zone = ovsrec_ct_zone_insert(ctx->txn);
> +        ovsrec_ct_zone_set_timeout_policy(zone, tp);
> +        ovsrec_datapath_update_ct_zones_setkey(dp, zone_id, zone);
> +    }
> +}
> +
> +static void
> +cmd_del_zone_tp(struct ctl_context *ctx)
> +{
> +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> +    int64_t zone_id;
> +
> +    bool must_exist = !shash_find(&ctx->options, "--if-exists");
> +    const char *dp_name = ctx->argv[1];
> +    ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
> +
> +    struct ovsrec_datapath *dp = find_datapath(vsctl_ctx, dp_name);
> +    if (!dp) {
> +        ctl_fatal("datapath %s does not exist", dp_name);
> +    }
> +
> +    struct ovsrec_ct_zone *zone = find_ct_zone(dp, zone_id);
> +    if (must_exist && !zone) {
> +        ctl_fatal("zone id %"PRIu64" does not exist", zone_id);
> +    }
> +
> +    if (zone) {
> +        ovsrec_datapath_update_ct_zones_delkey(dp, zone_id);
> +    }
> +}
> +
> +static void
> +cmd_list_zone_tp(struct ctl_context *ctx)
> +{
> +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> +
> +    struct ovsrec_datapath *dp = find_datapath(vsctl_ctx, ctx->argv[1]);
> +    if (!dp) {
> +        ctl_fatal("datapath: %s record not found", ctx->argv[1]);
> +    }
> +
> +    for (int i = 0; i < dp->n_ct_zones; i++) {
> +        struct ovsrec_ct_zone *zone = dp->value_ct_zones[i];
> +        ds_put_format(&ctx->output, "Zone:%"PRIu64", Timeout Policies: ",
> +                      dp->key_ct_zones[i]);
> +
> +        struct ovsrec_ct_timeout_policy *tp = zone->timeout_policy;
> +
> +        int j;
>
remove



> +        for (j = 0; j < tp->n_timeouts; j++) {
>


move into for loop
+        for (int j = 0; j < tp->n_timeouts; j++) {


> +            if (j == tp->n_timeouts - 1) {
> +                ds_put_format(&ctx->output, "%s=%"PRIu64"\n",
> +                          tp->key_timeouts[j], tp->value_timeouts[j]);
> +            } else {
> +                ds_put_format(&ctx->output, "%s=%"PRIu64" ",
> +                          tp->key_timeouts[j], tp->value_timeouts[j]);
> +            }
> +        }
> +    }
> +}
> +
> +static void
> +pre_get_zone(struct ctl_context *ctx)
> +{
> +    ovsdb_idl_add_column(ctx->idl, &ovsrec_open_vswitch_col_datapaths);
> +    ovsdb_idl_add_column(ctx->idl, &ovsrec_datapath_col_ct_zones);
> +    ovsdb_idl_add_column(ctx->idl, &ovsrec_ct_zone_col_timeout_policy);
> +    ovsdb_idl_add_column(ctx->idl,
> &ovsrec_ct_timeout_policy_col_timeouts);
> +}
> +
>  static void
>  cmd_add_br(struct ctl_context *ctx)
>  {
> @@ -2896,6 +3083,13 @@ static const struct ctl_command_syntax
> vsctl_commands[] = {
>      /* Switch commands. */
>      {"emer-reset", 0, 0, "", pre_cmd_emer_reset, cmd_emer_reset, NULL,
> "", RW},
>
> +    /* Zone and CT Timeout Policy commands. */
> +    {"add-zone-tp", 2, 19, "", pre_get_zone, cmd_add_zone_tp, NULL,
>

s/19/18/ ?


> +     "--may-exist", RW},
> +    {"del-zone-tp", 2, 2, "", pre_get_zone, cmd_del_zone_tp, NULL,
> +     "--if-exists", RW},
> +    {"list-zone-tp", 1, 1, "", pre_get_zone, cmd_list_zone_tp, NULL, "",
> RO},
> +
>      {NULL, 0, 0, NULL, NULL, NULL, NULL, NULL, RO},
>  };
>
> --
> 2.7.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

Thanks for the review.

On Mon, Aug 12, 2019 at 09:54:42PM -0700, Darrell Ball wrote:
> Thanks for the patch
> 
> Thanks for the fixups; mostly minor comments inline.
> 
> On Mon, Aug 12, 2019 at 5:53 PM Yi-Hung Wei <yihung.wei@gmail.com> wrote:
> 
> > From: William Tu <u9012063@gmail.com>
> >
> > The patch adds commands creating/deleting/listing conntrack zone
> > timeout policies:
> >   $ ovs-vsctl {add,del,list}-zone-tp dp zone=zone_id ...
> >
> > Signed-off-by: William Tu <u9012063@gmail.com>
> > ---
> >  tests/ovs-vsctl.at       |  34 ++++++++-
> >  utilities/ovs-vsctl.8.in |  26 +++++++
> >  utilities/ovs-vsctl.c    | 194
> > +++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 252 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> > index 46fa3c5b1a33..df15fb6901a0 100644
> > --- a/tests/ovs-vsctl.at
> > +++ b/tests/ovs-vsctl.at
> > @@ -805,6 +805,20 @@ AT_CHECK(
> >    [RUN_OVS_VSCTL([--if-exists remove netflow x targets '"1.2.3.4:567
> > "'])])
> >  AT_CHECK(
> >    [RUN_OVS_VSCTL([--if-exists clear netflow x targets])])
> > +
> > +AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath datapath_version=0 --
> > set Open_vSwitch . datapaths:"netdev"=@m])], [0], [stdout])
> > +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1
> > icmp_reply=2])])
> > +AT_CHECK([RUN_OVS_VSCTL([--may-exist add-zone-tp netdev zone=1
> > icmp_first=1 icmp_reply=2])])
> > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:1, Timeout
> > Policies: icmp_first=1 icmp_reply=2
> > +])
> > +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2
> > icmp_reply=3])])
> > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:1, Timeout
> > Policies: icmp_first=1 icmp_reply=2
> > +Zone:2, Timeout Policies: icmp_first=2 icmp_reply=3
> > +])
> > +AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=1])])
> > +AT_CHECK([RUN_OVS_VSCTL([--if-exists del-zone-tp netdev zone=1])])
> > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout
> > Policies: icmp_first=2 icmp_reply=3
> > +])
> >  OVS_VSCTL_CLEANUP
> >  AT_CLEANUP
> >
> > @@ -890,10 +904,10 @@ AT_CHECK([RUN_OVS_VSCTL([set bridge br0
> > flood_vlans=-1])],
> >  AT_CHECK([RUN_OVS_VSCTL([set bridge br0 flood_vlans=4096])],
> >    [1], [], [ovs-vsctl: constraint violation: 4096 is not in the valid
> > range 0 to 4095 (inclusive)
> >  ])
> > -AT_CHECK([RUN_OVS_VSCTL([set c br1 'connection-mode=xyz'])],
> > +AT_CHECK([RUN_OVS_VSCTL([set controller br1 'connection-mode=xyz'])],
> >    [1], [], [[ovs-vsctl: constraint violation: xyz is not one of the
> > allowed values ([in-band, out-of-band])
> >  ]])
> > -AT_CHECK([RUN_OVS_VSCTL([set c br1 connection-mode:x=y])],
> > +AT_CHECK([RUN_OVS_VSCTL([set controller br1 connection-mode:x=y])],
> >    [1], [], [ovs-vsctl: cannot specify key to set for non-map column
> > connection_mode
> >  ])
> >  AT_CHECK([RUN_OVS_VSCTL([add bridge br1 datapath_id x y])],
> > @@ -929,6 +943,22 @@ AT_CHECK([RUN_OVS_VSCTL([remove bridge br1
> > flood-vlans true])],
> >  AT_CHECK([RUN_OVS_VSCTL([clear bridge br1 name])],
> >    [1], [], [ovs-vsctl: cannot modify read-only column name in table Bridge
> >  ])
> > +
> > +AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath datapath_version=0 --
> > set Open_vSwitch . datapaths:"netdev"=@m])], [0], [stdout])
> >
> 
> If I execute
> 
> AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath datapath_version=0 --
> set Open_vSwitch . datapaths:"netdevvv"=@m])], [0], [stdout])
> 
> it works, but there is no such datapath type 'netdevvv'
> 
this is using database command, I don't think we should enforce it.

> I think it would be better to enforce an enum here as well thru the schema,
> as I mentioned in V2, since this handles errors better.
> This is actually the same idea as enforcing enum for timeout keys that was
> done for V3 to block bad timeout keys like "foo_bar=3"
> I commented patch 1 anyways.
> 
> 
> > +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdevxx zone=1 icmp_first=1
> > icmp_reply=2])],
> > +  [1], [], [ovs-vsctl: datapath netdevxx does not exist
> > +])
> > +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2
> > icmp_reply=3])])
> > +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2
> > icmp_reply=3])],
> > +  [1], [], [ovs-vsctl: zone id 2 already exists
> > +])
> > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout
> > Policies: icmp_first=2 icmp_reply=3
> > +])
> > +AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=11])],
> > +  [1], [], [ovs-vsctl: zone id 11 does not exist
> > +])
> > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout
> > Policies: icmp_first=2 icmp_reply=3
> > +])
> >  OVS_VSCTL_CLEANUP
> >  AT_CLEANUP
> >
> > diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
> > index 7c09df79bd29..5b9883ae1c3d 100644
> > --- a/utilities/ovs-vsctl.8.in
> > +++ b/utilities/ovs-vsctl.8.in
> > @@ -353,6 +353,32 @@ list.
> >  Prints the name of the bridge that contains \fIiface\fR on standard
> >  output.
> >  .
> > +.SS "Conntrack Zone Commands"
> > +These commands query and modify datapath CT zones and Timeout Policies.
> > +.
> > +.IP "[\fB\-\-may\-exist\fR] \fBadd\-zone\-tp \fIdatapath
> > \fBzone=\fIzone_id \fIpolicies\fR"
> > +Creates a conntrack zone timeout policy with \fIzone_id\fR in
> > +\fIdatapath\fR.  The \fIpolicies\fR consist of \fIkey\fB=\fIvalue\fR
> > +pairs, separated by spaces.  For example, \fBicmp_first=30
> > +icmp_reply=60\fR specifies a 30-second timeout policy for the first ICMP
> > +packet and a 60-second policy for ICMP reply packet.  See the
> > +\fBCT_Timeout_Policy\fR table in \fBovs-vswitchd.conf.db\fR(5) for the
> > +supported keys.
> > +.IP
> > +Without \fB\-\-may\-exist\fR, attempting to add a \fIzone_id\fR that
> > +already exists is an error.  With \fB\-\-may\-exist\fR,
> > +this command does nothing if \fIzone_id\fR is already created\fR.
> > +.
> > +.IP "[\fB\-\-if\-exists\fR] \fBdel\-zone\-tp \fIdatapath
> > \fBzone=\fIzone_id\fR"
> > +Delete the timeout policy associated with \fIzone_id\fR from
> > \fIdatapath\fR.
> > +.IP
> > +Without \fB\-\-if\-exists\fR, attempting to delete a zone that
> > +does not exist is an error.  With \fB\-\-if\-exists\fR, attempting to
> > +delete a zone that does not exist has no effect.
> > +.
> > +.IP "\fBlist\-zone\-tp \fIdatapath\fR"
> > +Prints the timeout policies of all zones in \fIdatapath\fR.
> > +.
> >  .SS "OpenFlow Controller Connectivity"
> >  .
> >  \fBovs\-vswitchd\fR can perform all configured bridging and switching
> > diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> > index 4948137efe8c..7419f723804c 100644
> > --- a/utilities/ovs-vsctl.c
> > +++ b/utilities/ovs-vsctl.c
> > @@ -40,6 +40,7 @@
> >  #include "ovsdb-idl.h"
> >  #include "openvswitch/poll-loop.h"
> >  #include "process.h"
> > +#include "simap.h"
> >  #include "stream.h"
> >  #include "stream-ssl.h"
> >  #include "smap.h"
> > @@ -49,6 +50,7 @@
> >  #include "table.h"
> >  #include "timeval.h"
> >  #include "util.h"
> > +#include "openvswitch/ofp-parse.h"
> >
> 
> include ordering is not right
> 
OK
> 
> >  #include "openvswitch/vconn.h"
> >  #include "openvswitch/vlog.h"
> >
> > @@ -1153,6 +1155,191 @@ cmd_emer_reset(struct ctl_context *ctx)
> >      vsctl_context_invalidate_cache(ctx);
> >  }
> >
> > +static struct ovsrec_datapath *
> > +find_datapath(struct vsctl_context *vsctl_ctx, const char *dp_name)
> > +{
> > +    const struct ovsrec_open_vswitch *ovs = vsctl_ctx->ovs;
> > +    int i;
> >
> 
> remove
> 
> 
> 
> > +
> > +    for (i = 0; i < ovs->n_datapaths; i++) {
> >
> 
> +    for (int i = 0; i < ovs->n_datapaths; i++) {
> 
> 
> > +        if (!strcmp(ovs->key_datapaths[i], dp_name)) {
> > +            return ovs->value_datapaths[i];
> > +        }
> > +    }
> > +    return NULL;
> > +}
> > +
> > +static struct ovsrec_ct_zone *
> > +find_ct_zone(struct ovsrec_datapath *dp, const int64_t zone_id)
> > +{
> > +    int i;
> >
> 
> remove
> 
> 
> > +
> > +    for (i = 0; i < dp->n_ct_zones; i++) {
> 
> 
> +    for (int i = 0; i < dp->n_ct_zones; i++) {
> 
> 
> > +        if (dp->key_ct_zones[i] == zone_id) {
> > +            return dp->value_ct_zones[i];
> > +        }
> > +    }
> > +    return NULL;
> > +}
> > +
> > +static struct ovsrec_ct_timeout_policy *
> > +create_timeout_policy(struct ctl_context *ctx, char **argv, int n_tps)
> >
> 
> recommend checking for n_tps of 0 and bailing out early
> Could also check in the caller cmd_add_zone_tp().
OK
> 
> 
> > +{
> > +    const struct ovsrec_ct_timeout_policy_table *tp_table;
> > +    const struct ovsrec_ct_timeout_policy *row;
> > +    struct ovsrec_ct_timeout_policy *tp = NULL;
> >
> 
> I think the above 3 declarations could be moved after 'done:' label.
> 
> +    struct simap new_tp = SIMAP_INITIALIZER(&new_tp);
> > +
> > +    char **policies = xzalloc(sizeof *policies * n_tps);
> > +    const char **key_timeouts = xmalloc(sizeof *key_timeouts * n_tps);
> > +    int64_t *value_timeouts = xmalloc(sizeof *value_timeouts * n_tps);
> > +
> > +    /* Parse timeout arguments. */
> > +    for (int i = 0; i < n_tps; i++) {
> > +        policies[i] = xstrdup(argv[i]);
> > +
> > +        char *key, *value;
> > +        char *policy = policies[i];
> > +        if (!ofputil_parse_key_value(&policy, &key, &value)) {
> > +            goto done;
> > +        }
> > +        key_timeouts[i] = key;
> > +        value_timeouts[i] = atoi(value);
> > +        simap_put(&new_tp, key, (unsigned int)value_timeouts[i]);
> > +    }
> > +
> > +done:
> > +    tp_table = ovsrec_ct_timeout_policy_table_get(ctx->idl);
> > +    OVSREC_CT_TIMEOUT_POLICY_TABLE_FOR_EACH (row, tp_table) {
> > +        struct simap s = SIMAP_INITIALIZER(&s);
> > +
> > +        /* Convert to simap. */
> > +        for (int i = 0; i < row->n_timeouts; i++) {
> > +            simap_put(&s, row->key_timeouts[i], row->value_timeouts[i]);
> > +        }
> > +
> > +        if (simap_equal(&s, &new_tp)) {
> > +            tp = CONST_CAST(struct ovsrec_ct_timeout_policy *, row);
> > +            simap_destroy(&s);
> > +            break;
> > +        }
> > +        simap_destroy(&s);
> > +    }
> > +
> > +    if (!tp) {
> > +        tp = ovsrec_ct_timeout_policy_insert(ctx->txn);
> > +        ovsrec_ct_timeout_policy_set_timeouts(tp, key_timeouts,
> > +                                              (const int64_t
> > *)value_timeouts,
> > +                                              n_tps);
> > +    }
> > +
> > +    for (int i = 0; i < n_tps; i++) {
> > +        free(policies[i]);
> > +    }
> > +    free(policies);
> > +    simap_destroy(&new_tp);
> > +    free(key_timeouts);
> > +    free(value_timeouts);
> > +    return tp;
> > +}
> > +
> > +static void
> > +cmd_add_zone_tp(struct ctl_context *ctx)
> > +{
> > +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> > +    struct ovsrec_ct_timeout_policy *tp;
> > +    int64_t zone_id;
> > +
> > +    const char *dp_name = ctx->argv[1];
> > +    ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
> > +    bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
> > +
> > +    struct ovsrec_datapath *dp = find_datapath(vsctl_ctx, dp_name);
> > +    if (!dp) {
> > +        ctl_fatal("datapath %s does not exist", dp_name);
> > +    }
> > +
> > +    int n_tps = ctx->argc - 3;
> > +    struct ovsrec_ct_zone *zone = find_ct_zone(dp, zone_id);
> > +
> > +    if (zone && !may_exist) {
> > +        ctl_fatal("zone id %"PRIu64" already exists", zone_id);
> > +    }
> > +
> > +    tp = create_timeout_policy(ctx, &ctx->argv[3], n_tps);
> > +    if (zone) {
> > +        ovsrec_ct_zone_set_timeout_policy(zone, tp);
> > +    } else {
> > +        zone = ovsrec_ct_zone_insert(ctx->txn);
> > +        ovsrec_ct_zone_set_timeout_policy(zone, tp);
> > +        ovsrec_datapath_update_ct_zones_setkey(dp, zone_id, zone);
> > +    }
> > +}
> > +
> > +static void
> > +cmd_del_zone_tp(struct ctl_context *ctx)
> > +{
> > +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> > +    int64_t zone_id;
> > +
> > +    bool must_exist = !shash_find(&ctx->options, "--if-exists");
> > +    const char *dp_name = ctx->argv[1];
> > +    ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
> > +
> > +    struct ovsrec_datapath *dp = find_datapath(vsctl_ctx, dp_name);
> > +    if (!dp) {
> > +        ctl_fatal("datapath %s does not exist", dp_name);
> > +    }
> > +
> > +    struct ovsrec_ct_zone *zone = find_ct_zone(dp, zone_id);
> > +    if (must_exist && !zone) {
> > +        ctl_fatal("zone id %"PRIu64" does not exist", zone_id);
> > +    }
> > +
> > +    if (zone) {
> > +        ovsrec_datapath_update_ct_zones_delkey(dp, zone_id);
> > +    }
> > +}
> > +
> > +static void
> > +cmd_list_zone_tp(struct ctl_context *ctx)
> > +{
> > +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> > +
> > +    struct ovsrec_datapath *dp = find_datapath(vsctl_ctx, ctx->argv[1]);
> > +    if (!dp) {
> > +        ctl_fatal("datapath: %s record not found", ctx->argv[1]);
> > +    }
> > +
> > +    for (int i = 0; i < dp->n_ct_zones; i++) {
> > +        struct ovsrec_ct_zone *zone = dp->value_ct_zones[i];
> > +        ds_put_format(&ctx->output, "Zone:%"PRIu64", Timeout Policies: ",
> > +                      dp->key_ct_zones[i]);
> > +
> > +        struct ovsrec_ct_timeout_policy *tp = zone->timeout_policy;
> > +
> > +        int j;
> >
> remove
> 
> 
> 
> > +        for (j = 0; j < tp->n_timeouts; j++) {
> >
> 
> 
> move into for loop
> +        for (int j = 0; j < tp->n_timeouts; j++) {
> 
> 
> > +            if (j == tp->n_timeouts - 1) {
> > +                ds_put_format(&ctx->output, "%s=%"PRIu64"\n",
> > +                          tp->key_timeouts[j], tp->value_timeouts[j]);
> > +            } else {
> > +                ds_put_format(&ctx->output, "%s=%"PRIu64" ",
> > +                          tp->key_timeouts[j], tp->value_timeouts[j]);
> > +            }
> > +        }
> > +    }
> > +}
> > +
> > +static void
> > +pre_get_zone(struct ctl_context *ctx)
> > +{
> > +    ovsdb_idl_add_column(ctx->idl, &ovsrec_open_vswitch_col_datapaths);
> > +    ovsdb_idl_add_column(ctx->idl, &ovsrec_datapath_col_ct_zones);
> > +    ovsdb_idl_add_column(ctx->idl, &ovsrec_ct_zone_col_timeout_policy);
> > +    ovsdb_idl_add_column(ctx->idl,
> > &ovsrec_ct_timeout_policy_col_timeouts);
> > +}
> > +
> >  static void
> >  cmd_add_br(struct ctl_context *ctx)
> >  {
> > @@ -2896,6 +3083,13 @@ static const struct ctl_command_syntax
> > vsctl_commands[] = {
> >      /* Switch commands. */
> >      {"emer-reset", 0, 0, "", pre_cmd_emer_reset, cmd_emer_reset, NULL,
> > "", RW},
> >
> > +    /* Zone and CT Timeout Policy commands. */
> > +    {"add-zone-tp", 2, 19, "", pre_get_zone, cmd_add_zone_tp, NULL,
> >
> 
> s/19/18/ ?

good catch, thanks
William

> 
> 
> > +     "--may-exist", RW},
> > +    {"del-zone-tp", 2, 2, "", pre_get_zone, cmd_del_zone_tp, NULL,
> > +     "--if-exists", RW},
> > +    {"list-zone-tp", 1, 1, "", pre_get_zone, cmd_list_zone_tp, NULL, "",
> > RO},
> > +
> >      {NULL, 0, 0, NULL, NULL, NULL, NULL, NULL, RO},
> >  };
> >
> > --
> > 2.7.4
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On Thu, Aug 15, 2019 at 10:53 AM William Tu <u9012063@gmail.com> wrote:

> Thanks for the review.
>
> On Mon, Aug 12, 2019 at 09:54:42PM -0700, Darrell Ball wrote:
> > Thanks for the patch
> >
> > Thanks for the fixups; mostly minor comments inline.
> >
> > On Mon, Aug 12, 2019 at 5:53 PM Yi-Hung Wei <yihung.wei@gmail.com>
> wrote:
> >
> > > From: William Tu <u9012063@gmail.com>
> > >
> > > The patch adds commands creating/deleting/listing conntrack zone
> > > timeout policies:
> > >   $ ovs-vsctl {add,del,list}-zone-tp dp zone=zone_id ...
> > >
> > > Signed-off-by: William Tu <u9012063@gmail.com>
> > > ---
> > >  tests/ovs-vsctl.at       |  34 ++++++++-
> > >  utilities/ovs-vsctl.8.in |  26 +++++++
> > >  utilities/ovs-vsctl.c    | 194
> > > +++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 252 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> > > index 46fa3c5b1a33..df15fb6901a0 100644
> > > --- a/tests/ovs-vsctl.at
> > > +++ b/tests/ovs-vsctl.at
> > > @@ -805,6 +805,20 @@ AT_CHECK(
> > >    [RUN_OVS_VSCTL([--if-exists remove netflow x targets '"1.2.3.4:567
> > > "'])])
> > >  AT_CHECK(
> > >    [RUN_OVS_VSCTL([--if-exists clear netflow x targets])])
> > > +
> > > +AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath
> datapath_version=0 --
> > > set Open_vSwitch . datapaths:"netdev"=@m])], [0], [stdout])
> > > +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1
> > > icmp_reply=2])])
> > > +AT_CHECK([RUN_OVS_VSCTL([--may-exist add-zone-tp netdev zone=1
> > > icmp_first=1 icmp_reply=2])])
> > > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:1, Timeout
> > > Policies: icmp_first=1 icmp_reply=2
> > > +])
> > > +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2
> > > icmp_reply=3])])
> > > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:1, Timeout
> > > Policies: icmp_first=1 icmp_reply=2
> > > +Zone:2, Timeout Policies: icmp_first=2 icmp_reply=3
> > > +])
> > > +AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=1])])
> > > +AT_CHECK([RUN_OVS_VSCTL([--if-exists del-zone-tp netdev zone=1])])
> > > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout
> > > Policies: icmp_first=2 icmp_reply=3
> > > +])
> > >  OVS_VSCTL_CLEANUP
> > >  AT_CLEANUP
> > >
> > > @@ -890,10 +904,10 @@ AT_CHECK([RUN_OVS_VSCTL([set bridge br0
> > > flood_vlans=-1])],
> > >  AT_CHECK([RUN_OVS_VSCTL([set bridge br0 flood_vlans=4096])],
> > >    [1], [], [ovs-vsctl: constraint violation: 4096 is not in the valid
> > > range 0 to 4095 (inclusive)
> > >  ])
> > > -AT_CHECK([RUN_OVS_VSCTL([set c br1 'connection-mode=xyz'])],
> > > +AT_CHECK([RUN_OVS_VSCTL([set controller br1 'connection-mode=xyz'])],
> > >    [1], [], [[ovs-vsctl: constraint violation: xyz is not one of the
> > > allowed values ([in-band, out-of-band])
> > >  ]])
> > > -AT_CHECK([RUN_OVS_VSCTL([set c br1 connection-mode:x=y])],
> > > +AT_CHECK([RUN_OVS_VSCTL([set controller br1 connection-mode:x=y])],
> > >    [1], [], [ovs-vsctl: cannot specify key to set for non-map column
> > > connection_mode
> > >  ])
> > >  AT_CHECK([RUN_OVS_VSCTL([add bridge br1 datapath_id x y])],
> > > @@ -929,6 +943,22 @@ AT_CHECK([RUN_OVS_VSCTL([remove bridge br1
> > > flood-vlans true])],
> > >  AT_CHECK([RUN_OVS_VSCTL([clear bridge br1 name])],
> > >    [1], [], [ovs-vsctl: cannot modify read-only column name in table
> Bridge
> > >  ])
> > > +
> > > +AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath
> datapath_version=0 --
> > > set Open_vSwitch . datapaths:"netdev"=@m])], [0], [stdout])
> > >
> >
> > If I execute
> >
> > AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath datapath_version=0 --
> > set Open_vSwitch . datapaths:"netdevvv"=@m])], [0], [stdout])
> >
> > it works, but there is no such datapath type 'netdevvv'
> >
> this is using database command, I don't think we should enforce it.
>

JTBC, there is nothing to do for this aspect in terms of the ovs-vsctl
command support.
I mentioned here for context, bcoz the tests are in this patch.
Below paragraph has more context, but don't worry about that either.


>
> > I think it would be better to enforce an enum here as well thru the
> schema,
> > as I mentioned in V2, since this handles errors better.
> > This is actually the same idea as enforcing enum for timeout keys that
> was
> > done for V3 to block bad timeout keys like "foo_bar=3"
> > I commented patch 1 anyways.
> >
> >
> > > +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdevxx zone=1 icmp_first=1
> > > icmp_reply=2])],
> > > +  [1], [], [ovs-vsctl: datapath netdevxx does not exist
> > > +])
> > > +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2
> > > icmp_reply=3])])
> > > +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2
> > > icmp_reply=3])],
> > > +  [1], [], [ovs-vsctl: zone id 2 already exists
> > > +])
> > > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout
> > > Policies: icmp_first=2 icmp_reply=3
> > > +])
> > > +AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=11])],
> > > +  [1], [], [ovs-vsctl: zone id 11 does not exist
> > > +])
> > > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout
> > > Policies: icmp_first=2 icmp_reply=3
> > > +])
> > >  OVS_VSCTL_CLEANUP
> > >  AT_CLEANUP
> > >
> > > diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
> > > index 7c09df79bd29..5b9883ae1c3d 100644
> > > --- a/utilities/ovs-vsctl.8.in
> > > +++ b/utilities/ovs-vsctl.8.in
> > > @@ -353,6 +353,32 @@ list.
> > >  Prints the name of the bridge that contains \fIiface\fR on standard
> > >  output.
> > >  .
> > > +.SS "Conntrack Zone Commands"
> > > +These commands query and modify datapath CT zones and Timeout
> Policies.
> > > +.
> > > +.IP "[\fB\-\-may\-exist\fR] \fBadd\-zone\-tp \fIdatapath
> > > \fBzone=\fIzone_id \fIpolicies\fR"
> > > +Creates a conntrack zone timeout policy with \fIzone_id\fR in
> > > +\fIdatapath\fR.  The \fIpolicies\fR consist of \fIkey\fB=\fIvalue\fR
> > > +pairs, separated by spaces.  For example, \fBicmp_first=30
> > > +icmp_reply=60\fR specifies a 30-second timeout policy for the first
> ICMP
> > > +packet and a 60-second policy for ICMP reply packet.  See the
> > > +\fBCT_Timeout_Policy\fR table in \fBovs-vswitchd.conf.db\fR(5) for the
> > > +supported keys.
> > > +.IP
> > > +Without \fB\-\-may\-exist\fR, attempting to add a \fIzone_id\fR that
> > > +already exists is an error.  With \fB\-\-may\-exist\fR,
> > > +this command does nothing if \fIzone_id\fR is already created\fR.
> > > +.
> > > +.IP "[\fB\-\-if\-exists\fR] \fBdel\-zone\-tp \fIdatapath
> > > \fBzone=\fIzone_id\fR"
> > > +Delete the timeout policy associated with \fIzone_id\fR from
> > > \fIdatapath\fR.
> > > +.IP
> > > +Without \fB\-\-if\-exists\fR, attempting to delete a zone that
> > > +does not exist is an error.  With \fB\-\-if\-exists\fR, attempting to
> > > +delete a zone that does not exist has no effect.
> > > +.
> > > +.IP "\fBlist\-zone\-tp \fIdatapath\fR"
> > > +Prints the timeout policies of all zones in \fIdatapath\fR.
> > > +.
> > >  .SS "OpenFlow Controller Connectivity"
> > >  .
> > >  \fBovs\-vswitchd\fR can perform all configured bridging and switching
> > > diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> > > index 4948137efe8c..7419f723804c 100644
> > > --- a/utilities/ovs-vsctl.c
> > > +++ b/utilities/ovs-vsctl.c
> > > @@ -40,6 +40,7 @@
> > >  #include "ovsdb-idl.h"
> > >  #include "openvswitch/poll-loop.h"
> > >  #include "process.h"
> > > +#include "simap.h"
> > >  #include "stream.h"
> > >  #include "stream-ssl.h"
> > >  #include "smap.h"
> > > @@ -49,6 +50,7 @@
> > >  #include "table.h"
> > >  #include "timeval.h"
> > >  #include "util.h"
> > > +#include "openvswitch/ofp-parse.h"
> > >
> >
> > include ordering is not right
> >
> OK
> >
> > >  #include "openvswitch/vconn.h"
> > >  #include "openvswitch/vlog.h"
> > >
> > > @@ -1153,6 +1155,191 @@ cmd_emer_reset(struct ctl_context *ctx)
> > >      vsctl_context_invalidate_cache(ctx);
> > >  }
> > >
> > > +static struct ovsrec_datapath *
> > > +find_datapath(struct vsctl_context *vsctl_ctx, const char *dp_name)
> > > +{
> > > +    const struct ovsrec_open_vswitch *ovs = vsctl_ctx->ovs;
> > > +    int i;
> > >
> >
> > remove
> >
> >
> >
> > > +
> > > +    for (i = 0; i < ovs->n_datapaths; i++) {
> > >
> >
> > +    for (int i = 0; i < ovs->n_datapaths; i++) {
> >
> >
> > > +        if (!strcmp(ovs->key_datapaths[i], dp_name)) {
> > > +            return ovs->value_datapaths[i];
> > > +        }
> > > +    }
> > > +    return NULL;
> > > +}
> > > +
> > > +static struct ovsrec_ct_zone *
> > > +find_ct_zone(struct ovsrec_datapath *dp, const int64_t zone_id)
> > > +{
> > > +    int i;
> > >
> >
> > remove
> >
> >
> > > +
> > > +    for (i = 0; i < dp->n_ct_zones; i++) {
> >
> >
> > +    for (int i = 0; i < dp->n_ct_zones; i++) {
> >
> >
> > > +        if (dp->key_ct_zones[i] == zone_id) {
> > > +            return dp->value_ct_zones[i];
> > > +        }
> > > +    }
> > > +    return NULL;
> > > +}
> > > +
> > > +static struct ovsrec_ct_timeout_policy *
> > > +create_timeout_policy(struct ctl_context *ctx, char **argv, int n_tps)
> > >
> >
> > recommend checking for n_tps of 0 and bailing out early
> > Could also check in the caller cmd_add_zone_tp().
> OK
> >
> >
> > > +{
> > > +    const struct ovsrec_ct_timeout_policy_table *tp_table;
> > > +    const struct ovsrec_ct_timeout_policy *row;
> > > +    struct ovsrec_ct_timeout_policy *tp = NULL;
> > >
> >
> > I think the above 3 declarations could be moved after 'done:' label.
> >
> > +    struct simap new_tp = SIMAP_INITIALIZER(&new_tp);
> > > +
> > > +    char **policies = xzalloc(sizeof *policies * n_tps);
> > > +    const char **key_timeouts = xmalloc(sizeof *key_timeouts * n_tps);
> > > +    int64_t *value_timeouts = xmalloc(sizeof *value_timeouts * n_tps);
> > > +
> > > +    /* Parse timeout arguments. */
> > > +    for (int i = 0; i < n_tps; i++) {
> > > +        policies[i] = xstrdup(argv[i]);
> > > +
> > > +        char *key, *value;
> > > +        char *policy = policies[i];
> > > +        if (!ofputil_parse_key_value(&policy, &key, &value)) {
> > > +            goto done;
> > > +        }
> > > +        key_timeouts[i] = key;
> > > +        value_timeouts[i] = atoi(value);
> > > +        simap_put(&new_tp, key, (unsigned int)value_timeouts[i]);
> > > +    }
> > > +
> > > +done:
> > > +    tp_table = ovsrec_ct_timeout_policy_table_get(ctx->idl);
> > > +    OVSREC_CT_TIMEOUT_POLICY_TABLE_FOR_EACH (row, tp_table) {
> > > +        struct simap s = SIMAP_INITIALIZER(&s);
> > > +
> > > +        /* Convert to simap. */
> > > +        for (int i = 0; i < row->n_timeouts; i++) {
> > > +            simap_put(&s, row->key_timeouts[i],
> row->value_timeouts[i]);
> > > +        }
> > > +
> > > +        if (simap_equal(&s, &new_tp)) {
> > > +            tp = CONST_CAST(struct ovsrec_ct_timeout_policy *, row);
> > > +            simap_destroy(&s);
> > > +            break;
> > > +        }
> > > +        simap_destroy(&s);
> > > +    }
> > > +
> > > +    if (!tp) {
> > > +        tp = ovsrec_ct_timeout_policy_insert(ctx->txn);
> > > +        ovsrec_ct_timeout_policy_set_timeouts(tp, key_timeouts,
> > > +                                              (const int64_t
> > > *)value_timeouts,
> > > +                                              n_tps);
> > > +    }
> > > +
> > > +    for (int i = 0; i < n_tps; i++) {
> > > +        free(policies[i]);
> > > +    }
> > > +    free(policies);
> > > +    simap_destroy(&new_tp);
> > > +    free(key_timeouts);
> > > +    free(value_timeouts);
> > > +    return tp;
> > > +}
> > > +
> > > +static void
> > > +cmd_add_zone_tp(struct ctl_context *ctx)
> > > +{
> > > +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> > > +    struct ovsrec_ct_timeout_policy *tp;
> > > +    int64_t zone_id;
> > > +
> > > +    const char *dp_name = ctx->argv[1];
> > > +    ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
> > > +    bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
> > > +
> > > +    struct ovsrec_datapath *dp = find_datapath(vsctl_ctx, dp_name);
> > > +    if (!dp) {
> > > +        ctl_fatal("datapath %s does not exist", dp_name);
> > > +    }
> > > +
> > > +    int n_tps = ctx->argc - 3;
> > > +    struct ovsrec_ct_zone *zone = find_ct_zone(dp, zone_id);
> > > +
> > > +    if (zone && !may_exist) {
> > > +        ctl_fatal("zone id %"PRIu64" already exists", zone_id);
> > > +    }
> > > +
> > > +    tp = create_timeout_policy(ctx, &ctx->argv[3], n_tps);
> > > +    if (zone) {
> > > +        ovsrec_ct_zone_set_timeout_policy(zone, tp);
> > > +    } else {
> > > +        zone = ovsrec_ct_zone_insert(ctx->txn);
> > > +        ovsrec_ct_zone_set_timeout_policy(zone, tp);
> > > +        ovsrec_datapath_update_ct_zones_setkey(dp, zone_id, zone);
> > > +    }
> > > +}
> > > +
> > > +static void
> > > +cmd_del_zone_tp(struct ctl_context *ctx)
> > > +{
> > > +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> > > +    int64_t zone_id;
> > > +
> > > +    bool must_exist = !shash_find(&ctx->options, "--if-exists");
> > > +    const char *dp_name = ctx->argv[1];
> > > +    ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
> > > +
> > > +    struct ovsrec_datapath *dp = find_datapath(vsctl_ctx, dp_name);
> > > +    if (!dp) {
> > > +        ctl_fatal("datapath %s does not exist", dp_name);
> > > +    }
> > > +
> > > +    struct ovsrec_ct_zone *zone = find_ct_zone(dp, zone_id);
> > > +    if (must_exist && !zone) {
> > > +        ctl_fatal("zone id %"PRIu64" does not exist", zone_id);
> > > +    }
> > > +
> > > +    if (zone) {
> > > +        ovsrec_datapath_update_ct_zones_delkey(dp, zone_id);
> > > +    }
> > > +}
> > > +
> > > +static void
> > > +cmd_list_zone_tp(struct ctl_context *ctx)
> > > +{
> > > +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> > > +
> > > +    struct ovsrec_datapath *dp = find_datapath(vsctl_ctx,
> ctx->argv[1]);
> > > +    if (!dp) {
> > > +        ctl_fatal("datapath: %s record not found", ctx->argv[1]);
> > > +    }
> > > +
> > > +    for (int i = 0; i < dp->n_ct_zones; i++) {
> > > +        struct ovsrec_ct_zone *zone = dp->value_ct_zones[i];
> > > +        ds_put_format(&ctx->output, "Zone:%"PRIu64", Timeout
> Policies: ",
> > > +                      dp->key_ct_zones[i]);
> > > +
> > > +        struct ovsrec_ct_timeout_policy *tp = zone->timeout_policy;
> > > +
> > > +        int j;
> > >
> > remove
> >
> >
> >
> > > +        for (j = 0; j < tp->n_timeouts; j++) {
> > >
> >
> >
> > move into for loop
> > +        for (int j = 0; j < tp->n_timeouts; j++) {
> >
> >
> > > +            if (j == tp->n_timeouts - 1) {
> > > +                ds_put_format(&ctx->output, "%s=%"PRIu64"\n",
> > > +                          tp->key_timeouts[j], tp->value_timeouts[j]);
> > > +            } else {
> > > +                ds_put_format(&ctx->output, "%s=%"PRIu64" ",
> > > +                          tp->key_timeouts[j], tp->value_timeouts[j]);
> > > +            }
> > > +        }
> > > +    }
> > > +}
> > > +
> > > +static void
> > > +pre_get_zone(struct ctl_context *ctx)
> > > +{
> > > +    ovsdb_idl_add_column(ctx->idl,
> &ovsrec_open_vswitch_col_datapaths);
> > > +    ovsdb_idl_add_column(ctx->idl, &ovsrec_datapath_col_ct_zones);
> > > +    ovsdb_idl_add_column(ctx->idl,
> &ovsrec_ct_zone_col_timeout_policy);
> > > +    ovsdb_idl_add_column(ctx->idl,
> > > &ovsrec_ct_timeout_policy_col_timeouts);
> > > +}
> > > +
> > >  static void
> > >  cmd_add_br(struct ctl_context *ctx)
> > >  {
> > > @@ -2896,6 +3083,13 @@ static const struct ctl_command_syntax
> > > vsctl_commands[] = {
> > >      /* Switch commands. */
> > >      {"emer-reset", 0, 0, "", pre_cmd_emer_reset, cmd_emer_reset, NULL,
> > > "", RW},
> > >
> > > +    /* Zone and CT Timeout Policy commands. */
> > > +    {"add-zone-tp", 2, 19, "", pre_get_zone, cmd_add_zone_tp, NULL,
> > >
> >
> > s/19/18/ ?
>
> good catch, thanks
> William
>
> >
> >
> > > +     "--may-exist", RW},
> > > +    {"del-zone-tp", 2, 2, "", pre_get_zone, cmd_del_zone_tp, NULL,
> > > +     "--if-exists", RW},
> > > +    {"list-zone-tp", 1, 1, "", pre_get_zone, cmd_list_zone_tp, NULL,
> "",
> > > RO},
> > > +
> > >      {NULL, 0, 0, NULL, NULL, NULL, NULL, NULL, RO},
> > >  };
> > >
> > > --
> > > 2.7.4
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>