Re: [PATCH v2] libxl: adjust handling of libxl_device_nic objects

[Jim Fehlig]

On 5/19/21 3:37 AM, Olaf Hering wrote:

libxl objects are supposed to be initialized and disposed.
Adjust libxlMakeNic to use an already initialized object, it is owned by
the caller.

Adjust libxlMakeNicList to initialize the list of objects, before they
are filled by libxlMakeNic. In case of error the objects are disposed
by libxl_domain_config_dispose.

The usage of g_new0 is suspicious in the context of libxl because the
memory allocated via glib is released with plain free() inside libxl.

Signed-off-by: Olaf Hering 
---
  src/libxl/libxl_conf.c | 36 +---
  1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 2d2aab7e66..e4afa578b0 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1345,8 +1345,6 @@ libxlMakeNic(virDomainDef *def,
  return -1;
  }
  
-libxl_device_nic_init(x_nic);

-
  virMacAddrGetRaw(_nic->mac, x_nic->mac);
  
  /*

@@ -1532,39 +1530,39 @@ libxlMakeNicList(virDomainDef *def,  
libxl_domain_config *d_config)
  {
  virDomainNetDef **l_nics = def->nets;
  size_t nnics = def->nnets;
-libxl_device_nic *x_nics;
  size_t i, nvnics = 0;
-
-x_nics = g_new0(libxl_device_nic, nnics);
+int ret = -1;
  
  for (i = 0; i < nnics; i++) {

  if (virDomainNetGetActualType(l_nics[i]) == 
VIR_DOMAIN_NET_TYPE_HOSTDEV)
  continue;
+nvnics++;
+}
+if (!nvnics)
+return 0;
+
+d_config->nics = g_new0(libxl_device_nic, nvnics);
+d_config->num_nics = nvnics;
+
+for (i = 0; i < nvnics; i++)
+libxl_device_nic_init(_config->nics[i]);
  
-if (libxlMakeNic(def, l_nics[i], _nics[nvnics], false))

+for (i = 0; i < nvnics; i++) {
+if (libxlMakeNic(def, l_nics[i], _config->nics[i], false))
  goto error;
  /*
   * The devid (at least right now) will not get initialized by
   * libxl in the setup case but is required for starting the
   * device-model.
   */
-if (x_nics[nvnics].devid < 0)
-x_nics[nvnics].devid = nvnics;
-
-nvnics++;
+if (d_config->nics[i].devid < 0)
+d_config->nics[i].devid = i;
  }


Same comment as the disk patch: initializing and making the NIC can occur in a 
single loop.


  
-VIR_SHRINK_N(x_nics, nnics, nnics - nvnics);

-d_config->nics = x_nics;
-d_config->num_nics = nvnics;
-
-return 0;
+ret = 0;
  
   error:

-for (i = 0; i < nnics; i++)
-libxl_device_nic_dispose(_nics[i]);
-VIR_FREE(x_nics);
-return -1;
+return ret;


Error label can be dropped here too.

Regards,
Jim

Re: [PATCH v1] libxl: adjust handling of libxl_device_disk objects

[Jim Fehlig]

On 5/19/21 2:42 AM, Olaf Hering wrote:

libxl objects are supposed to be initialized and disposed.
Correct the usage of libxl_device_disk objects which are allocated on
the stack. Initialize each one prior usage, and dispose them once done.

Adjust libxlMakeDisk to use an already initialized object, it is owned
by the caller.

Adjust libxlMakeDiskList to initialize the list of objects, before they
are filled by libxlMakeDisk. In case of error, the objects are disposed
by libxl_domain_config_dispose.

The usage of g_new0 is suspicious in the context of libxl because the
memory allocated via glib is released with plain free() inside libxl.


After looking at the glib memory allocation doc again, I'm suspicious about your 
suspicion :-)


https://developer.gnome.org/glib/stable/glib-Memory-Allocation.html

I recall being confused about the text in the Description before. All is clear 
and I'm confident of my understanding - until the last tiny paragraph:


"Since GLib 2.46 g_malloc() is hardcoded to always use the system malloc 
implementation."


Err, what? Ok, cool, I guess it is fine to use plain free with g_new et.al.

IIRC this has been discussed on the list in the past, likely buried in some 
unrelated mail and difficult to find. Perhaps others can chime in and give us 
enough confidence to no longer be suspicious.




Signed-off-by: Olaf Hering 
---
  src/libxl/libxl_conf.c   | 22 +-
  src/libxl/libxl_driver.c |  6 ++
  2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 3149ee3b4a..2d2aab7e66 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1114,8 +1114,6 @@ libxlMakeDisk(virDomainDiskDef *l_disk, libxl_device_disk 
*x_disk)
  int format = virDomainDiskGetFormat(l_disk);
  int actual_type = virStorageSourceGetActualType(l_disk->src);
  
-libxl_device_disk_init(x_disk);

-
  if (actual_type == VIR_STORAGE_TYPE_NETWORK) {
  if (STRNEQ_NULLABLE(driver, "qemu")) {
  virReportError(VIR_ERR_OPERATION_INVALID, "%s",
@@ -1265,26 +1263,24 @@ libxlMakeDiskList(virDomainDef *def, 
libxl_domain_config *d_config)
  {
  virDomainDiskDef **l_disks = def->disks;
  int ndisks = def->ndisks;
-libxl_device_disk *x_disks;
  size_t i;
+int ret = -1;
+
+d_config->disks = g_new0(libxl_device_disk, ndisks);
+d_config->num_disks = ndisks;
  
-x_disks = g_new0(libxl_device_disk, ndisks);

+for (i = 0; i < ndisks; i++)
+libxl_device_disk_init(_config->disks[i]);
  
  for (i = 0; i < ndisks; i++) {

-if (libxlMakeDisk(l_disks[i], _disks[i]) < 0)
+if (libxlMakeDisk(l_disks[i], _config->disks[i]) < 0)
  goto error;
  }


Initializing and making the disk can be done in one loop.

  
-d_config->disks = x_disks;

-d_config->num_disks = ndisks;
-
-return 0;
+ret = 0;
  
   error:

-for (i = 0; i < ndisks; i++)
-libxl_device_disk_dispose(_disks[i]);
-VIR_FREE(x_disks);
-return -1;
+return ret;


You can drop the error label and return success or failure inline.

Regards,
Jim


  }
  
  /*

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index d54cd41785..2b844bb3b5 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -2978,6 +2978,7 @@ libxlDomainChangeEjectableMedia(virDomainObj *vm, 
virDomainDiskDef *disk)
  size_t i;
  int ret = -1;
  
+libxl_device_disk_init(_disk);

  for (i = 0; i < vm->def->ndisks; i++) {
  if (vm->def->disks[i]->bus == disk->bus &&
  STREQ(vm->def->disks[i]->dst, disk->dst)) {
@@ -3018,6 +3019,7 @@ libxlDomainChangeEjectableMedia(virDomainObj *vm, 
virDomainDiskDef *disk)
  ret = 0;
  
   cleanup:

+libxl_device_disk_dispose(_disk);
  virObjectUnref(cfg);
  return ret;
  }
@@ -3030,6 +3032,7 @@ libxlDomainAttachDeviceDiskLive(virDomainObj *vm, 
virDomainDeviceDef *dev)
  libxl_device_disk x_disk;
  int ret = -1;
  
+libxl_device_disk_init(_disk);

  switch (l_disk->device)  {
  case VIR_DOMAIN_DISK_DEVICE_CDROM:
  ret = libxlDomainChangeEjectableMedia(vm, l_disk);
@@ -3091,6 +3094,7 @@ libxlDomainAttachDeviceDiskLive(virDomainObj *vm, 
virDomainDeviceDef *dev)
  }
  
   cleanup:

+libxl_device_disk_dispose(_disk);
  virObjectUnref(cfg);
  return ret;
  }
@@ -3329,6 +,7 @@ libxlDomainDetachDeviceDiskLive(virDomainObj *vm, 
virDomainDeviceDef *dev)
  int idx;
  int ret = -1;
  
+libxl_device_disk_init(_disk);

  switch (dev->data.disk->device)  {
  case VIR_DOMAIN_DISK_DEVICE_DISK:
  if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_XEN) {
@@ -3380,6 +3385,7 @@ libxlDomainDetachDeviceDiskLive(virDomainObj *vm, 
virDomainDeviceDef *dev)
  }
  
   cleanup:

+libxl_device_disk_dispose(_disk);
  virObjectUnref(cfg);
  return ret;
  }

Re: [PATCH v1] libxl: remove libxl_domain_config_init from libxlBuildDomainConfig

[Jim Fehlig]

On 5/19/21 2:40 AM, Olaf Hering wrote:

The passed libxl_domain_config is owned, and already initialized, by the
caller.

Signed-off-by: Olaf Hering 
---
  src/libxl/libxl_conf.c | 1 -
  1 file changed, 1 deletion(-)


Reviewed-by: Jim Fehlig 



diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 8ea9b35292..3149ee3b4a 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -2529,7 +2529,6 @@ libxlBuildDomainConfig(virPortAllocatorRange 
*graphicsports,
  {
  virCaps *caps = cfg->caps;
  libxl_ctx *ctx = cfg->ctx;
-libxl_domain_config_init(d_config);
  
  if (libxlMakeDomCreateInfo(ctx, def, _config->c_info) < 0)

  return -1;

memory allocations for libraries used by libvirt

[Olaf Hering]

Currently src/libxl/ allocates a bunch of buffers with variants of g_new0() or 
g_strdup(), which will be consumed by libxenlight.so. Once the objects which 
contain these buffers are not needed anymore, libxenlight.so will release them 
with ordinary calls to free() in its *_dispose() API. In other words: 
libxenlight.so does not use glib.

While the g_malloc docs of today's glib state that (apparently) the mistake of 
using a private allocator was recognized and corrected in glib 2.46, the same 
mistake might occur again in the future.

I wonder if a patch will be accepted which will add simple wrappers around 
calloc, strdup, like libxlCallocWrap and libxlStrupWrap, which provides the 
buffers expected by libxenlight.so and which it can simply free() again. Just 
so that it looks more "symmetric", as opposed to g_new0()/free() pairs.

Maybe this issue was already evaluated at the time libvirt was converted from 
virAlloc to g_new?


Olaf


pgps1gv9wsRCi.pgp
Description: Digitale Signatur von OpenPGP

Re: [PATCH 3/4] conf: add s390-pv as launch security type

[Daniel Henrique Barboza]

On 5/19/21 4:34 PM, Daniel Henrique Barboza wrote:



On 5/19/21 2:40 PM, Boris Fiuczynski wrote:

Add launch security type 's390-pv' as well as some tests.

Signed-off-by: Boris Fiuczynski 
---
  docs/schemas/domaincommon.rng |  1 +
  src/conf/domain_conf.c    |  8 +
  src/conf/domain_conf.h    |  1 +
  src/qemu/qemu_command.c   | 26 ++
  src/qemu/qemu_namespace.c |  1 +
  src/qemu/qemu_process.c   |  1 +
  src/qemu/qemu_validate.c  |  8 +
  .../launch-security-s390-pv-ignore-policy.xml | 24 +
  .../launch-security-s390-pv.xml   | 18 ++
  .../launch-security-s390-pv-ignore-policy.xml |  1 +
  tests/genericxml2xmltest.c    |  2 ++
  ...ty-s390-pv-ignore-policy.s390x-latest.args | 35 +++
  .../launch-security-s390-pv-ignore-policy.xml | 33 +
  .../launch-security-s390-pv.s390x-latest.args | 35 +++
  .../launch-security-s390-pv.xml   | 30 
  tests/qemuxml2argvtest.c  |  3 ++
  16 files changed, 227 insertions(+)
  create mode 100644 
tests/genericxml2xmlindata/launch-security-s390-pv-ignore-policy.xml
  create mode 100644 tests/genericxml2xmlindata/launch-security-s390-pv.xml
  create mode 12 
tests/genericxml2xmloutdata/launch-security-s390-pv-ignore-policy.xml
  create mode 100644 
tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.s390x-latest.args
  create mode 100644 
tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.xml
  create mode 100644 
tests/qemuxml2argvdata/launch-security-s390-pv.s390x-latest.args
  create mode 100644 tests/qemuxml2argvdata/launch-security-s390-pv.xml

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 3df13a0cf1..7c92e4c812 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -485,6 +485,7 @@
    
  
    sev
+  s390-pv
  
    
    


You added a new 's390-pv' security type, but down there you're using
the new confidential-guest-support feature from QEMU 6.0 which is also
valid for AMD and pSeries. I think you can do a little change in the idea
of these patches while keeping most of it. Instead of calling this new
support 's390-pv', call it 'confidential-guest-support' or 'CGS'.

My reasoning is that the QEMU community (namely David Gibson, qemu-ppc
maintainer) went into a lot of discussions back and forth to develop the
confidential-guest-support machine option, based on what was at first AMD-SEV
specific code, with the intention of make it easier for users to enable
secure guests across machine types. I believe Libvirt should follow suit
and do the same - a single option to enable secure guest supports for
all guests, with any differences in the support being handled by each arch
deep down in the driver.

Otherwise, what will end up happening is that when someone (probably myself)
come along with the secure guest support for pSeries (PEF), I will need to
create yet another launch type 'ppc64-pef' to do basically the same thing you're
already doing for s390x, which is adding '-machine 
confidential-guest-support=<>'
in the QEMU command line. Same thing with AMD SEV, and with any other
arch that QEMU might support with the confidential-guest-support option. We're
going to add extra XML parsing code and docs to handle the same thing.

Note that I'm not asking you to go ahead and implement the Libvirt support for
all the 3 archs. What I'm asking is to change the name of the launch security
type in the domain XML and docs to reflect that this will be the same type
that all other archs that has confidential-guest-support will end up using.




Just remembered that there's an open bug related to the generic
confidential-guest-support implementation in Libvirt like I mentioned
above:


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



Pavel, CCing you since you're the current assignee of the bug.




Daniel






Thanks,


Daniel






diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 228de5d715..11ec8c8b0c 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1393,6 +1393,7 @@ VIR_ENUM_IMPL(virDomainLaunchSecurity,
    VIR_DOMAIN_LAUNCH_SECURITY_LAST,
    "",
    "sev",
+  "s390-pv",
  );
  static virClass *virDomainObjClass;
@@ -14762,6 +14763,8 @@ virDomainSecDefParseXML(xmlNodePtr lsecNode,
  if (!sec->sev)
  return NULL;
  break;
+    case VIR_DOMAIN_LAUNCH_SECURITY_PV:
+    break;
  case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
  case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
  default:
@@ -26896,6 +26899,11 @@ virDomainSecDefFormat(virBuffer *buf, virDomainSecDef 
*sec)
  break;
  }
+    case VIR_DOMAIN_LAUNCH_SECURITY_PV:
+   

Re: [PATCH 3/4] conf: add s390-pv as launch security type

[Daniel Henrique Barboza]

On 5/19/21 2:40 PM, Boris Fiuczynski wrote:

Add launch security type 's390-pv' as well as some tests.

Signed-off-by: Boris Fiuczynski 
---
  docs/schemas/domaincommon.rng |  1 +
  src/conf/domain_conf.c|  8 +
  src/conf/domain_conf.h|  1 +
  src/qemu/qemu_command.c   | 26 ++
  src/qemu/qemu_namespace.c |  1 +
  src/qemu/qemu_process.c   |  1 +
  src/qemu/qemu_validate.c  |  8 +
  .../launch-security-s390-pv-ignore-policy.xml | 24 +
  .../launch-security-s390-pv.xml   | 18 ++
  .../launch-security-s390-pv-ignore-policy.xml |  1 +
  tests/genericxml2xmltest.c|  2 ++
  ...ty-s390-pv-ignore-policy.s390x-latest.args | 35 +++
  .../launch-security-s390-pv-ignore-policy.xml | 33 +
  .../launch-security-s390-pv.s390x-latest.args | 35 +++
  .../launch-security-s390-pv.xml   | 30 
  tests/qemuxml2argvtest.c  |  3 ++
  16 files changed, 227 insertions(+)
  create mode 100644 
tests/genericxml2xmlindata/launch-security-s390-pv-ignore-policy.xml
  create mode 100644 tests/genericxml2xmlindata/launch-security-s390-pv.xml
  create mode 12 
tests/genericxml2xmloutdata/launch-security-s390-pv-ignore-policy.xml
  create mode 100644 
tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.s390x-latest.args
  create mode 100644 
tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.xml
  create mode 100644 
tests/qemuxml2argvdata/launch-security-s390-pv.s390x-latest.args
  create mode 100644 tests/qemuxml2argvdata/launch-security-s390-pv.xml

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 3df13a0cf1..7c92e4c812 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -485,6 +485,7 @@

  
sev
+  s390-pv
  




You added a new 's390-pv' security type, but down there you're using
the new confidential-guest-support feature from QEMU 6.0 which is also
valid for AMD and pSeries. I think you can do a little change in the idea
of these patches while keeping most of it. Instead of calling this new
support 's390-pv', call it 'confidential-guest-support' or 'CGS'.

My reasoning is that the QEMU community (namely David Gibson, qemu-ppc
maintainer) went into a lot of discussions back and forth to develop the
confidential-guest-support machine option, based on what was at first AMD-SEV
specific code, with the intention of make it easier for users to enable
secure guests across machine types. I believe Libvirt should follow suit
and do the same - a single option to enable secure guest supports for
all guests, with any differences in the support being handled by each arch
deep down in the driver.

Otherwise, what will end up happening is that when someone (probably myself)
come along with the secure guest support for pSeries (PEF), I will need to
create yet another launch type 'ppc64-pef' to do basically the same thing you're
already doing for s390x, which is adding '-machine 
confidential-guest-support=<>'
in the QEMU command line. Same thing with AMD SEV, and with any other
arch that QEMU might support with the confidential-guest-support option. We're
going to add extra XML parsing code and docs to handle the same thing.

Note that I'm not asking you to go ahead and implement the Libvirt support for
all the 3 archs. What I'm asking is to change the name of the launch security
type in the domain XML and docs to reflect that this will be the same type
that all other archs that has confidential-guest-support will end up using.


Thanks,


Daniel






diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 228de5d715..11ec8c8b0c 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1393,6 +1393,7 @@ VIR_ENUM_IMPL(virDomainLaunchSecurity,
VIR_DOMAIN_LAUNCH_SECURITY_LAST,
"",
"sev",
+  "s390-pv",
  );
  
  static virClass *virDomainObjClass;

@@ -14762,6 +14763,8 @@ virDomainSecDefParseXML(xmlNodePtr lsecNode,
  if (!sec->sev)
  return NULL;
  break;
+case VIR_DOMAIN_LAUNCH_SECURITY_PV:
+break;
  case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
  case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
  default:
@@ -26896,6 +26899,11 @@ virDomainSecDefFormat(virBuffer *buf, virDomainSecDef 
*sec)
  break;
  }
  
+case VIR_DOMAIN_LAUNCH_SECURITY_PV:

+virBufferAsprintf(buf, "\n",
+  virDomainLaunchSecurityTypeToString(sec->sectype));
+break;
+
  case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
  case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
  break;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index dd78f30ace..1d92065c7b 

Re: [PATCH 2/4] qemu: add s390-pv-guest capability

[Daniel Henrique Barboza]

On 5/19/21 2:40 PM, Boris Fiuczynski wrote:

Add s390-pv-guest capability.

Signed-off-by: Boris Fiuczynski 
---


Reviewed-by: Daniel Henrique Barboza 


  src/qemu/qemu_capabilities.c| 2 ++
  src/qemu/qemu_capabilities.h| 1 +
  tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml | 1 +
  3 files changed, 4 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index d3f24feb6a..3d0b552f62 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -629,6 +629,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
/* 400 */
"compat-deprecated",
"acpi-index",
+  "s390-pv-guest",
  );
  
  
@@ -1347,6 +1348,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {

  { "am53c974", QEMU_CAPS_SCSI_AM53C974 },
  { "virtio-pmem-pci", QEMU_CAPS_DEVICE_VIRTIO_PMEM_PCI },
  { "vhost-user-blk", QEMU_CAPS_DEVICE_VHOST_USER_BLK },
+{ "s390-pv-guest", QEMU_CAPS_S390_PV_GUEST },
  };
  
  
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h

index fae8492e69..b76798501b 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -609,6 +609,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
syntax-check */
  /* 400 */
  QEMU_CAPS_COMPAT_DEPRECATED, /* -compat deprecated-(input|output) is 
supported */
  QEMU_CAPS_ACPI_INDEX, /* PCI device 'acpi-index' property */
+QEMU_CAPS_S390_PV_GUEST, /* -object s390-pv-guest,... */
  
  QEMU_CAPS_LAST /* this must always be the last item */

  } virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml 
b/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml
index 4c056c484f..2f840073bc 100644
--- a/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml
+++ b/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml
@@ -164,6 +164,7 @@



+  
600
0
39100242

Re: [PATCH 1/4] conf: refactor launch security to allow more types

[Daniel Henrique Barboza]

On 5/19/21 2:40 PM, Boris Fiuczynski wrote:

To allow other types of launch security the SEV type specific
parameters like e.g. policy need to be optional and be separated
from other new launch security types.
A test is added to ensure the previously required and now optional
launch security policy remains required when launch security type
is SEV.



I think you missed a breakline up there.


Reviewed-by: Daniel Henrique Barboza 


Signed-off-by: Boris Fiuczynski 
---
  docs/schemas/domaincommon.rng |  12 +-
  src/conf/domain_conf.c| 156 +++---
  src/conf/domain_conf.h|  13 +-
  src/conf/virconftypes.h   |   2 +
  src/qemu/qemu_cgroup.c|   4 +-
  src/qemu/qemu_command.c   |  38 -
  src/qemu/qemu_driver.c|   2 +-
  src/qemu/qemu_firmware.c  |   4 +-
  src/qemu/qemu_namespace.c |  20 ++-
  src/qemu/qemu_process.c   |  35 +++-
  src/qemu/qemu_validate.c  |  22 ++-
  src/security/security_dac.c   |   4 +-
  ...urity-sev-missing-policy.x86_64-2.12.0.err |   1 +
  .../launch-security-sev-missing-policy.xml|  34 
  tests/qemuxml2argvtest.c  |   1 +
  15 files changed, 253 insertions(+), 95 deletions(-)
  create mode 100644 
tests/qemuxml2argvdata/launch-security-sev-missing-policy.x86_64-2.12.0.err
  create mode 100644 
tests/qemuxml2argvdata/launch-security-sev-missing-policy.xml

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index a2e5c50c1d..3df13a0cf1 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -483,7 +483,9 @@

  

-sev
+
+  sev
+


  
@@ -496,9 +498,11 @@
  

  
-
-  
-
+
+  
+
+  
+
  

  
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b3ed3a9c5a..228de5d715 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3481,8 +3481,7 @@ virDomainResctrlDefFree(virDomainResctrlDef *resctrl)
  }
  
  
-static void

-virDomainSEVDefFree(virDomainSEVDef *def)
+void virDomainSEVDefFree(virDomainSEVDef *def)
  {
  if (!def)
  return;
@@ -3493,6 +3492,17 @@ virDomainSEVDefFree(virDomainSEVDef *def)
  g_free(def);
  }
  
+void virDomainSecDefFree(virDomainSecDef *def)

+{
+if (!def)
+return;
+
+virDomainSEVDefFree(def->sev);
+
+g_free(def);
+}
+
+
  static void
  virDomainOSDefClear(virDomainOSDef *os)
  {
@@ -3694,7 +3704,7 @@ void virDomainDefFree(virDomainDef *def)
  if (def->namespaceData && def->ns.free)
  (def->ns.free)(def->namespaceData);
  
-virDomainSEVDefFree(def->sev);

+virDomainSecDefFree(def->sec);
  
  xmlFreeNode(def->metadata);
  
@@ -14688,72 +14698,80 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode,

  xmlXPathContextPtr ctxt)
  {
  VIR_XPATH_NODE_AUTORESTORE(ctxt)
-virDomainSEVDef *def;
+g_autoptr(virDomainSEVDef) sev = g_new0(virDomainSEVDef, 1);
  unsigned long policy;
-g_autofree char *type = NULL;
  int rc = -1;
  
-def = g_new0(virDomainSEVDef, 1);

-
  ctxt->node = sevNode;
  
-if (!(type = virXMLPropString(sevNode, "type"))) {

-virReportError(VIR_ERR_XML_ERROR, "%s",
-   _("missing launch security type"));
-goto error;
-}
-
-def->sectype = virDomainLaunchSecurityTypeFromString(type);
-switch ((virDomainLaunchSecurity) def->sectype) {
-case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
-break;
-case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
-case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
-default:
-virReportError(VIR_ERR_XML_ERROR,
-   _("unsupported launch security type '%s'"),
-   type);
-goto error;
-}
-
  if (virXPathULongHex("string(./policy)", ctxt, ) < 0) {
  virReportError(VIR_ERR_XML_ERROR, "%s",
-   _("failed to get launch security policy"));
-goto error;
+   _("Failed to get launch security policy for "
+ "launch security type SEV"));
+return NULL;
  }
  
  /* the following attributes are platform dependent and if missing, we can

   * autofill them from domain capabilities later
   */
-rc = virXPathUInt("string(./cbitpos)", ctxt, >cbitpos);
+rc = virXPathUInt("string(./cbitpos)", ctxt, >cbitpos);
  if (rc == 0) {
-def->haveCbitpos = true;
+sev->haveCbitpos = true;
  } else if (rc == -2) {
  virReportError(VIR_ERR_XML_ERROR, "%s",
 _("Invalid format for launch security cbitpos"));
-goto error;
+   

Re: Qemu block filter insertion/removal API

[Vladimir Sementsov-Ogievskiy]

19.05.2021 16:02, Kevin Wolf wrote:

Am 19.05.2021 um 14:19 hat Vladimir Sementsov-Ogievskiy geschrieben:

19.05.2021 14:44, Kevin Wolf wrote:

Am 17.05.2021 um 14:44 hat Vladimir Sementsov-Ogievskiy geschrieben:

Hi all!

I'd like to be sure that we know where we are going to.

In blockdev-era where qemu user is aware about block nodes, all nodes have good 
names and controlled by user we can efficiently use block filters.

We already have some useful filters: copy-on-read, throttling, compress. In my 
parallel series I make backup-top filter public and useful without backup block 
jobs. But now filters could be inserted only together with opening their child. 
We can specify filters in qemu cmdline, or filter can take place in the block 
node chain created by blockdev-add.

Still, it would be good to insert/remove filters on demand.

Currently we are going to use x-blockdev-reopen for this. Still it can't be used to 
insert a filter above root node (as x-blockdev-reopen can change only block node options 
and their children). In my series "[PATCH 00/21] block: publish backup-top 
filter" I propose (as Kevin suggested) to modify qom-set, so that it can set drive 
option of running device. That's not difficult, but it means that we have different 
scenario of inserting/removing filters:

1. filter above root node X:

inserting:

- do blockdev-add to add a filter (and specify X as its child)
- do qom-set to set new filter as a rood node instead of X

removing

- do qom-set to make X a root node again
- do blockdev-del to drop a filter

2. filter between two block nodes P and X. (For example, X is a backing child 
of P)

inserting

- do blockdev-add to add a filter (and specify X as its child)
- do blockdev-reopen to set P.backing = filter

remvoing

- do blockdev-reopen to set P.backing = X
- do blockdev-del to drop a filter


And, probably we'll want transaction support for all these things.


Is it OK? Or do we need some kind of additional blockdev-replace command, that 
can replace one node by another, so in both cases we will do

inserting:
- blockdev-add filter
- blockdev-replace (make all parents of X to point to the new filter 
instead (except for the filter itself of course)

removing
- blockdev-replace (make all parante of filter to be parents of X instead)
- blockdev-del filter

It's simple to implement, and it seems for me that it is simpler to use. Any 
thoughts?


One reason I remember why we didn't decide to go this way in the many
"dynamic graph reconfiguration" discussions we had, is that it's not
generic enough to cover all cases. But I'm not sure if we ever
considered root nodes as a separate case. I acknowledge that having two
different interfaces is inconvenient, and integrating qom-set in a
transaction is rather unlikely to happen.

The reason why it's not generic is that it restricts you to doing the
same thing for all parents. Imagine this:

  +- virtio-blk
  |
  file <- qcow2 <-+
  |
  +- NBD export

Now you want to throttle the NBD export so that it doesn't interfere
with your VM too much. With your simple blockdev-replace this isn't
possible. You would have to add the filter to both users or to none.

In theory, blockdev-replace could take a list of the edges that should
be changed to the new node. The problem is that edges don't have names,
and even the parents don't necessarily have one (and if they do, they
are in separate namespaces, so a BlockBackend, a job and an export could
all have the same name), so finding a good way to refer to them in QMP
doesn't sound trivial.



Hm. I like the idea. And it seems feasible to me:

Both export and block jobs works through BlockBackend.

So, for block-jobs, we can add optional parameters like
source-blk-name, and target-blk-name. If parameters specified, blk's
will be named, and user will be able to do blockdev-replace.


I'm not sure if giving them a name is a good idea. Wouldn't it make the
BlockBackend accessible for the user who could then make a device use
it?


For export it's a bit trickier: it would be strange to add separate
argument for export blk, as export already has id. So, I'd do the
following:

1. make blk named (with same name as the export itself) iff name does
not conflict with other blks
2. deprecate duplicating existing blk names by export name.


Yes, if we decide that giving them a name is a good idea, it's possible,
but still a change that requires deprecation, as you say.

The third one is devices (which is what I actually meant when I said
BlockBackend), which also have anonymous BlockBackends in the -blockdev
world. The same approach could work, but it would essentially mean
unifying the QOM and the block namespace, which sounds more likely to
produce conflicts than exports.


Then, blockdev-replace take a parents list, where parent is either
node-name or blk name.


Note 

[PATCH 3/4] conf: add s390-pv as launch security type

[Boris Fiuczynski]

Add launch security type 's390-pv' as well as some tests.

Signed-off-by: Boris Fiuczynski 
---
 docs/schemas/domaincommon.rng |  1 +
 src/conf/domain_conf.c|  8 +
 src/conf/domain_conf.h|  1 +
 src/qemu/qemu_command.c   | 26 ++
 src/qemu/qemu_namespace.c |  1 +
 src/qemu/qemu_process.c   |  1 +
 src/qemu/qemu_validate.c  |  8 +
 .../launch-security-s390-pv-ignore-policy.xml | 24 +
 .../launch-security-s390-pv.xml   | 18 ++
 .../launch-security-s390-pv-ignore-policy.xml |  1 +
 tests/genericxml2xmltest.c|  2 ++
 ...ty-s390-pv-ignore-policy.s390x-latest.args | 35 +++
 .../launch-security-s390-pv-ignore-policy.xml | 33 +
 .../launch-security-s390-pv.s390x-latest.args | 35 +++
 .../launch-security-s390-pv.xml   | 30 
 tests/qemuxml2argvtest.c  |  3 ++
 16 files changed, 227 insertions(+)
 create mode 100644 
tests/genericxml2xmlindata/launch-security-s390-pv-ignore-policy.xml
 create mode 100644 tests/genericxml2xmlindata/launch-security-s390-pv.xml
 create mode 12 
tests/genericxml2xmloutdata/launch-security-s390-pv-ignore-policy.xml
 create mode 100644 
tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.s390x-latest.args
 create mode 100644 
tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.xml
 create mode 100644 
tests/qemuxml2argvdata/launch-security-s390-pv.s390x-latest.args
 create mode 100644 tests/qemuxml2argvdata/launch-security-s390-pv.xml

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 3df13a0cf1..7c92e4c812 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -485,6 +485,7 @@
   
 
   sev
+  s390-pv
 
   
   
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 228de5d715..11ec8c8b0c 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1393,6 +1393,7 @@ VIR_ENUM_IMPL(virDomainLaunchSecurity,
   VIR_DOMAIN_LAUNCH_SECURITY_LAST,
   "",
   "sev",
+  "s390-pv",
 );
 
 static virClass *virDomainObjClass;
@@ -14762,6 +14763,8 @@ virDomainSecDefParseXML(xmlNodePtr lsecNode,
 if (!sec->sev)
 return NULL;
 break;
+case VIR_DOMAIN_LAUNCH_SECURITY_PV:
+break;
 case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
 case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
 default:
@@ -26896,6 +26899,11 @@ virDomainSecDefFormat(virBuffer *buf, virDomainSecDef 
*sec)
 break;
 }
 
+case VIR_DOMAIN_LAUNCH_SECURITY_PV:
+virBufferAsprintf(buf, "\n",
+  virDomainLaunchSecurityTypeToString(sec->sectype));
+break;
+
 case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
 case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
 break;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index dd78f30ace..1d92065c7b 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2631,6 +2631,7 @@ struct _virDomainKeyWrapDef {
 typedef enum {
 VIR_DOMAIN_LAUNCH_SECURITY_NONE,
 VIR_DOMAIN_LAUNCH_SECURITY_SEV,
+VIR_DOMAIN_LAUNCH_SECURITY_PV,
 
 VIR_DOMAIN_LAUNCH_SECURITY_LAST,
 } virDomainLaunchSecurity;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 10dcf11d5b..67024f99b9 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6992,6 +6992,9 @@ qemuBuildMachineCommandLine(virCommand *cmd,
 case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
 virBufferAddLit(, ",memory-encryption=sev0");
 break;
+case VIR_DOMAIN_LAUNCH_SECURITY_PV:
+virBufferAddLit(, ",confidential-guest-support=pv0");
+break;
 case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
 break;
 case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
@@ -9879,6 +9882,26 @@ qemuBuildSEVCommandLine(virDomainObj *vm, virCommand 
*cmd,
 }
 
 
+static int
+qemuBuildPVCommandLine(virDomainObj *vm, virCommand *cmd)
+{
+g_autoptr(virJSONValue) props = NULL;
+g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
+qemuDomainObjPrivate *priv = vm->privateData;
+
+if (qemuMonitorCreateObjectProps(, "s390-pv-guest", "pv0",
+ NULL) < 0)
+return -1;
+
+if (qemuBuildObjectCommandlineFromJSON(, props, priv->qemuCaps) < 0)
+return -1;
+
+virCommandAddArg(cmd, "-object");
+virCommandAddArgBuffer(cmd, );
+return 0;
+}
+
+
 static int
 qemuBuildSecCommandLine(virDomainObj *vm, virCommand *cmd,
 virDomainSecDef *sec)
@@ -9890,6 +9913,9 @@ qemuBuildSecCommandLine(virDomainObj *vm, virCommand *cmd,
 case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
 return qemuBuildSEVCommandLine(vm, cmd, sec->sev);
 

[PATCH 1/4] conf: refactor launch security to allow more types

[Boris Fiuczynski]

To allow other types of launch security the SEV type specific
parameters like e.g. policy need to be optional and be separated
from other new launch security types.
A test is added to ensure the previously required and now optional
launch security policy remains required when launch security type
is SEV.

Signed-off-by: Boris Fiuczynski 
---
 docs/schemas/domaincommon.rng |  12 +-
 src/conf/domain_conf.c| 156 +++---
 src/conf/domain_conf.h|  13 +-
 src/conf/virconftypes.h   |   2 +
 src/qemu/qemu_cgroup.c|   4 +-
 src/qemu/qemu_command.c   |  38 -
 src/qemu/qemu_driver.c|   2 +-
 src/qemu/qemu_firmware.c  |   4 +-
 src/qemu/qemu_namespace.c |  20 ++-
 src/qemu/qemu_process.c   |  35 +++-
 src/qemu/qemu_validate.c  |  22 ++-
 src/security/security_dac.c   |   4 +-
 ...urity-sev-missing-policy.x86_64-2.12.0.err |   1 +
 .../launch-security-sev-missing-policy.xml|  34 
 tests/qemuxml2argvtest.c  |   1 +
 15 files changed, 253 insertions(+), 95 deletions(-)
 create mode 100644 
tests/qemuxml2argvdata/launch-security-sev-missing-policy.x86_64-2.12.0.err
 create mode 100644 
tests/qemuxml2argvdata/launch-security-sev-missing-policy.xml

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index a2e5c50c1d..3df13a0cf1 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -483,7 +483,9 @@
   
 
   
-sev
+
+  sev
+
   
   
 
@@ -496,9 +498,11 @@
 
   
 
-
-  
-
+
+  
+
+  
+
 
   
 
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b3ed3a9c5a..228de5d715 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3481,8 +3481,7 @@ virDomainResctrlDefFree(virDomainResctrlDef *resctrl)
 }
 
 
-static void
-virDomainSEVDefFree(virDomainSEVDef *def)
+void virDomainSEVDefFree(virDomainSEVDef *def)
 {
 if (!def)
 return;
@@ -3493,6 +3492,17 @@ virDomainSEVDefFree(virDomainSEVDef *def)
 g_free(def);
 }
 
+void virDomainSecDefFree(virDomainSecDef *def)
+{
+if (!def)
+return;
+
+virDomainSEVDefFree(def->sev);
+
+g_free(def);
+}
+
+
 static void
 virDomainOSDefClear(virDomainOSDef *os)
 {
@@ -3694,7 +3704,7 @@ void virDomainDefFree(virDomainDef *def)
 if (def->namespaceData && def->ns.free)
 (def->ns.free)(def->namespaceData);
 
-virDomainSEVDefFree(def->sev);
+virDomainSecDefFree(def->sec);
 
 xmlFreeNode(def->metadata);
 
@@ -14688,72 +14698,80 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode,
 xmlXPathContextPtr ctxt)
 {
 VIR_XPATH_NODE_AUTORESTORE(ctxt)
-virDomainSEVDef *def;
+g_autoptr(virDomainSEVDef) sev = g_new0(virDomainSEVDef, 1);
 unsigned long policy;
-g_autofree char *type = NULL;
 int rc = -1;
 
-def = g_new0(virDomainSEVDef, 1);
-
 ctxt->node = sevNode;
 
-if (!(type = virXMLPropString(sevNode, "type"))) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
-   _("missing launch security type"));
-goto error;
-}
-
-def->sectype = virDomainLaunchSecurityTypeFromString(type);
-switch ((virDomainLaunchSecurity) def->sectype) {
-case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
-break;
-case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
-case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
-default:
-virReportError(VIR_ERR_XML_ERROR,
-   _("unsupported launch security type '%s'"),
-   type);
-goto error;
-}
-
 if (virXPathULongHex("string(./policy)", ctxt, ) < 0) {
 virReportError(VIR_ERR_XML_ERROR, "%s",
-   _("failed to get launch security policy"));
-goto error;
+   _("Failed to get launch security policy for "
+ "launch security type SEV"));
+return NULL;
 }
 
 /* the following attributes are platform dependent and if missing, we can
  * autofill them from domain capabilities later
  */
-rc = virXPathUInt("string(./cbitpos)", ctxt, >cbitpos);
+rc = virXPathUInt("string(./cbitpos)", ctxt, >cbitpos);
 if (rc == 0) {
-def->haveCbitpos = true;
+sev->haveCbitpos = true;
 } else if (rc == -2) {
 virReportError(VIR_ERR_XML_ERROR, "%s",
_("Invalid format for launch security cbitpos"));
-goto error;
+return NULL;
 }
 
 rc = virXPathUInt("string(./reducedPhysBits)", ctxt,
-  >reduced_phys_bits);
+  >reduced_phys_bits);
 if (rc == 0) {
-

[PATCH 0/4] Support for launchSecurity type s390-pv

[Boris Fiuczynski]

This patch series introduces the launch security type s390-pv.
Specifying s390-pv as launch security type in an s390 domain prepares for
running the guest in protected virtualization secure mode, also known as
IBM Secure Execution.

Boris Fiuczynski (4):
  conf: refactor launch security to allow more types
  qemu: add s390-pv-guest capability
  conf: add s390-pv as launch security type
  docs: add s390-pv documentation

 docs/formatdomain.rst |   7 +
 docs/kbase/s390_protected_virt.rst|  55 +-
 docs/schemas/domaincommon.rng |  13 +-
 src/conf/domain_conf.c| 164 +++---
 src/conf/domain_conf.h|  14 +-
 src/conf/virconftypes.h   |   2 +
 src/qemu/qemu_capabilities.c  |   2 +
 src/qemu/qemu_capabilities.h  |   1 +
 src/qemu/qemu_cgroup.c|   4 +-
 src/qemu/qemu_command.c   |  64 ++-
 src/qemu/qemu_driver.c|   2 +-
 src/qemu/qemu_firmware.c  |   4 +-
 src/qemu/qemu_namespace.c |  21 ++-
 src/qemu/qemu_process.c   |  36 +++-
 src/qemu/qemu_validate.c  |  30 +++-
 src/security/security_dac.c   |   4 +-
 .../launch-security-s390-pv-ignore-policy.xml |  24 +++
 .../launch-security-s390-pv.xml   |  18 ++
 .../launch-security-s390-pv-ignore-policy.xml |   1 +
 tests/genericxml2xmltest.c|   2 +
 .../qemucapabilitiesdata/caps_6.0.0.s390x.xml |   1 +
 ...ty-s390-pv-ignore-policy.s390x-latest.args |  35 
 .../launch-security-s390-pv-ignore-policy.xml |  33 
 .../launch-security-s390-pv.s390x-latest.args |  35 
 .../launch-security-s390-pv.xml   |  30 
 ...urity-sev-missing-policy.x86_64-2.12.0.err |   1 +
 .../launch-security-sev-missing-policy.xml|  34 
 tests/qemuxml2argvtest.c  |   4 +
 28 files changed, 538 insertions(+), 103 deletions(-)
 create mode 100644 
tests/genericxml2xmlindata/launch-security-s390-pv-ignore-policy.xml
 create mode 100644 tests/genericxml2xmlindata/launch-security-s390-pv.xml
 create mode 12 
tests/genericxml2xmloutdata/launch-security-s390-pv-ignore-policy.xml
 create mode 100644 
tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.s390x-latest.args
 create mode 100644 
tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.xml
 create mode 100644 
tests/qemuxml2argvdata/launch-security-s390-pv.s390x-latest.args
 create mode 100644 tests/qemuxml2argvdata/launch-security-s390-pv.xml
 create mode 100644 
tests/qemuxml2argvdata/launch-security-sev-missing-policy.x86_64-2.12.0.err
 create mode 100644 
tests/qemuxml2argvdata/launch-security-sev-missing-policy.xml

-- 
2.30.2

[PATCH 4/4] docs: add s390-pv documentation

[Boris Fiuczynski]

Add documentation for launch security type s390-pv.

Signed-off-by: Boris Fiuczynski 
---
 docs/formatdomain.rst  |  7 
 docs/kbase/s390_protected_virt.rst | 55 +-
 2 files changed, 54 insertions(+), 8 deletions(-)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index fa5c14febc..635fd5a55f 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -8044,6 +8044,13 @@ Note: DEA/TDEA is synonymous with DES/TDES.
 Launch Security
 ---
 
+Specifying  in an s390 VM prepares for
+running the guest in protected virtualization secure mode, also known as
+IBM Secure Execution. For more required host and guest preparation steps, see
+`Protected Virtualization on s390 `__
+:since:`Since 7.4.0`
+
+
 The contents of the  element is used to provide
 the guest owners input used for creating an encrypted VM using the AMD SEV
 feature (Secure Encrypted Virtualization). SEV is an extension to the AMD-V
diff --git a/docs/kbase/s390_protected_virt.rst 
b/docs/kbase/s390_protected_virt.rst
index 1718a556d4..5c9ce0e21c 100644
--- a/docs/kbase/s390_protected_virt.rst
+++ b/docs/kbase/s390_protected_virt.rst
@@ -127,10 +127,13 @@ Protected virtualization guests support I/O using virtio 
devices.
 As the virtio data structures of secure guests are not accessible
 by the host, it is necessary to use shared memory ('bounce buffers').
 
-To enable virtio devices to use shared buffers, it is necessary
-to configure them with platform_iommu enabled. This can done by adding
-``iommu='on'`` to the driver element of a virtio device definition in the
-guest's XML, e.g.
+Since libvirt 7.4.0 the
+` `__
+element with type ``s390-pv`` should be used on protected virtualization 
guests.
+Without ``launchSecurity`` you must enable all virtio devices to use shared
+buffers by configuring them with platform_iommu enabled.
+This can done by adding ``iommu='on'`` to the driver element of a virtio
+device definition in the guest's XML, e.g.
 
 ::
 
@@ -140,8 +143,10 @@ guest's XML, e.g.
  

 
-It is mandatory to define all virtio bus devices in this way to
-prevent the host from attempting to access protected memory.
+Unless you are using ``launchSecurity`` you must define all virtio bus
+devices in this way to prevent the host from attempting to access
+protected memory.
+
 Ballooning will not work and is fenced by QEMU. It should be
 disabled by specifying
 
@@ -158,8 +163,42 @@ allocated 2K entries. A commonly used value for swiotlb is 
262144.
 Example guest definition
 
 
-Minimal domain XML for a protected virtualization guest, essentially
-it's mostly about the ``iommu`` property
+Minimal domain XML for a protected virtualization guest with
+the ``launchSecurity`` element of type ``s390-pv``
+
+::
+
+   
+ protected
+ 2048000
+ 2048000
+ 1
+ 
+   hvm
+ 
+ 
+ 
+   
+ 
+ 
+ 
+   
+   
+ 
+ 
+   
+   
+   
+ 
+ 
+   
+
+
+Example guest definition without launchSecurity
+===
+
+Minimal domain XML for a protected virtualization guest using the
+``iommu='on'`` setting for each virtio device.
 
 ::
 
-- 
2.30.2

[PATCH 2/4] qemu: add s390-pv-guest capability

[Boris Fiuczynski]

Add s390-pv-guest capability.

Signed-off-by: Boris Fiuczynski 
---
 src/qemu/qemu_capabilities.c| 2 ++
 src/qemu/qemu_capabilities.h| 1 +
 tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml | 1 +
 3 files changed, 4 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index d3f24feb6a..3d0b552f62 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -629,6 +629,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
   /* 400 */
   "compat-deprecated",
   "acpi-index",
+  "s390-pv-guest",
 );
 
 
@@ -1347,6 +1348,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
 { "am53c974", QEMU_CAPS_SCSI_AM53C974 },
 { "virtio-pmem-pci", QEMU_CAPS_DEVICE_VIRTIO_PMEM_PCI },
 { "vhost-user-blk", QEMU_CAPS_DEVICE_VHOST_USER_BLK },
+{ "s390-pv-guest", QEMU_CAPS_S390_PV_GUEST },
 };
 
 
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index fae8492e69..b76798501b 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -609,6 +609,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
syntax-check */
 /* 400 */
 QEMU_CAPS_COMPAT_DEPRECATED, /* -compat deprecated-(input|output) is 
supported */
 QEMU_CAPS_ACPI_INDEX, /* PCI device 'acpi-index' property */
+QEMU_CAPS_S390_PV_GUEST, /* -object s390-pv-guest,... */
 
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml 
b/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml
index 4c056c484f..2f840073bc 100644
--- a/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml
+++ b/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml
@@ -164,6 +164,7 @@
   
   
   
+  
   600
   0
   39100242
-- 
2.30.2

Re: [PATCH] conf: Report alias name of the detached device in error

[Laine Stump]

On 5/19/21 8:37 AM, Kristina Hanicova wrote:



On Wed, May 19, 2021 at 9:58 AM Michal Prívozník > wrote:


On 5/18/21 6:07 PM, Laine Stump wrote:
 > On 5/18/21 5:44 AM, Kristina Hanicova wrote:
 >> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1942367

 >>
 >> Signed-off-by: Kristina Hanicova mailto:khani...@redhat.com>>
 >> ---
 >>   src/conf/domain_conf.c | 19 +--
 >>   1 file changed, 13 insertions(+), 6 deletions(-)
 >>
 >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
 >> index 7044701fac..e21b9fb946 100644
 >> --- a/src/conf/domain_conf.c
 >> +++ b/src/conf/domain_conf.c
 >> @@ -15781,38 +15781,45 @@ virDomainNetFindIdx(virDomainDef *def,
 >> virDomainNetDef *net)
 >>   if (matchidx < 0) {
 >>   if (MACAddrSpecified && PCIAddrSpecified) {
 >>   virReportError(VIR_ERR_DEVICE_MISSING,
 >> -   _("no device matching MAC address %s
found
 >> on "
 >> +   _("no device matching MAC address %s and
 >> alias %s found on "
 >>    VIR_PCI_DEVICE_ADDRESS_FMT),
 >>  virMacAddrFormat(>mac, mac),
 >> +   NULLSTR(net->info.alias),
 >>  net->info.addr.pci.domain,
 >>  net->info.addr.pci.bus,
 >>  net->info.addr.pci.slot,
 >>  net->info.addr.pci.function);
 >>   } else if (MACAddrSpecified && CCWAddrSpecified) {
 >>   virReportError(VIR_ERR_DEVICE_MISSING,
 >> -   _("no device matching MAC address %s
found
 >> on "
 >> +   _("no device matching MAC address %s and
 >> alias %s  found on "
 >
 > These messages will look strange in the (most common) case where
alias
 > isn't specified, e.g.:
 >
 >     no device matching MAC address DE:AD:BE:EF:01:10
 >     and alias   found on [some CCW address]
 >
 > On the other hand, the idea of even further exploding this bunch of
 > conditionals to include all combinations is just horrible to
think about!
 >
 > What about instead reworking this to use a single
virReportError() that
 > references a few pointers setup beforehand and then substituting (a
 > properly i8n'ized!) "(unspecified)" for each item that hasn't been
 > specified, e.g.:
 >
 > g_autofree *addr = g_strdup(_("(unspecified)"));
 > const char *mac = _("(unspecified)");
 > const char *alias = _("(unspecified)");
 >
 > if (MACAddrSpecified)
 >     mac = virMacAddrFormat(>mac, mac);
 > if (net->info.alias)
 >     alias = net->info.alias
 >
 > if (CCWAddrSpecified)
 >    addr = virCCWAddressAsString(blah);
 > else if (PCIAddrSpecified)
 >    addr = virPCIDeviceAddressAsString(blah);
 >
 > virReportError(blah...
 >    _("no device found at address '%s' matching MAC
address
 > '%s' and alias '%s'"),
 >    addr, mac, alias);
 >
 > or something like that. It's still not ideal, but avoids the
conditional
 > explosion and I think is less confusing than having "alias"
followed by
 > nothing.

IIUC, NULLSTR() will expand to "" not an empty string.


Derp. Oh yeah, you're right!


"unspecified" sounds better. What I worry about is translations: in my
native language and it's not a problem to have the error message split
as you suggest. But maybe there are some languages where it might be
problem?


I think if it was grammatically a part of the sentence (like the verb or 
something) it would be problematic since the ordering might be wrong 
when translated, but otherwise it should be okay.


Actually having  make Kristina's patch seem much less problematic 
to me. It would be nice to use this opportunity to eliminate the big 
chain of different log messages inside if clauses though.





On the other hand - we can go with your suggestion and change this later
as soon as we learn it's problematic for translators.

Kristina, what's your opinion?

Michal


I think that it can be reworked in a way, that we will have a bool 
variable for

each item that can fail, e.g.:

bool aliasMatched = true;
bool addrMatched = true;
bool macMatched = true;

And we would set the corresponding variable to false if they did not match
before continuing. When reporting error, we would only report the one 
last thing

it specifically failed on:

if (!aliasMatched)
     virReportError(VIR_ERR_DEVICE_MISSING,
                    _("no device matching alias %s found"),
                    net->info.alias);

And so on.
But, it might be 

Re: [libvirt PATCH 1/2] storage: add support for QCOW2 cluster_size option

[Pavel Hrdina]

On Wed, May 19, 2021 at 02:49:14PM +0200, Peter Krempa wrote:
> On Thu, May 13, 2021 at 13:23:04 +0200, Pavel Hrdina wrote:
> > The default value hard-coded in QEMU (64KiB) is not always the ideal.
> > Having a possibility to set the cluster_size by user may in specific
> > use-cases improve performance for QCOW2 images.
> > 
> > QEMU internally has some limits, the value has to be between 512B and
> > 2048KiB and must by power of two, except when the image has Extended L2
> > Entries the minimal value has to be 16KiB.
> > 
> > Since qemu-img ensures the value is correct and the limit is not always
> > the same libvirt will not duplicate any of these checks as the error
> > message from qemu-img is good enough:
> > 
> > Cluster size must be a power of two between 512 and 2048k
> > 
> > Resolves: https://gitlab.com/libvirt/libvirt/-/issues/154
> > 
> > Signed-off-by: Pavel Hrdina 
> > ---
> 
> [...]
> 
> > @@ -867,6 +868,11 @@
> >  the file image is used in VM. To create non-raw file images, it
> >  requires QEMU version since 2.1. Since 
> > 1.2.7
> >
> > +  clusterSize
> > +   Changes the qcow2 cluster size where smaller size can improve 
> > image
> > +file size whereas larger size can improve performance.
> > +Since 7.4.0
> 
> I think we should refrain from advice on how the cluster size can be
> configured because it has way more nuance than this short sentence.

Reasonable, I can drop it as I was just rephrasing what qemu has in
their documentation.

> > +  
> >features
> >Format-specific features. Only used for qcow2 now.
> >  Valid sub-elements are:
> 
> 
> 
> > diff --git a/tests/storagevolxml2xmlin/vol-qcow2-clusterSize.xml 
> > b/tests/storagevolxml2xmlin/vol-qcow2-clusterSize.xml
> > new file mode 100644
> > index 00..22534982a1
> > --- /dev/null
> > +++ b/tests/storagevolxml2xmlin/vol-qcow2-clusterSize.xml
> > @@ -0,0 +1,17 @@
> > +
> > +  OtherDemo.img
> > +  /var/lib/libvirt/images/OtherDemo.img
> > +  5
> > +  294912
> > +  
> > +/var/lib/libvirt/images/OtherDemo.img
> > +
> 
> I wonder whether it wouldn't make more sense to stash cluster size under
> the 'format' tag.
> 
> Does the LVM volume XML have  ?

No it doesn't have it. Only for pool XML. In addition currently it has
only path attribute.

For qcow2 we have most of the options represented outside the 
element. I'm not saying it's correct as I cannot decide which place
would be better, but following what we already do with other qcow2
options I figured we should do the same here.

It is definitely possible to have it in .

Pavel

> > +
> > +  0644
> > +  0
> > +  0
> > +  unconfined_u:object_r:virt_image_t:s0
> > +
> > +128
> > +  
> > +


signature.asc
Description: PGP signature

Re: [libvirt PATCH 2/2] storage_file: add support to probe cluster_size from QCOW2 images

[Pavel Hrdina]

On Wed, May 19, 2021 at 02:57:18PM +0200, Peter Krempa wrote:
> On Thu, May 13, 2021 at 13:23:05 +0200, Pavel Hrdina wrote:
> > >From QEMU docs/interop/qcow2.txt :
> > 
> >Byte  20 - 23:   cluster_bits
> > Number of bits that are used for addressing an offset
> > within a cluster (1 << cluster_bits is the cluster 
> > size).
> > 
> > With this patch libvirt will be able to report the current cluster_size
> > for all existing storage volumes managed by storage driver.
> > 
> > Signed-off-by: Pavel Hrdina 
> > ---
> >  src/storage/storage_util.c|  3 ++
> >  src/storage_file/storage_file_probe.c | 55 +++
> >  2 files changed, 42 insertions(+), 16 deletions(-)
> 
> [...]
> 
> > diff --git a/src/storage_file/storage_file_probe.c 
> > b/src/storage_file/storage_file_probe.c
> > index afe64da02e..423597049f 100644
> > --- a/src/storage_file/storage_file_probe.c
> > +++ b/src/storage_file/storage_file_probe.c
> > @@ -94,6 +94,9 @@ struct FileTypeInfo {
> >/* Store a COW base image path (possibly 
> > relative),
> > * or NULL if there is no COW base image, to RES;
> > * return BACKING_STORE_* */
> > +int clusterBitsOffset; /* Byte offset from start of file where we find
> > +* number of cluster bits, -1 to skip. */
> > +int clusterBitsSize;   /* Number of bytes for cluster bits. */
> 
> This isn't actually used [1].

Good point, I'll drop it.

> >  const struct FileEncryptionInfo *cryptInfo; /* Encryption info */
> >  int (*getBackingStore)(char **res, int *format,
> > const char *buf, size_t buf_size);
> 
> Given how the value is stored in the header it seems that it won't
> really be usable for any other format. I think we should parse it via a
> callback rather than the generic offset/size parser.
> 
> Specifically since the cluster size is stored as number of bits.

That will be way better then mangling with the crazy struct. I'll change
it for v2.

Thanks for the review, I'll make the changes and once we figure out the
placement of cluster_size I'll post v2.

Pavel


signature.asc
Description: PGP signature

Re: Qemu block filter insertion/removal API

[Vladimir Sementsov-Ogievskiy]

19.05.2021 14:44, Kevin Wolf wrote:

Am 17.05.2021 um 14:44 hat Vladimir Sementsov-Ogievskiy geschrieben:

Hi all!

I'd like to be sure that we know where we are going to.

In blockdev-era where qemu user is aware about block nodes, all nodes have good 
names and controlled by user we can efficiently use block filters.

We already have some useful filters: copy-on-read, throttling, compress. In my 
parallel series I make backup-top filter public and useful without backup block 
jobs. But now filters could be inserted only together with opening their child. 
We can specify filters in qemu cmdline, or filter can take place in the block 
node chain created by blockdev-add.

Still, it would be good to insert/remove filters on demand.

Currently we are going to use x-blockdev-reopen for this. Still it can't be used to 
insert a filter above root node (as x-blockdev-reopen can change only block node options 
and their children). In my series "[PATCH 00/21] block: publish backup-top 
filter" I propose (as Kevin suggested) to modify qom-set, so that it can set drive 
option of running device. That's not difficult, but it means that we have different 
scenario of inserting/removing filters:

1. filter above root node X:

inserting:

   - do blockdev-add to add a filter (and specify X as its child)
   - do qom-set to set new filter as a rood node instead of X

removing

   - do qom-set to make X a root node again
   - do blockdev-del to drop a filter

2. filter between two block nodes P and X. (For example, X is a backing child 
of P)

inserting

   - do blockdev-add to add a filter (and specify X as its child)
   - do blockdev-reopen to set P.backing = filter

remvoing

   - do blockdev-reopen to set P.backing = X
   - do blockdev-del to drop a filter


And, probably we'll want transaction support for all these things.


Is it OK? Or do we need some kind of additional blockdev-replace command, that 
can replace one node by another, so in both cases we will do

inserting:
   - blockdev-add filter
   - blockdev-replace (make all parents of X to point to the new filter instead 
(except for the filter itself of course)

removing
   - blockdev-replace (make all parante of filter to be parents of X instead)
   - blockdev-del filter

It's simple to implement, and it seems for me that it is simpler to use. Any 
thoughts?


One reason I remember why we didn't decide to go this way in the many
"dynamic graph reconfiguration" discussions we had, is that it's not
generic enough to cover all cases. But I'm not sure if we ever
considered root nodes as a separate case. I acknowledge that having two
different interfaces is inconvenient, and integrating qom-set in a
transaction is rather unlikely to happen.

The reason why it's not generic is that it restricts you to doing the
same thing for all parents. Imagine this:

 +- virtio-blk
 |
 file <- qcow2 <-+
 |
 +- NBD export

Now you want to throttle the NBD export so that it doesn't interfere
with your VM too much. With your simple blockdev-replace this isn't
possible. You would have to add the filter to both users or to none.

In theory, blockdev-replace could take a list of the edges that should
be changed to the new node. The problem is that edges don't have names,
and even the parents don't necessarily have one (and if they do, they
are in separate namespaces, so a BlockBackend, a job and an export could
all have the same name), so finding a good way to refer to them in QMP
doesn't sound trivial.



Hm. I like the idea. And it seems feasible to me:

Both export and block jobs works through BlockBackend.

So, for block-jobs, we can add optional parameters like source-blk-name, and 
target-blk-name. If parameters specified, blk's will be named, and user will be 
able to do blockdev-replace.

For export it's a bit trickier: it would be strange to add separate argument 
for export blk, as export already has id. So, I'd do the following:

1. make blk named (with same name as the export itself) iff name does not 
conflict with other blks
2. deprecate duplicating existing blk names by export name.


Then, blockdev-replace take a parents list, where parent is either node-name or 
blk name.

--
Best regards,
Vladimir

Re: [libvirt PATCH 1/4] test: move nodedev xml2xml output to a separate dir

[Jonathon Jongsma]

On Wed, 2021-05-19 at 10:46 +0200, Michal Prívozník wrote:
> On 5/14/21 11:28 PM, Jonathon Jongsma wrote:
> > Currently, we're loading and parsing the xml from the input file,
> > and
> > then formatting it and then comparing it directly back to the input
> > file. This works for now, but is severely limiting as it relies on
> > the
> > input file being fully-specified and in the exact order as the
> > output
> > xml format.
> > 
> > If optional elements are ommitted in the input XML, the output xml
> > may include default values for the ommitted elements and thus the
> > output
> > will not match the input.
> > 
> > In order to allow more flexibility in testing, save the expected
> > output
> > to a seprate 'out' directory similar to what most of the other
> > xml2xml
> > tests are already doing.
> > 
> > Signed-off-by: Jonathon Jongsma 
> > ---
> 
> Fair enough - we have plenty of examples around, qemuxml2xmloutdata/
> to
> name the biggest one. However, what we also do (to save disk space)
> is
> to turn those files where input XML is the same as output XML into
> symlinks. I've identified a few which could be just a symlink:
> 
> tests/nodedevxml2xmlout $ for i in *; do if diff $i
> ../nodedevschemadata/$i >/dev/null ; then echo $i; fi; done
> DVD_GCC_4247N.xml
> DVD_with_media.xml
> ap_07_0038.xml
> ap_card07.xml
> ap_matrix.xml
> ap_matrix_mdev_types.xml
> 
> 
> I know you are modifying some of these files (mdev*), that's why I
> ran
> the command only after all your patches. For these patches we could
> make
> everything a symlink in 1/4 and then as we need to make changes (in
> 2/4
> and 3/4) remove those symlinks and make regular copies.
> 
> Do you think it's worth doing? If so, no need to resend, it's
> something
> I can fix before pushing. Plus marking @outfile variable in
> testCompareXMLToXMLHelper() as g_autofree (thanks to Pavel who
> noticed!).

Sure, that's fine with me. The vast majority of these files are staying
the same (which is why the tests currently pass -- the output is
identical to the input). So symlinks should be fine for those. 


Thanks,
Jonathon

Re: RFC: Qemu backup interface plans

[Vladimir Sementsov-Ogievskiy]

19.05.2021 14:20, Kevin Wolf wrote:

Am 19.05.2021 um 08:11 hat Vladimir Sementsov-Ogievskiy geschrieben:

2. Test, that we can start backup job with source = (target of
backup-top filter), so that we have "push backup with fleecing".
Make an option for backup to start without a filter, when we don't
need copy-before-write operations, to not create extra superfluous
filter.


OK, so the backup job is not really a backup job, but just anything
that copies data.


Not quite. For backup without a filter we should protect source from
changing, by unsharing WRITE permission on it.

I'll try to avoid adding an option. The logic should work like in
commit job: if there are current writers on source we create filter.
If there no writers, we just unshare writes and go without a filter.
And for this copy-before-write filter should be able to do
WRITE_UNCHANGED in case of fleecing.


If we ever get to the point where we would make a block-copy job visible
to the user, I wouldn't copy the restrictions from the current jobs, but
keep it really generic to cover all cases.

There is no way for the QMP command starting the job to know what the
user is planning to do with the image in the future. Even if it's
currently read-only, the user may want to add a writer later.

I think this means that we want to always add a filter node, and then
possibly dynamically switch between modes if being read-only provides a
significant advantage for the job.

Kevin



Still, in push-backup-with-fleecing scheme we really don't need the second 
filter, so why to insert extra thing to block graph?

I see your point still, that user may want to add writer later. Still, I'd be 
surprised if such use-cases exist now.

What about the following:

add some source-mode tri-state parameter for backup:

auto: insert filter iff there are existing writers [default]
filtered: insert filter unconditionally
immutable: don't insert filter. will fail if there are existing writers, and 
creating writers during block-job would be impossible

--
Best regards,
Vladimir

Plans for the next release

[Jiri Denemark]

We are getting close to the next release of libvirt. To aim for the
release on Jun 01 I suggest entering the freeze on Tuesday May 25 and
tagging RC2 on Thursday May 27.

I hope this works for everyone.

Jirka

[libvirt PATCH 05/10] virDomainAudioDefParseXML: Don't ignore return value of virDomainAudio*Parse()

[Tim Wiederhake]

Signed-off-by: Tim Wiederhake 
---
 src/conf/domain_conf.c | 50 +-
 1 file changed, 30 insertions(+), 20 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 2142e45fd5..1350c46039 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13155,17 +13155,21 @@ virDomainAudioDefParseXML(virDomainXMLOption *xmlopt 
G_GNUC_UNUSED,
 break;
 
 case VIR_DOMAIN_AUDIO_TYPE_COREAUDIO:
-if (inputNode)
-virDomainAudioCoreAudioParse(>backend.coreaudio.input, 
inputNode);
-if (outputNode)
-virDomainAudioCoreAudioParse(>backend.coreaudio.output, 
outputNode);
+if (inputNode &&
+virDomainAudioCoreAudioParse(>backend.coreaudio.input, 
inputNode) < 0)
+goto error;
+if (outputNode &&
+virDomainAudioCoreAudioParse(>backend.coreaudio.output, 
outputNode) < 0)
+goto error;
 break;
 
 case VIR_DOMAIN_AUDIO_TYPE_JACK:
-if (inputNode)
-virDomainAudioJackParse(>backend.jack.input, inputNode);
-if (outputNode)
-virDomainAudioJackParse(>backend.jack.output, outputNode);
+if (inputNode &&
+virDomainAudioJackParse(>backend.jack.input, inputNode) < 0)
+goto error;
+if (outputNode &&
+virDomainAudioJackParse(>backend.jack.output, outputNode) < 0)
+goto error;
 break;
 
 case VIR_DOMAIN_AUDIO_TYPE_OSS: {
@@ -13193,20 +13197,24 @@ virDomainAudioDefParseXML(virDomainXMLOption *xmlopt 
G_GNUC_UNUSED,
 def->backend.oss.dspPolicySet = true;
 }
 
-if (inputNode)
-virDomainAudioOSSParse(>backend.oss.input, inputNode);
-if (outputNode)
-virDomainAudioOSSParse(>backend.oss.output, outputNode);
+if (inputNode &&
+virDomainAudioOSSParse(>backend.oss.input, inputNode) < 0)
+goto error;
+if (outputNode &&
+virDomainAudioOSSParse(>backend.oss.output, outputNode) < 0)
+goto error;
 break;
 }
 
 case VIR_DOMAIN_AUDIO_TYPE_PULSEAUDIO:
 def->backend.pulseaudio.serverName = virXMLPropString(node, 
"serverName");
 
-if (inputNode)
-virDomainAudioPulseAudioParse(>backend.pulseaudio.input, 
inputNode);
-if (outputNode)
-virDomainAudioPulseAudioParse(>backend.pulseaudio.output, 
outputNode);
+if (inputNode &&
+virDomainAudioPulseAudioParse(>backend.pulseaudio.input, 
inputNode) < 0)
+goto error;
+if (outputNode &&
+virDomainAudioPulseAudioParse(>backend.pulseaudio.output, 
outputNode) < 0)
+goto error;
 break;
 
 case VIR_DOMAIN_AUDIO_TYPE_SDL: {
@@ -13214,10 +13222,12 @@ virDomainAudioDefParseXML(virDomainXMLOption *xmlopt 
G_GNUC_UNUSED,
VIR_XML_PROP_NONZERO, >backend.sdl.driver) < 0)
 goto error;
 
-if (inputNode)
-virDomainAudioSDLParse(>backend.sdl.input, inputNode);
-if (outputNode)
-virDomainAudioSDLParse(>backend.sdl.output, outputNode);
+if (inputNode &&
+virDomainAudioSDLParse(>backend.sdl.input, inputNode) < 0)
+goto error;
+if (outputNode &&
+virDomainAudioSDLParse(>backend.sdl.output, outputNode) < 0)
+goto error;
 break;
 }
 
-- 
2.26.3

[libvirt PATCH 04/10] virDomainAudioDefParseXML: Use virXMLProp*

[Tim Wiederhake]

This strictens the parser to disallow negative values (interpreted as
`UINT_MAX + value + 1`) for attribute `id`. Allowing negative
numbers to be interpreted this way makes no sense for this attribute.

Signed-off-by: Tim Wiederhake 
---
 src/conf/domain_conf.c | 74 --
 1 file changed, 21 insertions(+), 53 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9e6719265f..2142e45fd5 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13122,40 +13122,18 @@ virDomainAudioDefParseXML(virDomainXMLOption *xmlopt 
G_GNUC_UNUSED,
 {
 virDomainAudioDef *def;
 VIR_XPATH_NODE_AUTORESTORE(ctxt)
-g_autofree char *tmp = NULL;
-g_autofree char *typestr = NULL;
-int type;
 xmlNodePtr inputNode, outputNode;
 
 def = g_new0(virDomainAudioDef, 1);
 ctxt->node = node;
 
-typestr = virXMLPropString(node, "type");
-if (!typestr) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
-   _("missing audio 'type' attribute"));
-goto error;
-}
-
-if ((type = virDomainAudioTypeTypeFromString(typestr)) < 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("unknown audio type '%s'"), typestr);
+if (virXMLPropEnum(node, "type", virDomainAudioTypeTypeFromString,
+   VIR_XML_PROP_REQUIRED, >type) < 0)
 goto error;
-}
-def->type = type;
 
-tmp = virXMLPropString(node, "id");
-if (!tmp) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("missing audio 'id' attribute"));
-goto error;
-}
-if (virStrToLong_ui(tmp, NULL, 10, >id) < 0 ||
-def->id == 0) {
-virReportError(VIR_ERR_XML_ERROR,
-   _("invalid audio 'id' value '%s'"), tmp);
+if (virXMLPropUInt(node, "id", 10, VIR_XML_PROP_REQUIRED | 
VIR_XML_PROP_NONZERO,
+   >id) < 0)
 goto error;
-}
 
 inputNode = virXPathNode("./input", ctxt);
 outputNode = virXPathNode("./output", ctxt);
@@ -13191,29 +13169,25 @@ virDomainAudioDefParseXML(virDomainXMLOption *xmlopt 
G_GNUC_UNUSED,
 break;
 
 case VIR_DOMAIN_AUDIO_TYPE_OSS: {
-g_autofree char *tryMMap = virXMLPropString(node, "tryMMap");
-g_autofree char *exclusive = virXMLPropString(node, "exclusive");
-g_autofree char *dspPolicy = virXMLPropString(node, "dspPolicy");
+int dspPolicySet;
 
-if (tryMMap && ((def->backend.oss.tryMMap =
- virTristateBoolTypeFromString(tryMMap)) <= 0)) {
-virReportError(VIR_ERR_XML_ERROR,
-   _("unknown 'tryMMap' value '%s'"), tryMMap);
+if (virXMLPropTristateBool(node, "tryMMap", VIR_XML_PROP_NONE,
+   >backend.oss.tryMMap) < 0)
 goto error;
-}
 
-if (exclusive && ((def->backend.oss.exclusive =
-   virTristateBoolTypeFromString(exclusive)) <= 0)) {
-virReportError(VIR_ERR_XML_ERROR,
-   _("unknown 'exclusive' value '%s'"), exclusive);
+if (virXMLPropTristateBool(node, "exclusive", VIR_XML_PROP_NONE,
+   >backend.oss.exclusive) < 0)
+goto error;
+
+if ((dspPolicySet = virXMLPropInt(node, "dspPolicy", 10, 
VIR_XML_PROP_NONE,
+ >backend.oss.dspPolicy, 0)) < 0)
 goto error;
-}
 
-if (dspPolicy) {
-if (virStrToLong_i(dspPolicy, NULL, 10,
-   >backend.oss.dspPolicy) < 0) {
+if (dspPolicySet != 0) {
+if (def->backend.oss.dspPolicy < 0) {
 virReportError(VIR_ERR_XML_ERROR,
-   _("cannot parse 'dspPolicy' value '%s'"), 
dspPolicy);
+   _("cannot parse 'dspPolicy' value '%i'"),
+   def->backend.oss.dspPolicy);
 goto error;
 }
 def->backend.oss.dspPolicySet = true;
@@ -13236,16 +13210,10 @@ virDomainAudioDefParseXML(virDomainXMLOption *xmlopt 
G_GNUC_UNUSED,
 break;
 
 case VIR_DOMAIN_AUDIO_TYPE_SDL: {
-g_autofree char *driverstr = virXMLPropString(node, "driver");
-int driver;
-if (driverstr) {
-if ((driver = virDomainAudioSDLDriverTypeFromString(driverstr)) <= 
0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("unknown SDL driver '%s'"), driverstr);
-goto error;
-}
-def->backend.sdl.driver = driver;
-}
+if (virXMLPropEnum(node, "driver", 
virDomainAudioSDLDriverTypeFromString,
+   VIR_XML_PROP_NONZERO, >backend.sdl.driver) < 0)
+goto error;
+
 if (inputNode)
 virDomainAudioSDLParse(>backend.sdl.input, inputNode);

[libvirt PATCH 03/10] virDomainAudioDef: Change type of "sdl.driver" to virDomainAudioSDLDriver

[Tim Wiederhake]

Signed-off-by: Tim Wiederhake 
---
 src/conf/domain_conf.c | 17 +
 src/conf/domain_conf.h |  2 +-
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 758f699c2c..9e6719265f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13236,15 +13236,16 @@ virDomainAudioDefParseXML(virDomainXMLOption *xmlopt 
G_GNUC_UNUSED,
 break;
 
 case VIR_DOMAIN_AUDIO_TYPE_SDL: {
-g_autofree char *driver = virXMLPropString(node, "driver");
-if (driver &&
-(def->backend.sdl.driver =
- virDomainAudioSDLDriverTypeFromString(driver)) <= 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("unknown SDL driver '%s'"), driver);
-goto error;
+g_autofree char *driverstr = virXMLPropString(node, "driver");
+int driver;
+if (driverstr) {
+if ((driver = virDomainAudioSDLDriverTypeFromString(driverstr)) <= 
0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("unknown SDL driver '%s'"), driverstr);
+goto error;
+}
+def->backend.sdl.driver = driver;
 }
-
 if (inputNode)
 virDomainAudioSDLParse(>backend.sdl.input, inputNode);
 if (outputNode)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 462c61a807..fab856a5c7 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1578,7 +1578,7 @@ struct _virDomainAudioDef {
 struct {
 virDomainAudioIOSDL input;
 virDomainAudioIOSDL output;
-int driver; /* virDomainAudioSDLDriver */
+virDomainAudioSDLDriver driver;
 } sdl;
 struct {
 char *path;
-- 
2.26.3

[libvirt PATCH 00/10] Refactor more XML parsing boilerplate code, part XII

[Tim Wiederhake]

For background, see
https://listman.redhat.com/archives/libvir-list/2021-April/msg00668.html

Tim Wiederhake (10):
  virDomainAudioPulseAudioParse: Use virXMLProp*
  virDomainAudioDef: Change type of "type" to virDomainAudioType
  virDomainAudioDef: Change type of "sdl.driver" to
virDomainAudioSDLDriver
  virDomainAudioDefParseXML: Use virXMLProp*
  virDomainAudioDefParseXML: Don't ignore return value of
virDomainAudio*Parse()
  virDomainIOMMUDefParseXML: Use virXMLProp*
  virStorageAdapterParseXML: Use g_autofree
  virStorageAdapterFCHost: Change type of "type" to
virStorageAdapterType
  virStorageAdapterParseXML: Use virXMLProp*
  virDomainDeviceSpaprVioAddressParseXML: Use virXMLProp*

 src/bhyve/bhyve_command.c   |   2 +-
 src/conf/device_conf.c  |  14 +--
 src/conf/domain_conf.c  | 202 +++-
 src/conf/domain_conf.h  |   4 +-
 src/conf/storage_adapter_conf.c |  38 ++
 src/conf/storage_adapter_conf.h |   2 +-
 src/qemu/qemu_command.c |   4 +-
 src/qemu/qemu_validate.c|   2 +-
 8 files changed, 97 insertions(+), 171 deletions(-)

-- 
2.26.3

[libvirt PATCH 01/10] virDomainAudioPulseAudioParse: Use virXMLProp*

[Tim Wiederhake]

This strictens the parser to disallow negative values (interpreted as
`UINT_MAX + value + 1`) for attribute `latency`. Allowing negative
numbers to be interpreted this way makes no sense for this attribute.

Signed-off-by: Tim Wiederhake 
---
 src/conf/domain_conf.c | 10 ++
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b3ed3a9c5a..942d6f269a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13092,18 +13092,12 @@ static int
 virDomainAudioPulseAudioParse(virDomainAudioIOPulseAudio *def,
   xmlNodePtr node)
 {
-g_autofree char *latency = virXMLPropString(node, "latency");
-
 def->name = virXMLPropString(node, "name");
 def->streamName = virXMLPropString(node, "streamName");
 
-if (latency &&
-virStrToLong_ui(latency, NULL, 10,
->latency) < 0) {
-virReportError(VIR_ERR_XML_ERROR,
-   _("cannot parse 'latency' value '%s'"), latency);
+if (virXMLPropUInt(node, "latency", 10, VIR_XML_PROP_NONE,
+   >latency) < 0)
 return -1;
-}
 
 return 0;
 }
-- 
2.26.3

Re: [PATCH v2 2/3] conf: Parse/format XML input type 'evdev'

[Michal Prívozník]

On 5/11/21 12:16 PM, Kristina Hanicova wrote:
> Signed-off-by: Kristina Hanicova 
> ---
>  docs/formatdomain.rst| 37 -
>  docs/schemas/domaincommon.rng| 20 +++
>  src/conf/domain_audit.c  |  1 +
>  src/conf/domain_conf.c   | 66 
>  src/conf/domain_conf.h   | 12 +
>  src/conf/domain_validate.c   |  8 +++
>  src/libvirt_private.syms |  2 +
>  src/qemu/qemu_cgroup.c   |  2 +
>  src/qemu/qemu_command.c  |  1 +
>  src/qemu/qemu_domain_address.c   |  1 +
>  src/qemu/qemu_hotplug.c  |  1 +
>  src/qemu/qemu_validate.c |  6 +++
>  src/security/security_apparmor.c |  1 +
>  src/security/security_dac.c  |  2 +
>  src/security/security_selinux.c  |  2 +
>  src/security/virt-aa-helper.c|  3 +-
>  tests/qemuxml2argvdata/input-linux.xml   | 24 +
>  tests/qemuxml2xmloutdata/input-linux.xml |  1 +
>  18 files changed, 167 insertions(+), 23 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/input-linux.xml
>  create mode 12 tests/qemuxml2xmloutdata/input-linux.xml
> 

> index e8632e4d73..299b600e24 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c

> @@ -12021,12 +12032,36 @@ virDomainInputDefParseXML(virDomainXMLOption 
> *xmlopt,
>  goto error;
>  }
>  
> -if ((evdev = virXPathString("string(./source/@evdev)", ctxt)))
> -def->source.evdev = virFileSanitizePath(evdev);
> -if (def->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH && 
> !def->source.evdev) {
> -virReportError(VIR_ERR_XML_ERROR, "%s",
> -   _("Missing evdev path for input device passthrough"));
> -goto error;
> +if ((source = virXPathNode("./source", ctxt))) {
> +g_autofree char *evdev = NULL;
> +
> +if (def->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH)
> +evdev = virXMLPropString(source, "evdev");
> +else if (def->type == VIR_DOMAIN_INPUT_TYPE_EVDEV)
> +evdev = virXMLPropString(source, "dev");

So here you parse 

> +
> +if (evdev)
> +def->source.evdev = virFileSanitizePath(evdev);
> +
> +if (def->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH ||
> +def->type == VIR_DOMAIN_INPUT_TYPE_EVDEV) {
> +if (!def->source.evdev) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> +   _("Missing evdev path for input device"));
> +goto error;
> +}
> +}
> +
> +if (def->type == VIR_DOMAIN_INPUT_TYPE_EVDEV) {
> +if (virXMLPropEnum(source, "grab",
> +   virDomainInputSourceGrabTypeFromString,
> +   VIR_XML_PROP_NONZERO, >source.grab) < 0)
> +goto error;
> +
> +if (virXMLPropTristateSwitch(source, "repeat",
> + VIR_XML_PROP_NONE, 
> >source.repeat) < 0)
> +goto error;
> +}
>  }
>  
>  if (virDomainVirtioOptionsParseXML(virXPathNode("./driver", ctxt),
> @@ -26076,9 +26111,12 @@ virDomainInputDefFormat(virBuffer *buf,
>  {
>  const char *type = virDomainInputTypeToString(def->type);
>  const char *bus = virDomainInputBusTypeToString(def->bus);
> +const char *grab = 
> virDomainInputSourceGrabTypeToString(def->source.grab);
> +const char *repeat = virTristateSwitchTypeToString(def->source.repeat);
>  g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
>  g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
>  g_auto(virBuffer) driverAttrBuf = VIR_BUFFER_INITIALIZER;
> +g_auto(virBuffer) sourceAttrBuf = VIR_BUFFER_INITIALIZER;
>  
>  /* don't format keyboard into migratable XML for backward compatibility 
> */
>  if (flags & VIR_DOMAIN_DEF_FORMAT_MIGRATABLE &&
> @@ -26098,7 +26136,9 @@ virDomainInputDefFormat(virBuffer *buf,
>  return -1;
>  }
>  
> -virBufferAsprintf(, " type='%s' bus='%s'", type, bus);
> +virBufferAsprintf(, " type='%s'", type);
> +if (def->bus != VIR_DOMAIN_INPUT_BUS_NONE)
> +virBufferAsprintf(, " bus='%s'", bus);
>  
>  if (def->model) {
>  const char *model = virDomainInputModelTypeToString(def->model);
> @@ -26116,7 +26156,15 @@ virDomainInputDefFormat(virBuffer *buf,
>  
>  virXMLFormatElement(, "driver", , NULL);
>  
> -virBufferEscapeString(, "\n", 
> def->source.evdev);
> +virBufferEscapeString(, " evdev='%s'", def->source.evdev);

But here you format . What you can do is to format
dev=... here if def->type == VIR_DOMAIN_INPUT_TYPE_EVDEV, and format
evdev=... otherwise. Alternatively, you can stick with 'evdev' name even
for _TYPE_EVDEV (will require slightly more changes - to docs, RNG, XML,
...). Your call.

> +
> +if (def->source.grab)
> +

[libvirt PATCH 06/10] virDomainIOMMUDefParseXML: Use virXMLProp*

[Tim Wiederhake]

This strictens the parser to disallow negative values (interpreted as
`UINT_MAX + value + 1`) for attribute `aw_bits`. Allowing negative
numbers to be interpreted this way makes no sense for this attribute.

Signed-off-by: Tim Wiederhake 
---
 src/conf/domain_conf.c | 69 +++---
 1 file changed, 17 insertions(+), 52 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1350c46039..e35c38caa3 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -14805,71 +14805,36 @@ virDomainIOMMUDefParseXML(xmlNodePtr node,
 {
 VIR_XPATH_NODE_AUTORESTORE(ctxt)
 xmlNodePtr driver;
-int val;
-g_autofree char *tmp = NULL;
 g_autofree virDomainIOMMUDef *iommu = NULL;
 
 ctxt->node = node;
 
 iommu = g_new0(virDomainIOMMUDef, 1);
 
-if (!(tmp = virXMLPropString(node, "model"))) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
-   _("missing model for IOMMU device"));
+if (virXMLPropEnum(node, "model", virDomainIOMMUModelTypeFromString,
+   VIR_XML_PROP_REQUIRED, >model) < 0)
 return NULL;
-}
-
-if ((val = virDomainIOMMUModelTypeFromString(tmp)) < 0) {
-virReportError(VIR_ERR_XML_ERROR, _("unknown IOMMU model: %s"), tmp);
-return NULL;
-}
-
-iommu->model = val;
 
 if ((driver = virXPathNode("./driver", ctxt))) {
-VIR_FREE(tmp);
-if ((tmp = virXMLPropString(driver, "intremap"))) {
-if ((val = virTristateSwitchTypeFromString(tmp)) < 0) {
-virReportError(VIR_ERR_XML_ERROR, _("unknown intremap value: 
%s"), tmp);
-return NULL;
-}
-iommu->intremap = val;
-}
+if (virXMLPropTristateSwitch(driver, "intremap", VIR_XML_PROP_NONE,
+ >intremap) < 0)
+return NULL;
 
-VIR_FREE(tmp);
-if ((tmp = virXMLPropString(driver, "caching_mode"))) {
-if ((val = virTristateSwitchTypeFromString(tmp)) < 0) {
-virReportError(VIR_ERR_XML_ERROR, _("unknown caching_mode 
value: %s"), tmp);
-return NULL;
-}
-iommu->caching_mode = val;
-}
-VIR_FREE(tmp);
-if ((tmp = virXMLPropString(driver, "iotlb"))) {
-if ((val = virTristateSwitchTypeFromString(tmp)) < 0) {
-virReportError(VIR_ERR_XML_ERROR, _("unknown iotlb value: 
%s"), tmp);
-return NULL;
-}
-iommu->iotlb = val;
-}
+if (virXMLPropTristateSwitch(driver, "caching_mode", VIR_XML_PROP_NONE,
+ >caching_mode) < 0)
+return NULL;
 
-VIR_FREE(tmp);
-if ((tmp = virXMLPropString(driver, "eim"))) {
-if ((val = virTristateSwitchTypeFromString(tmp)) < 0) {
-virReportError(VIR_ERR_XML_ERROR, _("unknown eim value: %s"), 
tmp);
-return NULL;
-}
-iommu->eim = val;
-}
+if (virXMLPropTristateSwitch(driver, "iotlb", VIR_XML_PROP_NONE,
+ >iotlb) < 0)
+return NULL;
 
-VIR_FREE(tmp);
-if ((tmp = virXMLPropString(driver, "aw_bits"))) {
-if (virStrToLong_ui(tmp, NULL, 10, >aw_bits) < 0) {
-virReportError(VIR_ERR_XML_ERROR, _("unknown aw_bits value: 
%s"), tmp);
-return NULL;
-}
-}
+if (virXMLPropTristateSwitch(driver, "eim", VIR_XML_PROP_NONE,
+ >eim) < 0)
+return NULL;
 
+if (virXMLPropUInt(driver, "aw_bits", 10, VIR_XML_PROP_NONE,
+   >aw_bits) < 0)
+return NULL;
 }
 
 return g_steal_pointer();
-- 
2.26.3

[libvirt PATCH 02/10] virDomainAudioDef: Change type of "type" to virDomainAudioType

[Tim Wiederhake]

Signed-off-by: Tim Wiederhake 
---
 src/bhyve/bhyve_command.c |  2 +-
 src/conf/domain_conf.c| 18 ++
 src/conf/domain_conf.h|  2 +-
 src/qemu/qemu_command.c   |  4 ++--
 src/qemu/qemu_validate.c  |  2 +-
 5 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index f8e0ce5123..ab9d3026cc 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -524,7 +524,7 @@ bhyveBuildSoundArgStr(const virDomainDef *def G_GNUC_UNUSED,
 virCommandAddArg(cmd, "-s");
 
 if (audio) {
-switch ((virDomainAudioType) audio->type) {
+switch (audio->type) {
 case  VIR_DOMAIN_AUDIO_TYPE_OSS:
 if (virDomainAudioIOCommonIsSet(>input) ||
 virDomainAudioIOCommonIsSet(>output)) {
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 942d6f269a..758f699c2c 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2922,7 +2922,7 @@ virDomainAudioDefFree(virDomainAudioDef *def)
 if (!def)
 return;
 
-switch ((virDomainAudioType) def->type) {
+switch (def->type) {
 case VIR_DOMAIN_AUDIO_TYPE_NONE:
 break;
 
@@ -13123,24 +13123,26 @@ virDomainAudioDefParseXML(virDomainXMLOption *xmlopt 
G_GNUC_UNUSED,
 virDomainAudioDef *def;
 VIR_XPATH_NODE_AUTORESTORE(ctxt)
 g_autofree char *tmp = NULL;
-g_autofree char *type = NULL;
+g_autofree char *typestr = NULL;
+int type;
 xmlNodePtr inputNode, outputNode;
 
 def = g_new0(virDomainAudioDef, 1);
 ctxt->node = node;
 
-type = virXMLPropString(node, "type");
-if (!type) {
+typestr = virXMLPropString(node, "type");
+if (!typestr) {
 virReportError(VIR_ERR_XML_ERROR, "%s",
_("missing audio 'type' attribute"));
 goto error;
 }
 
-if ((def->type = virDomainAudioTypeTypeFromString(type)) < 0) {
+if ((type = virDomainAudioTypeTypeFromString(typestr)) < 0) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("unknown audio type '%s'"), type);
+   _("unknown audio type '%s'"), typestr);
 goto error;
 }
+def->type = type;
 
 tmp = virXMLPropString(node, "id");
 if (!tmp) {
@@ -13163,7 +13165,7 @@ virDomainAudioDefParseXML(virDomainXMLOption *xmlopt 
G_GNUC_UNUSED,
 if (outputNode && virDomainAudioCommonParse(>output, outputNode, 
ctxt) < 0)
 goto error;
 
-switch ((virDomainAudioType) def->type) {
+switch (def->type) {
 case VIR_DOMAIN_AUDIO_TYPE_NONE:
 break;
 
@@ -25465,7 +25467,7 @@ virDomainAudioDefFormat(virBuffer *buf,
 
 virBufferAsprintf(buf, "id, type);
 
-switch ((virDomainAudioType)def->type) {
+switch (def->type) {
 case VIR_DOMAIN_AUDIO_TYPE_NONE:
 break;
 
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index cf8481f1f6..462c61a807 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1543,7 +1543,7 @@ struct _virDomainAudioIOSDL {
 };
 
 struct _virDomainAudioDef {
-int type;
+virDomainAudioType type;
 
 unsigned int id;
 
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index d6c5308ef0..dcc060bde9 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7670,7 +7670,7 @@ qemuBuildAudioCommandLineArg(virCommand *cmd,
 qemuBuildAudioCommonArg(, "in", >input);
 qemuBuildAudioCommonArg(, "out", >output);
 
-switch ((virDomainAudioType)def->type) {
+switch (def->type) {
 case VIR_DOMAIN_AUDIO_TYPE_NONE:
 break;
 
@@ -7859,7 +7859,7 @@ qemuBuildAudioCommandLineEnv(virCommand *cmd,
 qemuBuildAudioCommonEnv(cmd, "QEMU_AUDIO_ADC_", >input);
 qemuBuildAudioCommonEnv(cmd, "QEMU_AUDIO_DAC_", >output);
 
-switch ((virDomainAudioType)audio->type) {
+switch (audio->type) {
 case VIR_DOMAIN_AUDIO_TYPE_NONE:
 break;
 
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 141203f979..e6ddb43113 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -4223,7 +4223,7 @@ qemuValidateDomainDeviceDefAudio(virDomainAudioDef *audio,
 }
 }
 
-switch ((virDomainAudioType)audio->type) {
+switch (audio->type) {
 case VIR_DOMAIN_AUDIO_TYPE_NONE:
 break;
 
-- 
2.26.3

[libvirt PATCH 09/10] virStorageAdapterParseXML: Use virXMLProp*

[Tim Wiederhake]

Signed-off-by: Tim Wiederhake 
---
 src/conf/storage_adapter_conf.c | 30 +++---
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/src/conf/storage_adapter_conf.c b/src/conf/storage_adapter_conf.c
index a834eee81f..e93b26f06f 100644
--- a/src/conf/storage_adapter_conf.c
+++ b/src/conf/storage_adapter_conf.c
@@ -170,31 +170,23 @@ virStorageAdapterParseXML(virStorageAdapter *adapter,
 {
 int type;
 VIR_XPATH_NODE_AUTORESTORE(ctxt)
-g_autofree char *adapter_type = NULL;
 
 ctxt->node = node;
 
-if ((adapter_type = virXMLPropString(node, "type"))) {
-if ((type = virStorageAdapterTypeFromString(adapter_type)) <= 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("Unknown pool adapter type '%s'"),
-   adapter_type);
+if ((type = virXMLPropEnum(node, "type", virStorageAdapterTypeFromString, 
VIR_XML_PROP_NONZERO, >type)) < 0)
 return -1;
-}
-adapter->type = type;
 
-if ((adapter->type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST) &&
-(virStorageAdapterParseXMLFCHost(node, >data.fchost)) < 0)
-return -1;
+if (type == 0)
+return virStorageAdapterParseXMLLegacy(node, ctxt, adapter);
 
-if ((adapter->type == VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST) &&
-(virStorageAdapterParseXMLSCSIHost(node, ctxt,
-   >data.scsi_host)) < 0)
-return -1;
-} else {
-if (virStorageAdapterParseXMLLegacy(node, ctxt, adapter) < 0)
-return -1;
-}
+if ((adapter->type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST) &&
+(virStorageAdapterParseXMLFCHost(node, >data.fchost)) < 0)
+return -1;
+
+if ((adapter->type == VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST) &&
+(virStorageAdapterParseXMLSCSIHost(node, ctxt,
+   >data.scsi_host)) < 0)
+return -1;
 
 return 0;
 }
-- 
2.26.3

[libvirt PATCH 08/10] virStorageAdapterFCHost: Change type of "type" to virStorageAdapterType

[Tim Wiederhake]

Signed-off-by: Tim Wiederhake 
---
 src/conf/storage_adapter_conf.c | 5 +++--
 src/conf/storage_adapter_conf.h | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/conf/storage_adapter_conf.c b/src/conf/storage_adapter_conf.c
index 717d00dc4a..a834eee81f 100644
--- a/src/conf/storage_adapter_conf.c
+++ b/src/conf/storage_adapter_conf.c
@@ -168,19 +168,20 @@ virStorageAdapterParseXML(virStorageAdapter *adapter,
   xmlNodePtr node,
   xmlXPathContextPtr ctxt)
 {
+int type;
 VIR_XPATH_NODE_AUTORESTORE(ctxt)
 g_autofree char *adapter_type = NULL;
 
 ctxt->node = node;
 
 if ((adapter_type = virXMLPropString(node, "type"))) {
-if ((adapter->type =
- virStorageAdapterTypeFromString(adapter_type)) <= 0) {
+if ((type = virStorageAdapterTypeFromString(adapter_type)) <= 0) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unknown pool adapter type '%s'"),
adapter_type);
 return -1;
 }
+adapter->type = type;
 
 if ((adapter->type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST) &&
 (virStorageAdapterParseXMLFCHost(node, >data.fchost)) < 0)
diff --git a/src/conf/storage_adapter_conf.h b/src/conf/storage_adapter_conf.h
index e6d9c864cd..1f0e74631e 100644
--- a/src/conf/storage_adapter_conf.h
+++ b/src/conf/storage_adapter_conf.h
@@ -54,7 +54,7 @@ struct _virStorageAdapterFCHost {
 
 typedef struct _virStorageAdapter virStorageAdapter;
 struct _virStorageAdapter {
-int type; /* virStorageAdapterType */
+virStorageAdapterType type;
 
 union {
 virStorageAdapterSCSIHost scsi_host;
-- 
2.26.3

[libvirt PATCH 07/10] virStorageAdapterParseXML: Use g_autofree

[Tim Wiederhake]

Signed-off-by: Tim Wiederhake 
---
 src/conf/storage_adapter_conf.c | 17 ++---
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/src/conf/storage_adapter_conf.c b/src/conf/storage_adapter_conf.c
index 6b5a58e1e7..717d00dc4a 100644
--- a/src/conf/storage_adapter_conf.c
+++ b/src/conf/storage_adapter_conf.c
@@ -168,9 +168,8 @@ virStorageAdapterParseXML(virStorageAdapter *adapter,
   xmlNodePtr node,
   xmlXPathContextPtr ctxt)
 {
-int ret = -1;
 VIR_XPATH_NODE_AUTORESTORE(ctxt)
-char *adapter_type = NULL;
+g_autofree char *adapter_type = NULL;
 
 ctxt->node = node;
 
@@ -180,27 +179,23 @@ virStorageAdapterParseXML(virStorageAdapter *adapter,
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unknown pool adapter type '%s'"),
adapter_type);
-goto cleanup;
+return -1;
 }
 
 if ((adapter->type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST) &&
 (virStorageAdapterParseXMLFCHost(node, >data.fchost)) < 0)
-goto cleanup;
+return -1;
 
 if ((adapter->type == VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST) &&
 (virStorageAdapterParseXMLSCSIHost(node, ctxt,
>data.scsi_host)) < 0)
-goto cleanup;
+return -1;
 } else {
 if (virStorageAdapterParseXMLLegacy(node, ctxt, adapter) < 0)
-goto cleanup;
+return -1;
 }
 
-ret = 0;
-
- cleanup:
-VIR_FREE(adapter_type);
-return ret;
+return 0;
 }
 
 
-- 
2.26.3

[libvirt PATCH 10/10] virDomainDeviceSpaprVioAddressParseXML: Use virXMLProp*

[Tim Wiederhake]

This strictens the parser to disallow negative values (interpreted as
`ULLONG_MAX + value + 1`) for attribute `reg`. Allowing negative
numbers to be interpreted this way makes no sense for this attribute, as it
refers to a 32 bit address space.

Signed-off-by: Tim Wiederhake 
---
 src/conf/device_conf.c | 14 ++
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
index 034f072df4..e587d90c59 100644
--- a/src/conf/device_conf.c
+++ b/src/conf/device_conf.c
@@ -417,19 +417,17 @@ int
 virDomainDeviceSpaprVioAddressParseXML(xmlNodePtr node,
   virDomainDeviceSpaprVioAddress *addr)
 {
-g_autofree char *reg = virXMLPropString(node, "reg");
+int reg;
 
 memset(addr, 0, sizeof(*addr));
 
-if (reg) {
-if (virStrToLong_ull(reg, NULL, 16, >reg) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Cannot parse  'reg' attribute"));
-return -1;
-}
+if ((reg = virXMLPropULongLong(node, "reg", 16, VIR_XML_PROP_NONE,
+   >reg)) < 0)
+return -1;
 
+if (reg != 0)
 addr->has_reg = true;
-}
+
 return 0;
 }
 
-- 
2.26.3

Re: [libvirt PATCH 2/2] storage_file: add support to probe cluster_size from QCOW2 images

[Peter Krempa]

On Thu, May 13, 2021 at 13:23:05 +0200, Pavel Hrdina wrote:
> >From QEMU docs/interop/qcow2.txt :
> 
>Byte  20 - 23:   cluster_bits
> Number of bits that are used for addressing an offset
> within a cluster (1 << cluster_bits is the cluster size).
> 
> With this patch libvirt will be able to report the current cluster_size
> for all existing storage volumes managed by storage driver.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/storage/storage_util.c|  3 ++
>  src/storage_file/storage_file_probe.c | 55 +++
>  2 files changed, 42 insertions(+), 16 deletions(-)

[...]

> diff --git a/src/storage_file/storage_file_probe.c 
> b/src/storage_file/storage_file_probe.c
> index afe64da02e..423597049f 100644
> --- a/src/storage_file/storage_file_probe.c
> +++ b/src/storage_file/storage_file_probe.c
> @@ -94,6 +94,9 @@ struct FileTypeInfo {
>/* Store a COW base image path (possibly relative),
> * or NULL if there is no COW base image, to RES;
> * return BACKING_STORE_* */
> +int clusterBitsOffset; /* Byte offset from start of file where we find
> +* number of cluster bits, -1 to skip. */
> +int clusterBitsSize;   /* Number of bytes for cluster bits. */

This isn't actually used [1].

>  const struct FileEncryptionInfo *cryptInfo; /* Encryption info */
>  int (*getBackingStore)(char **res, int *format,
> const char *buf, size_t buf_size);

Given how the value is stored in the header it seems that it won't
really be usable for any other format. I think we should parse it via a
callback rather than the generic offset/size parser.

Specifically since the cluster size is stored as number of bits.

> @@ -116,7 +119,8 @@ qedGetBackingStore(char **, int *, const char *, size_t);
>  #define QCOWX_HDR_VERSION (4)
>  #define QCOWX_HDR_BACKING_FILE_OFFSET (QCOWX_HDR_VERSION+4)
>  #define QCOWX_HDR_BACKING_FILE_SIZE (QCOWX_HDR_BACKING_FILE_OFFSET+8)
> -#define QCOWX_HDR_IMAGE_SIZE (QCOWX_HDR_BACKING_FILE_SIZE+4+4)
> +#define QCOWX_HDR_CLUSTER_BITS_OFFSET (QCOWX_HDR_BACKING_FILE_SIZE+4)
> +#define QCOWX_HDR_IMAGE_SIZE (QCOWX_HDR_CLUSTER_BITS_OFFSET+4)
>  
>  #define QCOW1_HDR_CRYPT (QCOWX_HDR_IMAGE_SIZE+8+1+1+2)
>  #define QCOW2_HDR_CRYPT (QCOWX_HDR_IMAGE_SIZE+8)



>  G_STATIC_ASSERT(G_N_ELEMENTS(fileTypeInfo) == VIR_STORAGE_FILE_LAST);
> @@ -890,6 +896,23 @@ virStorageFileProbeGetMetadata(virStorageSource *meta,
>  meta->capacity *= fileTypeInfo[meta->format].sizeMultiplier;
>  }
>  
> +if (fileTypeInfo[meta->format].clusterBitsOffset != -1) {
> +int clusterBits = 0;
> +
> +if ((fileTypeInfo[meta->format].clusterBitsOffset + 4) > len)

[1] ... you hardcode the value.

> +return 0;
> +
> +if (fileTypeInfo[meta->format].endian == LV_LITTLE_ENDIAN)
> +clusterBits = virReadBufInt32LE(buf +
> +
> fileTypeInfo[meta->format].clusterBitsOffset);
> +else
> +clusterBits = virReadBufInt32BE(buf +
> +
> fileTypeInfo[meta->format].clusterBitsOffset);
> +
> +if (clusterBits > 0)
> +meta->clusterSize = 1 << clusterBits;
> +}
> +
>  VIR_FREE(meta->backingStoreRaw);
>  if (fileTypeInfo[meta->format].getBackingStore != NULL) {
>  int store = 
> fileTypeInfo[meta->format].getBackingStore(>backingStoreRaw,
> -- 
> 2.31.1
> 

Re: [PATCH v2 3/3] virsh-domain: Drop support for old APIs in cmdSetmem and cmdSetmaxmem

[Peter Krempa]

On Wed, May 19, 2021 at 12:16:57 +0200, Michal Privoznik wrote:
> Some of our really old APIs are missing @flags argument. We
> introduced their variants with "Flags" suffix and wired some
> logic into virsh to call the new variant only if necessary. This
> enables virsh to talk to older daemon which may be lacking new
> APIs.
> 
> However, in case of cmdSetmem() we are talking about v0.1.1
> (virDomainSetMemory()) vs. v0.9.0 (virDomainSetMemoryFlags()) and
> in case of cmdSetmaxmem() we are talking about v0.0.3
> (virDomainSetMaxMemory()) vs v0.9.0 (virDomainSetMemoryFlags()).
> 
> Libvirt v0.9.0 was released more than 10 years ago and recently
> we dropped support for RHEL-7 which has v4.5.0 (released ~3 years
> ago). Thus it is not really necessary to have support in virsh
> for such old daemons.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  tools/virsh-domain.c | 28 
>  1 file changed, 8 insertions(+), 20 deletions(-)

Reviewed-by: Peter Krempa 

While I agree with the premise, please hold off pushing this patch to
let others chime in.

Re: Qemu block filter insertion/removal API

[Kevin Wolf]

Am 19.05.2021 um 14:19 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 19.05.2021 14:44, Kevin Wolf wrote:
> > Am 17.05.2021 um 14:44 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > Hi all!
> > > 
> > > I'd like to be sure that we know where we are going to.
> > > 
> > > In blockdev-era where qemu user is aware about block nodes, all nodes 
> > > have good names and controlled by user we can efficiently use block 
> > > filters.
> > > 
> > > We already have some useful filters: copy-on-read, throttling, compress. 
> > > In my parallel series I make backup-top filter public and useful without 
> > > backup block jobs. But now filters could be inserted only together with 
> > > opening their child. We can specify filters in qemu cmdline, or filter 
> > > can take place in the block node chain created by blockdev-add.
> > > 
> > > Still, it would be good to insert/remove filters on demand.
> > > 
> > > Currently we are going to use x-blockdev-reopen for this. Still it can't 
> > > be used to insert a filter above root node (as x-blockdev-reopen can 
> > > change only block node options and their children). In my series "[PATCH 
> > > 00/21] block: publish backup-top filter" I propose (as Kevin suggested) 
> > > to modify qom-set, so that it can set drive option of running device. 
> > > That's not difficult, but it means that we have different scenario of 
> > > inserting/removing filters:
> > > 
> > > 1. filter above root node X:
> > > 
> > > inserting:
> > > 
> > >- do blockdev-add to add a filter (and specify X as its child)
> > >- do qom-set to set new filter as a rood node instead of X
> > > 
> > > removing
> > > 
> > >- do qom-set to make X a root node again
> > >- do blockdev-del to drop a filter
> > > 
> > > 2. filter between two block nodes P and X. (For example, X is a backing 
> > > child of P)
> > > 
> > > inserting
> > > 
> > >- do blockdev-add to add a filter (and specify X as its child)
> > >- do blockdev-reopen to set P.backing = filter
> > > 
> > > remvoing
> > > 
> > >- do blockdev-reopen to set P.backing = X
> > >- do blockdev-del to drop a filter
> > > 
> > > 
> > > And, probably we'll want transaction support for all these things.
> > > 
> > > 
> > > Is it OK? Or do we need some kind of additional blockdev-replace command, 
> > > that can replace one node by another, so in both cases we will do
> > > 
> > > inserting:
> > >- blockdev-add filter
> > >- blockdev-replace (make all parents of X to point to the new filter 
> > > instead (except for the filter itself of course)
> > > 
> > > removing
> > >- blockdev-replace (make all parante of filter to be parents of X 
> > > instead)
> > >- blockdev-del filter
> > > 
> > > It's simple to implement, and it seems for me that it is simpler to use. 
> > > Any thoughts?
> > 
> > One reason I remember why we didn't decide to go this way in the many
> > "dynamic graph reconfiguration" discussions we had, is that it's not
> > generic enough to cover all cases. But I'm not sure if we ever
> > considered root nodes as a separate case. I acknowledge that having two
> > different interfaces is inconvenient, and integrating qom-set in a
> > transaction is rather unlikely to happen.
> > 
> > The reason why it's not generic is that it restricts you to doing the
> > same thing for all parents. Imagine this:
> > 
> >  +- virtio-blk
> >  |
> >  file <- qcow2 <-+
> >  |
> >  +- NBD export
> > 
> > Now you want to throttle the NBD export so that it doesn't interfere
> > with your VM too much. With your simple blockdev-replace this isn't
> > possible. You would have to add the filter to both users or to none.
> > 
> > In theory, blockdev-replace could take a list of the edges that should
> > be changed to the new node. The problem is that edges don't have names,
> > and even the parents don't necessarily have one (and if they do, they
> > are in separate namespaces, so a BlockBackend, a job and an export could
> > all have the same name), so finding a good way to refer to them in QMP
> > doesn't sound trivial.
> > 
> 
> Hm. I like the idea. And it seems feasible to me:
> 
> Both export and block jobs works through BlockBackend.
> 
> So, for block-jobs, we can add optional parameters like
> source-blk-name, and target-blk-name. If parameters specified, blk's
> will be named, and user will be able to do blockdev-replace.

I'm not sure if giving them a name is a good idea. Wouldn't it make the
BlockBackend accessible for the user who could then make a device use
it?

> For export it's a bit trickier: it would be strange to add separate
> argument for export blk, as export already has id. So, I'd do the
> following:
> 
> 1. make blk named (with same name as the export itself) iff name does
>not conflict with other blks
> 2. deprecate duplicating existing blk names by export name.

Yes, if we decide that giving them a name is a 

Re: [PATCH v2 2/3] virsh-domain: Fix @ret handling in cmdSetmem and cmdSetmaxmem

[Peter Krempa]

On Wed, May 19, 2021 at 12:16:56 +0200, Michal Privoznik wrote:
> These functions initialize @ret to true and only after something
> fails either they call cleanup code (which consists only from
> virshDomainFree()) and return false, or they set ret = false and
> carry on (when the failure occurred close to cleanup code).
> 
> Switch them to the usual pattern in which ret is initialized to
> failure, goto cleanup is used and ret is set to true only after
> everything succeeded.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  tools/virsh-domain.c | 28 ++--
>  1 file changed, 14 insertions(+), 14 deletions(-)

Reviewed-by: Peter Krempa 

Re: [PATCH v2 1/3] virsh: Fix logic wrt to --current flag in cmdSetmem

[Peter Krempa]

On Wed, May 19, 2021 at 12:16:55 +0200, Michal Privoznik wrote:
> In my commit of v7.1.0-rc1~376 I've simplified the logic of
> handling @flags. My assumption back then was that calling
> virDomainSetMemory() is equivalent to
> virDomainSetMemoryFlags(flags = 0). But that is not the case,
> because it is equivalent to virDomainSetMemoryFlags(flags =
> VIR_DOMAIN_AFFECT_LIVE). Fix the condition that calls the old
> API.
> 
> Fixes: b5e267e8c59a257652f88d034cb1e0ce1ed4b58a
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1961118
> Signed-off-by: Michal Privoznik 
> ---
>  tools/virsh-domain.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Peter Krempa 

Re: [libvirt PATCH 1/2] storage: add support for QCOW2 cluster_size option

[Peter Krempa]

On Thu, May 13, 2021 at 13:23:04 +0200, Pavel Hrdina wrote:
> The default value hard-coded in QEMU (64KiB) is not always the ideal.
> Having a possibility to set the cluster_size by user may in specific
> use-cases improve performance for QCOW2 images.
> 
> QEMU internally has some limits, the value has to be between 512B and
> 2048KiB and must by power of two, except when the image has Extended L2
> Entries the minimal value has to be 16KiB.
> 
> Since qemu-img ensures the value is correct and the limit is not always
> the same libvirt will not duplicate any of these checks as the error
> message from qemu-img is good enough:
> 
> Cluster size must be a power of two between 512 and 2048k
> 
> Resolves: https://gitlab.com/libvirt/libvirt/-/issues/154
> 
> Signed-off-by: Pavel Hrdina 
> ---

[...]

> @@ -867,6 +868,11 @@
>  the file image is used in VM. To create non-raw file images, it
>  requires QEMU version since 2.1. Since 
> 1.2.7
>
> +  clusterSize
> +   Changes the qcow2 cluster size where smaller size can improve 
> image
> +file size whereas larger size can improve performance.
> +Since 7.4.0

I think we should refrain from advice on how the cluster size can be
configured because it has way more nuance than this short sentence.

> +  
>features
>Format-specific features. Only used for qcow2 now.
>  Valid sub-elements are:



> diff --git a/tests/storagevolxml2xmlin/vol-qcow2-clusterSize.xml 
> b/tests/storagevolxml2xmlin/vol-qcow2-clusterSize.xml
> new file mode 100644
> index 00..22534982a1
> --- /dev/null
> +++ b/tests/storagevolxml2xmlin/vol-qcow2-clusterSize.xml
> @@ -0,0 +1,17 @@
> +
> +  OtherDemo.img
> +  /var/lib/libvirt/images/OtherDemo.img
> +  5
> +  294912
> +  
> +/var/lib/libvirt/images/OtherDemo.img
> +

I wonder whether it wouldn't make more sense to stash cluster size under
the 'format' tag.

Does the LVM volume XML have  ?

> +
> +  0644
> +  0
> +  0
> +  unconfined_u:object_r:virt_image_t:s0
> +
> +128
> +  
> +

Re: [PATCH] conf: Report alias name of the detached device in error

[Kristina Hanicova]

On Wed, May 19, 2021 at 9:58 AM Michal Prívozník 
wrote:

> On 5/18/21 6:07 PM, Laine Stump wrote:
> > On 5/18/21 5:44 AM, Kristina Hanicova wrote:
> >> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1942367
> >>
> >> Signed-off-by: Kristina Hanicova 
> >> ---
> >>   src/conf/domain_conf.c | 19 +--
> >>   1 file changed, 13 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> >> index 7044701fac..e21b9fb946 100644
> >> --- a/src/conf/domain_conf.c
> >> +++ b/src/conf/domain_conf.c
> >> @@ -15781,38 +15781,45 @@ virDomainNetFindIdx(virDomainDef *def,
> >> virDomainNetDef *net)
> >>   if (matchidx < 0) {
> >>   if (MACAddrSpecified && PCIAddrSpecified) {
> >>   virReportError(VIR_ERR_DEVICE_MISSING,
> >> -   _("no device matching MAC address %s found
> >> on "
> >> +   _("no device matching MAC address %s and
> >> alias %s found on "
> >>VIR_PCI_DEVICE_ADDRESS_FMT),
> >>  virMacAddrFormat(>mac, mac),
> >> +   NULLSTR(net->info.alias),
> >>  net->info.addr.pci.domain,
> >>  net->info.addr.pci.bus,
> >>  net->info.addr.pci.slot,
> >>  net->info.addr.pci.function);
> >>   } else if (MACAddrSpecified && CCWAddrSpecified) {
> >>   virReportError(VIR_ERR_DEVICE_MISSING,
> >> -   _("no device matching MAC address %s found
> >> on "
> >> +   _("no device matching MAC address %s and
> >> alias %s  found on "
> >
> > These messages will look strange in the (most common) case where alias
> > isn't specified, e.g.:
> >
> > no device matching MAC address DE:AD:BE:EF:01:10
> > and alias   found on [some CCW address]
> >
> > On the other hand, the idea of even further exploding this bunch of
> > conditionals to include all combinations is just horrible to think about!
> >
> > What about instead reworking this to use a single virReportError() that
> > references a few pointers setup beforehand and then substituting (a
> > properly i8n'ized!) "(unspecified)" for each item that hasn't been
> > specified, e.g.:
> >
> > g_autofree *addr = g_strdup(_("(unspecified)"));
> > const char *mac = _("(unspecified)");
> > const char *alias = _("(unspecified)");
> >
> > if (MACAddrSpecified)
> > mac = virMacAddrFormat(>mac, mac);
> > if (net->info.alias)
> > alias = net->info.alias
> >
> > if (CCWAddrSpecified)
> >addr = virCCWAddressAsString(blah);
> > else if (PCIAddrSpecified)
> >addr = virPCIDeviceAddressAsString(blah);
> >
> > virReportError(blah...
> >_("no device found at address '%s' matching MAC address
> > '%s' and alias '%s'"),
> >addr, mac, alias);
> >
> > or something like that. It's still not ideal, but avoids the conditional
> > explosion and I think is less confusing than having "alias" followed by
> > nothing.
>
> IIUC, NULLSTR() will expand to "" not an empty string.
> "unspecified" sounds better. What I worry about is translations: in my
> native language and it's not a problem to have the error message split
> as you suggest. But maybe there are some languages where it might be
> problem?
>
> On the other hand - we can go with your suggestion and change this later
> as soon as we learn it's problematic for translators.
>
> Kristina, what's your opinion?
>
> Michal
>
>
I think that it can be reworked in a way, that we will have a bool variable
for
each item that can fail, e.g.:

bool aliasMatched = true;
bool addrMatched = true;
bool macMatched = true;

And we would set the corresponding variable to false if they did not match
before continuing. When reporting error, we would only report the one last
thing
it specifically failed on:

if (!aliasMatched)
virReportError(VIR_ERR_DEVICE_MISSING,
   _("no device matching alias %s found"),
   net->info.alias);

And so on.
But, it might be misleading in case more items did not match.

Maybe we can still go with Laine's suggestion and replace "unspecified"
with "" if we worry about translations?

Kristína

Re: [PATCH libvirt v2] tests: Add capabilities for QEMU 6.0.0 on s390x

[Shalini Chellathurai Saroja]

On 5/19/21 10:41 AM, Andrea Bolognani wrote:

On Tue, May 18, 2021 at 03:53:09PM +0200, Shalini Chellathurai Saroja wrote:

Introduce replies and xml files for QEMU 6.0.0 on s390x.

Signed-off-by: Shalini Chellathurai Saroja 
---
The replies file is removed from this patch and is available in
https://gitlab.com/shalinichellathurai/libvirt/-/commit/3fb78dee79a668cae084d4a6d6ed2aa096e25004

  tests/domaincapsdata/qemu_6.0.0.s390x.xml |   238 +
  .../caps_6.0.0.s390x.replies  | 27223 
  .../qemucapabilitiesdata/caps_6.0.0.s390x.xml |  3305 ++
  ...default-video-type-s390x.s390x-latest.args | 6 +-
  .../disk-error-policy-s390x.s390x-latest.args | 4 +-
  .../fs9p-ccw.s390x-latest.args| 4 +-
  ...tdev-subsys-mdev-vfio-ap.s390x-latest.args | 4 +-
  ...ubsys-mdev-vfio-ccw-boot.s390x-latest.args | 4 +-
  ...othreads-virtio-scsi-ccw.s390x-latest.args | 8 +-
  ...t-cpu-kvm-ccw-virtio-2.7.s390x-latest.args | 4 +-
  ...t-cpu-kvm-ccw-virtio-4.2.s390x-latest.args | 6 +-
  ...t-cpu-tcg-ccw-virtio-2.7.s390x-latest.args | 4 +-
  ...t-cpu-tcg-ccw-virtio-4.2.s390x-latest.args | 4 +-
  .../s390x-ccw-graphics.s390x-latest.args  | 6 +-
  .../s390x-ccw-headless.s390x-latest.args  | 6 +-
  .../vhost-vsock-ccw-auto.s390x-latest.args| 4 +-
  .../vhost-vsock-ccw-iommu.s390x-latest.args   | 4 +-
  .../vhost-vsock-ccw.s390x-latest.args | 4 +-
  18 files changed, 30802 insertions(+), 36 deletions(-)
  create mode 100644 tests/domaincapsdata/qemu_6.0.0.s390x.xml
  create mode 100644 tests/qemucapabilitiesdata/caps_6.0.0.s390x.replies
  create mode 100644 tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml

Looks great now, thanks!

After rebasing on top of master, domaincapstest was failing so I ran

   $ VIR_TEST_REGENERATE_OUTPUT=1 meson test

and squashed in the resulting diff, which you can see below.

With that taken care of,

   Reviewed-by: Andrea Bolognani 

and pushed.


Hello Andrea,

Thank you for the review and acceptance of this patch.



diff --git a/tests/domaincapsdata/qemu_6.0.0.s390x.xml
b/tests/domaincapsdata/qemu_6.0.0.s390x.xml
index 00b8f2c9c5..d384f0859b 100644
--- a/tests/domaincapsdata/qemu_6.0.0.s390x.xml
+++ b/tests/domaincapsdata/qemu_6.0.0.s390x.xml
@@ -226,6 +226,13 @@
  builtin

  
+
+  
+path
+handle
+virtiofs
+  
+


  


--
Kind regards
Shalini Chellathurai Saroja
Linux on Z and Virtualization Development
Vorsitzende des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: RFC: Qemu backup interface plans

[Kevin Wolf]

Am 19.05.2021 um 13:49 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 19.05.2021 14:20, Kevin Wolf wrote:
> > Am 19.05.2021 um 08:11 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > > > 2. Test, that we can start backup job with source = (target of
> > > > > backup-top filter), so that we have "push backup with fleecing".
> > > > > Make an option for backup to start without a filter, when we don't
> > > > > need copy-before-write operations, to not create extra superfluous
> > > > > filter.
> > > > 
> > > > OK, so the backup job is not really a backup job, but just anything
> > > > that copies data.
> > > 
> > > Not quite. For backup without a filter we should protect source from
> > > changing, by unsharing WRITE permission on it.
> > > 
> > > I'll try to avoid adding an option. The logic should work like in
> > > commit job: if there are current writers on source we create filter.
> > > If there no writers, we just unshare writes and go without a filter.
> > > And for this copy-before-write filter should be able to do
> > > WRITE_UNCHANGED in case of fleecing.
> > 
> > If we ever get to the point where we would make a block-copy job visible
> > to the user, I wouldn't copy the restrictions from the current jobs, but
> > keep it really generic to cover all cases.
> > 
> > There is no way for the QMP command starting the job to know what the
> > user is planning to do with the image in the future. Even if it's
> > currently read-only, the user may want to add a writer later.
> > 
> > I think this means that we want to always add a filter node, and then
> > possibly dynamically switch between modes if being read-only provides a
> > significant advantage for the job.
> 
> Still, in push-backup-with-fleecing scheme we really don't need the
> second filter, so why to insert extra thing to block graph?
> 
> I see your point still, that user may want to add writer later. Still,
> I'd be surprised if such use-cases exist now.
> 
> What about the following:
> 
> add some source-mode tri-state parameter for backup:
> 
> auto: insert filter iff there are existing writers [default]
> filtered: insert filter unconditionally
> immutable: don't insert filter. will fail if there are existing
> writers, and creating writers during block-job would be impossible

Yes, that's an option, too.

Kevin

Re: Qemu block filter insertion/removal API

[Kevin Wolf]

Am 17.05.2021 um 14:44 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Hi all!
> 
> I'd like to be sure that we know where we are going to.
> 
> In blockdev-era where qemu user is aware about block nodes, all nodes have 
> good names and controlled by user we can efficiently use block filters.
> 
> We already have some useful filters: copy-on-read, throttling, compress. In 
> my parallel series I make backup-top filter public and useful without backup 
> block jobs. But now filters could be inserted only together with opening 
> their child. We can specify filters in qemu cmdline, or filter can take place 
> in the block node chain created by blockdev-add.
> 
> Still, it would be good to insert/remove filters on demand.
> 
> Currently we are going to use x-blockdev-reopen for this. Still it can't be 
> used to insert a filter above root node (as x-blockdev-reopen can change only 
> block node options and their children). In my series "[PATCH 00/21] block: 
> publish backup-top filter" I propose (as Kevin suggested) to modify qom-set, 
> so that it can set drive option of running device. That's not difficult, but 
> it means that we have different scenario of inserting/removing filters:
> 
> 1. filter above root node X:
> 
> inserting:
> 
>   - do blockdev-add to add a filter (and specify X as its child)
>   - do qom-set to set new filter as a rood node instead of X
> 
> removing
> 
>   - do qom-set to make X a root node again
>   - do blockdev-del to drop a filter
> 
> 2. filter between two block nodes P and X. (For example, X is a backing child 
> of P)
> 
> inserting
> 
>   - do blockdev-add to add a filter (and specify X as its child)
>   - do blockdev-reopen to set P.backing = filter
> 
> remvoing
> 
>   - do blockdev-reopen to set P.backing = X
>   - do blockdev-del to drop a filter
> 
> 
> And, probably we'll want transaction support for all these things.
> 
> 
> Is it OK? Or do we need some kind of additional blockdev-replace command, 
> that can replace one node by another, so in both cases we will do
> 
> inserting:
>   - blockdev-add filter
>   - blockdev-replace (make all parents of X to point to the new filter 
> instead (except for the filter itself of course)
> 
> removing
>   - blockdev-replace (make all parante of filter to be parents of X instead)
>   - blockdev-del filter
> 
> It's simple to implement, and it seems for me that it is simpler to use. Any 
> thoughts?

One reason I remember why we didn't decide to go this way in the many
"dynamic graph reconfiguration" discussions we had, is that it's not
generic enough to cover all cases. But I'm not sure if we ever
considered root nodes as a separate case. I acknowledge that having two
different interfaces is inconvenient, and integrating qom-set in a
transaction is rather unlikely to happen.

The reason why it's not generic is that it restricts you to doing the
same thing for all parents. Imagine this:

+- virtio-blk
|
file <- qcow2 <-+
|
+- NBD export

Now you want to throttle the NBD export so that it doesn't interfere
with your VM too much. With your simple blockdev-replace this isn't
possible. You would have to add the filter to both users or to none.

In theory, blockdev-replace could take a list of the edges that should
be changed to the new node. The problem is that edges don't have names,
and even the parents don't necessarily have one (and if they do, they
are in separate namespaces, so a BlockBackend, a job and an export could
all have the same name), so finding a good way to refer to them in QMP
doesn't sound trivial.

Kevin

Re: RFC: Qemu backup interface plans

[Kevin Wolf]

Am 19.05.2021 um 08:11 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > 2. Test, that we can start backup job with source = (target of
> > > backup-top filter), so that we have "push backup with fleecing".
> > > Make an option for backup to start without a filter, when we don't
> > > need copy-before-write operations, to not create extra superfluous
> > > filter.
> > 
> > OK, so the backup job is not really a backup job, but just anything
> > that copies data.
> 
> Not quite. For backup without a filter we should protect source from
> changing, by unsharing WRITE permission on it.
> 
> I'll try to avoid adding an option. The logic should work like in
> commit job: if there are current writers on source we create filter.
> If there no writers, we just unshare writes and go without a filter.
> And for this copy-before-write filter should be able to do
> WRITE_UNCHANGED in case of fleecing.

If we ever get to the point where we would make a block-copy job visible
to the user, I wouldn't copy the restrictions from the current jobs, but
keep it really generic to cover all cases.

There is no way for the QMP command starting the job to know what the
user is planning to do with the image in the future. Even if it's
currently read-only, the user may want to add a writer later.

I think this means that we want to always add a filter node, and then
possibly dynamically switch between modes if being read-only provides a
significant advantage for the job.

Kevin

Re: Qemu block filter insertion/removal API

[Vladimir Sementsov-Ogievskiy]

18.05.2021 19:49, Max Reitz wrote:

On 17.05.21 14:44, Vladimir Sementsov-Ogievskiy wrote:

Hi all!

I'd like to be sure that we know where we are going to.

In blockdev-era where qemu user is aware about block nodes, all nodes have good 
names and controlled by user we can efficiently use block filters.

We already have some useful filters: copy-on-read, throttling, compress. In my 
parallel series I make backup-top filter public and useful without backup block 
jobs. But now filters could be inserted only together with opening their child. 
We can specify filters in qemu cmdline, or filter can take place in the block 
node chain created by blockdev-add.

Still, it would be good to insert/remove filters on demand.

Currently we are going to use x-blockdev-reopen for this. Still it can't be used to 
insert a filter above root node (as x-blockdev-reopen can change only block node options 
and their children). In my series "[PATCH 00/21] block: publish backup-top 
filter" I propose (as Kevin suggested) to modify qom-set, so that it can set drive 
option of running device. That's not difficult, but it means that we have different 
scenario of inserting/removing filters:

1. filter above root node X:

inserting:

   - do blockdev-add to add a filter (and specify X as its child)
   - do qom-set to set new filter as a rood node instead of X

removing

   - do qom-set to make X a root node again
   - do blockdev-del to drop a filter

2. filter between two block nodes P and X. (For example, X is a backing child 
of P)

inserting

   - do blockdev-add to add a filter (and specify X as its child)
   - do blockdev-reopen to set P.backing = filter

remvoing

   - do blockdev-reopen to set P.backing = X
   - do blockdev-del to drop a filter


And, probably we'll want transaction support for all these things.


Is it OK? Or do we need some kind of additional blockdev-replace command, that 
can replace one node by another, so in both cases we will do

inserting:

   - blockdev-add filter
   - blockdev-replace (make all parents of X to point to the new filter instead 
(except for the filter itself of course)

removing
   - blockdev-replace (make all parante of filter to be parents of X instead)
   - blockdev-del filter


It's simple to implement, and it seems for me that it is simpler to use. Any 
thoughts?


I’m afraid as a non-user of the blockdev interface, I can’t give a valuable 
opinion that would have some actual weight.

Doesn’t stop me from giving my personal and potentially invaluable opinion, 
though, obviously:

I think we expect all users to know the block graph, so they should be able to 
distinguish between cases 1 and 2.  However, I can imagine having to 
distinguish still is kind of a pain, especially if it were trivial for qemu to 
let the user not having to worry about it at all.


I discussed it yesterday with my colleagues from Virtuozzo, who will have to be 
users of that interface. And they of course prefer one command for all the 
cases :)



Also, if you want a filter unconditionally above some node, all the qom-set and 
blockdev-reopen operations for all of the original node’s parents would need to 
happen atomically.  As you say, those operations should perhaps be 
transactionable anyway, but...  Implementing blockdev-replace would provide 
this for much less cost now, I suppose?

I guess it can be argued that the downside is that having blockdev-replace 
means less pressure to make qom-set for drive and blockdev-reopen 
transactionable.

But well.  I don’t really have anything against a blockdev-replace, but again, 
I don’t know whether my opinion on this topic really has weight.


Thanks, actually my opinion is the same. I think, I'll prepare a patch a day 
later if no answers here, and we'll be able to continue discussion on top of 
new patch.

Hmm I have one additional (weak, but still) argument for blockdev-replace: it 
just seems good to avoid touching extra subsystem in block-graph operations. 
For block-jobs we don't need to touch qdev guest block devices, we are good now 
with node-names and blockdev-add. So, it's good to save this bit of interface 
beauty if we don't have strict reason to drop it.

--
Best regards,
Vladimir

[PATCH v2 3/3] virsh-domain: Drop support for old APIs in cmdSetmem and cmdSetmaxmem

[Michal Privoznik]

Some of our really old APIs are missing @flags argument. We
introduced their variants with "Flags" suffix and wired some
logic into virsh to call the new variant only if necessary. This
enables virsh to talk to older daemon which may be lacking new
APIs.

However, in case of cmdSetmem() we are talking about v0.1.1
(virDomainSetMemory()) vs. v0.9.0 (virDomainSetMemoryFlags()) and
in case of cmdSetmaxmem() we are talking about v0.0.3
(virDomainSetMaxMemory()) vs v0.9.0 (virDomainSetMemoryFlags()).

Libvirt v0.9.0 was released more than 10 years ago and recently
we dropped support for RHEL-7 which has v4.5.0 (released ~3 years
ago). Thus it is not really necessary to have support in virsh
for such old daemons.

Signed-off-by: Michal Privoznik 
---
 tools/virsh-domain.c | 28 
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 5a5215ab4c..81357d23aa 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -9000,15 +9000,15 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd)
 bool config = vshCommandOptBool(cmd, "config");
 bool live = vshCommandOptBool(cmd, "live");
 bool current = vshCommandOptBool(cmd, "current");
-unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
+unsigned int flags = VIR_DOMAIN_AFFECT_LIVE;
 
 VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
 VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
 
+if (current)
+flags = VIR_DOMAIN_AFFECT_CURRENT;
 if (config)
 flags |= VIR_DOMAIN_AFFECT_CONFIG;
-if (live)
-flags |= VIR_DOMAIN_AFFECT_LIVE;
 
 if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
 return false;
@@ -9023,13 +9023,8 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd)
 goto cleanup;
 kibibytes = VIR_DIV_UP(bytes, 1024);
 
-if (!current && !live && !config) {
-if (virDomainSetMemory(dom, kibibytes) != 0)
-goto cleanup;
-} else {
-if (virDomainSetMemoryFlags(dom, kibibytes, flags) < 0)
-goto cleanup;
-}
+if (virDomainSetMemoryFlags(dom, kibibytes, flags) < 0)
+goto cleanup;
 
 ret = true;
  cleanup:
@@ -9101,16 +9096,9 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd)
 goto cleanup;
 kibibytes = VIR_DIV_UP(bytes, 1024);
 
-if (flags == 0) {
-if (virDomainSetMaxMemory(dom, kibibytes) != 0) {
-vshError(ctl, "%s", _("Unable to change MaxMemorySize"));
-goto cleanup;
-}
-} else {
-if (virDomainSetMemoryFlags(dom, kibibytes, flags | 
VIR_DOMAIN_MEM_MAXIMUM) < 0) {
-vshError(ctl, "%s", _("Unable to change MaxMemorySize"));
-goto cleanup;
-}
+if (virDomainSetMemoryFlags(dom, kibibytes, flags | 
VIR_DOMAIN_MEM_MAXIMUM) < 0) {
+vshError(ctl, "%s", _("Unable to change MaxMemorySize"));
+goto cleanup;
 }
 
 ret = true;
-- 
2.26.3

[PATCH v2 0/3] virsh: Fix logic wrt to --current flag in cmdSetmem

[Michal Privoznik]

v2 of:

https://listman.redhat.com/archives/libvir-list/2021-May/msg00484.html

diff to v1:
- Work in Jano's and Peter's review suggestions in 1/3.
- Patches 2/3 and 3/3 are new. I've done 2/3 before 3/3 because it's
still worth merging even if 3/3 is disliked.

Michal Prívozník (3):
  virsh: Fix logic wrt to --current flag in cmdSetmem
  virsh-domain: Fix @ret handling in cmdSetmem and cmdSetmaxmem
  virsh-domain: Drop support for old APIs in cmdSetmem and cmdSetmaxmem

 tools/virsh-domain.c | 48 +---
 1 file changed, 18 insertions(+), 30 deletions(-)

-- 
2.26.3

[PATCH v2 1/3] virsh: Fix logic wrt to --current flag in cmdSetmem

[Michal Privoznik]

In my commit of v7.1.0-rc1~376 I've simplified the logic of
handling @flags. My assumption back then was that calling
virDomainSetMemory() is equivalent to
virDomainSetMemoryFlags(flags = 0). But that is not the case,
because it is equivalent to virDomainSetMemoryFlags(flags =
VIR_DOMAIN_AFFECT_LIVE). Fix the condition that calls the old
API.

Fixes: b5e267e8c59a257652f88d034cb1e0ce1ed4b58a
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1961118
Signed-off-by: Michal Privoznik 
---
 tools/virsh-domain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 0825f82522..9316aae3fd 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -9025,7 +9025,7 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd)
 }
 kibibytes = VIR_DIV_UP(bytes, 1024);
 
-if (flags == 0) {
+if (!current && !live && !config) {
 if (virDomainSetMemory(dom, kibibytes) != 0)
 ret = false;
 } else {
-- 
2.26.3

[PATCH v2 2/3] virsh-domain: Fix @ret handling in cmdSetmem and cmdSetmaxmem

[Michal Privoznik]

These functions initialize @ret to true and only after something
fails either they call cleanup code (which consists only from
virshDomainFree()) and return false, or they set ret = false and
carry on (when the failure occurred close to cleanup code).

Switch them to the usual pattern in which ret is initialized to
failure, goto cleanup is used and ret is set to true only after
everything succeeded.

Signed-off-by: Michal Privoznik 
---
 tools/virsh-domain.c | 28 ++--
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 9316aae3fd..5a5215ab4c 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -8996,7 +8996,7 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd)
 unsigned long long bytes = 0;
 unsigned long long max;
 unsigned long kibibytes = 0;
-bool ret = true;
+bool ret = false;
 bool config = vshCommandOptBool(cmd, "config");
 bool live = vshCommandOptBool(cmd, "live");
 bool current = vshCommandOptBool(cmd, "current");
@@ -9019,20 +9019,20 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd)
 max = 1024ull * ULONG_MAX;
 else
 max = ULONG_MAX;
-if (vshCommandOptScaledInt(ctl, cmd, "size", , 1024, max) < 0) {
-virshDomainFree(dom);
-return false;
-}
+if (vshCommandOptScaledInt(ctl, cmd, "size", , 1024, max) < 0)
+goto cleanup;
 kibibytes = VIR_DIV_UP(bytes, 1024);
 
 if (!current && !live && !config) {
 if (virDomainSetMemory(dom, kibibytes) != 0)
-ret = false;
+goto cleanup;
 } else {
 if (virDomainSetMemoryFlags(dom, kibibytes, flags) < 0)
-ret = false;
+goto cleanup;
 }
 
+ret = true;
+ cleanup:
 virshDomainFree(dom);
 return ret;
 }
@@ -9074,7 +9074,7 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd)
 unsigned long long bytes = 0;
 unsigned long long max;
 unsigned long kibibytes = 0;
-bool ret = true;
+bool ret = false;
 bool config = vshCommandOptBool(cmd, "config");
 bool live = vshCommandOptBool(cmd, "live");
 bool current = vshCommandOptBool(cmd, "current");
@@ -9097,24 +9097,24 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd)
 max = 1024ull * ULONG_MAX;
 else
 max = ULONG_MAX;
-if (vshCommandOptScaledInt(ctl, cmd, "size", , 1024, max) < 0) {
-virshDomainFree(dom);
-return false;
-}
+if (vshCommandOptScaledInt(ctl, cmd, "size", , 1024, max) < 0)
+goto cleanup;
 kibibytes = VIR_DIV_UP(bytes, 1024);
 
 if (flags == 0) {
 if (virDomainSetMaxMemory(dom, kibibytes) != 0) {
 vshError(ctl, "%s", _("Unable to change MaxMemorySize"));
-ret = false;
+goto cleanup;
 }
 } else {
 if (virDomainSetMemoryFlags(dom, kibibytes, flags | 
VIR_DOMAIN_MEM_MAXIMUM) < 0) {
 vshError(ctl, "%s", _("Unable to change MaxMemorySize"));
-ret = false;
+goto cleanup;
 }
 }
 
+ret = true;
+ cleanup:
 virshDomainFree(dom);
 return ret;
 }
-- 
2.26.3

Re: RFC: Qemu backup interface plans

[Vladimir Sementsov-Ogievskiy]

18.05.2021 19:39, Max Reitz wrote:

Hi,

Your proposal sounds good to me in general.  Some small independent building 
blocks that seems to make sense to me.


Thanks! I hope it's not too difficult to read and understand my English.




On 17.05.21 14:07, Vladimir Sementsov-Ogievskiy wrote:

[...]


What we lack in this scheme:

1. handling dirty bitmap in backup-top filter: backup-top does copy-before-write 
operation on any guest write, when actually we are interested only in "dirty" 
regions for incremental backup

Probable solution would allowing specifying bitmap for sync=none mode of 
backup, but I think what I propose below is better.

2. [actually it's a tricky part of 1]: possibility to not do copy-before-write 
operations for regions that was already copied to final backup. With normal 
Qemu backup job, this is achieved by the fact that block-copy state with its 
internal bitmap is shared between backup job and copy-before-write filter.

3. Not a real problem but fact: backup block-job does nothing in the scheme, 
the whole job is done by filter. So, it would be interesting to have a 
possibility to simply insert/remove the filter, and avoid block-job creation 
and managing at all for external backup. (and I'd like to send another RFC on 
how to insert/remove filters, let's not discuss it here).


Next. Think about internal backup. It has one drawback too:
4. If target is remote with slow connection, copy-before-write operations will 
slow down guest writes appreciably.

It may be solved with help of image fleecing: we create temporary qcow2 image, 
setup fleecing scheme, and instead of exporting temp image through NBD we start 
a second backup with source = temporary image and target would be real backup 
target (NBD for example).


How would a second backup work here?  Wouldn’t one want a mirror job to copy 
the data off to the real target?

(Because I see backup as something intrinsically synchronous, whereas mirror by 
default is rather lazy.)

[Note from future me where I read more below: I see you acknowledge that you’ll 
need to modify backup to do what you need here, i.e. not do any CBW operations. 
 So it’s effectively the same as a mirror that ignores new dirty areas.  Which 
could work without changing mirror if block-copy were to set 
BDRV_REQ_WRITE_UNCHANGED for the fleecing case, and bdrv_co_write_req_finish() 
would skip bdrv_set_dirty() for such writes.]


I just feel myself closer with backup block-job than with mirror :) Finally, 
yes, there is no real difference in interface. But in realization, I prefer to 
continue developing block-copy. I hope, finally all jobs and img-convert would 
work through block-copy.

(and I'll need BDRV_REQ_WRITE_UNCHANGED anyway for fleecing, so user can use 
mirror or backup)



I mean, still has the problem that the mirror job can’t tell the CBW filter 
which areas are already copied off and so don’t need to be preserved anymore, 
but...


Still, with such solution there are same [1,2] problems, 3 becomes worse:


Not sure how 3 can become worse when you said above it isn’t a real problem (to 
which I agree).


It's my perfectionism :) Yes, it's still isn't a problem, but number of extra 
user-visible objects in architecture increases, which is not good I think.




5. We'll have two jobs and two automatically inserted filters, when actually 
one filter and one job are enough (as first job is needed only to insert a 
filter, second job doesn't need a filter at all).

Note also, that this (starting two backup jobs to make push backup with 
fleecing) doesn't work now, op-blockers will be against. It's simple to fix 
(and in Virtuozzo we live with downstream-only patch, which allows push backup 
with fleecing, based on starting two backup jobs).. But I never send a patch, 
as I have better plan, which will solve all listed problems.


So, what I propose:

1. We make backup-top filter public, so that it could be inserted/removed where 
user wants through QMP (how to properly insert/remove filter I'll post another 
RFC, as backup-top is not the only filter that can be usefully inserted 
somewhere). For this first step I've sent a series today:

   subject: [PATCH 00/21] block: publish backup-top filter
   id: <20210517064428.16223-1-vsement...@virtuozzo.com>
   patchew: 
https://patchew.org/QEMU/20210517064428.16223-1-vsement...@virtuozzo.com/

(note, that one of things in this series is rename 
s/backup-top/copy-before-write/, still, I call it backup-top in this letter)

This solves [3]. [4, 5] are solved partly: we still have one extra filter, 
created by backup block jobs, and also I didn't test does this work, probably 
some op-blockers or permissions should be tuned. So, let it be step 2:

2. Test, that we can start backup job with source = (target of backup-top filter), so 
that we have "push backup with fleecing". Make an option for backup to start 
without a filter, when we don't need copy-before-write operations, to not create extra 

[PATCH v2] libxl: adjust handling of libxl_device_nic objects

[Olaf Hering]

libxl objects are supposed to be initialized and disposed.
Adjust libxlMakeNic to use an already initialized object, it is owned by
the caller.

Adjust libxlMakeNicList to initialize the list of objects, before they
are filled by libxlMakeNic. In case of error the objects are disposed
by libxl_domain_config_dispose.

The usage of g_new0 is suspicious in the context of libxl because the
memory allocated via glib is released with plain free() inside libxl.

Signed-off-by: Olaf Hering 
---
 src/libxl/libxl_conf.c | 36 +---
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 2d2aab7e66..e4afa578b0 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1345,8 +1345,6 @@ libxlMakeNic(virDomainDef *def,
 return -1;
 }
 
-libxl_device_nic_init(x_nic);
-
 virMacAddrGetRaw(_nic->mac, x_nic->mac);
 
 /*
@@ -1532,39 +1530,39 @@ libxlMakeNicList(virDomainDef *def,  
libxl_domain_config *d_config)
 {
 virDomainNetDef **l_nics = def->nets;
 size_t nnics = def->nnets;
-libxl_device_nic *x_nics;
 size_t i, nvnics = 0;
-
-x_nics = g_new0(libxl_device_nic, nnics);
+int ret = -1;
 
 for (i = 0; i < nnics; i++) {
 if (virDomainNetGetActualType(l_nics[i]) == 
VIR_DOMAIN_NET_TYPE_HOSTDEV)
 continue;
+nvnics++;
+}
+if (!nvnics)
+return 0;
+
+d_config->nics = g_new0(libxl_device_nic, nvnics);
+d_config->num_nics = nvnics;
+
+for (i = 0; i < nvnics; i++)
+libxl_device_nic_init(_config->nics[i]);
 
-if (libxlMakeNic(def, l_nics[i], _nics[nvnics], false))
+for (i = 0; i < nvnics; i++) {
+if (libxlMakeNic(def, l_nics[i], _config->nics[i], false))
 goto error;
 /*
  * The devid (at least right now) will not get initialized by
  * libxl in the setup case but is required for starting the
  * device-model.
  */
-if (x_nics[nvnics].devid < 0)
-x_nics[nvnics].devid = nvnics;
-
-nvnics++;
+if (d_config->nics[i].devid < 0)
+d_config->nics[i].devid = i;
 }
 
-VIR_SHRINK_N(x_nics, nnics, nnics - nvnics);
-d_config->nics = x_nics;
-d_config->num_nics = nvnics;
-
-return 0;
+ret = 0;
 
  error:
-for (i = 0; i < nnics; i++)
-libxl_device_nic_dispose(_nics[i]);
-VIR_FREE(x_nics);
-return -1;
+return ret;
 }
 
 int

Re: [PATCH v1] libxl: adjust handling of libxl_device_nic objects

[Olaf Hering]

Am Wed, 19 May 2021 10:44:17 +0200
schrieb Olaf Hering :

>  for (i = 0; i < nnics; i++) {
>  if (virDomainNetGetActualType(l_nics[i]) == 
> VIR_DOMAIN_NET_TYPE_HOSTDEV)
> -continue;
> +nvnics++;
> +}

This part inverts the logic, should look like that instead:

 for (i = 0; i < nnics; i++) {
 if (virDomainNetGetActualType(l_nics[i]) == 
VIR_DOMAIN_NET_TYPE_HOSTDEV)
 continue;
+nvnics++;
+}


Olaf


pgpufzhUDAW2j.pgp
Description: Digitale Signatur von OpenPGP

[PATCH v1] libxl: adjust handling of libxl_device_nic objects

[Olaf Hering]

libxl objects are supposed to be initialized and disposed.
Adjust libxlMakeNic to use an already initialized object, it is owned by
the caller.

Adjust libxlMakeNicList to initialize the list of objects, before they
are filled by libxlMakeNic. In case of error the objects are disposed
by libxl_domain_config_dispose.

The usage of g_new0 is suspicious in the context of libxl because the
memory allocated via glib is released with plain free() inside libxl.

Signed-off-by: Olaf Hering 
---
 src/libxl/libxl_conf.c | 37 +
 1 file changed, 17 insertions(+), 20 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 2d2aab7e66..a2e73c8b4d 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1345,8 +1345,6 @@ libxlMakeNic(virDomainDef *def,
 return -1;
 }
 
-libxl_device_nic_init(x_nic);
-
 virMacAddrGetRaw(_nic->mac, x_nic->mac);
 
 /*
@@ -1532,39 +1530,38 @@ libxlMakeNicList(virDomainDef *def,  
libxl_domain_config *d_config)
 {
 virDomainNetDef **l_nics = def->nets;
 size_t nnics = def->nnets;
-libxl_device_nic *x_nics;
 size_t i, nvnics = 0;
-
-x_nics = g_new0(libxl_device_nic, nnics);
+int ret = -1;
 
 for (i = 0; i < nnics; i++) {
 if (virDomainNetGetActualType(l_nics[i]) == 
VIR_DOMAIN_NET_TYPE_HOSTDEV)
-continue;
+nvnics++;
+}
+if (!nvnics)
+return 0;
+
+d_config->nics = g_new0(libxl_device_nic, nvnics);
+d_config->num_nics = nvnics;
 
-if (libxlMakeNic(def, l_nics[i], _nics[nvnics], false))
+for (i = 0; i < nvnics; i++)
+libxl_device_nic_init(_config->nics[i]);
+
+for (i = 0; i < nvnics; i++) {
+if (libxlMakeNic(def, l_nics[i], _config->nics[i], false))
 goto error;
 /*
  * The devid (at least right now) will not get initialized by
  * libxl in the setup case but is required for starting the
  * device-model.
  */
-if (x_nics[nvnics].devid < 0)
-x_nics[nvnics].devid = nvnics;
-
-nvnics++;
+if (d_config->nics[i].devid < 0)
+d_config->nics[i].devid = i;
 }
 
-VIR_SHRINK_N(x_nics, nnics, nnics - nvnics);
-d_config->nics = x_nics;
-d_config->num_nics = nvnics;
-
-return 0;
+ret = 0;
 
  error:
-for (i = 0; i < nnics; i++)
-libxl_device_nic_dispose(_nics[i]);
-VIR_FREE(x_nics);
-return -1;
+return ret;
 }
 
 int

Re: [libvirt PATCH 4/4] docs: nodedev: document mdev uuid property

[Michal Prívozník]

On 5/14/21 11:29 PM, Jonathon Jongsma wrote:
> Signed-off-by: Jonathon Jongsma 
> ---
>  docs/formatnode.html.in | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in
> index 3065570405..9a505f0fe9 100644
> --- a/docs/formatnode.html.in
> +++ b/docs/formatnode.html.in
> @@ -384,6 +384,10 @@
>  The order that they appear in the xml definition determines 
> the
>  order that they will be written to the device.
>
> +  uuid
> +  
> +This element represents the UUID of the mediated device.
> +  
>start
>
>  This element represents the start policy for the device.  It
> 

Reviewed-by: Michal Privoznik 

and pushed because this is independent of the rest.

Michal

Re: [libvirt PATCH 2/4] nodedev: support auto-start property for mdevs

[Michal Prívozník]

On 5/14/21 11:28 PM, Jonathon Jongsma wrote:
> From: Boris Fiuczynski 
> 
> This adds a new element to the mdev capabilities xml schema that
> represents the start policy for a defined mediated device. The actual
> auto-start functionality is handled behind the scenes by mdevctl, but it
> wasn't yet hooked up in libvirt.
> 
> Signed-off-by: Boris Fiuczynski 
> Signed-off-by: Jonathon Jongsma 
> ---
>  docs/formatnode.html.in   | 10 ++
>  docs/schemas/nodedev.rng  | 11 ++
>  src/conf/node_device_conf.c   | 20 ++-
>  src/conf/node_device_conf.h   | 12 +++
>  src/libvirt_private.syms  |  2 ++
>  src/node_device/node_device_driver.c  |  7 ++-
>  .../mdevctl-list-multiple.out.xml |  4 
>  ...v_3627463d_b7f0_4fea_b468_f1da537d301b.xml |  1 +
>  ...v_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad.xml |  1 +
>  9 files changed, 66 insertions(+), 2 deletions(-)
> 


> diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
> index a60562e4fe..1a31133c4c 100644
> --- a/src/conf/node_device_conf.h
> +++ b/src/conf/node_device_conf.h
> @@ -124,6 +124,17 @@ typedef enum {
>  
>  VIR_ENUM_DECL(virNodeDevDRM);
>  
> +typedef enum {
> +/* Keep in sync with VIR_ENUM_IMPL in node_device_conf.c */

I know you copied this from pre-existing code, but it shouldn't be there
either. If this enum ever changes and corresponding VIR_ENUM_IMPL() is
not updated, compiler throws an error. I admit it's very well hidden,
but if you look at VIR_ENUM_IMPL() macro definition in
src/util/virenum.h at the very end of it is a G_STATIC_ASSERT() which
ensures that VIR_ENUM_IMPL() has as many items as the value of _LAST.

> +VIR_NODE_DEV_MDEV_START_MANUAL,
> +VIR_NODE_DEV_MDEV_START_AUTO,
> +
> +VIR_NODE_DEV_MDEV_START_LAST
> +} virNodeDevMdevStartType;
> +
> +VIR_ENUM_DECL(virNodeDevMdevStart);
> +
> +
>  typedef struct _virNodeDevCapSystemHardware virNodeDevCapSystemHardware;
>  struct _virNodeDevCapSystemHardware {
>  char *vendor_name;
> @@ -151,6 +162,7 @@ struct _virNodeDevCapMdev {
>  char *type;
>  unsigned int iommuGroupNumber;
>  char *uuid;
> +virNodeDevMdevStartType start;
>  virMediatedDeviceAttr **attributes;
>  size_t nattributes;
>  };

Michal

Re: [libvirt PATCH 1/4] test: move nodedev xml2xml output to a separate dir

[Michal Prívozník]

On 5/14/21 11:28 PM, Jonathon Jongsma wrote:
> Currently, we're loading and parsing the xml from the input file, and
> then formatting it and then comparing it directly back to the input
> file. This works for now, but is severely limiting as it relies on the
> input file being fully-specified and in the exact order as the output
> xml format.
> 
> If optional elements are ommitted in the input XML, the output xml
> may include default values for the ommitted elements and thus the output
> will not match the input.
> 
> In order to allow more flexibility in testing, save the expected output
> to a seprate 'out' directory similar to what most of the other xml2xml
> tests are already doing.
> 
> Signed-off-by: Jonathon Jongsma 
> ---

Fair enough - we have plenty of examples around, qemuxml2xmloutdata/ to
name the biggest one. However, what we also do (to save disk space) is
to turn those files where input XML is the same as output XML into
symlinks. I've identified a few which could be just a symlink:

tests/nodedevxml2xmlout $ for i in *; do if diff $i
../nodedevschemadata/$i >/dev/null ; then echo $i; fi; done
DVD_GCC_4247N.xml
DVD_with_media.xml
ap_07_0038.xml
ap_card07.xml
ap_matrix.xml
ap_matrix_mdev_types.xml


I know you are modifying some of these files (mdev*), that's why I ran
the command only after all your patches. For these patches we could make
everything a symlink in 1/4 and then as we need to make changes (in 2/4
and 3/4) remove those symlinks and make regular copies.

Do you think it's worth doing? If so, no need to resend, it's something
I can fix before pushing. Plus marking @outfile variable in
testCompareXMLToXMLHelper() as g_autofree (thanks to Pavel who noticed!).

Patches look good otherwise.

Michal

[PATCH v1] libxl: adjust handling of libxl_device_disk objects

[Olaf Hering]

libxl objects are supposed to be initialized and disposed.
Correct the usage of libxl_device_disk objects which are allocated on
the stack. Initialize each one prior usage, and dispose them once done.

Adjust libxlMakeDisk to use an already initialized object, it is owned
by the caller.

Adjust libxlMakeDiskList to initialize the list of objects, before they
are filled by libxlMakeDisk. In case of error, the objects are disposed
by libxl_domain_config_dispose.

The usage of g_new0 is suspicious in the context of libxl because the
memory allocated via glib is released with plain free() inside libxl.

Signed-off-by: Olaf Hering 
---
 src/libxl/libxl_conf.c   | 22 +-
 src/libxl/libxl_driver.c |  6 ++
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 3149ee3b4a..2d2aab7e66 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1114,8 +1114,6 @@ libxlMakeDisk(virDomainDiskDef *l_disk, libxl_device_disk 
*x_disk)
 int format = virDomainDiskGetFormat(l_disk);
 int actual_type = virStorageSourceGetActualType(l_disk->src);
 
-libxl_device_disk_init(x_disk);
-
 if (actual_type == VIR_STORAGE_TYPE_NETWORK) {
 if (STRNEQ_NULLABLE(driver, "qemu")) {
 virReportError(VIR_ERR_OPERATION_INVALID, "%s",
@@ -1265,26 +1263,24 @@ libxlMakeDiskList(virDomainDef *def, 
libxl_domain_config *d_config)
 {
 virDomainDiskDef **l_disks = def->disks;
 int ndisks = def->ndisks;
-libxl_device_disk *x_disks;
 size_t i;
+int ret = -1;
+
+d_config->disks = g_new0(libxl_device_disk, ndisks);
+d_config->num_disks = ndisks;
 
-x_disks = g_new0(libxl_device_disk, ndisks);
+for (i = 0; i < ndisks; i++)
+libxl_device_disk_init(_config->disks[i]);
 
 for (i = 0; i < ndisks; i++) {
-if (libxlMakeDisk(l_disks[i], _disks[i]) < 0)
+if (libxlMakeDisk(l_disks[i], _config->disks[i]) < 0)
 goto error;
 }
 
-d_config->disks = x_disks;
-d_config->num_disks = ndisks;
-
-return 0;
+ret = 0;
 
  error:
-for (i = 0; i < ndisks; i++)
-libxl_device_disk_dispose(_disks[i]);
-VIR_FREE(x_disks);
-return -1;
+return ret;
 }
 
 /*
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index d54cd41785..2b844bb3b5 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -2978,6 +2978,7 @@ libxlDomainChangeEjectableMedia(virDomainObj *vm, 
virDomainDiskDef *disk)
 size_t i;
 int ret = -1;
 
+libxl_device_disk_init(_disk);
 for (i = 0; i < vm->def->ndisks; i++) {
 if (vm->def->disks[i]->bus == disk->bus &&
 STREQ(vm->def->disks[i]->dst, disk->dst)) {
@@ -3018,6 +3019,7 @@ libxlDomainChangeEjectableMedia(virDomainObj *vm, 
virDomainDiskDef *disk)
 ret = 0;
 
  cleanup:
+libxl_device_disk_dispose(_disk);
 virObjectUnref(cfg);
 return ret;
 }
@@ -3030,6 +3032,7 @@ libxlDomainAttachDeviceDiskLive(virDomainObj *vm, 
virDomainDeviceDef *dev)
 libxl_device_disk x_disk;
 int ret = -1;
 
+libxl_device_disk_init(_disk);
 switch (l_disk->device)  {
 case VIR_DOMAIN_DISK_DEVICE_CDROM:
 ret = libxlDomainChangeEjectableMedia(vm, l_disk);
@@ -3091,6 +3094,7 @@ libxlDomainAttachDeviceDiskLive(virDomainObj *vm, 
virDomainDeviceDef *dev)
 }
 
  cleanup:
+libxl_device_disk_dispose(_disk);
 virObjectUnref(cfg);
 return ret;
 }
@@ -3329,6 +,7 @@ libxlDomainDetachDeviceDiskLive(virDomainObj *vm, 
virDomainDeviceDef *dev)
 int idx;
 int ret = -1;
 
+libxl_device_disk_init(_disk);
 switch (dev->data.disk->device)  {
 case VIR_DOMAIN_DISK_DEVICE_DISK:
 if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_XEN) {
@@ -3380,6 +3385,7 @@ libxlDomainDetachDeviceDiskLive(virDomainObj *vm, 
virDomainDeviceDef *dev)
 }
 
  cleanup:
+libxl_device_disk_dispose(_disk);
 virObjectUnref(cfg);
 return ret;
 }

Re: [PATCH libvirt v2] tests: Add capabilities for QEMU 6.0.0 on s390x

[Andrea Bolognani]

On Tue, May 18, 2021 at 03:53:09PM +0200, Shalini Chellathurai Saroja wrote:
> Introduce replies and xml files for QEMU 6.0.0 on s390x.
>
> Signed-off-by: Shalini Chellathurai Saroja 
> ---
> The replies file is removed from this patch and is available in
> https://gitlab.com/shalinichellathurai/libvirt/-/commit/3fb78dee79a668cae084d4a6d6ed2aa096e25004
>
>  tests/domaincapsdata/qemu_6.0.0.s390x.xml |   238 +
>  .../caps_6.0.0.s390x.replies  | 27223 
>  .../qemucapabilitiesdata/caps_6.0.0.s390x.xml |  3305 ++
>  ...default-video-type-s390x.s390x-latest.args | 6 +-
>  .../disk-error-policy-s390x.s390x-latest.args | 4 +-
>  .../fs9p-ccw.s390x-latest.args| 4 +-
>  ...tdev-subsys-mdev-vfio-ap.s390x-latest.args | 4 +-
>  ...ubsys-mdev-vfio-ccw-boot.s390x-latest.args | 4 +-
>  ...othreads-virtio-scsi-ccw.s390x-latest.args | 8 +-
>  ...t-cpu-kvm-ccw-virtio-2.7.s390x-latest.args | 4 +-
>  ...t-cpu-kvm-ccw-virtio-4.2.s390x-latest.args | 6 +-
>  ...t-cpu-tcg-ccw-virtio-2.7.s390x-latest.args | 4 +-
>  ...t-cpu-tcg-ccw-virtio-4.2.s390x-latest.args | 4 +-
>  .../s390x-ccw-graphics.s390x-latest.args  | 6 +-
>  .../s390x-ccw-headless.s390x-latest.args  | 6 +-
>  .../vhost-vsock-ccw-auto.s390x-latest.args| 4 +-
>  .../vhost-vsock-ccw-iommu.s390x-latest.args   | 4 +-
>  .../vhost-vsock-ccw.s390x-latest.args | 4 +-
>  18 files changed, 30802 insertions(+), 36 deletions(-)
>  create mode 100644 tests/domaincapsdata/qemu_6.0.0.s390x.xml
>  create mode 100644 tests/qemucapabilitiesdata/caps_6.0.0.s390x.replies
>  create mode 100644 tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml

Looks great now, thanks!

After rebasing on top of master, domaincapstest was failing so I ran

  $ VIR_TEST_REGENERATE_OUTPUT=1 meson test

and squashed in the resulting diff, which you can see below.

With that taken care of,

  Reviewed-by: Andrea Bolognani 

and pushed.


diff --git a/tests/domaincapsdata/qemu_6.0.0.s390x.xml
b/tests/domaincapsdata/qemu_6.0.0.s390x.xml
index 00b8f2c9c5..d384f0859b 100644
--- a/tests/domaincapsdata/qemu_6.0.0.s390x.xml
+++ b/tests/domaincapsdata/qemu_6.0.0.s390x.xml
@@ -226,6 +226,13 @@
 builtin
   
 
+
+  
+path
+handle
+virtiofs
+  
+
   
   
 
-- 
Andrea Bolognani / Red Hat / Virtualization

[PATCH v1] libxl: remove libxl_domain_config_init from libxlBuildDomainConfig

[Olaf Hering]

The passed libxl_domain_config is owned, and already initialized, by the
caller.

Signed-off-by: Olaf Hering 
---
 src/libxl/libxl_conf.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 8ea9b35292..3149ee3b4a 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -2529,7 +2529,6 @@ libxlBuildDomainConfig(virPortAllocatorRange 
*graphicsports,
 {
 virCaps *caps = cfg->caps;
 libxl_ctx *ctx = cfg->ctx;
-libxl_domain_config_init(d_config);
 
 if (libxlMakeDomCreateInfo(ctx, def, _config->c_info) < 0)
 return -1;

Re: [PATCH] virsh: Fix logic wrt to --current flag in cmdSetmem

[Peter Krempa]

On Tue, May 18, 2021 at 15:55:45 +0200, Pavel Hrdina wrote:
> On Tue, May 18, 2021 at 03:17:56PM +0200, Michal Prívozník wrote:
> > On 5/18/21 3:12 PM, Pavel Hrdina wrote:
> > > On Tue, May 18, 2021 at 03:02:55PM +0200, Ján Tomko wrote:
> > >> On a Tuesday in 2021, Michal Privoznik wrote:
> > >>> In my commit of v7.1.0-rc1~376 I've simplified the logic of
> > >>> handling @flags. My assumption back then was that calling
> > >>> virDomainSetMemory() is equivalent to
> > >>> virDomainSetMemoryFlags(flags = 0). But that is not the case,
> > >>> because it is equivalent to virDomainSetMemoryFlags(flags =
> > >>> VIR_DOMAIN_AFFECT_LIVE). Fix the condition that calls the old
> > >>> API.
> > >>>
> > >>> Fixes: b5e267e8c59a257652f88d034cb1e0ce1ed4b58a
> > >>
> > >> before that commit, if the user did not specify any of:
> > >>   config, live, current
> > >> we used the old API.
> > >>
> > >> After this change, the new API will be used for those cases.
> > > 
> > > The question is if we support using upstream virsh with old libvirt
> > > where only non-flags API is available. If not we should drop the
> > > non-flags API usage from virsh completely.
> > 
> > I think in general we do. In this specific case,
> > virDomainSetMemoryFlags() API was introduced in v0.9.0 release (which is
> > 10 years ago). And since we dropped RHEL-7 support recently and the
> > oldest QEMU we support is from late 2017 our attempt to be that
> > backwards compatible is questionable IMO.
> 
> Exactly my question. We have some support matrix where libvirt should be
> able to compile and run but there is no explicit support policy for
> using virsh with old libvirt or for obsolete APIs that should not be
> used. I would vote to use the same matrix as we already have.
> 
> That would allow us to remove probably all non-flags APIs from virsh
> code.

This fix, well a variation of it removing the old API would be the only
acceptable variant here.

The original code in virsh prior to the change would call the old API
also when no flag was used.

This patch is not really fixing the old behaviour though, because with
no flag you get the behavior of '--current' rather than the pre-existing
behavior of implying '--live'.

Thus we either declare that we do want to change the behaviour in virsh
and use it as an excuse to delete the use of the old api,

or

this patch needs to be fixed more to restore the logic,

or

just revert the original commit.

But this patch as-is is wrong and is not really fixing the behaviour
without adding more regression.

Re: [PATCH] conf: Report alias name of the detached device in error

[Michal Prívozník]

On 5/18/21 6:07 PM, Laine Stump wrote:
> On 5/18/21 5:44 AM, Kristina Hanicova wrote:
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1942367
>>
>> Signed-off-by: Kristina Hanicova 
>> ---
>>   src/conf/domain_conf.c | 19 +--
>>   1 file changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 7044701fac..e21b9fb946 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -15781,38 +15781,45 @@ virDomainNetFindIdx(virDomainDef *def,
>> virDomainNetDef *net)
>>   if (matchidx < 0) {
>>   if (MACAddrSpecified && PCIAddrSpecified) {
>>   virReportError(VIR_ERR_DEVICE_MISSING,
>> -   _("no device matching MAC address %s found
>> on "
>> +   _("no device matching MAC address %s and
>> alias %s found on "
>>    VIR_PCI_DEVICE_ADDRESS_FMT),
>>  virMacAddrFormat(>mac, mac),
>> +   NULLSTR(net->info.alias),
>>  net->info.addr.pci.domain,
>>  net->info.addr.pci.bus,
>>  net->info.addr.pci.slot,
>>  net->info.addr.pci.function);
>>   } else if (MACAddrSpecified && CCWAddrSpecified) {
>>   virReportError(VIR_ERR_DEVICE_MISSING,
>> -   _("no device matching MAC address %s found
>> on "
>> +   _("no device matching MAC address %s and
>> alias %s  found on "
> 
> These messages will look strange in the (most common) case where alias
> isn't specified, e.g.:
> 
>     no device matching MAC address DE:AD:BE:EF:01:10
>     and alias   found on [some CCW address]
> 
> On the other hand, the idea of even further exploding this bunch of
> conditionals to include all combinations is just horrible to think about!
> 
> What about instead reworking this to use a single virReportError() that
> references a few pointers setup beforehand and then substituting (a
> properly i8n'ized!) "(unspecified)" for each item that hasn't been
> specified, e.g.:
> 
> g_autofree *addr = g_strdup(_("(unspecified)"));
> const char *mac = _("(unspecified)");
> const char *alias = _("(unspecified)");
> 
> if (MACAddrSpecified)
>     mac = virMacAddrFormat(>mac, mac);
> if (net->info.alias)
>     alias = net->info.alias
> 
> if (CCWAddrSpecified)
>    addr = virCCWAddressAsString(blah);
> else if (PCIAddrSpecified)
>    addr = virPCIDeviceAddressAsString(blah);
> 
> virReportError(blah...
>    _("no device found at address '%s' matching MAC address
> '%s' and alias '%s'"),
>    addr, mac, alias);
> 
> or something like that. It's still not ideal, but avoids the conditional
> explosion and I think is less confusing than having "alias" followed by
> nothing.

IIUC, NULLSTR() will expand to "" not an empty string.
"unspecified" sounds better. What I worry about is translations: in my
native language and it's not a problem to have the error message split
as you suggest. But maybe there are some languages where it might be
problem?

On the other hand - we can go with your suggestion and change this later
as soon as we learn it's problematic for translators.

Kristina, what's your opinion?

Michal