[PATCH 0/4] Fix build warnings and errors for 32-bit and musl

[PATCH 0/4] Fix build warnings and errors for 32-bit and musl

[Stefano Brivio]

Stefano Brivio (4):
  treewide: Use 'z' length modifier for size_t/ssize_t conversions
  packet: Offset plus length is not always uint32_t, but it's always
    size_t
  tcp, tcp_splice: CONN_IDX subtraction of pointers isn't always long
  port_fwd, util: Include additional headers to fix build with musl

 netlink.c    |  2 +-
 packet.c     | 14 +++++++-------
 pcap.c       |  4 ++--
 port_fwd.c   |  2 ++
 tap.c        | 12 ++++++------
 tcp.c        | 40 ++++++++++++++++++++++------------------
 tcp_splice.c | 22 +++++++++++-----------
 util.h       |  1 +
 8 files changed, 52 insertions(+), 45 deletions(-)

-- 
2.39.2

[PATCH 1/4] treewide: Use 'z' length modifier for size_t/ssize_t conversions

[Stefano Brivio]

Types size_t and ssize_t are not necessarily long, it depends on the
architecture.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 netlink.c    |  2 +-
 packet.c     | 14 +++++++-------
 pcap.c       |  4 ++--
 tap.c        | 12 ++++++------
 tcp.c        |  3 ++-
 tcp_splice.c |  8 ++++----
 6 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/netlink.c b/netlink.c
index 6cc04a0..379d46e 100644
--- a/netlink.c
+++ b/netlink.c
@@ -130,7 +130,7 @@ static uint32_t nl_send(int s, void *req, uint16_t type,
 	if (n < 0)
 		die("netlink: Failed to send(): %s", strerror(errno));
 	else if (n < len)
-		die("netlink: Short send (%lu of %lu bytes)", n, len);
+		die("netlink: Short send (%zd of %zd bytes)", n, len);
 
 	return nh->nlmsg_seq;
 }
diff --git a/packet.c b/packet.c
index 9589824..12ac76b 100644
--- a/packet.c
+++ b/packet.c
@@ -36,7 +36,7 @@ void packet_add_do(struct pool *p, size_t len, const char *start,
 	size_t idx = p->count;
 
 	if (idx >= p->size) {
-		trace("add packet index %lu to pool with size %lu, %s:%i",
+		trace("add packet index %zu to pool with size %zu, %s:%i",
 		      idx, p->size, func, line);
 		return;
 	}
@@ -48,14 +48,14 @@ void packet_add_do(struct pool *p, size_t len, const char *start,
 	}
 
 	if (start + len > p->buf + p->buf_size) {
-		trace("add packet start %p, length: %lu, buffer end %p, %s:%i",
+		trace("add packet start %p, length: %zu, buffer end %p, %s:%i",
 		      (void *)start, len, (void *)(p->buf + p->buf_size),
 		      func, line);
 		return;
 	}
 
 	if (len > UINT16_MAX) {
-		trace("add packet length %lu, %s:%i", len, func, line);
+		trace("add packet length %zu, %s:%i", len, func, line);
 		return;
 	}
 
@@ -90,7 +90,7 @@ void *packet_get_do(const struct pool *p, size_t idx, size_t offset,
 {
 	if (idx >= p->size || idx >= p->count) {
 		if (func) {
-			trace("packet %lu from pool size: %lu, count: %lu, "
+			trace("packet %zu from pool size: %zu, count: %zu, "
 			      "%s:%i", idx, p->size, p->count, func, line);
 		}
 		return NULL;
@@ -98,7 +98,7 @@ void *packet_get_do(const struct pool *p, size_t idx, size_t offset,
 
 	if (len > UINT16_MAX || len + offset > UINT32_MAX) {
 		if (func) {
-			trace("packet data length %lu, offset %lu, %s:%i",
+			trace("packet data length %zu, offset %zu, %s:%i",
 			      len, offset, func, line);
 		}
 		return NULL;
@@ -106,7 +106,7 @@ void *packet_get_do(const struct pool *p, size_t idx, size_t offset,
 
 	if (p->pkt[idx].offset + len + offset > p->buf_size) {
 		if (func) {
-			trace("packet offset plus length %lu from size %lu, "
+			trace("packet offset plus length %lu from size %zu, "
 			      "%s:%i", p->pkt[idx].offset + len + offset,
 			      p->buf_size, func, line);
 		}
@@ -115,7 +115,7 @@ void *packet_get_do(const struct pool *p, size_t idx, size_t offset,
 
 	if (len + offset > p->pkt[idx].len) {
 		if (func) {
-			trace("data length %lu, offset %lu from length %u, "
+			trace("data length %zu, offset %zu from length %u, "
 			      "%s:%i", len, offset, p->pkt[idx].len,
 			      func, line);
 		}
diff --git a/pcap.c b/pcap.c
index 524612a..501d52d 100644
--- a/pcap.c
+++ b/pcap.c
@@ -101,7 +101,7 @@ void pcap(const char *pkt, size_t len)
 
 	gettimeofday(&tv, NULL);
 	if (pcap_frame(pkt, len, &tv) != 0)
-		debug("Cannot log packet, length %lu", len);
+		debug("Cannot log packet, length %zu", len);
 }
 
 /**
@@ -123,7 +123,7 @@ void pcap_multiple(const struct iovec *iov, unsigned int n, size_t offset)
 	for (i = 0; i < n; i++) {
 		if (pcap_frame((char *)iov[i].iov_base + offset,
 			       iov[i].iov_len - offset, &tv) != 0) {
-			debug("Cannot log packet, length %lu",
+			debug("Cannot log packet, length %zu",
 			      iov->iov_len - offset);
 			return;
 		}
diff --git a/tap.c b/tap.c
index 4f11000..2ceda8d 100644
--- a/tap.c
+++ b/tap.c
@@ -191,7 +191,7 @@ void tap_udp4_send(const struct ctx *c, struct in_addr src, in_port_t sport,
 	memcpy(data, in, len);
 
 	if (tap_send(c, buf, len + (data - buf)) < 0)
-		debug("tap: failed to send %lu bytes (IPv4)", len);
+		debug("tap: failed to send %zu bytes (IPv4)", len);
 }
 
 /**
@@ -214,7 +214,7 @@ void tap_icmp4_send(const struct ctx *c, struct in_addr src, struct in_addr dst,
 	csum_icmp4(icmp4h, icmp4h + 1, len - sizeof(*icmp4h));
 
 	if (tap_send(c, buf, len + (data - buf)) < 0)
-		debug("tap: failed to send %lu bytes (IPv4)", len);
+		debug("tap: failed to send %zu bytes (IPv4)", len);
 }
 
 /**
@@ -278,7 +278,7 @@ void tap_udp6_send(const struct ctx *c,
 	memcpy(data, in, len);
 
 	if (tap_send(c, buf, len + (data - buf)) < 1)
-		debug("tap: failed to send %lu bytes (IPv6)", len);
+		debug("tap: failed to send %zu bytes (IPv6)", len);
 }
 
 /**
@@ -302,7 +302,7 @@ void tap_icmp6_send(const struct ctx *c,
 	csum_icmp6(icmp6h, src, dst, icmp6h + 1, len - sizeof(*icmp6h));
 
 	if (tap_send(c, buf, len + (data - buf)) < 1)
-		debug("tap: failed to send %lu bytes (IPv6)", len);
+		debug("tap: failed to send %zu bytes (IPv6)", len);
 }
 
 /**
@@ -364,7 +364,7 @@ static void tap_send_remainder(const struct ctx *c, const struct iovec *iov,
 		ssize_t sent = send(c->fd_tap, base + offset, len - offset,
 				    MSG_NOSIGNAL);
 		if (sent < 0) {
-			err("tap: partial frame send (missing %lu bytes): %s",
+			err("tap: partial frame send (missing %zu bytes): %s",
 			    len - offset, strerror(errno));
 			return;
 		}
@@ -433,7 +433,7 @@ size_t tap_send_frames(const struct ctx *c, const struct iovec *iov, size_t n)
 		m = tap_send_frames_pasta(c, iov, n);
 
 	if (m < n)
-		debug("tap: failed to send %lu frames of %lu", n - m, n);
+		debug("tap: failed to send %zu frames of %zu", n - m, n);
 
 	pcap_multiple(iov, m, c->mode == MODE_PASST ? sizeof(uint32_t) : 0);
 
diff --git a/tcp.c b/tcp.c
index 40e3dec..44468ca 100644
--- a/tcp.c
+++ b/tcp.c
@@ -2570,7 +2570,8 @@ int tcp_tap_handler(struct ctx *c, uint8_t pif, int af,
 		return 1;
 	}
 
-	trace("TCP: packet length %lu from tap for index %lu", len, CONN_IDX(conn));
+	trace("TCP: packet length %zu from tap for index %lu",
+	      len, CONN_IDX(conn));
 
 	if (th->rst) {
 		conn_event(c, conn, CLOSED);
diff --git a/tcp_splice.c b/tcp_splice.c
index a5c1332..8d08bb4 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -321,7 +321,7 @@ static int tcp_splice_connect_finish(const struct ctx *c,
 
 			if (fcntl(conn->pipe[side][0], F_SETPIPE_SZ,
 				  c->tcp.pipe_size)) {
-				trace("TCP (spliced): cannot set %d->%d pipe size to %lu",
+				trace("TCP (spliced): cannot set %d->%d pipe size to %zu",
 				      side, !side, c->tcp.pipe_size);
 			}
 		}
@@ -554,7 +554,7 @@ retry:
 		readlen = splice(conn->s[fromside], NULL,
 				 conn->pipe[fromside][1], NULL, c->tcp.pipe_size,
 				 SPLICE_F_MOVE | SPLICE_F_NONBLOCK);
-		trace("TCP (spliced): %li from read-side call", readlen);
+		trace("TCP (spliced): %zi from read-side call", readlen);
 		if (readlen < 0) {
 			if (errno == EINTR)
 				goto retry;
@@ -580,7 +580,7 @@ eintr:
 		written = splice(conn->pipe[fromside][0], NULL,
 				 conn->s[!fromside], NULL, to_write,
 				 SPLICE_F_MOVE | more | SPLICE_F_NONBLOCK);
-		trace("TCP (spliced): %li from write-side call (passed %lu)",
+		trace("TCP (spliced): %zi from write-side call (passed %zu)",
 		      written, to_write);
 
 		/* Most common case: skip updating counters. */
@@ -718,7 +718,7 @@ static void tcp_splice_pipe_refill(const struct ctx *c)
 
 		if (fcntl(splice_pipe_pool[i][0], F_SETPIPE_SZ,
 			  c->tcp.pipe_size)) {
-			trace("TCP (spliced): cannot set pool pipe size to %lu",
+			trace("TCP (spliced): cannot set pool pipe size to %zu",
 			      c->tcp.pipe_size);
 		}
 	}
-- 
@@ -321,7 +321,7 @@ static int tcp_splice_connect_finish(const struct ctx *c,
 
 			if (fcntl(conn->pipe[side][0], F_SETPIPE_SZ,
 				  c->tcp.pipe_size)) {
-				trace("TCP (spliced): cannot set %d->%d pipe size to %lu",
+				trace("TCP (spliced): cannot set %d->%d pipe size to %zu",
 				      side, !side, c->tcp.pipe_size);
 			}
 		}
@@ -554,7 +554,7 @@ retry:
 		readlen = splice(conn->s[fromside], NULL,
 				 conn->pipe[fromside][1], NULL, c->tcp.pipe_size,
 				 SPLICE_F_MOVE | SPLICE_F_NONBLOCK);
-		trace("TCP (spliced): %li from read-side call", readlen);
+		trace("TCP (spliced): %zi from read-side call", readlen);
 		if (readlen < 0) {
 			if (errno == EINTR)
 				goto retry;
@@ -580,7 +580,7 @@ eintr:
 		written = splice(conn->pipe[fromside][0], NULL,
 				 conn->s[!fromside], NULL, to_write,
 				 SPLICE_F_MOVE | more | SPLICE_F_NONBLOCK);
-		trace("TCP (spliced): %li from write-side call (passed %lu)",
+		trace("TCP (spliced): %zi from write-side call (passed %zu)",
 		      written, to_write);
 
 		/* Most common case: skip updating counters. */
@@ -718,7 +718,7 @@ static void tcp_splice_pipe_refill(const struct ctx *c)
 
 		if (fcntl(splice_pipe_pool[i][0], F_SETPIPE_SZ,
 			  c->tcp.pipe_size)) {
-			trace("TCP (spliced): cannot set pool pipe size to %lu",
+			trace("TCP (spliced): cannot set pool pipe size to %zu",
 			      c->tcp.pipe_size);
 		}
 	}
-- 
2.39.2

Re: [PATCH 1/4] treewide: Use 'z' length modifier for size_t/ssize_t conversions

[David Gibson]

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

On Wed, Nov 29, 2023 at 02:46:07PM +0100, Stefano Brivio wrote:
> Types size_t and ssize_t are not necessarily long, it depends on the
> architecture.

Most LGTM, but a couple of nits:

[snip]
> @@ -106,7 +106,7 @@ void *packet_get_do(const struct pool *p, size_t idx, size_t offset,
>  
>  	if (p->pkt[idx].offset + len + offset > p->buf_size) {
>  		if (func) {
> -			trace("packet offset plus length %lu from size %lu, "
> +			trace("packet offset plus length %lu from size %zu, "

The change here is certainly correct.  But the remaining %lu is
dubious.  The value given is the sum of a uint32_t and two size_t, so
it could depend on platform what exactly that will be promoted to.  I
think we should probably either cast the result explicitly to (size_t)
and use %zu, or cast to (uint32_t) and use "%" PRIu32.

[snip]
> diff --git a/tcp_splice.c b/tcp_splice.c
> index a5c1332..8d08bb4 100644
> --- a/tcp_splice.c
> +++ b/tcp_splice.c
> @@ -321,7 +321,7 @@ static int tcp_splice_connect_finish(const struct ctx *c,
>  
>  			if (fcntl(conn->pipe[side][0], F_SETPIPE_SZ,
>  				  c->tcp.pipe_size)) {
> -				trace("TCP (spliced): cannot set %d->%d pipe size to %lu",
> +				trace("TCP (spliced): cannot set %d->%d pipe size to %zu",
>  				      side, !side, c->tcp.pipe_size);
>  			}
>  		}
> @@ -554,7 +554,7 @@ retry:
>  		readlen = splice(conn->s[fromside], NULL,
>  				 conn->pipe[fromside][1], NULL, c->tcp.pipe_size,
>  				 SPLICE_F_MOVE | SPLICE_F_NONBLOCK);
> -		trace("TCP (spliced): %li from read-side call", readlen);
> +		trace("TCP (spliced): %zi from read-side call", readlen);
>  		if (readlen < 0) {
>  			if (errno == EINTR)
>  				goto retry;
> @@ -580,7 +580,7 @@ eintr:
>  		written = splice(conn->pipe[fromside][0], NULL,
>  				 conn->s[!fromside], NULL, to_write,
>  				 SPLICE_F_MOVE | more | SPLICE_F_NONBLOCK);
> -		trace("TCP (spliced): %li from write-side call (passed %lu)",
> +		trace("TCP (spliced): %zi from write-side call (passed %zu)",
>  		      written, to_write);

'to_write' is actually an ssize_t which would suggest %zi.  However
looking at the code, I think to_write probably *should* be a size_t
instead.

>  
>  		/* Most common case: skip updating counters. */
> @@ -718,7 +718,7 @@ static void tcp_splice_pipe_refill(const struct ctx *c)
>  
>  		if (fcntl(splice_pipe_pool[i][0], F_SETPIPE_SZ,
>  			  c->tcp.pipe_size)) {
> -			trace("TCP (spliced): cannot set pool pipe size to %lu",
> +			trace("TCP (spliced): cannot set pool pipe size to %zu",
>  			      c->tcp.pipe_size);
>  		}
>  	}

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

Re: [PATCH 1/4] treewide: Use 'z' length modifier for size_t/ssize_t conversions

[Stefano Brivio]

On Thu, 30 Nov 2023 11:15:47 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Nov 29, 2023 at 02:46:07PM +0100, Stefano Brivio wrote:
> > Types size_t and ssize_t are not necessarily long, it depends on the
> > architecture.  
> 
> Most LGTM, but a couple of nits:
> 
> [snip]
> > @@ -106,7 +106,7 @@ void *packet_get_do(const struct pool *p, size_t idx, size_t offset,
> >  
> >  	if (p->pkt[idx].offset + len + offset > p->buf_size) {
> >  		if (func) {
> > -			trace("packet offset plus length %lu from size %lu, "
> > +			trace("packet offset plus length %lu from size %zu, "  
> 
> The change here is certainly correct.  But the remaining %lu is
> dubious.  The value given is the sum of a uint32_t and two size_t, so
> it could depend on platform what exactly that will be promoted to.  I
> think we should probably either cast the result explicitly to (size_t)
> and use %zu, or cast to (uint32_t) and use "%" PRIu32.
> 
> [snip]
> > diff --git a/tcp_splice.c b/tcp_splice.c
> > index a5c1332..8d08bb4 100644
> > --- a/tcp_splice.c
> > +++ b/tcp_splice.c
> > @@ -321,7 +321,7 @@ static int tcp_splice_connect_finish(const struct ctx *c,
> >  
> >  			if (fcntl(conn->pipe[side][0], F_SETPIPE_SZ,
> >  				  c->tcp.pipe_size)) {
> > -				trace("TCP (spliced): cannot set %d->%d pipe size to %lu",
> > +				trace("TCP (spliced): cannot set %d->%d pipe size to %zu",
> >  				      side, !side, c->tcp.pipe_size);
> >  			}
> >  		}
> > @@ -554,7 +554,7 @@ retry:
> >  		readlen = splice(conn->s[fromside], NULL,
> >  				 conn->pipe[fromside][1], NULL, c->tcp.pipe_size,
> >  				 SPLICE_F_MOVE | SPLICE_F_NONBLOCK);
> > -		trace("TCP (spliced): %li from read-side call", readlen);
> > +		trace("TCP (spliced): %zi from read-side call", readlen);
> >  		if (readlen < 0) {
> >  			if (errno == EINTR)
> >  				goto retry;
> > @@ -580,7 +580,7 @@ eintr:
> >  		written = splice(conn->pipe[fromside][0], NULL,
> >  				 conn->s[!fromside], NULL, to_write,
> >  				 SPLICE_F_MOVE | more | SPLICE_F_NONBLOCK);
> > -		trace("TCP (spliced): %li from write-side call (passed %lu)",
> > +		trace("TCP (spliced): %zi from write-side call (passed %zu)",
> >  		      written, to_write);  
> 
> 'to_write' is actually an ssize_t which would suggest %zi.  However
> looking at the code, I think to_write probably *should* be a size_t
> instead.

Oops, I didn't notice. Well, I know we're passing it to splice(), but
we're also using it like this:

		if (!never_read && written < to_write) {
			to_write -= written;
			goto retry;
		}

so I'd rather keep it as ssize_t for the moment (and re-spin this
series with a %zi here), just in case we happen to do something silly
with it and ssize_t is saving us.

-- 
Stefano

Re: [PATCH 1/4] treewide: Use 'z' length modifier for size_t/ssize_t conversions

[David Gibson]

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

On Thu, Nov 30, 2023 at 10:06:46AM +0100, Stefano Brivio wrote:
> On Thu, 30 Nov 2023 11:15:47 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, Nov 29, 2023 at 02:46:07PM +0100, Stefano Brivio wrote:
> > > Types size_t and ssize_t are not necessarily long, it depends on the
> > > architecture.  
> > 
> > Most LGTM, but a couple of nits:
> > 
> > [snip]
> > > @@ -106,7 +106,7 @@ void *packet_get_do(const struct pool *p, size_t idx, size_t offset,
> > >  
> > >  	if (p->pkt[idx].offset + len + offset > p->buf_size) {
> > >  		if (func) {
> > > -			trace("packet offset plus length %lu from size %lu, "
> > > +			trace("packet offset plus length %lu from size %zu, "  
> > 
> > The change here is certainly correct.  But the remaining %lu is
> > dubious.  The value given is the sum of a uint32_t and two size_t, so
> > it could depend on platform what exactly that will be promoted to.  I
> > think we should probably either cast the result explicitly to (size_t)
> > and use %zu, or cast to (uint32_t) and use "%" PRIu32.
> > 
> > [snip]
> > > diff --git a/tcp_splice.c b/tcp_splice.c
> > > index a5c1332..8d08bb4 100644
> > > --- a/tcp_splice.c
> > > +++ b/tcp_splice.c
> > > @@ -321,7 +321,7 @@ static int tcp_splice_connect_finish(const struct ctx *c,
> > >  
> > >  			if (fcntl(conn->pipe[side][0], F_SETPIPE_SZ,
> > >  				  c->tcp.pipe_size)) {
> > > -				trace("TCP (spliced): cannot set %d->%d pipe size to %lu",
> > > +				trace("TCP (spliced): cannot set %d->%d pipe size to %zu",
> > >  				      side, !side, c->tcp.pipe_size);
> > >  			}
> > >  		}
> > > @@ -554,7 +554,7 @@ retry:
> > >  		readlen = splice(conn->s[fromside], NULL,
> > >  				 conn->pipe[fromside][1], NULL, c->tcp.pipe_size,
> > >  				 SPLICE_F_MOVE | SPLICE_F_NONBLOCK);
> > > -		trace("TCP (spliced): %li from read-side call", readlen);
> > > +		trace("TCP (spliced): %zi from read-side call", readlen);
> > >  		if (readlen < 0) {
> > >  			if (errno == EINTR)
> > >  				goto retry;
> > > @@ -580,7 +580,7 @@ eintr:
> > >  		written = splice(conn->pipe[fromside][0], NULL,
> > >  				 conn->s[!fromside], NULL, to_write,
> > >  				 SPLICE_F_MOVE | more | SPLICE_F_NONBLOCK);
> > > -		trace("TCP (spliced): %li from write-side call (passed %lu)",
> > > +		trace("TCP (spliced): %zi from write-side call (passed %zu)",
> > >  		      written, to_write);  
> > 
> > 'to_write' is actually an ssize_t which would suggest %zi.  However
> > looking at the code, I think to_write probably *should* be a size_t
> > instead.
> 
> Oops, I didn't notice. Well, I know we're passing it to splice(), but
> we're also using it like this:
> 
> 		if (!never_read && written < to_write) {
> 			to_write -= written;
> 			goto retry;
> 		}
> 
> so I'd rather keep it as ssize_t for the moment (and re-spin this
> series with a %zi here), just in case we happen to do something silly
> with it and ssize_t is saving us.

Eh.. that's the only place we subtract, and the literal line above
verifies that the result will still be positive, so I think we're
safe.  But, whatever.

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

[PATCH 2/4] packet: Offset plus length is not always uint32_t, but it's always size_t

[Stefano Brivio]

According to gcc, PRIu32 matches the type of the argument we're
printing here on both 64 and 32-bits architectures. According to
Clang, though, that's not the case, as the result of the sum is an
unsigned long on 64-bit.

Use the z modifier, given that we're summing uint32_t to size_t, and
the result is at most promoted to size_t.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 packet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/packet.c b/packet.c
index 12ac76b..ccfc846 100644
--- a/packet.c
+++ b/packet.c
@@ -106,7 +106,7 @@ void *packet_get_do(const struct pool *p, size_t idx, size_t offset,
 
 	if (p->pkt[idx].offset + len + offset > p->buf_size) {
 		if (func) {
-			trace("packet offset plus length %lu from size %zu, "
+			trace("packet offset plus length %zu from size %zu, "
 			      "%s:%i", p->pkt[idx].offset + len + offset,
 			      p->buf_size, func, line);
 		}
-- 
@@ -106,7 +106,7 @@ void *packet_get_do(const struct pool *p, size_t idx, size_t offset,
 
 	if (p->pkt[idx].offset + len + offset > p->buf_size) {
 		if (func) {
-			trace("packet offset plus length %lu from size %zu, "
+			trace("packet offset plus length %zu from size %zu, "
 			      "%s:%i", p->pkt[idx].offset + len + offset,
 			      p->buf_size, func, line);
 		}
-- 
2.39.2

Re: [PATCH 2/4] packet: Offset plus length is not always uint32_t, but it's always size_t

[David Gibson]

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

On Wed, Nov 29, 2023 at 02:46:08PM +0100, Stefano Brivio wrote:
> According to gcc, PRIu32 matches the type of the argument we're
> printing here on both 64 and 32-bits architectures. According to
> Clang, though, that's not the case, as the result of the sum is an
> unsigned long on 64-bit.
> 
> Use the z modifier, given that we're summing uint32_t to size_t, and
> the result is at most promoted to size_t.

Heh, sorry, obviously hadn't read this patch when I commented on this
spot in the first one.  The problem here is that the final promoted
type depends on whether size_t is wider than uint32_t or not, which
can vary with architecture.

That said, I doubt we're likely to support anything with a size_t
strictly *less* than 32-bits, so %zu is probably safe.

> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  packet.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/packet.c b/packet.c
> index 12ac76b..ccfc846 100644
> --- a/packet.c
> +++ b/packet.c
> @@ -106,7 +106,7 @@ void *packet_get_do(const struct pool *p, size_t idx, size_t offset,
>  
>  	if (p->pkt[idx].offset + len + offset > p->buf_size) {
>  		if (func) {
> -			trace("packet offset plus length %lu from size %zu, "
> +			trace("packet offset plus length %zu from size %zu, "
>  			      "%s:%i", p->pkt[idx].offset + len + offset,
>  			      p->buf_size, func, line);
>  		}

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

Re: [PATCH 2/4] packet: Offset plus length is not always uint32_t, but it's always size_t

[Stefano Brivio]

On Thu, 30 Nov 2023 11:18:48 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Nov 29, 2023 at 02:46:08PM +0100, Stefano Brivio wrote:
> > According to gcc, PRIu32 matches the type of the argument we're
> > printing here on both 64 and 32-bits architectures. According to
> > Clang, though, that's not the case, as the result of the sum is an
> > unsigned long on 64-bit.
> > 
> > Use the z modifier, given that we're summing uint32_t to size_t, and
> > the result is at most promoted to size_t.  
> 
> Heh, sorry, obviously hadn't read this patch when I commented on this
> spot in the first one.  The problem here is that the final promoted
> type depends on whether size_t is wider than uint32_t or not, which
> can vary with architecture.
> 
> That said, I doubt we're likely to support anything with a size_t
> strictly *less* than 32-bits, so %zu is probably safe.
> 
> > 
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > ---
> >  packet.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/packet.c b/packet.c
> > index 12ac76b..ccfc846 100644
> > --- a/packet.c
> > +++ b/packet.c
> > @@ -106,7 +106,7 @@ void *packet_get_do(const struct pool *p, size_t idx, size_t offset,
> >  
> >  	if (p->pkt[idx].offset + len + offset > p->buf_size) {
> >  		if (func) {
> > -			trace("packet offset plus length %lu from size %zu, "
> > +			trace("packet offset plus length %zu from size %zu, "
> >  			      "%s:%i", p->pkt[idx].offset + len + offset,
> >  			      p->buf_size, func, line);
> >  		}  
> 

Re: [PATCH 2/4] packet: Offset plus length is not always uint32_t, but it's always size_t

[Stefano Brivio]

On Thu, 30 Nov 2023 11:18:48 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Nov 29, 2023 at 02:46:08PM +0100, Stefano Brivio wrote:
> > According to gcc, PRIu32 matches the type of the argument we're
> > printing here on both 64 and 32-bits architectures. According to
> > Clang, though, that's not the case, as the result of the sum is an
> > unsigned long on 64-bit.
> > 
> > Use the z modifier, given that we're summing uint32_t to size_t, and
> > the result is at most promoted to size_t.  
> 
> Heh, sorry, obviously hadn't read this patch when I commented on this
> spot in the first one.  The problem here is that the final promoted
> type depends on whether size_t is wider than uint32_t or not, which
> can vary with architecture.

...I'm not sure if it's just a matter of warnings, but gcc is perfectly
happy with PRIu32 for uint32_t + size_t on x86_64, so on top of the
architecture, promotion rules also seem to vary between compilers. Or
maybe it just doesn't complain about the possible format truncation.

> That said, I doubt we're likely to support anything with a size_t
> strictly *less* than 32-bits, so %zu is probably safe.

Ah, yes, I took that for granted. Looking into older architectures
where C would commonly be used, it looks like 16 bits of size_t would
only suffice for *selected versions* of PDP-11 (PDP-11/15 and
PDP-11/20, but not PDP-11/45 already, because the addressing space is
larger than 64 KiB).

Indeed there are 8 and 16 bits processors, but there doesn't appear to
be any other modern architecture where 16 bits suffice for addressable
memory (by design).

-- 
Stefano

Re: [PATCH 2/4] packet: Offset plus length is not always uint32_t, but it's always size_t

[David Gibson]

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

On Thu, Nov 30, 2023 at 10:07:00AM +0100, Stefano Brivio wrote:
> On Thu, 30 Nov 2023 11:18:48 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, Nov 29, 2023 at 02:46:08PM +0100, Stefano Brivio wrote:
> > > According to gcc, PRIu32 matches the type of the argument we're
> > > printing here on both 64 and 32-bits architectures. According to
> > > Clang, though, that's not the case, as the result of the sum is an
> > > unsigned long on 64-bit.
> > > 
> > > Use the z modifier, given that we're summing uint32_t to size_t, and
> > > the result is at most promoted to size_t.  
> > 
> > Heh, sorry, obviously hadn't read this patch when I commented on this
> > spot in the first one.  The problem here is that the final promoted
> > type depends on whether size_t is wider than uint32_t or not, which
> > can vary with architecture.
> 
> ...I'm not sure if it's just a matter of warnings, but gcc is perfectly
> happy with PRIu32 for uint32_t + size_t on x86_64, so on top of the
> architecture, promotion rules also seem to vary between compilers. Or
> maybe it just doesn't complain about the possible format truncation.

Huh.. that's pretty surprising.

> > That said, I doubt we're likely to support anything with a size_t
> > strictly *less* than 32-bits, so %zu is probably safe.
> 
> Ah, yes, I took that for granted. Looking into older architectures
> where C would commonly be used, it looks like 16 bits of size_t would
> only suffice for *selected versions* of PDP-11 (PDP-11/15 and
> PDP-11/20, but not PDP-11/45 already, because the addressing space is
> larger than 64 KiB).
> 
> Indeed there are 8 and 16 bits processors, but there doesn't appear to
> be any other modern architecture where 16 bits suffice for addressable
> memory (by design).

Right.

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

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

[Stefano Brivio]

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),
 	      it.it_value.tv_sec, it.it_value.tv_nsec / 1000 / 1000);
 
 	timerfd_settime(conn->timer, 0, &it, NULL);
@@ -750,7 +750,7 @@ static void conn_flag_do(const struct ctx *c, struct tcp_tap_conn *conn,
 
 		conn->flags &= flag;
 		if (flag_index >= 0) {
-			debug("TCP: index %li: %s dropped", CONN_IDX(conn),
+			debug("TCP: index %ti: %s dropped", CONN_IDX(conn),
 			      tcp_flag_str[flag_index]);
 		}
 	} else {
@@ -771,7 +771,7 @@ static void conn_flag_do(const struct ctx *c, struct tcp_tap_conn *conn,
 
 		conn->flags |= flag;
 		if (flag_index >= 0) {
-			debug("TCP: index %li: %s", CONN_IDX(conn),
+			debug("TCP: index %ti: %s", CONN_IDX(conn),
 			      tcp_flag_str[flag_index]);
 		}
 	}
@@ -821,12 +821,12 @@ static void conn_event_do(const struct ctx *c, struct tcp_tap_conn *conn,
 		new += 5;
 
 	if (prev != new) {
-		debug("TCP: index %li, %s: %s -> %s", CONN_IDX(conn),
+		debug("TCP: index %ti, %s: %s -> %s", CONN_IDX(conn),
 		      num == -1 	       ? "CLOSED" : tcp_event_str[num],
 		      prev == -1	       ? "CLOSED" : tcp_state_str[prev],
 		      (new == -1 || num == -1) ? "CLOSED" : tcp_state_str[new]);
 	} else {
-		debug("TCP: index %li, %s", CONN_IDX(conn),
+		debug("TCP: index %ti, %s", CONN_IDX(conn),
 		      num == -1 	       ? "CLOSED" : tcp_event_str[num]);
 	}
 
@@ -1209,7 +1209,7 @@ static void tcp_hash_insert(const struct ctx *c, struct tcp_tap_conn *conn)
 	conn->next_index = tc_hash[b] ? CONN_IDX(tc_hash[b]) : -1;
 	tc_hash[b] = conn;
 
-	debug("TCP: hash table insert: index %li, sock %i, bucket: %i, next: "
+	debug("TCP: hash table insert: index %ti, sock %i, bucket: %i, next: "
 	      "%p", CONN_IDX(conn), conn->sock, b,
 	      (void *)conn_at_idx(conn->next_index));
 }
@@ -1236,7 +1236,7 @@ static void tcp_hash_remove(const struct ctx *c,
 		}
 	}
 
-	debug("TCP: hash table remove: index %li, sock %i, bucket: %i, new: %p",
+	debug("TCP: hash table remove: index %ti, sock %i, bucket: %i, new: %p",
 	      CONN_IDX(conn), conn->sock, b,
 	      (void *)(prev ? conn_at_idx(prev->next_index) : tc_hash[b]));
 }
@@ -1264,7 +1264,7 @@ static void tcp_tap_conn_update(const struct ctx *c, struct tcp_tap_conn *old,
 		}
 	}
 
-	debug("TCP: hash table update: old index %li, new index %li, sock %i, "
+	debug("TCP: hash table update: old index %ti, new index %ti, sock %i, "
 	      "bucket: %i, old: %p, new: %p",
 	      CONN_IDX(old), CONN_IDX(new), new->sock, b,
 	      (void *)old, (void *)new);
@@ -1310,7 +1310,7 @@ void tcp_table_compact(struct ctx *c, union tcp_conn *hole)
 	union tcp_conn *from;
 
 	if (CONN_IDX(hole) == --c->tcp.conn_count) {
-		debug("TCP: table compaction: maximum index was %li (%p)",
+		debug("TCP: table compaction: maximum index was %ti (%p)",
 		      CONN_IDX(hole), (void *)hole);
 		memset(hole, 0, sizeof(*hole));
 		return;
@@ -1324,8 +1324,8 @@ void tcp_table_compact(struct ctx *c, union tcp_conn *hole)
 	else
 		tcp_tap_conn_update(c, &from->tap, &hole->tap);
 
-	debug("TCP: table compaction (spliced=%d): old index %li, new index %li, "
-	      "from: %p, to: %p",
+	debug("TCP: table compaction (spliced=%d): old index %ti, new index %ti"
+	      ", from: %p, to: %p",
 	      from->c.spliced, CONN_IDX(from), CONN_IDX(hole),
 	      (void *)from, (void *)hole);
 
@@ -1352,7 +1352,7 @@ static void tcp_conn_destroy(struct ctx *c, union tcp_conn *conn_union)
 static void tcp_rst_do(struct ctx *c, struct tcp_tap_conn *conn);
 #define tcp_rst(c, conn)						\
 	do {								\
-		debug("TCP: index %li, reset at %s:%i", CONN_IDX(conn), \
+		debug("TCP: index %ti, reset at %s:%i", CONN_IDX(conn),	\
 		      __func__, __LINE__);				\
 		tcp_rst_do(c, conn);					\
 	} while (0)
@@ -2570,7 +2570,7 @@ int tcp_tap_handler(struct ctx *c, uint8_t pif, int af,
 		return 1;
 	}
 
-	trace("TCP: packet length %zu from tap for index %lu",
+	trace("TCP: packet length %zu from tap for index %ti",
 	      len, CONN_IDX(conn));
 
 	if (th->rst) {
@@ -2811,17 +2811,19 @@ void tcp_timer_handler(struct ctx *c, union epoll_ref ref)
 		tcp_timer_ctl(c, conn);
 	} else if (conn->flags & ACK_FROM_TAP_DUE) {
 		if (!(conn->events & ESTABLISHED)) {
-			debug("TCP: index %li, handshake timeout", CONN_IDX(conn));
+			debug("TCP: index %ti, handshake timeout",
+			      CONN_IDX(conn));
 			tcp_rst(c, conn);
 		} else if (CONN_HAS(conn, SOCK_FIN_SENT | TAP_FIN_ACKED)) {
-			debug("TCP: index %li, FIN timeout", CONN_IDX(conn));
+			debug("TCP: index %ti, FIN timeout", CONN_IDX(conn));
 			tcp_rst(c, conn);
 		} else if (conn->retrans == TCP_MAX_RETRANS) {
-			debug("TCP: index %li, retransmissions count exceeded",
+			debug("TCP: index %ti, retransmissions count exceeded",
 			      CONN_IDX(conn));
 			tcp_rst(c, conn);
 		} else {
-			debug("TCP: index %li, ACK timeout, retry", CONN_IDX(conn));
+			debug("TCP: index %ti, ACK timeout, retry",
+			      CONN_IDX(conn));
 			conn->retrans++;
 			conn->seq_to_tap = conn->seq_ack_from_tap;
 			tcp_data_from_sock(c, conn);
@@ -2839,7 +2841,8 @@ void tcp_timer_handler(struct ctx *c, union epoll_ref ref)
 		 */
 		timerfd_settime(conn->timer, 0, &new, &old);
 		if (old.it_value.tv_sec == ACT_TIMEOUT) {
-			debug("TCP: index %li, activity timeout", CONN_IDX(conn));
+			debug("TCP: index %ti, activity timeout",
+			      CONN_IDX(conn));
 			tcp_rst(c, conn);
 		}
 	}
diff --git a/tcp_splice.c b/tcp_splice.c
index 8d08bb4..cb8a73e 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -138,7 +138,7 @@ static int tcp_splice_epoll_ctl(const struct ctx *c,
 	if (epoll_ctl(c->epollfd, m, conn->s[0], &ev[0]) ||
 	    epoll_ctl(c->epollfd, m, conn->s[1], &ev[1])) {
 		int ret = -errno;
-		err("TCP (spliced): index %li, ERROR on epoll_ctl(): %s",
+		err("TCP (spliced): index %ti, ERROR on epoll_ctl(): %s",
 		    CONN_IDX(conn), strerror(errno));
 		return ret;
 	}
@@ -165,8 +165,8 @@ static void conn_flag_do(const struct ctx *c, struct tcp_splice_conn *conn,
 
 		conn->flags &= flag;
 		if (flag_index >= 0) {
-			debug("TCP (spliced): index %li: %s dropped", CONN_IDX(conn),
-			      tcp_splice_flag_str[flag_index]);
+			debug("TCP (spliced): index %ti: %s dropped",
+			      CONN_IDX(conn), tcp_splice_flag_str[flag_index]);
 		}
 	} else {
 		int flag_index = fls(flag);
@@ -176,7 +176,7 @@ static void conn_flag_do(const struct ctx *c, struct tcp_splice_conn *conn,
 
 		conn->flags |= flag;
 		if (flag_index >= 0) {
-			debug("TCP (spliced): index %li: %s", CONN_IDX(conn),
+			debug("TCP (spliced): index %ti: %s", CONN_IDX(conn),
 			      tcp_splice_flag_str[flag_index]);
 		}
 	}
@@ -211,7 +211,7 @@ static void conn_event_do(const struct ctx *c, struct tcp_splice_conn *conn,
 
 		conn->events &= event;
 		if (flag_index >= 0) {
-			debug("TCP (spliced): index %li, ~%s", CONN_IDX(conn),
+			debug("TCP (spliced): index %ti, ~%s", CONN_IDX(conn),
 			      tcp_splice_event_str[flag_index]);
 		}
 	} else {
@@ -222,7 +222,7 @@ static void conn_event_do(const struct ctx *c, struct tcp_splice_conn *conn,
 
 		conn->events |= event;
 		if (flag_index >= 0) {
-			debug("TCP (spliced): index %li, %s", CONN_IDX(conn),
+			debug("TCP (spliced): index %ti, %s", CONN_IDX(conn),
 			      tcp_splice_event_str[flag_index]);
 		}
 	}
@@ -280,7 +280,7 @@ void tcp_splice_destroy(struct ctx *c, union tcp_conn *conn_union)
 
 	conn->events = SPLICE_CLOSED;
 	conn->flags = 0;
-	debug("TCP (spliced): index %li, CLOSED", CONN_IDX(conn));
+	debug("TCP (spliced): index %ti, CLOSED", CONN_IDX(conn));
 
 	tcp_table_compact(c, conn_union);
 }
-- 
@@ -138,7 +138,7 @@ static int tcp_splice_epoll_ctl(const struct ctx *c,
 	if (epoll_ctl(c->epollfd, m, conn->s[0], &ev[0]) ||
 	    epoll_ctl(c->epollfd, m, conn->s[1], &ev[1])) {
 		int ret = -errno;
-		err("TCP (spliced): index %li, ERROR on epoll_ctl(): %s",
+		err("TCP (spliced): index %ti, ERROR on epoll_ctl(): %s",
 		    CONN_IDX(conn), strerror(errno));
 		return ret;
 	}
@@ -165,8 +165,8 @@ static void conn_flag_do(const struct ctx *c, struct tcp_splice_conn *conn,
 
 		conn->flags &= flag;
 		if (flag_index >= 0) {
-			debug("TCP (spliced): index %li: %s dropped", CONN_IDX(conn),
-			      tcp_splice_flag_str[flag_index]);
+			debug("TCP (spliced): index %ti: %s dropped",
+			      CONN_IDX(conn), tcp_splice_flag_str[flag_index]);
 		}
 	} else {
 		int flag_index = fls(flag);
@@ -176,7 +176,7 @@ static void conn_flag_do(const struct ctx *c, struct tcp_splice_conn *conn,
 
 		conn->flags |= flag;
 		if (flag_index >= 0) {
-			debug("TCP (spliced): index %li: %s", CONN_IDX(conn),
+			debug("TCP (spliced): index %ti: %s", CONN_IDX(conn),
 			      tcp_splice_flag_str[flag_index]);
 		}
 	}
@@ -211,7 +211,7 @@ static void conn_event_do(const struct ctx *c, struct tcp_splice_conn *conn,
 
 		conn->events &= event;
 		if (flag_index >= 0) {
-			debug("TCP (spliced): index %li, ~%s", CONN_IDX(conn),
+			debug("TCP (spliced): index %ti, ~%s", CONN_IDX(conn),
 			      tcp_splice_event_str[flag_index]);
 		}
 	} else {
@@ -222,7 +222,7 @@ static void conn_event_do(const struct ctx *c, struct tcp_splice_conn *conn,
 
 		conn->events |= event;
 		if (flag_index >= 0) {
-			debug("TCP (spliced): index %li, %s", CONN_IDX(conn),
+			debug("TCP (spliced): index %ti, %s", CONN_IDX(conn),
 			      tcp_splice_event_str[flag_index]);
 		}
 	}
@@ -280,7 +280,7 @@ void tcp_splice_destroy(struct ctx *c, union tcp_conn *conn_union)
 
 	conn->events = SPLICE_CLOSED;
 	conn->flags = 0;
-	debug("TCP (spliced): index %li, CLOSED", CONN_IDX(conn));
+	debug("TCP (spliced): index %ti, CLOSED", CONN_IDX(conn));
 
 	tcp_table_compact(c, conn_union);
 }
-- 
2.39.2

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

[Stefano Brivio]

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

There, however, I guess that the new flow_idx() should return ptrdiff_t,
which is signed.

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.

-- 
Stefano

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

[David Gibson]

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

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

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

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

[Stefano Brivio]

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

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

[David Gibson]

[-- 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 --]

[PATCH 4/4] port_fwd, util: Include additional headers to fix build with musl

[Stefano Brivio]

lseek() is declared in unistd.h, and stdio.h provides sscanf().
Include these two headers in port_fwd.c.

SIGCHLD, even if used exclusively for clone(), is defined in
signal.h: add the include to util.h, as NS_CALL needs it.

Reported-by: lemmi <lemmi@nerd2nerd.org>
Link: https://github.com/void-linux/void-packages/actions/runs/6999782606/job/19039526604#step:7:57
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 port_fwd.c | 2 ++
 util.h     | 1 +
 2 files changed, 3 insertions(+)

diff --git a/port_fwd.c b/port_fwd.c
index 7943a30..6f6c836 100644
--- a/port_fwd.c
+++ b/port_fwd.c
@@ -17,6 +17,8 @@
 #include <errno.h>
 #include <fcntl.h>
 #include <sched.h>
+#include <unistd.h>
+#include <stdio.h>
 
 #include "util.h"
 #include "port_fwd.h"
diff --git a/util.h b/util.h
index 1f02588..86f1a7e 100644
--- a/util.h
+++ b/util.h
@@ -10,6 +10,7 @@
 #include <stdarg.h>
 #include <stdbool.h>
 #include <string.h>
+#include <signal.h>
 
 #include "log.h"
 
-- 
@@ -10,6 +10,7 @@
 #include <stdarg.h>
 #include <stdbool.h>
 #include <string.h>
+#include <signal.h>
 
 #include "log.h"
 
-- 
2.39.2

Re: [PATCH 4/4] port_fwd, util: Include additional headers to fix build with musl

[David Gibson]

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

On Wed, Nov 29, 2023 at 02:46:10PM +0100, Stefano Brivio wrote:
> lseek() is declared in unistd.h, and stdio.h provides sscanf().
> Include these two headers in port_fwd.c.
> 
> SIGCHLD, even if used exclusively for clone(), is defined in
> signal.h: add the include to util.h, as NS_CALL needs it.

In theory the clang-tidy warnings I recently suppressed for ensuring
we *directly* include the things we need would help avoid problems
like this.  Unfortunately it generates too many dumb warnings to be
usable.  I guess to do this correctly a checker would need to know the
official/standardised header for every libc function, and require you
to include that, whether or not that directly or directly contains it
on this particular system.

> Reported-by: lemmi <lemmi@nerd2nerd.org>
> Link: https://github.com/void-linux/void-packages/actions/runs/6999782606/job/19039526604#step:7:57
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  port_fwd.c | 2 ++
>  util.h     | 1 +
>  2 files changed, 3 insertions(+)
> 
> diff --git a/port_fwd.c b/port_fwd.c
> index 7943a30..6f6c836 100644
> --- a/port_fwd.c
> +++ b/port_fwd.c
> @@ -17,6 +17,8 @@
>  #include <errno.h>
>  #include <fcntl.h>
>  #include <sched.h>
> +#include <unistd.h>
> +#include <stdio.h>
>  
>  #include "util.h"
>  #include "port_fwd.h"
> diff --git a/util.h b/util.h
> index 1f02588..86f1a7e 100644
> --- a/util.h
> +++ b/util.h
> @@ -10,6 +10,7 @@
>  #include <stdarg.h>
>  #include <stdbool.h>
>  #include <string.h>
> +#include <signal.h>
>  
>  #include "log.h"
>  

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