Re: [libvirt PATCH v3] Conf: Move validation of virDomainGraphicsListenDef out of Parser

[Shiva]

On 4/6/23 20:43, Shiva wrote:

Now, earlier in this loop (not visible in this limited context though)
there's a check for VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK type. I
don't think it is special so we have two options:

   a) move it into virDomainGraphicsListenDefValidate(), or
   b) move those checks out of virDomainGraphicsListenDefValidate() 
right

into this loop; rendering virDomainGraphicsListenDefValidate() to be an
empty function which can then be dropped.

I don't have any preference, do you?

Michal


Greetings Sir

Let's go with option b), as it reduces the number of functions, 
moreover the function virDomainGraphicsListenDefValidate() has a 
pretty similar name to it's caller 
virDomainGraphicsDefListensValidate() and could be kindof annoying.


Do I apply your suggestion and resend the patch?

Thank You

Shiva


My apologies for cluttering the mailing list.

I have applied your suggestions and have sent a v4 Patch, signed with my 
full name.


I have also squashed v4 with v3.

Thank you

Shiva

Re: [libvirt PATCH v4] Conf: Move validation of virDomainGraphicsListenDef out of Parser

[K Shiva]

In an effort to separate the validation steps from the Parse stage,
a few validation checks of virDomainGraphicsListenDef have been moved from
virDomainGraphicsListenDefParseXML() in domain_conf.c to
virDomainGraphicsDefListensValidate() in domain_validate.c

Signed-off-by: Koninty Shiva Kiran 
---
 src/conf/domain_conf.c | 30 +-
 src/conf/domain_validate.c | 37 +++--
 2 files changed, 32 insertions(+), 35 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 70e4d52ee6..8df751cc46 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -10986,7 +10986,6 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr node,
 /**
  * virDomainGraphicsListenDefParseXML:
  * @def: listen def pointer to be filled
- * @graphics: graphics def pointer
  * @node: xml node of  element
  * @parent: xml node of  element
  * @flags: bit-wise or of VIR_DOMAIN_DEF_PARSE_*
@@ -10998,13 +10997,11 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr node,
  */
 static int
 virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDef *def,
-   virDomainGraphicsDef *graphics,
xmlNodePtr node,
xmlNodePtr parent,
unsigned int flags)
 {
 int ret = -1;
-const char *graphicsType = virDomainGraphicsTypeToString(graphics->type);
 g_autofree char *address = virXMLPropString(node, "address");
 g_autofree char *network = virXMLPropString(node, "network");
 g_autofree char *socketPath = virXMLPropString(node, "socket");
@@ -11021,31 +11018,6 @@ 
virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDef *def,
VIR_XML_PROP_REQUIRED, >type) < 0)
 goto error;
 
-switch (def->type) {
-case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET:
-if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
-graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("listen type 'socket' is not available for 
graphics type '%1$s'"),
-   graphicsType);
-goto error;
-}
-break;
-case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE:
-if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
-graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("listen type 'none' is not available for graphics 
type '%1$s'"),
-   graphicsType);
-goto error;
-}
-break;
-case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS:
-case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK:
-case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_LAST:
-break;
-}
-
 if (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS) {
 if (address && addressCompat && STRNEQ(address, addressCompat)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -11148,7 +11120,7 @@ virDomainGraphicsListensParseXML(virDomainGraphicsDef 
*def,
 def->listens = g_new0(virDomainGraphicsListenDef, nListens);
 
 for (i = 0; i < nListens; i++) {
-if (virDomainGraphicsListenDefParseXML(>listens[i], def,
+if (virDomainGraphicsListenDefParseXML(>listens[i],
listenNodes[i],
i == 0 ? node : NULL,
flags) < 0)
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 9265fef4f9..988f0854a5 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -2631,14 +2631,39 @@ static int
 virDomainGraphicsDefListensValidate(const virDomainGraphicsDef *def)
 {
 size_t i;
+const char *graphicsType = virDomainGraphicsTypeToString(def->type);
 
 for (i = 0; i < def->nListens; i++) {
-if (def->listens[i].type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK &&
-!def->listens[i].network) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
-   _("'network' attribute is required for "
- "listen type 'network'"));
-return -1;
+switch (def->listens[i].type) {
+case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK:
+if (!def->listens[i].network) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("'network' attribute is required for "
+ "listen type 'network'"));
+return -1;
+}
+break;
+case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET:
+if (def->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
+def->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ 

Re: [libvirt PATCH v3] Conf: Move validation of virDomainGraphicsListenDef out of Parser virDomainGraphicsListenDef out of Parser

[K Shiva]

In an effort to separate the validation steps from the Parse stage,
a few validation checks of virDomainGraphicsListenDef have been moved from
virDomainGraphicsListenDefParseXML() in domain_conf.c to
virDomainGraphicsDefListensValidate() in domain_validate.c

Signed-off-by: Koninty Shiva Kiran 
---
Change to previous mail:
https://listman.redhat.com/archives/libvir-list/2023-April/239286.html
- Previously forgot to add full name under signoff, added it here

 src/conf/domain_conf.c | 30 +-
 src/conf/domain_validate.c | 37 +++--
 2 files changed, 32 insertions(+), 35 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 70e4d52ee6..8df751cc46 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -10986,7 +10986,6 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr node,
 /**
  * virDomainGraphicsListenDefParseXML:
  * @def: listen def pointer to be filled
- * @graphics: graphics def pointer
  * @node: xml node of  element
  * @parent: xml node of  element
  * @flags: bit-wise or of VIR_DOMAIN_DEF_PARSE_*
@@ -10998,13 +10997,11 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr node,
  */
 static int
 virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDef *def,
-   virDomainGraphicsDef *graphics,
xmlNodePtr node,
xmlNodePtr parent,
unsigned int flags)
 {
 int ret = -1;
-const char *graphicsType = virDomainGraphicsTypeToString(graphics->type);
 g_autofree char *address = virXMLPropString(node, "address");
 g_autofree char *network = virXMLPropString(node, "network");
 g_autofree char *socketPath = virXMLPropString(node, "socket");
@@ -11021,31 +11018,6 @@ 
virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDef *def,
VIR_XML_PROP_REQUIRED, >type) < 0)
 goto error;
 
-switch (def->type) {
-case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET:
-if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
-graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("listen type 'socket' is not available for 
graphics type '%1$s'"),
-   graphicsType);
-goto error;
-}
-break;
-case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE:
-if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
-graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("listen type 'none' is not available for graphics 
type '%1$s'"),
-   graphicsType);
-goto error;
-}
-break;
-case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS:
-case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK:
-case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_LAST:
-break;
-}
-
 if (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS) {
 if (address && addressCompat && STRNEQ(address, addressCompat)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -11148,7 +11120,7 @@ virDomainGraphicsListensParseXML(virDomainGraphicsDef 
*def,
 def->listens = g_new0(virDomainGraphicsListenDef, nListens);
 
 for (i = 0; i < nListens; i++) {
-if (virDomainGraphicsListenDefParseXML(>listens[i], def,
+if (virDomainGraphicsListenDefParseXML(>listens[i],
listenNodes[i],
i == 0 ? node : NULL,
flags) < 0)
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 9265fef4f9..988f0854a5 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -2631,14 +2631,39 @@ static int
 virDomainGraphicsDefListensValidate(const virDomainGraphicsDef *def)
 {
 size_t i;
+const char *graphicsType = virDomainGraphicsTypeToString(def->type);
 
 for (i = 0; i < def->nListens; i++) {
-if (def->listens[i].type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK &&
-!def->listens[i].network) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
-   _("'network' attribute is required for "
- "listen type 'network'"));
-return -1;
+switch (def->listens[i].type) {
+case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK:
+if (!def->listens[i].network) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("'network' attribute is required for "
+ "listen type 'network'"));
+return -1;
+}
+break;
+case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET:
+if (def->type != 

[libvirt PATCH v4] Conf: Move validation of virDomainGraphicsListenDef out of Parser

[K Shiva]

From: K Shiva Kiran 

In an effort to separate the validation steps from the Parse stage,
a few validation checks of virDomainGraphicsListenDef have been moved from
virDomainGraphicsListenDefParseXML() in domain_conf.c to
virDomainGraphicsDefListensValidate() in domain_validate.c

Signed-off-by: K Shiva 
---
This is a v4 of:
https://listman.redhat.com/archives/libvir-list/2023-April/239286.html
This commit has been squashed with v3.
diff to v3:
- Dropped virDomainGraphicsListenDefValidate() as per suggestion
- Moved the above's code to virDomainGraphicsDefListensValidate()
- Moved an existing if() condition in virDomainGraphicsDefListensValidate() 
  into the switch() statement of the freshly moved code

 src/conf/domain_conf.c | 30 +-
 src/conf/domain_validate.c | 37 +++--
 2 files changed, 32 insertions(+), 35 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 70e4d52ee6..8df751cc46 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -10986,7 +10986,6 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr node,
 /**
  * virDomainGraphicsListenDefParseXML:
  * @def: listen def pointer to be filled
- * @graphics: graphics def pointer
  * @node: xml node of  element
  * @parent: xml node of  element
  * @flags: bit-wise or of VIR_DOMAIN_DEF_PARSE_*
@@ -10998,13 +10997,11 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr node,
  */
 static int
 virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDef *def,
-   virDomainGraphicsDef *graphics,
xmlNodePtr node,
xmlNodePtr parent,
unsigned int flags)
 {
 int ret = -1;
-const char *graphicsType = virDomainGraphicsTypeToString(graphics->type);
 g_autofree char *address = virXMLPropString(node, "address");
 g_autofree char *network = virXMLPropString(node, "network");
 g_autofree char *socketPath = virXMLPropString(node, "socket");
@@ -11021,31 +11018,6 @@ 
virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDef *def,
VIR_XML_PROP_REQUIRED, >type) < 0)
 goto error;
 
-switch (def->type) {
-case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET:
-if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
-graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("listen type 'socket' is not available for 
graphics type '%1$s'"),
-   graphicsType);
-goto error;
-}
-break;
-case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE:
-if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
-graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("listen type 'none' is not available for graphics 
type '%1$s'"),
-   graphicsType);
-goto error;
-}
-break;
-case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS:
-case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK:
-case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_LAST:
-break;
-}
-
 if (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS) {
 if (address && addressCompat && STRNEQ(address, addressCompat)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -11148,7 +11120,7 @@ virDomainGraphicsListensParseXML(virDomainGraphicsDef 
*def,
 def->listens = g_new0(virDomainGraphicsListenDef, nListens);
 
 for (i = 0; i < nListens; i++) {
-if (virDomainGraphicsListenDefParseXML(>listens[i], def,
+if (virDomainGraphicsListenDefParseXML(>listens[i],
listenNodes[i],
i == 0 ? node : NULL,
flags) < 0)
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 9265fef4f9..988f0854a5 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -2631,14 +2631,39 @@ static int
 virDomainGraphicsDefListensValidate(const virDomainGraphicsDef *def)
 {
 size_t i;
+const char *graphicsType = virDomainGraphicsTypeToString(def->type);
 
 for (i = 0; i < def->nListens; i++) {
-if (def->listens[i].type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK &&
-!def->listens[i].network) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
-   _("'network' attribute is required for "
- "listen type 'network'"));
-return -1;
+switch (def->listens[i].type) {
+case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK:
+if (!def->listens[i].network) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   

[libvirt PATCH 3/3] rpc/ssh: ssh_userauth_agent() is not supported on win32

[marcandre . lureau]

From: Marc-André Lureau 

The function does not exist on win32.

Signed-off-by: Marc-André Lureau 
---
 src/rpc/virnetlibsshsession.c | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c
index e71a79d0fb..e94b0d7a2e 100644
--- a/src/rpc/virnetlibsshsession.c
+++ b/src/rpc/virnetlibsshsession.c
@@ -60,7 +60,9 @@ typedef enum {
 VIR_NET_LIBSSH_AUTH_KEYBOARD_INTERACTIVE,
 VIR_NET_LIBSSH_AUTH_PASSWORD,
 VIR_NET_LIBSSH_AUTH_PRIVKEY,
-VIR_NET_LIBSSH_AUTH_AGENT
+#ifndef WIN32
+VIR_NET_LIBSSH_AUTH_AGENT,
+#endif
 } virNetLibsshAuthMethods;
 
 
@@ -698,6 +700,7 @@ virNetLibsshAuthenticate(virNetLibsshSession *sess)
 /* try to authenticate using the keyboard interactive way */
 ret = virNetLibsshAuthenticateKeyboardInteractive(sess, auth);
 break;
+#ifndef WIN32
 case VIR_NET_LIBSSH_AUTH_AGENT:
 /* try to authenticate using ssh-agent */
 ret = ssh_userauth_agent(sess->session, NULL);
@@ -708,6 +711,7 @@ virNetLibsshAuthenticate(virNetLibsshSession *sess)
errmsg);
 }
 break;
+#endif
 case VIR_NET_LIBSSH_AUTH_PRIVKEY:
 /* try to authenticate using the provided ssh key */
 ret = virNetLibsshAuthenticatePrivkey(sess, auth);
@@ -861,8 +865,13 @@ virNetLibsshSessionAuthAddPasswordAuth(virNetLibsshSession 
*sess,
 }
 
 int
-virNetLibsshSessionAuthAddAgentAuth(virNetLibsshSession *sess)
+virNetLibsshSessionAuthAddAgentAuth(virNetLibsshSession *sess G_GNUC_UNUSED)
 {
+#ifdef WIN32
+virReportError(VIR_ERR_LIBSSH, "%s",
+   _("Agent authentication is not supported on this host"));
+return -1;
+#else
 virNetLibsshAuthMethod *auth;
 
 virObjectLock(sess);
@@ -873,6 +882,7 @@ virNetLibsshSessionAuthAddAgentAuth(virNetLibsshSession 
*sess)
 
 virObjectUnlock(sess);
 return 0;
+#endif
 }
 
 int
-- 
2.39.2

[libvirt PATCH 2/3] meson: drop explicit python interpreter

[marcandre . lureau]

From: Marc-André Lureau 

meson wraps python scripts already on win32, so we end up with these
failing commands:

[1/359] "C:/msys64/ucrt64/bin/meson" "--internal" "exe" "--capture" 
"src/util/virkeycodetable_atset1.h" "--" "sh" 
"C:/msys64/home/marca/src/libvirt/scripts/meson-python.sh" 
"C:/msys64/ucrt64/bin/python3.EXE" "python" 
"C:/msys64/home/marca/src/libvirt/src/keycodemapdb/tools/keymap-gen" 
"code-table" "--lang" "stdc" "--varname" "virKeyCodeTable_atset1" 
"C:/msys64/home/marca/src/libvirt/src/keycodemapdb/data/keymaps.csv" "atset1"
FAILED: src/util/virkeycodetable_atset1.h
"C:/msys64/ucrt64/bin/meson" "--internal" "exe" "--capture" 
"src/util/virkeycodetable_atset1.h" "--" "sh" 
"C:/msys64/home/marca/src/libvirt/scripts/meson-python.sh" 
"C:/msys64/ucrt64/bin/python3.EXE" "python" 
"C:/msys64/home/marca/src/libvirt/src/keycodemapdb/tools/keymap-gen" 
"code-table" "--lang" "stdc" "--varname" "virKeyCodeTable_atset1" 
"C:/msys64/home/marca/src/libvirt/src/keycodemapdb/data/keymaps.csv" "atset1"

If LC_ALL, LANG and LC_CTYPE need to be set, it would probably be better
to use a meson environment() instead.

Signed-off-by: Marc-André Lureau 
---
 docs/manpages/meson.build | 4 ++--
 docs/meson.build  | 6 ++
 src/admin/meson.build | 4 ++--
 src/esx/meson.build   | 4 ++--
 src/hyperv/meson.build| 2 +-
 src/meson.build   | 8 
 src/util/meson.build  | 4 ++--
 7 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/docs/manpages/meson.build b/docs/manpages/meson.build
index 84b2e247e9..afcadaefbd 100644
--- a/docs/manpages/meson.build
+++ b/docs/manpages/meson.build
@@ -49,7 +49,7 @@ foreach name : keycode_list
 input: keymap_src_file,
 output: 'virkeycode-@0@.rst'.format(name),
 command: [
-  meson_python_prog, python3_prog, keymap_gen_prog, 'code-docs',
+  keymap_gen_prog, 'code-docs',
   '--lang', 'rst',
   '--title', 'virkeycode-@0@'.format(name),
   '--subtitle', 'Key code values for @0@'.format(name),
@@ -70,7 +70,7 @@ foreach name : keyname_list
 input: keymap_src_file,
 output: 'virkeyname-@0@.rst'.format(name),
 command: [
-  meson_python_prog, python3_prog, keymap_gen_prog, 'name-docs',
+  keymap_gen_prog, 'name-docs',
   '--lang', 'rst',
   '--title', 'virkeyname-@0@'.format(name),
   '--subtitle', 'Key name values for @0@'.format(name),
diff --git a/docs/meson.build b/docs/meson.build
index 8f84d08912..769efe7b6a 100644
--- a/docs/meson.build
+++ b/docs/meson.build
@@ -123,7 +123,7 @@ aclperms_gen = custom_target(
   input: access_perm_h,
   output: 'aclperms.htmlinc',
   command: [
-meson_python_prog, python3_prog, genaclperms_prog, '@INPUT@',
+genaclperms_prog, '@INPUT@',
   ],
   capture: true,
 )
@@ -145,7 +145,7 @@ docs_api_generated = custom_target(
 'libvirt-admin-api.xml',
   ],
   command: [
-meson_python_prog, python3_prog, apibuild_prog,
+apibuild_prog,
 meson.current_source_dir(),
 meson.current_build_dir(),
   ],
@@ -264,8 +264,6 @@ hvsupport_html_in = custom_target(
   'hvsupport.html.in',
   output: 'hvsupport.html.in',
   command: [
-meson_python_prog,
-python3_prog,
 hvsupport_prog,
 meson.project_source_root(),
 meson.project_build_root(),
diff --git a/src/admin/meson.build b/src/admin/meson.build
index 692cc128a3..e04d610f92 100644
--- a/src/admin/meson.build
+++ b/src/admin/meson.build
@@ -68,7 +68,7 @@ libvirt_admin_syms = custom_target(
   ],
   output: 'libvirt_admin.syms',
   command: [
-meson_python_prog, python3_prog, meson_gen_sym_prog,
+meson_gen_sym_prog,
 '@OUTPUT@', 'LIBVIRT_ADMIN_PRIVATE_' + meson.project_version(), '@INPUT@',
   ],
 )
@@ -79,7 +79,7 @@ if host_machine.system() == 'windows'
 input: libvirt_admin_syms,
 output: 'libvirt_admin.def',
 command: [
-  meson_python_prog, python3_prog, meson_gen_def_prog,
+  meson_gen_def_prog,
   '@INPUT@', '@OUTPUT@',
 ],
   )
diff --git a/src/esx/meson.build b/src/esx/meson.build
index d1f42fdcc4..4bd0dadd51 100644
--- a/src/esx/meson.build
+++ b/src/esx/meson.build
@@ -25,7 +25,7 @@ esx_gen_headers = custom_target(
 'esx_vi_types.generated.typeenum',
   ],
   command: [
-meson_python_prog, python3_prog, esx_vi_generator_prog,
+esx_vi_generator_prog,
 meson.project_source_root() / 'src',
 meson.project_build_root() / 'src',
 'header',
@@ -46,7 +46,7 @@ esx_gen_sources = custom_target(
 'esx_vi_types.generated.typetostring',
   ],
   command: [
-meson_python_prog, python3_prog, esx_vi_generator_prog,
+esx_vi_generator_prog,
 meson.project_source_root() / 'src',
 meson.project_build_root() / 'src',
 'source',
diff --git a/src/hyperv/meson.build b/src/hyperv/meson.build
index 3509ce12f7..446b6ddada 100644
--- a/src/hyperv/meson.build
+++ b/src/hyperv/meson.build
@@ -17,7 +17,7 @@ hyperv_gen_sources = custom_target(
 'hyperv_wmi_classes.generated.typedef',
   ],
   

[libvirt PATCH 1/3] meson: don't look for unix paths on win32

[marcandre . lureau]

From: Marc-André Lureau 

Or meson will complain with:
../meson.build:770:2: ERROR: Search directory /sbin is not an absolute path.

Signed-off-by: Marc-André Lureau 
---
 meson.build | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/meson.build b/meson.build
index 47ec7fcb74..f7f31a278d 100644
--- a/meson.build
+++ b/meson.build
@@ -742,11 +742,15 @@ conf.set('SIZEOF_LONG', cc.sizeof('long'))
 
 # Where we look for daemons and admin binaries during configure
 
-libvirt_sbin_path = [
-  '/sbin',
-  '/usr/sbin',
-  '/usr/local/sbin',
-]
+libvirt_sbin_path = []
+
+if host_machine.system() != 'windows'
+  libvirt_sbin_path += [
+'/sbin',
+'/usr/sbin',
+'/usr/local/sbin',
+  ]
+endif
 
 
 # required programs check
-- 
2.39.2

[libvirt PATCH 0/3] RFC: fix compilation on msys2

[marcandre . lureau]

From: Marc-André Lureau 

Hi,

libvirt fails to compile on msys2/win32. Here is a few patches that solve it,
but the way python scripts are being handled in general is a bit odd, so this is
RFC. (fwiw, a lot of tests fail though, I can send the log if anyone is
interested - or attach it to a gitlab issue)

Fixes:
https://gitlab.com/libvirt/libvirt/-/issues/453

Marc-André Lureau (3):
  meson: don't look for unix paths on win32
  meson: drop explicit python interpreter
  rpc/ssh: ssh_userauth_agent() is not supported on win32

 docs/manpages/meson.build |  4 ++--
 docs/meson.build  |  6 ++
 meson.build   | 14 +-
 src/admin/meson.build |  4 ++--
 src/esx/meson.build   |  4 ++--
 src/hyperv/meson.build|  2 +-
 src/meson.build   |  8 
 src/rpc/virnetlibsshsession.c | 14 --
 src/util/meson.build  |  4 ++--
 9 files changed, 36 insertions(+), 24 deletions(-)

-- 
2.39.2

Re: [libvirt PATCH v3] Conf: Move validation of virDomainGraphicsListenDef out of Parser

[Shiva]

Now, earlier in this loop (not visible in this limited context though)
there's a check for VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK type. I
don't think it is special so we have two options:

   a) move it into virDomainGraphicsListenDefValidate(), or
   b) move those checks out of virDomainGraphicsListenDefValidate() right
into this loop; rendering virDomainGraphicsListenDefValidate() to be an
empty function which can then be dropped.

I don't have any preference, do you?

Michal


Greetings Sir

Let's go with option b), as it reduces the number of functions, moreover 
the function virDomainGraphicsListenDefValidate() has a pretty similar 
name to it's caller virDomainGraphicsDefListensValidate() and could be 
kindof annoying.


Do I apply your suggestion and resend the patch?

Thank You

Shiva

Re: [libvirt PATCH v3] Conf: Move validation of virDomainGraphicsListenDef out of Parser

[Michal Prívozník]

On 4/6/23 14:51, K Shiva wrote:
> In an effort to separate the validation steps from the Parse stage, a
> part of the validation of virDomainGraphicsListenDef has been moved to
> domain_validate.h.
> 
> Signed-off-by: K Shiva 

Sorry for not raising this earlier, but we tend to use legal names here.

> ---
> This is a v3 of:
> https://listman.redhat.com/archives/libvir-list/2023-April/239265.html
> 
> diff to v2:
> - Cleaned patch as to be directly applied onto master branch
> 
>  src/conf/domain_conf.c | 30 +-
>  src/conf/domain_validate.c | 35 +++
>  2 files changed, 36 insertions(+), 29 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 70e4d52ee6..8df751cc46 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -10986,7 +10986,6 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr node,
>  /**
>   * virDomainGraphicsListenDefParseXML:
>   * @def: listen def pointer to be filled
> - * @graphics: graphics def pointer
>   * @node: xml node of  element
>   * @parent: xml node of  element
>   * @flags: bit-wise or of VIR_DOMAIN_DEF_PARSE_*
> @@ -10998,13 +10997,11 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr node,
>   */
>  static int
>  virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDef *def,
> -   virDomainGraphicsDef *graphics,
> xmlNodePtr node,
> xmlNodePtr parent,
> unsigned int flags)
>  {
>  int ret = -1;
> -const char *graphicsType = virDomainGraphicsTypeToString(graphics->type);
>  g_autofree char *address = virXMLPropString(node, "address");
>  g_autofree char *network = virXMLPropString(node, "network");
>  g_autofree char *socketPath = virXMLPropString(node, "socket");
> @@ -11021,31 +11018,6 @@ 
> virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDef *def,
> VIR_XML_PROP_REQUIRED, >type) < 0)
>  goto error;
>  
> -switch (def->type) {
> -case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET:
> -if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
> -graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -   _("listen type 'socket' is not available for 
> graphics type '%1$s'"),
> -   graphicsType);
> -goto error;
> -}
> -break;
> -case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE:
> -if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
> -graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -   _("listen type 'none' is not available for 
> graphics type '%1$s'"),
> -   graphicsType);
> -goto error;
> -}
> -break;
> -case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS:
> -case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK:
> -case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_LAST:
> -break;
> -}
> -
>  if (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS) {
>  if (address && addressCompat && STRNEQ(address, addressCompat)) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> @@ -11148,7 +11120,7 @@ virDomainGraphicsListensParseXML(virDomainGraphicsDef 
> *def,
>  def->listens = g_new0(virDomainGraphicsListenDef, nListens);
>  
>  for (i = 0; i < nListens; i++) {
> -if (virDomainGraphicsListenDefParseXML(>listens[i], def,
> +if (virDomainGraphicsListenDefParseXML(>listens[i],
> listenNodes[i],
> i == 0 ? node : NULL,
> flags) < 0)
> diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
> index 9265fef4f9..9a1a8ee156 100644
> --- a/src/conf/domain_validate.c
> +++ b/src/conf/domain_validate.c
> @@ -2627,6 +2627,39 @@ virDomainAudioDefValidate(const virDomainDef *def,
>  return 0;
>  }
>  
> +static int
> +virDomainGraphicsListenDefValidate(const virDomainGraphicsListenDef *def,
> +   const virDomainGraphicsDef *graphics)
> +{
> +const char *graphicsType = virDomainGraphicsTypeToString(graphics->type);
> +
> +switch (def->type) {
> +case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET:
> +if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
> +graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("listen type 'socket' is not available for 
> graphics type '%1$s'"),
> +   graphicsType);
> +return -1;
> +}
> +break;
> +case 

Re: Improve default machine type selection

[Andrea Bolognani]

On Thu, Apr 06, 2023 at 06:10:11AM -0700, Andrea Bolognani wrote:
> In conclusion, there currently doesn't seem to exist a way to define
> a useful integratorcp-based VM in libvirt, which IMO means we can
> safely change the default machine type for Arm architectures without
> any concerns about breaking existing VMs.
>
> I will look into whether the same can be said for RISC-V
> architectures. Hopefully that's the case.

Yeah, the spike machine on RISC-V is unusable in basically the same
ways that the integratorcp machine is on Arm. Let's just change the
default to virt on all of those then :)

-- 
Andrea Bolognani / Red Hat / Virtualization

[libvirt PATCH] util: acpi: include unistd.h

[Ján Tomko]

For lseek.

Signed-off-by: Ján Tomko 
---
Pushed as a build breaker fix.

 src/util/viracpi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/util/viracpi.c b/src/util/viracpi.c
index e66df99f03..c96b7c296b 100644
--- a/src/util/viracpi.c
+++ b/src/util/viracpi.c
@@ -22,6 +22,7 @@
 
 #include 
 #include 
+#include 
 
 #define LIBVIRT_VIRACPIPRIV_H_ALLOW
 #include "internal.h"
-- 
2.39.2

Re: [PATCH] qemu: snapshot: Allow inactive internal snapshots with uefi

[Ján Tomko]

On a Thursday in 2023, Peter Krempa wrote:

Historically the snapshot code attempted to forbid internal snapshots
with UEFI both in active and inactive case. Unfortunately due to the
intricacies of UEFI probing this didn't really work for inactive VMs
which made users rely on the feature.

Now with the changes to store detected UEFI environment also in the
inactive definition this broke the feature for those users.

Since the varstore doesn't really change that much in the lifecycle of a
VM it usually is okay to simply leave it as is.

Restore the functionality for inactive snapshots by disabling the check.

In the future when uefi snapshotting will be added the rest of the
condition will also be removed.

Resolves: https://gitlab.com/libvirt/libvirt/-/issues/460
Signed-off-by: Peter Krempa 
---
src/qemu/qemu_snapshot.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

[PATCH] qemu: snapshot: Allow inactive internal snapshots with uefi

[Peter Krempa]

Historically the snapshot code attempted to forbid internal snapshots
with UEFI both in active and inactive case. Unfortunately due to the
intricacies of UEFI probing this didn't really work for inactive VMs
which made users rely on the feature.

Now with the changes to store detected UEFI environment also in the
inactive definition this broke the feature for those users.

Since the varstore doesn't really change that much in the lifecycle of a
VM it usually is okay to simply leave it as is.

Restore the functionality for inactive snapshots by disabling the check.

In the future when uefi snapshotting will be added the rest of the
condition will also be removed.

Resolves: https://gitlab.com/libvirt/libvirt/-/issues/460
Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_snapshot.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 12ddf19c48..91de8b0c31 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -749,11 +749,17 @@ qemuSnapshotPrepare(virDomainObj *vm,
  * - if the variable store is raw, the snapshot fails
  * - allowing a qcow2 image as the varstore would make it eligible to 
receive
  *   the vmstate dump, which would make it huge
- * - offline snapshot would not snapshot the varstore at all
  *
- * Avoid the issues by forbidding internal snapshot with pflash completely.
+ * While offline snapshot would not snapshot the varstore at all, this used
+ * to work as auto-detected UEFI firmware was not present in the offline
+ * definition. Since in most cases the varstore doesn't change it's usually
+ * not an issue. Allow this as there are existing users of this case.
+ *
+ * Avoid the issues by forbidding internal snapshot with pflash if the
+ * VM is active.
  */
-if (found_internal &&
+if (active &&
+found_internal &&
 virDomainDefHasOldStyleUEFI(vm->def)) {
 virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("internal snapshots of a VM with pflash based "
-- 
2.39.2

Re: Improve default machine type selection

[Peter Krempa]

On Thu, Apr 06, 2023 at 06:32:30 -0700, Andrea Bolognani wrote:
> On Thu, Apr 06, 2023 at 09:32:28AM +0200, Peter Krempa wrote:
> > On Wed, Apr 05, 2023 at 15:19:07 -0600, Jim Fehlig wrote:
> > > # virsh create test.xml
> > > error: Failed to create domain from test.xml
> > > error: internal error: process exited while connecting to monitor:
> > > 2023-04-05T20:36:19.564896Z qemu-system-aarch64: Property
> > > 'integratorcp-machine.acpi' not found
> >
> > This is a known issue which should be already fixed if you use the
> > upcoming qemu version.
> >
> > Historically ACPI was controled via '-no-acpi' which is issued when you
> > don't use the '' feature element in the definition. Also
> > historically it was only used on x86. With development of the 'virt'
> > machine type for arm which does use acpi this was un-carefully extended
> > to aarch64 as well where it didn't work with non-virt machines.
> >
> > Now with the latest qemu which reports which machine type actually
> > support ACPI we avoid use of '-no-acpi' for those which don't support
> > it.
> >
> > Unfortunately the workaround of adding  element to definition for
> > a machine which doesn't have it doesn't work either, because a further
> > validation check forces you to configure uefi if you want acpi.
> 
> And even if you try to pile another workaround on top of that one by
> configuring the VM to use EFI, the end result is just a QEMU error:
> 
>   Property 'integratorcp-machine.pflash0' not found
> 
> Further proof that was never possible to build a useful working
> integratorcp-based VM with libvirt.

It was most likely possible before the time when ACPI was added to
'virt' machine because you didn't need to configure a pflash.

But that was a long time ago.

Re: Improve default machine type selection

[Andrea Bolognani]

On Thu, Apr 06, 2023 at 09:32:28AM +0200, Peter Krempa wrote:
> On Wed, Apr 05, 2023 at 15:19:07 -0600, Jim Fehlig wrote:
> > # virsh create test.xml
> > error: Failed to create domain from test.xml
> > error: internal error: process exited while connecting to monitor:
> > 2023-04-05T20:36:19.564896Z qemu-system-aarch64: Property
> > 'integratorcp-machine.acpi' not found
>
> This is a known issue which should be already fixed if you use the
> upcoming qemu version.
>
> Historically ACPI was controled via '-no-acpi' which is issued when you
> don't use the '' feature element in the definition. Also
> historically it was only used on x86. With development of the 'virt'
> machine type for arm which does use acpi this was un-carefully extended
> to aarch64 as well where it didn't work with non-virt machines.
>
> Now with the latest qemu which reports which machine type actually
> support ACPI we avoid use of '-no-acpi' for those which don't support
> it.
>
> Unfortunately the workaround of adding  element to definition for
> a machine which doesn't have it doesn't work either, because a further
> validation check forces you to configure uefi if you want acpi.

And even if you try to pile another workaround on top of that one by
configuring the VM to use EFI, the end result is just a QEMU error:

  Property 'integratorcp-machine.pflash0' not found

Further proof that was never possible to build a useful working
integratorcp-based VM with libvirt.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: Improve default machine type selection

[Andrea Bolognani]

On Wed, Apr 05, 2023 at 03:19:07PM -0600, Jim Fehlig wrote:
> On 3/16/23 11:56, Jim Fehlig wrote:
> > I just did a quick check with libvirt 9.1.0 (qemu is a bit older, at 7.1.0):
> >
> > # cat test.xml
> > 
> >    test
> >    2097152
> >    1
> >    
> >      hvm
> >       > type='pflash'>/usr/share/qemu/aavmf-aarch64-code.bin
> >      
> >      
> >    
> >    
> >    destroy
> >    restart
> >    destroy
> >    
> >      /usr/bin/qemu-system-aarch64
> >      
> >    
> >    
> >    
> >      
> >    
> > 
> > # virsh create test.xml
> > error: Failed to create domain from test.xml
> > error: internal error: Unexpected enum value 0 for 
> > virDomainDeviceAddressType
> >
> > I don't _think_ it's a downstream bug, nor fixed in git in the meantime.
> > It appears running the old integratorcp machine with libivrt is not
> > possible.
>
> I trimmed the config to remove things like virtio devices that are not
> supported by the default machine type, but still it does not work with
> libvirt:
>
> # cat test.xml
> 
>   test
>   2097152
>   1
>   
> hvm
>   
>   
> /usr/bin/qemu-system-aarch64
>   
> 
> # virsh create test.xml
> error: Failed to create domain from test.xml
> error: internal error: process exited while connecting to monitor:
> 2023-04-05T20:36:19.564896Z qemu-system-aarch64: Property
> 'integratorcp-machine.acpi' not found

As explained downthread by Peter, this is no longer the case as of
9.2.0. However, even though an empty integratorcp machine used to be
able to boot in 9.1.0, as you've found out adding *any kind of
device* to it would result in a failure.

That's still the case: if you have a barebones definition such as

  



  

then on startup you'll get

  Unexpected enum value 0 for virDomainDeviceAddressType

(raised by qemuBuildVirtioDevGetConfig:1013). If you add

  

libvirt itself will not have a problem with the configuration, but
QEMU will exit with

  -device 
{"driver":"virtio-blk-device","drive":"libvirt-1-format","id":"virtio-disk0","bootindex":1}:
No 'virtio-bus' bus found for device 'virtio-blk-device'

Finally, if you try using

  

you'll be greeted by

  Could not find PCI controller with index '0' required for device at
address ':00:00.0'

Even going out of your way and adding a

  

will not get you much further, because QEMU will then just report

  -device 
{"driver":"virtio-blk-pci","bus":"pci","addr":"0x0","drive":"libvirt-1-format","id":"virtio-disk0","bootindex":1}:
Bus 'pci' not found

In conclusion, there currently doesn't seem to exist a way to define
a useful integratorcp-based VM in libvirt, which IMO means we can
safely change the default machine type for Arm architectures without
any concerns about breaking existing VMs.

I will look into whether the same can be said for RISC-V
architectures. Hopefully that's the case.

> For
> the second, do any board types other than virt support kvm acceleration? It
> appears to be the only one
>
> https://www.qemu.org/docs/master/system/target-arm.html
>
> I guess it's a slippery slope. The virt board also requires specifying a cpu
> model, with the only reasonable values being host and max
>
> https://www.qemu.org/docs/master/system/arm/cpu-features.html#a-note-about-cpu-models-and-kvm
>
> In fact, that doc implies host is the only choice: "but mostly if KVM is
> enabled the host CPU type must be used". The status quo may by fine for
> domain type qemu, but it seems there's room for improvement for kvm domains.

Yeah, KVM VMs need host-passthrough while TCG VMs need a named CPU
model. There are plans to implement proper named CPU models that work
across accelerators, so hopefully this difference will be removed at
some point in the future.

-- 
Andrea Bolognani / Red Hat / Virtualization

[PATCH] viracpi: Fir error format string in virAcpiParseIORTNodeHeader()

[Michal Privoznik]

Inside of virAcpiParseIORTNodeHeader() there's an
virReportError() which reports size of a structure using sizeof()
operator. Well, it's not well documented but the returned type of
sizeof() is apparently size_t but the format string uses %lu.

Signed-off-by: Michal Privoznik 
---

Pushed under build breaker rule. Also verified this builds successfully
on a 32bit ARM.

 src/util/viracpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/viracpi.c b/src/util/viracpi.c
index 74fff0f0ac..e66df99f03 100644
--- a/src/util/viracpi.c
+++ b/src/util/viracpi.c
@@ -102,7 +102,7 @@ virAcpiParseIORTNodeHeader(int fd,
  * least size of header itself. */
 if (nodeHeader->len < sizeof(*nodeHeader)) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("IORT table node type %1$s has invalid length: got 
%2$u, expected at least %3$lu"),
+   _("IORT table node type %1$s has invalid length: got 
%2$u, expected at least %3$zu"),
NULLSTR(typeStr), (unsigned int)nodeHeader->len, 
sizeof(*nodeHeader));
 return -1;
 }
-- 
2.39.2

[libvirt PATCH v3] Conf: Move validation of virDomainGraphicsListenDef out of Parser

[K Shiva]

In an effort to separate the validation steps from the Parse stage, a
part of the validation of virDomainGraphicsListenDef has been moved to
domain_validate.h.

Signed-off-by: K Shiva 
---
This is a v3 of:
https://listman.redhat.com/archives/libvir-list/2023-April/239265.html

diff to v2:
- Cleaned patch as to be directly applied onto master branch

 src/conf/domain_conf.c | 30 +-
 src/conf/domain_validate.c | 35 +++
 2 files changed, 36 insertions(+), 29 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 70e4d52ee6..8df751cc46 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -10986,7 +10986,6 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr node,
 /**
  * virDomainGraphicsListenDefParseXML:
  * @def: listen def pointer to be filled
- * @graphics: graphics def pointer
  * @node: xml node of  element
  * @parent: xml node of  element
  * @flags: bit-wise or of VIR_DOMAIN_DEF_PARSE_*
@@ -10998,13 +10997,11 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr node,
  */
 static int
 virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDef *def,
-   virDomainGraphicsDef *graphics,
xmlNodePtr node,
xmlNodePtr parent,
unsigned int flags)
 {
 int ret = -1;
-const char *graphicsType = virDomainGraphicsTypeToString(graphics->type);
 g_autofree char *address = virXMLPropString(node, "address");
 g_autofree char *network = virXMLPropString(node, "network");
 g_autofree char *socketPath = virXMLPropString(node, "socket");
@@ -11021,31 +11018,6 @@ 
virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDef *def,
VIR_XML_PROP_REQUIRED, >type) < 0)
 goto error;
 
-switch (def->type) {
-case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET:
-if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
-graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("listen type 'socket' is not available for 
graphics type '%1$s'"),
-   graphicsType);
-goto error;
-}
-break;
-case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE:
-if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
-graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("listen type 'none' is not available for graphics 
type '%1$s'"),
-   graphicsType);
-goto error;
-}
-break;
-case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS:
-case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK:
-case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_LAST:
-break;
-}
-
 if (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS) {
 if (address && addressCompat && STRNEQ(address, addressCompat)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -11148,7 +11120,7 @@ virDomainGraphicsListensParseXML(virDomainGraphicsDef 
*def,
 def->listens = g_new0(virDomainGraphicsListenDef, nListens);
 
 for (i = 0; i < nListens; i++) {
-if (virDomainGraphicsListenDefParseXML(>listens[i], def,
+if (virDomainGraphicsListenDefParseXML(>listens[i],
listenNodes[i],
i == 0 ? node : NULL,
flags) < 0)
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 9265fef4f9..9a1a8ee156 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -2627,6 +2627,39 @@ virDomainAudioDefValidate(const virDomainDef *def,
 return 0;
 }
 
+static int
+virDomainGraphicsListenDefValidate(const virDomainGraphicsListenDef *def,
+   const virDomainGraphicsDef *graphics)
+{
+const char *graphicsType = virDomainGraphicsTypeToString(graphics->type);
+
+switch (def->type) {
+case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET:
+if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
+graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("listen type 'socket' is not available for 
graphics type '%1$s'"),
+   graphicsType);
+return -1;
+}
+break;
+case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE:
+if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
+graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("listen type 'none' is not available for graphics 
type '%1$s'"),
+   

Re: [PATCH v2] Conf: Move validation of virDomainGraphicsListenDef out of Parser

[Michal Prívozník]

On 4/5/23 19:28, K Shiva wrote:
> From: skran 
> 
> In an effort to separate the validation steps from the Parse stage, a
> part of the validation of virDomainGraphicsListenDef has been moved to
> domain_validate.h.
> 
> Signed-off-by: K Shiva 

Now, this is perfect! This message tells you what the commit does
(except, the patch does a bit more than what the commit message
describes, but more on that below).

> ---
> 
> This is a v2 of:
> https://listman.redhat.com/archives/libvir-list/2023-April/239205.html
> 
> diff to v1:
> - Made virDomainGraphicsListenDefValidate() static, called by
>   virDomainGraphicsDefListensValidate()
> - Removed unused Parameter (*graphics) out of Parse Function definition i.e
>   virDomainGraphicsListenParseXML() 
> - Corrected code format
> 

Splendid! Except, this is still a diff to your previous patch. And your
previous patch wasn't merged and thus this patch doesn't apply cleanly.
There's no virDomainStorageNetHostSocketValidate() or
virDomainGraphicsListenDefValidate() in current master branch.

>  src/conf/domain_conf.c |  9 ++---
>  src/conf/domain_validate.c | 68 --
>  src/conf/domain_validate.h |  4 ---
>  3 files changed, 37 insertions(+), 44 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 1e0ac737bb..746bb4efdf 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5836,7 +5836,7 @@ virDomainStorageNetworkParseHost(xmlNodePtr hostnode,
>  
>  host->socket = virXMLPropString(hostnode, "socket");
>  
> -// Socket Validation
> +/* Socket Validation */
>  if (virDomainStorageNetHostSocketValidate(host, transport) < 0)
>  goto cleanup;

This hunk (and some others) causes this patch doesn't apply cleanly.
Please make sure to send a patch that applies on top of master branch
cleanly:

  https://libvirt.org/submitting-patches.html

Also, this might be worth reading:

  https://libvirt.org/best-practices.html

Looking forward to v3.

Michal

Re: [PATCH v2] Conf: Move validation of virDomainGraphicsListenDef out of Parser

[Michal Prívozník]

On 4/5/23 19:56, Shiva wrote:
> Greetings Sir
> 
> My apologies for the incorrect formats as I am a beginner to git. Thank
> you very much for your corrections.

That's okay and GSoC was invented specifically for this reason. To
attract students into Open Source and its development. But at the same
time, review bandwidth is precious and therefore it's important to
listen to feedback. I'm glad to see v2 worked out better.

> 
> I have a few questions:
> 
> Can Questions and doubts be posted within the same mail that consists a
> patch file? Or should they be put in a separate email (as I am
> attempting to do now)?

It's okay to send it as two separate emails even. I mean, usually, when
we work on something and for instance can't make a decision (e.g. on a
design or something), we often just write an e-mail to the list,
explaining what it is we're working on and that we face this decision,
what are the options, etc. And other developers reply with their thoughts.

> 
> As per your instruction to make the functions static, I was able to make
> the below function (created by me) static by calling it from
> virDomainGraphicsDefListensValidate() within domain_validate.c
> 
> static int
> virDomainGraphicsListenDefValidate(const virDomainGraphicsListenDef *def,
>const virDomainGraphicsDef *graphics)

Yep, that's correct.

> 
> Similarly,
> In this patch: Link
> 
> I had created a function named;
> 
>  int virDomainStorageNetHostSocketValidate(const virStorageNetHostDef *host,
>  const char *transport);
> 
> that removes the validation checks of socket in
> virDomainStorageNetworkParseHost().
> While I found a function to call Graphics Validation within validate.c,
> I am unable to find a function that can be used to call
> NetworkHostSocketValidate() from within domain_validate.c. Could you
> please provide a pointer on how it could be done?

Yeah, you may need to write new code. As I've said, we're only gradually
moving code into domain_validate.c. But let me see:

So virDomainStorageNetworkParseHost() is called from exactly two places
(identified by 'git grep -npC10 virDomainStorageNetworkParseHost'):

virDomainBackupDefParseXML
virDomainStorageNetworkParseHosts

Let's focus on the latter for now, we'll get back to the former soon.
virDomainStorageNetworkParseHosts() is then called from the following
places:

virDomainHostdevSubsysSCSIiSCSIDefParseXML
virDomainDiskSourceNetworkParse

after repeating this process more times, we can find what device type
might end up calling this function and what conditions lead to it. For
instance, in this case it's a hostdev that has scsi subdev, iscsi
protocol, etc.

Now, looking at domain_validate.c there's
virDomainDeviceDefValidateInternal() which calls
virDomainHostdevDefValidate() for hostdevs, and then has a block of
checks for scsi subsys, but no further code for iscsi protocol. So that
looks like a place to add the check.

Basically, we're trying to reconstruct the path from domain_conf.c that
lead to the check in domain_validate.c.

And now let's return back to the other caller of the function:
virDomainBackupDefParseXML(). Unfortunately, the XML that's parsed there
is not validated - there's no vir*Validate() function called; so if this
check is removed and moved into domain_validate.c then this path would
be left buggy.

After all, it's probably okay to leave the check there. Introducing
validation to backup XMLs is solvable, but definitely not a bite sized
task for newcomer.

Michal

Re: [PATCH v2 1/3] util: Introduce virAcpi module

[Andrea Bolognani]

On Thu, Apr 06, 2023 at 10:57:02AM +0200, Michal Privoznik wrote:
> +++ b/src/util/viracpi.c
> +/* Basic sanity check. While there's a type specific data
> + * that follows the node header, the node length should be at
> + * least size of header itself. */
> +if (nodeHeader->len < sizeof(*nodeHeader)) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("IORT table node type %1$s has invalid length: 
> %2$u"),
> +   NULLSTR(typeStr), (unsigned int)nodeHeader->len);

As mentioned in the other thread, consider changing this to

  virReportError(VIR_ERR_INTERNAL_ERROR,
 _("IORT table node type %1$s has invalid length: got
%2$u, expected at least %3$lu"),
 NULLSTR(typeStr), (unsigned int)nodeHeader->len,
sizeof(*nodeHeader));

for a slightly more useful error message.

Either way,

  Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH 1/3] util: Introduce virAcpi module

[Andrea Bolognani]

On Thu, Apr 06, 2023 at 12:02:01PM +0200, Michal Prívozník wrote:
> On 4/6/23 11:20, Andrea Bolognani wrote:
> > On Thu, Apr 06, 2023 at 10:20:46AM +0200, Michal Prívozník wrote:
>  +typedef enum {
>  +VIR_IORT_NODE_TYPE_UNKNOWN = -1,
> >>>
> >>> Do we need this? virIORTNodeHeader.type is defined as unsigned.
> >
> > You didn't answer this one :)
>
> That's because I removed it in v2 ;-)

I hadn't realized you had posted that already O:-)

> > What about renaming @reference_id to @mappings_offset though, to
> > match the corresponding fields for nodes in virIORTHeader?
>
> I can do that, but then again - it's named 'Reference to ID Array' in
> the spec. And since I renamed SMMU_V1_2 to match whatever was in the
> spec I figured staying close to the standard is key. Or how do we decide
> when to stay true to the standard an when not?
>
> If anything, it should be named reference_to_id_array. But I believe
> that anybody who will extend this implementation will do the same as I
> did - open the code and the spec next to each other and try to find a
> match between tables in spec and structs in our code. At that point,
> whether this field is named 'reference_id', 'mappings_offset' or
> 'reference_to_id_array' doesn't really matter.

Yeah, it's unfortunate that the standard itself didn't adopt a
reasonable and consistent naming convention. Let's stick to what
they've chosen for now.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH 1/3] util: Introduce virAcpi module

[Michal Prívozník]

On 4/6/23 11:20, Andrea Bolognani wrote:
> On Thu, Apr 06, 2023 at 10:20:46AM +0200, Michal Prívozník wrote:
>> On 4/5/23 19:21, Andrea Bolognani wrote:
>>> On Wed, Apr 05, 2023 at 01:30:17PM +0200, Michal Privoznik wrote:
 +if (nodeHeader->len < sizeof(*nodeHeader)) {
 +virReportError(VIR_ERR_INTERNAL_ERROR,
 +   _("IORT table node type %1$s has invalid length: 
 %2$" PRIu16),
>>>
>>> I'm not sure this plays well with the recently introduced changes to
>>> translatable strings. Have you checked with Jirka?
>>
>> I haven't. But, gcc complains if only a part of string contains
>> positioned arguments. That is:
>>
>>   "argument value %1$s and another argument %s"
>>   "argument value %s and another argument %2$s"
>>
>> are invalid format string and compiler throws an error. Now, using no
>> positions works:
>>
>>   "argument value %s and another argument %s"
>>
>> but then, Jirka's syntax-check rule throws an error. But it doesn't for
>> the case I'm using:
>>
>>   "integer value %" PRIu16
> 
> Yeah, either your usage is fine or the syntax-check rule should be
> improve to catch it. Jirka?
> 
>> Although, one could argue that if the translate tool doesn't allow fixed
>> size integer types modifiers, then the tool is broken. Surely, libvirt's
>> not the only one using fixed size integers. But okay, for error messages
>> I can typecast arguments. IOW:
>>
>>  virReportError(VIR_ERR_INTERNAL_ERROR,
>> _("IORT table node type %1$s has invalid length: %2$u"),
>> NULLSTR(typeStr), (unsigned int)nodeHeader->len);
> 
> This looks fine.
> 
> Perhaps a bit more useful:
> 
>   virReportError(VIR_ERR_INTERNAL_ERROR,
>  _("IORT table node type %1$s has invalid length: got
> %2$u, expected at least %3$lu"),
>  NULLSTR(typeStr), (unsigned int)nodeHeader->len,
> sizeof(*nodeHeader));
> 
 +typedef enum {
 +VIR_IORT_NODE_TYPE_UNKNOWN = -1,
>>>
>>> Do we need this? virIORTNodeHeader.type is defined as unsigned.
> 
> You didn't answer this one :)

That's because I removed it in v2 ;-)

> 
 +typedef struct virIORTNodeHeader virIORTNodeHeader;
 +struct virIORTNodeHeader {
 +uint8_t type; /* One of virIORTNodeType */
 +uint16_t len;
 +uint8_t revision;
 +uint32_t identifier;
 +uint32_t nmappings;
 +uint32_t reference_id;
>>>
>>> Since we only care about type and length, we could simply not declare
>>> the other four fields at all, right?
>>
>> Yes, except these fields are defined by the standard. We may need them
>> in the future. Anyway - these are thrown right away anyway, if it's
>> added memory consumption you're worried about.
>> By the same token, we don't care about other types (root comples, PMCG,
>> ITS group, ...) and yet we have them in the enum.
> 
> It's fine to keep them, I'm not too concerned about the memory usage,
> I was just wondering how much we should push towards minimalism ;)
> 
> What about renaming @reference_id to @mappings_offset though, to
> match the corresponding fields for nodes in virIORTHeader?
> 

I can do that, but then again - it's named 'Reference to ID Array' in
the spec. And since I renamed SMMU_V1_2 to match whatever was in the
spec I figured staying close to the standard is key. Or how do we decide
when to stay true to the standard an when not?

If anything, it should be named reference_to_id_array. But I believe
that anybody who will extend this implementation will do the same as I
did - open the code and the spec next to each other and try to find a
match between tables in spec and structs in our code. At that point,
whether this field is named 'reference_id', 'mappings_offset' or
'reference_to_id_array' doesn't really matter.

Michal

Re: [PATCH 2/3] tests: Introduce viracpitest

[Andrea Bolognani]

On Thu, Apr 06, 2023 at 10:41:01AM +0200, Michal Prívozník wrote:
> On 4/5/23 19:24, Andrea Bolognani wrote:
> > With those typos fixed,
> >
> >   Signed-off-by: Andrea Bolognani 
>
> I believe you meant Reviewed-by, correct? ;-)

Oops! Of course O:-)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH 1/3] util: Introduce virAcpi module

[Andrea Bolognani]

On Thu, Apr 06, 2023 at 10:20:46AM +0200, Michal Prívozník wrote:
> On 4/5/23 19:21, Andrea Bolognani wrote:
> > On Wed, Apr 05, 2023 at 01:30:17PM +0200, Michal Privoznik wrote:
> >> +if (nodeHeader->len < sizeof(*nodeHeader)) {
> >> +virReportError(VIR_ERR_INTERNAL_ERROR,
> >> +   _("IORT table node type %1$s has invalid length: 
> >> %2$" PRIu16),
> >
> > I'm not sure this plays well with the recently introduced changes to
> > translatable strings. Have you checked with Jirka?
>
> I haven't. But, gcc complains if only a part of string contains
> positioned arguments. That is:
>
>   "argument value %1$s and another argument %s"
>   "argument value %s and another argument %2$s"
>
> are invalid format string and compiler throws an error. Now, using no
> positions works:
>
>   "argument value %s and another argument %s"
>
> but then, Jirka's syntax-check rule throws an error. But it doesn't for
> the case I'm using:
>
>   "integer value %" PRIu16

Yeah, either your usage is fine or the syntax-check rule should be
improve to catch it. Jirka?

> Although, one could argue that if the translate tool doesn't allow fixed
> size integer types modifiers, then the tool is broken. Surely, libvirt's
> not the only one using fixed size integers. But okay, for error messages
> I can typecast arguments. IOW:
>
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> _("IORT table node type %1$s has invalid length: %2$u"),
> NULLSTR(typeStr), (unsigned int)nodeHeader->len);

This looks fine.

Perhaps a bit more useful:

  virReportError(VIR_ERR_INTERNAL_ERROR,
 _("IORT table node type %1$s has invalid length: got
%2$u, expected at least %3$lu"),
 NULLSTR(typeStr), (unsigned int)nodeHeader->len,
sizeof(*nodeHeader));

> >> +typedef enum {
> >> +VIR_IORT_NODE_TYPE_UNKNOWN = -1,
> >
> > Do we need this? virIORTNodeHeader.type is defined as unsigned.

You didn't answer this one :)

> >> +typedef struct virIORTNodeHeader virIORTNodeHeader;
> >> +struct virIORTNodeHeader {
> >> +uint8_t type; /* One of virIORTNodeType */
> >> +uint16_t len;
> >> +uint8_t revision;
> >> +uint32_t identifier;
> >> +uint32_t nmappings;
> >> +uint32_t reference_id;
> >
> > Since we only care about type and length, we could simply not declare
> > the other four fields at all, right?
>
> Yes, except these fields are defined by the standard. We may need them
> in the future. Anyway - these are thrown right away anyway, if it's
> added memory consumption you're worried about.
> By the same token, we don't care about other types (root comples, PMCG,
> ITS group, ...) and yet we have them in the enum.

It's fine to keep them, I'm not too concerned about the memory usage,
I was just wondering how much we should push towards minimalism ;)

What about renaming @reference_id to @mappings_offset though, to
match the corresponding fields for nodes in virIORTHeader?

-- 
Andrea Bolognani / Red Hat / Virtualization

[PATCH v2 2/3] tests: Introduce viracpitest

[Michal Privoznik]

Introduce a test that checks newly introduced virAcpi module.
There are three IORT tables from a real HW (IORT_ampere,
IORT_gigabyte and IORT_qualcomm), then there's one from a VM
(IORT_virt_aarch64) and one that I handcrafted to be empty
(IORT_empty).

Signed-off-by: Michal Privoznik 
Reviewed-by: Andrea Bolognani 
---
 build-aux/syntax-check.mk   |   2 +-
 tests/meson.build   |   1 +
 tests/viracpidata/IORT_ampere   | Bin 0 -> 2304 bytes
 tests/viracpidata/IORT_empty| Bin 0 -> 65 bytes
 tests/viracpidata/IORT_gigabyte | Bin 0 -> 10100 bytes
 tests/viracpidata/IORT_qualcomm | Bin 0 -> 3560 bytes
 tests/viracpidata/IORT_virt_aarch64 | Bin 0 -> 128 bytes
 tests/viracpitest.c | 135 
 8 files changed, 137 insertions(+), 1 deletion(-)
 create mode 100644 tests/viracpidata/IORT_ampere
 create mode 100644 tests/viracpidata/IORT_empty
 create mode 100644 tests/viracpidata/IORT_gigabyte
 create mode 100644 tests/viracpidata/IORT_qualcomm
 create mode 100644 tests/viracpidata/IORT_virt_aarch64
 create mode 100644 tests/viracpitest.c

diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index 5829dc9011..64c1e2773e 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -1370,7 +1370,7 @@ exclude_file_name_regexp--sc_prohibit_close = \
   
(\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/vir(file|event)\.c|src/libvirt-stream\.c|tests/(vir.+mock\.c|commandhelper\.c|qemusecuritymock\.c)|tools/nss/libvirt_nss_(leases|macs)\.c)|tools/virt-qemu-qmp-proxy$$)
 
 exclude_file_name_regexp--sc_prohibit_empty_lines_at_EOF = \
-  
(^tests/(nodedevmdevctl|virhostcpu|virpcitest|virstoragetest)data/|docs/js/.*\.js|docs/fonts/.*\.woff|\.diff|tests/virconfdata/no-newline\.conf$$)
+  
(^tests/(nodedevmdevctl|viracpi|virhostcpu|virpcitest|virstoragetest)data/|docs/js/.*\.js|docs/fonts/.*\.woff|\.diff|tests/virconfdata/no-newline\.conf$$)
 
 exclude_file_name_regexp--sc_prohibit_fork_wrappers = \
   
(^(src/(util/(vircommand|virdaemon)|lxc/lxc_controller)|tests/testutils)\.c$$)
diff --git a/tests/meson.build b/tests/meson.build
index 24d08e4f97..35adbc2d56 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -269,6 +269,7 @@ tests += [
   { 'name': 'storagevolxml2xmltest' },
   { 'name': 'sysinfotest' },
   { 'name': 'utiltest' },
+  { 'name': 'viracpitest' },
   { 'name': 'viralloctest' },
   { 'name': 'virauthconfigtest' },
   { 'name': 'virbitmaptest' },
diff --git a/tests/viracpidata/IORT_ampere b/tests/viracpidata/IORT_ampere
new file mode 100644
index 
..02e7fd98499729025346c6ac3ae5cf10b0f1074d
GIT binary patch
literal 2304
zcmbW1y-EW?6opUzV}1)8%LN4mK`kr-7P4TWg&4%rLXlu$Lg$lfAhAGcZwV=;3b8Z6<$$zRpB*-Hx%AfcuV1Jg?AL*&3Um09*JpqZe{?O
zqhNo2j#YGkfFbw)ib1ii+>)bWKJ0S3tjuMt`ZLaZB1OlRM%>(tBecBB}{d#$GR#QJ1$|W
zYctkW#f#$-rnUxTGHSy)RgsHBpSXT>E
n$0ba4UB|lGsLR0_qjZI-uA5j_2Z`enrn>H8U0w7XmoVaAS)q0T

literal 0
HcmV?d1

diff --git a/tests/viracpidata/IORT_empty b/tests/viracpidata/IORT_empty
new file mode 100644
index 
..58b696fe935b432020377aabc76fe2b95376b48f
GIT binary patch
literal 65
ucmebD4+?Q$U|?Y6a`g=eiDdBcbPDqf3SnRbih)8U!4`B$SXqaJT?S#KA

literal 0
HcmV?d1

diff --git a/tests/viracpidata/IORT_gigabyte b/tests/viracpidata/IORT_gigabyte
new file mode 100644
index 
..b18b445b5de0911ba7190dfe396a72fec3fe6cf5
GIT binary patch
literal 10100
zcmeI1OHRWu5Qg2REq(9^AS76TA~tNhV95qiBr266RV`x4f;U?ZP>CaO1rESrH~oNhgq5>JSK2mi4?O{V>;(fI91sY?6o^twN}9A8X^z4OscMJd%Ejt6^<
zo$Nrz$NF$>*F_2;#-b&+H51}AsNpZVy_<6t=eh}%I??@99|OL|*gKAGch71O=h`F3(vuP5gZKakV%Y~+
zD>13!@}VLoSw2~g{cuiHq~RfsV@
z(9PynpX+p8l*6sikW!lF5}Hcb3_`W*WeOMOMwNiSS{Sk4Ei
zh>1=0y-jlzF_BX_$HZUO_?VOiGD+%l0c?$20f;fCe6|Kc$o$AQtESLYAp2^S*N^jIFPpwaF5v64#Xe(lq>
zk4fD?CP{scJdCCOBI}g*4F~e}0qzlBIzdcYgFrXSq~0N27v*r9G^CVfYY7d1;x0Vr
E57fpI9{>OV

literal 0
HcmV?d1

diff --git a/tests/viracpidata/IORT_qualcomm b/tests/viracpidata/IORT_qualcomm
new file mode 100644
index 
..5364a9968fa024f34b5e911513a08e8a7f88cf91
GIT binary patch
literal 3560
zcmb7_yK7WI6vk)w?gfdc+XO5}jbI^i_hDy|WN$P9V~9y%A;c$GhzN?HK?NJFM9@YX
zTmKYG!TbS&7AXbo*Yn+ZoZX!D-Z=xi^Z3p;bLMyV++Eway1jqI7;|ZJw6SjOW_htc
zEDGDArL`;Dm*?|%lFK2~PpYhnO$Ma|k4ZC3tC?blad8
z)4JsI;{czZk6z#9bS9q`KM%?s_r`81d;+7u=bm2>_cy;HAH)c~+o0P9y_nV|
zpWg@g{E2*C5tCid<@HQHZNUe-rSS3FnZW0bUl8{A
zSHEb~7tPV$Qa;ap-tT=s{oFIb2fL;4$=nf

[PATCH v2 3/3] virt-host-validate: Detect SMMU presence on ARMs by parsing IORT table

[Michal Privoznik]

In my previous commit v9.2.0-rc1~3 I've made virt-host-validate
to report host IOMMU check pass if IORT table is present. This is
not sufficient though, because IORT describes much more than just
IOMMU (well, it's called SMMU in ARM world). In fact, this can be
seen in previous commit which adds test cases: there are tables
(IORT_virt_aarch64) which does not contain any SMMU records.

But after previous commits, we can parse the table so switch to
that.

Fixes: 2c13a2a7c9c368ea81eccd4ba12d9cf34bdd331b
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2178885
Signed-off-by: Michal Privoznik 
Reviewed-by: Andrea Bolognani 
---
 tools/virt-host-validate-common.c | 18 +++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/tools/virt-host-validate-common.c 
b/tools/virt-host-validate-common.c
index e96986bb48..c8a23e2e99 100644
--- a/tools/virt-host-validate-common.c
+++ b/tools/virt-host-validate-common.c
@@ -26,6 +26,7 @@
 #include 
 #include 
 
+#include "viracpi.h"
 #include "viralloc.h"
 #include "vircgroup.h"
 #include "virfile.h"
@@ -389,13 +390,24 @@ int virHostValidateIOMMU(const char *hvname,
 }
 virHostMsgPass();
 } else if (ARCH_IS_ARM(arch)) {
-if (access("/sys/firmware/acpi/tables/IORT", F_OK) == 0) {
-virHostMsgPass();
-} else {
+if (access("/sys/firmware/acpi/tables/IORT", F_OK) != 0) {
 virHostMsgFail(level,
"No ACPI IORT table found, IOMMU not "
"supported by this hardware platform");
 return VIR_HOST_VALIDATE_FAILURE(level);
+} else {
+rc = virAcpiHasSMMU();
+if (rc < 0) {
+virHostMsgFail(level,
+   "Failed to parse ACPI IORT table");
+return VIR_HOST_VALIDATE_FAILURE(level);
+} else if (rc == 0) {
+virHostMsgFail(level,
+   "No SMMU found");
+return VIR_HOST_VALIDATE_FAILURE(level);
+} else {
+virHostMsgPass();
+}
 }
 } else {
 virHostMsgFail(level,
-- 
2.39.2

[PATCH v2 1/3] util: Introduce virAcpi module

[Michal Privoznik]

The aim of this new module is to contain code that's parsing ACPI
tables. For now, only parsing of IORT table is implemented (it's
ARM specific table). And since we only need to check whether the
table contains SMMU record, the code is very simplified.
I've followed the specification published here:

  https://developer.arm.com/documentation/den0049/latest/

Signed-off-by: Michal Privoznik 
---
 po/POTFILES  |   1 +
 src/libvirt_private.syms |   7 ++
 src/util/meson.build |   1 +
 src/util/viracpi.c   | 226 +++
 src/util/viracpi.h   |  23 
 src/util/viracpipriv.h   |  58 ++
 6 files changed, 316 insertions(+)
 create mode 100644 src/util/viracpi.c
 create mode 100644 src/util/viracpi.h
 create mode 100644 src/util/viracpipriv.h

diff --git a/po/POTFILES b/po/POTFILES
index fa769a8a95..b122f02818 100644
--- a/po/POTFILES
+++ b/po/POTFILES
@@ -244,6 +244,7 @@ src/storage_file/storage_source.c
 src/storage_file/storage_source_backingstore.c
 src/test/test_driver.c
 src/util/iohelper.c
+src/util/viracpi.c
 src/util/viralloc.c
 src/util/virarptable.c
 src/util/viraudit.c
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index e37373c3c9..1247b67a39 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1829,6 +1829,13 @@ vir_g_strdup_printf;
 vir_g_strdup_vprintf;
 
 
+# util/viracpi.c
+virAcpiHasSMMU;
+virAcpiParseIORT;
+virIORTNodeTypeTypeFromString;
+virIORTNodeTypeTypeToString;
+
+
 # util/viralloc.h
 virAppendElement;
 virDeleteElementsN;
diff --git a/src/util/meson.build b/src/util/meson.build
index c81500ea04..2fe6f7699e 100644
--- a/src/util/meson.build
+++ b/src/util/meson.build
@@ -1,5 +1,6 @@
 util_sources = [
   'glibcompat.c',
+  'viracpi.c',
   'viralloc.c',
   'virarch.c',
   'virarptable.c',
diff --git a/src/util/viracpi.c b/src/util/viracpi.c
new file mode 100644
index 00..05a2b29f0f
--- /dev/null
+++ b/src/util/viracpi.c
@@ -0,0 +1,226 @@
+/*
+ * viracpi.c: ACPI table(s) parser
+ *
+ * Copyright (C) 2023 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library;  If not, see
+ * .
+ */
+
+#include 
+
+#include 
+#include 
+
+#define LIBVIRT_VIRACPIPRIV_H_ALLOW
+#include "internal.h"
+#include "viracpi.h"
+#include "viracpipriv.h"
+#include "viralloc.h"
+#include "virerror.h"
+#include "virfile.h"
+#include "virlog.h"
+
+#define VIR_FROM_THIS VIR_FROM_NONE
+
+VIR_LOG_INIT("util.acpi");
+
+typedef struct virIORTHeader virIORTHeader;
+struct virIORTHeader {
+uint32_t signature;
+uint32_t length;
+uint8_t revision;
+uint8_t checksum;
+char oem_id[6];
+char oem_table_id[8];
+char oem_revision[4];
+char creator_id[4];
+char creator_revision[4];
+/* Technically, the following are not part of header, but
+ * they immediately follow the header and are in the table
+ * exactly once. */
+uint32_t nnodes;
+uint32_t nodes_offset;
+/* Here follows reserved and padding fields. Ain't nobody's
+ * interested in that. */
+} ATTRIBUTE_PACKED;
+
+VIR_ENUM_IMPL(virIORTNodeType,
+  VIR_IORT_NODE_TYPE_LAST,
+  "ITS Group",
+  "Named Component",
+  "Root Complex",
+  "SMMUv1 or SMMUv2",
+  "SMMUv3",
+  "PMCG",
+  "Memory range");
+
+
+static int
+virAcpiParseIORTNodeHeader(int fd,
+   const char *filename,
+   virIORTNodeHeader *nodeHeader)
+{
+g_autofree char *nodeHeaderBuf = NULL;
+const char *typeStr = NULL;
+int nodeHeaderLen;
+
+nodeHeaderLen = virFileReadHeaderFD(fd, sizeof(*nodeHeader), 
);
+if (nodeHeaderLen < 0) {
+virReportSystemError(errno,
+ _("cannot read node header '%1$s'"),
+ filename);
+return -1;
+}
+
+if (nodeHeaderLen != sizeof(*nodeHeader)) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("IORT table node header ended early"));
+return -1;
+}
+
+memcpy(nodeHeader, nodeHeaderBuf, nodeHeaderLen);
+
+typeStr = virIORTNodeTypeTypeToString(nodeHeader->type);
+
+VIR_DEBUG("IORT node header: type = %" PRIu8 " (%s) len = %" PRIu16,
+  nodeHeader->type, NULLSTR(typeStr), nodeHeader->len);
+
+/* Basic sanity 

[PATCH v2 0/3] virt-host-validate: Detect SMMU presence on ARMs by parsing IORT table

[Michal Privoznik]

v2 of:

https://listman.redhat.com/archives/libvir-list/2023-April/239251.html

diff to v1:
- Renamed couple of enum values,
- fixed their string translations,
- Other small nits pointed out by Andrea

Michal Prívozník (3):
  util: Introduce virAcpi module
  tests: Introduce viracpitest
  virt-host-validate: Detect SMMU presence on ARMs by parsing IORT table

 build-aux/syntax-check.mk   |   2 +-
 po/POTFILES |   1 +
 src/libvirt_private.syms|   7 +
 src/util/meson.build|   1 +
 src/util/viracpi.c  | 226 
 src/util/viracpi.h  |  23 +++
 src/util/viracpipriv.h  |  58 +++
 tests/meson.build   |   1 +
 tests/viracpidata/IORT_ampere   | Bin 0 -> 2304 bytes
 tests/viracpidata/IORT_empty| Bin 0 -> 65 bytes
 tests/viracpidata/IORT_gigabyte | Bin 0 -> 10100 bytes
 tests/viracpidata/IORT_qualcomm | Bin 0 -> 3560 bytes
 tests/viracpidata/IORT_virt_aarch64 | Bin 0 -> 128 bytes
 tests/viracpitest.c | 135 +
 tools/virt-host-validate-common.c   |  18 ++-
 15 files changed, 468 insertions(+), 4 deletions(-)
 create mode 100644 src/util/viracpi.c
 create mode 100644 src/util/viracpi.h
 create mode 100644 src/util/viracpipriv.h
 create mode 100644 tests/viracpidata/IORT_ampere
 create mode 100644 tests/viracpidata/IORT_empty
 create mode 100644 tests/viracpidata/IORT_gigabyte
 create mode 100644 tests/viracpidata/IORT_qualcomm
 create mode 100644 tests/viracpidata/IORT_virt_aarch64
 create mode 100644 tests/viracpitest.c

-- 
2.39.2

Re: [PATCH 2/3] tests: Introduce viracpitest

[Michal Prívozník]

On 4/5/23 19:24, Andrea Bolognani wrote:
> On Wed, Apr 05, 2023 at 01:30:18PM +0200, Michal Privoznik wrote:
>> 
>
> With those typos fixed,
> 
>   Signed-off-by: Andrea Bolognani 
> 

I believe you meant Reviewed-by, correct? ;-)

Michal

Re: [PATCH 1/3] util: Introduce virAcpi module

[Michal Prívozník]

On 4/5/23 19:21, Andrea Bolognani wrote:
> On Wed, Apr 05, 2023 at 01:30:17PM +0200, Michal Privoznik wrote:
>> +++ b/src/util/viracpi.c
>> +VIR_ENUM_IMPL(virIORTNodeType,
>> +  VIR_IORT_NODE_TYPE_LAST,
>> +  "ITS Group",
>> +  "Named Component",
>> +  "Root Complex",
>> +  "SMMU v1/v2",
>> +  "SMMU v3",
>> +  "PMCG",
>> +  "Memory range");
> 
> Just a thought: it could be nice to have these match exactly the
> strings found in the specification, i.e.
> 
>   VIR_ENUM_IMPL(virIORTNodeType,
> VIR_IORT_NODE_TYPE_LAST,
> "ITS Group",
> "Named component",
> "Root complex",
> "SMMUv1 or SMMUv2",
> "SMMUv3",
> "PMCG",
> "Memory range");
> 
> But it's fine to leave things as they are.

Fair enough. I've found "SMMU v1/v2" shorter but I guess I don't care
that much.

> 
>> +static int
>> +virAcpiParseIORTNodeHeader(int fd,
>> +   const char *filename,
>> +   virIORTNodeHeader *nodeHeader)
>> +{
>> +g_autofree char *nodeHeaderBuf = NULL;
>> +const char *typeStr = NULL;
>> +int nodeHeaderLen;
>> +
>> +nodeHeaderLen = virFileReadHeaderFD(fd, sizeof(*nodeHeader), 
>> );
>> +if (nodeHeaderLen < 0) {
>> +virReportSystemError(errno,
>> + _("cannot read node header '%1$s'"),
>> + filename);
>> +return -1;
>> +}
>> +
>> +if (nodeHeaderLen != sizeof(*nodeHeader)) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +   _("IORT table node header ended early"));
>> +return -1;
>> +}
>> +
>> +memcpy(nodeHeader, nodeHeaderBuf, nodeHeaderLen);
>> +
>> +typeStr = virIORTNodeTypeTypeToString(nodeHeader->type);
>> +
>> +VIR_DEBUG("IORT node header: type = %" PRIu8 " (%s) len = %" PRIu16,
>> +  nodeHeader->type, typeStr, nodeHeader->len);
> 
> Missing NULLSTR() call around typeStr here.
> 
>> +/* Basic sanity check. While there's a type specific data
>> + * that follows the node header, the node length should be at
>> + * least size of header itself. */
>> +if (nodeHeader->len < sizeof(*nodeHeader)) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>> +   _("IORT table node type %1$s has invalid length: 
>> %2$" PRIu16),
> 
> I'm not sure this plays well with the recently introduced changes to
> translatable strings. Have you checked with Jirka?

I haven't. But, gcc complains if only a part of string contains
positioned arguments. That is:

  "argument value %1$s and another argument %s"
  "argument value %s and another argument %2$s"

are invalid format string and compiler throws an error. Now, using no
positions works:

  "argument value %s and another argument %s"

but then, Jirka's syntax-check rule throws an error. But it doesn't for
the case I'm using:

  "integer value %" PRIu16

Although, one could argue that if the translate tool doesn't allow fixed
size integer types modifiers, then the tool is broken. Surely, libvirt's
not the only one using fixed size integers. But okay, for error messages
I can typecast arguments. IOW:

 virReportError(VIR_ERR_INTERNAL_ERROR,
_("IORT table node type %1$s has invalid length: %2$u"),
NULLSTR(typeStr), (unsigned int)nodeHeader->len);


> 
>> +++ b/src/util/viracpipriv.h
>> +typedef enum {
>> +VIR_IORT_NODE_TYPE_UNKNOWN = -1,
> 
> Do we need this? virIORTNodeHeader.type is defined as unsigned.
> 
>> +VIR_IORT_NODE_TYPE_ITS_GROUP = 0,
>> +VIR_IORT_NODE_TYPE_NAMED_COMPONENT,
>> +VIR_IORT_NODE_TYPE_ROOT_COMPLEX,
>> +VIR_IORT_NODE_TYPE_SMMU_V1_2,
>> +VIR_IORT_NODE_TYPE_SMMU_V3,
>> +VIR_IORT_NODE_TYPE_PMCG,
>> +VIR_IORT_NODE_TYPE_MEMORY_RANGE,
> 
> Same comment as above for the names of these. Again, the current ones
> are fine.
> 
>> +VIR_IORT_NODE_TYPE_LAST,
>> +} virIORTNodeType;
>> +
>> +VIR_ENUM_DECL(virIORTNodeType);
>> +
>> +typedef struct virIORTNodeHeader virIORTNodeHeader;
>> +struct virIORTNodeHeader {
>> +uint8_t type; /* One of virIORTNodeType */
>> +uint16_t len;
>> +uint8_t revision;
>> +uint32_t identifier;
>> +uint32_t nmappings;
>> +uint32_t reference_id;
> 
> Since we only care about type and length, we could simply not declare
> the other four fields at all, right?
> 

Yes, except these fields are defined by the standard. We may need them
in the future. Anyway - these are thrown right away anyway, if it's
added memory consumption you're worried about.
By the same token, we don't care about other types (root comples, PMCG,
ITS group, ...) and yet we have them in the enum.

Michal

Re: [PATCH 3/3] virt-host-validate: Detect SMMU presence on ARMs by parsing IORT table

[Michal Prívozník]

On 4/5/23 19:31, Andrea Bolognani wrote:
> On Wed, Apr 05, 2023 at 01:30:19PM +0200, Michal Privoznik wrote:
>> In my previous commit v9.2.0-rc1~3 I've made virt-host-validate
> 
> You're including the commit hash below in the Fixes: pseudo-header,
> so mentioning it here again feels unnecessary. But it's fine if you
> prefer keeping this.
> 
>> to report host IOMMU check pass if IORT table is present. This is
>> not sufficient though, because IORT describes much more than just
>> IOMMU (well, it's called SMMU in ARM world). In fact, this can be
>> seen in previous commit which adds test cases: there are tables
>> (IORT_virt_aarch64) which does not contain any SMMU records.
>>
>> But after previous commits, we can parse the table so switch to
>> that.
>>
>> Fixes: 2c13a2a7c9c368ea81eccd4ba12d9cf34bdd331b
> 
> Personally I've grown to quite like the alternative take on this
> pseudo-header
> 
>   Fixes: 2c13a2a ("virt-host-validate: Detect SMMU support on ARMs")
> 
> as seen very frequently in QEMU and other projects. I feel that it's
> more immediately informative than just the raw hash. But your version
> is perfectly okay too.

Since you're bringing this up, I find the QEMU style horrible. It boils
down to why we include 'Fixes' in the first place. IMO, the part of
commit message above is for humans, and that's why I tend to put output
of: 'git describe $hash' or 'git describe --contains $hash' there
because that gives me (as a human) a hint which version the regression
occurred in (v9.2.0 in this case). But for 'Fixes:' field - my
understanding is that it's for maintainers so that they know what other
commits to backport. And thus it should be as unique as possible. But
these abbreviated hashes are horrible in that sense.

TLDR: the length of abbreviated hashes changes over time

Longer version:
So, when you do 'git log --oneline' and see those abbreviated hashes,
for both libvirt.git and qemu.git they're both shortened to 10
characters. And it makes sense because each repo has relatively small
number of objects in the pack (~42K for libvirt and ~703K for qemu;
obtained via 'git count-objects -v' and looking at 'in-pack' value) and
10 chars are sufficient to guarantee uniqueness. But if you take a look
at a bigger repo, say kernel.git (~9M objects), 10 characters are no
longer sufficient and git needs to use 12 characters. IOW as the
repository grows, abbreviated hashes are less and less unique.

BTw: the code that picks the length for abbreviated hashes can be found
here:

  https://github.com/git/git/blob/master/object-name.c#L762

We can expect length to bump to 11 soon-ish for qemu. Not to mention,
that these calculations set the starting position, lower limit, after
which git still has to walk through all hashes to make sure there's no
conflict using that lower limit. Therefore, even adding one new commit
can cause conflict on say 10 characters.

And if I look at this from a downstream maintainer POV and my (probably
unique) workflow, then whenever I backport a patch, I also bring up
git-log (in less) and do a quick search of the hash '/$hash'. This finds
all 'Fixes: $hash' lines and thus brings my attention to fixes of the
commit I'm about to backport. Using these abbreviated hashes doesn't
work with my workflow (obviously). Now, sure - I can search for just an
abbreviated hash - but how much abbreviated?

Also, what good is the commit subject? I certainly do not remember
commits by their subjects. And thus I need to look at the commit anyway.
For instance, from your example I'd still need to:

  git show $abbrevHash

to understand what the commit did (okay, not in this example, because
I'm author of both commits and wrote them in span of a a couple of days;
but if I were a downstream maintainer you can bet I'd need to do that).

Therefore, using:

  Fixes: $abbrevHash ($commitSubject)

is a horrible idea that looks good on paper but in fact hurts everybody
involved.

> 
> Please include a reference to
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=2178885
> 
> though, since this is finishing the fix for that bug.

Yeah, there's still a case with device tree (i.e. no ACPI tables) so I
was dubious whether to put it there. But you're right, it can't hurt.

> 
> 
> Regardless of whether you decide to follow any of the suggestions
> above,
> 
>   Reviewed-by: Andrea Bolognani 
> 

Thanks,

Michal

Re: Improve default machine type selection

[Peter Krempa]

On Thu, Apr 06, 2023 at 09:32:28 +0200, Peter Krempa wrote:
> On Wed, Apr 05, 2023 at 15:19:07 -0600, Jim Fehlig wrote:
> > On 3/16/23 11:56, Jim Fehlig wrote:

[...]

> > error: Failed to create domain from test.xml
> > error: internal error: process exited while connecting to monitor:
> > 2023-04-05T20:36:19.564896Z qemu-system-aarch64: Property
> > 'integratorcp-machine.acpi' not found
> 
> This is a known issue which should be already fixed if you use the
> upcoming qemu version.
> 
> Historically ACPI was controled via '-no-acpi' which is issued when you
> don't use the '' feature element in the definition. Also
> historically it was only used on x86. With development of the 'virt'
> machine type for arm which does use acpi this was un-carefully extended
> to aarch64 as well where it didn't work with non-virt machines.
> 
> Now with the latest qemu which reports which machine type actually
> support ACPI we avoid use of '-no-acpi' for those which don't support
> it.
> 
> Unfortunately the workaround of adding  element to definition for
> a machine which doesn't have it doesn't work either, because a further
> validation check forces you to configure uefi if you want acpi.

This was also reported as an issue upstream:

https://gitlab.com/libvirt/libvirt/-/issues/297

Re: Improve default machine type selection

[Peter Krempa]

On Wed, Apr 05, 2023 at 15:19:07 -0600, Jim Fehlig wrote:
> On 3/16/23 11:56, Jim Fehlig wrote:
> > On 3/15/23 08:40, Ján Tomko wrote:
> > > On a Monday in 2023, Jim Fehlig wrote:
> > > > If an explicit machine type is not specified in the VM config,
> > > > the qemu driver will select the first machine type in the list
> > > > of machine types for the specified accelerator. See
> > > > virQEMUCapsGetPreferredMachine
> > > > 
> > > > https://gitlab.com/libvirt/libvirt/-/blob/master/src/qemu/qemu_capabilities.c#L6133
> > > > 
> > > > On my test machines, this works reasonably well for x86_64 where
> > > > the first machine type is pc-i440fx-7.1. But for aarch64, the
> > > > first machine is integratorcp, which is not very useful with
> > > > maxCpus=1 and other limitations.
> > > 
> > > Is it possible to run such machine with libvirt?
> > 
> > I just did a quick check with libvirt 9.1.0 (qemu is a bit older, at 7.1.0):
> > 
> > # cat test.xml
> > 
> >    test
> >    2097152
> >    1
> >    
> >      hvm
> >       > type='pflash'>/usr/share/qemu/aavmf-aarch64-code.bin
> >      
> >      
> >    
> >    
> >    destroy
> >    restart
> >    destroy
> >    
> >      /usr/bin/qemu-system-aarch64
> >      
> >    
> >    
> >    
> >      
> >    
> > 
> > # virsh create test.xml
> > error: Failed to create domain from test.xml
> > error: internal error: Unexpected enum value 0 for 
> > virDomainDeviceAddressType
> > 
> > I don't _think_ it's a downstream bug, nor fixed in git in the meantime.
> > It appears running the old integratorcp machine with libivrt is not
> > possible.
> 
> I trimmed the config to remove things like virtio devices that are not
> supported by the default machine type, but still it does not work with
> libvirt:
> 
> # cat test.xml
> 
>   test
>   2097152
>   1
>   
> hvm
>   
>   
> /usr/bin/qemu-system-aarch64
>   
> 
> # virsh create test.xml
> error: Failed to create domain from test.xml
> error: internal error: process exited while connecting to monitor:
> 2023-04-05T20:36:19.564896Z qemu-system-aarch64: Property
> 'integratorcp-machine.acpi' not found

This is a known issue which should be already fixed if you use the
upcoming qemu version.

Historically ACPI was controled via '-no-acpi' which is issued when you
don't use the '' feature element in the definition. Also
historically it was only used on x86. With development of the 'virt'
machine type for arm which does use acpi this was un-carefully extended
to aarch64 as well where it didn't work with non-virt machines.

Now with the latest qemu which reports which machine type actually
support ACPI we avoid use of '-no-acpi' for those which don't support
it.

Unfortunately the workaround of adding  element to definition for
a machine which doesn't have it doesn't work either, because a further
validation check forces you to configure uefi if you want acpi.