Re: [PATCH 05/32] tcp: Better helpers for converting between connection pointer and index

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 16 Nov 2022 15:41:45 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> The macro CONN_OR_NULL() is used to look up connections by index with
> bounds checking.  Replace it with an inline function, which means:
>     - Better type checking
>     - No danger of multiple evaluation of an @index with side effects
> 
> Also add a helper to perform the reverse translation: from connection
> pointer to index.  Introduce a macro for this which will make later
> cleanups easier and safer.

Ah, yes, much better, agreed. Just two things here:

> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  tcp.c | 83 ++++++++++++++++++++++++++++++++---------------------------
>  1 file changed, 45 insertions(+), 38 deletions(-)
> 
> diff --git a/tcp.c b/tcp.c
> index d043123..4e56a6c 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -518,14 +518,6 @@ struct tcp_conn {
>  	 (conn->events & (SOCK_FIN_RCVD | TAP_FIN_RCVD)))
>  #define CONN_HAS(conn, set)	((conn->events & (set)) == (set))
>  
> -#define CONN(index)		(tc + (index))
> -
> -/* We probably don't want to use gcc statement expressions (for portability), so
> - * use this only after well-defined sequence points (no pre-/post-increments).
> - */
> -#define CONN_OR_NULL(index)						\
> -	(((int)(index) >= 0 && (index) < TCP_MAX_CONNS) ? (tc + (index)) : NULL)
> -
>  static const char *tcp_event_str[] __attribute((__unused__)) = {
>  	"SOCK_ACCEPTED", "TAP_SYN_RCVD", "ESTABLISHED", "TAP_SYN_ACK_SENT",
>  
> @@ -705,6 +697,21 @@ static size_t tcp6_l2_flags_buf_bytes;
>  /* TCP connections */
>  static struct tcp_conn tc[TCP_MAX_CONNS];
>  
> +#define CONN(index)		(tc + (index))
> +#define CONN_IDX(conn)		((conn) - tc)
> +
> +/** conn_at_idx() - Find a connection by index, if present
> + * @index:	Index of connection to lookup
> + *
> + * Return:	Pointer to connection, or NULL if @index is out of bounds

Return: pointer [...]

> + */
> +static inline struct tcp_conn *conn_at_idx(int index)

The CONN_OR_NULL name made it very explicit that the pointer obtained
there could be NULL. On the other hand I find conn_at_idx() more
descriptive. But maybe conn_or_null() would be "safer". I don't really
have a preference.

-- 
Stefano