Re: [PATCH v3 08/16] flow: Introduce 'sidx' type to represent one side of one flow

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 30 Nov 2023 13:02:14 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> In a number of places, we use indices into the flow table to identify a
> specific flow.  We also have cases where we need to identify a particular
> side of a particular flow, and we expect those to become more common as
> we generalise the flow table to cover more things.
> 
> To assist with that, introduces flow_sidx_t, an index type which identifies
> a specific side of a specific flow in the table.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  flow.h       | 14 ++++++++++++++
>  flow_table.h | 36 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/flow.h b/flow.h
> index c820a15..4f12831 100644
> --- a/flow.h
> +++ b/flow.h
> @@ -39,6 +39,20 @@ struct flow_common {
>  #define FLOW_TABLE_PRESSURE		30	/* % of FLOW_MAX */
>  #define FLOW_FILE_PRESSURE		30	/* % of c->nofile */
>  
> +/**
> + * struct flow_sidx - ID for one side of a specific flow
> + * @side:	Side referenced (0 or 1)
> + * @flow:	Index of flow referenced
> + */
> +typedef struct flow_sidx {
> +	int		side :1;
> +	unsigned	flow :FLOW_INDEX_BITS;
> +} flow_sidx_t;
> +static_assert(sizeof(flow_sidx_t) <= sizeof(uint32_t),
> +	      "flow_sidx_t must fit within 32 bits");
> +
> +#define FLOW_SIDX_NONE ((flow_sidx_t){ .flow = FLOW_MAX })
> +
>  union flow;
>  
>  void flow_table_compact(struct ctx *c, union flow *hole);
> diff --git a/flow_table.h b/flow_table.h
> index 5e897bd..3c68d4a 100644
> --- a/flow_table.h
> +++ b/flow_table.h
> @@ -47,4 +47,40 @@ static inline unsigned flow_idx(const struct flow_common *f)
>   */
>  #define FLOW(idx)		(&flowtab[(idx)])
>  
> +/** flow_at_sidx - Flow entry for a given sidx
> + * @sidx:	Flow & side index
> + *
> + * Return: pointer to the corresponding flow entry, or NULL
> + */
> +static inline union flow *flow_at_sidx(flow_sidx_t sidx)
> +{
> +	if (sidx.flow >= FLOW_MAX)
> +		return NULL;
> +	return FLOW(sidx.flow);
> +}
> +
> +/** flow_sidx_t - Index of one side of a flow from common structure
> + * @f:		Common flow fields pointer
> + * @side:	Which side to refer to (0 or 1)
> + *
> + * Return: index of @f and @side in the flow table
> + */
> +static inline flow_sidx_t flow_sidx(const struct flow_common *f,
> +				    int side)
> +{
> +	ASSERT(side == !!side);

This gives me a false cppcheck (2.10) positive:

flow_table.h:71:2: style: The comparison 'side == !!side' is always true because 'side' and '!!side' represent the same value. [knownConditionTrueFalse]
 ASSERT(side == !!side);
 ^

I guess cppcheck commit 5d1fdf795829 ("Fix issue 7904: Handle double
nots in isSameExpression (#1305)") should be extended to also skip
checking double nots in equality comparisons:

diff --git a/lib/astutils.cpp b/lib/astutils.cpp
index 2c01884c9..8e88ec817 100644
--- a/lib/astutils.cpp
+++ b/lib/astutils.cpp
@@ -1424,10 +1424,10 @@ bool isSameExpression(bool cpp, bool macro, const Token *tok1, const Token *tok2
             tok2 = tok2->astOperand2();
     }
     // Skip double not
-    if (Token::simpleMatch(tok1, "!") && Token::simpleMatch(tok1->astOperand1(), "!") && !Token::simpleMatch(tok1->astParent(), "=")) {
+    if (Token::simpleMatch(tok1, "!") && Token::simpleMatch(tok1->astOperand1(), "!") && !Token::simpleMatch(tok1->astParent(), "=") && !Token::simpleMatch(tok1->astParent(), "==")) {
         return isSameExpression(cpp, macro, tok1->astOperand1()->astOperand1(), tok2, library, pure, followVar, errors);
     }
-    if (Token::simpleMatch(tok2, "!") && Token::simpleMatch(tok2->astOperand1(), "!") && !Token::simpleMatch(tok2->astParent(), "=")) {
+    if (Token::simpleMatch(tok2, "!") && Token::simpleMatch(tok2->astOperand1(), "!") && !Token::simpleMatch(tok2->astParent(), "=") && !Token::simpleMatch(tok2->astParent(), "==")) {
         return isSameExpression(cpp, macro, tok1, tok2->astOperand1()->astOperand1(), library, pure, followVar, errors);
     }
     const bool tok_str_eq = tok1->str() == tok2->str();

...anyway, I would just add an inline suppression for the moment, unless
you have better ideas.

-- 
@@ -1424,10 +1424,10 @@ bool isSameExpression(bool cpp, bool macro, const Token *tok1, const Token *tok2
             tok2 = tok2->astOperand2();
     }
     // Skip double not
-    if (Token::simpleMatch(tok1, "!") && Token::simpleMatch(tok1->astOperand1(), "!") && !Token::simpleMatch(tok1->astParent(), "=")) {
+    if (Token::simpleMatch(tok1, "!") && Token::simpleMatch(tok1->astOperand1(), "!") && !Token::simpleMatch(tok1->astParent(), "=") && !Token::simpleMatch(tok1->astParent(), "==")) {
         return isSameExpression(cpp, macro, tok1->astOperand1()->astOperand1(), tok2, library, pure, followVar, errors);
     }
-    if (Token::simpleMatch(tok2, "!") && Token::simpleMatch(tok2->astOperand1(), "!") && !Token::simpleMatch(tok2->astParent(), "=")) {
+    if (Token::simpleMatch(tok2, "!") && Token::simpleMatch(tok2->astOperand1(), "!") && !Token::simpleMatch(tok2->astParent(), "=") && !Token::simpleMatch(tok2->astParent(), "==")) {
         return isSameExpression(cpp, macro, tok1, tok2->astOperand1()->astOperand1(), library, pure, followVar, errors);
     }
     const bool tok_str_eq = tok1->str() == tok2->str();

...anyway, I would just add an inline suppression for the moment, unless
you have better ideas.

-- 
Stefano