Re: [libvirt] [PATCH v2 4/8] qemu: vfio-ccw device address generation

[Boris Fiuczynski]

On 05/14/2018 06:30 PM, John Ferlan wrote:



On 05/14/2018 03:43 AM, Boris Fiuczynski wrote:

On 05/10/2018 10:52 PM, John Ferlan wrote:



On 05/07/2018 10:41 AM, Boris Fiuczynski wrote:

From: Shalini Chellathurai Saroja 

Introduces the vfio-ccw model for mediated devices and prime vfio-ccw
devices such that CCW address will be generated.

Signed-off-by: Shalini Chellathurai Saroja 
Reviewed-by: Bjoern Walk 
Reviewed-by: Boris Fiuczynski 
Reviewed-by: Marc Hartmayer 
Reviewed-by: Stefan Zimmermann 
---
   docs/schemas/domaincommon.rng  |  5 -
   src/qemu/qemu_domain_address.c | 20 
   src/util/virmdev.c |  3 ++-
   src/util/virmdev.h |  1 +
   4 files changed, 27 insertions(+), 2 deletions(-)



Looking at all places using VIR_MDEV_MODEL_TYPE_VFIO_PCI - should this
patch make a change to virDomainHostdevDefPostParse to do something
similar - that is:

     if (model == VIR_MDEV_MODEL_TYPE_VFIO_CCW &&
  dev->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
... error message
    }

?

Let me know... I think it should and can add it before pushing...

You are correct. Good catch!
How about fixing it like this?
     if ((model == VIR_MDEV_MODEL_TYPE_VFIO_PCI &&
     dev->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) ||
     (model == VIR_MDEV_MODEL_TYPE_VFIO_CCW &&
     dev->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW)) {
     virReportError(VIR_ERR_XML_ERROR,
    _("Unsupported address type '%s' with mediated "
  "device model '%s'"),

virDomainDeviceAddressTypeToString(dev->info->type),
    virMediatedDeviceModelTypeToString(model));
     return -1;
     }



OK - added that...




Besides that I just saw that the indention of the second parameter of
method qemuDomainPrimeVfioDeviceAddresses is off by three blanks.



ah true - adjusted that.

I've merged with the latest top of tree and have pushed... So much flux
in the capabilities lately...

John

You are correct that lots of changes happened in the capabilities.
Thanks for merging the changes and pushing the series!


--
Mit freundlichen Grüßen/Kind regards
   Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

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

Re: [libvirt] [PATCH v2] xenconfig: xm: Fix checking for extra in parser

[Jim Fehlig]

On 05/12/2018 09:45 AM, Filip Alac wrote:

Parser assumed extra was always present when root was specified.
Fixed by handling root and extra separately.

Signed-off-by: Filip Alac 


Reviewed-by: Jim Fehlig 

and pushed. Thanks!

Regards,
Jim


---
  src/xenconfig/xen_xm.c | 7 +--
  1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c
index 8ef68bbc..4becb40b 100644
--- a/src/xenconfig/xen_xm.c
+++ b/src/xenconfig/xen_xm.c
@@ -91,10 +91,13 @@ xenParseXMOS(virConfPtr conf, virDomainDefPtr def)
  if (xenConfigGetString(conf, "root", , NULL) < 0)
  return -1;
  
-if (root) {

+if (root && extra) {
  if (virAsprintf(>os.cmdline, "root=%s %s", root, extra) < 0)
  return -1;
-} else {
+} else if (root) {
+if (virAsprintf(>os.cmdline, "root=%s", root) < 0)
+return -1;
+} else if (extra) {
  if (VIR_STRDUP(def->os.cmdline, extra) < 0)
  return -1;
  }



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

[libvirt] [PATCH] libxl: don't set hasManagedSave when performing save

[Jim Fehlig]

libxlDoDomainSave() is used in both the save and managedsave code
paths but was unconditionally setting hasManagedSave to true on
success. As a result, undefine would fail after a non-managed
save/restore operation. E.g.

virsh define; virsh start
virsh save; virsh restore
virsh shutdown
virsh undefine
error: Refusing to undefine while domain managed save image exists

Modify libxlDoDomainSave() to take an additional parameter to
specify managed vs non-managed save, and change callers to use it.

Signed-off-by: Jim Fehlig 
---
 src/libxl/libxl_driver.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 34adef8d48..df53dead0a 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -1653,8 +1653,10 @@ libxlDomainGetState(virDomainPtr dom,
  * virDomainObjPtr must be locked on invocation
  */
 static int
-libxlDoDomainSave(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
-  const char *to)
+libxlDoDomainSave(libxlDriverPrivatePtr driver,
+  virDomainObjPtr vm,
+  const char *to,
+  bool managed)
 {
 libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
 libxlSavefileHeader hdr;
@@ -1725,7 +1727,7 @@ libxlDoDomainSave(libxlDriverPrivatePtr driver, 
virDomainObjPtr vm,
 }
 
 libxlDomainCleanup(driver, vm);
-vm->hasManagedSave = true;
+vm->hasManagedSave = managed;
 ret = 0;
 
  cleanup:
@@ -1772,7 +1774,7 @@ libxlDomainSaveFlags(virDomainPtr dom, const char *to, 
const char *dxml,
 if (virDomainObjCheckActive(vm) < 0)
 goto endjob;
 
-if (libxlDoDomainSave(driver, vm, to) < 0)
+if (libxlDoDomainSave(driver, vm, to, false) < 0)
 goto endjob;
 
 if (!vm->persistent)
@@ -1989,7 +1991,7 @@ libxlDomainManagedSave(virDomainPtr dom, unsigned int 
flags)
 
 VIR_INFO("Saving state to %s", name);
 
-if (libxlDoDomainSave(driver, vm, name) < 0)
+if (libxlDoDomainSave(driver, vm, name, true) < 0)
 goto endjob;
 
 if (!vm->persistent)
-- 
2.16.3

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

Re: [libvirt] [PATCH 12/13] util: storage: Store PR manager alias in the definition

[John Ferlan]

On 05/14/2018 11:19 AM, Michal Privoznik wrote:
> On 05/14/2018 12:45 PM, Peter Krempa wrote:
>> Rather than always re-generating the alias store it in the definition
>> and in the status XML.
>>
>> Signed-off-by: Peter Krempa 
>> ---
>>  src/qemu/qemu_command.c   | 23 +++--
>>  src/qemu/qemu_command.h   |  3 +--
>>  src/qemu/qemu_domain.c| 16 +--
>>  src/qemu/qemu_hotplug.c   | 34 
>> ++-
>>  src/util/virstoragefile.c |  1 +
>>  src/util/virstoragefile.h |  3 +++
>>  tests/qemustatusxml2xmldata/modern-in.xml |  4 
>>  7 files changed, 37 insertions(+), 47 deletions(-)
> 
> Yes, this makes sense to me. I've kept alias in status XML for all the
> versions until the very last one. You have my ACK, but since John was
> opposed maybe we should ask for his opinion too (so that we don't go
> behind his back).
> 

I still see no purpose for an alias to be saved since it's static, but I
seem to be alone in that thinking. Just as much as I was alone in the
thinking that qemuDomainGetManagedPRAlias is unnecessary and that a
constant static string would work just the same. I'm also not convinced
that a non managed system should be supported, but I recall Michal
noting something during one of the reviews that it was useful for some
future work.

Shouldn't at the very least the formatting of the path and alias only
occur for !(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)... Would mean
passing @flags to virStoragePRDefFormat.  At the very least it'll make
it obvious about it's only being formatted for the active/status guest.


John

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

Re: [libvirt] [PATCH 10/13] qemu: command: Move check whether PR manager object props need to be built

[John Ferlan]

On 05/14/2018 06:45 AM, Peter Krempa wrote:
> Move it out of the formatter function and let the caller decide this.

s/formatter/format/

I cannot recall our current consistency...  Some times we seem to have
the lowest routine make the singular check and other times we have the
caller make the check.

> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_command.c | 9 +++--
>  src/qemu/qemu_hotplug.c | 3 +++
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 7df9979cb2..c38dde5a60 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9687,9 +9687,6 @@ qemuBuildPRManagerInfoProps(const virDomainDiskDef 
> *disk,
>  *propsret = NULL;
>  *aliasret = NULL;
> 
> -if (!disk->src->pr)
> -return 0;
> -
>  if (virStoragePRDefIsManaged(disk->src->pr)) {
>  if (VIR_STRDUP(alias, qemuDomainGetManagedPRAlias()) < 0)
>  goto cleanup;
> @@ -9725,6 +9722,9 @@ qemuBuildMasterPRCommandLine(virCommandPtr cmd,
>  for (i = 0; i < def->ndisks; i++) {
>  const virDomainDiskDef *disk = def->disks[i];
> 
> +if (!disk->src->pr)
> +continue;
> +
>  if (virStoragePRDefIsManaged(disk->src->pr)) {
>  if (managedAdded)
>  continue;
> @@ -9735,9 +9735,6 @@ qemuBuildMasterPRCommandLine(virCommandPtr cmd,
>  if (qemuBuildPRManagerInfoProps(disk, , ) < 0)
>  goto cleanup;
> 
> -if (!props)
> -continue;
> -

This one seems unrelated... although at this point I'm not sure how it
could happen...  I'd say keep it as is unless Michal had some other
reason or maybe remembers what it was there for originally.


John

>  if (!(tmp = 
> virQEMUBuildObjectCommandlineFromJSON("pr-manager-helper",
>alias,
>props)))
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 3a26876c10..6557711ec1 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -395,6 +395,9 @@ qemuMaybeBuildPRManagerInfoProps(virDomainObjPtr vm,
>  *propsret = NULL;
>  *aliasret = NULL;
> 
> +if (!disk->src->pr)
> +return 0;
> +
>  if (virStoragePRDefIsManaged(disk->src->pr) &&
>  priv->prDaemonRunning) {
>  /* @disk requires qemu-pr-helper but there's already one running. */
> 

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

Re: [libvirt] [PATCH 09/13] qemu: process: Change semantics of functions starting PR daemon

[John Ferlan]

On 05/14/2018 06:45 AM, Peter Krempa wrote:
> Libvirt only manages one PR daemon. This means that we don't need to
> pass the 'disk' object and also rename the functions dealing with this
> so that it's obvious we only deal with the managed PR daemon.
> 
> Signed-off-by: Peter Krempa 
 

Something strange happened here.


> ---
>  src/qemu/qemu_hotplug.c |  6 +++---
>  src/qemu/qemu_process.c | 37 -
>  src/qemu/qemu_process.h |  5 ++---
>  3 files changed, 21 insertions(+), 27 deletions(-)
> 

[...]

>  int
> -qemuProcessStartPRDaemon(virDomainObjPtr vm,
> - const virDomainDiskDef *disk)
> +qemuProcessStartManagedPRDaemon(virDomainObjPtr vm)
>  {
>  qemuDomainObjPrivatePtr priv = vm->privateData;
>  virQEMUDriverPtr driver = priv->driver;
> @@ -2640,10 +2639,6 @@ qemuProcessStartPRDaemon(virDomainObjPtr vm,
>  const unsigned long long timeout = 50; /* ms */
>  int ret = -1;
> 
> -if (!virStoragePRDefIsManaged(disk->src->pr) ||
> -priv->prDaemonRunning)
> -return 0;
> -
>  cfg = virQEMUDriverGetConfig(driver);
> 
>  if (!virFileIsExecutable(cfg->prHelperName)) {
> @@ -2734,7 +2729,7 @@ qemuProcessStartPRDaemon(virDomainObjPtr vm,
>  goto cleanup;
> 
>  priv->prDaemonRunning = true;
> -ret = 1;
> +ret = 0;

Unrelated, but since callers only care about < 0, no big deal...   I'm
assuming this is a holder from some other path which used the function
for a return value (e.g. like qemuDomainMaybeStartPRDaemon) at some
point during development.


No big deal - I don't expect this to be extracted, but was just paying
attention ;-)

John


>   cleanup:
>  if (ret < 0) {
>  virCommandAbort(cmd);
> @@ -2754,22 +2749,22 @@ qemuProcessStartPRDaemon(virDomainObjPtr vm,
> 
> 

[...]

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

Re: [libvirt] [PATCH 08/13] qemu: Assign managed PR path when preparing storage source

[John Ferlan]

On 05/14/2018 06:41 AM, Peter Krempa wrote:
> Rather than always checking which path to use pre-assign it when
> preparing storage source.
> 
> This reduces the need to pass 'vm' around too much. For later use the
> path can be retrieved from the status XML.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_command.c | 18 +-
>  src/qemu/qemu_command.h |  3 +--
>  src/qemu/qemu_domain.c  | 35 ++-
>  src/qemu/qemu_domain.h  |  3 +--
>  src/qemu/qemu_hotplug.c |  2 +-
>  src/qemu/qemu_process.c |  2 +-
>  6 files changed, 31 insertions(+), 32 deletions(-)
> 

Strange this patch got posted twice once w/ your own CC and once without.

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 2bdba7734a..7df9979cb2 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9667,7 +9667,6 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
> 
>  /**
>   * qemuBuildPRManagerInfoProps:
> - * @vm: domain object
>   * @disk: disk definition
>   * @propsret: Returns JSON object containing properties of the 
> pr-manager-helper object
>   * @aliasret: alias of the pr-manager-helper object
> @@ -9678,12 +9677,10 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
>   * -1 on failure (with error message set).
>   */
>  int
> -qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
> -const virDomainDiskDef *disk,
> +qemuBuildPRManagerInfoProps(const virDomainDiskDef *disk,
>  virJSONValuePtr *propsret,
>  char **aliasret)
>  {
> -char *socketPath = NULL;
>  char *alias = NULL;
>  int ret = -1;
> 
> @@ -9693,9 +9690,6 @@ qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
>  if (!disk->src->pr)
>  return 0;
> 
> -if (!(socketPath = qemuDomainGetPRSocketPath(vm, disk->src->pr)))
> -return ret;
> -
>  if (virStoragePRDefIsManaged(disk->src->pr)) {
>  if (VIR_STRDUP(alias, qemuDomainGetManagedPRAlias()) < 0)
>  goto cleanup;
> @@ -9705,7 +9699,7 @@ qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
>  }
> 
>  if (virJSONValueObjectCreate(propsret,
> - "s:path", socketPath,
> + "s:path", disk->src->pr->path,
>   NULL) < 0)
>  goto cleanup;
> 
> @@ -9713,14 +9707,12 @@ qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
>  ret = 0;
>   cleanup:
>  VIR_FREE(alias);
> -VIR_FREE(socketPath);
>  return ret;
>  }
> 
> 
>  static int
> -qemuBuildMasterPRCommandLine(virDomainObjPtr vm,
> - virCommandPtr cmd,
> +qemuBuildMasterPRCommandLine(virCommandPtr cmd,
>   const virDomainDef *def)
>  {
>  size_t i;
> @@ -9740,7 +9732,7 @@ qemuBuildMasterPRCommandLine(virDomainObjPtr vm,
>  managedAdded = true;
>  }
> 
> -if (qemuBuildPRManagerInfoProps(vm, disk, , ) < 0)
> +if (qemuBuildPRManagerInfoProps(disk, , ) < 0)
>  goto cleanup;
> 
>  if (!props)
> @@ -9934,7 +9926,7 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
>  if (qemuBuildMasterKeyCommandLine(cmd, priv) < 0)
>  goto error;
> 
> -if (qemuBuildMasterPRCommandLine(vm, cmd, def) < 0)
> +if (qemuBuildMasterPRCommandLine(cmd, def) < 0)

Rather than @vm - what was really desired is/was @priv, which is already
passed for qemuBuildMasterKeyCommandLine and could be for this too...
Thus not requiring the hunks that change path in disk->src->pr->path.
It is a static path beyond the priv->libDir part.

>  goto error;
> 
>  if (enableFips)
> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index da1fe679fe..621592cd79 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -55,8 +55,7 @@ virCommandPtr qemuBuildCommandLine(virQEMUDriverPtr driver,
> int **nicindexes);
> 
>  /* Generate the object properties for pr-manager */
> -int qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
> -const virDomainDiskDef *disk,
> +int qemuBuildPRManagerInfoProps(const virDomainDiskDef *disk,
>  virJSONValuePtr *propsret,
>  char **alias);
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index eaa796260c..92217e66fe 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -11930,6 +11930,22 @@ qemuDomainPrepareDiskCachemode(virDomainDiskDefPtr 
> disk)
>  }
> 
> 
> +static int
> +qemuDomainPrepareStorageSourcePR(virStorageSourcePtr src,
> + qemuDomainObjPrivatePtr priv)
> +{
> +if (!src->pr)
> +return 0;
> +
> +if (virStoragePRDefIsManaged(src->pr)) {
> +if (!(src->pr->path = qemuDomainGetManagedPRSocketPath(priv)))
> +return -1;
> +}

Re: [libvirt] [PATCH 05/13] util: storage: Drop pointless 'enabled' form PR definition

[John Ferlan]

On 05/14/2018 06:41 AM, Peter Krempa wrote:
> Everything can be disabled by not using the parent element. There's no
> need to store this explicitly. Additionally it does not add any value
> since any configuration is dropped if enabled='no' is configured.
> 
> Drop the attribute and adjust the code accordingly.t
> 
> Signed-off-by: Peter Krempa 
> ---
>  docs/formatdomain.html.in  |  21 ++--
>  docs/schemas/storagecommon.rng |   3 -
>  src/util/virstoragefile.c  | 117 
> +
>  src/util/virstoragefile.h  |   1 -
>  .../disk-virtio-scsi-reservations.xml  |   4 +-
>  5 files changed, 59 insertions(+), 87 deletions(-)
> 

As I've worked may way forward - I reread the docs and needed to return
here for commenting...


> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 80172c18d0..d69a669259 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -2583,7 +2583,7 @@
>disk type='block' device='lun'
>  driver name='qemu' type='raw'/
>  source dev='/dev/sda'
> -  reservations enabled='yes' managed='no'
> +  reservations managed='no'
>  source type='unix' path='/path/to/qemu-pr-helper' 
> mode='client'/
>/reservations
>  target dev='sda' bus='scsi'/
> @@ -2952,17 +2952,16 @@
>Since libvirt 4.4.0, the
>  reservations can be a sub-element of the
>  source element for storage sources (QEMU driver 
> only).
> -If present (and enabled) it enables persistent reservations for 
> SCSI
> +If present it enables persistent reservations for SCSI
>  based disks. The element has one mandatory attribute
> -enabled with accepted values yes and
> -no. If the feature is enabled, then there's another
> -mandatory attribute managed (accepted values are the
> -same as for enabled) that enables or disables 
> libvirt
> -spawning a helper process. When the PR is unmanaged, then 
> hypervisor
> -acts as a client and path to server socket must be provided in 
> child
> -element source, which currently accepts only the
> -following attributes: type with one value
> -unix, path with path the socket, and
> +managed with accepted values yes and
> +no. If managed is enabled libvirt 
> prepares
> +and manages any resources needed for the feature. When the PR is

s/for the feature././

s/PR is/persistent reservations are/

> +unmanaged, then hypervisor acts as a client and path to server

s/then/then the/

s/and path to server/and the path to the server/

> +socket must be provided in child element source,

s/in child/in the child/

> +which currently accepts only the following attributes:
> +type with one value unix,
> +path with path the socket, and

s/with path the socket/path to the socket/

>  finally mode which accepts one value
>  client and specifies the role of hypervisor.

s/and specifies/specifying

>  It's recommended to allow libvirt manage the persistent

John

[...]

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

Re: [libvirt] [PATCH 05/13] util: storage: Drop pointless 'enabled' form PR definition

[John Ferlan]

On 05/14/2018 06:41 AM, Peter Krempa wrote:
> Everything can be disabled by not using the parent element. There's no
> need to store this explicitly. Additionally it does not add any value
> since any configuration is dropped if enabled='no' is configured.
> 
> Drop the attribute and adjust the code accordingly.t
> 
> Signed-off-by: Peter Krempa 
> ---
>  docs/formatdomain.html.in  |  21 ++--
>  docs/schemas/storagecommon.rng |   3 -
>  src/util/virstoragefile.c  | 117 
> +
>  src/util/virstoragefile.h  |   1 -
>  .../disk-virtio-scsi-reservations.xml  |   4 +-
>  5 files changed, 59 insertions(+), 87 deletions(-)
> 

Hmm... I recall stating the same thing during v1 and v2 review, but got
a deafening the design of XML was agreed upon already...

John

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

Re: [libvirt] [PATCH 0/3] bhyve: allow locking memory

[Fabian Freyer]

On 14 May 2018, at 8:51, Roman Bogorodskiy wrote:

>   Peter Krempa wrote:
>
>> On Sun, May 13, 2018 at 14:01:25 +0400, Roman Bogorodskiy wrote:
>>>   Fabian Freyer wrote:
>>
>> [...]
>>
  13 files changed, 134 insertions(+)
  create mode 100644 tests/bhyveargv2xmldata/bhyveargv2xml-wired.args
  create mode 100644 tests/bhyveargv2xmldata/bhyveargv2xml-wired.xml
  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-wired.args
  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-wired.ldargs
  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-wired.xml
  create mode 100644 tests/bhyvexml2xmloutdata/bhyvexml2xmlout-wired.xml
>>>
>>> Thanks for the patches, I've pushed the series with some syntax-check
>>> fixes (trailing whitespaces, tabs instead of whitespaces) and added
>>> 'Signed-off-by' line which is mandatory now.
>>
>> Note that 'Signed-off-by' should _NOT_ be added without explicit consent
>> of the author of the patches. Otherwise it defeats the purpose of
>> certification that the author agrees and complies with the Developer
>> certificate of origin. Adding it without explicit consent makes the
>> Signed-off-by line just a useless piece of jewelery added to appease the
>> push hook.
>
> Oh, sorry about that. I assumed as Fabian is a long time contributor,
> he's familiar with the general project policies.
>
> Fabian, do you have any issue with that?
All good with me.

In case you need it from me, for the whole series:

Signed-off-by: Fabian Freyer 

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

Re: [libvirt] [PATCH v2 4/8] qemu: vfio-ccw device address generation

[John Ferlan]

On 05/14/2018 03:43 AM, Boris Fiuczynski wrote:
> On 05/10/2018 10:52 PM, John Ferlan wrote:
>>
>>
>> On 05/07/2018 10:41 AM, Boris Fiuczynski wrote:
>>> From: Shalini Chellathurai Saroja 
>>>
>>> Introduces the vfio-ccw model for mediated devices and prime vfio-ccw
>>> devices such that CCW address will be generated.
>>>
>>> Signed-off-by: Shalini Chellathurai Saroja 
>>> Reviewed-by: Bjoern Walk 
>>> Reviewed-by: Boris Fiuczynski 
>>> Reviewed-by: Marc Hartmayer 
>>> Reviewed-by: Stefan Zimmermann 
>>> ---
>>>   docs/schemas/domaincommon.rng  |  5 -
>>>   src/qemu/qemu_domain_address.c | 20 
>>>   src/util/virmdev.c |  3 ++-
>>>   src/util/virmdev.h |  1 +
>>>   4 files changed, 27 insertions(+), 2 deletions(-)
>>>
>>
>> Looking at all places using VIR_MDEV_MODEL_TYPE_VFIO_PCI - should this
>> patch make a change to virDomainHostdevDefPostParse to do something
>> similar - that is:
>>
>>     if (model == VIR_MDEV_MODEL_TYPE_VFIO_CCW &&
>>  dev->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
>> ... error message
>>    }
>>
>> ?
>>
>> Let me know... I think it should and can add it before pushing...
> You are correct. Good catch!
> How about fixing it like this?
>     if ((model == VIR_MDEV_MODEL_TYPE_VFIO_PCI &&
>     dev->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) ||
>     (model == VIR_MDEV_MODEL_TYPE_VFIO_CCW &&
>     dev->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW)) {
>     virReportError(VIR_ERR_XML_ERROR,
>    _("Unsupported address type '%s' with mediated "
>  "device model '%s'"),
> 
> virDomainDeviceAddressTypeToString(dev->info->type),
>    virMediatedDeviceModelTypeToString(model));
>     return -1;
>     }
> 

OK - added that...

> 
> 
> Besides that I just saw that the indention of the second parameter of
> method qemuDomainPrimeVfioDeviceAddresses is off by three blanks.
> 

ah true - adjusted that.

I've merged with the latest top of tree and have pushed... So much flux
in the capabilities lately...

John

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

Re: [libvirt] [PATCH 2/5] Introduce virCryptoHashBuf

[Stefan Berger]

On 05/11/2018 11:50 AM, Ján Tomko wrote:

A function that keeps the hash in binary form instead of converting
it to human-readable hexadecimal form.

Signed-off-by: Ján Tomko 
---
  src/util/vircrypto.c | 31 +--
  src/util/vircrypto.h |  7 +++
  2 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c
index 48b04fc8ce..1a2dcc28b7 100644
--- a/src/util/vircrypto.c
+++ b/src/util/vircrypto.c
@@ -54,28 +54,39 @@ struct virHashInfo {
  verify(ARRAY_CARDINALITY(hashinfo) == VIR_CRYPTO_HASH_LAST);
  
  int

-virCryptoHashString(virCryptoHash hash,
-const char *input,
-char **output)
+virCryptoHashBuf(virCryptoHash hash,
+ const char *input,
+ unsigned char *output)
  {
-unsigned char buf[VIR_CRYPTO_LARGEST_DIGEST_SIZE];
-size_t hashstrlen;
-size_t i;
-
  if (hash >= VIR_CRYPTO_HASH_LAST) {
  virReportError(VIR_ERR_INVALID_ARG,
 _("Unknown crypto hash %d"), hash);
  return -1;
  }
  
-hashstrlen = (hashinfo[hash].hashlen * 2) + 1;

-
-if (!(hashinfo[hash].func(input, strlen(input), buf))) {
+if (!(hashinfo[hash].func(input, strlen(input), output))) {
  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
 _("Unable to compute hash of data"));
  return -1;
  }
  
+return 0;

+}
+
+int
+virCryptoHashString(virCryptoHash hash,
+const char *input,
+char **output)
+{
+unsigned char buf[VIR_CRYPTO_LARGEST_DIGEST_SIZE];
+size_t hashstrlen;
+size_t i;
+
+if (virCryptoHashBuf(hash, input, buf) < 0)
+return -1;
+
+hashstrlen = (hashinfo[hash].hashlen * 2) + 1;
+


How about virCryptoHashBuf returning the number of raw bytes it produced 
so that not all callers need to look it up?



   Stefan



  if (VIR_ALLOC_N(*output, hashstrlen) < 0)
  return -1;
  
diff --git a/src/util/vircrypto.h b/src/util/vircrypto.h

index 81743d2f74..64984006be 100644
--- a/src/util/vircrypto.h
+++ b/src/util/vircrypto.h
@@ -41,6 +41,13 @@ typedef enum {
  VIR_CRYPTO_CIPHER_LAST
  } virCryptoCipher;
  
+int

+virCryptoHashBuf(virCryptoHash hash,
+ const char *input,
+ unsigned char *output)
+ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+ATTRIBUTE_RETURN_CHECK;
+
  int
  virCryptoHashString(virCryptoHash hash,
  const char *input,



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

Re: [libvirt] [PATCH 05/13] util: storage: Drop pointless 'enabled' form PR definition

[Michal Privoznik]

On 05/14/2018 12:41 PM, Peter Krempa wrote:
> Everything can be disabled by not using the parent element. There's no
> need to store this explicitly. Additionally it does not add any value
> since any configuration is dropped if enabled='no' is configured.
> 
> Drop the attribute and adjust the code accordingly.t

s/t$//

> 
> Signed-off-by: Peter Krempa 
> ---
>  docs/formatdomain.html.in  |  21 ++--
>  docs/schemas/storagecommon.rng |   3 -
>  src/util/virstoragefile.c  | 117 
> +
>  src/util/virstoragefile.h  |   1 -
>  .../disk-virtio-scsi-reservations.xml  |   4 +-
>  5 files changed, 59 insertions(+), 87 deletions(-)

Michal

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

Re: [libvirt] [PATCH 00/13] Fix XML for persistent reservations and refactor

[Michal Privoznik]

On 05/14/2018 12:41 PM, Peter Krempa wrote:
> The XML design for the PR stuff is slightly weird so fix it and refactor
> the code so that it will be much easier to use it with the blockdev
> infrastructure.
> 
> Peter Krempa (13):
>   qemu: hotplug: Fix spacing around addition operator
>   qemu: alias: Allow passing alias of parent when generating PR manager
> alias
>   qemu: command: Fix comment for qemuBuildPRManagerInfoProps
>   qemu: Move validation of PR manager support
>   util: storage: Drop pointless 'enabled' form PR definition
>   util: storage: Drop virStoragePRDefIsEnabled
>   util: storage: Allow passing  also for managed PR case
>   qemu: Assign managed PR path when preparing storage source
>   qemu: process: Change semantics of functions starting PR daemon
>   qemu: command: Move check whether PR manager object props need to be
> built
>   conf: domain: Add helper to check whether a domain def requires use of
> PR
>   util: storage: Store PR manager alias in the definition
>   qemu: hotplug: Replace qemuDomainDiskNeedRemovePR
> 
>  docs/formatdomain.html.in  |  21 ++--
>  docs/schemas/storagecommon.rng |   3 -
>  src/conf/domain_conf.c |  21 
>  src/conf/domain_conf.h |   3 +
>  src/libvirt_private.syms   |   2 +-
>  src/qemu/qemu_alias.c  |   4 +-
>  src/qemu/qemu_alias.h  |   2 +-
>  src/qemu/qemu_command.c|  61 +++---
>  src/qemu/qemu_command.h|   6 +-
>  src/qemu/qemu_domain.c |  66 ---
>  src/qemu/qemu_domain.h |   3 +-
>  src/qemu/qemu_hotplug.c|  83 +++---
>  src/qemu/qemu_process.c|  40 ++-
>  src/qemu/qemu_process.h|   5 +-
>  src/util/virstoragefile.c  | 124 
> -
>  src/util/virstoragefile.h  |   5 +-
>  tests/qemustatusxml2xmldata/modern-in.xml  |   4 +
>  .../disk-virtio-scsi-reservations.xml  |   4 +-
>  tests/qemuxml2xmltest.c|   2 +-
>  19 files changed, 191 insertions(+), 268 deletions(-)
> 

You have my ACK on the series, but I guess we'll need John's input on
12/13 because he was opposed to saving anything in status XML.

Michal

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

Re: [libvirt] [PATCH 12/13] util: storage: Store PR manager alias in the definition

[Michal Privoznik]

On 05/14/2018 12:45 PM, Peter Krempa wrote:
> Rather than always re-generating the alias store it in the definition
> and in the status XML.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_command.c   | 23 +++--
>  src/qemu/qemu_command.h   |  3 +--
>  src/qemu/qemu_domain.c| 16 +--
>  src/qemu/qemu_hotplug.c   | 34 
> ++-
>  src/util/virstoragefile.c |  1 +
>  src/util/virstoragefile.h |  3 +++
>  tests/qemustatusxml2xmldata/modern-in.xml |  4 
>  7 files changed, 37 insertions(+), 47 deletions(-)

Yes, this makes sense to me. I've kept alias in status XML for all the
versions until the very last one. You have my ACK, but since John was
opposed maybe we should ask for his opinion too (so that we don't go
behind his back).

Michal

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

[libvirt] ANNOUNCE: Plans for the next release

[Pavel Hrdina]

Hi,

I would like to make the next libvirt-dbus release with stable APIs,
there is a lot of them already implemented and other projects can start
adapting to use libvirt-dbus.

The release version would be 1.0.0 and currently libvirt-dbus will
cover APIs up to libvirt-3.0.0 as a start point because that libvirt
version is available in all downstream distributions currently
supported by libvirt.

So I would like to ask everyone to look at the APIs to check whether it
make sense and whether something is missing or could be excluded.

The idea of libvirt-dbus is not to export every API if there is a better
API to do the job, so for example:

- all APIs that have both versions with and without flags we will
  export only the one with flags.

- some of the Stats APIs can be omitted since the same values are
  also exported in the GetStats (virDomainListGetStats)

I would like to make the release this week so any help or suggestion is
welcome.  The API definition is in the data/org.libvirt.*.xml files.

Thanks and regards
Pavel


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

Re: [libvirt] [PATCH 03/12] Parse domain XML to generate virDomainLoaderDef & virDomainNvramDef.

[Peter Krempa]

On Mon, May 14, 2018 at 16:20:53 +0200, Marc Hartmayer wrote:
> On Mon, May 14, 2018 at 01:15 PM +0200, Prerna Saxena  
> wrote:
> > This patch is used to interpret domain XML and store the Loader &
> > Nvram's backing definitions as virStorageSource.
> > It also identifies if input XML used old or new-style declaration.
> > (This will later be used by the formatter).
> >
> > Signed-off-by: Prerna Saxena 
> > ---
> >  src/conf/domain_conf.c | 131 
> > ++---
> >  1 file changed, 124 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index f678e26..be43695 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -2884,8 +2884,8 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr loader)
> >  if (!loader)
> >  return;
> >
> > -VIR_FREE(loader->path);
> > -VIR_FREE(loader->nvram);
> > +virStorageSourceFree(loader->src);
> > +virStorageSourceFree(loader->nvram);
> >  VIR_FREE(loader->templt);
> >  VIR_FREE(loader);
> >  }
> > @@ -17986,17 +17986,62 @@ 
> > virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
> >
> >  static int
> >  virDomainLoaderDefParseXML(xmlNodePtr node,
> > +   xmlXPathContextPtr ctxt,
> > virDomainLoaderDefPtr loader)
> >  {
> >  int ret = -1;
> >  char *readonly_str = NULL;
> >  char *secure_str = NULL;
> >  char *type_str = NULL;
> > +char *tmp = NULL;
> > +xmlNodePtr cur;
> > +xmlXPathContextPtr cur_ctxt = ctxt;
> > +
> > +if (VIR_ALLOC(loader->src)) {
>  ^^
>  Usually it is checked for < 0.
> 
> > +goto cleanup;
> > +}
> > +loader->src->type = VIR_STORAGE_TYPE_LAST;
> > +loader->oldStyleLoader = false;
> >
> >  readonly_str = virXMLPropString(node, "readonly");
> >  secure_str = virXMLPropString(node, "secure");
> >  type_str = virXMLPropString(node, "type");
> > -loader->path = (char *) xmlNodeGetContent(node);
> > +
> > +if ((tmp = virXMLPropString(node, "backing")) &&
> > +(loader->src->type = virStorageTypeFromString(tmp)) <= 0) {
> 
> If virStorageTypeFromString fails there is a memory leak of @tmp.
> 
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +   _("unknown loader type '%s'"), tmp);
> > +goto cleanup;
> > +}
> > +VIR_FREE(tmp);
> > +
> > +for (cur = node->children; cur != NULL; cur = cur->next) {
> > +if (cur->type != XML_ELEMENT_NODE) {
> > +continue;
> > +}
> > +
> > +if (virXMLNodeNameEqual(cur, "source")) {
> > +/* new-style declaration found */
> > +if (virDomainStorageSourceParse(cur, cur_ctxt, loader->src, 0) 
> > < 0) {
> > +virReportError(VIR_ERR_XML_DETAIL,
> > +   _("Error parsing Loader source element"));
> > +goto cleanup;
> > +}
> > +break;
> > +}
> > +}
> > +
> > +/* Old-style absolute path found ? */
> > +if (loader->src->type == VIR_STORAGE_TYPE_LAST) {
> > +if (!(loader->src->path = (char *) xmlNodeGetContent(node))) {
> > +virReportError(VIR_ERR_XML_ERROR, "%s",
> > +   _("missing loader source"));
> > +goto cleanup;
> > +} else {
> > +loader->src->type = VIR_STORAGE_TYPE_FILE;
> > +loader->oldStyleLoader = true;
> > +}
> > +}
> >
> >  if (readonly_str &&
> >  (loader->readonly = virTristateBoolTypeFromString(readonly_str)) 
> > <= 0) {
> > @@ -18023,13 +18068,81 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
> >  }
> >
> >  ret = 0;
> > - cleanup:
> > +goto exit;
> > +cleanup:
> 
> I would replace: s/cleanup/error and s/exit/cleanup.
> 
> > +if (loader->src)
> > +  VIR_FREE(loader->src);
> 
> Shouldn’t it be a virStorageSourceFree() here? Not sure if it’s needed
> but it’s safer.

Additionally the preferred way is to use a second virStorageSourcePtr to
hold the data while it's being parsed and then use VIR_STEAL_PTR to put
it into the structure.

The cleanup section then can call virStorageSourceFree on the temp ptr
unconditionally and it also avoids this messy labels.

> 
> > +exit:
> >  VIR_FREE(readonly_str);
> >  VIR_FREE(secure_str);
> >  VIR_FREE(type_str);
> > +
> >  return ret;
> >  }
> >
> > +static int
> > +virDomainLoaderNvramDefParseXML(xmlNodePtr node,
> > +   xmlXPathContextPtr ctxt,
> > +   virDomainLoaderDefPtr loader)
> > +{
> > +int ret = -1;
> > +char *tmp = NULL;
> > +xmlNodePtr cur;
> > +
> > +if (VIR_ALLOC(loader->nvram)) {
>^^
>Usually it is 

Re: [libvirt] [PATCH] travis: Uninstall packages before upgrade

[Andrea Bolognani]

On Mon, 2018-05-14 at 16:12 +0200, Michal Privoznik wrote:
> This OSX support is becoming more and more hairy. It's fairly often
> broken and we do nothing but compile test it (we don't even run make
> check there). So we can't be really sure the compiled virsh/client
> library still works there. I think it's time to have a discussion about
> dropping OSX support. Do we know if we even have any consumers running
> libvirt on OSX?

According to https://brew.sh/analytics/install/ there's some 20K
people with libvirt installed on their macOS machine; I think it's
fair to assume at least a fraction of them are actually using it.

Compiling on non-Linux platforms allows us to keep libvirt farily
portable, so having FreeBSD, MinGW and macOS builds running as
part of our CI setup is IMHO extremely valuable.

I consider macOS very much a "best effort" kind of deal, since
it's basically impossible to solve all but the most trivial issues
hitting it without first gaining access to proprietary platforms,
which is something that I'm personally not interested in doing.

That said, as long as we can keep it building by tweaking a couple
of lines every few months, I don't see a compelling argument for
dropping macOS support.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 03/12] Parse domain XML to generate virDomainLoaderDef & virDomainNvramDef.

[Marc Hartmayer]

On Mon, May 14, 2018 at 01:15 PM +0200, Prerna Saxena  
wrote:
> This patch is used to interpret domain XML and store the Loader &
> Nvram's backing definitions as virStorageSource.
> It also identifies if input XML used old or new-style declaration.
> (This will later be used by the formatter).
>
> Signed-off-by: Prerna Saxena 
> ---
>  src/conf/domain_conf.c | 131 
> ++---
>  1 file changed, 124 insertions(+), 7 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index f678e26..be43695 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2884,8 +2884,8 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr loader)
>  if (!loader)
>  return;
>
> -VIR_FREE(loader->path);
> -VIR_FREE(loader->nvram);
> +virStorageSourceFree(loader->src);
> +virStorageSourceFree(loader->nvram);
>  VIR_FREE(loader->templt);
>  VIR_FREE(loader);
>  }
> @@ -17986,17 +17986,62 @@ 
> virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
>
>  static int
>  virDomainLoaderDefParseXML(xmlNodePtr node,
> +   xmlXPathContextPtr ctxt,
> virDomainLoaderDefPtr loader)
>  {
>  int ret = -1;
>  char *readonly_str = NULL;
>  char *secure_str = NULL;
>  char *type_str = NULL;
> +char *tmp = NULL;
> +xmlNodePtr cur;
> +xmlXPathContextPtr cur_ctxt = ctxt;
> +
> +if (VIR_ALLOC(loader->src)) {
 ^^
 Usually it is checked for < 0.

> +goto cleanup;
> +}
> +loader->src->type = VIR_STORAGE_TYPE_LAST;
> +loader->oldStyleLoader = false;
>
>  readonly_str = virXMLPropString(node, "readonly");
>  secure_str = virXMLPropString(node, "secure");
>  type_str = virXMLPropString(node, "type");
> -loader->path = (char *) xmlNodeGetContent(node);
> +
> +if ((tmp = virXMLPropString(node, "backing")) &&
> +(loader->src->type = virStorageTypeFromString(tmp)) <= 0) {

If virStorageTypeFromString fails there is a memory leak of @tmp.

> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("unknown loader type '%s'"), tmp);
> +goto cleanup;
> +}
> +VIR_FREE(tmp);
> +
> +for (cur = node->children; cur != NULL; cur = cur->next) {
> +if (cur->type != XML_ELEMENT_NODE) {
> +continue;
> +}
> +
> +if (virXMLNodeNameEqual(cur, "source")) {
> +/* new-style declaration found */
> +if (virDomainStorageSourceParse(cur, cur_ctxt, loader->src, 0) < 
> 0) {
> +virReportError(VIR_ERR_XML_DETAIL,
> +   _("Error parsing Loader source element"));
> +goto cleanup;
> +}
> +break;
> +}
> +}
> +
> +/* Old-style absolute path found ? */
> +if (loader->src->type == VIR_STORAGE_TYPE_LAST) {
> +if (!(loader->src->path = (char *) xmlNodeGetContent(node))) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> +   _("missing loader source"));
> +goto cleanup;
> +} else {
> +loader->src->type = VIR_STORAGE_TYPE_FILE;
> +loader->oldStyleLoader = true;
> +}
> +}
>
>  if (readonly_str &&
>  (loader->readonly = virTristateBoolTypeFromString(readonly_str)) <= 
> 0) {
> @@ -18023,13 +18068,81 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
>  }
>
>  ret = 0;
> - cleanup:
> +goto exit;
> +cleanup:

I would replace: s/cleanup/error and s/exit/cleanup.

> +if (loader->src)
> +  VIR_FREE(loader->src);

Shouldn’t it be a virStorageSourceFree() here? Not sure if it’s needed
but it’s safer.

> +exit:
>  VIR_FREE(readonly_str);
>  VIR_FREE(secure_str);
>  VIR_FREE(type_str);
> +
>  return ret;
>  }
>
> +static int
> +virDomainLoaderNvramDefParseXML(xmlNodePtr node,
> +   xmlXPathContextPtr ctxt,
> +   virDomainLoaderDefPtr loader)
> +{
> +int ret = -1;
> +char *tmp = NULL;
> +xmlNodePtr cur;
> +
> +if (VIR_ALLOC(loader->nvram)) {
   ^^
   Usually it is checked for < 0.


> +goto cleanup;
> +}

Unneeded braces.

> +
> +loader->nvram->type = VIR_STORAGE_TYPE_LAST;
> +loader->nvram->oldStyleNvram = false;
> +
> +if ((tmp = virXMLPropString(node, "backing")) &&
> +(loader->nvram->type = virStorageTypeFromString(tmp)) <= 0) {

If virStorageTypeFromString fails there is a memory leak of @tmp.

> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("unknown nvram type '%s'"), tmp);
> +goto cleanup;
> +}
> +VIR_FREE(tmp);
> +
> +for (cur = node->children; cur != NULL; cur = cur->next) {
> +if 

Re: [libvirt] [PATCH] travis: Uninstall packages before upgrade

[Michal Privoznik]

On 05/14/2018 12:07 PM, Andrea Bolognani wrote:
> numpy (needed by cgal) started having the same issue with
> linking as python, which makes upgrade and thus the entire
> build fail on macOS.
> 
> Instead of playing more tricks with linking/unlinking, just
> uninstall the problematic packages (and those dragging them
> in) before doing anything else.
> 
> Signed-off-by: Andrea Bolognani 
> ---
> Technically a build breaker, but since I'm changing the
> approach I'd rather wait for an explicit ACK before pushing.
> 
>  .travis.yml | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/.travis.yml b/.travis.yml
> index d3f72d46f3..140072b801 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -21,10 +21,10 @@ matrix:
>  - compiler: clang
>os: osx
>before_install:
> +- brew uninstall python mercurial postgis sfcgal cgal gdal
>  - brew update
> -- brew unlink python
>  - brew upgrade
> -- brew install rpcgen yajl
> +- brew install python rpcgen yajl
>script:
>  # We can't run make distcheck/syntax-check because they
>  # fail on macOS, but doing 'install' and 'dist' gives us
> 

This OSX support is becoming more and more hairy. It's fairly often
broken and we do nothing but compile test it (we don't even run make
check there). So we can't be really sure the compiled virsh/client
library still works there. I think it's time to have a discussion about
dropping OSX support. Do we know if we even have any consumers running
libvirt on OSX?

Michal

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

Re: [libvirt] [PATCH] tests: Fix qemucapsprobemock linking

[Michal Privoznik]

On 05/10/2018 11:13 PM, Martin Kletzander wrote:
> Add libvirt.la into qemucapsprobemock_la_LIBADD because that mock uses bunch 
> of
> libvirt internal functions.  Without this patch the following error occurs:
> 
>   $ ./qemucapsprobe /usr/bin/qemu-system-x86_64
>   ...
>   warning : virQEMUCapsLogProbeFailure:4204 : Failed to probe capabilities for
>   /usr/bin/qemu-system-x86_64: internal error: Failed to probe QEMU binary 
> with
>   QMP: /usr/bin/qemu-system-x86_64: symbol lookup error:
>   .../qemucapsprobemock.so: undefined symbol: virJSONValueObjectHasKey
> 
> Signed-off-by: Martin Kletzander 
> ---
> 
> TBH I didn't go into details why this is needed.  It still seems fishy to me.
> Not only that I get that error above as a libvirt error, but if i add just
> libvirt_util.la in there, I get:
> 
>   .../qemucapsprobe: symbol lookup error: .../qemucapsprobemock.so:
>   undefined symbol: libvirt_event_poll_update_handle_semaphore

This is because libvirt_utils is built WITH_DTRACE_PROBES enabled.
Therefore it requires libvirt_probes.lo. So adding these two should be
enough.

However, I'm unable to reproduce this. But what I am able to reproduce is:

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

  Missing symbol 'virFileCanonicalizePath'
  Aborted (core dumped)

This points to merely the same problem. In our mocks we are/want to use
our internal APIs. Therefore instead of relying on program that links
with the mock at runtime (VIR_TEST_MAIN_PRELOAD macro) being already
linked with libvirt.so I think we should link our mocks with it:

diff --git i/tests/Makefile.am w/tests/Makefile.am
index 621480dd0c..ac92190845 100644
--- i/tests/Makefile.am
+++ w/tests/Makefile.am
@@ -81,7 +81,8 @@ LDADDS = \
../src/libvirt.la
 
 MOCKLIBS_LIBS = \
-   $(GNULIB_LIBS)
+   $(GNULIB_LIBS) \
+   ../src/libvirt.la
 
 EXTRA_DIST = \
.valgrind.supp \

Michal


P.S. This opened a pandora box for me when I tried running qemucapsprobe
over the latest qemu compiled from git only to find that qemu has a
regression when it does not accept -no-user-config on the command line
anymore. Patch proposed upstream (link not yet available because their
list is very slow).

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

[libvirt] [PATCH] log: actually do substring matches with fnmatch

[Daniel P . Berrangé]

Historically we matched log filters with strstr(), and when switching to
fnmatch in cbb0fd3cfdc287f6f4653ef1f04a7cfb2ea51b27, it was stated that
we would continue to match substrings, with "foo" being equivalent to
"*foo*". Unfortuntely I forget to provide the code to actually make that
happen. This fixes it to prepend and append "*" if not already present in
the user's pattern.

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

diff --git a/src/util/virlog.c b/src/util/virlog.c
index be9fc0cf78..d548010b10 100644
--- a/src/util/virlog.c
+++ b/src/util/virlog.c
@@ -1409,6 +1409,8 @@ virLogFilterNew(const char *match,
 {
 virLogFilterPtr ret = NULL;
 char *mdup = NULL;
+size_t mlen = strlen(match);
+size_t off = 0;
 
 virCheckFlags(VIR_LOG_STACK_TRACE, NULL);
 
@@ -1418,9 +1420,19 @@ virLogFilterNew(const char *match,
 return NULL;
 }
 
-if (VIR_STRDUP_QUIET(mdup, match) < 0)
+if (VIR_ALLOC_N_QUIET(mdup, mlen + 3) < 0)
 return NULL;
 
+if (match[0] != '*') {
+mdup[off++] = '*';
+}
+memcpy(mdup + off, match, mlen + 1);
+off += mlen;
+if (match[mlen - 1] != '*') {
+mdup[off++] = '*';
+mdup[off++] = '\0';
+}
+
 if (VIR_ALLOC_QUIET(ret) < 0) {
 VIR_FREE(mdup);
 return NULL;
-- 
2.17.0

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

Re: [libvirt] [PATCH v2 3/5] qemu: Add gl property to graphics of type sdl in domain config

[John Ferlan]

On 05/14/2018 09:18 AM, Maciej Wolny wrote:
> On 14/05/18 13:40, Martin Kletzander wrote:
>> On Mon, May 14, 2018 at 08:27:35AM -0400, John Ferlan wrote:
>>>
>>>
>>> On 05/14/2018 07:24 AM, Martin Kletzander wrote:
 On Fri, May 11, 2018 at 03:09:20PM +0100, Maciej Wolny wrote:
> On 11/05/18 09:42, Martin Kletzander wrote:
>> On Thu, May 10, 2018 at 11:53:57AM +0100, Maciej Wolny wrote:
>>> Support OpenGL accelerated rendering when using SDL graphics in the
>>> domain config. Add associated test and documentation.
>>>
>>> Signed-off-by: Maciej Wolny 
>>> ---
>>> docs/formatdomain.html.in  |  6 +++
>>> docs/schemas/domaincommon.rng  |  8 
>>> src/conf/domain_conf.c | 44
>>> -
>>> src/conf/domain_conf.h |  1 +
>>
>> docs, conf and schemas fit together nicely, they should be in one
>> patch, but.
>>
>>> tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml | 38
>>> ++
>>> .../qemuxml2xmloutdata/video-virtio-gpu-sdl-gl.xml | 45
>>> ++
>>> tests/qemuxml2xmltest.c    |  1 +
>>
>> this has nothing to do with qemu (yet), also see Subject (I wouldn't say
>> 'qemu:' there, but rather something like 'docs, conf, schema:')
>>
>> For the XML tests above you can use genericxml2xmltest instead of the
>> QEMU-specific one.
>
> The option only makes sense in QEMU afaik, hence the naming.
>

 Yes, for now.  If someone who's building the code without QEMU driver
 changes
 the behaviour, the tests will pass if you keep it in qemuxml2xml, however
 genericxml2xml will catch that.  qemuxml2xml should be testing specifics
 where
 you behave based on some more information than just generic XML.

 I hope that's clear.

 Have a nice day.

>>>
>>> However, until qemuxml2argvtest can also pull out of genericxml2xmldata,
>>> then you'd have separate xml input and output files - is that what's
>>> desired?
>>>
>>> Taking a quick look just now - see the graphics-vnc-socket - do we want
>>> to duplicate having two input/output XML files which invariably will
>>> diverge? Ironically the generic one has a domain type == qemu, an
>>> emulator using qemu, and the socket path using QEMU - so while it's
>>> generic in one sense, it's not in others. Even more ironic is the qemu
>>> specific file uses "".
>>>
>>> Could/should generification of the xml2xml tests be considered a "bite
>>> sized task"?
>>>
>>
>> Oh, definitely.  It's only some time ago that the tests started to be usable
>> IIRC, so hopefully we'll migrate some XMLs here and there.  But maybe others
>> could chime in as well so that I don't speak for others.  I remember Pavel
>> having some ideas for cleaner separation of those.
> 
> So, do you guys want to leave that for a separate patch set or do you want me 
> to
> post a v3 with the changes Martin has requested?
> 

My opinion is leave it as is - hard to require you to do something we're
not requiring other patches at this point. I'm not a fan of duplication
- that is the task would be to essentially copy everything into the
generic xml2xml test at this point.

John

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

Re: [libvirt] [PATCH v2 3/5] qemu: Add gl property to graphics of type sdl in domain config

[Maciej Wolny]

On 14/05/18 13:40, Martin Kletzander wrote:
> On Mon, May 14, 2018 at 08:27:35AM -0400, John Ferlan wrote:
>>
>>
>> On 05/14/2018 07:24 AM, Martin Kletzander wrote:
>>> On Fri, May 11, 2018 at 03:09:20PM +0100, Maciej Wolny wrote:
 On 11/05/18 09:42, Martin Kletzander wrote:
> On Thu, May 10, 2018 at 11:53:57AM +0100, Maciej Wolny wrote:
>> Support OpenGL accelerated rendering when using SDL graphics in the
>> domain config. Add associated test and documentation.
>>
>> Signed-off-by: Maciej Wolny 
>> ---
>> docs/formatdomain.html.in  |  6 +++
>> docs/schemas/domaincommon.rng  |  8 
>> src/conf/domain_conf.c | 44
>> -
>> src/conf/domain_conf.h |  1 +
>
> docs, conf and schemas fit together nicely, they should be in one
> patch, but.
>
>> tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml | 38
>> ++
>> .../qemuxml2xmloutdata/video-virtio-gpu-sdl-gl.xml | 45
>> ++
>> tests/qemuxml2xmltest.c    |  1 +
>
> this has nothing to do with qemu (yet), also see Subject (I wouldn't say
> 'qemu:' there, but rather something like 'docs, conf, schema:')
>
> For the XML tests above you can use genericxml2xmltest instead of the
> QEMU-specific one.

 The option only makes sense in QEMU afaik, hence the naming.

>>>
>>> Yes, for now.  If someone who's building the code without QEMU driver
>>> changes
>>> the behaviour, the tests will pass if you keep it in qemuxml2xml, however
>>> genericxml2xml will catch that.  qemuxml2xml should be testing specifics
>>> where
>>> you behave based on some more information than just generic XML.
>>>
>>> I hope that's clear.
>>>
>>> Have a nice day.
>>>
>>
>> However, until qemuxml2argvtest can also pull out of genericxml2xmldata,
>> then you'd have separate xml input and output files - is that what's
>> desired?
>>
>> Taking a quick look just now - see the graphics-vnc-socket - do we want
>> to duplicate having two input/output XML files which invariably will
>> diverge? Ironically the generic one has a domain type == qemu, an
>> emulator using qemu, and the socket path using QEMU - so while it's
>> generic in one sense, it's not in others. Even more ironic is the qemu
>> specific file uses "".
>>
>> Could/should generification of the xml2xml tests be considered a "bite
>> sized task"?
>>
> 
> Oh, definitely.  It's only some time ago that the tests started to be usable
> IIRC, so hopefully we'll migrate some XMLs here and there.  But maybe others
> could chime in as well so that I don't speak for others.  I remember Pavel
> having some ideas for cleaner separation of those.

So, do you guys want to leave that for a separate patch set or do you want me to
post a v3 with the changes Martin has requested?

-- milloni

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

Re: [libvirt] [PATCH 2/5] Introduce virCryptoHashBuf

[Ján Tomko]

On Mon, May 14, 2018 at 11:17:51AM +0200, Michal Privoznik wrote:

On 05/11/2018 05:50 PM, Ján Tomko wrote:

A function that keeps the hash in binary form instead of converting
it to human-readable hexadecimal form.

Signed-off-by: Ján Tomko 
---
 src/util/vircrypto.c | 31 +--
 src/util/vircrypto.h |  7 +++
 2 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c
index 48b04fc8ce..1a2dcc28b7 100644
--- a/src/util/vircrypto.c
+++ b/src/util/vircrypto.c
@@ -54,28 +54,39 @@ struct virHashInfo {
 verify(ARRAY_CARDINALITY(hashinfo) == VIR_CRYPTO_HASH_LAST);

 int
-virCryptoHashString(virCryptoHash hash,
-const char *input,
-char **output)
+virCryptoHashBuf(virCryptoHash hash,
+ const char *input,
+ unsigned char *output)
 {
-unsigned char buf[VIR_CRYPTO_LARGEST_DIGEST_SIZE];
-size_t hashstrlen;
-size_t i;
-
 if (hash >= VIR_CRYPTO_HASH_LAST) {
 virReportError(VIR_ERR_INVALID_ARG,
_("Unknown crypto hash %d"), hash);
 return -1;
 }


This check feels useless. It's like if we were checking a value before
passing it to vir*TypeToString(). But it's pre-existing, so you have my
ACK. But a follow up patch removing it (=trivial) would be nice.


On one hand, this check should be pointless if the rest of our code is
correct.

On the other hand, our functions in src/util usually do perform some
validation of the parameters. Although it could be transformed to use
virReportEnumRangeError.



Also, don't forget to export the symbol in libvirt_private.syms ;-)
I'm wondering how linker succeeds in 3/5 where you use the function
without it being exported. Maybe my compiler decided to inline this
function?


Thanks, fixed.

Jano



Michal


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

Re: [libvirt] [PATCH v2 3/5] qemu: Add gl property to graphics of type sdl in domain config

[Martin Kletzander]

On Mon, May 14, 2018 at 08:27:35AM -0400, John Ferlan wrote:



On 05/14/2018 07:24 AM, Martin Kletzander wrote:

On Fri, May 11, 2018 at 03:09:20PM +0100, Maciej Wolny wrote:

On 11/05/18 09:42, Martin Kletzander wrote:

On Thu, May 10, 2018 at 11:53:57AM +0100, Maciej Wolny wrote:

Support OpenGL accelerated rendering when using SDL graphics in the
domain config. Add associated test and documentation.

Signed-off-by: Maciej Wolny 
---
docs/formatdomain.html.in  |  6 +++
docs/schemas/domaincommon.rng  |  8 
src/conf/domain_conf.c | 44
-
src/conf/domain_conf.h |  1 +


docs, conf and schemas fit together nicely, they should be in one
patch, but.


tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml | 38
++
.../qemuxml2xmloutdata/video-virtio-gpu-sdl-gl.xml | 45
++
tests/qemuxml2xmltest.c    |  1 +


this has nothing to do with qemu (yet), also see Subject (I wouldn't say
'qemu:' there, but rather something like 'docs, conf, schema:')

For the XML tests above you can use genericxml2xmltest instead of the
QEMU-specific one.


The option only makes sense in QEMU afaik, hence the naming.



Yes, for now.  If someone who's building the code without QEMU driver
changes
the behaviour, the tests will pass if you keep it in qemuxml2xml, however
genericxml2xml will catch that.  qemuxml2xml should be testing specifics
where
you behave based on some more information than just generic XML.

I hope that's clear.

Have a nice day.



However, until qemuxml2argvtest can also pull out of genericxml2xmldata,
then you'd have separate xml input and output files - is that what's
desired?

Taking a quick look just now - see the graphics-vnc-socket - do we want
to duplicate having two input/output XML files which invariably will
diverge? Ironically the generic one has a domain type == qemu, an
emulator using qemu, and the socket path using QEMU - so while it's
generic in one sense, it's not in others. Even more ironic is the qemu
specific file uses "".

Could/should generification of the xml2xml tests be considered a "bite
sized task"?



Oh, definitely.  It's only some time ago that the tests started to be usable
IIRC, so hopefully we'll migrate some XMLs here and there.  But maybe others
could chime in as well so that I don't speak for others.  I remember Pavel
having some ideas for cleaner separation of those.

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

Re: [libvirt] [PATCH v2 6/5] docs/news.xml: Update with QEMU SDL OpenGL Improvement

[John Ferlan]

$SUBJ:

docs: Update news.xml with QEMU SDL OpenGL Improvement


On 05/11/2018 10:47 AM, Maciej Wolny wrote:
> Signed-off-by: Maciej Wolny 
> ---
>  docs/news.xml | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/docs/news.xml b/docs/news.xml
> index 80181415c..e4a10f667 100644
> --- a/docs/news.xml
> +++ b/docs/news.xml
> @@ -37,6 +37,15 @@
>  
>  
>  
> +  
> +  
> +qemu: Add suport for OpenGL rendering with SDL
> +  
> +  
> +Domains using SDL as a graphics backend will now be able to use
> +OpenGL accelerated rendering.
> +  
> +  

This should have been put into the 4.4 section not the comments... And
each indention level can be 2 spaces... I will move/fix it...

Reviewed-by: John Ferlan 

John


>  
>  
>  
> 

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

Re: [libvirt] [PATCH v2 3/5] qemu: Add gl property to graphics of type sdl in domain config

[John Ferlan]

On 05/14/2018 07:24 AM, Martin Kletzander wrote:
> On Fri, May 11, 2018 at 03:09:20PM +0100, Maciej Wolny wrote:
>> On 11/05/18 09:42, Martin Kletzander wrote:
>>> On Thu, May 10, 2018 at 11:53:57AM +0100, Maciej Wolny wrote:
 Support OpenGL accelerated rendering when using SDL graphics in the
 domain config. Add associated test and documentation.

 Signed-off-by: Maciej Wolny 
 ---
 docs/formatdomain.html.in  |  6 +++
 docs/schemas/domaincommon.rng  |  8 
 src/conf/domain_conf.c | 44
 -
 src/conf/domain_conf.h |  1 +
>>>
>>> docs, conf and schemas fit together nicely, they should be in one
>>> patch, but.
>>>
 tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml | 38
 ++
 .../qemuxml2xmloutdata/video-virtio-gpu-sdl-gl.xml | 45
 ++
 tests/qemuxml2xmltest.c    |  1 +
>>>
>>> this has nothing to do with qemu (yet), also see Subject (I wouldn't say
>>> 'qemu:' there, but rather something like 'docs, conf, schema:')
>>>
>>> For the XML tests above you can use genericxml2xmltest instead of the
>>> QEMU-specific one.
>>
>> The option only makes sense in QEMU afaik, hence the naming.
>>
> 
> Yes, for now.  If someone who's building the code without QEMU driver
> changes
> the behaviour, the tests will pass if you keep it in qemuxml2xml, however
> genericxml2xml will catch that.  qemuxml2xml should be testing specifics
> where
> you behave based on some more information than just generic XML.
> 
> I hope that's clear.
> 
> Have a nice day.
> 

However, until qemuxml2argvtest can also pull out of genericxml2xmldata,
then you'd have separate xml input and output files - is that what's
desired?

Taking a quick look just now - see the graphics-vnc-socket - do we want
to duplicate having two input/output XML files which invariably will
diverge? Ironically the generic one has a domain type == qemu, an
emulator using qemu, and the socket path using QEMU - so while it's
generic in one sense, it's not in others. Even more ironic is the qemu
specific file uses "".

Could/should generification of the xml2xml tests be considered a "bite
sized task"?

John

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

Re: [libvirt] [PATCH v2 3/5] qemu: Add gl property to graphics of type sdl in domain config

[Martin Kletzander]

On Fri, May 11, 2018 at 03:09:20PM +0100, Maciej Wolny wrote:

On 11/05/18 09:42, Martin Kletzander wrote:

On Thu, May 10, 2018 at 11:53:57AM +0100, Maciej Wolny wrote:

Support OpenGL accelerated rendering when using SDL graphics in the
domain config. Add associated test and documentation.

Signed-off-by: Maciej Wolny 
---
docs/formatdomain.html.in  |  6 +++
docs/schemas/domaincommon.rng  |  8 
src/conf/domain_conf.c | 44 -
src/conf/domain_conf.h |  1 +


docs, conf and schemas fit together nicely, they should be in one patch, but.


tests/qemuxml2argvdata/video-virtio-gpu-sdl-gl.xml | 38 ++
.../qemuxml2xmloutdata/video-virtio-gpu-sdl-gl.xml | 45 ++
tests/qemuxml2xmltest.c    |  1 +


this has nothing to do with qemu (yet), also see Subject (I wouldn't say
'qemu:' there, but rather something like 'docs, conf, schema:')

For the XML tests above you can use genericxml2xmltest instead of the
QEMU-specific one.


The option only makes sense in QEMU afaik, hence the naming.



Yes, for now.  If someone who's building the code without QEMU driver changes
the behaviour, the tests will pass if you keep it in qemuxml2xml, however
genericxml2xml will catch that.  qemuxml2xml should be testing specifics where
you behave based on some more information than just generic XML.

I hope that's clear.

Have a nice day.

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

Re: [libvirt] [PATCH v2 4/5] qemu: Add QEMU_CAPS_SDL_GL to qemu capabilities

[Martin Kletzander]

On Fri, May 11, 2018 at 03:14:26PM +0100, Maciej Wolny wrote:

On 11/05/18 11:34, John Ferlan wrote:> I actually spent some time trying to 
figure out which magic incantation

of the virQEMUCaps* would work. I even tried various forms in
virQEMUCapsQMPSchemaQueries, but could not get the flag to be added from
qemu 2.4 and beyond.  Again, my "assumption" is that it was hand edited
into the 2.4 replies that was added here mainly because none of the
other available sdl * options were addressed - just the one that was wanted.


It was hand-edited. I'm still not sure how this work.. I ran the tool to
generate a .replies file; but based on that, how do I get the replies for
all historical version of QEMU, do I need to have each of the versions of
QEMU installed on my system.



Unfortunately, yes.  There was some work done so taht we could automate that,
but nobody actually finished the work.  Anyway, since QEMU does not expose that
capability in the QMP communication (what libvirt uses to check for the
support), we need to guess it based on the release version :(


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

[libvirt] [PATCH 07/12] Bhyve: Fix command line generation to correctly pick up local loader path.

[Prerna Saxena]

Signed-off-by: Prerna Saxena 
---
 src/bhyve/bhyve_command.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index 9413ae5..2b67014 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -518,10 +518,12 @@ virBhyveProcessBuildBhyveCmd(virConnectPtr conn,
 virCommandAddArgList(cmd, "-s", "0:0,hostbridge", NULL);
 
 if (def->os.bootloader == NULL &&
-def->os.loader) {
+def->os.loader &&
+def->os.loader->src &&
+def->os.loader->src->type == VIR_STORAGE_TYPE_FILE) {
 if ((bhyveDriverGetCaps(conn) & BHYVE_CAP_LPC_BOOTROM)) {
 virCommandAddArg(cmd, "-l");
-virCommandAddArgFormat(cmd, "bootrom,%s", def->os.loader->path);
+virCommandAddArgFormat(cmd, "bootrom,%s", 
def->os.loader->src->path);
 add_lpc = true;
 } else {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-- 
1.8.1.2

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

[libvirt] [PATCH 09/12] Vbox: Adjust references to 'loader' and 'nvram' elements given that these are now represented by virStorageSourcePtr.

[Prerna Saxena]

Signed-off-by: Prerna Saxena 
---
 src/vbox/vbox_common.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 72a24a3..60451a3 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -998,11 +998,16 @@ vboxSetBootDeviceOrder(virDomainDefPtr def, vboxDriverPtr 
data,
 VIR_DEBUG("def->os.initrd   %s", def->os.initrd);
 VIR_DEBUG("def->os.cmdline  %s", def->os.cmdline);
 VIR_DEBUG("def->os.root %s", def->os.root);
-if (def->os.loader) {
-VIR_DEBUG("def->os.loader->path %s", def->os.loader->path);
+if (def->os.loader &&
+def->os.loader->src &&
+def->os.loader->src->type == VIR_STORAGE_TYPE_FILE) {
+
+VIR_DEBUG("def->os.loader->src->path %s", 
def->os.loader->src->path);
 VIR_DEBUG("def->os.loader->readonly %d", def->os.loader->readonly);
 VIR_DEBUG("def->os.loader->type %d", def->os.loader->type);
-VIR_DEBUG("def->os.loader->nvram%s", def->os.loader->nvram);
+if (def->os.loader->nvram &&
+def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE)
+VIR_DEBUG("def->os.loader->nvram->path%s", 
def->os.loader->nvram->path);
 }
 VIR_DEBUG("def->os.bootloader   %s", def->os.bootloader);
 VIR_DEBUG("def->os.bootloaderArgs   %s", def->os.bootloaderArgs);
-- 
1.8.1.2

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

[libvirt] [PATCH 11/12] Test: Add a test snippet to evaluate command line generation for loader/nvram specified via virStorageSource

[Prerna Saxena]

Signed-off-by: Prerna Saxena 
---
 tests/qemuxml2argvdata/bios-nvram-network.args | 31 +++
 tests/qemuxml2argvdata/bios-nvram-network.xml  | 42 ++
 tests/qemuxml2argvtest.c   |  1 +
 3 files changed, 74 insertions(+)
 create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.args
 create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.xml

diff --git a/tests/qemuxml2argvdata/bios-nvram-network.args 
b/tests/qemuxml2argvdata/bios-nvram-network.args
new file mode 100644
index 000..3fc68ef
--- /dev/null
+++ b/tests/qemuxml2argvdata/bios-nvram-network.args
@@ -0,0 +1,31 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-x86_64 \
+-name test-bios \
+-S \
+-machine pc,accel=tcg,usb=off,dump-guest-core=off \
+-drive file=/usr/share/OVMF/OVMF_CODE.fd,if=pflash,format=raw,unit=0,\
+readonly=on \
+-drive file=iscsi://example.org:6000/iqn.1992-01.com.example/0,\
+if=pflash,format=raw,unit=1 \
+-m 1024 \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid 362d1fc1-df7d-193e-5c18-49a71bd1da66 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-test-bios/monitor.sock,\
+server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-boot order=c,menu=on \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
+-device usb-tablet,id=input0,bus=usb.0,port=1 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/bios-nvram-network.xml 
b/tests/qemuxml2argvdata/bios-nvram-network.xml
new file mode 100644
index 000..bad114c
--- /dev/null
+++ b/tests/qemuxml2argvdata/bios-nvram-network.xml
@@ -0,0 +1,42 @@
+
+  test-bios
+  362d1fc1-df7d-193e-5c18-49a71bd1da66
+  1048576
+  1048576
+  1
+  
+hvm
+
+  
+
+
+  
+
+  
+
+
+
+  
+  
+
+  
+  
+  destroy
+  restart
+  restart
+  
+/usr/bin/qemu-system-x86_64
+
+  
+  
+  
+
+
+
+
+
+
+
+
+  
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index ddf567b..7338dba 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -853,6 +853,7 @@ mymain(void)
 QEMU_CAPS_DEVICE_ISA_SERIAL,
 QEMU_CAPS_SGA);
 DO_TEST("bios-nvram", NONE);
+DO_TEST("bios-nvram-network", NONE);
 DO_TEST("bios-nvram-secure",
 QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
 QEMU_CAPS_DEVICE_PCI_BRIDGE,
-- 
1.8.1.2

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

[libvirt] [PATCH 05/12] Plumb the loader source into generation of QEMU command line.

[Prerna Saxena]

Given that nvram & loader elements can now be backed by a non-local
source too, adjust all steps leading to generation of
QEMU command line.

Signed-off-by: Prerna Saxena 
---
 src/qemu/qemu_cgroup.c  | 13 +
 src/qemu/qemu_command.c | 21 -
 src/qemu/qemu_domain.c  | 31 +-
 src/qemu/qemu_driver.c  |  7 ---
 src/qemu/qemu_process.c | 42 -
 src/security/security_dac.c |  6 --
 src/security/security_selinux.c |  6 --
 7 files changed, 88 insertions(+), 38 deletions(-)

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index d88eb78..2068eb0 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -580,16 +580,21 @@ qemuSetupMemoryCgroup(virDomainObjPtr vm)
 static int
 qemuSetupFirmwareCgroup(virDomainObjPtr vm)
 {
+virStorageSourcePtr src = NULL;
+
 if (!vm->def->os.loader)
 return 0;
 
-if (vm->def->os.loader->path &&
-qemuSetupImagePathCgroup(vm, vm->def->os.loader->path,
- vm->def->os.loader->readonly == 
VIR_TRISTATE_BOOL_YES) < 0)
+src = vm->def->os.loader->src;
+
+if (src->type == VIR_STORAGE_TYPE_FILE &&
+qemuSetupImagePathCgroup(vm, src->path,
+ src->readonly == VIR_TRISTATE_BOOL_YES) < 0)
 return -1;
 
 if (vm->def->os.loader->nvram &&
-qemuSetupImagePathCgroup(vm, vm->def->os.loader->nvram, false) < 0)
+vm->def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
+qemuSetupImagePathCgroup(vm, vm->def->os.loader->nvram->path, false) < 
0)
 return -1;
 
 return 0;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 08f67a4..e9d6e4b 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9260,6 +9260,7 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
 virDomainLoaderDefPtr loader = def->os.loader;
 virBuffer buf = VIR_BUFFER_INITIALIZER;
 int unit = 0;
+char *source = NULL;
 
 if (!loader)
 return;
@@ -9267,7 +9268,7 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
 switch ((virDomainLoader) loader->type) {
 case VIR_DOMAIN_LOADER_TYPE_ROM:
 virCommandAddArg(cmd, "-bios");
-virCommandAddArg(cmd, loader->path);
+virCommandAddArg(cmd, loader->src->path);
 break;
 
 case VIR_DOMAIN_LOADER_TYPE_PFLASH:
@@ -9279,9 +9280,14 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
  NULL);
 }
 
+if (qemuGetDriveSourceString(loader->src, NULL, ) < 0)
+break;
+
 virBufferAddLit(, "file=");
-virQEMUBuildBufferEscapeComma(, loader->path);
-virBufferAsprintf(, ",if=pflash,format=raw,unit=%d", unit);
+virQEMUBuildBufferEscapeComma(, source);
+free(source);
+virBufferAsprintf(, ",if=pflash,format=raw,unit=%d",
+  unit);
 unit++;
 
 if (loader->readonly) {
@@ -9294,9 +9300,14 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
 
 if (loader->nvram) {
 virBufferFreeAndReset();
+if (qemuGetDriveSourceString(loader->nvram, NULL, ) < 0)
+break;
+
 virBufferAddLit(, "file=");
-virQEMUBuildBufferEscapeComma(, loader->nvram);
-virBufferAsprintf(, ",if=pflash,format=raw,unit=%d", unit);
+virQEMUBuildBufferEscapeComma(, source);
+virBufferAsprintf(, ",if=pflash,format=raw,unit=%d",
+  unit);
+unit++;
 
 virCommandAddArg(cmd, "-drive");
 virCommandAddArgBuffer(cmd, );
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 9bb6d8a..2d4e299 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3318,6 +3318,9 @@ qemuDomainDefPostParse(virDomainDefPtr def,
  * function shall not fail in that case. It will be re-run on VM startup
  * with the capabilities populated. */
 virQEMUCapsPtr qemuCaps = parseOpaque;
+virDomainLoaderDefPtr ldr = NULL;
+char *nvramPath = NULL;
+
 int ret = -1;
 
 if (def->os.bootloader || def->os.bootloaderArgs) {
@@ -3332,13 +3335,20 @@ qemuDomainDefPostParse(virDomainDefPtr def,
 goto cleanup;
 }
 
-if (def->os.loader &&
-def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH &&
-def->os.loader->readonly == VIR_TRISTATE_SWITCH_ON &&
-!def->os.loader->nvram) {
-if (virAsprintf(>os.loader->nvram, "%s/%s_VARS.fd",
+ldr = def->os.loader;
+if (ldr &&
+ldr->type == VIR_DOMAIN_LOADER_TYPE_PFLASH &&
+ldr->readonly == VIR_TRISTATE_SWITCH_ON &&
+!ldr->nvram) {
+if (virAsprintf(, "%s/%s_VARS.fd",
 cfg->nvramDir, def->name) < 0)
 goto cleanup;
+ldr->nvram = 

[libvirt] [PATCH 06/12] Fix the domain def inference logic to correctly account for network-backed pflash devices.

[Prerna Saxena]

Signed-off-by: Prerna Saxena 
---
 src/qemu/qemu_parse_command.c | 30 +++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c
index 351425f..9b1a86e 100644
--- a/src/qemu/qemu_parse_command.c
+++ b/src/qemu/qemu_parse_command.c
@@ -650,6 +650,7 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt,
 int idx = -1;
 int busid = -1;
 int unitid = -1;
+bool is_firmware = false;
 
 if (qemuParseKeywords(val,
   ,
@@ -772,6 +773,9 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt,
 def->bus = VIR_DOMAIN_DISK_BUS_VIRTIO;
 } else if (STREQ(values[i], "xen")) {
 def->bus = VIR_DOMAIN_DISK_BUS_XEN;
+} else if (STREQ(values[i], "pflash")) {
+def->bus = VIR_DOMAIN_DISK_BUS_LAST;
+is_firmware = true;
 } else if (STREQ(values[i], "sd")) {
 def->bus = VIR_DOMAIN_DISK_BUS_SD;
 }
@@ -943,8 +947,25 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt,
 ignore_value(VIR_STRDUP(def->dst, "hda"));
 }
 
-if (!def->dst)
-goto error;
+if (!def->dst) {
+if (is_firmware && def->bus == VIR_DOMAIN_DISK_BUS_LAST) {
+if (!dom->os.loader && (VIR_ALLOC(dom->os.loader) < 0))
+goto error;
+if (def->src->readonly) {
+/* Loader spec */
+dom->os.loader->src = def->src;
+dom->os.loader->type = VIR_DOMAIN_LOADER_TYPE_PFLASH;
+} else {
+/* NVRAM Spec */
+if (!dom->os.loader->nvram && 
(VIR_ALLOC(dom->os.loader->nvram) < 0))
+goto error;
+dom->os.loader->nvram = def->src;
+}
+} else {
+goto error;
+}
+}
+
 if (STREQ(def->dst, "xvda"))
 def->dst[3] = 'a' + idx;
 else
@@ -2215,8 +2236,11 @@ qemuParseCommandLine(virCapsPtr caps,
 } else if (STREQ(arg, "-bios")) {
 WANT_VALUE();
 if (VIR_ALLOC(def->os.loader) < 0 ||
-VIR_STRDUP(def->os.loader->path, val) < 0)
+VIR_ALLOC(def->os.loader->src) < 0 ||
+VIR_STRDUP(def->os.loader->src->path, val) < 0)
 goto error;
+def->os.loader->src->type = VIR_STORAGE_TYPE_FILE;
+def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_ROM;
 } else if (STREQ(arg, "-initrd")) {
 WANT_VALUE();
 if (VIR_STRDUP(def->os.initrd, val) < 0)
-- 
1.8.1.2

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

[libvirt] [PATCH 01/12] Schema: Introduce XML schema for network-backed loader and nvram elements.

[Prerna Saxena]

Today, 'loader' and 'nvram' elements are supposed to be backed by a local disk.
Given that NVRAM data could be network-backed for high availability, this patch
defines XML spec for serving loader & nvram disks via the network.

Signed-off-by: Prerna Saxena 
---
 docs/schemas/domaincommon.rng | 108 +++---
 1 file changed, 90 insertions(+), 18 deletions(-)

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 0a6b29b..a6207db 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -276,7 +276,42 @@
 
   
 
-
+
+  
+
+  file
+  network
+
+  
+
+
+  
+ 
+  
+ 
+ 
+  
+ 
+ 
+  
+ 
+ 
+  
+ 
+ 
+  
+ 
+ 
+  
+ 
+ 
+  
+ 
+ 
+  
+ 
+  
+
   
 
 
@@ -287,7 +322,40 @@
   
 
 
-  
+  
+
+  file
+  network
+
+  
+
+
+  
+ 
+  
+ 
+ 
+  
+ 
+ 
+  
+ 
+ 
+  
+ 
+ 
+  
+ 
+ 
+  
+ 
+ 
+  
+ 
+ 
+  
+ 
+  
 
   
 
@@ -1494,25 +1562,29 @@
   
 
 
-  
-
-  
-
-  
-
-
-  
-
-
-  
-
-
-  
-
-  
+  
 
   
 
+  
+
+  
+
+  
+
+  
+  
+
+  
+  
+
+  
+  
+
+  
+
+  
+
   
 
   block
-- 
1.8.1.2

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

[libvirt] [PATCH 10/12] Xen: Adjust all references to loader & nvram elements given that they are now backed by virStorageSourcePtr

[Prerna Saxena]

Signed-off-by: Prerna Saxena 
---
 src/xenapi/xenapi_driver.c |  4 +++-
 src/xenconfig/xen_sxpr.c   | 19 +++
 src/xenconfig/xen_xm.c |  9 ++---
 3 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c
index 42b305d..4070660 100644
--- a/src/xenapi/xenapi_driver.c
+++ b/src/xenapi/xenapi_driver.c
@@ -1444,10 +1444,12 @@ xenapiDomainGetXMLDesc(virDomainPtr dom, unsigned int 
flags)
 char *value = NULL;
 defPtr->os.type = VIR_DOMAIN_OSTYPE_XEN;
 if (VIR_ALLOC(defPtr->os.loader) < 0 ||
-VIR_STRDUP(defPtr->os.loader->path, "pygrub") < 0) {
+VIR_ALLOC(defPtr->os.loader->src) < 0 ||
+VIR_STRDUP(defPtr->os.loader->src->path, "pygrub") < 0) {
 VIR_FREE(boot_policy);
 goto error;
 }
+defPtr->os.loader->src->type = VIR_STORAGE_TYPE_FILE;
 xen_vm_get_pv_kernel(session, , vm);
 if (STRNEQ(value, "")) {
 if (VIR_STRDUP(defPtr->os.kernel, value) < 0) {
diff --git a/src/xenconfig/xen_sxpr.c b/src/xenconfig/xen_sxpr.c
index e868c05..fd3165c 100644
--- a/src/xenconfig/xen_sxpr.c
+++ b/src/xenconfig/xen_sxpr.c
@@ -87,15 +87,17 @@ xenParseSxprOS(const struct sexpr *node,
int hvm)
 {
 if (hvm) {
-if (VIR_ALLOC(def->os.loader) < 0)
+if ((VIR_ALLOC(def->os.loader) < 0) ||
+(VIR_ALLOC(def->os.loader->src) < 0))
 goto error;
-if (sexpr_node_copy(node, "domain/image/hvm/loader", 
>os.loader->path) < 0)
+def->os.loader->src->type = VIR_STORAGE_TYPE_FILE;
+if (sexpr_node_copy(node, "domain/image/hvm/loader", 
>os.loader->src->path) < 0)
 goto error;
-if (def->os.loader->path == NULL) {
-if (sexpr_node_copy(node, "domain/image/hvm/kernel", 
>os.loader->path) < 0)
+if (def->os.loader->src->path == NULL) {
+if (sexpr_node_copy(node, "domain/image/hvm/kernel", 
>os.loader->src->path) < 0)
 goto error;
 
-if (def->os.loader->path == NULL) {
+if (def->os.loader->src->path == NULL) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
"%s", _("domain information incomplete, missing 
HVM loader"));
 return -1;
@@ -124,7 +126,8 @@ xenParseSxprOS(const struct sexpr *node,
 /* If HVM kenrel == loader, then old xend, so kill off kernel */
 if (hvm &&
 def->os.kernel &&
-STREQ(def->os.kernel, def->os.loader->path)) {
+def->os.loader->src &&
+STREQ(def->os.kernel, def->os.loader->src->path)) {
 VIR_FREE(def->os.kernel);
 }
 /* Drop kernel argument that has no value */
@@ -2259,9 +2262,9 @@ xenFormatSxpr(virConnectPtr conn, virDomainDefPtr def)
 if (hvm) {
 char bootorder[VIR_DOMAIN_BOOT_LAST+1];
 if (def->os.kernel)
-virBufferEscapeSexpr(, "(loader '%s')", 
def->os.loader->path);
+virBufferEscapeSexpr(, "(loader '%s')", 
def->os.loader->src->path);
 else
-virBufferEscapeSexpr(, "(kernel '%s')", 
def->os.loader->path);
+virBufferEscapeSexpr(, "(kernel '%s')", 
def->os.loader->src->path);
 
 virBufferAsprintf(, "(vcpus %u)", 
virDomainDefGetVcpusMax(def));
 if (virDomainDefHasVcpusOffline(def))
diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c
index 8ef68bb..c6a1f03 100644
--- a/src/xenconfig/xen_xm.c
+++ b/src/xenconfig/xen_xm.c
@@ -47,8 +47,10 @@ xenParseXMOS(virConfPtr conf, virDomainDefPtr def)
 const char *boot;
 
 if (VIR_ALLOC(def->os.loader) < 0 ||
-xenConfigCopyString(conf, "kernel", >os.loader->path) < 0)
+VIR_ALLOC(def->os.loader->src) < 0 ||
+xenConfigCopyString(conf, "kernel", >os.loader->src->path) < 
0)
 return -1;
+def->os.loader->src->type = VIR_STORAGE_TYPE_FILE;
 
 if (xenConfigGetString(conf, "boot", , "c") < 0)
 return -1;
@@ -481,9 +483,10 @@ xenFormatXMOS(virConfPtr conf, virDomainDefPtr def)
 if (xenConfigSetString(conf, "builder", "hvm") < 0)
 return -1;
 
-if (def->os.loader && def->os.loader->path &&
-xenConfigSetString(conf, "kernel", def->os.loader->path) < 0)
+if (def->os.loader && def->os.loader->src &&
+xenConfigSetString(conf, "kernel", def->os.loader->src->path) < 0)
 return -1;
+def->os.loader->src->type = VIR_STORAGE_TYPE_FILE;
 
 for (i = 0; i < def->os.nBootDevs; i++) {
 switch (def->os.bootDevs[i]) {
-- 
1.8.1.2

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

[libvirt] [PATCH 04/12] Format the loader source appropriately.

[Prerna Saxena]

If the initial XML used the old-style declaration as follows:
/path/to/file

we format it as was read.

However, if it used new-style declaration:

  


The formatter identifies that this is a new-style format
and renders it likewise.

Signed-off-by: Prerna Saxena 
---
 src/conf/domain_conf.c | 86 --
 1 file changed, 76 insertions(+), 10 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index be43695..d59a579 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -18094,7 +18094,7 @@ virDomainLoaderNvramDefParseXML(xmlNodePtr node,
 }
 
 loader->nvram->type = VIR_STORAGE_TYPE_LAST;
-loader->nvram->oldStyleNvram = false;
+loader->oldStyleNvram = false;
 
 if ((tmp = virXMLPropString(node, "backing")) &&
 (loader->nvram->type = virStorageTypeFromString(tmp)) <= 0) {
@@ -26212,11 +26212,19 @@ virDomainHugepagesFormat(virBufferPtr buf,
 
 static void
 virDomainLoaderDefFormat(virBufferPtr buf,
- virDomainLoaderDefPtr loader)
+ virDomainLoaderDefPtr loader,
+ unsigned int flags)
 {
 const char *readonly = virTristateBoolTypeToString(loader->readonly);
 const char *secure = virTristateBoolTypeToString(loader->secure);
 const char *type = virDomainLoaderTypeToString(loader->type);
+const char *backing = NULL;
+
+virBuffer attrBuf = VIR_BUFFER_INITIALIZER;
+virBuffer childBuf = VIR_BUFFER_INITIALIZER;
+
+virBufferSetChildIndent(, buf);
+
 
 virBufferAddLit(buf, "secure)
 virBufferAsprintf(buf, " secure='%s'", secure);
 
-virBufferAsprintf(buf, " type='%s'>", type);
+virBufferAsprintf(buf, " type='%s'", type);
+if (loader->src &&
+loader->src->type < VIR_STORAGE_TYPE_LAST) {
+if (!loader->oldStyleLoader) {
+/* Format this in the new style, using the
+ *  sub-element */
+if (virDomainStorageSourceFormat(, , loader->src,
+ flags, 0) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Cannot format loader source"));
+goto cleanup;
+}
+
+backing = virStorageTypeToString(loader->src->type);
+virBufferAsprintf(buf, " backing='%s'>", backing);
+
+if (virXMLFormatElement(buf, "source", , ) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Cannot format loader source"));
+goto cleanup;
+}
+
+} else
+/* Format this in the old-style, using absolute paths directly. */
+virBufferAsprintf(buf, ">%s", loader->src->path);
+} else
+virBufferAddLit(buf, ">\n");
+
+virBufferAddLit(buf, "\n");
 
-virBufferEscapeString(buf, "%s\n", loader->path);
 if (loader->nvram || loader->templt) {
+ignore_value(virBufferContentAndReset());
+ignore_value(virBufferContentAndReset());
+virBufferSetChildIndent(, buf);
+
 virBufferAddLit(buf, "templt);
-if (loader->nvram)
-virBufferEscapeString(buf, ">%s\n", loader->nvram);
-else
-virBufferAddLit(buf, "/>\n");
+
+if (loader->templt)
+virBufferEscapeString(buf, " template='%s'", loader->templt);
+
+if (loader->nvram) {
+backing = virStorageTypeToString(loader->nvram->type);
+if (!loader->oldStyleNvram) {
+if (virDomainStorageSourceFormat(, ,
+ loader->nvram, flags, 0) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Cannot format NVRAM source"));
+virBufferAddLit(buf, ">\n\n");
+goto cleanup;
+}
+
+virBufferEscapeString(buf, " backing='%s'>", backing);
+if (virXMLFormatElement(buf, "source", , ) < 
0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Cannot format NVRAM source"));
+virBufferAddLit(buf, "\n");
+goto cleanup;
+}
+} else {
+/* old-style NVRAM declaration found */
+virBufferAsprintf(buf, ">%s", loader->nvram->path);
+}
+virBufferAddLit(buf, "\n\n");
+}
 }
+cleanup:
+virBufferFreeAndReset();
+virBufferFreeAndReset();
+return;
 }
 
 static void
@@ -26899,7 +26965,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
 virBufferAsprintf(buf, "%s\n", 
def->os.initgroup);
 
 if (def->os.loader)
-

[libvirt] [PATCH 08/12] virt-aa-helper: Adjust references to loader & nvram elements to correctly parse the virStorageSource types.

[Prerna Saxena]

Signed-off-by: Prerna Saxena 
---
 src/security/virt-aa-helper.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index d0f9876..8217d67 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -1063,12 +1063,18 @@ get_files(vahControl * ctl)
 if (vah_add_file(, ctl->def->os.slic_table, "r") != 0)
 goto cleanup;
 
-if (ctl->def->os.loader && ctl->def->os.loader->path)
-if (vah_add_file(, ctl->def->os.loader->path, "rk") != 0)
+if (ctl->def->os.loader &&
+ctl->def->os.loader->src &&
+ctl->def->os.loader->src->type == VIR_STORAGE_TYPE_FILE &&
+ctl->def->os.loader->src->path)
+if (vah_add_file(, ctl->def->os.loader->src->path, "rk") != 0)
 goto cleanup;
 
-if (ctl->def->os.loader && ctl->def->os.loader->nvram)
-if (vah_add_file(, ctl->def->os.loader->nvram, "rwk") != 0)
+if (ctl->def->os.loader &&
+ctl->def->os.loader->nvram &&
+ctl->def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE &&
+ctl->def->os.loader->nvram->path)
+if (vah_add_file(, ctl->def->os.loader->nvram->path, "rwk") != 0)
 goto cleanup;
 
 for (i = 0; i < ctl->def->ngraphics; i++) {
-- 
1.8.1.2

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

[libvirt] [PATCH 00/12][v2] Introduce network-backed loader & nvram.

[Prerna Saxena]

Libvirt domain XML today only allows local filepaths that can be
used to specify a loader element or its matching NVRAM disk.
Given that Vms may themselves move across hypervisor hosts, it should be
possible to allocate loaders/NVRAM disks on network storage for
uninterrupted access.

This series extends the firmware loader & NVRAM disks to be hosted over
network-backed disks, for high availability.
It achieves this by embedding virStorageSource elements for loader &
nvram into _virDomainLoaderDef, as discussed in
https://www.redhat.com/archives/libvir-list/2018-March/msg01721.html.

Currently, the source type is annotated by introducing a new attribute 
"backing" for both
'loader' and 'nvram' elements. Hence, a sample XML with new annotation looks 
like this:


  

  
  


References:
--
v0 / Proposal: 
https://www.redhat.com/archives/libvir-list/2018-March/msg01721.html.
v1 : https://www.redhat.com/archives/libvir-list/2018-April/msg02024.html 

Changelog:
-
Changes since v1:
1. Split up the patch into smaller units.
2. Added doc snippet & tests.
3. Changed the formatting code in patch 4 to format a domain's XML in
the style that was read.

I found that encryption seems to be a property of the storage volume.
I didnt  include that in this series since it does not use backing type
as volume. Will include that later once the basic network support
patches get done.

Looking forward to feedback,
Prerna

Prerna Saxena (12):
  Schema: Introduce XML schema for network-backed loader and nvram
elements.
  Loader: Add a more elaborate definition.
  Parse domain XML to generate virDomainLoaderDef & virDomainNvramDef.
  Format the loader source appropriately.
  Plumb the loader source into generation of QEMU command line.
  Fix the domain def inference logic to correctly account for
network-backed pflash devices.
  Bhyve: Fix command line generation to correctly pick up local loader
path.
  virt-aa-helper: Adjust references to loader & nvram elements to
correctly parse the virStorageSource types.
  Vbox: Adjust references to 'loader' and 'nvram' elements given that   
 these are now represented by virStorageSourcePtr.
  Xen: Adjust all references to loader & nvram elements given that they
are now backed by virStorageSourcePtr
  Test: Add a test snippet to evaluate command line generation for
loader/nvram specified via virStorageSource
  Documentation: Add a blurb for the newly added XML snippets for loader
and nvram.

 docs/formatdomain.html.in  |  36 -
 docs/schemas/domaincommon.rng  | 108 ++---
 src/bhyve/bhyve_command.c  |   6 +-
 src/conf/domain_conf.c | 215 +++--
 src/conf/domain_conf.h |  11 +-
 src/qemu/qemu_cgroup.c |  13 +-
 src/qemu/qemu_command.c|  21 ++-
 src/qemu/qemu_domain.c |  31 ++--
 src/qemu/qemu_driver.c |   7 +-
 src/qemu/qemu_parse_command.c  |  30 +++-
 src/qemu/qemu_process.c|  42 +++--
 src/security/security_dac.c|   6 +-
 src/security/security_selinux.c|   6 +-
 src/security/virt-aa-helper.c  |  14 +-
 src/vbox/vbox_common.c |  11 +-
 src/xenapi/xenapi_driver.c |   4 +-
 src/xenconfig/xen_sxpr.c   |  19 ++-
 src/xenconfig/xen_xm.c |   9 +-
 tests/qemuxml2argvdata/bios-nvram-network.args |  31 
 tests/qemuxml2argvdata/bios-nvram-network.xml  |  42 +
 tests/qemuxml2argvtest.c   |   1 +
 21 files changed, 562 insertions(+), 101 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.args
 create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.xml

-- 
1.8.1.2

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

[libvirt] [PATCH 03/12] Parse domain XML to generate virDomainLoaderDef & virDomainNvramDef.

[Prerna Saxena]

This patch is used to interpret domain XML and store the Loader &
Nvram's backing definitions as virStorageSource.
It also identifies if input XML used old or new-style declaration.
(This will later be used by the formatter).

Signed-off-by: Prerna Saxena 
---
 src/conf/domain_conf.c | 131 ++---
 1 file changed, 124 insertions(+), 7 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f678e26..be43695 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2884,8 +2884,8 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr loader)
 if (!loader)
 return;
 
-VIR_FREE(loader->path);
-VIR_FREE(loader->nvram);
+virStorageSourceFree(loader->src);
+virStorageSourceFree(loader->nvram);
 VIR_FREE(loader->templt);
 VIR_FREE(loader);
 }
@@ -17986,17 +17986,62 @@ 
virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
 
 static int
 virDomainLoaderDefParseXML(xmlNodePtr node,
+   xmlXPathContextPtr ctxt,
virDomainLoaderDefPtr loader)
 {
 int ret = -1;
 char *readonly_str = NULL;
 char *secure_str = NULL;
 char *type_str = NULL;
+char *tmp = NULL;
+xmlNodePtr cur;
+xmlXPathContextPtr cur_ctxt = ctxt;
+
+if (VIR_ALLOC(loader->src)) {
+goto cleanup;
+}
+loader->src->type = VIR_STORAGE_TYPE_LAST;
+loader->oldStyleLoader = false;
 
 readonly_str = virXMLPropString(node, "readonly");
 secure_str = virXMLPropString(node, "secure");
 type_str = virXMLPropString(node, "type");
-loader->path = (char *) xmlNodeGetContent(node);
+
+if ((tmp = virXMLPropString(node, "backing")) &&
+(loader->src->type = virStorageTypeFromString(tmp)) <= 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("unknown loader type '%s'"), tmp);
+goto cleanup;
+}
+VIR_FREE(tmp);
+
+for (cur = node->children; cur != NULL; cur = cur->next) {
+if (cur->type != XML_ELEMENT_NODE) {
+continue;
+}
+
+if (virXMLNodeNameEqual(cur, "source")) {
+/* new-style declaration found */
+if (virDomainStorageSourceParse(cur, cur_ctxt, loader->src, 0) < 
0) {
+virReportError(VIR_ERR_XML_DETAIL,
+   _("Error parsing Loader source element"));
+goto cleanup;
+}
+break;
+}
+}
+
+/* Old-style absolute path found ? */
+if (loader->src->type == VIR_STORAGE_TYPE_LAST) {
+if (!(loader->src->path = (char *) xmlNodeGetContent(node))) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("missing loader source"));
+goto cleanup;
+} else {
+loader->src->type = VIR_STORAGE_TYPE_FILE;
+loader->oldStyleLoader = true;
+}
+}
 
 if (readonly_str &&
 (loader->readonly = virTristateBoolTypeFromString(readonly_str)) <= 0) 
{
@@ -18023,13 +18068,81 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
 }
 
 ret = 0;
- cleanup:
+goto exit;
+cleanup:
+if (loader->src)
+  VIR_FREE(loader->src);
+exit:
 VIR_FREE(readonly_str);
 VIR_FREE(secure_str);
 VIR_FREE(type_str);
+
 return ret;
 }
 
+static int
+virDomainLoaderNvramDefParseXML(xmlNodePtr node,
+   xmlXPathContextPtr ctxt,
+   virDomainLoaderDefPtr loader)
+{
+int ret = -1;
+char *tmp = NULL;
+xmlNodePtr cur;
+
+if (VIR_ALLOC(loader->nvram)) {
+goto cleanup;
+}
+
+loader->nvram->type = VIR_STORAGE_TYPE_LAST;
+loader->nvram->oldStyleNvram = false;
+
+if ((tmp = virXMLPropString(node, "backing")) &&
+(loader->nvram->type = virStorageTypeFromString(tmp)) <= 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("unknown nvram type '%s'"), tmp);
+goto cleanup;
+}
+VIR_FREE(tmp);
+
+for (cur = node->children; cur != NULL; cur = cur->next) {
+if (cur->type != XML_ELEMENT_NODE) {
+continue;
+}
+
+if (virXMLNodeNameEqual(cur, "template")) {
+loader->templt = virXPathString("string(./os/nvram[1]/@template)", 
ctxt);
+continue;
+}
+
+if (virXMLNodeNameEqual(cur, "source")) {
+if (virDomainStorageSourceParse(cur, ctxt, loader->nvram, 0) < 0) {
+virReportError(VIR_ERR_XML_DETAIL,
+   _("Error parsing nvram source element"));
+goto cleanup;
+}
+ret = 0;
+}
+}
+
+/* Old-style declaration found */
+if (loader->nvram->type == VIR_STORAGE_TYPE_LAST) {
+if (!(loader->nvram->path = (char *) xmlNodeGetContent(node))) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("missing 

[libvirt] [PATCH 12/12] Documentation: Add a blurb for the newly added XML snippets for loader and nvram.

[Prerna Saxena]

Signed-off-by: Prerna Saxena 
---
 docs/formatdomain.html.in | 36 +---
 1 file changed, 33 insertions(+), 3 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index caeb14e..b8cb7ac 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -112,6 +112,20 @@
 /os
 ...
 
+Where loader/NVRAM can also be described as:
+
+
+...
+  loader readonly='yes' secure='no' type='rom' backing='file'
+source file='/usr/share/OVMF/OVMF_CODE.fd'/
+  /loader
+  nvram backing='network' template='/usr/share/OVMF/OVMF_VARS.fd'
+source protocol='iscsi' 
name='iqn.2013-07.com.example:iscsi-nopool/2'
+  host name='example.com' port='3260'/
+/source
+  /nvram
+...
+
 
   type
   The content of the type element specifies the
@@ -143,7 +157,15 @@
 pflash. Moreover, some firmwares may
 implement the Secure boot feature. Attribute
 secure can be used then to control it.
-Since 2.1.0
+Since 2.1.0Since 
4.5.0
+Loader element can also be specified as a remote disk. The attribute
+backing (accepted values are
+file and network)can be used to represent
+local absolute paths as well as network-backed disks. The details of
+backing file may be specified as storage 
source
+Allowed backing type file replaces the old
+specification and extends to all hypervisors that supported it, while
+type network is only supported by QEMU.
   nvram
   Some UEFI firmwares may want to use a non-volatile memory to store
 some variables. In the host, this is represented as a file and the
@@ -154,7 +176,15 @@
 from the config file. Note, that for transient domains if the NVRAM 
file
 has been created by libvirt it is left behind and it is management
 application's responsibility to save and remove file (if needed to be
-persistent). Since 1.2.8
+persistent). Since 1.2.8
+Since 4.5.0, attribute
+backing(accepted values file
+and network)can be used to specify a
+storage source
+of type file or network. The value filedescribes
+absolute NVRAM paths and extends the present specification
+for all hypervisors that support this, while
+network applies only to QEMU.
   boot
   The dev attribute takes one of the values "fd", "hd",
 "cdrom" or "network" and is used to specify the next boot device
@@ -2707,7 +2737,7 @@
 
 
   
-  source
+  Disk source
   Representation of the disk source depends on the
   disk type attribute value as follows:
   
-- 
1.8.1.2

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

[libvirt] [PATCH 02/12] Loader: Add a more elaborate definition.

[Prerna Saxena]

Augment definition to include virStorageSourcePtr that
more comprehensively describes the nature of backing element.
Also include flags for annotating if input XML definition is
old-style or new-style.

Signed-off-by: Prerna Saxena 
---
 src/conf/domain_conf.h | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 15d228b..bbd021b 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1855,15 +1855,22 @@ typedef enum {
 
 VIR_ENUM_DECL(virDomainLoader)
 
+struct _virStorageSource;
+typedef struct _virStorageSource* virStorageSourcePtr;
+
 typedef struct _virDomainLoaderDef virDomainLoaderDef;
 typedef virDomainLoaderDef *virDomainLoaderDefPtr;
 struct _virDomainLoaderDef {
-char *path;
+virStorageSourcePtr src;
 int readonly;   /* enum virTristateBool */
 virDomainLoader type;
 int secure; /* enum virTristateBool */
-char *nvram;/* path to non-volatile RAM */
+virStorageSourcePtr nvram;  /* path to non-voliatile RAM */
 char *templt;   /* user override of path to master nvram */
+bool oldStyleLoader;  /* is this an old-style XML formatting,
+   * ie, absolute path is directly specified? */
+bool oldStyleNvram;   /* is this an old-style XML formatting,
+   * ie, absolute path is directly specified? */
 };
 
 void virDomainLoaderDefFree(virDomainLoaderDefPtr loader);
-- 
1.8.1.2

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

[libvirt] [PATCH 12/13] util: storage: Store PR manager alias in the definition

[Peter Krempa]

Rather than always re-generating the alias store it in the definition
and in the status XML.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_command.c   | 23 +++--
 src/qemu/qemu_command.h   |  3 +--
 src/qemu/qemu_domain.c| 16 +--
 src/qemu/qemu_hotplug.c   | 34 ++-
 src/util/virstoragefile.c |  1 +
 src/util/virstoragefile.h |  3 +++
 tests/qemustatusxml2xmldata/modern-in.xml |  4 
 7 files changed, 37 insertions(+), 47 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c38dde5a60..84d7d51c7c 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9669,7 +9669,6 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
  * qemuBuildPRManagerInfoProps:
  * @disk: disk definition
  * @propsret: Returns JSON object containing properties of the 
pr-manager-helper object
- * @aliasret: alias of the pr-manager-helper object
  *
  * Build the JSON properties for the pr-manager object.
  *
@@ -9678,32 +9677,19 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
  */
 int
 qemuBuildPRManagerInfoProps(const virDomainDiskDef *disk,
-virJSONValuePtr *propsret,
-char **aliasret)
+virJSONValuePtr *propsret)
 {
-char *alias = NULL;
 int ret = -1;

 *propsret = NULL;
-*aliasret = NULL;
-
-if (virStoragePRDefIsManaged(disk->src->pr)) {
-if (VIR_STRDUP(alias, qemuDomainGetManagedPRAlias()) < 0)
-goto cleanup;
-} else {
-if (!(alias = qemuDomainGetUnmanagedPRAlias(disk->info.alias)))
-goto cleanup;
-}

 if (virJSONValueObjectCreate(propsret,
  "s:path", disk->src->pr->path,
  NULL) < 0)
 goto cleanup;

-VIR_STEAL_PTR(*aliasret, alias);
 ret = 0;
  cleanup:
-VIR_FREE(alias);
 return ret;
 }

@@ -9715,7 +9701,6 @@ qemuBuildMasterPRCommandLine(virCommandPtr cmd,
 size_t i;
 bool managedAdded = false;
 virJSONValuePtr props = NULL;
-char *alias = NULL;
 char *tmp = NULL;
 int ret = -1;

@@ -9732,14 +9717,13 @@ qemuBuildMasterPRCommandLine(virCommandPtr cmd,
 managedAdded = true;
 }

-if (qemuBuildPRManagerInfoProps(disk, , ) < 0)
+if (qemuBuildPRManagerInfoProps(disk, ) < 0)
 goto cleanup;

 if (!(tmp = virQEMUBuildObjectCommandlineFromJSON("pr-manager-helper",
-  alias,
+  
disk->src->pr->mgralias,
   props)))
 goto cleanup;
-VIR_FREE(alias);
 virJSONValueFree(props);
 props = NULL;

@@ -9749,7 +9733,6 @@ qemuBuildMasterPRCommandLine(virCommandPtr cmd,

 ret = 0;
  cleanup:
-VIR_FREE(alias);
 virJSONValueFree(props);
 return ret;
 }
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index 621592cd79..28bc33558b 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -56,8 +56,7 @@ virCommandPtr qemuBuildCommandLine(virQEMUDriverPtr driver,

 /* Generate the object properties for pr-manager */
 int qemuBuildPRManagerInfoProps(const virDomainDiskDef *disk,
-virJSONValuePtr *propsret,
-char **alias);
+virJSONValuePtr *propsret);

 /* Generate the object properties for a secret */
 int qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo,
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 92217e66fe..1572ce5c2d 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1941,6 +1941,9 @@ qemuStorageSourcePrivateDataParse(xmlXPathContextPtr ctxt,
 src->nodestorage = 
virXPathString("string(./nodenames/nodename[@type='storage']/@name)", ctxt);
 src->nodeformat = 
virXPathString("string(./nodenames/nodename[@type='format']/@name)", ctxt);

+if (src->pr)
+src->pr->mgralias = virXPathString("string(./reservations/@mgralias)", 
ctxt);
+
 if (virStorageSourcePrivateDataParseRelPath(ctxt, src) < 0)
 return -1;

@@ -1961,6 +1964,9 @@ qemuStorageSourcePrivateDataFormat(virStorageSourcePtr 
src,
 virBufferAddLit(buf, "\n");
 }

+if (src->pr)
+virBufferAsprintf(buf, "\n", 
src->pr->mgralias);
+
 if (virStorageSourcePrivateDataFormatRelPath(src, buf) < 0)
 return -1;

@@ -11932,7 +11938,8 @@ qemuDomainPrepareDiskCachemode(virDomainDiskDefPtr disk)

 static int
 qemuDomainPrepareStorageSourcePR(virStorageSourcePtr src,
- qemuDomainObjPrivatePtr priv)
+ qemuDomainObjPrivatePtr priv,
+ const 

[libvirt] [PATCH 10/13] qemu: command: Move check whether PR manager object props need to be built

[Peter Krempa]

Move it out of the formatter function and let the caller decide this.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_command.c | 9 +++--
 src/qemu/qemu_hotplug.c | 3 +++
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 7df9979cb2..c38dde5a60 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9687,9 +9687,6 @@ qemuBuildPRManagerInfoProps(const virDomainDiskDef *disk,
 *propsret = NULL;
 *aliasret = NULL;

-if (!disk->src->pr)
-return 0;
-
 if (virStoragePRDefIsManaged(disk->src->pr)) {
 if (VIR_STRDUP(alias, qemuDomainGetManagedPRAlias()) < 0)
 goto cleanup;
@@ -9725,6 +9722,9 @@ qemuBuildMasterPRCommandLine(virCommandPtr cmd,
 for (i = 0; i < def->ndisks; i++) {
 const virDomainDiskDef *disk = def->disks[i];

+if (!disk->src->pr)
+continue;
+
 if (virStoragePRDefIsManaged(disk->src->pr)) {
 if (managedAdded)
 continue;
@@ -9735,9 +9735,6 @@ qemuBuildMasterPRCommandLine(virCommandPtr cmd,
 if (qemuBuildPRManagerInfoProps(disk, , ) < 0)
 goto cleanup;

-if (!props)
-continue;
-
 if (!(tmp = virQEMUBuildObjectCommandlineFromJSON("pr-manager-helper",
   alias,
   props)))
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 3a26876c10..6557711ec1 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -395,6 +395,9 @@ qemuMaybeBuildPRManagerInfoProps(virDomainObjPtr vm,
 *propsret = NULL;
 *aliasret = NULL;

+if (!disk->src->pr)
+return 0;
+
 if (virStoragePRDefIsManaged(disk->src->pr) &&
 priv->prDaemonRunning) {
 /* @disk requires qemu-pr-helper but there's already one running. */
-- 
2.16.2

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

[libvirt] [PATCH 13/13] qemu: hotplug: Replace qemuDomainDiskNeedRemovePR

[Peter Krempa]

The function can be replaced by much simpler logic.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_hotplug.c | 44 +++-
 1 file changed, 3 insertions(+), 41 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 9481123c19..96042bc06c 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3826,42 +3826,6 @@ static bool qemuIsMultiFunctionDevice(virDomainDefPtr 
def,
 }


-static int
-qemuDomainDiskNeedRemovePR(virDomainObjPtr vm,
-   virDomainDiskDefPtr disk,
-   bool *stopDaemon)
-{
-qemuDomainObjPrivatePtr priv = vm->privateData;
-size_t i;
-
-*stopDaemon = false;
-
-if (!disk->src->pr)
-return 0;
-
-if (!virStoragePRDefIsManaged(disk->src->pr))
-return 0;
-
-for (i = 0; i < vm->def->ndisks; i++) {
-const virDomainDiskDef *domainDisk = vm->def->disks[i];
-
-if (domainDisk == disk)
-continue;
-
-if (virStoragePRDefIsManaged(domainDisk->src->pr))
-break;
-}
-
-if (i != vm->def->ndisks)
-return 0;
-
-if (priv->prDaemonRunning)
-*stopDaemon = true;
-
-return 0;
-}
-
-
 static int
 qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
@@ -3875,7 +3839,6 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
 char *drivestr;
 char *objAlias = NULL;
 char *encAlias = NULL;
-bool stopPRDaemon = false;

 VIR_DEBUG("Removing disk %s from domain %p %s",
   disk->info.alias, vm, vm->def->name);
@@ -3913,9 +3876,6 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
 }
 }

-if (qemuDomainDiskNeedRemovePR(vm, disk, ) < 0)
-return -1;
-
 qemuDomainObjEnterMonitor(driver, vm);

 qemuMonitorDriveDel(priv->mon, drivestr);
@@ -3953,7 +3913,9 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
 }
 }

-if (stopPRDaemon)
+/* check if the last disk with managed PR was just removed */
+if (priv->prDaemonRunning &&
+!virDomainDefHasManagedPR(vm->def))
 qemuProcessKillManagedPRDaemon(vm);

 qemuDomainReleaseDeviceAddress(vm, >info, src);
-- 
2.16.2

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

[libvirt] [PATCH 11/13] conf: domain: Add helper to check whether a domain def requires use of PR

[Peter Krempa]

Extract the lookup code so that it can be reused later.

Signed-off-by: Peter Krempa 
---
 src/conf/domain_conf.c   | 21 +
 src/conf/domain_conf.h   |  3 +++
 src/libvirt_private.syms |  1 +
 src/qemu/qemu_process.c  | 23 ++-
 4 files changed, 27 insertions(+), 21 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 86229db654..6f16e4ade4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -29528,3 +29528,24 @@ virDomainDiskGetDetectZeroesMode(virDomainDiskDiscard 
discard,

 return detect_zeroes;
 }
+
+
+/**
+ * virDomainDefHasManagedPR:
+ * @def: domain definition
+ *
+ * Returns true if any of the domain disks requires the use of the managed
+ * persistent reservations infrastructure.
+ */
+bool
+virDomainDefHasManagedPR(const virDomainDef *def)
+{
+size_t i;
+
+for (i = 0; i < def->ndisks; i++) {
+if (virStoragePRDefIsManaged(def->disks[i]->src->pr))
+return true;
+}
+
+return false;
+}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 07d04fb2f9..f1add06155 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3543,4 +3543,7 @@ int
 virDomainDiskGetDetectZeroesMode(virDomainDiskDiscard discard,
  virDomainDiskDetectZeroes detect_zeroes);

+bool
+virDomainDefHasManagedPR(const virDomainDef *def);
+
 #endif /* __DOMAIN_CONF_H */
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index dd10be9753..a0b78f72ba 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -275,6 +275,7 @@ virDomainDefGetVcpus;
 virDomainDefGetVcpusMax;
 virDomainDefGetVcpusTopology;
 virDomainDefHasDeviceAddress;
+virDomainDefHasManagedPR;
 virDomainDefHasMemballoon;
 virDomainDefHasMemoryHotplug;
 virDomainDefHasUSB;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index af29bcc59e..5b73a61962 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2748,26 +2748,6 @@ qemuProcessStartManagedPRDaemon(virDomainObjPtr vm)
 }


-static int
-qemuProcessMaybeStartManagedPRDaemon(virDomainObjPtr vm)
-{
-bool hasManaged = false;
-size_t i;
-
-for (i = 0; i < vm->def->ndisks; i++) {
-if (virStoragePRDefIsManaged(vm->def->disks[i]->src->pr)) {
-hasManaged = true;
-break;
-}
-}
-
-if (!hasManaged)
-return 0;
-
-return qemuProcessStartManagedPRDaemon(vm);
-}
-
-
 static int
 qemuProcessInitPasswords(virQEMUDriverPtr driver,
  virDomainObjPtr vm,
@@ -6285,7 +6265,8 @@ qemuProcessLaunch(virConnectPtr conn,
 goto cleanup;

 VIR_DEBUG("Setting up managed PR daemon");
-if (qemuProcessMaybeStartManagedPRDaemon(vm) < 0)
+if (virDomainDefHasManagedPR(vm->def) &&
+qemuProcessStartManagedPRDaemon(vm) < 0)
 goto cleanup;

 VIR_DEBUG("Setting domain security labels");
-- 
2.16.2

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

[libvirt] [PATCH 09/13] qemu: process: Change semantics of functions starting PR daemon

[Peter Krempa]

Libvirt only manages one PR daemon. This means that we don't need to
pass the 'disk' object and also rename the functions dealing with this
so that it's obvious we only deal with the managed PR daemon.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_hotplug.c |  6 +++---
 src/qemu/qemu_process.c | 37 -
 src/qemu/qemu_process.h |  5 ++---
 3 files changed, 21 insertions(+), 27 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 77d37e5ef6..3a26876c10 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -377,7 +377,7 @@ qemuDomainMaybeStartPRDaemon(virDomainObjPtr vm,

 /* @disk requires qemu-pr-helper but none is running.
  * Start it now. */
-if (qemuProcessStartPRDaemon(vm, disk) < 0)
+if (qemuProcessStartManagedPRDaemon(vm) < 0)
 return -1;

 return 1;
@@ -567,7 +567,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
 qemuDomainDelDiskSrcTLSObject(driver, vm, disk->src);
 ignore_value(qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, true));
 if (prdStarted)
-qemuProcessKillPRDaemon(vm);
+qemuProcessKillManagedPRDaemon(vm);
 goto cleanup;
 }

@@ -3963,7 +3963,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
 }

 if (stopPRDaemon)
-qemuProcessKillPRDaemon(vm);
+qemuProcessKillManagedPRDaemon(vm);

 qemuDomainReleaseDeviceAddress(vm, >info, src);

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 42b91b39ac..af29bcc59e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2566,7 +2566,7 @@ qemuProcessBuildPRHelperPidfilePath(virDomainObjPtr vm)


 void
-qemuProcessKillPRDaemon(virDomainObjPtr vm)
+qemuProcessKillManagedPRDaemon(virDomainObjPtr vm)
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
 virErrorPtr orig_err;
@@ -2624,8 +2624,7 @@ qemuProcessStartPRDaemonHook(void *opaque)


 int
-qemuProcessStartPRDaemon(virDomainObjPtr vm,
- const virDomainDiskDef *disk)
+qemuProcessStartManagedPRDaemon(virDomainObjPtr vm)
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
 virQEMUDriverPtr driver = priv->driver;
@@ -2640,10 +2639,6 @@ qemuProcessStartPRDaemon(virDomainObjPtr vm,
 const unsigned long long timeout = 50; /* ms */
 int ret = -1;

-if (!virStoragePRDefIsManaged(disk->src->pr) ||
-priv->prDaemonRunning)
-return 0;
-
 cfg = virQEMUDriverGetConfig(driver);

 if (!virFileIsExecutable(cfg->prHelperName)) {
@@ -2734,7 +2729,7 @@ qemuProcessStartPRDaemon(virDomainObjPtr vm,
 goto cleanup;

 priv->prDaemonRunning = true;
-ret = 1;
+ret = 0;
  cleanup:
 if (ret < 0) {
 virCommandAbort(cmd);
@@ -2754,22 +2749,22 @@ qemuProcessStartPRDaemon(virDomainObjPtr vm,


 static int
-qemuProcessMaybeStartPRDaemon(virDomainObjPtr vm)
+qemuProcessMaybeStartManagedPRDaemon(virDomainObjPtr vm)
 {
+bool hasManaged = false;
 size_t i;
-int rv;

 for (i = 0; i < vm->def->ndisks; i++) {
-const virDomainDiskDef *disk = vm->def->disks[i];
-
-if ((rv = qemuProcessStartPRDaemon(vm, disk)) < 0)
-return -1;
-
-if (rv > 0)
-return 1;
+if (virStoragePRDefIsManaged(vm->def->disks[i]->src->pr)) {
+hasManaged = true;
+break;
+}
 }

-return 0;
+if (!hasManaged)
+return 0;
+
+return qemuProcessStartManagedPRDaemon(vm);
 }


@@ -6289,8 +6284,8 @@ qemuProcessLaunch(virConnectPtr conn,
 if (qemuProcessResctrlCreate(driver, vm) < 0)
 goto cleanup;

-VIR_DEBUG("Setting up PR daemon");
-if (qemuProcessMaybeStartPRDaemon(vm) < 0)
+VIR_DEBUG("Setting up managed PR daemon");
+if (qemuProcessMaybeStartManagedPRDaemon(vm) < 0)
 goto cleanup;

 VIR_DEBUG("Setting domain security labels");
@@ -6821,7 +6816,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
 qemuDomainMasterKeyRemove(priv);

 /* Do this before we delete the tree and remove pidfile. */
-qemuProcessKillPRDaemon(vm);
+qemuProcessKillManagedPRDaemon(vm);

 virFileDeleteTree(priv->libDir);
 virFileDeleteTree(priv->channelTargetDir);
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index eda6695415..a0e34b1c85 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -205,9 +205,8 @@ int qemuProcessRefreshDisks(virQEMUDriverPtr driver,
 virDomainObjPtr vm,
 qemuDomainAsyncJob asyncJob);

-int qemuProcessStartPRDaemon(virDomainObjPtr vm,
- const virDomainDiskDef *disk);
+int qemuProcessStartManagedPRDaemon(virDomainObjPtr vm);

-void qemuProcessKillPRDaemon(virDomainObjPtr vm);
+void qemuProcessKillManagedPRDaemon(virDomainObjPtr vm);

 #endif /* __QEMU_PROCESS_H__ */
-- 
2.16.2

--
libvir-list mailing list
libvir-list@redhat.com

[libvirt] [PATCH 05/13] util: storage: Drop pointless 'enabled' form PR definition

[Peter Krempa]

Everything can be disabled by not using the parent element. There's no
need to store this explicitly. Additionally it does not add any value
since any configuration is dropped if enabled='no' is configured.

Drop the attribute and adjust the code accordingly.t

Signed-off-by: Peter Krempa 
---
 docs/formatdomain.html.in  |  21 ++--
 docs/schemas/storagecommon.rng |   3 -
 src/util/virstoragefile.c  | 117 +
 src/util/virstoragefile.h  |   1 -
 .../disk-virtio-scsi-reservations.xml  |   4 +-
 5 files changed, 59 insertions(+), 87 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 80172c18d0..d69a669259 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2583,7 +2583,7 @@
   disk type='block' device='lun'
 driver name='qemu' type='raw'/
 source dev='/dev/sda'
-  reservations enabled='yes' managed='no'
+  reservations managed='no'
 source type='unix' path='/path/to/qemu-pr-helper' 
mode='client'/
   /reservations
 target dev='sda' bus='scsi'/
@@ -2952,17 +2952,16 @@
   Since libvirt 4.4.0, the
 reservations can be a sub-element of the
 source element for storage sources (QEMU driver only).
-If present (and enabled) it enables persistent reservations for 
SCSI
+If present it enables persistent reservations for SCSI
 based disks. The element has one mandatory attribute
-enabled with accepted values yes and
-no. If the feature is enabled, then there's another
-mandatory attribute managed (accepted values are the
-same as for enabled) that enables or disables libvirt
-spawning a helper process. When the PR is unmanaged, then 
hypervisor
-acts as a client and path to server socket must be provided in 
child
-element source, which currently accepts only the
-following attributes: type with one value
-unix, path with path the socket, and
+managed with accepted values yes and
+no. If managed is enabled libvirt 
prepares
+and manages any resources needed for the feature. When the PR is
+unmanaged, then hypervisor acts as a client and path to server
+socket must be provided in child element source,
+which currently accepts only the following attributes:
+type with one value unix,
+path with path the socket, and
 finally mode which accepts one value
 client and specifies the role of hypervisor.
 It's recommended to allow libvirt manage the persistent
diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng
index eed0b33347..cb4f14f52f 100644
--- a/docs/schemas/storagecommon.rng
+++ b/docs/schemas/storagecommon.rng
@@ -75,9 +75,6 @@

   
 
-  
-
-  
   
 
   
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 87c3499561..d6907e47bb 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1906,8 +1906,8 @@ virStoragePRDefFree(virStoragePRDefPtr prd)
 virStoragePRDefPtr
 virStoragePRDefParseXML(xmlXPathContextPtr ctxt)
 {
-virStoragePRDefPtr prd, ret = NULL;
-char *enabled = NULL;
+virStoragePRDefPtr prd;
+virStoragePRDefPtr ret = NULL;
 char *managed = NULL;
 char *type = NULL;
 char *path = NULL;
@@ -1916,81 +1916,65 @@ virStoragePRDefParseXML(xmlXPathContextPtr ctxt)
 if (VIR_ALLOC(prd) < 0)
 return NULL;

-if (!(enabled = virXPathString("string(./@enabled)", ctxt))) {
+if (!(managed = virXPathString("string(./@managed)", ctxt))) {
 virReportError(VIR_ERR_XML_ERROR, "%s",
-   _("missing @enabled attribute for "));
+   _("missing @managed attribute for "));
 goto cleanup;
 }

-if ((prd->enabled = virTristateBoolTypeFromString(enabled)) <= 0) {
+if ((prd->managed = virTristateBoolTypeFromString(managed)) <= 0) {
 virReportError(VIR_ERR_XML_ERROR,
-   _("invalid value for 'enabled': %s"), enabled);
+   _("invalid value for 'managed': %s"), managed);
 goto cleanup;
 }

-if (prd->enabled == VIR_TRISTATE_BOOL_YES) {
-if (!(managed = virXPathString("string(./@managed)", ctxt))) {
+if (prd->managed == VIR_TRISTATE_BOOL_NO) {
+type = virXPathString("string(./source[1]/@type)", ctxt);
+path = virXPathString("string(./source[1]/@path)", ctxt);
+mode = virXPathString("string(./source[1]/@mode)", ctxt);
+
+if (!type) {
 virReportError(VIR_ERR_XML_ERROR, "%s",
-   _("missing @managed attribute for 
"));
+   _("missing 

[libvirt] [PATCH 03/13] qemu: command: Fix comment for qemuBuildPRManagerInfoProps

[Peter Krempa]

The comment did not accurately describe the arguments.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_command.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 24b482efd8..d84cf6dffc 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9667,8 +9667,10 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,

 /**
  * qemuBuildPRManagerInfoProps:
- * @prd: disk PR runtime info
- * @propsret: JSON properties to return
+ * @vm: domain object
+ * @disk: disk definition
+ * @propsret: Returns JSON object containing properties of the 
pr-manager-helper object
+ * @aliasret: alias of the pr-manager-helper object
  *
  * Build the JSON properties for the pr-manager object.
  *
-- 
2.16.2

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

[libvirt] [PATCH 07/13] util: storage: Allow passing also for managed PR case

[Peter Krempa]

To allow storing status information in the XML move the validation that
the 'path' is not valid for managed PR daemon case into
qemuDomainValidateStorageSource and allow parsing of the data even in
case when managed='yes'.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_domain.c| 18 +-
 src/util/virstoragefile.c | 37 ++---
 2 files changed, 31 insertions(+), 24 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c8d2daa26f..eaa796260c 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4204,11 +4204,19 @@ qemuDomainValidateStorageSource(virStorageSourcePtr src,
 }
 }

-if (src->pr &&
-!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PR_MANAGER_HELPER)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("reservations not supported with this QEMU binary"));
-return -1;
+if (src->pr) {
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PR_MANAGER_HELPER)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("reservations not supported with this QEMU 
binary"));
+return -1;
+}
+
+if (virStoragePRDefIsManaged(src->pr) && src->pr->path) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("'path' attribute should not be provided for "
+ "managed reservations"));
+return -1;
+}
 }

 return 0;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index c89bdb9e49..dbbe758f30 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1928,11 +1928,11 @@ virStoragePRDefParseXML(xmlXPathContextPtr ctxt)
 goto cleanup;
 }

-if (prd->managed == VIR_TRISTATE_BOOL_NO) {
-type = virXPathString("string(./source[1]/@type)", ctxt);
-path = virXPathString("string(./source[1]/@path)", ctxt);
-mode = virXPathString("string(./source[1]/@mode)", ctxt);
+type = virXPathString("string(./source[1]/@type)", ctxt);
+path = virXPathString("string(./source[1]/@path)", ctxt);
+mode = virXPathString("string(./source[1]/@mode)", ctxt);

+if (prd->managed == VIR_TRISTATE_BOOL_NO || type || path || mode) {
 if (!type) {
 virReportError(VIR_ERR_XML_ERROR, "%s",
_("missing connection type for "));
@@ -1950,24 +1950,23 @@ virStoragePRDefParseXML(xmlXPathContextPtr ctxt)
_("missing connection mode for "));
 goto cleanup;
 }
+}

-if (STRNEQ(type, "unix")) {
-virReportError(VIR_ERR_XML_ERROR,
-   _("unsupported connection type for : 
%s"),
-   type);
-goto cleanup;
-}
-
-if (STRNEQ(mode, "client")) {
-virReportError(VIR_ERR_XML_ERROR,
-   _("unsupported connection mode for : 
%s"),
-   mode);
-goto cleanup;
-}
+if (type && STRNEQ(type, "unix")) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("unsupported connection type for : 
%s"),
+   type);
+goto cleanup;
+}

-VIR_STEAL_PTR(prd->path, path);
+if (mode && STRNEQ(mode, "client")) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("unsupported connection mode for : 
%s"),
+   mode);
+goto cleanup;
 }

+VIR_STEAL_PTR(prd->path, path);
 VIR_STEAL_PTR(ret, prd);

  cleanup:
@@ -1986,7 +1985,7 @@ virStoragePRDefFormat(virBufferPtr buf,
 {
 virBufferAsprintf(buf, "managed));
-if (prd->managed == VIR_TRISTATE_BOOL_NO) {
+if (prd->path) {
 virBufferAddLit(buf, ">\n");
 virBufferAdjustIndent(buf, 2);
 virBufferAddLit(buf, "https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 02/13] qemu: alias: Allow passing alias of parent when generating PR manager alias

[Peter Krempa]

For use with blockdev the PR manager will be bound to a virStorageSource
rather than a virDomainDiskDef, so we will need to use the correct
alias.

Allow passing a string rather than the whole disk.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_alias.c   | 4 ++--
 src/qemu/qemu_alias.h   | 2 +-
 src/qemu/qemu_command.c | 4 ++--
 src/qemu/qemu_hotplug.c | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
index bd714f7aee..578a33b284 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -783,11 +783,11 @@ qemuDomainGetManagedPRAlias(void)


 char *
-qemuDomainGetUnmanagedPRAlias(const virDomainDiskDef *disk)
+qemuDomainGetUnmanagedPRAlias(const char *parentalias)
 {
 char *ret;

-ignore_value(virAsprintf(, "pr-helper-%s", disk->info.alias));
+ignore_value(virAsprintf(, "pr-helper-%s", parentalias));

 return ret;
 }
diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h
index 76678658c0..51f64624d7 100644
--- a/src/qemu/qemu_alias.h
+++ b/src/qemu/qemu_alias.h
@@ -94,6 +94,6 @@ char *qemuAliasChardevFromDevAlias(const char *devAlias)

 const char *qemuDomainGetManagedPRAlias(void);

-char *qemuDomainGetUnmanagedPRAlias(const virDomainDiskDef *disk);
+char *qemuDomainGetUnmanagedPRAlias(const char *parentalias);

 #endif /* __QEMU_ALIAS_H__*/
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 11ad77f145..24b482efd8 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1482,7 +1482,7 @@ qemuBuildDriveSourcePR(virBufferPtr buf,

 if (virStoragePRDefIsManaged(disk->src->pr))
 defaultAlias = qemuDomainGetManagedPRAlias();
-else if (!(alias = qemuDomainGetUnmanagedPRAlias(disk)))
+else if (!(alias = qemuDomainGetUnmanagedPRAlias(disk->info.alias)))
 return -1;


@@ -9705,7 +9705,7 @@ qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
 if (VIR_STRDUP(alias, qemuDomainGetManagedPRAlias()) < 0)
 goto cleanup;
 } else {
-if (!(alias = qemuDomainGetUnmanagedPRAlias(disk)))
+if (!(alias = qemuDomainGetUnmanagedPRAlias(disk->info.alias)))
 goto cleanup;
 }

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 1d748ccffb..52e1abdcd3 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3842,7 +3842,7 @@ qemuDomainDiskNeedRemovePR(virDomainObjPtr vm,
 return 0;

 if (!virStoragePRDefIsManaged(disk->src->pr)) {
-*aliasret = qemuDomainGetUnmanagedPRAlias(disk);
+*aliasret = qemuDomainGetUnmanagedPRAlias(disk->info.alias);
 return *aliasret ? 0 : -1;
 }

-- 
2.16.2

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

[libvirt] [PATCH 06/13] util: storage: Drop virStoragePRDefIsEnabled

[Peter Krempa]

The function now does not do anything useful. Replace it by the pointer
check.

Signed-off-by: Peter Krempa 
---
 src/libvirt_private.syms  | 1 -
 src/qemu/qemu_command.c   | 4 ++--
 src/qemu/qemu_domain.c| 8 
 src/qemu/qemu_hotplug.c   | 2 +-
 src/util/virstoragefile.c | 7 ---
 src/util/virstoragefile.h | 1 -
 6 files changed, 7 insertions(+), 16 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d28a751ebd..dd10be9753 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2804,7 +2804,6 @@ virStorageNetHostTransportTypeToString;
 virStorageNetProtocolTypeToString;
 virStoragePRDefFormat;
 virStoragePRDefFree;
-virStoragePRDefIsEnabled;
 virStoragePRDefIsEqual;
 virStoragePRDefIsManaged;
 virStoragePRDefParseXML;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 29ca2005a0..2bdba7734a 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1477,7 +1477,7 @@ qemuBuildDriveSourcePR(virBufferPtr buf,
 char *alias = NULL;
 const char *defaultAlias = NULL;

-if (!virStoragePRDefIsEnabled(disk->src->pr))
+if (!disk->src->pr)
 return 0;

 if (virStoragePRDefIsManaged(disk->src->pr))
@@ -9690,7 +9690,7 @@ qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
 *propsret = NULL;
 *aliasret = NULL;

-if (!virStoragePRDefIsEnabled(disk->src->pr))
+if (!disk->src->pr)
 return 0;

 if (!(socketPath = qemuDomainGetPRSocketPath(vm, disk->src->pr)))
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 611a96d6be..c8d2daa26f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4204,7 +4204,7 @@ qemuDomainValidateStorageSource(virStorageSourcePtr src,
 }
 }

-if (virStoragePRDefIsEnabled(src->pr) &&
+if (src->pr &&
 !virQEMUCapsGet(qemuCaps, QEMU_CAPS_PR_MANAGER_HELPER)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("reservations not supported with this QEMU binary"));
@@ -10240,7 +10240,7 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg 
ATTRIBUTE_UNUSED,
 }

 /* qemu-pr-helper might require access to /dev/mapper/control. */
-if (virStoragePRDefIsEnabled(disk->src->pr) &&
+if (disk->src->pr &&
 qemuDomainCreateDevice(DEVICE_MAPPER_CONTROL_PATH, data, true) < 0)
 goto cleanup;

@@ -11273,7 +11273,7 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
 }

 /* qemu-pr-helper might require access to /dev/mapper/control. */
-if (virStoragePRDefIsEnabled(src->pr) &&
+if (src->pr &&
 (VIR_STRDUP(dmPath, DEVICE_MAPPER_CONTROL_PATH) < 0 ||
  VIR_APPEND_ELEMENT_COPY(paths, npaths, dmPath) < 0))
 goto cleanup;
@@ -12050,7 +12050,7 @@ qemuDomainGetPRSocketPath(virDomainObjPtr vm,
 const char *defaultAlias = NULL;
 char *ret = NULL;

-if (!virStoragePRDefIsEnabled(pr))
+if (!pr)
 return NULL;

 if (virStoragePRDefIsManaged(pr)) {
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 52e1abdcd3..39c457e607 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3838,7 +3838,7 @@ qemuDomainDiskNeedRemovePR(virDomainObjPtr vm,
 *aliasret = NULL;
 *stopDaemon = false;

-if (!virStoragePRDefIsEnabled(disk->src->pr))
+if (!disk->src->pr)
 return 0;

 if (!virStoragePRDefIsManaged(disk->src->pr)) {
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index d6907e47bb..c89bdb9e49 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2018,13 +2018,6 @@ virStoragePRDefIsEqual(virStoragePRDefPtr a,
 }


-bool
-virStoragePRDefIsEnabled(virStoragePRDefPtr prd)
-{
-return !!prd;
-}
-
-
 bool
 virStoragePRDefIsManaged(virStoragePRDefPtr prd)
 {
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index ec49152880..3a90c60fa5 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -396,7 +396,6 @@ void virStoragePRDefFormat(virBufferPtr buf,
virStoragePRDefPtr prd);
 bool virStoragePRDefIsEqual(virStoragePRDefPtr a,
 virStoragePRDefPtr b);
-bool virStoragePRDefIsEnabled(virStoragePRDefPtr prd);
 bool virStoragePRDefIsManaged(virStoragePRDefPtr prd);

 virSecurityDeviceLabelDefPtr
-- 
2.16.2

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

[libvirt] [PATCH 08/13] qemu: Assign managed PR path when preparing storage source

[Peter Krempa]

Rather than always checking which path to use pre-assign it when
preparing storage source.

This reduces the need to pass 'vm' around too much. For later use the
path can be retrieved from the status XML.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_command.c | 18 +-
 src/qemu/qemu_command.h |  3 +--
 src/qemu/qemu_domain.c  | 35 ++-
 src/qemu/qemu_domain.h  |  3 +--
 src/qemu/qemu_hotplug.c |  2 +-
 src/qemu/qemu_process.c |  2 +-
 6 files changed, 31 insertions(+), 32 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 2bdba7734a..7df9979cb2 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9667,7 +9667,6 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,

 /**
  * qemuBuildPRManagerInfoProps:
- * @vm: domain object
  * @disk: disk definition
  * @propsret: Returns JSON object containing properties of the 
pr-manager-helper object
  * @aliasret: alias of the pr-manager-helper object
@@ -9678,12 +9677,10 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
  * -1 on failure (with error message set).
  */
 int
-qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
-const virDomainDiskDef *disk,
+qemuBuildPRManagerInfoProps(const virDomainDiskDef *disk,
 virJSONValuePtr *propsret,
 char **aliasret)
 {
-char *socketPath = NULL;
 char *alias = NULL;
 int ret = -1;

@@ -9693,9 +9690,6 @@ qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
 if (!disk->src->pr)
 return 0;

-if (!(socketPath = qemuDomainGetPRSocketPath(vm, disk->src->pr)))
-return ret;
-
 if (virStoragePRDefIsManaged(disk->src->pr)) {
 if (VIR_STRDUP(alias, qemuDomainGetManagedPRAlias()) < 0)
 goto cleanup;
@@ -9705,7 +9699,7 @@ qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
 }

 if (virJSONValueObjectCreate(propsret,
- "s:path", socketPath,
+ "s:path", disk->src->pr->path,
  NULL) < 0)
 goto cleanup;

@@ -9713,14 +9707,12 @@ qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
 ret = 0;
  cleanup:
 VIR_FREE(alias);
-VIR_FREE(socketPath);
 return ret;
 }


 static int
-qemuBuildMasterPRCommandLine(virDomainObjPtr vm,
- virCommandPtr cmd,
+qemuBuildMasterPRCommandLine(virCommandPtr cmd,
  const virDomainDef *def)
 {
 size_t i;
@@ -9740,7 +9732,7 @@ qemuBuildMasterPRCommandLine(virDomainObjPtr vm,
 managedAdded = true;
 }

-if (qemuBuildPRManagerInfoProps(vm, disk, , ) < 0)
+if (qemuBuildPRManagerInfoProps(disk, , ) < 0)
 goto cleanup;

 if (!props)
@@ -9934,7 +9926,7 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
 if (qemuBuildMasterKeyCommandLine(cmd, priv) < 0)
 goto error;

-if (qemuBuildMasterPRCommandLine(vm, cmd, def) < 0)
+if (qemuBuildMasterPRCommandLine(cmd, def) < 0)
 goto error;

 if (enableFips)
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index da1fe679fe..621592cd79 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -55,8 +55,7 @@ virCommandPtr qemuBuildCommandLine(virQEMUDriverPtr driver,
int **nicindexes);

 /* Generate the object properties for pr-manager */
-int qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
-const virDomainDiskDef *disk,
+int qemuBuildPRManagerInfoProps(const virDomainDiskDef *disk,
 virJSONValuePtr *propsret,
 char **alias);

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index eaa796260c..92217e66fe 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -11930,6 +11930,22 @@ qemuDomainPrepareDiskCachemode(virDomainDiskDefPtr 
disk)
 }


+static int
+qemuDomainPrepareStorageSourcePR(virStorageSourcePtr src,
+ qemuDomainObjPrivatePtr priv)
+{
+if (!src->pr)
+return 0;
+
+if (virStoragePRDefIsManaged(src->pr)) {
+if (!(src->pr->path = qemuDomainGetManagedPRSocketPath(priv)))
+return -1;
+}
+
+return 0;
+}
+
+
 int
 qemuDomainPrepareDiskSource(virDomainDiskDefPtr disk,
 qemuDomainObjPrivatePtr priv,
@@ -11946,6 +11962,9 @@ qemuDomainPrepareDiskSource(virDomainDiskDefPtr disk,
 if (qemuDomainPrepareDiskSourceChain(disk, NULL, cfg, priv->qemuCaps) < 0)
 return -1;

+if (qemuDomainPrepareStorageSourcePR(disk->src, priv) < 0)
+return -1;
+
 return 0;
 }

@@ -12051,22 +12070,12 @@ qemuProcessEventFree(struct qemuProcessEvent *event)


 char *
-qemuDomainGetPRSocketPath(virDomainObjPtr vm,
-  virStoragePRDefPtr pr)

[libvirt] [PATCH 04/13] qemu: Move validation of PR manager support

[Peter Krempa]

Disk source definition should be validated in
qemuDomainValidateStorageSource rather than in individual generators of
command line arguments.

Change to the XML2XML test is required since now the definition is
actually validated at define time.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_command.c | 7 ---
 src/qemu/qemu_domain.c  | 7 +++
 tests/qemuxml2xmltest.c | 2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index d84cf6dffc..29ca2005a0 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9683,7 +9683,6 @@ qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
 virJSONValuePtr *propsret,
 char **aliasret)
 {
-qemuDomainObjPrivatePtr priv = vm->privateData;
 char *socketPath = NULL;
 char *alias = NULL;
 int ret = -1;
@@ -9694,12 +9693,6 @@ qemuBuildPRManagerInfoProps(virDomainObjPtr vm,
 if (!virStoragePRDefIsEnabled(disk->src->pr))
 return 0;

-if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_PR_MANAGER_HELPER)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("reservations not supported with this QEMU binary"));
-return ret;
-}
-
 if (!(socketPath = qemuDomainGetPRSocketPath(vm, disk->src->pr)))
 return ret;

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 233327b906..611a96d6be 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4204,6 +4204,13 @@ qemuDomainValidateStorageSource(virStorageSourcePtr src,
 }
 }

+if (virStoragePRDefIsEnabled(src->pr) &&
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PR_MANAGER_HELPER)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("reservations not supported with this QEMU binary"));
+return -1;
+}
+
 return 0;
 }

diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 762e0e2d25..44cba97d4a 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -388,7 +388,7 @@ mymain(void)
 DO_TEST("disk-virtio-scsi-num_queues",
 QEMU_CAPS_VIRTIO_SCSI);
 DO_TEST("disk-virtio-scsi-reservations",
-QEMU_CAPS_VIRTIO_SCSI);
+QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_PR_MANAGER_HELPER);
 DO_TEST("disk-virtio-scsi-cmd_per_lun",
 QEMU_CAPS_VIRTIO_SCSI);
 DO_TEST("disk-virtio-scsi-max_sectors",
-- 
2.16.2

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

[libvirt] [PATCH 00/13] Fix XML for persistent reservations and refactor

[Peter Krempa]

The XML design for the PR stuff is slightly weird so fix it and refactor
the code so that it will be much easier to use it with the blockdev
infrastructure.

Peter Krempa (13):
  qemu: hotplug: Fix spacing around addition operator
  qemu: alias: Allow passing alias of parent when generating PR manager
alias
  qemu: command: Fix comment for qemuBuildPRManagerInfoProps
  qemu: Move validation of PR manager support
  util: storage: Drop pointless 'enabled' form PR definition
  util: storage: Drop virStoragePRDefIsEnabled
  util: storage: Allow passing  also for managed PR case
  qemu: Assign managed PR path when preparing storage source
  qemu: process: Change semantics of functions starting PR daemon
  qemu: command: Move check whether PR manager object props need to be
built
  conf: domain: Add helper to check whether a domain def requires use of
PR
  util: storage: Store PR manager alias in the definition
  qemu: hotplug: Replace qemuDomainDiskNeedRemovePR

 docs/formatdomain.html.in  |  21 ++--
 docs/schemas/storagecommon.rng |   3 -
 src/conf/domain_conf.c |  21 
 src/conf/domain_conf.h |   3 +
 src/libvirt_private.syms   |   2 +-
 src/qemu/qemu_alias.c  |   4 +-
 src/qemu/qemu_alias.h  |   2 +-
 src/qemu/qemu_command.c|  61 +++---
 src/qemu/qemu_command.h|   6 +-
 src/qemu/qemu_domain.c |  66 ---
 src/qemu/qemu_domain.h |   3 +-
 src/qemu/qemu_hotplug.c|  83 +++---
 src/qemu/qemu_process.c|  40 ++-
 src/qemu/qemu_process.h|   5 +-
 src/util/virstoragefile.c  | 124 -
 src/util/virstoragefile.h  |   5 +-
 tests/qemustatusxml2xmldata/modern-in.xml  |   4 +
 .../disk-virtio-scsi-reservations.xml  |   4 +-
 tests/qemuxml2xmltest.c|   2 +-
 19 files changed, 191 insertions(+), 268 deletions(-)

-- 
2.16.2

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

[libvirt] [PATCH 01/13] qemu: hotplug: Fix spacing around addition operator

[Peter Krempa]

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

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index f3ff945787..1d748ccffb 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -484,7 +484,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
 if (!(devstr = qemuBuildDriveDevStr(vm->def, disk, 0, priv->qemuCaps)))
 goto error;

-if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0)
+if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks + 1) < 0)
 goto error;

 qemuDomainObjEnterMonitor(driver, vm);
-- 
2.16.2

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

Re: [libvirt] [dbus PATCH v2 3/3] virtDBusNetworkGetDHCPLeases: fix type for expirytime

[Pavel Hrdina]

On Fri, May 11, 2018 at 06:16:56PM +0200, Katerina Koukiou wrote:
> Signed-off-by: Katerina Koukiou 
> ---
>  data/org.libvirt.Network.xml | 2 +-
>  src/network.c| 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Pavel Hrdina 


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

Re: [libvirt] [dbus PATCH v2 2/3] Replace uint -> int wherever libvirt uses int type

[Pavel Hrdina]

On Fri, May 11, 2018 at 06:16:55PM +0200, Katerina Koukiou wrote:
> Follow this pattern even if negative values will not
> appear, in order to be consistent with libvirt APIs.
> 
> Signed-off-by: Katerina Koukiou 
> ---
> Added type change for MemoryStats
> 
>  data/org.libvirt.Connect.xml | 18 +-
>  data/org.libvirt.Domain.xml  | 18 +-
>  data/org.libvirt.Network.xml |  2 +-
>  data/org.libvirt.Secret.xml  |  2 +-
>  data/org.libvirt.StoragePool.xml |  2 +-
>  src/connect.c|  6 +++---
>  src/domain.c | 22 +++---
>  src/events.c | 12 ++--
>  src/network.c|  4 ++--
>  src/secret.c |  2 +-
>  src/storagepool.c|  2 +-
>  11 files changed, 45 insertions(+), 45 deletions(-)

Reviewed-by: Pavel Hrdina 


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

Re: [libvirt] [dbus PATCH v2 1/3] virtDBusNetworkGetDHCPLeases: fix lease type

[Pavel Hrdina]

On Fri, May 11, 2018 at 06:16:54PM +0200, Katerina Koukiou wrote:
> Signed-off-by: Katerina Koukiou 
> ---
>  src/network.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Some explanation would be nice, something like:

Commit 8eac355f3e removed *TypedToString conversion but missed to
update this one place.

Reviewed-by: Pavel Hrdina 


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

Re: [libvirt] [dbus PATCH v2 3/3] events: Register VIR_DOMAIN_EVENT_ID_IO_ERROR_REASON

[Pavel Hrdina]

On Fri, May 11, 2018 at 05:49:23PM +0200, Katerina Koukiou wrote:
> Signed-off-by: Katerina Koukiou 
> ---
> Return empty string when variable is NULL
> 
>  data/org.libvirt.Domain.xml |  8 
>  src/events.c| 31 +++
>  2 files changed, 39 insertions(+)

Reviewed-by: Pavel Hrdina 


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

Re: [libvirt] [dbus PATCH v2 2/3] events: Register VIR_DOMAIN_EVENT_ID_GRAPHICS

[Pavel Hrdina]

On Fri, May 11, 2018 at 05:49:22PM +0200, Katerina Koukiou wrote:
> Signed-off-by: Katerina Koukiou 
> ---
> Return empty string when variable is NULL
> 
>  data/org.libvirt.Domain.xml |  9 +++
>  src/events.c| 60 
> +
>  src/util.h  |  2 ++
>  3 files changed, 71 insertions(+)

Reviewed-by: Pavel Hrdina 


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

Re: [libvirt] [dbus PATCH v2 1/3] events: Register VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2

[Pavel Hrdina]

On Fri, May 11, 2018 at 05:49:21PM +0200, Katerina Koukiou wrote:
> Signed-off-by: Katerina Koukiou 
> ---
> Use only v2 of BlockJob
> 
>  data/org.libvirt.Domain.xml |  8 
>  src/events.c| 28 
>  2 files changed, 36 insertions(+)

Reviewed-by: Pavel Hrdina 


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

Re: [libvirt] [PATCH] travis: Uninstall packages before upgrade

[Daniel P . Berrangé]

On Mon, May 14, 2018 at 12:07:45PM +0200, Andrea Bolognani wrote:
> numpy (needed by cgal) started having the same issue with
> linking as python, which makes upgrade and thus the entire
> build fail on macOS.
> 
> Instead of playing more tricks with linking/unlinking, just
> uninstall the problematic packages (and those dragging them
> in) before doing anything else.
> 
> Signed-off-by: Andrea Bolognani 
> ---
> Technically a build breaker, but since I'm changing the
> approach I'd rather wait for an explicit ACK before pushing.
> 
>  .travis.yml | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/.travis.yml b/.travis.yml
> index d3f72d46f3..140072b801 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -21,10 +21,10 @@ matrix:
>  - compiler: clang
>os: osx
>before_install:
> +- brew uninstall python mercurial postgis sfcgal cgal gdal
>  - brew update
> -- brew unlink python
>  - brew upgrade
> -- brew install rpcgen yajl
> +- brew install python rpcgen yajl
>script:
>  # We can't run make distcheck/syntax-check because they
>  # fail on macOS, but doing 'install' and 'dist' gives us

Reviewed-by: Daniel P. Berrangé 

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[libvirt] [PATCH] travis: Uninstall packages before upgrade

[Andrea Bolognani]

numpy (needed by cgal) started having the same issue with
linking as python, which makes upgrade and thus the entire
build fail on macOS.

Instead of playing more tricks with linking/unlinking, just
uninstall the problematic packages (and those dragging them
in) before doing anything else.

Signed-off-by: Andrea Bolognani 
---
Technically a build breaker, but since I'm changing the
approach I'd rather wait for an explicit ACK before pushing.

 .travis.yml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index d3f72d46f3..140072b801 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -21,10 +21,10 @@ matrix:
 - compiler: clang
   os: osx
   before_install:
+- brew uninstall python mercurial postgis sfcgal cgal gdal
 - brew update
-- brew unlink python
 - brew upgrade
-- brew install rpcgen yajl
+- brew install python rpcgen yajl
   script:
 # We can't run make distcheck/syntax-check because they
 # fail on macOS, but doing 'install' and 'dist' gives us
-- 
2.17.0

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

Re: [libvirt] [PATCH 2/5] Introduce virCryptoHashBuf

[Michal Privoznik]

On 05/11/2018 05:50 PM, Ján Tomko wrote:
> A function that keeps the hash in binary form instead of converting
> it to human-readable hexadecimal form.
> 
> Signed-off-by: Ján Tomko 
> ---
>  src/util/vircrypto.c | 31 +--
>  src/util/vircrypto.h |  7 +++
>  2 files changed, 28 insertions(+), 10 deletions(-)
> 
> diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c
> index 48b04fc8ce..1a2dcc28b7 100644
> --- a/src/util/vircrypto.c
> +++ b/src/util/vircrypto.c
> @@ -54,28 +54,39 @@ struct virHashInfo {
>  verify(ARRAY_CARDINALITY(hashinfo) == VIR_CRYPTO_HASH_LAST);
>  
>  int
> -virCryptoHashString(virCryptoHash hash,
> -const char *input,
> -char **output)
> +virCryptoHashBuf(virCryptoHash hash,
> + const char *input,
> + unsigned char *output)
>  {
> -unsigned char buf[VIR_CRYPTO_LARGEST_DIGEST_SIZE];
> -size_t hashstrlen;
> -size_t i;
> -
>  if (hash >= VIR_CRYPTO_HASH_LAST) {
>  virReportError(VIR_ERR_INVALID_ARG,
> _("Unknown crypto hash %d"), hash);
>  return -1;
>  }

This check feels useless. It's like if we were checking a value before
passing it to vir*TypeToString(). But it's pre-existing, so you have my
ACK. But a follow up patch removing it (=trivial) would be nice.

Also, don't forget to export the symbol in libvirt_private.syms ;-)
I'm wondering how linker succeeds in 3/5 where you use the function
without it being exported. Maybe my compiler decided to inline this
function?

Michal

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

Re: [libvirt] [PATCH 0/5] Rely on GnuTLS for md5/sha256 functions

[Michal Privoznik]

On 05/11/2018 05:50 PM, Ján Tomko wrote:
> Ján Tomko (5):
>   vircrypto: provide constants for hash sizes
>   Introduce virCryptoHashBuf
>   esx: use virCryptoHashBuf
>   esx: Use VIR_CRYPTO_HASH_SIZE_MD5
>   vircrypto: Rely on GnuTLS for hash functions
> 
>  bootstrap.conf  |  2 --
>  src/esx/esx_network_driver.c| 22 --
>  src/esx/esx_storage_backend_iscsi.c | 46 +-
>  src/esx/esx_storage_backend_vmfs.c  | 20 ++---
>  src/util/vircrypto.c| 57 
> ++---
>  src/util/vircrypto.h| 10 +++
>  6 files changed, 100 insertions(+), 57 deletions(-)
> 

ACK series, but please see my comment in 2/5 before pushing.

Michal

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

Re: [libvirt] [PATCH v2 4/8] qemu: vfio-ccw device address generation

[Boris Fiuczynski]

On 05/10/2018 10:52 PM, John Ferlan wrote:



On 05/07/2018 10:41 AM, Boris Fiuczynski wrote:

From: Shalini Chellathurai Saroja 

Introduces the vfio-ccw model for mediated devices and prime vfio-ccw
devices such that CCW address will be generated.

Signed-off-by: Shalini Chellathurai Saroja 
Reviewed-by: Bjoern Walk 
Reviewed-by: Boris Fiuczynski 
Reviewed-by: Marc Hartmayer 
Reviewed-by: Stefan Zimmermann 
---
  docs/schemas/domaincommon.rng  |  5 -
  src/qemu/qemu_domain_address.c | 20 
  src/util/virmdev.c |  3 ++-
  src/util/virmdev.h |  1 +
  4 files changed, 27 insertions(+), 2 deletions(-)



Looking at all places using VIR_MDEV_MODEL_TYPE_VFIO_PCI - should this
patch make a change to virDomainHostdevDefPostParse to do something
similar - that is:

if (model == VIR_MDEV_MODEL_TYPE_VFIO_CCW &&
 dev->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
... error message
   }

?

Let me know... I think it should and can add it before pushing...

You are correct. Good catch!
How about fixing it like this?
if ((model == VIR_MDEV_MODEL_TYPE_VFIO_PCI &&
dev->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) ||
(model == VIR_MDEV_MODEL_TYPE_VFIO_CCW &&
dev->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW)) {
virReportError(VIR_ERR_XML_ERROR,
   _("Unsupported address type '%s' with mediated "
 "device model '%s'"),

virDomainDeviceAddressTypeToString(dev->info->type),
   virMediatedDeviceModelTypeToString(model));
return -1;
}



Besides that I just saw that the indention of the second parameter of 
method qemuDomainPrimeVfioDeviceAddresses is off by three blanks.




Reviewed-by: John Ferlan 

John

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




--
Mit freundlichen Grüßen/Kind regards
   Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

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

Re: [libvirt] [dbus PATCH] connect: Remove unused variable

[Ján Tomko]

On Mon, May 14, 2018 at 09:13:19AM +0200, Andrea Bolognani wrote:

Clang catches the issue:

 ../../src/connect.c:532:26: error: unused variable 'domain' 
[-Werror,-Wunused-variable]
 g_autoptr(virDomain) domain = NULL;
  ^

Signed-off-by: Andrea Bolognani 
---
src/connect.c | 1 -
1 file changed, 1 deletion(-)



Reviewed-by: Ján Tomko 

Jano


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

[libvirt] [dbus PATCH] connect: Remove unused variable

[Andrea Bolognani]

Clang catches the issue:

  ../../src/connect.c:532:26: error: unused variable 'domain' 
[-Werror,-Wunused-variable]
  g_autoptr(virDomain) domain = NULL;
   ^

Signed-off-by: Andrea Bolognani 
---
 src/connect.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/connect.c b/src/connect.c
index 6719e21..14a60dc 100644
--- a/src/connect.c
+++ b/src/connect.c
@@ -529,7 +529,6 @@ virtDBusConnectDomainSaveImageGetXMLDesc(GVariant *inArgs,
  GError **error)
 {
 virtDBusConnect *connect = userData;
-g_autoptr(virDomain) domain = NULL;
 const gchar *file;
 guint flags;
 g_autofree gchar *xml = NULL;
-- 
2.17.0

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

Re: [libvirt] [PATCH 0/3] Finish replacements for virDomainObjCheckActive

[Ján Tomko]

On Mon, May 14, 2018 at 12:32:12AM +0200, c...@lse.epita.fr wrote:

From: Clementine Hayat 

Follow-up of 
https://www.redhat.com/archives/libvir-list/2018-April/msg01591.html

Here is the changes asked by Ján Tomko:
* Refactor also checks when the error message is close to "domain is not
 running"
* Add forgotten checks in qemu_migration.c
* Revert one incorrect change in lxc_driver.c

Clementine Hayat (3):
 qemu: start using virDomainObjCheckActive
 lxc: start using virDomainObjCheckActive
 bhyve: start using virDomainObjCheckActive

src/bhyve/bhyve_driver.c  |  15 +--
src/lxc/lxc_driver.c  |  65 ++---
src/qemu/qemu_domain.c|   5 +-
src/qemu/qemu_driver.c| 271 --
src/qemu/qemu_migration.c |  10 +-
5 files changed, 74 insertions(+), 292 deletions(-)


Reviewed-by: Ján Tomko 

And pushed.

Jano


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

Re: [libvirt] [PATCH 0/3] bhyve: allow locking memory

[Roman Bogorodskiy]

  Peter Krempa wrote:

> On Sun, May 13, 2018 at 14:01:25 +0400, Roman Bogorodskiy wrote:
> >   Fabian Freyer wrote:
> 
> [...]
> 
> > >  13 files changed, 134 insertions(+)
> > >  create mode 100644 tests/bhyveargv2xmldata/bhyveargv2xml-wired.args
> > >  create mode 100644 tests/bhyveargv2xmldata/bhyveargv2xml-wired.xml
> > >  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-wired.args
> > >  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-wired.ldargs
> > >  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-wired.xml
> > >  create mode 100644 tests/bhyvexml2xmloutdata/bhyvexml2xmlout-wired.xml
> > 
> > Thanks for the patches, I've pushed the series with some syntax-check
> > fixes (trailing whitespaces, tabs instead of whitespaces) and added
> > 'Signed-off-by' line which is mandatory now.
> 
> Note that 'Signed-off-by' should _NOT_ be added without explicit consent
> of the author of the patches. Otherwise it defeats the purpose of
> certification that the author agrees and complies with the Developer
> certificate of origin. Adding it without explicit consent makes the
> Signed-off-by line just a useless piece of jewelery added to appease the
> push hook.

Oh, sorry about that. I assumed as Fabian is a long time contributor,
he's familiar with the general project policies.

Fabian, do you have any issue with that?

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



Roman Bogorodskiy


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

Re: [libvirt] [PATCH v2 1/8] qemu: introduce capability for virtual-css-bridge

[Boris Fiuczynski]

On 05/10/2018 10:46 PM, John Ferlan wrote:



On 05/07/2018 10:41 AM, Boris Fiuczynski wrote:

From: Shalini Chellathurai Saroja 

Let us introduce the capability QEMU_CAPS_CCW for virtual-css-bridge
and replace QEMU_CAPS_VIRTIO_CCW with QEMU_CAPS_CCW in code segments
which identify support for ccw devices.

The virtual-css-bridge is part of the ccw support introduced in QEMU 2.7.
The QEMU_CAPS_CCW capability is based on the existence of the QEMU type.

Let us also add the capability QEMU_CAPS_CCW to the tests which
require support for ccw devices.

Signed-off-by: Shalini Chellathurai Saroja 
Reviewed-by: Bjoern Walk 
Reviewed-by: Boris Fiuczynski 
---
  src/qemu/qemu_capabilities.c |  9 +++
  src/qemu/qemu_capabilities.h |  1 +
  src/qemu/qemu_domain.c   |  2 +-
  src/qemu/qemu_domain_address.c   |  4 +-
  src/qemu/qemu_hotplug.c  |  4 +-
  tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml |  1 +
  tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml |  1 +
  tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml  |  1 +
  tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml  |  1 +
  tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml  |  1 +
  tests/qemuhotplugtest.c  |  2 +-
  tests/qemuxml2argvtest.c | 86 
  tests/qemuxml2xmltest.c  | 26 +++
  13 files changed, 77 insertions(+), 62 deletions(-)



tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml

was missed on this patch - make check will fail, so it needs to be
adjusted now so that bisection doesn't some day have problems. I see it
was added in patch 2 and that

IOW:

+  
You are right of course. It is a miss on my part. Patch 2 contains the 
change and I was to lazy to run an iterated apply patch and build which 
would have caught this. Sorry.





You may also want to send a "final" v2.12.0 replies/caps patch later too...
You are right and Shalini has already prepared a patch to do so. She 
will send it out soon.




Reviewed-by: John Ferlan 

John

Thanks



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




--
Mit freundlichen Grüßen/Kind regards
   Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

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

Re: [libvirt] [PATCH 0/3] bhyve: allow locking memory

[Peter Krempa]

On Sun, May 13, 2018 at 14:01:25 +0400, Roman Bogorodskiy wrote:
>   Fabian Freyer wrote:

[...]

> >  13 files changed, 134 insertions(+)
> >  create mode 100644 tests/bhyveargv2xmldata/bhyveargv2xml-wired.args
> >  create mode 100644 tests/bhyveargv2xmldata/bhyveargv2xml-wired.xml
> >  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-wired.args
> >  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-wired.ldargs
> >  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-wired.xml
> >  create mode 100644 tests/bhyvexml2xmloutdata/bhyvexml2xmlout-wired.xml
> 
> Thanks for the patches, I've pushed the series with some syntax-check
> fixes (trailing whitespaces, tabs instead of whitespaces) and added
> 'Signed-off-by' line which is mandatory now.

Note that 'Signed-off-by' should _NOT_ be added without explicit consent
of the author of the patches. Otherwise it defeats the purpose of
certification that the author agrees and complies with the Developer
certificate of origin. Adding it without explicit consent makes the
Signed-off-by line just a useless piece of jewelery added to appease the
push hook.

> 
> Roman Bogorodskiy



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



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