> On Nov 13, 2017, at 12:13 PM, Yi-Hung Wei <[email protected]> wrote:
>
> With this patch, ovs-dpctl flush-conntrack accepts a conntrack 5-tuple
I'd also mention ovs-appctl, since ovs-dpctl doesn't work on all platforms.
> diff --git a/NEWS b/NEWS
> index 047f34b9f402..800eb84cd24f 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -11,6 +11,8 @@ Post-v2.8.0
> IPv6 packets.
> - Linux kernel 4.13
> * Add support for compiling OVS with the latest Linux 4.13 kernel
> + - "ovs-dpctl flush-conntrack" now accepts conntrack 5-tuple to delete a
> + connection tracking entry.
I would also mention the ovs-appctl, too. Maybe:
"flush-conntrack" in ovs-dpctl and ovs-appctl now accept a 5-tuple to
delete a specific connection tracking entry.
> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
> index a9f58f4eb449..bdd9ef5d2551 100644
> --- a/lib/ct-dpif.c
> +++ b/lib/ct-dpif.c
> @@ -20,6 +20,7 @@
> #include <errno.h>
>
> #include "ct-dpif.h"
> +#include "openvswitch/ofp-parse.h"
> #include "openvswitch/vlog.h"
>
> VLOG_DEFINE_THIS_MODULE(ct_dpif);
> @@ -434,3 +435,83 @@ 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 msg
> + * in 'ds'.
> + */
We would normally close the function comment on the previous line. Also, I
wouldn't abbreviate "message" in a comment.
> +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 || !tuple->src_port || !tuple->dst_port) {
> + ds_put_cstr(ds, "Miss at least one of the conntrack 5-tuple field");
I think this error message might be a little clearer: "At least one of the
conntrack 5-tuple fields is missing."
> + goto out2;
> + }
> +
> + free(copy);
> + return true;
> +
> +out:
> + ds_put_format(ds, "Failed to parse field %s", key);
> +out2:
> + free(copy);
> + return false;
Minor, but I think these labels could be more descriptive. What about
"error_with_msg" and "error" or something?
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 7569189d8f22..682b9bf35a75 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -1331,14 +1331,32 @@ 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;
>
> + /* Parse ct tuple */
> + if (argc > 1 && ct_dpif_parse_tuple(&tuple, argv[argc-1], &ds)) {
> + ptuple = &tuple;
> + argc--;
> + } else if (argc == 4) {
> + dpctl_error(dpctl_p, EINVAL, "%s", ds_cstr(&ds));
> + ds_destroy(&ds);
> + return EINVAL;
> + }
> + ds_destroy(&ds);
I don't think this handles the case of an error parsing 'tuple' when a datapath
name and zone isn't provided. It also reads a little funny, since 'ds' can
only have a value if there was an error, but it's called even on the non-error
condition. It's obviously correct, since it's a safe operation, but it's
unnecessary.
> +
> + /* Parse zone */
> if (argc > 1 && ovs_scan(argv[argc - 1], "zone=%"SCNu16, &zone)) {
> pzone = &zone;
> argc--;
> + } else if (argc == 3) {
> + dpctl_error(dpctl_p, EINVAL, "Invalid zone or conntrack tuple");
> + return EINVAL;
Same thing here about this only triggering if there wasn't a datapath name.
> diff --git a/lib/dpctl.man b/lib/dpctl.man
> index 675fe5af4914..f429f1653fa7 100644
> --- a/lib/dpctl.man
> +++ b/lib/dpctl.man
> @@ -217,10 +217,20 @@ 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.
> +\*(DX\fBflush\-conntrack\fR [\fIdp\fR] [\fBzone=\fIzone\fR] [\fBct-tuple\fR]
Shouldn't "ct-tuple" (and all the references below) be using "\fI", since it
should be an argument?
> +Flushes the connection entries in the tracker used by \fIdp\fR based on
> +\fIzone\fR and connection tracking tuple (\fBct-tuple\fR).
> +If \fBct-tuple\fR is not provided, flushes all the connection entries.
> If \fBzone=\fIzone\fR is specified, only flushes the connections in
> \fBzone\fR.
> +.IP
> +If \fBct-tuple\fR is provided, flushes the connection entry specified by
> +\fBct-tuple\fR in \fIzone\fR. \fBZone\fR defaults to \fBzone 0\fR if it is
I don't think you need to bold anything on this line. What about changing that
sentence to "The zone defaults to 0 if it is..."?
> +not provided.
> +Example of a IPv4 \fBct-tuple\fR:
I would say "An example of an IPv4 \fIct-tuple\fR:"
> +"ct_nw_src=10.1.1.1,ct_nw_dst=10.1.1.2,ct_nw_proto=6,ct_tp_src=1,ct_tp_dst=2".
I think it might be easier to read if you added a ".IP" here.
Thanks,
--Justin
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
