Re: [PATCH 00/12] Minor fixups for or inspired by clangd and related tools

[Stefano Brivio <sbrivio@redhat.com>]

On Wed,  6 Nov 2024 10:25:16 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> I've been experimenting with Zed and clangd recently.  Currently it
> generates an enormous number of largely spurious errors and warnings
> on the passt code base.  Mostly that's due to its default
> configurations not suiting us.  This series adds some configuration
> that addresses a number of those warnings, though there remain many
> more for now.
> 
> Some of the warnings also look reasonable, so I have a grab bag of
> fixes or workarounds for some of those two.

This looks good to me. I tested build and functionality on Alpine x86,
Debian armhf testing, Debian i686 testing, Debian amd64 testing, Fedora
Rawhide x86 with this series plus:

- [PATCH] fwd: Squash different-signedness comparison warning
- [PATCH 0/8] Avoid running cppcheck on system headers

on top.

Other than single comments about "[PATCH 0/8] Avoid running cppcheck on
system headers", the only issue this series adds is, with clang-tidy 16
(current version on Debian testing), a rain of:

/home/sbrivio/passt/.clang-tidy:3:5: error: unexpected scalar
    - "clang-diagnostic-*,clang-analyzer-*,*,-modernize-*"
    ^

but it's fine, using clang-tidy 18 (on armhf) and clang-tidy 19
(everywhere else) fixes that.

There are a bunch of pre-existing cppcheck and clang-tidy warnings that
remain after this, and I plan to deal with them:

1. clang-tidy 19 on 32-bit architectures:

--
/home/sbrivio/passt/tap.c:1087:16: error: comparison of integers of different signs: 'ssize_t' (aka 'int') and 'unsigned int' [clang-diagnostic-sign-compare,-warnings-as-errors]
        for (n = 0; n <= (ssize_t)TAP_BUF_BYTES - ETH_MAX_MTU; n += len) {
                    ~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/sbrivio/passt/tcp.c:728:11: error: performing an implicit widening conversion to type 'uint64_t' (aka 'unsigned long long') of a multiplication performed in type 'unsigned long' [bugprone-implicit-widening-of-multiplication-result,-warnings-as-errors]
        if (v >= SNDBUF_BIG)
                 ^
/home/sbrivio/passt/util.h:149:22: note: expanded from macro 'SNDBUF_BIG'
#define SNDBUF_BIG              (4UL * 1024 * 1024)
                                 ^
/home/sbrivio/passt/tcp.c:728:11: note: make conversion explicit to silence this warning
        if (v >= SNDBUF_BIG)
                 ^
/home/sbrivio/passt/util.h:149:22: note: expanded from macro 'SNDBUF_BIG'
#define SNDBUF_BIG              (4UL * 1024 * 1024)
                                 ^~~~~~~~~~~~~~~~~
/home/sbrivio/passt/tcp.c:728:11: note: perform multiplication in a wider type
        if (v >= SNDBUF_BIG)
                 ^
/home/sbrivio/passt/util.h:149:22: note: expanded from macro 'SNDBUF_BIG'
#define SNDBUF_BIG              (4UL * 1024 * 1024)
                                 ^~~~~~~~~~
/home/sbrivio/passt/tcp.c:730:15: error: performing an implicit widening conversion to type 'uint64_t' (aka 'unsigned long long') of a multiplication performed in type 'unsigned long' [bugprone-implicit-widening-of-multiplication-result,-warnings-as-errors]
        else if (v > SNDBUF_SMALL)
                     ^
/home/sbrivio/passt/util.h:150:24: note: expanded from macro 'SNDBUF_SMALL'
#define SNDBUF_SMALL            (128UL * 1024)
                                 ^
/home/sbrivio/passt/tcp.c:730:15: note: make conversion explicit to silence this warning
        else if (v > SNDBUF_SMALL)
                     ^
/home/sbrivio/passt/util.h:150:24: note: expanded from macro 'SNDBUF_SMALL'
#define SNDBUF_SMALL            (128UL * 1024)
                                 ^~~~~~~~~~~~
/home/sbrivio/passt/tcp.c:730:15: note: perform multiplication in a wider type
        else if (v > SNDBUF_SMALL)
                     ^
/home/sbrivio/passt/util.h:150:24: note: expanded from macro 'SNDBUF_SMALL'
#define SNDBUF_SMALL            (128UL * 1024)
                                 ^~~~~
/home/sbrivio/passt/tcp.c:731:17: error: performing an implicit widening conversion to type 'uint64_t' (aka 'unsigned long long') of a multiplication performed in type 'unsigned long' [bugprone-implicit-widening-of-multiplication-result,-warnings-as-errors]
                v -= v * (v - SNDBUF_SMALL) / (SNDBUF_BIG - SNDBUF_SMALL) / 2;
                              ^
/home/sbrivio/passt/util.h:150:24: note: expanded from macro 'SNDBUF_SMALL'
#define SNDBUF_SMALL            (128UL * 1024)
                                 ^
/home/sbrivio/passt/tcp.c:731:17: note: make conversion explicit to silence this warning
                v -= v * (v - SNDBUF_SMALL) / (SNDBUF_BIG - SNDBUF_SMALL) / 2;
                              ^
/home/sbrivio/passt/util.h:150:24: note: expanded from macro 'SNDBUF_SMALL'
#define SNDBUF_SMALL            (128UL * 1024)
                                 ^~~~~~~~~~~~
/home/sbrivio/passt/tcp.c:731:17: note: perform multiplication in a wider type
                v -= v * (v - SNDBUF_SMALL) / (SNDBUF_BIG - SNDBUF_SMALL) / 2;
                              ^
/home/sbrivio/passt/util.h:150:24: note: expanded from macro 'SNDBUF_SMALL'
#define SNDBUF_SMALL            (128UL * 1024)
                                 ^~~~~
--

2. cppcheck 2.16 on 32-bit only (!):

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

3. clang-tidy 19.1.2 on Alpine x86:

--
/home/sbrivio/passt/log.c:216:3: error: misleading indentation: statement is indented too deeply [readability-misleading-indentation,-warnings-as-errors]
  216 |                 logfile_rotate_move(fd, now);
      |                 ^
/home/sbrivio/passt/log.c:207:2: note: did you mean this line to be inside this 'if'
  207 |         if (fcntl(fd, F_SETFL, O_RDWR /* Drop O_APPEND: explicit lseek() */))
      |         ^
/home/sbrivio/passt/passt.c:314:53: error: conditional operator with identical true and false expressions [bugprone-branch-clone,-warnings-as-errors]
  314 |         nfds = epoll_wait(c.epollfd, events, EPOLL_EVENTS, TIMER_INTERVAL);
      |                                                            ^
/home/sbrivio/passt/passt.c:60:29: note: expanded from macro 'TIMER_INTERVAL'
   60 | #define TIMER_INTERVAL          MIN(TIMER_INTERVAL_, FLOW_TIMER_INTERVAL)
      |                                     ^
/home/sbrivio/passt/passt.c:59:30: note: expanded from macro 'TIMER_INTERVAL_'
   59 | #define TIMER_INTERVAL_         MIN(TIMER_INTERVAL__, ICMP_TIMER_INTERVAL)
      |                                     ^
/home/sbrivio/passt/passt.c:58:26: note: expanded from macro 'TIMER_INTERVAL__'
   58 | #define TIMER_INTERVAL__        MIN(TCP_TIMER_INTERVAL, UDP_TIMER_INTERVAL)
      |                                 ^
/home/sbrivio/passt/util.h:46:33: note: expanded from macro 'MIN'
   46 | #define MIN(x, y)               (((x) < (y)) ? (x) : (y))
      |                                              ^
/home/sbrivio/passt/tap.c:1139:38: error: 'socket' should use SOCK_CLOEXEC where possible [android-cloexec-socket,-warnings-as-errors]
 1139 |         int fd = socket(AF_UNIX, SOCK_STREAM, 0);
      |                                             ^
      |                                              | SOCK_CLOEXEC
/home/sbrivio/passt/tap.c:1158:51: error: 'socket' should use SOCK_CLOEXEC where possible [android-cloexec-socket,-warnings-as-errors]
 1158 |                 ex = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0);
      |                                                                 ^
      |                                                                  | SOCK_CLOEXEC
/home/sbrivio/passt/tcp.c:1414:44: error: 'socket' should use SOCK_CLOEXEC where possible [android-cloexec-socket,-warnings-as-errors]
 1414 |         s = socket(af, SOCK_STREAM | SOCK_NONBLOCK, IPPROTO_TCP);
      |                                                   ^
      |                                                    | SOCK_CLOEXEC
/home/sbrivio/passt/util.c:186:38: error: 'socket' should use SOCK_CLOEXEC where possible [android-cloexec-socket,-warnings-as-errors]
  186 |         if ((s = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP)) < 0) {
      |                                             ^
      |                                              | SOCK_CLOEXEC
--

4. cppcheck 2.14.2 on Alpine x86:

--
dhcpv6.c:431:32: style: Variable 'client_id' can be declared as pointer to const [constVariablePointer]
 struct opt_hdr *ia, *bad_ia, *client_id;
                               ^
util.h:168:0: information: Unmatched suppression: funcArgNamesDifferent [unmatchedSuppression]
int close_range(unsigned int first, unsigned int last, int flags) {
^
--

I'll apply everything in a bit, minus 2/8 to 4/8 of "[PATCH 0/8] Avoid
running cppcheck on system headers".

-- 
Stefano