Re: [libvirt] [PATCH v3 03/12] conf: Introduce a new PCI address extension flag

[Bjoern Walk]

Andrea Bolognani  [2018-08-16, 04:44PM +0200]:
> On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
> [...]
> > +qemuDomainDeviceSupportZPCI(virDomainDeviceDefPtr device)
> > +{
> > +switch ((virDomainDeviceType) device->type) {
> > +case VIR_DOMAIN_DEVICE_CHR:
> > +return false;
> > +
> > +case VIR_DOMAIN_DEVICE_CONTROLLER:
> > +case VIR_DOMAIN_DEVICE_DISK:
> > +case VIR_DOMAIN_DEVICE_LEASE:
> > +case VIR_DOMAIN_DEVICE_FS:
> > +case VIR_DOMAIN_DEVICE_NET:
> > +case VIR_DOMAIN_DEVICE_INPUT:
> > +case VIR_DOMAIN_DEVICE_SOUND:
> > +case VIR_DOMAIN_DEVICE_VIDEO:
> > +case VIR_DOMAIN_DEVICE_HOSTDEV:
> > +case VIR_DOMAIN_DEVICE_WATCHDOG:
> > +case VIR_DOMAIN_DEVICE_GRAPHICS:
> > +case VIR_DOMAIN_DEVICE_HUB:
> > +case VIR_DOMAIN_DEVICE_REDIRDEV:
> > +case VIR_DOMAIN_DEVICE_SMARTCARD:
> > +case VIR_DOMAIN_DEVICE_MEMBALLOON:
> > +case VIR_DOMAIN_DEVICE_NVRAM:
> > +case VIR_DOMAIN_DEVICE_RNG:
> > +case VIR_DOMAIN_DEVICE_SHMEM:
> > +case VIR_DOMAIN_DEVICE_TPM:
> > +case VIR_DOMAIN_DEVICE_PANIC:
> > +case VIR_DOMAIN_DEVICE_MEMORY:
> > +case VIR_DOMAIN_DEVICE_IOMMU:
> > +case VIR_DOMAIN_DEVICE_VSOCK:
> > +break;
> > +
> > +case VIR_DOMAIN_DEVICE_NONE:
> > +case VIR_DOMAIN_DEVICE_LAST:
> 
> Missing 'default' case.

I thought we explicitly don't want a default case so that the compiler
can catch this is another enum entry is added?


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 4/7] lockd_driver_lockd: Implement metadata flag

[John Ferlan]

On 08/14/2018 07:19 AM, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik 
> ---
>  src/locking/lock_daemon_dispatch.c | 12 ++--
>  src/locking/lock_driver_lockd.c| 31 +--
>  src/locking/lock_driver_lockd.h|  1 +
>  3 files changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/src/locking/lock_daemon_dispatch.c 
> b/src/locking/lock_daemon_dispatch.c
> index 10248ec0b5..c24571ceac 100644
> --- a/src/locking/lock_daemon_dispatch.c
> +++ b/src/locking/lock_daemon_dispatch.c
> @@ -37,6 +37,9 @@ VIR_LOG_INIT("locking.lock_daemon_dispatch");
>  
>  #include "lock_daemon_dispatch_stubs.h"
>  
> +#define DEFAULT_OFFSET 0
> +#define METADATA_OFFSET 1
> +

Curious, does this lead to the prospect of being able to acquire/use
offset==0 length==1 and offset==1 length==1 at least conceptually and
granularity wise?

Related or not, there's a strange NFSv3 collision between QEMU and NFS
related to some sort of fcntl locking granularity. More details that you
possibly want to read at:

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

I only note it because well it was on my 'short term history' radar and
suffice to say it's an ugly and not fun issue dealing with these locks.

>  static int
>  virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server 
> ATTRIBUTE_UNUSED,
>  virNetServerClientPtr client,
> @@ -50,13 +53,14 @@ 
> virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server 
> ATTRIBUTE_UNU
>  virNetServerClientGetPrivateData(client);
>  virLockSpacePtr lockspace;
>  unsigned int newFlags;
> -off_t start = 0;
> +off_t start = DEFAULT_OFFSET;
>  off_t len = 1;
>  
>  virMutexLock(>lock);
>  
>  virCheckFlagsGoto(VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED |
> -  VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE, 
> cleanup);
> +  VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE |
> +  VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_METADATA, cleanup);
>  
>  if (priv->restricted) {
>  virReportError(VIR_ERR_OPERATION_DENIED, "%s",
> @@ -82,6 +86,10 @@ 
> virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server 
> ATTRIBUTE_UNU
>  newFlags |= VIR_LOCK_SPACE_ACQUIRE_SHARED;
>  if (flags & VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE)
>  newFlags |= VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE;
> +if (flags & VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_METADATA) {
> +start = METADATA_OFFSET;
> +newFlags |= VIR_LOCK_SPACE_ACQUIRE_WAIT;
> +}

If this is documented in more detail, then it should be noted that a
METADATA lock forces usage of the wait API's. I can only imagine
someone, some day will ask for a wait w/ timeout to avoid waiting too long.

>  
>  if (virLockSpaceAcquireResource(lockspace,
>  args->name,
> diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
> index 957a963a7b..bd14ed8930 100644
> --- a/src/locking/lock_driver_lockd.c
> +++ b/src/locking/lock_driver_lockd.c
> @@ -475,9 +475,11 @@ static int 
> virLockManagerLockDaemonAddResource(virLockManagerPtr lock,
>  bool autoCreate = false;
>  
>  virCheckFlags(VIR_LOCK_MANAGER_RESOURCE_READONLY |
> -  VIR_LOCK_MANAGER_RESOURCE_SHARED, -1);
> +  VIR_LOCK_MANAGER_RESOURCE_SHARED |
> +  VIR_LOCK_MANAGER_RESOURCE_METADATA, -1);
>  
> -if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY)
> +if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY &&
> +!(flags & VIR_LOCK_MANAGER_RESOURCE_METADATA))
>  return 0;

Could someone pass READONLY & METADATA and not return 0 here?  Yes,
doesn't make sense, but combining them leads to the logic matrix question.

>  
>  switch (type) {
> @@ -489,7 +491,8 @@ static int 
> virLockManagerLockDaemonAddResource(virLockManagerPtr lock,
>  }
>  if (!driver->autoDiskLease) {
>  if (!(flags & (VIR_LOCK_MANAGER_RESOURCE_SHARED |
> -   VIR_LOCK_MANAGER_RESOURCE_READONLY)))
> +   VIR_LOCK_MANAGER_RESOURCE_READONLY |
> +   VIR_LOCK_MANAGER_RESOURCE_METADATA)))
>  priv->hasRWDisks = true;
>  return 0;
>  }
> @@ -602,6 +605,10 @@ static int 
> virLockManagerLockDaemonAddResource(virLockManagerPtr lock,
>  priv->resources[priv->nresources-1].flags |=
>  VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE;
>  
> +if (flags & VIR_LOCK_MANAGER_RESOURCE_METADATA)
> +priv->resources[priv->nresources-1].flags |=
> +VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_METADATA;
> +
>  return 0;
>  
>   error:
> @@ -626,12 +633,15 @@ static int 
> virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
>  

Re: [libvirt] [PATCH v2 3/7] lock_driver.h: Introduce metadata flag

[John Ferlan]

On 08/14/2018 07:19 AM, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik 
> ---
>  src/locking/lock_driver.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h
> index 8b7521..7c8f79520a 100644
> --- a/src/locking/lock_driver.h
> +++ b/src/locking/lock_driver.h
> @@ -56,6 +56,8 @@ typedef enum {
>  VIR_LOCK_MANAGER_RESOURCE_READONLY = (1 << 0),
>  /* The resource is assigned in shared, writable mode */
>  VIR_LOCK_MANAGER_RESOURCE_SHARED   = (1 << 1),
> +/* The resource is locked for metadata change */
> +VIR_LOCK_MANAGER_RESOURCE_METADATA = (1 << 2),

Does this work or make sense for lease type?

Of course this adds something that would "conceivably" be available such
that we may want to document it on
https://libvirt.org/internals/locking.html, but then again that's so
sparse it would take a bit of spelunking in the code to figure it out.

Anyway, since the name itself seems reasonable to me,

Reviewed-by: John Ferlan 

if you add a few more details around it related to disk vs. lease that'd
be nice. If you update the docs eventually with all that you've learned,
then that'd be great too!

John


>  } virLockManagerResourceFlags;
>  
>  typedef enum {
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] clean/simple Q35 support in libvirt+QEMU for guest OSes that don't support virtio-1.0

[Laine Stump]

(Several of us started an offline discussion on this topic, and it
quickly became complicated, so we decided it should continue upstream.
Here is a synopsis of the discussion so far (as *I've* interpreted it,
so corrections are welcome and apologies in advance for anything I got
wrong!) Some of the things are stated here as givens, but feel free to
rip them apart.)

Summary of the problem:

1) We want to persuade libvirt+QEMU users to move away from the i440fx
machinetype in favor of Q35. (NB: Someday this *might* lead to the
ability to deprecate and even remove the 440fx machinetype, but even if
that were to happen, it would be a *very long* time from now, so this
discussion is *not* about that!)

2) When Q35 machinetype is used, libvirt assigns virtio devices to a
slot on a PCI Express controller (because why have modern PCIe
controllers/slots available but force everything onto clunky old legacy
controllers?).

3) When a virtio device is plugged into an Express controller, QEMU
disables the device's IO port space, and it is put into "modern-only"
mode (this is done to avoid a rapid exhaustion of limited IO port space).

4) modern-only virtio devices won't work with a legacy (virtio-0.9-only)
guest driver, because virtio-0.9 requires IO port space.

5) Some guest OSes that we still want to support (and which would
otherwise work okay on a Q35 virtual machine) have virtio drivers too
old to support virtio-1.0 (CentOS6 and RHEL6 are examples of such OSes),
but due to the chain of reasons listed above, the "standard" config for
a Q35 guest generated by libvirt doesn't support virtio-0.9, hence
doesn't support these guest OSes.



And here's a list of possible solutions to this problem (note that
"consumers" means management applications such as OpenStack, oVirt,
virt-manager, virt-install, gnome-boxes, etc. In all cases, it's assumed
that the consumer's decision on the action to take will be based on
information from libosinfo). For completeness, I've included even the
possibilities that have been rejected, along with a brief synopsis of
(at least part of) the reason for rejection:

  (1) Add some way libvirt consumers can ask libvirt to place
  virtio devices on a legacy pci slot instead of pcie when
  the machinetype is q35 (qemu sets virtio devices in legacy
  PCI slots to transitional mode, so io port space is enabled
  and virtio-0.0 drivers will work).

  This has been proposed on libvir-list, but rejected. Here is
  the most elquently stated reasoning for the rejection I could
  find (with thanks to Dan Berrange):

 The domain XML is a way to express the configuration
 of the guest virtual machine.  What we're talking about
 here is a policy tunable for an internal libvirt QEMU
 driver algorithm, as so does not belong anywhere in the
 domain XML.


  (2) Add full-blown pci enumeration support to all libvirt consumers
  (i.e. they will need to build a model of the PCI bus topology
  of each guest, and keep track of which addresses are in use).
  They can then manually place virtio devices on legacy pci slots
  (again, triggering transitional mode) when the intended guest
  OS doesn't support virtio-0.9.

  (This is seen as requiring too much duplicated effort for
  development and support/maintenance, since up until now libvirt
  has been the single point of action for PCI address assignment
  (well, QEMU can do it too, but for > 10 years libvirt has
  *always* provided full PCI addresses for all devices)


  (3) Add virtio-1.0 support to all guest OSes. If this is done,
  existing libvirt configs will work.

  (Aside from the difficulty of backporting, and the fact that
  there are going to be some OSes that don't get it *at all*,
  there will always be older releases that haven't gotten the
  backport. So this isn't a complete solution).


  (4) Consumers can continue using the 440fx machinetype for guest
  OSes that don't support virtio-0.9

  (This would work, but perpetuates use of the 440fx
  machinetype, and all for just this one reason (at least in
  the case of CentOS6/RHEL6, which otherwise work just fine with
  Q35)).


  (5) Introduce  virtio-0.9, virtio-1.0 models in libvirt
  which are explicitly legacy-only and modern-only.
  QEMU doesn't need to change, as libvirt can simply set
  the right params on existing QEMU models to force the
  behavior.

  (NB: it's unclear to me whether virtio-0.9 simply won't
  work without forcing the device to be on a legacy PCI
  slot, or if that's just "a very bad idea" because it
  will mean that the device uses up extra io port space)

The offline discussion had basically come to the point of saying that
options (4) and (5) were the only reasonable ones, with option (5) being
preferred (I think).

As a starter for continuing the discussion, it seems to me that for
option 

Re: [libvirt] [PATCH v2 2/7] virlockspace: Introduce VIR_LOCK_SPACE_ACQUIRE_WAIT

[John Ferlan]

On 08/14/2018 07:19 AM, Michal Privoznik wrote:
> This flag modifies the way the lock is acquired. It waits for the
> lock to be set instead of usual set-or-fail logic that happens
> without this flag.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/util/virlockspace.c | 14 ++
>  src/util/virlockspace.h |  1 +
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 

Reviewed-by: John Ferlan 

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 1/7] virlockspace: Allow caller to specify start and length offset in virLockSpaceAcquireResource

[John Ferlan]

On 08/14/2018 07:19 AM, Michal Privoznik wrote:
> So far the virLockSpaceAcquireResource() locks the first byte in
> the underlying file. But caller might want to lock other range.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/locking/lock_daemon_dispatch.c |  3 +++
>  src/util/virlockspace.c| 15 ++-
>  src/util/virlockspace.h|  4 
>  tests/virlockspacetest.c   | 29 -
>  4 files changed, 41 insertions(+), 10 deletions(-)
> 


Reviewed-by: John Ferlan 

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] Investigation and possible fix of 1361592 - apparmor profiles do not include backing files

[Povilas Kanapickas]

On 16/08/2018 10:38, Peter Krempa wrote:
> To fix this you should record the backing format [1] into your overlay
> image. If we'd relax the code we'd face the regression in the security
> fix we've done.
> 
> [1] qemu-img creage -f qcow2 -F qcow2 -b backing-qcow2 overlay.qcow2
> 
> -F option specifies the format of the backing file
> 

Thanks a lot for your explanation, now I see that my proposal does not
make any sense. Your suggestion works fine and virt-aa-helper produces
correct output.

Do you think this situation should ideally be diagnosed by higher-level
tools such as virt-manager which right now emit a generic permission
denied error?

Maybe virt-aa-helper could also emit a comment into the apparmor profile
saying something like "image.img has a backing image xyz.img but it was
not probed because its format is not recorded into the overlay image"?

Regards,
Povilas

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 0/3] conf: qemu: support new Hyper-V enlightenments

[John Ferlan]

On 08/09/2018 09:14 AM, Vitaly Kuznetsov wrote:
> Several new Hyper-V enlightenments were recently added to Qemu:
> - hv-frequencies
> - hv-reenlightenment
> - hv-tlbflush
> 
> Support these in libvirt.
> 
> Vitaly Kuznetsov (3):
>   conf: qemu: add support for Hyper-V frequency MSRs
>   conf: qemu: add support for Hyper-V reenlightenment notifications
>   conf: qemu: add support for Hyper-V PV TLB flush
> 
>  docs/formatdomain.html.in   | 21 +
>  docs/schemas/domaincommon.rng   | 15 +++
>  src/conf/domain_conf.c  | 14 +-
>  src/conf/domain_conf.h  |  3 +++
>  src/cpu/cpu_x86.c   |  9 +
>  src/cpu/cpu_x86_data.h  |  3 +++
>  src/qemu/qemu_command.c |  3 +++
>  src/qemu/qemu_parse_command.c   |  3 +++
>  src/qemu/qemu_process.c |  3 +++
>  tests/qemuxml2argvdata/hyperv-off.xml   |  3 +++
>  tests/qemuxml2argvdata/hyperv.args  |  3 ++-
>  tests/qemuxml2argvdata/hyperv.xml   |  3 +++
>  tests/qemuxml2xmloutdata/hyperv-off.xml |  3 +++
>  tests/qemuxml2xmloutdata/hyperv.xml |  3 +++
>  14 files changed, 87 insertions(+), 2 deletions(-)
> 

Reviewed-by: John Ferlan 
(series)

and pushed,

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 09/12] qemu: Generate and use zPCI device in QEMU command line

[Andrea Bolognani]

On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
[...]
> +char *
> +qemuBuildZPCIDevStr(virDomainDeviceInfoPtr dev)
> +{
> +virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> +virBufferAddLit(, "zpci");
> +virBufferAsprintf(, ",uid=%u", dev->addr.pci.zpci->zpci_uid);
> +virBufferAsprintf(, ",fid=%u", dev->addr.pci.zpci->zpci_fid);
> +virBufferAsprintf(, ",target=%s", dev->alias);
> +virBufferAsprintf(, ",id=zpci%u", dev->addr.pci.zpci->zpci_uid);

Just making sure: is the uid a good choice for naming the zpci
device? I would perhaps expect the fid to be in there as well,
but since both have to be unique (right?) I guess it doesn't
really matter...

> +
> +if (virBufferCheckError() < 0) {
> +virBufferFreeAndReset();
> +return NULL;
> +}
> +
> +return virBufferContentAndReset();
> +}
> +
> +int
> +qemuCheckDeviceIsZPCI(virDomainDeviceInfoPtr info)

Based on the name, this is a predicate: it should return a
boolean value rather than an int.

> +{
> +if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
> +info->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
> +return 1;
> +}
> +
> +if (info->addr.pci.zpci) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("This QEMU doesn't support zpci devices"));
> +return -1;
> +}

Not sure about this second check. I guess the logic is supposed to
be that, when passed a "proper" zPCI device, the function would
have returned with the first check, and if we find a zPCI address
after that it's an error. Makes sense, but it's kinda obfuscated
and I'm not sure the error message is accurate: can it really only
be a problem of the QEMU binary not supporting the zPCI feature,
or could we have gotten here because the user is trying to assing
zPCI addresses to devices that don't support them?

Either way, calling a predicate should never result in an error
being reported, so you need to move this check somewhere else.

[...]
> +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.xml
> @@ -0,0 +1,18 @@
> +
> +  QEMUGuest1
> +  c7a5fdbd-edaf-9455-926a-d65c16db1809
> +  219100
> +  
> +hvm
> +  
> +  
> +/usr/bin/qemu-system-s390x
> +
> +  
> +  
> +
> +  
> +  
> +
> +  
> +

This test case would have been a great addition to the previous
commit, where you actually introduced the ability to automatically
allocate zPCI addresses...


Another thing that I forgot to ask earlier and this is as good a
time as any: once zPCI support has been merged, will users have
to opt-in to it using

  

or will they get zPCI devices by default? And if so, will it still
be possible for them to get CCW devices instead if wanted?

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 08/12] conf: Allocate/release 'uid' and 'fid' in PCI address

[Andrea Bolognani]

On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
[...]
> +static inline bool
> +virDeviceInfoPCIAddressExtensionPresent(const virDomainDeviceInfo *info)
> +{
> +return info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
> +info->addr.pci.zpci;
> +}

This should be called virDeviceInfoPCIAddressExtensionIsPresent()
since it's a predicate. I know the corresponding PCI function gets
the naming wrong, but that doesn't mean you should too :)

In the same vein, I don't think this (or the PCI version, for that
matter) need to be inline... I'd rather have them both as regular
functions in the .c file. I'll probably cook up a patch cleaning
up the PCI part later, see what the feedback is.

[...]
> +static int
> +virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds,
> +virZPCIDeviceAddressPtr addr)
> +{
> +if (!addr->uid_assigned &&
> +virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr)) {
> +return -1;
> +}
> +
> +if (!addr->fid_assigned &&
> +virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr)) {
> +virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
> +return -1;
> +}

Not sure I get the logic here... ReserveNextAddress() is supposed
to pick the next available address and reserve it, but IIUC this
will skip picking either id based on whether they were assigned.
I don't think that's what we want.

Also all functions that return an int should be called like

  if (virDomainZPCIAddressReserveNextUid() < 0) {
  ...
  }

[...]
> +static int
> +virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs,
> +   virDomainDeviceInfoPtr dev)
> +{

It's weird that this function doesn't get extFlags as an argument,
unlike the other ones you've introduced. Better make it consistent.

> +virZPCIDeviceAddressPtr zpci = dev->addr.pci.zpci;
> +
> +if (zpci && !dev->pciAddressExtFlags) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("zPCI is not 
> supported."));
> +return -1;
> +}

The error message is very generic here, it should at the very least
make it clear that zPCI is not supported *for the specific device*.

I'm not sure this is the best place to perform that kind of check,
either.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 50/62] qemu: driver: Don't pass 'virDomainDiskDefPtr' to qemuDomainGetStatsOneBlock

[Ján Tomko]

On Mon, Aug 13, 2018 at 06:00:24PM +0200, Peter Krempa wrote:

Allow reuse of qemuDomainGetStatsOneBlock to work with nodenames by
removing the code that looks up the stats data to the caller.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_driver.c | 24 +++-
1 file changed, 15 insertions(+), 9 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 49/62] qemu: monitor: Add APIs for refreshing disk capacity when using -blockdev

[Ján Tomko]

On Mon, Aug 13, 2018 at 06:00:23PM +0200, Peter Krempa wrote:

Disk image size data are not contained in the reply of query-blockstats
but need to be gathered from query-block. For use with -blockdev we
really need to call 'query-named-block-nodes' and process it to retrieve
the correct data.

This patch introduces qemuMonitorBlockStatsUpdateCapacityBlockdev which
updates the capacity data by nodename rather than device name.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_monitor.c  | 11 +++
src/qemu/qemu_monitor.h  |  4 
src/qemu/qemu_monitor_json.c | 46 
src/qemu/qemu_monitor_json.h |  3 +++
4 files changed, 64 insertions(+)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 0a29ad7502..f5dca42b38 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2311,6 +2311,17 @@ qemuMonitorBlockStatsUpdateCapacity(qemuMonitorPtr mon,
}


+int
+qemuMonitorBlockStatsUpdateCapacityBlockdev(qemuMonitorPtr mon,
+virHashTablePtr stats)
+{
+VIR_DEBUG("stats=%p", stats);
+
+QEMU_CHECK_MONITOR(mon);
+
+return qemuMonitorJSONBlockStatsUpdateCapacityBlockdev(mon, stats);
+}
+
int
qemuMonitorBlockResize(qemuMonitorPtr mon,
   const char *device,
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 649a925829..f83a18f563 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -599,6 +599,10 @@ int qemuMonitorBlockStatsUpdateCapacity(qemuMonitorPtr mon,
bool backingChain)
ATTRIBUTE_NONNULL(2);

+int qemuMonitorBlockStatsUpdateCapacityBlockdev(qemuMonitorPtr mon,
+virHashTablePtr stats)


Indentation is off.


+ATTRIBUTE_NONNULL(2);
+
int qemuMonitorBlockResize(qemuMonitorPtr mon,
   const char *device,
   const char *nodename,


Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 47/62] qemu: Explicitly find disks for stats totals

[Ján Tomko]

On Mon, Aug 13, 2018 at 06:00:21PM +0200, Peter Krempa wrote:

Rather than totalling every entry from 'query-block' for stats provided
by qemuDomainBlocksStatsGather total only stats for known disks. This
will allow to return data for nodenames and qdevs in the same hash so
that we can use them with -blockdev.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_driver.c | 25 ++---
1 file changed, 18 insertions(+), 7 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 48/62] qemu: monitor: Retrieve blockstats also by qdev and node-names

[Ján Tomko]

On Mon, Aug 13, 2018 at 06:00:22PM +0200, Peter Krempa wrote:

For use with -blockdev we need to be able to retrieve the stats by
'qdev' for the frontend device stats since 'device' will be empty. Note
that for non-blockdev case qdev and 'device' with 'drive-' skipped would
be the same.

Additionally so that we can report the highest written offset we need to
also be able to access them by node-name for backing chain purposes.

In cases when 'device' is empty it does not make sense to gather them.

Allow arranging the stats simultaneously in all the above dimensions.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_monitor_json.c | 55 
1 file changed, 50 insertions(+), 5 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] storage: Don't hold storage pool locked during wipeVol

[John Ferlan]

On 08/13/2018 06:49 AM, Michal Privoznik wrote:
> Currently the way virStorageVolWipe() works is it looks up
> pool containing given volume and hold it locked throughout while
> API execution. This is suboptimal because wiping a volume means
> writing data to it which can take ages. And if the whole pool is
> locked during that operation no other API can be issued (nor
> pool-list).
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/storage/storage_backend_iscsi_direct.c | 5 +
>  src/storage/storage_backend_rbd.c  | 7 ++-
>  src/storage/storage_util.c | 8 +++-
>  3 files changed, 18 insertions(+), 2 deletions(-)
> 

Why not be more similar to storageVolCreateXMLFrom? That is handle the
in_use incr/decr at the storage driver level. Seems we could create a
helper that would follow the same steps...

For volume wiping, RBD and iscsi-direct use the @pool (obj), but by
incrementing not only vol->in_use, but the pool asyncjobs we can inhibit
the undefine, destroy, or deletion of the pool that would cause issues
for those uses AFAICT. Inhibiting refresh during these operations is a
matter of perhaps opinion as to whether it's a good idea or not -
although I suppose if a volume is open for write (locked), trying to
open/read to get stat type information probably is going to wait anyway
(although I'll admit I haven't put much time or research into that
thought - just going by gut feel ;-)).

BTW: Wouldn't uploadVol have the same issues?  Seems we have only cared
about build vol from.  Since uploadVol checks vol->in_use it seems
logical using the same argument as above that it too should use some new
helper to change pool asyncjobs and vol in_use. The building setting
could just be another parameter.

John

> diff --git a/src/storage/storage_backend_iscsi_direct.c 
> b/src/storage/storage_backend_iscsi_direct.c
> index 1624066e9c..58d25557f1 100644
> --- a/src/storage/storage_backend_iscsi_direct.c
> +++ b/src/storage/storage_backend_iscsi_direct.c
> @@ -691,6 +691,9 @@ virStorageBackenISCSIDirectWipeVol(virStoragePoolObjPtr 
> pool,



Should be BackendISCSI instead of BackenISCSI

and I see this whole new module used single blank line spacing between
functions instead of 2 blank lines. Both would be trivial patches IMO.

>  if (!(iscsi = virStorageBackendISCSIDirectSetConnection(pool, NULL)))
>  return -1;
>  
> +vol->in_use++;
> +virObjectUnlock(pool);
> +
>  switch ((virStorageVolWipeAlgorithm) algorithm) {
>  case VIR_STORAGE_VOL_WIPE_ALG_ZERO:
>  if (virStorageBackendISCSIDirectVolWipeZero(vol, iscsi) < 0) {
> @@ -719,6 +722,8 @@ virStorageBackenISCSIDirectWipeVol(virStoragePoolObjPtr 
> pool,
>   cleanup:
>  virISCSIDirectDisconnect(iscsi);
>  iscsi_destroy_context(iscsi);
> +virObjectLock(pool);
> +vol->in_use--;
>  return ret;
>  }

[...]

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 0/4] Fix a SIGSEGV in libvirtd when querying AMD SEV info

[Brijesh Singh]

Hi Erik,

On 08/15/2018 10:02 AM, Erik Skultety wrote:

This series fixes the following BZ:
https://bugzilla.redhat.com/show_bug.cgi?id=1612009

TL;DR:
We don't format SEV platform data (PDH, certificate chain,...) into our qemu
caps cache which poses a problem after libvirtd restart when we restore from
the cache and get a segfault upon issuing virNodeGetSEVInfo.

I performed some tests on an AMD machine, but CC'ing Brijesh, he might give it
a test too.



I tested this series on my EPYC system (which supports SEV) and
everything seems to be working fine. I have verified the below
code snippet from BZ

import libvirt
conn = libvirt.open()
conn.getSEVInfo()

And I can confirm that getSEVInfo is able to get the PDH certificates etc.

Tested-by: Brijesh Singh 


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 07/12] conf: Introduce parser, formatter for uid and fid

[Andrea Bolognani]

On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
[...]
> +static int
> +virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci)
> +{
> +if (zpci->uid_assigned &&
> +(zpci->zpci_uid > VIR_DOMAIN_DEVICE_ZPCI_MAX_UID ||
> + zpci->zpci_uid == 0)) {
> +virReportError(VIR_ERR_XML_ERROR,
> +   _("Invalid PCI address uid='0x%x', "
> + "must be > 0x0 and <= 0x%x"),
> +   zpci->zpci_uid,
> +   VIR_DOMAIN_DEVICE_ZPCI_MAX_UID);
> +return 0;
> +}
> +
> +if (zpci->fid_assigned) {
> +/* We don't need to check fid because fid covers
> + * all range of uint32 type.
> + */
> +return 1;
> +}

This branch is pointless, just drop it (but leave the comment).

[...]
> @@ -37,6 +37,9 @@ typedef virPCIDeviceAddress *virPCIDeviceAddressPtr;
>  typedef struct _virPCIDeviceList virPCIDeviceList;
>  typedef virPCIDeviceList *virPCIDeviceListPtr;
>  
> +# define VIR_DOMAIN_DEVICE_ZPCI_MAX_UID UINT16_MAX
> +# define VIR_DOMAIN_DEVICE_ZPCI_MAX_FID UINT32_MAX

A single space between the name and the value will do.


This should be

  DO_TEST("disk-virtio-s390-zpci",
  QEMU_CAPS_DEVICE_ZPCI,
  QEMU_CAPS_CCW,
  QEMU_CAPS_VIRTIO_S390);

Same later.


The rest looks good.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/3] qemu: rename method for getting preferred machine type

[Daniel P . Berrangé]

The virQEMUCapsGetDefaultMachine() method doesn't get QEMU's default
machine any more, instead it gets the historical default that libvirt
prefers for each arch. Rename it, so that the old name can be used for
getting QEMU's default.

Signed-off-by: Daniel P. Berrangé 
---
 src/qemu/qemu_capabilities.c | 8 ++--
 src/qemu/qemu_capabilities.h | 2 +-
 tests/domaincapstest.c   | 2 +-
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index a0a1b97f1d..4e4f732889 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -4876,7 +4876,7 @@ virQEMUCapsCacheLookupDefault(virFileCachePtr cache,
 goto cleanup;
 }
 } else {
-machine = virQEMUCapsGetDefaultMachine(qemuCaps);
+machine = virQEMUCapsGetPreferredMachine(qemuCaps);
 }
 
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
@@ -4935,8 +4935,12 @@ virQEMUCapsIsMachineSupported(virQEMUCapsPtr qemuCaps,
 }
 
 
+/*
+ * The preferred machine to use if none is listed explicitly
+ * Note that this may differ from QEMU's own default machine
+ */
 const char *
-virQEMUCapsGetDefaultMachine(virQEMUCapsPtr qemuCaps)
+virQEMUCapsGetPreferredMachine(virQEMUCapsPtr qemuCaps)
 {
 if (!qemuCaps->nmachineTypes)
 return NULL;
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 26813a908c..88e81be09b 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -605,7 +605,7 @@ bool virQEMUCapsSupportsGICVersion(virQEMUCapsPtr qemuCaps,
 bool virQEMUCapsIsMachineSupported(virQEMUCapsPtr qemuCaps,
const char *canonical_machine);
 
-const char *virQEMUCapsGetDefaultMachine(virQEMUCapsPtr qemuCaps);
+const char *virQEMUCapsGetPreferredMachine(virQEMUCapsPtr qemuCaps);
 
 int virQEMUCapsInitGuestFromBinary(virCapsPtr caps,
const char *binary,
diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c
index 06e77fd586..3b94cad223 100644
--- a/tests/domaincapstest.c
+++ b/tests/domaincapstest.c
@@ -168,7 +168,7 @@ fillQemuCaps(virDomainCapsPtr domCaps,
 
 if (!domCaps->machine &&
 VIR_STRDUP(domCaps->machine,
-   virQEMUCapsGetDefaultMachine(qemuCaps)) < 0)
+   virQEMUCapsGetPreferredMachine(qemuCaps)) < 0)
 goto cleanup;
 
 if (virQEMUCapsFillDomainCaps(caps, domCaps, qemuCaps,
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 3/3] qemu: fix default machine for argv -> xml convertor

[Daniel P . Berrangé]

Historically the argv -> xml convertor wanted the same default machine
as we'd set when parsing xml. The latter has now changed, however, to
use a default defined by libvirt. The former needs fixing to again
honour the default QEMU machine.

This exposed a bug in handling for the aarch64 target, as QEMU does not
define any default machine. Thus we should not having been accepting
argv without a -machine provided.

Signed-off-by: Daniel P. Berrangé 
---
 src/qemu/qemu_capabilities.c  | 17 +++-
 src/qemu/qemu_capabilities.h  |  1 +
 src/qemu/qemu_driver.c|  6 ++-
 src/qemu/qemu_parse_command.c | 34 ++--
 src/qemu/qemu_parse_command.h |  8 +++-
 tests/qemuargv2xmldata/nomachine-aarch64.args | 11 -
 tests/qemuargv2xmldata/nomachine-aarch64.xml  | 40 ---
 tests/qemuargv2xmldata/nomachine-ppc64.xml|  4 +-
 tests/qemuargv2xmldata/nomachine-x86_64.xml   |  4 +-
 tests/qemuargv2xmldata/pseries-disk.xml   |  4 +-
 tests/qemuargv2xmldata/pseries-nvram.xml  |  4 +-
 tests/qemuargv2xmltest.c  | 18 -
 .../caps_1.5.3.x86_64.xml |  2 +-
 .../caps_1.6.0.x86_64.xml |  2 +-
 .../caps_1.7.0.x86_64.xml |  2 +-
 .../caps_2.1.1.x86_64.xml |  2 +-
 .../caps_2.10.0.ppc64.xml |  2 +-
 .../caps_2.10.0.s390x.xml |  2 +-
 .../caps_2.10.0.x86_64.xml|  2 +-
 .../caps_2.11.0.s390x.xml |  2 +-
 .../caps_2.11.0.x86_64.xml|  2 +-
 .../caps_2.12.0.ppc64.xml |  2 +-
 .../caps_2.12.0.s390x.xml |  2 +-
 .../caps_2.12.0.x86_64.xml|  2 +-
 .../caps_2.4.0.x86_64.xml |  2 +-
 .../caps_2.5.0.x86_64.xml |  2 +-
 .../qemucapabilitiesdata/caps_2.6.0.ppc64.xml |  2 +-
 .../caps_2.6.0.x86_64.xml |  2 +-
 .../qemucapabilitiesdata/caps_2.7.0.s390x.xml |  2 +-
 .../caps_2.7.0.x86_64.xml |  2 +-
 .../qemucapabilitiesdata/caps_2.8.0.s390x.xml |  2 +-
 .../caps_2.8.0.x86_64.xml |  2 +-
 .../qemucapabilitiesdata/caps_2.9.0.ppc64.xml |  2 +-
 .../qemucapabilitiesdata/caps_2.9.0.s390x.xml |  2 +-
 .../caps_2.9.0.x86_64.xml |  2 +-
 .../qemucapabilitiesdata/caps_3.0.0.ppc64.xml |  2 +-
 .../caps_3.0.0.x86_64.xml |  2 +-
 37 files changed, 97 insertions(+), 104 deletions(-)
 delete mode 100644 tests/qemuargv2xmldata/nomachine-aarch64.args
 delete mode 100644 tests/qemuargv2xmldata/nomachine-aarch64.xml

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 4e4f732889..72b550ae16 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -1625,6 +1625,7 @@ virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps)
 goto error;
 ret->machineTypes[i].maxCpus = qemuCaps->machineTypes[i].maxCpus;
 ret->machineTypes[i].hotplugCpus = 
qemuCaps->machineTypes[i].hotplugCpus;
+ret->machineTypes[i].qemuDefault = 
qemuCaps->machineTypes[i].qemuDefault;
 }
 
 if (VIR_ALLOC_N(ret->gicCapabilities, qemuCaps->ngicCapabilities) < 0)
@@ -2042,6 +2043,17 @@ const char 
*virQEMUCapsGetCanonicalMachine(virQEMUCapsPtr qemuCaps,
 return name;
 }
 
+const char *virQEMUCapsGetDefaultMachine(virQEMUCapsPtr qemuCaps)
+{
+size_t i;
+
+for (i = 0; i < qemuCaps->nmachineTypes; i++) {
+if (qemuCaps->machineTypes[i].qemuDefault)
+return qemuCaps->machineTypes[i].name;
+}
+
+return NULL;
+}
 
 int virQEMUCapsGetMachineMaxCpus(virQEMUCapsPtr qemuCaps,
  const char *name)
@@ -3776,10 +3788,11 @@ virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps)
   qemuCaps->machineTypes[i].alias);
 if (qemuCaps->machineTypes[i].hotplugCpus)
 virBufferAddLit(, " hotplugCpus='yes'");
+virBufferAsprintf(, " maxCpus='%u'",
+  qemuCaps->machineTypes[i].maxCpus);
 if (qemuCaps->machineTypes[i].qemuDefault)
 virBufferAddLit(, " default='yes'");
-virBufferAsprintf(, " maxCpus='%u'/>\n",
-  qemuCaps->machineTypes[i].maxCpus);
+virBufferAddLit(, "/>\n");
 }
 
 for (i = 0; i < qemuCaps->ngicCapabilities; i++) {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 88e81be09b..a410885215 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -558,6 +558,7 @@ bool virQEMUCapsIsCPUModeSupported(virQEMUCapsPtr qemuCaps,
virCPUMode mode);
 const char *virQEMUCapsGetCanonicalMachine(virQEMUCapsPtr qemuCaps,
const char *name);
+const char 

[libvirt] [PATCH 1/3] qemu: record the QEMU default machine in capabilities

[Daniel P . Berrangé]

We don't honour the QEMU default machine type anymore, always using the
libvirt chosen default instead. The QEMU argv parser, however, will need
to know the exacty QEMU default, so we must record that info.

Signed-off-by: Daniel P. Berrangé 
---
 src/qemu/qemu_capabilities.c | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index e6e199b2c6..a0a1b97f1d 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -515,6 +515,7 @@ struct virQEMUCapsMachineType {
 char *alias;
 unsigned int maxCpus;
 bool hotplugCpus;
+bool qemuDefault;
 };
 
 typedef struct _virQEMUCapsHostCPUData virQEMUCapsHostCPUData;
@@ -2324,8 +2325,10 @@ virQEMUCapsProbeQMPMachineTypes(virQEMUCapsPtr qemuCaps,
 preferredIdx = qemuCaps->nmachineTypes - 1;
 }
 
-if (machines[i]->isDefault)
+if (machines[i]->isDefault) {
+mach->qemuDefault = true;
 defIdx = qemuCaps->nmachineTypes - 1;
+}
 }
 
 /*
@@ -3355,7 +3358,7 @@ virQEMUCapsCachePrivFree(void *privData)
  *   ...
  *   
  *   ...
- *   
+ *   
  *   ...
  * 
  */
@@ -3520,6 +3523,11 @@ virQEMUCapsLoadCache(virArch hostArch,
 if (STREQ_NULLABLE(str, "yes"))
 qemuCaps->machineTypes[i].hotplugCpus = true;
 VIR_FREE(str);
+
+str = virXMLPropString(nodes[i], "default");
+if (STREQ_NULLABLE(str, "yes"))
+qemuCaps->machineTypes[i].qemuDefault = true;
+VIR_FREE(str);
 }
 }
 VIR_FREE(nodes);
@@ -3768,6 +3776,8 @@ virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps)
   qemuCaps->machineTypes[i].alias);
 if (qemuCaps->machineTypes[i].hotplugCpus)
 virBufferAddLit(, " hotplugCpus='yes'");
+if (qemuCaps->machineTypes[i].qemuDefault)
+virBufferAddLit(, " default='yes'");
 virBufferAsprintf(, " maxCpus='%u'/>\n",
   qemuCaps->machineTypes[i].maxCpus);
 }
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 0/3] Fix default machine when converting argv to xml

[Daniel P . Berrangé]

We must honour the QEMU built-in default machine when converting from argv

Daniel P. Berrangé (3):
  qemu: record the QEMU default machine in capabilities
  qemu: rename method for getting preferred machine type
  qemu: fix default machine for argv -> xml convertor

 src/qemu/qemu_capabilities.c  | 37 ++---
 src/qemu/qemu_capabilities.h  |  3 +-
 src/qemu/qemu_driver.c|  6 ++-
 src/qemu/qemu_parse_command.c | 34 ++--
 src/qemu/qemu_parse_command.h |  8 +++-
 tests/domaincapstest.c|  2 +-
 tests/qemuargv2xmldata/nomachine-aarch64.args | 11 -
 tests/qemuargv2xmldata/nomachine-aarch64.xml  | 40 ---
 tests/qemuargv2xmldata/nomachine-ppc64.xml|  4 +-
 tests/qemuargv2xmldata/nomachine-x86_64.xml   |  4 +-
 tests/qemuargv2xmldata/pseries-disk.xml   |  4 +-
 tests/qemuargv2xmldata/pseries-nvram.xml  |  4 +-
 tests/qemuargv2xmltest.c  | 18 -
 .../caps_1.5.3.x86_64.xml |  2 +-
 .../caps_1.6.0.x86_64.xml |  2 +-
 .../caps_1.7.0.x86_64.xml |  2 +-
 .../caps_2.1.1.x86_64.xml |  2 +-
 .../caps_2.10.0.ppc64.xml |  2 +-
 .../caps_2.10.0.s390x.xml |  2 +-
 .../caps_2.10.0.x86_64.xml|  2 +-
 .../caps_2.11.0.s390x.xml |  2 +-
 .../caps_2.11.0.x86_64.xml|  2 +-
 .../caps_2.12.0.ppc64.xml |  2 +-
 .../caps_2.12.0.s390x.xml |  2 +-
 .../caps_2.12.0.x86_64.xml|  2 +-
 .../caps_2.4.0.x86_64.xml |  2 +-
 .../caps_2.5.0.x86_64.xml |  2 +-
 .../qemucapabilitiesdata/caps_2.6.0.ppc64.xml |  2 +-
 .../caps_2.6.0.x86_64.xml |  2 +-
 .../qemucapabilitiesdata/caps_2.7.0.s390x.xml |  2 +-
 .../caps_2.7.0.x86_64.xml |  2 +-
 .../qemucapabilitiesdata/caps_2.8.0.s390x.xml |  2 +-
 .../caps_2.8.0.x86_64.xml |  2 +-
 .../qemucapabilitiesdata/caps_2.9.0.ppc64.xml |  2 +-
 .../qemucapabilitiesdata/caps_2.9.0.s390x.xml |  2 +-
 .../caps_2.9.0.x86_64.xml |  2 +-
 .../qemucapabilitiesdata/caps_3.0.0.ppc64.xml |  2 +-
 .../caps_3.0.0.x86_64.xml |  2 +-
 38 files changed, 116 insertions(+), 109 deletions(-)
 delete mode 100644 tests/qemuargv2xmldata/nomachine-aarch64.args
 delete mode 100644 tests/qemuargv2xmldata/nomachine-aarch64.xml

-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 06/12] conf: Introduce address caching for PCI extensions

[Andrea Bolognani]

On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
> This patch provides a caching mechanism for the device address
> extensions uid and fid on S390. For efficient sparse address allocation,
> we introduce two hash tables for uid/fid which hold the address set
> information per domain. Also in order to improve performance of
> searching available value, we introduce our own callbacks for the two
> hashtables. In this way, uid/fid is saved in hash key and hash value
> could be any non-NULL pointer due to no operation on hash value. That is
> also the reason why we don't introduce hash value free callback.
> 
> Signed-off-by: Yi Min Zhao 
> Reviewed-by: Boris Fiuczynski 
> Reviewed-by: Bjoern Walk 
> Reviewed-by: Ján Tomko 
> ---
>  src/conf/domain_addr.c | 85 
> ++
>  src/conf/domain_addr.h |  9 +
>  src/libvirt_private.syms   |  1 +
>  src/qemu/qemu_domain_address.c |  7 
>  4 files changed, 102 insertions(+)

I haven't looked into the hash table handling in detail but I
wonder if it's really necessary? IIUC you're using it just to
mark which uids and fids have been already used by a device,
which the PCI address allocation code does by setting bits
inside integer variables - having a custom hash table for the
same seems like overkill, and from the maintenance point of
view it would be great to have the logic for PCI address and
zPCI address allocation be similar unless diverging is strictly
necessary.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 05/12] qemu: Auto add pci-root for s390/s390x guests

[Cornelia Huck]

On Thu, 16 Aug 2018 16:52:18 +0200
Andrea Bolognani  wrote:

> On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
> > The pci-root depends on zpci capability. So autogenerate pci-root if
> > zpci exists.
> > 
> > Signed-off-by: Yi Min Zhao 
> > Reviewed-by: Boris Fiuczynski 
> > Reviewed-by: Stefan Zimmermann 
> > Reviewed-by: Bjoern Walk 
> > Reviewed-by: Ján Tomko 
> > ---
> >  src/qemu/qemu_domain.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index de056272e8..a84e5f06b2 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -3227,6 +3227,7 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def,
> >  case VIR_ARCH_S390X:
> >  addDefaultUSB = false;
> >  addPanicDevice = true;
> > +addPCIRoot = virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI);
> >  break;  
> 
> I wonder if you might want to do this, and other stuff, only if
> qemuDomainIsS390CCW()... It seems like that function is not used
> a lot, which might mean that someone is going to have to perform
> some serious cleaning up if there is ever another machine type
> on s390, or perhaps that it just doesn't quite apply here.

Not a libvirt person, but that other QEMU s390 machine type is long
gone (and was never supported in libvirt AFAIK anyway), and I don't see
any reason to add an additional s390 machine type to QEMU from where we
stand now.

> 
> If the latter,
> 
>   Reviewed-by: Andrea Bolognani 
> 


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 46/62] qemu: driver: Don't copy disk alias in qemuDomainBlocksStatsGather

[Ján Tomko]

On Mon, Aug 13, 2018 at 06:00:20PM +0200, Peter Krempa wrote:

The string is not modified so it does not make sense to have a copy.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_driver.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 45/62] qemu: hotplug: Implement removable media change for -blockdev

[Ján Tomko]

On Mon, Aug 13, 2018 at 06:00:19PM +0200, Peter Krempa wrote:

Use the new APIs which allow to manipulate the tray and media separately
and also allow using a nodename to refer to a media to implement media
changing.

With the new approach we don't have to call eject twice as the media is
removed by calling qemuMonitorBlockdevMediumRemove.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_hotplug.c | 95 -
1 file changed, 94 insertions(+), 1 deletion(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 05/12] qemu: Auto add pci-root for s390/s390x guests

[Andrea Bolognani]

On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
> The pci-root depends on zpci capability. So autogenerate pci-root if
> zpci exists.
> 
> Signed-off-by: Yi Min Zhao 
> Reviewed-by: Boris Fiuczynski 
> Reviewed-by: Stefan Zimmermann 
> Reviewed-by: Bjoern Walk 
> Reviewed-by: Ján Tomko 
> ---
>  src/qemu/qemu_domain.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index de056272e8..a84e5f06b2 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -3227,6 +3227,7 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def,
>  case VIR_ARCH_S390X:
>  addDefaultUSB = false;
>  addPanicDevice = true;
> +addPCIRoot = virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI);
>  break;

I wonder if you might want to do this, and other stuff, only if
qemuDomainIsS390CCW()... It seems like that function is not used
a lot, which might mean that someone is going to have to perform
some serious cleaning up if there is ever another machine type
on s390, or perhaps that it just doesn't quite apply here.

If the latter,

  Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 04/12] qemu: Enable PCI multi bus for S390 guests

[Andrea Bolognani]

On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
> QEMU on s390 supports PCI multibus since forever.
> 
> Signed-off-by: Yi Min Zhao 
> Reviewed-by: Boris Fiuczynski 
> Reviewed-by: Stefan Zimmermann 
> Reviewed-by: Bjoern Walk 
> Reviewed-by: Ján Tomko 
> ---
>  src/qemu/qemu_capabilities.c | 4 
>  1 file changed, 4 insertions(+)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 03/12] conf: Introduce a new PCI address extension flag

[Andrea Bolognani]

On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
[...]
> +qemuDomainDeviceSupportZPCI(virDomainDeviceDefPtr device)
> +{
> +switch ((virDomainDeviceType) device->type) {
> +case VIR_DOMAIN_DEVICE_CHR:
> +return false;
> +
> +case VIR_DOMAIN_DEVICE_CONTROLLER:
> +case VIR_DOMAIN_DEVICE_DISK:
> +case VIR_DOMAIN_DEVICE_LEASE:
> +case VIR_DOMAIN_DEVICE_FS:
> +case VIR_DOMAIN_DEVICE_NET:
> +case VIR_DOMAIN_DEVICE_INPUT:
> +case VIR_DOMAIN_DEVICE_SOUND:
> +case VIR_DOMAIN_DEVICE_VIDEO:
> +case VIR_DOMAIN_DEVICE_HOSTDEV:
> +case VIR_DOMAIN_DEVICE_WATCHDOG:
> +case VIR_DOMAIN_DEVICE_GRAPHICS:
> +case VIR_DOMAIN_DEVICE_HUB:
> +case VIR_DOMAIN_DEVICE_REDIRDEV:
> +case VIR_DOMAIN_DEVICE_SMARTCARD:
> +case VIR_DOMAIN_DEVICE_MEMBALLOON:
> +case VIR_DOMAIN_DEVICE_NVRAM:
> +case VIR_DOMAIN_DEVICE_RNG:
> +case VIR_DOMAIN_DEVICE_SHMEM:
> +case VIR_DOMAIN_DEVICE_TPM:
> +case VIR_DOMAIN_DEVICE_PANIC:
> +case VIR_DOMAIN_DEVICE_MEMORY:
> +case VIR_DOMAIN_DEVICE_IOMMU:
> +case VIR_DOMAIN_DEVICE_VSOCK:
> +break;
> +
> +case VIR_DOMAIN_DEVICE_NONE:
> +case VIR_DOMAIN_DEVICE_LAST:

Missing 'default' case.

> +virReportEnumRangeError(virDomainDeviceType, device->type);
> +return false;
> +}

Add an empty line here.

> +return true;
> +}

[...]
> +static int
> +qemuDomainFillDevicePCIExtensionFlagsIter(virDomainDefPtr def 
> ATTRIBUTE_UNUSED,
> +  virDomainDeviceDefPtr dev,
> +  virDomainDeviceInfoPtr info,
> +  void *opaque)
> +{
> +virQEMUCapsPtr qemuCaps = opaque;
> +
> +info->pciAddressExtFlags
> += qemuDomainDeviceCalculatePCIAddressExtensionFlags(qemuCaps, dev);

Add an empty line here.

> +return 0;
> +}

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 02/12] qemu: Introduce zPCI capability

[Andrea Bolognani]

On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
> Let's introduce zPCI capability.
> 
> Signed-off-by: Yi Min Zhao 
> Reviewed-by: Boris Fiuczynski 
> Reviewed-by: Stefan Zimmermann 
> Reviewed-by: Bjoern Walk 
> Reviewed-by: Ján Tomko 
> ---
>  src/qemu/qemu_capabilities.c | 2 ++
>  src/qemu/qemu_capabilities.h | 1 +
>  tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 1 +
>  tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 +
>  tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 +
>  tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml  | 1 +
>  tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml  | 1 +
>  tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml  | 1 +
>  8 files changed, 9 insertions(+)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 01/12] conf: Add definitions for 'uid' and 'fid' PCI address attributes

[Andrea Bolognani]

On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
> +typedef struct _virZPCIDeviceAddress virZPCIDeviceAddress;
> +typedef virZPCIDeviceAddress *virZPCIDeviceAddressPtr;
> +struct _virZPCIDeviceAddress {
> +unsigned int zpci_fid;
> +unsigned int zpci_uid;
> +bool fid_assigned;
> +bool uid_assigned;
> +};

A couple of questions about the approach here, one of which I have
mentioned already and one of which I probably haven't (my bad):

  * do you really need to have separate booleans tracking whether
or not either id has been assigned? Wouldn't the same approach
as virPCIDeviceAddressIsEmpty() work, eg. consider the address
absent if both are zero and present otherwise?

  * especially if you don't need the additional booleans, would it
be preferable to just add the two ids to the existing struct
instead of declaring a new one that you'll have to allocate
and make sure it's not NULL before accessing it? Again, I seem
to remember Laine feeling somewhat strongly about the topic.

Cosmetic issues:

  * uid should be listed before fid;

  * the zpci_ prefix is unnecessary if you have a separate struct
that contains "ZPCI" in the name. But see above :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC PATCH 01/17] qemu: setup shared memory without explicit numa configuration

[Marc-André Lureau]

Hi
On Thu, Aug 16, 2018 at 12:48 PM Daniel P. Berrangé  wrote:
>
> On Fri, Jul 13, 2018 at 03:28:08PM +0200, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau 
> >
> > When a domain is configured with 'shared' memory backing:
> >
> >   
> > 
> >   
> >
> > But no explicit NUMA configuration, let's configure a shared memory
> > backend associated with default -numa.
>
>
> > diff --git a/tests/qemuxml2argvdata/fd-memory-no-numa-topology.args 
> > b/tests/qemuxml2argvdata/fd-memory-no-numa-topology.args
> > index bd88daaa3b..400fb39cc6 100644
> > --- a/tests/qemuxml2argvdata/fd-memory-no-numa-topology.args
> > +++ b/tests/qemuxml2argvdata/fd-memory-no-numa-topology.args
> > @@ -11,6 +11,10 @@ QEMU_AUDIO_DRV=none \
> >  -m 14336 \
> >  -mem-prealloc \
> >  -smp 8,sockets=8,cores=1,threads=1 \
> > +-object memory-backend-file,id=ram-node,\
> > +mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-instance-0092/ram-node,\
> > +share=yes,size=15032385536 \
> > +-numa node,nodeid=0,memdev=ram-node \
>
> I'm not at all convinced it is safe todo this. We've been burnt in the
> past by adding  use of memory-backend objects causing migration to break
>
>
>   commit f309db1f4d51009bad0d32e12efc75530b66836b
>   Author: Michal Privoznik 
>   Date:   Thu Dec 18 12:36:48 2014 +0100
>
> qemu: Create memory-backend-{ram,file} iff needed
>
> Libvirt BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1175397
> QEMU BZ:https://bugzilla.redhat.com/show_bug.cgi?id=1170093
>
> This change doesn't really feel like it is required either. If the
> user wants NUMA, then the XML can just be written to request a NUMA
> topology with a single node.  Better to be explicit in the XML rather
> than silently adding things as a side effect

Ok, let's drop this patch then. I'll modify "qemu: use
memory-backend-memfd if possible" to make use of memfd differently,
with explicit NUMA.

>
> 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 :|
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list



-- 
Marc-André Lureau

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 44/62] qemu: monitor: Add APIs for cdrom tray handling for -blockdev

[Ján Tomko]

On Mon, Aug 13, 2018 at 06:00:18PM +0200, Peter Krempa wrote:

With blockdev we can use the full range of commands to manipulate the
tray and the medium separately. Implement monitor code for this.

Schema testing done in the qemumonitorjsontest allows us to verify that
we generate the commands correctly.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_monitor.c  |  51 +++
src/qemu/qemu_monitor.h  |  14 ++
src/qemu/qemu_monitor_json.c | 114 +++
src/qemu/qemu_monitor_json.h |  18 +++
tests/qemumonitorjsontest.c  |   8 +++
5 files changed, 205 insertions(+)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 49dc478f5b..0a29ad7502 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -4336,6 +4336,57 @@ qemuMonitorBlockdevDel(qemuMonitorPtr mon,
return qemuMonitorJSONBlockdevDel(mon, nodename);
}

+int
+qemuMonitorBlockdevTrayOpen(qemuMonitorPtr mon,
+const char *id,
+bool force)
+{
+VIR_DEBUG("id=%s force=%d", id, force);
+
+QEMU_CHECK_MONITOR(mon);
+
+return qemuMonitorJSONBlockdevTrayOpen(mon, id, force);
+}
+
+
+int
+qemuMonitorBlockdevTrayClose(qemuMonitorPtr mon,
+ const char *id)
+{
+VIR_DEBUG("id=%s", id);
+
+QEMU_CHECK_MONITOR(mon);
+
+return qemuMonitorJSONBlockdevTrayClose(mon, id);
+}
+
+
+int
+qemuMonitorBlockdevMediumRemove(qemuMonitorPtr mon,
+const char *id)
+{
+VIR_DEBUG("id=%s", id);
+
+QEMU_CHECK_MONITOR(mon);
+
+return qemuMonitorJSONBlockdevMediumRemove(mon, id);
+}
+
+
+


Extra empty line.


+int
+qemuMonitorBlockdevMediumInsert(qemuMonitorPtr mon,
+const char *id,
+const char *nodename)
+{
+VIR_DEBUG("id=%s nodename=%s", id, nodename);
+
+QEMU_CHECK_MONITOR(mon);
+
+return qemuMonitorJSONBlockdevMediumInsert(mon, id, nodename);
+}
+
+
char *
qemuMonitorGetSEVMeasurement(qemuMonitorPtr mon)
{


Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 43/62] qemu: hotplug: Prepare for blockdev-add/blockdev-del with backing chains

[Ján Tomko]

On Mon, Aug 13, 2018 at 06:00:17PM +0200, Peter Krempa wrote:

Initialize data for the whole backing chain when plugging in or removing
disks when a machine supports -blockdev.

Similarly to startup we need to prepare the structures for the whole
backing chain and take care of the copy-on-read feature.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_hotplug.c | 77 +++--
1 file changed, 62 insertions(+), 15 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 42/62] qemu: monitor: Handle BLOCK_IO_ERROR event properly with -blockdev

[Ján Tomko]

On Mon, Aug 13, 2018 at 06:00:16PM +0200, Peter Krempa wrote:

Use the 'node-name' provided in the event if 'device' is empty to look
up the disk.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_monitor.c  |  3 ++-
src/qemu/qemu_monitor.h  |  2 ++
src/qemu/qemu_monitor_json.c |  5 -
src/qemu/qemu_process.c  | 12 +++-
4 files changed, 19 insertions(+), 3 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt-glib][PATCH] configure: Drop GLIB2_TEST_REQUIRED

[Andrea Bolognani]

On Thu, 2018-08-16 at 14:56 +0200, Michal Privoznik wrote:
> Introduced in eb1f97a4b49a6e it is not needed anymore. The
> minimal required version of glib is now the same as the one
> required for tests.
> 
> This practically reverts the referenced commit.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  configure.ac  | 8 +---
>  tests/Makefile.am | 3 ---
>  2 files changed, 1 insertion(+), 10 deletions(-)

Thanks for taking care of this :)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 41/62] qemu: monitor: Handle TRAY_MOVED event correctly with -blockdev

[Ján Tomko]

On Mon, Aug 13, 2018 at 06:00:15PM +0200, Peter Krempa wrote:

Add handling of the 'id' field in the event which corresponds to the
QDEV id of the device.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_monitor.c  |  3 ++-
src/qemu/qemu_monitor.h  |  2 ++
src/qemu/qemu_monitor_json.c | 11 ---
src/qemu/qemu_process.c  |  3 ++-
4 files changed, 14 insertions(+), 5 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 1/2] process: wait longer on kill per assigned Hostdev

[Christian Ehrhardt]

On Tue, Aug 14, 2018 at 12:13 PM Bjoern Walk  wrote:

> Christian Ehrhardt  [2018-08-14,
> 11:27AM +0200]:
> > diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> > index ecea27a2d4..46360cc051 100644
> > --- a/src/util/virprocess.c
> > +++ b/src/util/virprocess.c
> > @@ -341,15 +341,19 @@ int virProcessKill(pid_t pid, int sig)
> >   * Returns 0 if it was killed gracefully, 1 if it
> >   * was killed forcibly, -1 if it is still alive,
> >   * or another error occurred.
> > + *
> > + * Callers can proide an extra delay to wait longer
> > + * than the default.
> >   */
> >  int
> > -virProcessKillPainfully(pid_t pid, bool force)
> > +virProcessKillPainfullyDelay(pid_t pid, bool force, unsigned int
> extradelay)
> >  {
> >  size_t i;
> >  int ret = -1;
> > +unsigned int delay = 75 + (extradelay*5);
> >  const char *signame = "TERM";
> >
> > -VIR_DEBUG("vpid=%lld force=%d", (long long)pid, force);
> > +VIR_DEBUG("vpid=%lld force=%d delay=%u", (long long)pid, force,
> pid);
>^
> This probably should be delay, right?
>

Absolutely correct, bad old copy-pasta from the beginning of this change.
Thanks for spotting it.

Example is with 12 Devices passed through and force, which means with both
patches applied:
- raise base value to wait after kill by 30 seconds (force is set) - second
patch
- request 12*2 seconds extra for the devices - this patch
That should be 200+(12*2*5)=320 and that matches what I see int he log:

2018-08-16 13:04:51.999+: 61243: debug :
virProcessKillPainfullyDelay:359 : vpid=60939 force=1 delay=320

I'm not pushing a V2 just for the change, but I have it queued with the
fixed variable.
Any other feedback on this or the sibling patch?
Especially since those aren't my usual apparmor changes I'd really want
some more reviews/acks before considering a push.

-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 40/62] qemu: process: Add lookup via QOM id to qemuProcessFindDomainDiskByAlias

[Ján Tomko]

On Mon, Aug 13, 2018 at 06:00:14PM +0200, Peter Krempa wrote:

Allow looking up also via QOM id and rename the function accordingly.
Also add documentation of the specifics.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_driver.c  |  2 +-
src/qemu/qemu_process.c | 42 +++---
src/qemu/qemu_process.h |  5 +++--
3 files changed, 35 insertions(+), 14 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f745a0392a..5dee701dc4 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4727,7 +4727,7 @@ processBlockJobEvent(virQEMUDriverPtr driver,
goto endjob;
}

-if ((disk = qemuProcessFindDomainDiskByAlias(vm, diskAlias)))
+if ((disk = qemuProcessFindDomainDiskByAliasOrQOM(vm, diskAlias, NULL)))
qemuBlockJobEventProcess(driver, vm, disk, QEMU_ASYNC_JOB_NONE, type, 
status);

 endjob:
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 3495733041..b713afa3a2 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -350,28 +350,48 @@ qemuProcessHandleMonitorError(qemuMonitorPtr mon 
ATTRIBUTE_UNUSED,
}


+/**
+ * qemuProcessFindDomainDiskByAliasOrQOM:
+ * @vm: domain object to search for the disk
+ * @alias: -drive or -device alias of the disk
+ * @qomid: QOM tree device name
+ *
+ * Looks up a disk in the domain definition of @vm which either matches the
+ * -drive or -device alias used for the backend and frontend respectively or 
the
+ *  QOM name. If @alias is empty it's treated as NULL as it's a mandatory field
+ *  in some cases.
+ *
+ *  Returns a disk from @vm or NULL if it could not be found.


Three out of the last four lines are misaligned.

With the crasher fixup applied:

Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 39/62] qemu: driver: Prepare qemuDomainBlockResize for blockdev

[Ján Tomko]

On Mon, Aug 13, 2018 at 06:00:13PM +0200, Peter Krempa wrote:

Use the nodename to resize the device rather than the drive alias.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_driver.c | 18 +++---
1 file changed, 15 insertions(+), 3 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 38/62] qemu: driver: Use QOM backend name for disk IO throttling APIs

[Ján Tomko]

On Mon, Aug 13, 2018 at 06:00:12PM +0200, Peter Krempa wrote:

With -blockdev the drive alias can't be used any more so we need to
switch to the QOM name.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_driver.c | 30 --
1 file changed, 20 insertions(+), 10 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 37/62] qemu: process: Setup disk io throttling for -blockdev

[Ján Tomko]

On Mon, Aug 13, 2018 at 06:00:11PM +0200, Peter Krempa wrote:

The proper way to do this would be to use the 'throttle' driver but
unfortunately it can't change the 'throttle_group' so we can't provide
feature parity. This hack uses the block_set_io_throttle command to do
so until we can properly replace it.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_process.c | 50 +
1 file changed, 50 insertions(+)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 36/62] qemu: command: Add helper to check if disk throttling is enabled

[Ján Tomko]

On Mon, Aug 13, 2018 at 06:00:10PM +0200, Peter Krempa wrote:

Add a helper which will use a collection of other helpers to determine
whether a disk requires throttling to be enabled.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_command.c | 10 ++
src/qemu/qemu_command.h |  3 +++
2 files changed, 13 insertions(+)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [libvirt-glib][PATCH] configure: Drop GLIB2_TEST_REQUIRED

[Michal Privoznik]

Introduced in eb1f97a4b49a6e it is not needed anymore. The
minimal required version of glib is now the same as the one
required for tests.

This practically reverts the referenced commit.

Signed-off-by: Michal Privoznik 
---
 configure.ac  | 8 +---
 tests/Makefile.am | 3 ---
 2 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/configure.ac b/configure.ac
index dd1a85a..adc9e6b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -13,7 +13,6 @@ LIBVIRT_REQUIRED=1.2.5
 AC_SUBST([LIBVIRT_REQUIRED]) dnl used in the .spec file
 GLIB2_REQUIRED=2.38.0
 AC_SUBST([GLIB2_REQUIRED]) dnl used in the .spec file
-GLIB2_TEST_REQUIRED=2.38.0
 GOBJECT_INTROSPECTION_REQUIRED=1.36.0
 LIBXML2_REQUIRED=2.0.0
 
@@ -107,17 +106,12 @@ PKG_CHECK_EXISTS(libvirt >= 1.2.6,
   AC_DEFINE([HAVE_VIR_NETWORK_GET_DHCP_LEASES], 1, [Have 
virNetworkGetDHCPLeases?])
   AC_MSG_RESULT([yes])
 ],[AC_MSG_RESULT([no])])
-enable_tests=no
-PKG_CHECK_MODULES(GLIB2, glib-2.0 >= $GLIB2_TEST_REQUIRED,
-  [enable_tests=yes],
-  [PKG_CHECK_MODULES(GLIB2, glib-2.0 >= $GLIB2_REQUIRED)])
+PKG_CHECK_MODULES(GLIB2, glib-2.0 >= $GLIB2_REQUIRED)
 PKG_CHECK_MODULES(GTHREAD2, gthread-2.0 >= $GLIB2_REQUIRED)
 PKG_CHECK_MODULES(GOBJECT2, gobject-2.0 >= $GLIB2_REQUIRED)
 PKG_CHECK_MODULES(GIO2, gio-2.0 >= $GLIB2_REQUIRED)
 PKG_CHECK_MODULES(LIBXML2, libxml-2.0 >= $LIBXML2_REQUIRED)
 
-AM_CONDITIONAL([ENABLE_TESTS], [test "$enable_tests" = "yes"])
-
 LIBVIRT_GLIB_GETTEXT
 dnl Should be in m4/virt-gettext.m4 but intltoolize is too
 dnl dumb to find it there
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 3f4ef6c..2fa847a 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -1,5 +1,3 @@
-if ENABLE_TESTS
-
 include $(top_srcdir)/build-aux/glib-tap.mk
 
 AM_CFLAGS = \
@@ -22,4 +20,3 @@ test_programs = test-gconfig test-events
 EXTRA_DIST += \
xml \
$(NULL)
-endif
-- 
2.16.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 35/62] qemu: command: format disk source commandline for -blockdev

[Ján Tomko]

On Mon, Aug 13, 2018 at 06:00:09PM +0200, Peter Krempa wrote:

Format the backing chain onto the commandline using the 'json' syntax
with -blockdev.

The command line formatter needs only minor tweaks to add the new
entries but we now need to initialize the strucutres that are used for


structures


every layer of the backing chain.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_command.c | 82 -
1 file changed, 75 insertions(+), 7 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 34/62] qemu: domain: Prepare qemuDomainDiskGetBackendAlias for -blockdev

[Ján Tomko]

On Mon, Aug 13, 2018 at 06:00:08PM +0200, Peter Krempa wrote:

Pass in the node name as the backend alias when -blockdev is used. As
copy-on-read is expressed by a separate -blockdev backing chain member
we need to decide which node name to use here.

For empty cdroms when using -blockdev there is no backend at all so NULL
is returned.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_domain.c | 21 +++--
1 file changed, 19 insertions(+), 2 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 33/62] qemu: block: Add generator for the 'copy-on-read' blockdev driver

[Ján Tomko]

On Mon, Aug 13, 2018 at 06:00:07PM +0200, Peter Krempa wrote:

The copy on read functionality is done using a separate layer in the
backing chain. Add function to generate properties for it.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_block.c | 22 ++
src/qemu/qemu_block.h |  2 ++
2 files changed, 24 insertions(+)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 32/62] qemu: proces: assign node names for user defined backing chains

[Ján Tomko]

s/proces/process/

On Mon, Aug 13, 2018 at 06:00:06PM +0200, Peter Krempa wrote:

Prepare the full backing chain by instantiating authentication and TLS
transport secrets and other necessary objects so that we can add the
full backing chain explicitly to qemu. This also includes allocation of
nodenames for the individual backing chain members.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_domain.c | 77 --
1 file changed, 75 insertions(+), 2 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 31/62] qemu: domain: Add field for storing node name for copy-on-read

[Ján Tomko]

On Mon, Aug 13, 2018 at 06:00:05PM +0200, Peter Krempa wrote:

The copy-on-read feature is expressed by adding a new node layer in
qemu when using -blockdev. Since we will keep these per-disk (as opposed
to per storage source) we need to store the appropriate node names in
the disk definition.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_domain.c| 11 +++
src/qemu/qemu_domain.h|  1 +
tests/qemustatusxml2xmldata/modern-in.xml |  3 +++
3 files changed, 15 insertions(+)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 30/62] qemu: command: Setup floppy drives via -device for blockdev

[Ján Tomko]

On Mon, Aug 13, 2018 at 06:00:04PM +0200, Peter Krempa wrote:

To allow referring to the drives via the QOM id we need to setup the
floppy drives with a proper ID. This means that -device should be used
for them.

There are the following quirks:
- FDC needs to be instantiated prior to any floppy device
- floppy drive specified via -device does not support 'bootindex'
   (hacked around by passing bootindexA=1 to the FDC)

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_command.c | 42 +++---
1 file changed, 31 insertions(+), 11 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 0/8] cpu: modularize the CPU map data file

[no-reply]

Hi,

This series was run against 'syntax-check' test by patchew.org, which failed, 
please find the details below:

Type: series
Message-id: 20180816121031.10902-1-berra...@redhat.com
Subject: [libvirt] [PATCH v2 0/8] cpu: modularize the CPU map data file

=== TEST SCRIPT BEGIN ===
#!/bin/bash
# Testing script will be invoked under the git checkout with
# HEAD pointing to a commit that has the patches applied on top of "base"
# branch
time bash -c './autogen.sh && make syntax-check'
=== TEST SCRIPT END ===

Updating bcb55ab053bc79561b55d0394490f4b64e0f2d01
>From https://github.com/patchew-project/libvirt
   f951277716..480d47cd7a  master -> master
 * [new tag]   patchew/20180816121031.10902-1-berra...@redhat.com 
-> patchew/20180816121031.10902-1-berra...@redhat.com
Switched to a new branch 'test'
e450f982d0 xml: report the filename (if any) when parsing files
bfa4d32033 cpu: split x86 map data into separate files
9eb6a91c8c cpu: split PPC64 map data into separate files
f3d5e0de5f cpu: move the CPU map data files into a src/cpu_map directory
1855c26bcf cpu: simplify failure cleanup paths
a51f445fa3 cpu: push more parsing logic into common code
47b0e0bf56 cpu: fix cleanup when signature parsing fails
b4928de106 cpu: allow include files for CPU definition

=== OUTPUT BEGIN ===
Updating submodules...
Submodule 'gnulib' (https://git.savannah.gnu.org/git/gnulib.git/) registered 
for path '.gnulib'
Submodule 'keycodemapdb' (https://gitlab.com/keycodemap/keycodemapdb.git) 
registered for path 'src/keycodemapdb'
Cloning into '/var/tmp/patchew-tester-tmp-o51rq8pj/src/.gnulib'...
fatal: unable to access 'https://git.savannah.gnu.org/git/gnulib.git/': The 
requested URL returned error: 502
fatal: clone of 'https://git.savannah.gnu.org/git/gnulib.git/' into submodule 
path '/var/tmp/patchew-tester-tmp-o51rq8pj/src/.gnulib' failed
Failed to clone '.gnulib'. Retry scheduled
Cloning into '/var/tmp/patchew-tester-tmp-o51rq8pj/src/src/keycodemapdb'...
Cloning into '/var/tmp/patchew-tester-tmp-o51rq8pj/src/.gnulib'...
Submodule path '.gnulib': checked out '68df637b5f1b5c10370f6981d2a43a5cf74368df'
Submodule path 'src/keycodemapdb': checked out 
'16e5b0787687d8904dad2c026107409eb9bfcb95'
Submodule path '.gnulib': checked out '68df637b5f1b5c10370f6981d2a43a5cf74368df'
Running bootstrap...
./bootstrap: Bootstrapping from checked-out libvirt sources...
./bootstrap: consider installing git-merge-changelog from gnulib
./bootstrap: getting gnulib files...
running: libtoolize --install --copy
libtoolize: putting auxiliary files in AC_CONFIG_AUX_DIR, 'build-aux'.
libtoolize: copying file 'build-aux/config.guess'
libtoolize: copying file 'build-aux/config.sub'
libtoolize: copying file 'build-aux/install-sh'
libtoolize: copying file 'build-aux/ltmain.sh'
libtoolize: putting macros in AC_CONFIG_MACRO_DIRS, 'm4'.
libtoolize: copying file 'm4/libtool.m4'
libtoolize: copying file 'm4/ltoptions.m4'
libtoolize: copying file 'm4/ltsugar.m4'
libtoolize: copying file 'm4/ltversion.m4'
libtoolize: copying file 'm4/lt~obsolete.m4'
./bootstrap: .gnulib/gnulib-tool--no-changelog   --aux-dir=build-aux   
--doc-base=doc   --lib=libgnu   --m4-base=m4/   --source-base=gnulib/lib/   
--tests-base=gnulib/tests   --local-dir=gnulib/local--lgpl=2 --with-tests 
--makefile-name=gnulib.mk --avoid=pt_chown --avoid=lock-tests   --libtool 
--import ...
Module list with included dependencies (indented):
absolute-header
  accept
accept-tests
alloca
alloca-opt
alloca-opt-tests
allocator
  areadlink
areadlink-tests
arpa_inet
arpa_inet-tests
assure
  autobuild
  base64
base64-tests
binary-io
binary-io-tests
  bind
bind-tests
  bitrotate
bitrotate-tests
btowc
btowc-tests
builtin-expect
  byteswap
byteswap-tests
  c-ctype
c-ctype-tests
  c-strcase
c-strcase-tests
  c-strcasestr
c-strcasestr-tests
  calloc-posix
  canonicalize-lgpl
canonicalize-lgpl-tests
careadlinkat
  chown
chown-tests
  clock-time
cloexec
cloexec-tests
  close
close-tests
  configmake
  connect
connect-tests
  count-leading-zeros
count-leading-zeros-tests
  count-one-bits
count-one-bits-tests
ctype
ctype-tests
  dirname-lgpl
dosname
double-slash-root
dup
dup-tests
dup2
dup2-tests
  environ
environ-tests
errno
errno-tests
error
  execinfo
exitfail
extensions
extern-inline
fatal-signal
  fclose
fclose-tests
  fcntl
  fcntl-h
fcntl-h-tests
fcntl-tests
fd-hook
  fdatasync
fdatasync-tests
fdopen
fdopen-tests
fflush
fflush-tests
  ffs
ffs-tests
  ffsl
ffsl-tests
fgetc-tests
filename
flexmember
float
float-tests
  fnmatch
fnmatch-tests
fpieee
fpucw
fpurge
fpurge-tests
fputc-tests
fread-tests
freading
freading-tests
fseek
fseek-tests
fseeko
fseeko-tests
fstat
fstat-tests
  

Re: [libvirt] [libvirt-glib][PATCH v2] Use new GObject define macros with private

[Michal Privoznik]

On 08/16/2018 12:24 PM, Andrea Bolognani wrote:
> On Thu, 2018-08-16 at 10:06 +0200, Michal Privoznik wrote:
> [...]
>> -GLIB2_REQUIRED=2.36.0
>> +GLIB2_REQUIRED=2.38.0
>>  AC_SUBST([GLIB2_REQUIRED]) dnl used in the .spec file
>>  GLIB2_TEST_REQUIRED=2.38.0
> 
> We can now get rid of GLIB2_TEST_REQUIRED. Would you mind sending
> a follow-up patch that does that?

Okay.

> 
> [...]
>>  static void 
>> gvir_config_capabilities_cpu_class_init(GVirConfigCapabilitiesCpuClass 
>> *klass)
>>  {
>> -g_type_class_add_private(klass, 
>> sizeof(GVirConfigCapabilitiesCpuPrivate));
>>  
> 
> Drop the empty line here.
> 
> [...]
>> @@ -47,7 +47,6 @@ static void 
>> gvir_config_domain_cpu_class_init(GVirConfigDomainCpuClass *klass)
>>  capabilities_class = GVIR_CONFIG_CAPABILITIES_CPU_CLASS(klass);
>>  capabilities_class->get_features = _gvir_config_domain_cpu_get_features;
>>  
> 
> Drop the empty line here.
> 
> [...]
>>  static void gvir_config_domain_power_management_class_init
>> -(GVirConfigDomainPowerManagementClass *klass)
>> +(GVirConfigDomainPowerManagementClass *klass G_GNUC_UNUSED)
> 
> Weird formatting here, maybe fix it while you're at it?

Okay.

> 
> [...]


> [...]
>> -#define gvir_output_stream_get_type _gvir_output_stream_get_type
>> -G_DEFINE_TYPE(GVirOutputStream, gvir_output_stream, G_TYPE_OUTPUT_STREAM);
>> -
>>  enum
>>  {
>>  PROP_0,
>> @@ -50,6 +47,9 @@ struct _GVirOutputStreamPrivate
>>  gsize count;
>>  };
>>  
>> +#define gvir_output_stream_get_type _gvir_output_stream_get_type
>> +G_DEFINE_TYPE_WITH_PRIVATE(GVirOutputStream, gvir_output_stream, 
>> G_TYPE_OUTPUT_STREAM);
>> +
> 
> Why did this move around? :D Update it in place please.

Becasue G_DEFINE_TYPE_WITH_PRIVATE needs the private data struct to
exist at the point where it is called:

libvirt-gobject-output-stream.c: In function '_gvir_output_stream_get_type':
libvirt-gobject-output-stream.c:35:28: error: invalid application of
'sizeof' to incomplete type 'GVirOutputStreamPrivate {aka struct
_GVirOutputStreamPrivate}'



> 
> With the above addressed
> 
>   Reviewed-by: Andrea Bolognani 
> 

Pushed, thanks.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 29/62] qemu: alias: Generate QDEV name of the block backend for disks

[Ján Tomko]

On Mon, Aug 13, 2018 at 06:00:03PM +0200, Peter Krempa wrote:

When we stop using -drive qemu stops reporting it in some of the monitor
commands. To allow referring the disk frontends and the corresponding


referring to


block backends we need to know these names. Unfortunately different
buses require different names.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_alias.c   | 86 +++--
src/qemu/qemu_alias.h   |  3 +-
src/qemu/qemu_hotplug.c |  2 +-
3 files changed, 65 insertions(+), 26 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 28/62] qemu: Add field to store QDEV path of a disk in private data

[Ján Tomko]

On Mon, Aug 13, 2018 at 06:00:02PM +0200, Peter Krempa wrote:

When using -blockdev you need to use the qdev path to refer to the disk
fronends. Add means for storing the path and getting it after restart.


frontends



Signed-off-by: Peter Krempa 
---
src/qemu/qemu_domain.c| 27 +++
src/qemu/qemu_domain.h|  2 ++
tests/qemustatusxml2xmldata/modern-in.xml |  3 +++
3 files changed, 32 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 032ad80baa..3058ceca79 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1065,6 +1065,7 @@ qemuDomainDiskPrivateDispose(void *obj)

VIR_FREE(priv->blockJobError);
virStorageSourceFree(priv->migrSource);
+VIR_FREE(priv->backendQomName);


Would look better after:

s/backendQomName/backendQOMName/


Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 1/3] vsh: Add API for printing tables.

[Daniel P . Berrangé]

On Thu, Aug 16, 2018 at 02:05:45PM +0200, Ján Tomko wrote:
> On Thu, Aug 16, 2018 at 12:56:24PM +0200, Simon Kobyda wrote:
> > It solves problems with alignment of columns. Width of each column
> > is calculated by its biggest cell. Should solve unicode bug.
> > In future, it may be implemented in virsh, virt-admin...
> > 
> > This API has 5 public functions:
> > - vshTableNew - adds new table and defines its header
> > - vshTableRowAppend - appends new row (for same number of columns as in
> > header)
> > - vshTablePrintToStdout
> > - vshTablePrintToString
> > - vshTableFree
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1574624
> > https://bugzilla.redhat.com/show_bug.cgi?id=1584630
> > 
> > Signed-off-by: Simon Kobyda 
> > ---
> > tools/Makefile.am |   4 +-
> > tools/vsh-table.c | 413 ++
> > tools/vsh-table.h |  42 +
> > 3 files changed, 458 insertions(+), 1 deletion(-)
> > create mode 100644 tools/vsh-table.c
> > create mode 100644 tools/vsh-table.h
> > 

> > +/**
> > + * vshTableGetColumnsWidths:
> > + * @table: table
> > + * @maxwidths: maximum count of characters for each columns
> > + * @widths: count of characters for each cell in the table
> > + *
> > + * Fill passed @maxwidths and @widths arrays with maximum number
> > + * of characters for columns and number of character per each
> > + * table cell, respectively.
> > + *
> > + * Handle unicode strings (user must have multibyte locale)
> > + */
> > +static int
> > +vshTableGetColumnsWidths(vshTablePtr table,
> > + size_t *maxwidths,
> > + size_t **widths,
> > + bool header)
> > +{
> > +int ret = -1;
> > +size_t i = 1;
> > +size_t j;
> > +size_t len;
> > +int tmp;
> > +wchar_t *wstr = NULL;
> > +size_t wstrlen;
> > +
> > +if (header)
> > +i = 0;
> > +else
> > +i = 1;
> > +for (; i < table->nrows; i++) {
> > +vshTableRowPtr row = table->rows[i];
> > +
> > +for (j = 0; j < row->ncells; j++) {
> 
> 
> > +/* strlen should return maximum possible length needed */
> > +wstrlen = strlen(row->cells[j]);
> > +VIR_FREE(wstr);
> > +if (VIR_ALLOC_N(wstr, wstrlen) < 0)
> > +goto cleanup;
> > +/* mbstowcs fails if machine is using singlebyte locale
> > + * and user tries to convert unicode(multibyte)
> > + * */
> > +if (mbstowcs(wstr, row->cells[j], wstrlen) ==
> > +(size_t) -1) {
> > +len = wstrlen;
> > +} else {
> > +tmp = wcswidth(wstr, wstrlen);
> > +if (tmp < 0)
> > +goto cleanup;
> > +len = (size_t)((unsigned)tmp);
> > +}
> 
> Whatever solution you use for converting a multi-byte string to a
> maximum count of characters, please put it in a separate function.
> That would make this function more readable.

Note that unfortnately libunistring is "GPLv2+ or LGPLv3+", so is not
compatible with libvirt.so

It is fine to use it from virsh since that's free to be GPLv2+ only,
but we mustn't link libunistring to libvirt.so.  So any helper func
would have to stay in the tools dir just for virsh/virt-admin


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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] Investigation and possible fix of 1361592 - apparmor profiles do not include backing files

[Povilas Kanapickas]

Hi,

I've looked into why apparmor profiles do not contain exceptions for
backing files of images which later leads to permission errors due to
apparmor containment. As of newest libvirt git master, only the first
level backing image is included, the subsequent images are omitted.

Below is my investigation of why this issue happens. It is based on
libvirt git master as of 2018-08-15 (e3e48d7cb82d58). The possible fix
is easy, but I don't have the background how to write tests for it, so
it's best that someone who knows the project well completes the fix

In my case I have the following file hierarchy:
 - image-master.qcow2 (has backing file image-backing-1.qcow2)
 - image-backing-1.qcow2 (has backing file image-backing-2.qcow2
 - image-backing-2.qcow2

The apparmor profiles are created by the virt-aa-helper tool. The
get_files function (src/security/virt-aa-helper.c:949) creates a list of
files that need to be accessed by the virtual machine. The call to
virDomainDiskDefForeachPath() is responsible for creating the list of
files required to access each disk given the disk metadata.

The disk->src argument contains the source file. The expected file
hierarchy would be this:

disk->src->path == path/to/image-master.qcow2
disk->src->backingStore->path == path/to/image-backing-1.qcow2
disk->src->backingStore->backingStore->path == path/to/image-backing-2.qcow2

Unfortunately only the first two levels are present and
disk->src->backingStore->backingStore points to a dummy object.

The backing store details are filled in virStorageFileGetMetadata()
call. It calls into virStorageFileGetMetadataRecurse
(src/util/virstoragefile.c:4789) which will collect metadata for a
single image and recurse into itself for backing files.

For us, the following part of virStorageFileGetMetadataRecurse is
important (simplified for brevity):

```
virStorageFileGetMetadataInternal(src, ..., );

if (src->backingStoreRaw) {
backingStore = ...

if (backingFormat == VIR_STORAGE_FILE_AUTO)
backingStore->format = VIR_STORAGE_FILE_RAW; [1]
else if (backingFormat == VIR_STORAGE_FILE_AUTO_SAFE)
backingStore->format = VIR_STORAGE_FILE_AUTO;
else
backingStore->format = backingFormat;

virStorageFileGetMetadataRecurse(backingStore, ...) [2]
}
```

The crux of the issue seems that the call to
virStorageFileGetMetadataInternal() for the image-master.qcow2 image
will set the `backingFormat` return value  to
VIR_STORAGE_FILE_AUTO. The code responsible is in qcowXGetBackingStore
(src/util/virstoragefile.c:491) called via
`fileTypeInfo[meta->format].getBackingStore(...)` at
src/util/virstoragefile.c:1042.

It backingFormat is then downgraded to VIR_STORAGE_FILE_RAW at
src/util/virstoragefile.c:4854 ([1] above). This causes the next recurse
call to
virStorageFileGetMetadataRecurse() ([2] above) to not investigate the
backing images at all in virStorageFileGetMetadataInternal() as
fileTypeInfo[VIR_STORAGE_FILE_RAW].getBackingStore will be NULL.

The possible solution is to return VIR_STORAGE_FILE_AUTO_SAFE from
qcowXGetBackingStore.  It probably does not make much sense to prevent
probing of qcow2 backing images as we probe the first level of backing
images anyway, so that return value only affect second and subsequent
levels of backing images. Looking into the implementation of
qcowXGetBackingStore it also does not seem that it performs any
operations that are unsafe by design.

Currently VIR_STORAGE_FILE_AUTO is used only in qedGetBackingStore and
it does not seem that probing of qcow images is unsafe enough

What do the developers think about this? Could someone complete the fix
with tests or advise me about the testing frameworks if there's not
enough time?

Thanks a lot!
Povilas

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 3/3] vsh: Added tests

[Ján Tomko]

On Thu, Aug 16, 2018 at 12:56:26PM +0200, Simon Kobyda wrote:

For now, there are 5 test cases
- testVshTableNew: Creating table with empty header
- testVshTableHeader: Printing table with/without header
- testVshTableRowAppend: Appending row with various number of cells.
 Only row with same number of cells as in header is accepted.
- testVshTableNewUnicode: Printing table with unicode characters.
 Checking correct alignment.
- testNTables: Create and print various types of tables - one column,
 one row table, table without content, standard table...

Signed-off-by: Simon Kobyda 
---
tests/Makefile.am|   8 ++
tests/vshtabletest.c | 247 +++
2 files changed, 255 insertions(+)
create mode 100644 tests/vshtabletest.c
+static int
+testVshTableNew(const void *opaque ATTRIBUTE_UNUSED)
+{
+int ret = 0;


no need for a 'ret' variable if you don't have a cleanup section.


+
+if (vshTableNew(NULL)) {
+fprintf(stderr, "expected failure when passing null to"
+"vshtablenew\n");


Missing space between 'to' and 'vshtablenew'


+ret = -1;
+}
+
+return ret;
+}
+
+static int
+testVshTableHeader(const void *opaque ATTRIBUTE_UNUSED)
+{
+int ret = 0;
+char *out;
+const char *exp = "\
+ 1   fedora28   running  \n\
+ 2   rhel7.5running  \n";
+const char *exp2 = "\
+ Id   Name   State\n\
+--\n\
+ 1fedora28   running  \n\
+ 2rhel7.5running  \n";


Please use one string literal per line in new code. That way you can align them.


+
+vshTablePtr table = vshTableNew("Id", "Name", "State",
+NULL); //to ask about return
+if (!table)
+goto cleanup;
+
+vshTableRowAppend(table, "1", "fedora28", "running", NULL);
+vshTableRowAppend(table, "2", "rhel7.5", "running",
+  NULL);
+


[...]


+static int
+testNTables(const void *opaque ATTRIBUTE_UNUSED)
+{
+int ret = 0;
+vshTablePtr table1;
+vshTablePtr table2;
+vshTablePtr table3;
+const char *exp1 = "\
+ Id   Name   Status   \n\
+--\n\
+ 1fedora28   running  \n\
+ 2rhel7.5running  \n";
+const char *exp2 = "\
+ Id   Name   Status  \n\
+-\n";
+const char *exp3 = "\
+ Id  \n\
+-\n\
+ 1   \n\
+ 2   \n\
+ 3   \n\
+ 4   \n";
+char *out1;
+char *out2;
+char *out3;
+
+table1 = vshTableNew("Id", "Name", "Status", NULL);
+if (!table1)
+goto cleanup;
+vshTableRowAppend(table1, "1", "fedora28", "running", NULL);
+vshTableRowAppend(table1, "2", "rhel7.5", "running", NULL);
+out1 = vshTablePrintToString(table1, true);
+
+table2 = vshTableNew("Id", "Name", "Status", NULL);
+if (!table2)
+goto cleanup;


out2 and out3 are unitialized if you jump to cleanup here.

Jano


+out2 = vshTablePrintToString(table2, true);
+
+table3 = vshTableNew("Id", NULL);


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 1/8] cpu: allow include files for CPU definition

[Daniel P . Berrangé]

Allow for syntax



to reference other files in the CPU database directory

Signed-off-by: Daniel P. Berrangé 
---
 src/cpu/cpu_map.c | 87 +--
 1 file changed, 84 insertions(+), 3 deletions(-)

diff --git a/src/cpu/cpu_map.c b/src/cpu/cpu_map.c
index d263eb8cdd..bcd3e55417 100644
--- a/src/cpu/cpu_map.c
+++ b/src/cpu/cpu_map.c
@@ -1,7 +1,7 @@
 /*
  * cpu_map.c: internal functions for handling CPU mapping configuration
  *
- * Copyright (C) 2009-2010 Red Hat, Inc.
+ * Copyright (C) 2009-2018 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -70,6 +70,84 @@ static int load(xmlXPathContextPtr ctxt,
 return ret;
 }
 
+static int
+cpuMapLoadInclude(const char *filename,
+  cpuMapLoadCallback cb,
+  void *data)
+{
+xmlDocPtr xml = NULL;
+xmlXPathContextPtr ctxt = NULL;
+int ret = -1;
+int element;
+char *mapfile;
+
+if (!(mapfile = virFileFindResource(filename,
+abs_topsrcdir "/src/cpu",
+PKGDATADIR)))
+return -1;
+
+VIR_DEBUG("Loading CPU map include from %s", mapfile);
+
+if (!(xml = virXMLParseFileCtxt(mapfile, )))
+goto cleanup;
+
+ctxt->node = xmlDocGetRootElement(xml);
+
+for (element = 0; element < CPU_MAP_ELEMENT_LAST; element++) {
+if (load(ctxt, element, cb, data) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("cannot parse CPU map '%s'"), mapfile);
+goto cleanup;
+}
+}
+
+ret = 0;
+
+ cleanup:
+xmlXPathFreeContext(ctxt);
+xmlFreeDoc(xml);
+VIR_FREE(mapfile);
+
+return ret;
+}
+
+
+static int
+loadIncludes(xmlXPathContextPtr ctxt,
+ cpuMapLoadCallback callback,
+ void *data)
+{
+int ret = -1;
+xmlNodePtr ctxt_node;
+xmlNodePtr *nodes = NULL;
+int n;
+size_t i;
+
+ctxt_node = ctxt->node;
+
+n = virXPathNodeSet("include", ctxt, );
+if (n < 0)
+goto cleanup;
+
+for (i = 0; i < n; i++) {
+char *filename = virXMLPropString(nodes[i], "filename");
+VIR_DEBUG("Finding CPU map include '%s'", filename);
+if (cpuMapLoadInclude(filename, callback, data) < 0) {
+VIR_FREE(filename);
+goto cleanup;
+}
+VIR_FREE(filename);
+}
+
+ret = 0;
+
+ cleanup:
+ctxt->node = ctxt_node;
+VIR_FREE(nodes);
+
+return ret;
+}
+
 
 int cpuMapLoad(const char *arch,
cpuMapLoadCallback cb,
@@ -88,7 +166,7 @@ int cpuMapLoad(const char *arch,
 PKGDATADIR)))
 return -1;
 
-VIR_DEBUG("Loading CPU map from %s", mapfile);
+VIR_DEBUG("Loading '%s' CPU map from %s", NULLSTR(arch), mapfile);
 
 if (arch == NULL) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -122,11 +200,14 @@ int cpuMapLoad(const char *arch,
 for (element = 0; element < CPU_MAP_ELEMENT_LAST; element++) {
 if (load(ctxt, element, cb, data) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("cannot parse CPU map for %s architecture"), 
arch);
+   _("cannot parse CPU map '%s'"), mapfile);
 goto cleanup;
 }
 }
 
+if (loadIncludes(ctxt, cb, data) < 0)
+goto cleanup;
+
 ret = 0;
 
  cleanup:
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 6/8] cpu: split PPC64 map data into separate files

[Daniel P . Berrangé]

Signed-off-by: Daniel P. Berrangé 
---
 src/cpu_map/Makefile.inc.am |  7 +
 src/cpu_map/index.xml   | 41 +
 src/cpu_map/ppc64_POWER6.xml|  6 +
 src/cpu_map/ppc64_POWER7.xml|  7 +
 src/cpu_map/ppc64_POWER8.xml|  8 ++
 src/cpu_map/ppc64_POWER9.xml|  6 +
 src/cpu_map/ppc64_POWERPC_e5500.xml |  6 +
 src/cpu_map/ppc64_POWERPC_e6500.xml |  6 +
 src/cpu_map/ppc64_vendors.xml   |  4 +++
 9 files changed, 57 insertions(+), 34 deletions(-)
 create mode 100644 src/cpu_map/ppc64_POWER6.xml
 create mode 100644 src/cpu_map/ppc64_POWER7.xml
 create mode 100644 src/cpu_map/ppc64_POWER8.xml
 create mode 100644 src/cpu_map/ppc64_POWER9.xml
 create mode 100644 src/cpu_map/ppc64_POWERPC_e5500.xml
 create mode 100644 src/cpu_map/ppc64_POWERPC_e6500.xml
 create mode 100644 src/cpu_map/ppc64_vendors.xml

diff --git a/src/cpu_map/Makefile.inc.am b/src/cpu_map/Makefile.inc.am
index 91728b9200..64453cc384 100644
--- a/src/cpu_map/Makefile.inc.am
+++ b/src/cpu_map/Makefile.inc.am
@@ -2,6 +2,13 @@
 cpumapdir = $(pkgdatadir)/cpu_map
 cpumap_DATA = \
cpu_map/index.xml \
+   cpu_map/ppc64_vendors.xml \
+   cpu_map/ppc64_POWER7.xml \
+   cpu_map/ppc64_POWER9.xml \
+   cpu_map/ppc64_POWERPC_e6500.xml \
+   cpu_map/ppc64_POWER6.xml \
+   cpu_map/ppc64_POWER8.xml \
+   cpu_map/ppc64_POWERPC_e5500.xml \
$(NULL)
 
 EXTRA_DIST += $(cpumap_DATA)
diff --git a/src/cpu_map/index.xml b/src/cpu_map/index.xml
index 9af190a579..ce4f0204b0 100644
--- a/src/cpu_map/index.xml
+++ b/src/cpu_map/index.xml
@@ -2340,43 +2340,16 @@
   
 
   
-
-
-
+
 
 
-
-  
-  
-
-
-
-  
-  
-  
-
-
-
-  
-  
-  
-  
-
-
-
-  
-  
-
+
+
+
+
 
 
-
-  
-  
-
-
-
-  
-  
-
+
+
   
 
diff --git a/src/cpu_map/ppc64_POWER6.xml b/src/cpu_map/ppc64_POWER6.xml
new file mode 100644
index 00..00e27495f4
--- /dev/null
+++ b/src/cpu_map/ppc64_POWER6.xml
@@ -0,0 +1,6 @@
+
+  
+
+
+  
+
diff --git a/src/cpu_map/ppc64_POWER7.xml b/src/cpu_map/ppc64_POWER7.xml
new file mode 100644
index 00..a071481805
--- /dev/null
+++ b/src/cpu_map/ppc64_POWER7.xml
@@ -0,0 +1,7 @@
+
+  
+
+
+
+  
+
diff --git a/src/cpu_map/ppc64_POWER8.xml b/src/cpu_map/ppc64_POWER8.xml
new file mode 100644
index 00..64d96fc4c4
--- /dev/null
+++ b/src/cpu_map/ppc64_POWER8.xml
@@ -0,0 +1,8 @@
+
+  
+
+
+
+
+  
+
diff --git a/src/cpu_map/ppc64_POWER9.xml b/src/cpu_map/ppc64_POWER9.xml
new file mode 100644
index 00..149fcde924
--- /dev/null
+++ b/src/cpu_map/ppc64_POWER9.xml
@@ -0,0 +1,6 @@
+
+  
+
+
+  
+
diff --git a/src/cpu_map/ppc64_POWERPC_e5500.xml 
b/src/cpu_map/ppc64_POWERPC_e5500.xml
new file mode 100644
index 00..3d64c8926c
--- /dev/null
+++ b/src/cpu_map/ppc64_POWERPC_e5500.xml
@@ -0,0 +1,6 @@
+
+  
+
+
+  
+
diff --git a/src/cpu_map/ppc64_POWERPC_e6500.xml 
b/src/cpu_map/ppc64_POWERPC_e6500.xml
new file mode 100644
index 00..b0d1006076
--- /dev/null
+++ b/src/cpu_map/ppc64_POWERPC_e6500.xml
@@ -0,0 +1,6 @@
+
+  
+
+  
+  
+
diff --git a/src/cpu_map/ppc64_vendors.xml b/src/cpu_map/ppc64_vendors.xml
new file mode 100644
index 00..52ad45c0bd
--- /dev/null
+++ b/src/cpu_map/ppc64_vendors.xml
@@ -0,0 +1,4 @@
+
+  
+  
+
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 3/8] cpu: push more parsing logic into common code

[Daniel P . Berrangé]

The x86 and ppc impls both duplicate some logic when parsing CPU
features. Change the callback signature so that this duplication can be
pushed up a level to common code.

Signed-off-by: Daniel P. Berrangé 
---
 src/cpu/cpu_map.c   |  98 +-
 src/cpu/cpu_map.h   |  22 ++---
 src/cpu/cpu_ppc64.c | 112 ++---
 src/cpu/cpu_x86.c   | 196 +---
 4 files changed, 143 insertions(+), 285 deletions(-)

diff --git a/src/cpu/cpu_map.c b/src/cpu/cpu_map.c
index bcd3e55417..400e6f1427 100644
--- a/src/cpu/cpu_map.c
+++ b/src/cpu/cpu_map.c
@@ -35,31 +35,47 @@
 
 VIR_LOG_INIT("cpu.cpu_map");
 
-VIR_ENUM_IMPL(cpuMapElement, CPU_MAP_ELEMENT_LAST,
-"vendor",
-"feature",
-"model")
-
-
-static int load(xmlXPathContextPtr ctxt,
-cpuMapElement element,
-cpuMapLoadCallback callback,
-void *data)
+static int
+loadData(const char *mapfile,
+ xmlXPathContextPtr ctxt,
+ const char *element,
+ cpuMapLoadCallback callback,
+ void *data)
 {
 int ret = -1;
 xmlNodePtr ctxt_node;
 xmlNodePtr *nodes = NULL;
 int n;
+size_t i;
+int rv;
 
 ctxt_node = ctxt->node;
 
-n = virXPathNodeSet(cpuMapElementTypeToString(element), ctxt, );
-if (n < 0)
+if ((n = virXPathNodeSet(element, ctxt, )) < 0)
 goto cleanup;
 
-if (n > 0 &&
-callback(element, ctxt, nodes, n, data) < 0)
+if (n > 0 && !callback) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Unexpected element '%s' in CPU map '%s'"), element, 
mapfile);
 goto cleanup;
+}
+
+for (i = 0; i < n; i++) {
+xmlNodePtr old = ctxt->node;
+char *name = virXMLPropString(nodes[i], "name");
+if (!name) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("cannot find %s name in CPU map '%s'"), element, 
mapfile);
+goto cleanup;
+}
+VIR_DEBUG("Load %s name %s", element, name);
+ctxt->node = nodes[i];
+rv = callback(ctxt, name, data);
+ctxt->node = old;
+VIR_FREE(name);
+if (rv < 0)
+goto cleanup;
+}
 
 ret = 0;
 
@@ -72,13 +88,14 @@ static int load(xmlXPathContextPtr ctxt,
 
 static int
 cpuMapLoadInclude(const char *filename,
-  cpuMapLoadCallback cb,
+  cpuMapLoadCallback vendorCB,
+  cpuMapLoadCallback featureCB,
+  cpuMapLoadCallback modelCB,
   void *data)
 {
 xmlDocPtr xml = NULL;
 xmlXPathContextPtr ctxt = NULL;
 int ret = -1;
-int element;
 char *mapfile;
 
 if (!(mapfile = virFileFindResource(filename,
@@ -93,13 +110,14 @@ cpuMapLoadInclude(const char *filename,
 
 ctxt->node = xmlDocGetRootElement(xml);
 
-for (element = 0; element < CPU_MAP_ELEMENT_LAST; element++) {
-if (load(ctxt, element, cb, data) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("cannot parse CPU map '%s'"), mapfile);
-goto cleanup;
-}
-}
+if (loadData(mapfile, ctxt, "vendor", vendorCB, data) < 0)
+goto cleanup;
+
+if (loadData(mapfile, ctxt, "feature", featureCB, data) < 0)
+goto cleanup;
+
+if (loadData(mapfile, ctxt, "model", modelCB, data) < 0)
+goto cleanup;
 
 ret = 0;
 
@@ -114,7 +132,9 @@ cpuMapLoadInclude(const char *filename,
 
 static int
 loadIncludes(xmlXPathContextPtr ctxt,
- cpuMapLoadCallback callback,
+ cpuMapLoadCallback vendorCB,
+ cpuMapLoadCallback featureCB,
+ cpuMapLoadCallback modelCB,
  void *data)
 {
 int ret = -1;
@@ -132,7 +152,7 @@ loadIncludes(xmlXPathContextPtr ctxt,
 for (i = 0; i < n; i++) {
 char *filename = virXMLPropString(nodes[i], "filename");
 VIR_DEBUG("Finding CPU map include '%s'", filename);
-if (cpuMapLoadInclude(filename, callback, data) < 0) {
+if (cpuMapLoadInclude(filename, vendorCB, featureCB, modelCB, data) < 
0) {
 VIR_FREE(filename);
 goto cleanup;
 }
@@ -150,7 +170,9 @@ loadIncludes(xmlXPathContextPtr ctxt,
 
 
 int cpuMapLoad(const char *arch,
-   cpuMapLoadCallback cb,
+   cpuMapLoadCallback vendorCB,
+   cpuMapLoadCallback featureCB,
+   cpuMapLoadCallback modelCB,
void *data)
 {
 xmlDocPtr xml = NULL;
@@ -158,7 +180,6 @@ int cpuMapLoad(const char *arch,
 virBuffer buf = VIR_BUFFER_INITIALIZER;
 char *xpath = NULL;
 int ret = -1;
-int element;
 char *mapfile;
 
 if (!(mapfile = virFileFindResource("cpu_map.xml",
@@ -174,12 +195,6 @@ int cpuMapLoad(const char *arch,
 goto cleanup;
 }
 
-if (cb == NULL) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   "%s", _("no callback 

[libvirt] [PATCH v2 4/8] cpu: simplify failure cleanup paths

[Daniel P . Berrangé]

Get rid of the separate 'error:' label, so all code paths jump straight
to the 'cleanup:' label.

Signed-off-by: Daniel P. Berrangé 
---
 src/cpu/cpu_ppc64.c | 38 
 src/cpu/cpu_x86.c   | 71 -
 2 files changed, 49 insertions(+), 60 deletions(-)

diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c
index 75da5b77d8..fcba7e9b37 100644
--- a/src/cpu/cpu_ppc64.c
+++ b/src/cpu/cpu_ppc64.c
@@ -288,27 +288,28 @@ ppc64VendorParse(xmlXPathContextPtr ctxt ATTRIBUTE_UNUSED,
 {
 struct ppc64_map *map = data;
 struct ppc64_vendor *vendor;
+int ret = -1;
 
 if (VIR_ALLOC(vendor) < 0)
 return -1;
 
 if (VIR_STRDUP(vendor->name, name) < 0)
-goto error;
+goto cleanup;
 
 if (ppc64VendorFind(map, vendor->name)) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("CPU vendor %s already defined"), vendor->name);
-goto error;
+goto cleanup;
 }
 
 if (VIR_APPEND_ELEMENT(map->vendors, map->nvendors, vendor) < 0)
-goto error;
+goto cleanup;
 
-return 0;
+ret = 0;
 
- error:
+ cleanup:
 ppc64VendorFree(vendor);
-return -1;
+return ret;
 }
 
 
@@ -327,15 +328,15 @@ ppc64ModelParse(xmlXPathContextPtr ctxt,
 int ret = -1;
 
 if (VIR_ALLOC(model) < 0)
-goto error;
+goto cleanup;
 
 if (VIR_STRDUP(model->name, name) < 0)
-goto error;
+goto cleanup;
 
 if (ppc64ModelFind(map, model->name)) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("CPU model %s already defined"), model->name);
-goto error;
+goto cleanup;
 }
 
 if (virXPathBoolean("boolean(./vendor)", ctxt)) {
@@ -344,14 +345,14 @@ ppc64ModelParse(xmlXPathContextPtr ctxt,
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Invalid vendor element in CPU model %s"),
model->name);
-goto error;
+goto cleanup;
 }
 
 if (!(model->vendor = ppc64VendorFind(map, vendor))) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unknown vendor %s referenced by CPU model %s"),
vendor, model->name);
-goto error;
+goto cleanup;
 }
 }
 
@@ -359,11 +360,11 @@ ppc64ModelParse(xmlXPathContextPtr ctxt,
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Missing PVR information for CPU model %s"),
model->name);
-goto error;
+goto cleanup;
 }
 
 if (VIR_ALLOC_N(model->data.pvr, n) < 0)
-goto error;
+goto cleanup;
 
 model->data.len = n;
 
@@ -374,7 +375,7 @@ ppc64ModelParse(xmlXPathContextPtr ctxt,
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Missing or invalid PVR value in CPU model %s"),
model->name);
-goto error;
+goto cleanup;
 }
 model->data.pvr[i].value = pvr;
 
@@ -382,24 +383,21 @@ ppc64ModelParse(xmlXPathContextPtr ctxt,
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Missing or invalid PVR mask in CPU model %s"),
model->name);
-goto error;
+goto cleanup;
 }
 model->data.pvr[i].mask = pvr;
 }
 
 if (VIR_APPEND_ELEMENT(map->models, map->nmodels, model) < 0)
-goto error;
+goto cleanup;
 
 ret = 0;
 
  cleanup:
+ppc64ModelFree(model);
 VIR_FREE(vendor);
 VIR_FREE(nodes);
 return ret;
-
- error:
-ppc64ModelFree(model);
-goto cleanup;
 }
 
 
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index 73af9d0885..8e4a3d0f77 100644
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -723,15 +723,15 @@ x86VendorParse(xmlXPathContextPtr ctxt,
 int ret = -1;
 
 if (VIR_ALLOC(vendor) < 0)
-goto error;
+goto cleanup;
 
 if (VIR_STRDUP(vendor->name, name) < 0)
-goto error;
+goto cleanup;
 
 if (x86VendorFind(map, vendor->name)) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("CPU vendor %s already defined"), vendor->name);
-goto error;
+goto cleanup;
 }
 
 string = virXPathString("string(@string)", ctxt);
@@ -739,24 +739,21 @@ x86VendorParse(xmlXPathContextPtr ctxt,
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Missing vendor string for CPU vendor %s"),
vendor->name);
-goto error;
+goto cleanup;
 }
 
 if (virCPUx86VendorToCPUID(string, >cpuid) < 0)
-goto error;
+goto cleanup;
 
 if (VIR_APPEND_ELEMENT(map->vendors, map->nvendors, vendor) < 0)
-goto error;
+goto cleanup;
 
 ret = 0;
 
  cleanup:
+x86VendorFree(vendor);
 VIR_FREE(string);
 return ret;
-
- 

[libvirt] [PATCH v2 8/8] xml: report the filename (if any) when parsing files

[Daniel P . Berrangé]

A generic "failed to parse xml document" message without telling us
which XML file failed is quite unhelpful.

Signed-off-by: Daniel P. Berrangé 
---
 src/util/virxml.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/util/virxml.c b/src/util/virxml.c
index a03a747e60..d1926f4605 100644
--- a/src/util/virxml.c
+++ b/src/util/virxml.c
@@ -847,7 +847,8 @@ virXMLParseHelper(int domcode,
 
 if (virGetLastErrorCode() == VIR_ERR_OK) {
 virGenericReportError(domcode, VIR_ERR_XML_ERROR,
-  "%s", _("failed to parse xml document"));
+  _("failed to parse xml document '%s'"),
+  filename ? filename : "[inline data]");
 }
 goto cleanup;
 }
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 0/8] cpu: modularize the CPU map data file

[Daniel P . Berrangé]

Currently we have a cpu_map.xml file that contains all the features and
CPU models for all architectures in one place. I frequently find myself
wondering about the differences between CPU models, but it is hard to
compare them as the list of features is huge.

With this patch series we end up with a large set of small files, one
per named CPU model, along with one for the feature and vendor
definitions

   cpu_map/index.xml
   cpu_map/ppc64_POWER6.xml
   cpu_map/ppc64_POWER7.xml
   cpu_map/ppc64_POWER8.xml
   cpu_map/ppc64_POWER9.xml
   cpu_map/ppc64_POWERPC_e5500.xml
   cpu_map/ppc64_POWERPC_e6500.xml
   cpu_map/ppc64_vendors.xml
   cpu_map/x86_486.xml
   cpu_map/x86_athlon.xml
   cpu_map/x86_Broadwell-IBRS.xml
   cpu_map/x86_Broadwell-noTSX-IBRS.xml
   cpu_map/x86_Broadwell-noTSX.xml
   cpu_map/x86_Broadwell.xml
   cpu_map/x86_Conroe.xml
   cpu_map/x86_core2duo.xml
   cpu_map/x86_coreduo.xml
   cpu_map/x86_cpu64-rhel5.xml
   cpu_map/x86_cpu64-rhel6.xml
   cpu_map/x86_EPYC-IBRS.xml
   cpu_map/x86_EPYC.xml
   cpu_map/x86_features.xml
   cpu_map/x86_Haswell-IBRS.xml
   cpu_map/x86_Haswell-noTSX-IBRS.xml
   cpu_map/x86_Haswell-noTSX.xml
   cpu_map/x86_Haswell.xml
   cpu_map/x86_IvyBridge-IBRS.xml
   cpu_map/x86_IvyBridge.xml
   cpu_map/x86_kvm32.xml
   cpu_map/x86_kvm64.xml
   cpu_map/x86_n270.xml
   cpu_map/x86_Nehalem-IBRS.xml
   cpu_map/x86_Nehalem.xml
   cpu_map/x86_Opteron_G1.xml
   cpu_map/x86_Opteron_G2.xml
   cpu_map/x86_Opteron_G3.xml
   cpu_map/x86_Opteron_G4.xml
   cpu_map/x86_Opteron_G5.xml
   cpu_map/x86_Penryn.xml
   cpu_map/x86_pentium2.xml
   cpu_map/x86_pentium3.xml
   cpu_map/x86_pentiumpro.xml
   cpu_map/x86_pentium.xml
   cpu_map/x86_phenom.xml
   cpu_map/x86_qemu32.xml
   cpu_map/x86_qemu64.xml
   cpu_map/x86_SandyBridge-IBRS.xml
   cpu_map/x86_SandyBridge.xml
   cpu_map/x86_Skylake-Client-IBRS.xml
   cpu_map/x86_Skylake-Client.xml
   cpu_map/x86_Skylake-Server-IBRS.xml
   cpu_map/x86_Skylake-Server.xml
   cpu_map/x86_vendors.xml
   cpu_map/x86_Westmere-IBRS.xml
   cpu_map/x86_Westmere.xml

The main cpu_map/index.xml file is now just a list of 
statements to pull in the individual files

Now we can easily see the differences in each model:

$ diff cpu_map/x86_Broadwell.xml cpu_map/x86_Skylake-Client.xml
2,3c2,3
<   
< 
---
>   
> 
5a6
> 
8a10
> 
18a21
> 
30a34
> 
42a47
> 
56a62
> 
57a64
> 
58a66,67
> 
> 

Changed in v2:

 - Moved all XML files into a new src/cpu_map/ directory
 - Simplify the goto labels for error code paths.
 - Code style fixes

Daniel P. Berrangé (8):
  cpu: allow include files for CPU definition
  cpu: fix cleanup when signature parsing fails
  cpu: push more parsing logic into common code
  cpu: simplify failure cleanup paths
  cpu: move the CPU map data files into a src/cpu_map directory
  cpu: split PPC64 map data into separate files
  cpu: split x86 map data into separate files
  xml: report the filename (if any) when parsing files

 libvirt.spec.in  |2 +-
 mingw-libvirt.spec.in|4 +-
 src/Makefile.am  |7 +-
 src/cpu/cpu_map.c|  161 +-
 src/cpu/cpu_map.h|   22 +-
 src/cpu/cpu_map.xml  | 2382 --
 src/cpu/cpu_ppc64.c  |  142 +-
 src/cpu/cpu_x86.c|  255 +--
 src/cpu_map/Makefile.inc.am  |   61 +
 src/cpu_map/index.xml|   75 +
 src/cpu_map/ppc64_POWER6.xml |6 +
 src/cpu_map/ppc64_POWER7.xml |7 +
 src/cpu_map/ppc64_POWER8.xml |8 +
 src/cpu_map/ppc64_POWER9.xml |6 +
 src/cpu_map/ppc64_POWERPC_e5500.xml  |6 +
 src/cpu_map/ppc64_POWERPC_e6500.xml  |6 +
 src/cpu_map/ppc64_vendors.xml|4 +
 src/cpu_map/x86_486.xml  |7 +
 src/cpu_map/x86_Broadwell-IBRS.xml   |   61 +
 src/cpu_map/x86_Broadwell-noTSX-IBRS.xml |   59 +
 src/cpu_map/x86_Broadwell-noTSX.xml  |   58 +
 src/cpu_map/x86_Broadwell.xml|   60 +
 src/cpu_map/x86_Conroe.xml   |   33 +
 src/cpu_map/x86_EPYC-IBRS.xml|   73 +
 src/cpu_map/x86_EPYC.xml |   72 +
 src/cpu_map/x86_Haswell-IBRS.xml |   57 +
 src/cpu_map/x86_Haswell-noTSX-IBRS.xml   |   55 +
 src/cpu_map/x86_Haswell-noTSX.xml|   54 +
 src/cpu_map/x86_Haswell.xml  |   56 +
 src/cpu_map/x86_IvyBridge-IBRS.xml   |   51 +
 src/cpu_map/x86_IvyBridge.xml|   50 +
 src/cpu_map/x86_Nehalem-IBRS.xml |   38 +
 src/cpu_map/x86_Nehalem.xml  |   37 +
 src/cpu_map/x86_Opteron_G1.xml   |   31 +
 src/cpu_map/x86_Opteron_G2.xml   |   35 +
 src/cpu_map/x86_Opteron_G3.xml   |   40 +
 src/cpu_map/x86_Opteron_G4.xml   |   50 +
 

[libvirt] [PATCH v2 2/8] cpu: fix cleanup when signature parsing fails

[Daniel P . Berrangé]

Two pieces of code accidentally jumped to the wrong label when they
failed causing incorrect cleanup, returning a partially initialized
CPU model struct.

Signed-off-by: Daniel P. Berrangé 
---
 src/cpu/cpu_x86.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index 809da94117..a045a8280c 100644
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -1242,7 +1242,7 @@ x86ModelParse(xmlXPathContextPtr ctxt,
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Invalid CPU signature family in model %s"),
model->name);
-goto cleanup;
+goto error;
 }
 
 rc = virXPathUInt("string(./signature/@model)", ctxt, );
@@ -1250,7 +1250,7 @@ x86ModelParse(xmlXPathContextPtr ctxt,
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Invalid CPU signature model in model %s"),
model->name);
-goto cleanup;
+goto error;
 }
 
 model->signature = x86MakeSignature(sigFamily, sigModel, 0);
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 5/8] cpu: move the CPU map data files into a src/cpu_map directory

[Daniel P . Berrangé]

In preparation for splitting up the CPU map data file, move it into a
dedicated directory of its own.

Signed-off-by: Daniel P. Berrangé 
---
 libvirt.spec.in|  2 +-
 mingw-libvirt.spec.in  |  4 ++--
 src/Makefile.am|  7 +--
 src/cpu/cpu_map.c  | 10 +-
 src/cpu_map/Makefile.inc.am|  7 +++
 src/{cpu/cpu_map.xml => cpu_map/index.xml} |  0
 6 files changed, 16 insertions(+), 14 deletions(-)
 create mode 100644 src/cpu_map/Makefile.inc.am
 rename src/{cpu/cpu_map.xml => cpu_map/index.xml} (100%)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 883c8a49e7..09f654b2ec 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1854,7 +1854,7 @@ exit 0
 %{_datadir}/libvirt/schemas/storagepool.rng
 %{_datadir}/libvirt/schemas/storagevol.rng
 
-%{_datadir}/libvirt/cpu_map.xml
+%{_datadir}/libvirt/cpu_map/*.xml
 
 %{_datadir}/libvirt/test-screenshot.png
 
diff --git a/mingw-libvirt.spec.in b/mingw-libvirt.spec.in
index cc1e619927..b28e40f7f7 100644
--- a/mingw-libvirt.spec.in
+++ b/mingw-libvirt.spec.in
@@ -260,7 +260,7 @@ rm -rf 
$RPM_BUILD_ROOT%{mingw64_libexecdir}/libvirt-guests.sh
 %{mingw32_datadir}/libvirt/api/libvirt-qemu-api.xml
 %{mingw32_datadir}/libvirt/api/libvirt-admin-api.xml
 
-%{mingw32_datadir}/libvirt/cpu_map.xml
+%{mingw32_datadir}/libvirt/cpu_map/*.xml
 
 %{mingw32_datadir}/libvirt/test-screenshot.png
 
@@ -347,7 +347,7 @@ rm -rf 
$RPM_BUILD_ROOT%{mingw64_libexecdir}/libvirt-guests.sh
 %{mingw64_datadir}/libvirt/api/libvirt-qemu-api.xml
 %{mingw64_datadir}/libvirt/api/libvirt-admin-api.xml
 
-%{mingw64_datadir}/libvirt/cpu_map.xml
+%{mingw64_datadir}/libvirt/cpu_map/*.xml
 
 %{mingw64_datadir}/libvirt/test-screenshot.png
 
diff --git a/src/Makefile.am b/src/Makefile.am
index db8c8ebd1a..2a3ed0d42d 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -100,6 +100,7 @@ man7_MANS =
 include util/Makefile.inc.am
 include conf/Makefile.inc.am
 include cpu/Makefile.inc.am
+include cpu_map/Makefile.inc.am
 include security/Makefile.inc.am
 include access/Makefile.inc.am
 include logging/Makefile.inc.am
@@ -364,12 +365,6 @@ check-local: check-protocol check-symfile check-symsorting 
\
 .PHONY: check-protocol $(PROTOCOL_STRUCTS:structs=struct)
 
 
-
-
-pkgdata_DATA = cpu/cpu_map.xml
-
-EXTRA_DIST +=  $(pkgdata_DATA)
-
 #
 #
 # Build up list of libvirt.la source files based on configure conditions
diff --git a/src/cpu/cpu_map.c b/src/cpu/cpu_map.c
index 400e6f1427..2079767df8 100644
--- a/src/cpu/cpu_map.c
+++ b/src/cpu/cpu_map.c
@@ -99,8 +99,8 @@ cpuMapLoadInclude(const char *filename,
 char *mapfile;
 
 if (!(mapfile = virFileFindResource(filename,
-abs_topsrcdir "/src/cpu",
-PKGDATADIR)))
+abs_topsrcdir "/src/cpu_map",
+PKGDATADIR "/cpu_map")))
 return -1;
 
 VIR_DEBUG("Loading CPU map include from %s", mapfile);
@@ -182,9 +182,9 @@ int cpuMapLoad(const char *arch,
 int ret = -1;
 char *mapfile;
 
-if (!(mapfile = virFileFindResource("cpu_map.xml",
-abs_topsrcdir "/src/cpu",
-PKGDATADIR)))
+if (!(mapfile = virFileFindResource("index.xml",
+abs_topsrcdir "/src/cpu_map",
+PKGDATADIR "/cpu_map")))
 return -1;
 
 VIR_DEBUG("Loading '%s' CPU map from %s", NULLSTR(arch), mapfile);
diff --git a/src/cpu_map/Makefile.inc.am b/src/cpu_map/Makefile.inc.am
new file mode 100644
index 00..91728b9200
--- /dev/null
+++ b/src/cpu_map/Makefile.inc.am
@@ -0,0 +1,7 @@
+
+cpumapdir = $(pkgdatadir)/cpu_map
+cpumap_DATA = \
+   cpu_map/index.xml \
+   $(NULL)
+
+EXTRA_DIST += $(cpumap_DATA)
diff --git a/src/cpu/cpu_map.xml b/src/cpu_map/index.xml
similarity index 100%
rename from src/cpu/cpu_map.xml
rename to src/cpu_map/index.xml
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 1/3] vsh: Add API for printing tables.

[Ján Tomko]

On Thu, Aug 16, 2018 at 12:56:24PM +0200, Simon Kobyda wrote:

It solves problems with alignment of columns. Width of each column
is calculated by its biggest cell. Should solve unicode bug.
In future, it may be implemented in virsh, virt-admin...

This API has 5 public functions:
- vshTableNew - adds new table and defines its header
- vshTableRowAppend - appends new row (for same number of columns as in
header)
- vshTablePrintToStdout
- vshTablePrintToString
- vshTableFree

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

Signed-off-by: Simon Kobyda 
---
tools/Makefile.am |   4 +-
tools/vsh-table.c | 413 ++
tools/vsh-table.h |  42 +
3 files changed, 458 insertions(+), 1 deletion(-)
create mode 100644 tools/vsh-table.c
create mode 100644 tools/vsh-table.h

diff --git a/tools/Makefile.am b/tools/Makefile.am
index 1452d984a0..f069167acc 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -144,7 +144,9 @@ libvirt_shell_la_LIBADD = \
$(READLINE_LIBS) \
../gnulib/lib/libgnu.la \
$(NULL)
-libvirt_shell_la_SOURCES = vsh.c vsh.h
+libvirt_shell_la_SOURCES = \
+   vsh.c vsh.h \
+   vsh-table.c vsh-table.h

virt_host_validate_SOURCES = \
virt-host-validate.c \
diff --git a/tools/vsh-table.c b/tools/vsh-table.c
new file mode 100644
index 00..8842e4e4fd
--- /dev/null
+++ b/tools/vsh-table.c
@@ -0,0 +1,413 @@
+/*
+ * vsh-table.c: table printing helper
+ *
+ * Copyright (C) 2018 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ *
+ * Authors:
+ *   Simon Kobyda 
+ *
+ */
+
+#include 
+#include "vsh-table.h"
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "viralloc.h"
+#include "virbuffer.h"
+#include "virstring.h"
+#include "virsh-util.h"
+
+typedef void (*vshPrintCB)(vshControl *ctl, const char *fmt, ...);
+
+struct _vshTableRow {
+char **cells;
+size_t ncells;
+};
+
+struct _vshTable {
+vshTableRowPtr *rows;
+size_t nrows;
+};
+
+static void
+vshTableRowFree(vshTableRowPtr row)
+{
+size_t i;
+
+if (!row)
+return;
+
+for (i = 0; i < row->ncells; i++)
+VIR_FREE(row->cells[i]);
+
+VIR_FREE(row->cells);
+VIR_FREE(row);
+}
+
+void
+vshTableFree(vshTablePtr table)
+{
+size_t i;
+
+if (!table)
+return;
+
+for (i = 0; i < table->nrows; i++)
+vshTableRowFree(table->rows[i]);
+VIR_FREE(table->rows);
+VIR_FREE(table);
+}
+
+/**
+ * vshTableRowNew:
+ * @arg: the first argument.
+ * @ap: list of variadic arguments
+ *
+ * Create a new row in the table. Each argument passed
+ * represents a cell in the row.
+ * Return: pointer to vshTableRowPtr row or NULL.
+ */
+static vshTableRowPtr
+vshTableRowNew(const char *arg, va_list ap)
+{
+vshTableRowPtr row = NULL;
+char *tmp = NULL;
+
+if (!arg) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+_("Table row cannot be empty"));
+goto error;
+}
+
+if (VIR_ALLOC(row) < 0)
+goto error;
+
+while (arg) {
+if (VIR_STRDUP(tmp, arg) < 0)
+goto error;
+
+if (VIR_APPEND_ELEMENT(row->cells, row->ncells, tmp) < 0)
+goto error;
+
+arg = va_arg(ap, const char *);
+}
+
+return row;
+
+ error:
+vshTableRowFree(row);
+return NULL;
+}
+
+/**
+ * vshTableNew:
+ * @arg: List of column names (NULL terminated)
+ *
+ * Create a new table.
+ *
+ * Returns: pointer to table or NULL.
+ */
+vshTablePtr
+vshTableNew(const char *arg, ...)
+{
+vshTablePtr table;
+vshTableRowPtr header = NULL;
+va_list ap;
+
+if (VIR_ALLOC(table) < 0)
+goto error;
+
+va_start(ap, arg);
+header = vshTableRowNew(arg, ap);
+va_end(ap);
+
+if (!header)
+goto error;
+
+if (VIR_APPEND_ELEMENT(table->rows, table->nrows, header) < 0)
+goto error;
+
+return table;
+ error:
+vshTableRowFree(header);
+vshTableFree(table);
+return NULL;
+}
+
+/**
+ * vshTableRowAppend:
+ * @table: table to append to
+ * @arg: cells of the row (NULL terminated)
+ *
+ * Append new row into the @table. The number of cells in the row has
+ * to be equal to the number of cells in the table header.
+ *
+ * 

Re: [libvirt] [RFC PATCH 01/17] qemu: setup shared memory without explicit numa configuration

[Daniel P . Berrangé]

On Thu, Aug 16, 2018 at 07:32:57AM -0400, John Ferlan wrote:
> 
> 
> On 08/16/2018 06:31 AM, Marc-André Lureau wrote:
> > Hi
> > 
> > On Thu, Aug 16, 2018 at 4:35 AM, John Ferlan  wrote:
> >>
> >>
> >> On 07/13/2018 09:28 AM, marcandre.lur...@redhat.com wrote:
> >>> From: Marc-André Lureau 
> >>>
> >>> When a domain is configured with 'shared' memory backing:
> >>>
> >>>   
> >>> 
> >>>   
> >>>
> >>> But no explicit NUMA configuration, let's configure a shared memory
> >>> backend associated with default -numa.
> >>>
> >>> Signed-off-by: Marc-André Lureau 
> >>> ---
> >>>  src/qemu/qemu_command.c   | 100 --
> >>>  .../fd-memory-no-numa-topology.args   |   4 +
> >>>  2 files changed, 73 insertions(+), 31 deletions(-)
> >>>
> >>
> >> NUMA, memory backends, and hugepages - not in my wheelhouse of
> >> knowledge. Hopefully Michal and/or Pavel will take a look!
> >>
> >> Is it possible someone may not want this type of thing to happen? Is
> > 
> > I assume someone that sets 'shared' memory mode may consider this as a bug 
> > fix.
> > 
> >> there an upside or downside to this?  What happens "today" when not
> > 
> > You get non-shared memory
> > 
> 
> So today someone asks for "shared" and then end up with "non-shared"? I
> don't think that's apparent from the "access" description in:
> 
> https://libvirt.org/formatdomain.html#elementsMemoryBacking
> 
> Then again, not in my wheelhouse of knowledge, so maybe that's just one
> of those givens. Of course that perhaps goes to your first answer of
> this being a "bug fix". Not something that's apparent from the existing
> documentation or commit description though. This probably should have
> been it's own separate patch and not included in this series.

If we can't honour the "shared" request, we should make sure libvirt
reports an error and aborts startup.

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 27/62] qemu: Use proper backingIndex when reporting stats for backing chain

[Ján Tomko]

On Mon, Aug 13, 2018 at 06:00:01PM +0200, Peter Krempa wrote:

Use the index stored in virStorageSource struct rather than
recalculating it. Currently we'd report proper numbers but that will
change with blockdev.

Signed-off-by: Peter Krempa 
---
src/qemu/qemu_driver.c | 13 +
1 file changed, 5 insertions(+), 8 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC PATCH 01/17] qemu: setup shared memory without explicit numa configuration

[John Ferlan]

On 08/16/2018 06:31 AM, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Aug 16, 2018 at 4:35 AM, John Ferlan  wrote:
>>
>>
>> On 07/13/2018 09:28 AM, marcandre.lur...@redhat.com wrote:
>>> From: Marc-André Lureau 
>>>
>>> When a domain is configured with 'shared' memory backing:
>>>
>>>   
>>> 
>>>   
>>>
>>> But no explicit NUMA configuration, let's configure a shared memory
>>> backend associated with default -numa.
>>>
>>> Signed-off-by: Marc-André Lureau 
>>> ---
>>>  src/qemu/qemu_command.c   | 100 --
>>>  .../fd-memory-no-numa-topology.args   |   4 +
>>>  2 files changed, 73 insertions(+), 31 deletions(-)
>>>
>>
>> NUMA, memory backends, and hugepages - not in my wheelhouse of
>> knowledge. Hopefully Michal and/or Pavel will take a look!
>>
>> Is it possible someone may not want this type of thing to happen? Is
> 
> I assume someone that sets 'shared' memory mode may consider this as a bug 
> fix.
> 
>> there an upside or downside to this?  What happens "today" when not
> 
> You get non-shared memory
> 

So today someone asks for "shared" and then end up with "non-shared"? I
don't think that's apparent from the "access" description in:

https://libvirt.org/formatdomain.html#elementsMemoryBacking

Then again, not in my wheelhouse of knowledge, so maybe that's just one
of those givens. Of course that perhaps goes to your first answer of
this being a "bug fix". Not something that's apparent from the existing
documentation or commit description though. This probably should have
been it's own separate patch and not included in this series.

>> generated? And of course, what about migration concerns about
>> unconditionally doing this for some target migration?
> 
> True, this will break migration though if the target uses
> numa/memory-backend-file.
> 
> What do you suggest?
> 

This needs to be configurable as we cannot break w/ migration. I'm not
quite sure how we document this other than as Dan suggests that if
someone wants NUMA, then they'll configure things properly. If this
'shared' ends up as 'non-shared' because NUMA isn't configured, then we
should indicate that. If the adding some property to the element to
indicate desire of 'shared' via usage of some (local) backing store
which means the guest probably isn't very migrate-able, then so bit it.

>>
>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>>> index 44ae8dcef7..f1235099b2 100644
>>> --- a/src/qemu/qemu_command.c
>>> +++ b/src/qemu/qemu_command.c
>>> @@ -3254,26 +3254,21 @@ qemuBuildMemoryBackendProps(virJSONValuePtr 
>>> *backendProps,
>>>
>>>
>>>  static int
>>> -qemuBuildMemoryCellBackendStr(virDomainDefPtr def,
>>> -  virQEMUDriverConfigPtr cfg,
>>> -  size_t cell,
>>> -  qemuDomainObjPrivatePtr priv,
>>> -  virBufferPtr buf)
>>> +qemuBuildMemoryBackendStr(virDomainDefPtr def,
>>> +  virQEMUDriverConfigPtr cfg,
>>> +  const char *alias,
>>
>> So the one "concern" I'd have here is some time in the future the @mem
>> gets allocated and handled like a real device eventually calling
>> virDomainDeviceInfoClear and that'd be a problem for the passed const
>> char * string.  Some future person's problem I guess!
>>
>>> +  int targetNode,
>>> +  unsigned long long memsize,
>>> +  qemuDomainObjPrivatePtr priv,
>>> +  virBufferPtr buf)
>>
>> As much as a long name is a pain, is this more of a :
>>
>> qemuBuildMemorySharedDefaultBackendStr
> 
> Why?
> 

nm, I think when first reading for some reason I had this "separate"
from the CellBackend call.

[...]

>>> +implicit = true;
>>> +} else {
>>> +ret = 0;
>>> +goto cleanup;
>>> +}
>>> +}
>>
>> So if ncells == 0 && def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_SHARED,
>> then we return 0 without doing the subsequent code? Is that expected? Is
>> there something done later that may be necessary, needed, or assumed.
> 
> No, before the patch, virDomainNumaGetNodeCount() is checked before
> calling qemuBuildNumaArgStr(). Now it is handled inside in case
> ncells==0 && def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_SHARED.
> 

Ah, yes - I missed the check when looking at the changes

-if (virDomainNumaGetNodeCount(def->numa) &&
-qemuBuildNumaArgStr(cfg, def, cmd, priv) < 0)

[...]

>>> diff --git a/tests/qemuxml2argvdata/fd-memory-no-numa-topology.args 
>>> b/tests/qemuxml2argvdata/fd-memory-no-numa-topology.args
>>> index bd88daaa3b..400fb39cc6 100644
>>> --- a/tests/qemuxml2argvdata/fd-memory-no-numa-topology.args
>>> +++ b/tests/qemuxml2argvdata/fd-memory-no-numa-topology.args
>>> @@ -11,6 +11,10 @@ QEMU_AUDIO_DRV=none \
>>>  -m 14336 \
>>>  -mem-prealloc \
>>>  -smp 8,sockets=8,cores=1,threads=1 \
>>> +-object 

Re: [libvirt] [PATCHv2 26/62] conf: Allow formatting and parsing of 'index' for disk source image

[Ján Tomko]

On Mon, Aug 13, 2018 at 06:00:00PM +0200, Peter Krempa wrote:

Similarly to backing store indexes which will become stable eventually
we need also to be able to format and store in the status XML for later
use the index for the top level of the backing chain.

Add XML formatter, parser, schema and docs.

Signed-off-by: Peter Krempa 
---
docs/formatdomain.html.in   |  7 ++-
docs/schemas/domaincommon.rng   | 19 +++
src/conf/domain_conf.c  | 21 +
.../qemuxml2argvdata/disk-backing-chains-index.xml  | 12 ++--
.../disk-backing-chains-index-active.xml| 12 ++--
5 files changed, 54 insertions(+), 17 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 1/3] vsh: Add API for printing tables.

[Daniel P . Berrangé]

On Thu, Aug 16, 2018 at 12:56:24PM +0200, Simon Kobyda wrote:
> It solves problems with alignment of columns. Width of each column
> is calculated by its biggest cell. Should solve unicode bug.
> In future, it may be implemented in virsh, virt-admin...
> 
> This API has 5 public functions:
> - vshTableNew - adds new table and defines its header
> - vshTableRowAppend - appends new row (for same number of columns as in
> header)
> - vshTablePrintToStdout
> - vshTablePrintToString
> - vshTableFree
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1574624
> https://bugzilla.redhat.com/show_bug.cgi?id=1584630
> 
> Signed-off-by: Simon Kobyda 
> ---
>  tools/Makefile.am |   4 +-
>  tools/vsh-table.c | 413 ++
>  tools/vsh-table.h |  42 +
>  3 files changed, 458 insertions(+), 1 deletion(-)
>  create mode 100644 tools/vsh-table.c
>  create mode 100644 tools/vsh-table.h
> 


> +/**
> + * vshTableGetColumnsWidths:
> + * @table: table
> + * @maxwidths: maximum count of characters for each columns
> + * @widths: count of characters for each cell in the table
> + *
> + * Fill passed @maxwidths and @widths arrays with maximum number
> + * of characters for columns and number of character per each
> + * table cell, respectively.
> + *
> + * Handle unicode strings (user must have multibyte locale)
> + */
> +static int
> +vshTableGetColumnsWidths(vshTablePtr table,
> + size_t *maxwidths,
> + size_t **widths,
> + bool header)
> +{
> +int ret = -1;
> +size_t i = 1;
> +size_t j;
> +size_t len;
> +int tmp;
> +wchar_t *wstr = NULL;
> +size_t wstrlen;
> +
> +if (header)
> +i = 0;
> +else
> +i = 1;
> +for (; i < table->nrows; i++) {
> +vshTableRowPtr row = table->rows[i];
> +
> +for (j = 0; j < row->ncells; j++) {
> +/* strlen should return maximum possible length needed */
> +wstrlen = strlen(row->cells[j]);
> +VIR_FREE(wstr);
> +if (VIR_ALLOC_N(wstr, wstrlen) < 0)
> +goto cleanup;
> +/* mbstowcs fails if machine is using singlebyte locale
> + * and user tries to convert unicode(multibyte)
> + * */
> +if (mbstowcs(wstr, row->cells[j], wstrlen) ==
> +(size_t) -1) {
> +len = wstrlen;
> +} else {
> +tmp = wcswidth(wstr, wstrlen);
> +if (tmp < 0)
> +goto cleanup;
> +len = (size_t)((unsigned)tmp);
> +}
> +widths[i][j] = len;
> +if (len > maxwidths[j])
> +maxwidths[j] = len;

After asking around I have found the right solution that we need to use
for measuring string width.  mbstowcs()/wcswidth() will get the answer
wrong wrt zero-width characters, combining characters, non-printable
characters, etc. We need to use the libunistring library:

  
https://www.gnu.org/software/libunistring/manual/libunistring.html#uniwidth_002eh


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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 0/5] Extend apparmor rules for libvirt 4.6

[Christian Ehrhardt]

Ok, with acks of last year and new ones in and no other feedback nor any
Freeze atm I'm pushing these changes any minute.
The qemu-smb related one will be dropped, the others pushed with the latest
cleanups as discussed in the per-patch threads.
Thanks everybody for your participation!

On Tue, Aug 14, 2018 at 8:18 AM Christian Ehrhardt <
christian.ehrha...@canonical.com> wrote:

> Hi,
> this is a summary of things I had to touch recently for libvirt 4.6.
> The first two patches are re-submissions and modifications of last
> year which were never totally challenged, but also not pushed.
>
> The first was lost in a discussion about virt-aa-helper, whicih eventually
> turned out to be clear that it could not help in that case.
>   -
> https://www.redhat.com/archives/libvir-list/2017-February/msg01598.html
>   - https://www.redhat.com/archives/libvir-list/2017-March/msg00052.html
>
> The second even got a few Acks, but neither made it into upstream yet.
> Parts of it where introduced already, in
>   7edcbd02 apparmor: allow libvirt to send term signal to unconfined
>   b482925c apparmor: support ptrace checks
> But there are still signals blocked with those rules, so I resubmit the
> remaining bit. Also I added the Acks to the resubmission.
>
> The third change came in recently via various bug reports which I
> finally wanted to adress - e.g. for ceph lib or smb. If we later on spot
> more cases that have predictable safe paths under /tmp we can add those.
>
> Finally the fifth change was triggered by me testing libvirt 4.6 in
> various conditions. Some of them were in containers, and the new libvirt
> behavior to carry more mount points into the qemu namespace triggers the
> need to rewrite the existing mount-moving rules that we added last year.
>
> *Updates in V2*
> - added Acks to path #1
> - split former patch #3 into #3/#4 to discuss /tmp access and qemu-smd
>   individually
> - rewrote reasoning and concerns as well as TODOs to improve later in
>   regard to the /tmp related commits #3/#4
> - Updated the rule since the trailing {,/} is not needed after **
>
> Christian Ehrhardt (5):
>   apparmor: allow openGraphicsFD for virt manager >1.4
>   apparmor: add mediation rules for unconfined guests
>   apparmor: allow expected /tmp access patterns
>   apparmor: allow qemu-smb access in /tmp
>   apparmor: allow to preserve /dev mountpoints into qemu namespaces
>
>  examples/apparmor/libvirt-qemu  | 20 
>  examples/apparmor/usr.sbin.libvirtd | 24 +---
>  2 files changed, 33 insertions(+), 11 deletions(-)
>
> --
> 2.17.1
>
>

-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 4/5] apparmor: allow qemu-smb access in /tmp

[Christian Ehrhardt]

On Wed, Aug 15, 2018 at 7:35 PM Jamie Strandboge 
wrote:

> On Tue, 2018-08-14 at 08:18 +0200, Christian Ehrhardt wrote:
> > The samba feature of qemu will place the samba config file in
> > /tmp/qemu-smb..
> >
> > But at least it has a predictable path identifying qemu-smb feature
> > itself by an infix in the path.
> >
> > This is a compromise of security and usability as the "owner"
> > restriction
> > will not protect guests among each other.
> > Therefore the rule added makes the feature usable, but does not allow
> > cross guest protection.
> >
> > Core issue is, that it is currently impossible to predict the PID
> > which would
> > follow "qemu-smb-", but long term, once the samba feature would be
> > exposed in
> > guest XML we'd prefer a virt-aa-helper based solution that can render
> > the
> > samba rule on demand and with a custom PID into the per guest
> > profile.
> >
> > But the same is true for manual user overrides for this feature as
> > well,
> > they can neither predict the PID, nor have a local include per-guest.
> > Thereby
> > punting this to the user to add the rule later will not make it
> > safer, but
> > only less usable.
>
> While the facts are true, there is something omitted and I come to a
> different conclusion. It is true that the pid is unpredictable, but
> with the 'owner /{,var/}tmp/**/ r,' rule, it is easy for a confined
> process to find the directories. Also, the rule 'owner /tmp/qemu-
> smb.*/{,**} rw,' (below) allows writing /tmp/qemu-smb.*/ so symlink
> attacks are possible by a rogue VM against other VMs. Rogue VMs could
> also use the directory in coordinated file sharing (but I mention this
> only for completeness, not as an objection, since there are other rules
> in the policy that 'overlap' and allow this sort of thing).
>
> It is definitely a usability improvement to include the rule, but I
> disagree that it isn't safer without it-- your addition applies to all
> libvirt/apparmor users, many of whom will not use the smb feature that
> isn't supported by the domain xml. Now, you could argue that users that
> never use the smb feature aren't affected by the proposed global rule
> (which is true), but omitting this patch, users can add the glob rule
> to the per-VM site policy in /etc/apparmor.d/libvirt/libvirt- for
> only the VMs they need it. This might be useful if they, for example,
> have one trusted VM that uses the smb feature and other untrusted VMs
> that do not. With the global rule, the untrusted VMs have access by
> default.
>
> These concerns are somewhat contrived and I'm ambivalent about the
> addition, but it bothers me that we are adding a rule for a use case
> that isn't directly supported by libvirt. +0 as is.
>

Thanks for laying out the security concerns on this in detail.
I agree, this might be too much for a general rule and has to stay out of
the abstraction.

We'd have to wait for the feature to be supported by libvirt to fully
support it.
Even then we might not have the pid at the time the rule is created since
qemu was not yet spawned at that point.


> If you can demonstrate that qemu guards against symlink attacks on the
> dir and is otherwise safe from attack (eg, open(..., O_CREAT|O_EXCL)
> followed by unlink(...)), etc, etc then I could be persuaded to give a
> +1.
>

I can't, therefore I'll abandon this change for now.


>
> > Signed-off-by: Christian Ehrhardt 
> > ---
> >  examples/apparmor/libvirt-qemu | 5 +
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/examples/apparmor/libvirt-qemu
> > b/examples/apparmor/libvirt-qemu
> > index 6971d3db03..350b13b824 100644
> > --- a/examples/apparmor/libvirt-qemu
> > +++ b/examples/apparmor/libvirt-qemu
> > @@ -191,6 +191,11 @@
> ># want more unique paths per rule.
> >/{,var/}tmp/ r,
> >owner /{,var/}tmp/**/ r,
> > +  # allow qemu smb feature specific path with write access
> > +  # TODO: This is a compromise between security and usability - once
> > e.g. samba
> > +  # would be expressed in libvirt XML it should be added on demand
> > via
> > +  # virt-aa-helper instead.
> > +  owner /tmp/qemu-smb.*/{,**} rw,
> >
> ># for file-posix getting limits since 9103f1ce
> >/sys/devices/**/block/*/queue/max_segments r,
> --
> Jamie Strandboge | http://www.canonical.com



-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v3 2/3] virsh: Implement new table API for virsh list

[Simon Kobyda]

Instead of printing it straight in virsh, it creates table struct
which is filled with header and rows(domains). It allows us to know
more about table before printing to calculate alignment right.

Signed-off-by: Simon Kobyda 
---
 tests/virshtest.c| 14 ++--
 tools/virsh-domain-monitor.c | 43 
 2 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/tests/virshtest.c b/tests/virshtest.c
index 94548a82d1..10cd0d356b 100644
--- a/tests/virshtest.c
+++ b/tests/virshtest.c
@@ -98,9 +98,9 @@ static int testCompareListDefault(const void *data 
ATTRIBUTE_UNUSED)
 {
   const char *const argv[] = { VIRSH_DEFAULT, "list", NULL };
   const char *exp = "\
- IdName   State\n\
-\n\
- 1 test   running\n\
+ Id   Name   State\n\
+--\n\
+ 1test   running  \n\
 \n";
   return testCompareOutputLit(exp, NULL, argv);
 }
@@ -109,10 +109,10 @@ static int testCompareListCustom(const void *data 
ATTRIBUTE_UNUSED)
 {
   const char *const argv[] = { VIRSH_CUSTOM, "list", NULL };
   const char *exp = "\
- IdName   State\n\
-\n\
- 1 fv0running\n\
- 2 fc4running\n\
+ Id   Name   State\n\
+--\n\
+ 1fv0running  \n\
+ 2fc4running  \n\
 \n";
   return testCompareOutputLit(exp, NULL, argv);
 }
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index b9b4f9739b..adc5bb1a7a 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -39,6 +39,7 @@
 #include "virmacaddr.h"
 #include "virxml.h"
 #include "virstring.h"
+#include "vsh-table.h"
 
 VIR_ENUM_DECL(virshDomainIOError)
 VIR_ENUM_IMPL(virshDomainIOError,
@@ -1901,6 +1902,7 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
 char id_buf[INT_BUFSIZE_BOUND(unsigned int)];
 unsigned int id;
 unsigned int flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE;
+vshTablePtr table = NULL;
 
 /* construct filter flags */
 if (vshCommandOptBool(cmd, "inactive") ||
@@ -1940,15 +1942,12 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
 /* print table header in legacy mode */
 if (optTable) {
 if (optTitle)
-vshPrintExtra(ctl, " %-5s %-30s %-10s %-20s\n%s\n",
-  _("Id"), _("Name"), _("State"), _("Title"),
-  "-"
-  "-");
+table = vshTableNew("Id", "Name", "State", "Title", NULL);
 else
-vshPrintExtra(ctl, " %-5s %-30s %s\n%s\n",
-  _("Id"), _("Name"), _("State"),
-  "-"
-  "---");
+table = vshTableNew("Id", "Name", "State", NULL);
+
+if (!table)
+goto cleanup;
 }
 
 for (i = 0; i < list->ndomains; i++) {
@@ -1973,20 +1972,22 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
 if (optTitle) {
 if (!(title = virshGetDomainDescription(ctl, dom, true, 0)))
 goto cleanup;
-
-vshPrint(ctl, " %-5s %-30s %-10s %-20s\n", id_buf,
- virDomainGetName(dom),
- state == -2 ? _("saved")
- : virshDomainStateToString(state),
- title);
-
+if (vshTableRowAppend(table, id_buf,
+  virDomainGetName(dom),
+  state == -2 ? _("saved")
+  : virshDomainStateToString(state),
+  title, NULL) < 0)
+goto cleanup;
 VIR_FREE(title);
 } else {
-vshPrint(ctl, " %-5s %-30s %s\n", id_buf,
- virDomainGetName(dom),
- state == -2 ? _("saved")
- : virshDomainStateToString(state));
+if (vshTableRowAppend(table, id_buf,
+  virDomainGetName(dom),
+  state == -2 ? _("saved")
+  : virshDomainStateToString(state),
+  NULL) < 0)
+goto cleanup;
 }
+
 } else if (optUUID && optName) {
 if (virDomainGetUUIDString(dom, uuid) < 0) {
 vshError(ctl, "%s", _("Failed to get domain's UUID"));
@@ -2004,8 +2005,12 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
 }
 }
 
+if (optTable)
+vshTablePrintToStdout(table, ctl);
+
 ret = true;
  cleanup:
+vshTableFree(table);
 

[libvirt] [PATCH v3 0/3] vsh: Introduce new API for printing tables

[Simon Kobyda]

Created new API for priting tables, mainly to solve alignment problems.
Implemented these test to virsh list. In the future, API may be
everywhere in virsh and virt-admin.
Also wrote basic tests for the new API, and corrected tests in virshtest
which are influenced by implementation of the API in virsh list.

Changes in v3:
- changed encoding of 3/3 patch, otherwise it cannot be applied

Changes in v2:
- added tests
- fixed alignment for unicode character which span more spaces
- moved ncolumns check to vshTableRowAppend
- changed arguments for functions vshTablePrint, vshTablePrintToStdout,
vshTablePrintToString

Simon Kobyda (3):
  vsh: Add API for printing tables.
  virsh: Implement new table API for virsh list
  vsh: Added tests

 tests/Makefile.am|   8 +
 tests/virshtest.c|  14 +-
 tests/vshtabletest.c | 247 +
 tools/Makefile.am|   4 +-
 tools/virsh-domain-monitor.c |  43 ++--
 tools/vsh-table.c| 413 +++
 tools/vsh-table.h|  42 
 7 files changed, 744 insertions(+), 27 deletions(-)
 create mode 100644 tests/vshtabletest.c
 create mode 100644 tools/vsh-table.c
 create mode 100644 tools/vsh-table.h

-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v3 1/3] vsh: Add API for printing tables.

[Simon Kobyda]

It solves problems with alignment of columns. Width of each column
is calculated by its biggest cell. Should solve unicode bug.
In future, it may be implemented in virsh, virt-admin...

This API has 5 public functions:
- vshTableNew - adds new table and defines its header
- vshTableRowAppend - appends new row (for same number of columns as in
header)
- vshTablePrintToStdout
- vshTablePrintToString
- vshTableFree

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

Signed-off-by: Simon Kobyda 
---
 tools/Makefile.am |   4 +-
 tools/vsh-table.c | 413 ++
 tools/vsh-table.h |  42 +
 3 files changed, 458 insertions(+), 1 deletion(-)
 create mode 100644 tools/vsh-table.c
 create mode 100644 tools/vsh-table.h

diff --git a/tools/Makefile.am b/tools/Makefile.am
index 1452d984a0..f069167acc 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -144,7 +144,9 @@ libvirt_shell_la_LIBADD = \
$(READLINE_LIBS) \
../gnulib/lib/libgnu.la \
$(NULL)
-libvirt_shell_la_SOURCES = vsh.c vsh.h
+libvirt_shell_la_SOURCES = \
+   vsh.c vsh.h \
+   vsh-table.c vsh-table.h
 
 virt_host_validate_SOURCES = \
virt-host-validate.c \
diff --git a/tools/vsh-table.c b/tools/vsh-table.c
new file mode 100644
index 00..8842e4e4fd
--- /dev/null
+++ b/tools/vsh-table.c
@@ -0,0 +1,413 @@
+/*
+ * vsh-table.c: table printing helper
+ *
+ * Copyright (C) 2018 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ *
+ * Authors:
+ *   Simon Kobyda 
+ *
+ */
+
+#include 
+#include "vsh-table.h"
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "viralloc.h"
+#include "virbuffer.h"
+#include "virstring.h"
+#include "virsh-util.h"
+
+typedef void (*vshPrintCB)(vshControl *ctl, const char *fmt, ...);
+
+struct _vshTableRow {
+char **cells;
+size_t ncells;
+};
+
+struct _vshTable {
+vshTableRowPtr *rows;
+size_t nrows;
+};
+
+static void
+vshTableRowFree(vshTableRowPtr row)
+{
+size_t i;
+
+if (!row)
+return;
+
+for (i = 0; i < row->ncells; i++)
+VIR_FREE(row->cells[i]);
+
+VIR_FREE(row->cells);
+VIR_FREE(row);
+}
+
+void
+vshTableFree(vshTablePtr table)
+{
+size_t i;
+
+if (!table)
+return;
+
+for (i = 0; i < table->nrows; i++)
+vshTableRowFree(table->rows[i]);
+VIR_FREE(table->rows);
+VIR_FREE(table);
+}
+
+/**
+ * vshTableRowNew:
+ * @arg: the first argument.
+ * @ap: list of variadic arguments
+ *
+ * Create a new row in the table. Each argument passed
+ * represents a cell in the row.
+ * Return: pointer to vshTableRowPtr row or NULL.
+ */
+static vshTableRowPtr
+vshTableRowNew(const char *arg, va_list ap)
+{
+vshTableRowPtr row = NULL;
+char *tmp = NULL;
+
+if (!arg) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+_("Table row cannot be empty"));
+goto error;
+}
+
+if (VIR_ALLOC(row) < 0)
+goto error;
+
+while (arg) {
+if (VIR_STRDUP(tmp, arg) < 0)
+goto error;
+
+if (VIR_APPEND_ELEMENT(row->cells, row->ncells, tmp) < 0)
+goto error;
+
+arg = va_arg(ap, const char *);
+}
+
+return row;
+
+ error:
+vshTableRowFree(row);
+return NULL;
+}
+
+/**
+ * vshTableNew:
+ * @arg: List of column names (NULL terminated)
+ *
+ * Create a new table.
+ *
+ * Returns: pointer to table or NULL.
+ */
+vshTablePtr
+vshTableNew(const char *arg, ...)
+{
+vshTablePtr table;
+vshTableRowPtr header = NULL;
+va_list ap;
+
+if (VIR_ALLOC(table) < 0)
+goto error;
+
+va_start(ap, arg);
+header = vshTableRowNew(arg, ap);
+va_end(ap);
+
+if (!header)
+goto error;
+
+if (VIR_APPEND_ELEMENT(table->rows, table->nrows, header) < 0)
+goto error;
+
+return table;
+ error:
+vshTableRowFree(header);
+vshTableFree(table);
+return NULL;
+}
+
+/**
+ * vshTableRowAppend:
+ * @table: table to append to
+ * @arg: cells of the row (NULL terminated)
+ *
+ * Append new row into the @table. The number of cells in the row has
+ * to be equal to the number of cells in the table header.
+ *
+ * Returns: 0 if succeeded, -1 if failed.
+ */
+int

[libvirt] [PATCH v3 3/3] vsh: Added tests

[Simon Kobyda]

For now, there are 5 test cases
- testVshTableNew: Creating table with empty header
- testVshTableHeader: Printing table with/without header
- testVshTableRowAppend: Appending row with various number of cells.
  Only row with same number of cells as in header is accepted.
- testVshTableNewUnicode: Printing table with unicode characters.
  Checking correct alignment.
- testNTables: Create and print various types of tables - one column,
  one row table, table without content, standard table...

Signed-off-by: Simon Kobyda 
---
 tests/Makefile.am|   8 ++
 tests/vshtabletest.c | 247 +++
 2 files changed, 255 insertions(+)
 create mode 100644 tests/vshtabletest.c

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 21a6c823d9..136fe16f71 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -206,6 +206,7 @@ test_programs = virshtest sockettest \
virhostdevtest \
virnetdevtest \
virtypedparamtest \
+   vshtabletest \
$(NULL)
 
 test_libraries = libshunload.la \
@@ -938,6 +939,13 @@ metadatatest_SOURCES = \
testutils.c testutils.h
 metadatatest_LDADD = $(LDADDS) $(LIBXML_LIBS)
 
+vshtabletest_SOURCES = \
+   vshtabletest.c \
+   testutils.c testutils.h
+vshtabletest_LDADD = \
+   $(LDADDS) \
+   ../tools/libvirt_shell.la
+
 virshtest_SOURCES = \
virshtest.c \
testutils.c testutils.h
diff --git a/tests/vshtabletest.c b/tests/vshtabletest.c
new file mode 100644
index 00..b41a205761
--- /dev/null
+++ b/tests/vshtabletest.c
@@ -0,0 +1,247 @@
+/*
+ * Copyright (C) 2018 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ */
+
+#include 
+
+#include 
+#include 
+#include 
+
+#include "internal.h"
+#include "testutils.h"
+#include "viralloc.h"
+#include "../tools/vsh-table.h"
+
+static int
+testVshTableNew(const void *opaque ATTRIBUTE_UNUSED)
+{
+int ret = 0;
+
+if (vshTableNew(NULL)) {
+fprintf(stderr, "expected failure when passing null to"
+"vshtablenew\n");
+ret = -1;
+}
+
+return ret;
+}
+
+static int
+testVshTableHeader(const void *opaque ATTRIBUTE_UNUSED)
+{
+int ret = 0;
+char *out;
+const char *exp = "\
+ 1   fedora28   running  \n\
+ 2   rhel7.5running  \n";
+const char *exp2 = "\
+ Id   Name   State\n\
+--\n\
+ 1fedora28   running  \n\
+ 2rhel7.5running  \n";
+
+vshTablePtr table = vshTableNew("Id", "Name", "State",
+NULL); //to ask about return
+if (!table)
+goto cleanup;
+
+vshTableRowAppend(table, "1", "fedora28", "running", NULL);
+vshTableRowAppend(table, "2", "rhel7.5", "running",
+  NULL);
+
+out = vshTablePrintToString(table, false);
+if (virTestCompareToString(exp, out) < 0)
+ret = -1;
+
+VIR_FREE(out);
+out = vshTablePrintToString(table, true);
+if (virTestCompareToString(exp2, out) < 0)
+ret = -1;
+
+ cleanup:
+VIR_FREE(out);
+vshTableFree(table);
+return ret;
+}
+
+static int
+testVshTableNewUnicode(const void *opaque ATTRIBUTE_UNUSED)
+{
+
+int ret = 0;
+char *out;
+
+char *locale = setlocale(LC_CTYPE, NULL);
+if (!setlocale(LC_CTYPE, "en_US.UTF-8"))
+return EXIT_AM_SKIP;
+
+const char *exp = "\
+ Id   名稱  государство  \n\
+-\n\
+ 1fedora28  running  \n\
+ 2rhel7.5   running  \n";
+vshTablePtr table;
+
+table = vshTableNew("Id", "名稱", "государство", NULL);
+if (!table)
+goto cleanup;
+
+vshTableRowAppend(table, "1", "fedora28", "running", NULL);
+vshTableRowAppend(table, "2", "rhel7.5", "running",
+  NULL);
+
+out = vshTablePrintToString(table, true);
+if (virTestCompareToString(exp, out) < 0)
+ret = -1;
+
+ cleanup:
+setlocale(LC_CTYPE, locale);
+VIR_FREE(out);
+vshTableFree(table);
+return ret;
+}
+
+static int
+testVshTableRowAppend(const void *opaque ATTRIBUTE_UNUSED)
+{
+int ret = 0;
+
+vshTablePtr table = vshTableNew("Id", "Name", NULL);
+if (!table)
+goto cleanup;
+
+if (vshTableRowAppend(table, NULL) >= 0) {
+fprintf(stderr, "Appending 

Re: [libvirt] [RFC PATCH 01/17] qemu: setup shared memory without explicit numa configuration

[Daniel P . Berrangé]

On Fri, Jul 13, 2018 at 03:28:08PM +0200, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau 
> 
> When a domain is configured with 'shared' memory backing:
> 
>   
> 
>   
> 
> But no explicit NUMA configuration, let's configure a shared memory
> backend associated with default -numa.


> diff --git a/tests/qemuxml2argvdata/fd-memory-no-numa-topology.args 
> b/tests/qemuxml2argvdata/fd-memory-no-numa-topology.args
> index bd88daaa3b..400fb39cc6 100644
> --- a/tests/qemuxml2argvdata/fd-memory-no-numa-topology.args
> +++ b/tests/qemuxml2argvdata/fd-memory-no-numa-topology.args
> @@ -11,6 +11,10 @@ QEMU_AUDIO_DRV=none \
>  -m 14336 \
>  -mem-prealloc \
>  -smp 8,sockets=8,cores=1,threads=1 \
> +-object memory-backend-file,id=ram-node,\
> +mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-instance-0092/ram-node,\
> +share=yes,size=15032385536 \
> +-numa node,nodeid=0,memdev=ram-node \

I'm not at all convinced it is safe todo this. We've been burnt in the
past by adding  use of memory-backend objects causing migration to break


  commit f309db1f4d51009bad0d32e12efc75530b66836b
  Author: Michal Privoznik 
  Date:   Thu Dec 18 12:36:48 2014 +0100

qemu: Create memory-backend-{ram,file} iff needed

Libvirt BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1175397
QEMU BZ:https://bugzilla.redhat.com/show_bug.cgi?id=1170093

This change doesn't really feel like it is required either. If the
user wants NUMA, then the XML can just be written to request a NUMA
topology with a single node.  Better to be explicit in the XML rather
than silently adding things as a side effect

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 3/4] qemu: Fix probing of AMD SEV support

[Erik Skultety]

So the procedure to detect SEV support works like this:
1) we detect that sev-guest is among the QOM types and set the cap flag
2) we probe the monitor for SEV support
- this is tricky, because QEMU with compiled SEV support will always
report -object sev-guest and query-sev-capabilities command, that
however doesn't mean SEV is supported
3) depending on what the monitor returned, we either keep or clear the
capability flag for SEV

Commit a349c6c21c6 added an explicit check for "GenericError" in the
monitor reply to prevent libvirtd to spam logs about missing
'query-sev-capabilities' command. At the same time though, it returned
success in this case which means that we didn't clear the capability
flag afterwards and happily formatted SEV into qemuCaps. Therefore,
adjust all the relevant callers to handle -1 on errors, 0 on SEV being
unsupported and 1 on SEV being supported.

Signed-off-by: Erik Skultety 
---
 src/qemu/qemu_capabilities.c | 15 +++
 src/qemu/qemu_monitor_json.c | 20 
 tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml |  1 -
 3 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index c17d26801e..fc46a380f6 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2767,18 +2767,20 @@ virQEMUCapsProbeQMPGICCapabilities(virQEMUCapsPtr 
qemuCaps,
 }
 
 
+/* Returns -1 on error, 0 if SEV is not supported, 1 if SEV is supported */
 static int
 virQEMUCapsProbeQMPSEVCapabilities(virQEMUCapsPtr qemuCaps,
qemuMonitorPtr mon)
 {
+int rc = -1;
 virSEVCapability *caps = NULL;
 
-if (qemuMonitorGetSEVCapabilities(mon, ) < 0)
-return -1;
+if ((rc = qemuMonitorGetSEVCapabilities(mon, )) <= 0)
+return rc;
 
 virSEVCapabilitiesFree(qemuCaps->sevCapabilities);
 qemuCaps->sevCapabilities = caps;
-return 0;
+return rc;
 }
 
 
@@ -4188,7 +4190,12 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
 
 /* Probe for SEV capabilities */
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) {
-if (virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon) < 0)
+int rc = virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon);
+
+if (rc < 0)
+goto cleanup;
+
+if (rc == 0)
 virQEMUCapsClear(qemuCaps, QEMU_CAPS_SEV_GUEST);
 }
 
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 3f99f39120..ed6caf4c2f 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -6435,6 +6435,20 @@ qemuMonitorJSONGetGICCapabilities(qemuMonitorPtr mon,
 }
 
 
+/**
+ * qemuMonitorJSONGetSEVCapabilities:
+ * @mon: qemu monitor object
+ * @capabilities: pointer to pointer to a SEV capability structure to be filled
+ *
+ * This function queries and fills in AMD's SEV platform-specific data.
+ * Note that from QEMU's POV both -object sev-guest and query-sev-capabilities
+ * can be present even if SEV is not available, which basically leaves us with
+ * checking for JSON "GenericError" in order to differentiate between
+ * compiled-in support and actual SEV support on the platform.
+ *
+ * Returns -1 on error, 0 if SEV is not supported, and 1 if SEV is supported on
+ * the platform.
+ */
 int
 qemuMonitorJSONGetSEVCapabilities(qemuMonitorPtr mon,
   virSEVCapability **capabilities)
@@ -6458,8 +6472,7 @@ qemuMonitorJSONGetSEVCapabilities(qemuMonitorPtr mon,
 if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
 goto cleanup;
 
-/* Both -object sev-guest and query-sev-capabilities can be present
- * even if SEV is not available */
+/* QEMU has only compiled-in support of SEV */
 if (qemuMonitorJSONHasError(reply, "GenericError")) {
 ret = 0;
 goto cleanup;
@@ -6511,8 +6524,7 @@ qemuMonitorJSONGetSEVCapabilities(qemuMonitorPtr mon,
 capability->cbitpos = cbitpos;
 capability->reduced_phys_bits = reduced_phys_bits;
 VIR_STEAL_PTR(*capabilities, capability);
-ret = 0;
-
+ret = 1;
  cleanup:
 virJSONValueFree(cmd);
 virJSONValueFree(reply);
diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml
index c8da1c5696..a9e8fe2dab 100644
--- a/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml
@@ -211,7 +211,6 @@
   
   
   
-  
   
   
   
-- 
2.14.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 4/4] qemu: caps: Format SEV platform data into qemuCaps cache

[Erik Skultety]

Since we're not saving the platform-specific data into a cache, we're
not going to populate the structure, which in turn will cause a crash
upon calling virNodeGetSEVInfo because of a NULL pointer dereference.
Ultimately, we should start caching this data along with host-specific
capabilities like NUMA and SELinux stuff into a separate cache, but for
the time being, this is a semi-proper fix for a potential crash.

Backtrace (requires libvirtd restart to load qemu caps from cache):
#0 qemuGetSEVInfoToParams
#1 qemuNodeGetSEVInfo
#2 virNodeGetSEVInfo
#3 remoteDispatchNodeGetSevInfo
#4 remoteDispatchNodeGetSevInfoHelper
#5 virNetServerProgramDispatchCall
#6 virNetServerProgramDispatch
#7 virNetServerProcessMsg
#8 virNetServerHandleJob
#9 virThreadPoolWorker
#10 virThreadHelper

https: //bugzilla.redhat.com/show_bug.cgi?id=1612009
Signed-off-by: Erik Skultety 
Acked-by: Peter Krempa 
---
 src/qemu/qemu_capabilities.c  | 100 ++
 tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml |   5 +-
 tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml |   6 ++
 3 files changed, 110 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index fc46a380f6..9edb4ca4d4 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -1570,6 +1570,25 @@ virQEMUCapsHostCPUDataClear(virQEMUCapsHostCPUDataPtr 
cpuData)
 }


+static int
+virQEMUCapsSEVInfoCopy(virSEVCapabilityPtr *dst,
+   virSEVCapabilityPtr src)
+{
+VIR_AUTOPTR(virSEVCapability) tmp = NULL;
+
+if (VIR_ALLOC(tmp) < 0 ||
+VIR_STRDUP(tmp->pdh, src->pdh) < 0 ||
+VIR_STRDUP(tmp->cert_chain, src->cert_chain) < 0)
+return -1;
+
+tmp->cbitpos = src->cbitpos;
+tmp->reduced_phys_bits = src->reduced_phys_bits;
+
+VIR_STEAL_PTR(*dst, tmp);
+return 0;
+}
+
+
 virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps)
 {
 virQEMUCapsPtr ret = virQEMUCapsNew();
@@ -1632,6 +1651,11 @@ virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr 
qemuCaps)
 for (i = 0; i < qemuCaps->ngicCapabilities; i++)
 ret->gicCapabilities[i] = qemuCaps->gicCapabilities[i];

+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST) &&
+virQEMUCapsSEVInfoCopy(>sevCapabilities,
+   qemuCaps->sevCapabilities) < 0)
+goto error;
+
 return ret;

  error:
@@ -3344,6 +3368,58 @@ virQEMUCapsCachePrivFree(void *privData)
 }


+static int
+virQEMUCapsParseSEVInfo(virQEMUCapsPtr qemuCaps, xmlXPathContextPtr ctxt)
+{
+VIR_AUTOPTR(virSEVCapability) sev = NULL;
+
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST))
+return 0;
+
+if (virXPathBoolean("boolean(./sev)", ctxt) == 0) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("missing SEV platform data in QEMU "
+ "capabilities cache"));
+return -1;
+}
+
+if (VIR_ALLOC(sev) < 0)
+return -1;
+
+if (virXPathUInt("string(./sev/cbitpos)", ctxt, >cbitpos) < 0) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("missing or malformed SEV cbitpos information "
+ "in QEMU capabilities cache"));
+return -1;
+}
+
+if (virXPathUInt("string(./sev/reducedPhysBits)", ctxt,
+ >reduced_phys_bits) < 0) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("missing or malformed SEV reducedPhysBits information 
"
+ "in QEMU capabilities cache"));
+return -1;
+}
+
+if (!(sev->pdh = virXPathString("string(./sev/pdh)", ctxt)))  {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("missing SEV pdh information "
+ "in QEMU capabilities cache"));
+return -1;
+}
+
+if (!(sev->cert_chain = virXPathString("string(./sev/certChain)", ctxt))) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("missing SEV certChain information "
+ "in QEMU capabilities cache"));
+return -1;
+}
+
+VIR_STEAL_PTR(qemuCaps->sevCapabilities, sev);
+return 0;
+}
+
+
 /*
  * Parsing a doc that looks like
  *
@@ -3592,6 +3668,9 @@ virQEMUCapsLoadCache(virArch hostArch,
 }
 VIR_FREE(nodes);

+if (virQEMUCapsParseSEVInfo(qemuCaps, ctxt) < 0)
+goto cleanup;
+
 virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_KVM);
 virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_QEMU);

@@ -3709,6 +3788,24 @@ virQEMUCapsFormatCPUModels(virQEMUCapsPtr qemuCaps,
 }


+static void
+virQEMUCapsFormatSEVInfo(virQEMUCapsPtr qemuCaps, virBufferPtr buf)
+{
+virSEVCapabilityPtr sev = virQEMUCapsGetSEVCapabilities(qemuCaps);
+
+virBufferAddLit(buf, "\n");
+virBufferAdjustIndent(buf, 2);
+virBufferAsprintf(buf, "%u\n", sev->cbitpos);
+   

[libvirt] [PATCH v2 1/4] tests: sev: Test launch-security with specific QEMU version

[Erik Skultety]

In order to test SEV we need real QEMU capabilities. Ideally, this would
be tested with -latest capabilities, however, our capabilities are
currently tied to Intel HW, even the 2.12.0 containing SEV were edited by
hand, so we can only use that one for now, as splitting the capabilities
according to the vendor is a refactor for another day. The need for real
capabilities comes from the extended SEV platform data (PDH, cbitpos,
etc.) we'll need to cache/parse.

Signed-off-by: Erik Skultety 
Acked-by: Peter Krempa 
---
 ...ev.args => launch-security-sev.x86_64-2.12.0.args} | 19 ---
 tests/qemuxml2argvtest.c  |  4 +---
 2 files changed, 13 insertions(+), 10 deletions(-)
 rename tests/qemuxml2argvdata/{launch-security-sev.args => 
launch-security-sev.x86_64-2.12.0.args} (54%)

diff --git a/tests/qemuxml2argvdata/launch-security-sev.args 
b/tests/qemuxml2argvdata/launch-security-sev.x86_64-2.12.0.args
similarity index 54%
rename from tests/qemuxml2argvdata/launch-security-sev.args
rename to tests/qemuxml2argvdata/launch-security-sev.x86_64-2.12.0.args
index 219a242e51..6da068e1a5 100644
--- a/tests/qemuxml2argvdata/launch-security-sev.args
+++ b/tests/qemuxml2argvdata/launch-security-sev.x86_64-2.12.0.args
@@ -5,25 +5,30 @@ USER=test \
 LOGNAME=test \
 QEMU_AUDIO_DRV=none \
 /usr/bin/qemu-system-x86_64 \
--name QEMUGuest1 \
+-name guest=QEMUGuest1,debug-threads=on \
 -S \
+-object secret,id=masterKey0,format=raw,\
+file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \
 -machine pc-1.0,accel=kvm,usb=off,dump-guest-core=off,memory-encryption=sev0 \
 -m 214 \
+-realtime mlock=off \
 -smp 1,sockets=1,cores=1,threads=1 \
 -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
 -display none \
 -no-user-config \
 -nodefaults \
--chardev 
socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
-server,nowait \
+-chardev socket,id=charmonitor,fd=1729,server,nowait \
 -mon chardev=charmonitor,id=monitor,mode=control \
 -rtc base=utc \
 -no-shutdown \
 -no-acpi \
--usb \
+-boot strict=on \
+-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \
 -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
--device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,\
-bootindex=1 \
+-device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \
 -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=1,policy=0x1,\
 dh-cert-file=/tmp/lib/domain--1-QEMUGuest1/dh_cert.base64,\
-session-file=/tmp/lib/domain--1-QEMUGuest1/session.base64
+session-file=/tmp/lib/domain--1-QEMUGuest1/session.base64 \
+-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
+resourcecontrol=deny \
+-msg timestamp=on
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 5de92e67e7..0e9eef66ee 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2972,9 +2972,7 @@ mymain(void)
 DO_TEST_CAPS_ARCH_LATEST("vhost-vsock-ccw", "s390x");
 DO_TEST_CAPS_ARCH_LATEST("vhost-vsock-ccw-auto", "s390x");
 
-DO_TEST("launch-security-sev",
-QEMU_CAPS_KVM,
-QEMU_CAPS_SEV_GUEST);
+DO_TEST_CAPS_VER("launch-security-sev", "2.12.0");
 
 if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL)
 virFileDeleteTree(fakerootdir);
-- 
2.14.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 2/4] qemu: Define and use a auto cleanup function with virSEVCapability

[Erik Skultety]

Keep with the recent effort of replacing as many explicit *Free
functions with their automatic equivalents.

Signed-off-by: Erik Skultety 
Acked-by: Peter Krempa 
---
 src/conf/domain_capabilities.h |  4 
 src/qemu/qemu_capabilities.c   | 12 
 src/qemu/qemu_monitor_json.c   | 11 ++-
 3 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h
index 755de1365f..45ebc436b9 100644
--- a/src/conf/domain_capabilities.h
+++ b/src/conf/domain_capabilities.h
@@ -25,6 +25,7 @@
 
 # include "internal.h"
 # include "domain_conf.h"
+# include "viralloc.h"
 
 typedef const char * (*virDomainCapsValToStr)(int value);
 
@@ -215,4 +216,7 @@ char * virDomainCapsFormat(virDomainCapsPtr const caps);
 
 void
 virSEVCapabilitiesFree(virSEVCapability *capabilities);
+
+VIR_DEFINE_AUTOPTR_FUNC(virSEVCapability, virSEVCapabilitiesFree);
+
 #endif /* __DOMAIN_CAPABILITIES_H__ */
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index e6e199b2c6..c17d26801e 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -5263,9 +5263,8 @@ static int
 virQEMUCapsFillDomainFeatureSEVCaps(virQEMUCapsPtr qemuCaps,
 virDomainCapsPtr domCaps)
 {
-virSEVCapability *sev;
 virSEVCapability *cap = qemuCaps->sevCapabilities;
-int ret = -1;
+VIR_AUTOPTR(virSEVCapability) sev = NULL;
 
 if (!cap)
 return 0;
@@ -5274,19 +5273,16 @@ virQEMUCapsFillDomainFeatureSEVCaps(virQEMUCapsPtr 
qemuCaps,
 return -1;
 
 if (VIR_STRDUP(sev->pdh, cap->pdh) < 0)
-goto cleanup;
+return -1;
 
 if (VIR_STRDUP(sev->cert_chain, cap->cert_chain) < 0)
-goto cleanup;
+return -1;
 
 sev->cbitpos = cap->cbitpos;
 sev->reduced_phys_bits = cap->reduced_phys_bits;
 VIR_STEAL_PTR(domCaps->sev, sev);
 
-ret = 0;
- cleanup:
-virSEVCapabilitiesFree(sev);
-return ret;
+return 0;
 }
 
 
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 2921f110a9..3f99f39120 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -6443,9 +6443,11 @@ qemuMonitorJSONGetSEVCapabilities(qemuMonitorPtr mon,
 virJSONValuePtr cmd;
 virJSONValuePtr reply = NULL;
 virJSONValuePtr caps;
-virSEVCapability *capability = NULL;
-const char *pdh = NULL, *cert_chain = NULL;
-unsigned int cbitpos, reduced_phys_bits;
+const char *pdh = NULL;
+const char *cert_chain = NULL;
+unsigned int cbitpos;
+unsigned int reduced_phys_bits;
+VIR_AUTOPTR(virSEVCapability) capability = NULL;
 
 *capabilities = NULL;
 
@@ -6476,7 +6478,7 @@ qemuMonitorJSONGetSEVCapabilities(qemuMonitorPtr mon,
 }
 
 if (virJSONValueObjectGetNumberUint(caps, "reduced-phys-bits",
-   _phys_bits) < 0) {
+_phys_bits) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("query-sev-capabilities reply was missing"
  " 'reduced-phys-bits' field"));
@@ -6512,7 +6514,6 @@ qemuMonitorJSONGetSEVCapabilities(qemuMonitorPtr mon,
 ret = 0;
 
  cleanup:
-virSEVCapabilitiesFree(capability);
 virJSONValueFree(cmd);
 virJSONValueFree(reply);
 
-- 
2.14.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 0/4] Fix a SIGSEGV in libvirtd when querying AMD SEV info

[Erik Skultety]

This series fixes the following BZ:
https://bugzilla.redhat.com/show_bug.cgi?id=1612009

TL;DR:
We don't format SEV platform data (PDH, certificate chain,...) into our qemu
caps cache which poses a problem after libvirtd restart when we restore from
the cache and get a segfault upon issuing virNodeGetSEVInfo.

Since v1:
- reworked patch 3 so that qemuMonitorJSONGetSEVCapabilities returns more
values in order to distinguish between error and whether SEV is actually
supported
- added the missing backtrace to patch 4
- no other patches were changed apart from patch 3

Erik Skultety (4):
  tests: sev: Test launch-security with specific QEMU version
  qemu: Define and use a auto cleanup function with virSEVCapability
  qemu: Fix probing of AMD SEV support
  qemu: caps: Format SEV platform data into qemuCaps cache

 src/conf/domain_capabilities.h |   4 +
 src/qemu/qemu_capabilities.c   | 127 +++--
 src/qemu/qemu_monitor_json.c   |  31 +++--
 tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml  |   5 +-
 tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml  |   6 +
 tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml   |   1 -
 ...args => launch-security-sev.x86_64-2.12.0.args} |  19 +--
 tests/qemuxml2argvtest.c   |   4 +-
 8 files changed, 164 insertions(+), 33 deletions(-)
 rename tests/qemuxml2argvdata/{launch-security-sev.args => 
launch-security-sev.x86_64-2.12.0.args} (54%)

--
2.14.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 0/2] spec: Add more nvram firmware paths

[Andrea Bolognani]

On Wed, 2018-08-15 at 18:29 -0400, Cole Robinson wrote:
> Add nvram firmware paths for ovmf ia32 and ovmf arm32. The latter
> is at least useful for upcoming Fedora which will support arm32
> installs, rather than just pre-created disk image imports
> 
> First patch is just a comment cleanup/improvement
> 
> v2:
> Separate patches per Andrea's review
> Fix LOADER overwriting per Andrea's review
> 
> Cole Robinson (2):
>   spec: Change nvram comments to reference edk2 package names
>   spec: Add firmware/nvram paths for edk2 arm and ia32
> 
>  libvirt.spec.in | 17 +
>  1 file changed, 13 insertions(+), 4 deletions(-)

Based on the assumption that paths and package names are
correct,

  Reviewed-by: Andrea Bolognani 

for the series.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu_command: Fix memleak in qemuBuildFloppyCommandLineControllerOptions

[Erik Skultety]

On Thu, Aug 16, 2018 at 12:22:24PM +0200, Michal Privoznik wrote:
> Even though the buffer is passed to virCommand we still need to
> free it.
>
> ==191201== 1,010 bytes in 1 blocks are definitely lost in loss record 826 of 
> 836
> ==191201==at 0x4C2CE3F: malloc (vg_replace_malloc.c:298)
> ==191201==by 0x4C2F1BF: realloc (vg_replace_malloc.c:785)
> ==191201==by 0x5D39E82: virReallocN (viralloc.c:245)
> ==191201==by 0x5D3E8F2: virBufferGrow (virbuffer.c:150)
> ==191201==by 0x5D3E9C8: virBufferAdd (virbuffer.c:185)
> ==191201==by 0x56EAC98: qemuBuildFloppyCommandLineControllerOptions 
> (qemu_command.c:2162)
> ==191201==by 0x56EB3E1: qemuBuildDisksCommandLine (qemu_command.c:2370)
> ==191201==by 0x570055E: qemuBuildCommandLine (qemu_command.c:10315)
> ==191201==by 0x575EA7F: qemuProcessCreatePretendCmd (qemu_process.c:6777)
> ==191201==by 0x113DAB: testCompareXMLToArgv (qemuxml2argvtest.c:598)
> ==191201==by 0x13A75B: virTestRun (testutils.c:180)
> ==191201==by 0x138BE8: mymain (qemuxml2argvtest.c:2975)
>
> Signed-off-by: Michal Privoznik 
> ---

Reviewed-by: Erik Skultety 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC PATCH 01/17] qemu: setup shared memory without explicit numa configuration

[Marc-André Lureau]

Hi

On Thu, Aug 16, 2018 at 4:35 AM, John Ferlan  wrote:
>
>
> On 07/13/2018 09:28 AM, marcandre.lur...@redhat.com wrote:
>> From: Marc-André Lureau 
>>
>> When a domain is configured with 'shared' memory backing:
>>
>>   
>> 
>>   
>>
>> But no explicit NUMA configuration, let's configure a shared memory
>> backend associated with default -numa.
>>
>> Signed-off-by: Marc-André Lureau 
>> ---
>>  src/qemu/qemu_command.c   | 100 --
>>  .../fd-memory-no-numa-topology.args   |   4 +
>>  2 files changed, 73 insertions(+), 31 deletions(-)
>>
>
> NUMA, memory backends, and hugepages - not in my wheelhouse of
> knowledge. Hopefully Michal and/or Pavel will take a look!
>
> Is it possible someone may not want this type of thing to happen? Is

I assume someone that sets 'shared' memory mode may consider this as a bug fix.

> there an upside or downside to this?  What happens "today" when not

You get non-shared memory

> generated? And of course, what about migration concerns about
> unconditionally doing this for some target migration?

True, this will break migration though if the target uses
numa/memory-backend-file.

What do you suggest?

>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 44ae8dcef7..f1235099b2 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -3254,26 +3254,21 @@ qemuBuildMemoryBackendProps(virJSONValuePtr 
>> *backendProps,
>>
>>
>>  static int
>> -qemuBuildMemoryCellBackendStr(virDomainDefPtr def,
>> -  virQEMUDriverConfigPtr cfg,
>> -  size_t cell,
>> -  qemuDomainObjPrivatePtr priv,
>> -  virBufferPtr buf)
>> +qemuBuildMemoryBackendStr(virDomainDefPtr def,
>> +  virQEMUDriverConfigPtr cfg,
>> +  const char *alias,
>
> So the one "concern" I'd have here is some time in the future the @mem
> gets allocated and handled like a real device eventually calling
> virDomainDeviceInfoClear and that'd be a problem for the passed const
> char * string.  Some future person's problem I guess!
>
>> +  int targetNode,
>> +  unsigned long long memsize,
>> +  qemuDomainObjPrivatePtr priv,
>> +  virBufferPtr buf)
>
> As much as a long name is a pain, is this more of a :
>
> qemuBuildMemorySharedDefaultBackendStr

Why?

>
>>  {
>>  virJSONValuePtr props = NULL;
>> -char *alias = NULL;
>> -int ret = -1;
>> -int rc;
>>  virDomainMemoryDef mem = { 0 };
>> -unsigned long long memsize = virDomainNumaGetNodeMemorySize(def->numa,
>> -cell);
>> -
>> -if (virAsprintf(, "ram-node%zu", cell) < 0)
>> -goto cleanup;
>> +int rc, ret = -1;
>>
>>  mem.size = memsize;
>> -mem.targetNode = cell;
>> -mem.info.alias = alias;
>> +mem.targetNode = targetNode;
>> +mem.info.alias = (char *)alias;
>>
>>  if ((rc = qemuBuildMemoryBackendProps(, alias, cfg, 
>> priv->qemuCaps,
>>def, , priv->autoNodeset, 
>> false)) < 0)> @@ -3284,9 +3279,30 @@ 
>> qemuBuildMemoryCellBackendStr(virDomainDefPtr def,
>>
>>  ret = rc;
>>
>> +cleanup:
>
> Fails 'make check syntax-check' :
>
> maint.mk: Top-level labels should be indented by one space
> make: *** [cfg.mk:898: sc_require_space_before_label] Error 1

argh, fixed

>
>> +virJSONValueFree(props);
>> +return ret;
>> +}
>> +
>> +
>> +static int
>> +qemuBuildMemoryCellBackendStr(virDomainDefPtr def,
>> +  virQEMUDriverConfigPtr cfg,
>> +  size_t cell,
>> +  qemuDomainObjPrivatePtr priv,
>> +  virBufferPtr buf)
>> +{
>> +char *alias = NULL;
>> +int ret = -1;
>> +unsigned long long memsize = virDomainNumaGetNodeMemorySize(def->numa, 
>> cell);
>> +
>> +if (virAsprintf(, "ram-node%zu", cell) < 0)
>> +goto cleanup;
>> +
>> +ret = qemuBuildMemoryBackendStr(def, cfg, alias, cell, memsize, priv, 
>> buf);
>> +
>>   cleanup:
>>  VIR_FREE(alias);
>> -virJSONValueFree(props);
>>
>>  return ret;
>>  }
>> @@ -7590,6 +7606,17 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
>>  size_t ncells = virDomainNumaGetNodeCount(def->numa);
>>  const long system_page_size = virGetSystemPageSizeKB();
>>  bool numa_distances = false;
>> +bool implicit = false;
>> +
>> +if (ncells == 0) {
>> +if (def->mem.access == VIR_DOMAIN_MEMORY_ACCESS_SHARED) {
>> +ncells = 1;
>
> Well, that's cheating for the subsequent for loop ;-)
>
>> +implicit = true;
>> +} else {
>> +ret = 0;
>> +goto cleanup;
>> +}
>> +}
>
> So if ncells == 0 && def->mem.access 

Re: [libvirt] [PATCH] qemu_command: Fix memleak in qemuBuildFloppyCommandLineControllerOptions

[Peter Krempa]

On Thu, Aug 16, 2018 at 12:22:24 +0200, Michal Privoznik wrote:
> Even though the buffer is passed to virCommand we still need to
> free it.

This is misleading. In fact we are NOT passing it to virCommandAddArgBuffer
in some cases as it would be properly freed in that case.

> ==191201== 1,010 bytes in 1 blocks are definitely lost in loss record 826 of 
> 836
> ==191201==at 0x4C2CE3F: malloc (vg_replace_malloc.c:298)
> ==191201==by 0x4C2F1BF: realloc (vg_replace_malloc.c:785)
> ==191201==by 0x5D39E82: virReallocN (viralloc.c:245)
> ==191201==by 0x5D3E8F2: virBufferGrow (virbuffer.c:150)
> ==191201==by 0x5D3E9C8: virBufferAdd (virbuffer.c:185)
> ==191201==by 0x56EAC98: qemuBuildFloppyCommandLineControllerOptions 
> (qemu_command.c:2162)
> ==191201==by 0x56EB3E1: qemuBuildDisksCommandLine (qemu_command.c:2370)
> ==191201==by 0x570055E: qemuBuildCommandLine (qemu_command.c:10315)
> ==191201==by 0x575EA7F: qemuProcessCreatePretendCmd (qemu_process.c:6777)
> ==191201==by 0x113DAB: testCompareXMLToArgv (qemuxml2argvtest.c:598)
> ==191201==by 0x13A75B: virTestRun (testutils.c:180)
> ==191201==by 0x138BE8: mymain (qemuxml2argvtest.c:2975)
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_command.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index ddb90895e0..96fc360f4a 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2225,6 +2225,7 @@ 
> qemuBuildFloppyCommandLineControllerOptions(virCommandPtr cmd,
>  VIR_FREE(backendAlias);
>  VIR_FREE(backendStr);
>  VIR_FREE(bootindexStr);
> +virBufferFreeAndReset(_opts);
>  return ret;

ACK if you fix the commit message


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 0/6] fdaskljrew

[Michal Privoznik]

On 08/16/2018 12:22 PM, Simon Kobyda wrote:
> I send this by mistake. Sorry.
> 

Also, please don't top post on technical lists. Always use either inline
or bottom post replies.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt-glib][PATCH v2] Use new GObject define macros with private

[Andrea Bolognani]

On Thu, 2018-08-16 at 10:06 +0200, Michal Privoznik wrote:
[...]
> -GLIB2_REQUIRED=2.36.0
> +GLIB2_REQUIRED=2.38.0
>  AC_SUBST([GLIB2_REQUIRED]) dnl used in the .spec file
>  GLIB2_TEST_REQUIRED=2.38.0

We can now get rid of GLIB2_TEST_REQUIRED. Would you mind sending
a follow-up patch that does that?

[...]
>  static void 
> gvir_config_capabilities_cpu_class_init(GVirConfigCapabilitiesCpuClass *klass)
>  {
> -g_type_class_add_private(klass, 
> sizeof(GVirConfigCapabilitiesCpuPrivate));
>  

Drop the empty line here.

[...]
> @@ -47,7 +47,6 @@ static void 
> gvir_config_domain_cpu_class_init(GVirConfigDomainCpuClass *klass)
>  capabilities_class = GVIR_CONFIG_CAPABILITIES_CPU_CLASS(klass);
>  capabilities_class->get_features = _gvir_config_domain_cpu_get_features;
>  

Drop the empty line here.

[...]
>  static void gvir_config_domain_power_management_class_init
> -(GVirConfigDomainPowerManagementClass *klass)
> +(GVirConfigDomainPowerManagementClass *klass G_GNUC_UNUSED)

Weird formatting here, maybe fix it while you're at it?

[...]
> @@ -128,7 +128,6 @@ static void 
> gvir_config_domain_class_init(GVirConfigDomainClass *klass)
>  {
>  GObjectClass *object_class = G_OBJECT_CLASS(klass);
>  
> -g_type_class_add_private(klass, sizeof(GVirConfigDomainPrivate));
>  

Drop one of the two empty lines here.

[...]
> @@ -90,7 +90,6 @@ static void 
> gvir_config_xml_doc_class_init(GVirConfigXmlDocClass *klass)
>  {
>  GObjectClass *object_class = G_OBJECT_CLASS(klass);
>  
> -g_type_class_add_private(klass, sizeof(GVirConfigXmlDocPrivate));
>  

Drop one of the two empty lines here.

[...]
> @@ -231,7 +231,6 @@ static void 
> gvir_connection_class_init(GVirConnectionClass *klass)
>   1,
>   GVIR_TYPE_DOMAIN);
>  
> -g_type_class_add_private(klass, sizeof(GVirConnectionPrivate));

Drop the empty line here.

[...]
> @@ -135,7 +135,6 @@ static void 
> gvir_domain_device_class_init(GVirDomainDeviceClass *klass)
>  
> G_PARAM_CONSTRUCT_ONLY |
>  
> G_PARAM_STATIC_STRINGS));
>  
> -g_type_class_add_private(klass, sizeof(GVirDomainDevicePrivate));

Drop the empty line here.

[...]
> @@ -126,7 +126,6 @@ static void 
> gvir_domain_snapshot_class_init(GVirDomainSnapshotClass *klass)
> 
> G_PARAM_CONSTRUCT_ONLY |
> 
> G_PARAM_STATIC_STRINGS));
>  
> -g_type_class_add_private(klass, sizeof(GVirDomainSnapshotPrivate));

Drop the empty line here.

[...]
> @@ -247,7 +247,6 @@ static void gvir_domain_class_init(GVirDomainClass *klass)
>  G_TYPE_NONE,
>  0);
>  
> -g_type_class_add_private(klass, sizeof(GVirDomainPrivate));

Drop the empty line here.

[...]
> @@ -196,7 +196,6 @@ static void 
> gvir_input_stream_class_init(GVirInputStreamClass *klass)
>  GObjectClass *gobject_class = G_OBJECT_CLASS(klass);
>  GInputStreamClass *ginputstream_class = G_INPUT_STREAM_CLASS(klass);
>  
> -g_type_class_add_private(klass, sizeof(GVirInputStreamPrivate));
>  

Drop one of the two empty lines here.

[...]
> @@ -126,7 +126,6 @@ static void gvir_interface_class_init(GVirInterfaceClass 
> *klass)
> 
> G_PARAM_CONSTRUCT_ONLY |
> 
> G_PARAM_STATIC_STRINGS));
>  
> -g_type_class_add_private(klass, sizeof(GVirInterfacePrivate));

Drop the empty line here.

[...]
> @@ -102,7 +102,6 @@ static void gvir_manager_class_init(GVirManagerClass 
> *klass)
>   1,
>   GVIR_TYPE_CONNECTION);
>  
> -g_type_class_add_private(klass, sizeof(GVirManagerPrivate));

Drop the empty line here.

[...]
> @@ -124,7 +124,6 @@ static void 
> gvir_network_dhcp_lease_class_init(GVirNetworkDHCPLeaseClass *klass)
>   G_PARAM_PRIVATE |
>   
> G_PARAM_STATIC_STRINGS));
>  
> -g_type_class_add_private(klass, sizeof(GVirNetworkDHCPLeasePrivate));

Drop the empty line here.

[...]
> @@ -142,7 +142,6 @@ static void 
> gvir_network_filter_class_init(GVirNetworkFilterClass *klass)
> 
> G_PARAM_CONSTRUCT_ONLY |
> 
> G_PARAM_STATIC_STRINGS));
>  
> -g_type_class_add_private(klass, sizeof(GVirNetworkFilterPrivate));

Drop the empty line here.

[...]
> @@ -142,7 +142,6 @@ static void gvir_network_class_init(GVirNetworkClass 
> *klass)
> 
> G_PARAM_CONSTRUCT_ONLY |
> 
> G_PARAM_STATIC_STRINGS));
>  

[libvirt] [PATCH] qemu_command: Fix memleak in qemuBuildFloppyCommandLineControllerOptions

[Michal Privoznik]

Even though the buffer is passed to virCommand we still need to
free it.

==191201== 1,010 bytes in 1 blocks are definitely lost in loss record 826 of 836
==191201==at 0x4C2CE3F: malloc (vg_replace_malloc.c:298)
==191201==by 0x4C2F1BF: realloc (vg_replace_malloc.c:785)
==191201==by 0x5D39E82: virReallocN (viralloc.c:245)
==191201==by 0x5D3E8F2: virBufferGrow (virbuffer.c:150)
==191201==by 0x5D3E9C8: virBufferAdd (virbuffer.c:185)
==191201==by 0x56EAC98: qemuBuildFloppyCommandLineControllerOptions 
(qemu_command.c:2162)
==191201==by 0x56EB3E1: qemuBuildDisksCommandLine (qemu_command.c:2370)
==191201==by 0x570055E: qemuBuildCommandLine (qemu_command.c:10315)
==191201==by 0x575EA7F: qemuProcessCreatePretendCmd (qemu_process.c:6777)
==191201==by 0x113DAB: testCompareXMLToArgv (qemuxml2argvtest.c:598)
==191201==by 0x13A75B: virTestRun (testutils.c:180)
==191201==by 0x138BE8: mymain (qemuxml2argvtest.c:2975)

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_command.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ddb90895e0..96fc360f4a 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2225,6 +2225,7 @@ qemuBuildFloppyCommandLineControllerOptions(virCommandPtr 
cmd,
 VIR_FREE(backendAlias);
 VIR_FREE(backendStr);
 VIR_FREE(bootindexStr);
+virBufferFreeAndReset(_opts);
 return ret;
 }
 
-- 
2.16.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 0/6] fdaskljrew

[Simon Kobyda]

I send this by mistake. Sorry.

Simon

On Thu, 2018-08-16 at 10:48 +0100, Daniel P. Berrangé wrote:
> On Thu, Aug 16, 2018 at 11:11:26AM +0200, Simon Kobyda wrote:
> > rpdwrewrewr
> 
> Please use a sensible subject line, and provide useful information
> about
> the patch series in this cover letter.
> 
> > 
> > Simon Kobyda (6):
> >   vsh: Add API for printing tables.
> >   virsh: Implement new table API for virsh list
> >   vsh: Added tests
> >   virsh: Implement vsh-table to iface-list
> >   virsh: Implement vshTable API to net-list and net-dhcp-leases
> >   virsh: Implement vshTable API to secret-list
> > 
> >  tests/Makefile.am|   8 +
> >  tests/virshtest.c|  14 +-
> >  tests/vshtabletest.c | 247 +
> >  tools/Makefile.am|   4 +-
> >  tools/virsh-domain-monitor.c |  43 ++--
> >  tools/virsh-interface.c  |  27 ++-
> >  tools/virsh-network.c|  50 +++--
> >  tools/virsh-secret.c |  30 ++-
> >  tools/vsh-table.c| 413
> > +++
> >  tools/vsh-table.h|  42 
> >  10 files changed, 817 insertions(+), 61 deletions(-)
> >  create mode 100644 tests/vshtabletest.c
> >  create mode 100644 tools/vsh-table.c
> >  create mode 100644 tools/vsh-table.h
> > 
> > -- 
> > 2.17.1
> > 
> > --
> > libvir-list mailing list
> > libvir-list@redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list
> 
> Regards,
> Daniel

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/5] tests: qemuxml2argvmock: Don't mock virCommandPassFD

[Peter Krempa]

On Thu, Aug 16, 2018 at 12:08:12 +0200, Michal Privoznik wrote:
> On 08/14/2018 03:21 PM, Peter Krempa wrote:
> > This function does not modify the host. It merely puts the file
> > descriptor into a list in virCommandPtr.
> > 
> > Signed-off-by: Peter Krempa 
> > ---
> >  tests/qemuxml2argvmock.c | 8 
> >  1 file changed, 8 deletions(-)
> > 
> > diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c
> > index 4df92cf396..c8a5f186d5 100644
> > --- a/tests/qemuxml2argvmock.c
> > +++ b/tests/qemuxml2argvmock.c
> > @@ -184,14 +184,6 @@ virNetDevRunEthernetScript(const char *ifname 
> > ATTRIBUTE_UNUSED,
> >  return 0;
> >  }
> > 
> > -void
> > -virCommandPassFD(virCommandPtr cmd ATTRIBUTE_UNUSED,
> > - int fd ATTRIBUTE_UNUSED,
> > - unsigned int flags ATTRIBUTE_UNUSED)
> > -{
> > -/* nada */
> > -}
> > -
> >  int
> >  virNetDevOpenvswitchGetVhostuserIfname(const char *path ATTRIBUTE_UNUSED,
> > char **ifname)
> > 
> 
> NACK, this indeed causes caller to close wrong FD:
> 
> 
> libvirt.git/tests $ ../run valgrind --trace-children=yes ./qemuxml2argvtest
> 
> ==210382== Warning: invalid file descriptor 1729 in syscall close()

I've posted a v2 of these yesterday in the morning.

Also in this case we know that fd 1729 is in fact invalid as we check
that is unused prior to this and the test would abort if it was used.

The new approach in v2 whitelists only the two FDs (which are still
invalid though) to be passed in virCommand since otherwise the command
line generator will not work.

Also note that when forking subprocesses we close a bunch of invalid FDs
anyways so it should not be a problem if we know they are not used
elsewhere.


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/4] cpu: allow include files for CPU definition

[Daniel P . Berrangé]

On Tue, Aug 14, 2018 at 12:55:19PM +0200, Jiri Denemark wrote:
> On Wed, Aug 01, 2018 at 18:02:29 +0100, Daniel P. Berrangé wrote:
> > Allow for syntax
> > 
> > 
> 
> It seems the code should just work with
> 
> 
> 
> but Makefile.am and libvirt.spec would need some adjustment.
> 
> > to reference other files in the CPU database directory
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  libvirt.spec.in   |  2 +-
> >  mingw-libvirt.spec.in |  4 +--
> >  src/Makefile.am   |  2 +-
> >  src/cpu/cpu_map.c | 84 +--
> >  4 files changed, 86 insertions(+), 6 deletions(-)
> > 
> > diff --git a/libvirt.spec.in b/libvirt.spec.in
> > index 19ae55cdaf..b6745dbffa 100644
> > --- a/libvirt.spec.in
> > +++ b/libvirt.spec.in
> > @@ -1856,7 +1856,7 @@ exit 0
> >  %{_datadir}/libvirt/schemas/storagepool.rng
> >  %{_datadir}/libvirt/schemas/storagevol.rng
> >  
> > -%{_datadir}/libvirt/cpu_map.xml
> > +%{_datadir}/libvirt/cpu_map*.xml
> >  
> >  %{_datadir}/libvirt/test-screenshot.png
> >  
> > diff --git a/mingw-libvirt.spec.in b/mingw-libvirt.spec.in
> > index cc1e619927..22fe7a000f 100644
> > --- a/mingw-libvirt.spec.in
> > +++ b/mingw-libvirt.spec.in
> > @@ -260,7 +260,7 @@ rm -rf 
> > $RPM_BUILD_ROOT%{mingw64_libexecdir}/libvirt-guests.sh
> >  %{mingw32_datadir}/libvirt/api/libvirt-qemu-api.xml
> >  %{mingw32_datadir}/libvirt/api/libvirt-admin-api.xml
> >  
> > -%{mingw32_datadir}/libvirt/cpu_map.xml
> > +%{mingw32_datadir}/libvirt/cpu_map*.xml
> >  
> >  %{mingw32_datadir}/libvirt/test-screenshot.png
> >  
> > @@ -347,7 +347,7 @@ rm -rf 
> > $RPM_BUILD_ROOT%{mingw64_libexecdir}/libvirt-guests.sh
> >  %{mingw64_datadir}/libvirt/api/libvirt-qemu-api.xml
> >  %{mingw64_datadir}/libvirt/api/libvirt-admin-api.xml
> >  
> > -%{mingw64_datadir}/libvirt/cpu_map.xml
> > +%{mingw64_datadir}/libvirt/cpu_map*.xml
> >  
> >  %{mingw64_datadir}/libvirt/test-screenshot.png
> >  
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index a4f213480e..11a7ac81e2 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -366,7 +366,7 @@ check-local: check-protocol check-symfile 
> > check-symsorting \
> >  
> >  
> >  
> > -pkgdata_DATA = cpu/cpu_map.xml
> > +pkgdata_DATA = $(wildcard $(srcdir)/cpu/cpu_map*.xml)
> >  
> >  EXTRA_DIST +=  $(pkgdata_DATA)
> >  
> > diff --git a/src/cpu/cpu_map.c b/src/cpu/cpu_map.c
> > index d263eb8cdd..9e090919ed 100644
> > --- a/src/cpu/cpu_map.c
> > +++ b/src/cpu/cpu_map.c
> > @@ -70,6 +70,83 @@ static int load(xmlXPathContextPtr ctxt,
> ..
> > +static int loadIncludes(xmlXPathContextPtr ctxt,
> > +cpuMapLoadCallback callback,
> > +void *data)
> > +{
> > +int ret = -1;
> > +xmlNodePtr ctxt_node;
> > +xmlNodePtr *nodes = NULL;
> > +int n;
> > +size_t i;
> > +
> > +ctxt_node = ctxt->node;
> > +
> > +n = virXPathNodeSet("include", ctxt, );
> > +if (n < 0)
> > +goto cleanup;
> > +
> > +for (i = 0; i < n; i++) {
> > +char *filename = virXMLPropString(nodes[i], "filename");
> 
> This would be a nice candidate for VIR_AUTOFREE(char *) :-)

Since none of the src/cpu code has been switched to this style yet, its
probably best not to introduce it here. Rather wait for the bulk convert


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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/5] tests: qemuxml2argvmock: Don't mock virCommandPassFD

[Michal Privoznik]

On 08/14/2018 03:21 PM, Peter Krempa wrote:
> This function does not modify the host. It merely puts the file
> descriptor into a list in virCommandPtr.
> 
> Signed-off-by: Peter Krempa 
> ---
>  tests/qemuxml2argvmock.c | 8 
>  1 file changed, 8 deletions(-)
> 
> diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c
> index 4df92cf396..c8a5f186d5 100644
> --- a/tests/qemuxml2argvmock.c
> +++ b/tests/qemuxml2argvmock.c
> @@ -184,14 +184,6 @@ virNetDevRunEthernetScript(const char *ifname 
> ATTRIBUTE_UNUSED,
>  return 0;
>  }
> 
> -void
> -virCommandPassFD(virCommandPtr cmd ATTRIBUTE_UNUSED,
> - int fd ATTRIBUTE_UNUSED,
> - unsigned int flags ATTRIBUTE_UNUSED)
> -{
> -/* nada */
> -}
> -
>  int
>  virNetDevOpenvswitchGetVhostuserIfname(const char *path ATTRIBUTE_UNUSED,
> char **ifname)
> 

NACK, this indeed causes caller to close wrong FD:


libvirt.git/tests $ ../run valgrind --trace-children=yes ./qemuxml2argvtest

==210382== Warning: invalid file descriptor 1729 in syscall close()


Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/5] qemu: command: Extract opening of TPM backend FDs for mocking purposes

[Michal Privoznik]

On 08/14/2018 03:21 PM, Peter Krempa wrote:
> Allow mocking of the file descriptor numbers used for the TPM
> passthrough mode by extracting the relevant code into an exported
> function.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_command.c | 41 +++--
>  src/qemu/qemu_command.h |  7 +++
>  2 files changed, 34 insertions(+), 14 deletions(-)

ACK

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 4/5] qemu: capabilities: Always assume QEMU_CAPS_ADD_FD

[Michal Privoznik]

On 08/14/2018 03:21 PM, Peter Krempa wrote:
> The capability was usable since qemu 1.3 so we can remove all the
> detection code.
> 
> Signed-off-by: Peter Krempa 
> ---
> 
> Note that the *replies files need to be renumbered. I've split that to a
> separate patch for ease of review. Notably because it was done
> automatically by tests/qemucapsfixreplies.
> 
>  src/qemu/qemu_capabilities.c   | 17 
>  src/qemu/qemu_capabilities.h   |  2 +-
>  src/qemu/qemu_command.c| 32 
> ++
>  .../qemucapabilitiesdata/caps_1.5.3.x86_64.replies | 17 
>  tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml   |  3 +-
>  .../qemucapabilitiesdata/caps_1.6.0.x86_64.replies | 17 
>  tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml   |  3 +-
>  .../qemucapabilitiesdata/caps_1.7.0.x86_64.replies | 17 
>  tests/qemucapabilitiesdata/caps_1.7.0.x86_64.xml   |  3 +-
>  .../qemucapabilitiesdata/caps_2.1.1.x86_64.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.1.1.x86_64.xml   |  3 +-
>  .../caps_2.10.0.aarch64.replies| 17 
>  tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml |  3 +-
>  .../qemucapabilitiesdata/caps_2.10.0.ppc64.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml   |  3 +-
>  .../qemucapabilitiesdata/caps_2.10.0.s390x.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml   |  3 +-
>  .../caps_2.10.0.x86_64.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml  |  3 +-
>  .../qemucapabilitiesdata/caps_2.11.0.s390x.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml   |  3 +-
>  .../caps_2.11.0.x86_64.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml  |  3 +-
>  .../caps_2.12.0.aarch64.replies| 17 
>  tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml |  3 +-
>  .../qemucapabilitiesdata/caps_2.12.0.ppc64.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml   |  3 +-
>  .../qemucapabilitiesdata/caps_2.12.0.s390x.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml   |  3 +-
>  .../caps_2.12.0.x86_64.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml  |  3 +-
>  .../qemucapabilitiesdata/caps_2.4.0.x86_64.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml   |  3 +-
>  .../qemucapabilitiesdata/caps_2.5.0.x86_64.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml   |  3 +-
>  .../caps_2.6.0.aarch64.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.6.0.aarch64.xml  |  3 +-
>  .../qemucapabilitiesdata/caps_2.6.0.ppc64.replies  | 17 
>  tests/qemucapabilitiesdata/caps_2.6.0.ppc64.xml|  3 +-
>  .../qemucapabilitiesdata/caps_2.6.0.x86_64.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml   |  3 +-
>  .../qemucapabilitiesdata/caps_2.7.0.s390x.replies  | 17 
>  tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml|  3 +-
>  .../qemucapabilitiesdata/caps_2.7.0.x86_64.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml   |  3 +-
>  .../qemucapabilitiesdata/caps_2.8.0.s390x.replies  | 17 
>  tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml|  3 +-
>  .../qemucapabilitiesdata/caps_2.8.0.x86_64.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml   |  3 +-
>  .../qemucapabilitiesdata/caps_2.9.0.ppc64.replies  | 17 
>  tests/qemucapabilitiesdata/caps_2.9.0.ppc64.xml|  3 +-
>  .../qemucapabilitiesdata/caps_2.9.0.s390x.replies  | 17 
>  tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml|  3 +-
>  .../qemucapabilitiesdata/caps_2.9.0.x86_64.replies | 17 
>  tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml   |  3 +-
>  .../qemucapabilitiesdata/caps_3.0.0.ppc64.replies  | 17 
>  tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml|  3 +-
>  .../qemucapabilitiesdata/caps_3.0.0.x86_64.replies | 17 
>  tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml   |  3 +-
>  tests/qemuxml2argvtest.c   |  2 --
>  60 files changed, 43 insertions(+), 570 deletions(-)

ACK if you squash 5/5 into this one.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 5/5] FIXUP: renumber replies lines after deleting section

[Michal Privoznik]

On 08/14/2018 03:21 PM, Peter Krempa wrote:
> For simpler review.
> 

Not only that. It's even to stop qemucapabilitiestest from failing after
previous patch.

> Done via tests/qemucapsfixreplies.
> ---
>  .../qemucapabilitiesdata/caps_1.5.3.x86_64.replies | 152 
>  .../qemucapabilitiesdata/caps_1.6.0.x86_64.replies | 152 
>  .../qemucapabilitiesdata/caps_1.7.0.x86_64.replies | 152 
>  .../qemucapabilitiesdata/caps_2.1.1.x86_64.replies | 152 
>  .../caps_2.10.0.aarch64.replies| 152 
>  .../qemucapabilitiesdata/caps_2.10.0.ppc64.replies | 152 
>  .../qemucapabilitiesdata/caps_2.10.0.s390x.replies | 152 
>  .../caps_2.10.0.x86_64.replies | 184 +--
>  .../qemucapabilitiesdata/caps_2.11.0.s390x.replies | 156 
>  .../caps_2.11.0.x86_64.replies | 184 +--
>  .../caps_2.12.0.aarch64.replies| 160 -
>  .../qemucapabilitiesdata/caps_2.12.0.ppc64.replies | 160 -
>  .../qemucapabilitiesdata/caps_2.12.0.s390x.replies | 164 -
>  .../caps_2.12.0.x86_64.replies | 196 
> ++---
>  .../qemucapabilitiesdata/caps_2.4.0.x86_64.replies | 164 -
>  .../qemucapabilitiesdata/caps_2.5.0.x86_64.replies | 168 +-
>  .../caps_2.6.0.aarch64.replies | 152 
>  .../qemucapabilitiesdata/caps_2.6.0.ppc64.replies  | 152 
>  .../qemucapabilitiesdata/caps_2.6.0.x86_64.replies | 168 +-
>  .../qemucapabilitiesdata/caps_2.7.0.s390x.replies  | 144 +++
>  .../qemucapabilitiesdata/caps_2.7.0.x86_64.replies | 168 +-
>  .../qemucapabilitiesdata/caps_2.8.0.s390x.replies  | 152 
>  .../qemucapabilitiesdata/caps_2.8.0.x86_64.replies | 168 +-
>  .../qemucapabilitiesdata/caps_2.9.0.ppc64.replies  | 152 
>  .../qemucapabilitiesdata/caps_2.9.0.s390x.replies  | 152 
>  .../qemucapabilitiesdata/caps_2.9.0.x86_64.replies | 184 +--
>  .../qemucapabilitiesdata/caps_3.0.0.ppc64.replies  | 160 -
>  .../qemucapabilitiesdata/caps_3.0.0.x86_64.replies | 196 
> ++---
>  28 files changed, 2274 insertions(+), 2274 deletions(-)

This needs to be squashed to previous patch.

ACK

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/5] tests: qemuxml2argv: modernize TPM passthrough tests

[Michal Privoznik]

On 08/14/2018 03:21 PM, Peter Krempa wrote:
> All supported qemus support FD passing so modify the tests to test the
> proper code path.
> 
> Signed-off-by: Peter Krempa 
> ---
>  tests/qemuxml2argvdata/tpm-passthrough-crb.args |  5 +++--
>  tests/qemuxml2argvdata/tpm-passthrough.args |  5 +++--
>  tests/qemuxml2argvmock.c| 16 
>  tests/qemuxml2argvtest.c|  2 ++
>  4 files changed, 24 insertions(+), 4 deletions(-)

ACK

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 0/6] fdaskljrew

[Ján Tomko]

On Thu, Aug 16, 2018 at 11:11:26AM +0200, Simon Kobyda wrote:

rpdwrewrewr



Grafsjlankladhyr.

Wnab


Simon Kobyda (6):
 vsh: Add API for printing tables.
 virsh: Implement new table API for virsh list
 vsh: Added tests
 virsh: Implement vsh-table to iface-list
 virsh: Implement vshTable API to net-list and net-dhcp-leases
 virsh: Implement vshTable API to secret-list



signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 0/6] fdaskljrew

[Daniel P . Berrangé]

On Thu, Aug 16, 2018 at 11:11:26AM +0200, Simon Kobyda wrote:
> rpdwrewrewr

Please use a sensible subject line, and provide useful information about
the patch series in this cover letter.

> 
> Simon Kobyda (6):
>   vsh: Add API for printing tables.
>   virsh: Implement new table API for virsh list
>   vsh: Added tests
>   virsh: Implement vsh-table to iface-list
>   virsh: Implement vshTable API to net-list and net-dhcp-leases
>   virsh: Implement vshTable API to secret-list
> 
>  tests/Makefile.am|   8 +
>  tests/virshtest.c|  14 +-
>  tests/vshtabletest.c | 247 +
>  tools/Makefile.am|   4 +-
>  tools/virsh-domain-monitor.c |  43 ++--
>  tools/virsh-interface.c  |  27 ++-
>  tools/virsh-network.c|  50 +++--
>  tools/virsh-secret.c |  30 ++-
>  tools/vsh-table.c| 413 +++
>  tools/vsh-table.h|  42 
>  10 files changed, 817 insertions(+), 61 deletions(-)
>  create mode 100644 tests/vshtabletest.c
>  create mode 100644 tools/vsh-table.c
>  create mode 100644 tools/vsh-table.h
> 
> -- 
> 2.17.1
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 3/6] vsh: Added tests

[Simon Kobyda]

For now, there are 5 test cases
- testVshTableNew: Creating table with empty header
- testVshTableHeader: Printing table with/without header
- testVshTableRowAppend: Appending row with various number of cells.
  Only row with same number of cells as in header is accepted.
- testVshTableNewUnicode: Printing table with unicode characters.
  Checking correct alignment.
- testNTables: Create and print varios types of tables - one column,
  one row table, table without content, standart table...

Signed-off-by: Simon Kobyda 
---
 tests/Makefile.am|   8 ++
 tests/vshtabletest.c | 247 +++
 2 files changed, 255 insertions(+)
 create mode 100644 tests/vshtabletest.c

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 302b50e1cd..89c2965d86 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -206,6 +206,7 @@ test_programs = virshtest sockettest \
virhostdevtest \
virnetdevtest \
virtypedparamtest \
+   vshtabletest \
$(NULL)
 
 test_libraries = libshunload.la \
@@ -938,6 +939,13 @@ metadatatest_SOURCES = \
testutils.c testutils.h
 metadatatest_LDADD = $(LDADDS) $(LIBXML_LIBS)
 
+vshtabletest_SOURCES = \
+   vshtabletest.c \
+   testutils.c testutils.h
+vshtabletest_LDADD = \
+   $(LDADDS) \
+   ../tools/libvirt_shell.la
+
 virshtest_SOURCES = \
virshtest.c \
testutils.c testutils.h
diff --git a/tests/vshtabletest.c b/tests/vshtabletest.c
new file mode 100644
index 00..b41a205761
--- /dev/null
+++ b/tests/vshtabletest.c
@@ -0,0 +1,247 @@
+/*
+ * Copyright (C) 2018 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ */
+
+#include 
+
+#include 
+#include 
+#include 
+
+#include "internal.h"
+#include "testutils.h"
+#include "viralloc.h"
+#include "../tools/vsh-table.h"
+
+static int
+testVshTableNew(const void *opaque ATTRIBUTE_UNUSED)
+{
+int ret = 0;
+
+if (vshTableNew(NULL)) {
+fprintf(stderr, "expected failure when passing null to"
+"vshtablenew\n");
+ret = -1;
+}
+
+return ret;
+}
+
+static int
+testVshTableHeader(const void *opaque ATTRIBUTE_UNUSED)
+{
+int ret = 0;
+char *out;
+const char *exp = "\
+ 1   fedora28   running  \n\
+ 2   rhel7.5running  \n";
+const char *exp2 = "\
+ Id   Name   State\n\
+--\n\
+ 1fedora28   running  \n\
+ 2rhel7.5running  \n";
+
+vshTablePtr table = vshTableNew("Id", "Name", "State",
+NULL); //to ask about return
+if (!table)
+goto cleanup;
+
+vshTableRowAppend(table, "1", "fedora28", "running", NULL);
+vshTableRowAppend(table, "2", "rhel7.5", "running",
+  NULL);
+
+out = vshTablePrintToString(table, false);
+if (virTestCompareToString(exp, out) < 0)
+ret = -1;
+
+VIR_FREE(out);
+out = vshTablePrintToString(table, true);
+if (virTestCompareToString(exp2, out) < 0)
+ret = -1;
+
+ cleanup:
+VIR_FREE(out);
+vshTableFree(table);
+return ret;
+}
+
+static int
+testVshTableNewUnicode(const void *opaque ATTRIBUTE_UNUSED)
+{
+
+int ret = 0;
+char *out;
+
+char *locale = setlocale(LC_CTYPE, NULL);
+if (!setlocale(LC_CTYPE, "en_US.UTF-8"))
+return EXIT_AM_SKIP;
+
+const char *exp = "\
+ Id   名稱  государство  \n\
+-\n\
+ 1fedora28  running  \n\
+ 2rhel7.5   running  \n";
+vshTablePtr table;
+
+table = vshTableNew("Id", "名稱", "государство", NULL);
+if (!table)
+goto cleanup;
+
+vshTableRowAppend(table, "1", "fedora28", "running", NULL);
+vshTableRowAppend(table, "2", "rhel7.5", "running",
+  NULL);
+
+out = vshTablePrintToString(table, true);
+if (virTestCompareToString(exp, out) < 0)
+ret = -1;
+
+ cleanup:
+setlocale(LC_CTYPE, locale);
+VIR_FREE(out);
+vshTableFree(table);
+return ret;
+}
+
+static int
+testVshTableRowAppend(const void *opaque ATTRIBUTE_UNUSED)
+{
+int ret = 0;
+
+vshTablePtr table = vshTableNew("Id", "Name", NULL);
+if (!table)
+goto cleanup;
+
+if (vshTableRowAppend(table, NULL) >= 0) {
+fprintf(stderr, "Appending NULL