Re: [RFC v2 1/1] qemu: add index for isa-serial device using target.port

[Divya Garg]

On 04/01/22 7:27 pm, Daniel P. Berrangé wrote:

On Tue, Jan 04, 2022 at 11:47:39AM +, John Levon wrote:

On Sat, Dec 11, 2021 at 07:57:47PM -0800, “Divya wrote:


From: Divya Garg 

VM XML accepts target.port but this does not get passed while building the qemu
command line for this VM.

Apologies, I failed to notice this had been sent out to the list; Re-posting
my comments from an internal thread, so all can see:

I don't think serial ports that are not isa-serial should clash with serial
ports like this.

In fact, from what I can see, as "port" is not used for any other type, we
should just completely ignore non-isa-serial ports here.

Agreed, conceptually the 'port' is only required to be unique within
the scope of the 'type' setting.


Sure Daniel. Let me update it and send the patch for the review.

Thanks
Regards
Divya



Regards,
Daniel

Re: [PATCH v3] Add VM info to improve error log message for qemu monitor

[Rohit Kumar]

On 04/01/22 6:27 pm, Ani Sinha wrote:


On Tue, 4 Jan 2022, Rohit Kumar wrote:


This change adds the domain name in the error and debug logs during
monitor IO processing so that we may infer which VM experienced
errors such as IO or socket hangup. This may help in debugging
monitor IO errors.

LGTM. If you wish you can add a comment why the new member domainName in
qemuMonitor stuct was essential.

Sure. I would update it in commit message if required.
I am waiting for Peter's reviews on this patch before proceeding futher.
Thanks.



Signed-off-by: Rohit Kumar 

Reviewed-by: Ani Sinha 

Thanks for the review, Ani.



---
  src/qemu/qemu_monitor.c | 36 +---
  1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index dda6ae9796..2b4582578a 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -80,6 +80,7 @@ struct _qemuMonitor {
  GSource *watch;

  virDomainObj *vm;
+char *domainName;

  qemuMonitorCallbacks *cb;
  void *callbackOpaque;
@@ -243,6 +244,7 @@ qemuMonitorDispose(void *obj)
  virCondDestroy(>notify);
  g_free(mon->buffer);
  g_free(mon->balloonpath);
+g_free(mon->domainName);
  }


@@ -583,8 +585,8 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,

  if (!error && !mon->goteof &&
  cond & G_IO_ERR) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Invalid file descriptor while waiting for 
monitor"));
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Invalid file descriptor while waiting for monitor 
(vm='%s')"), mon->domainName);
  mon->goteof = true;
  }
  }
@@ -609,13 +611,14 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
  virResetLastError();
  } else {
  if (virGetLastErrorCode() == VIR_ERR_OK && !mon->goteof)
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Error while processing monitor IO"));
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Error while processing monitor IO (vm='%s')"), 
mon->domainName);
  virCopyLastError(>lastError);
  virResetLastError();
  }

-VIR_DEBUG("Error on monitor %s", NULLSTR(mon->lastError.message));
+VIR_DEBUG("Error on monitor %s mon=%p vm=%p name=%s",
+  NULLSTR(mon->lastError.message), mon, mon->vm, 
mon->domainName);
  /* If IO process resulted in an error & we have a message,
   * then wakeup that waiter */
  if (mon->msg && !mon->msg->finished) {
@@ -636,7 +639,8 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
  /* Make sure anyone waiting wakes up now */
  virCondSignal(>notify);
  virObjectUnlock(mon);
-VIR_DEBUG("Triggering EOF callback");
+VIR_DEBUG("Triggering EOF callback mon=%p vm=%p name=%s",
+  mon, mon->vm, mon->domainName);
  (eofNotify)(mon, vm, mon->callbackOpaque);
  virObjectUnref(mon);
  } else if (error) {
@@ -646,7 +650,8 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
  /* Make sure anyone waiting wakes up now */
  virCondSignal(>notify);
  virObjectUnlock(mon);
-VIR_DEBUG("Triggering error callback");
+VIR_DEBUG("Triggering error callback mon=%p vm=%p name=%s",
+  mon, mon->vm, mon->domainName);
  (errorNotify)(mon, vm, mon->callbackOpaque);
  virObjectUnref(mon);
  } else {
@@ -694,6 +699,7 @@ qemuMonitorOpenInternal(virDomainObj *vm,
  mon->fd = fd;
  mon->context = g_main_context_ref(context);
  mon->vm = virObjectRef(vm);
+mon->domainName = g_strdup(NULLSTR(vm->def->name));
  mon->waitGreeting = true;
  mon->cb = cb;
  mon->callbackOpaque = opaque;
@@ -935,14 +941,14 @@ qemuMonitorSend(qemuMonitor *mon,

  /* Check whether qemu quit unexpectedly */
  if (mon->lastError.code != VIR_ERR_OK) {
-VIR_DEBUG("Attempt to send command while error is set %s",
-  NULLSTR(mon->lastError.message));
+VIR_DEBUG("Attempt to send command while error is set %s mon=%p vm=%p 
name=%s",
+  NULLSTR(mon->lastError.message), mon, mon->vm, 
mon->domainName);
  virSetError(>lastError);
  return -1;
  }
  if (mon->goteof) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("End of file from qemu monitor"));
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("End of file from qemu monitor (vm='%s')"), 
mon->domainName);
  return -1;
  }

@@ -955,15 +961,15 @@ qemuMonitorSend(qemuMonitor *mon,

  while (!mon->msg->finished) {
  if (virCondWait(>notify, >parent.lock) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-

[libvirt PATCH 15/17] docs: Add support page for libvirt on macOS

[Andrea Bolognani]

From: Roman Bolshakov 

Signed-off-by: Roman Bolshakov 
Signed-off-by: Andrea Bolognani 
---
 docs/docs.html.in  |   3 +
 docs/index.html.in |   3 +-
 docs/macos.html.in | 229 +
 3 files changed, 234 insertions(+), 1 deletion(-)
 create mode 100644 docs/macos.html.in

diff --git a/docs/docs.html.in b/docs/docs.html.in
index 8132090762..225827b693 100644
--- a/docs/docs.html.in
+++ b/docs/docs.html.in
@@ -16,6 +16,9 @@
 Windows
 Downloads for Windows
 
+macOS
+Working with libvirt on macOS
+
 Migration
 Migrating guests between machines
 
diff --git a/docs/index.html.in b/docs/index.html.in
index 2c4aa7c6d0..3c065badb7 100644
--- a/docs/index.html.in
+++ b/docs/index.html.in
@@ -28,7 +28,8 @@
   LXC,
   BHyve and
   more
-targets Linux, FreeBSD, Windows and 
macOS
+targets Linux, FreeBSD, Windows and
+  macOS
 is used by many applications
   
   Recent / forthcoming release changes
diff --git a/docs/macos.html.in b/docs/macos.html.in
new file mode 100644
index 00..54c93ea2fb
--- /dev/null
+++ b/docs/macos.html.in
@@ -0,0 +1,229 @@
+
+
+http://www.w3.org/1999/xhtml;>
+  
+macOS support
+
+
+
+
+  Libvirt works both as client and server (for 
+  "qemu" domain) on macOS High Sierra (10.13) and macOS Mojave (10.14)
+  since 4.7.0. Other macOS variants likely work but we neither tested nor
+  received reports for them.
+
+
+
+  "hvf" domain type adds support of https://developer.apple.com/documentation/hypervisor;>
+  Hypervisor.framework since 4.10.0. To use "hvf" domain, QEMU must
+  be at least 2.12 and macOS must be no less than Yosemite (10.10). "hvf"
+  domain type is similar to "kvm" but it has less features.
+
+
+
+  Hypervisor.framework is available on your machine if the sysctl command
+  returns 1:
+
+  sysctl -n kern.hv_support
+
+
+Installation
+
+
+  libvirt client (virsh), server (libvirtd) and development headers can be
+  installed from https://brew.sh;>homebrew:
+
+  brew install libvirt
+
+  http://virt-manager.org;>virt-manager and virt-viewer can be
+  installed from source via https://github.com/jeffreywildman/homebrew-virt-manager;>
+  Jeffrey Wildman's tap:
+
+  brew tap jeffreywildman/homebrew-virt-manager
+brew install virt-manager virt-viewer
+
+
+Running libvirtd locally
+
+
+  The server can be started manually:
+  libvirtd
+  or on system boot:
+  brew services start libvirt
+
+
+  Once started, you can use virsh to work with libvirtd:
+  virsh define domain.xml
+virsh start domain
+virsh shutdown domain
+
+  For more details on virsh, please see virsh
+  command reference or built-in help:
+  virsh help
+
+
+
+  Domain XML examples can be found on QEMU
+  driver page. Full reference is available on domain XML format page.
+
+
+
+  You can use virt-manager to connect to libvirtd (connection URI must be
+  specified on the first connection, then it'll be possible to omit it):
+  virt-manager -c qemu:///session
+  or, if you only need an access to the virtual display of a VM you can use
+  virt-viewer:
+  virt-viewer -c qemu:///session
+
+
+Working with external hypervisors
+
+  Details on the example domain XML files, capabilities and connection
+  string syntax used for connecting to external hypervisors can be found
+  online on hypervisor specific driver
+  pages.
+
+
+TLS Certificates
+
+
+  TLS certificates must be placed in the correct locations, before you will
+  be able to connect to QEMU servers over TLS.
+
+
+
+  Information on generating TLS certificates can be found here:
+
+
+http://wiki.libvirt.org/page/TLSSetup;>http://wiki.libvirt.org/page/TLSSetup
+
+
+  The Certificate Authority (CA) certificate file must be placed in:
+
+
+
+  ~/.cache/libvirt/pki/CA/cacert.pem
+
+
+
+  The Client certificate file must be placed in:
+
+
+
+  ~/.cache/libvirt/pki/libvirt/clientcert.pem
+
+
+
+  The Client key file must be placed in:
+
+
+
+  ~/.cache/libvirt/pki/libvirt/private/clientkey.pem
+
+
+Known issues
+
+  This is a list of issues that can be easily fixed and provide
+  substantial improvement of user experience:
+
+
+  
+virt-install doesn't work unless disks are created upfront. The reason
+is because VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA sets
+preallocate=falloc which is not supported by qemu-img on macOS.
+  
+  
+"hvf" is not default domain type when virt-install connects to the
+local libvirtd on macOS
+  
+  
+QXL VGA device and SPICE display cannot be used 

[libvirt PATCH 08/17] qemu: Make error message accel-agnostic

[Andrea Bolognani]

From: Roman Bolshakov 

With more acceleration types, KVM should be used only in error messages
related to KVM.

Signed-off-by: Roman Bolshakov 
Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_capabilities.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index eac1e65a39..893af0b635 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -5854,8 +5854,8 @@ virQEMUCapsCacheLookupDefault(virFileCache *cache,
 
 if (virQEMUCapsTypeIsAccelerated(virttype) && capsType == 
VIR_DOMAIN_VIRT_QEMU) {
 virReportError(VIR_ERR_INVALID_ARG,
-   _("KVM is not supported by '%s' on this host"),
-   binary);
+   _("the accel '%s' is not supported by '%s' on this 
host"),
+   virQEMUCapsAccelStr(virttype), binary);
 return NULL;
 }
 
-- 
2.31.1

[libvirt PATCH 17/17] fixup! NEWS: Mention Apple Silicon support for HVF

[Andrea Bolognani]

Signed-off-by: Andrea Bolognani 
---
 NEWS.rst | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/NEWS.rst b/NEWS.rst
index 29a39da9dd..2e1b576979 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -53,7 +53,8 @@ v8.0.0 (unreleased)
 since 2.12.
 
 It's supported on machines with Intel VT-x feature set that includes
-Extended Page Tables (EPT) and Unrestricted Mode since macOS 10.10.
+Extended Page Tables (EPT) and Unrestricted Mode, as well as recent
+machines powered by Apple Silicon, since macOS 10.10.
 
 * **Improvements**
 
-- 
2.31.1

[libvirt PATCH 16/17] news: Mention hvf domain type

[Andrea Bolognani]

From: Roman Bolshakov 

Signed-off-by: Roman Bolshakov 
Signed-off-by: Andrea Bolognani 
---
 NEWS.rst | 8 
 1 file changed, 8 insertions(+)

diff --git a/NEWS.rst b/NEWS.rst
index 313b4f52b6..29a39da9dd 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -47,6 +47,14 @@ v8.0.0 (unreleased)
 Libvirt is now able to set the size of translation block cache size
 (tb-size) for TCG domains.
 
+  * qemu: Add hvf domain type for Hypervisor.framework
+
+QEMU introduced experimental support of Hypervisor.framework
+since 2.12.
+
+It's supported on machines with Intel VT-x feature set that includes
+Extended Page Tables (EPT) and Unrestricted Mode since macOS 10.10.
+
 * **Improvements**
 
   * libxl: Implement the virDomainGetMessages API
-- 
2.31.1

[libvirt PATCH 09/17] qemu: Correct CPU capabilities probing for hvf

[Andrea Bolognani]

From: Roman Bolshakov 

With this change virsh domcapabilites shows:
  

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

Signed-off-by: Roman Bolshakov 
Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_capabilities.c | 25 ++---
 src/qemu/qemu_process.c  |  2 +-
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 893af0b635..c2ae87d747 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -741,6 +741,7 @@ struct _virQEMUCaps {
 
 /* Capabilities which may differ depending on the accelerator. */
 virQEMUCapsAccel kvm;
+virQEMUCapsAccel hvf;
 virQEMUCapsAccel tcg;
 };
 
@@ -791,13 +792,15 @@ const char *virQEMUCapsArchToString(virArch arch)
 static bool
 virQEMUCapsTypeIsAccelerated(virDomainVirtType type)
 {
-return type == VIR_DOMAIN_VIRT_KVM;
+return type == VIR_DOMAIN_VIRT_KVM ||
+   type == VIR_DOMAIN_VIRT_HVF;
 }
 
 static bool
 virQEMUCapsHaveAccel(virQEMUCaps *qemuCaps)
 {
-return virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM);
+return virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) ||
+   virQEMUCapsGet(qemuCaps, QEMU_CAPS_HVF);
 }
 
 static const char *
@@ -805,6 +808,8 @@ virQEMUCapsAccelStr(virDomainVirtType type)
 {
 if (type == VIR_DOMAIN_VIRT_KVM) {
 return "kvm";
+} else if (type == VIR_DOMAIN_VIRT_HVF) {
+return "hvf";
 } else {
 return "tcg";
 }
@@ -862,6 +867,8 @@ virQEMUCapsGetAccel(virQEMUCaps *qemuCaps,
 {
 if (type == VIR_DOMAIN_VIRT_KVM)
 return >kvm;
+else if (type == VIR_DOMAIN_VIRT_HVF)
+return >hvf;
 
 return >tcg;
 }
@@ -992,6 +999,8 @@ virQEMUCapsGetMachineTypesCaps(virQEMUCaps *qemuCaps,
  * take the set of machine types we probed first. */
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
 accel = >kvm;
+else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_HVF))
+accel = >hvf;
 else
 accel = >tcg;
 
@@ -2009,6 +2018,7 @@ virQEMUCaps *virQEMUCapsNewCopy(virQEMUCaps *qemuCaps)
 ret->cpuData = virCPUDataNewCopy(qemuCaps->cpuData);
 
 if (virQEMUCapsAccelCopy(>kvm, >kvm) < 0 ||
+virQEMUCapsAccelCopy(>hvf, >hvf) < 0 ||
 virQEMUCapsAccelCopy(>tcg, >tcg) < 0)
 return NULL;
 
@@ -2062,6 +2072,7 @@ void virQEMUCapsDispose(void *obj)
 virSEVCapabilitiesFree(qemuCaps->sevCapabilities);
 
 virQEMUCapsAccelClear(>kvm);
+virQEMUCapsAccelClear(>hvf);
 virQEMUCapsAccelClear(>tcg);
 }
 
@@ -2313,6 +2324,10 @@ virQEMUCapsIsVirtTypeSupported(virQEMUCaps *qemuCaps,
 virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG))
 return true;
 
+if (virtType == VIR_DOMAIN_VIRT_HVF &&
+virQEMUCapsGet(qemuCaps, QEMU_CAPS_HVF))
+return true;
+
 if (virtType == VIR_DOMAIN_VIRT_KVM &&
 virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
 return true;
@@ -2790,7 +2805,9 @@ bool
 virQEMUCapsHasMachines(virQEMUCaps *qemuCaps)
 {
 
-return !!qemuCaps->kvm.nmachineTypes || !!qemuCaps->tcg.nmachineTypes;
+return !!qemuCaps->kvm.nmachineTypes ||
+   !!qemuCaps->hvf.nmachineTypes ||
+   !!qemuCaps->tcg.nmachineTypes;
 }
 
 
@@ -4718,6 +4735,7 @@ virQEMUCapsFormatCache(virQEMUCaps *qemuCaps)
   virArchToString(qemuCaps->arch));
 
 virQEMUCapsFormatAccel(qemuCaps, , VIR_DOMAIN_VIRT_KVM);
+virQEMUCapsFormatAccel(qemuCaps, , VIR_DOMAIN_VIRT_HVF);
 virQEMUCapsFormatAccel(qemuCaps, , VIR_DOMAIN_VIRT_QEMU);
 
 for (i = 0; i < qemuCaps->ngicCapabilities; i++) {
@@ -5577,6 +5595,7 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch,
 qemuCaps->libvirtVersion = LIBVIR_VERSION_NUMBER;
 
 virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_KVM);
+virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_HVF);
 virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_QEMU);
 
 if (virQEMUCapsHaveAccel(qemuCaps)) {
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index de1146251d..c80c9bae2d 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -9288,7 +9288,7 @@ qemuProcessQMPLaunch(qemuProcessQMP *proc)
 if (proc->forceTCG)
 machine = "none,accel=tcg";
 else
-machine = "none,accel=kvm:tcg";
+machine = "none,accel=kvm:hvf:tcg";
 
 VIR_DEBUG("Try to probe capabilities of '%s' via QMP, machine %s",
   proc->binary, machine);
-- 
2.31.1

[libvirt PATCH 14/17] docs: Note hvf support for domain elements

[Andrea Bolognani]

From: Roman Bolshakov 

Many domain elements have "QEMU and KVM only" or "QEMU/KVM since x.y.z"
remarks. Most of the elements work for HVF domain, so it makes sense to
add respective notices for HVF domain.

All the elements have been manually tested.

Signed-off-by: Roman Bolshakov 
Signed-off-by: Andrea Bolognani 
---
 docs/formatdomain.rst | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 3e9de05249..e27fb23119 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -1426,7 +1426,8 @@ In case no restrictions need to be put on CPU model and 
its features, a simpler
   :since:`Since 7.1.0` with the QEMU driver.
 
Both ``host-model`` and ``host-passthrough`` modes make sense when a domain
-   can run directly on the host CPUs (for example, domains with type ``kvm``).
+   can run directly on the host CPUs (for example, domains with type ``kvm``
+   or ``hvf``).
The actual host CPU is irrelevant for domains with emulated virtual CPUs
(such as domains with type ``qemu``). However, for backward compatibility
``host-model`` may be implemented even for domains running on emulated CPUs
@@ -1750,7 +1751,7 @@ Each of these states allow for the same four possible 
actions.
The domain will be terminated and then restarted with a new name. (Only
supported by the libxl hypervisor driver.)
 
-QEMU/KVM supports the ``on_poweroff`` and ``on_reboot`` events handling the
+QEMU/KVM/HVF supports the ``on_poweroff`` and ``on_reboot`` events handling the
 ``destroy`` and ``restart`` actions, but the combination of ``on_poweroff`` set
 to ``restart`` and ``on_reboot`` set to ``destroy`` is forbidden.
 
@@ -1885,8 +1886,8 @@ are:
Physical address extension mode allows 32-bit guests to address more than 4
GB of memory.
 ``acpi``
-   ACPI is useful for power management, for example, with KVM guests it is
-   required for graceful shutdown to work.
+   ACPI is useful for power management, for example, with KVM or HVF guests it
+   is required for graceful shutdown to work.
 ``apic``
APIC allows the use of programmable IRQ management. :since:`Since 0.10.2
(QEMU only)` there is an optional attribute ``eoi`` with values ``on`` and
@@ -6195,14 +6196,16 @@ A video device.
 
You can provide the amount of video memory in kibibytes (blocks of 1024
bytes) using ``vram``. This is supported only for guest type of "vz", 
"qemu",
-   "vbox", "vmx" and "xen". If no value is provided the default is used. If the
+   "kvm", "hvf", "vbox", "vmx" and "xen".
+   If no value is provided the default is used. If the
size is not a power of two it will be rounded to closest one.
 
The number of screen can be set using ``heads``. This is supported only for
-   guests type of "vz", "kvm", "vbox" and "vmx".
+   guests type of "vz", "kvm", "hvf", "vbox" and "vmx".
 
-   For guest type of "kvm" or "qemu" and model type "qxl" there are optional
-   attributes. Attribute ``ram`` ( :since:`since 1.0.2` ) specifies the size of
+   For guest type of "kvm", "hvf" or "qemu" and model type "qxl" there are
+   optional attributes.
+   Attribute ``ram`` ( :since:`since 1.0.2` ) specifies the size of
the primary bar, while the attribute ``vram`` specifies the secondary bar
size. If ``ram`` or ``vram`` are not supplied a default value is used. The
``ram`` should also be rounded to power of two as ``vram``. There is also
-- 
2.31.1

[libvirt PATCH 13/17] docs: Add hvf on QEMU driver page

[Andrea Bolognani]

From: Roman Bolshakov 

It's worth to make the domain type a little bit more visible than a row
in news. An example of hvf domain is available on QEMU driver page.

While at it, mention Hypervisor.framework on index page.

Signed-off-by: Roman Bolshakov 
Signed-off-by: Andrea Bolognani 
---
 docs/drvqemu.rst   | 48 +++---
 docs/index.html.in |  1 +
 2 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/docs/drvqemu.rst b/docs/drvqemu.rst
index e18075d865..2611194fe6 100644
--- a/docs/drvqemu.rst
+++ b/docs/drvqemu.rst
@@ -1,13 +1,18 @@
 .. role:: since
 .. role:: removed
 
-==
-KVM/QEMU hypervisor driver
-==
+==
+QEMU/KVM/HVF hypervisor driver
+==
 
 The libvirt KVM/QEMU driver can manage any QEMU emulator from version 2.11.0 or
 later.
 
+It supports multiple QEMU accelerators: software
+emulation also known as TCG, hardware-assisted virtualization on Linux
+with KVM and hardware-assisted virtualization on macOS with
+Hypervisor.framework (:since:`since 8.0.0`).
+
 .. contents::
 
 Project Links
@@ -15,6 +20,7 @@ Project Links
 
 -  The `KVM `__ Linux hypervisor
 -  The `QEMU `__ emulator
+-  
`Hypervisor.framework`__` 
reference
 
 Deployment pre-requisites
 -
@@ -27,6 +33,9 @@ Deployment pre-requisites
 -  **KVM hypervisor**: The driver will probe ``/usr/bin`` for the presence of
``qemu-kvm`` and ``/dev/kvm`` device node. If both are found, then KVM fully
virtualized, hardware accelerated guests will be available.
+-  **Hypervisor.framework (HVF)**: The driver will probe ``sysctl`` for the
+   presence of ``Hypervisor.framework``. If it is found and QEMU is newer than
+   2.12, then it will be possible to create hardware accelerated guests.
 
 Connections to QEMU driver
 --
@@ -634,3 +643,36 @@ KVM hardware accelerated guest on i686

  

+
+HVF hardware accelerated guest on x86_64
+
+
+::
+
+   
+ hvf-demo
+ 4dea24b3-1d52-d8f3-2516-782e98a23fa0
+ 131072
+ 1
+ 
+   hvm
+ 
+ 
+   
+ 
+ 
+ 
+   /usr/local/bin/qemu-system-x86_64
+   
+   
+ 
+ 
+ 
+   
+   
+ 
+ 
+   
+   
+ 
+   
diff --git a/docs/index.html.in b/docs/index.html.in
index bf164edb58..2c4aa7c6d0 100644
--- a/docs/index.html.in
+++ b/docs/index.html.in
@@ -21,6 +21,7 @@
 is accessible from C, Python, Perl, Go and more
 is licensed under open source licenses
 supports KVM,
+  Hypervisor.framework,
   QEMU, Xen,
   Virtuozzo,
   VMWare ESX,
-- 
2.31.1

[libvirt PATCH 12/17] tests: Add HVF test cases

[Andrea Bolognani]

We need to use a hardcoded list of capabilities because we don't
yet have proper replies files obtained from QEMU running on actual
macOS machines.

Signed-off-by: Andrea Bolognani 
---
 .../hvf-aarch64-virt-headless.args| 48 +
 .../hvf-aarch64-virt-headless.xml | 45 +
 .../hvf-x86_64-q35-headless.args  | 47 +
 .../hvf-x86_64-q35-headless.xml   | 44 +
 tests/qemuxml2argvtest.c  | 18 
 .../hvf-aarch64-virt-headless.xml | 94 ++
 .../hvf-x86_64-q35-headless.xml   | 97 +++
 tests/qemuxml2xmltest.c   | 18 
 8 files changed, 411 insertions(+)
 create mode 100644 tests/qemuxml2argvdata/hvf-aarch64-virt-headless.args
 create mode 100644 tests/qemuxml2argvdata/hvf-aarch64-virt-headless.xml
 create mode 100644 tests/qemuxml2argvdata/hvf-x86_64-q35-headless.args
 create mode 100644 tests/qemuxml2argvdata/hvf-x86_64-q35-headless.xml
 create mode 100644 tests/qemuxml2xmloutdata/hvf-aarch64-virt-headless.xml
 create mode 100644 tests/qemuxml2xmloutdata/hvf-x86_64-q35-headless.xml

diff --git a/tests/qemuxml2argvdata/hvf-aarch64-virt-headless.args 
b/tests/qemuxml2argvdata/hvf-aarch64-virt-headless.args
new file mode 100644
index 00..0f1eed66c2
--- /dev/null
+++ b/tests/qemuxml2argvdata/hvf-aarch64-virt-headless.args
@@ -0,0 +1,48 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-test \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-test/.local/share \
+XDG_CACHE_HOME=/tmp/lib/domain--1-test/.cache \
+XDG_CONFIG_HOME=/tmp/lib/domain--1-test/.config \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-aarch64 \
+-name guest=test,debug-threads=on \
+-S \
+-object 
secret,id=masterKey0,format=raw,file=/tmp/lib/domain--1-test/master-key.aes \
+-machine virt,usb=off,dump-guest-core=off,gic-version=2 \
+-accel hvf \
+-drive 
file=/usr/share/edk2/aarch64/QEMU_EFI-silent-pflash.raw,if=pflash,format=raw,unit=0,readonly=on
 \
+-drive 
file=/var/lib/libvirt/qemu/nvram/test_VARS.fd,if=pflash,format=raw,unit=1 \
+-m 4096 \
+-realtime mlock=off \
+-smp 2,sockets=2,cores=1,threads=1 \
+-uuid 1b826c23-8767-47ad-a6b5-c83a88277f71 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev 
socket,id=charmonitor,path=/tmp/lib/domain--1-test/monitor.sock,server=on,wait=off
 \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc,driftfix=slew \
+-no-shutdown \
+-boot strict=on \
+-device 
pcie-root-port,port=8,chassis=1,id=pci.1,bus=pcie.0,multifunction=on,addr=0x1 \
+-device pcie-root-port,port=9,chassis=2,id=pci.2,bus=pcie.0,addr=0x1.0x1 \
+-device pcie-root-port,port=10,chassis=3,id=pci.3,bus=pcie.0,addr=0x1.0x2 \
+-device pcie-root-port,port=11,chassis=4,id=pci.4,bus=pcie.0,addr=0x1.0x3 \
+-device pcie-root-port,port=12,chassis=5,id=pci.5,bus=pcie.0,addr=0x1.0x4 \
+-device pcie-root-port,port=13,chassis=6,id=pci.6,bus=pcie.0,addr=0x1.0x5 \
+-device virtio-serial-pci,id=virtio-serial0,bus=pci.2,addr=0x0 \
+-drive 
file=/var/lib/libvirt/images/test.qcow2,format=qcow2,if=none,id=drive-virtio-disk0
 \
+-device 
virtio-blk-pci,bus=pci.3,addr=0x0,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1
 \
+-netdev user,id=hostnet0 \
+-device 
virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:9a:e6:c6,bus=pci.1,addr=0x0 
\
+-chardev pty,id=charserial0 \
+-serial chardev:charserial0 \
+-chardev 
socket,id=charchannel0,path=/tmp/channel/domain--1-test/org.qemu.guest_agent.0,server=on,wait=off
 \
+-device 
virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=org.qemu.guest_agent.0
 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.4,addr=0x0 \
+-object rng-random,id=objrng0,filename=/dev/urandom \
+-device virtio-rng-pci,rng=objrng0,id=rng0,bus=pci.5,addr=0x0 \
+-msg timestamp=on
diff --git a/tests/qemuxml2argvdata/hvf-aarch64-virt-headless.xml 
b/tests/qemuxml2argvdata/hvf-aarch64-virt-headless.xml
new file mode 100644
index 00..ef13820e17
--- /dev/null
+++ b/tests/qemuxml2argvdata/hvf-aarch64-virt-headless.xml
@@ -0,0 +1,45 @@
+
+  test
+  1b826c23-8767-47ad-a6b5-c83a88277f71
+  4194304
+  4194304
+  2
+  
+hvm
+/usr/share/edk2/aarch64/QEMU_EFI-silent-pflash.raw
+/var/lib/libvirt/qemu/nvram/test_VARS.fd
+
+  
+  
+
+  
+  
+
+
+
+  
+  destroy
+  restart
+  restart
+  
+/usr/bin/qemu-system-aarch64
+
+  
+  
+  
+
+
+
+  
+  
+
+
+
+  
+
+
+
+  /dev/urandom
+
+  
+
diff --git a/tests/qemuxml2argvdata/hvf-x86_64-q35-headless.args 
b/tests/qemuxml2argvdata/hvf-x86_64-q35-headless.args
new file mode 100644
index 00..b3358e3d59
--- /dev/null
+++ b/tests/qemuxml2argvdata/hvf-x86_64-q35-headless.args
@@ -0,0 +1,47 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-test \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-test/.local/share \

[libvirt PATCH 11/17] tests: Add HVF support to testutilsqemu

[Andrea Bolognani]

Signed-off-by: Andrea Bolognani 
---
 tests/testutilsqemu.c | 40 
 1 file changed, 40 insertions(+)

diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
index 7fdb82daec..a75995c77a 100644
--- a/tests/testutilsqemu.c
+++ b/tests/testutilsqemu.c
@@ -106,6 +106,18 @@ static const char *const *kvm_machines[VIR_ARCH_LAST] = {
 [VIR_ARCH_S390X] = s390x_machines,
 };
 
+static const char *const *hvf_machines[VIR_ARCH_LAST] = {
+[VIR_ARCH_I686] = NULL,
+[VIR_ARCH_X86_64] = x86_64_machines,
+[VIR_ARCH_AARCH64] = aarch64_machines,
+[VIR_ARCH_ARMV7L] = NULL,
+[VIR_ARCH_PPC64] = NULL,
+[VIR_ARCH_PPC] = NULL,
+[VIR_ARCH_RISCV32] = NULL,
+[VIR_ARCH_RISCV64] = NULL,
+[VIR_ARCH_S390X] = NULL,
+};
+
 static const char *qemu_default_ram_id[VIR_ARCH_LAST] = {
 [VIR_ARCH_I686] = "pc.ram",
 [VIR_ARCH_X86_64] = "pc.ram",
@@ -214,6 +226,18 @@ testQemuAddGuest(virCaps *caps,
   NULL, nmachines, machines);
 }
 
+if (hvf_machines[emu_arch] != NULL) {
+nmachines = g_strv_length((char **)hvf_machines[emu_arch]);
+machines = virCapabilitiesAllocMachines(hvf_machines[emu_arch],
+nmachines);
+if (machines == NULL)
+goto error;
+
+virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_HVF,
+  qemu_emulators[emu_arch],
+  NULL, nmachines, machines);
+}
+
 return 0;
 
  error:
@@ -403,6 +427,22 @@ int qemuTestCapsCacheInsert(virFileCache *cache,
 virQEMUCapsSet(tmpCaps, QEMU_CAPS_KVM);
 }
 }
+if (hvf_machines[i] != NULL) {
+for (j = 0; hvf_machines[i][j] != NULL; j++) {
+virQEMUCapsAddMachine(tmpCaps,
+  VIR_DOMAIN_VIRT_HVF,
+  hvf_machines[i][j],
+  NULL,
+  NULL,
+  0,
+  false,
+  false,
+  true,
+  defaultRAMid,
+  false);
+virQEMUCapsSet(tmpCaps, QEMU_CAPS_HVF);
+}
+}
 }
 
 if (virFileCacheInsertData(cache, qemu_emulators[i], tmpCaps) < 0) {
-- 
2.31.1

[libvirt PATCH 10/17] fixup! qemu: Fix HVF architecture check

[Andrea Bolognani]

Apple Silicon (aarch64) has HVF support, but there is no
32-bit Intel hardware that is HVF-capable.

Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_capabilities.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index c2ae87d747..da66cdbb22 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -3248,7 +3248,8 @@ virQEMUCapsProbeHVF(virQEMUCaps *qemuCaps)
 hv_support = 0;
 
 if (qemuCaps->version >= 2012000 &&
-ARCH_IS_X86(qemuCaps->arch) &&
+(qemuCaps->arch == VIR_ARCH_X86_64 ||
+ qemuCaps->arch == VIR_ARCH_AARCH64) &&
 hv_support) {
 virQEMUCapsSet(qemuCaps, QEMU_CAPS_HVF);
 }
-- 
2.31.1

[libvirt PATCH 06/17] qemu: Introduce virQEMUCapsHaveAccel

[Andrea Bolognani]

From: Roman Bolshakov 

The function should be used to check if qemu capabilities include a
hardware acceleration, i.e. accel is not TCG.

Signed-off-by: Roman Bolshakov 
Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_capabilities.c | 16 +---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index e2ce2a5d07..b504963ddf 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -794,6 +794,12 @@ virQEMUCapsTypeIsAccelerated(virDomainVirtType type)
 return type == VIR_DOMAIN_VIRT_KVM;
 }
 
+static bool
+virQEMUCapsHaveAccel(virQEMUCaps *qemuCaps)
+{
+return virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM);
+}
+
 /* Checks whether a domain with @guest arch can run natively on @host.
  */
 bool
@@ -4999,7 +5005,7 @@ virQEMUCapsIsValid(void *data,
 return false;
 }
 
-if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
+if (virQEMUCapsHaveAccel(qemuCaps)) {
 if (STRNEQ_NULLABLE(priv->hostCPUSignature, 
qemuCaps->hostCPUSignature)) {
 VIR_DEBUG("Outdated capabilities for '%s': host CPU changed "
   "('%s' vs '%s')",
@@ -5033,7 +5039,9 @@ virQEMUCapsIsValid(void *data,
   qemuCaps->binary);
 return false;
 }
+}
 
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
 kvmSupportsNesting = virQEMUCapsKVMSupportsNesting();
 if (kvmSupportsNesting != qemuCaps->kvmSupportsNesting) {
 VIR_DEBUG("Outdated capabilities for '%s': kvm kernel nested "
@@ -5498,7 +5506,7 @@ virQEMUCapsInitQMP(virQEMUCaps *qemuCaps,
  * for TCG capabilities by asking the same binary again and turning KVM
  * off.
  */
-if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) &&
+if (virQEMUCapsHaveAccel(qemuCaps) &&
 virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG) &&
 virQEMUCapsInitQMPSingle(qemuCaps, libDir, runUid, runGid, true) < 0)
 return -1;
@@ -5561,13 +5569,15 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch,
 virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_KVM);
 virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_QEMU);
 
-if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
+if (virQEMUCapsHaveAccel(qemuCaps)) {
 qemuCaps->hostCPUSignature = g_strdup(hostCPUSignature);
 qemuCaps->microcodeVersion = microcodeVersion;
 qemuCaps->cpuData = virCPUDataNewCopy(cpuData);
 
 qemuCaps->kernelVersion = g_strdup(kernelVersion);
+}
 
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
 qemuCaps->kvmSupportsNesting = virQEMUCapsKVMSupportsNesting();
 
 qemuCaps->kvmSupportsSecureGuest = virQEMUCapsKVMSupportsSecureGuest();
-- 
2.31.1

[libvirt PATCH 07/17] qemu: Introduce virQEMUCapsAccelStr

[Andrea Bolognani]

From: Roman Bolshakov 

This makes possible to add more accelerators by touching less code and
reduces code duplication.

Signed-off-by: Roman Bolshakov 
Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_capabilities.c | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index b504963ddf..eac1e65a39 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -800,6 +800,16 @@ virQEMUCapsHaveAccel(virQEMUCaps *qemuCaps)
 return virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM);
 }
 
+static const char *
+virQEMUCapsAccelStr(virDomainVirtType type)
+{
+if (type == VIR_DOMAIN_VIRT_KVM) {
+return "kvm";
+} else {
+return "tcg";
+}
+}
+
 /* Checks whether a domain with @guest arch can run natively on @host.
  */
 bool
@@ -4075,7 +4085,7 @@ virQEMUCapsLoadAccel(virQEMUCaps *qemuCaps,
  virDomainVirtType type)
 {
 virQEMUCapsAccel *caps = virQEMUCapsGetAccel(qemuCaps, type);
-const char *typeStr = type == VIR_DOMAIN_VIRT_KVM ? "kvm" : "tcg";
+const char *typeStr = virQEMUCapsAccelStr(type);
 
 if (virQEMUCapsLoadHostCPUModelInfo(caps, ctxt, typeStr) < 0)
 return -1;
@@ -4624,7 +4634,7 @@ virQEMUCapsFormatAccel(virQEMUCaps *qemuCaps,
virDomainVirtType type)
 {
 virQEMUCapsAccel *caps = virQEMUCapsGetAccel(qemuCaps, type);
-const char *typeStr = type == VIR_DOMAIN_VIRT_KVM ? "kvm" : "tcg";
+const char *typeStr = virQEMUCapsAccelStr(type);
 
 virQEMUCapsFormatHostCPUModelInfo(caps, buf, typeStr);
 virQEMUCapsFormatCPUModels(caps, buf, typeStr);
-- 
2.31.1

[libvirt PATCH 05/17] qemu: Introduce virQEMUCapsTypeIsAccelerated

[Andrea Bolognani]

From: Roman Bolshakov 

It replaces hardcoded checks that select accelCPU/accelCPUModels
(formerly known as kvmCPU/kvmCPUModels) for KVM. It'll be cleaner to use
the function when multiple accelerators are supported in qemu driver.

Explicit KVM domain checks should be done only when a feature is
available only for KVM.

Signed-off-by: Roman Bolshakov 
Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_capabilities.c | 13 +
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index ca060485fa..e2ce2a5d07 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -788,6 +788,11 @@ const char *virQEMUCapsArchToString(virArch arch)
 return virArchToString(arch);
 }
 
+static bool
+virQEMUCapsTypeIsAccelerated(virDomainVirtType type)
+{
+return type == VIR_DOMAIN_VIRT_KVM;
+}
 
 /* Checks whether a domain with @guest arch can run natively on @host.
  */
@@ -2328,7 +2333,7 @@ virQEMUCapsIsCPUModeSupported(virQEMUCaps *qemuCaps,
 
 switch (mode) {
 case VIR_CPU_MODE_HOST_PASSTHROUGH:
-return type == VIR_DOMAIN_VIRT_KVM &&
+return virQEMUCapsTypeIsAccelerated(type) &&
virQEMUCapsGuestIsNative(hostarch, qemuCaps->arch);
 
 case VIR_CPU_MODE_HOST_MODEL:
@@ -2990,7 +2995,7 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCaps *qemuCaps,
qemuMonitor *mon,
virDomainVirtType virtType)
 {
-const char *model = virtType == VIR_DOMAIN_VIRT_KVM ? "host" : "max";
+const char *model = virQEMUCapsTypeIsAccelerated(virtType) ? "host" : 
"max";
 g_autoptr(qemuMonitorCPUModelInfo) modelInfo = NULL;
 g_autoptr(qemuMonitorCPUModelInfo) nonMigratable = NULL;
 g_autoptr(GHashTable) hash = NULL;
@@ -3700,7 +3705,7 @@ virQEMUCapsInitHostCPUModel(virQEMUCaps *qemuCaps,
   virArchToString(qemuCaps->arch),
   virDomainVirtTypeToString(type));
 goto error;
-} else if (type == VIR_DOMAIN_VIRT_KVM &&
+} else if (virQEMUCapsTypeIsAccelerated(type) &&
virCPUGetHostIsSupported(qemuCaps->arch)) {
 if (!(fullCPU = virQEMUCapsProbeHostCPU(qemuCaps->arch, NULL)))
 goto error;
@@ -5827,7 +5832,7 @@ virQEMUCapsCacheLookupDefault(virFileCache *cache,
 if (virttype == VIR_DOMAIN_VIRT_NONE)
 virttype = capsType;
 
-if (virttype == VIR_DOMAIN_VIRT_KVM && capsType == VIR_DOMAIN_VIRT_QEMU) {
+if (virQEMUCapsTypeIsAccelerated(virttype) && capsType == 
VIR_DOMAIN_VIRT_QEMU) {
 virReportError(VIR_ERR_INVALID_ARG,
_("KVM is not supported by '%s' on this host"),
binary);
-- 
2.31.1

[libvirt PATCH 04/17] qemu: Expose hvf domain type if hvf is supported

[Andrea Bolognani]

From: Roman Bolshakov 

Signed-off-by: Roman Bolshakov 
Reviewed-by: Daniel P. Berrangé 
Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_capabilities.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 9b4f695770..ca060485fa 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -1081,6 +1081,10 @@ virQEMUCapsInitGuestFromBinary(virCaps *caps,
 virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_KVM,
   NULL, NULL, 0, NULL);
 }
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_HVF)) {
+virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_HVF,
+  NULL, NULL, 0, NULL);
+}
 
 if ((ARCH_IS_X86(guestarch) || guestarch == VIR_ARCH_AARCH64))
 virCapabilitiesAddGuestFeatureWithToggle(guest, 
VIR_CAPS_GUEST_FEATURE_TYPE_ACPI,
-- 
2.31.1

[libvirt PATCH 03/17] qemu: Query hvf capability on macOS

[Andrea Bolognani]

From: Roman Bolshakov 

There's no QMP command for querying if hvf is supported, therefore we
use sysctl interface that tells if Hypervisor.framework works/available
on the host.

Signed-off-by: Roman Bolshakov 
Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_capabilities.c | 33 +
 1 file changed, 33 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 7823cb3d48..9b4f695770 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -56,6 +56,10 @@
 #include 
 #include 
 #include 
+#ifdef __APPLE__
+# include 
+# include 
+#endif
 
 #define VIR_FROM_THIS VIR_FROM_QEMU
 
@@ -3191,6 +3195,32 @@ virQEMUCapsProbeQMPKVMState(virQEMUCaps *qemuCaps,
 return 0;
 }
 
+#ifdef __APPLE__
+static int
+virQEMUCapsProbeHVF(virQEMUCaps *qemuCaps)
+{
+int hv_support;
+size_t len = sizeof(hv_support);
+
+if (sysctlbyname("kern.hv_support", _support, , NULL, 0) < 0)
+hv_support = 0;
+
+if (qemuCaps->version >= 2012000 &&
+ARCH_IS_X86(qemuCaps->arch) &&
+hv_support) {
+virQEMUCapsSet(qemuCaps, QEMU_CAPS_HVF);
+}
+
+return 0;
+}
+#else
+static int
+virQEMUCapsProbeHVF(virQEMUCaps *qemuCaps G_GNUC_UNUSED)
+{
+  return 0;
+}
+#endif
+
 struct virQEMUCapsCommandLineProps {
 const char *option;
 const char *param;
@@ -5335,6 +5365,9 @@ virQEMUCapsInitQMPMonitor(virQEMUCaps *qemuCaps,
 if (virQEMUCapsProbeQMPKVMState(qemuCaps, mon) < 0)
 return -1;
 
+if (virQEMUCapsProbeHVF(qemuCaps) < 0)
+return -1;
+
 type = virQEMUCapsGetVirtType(qemuCaps);
 accel = virQEMUCapsGetAccel(qemuCaps, type);
 
-- 
2.31.1

[libvirt PATCH 02/17] qemu: Define hvf capability

[Andrea Bolognani]

From: Roman Bolshakov 

Signed-off-by: Roman Bolshakov 
Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_capabilities.c | 1 +
 src/qemu/qemu_capabilities.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 4f63322a9e..7823cb3d48 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -653,6 +653,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
   "query-dirty-rate", /* QEMU_CAPS_QUERY_DIRTY_RATE */
   "rbd-encryption", /* QEMU_CAPS_RBD_ENCRYPTION */
   "sev-guest-kernel-hashes", /* QEMU_CAPS_SEV_GUEST_KERNEL_HASHES 
*/
+  "hvf", /* QEMU_CAPS_HVF */
 );
 
 
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index aaac20a834..4ffec44f6d 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -632,6 +632,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
syntax-check */
 QEMU_CAPS_QUERY_DIRTY_RATE, /* accepts query-dirty-rate */
 QEMU_CAPS_RBD_ENCRYPTION, /* Ceph RBD encryption support */
 QEMU_CAPS_SEV_GUEST_KERNEL_HASHES, /* sev-guest.kernel-hashes= */
+QEMU_CAPS_HVF, /* Whether Hypervisor.framework is available */
 
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
-- 
2.31.1

[libvirt PATCH 01/17] conf: Add hvf domain type

[Andrea Bolognani]

From: Roman Bolshakov 

QEMU supports Hypervisor.framework since 2.12 as hvf accel.
Hypervisor.framework provides a lightweight interface to run a virtual
cpu on macOS without the need to install third-party kernel
extensions (KEXTs).

It's supported since macOS 10.10 on machines with Intel VT-x feature
set that includes Extended Page Tables (EPT) and Unrestricted Mode.

Signed-off-by: Roman Bolshakov 
Signed-off-by: Andrea Bolognani 
---
 docs/formatdomain.rst | 3 ++-
 docs/schemas/domaincommon.rng | 1 +
 src/conf/domain_conf.c| 1 +
 src/conf/domain_conf.h| 1 +
 src/qemu/qemu_command.c   | 4 
 5 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index d4f30bb8af..3e9de05249 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -20,7 +20,8 @@ Element and attribute overview
 
 The root element required for all virtual machines is named ``domain``. It has
 two attributes, the ``type`` specifies the hypervisor used for running the
-domain. The allowed values are driver specific, but include "xen", "kvm", 
"qemu"
+domain. The allowed values are driver specific, but include "xen", "kvm",
+"hvf" (:since:`since 8.0.0 and QEMU 2.12`), "qemu"
 and "lxc". The second attribute is ``id`` which is a unique integer identifier
 for the running guest machine. Inactive machines have no id value.
 
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 7fa5c2b8b5..bf9c12397f 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -230,6 +230,7 @@
 phyp 
 vz
 bhyve
+hvf
   
 
   
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5691b8d2d5..0faecf2bb4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -125,6 +125,7 @@ VIR_ENUM_IMPL(virDomainVirt,
   "parallels",
   "bhyve",
   "vz",
+  "hvf",
 );
 
 VIR_ENUM_IMPL(virDomainOS,
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 144ba4dd12..d4d8aa7e23 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -139,6 +139,7 @@ typedef enum {
 VIR_DOMAIN_VIRT_PARALLELS,
 VIR_DOMAIN_VIRT_BHYVE,
 VIR_DOMAIN_VIRT_VZ,
+VIR_DOMAIN_VIRT_HVF,
 
 VIR_DOMAIN_VIRT_LAST
 } virDomainVirtType;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index d822533ccb..3bacf3edf2 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7229,6 +7229,10 @@ qemuBuildAccelCommandLine(virCommand *cmd,
 }
 break;
 
+case VIR_DOMAIN_VIRT_HVF:
+virBufferAddLit(, "hvf");
+break;
+
 case VIR_DOMAIN_VIRT_KQEMU:
 case VIR_DOMAIN_VIRT_XEN:
 case VIR_DOMAIN_VIRT_LXC:
-- 
2.31.1

[libvirt PATCH 00/17] qemu: Introduce hvf domain type for Hypervisor.framework

[Andrea Bolognani]

In order to hopefully address [libvirt#147] at long last, I've picked
up Roman's patches from 2018 and attempted to forward-port them.

More specifically, I've used the [roolebo/hvf-domain] branch as a
starting point, since it seems to contain a few improvements over
[v2] and was just easier to pick up.

The code is mostly his own, so I've retained the existing authorship
information, but I've dropped Reviewed-by tags for commits that have
been modified in non-trivial ways. I've applied very minimal style
tweaks along the way, but overall I've tried to modify the existing
patches as little as possible.

I've added a few changes of my own, which I've marked as "fixup!"
when I felt that they should be squashed into the previous patch
rather than existing as separate commits.

The new test cases, such as they are, pass, and no regressions to KVM
support appear to have been introduced in the process. I don't
currently have access to a machine running macOS, so I can't verify
that it's actually possible to start a hardware-accelerated VM.

Changes from [v2]:

  * rebased on top of master;
  * added a couple of simple test cases.

Notes / to do items:

  * I've skipped [kvmonly] because it was not obvious how to
forward-port it, and things generally seemed to work fine even
without it. I have some ideas on how to integrate the logic in
the current version of the code if it turns out to be necessary
after all;
  * I've also skipped most of the changes to docs/formatdomain.rst,
namely the ones that replaced "QEMU" or "QEMU/KVM" in :since:
blocks. I'm not yet sure how to best deal with the situation, but
adding "and HVF" everywhere is almost certainly not it;
  * docs/macos.html.in needs to be converted to ReStructuredText
before merging.

Useful links:

  * GitLab: [abologna/hvf]
  * CI: [pipeline]

[libvirt#147] https://gitlab.com/libvirt/libvirt/-/issues/147
[roolebo/hvf-domain] https://github.com/roolebo/libvirt/tree/hvf-domain
[abologna/hvf] https://gitlab.com/abologna/libvirt/-/commits/hvf
[pipeline] https://gitlab.com/abologna/libvirt/-/pipelines/441350810
[kvmonly] 
https://github.com/roolebo/libvirt/commit/1752ccbfe53bf4f89f3f45e4ef96b7da7a4cf87b
[v2] https://listman.redhat.com/archives/libvir-list/2018-November/msg00802.html

Andrea Bolognani (4):
  fixup! qemu: Fix HVF architecture check
  tests: Add HVF support to testutilsqemu
  tests: Add HVF test cases
  fixup! NEWS: Mention Apple Silicon support for HVF

Roman Bolshakov (13):
  conf: Add hvf domain type
  qemu: Define hvf capability
  qemu: Query hvf capability on macOS
  qemu: Expose hvf domain type if hvf is supported
  qemu: Introduce virQEMUCapsTypeIsAccelerated
  qemu: Introduce virQEMUCapsHaveAccel
  qemu: Introduce virQEMUCapsAccelStr
  qemu: Make error message accel-agnostic
  qemu: Correct CPU capabilities probing for hvf
  docs: Add hvf on QEMU driver page
  docs: Note hvf support for domain elements
  docs: Add support page for libvirt on macOS
  news: Mention hvf domain type

 NEWS.rst  |   9 +
 docs/docs.html.in |   3 +
 docs/drvqemu.rst  |  48 +++-
 docs/formatdomain.rst |  22 +-
 docs/index.html.in|   4 +-
 docs/macos.html.in| 229 ++
 docs/schemas/domaincommon.rng |   1 +
 src/conf/domain_conf.c|   1 +
 src/conf/domain_conf.h|   1 +
 src/qemu/qemu_capabilities.c  | 107 +++-
 src/qemu/qemu_capabilities.h  |   1 +
 src/qemu/qemu_command.c   |   4 +
 src/qemu/qemu_process.c   |   2 +-
 .../hvf-aarch64-virt-headless.args|  48 
 .../hvf-aarch64-virt-headless.xml |  45 
 .../hvf-x86_64-q35-headless.args  |  47 
 .../hvf-x86_64-q35-headless.xml   |  44 
 tests/qemuxml2argvtest.c  |  18 ++
 .../hvf-aarch64-virt-headless.xml |  94 +++
 .../hvf-x86_64-q35-headless.xml   |  97 
 tests/qemuxml2xmltest.c   |  18 ++
 tests/testutilsqemu.c |  40 +++
 22 files changed, 857 insertions(+), 26 deletions(-)
 create mode 100644 docs/macos.html.in
 create mode 100644 tests/qemuxml2argvdata/hvf-aarch64-virt-headless.args
 create mode 100644 tests/qemuxml2argvdata/hvf-aarch64-virt-headless.xml
 create mode 100644 tests/qemuxml2argvdata/hvf-x86_64-q35-headless.args
 create mode 100644 tests/qemuxml2argvdata/hvf-x86_64-q35-headless.xml
 create mode 100644 tests/qemuxml2xmloutdata/hvf-aarch64-virt-headless.xml
 create mode 100644 tests/qemuxml2xmloutdata/hvf-x86_64-q35-headless.xml

-- 
2.31.1

[PATCH] ci: Refresh configuration

[Andrea Bolognani]

Notable changes:

  * drop parted and XFS headers.

Signed-off-by: Andrea Bolognani 
---
Pushed under the CI refresh rule.

Test pipeline: https://gitlab.com/abologna/libvirt/-/pipelines/441266800
Full patch: 
https://gitlab.com/libvirt/libvirt/-/commit/4c7316b2f00c847bc5dc9c8218c2585d2d04636a

 ci/containers/centos-8.Dockerfile | 2 --
 ci/containers/centos-stream-8.Dockerfile  | 2 --
 ci/containers/debian-10-cross-aarch64.Dockerfile  | 4 +---
 ci/containers/debian-10-cross-armv6l.Dockerfile   | 4 +---
 ci/containers/debian-10-cross-armv7l.Dockerfile   | 4 +---
 ci/containers/debian-10-cross-i686.Dockerfile | 4 +---
 ci/containers/debian-10-cross-mips.Dockerfile | 4 +---
 ci/containers/debian-10-cross-mips64el.Dockerfile | 4 +---
 ci/containers/debian-10-cross-mipsel.Dockerfile   | 4 +---
 ci/containers/debian-10-cross-ppc64le.Dockerfile  | 4 +---
 ci/containers/debian-10-cross-s390x.Dockerfile| 4 +---
 ci/containers/debian-10.Dockerfile| 2 --
 ci/containers/debian-11-cross-aarch64.Dockerfile  | 4 +---
 ci/containers/debian-11-cross-armv6l.Dockerfile   | 4 +---
 ci/containers/debian-11-cross-armv7l.Dockerfile   | 4 +---
 ci/containers/debian-11-cross-i686.Dockerfile | 4 +---
 ci/containers/debian-11-cross-mips64el.Dockerfile | 4 +---
 ci/containers/debian-11-cross-mipsel.Dockerfile   | 4 +---
 ci/containers/debian-11-cross-ppc64le.Dockerfile  | 4 +---
 ci/containers/debian-11-cross-s390x.Dockerfile| 4 +---
 ci/containers/debian-11.Dockerfile| 2 --
 ci/containers/debian-sid-cross-aarch64.Dockerfile | 4 +---
 ci/containers/debian-sid-cross-armv6l.Dockerfile  | 4 +---
 ci/containers/debian-sid-cross-armv7l.Dockerfile  | 4 +---
 ci/containers/debian-sid-cross-i686.Dockerfile| 4 +---
 ci/containers/debian-sid-cross-mips64el.Dockerfile| 4 +---
 ci/containers/debian-sid-cross-mipsel.Dockerfile  | 4 +---
 ci/containers/debian-sid-cross-ppc64le.Dockerfile | 4 +---
 ci/containers/debian-sid-cross-s390x.Dockerfile   | 4 +---
 ci/containers/debian-sid.Dockerfile   | 2 --
 ci/containers/fedora-34.Dockerfile| 2 --
 ci/containers/fedora-35-cross-mingw32.Dockerfile  | 1 -
 ci/containers/fedora-35-cross-mingw64.Dockerfile  | 1 -
 ci/containers/fedora-35.Dockerfile| 2 --
 ci/containers/fedora-rawhide-cross-mingw32.Dockerfile | 1 -
 ci/containers/fedora-rawhide-cross-mingw64.Dockerfile | 1 -
 ci/containers/fedora-rawhide.Dockerfile   | 2 --
 ci/containers/opensuse-leap-152.Dockerfile| 4 +---
 ci/containers/opensuse-tumbleweed.Dockerfile  | 4 +---
 ci/containers/ubuntu-1804.Dockerfile  | 2 --
 ci/containers/ubuntu-2004.Dockerfile  | 2 --
 41 files changed, 27 insertions(+), 105 deletions(-)

diff --git a/ci/containers/centos-8.Dockerfile 
b/ci/containers/centos-8.Dockerfile
index 358069ad88..f01ca6a0d9 100644
--- a/ci/containers/centos-8.Dockerfile
+++ b/ci/containers/centos-8.Dockerfile
@@ -67,7 +67,6 @@ RUN dnf update -y && \
 ninja-build \
 numactl-devel \
 numad \
-parted \
 parted-devel \
 perl \
 pkgconfig \
@@ -85,7 +84,6 @@ RUN dnf update -y && \
 systemd-devel \
 systemtap-sdt-devel \
 wireshark-devel \
-xfsprogs-devel \
 yajl-devel && \
 dnf autoremove -y && \
 dnf clean all -y && \

[... a lot more like this ...]

-- 
2.31.1

[PATCH] Account for fact that virDomainDeviceDefCopy() does an inactive copy

[Michal Privoznik]

In a few places (e.g. device attach/detach/update) we are given a
device XML, parse it but then need a copy of parsed data so that
the original can be passed to function handling the request over
inactive XML and the copy is then passed to function handling the
operation over live XML. Note, both functions consume passed
device on success, hence the need for copy.

The problem is in combination of how the copy is obtained and
where is passed. The copy is done by calling
virDomainDeviceDefCopy() which does only inactive copy, i.e. no
live information is copied over (e.g. no aliases).

Then, this copy (inactive XML effectively) is passed to function
handling live part of the operation (e.g.
qemuDomainUpdateDeviceLive()) and the definition containing all
the juicy, live bits is passed to function handling inactive part
of the operation (e.g. qemuDomainUpdateDeviceConfig()).

This is rather suboptimal, and XML copies should be passed to
their respective functions.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2036895
Signed-off-by: Michal Privoznik 
---
 src/lxc/lxc_driver.c   | 10 +-
 src/qemu/qemu_driver.c |  8 
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index fe583ccb76..7bc39120ee 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -4308,17 +4308,17 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom,
  false) < 0)
 goto endjob;
 
-if ((ret = lxcDomainAttachDeviceConfig(vmdef, dev)) < 0)
+if ((ret = lxcDomainAttachDeviceConfig(vmdef, dev_copy)) < 0)
 goto endjob;
 }
 
 if (flags & VIR_DOMAIN_AFFECT_LIVE) {
-if (virDomainDefCompatibleDevice(vm->def, dev_copy, NULL,
+if (virDomainDefCompatibleDevice(vm->def, dev, NULL,
  VIR_DOMAIN_DEVICE_ACTION_ATTACH,
  true) < 0)
 goto endjob;
 
-if ((ret = lxcDomainAttachDeviceLive(driver, vm, dev_copy)) < 0)
+if ((ret = lxcDomainAttachDeviceLive(driver, vm, dev)) < 0)
 goto endjob;
 /*
  * update domain status forcibly because the domain status may be
@@ -4475,12 +4475,12 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom,
 if (!vmdef)
 goto endjob;
 
-if ((ret = lxcDomainDetachDeviceConfig(vmdef, dev)) < 0)
+if ((ret = lxcDomainDetachDeviceConfig(vmdef, dev_copy)) < 0)
 goto endjob;
 }
 
 if (flags & VIR_DOMAIN_AFFECT_LIVE) {
-if ((ret = lxcDomainDetachDeviceLive(driver, vm, dev_copy)) < 0)
+if ((ret = lxcDomainDetachDeviceLive(driver, vm, dev)) < 0)
 goto endjob;
 /*
  * update domain status forcibly because the domain status may be
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b8537a4260..b1255da9f2 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8020,7 +8020,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
 
 /* virDomainDefCompatibleDevice call is delayed until we know the
  * device we're going to update. */
-if ((ret = qemuDomainUpdateDeviceConfig(vmdef, dev, priv->qemuCaps,
+if ((ret = qemuDomainUpdateDeviceConfig(vmdef, dev_copy, 
priv->qemuCaps,
 parse_flags,
 driver->xmlopt)) < 0)
 goto endjob;
@@ -8029,7 +8029,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
 if (flags & VIR_DOMAIN_AFFECT_LIVE) {
 /* virDomainDefCompatibleDevice call is delayed until we know the
  * device we're going to update. */
-if ((ret = qemuDomainUpdateDeviceLive(vm, dev_copy, dom, force)) < 0)
+if ((ret = qemuDomainUpdateDeviceLive(vm, dev, dom, force)) < 0)
 goto endjob;
 
 qemuDomainSaveStatus(vm);
@@ -8100,7 +8100,7 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriver *driver,
 if (!vmdef)
 goto cleanup;
 
-if (qemuDomainDetachDeviceConfig(vmdef, dev, priv->qemuCaps,
+if (qemuDomainDetachDeviceConfig(vmdef, dev_copy, priv->qemuCaps,
  parse_flags,
  driver->xmlopt) < 0)
 goto cleanup;
@@ -8109,7 +8109,7 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriver *driver,
 if (flags & VIR_DOMAIN_AFFECT_LIVE) {
 int rc;
 
-if ((rc = qemuDomainDetachDeviceLive(vm, dev_copy, driver, false)) < 0)
+if ((rc = qemuDomainDetachDeviceLive(vm, dev, driver, false)) < 0)
 goto cleanup;
 
 if (rc == 0 && qemuDomainUpdateDeviceList(driver, vm, 
QEMU_ASYNC_JOB_NONE) < 0)
-- 
2.34.1

Re: [PATCH v6 3/4] remove sysconfig files

[Olaf Hering]

Mon, 3 Jan 2022 03:18:11 -0800 Andrea Bolognani :

> The fact that we still QEMU_AUDIO_DRV and SDL_AUDIODRIVER in the
> service file even after your changes goes against this principle.

These variables are probably stale.
But, I just looked briefly at the code and could not figure out within minutes 
what the purpose was.
Perhaps I will remove them as well in case a v8 is required.

Olaf


pgpu37DJXPzuk.pgp
Description: Digitale Signatur von OpenPGP

Re: [PATCH 2/2] domain_conf: Use virXMLFormatElement*() more in virDomainDefFormatFeatures()

[Andrea Bolognani]

On Tue, Dec 14, 2021 at 03:01:19PM +0100, Michal Privoznik wrote:
>  case VIR_TRISTATE_SWITCH_ON:
> -   virBufferAsprintf(, "<%s state='on'/>\n", name);
> -   break;
> -
>  case VIR_TRISTATE_SWITCH_OFF:
> -   virBufferAsprintf(, "<%s state='off'/>\n", name);
> -   break;
> +virBufferAsprintf(, " state='%s'",
> +  
> virTristateSwitchTypeToString(def->features[i]));
> +
> +virXMLFormatElement(, name, , NULL);
> +break;

You even fixed indentation as a side effect! Very nice :)

>  case VIR_DOMAIN_FEATURE_GIC:
>  if (def->features[i] == VIR_TRISTATE_SWITCH_ON) {
> -virBufferAddLit(, "  if (def->gic_version != VIR_GIC_VERSION_NONE)
> -virBufferAsprintf(, " version='%s'",
> +virBufferAsprintf(, " version='%s'",
>
> virGICVersionTypeToString(def->gic_version));
> -virBufferAddLit(, "/>\n");
> +virXMLFormatElementEmpty(, "gic", , 
> NULL);

I think either adding an empty line before virXMLFormatElementEmpty()
or braces around the check on def->gic_version would improve
readability.

Regardless

  Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH 1/2] qemuxml2xmloutdata: Turn kvm-features.xml and kvm-features-off.xml into symlinks

[Andrea Bolognani]

On Tue, Dec 14, 2021 at 03:01:18PM +0100, Michal Privoznik wrote:
> qemuxml2xmloutdata: Turn kvm-features.xml and kvm-features-off.xml into 
> symlinks

Maybe change this to

  qemuxml2xmloutdata: Turn kvm-features*.xml into symlinks

for brevity's sake.

Either way,

  Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH 2/2] virnetdevopenvswitch: Fix 'burst' value passed to ovs-vsctl

[Martin Kletzander]

On Tue, Jan 04, 2022 at 01:53:06PM +0100, Michal Privoznik wrote:

As described in the previous commit, the units for 'burst' are
kibibytes and not kilobytes, i.e. multiples of 1024 not 1000.
Therefore, when constructing ovs-vsctl command the burst value
must be multiplied by 1024 and not just 1000. And because ovs
expects this size in bits the value has to be multiplied again by
8.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1510237#c26
Signed-off-by: Michal Privoznik 
---
src/util/virnetdevopenvswitch.c | 16 +---
1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
index 5dab545037..8741e0ed3a 100644
--- a/src/util/virnetdevopenvswitch.c
+++ b/src/util/virnetdevopenvswitch.c
@@ -638,11 +638,13 @@ virNetDevOpenvswitchFindUUID(const char *table,
}

/*
- * Average, peak, floor and burst in virNetDevBandwidth are in kbytes.
- * However other_config in ovs qos is in bit.
- * ingress_policing_rate in ovs interface is in kbit.
+ * In virNetDevBandwidthRate, average, peak, floor are in kilobyes and burst in
+ * kibibytes. However other_config in ovs qos is in bits and
+ * ingress_policing_rate in ovs interface is in kilobit for
+ * ingress_policing_rate and kibibit for ingress_policing_rate.


I guess you meant ingress_policing_burst at the end and the sentence
could be written a bit less confusingly.  With that fixed

Reviewed-by: Martin Kletzander 


signature.asc
Description: PGP signature

Re: [PATCH 1/2] docs: Clarify 'burst' units for QoS

[Martin Kletzander]

On Tue, Jan 04, 2022 at 01:53:05PM +0100, Michal Privoznik wrote:

The burst attribute for bandwidth specifies how much bytes can be
transmitted in a single burst. Therefore, the unit is in
multiples of 1024 (thus kibibytes) not SI-like 1000. It always
has been like that.


s/always has/has always/ I guess

and

Reviewed-by: Martin Kletzander 


signature.asc
Description: PGP signature

Re: [libvirt PATCH v3 0/4] docs: Unclutter top-level directory

[Andrea Bolognani]

On Tue, Jan 04, 2022 at 02:25:14PM +, Daniel P. Berrangé wrote:
> On Tue, Jan 04, 2022 at 02:55:58PM +0100, Andrea Bolognani wrote:
> >  docs/{ => images}/node.gif| Bin
>
> Ew, just noticed "gif" file, how did we let that get in here.
> We should burn it [1] !
>
> But seriously, pre-existing + tangential problem so not actually
> asking you to fix it in this series, unless you fancy it.

Yeah, I noticed that as well while preparing these patches and had
the very same reaction :) Sort of forgot about it in the meantime,
but now that you've reminded me I'll look into addressing it with a
follow-up patch.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH 0/3] storage: Use the FICLONE ioctl unconditionally on Linux

[Michal Prívozník]

On 12/28/21 19:40, Andrea Bolognani wrote:
> Test pipeline: https://gitlab.com/abologna/libvirt/-/pipelines/438075320
> 
> Andrea Bolognani (3):
>   storage: Use the FICLONE ioctl unconditionally on Linux
>   meson: Don't look for btrfs and xfs headers
>   spec: Drop BuildRequires on xfsprogs-devel
> 
>  libvirt.spec.in|  2 --
>  meson.build|  4 
>  src/storage/storage_util.c | 14 ++
>  3 files changed, 2 insertions(+), 18 deletions(-)
> 

Reviewed-by: Michal Privoznik 

Michal

Re: [libvirt PATCH v3 4/4] docs: Move font definitions with other CSS files

[Daniel P . Berrangé]

On Tue, Jan 04, 2022 at 02:56:02PM +0100, Andrea Bolognani wrote:
> We have a subdirectory specifically for CSS files now, so it makes
> sense to have the stylesheet that defines fonts to be there too.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  docs/{fonts/stylesheet.css => css/fonts.css} | 18 +-
>  docs/css/main.css|  2 +-
>  docs/css/meson.build |  1 +
>  docs/fonts/meson.build   |  1 -
>  4 files changed, 11 insertions(+), 11 deletions(-)
>  rename docs/{fonts/stylesheet.css => css/fonts.css} (60%)

Reviewed-by: Daniel P. Berrangé 


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: [libvirt PATCH v3 3/4] docs: Move all CSS files to a subdirectory

[Daniel P . Berrangé]

On Tue, Jan 04, 2022 at 02:56:01PM +0100, Andrea Bolognani wrote:
> This unclutters the top-level docs directory.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  docs/{ => css}/generic.css |  0
>  docs/{ => css}/libvirt.css |  4 ++--
>  docs/{ => css}/main.css|  2 +-
>  docs/css/meson.build   | 16 
>  docs/{ => css}/mobile.css  |  2 +-
>  docs/meson.build   |  5 +
>  docs/page.xsl  |  2 +-
>  7 files changed, 22 insertions(+), 9 deletions(-)
>  rename docs/{ => css}/generic.css (100%)
>  rename docs/{ => css}/libvirt.css (98%)
>  rename docs/{ => css}/main.css (66%)
>  create mode 100644 docs/css/meson.build
>  rename docs/{ => css}/mobile.css (95%)

Reviewed-by: Daniel P. Berrangé 


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: [libvirt PATCH v3 2/4] docs: Move all images to a subdirectory

[Daniel P . Berrangé]

On Tue, Jan 04, 2022 at 02:56:00PM +0100, Andrea Bolognani wrote:
> This unclutters the top-level docs directory.
> 
> Signed-off-by: Andrea Bolognani 
> Reviewed-by: Ani Sinha 
> ---
>  docs/api.rst  |   6 ++---
>  docs/goals.html.in|   2 +-
>  docs/{ => images}/event_loop_simple.png   | Bin
>  docs/{ => images}/event_loop_simple.svg   |   0
>  docs/{ => images}/event_loop_worker.png   | Bin
>  docs/{ => images}/event_loop_worker.svg   |   0
>  docs/{ => images}/libvirt-daemon-arch.png | Bin
>  docs/{ => images}/libvirt-daemon-arch.svg |   0
>  docs/{ => images}/libvirt-driver-arch.png | Bin
>  docs/{ => images}/libvirt-driver-arch.svg |   0
>  docs/{ => images}/libvirt-object-model.png| Bin
>  docs/{ => images}/libvirt-object-model.svg|   0
>  .../libvirt-virConnect-example.png| Bin
>  .../libvirt-virConnect-example.svg|   0
>  docs/images/meson.build   |  24 ++
>  .../{ => images}/migration-managed-direct.png | Bin
>  .../{ => images}/migration-managed-direct.svg |   0
>  docs/{ => images}/migration-managed-p2p.png   | Bin
>  docs/{ => images}/migration-managed-p2p.svg   |   0
>  docs/{ => images}/migration-native.png| Bin
>  docs/{ => images}/migration-native.svg|   0
>  docs/{ => images}/migration-tunnel.png| Bin
>  docs/{ => images}/migration-tunnel.svg|   0
>  .../migration-unmanaged-direct.png| Bin
>  .../migration-unmanaged-direct.svg|   0
>  docs/{ => images}/node.gif| Bin
>  docs/{ => images}/node.svg|   0
>  docs/internals.html.in|   2 +-
>  docs/internals/eventloop.html.in  |   4 +--
>  docs/meson.build  |  13 +-
>  docs/migration.rst|  10 
>  31 files changed, 37 insertions(+), 24 deletions(-)
>  rename docs/{ => images}/event_loop_simple.png (100%)
>  rename docs/{ => images}/event_loop_simple.svg (100%)
>  rename docs/{ => images}/event_loop_worker.png (100%)
>  rename docs/{ => images}/event_loop_worker.svg (100%)
>  rename docs/{ => images}/libvirt-daemon-arch.png (100%)
>  rename docs/{ => images}/libvirt-daemon-arch.svg (100%)
>  rename docs/{ => images}/libvirt-driver-arch.png (100%)
>  rename docs/{ => images}/libvirt-driver-arch.svg (100%)
>  rename docs/{ => images}/libvirt-object-model.png (100%)
>  rename docs/{ => images}/libvirt-object-model.svg (100%)
>  rename docs/{ => images}/libvirt-virConnect-example.png (100%)
>  rename docs/{ => images}/libvirt-virConnect-example.svg (100%)
>  create mode 100644 docs/images/meson.build
>  rename docs/{ => images}/migration-managed-direct.png (100%)
>  rename docs/{ => images}/migration-managed-direct.svg (100%)
>  rename docs/{ => images}/migration-managed-p2p.png (100%)
>  rename docs/{ => images}/migration-managed-p2p.svg (100%)
>  rename docs/{ => images}/migration-native.png (100%)
>  rename docs/{ => images}/migration-native.svg (100%)
>  rename docs/{ => images}/migration-tunnel.png (100%)
>  rename docs/{ => images}/migration-tunnel.svg (100%)
>  rename docs/{ => images}/migration-unmanaged-direct.png (100%)
>  rename docs/{ => images}/migration-unmanaged-direct.svg (100%)
>  rename docs/{ => images}/node.gif (100%)
>  rename docs/{ => images}/node.svg (100%)

Reviewed-by: Daniel P. Berrangé 


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: [libvirt PATCH v3 1/4] docs: Drop structures.svg

[Daniel P . Berrangé]

On Tue, Jan 04, 2022 at 02:55:59PM +0100, Andrea Bolognani wrote:
> It was introduced in ff4ede005567 but it doesn't seem to have
> ever actually been used anywhere.
> 
> Signed-off-by: Andrea Bolognani 
> Reviewed-by: Ani Sinha 
> ---
>  docs/structures.svg | 187 
>  1 file changed, 187 deletions(-)
>  delete mode 100644 docs/structures.svg

Reviewed-by: Daniel P. Berrangé 


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: [libvirt PATCH v3 0/4] docs: Unclutter top-level directory

[Daniel P . Berrangé]

On Tue, Jan 04, 2022 at 02:55:58PM +0100, Andrea Bolognani wrote:
> Changes from [v2]:
> 
>   * drop all changes related to icons.
> 
> Changes from [v1]:
> 
>   * extend scope to include icons and CSS files.
> 
> [v2] 
> https://listman.redhat.com/archives/libvir-list/2021-December/msg00828.html
> [v1] 
> https://listman.redhat.com/archives/libvir-list/2021-December/msg00818.html
> 
> Andrea Bolognani (4):
>   docs: Drop structures.svg
>   docs: Move all images to a subdirectory
>   docs: Move all CSS files to a subdirectory
>   docs: Move font definitions with other CSS files
> 

>  docs/{ => images}/node.gif| Bin
>  docs/{ => images}/node.svg|   0

Ew, just noticed "gif" file, how did we let that get in here.
We should burn it [1] !

But seriously, pre-existing + tangential problem so not actually
asking you to fix it in this series, unless you fancy it.

Regards,
Daniel

[1] https://burnallgifs.org/archives/
-- 
|: 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: [RFC v2 1/1] qemu: add index for isa-serial device using target.port

[Daniel P . Berrangé]

On Tue, Jan 04, 2022 at 11:47:39AM +, John Levon wrote:
> On Sat, Dec 11, 2021 at 07:57:47PM -0800, “Divya wrote:
> 
> > From: Divya Garg 
> > 
> > VM XML accepts target.port but this does not get passed while building the 
> > qemu
> > command line for this VM.
> 
> Apologies, I failed to notice this had been sent out to the list; Re-posting
> my comments from an internal thread, so all can see:
> 
> I don't think serial ports that are not isa-serial should clash with serial
> ports like this.
> 
> In fact, from what I can see, as "port" is not used for any other type, we
> should just completely ignore non-isa-serial ports here.

Agreed, conceptually the 'port' is only required to be unique within
the scope of the 'type' setting.


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

[libvirt PATCH v3 3/4] docs: Move all CSS files to a subdirectory

[Andrea Bolognani]

This unclutters the top-level docs directory.

Signed-off-by: Andrea Bolognani 
---
 docs/{ => css}/generic.css |  0
 docs/{ => css}/libvirt.css |  4 ++--
 docs/{ => css}/main.css|  2 +-
 docs/css/meson.build   | 16 
 docs/{ => css}/mobile.css  |  2 +-
 docs/meson.build   |  5 +
 docs/page.xsl  |  2 +-
 7 files changed, 22 insertions(+), 9 deletions(-)
 rename docs/{ => css}/generic.css (100%)
 rename docs/{ => css}/libvirt.css (98%)
 rename docs/{ => css}/main.css (66%)
 create mode 100644 docs/css/meson.build
 rename docs/{ => css}/mobile.css (95%)

diff --git a/docs/generic.css b/docs/css/generic.css
similarity index 100%
rename from docs/generic.css
rename to docs/css/generic.css
diff --git a/docs/libvirt.css b/docs/css/libvirt.css
similarity index 98%
rename from docs/libvirt.css
rename to docs/css/libvirt.css
index 5195588a8f..b08271ea4d 100644
--- a/docs/libvirt.css
+++ b/docs/css/libvirt.css
@@ -10,7 +10,7 @@
 }
 
 #home {
-background-image: url(logos/logo-banner-light-256.png);
+background-image: url(../logos/logo-banner-light-256.png);
 background-repeat: no-repeat;
 background-position: left center;
 height: 100px;
@@ -419,7 +419,7 @@ div.panel h2,
 #index.document h1 {
 border: 0px;
 text-indent: 100%; white-space: nowrap; overflow: hidden;
-background: url(logos/logo-banner-dark-800.png) no-repeat center center;
+background: url(../logos/logo-banner-dark-800.png) no-repeat center center;
 height: 300px;
 }
 
diff --git a/docs/main.css b/docs/css/main.css
similarity index 66%
rename from docs/main.css
rename to docs/css/main.css
index 71f7b7a6a0..8961f1a4b4 100644
--- a/docs/main.css
+++ b/docs/css/main.css
@@ -1,4 +1,4 @@
-@import url(fonts/stylesheet.css);
+@import url(../fonts/stylesheet.css);
 @import url(generic.css);
 @import url(libvirt.css);
 @import url(mobile.css);
diff --git a/docs/css/meson.build b/docs/css/meson.build
new file mode 100644
index 00..35e56347a6
--- /dev/null
+++ b/docs/css/meson.build
@@ -0,0 +1,16 @@
+docs_css_files = [
+  'generic.css',
+  'libvirt.css',
+  'main.css',
+  'mobile.css',
+]
+
+install_data(docs_css_files, install_dir: docs_html_dir / 'css')
+
+foreach file : docs_css_files
+  # This hack enables us to view the web pages
+  # from within the uninstalled build tree
+  configure_file(input: file, output: file, copy: true)
+
+  install_web_files += '@0@:@1@'.format(meson.current_source_dir() / file, 
docs_html_dir / 'css')
+endforeach
diff --git a/docs/mobile.css b/docs/css/mobile.css
similarity index 95%
rename from docs/mobile.css
rename to docs/css/mobile.css
index 366d0f1a5d..ae833b6eea 100644
--- a/docs/mobile.css
+++ b/docs/css/mobile.css
@@ -3,7 +3,7 @@
width: 100%;
display: block;
margin: 0px;
-   background: white url(logos/logo-banner-dark-256.png) no-repeat center 
center;
+   background: white url(../logos/logo-banner-dark-256.png) no-repeat 
center center;
height: 94px;
 }
 #home a {
diff --git a/docs/meson.build b/docs/meson.build
index 6f1ca5c6ff..3e912f21ad 100644
--- a/docs/meson.build
+++ b/docs/meson.build
@@ -13,11 +13,7 @@ docs_assets = [
   'favicon.ico',
   'favicon-16x16.png',
   'favicon-32x32.png',
-  'generic.css',
-  'libvirt.css',
-  'main.css',
   'manifest.json',
-  'mobile.css',
   'mstile-150x150.png',
 ]
 
@@ -288,6 +284,7 @@ html_xslt_gen = []
 
 # --- end of XSLT processing ---
 
+subdir('css')
 subdir('fonts')
 subdir('go')
 subdir('html')
diff --git a/docs/page.xsl b/docs/page.xsl
index 580387ac59..fd67918d3b 100644
--- a/docs/page.xsl
+++ b/docs/page.xsl
@@ -91,7 +91,7 @@
   
 
 
-
+
 
 
 
-- 
2.31.1

[libvirt PATCH v3 2/4] docs: Move all images to a subdirectory

[Andrea Bolognani]

This unclutters the top-level docs directory.

Signed-off-by: Andrea Bolognani 
Reviewed-by: Ani Sinha 
---
 docs/api.rst  |   6 ++---
 docs/goals.html.in|   2 +-
 docs/{ => images}/event_loop_simple.png   | Bin
 docs/{ => images}/event_loop_simple.svg   |   0
 docs/{ => images}/event_loop_worker.png   | Bin
 docs/{ => images}/event_loop_worker.svg   |   0
 docs/{ => images}/libvirt-daemon-arch.png | Bin
 docs/{ => images}/libvirt-daemon-arch.svg |   0
 docs/{ => images}/libvirt-driver-arch.png | Bin
 docs/{ => images}/libvirt-driver-arch.svg |   0
 docs/{ => images}/libvirt-object-model.png| Bin
 docs/{ => images}/libvirt-object-model.svg|   0
 .../libvirt-virConnect-example.png| Bin
 .../libvirt-virConnect-example.svg|   0
 docs/images/meson.build   |  24 ++
 .../{ => images}/migration-managed-direct.png | Bin
 .../{ => images}/migration-managed-direct.svg |   0
 docs/{ => images}/migration-managed-p2p.png   | Bin
 docs/{ => images}/migration-managed-p2p.svg   |   0
 docs/{ => images}/migration-native.png| Bin
 docs/{ => images}/migration-native.svg|   0
 docs/{ => images}/migration-tunnel.png| Bin
 docs/{ => images}/migration-tunnel.svg|   0
 .../migration-unmanaged-direct.png| Bin
 .../migration-unmanaged-direct.svg|   0
 docs/{ => images}/node.gif| Bin
 docs/{ => images}/node.svg|   0
 docs/internals.html.in|   2 +-
 docs/internals/eventloop.html.in  |   4 +--
 docs/meson.build  |  13 +-
 docs/migration.rst|  10 
 31 files changed, 37 insertions(+), 24 deletions(-)
 rename docs/{ => images}/event_loop_simple.png (100%)
 rename docs/{ => images}/event_loop_simple.svg (100%)
 rename docs/{ => images}/event_loop_worker.png (100%)
 rename docs/{ => images}/event_loop_worker.svg (100%)
 rename docs/{ => images}/libvirt-daemon-arch.png (100%)
 rename docs/{ => images}/libvirt-daemon-arch.svg (100%)
 rename docs/{ => images}/libvirt-driver-arch.png (100%)
 rename docs/{ => images}/libvirt-driver-arch.svg (100%)
 rename docs/{ => images}/libvirt-object-model.png (100%)
 rename docs/{ => images}/libvirt-object-model.svg (100%)
 rename docs/{ => images}/libvirt-virConnect-example.png (100%)
 rename docs/{ => images}/libvirt-virConnect-example.svg (100%)
 create mode 100644 docs/images/meson.build
 rename docs/{ => images}/migration-managed-direct.png (100%)
 rename docs/{ => images}/migration-managed-direct.svg (100%)
 rename docs/{ => images}/migration-managed-p2p.png (100%)
 rename docs/{ => images}/migration-managed-p2p.svg (100%)
 rename docs/{ => images}/migration-native.png (100%)
 rename docs/{ => images}/migration-native.svg (100%)
 rename docs/{ => images}/migration-tunnel.png (100%)
 rename docs/{ => images}/migration-tunnel.svg (100%)
 rename docs/{ => images}/migration-unmanaged-direct.png (100%)
 rename docs/{ => images}/migration-unmanaged-direct.svg (100%)
 rename docs/{ => images}/node.gif (100%)
 rename docs/{ => images}/node.svg (100%)

diff --git a/docs/api.rst b/docs/api.rst
index a8f527e197..d9f01fb403 100644
--- a/docs/api.rst
+++ b/docs/api.rst
@@ -260,6 +260,6 @@ rules and guidelines. In order to add new API functionality 
follow the
 instructions regarding `implementing a new API in
 libvirt `__.
 
-.. |first class objects exposed by the API| image:: libvirt-object-model.png
-.. |The libvirt driver architecture| image:: libvirt-driver-arch.png
-.. |The libvirt daemon and remote architecture| image:: libvirt-daemon-arch.png
+.. |first class objects exposed by the API| image:: 
images/libvirt-object-model.png
+.. |The libvirt driver architecture| image:: images/libvirt-driver-arch.png
+.. |The libvirt daemon and remote architecture| image:: 
images/libvirt-daemon-arch.png
diff --git a/docs/goals.html.in b/docs/goals.html.in
index 39d5e75359..d205bf4f42 100644
--- a/docs/goals.html.in
+++ b/docs/goals.html.in
@@ -15,7 +15,7 @@
 virtualized machine provided by the hypervisor
 
 
-  
+  
 
 Now we can define the goal of libvirt:  to provide a common and
 stable layer sufficient to securely manage domains on a node, possibly
diff --git a/docs/event_loop_simple.png b/docs/images/event_loop_simple.png
similarity index 100%
rename from docs/event_loop_simple.png
rename to docs/images/event_loop_simple.png
diff --git a/docs/event_loop_simple.svg b/docs/images/event_loop_simple.svg
similarity index 100%
rename from docs/event_loop_simple.svg
rename to docs/images/event_loop_simple.svg
diff --git a/docs/event_loop_worker.png b/docs/images/event_loop_worker.png
similarity index 100%
rename from docs/event_loop_worker.png
rename to docs/images/event_loop_worker.png
diff --git a/docs/event_loop_worker.svg 

[libvirt PATCH v3 4/4] docs: Move font definitions with other CSS files

[Andrea Bolognani]

We have a subdirectory specifically for CSS files now, so it makes
sense to have the stylesheet that defines fonts to be there too.

Signed-off-by: Andrea Bolognani 
---
 docs/{fonts/stylesheet.css => css/fonts.css} | 18 +-
 docs/css/main.css|  2 +-
 docs/css/meson.build |  1 +
 docs/fonts/meson.build   |  1 -
 4 files changed, 11 insertions(+), 11 deletions(-)
 rename docs/{fonts/stylesheet.css => css/fonts.css} (60%)

diff --git a/docs/fonts/stylesheet.css b/docs/css/fonts.css
similarity index 60%
rename from docs/fonts/stylesheet.css
rename to docs/css/fonts.css
index 1a06f22c35..214d324346 100644
--- a/docs/fonts/stylesheet.css
+++ b/docs/css/fonts.css
@@ -1,62 +1,62 @@
 @font-face {
font-family: 'LibvirtOverpass';
-   src: url('overpass-regular.woff') format('woff');
+   src: url('../fonts/overpass-regular.woff') format('woff');
font-weight: normal;
font-style: normal;
 }
 
 @font-face {
font-family: 'LibvirtOverpass';
-   src: url('overpass-italic.woff') format('woff');
+   src: url('../fonts/overpass-italic.woff') format('woff');
font-weight: normal;
font-style: italic;
 }
 
 @font-face {
font-family: 'LibvirtOverpass';
-   src: url('overpass-bold.woff') format('woff');
+   src: url('../fonts/overpass-bold.woff') format('woff');
font-weight: bold;
font-style: normal;
 }
 
 @font-face {
font-family: 'LibvirtOverpass';
-   src: url('overpass-bold-italic.woff') format('woff');
+   src: url('../fonts/overpass-bold-italic.woff') format('woff');
font-weight: bold;
font-style: italic;
 }
 
 @font-face {
font-family: 'LibvirtOverpassLight';
-   src: url('overpass-light.woff') format('woff');
+   src: url('../fonts/overpass-light.woff') format('woff');
font-weight: 300;
font-style: normal;
 }
 
 @font-face {
font-family: 'LibvirtOverpassLight';
-   src: url('overpass-light-italic.woff') format('woff');
+   src: url('../fonts/overpass-light-italic.woff') format('woff');
font-weight: 300;
font-style: italic;
 }
 
 @font-face {
font-family: 'LibvirtOverpassMono';
-   src: url('overpass-mono-regular.woff') format('woff');
+   src: url('../fonts/overpass-mono-regular.woff') format('woff');
font-weight: normal;
font-style: normal;
 }
 
 @font-face {
font-family: 'LibvirtOverpassMono';
-   src: url('overpass-mono-bold.woff') format('woff');
+   src: url('../fonts/overpass-mono-bold.woff') format('woff');
font-weight: bold;
font-style: normal;
 }
 
 @font-face {
font-family: 'LibvirtOverpassMonoLight';
-   src: url('overpass-mono-light.woff') format('woff');
+   src: url('../fonts/overpass-mono-light.woff') format('woff');
font-weight: 300;
font-style: normal;
 }
diff --git a/docs/css/main.css b/docs/css/main.css
index 8961f1a4b4..88e453aca6 100644
--- a/docs/css/main.css
+++ b/docs/css/main.css
@@ -1,4 +1,4 @@
-@import url(../fonts/stylesheet.css);
+@import url(fonts.css);
 @import url(generic.css);
 @import url(libvirt.css);
 @import url(mobile.css);
diff --git a/docs/css/meson.build b/docs/css/meson.build
index 35e56347a6..384f6e789f 100644
--- a/docs/css/meson.build
+++ b/docs/css/meson.build
@@ -1,4 +1,5 @@
 docs_css_files = [
+  'fonts.css',
   'generic.css',
   'libvirt.css',
   'main.css',
diff --git a/docs/fonts/meson.build b/docs/fonts/meson.build
index e4109c6e7d..53a060b972 100644
--- a/docs/fonts/meson.build
+++ b/docs/fonts/meson.build
@@ -1,6 +1,5 @@
 fonts = [
   'LICENSE.rst',
-  'stylesheet.css',
   'overpass-bold-italic.woff',
   'overpass-bold.woff',
   'overpass-italic.woff',
-- 
2.31.1

[libvirt PATCH v3 1/4] docs: Drop structures.svg

[Andrea Bolognani]

It was introduced in ff4ede005567 but it doesn't seem to have
ever actually been used anywhere.

Signed-off-by: Andrea Bolognani 
Reviewed-by: Ani Sinha 
---
 docs/structures.svg | 187 
 1 file changed, 187 deletions(-)
 delete mode 100644 docs/structures.svg

diff --git a/docs/structures.svg b/docs/structures.svg
deleted file mode 100644
index e52d606ae3..00
--- a/docs/structures.svg
+++ /dev/null
@@ -1,187 +0,0 @@
-
-
-
-
-http://www.w3.org/2000/svg;
-   xmlns:xlink="http://www.w3.org/1999/xlink;
-   width="458pt" height="312pt"
-   viewBox="885 525 7628 5190">
-
-
-
-
-
-
-   
-
-
-
-
-
-
-
-
-   
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-   
-
-
-
-
-
-
-
-
-   
-
-
-
-
-
-
-
-
-   
-
-
-
-
-
-
-
-
-   
-
-
-
-
-
-
-
-
-   
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-   
-
-
-
-
-
-
-
-
-   
-
-
-
-
-
-
-
-
-   
-
-
-
-
-
-
-Domains
-
-hash 
-
-virDomainPtr
-
-virConnectionPtr
-
-domains
-
-conn
-
-name
-
-"Domain-0"
-
-"fc4"
-
-table
-
-name
-
-based on
-
-ids
-
-
-- 
2.31.1

[libvirt PATCH v3 0/4] docs: Unclutter top-level directory

[Andrea Bolognani]

Changes from [v2]:

  * drop all changes related to icons.

Changes from [v1]:

  * extend scope to include icons and CSS files.

[v2] https://listman.redhat.com/archives/libvir-list/2021-December/msg00828.html
[v1] https://listman.redhat.com/archives/libvir-list/2021-December/msg00818.html

Andrea Bolognani (4):
  docs: Drop structures.svg
  docs: Move all images to a subdirectory
  docs: Move all CSS files to a subdirectory
  docs: Move font definitions with other CSS files

 docs/api.rst  |   6 +-
 docs/{fonts/stylesheet.css => css/fonts.css}  |  18 +-
 docs/{ => css}/generic.css|   0
 docs/{ => css}/libvirt.css|   4 +-
 docs/{ => css}/main.css   |   2 +-
 docs/css/meson.build  |  17 ++
 docs/{ => css}/mobile.css |   2 +-
 docs/fonts/meson.build|   1 -
 docs/goals.html.in|   2 +-
 docs/{ => images}/event_loop_simple.png   | Bin
 docs/{ => images}/event_loop_simple.svg   |   0
 docs/{ => images}/event_loop_worker.png   | Bin
 docs/{ => images}/event_loop_worker.svg   |   0
 docs/{ => images}/libvirt-daemon-arch.png | Bin
 docs/{ => images}/libvirt-daemon-arch.svg |   0
 docs/{ => images}/libvirt-driver-arch.png | Bin
 docs/{ => images}/libvirt-driver-arch.svg |   0
 docs/{ => images}/libvirt-object-model.png| Bin
 docs/{ => images}/libvirt-object-model.svg|   0
 .../libvirt-virConnect-example.png| Bin
 .../libvirt-virConnect-example.svg|   0
 docs/images/meson.build   |  24 +++
 .../{ => images}/migration-managed-direct.png | Bin
 .../{ => images}/migration-managed-direct.svg |   0
 docs/{ => images}/migration-managed-p2p.png   | Bin
 docs/{ => images}/migration-managed-p2p.svg   |   0
 docs/{ => images}/migration-native.png| Bin
 docs/{ => images}/migration-native.svg|   0
 docs/{ => images}/migration-tunnel.png| Bin
 docs/{ => images}/migration-tunnel.svg|   0
 .../migration-unmanaged-direct.png| Bin
 .../migration-unmanaged-direct.svg|   0
 docs/{ => images}/node.gif| Bin
 docs/{ => images}/node.svg|   0
 docs/internals.html.in|   2 +-
 docs/internals/eventloop.html.in  |   4 +-
 docs/meson.build  |  18 +-
 docs/migration.rst|  10 +-
 docs/page.xsl |   2 +-
 docs/structures.svg   | 187 --
 40 files changed, 69 insertions(+), 230 deletions(-)
 rename docs/{fonts/stylesheet.css => css/fonts.css} (60%)
 rename docs/{ => css}/generic.css (100%)
 rename docs/{ => css}/libvirt.css (98%)
 rename docs/{ => css}/main.css (68%)
 create mode 100644 docs/css/meson.build
 rename docs/{ => css}/mobile.css (95%)
 rename docs/{ => images}/event_loop_simple.png (100%)
 rename docs/{ => images}/event_loop_simple.svg (100%)
 rename docs/{ => images}/event_loop_worker.png (100%)
 rename docs/{ => images}/event_loop_worker.svg (100%)
 rename docs/{ => images}/libvirt-daemon-arch.png (100%)
 rename docs/{ => images}/libvirt-daemon-arch.svg (100%)
 rename docs/{ => images}/libvirt-driver-arch.png (100%)
 rename docs/{ => images}/libvirt-driver-arch.svg (100%)
 rename docs/{ => images}/libvirt-object-model.png (100%)
 rename docs/{ => images}/libvirt-object-model.svg (100%)
 rename docs/{ => images}/libvirt-virConnect-example.png (100%)
 rename docs/{ => images}/libvirt-virConnect-example.svg (100%)
 create mode 100644 docs/images/meson.build
 rename docs/{ => images}/migration-managed-direct.png (100%)
 rename docs/{ => images}/migration-managed-direct.svg (100%)
 rename docs/{ => images}/migration-managed-p2p.png (100%)
 rename docs/{ => images}/migration-managed-p2p.svg (100%)
 rename docs/{ => images}/migration-native.png (100%)
 rename docs/{ => images}/migration-native.svg (100%)
 rename docs/{ => images}/migration-tunnel.png (100%)
 rename docs/{ => images}/migration-tunnel.svg (100%)
 rename docs/{ => images}/migration-unmanaged-direct.png (100%)
 rename docs/{ => images}/migration-unmanaged-direct.svg (100%)
 rename docs/{ => images}/node.gif (100%)
 rename docs/{ => images}/node.svg (100%)
 delete mode 100644 docs/structures.svg

-- 
2.31.1

[PATCH 7/8] Dispatch error in virInitialize

[Martin Kletzander]

Callers that already do this anyway can be cleaned up thanks to this and the one
that does not (daemon startup) gains the benefit of the error being printed to
standard error output changing:

LIBVIRT_LOG_OUTPUTS=1:invalid libvirtd
/home/nert/dev/libvirt/upstream/build/src/libvirtd: initialisation failed

into:

LIBVIRT_LOG_OUTPUTS=1:invalid libvirtd
libvirt:  error : invalid argument: Invalid destination 'invalid' for output 
'1:invalid'
/home/nert/dev/libvirt/upstream/build/src/libvirtd: initialisation failed

Signed-off-by: Martin Kletzander 
---
 src/libvirt.c | 32 +++-
 1 file changed, 11 insertions(+), 21 deletions(-)

diff --git a/src/libvirt.c b/src/libvirt.c
index 35fd74fe08da..45315f484c97 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -309,11 +309,12 @@ virGlobalInit(void)
 int
 virInitialize(void)
 {
-if (virOnce(, virGlobalInit) < 0)
+if (virOnce(, virGlobalInit) < 0 ||
+virGlobalError) {
+virDispatchError(NULL);
 return -1;
+}
 
-if (virGlobalError)
-return -1;
 return 0;
 }
 
@@ -1200,18 +1201,15 @@ virConnectOpen(const char *name)
 virConnectPtr ret = NULL;
 
 if (virInitialize() < 0)
-goto error;
+return NULL;
 
 VIR_DEBUG("name=%s", NULLSTR(name));
 virResetLastError();
 ret = virConnectOpenInternal(name, NULL, 0);
 if (!ret)
-goto error;
-return ret;
+virDispatchError(NULL);
 
- error:
-virDispatchError(NULL);
-return NULL;
+return ret;
 }
 
 
@@ -1236,18 +1234,14 @@ virConnectOpenReadOnly(const char *name)
 virConnectPtr ret = NULL;
 
 if (virInitialize() < 0)
-goto error;
+return NULL;
 
 VIR_DEBUG("name=%s", NULLSTR(name));
 virResetLastError();
 ret = virConnectOpenInternal(name, NULL, VIR_CONNECT_RO);
 if (!ret)
-goto error;
+virDispatchError(NULL);
 return ret;
-
- error:
-virDispatchError(NULL);
-return NULL;
 }
 
 
@@ -1276,18 +1270,14 @@ virConnectOpenAuth(const char *name,
 virConnectPtr ret = NULL;
 
 if (virInitialize() < 0)
-goto error;
+return NULL;
 
 VIR_DEBUG("name=%s, auth=%p, flags=0x%x", NULLSTR(name), auth, flags);
 virResetLastError();
 ret = virConnectOpenInternal(name, auth, flags);
 if (!ret)
-goto error;
+virDispatchError(NULL);
 return ret;
-
- error:
-virDispatchError(NULL);
-return NULL;
 }
 
 
-- 
2.34.1

[PATCH 0/8] Fix an unfortunate deadlock

[Martin Kletzander]

Before this series:

# LIBVIRT_LOG_OUTPUTS=1:asdf:fdsa:meh libvirtd


After this series:

# LIBVIRT_LOG_OUTPUTS=1:asdf:fdsa:meh libvirtd
libvirt:  error : invalid argument: Invalid destination 'asdf' for output 
'1:asdf:fdsa:meh'


Martin Kletzander (8):
  util: Report error in virLogParseDefaultPriority
  util: Do not hide errors in virLogSetDefaultOutput
  util: Report error in virLogSetDefaultOutputToFile
  util: Initialize virLogMutex statically
  Exit on errors from virDaemonSetupLogging
  util: Check for errors in virLogSetFromEnv
  Dispatch error in virInitialize
  Do not print error in remote_daemon.c:main

 src/admin/libvirt-admin.c  |  3 ++-
 src/libvirt.c  | 35 ++-
 src/locking/lock_daemon.c  | 15 ++--
 src/logging/log_daemon.c   | 15 ++--
 src/lxc/lxc_controller.c   |  3 ++-
 src/remote/remote_daemon.c | 19 +++
 src/remote/remote_ssh_helper.c |  3 ++-
 src/security/virt-aa-helper.c  |  3 ++-
 src/util/vircommand.c  |  3 ++-
 src/util/virdaemon.c   | 34 --
 src/util/virdaemon.h   | 14 +--
 src/util/virlog.c  | 44 ++
 src/util/virlog.h  |  4 ++--
 tests/testutils.c  |  4 +++-
 14 files changed, 111 insertions(+), 88 deletions(-)

-- 
2.34.1

[PATCH 8/8] Do not print error in remote_daemon.c:main

[Martin Kletzander]

There is no need to do that since both fallible functions do that already.

Signed-off-by: Martin Kletzander 
---
 src/remote/remote_daemon.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
index 8a4610da83c8..84157e6cc19a 100644
--- a/src/remote/remote_daemon.c
+++ b/src/remote/remote_daemon.c
@@ -826,10 +826,8 @@ int main(int argc, char **argv) {
 };
 
 if (virGettextInitialize() < 0 ||
-virInitialize() < 0) {
-fprintf(stderr, _("%s: initialization failed\n"), argv[0]);
+virInitialize() < 0)
 exit(EXIT_FAILURE);
-}
 
 virUpdateSelfLastChanged(argv[0]);
 
-- 
2.34.1

[PATCH 1/8] util: Report error in virLogParseDefaultPriority

[Martin Kletzander]

Signed-off-by: Martin Kletzander 
---
 src/util/virlog.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/util/virlog.c b/src/util/virlog.c
index ac35e36148c6..90d3d7c5cb53 100644
--- a/src/util/virlog.c
+++ b/src/util/virlog.c
@@ -1187,6 +1187,10 @@ virLogParseDefaultPriority(const char *priority)
 return VIR_LOG_WARN;
 else if (STREQ(priority, "4") || STREQ(priority, "error"))
 return VIR_LOG_ERROR;
+
+virReportError(VIR_ERR_INVALID_ARG,
+   _("Failed to set logging priority, argument '%s' is "
+ "invalid"), priority);
 return -1;
 }
 
-- 
2.34.1

[PATCH 6/8] util: Check for errors in virLogSetFromEnv

[Martin Kletzander]

And make callers check the return value as well.  This helps error out early for
invalid environment variables.

That is desirable because it could lead to deadlocks.  This can happen when
resetting logging after fork() reports translated errors because gettext
functions are not reentrant.  Well, it is not limited to resetting logging after
fork(), it can be any translation at that phase, but parsing environment
variables is easy to make fail on purpose to show the result, it can also happen
just due to a typo.  Logging settings are also something that we want to report
errors on for example when it is being done over admin API.

Before this commit it is possible to deadlock the daemon on startup
with something like:

LIBVIRT_LOG_FILTERS='1:*' LIBVIRT_LOG_OUTPUTS=1:stdout libvirtd

where filters are used to enable more logging and hence make the race less rare
and outputs are set to invalid

Combined with the previous patches this changes
the following from:

...


to:

...
libvirtd: initialisation failed

The error message is improved in future commits and is also possible thanks to
this patch.

Signed-off-by: Martin Kletzander 
---
 src/admin/libvirt-admin.c  |  3 ++-
 src/libvirt.c  |  3 ++-
 src/lxc/lxc_controller.c   |  3 ++-
 src/remote/remote_ssh_helper.c |  3 ++-
 src/security/virt-aa-helper.c  |  3 ++-
 src/util/vircommand.c  |  3 ++-
 src/util/virdaemon.c   |  3 ++-
 src/util/virlog.c  | 26 ++
 src/util/virlog.h  |  2 +-
 tests/testutils.c  |  4 +++-
 10 files changed, 36 insertions(+), 17 deletions(-)

diff --git a/src/admin/libvirt-admin.c b/src/admin/libvirt-admin.c
index a3164a2395cf..01546a7bc270 100644
--- a/src/admin/libvirt-admin.c
+++ b/src/admin/libvirt-admin.c
@@ -55,7 +55,8 @@ virAdmGlobalInit(void)
 if (virErrorInitialize() < 0)
 goto error;
 
-virLogSetFromEnv();
+if (virLogSetFromEnv() < 0)
+goto error;
 
 #ifdef WITH_LIBINTL_H
 if (!bindtextdomain(PACKAGE, LOCALEDIR))
diff --git a/src/libvirt.c b/src/libvirt.c
index ef9fc403d083..35fd74fe08da 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -232,7 +232,8 @@ virGlobalInit(void)
 goto error;
 }
 
-virLogSetFromEnv();
+if (virLogSetFromEnv() < 0)
+goto error;
 
 virNetTLSInit();
 
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 67ebed361ae0..3c930eaacdee 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -2503,7 +2503,8 @@ int main(int argc, char *argv[])
 }
 
 /* Initialize logging */
-virLogSetFromEnv();
+if (virLogSetFromEnv() < 0)
+exit(EXIT_FAILURE);
 
 while (1) {
 int c;
diff --git a/src/remote/remote_ssh_helper.c b/src/remote/remote_ssh_helper.c
index 4f4dbff7d0ad..78f02020a745 100644
--- a/src/remote/remote_ssh_helper.c
+++ b/src/remote/remote_ssh_helper.c
@@ -402,7 +402,8 @@ int main(int argc, char **argv)
 virFileActivateDirOverrideForProg(argv[0]);
 
 /* Initialize the log system */
-virLogSetFromEnv();
+if (virLogSetFromEnv() < 0)
+exit(EXIT_FAILURE);
 
 uri_str = argv[1];
 VIR_DEBUG("Using URI %s", uri_str);
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index b7ffb5e2c324..28717b7e38b8 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -1449,7 +1449,8 @@ main(int argc, char **argv)
 virFileActivateDirOverrideForProg(argv[0]);
 
 /* Initialize the log system */
-virLogSetFromEnv();
+if (virLogSetFromEnv() < 0)
+exit(EXIT_FAILURE);
 
 /* clear the environment */
 environ = NULL;
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index ba5a2090768b..3b8c1c65c160 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -751,7 +751,8 @@ virExec(virCommand *cmd)
 VIR_FORCE_CLOSE(null);
 
 /* Initialize full logging for a while */
-virLogSetFromEnv();
+if (virLogSetFromEnv() < 0)
+goto fork_error;
 
 if (cmd->pidfile &&
 virPipe(pipesync) < 0)
diff --git a/src/util/virdaemon.c b/src/util/virdaemon.c
index 795206301227..5c0ca534cf16 100644
--- a/src/util/virdaemon.c
+++ b/src/util/virdaemon.c
@@ -183,7 +183,8 @@ virDaemonSetupLogging(const char *daemon_name,
 return -1;
 
 /* If there are some environment variables defined, use those instead */
-virLogSetFromEnv();
+if (virLogSetFromEnv() < 0)
+return -1;
 
 /*
  * Command line override for --verbose
diff --git a/src/util/virlog.c b/src/util/virlog.c
index 5848940c6ccb..71913a2997f4 100644
--- a/src/util/virlog.c
+++ b/src/util/virlog.c
@@ -1200,24 +1200,34 @@ virLogParseDefaultPriority(const char *priority)
  *
  * Sets virLogDefaultPriority, virLogFilters and virLogOutputs based on
  * environment variables.
+ *
+ * This function might fail which should also make the caller fail.
  */
-void
+int
 virLogSetFromEnv(void)
 {

[PATCH 5/8] Exit on errors from virDaemonSetupLogging

[Martin Kletzander]

This prevents starting any daemons with improper logging settings.  This is
desirable on its own, but will be even more beneficial when more functions start
reporting errors and failing on them, coming up in following patches

Signed-off-by: Martin Kletzander 
---
 src/locking/lock_daemon.c  | 15 ---
 src/logging/log_daemon.c   | 15 ---
 src/remote/remote_daemon.c | 15 ---
 src/util/virdaemon.c   | 31 ---
 src/util/virdaemon.h   | 14 +++---
 5 files changed, 51 insertions(+), 39 deletions(-)

diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index 107fb22bc2c9..ea81940a4325 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -913,13 +913,14 @@ int main(int argc, char **argv) {
 }
 VIR_FREE(remote_config_file);
 
-virDaemonSetupLogging("virtlockd",
-  config->log_level,
-  config->log_filters,
-  config->log_outputs,
-  privileged,
-  verbose,
-  godaemon);
+if (virDaemonSetupLogging("virtlockd",
+  config->log_level,
+  config->log_filters,
+  config->log_outputs,
+  privileged,
+  verbose,
+  godaemon) < 0)
+exit(EXIT_FAILURE);
 
 if (!pid_file &&
 virPidFileConstructPath(privileged,
diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c
index de6bf082e89b..fe7fa8534aec 100644
--- a/src/logging/log_daemon.c
+++ b/src/logging/log_daemon.c
@@ -719,13 +719,14 @@ int main(int argc, char **argv) {
 exit(EXIT_FAILURE);
 }
 
-virDaemonSetupLogging("virtlogd",
-  config->log_level,
-  config->log_filters,
-  config->log_outputs,
-  privileged,
-  verbose,
-  godaemon);
+if (virDaemonSetupLogging("virtlogd",
+  config->log_level,
+  config->log_filters,
+  config->log_outputs,
+  privileged,
+  verbose,
+  godaemon) < 0)
+exit(EXIT_FAILURE);
 
 if (!pid_file &&
 virPidFileConstructPath(privileged,
diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
index 4e10f3ad2307..8a4610da83c8 100644
--- a/src/remote/remote_daemon.c
+++ b/src/remote/remote_daemon.c
@@ -936,13 +936,14 @@ int main(int argc, char **argv) {
 exit(EXIT_FAILURE);
 }
 
-virDaemonSetupLogging(DAEMON_NAME,
-  config->log_level,
-  config->log_filters,
-  config->log_outputs,
-  privileged,
-  verbose,
-  godaemon);
+if (virDaemonSetupLogging(DAEMON_NAME,
+  config->log_level,
+  config->log_filters,
+  config->log_outputs,
+  privileged,
+  verbose,
+  godaemon) < 0)
+exit(EXIT_FAILURE);
 
 /* Let's try to initialize global variable that holds the host's boot 
time. */
 if (virHostBootTimeInit() < 0) {
diff --git a/src/util/virdaemon.c b/src/util/virdaemon.c
index bb2df2eb2cab..795206301227 100644
--- a/src/util/virdaemon.c
+++ b/src/util/virdaemon.c
@@ -151,7 +151,7 @@ virDaemonForkIntoBackground(const char *argv0)
  * but if verbose or error debugging is asked for then also output
  * informational and debug messages. Default size if 64 kB.
  */
-void
+int
 virDaemonSetupLogging(const char *daemon_name,
   unsigned int log_level,
   char *log_filters,
@@ -160,7 +160,8 @@ virDaemonSetupLogging(const char *daemon_name,
   bool verbose,
   bool godaemon)
 {
-virLogReset();
+if (virLogReset() < 0)
+return -1;
 
 /*
  * Libvirtd's order of precedence is:
@@ -169,15 +170,17 @@ virDaemonSetupLogging(const char *daemon_name,
  * Given the precedence, we must process the variables in the opposite
  * order, each one overriding the previous.
  */
-if (log_level != 0)
-virLogSetDefaultPriority(log_level);
+if (log_level != 0 &&
+virLogSetDefaultPriority(log_level) < 0)
+return -1;
 
 /* In case the config is empty, both filters and outputs will become empty,
  * however we can't start with empty outputs, thus we'll need to define and
  * setup a default one.
  */
-ignore_value(virLogSetFilters(log_filters));
-

[PATCH 4/8] util: Initialize virLogMutex statically

[Martin Kletzander]

The only difference is that we are not going to be guaranteed that the mutex is
normal (as opposed to recursive, although there is no system known to me that
would default to recursive mutexes), but that was done only to find occasional
errors (during runtime, back in 2010, commit 336fd879c00b).  Functions using
this mutex are mostly stable and unchanging, and it makes the virLogOnceInit()
function only return 0 (or possibly abort in glib calls).  On top of that we can
assume that the virLogMutex is always initialized which enables us to be more
consistent in some early error reporting.

Signed-off-by: Martin Kletzander 
---
 src/util/virlog.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/src/util/virlog.c b/src/util/virlog.c
index e368cada6024..5848940c6ccb 100644
--- a/src/util/virlog.c
+++ b/src/util/virlog.c
@@ -125,7 +125,7 @@ static void virLogOutputToFd(virLogSource *src,
 /*
  * Logs accesses must be serialized though a mutex
  */
-virMutex virLogMutex;
+static virMutex virLogMutex = VIR_MUTEX_INITIALIZER;
 
 void
 virLogLock(void)
@@ -250,9 +250,6 @@ virLogPriorityString(virLogPriority lvl)
 static int
 virLogOnceInit(void)
 {
-if (virMutexInit() < 0)
-return -1;
-
 virLogLock();
 virLogDefaultPriority = VIR_LOG_DEFAULT;
 
-- 
2.34.1

[PATCH 2/8] util: Do not hide errors in virLogSetDefaultOutput

[Martin Kletzander]

Signed-off-by: Martin Kletzander 
---
 src/util/virlog.c | 8 +---
 src/util/virlog.h | 2 +-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/util/virlog.c b/src/util/virlog.c
index 90d3d7c5cb53..bf791d901a24 100644
--- a/src/util/virlog.c
+++ b/src/util/virlog.c
@@ -201,7 +201,7 @@ virLogSetDefaultOutputToFile(const char *binary, bool 
privileged)
  * according to @binary, @godaemon, @privileged. This function should be run
  * exactly once at daemon startup, so no locks are used.
  */
-void
+int
 virLogSetDefaultOutput(const char *binary, bool godaemon, bool privileged)
 {
 bool have_journald = access("/run/systemd/journal/socket", W_OK) >= 0;
@@ -209,14 +209,16 @@ virLogSetDefaultOutput(const char *binary, bool godaemon, 
bool privileged)
 if (godaemon) {
 if (have_journald)
 virLogSetDefaultOutputToJournald();
-else
-virLogSetDefaultOutputToFile(binary, privileged);
+else if (virLogSetDefaultOutputToFile(binary, privileged) < 0)
+return -1;
 } else {
 if (!isatty(STDIN_FILENO) && have_journald)
 virLogSetDefaultOutputToJournald();
 else
 virLogSetDefaultOutputToStderr();
 }
+
+return 0;
 }
 
 
diff --git a/src/util/virlog.h b/src/util/virlog.h
index 460e54ba0501..a04811e4083c 100644
--- a/src/util/virlog.h
+++ b/src/util/virlog.h
@@ -154,7 +154,7 @@ void virLogFilterListFree(virLogFilter **list, int count);
 int virLogSetOutputs(const char *outputs);
 int virLogSetFilters(const char *filters);
 char *virLogGetDefaultOutput(void);
-void virLogSetDefaultOutput(const char *fname, bool godaemon, bool privileged);
+int virLogSetDefaultOutput(const char *fname, bool godaemon, bool privileged);
 
 /*
  * Internal logging API
-- 
2.34.1

[PATCH 3/8] util: Report error in virLogSetDefaultOutputToFile

[Martin Kletzander]

Signed-off-by: Martin Kletzander 
---
 src/util/virlog.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/util/virlog.c b/src/util/virlog.c
index bf791d901a24..e368cada6024 100644
--- a/src/util/virlog.c
+++ b/src/util/virlog.c
@@ -178,6 +178,7 @@ virLogSetDefaultOutputToFile(const char *binary, bool 
privileged)
 old_umask = umask(077);
 if (g_mkdir_with_parents(logdir, 0777) < 0) {
 umask(old_umask);
+virReportSystemError(errno, "%s", _("Could not create log 
directory"));
 return -1;
 }
 umask(old_umask);
-- 
2.34.1

Re: [libvirt PATCH v2 4/6] docs: Move all icons to a subdirectory

[Andrea Bolognani]

On Tue, Jan 04, 2022 at 12:07:59PM +, Daniel P. Berrangé wrote:
> On Tue, Dec 21, 2021 at 04:30:31PM +0100, Andrea Bolognani wrote:
> > This unclutters the top-level docs directory.
> >
> > Signed-off-by: Andrea Bolognani 
> > ---
> >  docs/browserconfig.xml  |   2 +-
> >  docs/{ => icons}/android-chrome-192x192.png | Bin
> >  docs/{ => icons}/android-chrome-256x256.png | Bin
> >  docs/{ => icons}/apple-touch-icon.png   | Bin
> >  docs/{ => icons}/favicon-16x16.png  | Bin
> >  docs/{ => icons}/favicon-32x32.png  | Bin
> >  docs/{ => icons}/favicon.ico| Bin
> >  docs/icons/meson.build  |  19 +++
> >  docs/{ => icons}/mstile-150x150.png | Bin
>
> Please don't move any of the favicon related images. These were placed
> at the root intentionally per the recommendations of the favicon
> generator we used for better client compatibility
>
> https://realfavicongenerator.net/faq

Okay, I'll respin without the favicon changes.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: .conf file setting(s) for packet filtering backend(s)

[Daniel P . Berrangé]

On Sun, Jan 02, 2022 at 09:41:37PM -0500, Laine Stump wrote:
> I'm currently working on switching the backend of the network driver from
> using iptables to using nftables. Due to some functionality that is not
> available with nftables (the rule that fixes up the checksum of DHCP packets
> which, btw, is only relevant for *very* old guests, e.g. RHEL5), this needs
> to be opt-in via a config file setting. In the meantime, in order to make
> this doable in a reasonable amount of time, I am *not* converting the
> nwfilter driver right away, and when I do it will need its own config file
> setting for opt-in.
> 
> I've never before looked at the code for the .conf file settings at all. I
> had assumed there would be some sort of "pull" API, where code in the
> drivers could call, e.g. virConfGetString("filter_backend") and it would
> return the config setting to the caller. But when I look at it, I see that
> all daemons use the same daemonConfigLoadFile() called from
> remote_daemon.c:main() (which is shared by all the daemons) and the
> daemonConfig object that is created to hold the config settings that are
> read is only visible within main() - the only way that a config setting is
> used is by main() "pushing" it out to a static variable somewhere else where
> it is later retrieved by the interested party, e.g. the way that main()
> calls daemonSetupNetDevOpenvswitch(config), which then sets the static
> virNetDevOpenvswitchTimeout in util/virnetdevopenvswitch.c.

I'd consider the OVS approach to be a bad example

Global state needing configurable parameters for stuff in util/ should
generally be considered a design flaw.  Global state should be exclusively
in the drivers, and then the desired values passed into the util APIs
explicitly.

ie  ovs_timeout should have been in qemu.conf (any other drivers' config
files if appropriate).

> (NB: util/virnetdevopenvswitch.c is linked into every deamon, so even for
> the daemons that don't use it, calls to virnetdevopenvswitch.c functions
> still compile properly (and calling them is harmless), so
> virNetDevOpenvswitchTimeout is set even for daemons that never call
> openvswitch APIs).

This is another bit of technical debt. We've been lazy with putting
stuff into util that really ought not to be there.

This stuff even gets into the libvirt.so that's used client side,
so the argument that we had a single monolithic libvirtd didn't
apply either.



> If I could count on all builds using split daemons (i.e. separate
> virtnetworkd and virtnwfilterd) then I could add a similar API in
> virfirewall.c that remote_daemon.c:main() could use to set "filter_backend"
> into a static in virfirewall.c (which is used by both drivers) and
> everything would just happily work:
> 
>virtnetworkd.conf:
>   filter_backend = nftables
> 
>virtnwfilterd.conf
>   filter_backend = iptables

Putting these settings into virtnetworkd.conf and virtnwfilterd.conf
certainly makes conceptual sense.

The problem you mention is avoided by not having global state in
virtfirewall.c. Just pass the setting into every API call whuere it
is relevant.

> However, I need to also deal with the possibility that the nwfilter and
> network drivers are in the same unified libvirtd binary, and in that case
> both drivers would have the same virfirewall.c:filter_backend setting, thus
> making it impossible to use the iptables backend for the nwfilter driver and
> nftables backend for the network driver. For that case I would need separate
> settings in the config for each driver, e.g.
> 
>libvirtd.conf:
>   network_filter_backend = nftables
>   nwfilter_backend = iptables

Definitely don't want this, as its just follwing thue mistake we did
with ovs.

> So should I perhaps declare the nftables backend for nwfilter to be a lost
> cause until everyone moves to split daemons, add a "filter_backend" setting
> that is directly set in virfirewall.c (by remote_daemon.c:main()), and then
> provide some sort of override in virFirewallNew so calls from the nwfilter
> driver can say "ignore the filter_backend setting and use iptables"?

I'm wondering how you're integrating nftables into virfirewall in
general ?

Currently we just have

VIR_FIREWALL_LAYER_ETHERNET,
VIR_FIREWALL_LAYER_IPV4,
VIR_FIREWALL_LAYER_IPV6,


which get mapped to ebtables, iptables and ip6tables internally.
Previously they could also get mapped to firewalld but we removed
that. This worked because both firewalld passthrough and the
native commands took the same syntax, so the choice of backends
was transparent to the caller.

Now with use of nftables, we have completely different syntax
for adding rules. IOW, the caller needs to decide which backend
to use, in order to decide what syntax to use with
virFirewallAddRule.

IIUC, with nftables there is no split between ethernet, ipv4
and ipv6 filtering. This makes the VIR_FIREWALL_LAYER_*
enum somewhat redundant/inappropriate as a high level
conceptual 

Re: [PATCH v3] Add VM info to improve error log message for qemu monitor

[Ani Sinha]

On Tue, 4 Jan 2022, Rohit Kumar wrote:

> This change adds the domain name in the error and debug logs during
> monitor IO processing so that we may infer which VM experienced
> errors such as IO or socket hangup. This may help in debugging
> monitor IO errors.

LGTM. If you wish you can add a comment why the new member domainName in
qemuMonitor stuct was essential.

>
> Signed-off-by: Rohit Kumar 

Reviewed-by: Ani Sinha 

> ---
>  src/qemu/qemu_monitor.c | 36 +---
>  1 file changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index dda6ae9796..2b4582578a 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -80,6 +80,7 @@ struct _qemuMonitor {
>  GSource *watch;
>
>  virDomainObj *vm;
> +char *domainName;
>
>  qemuMonitorCallbacks *cb;
>  void *callbackOpaque;
> @@ -243,6 +244,7 @@ qemuMonitorDispose(void *obj)
>  virCondDestroy(>notify);
>  g_free(mon->buffer);
>  g_free(mon->balloonpath);
> +g_free(mon->domainName);
>  }
>
>
> @@ -583,8 +585,8 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
>
>  if (!error && !mon->goteof &&
>  cond & G_IO_ERR) {
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -   _("Invalid file descriptor while waiting for 
> monitor"));
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Invalid file descriptor while waiting for 
> monitor (vm='%s')"), mon->domainName);
>  mon->goteof = true;
>  }
>  }
> @@ -609,13 +611,14 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
>  virResetLastError();
>  } else {
>  if (virGetLastErrorCode() == VIR_ERR_OK && !mon->goteof)
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -   _("Error while processing monitor IO"));
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Error while processing monitor IO 
> (vm='%s')"), mon->domainName);
>  virCopyLastError(>lastError);
>  virResetLastError();
>  }
>
> -VIR_DEBUG("Error on monitor %s", NULLSTR(mon->lastError.message));
> +VIR_DEBUG("Error on monitor %s mon=%p vm=%p name=%s",
> +  NULLSTR(mon->lastError.message), mon, mon->vm, 
> mon->domainName);
>  /* If IO process resulted in an error & we have a message,
>   * then wakeup that waiter */
>  if (mon->msg && !mon->msg->finished) {
> @@ -636,7 +639,8 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
>  /* Make sure anyone waiting wakes up now */
>  virCondSignal(>notify);
>  virObjectUnlock(mon);
> -VIR_DEBUG("Triggering EOF callback");
> +VIR_DEBUG("Triggering EOF callback mon=%p vm=%p name=%s",
> +  mon, mon->vm, mon->domainName);
>  (eofNotify)(mon, vm, mon->callbackOpaque);
>  virObjectUnref(mon);
>  } else if (error) {
> @@ -646,7 +650,8 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
>  /* Make sure anyone waiting wakes up now */
>  virCondSignal(>notify);
>  virObjectUnlock(mon);
> -VIR_DEBUG("Triggering error callback");
> +VIR_DEBUG("Triggering error callback mon=%p vm=%p name=%s",
> +  mon, mon->vm, mon->domainName);
>  (errorNotify)(mon, vm, mon->callbackOpaque);
>  virObjectUnref(mon);
>  } else {
> @@ -694,6 +699,7 @@ qemuMonitorOpenInternal(virDomainObj *vm,
>  mon->fd = fd;
>  mon->context = g_main_context_ref(context);
>  mon->vm = virObjectRef(vm);
> +mon->domainName = g_strdup(NULLSTR(vm->def->name));
>  mon->waitGreeting = true;
>  mon->cb = cb;
>  mon->callbackOpaque = opaque;
> @@ -935,14 +941,14 @@ qemuMonitorSend(qemuMonitor *mon,
>
>  /* Check whether qemu quit unexpectedly */
>  if (mon->lastError.code != VIR_ERR_OK) {
> -VIR_DEBUG("Attempt to send command while error is set %s",
> -  NULLSTR(mon->lastError.message));
> +VIR_DEBUG("Attempt to send command while error is set %s mon=%p 
> vm=%p name=%s",
> +  NULLSTR(mon->lastError.message), mon, mon->vm, 
> mon->domainName);
>  virSetError(>lastError);
>  return -1;
>  }
>  if (mon->goteof) {
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -   _("End of file from qemu monitor"));
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("End of file from qemu monitor (vm='%s')"), 
> mon->domainName);
>  return -1;
>  }
>
> @@ -955,15 +961,15 @@ qemuMonitorSend(qemuMonitor *mon,
>
>  while (!mon->msg->finished) {
>  if (virCondWait(>notify, >parent.lock) < 0) {
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -   _("Unable to wait on monitor 

Re: [PATCH 2/2] virnettlscontext: Don't pass static key length to gnutls_dh_params_generate2()

[Michal Prívozník]

On 1/4/22 12:59, Daniel P. Berrangé wrote:
> We shouldn't be introducing use of gnutls_sec_param_to_pk_bits at
> all IMHO, rather we should be removing use of gnutls_dh_params_generate2
> instead.
> 
> The recommendation is to use pre-generated DH parameters from the
> the FFDHE set of RFC7919.
> 
> In gnutls >= 3.6.0 this happens automatically.
> 
> In gnutls >= 3.5.6 && < 3.6.0 we can replace thegnutls_dh_params_generate2 +
> gnutls_certificate_set_dh_params pair of calls, with just
> gnutls_certificate_set_known_dh_params()

Fair enough, I don't know enough about gnutls, but let me see if I can
cook a patch.

Michal

[PATCH 1/2] docs: Clarify 'burst' units for QoS

[Michal Privoznik]

The burst attribute for bandwidth specifies how much bytes can be
transmitted in a single burst. Therefore, the unit is in
multiples of 1024 (thus kibibytes) not SI-like 1000. It always
has been like that.

The 'tc' output is still confusing though, for instance:

  # tc class add dev $DEV parent 1: classid 1:1 htb rate 1000kbps burst 2097152
  # tc class show dev vnet2
  class htb 1:1 root rate 8Mbit ceil 8Mbit burst 2Mb cburst 1600b

Please note that 2097152 = 2*1024*1024. Even the man page is
confusing. From tc(8):

  kb or kKilobytes
  mb or mMegabytes

But I guess this is because 'tc' predates IEC standardisation of
binary multiples and thus can't change without breaking scripts
parsing its output.

And while at it, adjust _virNetDevBandwidthRate struct member
description, to make it obvious which members use SI/IEC units.

Signed-off-by: Michal Privoznik 
---
 docs/formatnetwork.html.in| 2 +-
 src/util/virnetdevbandwidth.h | 8 
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
index b1b2391f43..fad43f77ea 100644
--- a/docs/formatnetwork.html.in
+++ b/docs/formatnetwork.html.in
@@ -634,7 +634,7 @@
   
   burst
   
-  Optional attribute which specifies the amount of kilobytes that
+  Optional attribute which specifies the amount of kibibytes that
   can be transmitted in a single burst at peak speed.
   
   floor
diff --git a/src/util/virnetdevbandwidth.h b/src/util/virnetdevbandwidth.h
index 3d520721f6..c82029fc0f 100644
--- a/src/util/virnetdevbandwidth.h
+++ b/src/util/virnetdevbandwidth.h
@@ -23,10 +23,10 @@
 
 typedef struct _virNetDevBandwidthRate virNetDevBandwidthRate;
 struct _virNetDevBandwidthRate {
-unsigned long long average;  /* kbytes/s */
-unsigned long long peak; /* kbytes/s */
-unsigned long long floor;/* kbytes/s */
-unsigned long long burst;/* kbytes */
+unsigned long long average;  /* kilobytes/s */
+unsigned long long peak; /* kilobytes/s */
+unsigned long long floor;/* kilobytes/s */
+unsigned long long burst;/* kibibytes */
 };
 
 typedef struct _virNetDevBandwidth virNetDevBandwidth;
-- 
2.34.1

[PATCH 0/2] virnetdevopenvswitch: Fix 'burst' value passed to ovs-vsctl

[Michal Privoznik]

*** BLURB HERE ***

Michal Prívozník (2):
  docs: Clarify 'burst' units for QoS
  virnetdevopenvswitch: Fix 'burst' value passed to ovs-vsctl

 docs/formatnetwork.html.in  |  2 +-
 src/util/virnetdevbandwidth.h   |  8 
 src/util/virnetdevopenvswitch.c | 16 +---
 3 files changed, 14 insertions(+), 12 deletions(-)

-- 
2.34.1

[PATCH 2/2] virnetdevopenvswitch: Fix 'burst' value passed to ovs-vsctl

[Michal Privoznik]

As described in the previous commit, the units for 'burst' are
kibibytes and not kilobytes, i.e. multiples of 1024 not 1000.
Therefore, when constructing ovs-vsctl command the burst value
must be multiplied by 1024 and not just 1000. And because ovs
expects this size in bits the value has to be multiplied again by
8.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1510237#c26
Signed-off-by: Michal Privoznik 
---
 src/util/virnetdevopenvswitch.c | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
index 5dab545037..8741e0ed3a 100644
--- a/src/util/virnetdevopenvswitch.c
+++ b/src/util/virnetdevopenvswitch.c
@@ -638,11 +638,13 @@ virNetDevOpenvswitchFindUUID(const char *table,
 }
 
 /*
- * Average, peak, floor and burst in virNetDevBandwidth are in kbytes.
- * However other_config in ovs qos is in bit.
- * ingress_policing_rate in ovs interface is in kbit.
+ * In virNetDevBandwidthRate, average, peak, floor are in kilobyes and burst in
+ * kibibytes. However other_config in ovs qos is in bits and
+ * ingress_policing_rate in ovs interface is in kilobit for
+ * ingress_policing_rate and kibibit for ingress_policing_rate.
  */
-#define VIR_NETDEV_TX_TO_OVS 8000
+#define KBYTE_TO_BITS(x) (x * 8000ULL)
+#define KIBIBYTE_TO_BITS(x) (x * 8192ULL)
 #define VIR_NETDEV_RX_TO_OVS 8
 
 /**
@@ -717,11 +719,11 @@ virNetDevOpenvswitchInterfaceSetQos(const char *ifname,
 g_autofree char *qos_uuid = NULL;
 g_autofree char *queue_uuid = NULL;
 
-average = g_strdup_printf("%llu", tx->average * VIR_NETDEV_TX_TO_OVS);
+average = g_strdup_printf("%llu", KBYTE_TO_BITS(tx->average));
 if (tx->burst)
-burst = g_strdup_printf("%llu", tx->burst * VIR_NETDEV_TX_TO_OVS);
+burst = g_strdup_printf("%llu", KIBIBYTE_TO_BITS(tx->burst));
 if (tx->peak)
-peak = g_strdup_printf("%llu", tx->peak * VIR_NETDEV_TX_TO_OVS);
+peak = g_strdup_printf("%llu", KBYTE_TO_BITS(tx->peak));
 
 virUUIDFormat(vmuuid, vmuuidstr);
 vmid_ex_id = g_strdup_printf("external-ids:vm-id=\"%s\"", vmuuidstr);
-- 
2.34.1

Re: [PATCH 1/3] qemu: Revert to using non-JSON commandline for -device

[Peter Krempa]

On Tue, Jan 04, 2022 at 12:24:25 +, Daniel P. Berrangé wrote:
> On Mon, Jan 03, 2022 at 04:25:54PM +0100, Peter Krempa wrote:
> > When -device is configured via JSON a bug is triggered in qemu were the
> > DEVICE_DELETED event for the removal of the device frontend is no longer
> > delivered to libvirt. Without the DEVICE_DELETED event we don't remove
> > the corresponding entries in the VM XML.
> > 
> > Until qemu will be fixed we must stop using the JSON syntax for -device.
> 
> Pointer to the qemu bug report for this issue ?

Oops, I forgot to add them to the commit message:

Libvirt bug report:
https://bugzilla.redhat.com/show_bug.cgi?id=2035237

qemu bug report:
https://bugzilla.redhat.com/show_bug.cgi?id=2036669

[PATCH v3] Add VM info to improve error log message for qemu monitor

[Rohit Kumar]

This change adds the domain name in the error and debug logs during
monitor IO processing so that we may infer which VM experienced
errors such as IO or socket hangup. This may help in debugging
monitor IO errors.

Signed-off-by: Rohit Kumar 
---
 src/qemu/qemu_monitor.c | 36 +---
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index dda6ae9796..2b4582578a 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -80,6 +80,7 @@ struct _qemuMonitor {
 GSource *watch;
 
 virDomainObj *vm;
+char *domainName;
 
 qemuMonitorCallbacks *cb;
 void *callbackOpaque;
@@ -243,6 +244,7 @@ qemuMonitorDispose(void *obj)
 virCondDestroy(>notify);
 g_free(mon->buffer);
 g_free(mon->balloonpath);
+g_free(mon->domainName);
 }
 
 
@@ -583,8 +585,8 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
 
 if (!error && !mon->goteof &&
 cond & G_IO_ERR) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Invalid file descriptor while waiting for 
monitor"));
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Invalid file descriptor while waiting for 
monitor (vm='%s')"), mon->domainName);
 mon->goteof = true;
 }
 }
@@ -609,13 +611,14 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
 virResetLastError();
 } else {
 if (virGetLastErrorCode() == VIR_ERR_OK && !mon->goteof)
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Error while processing monitor IO"));
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Error while processing monitor IO 
(vm='%s')"), mon->domainName);
 virCopyLastError(>lastError);
 virResetLastError();
 }
 
-VIR_DEBUG("Error on monitor %s", NULLSTR(mon->lastError.message));
+VIR_DEBUG("Error on monitor %s mon=%p vm=%p name=%s",
+  NULLSTR(mon->lastError.message), mon, mon->vm, 
mon->domainName);
 /* If IO process resulted in an error & we have a message,
  * then wakeup that waiter */
 if (mon->msg && !mon->msg->finished) {
@@ -636,7 +639,8 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
 /* Make sure anyone waiting wakes up now */
 virCondSignal(>notify);
 virObjectUnlock(mon);
-VIR_DEBUG("Triggering EOF callback");
+VIR_DEBUG("Triggering EOF callback mon=%p vm=%p name=%s",
+  mon, mon->vm, mon->domainName);
 (eofNotify)(mon, vm, mon->callbackOpaque);
 virObjectUnref(mon);
 } else if (error) {
@@ -646,7 +650,8 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
 /* Make sure anyone waiting wakes up now */
 virCondSignal(>notify);
 virObjectUnlock(mon);
-VIR_DEBUG("Triggering error callback");
+VIR_DEBUG("Triggering error callback mon=%p vm=%p name=%s",
+  mon, mon->vm, mon->domainName);
 (errorNotify)(mon, vm, mon->callbackOpaque);
 virObjectUnref(mon);
 } else {
@@ -694,6 +699,7 @@ qemuMonitorOpenInternal(virDomainObj *vm,
 mon->fd = fd;
 mon->context = g_main_context_ref(context);
 mon->vm = virObjectRef(vm);
+mon->domainName = g_strdup(NULLSTR(vm->def->name));
 mon->waitGreeting = true;
 mon->cb = cb;
 mon->callbackOpaque = opaque;
@@ -935,14 +941,14 @@ qemuMonitorSend(qemuMonitor *mon,
 
 /* Check whether qemu quit unexpectedly */
 if (mon->lastError.code != VIR_ERR_OK) {
-VIR_DEBUG("Attempt to send command while error is set %s",
-  NULLSTR(mon->lastError.message));
+VIR_DEBUG("Attempt to send command while error is set %s mon=%p vm=%p 
name=%s",
+  NULLSTR(mon->lastError.message), mon, mon->vm, 
mon->domainName);
 virSetError(>lastError);
 return -1;
 }
 if (mon->goteof) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("End of file from qemu monitor"));
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("End of file from qemu monitor (vm='%s')"), 
mon->domainName);
 return -1;
 }
 
@@ -955,15 +961,15 @@ qemuMonitorSend(qemuMonitor *mon,
 
 while (!mon->msg->finished) {
 if (virCondWait(>notify, >parent.lock) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Unable to wait on monitor condition"));
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Unable to wait on monitor condition (vm='%s')"), 
mon->domainName);
 goto cleanup;
 }
 }
 
 if (mon->lastError.code != VIR_ERR_OK) {
-VIR_DEBUG("Send command resulted in error %s",
-  NULLSTR(mon->lastError.message));
+VIR_DEBUG("Send 

Re: [PATCH 1/3] domain_conf: Added configs for RSS and Hash report.

[Daniel P . Berrangé]

On Thu, Dec 30, 2021 at 08:01:43AM +0200, Andrew Melnychenko wrote:
> Added "rss" and "rss_hash_report" configuration that should be used with
> qemu virtio RSS.
> Both options are triswitches. Used as "driver" options and affects only NIC
> with model type "virtio".
> In other patches - options should turn on virtio-net RSS and hash properties.
> 
> Signed-off-by: Andrew Melnychenko 
> ---
>  docs/formatdomain.rst | 15 +++
>  docs/schemas/domaincommon.rng | 10 ++
>  src/conf/domain_conf.c| 31 ++-
>  src/conf/domain_conf.h|  2 ++
>  4 files changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index d4f30bb8af..f7b784ed26 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -5305,6 +5305,21 @@ following attributes are available for the 
> ``"virtio"`` NIC driver:
> only for ``vhostuser`` type. :since:`Since 3.7.0 (QEMU and KVM only)`
> **In general you should leave this option alone, unless you are very 
> certain
> you know what you are doing.**
> +``rss``
> +   The ``rss`` option enables in-qemu/ebpf RSS for virtio NIC. RSS works with
> +   virtio and tap backends only. Virtio NIC will be launched with "rss"
> +   property. Qemu may load eBPF RSS if it has CAP_SYS_ADMIN permissions.

QEMU won't have CAP_SYS_ADMIN with libvirt unless someone goes out of their
way to make the libvirt host configuration insecure. We would consider such
a config to be unsupported aside from developers experimenting with stuff,
so I don't think it merits documentation.

> +   In other cases, "in-qemu" RSS is used.

So this is only realistic scenario that can be supported under
libvirt.

> +   **In general you should leave this option alone, unless you are very 
> certain
> +   you know what you are doing.**
> +``rss_hash_report``
> +   The ``rss_hash_report`` option enables in-qemu RSS hash report for virtio
> +   NIC. Virtio NIC will be launched with "hash" property. Ebpf RSS doesn't
> +   support hash report yet. Usually enabled alongside with ``rss``.
> +   Without ``rss`` option, the hash report doesn't affect steering
> +   itself but provides vnet header with a calculated hash.
> +   **In general you should leave this option alone, unless you are very 
> certain
> +   you know what you are doing.**
>  virtio options
> For virtio interfaces, `Virtio-specific options <#elementsVirtio>`__ can 
> also
> be set. ( :since:`Since 3.5.0` )

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 V4 6/6] NEWS: Mention new virDomainSetLaunchSecurityState API

[Daniel P . Berrangé]

On Mon, Dec 20, 2021 at 08:23:20PM -0700, Jim Fehlig wrote:
> Signed-off-by: Jim Fehlig 
> ---
>  NEWS.rst | 6 ++
>  1 file changed, 6 insertions(+)

Reviewed-by: Daniel P. Berrangé 


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 V4 4/6] qemu: Implement the virDomainSetLaunchSecurityState API

[Daniel P . Berrangé]

On Mon, Dec 20, 2021 at 08:23:18PM -0700, Jim Fehlig wrote:
> Set a launch secret in guest memory using the sev-inject-launch-secret
> QMP API. Only supported with qemu >= 6.0.0 and SEV-enabled guests in a
> paused state.
> 
> Signed-off-by: Jim Fehlig 
> ---
> 
> Daniel already r-b V3 of this patch, but I didn't include it since there's
> a bit of change in V4.
> 
>  src/qemu/qemu_driver.c   | 100 +++
>  src/qemu/qemu_monitor.c  |  14 +
>  src/qemu/qemu_monitor.h  |   7 +++
>  src/qemu/qemu_monitor_json.c |  45 
>  src/qemu/qemu_monitor_json.h |   6 +++
>  tests/qemumonitorjsontest.c  |   3 ++
>  6 files changed, 175 insertions(+)

Reviewed-by: Daniel P. Berrangé 


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 V4 3/6] qemu_capabilities: Introduce QEMU_CAPS_SEV_INJECT_LAUNCH_SECRET

[Daniel P . Berrangé]

On Mon, Dec 20, 2021 at 08:23:17PM -0700, Jim Fehlig wrote:
> The 'sev-inject-launch-secret' qmp command is only available with
> qemu >= 6.0.0. Introduce a capability for sev-inject-launch-secret.
> 
> Signed-off-by: Jim Fehlig 
> ---
>  src/qemu/qemu_capabilities.c | 2 ++
>  src/qemu/qemu_capabilities.h | 1 +
>  tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml | 1 +
>  tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml | 1 +
>  tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml | 1 +
>  5 files changed, 6 insertions(+)

Reviewed-by: Daniel P. Berrangé 

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: [RFC v2 1/1] qemu: add index for isa-serial device using target.port

[Divya Garg]

On 04/01/22 5:56 pm, Ani Sinha wrote:


On Tue, 4 Jan 2022, Divya Garg wrote:


On 04/01/22 5:47 pm, Ani Sinha wrote:

On Mon, 3 Jan 2022, Divya Garg wrote:


Thankyou Ani for the review. I will be taking up the comments
in next patchset along with other comments.

On 03/01/22 1:44 pm, Ani Sinha wrote:


On Mon, 3 Jan 2022, Divya Garg wrote:


Hi all !

Looking forward for the review comments on the patch.


For single patches, no need to use numbering.

I have also added the cover letter with the patch. Hence added the
numbering
here.

btw, cover letters are not essential for single patches:

https://urldefense.proofpoint.com/v2/url?u=https-3A__libvirt.org_submitting-2Dpatches.html=DwIBAg=s883GpUCOChKOHiocYtGcg=2QGHz-fTCVWImEBKe1ZcSe5t6UfasnhvdzD5DcixwOE=neaBi0ZuC9uGNNnNZGkXsPK_JEReYD2oA8csE4KE4LUYwtOWFxn6ipCd7_mPXNll=u-g3In7hJ90OdEzNmUj41bzzvmAIvOmLJSBpcAB2kQc=

Thankyou so much Ani. I added the cover letter because i was not sure if the
checks I am applying are needed or not. Hence wanted to explain all my cases
that I am handling through the cover letter.

yes that's perfectly fine.

Thanks Ani for your time.

Re: [RFC v2 1/1] qemu: add index for isa-serial device using target.port

[Ani Sinha]

On Tue, 4 Jan 2022, Divya Garg wrote:

> On 04/01/22 5:47 pm, Ani Sinha wrote:
> >
> > On Mon, 3 Jan 2022, Divya Garg wrote:
> >
> > > Thankyou Ani for the review. I will be taking up the comments
> > > in next patchset along with other comments.
> > >
> > > On 03/01/22 1:44 pm, Ani Sinha wrote:
> > >
> > > > On Mon, 3 Jan 2022, Divya Garg wrote:
> > > >
> > > > > Hi all !
> > > > >
> > > > > Looking forward for the review comments on the patch.
> > > > >
> > > > For single patches, no need to use numbering.
> > > I have also added the cover letter with the patch. Hence added the
> > > numbering
> > > here.
> > btw, cover letters are not essential for single patches:
> >
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__libvirt.org_submitting-2Dpatches.html=DwIBAg=s883GpUCOChKOHiocYtGcg=2QGHz-fTCVWImEBKe1ZcSe5t6UfasnhvdzD5DcixwOE=neaBi0ZuC9uGNNnNZGkXsPK_JEReYD2oA8csE4KE4LUYwtOWFxn6ipCd7_mPXNll=u-g3In7hJ90OdEzNmUj41bzzvmAIvOmLJSBpcAB2kQc=
>
> Thankyou so much Ani. I added the cover letter because i was not sure if the
> checks I am applying are needed or not. Hence wanted to explain all my cases
> that I am handling through the cover letter.

yes that's perfectly fine.

Re: [PATCH 0/4] Rework formatting

[Daniel P . Berrangé]

On Tue, Jan 04, 2022 at 09:14:26AM +0100, Michal Privoznik wrote:
> Please note that the test suite is temporarily broken after 2/4 but
> fixed in 3/4. This could be resolved be swapping those two patches, but
> I figured I keep the order to demonstrate the bug. However, I can do the
> swap if desired.

They certainly must be swapped before this is pushed, otherwise it
breaks 'git bisect' for anyone using 'meson test' as their validator
for the bisect.


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 1/3] qemu: Revert to using non-JSON commandline for -device

[Daniel P . Berrangé]

On Mon, Jan 03, 2022 at 04:25:54PM +0100, Peter Krempa wrote:
> When -device is configured via JSON a bug is triggered in qemu were the
> DEVICE_DELETED event for the removal of the device frontend is no longer
> delivered to libvirt. Without the DEVICE_DELETED event we don't remove
> the corresponding entries in the VM XML.
> 
> Until qemu will be fixed we must stop using the JSON syntax for -device.

Pointer to the qemu bug report for this issue ?


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: [RFC v2 1/1] qemu: add index for isa-serial device using target.port

[Divya Garg]

On 04/01/22 5:47 pm, Ani Sinha wrote:


On Mon, 3 Jan 2022, Divya Garg wrote:


Thankyou Ani for the review. I will be taking up the comments
in next patchset along with other comments.

On 03/01/22 1:44 pm, Ani Sinha wrote:


On Mon, 3 Jan 2022, Divya Garg wrote:


Hi all !

Looking forward for the review comments on the patch.


For single patches, no need to use numbering.

I have also added the cover letter with the patch. Hence added the numbering
here.

btw, cover letters are not essential for single patches:

https://urldefense.proofpoint.com/v2/url?u=https-3A__libvirt.org_submitting-2Dpatches.html=DwIBAg=s883GpUCOChKOHiocYtGcg=2QGHz-fTCVWImEBKe1ZcSe5t6UfasnhvdzD5DcixwOE=neaBi0ZuC9uGNNnNZGkXsPK_JEReYD2oA8csE4KE4LUYwtOWFxn6ipCd7_mPXNll=u-g3In7hJ90OdEzNmUj41bzzvmAIvOmLJSBpcAB2kQc=


Thankyou so much Ani. I added the cover letter because i was not sure if 
the checks I am applying are needed or not. Hence wanted to explain all 
my cases that I am handling through the cover letter.

Re: [RFC v2 1/1] qemu: add index for isa-serial device using target.port

[Divya Garg]

On 04/01/22 5:17 pm, John Levon wrote:

On Sat, Dec 11, 2021 at 07:57:47PM -0800, “Divya wrote:


From: Divya Garg 

VM XML accepts target.port but this does not get passed while building the qemu
command line for this VM.

Apologies, I failed to notice this had been sent out to the list; Re-posting
my comments from an internal thread, so all can see:

I don't think serial ports that are not isa-serial should clash with serial
ports like this.

In fact, from what I can see, as "port" is not used for any other type, we
should just completely ignore non-isa-serial ports here.


So you can simplify this all to, I think:

for (i = 0; i < isa_serial_count; i++) {
 if (type ! = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL):
continue

 if (def->serials[i]->port != -1):
 // check for clash / too high value
 // else mark port used
}

for (i = 0; i < isa_serial_count; i++) {
 if (type != VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL):
continue

 if (def->serials[i]->port == -1):
 // allocate lowest free port
 // error if none available
}

regards
john

Thankyou John ! I will be taking up the changes in my next patch.



This patch fixes this bug. In addition, this introduces additional checks in
the port allocation logic for isa-serial devices to :
* Check availability of requested ports
* Prevent duplicate device assignments to the same port.
* In case no ports are provided in the XML, this patch scans the list of unused
   isa-serial indices to automatically assign available ports for this VM.

Signed-off-by: Divya Garg 
---
  src/conf/domain_conf.c| 66 ---
  src/conf/domain_conf.h|  1 +
  src/qemu/qemu_command.c   | 21 --
  ...g-console-compat-2-live+console-virtio.xml |  4 +-
  .../qemuhotplug-console-compat-2-live.xml |  4 +-
  tests/qemuxml2argvdata/bios.args  |  2 +-
  .../qemuxml2argvdata/console-compat-auto.args |  2 +-
  .../console-compat-chardev.args   |  2 +-
  tests/qemuxml2argvdata/console-compat.args|  2 +-
  .../qemuxml2argvdata/console-virtio-many.args |  2 +-
  tests/qemuxml2argvdata/controller-order.args  |  2 +-
  .../name-escape.x86_64-2.11.0.args|  4 +-
  .../q35-virt-manager-basic.args   |  2 +-
  .../serial-dev-chardev-iobase.args|  2 +-
  .../qemuxml2argvdata/serial-dev-chardev.args  |  2 +-
  .../qemuxml2argvdata/serial-file-chardev.args |  2 +-
  tests/qemuxml2argvdata/serial-file-log.args   |  2 +-
  .../qemuxml2argvdata/serial-many-chardev.args |  4 +-
  .../qemuxml2argvdata/serial-pty-chardev.args  |  2 +-
  tests/qemuxml2argvdata/serial-spiceport.args  |  2 +-
  .../qemuxml2argvdata/serial-tcp-chardev.args  |  2 +-
  .../serial-tcp-telnet-chardev.args|  2 +-
  .../serial-tcp-tlsx509-chardev-notls.args |  4 +-
  .../serial-tcp-tlsx509-chardev-notls.xml  |  2 +-
  .../serial-tcp-tlsx509-chardev-verify.args|  4 +-
  .../serial-tcp-tlsx509-chardev-verify.xml |  2 +-
  .../serial-tcp-tlsx509-chardev.args   |  4 +-
  .../serial-tcp-tlsx509-chardev.xml|  2 +-
  .../serial-tcp-tlsx509-secret-chardev.args|  4 +-
  .../serial-tcp-tlsx509-secret-chardev.xml |  2 +-
  .../qemuxml2argvdata/serial-udp-chardev.args  |  4 +-
  .../qemuxml2argvdata/serial-unix-chardev.args |  4 +-
  .../serial-unix-chardev.x86_64-latest.args|  4 +-
  tests/qemuxml2argvdata/serial-vc-chardev.args |  2 +-
  tests/qemuxml2argvdata/user-aliases.args  |  4 +-
  .../virtio-9p-createmode.x86_64-latest.args   |  2 +-
  .../virtio-9p-multidevs.x86_64-latest.args|  2 +-
  .../x86_64-pc-graphics.x86_64-latest.args |  2 +-
  .../x86_64-pc-headless.x86_64-latest.args |  2 +-
  .../x86_64-q35-graphics.x86_64-latest.args|  2 +-
  .../x86_64-q35-headless.x86_64-latest.args|  2 +-
  .../serial-tcp-tlsx509-chardev.xml|  2 +-
  42 files changed, 126 insertions(+), 64 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 107d2a4f5d..e8b19828d4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5319,6 +5319,61 @@ virDomainHostdevDefPostParse(virDomainHostdevDef *dev,
  }
  
  
+static int

+virDomainChrIsaSerialDefPostParse(virDomainDef *def)
+{
+size_t i, j;
+size_t isa_serial_count = 0;
+int isa_device_index_arr[VIR_MAX_AVAILABLE_ISA_SERIAL_PORTS] = {0};
+bool used_serial_port[VIR_MAX_AVAILABLE_ISA_SERIAL_PORTS] = {false};
+
+for (i = 0; i < def->nserials; i++) {
+if (def->serials[i]->targetType ==
+VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL) {
+if (isa_serial_count >= VIR_MAX_AVAILABLE_ISA_SERIAL_PORTS) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Maximum supported number of ISA serial ports is 
%d."), 

[PATCH] qemu: agent: remove all code around disabled DEBUG_IO/DEBUG_RAW_IO definitions

[Ani Sinha]

DEBUG_IO and DEBUG_RAW_IO are disabled and hence the code #defined under them
are useless. Remove them. Also remove similar useless code from
qemu_monitor_json.

See also 4aae00bf1287f ("qemu: monitor: Remove disabled debug infrastructure")

Signed-off-by: Ani Sinha 
---
 src/qemu/qemu_agent.c| 51 
 src/qemu/qemu_monitor_json.c |  4 ---
 2 files changed, 55 deletions(-)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index cb3bf97415..f33cd47078 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -48,9 +48,6 @@ VIR_LOG_INIT("qemu.qemu_agent");
 
 #define LINE_ENDING "\n"
 
-#define DEBUG_IO 0
-#define DEBUG_RAW_IO 0
-
 /* We read from QEMU until seeing a \r\n pair to indicate a
  * completed reply or event. To avoid memory denial-of-service
  * though, we must have a size limit on amount of data we
@@ -137,25 +134,6 @@ static int qemuAgentOnceInit(void)
 VIR_ONCE_GLOBAL_INIT(qemuAgent);
 
 
-#if DEBUG_RAW_IO
-static char *
-qemuAgentEscapeNonPrintable(const char *text)
-{
-size_t i;
-g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
-for (i = 0; text[i] != '\0'; i++) {
-if (text[i] == '\\')
-virBufferAddLit(, "");
-else if (g_ascii_isprint(text[i]) || text[i] == '\n' ||
- (text[i] == '\r' && text[i+1] == '\n'))
-virBufferAddChar(, text[i]);
-else
-virBufferAsprintf(, "\\x%02x", text[i]);
-}
-return virBufferContentAndReset();
-}
-#endif
-
 
 static void qemuAgentDispose(void *obj)
 {
@@ -300,14 +278,6 @@ static int qemuAgentIOProcessData(qemuAgent *agent,
 {
 int used = 0;
 size_t i = 0;
-#if DEBUG_IO
-# if DEBUG_RAW_IO
-g_autofree char *str1 = qemuAgentEscapeNonPrintable(data);
-VIR_ERROR(_("[%s]"), str1);
-# else
-VIR_DEBUG("Data %zu bytes [%s]", len, data);
-# endif
-#endif
 
 while (used < len) {
 char *nl = strstr(data + used, LINE_ENDING);
@@ -344,17 +314,6 @@ qemuAgentIOProcess(qemuAgent *agent)
 if (agent->msg && agent->msg->txOffset == agent->msg->txLength)
 msg = agent->msg;
 
-#if DEBUG_IO
-# if DEBUG_RAW_IO
-g_autofree char *str1 = qemuAgentEscapeNonPrintable(msg ? msg->txBuffer : 
"");
-g_autofree char *str2 = qemuAgentEscapeNonPrintable(agent->buffer);
-VIR_ERROR(_("Process %zu %p %p [[[%s]]][[[%s]]]"),
-  agent->bufferOffset, agent->msg, msg, str1, str2);
-# else
-VIR_DEBUG("Process %zu", agent->bufferOffset);
-# endif
-#endif
-
 len = qemuAgentIOProcessData(agent,
  agent->buffer, agent->bufferOffset,
  msg);
@@ -369,9 +328,6 @@ qemuAgentIOProcess(qemuAgent *agent)
 VIR_FREE(agent->buffer);
 agent->bufferOffset = agent->bufferLength = 0;
 }
-#if DEBUG_IO
-VIR_DEBUG("Process done %zu used %d", agent->bufferOffset, len);
-#endif
 if (msg && msg->finished)
 virCondBroadcast(>notify);
 return len;
@@ -455,10 +411,6 @@ qemuAgentIORead(qemuAgent *agent)
 agent->buffer[agent->bufferOffset] = '\0';
 }
 
-#if DEBUG_IO
-VIR_DEBUG("Now read %zu bytes of data", agent->bufferOffset);
-#endif
-
 return ret;
 }
 
@@ -527,9 +479,6 @@ qemuAgentIO(GSocket *socket G_GNUC_UNUSED,
 virObjectRef(agent);
 /* lock access to the agent and protect fd */
 virObjectLock(agent);
-#if DEBUG_IO
-VIR_DEBUG("Agent %p I/O on watch %d socket %p cond %d", agent, 
agent->socket, cond);
-#endif
 
 if (agent->fd == -1 || !agent->watch) {
 virObjectUnlock(agent);
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index e45b43eb5a..0f1999f413 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -280,10 +280,6 @@ int qemuMonitorJSONIOProcess(qemuMonitor *mon,
 }
 }
 
-#if DEBUG_IO
-VIR_DEBUG("Total used %d bytes out of %zd available in buffer", used, len);
-#endif
-
 return used;
 }
 
-- 
2.25.1

Re: [RFC v2 1/1] qemu: add index for isa-serial device using target.port

[John Levon]

On Sat, Dec 11, 2021 at 07:57:47PM -0800, “Divya wrote:

> From: Divya Garg 
> 
> VM XML accepts target.port but this does not get passed while building the 
> qemu
> command line for this VM.

Apologies, I failed to notice this had been sent out to the list; Re-posting
my comments from an internal thread, so all can see:

I don't think serial ports that are not isa-serial should clash with serial
ports like this.

In fact, from what I can see, as "port" is not used for any other type, we
should just completely ignore non-isa-serial ports here.


So you can simplify this all to, I think:

for (i = 0; i < isa_serial_count; i++) {
if (type ! = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL):
   continue

if (def->serials[i]->port != -1):
// check for clash / too high value
// else mark port used
}

for (i = 0; i < isa_serial_count; i++) {
if (type != VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL):
   continue

if (def->serials[i]->port == -1):
// allocate lowest free port
// error if none available
}

regards
john

> 
> This patch fixes this bug. In addition, this introduces additional checks in
> the port allocation logic for isa-serial devices to :
> * Check availability of requested ports
> * Prevent duplicate device assignments to the same port.
> * In case no ports are provided in the XML, this patch scans the list of 
> unused
>   isa-serial indices to automatically assign available ports for this VM.
> 
> Signed-off-by: Divya Garg 
> ---
>  src/conf/domain_conf.c| 66 ---
>  src/conf/domain_conf.h|  1 +
>  src/qemu/qemu_command.c   | 21 --
>  ...g-console-compat-2-live+console-virtio.xml |  4 +-
>  .../qemuhotplug-console-compat-2-live.xml |  4 +-
>  tests/qemuxml2argvdata/bios.args  |  2 +-
>  .../qemuxml2argvdata/console-compat-auto.args |  2 +-
>  .../console-compat-chardev.args   |  2 +-
>  tests/qemuxml2argvdata/console-compat.args|  2 +-
>  .../qemuxml2argvdata/console-virtio-many.args |  2 +-
>  tests/qemuxml2argvdata/controller-order.args  |  2 +-
>  .../name-escape.x86_64-2.11.0.args|  4 +-
>  .../q35-virt-manager-basic.args   |  2 +-
>  .../serial-dev-chardev-iobase.args|  2 +-
>  .../qemuxml2argvdata/serial-dev-chardev.args  |  2 +-
>  .../qemuxml2argvdata/serial-file-chardev.args |  2 +-
>  tests/qemuxml2argvdata/serial-file-log.args   |  2 +-
>  .../qemuxml2argvdata/serial-many-chardev.args |  4 +-
>  .../qemuxml2argvdata/serial-pty-chardev.args  |  2 +-
>  tests/qemuxml2argvdata/serial-spiceport.args  |  2 +-
>  .../qemuxml2argvdata/serial-tcp-chardev.args  |  2 +-
>  .../serial-tcp-telnet-chardev.args|  2 +-
>  .../serial-tcp-tlsx509-chardev-notls.args |  4 +-
>  .../serial-tcp-tlsx509-chardev-notls.xml  |  2 +-
>  .../serial-tcp-tlsx509-chardev-verify.args|  4 +-
>  .../serial-tcp-tlsx509-chardev-verify.xml |  2 +-
>  .../serial-tcp-tlsx509-chardev.args   |  4 +-
>  .../serial-tcp-tlsx509-chardev.xml|  2 +-
>  .../serial-tcp-tlsx509-secret-chardev.args|  4 +-
>  .../serial-tcp-tlsx509-secret-chardev.xml |  2 +-
>  .../qemuxml2argvdata/serial-udp-chardev.args  |  4 +-
>  .../qemuxml2argvdata/serial-unix-chardev.args |  4 +-
>  .../serial-unix-chardev.x86_64-latest.args|  4 +-
>  tests/qemuxml2argvdata/serial-vc-chardev.args |  2 +-
>  tests/qemuxml2argvdata/user-aliases.args  |  4 +-
>  .../virtio-9p-createmode.x86_64-latest.args   |  2 +-
>  .../virtio-9p-multidevs.x86_64-latest.args|  2 +-
>  .../x86_64-pc-graphics.x86_64-latest.args |  2 +-
>  .../x86_64-pc-headless.x86_64-latest.args |  2 +-
>  .../x86_64-q35-graphics.x86_64-latest.args|  2 +-
>  .../x86_64-q35-headless.x86_64-latest.args|  2 +-
>  .../serial-tcp-tlsx509-chardev.xml|  2 +-
>  42 files changed, 126 insertions(+), 64 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 107d2a4f5d..e8b19828d4 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5319,6 +5319,61 @@ virDomainHostdevDefPostParse(virDomainHostdevDef *dev,
>  }
>  
>  
> +static int
> +virDomainChrIsaSerialDefPostParse(virDomainDef *def)
> +{
> +size_t i, j;
> +size_t isa_serial_count = 0;
> +int isa_device_index_arr[VIR_MAX_AVAILABLE_ISA_SERIAL_PORTS] = {0};
> +bool used_serial_port[VIR_MAX_AVAILABLE_ISA_SERIAL_PORTS] = {false};
> +
> +for (i = 0; i < def->nserials; i++) {
> +if (def->serials[i]->targetType ==
> +VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL) {
> +if (isa_serial_count >= VIR_MAX_AVAILABLE_ISA_SERIAL_PORTS) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Maximum supported number of ISA serial 
> ports is %d."), VIR_MAX_AVAILABLE_ISA_SERIAL_PORTS);

Re: [RFC v2 1/1] qemu: add index for isa-serial device using target.port

[Ani Sinha]

On Mon, 3 Jan 2022, Divya Garg wrote:

> Thankyou Ani for the review. I will be taking up the comments
> in next patchset along with other comments.
>
> On 03/01/22 1:44 pm, Ani Sinha wrote:
>
> >
> > On Mon, 3 Jan 2022, Divya Garg wrote:
> >
> > > Hi all !
> > >
> > > Looking forward for the review comments on the patch.
> > >
> > For single patches, no need to use numbering.
> I have also added the cover letter with the patch. Hence added the numbering
> here.

btw, cover letters are not essential for single patches:

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

Re: [libvirt PATCH v2 4/6] docs: Move all icons to a subdirectory

[Daniel P . Berrangé]

On Tue, Dec 21, 2021 at 04:30:31PM +0100, Andrea Bolognani wrote:
> This unclutters the top-level docs directory.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  docs/browserconfig.xml  |   2 +-
>  docs/{ => icons}/android-chrome-192x192.png | Bin
>  docs/{ => icons}/android-chrome-256x256.png | Bin
>  docs/{ => icons}/apple-touch-icon.png   | Bin
>  docs/{ => icons}/favicon-16x16.png  | Bin
>  docs/{ => icons}/favicon-32x32.png  | Bin
>  docs/{ => icons}/favicon.ico| Bin
>  docs/icons/meson.build  |  19 +++
>  docs/{ => icons}/mstile-150x150.png | Bin

Please don't move any of the favicon related images. These were placed
at the root intentionally per the recommendations of the favicon
generator we used for better client compatibility

https://realfavicongenerator.net/faq

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: [libvirt PATCH v2 2/6] docs: Add pointing to favicon.ico

[Daniel P . Berrangé]

On Tue, Dec 21, 2021 at 04:30:29PM +0100, Andrea Bolognani wrote:
> It's not strictly necessary when the icon lives in the top-level
> directory of the website, as browsers will fall back to that
> path when the  element is absent, but it's still considered
> good practice to spell out the path explicitly.

On the contrary, it is not good practice, as it breaks some
browsers. This was explicitly omitted:

  https://realfavicongenerator.net/faq#why_ico_not_declared

> Signed-off-by: Andrea Bolognani 
> ---
>  docs/page.xsl | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/docs/page.xsl b/docs/page.xsl
> index 580387ac59..52716ba4a5 100644
> --- a/docs/page.xsl
> +++ b/docs/page.xsl
> @@ -93,6 +93,7 @@
>  
>  
>   href="/apple-touch-icon.png"/>
> +
>   href="/favicon-32x32.png"/>
>   href="/favicon-16x16.png"/>
>  
> -- 
> 2.31.1
> 

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 2/2] virnettlscontext: Don't pass static key length to gnutls_dh_params_generate2()

[Daniel P . Berrangé]

On Tue, Dec 21, 2021 at 03:22:59PM +0100, Michal Privoznik wrote:
> As encryption norms get more strict it's easy to fall on the
> insecure side. For instance, so far we are generating 2048 bits
> long prime for Diffie-Hellman keys. Some systems consider this
> not long enough. While we may just keep increasing the value
> passed to the corresponding gnutls_* function, that is not well
> maintainable. Instead, we may do what's recommended in the
> gnutls_* manpage. From gnutls_dh_params_generate2(3):
> 
>   It is recommended not to set the number of bits directly, but
>   use gnutls_sec_param_to_pk_bits() instead.
> 
> Looking into the gnutls_sec_param_to_pk_bits() then [1], 2048
> bits corresponds to parameter MEDIUM. Therefore, we want to chose
> the next size (HIGH) to be future proof.
> 
> 1: https://www.gnutls.org/manual/gnutls.html#tab_003akey_002dsizes
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/rpc/virnettlscontext.c | 13 ++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
> index 3b6687e7f4..f0b1e8f9c1 100644
> --- a/src/rpc/virnettlscontext.c
> +++ b/src/rpc/virnettlscontext.c
> @@ -38,8 +38,6 @@
>  #include "virthread.h"
>  #include "configmake.h"
>  
> -#define DH_BITS 2048
> -
>  #define LIBVIRT_PKI_DIR SYSCONFDIR "/pki"
>  #define LIBVIRT_CACERT LIBVIRT_PKI_DIR "/CA/cacert.pem"
>  #define LIBVIRT_CACRL LIBVIRT_PKI_DIR "/CA/cacrl.pem"
> @@ -718,6 +716,15 @@ static virNetTLSContext *virNetTLSContextNew(const char 
> *cacert,
>   * security requirements.
>   */
>  if (isServer) {
> +unsigned int bits = 0;
> +
> +bits = gnutls_sec_param_to_pk_bits(GNUTLS_PK_DH, 
> GNUTLS_SEC_PARAM_HIGH);
> +if (bits == 0) {
> +virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
> +   _("Unable to get key length for diffie-hellman 
> parameters"));
> +goto error;
> +}
> +
>  err = gnutls_dh_params_init(>dhParams);
>  if (err < 0) {
>  virReportError(VIR_ERR_SYSTEM_ERROR,
> @@ -725,7 +732,7 @@ static virNetTLSContext *virNetTLSContextNew(const char 
> *cacert,
> gnutls_strerror(err));
>  goto error;
>  }
> -err = gnutls_dh_params_generate2(ctxt->dhParams, DH_BITS);
> +err = gnutls_dh_params_generate2(ctxt->dhParams, bits);
>  if (err < 0) {
>  virReportError(VIR_ERR_SYSTEM_ERROR,
> _("Unable to generate diffie-hellman parameters: 
> %s"),

We shouldn't be introducing use of gnutls_sec_param_to_pk_bits at
all IMHO, rather we should be removing use of gnutls_dh_params_generate2
instead.

The recommendation is to use pre-generated DH parameters from the
the FFDHE set of RFC7919.

In gnutls >= 3.6.0 this happens automatically.

In gnutls >= 3.5.6 && < 3.6.0 we can replace thegnutls_dh_params_generate2 +
gnutls_certificate_set_dh_params pair of calls, with just
gnutls_certificate_set_known_dh_params()

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] rpc: Require dtrace sources to be generated first

[Daniel P . Berrangé]

On Mon, Dec 20, 2021 at 12:02:41PM +0100, Michal Privoznik wrote:
> The virt_socket_lib is built from virnetsocket.c (among others).
> But this file includes virprobe.h which includes libvirt_probes.h
> which is a generated file. But this dependency is not recorded in
> meson which may lead to a failed build.
> 
> Signed-off-by: Michal Privoznik 
> ---
> 
> I don't know if this is the right fix or we need to go with
> declare_dependency(), or even something else. But this fixes the build
> for me.

We do similar to this in other places for dtrace_gen_headers

> 
>  src/rpc/meson.build | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/rpc/meson.build b/src/rpc/meson.build
> index 7fde92e6cf..36a2809adf 100644
> --- a/src/rpc/meson.build
> +++ b/src/rpc/meson.build
> @@ -9,6 +9,7 @@ socket_sources = [
>  virt_socket_lib = static_library(
>'virt_socket',
>[
> +dtrace_gen_headers,
>  socket_sources,
>],
>dependencies: [

Reviewed-by: Daniel P. Berrangé 


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 v2] Add VM info to improve error log message for qemu monitor

[Rohit Kumar]

On 04/01/22 3:56 pm, Ani Sinha wrote:


On Tue, 4 Jan 2022, Peter Krempa wrote:


On Tue, Jan 04, 2022 at 15:30:00 +0530, Ani Sinha wrote:

On Tue, 4 Jan 2022, Rohit Kumar wrote:

On 03/01/22 7:12 pm, Ani Sinha wrote:

On Wed, 22 Dec 2021, Rohit Kumar wrote:

[...]


@@ -694,6 +702,7 @@ qemuMonitorOpenInternal(virDomainObj *vm,
   mon->fd = fd;
   mon->context = g_main_context_ref(context);
   mon->vm = virObjectRef(vm);
+mon->domainName = g_strdup(vm->def->name);

do not forget to g_free() this during cleanup in the same function.

So, in cleanup: qemuMonitorDispose is called. And there I have added g_free()
to clean mon->domainName.

No, in cleanup, I see qemuMonitorClose() is called where do you do not add
any additional code to free the allocation.

This is what I see in cleanup code:

```
cleanup:
 /* We don't want the 'destroy' callback invoked during
  * cleanup from construction failure, because that can
  * give a double-unref on virDomainObj *in the caller,
  * so kill the callbacks now.
  */
 mon->cb = NULL;
 /* The caller owns 'fd' on failure */
 mon->fd = -1;
 qemuMonitorClose(mon);

qemuMonitorClose() eventually calls virObjectUnref(mon). Once the last
reference on the monitor object is removed the object is freed via
qemuMonitorDispose().

This patch has:

@@ -243,6 +244,7 @@ qemuMonitorDispose(void *obj)
  virCondDestroy(>notify);
  g_free(mon->buffer);
  g_free(mon->balloonpath);
+g_free(mon->domainName);
  }

Oh ok. I assumed that there was two cleanup paths :
one is the natural tear down where qemuMonitorDispose() would be called.
Other was failure during contruction itself which I thought needed
additional handling.
Its ok then, no need to add additional g_free in monitorOpen()


Ack. Thanks for the review Ani.

Re: [PATCH v2] Add VM info to improve error log message for qemu monitor

[Ani Sinha]

On Tue, 4 Jan 2022, Peter Krempa wrote:

> On Tue, Jan 04, 2022 at 15:30:00 +0530, Ani Sinha wrote:
> > On Tue, 4 Jan 2022, Rohit Kumar wrote:
> > > On 03/01/22 7:12 pm, Ani Sinha wrote:
> > > > On Wed, 22 Dec 2021, Rohit Kumar wrote:
>
> [...]
>
> > > > > @@ -694,6 +702,7 @@ qemuMonitorOpenInternal(virDomainObj *vm,
> > > > >   mon->fd = fd;
> > > > >   mon->context = g_main_context_ref(context);
> > > > >   mon->vm = virObjectRef(vm);
> > > > > +mon->domainName = g_strdup(vm->def->name);
> > > > do not forget to g_free() this during cleanup in the same function.
> > > So, in cleanup: qemuMonitorDispose is called. And there I have added 
> > > g_free()
> > > to clean mon->domainName.
> >
> > No, in cleanup, I see qemuMonitorClose() is called where do you do not add
> > any additional code to free the allocation.
> >
> > This is what I see in cleanup code:
> >
> > ```
> > cleanup:
> > /* We don't want the 'destroy' callback invoked during
> >  * cleanup from construction failure, because that can
> >  * give a double-unref on virDomainObj *in the caller,
> >  * so kill the callbacks now.
> >  */
> > mon->cb = NULL;
> > /* The caller owns 'fd' on failure */
> > mon->fd = -1;
> > qemuMonitorClose(mon);
>
> qemuMonitorClose() eventually calls virObjectUnref(mon). Once the last
> reference on the monitor object is removed the object is freed via
> qemuMonitorDispose().
>
> This patch has:
>
> @@ -243,6 +244,7 @@ qemuMonitorDispose(void *obj)
>  virCondDestroy(>notify);
>  g_free(mon->buffer);
>  g_free(mon->balloonpath);
> +g_free(mon->domainName);
>  }

Oh ok. I assumed that there was two cleanup paths :
one is the natural tear down where qemuMonitorDispose() would be called.
Other was failure during contruction itself which I thought needed
additional handling.
Its ok then, no need to add additional g_free in monitorOpen()

Re: [PATCH v2] Add VM info to improve error log message for qemu monitor

[Peter Krempa]

On Tue, Jan 04, 2022 at 15:30:00 +0530, Ani Sinha wrote:
> On Tue, 4 Jan 2022, Rohit Kumar wrote:
> > On 03/01/22 7:12 pm, Ani Sinha wrote:
> > > On Wed, 22 Dec 2021, Rohit Kumar wrote:

[...]

> > > > @@ -694,6 +702,7 @@ qemuMonitorOpenInternal(virDomainObj *vm,
> > > >   mon->fd = fd;
> > > >   mon->context = g_main_context_ref(context);
> > > >   mon->vm = virObjectRef(vm);
> > > > +mon->domainName = g_strdup(vm->def->name);
> > > do not forget to g_free() this during cleanup in the same function.
> > So, in cleanup: qemuMonitorDispose is called. And there I have added 
> > g_free()
> > to clean mon->domainName.
> 
> No, in cleanup, I see qemuMonitorClose() is called where do you do not add
> any additional code to free the allocation.
> 
> This is what I see in cleanup code:
> 
> ```
> cleanup:
> /* We don't want the 'destroy' callback invoked during
>  * cleanup from construction failure, because that can
>  * give a double-unref on virDomainObj *in the caller,
>  * so kill the callbacks now.
>  */
> mon->cb = NULL;
> /* The caller owns 'fd' on failure */
> mon->fd = -1;
> qemuMonitorClose(mon);

qemuMonitorClose() eventually calls virObjectUnref(mon). Once the last
reference on the monitor object is removed the object is freed via
qemuMonitorDispose().

This patch has:

@@ -243,6 +244,7 @@ qemuMonitorDispose(void *obj)
 virCondDestroy(>notify);
 g_free(mon->buffer);
 g_free(mon->balloonpath);
+g_free(mon->domainName);
 }

Re: [PATCH v2] Add VM info to improve error log message for qemu monitor

[Ani Sinha]

On Tue, 4 Jan 2022, Rohit Kumar wrote:

>
> On 03/01/22 7:12 pm, Ani Sinha wrote:
> >
> > On Wed, 22 Dec 2021, Rohit Kumar wrote:
> >
> > > This patch is to determine the VM which had IO or socket hangup error.
> > > Accessing directly vm->def->name inside qemuMonitorIO() or
> > > qemuMonitorSend()
> > > might leads to illegal access as we are out of 'vm' context and vm->def
> > > might
> > > not exist. Adding a field "domainName" inside mon object to access vm name
> > > and initialising it when creating mon object.
> > The commit message should desribe the purpose of the change, not how it is
> > done. That can be easily understood reading the code. As for why the
> > implementation follows a particular way, we can explain it in the code
> > comments.
> >
> > This change adds the domain name in the error and debug logs during
> > monitor IO processing so that we may infer which VM experienced those
> > errors etc. This may help in debugging monitor IO errors. IMHO this
> > decription is enough.
> Thanks Ani, I will update the commit message in the next patch.
> >
> > > Signed-off-by: Rohit Kumar 
> > > ---
> > >   diff to v1:
> > >- Adding a field domainName inside _qemuMonitor struct for accessing vm
> > > name
> > >  instead of directly accessing mon->vm->def->name.
> > >- Link to v1:
> > > https://urldefense.proofpoint.com/v2/url?u=https-3A__listman.redhat.com_archives_libvir-2Dlist_2021-2DDecember_msg00217.html=DwIBAg=s883GpUCOChKOHiocYtGcg=ABSkr7gy7ZTfApFfI-Xxt1gZNtsDDiXoXOXc0OrkyFs=W2jEgk2s_xjLOLC-j8lPBnwVgteHS0-yYEGMqin16nmfzndzNgBE9mEvtbvyVzQo=Sa2q9edb4cvXo-EUriIPwlJxNo-itpp6--ZxY7rla2U=
> > >- Talked with peter on RFC and he suggested me to add a field inside
> > >  monitor struct to get VM name.
> > >
> > >   src/qemu/qemu_monitor.c | 47 +
> > >   1 file changed, 29 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> > > index dda6ae9796..c3a0227795 100644
> > > --- a/src/qemu/qemu_monitor.c
> > > +++ b/src/qemu/qemu_monitor.c
> > > @@ -80,6 +80,7 @@ struct _qemuMonitor {
> > >   GSource *watch;
> > >
> > >   virDomainObj *vm;
> > > +char *domainName;
> > >
> > Here we can explain why we are adding an extra member as opposed to
> > referencing it directly.
> Right, we can access it directly.
> >
> > >   qemuMonitorCallbacks *cb;
> > >   void *callbackOpaque;
> > > @@ -243,6 +244,7 @@ qemuMonitorDispose(void *obj)
> > >   virCondDestroy(>notify);
> > >   g_free(mon->buffer);
> > >   g_free(mon->balloonpath);
> > > +g_free(mon->domainName);
> > >   }
> > >
> > >
> > > @@ -530,13 +532,18 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
> > >   qemuMonitor *mon = opaque;
> > >   bool error = false;
> > >   bool hangup = false;
> > > +virDomainObj *vm = NULL;
> > > +char *vmName = NULL;
> > >
> > These initializations are redundant since ...
> Acknowledged.
> > >   virObjectRef(mon);
> > >
> > > +vm = mon->vm;
> > > +vmName = mon->domainName;
> > you initialize them here anyway.
> I will remove these in next patch.
> >
> > > +
> > >   /* lock access to the monitor and protect fd */
> > >   virObjectLock(mon);
> > >   #if DEBUG_IO
> > > -VIR_DEBUG("Monitor %p I/O on socket %p cond %d", mon, socket, cond);
> > > +VIR_DEBUG("Monitor %p I/O on socket %p cond %d vm=%p name=%s", mon,
> > > socket, cond, vm, NULLSTR(vmName));
> > >   #endif
> > >   if (mon->fd == -1 || !mon->watch) {
> > >   virObjectUnlock(mon);
> > > @@ -583,8 +590,8 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
> > >
> > >   if (!error && !mon->goteof &&
> > >   cond & G_IO_ERR) {
> > > -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > > -   _("Invalid file descriptor while waiting for
> > > monitor"));
> > > +virReportError(VIR_ERR_INTERNAL_ERROR,
> > > +   _("%s: Invalid file descriptor while waiting
> > > for monitor"), NULLSTR(vmName));
> > As per
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__libvirt.org_coding-2Dstyle.html-23error-2Dmessage-2Dformat=DwIBAg=s883GpUCOChKOHiocYtGcg=ABSkr7gy7ZTfApFfI-Xxt1gZNtsDDiXoXOXc0OrkyFs=W2jEgk2s_xjLOLC-j8lPBnwVgteHS0-yYEGMqin16nmfzndzNgBE9mEvtbvyVzQo=zU-TBR-xTsKRXEW5gdgL0WRRYBg7B7cbLluu65_PfkI=
> > , please
> > enclose %s within quotes:
> > _("'%s': Invalid file descriptor while waiting for monitor")
> Ack. Thanks.
> > >   mon->goteof = true;
> > >   }
> > >   }
> > > @@ -609,13 +616,14 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
> > >   virResetLastError();
> > >   } else {
> > >   if (virGetLastErrorCode() == VIR_ERR_OK && !mon->goteof)
> > > -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > > -   _("Error while processing monitor IO"));
> > > +

Re: [PATCH v2 2/3] qemuxml2xmloutdata: Turn tpm-*.xml files into symlinks

[Peter Krempa]

On Tue, Jan 04, 2022 at 10:04:59 +0100, Michal Privoznik wrote:
> Make the tpm-*.xml files symlinks to their respective input XMLs
> from qemuxml2argvdata/ directory. Neither of the XMLs relies on
> autofill of any TPM data.
> 
> Signed-off-by: Michal Privoznik 
> ---

Reviewed-by: Peter Krempa 

[PATCH v2 3/3] conf: Make virDomainTPMDefFormat() return void

[Michal Privoznik]

The virDomainTPMDefFormat() function can't fail really. There's
no point in it returning an integer then. Make it return void and
fix both places which check for its retval.

And while at it, turn @def into a const pointer to make it
obvious the function does not modify passed struct.

Signed-off-by: Michal Privoznik 
Reviewed-by: Peter Krempa 
---
 src/conf/domain_conf.c | 12 +---
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9e854d031e..fe53a280d7 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -25490,9 +25490,9 @@ virDomainSoundCodecDefFormat(virBuffer *buf,
 return 0;
 }
 
-static int
+static void
 virDomainTPMDefFormat(virBuffer *buf,
-  virDomainTPMDef *def,
+  const virDomainTPMDef *def,
   unsigned int flags)
 {
 g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
@@ -25543,8 +25543,6 @@ virDomainTPMDefFormat(virBuffer *buf,
 virDomainDeviceInfoFormat(, >info, flags);
 
 virXMLFormatElement(buf, "tpm", , );
-
-return 0;
 }
 
 
@@ -28531,8 +28529,7 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def,
 }
 
 for (n = 0; n < def->ntpms; n++) {
-if (virDomainTPMDefFormat(buf, def->tpms[n], flags) < 0)
-return -1;
+virDomainTPMDefFormat(buf, def->tpms[n], flags);
 }
 
 for (n = 0; n < def->ngraphics; n++) {
@@ -29762,7 +29759,8 @@ virDomainDeviceDefCopy(virDomainDeviceDef *src,
 rc = virDomainChrDefFormat(, src->data.chr, flags);
 break;
 case VIR_DOMAIN_DEVICE_TPM:
-rc = virDomainTPMDefFormat(, src->data.tpm, flags);
+virDomainTPMDefFormat(, src->data.tpm, flags);
+rc = 0;
 break;
 case VIR_DOMAIN_DEVICE_PANIC:
 virDomainPanicDefFormat(, src->data.panic);
-- 
2.34.1

[PATCH v2 2/3] qemuxml2xmloutdata: Turn tpm-*.xml files into symlinks

[Michal Privoznik]

Make the tpm-*.xml files symlinks to their respective input XMLs
from qemuxml2argvdata/ directory. Neither of the XMLs relies on
autofill of any TPM data.

Signed-off-by: Michal Privoznik 
---
 .../tpm-emulator-tpm2-enc.xml | 12 -
 .../tpm-emulator-tpm2-pstate.xml  | 12 -
 tests/qemuxml2argvdata/tpm-emulator-tpm2.xml  | 13 +-
 tests/qemuxml2argvdata/tpm-emulator.xml   | 12 -
 .../qemuxml2argvdata/tpm-passthrough-crb.xml  | 12 -
 tests/qemuxml2argvdata/tpm-passthrough.xml| 12 -
 .../tpm-emulator-tpm2-enc.x86_64-latest.xml   | 41 +
 ...tpm-emulator-tpm2-pstate.x86_64-latest.xml | 39 +---
 .../tpm-emulator-tpm2.x86_64-latest.xml   | 44 +--
 .../tpm-emulator.x86_64-latest.xml| 39 +---
 .../tpm-passthrough-crb.x86_64-latest.xml | 41 +
 .../tpm-passthrough.x86_64-latest.xml | 41 +
 12 files changed, 67 insertions(+), 251 deletions(-)
 mode change 100644 => 12 
tests/qemuxml2xmloutdata/tpm-emulator-tpm2-enc.x86_64-latest.xml
 mode change 100644 => 12 
tests/qemuxml2xmloutdata/tpm-emulator-tpm2-pstate.x86_64-latest.xml
 mode change 100644 => 12 
tests/qemuxml2xmloutdata/tpm-emulator-tpm2.x86_64-latest.xml
 mode change 100644 => 12 
tests/qemuxml2xmloutdata/tpm-emulator.x86_64-latest.xml
 mode change 100644 => 12 
tests/qemuxml2xmloutdata/tpm-passthrough-crb.x86_64-latest.xml
 mode change 100644 => 12 
tests/qemuxml2xmloutdata/tpm-passthrough.x86_64-latest.xml

diff --git a/tests/qemuxml2argvdata/tpm-emulator-tpm2-enc.xml 
b/tests/qemuxml2argvdata/tpm-emulator-tpm2-enc.xml
index d889aae4f6..9c2279b28b 100644
--- a/tests/qemuxml2argvdata/tpm-emulator-tpm2-enc.xml
+++ b/tests/qemuxml2argvdata/tpm-emulator-tpm2-enc.xml
@@ -12,13 +12,18 @@
   
 
   
+  
+qemu64
+  
   
   destroy
   restart
   destroy
   
 /usr/bin/qemu-system-x86_64
-
+
+  
+
 
 
 
@@ -27,6 +32,9 @@
 
   
 
-
+
+
+  
+
   
 
diff --git a/tests/qemuxml2argvdata/tpm-emulator-tpm2-pstate.xml 
b/tests/qemuxml2argvdata/tpm-emulator-tpm2-pstate.xml
index 45fc4c0e1a..42e93cfcbe 100644
--- a/tests/qemuxml2argvdata/tpm-emulator-tpm2-pstate.xml
+++ b/tests/qemuxml2argvdata/tpm-emulator-tpm2-pstate.xml
@@ -12,19 +12,27 @@
   
 
   
+  
+qemu64
+  
   
   destroy
   restart
   destroy
   
 /usr/bin/qemu-system-x86_64
-
+
+  
+
 
 
 
 
   
 
-
+
+
+  
+
   
 
diff --git a/tests/qemuxml2argvdata/tpm-emulator-tpm2.xml 
b/tests/qemuxml2argvdata/tpm-emulator-tpm2.xml
index 68db8b9232..79acde218b 100644
--- a/tests/qemuxml2argvdata/tpm-emulator-tpm2.xml
+++ b/tests/qemuxml2argvdata/tpm-emulator-tpm2.xml
@@ -12,24 +12,33 @@
   
 
   
+  
+qemu64
+  
   
   destroy
   restart
   destroy
   
 /usr/bin/qemu-system-x86_64
-
+
+  
+
 
 
 
 
   
+
 
   
   
 
   
 
-
+
+
+  
+
   
 
diff --git a/tests/qemuxml2argvdata/tpm-emulator.xml 
b/tests/qemuxml2argvdata/tpm-emulator.xml
index defc3789ad..b98a3693b7 100644
--- a/tests/qemuxml2argvdata/tpm-emulator.xml
+++ b/tests/qemuxml2argvdata/tpm-emulator.xml
@@ -12,19 +12,27 @@
   
 
   
+  
+qemu64
+  
   
   destroy
   restart
   destroy
   
 /usr/bin/qemu-system-x86_64
-
+
+  
+
 
 
 
 
   
 
-
+
+
+  
+
   
 
diff --git a/tests/qemuxml2argvdata/tpm-passthrough-crb.xml 
b/tests/qemuxml2argvdata/tpm-passthrough-crb.xml
index 2fce5ca342..47c622bd84 100644
--- a/tests/qemuxml2argvdata/tpm-passthrough-crb.xml
+++ b/tests/qemuxml2argvdata/tpm-passthrough-crb.xml
@@ -12,13 +12,18 @@
   
 
   
+  
+qemu64
+  
   
   destroy
   restart
   destroy
   
 /usr/bin/qemu-system-x86_64
-
+
+  
+
 
 
 
@@ -27,6 +32,9 @@
 
   
 
-
+
+
+  
+
   
 
diff --git a/tests/qemuxml2argvdata/tpm-passthrough.xml 
b/tests/qemuxml2argvdata/tpm-passthrough.xml
index 036091d44f..1555de4e86 100644
--- a/tests/qemuxml2argvdata/tpm-passthrough.xml
+++ b/tests/qemuxml2argvdata/tpm-passthrough.xml
@@ -12,13 +12,18 @@
   
 
   
+  
+qemu64
+  
   
   destroy
   restart
   destroy
   
 /usr/bin/qemu-system-x86_64
-
+
+  
+
 
 
 
@@ -27,6 +32,9 @@
 
   
 
-
+
+
+  
+
   
 
diff --git a/tests/qemuxml2xmloutdata/tpm-emulator-tpm2-enc.x86_64-latest.xml 
b/tests/qemuxml2xmloutdata/tpm-emulator-tpm2-enc.x86_64-latest.xml
deleted file mode 100644
index 9c2279b28b..00
--- a/tests/qemuxml2xmloutdata/tpm-emulator-tpm2-enc.x86_64-latest.xml
+++ /dev/null
@@ -1,40 +0,0 @@
-
-  TPM-VM
-  11d7cd22-da89-3094-6212-079a48a309a1
-  2097152
-  512288
-  1
-  
-hvm
-
-
-  
-  
-
-  
-  

[PATCH v2 0/3] Rework formatting

[Michal Privoznik]

v2 of:

https://listman.redhat.com/archives/libvir-list/2022-January/msg00047.html

diff to v1:
- Pushed 1/4 from the original series, because it was acked and
independent of the rest.
- Swapped two patches to make the test suite pass after each single
patch.
- Renamed variable in 1/3 (3/4 in the original series) per Peter's
suggestion.

Michal Prívozník (3):
  conf: Rework  formatting
  qemuxml2xmloutdata: Turn tpm-*.xml files into symlinks
  conf: Make virDomainTPMDefFormat() return void

 src/conf/domain_conf.c| 65 ---
 .../tpm-emulator-tpm2-enc.xml | 12 +++-
 .../tpm-emulator-tpm2-pstate.xml  | 12 +++-
 tests/qemuxml2argvdata/tpm-emulator-tpm2.xml  | 13 +++-
 tests/qemuxml2argvdata/tpm-emulator.xml   | 12 +++-
 .../qemuxml2argvdata/tpm-passthrough-crb.xml  | 12 +++-
 tests/qemuxml2argvdata/tpm-passthrough.xml| 12 +++-
 .../tpm-emulator-tpm2-enc.x86_64-latest.xml   | 41 +---
 ...tpm-emulator-tpm2-pstate.x86_64-latest.xml | 39 +--
 .../tpm-emulator-tpm2.x86_64-latest.xml   | 44 +
 .../tpm-emulator.x86_64-latest.xml| 39 +--
 .../tpm-passthrough-crb.x86_64-latest.xml | 41 +---
 .../tpm-passthrough.x86_64-latest.xml | 41 +---
 13 files changed, 94 insertions(+), 289 deletions(-)
 mode change 100644 => 12 
tests/qemuxml2xmloutdata/tpm-emulator-tpm2-enc.x86_64-latest.xml
 mode change 100644 => 12 
tests/qemuxml2xmloutdata/tpm-emulator-tpm2-pstate.x86_64-latest.xml
 mode change 100644 => 12 
tests/qemuxml2xmloutdata/tpm-emulator-tpm2.x86_64-latest.xml
 mode change 100644 => 12 
tests/qemuxml2xmloutdata/tpm-emulator.x86_64-latest.xml
 mode change 100644 => 12 
tests/qemuxml2xmloutdata/tpm-passthrough-crb.x86_64-latest.xml
 mode change 100644 => 12 
tests/qemuxml2xmloutdata/tpm-passthrough.x86_64-latest.xml

-- 
2.34.1

[PATCH v2 1/3] conf: Rework formatting

[Michal Privoznik]

The  element formatting is handled in
virDomainTPMDefFormat() which uses the "old style" - appending
strings directly into the output buffer. With this, it's easy to
get conditions that tell when an element has ended wrong. In this
particular case, if both  and 
are to be formatted the current code puts a stray '>' into the
output buffer, resulting in invalid XML.

Rewrite the function to use virXMLFormatElement() which is more
clever.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2016599#c15
Signed-off-by: Michal Privoznik 
Reviewed-by: Peter Krempa 
---
 src/conf/domain_conf.c | 53 ++
 1 file changed, 22 insertions(+), 31 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index bba662bf4c..9e854d031e 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -25495,63 +25495,54 @@ virDomainTPMDefFormat(virBuffer *buf,
   virDomainTPMDef *def,
   unsigned int flags)
 {
-virBufferAsprintf(buf, "\n",
+g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
+g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
+g_auto(virBuffer) backendAttrBuf = VIR_BUFFER_INITIALIZER;
+g_auto(virBuffer) backendChildBuf = VIR_BUFFER_INIT_CHILD();
+
+virBufferAsprintf(, " model='%s'",
   virDomainTPMModelTypeToString(def->model));
-virBufferAdjustIndent(buf, 2);
-virBufferAsprintf(buf, "type));
 
 switch (def->type) {
 case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
-virBufferAddLit(buf, ">\n");
-virBufferAdjustIndent(buf, 2);
-virBufferEscapeString(buf, "\n",
+virBufferEscapeString(, "\n",
   def->data.passthrough.source->data.file.path);
-virBufferAdjustIndent(buf, -2);
-virBufferAddLit(buf, "\n");
 break;
 case VIR_DOMAIN_TPM_TYPE_EMULATOR:
-virBufferAsprintf(buf, " version='%s'",
+virBufferAsprintf(, " version='%s'",
   virDomainTPMVersionTypeToString(def->version));
 if (def->data.emulator.persistent_state)
-virBufferAddLit(buf, " persistent_state='yes'");
+virBufferAddLit(, " persistent_state='yes'");
 if (def->data.emulator.hassecretuuid) {
 char uuidstr[VIR_UUID_STRING_BUFLEN];
-virBufferAddLit(buf, ">\n");
-virBufferAdjustIndent(buf, 2);
-virBufferAsprintf(buf, "\n",
-virUUIDFormat(def->data.emulator.secretuuid, uuidstr));
-virBufferAdjustIndent(buf, -2);
+
+virBufferAsprintf(, "\n",
+  virUUIDFormat(def->data.emulator.secretuuid, 
uuidstr));
 }
 if (def->data.emulator.activePcrBanks) {
+g_auto(virBuffer) activePcrBanksBuf = 
VIR_BUFFER_INIT_CHILD();
 size_t i;
-virBufferAddLit(buf, ">\n");
-virBufferAdjustIndent(buf, 2);
-virBufferAddLit(buf, "\n");
-virBufferAdjustIndent(buf, 2);
+
 for (i = VIR_DOMAIN_TPM_PCR_BANK_SHA1; i < 
VIR_DOMAIN_TPM_PCR_BANK_LAST; i++) {
 if ((def->data.emulator.activePcrBanks & (1 << i)))
-virBufferAsprintf(buf, "<%s/>\n",
+virBufferAsprintf(, "<%s/>\n",
   virDomainTPMPcrBankTypeToString(i));
 }
-virBufferAdjustIndent(buf, -2);
-virBufferAddLit(buf, "\n");
-virBufferAdjustIndent(buf, -2);
+
+virXMLFormatElement(, "active_pcr_banks", NULL, 
);
 }
-if (def->data.emulator.hassecretuuid ||
-def->data.emulator.activePcrBanks)
-virBufferAddLit(buf, "\n");
-else
-virBufferAddLit(buf, "/>\n");
 break;
 case VIR_DOMAIN_TPM_TYPE_LAST:
 break;
 }
 
-virDomainDeviceInfoFormat(buf, >info, flags);
+virXMLFormatElement(, "backend", , 
);
+virDomainDeviceInfoFormat(, >info, flags);
 
-virBufferAdjustIndent(buf, -2);
-virBufferAddLit(buf, "\n");
+virXMLFormatElement(buf, "tpm", , );
 
 return 0;
 }
-- 
2.34.1

Re: [PATCH v2] Add VM info to improve error log message for qemu monitor

[Rohit Kumar]

On 04/01/22 2:17 pm, Peter Krempa wrote:

On Tue, Jan 04, 2022 at 09:45:30 +0100, Peter Krempa wrote:

On Tue, Jan 04, 2022 at 14:10:49 +0530, Rohit Kumar wrote:

On 03/01/22 10:12 pm, Peter Krempa wrote:

On Wed, Dec 22, 2021 at 22:39:21 -0800, Rohit Kumar wrote:

This patch is to determine the VM which had IO or socket hangup error.
Accessing directly vm->def->name inside qemuMonitorIO() or qemuMonitorSend()
might leads to illegal access as we are out of 'vm' context and vm->def might
not exist. Adding a field "domainName" inside mon object to access vm name
and initialising it when creating mon object.

Signed-off-by: Rohit Kumar 
---

[...]


@@ -609,13 +616,14 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
   virResetLastError();
   } else {
   if (virGetLastErrorCode() == VIR_ERR_OK && !mon->goteof)
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Error while processing monitor IO"));
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("%s: Error while processing monitor IO"), 
NULLSTR(vmName));

Same here. Additionally did you ever get to a situation where vmName
would be NULL?

There might be a situation when g_strdup() fails to allocate vm name, for
e.g. if host ran out of memory ?

No, g_strdup on out of memory condition abort()s the program completely.

The only time when g_strdup returns NULL is if the argument is NULL.


Let me know your thoughts on this, I would be happy to remove NULLSTR if
it's not the case.

That depends if you happened to see whether all callers avoid passing
NULL name for the VM which is very likely.

Alternatively you can do g_strdup(NULLSTR(vm->def->name)). Thus the
domain name variable will hold a copy of "" as the name. Since
it's for error messages only this is tolerable and allows you to avoid
all the other NULLSTR usage.
Sure, this would be much better, I will update this. Thanks for the 
suggestion.

Re: [PATCH 4/4] conf: Make virDomainTPMDefFormat() return void

[Peter Krempa]

On Tue, Jan 04, 2022 at 09:14:30 +0100, Michal Privoznik wrote:
> The virDomainTPMDefFormat() function can't fail really. There's
> no point in it returning an integer then. Make it return void and
> fix both places which check for its retval.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/conf/domain_conf.c | 12 +---
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index b8fef8586c..509f74c092 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -25476,9 +25476,9 @@ virDomainSoundCodecDefFormat(virBuffer *buf,
>  return 0;
>  }
>  
> -static int
> +static void
>  virDomainTPMDefFormat(virBuffer *buf,
> -  virDomainTPMDef *def,
> +  const virDomainTPMDef *def,

This isn't accounted for in the commit message.

>unsigned int flags)
>  {
>  g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
> @@ -25529,8 +25529,6 @@ virDomainTPMDefFormat(virBuffer *buf,
>  virDomainDeviceInfoFormat(, >info, flags);
>  
>  virXMLFormatElement(buf, "tpm", , );
> -
> -return 0;
>  }

Reviewed-by: Peter Krempa 

Re: [PATCH 3/4] conf: Rework formatting

[Peter Krempa]

On Tue, Jan 04, 2022 at 09:14:29 +0100, Michal Privoznik wrote:
> The  element formatting is handled in
> virDomainTPMDefFormat() which uses the "old style" - appending
> strings directly into the output buffer. With this, it's easy to
> get conditions that tell when an element has ended wrong. In this
> particular case, if both  and 
> are to be formatted the current code puts a stray '>' into the
> output buffer, resulting in invalid XML.
> 
> Rewrite the function to use virXMLFormatElement() which is more
> clever.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=2016599#c15
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/conf/domain_conf.c   | 53 
>  tests/qemuxml2argvdata/tpm-emulator-tpm2.xml |  1 -
>  2 files changed, 22 insertions(+), 32 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 716c6d2240..b8fef8586c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -25481,63 +25481,54 @@ virDomainTPMDefFormat(virBuffer *buf,
>virDomainTPMDef *def,
>unsigned int flags)
>  {
> -virBufferAsprintf(buf, "\n",
> +g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
> +g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
> +g_auto(virBuffer) backendAttrBuf = VIR_BUFFER_INITIALIZER;
> +g_auto(virBuffer) backendBuf = VIR_BUFFER_INIT_CHILD();

Cannonically this would be 'backendChildBuf'.

[...]

Above code:
Reviewed-by: Peter Krempa 

> diff --git a/tests/qemuxml2argvdata/tpm-emulator-tpm2.xml 
> b/tests/qemuxml2argvdata/tpm-emulator-tpm2.xml
> index 59dd68311f..79acde218b 100644
> --- a/tests/qemuxml2argvdata/tpm-emulator-tpm2.xml
> +++ b/tests/qemuxml2argvdata/tpm-emulator-tpm2.xml
> @@ -30,7 +30,6 @@
>  
>
>  
> -  >
>  
>
>
> -- 
> 2.34.1
> 

Re: [PATCH v2] Add VM info to improve error log message for qemu monitor

[Peter Krempa]

On Tue, Jan 04, 2022 at 09:45:30 +0100, Peter Krempa wrote:
> On Tue, Jan 04, 2022 at 14:10:49 +0530, Rohit Kumar wrote:
> > 
> > On 03/01/22 10:12 pm, Peter Krempa wrote:
> > > On Wed, Dec 22, 2021 at 22:39:21 -0800, Rohit Kumar wrote:
> > > > This patch is to determine the VM which had IO or socket hangup error.
> > > > Accessing directly vm->def->name inside qemuMonitorIO() or 
> > > > qemuMonitorSend()
> > > > might leads to illegal access as we are out of 'vm' context and vm->def 
> > > > might
> > > > not exist. Adding a field "domainName" inside mon object to access vm 
> > > > name
> > > > and initialising it when creating mon object.
> > > > 
> > > > Signed-off-by: Rohit Kumar 
> > > > ---
> 
> [...]
> 
> > > > @@ -609,13 +616,14 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
> > > >   virResetLastError();
> > > >   } else {
> > > >   if (virGetLastErrorCode() == VIR_ERR_OK && !mon->goteof)
> > > > -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > > > -   _("Error while processing monitor IO"));
> > > > +virReportError(VIR_ERR_INTERNAL_ERROR,
> > > > +   _("%s: Error while processing monitor 
> > > > IO"), NULLSTR(vmName));
> > > Same here. Additionally did you ever get to a situation where vmName
> > > would be NULL?
> > There might be a situation when g_strdup() fails to allocate vm name, for
> > e.g. if host ran out of memory ?
> 
> No, g_strdup on out of memory condition abort()s the program completely.
> 
> The only time when g_strdup returns NULL is if the argument is NULL.
> 
> > Let me know your thoughts on this, I would be happy to remove NULLSTR if
> > it's not the case.
> 
> That depends if you happened to see whether all callers avoid passing
> NULL name for the VM which is very likely.

Alternatively you can do g_strdup(NULLSTR(vm->def->name)). Thus the
domain name variable will hold a copy of "" as the name. Since
it's for error messages only this is tolerable and allows you to avoid
all the other NULLSTR usage.

Re: [PATCH v2] Add VM info to improve error log message for qemu monitor

[Peter Krempa]

On Tue, Jan 04, 2022 at 14:10:49 +0530, Rohit Kumar wrote:
> 
> On 03/01/22 10:12 pm, Peter Krempa wrote:
> > On Wed, Dec 22, 2021 at 22:39:21 -0800, Rohit Kumar wrote:
> > > This patch is to determine the VM which had IO or socket hangup error.
> > > Accessing directly vm->def->name inside qemuMonitorIO() or 
> > > qemuMonitorSend()
> > > might leads to illegal access as we are out of 'vm' context and vm->def 
> > > might
> > > not exist. Adding a field "domainName" inside mon object to access vm name
> > > and initialising it when creating mon object.
> > > 
> > > Signed-off-by: Rohit Kumar 
> > > ---

[...]

> > > @@ -609,13 +616,14 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
> > >   virResetLastError();
> > >   } else {
> > >   if (virGetLastErrorCode() == VIR_ERR_OK && !mon->goteof)
> > > -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > > -   _("Error while processing monitor IO"));
> > > +virReportError(VIR_ERR_INTERNAL_ERROR,
> > > +   _("%s: Error while processing monitor 
> > > IO"), NULLSTR(vmName));
> > Same here. Additionally did you ever get to a situation where vmName
> > would be NULL?
> There might be a situation when g_strdup() fails to allocate vm name, for
> e.g. if host ran out of memory ?

No, g_strdup on out of memory condition abort()s the program completely.

The only time when g_strdup returns NULL is if the argument is NULL.

> Let me know your thoughts on this, I would be happy to remove NULLSTR if
> it's not the case.

That depends if you happened to see whether all callers avoid passing
NULL name for the VM which is very likely.

Re: [PATCH v2] Add VM info to improve error log message for qemu monitor

[Rohit Kumar]

On 03/01/22 10:12 pm, Peter Krempa wrote:

On Wed, Dec 22, 2021 at 22:39:21 -0800, Rohit Kumar wrote:

This patch is to determine the VM which had IO or socket hangup error.
Accessing directly vm->def->name inside qemuMonitorIO() or qemuMonitorSend()
might leads to illegal access as we are out of 'vm' context and vm->def might
not exist. Adding a field "domainName" inside mon object to access vm name
and initialising it when creating mon object.

Signed-off-by: Rohit Kumar 
---
  diff to v1:
   - Adding a field domainName inside _qemuMonitor struct for accessing vm name
 instead of directly accessing mon->vm->def->name.
   - Link to v1: 
https://urldefense.proofpoint.com/v2/url?u=https-3A__listman.redhat.com_archives_libvir-2Dlist_2021-2DDecember_msg00217.html=DwIBAg=s883GpUCOChKOHiocYtGcg=ABSkr7gy7ZTfApFfI-Xxt1gZNtsDDiXoXOXc0OrkyFs=Zx9EFUnjB-Vnos3Ycje0GLa92yVGi-GQA7I5kFT9ll8jJUujvx-k2cEpTTb40KZW=KjRU2euJOCStdmi2A-a_axYFby40T-oNyr6kApd0nYI=
   - Talked with peter on RFC and he suggested me to add a field inside
 monitor struct to get VM name.
  
  src/qemu/qemu_monitor.c | 47 +

  1 file changed, 29 insertions(+), 18 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index dda6ae9796..c3a0227795 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c

[...]


@@ -530,13 +532,18 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
  qemuMonitor *mon = opaque;
  bool error = false;
  bool hangup = false;
+virDomainObj *vm = NULL;
+char *vmName = NULL;

These local variables are not very useful, you can access 'mon' directly
without the need for this alias ...

Thanks for the review Peter. I will update it in the next patch.


  
  virObjectRef(mon);
  
+vm = mon->vm;

+vmName = mon->domainName;
+
  /* lock access to the monitor and protect fd */
  virObjectLock(mon);
  #if DEBUG_IO
-VIR_DEBUG("Monitor %p I/O on socket %p cond %d", mon, socket, cond);
+VIR_DEBUG("Monitor %p I/O on socket %p cond %d vm=%p name=%s", mon, 
socket, cond, vm, NULLSTR(vmName));
  #endif

There's no much point in adding to thhese debug statements (inside
DEBUG_IO) as they are not compiled by default. In fact I'll propose a
removal of those.

Okay. I will remove these from here.



  if (mon->fd == -1 || !mon->watch) {
  virObjectUnlock(mon);
@@ -583,8 +590,8 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
  
  if (!error && !mon->goteof &&

  cond & G_IO_ERR) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Invalid file descriptor while waiting for 
monitor"));
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("%s: Invalid file descriptor while waiting for 
monitor"), NULLSTR(vmName));

Do not prefix the error message by the VM name, but rather put it at the
end or inside:

  Invalid file descriptor while waiting for monitor (vm='%s')

or smilar.

Acknowledge. Will update in next patch. Thanks.



  mon->goteof = true;
  }
  }
@@ -609,13 +616,14 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
  virResetLastError();
  } else {
  if (virGetLastErrorCode() == VIR_ERR_OK && !mon->goteof)
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Error while processing monitor IO"));
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("%s: Error while processing monitor IO"), 
NULLSTR(vmName));

Same here. Additionally did you ever get to a situation where vmName
would be NULL?
There might be a situation when g_strdup() fails to allocate vm name, 
for e.g. if host ran out of memory ?
Let me know your thoughts on this, I would be happy to remove NULLSTR if 
it's not the case.



  virCopyLastError(>lastError);
  virResetLastError();
  }
  
-VIR_DEBUG("Error on monitor %s", NULLSTR(mon->lastError.message));

+VIR_DEBUG("Error on monitor %s mon=%p vm=%p name=%s",
+  NULLSTR(mon->lastError.message), mon, vm, NULLSTR(vmName));

This is a good example.

Okay.



  /* If IO process resulted in an error & we have a message,
   * then wakeup that waiter */
  if (mon->msg && !mon->msg->finished) {
@@ -631,22 +639,22 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
   * will try to acquire the virDomainObj *mutex next */
  if (mon->goteof) {
  qemuMonitorEofNotifyCallback eofNotify = mon->cb->eofNotify;
-virDomainObj *vm = mon->vm;

You can remove this local when you are accessing it directly.

Sure. Ack.


  
  /* Make sure anyone waiting wakes up now */

  virCondSignal(>notify);
  virObjectUnlock(mon);
-VIR_DEBUG("Triggering EOF callback");
+VIR_DEBUG("Triggering EOF callback mon=%p vm=%p name=%s",
+  mon, vm, 

Re: [PATCH 1/4] qemuxml2xmltest: Introduce tpm-emulator-spapr test

[Peter Krempa]

On Tue, Jan 04, 2022 at 09:14:27 +0100, Michal Privoznik wrote:
> We already have the input xml because of xml2arg test. However,
> the corresponding xml2xml test case is missing. Make the expected
> XML a symlink to the input XML and clean the latter up a bit.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  tests/qemuxml2argvdata/tpm-emulator-spapr.xml | 74 +++
>  .../tpm-emulator-spapr.ppc64-latest.xml   |  1 +
>  tests/qemuxml2xmltest.c   |  1 +
>  3 files changed, 44 insertions(+), 32 deletions(-)
>  create mode 12 
> tests/qemuxml2xmloutdata/tpm-emulator-spapr.ppc64-latest.xml

Reviewed-by: Peter Krempa 

Re: [PATCH] conf: Extend TPM ABI stability check for

[Peter Krempa]

On Tue, Jan 04, 2022 at 09:29:15 +0100, Michal Privoznik wrote:
> Changing  means changing the guest ABI and as
> such must be prevented on both restoring from a file or
> migration.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2035888
> Signed-off-by: Michal Privoznik 
> ---
>  src/conf/domain_conf.c | 14 ++
>  1 file changed, 14 insertions(+)

Reviewed-by: Peter Krempa 

Re: [PATCH 2/4] qemuxml2xmloutdata: Turn tpm-*.xml files into symlinks

[Michal Prívozník]

On 1/4/22 09:33, Peter Krempa wrote:

> https://www.libvirt.org/hacking.html#preparing-patches
> 
> "If you're going to submit multiple patches, the automated tests must
> pass after each patch, not just after the last one."
> 

Alright then. Let me swap those two patches and post v2.

Michal

Re: [PATCH 2/4] qemuxml2xmloutdata: Turn tpm-*.xml files into symlinks

[Peter Krempa]

On Tue, Jan 04, 2022 at 09:31:39 +0100, Michal Prívozník wrote:
> On 1/4/22 09:30, Peter Krempa wrote:
> > On Tue, Jan 04, 2022 at 09:14:28 +0100, Michal Privoznik wrote:
> >> Make the tpm-*.xml files symlinks to their respective input XMLs
> >> from qemuxml2argvdata/ directory. This uncovers a bug in our
> >>  formatter which formats an invalid XML if both
> >>  and  elements are present for
> >> . This is going to be addressed in the next commit.
> >>
> >> Signed-off-by: Michal Privoznik 
> >> ---
> > 
> > [...]
> > 
> >> diff --git a/tests/qemuxml2argvdata/tpm-emulator-tpm2.xml 
> >> b/tests/qemuxml2argvdata/tpm-emulator-tpm2.xml
> >> index 68db8b9232..59dd68311f 100644
> >> --- a/tests/qemuxml2argvdata/tpm-emulator-tpm2.xml
> >> +++ b/tests/qemuxml2argvdata/tpm-emulator-tpm2.xml
> >> @@ -12,24 +12,34 @@
> >>
> >>  
> >>
> >> +  
> >> +qemu64
> >> +  
> >>
> >>destroy
> >>restart
> >>destroy
> >>
> >>  /usr/bin/qemu-system-x86_64
> >> -
> >> +
> >> +   >> function='0x2'/>
> >> +
> >>  
> >>  
> >>  
> >>  
> >>
> >> +
> >> +  >
> > 
> > 
> > Summary of Failures:
> > 
> > 253/315 libvirt / virschematest 
> >  FAIL1.47s   exit status 1
> 
> To cite from the cover letter:
> 
> Please note that the test suite is temporarily broken after 2/4 but
> fixed in 3/4. This could be resolved be swapping those two patches, but
> I figured I keep the order to demonstrate the bug. However, I can do the
> swap if desired.

https://www.libvirt.org/hacking.html#preparing-patches

"If you're going to submit multiple patches, the automated tests must
pass after each patch, not just after the last one."

Re: [PATCH v2] Add VM info to improve error log message for qemu monitor

[Rohit Kumar]

On 03/01/22 7:12 pm, Ani Sinha wrote:


On Wed, 22 Dec 2021, Rohit Kumar wrote:


This patch is to determine the VM which had IO or socket hangup error.
Accessing directly vm->def->name inside qemuMonitorIO() or qemuMonitorSend()
might leads to illegal access as we are out of 'vm' context and vm->def might
not exist. Adding a field "domainName" inside mon object to access vm name
and initialising it when creating mon object.

The commit message should desribe the purpose of the change, not how it is
done. That can be easily understood reading the code. As for why the
implementation follows a particular way, we can explain it in the code
comments.

This change adds the domain name in the error and debug logs during
monitor IO processing so that we may infer which VM experienced those
errors etc. This may help in debugging monitor IO errors. IMHO this
decription is enough.

Thanks Ani, I will update the commit message in the next patch.



Signed-off-by: Rohit Kumar 
---
  diff to v1:
   - Adding a field domainName inside _qemuMonitor struct for accessing vm name
 instead of directly accessing mon->vm->def->name.
   - Link to v1: 
https://urldefense.proofpoint.com/v2/url?u=https-3A__listman.redhat.com_archives_libvir-2Dlist_2021-2DDecember_msg00217.html=DwIBAg=s883GpUCOChKOHiocYtGcg=ABSkr7gy7ZTfApFfI-Xxt1gZNtsDDiXoXOXc0OrkyFs=W2jEgk2s_xjLOLC-j8lPBnwVgteHS0-yYEGMqin16nmfzndzNgBE9mEvtbvyVzQo=Sa2q9edb4cvXo-EUriIPwlJxNo-itpp6--ZxY7rla2U=
   - Talked with peter on RFC and he suggested me to add a field inside
 monitor struct to get VM name.

  src/qemu/qemu_monitor.c | 47 +
  1 file changed, 29 insertions(+), 18 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index dda6ae9796..c3a0227795 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -80,6 +80,7 @@ struct _qemuMonitor {
  GSource *watch;

  virDomainObj *vm;
+char *domainName;


Here we can explain why we are adding an extra member as opposed to
referencing it directly.

Right, we can access it directly.



  qemuMonitorCallbacks *cb;
  void *callbackOpaque;
@@ -243,6 +244,7 @@ qemuMonitorDispose(void *obj)
  virCondDestroy(>notify);
  g_free(mon->buffer);
  g_free(mon->balloonpath);
+g_free(mon->domainName);
  }


@@ -530,13 +532,18 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
  qemuMonitor *mon = opaque;
  bool error = false;
  bool hangup = false;
+virDomainObj *vm = NULL;
+char *vmName = NULL;


These initializations are redundant since ...

Acknowledged.

  virObjectRef(mon);

+vm = mon->vm;
+vmName = mon->domainName;

you initialize them here anyway.

I will remove these in next patch.



+
  /* lock access to the monitor and protect fd */
  virObjectLock(mon);
  #if DEBUG_IO
-VIR_DEBUG("Monitor %p I/O on socket %p cond %d", mon, socket, cond);
+VIR_DEBUG("Monitor %p I/O on socket %p cond %d vm=%p name=%s", mon, 
socket, cond, vm, NULLSTR(vmName));
  #endif
  if (mon->fd == -1 || !mon->watch) {
  virObjectUnlock(mon);
@@ -583,8 +590,8 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,

  if (!error && !mon->goteof &&
  cond & G_IO_ERR) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Invalid file descriptor while waiting for 
monitor"));
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("%s: Invalid file descriptor while waiting for 
monitor"), NULLSTR(vmName));

As per 
https://urldefense.proofpoint.com/v2/url?u=https-3A__libvirt.org_coding-2Dstyle.html-23error-2Dmessage-2Dformat=DwIBAg=s883GpUCOChKOHiocYtGcg=ABSkr7gy7ZTfApFfI-Xxt1gZNtsDDiXoXOXc0OrkyFs=W2jEgk2s_xjLOLC-j8lPBnwVgteHS0-yYEGMqin16nmfzndzNgBE9mEvtbvyVzQo=zU-TBR-xTsKRXEW5gdgL0WRRYBg7B7cbLluu65_PfkI=
 , please
enclose %s within quotes:
_("'%s': Invalid file descriptor while waiting for monitor")

Ack. Thanks.

  mon->goteof = true;
  }
  }
@@ -609,13 +616,14 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
  virResetLastError();
  } else {
  if (virGetLastErrorCode() == VIR_ERR_OK && !mon->goteof)
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Error while processing monitor IO"));
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("%s: Error while processing monitor IO"), 
NULLSTR(vmName));

Ditto.

Ack.

  virCopyLastError(>lastError);
  virResetLastError();
  }

-VIR_DEBUG("Error on monitor %s", NULLSTR(mon->lastError.message));
+VIR_DEBUG("Error on monitor %s mon=%p vm=%p name=%s",
+  NULLSTR(mon->lastError.message), mon, vm, NULLSTR(vmName));
  /* If IO process resulted in an error & we have a message,
   * then wakeup that waiter */
  if (mon->msg && !mon->msg->finished) {
@@ 

Re: [PATCH 2/4] qemuxml2xmloutdata: Turn tpm-*.xml files into symlinks

[Michal Prívozník]

On 1/4/22 09:30, Peter Krempa wrote:
> On Tue, Jan 04, 2022 at 09:14:28 +0100, Michal Privoznik wrote:
>> Make the tpm-*.xml files symlinks to their respective input XMLs
>> from qemuxml2argvdata/ directory. This uncovers a bug in our
>>  formatter which formats an invalid XML if both
>>  and  elements are present for
>> . This is going to be addressed in the next commit.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
> 
> [...]
> 
>> diff --git a/tests/qemuxml2argvdata/tpm-emulator-tpm2.xml 
>> b/tests/qemuxml2argvdata/tpm-emulator-tpm2.xml
>> index 68db8b9232..59dd68311f 100644
>> --- a/tests/qemuxml2argvdata/tpm-emulator-tpm2.xml
>> +++ b/tests/qemuxml2argvdata/tpm-emulator-tpm2.xml
>> @@ -12,24 +12,34 @@
>>
>>  
>>
>> +  
>> +qemu64
>> +  
>>
>>destroy
>>restart
>>destroy
>>
>>  /usr/bin/qemu-system-x86_64
>> -
>> +
>> +  > function='0x2'/>
>> +
>>  
>>  
>>  
>>  
>>
>> +
>> +  >
> 
> 
> Summary of Failures:
> 
> 253/315 libvirt / virschematest   
>FAIL1.47s   exit status 1

To cite from the cover letter:

Please note that the test suite is temporarily broken after 2/4 but
fixed in 3/4. This could be resolved be swapping those two patches, but
I figured I keep the order to demonstrate the bug. However, I can do the
swap if desired.

Michal

Re: [PATCH 2/4] qemuxml2xmloutdata: Turn tpm-*.xml files into symlinks

[Peter Krempa]

On Tue, Jan 04, 2022 at 09:14:28 +0100, Michal Privoznik wrote:
> Make the tpm-*.xml files symlinks to their respective input XMLs
> from qemuxml2argvdata/ directory. This uncovers a bug in our
>  formatter which formats an invalid XML if both
>  and  elements are present for
> . This is going to be addressed in the next commit.
> 
> Signed-off-by: Michal Privoznik 
> ---

[...]

> diff --git a/tests/qemuxml2argvdata/tpm-emulator-tpm2.xml 
> b/tests/qemuxml2argvdata/tpm-emulator-tpm2.xml
> index 68db8b9232..59dd68311f 100644
> --- a/tests/qemuxml2argvdata/tpm-emulator-tpm2.xml
> +++ b/tests/qemuxml2argvdata/tpm-emulator-tpm2.xml
> @@ -12,24 +12,34 @@
>
>  
>
> +  
> +qemu64
> +  
>
>destroy
>restart
>destroy
>
>  /usr/bin/qemu-system-x86_64
> -
> +
> +   function='0x2'/>
> +
>  
>  
>  
>  
>
> +
> +  >


Summary of Failures:

253/315 libvirt / virschematest 
 FAIL1.47s   exit status 1


>  
>
>
>  
>
>  
> -
> +
> +
> +   function='0x0'/>
> +
>
>  

[PATCH] conf: Extend TPM ABI stability check for

[Michal Privoznik]

Changing  means changing the guest ABI and as
such must be prevented on both restoring from a file or
migration.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2035888
Signed-off-by: Michal Privoznik 
---
 src/conf/domain_conf.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 716c6d2240..bba662bf4c 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -21994,6 +21994,20 @@ virDomainTPMDefCheckABIStability(virDomainTPMDef *src,
 return false;
 }
 
+switch (src->type) {
+case VIR_DOMAIN_TPM_TYPE_EMULATOR:
+if (src->data.emulator.activePcrBanks != 
dst->data.emulator.activePcrBanks) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("Target active PCR banks doesn't match source"));
+return false;
+}
+break;
+
+case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
+case VIR_DOMAIN_TPM_TYPE_LAST:
+break;
+}
+
 return virDomainDeviceInfoCheckABIStability(>info, >info);
 }
 
-- 
2.34.1

[PATCH 4/4] conf: Make virDomainTPMDefFormat() return void

[Michal Privoznik]

The virDomainTPMDefFormat() function can't fail really. There's
no point in it returning an integer then. Make it return void and
fix both places which check for its retval.

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

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b8fef8586c..509f74c092 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -25476,9 +25476,9 @@ virDomainSoundCodecDefFormat(virBuffer *buf,
 return 0;
 }
 
-static int
+static void
 virDomainTPMDefFormat(virBuffer *buf,
-  virDomainTPMDef *def,
+  const virDomainTPMDef *def,
   unsigned int flags)
 {
 g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
@@ -25529,8 +25529,6 @@ virDomainTPMDefFormat(virBuffer *buf,
 virDomainDeviceInfoFormat(, >info, flags);
 
 virXMLFormatElement(buf, "tpm", , );
-
-return 0;
 }
 
 
@@ -28517,8 +28515,7 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def,
 }
 
 for (n = 0; n < def->ntpms; n++) {
-if (virDomainTPMDefFormat(buf, def->tpms[n], flags) < 0)
-return -1;
+virDomainTPMDefFormat(buf, def->tpms[n], flags);
 }
 
 for (n = 0; n < def->ngraphics; n++) {
@@ -29748,7 +29745,8 @@ virDomainDeviceDefCopy(virDomainDeviceDef *src,
 rc = virDomainChrDefFormat(, src->data.chr, flags);
 break;
 case VIR_DOMAIN_DEVICE_TPM:
-rc = virDomainTPMDefFormat(, src->data.tpm, flags);
+virDomainTPMDefFormat(, src->data.tpm, flags);
+rc = 0;
 break;
 case VIR_DOMAIN_DEVICE_PANIC:
 virDomainPanicDefFormat(, src->data.panic);
-- 
2.34.1

[PATCH 3/4] conf: Rework formatting

[Michal Privoznik]

The  element formatting is handled in
virDomainTPMDefFormat() which uses the "old style" - appending
strings directly into the output buffer. With this, it's easy to
get conditions that tell when an element has ended wrong. In this
particular case, if both  and 
are to be formatted the current code puts a stray '>' into the
output buffer, resulting in invalid XML.

Rewrite the function to use virXMLFormatElement() which is more
clever.

https://bugzilla.redhat.com/show_bug.cgi?id=2016599#c15

Signed-off-by: Michal Privoznik 
---
 src/conf/domain_conf.c   | 53 
 tests/qemuxml2argvdata/tpm-emulator-tpm2.xml |  1 -
 2 files changed, 22 insertions(+), 32 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 716c6d2240..b8fef8586c 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -25481,63 +25481,54 @@ virDomainTPMDefFormat(virBuffer *buf,
   virDomainTPMDef *def,
   unsigned int flags)
 {
-virBufferAsprintf(buf, "\n",
+g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
+g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
+g_auto(virBuffer) backendAttrBuf = VIR_BUFFER_INITIALIZER;
+g_auto(virBuffer) backendBuf = VIR_BUFFER_INIT_CHILD();
+
+virBufferAsprintf(, " model='%s'",
   virDomainTPMModelTypeToString(def->model));
-virBufferAdjustIndent(buf, 2);
-virBufferAsprintf(buf, "type));
 
 switch (def->type) {
 case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
-virBufferAddLit(buf, ">\n");
-virBufferAdjustIndent(buf, 2);
-virBufferEscapeString(buf, "\n",
+virBufferEscapeString(, "\n",
   def->data.passthrough.source->data.file.path);
-virBufferAdjustIndent(buf, -2);
-virBufferAddLit(buf, "\n");
 break;
 case VIR_DOMAIN_TPM_TYPE_EMULATOR:
-virBufferAsprintf(buf, " version='%s'",
+virBufferAsprintf(, " version='%s'",
   virDomainTPMVersionTypeToString(def->version));
 if (def->data.emulator.persistent_state)
-virBufferAddLit(buf, " persistent_state='yes'");
+virBufferAddLit(, " persistent_state='yes'");
 if (def->data.emulator.hassecretuuid) {
 char uuidstr[VIR_UUID_STRING_BUFLEN];
-virBufferAddLit(buf, ">\n");
-virBufferAdjustIndent(buf, 2);
-virBufferAsprintf(buf, "\n",
-virUUIDFormat(def->data.emulator.secretuuid, uuidstr));
-virBufferAdjustIndent(buf, -2);
+
+virBufferAsprintf(, "\n",
+  virUUIDFormat(def->data.emulator.secretuuid, 
uuidstr));
 }
 if (def->data.emulator.activePcrBanks) {
+g_auto(virBuffer) activePcrBanksBuf = 
VIR_BUFFER_INIT_CHILD();
 size_t i;
-virBufferAddLit(buf, ">\n");
-virBufferAdjustIndent(buf, 2);
-virBufferAddLit(buf, "\n");
-virBufferAdjustIndent(buf, 2);
+
 for (i = VIR_DOMAIN_TPM_PCR_BANK_SHA1; i < 
VIR_DOMAIN_TPM_PCR_BANK_LAST; i++) {
 if ((def->data.emulator.activePcrBanks & (1 << i)))
-virBufferAsprintf(buf, "<%s/>\n",
+virBufferAsprintf(, "<%s/>\n",
   virDomainTPMPcrBankTypeToString(i));
 }
-virBufferAdjustIndent(buf, -2);
-virBufferAddLit(buf, "\n");
-virBufferAdjustIndent(buf, -2);
+
+virXMLFormatElement(, "active_pcr_banks", NULL, 
);
 }
-if (def->data.emulator.hassecretuuid ||
-def->data.emulator.activePcrBanks)
-virBufferAddLit(buf, "\n");
-else
-virBufferAddLit(buf, "/>\n");
 break;
 case VIR_DOMAIN_TPM_TYPE_LAST:
 break;
 }
 
-virDomainDeviceInfoFormat(buf, >info, flags);
+virXMLFormatElement(, "backend", , );
+virDomainDeviceInfoFormat(, >info, flags);
 
-virBufferAdjustIndent(buf, -2);
-virBufferAddLit(buf, "\n");
+virXMLFormatElement(buf, "tpm", , );
 
 return 0;
 }
diff --git a/tests/qemuxml2argvdata/tpm-emulator-tpm2.xml 
b/tests/qemuxml2argvdata/tpm-emulator-tpm2.xml
index 59dd68311f..79acde218b 100644
--- a/tests/qemuxml2argvdata/tpm-emulator-tpm2.xml
+++ b/tests/qemuxml2argvdata/tpm-emulator-tpm2.xml
@@ -30,7 +30,6 @@
 
   
 
-  >
 
   
   
-- 
2.34.1