On 9/9/22 13:29, Paolo Valerio wrote:
> Ilya Maximets <i.maxim...@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 <pvale...@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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
