> On Nov 21, 2017, at 5:00 PM, Yi-Hung Wei <[email protected]> wrote:
> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
> index 5cd6b5cfd870..3cad5c928e8b 100644
> --- a/lib/ct-dpif.c
> +++ b/lib/ct-dpif.c
>
> @@ -434,3 +435,110 @@ ct_dpif_format_tcp_stat(struct ds * ds, int tcp_state,
> int conn_per_state)
> ds_put_cstr(ds, "]");
> ds_put_format(ds, "=%u", conn_per_state);
> }
> +
> +/* Parses a specification of a conntrack 5-tuple from 's' into 'tuple'.
> + * Returns true on success. Otherwise, returns false and puts the error
> + * message in 'ds'. */
> +bool
> +ct_dpif_parse_tuple(struct ct_dpif_tuple *tuple, const char *s, struct ds
> *ds)
> +{
> ...
> + if (ipv6_is_zero(&tuple->src.in6) || ipv6_is_zero(&tuple->dst.in6) ||
> + !tuple->ip_proto) {
> + /* icmp_type, icmp_code, and icmp_id can be 0. */
> + if (tuple->ip_proto != IPPROTO_ICMP &&
> + tuple->ip_proto != IPPROTO_ICMP) {
I think that second one should be IPPROTO_ICMPV6.
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 7fc0e3afab37..06cfbc4abf21 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -1331,30 +1331,73 @@ dpctl_flush_conntrack(int argc, const char *argv[],
> struct dpctl_params *dpctl_p)
> {
> struct dpif *dpif;
> + struct ct_dpif_tuple tuple, *ptuple = NULL;
> + struct ds ds = DS_EMPTY_INITIALIZER;
> uint16_t zone, *pzone = NULL;
> char *name;
> - int error;
> + int error, i = 1;
> + bool get_dpif = false;
I think "got_dpif" is better, since it indicates that it was successful.
Otherwise, it sounds like something that's planned to be done.
> + } else if (argc == 4) {
> + dpctl_error(dpctl_p, error, "Invalid datapath");
> + return error;
> + }
> ...
> + if (error) {
> + dpctl_error(dpctl_p, error, "opening datapath");
> + return error;
> + }
There's some inconsistencies in capitalizing the first letter of error
messages. I think we more commonly don't capitalize them.
> diff --git a/lib/dpctl.man b/lib/dpctl.man
> index 675fe5af4914..ce202418a0ee 100644
> --- a/lib/dpctl.man
> +++ b/lib/dpctl.man
> @@ -217,10 +217,22 @@ are included. With \fB\-\-statistics\fR timeouts and
> timestamps are
> added to the output.
> .
> .TP
> -\*(DX\fBflush\-conntrack\fR [\fIdp\fR] [\fBzone=\fIzone\fR]
> -Flushes all the connection entries in the tracker used by \fIdp\fR.
> -If \fBzone=\fIzone\fR is specified, only flushes the connections in
> -\fBzone\fR.
> +\*(DX\fBflush\-conntrack\fR [\fIdp\fR] [\fBzone=\fIzone\fR] [\fIct-tuple\fR]
> +Flushes the connection entries in the tracker used by \fIdp\fR based on
> +\fIzone\fR and connection tracking tuple (\fIct-tuple\fR).
I don't think the parentheses are necessary around "ct-tuple".
I've attached an incremental patch to this message. Can you take a look? If
you agree with the changes, I'll commit this patch and the first from this
series.
Thanks,
--Justin
-=-=-=-=-=-=-=-=-
diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
index 0aed5bb6b43c..239c848d3735 100644
--- a/lib/ct-dpif.c
+++ b/lib/ct-dpif.c
@@ -498,7 +498,7 @@ ct_dpif_parse_tuple(struct ct_dpif_tuple *tuple, const char
*s, struct ds *ds)
!strcmp(key, "icmp_id") ) {
if (tuple->ip_proto != IPPROTO_ICMP &&
tuple->ip_proto != IPPROTO_ICMPV6) {
- ds_put_cstr(ds, "Invalid L4 fields");
+ ds_put_cstr(ds, "invalid L4 fields");
goto error;
}
uint16_t icmp_id;
@@ -515,8 +515,8 @@ ct_dpif_parse_tuple(struct ct_dpif_tuple *tuple, const char
*s, struct ds *ds)
free(err);
goto error_with_msg;
}
- }else {
- ds_put_format(ds, "Invalid conntrack tuple field: %s", key);
+ } else {
+ ds_put_format(ds, "invalid conntrack tuple field: %s", key);
goto error;
}
}
@@ -525,9 +525,9 @@ ct_dpif_parse_tuple(struct ct_dpif_tuple *tuple, const char
*s, struct ds *ds)
!tuple->ip_proto) {
/* icmp_type, icmp_code, and icmp_id can be 0. */
if (tuple->ip_proto != IPPROTO_ICMP &&
- tuple->ip_proto != IPPROTO_ICMP) {
+ tuple->ip_proto != IPPROTO_ICMPV6) {
if (!tuple->src_port || !tuple->dst_port) {
- ds_put_cstr(ds, "At least one of the conntrack 5-tuple fields "
+ ds_put_cstr(ds, "at least one of the conntrack 5-tuple fields "
"is missing.");
goto error;
}
@@ -538,7 +538,7 @@ ct_dpif_parse_tuple(struct ct_dpif_tuple *tuple, const char
*s, struct ds *ds)
return true;
error_with_msg:
- ds_put_format(ds, "Failed to parse field %s", key);
+ ds_put_format(ds, "failed to parse field %s", key);
error:
free(copy);
return false;
diff --git a/lib/dpctl.c b/lib/dpctl.c
index 06cfbc4abf21..867b42105130 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -1336,7 +1336,7 @@ dpctl_flush_conntrack(int argc, const char *argv[],
uint16_t zone, *pzone = NULL;
char *name;
int error, i = 1;
- bool get_dpif = false;
+ bool got_dpif = false;
/* Parse datapath name. It is not a mandatory parameter for this command.
* If it is not specified, we retrieve it from the current setup,
@@ -1344,14 +1344,14 @@ dpctl_flush_conntrack(int argc, const char *argv[],
if (argc >= 2) {
error = parsed_dpif_open(argv[i], false, &dpif);
if (!error) {
- get_dpif = true;
+ got_dpif = true;
i++;
} else if (argc == 4) {
- dpctl_error(dpctl_p, error, "Invalid datapath");
+ dpctl_error(dpctl_p, error, "invalid datapath");
return error;
}
}
- if (!get_dpif) {
+ if (!got_dpif) {
name = get_one_dp(dpctl_p);
if (!name) {
return EINVAL;
@@ -1371,7 +1371,7 @@ dpctl_flush_conntrack(int argc, const char *argv[],
}
/* Report error if there are more than one unparsed argument. */
if (argc - i > 1) {
- ds_put_cstr(&ds, "Invalid zone");
+ ds_put_cstr(&ds, "invalid zone");
error = EINVAL;
goto error;
}
@@ -1381,7 +1381,7 @@ dpctl_flush_conntrack(int argc, const char *argv[],
ptuple = &tuple;
i++;
}
- /* Report error if there is a unparsed argument. */
+ /* Report error if there is an unparsed argument. */
if (argc - i) {
error = EINVAL;
goto error;
@@ -1392,7 +1392,7 @@ dpctl_flush_conntrack(int argc, const char *argv[],
dpif_close(dpif);
return 0;
} else {
- ds_put_cstr(&ds, "Failed to flush conntrack");
+ ds_put_cstr(&ds, "failed to flush conntrack");
}
error:
diff --git a/lib/dpctl.man b/lib/dpctl.man
index ce202418a0ee..d7c95ff18317 100644
--- a/lib/dpctl.man
+++ b/lib/dpctl.man
@@ -219,7 +219,7 @@ added to the output.
.TP
\*(DX\fBflush\-conntrack\fR [\fIdp\fR] [\fBzone=\fIzone\fR] [\fIct-tuple\fR]
Flushes the connection entries in the tracker used by \fIdp\fR based on
-\fIzone\fR and connection tracking tuple (\fIct-tuple\fR).
+\fIzone\fR and connection tracking tuple \fIct-tuple\fR.
If \fIct-tuple\fR is not provided, flushes all the connection entries.
If \fBzone\fR=\fIzone\fR is specified, only flushes the connections in
\fIzone\fR.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
