> On Aug 28, 2019, at 3:14 PM, Yi-Hung Wei <yihung....@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}, _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev