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 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
    ...

Regards,
Tim

Tim Wiederhake (11):
  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()
  udevGetIntSysfsAttr: Return -1 for missing attributes
  virhostuptime: Fix rounding in uptime calculation
  tests: Prevent malloc with size 0
  vircryptotest: Directly assign string to avoid memcpy
  vircommand: Remove NULL check in virCommandAddArg
  vircommand: Simplify virCommandAddArg

 src/libxl/xen_xl.c                 |  5 ++---
 src/node_device/node_device_udev.c |  5 ++++-
 src/qemu/qemu_tpm.c                |  8 +++++---
 src/util/virarptable.c             |  2 +-
 src/util/vircommand.c              | 12 +-----------
 src/util/virfile.c                 |  4 ++--
 src/util/virhostuptime.c           |  4 +++-
 tests/commandhelper.c              |  2 +-
 tests/vircryptotest.c              |  5 +----
 tests/virpcimock.c                 |  2 +-
 tools/virsh-domain.c               |  8 ++++----
 11 files changed, 25 insertions(+), 32 deletions(-)

-- 
2.26.2

On Thu, Jan 28, 2021 at 11:24:30 +0100, Tim Wiederhake wrote:

[...]

> Tim Wiederhake (11):
>   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()
>   udevGetIntSysfsAttr: Return -1 for missing attributes
>   virhostuptime: Fix rounding in uptime calculation
>   tests: Prevent malloc with size 0
>   vircryptotest: Directly assign string to avoid memcpy
>   vircommand: Remove NULL check in virCommandAddArg
>   vircommand: Simplify virCommandAddArg

On patches with no explicit reply:

Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On Thu, Jan 28, 2021 at 11:24:30AM +0100, Tim Wiederhake wrote:
> 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 and the slow
> analysis, triggering time-outs in the CI, make this tool unfit for inclusion
> in libvirt's GitLab CI though.

Is it possible to make it viable for CI by disabling *all* checks by
default and then selectively re-enabling just the handful that are
useful and don't have false positives ?


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Thu, 2021-01-28 at 10:35 +0000, Daniel P. Berrangé wrote:
> On Thu, Jan 28, 2021 at 11:24:30AM +0100, Tim Wiederhake wrote:
> > 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 and
> > the slow
> > analysis, triggering time-outs in the CI, make this tool unfit for
> > inclusion
> > in libvirt's GitLab CI though.
> 
> Is it possible to make it viable for CI by disabling *all* checks by
> default and then selectively re-enabling just the handful that are
> useful and don't have false positives ?
> 
> Regards,
> Daniel

That is what I tried at first. Unfortunately, it does not work for
several reasons:

* clang-tidy does not like memory constraint environments -- at least
that appears to be the bottom of it. The result is random segfaults.

* There are false positive findings in the group of "clang-analyze-*"
and "clang-diagnostic-*", which cannot be disabled or rather, are
implicitly re-enabled no matter your configuration. E.g: Flagging 
https://gitlab.com/libvirt/libvirt/-/blob/master/src/util/virfdstream.c#L1240 as dead store, see above explation of "__attribute__((cleanup))" for
background.

* There are true positive findings in the same group of checks that I,
frankly, just disagree with. E.g: Flagging 
https://gitlab.com/libvirt/libvirt/-/blob/master/src/util/virobject.c#L228
 as tautological comparison (in the first iteration of the `while`
loop), as `klass` is declared ATTRIBUTE_NONNULL. "Unrolling" the first
iteration does not make the code better in my eyes. "no-lint" markers
for clang-tidy do exist, but I would rather not litter the code with
them to make some software happy.

* clang-tidy requires all files to be present, or the run will
terminate with a non-zero return code and analysis errors. This
includes generated files. My Meson-fu is not particularly great, and I
have not found a "run generators only" target. Building libvirt
completely before running clang-tidy cuts badly in gitlab's one-hour
time budget, and running the monster below, for which I would like to
profoundly apologize, is also not a future-proof option:

  [ -d "${bld_dir}" ] || CC=clang meson "${bld_dir}" "${src_dir}"
  ninja -C "${bld_dir}" \
    src/access/viraccessapicheck.c \
    src/access/viraccessapicheck.h \
    src/access/viraccessapichecklxc.c \
    src/access/viraccessapichecklxc.h \
    src/access/viraccessapicheckqemu.c \
    src/access/viraccessapicheckqemu.h \
    src/admin/admin_client.h \
    src/admin/admin_protocol.c \
    src/admin/admin_protocol.h \
    src/admin/admin_server_dispatch_stubs.h \
    src/esx/esx_vi.generated.c \
    src/esx/esx_vi.generated.h \
    src/esx/esx_vi_methods.generated.c \
    src/esx/esx_vi_methods.generated.h \
    src/esx/esx_vi_methods.generated.macro \
    src/esx/esx_vi_types.generated.c \
    src/esx/esx_vi_types.generated.h \
    src/esx/esx_vi_types.generated.typedef \
    src/esx/esx_vi_types.generated.typeenum \
    src/esx/esx_vi_types.generated.typefromstring \
    src/esx/esx_vi_types.generated.typetostring \
    src/hyperv/hyperv_wmi_classes.generated.c \
    src/hyperv/hyperv_wmi_classes.generated.h \
    src/hyperv/hyperv_wmi_classes.generated.typedef \
    src/libvirt_functions.stp \
    src/libvirt_probes.h \
    src/libvirt_probes.stp \
    src/locking/lock_daemon_dispatch_stubs.h \
    src/locking/lock_protocol.c \
    src/locking/lock_protocol.h \
    src/logging/log_daemon_dispatch_stubs.h \
    src/logging/log_protocol.c \
    src/logging/log_protocol.h \
    src/lxc/lxc_controller_dispatch.h \
    src/lxc/lxc_monitor_dispatch.h \
    src/lxc/lxc_monitor_protocol.c \
    src/lxc/lxc_monitor_protocol.h \
    src/qemu/libvirt_qemu_probes.h \
    src/qemu/libvirt_qemu_probes.stp \
    src/remote/lxc_client_bodies.h \
    src/remote/lxc_daemon_dispatch_stubs.h \
    src/remote/lxc_protocol.c \
    src/remote/lxc_protocol.h \
    src/remote/qemu_client_bodies.h \
    src/remote/qemu_daemon_dispatch_stubs.h \
    src/remote/qemu_protocol.c \
    src/remote/qemu_protocol.h \
    src/remote/remote_client_bodies.h \
    src/remote/remote_daemon_dispatch_stubs.h \
    src/remote/remote_protocol.c \
    src/remote/remote_protocol.h \
    src/rpc/virkeepaliveprotocol.c \
    src/rpc/virkeepaliveprotocol.h \
    src/rpc/virnetprotocol.c \
    src/rpc/virnetprotocol.h \
    src/util/virkeycodetable_atset1.h \
    src/util/virkeycodetable_atset2.h \
    src/util/virkeycodetable_atset3.h \
    src/util/virkeycodetable_linux.h \
    src/util/virkeycodetable_osx.h \
    src/util/virkeycodetable_qnum.h \
    src/util/virkeycodetable_usb.h \
    src/util/virkeycodetable_win32.h \
    src/util/virkeycodetable_xtkbd.h \
    src/util/virkeynametable_linux.h \
    src/util/virkeynametable_osx.h \
    src/util/virkeynametable_win32.h \
    tools/wireshark/src/libvirt/keepalive.h \
    tools/wireshark/src/libvirt/lxc.h \
    tools/wireshark/src/libvirt/protocol.h \
    tools/wireshark/src/libvirt/qemu.h \
    tools/wireshark/src/libvirt/remote.h
  ninja -C "${bld_dir}" clang-tidy

In my opinion, it might be worthwile to have a `.clang-tidy`
configuration for libvirt, but running the tests is up to humans for
the forseeable future, not the CI. At the very least until clang-tidy
understands `__attribute__(cleanup))`, which I would love to report as
a bug to clang-tidy, but the disabled "self user registration" and the
amount of stale bugs that have not seen any attention in the last five
years is not giving raise to hope:
https://bugs.llvm.org/buglist.cgi?component=clang-tidy&order=changeddate%2Cpriority%2Cbug_severity&product=clang-tools-extra&query_format=advanced&resolution=-
--

Cheers,
Tim