[ovs-dev] [PATCH v2 01/11] ct-dpif: New module.

Joe Stringer joestringer at nicira.com
Tue Nov 17 22:26:59 UTC 2015


On 5 November 2015 at 19:12, Daniele Di Proietto <diproiettod at vmware.com> wrote:
> This defines some structures (and their related formatting functions) to
> manipulate entries in connection tracking tables.
>
> It will be used by next commits.
>
> Based on original work by Jarno Rajahalme
>
> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
> Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>

One thought that came to mind, I don't know if this might help the
parseability of the output but should we comma-separate all the ct
info when formatting it? I think that would be more consistent with
the other parts of OVS.

Minor extra comments:

> +static void
> +ct_dpif_format_ipproto(struct ds *ds, uint16_t ipproto)
> +{
> +    const char *name;
> +
> +    name = (ipproto == IPPROTO_ICMP) ? "icmp"
> +        : (ipproto == IPPROTO_ICMPV6) ? "icmpv6"
> +        : (ipproto == IPPROTO_TCP) ? "tcp"
> +        : (ipproto == IPPROTO_UDP) ? "udp"
> +        : (ipproto == IPPROTO_SCTP) ? "sctp"
> +        : NULL;

Is it worth sharing this with the similar code in match_format()? It
could live in lib/packets.h.

I had some minor style changes when I went through this, mostly what I
saw as redundant comments:

diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
index 9a23a5cf65ec..d63e7a1de40b 100644
--- a/lib/ct-dpif.c
+++ b/lib/ct-dpif.c
@@ -118,7 +118,6 @@ ct_dpif_flush(struct dpif *dpif, const uint16_t *zone)
             : EOPNOTSUPP);
 }

-/* Free memory held by 'entry'. */
 void
 ct_dpif_entry_uninit(struct ct_dpif_entry *entry)
 {
@@ -129,9 +128,6 @@ ct_dpif_entry_uninit(struct ct_dpif_entry *entry)
     }
 }
 ^L
-/* Conntrack entry formatting. */
-
-/* Format conntrack 'entry' of 'type' to 'ds'. */
 void
 ct_dpif_format_entry(const struct ct_dpif_entry *entry, struct ds *ds,
                      bool verbose, bool print_stats)
@@ -186,8 +182,6 @@ ct_dpif_format_entry(const struct ct_dpif_entry
*entry, struct ds *ds,
         ds_put_cstr(ds, ")");
     }
 }
-^L
-/* Formatters for the parts of the conntrack entries. */

 static void
 ct_dpif_format_ipproto(struct ds *ds, uint16_t ipproto)
@@ -283,17 +277,15 @@ static void
 ct_dpif_format_flags(struct ds *ds, const char *title, uint32_t flags,
                      const struct flags *table)
 {
-    bool first = true;
-
     if (title) {
         ds_put_cstr(ds, title);
     }
     for (; table->name; table++) {
         if (flags & table->flag) {
-            ds_put_format(ds, first ? "%s" : ",%s", table->name);
-            first = false;
+            ds_put_format(ds, "%s,", table->name);
         }
     }
+    ds_chomp(ds, ',');
 }

 static const struct flags tcp_flags[] = {



More information about the dev mailing list