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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3098 bytes --]

On Thu, Nov 30, 2023 at 10:07:44AM +0100, Stefano Brivio wrote:
> 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.

Again, I don't think so.  ptrdiff_t is important because it allows for
the difference of two *unconstrained* pointers.  In our case the
pointer is not unconstrained, and we want to return it as whatever
type we typically use for an index into the connection table, which is
an unsigned int.

> > > 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.
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]