[ovs-dev] [PATCH v2 2/9] ovs-vsctl: Add conntrack zone commands.
Justin Pettit
jpettit at ovn.org
Sat Aug 3 00:42:16 UTC 2019
> On Aug 1, 2019, at 3:07 PM, Yi-Hung Wei <yihung.wei at gmail.com> wrote:
>
> From: William Tu <u9012063 at gmail.com>
>
> The patch adds commands creating/deleting/listing conntrack zone
> timeout policies:
> $ ovs-vsctl {add,del,list}-zone-tp zone=zone_id ...
I think the command also requires a datapath argument.
> Signed-off-by: William Tu <u9012063 at gmail.com>
> ---
> tests/ovs-vsctl.at | 34 +++++++-
> utilities/ovs-vsctl.8.in | 25 ++++++
> utilities/ovs-vsctl.c | 204 +++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 261 insertions(+), 2 deletions(-)
> diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
> index 7c09df79bd29..0925d4d97b39 100644
> --- a/utilities/ovs-vsctl.8.in
> +++ b/utilities/ovs-vsctl.8.in
> @@ -353,6 +353,31 @@ list.
> Prints the name of the bridge that contains \fIiface\fR on standard
> output.
> .
> +.SS "Conntrack Zone Commands"
> +These commands query and modify datapath CT zones and Timeout Policies.
> +.
> +.IP "\fBadd\-zone\-tp \fIdatapath \fBzone=\fIzone_id \fIpolicies\fR"
> +Creates a conntrack zone with \fIzone_id\fR under the datapath \fIdatapath\fR.
> +Associate the conntrack timeout policies to it by a list of
> +\fIkey\fB=\fIvalue\fR pairs, separeted by space. For example, specifying
> +30-second timeout policy for first icmp packet, and 60-second for icmp reply packet
> +by doing \fBicmp_first=30 icmp_reply=60\fR. See CT_Timeout_Policy TABLE
> +at \fBovs-vswitchd.conf.db\fR(5) for all available configurations.
> +.IP
> +Without \fB\-\-may\-exist\fR, attempting to add a \fIzone_id\fR that
> +already exists is an error. With \fB\-\-may\-exist\fR,
> +this command does nothing if \fIzone_id\fR is already created\fR.
> +.
> +.IP "\fBdel\-zone\-tp \fIdatapath \fBzone=\fIzone_id\fR"
> +Delete a zone under \fIdatapath\fR by specifying its zone ID.
> +.IP
> +Without \fB\-\-if\-exists\fR, attempting to delete a zone that
> +does not exist is an error. With \fB\-\-if\-exists\fR, attempting to
> +delete a zone that does not exist has no effect.
I think "--may-exist" and "--if-exists" should be included in the line describing the command. I have a few other suggested changes in the attached patch.
> diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> index 4948137efe8c..16578edfc331 100644
> --- a/utilities/ovs-vsctl.c
> +++ b/utilities/ovs-vsctl.c
>
> +static struct ovsrec_ct_timeout_policy *
> +create_timeout_policy(struct ctl_context *ctx, char **argv, int n_tps)
> +{
> + const struct ovsrec_ct_timeout_policy_table *tp_table;
> + const struct ovsrec_ct_timeout_policy *row;
> + struct ovsrec_ct_timeout_policy *tp = NULL;
> + const char **key_timeouts;
> + int64_t *value_timeouts;
> + struct simap new_tp, s;
> + int i, j;
I think 'i' and 'j' can be declared in the for loop definitions (and you don't actually need 'j' since it's not nested).
> + /* Parse timeout arguments. */
> + for (i = 0; i < n_tps; i++) {
> + char *key, *value, *pos, *copy;
'copy' doesn't appear to be used.
> +
> + pos = copy = xstrdup(argv[i]);
I think 'pos' is leaked. I had to go through some contortions to make it work without modifying 'argv'; let me know if you think the logic in the attached diff is correct.
> + free(key_timeouts);
> + free(value_timeouts);
> + return tp;
I think you also need to destroy 'new_tp'.
> +static void
> +cmd_add_zone(struct ctl_context *ctx)
I think these functions might be more accurately described with "_tp" at the end.
> +{
> + struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> + struct ovsrec_ct_timeout_policy *tp;
> + struct ovsrec_ct_zone *zone;
> + struct ovsrec_datapath *dp;
> + const char *dp_name;
> + int64_t zone_id;
> + bool may_exist;
> + int n_tps;
Minor style nit, but we usually declare the variables closer to their use now. I've updated some of them in the attached diff.
> + dp_name = ctx->argv[1];
> + ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
> + may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
> +
> + dp = find_datapath(vsctl_ctx, dp_name);
> + if (!dp) {
> + ctl_fatal("datapath: %s record not found", dp_name);
This error message is a bit out of style, the other code usually is more like "datapath %s does not exit".
> + return;
I don't think you need to return after a call to ctl_fatal().
> + zone = find_ct_zone(dp, zone_id);
> + if (zone) {
> + if (!may_exist) {
> + ctl_fatal("zone id %"PRIu64" alread exists", zone_id);
s/alread/already/
> +static void
> +cmd_del_zone(struct ctl_context *ctx)
> +{
> + struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> + struct ovsrec_ct_zone *zone;
> + struct ovsrec_datapath *dp;
> + const char *dp_name;
> + int64_t zone_id;
> + bool must_exist;
> +
> + must_exist = !shash_find(&ctx->options, "--if-exists");
> + dp_name = ctx->argv[1];
> + ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
> +
> + dp = find_datapath(vsctl_ctx, dp_name);
> + if (!dp) {
> + ctl_fatal("datapath: %s record not found.", dp_name);
There's some inconsistency about whether the errors have a period. I'd suggest removing them, since I think that's more common in the ovs-vsctl error messages.
> + return;
> + }
> +
> + zone = find_ct_zone(dp, zone_id);
> + if (must_exist && !zone) {
> + ctl_fatal("zone id %"PRIu64" not exists.", zone_id);
s/not exists/does not exist/
> +static void
> +cmd_list_zone(struct ctl_context *ctx)
> +{
> ...
> + struct ovsrec_ct_timeout_policy *tp;
> + struct ovsrec_ct_zone *zone;
> ...
> + int i, j;
These could all be declared in a tighter scope.
Assuming you agree with my proposed changes:
Acked-by: Justin Pettit <jpettit at ovn.org>
Thanks,
--Justin
-=-=-=-=-=-=-=-=-=-=-
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},
};
More information about the dev
mailing list