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

newer
[PATCH 0/8] Avoid running cppcheck...

older
[PATCH v3] tcp: unify payload and...

David Gibson

6 Nov 2024 6 Nov '24

12:25 a.m.

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. David Gibson (12): clang: Add .clang-format file Makefile: Simplify exclusion of qrap from static checks clang: Move clang-tidy configuration from Makefile to .clang-tidy arch: Avoid explicit access to 'environ' flow: Correct type of flowside_at_sidx() netlink: RTA_PAYLOAD() returns int, not size_t Makefile: Move NETNS_RUN_DIR definition to C code seccomp: Simplify handling of AUDIT_ARCH Makefile: Use -DARCH for qrap only Makefile: Don't attempt to auto-detect stack size clang: Add rudimentary clangd configuration util: Remove unused ffsl() function .clang-format | 126 ++++++++++++++++++++++++++++++++++++++++++++++ .clang-tidy | 93 ++++++++++++++++++++++++++++++++++ .clangd | 3 ++ Makefile | 136 +++----------------------------------------------- arch.c | 2 +- conf.c | 2 + flow_table.h | 2 +- netlink.c | 4 +- seccomp.sh | 14 +++++- util.h | 5 +- 10 files changed, 247 insertions(+), 140 deletions(-) create mode 100644 .clang-format create mode 100644 .clang-tidy create mode 100644 .clangd -- 2.47.0

Show replies by date

David Gibson

6 Nov 6 Nov

12:25 a.m.

New subject: [PATCH 01/12] clang: Add .clang-format file

I've been experimenting with clangd, but its default format style is horrid. Since our style is basically that of the Linux kernel, copy the .clang-format from the kernel, minus reference to a bunch of kernel specific macros. Signed-off-by: David Gibson --- .clang-format | 126 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 126 insertions(+) create mode 100644 .clang-format diff --git a/.clang-format b/.clang-format new file mode 100644 index 0000000..78f177a --- /dev/null +++ b/.clang-format @@ -0,0 +1,126 @@ +# SPDX-License-Identifier: GPL-2.0 +# +# clang-format configuration file. Intended for clang-format >= 11. +# +# For more information, see: +# +# Documentation/dev-tools/clang-format.rst +# https://clang.llvm.org/docs/ClangFormat.html +# https://clang.llvm.org/docs/ClangFormatStyleOptions.html +# +--- +AccessModifierOffset: -4 +AlignAfterOpenBracket: Align +AlignConsecutiveAssignments: false +AlignConsecutiveDeclarations: false +AlignEscapedNewlines: Left +AlignOperands: true +AlignTrailingComments: false +AllowAllParametersOfDeclarationOnNextLine: false +AllowShortBlocksOnASingleLine: false +AllowShortCaseLabelsOnASingleLine: false +AllowShortFunctionsOnASingleLine: None +AllowShortIfStatementsOnASingleLine: false +AllowShortLoopsOnASingleLine: false +AlwaysBreakAfterDefinitionReturnType: None +AlwaysBreakAfterReturnType: None +AlwaysBreakBeforeMultilineStrings: false +AlwaysBreakTemplateDeclarations: false +BinPackArguments: true +BinPackParameters: true +BraceWrapping: + AfterClass: false + AfterControlStatement: false + AfterEnum: false + AfterFunction: true + AfterNamespace: true + AfterObjCDeclaration: false + AfterStruct: false + AfterUnion: false + AfterExternBlock: false + BeforeCatch: false + BeforeElse: false + IndentBraces: false + SplitEmptyFunction: true + SplitEmptyRecord: true + SplitEmptyNamespace: true +BreakBeforeBinaryOperators: None +BreakBeforeBraces: Custom +BreakBeforeInheritanceComma: false +BreakBeforeTernaryOperators: false +BreakConstructorInitializersBeforeComma: false +BreakConstructorInitializers: BeforeComma +BreakAfterJavaFieldAnnotations: false +BreakStringLiterals: false +ColumnLimit: 80 +CommentPragmas: '^ IWYU pragma:' +CompactNamespaces: false +ConstructorInitializerAllOnOneLineOrOnePerLine: false +ConstructorInitializerIndentWidth: 8 +ContinuationIndentWidth: 8 +Cpp11BracedListStyle: false +DerivePointerAlignment: false +DisableFormat: false +ExperimentalAutoDetectBinPacking: false +FixNamespaceComments: false + +# Taken from: +# git grep -h '^#define [^[:space:]]*for_each[^[:space:]]*(' include/ tools/ \ +# | sed "s,^#define $[^[:space:]]*for_each[^[:space:]]*$(.*$, - '\1'," \ +# | LC_ALL=C sort -u +ForEachMacros: + - 'for_each_nst' + +IncludeBlocks: Preserve +IncludeCategories: + - Regex: '.*' + Priority: 1 +IncludeIsMainRegex: '(Test)?$' +IndentCaseLabels: false +IndentGotoLabels: false +IndentPPDirectives: None +IndentWidth: 8 +IndentWrappedFunctionNames: false +JavaScriptQuotes: Leave +JavaScriptWrapImports: true +KeepEmptyLinesAtTheStartOfBlocks: false +MacroBlockBegin: '' +MacroBlockEnd: '' +MaxEmptyLinesToKeep: 1 +NamespaceIndentation: None +ObjCBinPackProtocolList: Auto +ObjCBlockIndentWidth: 8 +ObjCSpaceAfterProperty: true +ObjCSpaceBeforeProtocolList: true + +# Taken from git's rules +PenaltyBreakAssignment: 10 +PenaltyBreakBeforeFirstCallParameter: 30 +PenaltyBreakComment: 10 +PenaltyBreakFirstLessLess: 0 +PenaltyBreakString: 10 +PenaltyExcessCharacter: 100 +PenaltyReturnTypeOnItsOwnLine: 60 + +PointerAlignment: Right +ReflowComments: false +SortIncludes: false +SortUsingDeclarations: false +SpaceAfterCStyleCast: false +SpaceAfterTemplateKeyword: true +SpaceBeforeAssignmentOperators: true +SpaceBeforeCtorInitializerColon: true +SpaceBeforeInheritanceColon: true +SpaceBeforeParens: ControlStatementsExceptForEachMacros +SpaceBeforeRangeBasedForLoopColon: true +SpaceInEmptyParentheses: false +SpacesBeforeTrailingComments: 1 +SpacesInAngles: false +SpacesInContainerLiterals: false +SpacesInCStyleCastParentheses: false +SpacesInParentheses: false +SpacesInSquareBrackets: false +Standard: Cpp03 +TabWidth: 8 +UseTab: Always +... -- 2.47.0

David Gibson

12:25 a.m.

New subject: [PATCH 02/12] Makefile: Simplify exclusion of qrap from static checks

There are things in qrap.c that clang-tidy complains about that aren't worth fixing. So, we currently exclude it using $(filter-out). However, we already have a make variable which has just the passt sources, excluding qrap, so we can use that instead of the awkward filter-out expression. Currently, we still include qrap.c for cppcheck, but there's not much point doing so: it's, well, qrap, so we don't care that much about lints. Exclude it from cppcheck as well, for consistency. Signed-off-by: David Gibson --- Makefile | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Makefile b/Makefile index c1c6e30..8e14309 100644 --- a/Makefile +++ b/Makefile @@ -262,7 +262,7 @@ docs: README.md # parentheses to reinforce that certainly won't improve readability. -clang-tidy: $(filter-out qrap.c,$(SRCS)) $(HEADERS) +clang-tidy: $(PASST_SRCS) $(HEADERS) clang-tidy -checks=*,-modernize-*,\ -clang-analyzer-valist.Uninitialized,\ -cppcoreguidelines-init-variables,\ @@ -290,14 +290,14 @@ clang-tidy: $(filter-out qrap.c,$(SRCS)) $(HEADERS) -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 + --warnings-as-errors=* $(PASST_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) VER := $(shell $(CC) -dumpversion) SYSTEM_INCLUDES += /usr/lib/gcc/$(TARGET)/$(VER)/include endif -cppcheck: $(SRCS) $(HEADERS) +cppcheck: $(PASST_SRCS) $(HEADERS) if cppcheck --check-level=exhaustive /dev/null > /dev/null 2>&1; then \ CPPCHECK_EXHAUSTIVE="--check-level=exhaustive"; \ else \ @@ -313,4 +313,4 @@ cppcheck: $(SRCS) $(HEADERS) --inline-suppr \ --suppress=unusedStructMember \ $(filter -D%,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) \ - $(SRCS) $(HEADERS) + $(PASST_SRCS) $(HEADERS) -- 2.47.0

David Gibson

12:25 a.m.

New subject: [PATCH 03/12] clang: Move clang-tidy configuration from Makefile to .clang-tidy

Currently we configure clang-tidy with a very long command line spelled out in the Makefile (mostly a big list of lints to disable). Move it from here into a .clang-tidy configuration file, so that the config is accessible if clang-tidy is invoked in other ways (e.g. via clangd) as well. As a bonus this also means that we can move the bulky comments about why we're suppressing various tests inline with the relevant config lines. Signed-off-by: David Gibson --- .clang-tidy | 93 +++++++++++++++++++++++++++++++++++++++++++ Makefile | 111 +--------------------------------------------------- 2 files changed, 95 insertions(+), 109 deletions(-) create mode 100644 .clang-tidy diff --git a/.clang-tidy b/.clang-tidy new file mode 100644 index 0000000..9d346ec --- /dev/null +++ b/.clang-tidy @@ -0,0 +1,93 @@ +--- +Checks: + - "clang-diagnostic-*,clang-analyzer-*,*,-modernize-*" + + # TODO: enable once https://bugs.llvm.org/show_bug.cgi?id=41311 is fixed + - "-clang-analyzer-valist.Uninitialized" + + # Dubious value, would kill readability + - "-cppcoreguidelines-init-variables" + + # Dubious value over the compiler's built-in warning. Would + # increase verbosity. + - "-bugprone-assignment-in-if-condition" + + # Debatable whether these improve readability, right now it would look + # like a mess + - "-google-readability-braces-around-statements" + - "-hicpp-braces-around-statements" + - "-readability-braces-around-statements" + + # TODO: in most cases they are justified, but probably not everywhere + # + - "-readability-magic-numbers" + - "-cppcoreguidelines-avoid-magic-numbers" + + # TODO: this is Linux-only for the moment, nice to fix eventually + - "-llvmlibc-restrict-system-libc-headers" + + # Those are needed for syscalls, epoll_wait flags, etc. + - "-hicpp-signed-bitwise" + + # Probably not doable to impement this without plain memcpy(), memset() + - "-clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling" + + # TODO: not really important, but nice to fix eventually + - "-llvm-include-order" + + # Dubious value, would kill readability + - "-readability-isolate-declaration" + + # TODO: nice to fix eventually + - "-bugprone-narrowing-conversions" + - "-cppcoreguidelines-narrowing-conversions" + + # TODO: check, fix, and more in general constify wherever possible + - "-cppcoreguidelines-avoid-non-const-global-variables" + + # TODO: check paths where it might make sense to improve performance + - "-altera-unroll-loops" + - "-altera-id-dependent-backward-branch" + + # Not much can be done about them other than being careful + - "-bugprone-easily-swappable-parameters" + + # TODO: split reported functions + - "-readability-function-cognitive-complexity" + + # "Poor" alignment needed for structs reflecting message formats/headers + - "-altera-struct-pack-align" + + # TODO: check again if multithreading is implemented + - "-concurrency-mt-unsafe" + + # Complains about any identifier <3 characters, reasonable for + # globals, pointlessly verbose for locals and parameters. + - "-readability-identifier-length" + + # Wants to include headers which *directly* provide the things + # we use. That sounds nice, but means it will often want a OS + # specific header instead of a mostly standard one, such as + # instead of . + - "-misc-include-cleaner" + + # Want to replace all #defines of integers with enums. Kind of + # 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 + - "-cppcoreguidelines-macro-to-enum" + + # 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. + - "-readability-math-missing-parentheses" +WarningsAsErrors: "*" +HeaderFileExtensions: + - h +ImplementationFileExtensions: + - c +HeaderFilterRegex: "" +FormatStyle: none +CheckOptions: + bugprone-suspicious-string-compare.WarnOnImplicitComparison: "false" +SystemHeaders: false diff --git a/Makefile b/Makefile index 8e14309..f1e9937 100644 --- a/Makefile +++ b/Makefile @@ -181,116 +181,9 @@ docs: README.md done < README.md; \ ) > README.plain.md -# Checkers currently disabled for clang-tidy: -# - llvmlibc-restrict-system-libc-headers -# TODO: this is Linux-only for the moment, nice to fix eventually -# -# - google-readability-braces-around-statements -# - hicpp-braces-around-statements -# - readability-braces-around-statements -# Debatable whether that improves readability, right now it would look -# like a mess -# -# - readability-magic-numbers -# - cppcoreguidelines-avoid-magic-numbers -# TODO: in most cases they are justified, but probably not everywhere -# -# - clang-analyzer-valist.Uninitialized -# TODO: enable once https://bugs.llvm.org/show_bug.cgi?id=41311 is fixed -# -# - clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling -# Probably not doable to impement this without plain memcpy(), memset() -# -# - cppcoreguidelines-init-variables -# Dubious value, would kill readability -# -# - hicpp-signed-bitwise -# Those are needed for syscalls, epoll_wait flags, etc. -# -# - llvm-include-order -# TODO: not really important, but nice to fix eventually -# -# - readability-isolate-declaration -# Dubious value, would kill readability -# -# - bugprone-narrowing-conversions -# - cppcoreguidelines-narrowing-conversions -# TODO: nice to fix eventually -# -# - cppcoreguidelines-avoid-non-const-global-variables -# TODO: check, fix, and more in general constify wherever possible -# -# - altera-unroll-loops -# - altera-id-dependent-backward-branch -# TODO: check paths where it might make sense to improve performance -# -# - bugprone-easily-swappable-parameters -# Not much can be done about them other than being careful -# -# - readability-function-cognitive-complexity -# TODO: split reported functions -# -# - altera-struct-pack-align -# "Poor" alignment needed for structs reflecting message formats/headers -# -# - concurrency-mt-unsafe -# TODO: check again if multithreading is implemented -# -# - readability-identifier-length -# Complains about any identifier <3 characters, reasonable for -# globals, pointlessly verbose for locals and parameters. -# -# - bugprone-assignment-in-if-condition -# Dubious value over the compiler's built-in warning. Would -# increase verbosity. -# -# - misc-include-cleaner -# Wants to include headers which *directly* provide the things -# we use. That sounds nice, but means it will often want a OS -# specific header instead of a mostly standard one, such as -# instead of . -# -# - cppcoreguidelines-macro-to-enum -# Want to replace all #defines of integers with enums. Kind of -# 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: $(PASST_SRCS) $(HEADERS) - clang-tidy -checks=*,-modernize-*,\ - -clang-analyzer-valist.Uninitialized,\ - -cppcoreguidelines-init-variables,\ - -bugprone-assignment-in-if-condition,\ - -google-readability-braces-around-statements,\ - -hicpp-braces-around-statements,\ - -readability-braces-around-statements,\ - -readability-magic-numbers,\ - -llvmlibc-restrict-system-libc-headers,\ - -hicpp-signed-bitwise,\ - -clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling,\ - -llvm-include-order,\ - -cppcoreguidelines-avoid-magic-numbers,\ - -readability-isolate-declaration,\ - -bugprone-narrowing-conversions,\ - -cppcoreguidelines-narrowing-conversions,\ - -cppcoreguidelines-avoid-non-const-global-variables,\ - -altera-unroll-loops,-altera-id-dependent-backward-branch,\ - -bugprone-easily-swappable-parameters,\ - -readability-function-cognitive-complexity,\ - -altera-struct-pack-align,\ - -concurrency-mt-unsafe,\ - -readability-identifier-length,\ - -misc-include-cleaner,\ - -cppcoreguidelines-macro-to-enum,\ - -readability-math-missing-parentheses \ - -config='{CheckOptions: [{key: bugprone-suspicious-string-compare.WarnOnImplicitComparison, value: "false"}]}' \ - --warnings-as-errors=* $(PASST_SRCS) -- $(filter-out -pie,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) -DCLANG_TIDY_58992 + clang-tidy $(PASST_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.47.0

David Gibson

12:25 a.m.

New subject: [PATCH 04/12] arch: Avoid explicit access to 'environ'

We pass 'environ' to execve() in arch_avc2_exec(), so that we retain the environment in the current process. But the declaration of 'environ' is a bit weird - it doesn't seem to be in a standard header, requiring a manual explicit declaration. But, we can avoid needing to reference it explicitly by using execv() instead of execve(). This removes a clang warning. Signed-off-by: David Gibson --- arch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch.c b/arch.c index d1dfb73..e1ee729 100644 --- a/arch.c +++ b/arch.c @@ -45,7 +45,7 @@ void arch_avx2_exec(char **argv) "%s.avx2", exe)) die_perror("Can't build AVX2 executable path"); - execve(new_path, argv, environ); + execv(new_path, argv); warn_perror("Can't run AVX2 build, using non-AVX2 version"); } } -- 2.47.0

David Gibson

12:25 a.m.

New subject: [PATCH 05/12] flow: Correct type of flowside_at_sidx()

Due to a copy-pasta error, this returns 'PIF_NONE' instead of NULL on the failure case. PIF_NONE expands to 0, which turns into NULL, but it's still confusing, so fix it. This removes a clang warning. Signed-off-by: David Gibson --- flow_table.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flow_table.h b/flow_table.h index a499e7b..f15db53 100644 --- a/flow_table.h +++ b/flow_table.h @@ -110,7 +110,7 @@ static inline const struct flowside *flowside_at_sidx(flow_sidx_t sidx) const union flow *flow = flow_at_sidx(sidx); if (!flow) - return PIF_NONE; + return NULL; return &flow->f.side[sidx.sidei]; } -- 2.47.0

David Gibson

12:25 a.m.

New subject: [PATCH 06/12] netlink: RTA_PAYLOAD() returns int, not size_t

Since it's the size of a chunk of memory it would seem logical that RTA_PAYLOAD() returns size_t. However, it doesn't - it explicitly casts its result to an int. RTNH_OK(), which often takes the result of RTA_PAYLOAD() as a parameter compares it to an int, so using size_t can result in comparison of different-signed integer warnings from clang. Signed-off-by: David Gibson --- netlink.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/netlink.c b/netlink.c index 0bdbabf..4aba2a3 100644 --- a/netlink.c +++ b/netlink.c @@ -353,7 +353,7 @@ unsigned int nl_get_ext_if(int s, sa_family_t af) */ bool nl_route_get_def_multipath(struct rtattr *rta, void *gw) { - size_t nh_len = RTA_PAYLOAD(rta); + int nh_len = RTA_PAYLOAD(rta); struct rtnexthop *rtnh; bool found = false; int hops = -1; @@ -582,7 +582,7 @@ int nl_route_dup(int s_src, unsigned int ifi_src, *(unsigned int *)RTA_DATA(rta) = ifi_dst; } else if (rta->rta_type == RTA_MULTIPATH) { - size_t nh_len = RTA_PAYLOAD(rta); + int nh_len = RTA_PAYLOAD(rta); struct rtnexthop *rtnh; for (rtnh = (struct rtnexthop *)RTA_DATA(rta); -- 2.47.0

David Gibson

12:25 a.m.

New subject: [PATCH 07/12] Makefile: Move NETNS_RUN_DIR definition to C code

NETNS_RUN_DIR is set in the Makefile, then passed into the C code with -D. But NETNS_RUN_DIR is just a fixed string, it doesn't depend on any make probes or variables, so there's really no reason to handle it via the Makefile. Just move it to a plain #define in conf.c. Signed-off-by: David Gibson --- Makefile | 1 - conf.c | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index f1e9937..41f24e8 100644 --- a/Makefile +++ b/Makefile @@ -44,7 +44,6 @@ FLAGS := -Wall -Wextra -Wno-format-zero-length FLAGS += -pedantic -std=c11 -D_XOPEN_SOURCE=700 -D_GNU_SOURCE FLAGS += $(FORTIFY_FLAG) -O2 -pie -fPIE FLAGS += -DPAGE_SIZE=$(shell getconf PAGE_SIZE) -FLAGS += -DNETNS_RUN_DIR=\"/run/netns\" FLAGS += -DPASST_AUDIT_ARCH=AUDIT_ARCH_$(AUDIT_ARCH) FLAGS += -DRLIMIT_STACK_VAL=$(RLIMIT_STACK_VAL) FLAGS += -DARCH=\"$(TARGET_ARCH)\" diff --git a/conf.c b/conf.c index 14411b4..86566db 100644 --- a/conf.c +++ b/conf.c @@ -46,6 +46,8 @@ #include "isolation.h" #include "log.h" +#define NETNS_RUN_DIR "/run/netns" + /** * next_chunk - Return the next piece of a string delimited by a character * @s: String to search -- 2.47.0

David Gibson

12:25 a.m.

New subject: [PATCH 08/12] seccomp: Simplify handling of AUDIT_ARCH

Currently we construct the AUDIT_ARCH variable in the Makefile, then pass it into the C code with -D. The only place that uses it, though is the BPF filter generated by seccomp.sh. seccomp.sh already needs to do things differently depending on the arch, so it might as well just insert the expanded AUDIT_ARCH directly into the generated code, rather than using a #define. Arguably this is better, even, since it ensures more locally that the arch the BPF checks for matches the arch seccomp.sh built the filter for. Signed-off-by: David Gibson --- Makefile | 9 --------- seccomp.sh | 14 ++++++++++++-- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/Makefile b/Makefile index 41f24e8..c521d04 100644 --- a/Makefile +++ b/Makefile @@ -25,14 +25,6 @@ TARGET ?= $(shell $(CC) -dumpmachine) TARGET_ARCH := $(shell echo $(TARGET) | cut -f1 -d- | tr [A-Z] [a-z]) TARGET_ARCH := $(shell echo $(TARGET_ARCH) | sed 's/powerpc/ppc/') -AUDIT_ARCH := $(shell echo $(TARGET_ARCH) | tr [a-z] [A-Z] | sed 's/^ARM.*/ARM/') -AUDIT_ARCH := $(shell echo $(AUDIT_ARCH) | sed 's/I[456]86/I386/') -AUDIT_ARCH := $(shell echo $(AUDIT_ARCH) | sed 's/PPC64/PPC/') -AUDIT_ARCH := $(shell echo $(AUDIT_ARCH) | sed 's/PPCLE/PPC64LE/') -AUDIT_ARCH := $(shell echo $(AUDIT_ARCH) | sed 's/MIPS64EL/MIPSEL64/') -AUDIT_ARCH := $(shell echo $(AUDIT_ARCH) | sed 's/HPPA/PARISC/') -AUDIT_ARCH := $(shell echo $(AUDIT_ARCH) | sed 's/SH4/SH/') - # On some systems enabling optimization also enables source fortification, # automagically. Do not override it. FORTIFY_FLAG := @@ -44,7 +36,6 @@ FLAGS := -Wall -Wextra -Wno-format-zero-length FLAGS += -pedantic -std=c11 -D_XOPEN_SOURCE=700 -D_GNU_SOURCE FLAGS += $(FORTIFY_FLAG) -O2 -pie -fPIE FLAGS += -DPAGE_SIZE=$(shell getconf PAGE_SIZE) -FLAGS += -DPASST_AUDIT_ARCH=AUDIT_ARCH_$(AUDIT_ARCH) FLAGS += -DRLIMIT_STACK_VAL=$(RLIMIT_STACK_VAL) FLAGS += -DARCH=\"$(TARGET_ARCH)\" FLAGS += -DVERSION=\"$(VERSION)\" diff --git a/seccomp.sh b/seccomp.sh index 38aa826..6499c58 100755 --- a/seccomp.sh +++ b/seccomp.sh @@ -20,6 +20,15 @@ OUT="$(mktemp)" [ -z "${ARCH}" ] && ARCH="$(uname -m)" [ -z "${CC}" ] && CC="cc" +AUDIT_ARCH="AUDIT_ARCH_$(echo ${ARCH} | tr [a-z] [A-Z] \ + | sed 's/^ARM.*/ARM/' \ + | sed 's/I[456]86/I386/' \ + | sed 's/PPC64/PPC/' \ + | sed 's/PPCLE/PPC64LE/' \ + | sed 's/MIPS64EL/MIPSEL64/' \ + | sed 's/HPPA/PARISC/' \ + | sed 's/SH4/SH/')" + HEADER="/* This file was automatically generated by $(basename ${0}) */ #ifndef AUDIT_ARCH_PPC64LE @@ -32,7 +41,7 @@ struct sock_filter filter_@PROFILE@[] = { /* cppcheck-suppress [badBitmaskCheck, unmatchedSuppression] */ BPF_STMT(BPF_LD | BPF_W | BPF_ABS, (offsetof(struct seccomp_data, arch))), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, PASST_AUDIT_ARCH, 0, @KILL@), + BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, @AUDIT_ARCH@, 0, @KILL@), /* cppcheck-suppress [badBitmaskCheck, unmatchedSuppression] */ BPF_STMT(BPF_LD | BPF_W | BPF_ABS, (offsetof(struct seccomp_data, nr))), @@ -233,7 +242,8 @@ gen_profile() { sub ${__i} CALL "NR:${__nr}" "NAME:${__name}" "ALLOW:${__allow}" done - finish PRE "PROFILE:${__profile}" "KILL:$(( __statements + 1))" + finish PRE "PROFILE:${__profile}" "KILL:$(( __statements + 1))" \ + "AUDIT_ARCH:${AUDIT_ARCH}" } printf '%s\n' "${HEADER}" > "${OUT}" -- 2.47.0

David Gibson

12:25 a.m.

New subject: [PATCH 09/12] Makefile: Use -DARCH for qrap only

We insert -DARCH for all compiles, based on TARGET_ARCH determined in the Makefile. However, this is only used in qrap.c, not anywhere else in passt or pasta. Only supply this -D when compiling qrap specifically. Signed-off-by: David Gibson --- Makefile | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Makefile b/Makefile index c521d04..2a8540a 100644 --- a/Makefile +++ b/Makefile @@ -37,7 +37,6 @@ FLAGS += -pedantic -std=c11 -D_XOPEN_SOURCE=700 -D_GNU_SOURCE FLAGS += $(FORTIFY_FLAG) -O2 -pie -fPIE FLAGS += -DPAGE_SIZE=$(shell getconf PAGE_SIZE) FLAGS += -DRLIMIT_STACK_VAL=$(RLIMIT_STACK_VAL) -FLAGS += -DARCH=\"$(TARGET_ARCH)\" FLAGS += -DVERSION=\"$(VERSION)\" FLAGS += -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS) @@ -107,7 +106,7 @@ pasta.avx2 pasta.1 pasta: pasta%: passt% ln -sf $< $@ qrap: $(QRAP_SRCS) passt.h - $(CC) $(FLAGS) $(CFLAGS) $(CPPFLAGS) $(QRAP_SRCS) -o qrap $(LDFLAGS) + $(CC) $(FLAGS) $(CFLAGS) $(CPPFLAGS) -DARCH=\"$(TARGET_ARCH)\" $(QRAP_SRCS) -o qrap $(LDFLAGS) valgrind: EXTRA_SYSCALLS += rt_sigprocmask rt_sigtimedwait rt_sigaction \ rt_sigreturn getpid gettid kill clock_gettime mmap \ -- 2.47.0

David Gibson

12:25 a.m.

New subject: [PATCH 10/12] Makefile: Don't attempt to auto-detect stack size

We probe the available stack limit in the Makefile using rlimit, then use that to set the size of the stack when we clone() extra threads. But the rlimit at compile time need not be the same as the rlimit at runtime, so that's not particularly sensible. Ideally, we'd set the stack size based on an estimate of the actual maximum stack usage of all our clone()ed functions. We don't have that at the moment, but to keep things simple just set it to 1MiB - that's what the current probe will set things to on my default configuration Fedora 40, so it's likely to be fine in most cases. Signed-off-by: David Gibson --- Makefile | 6 ------ util.h | 2 +- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/Makefile b/Makefile index 2a8540a..56bf2e8 100644 --- a/Makefile +++ b/Makefile @@ -15,11 +15,6 @@ VERSION ?= $(shell git describe --tags HEAD 2>/dev/null || echo "unknown\ versio # the IPv6 socket API? (Linux does) DUAL_STACK_SOCKETS := 1 -RLIMIT_STACK_VAL := $(shell /bin/sh -c 'ulimit -s') -ifeq ($(RLIMIT_STACK_VAL),unlimited) -RLIMIT_STACK_VAL := 1024 -endif - TARGET ?= $(shell $(CC) -dumpmachine) # Get 'uname -m'-like architecture description for target TARGET_ARCH := $(shell echo $(TARGET) | cut -f1 -d- | tr [A-Z] [a-z]) @@ -36,7 +31,6 @@ FLAGS := -Wall -Wextra -Wno-format-zero-length FLAGS += -pedantic -std=c11 -D_XOPEN_SOURCE=700 -D_GNU_SOURCE FLAGS += $(FORTIFY_FLAG) -O2 -pie -fPIE FLAGS += -DPAGE_SIZE=$(shell getconf PAGE_SIZE) -FLAGS += -DRLIMIT_STACK_VAL=$(RLIMIT_STACK_VAL) FLAGS += -DVERSION=\"$(VERSION)\" FLAGS += -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS) diff --git a/util.h b/util.h index 3fc64cf..c341236 100644 --- a/util.h +++ b/util.h @@ -132,7 +132,7 @@ static inline uint32_t ntohl_unaligned(const void *p) return ntohl(val); } -#define NS_FN_STACK_SIZE (RLIMIT_STACK_VAL * 1024 / 8) +#define NS_FN_STACK_SIZE (1024 * 1024) /* 1MiB */ int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags, void *arg); #define NS_CALL(fn, arg) \ -- 2.47.0

David Gibson

12:25 a.m.

New subject: [PATCH 11/12] clang: Add rudimentary clangd configuration

clangd's default configuration seems to try to treat .h files as C++ not C. There are many more spurious warnings generated at present, but this removes some of the most egregious ones. Signed-off-by: David Gibson --- .clangd | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .clangd diff --git a/.clangd b/.clangd new file mode 100644 index 0000000..41bec92 --- /dev/null +++ b/.clangd @@ -0,0 +1,3 @@ +CompileFlags: + # Don't try to interpret our headers as C++' + Add: [-xc, -Wall] -- 2.47.0

David Gibson

12:25 a.m.

New subject: [PATCH 12/12] util: Remove unused ffsl() function

We supply a weak alias for ffsl() in case it's not defined in our libc. Except.. we don't have any users for it any more, so remove it. make cppcheck doesn't spot this at present for complicated reasons, but it might with tweaks to the options I'm experimenting with. Signed-off-by: David Gibson --- util.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/util.h b/util.h index c341236..2858b10 100644 --- a/util.h +++ b/util.h @@ -158,9 +158,6 @@ int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags, struct ctx; -/* cppcheck-suppress funcArgNamesDifferent */ -__attribute__ ((weak)) int ffsl(long int i) { return __builtin_ffsl(i); } - #ifdef CLOSE_RANGE_UNSHARE /* Linux kernel >= 5.9 */ /* glibc < 2.34 and musl as of 1.2.5 need these */ #ifndef SYS_close_range -- 2.47.0

Stefano Brivio

8:13 p.m.

On Wed, 6 Nov 2024 10:25:16 +1100 David Gibson 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

David Gibson

9:47 p.m.

On Wed, Nov 06, 2024 at 08:13:29PM +0100, Stefano Brivio wrote:

...

On Wed, 6 Nov 2024 10:25:16 +1100 David Gibson 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.

I thought this might just because I was mixing a string with a list of options with a json/yaml list of options. Alas, no, I think clang-tidy 16 only accepts a big string here rather than a yaml list. Doing it as a string would prevent the interspersing of the explanatory comments, so I don't think it's worth it.

...

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

Ok. Do you need anything from me? [snip]

...

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) { ^

Weird, looks like another false positive, maybe with the same cause as that other knownConditionTrueFalse thing we hit.

...

--

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); | ^

I think my FALLOC_FL_COLLAPSE_RANGE change should fix this as a side effect - the odd indentation is because of the #ifdef cutting off part ofthe if. -- 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

Stefano Brivio

7 Nov 7 Nov

8:03 a.m.

On Thu, 7 Nov 2024 07:47:20 +1100 David Gibson wrote:

...

On Wed, Nov 06, 2024 at 08:13:29PM +0100, Stefano Brivio wrote:

...
On Wed, 6 Nov 2024 10:25:16 +1100 David Gibson 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.

I thought this might just because I was mixing a string with a list of options with a json/yaml list of options. Alas, no, I think clang-tidy 16 only accepts a big string here rather than a yaml list. Doing it as a string would prevent the interspersing of the explanatory comments, so I don't think it's worth it.

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

Ok. Do you need anything from me?

No, no, thanks, I think I already figured out most of those.

...

[snip]

...
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) { ^

Weird, looks like another false positive, maybe with the same cause as that other knownConditionTrueFalse thing we hit.

It could be a similar cause, but the fact that it only happens on armhf and i686 makes me think it's not really the same.

...

...
--

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); | ^

I think my FALLOC_FL_COLLAPSE_RANGE change should fix this as a side effect - the odd indentation is because of the #ifdef cutting off part ofthe if.

Oops, sorry, this actually goes away with the second series. -- Stefano

Stefano Brivio

3:55 p.m.

On Wed, 6 Nov 2024 10:25:16 +1100 David Gibson 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.

Applied. -- Stefano

188

Age (days ago)

190

Last active (days ago)

List overview

Download

16 comments

2 participants

participants (2)

David Gibson
Stefano Brivio