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

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 30 Nov 2023 11:27:21 +1100
David Gibson <david@gibson.dropbear.id.au> 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 <sbrivio@redhat.com> 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 <sbrivio@redhat.com>
> > > ---
> > >  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