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

[William Tu]

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 at gmail.com> wrote:
> > 
> > From: William Tu <u9012063 at 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 at 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 at ovn.org>
> 
> Thanks,
> 
> --Justin
> 

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

Regards,
William