RFC Enable repository wide style checking
Get series with b4
b4 am [email protected]Versions newest first
- v1 RFC Latest10 patches 9 repliesIn review
Patches in v1 No applies recorded yet
| # | Subject and files | Size | Replies | Applied |
|---|---|---|---|---|
| 1/10 | tests: import GNULIB's syntax-check infrastructureMakefile cfg.mk maint.mk +3 more | +1693 | 0 replies | |
| 2/10 | maint: remove double semicolons in many filesblock/ cfg.mk hw/ +3 more | +10 | 2 replies | |
| 3/10 | maint: remove / fix many doubled wordsblock/ block/ cfg.mk +21 more | +29 | 1 reply | |
| 4/10 | maint: remove unused include for assert.hcfg.mk disas/ hw/ +6 more | +0 | 0 replies | |
| 5/10 | maint: remove unused include for dirent.hcfg.mk fsdev/ hw/ +3 more | +0 | 0 replies | |
| 6/10 | maint: remove unused include for signal.hbsd-user/ cfg.mk hw/ +8 more | +0 | 0 replies | |
| 7/10 | maint: remove unused include for strings.hcfg.mk disas/ hw/ +2 more | +0 | 0 replies | |
| 8/10 | maint: avoid useless "if (foo) free(foo)" patternbackends/ bsd-user/ cfg.mk +18 more | +42 | 0 replies | |
| 9/10 | maint: add check for use of POSIX functions which are not reentrant safeMakefile.nonreentrant cfg.mk | +134 | 0 replies | |
| 10/10 | maint: enable checking for qemu/osdep.h header usagecfg.mk | +2 | 0 replies |
Reviews trailers per patch
No Reviewed-by, Acked-by or Tested-by trailers for v1 yet.
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