Skip to content

RFC Enable repository wide style checking

Daniel P. Berrange qemu-devel 1 version In review

Get series with b4
b4 am [email protected]

Versions newest first

  1. v1 RFC Latest10 patches 9 repliesIn review

Patches in v1 No applies recorded yet

#Subject and filesSizeRepliesApplied
1/10tests: import GNULIB's syntax-check infrastructureMakefile cfg.mk maint.mk +3 more+1693 -10 replies
2/10maint: remove double semicolons in many filesblock/vhdx.c cfg.mk hw/arm/vexpress.c +3 more+10 -72 replies
3/10maint: remove / fix many doubled wordsblock/qcow2-cluster.c block/qcow2-refcount.c cfg.mk +21 more+29 -271 reply
4/10maint: remove unused include for assert.hcfg.mk disas/ia64.c hw/i386/xen/xen_platform.c +6 more+0 -100 replies
5/10maint: remove unused include for dirent.hcfg.mk fsdev/virtio-9p-marshal.c hw/tpm/tpm_passthrough.c +3 more+0 -70 replies
6/10maint: remove unused include for signal.hbsd-user/signal.c cfg.mk hw/block/xen_disk.c +8 more+0 -110 replies
7/10maint: remove unused include for strings.hcfg.mk disas/microblaze.c hw/pci/shpc.c +2 more+0 -50 replies
8/10maint: avoid useless "if (foo) free(foo)" patternbackends/hostmem-file.c bsd-user/elfload.c cfg.mk +18 more+42 -940 replies
9/10maint: add check for use of POSIX functions which are not reentrant safeMakefile.nonreentrant cfg.mk+134 -00 replies
10/10maint: enable checking for qemu/osdep.h header usagecfg.mk+2 -20 replies

Reviews trailers per patch

No Reviewed-by, Acked-by or Tested-by trailers for v1 yet.

Read the v1 thread

Cover letter [Qemu-devel] [PATCH RFC 00/10] Enable repository wide style checking

Historically QEMU has relied on the checkpatch.pl script, borrowed from Linux, to check coding style compliance on patches which are submitted. For what it is designed for, it does a reasonable job, but I feel that QEMU would benefit from some more checking in this area, in particular checks that run across the entire repository, not just new patches.

Rather than attempt to replace checkpatch.pl, this series illustrates how we can augment our existing style compliance checking. This imports the infrastructure from GNULIB which provides a 'syntax-check' target in the makefiles.

By default the checks are run against all files which are committed to GIT, as identified by the 'vc-list-files' script. On a per-check basis you can define a list of file exclusions, so you can skip checks in places where there are valid reasons for the exceptional code style. It is also possible to set up rules so that they can be skipped on a per-line basis with magic comments in the source though this is not illustrated in this series.

Each style rule is provided by a separate make target named with an 'sc_' prefix. There are some boilerplate make rules provided to simplify the process of defining new checks. The maint.mk file provides the set of rules defined by the GNULIB project and the cfg.mk file is intended to contain any further rules desired by QEMU. That is not to say that QEMU must use all the rules defined by GNULIB - it is possible to turn each rule on/off globally as desired.

In this series the first patch sets up the infrastructure such that any developer can do

make syntax-check

to see violations in their local tree. The following patches then incrementally enable some of the rules and fix up the violations they detect. None of the things I've detected are particularly interesting/troublesome until I get to the second to last patch where I look for non-reentrant safe POSIX function usage. There are some areas identified that I think have potential lead to data corruption / crashes if we're unlucky with usage across threads.

The very last patch illustrates how we would mandate that inclusion of 'osdep.h' as the first header in all .c files Although I've enabled it, I didn't try to fix the .c files so 'make syntax-check' will fail with patch 10 applied. It should however pass for all patches upto #9

There are plenty more rules that could be enabled and there is obviously scope to right more custom rules.

The intent would be that developers run 'make syntax-check' before sending patches to check rules. It is also wired into 'make check' to make it harder to forget. It would also be expected that maintainers would run syntax-check on any patches they receive from contributors and reject them if failing, in the same way they'd be rejected if checkpatch.pl fails. Finally the automated build systems that various people are running should also run the 'make syntax-check' if they are not already running the 'make check' rule.

Daniel P. Berrange (10):
tests: import GNULIB's syntax-check infrastructure
maint: remove double semicolons in many files
maint: remove / fix many doubled words
maint: remove unused include for assert.h
maint: remove unused include for dirent.h
maint: remove unused include for signal.h
maint: remove unused include for strings.h
maint: avoid useless "if (foo) free(foo)" pattern
maint: add check for use of POSIX functions which are not reentrant
safe
maint: enable checking for qemu/osdep.h header usage

 Makefile                       |    5 +
 Makefile.nonreentrant          |  120 ++++
 backends/hostmem-file.c        |    4 +-
 block/qcow2-cluster.c          |    4 +-
 block/qcow2-refcount.c         |    2 +-
 block/vhdx.c                   |    2 +-
 bsd-user/elfload.c             |    4 +-
 bsd-user/signal.c              |    1 -
 cfg.mk                         |  156 +++++
 disas/ia64.c                   |    1 -
 disas/microblaze.c             |    1 -
 disas/sparc.c                  |    3 +-
 docs/libcacard.txt             |    4 +-
 docs/multiseat.txt             |    2 +-
 docs/specs/qcow2.txt           |    2 +-
 docs/specs/rocker.txt          |    2 +-
 fsdev/virtio-9p-marshal.c      |    1 -
 hw/arm/vexpress.c              |    4 +-
 hw/block/xen_disk.c            |    1 -
 hw/bt/hci.c                    |    9 +-
 hw/core/loader.c               |    3 +-
 hw/core/qdev-properties.c      |    4 +-
 hw/display/exynos4210_fimd.c   |    4 +-
 hw/i386/xen/xen_platform.c     |    2 -
 hw/intc/arm_gic.c              |    2 +-
 hw/mips/mips_r4k.c             |    4 +-
 hw/net/fsl_etsec/rings.c       |    4 +-
 hw/net/rocker/rocker.c         |    4 +-
 hw/net/rocker/rocker_desc.c    |    8 +-
 hw/net/rtl8139.c               |    2 +-
 hw/net/xen_nic.c               |    1 -
 hw/nvram/fw_cfg.c              |    4 +-
 hw/pci-host/prep.c             |    4 +-
 hw/pci/shpc.c                  |    1 -
 hw/sd/sd.c                     |    3 +-
 hw/tpm/tpm_passthrough.c       |    2 -
 hw/usb/hcd-xhci.c              |    4 +-
 hw/usb/host-libusb.c           |    2 +-
 hw/usb/redirect.c              |    2 -
 hw/vfio/common.c               |    2 +-
 hw/vfio/pci.c                  |    1 -
 hw/xen/xen_pt_config_init.c    |    4 +-
 include/block/block.h          |    2 +-
 include/exec/memory.h          |    2 +-
 linux-user/elfload.c           |    2 +-
 linux-user/signal.c            |    1 -
 maint.mk                       | 1228 ++++++++++++++++++++++++++++++++++++++++
 migration/rdma.c               |    2 +-
 migration/savevm.c             |    8 +-
 numa.c                         |    2 +-
 os-win32.c                     |    1 -
 page_cache.c                   |    1 -
 qemu-char.c                    |    5 +-
 qemu-doc.texi                  |    2 +-
 qemu-img.texi                  |    2 +-
 qemu-options.hx                |    2 +-
 qga/commands-win32.c           |    2 +-
 scripts/useless-if-before-free |  207 +++++++
 scripts/vc-list-files          |  113 ++++
 target-arm/cpu.h               |    4 +-
 target-arm/helper.c            |    2 +-
 target-arm/translate.c         |    2 +-
 target-i386/translate.c        |    1 -
 target-lm32/helper.c           |    2 +-
 target-microblaze/op_helper.c  |    1 -
 target-microblaze/translate.c  |    2 +-
 target-mips/helper.c           |    1 -
 target-moxie/helper.c          |    3 +-
 target-moxie/translate.c       |    1 -
 target-sh4/helper.c            |    1 -
 target-sh4/op_helper.c         |    1 -
 target-tricore/helper.c        |    1 -
 tests/Makefile                 |    2 +-
 tests/bios-tables-test.c       |   36 +-
 tests/tcg/testthread.c         |    1 -
 tests/test-xbzrle.c            |    2 -
 ui/spice-display.c             |   14 +-
 util/bitmap.c                  |    2 +-
 78 files changed, 1901 insertions(+), 155 deletions(-)
 create mode 100644 Makefile.nonreentrant
 create mode 100644 cfg.mk
 create mode 100644 maint.mk
 create mode 100755 scripts/useless-if-before-free
 create mode 100755 scripts/vc-list-files
Signature · 1 line
2.4.3