Message ID | 1564697253-37992-3-git-send-email-yihung.wei@gmail.com |
---|---|
State | Superseded |
Headers | show |
Series | Support zone-based conntrack timeout policy | expand |
> On Aug 1, 2019, at 3:07 PM, Yi-Hung Wei <yihung.wei@gmail.com> wrote: > > From: William Tu <u9012063@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 <u9012063@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@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}, };
Thanks for the patch I noticed '--may-exist' and '--if-exists' are supported now for add--zone-tp/del-zone-tp - thanks The check for duplicate timeout policies now correctly checks all key and values - thanks Some more comments inline I am trying to avoid duplicate comment from Justin, so I just won't comment on some parts in this version to avoid confusion. On Thu, Aug 1, 2019 at 3:09 PM Yi-Hung Wei <yihung.wei@gmail.com> wrote: > From: William Tu <u9012063@gmail.com> > > The patch adds commands creating/deleting/listing conntrack zone > timeout policies: > $ ovs-vsctl {add,del,list}-zone-tp zone=zone_id ... > > Signed-off-by: William Tu <u9012063@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/tests/ovs-vsctl.at b/tests/ovs-vsctl.at > index 46fa3c5b1a33..f0c5975edd0e 100644 > --- a/tests/ovs-vsctl.at > +++ b/tests/ovs-vsctl.at > @@ -805,6 +805,20 @@ AT_CHECK( > [RUN_OVS_VSCTL([--if-exists remove netflow x targets '"1.2.3.4:567 > "'])]) > AT_CHECK( > [RUN_OVS_VSCTL([--if-exists clear netflow x targets])]) > + > +AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath datapath_version=0 -- > set Open_vSwitch . datapaths:"netdev"=@m])], [0], [stdout]) > What happens if we add a datapath type here and there are no bridges of that type; meaning the datapath of that type does not even exist. Seems like a contradiction. Maybe we should check for that at least and raise an error. Ideally, it is better if these 'datapaths' are auto-managed by bridge creation/deletion with given datapath types, but we can certainly defer that. > +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1 > icmp_reply=2])]) > I mentioned this in V1: There is no filtering of bad timeout key; for example AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1 foo_bar=2])]) is accepted as valid Even worse, a minor typo will go unnoticed - missing 'y' in 'icmp_reply'. AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1 icmp_repl=2])]) > +AT_CHECK([RUN_OVS_VSCTL([--may-exist add-zone-tp netdev zone=1 > icmp_first=1 icmp_reply=2])]) > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:1, Timeout > Policies: icmp_first=1 icmp_reply=2 > I mentioned in V1 We should check all possible timeout keys to make sure they work. > +]) > +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2 > icmp_reply=3])]) > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:1, Timeout > Policies: icmp_first=1 icmp_reply=2 > +Zone:2, Timeout Policies: icmp_first=2 icmp_reply=3 > +]) > +AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=1])]) > +AT_CHECK([RUN_OVS_VSCTL([--if-exists del-zone-tp netdev zone=1])]) > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout > Policies: icmp_first=2 icmp_reply=3 > +]) > OVS_VSCTL_CLEANUP > AT_CLEANUP > > @@ -890,10 +904,10 @@ AT_CHECK([RUN_OVS_VSCTL([set bridge br0 > flood_vlans=-1])], > AT_CHECK([RUN_OVS_VSCTL([set bridge br0 flood_vlans=4096])], > [1], [], [ovs-vsctl: constraint violation: 4096 is not in the valid > range 0 to 4095 (inclusive) > ]) > -AT_CHECK([RUN_OVS_VSCTL([set c br1 'connection-mode=xyz'])], > +AT_CHECK([RUN_OVS_VSCTL([set controller br1 'connection-mode=xyz'])], > I mentioned in V1. I don't think we should make unrelated changes in a feature patch especially since it seems the author wanted to convey short form syntax is valid > [1], [], [[ovs-vsctl: constraint violation: xyz is not one of the > allowed values ([in-band, out-of-band]) > ]]) > -AT_CHECK([RUN_OVS_VSCTL([set c br1 connection-mode:x=y])], > +AT_CHECK([RUN_OVS_VSCTL([set controller br1 connection-mode:x=y])], > [1], [], [ovs-vsctl: cannot specify key to set for non-map column > connection_mode > ]) > AT_CHECK([RUN_OVS_VSCTL([add bridge br1 datapath_id x y])], > @@ -929,6 +943,22 @@ AT_CHECK([RUN_OVS_VSCTL([remove bridge br1 > flood-vlans true])], > AT_CHECK([RUN_OVS_VSCTL([clear bridge br1 name])], > [1], [], [ovs-vsctl: cannot modify read-only column name in table Bridge > ]) > + > +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 > +]) > +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 > +]) > +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. > +]) > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout > Policies: icmp_first=2 icmp_reply=3 > Ideally, we should be able to list one zone, but I know I did not mention that before, so lets defer that or not worry about it altogether. > +]) > OVS_VSCTL_CLEANUP > AT_CLEANUP > > 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. > +. > +.IP "\fBlist\-zone\-tp \fIdatapath\fR" > +Prints the timeout policies of all zones under the \fIdatapath\fR. > +. > .SS "OpenFlow Controller Connectivity" > . > \fBovs\-vswitchd\fR can perform all configured bridging and switching > 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 > @@ -40,6 +40,7 @@ > #include "ovsdb-idl.h" > #include "openvswitch/poll-loop.h" > #include "process.h" > +#include "simap.h" > #include "stream.h" > #include "stream-ssl.h" > #include "smap.h" > @@ -49,6 +50,7 @@ > #include "table.h" > #include "timeval.h" > #include "util.h" > +#include "openvswitch/ofp-parse.h" > #include "openvswitch/vconn.h" > #include "openvswitch/vlog.h" > > @@ -1153,6 +1155,201 @@ cmd_emer_reset(struct ctl_context *ctx) > vsctl_context_invalidate_cache(ctx); > } > > +static struct ovsrec_datapath * > +find_datapath(struct vsctl_context *vsctl_ctx, const char *dp_name) > +{ > + const struct ovsrec_open_vswitch *ovs = vsctl_ctx->ovs; > + int i; > + > + for (i = 0; i < ovs->n_datapaths; i++) { > + if (!strcmp(ovs->key_datapaths[i], dp_name)) { > + return ovs->value_datapaths[i]; > + } > + } > + return NULL; > +} > + > +static struct ovsrec_ct_zone * > +find_ct_zone(struct ovsrec_datapath *dp, const int64_t zone_id) > +{ > + int i; > + > + for (i = 0; i < dp->n_ct_zones; i++) { > + if (dp->key_ct_zones[i] == zone_id) { > + return dp->value_ct_zones[i]; > + } > + } > + return NULL; > +} > + > +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; > + > + simap_init(&new_tp); > + > + key_timeouts = xmalloc(sizeof *key_timeouts * n_tps); > + value_timeouts = xmalloc(sizeof *value_timeouts * n_tps); > + > + /* Parse timeout arguments. */ > + for (i = 0; i < n_tps; i++) { > + char *key, *value, *pos, *copy; > + > + pos = copy = xstrdup(argv[i]); > + if (!ofputil_parse_key_value(&pos, &key, &value)) { > + goto done; > + } > + key_timeouts[i] = key; > + value_timeouts[i] = atoi(value); > + simap_put(&new_tp, key, (unsigned int)value_timeouts[i]); > Be careful how you free the string to be parsed... If still not fixed in V3, we can deal with it. > + } > +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]); > + } > + > + if (simap_equal(&s, &new_tp)) { > + tp = CONST_CAST(struct ovsrec_ct_timeout_policy *, row); > + simap_destroy(&s); > + break; > + } > + simap_destroy(&s); > + } > + > + if (!tp) { > + tp = ovsrec_ct_timeout_policy_insert(ctx->txn); > + ovsrec_ct_timeout_policy_set_timeouts(tp, key_timeouts, > + (const int64_t > *)value_timeouts, > + n_tps); > + } > + > + free(key_timeouts); > + free(value_timeouts); > + return tp; > +} > + > +static void > +cmd_add_zone(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]; > + 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); > + return; > + } > + > + n_tps = ctx->argc - 3; > + tp = create_timeout_policy(ctx, &ctx->argv[3], n_tps); > + > + zone = find_ct_zone(dp, zone_id); > + if (zone) { > + if (!may_exist) { > + ctl_fatal("zone id %"PRIu64" alread exists", zone_id); > In this error case, we have already created a new timeout policy above, here: + tp = create_timeout_policy(ctx, &ctx->argv[3], n_tps); I do not think that is a good idea. If it is an error, don't create anything. > + return; > + } > + ovsrec_ct_zone_set_timeout_policy(zone, tp); > + } else { > + zone = ovsrec_ct_zone_insert(ctx->txn); > + ovsrec_ct_zone_set_timeout_policy(zone, tp); > + ovsrec_datapath_update_ct_zones_setkey(dp, zone_id, zone); > + } > +} > + > +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); > + return; > + } > + > + zone = find_ct_zone(dp, zone_id); > + if (must_exist && !zone) { > + ctl_fatal("zone id %"PRIu64" not exists.", zone_id); > + return; > + } > + > + if (zone) { > + ovsrec_datapath_update_ct_zones_delkey(dp, zone_id); > + } > +} > + > +static void > +cmd_list_zone(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]); > + 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]; > + ds_put_format(&ctx->output, "Zone:%"PRIu64", Timeout Policies: ", > + dp->key_ct_zones[i]); > + > + tp = zone->timeout_policy; > + > + 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]); > What exactly are we printing above when the number of timeouts is zero ? Maybe something like this: for (j = 0; j < tp->n_timeouts ; j++) { ds_put_format(&ctx->output, "%s=%"PRIu64" ", tp->key_timeouts[j], tp->value_timeouts[j]); } ds_put_format(&ctx->output, "\n"); will be better. > + } > +} > + > +static void > +pre_get_zone(struct ctl_context *ctx) > +{ > + ovsdb_idl_add_column(ctx->idl, &ovsrec_open_vswitch_col_datapaths); > + ovsdb_idl_add_column(ctx->idl, &ovsrec_datapath_col_ct_zones); > + ovsdb_idl_add_column(ctx->idl, &ovsrec_ct_zone_col_timeout_policy); > + ovsdb_idl_add_column(ctx->idl, > &ovsrec_ct_timeout_policy_col_timeouts); > +} > + > static void > cmd_add_br(struct ctl_context *ctx) > { > @@ -2896,6 +3093,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, 19, "", pre_get_zone, cmd_add_zone, NULL, > + "--may-exist", RW}, > + {"del-zone-tp", 2, 2, "", pre_get_zone, cmd_del_zone, NULL, > + "--if-exists", RW}, > + {"list-zone-tp", 1, 1, "", pre_get_zone, cmd_list_zone, NULL, "", RO}, > + > {NULL, 0, 0, NULL, NULL, NULL, NULL, NULL, RO}, > }; > > -- > 2.7.4 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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@gmail.com> wrote: > > > > From: William Tu <u9012063@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@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@ovn.org> > > Thanks, > > --Justin > Thanks for the review. I will apply your diff and test. Regards, William
Thanks for the review. On Mon, Aug 05, 2019 at 04:12:02PM -0700, Darrell Ball wrote: > Thanks for the patch > > I noticed '--may-exist' and '--if-exists' are supported now for > add--zone-tp/del-zone-tp - thanks > The check for duplicate timeout policies now correctly checks all key and > values - thanks yes, thanks. Will do it in next version. > > Some more comments inline > I am trying to avoid duplicate comment from Justin, so I just won't comment > on some parts in this version > to avoid confusion. > > > On Thu, Aug 1, 2019 at 3:09 PM Yi-Hung Wei <yihung.wei@gmail.com> wrote: > > > From: William Tu <u9012063@gmail.com> > > > > The patch adds commands creating/deleting/listing conntrack zone > > timeout policies: > > $ ovs-vsctl {add,del,list}-zone-tp zone=zone_id ... > > > > Signed-off-by: William Tu <u9012063@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/tests/ovs-vsctl.at b/tests/ovs-vsctl.at > > index 46fa3c5b1a33..f0c5975edd0e 100644 > > --- a/tests/ovs-vsctl.at > > +++ b/tests/ovs-vsctl.at > > @@ -805,6 +805,20 @@ AT_CHECK( > > [RUN_OVS_VSCTL([--if-exists remove netflow x targets '"1.2.3.4:567 > > "'])]) > > AT_CHECK( > > [RUN_OVS_VSCTL([--if-exists clear netflow x targets])]) > > + > > +AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath datapath_version=0 -- > > set Open_vSwitch . datapaths:"netdev"=@m])], [0], [stdout]) > > > > What happens if we add a datapath type here and there are no bridges of > that type; meaning the datapath of that type does not even exist. > Seems like a contradiction. > Maybe we should check for that at least and raise an error. > Ideally, it is better if these 'datapaths' are auto-managed by bridge > creation/deletion with given datapath types, > but we can certainly defer that. > > > > +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1 > > icmp_reply=2])]) > > > > I mentioned this in V1: > There is no filtering of bad timeout key; for example > > AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1 > foo_bar=2])]) > > is accepted as valid > > Even worse, a minor typo will go unnoticed - missing 'y' in 'icmp_reply'. > > AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1 > icmp_repl=2])]) agree. > > > > +AT_CHECK([RUN_OVS_VSCTL([--may-exist add-zone-tp netdev zone=1 > > icmp_first=1 icmp_reply=2])]) > > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:1, Timeout > > Policies: icmp_first=1 icmp_reply=2 > > > > I mentioned in V1 > We should check all possible timeout keys to make sure they work. OK. > > > > +]) > > +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2 > > icmp_reply=3])]) > > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:1, Timeout > > Policies: icmp_first=1 icmp_reply=2 > > +Zone:2, Timeout Policies: icmp_first=2 icmp_reply=3 > > +]) > > +AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=1])]) > > +AT_CHECK([RUN_OVS_VSCTL([--if-exists del-zone-tp netdev zone=1])]) > > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout > > Policies: icmp_first=2 icmp_reply=3 > > +]) > > OVS_VSCTL_CLEANUP > > AT_CLEANUP > > > > @@ -890,10 +904,10 @@ AT_CHECK([RUN_OVS_VSCTL([set bridge br0 > > flood_vlans=-1])], > > AT_CHECK([RUN_OVS_VSCTL([set bridge br0 flood_vlans=4096])], > > [1], [], [ovs-vsctl: constraint violation: 4096 is not in the valid > > range 0 to 4095 (inclusive) > > ]) > > -AT_CHECK([RUN_OVS_VSCTL([set c br1 'connection-mode=xyz'])], > > +AT_CHECK([RUN_OVS_VSCTL([set controller br1 'connection-mode=xyz'])], > > > > I mentioned in V1. > I don't think we should make unrelated changes in a feature patch > especially since it seems the author wanted to convey > short form syntax is valid This has to be there, otherwise test fails. > > > > [1], [], [[ovs-vsctl: constraint violation: xyz is not one of the > > allowed values ([in-band, out-of-band]) > > ]]) > > -AT_CHECK([RUN_OVS_VSCTL([set c br1 connection-mode:x=y])], > > +AT_CHECK([RUN_OVS_VSCTL([set controller br1 connection-mode:x=y])], > > [1], [], [ovs-vsctl: cannot specify key to set for non-map column > > connection_mode > > ]) > > AT_CHECK([RUN_OVS_VSCTL([add bridge br1 datapath_id x y])], > > @@ -929,6 +943,22 @@ AT_CHECK([RUN_OVS_VSCTL([remove bridge br1 > > flood-vlans true])], > > AT_CHECK([RUN_OVS_VSCTL([clear bridge br1 name])], > > [1], [], [ovs-vsctl: cannot modify read-only column name in table Bridge > > ]) > > + > > +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 > > +]) > > +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 > > +]) > > +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. > > +]) > > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout > > Policies: icmp_first=2 icmp_reply=3 > > > > Ideally, we should be able to list one zone, but I know I did not mention > that before, so lets defer that or not worry about it > altogether. > > > > +]) > > OVS_VSCTL_CLEANUP > > AT_CLEANUP > > > > 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. > > +. > > +.IP "\fBlist\-zone\-tp \fIdatapath\fR" > > +Prints the timeout policies of all zones under the \fIdatapath\fR. > > +. > > .SS "OpenFlow Controller Connectivity" > > . > > \fBovs\-vswitchd\fR can perform all configured bridging and switching > > 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 > > @@ -40,6 +40,7 @@ > > #include "ovsdb-idl.h" > > #include "openvswitch/poll-loop.h" > > #include "process.h" > > +#include "simap.h" > > #include "stream.h" > > #include "stream-ssl.h" > > #include "smap.h" > > @@ -49,6 +50,7 @@ > > #include "table.h" > > #include "timeval.h" > > #include "util.h" > > +#include "openvswitch/ofp-parse.h" > > #include "openvswitch/vconn.h" > > #include "openvswitch/vlog.h" > > > > @@ -1153,6 +1155,201 @@ cmd_emer_reset(struct ctl_context *ctx) > > vsctl_context_invalidate_cache(ctx); > > } > > > > +static struct ovsrec_datapath * > > +find_datapath(struct vsctl_context *vsctl_ctx, const char *dp_name) > > +{ > > + const struct ovsrec_open_vswitch *ovs = vsctl_ctx->ovs; > > + int i; > > + > > + for (i = 0; i < ovs->n_datapaths; i++) { > > + if (!strcmp(ovs->key_datapaths[i], dp_name)) { > > + return ovs->value_datapaths[i]; > > + } > > + } > > + return NULL; > > +} > > + > > +static struct ovsrec_ct_zone * > > +find_ct_zone(struct ovsrec_datapath *dp, const int64_t zone_id) > > +{ > > + int i; > > + > > + for (i = 0; i < dp->n_ct_zones; i++) { > > + if (dp->key_ct_zones[i] == zone_id) { > > + return dp->value_ct_zones[i]; > > + } > > + } > > + return NULL; > > +} > > + > > +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; > > + > > + simap_init(&new_tp); > > + > > + key_timeouts = xmalloc(sizeof *key_timeouts * n_tps); > > + value_timeouts = xmalloc(sizeof *value_timeouts * n_tps); > > + > > + /* Parse timeout arguments. */ > > + for (i = 0; i < n_tps; i++) { > > + char *key, *value, *pos, *copy; > > + > > + pos = copy = xstrdup(argv[i]); > > + if (!ofputil_parse_key_value(&pos, &key, &value)) { > > + goto done; > > + } > > + key_timeouts[i] = key; > > + value_timeouts[i] = atoi(value); > > + simap_put(&new_tp, key, (unsigned int)value_timeouts[i]); > > > > Be careful how you free the string to be parsed... > If still not fixed in V3, we can deal with it. > > > > + } > > +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]); > > + } > > + > > + if (simap_equal(&s, &new_tp)) { > > + tp = CONST_CAST(struct ovsrec_ct_timeout_policy *, row); > > + simap_destroy(&s); > > + break; > > + } > > + simap_destroy(&s); > > + } > > + > > + if (!tp) { > > + tp = ovsrec_ct_timeout_policy_insert(ctx->txn); > > + ovsrec_ct_timeout_policy_set_timeouts(tp, key_timeouts, > > + (const int64_t > > *)value_timeouts, > > + n_tps); > > + } > > + > > + free(key_timeouts); > > + free(value_timeouts); > > + return tp; > > +} > > + > > +static void > > +cmd_add_zone(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]; > > + 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); > > + return; > > + } > > + > > + n_tps = ctx->argc - 3; > > + tp = create_timeout_policy(ctx, &ctx->argv[3], n_tps); > > + > > + zone = find_ct_zone(dp, zone_id); > > + if (zone) { > > + if (!may_exist) { > > + ctl_fatal("zone id %"PRIu64" alread exists", zone_id); > > > > In this error case, we have already created a new timeout policy above, > here: > > + tp = create_timeout_policy(ctx, &ctx->argv[3], n_tps); > > I do not think that is a good idea. > If it is an error, don't create anything. OK I will fix it. > > > > > + return; > > + } > > + ovsrec_ct_zone_set_timeout_policy(zone, tp); > > + } else { > > + zone = ovsrec_ct_zone_insert(ctx->txn); > > + ovsrec_ct_zone_set_timeout_policy(zone, tp); > > + ovsrec_datapath_update_ct_zones_setkey(dp, zone_id, zone); > > + } > > +} > > + > > +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); > > + return; > > + } > > + > > + zone = find_ct_zone(dp, zone_id); > > + if (must_exist && !zone) { > > + ctl_fatal("zone id %"PRIu64" not exists.", zone_id); > > + return; > > + } > > + > > + if (zone) { > > + ovsrec_datapath_update_ct_zones_delkey(dp, zone_id); > > + } > > +} > > + > > +static void > > +cmd_list_zone(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]); > > + 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]; > > + ds_put_format(&ctx->output, "Zone:%"PRIu64", Timeout Policies: ", > > + dp->key_ct_zones[i]); > > + > > + tp = zone->timeout_policy; > > + > > + 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]); > > > > What exactly are we printing above when the number of timeouts is zero ? > > Maybe something like this: > > for (j = 0; j < tp->n_timeouts ; j++) { > ds_put_format(&ctx->output, "%s=%"PRIu64" ", > tp->key_timeouts[j], tp->value_timeouts[j]); > } > ds_put_format(&ctx->output, "\n"); > > will be better. OK Will fix it. Thank you for your review. William > > > > > + } > > +} > > + > > +static void > > +pre_get_zone(struct ctl_context *ctx) > > +{ > > + ovsdb_idl_add_column(ctx->idl, &ovsrec_open_vswitch_col_datapaths); > > + ovsdb_idl_add_column(ctx->idl, &ovsrec_datapath_col_ct_zones); > > + ovsdb_idl_add_column(ctx->idl, &ovsrec_ct_zone_col_timeout_policy); > > + ovsdb_idl_add_column(ctx->idl, > > &ovsrec_ct_timeout_policy_col_timeouts); > > +} > > + > > static void > > cmd_add_br(struct ctl_context *ctx) > > { > > @@ -2896,6 +3093,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, 19, "", pre_get_zone, cmd_add_zone, NULL, > > + "--may-exist", RW}, > > + {"del-zone-tp", 2, 2, "", pre_get_zone, cmd_del_zone, NULL, > > + "--if-exists", RW}, > > + {"list-zone-tp", 1, 1, "", pre_get_zone, cmd_list_zone, NULL, "", RO}, > > + > > {NULL, 0, 0, NULL, NULL, NULL, NULL, NULL, RO}, > > }; > > > > -- > > 2.7.4 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Wed, Aug 7, 2019 at 1:37 PM William Tu <u9012063@gmail.com> wrote: > Thanks for the review. > > On Mon, Aug 05, 2019 at 04:12:02PM -0700, Darrell Ball wrote: > > Thanks for the patch > > > > I noticed '--may-exist' and '--if-exists' are supported now for > > add--zone-tp/del-zone-tp - thanks > > The check for duplicate timeout policies now correctly checks all key and > > values - thanks > > yes, thanks. Will do it in next version. > > > > Some more comments inline > > I am trying to avoid duplicate comment from Justin, so I just won't > comment > > on some parts in this version > > to avoid confusion. > > > > > > On Thu, Aug 1, 2019 at 3:09 PM Yi-Hung Wei <yihung.wei@gmail.com> wrote: > > > > > From: William Tu <u9012063@gmail.com> > > > > > > The patch adds commands creating/deleting/listing conntrack zone > > > timeout policies: > > > $ ovs-vsctl {add,del,list}-zone-tp zone=zone_id ... > > > > > > Signed-off-by: William Tu <u9012063@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/tests/ovs-vsctl.at b/tests/ovs-vsctl.at > > > index 46fa3c5b1a33..f0c5975edd0e 100644 > > > --- a/tests/ovs-vsctl.at > > > +++ b/tests/ovs-vsctl.at > > > @@ -805,6 +805,20 @@ AT_CHECK( > > > [RUN_OVS_VSCTL([--if-exists remove netflow x targets '"1.2.3.4:567 > > > "'])]) > > > AT_CHECK( > > > [RUN_OVS_VSCTL([--if-exists clear netflow x targets])]) > > > + > > > +AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath > datapath_version=0 -- > > > set Open_vSwitch . datapaths:"netdev"=@m])], [0], [stdout]) > > > > > > > What happens if we add a datapath type here and there are no bridges of > > that type; meaning the datapath of that type does not even exist. > > Seems like a contradiction. > > Maybe we should check for that at least and raise an error. > > Ideally, it is better if these 'datapaths' are auto-managed by bridge > > creation/deletion with given datapath types, > > but we can certainly defer that. > > > > > > > +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1 > > > icmp_reply=2])]) > > > > > > > I mentioned this in V1: > > There is no filtering of bad timeout key; for example > > > > AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1 > > foo_bar=2])]) > > > > is accepted as valid > > > > Even worse, a minor typo will go unnoticed - missing 'y' in 'icmp_reply'. > > > > AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1 > > icmp_repl=2])]) > > agree. > > > > > > > +AT_CHECK([RUN_OVS_VSCTL([--may-exist add-zone-tp netdev zone=1 > > > icmp_first=1 icmp_reply=2])]) > > > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:1, Timeout > > > Policies: icmp_first=1 icmp_reply=2 > > > > > > > I mentioned in V1 > > We should check all possible timeout keys to make sure they work. > > OK. > > > > > > > +]) > > > +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2 > > > icmp_reply=3])]) > > > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:1, Timeout > > > Policies: icmp_first=1 icmp_reply=2 > > > +Zone:2, Timeout Policies: icmp_first=2 icmp_reply=3 > > > +]) > > > +AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=1])]) > > > +AT_CHECK([RUN_OVS_VSCTL([--if-exists del-zone-tp netdev zone=1])]) > > > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout > > > Policies: icmp_first=2 icmp_reply=3 > > > +]) > > > OVS_VSCTL_CLEANUP > > > AT_CLEANUP > > > > > > @@ -890,10 +904,10 @@ AT_CHECK([RUN_OVS_VSCTL([set bridge br0 > > > flood_vlans=-1])], > > > AT_CHECK([RUN_OVS_VSCTL([set bridge br0 flood_vlans=4096])], > > > [1], [], [ovs-vsctl: constraint violation: 4096 is not in the valid > > > range 0 to 4095 (inclusive) > > > ]) > > > -AT_CHECK([RUN_OVS_VSCTL([set c br1 'connection-mode=xyz'])], > > > +AT_CHECK([RUN_OVS_VSCTL([set controller br1 'connection-mode=xyz'])], > > > > > > > I mentioned in V1. > > I don't think we should make unrelated changes in a feature patch > > especially since it seems the author wanted to convey > > short form syntax is valid > > This has to be there, otherwise test fails. > yep, I got the obvious reason after running the negative test. +ovs-vsctl: multiple table names match "c" > > > > > > > [1], [], [[ovs-vsctl: constraint violation: xyz is not one of the > > > allowed values ([in-band, out-of-band]) > > > ]]) > > > -AT_CHECK([RUN_OVS_VSCTL([set c br1 connection-mode:x=y])], > > > +AT_CHECK([RUN_OVS_VSCTL([set controller br1 connection-mode:x=y])], > > > [1], [], [ovs-vsctl: cannot specify key to set for non-map column > > > connection_mode > > > ]) > > > AT_CHECK([RUN_OVS_VSCTL([add bridge br1 datapath_id x y])], > > > @@ -929,6 +943,22 @@ AT_CHECK([RUN_OVS_VSCTL([remove bridge br1 > > > flood-vlans true])], > > > AT_CHECK([RUN_OVS_VSCTL([clear bridge br1 name])], > > > [1], [], [ovs-vsctl: cannot modify read-only column name in table > Bridge > > > ]) > > > + > > > +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 > > > +]) > > > +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 > > > +]) > > > +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. > > > +]) > > > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout > > > Policies: icmp_first=2 icmp_reply=3 > > > > > > > Ideally, we should be able to list one zone, but I know I did not mention > > that before, so lets defer that or not worry about it > > altogether. > > > > > > > +]) > > > OVS_VSCTL_CLEANUP > > > AT_CLEANUP > > > > > > 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. > > > +. > > > +.IP "\fBlist\-zone\-tp \fIdatapath\fR" > > > +Prints the timeout policies of all zones under the \fIdatapath\fR. > > > +. > > > .SS "OpenFlow Controller Connectivity" > > > . > > > \fBovs\-vswitchd\fR can perform all configured bridging and switching > > > 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 > > > @@ -40,6 +40,7 @@ > > > #include "ovsdb-idl.h" > > > #include "openvswitch/poll-loop.h" > > > #include "process.h" > > > +#include "simap.h" > > > #include "stream.h" > > > #include "stream-ssl.h" > > > #include "smap.h" > > > @@ -49,6 +50,7 @@ > > > #include "table.h" > > > #include "timeval.h" > > > #include "util.h" > > > +#include "openvswitch/ofp-parse.h" > > > #include "openvswitch/vconn.h" > > > #include "openvswitch/vlog.h" > > > > > > @@ -1153,6 +1155,201 @@ cmd_emer_reset(struct ctl_context *ctx) > > > vsctl_context_invalidate_cache(ctx); > > > } > > > > > > +static struct ovsrec_datapath * > > > +find_datapath(struct vsctl_context *vsctl_ctx, const char *dp_name) > > > +{ > > > + const struct ovsrec_open_vswitch *ovs = vsctl_ctx->ovs; > > > + int i; > > > + > > > + for (i = 0; i < ovs->n_datapaths; i++) { > > > + if (!strcmp(ovs->key_datapaths[i], dp_name)) { > > > + return ovs->value_datapaths[i]; > > > + } > > > + } > > > + return NULL; > > > +} > > > + > > > +static struct ovsrec_ct_zone * > > > +find_ct_zone(struct ovsrec_datapath *dp, const int64_t zone_id) > > > +{ > > > + int i; > > > + > > > + for (i = 0; i < dp->n_ct_zones; i++) { > > > + if (dp->key_ct_zones[i] == zone_id) { > > > + return dp->value_ct_zones[i]; > > > + } > > > + } > > > + return NULL; > > > +} > > > + > > > +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; > > > + > > > + simap_init(&new_tp); > > > + > > > + key_timeouts = xmalloc(sizeof *key_timeouts * n_tps); > > > + value_timeouts = xmalloc(sizeof *value_timeouts * n_tps); > > > + > > > + /* Parse timeout arguments. */ > > > + for (i = 0; i < n_tps; i++) { > > > + char *key, *value, *pos, *copy; > > > + > > > + pos = copy = xstrdup(argv[i]); > > > + if (!ofputil_parse_key_value(&pos, &key, &value)) { > > > + goto done; > > > + } > > > + key_timeouts[i] = key; > > > + value_timeouts[i] = atoi(value); > > > + simap_put(&new_tp, key, (unsigned int)value_timeouts[i]); > > > > > > > Be careful how you free the string to be parsed... > > If still not fixed in V3, we can deal with it. > > > > > > > + } > > > +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]); > > > + } > > > + > > > + if (simap_equal(&s, &new_tp)) { > > > + tp = CONST_CAST(struct ovsrec_ct_timeout_policy *, row); > > > + simap_destroy(&s); > > > + break; > > > + } > > > + simap_destroy(&s); > > > + } > > > + > > > + if (!tp) { > > > + tp = ovsrec_ct_timeout_policy_insert(ctx->txn); > > > + ovsrec_ct_timeout_policy_set_timeouts(tp, key_timeouts, > > > + (const int64_t > > > *)value_timeouts, > > > + n_tps); > > > + } > > > + > > > + free(key_timeouts); > > > + free(value_timeouts); > > > + return tp; > > > +} > > > + > > > +static void > > > +cmd_add_zone(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]; > > > + 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); > > > + return; > > > + } > > > + > > > + n_tps = ctx->argc - 3; > > > + tp = create_timeout_policy(ctx, &ctx->argv[3], n_tps); > > > + > > > + zone = find_ct_zone(dp, zone_id); > > > + if (zone) { > > > + if (!may_exist) { > > > + ctl_fatal("zone id %"PRIu64" alread exists", zone_id); > > > > > > > In this error case, we have already created a new timeout policy above, > > here: > > > > + tp = create_timeout_policy(ctx, &ctx->argv[3], n_tps); > > > > I do not think that is a good idea. > > If it is an error, don't create anything. > > OK I will fix it. > > > > > > > > > + return; > > > + } > > > + ovsrec_ct_zone_set_timeout_policy(zone, tp); > > > + } else { > > > + zone = ovsrec_ct_zone_insert(ctx->txn); > > > + ovsrec_ct_zone_set_timeout_policy(zone, tp); > > > + ovsrec_datapath_update_ct_zones_setkey(dp, zone_id, zone); > > > + } > > > +} > > > + > > > +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); > > > + return; > > > + } > > > + > > > + zone = find_ct_zone(dp, zone_id); > > > + if (must_exist && !zone) { > > > + ctl_fatal("zone id %"PRIu64" not exists.", zone_id); > > > + return; > > > + } > > > + > > > + if (zone) { > > > + ovsrec_datapath_update_ct_zones_delkey(dp, zone_id); > > > + } > > > +} > > > + > > > +static void > > > +cmd_list_zone(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]); > > > + 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]; > > > + ds_put_format(&ctx->output, "Zone:%"PRIu64", Timeout > Policies: ", > > > + dp->key_ct_zones[i]); > > > + > > > + tp = zone->timeout_policy; > > > + > > > + 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]); > > > > > > > What exactly are we printing above when the number of timeouts is zero ? > > > > Maybe something like this: > > > > for (j = 0; j < tp->n_timeouts ; j++) { > > ds_put_format(&ctx->output, "%s=%"PRIu64" ", > > tp->key_timeouts[j], tp->value_timeouts[j]); > > } > > ds_put_format(&ctx->output, "\n"); > > > > will be better. > > OK Will fix it. > > Thank you for your review. > William > > > > > > > > > + } > > > +} > > > + > > > +static void > > > +pre_get_zone(struct ctl_context *ctx) > > > +{ > > > + ovsdb_idl_add_column(ctx->idl, > &ovsrec_open_vswitch_col_datapaths); > > > + ovsdb_idl_add_column(ctx->idl, &ovsrec_datapath_col_ct_zones); > > > + ovsdb_idl_add_column(ctx->idl, > &ovsrec_ct_zone_col_timeout_policy); > > > + ovsdb_idl_add_column(ctx->idl, > > > &ovsrec_ct_timeout_policy_col_timeouts); > > > +} > > > + > > > static void > > > cmd_add_br(struct ctl_context *ctx) > > > { > > > @@ -2896,6 +3093,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, 19, "", pre_get_zone, cmd_add_zone, NULL, > > > + "--may-exist", RW}, > > > + {"del-zone-tp", 2, 2, "", pre_get_zone, cmd_del_zone, NULL, > > > + "--if-exists", RW}, > > > + {"list-zone-tp", 1, 1, "", pre_get_zone, cmd_list_zone, NULL, "", > RO}, > > > + > > > {NULL, 0, 0, NULL, NULL, NULL, NULL, NULL, RO}, > > > }; > > > > > > -- > > > 2.7.4 > > > > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at index 46fa3c5b1a33..f0c5975edd0e 100644 --- a/tests/ovs-vsctl.at +++ b/tests/ovs-vsctl.at @@ -805,6 +805,20 @@ AT_CHECK( [RUN_OVS_VSCTL([--if-exists remove netflow x targets '"1.2.3.4:567"'])]) AT_CHECK( [RUN_OVS_VSCTL([--if-exists clear netflow x targets])]) + +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 netdev zone=1 icmp_first=1 icmp_reply=2])]) +AT_CHECK([RUN_OVS_VSCTL([--may-exist add-zone-tp netdev zone=1 icmp_first=1 icmp_reply=2])]) +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:1, Timeout Policies: icmp_first=1 icmp_reply=2 +]) +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2 icmp_reply=3])]) +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:1, Timeout Policies: icmp_first=1 icmp_reply=2 +Zone:2, Timeout Policies: icmp_first=2 icmp_reply=3 +]) +AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=1])]) +AT_CHECK([RUN_OVS_VSCTL([--if-exists del-zone-tp netdev zone=1])]) +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout Policies: icmp_first=2 icmp_reply=3 +]) OVS_VSCTL_CLEANUP AT_CLEANUP @@ -890,10 +904,10 @@ AT_CHECK([RUN_OVS_VSCTL([set bridge br0 flood_vlans=-1])], AT_CHECK([RUN_OVS_VSCTL([set bridge br0 flood_vlans=4096])], [1], [], [ovs-vsctl: constraint violation: 4096 is not in the valid range 0 to 4095 (inclusive) ]) -AT_CHECK([RUN_OVS_VSCTL([set c br1 'connection-mode=xyz'])], +AT_CHECK([RUN_OVS_VSCTL([set controller br1 'connection-mode=xyz'])], [1], [], [[ovs-vsctl: constraint violation: xyz is not one of the allowed values ([in-band, out-of-band]) ]]) -AT_CHECK([RUN_OVS_VSCTL([set c br1 connection-mode:x=y])], +AT_CHECK([RUN_OVS_VSCTL([set controller br1 connection-mode:x=y])], [1], [], [ovs-vsctl: cannot specify key to set for non-map column connection_mode ]) AT_CHECK([RUN_OVS_VSCTL([add bridge br1 datapath_id x y])], @@ -929,6 +943,22 @@ AT_CHECK([RUN_OVS_VSCTL([remove bridge br1 flood-vlans true])], AT_CHECK([RUN_OVS_VSCTL([clear bridge br1 name])], [1], [], [ovs-vsctl: cannot modify read-only column name in table Bridge ]) + +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 +]) +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 +]) +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. +]) +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout Policies: icmp_first=2 icmp_reply=3 +]) OVS_VSCTL_CLEANUP AT_CLEANUP 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. +. +.IP "\fBlist\-zone\-tp \fIdatapath\fR" +Prints the timeout policies of all zones under the \fIdatapath\fR. +. .SS "OpenFlow Controller Connectivity" . \fBovs\-vswitchd\fR can perform all configured bridging and switching 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 @@ -40,6 +40,7 @@ #include "ovsdb-idl.h" #include "openvswitch/poll-loop.h" #include "process.h" +#include "simap.h" #include "stream.h" #include "stream-ssl.h" #include "smap.h" @@ -49,6 +50,7 @@ #include "table.h" #include "timeval.h" #include "util.h" +#include "openvswitch/ofp-parse.h" #include "openvswitch/vconn.h" #include "openvswitch/vlog.h" @@ -1153,6 +1155,201 @@ cmd_emer_reset(struct ctl_context *ctx) vsctl_context_invalidate_cache(ctx); } +static struct ovsrec_datapath * +find_datapath(struct vsctl_context *vsctl_ctx, const char *dp_name) +{ + const struct ovsrec_open_vswitch *ovs = vsctl_ctx->ovs; + int i; + + for (i = 0; i < ovs->n_datapaths; i++) { + if (!strcmp(ovs->key_datapaths[i], dp_name)) { + return ovs->value_datapaths[i]; + } + } + return NULL; +} + +static struct ovsrec_ct_zone * +find_ct_zone(struct ovsrec_datapath *dp, const int64_t zone_id) +{ + int i; + + for (i = 0; i < dp->n_ct_zones; i++) { + if (dp->key_ct_zones[i] == zone_id) { + return dp->value_ct_zones[i]; + } + } + return NULL; +} + +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; + + simap_init(&new_tp); + + key_timeouts = xmalloc(sizeof *key_timeouts * n_tps); + value_timeouts = xmalloc(sizeof *value_timeouts * n_tps); + + /* Parse timeout arguments. */ + for (i = 0; i < n_tps; i++) { + char *key, *value, *pos, *copy; + + pos = copy = xstrdup(argv[i]); + if (!ofputil_parse_key_value(&pos, &key, &value)) { + goto done; + } + key_timeouts[i] = key; + value_timeouts[i] = atoi(value); + simap_put(&new_tp, key, (unsigned int)value_timeouts[i]); + } +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]); + } + + if (simap_equal(&s, &new_tp)) { + tp = CONST_CAST(struct ovsrec_ct_timeout_policy *, row); + simap_destroy(&s); + break; + } + simap_destroy(&s); + } + + if (!tp) { + tp = ovsrec_ct_timeout_policy_insert(ctx->txn); + ovsrec_ct_timeout_policy_set_timeouts(tp, key_timeouts, + (const int64_t *)value_timeouts, + n_tps); + } + + free(key_timeouts); + free(value_timeouts); + return tp; +} + +static void +cmd_add_zone(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]; + 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); + return; + } + + n_tps = ctx->argc - 3; + tp = create_timeout_policy(ctx, &ctx->argv[3], n_tps); + + zone = find_ct_zone(dp, zone_id); + if (zone) { + if (!may_exist) { + ctl_fatal("zone id %"PRIu64" alread exists", zone_id); + return; + } + ovsrec_ct_zone_set_timeout_policy(zone, tp); + } else { + zone = ovsrec_ct_zone_insert(ctx->txn); + ovsrec_ct_zone_set_timeout_policy(zone, tp); + ovsrec_datapath_update_ct_zones_setkey(dp, zone_id, zone); + } +} + +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); + return; + } + + zone = find_ct_zone(dp, zone_id); + if (must_exist && !zone) { + ctl_fatal("zone id %"PRIu64" not exists.", zone_id); + return; + } + + if (zone) { + ovsrec_datapath_update_ct_zones_delkey(dp, zone_id); + } +} + +static void +cmd_list_zone(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]); + 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]; + ds_put_format(&ctx->output, "Zone:%"PRIu64", Timeout Policies: ", + dp->key_ct_zones[i]); + + tp = zone->timeout_policy; + + 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]); + } +} + +static void +pre_get_zone(struct ctl_context *ctx) +{ + ovsdb_idl_add_column(ctx->idl, &ovsrec_open_vswitch_col_datapaths); + ovsdb_idl_add_column(ctx->idl, &ovsrec_datapath_col_ct_zones); + ovsdb_idl_add_column(ctx->idl, &ovsrec_ct_zone_col_timeout_policy); + ovsdb_idl_add_column(ctx->idl, &ovsrec_ct_timeout_policy_col_timeouts); +} + static void cmd_add_br(struct ctl_context *ctx) { @@ -2896,6 +3093,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, 19, "", pre_get_zone, cmd_add_zone, NULL, + "--may-exist", RW}, + {"del-zone-tp", 2, 2, "", pre_get_zone, cmd_del_zone, NULL, + "--if-exists", RW}, + {"list-zone-tp", 1, 1, "", pre_get_zone, cmd_list_zone, NULL, "", RO}, + {NULL, 0, 0, NULL, NULL, NULL, NULL, NULL, RO}, };