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