[libvirt PATCH v2 00/10] Random bits found by clang-tidy
by Tim Wiederhake
clang-tidy is a static code analysis tool under the llvm umbrella. It is
primarily meant to be used on C++ code bases, but some of the checks it
provides also apply to C.
The findings vary in severity and contain pseudo-false-positives, i.e.
clang-tidy is flagging potential execution flows that could happen in
theory but are virtually impossible in real life: In function
`virGetUnprivSGIOSysfsPath`, variables `maj` and `min` would be read
unintialized if `stat()` failed and set `errno` to a negative value, to name
just one example.
The main source of false positive findings is the lack of support for
`__attribute__((cleanup))` in clang-tidy, which is heavily used in libvirt
through glib's `g_autofree` and `g_auto()` macros:
#include <stdlib.h>
void freeptr(int** p) {
if (*p)
free(*p);
}
int main() {
__attribute__((cleanup(freeptr))) int *ptr = NULL;
ptr = calloc(sizeof(int), 1);
return 0; /* flagged as memory leak of `ptr` */
}
This sadly renders clang-tidy's analysis of dynamic memory useless, hiding all
real issues that it could otherwise find.
Meson provides excellent integration for clang-tidy (a "clang-tidy" target is
automatically generated if a ".clang-tidy" configuration file is present
in the project's root directory). The amount of false-positives (many of
which present in a group of checks that cannot be disabled), random segfaults
in memory constraint environments such as the containers in the CI, and the
slow analysis (triggering time-outs in the CI), make this tool unfit for
inclusion in libvirt's GitLab CI though.
The patches in this series are the result of fixing some of the issues
reported by running
CC=clang meson build
ninja -C build # generate sources and header files
ninja -C build clang-tidy
with the following `.clang-tidy` configuration file:
---
Checks: >
*,
-abseil-*,
-android-*,
-boost-*,
-cppcoreguidelines-*,
-fuchsia-*,
-google-*,
-hicpp-*,
-llvm-*,
-modernize-*,
-mpi-,
-objc-,
-openmp-,
-zircon-*,
-readability-braces-around-statements,
-readability-magic-numbers
WarningsAsErrors: '*'
HeaderFilterRegex: ''
FormatStyle: none
...
V1: https://www.redhat.com/archives/libvir-list/2021-January/msg01152.html
Changes since V1:
* Expanded the justification for the "Replace bzero() with memset()" patch.
* Rewrote "udevProcessCCW: Initialize variable".
* Rewrote "tests: Prevent mallo with size 0".
* Dropped "vircommand: Remove NULL check in virCommandAddArg".
Note that this series now depends on the rewrite of tests/commandhelper.c,
which can be found here:
https://www.redhat.com/archives/libvir-list/2021-February/msg00034.html
Regards,
Tim
Tim Wiederhake (10):
virfile: Remove redundant #ifndef
xen: Fix indentation in xenParseXLSpice
qemu_tpm: Fix indentation in qemuTPMEmulatorBuildCommand
virsh-domain: Fix error handling of pthread_sigmask
Replace bzero() with memset()
udevProcessCCW: Initialize variable
virhostuptime: Fix rounding in uptime calculation
tests: Prevent malloc with size 0
vircryptotest: Directly assign string to avoid memcpy
vircommand: Simplify virCommandAddArg
src/libxl/xen_xl.c | 5 ++---
src/node_device/node_device_udev.c | 2 +-
src/qemu/qemu_tpm.c | 8 +++++---
src/util/virarptable.c | 2 +-
src/util/vircommand.c | 7 +------
src/util/virfile.c | 4 ++--
src/util/virhostuptime.c | 4 +++-
tests/commandhelper.c | 3 +++
tests/vircryptotest.c | 5 +----
tests/virpcimock.c | 2 +-
tools/virsh-domain.c | 8 ++++----
11 files changed, 24 insertions(+), 26 deletions(-)
--
2.26.2
4 years, 2 months
- 2
- 13
[PATCH] virfile: workaround for when posix_fallocate() is not supported by FS
by Roman Bogorodskiy
posix_fallocate() might be not supported by a filesystem, for example,
it's not supported by ZFS. In that case it fails with
return code 22 (EINVAL), and thus safezero_posix_fallocate() returns -1.
As safezero_posix_fallocate() is the first function tried by safezero()
and it tries other functions only when it returns -2, it fails
immediately without falling back to other methods, such as
safezero_slow().
Fix that by returning -2 if posix_fallocate() returns EINVAL, to give
safezero() a chance to try other functions.
Signed-off-by: Roman Bogorodskiy <bogorodskiy(a)gmail.com>
---
src/util/virfile.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 609cafdd34..67a350deb3 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -1119,6 +1119,16 @@ safezero_posix_fallocate(int fd, off_t offset, off_t len)
int ret = posix_fallocate(fd, offset, len);
if (ret == 0)
return 0;
+ else if (ret == EINVAL)
+ /* EINVAL is returned when either:
+ - Operation is not supported by the underlying filesystem,
+ - offset or len argument values are invalid.
+ Assuming that offset and len are valid, this error means
+ the operation is not supported, and we need to fall back
+ to other methods.
+ */
+ return -2;
+
errno = ret;
return -1;
}
--
2.30.0
4 years, 2 months
- 2
- 1