[PATCH 0/6] Fix some more static checker warnings

[PATCH 0/6] Fix some more static checker warnings

[Stefano Brivio]

Stefano Brivio (6):
  dhcpv6: Use for loop instead of goto to avoid false positive cppcheck
    warning
  dhcpv6: Turn some option headers pointers to const
  tap: Cast TAP_BUF_BYTES - ETH_MAX_MTU to ssize_t, not TAP_BUF_BYTES
  util: Define small and big thresholds for socket buffers as unsigned
    long long
  passt: Use NOLINT clang-tidy block instead of NOLINTNEXTLINE
  tap, tcp, util: Add some missing SOCK_CLOEXEC flags

 dhcpv6.c | 51 +++++++++++++++++++++++----------------------------
 passt.c  |  3 ++-
 tap.c    |  7 ++++---
 tcp.c    |  2 +-
 util.c   |  3 ++-
 util.h   |  9 ++++++---
 6 files changed, 38 insertions(+), 37 deletions(-)

-- 
2.43.0

[PATCH 1/6] dhcpv6: Use for loop instead of goto to avoid false positive cppcheck warning

[Stefano Brivio]

cppcheck 2.16.0 reports:

dhcpv6.c:334:14: style: The comparison 'ia_type == 3' is always true. [knownConditionTrueFalse]
 if (ia_type == OPT_IA_NA) {
             ^
dhcpv6.c:306:12: note: 'ia_type' is assigned value '3' here.
 ia_type = OPT_IA_NA;
           ^
dhcpv6.c:334:14: note: The comparison 'ia_type == 3' is always true.
 if (ia_type == OPT_IA_NA) {
             ^

this is not really the case as we set ia_type to OPT_IA_TA and then
jump back.

Anyway, there's no particular reason to use a goto here: add a trivial
foreach() macro to go through elements of an array and use it instead.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 dhcpv6.c | 47 +++++++++++++++++++++--------------------------
 util.h   |  3 +++
 2 files changed, 24 insertions(+), 26 deletions(-)

diff --git a/dhcpv6.c b/dhcpv6.c
index 14a5c7e..f2e7307 100644
--- a/dhcpv6.c
+++ b/dhcpv6.c
@@ -296,47 +296,42 @@ static struct opt_hdr *dhcpv6_opt(const struct pool *p, size_t *offset,
 static struct opt_hdr *dhcpv6_ia_notonlink(const struct pool *p,
 					   struct in6_addr *la)
 {
+	int ia_types[2] = { OPT_IA_NA, OPT_IA_TA }, *ia_type;
+	const struct opt_ia_addr *opt_addr;
 	char buf[INET6_ADDRSTRLEN];
 	struct in6_addr req_addr;
 	const struct opt_hdr *h;
 	struct opt_hdr *ia;
 	size_t offset;
-	int ia_type;
 
-	ia_type = OPT_IA_NA;
-ia_ta:
-	offset = 0;
-	while ((ia = dhcpv6_opt(p, &offset, ia_type))) {
-		if (ntohs(ia->l) < OPT_VSIZE(ia_na))
-			return NULL;
+	foreach(ia_type, ia_types) {
+		offset = 0;
+		while ((ia = dhcpv6_opt(p, &offset, *ia_type))) {
+			if (ntohs(ia->l) < OPT_VSIZE(ia_na))
+				return NULL;
 
-		offset += sizeof(struct opt_ia_na);
+			offset += sizeof(struct opt_ia_na);
 
-		while ((h = dhcpv6_opt(p, &offset, OPT_IAAADR))) {
-			const struct opt_ia_addr *opt_addr;
+			while ((h = dhcpv6_opt(p, &offset, OPT_IAAADR))) {
+				if (ntohs(h->l) != OPT_VSIZE(ia_addr))
+					return NULL;
 
-			if (ntohs(h->l) != OPT_VSIZE(ia_addr))
-				return NULL;
+				opt_addr = (const struct opt_ia_addr *)h;
+				req_addr = opt_addr->addr;
+				if (!IN6_ARE_ADDR_EQUAL(la, &req_addr))
+					goto err;
 
-			opt_addr = (const struct opt_ia_addr *)h;
-			req_addr = opt_addr->addr;
-			if (!IN6_ARE_ADDR_EQUAL(la, &req_addr)) {
-				info("DHCPv6: requested address %s not on link",
-				     inet_ntop(AF_INET6, &req_addr,
-					       buf, sizeof(buf)));
-				return ia;
+				offset += sizeof(struct opt_ia_addr);
 			}
-
-			offset += sizeof(struct opt_ia_addr);
 		}
 	}
 
-	if (ia_type == OPT_IA_NA) {
-		ia_type = OPT_IA_TA;
-		goto ia_ta;
-	}
-
 	return NULL;
+
+err:
+	info("DHCPv6: requested address %s not on link",
+	     inet_ntop(AF_INET6, &req_addr, buf, sizeof(buf)));
+	return ia;
 }
 
 /**
diff --git a/util.h b/util.h
index 0bf396a..582ef57 100644
--- a/util.h
+++ b/util.h
@@ -102,6 +102,9 @@
 
 #define ARRAY_SIZE(a)		((int)(sizeof(a) / sizeof((a)[0])))
 
+#define foreach(item, array)						\
+	for ((item) = (array); (item) - (array) < ARRAY_SIZE(array); (item)++)
+
 #define IN_INTERVAL(a, b, x)	((x) >= (a) && (x) <= (b))
 #define FD_PROTO(x, proto)						\
 	(IN_INTERVAL(c->proto.fd_min, c->proto.fd_max, (x)))
-- 
@@ -102,6 +102,9 @@
 
 #define ARRAY_SIZE(a)		((int)(sizeof(a) / sizeof((a)[0])))
 
+#define foreach(item, array)						\
+	for ((item) = (array); (item) - (array) < ARRAY_SIZE(array); (item)++)
+
 #define IN_INTERVAL(a, b, x)	((x) >= (a) && (x) <= (b))
 #define FD_PROTO(x, proto)						\
 	(IN_INTERVAL(c->proto.fd_min, c->proto.fd_max, (x)))
-- 
2.43.0

Re: [PATCH 1/6] dhcpv6: Use for loop instead of goto to avoid false positive cppcheck warning

[David Gibson]

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

On Thu, Nov 07, 2024 at 07:43:26PM +0100, Stefano Brivio wrote:
> cppcheck 2.16.0 reports:
> 
> dhcpv6.c:334:14: style: The comparison 'ia_type == 3' is always true. [knownConditionTrueFalse]
>  if (ia_type == OPT_IA_NA) {
>              ^
> dhcpv6.c:306:12: note: 'ia_type' is assigned value '3' here.
>  ia_type = OPT_IA_NA;
>            ^
> dhcpv6.c:334:14: note: The comparison 'ia_type == 3' is always true.
>  if (ia_type == OPT_IA_NA) {
>              ^
> 
> this is not really the case as we set ia_type to OPT_IA_TA and then
> jump back.
> 
> Anyway, there's no particular reason to use a goto here: add a trivial
> foreach() macro to go through elements of an array and use it instead.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

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

> ---
>  dhcpv6.c | 47 +++++++++++++++++++++--------------------------
>  util.h   |  3 +++
>  2 files changed, 24 insertions(+), 26 deletions(-)
> 
> diff --git a/dhcpv6.c b/dhcpv6.c
> index 14a5c7e..f2e7307 100644
> --- a/dhcpv6.c
> +++ b/dhcpv6.c
> @@ -296,47 +296,42 @@ static struct opt_hdr *dhcpv6_opt(const struct pool *p, size_t *offset,
>  static struct opt_hdr *dhcpv6_ia_notonlink(const struct pool *p,
>  					   struct in6_addr *la)
>  {
> +	int ia_types[2] = { OPT_IA_NA, OPT_IA_TA }, *ia_type;
> +	const struct opt_ia_addr *opt_addr;
>  	char buf[INET6_ADDRSTRLEN];
>  	struct in6_addr req_addr;
>  	const struct opt_hdr *h;
>  	struct opt_hdr *ia;
>  	size_t offset;
> -	int ia_type;
>  
> -	ia_type = OPT_IA_NA;
> -ia_ta:
> -	offset = 0;
> -	while ((ia = dhcpv6_opt(p, &offset, ia_type))) {
> -		if (ntohs(ia->l) < OPT_VSIZE(ia_na))
> -			return NULL;
> +	foreach(ia_type, ia_types) {
> +		offset = 0;
> +		while ((ia = dhcpv6_opt(p, &offset, *ia_type))) {
> +			if (ntohs(ia->l) < OPT_VSIZE(ia_na))
> +				return NULL;
>  
> -		offset += sizeof(struct opt_ia_na);
> +			offset += sizeof(struct opt_ia_na);
>  
> -		while ((h = dhcpv6_opt(p, &offset, OPT_IAAADR))) {
> -			const struct opt_ia_addr *opt_addr;
> +			while ((h = dhcpv6_opt(p, &offset, OPT_IAAADR))) {
> +				if (ntohs(h->l) != OPT_VSIZE(ia_addr))
> +					return NULL;
>  
> -			if (ntohs(h->l) != OPT_VSIZE(ia_addr))
> -				return NULL;
> +				opt_addr = (const struct opt_ia_addr *)h;
> +				req_addr = opt_addr->addr;
> +				if (!IN6_ARE_ADDR_EQUAL(la, &req_addr))
> +					goto err;
>  
> -			opt_addr = (const struct opt_ia_addr *)h;
> -			req_addr = opt_addr->addr;
> -			if (!IN6_ARE_ADDR_EQUAL(la, &req_addr)) {
> -				info("DHCPv6: requested address %s not on link",
> -				     inet_ntop(AF_INET6, &req_addr,
> -					       buf, sizeof(buf)));
> -				return ia;
> +				offset += sizeof(struct opt_ia_addr);
>  			}
> -
> -			offset += sizeof(struct opt_ia_addr);
>  		}
>  	}
>  
> -	if (ia_type == OPT_IA_NA) {
> -		ia_type = OPT_IA_TA;
> -		goto ia_ta;
> -	}
> -
>  	return NULL;
> +
> +err:
> +	info("DHCPv6: requested address %s not on link",
> +	     inet_ntop(AF_INET6, &req_addr, buf, sizeof(buf)));
> +	return ia;
>  }
>  
>  /**
> diff --git a/util.h b/util.h
> index 0bf396a..582ef57 100644
> --- a/util.h
> +++ b/util.h
> @@ -102,6 +102,9 @@
>  
>  #define ARRAY_SIZE(a)		((int)(sizeof(a) / sizeof((a)[0])))
>  
> +#define foreach(item, array)						\
> +	for ((item) = (array); (item) - (array) < ARRAY_SIZE(array); (item)++)
> +
>  #define IN_INTERVAL(a, b, x)	((x) >= (a) && (x) <= (b))
>  #define FD_PROTO(x, proto)						\
>  	(IN_INTERVAL(c->proto.fd_min, c->proto.fd_max, (x)))

-- 
David Gibson (he or they)	| 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/6] dhcpv6: Turn some option headers pointers to const

[Stefano Brivio]

cppcheck 2.14.2 on Alpine reports:

dhcpv6.c:431:32: style: Variable 'client_id' can be declared as pointer to const [constVariablePointer]
 struct opt_hdr *ia, *bad_ia, *client_id;
                               ^

It's not only 'client_id': we can declare 'ia' as const pointer too.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 dhcpv6.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/dhcpv6.c b/dhcpv6.c
index f2e7307..0523bba 100644
--- a/dhcpv6.c
+++ b/dhcpv6.c
@@ -423,11 +423,11 @@ search:
 int dhcpv6(struct ctx *c, const struct pool *p,
 	   const struct in6_addr *saddr, const struct in6_addr *daddr)
 {
-	struct opt_hdr *ia, *bad_ia, *client_id;
-	const struct opt_hdr *server_id;
+	const struct opt_hdr *client_id, *server_id, *ia;
 	const struct in6_addr *src;
 	const struct msg_hdr *mh;
 	const struct udphdr *uh;
+	struct opt_hdr *bad_ia;
 	size_t mlen, n;
 
 	uh = packet_get(p, 0, 0, sizeof(*uh), &mlen);
-- 
@@ -423,11 +423,11 @@ search:
 int dhcpv6(struct ctx *c, const struct pool *p,
 	   const struct in6_addr *saddr, const struct in6_addr *daddr)
 {
-	struct opt_hdr *ia, *bad_ia, *client_id;
-	const struct opt_hdr *server_id;
+	const struct opt_hdr *client_id, *server_id, *ia;
 	const struct in6_addr *src;
 	const struct msg_hdr *mh;
 	const struct udphdr *uh;
+	struct opt_hdr *bad_ia;
 	size_t mlen, n;
 
 	uh = packet_get(p, 0, 0, sizeof(*uh), &mlen);
-- 
2.43.0

Re: [PATCH 2/6] dhcpv6: Turn some option headers pointers to const

[David Gibson]

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

On Thu, Nov 07, 2024 at 07:43:27PM +0100, Stefano Brivio wrote:
> cppcheck 2.14.2 on Alpine reports:
> 
> dhcpv6.c:431:32: style: Variable 'client_id' can be declared as pointer to const [constVariablePointer]
>  struct opt_hdr *ia, *bad_ia, *client_id;
>                                ^
> 
> It's not only 'client_id': we can declare 'ia' as const pointer too.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

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

> ---
>  dhcpv6.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/dhcpv6.c b/dhcpv6.c
> index f2e7307..0523bba 100644
> --- a/dhcpv6.c
> +++ b/dhcpv6.c
> @@ -423,11 +423,11 @@ search:
>  int dhcpv6(struct ctx *c, const struct pool *p,
>  	   const struct in6_addr *saddr, const struct in6_addr *daddr)
>  {
> -	struct opt_hdr *ia, *bad_ia, *client_id;
> -	const struct opt_hdr *server_id;
> +	const struct opt_hdr *client_id, *server_id, *ia;
>  	const struct in6_addr *src;
>  	const struct msg_hdr *mh;
>  	const struct udphdr *uh;
> +	struct opt_hdr *bad_ia;
>  	size_t mlen, n;
>  
>  	uh = packet_get(p, 0, 0, sizeof(*uh), &mlen);

-- 
David Gibson (he or they)	| 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/6] tap: Cast TAP_BUF_BYTES - ETH_MAX_MTU to ssize_t, not TAP_BUF_BYTES

[Stefano Brivio]

Given that we're comparing against 'n', which is signed, we cast
TAP_BUF_BYTES to ssize_t so that the maximum buffer usage, calculated
as the difference between TAP_BUF_BYTES and ETH_MAX_MTU, will also be
signed.

This doesn't necessarily happen on 32-bit architectures, though. On
armhf and i686, clang-tidy 18.1.8 and 19.1.2 report:

/home/pi/passt/tap.c:1087:16: error: comparison of integers of different signs: 'ssize_t' (aka 'int') and 'unsigned int' [clang-diagnostic-sign-compare,-warnings-as-errors]
 1087 |         for (n = 0; n <= (ssize_t)TAP_BUF_BYTES - ETH_MAX_MTU; n += len) {
      |                     ~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

cast the whole difference to ssize_t, as we know it's going to be
positive anyway, instead of relying on that side effect.

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

diff --git a/tap.c b/tap.c
index f638f2c..a3ba958 100644
--- a/tap.c
+++ b/tap.c
@@ -1084,7 +1084,7 @@ static void tap_pasta_input(struct ctx *c, const struct timespec *now)
 
 	tap_flush_pools();
 
-	for (n = 0; n <= (ssize_t)TAP_BUF_BYTES - ETH_MAX_MTU; n += len) {
+	for (n = 0; n <= (ssize_t)(TAP_BUF_BYTES - ETH_MAX_MTU); n += len) {
 		len = read(c->fd_tap, pkt_buf + n, ETH_MAX_MTU);
 
 		if (len == 0) {
-- 
@@ -1084,7 +1084,7 @@ static void tap_pasta_input(struct ctx *c, const struct timespec *now)
 
 	tap_flush_pools();
 
-	for (n = 0; n <= (ssize_t)TAP_BUF_BYTES - ETH_MAX_MTU; n += len) {
+	for (n = 0; n <= (ssize_t)(TAP_BUF_BYTES - ETH_MAX_MTU); n += len) {
 		len = read(c->fd_tap, pkt_buf + n, ETH_MAX_MTU);
 
 		if (len == 0) {
-- 
2.43.0

Re: [PATCH 3/6] tap: Cast TAP_BUF_BYTES - ETH_MAX_MTU to ssize_t, not TAP_BUF_BYTES

[David Gibson]

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

On Thu, Nov 07, 2024 at 07:43:28PM +0100, Stefano Brivio wrote:
> Given that we're comparing against 'n', which is signed, we cast
> TAP_BUF_BYTES to ssize_t so that the maximum buffer usage, calculated
> as the difference between TAP_BUF_BYTES and ETH_MAX_MTU, will also be
> signed.
> 
> This doesn't necessarily happen on 32-bit architectures, though. On
> armhf and i686, clang-tidy 18.1.8 and 19.1.2 report:
> 
> /home/pi/passt/tap.c:1087:16: error: comparison of integers of different signs: 'ssize_t' (aka 'int') and 'unsigned int' [clang-diagnostic-sign-compare,-warnings-as-errors]
>  1087 |         for (n = 0; n <= (ssize_t)TAP_BUF_BYTES - ETH_MAX_MTU; n += len) {
>       |                     ~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> cast the whole difference to ssize_t, as we know it's going to be
> positive anyway, instead of relying on that side effect.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

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

> ---
>  tap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tap.c b/tap.c
> index f638f2c..a3ba958 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -1084,7 +1084,7 @@ static void tap_pasta_input(struct ctx *c, const struct timespec *now)
>  
>  	tap_flush_pools();
>  
> -	for (n = 0; n <= (ssize_t)TAP_BUF_BYTES - ETH_MAX_MTU; n += len) {
> +	for (n = 0; n <= (ssize_t)(TAP_BUF_BYTES - ETH_MAX_MTU); n += len) {
>  		len = read(c->fd_tap, pkt_buf + n, ETH_MAX_MTU);
>  
>  		if (len == 0) {

-- 
David Gibson (he or they)	| 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/6] util: Define small and big thresholds for socket buffers as unsigned long long

[Stefano Brivio]

On 32-bit architectures, clang-tidy reports:

/home/pi/passt/tcp.c:728:11: error: performing an implicit widening conversion to type 'uint64_t' (aka 'unsigned long long') of a multiplication performed in type 'unsigned long' [bugprone-implicit-widening-of-multiplication-result,-warnings-as-errors]
  728 |         if (v >= SNDBUF_BIG)
      |                  ^
/home/pi/passt/util.h:158:22: note: expanded from macro 'SNDBUF_BIG'
  158 | #define SNDBUF_BIG              (4UL * 1024 * 1024)
      |                                  ^
/home/pi/passt/tcp.c:728:11: note: make conversion explicit to silence this warning
  728 |         if (v >= SNDBUF_BIG)
      |                  ^
/home/pi/passt/util.h:158:22: note: expanded from macro 'SNDBUF_BIG'
  158 | #define SNDBUF_BIG              (4UL * 1024 * 1024)
      |                                  ^~~~~~~~~~~~~~~~~
/home/pi/passt/tcp.c:728:11: note: perform multiplication in a wider type
  728 |         if (v >= SNDBUF_BIG)
      |                  ^
/home/pi/passt/util.h:158:22: note: expanded from macro 'SNDBUF_BIG'
  158 | #define SNDBUF_BIG              (4UL * 1024 * 1024)
      |                                  ^~~~~~~~~~
/home/pi/passt/tcp.c:730:15: error: performing an implicit widening conversion to type 'uint64_t' (aka 'unsigned long long') of a multiplication performed in type 'unsigned long' [bugprone-implicit-widening-of-multiplication-result,-warnings-as-errors]
  730 |         else if (v > SNDBUF_SMALL)
      |                      ^
/home/pi/passt/util.h:159:24: note: expanded from macro 'SNDBUF_SMALL'
  159 | #define SNDBUF_SMALL            (128UL * 1024)
      |                                  ^
/home/pi/passt/tcp.c:730:15: note: make conversion explicit to silence this warning
  730 |         else if (v > SNDBUF_SMALL)
      |                      ^
/home/pi/passt/util.h:159:24: note: expanded from macro 'SNDBUF_SMALL'
  159 | #define SNDBUF_SMALL            (128UL * 1024)
      |                                  ^~~~~~~~~~~~
/home/pi/passt/tcp.c:730:15: note: perform multiplication in a wider type
  730 |         else if (v > SNDBUF_SMALL)
      |                      ^
/home/pi/passt/util.h:159:24: note: expanded from macro 'SNDBUF_SMALL'
  159 | #define SNDBUF_SMALL            (128UL * 1024)
      |                                  ^~~~~
/home/pi/passt/tcp.c:731:17: error: performing an implicit widening conversion to type 'uint64_t' (aka 'unsigned long long') of a multiplication performed in type 'unsigned long' [bugprone-implicit-widening-of-multiplication-result,-warnings-as-errors]
  731 |                 v -= v * (v - SNDBUF_SMALL) / (SNDBUF_BIG - SNDBUF_SMALL) / 2;
      |                               ^
/home/pi/passt/util.h:159:24: note: expanded from macro 'SNDBUF_SMALL'
  159 | #define SNDBUF_SMALL            (128UL * 1024)
      |                                  ^
/home/pi/passt/tcp.c:731:17: note: make conversion explicit to silence this warning
  731 |                 v -= v * (v - SNDBUF_SMALL) / (SNDBUF_BIG - SNDBUF_SMALL) / 2;
      |                               ^
/home/pi/passt/util.h:159:24: note: expanded from macro 'SNDBUF_SMALL'
  159 | #define SNDBUF_SMALL            (128UL * 1024)
      |                                  ^~~~~~~~~~~~
/home/pi/passt/tcp.c:731:17: note: perform multiplication in a wider type
  731 |                 v -= v * (v - SNDBUF_SMALL) / (SNDBUF_BIG - SNDBUF_SMALL) / 2;
      |                               ^
/home/pi/passt/util.h:159:24: note: expanded from macro 'SNDBUF_SMALL'
  159 | #define SNDBUF_SMALL            (128UL * 1024)
      |                                  ^~~~~

because, wherever we use those thresholds, we define the other term
of comparison as uint64_t. Define the thresholds as unsigned long long
as well, to make sure we match types.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 util.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/util.h b/util.h
index 582ef57..963f57b 100644
--- a/util.h
+++ b/util.h
@@ -158,9 +158,9 @@ int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
 			 (void *)(arg));				\
 	} while (0)
 
-#define RCVBUF_BIG		(2UL * 1024 * 1024)
-#define SNDBUF_BIG		(4UL * 1024 * 1024)
-#define SNDBUF_SMALL		(128UL * 1024)
+#define RCVBUF_BIG		(2ULL * 1024 * 1024)
+#define SNDBUF_BIG		(4ULL * 1024 * 1024)
+#define SNDBUF_SMALL		(128ULL * 1024)
 
 #include <net/if.h>
 #include <limits.h>
-- 
@@ -158,9 +158,9 @@ int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
 			 (void *)(arg));				\
 	} while (0)
 
-#define RCVBUF_BIG		(2UL * 1024 * 1024)
-#define SNDBUF_BIG		(4UL * 1024 * 1024)
-#define SNDBUF_SMALL		(128UL * 1024)
+#define RCVBUF_BIG		(2ULL * 1024 * 1024)
+#define SNDBUF_BIG		(4ULL * 1024 * 1024)
+#define SNDBUF_SMALL		(128ULL * 1024)
 
 #include <net/if.h>
 #include <limits.h>
-- 
2.43.0

Re: [PATCH 4/6] util: Define small and big thresholds for socket buffers as unsigned long long

[David Gibson]

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

On Thu, Nov 07, 2024 at 07:43:29PM +0100, Stefano Brivio wrote:
> On 32-bit architectures, clang-tidy reports:
> 
> /home/pi/passt/tcp.c:728:11: error: performing an implicit widening conversion to type 'uint64_t' (aka 'unsigned long long') of a multiplication performed in type 'unsigned long' [bugprone-implicit-widening-of-multiplication-result,-warnings-as-errors]
>   728 |         if (v >= SNDBUF_BIG)
>       |                  ^
> /home/pi/passt/util.h:158:22: note: expanded from macro 'SNDBUF_BIG'
>   158 | #define SNDBUF_BIG              (4UL * 1024 * 1024)
>       |                                  ^
> /home/pi/passt/tcp.c:728:11: note: make conversion explicit to silence this warning
>   728 |         if (v >= SNDBUF_BIG)
>       |                  ^
> /home/pi/passt/util.h:158:22: note: expanded from macro 'SNDBUF_BIG'
>   158 | #define SNDBUF_BIG              (4UL * 1024 * 1024)
>       |                                  ^~~~~~~~~~~~~~~~~
> /home/pi/passt/tcp.c:728:11: note: perform multiplication in a wider type
>   728 |         if (v >= SNDBUF_BIG)
>       |                  ^
> /home/pi/passt/util.h:158:22: note: expanded from macro 'SNDBUF_BIG'
>   158 | #define SNDBUF_BIG              (4UL * 1024 * 1024)
>       |                                  ^~~~~~~~~~
> /home/pi/passt/tcp.c:730:15: error: performing an implicit widening conversion to type 'uint64_t' (aka 'unsigned long long') of a multiplication performed in type 'unsigned long' [bugprone-implicit-widening-of-multiplication-result,-warnings-as-errors]
>   730 |         else if (v > SNDBUF_SMALL)
>       |                      ^
> /home/pi/passt/util.h:159:24: note: expanded from macro 'SNDBUF_SMALL'
>   159 | #define SNDBUF_SMALL            (128UL * 1024)
>       |                                  ^
> /home/pi/passt/tcp.c:730:15: note: make conversion explicit to silence this warning
>   730 |         else if (v > SNDBUF_SMALL)
>       |                      ^
> /home/pi/passt/util.h:159:24: note: expanded from macro 'SNDBUF_SMALL'
>   159 | #define SNDBUF_SMALL            (128UL * 1024)
>       |                                  ^~~~~~~~~~~~
> /home/pi/passt/tcp.c:730:15: note: perform multiplication in a wider type
>   730 |         else if (v > SNDBUF_SMALL)
>       |                      ^
> /home/pi/passt/util.h:159:24: note: expanded from macro 'SNDBUF_SMALL'
>   159 | #define SNDBUF_SMALL            (128UL * 1024)
>       |                                  ^~~~~
> /home/pi/passt/tcp.c:731:17: error: performing an implicit widening conversion to type 'uint64_t' (aka 'unsigned long long') of a multiplication performed in type 'unsigned long' [bugprone-implicit-widening-of-multiplication-result,-warnings-as-errors]
>   731 |                 v -= v * (v - SNDBUF_SMALL) / (SNDBUF_BIG - SNDBUF_SMALL) / 2;
>       |                               ^
> /home/pi/passt/util.h:159:24: note: expanded from macro 'SNDBUF_SMALL'
>   159 | #define SNDBUF_SMALL            (128UL * 1024)
>       |                                  ^
> /home/pi/passt/tcp.c:731:17: note: make conversion explicit to silence this warning
>   731 |                 v -= v * (v - SNDBUF_SMALL) / (SNDBUF_BIG - SNDBUF_SMALL) / 2;
>       |                               ^
> /home/pi/passt/util.h:159:24: note: expanded from macro 'SNDBUF_SMALL'
>   159 | #define SNDBUF_SMALL            (128UL * 1024)
>       |                                  ^~~~~~~~~~~~
> /home/pi/passt/tcp.c:731:17: note: perform multiplication in a wider type
>   731 |                 v -= v * (v - SNDBUF_SMALL) / (SNDBUF_BIG - SNDBUF_SMALL) / 2;
>       |                               ^
> /home/pi/passt/util.h:159:24: note: expanded from macro 'SNDBUF_SMALL'
>   159 | #define SNDBUF_SMALL            (128UL * 1024)
>       |                                  ^~~~~
> 
> because, wherever we use those thresholds, we define the other term
> of comparison as uint64_t. Define the thresholds as unsigned long long
> as well, to make sure we match types.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

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

> ---
>  util.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/util.h b/util.h
> index 582ef57..963f57b 100644
> --- a/util.h
> +++ b/util.h
> @@ -158,9 +158,9 @@ int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
>  			 (void *)(arg));				\
>  	} while (0)
>  
> -#define RCVBUF_BIG		(2UL * 1024 * 1024)
> -#define SNDBUF_BIG		(4UL * 1024 * 1024)
> -#define SNDBUF_SMALL		(128UL * 1024)
> +#define RCVBUF_BIG		(2ULL * 1024 * 1024)
> +#define SNDBUF_BIG		(4ULL * 1024 * 1024)
> +#define SNDBUF_SMALL		(128ULL * 1024)
>  
>  #include <net/if.h>
>  #include <limits.h>

-- 
David Gibson (he or they)	| 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 5/6] passt: Use NOLINT clang-tidy block instead of NOLINTNEXTLINE

[Stefano Brivio]

For some reason, this is only reported by clang-tidy 19.1.2 on
Alpine:

/home/sbrivio/passt/passt.c:314:53: error: conditional operator with identical true and false expressions [bugprone-branch-clone,-warnings-as-errors]
  314 |         nfds = epoll_wait(c.epollfd, events, EPOLL_EVENTS, TIMER_INTERVAL);
      |                                                            ^

We do have a suppression, but not on the line preceding it, because
we also need a cppcheck suppression there. Use NOLINTBEGIN/NOLINTEND
for the clang-tidy suppression.

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

diff --git a/passt.c b/passt.c
index eaf231d..fac6101 100644
--- a/passt.c
+++ b/passt.c
@@ -309,9 +309,10 @@ int main(int argc, char **argv)
 	timer_init(&c, &now);
 
 loop:
-	/* NOLINTNEXTLINE(bugprone-branch-clone): intervals can be the same */
+	/* NOLINTBEGIN(bugprone-branch-clone): intervals can be the same */
 	/* cppcheck-suppress [duplicateValueTernary, unmatchedSuppression] */
 	nfds = epoll_wait(c.epollfd, events, EPOLL_EVENTS, TIMER_INTERVAL);
+	/* NOLINTEND(bugprone-branch-clone) */
 	if (nfds == -1 && errno != EINTR)
 		die_perror("epoll_wait() failed in main loop");
 
-- 
@@ -309,9 +309,10 @@ int main(int argc, char **argv)
 	timer_init(&c, &now);
 
 loop:
-	/* NOLINTNEXTLINE(bugprone-branch-clone): intervals can be the same */
+	/* NOLINTBEGIN(bugprone-branch-clone): intervals can be the same */
 	/* cppcheck-suppress [duplicateValueTernary, unmatchedSuppression] */
 	nfds = epoll_wait(c.epollfd, events, EPOLL_EVENTS, TIMER_INTERVAL);
+	/* NOLINTEND(bugprone-branch-clone) */
 	if (nfds == -1 && errno != EINTR)
 		die_perror("epoll_wait() failed in main loop");
 
-- 
2.43.0

Re: [PATCH 5/6] passt: Use NOLINT clang-tidy block instead of NOLINTNEXTLINE

[David Gibson]

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

On Thu, Nov 07, 2024 at 07:43:30PM +0100, Stefano Brivio wrote:
> For some reason, this is only reported by clang-tidy 19.1.2 on
> Alpine:
> 
> /home/sbrivio/passt/passt.c:314:53: error: conditional operator with identical true and false expressions [bugprone-branch-clone,-warnings-as-errors]
>   314 |         nfds = epoll_wait(c.epollfd, events, EPOLL_EVENTS, TIMER_INTERVAL);
>       |                                                            ^
> 
> We do have a suppression, but not on the line preceding it, because
> we also need a cppcheck suppression there. Use NOLINTBEGIN/NOLINTEND
> for the clang-tidy suppression.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

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

> ---
>  passt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/passt.c b/passt.c
> index eaf231d..fac6101 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -309,9 +309,10 @@ int main(int argc, char **argv)
>  	timer_init(&c, &now);
>  
>  loop:
> -	/* NOLINTNEXTLINE(bugprone-branch-clone): intervals can be the same */
> +	/* NOLINTBEGIN(bugprone-branch-clone): intervals can be the same */
>  	/* cppcheck-suppress [duplicateValueTernary, unmatchedSuppression] */
>  	nfds = epoll_wait(c.epollfd, events, EPOLL_EVENTS, TIMER_INTERVAL);
> +	/* NOLINTEND(bugprone-branch-clone) */
>  	if (nfds == -1 && errno != EINTR)
>  		die_perror("epoll_wait() failed in main loop");
>  

-- 
David Gibson (he or they)	| 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 6/6] tap, tcp, util: Add some missing SOCK_CLOEXEC flags

[Stefano Brivio]

I have no idea why, but these are reported by clang-tidy (19.2.1) on
Alpine (x86) only:

/home/sbrivio/passt/tap.c:1139:38: error: 'socket' should use SOCK_CLOEXEC where possible [android-cloexec-socket,-warnings-as-errors]
 1139 |         int fd = socket(AF_UNIX, SOCK_STREAM, 0);
      |                                             ^
      |                                              | SOCK_CLOEXEC
/home/sbrivio/passt/tap.c:1158:51: error: 'socket' should use SOCK_CLOEXEC where possible [android-cloexec-socket,-warnings-as-errors]
 1158 |                 ex = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0);
      |                                                                 ^
      |                                                                  | SOCK_CLOEXEC
/home/sbrivio/passt/tcp.c:1413:44: error: 'socket' should use SOCK_CLOEXEC where possible [android-cloexec-socket,-warnings-as-errors]
 1413 |         s = socket(af, SOCK_STREAM | SOCK_NONBLOCK, IPPROTO_TCP);
      |                                                   ^
      |                                                    | SOCK_CLOEXEC
/home/sbrivio/passt/util.c:188:38: error: 'socket' should use SOCK_CLOEXEC where possible [android-cloexec-socket,-warnings-as-errors]
  188 |         if ((s = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP)) < 0) {
      |                                             ^
      |                                              | SOCK_CLOEXEC

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 tap.c  | 5 +++--
 tcp.c  | 2 +-
 util.c | 3 ++-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/tap.c b/tap.c
index a3ba958..14d9b3d 100644
--- a/tap.c
+++ b/tap.c
@@ -1136,7 +1136,7 @@ void tap_handler_pasta(struct ctx *c, uint32_t events,
  */
 int tap_sock_unix_open(char *sock_path)
 {
-	int fd = socket(AF_UNIX, SOCK_STREAM, 0);
+	int fd = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0);
 	struct sockaddr_un addr = {
 		.sun_family = AF_UNIX,
 	};
@@ -1155,7 +1155,8 @@ int tap_sock_unix_open(char *sock_path)
 					UNIX_SOCK_PATH, i))
 			die_perror("Can't build UNIX domain socket path");
 
-		ex = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0);
+		ex = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK | SOCK_CLOEXEC,
+			    0);
 		if (ex < 0)
 			die_perror("Failed to check for UNIX domain conflicts");
 
diff --git a/tcp.c b/tcp.c
index a3d48fa..6a98dfa 100644
--- a/tcp.c
+++ b/tcp.c
@@ -1410,7 +1410,7 @@ static int tcp_conn_new_sock(const struct ctx *c, sa_family_t af)
 {
 	int s;
 
-	s = socket(af, SOCK_STREAM | SOCK_NONBLOCK, IPPROTO_TCP);
+	s = socket(af, SOCK_STREAM | SOCK_NONBLOCK | SOCK_CLOEXEC, IPPROTO_TCP);
 
 	if (s > FD_REF_MAX) {
 		close(s);
diff --git a/util.c b/util.c
index dddef93..3448f30 100644
--- a/util.c
+++ b/util.c
@@ -183,7 +183,8 @@ void sock_probe_mem(struct ctx *c)
 	int v = INT_MAX / 2, s;
 	socklen_t sl;
 
-	if ((s = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP)) < 0) {
+	s = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, IPPROTO_TCP);
+	if (s < 0) {
 		c->low_wmem = c->low_rmem = 1;
 		return;
 	}
-- 
@@ -183,7 +183,8 @@ void sock_probe_mem(struct ctx *c)
 	int v = INT_MAX / 2, s;
 	socklen_t sl;
 
-	if ((s = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP)) < 0) {
+	s = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, IPPROTO_TCP);
+	if (s < 0) {
 		c->low_wmem = c->low_rmem = 1;
 		return;
 	}
-- 
2.43.0

Re: [PATCH 6/6] tap, tcp, util: Add some missing SOCK_CLOEXEC flags

[David Gibson]

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

On Thu, Nov 07, 2024 at 07:43:31PM +0100, Stefano Brivio wrote:
> I have no idea why, but these are reported by clang-tidy (19.2.1) on
> Alpine (x86) only:
> 
> /home/sbrivio/passt/tap.c:1139:38: error: 'socket' should use SOCK_CLOEXEC where possible [android-cloexec-socket,-warnings-as-errors]
>  1139 |         int fd = socket(AF_UNIX, SOCK_STREAM, 0);
>       |                                             ^
>       |                                              | SOCK_CLOEXEC
> /home/sbrivio/passt/tap.c:1158:51: error: 'socket' should use SOCK_CLOEXEC where possible [android-cloexec-socket,-warnings-as-errors]
>  1158 |                 ex = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0);
>       |                                                                 ^
>       |                                                                  | SOCK_CLOEXEC
> /home/sbrivio/passt/tcp.c:1413:44: error: 'socket' should use SOCK_CLOEXEC where possible [android-cloexec-socket,-warnings-as-errors]
>  1413 |         s = socket(af, SOCK_STREAM | SOCK_NONBLOCK, IPPROTO_TCP);
>       |                                                   ^
>       |                                                    | SOCK_CLOEXEC
> /home/sbrivio/passt/util.c:188:38: error: 'socket' should use SOCK_CLOEXEC where possible [android-cloexec-socket,-warnings-as-errors]
>   188 |         if ((s = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP)) < 0) {
>       |                                             ^
>       |                                              | SOCK_CLOEXEC
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

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

> ---
>  tap.c  | 5 +++--
>  tcp.c  | 2 +-
>  util.c | 3 ++-
>  3 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/tap.c b/tap.c
> index a3ba958..14d9b3d 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -1136,7 +1136,7 @@ void tap_handler_pasta(struct ctx *c, uint32_t events,
>   */
>  int tap_sock_unix_open(char *sock_path)
>  {
> -	int fd = socket(AF_UNIX, SOCK_STREAM, 0);
> +	int fd = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0);
>  	struct sockaddr_un addr = {
>  		.sun_family = AF_UNIX,
>  	};
> @@ -1155,7 +1155,8 @@ int tap_sock_unix_open(char *sock_path)
>  					UNIX_SOCK_PATH, i))
>  			die_perror("Can't build UNIX domain socket path");
>  
> -		ex = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0);
> +		ex = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK | SOCK_CLOEXEC,
> +			    0);
>  		if (ex < 0)
>  			die_perror("Failed to check for UNIX domain conflicts");
>  
> diff --git a/tcp.c b/tcp.c
> index a3d48fa..6a98dfa 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -1410,7 +1410,7 @@ static int tcp_conn_new_sock(const struct ctx *c, sa_family_t af)
>  {
>  	int s;
>  
> -	s = socket(af, SOCK_STREAM | SOCK_NONBLOCK, IPPROTO_TCP);
> +	s = socket(af, SOCK_STREAM | SOCK_NONBLOCK | SOCK_CLOEXEC, IPPROTO_TCP);
>  
>  	if (s > FD_REF_MAX) {
>  		close(s);
> diff --git a/util.c b/util.c
> index dddef93..3448f30 100644
> --- a/util.c
> +++ b/util.c
> @@ -183,7 +183,8 @@ void sock_probe_mem(struct ctx *c)
>  	int v = INT_MAX / 2, s;
>  	socklen_t sl;
>  
> -	if ((s = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP)) < 0) {
> +	s = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, IPPROTO_TCP);
> +	if (s < 0) {
>  		c->low_wmem = c->low_rmem = 1;
>  		return;
>  	}

-- 
David Gibson (he or they)	| 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 --]