Re: [PATCH 0/4] move validation of virtio options

[Bjoern Walk]

Michal Privoznik  [2020-04-27, 05:27PM +0200]:
> On 4/23/20 3:15 PM, Bjoern Walk wrote:
> > Move validation of virtio options (iommu, ats, packed) from QEMU command
> > line generation to domain validation. As a drive-by, increase the
> > granularity of tests for virtio options.
> > 
> > Bjoern Walk (4):
> >tests: use latest caps for virtio-options test
> >tests: more fine-granular tests for virtio-options
> >qemu: move virtio capability validation
> >qemu: command: make qemuBuildVirtioOptionsStr void
> > 
> >   src/qemu/qemu_command.c   |  59 +++---
> >   src/qemu/qemu_validate.c  |  70 +++-
> >   .../virtio-options-controller-ats.args|  32 ++
> >   .../virtio-options-controller-ats.xml |  38 +++
> >   .../virtio-options-controller-iommu.args  |  34 ++
> >   .../virtio-options-controller-iommu.xml   |  38 +++
> >   .../virtio-options-controller-packed.args |  32 ++
> >   .../virtio-options-controller-packed.xml  |  38 +++
> >   .../virtio-options-disk-ats.args  |  36 +++
> >   .../virtio-options-disk-ats.xml   |  34 ++
> >   .../virtio-options-disk-iommu.args|  36 +++
> >   .../virtio-options-disk-iommu.xml |  34 ++
> >   .../virtio-options-disk-packed.args   |  36 +++
> >   .../virtio-options-disk-packed.xml|  34 ++
> >   .../virtio-options-fs-ats.args|  34 ++
> >   .../virtio-options-fs-ats.xml |  34 ++
> >   .../virtio-options-fs-iommu.args  |  34 ++
> >   .../virtio-options-fs-iommu.xml   |  34 ++
> >   .../virtio-options-fs-packed.args |  34 ++
> >   .../virtio-options-fs-packed.xml  |  34 ++
> >   .../virtio-options-input-ats.args |  30 ++
> >   .../virtio-options-input-ats.xml  |  30 ++
> >   .../virtio-options-input-iommu.args   |  30 ++
> >   .../virtio-options-input-iommu.xml|  30 ++
> >   .../virtio-options-input-packed.args  |  30 ++
> >   .../virtio-options-input-packed.xml   |  30 ++
> >   .../virtio-options-memballoon-ats.args|  28 +
> >   .../virtio-options-memballoon-ats.xml |  23 
> >   .../virtio-options-memballoon-iommu.args  |  28 +
> >   .../virtio-options-memballoon-iommu.xml   |  23 
> >   .../virtio-options-memballoon-packed.args |  28 +
> >   .../virtio-options-memballoon-packed.xml  |  23 
> >   .../virtio-options-net-ats.args   |  34 ++
> >   .../virtio-options-net-ats.xml|  34 ++
> >   .../virtio-options-net-iommu.args |  34 ++
> >   .../virtio-options-net-iommu.xml  |  34 ++
> >   .../virtio-options-net-packed.args|  34 ++
> >   .../virtio-options-net-packed.xml |  34 ++
> >   .../virtio-options-rng-ats.args   |  32 ++
> >   .../virtio-options-rng-ats.xml|  32 ++
> >   .../virtio-options-rng-iommu.args |  34 ++
> >   .../virtio-options-rng-iommu.xml  |  32 ++
> >   .../virtio-options-rng-packed.args|  32 ++
> >   .../virtio-options-rng-packed.xml |  32 ++
> >   .../virtio-options-video-ats.args |  34 ++
> >   .../virtio-options-video-ats.xml  |  36 +++
> >   .../virtio-options-video-iommu.args   |  34 ++
> >   .../virtio-options-video-iommu.xml|  36 +++
> >   .../virtio-options-video-packed.args  |  34 ++
> >   .../virtio-options-video-packed.xml   |  36 +++
> >   .../virtio-options.x86_64-latest.args |  69 
> >   tests/qemuxml2argvdata/virtio-options.xml |   5 +-
> >   tests/qemuxml2argvtest.c  | 101 --
> >   .../virtio-options.x86_64-latest.xml  |   1 +
> >   tests/qemuxml2xmltest.c   |  16 +--
> >   55 files changed, 1818 insertions(+), 70 deletions(-)
> >   create mode 100644 
> > tests/qemuxml2argvdata/virtio-options-controller-ats.args
> >   create mode 100644 
> > tests/qemuxml2argvdata/virtio-options-controller-ats.xml
> >   create mode 100644 
> > tests/qemuxml2argvdata/virtio-options-controller-iommu.args
> >   create mode 100644 
> > tests/qemuxml2argvdata/virtio-options-controller-iommu.xml
> >   create mode 100644 
> > tests/qemuxml2argvdata/virtio-options-controller-packed.args
> >   create mode 100644 
> > tests/qemuxml2argvdata/virtio-options-controller-packed.xml
> >   create mode 100644 tests/qemuxml2argvdata/virtio-options-disk-ats.args
> >   create mode 100644 tests/qemuxml2argvdata/virtio-options-disk-ats.xml
> >   create mode 100644 tests/qemuxml2argvdata/virtio-options-disk-iommu.args
> >   create mode 100644 

Re: [PATCH v5 0/4] introduction of migration_version attribute for VFIO live migration

[Yan Zhao]

On Mon, Apr 27, 2020 at 11:37:43PM +0800, Dr. David Alan Gilbert wrote:
> * Yan Zhao (yan.y.z...@intel.com) wrote:
> > On Sat, Apr 25, 2020 at 03:10:49AM +0800, Dr. David Alan Gilbert wrote:
> > > * Yan Zhao (yan.y.z...@intel.com) wrote:
> > > > On Tue, Apr 21, 2020 at 08:08:49PM +0800, Tian, Kevin wrote:
> > > > > > From: Yan Zhao
> > > > > > Sent: Tuesday, April 21, 2020 10:37 AM
> > > > > > 
> > > > > > On Tue, Apr 21, 2020 at 06:56:00AM +0800, Alex Williamson wrote:
> > > > > > > On Sun, 19 Apr 2020 21:24:57 -0400
> > > > > > > Yan Zhao  wrote:
> > > > > > >
> > > > > > > > On Fri, Apr 17, 2020 at 07:24:57PM +0800, Cornelia Huck wrote:
> > > > > > > > > On Fri, 17 Apr 2020 05:52:02 -0400
> > > > > > > > > Yan Zhao  wrote:
> > > > > > > > >
> > > > > > > > > > On Fri, Apr 17, 2020 at 04:44:50PM +0800, Cornelia Huck 
> > > > > > > > > > wrote:
> > > > > > > > > > > On Mon, 13 Apr 2020 01:52:01 -0400
> > > > > > > > > > > Yan Zhao  wrote:
> > > > > > > > > > >
> > > > > > > > > > > > This patchset introduces a migration_version attribute 
> > > > > > > > > > > > under sysfs
> > > > > > of VFIO
> > > > > > > > > > > > Mediated devices.
> > > > > > > > > > > >
> > > > > > > > > > > > This migration_version attribute is used to check 
> > > > > > > > > > > > migration
> > > > > > compatibility
> > > > > > > > > > > > between two mdev devices.
> > > > > > > > > > > >
> > > > > > > > > > > > Currently, it has two locations:
> > > > > > > > > > > > (1) under mdev_type node,
> > > > > > > > > > > > which can be used even before device creation, but 
> > > > > > > > > > > > only for
> > > > > > mdev
> > > > > > > > > > > > devices of the same mdev type.
> > > > > > > > > > > > (2) under mdev device node,
> > > > > > > > > > > > which can only be used after the mdev devices are 
> > > > > > > > > > > > created, but
> > > > > > the src
> > > > > > > > > > > > and target mdev devices are not necessarily be of 
> > > > > > > > > > > > the same
> > > > > > mdev type
> > > > > > > > > > > > (The second location is newly added in v5, in order to 
> > > > > > > > > > > > keep
> > > > > > consistent
> > > > > > > > > > > > with the migration_version node for migratable 
> > > > > > > > > > > > pass-though
> > > > > > devices)
> > > > > > > > > > >
> > > > > > > > > > > What is the relationship between those two attributes?
> > > > > > > > > > >
> > > > > > > > > > (1) is for mdev devices specifically, and (2) is provided 
> > > > > > > > > > to keep the
> > > > > > same
> > > > > > > > > > sysfs interface as with non-mdev cases. so (2) is for both 
> > > > > > > > > > mdev
> > > > > > devices and
> > > > > > > > > > non-mdev devices.
> > > > > > > > > >
> > > > > > > > > > in future, if we enable vfio-pci vendor ops, (i.e. a 
> > > > > > > > > > non-mdev device
> > > > > > > > > > is binding to vfio-pci, but is able to register migration 
> > > > > > > > > > region and do
> > > > > > > > > > migration transactions from a vendor provided affiliate 
> > > > > > > > > > driver),
> > > > > > > > > > the vendor driver would export (2) directly, under device 
> > > > > > > > > > node.
> > > > > > > > > > It is not able to provide (1) as there're no mdev devices 
> > > > > > > > > > involved.
> > > > > > > > >
> > > > > > > > > Ok, creating an alternate attribute for non-mdev devices 
> > > > > > > > > makes sense.
> > > > > > > > > However, wouldn't that rather be a case (3)? The change here 
> > > > > > > > > only
> > > > > > > > > refers to mdev devices.
> > > > > > > > >
> > > > > > > > as you pointed below, (3) and (2) serve the same purpose.
> > > > > > > > and I think a possible usage is to migrate between a non-mdev 
> > > > > > > > device and
> > > > > > > > an mdev device. so I think it's better for them both to use (2) 
> > > > > > > > rather
> > > > > > > > than creating (3).
> > > > > > >
> > > > > > > An mdev type is meant to define a software compatible interface, 
> > > > > > > so in
> > > > > > > the case of mdev->mdev migration, doesn't migrating to a 
> > > > > > > different type
> > > > > > > fail the most basic of compatibility tests that we expect 
> > > > > > > userspace to
> > > > > > > perform?  IOW, if two mdev types are migration compatible, it 
> > > > > > > seems a
> > > > > > > prerequisite to that is that they provide the same software 
> > > > > > > interface,
> > > > > > > which means they should be the same mdev type.
> > > > > > >
> > > > > > > In the hybrid cases of mdev->phys or phys->mdev, how does a
> > > > > > management
> > > > > > > tool begin to even guess what might be compatible?  Are we 
> > > > > > > expecting
> > > > > > > libvirt to probe ever device with this attribute in the system?  
> > > > > > > Is
> > > > > > > there going to be a new class hierarchy created to enumerate all
> > > > > > > possible migrate-able devices?
> > > > > > >
> > > > > > yes, management tool needs to guess and test migration compatible
> > > > > > between two devices. But I think it's 

Re: [libvirt PATCH v4 1/3] add QEMU_CAPS_TCG_DISABLED and probe conditionally

[tobin]

On 2020-04-27 16:00, Michal Privoznik wrote:

On 4/27/20 9:16 PM, tobin wrote:

On 2020-04-27 04:15, Michal Privoznik wrote:

On 4/24/20 5:52 PM, tobin wrote:

On 2020-04-24 11:31, Michal Privoznik wrote:

On 4/22/20 11:50 PM, Tobin Feldman-Fitzthum wrote:

Only probe QEMU binary with accel=tcg if TCG is not disabled.
Similarly, only add a VIR_DOMAIN_VIRT_QEMU guest if TCG
is available.

Signed-off-by: Tobin Feldman-Fitzthum 
---
  src/qemu/qemu_capabilities.c | 22 ++
  src/qemu/qemu_capabilities.h |  1 +
  2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c 
b/src/qemu/qemu_capabilities.c

index fe311048d4..c56b2d8f0e 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -573,6 +573,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
    "fsdev.multidevs",
    "virtio.packed",
    "pcie-root-port.hotplug",
+  "tcg-disabled",
  );
    @@ -1014,13 +1015,16 @@ 
virQEMUCapsInitGuestFromBinary(virCapsPtr caps,
  virCapabilitiesAddGuestFeatureWithToggle(guest, 
VIR_CAPS_GUEST_FEATURE_TYPE_DISKSNAPSHOT,

true, false);
  -    if (virCapabilitiesAddGuestDomain(guest,
- VIR_DOMAIN_VIRT_QEMU,
- NULL,
- NULL,
- 0,
- NULL) == NULL)
-    goto cleanup;
+    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED)) {
+    if (virCapabilitiesAddGuestDomain(guest,
+ VIR_DOMAIN_VIRT_QEMU,
+ NULL,
+ NULL,
+ 0,
+ NULL) == NULL) {
+    goto cleanup;
+    }
+    }
    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
  if (virCapabilitiesAddGuestDomain(guest,
@@ -2295,7 +2299,8 @@ bool
  virQEMUCapsIsVirtTypeSupported(virQEMUCapsPtr qemuCaps,
virDomainVirtType virtType)
  {
-    if (virtType == VIR_DOMAIN_VIRT_QEMU)
+    if (virtType == VIR_DOMAIN_VIRT_QEMU &&
+    !virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED))
  return true;
    if (virtType == VIR_DOMAIN_VIRT_KVM &&
@@ -5150,6 +5155,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
   * off.
   */
  if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) &&
+    !virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED) 
&&
  virQEMUCapsInitQMPSingle(qemuCaps, libDir, 
runUid, runGid, true) < 0)

  return -1;
  diff --git a/src/qemu/qemu_capabilities.h 
b/src/qemu/qemu_capabilities.h

index 1681fc79a8..abc4ee82cb 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -554,6 +554,7 @@ typedef enum { /* virQEMUCapsFlags grouping 
marker for syntax-check */

  QEMU_CAPS_FSDEV_MULTIDEVS, /* fsdev.multidevs */
  QEMU_CAPS_VIRTIO_PACKED_QUEUES, /* virtio.packed */
  QEMU_CAPS_PCIE_ROOT_PORT_HOTPLUG, /* 
pcie-root-port.hotplug */

+    QEMU_CAPS_TCG_DISABLED, /* QEMU does not support TCG */


Actually, I think this should be a positive capability, e.g.
QEMU_CAPS_TCG to reflect whether QEMU supports TCG or not. I can do
the change locally before pushing, if you agree with the change.

Michal


I can see why a positive capability might be a little more 
intuitive,

but one thing to keep in mind is that if there is a capability for
the TCG being enabled, we will have to do a bit more work to make 
sure

it is always set correctly. Older versions of QEMU don't report that
the TCG is enabled, for instance, so we would need to make sure we
always set TCG_ENABLED for those versions. This applies not only 
when

probing for caps, but also when reading capabilities from XML.


That is okay, if libvirtd's or qemu's ctime changes, or any version 
of

the two then capabilities are invalidated and reprobed. This means
that introducing a new capability causes libvirt to reprobe all caps
on startup. So we don't have to be that careful about old caps XMLs
(note, this is different to case when reading caps XMLs on a say
daemon restart when nothing changed, we have to be careful there).

And as far as positive/negative boolean flag goes - in either cases 
we

will have false positives. Say, a given QEMU binary doesn't support
TCG but also the binary is old and doesn't allow TCG detection. I
don't see any difference between caps reporting TCG flag set
(erroneously), or TCG_DISABLED flag not set (again erroneously). 
Since

we are not able to detect the flag for older versions, we have to
guess - and that is what we are doing already in these cases - see
virQEMUCapsInitQMPVersionCaps().


I think
these are solvable problems (although I suspect there might be some
interesting edge cases), but if we have a negative capability, we
don't have to worry about any of this. We set it in the few cases 
where
TCG is disabled and otherwise the TCG is always assumed to be 
active.




Again, I don't see any difference here, sorry.

Michal


Ok, I have some doubts, but I have an earlier version of the patch 
with

a positive capability. I will dig that up and send it through in the
next day or two.


No need. I have all the changes needed in a local branch, I will just
squashed them. All I wanted was 

Re: [libvirt PATCH v4 1/3] add QEMU_CAPS_TCG_DISABLED and probe conditionally

[Michal Privoznik]

On 4/27/20 9:16 PM, tobin wrote:

On 2020-04-27 04:15, Michal Privoznik wrote:

On 4/24/20 5:52 PM, tobin wrote:

On 2020-04-24 11:31, Michal Privoznik wrote:

On 4/22/20 11:50 PM, Tobin Feldman-Fitzthum wrote:

Only probe QEMU binary with accel=tcg if TCG is not disabled.
Similarly, only add a VIR_DOMAIN_VIRT_QEMU guest if TCG
is available.

Signed-off-by: Tobin Feldman-Fitzthum 
---
  src/qemu/qemu_capabilities.c | 22 ++
  src/qemu/qemu_capabilities.h |  1 +
  2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c 
b/src/qemu/qemu_capabilities.c

index fe311048d4..c56b2d8f0e 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -573,6 +573,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
    "fsdev.multidevs",
    "virtio.packed",
    "pcie-root-port.hotplug",
+  "tcg-disabled",
  );
    @@ -1014,13 +1015,16 @@ 
virQEMUCapsInitGuestFromBinary(virCapsPtr caps,
  virCapabilitiesAddGuestFeatureWithToggle(guest, 
VIR_CAPS_GUEST_FEATURE_TYPE_DISKSNAPSHOT,

true, false);
  -    if (virCapabilitiesAddGuestDomain(guest,
- VIR_DOMAIN_VIRT_QEMU,
- NULL,
- NULL,
- 0,
- NULL) == NULL)
-    goto cleanup;
+    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED)) {
+    if (virCapabilitiesAddGuestDomain(guest,
+ VIR_DOMAIN_VIRT_QEMU,
+ NULL,
+ NULL,
+ 0,
+ NULL) == NULL) {
+    goto cleanup;
+    }
+    }
    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
  if (virCapabilitiesAddGuestDomain(guest,
@@ -2295,7 +2299,8 @@ bool
  virQEMUCapsIsVirtTypeSupported(virQEMUCapsPtr qemuCaps,
virDomainVirtType virtType)
  {
-    if (virtType == VIR_DOMAIN_VIRT_QEMU)
+    if (virtType == VIR_DOMAIN_VIRT_QEMU &&
+    !virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED))
  return true;
    if (virtType == VIR_DOMAIN_VIRT_KVM &&
@@ -5150,6 +5155,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
   * off.
   */
  if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) &&
+    !virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED) &&
  virQEMUCapsInitQMPSingle(qemuCaps, libDir, runUid, 
runGid, true) < 0)

  return -1;
  diff --git a/src/qemu/qemu_capabilities.h 
b/src/qemu/qemu_capabilities.h

index 1681fc79a8..abc4ee82cb 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -554,6 +554,7 @@ typedef enum { /* virQEMUCapsFlags grouping 
marker for syntax-check */

  QEMU_CAPS_FSDEV_MULTIDEVS, /* fsdev.multidevs */
  QEMU_CAPS_VIRTIO_PACKED_QUEUES, /* virtio.packed */
  QEMU_CAPS_PCIE_ROOT_PORT_HOTPLUG, /* pcie-root-port.hotplug */
+    QEMU_CAPS_TCG_DISABLED, /* QEMU does not support TCG */


Actually, I think this should be a positive capability, e.g.
QEMU_CAPS_TCG to reflect whether QEMU supports TCG or not. I can do
the change locally before pushing, if you agree with the change.

Michal


I can see why a positive capability might be a little more intuitive,
but one thing to keep in mind is that if there is a capability for
the TCG being enabled, we will have to do a bit more work to make sure
it is always set correctly. Older versions of QEMU don't report that
the TCG is enabled, for instance, so we would need to make sure we
always set TCG_ENABLED for those versions. This applies not only when
probing for caps, but also when reading capabilities from XML.


That is okay, if libvirtd's or qemu's ctime changes, or any version of
the two then capabilities are invalidated and reprobed. This means
that introducing a new capability causes libvirt to reprobe all caps
on startup. So we don't have to be that careful about old caps XMLs
(note, this is different to case when reading caps XMLs on a say
daemon restart when nothing changed, we have to be careful there).

And as far as positive/negative boolean flag goes - in either cases we
will have false positives. Say, a given QEMU binary doesn't support
TCG but also the binary is old and doesn't allow TCG detection. I
don't see any difference between caps reporting TCG flag set
(erroneously), or TCG_DISABLED flag not set (again erroneously). Since
we are not able to detect the flag for older versions, we have to
guess - and that is what we are doing already in these cases - see
virQEMUCapsInitQMPVersionCaps().


I think
these are solvable problems (although I suspect there might be some
interesting edge cases), but if we have a negative capability, we
don't have to worry about any of this. We set it in the few cases where
TCG is disabled and otherwise the TCG is always assumed to be active.



Again, I don't see any difference here, sorry.

Michal


Ok, I have some doubts, but I have an earlier version of the patch with
a positive capability. I will dig that up and send it through in the
next day or two.


No need. I have all the changes needed in a local branch, I will just 
squashed them. All I wanted was your blessing :-)


Michal

Re: [libvirt PATCH v4 1/3] add QEMU_CAPS_TCG_DISABLED and probe conditionally

[tobin]

On 2020-04-27 04:15, Michal Privoznik wrote:

On 4/24/20 5:52 PM, tobin wrote:

On 2020-04-24 11:31, Michal Privoznik wrote:

On 4/22/20 11:50 PM, Tobin Feldman-Fitzthum wrote:

Only probe QEMU binary with accel=tcg if TCG is not disabled.
Similarly, only add a VIR_DOMAIN_VIRT_QEMU guest if TCG
is available.

Signed-off-by: Tobin Feldman-Fitzthum 
---
  src/qemu/qemu_capabilities.c | 22 ++
  src/qemu/qemu_capabilities.h |  1 +
  2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c 
b/src/qemu/qemu_capabilities.c

index fe311048d4..c56b2d8f0e 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -573,6 +573,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
    "fsdev.multidevs",
    "virtio.packed",
    "pcie-root-port.hotplug",
+  "tcg-disabled",
  );
    @@ -1014,13 +1015,16 @@ 
virQEMUCapsInitGuestFromBinary(virCapsPtr caps,
  virCapabilitiesAddGuestFeatureWithToggle(guest, 
VIR_CAPS_GUEST_FEATURE_TYPE_DISKSNAPSHOT,
   
true, false);

  -    if (virCapabilitiesAddGuestDomain(guest,
-  
VIR_DOMAIN_VIRT_QEMU,
-  
NULL,
-  
NULL,
-  
0,
-  
NULL) == NULL)

-    goto cleanup;
+    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED)) {
+    if (virCapabilitiesAddGuestDomain(guest,
+  
VIR_DOMAIN_VIRT_QEMU,
+  
NULL,
+  
NULL,
+  
0,
+  
NULL) == NULL) {

+    goto cleanup;
+    }
+    }
    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
  if (virCapabilitiesAddGuestDomain(guest,
@@ -2295,7 +2299,8 @@ bool
  virQEMUCapsIsVirtTypeSupported(virQEMUCapsPtr qemuCaps,
 
virDomainVirtType virtType)

  {
-    if (virtType == VIR_DOMAIN_VIRT_QEMU)
+    if (virtType == VIR_DOMAIN_VIRT_QEMU &&
+    !virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED))
  return true;
    if (virtType == VIR_DOMAIN_VIRT_KVM &&
@@ -5150,6 +5155,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
   * off.
   */
  if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) &&
+    !virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED) &&
  virQEMUCapsInitQMPSingle(qemuCaps, libDir, 
runUid, runGid, true) < 0)

  return -1;
  diff --git a/src/qemu/qemu_capabilities.h 
b/src/qemu/qemu_capabilities.h

index 1681fc79a8..abc4ee82cb 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -554,6 +554,7 @@ typedef enum { /* virQEMUCapsFlags grouping 
marker for syntax-check */

  QEMU_CAPS_FSDEV_MULTIDEVS, /* fsdev.multidevs */
  QEMU_CAPS_VIRTIO_PACKED_QUEUES, /* virtio.packed */
  QEMU_CAPS_PCIE_ROOT_PORT_HOTPLUG, /* 
pcie-root-port.hotplug */

+    QEMU_CAPS_TCG_DISABLED, /* QEMU does not support TCG */


Actually, I think this should be a positive capability, e.g.
QEMU_CAPS_TCG to reflect whether QEMU supports TCG or not. I can do
the change locally before pushing, if you agree with the change.

Michal


I can see why a positive capability might be a little more intuitive,
but one thing to keep in mind is that if there is a capability for
the TCG being enabled, we will have to do a bit more work to make sure
it is always set correctly. Older versions of QEMU don't report that
the TCG is enabled, for instance, so we would need to make sure we
always set TCG_ENABLED for those versions. This applies not only when
probing for caps, but also when reading capabilities from XML.


That is okay, if libvirtd's or qemu's ctime changes, or any version of
the two then capabilities are invalidated and reprobed. This means
that introducing a new capability causes libvirt to reprobe all caps
on startup. So we don't have to be that careful about old caps XMLs
(note, this is different to case when reading caps XMLs on a say
daemon restart when nothing changed, we have to be careful there).

And as far as positive/negative boolean flag goes - in either cases we
will have false positives. Say, a given QEMU binary doesn't support
TCG but also the binary is old and doesn't allow TCG detection. I
don't see any difference between caps reporting TCG flag set
(erroneously), or TCG_DISABLED flag not set (again erroneously). Since
we are not able to detect the flag for older versions, we have to
guess - and that is what we are doing already in these cases - see
virQEMUCapsInitQMPVersionCaps().


I think
these are solvable problems (although I suspect there might be some
interesting edge cases), but if we have a negative capability, we
don't have to worry about any of this. We set it in the few cases 
where

TCG 

Re: [libvirt-ci PATCH 06/13] lcitool: Introduce methods to load and validate the TOML config

[Andrea Bolognani]

On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
> +def __init__(self):
> +path = self._get_config_file("config.toml")
> +
> +try:
> +self.dict = toml.load(path)

I'm not too keen on the name "dict". What about "values"?

> +self._validate()
> +
> +except Exception as ex:
> +raise Exception(
> +"Missing or invalid config.toml file: {}".format(ex)
> +)

This will cause lcitool to blow up pretty spectacularly if you
haven't created a configuration file already:

  $ ./lcitool update all all
  Traceback (most recent call last):
File "./lcitool", line 141, in __init__
  self.dict = toml.load(path)
File "/usr/lib/python3.6/site-packages/toml.py", line 87, in load
  with io.open(f, encoding='utf-8') as ffile:
  FileNotFoundError: [Errno 2] No such file or directory: 
'/home/abologna/.config/lcitool/config.toml'

  During handling of the above exception, another exception occurred:

  Traceback (most recent call last):
File "./lcitool", line 1043, in 
  Application().run()
File "./lcitool", line 395, in __init__
  self._config = Config()
File "./lcitool", line 146, in __init__
  "Missing or invalid config.toml file: {}".format(ex)
  Exception: Missing or invalid config.toml file: [Errno 2] No such file or 
directory: '/home/abologna/.config/lcitool/config.toml'

This could technically happen already, but that would have been as
the result of invalid data being committed to the repository, so it
never showed up in practice.

You'll need to refactor the script so that exceptions can be caught
and pretty-printed even when they occur outside of Application.run().

> +# fill in default options
> +self._fill_default_options(self.dict)

You don't need to pass the dictionary as argument - it's already
part of self at this point.

> +def _fill_default_options(cfg):
> +flavor = cfg.get("install").get("flavor", "test")
> +cfg["install"]["flavor"] = flavor
> +
> +if flavor == "gitlab":
> +url = cfg.get("gitlab").get("gitlab_url", "https://gitlab.com;)
> +cfg["gitlab"]["gitlab_url"] = url

It would be neat if, instead of hardcoding our defaults in the
script, we did something like

  cfg.get("gitlab").get("gitlab_url", default_gitlab_url)

where default_gitlab_url is obtained from the inventory.

Alternatively, and this is probably a cleaner approach, we could make
it so lcitool reads config.toml from the source directory, which
would be uncommented, first, and then the one in the user's home
directory, adding / overriding only those keys that are found: this
would ensure, among other things, that the comments in the sample
configuration file never get out of sync with the actual defaults,
and also I think the amount of boring code necessary in patch 11.

> +@staticmethod
> +def _validate_section(cfg, section, *args):

I don't like *args very much, and would prefer if this was just a
regular list.

> +if cfg.get(section, None) is None:
> +raise KeyError("Section '[{}]' not found".format(section))

No need for square brackets around the name, you already mentioned
that it's a section we're talking about.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt-dbus PATCH 7/9] man: Convert to reStructuredText

[Andrea Bolognani]

On Mon, 2020-04-27 at 18:37 +0200, Andrea Bolognani wrote:
> On Mon, 2020-04-27 at 16:27 +0100, Daniel P. Berrangé wrote:
> > On Sat, Apr 25, 2020 at 10:59:44PM +0200, Andrea Bolognani wrote:
> > > -prog_pod2man = find_program('pod2man')
> > > +prog_rst2man = find_program('rst2man')
> > 
> > This broke on RHEL-7 CI since it needs a -3 suffix, and IIRC will break
> > other distros which need a .py suffix
> > 
> > In libvirt configure.ac we pick from
> > 
> >rst2man rst2man.py rst2man-3
> 
> Right. I'll commit the obvious fix. Thanks for the heads-up!

... except Pavel already did :)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt-dbus PATCH 7/9] man: Convert to reStructuredText

[Andrea Bolognani]

On Mon, 2020-04-27 at 16:27 +0100, Daniel P. Berrangé wrote:
> On Sat, Apr 25, 2020 at 10:59:44PM +0200, Andrea Bolognani wrote:
> > -prog_pod2man = find_program('pod2man')
> > +prog_rst2man = find_program('rst2man')
> 
> This broke on RHEL-7 CI since it needs a -3 suffix, and IIRC will break
> other distros which need a .py suffix
> 
> In libvirt configure.ac we pick from
> 
>rst2man rst2man.py rst2man-3

Right. I'll commit the obvious fix. Thanks for the heads-up!

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt-ci PATCH 05/13] config: Introduce a new global config.toml configuration file

[Andrea Bolognani]

On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
> and it also
> has some nice features over the plain INI should we find ourselves in
> need of any of such extended features in the future, e.g. table nesting.

Let's hope we never will! :)

> diff --git a/config.toml b/config.toml

This is specific to lcitool, so it should go in guests/ rather than
in the top-level directory.

> +# Configuration file for the lcitool -- 
> https://gitlab.com/libvirt/libvirt-ci/
> +# 
> 

s/the //, and I would leave out the URL as well.

> +# Initial root password to be set by ansible on the appliance. This password
> +# will only be necessary for serial console access in case something goes
> +# horribly wrong, for all other use cases, SSH key authentication will be 
> used
> +# instead. (mandatory)
> +#root_password = ""

s/ansible/Ansible/


With these nits addressed,

  Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt-ci PATCH 04/13] lcitool: Use a temporary JSON file to pass extra variables

[Andrea Bolognani]

On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
> This patch is a pre-requisite config file consolidation. Currently we've
> got a number of files which serve as a configuration either to the
> lcitool itself or to the ansible playbooks (majority).  Once we replace
> these with a single global lcitool config, we'd end up passing tokens
> (potentially some passwords) as ansible extra variables bare naked on
> the cmdline. In order to prevent this security flaw use temporary JSON
> file holding all these extra variables and pass it as follows:
> 
> $ ansible-playbook --extra-vars @extra_vars.json playbook.yml

I find it impossible not to point out that, if the configuration
file was in YAML format, then we could pass its contents to Ansible
without having to create a temporary file *or* risk exposing
sensitive data O:-)

> @@ -504,21 +504,26 @@ class Application:
>  git_remote = "default"
>  git_branch = "master"
>  
> +tempdir = tempfile.TemporaryDirectory(prefix='lcitool')

If you want to pass prefix, do the same thing for the call introduced
in the previous commit.

Also, double quotes around strings please.

>  ansible_cfg_path = os.path.join(base, "ansible.cfg")
>  playbook_base = os.path.join(base, "playbooks", playbook)
>  playbook_path = os.path.join(playbook_base, "main.yml")
> +extra_vars_path = os.path.join(tempdir.name, 'extra_vars.json')

Double quotes.

>  
> -extra_vars = json.dumps({
> -"base": base,
> -"playbook_base": playbook_base,
> -"root_password_file": root_pass_file,
> -"flavor": flavor,
> -"selected_projects": selected_projects,
> -"git_remote": git_remote,
> -"git_branch": git_branch,
> -"gitlab_url_file": gitlab_url_file,
> -"gitlab_runner_token_file": gitlab_runner_token_file,
> -})
> +with open(extra_vars_path, 'w') as fp:

Double quotes.

> +extra_vars = {
> +"base": base,
> +"playbook_base": playbook_base,
> +"root_password_file": root_pass_file,
> +"flavor": flavor,
> +"selected_projects": selected_projects,
> +"git_remote": git_remote,
> +"git_branch": git_branch,
> +"gitlab_url_file": gitlab_url_file,
> +"gitlab_runner_token_file": gitlab_runner_token_file,
> +}
> +json.dump(extra_vars, fp)

Moving the definition of the dictionary is not needed: just do

  extra_vars = {
  ...
  }

  with open(...) as fp:
  json.dumps(extra_vars, fp)

which keeps the with scope nice and small.


With these few nits fixed,

  Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt-dbus PATCH] docs/meson: fix rst2man detection

[Pavel Hrdina]

On Mon, Apr 27, 2020 at 05:19:27PM +0100, Daniel P. Berrangé wrote:
> On Mon, Apr 27, 2020 at 06:17:14PM +0200, Pavel Hrdina wrote:
> > In libvirt project we use python3 version exclusively so we should do
> > the same in libvirt-dbus.  This means that we have to check other
> > possible binary names.
> 
> nitpick  s/binary/command/  since python scripts aren't binaries :-)

Right :) that's better, I did not like the 'binary' as well but for some
reason I was not able to recall 'command'.

Thanks.


signature.asc
Description: PGP signature

Re: [libvirt-dbus PATCH] docs/meson: fix rst2man detection

[Daniel P . Berrangé]

On Mon, Apr 27, 2020 at 06:17:14PM +0200, Pavel Hrdina wrote:
> In libvirt project we use python3 version exclusively so we should do
> the same in libvirt-dbus.  This means that we have to check other
> possible binary names.

nitpick  s/binary/command/  since python scripts aren't binaries :-)

> 
> Signed-off-by: Pavel Hrdina 
> ---
>  docs/meson.build | 2 +-
>  libvirt-dbus.spec.in | 6 +-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/meson.build b/docs/meson.build
> index fbeaf12..c2e8ae7 100644
> --- a/docs/meson.build
> +++ b/docs/meson.build
> @@ -1,4 +1,4 @@
> -prog_rst2man = find_program('rst2man')
> +prog_rst2man = find_program('rst2man', 'rst2man.py', 'rst2man-3')
>  
>  configure_file(
>  command: [prog_rst2man.path(), '@INPUT@', '@OUTPUT@'],
> diff --git a/libvirt-dbus.spec.in b/libvirt-dbus.spec.in
> index 84172fa..4e6ff85 100644
> --- a/libvirt-dbus.spec.in
> +++ b/libvirt-dbus.spec.in
> @@ -19,7 +19,11 @@ BuildRequires: meson >= %{meson_version}
>  BuildRequires: glib2-devel >= %{glib2_version}
>  BuildRequires: libvirt-devel >= %{libvirt_version}
>  BuildRequires: libvirt-glib-devel >= %{libvirt_glib_version}
> -BuildRequires: /usr/bin/rst2man
> +%if 0%{?rhel} == 7
> +BuildRequires: python36-docutils
> +%else
> +BuildRequires: python3-docutils
> +%endif

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

[libvirt-dbus PATCH] docs/meson: fix rst2man detection

[Pavel Hrdina]

In libvirt project we use python3 version exclusively so we should do
the same in libvirt-dbus.  This means that we have to check other
possible binary names.

Signed-off-by: Pavel Hrdina 
---
 docs/meson.build | 2 +-
 libvirt-dbus.spec.in | 6 +-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/docs/meson.build b/docs/meson.build
index fbeaf12..c2e8ae7 100644
--- a/docs/meson.build
+++ b/docs/meson.build
@@ -1,4 +1,4 @@
-prog_rst2man = find_program('rst2man')
+prog_rst2man = find_program('rst2man', 'rst2man.py', 'rst2man-3')
 
 configure_file(
 command: [prog_rst2man.path(), '@INPUT@', '@OUTPUT@'],
diff --git a/libvirt-dbus.spec.in b/libvirt-dbus.spec.in
index 84172fa..4e6ff85 100644
--- a/libvirt-dbus.spec.in
+++ b/libvirt-dbus.spec.in
@@ -19,7 +19,11 @@ BuildRequires: meson >= %{meson_version}
 BuildRequires: glib2-devel >= %{glib2_version}
 BuildRequires: libvirt-devel >= %{libvirt_version}
 BuildRequires: libvirt-glib-devel >= %{libvirt_glib_version}
-BuildRequires: /usr/bin/rst2man
+%if 0%{?rhel} == 7
+BuildRequires: python36-docutils
+%else
+BuildRequires: python3-docutils
+%endif
 
 Requires: dbus
 Requires: glib2 >= %{glib2_version}
-- 
2.25.4

Re: [libvirt PATCH] qemu: re-add padding to the saved state images

[Andrea Bolognani]

On Mon, 2020-04-27 at 14:41 +0100, Daniel P. Berrangé wrote:
> In the past we added 1024 bytes of padding to saved state images so that
> users can run "virsh managedsave-edit $GUEST" and make XML changes which
> increase the size of the XML document. This padding was accidentally
> lost a while back
> 
>   commit 6b9b21db7079888a05d192b079e68290bdf14a76
>   Author: Peter Krempa 
>   Date:   Wed Feb 17 13:10:11 2016 +0100
> 
> qemu: Remove unnecessary calculations in qemuDomainSaveMemory
> 
> The original 1024 bytes was unreasonably stingy when we consider that
> the QEMU state is typically going to be many 100's of MB in size. Thus
> this adds 64 KB of padding after the XML which should cope with any
> plausible modifications a user will want to make.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/qemu/qemu_driver.c | 17 ++---
>  1 file changed, 10 insertions(+), 7 deletions(-)

Please consider including a link to

  https://bugzilla.redhat.com/show_bug.cgi?id=1229255

in the commit message.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt-dbus PATCH 7/9] man: Convert to reStructuredText

[Pavel Hrdina]

On Mon, Apr 27, 2020 at 04:27:38PM +0100, Daniel P. Berrangé wrote:
> On Sat, Apr 25, 2020 at 10:59:44PM +0200, Andrea Bolognani wrote:
> 
> > diff --git a/docs/meson.build b/docs/meson.build
> > index e62e71a..fbeaf12 100644
> > --- a/docs/meson.build
> > +++ b/docs/meson.build
> > @@ -1,8 +1,8 @@
> > -prog_pod2man = find_program('pod2man')
> > +prog_rst2man = find_program('rst2man')
> 
> This broke on RHEL-7 CI since it needs a -3 suffix, and IIRC will break
> other distros which need a .py suffix
> 
> In libvirt configure.ac we pick from
> 
>rst2man rst2man.py rst2man-3

Technically the rst2man is correct for libvirt-dbus as that's the same
thing we have in libvirt-dbus.spec file, but we install python3 version
into our CI images.

I guess it would be ideal to follow what libvirt is doing so I'll post
a patch that requires python3-docutils or python36-docutils and add all
of the mentioned names into the meson.build file.

Pavel


signature.asc
Description: PGP signature

Races / crashes in shutdown of libvirtd daemon

[Daniel P . Berrangé]

We got a new BZ filed about a libvirtd crash in shutdown

  https://bugzilla.redhat.com/show_bug.cgi?id=1828207

We can see from the stack trace that the "interface" driver is in
the middle of servicing an RPC call for virConnectListAllInterfaces()

Meanwhile the libvirtd daemon is doing virObjectUnref(dmn) on the
virNetDaemonPtr object.

The fact that it is doing this unref, means that it must have already
call virStateCleanup(), given the code sequence:


/* Run event loop. */
virNetDaemonRun(dmn);

ret = 0;

virHookCall(VIR_HOOK_DRIVER_DAEMON, "-", VIR_HOOK_DAEMON_OP_SHUTDOWN,
0, "shutdown", NULL, NULL);

 cleanup:
/* Keep cleanup order in inverse order of startup */
virNetDaemonClose(dmn);

virNetlinkEventServiceStopAll();

if (driversInitialized) {
/* NB: Possible issue with timing window between driversInitialized
 * setting if virNetlinkEventServerStart fails */
driversInitialized = false;
virStateCleanup();
}

virObjectUnref(adminProgram);
virObjectUnref(srvAdm);
virObjectUnref(qemuProgram);
virObjectUnref(lxcProgram);
virObjectUnref(remoteProgram);
virObjectUnref(srv);
virObjectUnref(dmn);


Unless I'm missing something non-obvious, this cleanup code path is
inherantly broken & racy.   When virNetDaemonRun() returns the RPC
worker threads are all still active. They are all liable to still
be executing RPC calls, which means any of the drivers may be in
use. So calling  virStateCleanup() is an inherantly dangerous
thing to do. There is the further complication that once we have
exitted the main loop we may prevent the RPC calls from ever
completing, as they may be waiting on an event to be dispatched.

I know we're had various patch proposals in the past to improve the
robustness of shutdown cleanup but I can't remember the outcome of the
reviews. Hopefully people involved in those threads can jump in here...


IMHO the key problem here is the virNetDeamonRun() method which just
looks at the "quit" flag and immediately returns if it is set.
This needs to be changed so that when it sees quit == true, it takes
the following actions

  1. Call virNetDaemonClose() to drop all RPC clients and thus prevent
 new RPC calls arriving
  2. Flush any RPC calls which are queued but not yet assigned to a
 worker thread
  3. Tell worker threads to exit after finishing their current job
  4. Wait for all worker threads to exit
  5. Now virNetDaemonRun may return

At this point we can call virStateCleanup and the various other
things, as we know no drivers are still active in RPC calls.

Having said that, there could be background threads in the the
drivers which are doing work that uses the event loop thread.

So we probably need a virStateClose() method that we call from
virNetDaemonRun, *after* all worker threads are gone, which would
cleanup any background threads while the event loop is still
running.


The issue is that step 4 above ("Wait for all worker threads to exit")
may take too long, or indeed never complete.  To deal with this, it
will need a timeout. In the remote_daemon.c cleanup code path, if
there are still worker threads present, then we need to skip all
cleanup and simply call _exit(0) to terminate the process with no
attempt at cleanup, since it would be unsafe to try anything else.

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 v5 0/4] introduction of migration_version attribute for VFIO live migration

[Dr. David Alan Gilbert]

* Yan Zhao (yan.y.z...@intel.com) wrote:
> On Sat, Apr 25, 2020 at 03:10:49AM +0800, Dr. David Alan Gilbert wrote:
> > * Yan Zhao (yan.y.z...@intel.com) wrote:
> > > On Tue, Apr 21, 2020 at 08:08:49PM +0800, Tian, Kevin wrote:
> > > > > From: Yan Zhao
> > > > > Sent: Tuesday, April 21, 2020 10:37 AM
> > > > > 
> > > > > On Tue, Apr 21, 2020 at 06:56:00AM +0800, Alex Williamson wrote:
> > > > > > On Sun, 19 Apr 2020 21:24:57 -0400
> > > > > > Yan Zhao  wrote:
> > > > > >
> > > > > > > On Fri, Apr 17, 2020 at 07:24:57PM +0800, Cornelia Huck wrote:
> > > > > > > > On Fri, 17 Apr 2020 05:52:02 -0400
> > > > > > > > Yan Zhao  wrote:
> > > > > > > >
> > > > > > > > > On Fri, Apr 17, 2020 at 04:44:50PM +0800, Cornelia Huck wrote:
> > > > > > > > > > On Mon, 13 Apr 2020 01:52:01 -0400
> > > > > > > > > > Yan Zhao  wrote:
> > > > > > > > > >
> > > > > > > > > > > This patchset introduces a migration_version attribute 
> > > > > > > > > > > under sysfs
> > > > > of VFIO
> > > > > > > > > > > Mediated devices.
> > > > > > > > > > >
> > > > > > > > > > > This migration_version attribute is used to check 
> > > > > > > > > > > migration
> > > > > compatibility
> > > > > > > > > > > between two mdev devices.
> > > > > > > > > > >
> > > > > > > > > > > Currently, it has two locations:
> > > > > > > > > > > (1) under mdev_type node,
> > > > > > > > > > > which can be used even before device creation, but 
> > > > > > > > > > > only for
> > > > > mdev
> > > > > > > > > > > devices of the same mdev type.
> > > > > > > > > > > (2) under mdev device node,
> > > > > > > > > > > which can only be used after the mdev devices are 
> > > > > > > > > > > created, but
> > > > > the src
> > > > > > > > > > > and target mdev devices are not necessarily be of the 
> > > > > > > > > > > same
> > > > > mdev type
> > > > > > > > > > > (The second location is newly added in v5, in order to 
> > > > > > > > > > > keep
> > > > > consistent
> > > > > > > > > > > with the migration_version node for migratable pass-though
> > > > > devices)
> > > > > > > > > >
> > > > > > > > > > What is the relationship between those two attributes?
> > > > > > > > > >
> > > > > > > > > (1) is for mdev devices specifically, and (2) is provided to 
> > > > > > > > > keep the
> > > > > same
> > > > > > > > > sysfs interface as with non-mdev cases. so (2) is for both 
> > > > > > > > > mdev
> > > > > devices and
> > > > > > > > > non-mdev devices.
> > > > > > > > >
> > > > > > > > > in future, if we enable vfio-pci vendor ops, (i.e. a non-mdev 
> > > > > > > > > device
> > > > > > > > > is binding to vfio-pci, but is able to register migration 
> > > > > > > > > region and do
> > > > > > > > > migration transactions from a vendor provided affiliate 
> > > > > > > > > driver),
> > > > > > > > > the vendor driver would export (2) directly, under device 
> > > > > > > > > node.
> > > > > > > > > It is not able to provide (1) as there're no mdev devices 
> > > > > > > > > involved.
> > > > > > > >
> > > > > > > > Ok, creating an alternate attribute for non-mdev devices makes 
> > > > > > > > sense.
> > > > > > > > However, wouldn't that rather be a case (3)? The change here 
> > > > > > > > only
> > > > > > > > refers to mdev devices.
> > > > > > > >
> > > > > > > as you pointed below, (3) and (2) serve the same purpose.
> > > > > > > and I think a possible usage is to migrate between a non-mdev 
> > > > > > > device and
> > > > > > > an mdev device. so I think it's better for them both to use (2) 
> > > > > > > rather
> > > > > > > than creating (3).
> > > > > >
> > > > > > An mdev type is meant to define a software compatible interface, so 
> > > > > > in
> > > > > > the case of mdev->mdev migration, doesn't migrating to a different 
> > > > > > type
> > > > > > fail the most basic of compatibility tests that we expect userspace 
> > > > > > to
> > > > > > perform?  IOW, if two mdev types are migration compatible, it seems 
> > > > > > a
> > > > > > prerequisite to that is that they provide the same software 
> > > > > > interface,
> > > > > > which means they should be the same mdev type.
> > > > > >
> > > > > > In the hybrid cases of mdev->phys or phys->mdev, how does a
> > > > > management
> > > > > > tool begin to even guess what might be compatible?  Are we expecting
> > > > > > libvirt to probe ever device with this attribute in the system?  Is
> > > > > > there going to be a new class hierarchy created to enumerate all
> > > > > > possible migrate-able devices?
> > > > > >
> > > > > yes, management tool needs to guess and test migration compatible
> > > > > between two devices. But I think it's not the problem only for
> > > > > mdev->phys or phys->mdev. even for mdev->mdev, management tool needs
> > > > > to
> > > > > first assume that the two mdevs have the same type of parent devices
> > > > > (e.g.their pciids are equal). otherwise, it's still enumerating
> > > > > possibilities.
> > > > > 
> > > > > on the other hand, for two mdevs,
> > 

Re: [libvirt-dbus PATCH 7/9] man: Convert to reStructuredText

[Daniel P . Berrangé]

On Sat, Apr 25, 2020 at 10:59:44PM +0200, Andrea Bolognani wrote:

> diff --git a/docs/meson.build b/docs/meson.build
> index e62e71a..fbeaf12 100644
> --- a/docs/meson.build
> +++ b/docs/meson.build
> @@ -1,8 +1,8 @@
> -prog_pod2man = find_program('pod2man')
> +prog_rst2man = find_program('rst2man')

This broke on RHEL-7 CI since it needs a -3 suffix, and IIRC will break
other distros which need a .py suffix

In libvirt configure.ac we pick from

   rst2man rst2man.py rst2man-3


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 0/4] move validation of virtio options

[Michal Privoznik]

On 4/23/20 3:15 PM, Bjoern Walk wrote:

Move validation of virtio options (iommu, ats, packed) from QEMU command
line generation to domain validation. As a drive-by, increase the
granularity of tests for virtio options.

Bjoern Walk (4):
   tests: use latest caps for virtio-options test
   tests: more fine-granular tests for virtio-options
   qemu: move virtio capability validation
   qemu: command: make qemuBuildVirtioOptionsStr void

  src/qemu/qemu_command.c   |  59 +++---
  src/qemu/qemu_validate.c  |  70 +++-
  .../virtio-options-controller-ats.args|  32 ++
  .../virtio-options-controller-ats.xml |  38 +++
  .../virtio-options-controller-iommu.args  |  34 ++
  .../virtio-options-controller-iommu.xml   |  38 +++
  .../virtio-options-controller-packed.args |  32 ++
  .../virtio-options-controller-packed.xml  |  38 +++
  .../virtio-options-disk-ats.args  |  36 +++
  .../virtio-options-disk-ats.xml   |  34 ++
  .../virtio-options-disk-iommu.args|  36 +++
  .../virtio-options-disk-iommu.xml |  34 ++
  .../virtio-options-disk-packed.args   |  36 +++
  .../virtio-options-disk-packed.xml|  34 ++
  .../virtio-options-fs-ats.args|  34 ++
  .../virtio-options-fs-ats.xml |  34 ++
  .../virtio-options-fs-iommu.args  |  34 ++
  .../virtio-options-fs-iommu.xml   |  34 ++
  .../virtio-options-fs-packed.args |  34 ++
  .../virtio-options-fs-packed.xml  |  34 ++
  .../virtio-options-input-ats.args |  30 ++
  .../virtio-options-input-ats.xml  |  30 ++
  .../virtio-options-input-iommu.args   |  30 ++
  .../virtio-options-input-iommu.xml|  30 ++
  .../virtio-options-input-packed.args  |  30 ++
  .../virtio-options-input-packed.xml   |  30 ++
  .../virtio-options-memballoon-ats.args|  28 +
  .../virtio-options-memballoon-ats.xml |  23 
  .../virtio-options-memballoon-iommu.args  |  28 +
  .../virtio-options-memballoon-iommu.xml   |  23 
  .../virtio-options-memballoon-packed.args |  28 +
  .../virtio-options-memballoon-packed.xml  |  23 
  .../virtio-options-net-ats.args   |  34 ++
  .../virtio-options-net-ats.xml|  34 ++
  .../virtio-options-net-iommu.args |  34 ++
  .../virtio-options-net-iommu.xml  |  34 ++
  .../virtio-options-net-packed.args|  34 ++
  .../virtio-options-net-packed.xml |  34 ++
  .../virtio-options-rng-ats.args   |  32 ++
  .../virtio-options-rng-ats.xml|  32 ++
  .../virtio-options-rng-iommu.args |  34 ++
  .../virtio-options-rng-iommu.xml  |  32 ++
  .../virtio-options-rng-packed.args|  32 ++
  .../virtio-options-rng-packed.xml |  32 ++
  .../virtio-options-video-ats.args |  34 ++
  .../virtio-options-video-ats.xml  |  36 +++
  .../virtio-options-video-iommu.args   |  34 ++
  .../virtio-options-video-iommu.xml|  36 +++
  .../virtio-options-video-packed.args  |  34 ++
  .../virtio-options-video-packed.xml   |  36 +++
  .../virtio-options.x86_64-latest.args |  69 
  tests/qemuxml2argvdata/virtio-options.xml |   5 +-
  tests/qemuxml2argvtest.c  | 101 --
  .../virtio-options.x86_64-latest.xml  |   1 +
  tests/qemuxml2xmltest.c   |  16 +--
  55 files changed, 1818 insertions(+), 70 deletions(-)
  create mode 100644 tests/qemuxml2argvdata/virtio-options-controller-ats.args
  create mode 100644 tests/qemuxml2argvdata/virtio-options-controller-ats.xml
  create mode 100644 tests/qemuxml2argvdata/virtio-options-controller-iommu.args
  create mode 100644 tests/qemuxml2argvdata/virtio-options-controller-iommu.xml
  create mode 100644 
tests/qemuxml2argvdata/virtio-options-controller-packed.args
  create mode 100644 tests/qemuxml2argvdata/virtio-options-controller-packed.xml
  create mode 100644 tests/qemuxml2argvdata/virtio-options-disk-ats.args
  create mode 100644 tests/qemuxml2argvdata/virtio-options-disk-ats.xml
  create mode 100644 tests/qemuxml2argvdata/virtio-options-disk-iommu.args
  create mode 100644 tests/qemuxml2argvdata/virtio-options-disk-iommu.xml
  create mode 100644 tests/qemuxml2argvdata/virtio-options-disk-packed.args
  create mode 100644 tests/qemuxml2argvdata/virtio-options-disk-packed.xml
  create mode 100644 tests/qemuxml2argvdata/virtio-options-fs-ats.args
  create mode 100644 tests/qemuxml2argvdata/virtio-options-fs-ats.xml
  create mode 100644 

Re: [PATCH 2/4] tests: more fine-granular tests for virtio-options

[Michal Privoznik]

On 4/27/20 4:41 PM, Bjoern Walk wrote:

Michal Privoznik  [2020-04-27, 12:06PM +0200]:

Impressive. But should we turn these into DO_TEST_CAPS_LATEST() and
DO_TEST_CAPS_LATEST_FAILURE() respectively? That would need to be followed
by .args rename but I'm okay doing both locally if you agree.


Hmm, I am still a bit fuzzy on the semantics for the _LATEST() tests.
For the positive tests, I guess that's fine, I don't see any
side-effects with other capabilities. But how does it work with the
negative tests? Can we switch off a capability here, or am I
misunderstanding something? But yes, if it's working, I don't see a
reason not to change it, even better, if you do the work :)


Ah, I thought we are testing different XMLs but we are really testing 
the same XMLs with a different set of capabilities. Yeah, we can change 
the DO_TEST_CAPS_LATEST() then. The negative tests should stay as they 
are. Sorry for the noise.


Michal

Re: [libvirt-dbus PATCH 8/9] meson: Install documentation

[Pavel Hrdina]

On Mon, Apr 27, 2020 at 04:54:33PM +0200, Andrea Bolognani wrote:
> On Mon, 2020-04-27 at 15:30 +0100, Daniel P. Berrangé wrote:
> > On Mon, Apr 27, 2020 at 04:16:57PM +0200, Andrea Bolognani wrote:
> > > On Mon, 2020-04-27 at 15:38 +0200, Pavel Hrdina wrote:
> > > > I've checked systemd and they are installing these files using meson but
> > > > GLib for example is not installing them.
> > > > 
> > > > After checking other libvirt projects we are not installing this files
> > > > using autotools or other build systems.
> > > 
> > > I can bring up another counter-example that's close to us: QEMU
> > > installs these files, along with a bunch of other documentation.
> > 
> > QEMU isn't a good example of best practice as it is a completely
> > custom written set of build system rules.
> 
> The fact that it accomplishes something using a custom set of tools
> doesn't automatically make the result not a good idea.
> 
> > > The packaging for both Fedora and Debian go out of their way to
> > > include them, and at least for the latter it's strongly recommended
> > > by policy to include NEWS while including COPYING is mandatory.
> > > Clearly both projects consider this information important to users,
> > > and I don't see any disadvantage in installing them even for people
> > > who build from sources directly instead of using a package, only
> > > advantages really.
> > 
> > I think these are different situations though. Distro vendors include
> > the files because the end users don't have any direct visibility of
> > the source tarball. So the only way users would see the files is if
> > the binary packages included them. I don't think that extends to
> > building from source in general & don't see a need for us to do this
> > specially in the libvirt-dbus module, or indeed our other modules
> > in general.
> 
> The fact that something is not strictly needed doesn't automatically
> make it not worth pursuing.
> 
> 
> Anyway, I don't care enough about this to keep the discussion going
> any further, so I'm just going to go ahead and revert the change.
> 
> Can we have a libvirt-dbus release now? :)

Yes and no :) when I was testing your changes the test suite failed for
me as it was missing dbus-daemon package which is no longer installed by
default on Fedora.  I'm already looking into dbus-broker and how to use
it instead but it looks like it might not be that ease :/.

I'll continue with the investigation to see if there is some way how we
can use dbus-broker for our testing or I'll document somewhere that we
require dbus-daemon and why.

Pavel


signature.asc
Description: PGP signature

[libvirt-dbus PATCH 1/2] spec: Pick documentation from source directory

[Andrea Bolognani]

We will stop installing documentation very soon, at which point we
will not longer be able to pick it from the installation directory.

This reverts commit 651f43f5e4a88bdbe8bbb6f7fa5f44b5c176ade8.

Signed-off-by: Andrea Bolognani 
---
 libvirt-dbus.spec.in | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/libvirt-dbus.spec.in b/libvirt-dbus.spec.in
index e756ff6..84172fa 100644
--- a/libvirt-dbus.spec.in
+++ b/libvirt-dbus.spec.in
@@ -50,9 +50,7 @@ getent passwd %{system_user} >/dev/null || \
 exit 0
 
 %files
-%doc %{_docdir}/%{name}/AUTHORS.rst
-%doc %{_docdir}/%{name}/NEWS.rst
-%exclude %{_docdir}/%{name}/COPYING
+%doc AUTHORS.rst NEWS.rst
 %license COPYING
 %{_sbindir}/libvirt-dbus
 %{_datadir}/dbus-1/services/org.libvirt.service
-- 
2.25.4

[libvirt-dbus PATCH 2/2] meson: Don't install documentation

[Andrea Bolognani]

Standard practice is to not install these files when building from
source.

This reverts commit 64a1a72be1ecac30359e6964557eeffedc06bb7d.

Signed-off-by: Andrea Bolognani 
---
 meson.build | 18 --
 1 file changed, 18 deletions(-)

diff --git a/meson.build b/meson.build
index 64e1fb6..7518e94 100644
--- a/meson.build
+++ b/meson.build
@@ -12,7 +12,6 @@ project(
 prefix = get_option('prefix')
 datadir = prefix / get_option('datadir')
 sbindir = prefix / get_option('sbindir')
-docdir = datadir / 'doc' / meson.project_name()
 
 opt_dirs = [
 'dbus_interfaces',
@@ -259,7 +258,6 @@ if git
 configuration: { 'contributorslist': authors.stdout() },
 input: 'AUTHORS.rst.in',
 output: 'AUTHORS.rst',
-install_dir: docdir,
 )
 
 foreach file : [ 'libvirt-dbus.spec', 'AUTHORS.rst' ]
@@ -268,22 +266,6 @@ if git
 endif
 
 
-# Install documentation
-
-docs = [
-'COPYING',
-'NEWS.rst',
-]
-if not git
-docs += ['AUTHORS.rst']
-endif
-
-install_data(
-docs,
-install_dir: docdir,
-)
-
-
 # Include sub-directories
 
 subdir('data')
-- 
2.25.4

[libvirt-dbus PATCH 0/2] Revert recent commits

[Andrea Bolognani]

Pushed under the "we agreed to get rid of this" rule.

Andrea Bolognani (2):
  spec: Pick documentation from source directory
  meson: Don't install documentation

 libvirt-dbus.spec.in |  4 +---
 meson.build  | 18 --
 2 files changed, 1 insertion(+), 21 deletions(-)

-- 
2.25.4

Re: [libvirt-dbus PATCH 8/9] meson: Install documentation

[Andrea Bolognani]

On Mon, 2020-04-27 at 15:30 +0100, Daniel P. Berrangé wrote:
> On Mon, Apr 27, 2020 at 04:16:57PM +0200, Andrea Bolognani wrote:
> > On Mon, 2020-04-27 at 15:38 +0200, Pavel Hrdina wrote:
> > > I've checked systemd and they are installing these files using meson but
> > > GLib for example is not installing them.
> > > 
> > > After checking other libvirt projects we are not installing this files
> > > using autotools or other build systems.
> > 
> > I can bring up another counter-example that's close to us: QEMU
> > installs these files, along with a bunch of other documentation.
> 
> QEMU isn't a good example of best practice as it is a completely
> custom written set of build system rules.

The fact that it accomplishes something using a custom set of tools
doesn't automatically make the result not a good idea.

> > The packaging for both Fedora and Debian go out of their way to
> > include them, and at least for the latter it's strongly recommended
> > by policy to include NEWS while including COPYING is mandatory.
> > Clearly both projects consider this information important to users,
> > and I don't see any disadvantage in installing them even for people
> > who build from sources directly instead of using a package, only
> > advantages really.
> 
> I think these are different situations though. Distro vendors include
> the files because the end users don't have any direct visibility of
> the source tarball. So the only way users would see the files is if
> the binary packages included them. I don't think that extends to
> building from source in general & don't see a need for us to do this
> specially in the libvirt-dbus module, or indeed our other modules
> in general.

The fact that something is not strictly needed doesn't automatically
make it not worth pursuing.


Anyway, I don't care enough about this to keep the discussion going
any further, so I'm just going to go ahead and revert the change.

Can we have a libvirt-dbus release now? :)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH] qemu: re-add padding to the saved state images

[Eric Blake]

On 4/27/20 8:41 AM, Daniel P. Berrangé wrote:

In the past we added 1024 bytes of padding to saved state images so that
users can run "virsh managedsave-edit $GUEST" and make XML changes which
increase the size of the XML document. This padding was accidentally
lost a while back

   commit 6b9b21db7079888a05d192b079e68290bdf14a76
   Author: Peter Krempa 
   Date:   Wed Feb 17 13:10:11 2016 +0100

 qemu: Remove unnecessary calculations in qemuDomainSaveMemory

The original 1024 bytes was unreasonably stingy when we consider that
the QEMU state is typically going to be many 100's of MB in size. Thus
this adds 64 KB of padding after the XML which should cope with any
plausible modifications a user will want to make.

Signed-off-by: Daniel P. Berrangé 
---
  src/qemu/qemu_driver.c | 17 ++---
  1 file changed, 10 insertions(+), 7 deletions(-)


Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 0/4] move validation of virtio options

[Bjoern Walk]

Daniel Henrique Barboza  [2020-04-24, 05:57PM -0300]:
> 
> 
> On 4/23/20 10:15 AM, Bjoern Walk wrote:
> > Move validation of virtio options (iommu, ats, packed) from QEMU command
> > line generation to domain validation. As a drive-by, increase the
> > granularity of tests for virtio options.
> 
> Thanks for contributing with the move of validation code. The new
> fine-grained tests are a nice touch too.
> 
> 
> All patches:
> 
> Reviewed-by: Daniel Henrique Barboza 

Thanks!

-- 
IBM Systems
Linux on Z & Virtualization Development
--
IBM Deutschland Research & Development GmbH
Schönaicher Str. 220, 71032 Böblingen
Phone: +49 7031 16 1819
--
Vorsitzende des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


signature.asc
Description: PGP signature

Re: [PATCH 1/4] tests: use latest caps for virtio-options test

[Bjoern Walk]

Michal Privoznik  [2020-04-27, 12:06PM +0200]:
> On 4/23/20 3:15 PM, Bjoern Walk wrote:
> > Convert the virtio-options test in qemuxml2argv and qemuxml2xml to use
> > the latest available QEMU capabilities.
> > 
> > Reviewed-by: Boris Fiuczynski 
> > Signed-off-by: Bjoern Walk 
> > ---
> >   .../virtio-options.x86_64-latest.args | 69 +++
> >   tests/qemuxml2argvdata/virtio-options.xml |  5 +-
> >   tests/qemuxml2argvtest.c  | 12 +---
> >   .../virtio-options.x86_64-latest.xml  |  1 +
> >   tests/qemuxml2xmltest.c   | 16 +
> >   5 files changed, 76 insertions(+), 27 deletions(-)
> >   create mode 100644 
> > tests/qemuxml2argvdata/virtio-options.x86_64-latest.args
> >   create mode 12 
> > tests/qemuxml2xmloutdata/virtio-options.x86_64-latest.xml
> 
> This means that tests/qemuxml2argvdata/virtio-options.args can be removed
> then (what git detects as rename). No need to resend, I will fix locally
> before pushing after we figure out 2/4.

Right, missed that artifact. Thanks for fixing.

> 
> Michal
> 


signature.asc
Description: PGP signature

Re: [libvirt-ci PATCH 03/13] lcitool: Prefer tempfile's native wrappers over low level primitives

[Andrea Bolognani]

On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
> Rather than requiring shutil module to get rid of the temporary
> directory we're creating for virt-install, let's use the
> TemporaryDirectory method instead which returns a file-like object which
> can be used to clean up the standard Python way.
> Although the internal exit handlers will take care of closing the
> temporary directories (and thus removing their contents) automatically,
> let's be explicit anyway and use the 'finally' clause to clean these up
> on both success and failure.
> 
> Signed-off-by: Erik Skultety 
> ---
>  guests/lcitool | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH 2/4] tests: more fine-granular tests for virtio-options

[Bjoern Walk]

Michal Privoznik  [2020-04-27, 12:06PM +0200]:
> Impressive. But should we turn these into DO_TEST_CAPS_LATEST() and
> DO_TEST_CAPS_LATEST_FAILURE() respectively? That would need to be followed
> by .args rename but I'm okay doing both locally if you agree.

Hmm, I am still a bit fuzzy on the semantics for the _LATEST() tests.
For the positive tests, I guess that's fine, I don't see any
side-effects with other capabilities. But how does it work with the
negative tests? Can we switch off a capability here, or am I
misunderstanding something? But yes, if it's working, I don't see a
reason not to change it, even better, if you do the work :)

Thanks.

> 
> Michal
> 


signature.asc
Description: PGP signature

Re: [libvirt-dbus PATCH 8/9] meson: Install documentation

[Daniel P . Berrangé]

On Mon, Apr 27, 2020 at 04:16:57PM +0200, Andrea Bolognani wrote:
> Sorry, I only see these messages now, after having pushed already :(
> 
> On Mon, 2020-04-27 at 15:38 +0200, Pavel Hrdina wrote:
> > On Mon, Apr 27, 2020 at 01:52:13PM +0100, Daniel P. Berrangé wrote:
> > > On Sat, Apr 25, 2020 at 10:59:45PM +0200, Andrea Bolognani wrote:
> > > > People who install the RPM packages already get this, but it's
> > > > good practice so let's make it happen for those who install from
> > > > source as well.
> > > 
> > > Do we really want this ?  I've not noticed apps doing this in
> > > general - these files are usually non-installed with autotools
> > > and meson AFAIK.
> > 
> > It was probably my motivation not to install them in the first place as
> > well because the user already has the files from sources so there is
> > usually no need to install them.
> 
> One doesn't necessarily keep the source directory around after
> installing a piece or software, and the person installing it might
> be different from the one interested in looking at these files:
> think the user on a managed server curious about what features a
> newly-deployed version of some software introduces.
>
> > I've checked systemd and they are installing these files using meson but
> > GLib for example is not installing them.
> > 
> > After checking other libvirt projects we are not installing this files
> > using autotools or other build systems.
> 
> I can bring up another counter-example that's close to us: QEMU
> installs these files, along with a bunch of other documentation.

QEMU isn't a good example of best practice as it is a completely
custom written set of build system rules.  

> > Taking all of that into account I'm changing my mind and we should not
> > install these files from sources as well.
> 
> I still think it's a good idea.
> 
> The packaging for both Fedora and Debian go out of their way to
> include them, and at least for the latter it's strongly recommended
> by policy to include NEWS while including COPYING is mandatory.
> Clearly both projects consider this information important to users,
> and I don't see any disadvantage in installing them even for people
> who build from sources directly instead of using a package, only
> advantages really.

I think these are different situations though. Distro vendors include
the files because the end users don't have any direct visibility of
the source tarball. So the only way users would see the files is if
the binary packages included them. I don't think that extends to
building from source in general & don't see a need for us to do this
specially in the libvirt-dbus module, or indeed our other modules
in general.

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-ci PATCH 02/13] lcitool: Decrease the indent when creating a tempdir for initrd injection

[Andrea Bolognani]

On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
> The 'with' statement doesn't define a new code block [1], thus no local
> namespace is created. Therefore, we can still access the @content
> variable outside of the 'with' block. So, there's really no need to
> hold the @initrd_template file open longer than necessary (not that it
> would be a big deal anyway).
> 
> [1] https://docs.python.org/3.7/reference/executionmodel.html
> 
> Signed-off-by: Erik Skultety 
> ---
>  guests/lcitool | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt-dbus PATCH 8/9] meson: Install documentation

[Andrea Bolognani]

Sorry, I only see these messages now, after having pushed already :(

On Mon, 2020-04-27 at 15:38 +0200, Pavel Hrdina wrote:
> On Mon, Apr 27, 2020 at 01:52:13PM +0100, Daniel P. Berrangé wrote:
> > On Sat, Apr 25, 2020 at 10:59:45PM +0200, Andrea Bolognani wrote:
> > > People who install the RPM packages already get this, but it's
> > > good practice so let's make it happen for those who install from
> > > source as well.
> > 
> > Do we really want this ?  I've not noticed apps doing this in
> > general - these files are usually non-installed with autotools
> > and meson AFAIK.
> 
> It was probably my motivation not to install them in the first place as
> well because the user already has the files from sources so there is
> usually no need to install them.

One doesn't necessarily keep the source directory around after
installing a piece or software, and the person installing it might
be different from the one interested in looking at these files:
think the user on a managed server curious about what features a
newly-deployed version of some software introduces.

> I've checked systemd and they are installing these files using meson but
> GLib for example is not installing them.
> 
> After checking other libvirt projects we are not installing this files
> using autotools or other build systems.

I can bring up another counter-example that's close to us: QEMU
installs these files, along with a bunch of other documentation.

> Taking all of that into account I'm changing my mind and we should not
> install these files from sources as well.

I still think it's a good idea.

The packaging for both Fedora and Debian go out of their way to
include them, and at least for the latter it's strongly recommended
by policy to include NEWS while including COPYING is mandatory.
Clearly both projects consider this information important to users,
and I don't see any disadvantage in installing them even for people
who build from sources directly instead of using a package, only
advantages really.

That said, if you guys feel strongly against installing them, we
can revert my last two commits quite easily :)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt-ci PATCH 01/13] requirements: Introduce a requirements.txt file

[Andrea Bolognani]

On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
> Right now, lcitool's users have 2 options to satisfy the tool's
> dependencies: install them through the system package manager
> system-wide or install through pip manually.
> Since pip gives us the option to automate this process a tiny bit, let's
> ship a requirements file. This targets users who are used to work
> with Python virtual environments and install whatever it is necessary
> inside.

I remember Katerina suggesting that we add this, literally years
ago O:-)

> +++ b/requirements.txt

This is only relevat to lcitool, so it should go into the guests/
directory.

> +ansible
> +jenkins-job-builder
> +toml

You don't actually use the toml module until 6/13, so leave this
last line out until then.

I haven't actually tested whether this work, but I trust you did
and it certainly will not get in the way, so with the above fixed

  Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt-ci RFC PATCH 00/13] Introduce a new global TOML config file for lcitool

[Andrea Bolognani]

On Fri, 2020-04-24 at 08:47 +0200, Erik Skultety wrote:
> On Thu, Apr 23, 2020 at 06:46:09PM +0200, Andrea Bolognani wrote:
> > Now, I know I'm the one who suggested TOML in the first place... But
> > looking at the series now I can't help but think, why not YAML? O:-)
> 
> To be honest, even before you originally mentioned TOML, I myself had INI in
> mind, so then I thought, yeah, why not go with TOML then, it's similar and 
> more
> powerful.
> I did some comparison of several formats, because like you say, with YAML we'd
> be more close to Ansible and I was on the cusp of choosing between YAML and
> TOML and I felt like TOML was still more readable and expressive in terms of
> simple configuration (and that's what Linux users are IMO used to from INI).

Are you sure you didn't mean s/Linux/Windows/ here? ;)

> I'd still prefer TOML, but I don't really have a compelling reason to argue
> against YAML other than readability which I already admitted to be just a
> matter of taste. Now on a more serious note, I'll wait for your detailed 
> review
> and then rework it in YAML in vX.

Eh, you know what, whatever. YAML is fine, TOML is fine, bringing
in an additional Python module is not a big deal. Let's fix actual
issues (if there are any) and skip at least some of the bikeshedding.

-- 
Andrea Bolognani / Red Hat / Virtualization

[PATCH libvirt-python 5/5] libvirtaio: Fix return types of callback

[Philipp Hahn]

libvirt defines the signature for the callback functions, e.g. the
functions for remove() must return -1 on error and 0 on success. Raising
an exception violates that contract.

_remove_timeout() did not explicitly handle a double-remove and
implicitly passed on the exception.

update() expects no return value, so remove the pointless return to pass
on None.

Signed-off-by: Philipp Hahn 
---
 libvirtaio.py | 18 --
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/libvirtaio.py b/libvirtaio.py
index 328e6f2..f1811c1 100644
--- a/libvirtaio.py
+++ b/libvirtaio.py
@@ -370,13 +370,13 @@ class virEventAsyncIOImpl(object):
 
https://libvirt.org/html/libvirt-libvirt-event.html#virEventUpdateHandleFunc
 '''
 self.log.debug('update_handle(watch=%d, event=%d)', watch, event)
-return self.callbacks[watch].update(event=event)
+self.callbacks[watch].update(event=event)
 
 def _remove_handle(self, watch):
 '''Unregister a callback from a file handle.
 
 :param int watch: file descriptor watch to stop listening on
-:returns: None (see source for explanation)
+:returns: -1 on error, 0 on success
 
 .. seealso::
 
https://libvirt.org/html/libvirt-libvirt-event.html#virEventRemoveHandleFunc
@@ -386,12 +386,13 @@ class virEventAsyncIOImpl(object):
 callback = self.callbacks.pop(watch)
 except KeyError as err:
 self.log.warning('remove_handle(): no such handle: %r', 
err.args[0])
-raise
+return -1
 fd = callback.descriptor.fd
 assert callback is self.descriptors[fd].remove_handle(watch)
 if len(self.descriptors[fd].callbacks) == 0:
 del self.descriptors[fd]
 callback.close()
+return 0
 
 def _add_timeout(self, timeout, cb, opaque):
 '''Register a callback for a timer event
@@ -425,20 +426,25 @@ class virEventAsyncIOImpl(object):
 
https://libvirt.org/html/libvirt-libvirt-event.html#virEventUpdateTimeoutFunc
 '''
 self.log.debug('update_timeout(timer=%d, timeout=%d)', timer, timeout)
-return self.callbacks[timer].update(timeout=timeout)
+self.callbacks[timer].update(timeout=timeout)
 
 def _remove_timeout(self, timer):
 '''Unregister a callback for a timer
 
 :param int timer: the timer to remove
-:returns: None (see source for explanation)
+:returns: -1 on error, 0 on success
 
 .. seealso::
 
https://libvirt.org/html/libvirt-libvirt-event.html#virEventRemoveTimeoutFunc
 '''
 self.log.debug('remove_timeout(timer=%d)', timer)
-callback = self.callbacks.pop(timer)
+try:
+callback = self.callbacks.pop(timer)
+except KeyError as err:
+self.log.warning('remove_timeout(): no such timeout: %r', 
err.args[0])
+return -1
 callback.close()
+return 0
 
 
 _current_impl = None
-- 
2.20.1

[PATCH libvirt-python 1/5] generator: Fix undefined variables file

[Philipp Hahn]

Signed-off-by: Philipp Hahn 
---
 generator.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/generator.py b/generator.py
index 426f007..7f4ae1f 100755
--- a/generator.py
+++ b/generator.py
@@ -928,7 +928,7 @@ def buildStubs(module, api_xml):
 parser.close()
 except IOError:
 msg = sys.exc_info()[1]
-print(file, ":", msg)
+print(api_xml, ":", msg)
 sys.exit(1)
 
 n = len(list(funcs.keys()))
@@ -948,7 +948,7 @@ def buildStubs(module, api_xml):
 parser.close()
 except IOError:
 msg = sys.exc_info()[1]
-print(file, ":", msg)
+print(override_api_xml, ":", msg)
 
 if not quiet:
 # XXX: This is not right, same function already in @functions
-- 
2.20.1

[PATCH libvirt-python 2/5] generator: Fix string formatting

[Philipp Hahn]

remove excessive arguments.

Signed-off-by: Philipp Hahn 
---
 generator.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/generator.py b/generator.py
index 7f4ae1f..a14 100755
--- a/generator.py
+++ b/generator.py
@@ -766,7 +766,7 @@ def print_function_wrapper(module, name, output, export, 
include):
 if file == "python_accessor":
 if args[1][1] == "char *":
 c_call = "\nVIR_FREE(%s->%s);\n" % (
- args[0][0], args[1][0], args[0][0], 
args[1][0])
+ args[0][0], args[1][0])
 c_call = c_call + "%s->%s = (%s)strdup((const xmlChar 
*)%s);\n" % (args[0][0],
  args[1][0], args[1][1], args[1][0])
 else:
-- 
2.20.1

[PATCH libvirt-python 3/5] generator: Fix domainSnapshot.listAllChildren()

[Philipp Hahn]

virDomainSnapshot(dom, _obj)
expects a reference to the virDomain as its first argument, but
virDomainSnapshot.listAllChildren()
passes `self` instead:

libvirt.py:6459: error: Argument 1 to "virDomainSnapshot" has incompatible type 
"virDomainSnapshot"; expected "virDomain"

>>> import libvirt
>>> con = libvirt.open('test:///default')
>>> dom = con.lookupByName("test")
>>> first = 
>>> dom.snapshotCreateXML("""First""")
>>> second = 
>>> dom.snapshotCreateXML("""Second""")
>>> child, == first.listAllChildren()
>>> second.domain()

 ^
>>> child.domain()

 ^

Signed-off-by: Philipp Hahn 
---
 libvirt-override-virDomainSnapshot.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libvirt-override-virDomainSnapshot.py 
b/libvirt-override-virDomainSnapshot.py
index ec53358..87eac86 100644
--- a/libvirt-override-virDomainSnapshot.py
+++ b/libvirt-override-virDomainSnapshot.py
@@ -14,6 +14,6 @@
 
 retlist = list()
 for snapptr in ret:
-retlist.append(virDomainSnapshot(self, _obj=snapptr))
+retlist.append(virDomainSnapshot(self.domain(), _obj=snapptr))
 
 return retlist
-- 
2.20.1

[PATCH libvirt-python 4/5] qemu-api: Fix return type

[Philipp Hahn]

The API XML description uses "C types": "str *" is not valid.

Signed-off-by: Philipp Hahn 
---
 libvirt-qemu-override-api.xml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libvirt-qemu-override-api.xml b/libvirt-qemu-override-api.xml
index ca0dae9..b280a88 100644
--- a/libvirt-qemu-override-api.xml
+++ b/libvirt-qemu-override-api.xml
@@ -3,14 +3,14 @@
   
   
 Send an arbitrary monitor command through qemu monitor of 
domain
-
+
 
 
 
   
   
 Send a Guest Agent command to domain
-
+
 
 
 
-- 
2.20.1

[PATCH libvirt-python 0/5] Fixes from adding type annotation

[Philipp Hahn]

Hello,

as announced a long time ago with

and recently refreshed with

I'm working on adding PEP 484 type hints
 to the Python binding of
libvirt.

I have finished this work now and have a working version at
 which
consists of 90 patches in total as I has to go over evry file to
understand and fix all things.

As that patch bomb is quiet large I'm going to submit them in smaller
chunks to make them more reviewable. Today I start with the first round
consisting of "real" bugs in the current code:

Philipp Hahn (5):
  generator: Fix undefined variables file
  generator: Fix string formatting
  generator: Fix domainSnapshot.listAllChildren()
  qemu-api: Fix return type
  libvirtaio: Fix return types of callback


 generator.py  |  6 +++---
 libvirt-override-virDomainSnapshot.py |  2 +-
 libvirt-qemu-override-api.xml |  4 ++--
 libvirtaio.py | 18 --
 4 files changed, 18 insertions(+), 12 deletions(-)


After that I plan to continue with:

2. fix examples/ to work with Python 3
3. Cleanup code tree-wide
4. Cleanup generator.py
5. Cleanup sanitytest.py
6. Teach generator.py to add PEP 484 annotation
7. Assorted cleanups

(the order and chunking is not final yet)

> examples/domipaddrs: Convert to python 3 print()
> examples/domipaddrs: Fix Python 2 dict.iteritems()
> examples/*: Remove stray semicolon
> example/dhcp*: Fix None comparison
> examples/event-test: Remove unneeded global statement
> examples/event-test: Work with old version of python-libvirt
> examples/event-test: Use atexit for Python 3
> examples/esxlist: Fix Python 2 raw_input()
> examples/consolecallback: Add var to save callback
> examples/consolecallback: Fix assorted errors
> examples: Add missing return values

> libvirtaio: Drop object(*args, **kwargs)
> libvirtaio: Fix return type
> libvirtaio: assert callback type

> Do not use bare except
> Cleanup imports
> Fix white space
> Remove legacy libvirtError arguments

> stream: Fix exception traceback handling
> override: Simplify exception handling
> generator: Simplify exception handling
> generator: Change type of quiet to bool
> generator: Remove unneeded line continuation
> generator: Convert to 'not in' and 'is not'
> generator: Remove dead variable assignments
> generator: Remove skipped_modules
> generator: Remove useless sort key
> generator: Fix return type on failure
> generator: Merge now identical if-elif-else cases
> generator: Use more string formatting
> generator: Simplify string concatentaion
> generator: Use enumerate()
> generator: Use increment assignment
> generator: Use string concatenation
> generator: Remove global declarations
> generator: Initialize function_classes directly
> generator: Check contained in hash
> generator: Use dict.item() to walk keys and values
> generator: Walk only the values
> generator: Directly get dict length
> generator: Just walk the dict
> generator: Use splitlines()
> generator: Open file with context manager
> generator: Refactor parser creation
> generator: Remove unused SAX content handler methods
> generator: Use SAX method names
> generator: Use string formatting
> generator: Convert in_function to boolean
> generator: Simplify XML attribute fetching
> generator: Initialize with empty strings
> generator: Expand tuple to names in for loop
> generator: Store arguments and return as tuple
> generator: Fixed writing cached=None
> generator: Simplify sorting
> generator: Simplify loop break
> generator: Simplify boolean condition
> generator: Convert dict() to set()
> generator: Converto to defaultdict()
> generator: Add PEP 484 type annotations
> override: Add manual PEP 484 type annotations
> sanitytest: Skip type annotations
> stream: Simplify boolean condition
> domain: Fix None comparison
> stream: no type change
> stream: Convert type() to isinstance()
> stream: Return None from callback
> connect: Just clear all event handlers
> override: no type change
> sanitytest: Do not re-declare set
> sanitytest: Drop else:pass
> sanitytest: Drop Python 2 compatibility
> sanitytest: Add PEP 484 type annotations
> sanitytest: Use 3-tuple for basicklassmap
> sanitytest: Use 3-tuple for finalklassmap
> sanitytest: Use set for tracking used functions
> sanitytest: Use str.startswith() instead of str[0]
> generator: Generate PEP 484 type annotation
> override: Catch type error
> generator: Special handling for virStoragePool.listAllVolumes
> generator: Merge code for __init__ genration
> generator: Use empty string instead of None
> generator: break lines in generated code
> generator: Expand tuple to names in for loop
> generator: Work around type change
> generator: use pointer wrapper for all objects

> examples: Add/fix PEP 484 

[libvirt PATCH] qemu: re-add padding to the saved state images

[Daniel P . Berrangé]

In the past we added 1024 bytes of padding to saved state images so that
users can run "virsh managedsave-edit $GUEST" and make XML changes which
increase the size of the XML document. This padding was accidentally
lost a while back

  commit 6b9b21db7079888a05d192b079e68290bdf14a76
  Author: Peter Krempa 
  Date:   Wed Feb 17 13:10:11 2016 +0100

qemu: Remove unnecessary calculations in qemuDomainSaveMemory

The original 1024 bytes was unreasonably stingy when we consider that
the QEMU state is typically going to be many 100's of MB in size. Thus
this adds 64 KB of padding after the XML which should cope with any
plausible modifications a user will want to make.

Signed-off-by: Daniel P. Berrangé 
---
 src/qemu/qemu_driver.c | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 01330e55e7..d5aeeac66d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2922,20 +2922,23 @@ virQEMUSaveDataWrite(virQEMUSaveDataPtr data,
 
 len = xml_len + cookie_len;
 
-if (header->data_len > 0) {
+if (header->data_len == 0) {
+/* This 64kb padding allows the user to edit the XML in
+ * a saved state image and have the new XML be larger
+ * that what was originally saved
+ */
+header->data_len = len + (64 * 1024);
+} else {
 if (len > header->data_len) {
 virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("new xml too large to fit in file"));
 return -1;
 }
-
-zerosLen = header->data_len - len;
-if (VIR_ALLOC_N(zeros, zerosLen) < 0)
-return -1;
-} else {
-header->data_len = len;
 }
 
+zerosLen = header->data_len - len;
+zeros = g_new0(char, zerosLen);
+
 if (data->cookie)
 header->cookieOffset = xml_len;
 
-- 
2.25.3

Re: [libvirt-dbus PATCH 8/9] meson: Install documentation

[Pavel Hrdina]

On Mon, Apr 27, 2020 at 01:52:13PM +0100, Daniel P. Berrangé wrote:
> On Sat, Apr 25, 2020 at 10:59:45PM +0200, Andrea Bolognani wrote:
> > People who install the RPM packages already get this, but it's
> > good practice so let's make it happen for those who install from
> > source as well.
> 
> Do we really want this ?  I've not noticed apps doing this in
> general - these files are usually non-installed with autotools
> and meson AFAIK.

It was probably my motivation not to install them in the first place as
well because the user already has the files from sources so there is
usually no need to install them.

I've checked systemd and they are installing these files using meson but
GLib for example is not installing them.

After checking other libvirt projects we are not installing this files
using autotools or other build systems.

Taking all of that into account I'm changing my mind and we should not
install these files from sources as well.

Pavel


signature.asc
Description: PGP signature

Re: [libvirt-dbus PATCH 8/9] meson: Install documentation

[Andrea Bolognani]

On Mon, 2020-04-27 at 14:48 +0200, Andrea Bolognani wrote:
> On Mon, 2020-04-27 at 14:20 +0200, Pavel Hrdina wrote:
> > On Sat, Apr 25, 2020 at 10:59:45PM +0200, Andrea Bolognani wrote:
> > >  prefix = get_option('prefix')
> > >  datadir = prefix / get_option('datadir')
> > >  sbindir = prefix / get_option('sbindir')
> > > +set_variable('docdir', datadir / 'doc/libvirt-dbus')
> > 
> > No need to use set_variable(), you can use directly:
> > 
> > docdir = datadir / 'doc' / 'libvirt-dbus'
> > 
> > The set_variable() function is usually used in places where you need to
> > dynamically generate the variable name.
> 
> I see! This is my first time touching a Meson-based build system,
> so I mostly copy-pasted my way through O:-)
> 
> I'll adopt your version.

I'm actually going to use a slight variation,

  docdir = datadir / 'doc' / meson.project_name()

to avoid repetition.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH 2/2] CONTRIBUTING: Include note about build system tools

[Andrea Bolognani]

On Mon, 2020-04-27 at 14:05 +0200, Michal Privoznik wrote:
> On 4/27/20 1:15 PM, Andrea Bolognani wrote:
> > On Mon, 2020-04-27 at 12:03 +0100, Daniel P. Berrangé wrote:
> > > This group doesn't exist on any RHEL distro, and dnf itself only
> > > exists in RHEL >= 8.  I htink we should be just listing the packages
> > > we need explicitly rather than relying on a group whose name & contents
> > > change over time.
> > 
> > I expect most people doing development work on libvirt are using
> > Fedora rather than RHEL,
> 
> I beg to differ :-)

So you use RHEL for libvirt development?

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt-dbus PATCH 8/9] meson: Install documentation

[Daniel P . Berrangé]

On Sat, Apr 25, 2020 at 10:59:45PM +0200, Andrea Bolognani wrote:
> People who install the RPM packages already get this, but it's
> good practice so let's make it happen for those who install from
> source as well.

Do we really want this ?  I've not noticed apps doing this in
general - these files are usually non-installed with autotools
and meson AFAIK.

> 
> The process is straightforward, but we have to be a bit careful
> with AUTHORS because this specific file is found in different
> directories depending on whether we're building from git or from
> a release.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  meson.build | 18 ++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/meson.build b/meson.build
> index 7518e94..9395bc4 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -12,6 +12,7 @@ project(
>  prefix = get_option('prefix')
>  datadir = prefix / get_option('datadir')
>  sbindir = prefix / get_option('sbindir')
> +set_variable('docdir', datadir / 'doc/libvirt-dbus')
>  
>  opt_dirs = [
>  'dbus_interfaces',
> @@ -258,6 +259,7 @@ if git
>  configuration: { 'contributorslist': authors.stdout() },
>  input: 'AUTHORS.rst.in',
>  output: 'AUTHORS.rst',
> +install_dir: docdir,
>  )
>  
>  foreach file : [ 'libvirt-dbus.spec', 'AUTHORS.rst' ]
> @@ -266,6 +268,22 @@ if git
>  endif
>  
>  
> +# Install documentation
> +
> +docs = [
> +'COPYING',
> +'NEWS.rst',
> +]
> +if not git
> +docs += ['AUTHORS.rst']
> +endif
> +
> +install_data(
> +docs,
> +install_dir: docdir,
> +)
> +
> +
>  # Include sub-directories
>  
>  subdir('data')
> -- 
> 2.25.3
> 

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-dbus PATCH 9/9] spec: Pick documentation from installation directory

[Andrea Bolognani]

On Mon, 2020-04-27 at 14:39 +0200, Pavel Hrdina wrote:
> On Sat, Apr 25, 2020 at 10:59:46PM +0200, Andrea Bolognani wrote:
> >  %files
> > -%doc AUTHORS.rst NEWS.rst
> > -%license COPYING
> > +%doc %{_docdir}/%{name}/AUTHORS.rst
> > +%doc %{_docdir}/%{name}/NEWS.rst
> > +%license %{_docdir}/%{name}/COPYING
> 
> This patch will change the location of COPYING file from
> 
> /usr/share/licenses/libvirt-dbus/COPYING
> 
> to
> 
> /usr/share/doc/libvirt-dbus/COPYING
> 
> Which is probably OK but the preferred location is the licenses
> directory.  I would like to keep this behavior for RPM packages
> which means move the file in the %install phase or using this:
> 
> %exclude %{_docdir}/%{name}/COPYING
> %license COPYING

That's a nuance of RPM packaging that I completely missed.

I'll squash in the %exclude trick you suggest and verify that the
file actually ends up in the expected location before pushing.

Thanks for the speedy review! :)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt-dbus PATCH 8/9] meson: Install documentation

[Andrea Bolognani]

On Mon, 2020-04-27 at 14:20 +0200, Pavel Hrdina wrote:
> On Sat, Apr 25, 2020 at 10:59:45PM +0200, Andrea Bolognani wrote:
> >  prefix = get_option('prefix')
> >  datadir = prefix / get_option('datadir')
> >  sbindir = prefix / get_option('sbindir')
> > +set_variable('docdir', datadir / 'doc/libvirt-dbus')
> 
> No need to use set_variable(), you can use directly:
> 
> docdir = datadir / 'doc' / 'libvirt-dbus'
> 
> The set_variable() function is usually used in places where you need to
> dynamically generate the variable name.

I see! This is my first time touching a Meson-based build system,
so I mostly copy-pasted my way through O:-)

I'll adopt your version.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH RESEND v1 2/7] qemu: Implement the CFPC pSeries feature

[Michal Privoznik]

On 4/27/20 2:14 PM, Daniel Henrique Barboza wrote:

This patch adds the implementation of the CFPC pSeries feature,
using the QEMU_CAPS_MACHINE_PSERIES_CAP_CFPC capability added
in the previous patch.

CPFC can have the values "broken", "workaround" or "fixed". Extra
code is required to handle it since it's not a regular tristate
capability.

This is the XML format for the cap:


   


Signed-off-by: Daniel Henrique Barboza 
---
  docs/formatdomain.html.in | 11 +
  docs/schemas/domaincommon.rng | 15 +++
  src/conf/domain_conf.c| 44 +++
  src/conf/domain_conf.h| 12 +
  src/libvirt_private.syms  |  1 +
  src/qemu/qemu_command.c   |  5 +++
  src/qemu/qemu_validate.c  | 11 +
  tests/qemuxml2argvdata/pseries-features.args  |  3 +-
  tests/qemuxml2argvdata/pseries-features.xml   |  1 +
  tests/qemuxml2argvtest.c  | 17 ++-
  tests/qemuxml2xmloutdata/pseries-features.xml |  1 +
  tests/qemuxml2xmltest.c   |  3 +-
  12 files changed, 121 insertions(+), 3 deletions(-)





diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index ecb80ef8f2..8594049e52 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1816,6 +1816,7 @@ typedef enum {
  VIR_DOMAIN_FEATURE_MSRS,
  VIR_DOMAIN_FEATURE_CCF_ASSIST,
  VIR_DOMAIN_FEATURE_XEN,
+VIR_DOMAIN_FEATURE_CFPC,
  
  VIR_DOMAIN_FEATURE_LAST

  } virDomainFeature;
@@ -1987,6 +1988,17 @@ typedef enum {
  
  VIR_ENUM_DECL(virDomainHPTResizing);
  
+typedef enum {

+VIR_DOMAIN_CFPC_NONE = 0,
+VIR_DOMAIN_CFPC_BROKEN,
+VIR_DOMAIN_CFPC_WORKAROUND,
+VIR_DOMAIN_CFPC_FIXED,
+
+VIR_DOMAIN_CFPC_LAST
+} virDomainCFPC;
+
+VIR_ENUM_DECL(virDomainCFPC);
+


This declares both:

virDomainCFPCTypeToString()
virDomainCFPCTypeFromString()


  /* Operating system configuration data & machine / arch */
  struct _virDomainOSEnv {
  char *name;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a9694f34c0..308959b493 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -235,6 +235,7 @@ virDomainBlockIoTuneInfoHasMaxLength;
  virDomainBootTypeFromString;
  virDomainBootTypeToString;
  virDomainCapabilitiesPolicyTypeToString;
+virDomainCFPCTypeToString;


And even though we need only this, I'd like to expose TypeFromString() 
too for a possible future use. I mean, it's declared in the header file.



  virDomainChrConsoleTargetTypeFromString;
  virDomainChrConsoleTargetTypeToString;
  virDomainChrDefForeach;


Michal

Re: [PATCH RESEND v1 0/7] add Spectre related PowerPC features

[Michal Privoznik]

On 4/27/20 2:14 PM, Daniel Henrique Barboza wrote:

Recent changes in master (adding aio_io_uring cap, splitting
feature parsing code to a new function, updates in news.xml)
broke rebase for all patches in the series. Re-sending it, rebased
with master at 606fb3979ab4.



Hi,

This series implements 3 Spectre related PowerPC features that
were added back in QEMU 2.12:

- CFPC: Cache Flush on Privilege Change
- SBBC: Speculation Barrier Bounds Checking
- IBS:  Indirect Branch Speculation

These options aren't much of a problem for users using latest
hardware and guests with recent Linux kernels. Users with outdated
hardware/firmware or trying to run AIX guests/guests with older
kernels, however, will need to fine tune these options because
QEMU defaults won't work.

Instead of making users rely on  elements to
hardcode the options in the XML, let's support them in Libvirt.



Daniel Henrique Barboza (7):
   qemu: Add capability for CFPC pSeries feature
   qemu: Implement the CFPC pSeries feature
   qemu: Add capability for SBBC pSeries feature
   qemu: Implement the SBBC pSeries feature
   qemu: Add capability for IBS pSeries feature
   qemu: Implement the IBS pSeries feature
   news: Update for the recent added pSeries features

  docs/formatdomain.html.in |  36 +
  docs/news.xml |  10 ++
  docs/schemas/domaincommon.rng |  47 ++
  src/conf/domain_conf.c| 134 ++
  src/conf/domain_conf.h|  38 +
  src/libvirt_private.syms  |   3 +
  src/qemu/qemu_capabilities.c  |   8 ++
  src/qemu/qemu_capabilities.h  |   5 +
  src/qemu/qemu_command.c   |  15 ++
  src/qemu/qemu_validate.c  |  33 +
  .../caps_2.12.0.ppc64.xml |   3 +
  .../qemucapabilitiesdata/caps_3.0.0.ppc64.xml |   3 +
  .../qemucapabilitiesdata/caps_3.1.0.ppc64.xml |   3 +
  .../qemucapabilitiesdata/caps_4.0.0.ppc64.xml |   3 +
  .../qemucapabilitiesdata/caps_4.2.0.ppc64.xml |   3 +
  .../qemucapabilitiesdata/caps_5.0.0.ppc64.xml |   3 +
  tests/qemuxml2argvdata/pseries-features.args  |   3 +-
  tests/qemuxml2argvdata/pseries-features.xml   |   3 +
  tests/qemuxml2argvtest.c  |  53 ++-
  tests/qemuxml2xmloutdata/pseries-features.xml |   3 +
  tests/qemuxml2xmltest.c   |   5 +-
  21 files changed, 411 insertions(+), 3 deletions(-)



I've fixed all three TypeFromString()/TypeToString() places, and pushed.

Reviewed-by: Michal Privoznik 

Michal

Re: [libvirt-dbus PATCH 0/9] Convert documentation to reStructuredText, other cleanups

[Pavel Hrdina]

On Sat, Apr 25, 2020 at 10:59:37PM +0200, Andrea Bolognani wrote:
> I think it's high time that we have a new libvirt-dbus release, since
> the last one was more than a year ago: since then, there has been at
> least one bug fix necessary to keep it building on modern platforms,
> an additional API has been implemented, and of course we've switched
> the build system from autotools to Meson.
> 
> I validated the status of what's in master by importing a snapshot
> into the Debian package and everything seems to be in order, so I'm
> fairly confident we could basically just tag a release right away;
> there are, however, a few straightforward improvements that I would
> like to get into the new release, so here we are :)
> 
> Andrea Bolognani (9):
>   git: Minimize ignore patterns
>   spec: Install fewer documentation files
>   README: Convert to reStructuredText
>   HACKING: Convert to reStructuredText
>   NEWS: Convert to reStructuredText
>   AUTHORS: Convert to reStructuredText
>   man: Convert to reStructuredText
>   meson: Install documentation
>   spec: Pick documentation from installation directory

With the issues in last two patches fixed

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature

Re: [libvirt-dbus PATCH 9/9] spec: Pick documentation from installation directory

[Pavel Hrdina]

On Sat, Apr 25, 2020 at 10:59:46PM +0200, Andrea Bolognani wrote:
> Now that the documentation is installed just like other files,
> we can also pick it from the installation directory instead of
> the source directory.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  libvirt-dbus.spec.in | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/libvirt-dbus.spec.in b/libvirt-dbus.spec.in
> index 84172fa..2230ffc 100644
> --- a/libvirt-dbus.spec.in
> +++ b/libvirt-dbus.spec.in
> @@ -50,8 +50,9 @@ getent passwd %{system_user} >/dev/null || \
>  exit 0
>  
>  %files
> -%doc AUTHORS.rst NEWS.rst
> -%license COPYING
> +%doc %{_docdir}/%{name}/AUTHORS.rst
> +%doc %{_docdir}/%{name}/NEWS.rst
> +%license %{_docdir}/%{name}/COPYING
>  %{_sbindir}/libvirt-dbus
>  %{_datadir}/dbus-1/services/org.libvirt.service
>  %{_datadir}/dbus-1/system-services/org.libvirt.service

This patch will change the location of COPYING file from

/usr/share/licenses/libvirt-dbus/COPYING

to

/usr/share/doc/libvirt-dbus/COPYING

Which is probably OK but the preferred location is the licenses
directory.  I would like to keep this behavior for RPM packages
which means move the file in the %install phase or using this:

%exclude %{_docdir}/%{name}/COPYING
%license COPYING

Pavel


signature.asc
Description: PGP signature

Re: [libvirt-dbus PATCH 8/9] meson: Install documentation

[Pavel Hrdina]

On Sat, Apr 25, 2020 at 10:59:45PM +0200, Andrea Bolognani wrote:
> People who install the RPM packages already get this, but it's
> good practice so let's make it happen for those who install from
> source as well.
> 
> The process is straightforward, but we have to be a bit careful
> with AUTHORS because this specific file is found in different
> directories depending on whether we're building from git or from
> a release.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  meson.build | 18 ++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/meson.build b/meson.build
> index 7518e94..9395bc4 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -12,6 +12,7 @@ project(
>  prefix = get_option('prefix')
>  datadir = prefix / get_option('datadir')
>  sbindir = prefix / get_option('sbindir')
> +set_variable('docdir', datadir / 'doc/libvirt-dbus')

No need to use set_variable(), you can use directly:

docdir = datadir / 'doc' / 'libvirt-dbus'

The set_variable() function is usually used in places where you need to
dynamically generate the variable name.

Pavel


signature.asc
Description: PGP signature

[PATCH RESEND v1 4/7] qemu: Implement the SBBC pSeries feature

[Daniel Henrique Barboza]

This patch adds the implementation of the SBBC pSeries feature,
using the QEMU_CAPS_MACHINE_PSERIES_CAP_SBBC capability added
in the previous patch.

Like the previously added CFPC feature, SBBC can have the values
"broken", "workaround" or "fixed". Extra code is required to handle
it since it's not a regular tristate capability.

This is the XML format for the cap:


  


Signed-off-by: Daniel Henrique Barboza 
---
 docs/formatdomain.html.in | 11 +
 docs/schemas/domaincommon.rng | 15 +++
 src/conf/domain_conf.c| 44 +++
 src/conf/domain_conf.h| 12 +
 src/libvirt_private.syms  |  1 +
 src/qemu/qemu_command.c   |  5 +++
 src/qemu/qemu_validate.c  | 11 +
 tests/qemuxml2argvdata/pseries-features.args  |  2 +-
 tests/qemuxml2argvdata/pseries-features.xml   |  1 +
 tests/qemuxml2argvtest.c  | 21 -
 tests/qemuxml2xmloutdata/pseries-features.xml |  1 +
 tests/qemuxml2xmltest.c   |  3 +-
 12 files changed, 123 insertions(+), 4 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index a99407675f..dd8172fff2 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2079,6 +2079,7 @@
   ccf-assist state='on'/
   msrs unknown='ignore'/
   cfpc value='workaround'/
+  sbbc value='workaround'/
 /features
 ...
 
@@ -2417,6 +2418,16 @@
   default will be used.
   Since 6.3.0 (QEMU/KVM only)
   
+  sbbc
+  Configure sbbc (Speculation Barrier Bounds Checking) availability for
+  pSeries guests.
+  Possible values for the value attribute
+  are broken (no protection), workaround
+  (software workaround available) and fixed (fixed in
+  hardware). If the attribute is not defined, the hypervisor
+  default will be used.
+  Since 6.3.0 (QEMU/KVM only)
+  
 
 
 Time keeping
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 6441e01717..2a7a433c03 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -5437,6 +5437,9 @@
   
 
   
+  
+
+  
 
   
 
@@ -5708,6 +5711,18 @@
 
   
 
+  
+
+  
+
+  broken
+  workaround
+  fixed
+
+  
+
+  
+
   
 
   
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e53586cdf2..226a0012fc 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -175,6 +175,7 @@ VIR_ENUM_IMPL(virDomainFeature,
   "ccf-assist",
   "xen",
   "cfpc",
+  "sbbc",
 );
 
 VIR_ENUM_IMPL(virDomainCapabilitiesPolicy,
@@ -1275,6 +1276,14 @@ VIR_ENUM_IMPL(virDomainCFPC,
   "fixed",
 );
 
+VIR_ENUM_IMPL(virDomainSBBC,
+  VIR_DOMAIN_SBBC_LAST,
+  "none",
+  "broken",
+  "workaround",
+  "fixed",
+);
+
 /* Internal mapping: subset of block job types that can be present in
  *  XML (remaining types are not two-phase). */
 VIR_ENUM_DECL(virDomainBlockJob);
@@ -19350,6 +19359,21 @@ virDomainFeaturesDefParse(virDomainDefPtr def,
 }
 break;
 
+case VIR_DOMAIN_FEATURE_SBBC:
+tmp = virXMLPropString(nodes[i], "value");
+if (tmp) {
+int value = virDomainSBBCTypeFromString(tmp);
+if (value < 0 || value == VIR_DOMAIN_SBBC_NONE) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Unknown value: %s"),
+   tmp);
+goto error;
+}
+def->features[val] = value;
+VIR_FREE(tmp);
+}
+break;
+
 case VIR_DOMAIN_FEATURE_HTM:
 case VIR_DOMAIN_FEATURE_NESTED_HV:
 case VIR_DOMAIN_FEATURE_CCF_ASSIST:
@@ -23413,6 +23437,18 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr 
src,
 }
 break;
 
+case VIR_DOMAIN_FEATURE_SBBC:
+if (src->features[i] != dst->features[i]) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("State of feature '%s' differs: "
+ "source: '%s=%s', destination: '%s=%s'"),
+   featureName,
+   "value", 
virDomainSBBCTypeToString(src->features[i]),
+   "value", 
virDomainSBBCTypeToString(dst->features[i]));
+return false;
+}
+break;
+
 case VIR_DOMAIN_FEATURE_MSRS:
 break;
 
@@ -29255,6 +29291,14 @@ virDomainDefFormatFeatures(virBufferPtr buf,
   

[PATCH RESEND v1 6/7] qemu: Implement the IBS pSeries feature

[Daniel Henrique Barboza]

This patch adds the implementation of the IBS pSeries feature,
using the QEMU_CAPS_MACHINE_PSERIES_CAP_IBS capability added
in the previous patch.

IBS can have the following values: "broken", "workaround",
"fixed-ibs", "fixed-ccd" and "fixed-na".

This is the XML format for the cap:


  


Signed-off-by: Daniel Henrique Barboza 
---
 docs/formatdomain.html.in | 14 ++
 docs/schemas/domaincommon.rng | 17 +++
 src/conf/domain_conf.c| 46 +++
 src/conf/domain_conf.h| 14 ++
 src/libvirt_private.syms  |  1 +
 src/qemu/qemu_command.c   |  5 ++
 src/qemu/qemu_validate.c  | 11 +
 tests/qemuxml2argvdata/pseries-features.args  |  2 +-
 tests/qemuxml2argvdata/pseries-features.xml   |  1 +
 tests/qemuxml2argvtest.c  | 25 --
 tests/qemuxml2xmloutdata/pseries-features.xml |  1 +
 tests/qemuxml2xmltest.c   |  3 +-
 12 files changed, 135 insertions(+), 5 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index dd8172fff2..91d6f6c0d3 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2080,6 +2080,7 @@
   msrs unknown='ignore'/
   cfpc value='workaround'/
   sbbc value='workaround'/
+  ibs value='fixed-na'/
 /features
 ...
 
@@ -2428,6 +2429,19 @@
   default will be used.
   Since 6.3.0 (QEMU/KVM only)
   
+  ibs
+  Configure ibs (Indirect Branch Speculation) availability for
+  pSeries guests.
+  Possible values for the value attribute
+  are broken (no protection), workaround
+  (count cache flush), fixed-ibs (fixed by
+  serializing indirect branches), fixed-ccd (fixed by
+  disabling the cache count) and fixed-na (fixed in
+  hardware - no longer applicable).
+  If the attribute is not defined, the hypervisor
+  default will be used.
+  Since 6.3.0 (QEMU/KVM only)
+  
 
 
 Time keeping
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 2a7a433c03..9d60b090f3 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -5440,6 +5440,9 @@
   
 
   
+  
+
+  
 
   
 
@@ -5723,6 +5726,20 @@
 
   
 
+  
+
+  
+
+  broken
+  workaround
+  fixed-ibs
+  fixed-ccd
+  fixed-na
+
+  
+
+  
+
   
 
   
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 226a0012fc..8a87586936 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -176,6 +176,7 @@ VIR_ENUM_IMPL(virDomainFeature,
   "xen",
   "cfpc",
   "sbbc",
+  "ibs",
 );
 
 VIR_ENUM_IMPL(virDomainCapabilitiesPolicy,
@@ -1284,6 +1285,16 @@ VIR_ENUM_IMPL(virDomainSBBC,
   "fixed",
 );
 
+VIR_ENUM_IMPL(virDomainIBS,
+  VIR_DOMAIN_IBS_LAST,
+  "none",
+  "broken",
+  "workaround",
+  "fixed-ibs",
+  "fixed-ccd",
+  "fixed-na",
+);
+
 /* Internal mapping: subset of block job types that can be present in
  *  XML (remaining types are not two-phase). */
 VIR_ENUM_DECL(virDomainBlockJob);
@@ -19374,6 +19385,21 @@ virDomainFeaturesDefParse(virDomainDefPtr def,
 }
 break;
 
+case VIR_DOMAIN_FEATURE_IBS:
+tmp = virXMLPropString(nodes[i], "value");
+if (tmp) {
+int value = virDomainIBSTypeFromString(tmp);
+if (value < 0 || value == VIR_DOMAIN_IBS_NONE) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Unknown value: %s"),
+   tmp);
+goto error;
+}
+def->features[val] = value;
+VIR_FREE(tmp);
+}
+break;
+
 case VIR_DOMAIN_FEATURE_HTM:
 case VIR_DOMAIN_FEATURE_NESTED_HV:
 case VIR_DOMAIN_FEATURE_CCF_ASSIST:
@@ -23449,6 +23475,18 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr 
src,
 }
 break;
 
+case VIR_DOMAIN_FEATURE_IBS:
+if (src->features[i] != dst->features[i]) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("State of feature '%s' differs: "
+ "source: '%s=%s', destination: '%s=%s'"),
+   featureName,
+   "value", 
virDomainIBSTypeToString(src->features[i]),
+   "value", 
virDomainIBSTypeToString(dst->features[i]));
+return false;
+}
+break;
+
 case VIR_DOMAIN_FEATURE_MSRS:

[PATCH RESEND v1 1/7] qemu: Add capability for CFPC pSeries feature

[Daniel Henrique Barboza]

CFPC (Cache Flush on Privilege Change) is one of the capabilities
added to QEMU to mitigate Spectre vulnerabilities in Power chips.
It was implemented in QEMU 2.12 by commit 6898aed77f46.

This capability is still used today due to differences in how
the host setup (hardware and firmware/kernel) can handle this
mitigation. Its default value also varies with the pseries machine
version of the time. There's also certain OSes, like AIX, that
might not support the default value of the pseries machine the
guest uses.

Exposing this in the Libvirt XML as a feature will allow users to tune
CFPC values in a cleaner way, instead of hacking parameters in
 elements.

Signed-off-by: Daniel Henrique Barboza 
---
 src/qemu/qemu_capabilities.c | 2 ++
 src/qemu/qemu_capabilities.h | 1 +
 tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 +
 tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml  | 1 +
 8 files changed, 9 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index f6b3c96a3d..4838f3cfb5 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -574,6 +574,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
   "virtio.packed",
   "pcie-root-port.hotplug",
   "aio.io_uring",
+  "machine.pseries.cap-cfpc",
 );
 
 
@@ -1617,6 +1618,7 @@ static struct virQEMUCapsStringFlags 
virQEMUCapsMachinePropsPSeries[] = {
 { "cap-htm", QEMU_CAPS_MACHINE_PSERIES_CAP_HTM },
 { "cap-nested-hv", QEMU_CAPS_MACHINE_PSERIES_CAP_NESTED_HV },
 { "cap-ccf-assist", QEMU_CAPS_MACHINE_PSERIES_CAP_CCF_ASSIST },
+{ "cap-cfpc", QEMU_CAPS_MACHINE_PSERIES_CAP_CFPC },
 };
 
 static struct virQEMUCapsStringFlags virQEMUCapsMachinePropsVirt[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 0e9a161f94..88cf44ed59 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -555,6 +555,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
syntax-check */
 QEMU_CAPS_VIRTIO_PACKED_QUEUES, /* virtio.packed */
 QEMU_CAPS_PCIE_ROOT_PORT_HOTPLUG, /* pcie-root-port.hotplug */
 QEMU_CAPS_AIO_IO_URING, /* -blockdev {...,"aio":"io_uring",...} */
+QEMU_CAPS_MACHINE_PSERIES_CAP_CFPC, /* -machine pseries.cap-cfpc */
 
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml
index 38a3103c4a..cdd4f26993 100644
--- a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml
@@ -154,6 +154,7 @@
   
   
   
+  
   2011090
   0
   42900289
diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml
index 9a0b9c05c2..84e9ad2dcc 100644
--- a/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml
@@ -156,6 +156,7 @@
   
   
   
+  
   2012050
   0
   42900239
diff --git a/tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml
index 6801023208..3d70a67dab 100644
--- a/tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml
@@ -161,6 +161,7 @@
   
   
   
+  
   391
   0
   42900240
diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml
index f7e69fcc97..ce2d470cb2 100644
--- a/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml
@@ -175,6 +175,7 @@
   
   
   
+  
   400
   0
   42900240
diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml
index 99ec98e8cd..a813776660 100644
--- a/tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml
@@ -180,6 +180,7 @@
   
   
   
+  
   4001050
   0
   42900242
diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml
index b08916132a..c33786b0bf 100644
--- a/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml
@@ -192,6 +192,7 @@
   
   
   
+  
   4002050
   0
   42900241
-- 
2.25.4

[PATCH RESEND v1 2/7] qemu: Implement the CFPC pSeries feature

[Daniel Henrique Barboza]

This patch adds the implementation of the CFPC pSeries feature,
using the QEMU_CAPS_MACHINE_PSERIES_CAP_CFPC capability added
in the previous patch.

CPFC can have the values "broken", "workaround" or "fixed". Extra
code is required to handle it since it's not a regular tristate
capability.

This is the XML format for the cap:


  


Signed-off-by: Daniel Henrique Barboza 
---
 docs/formatdomain.html.in | 11 +
 docs/schemas/domaincommon.rng | 15 +++
 src/conf/domain_conf.c| 44 +++
 src/conf/domain_conf.h| 12 +
 src/libvirt_private.syms  |  1 +
 src/qemu/qemu_command.c   |  5 +++
 src/qemu/qemu_validate.c  | 11 +
 tests/qemuxml2argvdata/pseries-features.args  |  3 +-
 tests/qemuxml2argvdata/pseries-features.xml   |  1 +
 tests/qemuxml2argvtest.c  | 17 ++-
 tests/qemuxml2xmloutdata/pseries-features.xml |  1 +
 tests/qemuxml2xmltest.c   |  3 +-
 12 files changed, 121 insertions(+), 3 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index c5305739d2..a99407675f 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2078,6 +2078,7 @@
   htm state='on'/
   ccf-assist state='on'/
   msrs unknown='ignore'/
+  cfpc value='workaround'/
 /features
 ...
 
@@ -2406,6 +2407,16 @@
   defined, the hypervisor default will be used.
   Since 5.9.0 (QEMU/KVM only)
   
+  cfpc
+  Configure cfpc (Cache Flush on Privilege Change) availability for
+  pSeries guests.
+  Possible values for the value attribute
+  are broken (no protection), workaround
+  (software workaround available) and fixed (fixed in
+  hardware). If the attribute is not defined, the hypervisor
+  default will be used.
+  Since 6.3.0 (QEMU/KVM only)
+  
 
 
 Time keeping
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 7f18e5b669..6441e01717 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -5434,6 +5434,9 @@
   
 
   
+  
+
+  
 
   
 
@@ -5693,6 +5696,18 @@
 
   
 
+  
+
+  
+
+  broken
+  workaround
+  fixed
+
+  
+
+  
+
   
 
   
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 89cd8c5f32..e53586cdf2 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -174,6 +174,7 @@ VIR_ENUM_IMPL(virDomainFeature,
   "msrs",
   "ccf-assist",
   "xen",
+  "cfpc",
 );
 
 VIR_ENUM_IMPL(virDomainCapabilitiesPolicy,
@@ -1266,6 +1267,14 @@ VIR_ENUM_IMPL(virDomainOsDefFirmware,
   "efi",
 );
 
+VIR_ENUM_IMPL(virDomainCFPC,
+  VIR_DOMAIN_CFPC_LAST,
+  "none",
+  "broken",
+  "workaround",
+  "fixed",
+);
+
 /* Internal mapping: subset of block job types that can be present in
  *  XML (remaining types are not two-phase). */
 VIR_ENUM_DECL(virDomainBlockJob);
@@ -19326,6 +19335,21 @@ virDomainFeaturesDefParse(virDomainDefPtr def,
 }
 break;
 
+case VIR_DOMAIN_FEATURE_CFPC:
+tmp = virXMLPropString(nodes[i], "value");
+if (tmp) {
+int value = virDomainCFPCTypeFromString(tmp);
+if (value < 0 || value == VIR_DOMAIN_CFPC_NONE) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Unknown value: %s"),
+   tmp);
+goto error;
+}
+def->features[val] = value;
+VIR_FREE(tmp);
+}
+break;
+
 case VIR_DOMAIN_FEATURE_HTM:
 case VIR_DOMAIN_FEATURE_NESTED_HV:
 case VIR_DOMAIN_FEATURE_CCF_ASSIST:
@@ -23377,6 +23401,18 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr 
src,
 }
 break;
 
+case VIR_DOMAIN_FEATURE_CFPC:
+if (src->features[i] != dst->features[i]) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("State of feature '%s' differs: "
+ "source: '%s=%s', destination: '%s=%s'"),
+   featureName,
+   "value", 
virDomainCFPCTypeToString(src->features[i]),
+   "value", 
virDomainCFPCTypeToString(dst->features[i]));
+return false;
+}
+break;
+
 case VIR_DOMAIN_FEATURE_MSRS:
 break;
 
@@ -29211,6 +29247,14 @@ virDomainDefFormatFeatures(virBufferPtr buf,
   

[PATCH RESEND v1 7/7] news: Update for the recent added pSeries features

[Daniel Henrique Barboza]

Update news.xml to inform about the availability of CFPC, SBBC and
IBS features.

Signed-off-by: Daniel Henrique Barboza 
---
 docs/news.xml | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index 5835013c19..80819aec23 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -109,6 +109,16 @@
   and/or fine tuned per individual host.
 
   
+  
+
+  qemu: Implement pSeries Spectre mitigation features
+
+
+  Users can now setup the following capabilities of pSeries guests:
+  CFPC (Cache Flush on Privilege Change), SBBC (Speculation Barrier
+  Bounds Checking) and IBS (Indirect Branch Speculation).
+
+  
 
 
 
-- 
2.25.4

[PATCH RESEND v1 5/7] qemu: Add capability for IBS pSeries feature

[Daniel Henrique Barboza]

IBS (Indirect Branch Speculation) is the last capability added
in QEMU 2.12 related to Spectre mitigation for Power. It was
added in commit 4be8d4e7d935.

This patch introduces it as QEMU_CAPS_MACHINE_PSERIES_CAP_IBS.
Like CFPC and SBBC, users might want to tune in IBS based on
their HW and guest OS requirements, and it's better to do it
so in a proper Libvirt feature than to put QEMU arguments
in the middle of the domain XML.

Signed-off-by: Daniel Henrique Barboza 
---
 src/qemu/qemu_capabilities.c | 2 ++
 src/qemu/qemu_capabilities.h | 1 +
 tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 +
 tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml  | 1 +
 8 files changed, 9 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 2e2b9874a7..3e00299a91 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -578,6 +578,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
 
   /* 365 */
   "machine.pseries.cap-sbbc",
+  "machine.pseries.cap-ibs",
 );
 
 
@@ -1623,6 +1624,7 @@ static struct virQEMUCapsStringFlags 
virQEMUCapsMachinePropsPSeries[] = {
 { "cap-ccf-assist", QEMU_CAPS_MACHINE_PSERIES_CAP_CCF_ASSIST },
 { "cap-cfpc", QEMU_CAPS_MACHINE_PSERIES_CAP_CFPC },
 { "cap-sbbc", QEMU_CAPS_MACHINE_PSERIES_CAP_SBBC },
+{ "cap-ibs", QEMU_CAPS_MACHINE_PSERIES_CAP_IBS },
 };
 
 static struct virQEMUCapsStringFlags virQEMUCapsMachinePropsVirt[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 4040c50dc4..281aca5ea7 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -559,6 +559,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
syntax-check */
 
 /* 365 */
 QEMU_CAPS_MACHINE_PSERIES_CAP_SBBC, /* -machine pseries.cap-sbbc */
+QEMU_CAPS_MACHINE_PSERIES_CAP_IBS, /* -machine pseries.cap-ibs */
 
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml
index 2046f1097c..4c1758fbfe 100644
--- a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml
@@ -156,6 +156,7 @@
   
   
   
+  
   2011090
   0
   42900289
diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml
index 9e71080152..a8390a12eb 100644
--- a/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml
@@ -158,6 +158,7 @@
   
   
   
+  
   2012050
   0
   42900239
diff --git a/tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml
index f13b384e91..d96caaa9ed 100644
--- a/tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml
@@ -163,6 +163,7 @@
   
   
   
+  
   391
   0
   42900240
diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml
index 674e4b4944..44c1b9205e 100644
--- a/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml
@@ -177,6 +177,7 @@
   
   
   
+  
   400
   0
   42900240
diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml
index f89498171b..2eef337cc4 100644
--- a/tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml
@@ -182,6 +182,7 @@
   
   
   
+  
   4001050
   0
   42900242
diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml
index ebc39130df..d972def4b3 100644
--- a/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml
@@ -194,6 +194,7 @@
   
   
   
+  
   4002050
   0
   42900241
-- 
2.25.4

[PATCH RESEND v1 0/7] add Spectre related PowerPC features

[Daniel Henrique Barboza]

Recent changes in master (adding aio_io_uring cap, splitting
feature parsing code to a new function, updates in news.xml)
broke rebase for all patches in the series. Re-sending it, rebased
with master at 606fb3979ab4.



Hi,

This series implements 3 Spectre related PowerPC features that
were added back in QEMU 2.12:

- CFPC: Cache Flush on Privilege Change
- SBBC: Speculation Barrier Bounds Checking 
- IBS:  Indirect Branch Speculation

These options aren't much of a problem for users using latest
hardware and guests with recent Linux kernels. Users with outdated
hardware/firmware or trying to run AIX guests/guests with older
kernels, however, will need to fine tune these options because
QEMU defaults won't work.

Instead of making users rely on  elements to
hardcode the options in the XML, let's support them in Libvirt.



Daniel Henrique Barboza (7):
  qemu: Add capability for CFPC pSeries feature
  qemu: Implement the CFPC pSeries feature
  qemu: Add capability for SBBC pSeries feature
  qemu: Implement the SBBC pSeries feature
  qemu: Add capability for IBS pSeries feature
  qemu: Implement the IBS pSeries feature
  news: Update for the recent added pSeries features

 docs/formatdomain.html.in |  36 +
 docs/news.xml |  10 ++
 docs/schemas/domaincommon.rng |  47 ++
 src/conf/domain_conf.c| 134 ++
 src/conf/domain_conf.h|  38 +
 src/libvirt_private.syms  |   3 +
 src/qemu/qemu_capabilities.c  |   8 ++
 src/qemu/qemu_capabilities.h  |   5 +
 src/qemu/qemu_command.c   |  15 ++
 src/qemu/qemu_validate.c  |  33 +
 .../caps_2.12.0.ppc64.xml |   3 +
 .../qemucapabilitiesdata/caps_3.0.0.ppc64.xml |   3 +
 .../qemucapabilitiesdata/caps_3.1.0.ppc64.xml |   3 +
 .../qemucapabilitiesdata/caps_4.0.0.ppc64.xml |   3 +
 .../qemucapabilitiesdata/caps_4.2.0.ppc64.xml |   3 +
 .../qemucapabilitiesdata/caps_5.0.0.ppc64.xml |   3 +
 tests/qemuxml2argvdata/pseries-features.args  |   3 +-
 tests/qemuxml2argvdata/pseries-features.xml   |   3 +
 tests/qemuxml2argvtest.c  |  53 ++-
 tests/qemuxml2xmloutdata/pseries-features.xml |   3 +
 tests/qemuxml2xmltest.c   |   5 +-
 21 files changed, 411 insertions(+), 3 deletions(-)

-- 
2.25.4

[PATCH RESEND v1 3/7] qemu: Add capability for SBBC pSeries feature

[Daniel Henrique Barboza]

SBBC (Speculation Barrier Bounds Checking) is another capability
related to Spectre mitigation efforts in Power processors. It
was implemented in QEMU 2.12 by commit 09114fd81799.

This patch introduces it as QEMU_CAPS_MACHINE_PSERIES_CAP_SBBC to
be implemented in the next patch. Like the case with the now
implemented CFPC, exposing this feature in the XML allows for
a cleaner way for users to tune the SBBC accordingly, given
that not all hypervisor and guest setups supports this
Spectre mitigation.

Signed-off-by: Daniel Henrique Barboza 
---
 src/qemu/qemu_capabilities.c | 4 
 src/qemu/qemu_capabilities.h | 3 +++
 tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 +
 tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml  | 1 +
 8 files changed, 13 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 4838f3cfb5..2e2b9874a7 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -575,6 +575,9 @@ VIR_ENUM_IMPL(virQEMUCaps,
   "pcie-root-port.hotplug",
   "aio.io_uring",
   "machine.pseries.cap-cfpc",
+
+  /* 365 */
+  "machine.pseries.cap-sbbc",
 );
 
 
@@ -1619,6 +1622,7 @@ static struct virQEMUCapsStringFlags 
virQEMUCapsMachinePropsPSeries[] = {
 { "cap-nested-hv", QEMU_CAPS_MACHINE_PSERIES_CAP_NESTED_HV },
 { "cap-ccf-assist", QEMU_CAPS_MACHINE_PSERIES_CAP_CCF_ASSIST },
 { "cap-cfpc", QEMU_CAPS_MACHINE_PSERIES_CAP_CFPC },
+{ "cap-sbbc", QEMU_CAPS_MACHINE_PSERIES_CAP_SBBC },
 };
 
 static struct virQEMUCapsStringFlags virQEMUCapsMachinePropsVirt[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 88cf44ed59..4040c50dc4 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -557,6 +557,9 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
syntax-check */
 QEMU_CAPS_AIO_IO_URING, /* -blockdev {...,"aio":"io_uring",...} */
 QEMU_CAPS_MACHINE_PSERIES_CAP_CFPC, /* -machine pseries.cap-cfpc */
 
+/* 365 */
+QEMU_CAPS_MACHINE_PSERIES_CAP_SBBC, /* -machine pseries.cap-sbbc */
+
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
 
diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml
index cdd4f26993..2046f1097c 100644
--- a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml
@@ -155,6 +155,7 @@
   
   
   
+  
   2011090
   0
   42900289
diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml
index 84e9ad2dcc..9e71080152 100644
--- a/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml
@@ -157,6 +157,7 @@
   
   
   
+  
   2012050
   0
   42900239
diff --git a/tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml
index 3d70a67dab..f13b384e91 100644
--- a/tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml
@@ -162,6 +162,7 @@
   
   
   
+  
   391
   0
   42900240
diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml
index ce2d470cb2..674e4b4944 100644
--- a/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml
@@ -176,6 +176,7 @@
   
   
   
+  
   400
   0
   42900240
diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml
index a813776660..f89498171b 100644
--- a/tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml
@@ -181,6 +181,7 @@
   
   
   
+  
   4001050
   0
   42900242
diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml
index c33786b0bf..ebc39130df 100644
--- a/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml
@@ -193,6 +193,7 @@
   
   
   
+  
   4002050
   0
   42900241
-- 
2.25.4

Re: [libvirt PATCH 2/2] CONTRIBUTING: Include note about build system tools

[Michal Privoznik]

On 4/27/20 1:15 PM, Andrea Bolognani wrote:

On Mon, 2020-04-27 at 12:03 +0100, Daniel P. Berrangé wrote:

On Mon, Apr 27, 2020 at 12:54:21PM +0200, Andrea Bolognani wrote:

+Note that, for the RHEL-based case, if you're on a machine where you
+haven't done any C development before, you will probably also need
+to run
+
+::
+
+   $ sudo dnf groupinstall "C Development Tools and Libraries"


This group doesn't exist on any RHEL distro, and dnf itself only
exists in RHEL >= 8.  I htink we should be just listing the packages
we need explicitly rather than relying on a group whose name & contents
change over time.


I expect most people doing development work on libvirt are using
Fedora rather than RHEL,


I beg to differ :-)

But my distro will install all build dependencies when I want to install 
libvirt anyways, so good enough for me.


Michal

Re: [PATCH v1 0/7] add Spectre related PowerPC features

[Michal Privoznik]

On 4/23/20 12:07 PM, Daniel Henrique Barboza wrote:

Ping




Hey, patches look good, but they need rebasing (sorry about that). Can 
you please rebase and resend?


Michal

Re: [libvirt PATCH 2/2] CONTRIBUTING: Include note about build system tools

[Andrea Bolognani]

On Mon, 2020-04-27 at 12:19 +0100, Daniel P. Berrangé wrote:
> On Mon, Apr 27, 2020 at 01:15:08PM +0200, Andrea Bolognani wrote:
> > I expect most people doing development work on libvirt are using
> > Fedora rather than RHEL, and I really don't want this document to
> > become too verbose or duplicate too much information that's already
> > available elsewhere... If we can cover ~90% of potential users with
> > just a few commands, I think that's preferable.
> 
> Just listing the extra packages is simpler than the group name.
> 
>dnf install gcc make automake libtool autoconf rpm-build

Point taken, the list is short and unlikely to change much.

Can I have your Reviewed-by for the series if I squash your version
in?

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH 2/2] CONTRIBUTING: Include note about build system tools

[Daniel P . Berrangé]

On Mon, Apr 27, 2020 at 01:15:08PM +0200, Andrea Bolognani wrote:
> On Mon, 2020-04-27 at 12:03 +0100, Daniel P. Berrangé wrote:
> > On Mon, Apr 27, 2020 at 12:54:21PM +0200, Andrea Bolognani wrote:
> > > +Note that, for the RHEL-based case, if you're on a machine where you
> > > +haven't done any C development before, you will probably also need
> > > +to run
> > > +
> > > +::
> > > +
> > > +   $ sudo dnf groupinstall "C Development Tools and Libraries"
> > 
> > This group doesn't exist on any RHEL distro, and dnf itself only
> > exists in RHEL >= 8.  I htink we should be just listing the packages
> > we need explicitly rather than relying on a group whose name & contents
> > change over time.
> 
> I expect most people doing development work on libvirt are using
> Fedora rather than RHEL, and I really don't want this document to
> become too verbose or duplicate too much information that's already
> available elsewhere... If we can cover ~90% of potential users with
> just a few commands, I think that's preferable.

Just listing the extra packages is simpler than the group name.

   dnf install gcc make automake libtool autoconf rpm-build



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 2/2] CONTRIBUTING: Include note about build system tools

[Andrea Bolognani]

On Mon, 2020-04-27 at 12:03 +0100, Daniel P. Berrangé wrote:
> On Mon, Apr 27, 2020 at 12:54:21PM +0200, Andrea Bolognani wrote:
> > +Note that, for the RHEL-based case, if you're on a machine where you
> > +haven't done any C development before, you will probably also need
> > +to run
> > +
> > +::
> > +
> > +   $ sudo dnf groupinstall "C Development Tools and Libraries"
> 
> This group doesn't exist on any RHEL distro, and dnf itself only
> exists in RHEL >= 8.  I htink we should be just listing the packages
> we need explicitly rather than relying on a group whose name & contents
> change over time.

I expect most people doing development work on libvirt are using
Fedora rather than RHEL, and I really don't want this document to
become too verbose or duplicate too much information that's already
available elsewhere... If we can cover ~90% of potential users with
just a few commands, I think that's preferable.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH 2/2] CONTRIBUTING: Include note about build system tools

[Daniel P . Berrangé]

On Mon, Apr 27, 2020 at 12:54:21PM +0200, Andrea Bolognani wrote:
> Debian always runs autoreconf at package build time, which means
> that apt-get build-dep will bring in everything that's needed to
> build libvirt from a git clone; Fedora and RHEL, however, skip
> this step, so we have to install some extra packages manually.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  CONTRIBUTING.rst | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst
> index 12b6e3f0e8..32829b1d96 100644
> --- a/CONTRIBUTING.rst
> +++ b/CONTRIBUTING.rst
> @@ -33,6 +33,15 @@ if you're on a RHEL-based distribution or
>  
>  if you're on a Debian-based one.
>  
> +Note that, for the RHEL-based case, if you're on a machine where you
> +haven't done any C development before, you will probably also need
> +to run
> +
> +::
> +
> +   $ sudo dnf groupinstall "C Development Tools and Libraries"

This group doesn't exist on any RHEL distro, and dnf itself only
exists in RHEL >= 8.  I htink we should be just listing the packages
we need explicitly rather than relying on a group whose name & contents
change over time.

> +   $ sudo dnf install rpm-build
> +
>  You might still be missing some dependencies if your distribution is
>  shipping an old libvirt version, but that will get you much closer to
>  where you need to be to build successfully from source.

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 2/2] CONTRIBUTING: Include note about build system tools

[Andrea Bolognani]

Debian always runs autoreconf at package build time, which means
that apt-get build-dep will bring in everything that's needed to
build libvirt from a git clone; Fedora and RHEL, however, skip
this step, so we have to install some extra packages manually.

Signed-off-by: Andrea Bolognani 
---
 CONTRIBUTING.rst | 9 +
 1 file changed, 9 insertions(+)

diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst
index 12b6e3f0e8..32829b1d96 100644
--- a/CONTRIBUTING.rst
+++ b/CONTRIBUTING.rst
@@ -33,6 +33,15 @@ if you're on a RHEL-based distribution or
 
 if you're on a Debian-based one.
 
+Note that, for the RHEL-based case, if you're on a machine where you
+haven't done any C development before, you will probably also need
+to run
+
+::
+
+   $ sudo dnf groupinstall "C Development Tools and Libraries"
+   $ sudo dnf install rpm-build
+
 You might still be missing some dependencies if your distribution is
 shipping an old libvirt version, but that will get you much closer to
 where you need to be to build successfully from source.
-- 
2.25.4

[libvirt PATCH 0/2] CONTRIBUTING: Clean up and improve

[Andrea Bolognani]

Andrea Bolognani (2):
  CONTRIBUTING: Indent command by three spaces
  CONTRIBUTING: Include note about build system tools

 CONTRIBUTING.rst | 19 ++-
 1 file changed, 14 insertions(+), 5 deletions(-)

-- 
2.25.4

[libvirt PATCH 1/2] CONTRIBUTING: Indent command by three spaces

[Andrea Bolognani]

This is the proper way to do it according to our reStructuredText
style guidelines.

Signed-off-by: Andrea Bolognani 
---
 CONTRIBUTING.rst | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst
index f476700fdd..12b6e3f0e8 100644
--- a/CONTRIBUTING.rst
+++ b/CONTRIBUTING.rst
@@ -12,9 +12,9 @@ your git clone run:
 
 ::
 
-  $ mkdir build && cd build
-  $ ../autogen.sh
-  $ make
+   $ mkdir build && cd build
+   $ ../autogen.sh
+   $ make
 
 You'll find the freshly-built document in ``docs/contribute.html``.
 
@@ -23,13 +23,13 @@ up your system by calling
 
 ::
 
-  $ sudo dnf builddep libvirt
+   $ sudo dnf builddep libvirt
 
 if you're on a RHEL-based distribution or
 
 ::
 
-  $ sudo apt-get build-dep libvirt
+   $ sudo apt-get build-dep libvirt
 
 if you're on a Debian-based one.
 
-- 
2.25.4

Re: [PATCH] qemu: fix code format problem

[Andrea Bolognani]

On Sun, 2020-04-26 at 11:56 +, yubihong wrote:
> There are some code format problems in src/libvirt-domain.c. This patch fixes
> these problems.

This patch does not apply:

  Applying: qemu: fix code format problem
  error: git diff header lacks filename information when removing 1 leading 
pathname component (line 6)
  Patch failed at 0001 qemu: fix code format problem
  hint: Use 'git am --show-current-patch' to see the failed patch
  When you have resolved this problem, run "git am --continue".
  If you prefer to skip this patch, run "git am --skip" instead.
  To restore the original branch and stop patching, run "git am --abort".

I think the issue is caused by the comments that you added at the
very top. If you want to add some information at the time of posting
that doesn't need to be retained in the git history, it should go...

> > From 55cd85345b2dc50f44c1e382563482d40142382b Mon Sep 17 00:00:00
> 2001
> From: yubihong 
> Date: Fri, 24 Apr 2020 17:44:43 +0800
> Subject: [PATCH] qemu: fix code format problem
> 
> Signed-off-by:Yu Bihong 
> ---

... right here.

>  src/libvirt-domain.c | 16 
>  1 file changed, 8 insertions(+), 8 deletions(-)

Please resend the patch in a form that can be applied. See

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

for hints on how to achieve that.

While you're at it, please also fix your Signed-off-by so that it
looks like

  Signed-off-by: Yu Bihong 

(notice the additional whitespace) and configure your git client so
that authorship information (name and last name) match those in the
Signed-off-by.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH 3/4] qemu: move virtio capability validation

[Boris Fiuczynski]

Looks OK to me.
Reviewed-by: Boris Fiuczynski 

On 4/23/20 3:15 PM, Bjoern Walk wrote:

Move capability validation of virtio options from command line
generation to post-parse device validation where it belongs.

Signed-off-by: Bjoern Walk 
---
  src/qemu/qemu_command.c  | 41 ++--
  src/qemu/qemu_validate.c | 70 +++--
  tests/qemuxml2argvtest.c | 84 
  3 files changed, 120 insertions(+), 75 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 95402fc4..ca9d3f10 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -588,39 +588,20 @@ qemuBuildVirtioDevStr(virBufferPtr buf,
  
  static int

  qemuBuildVirtioOptionsStr(virBufferPtr buf,
-  virDomainVirtioOptionsPtr virtio,
-  virQEMUCapsPtr qemuCaps)
+  virDomainVirtioOptionsPtr virtio)
  {
  if (!virtio)
  return 0;
  
  if (virtio->iommu != VIR_TRISTATE_SWITCH_ABSENT) {

-if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_PCI_IOMMU_PLATFORM)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("the iommu setting is not supported "
- "with this QEMU binary"));
-return -1;
-}
  virBufferAsprintf(buf, ",iommu_platform=%s",
virTristateSwitchTypeToString(virtio->iommu));
  }
  if (virtio->ats != VIR_TRISTATE_SWITCH_ABSENT) {
-if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_PCI_ATS)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("the ats setting is not supported with this "
- "QEMU binary"));
-return -1;
-}
  virBufferAsprintf(buf, ",ats=%s",
virTristateSwitchTypeToString(virtio->ats));
  }
  if (virtio->packed != VIR_TRISTATE_SWITCH_ABSENT) {
-if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_PACKED_QUEUES)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("the packed setting is not supported with this "
- "QEMU binary"));
-return -1;
-}
  virBufferAsprintf(buf, ",packed=%s",
virTristateSwitchTypeToString(virtio->packed));
  }
@@ -2150,7 +2131,7 @@ qemuBuildDiskDeviceStr(const virDomainDef *def,
  virBufferAsprintf(, ",num-queues=%u", disk->queues);
  }
  
-if (qemuBuildVirtioOptionsStr(, disk->virtio, qemuCaps) < 0)

+if (qemuBuildVirtioOptionsStr(, disk->virtio) < 0)
  return NULL;
  
  if (qemuBuildDeviceAddressStr(, def, >info, qemuCaps) < 0)

@@ -2615,7 +2596,7 @@ qemuBuildVHostUserFsCommandLine(virCommandPtr cmd,
  virBufferAsprintf(, ",queue-size=%llu", fs->queue_size);
  virBufferAddLit(, ",tag=");
  virQEMUBuildBufferEscapeComma(, fs->dst);
-if (qemuBuildVirtioOptionsStr(, fs->virtio, priv->qemuCaps) < 0)
+if (qemuBuildVirtioOptionsStr(, fs->virtio) < 0)
  return -1;
  
  if (qemuBuildDeviceAddressStr(, def, >info, priv->qemuCaps) < 0)

@@ -2685,7 +2666,7 @@ qemuBuildFSDevStr(const virDomainDef *def,
  virBufferAddLit(, ",mount_tag=");
  virQEMUBuildBufferEscapeComma(, fs->dst);
  
-if (qemuBuildVirtioOptionsStr(, fs->virtio, qemuCaps) < 0)

+if (qemuBuildVirtioOptionsStr(, fs->virtio) < 0)
  return NULL;
  
  if (qemuBuildDeviceAddressStr(, def, >info, qemuCaps) < 0)

@@ -2917,7 +2898,7 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
def->iothread);
  }
  
-if (qemuBuildVirtioOptionsStr(, def->virtio, qemuCaps) < 0)

+if (qemuBuildVirtioOptionsStr(, def->virtio) < 0)
  return -1;
  break;
  case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC:
@@ -2964,7 +2945,7 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
  virBufferAsprintf(, ",vectors=%d",
def->opts.vioserial.vectors);
  }
-if (qemuBuildVirtioOptionsStr(, def->virtio, qemuCaps) < 0)
+if (qemuBuildVirtioOptionsStr(, def->virtio) < 0)
  return -1;
  break;
  
@@ -3916,7 +3897,7 @@ qemuBuildNicDevStr(virDomainDefPtr def,

  if (bootindex)
  virBufferAsprintf(, ",bootindex=%u", bootindex);
  if (usingVirtio &&
-qemuBuildVirtioOptionsStr(, net->virtio, qemuCaps) < 0)
+qemuBuildVirtioOptionsStr(, net->virtio) < 0)
  return NULL;
  
  return virBufferContentAndReset();

@@ -4158,7 +4139,7 @@ qemuBuildMemballoonCommandLine(virCommandPtr cmd,

virTristateSwitchTypeToString(def->memballoon->autodeflate));
  }
  
-if (qemuBuildVirtioOptionsStr(, def->memballoon->virtio, 

Re: [PATCH 2/4] tests: more fine-granular tests for virtio-options

[Michal Privoznik]

On 4/23/20 3:15 PM, Bjoern Walk wrote:

Add separate tests for individual options and devices for virtio-options
to have the ability to do more fine-granular testing of various
combinations.

Also, add negative tests for unavailable capabilities.

Reviewed-by: Boris Fiuczynski 
Signed-off-by: Bjoern Walk 
---
  .../virtio-options-controller-ats.args| 32 ++
  .../virtio-options-controller-ats.xml | 38 +++
  .../virtio-options-controller-iommu.args  | 34 +++
  .../virtio-options-controller-iommu.xml   | 38 +++
  .../virtio-options-controller-packed.args | 32 ++
  .../virtio-options-controller-packed.xml  | 38 +++
  .../virtio-options-disk-ats.args  | 36 +++
  .../virtio-options-disk-ats.xml   | 34 +++
  .../virtio-options-disk-iommu.args| 36 +++
  .../virtio-options-disk-iommu.xml | 34 +++
  .../virtio-options-disk-packed.args   | 36 +++
  .../virtio-options-disk-packed.xml| 34 +++
  .../virtio-options-fs-ats.args| 34 +++
  .../virtio-options-fs-ats.xml | 34 +++
  .../virtio-options-fs-iommu.args  | 34 +++
  .../virtio-options-fs-iommu.xml   | 34 +++
  .../virtio-options-fs-packed.args | 34 +++
  .../virtio-options-fs-packed.xml  | 34 +++
  .../virtio-options-input-ats.args | 30 ++
  .../virtio-options-input-ats.xml  | 30 ++
  .../virtio-options-input-iommu.args   | 30 ++
  .../virtio-options-input-iommu.xml| 30 ++
  .../virtio-options-input-packed.args  | 30 ++
  .../virtio-options-input-packed.xml   | 30 ++
  .../virtio-options-memballoon-ats.args| 28 ++
  .../virtio-options-memballoon-ats.xml | 23 +
  .../virtio-options-memballoon-iommu.args  | 28 ++
  .../virtio-options-memballoon-iommu.xml   | 23 +
  .../virtio-options-memballoon-packed.args | 28 ++
  .../virtio-options-memballoon-packed.xml  | 23 +
  .../virtio-options-net-ats.args   | 34 +++
  .../virtio-options-net-ats.xml| 34 +++
  .../virtio-options-net-iommu.args | 34 +++
  .../virtio-options-net-iommu.xml  | 34 +++
  .../virtio-options-net-packed.args| 34 +++
  .../virtio-options-net-packed.xml | 34 +++
  .../virtio-options-rng-ats.args   | 32 ++
  .../virtio-options-rng-ats.xml| 32 ++
  .../virtio-options-rng-iommu.args | 34 +++
  .../virtio-options-rng-iommu.xml  | 32 ++
  .../virtio-options-rng-packed.args| 32 ++
  .../virtio-options-rng-packed.xml | 32 ++
  .../virtio-options-video-ats.args | 34 +++
  .../virtio-options-video-ats.xml  | 36 +++
  .../virtio-options-video-iommu.args   | 34 +++
  .../virtio-options-video-iommu.xml| 36 +++
  .../virtio-options-video-packed.args  | 34 +++
  .../virtio-options-video-packed.xml   | 36 +++
  tests/qemuxml2argvtest.c  | 99 +++
  49 files changed, 1666 insertions(+)
  create mode 100644 tests/qemuxml2argvdata/virtio-options-controller-ats.args
  create mode 100644 tests/qemuxml2argvdata/virtio-options-controller-ats.xml
  create mode 100644 tests/qemuxml2argvdata/virtio-options-controller-iommu.args
  create mode 100644 tests/qemuxml2argvdata/virtio-options-controller-iommu.xml
  create mode 100644 
tests/qemuxml2argvdata/virtio-options-controller-packed.args
  create mode 100644 tests/qemuxml2argvdata/virtio-options-controller-packed.xml
  create mode 100644 tests/qemuxml2argvdata/virtio-options-disk-ats.args
  create mode 100644 tests/qemuxml2argvdata/virtio-options-disk-ats.xml
  create mode 100644 tests/qemuxml2argvdata/virtio-options-disk-iommu.args
  create mode 100644 tests/qemuxml2argvdata/virtio-options-disk-iommu.xml
  create mode 100644 tests/qemuxml2argvdata/virtio-options-disk-packed.args
  create mode 100644 tests/qemuxml2argvdata/virtio-options-disk-packed.xml
  create mode 100644 tests/qemuxml2argvdata/virtio-options-fs-ats.args
  create mode 100644 tests/qemuxml2argvdata/virtio-options-fs-ats.xml
  create mode 100644 tests/qemuxml2argvdata/virtio-options-fs-iommu.args
  create mode 100644 tests/qemuxml2argvdata/virtio-options-fs-iommu.xml
  create mode 100644 tests/qemuxml2argvdata/virtio-options-fs-packed.args
  create mode 100644 tests/qemuxml2argvdata/virtio-options-fs-packed.xml
  create mode 100644 tests/qemuxml2argvdata/virtio-options-input-ats.args
  create mode 100644 tests/qemuxml2argvdata/virtio-options-input-ats.xml
  create mode 100644 tests/qemuxml2argvdata/virtio-options-input-iommu.args
  create mode 100644 tests/qemuxml2argvdata/virtio-options-input-iommu.xml
  create 

Re: [PATCH 4/4] qemu: command: make qemuBuildVirtioOptionsStr void

[Boris Fiuczynski]

Thanks, Björn.
Reviewed-by: Boris Fiuczynski 


On 4/23/20 3:15 PM, Bjoern Walk wrote:

Now that qemuBuildVirtioOptionsStr can not fail anymore, remove its
return value and make it void.

Signed-off-by: Bjoern Walk 
---
  src/qemu/qemu_command.c | 38 +-
  1 file changed, 13 insertions(+), 25 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ca9d3f10..169a418f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -586,12 +586,12 @@ qemuBuildVirtioDevStr(virBufferPtr buf,
  return 0;
  }
  
-static int

+static void
  qemuBuildVirtioOptionsStr(virBufferPtr buf,
virDomainVirtioOptionsPtr virtio)
  {
  if (!virtio)
-return 0;
+return;
  
  if (virtio->iommu != VIR_TRISTATE_SWITCH_ABSENT) {

  virBufferAsprintf(buf, ",iommu_platform=%s",
@@ -605,8 +605,6 @@ qemuBuildVirtioOptionsStr(virBufferPtr buf,
  virBufferAsprintf(buf, ",packed=%s",
virTristateSwitchTypeToString(virtio->packed));
  }
-
-return 0;
  }
  
  static int

@@ -2131,8 +2129,7 @@ qemuBuildDiskDeviceStr(const virDomainDef *def,
  virBufferAsprintf(, ",num-queues=%u", disk->queues);
  }
  
-if (qemuBuildVirtioOptionsStr(, disk->virtio) < 0)

-return NULL;
+qemuBuildVirtioOptionsStr(, disk->virtio);
  
  if (qemuBuildDeviceAddressStr(, def, >info, qemuCaps) < 0)

  return NULL;
@@ -2596,8 +2593,7 @@ qemuBuildVHostUserFsCommandLine(virCommandPtr cmd,
  virBufferAsprintf(, ",queue-size=%llu", fs->queue_size);
  virBufferAddLit(, ",tag=");
  virQEMUBuildBufferEscapeComma(, fs->dst);
-if (qemuBuildVirtioOptionsStr(, fs->virtio) < 0)
-return -1;
+qemuBuildVirtioOptionsStr(, fs->virtio);
  
  if (qemuBuildDeviceAddressStr(, def, >info, priv->qemuCaps) < 0)

  return -1;
@@ -2666,8 +2662,7 @@ qemuBuildFSDevStr(const virDomainDef *def,
  virBufferAddLit(, ",mount_tag=");
  virQEMUBuildBufferEscapeComma(, fs->dst);
  
-if (qemuBuildVirtioOptionsStr(, fs->virtio) < 0)

-return NULL;
+qemuBuildVirtioOptionsStr(, fs->virtio);
  
  if (qemuBuildDeviceAddressStr(, def, >info, qemuCaps) < 0)

  return NULL;
@@ -2898,8 +2893,7 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
def->iothread);
  }
  
-if (qemuBuildVirtioOptionsStr(, def->virtio) < 0)

-return -1;
+qemuBuildVirtioOptionsStr(, def->virtio);
  break;
  case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC:
  virBufferAddLit(, "lsi");
@@ -2945,8 +2939,7 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
  virBufferAsprintf(, ",vectors=%d",
def->opts.vioserial.vectors);
  }
-if (qemuBuildVirtioOptionsStr(, def->virtio) < 0)
-return -1;
+qemuBuildVirtioOptionsStr(, def->virtio);
  break;
  
  case VIR_DOMAIN_CONTROLLER_TYPE_CCID:

@@ -3896,9 +3889,8 @@ qemuBuildNicDevStr(virDomainDefPtr def,
  return NULL;
  if (bootindex)
  virBufferAsprintf(, ",bootindex=%u", bootindex);
-if (usingVirtio &&
-qemuBuildVirtioOptionsStr(, net->virtio) < 0)
-return NULL;
+if (usingVirtio)
+qemuBuildVirtioOptionsStr(, net->virtio);
  
  return virBufferContentAndReset();

  }
@@ -4139,8 +4131,7 @@ qemuBuildMemballoonCommandLine(virCommandPtr cmd,

virTristateSwitchTypeToString(def->memballoon->autodeflate));
  }
  
-if (qemuBuildVirtioOptionsStr(, def->memballoon->virtio) < 0)

-return -1;
+qemuBuildVirtioOptionsStr(, def->memballoon->virtio);
  
  if (qemuCommandAddExtDevice(cmd, >memballoon->info) < 0)

  return -1;
@@ -4231,8 +4222,7 @@ qemuBuildVirtioInputDevStr(const virDomainDef *def,
  if (qemuBuildDeviceAddressStr(, def, >info, qemuCaps) < 0)
  return NULL;
  
-if (qemuBuildVirtioOptionsStr(, dev->virtio) < 0)

-return NULL;
+qemuBuildVirtioOptionsStr(, dev->virtio);
  
  return virBufferContentAndReset();

  }
@@ -4542,8 +4532,7 @@ qemuBuildDeviceVideoStr(const virDomainDef *def,
  if (qemuBuildDeviceAddressStr(, def, >info, qemuCaps) < 0)
  return NULL;
  
-if (qemuBuildVirtioOptionsStr(, video->virtio) < 0)

-return NULL;
+qemuBuildVirtioOptionsStr(, video->virtio);
  
  return virBufferContentAndReset();

  }
@@ -5758,8 +5747,7 @@ qemuBuildRNGDevStr(const virDomainDef *def,
  virBufferAddLit(, ",period=1000");
  }
  
-if (qemuBuildVirtioOptionsStr(, dev->virtio) < 0)

-return NULL;
+qemuBuildVirtioOptionsStr(, dev->virtio);
  
  if (qemuBuildDeviceAddressStr(, def, >info, qemuCaps) < 0)

  return NULL;




--
Mit freundlichen Grüßen/Kind 

Re: [PATCH 1/4] tests: use latest caps for virtio-options test

[Michal Privoznik]

On 4/23/20 3:15 PM, Bjoern Walk wrote:

Convert the virtio-options test in qemuxml2argv and qemuxml2xml to use
the latest available QEMU capabilities.

Reviewed-by: Boris Fiuczynski 
Signed-off-by: Bjoern Walk 
---
  .../virtio-options.x86_64-latest.args | 69 +++
  tests/qemuxml2argvdata/virtio-options.xml |  5 +-
  tests/qemuxml2argvtest.c  | 12 +---
  .../virtio-options.x86_64-latest.xml  |  1 +
  tests/qemuxml2xmltest.c   | 16 +
  5 files changed, 76 insertions(+), 27 deletions(-)
  create mode 100644 tests/qemuxml2argvdata/virtio-options.x86_64-latest.args
  create mode 12 tests/qemuxml2xmloutdata/virtio-options.x86_64-latest.xml


This means that tests/qemuxml2argvdata/virtio-options.args can be 
removed then (what git detects as rename). No need to resend, I will fix 
locally before pushing after we figure out 2/4.


Michal

Re: [PATCH 4/4] util: qcow2GetExtensions: Remove support for 'data file' extension

[Peter Krempa]

On Fri, Apr 24, 2020 at 10:25:15 -0500, Eric Blake wrote:
> On 4/24/20 10:18 AM, Peter Krempa wrote:
> > On Fri, Apr 24, 2020 at 09:52:00 -0500, Eric Blake wrote:
> > > On 4/24/20 4:24 AM, Peter Krempa wrote:
> > > > The implementation was never finished in libvirt. Remove it.
> > > > 
> > > > Signed-off-by: Peter Krempa 
> > > > ---
> > > >src/util/virstoragefile.c | 19 ++-
> > > >1 file changed, 2 insertions(+), 17 deletions(-)
> > > 
> > > Shouldn't we at least recognize if there is a qcow2 file with the 
> > > extension
> > > header set, at which point we can error out and tell the user their image 
> > > is
> > > unsupported, rather than trying to use it without the data file?
> > 
> > I don't think it's scalable to do so unless qemu exposes a whitelist of
> > supported extensions we can declare we support. Otherwise we are
> > band-aiding stuff we lear that exists and ignore any other extensions.
> 
> external data files have an incompatible feature bit set in addition to the
> header extension.  Maybe whitelisting header extensions is prohibitive, but
> we should be able to recognize when a qcow2 file has an incompatible feature
> bit set that we are not aware of, and assume that since we don't know about
> the feature, we also don't know how to safely pass that file to qemu.
> 
> That is, we could quickly reject qcow2 files with offset 72-79 containing
> bit 2 set, until we are ready to support external data files in libvirt.

Are there any specific reasons to do so? If so we should track them in a
bug/issue in the first place. Additionally I'm against extending the
qcow2 header parser beyond what's strictly necessary. In the end our
reimplementation of of the parser is considered a security measure.

I've pushed the patches for now. If somebody will require the 'data
file' field the last patch can always be reverted.

Re: [PATCH] qemu: fix code format problem

[Michal Privoznik]

On 4/26/20 1:56 PM, yubihong wrote:

There are some code format problems in src/libvirt-domain.c. This patch fixes
these problems.


From 55cd85345b2dc50f44c1e382563482d40142382b Mon Sep 17 00:00:00

2001
From: yubihong 
Date: Fri, 24 Apr 2020 17:44:43 +0800
Subject: [PATCH] qemu: fix code format problem

Signed-off-by:Yu Bihong 
---
  src/libvirt-domain.c | 16 
  1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index a12809c..d659f1b
100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -8194,11 +8194,11 @@ virDomainAttachDevice(virDomainPtr domain,
const char *xml)
  virCheckReadOnlyGoto(conn->flags, error);

  if (conn->driver->domainAttachDevice) {
-   int ret;
-   ret = conn->driver->domainAttachDevice(domain, xml);
-   if (ret < 0)
-  goto error;
-   return ret;
+int ret;
+ret = conn->driver->domainAttachDevice(domain, xml);
+if (ret < 0)
+goto error;
+return ret;
  }

  virReportUnsupportedError();
@@ -8299,9 +8299,9 @@ virDomainDetachDevice(virDomainPtr domain,
const char *xml)
  if (conn->driver->domainDetachDevice) {
  int ret;
  ret = conn->driver->domainDetachDevice(domain, xml);
- if (ret < 0)
- goto error;
- return ret;
+if (ret < 0)
+goto error;
+return ret;
   }

  virReportUnsupportedError();
--
1.8.3.1



This patch doesn't apply cleanly. Can you please resend using 
git-send-email as our documentation suggest?


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

The idea looks good, but I guess there are more places in the file that 
are misaligned. Might be worth extending your patch and include them.


Michal

Re: [libvirt-dbus PATCH 0/9] Convert documentation to reStructuredText, other cleanups

[Andrea Bolognani]

On Mon, 2020-04-27 at 10:17 +0200, Erik Skultety wrote:
> On Sat, Apr 25, 2020 at 10:59:37PM +0200, Andrea Bolognani wrote:
> > I think it's high time that we have a new libvirt-dbus release, since
> > the last one was more than a year ago: since then, there has been at
> > least one bug fix necessary to keep it building on modern platforms,
> > an additional API has been implemented, and of course we've switched
> > the build system from autotools to Meson.
> > 
> > I validated the status of what's in master by importing a snapshot
> > into the Debian package and everything seems to be in order, so I'm
> > fairly confident we could basically just tag a release right away;
> > there are, however, a few straightforward improvements that I would
> > like to get into the new release, so here we are :)
> 
> Apart from patch 8/9 which I can't review confidently, the rest looks good,
> provided you've tested this thoroughly, for the whole series:
> 
> Reviewed-by: Erik Skultety 

Thanks! Pavel promised he'd look at this series, and consider making
a new release, soon :)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH V3 5/5] cpu: Introduce getHost support for ARM CPU driver

[Zhenyu Zheng]

  ping for reviews

On Wed, Apr 22, 2020 at 3:12 PM ZhengZhenyu 
wrote:

> Introduce getHost support for ARM CPU driver, read
> CPU vendor_id, part_id and flags from registers
> directly. These codes will only be compiled on
> aarch64 hardwares.
>
> Signed-off-by: Zhenyu Zheng 
> ---
>  src/cpu/cpu_arm.c | 204 ++
>  1 file changed, 204 insertions(+)
>
> diff --git a/src/cpu/cpu_arm.c b/src/cpu/cpu_arm.c
> index 6e9ff9bf11..ec50a5615d 100644
> --- a/src/cpu/cpu_arm.c
> +++ b/src/cpu/cpu_arm.c
> @@ -21,6 +21,10 @@
>   */
>
>  #include 
> +#if defined(__aarch64__)
> +# include 
> +# include 
> +#endif
>
>  #include "viralloc.h"
>  #include "cpu.h"
> @@ -32,6 +36,15 @@
>
>  #define VIR_FROM_THIS VIR_FROM_CPU
>
> +#if defined(__aarch64__)
> +/* Shift bit mask for parsing cpu flags */
> +# define BIT_SHIFTS(n) (1UL << (n))
> +/* The current max number of cpu flags on ARM is 32 */
> +# define MAX_CPU_FLAGS 32
> +#endif
> +
> +VIR_LOG_INIT("cpu.cpu_arm");
> +
>  static const virArch archs[] = {
>  VIR_ARCH_ARMV6L,
>  VIR_ARCH_ARMV7B,
> @@ -491,12 +504,203 @@ virCPUarmValidateFeatures(virCPUDefPtr cpu)
>  return 0;
>  }
>
> +#if defined(__aarch64__)
> +/**
> + * virCPUarmCpuDataFromRegs:
> + *
> + * @data: 64-bit arm CPU specific data
> + *
> + * Fetches CPU vendor_id and part_id from MIDR_EL1 register, parse CPU
> + * flags from AT_HWCAP. There are currently 32 valid flags  on ARM arch
> + * represented by each bit.
> + */
> +static int
> +virCPUarmCpuDataFromRegs(virCPUarmData *data)
> +{
> +/* Generate human readable flag list according to the order of */
> +/* AT_HWCAP bit map */
> +const char *flag_list[MAX_CPU_FLAGS] = {
> +"fp", "asimd", "evtstrm", "aes", "pmull", "sha1", "sha2",
> +"crc32", "atomics", "fphp", "asimdhp", "cpuid", "asimdrdm",
> +"jscvt", "fcma", "lrcpc", "dcpop", "sha3", "sm3", "sm4",
> +"asimddp", "sha512", "sve", "asimdfhm", "dit", "uscat",
> +"ilrcpc", "flagm", "ssbs", "sb", "paca", "pacg"};
> +unsigned long cpuid, hwcaps;
> +char **features = NULL;
> +g_autofree char *cpu_feature_str = NULL;
> +int cpu_feature_index = 0;
> +size_t i;
> +
> +if (!(getauxval(AT_HWCAP) & HWCAP_CPUID)) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("CPUID registers unavailable"));
> +return -1;
> +}
> +
> +/* read the cpuid data from MIDR_EL1 register */
> +asm("mrs %0, MIDR_EL1" : "=r" (cpuid));
> +VIR_DEBUG("CPUID read from register:  0x%016lx", cpuid);
> +
> +/* parse the coresponding part_id bits */
> +data->pvr = cpuid>>4&0xFFF;
> +/* parse the coresponding vendor_id bits */
> +data->vendor_id = cpuid>>24&0xFF;
> +
> +hwcaps = getauxval(AT_HWCAP);
> +VIR_DEBUG("CPU flags read from register:  0x%016lx", hwcaps);
> +
> +if (VIR_ALLOC_N(features, MAX_CPU_FLAGS) < 0)
> +return -1;
> +
> +/* shift bit map mask to parse for CPU flags */
> +for (i = 0; i< MAX_CPU_FLAGS; i++) {
> +if (hwcaps & BIT_SHIFTS(i)) {
> +features[cpu_feature_index] = g_strdup(flag_list[i]);
> +cpu_feature_index++;
> +}
> +}
> +
> +if (cpu_feature_index > 1) {
> +cpu_feature_str = virStringListJoin((const char **)features, " ");
> +if (!cpu_feature_str)
> +goto error;
> +}
> +data->features = g_strdup(cpu_feature_str);
> +
> +return 0;
> +
> + error:
> +virStringListFree(features);
> +return -1;
> +}
> +
> +static int
> +virCPUarmDataParseFeatures(virCPUDefPtr cpu,
> +   const virCPUarmData *cpuData)
> +{
> +int ret = -1;
> +size_t i;
> +char **features;
> +
> +if (!cpu || !cpuData)
> +return ret;
> +
> +if (!(features = virStringSplitCount(cpuData->features, " ",
> + 0, >nfeatures)))
> +return ret;
> +if (cpu->nfeatures) {
> +if (VIR_ALLOC_N(cpu->features, cpu->nfeatures) < 0)
> +goto error;
> +
> +for (i = 0; i < cpu->nfeatures; i++) {
> +cpu->features[i].policy = VIR_CPU_FEATURE_REQUIRE;
> +cpu->features[i].name = g_strdup(features[i]);
> +}
> +}
> +
> +ret = 0;
> +
> + cleanup:
> +virStringListFree(features);
> +return ret;
> +
> + error:
> +for (i = 0; i < cpu->nfeatures; i++)
> +VIR_FREE(cpu->features[i].name);
> +VIR_FREE(cpu->features);
> +cpu->nfeatures = 0;
> +goto cleanup;
> +}
> +
> +static int
> +virCPUarmDecode(virCPUDefPtr cpu,
> +const virCPUarmData *cpuData,
> +virDomainCapsCPUModelsPtr models)
> +{
> +virCPUarmMapPtr map;
> +virCPUarmModelPtr model;
> +virCPUarmVendorPtr vendor = NULL;
> +
> +if (!cpuData || !(map = virCPUarmGetMap()))
> +return -1;
> +
> +if (!(model = virCPUarmModelFindByPVR(map, cpuData->pvr))) {
> +

Re: [PATCH V3 4/5] cpu: Add helper funtions to parse vendor and model

[Zhenyu Zheng]

  ping for reviews

On Wed, Apr 22, 2020 at 3:12 PM ZhengZhenyu 
wrote:

> Add helper functions to parse vendor and model from
> xml for ARM arch, and use them as callbacks when
> load cpu maps.
>
> Signed-off-by: Zhenyu Zheng 
> ---
>  src/cpu/cpu_arm.c | 173 +-
>  1 file changed, 170 insertions(+), 3 deletions(-)
>
> diff --git a/src/cpu/cpu_arm.c b/src/cpu/cpu_arm.c
> index 2009904cc9..6e9ff9bf11 100644
> --- a/src/cpu/cpu_arm.c
> +++ b/src/cpu/cpu_arm.c
> @@ -204,6 +204,174 @@ virCPUarmMapFeatureParse(xmlXPathContextPtr ctxt
> G_GNUC_UNUSED,
>  return 0;
>  }
>
> +static virCPUarmVendorPtr
> +virCPUarmVendorFindByID(virCPUarmMapPtr map,
> +unsigned long vendor_id)
> +{
> +size_t i;
> +
> +for (i = 0; i < map->nvendors; i++) {
> +if (map->vendors[i]->value == vendor_id)
> +return map->vendors[i];
> +}
> +
> +return NULL;
> +}
> +
> +
> +static virCPUarmVendorPtr
> +virCPUarmVendorFindByName(virCPUarmMapPtr map,
> +  const char *name)
> +{
> +size_t i;
> +
> +for (i = 0; i < map->nvendors; i++) {
> +if (STREQ(map->vendors[i]->name, name))
> +return map->vendors[i];
> +}
> +
> +return NULL;
> +}
> +
> +
> +static int
> +virCPUarmVendorParse(xmlXPathContextPtr ctxt,
> +   const char *name,
> +   void *data)
> +{
> +virCPUarmMapPtr map = data;
> +virCPUarmVendorPtr vendor = NULL;
> +int ret = -1;
> +
> +if (VIR_ALLOC(vendor) < 0)
> +return ret;
> +
> +vendor->name = g_strdup(name);
> +
> +if (virCPUarmVendorFindByName(map, vendor->name)) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("CPU vendor %s already defined"),
> +   vendor->name);
> +goto cleanup;
> +}
> +
> +if (virXPathULongHex("string(@value)", ctxt, >value) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   "%s", _("Missing CPU vendor value"));
> +goto cleanup;
> +}
> +
> +if (virCPUarmVendorFindByID(map, vendor->value)) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("CPU vendor value 0x%2lx already defined"),
> +   vendor->value);
> +goto cleanup;
> +}
> +
> +if (VIR_APPEND_ELEMENT(map->vendors, map->nvendors, vendor) < 0)
> +goto cleanup;
> +
> +ret = 0;
> +
> + cleanup:
> +virCPUarmVendorFree(vendor);
> +return ret;
> +
> +}
> +
> +static virCPUarmModelPtr
> +virCPUarmModelFind(virCPUarmMapPtr map,
> +   const char *name)
> +{
> +size_t i;
> +
> +for (i = 0; i < map->nmodels; i++) {
> +if (STREQ(map->models[i]->name, name))
> +return map->models[i];
> +}
> +
> +return NULL;
> +}
> +
> +#if defined(__aarch64__)
> +static virCPUarmModelPtr
> +virCPUarmModelFindByPVR(virCPUarmMapPtr map,
> +unsigned long pvr)
> +{
> +size_t i;
> +
> +for (i = 0; i < map->nmodels; i++) {
> +if (map->models[i]->data.pvr == pvr)
> +return map->models[i];
> +}
> +
> +return NULL;
> +}
> +#endif
> +
> +static int
> +virCPUarmModelParse(xmlXPathContextPtr ctxt,
> +  const char *name,
> +  void *data)
> +{
> +virCPUarmMapPtr map = data;
> +virCPUarmModel *model;
> +g_autofree xmlNodePtr *nodes = NULL;
> +g_autofree char *vendor = NULL;
> +
> +if (VIR_ALLOC(model) < 0)
> +goto error;
> +
> +model->name = g_strdup(name);
> +
> +if (virCPUarmModelFind(map, model->name)) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("CPU model %s already defined"),
> +   model->name);
> +goto error;
> +}
> +
> +if (virXPathBoolean("boolean(./vendor)", ctxt)) {
> +vendor = virXPathString("string(./vendor/@name)", ctxt);
> +if (!vendor) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Invalid vendor element in CPU model %s"),
> +   model->name);
> +goto error;
> +}
> +
> +if (!(model->vendor = virCPUarmVendorFindByName(map, vendor))) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Unknown vendor %s referenced by CPU model
> %s"),
> +   vendor, model->name);
> +goto error;
> +}
> +}
> +
> +if (!virXPathBoolean("boolean(./pvr)", ctxt)) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Missing PVR information for CPU model %s"),
> +   model->name);
> +goto error;
> +}
> +
> +if (virXPathULongHex("string(./pvr/@value)", ctxt, >data.pvr)
> < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Missing or invalid PVR value in CPU model %s"),
> + 

Re: [PATCH V3 0/5] Introduce getHost support for ARM CPU driver

[Zhenyu Zheng]

ping for reviews

On Wed, Apr 22, 2020 at 3:11 PM ZhengZhenyu 
wrote:

> Introduce getHost support for ARM CPU driver. First add
> some data about commonly used ARM CPU models, and their
> vendors into cpu_map, then added some helper methods as
> callbacks to load them. Read and parse vendor_id, part_id
> and CPU flags of local CPU from corresponding registers.
>
> Signed-off-by: Zhenyu Zheng 
>
> ZhengZhenyu (5):
>   cpu_map: Introduce ARM cpu models
>   cpu: Introduce virCPUarmData to virCPUData
>   cpu: Introduce ARM related structs
>   cpu: Add helper funtions to parse vendor and model
>   cpu: Introduce getHost support for ARM CPU driver
>
>  src/cpu/Makefile.inc.am   |   1 +
>  src/cpu/cpu.h |   2 +
>  src/cpu/cpu_arm.c | 450 +-
>  src/cpu/cpu_arm_data.h|  31 ++
>  src/cpu_map/Makefile.inc.am   |   7 +
>  src/cpu_map/arm_Falkor.xml|  16 ++
>  src/cpu_map/arm_Kunpeng-920.xml   |  24 ++
>  src/cpu_map/arm_ThunderX299xx.xml |  16 ++
>  src/cpu_map/arm_cortex-a53.xml|  16 ++
>  src/cpu_map/arm_cortex-a57.xml|  15 +
>  src/cpu_map/arm_cortex-a72.xml|  15 +
>  src/cpu_map/arm_vendors.xml   |  14 +
>  src/cpu_map/index.xml |  15 +
>  13 files changed, 619 insertions(+), 3 deletions(-)
>  create mode 100644 src/cpu/cpu_arm_data.h
>  create mode 100644 src/cpu_map/arm_Falkor.xml
>  create mode 100644 src/cpu_map/arm_Kunpeng-920.xml
>  create mode 100644 src/cpu_map/arm_ThunderX299xx.xml
>  create mode 100644 src/cpu_map/arm_cortex-a53.xml
>  create mode 100644 src/cpu_map/arm_cortex-a57.xml
>  create mode 100644 src/cpu_map/arm_cortex-a72.xml
>  create mode 100644 src/cpu_map/arm_vendors.xml
>
> --
> 2.26.0.windows.1
>
>

Re: [PATCH V3 1/5] cpu_map: Introduce ARM cpu models

[Zhenyu Zheng]

  ping for reviews

On Wed, Apr 22, 2020 at 3:12 PM ZhengZhenyu 
wrote:

> Introduce vendors and some commonly used models
> for ARM arch, these will be used for virConnectionGetCapabilities
> for ARM CPUs.
>
> Signed-off-by: Zhenyu Zheng 
> ---
>  src/cpu_map/Makefile.inc.am   |  7 +++
>  src/cpu_map/arm_Falkor.xml| 16 
>  src/cpu_map/arm_Kunpeng-920.xml   | 24 
>  src/cpu_map/arm_ThunderX299xx.xml | 16 
>  src/cpu_map/arm_cortex-a53.xml| 16 
>  src/cpu_map/arm_cortex-a57.xml| 15 +++
>  src/cpu_map/arm_cortex-a72.xml| 15 +++
>  src/cpu_map/arm_vendors.xml   | 14 ++
>  src/cpu_map/index.xml | 15 +++
>  9 files changed, 138 insertions(+)
>  create mode 100644 src/cpu_map/arm_Falkor.xml
>  create mode 100644 src/cpu_map/arm_Kunpeng-920.xml
>  create mode 100644 src/cpu_map/arm_ThunderX299xx.xml
>  create mode 100644 src/cpu_map/arm_cortex-a53.xml
>  create mode 100644 src/cpu_map/arm_cortex-a57.xml
>  create mode 100644 src/cpu_map/arm_cortex-a72.xml
>  create mode 100644 src/cpu_map/arm_vendors.xml
>
> diff --git a/src/cpu_map/Makefile.inc.am b/src/cpu_map/Makefile.inc.am
> index be64c9a0d4..93c2b19ddf 100644
> --- a/src/cpu_map/Makefile.inc.am
> +++ b/src/cpu_map/Makefile.inc.am
> @@ -2,7 +2,14 @@
>
>  cpumapdir = $(pkgdatadir)/cpu_map
>  cpumap_DATA = \
> +cpu_map/arm_cortex-a53.xml \
> +   cpu_map/arm_cortex-a57.xml \
> +   cpu_map/arm_cortex-a72.xml \
> cpu_map/arm_features.xml \
> +   cpu_map/arm_Kunpeng-920.xml \
> +   cpu_map/arm_ThunderX299xx.xml \
> +   cpu_map/arm_Falkor.xml \
> +   cpu_map/arm_vendors.xml \
> cpu_map/index.xml \
> cpu_map/ppc64_vendors.xml \
> cpu_map/ppc64_POWER7.xml \
> diff --git a/src/cpu_map/arm_Falkor.xml b/src/cpu_map/arm_Falkor.xml
> new file mode 100644
> index 00..902ed2b6ba
> --- /dev/null
> +++ b/src/cpu_map/arm_Falkor.xml
> @@ -0,0 +1,16 @@
> +
> +  
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +  
> +
> diff --git a/src/cpu_map/arm_Kunpeng-920.xml
> b/src/cpu_map/arm_Kunpeng-920.xml
> new file mode 100644
> index 00..668b8b50dc
> --- /dev/null
> +++ b/src/cpu_map/arm_Kunpeng-920.xml
> @@ -0,0 +1,24 @@
> +
> +  
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +  
> +
> diff --git a/src/cpu_map/arm_ThunderX299xx.xml
> b/src/cpu_map/arm_ThunderX299xx.xml
> new file mode 100644
> index 00..9ab3d939e9
> --- /dev/null
> +++ b/src/cpu_map/arm_ThunderX299xx.xml
> @@ -0,0 +1,16 @@
> +
> +  
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +  
> +
> diff --git a/src/cpu_map/arm_cortex-a53.xml
> b/src/cpu_map/arm_cortex-a53.xml
> new file mode 100644
> index 00..963d6d36e3
> --- /dev/null
> +++ b/src/cpu_map/arm_cortex-a53.xml
> @@ -0,0 +1,16 @@
> +
> +  
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +  
> +
> diff --git a/src/cpu_map/arm_cortex-a57.xml
> b/src/cpu_map/arm_cortex-a57.xml
> new file mode 100644
> index 00..92a044ea92
> --- /dev/null
> +++ b/src/cpu_map/arm_cortex-a57.xml
> @@ -0,0 +1,15 @@
> +
> +  
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +  
> +
> diff --git a/src/cpu_map/arm_cortex-a72.xml
> b/src/cpu_map/arm_cortex-a72.xml
> new file mode 100644
> index 00..9664eacd7b
> --- /dev/null
> +++ b/src/cpu_map/arm_cortex-a72.xml
> @@ -0,0 +1,15 @@
> +
> +  
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +  
> +
> diff --git a/src/cpu_map/arm_vendors.xml b/src/cpu_map/arm_vendors.xml
> new file mode 100644
> index 00..703c2112b1
> --- /dev/null
> +++ b/src/cpu_map/arm_vendors.xml
> @@ -0,0 +1,14 @@
> +
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +  
> +
> diff --git a/src/cpu_map/index.xml b/src/cpu_map/index.xml
> index 50b030de29..20646a031c 100644
> --- a/src/cpu_map/index.xml
> +++ b/src/cpu_map/index.xml
> @@ -85,6 +85,21 @@
>
>
>
> +
>  
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
>
>  
> --
> 2.26.0.windows.1
>
>

Re: [PATCH V3 3/5] cpu: Introduce ARM related structs

[Zhenyu Zheng]

  ping for reviews

On Wed, Apr 22, 2020 at 3:12 PM ZhengZhenyu 
wrote:

> Introduce vendor and model struct and related
> cleanup functions for ARM cpu.
>
> Signed-off-by: Zhenyu Zheng 
> ---
>  src/cpu/cpu_arm.c | 73 +++
>  1 file changed, 73 insertions(+)
>
> diff --git a/src/cpu/cpu_arm.c b/src/cpu/cpu_arm.c
> index ee5802198f..2009904cc9 100644
> --- a/src/cpu/cpu_arm.c
> +++ b/src/cpu/cpu_arm.c
> @@ -1,6 +1,7 @@
>  /*
>   * cpu_arm.c: CPU driver for arm CPUs
>   *
> + * Copyright (C) 2020 Huawei Technologies Co., Ltd.
>   * Copyright (C) 2013 Red Hat, Inc.
>   * Copyright (C) Canonical Ltd. 2012
>   *
> @@ -23,7 +24,9 @@
>
>  #include "viralloc.h"
>  #include "cpu.h"
> +#include "cpu_arm.h"
>  #include "cpu_map.h"
> +#include "virlog.h"
>  #include "virstring.h"
>  #include "virxml.h"
>
> @@ -36,6 +39,21 @@ static const virArch archs[] = {
>  VIR_ARCH_AARCH64,
>  };
>
> +typedef struct _virCPUarmVendor virCPUarmVendor;
> +typedef virCPUarmVendor *virCPUarmVendorPtr;
> +struct _virCPUarmVendor {
> +char *name;
> +unsigned long value;
> +};
> +
> +typedef struct _virCPUarmModel virCPUarmModel;
> +typedef virCPUarmModel *virCPUarmModelPtr;
> +struct _virCPUarmModel {
> +char *name;
> +virCPUarmVendorPtr vendor;
> +virCPUarmData data;
> +};
> +
>  typedef struct _virCPUarmFeature virCPUarmFeature;
>  typedef virCPUarmFeature *virCPUarmFeaturePtr;
>  struct _virCPUarmFeature {
> @@ -64,6 +82,10 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(virCPUarmFeature,
> virCPUarmFeatureFree);
>  typedef struct _virCPUarmMap virCPUarmMap;
>  typedef virCPUarmMap *virCPUarmMapPtr;
>  struct _virCPUarmMap {
> +size_t nvendors;
> +virCPUarmVendorPtr *vendors;
> +size_t nmodels;
> +virCPUarmModelPtr *models;
>  GPtrArray *features;
>  };
>
> @@ -81,12 +103,62 @@ virCPUarmMapNew(void)
>  return map;
>  }
>
> +static void
> +virCPUarmDataClear(virCPUarmData *data)
> +{
> +if (!data)
> +return;
> +
> +VIR_FREE(data->features);
> +}
> +
> +static void
> +virCPUarmDataFree(virCPUDataPtr cpuData)
> +{
> +if (!cpuData)
> +return;
> +
> +virCPUarmDataClear(>data.arm);
> +VIR_FREE(cpuData);
> +}
> +
> +static void
> +virCPUarmModelFree(virCPUarmModelPtr model)
> +{
> +if (!model)
> +return;
> +
> +virCPUarmDataClear(>data);
> +g_free(model->name);
> +g_free(model);
> +}
> +
> +static void
> +virCPUarmVendorFree(virCPUarmVendorPtr vendor)
> +{
> +if (!vendor)
> +return;
> +
> +g_free(vendor->name);
> +g_free(vendor);
> +}
> +
>  static void
>  virCPUarmMapFree(virCPUarmMapPtr map)
>  {
>  if (!map)
>  return;
>
> +size_t i;
> +
> +for (i = 0; i < map->nmodels; i++)
> +virCPUarmModelFree(map->models[i]);
> +g_free(map->models);
> +
> +for (i = 0; i < map->nvendors; i++)
> +virCPUarmVendorFree(map->vendors[i]);
> +g_free(map->vendors);
> +
>  g_ptr_array_free(map->features, TRUE);
>
>  g_free(map);
> @@ -259,6 +331,7 @@ struct cpuArchDriver cpuDriverArm = {
>  .compare = virCPUarmCompare,
>  .decode = NULL,
>  .encode = NULL,
> +.dataFree = virCPUarmDataFree,
>  .baseline = virCPUarmBaseline,
>  .update = virCPUarmUpdate,
>  .validateFeatures = virCPUarmValidateFeatures,
> --
> 2.26.0.windows.1
>
>

Re: [PATCH V3 2/5] cpu: Introduce virCPUarmData to virCPUData

[Zhenyu Zheng]

  ping for reviews

On Wed, Apr 22, 2020 at 3:12 PM ZhengZhenyu 
wrote:

> Introduce virCPUarmData to virCPUData
>
> Signed-off-by: Zhenyu Zheng 
> ---
>  src/cpu/Makefile.inc.am |  1 +
>  src/cpu/cpu.h   |  2 ++
>  src/cpu/cpu_arm_data.h  | 31 +++
>  3 files changed, 34 insertions(+)
>  create mode 100644 src/cpu/cpu_arm_data.h
>
> diff --git a/src/cpu/Makefile.inc.am b/src/cpu/Makefile.inc.am
> index 0abeee87b6..bea203fb5c 100644
> --- a/src/cpu/Makefile.inc.am
> +++ b/src/cpu/Makefile.inc.am
> @@ -10,6 +10,7 @@ CPU_SOURCES = \
> cpu/cpu_s390.c \
> cpu/cpu_arm.h \
> cpu/cpu_arm.c \
> +   cpu/cpu_arm_data.h \
> cpu/cpu_ppc64.h \
> cpu/cpu_ppc64.c \
> cpu/cpu_ppc64_data.h \
> diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h
> index f779d2be17..ec22a183a1 100644
> --- a/src/cpu/cpu.h
> +++ b/src/cpu/cpu.h
> @@ -27,6 +27,7 @@
>  #include "cpu_conf.h"
>  #include "cpu_x86_data.h"
>  #include "cpu_ppc64_data.h"
> +#include "cpu_arm_data.h"
>
>
>  typedef struct _virCPUData virCPUData;
> @@ -36,6 +37,7 @@ struct _virCPUData {
>  union {
>  virCPUx86Data x86;
>  virCPUppc64Data ppc64;
> +virCPUarmData arm;
>  /* generic driver needs no data */
>  } data;
>  };
> diff --git a/src/cpu/cpu_arm_data.h b/src/cpu/cpu_arm_data.h
> new file mode 100644
> index 00..cf12ca8c2e
> --- /dev/null
> +++ b/src/cpu/cpu_arm_data.h
> @@ -0,0 +1,31 @@
> +/*
> + * cpu_arm_data.h: 64-bit arm CPU specific data
> + *
> + * Copyright (C) 2020 Huawei Technologies Co., Ltd.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library;  If not, see
> + * .
> + *
> + */
> +
> +#pragma once
> +
> +#define VIR_CPU_ARM_DATA_INIT { 0 }
> +
> +typedef struct _virCPUarmData virCPUarmData;
> +struct _virCPUarmData {
> +unsigned long vendor_id;
> +unsigned long pvr;
> +char *features;
> +};
> --
> 2.26.0.windows.1
>
>
>

Re: [PATCH libvirt-python] examples: Fix connection error handling 2

[Michal Privoznik]

On 4/27/20 10:08 AM, Philipp Hahn wrote:

Fix two more cases in examples as
libvirt.open*() does not return None but raises an exception

Fixes: 283e2bc693746164b6e14d6fe3ccd38a07bf
Signed-off-by: Philipp Hahn 
---
  examples/dhcpleases.py | 5 +++--
  examples/domipaddrs.py | 5 +++--
  2 files changed, 6 insertions(+), 4 deletions(-)


Reviewed-by: Michal Privoznik 

and pushed.

Michal

[PATCH] qemu: fix code format problem

[yubihong]

There are some code format problems in src/libvirt-domain.c. This patch fixes
these problems.

>From 55cd85345b2dc50f44c1e382563482d40142382b Mon Sep 17 00:00:00
2001
From: yubihong 
Date: Fri, 24 Apr 2020 17:44:43 +0800
Subject: [PATCH] qemu: fix code format problem

Signed-off-by:Yu Bihong 
---
 src/libvirt-domain.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index a12809c..d659f1b
100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -8194,11 +8194,11 @@ virDomainAttachDevice(virDomainPtr domain,
const char *xml)
 virCheckReadOnlyGoto(conn->flags, error);

 if (conn->driver->domainAttachDevice) {
-   int ret;
-   ret = conn->driver->domainAttachDevice(domain, xml);
-   if (ret < 0)
-  goto error;
-   return ret;
+int ret;
+ret = conn->driver->domainAttachDevice(domain, xml);
+if (ret < 0)
+goto error;
+return ret;
 }

 virReportUnsupportedError();
@@ -8299,9 +8299,9 @@ virDomainDetachDevice(virDomainPtr domain,
const char *xml)
 if (conn->driver->domainDetachDevice) {
 int ret;
 ret = conn->driver->domainDetachDevice(domain, xml);
- if (ret < 0)
- goto error;
- return ret;
+if (ret < 0)
+goto error;
+return ret;
  }

 virReportUnsupportedError();
--
1.8.3.1

Re: [libvirt-dbus PATCH 0/9] Convert documentation to reStructuredText, other cleanups

[Erik Skultety]

On Sat, Apr 25, 2020 at 10:59:37PM +0200, Andrea Bolognani wrote:
> I think it's high time that we have a new libvirt-dbus release, since
> the last one was more than a year ago: since then, there has been at
> least one bug fix necessary to keep it building on modern platforms,
> an additional API has been implemented, and of course we've switched
> the build system from autotools to Meson.
> 
> I validated the status of what's in master by importing a snapshot
> into the Debian package and everything seems to be in order, so I'm
> fairly confident we could basically just tag a release right away;
> there are, however, a few straightforward improvements that I would
> like to get into the new release, so here we are :)

Apart from patch 8/9 which I can't review confidently, the rest looks good,
provided you've tested this thoroughly, for the whole series:

Reviewed-by: Erik Skultety 

Re: [libvirt PATCH v4 1/3] add QEMU_CAPS_TCG_DISABLED and probe conditionally

[Michal Privoznik]

On 4/24/20 5:52 PM, tobin wrote:

On 2020-04-24 11:31, Michal Privoznik wrote:

On 4/22/20 11:50 PM, Tobin Feldman-Fitzthum wrote:

Only probe QEMU binary with accel=tcg if TCG is not disabled.
Similarly, only add a VIR_DOMAIN_VIRT_QEMU guest if TCG
is available.

Signed-off-by: Tobin Feldman-Fitzthum 
---
  src/qemu/qemu_capabilities.c | 22 ++
  src/qemu/qemu_capabilities.h |  1 +
  2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index fe311048d4..c56b2d8f0e 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -573,6 +573,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
    "fsdev.multidevs",
    "virtio.packed",
    "pcie-root-port.hotplug",
+  "tcg-disabled",
  );
    @@ -1014,13 +1015,16 @@ virQEMUCapsInitGuestFromBinary(virCapsPtr 
caps,
  virCapabilitiesAddGuestFeatureWithToggle(guest, 
VIR_CAPS_GUEST_FEATURE_TYPE_DISKSNAPSHOT,

   true, false);
  -    if (virCapabilitiesAddGuestDomain(guest,
-  VIR_DOMAIN_VIRT_QEMU,
-  NULL,
-  NULL,
-  0,
-  NULL) == NULL)
-    goto cleanup;
+    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED)) {
+    if (virCapabilitiesAddGuestDomain(guest,
+  VIR_DOMAIN_VIRT_QEMU,
+  NULL,
+  NULL,
+  0,
+  NULL) == NULL) {
+    goto cleanup;
+    }
+    }
    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
  if (virCapabilitiesAddGuestDomain(guest,
@@ -2295,7 +2299,8 @@ bool
  virQEMUCapsIsVirtTypeSupported(virQEMUCapsPtr qemuCaps,
 virDomainVirtType virtType)
  {
-    if (virtType == VIR_DOMAIN_VIRT_QEMU)
+    if (virtType == VIR_DOMAIN_VIRT_QEMU &&
+    !virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED))
  return true;
    if (virtType == VIR_DOMAIN_VIRT_KVM &&
@@ -5150,6 +5155,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
   * off.
   */
  if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) &&
+    !virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED) &&
  virQEMUCapsInitQMPSingle(qemuCaps, libDir, runUid, runGid, 
true) < 0)

  return -1;
  diff --git a/src/qemu/qemu_capabilities.h 
b/src/qemu/qemu_capabilities.h

index 1681fc79a8..abc4ee82cb 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -554,6 +554,7 @@ typedef enum { /* virQEMUCapsFlags grouping 
marker for syntax-check */

  QEMU_CAPS_FSDEV_MULTIDEVS, /* fsdev.multidevs */
  QEMU_CAPS_VIRTIO_PACKED_QUEUES, /* virtio.packed */
  QEMU_CAPS_PCIE_ROOT_PORT_HOTPLUG, /* pcie-root-port.hotplug */
+    QEMU_CAPS_TCG_DISABLED, /* QEMU does not support TCG */


Actually, I think this should be a positive capability, e.g.
QEMU_CAPS_TCG to reflect whether QEMU supports TCG or not. I can do
the change locally before pushing, if you agree with the change.

Michal


I can see why a positive capability might be a little more intuitive,
but one thing to keep in mind is that if there is a capability for
the TCG being enabled, we will have to do a bit more work to make sure
it is always set correctly. Older versions of QEMU don't report that
the TCG is enabled, for instance, so we would need to make sure we
always set TCG_ENABLED for those versions. This applies not only when
probing for caps, but also when reading capabilities from XML. 


That is okay, if libvirtd's or qemu's ctime changes, or any version of 
the two then capabilities are invalidated and reprobed. This means that 
introducing a new capability causes libvirt to reprobe all caps on 
startup. So we don't have to be that careful about old caps XMLs (note, 
this is different to case when reading caps XMLs on a say daemon restart 
when nothing changed, we have to be careful there).


And as far as positive/negative boolean flag goes - in either cases we 
will have false positives. Say, a given QEMU binary doesn't support TCG 
but also the binary is old and doesn't allow TCG detection. I don't see 
any difference between caps reporting TCG flag set (erroneously), or 
TCG_DISABLED flag not set (again erroneously). Since we are not able to 
detect the flag for older versions, we have to guess - and that is what 
we are doing already in these cases - see virQEMUCapsInitQMPVersionCaps().



I think
these are solvable problems (although I suspect there might be some
interesting edge cases), but if we have a negative capability, we
don't have to worry about any of this. We set it in the few cases where
TCG is disabled and otherwise the TCG is always assumed 

[PATCH libvirt-python] examples: Fix connection error handling 2

[Philipp Hahn]

Fix two more cases in examples as
libvirt.open*() does not return None but raises an exception

Fixes: 283e2bc693746164b6e14d6fe3ccd38a07bf
Signed-off-by: Philipp Hahn 
---
 examples/dhcpleases.py | 5 +++--
 examples/domipaddrs.py | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/examples/dhcpleases.py b/examples/dhcpleases.py
index f394541..6f80fd5 100755
--- a/examples/dhcpleases.py
+++ b/examples/dhcpleases.py
@@ -22,8 +22,9 @@ else:
 usage()
 sys.exit(2)
 
-conn = libvirt.open(uri)
-if conn == None:
+try:
+conn = libvirt.open(uri)
+except libvirt.libvirtError:
 print("Unable to open connection to libvirt")
 sys.exit(1)
 
diff --git a/examples/domipaddrs.py b/examples/domipaddrs.py
index bda308c..e893995 100755
--- a/examples/domipaddrs.py
+++ b/examples/domipaddrs.py
@@ -21,8 +21,9 @@ else:
 usage()
 sys.exit(2)
 
-conn = libvirt.open(uri)
-if conn == None:
+try:
+conn = libvirt.open(uri)
+except libvirt.libvirtError:
 print "Unable to open connection to libvirt"
 sys.exit(1)
 
-- 
2.20.1

Re: [libvirt PATCH] CONTRIBUTING: Include information on build dependencies

[Andrea Bolognani]

On Thu, 2020-04-23 at 21:30 -0400, Laine Stump wrote:
> BTW, I just went through this on a new Fedora 31 machine (AMD Ryzen 
> 3950x! w00t!!) that I had installed with "Fedora Workstation", and 
> thought it might be useful to list exactly what was still missing for 
> certain "developer build" tasks after running "dnf builddep libvirt" on 
> a clean OS install. I found that I also had to install the following:
> 
> 
> 1) dnf install make
> 
> 
> This was required (duh!) to build from source tar, but not a part of the 
> base OS install nor in Build-Requires (I guess it makes sense - what 
> self-respecting Linux distro doesn't have basic build tools like make in 
> the base install?)

Plenty! Especially in the age of containers, where everyone is
scrambling to bring down the size of the base install. And it makes
sense, too: if you're going to run Apache or something like that on
the system, it's really not necessary to have make available.

> 2) dnf install autoconf automake libtool
> 
> 
> These three were required (but not in base OS + "dnf builddep libvirt" 
> to do a successful "./autogen.sh --system && make check" (i.e. build 
> from a fresh git clone of the tree).
> 
> 
> Again, I can see why autoconf, automake, and libtool wouldn't be in the 
> specfile Build-Requires:, since they *aren't* required when building 
> from a source tar, which already includes the configure script and 
> Makefile.in's that are generated using autotools.

That is not entirely true: if you look at our spec file, you'll find

  # Default to skipping autoreconf.  Distros can change just this one line
  # (or provide a command-line override) if they backport any patches that
  # touch configure.ac or Makefile.am.
  %{!?enable_autotools:%global enable_autotools 0}

  %if 0%{?enable_autotools}
  BuildRequires: autoconf
  BuildRequires: automake
  BuildRequires: gettext-devel
  BuildRequires: libtool
  %endif

  %if 0%{?enable_autotools}
   autoreconf -if
  %endif

so we clearly anticipate the possibility of running autoreconf at
build time. Debian, for example, while obviously not using our spec
file, mandates this behavior for all its packages...

I think some packages are left out of build dependencies because they
are used by so many that you'd see them repeated all over the place.
Debian has a special package defined for this very purpose:

  https://packages.debian.org/sid/build-essential

As you can see, it includes gcc and make plus dpkg-dev (roughly
equivalent to rpmbuild, although to build most packages currently in
the archive you'll also want debhelper at the very least in addition
to that), but not autotools.

For Fedora, the closest I've found is

  sudo dnf groupinfo "C Development Tools and Libraries"
  Group: C Development Tools and Libraries
   Description: These tools include core development tools such as automake, 
gcc and debuggers.
   Mandatory Packages:
 autoconf
 automake
 binutils
 bison
 flex
 gcc
 gcc-c++
 gdb
 glibc-devel
 libtool
 make
 pkgconf
 strace
   Default Packages:
 ...
   Optional Packages:
 ...

which includes autotools but not rpmbuild, and has a very
inconvenient name :)

> 4) dnf install cppi dwarves python3-flake8
> 
> 
> These three were *not* required to successfully complete any build 
> operation, but parts of "make syntax-check" were skipped because they 
> weren't present (and so not having them might result in a developer 
> believing that their patches had passed "make syntax-check, when in fact 
> they had not).
> 
> 
> At least cppi and python3-flake8 can't be Build-Required: in the 
> specfile for building because they aren't available on RHEL8/CentOS8 (at 
> least not without EPEL - haven't checked there yet, but of course EPEL 
> packages aren't available in the official build environment, so it's 
> kind of irrelevant)(oh, and also make syntax-check isn't run as part of 
> building the rpms, so those programs are never run during an official 
> build anyway, i.e. adding them to the Build-Requires: of the specfile 
> would be a lie :-)

Yeah, we could easily list them as requirements on Fedora only, but
since we don't actually run syntax-check at package build time it
doesn't feel right.

> Anyway, does anyone think it's worth adding a short bit to this file 
> about these extra packages? Or should we keep this file simple and 
> rather let newcomers (and old timers who've forgotten and are setting up 
> a new machine) figure it out for themselves?

I'll post a follow-up that adds instructions for installing autotools
as a prerequisite. That will surely age well, now that the switch to
Meson is looming O:-)

I think I'll leave out the set of packages in 4), though. I don't
want the instructions to become too long, and I think even without
those packages we'll still catch most issues.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH] network: Remove memory leak caused by wrong initialization

[Michal Privoznik]

On 4/25/20 6:35 PM, Julio Faracco wrote:

This commit fix a wrong variable initialization. There is a variable
called `new_lease` which is being initialized with the content of
parameter `lease`. To avoid memory leak, the proper way is initialize
with NULL first. This wrong statement was added by commit 97a0aa24.
There are some other improvements also.

Signed-off-by: Julio Faracco 
---
  src/conf/network_conf.c | 38 ++---
  src/network/bridge_driver.c |  8 ++--
  2 files changed, 21 insertions(+), 25 deletions(-)


Reviewed-by: Michal Privoznik 

and pushed.

Michal