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....@gmail.com> wrote: > > > > From: William Tu <u9012...@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 <u9012...@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 <jpet...@ovn.org> > > Thanks, > > --Justin > Thanks for the review. I will apply your diff and test. Regards, William _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev