| Message ID | CANr6G5wGLe7kkTVWS=XZLqw_JBULh9jn3k3JmymnfE-emZ=WSQ@mail.gmail.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Joe, thanks for the comments, answers inline I'm about to send a v3 to the list. On 17/11/2015 14:26, "Joe Stringer" <joestringer@nicira.com> wrote: >On 5 November 2015 at 19:12, Daniele Di Proietto <diproiettod@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@nicira.com> >> Signed-off-by: Daniele Di Proietto <diproiettod@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. That's a good point. I changed the format to match more closely the rest of OVS (specifically the odp format). That meant replacing spaces with commas and separating flags with "|" instead of ",". > >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. The code in match_format() tries to summarize L3 and L4 type in one field. This function deals exclusively with L4 types. I'd rather leave it as it is, but if you feel strongly about it I can change it later. > >I had some minor style changes when I went through this, mostly what I >saw as redundant comments: I applied the suggested diff, thanks! > >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[] = {
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