* [PATCH 0/4] RFC: Updates for cppcheck-2.12 warnings
@ 2023-09-29 5:50 David Gibson
2023-09-29 5:50 ` [PATCH 1/4] cppcheck: Make many pointers const David Gibson
` (5 more replies)
0 siblings, 6 replies; 9+ messages in thread
From: David Gibson @ 2023-09-29 5:50 UTC (permalink / raw)
To: passt-dev, Stefano Brivio; +Cc: David Gibson
cppcheck 2.12 (which Fedora 38 has updated, for one) introduces a
number of new warnings. Unfortunately, at least one of these is a
clear bug in cppcheck.
This series fixes a number of the new warnings reported in passt
(patches 1..3) and works around the remaining cppcheck bug (patch 4).
I'm pretty confident that patches 1 & 2 are safe and beneficial to
apply regardless of which cppcheck we're using.
Patch 3 is a little more dubious, because it potentially increases the
cppcheck runtime. On my system it doesn't seem to make a significant
difference, but that might not always stay true.
Patch 4 is a tricky one. It applies a specific suppression to work
around the cppcheck bug. That's necessary to get a pass with the
currently available cppcheck. However, it's ugly and we'd like to
remove it once the bug is fixed, but have no obvious way to remind us
to do that. What we want to do here kind of depends how long it takes
the bug to be fixed, which isn't clear at the moment.
David Gibson (4):
cppcheck: Make many pointers const
conf: Remove overly cryptic selection of forward table
cppcheck: Use "exhaustive" level checking when available
cppcheck: Work around bug in cppcheck 2.12.0
Makefile | 6 ++++++
conf.c | 25 +++++++++++--------------
isolation.c | 2 +-
isolation.h | 2 +-
log.c | 6 +++---
netlink.c | 6 +++---
netlink.h | 6 +++---
pasta.c | 4 ++--
pasta.h | 2 +-
tap.c | 14 +++++++-------
tap.h | 6 +++---
tcp.c | 25 ++++++++++++++++---------
tcp_conn.h | 2 +-
tcp_splice.c | 5 +++--
tcp_splice.h | 3 ++-
udp.c | 4 ++--
udp.h | 2 +-
util.c | 5 +++--
util.h | 4 ++--
19 files changed, 71 insertions(+), 58 deletions(-)
--
2.41.0
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/4] cppcheck: Make many pointers const 2023-09-29 5:50 [PATCH 0/4] RFC: Updates for cppcheck-2.12 warnings David Gibson @ 2023-09-29 5:50 ` David Gibson 2023-09-29 5:50 ` [PATCH 2/4] conf: Remove overly cryptic selection of forward table David Gibson ` (4 subsequent siblings) 5 siblings, 0 replies; 9+ messages in thread From: David Gibson @ 2023-09-29 5:50 UTC (permalink / raw) To: passt-dev, Stefano Brivio; +Cc: David Gibson Newer versions of cppcheck (as of 2.12.0, at least) added a warning for pointers which could be declared to point at const data, but aren't. Based on that, make many pointers throughout the codebase const. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- conf.c | 5 +++-- isolation.c | 2 +- isolation.h | 2 +- log.c | 6 +++--- netlink.c | 6 +++--- netlink.h | 6 +++--- pasta.c | 4 ++-- pasta.h | 2 +- tap.c | 14 +++++++------- tap.h | 6 +++--- tcp.c | 18 +++++++++--------- tcp_conn.h | 2 +- tcp_splice.c | 5 +++-- tcp_splice.h | 3 ++- udp.c | 4 ++-- udp.h | 2 +- util.c | 5 +++-- util.h | 4 ++-- 18 files changed, 50 insertions(+), 46 deletions(-) diff --git a/conf.c b/conf.c index 0ad6e23..551a9da 100644 --- a/conf.c +++ b/conf.c @@ -419,7 +419,8 @@ bind_fail: * @addr: Address found in /etc/resolv.conf * @conf: Pointer to reference of current entry in array of IPv4 resolvers */ -static void add_dns4(struct ctx *c, struct in_addr *addr, struct in_addr **conf) +static void add_dns4(struct ctx *c, const struct in_addr *addr, + struct in_addr **conf) { /* Guest or container can only access local addresses via redirect */ if (IN4_IS_ADDR_LOOPBACK(addr)) { @@ -1177,7 +1178,7 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid) void conf(struct ctx *c, int argc, char **argv) { int netns_only = 0; - struct option options[] = { + const struct option options[] = { {"debug", no_argument, NULL, 'd' }, {"quiet", no_argument, NULL, 'q' }, {"foreground", no_argument, NULL, 'f' }, diff --git a/isolation.c b/isolation.c index 1866724..f394e93 100644 --- a/isolation.c +++ b/isolation.c @@ -303,7 +303,7 @@ void isolate_user(uid_t uid, gid_t gid, bool use_userns, const char *userns, * Mustn't: * - Remove syscalls we need to daemonise */ -int isolate_prefork(struct ctx *c) +int isolate_prefork(const struct ctx *c) { int flags = CLONE_NEWIPC | CLONE_NEWNS | CLONE_NEWUTS; uint64_t ns_caps = 0; diff --git a/isolation.h b/isolation.h index 6ca00bf..846b2af 100644 --- a/isolation.h +++ b/isolation.h @@ -10,7 +10,7 @@ void isolate_initial(void); void isolate_user(uid_t uid, gid_t gid, bool use_userns, const char *userns, enum passt_modes mode); -int isolate_prefork(struct ctx *c); +int isolate_prefork(const struct ctx *c); void isolate_postfork(const struct ctx *c); #endif /* ISOLATION_H */ diff --git a/log.c b/log.c index 63d7801..c57bc8d 100644 --- a/log.c +++ b/log.c @@ -216,7 +216,7 @@ void logfile_init(const char *name, const char *path, size_t size) * * #syscalls lseek ppc64le:_llseek ppc64:_llseek armv6l:_llseek armv7l:_llseek */ -static void logfile_rotate_fallocate(int fd, struct timespec *ts) +static void logfile_rotate_fallocate(int fd, const struct timespec *ts) { char buf[BUFSIZ], *nl; int n; @@ -253,7 +253,7 @@ static void logfile_rotate_fallocate(int fd, struct timespec *ts) * #syscalls lseek ppc64le:_llseek ppc64:_llseek armv6l:_llseek armv7l:_llseek * #syscalls ftruncate */ -static void logfile_rotate_move(int fd, struct timespec *ts) +static void logfile_rotate_move(int fd, const struct timespec *ts) { int header_len, write_offset, end, discard, n; char buf[BUFSIZ], *nl; @@ -318,7 +318,7 @@ out: * * fallocate() passed as EXTRA_SYSCALL only if FALLOC_FL_COLLAPSE_RANGE is there */ -static int logfile_rotate(int fd, struct timespec *ts) +static int logfile_rotate(int fd, const struct timespec *ts) { if (fcntl(fd, F_SETFL, O_RDWR /* Drop O_APPEND: explicit lseek() */)) return -errno; diff --git a/netlink.c b/netlink.c index 6c57a68..768c6fd 100644 --- a/netlink.c +++ b/netlink.c @@ -345,7 +345,7 @@ int nl_route_get_def(int s, unsigned int ifi, sa_family_t af, void *gw) * * Return: 0 on success, negative error code on failure */ -int nl_route_set_def(int s, unsigned int ifi, sa_family_t af, void *gw) +int nl_route_set_def(int s, unsigned int ifi, sa_family_t af, const void *gw) { struct req_t { struct nlmsghdr nlh; @@ -593,7 +593,7 @@ int nl_addr_get(int s, unsigned int ifi, sa_family_t af, * Return: 0 on success, negative error code on failure */ int nl_addr_set(int s, unsigned int ifi, sa_family_t af, - void *addr, int prefix_len) + const void *addr, int prefix_len) { struct req_t { struct nlmsghdr nlh; @@ -758,7 +758,7 @@ int nl_link_get_mac(int s, unsigned int ifi, void *mac) * * Return: 0 on success, negative error code on failure */ -int nl_link_set_mac(int s, unsigned int ifi, void *mac) +int nl_link_set_mac(int s, unsigned int ifi, const void *mac) { struct req_t { struct nlmsghdr nlh; diff --git a/netlink.h b/netlink.h index 9f4f8f4..3a1f0de 100644 --- a/netlink.h +++ b/netlink.h @@ -12,17 +12,17 @@ extern int nl_sock_ns; void nl_sock_init(const struct ctx *c, bool ns); unsigned int nl_get_ext_if(int s, sa_family_t af); int nl_route_get_def(int s, unsigned int ifi, sa_family_t af, void *gw); -int nl_route_set_def(int s, unsigned int ifi, sa_family_t af, void *gw); +int nl_route_set_def(int s, unsigned int ifi, sa_family_t af, const void *gw); int nl_route_dup(int s_src, unsigned int ifi_src, int s_dst, unsigned int ifi_dst, sa_family_t af); int nl_addr_get(int s, unsigned int ifi, sa_family_t af, void *addr, int *prefix_len, void *addr_l); int nl_addr_set(int s, unsigned int ifi, sa_family_t af, - void *addr, int prefix_len); + const void *addr, int prefix_len); int nl_addr_dup(int s_src, unsigned int ifi_src, int s_dst, unsigned int ifi_dst, sa_family_t af); int nl_link_get_mac(int s, unsigned int ifi, void *mac); -int nl_link_set_mac(int s, unsigned int ifi, void *mac); +int nl_link_set_mac(int s, unsigned int ifi, const void *mac); int nl_link_up(int s, unsigned int ifi, int mtu); #endif /* NETLINK_H */ diff --git a/pasta.c b/pasta.c index 9bc26d5..5fd44fe 100644 --- a/pasta.c +++ b/pasta.c @@ -362,7 +362,7 @@ void pasta_ns_conf(struct ctx *c) * * Return: inotify file descriptor, -1 on failure or if not needed/applicable */ -int pasta_netns_quit_init(struct ctx *c) +int pasta_netns_quit_init(const struct ctx *c) { int flags = O_NONBLOCK | O_CLOEXEC; union epoll_ref ref = { .type = EPOLL_TYPE_NSQUIT }; @@ -399,7 +399,7 @@ int pasta_netns_quit_init(struct ctx *c) void pasta_netns_quit_handler(struct ctx *c, int inotify_fd) { char buf[sizeof(struct inotify_event) + NAME_MAX + 1]; - struct inotify_event *in_ev = (struct inotify_event *)buf; + const struct inotify_event *in_ev = (struct inotify_event *)buf; if (read(inotify_fd, buf, sizeof(buf)) < (ssize_t)sizeof(*in_ev)) return; diff --git a/pasta.h b/pasta.h index 8722ca1..5d20063 100644 --- a/pasta.h +++ b/pasta.h @@ -13,7 +13,7 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid, int argc, char *argv[]); void pasta_ns_conf(struct ctx *c); void pasta_child_handler(int signal); -int pasta_netns_quit_init(struct ctx *c); +int pasta_netns_quit_init(const struct ctx *c); void pasta_netns_quit_handler(struct ctx *c, int inotify_fd); #endif /* PASTA_H */ diff --git a/tap.c b/tap.c index 93db989..93bb348 100644 --- a/tap.c +++ b/tap.c @@ -203,7 +203,7 @@ void tap_udp4_send(const struct ctx *c, struct in_addr src, in_port_t sport, * @len: ICMP packet length, including ICMP header */ void tap_icmp4_send(const struct ctx *c, struct in_addr src, struct in_addr dst, - void *in, size_t len) + const void *in, size_t len) { char buf[USHRT_MAX]; void *ip4h = tap_push_l2h(c, buf, ETH_P_IP); @@ -291,7 +291,7 @@ void tap_udp6_send(const struct ctx *c, */ void tap_icmp6_send(const struct ctx *c, const struct in6_addr *src, const struct in6_addr *dst, - void *in, size_t len) + const void *in, size_t len) { char buf[USHRT_MAX]; void *ip6h = tap_push_l2h(c, buf, ETH_P_IPV6); @@ -315,7 +315,7 @@ void tap_icmp6_send(const struct ctx *c, * * #syscalls:pasta write */ -static size_t tap_send_frames_pasta(struct ctx *c, +static size_t tap_send_frames_pasta(const struct ctx *c, const struct iovec *iov, size_t n) { size_t i; @@ -414,7 +414,7 @@ static size_t tap_send_frames_passt(const struct ctx *c, * @iov: Array of buffers, each containing one frame (with L2 headers) * @n: Number of buffers/frames in @iov */ -void tap_send_frames(struct ctx *c, const struct iovec *iov, size_t n) +void tap_send_frames(const struct ctx *c, const struct iovec *iov, size_t n) { size_t m; @@ -706,7 +706,7 @@ append: } for (j = 0, seq = tap4_l4; j < seq_count; j++, seq++) { - struct pool *p = (struct pool *)&seq->p; + const struct pool *p = (const struct pool *)&seq->p; size_t k; tap_packet_debug(NULL, NULL, seq, 0, NULL, p->count); @@ -869,7 +869,7 @@ append: } for (j = 0, seq = tap6_l4; j < seq_count; j++, seq++) { - struct pool *p = (struct pool *)&seq->p; + const struct pool *p = (const struct pool *)&seq->p; size_t k; tap_packet_debug(NULL, NULL, NULL, seq->protocol, seq, @@ -1022,7 +1022,7 @@ redo: pool_flush(pool_tap6); restart: while ((len = read(c->fd_tap, pkt_buf + n, TAP_BUF_BYTES - n)) > 0) { - struct ethhdr *eh = (struct ethhdr *)(pkt_buf + n); + const struct ethhdr *eh = (struct ethhdr *)(pkt_buf + n); if (len < (ssize_t)sizeof(*eh) || len > (ssize_t)ETH_MAX_MTU) { n += len; diff --git a/tap.h b/tap.h index 021fb7c..68509d1 100644 --- a/tap.h +++ b/tap.h @@ -62,7 +62,7 @@ void tap_udp4_send(const struct ctx *c, struct in_addr src, in_port_t sport, struct in_addr dst, in_port_t dport, const void *in, size_t len); void tap_icmp4_send(const struct ctx *c, struct in_addr src, struct in_addr dst, - void *in, size_t len); + const void *in, size_t len); const struct in6_addr *tap_ip6_daddr(const struct ctx *c, const struct in6_addr *src); void tap_udp6_send(const struct ctx *c, @@ -71,9 +71,9 @@ void tap_udp6_send(const struct ctx *c, uint32_t flow, const void *in, size_t len); void tap_icmp6_send(const struct ctx *c, const struct in6_addr *src, const struct in6_addr *dst, - void *in, size_t len); + const void *in, size_t len); int tap_send(const struct ctx *c, const void *data, size_t len); -void tap_send_frames(struct ctx *c, const struct iovec *iov, size_t n); +void tap_send_frames(const struct ctx *c, const struct iovec *iov, size_t n); void tap_update_mac(struct tap_hdr *taph, const unsigned char *eth_d, const unsigned char *eth_s); void tap_listen_handler(struct ctx *c, uint32_t events); diff --git a/tcp.c b/tcp.c index 680874f..1204e7b 100644 --- a/tcp.c +++ b/tcp.c @@ -1016,7 +1016,7 @@ void tcp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s) * tcp_sock4_iov_init() - Initialise scatter-gather L2 buffers for IPv4 sockets * @c: Execution context */ -static void tcp_sock4_iov_init(struct ctx *c) +static void tcp_sock4_iov_init(const struct ctx *c) { struct iphdr iph = L2_BUF_IP4_INIT(IPPROTO_TCP); struct iovec *iov; @@ -1237,7 +1237,7 @@ static void tcp_hash_remove(const struct ctx *c, * @old: Old location of tcp_tap_conn * @new: New location of tcp_tap_conn */ -static void tcp_tap_conn_update(struct ctx *c, struct tcp_tap_conn *old, +static void tcp_tap_conn_update(const struct ctx *c, struct tcp_tap_conn *old, struct tcp_tap_conn *new) { struct tcp_tap_conn *entry, *prev = NULL; @@ -1327,7 +1327,7 @@ void tcp_table_compact(struct ctx *c, union tcp_conn *hole) */ static void tcp_conn_destroy(struct ctx *c, union tcp_conn *conn_union) { - struct tcp_tap_conn *conn = &conn_union->tap; + const struct tcp_tap_conn *conn = &conn_union->tap; close(conn->sock); if (conn->timer != -1) @@ -1349,7 +1349,7 @@ static void tcp_rst_do(struct ctx *c, struct tcp_tap_conn *conn); * tcp_l2_flags_buf_flush() - Send out buffers for segments with no data (flags) * @c: Execution context */ -static void tcp_l2_flags_buf_flush(struct ctx *c) +static void tcp_l2_flags_buf_flush(const struct ctx *c) { tap_send_frames(c, tcp6_l2_flags_iov, tcp6_l2_flags_buf_used); tcp6_l2_flags_buf_used = 0; @@ -1362,7 +1362,7 @@ static void tcp_l2_flags_buf_flush(struct ctx *c) * tcp_l2_data_buf_flush() - Send out buffers for segments with data * @c: Execution context */ -static void tcp_l2_data_buf_flush(struct ctx *c) +static void tcp_l2_data_buf_flush(const struct ctx *c) { tap_send_frames(c, tcp6_l2_iov, tcp6_l2_buf_used); tcp6_l2_buf_used = 0; @@ -2098,7 +2098,7 @@ static void tcp_conn_from_tap(struct ctx *c, * * Return: 0 on success, negative error code from recv() on failure */ -static int tcp_sock_consume(struct tcp_tap_conn *conn, uint32_t ack_seq) +static int tcp_sock_consume(const struct tcp_tap_conn *conn, uint32_t ack_seq) { /* Simply ignore out-of-order ACKs: we already consumed the data we * needed from the buffer, and we won't rewind back to a lower ACK @@ -2124,14 +2124,14 @@ static int tcp_sock_consume(struct tcp_tap_conn *conn, uint32_t ack_seq) * @seq: Sequence number to be sent * @now: Current timestamp */ -static void tcp_data_to_tap(struct ctx *c, struct tcp_tap_conn *conn, +static void tcp_data_to_tap(const struct ctx *c, struct tcp_tap_conn *conn, ssize_t plen, int no_csum, uint32_t seq) { struct iovec *iov; if (CONN_V4(conn)) { struct tcp4_l2_buf_t *b = &tcp4_l2_buf[tcp4_l2_buf_used]; - uint16_t *check = no_csum ? &(b - 1)->iph.check : NULL; + const uint16_t *check = no_csum ? &(b - 1)->iph.check : NULL; iov = tcp4_l2_iov + tcp4_l2_buf_used++; iov->iov_len = tcp_l2_buf_fill_headers(c, conn, b, plen, @@ -2704,7 +2704,7 @@ static void tcp_snat_inbound(const struct ctx *c, union inany_addr *addr) static void tcp_tap_conn_from_sock(struct ctx *c, union tcp_listen_epoll_ref ref, struct tcp_tap_conn *conn, int s, - struct sockaddr *sa, + const struct sockaddr *sa, const struct timespec *now) { conn->c.spliced = false; diff --git a/tcp_conn.h b/tcp_conn.h index d67ea62..0751e00 100644 --- a/tcp_conn.h +++ b/tcp_conn.h @@ -190,7 +190,7 @@ extern union tcp_conn tc[]; extern int init_sock_pool4 [TCP_SOCK_POOL_SIZE]; extern int init_sock_pool6 [TCP_SOCK_POOL_SIZE]; -void tcp_splice_conn_update(struct ctx *c, struct tcp_splice_conn *new); +void tcp_splice_conn_update(const struct ctx *c, struct tcp_splice_conn *new); void tcp_table_compact(struct ctx *c, union tcp_conn *hole); void tcp_splice_destroy(struct ctx *c, union tcp_conn *conn_union); void tcp_splice_timer(struct ctx *c, union tcp_conn *conn_union); diff --git a/tcp_splice.c b/tcp_splice.c index a572aca..54fc317 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -253,7 +253,7 @@ static void conn_event_do(const struct ctx *c, struct tcp_splice_conn *conn, * @c: Execution context * @new: New location of tcp_splice_conn */ -void tcp_splice_conn_update(struct ctx *c, struct tcp_splice_conn *new) +void tcp_splice_conn_update(const struct ctx *c, struct tcp_splice_conn *new) { tcp_splice_epoll_ctl(c, new); if (tcp_splice_epoll_ctl(c, new)) @@ -486,7 +486,8 @@ static void tcp_splice_dir(struct tcp_splice_conn *conn, int ref_sock, * Return: true if able to create a spliced connection, false otherwise * #syscalls:pasta setsockopt */ -bool tcp_splice_conn_from_sock(struct ctx *c, union tcp_listen_epoll_ref ref, +bool tcp_splice_conn_from_sock(const struct ctx *c, + union tcp_listen_epoll_ref ref, struct tcp_splice_conn *conn, int s, const struct sockaddr *sa) { diff --git a/tcp_splice.h b/tcp_splice.h index e7a583a..dc486f1 100644 --- a/tcp_splice.h +++ b/tcp_splice.h @@ -10,7 +10,8 @@ struct tcp_splice_conn; void tcp_splice_sock_handler(struct ctx *c, struct tcp_splice_conn *conn, int s, uint32_t events); -bool tcp_splice_conn_from_sock(struct ctx *c, union tcp_listen_epoll_ref ref, +bool tcp_splice_conn_from_sock(const struct ctx *c, + union tcp_listen_epoll_ref ref, struct tcp_splice_conn *conn, int s, const struct sockaddr *sa); void tcp_splice_init(struct ctx *c); diff --git a/udp.c b/udp.c index ed1b7a5..cadf393 100644 --- a/udp.c +++ b/udp.c @@ -691,7 +691,7 @@ static size_t udp_update_hdr6(const struct ctx *c, int n, in_port_t dstport, * * Return: size of tap frame with headers */ -static void udp_tap_send(struct ctx *c, +static void udp_tap_send(const struct ctx *c, unsigned int start, unsigned int n, in_port_t dstport, bool v6, const struct timespec *now) { @@ -726,7 +726,7 @@ static void udp_tap_send(struct ctx *c, * * #syscalls recvmmsg */ -void udp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events, +void udp_sock_handler(const struct ctx *c, union epoll_ref ref, uint32_t events, const struct timespec *now) { /* For not entirely clear reasons (data locality?) pasta gets diff --git a/udp.h b/udp.h index 0ee0695..7837134 100644 --- a/udp.h +++ b/udp.h @@ -8,7 +8,7 @@ #define UDP_TIMER_INTERVAL 1000 /* ms */ -void udp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events, +void udp_sock_handler(const struct ctx *c, union epoll_ref ref, uint32_t events, const struct timespec *now); int udp_tap_handler(struct ctx *c, int af, const void *saddr, const void *daddr, const struct pool *p, int idx, const struct timespec *now); diff --git a/util.c b/util.c index 34c32b9..1f35382 100644 --- a/util.c +++ b/util.c @@ -320,7 +320,8 @@ void bitmap_clear(uint8_t *map, int bit) */ int bitmap_isset(const uint8_t *map, int bit) { - unsigned long *word = (unsigned long *)map + BITMAP_WORD(bit); + const unsigned long *word + = (const unsigned long *)map + BITMAP_WORD(bit); return !!(*word & BITMAP_BIT(bit)); } @@ -337,7 +338,7 @@ int bitmap_isset(const uint8_t *map, int bit) * #syscalls:pasta ppc64le:_llseek ppc64:_llseek armv6l:_llseek armv7l:_llseek */ void procfs_scan_listen(struct ctx *c, uint8_t proto, int ip_version, int ns, - uint8_t *map, uint8_t *exclude) + uint8_t *map, const uint8_t *exclude) { char *path, *line; struct lineread lr; diff --git a/util.h b/util.h index 57a05fb..60d6872 100644 --- a/util.h +++ b/util.h @@ -218,7 +218,7 @@ void bitmap_clear(uint8_t *map, int bit); int bitmap_isset(const uint8_t *map, int bit); char *line_read(char *buf, size_t len, int fd); void procfs_scan_listen(struct ctx *c, uint8_t proto, int ip_version, int ns, - uint8_t *map, uint8_t *exclude); + uint8_t *map, const uint8_t *exclude); void ns_enter(const struct ctx *c); bool ns_is_init(void); void write_pidfile(int fd, pid_t pid); @@ -235,7 +235,7 @@ int write_file(const char *path, const char *buf); * clang-tidy suppressions, because the warning doesn't show on the syscall * itself but later when we access the supposedly uninitialised field. */ -static inline void sa_init(struct sockaddr *sa, socklen_t *sl) +static inline void sa_init(struct sockaddr *sa, const socklen_t *sl) { #ifdef CLANG_TIDY_58992 if (sa) -- @@ -218,7 +218,7 @@ void bitmap_clear(uint8_t *map, int bit); int bitmap_isset(const uint8_t *map, int bit); char *line_read(char *buf, size_t len, int fd); void procfs_scan_listen(struct ctx *c, uint8_t proto, int ip_version, int ns, - uint8_t *map, uint8_t *exclude); + uint8_t *map, const uint8_t *exclude); void ns_enter(const struct ctx *c); bool ns_is_init(void); void write_pidfile(int fd, pid_t pid); @@ -235,7 +235,7 @@ int write_file(const char *path, const char *buf); * clang-tidy suppressions, because the warning doesn't show on the syscall * itself but later when we access the supposedly uninitialised field. */ -static inline void sa_init(struct sockaddr *sa, socklen_t *sl) +static inline void sa_init(struct sockaddr *sa, const socklen_t *sl) { #ifdef CLANG_TIDY_58992 if (sa) -- 2.41.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/4] conf: Remove overly cryptic selection of forward table 2023-09-29 5:50 [PATCH 0/4] RFC: Updates for cppcheck-2.12 warnings David Gibson 2023-09-29 5:50 ` [PATCH 1/4] cppcheck: Make many pointers const David Gibson @ 2023-09-29 5:50 ` David Gibson 2023-09-29 5:50 ` [PATCH 3/4] cppcheck: Use "exhaustive" level checking when available David Gibson ` (3 subsequent siblings) 5 siblings, 0 replies; 9+ messages in thread From: David Gibson @ 2023-09-29 5:50 UTC (permalink / raw) To: passt-dev, Stefano Brivio; +Cc: David Gibson In a couple of places in conf(), we use a local 'fwd' variable to reference one of our forwarding tables. The value depends on which command line option we're currently looking at, and is initialized rather cryptically from an assignment side-effect within the if condition checking that option. Newer versions of cppcheck complain about this assignment being an always true condition, but in any case it's both clearer and slightly shorter to use separate if branches for the two cases and set the forwarding parameter more directly. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- conf.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/conf.c b/conf.c index 551a9da..a235b31 100644 --- a/conf.c +++ b/conf.c @@ -1742,14 +1742,12 @@ void conf(struct ctx *c, int argc, char **argv) /* Inbound port options can be parsed now (after IPv4/IPv6 settings) */ optind = 1; do { - struct port_fwd *fwd; - name = getopt_long(argc, argv, optstring, options, NULL); - if ((name == 't' && (fwd = &c->tcp.fwd_in)) || - (name == 'u' && (fwd = &c->udp.fwd_in.f))) { - conf_ports(c, name, optarg, fwd); - } + if (name == 't') + conf_ports(c, name, optarg, &c->tcp.fwd_in); + else if (name == 'u') + conf_ports(c, name, optarg, &c->udp.fwd_in.f); } while (name != -1); if (c->mode == MODE_PASTA) @@ -1777,14 +1775,12 @@ void conf(struct ctx *c, int argc, char **argv) /* ...and outbound port options now that namespaces are set up. */ optind = 1; do { - struct port_fwd *fwd; - name = getopt_long(argc, argv, optstring, options, NULL); - if ((name == 'T' && (fwd = &c->tcp.fwd_out)) || - (name == 'U' && (fwd = &c->udp.fwd_out.f))) { - conf_ports(c, name, optarg, fwd); - } + if (name == 'T') + conf_ports(c, name, optarg, &c->tcp.fwd_out); + else if (name == 'U') + conf_ports(c, name, optarg, &c->udp.fwd_out.f); } while (name != -1); if (!c->ifi4) -- @@ -1742,14 +1742,12 @@ void conf(struct ctx *c, int argc, char **argv) /* Inbound port options can be parsed now (after IPv4/IPv6 settings) */ optind = 1; do { - struct port_fwd *fwd; - name = getopt_long(argc, argv, optstring, options, NULL); - if ((name == 't' && (fwd = &c->tcp.fwd_in)) || - (name == 'u' && (fwd = &c->udp.fwd_in.f))) { - conf_ports(c, name, optarg, fwd); - } + if (name == 't') + conf_ports(c, name, optarg, &c->tcp.fwd_in); + else if (name == 'u') + conf_ports(c, name, optarg, &c->udp.fwd_in.f); } while (name != -1); if (c->mode == MODE_PASTA) @@ -1777,14 +1775,12 @@ void conf(struct ctx *c, int argc, char **argv) /* ...and outbound port options now that namespaces are set up. */ optind = 1; do { - struct port_fwd *fwd; - name = getopt_long(argc, argv, optstring, options, NULL); - if ((name == 'T' && (fwd = &c->tcp.fwd_out)) || - (name == 'U' && (fwd = &c->udp.fwd_out.f))) { - conf_ports(c, name, optarg, fwd); - } + if (name == 'T') + conf_ports(c, name, optarg, &c->tcp.fwd_out); + else if (name == 'U') + conf_ports(c, name, optarg, &c->udp.fwd_out.f); } while (name != -1); if (!c->ifi4) -- 2.41.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/4] cppcheck: Use "exhaustive" level checking when available 2023-09-29 5:50 [PATCH 0/4] RFC: Updates for cppcheck-2.12 warnings David Gibson 2023-09-29 5:50 ` [PATCH 1/4] cppcheck: Make many pointers const David Gibson 2023-09-29 5:50 ` [PATCH 2/4] conf: Remove overly cryptic selection of forward table David Gibson @ 2023-09-29 5:50 ` David Gibson 2023-09-29 5:50 ` [PATCH 4/4] cppcheck: Work around bug in cppcheck 2.12.0 David Gibson ` (2 subsequent siblings) 5 siblings, 0 replies; 9+ messages in thread From: David Gibson @ 2023-09-29 5:50 UTC (permalink / raw) To: passt-dev, Stefano Brivio; +Cc: David Gibson Recent enough cppcheck versions (at least as of cppcheck 2.12) give this error processing conf.c: conf.c:1179:1: information: ValueFlow analysis is limited in conf. Use --check-level=exhaustive if full analysis is wanted. [checkLevelNormal] Adding --check-level=exhaustive doesn't seem to significantly increase the cppcheck run time for us, so enable it when possible, suppressing that warning. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- Makefile | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Makefile b/Makefile index af3bc75..941086a 100644 --- a/Makefile +++ b/Makefile @@ -278,6 +278,11 @@ clang-tidy: $(SRCS) $(HEADERS) -config='{CheckOptions: [{key: bugprone-suspicious-string-compare.WarnOnImplicitComparison, value: "false"}]}' \ --warnings-as-errors=* $(SRCS) -- $(filter-out -pie,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) -DCLANG_TIDY_58992 +CPPCHECK_EXHAUSTIVE := +ifeq ($(shell cppcheck --check-level=exhaustive /dev/null > /dev/null 2>&1; echo $$?),0) + CPPCHECK_EXHAUSTIVE += --check-level=exhaustive +endif + SYSTEM_INCLUDES := /usr/include $(wildcard /usr/include/$(TARGET)) ifeq ($(shell $(CC) -v 2>&1 | grep -c "gcc version"),1) VER := $(shell $(CC) -dumpversion) @@ -286,6 +291,7 @@ endif cppcheck: $(SRCS) $(HEADERS) cppcheck --std=c11 --error-exitcode=1 --enable=all --force \ --inconclusive --library=posix --quiet \ + $(CPPCHECK_EXHAUSTIVE) \ $(SYSTEM_INCLUDES:%=-I%) \ $(SYSTEM_INCLUDES:%=--config-exclude=%) \ $(SYSTEM_INCLUDES:%=--suppress=*:%/*) \ -- @@ -278,6 +278,11 @@ clang-tidy: $(SRCS) $(HEADERS) -config='{CheckOptions: [{key: bugprone-suspicious-string-compare.WarnOnImplicitComparison, value: "false"}]}' \ --warnings-as-errors=* $(SRCS) -- $(filter-out -pie,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) -DCLANG_TIDY_58992 +CPPCHECK_EXHAUSTIVE := +ifeq ($(shell cppcheck --check-level=exhaustive /dev/null > /dev/null 2>&1; echo $$?),0) + CPPCHECK_EXHAUSTIVE += --check-level=exhaustive +endif + SYSTEM_INCLUDES := /usr/include $(wildcard /usr/include/$(TARGET)) ifeq ($(shell $(CC) -v 2>&1 | grep -c "gcc version"),1) VER := $(shell $(CC) -dumpversion) @@ -286,6 +291,7 @@ endif cppcheck: $(SRCS) $(HEADERS) cppcheck --std=c11 --error-exitcode=1 --enable=all --force \ --inconclusive --library=posix --quiet \ + $(CPPCHECK_EXHAUSTIVE) \ $(SYSTEM_INCLUDES:%=-I%) \ $(SYSTEM_INCLUDES:%=--config-exclude=%) \ $(SYSTEM_INCLUDES:%=--suppress=*:%/*) \ -- 2.41.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 4/4] cppcheck: Work around bug in cppcheck 2.12.0 2023-09-29 5:50 [PATCH 0/4] RFC: Updates for cppcheck-2.12 warnings David Gibson ` (2 preceding siblings ...) 2023-09-29 5:50 ` [PATCH 3/4] cppcheck: Use "exhaustive" level checking when available David Gibson @ 2023-09-29 5:50 ` David Gibson 2023-09-29 5:52 ` [PATCH 0/4] RFC: Updates for cppcheck-2.12 warnings David Gibson 2023-09-29 15:31 ` Stefano Brivio 5 siblings, 0 replies; 9+ messages in thread From: David Gibson @ 2023-09-29 5:50 UTC (permalink / raw) To: passt-dev, Stefano Brivio; +Cc: David Gibson cppcheck 2.12.0 (and maybe some other versions) things this if condition is always true, which is demonstrably not true. Work around the bug for now. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- tcp.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tcp.c b/tcp.c index 1204e7b..a9a6f2a 100644 --- a/tcp.c +++ b/tcp.c @@ -1553,6 +1553,13 @@ static int tcp_update_seqack_wnd(const struct ctx *c, struct tcp_tap_conn *conn, conn->wnd_to_tap = MIN(new_wnd_to_tap >> conn->ws_to_tap, USHRT_MAX); + /* Certain cppcheck versions, e.g. 2.12.0 have a bug where they think + * the MIN() above restricts conn->wnd_to_tap to be zero. That's + * clearly incorrect, but until the bug is fixed, work around it. + * https://bugzilla.redhat.com/show_bug.cgi?id=2240705 + * https://sourceforge.net/p/cppcheck/discussion/general/thread/f5b1a00646/ + */ + /* cppcheck-suppress [knownConditionTrueFalse, unmatchedSuppression] */ if (!conn->wnd_to_tap) conn_flag(c, conn, ACK_TO_TAP_DUE); -- @@ -1553,6 +1553,13 @@ static int tcp_update_seqack_wnd(const struct ctx *c, struct tcp_tap_conn *conn, conn->wnd_to_tap = MIN(new_wnd_to_tap >> conn->ws_to_tap, USHRT_MAX); + /* Certain cppcheck versions, e.g. 2.12.0 have a bug where they think + * the MIN() above restricts conn->wnd_to_tap to be zero. That's + * clearly incorrect, but until the bug is fixed, work around it. + * https://bugzilla.redhat.com/show_bug.cgi?id=2240705 + * https://sourceforge.net/p/cppcheck/discussion/general/thread/f5b1a00646/ + */ + /* cppcheck-suppress [knownConditionTrueFalse, unmatchedSuppression] */ if (!conn->wnd_to_tap) conn_flag(c, conn, ACK_TO_TAP_DUE); -- 2.41.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 0/4] RFC: Updates for cppcheck-2.12 warnings 2023-09-29 5:50 [PATCH 0/4] RFC: Updates for cppcheck-2.12 warnings David Gibson ` (3 preceding siblings ...) 2023-09-29 5:50 ` [PATCH 4/4] cppcheck: Work around bug in cppcheck 2.12.0 David Gibson @ 2023-09-29 5:52 ` David Gibson 2023-09-29 15:31 ` Stefano Brivio 5 siblings, 0 replies; 9+ messages in thread From: David Gibson @ 2023-09-29 5:52 UTC (permalink / raw) To: passt-dev, Stefano Brivio [-- Attachment #1: Type: text/plain, Size: 2292 bytes --] On Fri, Sep 29, 2023 at 03:50:18PM +1000, David Gibson wrote: > cppcheck 2.12 (which Fedora 38 has updated, for one) introduces a > number of new warnings. Unfortunately, at least one of these is a > clear bug in cppcheck. > > This series fixes a number of the new warnings reported in passt > (patches 1..3) and works around the remaining cppcheck bug (patch 4). > I'm pretty confident that patches 1 & 2 are safe and beneficial to > apply regardless of which cppcheck we're using. > > Patch 3 is a little more dubious, because it potentially increases the > cppcheck runtime. On my system it doesn't seem to make a significant > difference, but that might not always stay true. > > Patch 4 is a tricky one. It applies a specific suppression to work > around the cppcheck bug. That's necessary to get a pass with the > currently available cppcheck. However, it's ugly and we'd like to > remove it once the bug is fixed, but have no obvious way to remind us > to do that. What we want to do here kind of depends how long it takes > the bug to be fixed, which isn't clear at the moment. Forgot to mention that this was based on the siphash series, although I don't think it will have any conflicts. > David Gibson (4): > cppcheck: Make many pointers const > conf: Remove overly cryptic selection of forward table > cppcheck: Use "exhaustive" level checking when available > cppcheck: Work around bug in cppcheck 2.12.0 > > Makefile | 6 ++++++ > conf.c | 25 +++++++++++-------------- > isolation.c | 2 +- > isolation.h | 2 +- > log.c | 6 +++--- > netlink.c | 6 +++--- > netlink.h | 6 +++--- > pasta.c | 4 ++-- > pasta.h | 2 +- > tap.c | 14 +++++++------- > tap.h | 6 +++--- > tcp.c | 25 ++++++++++++++++--------- > tcp_conn.h | 2 +- > tcp_splice.c | 5 +++-- > tcp_splice.h | 3 ++- > udp.c | 4 ++-- > udp.h | 2 +- > util.c | 5 +++-- > util.h | 4 ++-- > 19 files changed, 71 insertions(+), 58 deletions(-) > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/4] RFC: Updates for cppcheck-2.12 warnings 2023-09-29 5:50 [PATCH 0/4] RFC: Updates for cppcheck-2.12 warnings David Gibson ` (4 preceding siblings ...) 2023-09-29 5:52 ` [PATCH 0/4] RFC: Updates for cppcheck-2.12 warnings David Gibson @ 2023-09-29 15:31 ` Stefano Brivio 2023-10-03 2:36 ` David Gibson 5 siblings, 1 reply; 9+ messages in thread From: Stefano Brivio @ 2023-09-29 15:31 UTC (permalink / raw) To: David Gibson; +Cc: passt-dev On Fri, 29 Sep 2023 15:50:18 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > cppcheck 2.12 (which Fedora 38 has updated, for one) introduces a > number of new warnings. Unfortunately, at least one of these is a > clear bug in cppcheck. > > This series fixes a number of the new warnings reported in passt > (patches 1..3) and works around the remaining cppcheck bug (patch 4). > I'm pretty confident that patches 1 & 2 are safe and beneficial to > apply regardless of which cppcheck we're using. > > Patch 3 is a little more dubious, because it potentially increases the > cppcheck runtime. On my system it doesn't seem to make a significant > difference, but that might not always stay true. On my system, it's 23 seconds instead of 21... I don't really see a problem with that. > Patch 4 is a tricky one. It applies a specific suppression to work > around the cppcheck bug. That's necessary to get a pass with the > currently available cppcheck. However, it's ugly and we'd like to > remove it once the bug is fixed, but have no obvious way to remind us > to do that. What we want to do here kind of depends how long it takes > the bug to be fixed, which isn't clear at the moment. I don't see a big issue with this either, we already have one suppression like that in tcp_clamp_window() where we kind of identified the issue but it hasn't been solved yet. Once it's fixed, we'll hopefully notice and drop the suppression if cppcheck 2.12 is old enough by then, but if we don't, I don't think it's a drama. The whole series looks good to me by the way. -- Stefano ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/4] RFC: Updates for cppcheck-2.12 warnings 2023-09-29 15:31 ` Stefano Brivio @ 2023-10-03 2:36 ` David Gibson 2023-10-05 6:19 ` Stefano Brivio 0 siblings, 1 reply; 9+ messages in thread From: David Gibson @ 2023-10-03 2:36 UTC (permalink / raw) To: Stefano Brivio; +Cc: passt-dev [-- Attachment #1: Type: text/plain, Size: 2148 bytes --] On Fri, Sep 29, 2023 at 05:31:34PM +0200, Stefano Brivio wrote: > On Fri, 29 Sep 2023 15:50:18 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > cppcheck 2.12 (which Fedora 38 has updated, for one) introduces a > > number of new warnings. Unfortunately, at least one of these is a > > clear bug in cppcheck. > > > > This series fixes a number of the new warnings reported in passt > > (patches 1..3) and works around the remaining cppcheck bug (patch 4). > > I'm pretty confident that patches 1 & 2 are safe and beneficial to > > apply regardless of which cppcheck we're using. > > > > Patch 3 is a little more dubious, because it potentially increases the > > cppcheck runtime. On my system it doesn't seem to make a significant > > difference, but that might not always stay true. > > On my system, it's 23 seconds instead of 21... I don't really see a > problem with that. Right, it's like 16s vs 15s for me. I was just a bit concerned they might add more, very expensive tests under the "exhaustive" set later on. > > Patch 4 is a tricky one. It applies a specific suppression to work > > around the cppcheck bug. That's necessary to get a pass with the > > currently available cppcheck. However, it's ugly and we'd like to > > remove it once the bug is fixed, but have no obvious way to remind us > > to do that. What we want to do here kind of depends how long it takes > > the bug to be fixed, which isn't clear at the moment. > > I don't see a big issue with this either, we already have one > suppression like that in tcp_clamp_window() where we kind of identified > the issue but it hasn't been solved yet. > > Once it's fixed, we'll hopefully notice and drop the suppression if > cppcheck 2.12 is old enough by then, but if we don't, I don't think it's > a drama. > > The whole series looks good to me by the way. Ok. Well, apply whenever you're ready then, I guess. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/4] RFC: Updates for cppcheck-2.12 warnings 2023-10-03 2:36 ` David Gibson @ 2023-10-05 6:19 ` Stefano Brivio 0 siblings, 0 replies; 9+ messages in thread From: Stefano Brivio @ 2023-10-05 6:19 UTC (permalink / raw) To: David Gibson; +Cc: passt-dev On Tue, 3 Oct 2023 13:36:12 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > On Fri, Sep 29, 2023 at 05:31:34PM +0200, Stefano Brivio wrote: > > On Fri, 29 Sep 2023 15:50:18 +1000 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > cppcheck 2.12 (which Fedora 38 has updated, for one) introduces a > > > number of new warnings. Unfortunately, at least one of these is a > > > clear bug in cppcheck. > > > > > > This series fixes a number of the new warnings reported in passt > > > (patches 1..3) and works around the remaining cppcheck bug (patch 4). > > > I'm pretty confident that patches 1 & 2 are safe and beneficial to > > > apply regardless of which cppcheck we're using. > > > > > > Patch 3 is a little more dubious, because it potentially increases the > > > cppcheck runtime. On my system it doesn't seem to make a significant > > > difference, but that might not always stay true. > > > > On my system, it's 23 seconds instead of 21... I don't really see a > > problem with that. > > Right, it's like 16s vs 15s for me. I was just a bit concerned they > might add more, very expensive tests under the "exhaustive" set later > on. > > > > Patch 4 is a tricky one. It applies a specific suppression to work > > > around the cppcheck bug. That's necessary to get a pass with the > > > currently available cppcheck. However, it's ugly and we'd like to > > > remove it once the bug is fixed, but have no obvious way to remind us > > > to do that. What we want to do here kind of depends how long it takes > > > the bug to be fixed, which isn't clear at the moment. > > > > I don't see a big issue with this either, we already have one > > suppression like that in tcp_clamp_window() where we kind of identified > > the issue but it hasn't been solved yet. > > > > Once it's fixed, we'll hopefully notice and drop the suppression if > > cppcheck 2.12 is old enough by then, but if we don't, I don't think it's > > a drama. > > > > The whole series looks good to me by the way. > > Ok. Well, apply whenever you're ready then, I guess. Sure, applied now. -- Stefano ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-10-05 6:19 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-09-29 5:50 [PATCH 0/4] RFC: Updates for cppcheck-2.12 warnings David Gibson 2023-09-29 5:50 ` [PATCH 1/4] cppcheck: Make many pointers const David Gibson 2023-09-29 5:50 ` [PATCH 2/4] conf: Remove overly cryptic selection of forward table David Gibson 2023-09-29 5:50 ` [PATCH 3/4] cppcheck: Use "exhaustive" level checking when available David Gibson 2023-09-29 5:50 ` [PATCH 4/4] cppcheck: Work around bug in cppcheck 2.12.0 David Gibson 2023-09-29 5:52 ` [PATCH 0/4] RFC: Updates for cppcheck-2.12 warnings David Gibson 2023-09-29 15:31 ` Stefano Brivio 2023-10-03 2:36 ` David Gibson 2023-10-05 6:19 ` Stefano Brivio
Code repositories for project(s) associated with this public inbox https://passt.top/passt This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for IMAP folder(s).