[ovs-dev,v2,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 zone=zone_id ...

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

> On Aug 1, 2019, at 3:07 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 zone=zone_id ...

I think the command also requires a datapath argument.

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

> diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
> index 7c09df79bd29..0925d4d97b39 100644
> --- a/utilities/ovs-vsctl.8.in
> +++ b/utilities/ovs-vsctl.8.in
> @@ -353,6 +353,31 @@ 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 "\fBadd\-zone\-tp \fIdatapath \fBzone=\fIzone_id \fIpolicies\fR"
> +Creates a conntrack zone with \fIzone_id\fR under the datapath \fIdatapath\fR.
> +Associate the conntrack timeout policies to it by a list of
> +\fIkey\fB=\fIvalue\fR pairs, separeted by space.  For example, specifying
> +30-second timeout policy for first icmp packet, and 60-second for icmp reply packet
> +by doing \fBicmp_first=30 icmp_reply=60\fR.  See CT_Timeout_Policy TABLE
> +at \fBovs-vswitchd.conf.db\fR(5) for all available configurations.
> +.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 "\fBdel\-zone\-tp \fIdatapath \fBzone=\fIzone_id\fR"
> +Delete a zone under \fIdatapath\fR by specifying its zone ID.
> +.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.

I think "--may-exist" and "--if-exists" should be included in the line describing the command.  I have a few other suggested changes in the attached patch.

> diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> index 4948137efe8c..16578edfc331 100644
> --- a/utilities/ovs-vsctl.c
> +++ b/utilities/ovs-vsctl.c
> 
> +static struct ovsrec_ct_timeout_policy *
> +create_timeout_policy(struct ctl_context *ctx, char **argv, int n_tps)
> +{
> +    const struct ovsrec_ct_timeout_policy_table *tp_table;
> +    const struct ovsrec_ct_timeout_policy *row;
> +    struct ovsrec_ct_timeout_policy *tp = NULL;
> +    const char **key_timeouts;
> +    int64_t *value_timeouts;
> +    struct simap new_tp, s;
> +    int i, j;

I think 'i' and 'j' can be declared in the for loop definitions (and you don't actually need 'j' since it's not nested).

> +    /* Parse timeout arguments. */
> +    for (i = 0; i < n_tps; i++) {
> +        char *key, *value, *pos, *copy;

'copy' doesn't appear to be used.

> +
> +        pos = copy = xstrdup(argv[i]);

I think 'pos' is leaked.  I had to go through some contortions to make it work without modifying 'argv'; let me know if you think the logic in the attached diff is correct.

> +    free(key_timeouts);
> +    free(value_timeouts);
> +    return tp;

I think you also need to destroy 'new_tp'.

> +static void
> +cmd_add_zone(struct ctl_context *ctx)

I think these functions might be more accurately described with "_tp" at the end.

> +{
> +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> +    struct ovsrec_ct_timeout_policy *tp;
> +    struct ovsrec_ct_zone *zone;
> +    struct ovsrec_datapath *dp;
> +    const char *dp_name;
> +    int64_t zone_id;
> +    bool may_exist;
> +    int n_tps;

Minor style nit, but we usually declare the variables closer to their use now.  I've updated some of them in the attached diff.

> +    dp_name = ctx->argv[1];
> +    ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
> +    may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
> +
> +    dp = find_datapath(vsctl_ctx, dp_name);
> +    if (!dp) {
> +        ctl_fatal("datapath: %s record not found", dp_name);

This error message is a bit out of style, the other code usually is more like "datapath %s does not exit".

> +        return;

I don't think you need to return after a call to ctl_fatal().

> +    zone = find_ct_zone(dp, zone_id);
> +    if (zone) {
> +        if (!may_exist) {
> +            ctl_fatal("zone id %"PRIu64" alread exists", zone_id);

s/alread/already/

> +static void
> +cmd_del_zone(struct ctl_context *ctx)
> +{
> +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> +    struct ovsrec_ct_zone *zone;
> +    struct ovsrec_datapath *dp;
> +    const char *dp_name;
> +    int64_t zone_id;
> +    bool must_exist;
> +
> +    must_exist = !shash_find(&ctx->options, "--if-exists");
> +    dp_name = ctx->argv[1];
> +    ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
> +
> +    dp = find_datapath(vsctl_ctx, dp_name);
> +    if (!dp) {
> +        ctl_fatal("datapath: %s record not found.", dp_name);

There's some inconsistency about whether the errors have a period.  I'd suggest removing them, since I think that's more common in the ovs-vsctl error messages.

> +        return;
> +    }
> +
> +    zone = find_ct_zone(dp, zone_id);
> +    if (must_exist && !zone) {
> +        ctl_fatal("zone id %"PRIu64" not exists.", zone_id);

s/not exists/does not exist/

> +static void
> +cmd_list_zone(struct ctl_context *ctx)
> +{
> ...
> +    struct ovsrec_ct_timeout_policy *tp;
> +    struct ovsrec_ct_zone *zone;
> ...
> +    int i, j;

These could all be declared in a tighter scope.

Assuming you agree with my proposed changes:

Acked-by: Justin Pettit <jpettit@ovn.org>

Thanks,

--Justin


-=-=-=-=-=-=-=-=-=-=-

diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
index f0c5975edd0e..df15fb6901a0 100644
--- a/tests/ovs-vsctl.at
+++ b/tests/ovs-vsctl.at
@@ -946,16 +946,16 @@ AT_CHECK([RUN_OVS_VSCTL([clear bridge br1 name])],
 
 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 netdevxx zone=1 icmp_first=1 icmp_reply=2])],
-  [1], [], [ovs-vsctl: datapath: netdevxx record not found
+  [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 alread exists
+  [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 not exists.
+  [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
 ])
diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
index 0925d4d97b39..5b9883ae1c3d 100644
--- a/utilities/ovs-vsctl.8.in
+++ b/utilities/ovs-vsctl.8.in
@@ -356,27 +356,28 @@ output.
 .SS "Conntrack Zone Commands"
 These commands query and modify datapath CT zones and Timeout Policies.
 .
-.IP "\fBadd\-zone\-tp \fIdatapath \fBzone=\fIzone_id \fIpolicies\fR"
-Creates a conntrack zone with \fIzone_id\fR under the datapath \fIdatapath\fR.
-Associate the conntrack timeout policies to it by a list of
-\fIkey\fB=\fIvalue\fR pairs, separeted by space.  For example, specifying
-30-second timeout policy for first icmp packet, and 60-second for icmp reply packet
-by doing \fBicmp_first=30 icmp_reply=60\fR.  See CT_Timeout_Policy TABLE
-at \fBovs-vswitchd.conf.db\fR(5) for all available configurations.
+.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 "\fBdel\-zone\-tp \fIdatapath \fBzone=\fIzone_id\fR"
-Delete a zone under \fIdatapath\fR by specifying its zone ID.
+.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 under the \fIdatapath\fR.
+Prints the timeout policies of all zones in \fIdatapath\fR.
 .
 .SS "OpenFlow Controller Connectivity"
 .
diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
index 16578edfc331..059e45dc5438 100644
--- a/utilities/ovs-vsctl.c
+++ b/utilities/ovs-vsctl.c
@@ -1188,36 +1188,34 @@ create_timeout_policy(struct ctl_context *ctx, char **argv, int n_tps)
     const struct ovsrec_ct_timeout_policy_table *tp_table;
     const struct ovsrec_ct_timeout_policy *row;
     struct ovsrec_ct_timeout_policy *tp = NULL;
-    const char **key_timeouts;
-    int64_t *value_timeouts;
-    struct simap new_tp, s;
-    int i, j;
+    struct simap new_tp = SIMAP_INITIALIZER(&new_tp);
 
-    simap_init(&new_tp);
-
-    key_timeouts = xmalloc(sizeof *key_timeouts * n_tps);
-    value_timeouts = xmalloc(sizeof *value_timeouts * n_tps);
+    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 (i = 0; i < n_tps; i++) {
-        char *key, *value, *pos, *copy;
+    for (int i = 0; i < n_tps; i++) {
+        policies[i] = xstrdup(argv[i]);
 
-        pos = copy = xstrdup(argv[i]);
-        if (!ofputil_parse_key_value(&pos, &key, &value)) {
+        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:
 
+done:
     tp_table = ovsrec_ct_timeout_policy_table_get(ctx->idl);
     OVSREC_CT_TIMEOUT_POLICY_TABLE_FOR_EACH (row, tp_table) {
-        simap_init(&s);
-        /* Covert to simap. */
-        for (j = 0; j < row->n_timeouts; j++) {
-            simap_put(&s, row->key_timeouts[j], row->value_timeouts[j]);
+        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)) {
@@ -1235,41 +1233,39 @@ done:
                                               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(struct ctl_context *ctx)
+cmd_add_zone_tp(struct ctl_context *ctx)
 {
     struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
     struct ovsrec_ct_timeout_policy *tp;
-    struct ovsrec_ct_zone *zone;
-    struct ovsrec_datapath *dp;
-    const char *dp_name;
     int64_t zone_id;
-    bool may_exist;
-    int n_tps;
 
-    dp_name = ctx->argv[1];
+    const char *dp_name = ctx->argv[1];
     ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
-    may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
+    bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
 
-    dp = find_datapath(vsctl_ctx, dp_name);
+    struct ovsrec_datapath *dp = find_datapath(vsctl_ctx, dp_name);
     if (!dp) {
-        ctl_fatal("datapath: %s record not found", dp_name);
-        return;
+        ctl_fatal("datapath %s does not exist", dp_name);
     }
 
-    n_tps = ctx->argc - 3;
+    int n_tps = ctx->argc - 3;
     tp = create_timeout_policy(ctx, &ctx->argv[3], n_tps);
 
-    zone = find_ct_zone(dp, zone_id);
+    struct ovsrec_ct_zone *zone = find_ct_zone(dp, zone_id);
     if (zone) {
         if (!may_exist) {
-            ctl_fatal("zone id %"PRIu64" alread exists", zone_id);
-            return;
+            ctl_fatal("zone id %"PRIu64" already exists", zone_id);
         }
         ovsrec_ct_zone_set_timeout_policy(zone, tp);
     } else {
@@ -1280,29 +1276,23 @@ cmd_add_zone(struct ctl_context *ctx)
 }
 
 static void
-cmd_del_zone(struct ctl_context *ctx)
+cmd_del_zone_tp(struct ctl_context *ctx)
 {
     struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
-    struct ovsrec_ct_zone *zone;
-    struct ovsrec_datapath *dp;
-    const char *dp_name;
     int64_t zone_id;
-    bool must_exist;
 
-    must_exist = !shash_find(&ctx->options, "--if-exists");
-    dp_name = ctx->argv[1];
+    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);
 
-    dp = find_datapath(vsctl_ctx, dp_name);
+    struct ovsrec_datapath *dp = find_datapath(vsctl_ctx, dp_name);
     if (!dp) {
-        ctl_fatal("datapath: %s record not found.", dp_name);
-        return;
+        ctl_fatal("datapath %s does not exist", dp_name);
     }
 
-    zone = find_ct_zone(dp, zone_id);
+    struct ovsrec_ct_zone *zone = find_ct_zone(dp, zone_id);
     if (must_exist && !zone) {
-        ctl_fatal("zone id %"PRIu64" not exists.", zone_id);
-        return;
+        ctl_fatal("zone id %"PRIu64" does not exist", zone_id);
     }
 
     if (zone) {
@@ -1311,31 +1301,28 @@ cmd_del_zone(struct ctl_context *ctx)
 }
 
 static void
-cmd_list_zone(struct ctl_context *ctx)
+cmd_list_zone_tp(struct ctl_context *ctx)
 {
     struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
-    struct ovsrec_ct_timeout_policy *tp;
-    struct ovsrec_ct_zone *zone;
-    struct ovsrec_datapath *dp;
-    int i, j;
 
-    dp = find_datapath(vsctl_ctx, ctx->argv[1]);
+    struct ovsrec_datapath *dp = find_datapath(vsctl_ctx, ctx->argv[1]);
     if (!dp) {
         ctl_fatal("datapath: %s record not found", ctx->argv[1]);
-        return;
     }
 
-    for (i = 0; i < dp->n_ct_zones; i++) {
-        zone = dp->value_ct_zones[i];
+    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]);
 
-        tp = zone->timeout_policy;
+        struct ovsrec_ct_timeout_policy *tp = zone->timeout_policy;
 
+        int j;
         for (j = 0; j < tp->n_timeouts - 1; j++) {
             ds_put_format(&ctx->output, "%s=%"PRIu64" ",
                           tp->key_timeouts[j], tp->value_timeouts[j]);
         }
+
         ds_put_format(&ctx->output, "%s=%"PRIu64"\n",
                       tp->key_timeouts[j], tp->value_timeouts[j]);
     }
@@ -3094,11 +3081,11 @@ static const struct ctl_command_syntax vsctl_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, NULL,
+    {"add-zone-tp", 2, 19, "", pre_get_zone, cmd_add_zone_tp, NULL,
      "--may-exist", RW},
-    {"del-zone-tp", 2, 2, "", pre_get_zone, cmd_del_zone, NULL,
+    {"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, NULL, "", RO},
+    {"list-zone-tp", 1, 1, "", pre_get_zone, cmd_list_zone_tp, NULL, "", RO},
 
     {NULL, 0, 0, NULL, NULL, NULL, NULL, NULL, RO},
 };

Thanks for the patch

I noticed '--may-exist' and '--if-exists' are supported now for
add--zone-tp/del-zone-tp - thanks
The check for duplicate timeout policies now correctly checks all key and
values - thanks

Some more comments inline
I am trying to avoid duplicate comment from Justin, so I just won't comment
on some parts in this version
to avoid confusion.


On Thu, Aug 1, 2019 at 3:09 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 zone=zone_id ...
>
> Signed-off-by: William Tu <u9012063@gmail.com>
> ---
>  tests/ovs-vsctl.at       |  34 +++++++-
>  utilities/ovs-vsctl.8.in |  25 ++++++
>  utilities/ovs-vsctl.c    | 204
> +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 261 insertions(+), 2 deletions(-)
>
> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> index 46fa3c5b1a33..f0c5975edd0e 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])
>

What happens if we add a datapath type here and there are no bridges of
that type; meaning the datapath of that type does not even exist.
Seems like a contradiction.
Maybe we should check for that at least and raise an error.
Ideally, it is better if these 'datapaths' are auto-managed by bridge
creation/deletion with given datapath types,
but we can certainly defer that.


> +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1
> icmp_reply=2])])
>

I mentioned this in V1:
There is no filtering of bad timeout key; for example

AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1
foo_bar=2])])

is accepted as valid

Even worse, a minor typo will go unnoticed - missing 'y' in 'icmp_reply'.

AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1
icmp_repl=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
>

I mentioned in V1
We should check all possible timeout keys to make sure they work.


> +])
> +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'])],
>

I mentioned in V1.
I don't think we should make unrelated changes in a feature patch
especially since it seems the author wanted to convey
short form syntax is valid


>    [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])
> +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdevxx zone=1 icmp_first=1
> icmp_reply=2])],
> +  [1], [], [ovs-vsctl: datapath: netdevxx record not found
> +])
> +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 alread 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 not exists.
> +])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout
> Policies: icmp_first=2 icmp_reply=3
>

Ideally, we should be able to list one zone, but I know I did not mention
that before, so lets defer that or not worry about it
altogether.


> +])
>  OVS_VSCTL_CLEANUP
>  AT_CLEANUP
>
> diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
> index 7c09df79bd29..0925d4d97b39 100644
> --- a/utilities/ovs-vsctl.8.in
> +++ b/utilities/ovs-vsctl.8.in
> @@ -353,6 +353,31 @@ 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 "\fBadd\-zone\-tp \fIdatapath \fBzone=\fIzone_id \fIpolicies\fR"
> +Creates a conntrack zone with \fIzone_id\fR under the datapath
> \fIdatapath\fR.
> +Associate the conntrack timeout policies to it by a list of
> +\fIkey\fB=\fIvalue\fR pairs, separeted by space.  For example, specifying
> +30-second timeout policy for first icmp packet, and 60-second for icmp
> reply packet
> +by doing \fBicmp_first=30 icmp_reply=60\fR.  See CT_Timeout_Policy TABLE
> +at \fBovs-vswitchd.conf.db\fR(5) for all available configurations.
> +.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 "\fBdel\-zone\-tp \fIdatapath \fBzone=\fIzone_id\fR"
> +Delete a zone under \fIdatapath\fR by specifying its zone ID.
> +.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 under the \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..16578edfc331 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 "openvswitch/vconn.h"
>  #include "openvswitch/vlog.h"
>
> @@ -1153,6 +1155,201 @@ 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;
> +
> +    for (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;
> +
> +    for (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)
> +{
> +    const struct ovsrec_ct_timeout_policy_table *tp_table;
> +    const struct ovsrec_ct_timeout_policy *row;
> +    struct ovsrec_ct_timeout_policy *tp = NULL;
> +    const char **key_timeouts;
> +    int64_t *value_timeouts;
> +    struct simap new_tp, s;
> +    int i, j;
> +
> +    simap_init(&new_tp);
> +
> +    key_timeouts = xmalloc(sizeof *key_timeouts * n_tps);
> +    value_timeouts = xmalloc(sizeof *value_timeouts * n_tps);
> +
> +    /* Parse timeout arguments. */
> +    for (i = 0; i < n_tps; i++) {
> +        char *key, *value, *pos, *copy;
> +
> +        pos = copy = xstrdup(argv[i]);
> +        if (!ofputil_parse_key_value(&pos, &key, &value)) {
> +            goto done;
> +        }
> +        key_timeouts[i] = key;
> +        value_timeouts[i] = atoi(value);
> +        simap_put(&new_tp, key, (unsigned int)value_timeouts[i]);
>

Be careful how you free the string to be parsed...
If still not fixed in V3, we can deal with it.


> +    }
> +done:
> +
> +    tp_table = ovsrec_ct_timeout_policy_table_get(ctx->idl);
> +    OVSREC_CT_TIMEOUT_POLICY_TABLE_FOR_EACH (row, tp_table) {
> +        simap_init(&s);
> +        /* Covert to simap. */
> +        for (j = 0; j < row->n_timeouts; j++) {
> +            simap_put(&s, row->key_timeouts[j], row->value_timeouts[j]);
> +        }
> +
> +        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);
> +    }
> +
> +    free(key_timeouts);
> +    free(value_timeouts);
> +    return tp;
> +}
> +
> +static void
> +cmd_add_zone(struct ctl_context *ctx)
> +{
> +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> +    struct ovsrec_ct_timeout_policy *tp;
> +    struct ovsrec_ct_zone *zone;
> +    struct ovsrec_datapath *dp;
> +    const char *dp_name;
> +    int64_t zone_id;
> +    bool may_exist;
> +    int n_tps;
> +
> +    dp_name = ctx->argv[1];
> +    ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
> +    may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
> +
> +    dp = find_datapath(vsctl_ctx, dp_name);
> +    if (!dp) {
> +        ctl_fatal("datapath: %s record not found", dp_name);
> +        return;
> +    }
> +
> +    n_tps = ctx->argc - 3;
> +    tp = create_timeout_policy(ctx, &ctx->argv[3], n_tps);
> +
> +    zone = find_ct_zone(dp, zone_id);
> +    if (zone) {
> +        if (!may_exist) {
> +            ctl_fatal("zone id %"PRIu64" alread exists", zone_id);
>

In this error case, we have already created a new timeout policy above,
here:

+    tp = create_timeout_policy(ctx, &ctx->argv[3], n_tps);

I do not think that is a good idea.
If it is an error, don't create anything.



> +            return;
> +        }
> +        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(struct ctl_context *ctx)
> +{
> +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> +    struct ovsrec_ct_zone *zone;
> +    struct ovsrec_datapath *dp;
> +    const char *dp_name;
> +    int64_t zone_id;
> +    bool must_exist;
> +
> +    must_exist = !shash_find(&ctx->options, "--if-exists");
> +    dp_name = ctx->argv[1];
> +    ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
> +
> +    dp = find_datapath(vsctl_ctx, dp_name);
> +    if (!dp) {
> +        ctl_fatal("datapath: %s record not found.", dp_name);
> +        return;
> +    }
> +
> +    zone = find_ct_zone(dp, zone_id);
> +    if (must_exist && !zone) {
> +        ctl_fatal("zone id %"PRIu64" not exists.", zone_id);
> +        return;
> +    }
> +
> +    if (zone) {
> +        ovsrec_datapath_update_ct_zones_delkey(dp, zone_id);
> +    }
> +}
> +
> +static void
> +cmd_list_zone(struct ctl_context *ctx)
> +{
> +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> +    struct ovsrec_ct_timeout_policy *tp;
> +    struct ovsrec_ct_zone *zone;
> +    struct ovsrec_datapath *dp;
> +    int i, j;
> +
> +    dp = find_datapath(vsctl_ctx, ctx->argv[1]);
> +    if (!dp) {
> +        ctl_fatal("datapath: %s record not found", ctx->argv[1]);
> +        return;
> +    }
> +
> +    for (i = 0; i < dp->n_ct_zones; i++) {
> +        zone = dp->value_ct_zones[i];
> +        ds_put_format(&ctx->output, "Zone:%"PRIu64", Timeout Policies: ",
> +                      dp->key_ct_zones[i]);
> +
> +        tp = zone->timeout_policy;
> +
> +        for (j = 0; j < tp->n_timeouts - 1; j++) {
> +            ds_put_format(&ctx->output, "%s=%"PRIu64" ",
> +                          tp->key_timeouts[j], tp->value_timeouts[j]);
> +        }
> +        ds_put_format(&ctx->output, "%s=%"PRIu64"\n",
> +                      tp->key_timeouts[j], tp->value_timeouts[j]);
>

What exactly are we printing above when the number of timeouts is zero ?

Maybe something like this:

        for (j = 0; j < tp->n_timeouts ; j++) {
            ds_put_format(&ctx->output, "%s=%"PRIu64" ",
                          tp->key_timeouts[j], tp->value_timeouts[j]);
        }
        ds_put_format(&ctx->output, "\n");

will be better.



> +    }
> +}
> +
> +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 +3093,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, NULL,
> +     "--may-exist", RW},
> +    {"del-zone-tp", 2, 2, "", pre_get_zone, cmd_del_zone, NULL,
> +     "--if-exists", RW},
> +    {"list-zone-tp", 1, 1, "", pre_get_zone, cmd_list_zone, 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
>

On Fri, Aug 02, 2019 at 05:42:16PM -0700, Justin Pettit wrote:
> 
> > On Aug 1, 2019, at 3:07 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 zone=zone_id ...
> 
> I think the command also requires a datapath argument.

Thanks, I will fix it in next version.

> 
> > Signed-off-by: William Tu <u9012063@gmail.com>
> > ---
> > tests/ovs-vsctl.at       |  34 +++++++-
> > utilities/ovs-vsctl.8.in |  25 ++++++
> > utilities/ovs-vsctl.c    | 204 +++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 261 insertions(+), 2 deletions(-)
> 
> > diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
> > index 7c09df79bd29..0925d4d97b39 100644
> > --- a/utilities/ovs-vsctl.8.in
> > +++ b/utilities/ovs-vsctl.8.in
> > @@ -353,6 +353,31 @@ 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 "\fBadd\-zone\-tp \fIdatapath \fBzone=\fIzone_id \fIpolicies\fR"
> > +Creates a conntrack zone with \fIzone_id\fR under the datapath \fIdatapath\fR.
> > +Associate the conntrack timeout policies to it by a list of
> > +\fIkey\fB=\fIvalue\fR pairs, separeted by space.  For example, specifying
> > +30-second timeout policy for first icmp packet, and 60-second for icmp reply packet
> > +by doing \fBicmp_first=30 icmp_reply=60\fR.  See CT_Timeout_Policy TABLE
> > +at \fBovs-vswitchd.conf.db\fR(5) for all available configurations.
> > +.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 "\fBdel\-zone\-tp \fIdatapath \fBzone=\fIzone_id\fR"
> > +Delete a zone under \fIdatapath\fR by specifying its zone ID.
> > +.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.
> 
> I think "--may-exist" and "--if-exists" should be included in the line describing the command.  I have a few other suggested changes in the attached patch.
> 
> > diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> > index 4948137efe8c..16578edfc331 100644
> > --- a/utilities/ovs-vsctl.c
> > +++ b/utilities/ovs-vsctl.c
> > 
> > +static struct ovsrec_ct_timeout_policy *
> > +create_timeout_policy(struct ctl_context *ctx, char **argv, int n_tps)
> > +{
> > +    const struct ovsrec_ct_timeout_policy_table *tp_table;
> > +    const struct ovsrec_ct_timeout_policy *row;
> > +    struct ovsrec_ct_timeout_policy *tp = NULL;
> > +    const char **key_timeouts;
> > +    int64_t *value_timeouts;
> > +    struct simap new_tp, s;
> > +    int i, j;
> 
> I think 'i' and 'j' can be declared in the for loop definitions (and you don't actually need 'j' since it's not nested).
> 
> > +    /* Parse timeout arguments. */
> > +    for (i = 0; i < n_tps; i++) {
> > +        char *key, *value, *pos, *copy;
> 
> 'copy' doesn't appear to be used.
> 
> > +
> > +        pos = copy = xstrdup(argv[i]);
> 
> I think 'pos' is leaked.  I had to go through some contortions to make it work without modifying 'argv'; let me know if you think the logic in the attached diff is correct.
> 
> > +    free(key_timeouts);
> > +    free(value_timeouts);
> > +    return tp;
> 
> I think you also need to destroy 'new_tp'.
> 
> > +static void
> > +cmd_add_zone(struct ctl_context *ctx)
> 
> I think these functions might be more accurately described with "_tp" at the end.
> 
> > +{
> > +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> > +    struct ovsrec_ct_timeout_policy *tp;
> > +    struct ovsrec_ct_zone *zone;
> > +    struct ovsrec_datapath *dp;
> > +    const char *dp_name;
> > +    int64_t zone_id;
> > +    bool may_exist;
> > +    int n_tps;
> 
> Minor style nit, but we usually declare the variables closer to their use now.  I've updated some of them in the attached diff.
> 
> > +    dp_name = ctx->argv[1];
> > +    ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
> > +    may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
> > +
> > +    dp = find_datapath(vsctl_ctx, dp_name);
> > +    if (!dp) {
> > +        ctl_fatal("datapath: %s record not found", dp_name);
> 
> This error message is a bit out of style, the other code usually is more like "datapath %s does not exit".
> 
> > +        return;
> 
> I don't think you need to return after a call to ctl_fatal().
> 
> > +    zone = find_ct_zone(dp, zone_id);
> > +    if (zone) {
> > +        if (!may_exist) {
> > +            ctl_fatal("zone id %"PRIu64" alread exists", zone_id);
> 
> s/alread/already/
> 
> > +static void
> > +cmd_del_zone(struct ctl_context *ctx)
> > +{
> > +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> > +    struct ovsrec_ct_zone *zone;
> > +    struct ovsrec_datapath *dp;
> > +    const char *dp_name;
> > +    int64_t zone_id;
> > +    bool must_exist;
> > +
> > +    must_exist = !shash_find(&ctx->options, "--if-exists");
> > +    dp_name = ctx->argv[1];
> > +    ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
> > +
> > +    dp = find_datapath(vsctl_ctx, dp_name);
> > +    if (!dp) {
> > +        ctl_fatal("datapath: %s record not found.", dp_name);
> 
> There's some inconsistency about whether the errors have a period.  I'd suggest removing them, since I think that's more common in the ovs-vsctl error messages.
> 
> > +        return;
> > +    }
> > +
> > +    zone = find_ct_zone(dp, zone_id);
> > +    if (must_exist && !zone) {
> > +        ctl_fatal("zone id %"PRIu64" not exists.", zone_id);
> 
> s/not exists/does not exist/
> 
> > +static void
> > +cmd_list_zone(struct ctl_context *ctx)
> > +{
> > ...
> > +    struct ovsrec_ct_timeout_policy *tp;
> > +    struct ovsrec_ct_zone *zone;
> > ...
> > +    int i, j;
> 
> These could all be declared in a tighter scope.
> 
> Assuming you agree with my proposed changes:
> 
> Acked-by: Justin Pettit <jpettit@ovn.org>
> 
> Thanks,
> 
> --Justin
> 

Thanks for the review. I will apply your diff and test.

Regards,
William

Thanks for the review.

On Mon, Aug 05, 2019 at 04:12:02PM -0700, Darrell Ball wrote:
> Thanks for the patch
> 
> I noticed '--may-exist' and '--if-exists' are supported now for
> add--zone-tp/del-zone-tp - thanks
> The check for duplicate timeout policies now correctly checks all key and
> values - thanks

yes, thanks. Will do it in next version.
> 
> Some more comments inline
> I am trying to avoid duplicate comment from Justin, so I just won't comment
> on some parts in this version
> to avoid confusion.
> 
> 
> On Thu, Aug 1, 2019 at 3:09 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 zone=zone_id ...
> >
> > Signed-off-by: William Tu <u9012063@gmail.com>
> > ---
> >  tests/ovs-vsctl.at       |  34 +++++++-
> >  utilities/ovs-vsctl.8.in |  25 ++++++
> >  utilities/ovs-vsctl.c    | 204
> > +++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 261 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> > index 46fa3c5b1a33..f0c5975edd0e 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])
> >
> 
> What happens if we add a datapath type here and there are no bridges of
> that type; meaning the datapath of that type does not even exist.
> Seems like a contradiction.
> Maybe we should check for that at least and raise an error.
> Ideally, it is better if these 'datapaths' are auto-managed by bridge
> creation/deletion with given datapath types,
> but we can certainly defer that.
> 
> 
> > +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1
> > icmp_reply=2])])
> >
> 
> I mentioned this in V1:
> There is no filtering of bad timeout key; for example
> 
> AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1
> foo_bar=2])])
> 
> is accepted as valid
> 
> Even worse, a minor typo will go unnoticed - missing 'y' in 'icmp_reply'.
> 
> AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1
> icmp_repl=2])])

agree. 
> 
> 
> > +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
> >
> 
> I mentioned in V1
> We should check all possible timeout keys to make sure they work.

OK.
> 
> 
> > +])
> > +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'])],
> >
> 
> I mentioned in V1.
> I don't think we should make unrelated changes in a feature patch
> especially since it seems the author wanted to convey
> short form syntax is valid

This has to be there, otherwise test fails.
> 
> 
> >    [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])
> > +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdevxx zone=1 icmp_first=1
> > icmp_reply=2])],
> > +  [1], [], [ovs-vsctl: datapath: netdevxx record not found
> > +])
> > +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 alread 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 not exists.
> > +])
> > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout
> > Policies: icmp_first=2 icmp_reply=3
> >
> 
> Ideally, we should be able to list one zone, but I know I did not mention
> that before, so lets defer that or not worry about it
> altogether.
> 
> 
> > +])
> >  OVS_VSCTL_CLEANUP
> >  AT_CLEANUP
> >
> > diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
> > index 7c09df79bd29..0925d4d97b39 100644
> > --- a/utilities/ovs-vsctl.8.in
> > +++ b/utilities/ovs-vsctl.8.in
> > @@ -353,6 +353,31 @@ 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 "\fBadd\-zone\-tp \fIdatapath \fBzone=\fIzone_id \fIpolicies\fR"
> > +Creates a conntrack zone with \fIzone_id\fR under the datapath
> > \fIdatapath\fR.
> > +Associate the conntrack timeout policies to it by a list of
> > +\fIkey\fB=\fIvalue\fR pairs, separeted by space.  For example, specifying
> > +30-second timeout policy for first icmp packet, and 60-second for icmp
> > reply packet
> > +by doing \fBicmp_first=30 icmp_reply=60\fR.  See CT_Timeout_Policy TABLE
> > +at \fBovs-vswitchd.conf.db\fR(5) for all available configurations.
> > +.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 "\fBdel\-zone\-tp \fIdatapath \fBzone=\fIzone_id\fR"
> > +Delete a zone under \fIdatapath\fR by specifying its zone ID.
> > +.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 under the \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..16578edfc331 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 "openvswitch/vconn.h"
> >  #include "openvswitch/vlog.h"
> >
> > @@ -1153,6 +1155,201 @@ 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;
> > +
> > +    for (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;
> > +
> > +    for (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)
> > +{
> > +    const struct ovsrec_ct_timeout_policy_table *tp_table;
> > +    const struct ovsrec_ct_timeout_policy *row;
> > +    struct ovsrec_ct_timeout_policy *tp = NULL;
> > +    const char **key_timeouts;
> > +    int64_t *value_timeouts;
> > +    struct simap new_tp, s;
> > +    int i, j;
> > +
> > +    simap_init(&new_tp);
> > +
> > +    key_timeouts = xmalloc(sizeof *key_timeouts * n_tps);
> > +    value_timeouts = xmalloc(sizeof *value_timeouts * n_tps);
> > +
> > +    /* Parse timeout arguments. */
> > +    for (i = 0; i < n_tps; i++) {
> > +        char *key, *value, *pos, *copy;
> > +
> > +        pos = copy = xstrdup(argv[i]);
> > +        if (!ofputil_parse_key_value(&pos, &key, &value)) {
> > +            goto done;
> > +        }
> > +        key_timeouts[i] = key;
> > +        value_timeouts[i] = atoi(value);
> > +        simap_put(&new_tp, key, (unsigned int)value_timeouts[i]);
> >
> 
> Be careful how you free the string to be parsed...
> If still not fixed in V3, we can deal with it.
> 
> 
> > +    }
> > +done:
> > +
> > +    tp_table = ovsrec_ct_timeout_policy_table_get(ctx->idl);
> > +    OVSREC_CT_TIMEOUT_POLICY_TABLE_FOR_EACH (row, tp_table) {
> > +        simap_init(&s);
> > +        /* Covert to simap. */
> > +        for (j = 0; j < row->n_timeouts; j++) {
> > +            simap_put(&s, row->key_timeouts[j], row->value_timeouts[j]);
> > +        }
> > +
> > +        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);
> > +    }
> > +
> > +    free(key_timeouts);
> > +    free(value_timeouts);
> > +    return tp;
> > +}
> > +
> > +static void
> > +cmd_add_zone(struct ctl_context *ctx)
> > +{
> > +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> > +    struct ovsrec_ct_timeout_policy *tp;
> > +    struct ovsrec_ct_zone *zone;
> > +    struct ovsrec_datapath *dp;
> > +    const char *dp_name;
> > +    int64_t zone_id;
> > +    bool may_exist;
> > +    int n_tps;
> > +
> > +    dp_name = ctx->argv[1];
> > +    ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
> > +    may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
> > +
> > +    dp = find_datapath(vsctl_ctx, dp_name);
> > +    if (!dp) {
> > +        ctl_fatal("datapath: %s record not found", dp_name);
> > +        return;
> > +    }
> > +
> > +    n_tps = ctx->argc - 3;
> > +    tp = create_timeout_policy(ctx, &ctx->argv[3], n_tps);
> > +
> > +    zone = find_ct_zone(dp, zone_id);
> > +    if (zone) {
> > +        if (!may_exist) {
> > +            ctl_fatal("zone id %"PRIu64" alread exists", zone_id);
> >
> 
> In this error case, we have already created a new timeout policy above,
> here:
> 
> +    tp = create_timeout_policy(ctx, &ctx->argv[3], n_tps);
> 
> I do not think that is a good idea.
> If it is an error, don't create anything.

OK I will fix it.
> 
> 
> 
> > +            return;
> > +        }
> > +        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(struct ctl_context *ctx)
> > +{
> > +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> > +    struct ovsrec_ct_zone *zone;
> > +    struct ovsrec_datapath *dp;
> > +    const char *dp_name;
> > +    int64_t zone_id;
> > +    bool must_exist;
> > +
> > +    must_exist = !shash_find(&ctx->options, "--if-exists");
> > +    dp_name = ctx->argv[1];
> > +    ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
> > +
> > +    dp = find_datapath(vsctl_ctx, dp_name);
> > +    if (!dp) {
> > +        ctl_fatal("datapath: %s record not found.", dp_name);
> > +        return;
> > +    }
> > +
> > +    zone = find_ct_zone(dp, zone_id);
> > +    if (must_exist && !zone) {
> > +        ctl_fatal("zone id %"PRIu64" not exists.", zone_id);
> > +        return;
> > +    }
> > +
> > +    if (zone) {
> > +        ovsrec_datapath_update_ct_zones_delkey(dp, zone_id);
> > +    }
> > +}
> > +
> > +static void
> > +cmd_list_zone(struct ctl_context *ctx)
> > +{
> > +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> > +    struct ovsrec_ct_timeout_policy *tp;
> > +    struct ovsrec_ct_zone *zone;
> > +    struct ovsrec_datapath *dp;
> > +    int i, j;
> > +
> > +    dp = find_datapath(vsctl_ctx, ctx->argv[1]);
> > +    if (!dp) {
> > +        ctl_fatal("datapath: %s record not found", ctx->argv[1]);
> > +        return;
> > +    }
> > +
> > +    for (i = 0; i < dp->n_ct_zones; i++) {
> > +        zone = dp->value_ct_zones[i];
> > +        ds_put_format(&ctx->output, "Zone:%"PRIu64", Timeout Policies: ",
> > +                      dp->key_ct_zones[i]);
> > +
> > +        tp = zone->timeout_policy;
> > +
> > +        for (j = 0; j < tp->n_timeouts - 1; j++) {
> > +            ds_put_format(&ctx->output, "%s=%"PRIu64" ",
> > +                          tp->key_timeouts[j], tp->value_timeouts[j]);
> > +        }
> > +        ds_put_format(&ctx->output, "%s=%"PRIu64"\n",
> > +                      tp->key_timeouts[j], tp->value_timeouts[j]);
> >
> 
> What exactly are we printing above when the number of timeouts is zero ?
> 
> Maybe something like this:
> 
>         for (j = 0; j < tp->n_timeouts ; j++) {
>             ds_put_format(&ctx->output, "%s=%"PRIu64" ",
>                           tp->key_timeouts[j], tp->value_timeouts[j]);
>         }
>         ds_put_format(&ctx->output, "\n");
> 
> will be better.

OK Will fix it.

Thank you for your review.
William
> 
> 
> 
> > +    }
> > +}
> > +
> > +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 +3093,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, NULL,
> > +     "--may-exist", RW},
> > +    {"del-zone-tp", 2, 2, "", pre_get_zone, cmd_del_zone, NULL,
> > +     "--if-exists", RW},
> > +    {"list-zone-tp", 1, 1, "", pre_get_zone, cmd_list_zone, 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 Wed, Aug 7, 2019 at 1:37 PM William Tu <u9012063@gmail.com> wrote:

> Thanks for the review.
>
> On Mon, Aug 05, 2019 at 04:12:02PM -0700, Darrell Ball wrote:
> > Thanks for the patch
> >
> > I noticed '--may-exist' and '--if-exists' are supported now for
> > add--zone-tp/del-zone-tp - thanks
> > The check for duplicate timeout policies now correctly checks all key and
> > values - thanks
>
> yes, thanks. Will do it in next version.
> >
> > Some more comments inline
> > I am trying to avoid duplicate comment from Justin, so I just won't
> comment
> > on some parts in this version
> > to avoid confusion.
> >
> >
> > On Thu, Aug 1, 2019 at 3:09 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 zone=zone_id ...
> > >
> > > Signed-off-by: William Tu <u9012063@gmail.com>
> > > ---
> > >  tests/ovs-vsctl.at       |  34 +++++++-
> > >  utilities/ovs-vsctl.8.in |  25 ++++++
> > >  utilities/ovs-vsctl.c    | 204
> > > +++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 261 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> > > index 46fa3c5b1a33..f0c5975edd0e 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])
> > >
> >
> > What happens if we add a datapath type here and there are no bridges of
> > that type; meaning the datapath of that type does not even exist.
> > Seems like a contradiction.
> > Maybe we should check for that at least and raise an error.
> > Ideally, it is better if these 'datapaths' are auto-managed by bridge
> > creation/deletion with given datapath types,
> > but we can certainly defer that.
> >
> >
> > > +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1
> > > icmp_reply=2])])
> > >
> >
> > I mentioned this in V1:
> > There is no filtering of bad timeout key; for example
> >
> > AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1
> > foo_bar=2])])
> >
> > is accepted as valid
> >
> > Even worse, a minor typo will go unnoticed - missing 'y' in 'icmp_reply'.
> >
> > AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1
> > icmp_repl=2])])
>
> agree.
> >
> >
> > > +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
> > >
> >
> > I mentioned in V1
> > We should check all possible timeout keys to make sure they work.
>
> OK.
> >
> >
> > > +])
> > > +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'])],
> > >
> >
> > I mentioned in V1.
> > I don't think we should make unrelated changes in a feature patch
> > especially since it seems the author wanted to convey
> > short form syntax is valid
>
> This has to be there, otherwise test fails.
>


yep, I got the obvious reason after running the negative test.

+ovs-vsctl: multiple table names match "c"




> >
> >
> > >    [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])
> > > +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdevxx zone=1 icmp_first=1
> > > icmp_reply=2])],
> > > +  [1], [], [ovs-vsctl: datapath: netdevxx record not found
> > > +])
> > > +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 alread 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 not exists.
> > > +])
> > > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout
> > > Policies: icmp_first=2 icmp_reply=3
> > >
> >
> > Ideally, we should be able to list one zone, but I know I did not mention
> > that before, so lets defer that or not worry about it
> > altogether.
> >
> >
> > > +])
> > >  OVS_VSCTL_CLEANUP
> > >  AT_CLEANUP
> > >
> > > diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
> > > index 7c09df79bd29..0925d4d97b39 100644
> > > --- a/utilities/ovs-vsctl.8.in
> > > +++ b/utilities/ovs-vsctl.8.in
> > > @@ -353,6 +353,31 @@ 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 "\fBadd\-zone\-tp \fIdatapath \fBzone=\fIzone_id \fIpolicies\fR"
> > > +Creates a conntrack zone with \fIzone_id\fR under the datapath
> > > \fIdatapath\fR.
> > > +Associate the conntrack timeout policies to it by a list of
> > > +\fIkey\fB=\fIvalue\fR pairs, separeted by space.  For example,
> specifying
> > > +30-second timeout policy for first icmp packet, and 60-second for icmp
> > > reply packet
> > > +by doing \fBicmp_first=30 icmp_reply=60\fR.  See CT_Timeout_Policy
> TABLE
> > > +at \fBovs-vswitchd.conf.db\fR(5) for all available configurations.
> > > +.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 "\fBdel\-zone\-tp \fIdatapath \fBzone=\fIzone_id\fR"
> > > +Delete a zone under \fIdatapath\fR by specifying its zone ID.
> > > +.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 under the \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..16578edfc331 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 "openvswitch/vconn.h"
> > >  #include "openvswitch/vlog.h"
> > >
> > > @@ -1153,6 +1155,201 @@ 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;
> > > +
> > > +    for (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;
> > > +
> > > +    for (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)
> > > +{
> > > +    const struct ovsrec_ct_timeout_policy_table *tp_table;
> > > +    const struct ovsrec_ct_timeout_policy *row;
> > > +    struct ovsrec_ct_timeout_policy *tp = NULL;
> > > +    const char **key_timeouts;
> > > +    int64_t *value_timeouts;
> > > +    struct simap new_tp, s;
> > > +    int i, j;
> > > +
> > > +    simap_init(&new_tp);
> > > +
> > > +    key_timeouts = xmalloc(sizeof *key_timeouts * n_tps);
> > > +    value_timeouts = xmalloc(sizeof *value_timeouts * n_tps);
> > > +
> > > +    /* Parse timeout arguments. */
> > > +    for (i = 0; i < n_tps; i++) {
> > > +        char *key, *value, *pos, *copy;
> > > +
> > > +        pos = copy = xstrdup(argv[i]);
> > > +        if (!ofputil_parse_key_value(&pos, &key, &value)) {
> > > +            goto done;
> > > +        }
> > > +        key_timeouts[i] = key;
> > > +        value_timeouts[i] = atoi(value);
> > > +        simap_put(&new_tp, key, (unsigned int)value_timeouts[i]);
> > >
> >
> > Be careful how you free the string to be parsed...
> > If still not fixed in V3, we can deal with it.
> >
> >
> > > +    }
> > > +done:
> > > +
> > > +    tp_table = ovsrec_ct_timeout_policy_table_get(ctx->idl);
> > > +    OVSREC_CT_TIMEOUT_POLICY_TABLE_FOR_EACH (row, tp_table) {
> > > +        simap_init(&s);
> > > +        /* Covert to simap. */
> > > +        for (j = 0; j < row->n_timeouts; j++) {
> > > +            simap_put(&s, row->key_timeouts[j],
> row->value_timeouts[j]);
> > > +        }
> > > +
> > > +        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);
> > > +    }
> > > +
> > > +    free(key_timeouts);
> > > +    free(value_timeouts);
> > > +    return tp;
> > > +}
> > > +
> > > +static void
> > > +cmd_add_zone(struct ctl_context *ctx)
> > > +{
> > > +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> > > +    struct ovsrec_ct_timeout_policy *tp;
> > > +    struct ovsrec_ct_zone *zone;
> > > +    struct ovsrec_datapath *dp;
> > > +    const char *dp_name;
> > > +    int64_t zone_id;
> > > +    bool may_exist;
> > > +    int n_tps;
> > > +
> > > +    dp_name = ctx->argv[1];
> > > +    ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
> > > +    may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
> > > +
> > > +    dp = find_datapath(vsctl_ctx, dp_name);
> > > +    if (!dp) {
> > > +        ctl_fatal("datapath: %s record not found", dp_name);
> > > +        return;
> > > +    }
> > > +
> > > +    n_tps = ctx->argc - 3;
> > > +    tp = create_timeout_policy(ctx, &ctx->argv[3], n_tps);
> > > +
> > > +    zone = find_ct_zone(dp, zone_id);
> > > +    if (zone) {
> > > +        if (!may_exist) {
> > > +            ctl_fatal("zone id %"PRIu64" alread exists", zone_id);
> > >
> >
> > In this error case, we have already created a new timeout policy above,
> > here:
> >
> > +    tp = create_timeout_policy(ctx, &ctx->argv[3], n_tps);
> >
> > I do not think that is a good idea.
> > If it is an error, don't create anything.
>
> OK I will fix it.
> >
> >
> >
> > > +            return;
> > > +        }
> > > +        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(struct ctl_context *ctx)
> > > +{
> > > +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> > > +    struct ovsrec_ct_zone *zone;
> > > +    struct ovsrec_datapath *dp;
> > > +    const char *dp_name;
> > > +    int64_t zone_id;
> > > +    bool must_exist;
> > > +
> > > +    must_exist = !shash_find(&ctx->options, "--if-exists");
> > > +    dp_name = ctx->argv[1];
> > > +    ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
> > > +
> > > +    dp = find_datapath(vsctl_ctx, dp_name);
> > > +    if (!dp) {
> > > +        ctl_fatal("datapath: %s record not found.", dp_name);
> > > +        return;
> > > +    }
> > > +
> > > +    zone = find_ct_zone(dp, zone_id);
> > > +    if (must_exist && !zone) {
> > > +        ctl_fatal("zone id %"PRIu64" not exists.", zone_id);
> > > +        return;
> > > +    }
> > > +
> > > +    if (zone) {
> > > +        ovsrec_datapath_update_ct_zones_delkey(dp, zone_id);
> > > +    }
> > > +}
> > > +
> > > +static void
> > > +cmd_list_zone(struct ctl_context *ctx)
> > > +{
> > > +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> > > +    struct ovsrec_ct_timeout_policy *tp;
> > > +    struct ovsrec_ct_zone *zone;
> > > +    struct ovsrec_datapath *dp;
> > > +    int i, j;
> > > +
> > > +    dp = find_datapath(vsctl_ctx, ctx->argv[1]);
> > > +    if (!dp) {
> > > +        ctl_fatal("datapath: %s record not found", ctx->argv[1]);
> > > +        return;
> > > +    }
> > > +
> > > +    for (i = 0; i < dp->n_ct_zones; i++) {
> > > +        zone = dp->value_ct_zones[i];
> > > +        ds_put_format(&ctx->output, "Zone:%"PRIu64", Timeout
> Policies: ",
> > > +                      dp->key_ct_zones[i]);
> > > +
> > > +        tp = zone->timeout_policy;
> > > +
> > > +        for (j = 0; j < tp->n_timeouts - 1; j++) {
> > > +            ds_put_format(&ctx->output, "%s=%"PRIu64" ",
> > > +                          tp->key_timeouts[j], tp->value_timeouts[j]);
> > > +        }
> > > +        ds_put_format(&ctx->output, "%s=%"PRIu64"\n",
> > > +                      tp->key_timeouts[j], tp->value_timeouts[j]);
> > >
> >
> > What exactly are we printing above when the number of timeouts is zero ?
> >
> > Maybe something like this:
> >
> >         for (j = 0; j < tp->n_timeouts ; j++) {
> >             ds_put_format(&ctx->output, "%s=%"PRIu64" ",
> >                           tp->key_timeouts[j], tp->value_timeouts[j]);
> >         }
> >         ds_put_format(&ctx->output, "\n");
> >
> > will be better.
>
> OK Will fix it.
>
> Thank you for your review.
> William
> >
> >
> >
> > > +    }
> > > +}
> > > +
> > > +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 +3093,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, NULL,
> > > +     "--may-exist", RW},
> > > +    {"del-zone-tp", 2, 2, "", pre_get_zone, cmd_del_zone, NULL,
> > > +     "--if-exists", RW},
> > > +    {"list-zone-tp", 1, 1, "", pre_get_zone, cmd_list_zone, 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
>