[ovs-dev] [PATCH v2 1/3] ct-dpif, dpif-netlink: Support conntrack flush by ct 5-tuple

[Justin Pettit]

> On Nov 21, 2017, at 5:00 PM, Yi-Hung Wei <yihung.wei at gmail.com> wrote:
> 
> This patch adds support of flushing a conntrack entry specified by the
> conntrack 5-tuple, and provides the implementation in dpif-netlink.
> The implementation of dpif-netlink in the linux datapath utilizes the
> NFNL_SUBSYS_CTNETLINK netlink subsystem to delete a conntrack entry in
> nf_conntrack.

It would be good to mention that this patch doesn't support the userspace or Windows datapaths.  I'd like to add the following sentence to the commit message:

	Future patches will add support for the userspace and Windows datapaths.

> diff --git a/lib/netlink-conntrack.c b/lib/netlink-conntrack.c
> index 1e1bb2f79d1d..1f0b9121036d 100644
> --- a/lib/netlink-conntrack.c
> +++ b/lib/netlink-conntrack.c
> 
> ...
> +int
> +nl_ct_flush_tuple(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));

When reviewing this patch, I noticed an issue with how Windows was handling conntrack zones for flush.  I sent out a patch:

	https://mail.openvswitch.org/pipermail/ovs-dev/2017-December/341610.html

It's not a blocker for this patch, since this series doesn't add support for flushing by 5-tuple on Windows.  However, I wanted to point out that it will need to be fixed before that support is added.

I thought a couple of the function descriptions could be clearer with a bit more formatting and slight changes to the text.  I've appended those as a patch to this message.

If you agree with my suggestions, I'll commit this patch with those changes.

Thanks,

--Justin


-=-=-=-=-=-=-=-=-


diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
index 5cd6b5cfd870..cee4791565fb 100644
--- a/lib/ct-dpif.c
+++ b/lib/ct-dpif.c
@@ -110,13 +110,14 @@ ct_dpif_dump_done(struct ct_dpif_dump_state *dump)
             : EOPNOTSUPP);
 }
 ^L
-/* Flush the entries in the connection tracker used by 'dpif'.
- * If both 'zone' and 'tuple' are NULL, flush all the conntrack entries.
- * If 'zone' is not NULL, and 'tuple' is NULL, flush all the conntrack
- * entries in '*zone'.
- * If 'tuple' is not NULL, flush the conntrack entry specified by 'tuple'
- * in '*zone'. In this case, we use default zone (zone 0) if 'zone' is
- * NULL. */
+/* Flush the entries in the connection tracker used by 'dpif'.  The
+ * arguments have the following behavior:
+ *
+ *   - If both 'zone' and 'tuple' are NULL, flush all the conntrack entries.
+ *   - If 'zone' is not NULL, and 'tuple' is NULL, flush all the conntrack
+ *     entries in '*zone'.
+ *   - If 'tuple' is not NULL, flush the conntrack entry specified by 'tuple'
+ *     in '*zone'. If 'zone' is NULL, use the default zone (zone 0). */
 int
 ct_dpif_flush(struct dpif *dpif, const uint16_t *zone,
               const struct ct_dpif_tuple *tuple)
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index 33d7f2a64367..947bf5e31362 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -425,13 +425,16 @@ struct dpif_class {
                         struct ct_dpif_entry *entry);
     int (*ct_dump_done)(struct dpif *, struct ct_dpif_dump_state *state);
 
-    /* Flushes the connection tracking tables.
-     * If both 'zone' and 'tuple' are NULL, flush all the conntrack entries.
-     * If 'zone' is not NULL, and 'tuple' is NULL, flush all the conntrack
-     * entries in '*zone'.
-     * If 'tuple' is not NULL, flush the conntrack entry specified by 'tuple'
-     * in '*zone'. In this case, we use default zone (zone 0) if 'zone' is
-     * NULL. */
+    /* Flushes the connection tracking tables.  The arguments have the
+     * following behavior:
+     *
+     *   - If both 'zone' and 'tuple' are NULL, flush all the conntrack
+     *     entries.
+     *   - If 'zone' is not NULL, and 'tuple' is NULL, flush all the
+     *     conntrack entries in '*zone'.
+     *   - If 'tuple' is not NULL, flush the conntrack entry specified by
+     *     'tuple' in '*zone'. If 'zone' is NULL, use the default zone
+     *     (zone 0). */
     int (*ct_flush)(struct dpif *, const uint16_t *zone,
                     const struct ct_dpif_tuple *tuple);