Hi Justin, Thanks for your feedbacks.
On Thu, Sep 12, 2019 at 02:14:56PM -0700, Justin Pettit wrote: > > > On Aug 28, 2019, at 3:14 PM, Yi-Hung Wei <[email protected]> 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. OK > > > +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. OK, looks much better! > > > 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. OK, using 3 makes sure at least one policies is added. > > Is there a reason you limited this to 18 arguments and not use INT_MAX? I use 18 because at most we have 11 tcp, 3 udp, 2 icmp, total of 16 and plus (dp_name, zone_id), so total is 18. I think using INT_MAX is fine, because at db schema, the value type is set. I will merge your diff and send next version. Thanks William > > Let me know if you agree with the incremental. > > Thanks, > > --Justin > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
