| Message ID | 165962927482.2700301.731551660914235001.stgit@fed.void |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,1/2] netlink-conntrack: Do not fail to parse if optional TCP protocol attributes are not found. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 8/4/22 18:07, Paolo Valerio wrote: > This patch avoids to show flags_orig/flags_reply key if they have no value. > E.g., the following: > > NEW tcp,orig=([...]),reply=([...]),id=1800618864, > status=CONFIRMED|SRC_NAT_DONE|DST_NAT_DONE,timeout=120, > protoinfo=(state_orig=SYN_SENT,state_reply=SYN_SENT,wscale_orig=7, > wscale_reply=0,flags_orig=WINDOW_SCALE|SACK_PERM,flags_reply=) > > becomes: > > NEW tcp,orig=([...]),reply=([...]),id=1800618864, > status=CONFIRMED|SRC_NAT_DONE|DST_NAT_DONE,timeout=120, > protoinfo=(state_orig=SYN_SENT,state_reply=SYN_SENT,wscale_orig=7, > wscale_reply=0,flags_orig=WINDOW_SCALE|SACK_PERM) > > Signed-off-by: Paolo Valerio <pvalerio@redhat.com> > --- > lib/ct-dpif.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c > index cfc2315e3..f1a375523 100644 > --- a/lib/ct-dpif.c > +++ b/lib/ct-dpif.c > @@ -512,10 +512,16 @@ ct_dpif_format_protoinfo_tcp_verbose(struct ds *ds, > protoinfo->tcp.wscale_orig, > protoinfo->tcp.wscale_reply); > } > - ct_dpif_format_flags(ds, ",flags_orig=", protoinfo->tcp.flags_orig, > - tcp_flags); > - ct_dpif_format_flags(ds, ",flags_reply=", protoinfo->tcp.flags_reply, > - tcp_flags); > + > + if (protoinfo->tcp.flags_orig) { > + ct_dpif_format_flags(ds, ",flags_orig=", protoinfo->tcp.flags_orig, > + tcp_flags); > + } > + > + if (protoinfo->tcp.flags_reply) { > + ct_dpif_format_flags(ds, ",flags_reply=", protoinfo->tcp.flags_reply, > + tcp_flags); > + } Hmm. I'm trying to understand why ct_dpif_format_flags() exists at all. Shouldn't this be just: format_flags_masked(ds, "flags_orig", packet_tcp_flag_to_string, protoinfo->tcp.flags_orig, TCP_FLAGS(OVS_BE16_MAX), TCP_FLAGS(OVS_BE16_MAX)); ? This will change the appearance of the flags, so maybe tcp_flags[] array should be replaced with a simple conversion function. In any case the convention seems to be to print a '0' if the field is empty. See the implementation of format_flags() function. And that seems to make sense in the verbose output. Best regards, Ilya Maximets.
Ilya Maximets <i.maximets@ovn.org> writes: > On 8/4/22 18:07, Paolo Valerio wrote: >> This patch avoids to show flags_orig/flags_reply key if they have no value. >> E.g., the following: >> >> NEW tcp,orig=([...]),reply=([...]),id=1800618864, >> status=CONFIRMED|SRC_NAT_DONE|DST_NAT_DONE,timeout=120, >> protoinfo=(state_orig=SYN_SENT,state_reply=SYN_SENT,wscale_orig=7, >> wscale_reply=0,flags_orig=WINDOW_SCALE|SACK_PERM,flags_reply=) >> >> becomes: >> >> NEW tcp,orig=([...]),reply=([...]),id=1800618864, >> status=CONFIRMED|SRC_NAT_DONE|DST_NAT_DONE,timeout=120, >> protoinfo=(state_orig=SYN_SENT,state_reply=SYN_SENT,wscale_orig=7, >> wscale_reply=0,flags_orig=WINDOW_SCALE|SACK_PERM) >> >> Signed-off-by: Paolo Valerio <pvalerio@redhat.com> >> --- >> lib/ct-dpif.c | 14 ++++++++++---- >> 1 file changed, 10 insertions(+), 4 deletions(-) >> >> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c >> index cfc2315e3..f1a375523 100644 >> --- a/lib/ct-dpif.c >> +++ b/lib/ct-dpif.c >> @@ -512,10 +512,16 @@ ct_dpif_format_protoinfo_tcp_verbose(struct ds *ds, >> protoinfo->tcp.wscale_orig, >> protoinfo->tcp.wscale_reply); >> } >> - ct_dpif_format_flags(ds, ",flags_orig=", protoinfo->tcp.flags_orig, >> - tcp_flags); >> - ct_dpif_format_flags(ds, ",flags_reply=", protoinfo->tcp.flags_reply, >> - tcp_flags); >> + >> + if (protoinfo->tcp.flags_orig) { >> + ct_dpif_format_flags(ds, ",flags_orig=", protoinfo->tcp.flags_orig, >> + tcp_flags); >> + } >> + >> + if (protoinfo->tcp.flags_reply) { >> + ct_dpif_format_flags(ds, ",flags_reply=", protoinfo->tcp.flags_reply, >> + tcp_flags); >> + } > > Hmm. I'm trying to understand why ct_dpif_format_flags() exists at all. > Shouldn't this be just: > > format_flags_masked(ds, "flags_orig", packet_tcp_flag_to_string, > protoinfo->tcp.flags_orig, TCP_FLAGS(OVS_BE16_MAX), > TCP_FLAGS(OVS_BE16_MAX)); > > ? > > This will change the appearance of the flags, so maybe tcp_flags[] array > should be replaced with a simple conversion function. > Uhm, I guess you're right. It seems redundant and could be removed. What about something like this? diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c index cfc2315e3..6f17a26b5 100644 --- a/lib/ct-dpif.c +++ b/lib/ct-dpif.c @@ -35,20 +35,11 @@ static void ct_dpif_format_counters(struct ds *, const struct ct_dpif_counters *); static void ct_dpif_format_timestamp(struct ds *, const struct ct_dpif_timestamp *); -static void ct_dpif_format_flags(struct ds *, const char *title, - uint32_t flags, const struct flags *); static void ct_dpif_format_protoinfo(struct ds *, const char *title, const struct ct_dpif_protoinfo *, bool verbose); static void ct_dpif_format_helper(struct ds *, const char *title, const struct ct_dpif_helper *); - -static const struct flags ct_dpif_status_flags[] = { -#define CT_DPIF_STATUS_FLAG(FLAG) { CT_DPIF_STATUS_##FLAG, #FLAG }, - CT_DPIF_STATUS_FLAGS -#undef CT_DPIF_STATUS_FLAG - { 0, NULL } /* End marker. */ -}; /* Dumping */ @@ -275,6 +266,20 @@ ct_dpif_entry_uninit(struct ct_dpif_entry *entry) } } +static const char * +ct_dpif_status_flags(uint32_t flags) +{ + switch (flags) { +#define CT_DPIF_STATUS_FLAG(FLAG) \ + case CT_DPIF_STATUS_##FLAG: \ + return #FLAG; + CT_DPIF_STATUS_FLAGS +#undef CT_DPIF_TCP_FLAG + default: + return NULL; + } +} + void ct_dpif_format_entry(const struct ct_dpif_entry *entry, struct ds *ds, bool verbose, bool print_stats) @@ -305,8 +310,9 @@ ct_dpif_format_entry(const struct ct_dpif_entry *entry, struct ds *ds, ds_put_format(ds, ",zone=%"PRIu16, entry->zone); } if (verbose) { - ct_dpif_format_flags(ds, ",status=", entry->status, - ct_dpif_status_flags); + format_flags_masked(ds, ",status", ct_dpif_status_flags, + entry->status, CT_DPIF_STATUS_MASK, + CT_DPIF_STATUS_MASK); } if (print_stats) { ds_put_format(ds, ",timeout=%"PRIu32, entry->timeout); @@ -415,28 +421,6 @@ ct_dpif_format_tuple(struct ds *ds, const struct ct_dpif_tuple *tuple) } } -static void -ct_dpif_format_flags(struct ds *ds, const char *title, uint32_t flags, - const struct flags *table) -{ - if (title) { - ds_put_cstr(ds, title); - } - for (; table->name; table++) { - if (flags & table->flag) { - ds_put_format(ds, "%s|", table->name); - } - } - ds_chomp(ds, '|'); -} - -static const struct flags tcp_flags[] = { -#define CT_DPIF_TCP_FLAG(FLAG) { CT_DPIF_TCPF_##FLAG, #FLAG }, - CT_DPIF_TCP_FLAGS -#undef CT_DPIF_TCP_FLAG - { 0, NULL } /* End marker. */ -}; - const char *ct_dpif_tcp_state_string[] = { #define CT_DPIF_TCP_STATE(STATE) [CT_DPIF_TCPS_##STATE] = #STATE, CT_DPIF_TCP_STATES @@ -498,6 +482,20 @@ ct_dpif_format_protoinfo_tcp(struct ds *ds, ct_dpif_format_enum(ds, "state=", tcp_state, ct_dpif_tcp_state_string); } +static const char * +ct_dpif_tcp_flags(uint32_t flags) +{ + switch (flags) { +#define CT_DPIF_TCP_FLAG(FLAG) \ + case CT_DPIF_TCPF_##FLAG: \ + return #FLAG; + CT_DPIF_TCP_FLAGS +#undef CT_DPIF_TCP_FLAG + default: + return NULL; + } +} + static void ct_dpif_format_protoinfo_tcp_verbose(struct ds *ds, const struct ct_dpif_protoinfo *protoinfo) @@ -512,10 +510,14 @@ ct_dpif_format_protoinfo_tcp_verbose(struct ds *ds, protoinfo->tcp.wscale_orig, protoinfo->tcp.wscale_reply); } - ct_dpif_format_flags(ds, ",flags_orig=", protoinfo->tcp.flags_orig, - tcp_flags); - ct_dpif_format_flags(ds, ",flags_reply=", protoinfo->tcp.flags_reply, - tcp_flags); + + format_flags_masked(ds, ",flags_orig", ct_dpif_tcp_flags, + protoinfo->tcp.flags_orig, CT_DPIF_TCPF_MASK, + CT_DPIF_TCPF_MASK); + + format_flags_masked(ds, ",flags_reply", ct_dpif_tcp_flags, + protoinfo->tcp.flags_reply, CT_DPIF_TCPF_MASK, + CT_DPIF_TCPF_MASK); } static void diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h index b59cba962..2848549b0 100644 --- a/lib/ct-dpif.h +++ b/lib/ct-dpif.h @@ -103,6 +103,8 @@ enum ct_dpif_tcp_flags { #undef CT_DPIF_TCP_FLAG }; +#define CT_DPIF_TCPF_MASK ((CT_DPIF_TCPF_MAXACK_SET << 1) - 1) + extern const char *ct_dpif_sctp_state_string[]; #define CT_DPIF_SCTP_STATES \ @@ -173,6 +175,8 @@ enum ct_dpif_status_flags { #undef CT_DPIF_STATUS_FLAG }; +#define CT_DPIF_STATUS_MASK ((CT_DPIF_STATUS_UNTRACKED << 1) - 1) + struct ct_dpif_entry { /* Const members. */ struct ct_dpif_tuple tuple_orig; > In any case the convention seems to be to print a '0' if the field is empty. > See the implementation of format_flags() function. And that seems to make > sense in the verbose output. > > Best regards, Ilya Maximets.
On 9/9/22 13:29, Paolo Valerio wrote: > Ilya Maximets <i.maximets@ovn.org> writes: > >> On 8/4/22 18:07, Paolo Valerio wrote: >>> This patch avoids to show flags_orig/flags_reply key if they have no value. >>> E.g., the following: >>> >>> NEW tcp,orig=([...]),reply=([...]),id=1800618864, >>> status=CONFIRMED|SRC_NAT_DONE|DST_NAT_DONE,timeout=120, >>> protoinfo=(state_orig=SYN_SENT,state_reply=SYN_SENT,wscale_orig=7, >>> wscale_reply=0,flags_orig=WINDOW_SCALE|SACK_PERM,flags_reply=) >>> >>> becomes: >>> >>> NEW tcp,orig=([...]),reply=([...]),id=1800618864, >>> status=CONFIRMED|SRC_NAT_DONE|DST_NAT_DONE,timeout=120, >>> protoinfo=(state_orig=SYN_SENT,state_reply=SYN_SENT,wscale_orig=7, >>> wscale_reply=0,flags_orig=WINDOW_SCALE|SACK_PERM) >>> >>> Signed-off-by: Paolo Valerio <pvalerio@redhat.com> >>> --- >>> lib/ct-dpif.c | 14 ++++++++++---- >>> 1 file changed, 10 insertions(+), 4 deletions(-) >>> >>> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c >>> index cfc2315e3..f1a375523 100644 >>> --- a/lib/ct-dpif.c >>> +++ b/lib/ct-dpif.c >>> @@ -512,10 +512,16 @@ ct_dpif_format_protoinfo_tcp_verbose(struct ds *ds, >>> protoinfo->tcp.wscale_orig, >>> protoinfo->tcp.wscale_reply); >>> } >>> - ct_dpif_format_flags(ds, ",flags_orig=", protoinfo->tcp.flags_orig, >>> - tcp_flags); >>> - ct_dpif_format_flags(ds, ",flags_reply=", protoinfo->tcp.flags_reply, >>> - tcp_flags); >>> + >>> + if (protoinfo->tcp.flags_orig) { >>> + ct_dpif_format_flags(ds, ",flags_orig=", protoinfo->tcp.flags_orig, >>> + tcp_flags); >>> + } >>> + >>> + if (protoinfo->tcp.flags_reply) { >>> + ct_dpif_format_flags(ds, ",flags_reply=", protoinfo->tcp.flags_reply, >>> + tcp_flags); >>> + } >> >> Hmm. I'm trying to understand why ct_dpif_format_flags() exists at all. >> Shouldn't this be just: >> >> format_flags_masked(ds, "flags_orig", packet_tcp_flag_to_string, >> protoinfo->tcp.flags_orig, TCP_FLAGS(OVS_BE16_MAX), >> TCP_FLAGS(OVS_BE16_MAX)); >> >> ? >> >> This will change the appearance of the flags, so maybe tcp_flags[] array >> should be replaced with a simple conversion function. >> > > Uhm, I guess you're right. It seems redundant and could be removed. > What about something like this? The code below looks OK at the first glance. Best regards, Ilya Maximets. > > diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c > index cfc2315e3..6f17a26b5 100644 > --- a/lib/ct-dpif.c > +++ b/lib/ct-dpif.c > @@ -35,20 +35,11 @@ static void ct_dpif_format_counters(struct ds *, > const struct ct_dpif_counters *); > static void ct_dpif_format_timestamp(struct ds *, > const struct ct_dpif_timestamp *); > -static void ct_dpif_format_flags(struct ds *, const char *title, > - uint32_t flags, const struct flags *); > static void ct_dpif_format_protoinfo(struct ds *, const char *title, > const struct ct_dpif_protoinfo *, > bool verbose); > static void ct_dpif_format_helper(struct ds *, const char *title, > const struct ct_dpif_helper *); > - > -static const struct flags ct_dpif_status_flags[] = { > -#define CT_DPIF_STATUS_FLAG(FLAG) { CT_DPIF_STATUS_##FLAG, #FLAG }, > - CT_DPIF_STATUS_FLAGS > -#undef CT_DPIF_STATUS_FLAG > - { 0, NULL } /* End marker. */ > -}; > > /* Dumping */ > > @@ -275,6 +266,20 @@ ct_dpif_entry_uninit(struct ct_dpif_entry *entry) > } > } > > +static const char * > +ct_dpif_status_flags(uint32_t flags) > +{ > + switch (flags) { > +#define CT_DPIF_STATUS_FLAG(FLAG) \ > + case CT_DPIF_STATUS_##FLAG: \ > + return #FLAG; > + CT_DPIF_STATUS_FLAGS > +#undef CT_DPIF_TCP_FLAG > + default: > + return NULL; > + } > +} > + > void > ct_dpif_format_entry(const struct ct_dpif_entry *entry, struct ds *ds, > bool verbose, bool print_stats) > @@ -305,8 +310,9 @@ ct_dpif_format_entry(const struct ct_dpif_entry *entry, struct ds *ds, > ds_put_format(ds, ",zone=%"PRIu16, entry->zone); > } > if (verbose) { > - ct_dpif_format_flags(ds, ",status=", entry->status, > - ct_dpif_status_flags); > + format_flags_masked(ds, ",status", ct_dpif_status_flags, > + entry->status, CT_DPIF_STATUS_MASK, > + CT_DPIF_STATUS_MASK); > } > if (print_stats) { > ds_put_format(ds, ",timeout=%"PRIu32, entry->timeout); > @@ -415,28 +421,6 @@ ct_dpif_format_tuple(struct ds *ds, const struct ct_dpif_tuple *tuple) > } > } > > -static void > -ct_dpif_format_flags(struct ds *ds, const char *title, uint32_t flags, > - const struct flags *table) > -{ > - if (title) { > - ds_put_cstr(ds, title); > - } > - for (; table->name; table++) { > - if (flags & table->flag) { > - ds_put_format(ds, "%s|", table->name); > - } > - } > - ds_chomp(ds, '|'); > -} > - > -static const struct flags tcp_flags[] = { > -#define CT_DPIF_TCP_FLAG(FLAG) { CT_DPIF_TCPF_##FLAG, #FLAG }, > - CT_DPIF_TCP_FLAGS > -#undef CT_DPIF_TCP_FLAG > - { 0, NULL } /* End marker. */ > -}; > - > const char *ct_dpif_tcp_state_string[] = { > #define CT_DPIF_TCP_STATE(STATE) [CT_DPIF_TCPS_##STATE] = #STATE, > CT_DPIF_TCP_STATES > @@ -498,6 +482,20 @@ ct_dpif_format_protoinfo_tcp(struct ds *ds, > ct_dpif_format_enum(ds, "state=", tcp_state, ct_dpif_tcp_state_string); > } > > +static const char * > +ct_dpif_tcp_flags(uint32_t flags) > +{ > + switch (flags) { > +#define CT_DPIF_TCP_FLAG(FLAG) \ > + case CT_DPIF_TCPF_##FLAG: \ > + return #FLAG; > + CT_DPIF_TCP_FLAGS > +#undef CT_DPIF_TCP_FLAG > + default: > + return NULL; > + } > +} > + > static void > ct_dpif_format_protoinfo_tcp_verbose(struct ds *ds, > const struct ct_dpif_protoinfo *protoinfo) > @@ -512,10 +510,14 @@ ct_dpif_format_protoinfo_tcp_verbose(struct ds *ds, > protoinfo->tcp.wscale_orig, > protoinfo->tcp.wscale_reply); > } > - ct_dpif_format_flags(ds, ",flags_orig=", protoinfo->tcp.flags_orig, > - tcp_flags); > - ct_dpif_format_flags(ds, ",flags_reply=", protoinfo->tcp.flags_reply, > - tcp_flags); > + > + format_flags_masked(ds, ",flags_orig", ct_dpif_tcp_flags, > + protoinfo->tcp.flags_orig, CT_DPIF_TCPF_MASK, > + CT_DPIF_TCPF_MASK); > + > + format_flags_masked(ds, ",flags_reply", ct_dpif_tcp_flags, > + protoinfo->tcp.flags_reply, CT_DPIF_TCPF_MASK, > + CT_DPIF_TCPF_MASK); > } > > static void > diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h > index b59cba962..2848549b0 100644 > --- a/lib/ct-dpif.h > +++ b/lib/ct-dpif.h > @@ -103,6 +103,8 @@ enum ct_dpif_tcp_flags { > #undef CT_DPIF_TCP_FLAG > }; > > +#define CT_DPIF_TCPF_MASK ((CT_DPIF_TCPF_MAXACK_SET << 1) - 1) > + > extern const char *ct_dpif_sctp_state_string[]; > > #define CT_DPIF_SCTP_STATES \ > @@ -173,6 +175,8 @@ enum ct_dpif_status_flags { > #undef CT_DPIF_STATUS_FLAG > }; > > +#define CT_DPIF_STATUS_MASK ((CT_DPIF_STATUS_UNTRACKED << 1) - 1) > + > struct ct_dpif_entry { > /* Const members. */ > struct ct_dpif_tuple tuple_orig; > > >> In any case the convention seems to be to print a '0' if the field is empty. >> See the implementation of format_flags() function. And that seems to make >> sense in the verbose output. >> >> Best regards, Ilya Maximets. > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c index cfc2315e3..f1a375523 100644 --- a/lib/ct-dpif.c +++ b/lib/ct-dpif.c @@ -512,10 +512,16 @@ ct_dpif_format_protoinfo_tcp_verbose(struct ds *ds, protoinfo->tcp.wscale_orig, protoinfo->tcp.wscale_reply); } - ct_dpif_format_flags(ds, ",flags_orig=", protoinfo->tcp.flags_orig, - tcp_flags); - ct_dpif_format_flags(ds, ",flags_reply=", protoinfo->tcp.flags_reply, - tcp_flags); + + if (protoinfo->tcp.flags_orig) { + ct_dpif_format_flags(ds, ",flags_orig=", protoinfo->tcp.flags_orig, + tcp_flags); + } + + if (protoinfo->tcp.flags_reply) { + ct_dpif_format_flags(ds, ",flags_reply=", protoinfo->tcp.flags_reply, + tcp_flags); + } } static void
This patch avoids to show flags_orig/flags_reply key if they have no value. E.g., the following: NEW tcp,orig=([...]),reply=([...]),id=1800618864, status=CONFIRMED|SRC_NAT_DONE|DST_NAT_DONE,timeout=120, protoinfo=(state_orig=SYN_SENT,state_reply=SYN_SENT,wscale_orig=7, wscale_reply=0,flags_orig=WINDOW_SCALE|SACK_PERM,flags_reply=) becomes: NEW tcp,orig=([...]),reply=([...]),id=1800618864, status=CONFIRMED|SRC_NAT_DONE|DST_NAT_DONE,timeout=120, protoinfo=(state_orig=SYN_SENT,state_reply=SYN_SENT,wscale_orig=7, wscale_reply=0,flags_orig=WINDOW_SCALE|SACK_PERM) Signed-off-by: Paolo Valerio <pvalerio@redhat.com> --- lib/ct-dpif.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)