* [PATCH v2 1/7] tests: introduce tree-wide code style checking
2022-07-04 15:22 [PATCH v2 0/7] tests: introduce a tree-wide code style checking facility Daniel P. Berrangé
@ 2022-07-04 15:22 ` Daniel P. Berrangé
2022-07-04 15:46 ` Peter Maydell
2022-07-04 15:22 ` [PATCH v2 2/7] misc: fix mixups of bool constants with int variables Daniel P. Berrangé
` (5 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrangé @ 2022-07-04 15:22 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Alex Bennée, Thomas Huth, Peter Maydell,
Philippe Mathieu-Daudé, Daniel P. Berrangé
Historically QEMU has used the 'scripts/checkpatch.pl' script to
validate various style rules but there are a number of issues:
- It requires the contributor to remember to run it as it
is not wired into 'make check'
- While it can be told to check whole files, in practice
it is usually only used to check patch diffs, because the
former would flag up pages of pre-existing violations that
have never been fixed
- It is said to be OK to ignore some things reported by the
script, but don't record these exceptional cases anywere.
Thus contributors looking at existing violations in tree
are never sure whether they are intentional or historical
unfixed technical debt.
- There is no distinct reporting for each type of check
performed and as a consequence there is also no way to
mark particular files to be skipped for particular checks
This commit aims to give us a better approach to checking many types
of code style problems by introducing a flexible way to define whole
tree style checks.
The logic is essentially an import of the 'top/maint.mk' file from
GNULIB, with the following changes applied
- Logic unrelated to the GNULIB syntax-check functionality
is removed.
- sc_excl is redefined, so that whitespace is turned into
an '|' condition. This allows filename exclusion lists
to span multiple lines making it more readable.
- VC_LIST is changed to directly invoke git instead of
indirectly via the vc-list script, since QEMU does not
need portaility across many source control systems.
- .DEFAULT_GOAL is set to 'syntax-check' since this is being
used in a self-contained manner and thus doesn't need to
play nice with our makefile rules QEMU has
This commit does the bare minimum introducing the basic infra:
- tests/style-infra.mak - the cut down import of maint.mk
- tests/style-excludes.mak - where the list of filename
exclusions per test will be maintained (empty)
- tests/style.mak - where the interesting tests live (empty)
As a general rule new checks can be implemented by merely defining
a regex matching the code pattern that should be blocked.
For example, consider we want to stop people using the 'bool' type
entirely. A rule starting with the prefix 'sc_' is defined in the
style.mak file:
sc_prohibit_bool:
prohibit='\<bool\>' \
halt='do not use the bool type' \
$(_sc_search_regexp)
Where '$(_sc_search_regexp)' (acquired from style-infra.mak)
contains all the magic to perform the tree-wide search for the
offending code pattern, reporting '$(halt)' as the error message
if violations are found.
If certain files need to be skipped for certain tests this can
be achieved by defining a variable with 'exclude_file_name_regexp--'
followed by the name of the check
exclude_file_name_regexp--sc_prohibit_bool = \
i-am-allowed-to-use-bool.c \
and-so-am-i.c
Some checks can't be easily implemented by a simple per-line matching
regex. Since the checks are just implemented in make/shell, custom
rules can run essentially arbitrary logic.
Note that the checks require the use of 'git' to detect the list of
source files to search. Thus the check is skipped when not running
from a git repository.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
meson.build | 3 +
tests/Makefile.include | 3 +-
tests/meson.build | 19 +++
tests/style-excludes.mak | 4 +
tests/style-infra.mak | 265 +++++++++++++++++++++++++++++++++++++++
tests/style.mak | 24 ++++
6 files changed, 317 insertions(+), 1 deletion(-)
create mode 100644 tests/style-excludes.mak
create mode 100644 tests/style-infra.mak
create mode 100644 tests/style.mak
diff --git a/meson.build b/meson.build
index 65a885ea69..420102353e 100644
--- a/meson.build
+++ b/meson.build
@@ -18,6 +18,9 @@ config_host = keyval.load(meson.current_build_dir() / 'config-host.mak')
enable_modules = 'CONFIG_MODULES' in config_host
enable_static = 'CONFIG_STATIC' in config_host
+make = find_program(config_host['MAKE'])
+in_gitrepo = run_command('test', '-d', '.git', check: false).returncode() == 0
+
# Allow both shared and static libraries unless --enable-static
static_kwargs = enable_static ? {'static': true} : {}
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 3accb83b13..f7c1d2644e 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -3,12 +3,13 @@
.PHONY: check-help
check-help:
@echo "Regression testing targets:"
- @echo " $(MAKE) check Run block, qapi-schema, unit, softfloat, qtest and decodetree tests"
+ @echo " $(MAKE) check Run block, qapi-schema, unit, style, softfloat, qtest and decodetree tests"
@echo " $(MAKE) bench Run speed tests"
@echo
@echo "Individual test suites:"
@echo " $(MAKE) check-qtest-TARGET Run qtest tests for given target"
@echo " $(MAKE) check-qtest Run qtest tests"
+ @echo " $(MAKE) check-style Run style checks"
@echo " $(MAKE) check-unit Run qobject tests"
@echo " $(MAKE) check-qapi-schema Run QAPI schema tests"
@echo " $(MAKE) check-block Run block tests"
diff --git a/tests/meson.build b/tests/meson.build
index 8e318ec513..e26cadbc8a 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -89,6 +89,25 @@ if get_option('tcg').allowed()
endif
endif
+if in_gitrepo
+ rc = run_command(
+ 'sed', '-n',
+ 's/^sc_\\([a-zA-Z0-9_-]*\\):.*/\\1/p',
+ meson.current_source_dir() / 'style.mak',
+ check: true,
+ )
+
+ sc_tests = rc.stdout().strip().split()
+
+ foreach check: sc_tests
+ test(check,
+ make,
+ args: [ '-f', files('style.mak'), 'sc_' + check ],
+ workdir: meson.project_source_root(),
+ suite: 'style')
+ endforeach
+endif
+
subdir('unit')
subdir('qapi-schema')
subdir('qtest')
diff --git a/tests/style-excludes.mak b/tests/style-excludes.mak
new file mode 100644
index 0000000000..32c0e9c328
--- /dev/null
+++ b/tests/style-excludes.mak
@@ -0,0 +1,4 @@
+# -*- makefile -*-
+#
+# Filenames that should be excluded from specific
+# style checks performed by style.mak
diff --git a/tests/style-infra.mak b/tests/style-infra.mak
new file mode 100644
index 0000000000..99229f0c3f
--- /dev/null
+++ b/tests/style-infra.mak
@@ -0,0 +1,265 @@
+# -*- makefile -*-
+#
+# Rules for simple code style checks applying across the
+# whole tree. Partially derived from GNULIB's 'maint.mk'
+#
+# Copyright (C) 2008-2019 Red Hat, Inc.
+# Copyright (C) 2003-2019 Free Software Foundation, Inc.
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see
+# <http://www.gnu.org/licenses/>.
+
+ME := tests/style.mak
+
+srcdir = .
+
+AWK ?= awk
+GREP ?= grep
+SED ?= sed
+
+# Helper variables.
+_empty =
+_sp = $(_empty) $(_empty)
+
+VC_LIST = (cd $(srcdir) && git ls-tree -r 'HEAD:' | \
+ sed -n "s|^100[^ ]*.||p" )
+
+# You can override this variable in cfg.mk if your gnulib submodule lives
+# in a different location.
+gnulib_dir ?= $(shell if test -f $(srcdir)/gnulib/gnulib-tool; then \
+ echo $(srcdir)/gnulib; \
+ else \
+ echo ${GNULIB_SRCDIR}; \
+ fi)
+
+# You can override this variable in cfg.mk to set your own regexp
+# matching files to ignore.
+VC_LIST_ALWAYS_EXCLUDE_REGEX ?= ^$$
+
+# This is to preprocess robustly the output of $(VC_LIST), so that even
+# when $(srcdir) is a pathological name like "....", the leading sed command
+# removes only the intended prefix.
+_dot_escaped_srcdir = $(subst .,\.,$(srcdir))
+
+# Post-process $(VC_LIST) output, prepending $(srcdir)/, but only
+# when $(srcdir) is not ".".
+ifeq ($(srcdir),.)
+ _prepend_srcdir_prefix =
+else
+ _prepend_srcdir_prefix = | $(SED) 's|^|$(srcdir)/|'
+endif
+
+# In order to be able to consistently filter "."-relative names,
+# (i.e., with no $(srcdir) prefix), this definition is careful to
+# remove any $(srcdir) prefix, and to restore what it removes.
+_sc_excl = \
+ $(or $(subst $(_sp),|,$(exclude_file_name_regexp--$@)),^$$)
+
+VC_LIST_EXCEPT = \
+ $(VC_LIST) | $(SED) 's|^$(_dot_escaped_srcdir)/||' \
+ | if test -f $(srcdir)/.x-$@; then $(GREP) -vEf $(srcdir)/.x-$@; \
+ else $(GREP) -Ev -e "$${VC_LIST_EXCEPT_DEFAULT-ChangeLog}"; fi \
+ | $(GREP) -Ev -e '($(VC_LIST_ALWAYS_EXCLUDE_REGEX)|$(_sc_excl))' \
+ $(_prepend_srcdir_prefix)
+
+
+# Prevent programs like 'sort' from considering distinct strings to be equal.
+# Doing it here saves us from having to set LC_ALL elsewhere in this file.
+export LC_ALL = C
+
+# Collect the names of rules starting with 'sc_'.
+syntax-check-rules := $(sort $(shell env LC_ALL=C $(SED) -n \
+ 's/^\(sc_[a-zA-Z0-9_-]*\):.*/\1/p' $(srcdir)/$(ME)))
+.PHONY: $(syntax-check-rules)
+
+ifeq ($(shell $(VC_LIST) >/dev/null 2>&1; echo $$?),0)
+ local-checks-available += $(syntax-check-rules)
+else
+ local-checks-available += no-vc-detected
+no-vc-detected:
+ @echo "No version control files detected; skipping syntax check"
+endif
+.PHONY: $(local-checks-available)
+
+# Arrange to print the name of each syntax-checking rule just before running it.
+$(syntax-check-rules): %: %.m
+sc_m_rules_ = $(patsubst %, %.m, $(syntax-check-rules))
+.PHONY: $(sc_m_rules_)
+$(sc_m_rules_):
+ @echo $(patsubst sc_%.m, %, $@)
+ @date +%s.%N > .sc-start-$(basename $@)
+
+# Compute and print the elapsed time for each syntax-check rule.
+sc_z_rules_ = $(patsubst %, %.z, $(syntax-check-rules))
+.PHONY: $(sc_z_rules_)
+$(sc_z_rules_): %.z: %
+ @end=$$(date +%s.%N); \
+ start=$$(cat .sc-start-$*); \
+ rm -f .sc-start-$*; \
+ $(AWK) -v s=$$start -v e=$$end \
+ 'END {printf "%.2f $(patsubst sc_%,%,$*)\n", e - s}' < /dev/null
+
+# The patsubst here is to replace each sc_% rule with its sc_%.z wrapper
+# that computes and prints elapsed time.
+local-check := \
+ $(patsubst sc_%, sc_%.z, \
+ $(filter-out $(local-checks-to-skip), $(local-checks-available)))
+
+syntax-check: $(local-check)
+
+.DEFAULT_GOAL := syntax-check
+
+# _sc_search_regexp
+#
+# This macro searches for a given construct in the selected files and
+# then takes some action.
+#
+# Parameters (shell variables):
+#
+# prohibit | require
+#
+# Regular expression (ERE) denoting either a forbidden construct
+# or a required construct. Those arguments are exclusive.
+#
+# exclude
+#
+# Regular expression (ERE) denoting lines to ignore that matched
+# a prohibit construct. For example, this can be used to exclude
+# comments that mention why the nearby code uses an alternative
+# construct instead of the simpler prohibited construct.
+#
+# in_vc_files | in_files
+#
+# grep-E-style regexp selecting the files to check. For in_vc_files,
+# the regexp is used to select matching files from the list of all
+# version-controlled files; for in_files, it's from the names printed
+# by "find $(srcdir)". When neither is specified, use all files that
+# are under version control.
+#
+# containing | non_containing
+#
+# Select the files (non) containing strings matching this regexp.
+# If both arguments are specified then CONTAINING takes
+# precedence.
+#
+# with_grep_options
+#
+# Extra options for grep.
+#
+# ignore_case
+#
+# Ignore case.
+#
+# halt
+#
+# Message to display before to halting execution.
+#
+# Finally, you may exempt files based on an ERE matching file names.
+# For example, to exempt from the sc_space_tab check all files with the
+# .diff suffix, set this Make variable:
+#
+# exclude_file_name_regexp--sc_space_tab = \.diff$
+#
+# Note that while this functionality is mostly inherited via VC_LIST_EXCEPT,
+# when filtering by name via in_files, we explicitly filter out matching
+# names here as well.
+
+# Initialize each, so that envvar settings cannot interfere.
+export require =
+export prohibit =
+export exclude =
+export in_vc_files =
+export in_files =
+export containing =
+export non_containing =
+export halt =
+export with_grep_options =
+
+# By default, _sc_search_regexp does not ignore case.
+export ignore_case =
+_ignore_case = $$(test -n "$$ignore_case" && printf %s -i || :)
+
+define _sc_say_and_exit
+ dummy=; : so we do not need a semicolon before each use; \
+ { printf '%s\n' "$(ME): $$msg" 1>&2; exit 1; };
+endef
+
+define _sc_search_regexp
+ dummy=; : so we do not need a semicolon before each use; \
+ \
+ : Check arguments; \
+ test -n "$$prohibit" && test -n "$$require" \
+ && { msg='Cannot specify both prohibit and require' \
+ $(_sc_say_and_exit) } || :; \
+ test -z "$$prohibit" && test -z "$$require" \
+ && { msg='Should specify either prohibit or require' \
+ $(_sc_say_and_exit) } || :; \
+ test -z "$$prohibit" && test -n "$$exclude" \
+ && { msg='Use of exclude requires a prohibit pattern' \
+ $(_sc_say_and_exit) } || :; \
+ test -n "$$in_vc_files" && test -n "$$in_files" \
+ && { msg='Cannot specify both in_vc_files and in_files' \
+ $(_sc_say_and_exit) } || :; \
+ test "x$$halt" != x \
+ || { msg='halt not defined' $(_sc_say_and_exit) }; \
+ \
+ : Filter by file name; \
+ if test -n "$$in_files"; then \
+ files=$$(find $(srcdir) | $(GREP) -E "$$in_files" \
+ | $(GREP) -Ev '$(_sc_excl)'); \
+ else \
+ files=$$($(VC_LIST_EXCEPT)); \
+ if test -n "$$in_vc_files"; then \
+ files=$$(echo "$$files" | $(GREP) -E "$$in_vc_files"); \
+ fi; \
+ fi; \
+ \
+ : Filter by content; \
+ test -n "$$files" \
+ && test -n "$$containing" \
+ && { files=$$(echo "$$files" | xargs $(GREP) -l "$$containing"); } \
+ || :; \
+ test -n "$$files" \
+ && test -n "$$non_containing" \
+ && { files=$$(echo "$$files" | xargs $(GREP) -vl "$$non_containing"); } \
+ || :; \
+ \
+ : Check for the construct; \
+ if test -n "$$files"; then \
+ if test -n "$$prohibit"; then \
+ echo "$$files" \
+ | xargs $(GREP) $$with_grep_options $(_ignore_case) -nE \
+ "$$prohibit" /dev/null \
+ | $(GREP) -vE "$${exclude:-^$$}" \
+ && { msg="$$halt" $(_sc_say_and_exit) } \
+ || :; \
+ else \
+ echo "$$files" \
+ | xargs \
+ $(GREP) $$with_grep_options $(_ignore_case) -LE "$$require" \
+ | $(GREP) . \
+ && { msg="$$halt" $(_sc_say_and_exit) } \
+ || :; \
+ fi \
+ else :; \
+ fi || :;
+endef
+
+# Perl block to convert a match to FILE_NAME:LINENO:TEST
+perl_filename_lineno_text_ = \
+ -e ' {' \
+ -e ' $$n = ($$` =~ tr/\n/\n/ + 1);' \
+ -e ' ($$v = $$&) =~ s/\n/\\n/g;' \
+ -e ' print "$$ARGV:$$n:$$v\n";' \
+ -e ' }'
diff --git a/tests/style.mak b/tests/style.mak
new file mode 100644
index 0000000000..32c7e706ba
--- /dev/null
+++ b/tests/style.mak
@@ -0,0 +1,24 @@
+# -*- makefile -*-
+#
+# Rules for simple code style checks applying across the
+# whole tree. Partially derived from GNULIB's 'maint.mk'
+#
+# Copyright (C) 2008-2019 Red Hat, Inc.
+# Copyright (C) 2003-2019 Free Software Foundation, Inc.
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see
+# <http://www.gnu.org/licenses/>.
+
+include tests/style-infra.mak
+include tests/style-excludes.mak
--
2.36.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 1/7] tests: introduce tree-wide code style checking
2022-07-04 15:22 ` [PATCH v2 1/7] tests: introduce tree-wide code style checking Daniel P. Berrangé
@ 2022-07-04 15:46 ` Peter Maydell
2022-07-04 16:12 ` Daniel P. Berrangé
2022-07-07 16:43 ` Daniel P. Berrangé
0 siblings, 2 replies; 22+ messages in thread
From: Peter Maydell @ 2022-07-04 15:46 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Paolo Bonzini, Alex Bennée, Thomas Huth,
Philippe Mathieu-Daudé
On Mon, 4 Jul 2022 at 16:23, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> Historically QEMU has used the 'scripts/checkpatch.pl' script to
> validate various style rules but there are a number of issues:
> meson.build | 3 +
> tests/Makefile.include | 3 +-
> tests/meson.build | 19 +++
> tests/style-excludes.mak | 4 +
> tests/style-infra.mak | 265 +++++++++++++++++++++++++++++++++++++++
> tests/style.mak | 24 ++++
From my point of view the main issue with checkpatch.pl is
that nobody in the QEMU developers particularly understands
it or is enthusiastic about trying to add more tests to it
or adjust the existing ones where QEMU style diverges from
the kernel style (but nor are we tracking and upgrading to
newer versions of the kernel's script).
This seems to be aiming to replace a complex and hard to
understand perl script with a complex and hard to understand
makefile. I can't say I'm terribly enthusiastic :-/
thanks
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/7] tests: introduce tree-wide code style checking
2022-07-04 15:46 ` Peter Maydell
@ 2022-07-04 16:12 ` Daniel P. Berrangé
2022-07-07 16:43 ` Daniel P. Berrangé
1 sibling, 0 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2022-07-04 16:12 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-devel, Paolo Bonzini, Alex Bennée, Thomas Huth,
Philippe Mathieu-Daudé
On Mon, Jul 04, 2022 at 04:46:53PM +0100, Peter Maydell wrote:
> On Mon, 4 Jul 2022 at 16:23, Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > Historically QEMU has used the 'scripts/checkpatch.pl' script to
> > validate various style rules but there are a number of issues:
>
> > meson.build | 3 +
> > tests/Makefile.include | 3 +-
> > tests/meson.build | 19 +++
> > tests/style-excludes.mak | 4 +
> > tests/style-infra.mak | 265 +++++++++++++++++++++++++++++++++++++++
> > tests/style.mak | 24 ++++
>
> From my point of view the main issue with checkpatch.pl is
> that nobody in the QEMU developers particularly understands
> it or is enthusiastic about trying to add more tests to it
> or adjust the existing ones where QEMU style diverges from
> the kernel style (but nor are we tracking and upgrading to
> newer versions of the kernel's script).
>
> This seems to be aiming to replace a complex and hard to
> understand perl script with a complex and hard to understand
> makefile. I can't say I'm terribly enthusiastic :-/
I think the downsides comapred here are rather different orders of
magnitude. The checkpatch.pl script is 3000 lines of code where we
have years of experiance that no one in QEMU likes touching it.
The makefile here is 265 lines of which 50% is comments of license
text. In terms of what contributors most care about though, is
how you add new rules, and most of the time that's involves just
adding a 3 line make rule based off a regex to match the code
pattern you want to prohibit. Some of this is a bit crufty to
look at, but we've got years of experiance in libvirt with many
contributors frequently adding new tests.
It only gets hairy if the pattern you're trying to forbid needs
to match across multiple lines of text - hence the difference in
complexity between matching 'osdep.h' exists in .c, vs 'osdep.h'
exists as the very first #include. IME, the single-line matches
are most typical need that is addressed.
So while I wont claim this proposal is perfect, IMHO this would
be a significant step fowards over checkpatch.pl.
With 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 :|
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/7] tests: introduce tree-wide code style checking
2022-07-04 15:46 ` Peter Maydell
2022-07-04 16:12 ` Daniel P. Berrangé
@ 2022-07-07 16:43 ` Daniel P. Berrangé
1 sibling, 0 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2022-07-07 16:43 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-devel, Paolo Bonzini, Alex Bennée, Thomas Huth,
Philippe Mathieu-Daudé
On Mon, Jul 04, 2022 at 04:46:53PM +0100, Peter Maydell wrote:
> On Mon, 4 Jul 2022 at 16:23, Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > Historically QEMU has used the 'scripts/checkpatch.pl' script to
> > validate various style rules but there are a number of issues:
>
> > meson.build | 3 +
> > tests/Makefile.include | 3 +-
> > tests/meson.build | 19 +++
> > tests/style-excludes.mak | 4 +
> > tests/style-infra.mak | 265 +++++++++++++++++++++++++++++++++++++++
> > tests/style.mak | 24 ++++
>
> From my point of view the main issue with checkpatch.pl is
> that nobody in the QEMU developers particularly understands
> it or is enthusiastic about trying to add more tests to it
> or adjust the existing ones where QEMU style diverges from
> the kernel style (but nor are we tracking and upgrading to
> newer versions of the kernel's script).
>
> This seems to be aiming to replace a complex and hard to
> understand perl script with a complex and hard to understand
> makefile. I can't say I'm terribly enthusiastic :-/
Taking that feedback on board, I've proposed a new version which is
written in Python, and uses a plain yaml config file, which I admit
results in an easier to understand and more attractive impl than
this makefile based one.
With 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 :|
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 2/7] misc: fix mixups of bool constants with int variables
2022-07-04 15:22 [PATCH v2 0/7] tests: introduce a tree-wide code style checking facility Daniel P. Berrangé
2022-07-04 15:22 ` [PATCH v2 1/7] tests: introduce tree-wide code style checking Daniel P. Berrangé
@ 2022-07-04 15:22 ` Daniel P. Berrangé
2022-07-04 15:38 ` Peter Maydell
2022-07-04 15:22 ` [PATCH v2 3/7] tests/style: check for " Daniel P. Berrangé
` (4 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrangé @ 2022-07-04 15:22 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Alex Bennée, Thomas Huth, Peter Maydell,
Philippe Mathieu-Daudé, Daniel P. Berrangé
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
block/vhdx-log.c | 2 +-
hw/xtensa/sim.c | 4 ++--
nbd/client.c | 4 ++--
target/i386/cpu-dump.c | 3 ++-
ui/spice-display.c | 4 ++--
5 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index ff0d4e0da0..8f34755a6f 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -215,7 +215,7 @@ exit:
static bool vhdx_log_hdr_is_valid(VHDXLogEntries *log, VHDXLogEntryHeader *hdr,
BDRVVHDXState *s)
{
- int valid = false;
+ bool valid = false;
if (hdr->signature != VHDX_LOG_SIGNATURE) {
goto exit;
diff --git a/hw/xtensa/sim.c b/hw/xtensa/sim.c
index 946c71cb5b..70fce7fb85 100644
--- a/hw/xtensa/sim.c
+++ b/hw/xtensa/sim.c
@@ -97,9 +97,9 @@ void xtensa_sim_load_kernel(XtensaCPU *cpu, MachineState *machine)
{
const char *kernel_filename = machine->kernel_filename;
#if TARGET_BIG_ENDIAN
- int big_endian = true;
+ int big_endian = 1;
#else
- int big_endian = false;
+ int big_endian = 0;
#endif
if (kernel_filename) {
diff --git a/nbd/client.c b/nbd/client.c
index 30d5383cb1..fee3959d24 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -832,8 +832,8 @@ static int nbd_list_meta_contexts(QIOChannel *ioc,
Error **errp)
{
int ret;
- int seen_any = false;
- int seen_qemu = false;
+ bool seen_any = false;
+ bool seen_qemu = false;
if (nbd_send_meta_query(ioc, NBD_OPT_LIST_META_CONTEXT,
info->name, NULL, errp) < 0) {
diff --git a/target/i386/cpu-dump.c b/target/i386/cpu-dump.c
index 08ac957e99..43521c74c8 100644
--- a/target/i386/cpu-dump.c
+++ b/target/i386/cpu-dump.c
@@ -275,7 +275,8 @@ static void dump_apic_icr(APICCommonState *s, CPUX86State *env)
static void dump_apic_interrupt(const char *name, uint32_t *ireg_tab,
uint32_t *tmr_tab)
{
- int i, empty = true;
+ int i;
+ bool empty = true;
qemu_printf("%s\t ", name);
for (i = 0; i < 256; i++) {
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 494168e7fe..5d3b64413f 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -541,14 +541,14 @@ static int interface_get_command(QXLInstance *sin, QXLCommandExt *ext)
{
SimpleSpiceDisplay *ssd = container_of(sin, SimpleSpiceDisplay, qxl);
SimpleSpiceUpdate *update;
- int ret = false;
+ int ret = 0;
qemu_mutex_lock(&ssd->lock);
update = QTAILQ_FIRST(&ssd->updates);
if (update != NULL) {
QTAILQ_REMOVE(&ssd->updates, update, next);
*ext = update->ext;
- ret = true;
+ ret = 1;
}
qemu_mutex_unlock(&ssd->lock);
--
2.36.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 2/7] misc: fix mixups of bool constants with int variables
2022-07-04 15:22 ` [PATCH v2 2/7] misc: fix mixups of bool constants with int variables Daniel P. Berrangé
@ 2022-07-04 15:38 ` Peter Maydell
0 siblings, 0 replies; 22+ messages in thread
From: Peter Maydell @ 2022-07-04 15:38 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Paolo Bonzini, Alex Bennée, Thomas Huth,
Philippe Mathieu-Daudé
On Mon, 4 Jul 2022 at 16:23, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> block/vhdx-log.c | 2 +-
> hw/xtensa/sim.c | 4 ++--
> nbd/client.c | 4 ++--
> target/i386/cpu-dump.c | 3 ++-
> ui/spice-display.c | 4 ++--
> 5 files changed, 9 insertions(+), 8 deletions(-)
> diff --git a/nbd/client.c b/nbd/client.c
> index 30d5383cb1..fee3959d24 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -832,8 +832,8 @@ static int nbd_list_meta_contexts(QIOChannel *ioc,
> Error **errp)
> {
> int ret;
> - int seen_any = false;
> - int seen_qemu = false;
> + bool seen_any = false;
> + bool seen_qemu = false;
>
> if (nbd_send_meta_query(ioc, NBD_OPT_LIST_META_CONTEXT,
> info->name, NULL, errp) < 0) {
The code in this function does a bitwise OR into seen_qemu later --
bitwise OR into a bool also seems like bad style that something
is probably going to complain about. (I guess currently -Wbool-operation
doesn't care about it or else we don't enable that, but it might
in future.)
The other changes look OK.
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 3/7] tests/style: check for mixups of bool constants with int variables
2022-07-04 15:22 [PATCH v2 0/7] tests: introduce a tree-wide code style checking facility Daniel P. Berrangé
2022-07-04 15:22 ` [PATCH v2 1/7] tests: introduce tree-wide code style checking Daniel P. Berrangé
2022-07-04 15:22 ` [PATCH v2 2/7] misc: fix mixups of bool constants with int variables Daniel P. Berrangé
@ 2022-07-04 15:22 ` Daniel P. Berrangé
2022-07-04 15:23 ` [PATCH v2 4/7] misc: fix commonly doubled up words Daniel P. Berrangé
` (3 subsequent siblings)
6 siblings, 0 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2022-07-04 15:22 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Alex Bennée, Thomas Huth, Peter Maydell,
Philippe Mathieu-Daudé, Daniel P. Berrangé
The 'true' and 'false' constants should only ever be used with the
'bool' type, never 'int'.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
tests/style.mak | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/tests/style.mak b/tests/style.mak
index 32c7e706ba..ae658395c9 100644
--- a/tests/style.mak
+++ b/tests/style.mak
@@ -22,3 +22,9 @@
include tests/style-infra.mak
include tests/style-excludes.mak
+
+# Use 'bool', not 'int', when assigning true or false
+sc_int_assign_bool:
+ @prohibit='\<int\>.*= *(true|false)\b' \
+ halt='use bool type for boolean values' \
+ $(_sc_search_regexp)
--
2.36.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v2 4/7] misc: fix commonly doubled up words
2022-07-04 15:22 [PATCH v2 0/7] tests: introduce a tree-wide code style checking facility Daniel P. Berrangé
` (2 preceding siblings ...)
2022-07-04 15:22 ` [PATCH v2 3/7] tests/style: check for " Daniel P. Berrangé
@ 2022-07-04 15:23 ` Daniel P. Berrangé
2022-07-04 15:52 ` Peter Maydell
2022-07-04 15:23 ` [PATCH v2 5/7] tests/style: check for " Daniel P. Berrangé
` (2 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrangé @ 2022-07-04 15:23 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Alex Bennée, Thomas Huth, Peter Maydell,
Philippe Mathieu-Daudé, Daniel P. Berrangé
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
block/linux-aio.c | 2 +-
block/qcow2-bitmap.c | 8 ++++----
contrib/plugins/cache.c | 2 +-
disas/libvixl/vixl/invalset.h | 2 +-
docs/devel/qom.rst | 4 ++--
docs/interop/live-block-operations.rst | 4 ++--
docs/system/arm/cpu-features.rst | 2 +-
docs/system/devices/cxl.rst | 2 +-
docs/system/s390x/bootdevices.rst | 2 +-
docs/system/tls.rst | 2 +-
docs/tools/qemu-pr-helper.rst | 2 +-
hw/core/clock.c | 2 +-
hw/intc/arm_gicv3_redist.c | 2 +-
hw/misc/iotkit-secctl.c | 2 +-
hw/misc/iotkit-sysctl.c | 4 ++--
hw/s390x/s390-ccw.c | 2 +-
hw/usb/u2f.h | 2 +-
include/hw/qdev-core.h | 2 +-
include/user/safe-syscall.h | 2 +-
linux-user/i386/cpu_loop.c | 2 +-
pc-bios/s390-ccw/virtio-scsi.c | 2 +-
python/Makefile | 2 +-
python/qemu/utils/__init__.py | 2 +-
target/arm/translate.c | 2 +-
target/i386/cpu.c | 2 +-
tcg/i386/tcg-target.c.inc | 2 +-
tests/qtest/microbit-test.c | 4 ++--
tools/virtiofsd/fuse_virtio.c | 2 +-
28 files changed, 35 insertions(+), 35 deletions(-)
diff --git a/block/linux-aio.c b/block/linux-aio.c
index 9c2393a2f7..d2cfb7f523 100644
--- a/block/linux-aio.c
+++ b/block/linux-aio.c
@@ -461,7 +461,7 @@ LinuxAioState *laio_init(Error **errp)
s = g_malloc0(sizeof(*s));
rc = event_notifier_init(&s->e, false);
if (rc < 0) {
- error_setg_errno(errp, -rc, "failed to to initialize event notifier");
+ error_setg_errno(errp, -rc, "failed to initialize event notifier");
goto out_free_state;
}
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 8fb4731551..5529368df4 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -787,10 +787,10 @@ static int bitmap_list_store(BlockDriverState *bs, Qcow2BitmapList *bm_list,
}
}
- /* Actually, even in in-place case ignoring QCOW2_OL_BITMAP_DIRECTORY is not
- * necessary, because we drop QCOW2_AUTOCLEAR_BITMAPS when updating bitmap
- * directory in-place (actually, turn-off the extension), which is checked
- * in qcow2_check_metadata_overlap() */
+ /* Actually, even in the in-place case ignoring QCOW2_OL_BITMAP_DIRECTORY
+ * is not necessary, because we drop QCOW2_AUTOCLEAR_BITMAPS when updating
+ * bitmap directory in-place (actually, turn-off the extension), which is
+ * checked in qcow2_check_metadata_overlap() */
ret = qcow2_pre_write_overlap_check(
bs, in_place ? QCOW2_OL_BITMAP_DIRECTORY : 0, dir_offset, dir_size,
false);
diff --git a/contrib/plugins/cache.c b/contrib/plugins/cache.c
index b9226e7c40..ac1510aaa1 100644
--- a/contrib/plugins/cache.c
+++ b/contrib/plugins/cache.c
@@ -38,7 +38,7 @@ enum EvictionPolicy policy;
* put in any of the blocks inside the set. The number of block per set is
* called the associativity (assoc).
*
- * Each block contains the the stored tag and a valid bit. Since this is not
+ * Each block contains the stored tag and a valid bit. Since this is not
* a functional simulator, the data itself is not stored. We only identify
* whether a block is in the cache or not by searching for its tag.
*
diff --git a/disas/libvixl/vixl/invalset.h b/disas/libvixl/vixl/invalset.h
index 2e0871f8c3..f5d6c43d81 100644
--- a/disas/libvixl/vixl/invalset.h
+++ b/disas/libvixl/vixl/invalset.h
@@ -102,7 +102,7 @@ template<TEMPLATE_INVALSET_P_DECL> class InvalSet {
size_t size() const;
// Returns true if no elements are stored in the set.
- // Note that this does not mean the the backing storage is empty: it can still
+ // Note that this does not mean the backing storage is empty: it can still
// contain invalid elements.
bool empty() const;
diff --git a/docs/devel/qom.rst b/docs/devel/qom.rst
index e5fe3597cd..62c39c9c88 100644
--- a/docs/devel/qom.rst
+++ b/docs/devel/qom.rst
@@ -372,8 +372,8 @@ This accepts an array of interface type names.
{ TYPE_USER_CREATABLE },
{ NULL })
-If the type is not intended to be instantiated, then then
-the OBJECT_DEFINE_ABSTRACT_TYPE() macro can be used instead:
+If the type is not intended to be instantiated, then the
+OBJECT_DEFINE_ABSTRACT_TYPE() macro can be used instead:
.. code-block:: c
:caption: Defining a simple abstract type
diff --git a/docs/interop/live-block-operations.rst b/docs/interop/live-block-operations.rst
index 39e62c9915..135784ab33 100644
--- a/docs/interop/live-block-operations.rst
+++ b/docs/interop/live-block-operations.rst
@@ -53,7 +53,7 @@ files in a disk image backing chain:
(1) Directional: 'base' and 'top'. Given the simple disk image chain
above, image [A] can be referred to as 'base', and image [B] as
- 'top'. (This terminology can be seen in in QAPI schema file,
+ 'top'. (This terminology can be seen in the QAPI schema file,
block-core.json.)
(2) Relational: 'backing file' and 'overlay'. Again, taking the same
@@ -825,7 +825,7 @@ entire disk image chain, to a target, using ``blockdev-mirror`` would be:
job ready to be completed
(5) Gracefully complete the 'mirror' block device job, and notice the
- the event ``BLOCK_JOB_COMPLETED``
+ event ``BLOCK_JOB_COMPLETED``
(6) Shutdown the guest by issuing the QMP ``quit`` command so that
caches are flushed
diff --git a/docs/system/arm/cpu-features.rst b/docs/system/arm/cpu-features.rst
index 3fd76fa0b4..c2c01ec7d2 100644
--- a/docs/system/arm/cpu-features.rst
+++ b/docs/system/arm/cpu-features.rst
@@ -284,7 +284,7 @@ SVE CPU Property Parsing Semantics
CPU Property Dependencies and Constraints").
4) If one or more vector lengths have been explicitly enabled and at
- at least one of the dependency lengths of the maximum enabled length
+ least one of the dependency lengths of the maximum enabled length
has been explicitly disabled, then an error is generated (see
constraint (2) of "SVE CPU Property Dependencies and Constraints").
diff --git a/docs/system/devices/cxl.rst b/docs/system/devices/cxl.rst
index a57e4c4e5c..36031325cc 100644
--- a/docs/system/devices/cxl.rst
+++ b/docs/system/devices/cxl.rst
@@ -15,7 +15,7 @@ with CXL Host Bridges, which have CXL Root Ports which may be directly
attached to CXL or PCI End Points. Alternatively there may be CXL Switches
with CXL and PCI Endpoints attached below them. In many cases additional
control and capabilities are exposed via PCI Express interfaces.
-This sharing of interfaces and hence emulation code is is reflected
+This sharing of interfaces and hence emulation code is reflected
in how the devices are emulated in QEMU. In most cases the various
CXL elements are built upon an equivalent PCIe devices.
diff --git a/docs/system/s390x/bootdevices.rst b/docs/system/s390x/bootdevices.rst
index 9e591cb9dc..b5950133e8 100644
--- a/docs/system/s390x/bootdevices.rst
+++ b/docs/system/s390x/bootdevices.rst
@@ -65,7 +65,7 @@ you can specify it via the ``-global s390-ipl.netboot_fw=filename``
command line option.
The ``bootindex`` property is especially important for booting via the network.
-If you don't specify the the ``bootindex`` property here, the network bootloader
+If you don't specify the ``bootindex`` property here, the network bootloader
firmware code won't get loaded into the guest memory so that the network boot
will fail. For a successful network boot, try something like this::
diff --git a/docs/system/tls.rst b/docs/system/tls.rst
index 1a04674362..e284c82801 100644
--- a/docs/system/tls.rst
+++ b/docs/system/tls.rst
@@ -182,7 +182,7 @@ certificates.
--template client-hostNNN.info \
--outfile client-hostNNN-cert.pem
-The subject alt name extension data is not required for clients, so the
+The subject alt name extension data is not required for clients, so
the ``dns_name`` and ``ip_address`` fields are not included. The
``tls_www_client`` keyword is the key purpose extension to indicate this
certificate is intended for usage in a web client. Although QEMU network
diff --git a/docs/tools/qemu-pr-helper.rst b/docs/tools/qemu-pr-helper.rst
index eaebe40da0..bbd8b6c3ce 100644
--- a/docs/tools/qemu-pr-helper.rst
+++ b/docs/tools/qemu-pr-helper.rst
@@ -22,7 +22,7 @@ storage fabric. QEMU's SCSI passthrough devices ``scsi-block``
and ``scsi-generic`` support passing guest persistent reservation
requests to a privileged external helper program. :program:`qemu-pr-helper`
is that external helper; it creates a socket which QEMU can
-connect to to communicate with it.
+connect to communicate with it.
If you want to run VMs in a setup like this, this helper should be
started as a system service, and you should read the QEMU manual
diff --git a/hw/core/clock.c b/hw/core/clock.c
index 916875e07a..d82e44cd1a 100644
--- a/hw/core/clock.c
+++ b/hw/core/clock.c
@@ -68,7 +68,7 @@ static uint64_t clock_get_child_period(Clock *clk)
{
/*
* Return the period to be used for child clocks, which is the parent
- * clock period adjusted for for multiplier and divider effects.
+ * clock period adjusted for multiplier and divider effects.
*/
return muldiv64(clk->period, clk->multiplier, clk->divider);
}
diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c
index f1ecb2502b..c92ceecc16 100644
--- a/hw/intc/arm_gicv3_redist.c
+++ b/hw/intc/arm_gicv3_redist.c
@@ -492,7 +492,7 @@ static MemTxResult gicr_writel(GICv3CPUState *cs, hwaddr offset,
return MEMTX_OK;
case GICR_WAKER:
/* Only the ProcessorSleep bit is writable. When the guest sets
- * it it requests that we transition the channel between the
+ * it, it requests that we transition the channel between the
* redistributor and the cpu interface to quiescent, and that
* we set the ChildrenAsleep bit once the inteface has reached the
* quiescent state.
diff --git a/hw/misc/iotkit-secctl.c b/hw/misc/iotkit-secctl.c
index 7b41cfa8fc..b5a9e30a2c 100644
--- a/hw/misc/iotkit-secctl.c
+++ b/hw/misc/iotkit-secctl.c
@@ -114,7 +114,7 @@ static const uint8_t iotkit_secctl_ns_sse300_idregs[] = {
* AHB expansion, APB expansion) are all set up so that they are
* in 16-aligned blocks so offsets 0xN0, 0xN4, 0xN8, 0xNC are PPCs
* 0, 1, 2, 3 of that type, so we can convert a register address offset
- * into an an index into a PPC array easily.
+ * into an index into a PPC array easily.
*/
static inline int offset_to_ppc_idx(uint32_t offset)
{
diff --git a/hw/misc/iotkit-sysctl.c b/hw/misc/iotkit-sysctl.c
index 9ee8fe8495..7147e2f84e 100644
--- a/hw/misc/iotkit-sysctl.c
+++ b/hw/misc/iotkit-sysctl.c
@@ -237,7 +237,7 @@ static uint64_t iotkit_sysctl_read(void *opaque, hwaddr offset,
r = s->ewctrl;
break;
case ARMSSE_SSE300:
- /* In SSE300 this offset is is NMI_ENABLE */
+ /* In SSE300 this offset is NMI_ENABLE */
r = s->nmi_enable;
break;
default:
@@ -555,7 +555,7 @@ static void iotkit_sysctl_write(void *opaque, hwaddr offset,
s->ewctrl = value;
break;
case ARMSSE_SSE300:
- /* In SSE300 this offset is is NMI_ENABLE */
+ /* In SSE300 this offset is NMI_ENABLE */
qemu_log_mask(LOG_UNIMP, "IoTKit SysCtl NMI_ENABLE unimplemented\n");
s->nmi_enable = value;
break;
diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
index 2fc8bb9c23..e2d86d96e7 100644
--- a/hw/s390x/s390-ccw.c
+++ b/hw/s390x/s390-ccw.c
@@ -57,7 +57,7 @@ IOInstEnding s390_ccw_store(SubchDev *sch)
/*
* This code is called for both virtual and passthrough devices,
- * but only applies to to the latter. This ugly check makes that
+ * but only applies to the latter. This ugly check makes that
* distinction for us.
*/
if (object_dynamic_cast(OBJECT(sch->driver_data), TYPE_S390_CCW)) {
diff --git a/hw/usb/u2f.h b/hw/usb/u2f.h
index db30f3586b..a408a82927 100644
--- a/hw/usb/u2f.h
+++ b/hw/usb/u2f.h
@@ -74,7 +74,7 @@ typedef struct U2FKeyState {
/*
* API to be used by the U2F key device variants (i.e. hw/u2f-*.c)
- * to interact with the the U2F key base device (i.e. hw/u2f.c)
+ * to interact with the U2F key base device (i.e. hw/u2f.c)
*/
void u2f_send_to_guest(U2FKeyState *key,
const uint8_t packet[U2FHID_PACKET_SIZE]);
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 98774e2835..785dd5a56e 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -386,7 +386,7 @@ bool qdev_realize_and_unref(DeviceState *dev, BusState *bus, Error **errp);
*
* - unrealize any child buses by calling qbus_unrealize()
* (this will recursively unrealize any devices on those buses)
- * - call the the unrealize method of @dev
+ * - call the unrealize method of @dev
*
* The device can then be freed by causing its reference count to go
* to zero.
diff --git a/include/user/safe-syscall.h b/include/user/safe-syscall.h
index 61a04e2b5a..ddceef12e2 100644
--- a/include/user/safe-syscall.h
+++ b/include/user/safe-syscall.h
@@ -70,7 +70,7 @@
* If the host libc is used then the implementation will appear to work
* most of the time, but there will be a race condition where a
* signal could arrive just before we make the host syscall inside libc,
- * and then then guest syscall will not correctly be interrupted.
+ * and then the guest syscall will not correctly be interrupted.
* Instead the implementation of the guest syscall can use the safe_syscall
* function but otherwise just return the result or errno in the usual
* way; the main loop code will take care of restarting the syscall
diff --git a/linux-user/i386/cpu_loop.c b/linux-user/i386/cpu_loop.c
index 492701dd9a..42837399bc 100644
--- a/linux-user/i386/cpu_loop.c
+++ b/linux-user/i386/cpu_loop.c
@@ -137,7 +137,7 @@ static void emulate_vsyscall(CPUX86State *env)
}
/*
- * Validate the the pointer arguments.
+ * Validate the pointer arguments.
*/
switch (syscall) {
case TARGET_NR_gettimeofday:
diff --git a/pc-bios/s390-ccw/virtio-scsi.c b/pc-bios/s390-ccw/virtio-scsi.c
index 2c8d0f3097..45256b3fdb 100644
--- a/pc-bios/s390-ccw/virtio-scsi.c
+++ b/pc-bios/s390-ccw/virtio-scsi.c
@@ -195,7 +195,7 @@ static bool scsi_read_capacity(VDev *vdev,
/* virtio-scsi routines */
/*
- * Tries to locate a SCSI device and and adds the information for the found
+ * Tries to locate a SCSI device and adds the information for the found
* device to the vdev->scsi_device structure.
* Returns 0 if SCSI device could be located, or a error code < 0 otherwise
*/
diff --git a/python/Makefile b/python/Makefile
index 3334311362..b170708398 100644
--- a/python/Makefile
+++ b/python/Makefile
@@ -29,7 +29,7 @@ help:
@echo " Performs no environment setup of any kind."
@echo ""
@echo "make develop:"
- @echo " Install deps needed for for 'make check',"
+ @echo " Install deps needed for 'make check',"
@echo " and install the qemu package in editable mode."
@echo " (Can be used in or outside of a venv.)"
@echo ""
diff --git a/python/qemu/utils/__init__.py b/python/qemu/utils/__init__.py
index 9fb273b13d..017cfdcda7 100644
--- a/python/qemu/utils/__init__.py
+++ b/python/qemu/utils/__init__.py
@@ -79,7 +79,7 @@ def add_visual_margin(
:param content: The text to wrap and decorate.
:param width:
The number of columns to use, including for the decoration
- itself. The default (None) uses the the available width of the
+ itself. The default (None) uses the available width of the
current terminal, or a fallback of 72 lines. A negative number
subtracts a fixed-width from the default size. The default obeys
the COLUMNS environment variable, if set.
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 6617de775f..73d1faf917 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -8056,7 +8056,7 @@ static TCGv_i32 op_addr_block_pre(DisasContext *s, arg_ldst_block *a, int n)
* If the writeback is incrementing SP rather than
* decrementing it, and the initial SP is below the
* stack limit but the final written-back SP would
- * be above, then then we must not perform any memory
+ * be above, then we must not perform any memory
* accesses, but it is IMPDEF whether we generate
* an exception. We choose to do so in this case.
* At this point 'addr' is the lowest address, so
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 6a57ef13af..194b5a31af 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3632,7 +3632,7 @@ static const X86CPUDefinition builtin_x86_defs[] = {
CPUID_7_0_EDX_CORE_CAPABILITY,
.features[FEAT_CORE_CAPABILITY] =
MSR_CORE_CAP_SPLIT_LOCK_DETECT,
- /* XSAVES is is added in version 3 */
+ /* XSAVES is added in version 3 */
.features[FEAT_XSAVE] =
CPUID_XSAVE_XSAVEOPT | CPUID_XSAVE_XSAVEC |
CPUID_XSAVE_XGETBV1,
diff --git a/tcg/i386/tcg-target.c.inc b/tcg/i386/tcg-target.c.inc
index d52206ba4d..cb04e4b3ad 100644
--- a/tcg/i386/tcg-target.c.inc
+++ b/tcg/i386/tcg-target.c.inc
@@ -1652,7 +1652,7 @@ static void tcg_out_branch(TCGContext *s, int call, const tcg_insn_unit *dest)
} else {
/* rip-relative addressing into the constant pool.
This is 6 + 8 = 14 bytes, as compared to using an
- an immediate load 10 + 6 = 16 bytes, plus we may
+ immediate load 10 + 6 = 16 bytes, plus we may
be able to re-use the pool constant for more calls. */
tcg_out_opc(s, OPC_GRP5, 0, 0, 0);
tcg_out8(s, (call ? EXT5_CALLN_Ev : EXT5_JMPN_Ev) << 3 | 5);
diff --git a/tests/qtest/microbit-test.c b/tests/qtest/microbit-test.c
index 04e199ec33..6cc1f5d932 100644
--- a/tests/qtest/microbit-test.c
+++ b/tests/qtest/microbit-test.c
@@ -449,9 +449,9 @@ static void test_nrf51_timer(void)
timer_set_prescaler(qts, 0);
/* Swept over in first step */
timer_set_cc(qts, 0, 2);
- /* Barely miss on first step */
+ /* Barely miss in first step */
timer_set_cc(qts, 1, 162);
- /* Spot on on third step */
+ /* Spot on in third step */
timer_set_cc(qts, 2, 480);
timer_assert_events(qts, 0, 0, 0, 0);
diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index a52eacf82e..9368e292e4 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -379,7 +379,7 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
copy_iov(iov, count, in_sg, in_num, iov_len);
/*
- * Build a copy of the the in_sg iov so we can skip bits in it,
+ * Build a copy of the in_sg iov so we can skip bits in it,
* including changing the offsets
*/
in_sg_cpy = g_new(struct iovec, in_num);
--
2.36.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 4/7] misc: fix commonly doubled up words
2022-07-04 15:23 ` [PATCH v2 4/7] misc: fix commonly doubled up words Daniel P. Berrangé
@ 2022-07-04 15:52 ` Peter Maydell
2022-07-07 12:30 ` Daniel P. Berrangé
0 siblings, 1 reply; 22+ messages in thread
From: Peter Maydell @ 2022-07-04 15:52 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Paolo Bonzini, Alex Bennée, Thomas Huth,
Philippe Mathieu-Daudé
On Mon, 4 Jul 2022 at 16:23, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> --- a/docs/tools/qemu-pr-helper.rst
> +++ b/docs/tools/qemu-pr-helper.rst
> @@ -22,7 +22,7 @@ storage fabric. QEMU's SCSI passthrough devices ``scsi-block``
> and ``scsi-generic`` support passing guest persistent reservation
> requests to a privileged external helper program. :program:`qemu-pr-helper`
> is that external helper; it creates a socket which QEMU can
> -connect to to communicate with it.
> +connect to communicate with it.
This text is correct as it stands, and the change is wrong.
> diff --git a/tests/qtest/microbit-test.c b/tests/qtest/microbit-test.c
> index 04e199ec33..6cc1f5d932 100644
> --- a/tests/qtest/microbit-test.c
> +++ b/tests/qtest/microbit-test.c
> @@ -449,9 +449,9 @@ static void test_nrf51_timer(void)
> timer_set_prescaler(qts, 0);
> /* Swept over in first step */
> timer_set_cc(qts, 0, 2);
> - /* Barely miss on first step */
> + /* Barely miss in first step */
> timer_set_cc(qts, 1, 162);
> - /* Spot on on third step */
> + /* Spot on in third step */
> timer_set_cc(qts, 2, 480);
These changes also look wrong.
The rest seems OK.
thanks
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/7] misc: fix commonly doubled up words
2022-07-04 15:52 ` Peter Maydell
@ 2022-07-07 12:30 ` Daniel P. Berrangé
2022-07-07 12:35 ` Peter Maydell
0 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrangé @ 2022-07-07 12:30 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-devel, Paolo Bonzini, Alex Bennée, Thomas Huth,
Philippe Mathieu-Daudé
On Mon, Jul 04, 2022 at 04:52:40PM +0100, Peter Maydell wrote:
> On Mon, 4 Jul 2022 at 16:23, Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
>
> > --- a/docs/tools/qemu-pr-helper.rst
> > +++ b/docs/tools/qemu-pr-helper.rst
> > @@ -22,7 +22,7 @@ storage fabric. QEMU's SCSI passthrough devices ``scsi-block``
> > and ``scsi-generic`` support passing guest persistent reservation
> > requests to a privileged external helper program. :program:`qemu-pr-helper`
> > is that external helper; it creates a socket which QEMU can
> > -connect to to communicate with it.
> > +connect to communicate with it.
>
> This text is correct as it stands, and the change is wrong.
I think the current text is a rather awkward, so I'll rephrase it
next time.
> > diff --git a/tests/qtest/microbit-test.c b/tests/qtest/microbit-test.c
> > index 04e199ec33..6cc1f5d932 100644
> > --- a/tests/qtest/microbit-test.c
> > +++ b/tests/qtest/microbit-test.c
> > @@ -449,9 +449,9 @@ static void test_nrf51_timer(void)
> > timer_set_prescaler(qts, 0);
> > /* Swept over in first step */
> > timer_set_cc(qts, 0, 2);
> > - /* Barely miss on first step */
> > + /* Barely miss in first step */
> > timer_set_cc(qts, 1, 162);
> > - /* Spot on on third step */
> > + /* Spot on in third step */
> > timer_set_cc(qts, 2, 480);
>
> These changes also look wrong.
It makes them consistent wth the first comment "Swept over in first step"
Also that 'Barely miss in first step' ought to say 'secound step'
With 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 :|
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/7] misc: fix commonly doubled up words
2022-07-07 12:30 ` Daniel P. Berrangé
@ 2022-07-07 12:35 ` Peter Maydell
0 siblings, 0 replies; 22+ messages in thread
From: Peter Maydell @ 2022-07-07 12:35 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Paolo Bonzini, Alex Bennée, Thomas Huth,
Philippe Mathieu-Daudé
On Thu, 7 Jul 2022 at 13:30, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Mon, Jul 04, 2022 at 04:52:40PM +0100, Peter Maydell wrote:
> > On Mon, 4 Jul 2022 at 16:23, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > >
> > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > ---
> >
> > > --- a/docs/tools/qemu-pr-helper.rst
> > > +++ b/docs/tools/qemu-pr-helper.rst
> > > @@ -22,7 +22,7 @@ storage fabric. QEMU's SCSI passthrough devices ``scsi-block``
> > > and ``scsi-generic`` support passing guest persistent reservation
> > > requests to a privileged external helper program. :program:`qemu-pr-helper`
> > > is that external helper; it creates a socket which QEMU can
> > > -connect to to communicate with it.
> > > +connect to communicate with it.
> >
> > This text is correct as it stands, and the change is wrong.
>
> I think the current text is a rather awkward, so I'll rephrase it
> next time.
>
> > > diff --git a/tests/qtest/microbit-test.c b/tests/qtest/microbit-test.c
> > > index 04e199ec33..6cc1f5d932 100644
> > > --- a/tests/qtest/microbit-test.c
> > > +++ b/tests/qtest/microbit-test.c
> > > @@ -449,9 +449,9 @@ static void test_nrf51_timer(void)
> > > timer_set_prescaler(qts, 0);
> > > /* Swept over in first step */
> > > timer_set_cc(qts, 0, 2);
> > > - /* Barely miss on first step */
> > > + /* Barely miss in first step */
> > > timer_set_cc(qts, 1, 162);
> > > - /* Spot on on third step */
> > > + /* Spot on in third step */
> > > timer_set_cc(qts, 2, 480);
> >
> > These changes also look wrong.
>
> It makes them consistent wth the first comment "Swept over in first step"
There's a difference between "in step X" -- during the duration
of the step, and "on step X" -- at the point where the step stops.
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 5/7] tests/style: check for commonly doubled up words
2022-07-04 15:22 [PATCH v2 0/7] tests: introduce a tree-wide code style checking facility Daniel P. Berrangé
` (3 preceding siblings ...)
2022-07-04 15:23 ` [PATCH v2 4/7] misc: fix commonly doubled up words Daniel P. Berrangé
@ 2022-07-04 15:23 ` Daniel P. Berrangé
2022-07-04 15:23 ` [PATCH v2 6/7] misc: ensure qemu/osdep.h is included in all .c files Daniel P. Berrangé
2022-07-04 15:23 ` [PATCH v2 7/7] tests/style: check " Daniel P. Berrangé
6 siblings, 0 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2022-07-04 15:23 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Alex Bennée, Thomas Huth, Peter Maydell,
Philippe Mathieu-Daudé, Daniel P. Berrangé
This style check looks for cases where the words
the then in an on if is it but for or at and do to
are repeated in a sentence. It doesn't use the simple regex match logic
because it needs to match repeats across lines, so has a custom crafted
rule.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
tests/style-excludes.mak | 12 ++++++++++++
tests/style.mak | 24 ++++++++++++++++++++++++
2 files changed, 36 insertions(+)
diff --git a/tests/style-excludes.mak b/tests/style-excludes.mak
index 32c0e9c328..931dd03a64 100644
--- a/tests/style-excludes.mak
+++ b/tests/style-excludes.mak
@@ -2,3 +2,15 @@
#
# Filenames that should be excluded from specific
# style checks performed by style.mak
+
+exclude_file_name_regexp--sc_prohibit_doubled_word = \
+ disas/sparc\.c \
+ hw/char/terminal3270\.c \
+ include/crypto/afsplit\.h \
+ qemu-options\.hx \
+ scripts/checkpatch\.pl \
+ target/s390x/tcg/insn-data\.def \
+ pc-bios/slof\.bin \
+ tests/qemu-iotests/142(\.out)? \
+ tests/qtest/arm-cpu-features\.c \
+ ui/cursor\.c
diff --git a/tests/style.mak b/tests/style.mak
index ae658395c9..4056bde619 100644
--- a/tests/style.mak
+++ b/tests/style.mak
@@ -28,3 +28,27 @@ sc_int_assign_bool:
@prohibit='\<int\>.*= *(true|false)\b' \
halt='use bool type for boolean values' \
$(_sc_search_regexp)
+
+prohibit_doubled_words_ = \
+ the then in an on if is it but for or at and do to can
+# expand the regex before running the check to avoid using expensive captures
+prohibit_doubled_word_expanded_ = \
+ $(join $(prohibit_doubled_words_),$(addprefix \s+,$(prohibit_doubled_words_)))
+prohibit_doubled_word_RE_ ?= \
+ /\b(?:$(subst $(_sp),|,$(prohibit_doubled_word_expanded_)))\b/gims
+prohibit_doubled_word_ = \
+ -e 'while ($(prohibit_doubled_word_RE_))' \
+ $(perl_filename_lineno_text_)
+
+# Define this to a regular expression that matches
+# any filename:dd:match lines you want to ignore.
+# The default is to ignore no matches.
+ignore_doubled_word_match_RE_ ?= ^$$
+
+sc_prohibit_doubled_word:
+ @$(VC_LIST_EXCEPT) \
+ | xargs perl -n -0777 $(prohibit_doubled_word_) \
+ | $(GREP) -vE '$(ignore_doubled_word_match_RE_)' \
+ | $(GREP) . \
+ && { echo '$(ME): doubled words' 1>&2; exit 1; } \
+ || :
--
2.36.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v2 6/7] misc: ensure qemu/osdep.h is included in all .c files
2022-07-04 15:22 [PATCH v2 0/7] tests: introduce a tree-wide code style checking facility Daniel P. Berrangé
` (4 preceding siblings ...)
2022-07-04 15:23 ` [PATCH v2 5/7] tests/style: check for " Daniel P. Berrangé
@ 2022-07-04 15:23 ` Daniel P. Berrangé
2022-07-04 15:38 ` Warner Losh
2022-07-04 15:23 ` [PATCH v2 7/7] tests/style: check " Daniel P. Berrangé
6 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrangé @ 2022-07-04 15:23 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Alex Bennée, Thomas Huth, Peter Maydell,
Philippe Mathieu-Daudé, Daniel P. Berrangé
A few files relied on qemu/osdep.h being included via a common
header. Another file didn't need it because it was actually an
included file, so ought to have been named .c.inc
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
bsd-user/arm/signal.c | 2 ++
bsd-user/arm/target_arch_cpu.c | 3 +++
bsd-user/{elfcore.c => elfcore.c.inc} | 0
bsd-user/elfload.c | 2 +-
bsd-user/freebsd/os-sys.c | 2 ++
bsd-user/i386/signal.c | 2 ++
bsd-user/qemu.h | 1 -
bsd-user/x86_64/signal.c | 2 ++
crypto/rsakey.c | 1 +
qga/cutils.c | 2 ++
10 files changed, 15 insertions(+), 2 deletions(-)
rename bsd-user/{elfcore.c => elfcore.c.inc} (100%)
diff --git a/bsd-user/arm/signal.c b/bsd-user/arm/signal.c
index 2b1dd745d1..eca20ac4d7 100644
--- a/bsd-user/arm/signal.c
+++ b/bsd-user/arm/signal.c
@@ -17,6 +17,8 @@
* along with this program; if not, see <http://www.gnu.org/licenses/>.
*/
+#include "qemu/osdep.h"
+
#include "qemu.h"
/*
diff --git a/bsd-user/arm/target_arch_cpu.c b/bsd-user/arm/target_arch_cpu.c
index 02bf9149d5..186cf43fb9 100644
--- a/bsd-user/arm/target_arch_cpu.c
+++ b/bsd-user/arm/target_arch_cpu.c
@@ -16,6 +16,9 @@
* You should have received a copy of the GNU General Public License
* along with this program; if not, see <http://www.gnu.org/licenses/>.
*/
+
+#include "qemu/osdep.h"
+
#include "target_arch.h"
void target_cpu_set_tls(CPUARMState *env, target_ulong newtls)
diff --git a/bsd-user/elfcore.c b/bsd-user/elfcore.c.inc
similarity index 100%
rename from bsd-user/elfcore.c
rename to bsd-user/elfcore.c.inc
diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
index f8edb22f2a..1717a454dc 100644
--- a/bsd-user/elfload.c
+++ b/bsd-user/elfload.c
@@ -121,7 +121,7 @@ static void bswap_note(struct elf_note *en) { }
#endif /* ! BSWAP_NEEDED */
-#include "elfcore.c"
+#include "elfcore.c.inc"
/*
* 'copy_elf_strings()' copies argument/envelope strings from user
diff --git a/bsd-user/freebsd/os-sys.c b/bsd-user/freebsd/os-sys.c
index 309e27b9d6..1eab1be6f6 100644
--- a/bsd-user/freebsd/os-sys.c
+++ b/bsd-user/freebsd/os-sys.c
@@ -17,6 +17,8 @@
* along with this program; if not, see <http://www.gnu.org/licenses/>.
*/
+#include "qemu/osdep.h"
+
#include "qemu.h"
#include "target_arch_sysarch.h"
diff --git a/bsd-user/i386/signal.c b/bsd-user/i386/signal.c
index 5dd975ce56..db5b774213 100644
--- a/bsd-user/i386/signal.c
+++ b/bsd-user/i386/signal.c
@@ -17,6 +17,8 @@
* along with this program; if not, see <http://www.gnu.org/licenses/>.
*/
+#include "qemu/osdep.h"
+
#include "qemu.h"
/*
diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
index be6105385e..0ceecfb6df 100644
--- a/bsd-user/qemu.h
+++ b/bsd-user/qemu.h
@@ -17,7 +17,6 @@
#ifndef QEMU_H
#define QEMU_H
-#include "qemu/osdep.h"
#include "cpu.h"
#include "qemu/units.h"
#include "exec/cpu_ldst.h"
diff --git a/bsd-user/x86_64/signal.c b/bsd-user/x86_64/signal.c
index c3875bc4c6..217f9ceb66 100644
--- a/bsd-user/x86_64/signal.c
+++ b/bsd-user/x86_64/signal.c
@@ -16,6 +16,8 @@
* along with this program; if not, see <http://www.gnu.org/licenses/>.
*/
+#include "qemu/osdep.h"
+
#include "qemu.h"
/*
diff --git a/crypto/rsakey.c b/crypto/rsakey.c
index cc40e072f0..dcdbd9ec57 100644
--- a/crypto/rsakey.c
+++ b/crypto/rsakey.c
@@ -19,6 +19,7 @@
*
*/
+#include "qemu/osdep.h"
#include "rsakey.h"
void qcrypto_akcipher_rsakey_free(QCryptoAkCipherRSAKey *rsa_key)
diff --git a/qga/cutils.c b/qga/cutils.c
index b8e142ef64..c53dd418c7 100644
--- a/qga/cutils.c
+++ b/qga/cutils.c
@@ -2,6 +2,8 @@
* This work is licensed under the terms of the GNU GPL, version 2 or later.
* See the COPYING file in the top-level directory.
*/
+
+#include "qemu/osdep.h"
#include "cutils.h"
#include "qapi/error.h"
--
2.36.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 6/7] misc: ensure qemu/osdep.h is included in all .c files
2022-07-04 15:23 ` [PATCH v2 6/7] misc: ensure qemu/osdep.h is included in all .c files Daniel P. Berrangé
@ 2022-07-04 15:38 ` Warner Losh
2022-07-04 15:46 ` Daniel P. Berrangé
0 siblings, 1 reply; 22+ messages in thread
From: Warner Losh @ 2022-07-04 15:38 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: QEMU Developers, Paolo Bonzini, Alex Bennée, Thomas Huth,
Peter Maydell, Philippe Mathieu-Daudé
[-- Attachment #1: Type: text/plain, Size: 4934 bytes --]
On Mon, Jul 4, 2022 at 9:28 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:
> A few files relied on qemu/osdep.h being included via a common
> header. Another file didn't need it because it was actually an
> included file, so ought to have been named .c.inc
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> bsd-user/arm/signal.c | 2 ++
> bsd-user/arm/target_arch_cpu.c | 3 +++
> bsd-user/{elfcore.c => elfcore.c.inc} | 0
> bsd-user/elfload.c | 2 +-
> bsd-user/freebsd/os-sys.c | 2 ++
> bsd-user/i386/signal.c | 2 ++
> bsd-user/qemu.h | 1 -
> bsd-user/x86_64/signal.c | 2 ++
> crypto/rsakey.c | 1 +
> qga/cutils.c | 2 ++
> 10 files changed, 15 insertions(+), 2 deletions(-)
> rename bsd-user/{elfcore.c => elfcore.c.inc} (100%)
>
The change to bsd-user is fine, though will cause many ripples in the
upstream
branch when I merge it. The ripples likely are worth it in the long run,
and knowing
they are coming and helps me prepare the tree for the merge.
It also reminds me that once I'm done upstreaming, there's likely benefit
from having
a common elf loader / core generator as much of this code is copied from
linux-user
with the qemu style layered on top....
Reviewed-by: Warner Losh <imp@bsdimp.com>
> diff --git a/bsd-user/arm/signal.c b/bsd-user/arm/signal.c
> index 2b1dd745d1..eca20ac4d7 100644
> --- a/bsd-user/arm/signal.c
> +++ b/bsd-user/arm/signal.c
> @@ -17,6 +17,8 @@
> * along with this program; if not, see <http://www.gnu.org/licenses/>.
> */
>
> +#include "qemu/osdep.h"
> +
> #include "qemu.h"
>
> /*
> diff --git a/bsd-user/arm/target_arch_cpu.c
> b/bsd-user/arm/target_arch_cpu.c
> index 02bf9149d5..186cf43fb9 100644
> --- a/bsd-user/arm/target_arch_cpu.c
> +++ b/bsd-user/arm/target_arch_cpu.c
> @@ -16,6 +16,9 @@
> * You should have received a copy of the GNU General Public License
> * along with this program; if not, see <http://www.gnu.org/licenses/>.
> */
> +
> +#include "qemu/osdep.h"
> +
> #include "target_arch.h"
>
> void target_cpu_set_tls(CPUARMState *env, target_ulong newtls)
> diff --git a/bsd-user/elfcore.c b/bsd-user/elfcore.c.inc
> similarity index 100%
> rename from bsd-user/elfcore.c
> rename to bsd-user/elfcore.c.inc
> diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
> index f8edb22f2a..1717a454dc 100644
> --- a/bsd-user/elfload.c
> +++ b/bsd-user/elfload.c
> @@ -121,7 +121,7 @@ static void bswap_note(struct elf_note *en) { }
>
> #endif /* ! BSWAP_NEEDED */
>
> -#include "elfcore.c"
> +#include "elfcore.c.inc"
>
> /*
> * 'copy_elf_strings()' copies argument/envelope strings from user
> diff --git a/bsd-user/freebsd/os-sys.c b/bsd-user/freebsd/os-sys.c
> index 309e27b9d6..1eab1be6f6 100644
> --- a/bsd-user/freebsd/os-sys.c
> +++ b/bsd-user/freebsd/os-sys.c
> @@ -17,6 +17,8 @@
> * along with this program; if not, see <http://www.gnu.org/licenses/>.
> */
>
> +#include "qemu/osdep.h"
> +
> #include "qemu.h"
> #include "target_arch_sysarch.h"
>
> diff --git a/bsd-user/i386/signal.c b/bsd-user/i386/signal.c
> index 5dd975ce56..db5b774213 100644
> --- a/bsd-user/i386/signal.c
> +++ b/bsd-user/i386/signal.c
> @@ -17,6 +17,8 @@
> * along with this program; if not, see <http://www.gnu.org/licenses/>.
> */
>
> +#include "qemu/osdep.h"
> +
> #include "qemu.h"
>
> /*
> diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
> index be6105385e..0ceecfb6df 100644
> --- a/bsd-user/qemu.h
> +++ b/bsd-user/qemu.h
> @@ -17,7 +17,6 @@
> #ifndef QEMU_H
> #define QEMU_H
>
> -#include "qemu/osdep.h"
> #include "cpu.h"
> #include "qemu/units.h"
> #include "exec/cpu_ldst.h"
> diff --git a/bsd-user/x86_64/signal.c b/bsd-user/x86_64/signal.c
> index c3875bc4c6..217f9ceb66 100644
> --- a/bsd-user/x86_64/signal.c
> +++ b/bsd-user/x86_64/signal.c
> @@ -16,6 +16,8 @@
> * along with this program; if not, see <http://www.gnu.org/licenses/>.
> */
>
> +#include "qemu/osdep.h"
> +
> #include "qemu.h"
>
> /*
> diff --git a/crypto/rsakey.c b/crypto/rsakey.c
> index cc40e072f0..dcdbd9ec57 100644
> --- a/crypto/rsakey.c
> +++ b/crypto/rsakey.c
> @@ -19,6 +19,7 @@
> *
> */
>
> +#include "qemu/osdep.h"
> #include "rsakey.h"
>
> void qcrypto_akcipher_rsakey_free(QCryptoAkCipherRSAKey *rsa_key)
> diff --git a/qga/cutils.c b/qga/cutils.c
> index b8e142ef64..c53dd418c7 100644
> --- a/qga/cutils.c
> +++ b/qga/cutils.c
> @@ -2,6 +2,8 @@
> * This work is licensed under the terms of the GNU GPL, version 2 or
> later.
> * See the COPYING file in the top-level directory.
> */
> +
> +#include "qemu/osdep.h"
> #include "cutils.h"
>
> #include "qapi/error.h"
> --
> 2.36.1
>
>
>
[-- Attachment #2: Type: text/html, Size: 6673 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 6/7] misc: ensure qemu/osdep.h is included in all .c files
2022-07-04 15:38 ` Warner Losh
@ 2022-07-04 15:46 ` Daniel P. Berrangé
2022-07-04 16:08 ` Warner Losh
0 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrangé @ 2022-07-04 15:46 UTC (permalink / raw)
To: Warner Losh
Cc: QEMU Developers, Paolo Bonzini, Alex Bennée, Thomas Huth,
Peter Maydell, Philippe Mathieu-Daudé
On Mon, Jul 04, 2022 at 09:38:46AM -0600, Warner Losh wrote:
> On Mon, Jul 4, 2022 at 9:28 AM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
>
> > A few files relied on qemu/osdep.h being included via a common
> > header. Another file didn't need it because it was actually an
> > included file, so ought to have been named .c.inc
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> > bsd-user/arm/signal.c | 2 ++
> > bsd-user/arm/target_arch_cpu.c | 3 +++
> > bsd-user/{elfcore.c => elfcore.c.inc} | 0
> > bsd-user/elfload.c | 2 +-
> > bsd-user/freebsd/os-sys.c | 2 ++
> > bsd-user/i386/signal.c | 2 ++
> > bsd-user/qemu.h | 1 -
> > bsd-user/x86_64/signal.c | 2 ++
> > crypto/rsakey.c | 1 +
> > qga/cutils.c | 2 ++
> > 10 files changed, 15 insertions(+), 2 deletions(-)
> > rename bsd-user/{elfcore.c => elfcore.c.inc} (100%)
> >
>
> The change to bsd-user is fine, though will cause many ripples in the
> upstream
> branch when I merge it. The ripples likely are worth it in the long run,
> and knowing
> they are coming and helps me prepare the tree for the merge.
If you prefer to delay these changes I don't mind. It just means that
it would need a 'bsd-user/.*' exclude rule in the next patch to
temporarily skip this chck for bsd-user code.
> It also reminds me that once I'm done upstreaming, there's likely benefit
> from having
> a common elf loader / core generator as much of this code is copied from
> linux-user
> with the qemu style layered on top....
>
> Reviewed-by: Warner Losh <imp@bsdimp.com>
With 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 :|
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 6/7] misc: ensure qemu/osdep.h is included in all .c files
2022-07-04 15:46 ` Daniel P. Berrangé
@ 2022-07-04 16:08 ` Warner Losh
0 siblings, 0 replies; 22+ messages in thread
From: Warner Losh @ 2022-07-04 16:08 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: QEMU Developers, Paolo Bonzini, Alex Bennée, Thomas Huth,
Peter Maydell, Philippe Mathieu-Daudé
[-- Attachment #1: Type: text/plain, Size: 2343 bytes --]
On Mon, Jul 4, 2022, 9:46 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Mon, Jul 04, 2022 at 09:38:46AM -0600, Warner Losh wrote:
> > On Mon, Jul 4, 2022 at 9:28 AM Daniel P. Berrangé <berrange@redhat.com>
> > wrote:
> >
> > > A few files relied on qemu/osdep.h being included via a common
> > > header. Another file didn't need it because it was actually an
> > > included file, so ought to have been named .c.inc
> > >
> > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > ---
> > > bsd-user/arm/signal.c | 2 ++
> > > bsd-user/arm/target_arch_cpu.c | 3 +++
> > > bsd-user/{elfcore.c => elfcore.c.inc} | 0
> > > bsd-user/elfload.c | 2 +-
> > > bsd-user/freebsd/os-sys.c | 2 ++
> > > bsd-user/i386/signal.c | 2 ++
> > > bsd-user/qemu.h | 1 -
> > > bsd-user/x86_64/signal.c | 2 ++
> > > crypto/rsakey.c | 1 +
> > > qga/cutils.c | 2 ++
> > > 10 files changed, 15 insertions(+), 2 deletions(-)
> > > rename bsd-user/{elfcore.c => elfcore.c.inc} (100%)
> > >
> >
> > The change to bsd-user is fine, though will cause many ripples in the
> > upstream
> > branch when I merge it. The ripples likely are worth it in the long run,
> > and knowing
> > they are coming and helps me prepare the tree for the merge.
>
> If you prefer to delay these changes I don't mind. It just means that
> it would need a 'bsd-user/.*' exclude rule in the next patch to
> temporarily skip this chck for bsd-user code.
>
No. Go ahead. I know I need to do something when I do the next merge.
And it shouldn't be a big deal to do now, otherwise I'll forget...
Warner
> It also reminds me that once I'm done upstreaming, there's likely benefit
> > from having
> > a common elf loader / core generator as much of this code is copied from
> > linux-user
> > with the qemu style layered on top....
> >
> > Reviewed-by: Warner Losh <imp@bsdimp.com>
>
>
> With 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 :|
>
>
[-- Attachment #2: Type: text/html, Size: 4126 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 7/7] tests/style: check qemu/osdep.h is included in all .c files
2022-07-04 15:22 [PATCH v2 0/7] tests: introduce a tree-wide code style checking facility Daniel P. Berrangé
` (5 preceding siblings ...)
2022-07-04 15:23 ` [PATCH v2 6/7] misc: ensure qemu/osdep.h is included in all .c files Daniel P. Berrangé
@ 2022-07-04 15:23 ` Daniel P. Berrangé
2022-07-04 15:47 ` Peter Maydell
6 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrangé @ 2022-07-04 15:23 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Alex Bennée, Thomas Huth, Peter Maydell,
Philippe Mathieu-Daudé, Daniel P. Berrangé
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
tests/style-excludes.mak | 17 +++++++++++++++++
tests/style.mak | 6 ++++++
2 files changed, 23 insertions(+)
diff --git a/tests/style-excludes.mak b/tests/style-excludes.mak
index 931dd03a64..df158e1d9d 100644
--- a/tests/style-excludes.mak
+++ b/tests/style-excludes.mak
@@ -14,3 +14,20 @@ exclude_file_name_regexp--sc_prohibit_doubled_word = \
tests/qemu-iotests/142(\.out)? \
tests/qtest/arm-cpu-features\.c \
ui/cursor\.c
+
+exclude_file_name_regexp--sc_c_file_osdep_h = \
+ contrib/plugins/.* \
+ linux-user/(mips64|x86_64)/(signal|cpu_loop)\.c \
+ pc-bios/.* \
+ scripts/coverity-scan/model\.c \
+ scripts/xen-detect\.c \
+ subprojects/.* \
+ target/hexagon/(gen_semantics|gen_dectree_import)\.c \
+ target/s390x/gen-features\.c \
+ tests/migration/s390x/a-b-bios\.c \
+ tests/multiboot/.* \
+ tests/plugin/.* \
+ tests/tcg/.* \
+ tests/uefi-test-tools/.* \
+ tests/unit/test-rcu-(simpleq|slist|tailq)\.c \
+ tools/ebpf/rss.bpf.c
diff --git a/tests/style.mak b/tests/style.mak
index 4056bde619..301d978155 100644
--- a/tests/style.mak
+++ b/tests/style.mak
@@ -52,3 +52,9 @@ sc_prohibit_doubled_word:
| $(GREP) . \
&& { echo '$(ME): doubled words' 1>&2; exit 1; } \
|| :
+
+sc_c_file_osdep_h:
+ @require='#include "qemu/osdep.h"' \
+ in_vc_files='\.c$$' \
+ halt='all C files must include qemu/osdep.h' \
+ $(_sc_search_regexp)
--
2.36.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 7/7] tests/style: check qemu/osdep.h is included in all .c files
2022-07-04 15:23 ` [PATCH v2 7/7] tests/style: check " Daniel P. Berrangé
@ 2022-07-04 15:47 ` Peter Maydell
2022-07-04 15:50 ` Daniel P. Berrangé
0 siblings, 1 reply; 22+ messages in thread
From: Peter Maydell @ 2022-07-04 15:47 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Paolo Bonzini, Alex Bennée, Thomas Huth,
Philippe Mathieu-Daudé
On Mon, 4 Jul 2022 at 16:23, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> +
> +sc_c_file_osdep_h:
> + @require='#include "qemu/osdep.h"' \
> + in_vc_files='\.c$$' \
> + halt='all C files must include qemu/osdep.h' \
> + $(_sc_search_regexp)
The rule is not just "included in all C files", but "included
as the *first* include in all C files".
thanks
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 7/7] tests/style: check qemu/osdep.h is included in all .c files
2022-07-04 15:47 ` Peter Maydell
@ 2022-07-04 15:50 ` Daniel P. Berrangé
2022-07-04 15:55 ` Peter Maydell
0 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrangé @ 2022-07-04 15:50 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-devel, Paolo Bonzini, Alex Bennée, Thomas Huth,
Philippe Mathieu-Daudé
On Mon, Jul 04, 2022 at 04:47:16PM +0100, Peter Maydell wrote:
> On Mon, 4 Jul 2022 at 16:23, Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>
> > +
> > +sc_c_file_osdep_h:
> > + @require='#include "qemu/osdep.h"' \
> > + in_vc_files='\.c$$' \
> > + halt='all C files must include qemu/osdep.h' \
> > + $(_sc_search_regexp)
>
> The rule is not just "included in all C files", but "included
> as the *first* include in all C files".
Oh right, so we can copy a rule from libvirt to validate that.
It would look like this, but s,config.h,qemu/osdep.h,
# Print each file name for which the first #include does not match
# $(config_h_header). Like grep -m 1, this only looks at the first match.
perl_config_h_first_ = \
-e 'BEGIN {$$ret = 0}' \
-e 'if (/^\# *include\b/) {' \
-e ' if (not m{^\# *include $(config_h_header)}) {' \
-e ' print "$$ARGV\n";' \
-e ' $$ret = 1;' \
-e ' }' \
-e ' \# Move on to next file after first include' \
-e ' close ARGV;' \
-e '}' \
-e 'END {exit $$ret}'
# You must include <config.h> before including any other header file.
# This can possibly be via a package-specific header, if given by syntax-check.mk.
sc_require_config_h_first:
@if $(VC_LIST_EXCEPT) | $(GREP) '\.c$$' > /dev/null; then \
files=$$($(VC_LIST_EXCEPT) | $(GREP) '\.c$$') && \
perl -n $(perl_config_h_first_) $$files || \
{ echo 'the above files include some other header' \
'before <config.h>' 1>&2; exit 1; } || :; \
else :; \
fi
With 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 :|
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 7/7] tests/style: check qemu/osdep.h is included in all .c files
2022-07-04 15:50 ` Daniel P. Berrangé
@ 2022-07-04 15:55 ` Peter Maydell
2022-07-04 16:15 ` Daniel P. Berrangé
0 siblings, 1 reply; 22+ messages in thread
From: Peter Maydell @ 2022-07-04 15:55 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Paolo Bonzini, Alex Bennée, Thomas Huth,
Philippe Mathieu-Daudé
On Mon, 4 Jul 2022 at 16:50, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Mon, Jul 04, 2022 at 04:47:16PM +0100, Peter Maydell wrote:
> > On Mon, 4 Jul 2022 at 16:23, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > >
> > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> >
> > > +
> > > +sc_c_file_osdep_h:
> > > + @require='#include "qemu/osdep.h"' \
> > > + in_vc_files='\.c$$' \
> > > + halt='all C files must include qemu/osdep.h' \
> > > + $(_sc_search_regexp)
> >
> > The rule is not just "included in all C files", but "included
> > as the *first* include in all C files".
>
> Oh right, so we can copy a rule from libvirt to validate that.
>
> It would look like this, but s,config.h,qemu/osdep.h,
>
>
> # Print each file name for which the first #include does not match
> # $(config_h_header). Like grep -m 1, this only looks at the first match.
> perl_config_h_first_ = \
> -e 'BEGIN {$$ret = 0}' \
> -e 'if (/^\# *include\b/) {' \
> -e ' if (not m{^\# *include $(config_h_header)}) {' \
> -e ' print "$$ARGV\n";' \
> -e ' $$ret = 1;' \
> -e ' }' \
> -e ' \# Move on to next file after first include' \
> -e ' close ARGV;' \
> -e '}' \
> -e 'END {exit $$ret}'
>
> # You must include <config.h> before including any other header file.
> # This can possibly be via a package-specific header, if given by syntax-check.mk.
> sc_require_config_h_first:
> @if $(VC_LIST_EXCEPT) | $(GREP) '\.c$$' > /dev/null; then \
> files=$$($(VC_LIST_EXCEPT) | $(GREP) '\.c$$') && \
> perl -n $(perl_config_h_first_) $$files || \
> { echo 'the above files include some other header' \
> 'before <config.h>' 1>&2; exit 1; } || :; \
> else :; \
> fi
As an example syntax checking rule I think this makes a pretty
convincing case for the argument "make is the wrong language/framework
for this job"...
thanks
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 7/7] tests/style: check qemu/osdep.h is included in all .c files
2022-07-04 15:55 ` Peter Maydell
@ 2022-07-04 16:15 ` Daniel P. Berrangé
0 siblings, 0 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2022-07-04 16:15 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-devel, Paolo Bonzini, Alex Bennée, Thomas Huth,
Philippe Mathieu-Daudé
On Mon, Jul 04, 2022 at 04:55:45PM +0100, Peter Maydell wrote:
> On Mon, 4 Jul 2022 at 16:50, Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Mon, Jul 04, 2022 at 04:47:16PM +0100, Peter Maydell wrote:
> > > On Mon, 4 Jul 2022 at 16:23, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > >
> > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > >
> > > > +
> > > > +sc_c_file_osdep_h:
> > > > + @require='#include "qemu/osdep.h"' \
> > > > + in_vc_files='\.c$$' \
> > > > + halt='all C files must include qemu/osdep.h' \
> > > > + $(_sc_search_regexp)
> > >
> > > The rule is not just "included in all C files", but "included
> > > as the *first* include in all C files".
> >
> > Oh right, so we can copy a rule from libvirt to validate that.
> >
> > It would look like this, but s,config.h,qemu/osdep.h,
> >
> >
> > # Print each file name for which the first #include does not match
> > # $(config_h_header). Like grep -m 1, this only looks at the first match.
> > perl_config_h_first_ = \
> > -e 'BEGIN {$$ret = 0}' \
> > -e 'if (/^\# *include\b/) {' \
> > -e ' if (not m{^\# *include $(config_h_header)}) {' \
> > -e ' print "$$ARGV\n";' \
> > -e ' $$ret = 1;' \
> > -e ' }' \
> > -e ' \# Move on to next file after first include' \
> > -e ' close ARGV;' \
> > -e '}' \
> > -e 'END {exit $$ret}'
> >
> > # You must include <config.h> before including any other header file.
> > # This can possibly be via a package-specific header, if given by syntax-check.mk.
> > sc_require_config_h_first:
> > @if $(VC_LIST_EXCEPT) | $(GREP) '\.c$$' > /dev/null; then \
> > files=$$($(VC_LIST_EXCEPT) | $(GREP) '\.c$$') && \
> > perl -n $(perl_config_h_first_) $$files || \
> > { echo 'the above files include some other header' \
> > 'before <config.h>' 1>&2; exit 1; } || :; \
> > else :; \
> > fi
>
> As an example syntax checking rule I think this makes a pretty
> convincing case for the argument "make is the wrong language/framework
> for this job"...
Matching contextually across multiple lines of text is admittedly hard.
IME most of the usage of this syntax checking facility we had in libvirt
can be handled using single line matches, which are trivial to provide.
With 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 :|
^ permalink raw reply [flat|nested] 22+ messages in thread