Re: [PATCH 3/4] tcp, tcp_splice: CONN_IDX subtraction of pointers isn't always long

30 Nov 2023

On Thu, 30 Nov 2023 11:27:21 +1100
David Gibson  wrote:
...
On Wed, Nov 29, 2023 at 02:58:42PM +0100, Stefano Brivio wrote:
...
On Wed, 29 Nov 2023 14:46:09 +0100
Stefano Brivio  wrote:
...
On 32-bit architectures, it's a regular int. C99 introduced ptrdiff_t
for this case, with a matching length modifier, 't'.
Signed-off-by: Stefano Brivio 
---
 tcp.c        | 39 +++++++++++++++++++++------------------
 tcp_splice.c | 14 +++++++-------
 2 files changed, 28 insertions(+), 25 deletions(-)

diff --git a/tcp.c b/tcp.c
index 44468ca..c32c9cb 100644
--- a/tcp.c
+++ b/tcp.c
@@ -727,7 +727,7 @@ static void tcp_timer_ctl(const struct ctx *c, struct tcp_tap_conn *conn)
      it.it_value.tv_sec = ACT_TIMEOUT;
  }
-	debug("TCP: index %li, timer expires in %lu.%03lus", CONN_IDX(conn),
+	debug("TCP: index %ti, timer expires in %lu.%03lus", CONN_IDX(conn),
[...]
Oops, I just realised this clashes with your "[PATCH v2 03/11] flow,
tcp: Consolidate flow pointer<->index helpers".
And then a bunch will be obsoleted by "flow, tcp: Add logging helpers
for connection related messages".
...
There, however, I guess that the new flow_idx() should return ptrdiff_t,
which is signed.
Actually, no, I don't think so.  Yes the expression that generates it
is naturally of type ptrdiff_t.  But it's a bug to call flow_idx() on
something not in the flow table, and places where we want to pass *in*
a flow table index it makes more sense for it to be unsigned.  So I
think flow indices should be unsigned throughout.
I also think it should be unsigned (s/which is signed/which happens to
be signed/ in my comment above). At the same time there's no such thing
as an unsigned version of ptrdiff_t, so, given the other choices, I
would still argue that we should use ptrdiff_t.
...
...
I can drop this patch if you re-spin it (assuming it makes sense to
you), or I can adapt it on top of your patch -- whatever is most
convenient for you.
I have a couple of reasons to re-spin anyway.  So how about you drop
this, and I'll double check that I get the format specifiers sane
after my series?
Sure, dropping this.

-- 
Stefano

    