date:20220523

Re: [PATCH] qemuxml2xmltests.c: convert pseries tests to DO_TEST_CAPS_ARCH_LATEST

2022-05-23 Thread Daniel Henrique Barboza





On 5/23/22 07:39, Martin Kletzander wrote:

On Sat, May 21, 2022 at 01:59:05PM -0300, Daniel Henrique Barboza wrote:

qemuxml2xmltests that have "pseries" in the name now use the
DO_TEST_CAPS_LATEST_ARCH() macro.

Signed-off-by: Daniel Henrique Barboza 


Reviewed-by: Martin Kletzander 



Pushed. Thanks!


Daniel

Re: [PATCH 0/3] qemuxml2argvtest: add ARG_CAPS_HOST_CPU_MODEL

2022-05-23 Thread Daniel Henrique Barboza


Pushed fixing the typo in patch 2. Thanks!


Daniel

On 5/20/22 17:47, Daniel Henrique Barboza wrote:

Hi,

This series attempts to fix a problem I found when converting the
"pseries-cpu-compat-power9" to use DO_TEST_CAPS_ARCH_LATEST* macros.
More information about the problem per se can be found in patch 02.
Patch 03 is where I was able to convert the said test.

Patch 01 is a trivial fix I found out when reading code.


Daniel Henrique Barboza (3):
   qemu_capspriv.h: fix identation
   testutilsqemu: introduce ARG_CAPS_HOST_CPU_MODEL
   qemuxml2argvtest.c: use CAPS_ARCH_LATEST() with
 pseries-cpu-compat-power9

  src/qemu/qemu_capspriv.h  |  4 +-
  ...eries-cpu-compat-power9.ppc64-latest.args} | 12 +++---
  ...series-cpu-compat-power9.ppc64-latest.err} |  0
  tests/qemuxml2argvtest.c  | 41 ---
  tests/testutilsqemu.c |  4 ++
  tests/testutilsqemu.h | 18 
  6 files changed, 49 insertions(+), 30 deletions(-)
  rename tests/qemuxml2argvdata/{pseries-cpu-compat-power9.args => 
pseries-cpu-compat-power9.ppc64-latest.args} (60%)
  rename tests/qemuxml2argvdata/{pseries-cpu-compat-power9.err => 
pseries-cpu-compat-power9.ppc64-latest.err} (100%)

[libvirt PATCH 07/13] syntax-check: Drop unused machinery

2022-05-23 Thread Andrea Bolognani

_equal is not used anywhere; the rest of the code implements the
syntax-check target, which takes care of figuring out the list of
checks that have been defined and running them, printing the name
of each check along with its execution time.

This was useful when we were using autotools, but these days we
have meson driving the entire build process and each of the
checks is registered as a separate test, which gives us all of
the features described above for free.

Signed-off-by: Andrea Bolognani 
---
 build-aux/syntax-check.mk | 38 --
 1 file changed, 38 deletions(-)

diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index 1252cb8eb4..da0140c355 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -32,11 +32,6 @@ AWK ?= awk
 _empty =
 _sp = $(_empty) $(_empty)
 
-# _equal,S1,S2
-# 
-# If S1 == S2, return S1, otherwise the empty string.
-_equal = $(and $(findstring $(1),$(2)),$(findstring $(2),$(1)))
-
 VC_LIST = cd $(top_srcdir); git ls-tree -r 'HEAD:' | \
   sed -n "s|^100[^ ]*.||p"
 
@@ -71,39 +66,6 @@ export LC_ALL = C
 ## Sanity checks.  ##
 ## --- ##
 
-_cfg_mk := $(wildcard $(top_srcdir)/build-aux/syntax-check.mk)
-
-# Collect the names of rules starting with 'sc_'.
-syntax-check-rules := $(sort $(shell $(SED) -n \
-   's/^\(sc_[a-zA-Z0-9_-]*\):.*/\1/p' $(top_srcdir)/$(ME) $(_cfg_mk)))
-.PHONY: $(syntax-check-rules)
-
-# 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), $(syntax-check-rules)))
-
-syntax-check: $(local-check)
-
 # Files that should never cause syntax check failures.
 VC_LIST_ALWAYS_EXCLUDE_REGEX = \
   \.(po|ico|png)$$
-- 
2.35.3

[libvirt PATCH 12/13] syntax-check: Drop sc_ prefix when adding checks to meson

2022-05-23 Thread Andrea Bolognani

All checks are added to the syntax-check suite, and this name is
displayed prominently in the output of 'meson test', so there
really is no need to include the sc_ prefix too.

Signed-off-by: Andrea Bolognani 
---
 build-aux/meson.build | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/build-aux/meson.build b/build-aux/meson.build
index 96562a4f4a..fcd231a386 100644
--- a/build-aux/meson.build
+++ b/build-aux/meson.build
@@ -47,7 +47,7 @@ configure_file(
 
 rc = run_command(
   'sed', '-n',
-  's/^\\(sc_[a-zA-Z0-9_-]*\\):.*/\\1/p',
+  's/^sc_\\([a-zA-Z0-9_-]*\\):.*/\\1/p',
   meson.current_source_dir() / 'syntax-check.mk',
   check: true,
 )
@@ -62,7 +62,7 @@ if git
 test(
   target,
   make_prog,
-  args: [ '-C', meson.current_build_dir(), target ],
+  args: [ '-C', meson.current_build_dir(), 'sc_@0@'.format(target) ],
   depends: [
 potfiles_dep,
   ],
-- 
2.35.3

[libvirt PATCH 13/13] syntax-check: Enforce sc_prohibit_backslash_alignment everywhere

2022-05-23 Thread Andrea Bolognani

Basically all files in the repository are already passing the
check, except for syntax-check.mk itself. Fix that, and stop
limiting the files on which the test is performed.

These changes have been generated by running

  $ sed -Ei 's/[ '$'\t'']+\\$/ \\/g' $(git grep -El '[ '$'\t'']+\\$')

Signed-off-by: Andrea Bolognani 
---
 build-aux/syntax-check.mk | 416 +++---
 ci/Makefile   |   2 +-
 2 files changed, 207 insertions(+), 211 deletions(-)

diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index b770e89efa..0b832706ba 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -863,7 +863,6 @@ sc_prohibit_author:
 # or tabs (at least one of them) right before the trailing backslash
 sc_prohibit_backslash_alignment:
@prohibit='(  | )\\$$' \
-   in_vc_files='*\.([chx]|am|mk)$$' \
halt='Do not attempt to right-align backslashes' \
  $(_sc_search_regexp)
 
@@ -900,28 +899,28 @@ sc_prohibit_dirent_d_type:
  $(_sc_search_regexp)
 
 sc_cast_of_argument_to_free:
-   @prohibit='\&2;   \
-  exit 1; }\
+   @$(VC_LIST_EXCEPT) \
+ | xargs $(GREP) -nEA2 '[^rp]error *\(' /dev/null \
+ | $(GREP) -E '"Warning|"Fatal|"fatal' \
+ && { echo 'use FATAL, WARNING or warning' 1>&2; \
+  exit 1; } \
  || :
 
 # Error messages should not end with a period
 sc_error_message_period:
-   @$(VC_LIST_EXCEPT)  \
- | xargs $(GREP) -nEA2 '[^rp]error *\(' /dev/null  \
- | $(GREP) -E '[^."]\."'   \
- && { echo 'found error message ending in period' 1>&2;\
-  exit 1; }\
+   @$(VC_LIST_EXCEPT) \
+ | xargs $(GREP) -nEA2 '[^rp]error *\(' /dev/null \
+ | $(GREP) -E '[^."]\."' \
+ && { echo 'found error message ending in period' 1>&2; \
+  exit 1; } \
  || :
 
 # Don't use cpp tests of this symbol.  All code assumes config.h is included.
 sc_prohibit_have_config_h:
-   @prohibit='^# *if.*HAVE''_CONFIG_H' \
-   halt='found use of HAVE''_CONFIG_H; remove' \
+   @prohibit='^# *if.*HAVE''_CONFIG_H' \
+   halt='found use of HAVE''_CONFIG_H; remove' \
  $(_sc_search_regexp)
 
 # Nearly all .c files must include .  However, we also permit this
@@ -970,49 +969,49 @@ sc_prohibit_have_config_h:
 # config_h_header must be suitable for grep -E.
 config_h_header ?= 
 sc_require_config_h:
-   @require='^# *include $(config_h_header)'   \
-   in_vc_files='\.c$$' \
-   halt='the above files do not include '\
+   @require='^# *include $(config_h_header)' \
+   in_vc_files='\.c$$' \
+   halt='the above files do not include ' \
  $(_sc_search_regexp)
 
 # 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 '}'   \
+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  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 ' 1>&2; exit 1; } || :;   \
-   else :; \
+   @if $(VC_LIST_EXCEPT) | $(GREP) '\.c$$' > /dev/null; then \
+

[libvirt PATCH 10/13] syntax-check: Reorganize file

2022-05-23 Thread Andrea Bolognani

Due to the way make works, we are not forced to follow a strict
order in defining rules and variables. In fact _sc_search_regexp,
which is used by all checks, is only defined halfway through the
file.

Shuffle things around so that the things that we need to look at
the most frequently are closer to the top of the file.

Signed-off-by: Andrea Bolognani 
---
 build-aux/syntax-check.mk | 358 +++---
 1 file changed, 182 insertions(+), 176 deletions(-)

diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index 13f565d1b8..a37e43b9f6 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -19,43 +19,10 @@
 # along with this program.  If not, see
 # .
 
-# Helper variables.
-_empty =
-_sp = $(_empty) $(_empty)
-
-VC_LIST = cd $(top_srcdir); git ls-tree -r 'HEAD:' | \
-  sed -n "s|^100[^ ]*.||p"
-
-# This is to preprocess robustly the output of $(VC_LIST), so that even
-# when $(top_srcdir) is a pathological name like "", the leading sed 
command
-# removes only the intended prefix.
-_dot_escaped_srcdir = $(subst .,\.,$(top_srcdir))
-_dot_escaped_builddir = $(subst .,\.,$(top_builddir))
 
-# Post-process $(VC_LIST) output, prepending $(top_srcdir)/, but only
-# when $(top_srcdir) is not ".".
-ifeq ($(top_srcdir),.)
-  _prepend_srcdir_prefix =
-else
-  _prepend_srcdir_prefix = | $(SED) 's|^|$(top_srcdir)/|'
-endif
-
-# In order to be able to consistently filter "."-relative names,
-# (i.e., with no $(top_srcdir) prefix), this definition is careful to
-# remove any $(top_srcdir) prefix, and to restore what it removes.
-_sc_excl = \
-  $(or $(exclude_file_name_regexp--$@),^$$)
-VC_LIST_EXCEPT = \
-  $(VC_LIST) | $(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
-
-## --- ##
-## Sanity checks.  ##
-## --- ##
+## - ##
+## Rules ##
+## - ##
 
 # Files that should never cause syntax check failures.
 VC_LIST_ALWAYS_EXCLUDE_REGEX = \
@@ -927,143 +894,6 @@ sc_prohibit_dirent_d_type:
halt='do not use the d_type field in "struct dirent"' \
  $(_sc_search_regexp)
 
-
-# _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 $(top_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' "$$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" \
-

[libvirt PATCH 11/13] syntax-check: Add all target

2022-05-23 Thread Andrea Bolognani

The makefile is an implementation detail, so point users towards
the proper way of running syntax-check if they happen to call it
directly.

Signed-off-by: Andrea Bolognani 
---
 build-aux/syntax-check.mk | 5 +
 1 file changed, 5 insertions(+)

diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index a37e43b9f6..b770e89efa 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -20,6 +20,11 @@
 # .
 
 
+all:
+   @echo "Do not call this file directly, use 'meson test' instead" >&2; \
+   exit 1
+
+
 ## - ##
 ## Rules ##
 ## - ##
-- 
2.35.3

[libvirt PATCH 05/13] syntax-check: Use VC_LIST_EXCEPT in sc_prohibit_backup_files

2022-05-23 Thread Andrea Bolognani

We can assume that VC_LIST_ALWAYS_EXCLUDE_REGEX will not be
defined in a way that would catch backup files.

Signed-off-by: Andrea Bolognani 
---
 build-aux/syntax-check.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index 5244d4ee2d..d6f2a0602d 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -1326,7 +1326,7 @@ sc_prohibit_defined_have_decl_tests:
 
 # Prohibit checked in backup files.
 sc_prohibit_backup_files:
-   @$(VC_LIST) | $(GREP) '~$$' &&  \
+   @$(VC_LIST_EXCEPT) | $(GREP) '~$$' && \
  { echo '$(ME): found version controlled backup file' 1>&2;\
exit 1; } || :
 
-- 
2.35.3

[libvirt PATCH 09/13] syntax-check: Detect awk the same as all other programs

2022-05-23 Thread Andrea Bolognani

Signed-off-by: Andrea Bolognani 
---
 build-aux/Makefile.in | 1 +
 build-aux/meson.build | 3 +++
 build-aux/syntax-check.mk | 5 -
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/build-aux/Makefile.in b/build-aux/Makefile.in
index 9ccbec7b1b..7ee4680847 100644
--- a/build-aux/Makefile.in
+++ b/build-aux/Makefile.in
@@ -7,6 +7,7 @@ RUNUTF8 = @runutf8@
 PYTHON = @PYTHON3@
 GREP = @GREP@
 SED = @SED@
+AWK = @AWK@
 
 # include syntax-check.mk file
 include $(top_srcdir)/build-aux/syntax-check.mk
diff --git a/build-aux/meson.build b/build-aux/meson.build
index bbfa27c4c6..96562a4f4a 100644
--- a/build-aux/meson.build
+++ b/build-aux/meson.build
@@ -26,6 +26,8 @@ else
   grep_prog = find_program('grep')
 endif
 
+awk_prog = find_program('awk')
+
 syntax_check_conf = configuration_data({
   'top_srcdir': meson.source_root(),
   'top_builddir': meson.build_root(),
@@ -34,6 +36,7 @@ syntax_check_conf = configuration_data({
   'PYTHON3': python3_prog.path(),
   'GREP': grep_prog.path(),
   'SED': sed_prog.path(),
+  'AWK': awk_prog.path(),
 })
 
 configure_file(
diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index 4573655074..13f565d1b8 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -19,11 +19,6 @@
 # along with this program.  If not, see
 # .
 
-# These variables ought to be defined through the configure.ac section
-# of the module description. But some packages import this file directly,
-# ignoring the module description.
-AWK ?= awk
-
 # Helper variables.
 _empty =
 _sp = $(_empty) $(_empty)
-- 
2.35.3

[libvirt PATCH 03/13] syntax-check: Remove sc_copyright_usage exception

2022-05-23 Thread Andrea Bolognani

The pattern in build-aux/syntax-check.mk is written specifically
so that it won't match itself, which makes having an exception
for the file unnecessary.

Signed-off-by: Andrea Bolognani 
---
 build-aux/syntax-check.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index 0637a42e94..8d30b0e75a 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -1573,7 +1573,7 @@ exclude_file_name_regexp--sc_gettext_init = \
^((tests|examples)/|tools/virt-login-shell.c)
 
 exclude_file_name_regexp--sc_copyright_usage = \
-  ^COPYING(|\.LESSER)|build-aux/syntax-check.mk$$
+  ^COPYING(|\.LESSER)$$
 
 exclude_file_name_regexp--sc_flags_usage = \
   
^(build-aux/syntax-check\.mk|docs/|src/util/virnetdevtap\.c$$|tests/((vir(cgroup|pci|test|usb)|nss|qemuxml2argv|qemusecurity)mock|virfilewrapper)\.c$$)
-- 
2.35.3

[libvirt PATCH 08/13] syntax-check: Drop ME variable

2022-05-23 Thread Andrea Bolognani

It's only used in diagnostics, and even there it's not
particularly useful and can make it more difficult to spot the
actual error message.

Signed-off-by: Andrea Bolognani 
---
 build-aux/syntax-check.mk | 58 ++-
 1 file changed, 27 insertions(+), 31 deletions(-)

diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index da0140c355..4573655074 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -19,10 +19,6 @@
 # along with this program.  If not, see
 # .
 
-# This is reported not to work with make-3.79.1
-# ME := $(word $(words $(MAKEFILE_LIST)),$(MAKEFILE_LIST))
-ME := build-aux/syntax-check.mk
-
 # These variables ought to be defined through the configure.ac section
 # of the module description. But some packages import this file directly,
 # ignoring the module description.
@@ -103,7 +99,7 @@ sc_flags_usage:
$(top_srcdir)/include/libvirt/libvirt-lxc.h \
$(top_srcdir)/include/libvirt/libvirt-admin.h \
  | $(GREP) -c '\(long\|unsigned\) flags')" != 4 && \
- { echo '$(ME): new API should use "unsigned int flags"' 1>&2; \
+ { echo 'new API should use "unsigned int flags"' 1>&2; \
exit 1; } || :
@prohibit=' flags G_GNUC_UNUSED' \
exclude='virSecurityDomainImageLabelFlags' \
@@ -466,7 +462,7 @@ sc_libvirt_unmarked_diagnostics:
$(GREP) -A1 -nE '\<$(func_re) *\(.*,$$' /dev/null; } \
   | $(SED) -E 's/_\("([^\"]|\\.)+"//;s/"%s"//' \
   | $(GREP) '"' && \
- { echo '$(ME): found unmarked diagnostic(s)' 1>&2; \
+ { echo 'found unmarked diagnostic(s)' 1>&2; \
exit 1; } || :
 
 # Like the above, but prohibit a newline at the end of a diagnostic.
@@ -480,7 +476,7 @@ sc_prohibit_newline_at_end_of_diagnostic:
@$(VC_LIST_EXCEPT) | xargs $(GREP) -A2 -nE \
'\<$(func_re) *\(' /dev/null \
| $(GREP) '\\n"' \
- && { echo '$(ME): newline at end of message(s)' 1>&2; \
+ && { echo 'newline at end of message(s)' 1>&2; \
exit 1; } || :
 
 # Look for diagnostics that lack a % in the format string, except that we
@@ -495,7 +491,7 @@ sc_prohibit_diagnostic_without_format:
-e '/(vah_(error|warning))/d' \
-e '/\<$(func_re) *\([^"]*"([^%"]|"\n[^"]*")*"[,)]/p' \
| $(GREP) -vE 'VIR_ERROR' && \
- { echo '$(ME): found diagnostic without %' 1>&2; \
+ { echo 'found diagnostic without %' 1>&2; \
exit 1; } || :
 
 # The strings "" and "%s" should never be marked for translation.
@@ -515,17 +511,17 @@ sc_require_whitespace_in_translation:
@$(VC_LIST_EXCEPT) | xargs $(GREP) -n -A1 '"$$' /dev/null \
   | $(SED) -ne ':l; /"$$/ {N;b l;}; s/"\n[^"]*"/""/g; s/\\n/ /g' \
-e '/_(.*[^\ ]""[^\ ]/p' | $(GREP) . && \
- { echo '$(ME): missing whitespace at line split' 1>&2; \
+ { echo 'missing whitespace at line split' 1>&2; \
exit 1; } || :
 
 # Enforce recommended preprocessor indentation style.
 sc_preprocessor_indentation:
@if cppi --version >/dev/null 2>&1; then \
  $(VC_LIST_EXCEPT) | $(GREP) -E '\.[ch](\.in)?$$' | xargs cppi -a -c \
-   || { echo '$(ME): incorrect preprocessor indentation' 1>&2; \
+   || { echo 'incorrect preprocessor indentation' 1>&2; \
exit 1; }; \
else \
- echo '$(ME): skipping test $@: cppi not installed' 1>&2; \
+ echo 'skipping test $@: cppi not installed' 1>&2; \
fi
 
 # Enforce similar spec file indentation style, by running cppi on a
@@ -539,10 +535,10 @@ sc_spec_indentation:
-e 's|^\( *[^#/ ]\)|// \1|; s|^\( */[^/]\)|// \1|' $$f \
| cppi -a -c 2>&1 | $(SED) "s|standard input|$$f|"; \
  done | { if $(GREP) . >&2; then false; else :; fi; } \
-   || { echo '$(ME): incorrect preprocessor indentation' 1>&2; \
+   || { echo 'incorrect preprocessor indentation' 1>&2; \
exit 1; }; \
else \
- echo '$(ME): skipping test $@: cppi not installed' 1>&2; \
+ echo 'skipping test $@: cppi not installed' 1>&2; \
fi
 
 
@@ -601,7 +597,7 @@ sc_require_enum_last_marker:
 -e '/VIR_ENUM_IMPL[^,]*,[^,]*,[^,]*[^_,][^L,][^A,][^S,][^T,],/p' \
 -e '/VIR_ENUM_IMPL[^,]*,[^,]\{0,4\},/p' \
   | $(GREP) . && \
- { echo '$(ME): enum impl needs _LAST marker on second line' 1>&2; \
+ { echo 'enum impl needs _LAST marker on second line' 1>&2; \
exit 1; } || :
 
 # We're intentionally ignoring a few warnings
@@ -623,7 +619,7 @@ sc_flake8:
ALL_PY=$$(printf "%s\n%s" "$$DOT_PY" "$$BANG_PY" | sort -u); \
echo "$$ALL_PY" | xargs $(FLAKE8) --ignore $(FLAKE8_IGNORE) 
--show-source; \
else \
-   echo '$(ME): skipping test $@: flake8 not installed' 1>&2; \
+

[libvirt PATCH 02/13] syntax-check: Remove sc_gettext_init exception

2022-05-23 Thread Andrea Bolognani

The file src/util/vireventglib.c doesn't contain a main() function
and so it's not even considered by the check.

Signed-off-by: Andrea Bolognani 
---
 build-aux/syntax-check.mk | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index 8689d01f19..0637a42e94 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -1569,7 +1569,8 @@ exclude_file_name_regexp--sc_avoid_strcase = 
^tools/vsh\.h$$
 
 exclude_file_name_regexp--sc_avoid_write = ^src/libvirt-stream\.c$$
 
-exclude_file_name_regexp--sc_gettext_init = 
^((tests|examples)/|tools/virt-login-shell.c|src/util/vireventglib\.c)
+exclude_file_name_regexp--sc_gettext_init = \
+   ^((tests|examples)/|tools/virt-login-shell.c)
 
 exclude_file_name_regexp--sc_copyright_usage = \
   ^COPYING(|\.LESSER)|build-aux/syntax-check.mk$$
-- 
2.35.3

[libvirt PATCH 01/13] syntax-check: Drop sc_bindtextdomain check

2022-05-23 Thread Andrea Bolognani

This is one of the standard checks that we have inherited from
gnulib, but it's not applicable to libvirt because we don't want
plain bindtextdomain() to be used: virGettextInitialize() is our
own private API that should be used instead.

The sc_gettext_init check ensures that our private API is used
in all the places where it makes sense, and the sc_bindtextdomain
check was disabled entirely via a blanket exception. Drop it
instead of keeping dead code around.

Signed-off-by: Andrea Bolognani 
---
 build-aux/syntax-check.mk | 11 ---
 1 file changed, 11 deletions(-)

diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index 6b54f830f1..8689d01f19 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -1297,15 +1297,6 @@ sc_prohibit_dirent_without_use:
re='\<($(_dirent_syms_re))\>'   \
  $(_sc_header_without_use)
 
-# Ensure that each .c file containing a "main" function also
-# calls bindtextdomain.
-sc_bindtextdomain:
-   @require='bindtextdomain *\('   \
-   in_vc_files='\.c$$' \
-   containing='\

[libvirt PATCH 06/13] syntax-check: Simplify VC_LIST_ALWAYS_EXCLUDE_REGEX

2022-05-23 Thread Andrea Bolognani

Most of the pattern is no longer relevant, because the files it
was intended to match have been dropped from the repository.

Specifically:

   filescommitdate
  --    --
   *.gif  6cb131e5cbd0  2022-01-19
   *.fig  9ad637c9651f  2020-07-10
  docs/news*.html.in  f45735786a3d  2020-06-02
docs/*.patch  6be034a8c062  2018-08-23

We can also avoid having a fallback value for the pattern: that
made sense when the implementation was coming from gnulib, as
they wouldn't be able to know in advance if the user would need
to provide their own exclude patterns, but that scenario is no
longer relevant to us.

Signed-off-by: Andrea Bolognani 
---
 build-aux/syntax-check.mk | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index d6f2a0602d..1252cb8eb4 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -40,10 +40,6 @@ _equal = $(and $(findstring $(1),$(2)),$(findstring 
$(2),$(1)))
 VC_LIST = cd $(top_srcdir); git ls-tree -r 'HEAD:' | \
   sed -n "s|^100[^ ]*.||p"
 
-# You can override this variable in syntax-check.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 $(top_srcdir) is a pathological name like "", the leading sed 
command
 # removes only the intended prefix.
@@ -110,7 +106,7 @@ syntax-check: $(local-check)
 
 # Files that should never cause syntax check failures.
 VC_LIST_ALWAYS_EXCLUDE_REGEX = \
-  (^(docs/(news(-[0-9]*)?\.html\.in|.*\.patch))|\.(po|fig|gif|ico|png))$$
+  \.(po|ico|png)$$
 
 # Avoid uses of write(2).  Either switch to streams (fwrite), or use
 # the safewrite wrapper.
-- 
2.35.3

[libvirt PATCH 04/13] syntax-check: Improve sc_prohibit_config_h_in_headers check

2022-05-23 Thread Andrea Bolognani

In its current form, the check will not only catch the intended

  #include 

but also stuff like

  #include 
  #include "qemu_interop_config.h"
  #include 

The last one is problematic, because it's used in config.h itself.
Making the pattern more strict allows us to drop the exception.

Signed-off-by: Andrea Bolognani 
---
 build-aux/syntax-check.mk | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index 8d30b0e75a..5244d4ee2d 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -694,7 +694,7 @@ sc_prohibit_include_public_headers_brackets:
 #  is only needed in .c files; .h files do not need it since
 # .c files must include config.h before any other .h.
 sc_prohibit_config_h_in_headers:
-   @prohibit='^# *include\>.*config\.h' \
+   @prohibit='^# *include *[<"]config\.h' \
in_vc_files='\.h$$' \
halt='headers should not include ' \
  $(_sc_search_regexp)
@@ -1707,6 +1707,3 @@ exclude_file_name_regexp--sc_prohibit_backslash_alignment 
= \
 
 exclude_file_name_regexp--sc_prohibit_select = \
   
^build-aux/syntax-check\.mk|src/util/vireventglibwatch\.c|tests/meson\.build$$
-
-exclude_file_name_regexp--sc_prohibit_config_h_in_headers = \
-  ^config\.h$$
-- 
2.35.3

[libvirt PATCH 00/13] syntax-check: Cleanups and improvements

2022-05-23 Thread Andrea Bolognani

  $ meson test --suite blurb

Andrea Bolognani (13):
  syntax-check: Drop sc_bindtextdomain check
  syntax-check: Remove sc_gettext_init exception
  syntax-check: Remove sc_copyright_usage exception
  syntax-check: Improve sc_prohibit_config_h_in_headers check
  syntax-check: Use VC_LIST_EXCEPT in sc_prohibit_backup_files
  syntax-check: Simplify VC_LIST_ALWAYS_EXCLUDE_REGEX
  syntax-check: Drop unused machinery
  syntax-check: Drop ME variable
  syntax-check: Detect awk the same as all other programs
  syntax-check: Reorganize file
  syntax-check: Add all target
  syntax-check: Drop sc_ prefix when adding checks to meson
  syntax-check: Enforce sc_prohibit_backslash_alignment everywhere

 build-aux/Makefile.in |   1 +
 build-aux/meson.build |   7 +-
 build-aux/syntax-check.mk | 773 ++
 ci/Makefile   |   2 +-
 4 files changed, 365 insertions(+), 418 deletions(-)

-- 
2.35.3

Re: [PATCH RFC 06/10] virprocess: Core Scheduling support

2022-05-23 Thread Daniel P . Berrangé

On Mon, May 09, 2022 at 05:02:13PM +0200, Michal Privoznik wrote:
> Since its 5.14 release the Linux kernel allows userspace to
> define trusted groups of processes/threads that can run on
> sibling Hyper Threads (HT) at the same time. This is to mitigate
> side channel attacks like L1TF or MDS. If there are no tasks to
> fully utilize all HTs, then a HT will idle instead of running a
> task from another (un-)trusted group.
> 
> On low level, this is implemented by cookies (effectively an UL
> value): processes in the same trusted group share the same cookie
> and cookie is unique to the group. There are four basic
> operations:
> 
> 1) PR_SCHED_CORE_GET -- get cookie of given PID,
> 2) PR_SCHED_CORE_CREATE -- create a new unique cookie for PID,
> 3) PR_SCHED_CORE_SHARE_TO -- push cookie of the caller onto
>another PID,
> 4) PR_SCHED_CORE_SHARE_FROM -- pull cookie of another PID into
>the caller.
> 
> Since a system where the code is built can be different to the
> one where the code is ran let's provide declaration of some
> values. It's not unusual for distros to ship older linux-headers
> than the actual kernel.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/libvirt_private.syms |   4 ++
>  src/util/virprocess.c| 124 +++
>  src/util/virprocess.h|   8 +++
>  3 files changed, 136 insertions(+)

Reviewed-by: Daniel P. Berrangé 


> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> index 36d7df050a..cd4f3fc7e7 100644
> --- a/src/util/virprocess.c
> +++ b/src/util/virprocess.c
> @@ -57,6 +57,10 @@
>  # include 
>  #endif
>  
> +#if WITH_CAPNG

This feels odd - what relation has CAPNG got with prctl ?

> +# include 
> +#endif
> +
>  #include "virprocess.h"
>  #include "virerror.h"
>  #include "viralloc.h"
> @@ -1906,3 +1910,123 @@ virProcessGetSchedInfo(unsigned long long *cpuWait,
>  return 0;
>  }
>  #endif /* __linux__ */
> +
> +#ifdef __linux__
> +# ifndef PR_SCHED_CORE
> +/* Copied from linux/prctl.h */
> +#  define PR_SCHED_CORE 62
> +#  define PR_SCHED_CORE_GET 0
> +#  define PR_SCHED_CORE_CREATE  1 /* create unique core_sched cookie */
> +#  define PR_SCHED_CORE_SHARE_TO2 /* push core_sched cookie to pid */
> +#  define PR_SCHED_CORE_SHARE_FROM  3 /* pull core_sched cookie to pid */
> +# endif
> +
> +/* Unfortunately, kernel-headers forgot to export these. */
> +# ifndef PR_SCHED_CORE_SCOPE_THREAD
> +#  define PR_SCHED_CORE_SCOPE_THREAD 0
> +#  define PR_SCHED_CORE_SCOPE_THREAD_GROUP 1
> +#  define PR_SCHED_CORE_SCOPE_PROCESS_GROUP 2
> +# endif
> +
> +/**
> + * virProcessSchedCoreAvailable:
> + *
> + * Check whether kernel supports Core Scheduling (CONFIG_SCHED_CORE), i.e. 
> only
> + * a defined set of PIDs/TIDs can run on sibling Hyper Threads at the same
> + * time.
> + *
> + * Returns: 1 if Core Scheduling is available,
> + *  0 if Core Scheduling is NOT available,
> + * -1 otherwise.
> + */
> +int
> +virProcessSchedCoreAvailable(void)
> +{
> +unsigned long cookie = 0;
> +int rc;
> +
> +/* Let's just see if we can get our own sched cookie, and if yes we can
> + * safely assume CONFIG_SCHED_CORE kernel is available. */
> +rc = prctl(PR_SCHED_CORE, PR_SCHED_CORE_GET, 0,
> +   PR_SCHED_CORE_SCOPE_THREAD, );
> +
> +return rc == 0 ? 1 : errno == EINVAL ? 0 : -1;
> +}
> +
> +/**
> + * virProcessSchedCoreCreate:
> + *
> + * Creates a new trusted group for the caller process.
> + *
> + * Returns: 0 on success,
> + * -1 otherwise, with errno set.
> + */
> +int
> +virProcessSchedCoreCreate(void)
> +{
> +/* pid = 0 (3rd argument) means the calling process. */
> +return prctl(PR_SCHED_CORE, PR_SCHED_CORE_CREATE, 0,
> + PR_SCHED_CORE_SCOPE_THREAD_GROUP, 0);
> +}
> +
> +/**
> + * virProcessSchedCoreShareFrom:
> + * @pid: PID to share group with
> + *
> + * Places the current caller process into the trusted group of @pid.
> + *
> + * Returns: 0 on success,
> + * -1 otherwise, with errno set.
> + */
> +int
> +virProcessSchedCoreShareFrom(pid_t pid)
> +{
> +return prctl(PR_SCHED_CORE, PR_SCHED_CORE_SHARE_FROM, pid,
> + PR_SCHED_CORE_SCOPE_THREAD, 0);
> +}
> +
> +/**
> + * virProcessSchedCoreShareTo:
> + * @pid: PID to share group with
> + *
> + * Places foreign @pid into the trusted group of the current caller process.
> + *
> + * Returns: 0 on success,
> + * -1 otherwise, with errno set.
> + */
> +int
> +virProcessSchedCoreShareTo(pid_t pid)
> +{
> +return prctl(PR_SCHED_CORE, PR_SCHED_CORE_SHARE_TO, pid,
> + PR_SCHED_CORE_SCOPE_THREAD, 0);
> +}
> +
> +#else /* !__linux__ */
> +
> +int
> +virProcessSchedCoreAvailable(void)
> +{
> +return 0;
> +}
> +
> +int
> +virProcessSchedCoreCreate(void)
> +{
> +errno = ENOSYS;
> +return -1;
> +}
> +
> +int
> +virProcessSchedCoreShareFrom(pid_t pid G_GNUC_UNUSED)
> +{
> +errno = ENOSYS;
> +return -1;
> +}
> +
> +int
>

Re: [PATCH RFC 05/10] qemu_virtiofs: Separate PID read code into qemuVirtioFSGetPid

2022-05-23 Thread Daniel P . Berrangé

On Mon, May 09, 2022 at 05:02:12PM +0200, Michal Privoznik wrote:
> In near future it will be necessary to know the PID of virtiofsd
> started for QEMU. Move the code into a separate function
> (qemuVirtioFSGetPid()) and export it in the header file.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_virtiofs.c | 38 +-
>  src/qemu/qemu_virtiofs.h |  5 +
>  2 files changed, 30 insertions(+), 13 deletions(-)

Reviewed-by: Daniel P. Berrangé 

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

Re: [PATCH RFC 04/10] qemu_tpm: Expose qemuTPMEmulatorGetPid()

2022-05-23 Thread Daniel P . Berrangé

On Mon, May 09, 2022 at 05:02:11PM +0200, Michal Privoznik wrote:
> In near future it will be necessary to know the PID of swtpm
> process for QEMU. Export the function that does just that
> (qemuTPMEmulatorGetPid()).
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_tpm.c | 2 +-
>  src/qemu/qemu_tpm.h | 7 +++
>  2 files changed, 8 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé 


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

Re: [PATCH RFC 03/10] qemu_vhost_user_gpu: Export qemuVhostUserGPUGetPid()

2022-05-23 Thread Daniel P . Berrangé

On Mon, May 09, 2022 at 05:02:10PM +0200, Michal Privoznik wrote:
> In near future it will be necessary to know the PID of
> vhost-user-gpu process for QEMU. Export the function that does
> just that (qemuVhostUserGPUGetPid()).
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_vhost_user_gpu.c | 2 +-
>  src/qemu/qemu_vhost_user_gpu.h | 8 
>  2 files changed, 9 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé 


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

Re: [PATCH RFC 02/10] qemu_dbus: Separate PID read code into qemuDBusGetPID

2022-05-23 Thread Daniel P . Berrangé

On Mon, May 09, 2022 at 05:02:09PM +0200, Michal Privoznik wrote:
> In near future it will be necessary to know the PID of DBus
> daemon started for QEMU. Move the code into a separate function
> (qemuDBusGetPID()) and export it in the header file.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_dbus.c | 42 +-
>  src/qemu/qemu_dbus.h |  4 
>  2 files changed, 33 insertions(+), 13 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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

Re: [PATCH RFC 01/10] qemu_tpm: Make APIs work over a single virDomainTPMDef

2022-05-23 Thread Daniel P . Berrangé

On Mon, May 09, 2022 at 05:02:08PM +0200, Michal Privoznik wrote:
> In qemu_extdevice.c lives code that handles helper daemons that
> are required for some types of devices (e.g. virtiofsd,
> vhost-user-gpu, swtpm, etc.). These devices have their own
> handling code in separate files, with only a very basic functions
> exposed (e.g. for starting/stopping helper process, placing it
> into given CGroup, etc.). And these functions all work over a
> single instance of device (virDomainVideoDef *, virDomainFSDef *,
> etc.), except for TPM handling code which takes virDomainDef *
> and iterates over it inside its module.
> 
> Remove this oddness and make qemuExtTPM*() functions look closer
> to the rest of the code.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_extdevice.c | 51 --
>  src/qemu/qemu_tpm.c   | 89 +++
>  src/qemu/qemu_tpm.h   | 11 +++--
>  3 files changed, 69 insertions(+), 82 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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

Re: [PATCH RFC 07/10] virCommand: Introduce APIs for core scheduling

2022-05-23 Thread Daniel P . Berrangé

On Mon, May 09, 2022 at 05:02:14PM +0200, Michal Privoznik wrote:
> There are two modes of core scheduling that are handy wrt
> virCommand:
> 
> 1) create new trusted group when executing a virCommand
> 
> 2) place freshly executed virCommand into the trusted group of
>another process.
> 
> Therefore, implement these two new operations as new APIs:
> virCommandSetRunAlone() and virCommandSetRunAmong(),
> respectively.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/libvirt_private.syms |  2 ++
>  src/util/vircommand.c| 74 
>  src/util/vircommand.h|  5 +++
>  3 files changed, 81 insertions(+)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 252d7e029f..8f2b789cee 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2079,6 +2079,8 @@ virCommandSetOutputBuffer;
>  virCommandSetOutputFD;
>  virCommandSetPidFile;
>  virCommandSetPreExecHook;
> +virCommandSetRunAlone;
> +virCommandSetRunAmong;
>  virCommandSetSELinuxLabel;
>  virCommandSetSendBuffer;
>  virCommandSetUID;
> diff --git a/src/util/vircommand.c b/src/util/vircommand.c
> index 41cf552d7b..db20620f7c 100644
> --- a/src/util/vircommand.c
> +++ b/src/util/vircommand.c
> @@ -148,6 +148,9 @@ struct _virCommand {
>  #endif
>  int mask;
>  
> +bool schedCore;
> +pid_t schedCorePID;
> +
>  virCommandSendBuffer *sendBuffers;
>  size_t numSendBuffers;
>  };
> @@ -434,6 +437,22 @@ virCommandHandshakeChild(virCommand *cmd)
>  static int
>  virExecCommon(virCommand *cmd, gid_t *groups, int ngroups)
>  {
> +/* Do this before dropping capabilities. */
> +if (cmd->schedCore &&
> +virProcessSchedCoreCreate() < 0) {
> +virReportSystemError(errno, "%s",
> + _("Unable to set SCHED_CORE"));
> +return -1;
> +}
> +
> +if (cmd->schedCorePID >= 0 &&
> +virProcessSchedCoreShareFrom(cmd->schedCorePID) < 0) {
> +virReportSystemError(errno,
> + _("Unable to run among %llu"),
> + (unsigned long long) cmd->schedCorePID);
> +return -1;
> +}
> +
>  if (cmd->uid != (uid_t)-1 || cmd->gid != (gid_t)-1 ||
>  cmd->capabilities || (cmd->flags & VIR_EXEC_CLEAR_CAPS)) {
>  VIR_DEBUG("Setting child uid:gid to %d:%d with caps %llx",
> @@ -964,6 +983,7 @@ virCommandNewArgs(const char *const*args)
>  cmd->pid = -1;
>  cmd->uid = -1;
>  cmd->gid = -1;
> +cmd->schedCorePID = -1;
>  
>  virCommandAddArgSet(cmd, args);
>  
> @@ -3437,3 +3457,57 @@ virCommandRunNul(virCommand *cmd G_GNUC_UNUSED,
>  return -1;
>  }
>  #endif /* WIN32 */
> +
> +/**
> + * virCommandSetRunAlone:
> + *
> + * Create new trusted group when running the command. In other words, the
> + * process won't be scheduled to run on a core among with processes from
> + * another, untrusted group.
> + */
> +void
> +virCommandSetRunAlone(virCommand *cmd)
> +{
> +if (virCommandHasError(cmd))
> +return;
> +
> +if (cmd->schedCorePID >= 0) {
> +/* Can't mix these two. */
> +cmd->has_error = -1;
> +VIR_DEBUG("cannot mix with virCommandSetRunAmong()");
> +return;
> +}
> +
> +cmd->schedCore = true;
> +}
> +
> +/**
> + * virCommandSetRunAmong:
> + * @pid: pid from a trusted group
> + *
> + * When spawning the command place it into the trusted group of @pid so that
> + * these two processes can run on Hyper Threads of a single core at the same
> + * time.
> + */
> +void
> +virCommandSetRunAmong(virCommand *cmd,
> +  pid_t pid)
> +{
> +if (virCommandHasError(cmd))
> +return;
> +
> +if (cmd->schedCore) {
> +/* Can't mix these two. */
> +VIR_DEBUG("cannot mix with virCommandSetRunAlone()");
> +cmd->has_error = -1;
> +return;
> +}
> +
> +if (pid < 0) {
> +VIR_DEBUG("invalid pid value: %lld", (long long) pid);
> +cmd->has_error = -1;
> +return;
> +}
> +
> +cmd->schedCorePID = pid;
> +}

It strikes me that we can handle this with only 1 variable and
thus avoid the possibility of mutually incompatible settings.

PID == 0 is not a valid PID for sharing a group with so we
can have

  pid_t schedCore;

with

  0 == no core scheduling
  1 == don't be silly we shouldn't ever copy 'init's PID :-)
  >1 == copy scheduling group from said PID
  -1 == create a new scheduling group


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

Re: [PATCH RFC 10/10] qemu: Place helper processes into the same trusted group

2022-05-23 Thread Daniel P . Berrangé

On Mon, May 09, 2022 at 05:02:17PM +0200, Michal Privoznik wrote:
> Since the level of trust that QEMU has is the same level of trust
> that helper processes have there's no harm in placing all of them
> into the same group.

This assumption feels like it might be a bit of a stretch. I
recall discussing this with Paolo to some extent a long time
back, but let me recap my understanding.

IIUC, the attack scenario is that a guest vCPU thread is scheduled
on a SMT sibling with another thread that is NOT running guest OS
code. "another thread" in this context refers to many things

  - Random host OS processes
  - QEMU vCPU threads from a different geust
  - QEMU emulator threads from any guest
  - QEMU helper process threads from any guest

Consider for example, if the QEMU emulator thread contains a password
used for logging into a remote RBD/Ceph server. That is a secret
credential that the guest OS should not have permission to access.

Consider alternatively that the QEMU emulator is making a TLS connection
to some service, and there are keys negotiated for the TLS session. While
some of the data transmitted over the session is known to the guest OS,
we shouldn't assume it all is.

Now in the case of QEMU emulator threads I think you can make a somewhat
decent case that we don't have to worry about it. Most of the keys/passwds
are used once at cold boot, so there's no attack window for vCPUs at that
point. There is a small window of risk when hotplugging. If someone is
really concerned about this though, they shouldn't have let QEMU have
these credentials in the first place, as its already vulnerable to a
guest escape. eg use kernel RBD instead of letting QEMU directly login
to RBD.

IOW, on balance of probabilities it is reasonable to let QEMU emulator
threads be in the same core scheduling domain as vCPU threads.

In the case of external QEMU helper processes though, I think it is
a far less clearcut decision.  There are a number of reasons why helper
processes are used, but at least one significant motivating factor is
security isolation between QEMU & the helper - they can only communicate
and share information through certain controlled mechanisms.

With this in mind I think it is risky to assume that it is  safe to
run QEMU and helper processes in the same core scheduling group. At
the same time there are likely cases where it is also just fine to
do so.

If we separate helper processes from QEMU vCPUs this is not as wasteful
as it sounds. Some the helper processes are running trusted code, there
is no need for helper processes from different guests to be isolated.
They can all just live in the default core scheduling domain.

I feel like I'm talking myself into suggesting the core scheduling
host knob in qemu.conf needs to be more than just a single boolean.
Either have two knobs - one to turn it on/off and one to control
whether helpers are split or combined - or have one knob and make
it an enumeration.

One possible complication comes if we consider a guest that is
pinned, but not on the fine grained per-vCPU basis.

eg if guest is set to allow floating over a sub-set of host CPUs
we need to make sure that it is possible to actually execute the
guest still. ie if entire guest is pinned to 1 host CPU but our
config implies use of 2 distinct core scheduling domains, we have
an unsolvable constraint.

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

Re: [PATCH] qemu_hotplug: Deny changing @rss and @rss_hash_report attributes of virtio vNICs

2022-05-23 Thread Laine Stump


On 5/23/22 11:22 AM, Michal Privoznik wrote:

We have virDomainUpdateDeviceFlags() API that allows changing of
some attributes of a device whilst domain is still running (e.g.
setting different QoS, link state change on vNICs). But only very
limited set of attributes can be changed and we have to check
whether user isn't trying to sneak in a change that's not
allowed. Well, in case of a virtio vNIC we forgot to check for
@rss and @rss_hash_report attributes of .

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2082540
Signed-off-by: Michal Privoznik 
---
  src/qemu/qemu_hotplug.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)


Reviewed-by: Laine Stump 

(too bad there's not an elegant automated way of pointing out when a new 
field is added that can't be updated at runtime)




diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 24df66cc9f..f795568299 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3662,7 +3662,9 @@ qemuDomainChangeNet(virQEMUDriver *driver,
   olddev->driver.virtio.guest.tso4 != newdev->driver.virtio.guest.tso4 
||
   olddev->driver.virtio.guest.tso6 != newdev->driver.virtio.guest.tso6 
||
   olddev->driver.virtio.guest.ecn != newdev->driver.virtio.guest.ecn ||
- olddev->driver.virtio.guest.ufo != newdev->driver.virtio.guest.ufo)) {
+ olddev->driver.virtio.guest.ufo != newdev->driver.virtio.guest.ufo ||
+ olddev->driver.virtio.rss != newdev->driver.virtio.rss ||
+ olddev->driver.virtio.rss_hash_report != 
newdev->driver.virtio.rss_hash_report)) {
  virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
 _("cannot modify virtio network device driver 
attributes"));
  goto cleanup;

Re: [PATCH RFC 00/10] qemu: Enable SCHED_CORE for domains and helper processes

2022-05-23 Thread Daniel P . Berrangé

On Mon, May 09, 2022 at 05:02:07PM +0200, Michal Privoznik wrote:
> The Linux kernel offers a way to mitigate side channel attacks on Hyper
> Threads (e.g. MDS and L1TF). Long story short, userspace can define
> groups of processes (aka trusted groups) and only processes within one
> group can run on sibling Hyper Threads. The group membership is
> automatically preserved on fork() and exec().
> 
> Now, there is one scenario which I don't cover in my series and I'd like
> to hear proposal: if there are two guests with odd number of vCPUs they
> can no longer run on sibling Hyper Threads because my patches create
> separate group for each QEMU. This is a performance penalty. Ideally, we
> would have a knob inside domain XML that would place two or more domains
> into the same trusted group. But since there's pre-existing example (of
> sharing a piece of information between two domains) I've failed to come
> up with something usable.

Right now users have two choices

  - Run with SMT enabled. 100% of CPUs available. VMs are vulnerable
  - Run with SMT disabled. 50% of CPUs available. VMs are safe

What the core scheduling gives is somewhere inbetween, depending on
the vCPU count. If we assume all guests have even CPUs then

  - Run with SMT enabled + core scheduling. 100% of CPUs available.
100% of CPUs are used, VMs are safe

This is the ideal scenario, and probably the fairly common scenario
too as IMHO even number CPU counts are likely to be typical.

If we assume the worst case, of entirely 1 vCPU guests then we have

  - Run with SMT enabled + core scheduling. 100% of CPUs available.
50% of CPUs are used, VMs are safe

This feels highly unlikely though, as all except tiny workloads
want > 1 vCPU.

With entirely 3 vCPU guests then we have

  - Run with SMT enabled + core scheduling. 100% of CPUs available.
75% of CPUs are used, VMs are safe

With entirely 5 vCPU guests then we have

  - Run with SMT enabled + core scheduling. 100% of CPUs available.
83% of CPUs are used, VMs are safe

If we have a mix of even and odd numbered vCPU guests, with mostly
even numbered, then I think utilization will  be high enough that
almost no one will care about the last few %.

While we could try to come up with a way to express sharing of
cores between VMs I don't think its worth it, in the absence of
someone presenting compelling data why it'll be needed in a non
niche use case. Bear in mind, that users can also resort to
pinning VMs explicitly to get sharing.

In terms of defaults I'd very much like us to default to enabling
core scheduling, so that we have a secure deployment out of the box.
The only caveat is that this does have the potential to be interpreted
as a regression for existing deployments in some cases. Perhaps we
should make it a meson option for distros to decide whether to ship
with it turned on out of the box or not ?

I don't think we need core scheduling to be a VM XML config option,
because security is really a host level matter IMHO, such that it
does't make sense to have both secure & insecure VMs co-located.

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

[PATCH] nodedev: prevent internal error on dev_busid parse

2022-05-23 Thread Boris Fiuczynski

As "none" is a legal value represented in the sysfs attribute dev_busid
this patch prevents libvirt from incorrectly reporting an internal error.

Signed-off-by: Boris Fiuczynski 
Suggested-by: Michal Privoznik 
---
 src/node_device/node_device_udev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/node_device/node_device_udev.c 
b/src/node_device/node_device_udev.c
index 656130e98a..44ec22cf75 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1140,7 +1140,7 @@ udevProcessCSS(struct udev_device *device,
 /* process optional channel devices information */
 udevGetStringSysfsAttr(device, "dev_busid", _busid);
 
-if (dev_busid != NULL)
+if (dev_busid != NULL && STRNEQ(dev_busid, "none"))
 def->caps->data.ccw_dev.channel_dev_addr = 
virCCWDeviceAddressFromString(dev_busid);
 
 if (virNodeDeviceGetCSSDynamicCaps(def->sysfs_path, 
>caps->data.ccw_dev) < 0)
-- 
2.36.1

Re: [PATCH 00/17] nodedev: add optional device address of channel device to css device

2022-05-23 Thread Boris Fiuczynski


On 5/23/22 4:40 PM, Michal Prívozník wrote:

I'm fixing small memleaks I've raised in 16/17 and merging.

Reviewed-by: Michal Privoznik

Michal


Thanks Michal

--
Mit freundlichen Grüßen/Kind regards
   Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH 16/17] nodedev: add optional device address of channel device to css device

2022-05-23 Thread Boris Fiuczynski


On 5/23/22 4:40 PM, Michal Prívozník wrote:

On 5/13/22 12:31, Boris Fiuczynski wrote:

Add the new introduced sysfs attribute dev_busid which provides the address
of the device in the subchannel independent from the bound device driver.
It is added if available in the sysfs as optional channel_dev_addr element into
the css device capabilty providing the ccw deivce address attributes cssid,
ssid and devno.

Signed-off-by: Boris Fiuczynski 
---
  src/conf/node_device_conf.c| 26 ++
  src/conf/node_device_conf.h|  2 ++
  src/conf/schemas/nodedev.rng   |  5 +
  src/node_device/node_device_udev.c |  8 
  4 files changed, 41 insertions(+)

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 5aafcbb838..d5ee8da3ee 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -645,6 +645,17 @@ virNodeDeviceCapCSSDefFormat(virBuffer *buf,
  
  virNodeDeviceCapCCWDefFormat(buf, data);
  
+if (ccw_dev.channel_dev_addr) {

+virCCWDeviceAddress *ccw = ccw_dev.channel_dev_addr;
+virBufferAddLit(buf, "\n");
+virBufferAdjustIndent(buf, 2);
+virBufferAsprintf(buf, "0x%x\n", ccw->cssid);
+virBufferAsprintf(buf, "0x%x\n", ccw->ssid);
+virBufferAsprintf(buf, "0x%04x\n", ccw->devno);
+virBufferAdjustIndent(buf, -2);
+virBufferAddLit(buf, "\n");
+}
+
  if (ccw_dev.flags & VIR_NODE_DEV_CAP_FLAG_CSS_MDEV)
  virNodeDeviceCapMdevTypesFormat(buf,
  ccw_dev.mdev_types,
@@ -1257,6 +1268,8 @@ virNodeDevCapCSSParseXML(xmlXPathContextPtr ctxt,
  g_autofree xmlNodePtr *nodes = NULL;
  int n = 0;
  size_t i = 0;
+xmlNodePtr channel_ddno = NULL;
+g_autofree virCCWDeviceAddress *channel_dev = NULL;
  
  ctxt->node = node;
  
@@ -1271,6 +1284,19 @@ virNodeDevCapCSSParseXML(xmlXPathContextPtr ctxt,

  return -1;
  }
  
+// channel_dev_addr is optional


c89 style of comments please :-)


+if ((channel_ddno = virXPathNode("./channel_dev_addr[1]", ctxt))) {
+channel_dev = g_new0(virCCWDeviceAddress, 1);


This variable is not used anywhere else but in this block. It may be
worth declaring it within.


Yes, good idea.




+
+if (virNodeDevCCWDeviceAddressParseXML(ctxt,
+   channel_ddno,
+   def->name,
+   channel_dev) < 0)
+return -1;
+
+ccw_dev->channel_dev_addr = g_steal_pointer(_dev);


So this steals allocation from passed variable. But corresponding free()
call in virNodeDevCapsDefFree() is missing.


Ups, correct. Thanks for fixing it.




+}
+
  return 0;
  }
  
diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h

index e4d1f67d53..d1751ed874 100644
--- a/src/conf/node_device_conf.h
+++ b/src/conf/node_device_conf.h
@@ -24,6 +24,7 @@
  
  #include "internal.h"

  #include "virbitmap.h"
+#include "virccw.h"
  #include "virpcivpd.h"
  #include "virscsihost.h"
  #include "virpci.h"
@@ -279,6 +280,7 @@ struct _virNodeDevCapCCW {
  unsigned int flags; /* enum virNodeDevCCWCapFlags */
  virMediatedDeviceType **mdev_types;
  size_t nmdev_types;
+virCCWDeviceAddress *channel_dev_addr;
  };
  
  typedef struct _virNodeDevCapVDPA virNodeDevCapVDPA;

diff --git a/src/conf/schemas/nodedev.rng b/src/conf/schemas/nodedev.rng
index a9f83e048c..e40243e257 100644
--- a/src/conf/schemas/nodedev.rng
+++ b/src/conf/schemas/nodedev.rng
@@ -682,6 +682,11 @@
css
  
  
+
+  
+
+  
+
  

  
diff --git a/src/node_device/node_device_udev.c 
b/src/node_device/node_device_udev.c
index 4bb841c66b..16314627ca 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1124,6 +1124,8 @@ static int
  udevProcessCSS(struct udev_device *device,
 virNodeDeviceDef *def)
  {
+char *dev_busid;
+
  /* only process IO subchannel and vfio-ccw devices to keep the list sane 
*/
  if (!def->driver ||
  (STRNEQ(def->driver, "io_subchannel") &&
@@ -1135,6 +1137,12 @@ udevProcessCSS(struct udev_device *device,
  
  udevGenerateDeviceName(device, def, NULL);
  
+/* process optional channel devices information */

+udevGetStringSysfsAttr(device, "dev_busid", _busid);


This allocates dev_busid, which is never freed. it's sufficient to use
g_autofree for the variable.

Looking into the kernel sources, we may either get proper address here
or "none":

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/s390/cio/css.c#n433

Is it worth to bother with "none" string here?


OK, it's fair as it currently would result in a libvirt internal error.
I will send another patch with an additional check for "none" shortly.




+
+if (dev_busid != NULL)
+

[PATCH] qemu_hotplug: Deny changing @rss and @rss_hash_report attributes of virtio vNICs

2022-05-23 Thread Michal Privoznik

We have virDomainUpdateDeviceFlags() API that allows changing of
some attributes of a device whilst domain is still running (e.g.
setting different QoS, link state change on vNICs). But only very
limited set of attributes can be changed and we have to check
whether user isn't trying to sneak in a change that's not
allowed. Well, in case of a virtio vNIC we forgot to check for
@rss and @rss_hash_report attributes of .

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2082540
Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_hotplug.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 24df66cc9f..f795568299 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3662,7 +3662,9 @@ qemuDomainChangeNet(virQEMUDriver *driver,
  olddev->driver.virtio.guest.tso4 != newdev->driver.virtio.guest.tso4 
||
  olddev->driver.virtio.guest.tso6 != newdev->driver.virtio.guest.tso6 
||
  olddev->driver.virtio.guest.ecn != newdev->driver.virtio.guest.ecn ||
- olddev->driver.virtio.guest.ufo != newdev->driver.virtio.guest.ufo)) {
+ olddev->driver.virtio.guest.ufo != newdev->driver.virtio.guest.ufo ||
+ olddev->driver.virtio.rss != newdev->driver.virtio.rss ||
+ olddev->driver.virtio.rss_hash_report != 
newdev->driver.virtio.rss_hash_report)) {
 virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("cannot modify virtio network device driver 
attributes"));
 goto cleanup;
-- 
2.35.1

Re: [libvirt PATCH] qemu: Do not return NULL when qemuMigrationSrcBegin succeeds

2022-05-23 Thread Michal Prívozník

On 5/23/22 16:46, Jiri Denemark wrote:
> My recent commit v8.3.0-201-gc500955e95 tried to fix a regression which
> would cause the function to return success even if virCloseCallbacksSet
> failed. But due to a strange code flow in the function introduced an
> opposite regression. The function would return NULL on success when
> called without VIR_MIGRATE_CHANGE_PROTECTION flag.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_migration.c | 20 ++--
>  1 file changed, 10 insertions(+), 10 deletions(-)

Reviewed-by: Michal Privoznik 

Michal

[libvirt PATCH] qemu: Do not return NULL when qemuMigrationSrcBegin succeeds

2022-05-23 Thread Jiri Denemark

My recent commit v8.3.0-201-gc500955e95 tried to fix a regression which
would cause the function to return success even if virCloseCallbacksSet
failed. But due to a strange code flow in the function introduced an
opposite regression. The function would return NULL on success when
called without VIR_MIGRATE_CHANGE_PROTECTION flag.

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_migration.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 38596fa4de..6cc68a567a 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2534,23 +2534,23 @@ qemuMigrationSrcBegin(virConnectPtr conn,
 if (virCloseCallbacksSet(driver->closeCallbacks, vm, conn,
  qemuMigrationSrcCleanup) < 0)
 goto endjob;
-qemuMigrationJobContinue(vm);
-} else {
-goto endjob;
 }
 
 ret = g_steal_pointer();
 
+ endjob:
+if (flags & VIR_MIGRATE_CHANGE_PROTECTION) {
+if (ret)
+qemuMigrationJobContinue(vm);
+else
+qemuMigrationJobFinish(vm);
+} else {
+qemuDomainObjEndJob(vm);
+}
+
  cleanup:
 virDomainObjEndAPI();
 return ret;
-
- endjob:
-if (flags & VIR_MIGRATE_CHANGE_PROTECTION)
-qemuMigrationJobFinish(vm);
-else
-qemuDomainObjEndJob(vm);
-goto cleanup;
 }
 
 
-- 
2.35.1

Re: [PATCH 16/17] nodedev: add optional device address of channel device to css device

2022-05-23 Thread Michal Prívozník

On 5/13/22 12:31, Boris Fiuczynski wrote:
> Add the new introduced sysfs attribute dev_busid which provides the address
> of the device in the subchannel independent from the bound device driver.
> It is added if available in the sysfs as optional channel_dev_addr element 
> into
> the css device capabilty providing the ccw deivce address attributes cssid,
> ssid and devno.
> 
> Signed-off-by: Boris Fiuczynski 
> ---
>  src/conf/node_device_conf.c| 26 ++
>  src/conf/node_device_conf.h|  2 ++
>  src/conf/schemas/nodedev.rng   |  5 +
>  src/node_device/node_device_udev.c |  8 
>  4 files changed, 41 insertions(+)
> 
> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
> index 5aafcbb838..d5ee8da3ee 100644
> --- a/src/conf/node_device_conf.c
> +++ b/src/conf/node_device_conf.c
> @@ -645,6 +645,17 @@ virNodeDeviceCapCSSDefFormat(virBuffer *buf,
>  
>  virNodeDeviceCapCCWDefFormat(buf, data);
>  
> +if (ccw_dev.channel_dev_addr) {
> +virCCWDeviceAddress *ccw = ccw_dev.channel_dev_addr;
> +virBufferAddLit(buf, "\n");
> +virBufferAdjustIndent(buf, 2);
> +virBufferAsprintf(buf, "0x%x\n", ccw->cssid);
> +virBufferAsprintf(buf, "0x%x\n", ccw->ssid);
> +virBufferAsprintf(buf, "0x%04x\n", ccw->devno);
> +virBufferAdjustIndent(buf, -2);
> +virBufferAddLit(buf, "\n");
> +}
> +
>  if (ccw_dev.flags & VIR_NODE_DEV_CAP_FLAG_CSS_MDEV)
>  virNodeDeviceCapMdevTypesFormat(buf,
>  ccw_dev.mdev_types,
> @@ -1257,6 +1268,8 @@ virNodeDevCapCSSParseXML(xmlXPathContextPtr ctxt,
>  g_autofree xmlNodePtr *nodes = NULL;
>  int n = 0;
>  size_t i = 0;
> +xmlNodePtr channel_ddno = NULL;
> +g_autofree virCCWDeviceAddress *channel_dev = NULL;
>  
>  ctxt->node = node;
>  
> @@ -1271,6 +1284,19 @@ virNodeDevCapCSSParseXML(xmlXPathContextPtr ctxt,
>  return -1;
>  }
>  
> +// channel_dev_addr is optional

c89 style of comments please :-)

> +if ((channel_ddno = virXPathNode("./channel_dev_addr[1]", ctxt))) {
> +channel_dev = g_new0(virCCWDeviceAddress, 1);

This variable is not used anywhere else but in this block. It may be
worth declaring it within.

> +
> +if (virNodeDevCCWDeviceAddressParseXML(ctxt,
> +   channel_ddno,
> +   def->name,
> +   channel_dev) < 0)
> +return -1;
> +
> +ccw_dev->channel_dev_addr = g_steal_pointer(_dev);

So this steals allocation from passed variable. But corresponding free()
call in virNodeDevCapsDefFree() is missing.

> +}
> +
>  return 0;
>  }
>  
> diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
> index e4d1f67d53..d1751ed874 100644
> --- a/src/conf/node_device_conf.h
> +++ b/src/conf/node_device_conf.h
> @@ -24,6 +24,7 @@
>  
>  #include "internal.h"
>  #include "virbitmap.h"
> +#include "virccw.h"
>  #include "virpcivpd.h"
>  #include "virscsihost.h"
>  #include "virpci.h"
> @@ -279,6 +280,7 @@ struct _virNodeDevCapCCW {
>  unsigned int flags; /* enum virNodeDevCCWCapFlags */
>  virMediatedDeviceType **mdev_types;
>  size_t nmdev_types;
> +virCCWDeviceAddress *channel_dev_addr;
>  };
>  
>  typedef struct _virNodeDevCapVDPA virNodeDevCapVDPA;
> diff --git a/src/conf/schemas/nodedev.rng b/src/conf/schemas/nodedev.rng
> index a9f83e048c..e40243e257 100644
> --- a/src/conf/schemas/nodedev.rng
> +++ b/src/conf/schemas/nodedev.rng
> @@ -682,6 +682,11 @@
>css
>  
>  
> +
> +  
> +
> +  
> +
>  
>
>  
> diff --git a/src/node_device/node_device_udev.c 
> b/src/node_device/node_device_udev.c
> index 4bb841c66b..16314627ca 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1124,6 +1124,8 @@ static int
>  udevProcessCSS(struct udev_device *device,
> virNodeDeviceDef *def)
>  {
> +char *dev_busid;
> +
>  /* only process IO subchannel and vfio-ccw devices to keep the list sane 
> */
>  if (!def->driver ||
>  (STRNEQ(def->driver, "io_subchannel") &&
> @@ -1135,6 +1137,12 @@ udevProcessCSS(struct udev_device *device,
>  
>  udevGenerateDeviceName(device, def, NULL);
>  
> +/* process optional channel devices information */
> +udevGetStringSysfsAttr(device, "dev_busid", _busid);

This allocates dev_busid, which is never freed. it's sufficient to use
g_autofree for the variable.

Looking into the kernel sources, we may either get proper address here
or "none":

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/s390/cio/css.c#n433

Is it worth to bother with "none" string here?

> +
> +if (dev_busid != NULL)
> +def->caps->data.ccw_dev.channel_dev_addr = 
>

Re: [PATCH 00/17] nodedev: add optional device address of channel device to css device

2022-05-23 Thread Michal Prívozník

On 5/13/22 12:30, Boris Fiuczynski wrote:
> While this series started with the intend to add the optional device
> address of a subchannel device to the nodedev css device the outcome now
> also includes a small fix in the error reporting of css cap XML parsing
> as well as a refactoring of generic ccw code into virccw in utils.
> 
> Boris Fiuczynski (17):
>   nodedev: fix reported error msg in css cap XML parsing
>   util: refactor virDomainDeviceCCWAddress into virccw.h
>   util: refactor virDomainCCWAddressAsString into virccw
>   util: make reuse of ccw device address format constant
>   util: refactor ccw address constants into virccw
>   util: refactor virDomainCCWAddressIncrement into virccw
>   util: refactor virDomainDeviceCCWAddressIsValid into virccw
>   util: refactor virDomainDeviceCCWAddressEqual into virccw
>   conf: adjust method name virDomainDeviceCCWAddressParseXML
>   util: add ccw device address parsing into virccw
>   util: add virCCWDeviceAddressFromString to virccw
>   nodedev: refactor css format from ccw format method
>   nodedev: refactor ccw device address parsing from XML
>   nodedev: refactor css XML parsing from ccw XML parsing
>   schemas: refactor out nodedev ccw address schema
>   nodedev: add optional device address of channel device to css device
>   nodedev: add tests for optional device address to css device
> 
>  po/POTFILES.in|   1 +
>  src/conf/device_conf.c|  28 +--
>  src/conf/device_conf.h|  23 +--
>  src/conf/domain_addr.c|  28 +--
>  src/conf/domain_addr.h|   5 +-
>  src/conf/domain_conf.c|  19 +-
>  src/conf/domain_conf.h|   6 +-
>  src/conf/node_device_conf.c   | 171 +-
>  src/conf/node_device_conf.h   |   2 +
>  src/conf/schemas/nodedev.rng  |  27 +--
>  src/libvirt_private.syms  |  14 +-
>  src/node_device/node_device_driver.c  |   4 +-
>  src/node_device/node_device_udev.c|  16 +-
>  src/qemu/qemu_agent.c |   4 +-
>  src/qemu/qemu_agent.h |   2 +-
>  src/qemu/qemu_command.c   |   2 +-
>  src/util/meson.build  |   1 +
>  src/util/virccw.c | 103 +++
>  src/util/virccw.h |  52 ++
>  .../css_0_0_1-invalid.xml |  10 +
>  ...s_0_0_fffe_mdev_types_channel_dev_addr.xml |  22 +++
>  .../css_0_0__channel_dev_addr-invalid.xml |  15 ++
>  .../css_0_0__channel_dev_addr.xml |  15 ++
>  ...s_0_0_fffe_mdev_types_channel_dev_addr.xml |   1 +
>  .../css_0_0__channel_dev_addr.xml |   1 +
>  tests/nodedevxml2xmltest.c|   2 +
>  26 files changed, 418 insertions(+), 156 deletions(-)
>  create mode 100644 src/util/virccw.c
>  create mode 100644 src/util/virccw.h
>  create mode 100644 tests/nodedevschemadata/css_0_0_1-invalid.xml
>  create mode 100644 
> tests/nodedevschemadata/css_0_0_fffe_mdev_types_channel_dev_addr.xml
>  create mode 100644 
> tests/nodedevschemadata/css_0_0__channel_dev_addr-invalid.xml
>  create mode 100644 tests/nodedevschemadata/css_0_0__channel_dev_addr.xml
>  create mode 12 
> tests/nodedevxml2xmlout/css_0_0_fffe_mdev_types_channel_dev_addr.xml
>  create mode 12 tests/nodedevxml2xmlout/css_0_0__channel_dev_addr.xml
> 

I'm fixing small memleaks I've raised in 16/17 and merging.

Reviewed-by: Michal Privoznik 

Michal

[PATCH 3/3] network: Generate TFTP config regardless of DHCP

2022-05-23 Thread Michal Privoznik

We already allow users to provide TFTP root path in network XML
and not specify any DHCP. This makes sense, because dnsmasq is
not only DHCP server but also TFTP server and users might have
a DHCP server configured on their own, outside of libvirt's
control and want just the TFTP part.

By moving TFTP config generator out of DHCP generator and calling
it for every IPv4 range, users can finally enable just TFTP.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2026765
Signed-off-by: Michal Privoznik 
---
 src/network/bridge_driver.c   | 30 ++-
 .../networkxml2confdata/netboot-network.conf  |  4 +--
 tests/networkxml2confdata/netboot-tftp.conf   | 13 
 tests/networkxml2confdata/netboot-tftp.xml|  9 ++
 tests/networkxml2conftest.c   |  1 +
 tests/networkxml2xmlin/netboot-tftp.xml   |  1 +
 tests/networkxml2xmlout/netboot-tftp.xml  |  1 +
 tests/networkxml2xmltest.c|  1 +
 8 files changed, 50 insertions(+), 10 deletions(-)
 create mode 100644 tests/networkxml2confdata/netboot-tftp.conf
 create mode 100644 tests/networkxml2confdata/netboot-tftp.xml
 create mode 12 tests/networkxml2xmlin/netboot-tftp.xml
 create mode 12 tests/networkxml2xmlout/netboot-tftp.xml

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index f087e07c52..7449c7e02a 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1088,11 +1088,6 @@ networkDnsmasqConfDHCP(virBuffer *buf,
 virBufferAddLit(buf, "dhcp-authoritative\n");
 }
 
-if (ipdef->tftproot) {
-virBufferAddLit(buf, "enable-tftp\n");
-virBufferAsprintf(buf, "tftp-root=%s\n", ipdef->tftproot);
-}
-
 if (ipdef->bootfile) {
 if (VIR_SOCKET_ADDR_VALID(>bootserver)) {
 g_autofree char *bootserver = 
virSocketAddrFormat(>bootserver);
@@ -,6 +1106,22 @@ networkDnsmasqConfDHCP(virBuffer *buf,
 }
 
 
+static void
+networkDnsmasqConfTFTP(virBuffer *buf,
+   virNetworkIPDef *ipdef,
+   bool *enableTFTP)
+{
+if (!ipdef->tftproot)
+return;
+
+if (!*enableTFTP) {
+virBufferAddLit(buf, "enable-tftp\n");
+*enableTFTP = true;
+}
+virBufferAsprintf(buf, "tftp-root=%s\n", ipdef->tftproot);
+}
+
+
 int
 networkDnsmasqConfContents(virNetworkObj *obj,
const char *pidfile,
@@ -1129,6 +1140,7 @@ networkDnsmasqConfContents(virNetworkObj *obj,
 virNetworkIPDef *ipv4def = NULL;
 virNetworkIPDef *ipv6def = NULL;
 bool ipv6SLAAC = false;
+bool enableTFTP = false;
 
 *configstr = NULL;
 
@@ -1339,6 +1351,8 @@ networkDnsmasqConfContents(virNetworkObj *obj,
 ipv4def = ipdef;
 }
 }
+
+networkDnsmasqConfTFTP(, ipdef, );
 }
 if (VIR_SOCKET_ADDR_IS_FAMILY(>address, AF_INET6)) {
 if (ipdef->nranges || ipdef->nhosts) {
@@ -1500,7 +1514,7 @@ networkStartDhcpDaemon(virNetworkDriverState *driver,
 i = 0;
 while ((ipdef = virNetworkDefGetIPByIndex(def, AF_UNSPEC, i))) {
 i++;
-if (ipdef->nranges || ipdef->nhosts)
+if (ipdef->nranges || ipdef->nhosts || ipdef->tftproot)
 needDnsmasq = true;
 }
 
@@ -3255,7 +3269,7 @@ networkUpdate(virNetworkPtr net,
 for (i = 0;
  (ipdef = virNetworkDefGetIPByIndex(def, AF_INET, i));
  i++) {
-if (ipdef->nranges || ipdef->nhosts) {
+if (ipdef->nranges || ipdef->nhosts || ipdef->tftproot) {
 oldDhcpActive = true;
 break;
 }
@@ -3370,7 +3384,7 @@ networkUpdate(virNetworkPtr net,
 
 for (i = 0; (ipdef = virNetworkDefGetIPByIndex(def, AF_INET, i));
  i++) {
-if (ipdef->nranges || ipdef->nhosts) {
+if (ipdef->nranges || ipdef->nhosts || ipdef->tftproot) {
 newDhcpActive = true;
 break;
 }
diff --git a/tests/networkxml2confdata/netboot-network.conf 
b/tests/networkxml2confdata/netboot-network.conf
index a13239a54f..32ef25b05f 100644
--- a/tests/networkxml2confdata/netboot-network.conf
+++ b/tests/networkxml2confdata/netboot-network.conf
@@ -10,11 +10,11 @@ expand-hosts
 except-interface=lo
 bind-dynamic
 interface=virbr1
+enable-tftp
+tftp-root=/var/lib/tftproot
 dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0
 dhcp-no-override
 dhcp-authoritative
-enable-tftp
-tftp-root=/var/lib/tftproot
 dhcp-boot=pxeboot.img
 dhcp-lease-max=253
 dhcp-hostsfile=/var/lib/libvirt/dnsmasq/netboot.hostsfile
diff --git a/tests/networkxml2confdata/netboot-tftp.conf 
b/tests/networkxml2confdata/netboot-tftp.conf
new file mode 100644
index 00..45615f3c33
--- /dev/null
+++ b/tests/networkxml2confdata/netboot-tftp.conf
@@ -0,0 +1,13 @@
+##WARNING:  THIS IS AN AUTO-GENERATED FILE. CHANGES TO IT ARE LIKELY TO BE

[PATCH 2/3] network: Separate DHCP config generator into a function

2022-05-23 Thread Michal Privoznik

Generating configuration file for dnsmasq is done in
networkDnsmasqConfContents() which is this big, self-contained
function. Separate at least DHCP part into its own function for
better readability.

Signed-off-by: Michal Privoznik 
---
 src/network/bridge_driver.c | 227 +++-
 1 file changed, 120 insertions(+), 107 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 10099571c2..f087e07c52 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -994,6 +994,123 @@ networkDnsmasqConfLocalPTRs(virBuffer *buf,
 }
 
 
+static int
+networkDnsmasqConfDHCP(virBuffer *buf,
+   virNetworkIPDef *ipdef,
+   const char *bridge,
+   int *nbleases,
+   dnsmasqContext *dctx)
+{
+int r;
+int prefix;
+
+if (!ipdef)
+return 0;
+
+prefix = virNetworkIPDefPrefix(ipdef);
+if (prefix < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("bridge '%s' has an invalid prefix"),
+   bridge);
+return -1;
+}
+for (r = 0; r < ipdef->nranges; r++) {
+int thisRange;
+virNetworkDHCPRangeDef range = ipdef->ranges[r];
+g_autofree char *leasetime = NULL;
+g_autofree char *saddr = NULL;
+g_autofree char *eaddr = NULL;
+
+if (!(saddr = virSocketAddrFormat()) ||
+!(eaddr = virSocketAddrFormat()))
+return -1;
+
+if (VIR_SOCKET_ADDR_IS_FAMILY(>address, AF_INET6)) {
+virBufferAsprintf(buf, "dhcp-range=%s,%s,%d",
+  saddr, eaddr, prefix);
+} else {
+/* IPv4 - dnsmasq requires a netmask rather than prefix */
+virSocketAddr netmask;
+g_autofree char *netmaskStr = NULL;
+
+if (virSocketAddrPrefixToNetmask(prefix, , AF_INET) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Failed to translate bridge '%s' "
+ "prefix %d to netmask"),
+   bridge, prefix);
+return -1;
+}
+
+if (!(netmaskStr = virSocketAddrFormat()))
+return -1;
+virBufferAsprintf(buf, "dhcp-range=%s,%s,%s",
+  saddr, eaddr, netmaskStr);
+}
+
+if ((leasetime = networkBuildDnsmasqLeaseTime(range.lease)))
+virBufferAsprintf(buf, ",%s", leasetime);
+
+virBufferAddLit(buf, "\n");
+
+thisRange = virSocketAddrGetRange(,
+  ,
+  >address,
+  virNetworkIPDefPrefix(ipdef));
+if (thisRange < 0)
+return -1;
+*nbleases += thisRange;
+}
+
+/*
+ * For static-only DHCP, i.e. with no range but at least one
+ * host element, we have to add a special --dhcp-range option
+ * to enable the service in dnsmasq. (this is for dhcp-hosts=
+ * support)
+ */
+if (!ipdef->nranges && ipdef->nhosts) {
+g_autofree char *bridgeaddr = virSocketAddrFormat(>address);
+if (!bridgeaddr)
+return -1;
+virBufferAsprintf(buf, "dhcp-range=%s,static",
+  bridgeaddr);
+if (VIR_SOCKET_ADDR_IS_FAMILY(>address, AF_INET6))
+virBufferAsprintf(buf, ",%d", prefix);
+virBufferAddLit(buf, "\n");
+}
+
+if (networkBuildDnsmasqDhcpHostsList(dctx, ipdef) < 0)
+return -1;
+
+/* Note: the following is IPv4 only */
+if (VIR_SOCKET_ADDR_IS_FAMILY(>address, AF_INET)) {
+if (ipdef->nranges || ipdef->nhosts) {
+virBufferAddLit(buf, "dhcp-no-override\n");
+virBufferAddLit(buf, "dhcp-authoritative\n");
+}
+
+if (ipdef->tftproot) {
+virBufferAddLit(buf, "enable-tftp\n");
+virBufferAsprintf(buf, "tftp-root=%s\n", ipdef->tftproot);
+}
+
+if (ipdef->bootfile) {
+if (VIR_SOCKET_ADDR_VALID(>bootserver)) {
+g_autofree char *bootserver = 
virSocketAddrFormat(>bootserver);
+
+if (!bootserver)
+return -1;
+virBufferAsprintf(buf, "dhcp-boot=%s%s%s\n",
+  ipdef->bootfile, ",,", bootserver);
+} else {
+virBufferAsprintf(buf, "dhcp-boot=%s\n", ipdef->bootfile);
+}
+}
+}
+
+return 0;
+}
+
+
 int
 networkDnsmasqConfContents(virNetworkObj *obj,
const char *pidfile,
@@ -1248,113 +1365,9 @@ networkDnsmasqConfContents(virNetworkObj *obj,
  "on the same network interface.");
 }
 
-ipdef = ipv4def ? ipv4def : ipv6def;
-
-while (ipdef) {
-int prefix;
-int r;
-
-prefix =

[PATCH 1/3] network: Initialize variables in networkDnsmasqConfContents()

2022-05-23 Thread Michal Privoznik

In networkDnsmasqConfContents() there's a for() loop which
initializes some variables in its initialization block. This
makes both the loop() statement and variable declaration block
look needlessly ugly. Speaking of variable declaration, also move
some variables which are used only within blocks into their
respective blocks.

Signed-off-by: Michal Privoznik 
---
 src/network/bridge_driver.c | 14 ++
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index ca1e0ca50f..10099571c2 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1004,15 +1004,14 @@ networkDnsmasqConfContents(virNetworkObj *obj,
 {
 virNetworkDef *def = virNetworkObjGetDef(obj);
 g_auto(virBuffer) configbuf = VIR_BUFFER_INITIALIZER;
-int r;
 int nbleases = 0;
 size_t i;
 virNetworkDNSDef *dns = >dns;
 bool wantDNS = dns->enable != VIR_TRISTATE_BOOL_NO;
-virNetworkIPDef *ipdef;
-virNetworkIPDef *ipv4def;
-virNetworkIPDef *ipv6def;
-bool ipv6SLAAC;
+virNetworkIPDef *ipdef = NULL;
+virNetworkIPDef *ipv4def = NULL;
+virNetworkIPDef *ipv6def = NULL;
+bool ipv6SLAAC = false;
 
 *configstr = NULL;
 
@@ -1211,9 +1210,7 @@ networkDnsmasqConfContents(virNetworkObj *obj,
 }
 
 /* Find the first dhcp for both IPv4 and IPv6 */
-for (i = 0, ipv4def = NULL, ipv6def = NULL, ipv6SLAAC = false;
- (ipdef = virNetworkDefGetIPByIndex(def, AF_UNSPEC, i));
- i++) {
+for (i = 0; (ipdef = virNetworkDefGetIPByIndex(def, AF_UNSPEC, i)); i++) {
 if (VIR_SOCKET_ADDR_IS_FAMILY(>address, AF_INET)) {
 if (ipdef->nranges || ipdef->nhosts) {
 if (ipv4def) {
@@ -1255,6 +1252,7 @@ networkDnsmasqConfContents(virNetworkObj *obj,
 
 while (ipdef) {
 int prefix;
+int r;
 
 prefix = virNetworkIPDefPrefix(ipdef);
 if (prefix < 0) {
-- 
2.35.1

[PATCH RESEND 0/3] network: Generate TFTP config regardless of DHCP

2022-05-23 Thread Michal Privoznik

This is rebased version of:

https://listman.redhat.com/archives/libvir-list/2021-December/226045.html

Michal Prívozník (3):
  network: Initialize variables in networkDnsmasqConfContents()
  network: Separate DHCP config generator into a function
  network: Generate TFTP config regardless of DHCP

 src/network/bridge_driver.c   | 259 ++
 .../networkxml2confdata/netboot-network.conf  |   4 +-
 tests/networkxml2confdata/netboot-tftp.conf   |  13 +
 tests/networkxml2confdata/netboot-tftp.xml|   9 +
 tests/networkxml2conftest.c   |   1 +
 tests/networkxml2xmlin/netboot-tftp.xml   |   1 +
 tests/networkxml2xmlout/netboot-tftp.xml  |   1 +
 tests/networkxml2xmltest.c|   1 +
 8 files changed, 170 insertions(+), 119 deletions(-)
 create mode 100644 tests/networkxml2confdata/netboot-tftp.conf
 create mode 100644 tests/networkxml2confdata/netboot-tftp.xml
 create mode 12 tests/networkxml2xmlin/netboot-tftp.xml
 create mode 12 tests/networkxml2xmlout/netboot-tftp.xml

-- 
2.35.1

[PATCH 12/17] virDomainChrDefParseXML: Switch to virXMLPropEnumDefault()

2022-05-23 Thread Michal Privoznik

The virDomainChrDefParseXML() function uses old style of parsing
XML (virXMLPropString + str2enum conversion). Use
virXMLPropEnumDefault() which encapsulates those steps.

Signed-off-by: Michal Privoznik 
---
 src/ch/ch_monitor.c  |  2 +-
 src/conf/domain_conf.c   | 82 +++-
 src/conf/domain_conf.h   |  2 +-
 src/conf/domain_validate.c   |  2 +-
 src/conf/virchrdev.c | 29 +++
 src/libxl/libxl_conf.c   | 20 
 src/libxl/xen_common.c   | 23 -
 src/qemu/qemu_command.c  |  6 +--
 src/qemu/qemu_domain.c   | 34 -
 src/qemu/qemu_monitor_json.c |  2 +-
 src/qemu/qemu_process.c  |  2 +-
 src/qemu/qemu_validate.c |  2 +-
 src/security/security_apparmor.c |  4 +-
 src/security/security_dac.c  |  4 +-
 src/security/security_selinux.c  | 24 ++
 src/vmx/vmx.c| 26 ++
 tests/testutilsqemu.c|  2 +-
 17 files changed, 197 insertions(+), 69 deletions(-)

diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
index 2c6b83a1b5..d6fac642da 100644
--- a/src/ch/ch_monitor.c
+++ b/src/ch/ch_monitor.c
@@ -281,7 +281,7 @@ virCHMonitorBuildNetJson(virJSONValue *nets,
 }
 break;
 case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
-if ((virDomainChrType)netdef->data.vhostuser->type != 
VIR_DOMAIN_CHR_TYPE_UNIX) {
+if (netdef->data.vhostuser->type != VIR_DOMAIN_CHR_TYPE_UNIX) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("vhost_user type support UNIX socket in this 
CH"));
 return -1;
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 44b507b74d..b5ce80eb76 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2694,7 +2694,7 @@ virDomainChrSourceDefGetPath(virDomainChrSourceDef *chr)
 if (!chr)
 return NULL;
 
-switch ((virDomainChrType) chr->type) {
+switch (chr->type) {
 case VIR_DOMAIN_CHR_TYPE_PTY:
 case VIR_DOMAIN_CHR_TYPE_DEV:
 case VIR_DOMAIN_CHR_TYPE_FILE:
@@ -2761,6 +2761,13 @@ virDomainChrSourceDefClear(virDomainChrSourceDef *def)
 case VIR_DOMAIN_CHR_TYPE_DBUS:
 VIR_FREE(def->data.dbus.channel);
 break;
+case VIR_DOMAIN_CHR_TYPE_NULL:
+case VIR_DOMAIN_CHR_TYPE_VC:
+case VIR_DOMAIN_CHR_TYPE_STDIO:
+case VIR_DOMAIN_CHR_TYPE_SPICEVMC:
+case VIR_DOMAIN_CHR_TYPE_QEMU_VDAGENT:
+case VIR_DOMAIN_CHR_TYPE_LAST:
+break;
 }
 
 VIR_FREE(def->logfile);
@@ -2778,7 +2785,7 @@ virDomainChrSourceDefCopy(virDomainChrSourceDef *dest,
 dest->logfile = g_strdup(src->logfile);
 dest->logappend = src->logappend;
 
-switch ((virDomainChrType)src->type) {
+switch (src->type) {
 case VIR_DOMAIN_CHR_TYPE_FILE:
 case VIR_DOMAIN_CHR_TYPE_PTY:
 case VIR_DOMAIN_CHR_TYPE_DEV:
@@ -2875,7 +2882,7 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef 
*src,
 if (tgt->type != src->type)
 return false;
 
-switch ((virDomainChrType)src->type) {
+switch (src->type) {
 case VIR_DOMAIN_CHR_TYPE_FILE:
 return src->data.file.append == tgt->data.file.append &&
 STREQ_NULLABLE(src->data.file.path, tgt->data.file.path);
@@ -11270,7 +11277,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDef 
*def,
 goto error;
 }
 
-switch ((virDomainChrType) def->type) {
+switch (def->type) {
 case VIR_DOMAIN_CHR_TYPE_FILE:
 if (virDomainChrSourceDefParseFile(def, sources[0]) < 0)
 goto error;
@@ -11477,7 +11484,6 @@ virDomainChrDefParseXML(virDomainXMLOption *xmlopt,
 xmlNodePtr target;
 const char *nodeName;
 virDomainChrDef *def;
-g_autofree char *type = NULL;
 VIR_XPATH_NODE_AUTORESTORE(ctxt)
 
 ctxt->node = node;
@@ -11485,15 +11491,12 @@ virDomainChrDefParseXML(virDomainXMLOption *xmlopt,
 if (!(def = virDomainChrDefNew(xmlopt)))
 return NULL;
 
-type = virXMLPropString(node, "type");
-if (type == NULL) {
-def->source->type = VIR_DOMAIN_CHR_TYPE_PTY;
-} else if ((def->source->type = virDomainChrTypeFromString(type)) < 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("unknown type presented to host for character device: 
%s"),
-   type);
+if (virXMLPropEnumDefault(node, "type",
+  virDomainChrTypeFromString,
+  VIR_XML_PROP_NONE,
+  >source->type,
+  VIR_DOMAIN_CHR_TYPE_PTY) < 0)
 goto error;
-}
 
 nodeName = (const char *) node->name;
 if ((def->deviceType = virDomainChrDeviceTypeFromString(nodeName)) < 0) {
@@ -11551,7 +11554,6 @@ virDomainSmartcardDefParseXML(virDomainXMLOption 
*xmlopt,
   unsigned int flags)
 {

[PATCH 11/17] virDomainDiskDefParseSourceXML: Switch to virXMLPropEnumDefault()

2022-05-23 Thread Michal Privoznik

The virDomainDiskDefParseSourceXML() function uses old style of
parsing XML (virXMLPropString + str2enum conversion). Use
virXMLPropEnumDefault() which encapsulates those steps.

Signed-off-by: Michal Privoznik 
---
 src/conf/domain_conf.c | 14 +-
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c42ac9c0c9..44b507b74d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9039,15 +9039,11 @@ virDomainDiskDefParseSourceXML(virDomainXMLOption 
*xmlopt,
 if (virDomainStorageSourceParse(tmp, ctxt, src, flags, xmlopt) < 0)
 return NULL;
 
-if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) {
-g_autofree char *sourceindex = NULL;
-
-if ((sourceindex = virXMLPropString(tmp, "index")) &&
-virStrToLong_uip(sourceindex, NULL, 10, >id) < 0) {
-virReportError(VIR_ERR_XML_ERROR,
-   _("invalid disk index '%s'"), sourceindex);
-return NULL;
-}
+if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) &&
+virXMLPropUInt(tmp, "index", 10,
+   VIR_XML_PROP_NONE,
+   >id) < 0) {
+return NULL;
 }
 } else {
 /* Reset src->type in case when 'source' was not present */
-- 
2.35.1

[PATCH 16/17] virDomainHubDefParseXML: Switch to virXMLPropEnumDefault()

2022-05-23 Thread Michal Privoznik

The virDomainHubDefParseXML() function uses old style of parsing
XML (virXMLPropString + str2enum conversion). Use
virXMLPropEnumDefault() which encapsulates those steps.

Signed-off-by: Michal Privoznik 
---
 src/conf/domain_conf.c | 17 -
 src/conf/domain_conf.h | 14 +++---
 2 files changed, 11 insertions(+), 20 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 44ab79c1f0..52a34cd131 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -11934,23 +11934,14 @@ virDomainHubDefParseXML(virDomainXMLOption *xmlopt,
 unsigned int flags)
 {
 virDomainHubDef *def;
-g_autofree char *type = NULL;
 
 def = g_new0(virDomainHubDef, 1);
 
-type = virXMLPropString(node, "type");
-
-if (!type) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   "%s", _("missing hub device type"));
+if (virXMLPropEnum(node, "type",
+   virDomainHubTypeFromString,
+   VIR_XML_PROP_REQUIRED,
+   >type) < 0)
 goto error;
-}
-
-if ((def->type = virDomainHubTypeFromString(type)) < 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("unknown hub device type '%s'"), type);
-goto error;
-}
 
 if (virDomainDeviceInfoParseXML(xmlopt, node, ctxt, >info, flags) < 0)
 goto error;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index b3d51565e3..cad19a3d5d 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1381,8 +1381,14 @@ struct _virDomainSmartcardDef {
 virDomainDeviceInfo info;
 };
 
+typedef enum {
+VIR_DOMAIN_HUB_TYPE_USB,
+
+VIR_DOMAIN_HUB_TYPE_LAST
+} virDomainHubType;
+
 struct _virDomainHubDef {
-int type;
+virDomainHubType type;
 virDomainDeviceInfo info;
 };
 
@@ -1881,12 +1887,6 @@ typedef enum {
 VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_LAST
 } virDomainGraphicsListenType;
 
-typedef enum {
-VIR_DOMAIN_HUB_TYPE_USB,
-
-VIR_DOMAIN_HUB_TYPE_LAST
-} virDomainHubType;
-
 struct _virDomainGraphicsListenDef {
 virDomainGraphicsListenType type;
 char *address;
-- 
2.35.1

[PATCH 15/17] virDomainInputDefParseXML: Switch to virXMLPropEnumDefault()

2022-05-23 Thread Michal Privoznik

The virDomainInputDefParseXML() function uses old style of
parsing XML (virXMLPropString + str2enum conversion). Use
virXMLPropEnumDefault() which encapsulates those steps.

Signed-off-by: Michal Privoznik 
---
 src/conf/domain_audit.c  |  2 +-
 src/conf/domain_conf.c   | 86 +---
 src/conf/domain_conf.h   |  6 +--
 src/conf/domain_validate.c   |  2 +-
 src/libxl/libxl_conf.c   |  4 ++
 src/libxl/xen_xl.c   |  3 ++
 src/libxl/xen_xm.c   |  3 ++
 src/qemu/qemu_cgroup.c   | 12 +
 src/qemu/qemu_command.c  | 10 ++--
 src/qemu/qemu_domain_address.c   |  4 +-
 src/qemu/qemu_hotplug.c  |  4 +-
 src/qemu/qemu_validate.c |  4 +-
 src/security/security_apparmor.c |  2 +-
 src/security/security_dac.c  |  4 +-
 src/security/security_selinux.c  |  4 +-
 15 files changed, 80 insertions(+), 70 deletions(-)

diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c
index 17a01c51ba..9ce14de80b 100644
--- a/src/conf/domain_audit.c
+++ b/src/conf/domain_audit.c
@@ -949,7 +949,7 @@ virDomainAuditInput(virDomainObj *vm,
 if (!(vmname = virAuditEncode("vm", vm->def->name)))
 return;
 
-switch ((virDomainInputType) input->type) {
+switch (input->type) {
 case VIR_DOMAIN_INPUT_TYPE_MOUSE:
 case VIR_DOMAIN_INPUT_TYPE_TABLET:
 case VIR_DOMAIN_INPUT_TYPE_KBD:
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 85aae73873..44ab79c1f0 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1942,7 +1942,7 @@ void virDomainGraphicsDefFree(virDomainGraphicsDef *def)
 
 const char *virDomainInputDefGetPath(virDomainInputDef *input)
 {
-switch ((virDomainInputType) input->type) {
+switch (input->type) {
 case VIR_DOMAIN_INPUT_TYPE_MOUSE:
 case VIR_DOMAIN_INPUT_TYPE_TABLET:
 case VIR_DOMAIN_INPUT_TYPE_KBD:
@@ -11818,70 +11818,54 @@ virDomainInputDefParseXML(virDomainXMLOption *xmlopt,
 {
 VIR_XPATH_NODE_AUTORESTORE(ctxt)
 virDomainInputDef *def;
-g_autofree char *type = NULL;
-g_autofree char *bus = NULL;
-g_autofree char *model = NULL;
+virDomainInputBus bus = VIR_DOMAIN_INPUT_BUS_PS2;
 xmlNodePtr source = NULL;
 
 def = g_new0(virDomainInputDef, 1);
 
 ctxt->node = node;
 
-type = virXMLPropString(node, "type");
-bus = virXMLPropString(node, "bus");
-model = virXMLPropString(node, "model");
-
-if (!type) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   "%s", _("missing input device type"));
-goto error;
-}
-
-if ((def->type = virDomainInputTypeFromString(type)) < 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("unknown input device type '%s'"), type);
+if (virXMLPropEnum(node, "type",
+   virDomainInputTypeFromString,
+   VIR_XML_PROP_REQUIRED,
+   >type) < 0)
 goto error;
-}
 
-if (model &&
-((def->model = virDomainInputModelTypeFromString(model)) < 0 ||
- def->model == VIR_DOMAIN_INPUT_MODEL_DEFAULT)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("unknown input model '%s'"), model);
+if (virXMLPropEnum(node, "model",
+   virDomainInputModelTypeFromString,
+   VIR_XML_PROP_NONZERO,
+   >model) < 0)
 goto error;
-}
-
-if (bus) {
-if ((def->bus = virDomainInputBusTypeFromString(bus)) < 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("unknown input bus type '%s'"), bus);
-goto error;
-}
 
-} else {
-if (dom->os.type == VIR_DOMAIN_OSTYPE_HVM) {
-if ((def->type == VIR_DOMAIN_INPUT_TYPE_MOUSE ||
-def->type == VIR_DOMAIN_INPUT_TYPE_KBD) &&
-(ARCH_IS_X86(dom->os.arch) || dom->os.arch == VIR_ARCH_NONE)) {
-def->bus = VIR_DOMAIN_INPUT_BUS_PS2;
-} else if (ARCH_IS_S390(dom->os.arch) ||
-   def->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH) {
-def->bus = VIR_DOMAIN_INPUT_BUS_VIRTIO;
-} else if (def->type == VIR_DOMAIN_INPUT_TYPE_EVDEV) {
-def->bus = VIR_DOMAIN_INPUT_BUS_NONE;
-} else {
-def->bus = VIR_DOMAIN_INPUT_BUS_USB;
-}
-} else if (dom->os.type == VIR_DOMAIN_OSTYPE_XEN ||
-   dom->os.type == VIR_DOMAIN_OSTYPE_XENPVH) {
-def->bus = VIR_DOMAIN_INPUT_BUS_XEN;
+if (dom->os.type == VIR_DOMAIN_OSTYPE_HVM) {
+if ((def->type == VIR_DOMAIN_INPUT_TYPE_MOUSE ||
+ def->type == VIR_DOMAIN_INPUT_TYPE_KBD) &&
+(ARCH_IS_X86(dom->os.arch) || dom->os.arch == VIR_ARCH_NONE)) {
+bus = VIR_DOMAIN_INPUT_BUS_PS2;
+} else if (ARCH_IS_S390(dom->os.arch) ||
+   def->type

[PATCH 17/17] virDomainTimerDefParseXML: Switch to virXMLPropEnumDefault()

2022-05-23 Thread Michal Privoznik

The virDomainTimerDefParseXML() function uses old style of
parsing XML (virXMLPropString + str2enum conversion). Use
virXMLPropEnumDefault() which encapsulates those steps.

Signed-off-by: Michal Privoznik 
---
 src/conf/domain_conf.c   | 105 +--
 src/conf/domain_conf.h   |  14 +++---
 src/libxl/libxl_conf.c   |   6 ++-
 src/libxl/xen_common.c   |   6 ++-
 src/lxc/lxc_cgroup.c |   2 +-
 src/lxc/lxc_controller.c |   2 +-
 src/qemu/qemu_command.c  |  11 +++-
 src/qemu/qemu_validate.c |   8 ++-
 8 files changed, 69 insertions(+), 85 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 52a34cd131..27fe6c9fbf 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -11962,98 +11962,61 @@ virDomainTimerDefParseXML(xmlNodePtr node,
 virDomainTimerDef *def;
 VIR_XPATH_NODE_AUTORESTORE(ctxt)
 xmlNodePtr catchup;
-int ret;
-g_autofree char *name = NULL;
-g_autofree char *tickpolicy = NULL;
-g_autofree char *track = NULL;
-g_autofree char *mode = NULL;
 
 def = g_new0(virDomainTimerDef, 1);
 
 ctxt->node = node;
 
-name = virXMLPropString(node, "name");
-if (name == NULL) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   "%s", _("missing timer name"));
+if (virXMLPropEnum(node, "name",
+   virDomainTimerNameTypeFromString,
+   VIR_XML_PROP_REQUIRED,
+   >name) < 0)
 goto error;
-}
-if ((def->name = virDomainTimerNameTypeFromString(name)) < 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("unknown timer name '%s'"), name);
-goto error;
-}
 
 if (virXMLPropTristateBool(node, "present",
VIR_XML_PROP_NONE,
>present) < 0)
 goto error;
 
-tickpolicy = virXMLPropString(node, "tickpolicy");
-if (tickpolicy != NULL) {
-if ((def->tickpolicy = 
virDomainTimerTickpolicyTypeFromString(tickpolicy)) <= 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("unknown timer tickpolicy '%s'"), tickpolicy);
-goto error;
-}
-}
+if (virXMLPropEnum(node, "tickpolicy",
+   virDomainTimerTickpolicyTypeFromString,
+   VIR_XML_PROP_NONZERO,
+   >tickpolicy) < 0)
+goto error;
 
-track = virXMLPropString(node, "track");
-if (track != NULL) {
-if ((def->track = virDomainTimerTrackTypeFromString(track)) <= 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("unknown timer track '%s'"), track);
-goto error;
-}
-}
+if (virXMLPropEnum(node, "track",
+   virDomainTimerTrackTypeFromString,
+   VIR_XML_PROP_NONZERO,
+   >track) < 0)
+goto error;
 
-ret = virXPathULongLong("string(./@frequency)", ctxt, >frequency);
-if (ret == -1) {
-def->frequency = 0;
-} else if (ret < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   "%s", _("invalid timer frequency"));
+if (virXMLPropULongLong(node, "frequency", 10,
+VIR_XML_PROP_NONE,
+>frequency) < 0)
 goto error;
-}
 
-mode = virXMLPropString(node, "mode");
-if (mode != NULL) {
-if ((def->mode = virDomainTimerModeTypeFromString(mode)) <= 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("unknown timer mode '%s'"), mode);
-goto error;
-}
-}
+if (virXMLPropEnum(node, "mode",
+   virDomainTimerModeTypeFromString,
+   VIR_XML_PROP_NONZERO,
+   >mode) < 0)
+goto error;
 
 catchup = virXPathNode("./catchup", ctxt);
 if (catchup != NULL) {
-ret = virXPathULong("string(./catchup/@threshold)", ctxt,
->catchup.threshold);
-if (ret == -1) {
-def->catchup.threshold = 0;
-} else if (ret < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   "%s", _("invalid catchup threshold"));
+if (virXMLPropUInt(catchup, "threshold", 10,
+   VIR_XML_PROP_NONE,
+   >catchup.threshold) < 0)
 goto error;
-}
 
-ret = virXPathULong("string(./catchup/@slew)", ctxt, 
>catchup.slew);
-if (ret == -1) {
-def->catchup.slew = 0;
-} else if (ret < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   "%s", _("invalid catchup slew"));
+if (virXMLPropUInt(catchup, "slew", 10,
+   VIR_XML_PROP_NONE,
+   >catchup.slew) < 0)
 goto error;
-

[PATCH 14/17] virDomainPanicDefParseXML: Switch to virXMLPropEnumDefault()

2022-05-23 Thread Michal Privoznik

The virDomainPanicDefParseXML() function uses old style of
parsing XML (virXMLPropString + str2enum conversion). Use
virXMLPropEnumDefault() which encapsulates those steps.

Signed-off-by: Michal Privoznik 
---
 src/conf/domain_conf.c   | 11 ---
 src/conf/domain_conf.h   |  2 +-
 src/qemu/qemu_command.c  |  2 +-
 src/qemu/qemu_validate.c |  2 +-
 4 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 4044515bdc..85aae73873 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -11788,7 +11788,6 @@ virDomainPanicDefParseXML(virDomainXMLOption *xmlopt,
   unsigned int flags)
 {
 virDomainPanicDef *panic;
-g_autofree char *model = NULL;
 
 panic = g_new0(virDomainPanicDef, 1);
 
@@ -11796,13 +11795,11 @@ virDomainPanicDefParseXML(virDomainXMLOption *xmlopt,
 >info, flags) < 0)
 goto error;
 
-model = virXMLPropString(node, "model");
-if (model != NULL &&
-(panic->model = virDomainPanicModelTypeFromString(model)) < 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("unknown panic model '%s'"), model);
+if (virXMLPropEnum(node, "model",
+   virDomainPanicModelTypeFromString,
+   VIR_XML_PROP_NONE,
+   >model) < 0)
 goto error;
-}
 
 return panic;
 
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 13ffdbfe88..3abc1dba36 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2619,7 +2619,7 @@ typedef enum {
 } virDomainPanicModel;
 
 struct _virDomainPanicDef {
-int model; /* virDomainPanicModel */
+virDomainPanicModel model;
 virDomainDeviceInfo info;
 };
 
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6f716c63a0..d3d69fed4f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9995,7 +9995,7 @@ qemuBuildPanicCommandLine(virCommand *cmd,
 size_t i;
 
 for (i = 0; i < def->npanics; i++) {
-switch ((virDomainPanicModel) def->panics[i]->model) {
+switch (def->panics[i]->model) {
 case VIR_DOMAIN_PANIC_MODEL_ISA: {
 g_autoptr(virJSONValue) props = NULL;
 
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 8842e2f837..3a82a2adfa 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -927,7 +927,7 @@ qemuValidateDomainDefPanic(const virDomainDef *def,
 size_t i;
 
 for (i = 0; i < def->npanics; i++) {
-switch ((virDomainPanicModel) def->panics[i]->model) {
+switch (def->panics[i]->model) {
 case VIR_DOMAIN_PANIC_MODEL_S390:
 /* For s390 guests, the hardware provides the same
  * functionality as the pvpanic device. The address
-- 
2.35.1

[PATCH 05/17] virDomainStorageNetworkParseHost: Switch to virXMLPropEnumDefault()

2022-05-23 Thread Michal Privoznik

The virDomainStorageNetworkParseHost() function uses old style of
parsing XML (virXMLPropString + str2enum conversion). Use
virXMLPropEnumDefault() which encapsulates those steps.

Signed-off-by: Michal Privoznik 
---
 src/conf/domain_conf.c| 19 +++
 src/conf/storage_source_conf.h|  2 +-
 src/qemu/qemu_backup.c|  2 +-
 src/qemu/qemu_block.c |  2 +-
 src/qemu/qemu_command.c   |  2 ++
 src/qemu/qemu_monitor_json.c  |  2 +-
 .../storage_file_backend_gluster.c|  2 +-
 .../storage_source_backingstore.c | 16 ++--
 8 files changed, 24 insertions(+), 23 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a9c424e71d..17ac74abcd 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7006,22 +7006,17 @@ virDomainStorageNetworkParseHost(xmlNodePtr hostnode,
  virStorageNetHostDef *host)
 {
 int ret = -1;
-g_autofree char *transport = NULL;
 g_autofree char *port = NULL;
 
 memset(host, 0, sizeof(*host));
-host->transport = VIR_STORAGE_NET_HOST_TRANS_TCP;
 
 /* transport can be tcp (default), unix or rdma.  */
-if ((transport = virXMLPropString(hostnode, "transport"))) {
-host->transport = virStorageNetHostTransportTypeFromString(transport);
-if (host->transport < 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("unknown protocol transport type '%s'"),
-   transport);
-goto cleanup;
-}
-}
+if (virXMLPropEnumDefault(hostnode, "transport",
+  virStorageNetHostTransportTypeFromString,
+  VIR_XML_PROP_NONE,
+  >transport,
+  VIR_STORAGE_NET_HOST_TRANS_TCP) < 0)
+goto cleanup;
 
 host->socket = virXMLPropString(hostnode, "socket");
 
@@ -7037,7 +7032,7 @@ virDomainStorageNetworkParseHost(xmlNodePtr hostnode,
 virReportError(VIR_ERR_XML_ERROR,
_("transport '%s' does not support "
  "socket attribute"),
-   transport);
+   
virStorageNetHostTransportTypeToString(host->transport));
 goto cleanup;
 }
 
diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h
index f2440cec6a..3afad96bdb 100644
--- a/src/conf/storage_source_conf.h
+++ b/src/conf/storage_source_conf.h
@@ -150,7 +150,7 @@ typedef struct _virStorageNetHostDef virStorageNetHostDef;
 struct _virStorageNetHostDef {
 char *name;
 unsigned int port;
-int transport; /* virStorageNetHostTransport */
+virStorageNetHostTransport transport;
 char *socket;  /* path to unix socket */
 };
 
diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c
index 427c090dd8..ef87f99177 100644
--- a/src/qemu/qemu_backup.c
+++ b/src/qemu/qemu_backup.c
@@ -73,7 +73,7 @@ qemuBackupPrepare(virDomainBackupDef *def)
 def->server->name = g_strdup("localhost");
 }
 
-switch ((virStorageNetHostTransport) def->server->transport) {
+switch (def->server->transport) {
 case VIR_STORAGE_NET_HOST_TRANS_TCP:
 /* TODO: Update qemu.conf to provide a port range,
  * probably starting at 10809, for obtaining automatic
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 877c66d62b..ba5e5ef625 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -452,7 +452,7 @@ 
qemuBlockStorageSourceBuildJSONSocketAddress(virStorageNetHostDef *host,
 const char *field;
 g_autofree char *port = NULL;
 
-switch ((virStorageNetHostTransport) host->transport) {
+switch (host->transport) {
 case VIR_STORAGE_NET_HOST_TRANS_TCP:
 if (legacy)
 transport = "tcp";
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 7eb7def747..dac8aabad4 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1619,6 +1619,8 @@ qemuBuildNetworkDriveStr(virStorageSource *src,
 virBufferAsprintf(, "unix:%s", src->hosts->socket);
 break;
 
+case VIR_STORAGE_NET_HOST_TRANS_RDMA:
+case VIR_STORAGE_NET_HOST_TRANS_LAST:
 default:
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("nbd does not support transport '%s'"),
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index dc05dfd047..afe45c415b 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -6499,7 +6499,7 @@ qemuMonitorJSONNBDServerStart(qemuMonitor *mon,
 g_autoptr(virJSONValue) addr = NULL;
 g_autofree char *port_str = NULL;
 
-switch ((virStorageNetHostTransport)server->transport) {
+

[PATCH 13/17] virDomainTPMDefParseXML: Switch to virXMLPropEnumDefault()

2022-05-23 Thread Michal Privoznik

The virDomainTPMDefParseXML() function uses old style of parsing
XML (virXMLPropString + str2enum conversion). Use
virXMLPropEnumDefault() which encapsulates those steps.

Signed-off-by: Michal Privoznik 
---
 src/conf/domain_conf.c   | 43 
 src/conf/domain_conf.h   |  6 +++---
 src/qemu/qemu_command.c  |  2 +-
 src/qemu/qemu_domain.c   |  2 +-
 src/qemu/qemu_validate.c |  1 +
 5 files changed, 19 insertions(+), 35 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b5ce80eb76..4044515bdc 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -11677,9 +11677,6 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt,
 int nnodes;
 size_t i;
 g_autofree char *path = NULL;
-g_autofree char *model = NULL;
-g_autofree char *backend = NULL;
-g_autofree char *version = NULL;
 g_autofree char *secretuuid = NULL;
 g_autofree char *persistent_state = NULL;
 g_autofree xmlNodePtr *backends = NULL;
@@ -11688,13 +11685,11 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt,
 
 def = g_new0(virDomainTPMDef, 1);
 
-model = virXMLPropString(node, "model");
-if (model != NULL &&
-(def->model = virDomainTPMModelTypeFromString(model)) < 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("Unknown TPM frontend model '%s'"), model);
+if (virXMLPropEnum(node, "model",
+   virDomainTPMModelTypeFromString,
+   VIR_XML_PROP_NONE,
+   >model) < 0)
 goto error;
-}
 
 ctxt->node = node;
 
@@ -11713,30 +11708,18 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt,
 goto error;
 }
 
-if (!(backend = virXMLPropString(backends[0], "type"))) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
-   _("missing TPM device backend type"));
+if (virXMLPropEnum(backends[0], "type",
+   virDomainTPMBackendTypeFromString,
+   VIR_XML_PROP_REQUIRED,
+   >type) < 0)
 goto error;
-}
 
-if ((def->type = virDomainTPMBackendTypeFromString(backend)) < 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("Unknown TPM backend type '%s'"),
-   backend);
+if (virXMLPropEnumDefault(backends[0], "version",
+  virDomainTPMVersionTypeFromString,
+  VIR_XML_PROP_NONE,
+  >version,
+  VIR_DOMAIN_TPM_VERSION_DEFAULT) < 0)
 goto error;
-}
-
-version = virXMLPropString(backends[0], "version");
-if (!version) {
-def->version = VIR_DOMAIN_TPM_VERSION_DEFAULT;
-} else {
-if ((def->version = virDomainTPMVersionTypeFromString(version)) < 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("Unsupported TPM version '%s'"),
-   version);
-goto error;
-}
-}
 
 switch (def->type) {
 case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index a81fb09678..13ffdbfe88 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1423,10 +1423,10 @@ typedef enum {
 #define VIR_DOMAIN_TPM_DEFAULT_DEVICE "/dev/tpm0"
 
 struct _virDomainTPMDef {
-int type; /* virDomainTPMBackendType */
+virDomainTPMBackendType type;
 virDomainDeviceInfo info;
-int model; /* virDomainTPMModel */
-int version; /* virDomainTPMVersion */
+virDomainTPMModel model;
+virDomainTPMVersion version;
 union {
 struct {
 virDomainChrSourceDef *source;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 365b7d8292..6f716c63a0 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9805,7 +9805,7 @@ qemuBuildTPMCommandLine(virCommand *cmd,
 g_autoptr(qemuFDPass) passtpm = NULL;
 g_autoptr(qemuFDPass) passcancel = NULL;
 
-switch ((virDomainTPMBackendType) tpm->type) {
+switch (tpm->type) {
 case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: {
 VIR_AUTOCLOSE fdtpm = -1;
 VIR_AUTOCLOSE fdcancel = -1;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 3432c83153..d3da566bae 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -11737,7 +11737,7 @@ 
qemuDomainDeviceBackendChardevForeachOne(virDomainDeviceDef *dev,
 return cb(dev, dev->data.rng->source.chardev, opaque);
 
 case VIR_DOMAIN_DEVICE_TPM:
-switch ((virDomainTPMBackendType) dev->data.tpm->type) {
+switch (dev->data.tpm->type) {
 case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
 return cb(dev, dev->data.tpm->data.passthrough.source, opaque);
 
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 4d6355741e..8842e2f837 100644
--- a/src/qemu/qemu_validate.c
+++

[PATCH 06/17] virDomainHostdevSubsysSCSIDefParseXML: Switch to virXMLPropEnumDefault()

2022-05-23 Thread Michal Privoznik

The virDomainHostdevSubsysSCSIDefParseXML() function uses old
style of parsing XML (virXMLPropString + str2enum conversion).
Use virXMLPropEnumDefault() which encapsulates those steps.

Signed-off-by: Michal Privoznik 
---
 src/conf/domain_conf.c   | 19 ++-
 src/conf/domain_conf.h   |  2 +-
 src/qemu/qemu_command.c  |  4 ++--
 src/qemu/qemu_domain.c   |  4 ++--
 src/qemu/qemu_driver.c   |  2 +-
 src/qemu/qemu_process.c  |  2 +-
 tests/qemuxml2argvtest.c |  2 +-
 7 files changed, 14 insertions(+), 21 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 17ac74abcd..ded2c4aacf 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7233,20 +7233,13 @@ virDomainHostdevSubsysSCSIDefParseXML(xmlNodePtr 
sourcenode,
   unsigned int flags,
   virDomainXMLOption *xmlopt)
 {
-g_autofree char *protocol = NULL;
+if (virXMLPropEnum(sourcenode, "protocol",
+   virDomainHostdevSubsysSCSIProtocolTypeFromString,
+   VIR_XML_PROP_NONE,
+   >protocol) < 0)
+return -1;
 
-if ((protocol = virXMLPropString(sourcenode, "protocol"))) {
-scsisrc->protocol =
-virDomainHostdevSubsysSCSIProtocolTypeFromString(protocol);
-if (scsisrc->protocol < 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("Unknown SCSI subsystem protocol '%s'"),
-   protocol);
-return -1;
-}
-}
-
-switch ((virDomainHostdevSCSIProtocolType) scsisrc->protocol) {
+switch (scsisrc->protocol) {
 case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_NONE:
 return virDomainHostdevSubsysSCSIHostDefParseXML(sourcenode, ctxt, 
scsisrc,
  flags, xmlopt);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index f325bfb51e..667845ac10 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -270,7 +270,7 @@ typedef enum {
 } virDomainDeviceSGIO;
 
 struct _virDomainHostdevSubsysSCSI {
-int protocol; /* enum virDomainHostdevSCSIProtocolType */
+virDomainHostdevSCSIProtocolType protocol;
 virDomainDeviceSGIO sgio;
 virTristateBool rawio;
 union {
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index dac8aabad4..5ea88bf239 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5284,7 +5284,7 @@ qemuBuildHostdevSCSIDetachPrepare(virDomainHostdevDef 
*hostdev,
 virStorageSource *src;
 qemuDomainStorageSourcePrivate *srcpriv;
 
-switch ((virDomainHostdevSCSIProtocolType) scsisrc->protocol) {
+switch (scsisrc->protocol) {
 case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_NONE:
 src = scsisrc->u.host.src;
 break;
@@ -5325,7 +5325,7 @@ qemuBuildHostdevSCSIAttachPrepare(virDomainHostdevDef 
*hostdev,
 virStorageSource *src = NULL;
 
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV_HOSTDEV_SCSI)) {
-switch ((virDomainHostdevSCSIProtocolType) scsisrc->protocol) {
+switch (scsisrc->protocol) {
 case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_NONE:
 src = scsisrc->u.host.src;
 break;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 948ab76304..30ef5b7550 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5731,7 +5731,7 @@ 
qemuDomainDeviceHostdevDefPostParseRestoreBackendAlias(virDomainHostdevDef *host
 !virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV_HOSTDEV_SCSI))
 return 0;
 
-switch ((virDomainHostdevSCSIProtocolType) scsisrc->protocol) {
+switch (scsisrc->protocol) {
 case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_NONE:
 if (!scsisrc->u.host.src)
 scsisrc->u.host.src = virStorageSourceNew();
@@ -10961,7 +10961,7 @@ qemuDomainPrepareHostdev(virDomainHostdevDef *hostdev,
 virDomainHostdevSubsysSCSI *scsisrc = >source.subsys.u.scsi;
 virStorageSource *src = NULL;
 
-switch ((virDomainHostdevSCSIProtocolType) scsisrc->protocol) {
+switch (scsisrc->protocol) {
 case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_NONE:
 virObjectUnref(scsisrc->u.host.src);
 scsisrc->u.host.src = virStorageSourceNew();
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1d50aa5271..f0be25a12d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6284,7 +6284,7 @@ 
qemuConnectDomainXMLToNativePrepareHostHostdev(virDomainHostdevDef *hostdev)
 if (virHostdevIsSCSIDevice(hostdev)) {
 virDomainHostdevSubsysSCSI *scsisrc = >source.subsys.u.scsi;
 
-switch ((virDomainHostdevSCSIProtocolType) scsisrc->protocol) {
+switch (scsisrc->protocol) {
 case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_NONE: {
 virDomainHostdevSubsysSCSIHost *scsihostsrc = >u.host;

[PATCH 09/17] virDomainDiskDefMirrorParse: Switch to virXMLPropEnumDefault()

2022-05-23 Thread Michal Privoznik

The virDomainDiskDefMirrorParse() function uses old style of
parsing XML (virXMLPropString + str2enum conversion). Use
virXMLPropEnumDefault() which encapsulates those steps.

Signed-off-by: Michal Privoznik 
---
 src/conf/domain_conf.c  | 26 ++
 src/conf/domain_conf.h  |  4 ++--
 src/qemu/qemu_process.c |  6 --
 3 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index bf9db38340..4e1835fd34 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8804,21 +8804,16 @@ virDomainDiskDefMirrorParse(virDomainDiskDef *def,
 VIR_XPATH_NODE_AUTORESTORE(ctxt)
 g_autofree char *mirrorFormat = NULL;
 g_autofree char *mirrorType = NULL;
-g_autofree char *ready = NULL;
-g_autofree char *blockJob = NULL;
 g_autofree char *index = NULL;
 
 ctxt->node = cur;
 
-if ((blockJob = virXMLPropString(cur, "job"))) {
-if ((def->mirrorJob = virDomainBlockJobTypeFromString(blockJob)) <= 0) 
{
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("unknown mirror job type '%s'"), blockJob);
-return -1;
-}
-} else {
-def->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_COPY;
-}
+if (virXMLPropEnumDefault(cur, "job",
+  virDomainBlockJobTypeFromString,
+  VIR_XML_PROP_NONZERO,
+  >mirrorJob,
+  VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) < 0)
+return -1;
 
 if ((mirrorType = virXMLPropString(cur, "type"))) {
 mirrorFormat = virXPathString("string(./format/@type)", ctxt);
@@ -8860,12 +8855,11 @@ virDomainDiskDefMirrorParse(virDomainDiskDef *def,
 }
 }
 
-if ((ready = virXMLPropString(cur, "ready")) &&
-(def->mirrorState = virDomainDiskMirrorStateTypeFromString(ready)) < 
0) {
-virReportError(VIR_ERR_XML_ERROR,
-   _("unknown mirror ready state %s"), ready);
+if (virXMLPropEnum(cur, "ready",
+   virDomainDiskMirrorStateTypeFromString,
+   VIR_XML_PROP_NONE,
+   >mirrorState) < 0)
 return -1;
-}
 
 if (virParseScaledValue("./format/metadata_cache/max_size", NULL,
 ctxt,
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 2f00cc7f79..30aa0ed8d3 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -564,8 +564,8 @@ struct _virDomainDiskDef {
 unsigned int rotation_rate;
 
 virStorageSource *mirror;
-int mirrorState; /* enum virDomainDiskMirrorState */
-int mirrorJob; /* virDomainBlockJobType */
+virDomainDiskMirrorState mirrorState;
+virDomainBlockJobType mirrorJob;
 
 struct {
 unsigned int cylinders;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 929986745e..85bf452a59 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8660,8 +8660,10 @@ qemuProcessRefreshLegacyBlockjob(void *payload,
 }
 
 if (jobtype == QEMU_BLOCKJOB_TYPE_COMMIT &&
-disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT)
-jobtype = disk->mirrorJob;
+disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT) {
+/* This typecast is safe because of how the enum is declared. */
+jobtype = (qemuBlockJobType) disk->mirrorJob;
+}
 
 if (!(job = qemuBlockJobDiskNew(vm, disk, jobtype, jobname)))
 return -1;
-- 
2.35.1

[PATCH 08/17] virDomainDiskSourceNVMeParse: Switch to virXMLPropEnumDefault()

2022-05-23 Thread Michal Privoznik

The virDomainDiskSourceNVMeParse() function uses old style of
parsing XML (virXMLPropString + str2enum conversion). Use
virXMLPropEnumDefault() which encapsulates those steps.

Signed-off-by: Michal Privoznik 
---
 src/conf/domain_conf.c | 15 +++
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 561af84eed..bf9db38340 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8405,7 +8405,6 @@ virDomainDiskSourceNVMeParse(xmlNodePtr node,
 {
 g_autoptr(virStorageSourceNVMeDef) nvme = NULL;
 g_autofree char *type = NULL;
-g_autofree char *namespc = NULL;
 xmlNodePtr address;
 
 nvme = g_new0(virStorageSourceNVMeDef, 1);
@@ -8423,18 +8422,10 @@ virDomainDiskSourceNVMeParse(xmlNodePtr node,
 return -1;
 }
 
-if (!(namespc = virXMLPropString(node, "namespace"))) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
-   _("missing 'namespace' attribute to disk source"));
+if (virXMLPropULongLong(node, "namespace", 10,
+VIR_XML_PROP_REQUIRED,
+>namespc) < 0)
 return -1;
-}
-
-if (virStrToLong_ull(namespc, NULL, 10, >namespc) < 0) {
-virReportError(VIR_ERR_XML_ERROR,
-   _("malformed namespace '%s'"),
-   namespc);
-return -1;
-}
 
 if (virXMLPropTristateBool(node, "managed", VIR_XML_PROP_NONE,
>managed) < 0)
-- 
2.35.1

[PATCH 07/17] virDomainHostdevSubsysSCSIVHostDefParseXML: Switch to virXMLPropEnumDefault()

2022-05-23 Thread Michal Privoznik

The virDomainHostdevSubsysSCSIVHostDefParseXML() function uses
old style of parsing XML (virXMLPropString + str2enum
conversion). Use virXMLPropEnumDefault() which encapsulates those
steps.

Signed-off-by: Michal Privoznik 
---
 src/conf/domain_conf.c | 19 +--
 src/conf/domain_conf.h |  4 ++--
 2 files changed, 7 insertions(+), 16 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index ded2c4aacf..561af84eed 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7262,24 +7262,15 @@ virDomainHostdevSubsysSCSIVHostDefParseXML(xmlNodePtr 
sourcenode,
virDomainHostdevDef *def)
 {
 virDomainHostdevSubsysSCSIVHost *hostsrc = >source.subsys.u.scsi_host;
-g_autofree char *protocol = NULL;
 g_autofree char *wwpn = NULL;
 
-if (!(protocol = virXMLPropString(sourcenode, "protocol"))) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
-   _("Missing scsi_host subsystem protocol"));
+if (virXMLPropEnum(sourcenode, "protocol",
+   virDomainHostdevSubsysSCSIHostProtocolTypeFromString,
+   VIR_XML_PROP_NONZERO | VIR_XML_PROP_REQUIRED,
+   >protocol) < 0)
 return -1;
-}
 
-if ((hostsrc->protocol =
- virDomainHostdevSubsysSCSIHostProtocolTypeFromString(protocol)) <= 0) 
{
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("Unknown scsi_host subsystem protocol '%s'"),
-   protocol);
-return -1;
-}
-
-switch ((virDomainHostdevSubsysSCSIHostProtocolType) hostsrc->protocol) {
+switch (hostsrc->protocol) {
 case VIR_DOMAIN_HOSTDEV_SUBSYS_SCSI_HOST_PROTOCOL_TYPE_VHOST:
 if (!(wwpn = virXMLPropString(sourcenode, "wwpn"))) {
 virReportError(VIR_ERR_XML_ERROR, "%s",
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 667845ac10..2f00cc7f79 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -287,7 +287,7 @@ struct _virDomainHostdevSubsysMediatedDev {
 };
 
 typedef enum {
-VIR_DOMAIN_HOSTDEV_SUBSYS_SCSI_HOST_PROTOCOL_TYPE_NONE,
+VIR_DOMAIN_HOSTDEV_SUBSYS_SCSI_HOST_PROTOCOL_TYPE_NONE = 0,
 VIR_DOMAIN_HOSTDEV_SUBSYS_SCSI_HOST_PROTOCOL_TYPE_VHOST,
 
 VIR_DOMAIN_HOSTDEV_SUBSYS_SCSI_HOST_PROTOCOL_TYPE_LAST,
@@ -307,7 +307,7 @@ typedef enum {
 VIR_ENUM_DECL(virDomainHostdevSubsysSCSIVHostModel);
 
 struct _virDomainHostdevSubsysSCSIVHost {
-int protocol; /* enum virDomainHostdevSubsysSCSIHostProtocolType */
+virDomainHostdevSubsysSCSIHostProtocolType protocol;
 char *wwpn;
 virDomainHostdevSubsysSCSIVHostModelType model;
 };
-- 
2.35.1

[PATCH 10/17] virDomainDiskSourcePoolDefParse: Switch to virXMLPropEnumDefault()

2022-05-23 Thread Michal Privoznik

The virDomainDiskSourcePoolDefParse() function uses old style of
parsing XML (virXMLPropString + str2enum conversion). Use
virXMLPropEnumDefault() which encapsulates those steps.

Signed-off-by: Michal Privoznik 
---
 src/conf/domain_conf.c | 12 
 src/conf/storage_source_conf.h |  2 +-
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 4e1835fd34..c42ac9c0c9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8193,7 +8193,6 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node,
 {
 virStorageSourcePoolDef *source;
 int ret = -1;
-g_autofree char *mode = NULL;
 
 *srcpool = NULL;
 
@@ -8201,7 +8200,6 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node,
 
 source->pool = virXMLPropString(node, "pool");
 source->volume = virXMLPropString(node, "volume");
-mode = virXMLPropString(node, "mode");
 
 /* CD-ROM and Floppy allows no source */
 if (!source->pool && !source->volume) {
@@ -8216,13 +8214,11 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node,
 goto cleanup;
 }
 
-if (mode &&
-(source->mode = virStorageSourcePoolModeTypeFromString(mode)) <= 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("unknown source mode '%s' for volume type disk"),
-   mode);
+if (virXMLPropEnum(node, "mode",
+   virStorageSourcePoolModeTypeFromString,
+   VIR_XML_PROP_NONZERO,
+   >mode) < 0)
 goto cleanup;
-}
 
 *srcpool = g_steal_pointer();
 ret = 0;
diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h
index 3afad96bdb..b99a952702 100644
--- a/src/conf/storage_source_conf.h
+++ b/src/conf/storage_source_conf.h
@@ -200,7 +200,7 @@ struct _virStorageSourcePoolDef {
 int voltype; /* virStorageVolType, internal only */
 int pooltype; /* virStoragePoolType from storage_conf.h, internal only */
 virStorageType actualtype; /* internal only */
-int mode; /* virStorageSourcePoolMode, currently makes sense only for 
iscsi pool */
+virStorageSourcePoolMode mode; /* currently makes sense only for iscsi 
pool */
 };
 
 
-- 
2.35.1

[PATCH 01/17] Drop needless typecast to virStorageType enum

2022-05-23 Thread Michal Privoznik

There are three places (two in domain_conf.c and one in
qemu_migration.c) where a virStorageSource->type is typecasted to
virStorageType (for the purpose of catching missing enum member
in a switch() statement at compile time). This is needless,
because as of v8.2.0-rc1~120 the struct member is of proper type.

Signed-off-by: Michal Privoznik 
---
 src/conf/domain_conf.c| 4 ++--
 src/qemu/qemu_migration.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 0b30ebc96b..6ff994dd12 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8659,7 +8659,7 @@ virDomainStorageSourceParse(xmlNodePtr node,
 
 ctxt->node = node;
 
-switch ((virStorageType)src->type) {
+switch (src->type) {
 case VIR_STORAGE_TYPE_FILE:
 src->path = virXMLPropString(node, "file");
 break;
@@ -23350,7 +23350,7 @@ virDomainDiskSourceFormat(virBuffer *buf,
 g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
 g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
 
-switch ((virStorageType)src->type) {
+switch (src->type) {
 case VIR_STORAGE_TYPE_FILE:
 virBufferEscapeString(, " file='%s'", src->path);
 break;
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 38596fa4de..176c25cd11 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -182,7 +182,7 @@ qemuMigrationDstPrecreateDisk(virConnectPtr *conn,
 
 VIR_DEBUG("Precreate disk type=%s", 
virStorageTypeToString(disk->src->type));
 
-switch ((virStorageType)disk->src->type) {
+switch (disk->src->type) {
 case VIR_STORAGE_TYPE_FILE:
 if (!virDomainDiskGetSource(disk)) {
 VIR_DEBUG("Dropping sourceless disk '%s'",
-- 
2.35.1

[PATCH 02/17] virStorageSourceGetActualType: Change type of retval

2022-05-23 Thread Michal Privoznik

The virStorageSourceGetActualType() function returns either
virStorageSource->type (which is of type virStorageType), or
virStorageSourcePoolDef->type, which really stores a value of the
same enum. Thus, the latter struct can be changed so that the
virStorageSourceGetActualType() function can return correct type
instead of generic int.

Signed-off-by: Michal Privoznik 
---
 src/conf/storage_source_conf.c|  2 +-
 src/conf/storage_source_conf.h|  4 ++--
 src/libxl/libxl_conf.c|  2 +-
 src/libxl/libxl_domain.c  |  2 +-
 src/libxl/xen_xl.c|  4 ++--
 src/locking/domain_lock.c |  2 +-
 src/qemu/qemu_block.c | 12 ++--
 src/qemu/qemu_command.c   |  8 
 src/qemu/qemu_domain.c|  4 ++--
 src/qemu/qemu_driver.c|  2 +-
 src/qemu/qemu_migration.c |  4 ++--
 src/qemu/qemu_snapshot.c  | 16 
 src/storage_file/storage_source.c |  4 ++--
 13 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/src/conf/storage_source_conf.c b/src/conf/storage_source_conf.c
index 1a7284ec12..2b4cf5e241 100644
--- a/src/conf/storage_source_conf.c
+++ b/src/conf/storage_source_conf.c
@@ -1004,7 +1004,7 @@ virStorageSourcePoolDefFree(virStorageSourcePoolDef *def)
  * and virDomainDiskTranslateSourcePool was called on @def the actual type
  * of the storage volume is returned rather than VIR_STORAGE_TYPE_VOLUME.
  */
-int
+virStorageType
 virStorageSourceGetActualType(const virStorageSource *def)
 {
 if (def->type == VIR_STORAGE_TYPE_VOLUME &&
diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h
index 035ea7016d..f2440cec6a 100644
--- a/src/conf/storage_source_conf.h
+++ b/src/conf/storage_source_conf.h
@@ -199,7 +199,7 @@ struct _virStorageSourcePoolDef {
 char *volume; /* volume name */
 int voltype; /* virStorageVolType, internal only */
 int pooltype; /* virStoragePoolType from storage_conf.h, internal only */
-int actualtype; /* virStorageType, internal only */
+virStorageType actualtype; /* internal only */
 int mode; /* virStorageSourcePoolMode, currently makes sense only for 
iscsi pool */
 };
 
@@ -469,7 +469,7 @@ virStorageSourcePoolDefFree(virStorageSourcePoolDef *def);
 void
 virStorageSourceClear(virStorageSource *def);
 
-int
+virStorageType
 virStorageSourceGetActualType(const virStorageSource *def);
 
 bool
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 726ecdc945..401e47344e 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1067,7 +1067,7 @@ libxlMakeDisk(virDomainDiskDef *l_disk, libxl_device_disk 
*x_disk)
 {
 const char *driver = virDomainDiskGetDriver(l_disk);
 int format = virDomainDiskGetFormat(l_disk);
-int actual_type = virStorageSourceGetActualType(l_disk->src);
+virStorageType actual_type = virStorageSourceGetActualType(l_disk->src);
 
 if (actual_type == VIR_STORAGE_TYPE_NETWORK) {
 if (STRNEQ_NULLABLE(driver, "qemu")) {
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index af938192a9..49ac09d8a4 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -330,7 +330,7 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDef *dev,
 
 if (dev->type == VIR_DOMAIN_DEVICE_DISK) {
 virDomainDiskDef *disk = dev->data.disk;
-int actual_type = virStorageSourceGetActualType(disk->src);
+virStorageType actual_type = virStorageSourceGetActualType(disk->src);
 int format = virDomainDiskGetFormat(disk);
 
 /* for network-based disks, set 'qemu' as the default driver */
diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c
index 6b7f638783..eb3b0b3718 100644
--- a/src/libxl/xen_xl.c
+++ b/src/libxl/xen_xl.c
@@ -1489,14 +1489,14 @@ xenFormatXLDiskSrcNet(virStorageSource *src)
 static int
 xenFormatXLDiskSrc(virStorageSource *src, char **srcstr)
 {
-int actualType = virStorageSourceGetActualType(src);
+virStorageType actualType = virStorageSourceGetActualType(src);
 
 *srcstr = NULL;
 
 if (virStorageSourceIsEmpty(src))
 return 0;
 
-switch ((virStorageType)actualType) {
+switch (actualType) {
 case VIR_STORAGE_TYPE_BLOCK:
 case VIR_STORAGE_TYPE_FILE:
 case VIR_STORAGE_TYPE_DIR:
diff --git a/src/locking/domain_lock.c b/src/locking/domain_lock.c
index 9934e91aa9..8966980532 100644
--- a/src/locking/domain_lock.c
+++ b/src/locking/domain_lock.c
@@ -72,7 +72,7 @@ static int virDomainLockManagerAddImage(virLockManager *lock,
 virStorageSource *src)
 {
 unsigned int diskFlags = 0;
-int type = virStorageSourceGetActualType(src);
+virStorageType type = virStorageSourceGetActualType(src);
 
 if (!src->path)
 return 0;
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 60e03d418e..877c66d62b 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -1128,7 +1128,7 @@ virJSONValue *

[PATCH 03/17] virDomainBackupDefParse: Switch to virXMLPropEnumDefault()

2022-05-23 Thread Michal Privoznik

The virDomainBackupDefParse() function uses old style of parsing
XML (virXMLPropString + str2enum conversion). Use
virXMLPropEnumDefault() which encapsulates those steps.

Signed-off-by: Michal Privoznik 
---
 src/conf/backup_conf.c | 16 ++--
 src/conf/backup_conf.h |  2 +-
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c
index 2a7fa95e0c..11d533a905 100644
--- a/src/conf/backup_conf.c
+++ b/src/conf/backup_conf.c
@@ -201,22 +201,18 @@ virDomainBackupDefParse(xmlXPathContextPtr ctxt,
 g_autoptr(virDomainBackupDef) def = NULL;
 g_autofree xmlNodePtr *nodes = NULL;
 xmlNodePtr node = NULL;
-g_autofree char *mode = NULL;
 bool push;
 size_t i;
 int n;
 
 def = g_new0(virDomainBackupDef, 1);
 
-def->type = VIR_DOMAIN_BACKUP_TYPE_PUSH;
-
-if ((mode = virXMLPropString(ctxt->node, "mode"))) {
-if ((def->type = virDomainBackupTypeFromString(mode)) <= 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("unknown backup mode '%s'"), mode);
-return NULL;
-}
-}
+if (virXMLPropEnumDefault(ctxt->node, "mode",
+  virDomainBackupTypeFromString,
+  VIR_XML_PROP_NONZERO,
+  >type,
+  VIR_DOMAIN_BACKUP_TYPE_PUSH) < 0)
+return NULL;
 
 push = def->type == VIR_DOMAIN_BACKUP_TYPE_PUSH;
 
diff --git a/src/conf/backup_conf.h b/src/conf/backup_conf.h
index dc66b75892..413488e123 100644
--- a/src/conf/backup_conf.h
+++ b/src/conf/backup_conf.h
@@ -76,7 +76,7 @@ struct _virDomainBackupDiskDef {
 typedef struct _virDomainBackupDef virDomainBackupDef;
 struct _virDomainBackupDef {
 /* Public XML.  */
-int type; /* virDomainBackupType */
+virDomainBackupType type;
 char *incremental;
 virStorageNetHostDef *server; /* only when type == PULL */
 virTristateBool tls; /* use TLS for NBD */
-- 
2.35.1

[PATCH 04/17] virDomainDeviceAddressParseXML: Switch to virXMLPropEnumDefault()

2022-05-23 Thread Michal Privoznik

The virDomainDeviceAddressParseXML() function uses old style of parsing
XML (virXMLPropString + str2enum conversion). Use
virXMLPropEnumDefault() which encapsulates those steps.

Signed-off-by: Michal Privoznik 
---
 src/conf/device_conf.c | 12 -
 src/conf/device_conf.h |  4 +--
 src/conf/domain_conf.c | 22 +---
 src/conf/domain_validate.c |  2 +-
 src/qemu/qemu_command.c|  4 +--
 src/qemu/qemu_monitor.c| 54 +++---
 src/qemu/qemu_validate.c   |  2 +-
 7 files changed, 57 insertions(+), 43 deletions(-)

diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
index cb523d3a0d..f5270a54ea 100644
--- a/src/conf/device_conf.c
+++ b/src/conf/device_conf.c
@@ -100,7 +100,7 @@ virDomainDeviceInfoAddressIsEqual(const virDomainDeviceInfo 
*a,
 if (a->type != b->type)
 return false;
 
-switch ((virDomainDeviceAddressType) a->type) {
+switch (a->type) {
 case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE:
 case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST:
 /* address types below don't have any specific data */
@@ -457,6 +457,16 @@ virDomainDeviceAddressIsValid(virDomainDeviceInfo *info,
 
 case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB:
 return true;
+
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE:
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL:
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCID:
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO:
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA:
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM:
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_UNASSIGNED:
+case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST:
+return false;
 }
 
 return false;
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
index b6b710d313..d8f4ca4f17 100644
--- a/src/conf/device_conf.h
+++ b/src/conf/device_conf.h
@@ -32,7 +32,7 @@
 #include "virenum.h"
 
 typedef enum {
-VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE,
+VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE = 0,
 VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI,
 VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE,
 VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL,
@@ -128,7 +128,7 @@ struct _virDomainDeviceDimmAddress {
 typedef struct _virDomainDeviceInfo virDomainDeviceInfo;
 struct _virDomainDeviceInfo {
 char *alias;
-int type; /* virDomainDeviceAddressType */
+virDomainDeviceAddressType type;
 union {
 virPCIDeviceAddress pci;
 virDomainDeviceDriveAddress drive;
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6ff994dd12..a9c424e71d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6536,7 +6536,7 @@ virDomainDeviceInfoFormat(virBuffer *buf,
 virBufferAsprintf(, " type='%s'",
   virDomainDeviceAddressTypeToString(info->type));
 
-switch ((virDomainDeviceAddressType) info->type) {
+switch (info->type) {
 case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI:
 if (!virPCIDeviceAddressIsEmpty(>addr.pci)) {
 virBufferAsprintf(, " domain='0x%04x' bus='0x%02x' "
@@ -6709,21 +6709,13 @@ static int
 virDomainDeviceAddressParseXML(xmlNodePtr address,
virDomainDeviceInfo *info)
 {
-g_autofree char *type = virXMLPropString(address, "type");
-
-if (type) {
-if ((info->type = virDomainDeviceAddressTypeFromString(type)) <= 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("unknown address type '%s'"), type);
-return -1;
-}
-} else {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   "%s", _("No type specified for device address"));
+if (virXMLPropEnum(address, "type",
+   virDomainDeviceAddressTypeFromString,
+   VIR_XML_PROP_NONZERO | VIR_XML_PROP_REQUIRED,
+   >type) < 0)
 return -1;
-}
 
-switch ((virDomainDeviceAddressType) info->type) {
+switch (info->type) {
 case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI:
 if (virPCIDeviceAddressParseXML(address, >addr.pci) < 0)
 return -1;
@@ -20658,7 +20650,7 @@ 
virDomainDeviceInfoCheckABIStability(virDomainDeviceInfo *src,
 return false;
 }
 
-switch ((virDomainDeviceAddressType) src->type) {
+switch (src->type) {
 case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI:
 if (src->addr.pci.domain != dst->addr.pci.domain ||
 src->addr.pci.bus != dst->addr.pci.bus ||
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 627c366fe9..28234f910d 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -2459,7 +2459,7 @@ virDomainDeviceInfoValidate(const virDomainDeviceDef *dev)
 if (!(info = virDomainDeviceGetInfo(dev)))
 return 0;
 
-switch ((virDomainDeviceAddressType) info->type) {
+switch (info->type) {
 case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI:
 case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE:
 case

[PATCH REBASE 00/17] Use virXMLPropEnum() more (part I)

2022-05-23 Thread Michal Privoznik

This is rebased version of:

https://listman.redhat.com/archives/libvir-list/2022-April/229941.html

Michal Prívozník (17):
  Drop needless typecast to virStorageType enum
  virStorageSourceGetActualType: Change type of retval
  virDomainBackupDefParse: Switch to virXMLPropEnumDefault()
  virDomainDeviceAddressParseXML: Switch to virXMLPropEnumDefault()
  virDomainStorageNetworkParseHost: Switch to virXMLPropEnumDefault()
  virDomainHostdevSubsysSCSIDefParseXML: Switch to
virXMLPropEnumDefault()
  virDomainHostdevSubsysSCSIVHostDefParseXML: Switch to
virXMLPropEnumDefault()
  virDomainDiskSourceNVMeParse: Switch to virXMLPropEnumDefault()
  virDomainDiskDefMirrorParse: Switch to virXMLPropEnumDefault()
  virDomainDiskSourcePoolDefParse: Switch to virXMLPropEnumDefault()
  virDomainDiskDefParseSourceXML: Switch to virXMLPropEnumDefault()
  virDomainChrDefParseXML: Switch to virXMLPropEnumDefault()
  virDomainTPMDefParseXML: Switch to virXMLPropEnumDefault()
  virDomainPanicDefParseXML: Switch to virXMLPropEnumDefault()
  virDomainInputDefParseXML: Switch to virXMLPropEnumDefault()
  virDomainHubDefParseXML: Switch to virXMLPropEnumDefault()
  virDomainTimerDefParseXML: Switch to virXMLPropEnumDefault()

 src/ch/ch_monitor.c   |   2 +-
 src/conf/backup_conf.c|  16 +-
 src/conf/backup_conf.h|   2 +-
 src/conf/device_conf.c|  12 +-
 src/conf/device_conf.h|   4 +-
 src/conf/domain_audit.c   |   2 +-
 src/conf/domain_conf.c| 494 ++
 src/conf/domain_conf.h|  54 +-
 src/conf/domain_validate.c|   6 +-
 src/conf/storage_source_conf.c|   2 +-
 src/conf/storage_source_conf.h|   8 +-
 src/conf/virchrdev.c  |  29 +
 src/libxl/libxl_conf.c|  32 +-
 src/libxl/libxl_domain.c  |   2 +-
 src/libxl/xen_common.c|  29 +-
 src/libxl/xen_xl.c|   7 +-
 src/libxl/xen_xm.c|   3 +
 src/locking/domain_lock.c |   2 +-
 src/lxc/lxc_cgroup.c  |   2 +-
 src/lxc/lxc_controller.c  |   2 +-
 src/qemu/qemu_backup.c|   2 +-
 src/qemu/qemu_block.c |  14 +-
 src/qemu/qemu_cgroup.c|  12 +
 src/qemu/qemu_command.c   |  49 +-
 src/qemu/qemu_domain.c|  44 +-
 src/qemu/qemu_domain_address.c|   4 +-
 src/qemu/qemu_driver.c|   4 +-
 src/qemu/qemu_hotplug.c   |   4 +-
 src/qemu/qemu_migration.c |   6 +-
 src/qemu/qemu_monitor.c   |  54 +-
 src/qemu/qemu_monitor_json.c  |   4 +-
 src/qemu/qemu_process.c   |  10 +-
 src/qemu/qemu_snapshot.c  |  16 +-
 src/qemu/qemu_validate.c  |  19 +-
 src/security/security_apparmor.c  |   6 +-
 src/security/security_dac.c   |   8 +-
 src/security/security_selinux.c   |  28 +-
 .../storage_file_backend_gluster.c|   2 +-
 src/storage_file/storage_source.c |   4 +-
 .../storage_source_backingstore.c |  16 +-
 src/vmx/vmx.c |  26 +
 tests/qemuxml2argvtest.c  |   2 +-
 tests/testutilsqemu.c |   2 +-
 43 files changed, 557 insertions(+), 489 deletions(-)

-- 
2.35.1

Re: [PATCH 0/2] Fix missing 'manual' mode for default snapshot disk definition and clean up docs

2022-05-23 Thread Michal Prívozník

On 5/23/22 13:56, Peter Krempa wrote:
> Peter Krempa (2):
>   docs: domain: Remove extraneous quotes
>   schemas: Allow 'manual' snapshot mode in domain definition
> 
>  docs/formatdomain.rst | 20 ++--
>  src/conf/schemas/domaincommon.rng |  1 +
>  2 files changed, 11 insertions(+), 10 deletions(-)
> 

Reviewed-by: Michal Privoznik 

Michal

[PATCH 2/2] schemas: Allow 'manual' snapshot mode in domain definition

2022-05-23 Thread Peter Krempa

Commit a1465e661e7 added the 'manual' disk snapshot mode documentation
but didn't allow it in the schema as default snapshot mode for a disk.

Add the needed value into the schema.

Signed-off-by: Peter Krempa 
---
 src/conf/schemas/domaincommon.rng | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/conf/schemas/domaincommon.rng 
b/src/conf/schemas/domaincommon.rng
index 7bef4910e1..cc598212a8 100644
--- a/src/conf/schemas/domaincommon.rng
+++ b/src/conf/schemas/domaincommon.rng
@@ -1544,6 +1544,7 @@
 no
 internal
 external
+manual
   
 
   
-- 
2.35.3

[PATCH 1/2] docs: domain: Remove extraneous quotes

2022-05-23 Thread Peter Krempa

Certain documentation bits tried to put a reference of a value into
quotes, but that's not needed for both the pure view of the rST source
and the rendered output.

Signed-off-by: Peter Krempa 
---
 docs/formatdomain.rst | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 089a99b4b5..085a929bee 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -576,8 +576,8 @@ layout of sub-elements, with supported values of:

The ``sysinfo`` element can have multiple ``entry`` child elements. Each
element then has mandatory ``name`` attribute, which defines the name of the
-   blob and must begin with ``"opt/"`` and to avoid clashing with other names 
is
-   advised to be in form ``"opt/$RFQDN/$name"`` where ``$RFQDN`` is a reverse
+   blob and must begin with ``opt/`` and to avoid clashing with other names is
+   advised to be in form ``opt/$RFQDN/$name`` where ``$RFQDN`` is a reverse
fully qualified domain name you control. Then, the element can either 
contain
the value (to set the blob value directly), or ``file`` attribute (to set 
the
blob value from the file).
@@ -2613,11 +2613,11 @@ paravirtualized driver is specified via the ``disk`` 
element.
   ``device`` is 'lun'. :since:`Since 1.0.2`
``snapshot``
   Indicates the default behavior of the disk during disk snapshots:
-  "``internal``" requires a file format such as qcow2 that can store both
-  the snapshot and the data changes since the snapshot; "``external``" will
-  separate the snapshot from the live data; "``no``" means the disk will
+  ``internal`` requires a file format such as qcow2 that can store both
+  the snapshot and the data changes since the snapshot; ``external`` will
+  separate the snapshot from the live data; ``no`` means the disk will
   not participate in snapshots; and ``manual`` allows snapshotting done via
-  an unmanaged storage provider. Read-only disks default to "``no``", while
+  an unmanaged storage provider. Read-only disks default to ``no``, while
   the default for other disks depends on the hypervisor's capabilities.
   Some hypervisors allow a per-snapshot choice as well, during `domain
   snapshot creation `__. Not all snapshot modes are
@@ -5001,10 +5001,10 @@ directly reference an SRIOV VF device:
...

 The  element required attribute ``type`` will be set to either
-``"persistent"`` to indicate a device that should always be present in the
-domain, or ``"transient"`` to indicate a device that may periodically be
+``persistent`` to indicate a device that should always be present in the
+domain, or ``transient`` to indicate a device that may periodically be
 removed, then later re-added to the domain. When type="transient", there should
-be a second attribute to  called ``"persistent"`` - this attribute
+be a second attribute to  called ``persistent`` - this attribute
 should be set to the alias name of the other device in the pair (the one that
 has ``

[PATCH 0/2] Fix missing 'manual' mode for default snapshot disk definition and clean up docs

2022-05-23 Thread Peter Krempa

Peter Krempa (2):
  docs: domain: Remove extraneous quotes
  schemas: Allow 'manual' snapshot mode in domain definition

 docs/formatdomain.rst | 20 ++--
 src/conf/schemas/domaincommon.rng |  1 +
 2 files changed, 11 insertions(+), 10 deletions(-)

-- 
2.35.3

Re: [libvirt PATCH v2 0/7] po: Various fixes and cleanups

2022-05-23 Thread Michal Prívozník

On 5/18/22 11:52, Andrea Bolognani wrote:
> Changes from [v1]
> 
>   * instead of checking that the potfile doesn't contain unwanted
> comments at syntax-check time, prevent them from being added by
> passing all xgettext options explicitly ourselves.
> 
> [v1] https://listman.redhat.com/archives/libvir-list/2022-May/231526.html
> 
> Andrea Bolognani (7):
>   po: Drop unwanted comments from potfile
>   po: Stop using 'glib' preset for i18n.gettext()
>   po: Drop prefixes from POTFILES.in
>   po: Don't generate POTFILES
>   syntax-check: Don't exclude src/false.c from sc_po_check
>   po: Sort LINGUAS
>   syntax-check: Introduce sc_linguas_sorting
> 
>  build-aux/syntax-check.mk |  21 ++-
>  po/LINGUAS|   8 +-
>  po/POTFILES   | 384 ++
>  po/POTFILES.in| 384 --
>  po/libvirt.pot| 216 -
>  po/meson.build|  17 +-
>  6 files changed, 406 insertions(+), 624 deletions(-)
>  create mode 100644 po/POTFILES
>  delete mode 100644 po/POTFILES.in
> 

Reviewed-by: Michal Privoznik 

Michal

Re: [PATCH v2] remote_daemon: Don't run virStateCleanup() if virStateReload() is still running

2022-05-23 Thread Martin Kletzander


On Thu, May 19, 2022 at 04:49:32PM +0200, Michal Privoznik wrote:

When a SIGHUP is received a thread is spawned that runs
virStateReload(). However, if SIGINT is received while the former
thread is still running then we may get into problematic
situation: the cleanup code in main() sees drivers initialized
and thus calls virStateCleanup(). So now we have two threads, one
running virStateReload() the other virStateCleanup(). In this
situation it's very likely that a race condition occurs and
either of threads causes SIGSEGV.

To fix this, unmark drivers as initialized in the
virStateReload() thread for the time the function runs.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2075837
Signed-off-by: Michal Privoznik 


Reviewed-by: Martin Kletzander 


---

v2 of:

https://listman.redhat.com/archives/libvir-list/2022-April/230415.html

diff to v1:
- reworked how int is set (instead of inc/dec I'm using set(0)/set(1))
so that reload can be attempted again and again if previous attempt
failed.



Looks perfect, thanks for taking the time to work my suggestions in.


signature.asc
Description: PGP signature

Re: [libvirt PATCH] qemu: Fix error propagation in qemuMigrationBegin

2022-05-23 Thread Peter Krempa

On Mon, May 23, 2022 at 12:55:17 +0200, Jiri Denemark wrote:
> Commit v8.3.0-152-g49ef0f95c6 removed explicit VIR_FREE from
> qemuMigrationBegin, effectively reverting v1.2.14-57-g77ddd0bba2
> 
> The xml variable was used to hold the return value and thus had to be
> unset when an error happened after xml was already non-NULL. Such code
> may be quite confusing though and we usually avoid it by not storing
> anything to a return variable until everything succeeded.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_migration.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)

Oops.

Reviewed-by: Peter Krempa

[libvirt PATCH] qemu: Fix error propagation in qemuMigrationBegin

2022-05-23 Thread Jiri Denemark

Commit v8.3.0-152-g49ef0f95c6 removed explicit VIR_FREE from
qemuMigrationBegin, effectively reverting v1.2.14-57-g77ddd0bba2

The xml variable was used to hold the return value and thus had to be
unset when an error happened after xml was already non-NULL. Such code
may be quite confusing though and we usually avoid it by not storing
anything to a return variable until everything succeeded.

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_migration.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 438f2bc999..38596fa4de 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2487,6 +2487,7 @@ qemuMigrationSrcBegin(virConnectPtr conn,
 virQEMUDriver *driver = conn->privateData;
 g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
 g_autofree char *xml = NULL;
+char *ret = NULL;
 virDomainAsyncJob asyncJob;
 
 if (cfg->migrateTLSForce &&
@@ -2538,9 +2539,11 @@ qemuMigrationSrcBegin(virConnectPtr conn,
 goto endjob;
 }
 
+ret = g_steal_pointer();
+
  cleanup:
 virDomainObjEndAPI();
-return g_steal_pointer();
+return ret;
 
  endjob:
 if (flags & VIR_MIGRATE_CHANGE_PROTECTION)
-- 
2.35.1

Re: [PATCH] qemuxml2xmltests.c: convert pseries tests to DO_TEST_CAPS_ARCH_LATEST

2022-05-23 Thread Martin Kletzander


On Sat, May 21, 2022 at 01:59:05PM -0300, Daniel Henrique Barboza wrote:

qemuxml2xmltests that have "pseries" in the name now use the
DO_TEST_CAPS_LATEST_ARCH() macro.

Signed-off-by: Daniel Henrique Barboza 


Reviewed-by: Martin Kletzander 


signature.asc
Description: PGP signature

Re: [PATCH 3/3] qemuxml2argvtest.c: use CAPS_ARCH_LATEST() with pseries-cpu-compat-power9

2022-05-23 Thread Martin Kletzander


On Fri, May 20, 2022 at 05:47:04PM -0300, Daniel Henrique Barboza wrote:

Use the newly added ARG_CAPS_HOST_CPU_MODEL to set which host CPU we
expect the test to use - the test should fail when using a POWER8 host
cpu but complete when using a POWER9 host cpu.

Two new macros were added because we will be adding similar tests in the
near future when adding support for the Power10 chip.

Signed-off-by: Daniel Henrique Barboza 


Reviewed-by: Martin Kletzander 


signature.asc
Description: PGP signature

Re: [PATCH 2/3] testutilsqemu: introduce ARG_CAPS_HOST_CPU_MODEL

2022-05-23 Thread Martin Kletzander


On Fri, May 20, 2022 at 05:47:03PM -0300, Daniel Henrique Barboza wrote:

When loading a latest caps for an arch for the first time the following
occurs in testQemuInfoInitArgs():

- the caps file is located. It's not in the cache since it's the first time
it's being read;
- the cachecaps are retrieved using qemuTestParseCapabilitiesArch() and
stored in the capscache;
- FLAG_REAL_CAPS is set and regular flow continues.



I must say the FLAG_REAL_CAPS is a little bit confusing in our tests.


Loading the same latest caps for the second time the caps are loaded from the
cache, skipping qemuTestParseCapabilitiesArch(). By skipping this function it
means that it also skips virQEMUCapsLoadCache() and, more relevant to
our case, virQEMUCapsInitHostCPUModel(). This function will use the
current arch and cpuModel settings to write the qemuCaps that are being
stored in the cache. And we're also setting FLAG_REAL_CAPS, meaning that
we won't be updating the qemucaps host model via testUpdateQEMUCaps() as
well.

This has side-effects such as:

- the first time the latest caps for an arch is loaded determines the
cpuModel it'll use during the current qemuxml2argvtest run. For
example, when running all tests, the first time the latest ppc64 caps
are read is on "disk-floppy-pseries" test. Since the current host arch
at this point is x86_64, the cpuModel that will be set for this
capability is "core2duo";

- every other latest arch test will use the same hostCPU as the first
one set since we read it from the cache after the first run.
qemuTestSetHostCPU() makes no difference because we won't update the
host model due to FLAG_REAL_CAPS being set. Using the previous example,
every other latest ppc64 test that will be run will be using the
"core2duo" cpuModel.

Using fake capabilities (e.g. using DO_TEST()) prevent FLAG_REAL_CAPS
to


s/prevent/prevents/



be set, meaning that the cpuModel will be updated using the current
settings the test is being ran due to testUpdateQEMUCaps().

Note that not all latest caps arch tests care about the cpuModel being
set to an unexpected default cpuModel. But some tests will care, e.g.
"pseries-cpu-compat-power9", and changing it from DO_TEST() to
DO_TEST_CAPS_ARCH_LATEST() will make it fail every time the
"disk-floppy-pseries" is being ran first.

One way of fixing it is to rethink all the existing logic, for example
not setting FLAG_REAL_CAPS for latest arch tests. Another way is
presented here. ARGS_CAPS_HOST_CPU_MODEL is a new testQemuInfo arg that
allow us to set any specific host CPU model we want when running latest
arch caps tests. This new arg can then be used when converting existing
DO_TEST() testcases to DO_TEST_CAPS_ARCH_LATEST() that requires a
specific host CPU setting to be successful, which we're going to do in
the next patch with "pseries-cpu-compat-power9".

Signed-off-by: Daniel Henrique Barboza 


Reviewed-by: Martin Kletzander 

I hope I understood all the nuances, but it seems reasonable.


signature.asc
Description: PGP signature

Re: [PATCH 1/3] qemu_capspriv.h: fix identation

2022-05-23 Thread Martin Kletzander


On Fri, May 20, 2022 at 05:47:02PM -0300, Daniel Henrique Barboza wrote:

Fix identation of virQEMUCapsUpdateHostCPUModel() params.

Signed-off-by: Daniel Henrique Barboza 


trivial,

Reviewed-by: Martin Kletzander 


---
src/qemu/qemu_capspriv.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_capspriv.h b/src/qemu/qemu_capspriv.h
index f4f4a99d32..26bf2d3571 100644
--- a/src/qemu/qemu_capspriv.h
+++ b/src/qemu/qemu_capspriv.h
@@ -63,8 +63,8 @@ virQEMUCapsInitHostCPUModel(virQEMUCaps *qemuCaps,

void
virQEMUCapsUpdateHostCPUModel(virQEMUCaps *qemuCaps,
-virArch hostArch,
-virDomainVirtType type);
+  virArch hostArch,
+  virDomainVirtType type);
int
virQEMUCapsInitCPUModel(virQEMUCaps *qemuCaps,
virDomainVirtType type,
--
2.32.0



signature.asc
Description: PGP signature

Re: [PATCH RFC 00/10] qemu: Enable SCHED_CORE for domains and helper processes

2022-05-23 Thread Michal Prívozník

On 5/18/22 14:48, Michal Prívozník wrote:
> On 5/9/22 17:02, Michal Privoznik wrote:
>>
> 
> Polite ping.

Less polite ping.

Michal

Re: [libvirt PATCH] apparmor: Enable locking AAVMF firmware

2022-05-23 Thread Martin Kletzander


On Mon, May 23, 2022 at 10:33:39AM +0200, Andrea Bolognani wrote:

We already allow this for OVMF.

Closes: https://gitlab.com/libvirt/libvirt/-/issues/312
Signed-off-by: Andrea Bolognani 


Reviewed-by: Martin Kletzander 


---
src/security/apparmor/libvirt-qemu | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/security/apparmor/libvirt-qemu 
b/src/security/apparmor/libvirt-qemu
index c29168da27..02ee273e7e 100644
--- a/src/security/apparmor/libvirt-qemu
+++ b/src/security/apparmor/libvirt-qemu
@@ -78,7 +78,7 @@
  /var/lib/dbus/machine-id r,

  # access to firmware's etc
-  /usr/share/AAVMF/** r,
+  /usr/share/AAVMF/** rk,
  /usr/share/bochs/** r,
  /usr/share/edk2-ovmf/** rk,
  /usr/share/kvm/** r,
--
2.35.3



signature.asc
Description: PGP signature

[libvirt PATCH] apparmor: Enable locking AAVMF firmware

2022-05-23 Thread Andrea Bolognani

We already allow this for OVMF.

Closes: https://gitlab.com/libvirt/libvirt/-/issues/312
Signed-off-by: Andrea Bolognani 
---
 src/security/apparmor/libvirt-qemu | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/security/apparmor/libvirt-qemu 
b/src/security/apparmor/libvirt-qemu
index c29168da27..02ee273e7e 100644
--- a/src/security/apparmor/libvirt-qemu
+++ b/src/security/apparmor/libvirt-qemu
@@ -78,7 +78,7 @@
   /var/lib/dbus/machine-id r,
 
   # access to firmware's etc
-  /usr/share/AAVMF/** r,
+  /usr/share/AAVMF/** rk,
   /usr/share/bochs/** r,
   /usr/share/edk2-ovmf/** rk,
   /usr/share/kvm/** r,
-- 
2.35.3

71 matches

Mail list logo