> 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. > 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 -=-=-=-=-=-=-=-=-=-=- diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at index f0c5975edd0e..df15fb6901a0 100644 --- a/tests/ovs-vsctl.at +++ b/tests/ovs-vsctl.at @@ -946,16 +946,16 @@ AT_CHECK([RUN_OVS_VSCTL([clear bridge br1 name])], AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath datapath_version=0 -- set Open_vSwitch . datapaths:"netdev"=@m])], [0], [stdout]) AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdevxx zone=1 icmp_first=1 icmp_reply=2])], - [1], [], [ovs-vsctl: datapath: netdevxx record not found + [1], [], [ovs-vsctl: datapath netdevxx does not exist ]) AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2 icmp_reply=3])]) AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2 icmp_reply=3])], - [1], [], [ovs-vsctl: zone id 2 alread exists + [1], [], [ovs-vsctl: zone id 2 already exists ]) AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout Policies: icmp_first=2 icmp_reply=3 ]) AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=11])], - [1], [], [ovs-vsctl: zone id 11 not exists. + [1], [], [ovs-vsctl: zone id 11 does not exist ]) AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout Policies: icmp_first=2 icmp_reply=3 ]) diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in index 0925d4d97b39..5b9883ae1c3d 100644 --- a/utilities/ovs-vsctl.8.in +++ b/utilities/ovs-vsctl.8.in @@ -356,27 +356,28 @@ 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 "[\fB\-\-may\-exist\fR] \fBadd\-zone\-tp \fIdatapath \fBzone=\fIzone_id \fIpolicies\fR" +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 +\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. . -.IP "\fBdel\-zone\-tp \fIdatapath \fBzone=\fIzone_id\fR" -Delete a zone under \fIdatapath\fR by specifying its zone ID. +.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. .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. . .IP "\fBlist\-zone\-tp \fIdatapath\fR" -Prints the timeout policies of all zones under the \fIdatapath\fR. +Prints the timeout policies of all zones in \fIdatapath\fR. . .SS "OpenFlow Controller Connectivity" . diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c index 16578edfc331..059e45dc5438 100644 --- a/utilities/ovs-vsctl.c +++ b/utilities/ovs-vsctl.c @@ -1188,36 +1188,34 @@ 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; + struct simap new_tp = SIMAP_INITIALIZER(&new_tp); - simap_init(&new_tp); - - key_timeouts = xmalloc(sizeof *key_timeouts * n_tps); - value_timeouts = xmalloc(sizeof *value_timeouts * n_tps); + char **policies = xzalloc(sizeof *policies * n_tps); + const char **key_timeouts = xmalloc(sizeof *key_timeouts * n_tps); + int64_t *value_timeouts = xmalloc(sizeof *value_timeouts * n_tps); /* Parse timeout arguments. */ - for (i = 0; i < n_tps; i++) { - char *key, *value, *pos, *copy; + for (int i = 0; i < n_tps; i++) { + policies[i] = xstrdup(argv[i]); - pos = copy = xstrdup(argv[i]); - if (!ofputil_parse_key_value(&pos, &key, &value)) { + char *key, *value; + char *policy = policies[i]; + if (!ofputil_parse_key_value(&policy, &key, &value)) { goto done; } key_timeouts[i] = key; value_timeouts[i] = atoi(value); simap_put(&new_tp, key, (unsigned int)value_timeouts[i]); } -done: +done: tp_table = ovsrec_ct_timeout_policy_table_get(ctx->idl); OVSREC_CT_TIMEOUT_POLICY_TABLE_FOR_EACH (row, tp_table) { - simap_init(&s); - /* Covert to simap. */ - for (j = 0; j < row->n_timeouts; j++) { - simap_put(&s, row->key_timeouts[j], row->value_timeouts[j]); + struct simap s = SIMAP_INITIALIZER(&s); + + /* Convert to simap. */ + for (int i = 0; i < row->n_timeouts; i++) { + simap_put(&s, row->key_timeouts[i], row->value_timeouts[i]); } if (simap_equal(&s, &new_tp)) { @@ -1235,41 +1233,39 @@ done: n_tps); } + for (int i = 0; i < n_tps; i++) { + free(policies[i]); + } + free(policies); + simap_destroy(&new_tp); free(key_timeouts); free(value_timeouts); return tp; } static void -cmd_add_zone(struct ctl_context *ctx) +cmd_add_zone_tp(struct ctl_context *ctx) { 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; - dp_name = ctx->argv[1]; + const char *dp_name = ctx->argv[1]; ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id); - may_exist = shash_find(&ctx->options, "--may-exist") != NULL; + bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL; - dp = find_datapath(vsctl_ctx, dp_name); + struct ovsrec_datapath *dp = find_datapath(vsctl_ctx, dp_name); if (!dp) { - ctl_fatal("datapath: %s record not found", dp_name); - return; + ctl_fatal("datapath %s does not exist", dp_name); } - n_tps = ctx->argc - 3; + int n_tps = ctx->argc - 3; tp = create_timeout_policy(ctx, &ctx->argv[3], n_tps); - zone = find_ct_zone(dp, zone_id); + struct ovsrec_ct_zone *zone = find_ct_zone(dp, zone_id); if (zone) { if (!may_exist) { - ctl_fatal("zone id %"PRIu64" alread exists", zone_id); - return; + ctl_fatal("zone id %"PRIu64" already exists", zone_id); } ovsrec_ct_zone_set_timeout_policy(zone, tp); } else { @@ -1280,29 +1276,23 @@ cmd_add_zone(struct ctl_context *ctx) } static void -cmd_del_zone(struct ctl_context *ctx) +cmd_del_zone_tp(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]; + bool must_exist = !shash_find(&ctx->options, "--if-exists"); + const char *dp_name = ctx->argv[1]; ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id); - dp = find_datapath(vsctl_ctx, dp_name); + struct ovsrec_datapath *dp = find_datapath(vsctl_ctx, dp_name); if (!dp) { - ctl_fatal("datapath: %s record not found.", dp_name); - return; + ctl_fatal("datapath %s does not exist", dp_name); } - zone = find_ct_zone(dp, zone_id); + struct ovsrec_ct_zone *zone = find_ct_zone(dp, zone_id); if (must_exist && !zone) { - ctl_fatal("zone id %"PRIu64" not exists.", zone_id); - return; + ctl_fatal("zone id %"PRIu64" does not exist", zone_id); } if (zone) { @@ -1311,31 +1301,28 @@ cmd_del_zone(struct ctl_context *ctx) } static void -cmd_list_zone(struct ctl_context *ctx) +cmd_list_zone_tp(struct ctl_context *ctx) { struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx); - struct ovsrec_ct_timeout_policy *tp; - struct ovsrec_ct_zone *zone; - struct ovsrec_datapath *dp; - int i, j; - dp = find_datapath(vsctl_ctx, ctx->argv[1]); + struct ovsrec_datapath *dp = find_datapath(vsctl_ctx, ctx->argv[1]); if (!dp) { ctl_fatal("datapath: %s record not found", ctx->argv[1]); - return; } - for (i = 0; i < dp->n_ct_zones; i++) { - zone = dp->value_ct_zones[i]; + for (int i = 0; i < dp->n_ct_zones; i++) { + struct ovsrec_ct_zone *zone = dp->value_ct_zones[i]; ds_put_format(&ctx->output, "Zone:%"PRIu64", Timeout Policies: ", dp->key_ct_zones[i]); - tp = zone->timeout_policy; + struct ovsrec_ct_timeout_policy *tp = zone->timeout_policy; + int j; for (j = 0; j < tp->n_timeouts - 1; j++) { ds_put_format(&ctx->output, "%s=%"PRIu64" ", tp->key_timeouts[j], tp->value_timeouts[j]); } + ds_put_format(&ctx->output, "%s=%"PRIu64"\n", tp->key_timeouts[j], tp->value_timeouts[j]); } @@ -3094,11 +3081,11 @@ 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, 19, "", pre_get_zone, cmd_add_zone, NULL, + {"add-zone-tp", 2, 19, "", pre_get_zone, cmd_add_zone_tp, NULL, "--may-exist", RW}, - {"del-zone-tp", 2, 2, "", pre_get_zone, cmd_del_zone, NULL, + {"del-zone-tp", 2, 2, "", pre_get_zone, cmd_del_zone_tp, NULL, "--if-exists", RW}, - {"list-zone-tp", 1, 1, "", pre_get_zone, cmd_list_zone, NULL, "", RO}, + {"list-zone-tp", 1, 1, "", pre_get_zone, cmd_list_zone_tp, NULL, "", RO}, {NULL, 0, 0, NULL, NULL, NULL, NULL, NULL, RO}, }; _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev