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 <[email protected]>
> ---
>  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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to