Thanks Justin for reviewing this series. I will repin v2 based on your comments.
On Wed, Nov 15, 2017 at 4:28 PM, Justin Pettit <[email protected]> wrote: > > --- a/lib/ct-dpif.c > > +++ b/lib/ct-dpif.c > > @@ -111,19 +111,30 @@ ct_dpif_dump_done(struct ct_dpif_dump_state *dump) > > } > > > > /* Flush the entries in the connection tracker used by 'dpif'. > > - * > > - * If 'zone' is not NULL, flush only the entries in '*zone'. */ > > + * If both 'zone' and 'tuple' are NULL, flush all the conntrack entries. > > + * If 'zone' is not NULL, and 'tuple's is NULL, flush all the conntrack > > I'd drop the "s" in "'tuple's". > Sure. > > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > > index 599308d257a8..0e1696a94e31 100644 > > --- a/lib/dpif-netdev.c > > +++ b/lib/dpif-netdev.c > > @@ -5741,10 +5741,14 @@ dpif_netdev_ct_dump_done(struct dpif *dpif > OVS_UNUSED, > > } > > > > static int > > -dpif_netdev_ct_flush(struct dpif *dpif, const uint16_t *zone) > > +dpif_netdev_ct_flush(struct dpif *dpif, const uint16_t *zone, > > + const struct ct_dpif_tuple *tuple) > > { > > struct dp_netdev *dp = get_dp_netdev(dpif); > > > > + if (tuple) { > > + return EOPNOTSUPP; > > + } > > return conntrack_flush(&dp->conntrack, zone); > > } > > Do you plan on implementing this? > Yes. Will have that in v2. > > diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h > > index 1d82a0939aa1..083d67c8d5b0 100644 > > --- a/lib/dpif-provider.h > > +++ b/lib/dpif-provider.h > > ... > > - /* Flushes the connection tracking tables. If 'zone' is not NULL, > > - * only deletes connections in '*zone'. */ > > - int (*ct_flush)(struct dpif *, const uint16_t *zone); > > + /* Flushes the connection tracking tables. > > + * If both 'zone' and 'tuple' are NULL, flush all the conntrack > entries. > > + * If 'zone' is not NULL, and 'tuple's is NULL, flush all the > conntrack > > + * entries in '*zone'. > > Same comment as above about "'tuple's'". > Done in v2. > > > diff --git a/lib/netlink-conntrack.c b/lib/netlink-conntrack.c > > index 1e1bb2f79d1d..9094779aaa55 100644 > > --- a/lib/netlink-conntrack.c > > +++ b/lib/netlink-conntrack.c > > .... > > +int > > +nl_ct_delete(const struct ct_dpif_tuple *tuple, uint16_t zone) > > +{ > > + int err; > > + struct ofpbuf buf; > > + > > + ofpbuf_init(&buf, NL_DUMP_BUFSIZE); > > + nl_msg_put_nfgenmsg(&buf, 0, tuple->l3_type, NFNL_SUBSYS_CTNETLINK, > > + IPCTNL_MSG_CT_DELETE, NLM_F_REQUEST); > > + > > + nl_msg_put_be16(&buf, CTA_ZONE, htons(zone)); > > + if (!nl_ct_put_ct_tuple(&buf, tuple, CTA_TUPLE_ORIG)) { > > + err = EOPNOTSUPP; > > + goto out; > > + } > > + err = nl_transact(NETLINK_NETFILTER, &buf, NULL); > > +out: > > + ofpbuf_uninit(&buf); > > + return err; > > +} > > The Linux code for nl_ct_flush_zone() seems to go through some contortions > due to there being a problem with flushing a specific zone. I took a brief > look at the kernel code, and it looks like it does properly handle zones if > the tuple is provided, but not otherwise. Is that a correct reading? > Yes, that is correct. On a more cosmetic note, most of the external interface refers to deleting > a specific conntrack entry as a "flush", but the internal code refers to it > as a "delete". I think this is a reasonable distinction, but if it's not > carried externally, then it doesn't seem necessary internally. > Right, it make sense to rename the "delete" to "flush" to make it consistent. Also, this delete and flush code is nearly identical, so it seems like > there's some opportunity to just call it "flush" and consolidate to a > single implementation that handles a tuple if it's provided. > I am more incline to rename the nl_ct_delete() to nl_ct_flush_tuple(). I agree that there are some code duplication in the delete and flush code, but consolidate them to a single implementation will lead to the following two functions. It is a bit confusing since the first one will either flush all entries or flush by a tuple. - nl_ct_flush(const struct ct_dpif_tuple *tupe, uint16_t zone); // flush all or a tuple - nl_ct_flush_zone(uint16_t zone) // flush a particular zone Instead, if we rename nl_ct_delete() to nl_ct_flush_tuple(), we will have the following three functions, and it seems to be more clear to me. - nl_ct_flush(void) // flush all - nl_ct_flush_zone(uint16_t zone) // flush a particular zone - nl_ct_flush_tuple(const struct ct_dpif_tuple *tuple, uint16_t zone) // flush a tuple > +static bool > > +nl_ct_put_tuple(struct ofpbuf *buf, const struct ct_dpif_tuple *tuple) > > +{ > > + if (!nl_ct_put_tuple_ip(buf, tuple)) { > > + return false; > > + } > > + if (!nl_ct_put_tuple_proto(buf, tuple)) { > > + return false; > > + } > > + return true; > > +} > > + > > +static bool > > +nl_ct_put_ct_tuple(struct ofpbuf *buf, const struct ct_dpif_tuple > *tuple, > > + enum ctattr_type type) > > +{ > > + if (type != CTA_TUPLE_ORIG && type != CTA_TUPLE_REPLY && > > + type != CTA_TUPLE_MASTER) { > > + return false; > > + } > > + > > + size_t offset = nl_msg_start_nested(buf, type); > > + if (!nl_ct_put_tuple(buf, tuple)) { > > + return false; > > + } > > + > > + nl_msg_end_nested(buf, offset); > > + return true; > > +} > > The names for nl_ct_put_ct_tuple() and nl_ct_put_tuple() are very > similar. nl_ct_put_tuple() doesn't do much, what about just folding that > into nl_ct_put_ct_tuple()? > Sure, I will fold that into nl_ct_put_ct_tuple() in v2. Thanks, -Yi-Hung _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
