* [PATCH 1/8] Makefile: Exclude qrap.c from clang-tidy checks
2024-10-24 23:04 [PATCH 0/8] Take care of clang-tidy warnings with LLVM >= 16 Stefano Brivio
@ 2024-10-24 23:04 ` Stefano Brivio
2024-10-25 0:35 ` David Gibson
2024-10-24 23:04 ` [PATCH 2/8] treewide: Comply with CERT C rule ERR33-C for snprintf() Stefano Brivio
` (6 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Stefano Brivio @ 2024-10-24 23:04 UTC (permalink / raw)
To: passt-dev
We'll deprecate qrap(1) soon, and warnings reported by clang-tidy as
of LLVM versions 16 and later would need a bunch of changes there to
be addressed, mostly around CERT C rule ERR33-C and checking return
code from snprintf().
It makes no sense to fix warnings in qrap just for the sake of it, so
officially declare the bitrotting season open.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
Makefile | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Makefile b/Makefile
index 74a9513..2ebc81e 100644
--- a/Makefile
+++ b/Makefile
@@ -271,7 +271,7 @@ docs: README.md
# weird for cases like standalone constants, and causes other
# awkwardness for a bunch of cases we use
-clang-tidy: $(SRCS) $(HEADERS)
+clang-tidy: $(filter-out qrap.c,$(SRCS)) $(HEADERS)
clang-tidy -checks=*,-modernize-*,\
-clang-analyzer-valist.Uninitialized,\
-cppcoreguidelines-init-variables,\
@@ -298,7 +298,7 @@ clang-tidy: $(SRCS) $(HEADERS)
-misc-include-cleaner,\
-cppcoreguidelines-macro-to-enum \
-config='{CheckOptions: [{key: bugprone-suspicious-string-compare.WarnOnImplicitComparison, value: "false"}]}' \
- --warnings-as-errors=* $(SRCS) -- $(filter-out -pie,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) -DCLANG_TIDY_58992
+ --warnings-as-errors=* $(filter-out qrap.c,$(SRCS)) -- $(filter-out -pie,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) -DCLANG_TIDY_58992
SYSTEM_INCLUDES := /usr/include $(wildcard /usr/include/$(TARGET))
ifeq ($(shell $(CC) -v 2>&1 | grep -c "gcc version"),1)
--
@@ -271,7 +271,7 @@ docs: README.md
# weird for cases like standalone constants, and causes other
# awkwardness for a bunch of cases we use
-clang-tidy: $(SRCS) $(HEADERS)
+clang-tidy: $(filter-out qrap.c,$(SRCS)) $(HEADERS)
clang-tidy -checks=*,-modernize-*,\
-clang-analyzer-valist.Uninitialized,\
-cppcoreguidelines-init-variables,\
@@ -298,7 +298,7 @@ clang-tidy: $(SRCS) $(HEADERS)
-misc-include-cleaner,\
-cppcoreguidelines-macro-to-enum \
-config='{CheckOptions: [{key: bugprone-suspicious-string-compare.WarnOnImplicitComparison, value: "false"}]}' \
- --warnings-as-errors=* $(SRCS) -- $(filter-out -pie,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) -DCLANG_TIDY_58992
+ --warnings-as-errors=* $(filter-out qrap.c,$(SRCS)) -- $(filter-out -pie,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) -DCLANG_TIDY_58992
SYSTEM_INCLUDES := /usr/include $(wildcard /usr/include/$(TARGET))
ifeq ($(shell $(CC) -v 2>&1 | grep -c "gcc version"),1)
--
2.43.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 1/8] Makefile: Exclude qrap.c from clang-tidy checks
2024-10-24 23:04 ` [PATCH 1/8] Makefile: Exclude qrap.c from clang-tidy checks Stefano Brivio
@ 2024-10-25 0:35 ` David Gibson
0 siblings, 0 replies; 20+ messages in thread
From: David Gibson @ 2024-10-25 0:35 UTC (permalink / raw)
To: Stefano Brivio; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 1926 bytes --]
On Fri, Oct 25, 2024 at 01:04:31AM +0200, Stefano Brivio wrote:
> We'll deprecate qrap(1) soon, and warnings reported by clang-tidy as
> of LLVM versions 16 and later would need a bunch of changes there to
> be addressed, mostly around CERT C rule ERR33-C and checking return
> code from snprintf().
>
> It makes no sense to fix warnings in qrap just for the sake of it, so
> officially declare the bitrotting season open.
>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> Makefile | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 74a9513..2ebc81e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -271,7 +271,7 @@ docs: README.md
> # weird for cases like standalone constants, and causes other
> # awkwardness for a bunch of cases we use
>
> -clang-tidy: $(SRCS) $(HEADERS)
> +clang-tidy: $(filter-out qrap.c,$(SRCS)) $(HEADERS)
> clang-tidy -checks=*,-modernize-*,\
> -clang-analyzer-valist.Uninitialized,\
> -cppcoreguidelines-init-variables,\
> @@ -298,7 +298,7 @@ clang-tidy: $(SRCS) $(HEADERS)
> -misc-include-cleaner,\
> -cppcoreguidelines-macro-to-enum \
> -config='{CheckOptions: [{key: bugprone-suspicious-string-compare.WarnOnImplicitComparison, value: "false"}]}' \
> - --warnings-as-errors=* $(SRCS) -- $(filter-out -pie,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) -DCLANG_TIDY_58992
> + --warnings-as-errors=* $(filter-out qrap.c,$(SRCS)) -- $(filter-out -pie,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) -DCLANG_TIDY_58992
>
> SYSTEM_INCLUDES := /usr/include $(wildcard /usr/include/$(TARGET))
> ifeq ($(shell $(CC) -v 2>&1 | grep -c "gcc version"),1)
--
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 --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/8] treewide: Comply with CERT C rule ERR33-C for snprintf()
2024-10-24 23:04 [PATCH 0/8] Take care of clang-tidy warnings with LLVM >= 16 Stefano Brivio
2024-10-24 23:04 ` [PATCH 1/8] Makefile: Exclude qrap.c from clang-tidy checks Stefano Brivio
@ 2024-10-24 23:04 ` Stefano Brivio
2024-10-25 0:48 ` David Gibson
2024-10-24 23:04 ` [PATCH 3/8] treewide: Silence cert-err33-c clang-tidy warnings for fprintf() Stefano Brivio
` (5 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Stefano Brivio @ 2024-10-24 23:04 UTC (permalink / raw)
To: passt-dev
clang-tidy, starting from LLVM version 16, up to at least LLVM version
19, now checks that we detect and handle errors for snprintf() as
requested by CERT C rule ERR33-C. These warnings were logged with LLVM
version 19.1.2 (at least Debian and Fedora match):
/home/sbrivio/passt/arch.c:43:3: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [cert-err33-c,-warnings-as-errors]
43 | snprintf(new_path, PATH_MAX + sizeof(".avx2"), "%s.avx2", exe);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/sbrivio/passt/arch.c:43:3: note: cast the expression to void to silence this warning
/home/sbrivio/passt/conf.c:577:4: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [cert-err33-c,-warnings-as-errors]
577 | snprintf(netns, PATH_MAX, "/proc/%ld/ns/net", pidval);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/sbrivio/passt/conf.c:577:4: note: cast the expression to void to silence this warning
/home/sbrivio/passt/conf.c:579:5: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [cert-err33-c,-warnings-as-errors]
579 | snprintf(userns, PATH_MAX, "/proc/%ld/ns/user",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
580 | pidval);
| ~~~~~~~
/home/sbrivio/passt/conf.c:579:5: note: cast the expression to void to silence this warning
/home/sbrivio/passt/pasta.c:105:2: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [cert-err33-c,-warnings-as-errors]
105 | snprintf(ns, PATH_MAX, "/proc/%i/ns/net", pasta_child_pid);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/sbrivio/passt/pasta.c:105:2: note: cast the expression to void to silence this warning
/home/sbrivio/passt/pasta.c:242:2: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [cert-err33-c,-warnings-as-errors]
242 | snprintf(uidmap, BUFSIZ, "0 %u 1", uid);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/sbrivio/passt/pasta.c:242:2: note: cast the expression to void to silence this warning
/home/sbrivio/passt/pasta.c:243:2: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [cert-err33-c,-warnings-as-errors]
243 | snprintf(gidmap, BUFSIZ, "0 %u 1", gid);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/sbrivio/passt/pasta.c:243:2: note: cast the expression to void to silence this warning
/home/sbrivio/passt/tap.c:1155:4: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [cert-err33-c,-warnings-as-errors]
1155 | snprintf(path, UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/sbrivio/passt/tap.c:1155:4: note: cast the expression to void to silence this warning
Don't silence the warnings as they might actually have some merit. Add
an snprintf_check() function, instead, checking that we're not
truncating messages while printing to buffers and terminating if we
do.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
arch.c | 4 +++-
conf.c | 11 +++++++----
pasta.c | 7 ++++---
tap.c | 8 +++++---
util.c | 22 ++++++++++++++++++++++
util.h | 3 +++
6 files changed, 44 insertions(+), 11 deletions(-)
diff --git a/arch.c b/arch.c
index 04bebfc..edbe666 100644
--- a/arch.c
+++ b/arch.c
@@ -19,6 +19,7 @@
#include <unistd.h>
#include "log.h"
+#include "util.h"
/**
* arch_avx2_exec() - Switch to AVX2 build if supported
@@ -40,7 +41,8 @@ void arch_avx2_exec(char **argv)
if (__builtin_cpu_supports("avx2")) {
char new_path[PATH_MAX + sizeof(".avx2")];
- snprintf(new_path, PATH_MAX + sizeof(".avx2"), "%s.avx2", exe);
+ snprintf_check("Can't build AVX2 executable path", new_path,
+ PATH_MAX + sizeof(".avx2"), "%s.avx2", exe);
execve(new_path, argv, environ);
warn_perror("Can't run AVX2 build, using non-AVX2 version");
}
diff --git a/conf.c b/conf.c
index b3b5342..2122600 100644
--- a/conf.c
+++ b/conf.c
@@ -574,10 +574,13 @@ static void conf_pasta_ns(int *netns_only, char *userns, char *netns,
if (pidval < 0 || pidval > INT_MAX)
die("Invalid PID %s", argv[optind]);
- snprintf(netns, PATH_MAX, "/proc/%ld/ns/net", pidval);
- if (!*userns)
- snprintf(userns, PATH_MAX, "/proc/%ld/ns/user",
- pidval);
+ snprintf_check("Can't build netns path", netns,
+ PATH_MAX, "/proc/%ld/ns/net", pidval);
+ if (!*userns) {
+ snprintf_check("Can't build userns path",
+ userns, PATH_MAX,
+ "/proc/%ld/ns/user", pidval);
+ }
}
}
diff --git a/pasta.c b/pasta.c
index 307fb4a..ce9ae7a 100644
--- a/pasta.c
+++ b/pasta.c
@@ -102,7 +102,8 @@ static int pasta_wait_for_ns(void *arg)
int flags = O_RDONLY | O_CLOEXEC;
char ns[PATH_MAX];
- snprintf(ns, PATH_MAX, "/proc/%i/ns/net", pasta_child_pid);
+ snprintf_check("Can't build netns path", ns,
+ PATH_MAX, "/proc/%i/ns/net", pasta_child_pid);
do {
while ((c->pasta_netns_fd = open(ns, flags)) < 0) {
if (errno != ENOENT)
@@ -239,8 +240,8 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid,
c->quiet = 1;
/* Configure user and group mappings */
- snprintf(uidmap, BUFSIZ, "0 %u 1", uid);
- snprintf(gidmap, BUFSIZ, "0 %u 1", gid);
+ snprintf_check("Can't build uidmap", uidmap, BUFSIZ, "0 %u 1", uid);
+ snprintf_check("Can't build gidmap", gidmap, BUFSIZ, "0 %u 1", gid);
if (write_file("/proc/self/uid_map", uidmap) ||
write_file("/proc/self/setgroups", "deny") ||
diff --git a/tap.c b/tap.c
index c53a39b..51eb134 100644
--- a/tap.c
+++ b/tap.c
@@ -1149,10 +1149,12 @@ int tap_sock_unix_open(char *sock_path)
char *path = addr.sun_path;
int ex, ret;
- if (*sock_path)
+ if (*sock_path) {
memcpy(path, sock_path, UNIX_PATH_MAX);
- else
- snprintf(path, UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i);
+ } else {
+ snprintf_check("Can't build UNIX domain path", path,
+ UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i);
+ }
ex = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0);
if (ex < 0)
diff --git a/util.c b/util.c
index eba7d52..eff6787 100644
--- a/util.c
+++ b/util.c
@@ -749,3 +749,25 @@ void close_open_files(int argc, char **argv)
if (rc)
die_perror("Failed to close files leaked by parent");
}
+
+/**
+ * snprintf_check() - snprintf() wrapper, terminating on truncation
+ * @errmsg: Error string to be printed while terminating
+ * @str: Output buffer
+ * @size: Maximum size to write to @str
+ * @format: Message
+ */
+void snprintf_check(const char *errstr,
+ char *str, size_t size, const char *format, ...)
+{
+ va_list ap;
+ int rc;
+
+ va_start(ap, format);
+
+ rc = snprintf(str, size, format, ap);
+ if (rc < 0 || (size_t)rc >= size)
+ die("%s", errstr);
+
+ va_end(ap);
+}
diff --git a/util.h b/util.h
index 2c1e08e..8449d00 100644
--- a/util.h
+++ b/util.h
@@ -11,6 +11,7 @@
#include <stdbool.h>
#include <stddef.h>
#include <stdint.h>
+#include <stdio.h>
#include <string.h>
#include <signal.h>
#include <arpa/inet.h>
@@ -200,6 +201,8 @@ int write_file(const char *path, const char *buf);
int write_all_buf(int fd, const void *buf, size_t len);
int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip);
void close_open_files(int argc, char **argv);
+void snprintf_check(const char *errstr,
+ char *str, size_t size, const char *format, ...);
/**
* af_name() - Return name of an address family
--
@@ -11,6 +11,7 @@
#include <stdbool.h>
#include <stddef.h>
#include <stdint.h>
+#include <stdio.h>
#include <string.h>
#include <signal.h>
#include <arpa/inet.h>
@@ -200,6 +201,8 @@ int write_file(const char *path, const char *buf);
int write_all_buf(int fd, const void *buf, size_t len);
int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip);
void close_open_files(int argc, char **argv);
+void snprintf_check(const char *errstr,
+ char *str, size_t size, const char *format, ...);
/**
* af_name() - Return name of an address family
--
2.43.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 2/8] treewide: Comply with CERT C rule ERR33-C for snprintf()
2024-10-24 23:04 ` [PATCH 2/8] treewide: Comply with CERT C rule ERR33-C for snprintf() Stefano Brivio
@ 2024-10-25 0:48 ` David Gibson
2024-10-25 7:53 ` Stefano Brivio
0 siblings, 1 reply; 20+ messages in thread
From: David Gibson @ 2024-10-25 0:48 UTC (permalink / raw)
To: Stefano Brivio; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 9574 bytes --]
On Fri, Oct 25, 2024 at 01:04:32AM +0200, Stefano Brivio wrote:
> clang-tidy, starting from LLVM version 16, up to at least LLVM version
> 19, now checks that we detect and handle errors for snprintf() as
> requested by CERT C rule ERR33-C. These warnings were logged with LLVM
> version 19.1.2 (at least Debian and Fedora match):
>
> /home/sbrivio/passt/arch.c:43:3: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [cert-err33-c,-warnings-as-errors]
> 43 | snprintf(new_path, PATH_MAX + sizeof(".avx2"), "%s.avx2", exe);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /home/sbrivio/passt/arch.c:43:3: note: cast the expression to void to silence this warning
> /home/sbrivio/passt/conf.c:577:4: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [cert-err33-c,-warnings-as-errors]
> 577 | snprintf(netns, PATH_MAX, "/proc/%ld/ns/net", pidval);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /home/sbrivio/passt/conf.c:577:4: note: cast the expression to void to silence this warning
> /home/sbrivio/passt/conf.c:579:5: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [cert-err33-c,-warnings-as-errors]
> 579 | snprintf(userns, PATH_MAX, "/proc/%ld/ns/user",
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 580 | pidval);
> | ~~~~~~~
> /home/sbrivio/passt/conf.c:579:5: note: cast the expression to void to silence this warning
> /home/sbrivio/passt/pasta.c:105:2: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [cert-err33-c,-warnings-as-errors]
> 105 | snprintf(ns, PATH_MAX, "/proc/%i/ns/net", pasta_child_pid);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /home/sbrivio/passt/pasta.c:105:2: note: cast the expression to void to silence this warning
> /home/sbrivio/passt/pasta.c:242:2: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [cert-err33-c,-warnings-as-errors]
> 242 | snprintf(uidmap, BUFSIZ, "0 %u 1", uid);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /home/sbrivio/passt/pasta.c:242:2: note: cast the expression to void to silence this warning
> /home/sbrivio/passt/pasta.c:243:2: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [cert-err33-c,-warnings-as-errors]
> 243 | snprintf(gidmap, BUFSIZ, "0 %u 1", gid);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /home/sbrivio/passt/pasta.c:243:2: note: cast the expression to void to silence this warning
> /home/sbrivio/passt/tap.c:1155:4: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [cert-err33-c,-warnings-as-errors]
> 1155 | snprintf(path, UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /home/sbrivio/passt/tap.c:1155:4: note: cast the expression to void to silence this warning
>
> Don't silence the warnings as they might actually have some merit. Add
> an snprintf_check() function, instead, checking that we're not
> truncating messages while printing to buffers and terminating if we
> do.
Huh, I wonder why I wasn't seeing these with clang 18.1.8.1. Still,
LGTM except for a couple of nits.
>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
> arch.c | 4 +++-
> conf.c | 11 +++++++----
> pasta.c | 7 ++++---
> tap.c | 8 +++++---
> util.c | 22 ++++++++++++++++++++++
> util.h | 3 +++
> 6 files changed, 44 insertions(+), 11 deletions(-)
>
> diff --git a/arch.c b/arch.c
> index 04bebfc..edbe666 100644
> --- a/arch.c
> +++ b/arch.c
> @@ -19,6 +19,7 @@
> #include <unistd.h>
>
> #include "log.h"
> +#include "util.h"
>
> /**
> * arch_avx2_exec() - Switch to AVX2 build if supported
> @@ -40,7 +41,8 @@ void arch_avx2_exec(char **argv)
> if (__builtin_cpu_supports("avx2")) {
> char new_path[PATH_MAX + sizeof(".avx2")];
>
> - snprintf(new_path, PATH_MAX + sizeof(".avx2"), "%s.avx2", exe);
> + snprintf_check("Can't build AVX2 executable path", new_path,
For all of these messages I'd suggest "XXX path is too long" rather
than just "Can't build XXX path" in order to be more explicit and give
users a clue to a workaround.
> + PATH_MAX + sizeof(".avx2"), "%s.avx2", exe);
> execve(new_path, argv, environ);
> warn_perror("Can't run AVX2 build, using non-AVX2 version");
> }
> diff --git a/conf.c b/conf.c
> index b3b5342..2122600 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -574,10 +574,13 @@ static void conf_pasta_ns(int *netns_only, char *userns, char *netns,
> if (pidval < 0 || pidval > INT_MAX)
> die("Invalid PID %s", argv[optind]);
>
> - snprintf(netns, PATH_MAX, "/proc/%ld/ns/net", pidval);
> - if (!*userns)
> - snprintf(userns, PATH_MAX, "/proc/%ld/ns/user",
> - pidval);
> + snprintf_check("Can't build netns path", netns,
> + PATH_MAX, "/proc/%ld/ns/net", pidval);
> + if (!*userns) {
> + snprintf_check("Can't build userns path",
> + userns, PATH_MAX,
> + "/proc/%ld/ns/user", pidval);
> + }
> }
> }
>
> diff --git a/pasta.c b/pasta.c
> index 307fb4a..ce9ae7a 100644
> --- a/pasta.c
> +++ b/pasta.c
> @@ -102,7 +102,8 @@ static int pasta_wait_for_ns(void *arg)
> int flags = O_RDONLY | O_CLOEXEC;
> char ns[PATH_MAX];
>
> - snprintf(ns, PATH_MAX, "/proc/%i/ns/net", pasta_child_pid);
> + snprintf_check("Can't build netns path", ns,
> + PATH_MAX, "/proc/%i/ns/net", pasta_child_pid);
> do {
> while ((c->pasta_netns_fd = open(ns, flags)) < 0) {
> if (errno != ENOENT)
> @@ -239,8 +240,8 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid,
> c->quiet = 1;
>
> /* Configure user and group mappings */
> - snprintf(uidmap, BUFSIZ, "0 %u 1", uid);
> - snprintf(gidmap, BUFSIZ, "0 %u 1", gid);
> + snprintf_check("Can't build uidmap", uidmap, BUFSIZ, "0 %u 1", uid);
> + snprintf_check("Can't build gidmap", gidmap, BUFSIZ, "0 %u 1", gid);
>
> if (write_file("/proc/self/uid_map", uidmap) ||
> write_file("/proc/self/setgroups", "deny") ||
> diff --git a/tap.c b/tap.c
> index c53a39b..51eb134 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -1149,10 +1149,12 @@ int tap_sock_unix_open(char *sock_path)
> char *path = addr.sun_path;
> int ex, ret;
>
> - if (*sock_path)
> + if (*sock_path) {
> memcpy(path, sock_path, UNIX_PATH_MAX);
> - else
> - snprintf(path, UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i);
> + } else {
> + snprintf_check("Can't build UNIX domain path", path,
I'd suggest explicitly saying "Unix domain _socket_",
> + UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i);
> + }
>
> ex = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0);
> if (ex < 0)
> diff --git a/util.c b/util.c
> index eba7d52..eff6787 100644
> --- a/util.c
> +++ b/util.c
> @@ -749,3 +749,25 @@ void close_open_files(int argc, char **argv)
> if (rc)
> die_perror("Failed to close files leaked by parent");
> }
> +
> +/**
> + * snprintf_check() - snprintf() wrapper, terminating on truncation
> + * @errmsg: Error string to be printed while terminating
> + * @str: Output buffer
> + * @size: Maximum size to write to @str
> + * @format: Message
> + */
> +void snprintf_check(const char *errstr,
> + char *str, size_t size, const char *format, ...)
YMMV, but I always find passing a format / error string into a
function that's not primarily about error handling kind of clunky.
How much bulkier would it be to make this wrapper return a boolean and
do an explicit:
if (!sprintf_check(...))
die("blah blah blah");
at the call sites? It might make the control flow a little more
obvious, and means we could add parameters to the error message if
there are cases where that makes sense.
> +{
> + va_list ap;
> + int rc;
> +
> + va_start(ap, format);
> +
> + rc = snprintf(str, size, format, ap);
> + if (rc < 0 || (size_t)rc >= size)
> + die("%s", errstr);
> +
> + va_end(ap);
> +}
> diff --git a/util.h b/util.h
> index 2c1e08e..8449d00 100644
> --- a/util.h
> +++ b/util.h
> @@ -11,6 +11,7 @@
> #include <stdbool.h>
> #include <stddef.h>
> #include <stdint.h>
> +#include <stdio.h>
> #include <string.h>
> #include <signal.h>
> #include <arpa/inet.h>
> @@ -200,6 +201,8 @@ int write_file(const char *path, const char *buf);
> int write_all_buf(int fd, const void *buf, size_t len);
> int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip);
> void close_open_files(int argc, char **argv);
> +void snprintf_check(const char *errstr,
> + char *str, size_t size, const char *format, ...);
>
> /**
> * af_name() - Return name of an address family
--
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 --]
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/8] treewide: Comply with CERT C rule ERR33-C for snprintf()
2024-10-25 0:48 ` David Gibson
@ 2024-10-25 7:53 ` Stefano Brivio
0 siblings, 0 replies; 20+ messages in thread
From: Stefano Brivio @ 2024-10-25 7:53 UTC (permalink / raw)
To: David Gibson; +Cc: passt-dev
On Fri, 25 Oct 2024 11:48:55 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Fri, Oct 25, 2024 at 01:04:32AM +0200, Stefano Brivio wrote:
> > clang-tidy, starting from LLVM version 16, up to at least LLVM version
> > 19, now checks that we detect and handle errors for snprintf() as
> > requested by CERT C rule ERR33-C. These warnings were logged with LLVM
> > version 19.1.2 (at least Debian and Fedora match):
> >
> > /home/sbrivio/passt/arch.c:43:3: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [cert-err33-c,-warnings-as-errors]
> > 43 | snprintf(new_path, PATH_MAX + sizeof(".avx2"), "%s.avx2", exe);
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > /home/sbrivio/passt/arch.c:43:3: note: cast the expression to void to silence this warning
> > /home/sbrivio/passt/conf.c:577:4: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [cert-err33-c,-warnings-as-errors]
> > 577 | snprintf(netns, PATH_MAX, "/proc/%ld/ns/net", pidval);
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > /home/sbrivio/passt/conf.c:577:4: note: cast the expression to void to silence this warning
> > /home/sbrivio/passt/conf.c:579:5: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [cert-err33-c,-warnings-as-errors]
> > 579 | snprintf(userns, PATH_MAX, "/proc/%ld/ns/user",
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 580 | pidval);
> > | ~~~~~~~
> > /home/sbrivio/passt/conf.c:579:5: note: cast the expression to void to silence this warning
> > /home/sbrivio/passt/pasta.c:105:2: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [cert-err33-c,-warnings-as-errors]
> > 105 | snprintf(ns, PATH_MAX, "/proc/%i/ns/net", pasta_child_pid);
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > /home/sbrivio/passt/pasta.c:105:2: note: cast the expression to void to silence this warning
> > /home/sbrivio/passt/pasta.c:242:2: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [cert-err33-c,-warnings-as-errors]
> > 242 | snprintf(uidmap, BUFSIZ, "0 %u 1", uid);
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > /home/sbrivio/passt/pasta.c:242:2: note: cast the expression to void to silence this warning
> > /home/sbrivio/passt/pasta.c:243:2: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [cert-err33-c,-warnings-as-errors]
> > 243 | snprintf(gidmap, BUFSIZ, "0 %u 1", gid);
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > /home/sbrivio/passt/pasta.c:243:2: note: cast the expression to void to silence this warning
> > /home/sbrivio/passt/tap.c:1155:4: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [cert-err33-c,-warnings-as-errors]
> > 1155 | snprintf(path, UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i);
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > /home/sbrivio/passt/tap.c:1155:4: note: cast the expression to void to silence this warning
> >
> > Don't silence the warnings as they might actually have some merit. Add
> > an snprintf_check() function, instead, checking that we're not
> > truncating messages while printing to buffers and terminating if we
> > do.
>
> Huh, I wonder why I wasn't seeing these with clang 18.1.8.1. Still,
> LGTM except for a couple of nits.
Somewhat interestingly, I also don't see those on Fedora 40 with LLVM
18.1.6, but I see them on Fedora Rawhide with LLVM 19, and on Debian
testing with both LLVM 16 and LLVM 19.
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > ---
> > arch.c | 4 +++-
> > conf.c | 11 +++++++----
> > pasta.c | 7 ++++---
> > tap.c | 8 +++++---
> > util.c | 22 ++++++++++++++++++++++
> > util.h | 3 +++
> > 6 files changed, 44 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch.c b/arch.c
> > index 04bebfc..edbe666 100644
> > --- a/arch.c
> > +++ b/arch.c
> > @@ -19,6 +19,7 @@
> > #include <unistd.h>
> >
> > #include "log.h"
> > +#include "util.h"
> >
> > /**
> > * arch_avx2_exec() - Switch to AVX2 build if supported
> > @@ -40,7 +41,8 @@ void arch_avx2_exec(char **argv)
> > if (__builtin_cpu_supports("avx2")) {
> > char new_path[PATH_MAX + sizeof(".avx2")];
> >
> > - snprintf(new_path, PATH_MAX + sizeof(".avx2"), "%s.avx2", exe);
> > + snprintf_check("Can't build AVX2 executable path", new_path,
>
> For all of these messages I'd suggest "XXX path is too long" rather
> than just "Can't build XXX path" in order to be more explicit and give
> users a clue to a workaround.
I originally wanted to do that, but snprintf() could return a negative
value on "output error", which we should never get, but what if we have
a C library interpreting that loosely and including conversion errors?
Then we can't (practically) be more specific, unless we print messages
in snprintf_check() itself, but you're suggesting we avoid doing that
below, so I'd rather keep those as they are. We'll never print these
anyway.
> > + PATH_MAX + sizeof(".avx2"), "%s.avx2", exe);
> > execve(new_path, argv, environ);
> > warn_perror("Can't run AVX2 build, using non-AVX2 version");
> > }
> > diff --git a/conf.c b/conf.c
> > index b3b5342..2122600 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -574,10 +574,13 @@ static void conf_pasta_ns(int *netns_only, char *userns, char *netns,
> > if (pidval < 0 || pidval > INT_MAX)
> > die("Invalid PID %s", argv[optind]);
> >
> > - snprintf(netns, PATH_MAX, "/proc/%ld/ns/net", pidval);
> > - if (!*userns)
> > - snprintf(userns, PATH_MAX, "/proc/%ld/ns/user",
> > - pidval);
> > + snprintf_check("Can't build netns path", netns,
> > + PATH_MAX, "/proc/%ld/ns/net", pidval);
> > + if (!*userns) {
> > + snprintf_check("Can't build userns path",
> > + userns, PATH_MAX,
> > + "/proc/%ld/ns/user", pidval);
> > + }
> > }
> > }
> >
> > diff --git a/pasta.c b/pasta.c
> > index 307fb4a..ce9ae7a 100644
> > --- a/pasta.c
> > +++ b/pasta.c
> > @@ -102,7 +102,8 @@ static int pasta_wait_for_ns(void *arg)
> > int flags = O_RDONLY | O_CLOEXEC;
> > char ns[PATH_MAX];
> >
> > - snprintf(ns, PATH_MAX, "/proc/%i/ns/net", pasta_child_pid);
> > + snprintf_check("Can't build netns path", ns,
> > + PATH_MAX, "/proc/%i/ns/net", pasta_child_pid);
> > do {
> > while ((c->pasta_netns_fd = open(ns, flags)) < 0) {
> > if (errno != ENOENT)
> > @@ -239,8 +240,8 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid,
> > c->quiet = 1;
> >
> > /* Configure user and group mappings */
> > - snprintf(uidmap, BUFSIZ, "0 %u 1", uid);
> > - snprintf(gidmap, BUFSIZ, "0 %u 1", gid);
> > + snprintf_check("Can't build uidmap", uidmap, BUFSIZ, "0 %u 1", uid);
> > + snprintf_check("Can't build gidmap", gidmap, BUFSIZ, "0 %u 1", gid);
> >
> > if (write_file("/proc/self/uid_map", uidmap) ||
> > write_file("/proc/self/setgroups", "deny") ||
> > diff --git a/tap.c b/tap.c
> > index c53a39b..51eb134 100644
> > --- a/tap.c
> > +++ b/tap.c
> > @@ -1149,10 +1149,12 @@ int tap_sock_unix_open(char *sock_path)
> > char *path = addr.sun_path;
> > int ex, ret;
> >
> > - if (*sock_path)
> > + if (*sock_path) {
> > memcpy(path, sock_path, UNIX_PATH_MAX);
> > - else
> > - snprintf(path, UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i);
> > + } else {
> > + snprintf_check("Can't build UNIX domain path", path,
>
> I'd suggest explicitly saying "Unix domain _socket_",
Oops, changed.
> > + UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i);
> > + }
> >
> > ex = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0);
> > if (ex < 0)
> > diff --git a/util.c b/util.c
> > index eba7d52..eff6787 100644
> > --- a/util.c
> > +++ b/util.c
> > @@ -749,3 +749,25 @@ void close_open_files(int argc, char **argv)
> > if (rc)
> > die_perror("Failed to close files leaked by parent");
> > }
> > +
> > +/**
> > + * snprintf_check() - snprintf() wrapper, terminating on truncation
> > + * @errmsg: Error string to be printed while terminating
> > + * @str: Output buffer
> > + * @size: Maximum size to write to @str
> > + * @format: Message
> > + */
> > +void snprintf_check(const char *errstr,
> > + char *str, size_t size, const char *format, ...)
>
> YMMV, but I always find passing a format / error string into a
> function that's not primarily about error handling kind of clunky.
> How much bulkier would it be to make this wrapper return a boolean and
> do an explicit:
> if (!sprintf_check(...))
> die("blah blah blah");
>
> at the call sites? It might make the control flow a little more
> obvious, and means we could add parameters to the error message if
> there are cases where that makes sense.
Right, yes. Changed, but with false (0) meaning success instead of
failure, for consistency.
--
Stefano
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/8] treewide: Silence cert-err33-c clang-tidy warnings for fprintf()
2024-10-24 23:04 [PATCH 0/8] Take care of clang-tidy warnings with LLVM >= 16 Stefano Brivio
2024-10-24 23:04 ` [PATCH 1/8] Makefile: Exclude qrap.c from clang-tidy checks Stefano Brivio
2024-10-24 23:04 ` [PATCH 2/8] treewide: Comply with CERT C rule ERR33-C for snprintf() Stefano Brivio
@ 2024-10-24 23:04 ` Stefano Brivio
2024-10-25 0:52 ` David Gibson
2024-10-24 23:04 ` [PATCH 4/8] Makefile: Disable readability-math-missing-parentheses clang-tidy check Stefano Brivio
` (4 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Stefano Brivio @ 2024-10-24 23:04 UTC (permalink / raw)
To: passt-dev
We use fprintf() to print to standard output or standard error
streams. If something gets truncated or there's an output error, we
don't really want to try and report that, and at the same time it's
not abnormal behaviour upon which we should terminate, either.
Just silence the warning with an ugly FPRINTF() variadic macro casting
the fprintf() expressions to void.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
conf.c | 46 +++++++++++++++++++++++-----------------------
log.c | 6 +++---
util.h | 3 +++
3 files changed, 29 insertions(+), 26 deletions(-)
diff --git a/conf.c b/conf.c
index 2122600..9af15fe 100644
--- a/conf.c
+++ b/conf.c
@@ -731,19 +731,19 @@ static unsigned int conf_ip6(unsigned int ifi, struct ip6_ctx *ip6)
static void usage(const char *name, FILE *f, int status)
{
if (strstr(name, "pasta")) {
- fprintf(f, "Usage: %s [OPTION]... [COMMAND] [ARGS]...\n", name);
- fprintf(f, " %s [OPTION]... PID\n", name);
- fprintf(f, " %s [OPTION]... --netns [PATH|NAME]\n", name);
- fprintf(f,
+ FPRINTF(f, "Usage: %s [OPTION]... [COMMAND] [ARGS]...\n", name);
+ FPRINTF(f, " %s [OPTION]... PID\n", name);
+ FPRINTF(f, " %s [OPTION]... --netns [PATH|NAME]\n", name);
+ FPRINTF(f,
"\n"
"Without PID or --netns, run the given command or a\n"
"default shell in a new network and user namespace, and\n"
"connect it via pasta.\n");
} else {
- fprintf(f, "Usage: %s [OPTION]...\n", name);
+ FPRINTF(f, "Usage: %s [OPTION]...\n", name);
}
- fprintf(f,
+ FPRINTF(f,
"\n"
" -d, --debug Be verbose\n"
" --trace Be extra verbose, implies --debug\n"
@@ -760,17 +760,17 @@ static void usage(const char *name, FILE *f, int status)
" --version Show version and exit\n");
if (strstr(name, "pasta")) {
- fprintf(f,
+ FPRINTF(f,
" -I, --ns-ifname NAME namespace interface name\n"
" default: same interface name as external one\n");
} else {
- fprintf(f,
+ FPRINTF(f,
" -s, --socket PATH UNIX domain socket path\n"
" default: probe free path starting from "
UNIX_SOCK_PATH "\n", 1);
}
- fprintf(f,
+ FPRINTF(f,
" -F, --fd FD Use FD as pre-opened connected socket\n"
" -p, --pcap FILE Log tap-facing traffic to pcap file\n"
" -P, --pid FILE Write own PID to the given file\n"
@@ -801,28 +801,28 @@ static void usage(const char *name, FILE *f, int status)
" can be specified multiple times\n"
" a single, empty option disables DNS information\n");
if (strstr(name, "pasta"))
- fprintf(f, " default: don't use any addresses\n");
+ FPRINTF(f, " default: don't use any addresses\n");
else
- fprintf(f, " default: use addresses from /etc/resolv.conf\n");
- fprintf(f,
+ FPRINTF(f, " default: use addresses from /etc/resolv.conf\n");
+ FPRINTF(f,
" -S, --search LIST Space-separated list, search domains\n"
" a single, empty option disables the DNS search list\n");
if (strstr(name, "pasta"))
- fprintf(f, " default: don't use any search list\n");
+ FPRINTF(f, " default: don't use any search list\n");
else
- fprintf(f, " default: use search list from /etc/resolv.conf\n");
+ FPRINTF(f, " default: use search list from /etc/resolv.conf\n");
if (strstr(name, "pasta"))
- fprintf(f, " --dhcp-dns \tPass DNS list via DHCP/DHCPv6/NDP\n");
+ FPRINTF(f, " --dhcp-dns \tPass DNS list via DHCP/DHCPv6/NDP\n");
else
- fprintf(f, " --no-dhcp-dns No DNS list in DHCP/DHCPv6/NDP\n");
+ FPRINTF(f, " --no-dhcp-dns No DNS list in DHCP/DHCPv6/NDP\n");
if (strstr(name, "pasta"))
- fprintf(f, " --dhcp-search Pass list via DHCP/DHCPv6/NDP\n");
+ FPRINTF(f, " --dhcp-search Pass list via DHCP/DHCPv6/NDP\n");
else
- fprintf(f, " --no-dhcp-search No list in DHCP/DHCPv6/NDP\n");
+ FPRINTF(f, " --no-dhcp-search No list in DHCP/DHCPv6/NDP\n");
- fprintf(f,
+ FPRINTF(f,
" --map-host-loopback ADDR Translate ADDR to refer to host\n"
" can be specified zero to two times (for IPv4 and IPv6)\n"
" default: gateway address\n"
@@ -850,7 +850,7 @@ static void usage(const char *name, FILE *f, int status)
if (strstr(name, "pasta"))
goto pasta_opts;
- fprintf(f,
+ FPRINTF(f,
" -1, --one-off Quit after handling one single client\n"
" -t, --tcp-ports SPEC TCP port forwarding to guest\n"
" can be specified multiple times\n"
@@ -881,7 +881,7 @@ static void usage(const char *name, FILE *f, int status)
pasta_opts:
- fprintf(f,
+ FPRINTF(f,
" -t, --tcp-ports SPEC TCP port forwarding to namespace\n"
" can be specified multiple times\n"
" SPEC can be:\n"
@@ -1419,9 +1419,9 @@ void conf(struct ctx *c, int argc, char **argv)
break;
case 14:
- fprintf(stdout,
+ FPRINTF(stdout,
c->mode == MODE_PASTA ? "pasta " : "passt ");
- fprintf(stdout, VERSION_BLOB);
+ FPRINTF(stdout, VERSION_BLOB);
exit(EXIT_SUCCESS);
case 15:
ret = snprintf(c->ip4.ifname_out,
diff --git a/log.c b/log.c
index a61468e..6932885 100644
--- a/log.c
+++ b/log.c
@@ -274,7 +274,7 @@ void vlogmsg(bool newline, bool cont, int pri, const char *format, va_list ap)
char timestr[LOGTIME_STRLEN];
logtime_fmt(timestr, sizeof(timestr), now);
- fprintf(stderr, "%s: ", timestr);
+ FPRINTF(stderr, "%s: ", timestr);
}
if ((log_mask & LOG_MASK(LOG_PRI(pri))) || !log_conf_parsed) {
@@ -293,7 +293,7 @@ void vlogmsg(bool newline, bool cont, int pri, const char *format, va_list ap)
(log_stderr && (log_mask & LOG_MASK(LOG_PRI(pri))))) {
(void)vfprintf(stderr, format, ap);
if (newline && format[strlen(format)] != '\n')
- fprintf(stderr, "\n");
+ FPRINTF(stderr, "\n");
}
}
@@ -399,7 +399,7 @@ void passt_vsyslog(bool newline, int pri, const char *format, va_list ap)
n += snprintf(buf + n, BUFSIZ - n, "\n");
if (log_sock >= 0 && send(log_sock, buf, n, 0) != n && log_stderr)
- fprintf(stderr, "Failed to send %i bytes to syslog\n", n);
+ FPRINTF(stderr, "Failed to send %i bytes to syslog\n", n);
}
/**
diff --git a/util.h b/util.h
index 8449d00..d614094 100644
--- a/util.h
+++ b/util.h
@@ -270,6 +270,9 @@ static inline bool mod_between(unsigned x, unsigned i, unsigned j, unsigned m)
return mod_sub(x, i, m) < mod_sub(j, i, m);
}
+/* FPRINTF() intentionally silences cert-err33-c clang-tidy warnings */
+#define FPRINTF(f, ...) (void)fprintf(f, __VA_ARGS__)
+
/*
* Workarounds for https://github.com/llvm/llvm-project/issues/58992
*
--
@@ -270,6 +270,9 @@ static inline bool mod_between(unsigned x, unsigned i, unsigned j, unsigned m)
return mod_sub(x, i, m) < mod_sub(j, i, m);
}
+/* FPRINTF() intentionally silences cert-err33-c clang-tidy warnings */
+#define FPRINTF(f, ...) (void)fprintf(f, __VA_ARGS__)
+
/*
* Workarounds for https://github.com/llvm/llvm-project/issues/58992
*
--
2.43.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 3/8] treewide: Silence cert-err33-c clang-tidy warnings for fprintf()
2024-10-24 23:04 ` [PATCH 3/8] treewide: Silence cert-err33-c clang-tidy warnings for fprintf() Stefano Brivio
@ 2024-10-25 0:52 ` David Gibson
0 siblings, 0 replies; 20+ messages in thread
From: David Gibson @ 2024-10-25 0:52 UTC (permalink / raw)
To: Stefano Brivio; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 7409 bytes --]
On Fri, Oct 25, 2024 at 01:04:33AM +0200, Stefano Brivio wrote:
> We use fprintf() to print to standard output or standard error
> streams. If something gets truncated or there's an output error, we
> don't really want to try and report that, and at the same time it's
> not abnormal behaviour upon which we should terminate, either.
>
> Just silence the warning with an ugly FPRINTF() variadic macro casting
> the fprintf() expressions to void.
Again, not sure why I'm not seeing this with my clang version, but regardless:
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
> conf.c | 46 +++++++++++++++++++++++-----------------------
> log.c | 6 +++---
> util.h | 3 +++
> 3 files changed, 29 insertions(+), 26 deletions(-)
>
> diff --git a/conf.c b/conf.c
> index 2122600..9af15fe 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -731,19 +731,19 @@ static unsigned int conf_ip6(unsigned int ifi, struct ip6_ctx *ip6)
> static void usage(const char *name, FILE *f, int status)
> {
> if (strstr(name, "pasta")) {
> - fprintf(f, "Usage: %s [OPTION]... [COMMAND] [ARGS]...\n", name);
> - fprintf(f, " %s [OPTION]... PID\n", name);
> - fprintf(f, " %s [OPTION]... --netns [PATH|NAME]\n", name);
> - fprintf(f,
> + FPRINTF(f, "Usage: %s [OPTION]... [COMMAND] [ARGS]...\n", name);
> + FPRINTF(f, " %s [OPTION]... PID\n", name);
> + FPRINTF(f, " %s [OPTION]... --netns [PATH|NAME]\n", name);
> + FPRINTF(f,
> "\n"
> "Without PID or --netns, run the given command or a\n"
> "default shell in a new network and user namespace, and\n"
> "connect it via pasta.\n");
> } else {
> - fprintf(f, "Usage: %s [OPTION]...\n", name);
> + FPRINTF(f, "Usage: %s [OPTION]...\n", name);
> }
>
> - fprintf(f,
> + FPRINTF(f,
> "\n"
> " -d, --debug Be verbose\n"
> " --trace Be extra verbose, implies --debug\n"
> @@ -760,17 +760,17 @@ static void usage(const char *name, FILE *f, int status)
> " --version Show version and exit\n");
>
> if (strstr(name, "pasta")) {
> - fprintf(f,
> + FPRINTF(f,
> " -I, --ns-ifname NAME namespace interface name\n"
> " default: same interface name as external one\n");
> } else {
> - fprintf(f,
> + FPRINTF(f,
> " -s, --socket PATH UNIX domain socket path\n"
> " default: probe free path starting from "
> UNIX_SOCK_PATH "\n", 1);
> }
>
> - fprintf(f,
> + FPRINTF(f,
> " -F, --fd FD Use FD as pre-opened connected socket\n"
> " -p, --pcap FILE Log tap-facing traffic to pcap file\n"
> " -P, --pid FILE Write own PID to the given file\n"
> @@ -801,28 +801,28 @@ static void usage(const char *name, FILE *f, int status)
> " can be specified multiple times\n"
> " a single, empty option disables DNS information\n");
> if (strstr(name, "pasta"))
> - fprintf(f, " default: don't use any addresses\n");
> + FPRINTF(f, " default: don't use any addresses\n");
> else
> - fprintf(f, " default: use addresses from /etc/resolv.conf\n");
> - fprintf(f,
> + FPRINTF(f, " default: use addresses from /etc/resolv.conf\n");
> + FPRINTF(f,
> " -S, --search LIST Space-separated list, search domains\n"
> " a single, empty option disables the DNS search list\n");
> if (strstr(name, "pasta"))
> - fprintf(f, " default: don't use any search list\n");
> + FPRINTF(f, " default: don't use any search list\n");
> else
> - fprintf(f, " default: use search list from /etc/resolv.conf\n");
> + FPRINTF(f, " default: use search list from /etc/resolv.conf\n");
>
> if (strstr(name, "pasta"))
> - fprintf(f, " --dhcp-dns \tPass DNS list via DHCP/DHCPv6/NDP\n");
> + FPRINTF(f, " --dhcp-dns \tPass DNS list via DHCP/DHCPv6/NDP\n");
> else
> - fprintf(f, " --no-dhcp-dns No DNS list in DHCP/DHCPv6/NDP\n");
> + FPRINTF(f, " --no-dhcp-dns No DNS list in DHCP/DHCPv6/NDP\n");
>
> if (strstr(name, "pasta"))
> - fprintf(f, " --dhcp-search Pass list via DHCP/DHCPv6/NDP\n");
> + FPRINTF(f, " --dhcp-search Pass list via DHCP/DHCPv6/NDP\n");
> else
> - fprintf(f, " --no-dhcp-search No list in DHCP/DHCPv6/NDP\n");
> + FPRINTF(f, " --no-dhcp-search No list in DHCP/DHCPv6/NDP\n");
>
> - fprintf(f,
> + FPRINTF(f,
> " --map-host-loopback ADDR Translate ADDR to refer to host\n"
> " can be specified zero to two times (for IPv4 and IPv6)\n"
> " default: gateway address\n"
> @@ -850,7 +850,7 @@ static void usage(const char *name, FILE *f, int status)
> if (strstr(name, "pasta"))
> goto pasta_opts;
>
> - fprintf(f,
> + FPRINTF(f,
> " -1, --one-off Quit after handling one single client\n"
> " -t, --tcp-ports SPEC TCP port forwarding to guest\n"
> " can be specified multiple times\n"
> @@ -881,7 +881,7 @@ static void usage(const char *name, FILE *f, int status)
>
> pasta_opts:
>
> - fprintf(f,
> + FPRINTF(f,
> " -t, --tcp-ports SPEC TCP port forwarding to namespace\n"
> " can be specified multiple times\n"
> " SPEC can be:\n"
> @@ -1419,9 +1419,9 @@ void conf(struct ctx *c, int argc, char **argv)
>
> break;
> case 14:
> - fprintf(stdout,
> + FPRINTF(stdout,
> c->mode == MODE_PASTA ? "pasta " : "passt ");
> - fprintf(stdout, VERSION_BLOB);
> + FPRINTF(stdout, VERSION_BLOB);
> exit(EXIT_SUCCESS);
> case 15:
> ret = snprintf(c->ip4.ifname_out,
> diff --git a/log.c b/log.c
> index a61468e..6932885 100644
> --- a/log.c
> +++ b/log.c
> @@ -274,7 +274,7 @@ void vlogmsg(bool newline, bool cont, int pri, const char *format, va_list ap)
> char timestr[LOGTIME_STRLEN];
>
> logtime_fmt(timestr, sizeof(timestr), now);
> - fprintf(stderr, "%s: ", timestr);
> + FPRINTF(stderr, "%s: ", timestr);
> }
>
> if ((log_mask & LOG_MASK(LOG_PRI(pri))) || !log_conf_parsed) {
> @@ -293,7 +293,7 @@ void vlogmsg(bool newline, bool cont, int pri, const char *format, va_list ap)
> (log_stderr && (log_mask & LOG_MASK(LOG_PRI(pri))))) {
> (void)vfprintf(stderr, format, ap);
> if (newline && format[strlen(format)] != '\n')
> - fprintf(stderr, "\n");
> + FPRINTF(stderr, "\n");
> }
> }
>
> @@ -399,7 +399,7 @@ void passt_vsyslog(bool newline, int pri, const char *format, va_list ap)
> n += snprintf(buf + n, BUFSIZ - n, "\n");
>
> if (log_sock >= 0 && send(log_sock, buf, n, 0) != n && log_stderr)
> - fprintf(stderr, "Failed to send %i bytes to syslog\n", n);
> + FPRINTF(stderr, "Failed to send %i bytes to syslog\n", n);
> }
>
> /**
> diff --git a/util.h b/util.h
> index 8449d00..d614094 100644
> --- a/util.h
> +++ b/util.h
> @@ -270,6 +270,9 @@ static inline bool mod_between(unsigned x, unsigned i, unsigned j, unsigned m)
> return mod_sub(x, i, m) < mod_sub(j, i, m);
> }
>
> +/* FPRINTF() intentionally silences cert-err33-c clang-tidy warnings */
> +#define FPRINTF(f, ...) (void)fprintf(f, __VA_ARGS__)
> +
> /*
> * Workarounds for https://github.com/llvm/llvm-project/issues/58992
> *
--
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 --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/8] Makefile: Disable readability-math-missing-parentheses clang-tidy check
2024-10-24 23:04 [PATCH 0/8] Take care of clang-tidy warnings with LLVM >= 16 Stefano Brivio
` (2 preceding siblings ...)
2024-10-24 23:04 ` [PATCH 3/8] treewide: Silence cert-err33-c clang-tidy warnings for fprintf() Stefano Brivio
@ 2024-10-24 23:04 ` Stefano Brivio
2024-10-25 0:53 ` David Gibson
2024-10-24 23:04 ` [PATCH 5/8] treewide: Suppress clang-tidy warning if we already use O_CLOEXEC or if we can't Stefano Brivio
` (3 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Stefano Brivio @ 2024-10-24 23:04 UTC (permalink / raw)
To: passt-dev
With clang-tidy and LLVM 19:
/home/sbrivio/passt/conf.c:1218:29: error: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses,-warnings-as-errors]
1218 | const char *octet = str + 3 * i;
| ^~~~~~
| ( )
/home/sbrivio/passt/ndp.c:285:18: error: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses,-warnings-as-errors]
285 | .len = 1 + 2 * n,
| ^~~~~~
| ( )
/home/sbrivio/passt/ndp.c:329:23: error: '%' has higher precedence than '-'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses,-warnings-as-errors]
329 | memset(ptr, 0, 8 - dns_s_len % 8); /* padding */
| ^~~~~~~~~~~~~~
| ( )
/home/sbrivio/passt/pcap.c:131:20: error: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses,-warnings-as-errors]
131 | pcap_frame(iov + i * frame_parts, frame_parts, offset, &now);
| ^~~~~~~~~~~~~~~~
| ( )
/home/sbrivio/passt/util.c:216:10: error: '/' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses,-warnings-as-errors]
216 | return (a->tv_nsec + 1000000000 - b->tv_nsec) / 1000 +
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| ( )
/home/sbrivio/passt/util.c:217:10: error: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses,-warnings-as-errors]
217 | (a->tv_sec - b->tv_sec - 1) * 1000000;
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| ( )
/home/sbrivio/passt/util.c:220:9: error: '/' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses,-warnings-as-errors]
220 | return (a->tv_nsec - b->tv_nsec) / 1000 +
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| ( )
/home/sbrivio/passt/util.c:221:9: error: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses,-warnings-as-errors]
221 | (a->tv_sec - b->tv_sec) * 1000000;
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| ( )
/home/sbrivio/passt/util.c:545:32: error: '/' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses,-warnings-as-errors]
545 | return clone(fn, stack_area + stack_size / 2, flags, arg);
| ^~~~~~~~~~~~~~~
| ( )
Just... no.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
Makefile | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index 2ebc81e..9afbb74 100644
--- a/Makefile
+++ b/Makefile
@@ -270,6 +270,12 @@ docs: README.md
# makes sense when those defines form an enum-like set, but
# weird for cases like standalone constants, and causes other
# awkwardness for a bunch of cases we use
+#
+# - readability-math-missing-parentheses
+# It's been a couple of centuries since multiplication has been granted
+# precedence over addition in modern mathematical notation. Adding
+# parentheses to reinforce that certainly won't improve readability.
+
clang-tidy: $(filter-out qrap.c,$(SRCS)) $(HEADERS)
clang-tidy -checks=*,-modernize-*,\
@@ -296,7 +302,8 @@ clang-tidy: $(filter-out qrap.c,$(SRCS)) $(HEADERS)
-concurrency-mt-unsafe,\
-readability-identifier-length,\
-misc-include-cleaner,\
- -cppcoreguidelines-macro-to-enum \
+ -cppcoreguidelines-macro-to-enum,\
+ -readability-math-missing-parentheses \
-config='{CheckOptions: [{key: bugprone-suspicious-string-compare.WarnOnImplicitComparison, value: "false"}]}' \
--warnings-as-errors=* $(filter-out qrap.c,$(SRCS)) -- $(filter-out -pie,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) -DCLANG_TIDY_58992
--
@@ -270,6 +270,12 @@ docs: README.md
# makes sense when those defines form an enum-like set, but
# weird for cases like standalone constants, and causes other
# awkwardness for a bunch of cases we use
+#
+# - readability-math-missing-parentheses
+# It's been a couple of centuries since multiplication has been granted
+# precedence over addition in modern mathematical notation. Adding
+# parentheses to reinforce that certainly won't improve readability.
+
clang-tidy: $(filter-out qrap.c,$(SRCS)) $(HEADERS)
clang-tidy -checks=*,-modernize-*,\
@@ -296,7 +302,8 @@ clang-tidy: $(filter-out qrap.c,$(SRCS)) $(HEADERS)
-concurrency-mt-unsafe,\
-readability-identifier-length,\
-misc-include-cleaner,\
- -cppcoreguidelines-macro-to-enum \
+ -cppcoreguidelines-macro-to-enum,\
+ -readability-math-missing-parentheses \
-config='{CheckOptions: [{key: bugprone-suspicious-string-compare.WarnOnImplicitComparison, value: "false"}]}' \
--warnings-as-errors=* $(filter-out qrap.c,$(SRCS)) -- $(filter-out -pie,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) -DCLANG_TIDY_58992
--
2.43.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 4/8] Makefile: Disable readability-math-missing-parentheses clang-tidy check
2024-10-24 23:04 ` [PATCH 4/8] Makefile: Disable readability-math-missing-parentheses clang-tidy check Stefano Brivio
@ 2024-10-25 0:53 ` David Gibson
2024-10-25 7:53 ` Stefano Brivio
0 siblings, 1 reply; 20+ messages in thread
From: David Gibson @ 2024-10-25 0:53 UTC (permalink / raw)
To: Stefano Brivio; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 5582 bytes --]
On Fri, Oct 25, 2024 at 01:04:34AM +0200, Stefano Brivio wrote:
> With clang-tidy and LLVM 19:
Oh.. LLVM *19*, not *16*. That would explain why I'm not seeing the errors.
>
> /home/sbrivio/passt/conf.c:1218:29: error: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses,-warnings-as-errors]
> 1218 | const char *octet = str + 3 * i;
> | ^~~~~~
> | ( )
> /home/sbrivio/passt/ndp.c:285:18: error: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses,-warnings-as-errors]
> 285 | .len = 1 + 2 * n,
> | ^~~~~~
> | ( )
> /home/sbrivio/passt/ndp.c:329:23: error: '%' has higher precedence than '-'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses,-warnings-as-errors]
> 329 | memset(ptr, 0, 8 - dns_s_len % 8); /* padding */
> | ^~~~~~~~~~~~~~
> | ( )
> /home/sbrivio/passt/pcap.c:131:20: error: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses,-warnings-as-errors]
> 131 | pcap_frame(iov + i * frame_parts, frame_parts, offset, &now);
> | ^~~~~~~~~~~~~~~~
> | ( )
> /home/sbrivio/passt/util.c:216:10: error: '/' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses,-warnings-as-errors]
> 216 | return (a->tv_nsec + 1000000000 - b->tv_nsec) / 1000 +
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> | ( )
> /home/sbrivio/passt/util.c:217:10: error: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses,-warnings-as-errors]
> 217 | (a->tv_sec - b->tv_sec - 1) * 1000000;
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> | ( )
> /home/sbrivio/passt/util.c:220:9: error: '/' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses,-warnings-as-errors]
> 220 | return (a->tv_nsec - b->tv_nsec) / 1000 +
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> | ( )
> /home/sbrivio/passt/util.c:221:9: error: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses,-warnings-as-errors]
> 221 | (a->tv_sec - b->tv_sec) * 1000000;
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> | ( )
> /home/sbrivio/passt/util.c:545:32: error: '/' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses,-warnings-as-errors]
> 545 | return clone(fn, stack_area + stack_size / 2, flags, arg);
> | ^~~~~~~~~~~~~~~
> | ( )
>
> Just... no.
I quite agree.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
> Makefile | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index 2ebc81e..9afbb74 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -270,6 +270,12 @@ docs: README.md
> # makes sense when those defines form an enum-like set, but
> # weird for cases like standalone constants, and causes other
> # awkwardness for a bunch of cases we use
> +#
> +# - readability-math-missing-parentheses
> +# It's been a couple of centuries since multiplication has been granted
> +# precedence over addition in modern mathematical notation. Adding
> +# parentheses to reinforce that certainly won't improve readability.
> +
>
> clang-tidy: $(filter-out qrap.c,$(SRCS)) $(HEADERS)
> clang-tidy -checks=*,-modernize-*,\
> @@ -296,7 +302,8 @@ clang-tidy: $(filter-out qrap.c,$(SRCS)) $(HEADERS)
> -concurrency-mt-unsafe,\
> -readability-identifier-length,\
> -misc-include-cleaner,\
> - -cppcoreguidelines-macro-to-enum \
> + -cppcoreguidelines-macro-to-enum,\
> + -readability-math-missing-parentheses \
> -config='{CheckOptions: [{key: bugprone-suspicious-string-compare.WarnOnImplicitComparison, value: "false"}]}' \
> --warnings-as-errors=* $(filter-out qrap.c,$(SRCS)) -- $(filter-out -pie,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) -DCLANG_TIDY_58992
>
--
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 --]
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 4/8] Makefile: Disable readability-math-missing-parentheses clang-tidy check
2024-10-25 0:53 ` David Gibson
@ 2024-10-25 7:53 ` Stefano Brivio
0 siblings, 0 replies; 20+ messages in thread
From: Stefano Brivio @ 2024-10-25 7:53 UTC (permalink / raw)
To: David Gibson; +Cc: passt-dev
On Fri, 25 Oct 2024 11:53:41 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Fri, Oct 25, 2024 at 01:04:34AM +0200, Stefano Brivio wrote:
> > With clang-tidy and LLVM 19:
>
> Oh.. LLVM *19*, not *16*. That would explain why I'm not seeing the errors.
These only show up with LLVM 19, but most of the other ones also appear
with LLVM 16.
--
Stefano
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 5/8] treewide: Suppress clang-tidy warning if we already use O_CLOEXEC or if we can't
2024-10-24 23:04 [PATCH 0/8] Take care of clang-tidy warnings with LLVM >= 16 Stefano Brivio
` (3 preceding siblings ...)
2024-10-24 23:04 ` [PATCH 4/8] Makefile: Disable readability-math-missing-parentheses clang-tidy check Stefano Brivio
@ 2024-10-24 23:04 ` Stefano Brivio
2024-10-24 23:04 ` [PATCH 6/8] treewide: Address cert-err33-c clang-tidy warnings for clock and timer functions Stefano Brivio
` (2 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Stefano Brivio @ 2024-10-24 23:04 UTC (permalink / raw)
To: passt-dev
In pcap_init(), we open the packet capture file with O_CLOEXEC only
when possible.
In logfile_init() and pidfile_open(), the fact that we pass a third
'mode' argument to open() seems to confuse the android-cloexec-open
checker in LLVM versions from 16 to 19 (at least).
The checker is suggesting to add O_CLOEXEC to 'mode', and not in
'flags', where we already have it.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
log.c | 4 ++++
pcap.c | 1 +
util.c | 4 ++++
3 files changed, 9 insertions(+)
diff --git a/log.c b/log.c
index 6932885..154466f 100644
--- a/log.c
+++ b/log.c
@@ -416,7 +416,11 @@ void logfile_init(const char *name, const char *path, size_t size)
if (readlink("/proc/self/exe", exe, PATH_MAX - 1) < 0)
die_perror("Failed to read own /proc/self/exe link");
+ /* We use O_CLOEXEC here, but clang-tidy as of LLVM 16 to 19 looks for
+ * it in the 'mode' argument if we have one, so...
+ */
log_file = open(path, O_CREAT | O_TRUNC | O_APPEND | O_RDWR | O_CLOEXEC,
+ /* NOLINTNEXTLINE(android-cloexec-open) */
S_IRUSR | S_IWUSR);
if (log_file == -1)
die_perror("Couldn't open log file %s", path);
diff --git a/pcap.c b/pcap.c
index 6ee6cdf..6753cfb 100644
--- a/pcap.c
+++ b/pcap.c
@@ -167,6 +167,7 @@ void pcap_init(struct ctx *c)
return;
flags |= c->foreground ? O_CLOEXEC : 0;
+ /* NOLINTNEXTLINE(android-cloexec-open): ...only where possible */
pcap_fd = open(c->pcap, flags, S_IRUSR | S_IWUSR);
if (pcap_fd == -1) {
perror("open");
diff --git a/util.c b/util.c
index eff6787..b719f9a 100644
--- a/util.c
+++ b/util.c
@@ -419,7 +419,11 @@ int pidfile_open(const char *path)
if (!*path)
return -1;
+ /* We use O_CLOEXEC here, but clang-tidy as of LLVM 16 to 19 looks for
+ * it in the 'mode' argument if we have one
+ */
if ((fd = open(path, O_CREAT | O_TRUNC | O_WRONLY | O_CLOEXEC,
+ /* NOLINTNEXTLINE(android-cloexec-open) */
S_IRUSR | S_IWUSR)) < 0) {
perror("PID file open");
exit(EXIT_FAILURE);
--
@@ -419,7 +419,11 @@ int pidfile_open(const char *path)
if (!*path)
return -1;
+ /* We use O_CLOEXEC here, but clang-tidy as of LLVM 16 to 19 looks for
+ * it in the 'mode' argument if we have one
+ */
if ((fd = open(path, O_CREAT | O_TRUNC | O_WRONLY | O_CLOEXEC,
+ /* NOLINTNEXTLINE(android-cloexec-open) */
S_IRUSR | S_IWUSR)) < 0) {
perror("PID file open");
exit(EXIT_FAILURE);
--
2.43.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 6/8] treewide: Address cert-err33-c clang-tidy warnings for clock and timer functions
2024-10-24 23:04 [PATCH 0/8] Take care of clang-tidy warnings with LLVM >= 16 Stefano Brivio
` (4 preceding siblings ...)
2024-10-24 23:04 ` [PATCH 5/8] treewide: Suppress clang-tidy warning if we already use O_CLOEXEC or if we can't Stefano Brivio
@ 2024-10-24 23:04 ` Stefano Brivio
2024-10-25 1:00 ` David Gibson
2024-10-24 23:04 ` [PATCH 7/8] udp: Take care of cert-int09-c clang-tidy warning for enum udp_iov_idx Stefano Brivio
2024-10-24 23:04 ` [PATCH 8/8] util: Don't use errno after a successful call in __daemon() Stefano Brivio
7 siblings, 1 reply; 20+ messages in thread
From: Stefano Brivio @ 2024-10-24 23:04 UTC (permalink / raw)
To: passt-dev
For clock_gettime(), we shouldn't ignore errors if they happen at
initialisation phase, because something is seriously wrong and it's
not helpful if we proceed as if nothing happened.
As we're up and running, though, it's probably better to use a stale
value than to terminate altogether. Make sure we use a zero value if
we don't have a stale one somewhere.
For timerfd_gettime() and timerfd_settime() failures, report an error,
there isn't much else we can do.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
passt.c | 8 +++++---
pcap.c | 12 ++++++------
tcp.c | 12 +++++++++---
3 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/passt.c b/passt.c
index ad6f0bc..e987f0d 100644
--- a/passt.c
+++ b/passt.c
@@ -207,7 +207,8 @@ int main(int argc, char **argv)
struct timespec now;
struct sigaction sa;
- clock_gettime(CLOCK_MONOTONIC, &log_start);
+ if (clock_gettime(CLOCK_MONOTONIC, &log_start))
+ die_perror("Failed to get CLOCK_MONOTONIC time");
arch_avx2_exec(argv);
@@ -265,7 +266,8 @@ int main(int argc, char **argv)
secret_init(&c);
- clock_gettime(CLOCK_MONOTONIC, &now);
+ if (clock_gettime(CLOCK_MONOTONIC, &now))
+ die_perror("Failed to get CLOCK_MONOTONIC time");
flow_init();
@@ -313,7 +315,7 @@ loop:
if (nfds == -1 && errno != EINTR)
die_perror("epoll_wait() failed in main loop");
- clock_gettime(CLOCK_MONOTONIC, &now);
+ (void)clock_gettime(CLOCK_MONOTONIC, &now);
for (i = 0; i < nfds; i++) {
union epoll_ref ref = *((union epoll_ref *)&events[i].data.u64);
diff --git a/pcap.c b/pcap.c
index 6753cfb..156c953 100644
--- a/pcap.c
+++ b/pcap.c
@@ -100,12 +100,12 @@ static void pcap_frame(const struct iovec *iov, size_t iovcnt,
void pcap(const char *pkt, size_t l2len)
{
struct iovec iov = { (char *)pkt, l2len };
- struct timespec now;
+ struct timespec now = { 0 };
if (pcap_fd == -1)
return;
- clock_gettime(CLOCK_REALTIME, &now);
+ (void)clock_gettime(CLOCK_REALTIME, &now);
pcap_frame(&iov, 1, 0, &now);
}
@@ -119,13 +119,13 @@ void pcap(const char *pkt, size_t l2len)
void pcap_multiple(const struct iovec *iov, size_t frame_parts, unsigned int n,
size_t offset)
{
- struct timespec now;
+ struct timespec now = { 0 };
unsigned int i;
if (pcap_fd == -1)
return;
- clock_gettime(CLOCK_REALTIME, &now);
+ (void)clock_gettime(CLOCK_REALTIME, &now);
for (i = 0; i < n; i++)
pcap_frame(iov + i * frame_parts, frame_parts, offset, &now);
@@ -143,12 +143,12 @@ void pcap_multiple(const struct iovec *iov, size_t frame_parts, unsigned int n,
/* cppcheck-suppress unusedFunction */
void pcap_iov(const struct iovec *iov, size_t iovcnt, size_t offset)
{
- struct timespec now;
+ struct timespec now = { 0 };
if (pcap_fd == -1)
return;
- clock_gettime(CLOCK_REALTIME, &now);
+ (void)clock_gettime(CLOCK_REALTIME, &now);
pcap_frame(iov, iovcnt, offset, &now);
}
diff --git a/tcp.c b/tcp.c
index 0d22e07..d55caee 100644
--- a/tcp.c
+++ b/tcp.c
@@ -548,7 +548,8 @@ static void tcp_timer_ctl(const struct ctx *c, struct tcp_tap_conn *conn)
(unsigned long long)it.it_value.tv_sec,
(unsigned long long)it.it_value.tv_nsec / 1000 / 1000);
- timerfd_settime(conn->timer, 0, &it, NULL);
+ if (timerfd_settime(conn->timer, 0, &it, NULL))
+ flow_err(conn, "failed to set timer: %s", strerror(errno));
}
/**
@@ -2240,7 +2241,9 @@ void tcp_timer_handler(const struct ctx *c, union epoll_ref ref)
* timer is currently armed, this event came from a previous setting,
* and we just set the timer to a new point in the future: discard it.
*/
- timerfd_gettime(conn->timer, &check_armed);
+ if (timerfd_gettime(conn->timer, &check_armed))
+ flow_err(conn, "failed to read timer: %s", strerror(errno));
+
if (check_armed.it_value.tv_sec || check_armed.it_value.tv_nsec)
return;
@@ -2278,7 +2281,10 @@ void tcp_timer_handler(const struct ctx *c, union epoll_ref ref)
* case. This avoids having to preemptively reset the timer on
* ~ACK_TO_TAP_DUE or ~ACK_FROM_TAP_DUE.
*/
- timerfd_settime(conn->timer, 0, &new, &old);
+ if (timerfd_settime(conn->timer, 0, &new, &old))
+ flow_err(conn, "failed to set timer: %s",
+ strerror(errno));
+
if (old.it_value.tv_sec == ACT_TIMEOUT) {
flow_dbg(conn, "activity timeout");
tcp_rst(c, conn);
--
@@ -548,7 +548,8 @@ static void tcp_timer_ctl(const struct ctx *c, struct tcp_tap_conn *conn)
(unsigned long long)it.it_value.tv_sec,
(unsigned long long)it.it_value.tv_nsec / 1000 / 1000);
- timerfd_settime(conn->timer, 0, &it, NULL);
+ if (timerfd_settime(conn->timer, 0, &it, NULL))
+ flow_err(conn, "failed to set timer: %s", strerror(errno));
}
/**
@@ -2240,7 +2241,9 @@ void tcp_timer_handler(const struct ctx *c, union epoll_ref ref)
* timer is currently armed, this event came from a previous setting,
* and we just set the timer to a new point in the future: discard it.
*/
- timerfd_gettime(conn->timer, &check_armed);
+ if (timerfd_gettime(conn->timer, &check_armed))
+ flow_err(conn, "failed to read timer: %s", strerror(errno));
+
if (check_armed.it_value.tv_sec || check_armed.it_value.tv_nsec)
return;
@@ -2278,7 +2281,10 @@ void tcp_timer_handler(const struct ctx *c, union epoll_ref ref)
* case. This avoids having to preemptively reset the timer on
* ~ACK_TO_TAP_DUE or ~ACK_FROM_TAP_DUE.
*/
- timerfd_settime(conn->timer, 0, &new, &old);
+ if (timerfd_settime(conn->timer, 0, &new, &old))
+ flow_err(conn, "failed to set timer: %s",
+ strerror(errno));
+
if (old.it_value.tv_sec == ACT_TIMEOUT) {
flow_dbg(conn, "activity timeout");
tcp_rst(c, conn);
--
2.43.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 6/8] treewide: Address cert-err33-c clang-tidy warnings for clock and timer functions
2024-10-24 23:04 ` [PATCH 6/8] treewide: Address cert-err33-c clang-tidy warnings for clock and timer functions Stefano Brivio
@ 2024-10-25 1:00 ` David Gibson
2024-10-25 7:53 ` Stefano Brivio
0 siblings, 1 reply; 20+ messages in thread
From: David Gibson @ 2024-10-25 1:00 UTC (permalink / raw)
To: Stefano Brivio; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 5084 bytes --]
On Fri, Oct 25, 2024 at 01:04:36AM +0200, Stefano Brivio wrote:
> For clock_gettime(), we shouldn't ignore errors if they happen at
> initialisation phase, because something is seriously wrong and it's
> not helpful if we proceed as if nothing happened.
>
> As we're up and running, though, it's probably better to use a stale
> value than to terminate altogether. Make sure we use a zero value if
> we don't have a stale one somewhere.
>
> For timerfd_gettime() and timerfd_settime() failures, report an error,
> there isn't much else we can do.
>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
> passt.c | 8 +++++---
> pcap.c | 12 ++++++------
> tcp.c | 12 +++++++++---
> 3 files changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/passt.c b/passt.c
> index ad6f0bc..e987f0d 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -207,7 +207,8 @@ int main(int argc, char **argv)
> struct timespec now;
> struct sigaction sa;
>
> - clock_gettime(CLOCK_MONOTONIC, &log_start);
> + if (clock_gettime(CLOCK_MONOTONIC, &log_start))
> + die_perror("Failed to get CLOCK_MONOTONIC time");
>
> arch_avx2_exec(argv);
>
> @@ -265,7 +266,8 @@ int main(int argc, char **argv)
>
> secret_init(&c);
>
> - clock_gettime(CLOCK_MONOTONIC, &now);
> + if (clock_gettime(CLOCK_MONOTONIC, &now))
> + die_perror("Failed to get CLOCK_MONOTONIC time");
>
> flow_init();
>
> @@ -313,7 +315,7 @@ loop:
> if (nfds == -1 && errno != EINTR)
> die_perror("epoll_wait() failed in main loop");
>
> - clock_gettime(CLOCK_MONOTONIC, &now);
> + (void)clock_gettime(CLOCK_MONOTONIC, &now);
I think we should err() here.
>
> for (i = 0; i < nfds; i++) {
> union epoll_ref ref = *((union epoll_ref *)&events[i].data.u64);
> diff --git a/pcap.c b/pcap.c
> index 6753cfb..156c953 100644
> --- a/pcap.c
> +++ b/pcap.c
> @@ -100,12 +100,12 @@ static void pcap_frame(const struct iovec *iov, size_t iovcnt,
> void pcap(const char *pkt, size_t l2len)
> {
> struct iovec iov = { (char *)pkt, l2len };
> - struct timespec now;
> + struct timespec now = { 0 };
>
> if (pcap_fd == -1)
> return;
>
> - clock_gettime(CLOCK_REALTIME, &now);
> + (void)clock_gettime(CLOCK_REALTIME, &now);
..and here..
> pcap_frame(&iov, 1, 0, &now);
> }
>
> @@ -119,13 +119,13 @@ void pcap(const char *pkt, size_t l2len)
> void pcap_multiple(const struct iovec *iov, size_t frame_parts, unsigned int n,
> size_t offset)
> {
> - struct timespec now;
> + struct timespec now = { 0 };
> unsigned int i;
>
> if (pcap_fd == -1)
> return;
>
> - clock_gettime(CLOCK_REALTIME, &now);
> + (void)clock_gettime(CLOCK_REALTIME, &now);
..and here..
> for (i = 0; i < n; i++)
> pcap_frame(iov + i * frame_parts, frame_parts, offset, &now);
> @@ -143,12 +143,12 @@ void pcap_multiple(const struct iovec *iov, size_t frame_parts, unsigned int n,
> /* cppcheck-suppress unusedFunction */
> void pcap_iov(const struct iovec *iov, size_t iovcnt, size_t offset)
> {
> - struct timespec now;
> + struct timespec now = { 0 };
>
> if (pcap_fd == -1)
> return;
>
> - clock_gettime(CLOCK_REALTIME, &now);
> + (void)clock_gettime(CLOCK_REALTIME, &now);
..and here..
> pcap_frame(iov, iovcnt, offset, &now);
> }
>
> diff --git a/tcp.c b/tcp.c
> index 0d22e07..d55caee 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -548,7 +548,8 @@ static void tcp_timer_ctl(const struct ctx *c, struct tcp_tap_conn *conn)
> (unsigned long long)it.it_value.tv_sec,
> (unsigned long long)it.it_value.tv_nsec / 1000 / 1000);
>
> - timerfd_settime(conn->timer, 0, &it, NULL);
> + if (timerfd_settime(conn->timer, 0, &it, NULL))
> + flow_err(conn, "failed to set timer: %s", strerror(errno));
> }
>
> /**
> @@ -2240,7 +2241,9 @@ void tcp_timer_handler(const struct ctx *c, union epoll_ref ref)
> * timer is currently armed, this event came from a previous setting,
> * and we just set the timer to a new point in the future: discard it.
> */
> - timerfd_gettime(conn->timer, &check_armed);
> + if (timerfd_gettime(conn->timer, &check_armed))
> + flow_err(conn, "failed to read timer: %s", strerror(errno));
> +
> if (check_armed.it_value.tv_sec || check_armed.it_value.tv_nsec)
> return;
>
> @@ -2278,7 +2281,10 @@ void tcp_timer_handler(const struct ctx *c, union epoll_ref ref)
> * case. This avoids having to preemptively reset the timer on
> * ~ACK_TO_TAP_DUE or ~ACK_FROM_TAP_DUE.
> */
> - timerfd_settime(conn->timer, 0, &new, &old);
> + if (timerfd_settime(conn->timer, 0, &new, &old))
> + flow_err(conn, "failed to set timer: %s",
> + strerror(errno));
> +
> if (old.it_value.tv_sec == ACT_TIMEOUT) {
> flow_dbg(conn, "activity timeout");
> tcp_rst(c, conn);
--
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 --]
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 6/8] treewide: Address cert-err33-c clang-tidy warnings for clock and timer functions
2024-10-25 1:00 ` David Gibson
@ 2024-10-25 7:53 ` Stefano Brivio
0 siblings, 0 replies; 20+ messages in thread
From: Stefano Brivio @ 2024-10-25 7:53 UTC (permalink / raw)
To: David Gibson; +Cc: passt-dev
On Fri, 25 Oct 2024 12:00:26 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Fri, Oct 25, 2024 at 01:04:36AM +0200, Stefano Brivio wrote:
> > For clock_gettime(), we shouldn't ignore errors if they happen at
> > initialisation phase, because something is seriously wrong and it's
> > not helpful if we proceed as if nothing happened.
> >
> > As we're up and running, though, it's probably better to use a stale
> > value than to terminate altogether. Make sure we use a zero value if
> > we don't have a stale one somewhere.
> >
> > For timerfd_gettime() and timerfd_settime() failures, report an error,
> > there isn't much else we can do.
> >
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > ---
> > passt.c | 8 +++++---
> > pcap.c | 12 ++++++------
> > tcp.c | 12 +++++++++---
> > 3 files changed, 20 insertions(+), 12 deletions(-)
> >
> > diff --git a/passt.c b/passt.c
> > index ad6f0bc..e987f0d 100644
> > --- a/passt.c
> > +++ b/passt.c
> > @@ -207,7 +207,8 @@ int main(int argc, char **argv)
> > struct timespec now;
> > struct sigaction sa;
> >
> > - clock_gettime(CLOCK_MONOTONIC, &log_start);
> > + if (clock_gettime(CLOCK_MONOTONIC, &log_start))
> > + die_perror("Failed to get CLOCK_MONOTONIC time");
> >
> > arch_avx2_exec(argv);
> >
> > @@ -265,7 +266,8 @@ int main(int argc, char **argv)
> >
> > secret_init(&c);
> >
> > - clock_gettime(CLOCK_MONOTONIC, &now);
> > + if (clock_gettime(CLOCK_MONOTONIC, &now))
> > + die_perror("Failed to get CLOCK_MONOTONIC time");
> >
> > flow_init();
> >
> > @@ -313,7 +315,7 @@ loop:
> > if (nfds == -1 && errno != EINTR)
> > die_perror("epoll_wait() failed in main loop");
> >
> > - clock_gettime(CLOCK_MONOTONIC, &now);
> > + (void)clock_gettime(CLOCK_MONOTONIC, &now);
>
> I think we should err() here.
Oops, right, added here and below.
--
Stefano
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 7/8] udp: Take care of cert-int09-c clang-tidy warning for enum udp_iov_idx
2024-10-24 23:04 [PATCH 0/8] Take care of clang-tidy warnings with LLVM >= 16 Stefano Brivio
` (5 preceding siblings ...)
2024-10-24 23:04 ` [PATCH 6/8] treewide: Address cert-err33-c clang-tidy warnings for clock and timer functions Stefano Brivio
@ 2024-10-24 23:04 ` Stefano Brivio
2024-10-25 1:02 ` David Gibson
2024-10-24 23:04 ` [PATCH 8/8] util: Don't use errno after a successful call in __daemon() Stefano Brivio
7 siblings, 1 reply; 20+ messages in thread
From: Stefano Brivio @ 2024-10-24 23:04 UTC (permalink / raw)
To: passt-dev
/home/sbrivio/passt/udp.c:171:1: error: inital values in enum 'udp_iov_idx' are not consistent, consider explicit initialization of all, none or only the first enumerator [cert-int09-c,readability-enum-initial-value,-warnings-as-errors]
171 | enum udp_iov_idx {
| ^
172 | UDP_IOV_TAP = 0,
173 | UDP_IOV_ETH = 1,
174 | UDP_IOV_IP = 2,
175 | UDP_IOV_PAYLOAD = 3,
176 | UDP_NUM_IOVS
|
| = 4
Make sure we initialise all the values, in this case.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
udp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/udp.c b/udp.c
index 100610f..89f2959 100644
--- a/udp.c
+++ b/udp.c
@@ -173,7 +173,7 @@ enum udp_iov_idx {
UDP_IOV_ETH = 1,
UDP_IOV_IP = 2,
UDP_IOV_PAYLOAD = 3,
- UDP_NUM_IOVS
+ UDP_NUM_IOVS = UDP_IOV_PAYLOAD + 1,
};
/* IOVs and msghdr arrays for receiving datagrams from sockets */
--
@@ -173,7 +173,7 @@ enum udp_iov_idx {
UDP_IOV_ETH = 1,
UDP_IOV_IP = 2,
UDP_IOV_PAYLOAD = 3,
- UDP_NUM_IOVS
+ UDP_NUM_IOVS = UDP_IOV_PAYLOAD + 1,
};
/* IOVs and msghdr arrays for receiving datagrams from sockets */
--
2.43.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 7/8] udp: Take care of cert-int09-c clang-tidy warning for enum udp_iov_idx
2024-10-24 23:04 ` [PATCH 7/8] udp: Take care of cert-int09-c clang-tidy warning for enum udp_iov_idx Stefano Brivio
@ 2024-10-25 1:02 ` David Gibson
2024-10-25 7:53 ` Stefano Brivio
0 siblings, 1 reply; 20+ messages in thread
From: David Gibson @ 2024-10-25 1:02 UTC (permalink / raw)
To: Stefano Brivio; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 1595 bytes --]
On Fri, Oct 25, 2024 at 01:04:37AM +0200, Stefano Brivio wrote:
> /home/sbrivio/passt/udp.c:171:1: error: inital values in enum 'udp_iov_idx' are not consistent, consider explicit initialization of all, none or only the first enumerator [cert-int09-c,readability-enum-initial-value,-warnings-as-errors]
> 171 | enum udp_iov_idx {
> | ^
> 172 | UDP_IOV_TAP = 0,
> 173 | UDP_IOV_ETH = 1,
> 174 | UDP_IOV_IP = 2,
> 175 | UDP_IOV_PAYLOAD = 3,
> 176 | UDP_NUM_IOVS
> |
> | = 4
>
> Make sure we initialise all the values, in this case.
Oof. Although it's a bit weird, I quite like the existing style,
because it gives the correct value for NUM_WHATEVER without requiring
editing if a new explicit value is inserted above it.
I'd be inclined to use a suppression rather than changing the code.
>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
> udp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/udp.c b/udp.c
> index 100610f..89f2959 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -173,7 +173,7 @@ enum udp_iov_idx {
> UDP_IOV_ETH = 1,
> UDP_IOV_IP = 2,
> UDP_IOV_PAYLOAD = 3,
> - UDP_NUM_IOVS
> + UDP_NUM_IOVS = UDP_IOV_PAYLOAD + 1,
> };
>
> /* IOVs and msghdr arrays for receiving datagrams from sockets */
--
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 --]
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 7/8] udp: Take care of cert-int09-c clang-tidy warning for enum udp_iov_idx
2024-10-25 1:02 ` David Gibson
@ 2024-10-25 7:53 ` Stefano Brivio
0 siblings, 0 replies; 20+ messages in thread
From: Stefano Brivio @ 2024-10-25 7:53 UTC (permalink / raw)
To: David Gibson; +Cc: passt-dev
On Fri, 25 Oct 2024 12:02:17 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Fri, Oct 25, 2024 at 01:04:37AM +0200, Stefano Brivio wrote:
> > /home/sbrivio/passt/udp.c:171:1: error: inital values in enum 'udp_iov_idx' are not consistent, consider explicit initialization of all, none or only the first enumerator [cert-int09-c,readability-enum-initial-value,-warnings-as-errors]
> > 171 | enum udp_iov_idx {
> > | ^
> > 172 | UDP_IOV_TAP = 0,
> > 173 | UDP_IOV_ETH = 1,
> > 174 | UDP_IOV_IP = 2,
> > 175 | UDP_IOV_PAYLOAD = 3,
> > 176 | UDP_NUM_IOVS
> > |
> > | = 4
> >
> > Make sure we initialise all the values, in this case.
>
> Oof. Although it's a bit weird, I quite like the existing style,
> because it gives the correct value for NUM_WHATEVER without requiring
> editing if a new explicit value is inserted above it.
>
> I'd be inclined to use a suppression rather than changing the code.
I see your point, but still it would be practical to just comply with
INT09-C:
https://wiki.sei.cmu.edu/confluence/display/c/INT09-C.+Ensure+enumeration+constants+map+to+unique+values
...avoiding integer assignments altogether would do the trick as well.
Perhaps I should go with that instead.
--
Stefano
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 8/8] util: Don't use errno after a successful call in __daemon()
2024-10-24 23:04 [PATCH 0/8] Take care of clang-tidy warnings with LLVM >= 16 Stefano Brivio
` (6 preceding siblings ...)
2024-10-24 23:04 ` [PATCH 7/8] udp: Take care of cert-int09-c clang-tidy warning for enum udp_iov_idx Stefano Brivio
@ 2024-10-24 23:04 ` Stefano Brivio
2024-10-25 1:04 ` David Gibson
7 siblings, 1 reply; 20+ messages in thread
From: Stefano Brivio @ 2024-10-24 23:04 UTC (permalink / raw)
To: passt-dev
I thought we could just set errno to 0, do a bunch of stuff, and check
that errno didn't change to infer we succeeded. But clang-tidy,
starting with LLVM 19, reports:
/home/sbrivio/passt/util.c:465:6: error: An undefined value may be read from 'errno' [clang-analyzer-unix.Errno,-warnings-as-errors]
465 | if (errno)
| ^
/usr/include/errno.h:38:16: note: expanded from macro 'errno'
38 | # define errno (*__errno_location ())
| ^~~~~~~~~~~~~~~~~~~~~~
/home/sbrivio/passt/util.c:446:6: note: Assuming the condition is false
446 | if (pid == -1) {
| ^~~~~~~~~
/home/sbrivio/passt/util.c:446:2: note: Taking false branch
446 | if (pid == -1) {
| ^
/home/sbrivio/passt/util.c:451:6: note: Assuming 'pid' is 0
451 | if (pid) {
| ^~~
/home/sbrivio/passt/util.c:451:2: note: Taking false branch
451 | if (pid) {
| ^
/home/sbrivio/passt/util.c:463:2: note: Assuming that 'close' is successful; 'errno' becomes undefined after the call
463 | close(devnull_fd);
| ^~~~~~~~~~~~~~~~~
/home/sbrivio/passt/util.c:465:6: note: An undefined value may be read from 'errno'
465 | if (errno)
| ^
/usr/include/errno.h:38:16: note: expanded from macro 'errno'
38 | # define errno (*__errno_location ())
| ^~~~~~~~~~~~~~~~~~~~~~
And the LLVM documentation for the unix.Errno checker, 1.1.8.3
unix.Errno (C), mentions, at:
https://clang.llvm.org/docs/analyzer/checkers.html#unix-errno
that:
The C and POSIX standards often do not define if a standard library
function may change value of errno if the call does not fail.
Therefore, errno should only be used if it is known from the return
value of a function that the call has failed.
which is, somewhat surprisingly, the case for close().
Instead of using errno, check the actual return values of the calls
we issue here.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
util.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/util.c b/util.c
index b719f9a..02e18fb 100644
--- a/util.c
+++ b/util.c
@@ -453,16 +453,11 @@ int __daemon(int pidfile_fd, int devnull_fd)
exit(EXIT_SUCCESS);
}
- errno = 0;
-
- setsid();
-
- dup2(devnull_fd, STDIN_FILENO);
- dup2(devnull_fd, STDOUT_FILENO);
- dup2(devnull_fd, STDERR_FILENO);
- close(devnull_fd);
-
- if (errno)
+ if (setsid() < 0 ||
+ dup2(devnull_fd, STDIN_FILENO) < 0 ||
+ dup2(devnull_fd, STDOUT_FILENO) < 0 ||
+ dup2(devnull_fd, STDERR_FILENO) < 0 ||
+ close(devnull_fd))
exit(EXIT_FAILURE);
return 0;
--
@@ -453,16 +453,11 @@ int __daemon(int pidfile_fd, int devnull_fd)
exit(EXIT_SUCCESS);
}
- errno = 0;
-
- setsid();
-
- dup2(devnull_fd, STDIN_FILENO);
- dup2(devnull_fd, STDOUT_FILENO);
- dup2(devnull_fd, STDERR_FILENO);
- close(devnull_fd);
-
- if (errno)
+ if (setsid() < 0 ||
+ dup2(devnull_fd, STDIN_FILENO) < 0 ||
+ dup2(devnull_fd, STDOUT_FILENO) < 0 ||
+ dup2(devnull_fd, STDERR_FILENO) < 0 ||
+ close(devnull_fd))
exit(EXIT_FAILURE);
return 0;
--
2.43.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 8/8] util: Don't use errno after a successful call in __daemon()
2024-10-24 23:04 ` [PATCH 8/8] util: Don't use errno after a successful call in __daemon() Stefano Brivio
@ 2024-10-25 1:04 ` David Gibson
0 siblings, 0 replies; 20+ messages in thread
From: David Gibson @ 2024-10-25 1:04 UTC (permalink / raw)
To: Stefano Brivio; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 3281 bytes --]
On Fri, Oct 25, 2024 at 01:04:38AM +0200, Stefano Brivio wrote:
> I thought we could just set errno to 0, do a bunch of stuff, and check
> that errno didn't change to infer we succeeded. But clang-tidy,
> starting with LLVM 19, reports:
>
> /home/sbrivio/passt/util.c:465:6: error: An undefined value may be read from 'errno' [clang-analyzer-unix.Errno,-warnings-as-errors]
> 465 | if (errno)
> | ^
> /usr/include/errno.h:38:16: note: expanded from macro 'errno'
> 38 | # define errno (*__errno_location ())
> | ^~~~~~~~~~~~~~~~~~~~~~
> /home/sbrivio/passt/util.c:446:6: note: Assuming the condition is false
> 446 | if (pid == -1) {
> | ^~~~~~~~~
> /home/sbrivio/passt/util.c:446:2: note: Taking false branch
> 446 | if (pid == -1) {
> | ^
> /home/sbrivio/passt/util.c:451:6: note: Assuming 'pid' is 0
> 451 | if (pid) {
> | ^~~
> /home/sbrivio/passt/util.c:451:2: note: Taking false branch
> 451 | if (pid) {
> | ^
> /home/sbrivio/passt/util.c:463:2: note: Assuming that 'close' is successful; 'errno' becomes undefined after the call
> 463 | close(devnull_fd);
> | ^~~~~~~~~~~~~~~~~
> /home/sbrivio/passt/util.c:465:6: note: An undefined value may be read from 'errno'
> 465 | if (errno)
> | ^
> /usr/include/errno.h:38:16: note: expanded from macro 'errno'
> 38 | # define errno (*__errno_location ())
> | ^~~~~~~~~~~~~~~~~~~~~~
>
> And the LLVM documentation for the unix.Errno checker, 1.1.8.3
> unix.Errno (C), mentions, at:
>
> https://clang.llvm.org/docs/analyzer/checkers.html#unix-errno
>
> that:
>
> The C and POSIX standards often do not define if a standard library
> function may change value of errno if the call does not fail.
> Therefore, errno should only be used if it is known from the return
> value of a function that the call has failed.
>
> which is, somewhat surprisingly, the case for close().
Ah, yeah.
> Instead of using errno, check the actual return values of the calls
> we issue here.
>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> util.c | 15 +++++----------
> 1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/util.c b/util.c
> index b719f9a..02e18fb 100644
> --- a/util.c
> +++ b/util.c
> @@ -453,16 +453,11 @@ int __daemon(int pidfile_fd, int devnull_fd)
> exit(EXIT_SUCCESS);
> }
>
> - errno = 0;
> -
> - setsid();
> -
> - dup2(devnull_fd, STDIN_FILENO);
> - dup2(devnull_fd, STDOUT_FILENO);
> - dup2(devnull_fd, STDERR_FILENO);
> - close(devnull_fd);
> -
> - if (errno)
> + if (setsid() < 0 ||
> + dup2(devnull_fd, STDIN_FILENO) < 0 ||
> + dup2(devnull_fd, STDOUT_FILENO) < 0 ||
> + dup2(devnull_fd, STDERR_FILENO) < 0 ||
> + close(devnull_fd))
> exit(EXIT_FAILURE);
>
> return 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 --]
^ permalink raw reply [flat|nested] 20+ messages in thread