[ovs-dev] [PATCH v5 2/9] ovs-vsctl: Add conntrack zone commands.

[Justin Pettit]

> On Aug 28, 2019, at 3:14 PM, Yi-Hung Wei <yihung.wei at gmail.com> wrote:
> 
> 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"
> ...
> +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.

I don't think you need that final \fR.

I also made a few minor language tweaks in the icremental.

> diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> index 4948137efe8c..76a708bd5069 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)
> +{


I think it's clearer if we switch "argv" to "tps", since the argument should only be timeout policies.

> +static void
> +cmd_list_zone_tp(struct ctl_context *ctx)
> +{
> ...
> +        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]);
> +            }

I think this can be done without the duplicated code with a ds_chomp() and ds_put_char().  Let me know if you agree with the incremental patch at the bottom.

> 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, 18, "", pre_get_zone, cmd_add_zone_tp, NULL,
> +     "--may-exist", RW},

Is there a reason not to make the minimum arguments 3 instead of 2?  It seems like it's required in the code.

Is there a reason you limited this to 18 arguments and not use INT_MAX?

Let me know if you agree with the incremental.

Thanks,

--Justin


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

diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
index 5b9883ae1c3d..14a8aa4a48ac 100644
--- a/utilities/ovs-vsctl.8.in
+++ b/utilities/ovs-vsctl.8.in
@@ -361,13 +361,13 @@ 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
+packet and a 60-second policy for ICMP reply packets.  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.
+this command does nothing if \fIzone_id\fR already exists.
 .
 .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.
diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
index 76a708bd5069..43f64d4711f9 100644
--- a/utilities/ovs-vsctl.c
+++ b/utilities/ovs-vsctl.c
@@ -1180,7 +1180,7 @@ find_ct_zone(struct ovsrec_datapath *dp, const int64_t zone_id)
 }
 
 static struct ovsrec_ct_timeout_policy *
-create_timeout_policy(struct ctl_context *ctx, char **argv, int n_tps)
+create_timeout_policy(struct ctl_context *ctx, char **tps, int n_tps)
 {
     const struct ovsrec_ct_timeout_policy_table *tp_table;
     const struct ovsrec_ct_timeout_policy *row;
@@ -1193,7 +1193,7 @@ create_timeout_policy(struct ctl_context *ctx, char **argv, int n_tps)
 
     /* Parse timeout arguments. */
     for (int i = 0; i < n_tps; i++) {
-        policies[i] = xstrdup(argv[i]);
+        policies[i] = xstrdup(tps[i]);
 
         char *key, *value;
         char *policy = policies[i];
@@ -1320,14 +1320,11 @@ cmd_list_zone_tp(struct ctl_context *ctx)
         struct ovsrec_ct_timeout_policy *tp = zone->timeout_policy;
 
         for (int j = 0; j < tp->n_timeouts; j++) {
-            if (j == tp->n_timeouts - 1) {
-                ds_put_format(&ctx->output, "%s=%"PRIu64"\n",
+            ds_put_format(&ctx->output, "%s=%"PRIu64" ",
                           tp->key_timeouts[j], tp->value_timeouts[j]);
-            } else {
-                ds_put_format(&ctx->output, "%s=%"PRIu64" ",
-                          tp->key_timeouts[j], tp->value_timeouts[j]);
-            }
         }
+        ds_chomp(&ctx->output, ' ');
+        ds_put_char(&ctx->output, '\n');
     }
 }
 
@@ -3084,7 +3081,7 @@ 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, 18, "", pre_get_zone, cmd_add_zone_tp, NULL,
+    {"add-zone-tp", 3, INT_MAX, "", pre_get_zone, cmd_add_zone_tp, NULL,
      "--may-exist", RW},
     {"del-zone-tp", 2, 2, "", pre_get_zone, cmd_del_zone_tp, NULL,
      "--if-exists", RW},