> On Nov 13, 2017, at 12:13 PM, Yi-Hung Wei <yihung....@gmail.com> 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 d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev