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

Stefano Brivio

29 Nov 2023 29 Nov '23

2:46 p.m.

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

Show replies by date

Stefano Brivio

29 Nov 29 Nov

2:46 p.m.

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

Types size_t and ssize_t are not necessarily long, it depends on the architecture. Signed-off-by: Stefano Brivio --- 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); } } -- 2.39.2

David Gibson

30 Nov 30 Nov

1:15 a.m.

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

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

Stefano Brivio

10:06 a.m.

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

On Thu, 30 Nov 2023 11:15:47 +1100 David Gibson 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

David Gibson

1 Dec 1 Dec

12:10 a.m.

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

On Thu, Nov 30, 2023 at 10:06:46AM +0100, Stefano Brivio wrote:

...

On Thu, 30 Nov 2023 11:15:47 +1100 David Gibson 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

Stefano Brivio

29 Nov 29 Nov

2:46 p.m.

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

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 --- 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); } -- 2.39.2

David Gibson

30 Nov 30 Nov

1:18 a.m.

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

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

Stefano Brivio

10:06 a.m.

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

On Thu, 30 Nov 2023 11:18:48 +1100 David Gibson 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 --- 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); }

Stefano Brivio

10:07 a.m.

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

On Thu, 30 Nov 2023 11:18:48 +1100 David Gibson 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

David Gibson

1 Dec 1 Dec

12:12 a.m.

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

On Thu, Nov 30, 2023 at 10:07:00AM +0100, Stefano Brivio wrote:

...

On Thu, 30 Nov 2023 11:18:48 +1100 David Gibson 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

Stefano Brivio

29 Nov 29 Nov

2:46 p.m.

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

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), 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); } -- 2.39.2

Stefano Brivio

2:58 p.m.

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

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

David Gibson

30 Nov 30 Nov

1:27 a.m.

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

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

Stefano Brivio

10:07 a.m.

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

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

David Gibson

1 Dec 1 Dec

12:13 a.m.

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

On Thu, Nov 30, 2023 at 10:07:44AM +0100, Stefano Brivio wrote:

...

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.

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

Stefano Brivio

29 Nov 29 Nov

2:46 p.m.

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

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 Link: https://github.com/void-linux/void-packages/actions/runs/6999782606/job/1903... Signed-off-by: Stefano Brivio --- 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 #include #include +#include +#include #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 #include #include +#include #include "log.h" -- 2.39.2

David Gibson

30 Nov 30 Nov

1:30 a.m.

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

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 Link: https://github.com/void-linux/void-packages/actions/runs/6999782606/job/1903... Signed-off-by: Stefano Brivio

Reviewed-by: David Gibson

...

--- 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 #include #include +#include +#include

#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 #include #include +#include

#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

534

Age (days ago)

535

Last active (days ago)

List overview

Download

16 comments

2 participants

participants (2)

David Gibson
Stefano Brivio

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

tags

participants (2)