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

[William Tu]

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 <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.

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
> 
>