[ovs-dev] [PATCH 2/2] dpctl: Support flush conntrack by conntrack 5-tuple
Yi-Hung Wei
yihung.wei at gmail.com
Wed Nov 22 01:01:28 UTC 2017
On Wed, Nov 15, 2017 at 5:28 PM, Justin Pettit <jpettit at ovn.org> 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.
>
Sure.
> diff --git a/NEWS b/NEWS
> > index 047f34b9f402..800eb84cd24f 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -11,6 +11,8 @@ Post-v2.8.0
> > * 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.
>
Sure.
> 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.
>
Thanks, done in v2.
> +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."
>
Sure, it is more clear.
> > + 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?
>
Sounds good. I will fold in that in v2.
> 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.
>
I rewrite the error reporting logic in v2. It catches more error cases, but
it also makes the code to be more complicated since all the 3 parameters
are optional on this command. I hope it can reduce the chance to
accidentally flush conntrack entries.
> 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?
>
Yes, I will fold in all the suggestion in dpctl.man in v2.
Thanks,
-Yi-Hung
More information about the dev
mailing list