* [PATCH 01/12] clang: Add .clang-format file
2024-11-05 23:25 [PATCH 00/12] Minor fixups for or inspired by clangd and related tools David Gibson
@ 2024-11-05 23:25 ` David Gibson
2024-11-05 23:25 ` [PATCH 02/12] Makefile: Simplify exclusion of qrap from static checks David Gibson
` (12 subsequent siblings)
13 siblings, 0 replies; 17+ messages in thread
From: David Gibson @ 2024-11-05 23:25 UTC (permalink / raw)
To: Stefano Brivio, passt-dev; +Cc: David Gibson
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 <david@gibson.dropbear.id.au>
---
.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
+...
--
@@ -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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 02/12] Makefile: Simplify exclusion of qrap from static checks
2024-11-05 23:25 [PATCH 00/12] Minor fixups for or inspired by clangd and related tools David Gibson
2024-11-05 23:25 ` [PATCH 01/12] clang: Add .clang-format file David Gibson
@ 2024-11-05 23:25 ` David Gibson
2024-11-05 23:25 ` [PATCH 03/12] clang: Move clang-tidy configuration from Makefile to .clang-tidy David Gibson
` (11 subsequent siblings)
13 siblings, 0 replies; 17+ messages in thread
From: David Gibson @ 2024-11-05 23:25 UTC (permalink / raw)
To: Stefano Brivio, passt-dev; +Cc: David Gibson
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 <david@gibson.dropbear.id.au>
---
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)
--
@@ -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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 03/12] clang: Move clang-tidy configuration from Makefile to .clang-tidy
2024-11-05 23:25 [PATCH 00/12] Minor fixups for or inspired by clangd and related tools David Gibson
2024-11-05 23:25 ` [PATCH 01/12] clang: Add .clang-format file David Gibson
2024-11-05 23:25 ` [PATCH 02/12] Makefile: Simplify exclusion of qrap from static checks David Gibson
@ 2024-11-05 23:25 ` David Gibson
2024-11-05 23:25 ` [PATCH 04/12] arch: Avoid explicit access to 'environ' David Gibson
` (10 subsequent siblings)
13 siblings, 0 replies; 17+ messages in thread
From: David Gibson @ 2024-11-05 23:25 UTC (permalink / raw)
To: Stefano Brivio, passt-dev; +Cc: David Gibson
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 <david@gibson.dropbear.id.au>
---
.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
+ # <linux/limits.h> instead of <limits.h>.
+ - "-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
-# <linux/limits.h> instead of <limits.h>.
-#
-# - 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)
--
@@ -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
-# <linux/limits.h> instead of <limits.h>.
-#
-# - 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 04/12] arch: Avoid explicit access to 'environ'
2024-11-05 23:25 [PATCH 00/12] Minor fixups for or inspired by clangd and related tools David Gibson
` (2 preceding siblings ...)
2024-11-05 23:25 ` [PATCH 03/12] clang: Move clang-tidy configuration from Makefile to .clang-tidy David Gibson
@ 2024-11-05 23:25 ` David Gibson
2024-11-05 23:25 ` [PATCH 05/12] flow: Correct type of flowside_at_sidx() David Gibson
` (9 subsequent siblings)
13 siblings, 0 replies; 17+ messages in thread
From: David Gibson @ 2024-11-05 23:25 UTC (permalink / raw)
To: Stefano Brivio, passt-dev; +Cc: David Gibson
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 <david@gibson.dropbear.id.au>
---
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");
}
}
--
@@ -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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 05/12] flow: Correct type of flowside_at_sidx()
2024-11-05 23:25 [PATCH 00/12] Minor fixups for or inspired by clangd and related tools David Gibson
` (3 preceding siblings ...)
2024-11-05 23:25 ` [PATCH 04/12] arch: Avoid explicit access to 'environ' David Gibson
@ 2024-11-05 23:25 ` David Gibson
2024-11-05 23:25 ` [PATCH 06/12] netlink: RTA_PAYLOAD() returns int, not size_t David Gibson
` (8 subsequent siblings)
13 siblings, 0 replies; 17+ messages in thread
From: David Gibson @ 2024-11-05 23:25 UTC (permalink / raw)
To: Stefano Brivio, passt-dev; +Cc: David Gibson
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 <david@gibson.dropbear.id.au>
---
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];
}
--
@@ -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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 06/12] netlink: RTA_PAYLOAD() returns int, not size_t
2024-11-05 23:25 [PATCH 00/12] Minor fixups for or inspired by clangd and related tools David Gibson
` (4 preceding siblings ...)
2024-11-05 23:25 ` [PATCH 05/12] flow: Correct type of flowside_at_sidx() David Gibson
@ 2024-11-05 23:25 ` David Gibson
2024-11-05 23:25 ` [PATCH 07/12] Makefile: Move NETNS_RUN_DIR definition to C code David Gibson
` (7 subsequent siblings)
13 siblings, 0 replies; 17+ messages in thread
From: David Gibson @ 2024-11-05 23:25 UTC (permalink / raw)
To: Stefano Brivio, passt-dev; +Cc: David Gibson
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 <david@gibson.dropbear.id.au>
---
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);
--
@@ -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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 07/12] Makefile: Move NETNS_RUN_DIR definition to C code
2024-11-05 23:25 [PATCH 00/12] Minor fixups for or inspired by clangd and related tools David Gibson
` (5 preceding siblings ...)
2024-11-05 23:25 ` [PATCH 06/12] netlink: RTA_PAYLOAD() returns int, not size_t David Gibson
@ 2024-11-05 23:25 ` David Gibson
2024-11-05 23:25 ` [PATCH 08/12] seccomp: Simplify handling of AUDIT_ARCH David Gibson
` (6 subsequent siblings)
13 siblings, 0 replies; 17+ messages in thread
From: David Gibson @ 2024-11-05 23:25 UTC (permalink / raw)
To: Stefano Brivio, passt-dev; +Cc: David Gibson
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 <david@gibson.dropbear.id.au>
---
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
--
@@ -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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 08/12] seccomp: Simplify handling of AUDIT_ARCH
2024-11-05 23:25 [PATCH 00/12] Minor fixups for or inspired by clangd and related tools David Gibson
` (6 preceding siblings ...)
2024-11-05 23:25 ` [PATCH 07/12] Makefile: Move NETNS_RUN_DIR definition to C code David Gibson
@ 2024-11-05 23:25 ` David Gibson
2024-11-05 23:25 ` [PATCH 09/12] Makefile: Use -DARCH for qrap only David Gibson
` (5 subsequent siblings)
13 siblings, 0 replies; 17+ messages in thread
From: David Gibson @ 2024-11-05 23:25 UTC (permalink / raw)
To: Stefano Brivio, passt-dev; +Cc: David Gibson
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 <david@gibson.dropbear.id.au>
---
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}"
--
@@ -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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 09/12] Makefile: Use -DARCH for qrap only
2024-11-05 23:25 [PATCH 00/12] Minor fixups for or inspired by clangd and related tools David Gibson
` (7 preceding siblings ...)
2024-11-05 23:25 ` [PATCH 08/12] seccomp: Simplify handling of AUDIT_ARCH David Gibson
@ 2024-11-05 23:25 ` David Gibson
2024-11-05 23:25 ` [PATCH 10/12] Makefile: Don't attempt to auto-detect stack size David Gibson
` (4 subsequent siblings)
13 siblings, 0 replies; 17+ messages in thread
From: David Gibson @ 2024-11-05 23:25 UTC (permalink / raw)
To: Stefano Brivio, passt-dev; +Cc: David Gibson
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 <david@gibson.dropbear.id.au>
---
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 \
--
@@ -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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 10/12] Makefile: Don't attempt to auto-detect stack size
2024-11-05 23:25 [PATCH 00/12] Minor fixups for or inspired by clangd and related tools David Gibson
` (8 preceding siblings ...)
2024-11-05 23:25 ` [PATCH 09/12] Makefile: Use -DARCH for qrap only David Gibson
@ 2024-11-05 23:25 ` David Gibson
2024-11-05 23:25 ` [PATCH 11/12] clang: Add rudimentary clangd configuration David Gibson
` (3 subsequent siblings)
13 siblings, 0 replies; 17+ messages in thread
From: David Gibson @ 2024-11-05 23:25 UTC (permalink / raw)
To: Stefano Brivio, passt-dev; +Cc: David Gibson
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 <david@gibson.dropbear.id.au>
---
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) \
--
@@ -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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 11/12] clang: Add rudimentary clangd configuration
2024-11-05 23:25 [PATCH 00/12] Minor fixups for or inspired by clangd and related tools David Gibson
` (9 preceding siblings ...)
2024-11-05 23:25 ` [PATCH 10/12] Makefile: Don't attempt to auto-detect stack size David Gibson
@ 2024-11-05 23:25 ` David Gibson
2024-11-05 23:25 ` [PATCH 12/12] util: Remove unused ffsl() function David Gibson
` (2 subsequent siblings)
13 siblings, 0 replies; 17+ messages in thread
From: David Gibson @ 2024-11-05 23:25 UTC (permalink / raw)
To: Stefano Brivio, passt-dev; +Cc: David Gibson
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 <david@gibson.dropbear.id.au>
---
.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]
--
@@ -0,0 +1,3 @@
+CompileFlags:
+ # Don't try to interpret our headers as C++'
+ Add: [-xc, -Wall]
--
2.47.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 12/12] util: Remove unused ffsl() function
2024-11-05 23:25 [PATCH 00/12] Minor fixups for or inspired by clangd and related tools David Gibson
` (10 preceding siblings ...)
2024-11-05 23:25 ` [PATCH 11/12] clang: Add rudimentary clangd configuration David Gibson
@ 2024-11-05 23:25 ` David Gibson
2024-11-06 19:13 ` [PATCH 00/12] Minor fixups for or inspired by clangd and related tools Stefano Brivio
2024-11-07 14:55 ` Stefano Brivio
13 siblings, 0 replies; 17+ messages in thread
From: David Gibson @ 2024-11-05 23:25 UTC (permalink / raw)
To: Stefano Brivio, passt-dev; +Cc: David Gibson
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 <david@gibson.dropbear.id.au>
---
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
--
@@ -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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 00/12] Minor fixups for or inspired by clangd and related tools
2024-11-05 23:25 [PATCH 00/12] Minor fixups for or inspired by clangd and related tools David Gibson
` (11 preceding siblings ...)
2024-11-05 23:25 ` [PATCH 12/12] util: Remove unused ffsl() function David Gibson
@ 2024-11-06 19:13 ` Stefano Brivio
2024-11-06 20:47 ` David Gibson
2024-11-07 14:55 ` Stefano Brivio
13 siblings, 1 reply; 17+ messages in thread
From: Stefano Brivio @ 2024-11-06 19:13 UTC (permalink / raw)
To: David Gibson; +Cc: passt-dev
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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 00/12] Minor fixups for or inspired by clangd and related tools
2024-11-06 19:13 ` [PATCH 00/12] Minor fixups for or inspired by clangd and related tools Stefano Brivio
@ 2024-11-06 20:47 ` David Gibson
2024-11-07 7:03 ` Stefano Brivio
0 siblings, 1 reply; 17+ messages in thread
From: David Gibson @ 2024-11-06 20:47 UTC (permalink / raw)
To: Stefano Brivio; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 3151 bytes --]
On Wed, Nov 06, 2024 at 08:13:29PM +0100, Stefano Brivio wrote:
> 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.
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 00/12] Minor fixups for or inspired by clangd and related tools
2024-11-06 20:47 ` David Gibson
@ 2024-11-07 7:03 ` Stefano Brivio
0 siblings, 0 replies; 17+ messages in thread
From: Stefano Brivio @ 2024-11-07 7:03 UTC (permalink / raw)
To: David Gibson; +Cc: passt-dev
On Thu, 7 Nov 2024 07:47:20 +1100
David Gibson <david@gibson.dropbear.id.au> 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 <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.
>
> 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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 00/12] Minor fixups for or inspired by clangd and related tools
2024-11-05 23:25 [PATCH 00/12] Minor fixups for or inspired by clangd and related tools David Gibson
` (12 preceding siblings ...)
2024-11-06 19:13 ` [PATCH 00/12] Minor fixups for or inspired by clangd and related tools Stefano Brivio
@ 2024-11-07 14:55 ` Stefano Brivio
13 siblings, 0 replies; 17+ messages in thread
From: Stefano Brivio @ 2024-11-07 14:55 UTC (permalink / raw)
To: David Gibson; +Cc: passt-dev
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.
Applied.
--
Stefano
^ permalink raw reply [flat|nested] 17+ messages in thread