date:20180302

[libvirt] [BUG]libvirt doesn't show the real state of vm

2018-03-02 Thread Wuzongyong (Euler Dept)

Hi,

I notice that monitor.sock still exist when I send SIGKILL to qemu process by 
accident, and I can't find the qemu process
through `ps -ef |grep qemu` command.
Then I execute `virsh list --all`, libvirt just show that the vm still in 
running state. I can't find log "Received EOF on xxx" either
when set log level to debug.

Any suggestion will be appreciated!


Thanks,
Zongyong Wu

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

Re: [libvirt] [PATCH v5 12/12] qemu: Remove old qemuDomainDeviceDefValidateControllerPCI()

2018-03-02 Thread Laine Stump

On 03/02/2018 10:13 AM, Andrea Bolognani wrote:
> We've implemented all existing checks, and more, in the new
> function, so we can finally drop the old one.
>
> Signed-off-by: Andrea Bolognani 

Reviewed-by: Laine Stump 

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

Re: [libvirt] [PATCH v5 11/12] qemu: Validate PCI controllers (QEMU capabilities)

2018-03-02 Thread Laine Stump

On 03/02/2018 10:13 AM, Andrea Bolognani wrote:
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_domain.c | 190 
> -
>  1 file changed, 77 insertions(+), 113 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index f54e7b87ae..e0ab43e139 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4270,11 +4270,8 @@ qemuDomainDeviceDefValidateControllerSCSI(const 
> virDomainControllerDef *controll
>  static int
>  qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef 
> *controller,
>  const virDomainDef *def,
> -virQEMUCapsPtr qemuCaps)
> +virQEMUCapsPtr qemuCaps 
> ATTRIBUTE_UNUSED)
>  {
> -virDomainControllerModelPCI model = controller->model;
> -const virDomainPCIControllerOpts *pciopts;
> -
>  /* skip pcie-root */
>  if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)
>  return 0;
> @@ -4285,119 +4282,50 @@ qemuDomainDeviceDefValidateControllerPCIOld(const 
> virDomainControllerDef *contro
>  controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT)
>  return 0;
>  
> -pciopts = >opts.pciopts;
> -
> -/* Second pass - now the model specific checks */
> -switch (model) {
> -case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
> -if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -   _("the pci-bridge controller is not supported "
> - "in this QEMU binary"));
> -return -1;
> -}
> -
> -break;
> -
> -case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
> -if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB)) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -   _("the pxb controller is not supported in this "
> - "QEMU binary"));
> -return -1;
> -}
> -
> -break;
> -
> -case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
> -if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -   _("the dmi-to-pci-bridge (i82801b11-bridge) "
> - "controller is not supported in this QEMU 
> binary"));
> -return -1;
> -}
> -
> -break;
> -
> -case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
> -if ((pciopts->modelName == 
> VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) &&
> -!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IOH3420)) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -   _("the pcie-root-port (ioh3420) controller "
> - "is not supported in this QEMU binary"));
> -return -1;
> -}
> -
> -if ((pciopts->modelName == 
> VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT) &&
> -!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCIE_ROOT_PORT)) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -   _("the pcie-root-port (pcie-root-port) controller 
> "
> - "is not supported in this QEMU binary"));
> -return -1;
> -}
> -
> -break;
> -
> -case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
> -if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_X3130_UPSTREAM)) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -   _("the pcie-switch-upstream-port (x3130-upstream) 
> "
> - "controller is not supported in this QEMU 
> binary"));
> -return -1;
> -}
> -
> -break;
> -
> -case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
> -if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM)) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -   _("The pcie-switch-downstream-port "
> - "(xio3130-downstream) controller is not "
> - "supported in this QEMU binary"));
> -return -1;
> -}
> -
> -break;
> -
> -case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
> -if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB_PCIE)) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -   _("the pxb-pcie controller is not supported "
> - "in this QEMU binary"));
> -return -1;
> -}
> -
> -break;
> -
> -case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> -/* Skip the implicit one */
> -if

Re: [libvirt] [PATCH v5 10/12] qemu: Validate PCI controller options (chassis and port)

2018-03-02 Thread Laine Stump

On 03/02/2018 10:13 AM, Andrea Bolognani wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1483816
>
> Signed-off-by: Andrea Bolognani 

Reviewed-by: Laine Stump 

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

Re: [libvirt] [PATCH v5 09/12] qemu: Validate PCI controller options (chassisNr)

2018-03-02 Thread Laine Stump

On 03/02/2018 10:13 AM, Andrea Bolognani wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1483816
>
> Signed-off-by: Andrea Bolognani 

Reviewed-by: Laine Stump 

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

Re: [libvirt] [PATCH 0/6] qemu: domain: Private data handling refactors

2018-03-02 Thread John Ferlan



On 03/01/2018 12:59 PM, Peter Krempa wrote:
> Extracted from a huge work-in-progres series since these look
> stand-alone.
> 
> Peter Krempa (6):
>   qemu: Add qemu functions for storage source private data handling
>   qemu: domain: Split out formating of Job data from private data
> formatter
>   qemu: domain: Don't overwrite job type in private data
>   qemu: domain: Return early in qemuDomainObjPrivateXMLFormatJob
>   qemu: domain: Use virXMLFormatElement in
> qemuDomainObjPrivateXMLFormatJob
>   qemu: domain: Extract parsing of job-related private XML
> 
>  src/qemu/qemu_domain.c | 218 
> ++---
>  1 file changed, 134 insertions(+), 84 deletions(-)
> 

Series,

Reviewed-by: John Ferlan 

BTW: Your patch 1 has some similarities to Michal's patch 5 for the
persistent reservations.

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

Re: [libvirt] [PATCH 1/2] conf: nodedev: Don't refresh host caps in testdriver

2018-03-02 Thread John Ferlan

On 03/02/2018 04:36 PM, Cole Robinson wrote:
> On 03/02/2018 04:02 PM, John Ferlan wrote:
>>
>>
>> On 02/23/2018 06:16 PM, Cole Robinson wrote:
>>> Add a 'testdriver' bool that we set for test_driver.c nodedevs
>>> which will skip accessing host resources via virNodeDeviceUpdateCaps
>>>
>>> Signed-off-by: Cole Robinson 
>>> ---
>>>  src/conf/node_device_conf.c | 3 +++
>>>  src/conf/node_device_conf.h | 1 +
>>>  src/test/test_driver.c  | 2 ++
>>>  3 files changed, 6 insertions(+)
>>>
>>> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
>>> index fd8f4e4a9..90c940f11 100644
>>> --- a/src/conf/node_device_conf.c
>>> +++ b/src/conf/node_device_conf.c
>>> @@ -2425,6 +2425,9 @@ virNodeDeviceUpdateCaps(virNodeDeviceDefPtr def)
>>>  {
>>>  virNodeDevCapsDefPtr cap = def->caps;
>>>  
>>> +if (def->testdriver)
>>> +return 0;
>>> +
>>>  while (cap) {
>>>  switch (cap->data.type) {
>>>  case VIR_NODE_DEV_CAP_SCSI_HOST:
>>> diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
>>> index 685ae3034..665f766e2 100644
>>> --- a/src/conf/node_device_conf.h
>>> +++ b/src/conf/node_device_conf.h
>>> @@ -316,6 +316,7 @@ struct _virNodeDeviceDef {
>>>  char *driver;   /* optional driver name */
>>>  char *devnode;  /* /dev path */
>>>  char **devlinks;/* /dev links */
>>> +bool testdriver;/* if true, skip host checks */
>>
>> Not sure this should be in virNodeDeviceDef... I think it should be in
>> virNodeDeviceObj. Yes, a bit more work, but I think cleaner. You'd need
>> to create an accessor function in order to set the flag from
>> test_driver. Then avoid calling virNodeDeviceUpdateCaps only from
>> virNodeDeviceMatch if the flag is set.
>>
>> Also instead of "testdriver", how about "skipUpdateCaps" since the
>> purpose of this is to skip calling virNodeDeviceUpdateCaps?
>>
>> At least that hides that this currently is only for the test driver.
>> Perhaps in the future there could be some other reason to not want to
>> update the caps for some specific definition after perhaps it's "known"
>> or "determined" that a specific update had occurred.
> 
> Okay, thanks for review. How about finding a way to remove UpdateCaps
> from generic conf.c implementations instead? Seems wrong that something
> functions needed to implement ListAllDevices touch host resources, I
> don't think other objects work like that
> 

The call to add update for export was a fairly recent, commit id
'd18feadc'. Seems that Erik Skultety has the most recent knowledge.
Hopefully he reads and chimes in...

The nodedev driver isn't like other drivers w/r/t being able to define
or create defs on your own. Instead you rely on udev (or gasp hal) in
order to provide "events" that would add, change, delete the device and
get it's capabilities.

It gets worse because even though an event is fired, it doesn't mean the
update has really occurred. I know mdevs and npiv scsi_host/target's are
afflicted by similar problems - udev says it's got an event, but some
other layer such as systemd is still filling in the data. For npiv
devices, I found that there was a possibility if the timing was just
right that the wwnn, wwpn, and fabric_wwn fields were set to 0 or
, see bz 1319544 for some details. I did generate a
patch to work around it, but it wasn't accepted because it was a hack or
workaround essentially. I know I've reviewed a patch recently in the
mdev space for a somewhat similar problem.

I wonder if it would be better to have some sort of refresh logic that
would do the same/similar type magic at least with respect to getting
updates for the 4 types (SCSI_HOST, SCSI_TARGET, NET, PCI_DEV) that seem
to be lagging or needing to get the latest information. Ironically I
would half expect MDEV or MDEV_TYPES to be included in the need update
list if only because of the similar issues to npiv at least w/r/t
systemd interaction. In any case, I'm not sure there's a simple way to
"know" your data is out of date.

Perhaps the UpdateCaps call for Export isn't necessary, but I can see a
reason for it - that "automagic update" functionality. Just because it
doesn't work well for the test driver isn't perhaps a "good enough"
reason to just remove it.

I'm willing to be convinced otherwise though!

John

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

Re: [libvirt] [PATCH v5 08/12] qemu: Validate PCI controller options (numaNode)

2018-03-02 Thread Laine Stump

On 03/02/2018 10:13 AM, Andrea Bolognani wrote:
> This change catches an invalid use of the option in our
> test suite.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1483816
>
> Signed-off-by: Andrea Bolognani 

Reviewed-by: Laine Stump 

> ---
>  src/qemu/qemu_domain.c | 52 
> ++
>  tests/qemuxml2argvdata/pcie-expander-bus.xml   |  3 --
>  tests/qemuxml2xmloutdata/pcie-expander-bus.xml |  4 +-
>  3 files changed, 53 insertions(+), 6 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 1354d9850a..07ed006f70 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4747,6 +4747,58 @@ qemuDomainDeviceDefValidateControllerPCI(const 
> virDomainControllerDef *cont,
>  return -1;
>  }
>  
> +/* numaNode */
> +switch ((virDomainControllerModelPCI) cont->model) {
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
> +/* numaNode can be used for these controllers, but it's not set
> + * automatically so it can be missing */
> +break;
> +
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> +/* Only PHBs support numaNode */
> +if (pciopts->numaNode != -1 &&
> +pciopts->modelName != 
> VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("Option '%s' is not valid for '%s' controller"),
> +   "numaNode", model);
> +return -1;
> +}
> +
> +/* However, the default PHB doesn't support numaNode */
> +if (pciopts->numaNode != -1 &&
> +pciopts->modelName == 
> VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE &&
> +pciopts->targetIndex == 0) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("Option '%s' is not valid for '%s' controller "
> + "with '%s' equal to 0"),
> +   "numaNode", model,
> +   "targetIndex");
> +return -1;
> +}
> +break;
> +
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
> +case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
> +if (pciopts->numaNode != -1) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("Option '%s' is not valid for '%s' controller"),
> +   "numaNode", model);
> +return -1;
> +}
> +break;
> +
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
> +default:
> +virReportEnumRangeError(virDomainControllerModelPCI, cont->model);
> +return -1;
> +}
> +
>  return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps);
>  }
>  
> diff --git a/tests/qemuxml2argvdata/pcie-expander-bus.xml 
> b/tests/qemuxml2argvdata/pcie-expander-bus.xml
> index 237430a1e5..5c5d34d1e0 100644
> --- a/tests/qemuxml2argvdata/pcie-expander-bus.xml
> +++ b/tests/qemuxml2argvdata/pcie-expander-bus.xml
> @@ -35,9 +35,6 @@
>
>  
>  
> -  
> -1
> -  
>
>  
>  
> diff --git a/tests/qemuxml2xmloutdata/pcie-expander-bus.xml 
> b/tests/qemuxml2xmloutdata/pcie-expander-bus.xml
> index d5e741b80d..b6498fd2eb 100644
> --- a/tests/qemuxml2xmloutdata/pcie-expander-bus.xml
> +++ b/tests/qemuxml2xmloutdata/pcie-expander-bus.xml
> @@ -36,9 +36,7 @@
>  
>  
>
> -  
> -1
> -  
> +  
> function='0x0'/>
>  
>  


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

Re: [libvirt] [PATCH v5 07/12] qemu: Validate PCI controller options (busNr)

2018-03-02 Thread Laine Stump

On 03/02/2018 10:13 AM, Andrea Bolognani wrote:
> This change catches an invalid use of the option in our
> test suite.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1483816
>
> Signed-off-by: Andrea Bolognani 

Reviewed-by: Laine Stump 

> ---
>  src/qemu/qemu_domain.c | 46 
> +++---
>  tests/qemuxml2argvdata/pcie-expander-bus.xml   |  2 +-
>  tests/qemuxml2xmloutdata/pcie-expander-bus.xml |  2 +-
>  3 files changed, 36 insertions(+), 14 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 9b8d2e864d..1354d9850a 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4306,12 +4306,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const 
> virDomainControllerDef *contro
>  break;
>  
>  case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
> -if (pciopts->busNr == -1) {
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -   _("autogenerated pci-expander-bus options not 
> set"));
> -return -1;
> -}
> -
>  if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB)) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("the pxb controller is not supported in this "
> @@ -4385,12 +4379,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const 
> virDomainControllerDef *contro
>  break;
>  
>  case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
> -if (pciopts->busNr == -1) {
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -   _("autogenerated pcie-expander-bus options not 
> set"));
> -return -1;
> -}
> -
>  if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB_PCIE)) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("the pxb-pcie controller is not supported "
> @@ -4725,6 +4713,40 @@ qemuDomainDeviceDefValidateControllerPCI(const 
> virDomainControllerDef *cont,
>  return -1;
>  }
>  
> +/* busNr */
> +switch ((virDomainControllerModelPCI) cont->model) {
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
> +if (pciopts->busNr == -1) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Option '%s' not set for '%s' controller"),
> +   "busNr", model);
> +return -1;
> +}
> +break;
> +
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
> +case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
> +if (pciopts->busNr != -1) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("Option '%s' is not valid for '%s' controller"),
> +   "busNr", model);
> +return -1;
> +}
> +break;
> +
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
> +default:
> +virReportEnumRangeError(virDomainControllerModelPCI, cont->model);
> +return -1;
> +}
> +
>  return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps);
>  }
>  
> diff --git a/tests/qemuxml2argvdata/pcie-expander-bus.xml 
> b/tests/qemuxml2argvdata/pcie-expander-bus.xml
> index ac01c26ccf..237430a1e5 100644
> --- a/tests/qemuxml2argvdata/pcie-expander-bus.xml
> +++ b/tests/qemuxml2argvdata/pcie-expander-bus.xml
> @@ -35,7 +35,7 @@
>
>  
>  
> -  
> +  
>  1
>
>
> diff --git a/tests/qemuxml2xmloutdata/pcie-expander-bus.xml 
> b/tests/qemuxml2xmloutdata/pcie-expander-bus.xml
> index aaac423cac..d5e741b80d 100644
> --- a/tests/qemuxml2xmloutdata/pcie-expander-bus.xml
> +++ b/tests/qemuxml2xmloutdata/pcie-expander-bus.xml
> @@ -36,7 +36,7 @@
>  
>  
>
> -  
> +  
>  1
>
> function='0x0'/>


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

Re: [libvirt] [PATCH] virtlockd: acquire locks on re-exec

2018-03-02 Thread Jim Fehlig


On 03/02/2018 11:12 AM, Daniel P. Berrangé wrote:

On Fri, Mar 02, 2018 at 04:52:23PM +, Daniel P. Berrangé wrote:

On Thu, Mar 01, 2018 at 04:42:36PM -0700, Jim Fehlig wrote:

Locks held by virtlockd are dropped on re-exec.

virtlockd   94306 POSIX 5.4G WRITE 0 0   0 /tmp/test.qcow2
virtlockd   94306 POSIX   5B WRITE 0 0   0 /run/virtlockd.pid
virtlockd   94306 POSIX   5B WRITE 0 0   0 /run/virtlockd.pid

Acquire locks in PostExecRestart code path.


This is really strange and should *not* be happening.  POSIX locks
are supposed to be preserved across execve() if the FD has CLOEXEC
unset, and you don't fork() before the exec.


[snip]


So I wonder what we've screwed up to cause the locks to get
released - reaquiring them definitely isn't desirable as we
should not loose them in the first place !


This is really very strange. The problem seems to be the existance of
threads at time of execve().

If you spawn a thread and the thread exits, and you execve the locks
are preserved.

If you spawn a thread and the thread is still running, and you execve
the locks are lost.


Indeed you are correct. I'm seeing the same behavior with the below 
modifications to your demo. The lock is preserved after execve when BREAK_FLOCK 
is 0, but removed when BREAK_FLOCK is 1.


--- lock.c  2018-03-02 15:10:59.200154182 -0700
+++ lock-thr.c  2018-03-02 15:14:30.501441105 -0700
@@ -4,6 +4,15 @@
 #include 
 #include 

+#define BREAK_FLOCK 1
+
+static void *thr_func(void *arg)
+{
+#if BREAK_FLOCK == 1
+while (1)
+#endif
+  sleep(5);
+}

 int main(int argc, char **argv) {

@@ -33,6 +42,13 @@
 sleep(50);

   } else {
+pthread_t thr;
+
+if (pthread_create(, NULL, thr_func, NULL) != 0) {
+  fprintf(stderr, "pthread_create failed\n");
+  abort();
+}
+
 int fd = open("lock.txt", O_WRONLY|O_CREAT|O_TRUNC, 0755);
 if (fd < 0)
   abort();


This behaviour makes no sense at all to time. Why should it matter if
the thread exits itself, or is force exited during execve(). I wonder
if it is even possibly a kernel bug.


I'll attach the reproducer to an internal bug (sorry!), but will report back 
here with any findings.


Regards,
Jim

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

Re: [libvirt] [PATCH v5 06/12] qemu: Validate PCI controller options (pcihole64)

2018-03-02 Thread Laine Stump

On 03/02/2018 10:13 AM, Andrea Bolognani wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1483816
>
> Signed-off-by: Andrea Bolognani 

Reviewed-by: Laine Stump 

> ---
>  src/qemu/qemu_domain.c | 40 
>  1 file changed, 40 insertions(+)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 54e47acd99..9b8d2e864d 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4685,6 +4685,46 @@ qemuDomainDeviceDefValidateControllerPCI(const 
> virDomainControllerDef *cont,
>  return -1;
>  }
>  
> +/* pcihole64 */
> +switch ((virDomainControllerModelPCI) cont->model) {
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
> +/* The pcihole64 option only applies to x86 guests */
> +if ((pciopts->pcihole64 ||
> + pciopts->pcihole64size != 0) &&
> +!ARCH_IS_X86(def->os.arch)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("Option '%s' is not valid for '%s' controller "
> + "on '%s' architecture or '%s' machine type"),
> +   "pcihole64", model,
> +   virArchToString(def->os.arch), def->os.machine);
> +return -1;
> +}
> +break;
> +
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
> +case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
> +if (pciopts->pcihole64 ||
> +pciopts->pcihole64size != 0) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("Option '%s' is not valid for '%s' controller"),
> +   "pcihole64", model);
> +return -1;
> +}
> +break;
> +
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
> +default:
> +virReportEnumRangeError(virDomainControllerModelPCI, cont->model);
> +return -1;
> +}
> +
>  return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps);
>  }
>  


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

Re: [libvirt] [PATCH v5 02/12] qemu: Create new qemuDomainDeviceDefValidateControllerPCI()

2018-03-02 Thread Laine Stump

On 03/02/2018 10:13 AM, Andrea Bolognani wrote:
> The esisting function is renamed and called from the new one, so

Forgot to mention this before:

  s/esisting/existing/

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

Re: [libvirt] [PATCH v5 05/12] qemu: Validate PCI controller options (targetIndex)

2018-03-02 Thread Laine Stump

On 03/02/2018 10:13 AM, Andrea Bolognani wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1483816
>
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_domain.c | 52 
> --
>  1 file changed, 46 insertions(+), 6 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 3ef5d74e7a..54e47acd99 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4401,12 +4401,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const 
> virDomainControllerDef *contro
>  break;
>  
>  case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> -if (pciopts->targetIndex == -1) {
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -   _("autogenerated pci-root options not set"));
> -return -1;
> -}
> -
>  /* Skip the implicit one */
>  if (pciopts->targetIndex == 0)
>  return 0;
> @@ -4645,6 +4639,52 @@ qemuDomainDeviceDefValidateControllerPCI(const 
> virDomainControllerDef *cont,
>  return -1;
>  }
>  
> +/* targetIndex */
> +switch ((virDomainControllerModelPCI) cont->model) {
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> +/* PHBs for pSeries guests must have been assigned a targetIndex */
> +if (pciopts->targetIndex == -1 &&
> +pciopts->modelName == 
> VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Option '%s' not set for '%s' controller"),
> +   "targetIndex", model);

This is the option I was talking about in the review for 03/12. As it
is, the message is going to say:

   Option 'targetIndex' not set for 'pci-root' controller

It would be better if it were something like

    Required option 'targetIndex' not set for
'spapr-pci-host-bridge' controller

(Maybe there's a way to work in 'pci-root' to the message too. It's too
late on Friday afternoon for me to try and figure it out).

Otherwise it fits with the pattern of everything else, so

Reviewed-by: Laine Stump 

> +return -1;
> +}
> +
> +/* targetIndex only applies to PHBs, so for any other pci-root
> + * controller it being present is an error */
> +if (pciopts->targetIndex != -1 &&
> +pciopts->modelName != 
> VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("Option '%s' is not valid for '%s' controller"),
> +   "targetIndex", model);
> +return -1;
> +}
> +break;
> +
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
> +case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
> +if (pciopts->targetIndex != -1) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("Option '%s' is not valid for '%s' controller"),
> +   "targetIndex", model);
> +return -1;
> +}
> +break;
> +
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
> +default:
> +virReportEnumRangeError(virDomainControllerModelPCI, cont->model);
> +return -1;
> +}
> +
>  return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps);
>  }
>  


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

Re: [libvirt] [PATCH] libxl: don't hardcode scheduler weight

2018-03-02 Thread John Ferlan



On 02/22/2018 05:20 PM, Jim Fehlig wrote:
> Long ago in commit dfa1e1dd53 the scheduler weight was accidentally
> hardcoded to 1000. Weight is a setting with no unit since it is
> relative to the weight of other domains. If no weight is specified,
> libxl defaults to 256.
> 
> Instead of hardcoding the weight to 1000, honor any  specified
> in . libvirt's notion of shares is synonomous to libxl's
> scheduler weight setting. If shares is unspecified, defer default
> weight setting to libxl.
> 
> Removing the hardcoded weight required some test fixup. While at it,
> add an explicit test for  conversion to scheduler weight.
> 
> Signed-off-by: Jim Fehlig 
> ---
> 
> Honoring  specified by the user is certainly desirable,
> but I'm not sure about changing the default weight. One problematic
> scenario that came to mind: Several domains started by pre-patch
> libvirtd with weight=1000, update libvirtd with patch, start more
> domains with weight=256. The pre-patch domains unknowingly have
> a weight nearly 4 times that of post-patch domains.

Put 'em all on a diet.  Don't try to do it overnight though because that
never works and it makes them all hangry...  Slowly try to have them
lose weight over the course a few months and in no time they'll be slim,
trim, and less cumbersome. Whether they'll be happy - who knows ;-)

> 
>  src/libxl/libxl_conf.c |  4 +-
>  tests/libxlxml2domconfigdata/basic-hvm.json|  2 +-
>  tests/libxlxml2domconfigdata/basic-pv.json |  2 +-
>  tests/libxlxml2domconfigdata/cpu-shares-hvm.json   | 89 
> ++
>  tests/libxlxml2domconfigdata/cpu-shares-hvm.xml| 39 ++
>  tests/libxlxml2domconfigdata/moredevs-hvm.json |  2 +-
>  tests/libxlxml2domconfigdata/multiple-ip.json  |  2 +-
>  .../libxlxml2domconfigdata/variable-clock-hvm.json |  2 +-
>  tests/libxlxml2domconfigdata/vnuma-hvm.json|  2 +-
>  tests/libxlxml2domconfigtest.c |  1 +
>  10 files changed, 138 insertions(+), 7 deletions(-)
> 

Perhaps this is a "news" worthy type change along with the caveat about
those more weighty domains that were allowed to feed at the trough
without being limited.

In any case, this all seems reasonable to me, but it also seems you have
a self inflicted merge conflict.

Without compiling, via visual inspection:

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 1/2] conf: nodedev: Don't refresh host caps in testdriver

2018-03-02 Thread Cole Robinson

On 03/02/2018 04:02 PM, John Ferlan wrote:
> 
> 
> On 02/23/2018 06:16 PM, Cole Robinson wrote:
>> Add a 'testdriver' bool that we set for test_driver.c nodedevs
>> which will skip accessing host resources via virNodeDeviceUpdateCaps
>>
>> Signed-off-by: Cole Robinson 
>> ---
>>  src/conf/node_device_conf.c | 3 +++
>>  src/conf/node_device_conf.h | 1 +
>>  src/test/test_driver.c  | 2 ++
>>  3 files changed, 6 insertions(+)
>>
>> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
>> index fd8f4e4a9..90c940f11 100644
>> --- a/src/conf/node_device_conf.c
>> +++ b/src/conf/node_device_conf.c
>> @@ -2425,6 +2425,9 @@ virNodeDeviceUpdateCaps(virNodeDeviceDefPtr def)
>>  {
>>  virNodeDevCapsDefPtr cap = def->caps;
>>  
>> +if (def->testdriver)
>> +return 0;
>> +
>>  while (cap) {
>>  switch (cap->data.type) {
>>  case VIR_NODE_DEV_CAP_SCSI_HOST:
>> diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
>> index 685ae3034..665f766e2 100644
>> --- a/src/conf/node_device_conf.h
>> +++ b/src/conf/node_device_conf.h
>> @@ -316,6 +316,7 @@ struct _virNodeDeviceDef {
>>  char *driver;   /* optional driver name */
>>  char *devnode;  /* /dev path */
>>  char **devlinks;/* /dev links */
>> +bool testdriver;/* if true, skip host checks */
> 
> Not sure this should be in virNodeDeviceDef... I think it should be in
> virNodeDeviceObj. Yes, a bit more work, but I think cleaner. You'd need
> to create an accessor function in order to set the flag from
> test_driver. Then avoid calling virNodeDeviceUpdateCaps only from
> virNodeDeviceMatch if the flag is set.
> 
> Also instead of "testdriver", how about "skipUpdateCaps" since the
> purpose of this is to skip calling virNodeDeviceUpdateCaps?
> 
> At least that hides that this currently is only for the test driver.
> Perhaps in the future there could be some other reason to not want to
> update the caps for some specific definition after perhaps it's "known"
> or "determined" that a specific update had occurred.

Okay, thanks for review. How about finding a way to remove UpdateCaps
from generic conf.c implementations instead? Seems wrong that something
functions needed to implement ListAllDevices touch host resources, I
don't think other objects work like that

Thanks,
Cole

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

Re: [libvirt] [PATCH v5 04/12] qemu: Validate PCI controller options (index)

2018-03-02 Thread Laine Stump

On 03/02/2018 10:13 AM, Andrea Bolognani wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1483816
>
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_domain.c | 68 
> +++---
>  1 file changed, 43 insertions(+), 25 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 13c2b557fb..3ef5d74e7a 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4285,31 +4285,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const 
> virDomainControllerDef *contro
>  controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT)
>  return 0;
>  
> -/* First pass - just check the controller index for the model's
> - * that we care to check... */
> -switch (model) {
> -case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
> -case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
> -case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
> -case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
> -case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
> -case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
> -case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
> -if (controller->idx == 0) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -   _("index for pci controllers of model '%s' must 
> be > 0"),
> -   virDomainControllerModelPCITypeToString(model));
> -return -1;
> -}
> -break;
> -
> -case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> -case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
> -case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT:
> -case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
> -break;
> -}
> -
>  pciopts = >opts.pciopts;
>  
>  /* Second pass - now the model specific checks */
> @@ -4627,6 +4602,49 @@ qemuDomainDeviceDefValidateControllerPCI(const 
> virDomainControllerDef *cont,
>  return -1;
>  }
>  
> +/* index */
> +switch ((virDomainControllerModelPCI) cont->model) {
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
> +case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
> +if (cont->idx == 0) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("Index for '%s' controllers must be > 0"),
> +   model);
> +return -1;
> +}
> +break;
> +
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
> +/* pSeries guests can have multiple PHBs, so it's expected that
> + * the index will not be zero for some of them */
> +if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT &&
> +pciopts->modelName == 
> VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) {
> +break;
> +}
> +
> +/* For all other pci-root and pcie-root controllers, though,
> + * the index must be zero*/
> +if (cont->idx != 0) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("Index for '%s' controllers must be 0"),
> +   model);
> +return -1;
> +}
> +break;
> +
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
> +default:
> +virReportEnumRangeError(virDomainControllerModelPCI, cont->model);
> +return -1;
> +}
> +
>  return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps);
>  }
>  


Reviewed-by: Laine Stump 


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

Re: [libvirt] [PATCH v5 03/12] qemu: Validate PCI controller options (modelName)

2018-03-02 Thread Laine Stump

On 03/02/2018 10:13 AM, Andrea Bolognani wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1483816
>
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_domain.c | 229 
> +++--
>  1 file changed, 145 insertions(+), 84 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index a45a676520..13c2b557fb 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4274,7 +4274,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const 
> virDomainControllerDef *contro
>  {
>  virDomainControllerModelPCI model = controller->model;
>  const virDomainPCIControllerOpts *pciopts;
> -const char *modelName = NULL;
>  
>  /* skip pcie-root */
>  if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)
> @@ -4312,24 +4311,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const 
> virDomainControllerDef *contro
>  }
>  
>  pciopts = >opts.pciopts;
> -if (controller->model != VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT &&
> -controller->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST) {
> -if (pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) 
> {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> -   _("autogenerated %s options not set"),
> -   
> virDomainControllerModelPCITypeToString(controller->model));
> -return -1;
> -}
> -
> -modelName = 
> virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
> -if (!modelName) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> -   _("unknown %s modelName value %d"),
> -   
> virDomainControllerModelPCITypeToString(controller->model),
> -   pciopts->modelName);
> -return -1;
> -}
> -}
>  
>  /* Second pass - now the model specific checks */
>  switch (model) {
> @@ -4340,14 +4321,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const 
> virDomainControllerDef *contro
>  return -1;
>  }
>  
> -if (pciopts->modelName != 
> VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -   _("PCI controller model name '%s' is not valid "
> - "for a pci-bridge"),
> -   modelName);
> -return -1;
> -}
> -
>  if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("the pci-bridge controller is not supported "
> @@ -4364,14 +4337,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const 
> virDomainControllerDef *contro
>  return -1;
>  }
>  
> -if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -   _("PCI controller model name '%s' is not valid "
> - "for a pci-expander-bus"),
> -   modelName);
> -return -1;
> -}
> -
>  if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB)) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("the pxb controller is not supported in this "
> @@ -4382,14 +4347,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const 
> virDomainControllerDef *contro
>  break;
>  
>  case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
> -if (pciopts->modelName != 
> VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -   _("PCI controller model name '%s' is not valid "
> - "for a dmi-to-pci-bridge"),
> -   modelName);
> -return -1;
> -}
> -
>  if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("the dmi-to-pci-bridge (i82801b11-bridge) "
> @@ -4406,15 +4363,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const 
> virDomainControllerDef *contro
>  return -1;
>  }
>  
> -if ((pciopts->modelName != 
> VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) &&
> -(pciopts->modelName != 
> VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT)) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -   _("PCI controller model name '%s' is not valid "
> - "for a pcie-root-port"),
> -   modelName);
> -return -1;
> -}
> -
>  if ((pciopts->modelName == 
> VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) &&
>  !virQEMUCapsGet(qemuCaps,

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

2018-03-02 Thread John Ferlan



On 02/26/2018 05:20 AM, Shalini Chellathurai Saroja wrote:
> Let us introduce the xml and reply files for QEMU 2.11.0 on s390x.
> 
> Signed-off-by: Shalini Chellathurai Saroja 
> Reviewed-by: Bjoern Walk 
> Reviewed-by: Boris Fiuczynski 
> ---
>  .../qemucapabilitiesdata/caps_2.11.0.s390x.replies | 18008 
> +++
>  tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml   |  2641 +++
>  tests/qemucapabilitiestest.c   | 1 +
>  3 files changed, 20650 insertions(+)
>  create mode 100644 tests/qemucapabilitiesdata/caps_2.11.0.s390x.replies
>  create mode 100644 tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml
> 

Looks reasonable to me... since Bjoern and Boris have also reviewed I'll
push once the tree is not frozen...

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 2/2] test: Implement virConnectListAllNodeDevices

2018-03-02 Thread John Ferlan



On 02/23/2018 06:16 PM, Cole Robinson wrote:
> Signed-off-by: Cole Robinson 
> ---
>  src/test/test_driver.c | 13 +
>  1 file changed, 13 insertions(+)
> 
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 39784c9fa..844e99dd7 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -5385,6 +5385,18 @@ testNodeListDevices(virConnectPtr conn,
>  cap, names, maxnames);
>  }
>  
> +static int
> +testConnectListAllNodeDevices(virConnectPtr conn,
> +  virNodeDevicePtr **devices,
> +  unsigned int flags)
> +{
> +testDriverPtr driver = conn->privateData;
> +
> +virCheckFlags(VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_CAP, -1);
> +
> +return virNodeDeviceObjListExport(conn, driver->devs, devices,
> +  NULL, flags);
> +}
>  
>  static virNodeDevicePtr
>  testNodeDeviceLookupByName(virConnectPtr conn, const char *name)
> @@ -7022,6 +7034,7 @@ static virStorageDriver testStorageDriver = {
>  };
>  
>  static virNodeDeviceDriver testNodeDeviceDriver = {
> +.connectListAllNodeDevices = testConnectListAllNodeDevices, /* 4.1.0 */

At least 4.2.0 now

Reviewed-by: John Ferlan 

John

>  .connectNodeDeviceEventRegisterAny = 
> testConnectNodeDeviceEventRegisterAny, /* 2.2.0 */
>  .connectNodeDeviceEventDeregisterAny = 
> testConnectNodeDeviceEventDeregisterAny, /* 2.2.0 */
>  .nodeNumOfDevices = testNodeNumOfDevices, /* 0.7.2 */
> 

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

Re: [libvirt] [PATCH 1/2] conf: nodedev: Don't refresh host caps in testdriver

2018-03-02 Thread John Ferlan



On 02/23/2018 06:16 PM, Cole Robinson wrote:
> Add a 'testdriver' bool that we set for test_driver.c nodedevs
> which will skip accessing host resources via virNodeDeviceUpdateCaps
> 
> Signed-off-by: Cole Robinson 
> ---
>  src/conf/node_device_conf.c | 3 +++
>  src/conf/node_device_conf.h | 1 +
>  src/test/test_driver.c  | 2 ++
>  3 files changed, 6 insertions(+)
> 
> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
> index fd8f4e4a9..90c940f11 100644
> --- a/src/conf/node_device_conf.c
> +++ b/src/conf/node_device_conf.c
> @@ -2425,6 +2425,9 @@ virNodeDeviceUpdateCaps(virNodeDeviceDefPtr def)
>  {
>  virNodeDevCapsDefPtr cap = def->caps;
>  
> +if (def->testdriver)
> +return 0;
> +
>  while (cap) {
>  switch (cap->data.type) {
>  case VIR_NODE_DEV_CAP_SCSI_HOST:
> diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
> index 685ae3034..665f766e2 100644
> --- a/src/conf/node_device_conf.h
> +++ b/src/conf/node_device_conf.h
> @@ -316,6 +316,7 @@ struct _virNodeDeviceDef {
>  char *driver;   /* optional driver name */
>  char *devnode;  /* /dev path */
>  char **devlinks;/* /dev links */
> +bool testdriver;/* if true, skip host checks */

Not sure this should be in virNodeDeviceDef... I think it should be in
virNodeDeviceObj. Yes, a bit more work, but I think cleaner. You'd need
to create an accessor function in order to set the flag from
test_driver. Then avoid calling virNodeDeviceUpdateCaps only from
virNodeDeviceMatch if the flag is set.

Also instead of "testdriver", how about "skipUpdateCaps" since the
purpose of this is to skip calling virNodeDeviceUpdateCaps?

At least that hides that this currently is only for the test driver.
Perhaps in the future there could be some other reason to not want to
update the caps for some specific definition after perhaps it's "known"
or "determined" that a specific update had occurred.

John

>  virNodeDevCapsDefPtr caps;  /* optional device capabilities */
>  };
>  
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 043caa976..39784c9fa 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -1165,6 +1165,7 @@ testParseNodedevs(testDriverPtr privconn,
>  if (!def)
>  goto error;
>  
> +def->testdriver = true;
>  if (!(obj = virNodeDeviceObjListAssignDef(privconn->devs, def))) {
>  virNodeDeviceDefFree(def);
>  goto error;
> @@ -5565,6 +5566,7 @@ testNodeDeviceMockCreateVport(testDriverPtr driver,
>  caps = caps->next;
>  }
>  
> +def->testdriver = true;
>  if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def)))
>  goto cleanup;
>  def = NULL;
> 

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

Re: [libvirt] [PATCH v5 02/12] qemu: Create new qemuDomainDeviceDefValidateControllerPCI()

2018-03-02 Thread Laine Stump

On 03/02/2018 10:13 AM, Andrea Bolognani wrote:
> The esisting function is renamed and called from the new one, so
> that even while we're in the process of implementing new checks
> all the existing ones will be performed.
>
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_domain.c | 29 ++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 8b4efc82de..a45a676520 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4268,9 +4268,9 @@ qemuDomainDeviceDefValidateControllerSCSI(const 
> virDomainControllerDef *controll
>  
>  
>  static int
> -qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef 
> *controller,
> - const virDomainDef *def,
> - virQEMUCapsPtr qemuCaps)
> +qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef 
> *controller,
> +const virDomainDef *def,
> +virQEMUCapsPtr qemuCaps)
>  {
>  virDomainControllerModelPCI model = controller->model;
>  const virDomainPCIControllerOpts *pciopts;
> @@ -4547,6 +4547,29 @@ qemuDomainDeviceDefValidateControllerPCI(const 
> virDomainControllerDef *controlle
>  }
>  
>  
> +static int
> +qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont,
> + const virDomainDef *def,
> + virQEMUCapsPtr qemuCaps)
> +
> +{
> +const virDomainPCIControllerOpts *pciopts = >opts.pciopts;
> +const char *model = virDomainControllerModelPCITypeToString(cont->model);
> +const char *modelName = 
> virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
> +
> +if (!model) {
> +virReportEnumRangeError(virDomainControllerModelPCI, cont->model);
> +return -1;
> +}
> +if (!modelName) {
> +virReportEnumRangeError(virDomainControllerPCIModelName, 
> pciopts->modelName);
> +return -1;
> +}
> +
> +return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps);
> +}
> +
> +
>  static int
>  qemuDomainDeviceDefValidateControllerSATA(const virDomainControllerDef 
> *controller,
>const virDomainDef *def,


Sure. Seems as reasonable a way as any to add the new and remove the old
without ending up with confusing diffs.


Reviewed-by: Laine Stump 


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

Re: [libvirt] [PATCH v5 01/12] conf: Assign explicit value to VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE

2018-03-02 Thread Laine Stump

On 03/02/2018 10:13 AM, Andrea Bolognani wrote:
> Pretty much any reasonable compiler would do this automatically,
> but there's no harm in being explicit about it.
>
> Signed-off-by: Andrea Bolognani 
> ---
>  src/conf/domain_conf.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 368f16f3fb..a04f96169c 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -700,7 +700,7 @@ typedef enum {
>  } virDomainControllerModelPCI;
>  
>  typedef enum {
> -VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE,
> +VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE = 0,
>  VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE,
>  VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE,
>  VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420,

I'm not sure why you want it, but don't see any harm (if it's for
consistency, there are many other enums that don't explicitly list the
first item as 0).

Reviewed-by: Laine Stump 

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

Re: [libvirt] [PATCH] virtlockd: acquire locks on re-exec

2018-03-02 Thread Daniel P . Berrangé

On Fri, Mar 02, 2018 at 04:52:23PM +, Daniel P. Berrangé wrote:
> On Thu, Mar 01, 2018 at 04:42:36PM -0700, Jim Fehlig wrote:
> > Locks held by virtlockd are dropped on re-exec.
> > 
> > virtlockd   94306 POSIX 5.4G WRITE 0 0   0 /tmp/test.qcow2
> > virtlockd   94306 POSIX   5B WRITE 0 0   0 /run/virtlockd.pid
> > virtlockd   94306 POSIX   5B WRITE 0 0   0 /run/virtlockd.pid
> > 
> > Acquire locks in PostExecRestart code path.
> 
> This is really strange and should *not* be happening.  POSIX locks
> are supposed to be preserved across execve() if the FD has CLOEXEC
> unset, and you don't fork() before the exec.

[snip]

> So I wonder what we've screwed up to cause the locks to get
> released - reaquiring them definitely isn't desirable as we
> should not loose them in the first place !

This is really very strange. The problem seems to be the existance of
threads at time of execve().

If you spawn a thread and the thread exits, and you execve the locks
are preserved.

If you spawn a thread and the thread is still running, and you execve
the locks are lost.

This behaviour makes no sense at all to time. Why should it matter if
the thread exits itself, or is force exited during execve(). I wonder
if it is even possibly a kernel bug.

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

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

Re: [libvirt] [PATCH v2 11/12] qemu_hotplug: Hotunplug of reservations

2018-03-02 Thread John Ferlan



On 02/21/2018 01:11 PM, Michal Privoznik wrote:
> Just line in previous commit, if we are the last ones that are
> using the pr-manager delete it and also kill the pr-helper
> process.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c  | 19 +++
>  src/qemu/qemu_domain.h  |  3 +++
>  src/qemu/qemu_hotplug.c | 22 ++
>  3 files changed, 44 insertions(+)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index f7da62dba..acfa1d88c 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -11650,6 +11650,25 @@ 
> qemuDomainCheckMigrationCapabilities(virQEMUDriverPtr driver,
>  }
>  
>  
> +size_t
> +qemuDomainGetPRDManagedCount(const virDomainDef *def)
> +{
> +size_t i;
> +size_t nmanaged = 0;
> +
> +for (i = 0; i < def->ndisks; i++) {
> +virDomainDiskDefPtr disk = def->disks[i];
> +
> +if (!virStoragePRDefIsManaged(disk->src->pr))
> +continue;
> +
> +nmanaged++;
> +}
> +
> +return nmanaged;
> +}
> +
> +

The above feels like it could have gone in much earlier and could have
been useful for the qemuProcessKillPRDaemons changes.

>  static int
>  qemuDomainPrepareDiskPRD(qemuDomainObjPrivatePtr priv,
>   virDomainDiskDefPtr disk)
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 418b47153..c2f67f379 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -1007,6 +1007,9 @@ qemuDomainCheckMigrationCapabilities(virQEMUDriverPtr 
> driver,
>   virDomainObjPtr vm,
>   qemuDomainAsyncJob asyncJob);
>  
> +size_t
> +qemuDomainGetPRDManagedCount(const virDomainDef *def);
> +
>  int
>  qemuDomainPrepareDiskSource(virDomainDiskDefPtr disk,
>  qemuDomainObjPrivatePtr priv,
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 2ebb68fbc..d8460cdb4 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -400,6 +400,13 @@ qemuDestroyPRDefObject(virDomainObjPtr vm,
>  }
>  
>  
> +static bool
> +qemuDomainDiskNeedRemovePR(virDomainObjPtr vm)
> +{
> +return qemuDomainGetPRDManagedCount(vm->def) == 1;
> +}
> +
> +>  /**
>   * qemuDomainAttachDiskGeneric:
>   *
> @@ -3812,6 +3819,8 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
>  char *drivestr;
>  char *objAlias = NULL;
>  char *encAlias = NULL;
> +const char *prAlias = NULL;
> +const char *prPath = NULL;
>  
>  VIR_DEBUG("Removing disk %s from domain %p %s",
>disk->info.alias, vm, vm->def->name);
> @@ -3849,6 +3858,14 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
>  }
>  }
>  
> +if (qemuDomainDiskNeedRemovePR(vm)) {
> +qemuDomainStorageSourcePrivatePtr srcPriv;
> +
> +srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
> +prAlias = srcPriv->prd->alias;
> +prPath = srcPriv->prd->path;
> +}
> +

As noted in previous patch - should we really care? It's mostly a
thrashing concern, but here you don't really even remove the object if
this isn't the last one using PR.

>  qemuDomainObjEnterMonitor(driver, vm);
>  
>  qemuMonitorDriveDel(priv->mon, drivestr);
> @@ -3864,6 +3881,9 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
>  ignore_value(qemuMonitorDelObject(priv->mon, encAlias));
>  VIR_FREE(encAlias);
>  
> +if (prAlias)
> +ignore_value(qemuMonitorDelObject(priv->mon, prAlias));
> +

Since you only set prAlias when you'll be needing to remove the PR, thus
if there was more than one lun w/ PR, you wouldn't delete the object.

>  if (disk->src->haveTLS)
>  ignore_value(qemuMonitorDelObject(priv->mon, disk->src->tlsAlias));
>  
> @@ -3882,6 +3902,8 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
>  }
>  }
>  
> +qemuDestroyPRDefObject(vm, prAlias, prPath);
> +

If the two patches were combined it'd be more easier to see, but this
would also not unlink the socket path... If there were 2 luns using PR
and just 1 was removed, we wouldn't delete the object and prAlias would
still be NULL meaning qemuDestroyPRDefObject returns immediately and
nothing really happens.

Again, the whole socket path and managed vs. unmanaged is not very clear
at least to me.

John

>  qemuDomainReleaseDeviceAddress(vm, >info, src);
>  
>  if (qemuSecurityRestoreDiskLabel(driver, vm, disk) < 0)
> 

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

Re: [libvirt] [PATCH v2 12/12] qemu: Detect pr-manager-helper capability

2018-03-02 Thread John Ferlan



On 02/21/2018 01:11 PM, Michal Privoznik wrote:
> The capability tracks if qemu has pr-manager-helper object.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_capabilities.c | 1 +
>  1 file changed, 1 insertion(+)
> 

As noted earlier - this should just be merged with patch 3.

John

> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index d0005c71d..21fbecbf6 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -1695,6 +1695,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] 
> = {
>  { "sclplmconsole", QEMU_CAPS_DEVICE_SCLPLMCONSOLE },
>  { "isa-serial", QEMU_CAPS_DEVICE_ISA_SERIAL },
>  { "pl011", QEMU_CAPS_DEVICE_PL011 },
> +{ "pr-manager-helper", QEMU_CAPS_PR_MANAGER_HELPER },
>  };
>  
>  static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBalloon[] = 
> {
> 

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

Re: [libvirt] [PATCH v2 10/12] qemu_hotplug: Hotplug of reservations

2018-03-02 Thread John Ferlan



On 02/21/2018 01:11 PM, Michal Privoznik wrote:
> Surprisingly, nothing special is happening here. If we are the
> first to use the managed helper then spawn it. If not, we're
> almost done.

But wasn't there a very special reason to start it between fork and
exec? Does that still hold true? That is why can we start the daemon
after exec now, but we couldn't before in the previous patch?

It feels quite "disjoint" to have the unplug in a subsequent patch too.
Considering them both in one just seems "better", but it's not a deal
breaker.

> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_hotplug.c | 72 
> +
>  src/qemu/qemu_process.c | 38 +-
>  src/qemu/qemu_process.h |  7 +
>  3 files changed, 110 insertions(+), 7 deletions(-)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index f28006e3c..2ebb68fbc 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -348,6 +348,58 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
>  }
>  
>  
> +static int
> +qemuBuildPRDefInfoProps(virDomainObjPtr vm,

qemuBuild?  Why not qemuDomainAdd?

> +virDomainDiskDefPtr disk,
> +virJSONValuePtr *prmgrProps,
> +const char **prAlias,
> +const char **prPath)
> +{
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +qemuDomainStorageSourcePrivatePtr srcPriv;
> +virJSONValuePtr props = NULL;
> +int ret = -1;
> +
> +srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);

ohh, umm, in qemuDomainAttachDiskGeneric there's a :

 srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
 if (srcPriv) {
 secinfo = srcPriv->secinfo;
 encinfo = srcPriv->encinfo;
 }

Which makes light dawn that it "was" possible for srcPriv == NULL and
the "thought" that the subsequent deref is going to fail rather
spectacularly. See commit '8056721cb' (and a couple of others).

Although it seems patch 4 and your change to qemuDomainPrepareDiskSource
to call qemuDomainStorageSourcePrivateNew instead of having it called in
qemuDomainSecretStorageSourcePrepare would seem to ensure that disk
source privateData is always allocated now - meaning a number of other
places where srcPriv is/was checked are no longer necessary.

Maybe that one change needs to be "extracted" out to make things
obvious. Not required, but just thinking and typing thoughts.

> +
> +*prmgrProps = NULL;
> +
> +if (priv->prPid != (pid_t) -1 ||
> +!srcPriv->prd ||
> +!srcPriv->prd->alias)
> +return 0;
> +
> +if (virJSONValueObjectCreate(,
> + "s:path", srcPriv->prd->path,
> + NULL) < 0)
> +goto cleanup;
> +> +if (qemuProcessSetupOnePRDaemon(vm, disk) < 0)
> +goto cleanup;> +
> +*prAlias = srcPriv->prd->alias;
> +*prPath = srcPriv->prd->path;
> +*prmgrProps = props;
> +props = NULL;
> +ret = 0;
> + cleanup:
> +virJSONValueFree(props);
> +return ret;
> +}
> +
> +
> +static void
> +qemuDestroyPRDefObject(virDomainObjPtr vm,

qemuDestroy?  Why not qemuDomainDel?

> +   const char *alias,
> +   const char *path)
> +{
> +if (!alias)
> +return;

BTW: This is where I'd expect to remove the object and then...

> +
> +qemuProcessKillPRDaemons(vm, path, false);

Just unlink the path...  See some thoughts below...

> +}
> +
> +
>  /**
>   * qemuDomainAttachDiskGeneric:
>   *
> @@ -365,12 +417,16 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>  char *devstr = NULL;
>  char *drivestr = NULL;
>  char *drivealias = NULL;
> +const char *prAlias = NULL;
> +const char *prPath = NULL;
>  bool driveAdded = false;
>  bool secobjAdded = false;
>  bool encobjAdded = false;
> +bool prmgrAdded = false;
>  virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>  virJSONValuePtr secobjProps = NULL;
>  virJSONValuePtr encobjProps = NULL;
> +virJSONValuePtr prmgrProps = NULL;
>  qemuDomainStorageSourcePrivatePtr srcPriv;
>  qemuDomainSecretInfoPtr secinfo = NULL;
>  qemuDomainSecretInfoPtr encinfo = NULL;
> @@ -403,6 +459,9 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>disk->info.alias) < 0)
>  goto error;
>  
> +if (qemuBuildPRDefInfoProps(vm, disk, , , ) < 
> 0)
> +goto error;
> +
>  if (!(drivestr = qemuBuildDriveStr(disk, false, priv->qemuCaps)))
>  goto error;
>  
> @@ -435,6 +494,15 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>  encobjAdded = true;
>  }
>  
> +if (prmgrProps) {
> +rv = qemuMonitorAddObject(priv->mon, "pr-manager-helper", prAlias,
> +  prmgrProps);
> +prmgrProps

[libvirt] [PATCH 7/7] tests: qemuxml2xml: Add status XML with outgoing migration with NBD

2018-03-02 Thread Peter Krempa

Signed-off-by: Peter Krempa 
---
 .../qemustatusxml2xmldata/migration-out-nbd-in.xml | 449 +
 .../migration-out-nbd-out.xml  | 449 +
 tests/qemuxml2xmltest.c|   1 +
 3 files changed, 899 insertions(+)
 create mode 100644 tests/qemustatusxml2xmldata/migration-out-nbd-in.xml
 create mode 100644 tests/qemustatusxml2xmldata/migration-out-nbd-out.xml

diff --git a/tests/qemustatusxml2xmldata/migration-out-nbd-in.xml 
b/tests/qemustatusxml2xmldata/migration-out-nbd-in.xml
new file mode 100644
index 00..6d87c1ec5c
--- /dev/null
+++ b/tests/qemustatusxml2xmldata/migration-out-nbd-in.xml
@@ -0,0 +1,449 @@
+
+  
+  
+  
+
+
+  
+  
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+  
+  
+
+
+  
+  
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+  
+  
+  
+  
+  
+  
+  
+  
+upstream
+dcf47dbd-46d1-4d5b-b442-262a806a333a
+1024000
+1024000
+
+  
+
+8
+
+  
+
+
+  /machine
+
+
+  hvm
+  
+
+
+  
+  
+  
+
+
+  
+
+
+  
+
+
+  
+  
+  
+
+destroy
+restart
+restart
+
+  
+  
+
+
+  /usr/bin/qemu-system-x86_64
+  
+
+
+
+  
+  
+
+  base.qcow2
+
+  
+  
+
+
+
+
+  
+  
+
+
+
+
+
+
+
+
+  
+  
+
+
+  
+  
+
+
+
+  
+  
+
+
+
+  
+  
+
+
+
+  
+  
+
+  
+  
+
+
+  
+  
+
+
+  
+  
+
+
+  
+  
+
+  
+  
+
+
+
+  
+
+
+
+
+
+  
+  
+
+
+  
+
+
+  
+  
+
+
+
+  
+  
+
+
+
+
+  
+  
+
+
+
+  
+  
+
+
+  
+  
+
+  
+  
+
+  
+  
+
+
+  
+  
+
+
+  
+  
+
+
+
+  
+  
+
+
+  
+  
+
+
+  
+  
+
+
+  
+  
+/dev/random
+
+
+  
+
+
+  +0:+0
+  +0:+0
+
+  
+
diff --git a/tests/qemustatusxml2xmldata/migration-out-nbd-out.xml 
b/tests/qemustatusxml2xmldata/migration-out-nbd-out.xml
new file mode 100644
index 00..6d87c1ec5c
--- /dev/null
+++ b/tests/qemustatusxml2xmldata/migration-out-nbd-out.xml
@@ -0,0 +1,449 @@
+
+  
+  
+  
+
+
+  
+  
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+

[libvirt] [PATCH 4/7] tests: qemuxml2xml: Remove fake status XML testing

2018-03-02 Thread Peter Krempa

Now that the better approach is in place we can remove the old functions
doing the fake formatting.

Signed-off-by: Peter Krempa 
---
 tests/qemuxml2xmltest.c | 218 +---
 1 file changed, 1 insertion(+), 217 deletions(-)

diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index cf9288db72..980d7fb0b3 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -39,29 +39,6 @@ struct testInfo {
 virQEMUCapsPtr qemuCaps;
 };

-static int
-qemuXML2XMLActivePreFormatCallback(virDomainDefPtr def,
-   const void *opaque)
-{
-struct testInfo *info = (struct testInfo *) opaque;
-size_t i;
-
-/* store vCPU bitmap so that the status XML can be created faithfully */
-if (!info->activeVcpus)
-info->activeVcpus = virDomainDefGetOnlineVcpumap(def);
-
-info->blockjobs = false;
-
-/* remember whether we have mirror jobs */
-for (i = 0; i < def->ndisks; i++) {
-if (def->disks[i]->mirror) {
-info->blockjobs = true;
-break;
-}
-}
-
-return 0;
-}

 static int
 testXML2XMLActive(const void *opaque)
@@ -70,7 +47,7 @@ testXML2XMLActive(const void *opaque)

 return testCompareDomXML2XMLFiles(driver.caps, driver.xmlopt,
   info->inName, info->outActiveName, true,
-  qemuXML2XMLActivePreFormatCallback,
+  NULL,
   opaque, 0,
   TEST_COMPARE_DOM_XML2XML_RESULT_SUCCESS);
 }
@@ -88,195 +65,6 @@ testXML2XMLInactive(const void *opaque)
 }


-static const char testStatusXMLPrefixHeader[] =
-"\n"
-"  \n"
-"  \n";
-
-static const char testStatusXMLPrefixBodyStatic[] =
-"\n"
-"  \n"
-"  \n"
-"  \n"
-"  \n"
-"\n"
-"\n"
-"  \n"
-"  \n"
-"  \n"
-"  \n"
-"  \n"
-"\n"
-"\n"
-"\n"
-"\n"
-"\n";
-
-static const char testStatusXMLSuffix[] =
-"\n";
-
-
-static void
-testGetStatuXMLPrefixVcpus(virBufferPtr buf,
-   const struct testInfo *data)
-{
-ssize_t vcpuid = -1;
-
-virBufferAddLit(buf, "\n");
-virBufferAdjustIndent(buf, 2);
-
-/* Make sure we can format the fake vcpu list. The test will fail 
regardles. */
-if (data->activeVcpus) {
-while ((vcpuid = virBitmapNextSetBit(data->activeVcpus, vcpuid)) >= 0)
-virBufferAsprintf(buf, "\n",
-  vcpuid, vcpuid + 3803519);
-}
-
-virBufferAdjustIndent(buf, -2);
-virBufferAddLit(buf, "\n");
-}
-
-
-static void
-testGetStatusXMLAddBlockjobs(virBufferPtr buf,
- const struct testInfo *data)
-{
-virBufferAsprintf(buf, "\n",
-  
virTristateBoolTypeToString(virTristateBoolFromBool(data->blockjobs)));
-}
-
-
-static char *
-testGetStatusXMLPrefix(const struct testInfo *data)
-{
-virBuffer buf = VIR_BUFFER_INITIALIZER;
-
-virBufferAdd(, testStatusXMLPrefixHeader, -1);
-virBufferAdjustIndent(, 2);
-
-testGetStatuXMLPrefixVcpus(, data);
-
-virBufferAddStr(, testStatusXMLPrefixBodyStatic);
-
-testGetStatusXMLAddBlockjobs(, data);
-
-virBufferAdjustIndent(, -2);
-
-return virBufferContentAndReset();
-}
-
-
-static int
-testProcessStatusXML(virDomainObjPtr vm)
-{
-size_t i;
-
-/* fix the private 'blockjob' flag for disks */
-for (i = 0; i < vm->def->ndisks; i++) {
-virDomainDiskDefPtr disk = vm->def->disks[i];
-qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
-
-diskPriv->blockjob = !!disk->mirror;
-}
-
-return 0;
-}
-
-
-static int
-testCompareStatusXMLToXMLOldFiles(const void *opaque)
-{
-const struct testInfo *data = opaque;
-virBuffer buf = VIR_BUFFER_INITIALIZER;
-xmlDocPtr xml = NULL;
-virDomainObjPtr obj = NULL;
-char *expect = NULL;
-char *actual = NULL;
-char *source = NULL;
-char *header = NULL;
-char *inFile = NULL, *outActiveFile = NULL;
-int ret = -1;
-int keepBlanksDefault = xmlKeepBlanksDefault(0);
-
-if (virTestLoadFile(data->inName, ) < 0)
-goto cleanup;
-if (virTestLoadFile(data->outActiveName, ) < 0)
-goto cleanup;
-
-if (!(header = testGetStatusXMLPrefix(data)))
-goto cleanup;
-
-/* construct faked source status XML */
-virBufferAdd(, header, -1);
-virBufferAdjustIndent(, 2);
-virBufferAddStr(, inFile);
-virBufferAdjustIndent(, -2);
-virBufferAdd(, testStatusXMLSuffix, -1);
-
-if (!(source = virBufferContentAndReset())) {
-VIR_TEST_DEBUG("Failed to create the source XML");
-goto cleanup;
-}
-
-/* construct the expect string */
-virBufferAdd(, header, -1);
-virBufferAdjustIndent(, 2);
-virBufferAddStr(, outActiveFile);
-virBufferAdjustIndent(, -2);
-virBufferAdd(, testStatusXMLSuffix, -1);
-
-if (!(expect =

[libvirt] [PATCH 1/7] tests: qemuxml2xml: Rename testInfoFree to testInfoClear

2018-03-02 Thread Peter Krempa

Signed-off-by: Peter Krempa 
---
 tests/qemuxml2xmltest.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 0eb9e6c77a..2cbe0c82a7 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -278,7 +278,7 @@ testCompareStatusXMLToXMLFiles(const void *opaque)


 static void
-testInfoFree(struct testInfo *info)
+testInfoClear(struct testInfo *info)
 {
 VIR_FREE(info->inName);
 VIR_FREE(info->outActiveName);
@@ -345,7 +345,7 @@ testInfoSet(struct testInfo *info,
 return 0;

  error:
-testInfoFree(info);
+testInfoClear(info);
 return -1;
 }

@@ -404,7 +404,7 @@ mymain(void)
 testCompareStatusXMLToXMLFiles, ) < 0) \
 ret = -1; \
 } \
-testInfoFree(); \
+testInfoClear(); \
 } while (0)

 # define NONE QEMU_CAPS_LAST
-- 
2.16.2

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

[libvirt] [PATCH 6/7] tests: qemuxml2xml: Add modern example of status XML to the test

2018-03-02 Thread Peter Krempa

Signed-off-by: Peter Krempa 
---
 tests/qemustatusxml2xmldata/modern-in.xml  | 443 +
 tests/qemustatusxml2xmldata/modern-out.xml | 443 +
 tests/qemuxml2xmltest.c|   1 +
 3 files changed, 887 insertions(+)
 create mode 100644 tests/qemustatusxml2xmldata/modern-in.xml
 create mode 100644 tests/qemustatusxml2xmldata/modern-out.xml

diff --git a/tests/qemustatusxml2xmldata/modern-in.xml 
b/tests/qemustatusxml2xmldata/modern-in.xml
new file mode 100644
index 00..be12da7314
--- /dev/null
+++ b/tests/qemustatusxml2xmldata/modern-in.xml
@@ -0,0 +1,443 @@
+
+  
+  
+  
+
+
+  
+  
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+  
+  
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+  
+  
+  
+  
+  
+  
+  
+  
+upstream
+dcf47dbd-46d1-4d5b-b442-262a806a333a
+1024000
+1024000
+
+  
+
+8
+
+  
+
+
+  /machine
+
+
+  hvm
+  
+
+
+  
+  
+  
+
+
+  
+
+
+  
+
+
+  
+  
+  
+
+destroy
+restart
+restart
+
+  
+  
+
+
+  /usr/bin/qemu-system-x86_64
+  
+
+
+
+  
+  
+
+  base.qcow2
+
+  
+  
+
+
+
+
+  
+  
+
+
+
+
+
+
+
+
+  
+  
+
+
+  
+  
+
+
+
+  
+  
+
+
+
+  
+  
+
+
+
+  
+  
+
+  
+  
+
+
+  
+  
+
+
+  
+  
+
+
+  
+  
+
+  
+  
+
+
+
+  
+
+
+
+
+
+  
+  
+
+
+  
+
+
+  
+  
+
+
+
+  
+  
+
+
+
+
+  
+  
+
+
+
+  
+  
+
+
+  
+  
+
+  
+  
+
+  
+  
+
+
+  
+  
+
+
+  
+  
+
+
+
+  
+  
+
+
+  
+  
+
+
+  
+  
+
+
+  
+  
+/dev/random
+
+
+  
+
+
+  +0:+0
+  +0:+0
+
+  
+
diff --git a/tests/qemustatusxml2xmldata/modern-out.xml 
b/tests/qemustatusxml2xmldata/modern-out.xml
new file mode 100644
index 00..be12da7314
--- /dev/null
+++ b/tests/qemustatusxml2xmldata/modern-out.xml
@@ -0,0 +1,443 @@
+
+  
+  
+  
+
+
+  
+  
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+

[libvirt] [PATCH 3/7] tests: qemuxml2xml: Add proper domain status XML testing

2018-03-02 Thread Peter Krempa

Add new approach to properly test status XML files by supplying a full
XML file rather than generating synthetic test cases by prepending the
status header. The two tests introduced here are copies of existing
cases using the synthetic header so that current level of testing is
kept. The files are chosen to excercise the vcpu and blockjob quirks
present in the current testing.

Signed-off-by: Peter Krempa 
---
 tests/Makefile.am  |   1 +
 tests/qemustatusxml2xmldata/blockjob-mirror-in.xml |  96 ++
 .../qemustatusxml2xmldata/blockjob-mirror-out.xml  |  96 ++
 tests/qemustatusxml2xmldata/vcpus-multi-in.xml | 343 +
 tests/qemustatusxml2xmldata/vcpus-multi-out.xml| 343 +
 tests/qemuxml2xmltest.c| 109 ++-
 6 files changed, 981 insertions(+), 7 deletions(-)
 create mode 100644 tests/qemustatusxml2xmldata/blockjob-mirror-in.xml
 create mode 100644 tests/qemustatusxml2xmldata/blockjob-mirror-out.xml
 create mode 100644 tests/qemustatusxml2xmldata/vcpus-multi-in.xml
 create mode 100644 tests/qemustatusxml2xmldata/vcpus-multi-out.xml

diff --git a/tests/Makefile.am b/tests/Makefile.am
index d794df3e5c..1f60ee0393 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -128,6 +128,7 @@ EXTRA_DIST = \
qemumonitorjsondata \
qemuxml2argvdata \
qemuxml2xmloutdata \
+   qemustatusxml2xmloutdata \
qemumemlockdata \
secretxml2xmlin \
securityselinuxhelperdata \
diff --git a/tests/qemustatusxml2xmldata/blockjob-mirror-in.xml 
b/tests/qemustatusxml2xmldata/blockjob-mirror-in.xml
new file mode 100644
index 00..a22d2173e7
--- /dev/null
+++ b/tests/qemustatusxml2xmldata/blockjob-mirror-in.xml
@@ -0,0 +1,96 @@
+
+  
+  
+  
+
+  
+  
+
+
+
+
+  
+  
+
+
+
+
+
+  
+  
+  
+  
+  
+  
+  
+QEMUGuest1
+c7a5fdbd-edaf-9455-926a-d65c16db1809
+219136
+219136
+1
+
+  hvm
+  
+
+
+destroy
+restart
+destroy
+
+  /usr/bin/qemu-system-i686
+  
+
+
+
+
+  
+  
+
+
+
+  
+  
+
+
+
+
+
+
+  
+  
+
+
+
+
+  
+  
+
+
+
+  
+  
+
+
+
+
+  
+  
+
+
+
+  
+  
+
+  
+  
+
+  
+  
+  
+  
+  
+
+  
+
+  
+
diff --git a/tests/qemustatusxml2xmldata/blockjob-mirror-out.xml 
b/tests/qemustatusxml2xmldata/blockjob-mirror-out.xml
new file mode 100644
index 00..a22d2173e7
--- /dev/null
+++ b/tests/qemustatusxml2xmldata/blockjob-mirror-out.xml
@@ -0,0 +1,96 @@
+
+  
+  
+  
+
+  
+  
+
+
+
+
+  
+  
+
+
+
+
+
+  
+  
+  
+  
+  
+  
+  
+QEMUGuest1
+c7a5fdbd-edaf-9455-926a-d65c16db1809
+219136
+219136
+1
+
+  hvm
+  
+
+
+destroy
+restart
+destroy
+
+  /usr/bin/qemu-system-i686
+  
+
+
+
+
+  
+  
+
+
+
+  
+  
+
+
+
+
+
+
+  
+  
+
+
+
+
+  
+  
+
+
+
+  
+  
+
+
+
+
+  
+  
+
+
+
+  
+  
+
+  
+  
+
+  
+  
+  
+  
+  
+
+  
+
+  
+
diff --git a/tests/qemustatusxml2xmldata/vcpus-multi-in.xml 
b/tests/qemustatusxml2xmldata/vcpus-multi-in.xml
new file mode 100644
index 00..c99046ce8d
--- /dev/null
+++ b/tests/qemustatusxml2xmldata/vcpus-multi-in.xml
@@ -0,0 +1,343 @@
+
+  
+  
+  
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+

[libvirt] [PATCH 2/7] tests: qemuxml2xml: Remove testing with allowed format detection

2018-03-02 Thread Peter Krempa

Nobody should use format detection due to security implications.

Signed-off-by: Peter Krempa 
---
 tests/qemuxml2argvdata/disk-drive-detect-zeroes.xml| 2 +-
 tests/qemuxml2argvdata/hugepages-memaccess.xml | 1 +
 tests/qemuxml2argvdata/hugepages-memaccess2.xml| 1 +
 tests/qemuxml2argvdata/hugepages-pages4.xml| 1 +
 tests/qemuxml2argvdata/hugepages-pages5.xml| 1 +
 tests/qemuxml2argvdata/hugepages-pages6.xml| 1 +
 tests/qemuxml2argvdata/memory-hotplug-nvdimm-access.xml| 1 +
 tests/qemuxml2argvdata/memory-hotplug-nvdimm-label.xml | 1 +
 tests/qemuxml2argvdata/memory-hotplug-nvdimm.xml   | 1 +
 tests/qemuxml2argvdata/serial-tcp-tlsx509-chardev-notls.xml| 1 +
 tests/qemuxml2xmloutdata/aarch64-aavmf-virtio-mmio.xml | 1 +
 tests/qemuxml2xmloutdata/aarch64-virtio-pci-default.xml| 1 +
 tests/qemuxml2xmloutdata/aarch64-virtio-pci-manual-addresses.xml   | 1 +
 tests/qemuxml2xmloutdata/autoindex.xml | 1 +
 tests/qemuxml2xmloutdata/balloon-device-auto.xml   | 1 +
 tests/qemuxml2xmloutdata/balloon-device-period.xml | 1 +
 tests/qemuxml2xmloutdata/bios-nvram-os-interleave.xml  | 1 +
 tests/qemuxml2xmloutdata/bios-nvram.xml| 1 +
 tests/qemuxml2xmloutdata/blkiotune-device.xml  | 1 +
 tests/qemuxml2xmloutdata/blkiotune.xml | 1 +
 tests/qemuxml2xmloutdata/boot-menu-disable-with-timeout.xml| 1 +
 tests/qemuxml2xmloutdata/boot-menu-disable.xml | 1 +
 tests/qemuxml2xmloutdata/boot-menu-enable-with-timeout.xml | 1 +
 tests/qemuxml2xmloutdata/boot-multi.xml| 1 +
 tests/qemuxml2xmloutdata/boot-order.xml| 2 ++
 tests/qemuxml2xmloutdata/channel-guestfwd.xml  | 1 +
 tests/qemuxml2xmloutdata/channel-virtio-auto.xml   | 1 +
 tests/qemuxml2xmloutdata/channel-virtio-state-active.xml   | 1 +
 tests/qemuxml2xmloutdata/channel-virtio-state-inactive.xml | 1 +
 tests/qemuxml2xmloutdata/channel-virtio.xml| 1 +
 tests/qemuxml2xmloutdata/clock-catchup.xml | 1 +
 tests/qemuxml2xmloutdata/console-compat-auto.xml   | 1 +
 tests/qemuxml2xmloutdata/console-virtio-many.xml   | 1 +
 tests/qemuxml2xmloutdata/console-virtio.xml| 1 +
 tests/qemuxml2xmloutdata/cpu-host-passthrough-features.xml | 1 +
 tests/qemuxml2xmloutdata/cputune-iothreads.xml | 1 +
 tests/qemuxml2xmloutdata/cputune-iothreadsched-zeropriority.xml| 1 +
 tests/qemuxml2xmloutdata/cputune-iothreadsched.xml | 1 +
 tests/qemuxml2xmloutdata/cputune-zero-shares.xml   | 1 +
 tests/qemuxml2xmloutdata/cputune.xml   | 1 +
 tests/qemuxml2xmloutdata/disk-drive-copy-on-read.xml   | 2 +-
 tests/qemuxml2xmloutdata/disk-drive-discard.xml| 2 +-
 tests/qemuxml2xmloutdata/disk-mirror-active.xml| 5 +
 tests/qemuxml2xmloutdata/disk-mirror-inactive.xml  | 4 
 tests/qemuxml2xmloutdata/disk-mirror-old-inactive.xml  | 4 
 tests/qemuxml2xmloutdata/disk-mirror-old.xml   | 7 ++-
 tests/qemuxml2xmloutdata/disk-scsi-device-auto.xml | 2 ++
 tests/qemuxml2xmloutdata/disk-scsi-device.xml  | 2 ++
 tests/qemuxml2xmloutdata/disk-scsi-disk-vpd.xml| 2 ++
 tests/qemuxml2xmloutdata/disk-scsi-lun-passthrough-sgio.xml| 2 ++
 tests/qemuxml2xmloutdata/disk-scsi-megasas.xml | 2 ++
 tests/qemuxml2xmloutdata/disk-scsi-mptsas1068.xml  | 2 ++
 tests/qemuxml2xmloutdata/disk-scsi-virtio-scsi.xml | 2 ++
 tests/qemuxml2xmloutdata/disk-scsi-vscsi.xml   | 2 ++
 tests/qemuxml2xmloutdata/disk-serial.xml   | 3 +++
 tests/qemuxml2xmloutdata/disk-source-pool-mode.xml | 4 
 tests/qemuxml2xmloutdata/disk-source-pool.xml  | 2 ++
 tests/qemuxml2xmloutdata/disk-usb-device.xml   | 2 ++
 tests/qemuxml2xmloutdata/disk-virtio-scsi-cmd_per_lun.xml  | 1 +
 tests/qemuxml2xmloutdata/disk-virtio-scsi-ioeventfd.xml| 1 +
 tests/qemuxml2xmloutdata/disk-virtio-scsi-max_sectors.xml  | 1 +
 tests/qemuxml2xmloutdata/disk-virtio-scsi-num_queues.xml   | 1 +
 tests/qemuxml2xmloutdata/graphics-listen-network.xml   | 1 +
 tests/qemuxml2xmloutdata/graphics-listen-network2.xml  | 1 +
 tests/qemuxml2xmloutdata/graphics-spice-compression.xml| 1 +

[libvirt] [PATCH 0/7] tests: qemu: Do proper status XML testing

2018-03-02 Thread Peter Krempa

The 'fake' status XML testing we were doing until now was terrible and
would not catch most of the problems. Also the code was too complicated.

This adds a way simpler approach which actually works. (for the small
price of adding a lot of test data lines)

Peter Krempa (7):
  tests: qemuxml2xml: Rename testInfoFree to testInfoClear
  tests: qemuxml2xml: Remove testing with allowed format detection
  tests: qemuxml2xml: Add proper domain status XML testing
  tests: qemuxml2xml: Remove fake status XML testing
  tests: util: Remove callback from testCompareDomXML2XMLFiles
  tests: qemuxml2xml: Add modern example of status XML to the test
  tests: qemuxml2xml: Add status XML with outgoing migration with NBD

 tests/Makefile.am  |   1 +
 tests/genericxml2xmltest.c |   2 +-
 tests/lxcxml2xmltest.c |   2 +-
 tests/qemustatusxml2xmldata/blockjob-mirror-in.xml |  96 +
 .../qemustatusxml2xmldata/blockjob-mirror-out.xml  |  96 +
 .../qemustatusxml2xmldata/migration-out-nbd-in.xml | 449 +
 .../migration-out-nbd-out.xml  | 449 +
 tests/qemustatusxml2xmldata/modern-in.xml  | 443 
 tests/qemustatusxml2xmldata/modern-out.xml | 443 
 tests/qemustatusxml2xmldata/vcpus-multi-in.xml | 343 
 tests/qemustatusxml2xmldata/vcpus-multi-out.xml| 343 
 .../qemuxml2argvdata/disk-drive-detect-zeroes.xml  |   2 +-
 tests/qemuxml2argvdata/hugepages-memaccess.xml |   1 +
 tests/qemuxml2argvdata/hugepages-memaccess2.xml|   1 +
 tests/qemuxml2argvdata/hugepages-pages4.xml|   1 +
 tests/qemuxml2argvdata/hugepages-pages5.xml|   1 +
 tests/qemuxml2argvdata/hugepages-pages6.xml|   1 +
 .../memory-hotplug-nvdimm-access.xml   |   1 +
 .../memory-hotplug-nvdimm-label.xml|   1 +
 tests/qemuxml2argvdata/memory-hotplug-nvdimm.xml   |   1 +
 .../serial-tcp-tlsx509-chardev-notls.xml   |   1 +
 .../aarch64-aavmf-virtio-mmio.xml  |   1 +
 .../aarch64-virtio-pci-default.xml |   1 +
 .../aarch64-virtio-pci-manual-addresses.xml|   1 +
 tests/qemuxml2xmloutdata/autoindex.xml |   1 +
 tests/qemuxml2xmloutdata/balloon-device-auto.xml   |   1 +
 tests/qemuxml2xmloutdata/balloon-device-period.xml |   1 +
 .../bios-nvram-os-interleave.xml   |   1 +
 tests/qemuxml2xmloutdata/bios-nvram.xml|   1 +
 tests/qemuxml2xmloutdata/blkiotune-device.xml  |   1 +
 tests/qemuxml2xmloutdata/blkiotune.xml |   1 +
 .../boot-menu-disable-with-timeout.xml |   1 +
 tests/qemuxml2xmloutdata/boot-menu-disable.xml |   1 +
 .../boot-menu-enable-with-timeout.xml  |   1 +
 tests/qemuxml2xmloutdata/boot-multi.xml|   1 +
 tests/qemuxml2xmloutdata/boot-order.xml|   2 +
 tests/qemuxml2xmloutdata/channel-guestfwd.xml  |   1 +
 tests/qemuxml2xmloutdata/channel-virtio-auto.xml   |   1 +
 .../channel-virtio-state-active.xml|   1 +
 .../channel-virtio-state-inactive.xml  |   1 +
 tests/qemuxml2xmloutdata/channel-virtio.xml|   1 +
 tests/qemuxml2xmloutdata/clock-catchup.xml |   1 +
 tests/qemuxml2xmloutdata/console-compat-auto.xml   |   1 +
 tests/qemuxml2xmloutdata/console-virtio-many.xml   |   1 +
 tests/qemuxml2xmloutdata/console-virtio.xml|   1 +
 .../cpu-host-passthrough-features.xml  |   1 +
 tests/qemuxml2xmloutdata/cputune-iothreads.xml |   1 +
 .../cputune-iothreadsched-zeropriority.xml |   1 +
 tests/qemuxml2xmloutdata/cputune-iothreadsched.xml |   1 +
 tests/qemuxml2xmloutdata/cputune-zero-shares.xml   |   1 +
 tests/qemuxml2xmloutdata/cputune.xml   |   1 +
 .../qemuxml2xmloutdata/disk-drive-copy-on-read.xml |   2 +-
 tests/qemuxml2xmloutdata/disk-drive-discard.xml|   2 +-
 tests/qemuxml2xmloutdata/disk-mirror-active.xml|   5 +
 tests/qemuxml2xmloutdata/disk-mirror-inactive.xml  |   4 +
 .../disk-mirror-old-inactive.xml   |   4 +
 tests/qemuxml2xmloutdata/disk-mirror-old.xml   |   7 +-
 tests/qemuxml2xmloutdata/disk-scsi-device-auto.xml |   2 +
 tests/qemuxml2xmloutdata/disk-scsi-device.xml  |   2 +
 tests/qemuxml2xmloutdata/disk-scsi-disk-vpd.xml|   2 +
 .../disk-scsi-lun-passthrough-sgio.xml |   2 +
 tests/qemuxml2xmloutdata/disk-scsi-megasas.xml |   2 +
 tests/qemuxml2xmloutdata/disk-scsi-mptsas1068.xml  |   2 +
 tests/qemuxml2xmloutdata/disk-scsi-virtio-scsi.xml |   2 +
 tests/qemuxml2xmloutdata/disk-scsi-vscsi.xml   |   2 +
 tests/qemuxml2xmloutdata/disk-serial.xml   |   3 +
 tests/qemuxml2xmloutdata/disk-source-pool-mode.xml |   4 +
 tests/qemuxml2xmloutdata/disk-source-pool.xml  |   2 +
 tests/qemuxml2xmloutdata/disk-usb-device.xml   |   2 +

[libvirt] [PATCH 5/7] tests: util: Remove callback from testCompareDomXML2XMLFiles

2018-03-02 Thread Peter Krempa

The testCompareDomXML2XMLPreFormatCallback is no longer used and thus
can be removed.

Signed-off-by: Peter Krempa 
---
 tests/genericxml2xmltest.c | 2 +-
 tests/lxcxml2xmltest.c | 2 +-
 tests/qemuxml2xmltest.c| 5 ++---
 tests/testutils.c  | 8 +---
 tests/testutils.h  | 5 -
 5 files changed, 5 insertions(+), 17 deletions(-)

diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c
index c33fce1922..d8270a6cae 100644
--- a/tests/genericxml2xmltest.c
+++ b/tests/genericxml2xmltest.c
@@ -40,7 +40,7 @@ testCompareXMLToXMLHelper(const void *data)

 ret = testCompareDomXML2XMLFiles(caps, xmlopt, xml_in,
  info->different ? xml_out : xml_in,
- !info->inactive_only, NULL, NULL, 0,
+ !info->inactive_only, 0,
  info->expectResult);
  cleanup:
 VIR_FREE(xml_in);
diff --git a/tests/lxcxml2xmltest.c b/tests/lxcxml2xmltest.c
index 57751a5773..3b96862c62 100644
--- a/tests/lxcxml2xmltest.c
+++ b/tests/lxcxml2xmltest.c
@@ -46,7 +46,7 @@ testCompareXMLToXMLHelper(const void *data)
 ret = testCompareDomXML2XMLFiles(caps, xmlopt, xml_in,
  info->different ? xml_out : xml_in,
  !info->inactive_only,
- NULL, NULL, info->parse_flags,
+ info->parse_flags,
  TEST_COMPARE_DOM_XML2XML_RESULT_SUCCESS);
  cleanup:
 VIR_FREE(xml_in);
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 980d7fb0b3..ce4dae49c3 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -47,8 +47,7 @@ testXML2XMLActive(const void *opaque)

 return testCompareDomXML2XMLFiles(driver.caps, driver.xmlopt,
   info->inName, info->outActiveName, true,
-  NULL,
-  opaque, 0,
+  0,
   TEST_COMPARE_DOM_XML2XML_RESULT_SUCCESS);
 }

@@ -60,7 +59,7 @@ testXML2XMLInactive(const void *opaque)

 return testCompareDomXML2XMLFiles(driver.caps, driver.xmlopt, info->inName,
   info->outInactiveName, false,
-  NULL, opaque, 0,
+  0,
   TEST_COMPARE_DOM_XML2XML_RESULT_SUCCESS);
 }

diff --git a/tests/testutils.c b/tests/testutils.c
index 17959aaf4f..040ef1d2f7 100644
--- a/tests/testutils.c
+++ b/tests/testutils.c
@@ -1241,8 +1241,7 @@ virDomainXMLOptionPtr 
virTestGenericDomainXMLConfInit(void)
 int
 testCompareDomXML2XMLFiles(virCapsPtr caps, virDomainXMLOptionPtr xmlopt,
const char *infile, const char *outfile, bool live,
-   testCompareDomXML2XMLPreFormatCallback cb,
-   const void *opaque, unsigned int parseFlags,
+   unsigned int parseFlags,
testCompareDomXML2XMLResult expectResult)
 {
 char *actual = NULL;
@@ -1273,11 +1272,6 @@ testCompareDomXML2XMLFiles(virCapsPtr caps, 
virDomainXMLOptionPtr xmlopt,
 goto out;
 }

-if (cb && cb(def, opaque) < 0) {
-result = TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_CB;
-goto out;
-}
-
 if (!(actual = virDomainDefFormat(def, caps, format_flags))) {
 result = TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_FORMAT;
 goto out;
diff --git a/tests/testutils.h b/tests/testutils.h
index 668a79d95c..d840875bc1 100644
--- a/tests/testutils.h
+++ b/tests/testutils.h
@@ -153,20 +153,15 @@ typedef enum {
 TEST_COMPARE_DOM_XML2XML_RESULT_SUCCESS,
 TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE,
 TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_STABILITY,
-TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_CB,
 TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_FORMAT,
 TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_COMPARE,
 } testCompareDomXML2XMLResult;

-typedef int (*testCompareDomXML2XMLPreFormatCallback)(virDomainDefPtr def,
-  const void *opaque);
 int testCompareDomXML2XMLFiles(virCapsPtr caps,
virDomainXMLOptionPtr xmlopt,
const char *inxml,
const char *outfile,
bool live,
-   testCompareDomXML2XMLPreFormatCallback cb,
-   const void *opaque,
unsigned int parseFlags,
testCompareDomXML2XMLResult expectResult);

-- 
2.16.2

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

Re: [libvirt] [PATCH 5/6] qemu: domain: Use virXMLFormatElement in qemuDomainObjPrivateXMLFormatJob

2018-03-02 Thread Peter Krempa

On Thu, Mar 01, 2018 at 18:59:46 +0100, Peter Krempa wrote:
> Modernize the code by using the clever formatter rather than checking
> manually when to format the end of the element.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_domain.c | 34 +-
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index b7fb9f264d..e4088665ee 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2035,11 +2035,13 @@ qemuDomainObjPrivateXMLFormatAllowReboot(virBufferPtr 
> buf,
>  }
> 
> 
> -static void
> +static int
>  qemuDomainObjPrivateXMLFormatJob(virBufferPtr buf,
>   virDomainObjPtr vm,
>   qemuDomainObjPrivatePtr priv)
>  {
> +virBuffer attrBuf = VIR_BUFFER_INITIALIZER;
> +virBuffer childBuf = VIR_BUFFER_INITIALIZER;
>  qemuDomainJob job = priv->job.active;
> 
>  if (!qemuDomainTrackJob(job))
> @@ -2047,37 +2049,34 @@ qemuDomainObjPrivateXMLFormatJob(virBufferPtr buf,
> 
>  if (job == QEMU_JOB_NONE &&
>  priv->job.asyncJob == QEMU_ASYNC_JOB_NONE)
> -return;
> +return 0;
> +
> +virBufferSetChildIndent(, buf);
> 
> -virBufferAsprintf(buf, " +virBufferAsprintf(, "type='%s' async='%s'",

s/"type/" type/ here otherwise they'd be squashed together. It was found
by a test-suite improvement that I'll post shortly.


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

Re: [libvirt] [PATCH] virtlockd: acquire locks on re-exec

2018-03-02 Thread Daniel P . Berrangé

On Thu, Mar 01, 2018 at 04:42:36PM -0700, Jim Fehlig wrote:
> Locks held by virtlockd are dropped on re-exec.
> 
> virtlockd   94306 POSIX 5.4G WRITE 0 0   0 /tmp/test.qcow2
> virtlockd   94306 POSIX   5B WRITE 0 0   0 /run/virtlockd.pid
> virtlockd   94306 POSIX   5B WRITE 0 0   0 /run/virtlockd.pid
> 
> Acquire locks in PostExecRestart code path.

This is really strange and should *not* be happening.  POSIX locks
are supposed to be preserved across execve() if the FD has CLOEXEC
unset, and you don't fork() before the exec.

eg see this demo program:

#include 
#include 
#include 
#include 
#include 


int main(int argc, char **argv) {

  if (argc == 2) {
int fd = atoi(argv[1]);

struct flock fl = {
  .l_type = F_WRLCK,
  .l_whence = SEEK_SET,
  .l_start = 0,
  .l_len = 42,
};

if (fcntl(fd, F_GETLK, ) < 0)
  abort();

int flags;
if ((flags = fcntl(fd, F_GETFD)) < 0)
  abort();
flags |= FD_CLOEXEC;

if (fcntl(fd, F_SETFD, flags) < 0)
  abort();

fprintf(stderr, "Owned %d\n", fl.l_pid);
fprintf(stderr, "Execd\n");
sleep(50);

  } else {
int fd = open("lock.txt", O_WRONLY|O_CREAT|O_TRUNC, 0755);
if (fd < 0)
  abort();

struct flock fl = {
  .l_type = F_WRLCK,
  .l_whence = SEEK_SET,
  .l_start = 0,
  .l_len = 42,
};

if (fcntl(fd, F_SETLK, ) < 0)
  abort();

int flags;

if ((flags = fcntl(fd, F_GETFD)) < 0)
  abort();
flags &= ~FD_CLOEXEC;

if (fcntl(fd, F_SETFD, flags) < 0)
  abort();

char fdstr[100];
snprintf(fdstr, sizeof(fdstr), "%d", fd);

char *newargv[] = { argv[0], fdstr, NULL };

fprintf(stderr, "Waiting\n");
sleep(10);
execve(argv[0], newargv, NULL);
  }
}



If you run this, you'll see the lock still exists after execveI().

So I wonder what we've screwed up to cause the locks to get
released - reaquiring them definitely isn't desirable as we
should not loose them in the first place !

> 
> Signed-off-by: Jim Fehlig 
> ---
> 
> The CLOEXEC flag is set in virLockSpaceNewPostExecRestart(), so I assume
> it is fine to call virFileLock() here as well.
> 
>  src/util/virlockspace.c | 16 
>  1 file changed, 16 insertions(+)
> 
> diff --git a/src/util/virlockspace.c b/src/util/virlockspace.c
> index 41af0cdb6..420878b0a 100644
> --- a/src/util/virlockspace.c
> +++ b/src/util/virlockspace.c
> @@ -337,6 +337,7 @@ virLockSpacePtr 
> virLockSpaceNewPostExecRestart(virJSONValuePtr object)
>  virJSONValuePtr owners;
>  size_t j;
>  ssize_t m;
> +bool shared = false;
>  
>  if (VIR_ALLOC(res) < 0)
>  goto error;
> @@ -389,6 +390,21 @@ virLockSpacePtr 
> virLockSpaceNewPostExecRestart(virJSONValuePtr object)
>  goto error;
>  }
>  
> +shared = !!(res->flags & VIR_LOCK_SPACE_ACQUIRE_SHARED);
> +if (virFileLock(res->fd, shared, 0, 1, false) < 0) {
> +if (errno == EACCES || errno == EAGAIN) {
> +virReportError(VIR_ERR_RESOURCE_BUSY,
> +   _("Lockspace resource '%s' is locked"),
> +   res->name);
> +} else {
> +virReportSystemError(errno,
> + _("Unable to acquire lock on '%s'"),
> + res->path);
> +}
> +virLockSpaceResourceFree(res);
> +goto error;
> +}
> +
>  if (!(owners = virJSONValueObjectGet(child, "owners"))) {
>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("Missing resource owners in JSON document"));
> -- 
> 2.16.2
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

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

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

Re: [libvirt] [PATCH v2 09/12] qemu: Start PR daemons on domain startup

2018-03-02 Thread John Ferlan

On 02/21/2018 01:11 PM, Michal Privoznik wrote:
> Before we exec() qemu we have to spawn pr-helper processes for
> all managed reservations (well, technically there can only one).

"can be only one"

Since there can only be one why do we bother w/ plurality?

> The only caveat there is that we should place the process into
> the same namespace and cgroup as qemu (so that it shares the same
> view of the system). But we can do that only after we've forked.
> That means calling the setup function between fork() and exec().

Seems like a good note/comment in the code where
qemuProcessSetupPRDaemons is called so that someone doesn't have to go
back to the commit message in order to find out why the call was placed
where it was.

> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_process.c | 164 
> 
>  1 file changed, 164 insertions(+)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index dc33eb7bc..33e0ad30c 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -2551,6 +2551,164 @@ qemuProcessResctrlCreate(virQEMUDriverPtr driver,
>  ret = 0;
>   cleanup:
>  virObjectUnref(caps);
> +
> +return ret;
> +}
> +
> +
> +static void
> +qemuProcessKillPRDaemons(virDomainObjPtr vm)

Daemon*s* is is a misnomer

> +{
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +
> +if (priv->prPid == (pid_t) -1)
> +return;
> +
> +virProcessKillPainfully(priv->prPid, true);
> +priv->prPid = (pid_t) -1;
> +}
> +
> +
> +static int
> +qemuProcessSetupOnePRDaemonHook(void *opaque)
> +{
> +virDomainObjPtr vm = opaque;
> +size_t i, nfds = 0;
> +int *fds = NULL;
> +int ret = -1;
> +
> +if (virProcessGetNamespaces(vm->pid, , ) < 0)
> +return ret;

 another false positive for Coverity since it for some reason
believes "virProcessGetNamespaces" allocates memory that is stored into
"fds" when there's an error returned.

Strangely, setting *nfdlist = 0 in the (ret < 0) part of cleanup: in
virProcessGetNamespaces and going to cleanup here magically clears the
error...

> +
> +if (nfds > 0 &&
> +virProcessSetNamespaces(nfds, fds) < 0)
> +goto cleanup;
> +
> +ret = 0;
> + cleanup:
> +for (i = 0; i < nfds; i++)
> +VIR_FORCE_CLOSE(fds[i]);
> +VIR_FREE(fds);
> +return ret;
> +}
> +
> +

If we returned:

 -1 error, 0 started, and 1 unnecessary, then our caller could be a bit
smarter about needing to run through the whole ndisks list.

> +static int
> +qemuProcessSetupOnePRDaemon(virDomainObjPtr vm,
> +virDomainDiskDefPtr disk)
> +{
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +virQEMUDriverPtr driver = priv->driver;
> +virQEMUDriverConfigPtr cfg;
> +qemuDomainStorageSourcePrivatePtr srcPriv;
> +qemuDomainDiskPRDPtr prd;
> +char *pidfile = NULL;
> +pid_t cpid = -1;
> +virCommandPtr cmd = NULL;
> +virTimeBackOffVar timebackoff;
> +const unsigned long long timeout = 50; /* ms */
> +int ret = -1;
> +
> +if (priv->prPid != (pid_t) -1 ||
> +!virStoragePRDefIsManaged(disk->src->pr))
> +return 0;

Ah... so this is where we ensure there is only ever one pr-helper... and
this !managed usage still doesn't help my earlier confusion.

In one mode we have:

  -object pr-manager-helper,id=pr-helper-sdb,\
  path=/path/to/qemu-pr-helper.sock

and the other we have:

  -object pr-manager-helper,id=pr-helper0,\
  path=/tmp/lib/domain--1-QEMUGuest1/pr-helper0.sock

But for "everything" (or both) we still have the qemu-pr-helper process.
For both the alias/id is added to the -drive command line using
"file.pr-manager=$alias".

In one mode we would start the qemu-pr-helper, but in the other we're
not. So, if none of the lun's used the second mode, then what do they
connect to?

Furthermore, the managed mode helps decide which alias to use and of
course which socket will be used.  With the alias, I had the earlier
question/confusion over how many objects would be created using the
"other" mode in the above examples since "theoretically speaking" of
course the id= should be unique.  Then the path is just a client path to
the socket which in the former mode is user defined and the latter mode
is static or the same for every lun using that mode. So can we have more
than one lun using the same client socket path?

I don't know if the above helps explain my confusion or makes it even
harder to understand. I hope it helps.

> +
> +cfg = virQEMUDriverGetConfig(driver);
> +srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
> +prd = srcPriv->prd;
> +
> +if (!virFileIsExecutable(cfg->prHelperName)) {
> +virReportSystemError(errno, _("'%s' is not a suitable pr helper"),
> + cfg->prHelperName);
> +goto cleanup;
> +}
> +
> +if (!(pidfile =

Re: [libvirt] [PATCH v2 08/12] qemu_domain: Track pr-helper PID in status XML

2018-03-02 Thread John Ferlan



On 02/21/2018 01:11 PM, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 16 
>  src/qemu/qemu_domain.h |  2 ++
>  2 files changed, 18 insertions(+)
> 

This seems reasonable...  Although given the next patch and usage of
"daemon*s*" I'd almost expect this to be an array of prPid; otherwise,
plurality in the subsequent patch really don't make sense.

John

> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 45205fd03..f7da62dba 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1864,6 +1864,7 @@ qemuDomainObjPrivateAlloc(void *opaque)
>  
>  priv->migMaxBandwidth = QEMU_DOMAIN_MIG_BANDWIDTH_MAX;
>  priv->driver = opaque;
> +priv->prPid = (pid_t) -1;
>  
>  return priv;
>  
> @@ -1926,6 +1927,8 @@ qemuDomainObjPrivateDataClear(qemuDomainObjPrivatePtr 
> priv)
>  
>  virBitmapFree(priv->migrationCaps);
>  priv->migrationCaps = NULL;
> +
> +priv->prPid = (pid_t) -1;
>  }
>  
>  
> @@ -2172,6 +2175,11 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf,
>  if (qemuDomainObjPrivateXMLFormatBlockjobs(buf, vm) < 0)
>  return -1;
>  
> +if (priv->prPid != (pid_t) -1) {
> +virBufferAsprintf(buf, "%lld\n",
> +  (long long) priv->prPid);
> +}
> +
>  return 0;
>  }
>  
> @@ -2318,6 +2326,7 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
>  xmlNodePtr *nodes = NULL;
>  xmlNodePtr node = NULL;
>  virQEMUCapsPtr qemuCaps = NULL;
> +long long prPid = -1;
>  
>  if (VIR_ALLOC(priv->monConfig) < 0)
>  goto error;
> @@ -2524,6 +2533,13 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
>  if (qemuDomainObjPrivateXMLParseBlockjobs(priv, ctxt) < 0)
>  goto error;
>  
> +if (virXPathLongLong("string(./prPid)", ctxt, ) < -1) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("unable to parse "));
> +goto error;
> +}
> +priv->prPid = (pid_t) prPid;
> +
>  return 0;
>  
>   error:
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index b9258eb8e..418b47153 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -342,6 +342,8 @@ struct _qemuDomainObjPrivate {
>  /* Migration capabilities. Rechecked on reconnect, not to be saved in
>   * private XML. */
>  virBitmapPtr migrationCaps;
> +
> +pid_t prPid;
>  };
>  
>  # define QEMU_DOMAIN_PRIVATE(vm) \
> 

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

Re: [libvirt] [PATCH v3 02/12] qemu: Create new qemuDomainDeviceDefValidateControllerPCI()

2018-03-02 Thread Laine Stump

On 03/02/2018 08:05 AM, Andrea Bolognani wrote:
> On Fri, 2018-03-02 at 07:34 -0500, Laine Stump wrote:
>> On 02/21/2018 09:14 AM, Andrea Bolognani wrote:
> [...]
>>> +static int
>>> +qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef 
>>> *cont,
>>> + const virDomainDef *def,
>>> + virQEMUCapsPtr qemuCaps)
>>> +
>>> +{
>>> +const virDomainPCIControllerOpts *pciopts = >opts.pciopts;
>>> +const char *model = 
>>> virDomainControllerModelPCITypeToString(cont->model);
>>> +const char *modelName = 
>>> virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
>>> +
>>> +if (!model) {
>>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>>> +   _("Unknown virDomainControllerModelPCI value: %d"),
>>> +   cont->model);
>>> +return -1;
>>> +}
>>> +if (!modelName) {
>>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>>> +   _("Unknown virDomainControllerPCIModelName value: 
>>> %d"),
>>> +   pciopts->modelName);
>>> +return -1;
>>> +}
>> (meant to send this before, but kept forgetting...)
>>
>> 1) I thought modelName wasn't set for pci-root. Doesn't the above cause
>> a validation error in that case? (too early in the day, haven't tried it)
> The default value is _MODEL_NAME_NONE aka zero, which is still part
> of the enumeration, so virDomainControllerPCIModelNameTypeToString()
> won't return NULL and no error will be raised. For pSeries guests,
> it will be set to _MODEL_NAME_SPAPR_PCI_HOST_BRIDGE so once again no
> problem there.

Ah, right. So the name is "none", but when we're formatting for dumpxml
we skip it if the value is 0, so that never shows up. "Nevermind"


>
>> 2) danpb made a nice new function to standardize/simplify errors of the
>> above type: virReportEnumRangeError().
> His efforts on switch normalization and me rebasing this series
> happened pretty much at the same time; more specifically, the
> function you're talking about was introduced in 3b1020ac805e, while
> my series is based on the earlier f565321b26df.

Yep, I saw his patches at about the same time as yours, and since you're
respinning I thought I'd point it out.

> I guess this means another rebase! Yay! \o/

Someday you'll learn to do what I'm doing right now - I want to add some
extra validation of pci controller options, but I'm waiting to write any
code until you've pushed your patches. :-)

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

[libvirt] [PATCH v5 06/12] qemu: Validate PCI controller options (pcihole64)

2018-03-02 Thread Andrea Bolognani

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

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

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 54e47acd99..9b8d2e864d 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4685,6 +4685,46 @@ qemuDomainDeviceDefValidateControllerPCI(const 
virDomainControllerDef *cont,
 return -1;
 }
 
+/* pcihole64 */
+switch ((virDomainControllerModelPCI) cont->model) {
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
+/* The pcihole64 option only applies to x86 guests */
+if ((pciopts->pcihole64 ||
+ pciopts->pcihole64size != 0) &&
+!ARCH_IS_X86(def->os.arch)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Option '%s' is not valid for '%s' controller "
+ "on '%s' architecture or '%s' machine type"),
+   "pcihole64", model,
+   virArchToString(def->os.arch), def->os.machine);
+return -1;
+}
+break;
+
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
+case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
+if (pciopts->pcihole64 ||
+pciopts->pcihole64size != 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Option '%s' is not valid for '%s' controller"),
+   "pcihole64", model);
+return -1;
+}
+break;
+
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
+default:
+virReportEnumRangeError(virDomainControllerModelPCI, cont->model);
+return -1;
+}
+
 return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps);
 }
 
-- 
2.14.3

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

[libvirt] [PATCH v5 12/12] qemu: Remove old qemuDomainDeviceDefValidateControllerPCI()

2018-03-02 Thread Andrea Bolognani

We've implemented all existing checks, and more, in the new
function, so we can finally drop the old one.

Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_domain.c | 21 +
 1 file changed, 1 insertion(+), 20 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index e0ab43e139..735341ae16 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4267,25 +4267,6 @@ qemuDomainDeviceDefValidateControllerSCSI(const 
virDomainControllerDef *controll
 }
 
 
-static int
-qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef 
*controller,
-const virDomainDef *def,
-virQEMUCapsPtr qemuCaps 
ATTRIBUTE_UNUSED)
-{
-/* skip pcie-root */
-if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)
-return 0;
-
-/* Skip pci-root, except for pSeries guests (which actually
- * support more than one PCI Host Bridge per guest) */
-if (!qemuDomainIsPSeries(def) &&
-controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT)
-return 0;
-
-return 0;
-}
-
-
 /**
  * virDomainControllerPCIModelNameToQEMUCaps:
  * @modelName: model name
@@ -4823,7 +4804,7 @@ qemuDomainDeviceDefValidateControllerPCI(const 
virDomainControllerDef *cont,
 }
 
  done:
-return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps);
+return 0;
 }
 
 
-- 
2.14.3

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

[libvirt] [PATCH v5 03/12] qemu: Validate PCI controller options (modelName)

2018-03-02 Thread Andrea Bolognani

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

Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_domain.c | 229 +++--
 1 file changed, 145 insertions(+), 84 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index a45a676520..13c2b557fb 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4274,7 +4274,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const 
virDomainControllerDef *contro
 {
 virDomainControllerModelPCI model = controller->model;
 const virDomainPCIControllerOpts *pciopts;
-const char *modelName = NULL;
 
 /* skip pcie-root */
 if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)
@@ -4312,24 +4311,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const 
virDomainControllerDef *contro
 }
 
 pciopts = >opts.pciopts;
-if (controller->model != VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT &&
-controller->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST) {
-if (pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("autogenerated %s options not set"),
-   
virDomainControllerModelPCITypeToString(controller->model));
-return -1;
-}
-
-modelName = 
virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
-if (!modelName) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("unknown %s modelName value %d"),
-   
virDomainControllerModelPCITypeToString(controller->model),
-   pciopts->modelName);
-return -1;
-}
-}
 
 /* Second pass - now the model specific checks */
 switch (model) {
@@ -4340,14 +4321,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const 
virDomainControllerDef *contro
 return -1;
 }
 
-if (pciopts->modelName != 
VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("PCI controller model name '%s' is not valid "
- "for a pci-bridge"),
-   modelName);
-return -1;
-}
-
 if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("the pci-bridge controller is not supported "
@@ -4364,14 +4337,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const 
virDomainControllerDef *contro
 return -1;
 }
 
-if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("PCI controller model name '%s' is not valid "
- "for a pci-expander-bus"),
-   modelName);
-return -1;
-}
-
 if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("the pxb controller is not supported in this "
@@ -4382,14 +4347,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const 
virDomainControllerDef *contro
 break;
 
 case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
-if (pciopts->modelName != 
VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("PCI controller model name '%s' is not valid "
- "for a dmi-to-pci-bridge"),
-   modelName);
-return -1;
-}
-
 if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("the dmi-to-pci-bridge (i82801b11-bridge) "
@@ -4406,15 +4363,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const 
virDomainControllerDef *contro
 return -1;
 }
 
-if ((pciopts->modelName != 
VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) &&
-(pciopts->modelName != 
VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("PCI controller model name '%s' is not valid "
- "for a pcie-root-port"),
-   modelName);
-return -1;
-}
-
 if ((pciopts->modelName == 
VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) &&
 !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IOH3420)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -4434,14 +4382,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const 
virDomainControllerDef *contro
 break;
 
 case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
-if

[libvirt] [PATCH v5 07/12] qemu: Validate PCI controller options (busNr)

2018-03-02 Thread Andrea Bolognani

This change catches an invalid use of the option in our
test suite.

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

Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_domain.c | 46 +++---
 tests/qemuxml2argvdata/pcie-expander-bus.xml   |  2 +-
 tests/qemuxml2xmloutdata/pcie-expander-bus.xml |  2 +-
 3 files changed, 36 insertions(+), 14 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 9b8d2e864d..1354d9850a 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4306,12 +4306,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const 
virDomainControllerDef *contro
 break;
 
 case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
-if (pciopts->busNr == -1) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("autogenerated pci-expander-bus options not 
set"));
-return -1;
-}
-
 if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("the pxb controller is not supported in this "
@@ -4385,12 +4379,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const 
virDomainControllerDef *contro
 break;
 
 case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
-if (pciopts->busNr == -1) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("autogenerated pcie-expander-bus options not 
set"));
-return -1;
-}
-
 if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB_PCIE)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("the pxb-pcie controller is not supported "
@@ -4725,6 +4713,40 @@ qemuDomainDeviceDefValidateControllerPCI(const 
virDomainControllerDef *cont,
 return -1;
 }
 
+/* busNr */
+switch ((virDomainControllerModelPCI) cont->model) {
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
+if (pciopts->busNr == -1) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Option '%s' not set for '%s' controller"),
+   "busNr", model);
+return -1;
+}
+break;
+
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
+case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
+if (pciopts->busNr != -1) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Option '%s' is not valid for '%s' controller"),
+   "busNr", model);
+return -1;
+}
+break;
+
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
+default:
+virReportEnumRangeError(virDomainControllerModelPCI, cont->model);
+return -1;
+}
+
 return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps);
 }
 
diff --git a/tests/qemuxml2argvdata/pcie-expander-bus.xml 
b/tests/qemuxml2argvdata/pcie-expander-bus.xml
index ac01c26ccf..237430a1e5 100644
--- a/tests/qemuxml2argvdata/pcie-expander-bus.xml
+++ b/tests/qemuxml2argvdata/pcie-expander-bus.xml
@@ -35,7 +35,7 @@
   
 
 
-  
+  
 1
   
   
diff --git a/tests/qemuxml2xmloutdata/pcie-expander-bus.xml 
b/tests/qemuxml2xmloutdata/pcie-expander-bus.xml
index aaac423cac..d5e741b80d 100644
--- a/tests/qemuxml2xmloutdata/pcie-expander-bus.xml
+++ b/tests/qemuxml2xmloutdata/pcie-expander-bus.xml
@@ -36,7 +36,7 @@
 
 
   
-  
+  
 1
   
   
-- 
2.14.3

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

Re: [libvirt] [PATCH v2 10/12] qemu_hotplug: Hotplug of reservations

2018-03-02 Thread Ján Tomko


On Wed, Feb 21, 2018 at 07:11:35PM +0100, Michal Privoznik wrote:

Surprisingly, nothing special is happening here. If we are the
first to use the managed helper then spawn it. If not, we're
almost done.

Signed-off-by: Michal Privoznik 
---
src/qemu/qemu_hotplug.c | 72 +
src/qemu/qemu_process.c | 38 +-
src/qemu/qemu_process.h |  7 +
3 files changed, 110 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index f28006e3c..2ebb68fbc 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -348,6 +348,58 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
}


+static int
+qemuBuildPRDefInfoProps(virDomainObjPtr vm,
+virDomainDiskDefPtr disk,
+virJSONValuePtr *prmgrProps,
+const char **prAlias,
+const char **prPath)
+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+qemuDomainStorageSourcePrivatePtr srcPriv;
+virJSONValuePtr props = NULL;
+int ret = -1;
+
+srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
+
+*prmgrProps = NULL;
+
+if (priv->prPid != (pid_t) -1 ||
+!srcPriv->prd ||
+!srcPriv->prd->alias)
+return 0;
+


If !srcPriv->prd is NULL, you should not dereference it.

Jan


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

[libvirt] [PATCH v5 02/12] qemu: Create new qemuDomainDeviceDefValidateControllerPCI()

2018-03-02 Thread Andrea Bolognani

The esisting function is renamed and called from the new one, so
that even while we're in the process of implementing new checks
all the existing ones will be performed.

Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_domain.c | 29 ++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 8b4efc82de..a45a676520 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4268,9 +4268,9 @@ qemuDomainDeviceDefValidateControllerSCSI(const 
virDomainControllerDef *controll
 
 
 static int
-qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef 
*controller,
- const virDomainDef *def,
- virQEMUCapsPtr qemuCaps)
+qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef 
*controller,
+const virDomainDef *def,
+virQEMUCapsPtr qemuCaps)
 {
 virDomainControllerModelPCI model = controller->model;
 const virDomainPCIControllerOpts *pciopts;
@@ -4547,6 +4547,29 @@ qemuDomainDeviceDefValidateControllerPCI(const 
virDomainControllerDef *controlle
 }
 
 
+static int
+qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont,
+ const virDomainDef *def,
+ virQEMUCapsPtr qemuCaps)
+
+{
+const virDomainPCIControllerOpts *pciopts = >opts.pciopts;
+const char *model = virDomainControllerModelPCITypeToString(cont->model);
+const char *modelName = 
virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
+
+if (!model) {
+virReportEnumRangeError(virDomainControllerModelPCI, cont->model);
+return -1;
+}
+if (!modelName) {
+virReportEnumRangeError(virDomainControllerPCIModelName, 
pciopts->modelName);
+return -1;
+}
+
+return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps);
+}
+
+
 static int
 qemuDomainDeviceDefValidateControllerSATA(const virDomainControllerDef 
*controller,
   const virDomainDef *def,
-- 
2.14.3

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

[libvirt] [PATCH v5 09/12] qemu: Validate PCI controller options (chassisNr)

2018-03-02 Thread Andrea Bolognani

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

Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_domain.c | 40 ++--
 1 file changed, 34 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 07ed006f70..d4ac4e2038 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4290,12 +4290,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const 
virDomainControllerDef *contro
 /* Second pass - now the model specific checks */
 switch (model) {
 case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
-if (pciopts->chassisNr == -1) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("autogenerated pci-bridge options not set"));
-return -1;
-}
-
 if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("the pci-bridge controller is not supported "
@@ -4799,6 +4793,40 @@ qemuDomainDeviceDefValidateControllerPCI(const 
virDomainControllerDef *cont,
 return -1;
 }
 
+/* chassisNr */
+switch ((virDomainControllerModelPCI) cont->model) {
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
+if (pciopts->chassisNr == -1) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Option '%s' not set for '%s' controller"),
+   "chassisNr", model);
+return -1;
+}
+break;
+
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
+case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
+if (pciopts->chassisNr != -1) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Option '%s' is not valid for '%s' controller"),
+   "chassisNr", model);
+return -1;
+}
+break;
+
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
+default:
+virReportEnumRangeError(virDomainControllerModelPCI, cont->model);
+return -1;
+}
+
 return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps);
 }
 
-- 
2.14.3

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

[libvirt] [PATCH v5 08/12] qemu: Validate PCI controller options (numaNode)

2018-03-02 Thread Andrea Bolognani

This change catches an invalid use of the option in our
test suite.

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

Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_domain.c | 52 ++
 tests/qemuxml2argvdata/pcie-expander-bus.xml   |  3 --
 tests/qemuxml2xmloutdata/pcie-expander-bus.xml |  4 +-
 3 files changed, 53 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 1354d9850a..07ed006f70 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4747,6 +4747,58 @@ qemuDomainDeviceDefValidateControllerPCI(const 
virDomainControllerDef *cont,
 return -1;
 }
 
+/* numaNode */
+switch ((virDomainControllerModelPCI) cont->model) {
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
+/* numaNode can be used for these controllers, but it's not set
+ * automatically so it can be missing */
+break;
+
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
+/* Only PHBs support numaNode */
+if (pciopts->numaNode != -1 &&
+pciopts->modelName != 
VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Option '%s' is not valid for '%s' controller"),
+   "numaNode", model);
+return -1;
+}
+
+/* However, the default PHB doesn't support numaNode */
+if (pciopts->numaNode != -1 &&
+pciopts->modelName == 
VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE &&
+pciopts->targetIndex == 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Option '%s' is not valid for '%s' controller "
+ "with '%s' equal to 0"),
+   "numaNode", model,
+   "targetIndex");
+return -1;
+}
+break;
+
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
+case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
+if (pciopts->numaNode != -1) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Option '%s' is not valid for '%s' controller"),
+   "numaNode", model);
+return -1;
+}
+break;
+
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
+default:
+virReportEnumRangeError(virDomainControllerModelPCI, cont->model);
+return -1;
+}
+
 return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps);
 }
 
diff --git a/tests/qemuxml2argvdata/pcie-expander-bus.xml 
b/tests/qemuxml2argvdata/pcie-expander-bus.xml
index 237430a1e5..5c5d34d1e0 100644
--- a/tests/qemuxml2argvdata/pcie-expander-bus.xml
+++ b/tests/qemuxml2argvdata/pcie-expander-bus.xml
@@ -35,9 +35,6 @@
   
 
 
-  
-1
-  
   
 
 
diff --git a/tests/qemuxml2xmloutdata/pcie-expander-bus.xml 
b/tests/qemuxml2xmloutdata/pcie-expander-bus.xml
index d5e741b80d..b6498fd2eb 100644
--- a/tests/qemuxml2xmloutdata/pcie-expander-bus.xml
+++ b/tests/qemuxml2xmloutdata/pcie-expander-bus.xml
@@ -36,9 +36,7 @@
 
 
   
-  
-1
-  
+  
   
 
 
-- 
2.14.3

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

[libvirt] [PATCH v5 05/12] qemu: Validate PCI controller options (targetIndex)

2018-03-02 Thread Andrea Bolognani

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

Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_domain.c | 52 --
 1 file changed, 46 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 3ef5d74e7a..54e47acd99 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4401,12 +4401,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const 
virDomainControllerDef *contro
 break;
 
 case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
-if (pciopts->targetIndex == -1) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("autogenerated pci-root options not set"));
-return -1;
-}
-
 /* Skip the implicit one */
 if (pciopts->targetIndex == 0)
 return 0;
@@ -4645,6 +4639,52 @@ qemuDomainDeviceDefValidateControllerPCI(const 
virDomainControllerDef *cont,
 return -1;
 }
 
+/* targetIndex */
+switch ((virDomainControllerModelPCI) cont->model) {
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
+/* PHBs for pSeries guests must have been assigned a targetIndex */
+if (pciopts->targetIndex == -1 &&
+pciopts->modelName == 
VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Option '%s' not set for '%s' controller"),
+   "targetIndex", model);
+return -1;
+}
+
+/* targetIndex only applies to PHBs, so for any other pci-root
+ * controller it being present is an error */
+if (pciopts->targetIndex != -1 &&
+pciopts->modelName != 
VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Option '%s' is not valid for '%s' controller"),
+   "targetIndex", model);
+return -1;
+}
+break;
+
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
+case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
+if (pciopts->targetIndex != -1) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Option '%s' is not valid for '%s' controller"),
+   "targetIndex", model);
+return -1;
+}
+break;
+
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
+default:
+virReportEnumRangeError(virDomainControllerModelPCI, cont->model);
+return -1;
+}
+
 return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps);
 }
 
-- 
2.14.3

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

[libvirt] [PATCH v5 11/12] qemu: Validate PCI controllers (QEMU capabilities)

2018-03-02 Thread Andrea Bolognani

Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_domain.c | 190 -
 1 file changed, 77 insertions(+), 113 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f54e7b87ae..e0ab43e139 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4270,11 +4270,8 @@ qemuDomainDeviceDefValidateControllerSCSI(const 
virDomainControllerDef *controll
 static int
 qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef 
*controller,
 const virDomainDef *def,
-virQEMUCapsPtr qemuCaps)
+virQEMUCapsPtr qemuCaps 
ATTRIBUTE_UNUSED)
 {
-virDomainControllerModelPCI model = controller->model;
-const virDomainPCIControllerOpts *pciopts;
-
 /* skip pcie-root */
 if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)
 return 0;
@@ -4285,119 +4282,50 @@ qemuDomainDeviceDefValidateControllerPCIOld(const 
virDomainControllerDef *contro
 controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT)
 return 0;
 
-pciopts = >opts.pciopts;
-
-/* Second pass - now the model specific checks */
-switch (model) {
-case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
-if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("the pci-bridge controller is not supported "
- "in this QEMU binary"));
-return -1;
-}
-
-break;
-
-case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
-if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("the pxb controller is not supported in this "
- "QEMU binary"));
-return -1;
-}
-
-break;
-
-case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
-if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("the dmi-to-pci-bridge (i82801b11-bridge) "
- "controller is not supported in this QEMU 
binary"));
-return -1;
-}
-
-break;
-
-case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
-if ((pciopts->modelName == 
VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) &&
-!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IOH3420)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("the pcie-root-port (ioh3420) controller "
- "is not supported in this QEMU binary"));
-return -1;
-}
-
-if ((pciopts->modelName == 
VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT) &&
-!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCIE_ROOT_PORT)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("the pcie-root-port (pcie-root-port) controller "
- "is not supported in this QEMU binary"));
-return -1;
-}
-
-break;
-
-case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
-if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_X3130_UPSTREAM)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("the pcie-switch-upstream-port (x3130-upstream) "
- "controller is not supported in this QEMU 
binary"));
-return -1;
-}
-
-break;
-
-case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
-if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("The pcie-switch-downstream-port "
- "(xio3130-downstream) controller is not "
- "supported in this QEMU binary"));
-return -1;
-}
-
-break;
-
-case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
-if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB_PCIE)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("the pxb-pcie controller is not supported "
- "in this QEMU binary"));
-return -1;
-}
-
-break;
-
-case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
-/* Skip the implicit one */
-if (pciopts->targetIndex == 0)
-return 0;
-
-if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE)) 
{
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("the spapr-pci-host-bridge controller is not "
-

[libvirt] [PATCH v5 10/12] qemu: Validate PCI controller options (chassis and port)

2018-03-02 Thread Andrea Bolognani

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

Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_domain.c | 58 +++---
 1 file changed, 45 insertions(+), 13 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index d4ac4e2038..f54e7b87ae 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4320,12 +4320,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const 
virDomainControllerDef *contro
 break;
 
 case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
-if (pciopts->chassis == -1 || pciopts->port == -1) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("autogenerated pcie-root-port options not set"));
-return -1;
-}
-
 if ((pciopts->modelName == 
VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) &&
 !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IOH3420)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -4355,13 +4349,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const 
virDomainControllerDef *contro
 break;
 
 case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
-if (pciopts->chassis == -1 || pciopts->port == -1) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("autogenerated pcie-switch-downstream-port "
- "options not set"));
-return -1;
-}
-
 if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("The pcie-switch-downstream-port "
@@ -4827,6 +4814,51 @@ qemuDomainDeviceDefValidateControllerPCI(const 
virDomainControllerDef *cont,
 return -1;
 }
 
+/* chassis and port */
+switch ((virDomainControllerModelPCI) cont->model) {
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
+if (pciopts->chassis == -1) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Option '%s' not set for '%s' controller"),
+   "chassis", model);
+return -1;
+}
+if (pciopts->port == -1) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Option '%s' not set for '%s' controller"),
+   "port", model);
+return -1;
+}
+break;
+
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
+case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
+if (pciopts->chassis != -1) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Option '%s' is not valid for '%s' controller"),
+   "chassis", model);
+return -1;
+}
+if (pciopts->port != -1) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Option '%s' is not valid for '%s' controller"),
+   "port", model);
+return -1;
+}
+break;
+
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
+default:
+virReportEnumRangeError(virDomainControllerModelPCI, cont->model);
+}
+
 return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps);
 }
 
-- 
2.14.3

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

[libvirt] [PATCH v5 04/12] qemu: Validate PCI controller options (index)

2018-03-02 Thread Andrea Bolognani

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

Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_domain.c | 68 +++---
 1 file changed, 43 insertions(+), 25 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 13c2b557fb..3ef5d74e7a 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4285,31 +4285,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const 
virDomainControllerDef *contro
 controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT)
 return 0;
 
-/* First pass - just check the controller index for the model's
- * that we care to check... */
-switch (model) {
-case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
-case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
-case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
-case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
-case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
-case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
-case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
-if (controller->idx == 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("index for pci controllers of model '%s' must be 
> 0"),
-   virDomainControllerModelPCITypeToString(model));
-return -1;
-}
-break;
-
-case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
-case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
-case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT:
-case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
-break;
-}
-
 pciopts = >opts.pciopts;
 
 /* Second pass - now the model specific checks */
@@ -4627,6 +4602,49 @@ qemuDomainDeviceDefValidateControllerPCI(const 
virDomainControllerDef *cont,
 return -1;
 }
 
+/* index */
+switch ((virDomainControllerModelPCI) cont->model) {
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
+case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
+if (cont->idx == 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Index for '%s' controllers must be > 0"),
+   model);
+return -1;
+}
+break;
+
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
+/* pSeries guests can have multiple PHBs, so it's expected that
+ * the index will not be zero for some of them */
+if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT &&
+pciopts->modelName == 
VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) {
+break;
+}
+
+/* For all other pci-root and pcie-root controllers, though,
+ * the index must be zero*/
+if (cont->idx != 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Index for '%s' controllers must be 0"),
+   model);
+return -1;
+}
+break;
+
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
+default:
+virReportEnumRangeError(virDomainControllerModelPCI, cont->model);
+return -1;
+}
+
 return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps);
 }
 
-- 
2.14.3

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

[libvirt] [PATCH v5 00/12] qemu: Validate PCI controller options

2018-03-02 Thread Andrea Bolognani

Applies cleanly on top of 328b8dbe8bee9939c7108fdec4fda05fd02511f6.

Changes from [v4]:

  * patch 1/12 is new;
  * use virReportEnumRangeError(), as suggested by laine.

Changes from [v3]:

  * don't introduce new test cases that won't be able to provide
full test coverage anyway, as suggested by laine.

Changes from [v2]:

  * replace the old implementation bit by bit using a clever trick
suggested by pkrempa;
  * don't move QEMU capability validation;
  * add a default: label to all switch statements as recommended
by danpb.

Changes from [v1]:

  * error out instead of silently accept invalid options;
  * shave quite a lot of yaks.

[v4] https://www.redhat.com/archives/libvir-list/2018-February/msg01232.html
[v3] https://www.redhat.com/archives/libvir-list/2018-February/msg00996.html
[v2] https://www.redhat.com/archives/libvir-list/2018-February/msg00813.html
[v1] https://www.redhat.com/archives/libvir-list/2018-February/msg00244.html

Andrea Bolognani (12):
  conf: Assign explicit value to
VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE
  qemu: Create new qemuDomainDeviceDefValidateControllerPCI()
  qemu: Validate PCI controller options (modelName)
  qemu: Validate PCI controller options (index)
  qemu: Validate PCI controller options (targetIndex)
  qemu: Validate PCI controller options (pcihole64)
  qemu: Validate PCI controller options (busNr)
  qemu: Validate PCI controller options (numaNode)
  qemu: Validate PCI controller options (chassisNr)
  qemu: Validate PCI controller options (chassis and port)
  qemu: Validate PCI controllers (QEMU capabilities)
  qemu: Remove old qemuDomainDeviceDefValidateControllerPCI()

 src/conf/domain_conf.h |   2 +-
 src/qemu/qemu_domain.c | 571 ++---
 tests/qemuxml2argvdata/pcie-expander-bus.xml   |   3 -
 tests/qemuxml2xmloutdata/pcie-expander-bus.xml |   4 +-
 4 files changed, 418 insertions(+), 162 deletions(-)

-- 
2.14.3

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

[libvirt] [PATCH v5 01/12] conf: Assign explicit value to VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE

2018-03-02 Thread Andrea Bolognani

Pretty much any reasonable compiler would do this automatically,
but there's no harm in being explicit about it.

Signed-off-by: Andrea Bolognani 
---
 src/conf/domain_conf.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 368f16f3fb..a04f96169c 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -700,7 +700,7 @@ typedef enum {
 } virDomainControllerModelPCI;
 
 typedef enum {
-VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE,
+VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE = 0,
 VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE,
 VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE,
 VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420,
-- 
2.14.3

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

Re: [libvirt] [PATCH] conf: set postParseFailed even without ALLOW_POST_PARSE_FAIL

2018-03-02 Thread Peter Krempa

On Fri, Mar 02, 2018 at 15:17:55 +0100, Ján Tomko wrote:
> We allow the postParse callbacks to fail for some reasons (missing
> emulator binary) when parsing the configs from /etc/libvirt.
> In that case, def->postParseFailed is set to true and the post
> parse callbacks are re-executed on domain startup.
> 
> However this bool was only set when virDomainDefPostParse was called
> with the ALLOW_POST_PARSE_FAIL flag set. If the callback failed
> again on domain startup, the bool would be reset and subsequent
> startups would not attempt to reexecute the callback.
> 
> Signed-off-by: Ján Tomko 
> ---
>  src/conf/domain_conf.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

ACK


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 06/12] qemu: Generate cmd line at startup

2018-03-02 Thread Peter Krempa

On Fri, Mar 02, 2018 at 08:22:32 -0500, John Ferlan wrote:
> 
> 
> On 02/21/2018 01:11 PM, Michal Privoznik wrote:
> > This is the easier part. All we need to do here is put -object
> > pr-manger-helper,id=$alias,path=$socketPath and then just
> > reference the object in -drive file.pr-manger=$alias.
> 
> s/manger/manager/
> 
> My fingers usually the same way though as manger
> 
> > 
> > Signed-off-by: Michal Privoznik 
> > ---
> >  src/qemu/qemu_command.c| 40 
> > ++
> >  .../disk-virtio-scsi-reservations-not-managed.args | 28 +++
> >  .../disk-virtio-scsi-reservations.args | 29 
> >  tests/qemuxml2argvtest.c   |  8 +
> >  4 files changed, 105 insertions(+)
> >  create mode 100644 
> > tests/qemuxml2argvdata/disk-virtio-scsi-reservations-not-managed.args
> >  create mode 100644 
> > tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args
> > 
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index fa0aa5d5c..069d60d35 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -1514,6 +1514,20 @@ qemuDiskSourceGetProps(virStorageSourcePtr src)
> >  }
> >  
> >  
> > +static void
> > +qemuBuildDriveSourcePR(virBufferPtr buf,
> > +   virStorageSourcePtr src)
> > +{
> > +qemuDomainStorageSourcePrivatePtr srcPriv = 
> > QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
> > +qemuDomainDiskPRDPtr prd = srcPriv->prd;
> > +
> > +if (!prd || !prd->alias)
> > +return;
> > +
> > +virBufferAsprintf(buf, ",file.pr-manager=%s", prd->alias);
> > +}
> > +
> > +
> >  static int
> >  qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
> >  virQEMUCapsPtr qemuCaps,
> > @@ -1590,6 +1604,8 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
> >  
> >  if (disk->src->debug)
> >  virBufferAsprintf(buf, ",file.debug=%d", 
> > disk->src->debugLevel);
> > +
> > +qemuBuildDriveSourcePR(buf, disk->src);
> >  } else {
> >  if (!(source = virQEMUBuildDriveCommandlineFromJSON(srcprops)))
> >  goto cleanup;
> > @@ -9789,6 +9805,28 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
> >  }
> >  
> >  
> > +static void
> > +qemuBuildMasterPRCommandLine(virCommandPtr cmd,
> > + const virDomainDef *def)
> > +{
> > +size_t i;
> > +
> > +for (i = 0; i < def->ndisks; i++) {
> > +const virDomainDiskDef *disk = def->disks[i];
> > +qemuDomainStorageSourcePrivatePtr srcPriv = 
> > QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
> > +qemuDomainDiskPRDPtr prd = srcPriv->prd;
> > +virBuffer buf = VIR_BUFFER_INITIALIZER;
> > +
> > +if (!prd || !prd->alias)
> > +continue;
> > +
> > +virBufferAsprintf(, "pr-manager-helper,id=%s,path=%s", 
> > prd->alias, prd->path);
> > +virCommandAddArg(cmd, "-object");
> > +virCommandAddArgBuffer(cmd, );
> 
> What happens when there's more than one disk using the managed mode
> where you have a "static" alias and path, wouldn't there be multiple
> lines with:
> 
> -object
> pr-manager-helper,id=pr-helper0,path=/tmp/lib/domain--1-QEMUGuest1/pr-helper0.sock
> 
> ?  And how is QEMU going to react to that?
> 
> IOW: Shouldn't this code know it's already created an object for that
> case and not generate another one?
> 
> The other one :
> 
> -object pr-manager-helper,id=pr-helper-sdb,path=/path/to/qemu-pr-helper.sock
> 
> I get, but I'm still not thrilled with "sdb" as opposed to the
> disk->info.alias of "scsi0-0-0-0" more commonly used. IIRC, there's no
> guarantee that what libvirt calls "sdb" ends up being "sdb" on the
> guest. My vague recollection of the algorithm that "automagically"
> generates the address based on sda, sdb, sdc, etc. So "sdb" IIRC would
> related to an address that would create an alias using "0-0-1"; whereas,
> "sda" would create that "0-0-0" value.
> 
> The fact that you've defined the  and  originally
> avoids the virDomainDiskDefAssignAddress algorithm. Also see the
> virDiskNameToBusDeviceIndex as well as the virDomainHostdevAssignAddress
> (and code around that) in order to see the awfulness of which I write.
> The real fun begins when you have 's and 's.
> 
> BTW: Seeing this and thinking about the command line jiggles a memory
> thread related to virStorageSourceParseBacking* and related tests where
> the code can handle various JSON outputs where it's not clear to me
> whether you'll need to add tests for the "file.pr-manager" processing. I
> think you might, but Peter understands more of that than I do.

No, this should not be handled in JSON at all. Referencing an alias in
JSON is wrong since it is tied to the one single run of the VM where
that would be created and could be something completely different in any
other run of the VM


signature.asc
Description: PGP signature
--
libvir-list

[libvirt] [PATCH] conf: set postParseFailed even without ALLOW_POST_PARSE_FAIL

2018-03-02 Thread Ján Tomko

We allow the postParse callbacks to fail for some reasons (missing
emulator binary) when parsing the configs from /etc/libvirt.
In that case, def->postParseFailed is set to true and the post
parse callbacks are re-executed on domain startup.

However this bool was only set when virDomainDefPostParse was called
with the ALLOW_POST_PARSE_FAIL flag set. If the callback failed
again on domain startup, the bool would be reset and subsequent
startups would not attempt to reexecute the callback.

Signed-off-by: Ján Tomko 
---
 src/conf/domain_conf.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index fcafc8b2f..a248d73de 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5027,6 +5027,9 @@ virDomainDefPostParseCheckFailure(virDomainDefPtr def,
   unsigned int parseFlags,
   int ret)
 {
+if (ret != 0)
+def->postParseFailed = true;
+
 if (ret <= 0)
 return ret;
 
@@ -5034,7 +5037,6 @@ virDomainDefPostParseCheckFailure(virDomainDefPtr def,
 return -1;
 
 virResetLastError();
-def->postParseFailed = true;
 return 0;
 }
 
-- 
2.13.6

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

Re: [libvirt] [PATCH v2 07/12] qemu: Introduce pr_helper to qemu.conf

2018-03-02 Thread John Ferlan



On 02/21/2018 01:11 PM, Michal Privoznik wrote:
> Just like we allow users overriding path to bridge-helper
> detected at compile time we can allow them to override path to
> qemu-pr-helper.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  m4/virt-driver-qemu.m4 | 5 +
>  src/qemu/libvirtd_qemu.aug | 1 +
>  src/qemu/qemu.conf | 4 
>  src/qemu/qemu_conf.c   | 7 ++-
>  src/qemu/qemu_conf.h   | 1 +
>  src/qemu/test_libvirtd_qemu.aug.in | 1 +
>  6 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/m4/virt-driver-qemu.m4 b/m4/virt-driver-qemu.m4
> index b9bafdab9..0e0f261d6 100644
> --- a/m4/virt-driver-qemu.m4
> +++ b/m4/virt-driver-qemu.m4
> @@ -57,6 +57,11 @@ AC_DEFUN([LIBVIRT_DRIVER_CHECK_QEMU], [
> [/usr/libexec:/usr/lib/qemu:/usr/lib])
>AC_DEFINE_UNQUOTED([QEMU_BRIDGE_HELPER], ["$QEMU_BRIDGE_HELPER"],
>   [QEMU bridge helper])
> +  AC_PATH_PROG([QEMU_PR_HELPER], [qemu-pr-helper],
> +   [/usr/libexec/qemu-pr-helper],
> +   [/usr/libexec:/usr/lib/qemu:/usr/lib])
> +  AC_DEFINE_UNQUOTED([QEMU_PR_HELPER], ["$QEMU_PR_HELPER"],
> + [QEMU PR helper])
>  ])
>  
>  AC_DEFUN([LIBVIRT_DRIVER_RESULT_QEMU], [
> diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
> index c19bf3a43..2dc16e91f 100644
> --- a/src/qemu/libvirtd_qemu.aug
> +++ b/src/qemu/libvirtd_qemu.aug
> @@ -86,6 +86,7 @@ module Libvirtd_qemu =
> let process_entry = str_entry "hugetlbfs_mount"
>   | bool_entry "clear_emulator_capabilities"
>   | str_entry "bridge_helper"
> + | str_entry "pr_helper"
>   | bool_entry "set_process_name"
>   | int_entry "max_processes"
>   | int_entry "max_files"
> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> index 43dd561cc..4bc668406 100644
> --- a/src/qemu/qemu.conf
> +++ b/src/qemu/qemu.conf
> @@ -775,3 +775,7 @@
>  # This directory is used for memoryBacking source if configured as file.
>  # NOTE: big files will be stored here
>  #memory_backing_dir = "/var/lib/libvirt/qemu/ram"
> +
> +# Path to the SCSI persistent reservations helper. This helper is
> +# used whenever  are enabled for SCSI disks.
> +#pr_helper = "/usr/libexec/qemu-pr-helper"

So "how" would I know as a user/admin whether or not I *needed* to
uncomment this? or how it's used. From how I read bridge_helper it's
there because "This executable is used to create 
interfaces when libvirtd is running unprivileged." - So then the
question becomes does PR make sense or is it useful for the running
unprivileged case? In fact, IIRC, using PR requires a bit of special
permissions and certain capabilities anyway (related to sg_io or rawio).
So is having this available in qemu.conf "required"? I cannot imagine
the name of the image changing.

Are there any other qualifiers than '-k' that may be useful to turn on
in the pr helper...

> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 36cf3a281..8c69dbe75 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -307,7 +307,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool 
> privileged)
>  goto error;
>  }
>  
> -if (VIR_STRDUP(cfg->bridgeHelperName, QEMU_BRIDGE_HELPER) < 0)
> +if (VIR_STRDUP(cfg->bridgeHelperName, QEMU_BRIDGE_HELPER) < 0 ||
> +VIR_STRDUP(cfg->prHelperName, QEMU_PR_HELPER) < 0)
>  goto error;
>  
>  cfg->clearEmulatorCapabilities = true;
> @@ -392,6 +393,7 @@ static void virQEMUDriverConfigDispose(void *obj)
>  }
>  VIR_FREE(cfg->hugetlbfs);
>  VIR_FREE(cfg->bridgeHelperName);
> +VIR_FREE(cfg->prHelperName);
>  
>  VIR_FREE(cfg->saveImageFormat);
>  VIR_FREE(cfg->dumpImageFormat);
> @@ -759,6 +761,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr 
> cfg,
>  if (virConfGetValueString(conf, "bridge_helper", >bridgeHelperName) 
> < 0)
>  goto cleanup;
>  
> +if (virConfGetValueString(conf, "pr_helper", >prHelperName) < 0)
> +goto cleanup;
> +
>  if (virConfGetValueBool(conf, "mac_filter", >macFilter) < 0)
>  goto cleanup;
>  
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index 83fd45282..5baa3af5d 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -153,6 +153,7 @@ struct _virQEMUDriverConfig {
>  size_t nhugetlbfs;
>  
>  char *bridgeHelperName;
> +char *prHelperName;
>  
>  bool macFilter;
>  
> diff --git a/src/qemu/test_libvirtd_qemu.aug.in 
> b/src/qemu/test_libvirtd_qemu.aug.in
> index 688e5b9fd..c0efae47b 100644
> --- a/src/qemu/test_libvirtd_qemu.aug.in
> +++ b/src/qemu/test_libvirtd_qemu.aug.in
> @@ -100,3 +100,4 @@ module Test_libvirtd_qemu =
>  { "1" = "mount" }
>  }
>  { "memory_backing_dir" = "/var/lib/libvirt/qemu/ram" }
> +{ "pr_helper" = "/usr/libexec/qemu-pr-helper" }
> 

I'm

Re: [libvirt] [PATCH v2 06/12] qemu: Generate cmd line at startup

2018-03-02 Thread John Ferlan



On 02/21/2018 01:11 PM, Michal Privoznik wrote:
> This is the easier part. All we need to do here is put -object
> pr-manger-helper,id=$alias,path=$socketPath and then just
> reference the object in -drive file.pr-manger=$alias.

s/manger/manager/

My fingers usually the same way though as manger

> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_command.c| 40 
> ++
>  .../disk-virtio-scsi-reservations-not-managed.args | 28 +++
>  .../disk-virtio-scsi-reservations.args | 29 
>  tests/qemuxml2argvtest.c   |  8 +
>  4 files changed, 105 insertions(+)
>  create mode 100644 
> tests/qemuxml2argvdata/disk-virtio-scsi-reservations-not-managed.args
>  create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index fa0aa5d5c..069d60d35 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1514,6 +1514,20 @@ qemuDiskSourceGetProps(virStorageSourcePtr src)
>  }
>  
>  
> +static void
> +qemuBuildDriveSourcePR(virBufferPtr buf,
> +   virStorageSourcePtr src)
> +{
> +qemuDomainStorageSourcePrivatePtr srcPriv = 
> QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
> +qemuDomainDiskPRDPtr prd = srcPriv->prd;
> +
> +if (!prd || !prd->alias)
> +return;
> +
> +virBufferAsprintf(buf, ",file.pr-manager=%s", prd->alias);
> +}
> +
> +
>  static int
>  qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
>  virQEMUCapsPtr qemuCaps,
> @@ -1590,6 +1604,8 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
>  
>  if (disk->src->debug)
>  virBufferAsprintf(buf, ",file.debug=%d", disk->src->debugLevel);
> +
> +qemuBuildDriveSourcePR(buf, disk->src);
>  } else {
>  if (!(source = virQEMUBuildDriveCommandlineFromJSON(srcprops)))
>  goto cleanup;
> @@ -9789,6 +9805,28 @@ qemuBuildPanicCommandLine(virCommandPtr cmd,
>  }
>  
>  
> +static void
> +qemuBuildMasterPRCommandLine(virCommandPtr cmd,
> + const virDomainDef *def)
> +{
> +size_t i;
> +
> +for (i = 0; i < def->ndisks; i++) {
> +const virDomainDiskDef *disk = def->disks[i];
> +qemuDomainStorageSourcePrivatePtr srcPriv = 
> QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
> +qemuDomainDiskPRDPtr prd = srcPriv->prd;
> +virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> +if (!prd || !prd->alias)
> +continue;
> +
> +virBufferAsprintf(, "pr-manager-helper,id=%s,path=%s", 
> prd->alias, prd->path);
> +virCommandAddArg(cmd, "-object");
> +virCommandAddArgBuffer(cmd, );

What happens when there's more than one disk using the managed mode
where you have a "static" alias and path, wouldn't there be multiple
lines with:

-object
pr-manager-helper,id=pr-helper0,path=/tmp/lib/domain--1-QEMUGuest1/pr-helper0.sock

?  And how is QEMU going to react to that?

IOW: Shouldn't this code know it's already created an object for that
case and not generate another one?

The other one :

-object pr-manager-helper,id=pr-helper-sdb,path=/path/to/qemu-pr-helper.sock

I get, but I'm still not thrilled with "sdb" as opposed to the
disk->info.alias of "scsi0-0-0-0" more commonly used. IIRC, there's no
guarantee that what libvirt calls "sdb" ends up being "sdb" on the
guest. My vague recollection of the algorithm that "automagically"
generates the address based on sda, sdb, sdc, etc. So "sdb" IIRC would
related to an address that would create an alias using "0-0-1"; whereas,
"sda" would create that "0-0-0" value.

The fact that you've defined the  and  originally
avoids the virDomainDiskDefAssignAddress algorithm. Also see the
virDiskNameToBusDeviceIndex as well as the virDomainHostdevAssignAddress
(and code around that) in order to see the awfulness of which I write.
The real fun begins when you have 's and 's.

BTW: Seeing this and thinking about the command line jiggles a memory
thread related to virStorageSourceParseBacking* and related tests where
the code can handle various JSON outputs where it's not clear to me
whether you'll need to add tests for the "file.pr-manager" processing. I
think you might, but Peter understands more of that than I do.

John

> +}
> +}
> +
> +
>  /**
>   * qemuBuildCommandLineValidate:
>   *
> @@ -9941,6 +9979,8 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
>  if (qemuBuildMasterKeyCommandLine(cmd, priv) < 0)
>  goto error;
>  
> +qemuBuildMasterPRCommandLine(cmd, def);
> +
>  if (enableFips)
>  virCommandAddArg(cmd, "-enable-fips");
>  
> diff --git 
> a/tests/qemuxml2argvdata/disk-virtio-scsi-reservations-not-managed.args 
> b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations-not-managed.args
> new file mode 100644
> index 0..387432c18
> --- /dev/null
> +++

Re: [libvirt] [PATCH v3 02/12] qemu: Create new qemuDomainDeviceDefValidateControllerPCI()

2018-03-02 Thread Andrea Bolognani

On Fri, 2018-03-02 at 07:34 -0500, Laine Stump wrote:
> On 02/21/2018 09:14 AM, Andrea Bolognani wrote:
[...]
> > +static int
> > +qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef 
> > *cont,
> > + const virDomainDef *def,
> > + virQEMUCapsPtr qemuCaps)
> > +
> > +{
> > +const virDomainPCIControllerOpts *pciopts = >opts.pciopts;
> > +const char *model = 
> > virDomainControllerModelPCITypeToString(cont->model);
> > +const char *modelName = 
> > virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
> > +
> > +if (!model) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR,
> > +   _("Unknown virDomainControllerModelPCI value: %d"),
> > +   cont->model);
> > +return -1;
> > +}
> > +if (!modelName) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR,
> > +   _("Unknown virDomainControllerPCIModelName value: 
> > %d"),
> > +   pciopts->modelName);
> > +return -1;
> > +}
> 
> (meant to send this before, but kept forgetting...)
> 
> 1) I thought modelName wasn't set for pci-root. Doesn't the above cause
> a validation error in that case? (too early in the day, haven't tried it)

The default value is _MODEL_NAME_NONE aka zero, which is still part
of the enumeration, so virDomainControllerPCIModelNameTypeToString()
won't return NULL and no error will be raised. For pSeries guests,
it will be set to _MODEL_NAME_SPAPR_PCI_HOST_BRIDGE so once again no
problem there.

> 2) danpb made a nice new function to standardize/simplify errors of the
> above type: virReportEnumRangeError().

His efforts on switch normalization and me rebasing this series
happened pretty much at the same time; more specifically, the
function you're talking about was introduced in 3b1020ac805e, while
my series is based on the earlier f565321b26df.

I guess this means another rebase! Yay! \o/

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v2 05/12] qemu: Store pr runtime data in status XML

2018-03-02 Thread John Ferlan



On 02/21/2018 01:11 PM, Michal Privoznik wrote:
> Now that we generate pr-manager alias and socket path store them
> in status XML so that they are preserved across daemon restarts.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 90 
> --
>  1 file changed, 88 insertions(+), 2 deletions(-)
> 

Something that dawned on my this morning (sorry ;-)) - the ->alias could
easily be generated at reconnect time too.

> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index dffc4c30e..45205fd03 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2540,6 +2540,92 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
>  }
>  
>  
> +static int
> +qemuStorageSourcePrivateDataParsePR(xmlXPathContextPtr ctxt,
> +virStorageSourcePtr src)
> +{
> +qemuDomainStorageSourcePrivatePtr srcPriv = 
> QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
> +qemuDomainDiskPRDPtr prd = NULL;
> +int rc;
> +int ret = -1;
> +
> +if ((rc = virXPathBoolean("boolean(./prd)", ctxt)) ==  0) {
^^
Extra space above

> +return 0;
> +} else if (rc < 0) {
> +return ret;
> +}
> +
> +if (VIR_ALLOC(prd) < 0)
> +goto cleanup;

return ret works too since prd == NULL

> +
> +if (!(prd->alias = virXPathString("string(./prd/alias)", ctxt)) ||
> +!(prd->path = virXPathString("string(./prd/path)", ctxt))) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("malformed "));
> +goto cleanup;
> +}
> +
> +VIR_STEAL_PTR(srcPriv->prd, prd);
> +ret = 0;
> + cleanup:
> +qemuDomainDiskPRDFree(prd);
> +return ret;
> +}
> +
> +
> +static int
> +qemuStorageSourcePrivateDataFormatPR(virStorageSourcePtr src,
> + virBufferPtr buf)
> +{
> +qemuDomainStorageSourcePrivatePtr srcPriv = 
> QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
> +qemuDomainDiskPRDPtr prd;
> +
> +if (!srcPriv || !srcPriv->prd)
> +return 0;
> +
> +prd = srcPriv->prd;

Does saving the information really "matter" in whatever case it is that
uses a 'static' alias and path?  IOW: Should we save some sort of
boolean to indicate PR was desired and then if path is also provided, we
can use that path; otherwise, use the 'static' path when we try to
reconnect the socket.

> +
> +virBufferAddLit(buf, "\n");
> +virBufferAdjustIndent(buf, 2);
> +virBufferAsprintf(buf, "%s\n", prd->alias);
> +virBufferAsprintf(buf, "%s\n", prd->path);

alias and path could be attributes of prd too rather than elements on
their own, but that's just your implementation detail...  IDC, but
figured I'd note it anyway.

John


> +virBufferAdjustIndent(buf, -2);
> +virBufferAddLit(buf, "\n");
> +return 0;
> +}
> +
> +
> +static int
> +qemuStorageSourcePrivateDataParse(xmlXPathContextPtr ctxt,
> +  virStorageSourcePtr src)
> +{
> +if (!(src->privateData = qemuDomainStorageSourcePrivateNew()))
> +return -1;
> +
> +if (virStorageSourcePrivateDataParseRelPath(ctxt, src) < 0)
> +return -1;
> +
> +if (qemuStorageSourcePrivateDataParsePR(ctxt, src) < 0)
> +return -1;
> +
> +return 0;
> +}
> +
> +
> +static int
> +qemuStorageSourcePrivateDataFormat(virStorageSourcePtr src,
> +   virBufferPtr buf)
> +{
> +if (virStorageSourcePrivateDataFormatRelPath(src, buf) < 0)
> +return -1;
> +
> +if (qemuStorageSourcePrivateDataFormatPR(src, buf) < 0)
> +return -1;
> +
> +return 0;
> +}
> +
> +
>  virDomainXMLPrivateDataCallbacks virQEMUDriverPrivateDataCallbacks = {
>  .alloc = qemuDomainObjPrivateAlloc,
>  .free = qemuDomainObjPrivateFree,
> @@ -2548,8 +2634,8 @@ virDomainXMLPrivateDataCallbacks 
> virQEMUDriverPrivateDataCallbacks = {
>  .chrSourceNew = qemuDomainChrSourcePrivateNew,
>  .parse = qemuDomainObjPrivateXMLParse,
>  .format = qemuDomainObjPrivateXMLFormat,
> -.storageParse = virStorageSourcePrivateDataParseRelPath,
> -.storageFormat = virStorageSourcePrivateDataFormatRelPath,
> +.storageParse = qemuStorageSourcePrivateDataParse,
> +.storageFormat = qemuStorageSourcePrivateDataFormat,
>  };
>  
>  
> 

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

Re: [libvirt] [PATCH v2 01/12] virstoragefile: Introduce virStoragePRDef

2018-03-02 Thread John Ferlan



On 02/21/2018 01:11 PM, Michal Privoznik wrote:
> This is a definition that holds information on SCSI persistent
> reservation settings. The XML part looks like this:
> 
>   
> 
>   
> 
> If @managed is set to 'yes' then the  is not parsed.
> This design was agreed on here:
> 
> https://www.redhat.com/archives/libvir-list/2017-November/msg01005.html
> 
> Signed-off-by: Michal Privoznik 
> ---
>  docs/formatdomain.html.in  |  25 +++-
>  docs/schemas/domaincommon.rng  |  34 +
>  docs/schemas/storagecommon.rng |  50 +++
>  src/conf/domain_conf.c |  36 +
>  src/libvirt_private.syms   |   3 +
>  src/util/virstoragefile.c  | 148 
> +
>  src/util/virstoragefile.h  |  15 +++
>  .../disk-virtio-scsi-reservations-not-managed.xml  |  40 ++
>  .../disk-virtio-scsi-reservations.xml  |  38 ++
>  .../disk-virtio-scsi-reservations-not-managed.xml  |   1 +
>  .../disk-virtio-scsi-reservations.xml  |   1 +
>  tests/qemuxml2xmltest.c|   4 +
>  12 files changed, 364 insertions(+), 31 deletions(-)
>  create mode 100644 
> tests/qemuxml2argvdata/disk-virtio-scsi-reservations-not-managed.xml
>  create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml
>  create mode 12 
> tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations-not-managed.xml
>  create mode 12 tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations.xml
> 

Something else that dawned on me when looking at the original series.

[...]

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index f2ddde7a3..a1a6b0162 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5199,6 +5199,13 @@ virDomainDiskDefValidate(const virDomainDiskDef *disk)
>  }
>  }
>  
> +if (disk->src->pr &&
> +disk->device != VIR_DOMAIN_DISK_DEVICE_LUN) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _(" allowed only for lun disks"));
> +return -1;
> +}
> +

If someone is using a storage pool such as:

   
 
 
 
   

then how do they define/use the PR? For LUN's like iSCSI and NPIV it can
be easier or even preferred to use the storage pool as opposed to using
the  syntax.

Search on iscsi in the formatdomain page for iSCSI syntax examples.

See: https://wiki.libvirt.org/page/NPIV_in_libvirt for the NPIV syntax
examples.

John

>  /* Reject disks with a bus type that is not compatible with the
>   * given address type. The function considers only buses that are
>   * handled in common code. For other bus types it's not possible
> @@ -8613,6 +8620,29 @@ virDomainDiskSourcePrivateDataParse(xmlXPathContextPtr 
> ctxt,
>  }
>  
>  

[...]

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

Re: [libvirt] [PATCH 2/6] qemu: use query-cpus-fast in JSON monitor

2018-03-02 Thread Peter Krempa

On Fri, Mar 02, 2018 at 13:32:19 +0100, Viktor Mihajlovski wrote:
> On 02.03.2018 12:33, Peter Krempa wrote:
> [...]
> >> @@ -2028,8 +2029,12 @@ qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
> >>  (qemuMonitorJSONGetHotpluggableCPUs(mon, , 
> >> )) < 0)
> >>  goto cleanup;
> >>  
> >> +fast = virQEMUCapsGet(QEMU_DOMAIN_PRIVATE(mon->vm)->qemuCaps,
> > 
> > Don't do this. You'll need to determine it sooner. The main reason being
> > that once you are in the monitor code, 'vm' is unlocked and should not
> > be accesssed. Also we don't access the VM object trhough the mon object
> > usually.
> > OK. I've been struggling over this part since the availability of
> query-cpus-fast is tied to the QEMU instance and I didn't want to
> duplicate the capability in the qemuMonitor struct. Looks like I have to ...

You can pass it from the caller as we do in more than one place.


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

Re: [libvirt] [PATCH v3 02/12] qemu: Create new qemuDomainDeviceDefValidateControllerPCI()

2018-03-02 Thread Laine Stump

On 02/21/2018 09:14 AM, Andrea Bolognani wrote:
> The esisting function is renamed and called from the new one, so
> that even while we're in the process of implementing new checks
> all the existing ones will be performed.
>
> Signed-off-by: Andrea Bolognani 
> ---
> Based on the reviewer's preference, this patch can be pushed on
> its own or squashed into the next one.
>
>  src/qemu/qemu_domain.c | 33 ++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index b1308e5a49..151d33aee3 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4247,9 +4247,9 @@ qemuDomainDeviceDefValidateControllerSCSI(const 
> virDomainControllerDef *controll
>  
>  
>  static int
> -qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef 
> *controller,
> - const virDomainDef *def,
> - virQEMUCapsPtr qemuCaps)
> +qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef 
> *controller,
> +const virDomainDef *def,
> +virQEMUCapsPtr qemuCaps)
>  {
>  virDomainControllerModelPCI model = controller->model;
>  const virDomainPCIControllerOpts *pciopts;
> @@ -4526,6 +4526,33 @@ qemuDomainDeviceDefValidateControllerPCI(const 
> virDomainControllerDef *controlle
>  }
>  
>  
> +static int
> +qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont,
> + const virDomainDef *def,
> + virQEMUCapsPtr qemuCaps)
> +
> +{
> +const virDomainPCIControllerOpts *pciopts = >opts.pciopts;
> +const char *model = virDomainControllerModelPCITypeToString(cont->model);
> +const char *modelName = 
> virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
> +
> +if (!model) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Unknown virDomainControllerModelPCI value: %d"),
> +   cont->model);
> +return -1;
> +}
> +if (!modelName) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Unknown virDomainControllerPCIModelName value: 
> %d"),
> +   pciopts->modelName);
> +return -1;
> +}

(meant to send this before, but kept forgetting...)

1) I thought modelName wasn't set for pci-root. Doesn't the above cause
a validation error in that case? (too early in the day, haven't tried it)

2) danpb made a nice new function to standardize/simplify errors of the
above type: virReportEnumRangeError().


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

Re: [libvirt] [PATCH 2/6] qemu: use query-cpus-fast in JSON monitor

2018-03-02 Thread Viktor Mihajlovski

On 02.03.2018 12:33, Peter Krempa wrote:
[...]
>> @@ -2028,8 +2029,12 @@ qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
>>  (qemuMonitorJSONGetHotpluggableCPUs(mon, , 
>> )) < 0)
>>  goto cleanup;
>>  
>> +fast = virQEMUCapsGet(QEMU_DOMAIN_PRIVATE(mon->vm)->qemuCaps,
> 
> Don't do this. You'll need to determine it sooner. The main reason being
> that once you are in the monitor code, 'vm' is unlocked and should not
> be accesssed. Also we don't access the VM object trhough the mon object
> usually.
> OK. I've been struggling over this part since the availability of
query-cpus-fast is tied to the QEMU instance and I didn't want to
duplicate the capability in the qemuMonitor struct. Looks like I have to ...

I'll rework the series (including your other comments).
[...]
-- 
Regards,
 Viktor Mihajlovski

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

Re: [libvirt] [PATCH v2 1/2] qemu: log the crash information for S390

2018-03-02 Thread Bjoern Walk

John Ferlan  [2018-03-01, 02:22PM -0500]:
> On 02/27/2018 04:32 AM, Bjoern Walk wrote:
> > +if (VIR_STRDUP(ret->data.s390.reason, reason) < 0) {
> > +virReportOOMError();
> > +goto error;
> > +}
> 
> No need for the OOMError as VIR_STRDUP will splat that for you (the
> _QUIET one won't).
> 
> I can fix that up before pushing though.

I always forget this. Appreciated and thanks a lot.

-- 
IBM Systems
Linux on Z & Virtualization Development

IBM Deutschland Research & Development GmbH
Schönaicher Str. 220, 71032 Böblingen
Phone: +49 7031 16 1819

Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294 


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

Re: [libvirt] [PATCH] qemu: remove unused QEMU_CAPS_ENABLE_KVM

2018-03-02 Thread Ján Tomko


On Fri, Mar 02, 2018 at 05:07:16AM +0300, Klim Kireev wrote:

It needs for the -enable-kvm flag. This qemu flag can be compiled out,
but we already detect that case with the QEMU_CAPS_KVM check.
So this check is redundant and can be removed

Signed-off-by: Klim Kireev 
---
src/qemu/qemu_capabilities.c   | 13 +
src/qemu/qemu_capabilities.h   |  2 +-
src/qemu/qemu_command.c| 14 ++
tests/qemucaps2xmldata/all_1.6.0-1.caps|  2 +-
tests/qemucaps2xmldata/nodisksnapshot_1.6.0-1.caps |  2 +-
tests/qemuhelptest.c   |  8 +++-
tests/qemuxml2argvdata/controller-order.args   |  1 -
tests/qemuxml2argvdata/cpu-host-kvmclock.args  |  1 -
tests/qemuxml2argvdata/cpu-kvmclock.args   |  1 -
tests/qemuxml2argvtest.c   | 11 +--
10 files changed, 14 insertions(+), 41 deletions(-)




diff --git a/tests/qemuxml2argvdata/controller-order.args 
b/tests/qemuxml2argvdata/controller-order.args
index 70a8ba9ce..fea56cc05 100644
--- a/tests/qemuxml2argvdata/controller-order.args
+++ b/tests/qemuxml2argvdata/controller-order.args
@@ -8,7 +8,6 @@ QEMU_AUDIO_DRV=spice \
-name fdr \
-S \
-M rhel6.1.0 \
--enable-kvm \


I presume the -enable-kvm option served some purpose and would like to
see some evidence that we can leave it out, rather than silently
breaking libvirt with some QEMUs that worked before.

On the other hand, these QEMUs (and kvm binaries?) are quite old, maybe
it's time to deprecate everything older than 1.3.0, fail loudly and
get rid of lots of legacy code, like most of the text monitor code,
parsing of 'qemu --help' [0] output and lots of old capabilities.

Jan

[0] if Michal won't have any objections


-m 4096 \
-smp 4,sockets=4,cores=1,threads=1 \
-uuid d091ea82-29e6-2e34-3005-f02617b36e87 \


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

Re: [libvirt] update keycodemapdb submodule for 4.1.0 release

2018-03-02 Thread Martin Kletzander


On Fri, Mar 02, 2018 at 10:45:35AM +, Daniel P. Berrangé wrote:

On Fri, Mar 02, 2018 at 11:24:03AM +0100, Andrea Bolognani wrote:

On Fri, 2018-03-02 at 08:08 +0100, Martin Kletzander wrote:
> On Thu, Mar 01, 2018 at 04:52:09PM +, Daniel P. Berrangé wrote:
> > You show that we we missing CI coverage for python3 though, so we
> > should address that. We could make our Fedora Jenkins CI builds use py3,
> > along with one of our Travis scenarios, so we get a mix of py2
> > coverage in both Jenkins & Travis.
>
> Do we?  I think the builds were switched so that we can run unit tests on
> virt-manager so now we have CI coverage for py2 as well as py3.

We use both Python 2 and 3, but only for project that are themselves
implemented in Python (libvirt-python and virt-manager); what I
think Dan is referring to is that we only ever use Python 2 to call
keycodemapdb's own tools, so there's no Python 3 coverage for that.


Yeah exactly - some bits of libvirt build process use python scripts and
so we should test that under both.



Oh, thanks for the explanation, that makes sense.


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


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

Re: [libvirt] [PATCH 1/6] qemu: add capability detection for query-cpus-fast

2018-03-02 Thread Peter Krempa

On Fri, Mar 02, 2018 at 10:29:06 +0100, Viktor Mihajlovski wrote:
> Detect whether QEMU supports the QMP query-cpus-fast API
> and set QEMU_CAPS_QUERY_CPUS_FAST in this case.
> 
> Signed-off-by: Viktor Mihajlovski 
> Reviewed-by: Boris Fiuczynski 
> Reviewed-by: Marc Hartmayer 
> ---
>  src/qemu/qemu_capabilities.c | 4 +++-
>  src/qemu/qemu_capabilities.h | 1 +
>  2 files changed, 4 insertions(+), 1 deletion(-)

ACK, although you should add a qemucapabilitiestest case for the new
qemu supporting this command first probably.



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

Re: [libvirt] [PATCH 4/6] qemu: add architecture-specific CPU info handling

2018-03-02 Thread Peter Krempa

On Fri, Mar 02, 2018 at 10:29:09 +0100, Viktor Mihajlovski wrote:
> Extract architecture specific data from query-cpus[-fast] if
> available. A new function qemuMonitorJSONExtractCPUArchInfo()
> uses a call-back table to find and call architecture-specific
> extraction handlers.
> 
> Initially, there's a handler for s390 cpu info to
> set the halted property depending on the s390 cpu state
> returned by QEMU. With this it's still possible to report
> the halted condition even when using query-cpus-fast.
> 
> Signed-off-by: Viktor Mihajlovski 
> Reviewed-by: Boris Fiuczynski 
> ---
>  src/qemu/qemu_monitor.c  |   6 ++-
>  src/qemu/qemu_monitor_json.c | 123 
> ++-
>  2 files changed, 126 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index dad1383..5a0e8a7 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -2083,12 +2083,16 @@ qemuMonitorGetCpuHalted(qemuMonitorPtr mon,
>  size_t i;
>  int rc;
>  virBitmapPtr ret = NULL;
> +bool fast;
>  
>  QEMU_CHECK_MONITOR_NULL(mon);
>  
> +fast = virQEMUCapsGet(QEMU_DOMAIN_PRIVATE(mon->vm)->qemuCaps,

As said, mon->vm should not be used here.

> +  QEMU_CAPS_QUERY_CPUS_FAST);
> +
>  if (mon->json)
>  rc = qemuMonitorJSONQueryCPUs(mon, , , false,
> -  false);
> +  fast);
>  else
>  rc = qemuMonitorTextQueryCPUs(mon, , );
>  
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 2ecdf0a..a408cfd 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -1451,15 +1451,128 @@ int qemuMonitorJSONSystemReset(qemuMonitorPtr mon)
>  }
>  
>  
> -/*
> +typedef struct {
> +const char *state;
> +bool halted;
> +} s390CpuStateMap;
> +
> +/**
> + * qemuMonitorJSONExtractCPUS390Info:
> + * @jsoncpu: pointer to a single JSON cpu entry
> + * @cpu: pointer to a single cpu entry
> + *
> + * Derive the legacy cpu info 'halted' information
> + * from the more accurate s390 cpu state. @cpu is
> + * modified only on success.
> + *
> + * Note: the 'uninitialized' s390 cpu state can't be
> + *   mapped to halted yes/no.
> + *
> + * A s390 cpu entry could look like this
> + *  { "arch": "s390",
> + *"cpu-index": 0,
> + *"qom-path": "/machine/unattached/device[0]",
> + *"thread_id": 3081,
> + *"cpu-state": "operating" }
> + *
> + * Returns 0 on success, and -2 if no cpu-state field was
> + * found or the state value is unknown.
> + */
> +static int
> +qemuMonitorJSONExtractCPUS390Info(virJSONValuePtr jsoncpu,
> +  struct qemuMonitorQueryCpusEntry *cpu)
> +{
> +const char *cpu_state;
> +s390CpuStateMap states[] = {
> +{ "operating", false},
> +{ "load", false},
> +{ "stopped", true},
> +{ "check-stop", true},
> +};
> +size_t i;
> +
> +if ((cpu_state = virJSONValueObjectGetString(jsoncpu, "cpu-state"))) {
> +for (i = 0; i < ARRAY_CARDINALITY(states); i++) {
> +if (STREQ(states[i].state, cpu_state)) {
> +cpu->halted = states[i].halted;
> +return 0;
> +}
> +}
> +}
> +
> +return -2;
> +}
> +
> +typedef struct {
> +const char *arch;
> +int (*extract)(virJSONValuePtr jsoncpu,
> +   struct qemuMonitorQueryCpusEntry *cpu);
> +} qemuCpuArchInfoHandler;
> +
> +
> +/**
> + * qemuMonitorJSONExtractCPUArchInfo:
> + * @arch: virtual CPU's architecture
> + * @jsoncpu: pointer to a single JSON cpu entry
> + * @cpu: pointer to a single cpu entry
> + *
> + * Extracts architecure specific virtual CPU data for a single
> + * CPU from the QAPI response using an architecture specific
> + * callback.
>   *
> + * To add a support for a new architecture, extend the array
> + * 'handlers' with a line like
> + * ...
> + *   { "newarch", qemuMonitorJSONExtractCPUNewarch },

This should be obvious from the code.

> + * ...
> + * and implement the extraction callback.
> + * Check the QEMU QAPI schema for valid architecture names.
> + *
> + * Returns the called handler's return value or 0, if no handler
> + * was called.
> + */
> +static int
> +qemuMonitorJSONExtractCPUArchInfo(const char *arch, virJSONValuePtr jsoncpu,
> +  struct qemuMonitorQueryCpusEntry *cpu)

Please don't mix header alignment styles.

> +{
> +qemuCpuArchInfoHandler handlers[] = {
> +{ "s390", qemuMonitorJSONExtractCPUS390Info },
> +};
> +size_t i;
> +
> +for (i = 0; i < ARRAY_CARDINALITY(handlers); i++) {
> +if (STREQ(handlers[i].arch, arch))
> +return handlers[i].extract(jsoncpu, cpu);

You can remove the callback topology and hard-code the STREQs. It will
result in the same execution

Re: [libvirt] [PATCH 2/6] qemu: use query-cpus-fast in JSON monitor

2018-03-02 Thread Peter Krempa

On Fri, Mar 02, 2018 at 10:29:07 +0100, Viktor Mihajlovski wrote:
> Use query-cpus-fast instead of query-cpus if supported by QEMU.
> Based on the QEMU_CAPS_QUERY_CPUS_FAST capability.
> 
> Signed-off-by: Viktor Mihajlovski 
> Reviewed-by: Boris Fiuczynski 
> ---
>  src/qemu/qemu_monitor.c  | 10 --
>  src/qemu/qemu_monitor_json.c | 26 +-
>  src/qemu/qemu_monitor_json.h |  3 ++-
>  tests/qemumonitorjsontest.c  |  2 +-
>  4 files changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index ad5c572..dad1383 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -2012,6 +2012,7 @@ qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
>  int ret = -1;
>  int rc;
>  qemuMonitorCPUInfoPtr info = NULL;
> +bool fast;
>  
>  QEMU_CHECK_MONITOR(mon);
>  
> @@ -2028,8 +2029,12 @@ qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
>  (qemuMonitorJSONGetHotpluggableCPUs(mon, , 
> )) < 0)
>  goto cleanup;
>  
> +fast = virQEMUCapsGet(QEMU_DOMAIN_PRIVATE(mon->vm)->qemuCaps,

Don't do this. You'll need to determine it sooner. The main reason being
that once you are in the monitor code, 'vm' is unlocked and should not
be accesssed. Also we don't access the VM object trhough the mon object
usually.

> +  QEMU_CAPS_QUERY_CPUS_FAST);
> +
>  if (mon->json)
> -rc = qemuMonitorJSONQueryCPUs(mon, , , 
> hotplug);
> +rc = qemuMonitorJSONQueryCPUs(mon, , , 
> hotplug,
> +  fast);
>  else
>  rc = qemuMonitorTextQueryCPUs(mon, , );
>  
> @@ -2082,7 +2087,8 @@ qemuMonitorGetCpuHalted(qemuMonitorPtr mon,
>  QEMU_CHECK_MONITOR_NULL(mon);
>  
>  if (mon->json)
> -rc = qemuMonitorJSONQueryCPUs(mon, , , false);
> +rc = qemuMonitorJSONQueryCPUs(mon, , , false,
> +  false);
>  else
>  rc = qemuMonitorTextQueryCPUs(mon, , );
>  
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index a09e93e..2ecdf0a 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -1466,7 +1466,8 @@ int qemuMonitorJSONSystemReset(qemuMonitorPtr mon)
>  static int
>  qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data,
>struct qemuMonitorQueryCpusEntry **entries,
> -  size_t *nentries)
> +  size_t *nentries,
> +  bool fast)
>  {
>  struct qemuMonitorQueryCpusEntry *cpus = NULL;
>  int ret = -1;
> @@ -1491,11 +1492,13 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data,
>  }
>  
>  /* Some older qemu versions don't report the thread_id so treat this 
> as
> - * non-fatal, simply returning no data */
> -ignore_value(virJSONValueObjectGetNumberInt(entry, "CPU", ));
> -ignore_value(virJSONValueObjectGetNumberInt(entry, "thread_id", 
> ));
> + * non-fatal, simply returning no data.
> + * The return data of query-cpus-fast has different field names
> + */
> +ignore_value(virJSONValueObjectGetNumberInt(entry, fast ? 
> "cpu-index" : "CPU", ));
> +ignore_value(virJSONValueObjectGetNumberInt(entry, fast ? 
> "thread-id" : "thread_id", ));

No ternary operators please. Add an if-block and two code paths.

>  ignore_value(virJSONValueObjectGetBoolean(entry, "halted", ));
> -qom_path = virJSONValueObjectGetString(entry, "qom_path");
> +qom_path = virJSONValueObjectGetString(entry, fast ? "qom-path" : 
> "qom_path");
>  
>  cpus[i].qemu_id = cpuid;
>  cpus[i].tid = thread;
> @@ -1520,10 +1523,12 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data,
>   * @mon: monitor object
>   * @entries: filled with detected entries on success
>   * @nentries: number of entries returned
> + * @force: force exit on error
> + * @fast: use query-cpus-fast
>   *
>   * Queries qemu for cpu-related information. Failure to execute the command 
> or
>   * extract results does not produce an error as libvirt can continue without
> - * this information.
> + * this information, unless the caller has specified @force == true.
>   *
>   * Returns 0 on success, -1 on a fatal error (oom ...) and -2 if the
>   * query failed gracefully.
> @@ -1532,10 +1537,13 @@ int
>  qemuMonitorJSONQueryCPUs(qemuMonitorPtr mon,
>   struct qemuMonitorQueryCpusEntry **entries,
>   size_t *nentries,
> - bool force)
> + bool force,
> + bool fast)
>  {
>  int ret = -1;
> -virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-cpus", NULL);
> +virJSONValuePtr cmd =
> +qemuMonitorJSONMakeCommand(fast ? "query-cpus-fast" : "query-cpus",
> +

Re: [libvirt] update keycodemapdb submodule for 4.1.0 release

2018-03-02 Thread Daniel P . Berrangé

On Fri, Mar 02, 2018 at 11:24:03AM +0100, Andrea Bolognani wrote:
> On Fri, 2018-03-02 at 08:08 +0100, Martin Kletzander wrote:
> > On Thu, Mar 01, 2018 at 04:52:09PM +, Daniel P. Berrangé wrote:
> > > You show that we we missing CI coverage for python3 though, so we
> > > should address that. We could make our Fedora Jenkins CI builds use py3,
> > > along with one of our Travis scenarios, so we get a mix of py2
> > > coverage in both Jenkins & Travis.
> > 
> > Do we?  I think the builds were switched so that we can run unit tests on
> > virt-manager so now we have CI coverage for py2 as well as py3.
> 
> We use both Python 2 and 3, but only for project that are themselves
> implemented in Python (libvirt-python and virt-manager); what I
> think Dan is referring to is that we only ever use Python 2 to call
> keycodemapdb's own tools, so there's no Python 3 coverage for that.

Yeah exactly - some bits of libvirt build process use python scripts and
so we should test that under both.

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

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

Re: [libvirt] update keycodemapdb submodule for 4.1.0 release

2018-03-02 Thread Andrea Bolognani

On Fri, 2018-03-02 at 08:08 +0100, Martin Kletzander wrote:
> On Thu, Mar 01, 2018 at 04:52:09PM +, Daniel P. Berrangé wrote:
> > You show that we we missing CI coverage for python3 though, so we
> > should address that. We could make our Fedora Jenkins CI builds use py3,
> > along with one of our Travis scenarios, so we get a mix of py2
> > coverage in both Jenkins & Travis.
> 
> Do we?  I think the builds were switched so that we can run unit tests on
> virt-manager so now we have CI coverage for py2 as well as py3.

We use both Python 2 and 3, but only for project that are themselves
implemented in Python (libvirt-python and virt-manager); what I
think Dan is referring to is that we only ever use Python 2 to call
keycodemapdb's own tools, so there's no Python 3 coverage for that.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [RFC PATCH v3 3/9] qemu: Prefer qom-list-properties to device-list-properties

2018-03-02 Thread Andrea Bolognani

On Thu, 2018-03-01 at 19:03 +0100, Andrea Bolognani wrote:
> @@ -2883,6 +2883,12 @@ virQEMUCapsProbeQMPObjects(virQEMUCapsPtr qemuCaps,
>  int nvalues;
>  char **values;
>  size_t i;
> +const char *impl = "device-list-properties";
> +
> +/* Prefer qom-list-properties if available, since it allows us to gather
> + * information about objects that device-list-properties doesn't support 
> */
> +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QOM_LIST_PROPERTIES))
> +impl = "qom-list-properties";
>
>  if ((nvalues = qemuMonitorGetObjectTypes(mon, )) < 0)
>  return -1;
> @@ -2899,9 +2905,7 @@ virQEMUCapsProbeQMPObjects(virQEMUCapsPtr qemuCaps,
>  if (cap >= 0 && !virQEMUCapsGet(qemuCaps, cap))
>  continue;
>
> -if ((nvalues = qemuMonitorGetObjectProps(mon,
> - type,
> - )) < 0)
> +if ((nvalues = qemuMonitorGetObjectProps(mon, impl, type, )) 
> < 0)
>  return -1;

I should note that I don't love this approach: I would much rather
pass qemuCaps all the way down to qemuMonitorJSONGetObjectProps()
and make the decision there for better encapsulation.

However, qemu_capabilities depends on qemu_monitor, so that would
introduce a circular dependency and I'm not sure trying to break it
up is worth the effort.

So if anyone fells like weighing in on the subject, or provide
alternative suggestions, I'm all ears.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[libvirt] [PATCH 3/6] tests: add qemumonitorjson tests for query-cpus-fast

2018-03-02 Thread Viktor Mihajlovski

Extended the json monitor test program with support for query-cpus-fast
and added a sample file set for x86 data obtained using the it.

Signed-off-by: Viktor Mihajlovski 
Reviewed-by: Boris Fiuczynski 
---
 ...qemumonitorjson-cpuinfo-x86-full-fast-cpus.json |  71 +
 ...umonitorjson-cpuinfo-x86-full-fast-hotplug.json | 115 
 .../qemumonitorjson-cpuinfo-x86-full-fast.data | 109 +++
 tests/qemumonitorjsontest.c| 116 -
 tests/qemumonitortestutils.c   |   6 ++
 tests/qemumonitortestutils.h   |   1 +
 6 files changed, 393 insertions(+), 25 deletions(-)
 create mode 100644 
tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast-cpus.json
 create mode 100644 
tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast-hotplug.json
 create mode 100644 
tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast.data

diff --git 
a/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast-cpus.json 
b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast-cpus.json
new file mode 100644
index 000..88fd2b8
--- /dev/null
+++ b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast-cpus.json
@@ -0,0 +1,71 @@
+{
+  "return": [
+{
+  "arch": "x86",
+  "cpu-index": 0,
+  "qom-path": "/machine/unattached/device[0]",
+  "thread-id": 895040
+},
+{
+  "arch": "x86",
+  "cpu-index": 1,
+  "qom-path": "/machine/peripheral/vcpu1",
+  "thread-id": 895056
+},
+{
+  "arch": "x86",
+  "cpu-index": 2,
+  "qom-path": "/machine/peripheral/vcpu2",
+  "thread-id": 895057
+},
+{
+  "arch": "x86",
+  "cpu-index": 3,
+  "qom-path": "/machine/peripheral/vcpu3",
+  "thread-id": 895058
+},
+{
+  "arch": "x86",
+  "cpu-index": 4,
+  "qom-path": "/machine/peripheral/vcpu4",
+  "thread-id": 895059
+},
+{
+  "arch": "x86",
+  "cpu-index": 5,
+  "qom-path": "/machine/peripheral/vcpu5",
+  "thread-id": 895060
+},
+{
+  "arch": "x86",
+  "cpu-index": 6,
+  "qom-path": "/machine/peripheral/vcpu6",
+  "thread-id": 895061
+},
+{
+  "arch": "x86",
+  "cpu-index": 7,
+  "qom-path": "/machine/peripheral/vcpu7",
+  "thread-id": 895062
+},
+{
+  "arch": "x86",
+  "cpu-index": 8,
+  "qom-path": "/machine/peripheral/vcpu8",
+  "thread-id": 895063
+},
+{
+  "arch": "x86",
+  "cpu-index": 9,
+  "qom-path": "/machine/peripheral/vcpu9",
+  "thread-id": 895064
+},
+{
+  "arch": "x86",
+  "cpu-index": 10,
+  "qom-path": "/machine/peripheral/vcpu10",
+  "thread-id": 895065
+}
+  ],
+  "id": "libvirt-52"
+}
diff --git 
a/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast-hotplug.json 
b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast-hotplug.json
new file mode 100644
index 000..aff5aa3
--- /dev/null
+++ 
b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast-hotplug.json
@@ -0,0 +1,115 @@
+{
+  "return": [
+{
+  "props": {
+"core-id": 0,
+"thread-id": 0,
+"socket-id": 10
+  },
+  "vcpus-count": 1,
+  "qom-path": "/machine/peripheral/vcpu10",
+  "type": "Broadwell-x86_64-cpu"
+},
+{
+  "props": {
+"core-id": 0,
+"thread-id": 0,
+"socket-id": 9
+  },
+  "vcpus-count": 1,
+  "qom-path": "/machine/peripheral/vcpu9",
+  "type": "Broadwell-x86_64-cpu"
+},
+{
+  "props": {
+"core-id": 0,
+"thread-id": 0,
+"socket-id": 8
+  },
+  "vcpus-count": 1,
+  "qom-path": "/machine/peripheral/vcpu8",
+  "type": "Broadwell-x86_64-cpu"
+},
+{
+  "props": {
+"core-id": 0,
+"thread-id": 0,
+"socket-id": 7
+  },
+  "vcpus-count": 1,
+  "qom-path": "/machine/peripheral/vcpu7",
+  "type": "Broadwell-x86_64-cpu"
+},
+{
+  "props": {
+"core-id": 0,
+"thread-id": 0,
+"socket-id": 6
+  },
+  "vcpus-count": 1,
+  "qom-path": "/machine/peripheral/vcpu6",
+  "type": "Broadwell-x86_64-cpu"
+},
+{
+  "props": {
+"core-id": 0,
+"thread-id": 0,
+"socket-id": 5
+  },
+  "vcpus-count": 1,
+  "qom-path": "/machine/peripheral/vcpu5",
+  "type": "Broadwell-x86_64-cpu"
+},
+{
+  "props": {
+"core-id": 0,
+"thread-id": 0,
+"socket-id": 4
+  },
+  "vcpus-count": 1,
+  "qom-path": "/machine/peripheral/vcpu4",
+  "type": "Broadwell-x86_64-cpu"
+},
+{
+  "props": {
+"core-id": 0,
+"thread-id": 0,
+"socket-id": 3
+  },
+  "vcpus-count": 1,
+  "qom-path":

[libvirt] [PATCH 0/6] Use query-cpus-fast instead of query-cpus

2018-03-02 Thread Viktor Mihajlovski

The QEMU monitor commmand query-cpus is deprecated starting
with QEMU 2.12.0 because it can adversely affect the performance of
a running virtual machine.

This series enables libvirt to use the new query-cpus-fast interface
if supported by the local QEMU instance and is required in order
to support QEMU once the interface has been removed.

query-cpus-fast doesn't return the halted state for a virtual CPU,
meaning that the vcpu..halted value won't be reported with
'virsh domstats' anymore. This is OK, as stats values are not
guaranteed to be reported under all circumstances and on all
architectures.

Upstream discussion consensus was that the halted state was
problematic anyway, as it had different semantics per architecture.
The only known exploitation happened for s390, for this architecture
the halted state will be computed based on an architecture-specific
cpu value returned in query-cpus-fast.

Viktor Mihajlovski (6):
  qemu: add capability detection for query-cpus-fast
  qemu: use query-cpus-fast in JSON monitor
  tests: add qemumonitorjson tests for query-cpus-fast
  qemu: add architecture-specific CPU info handling
  tests: add testcase for s390 query-cpus-fast
  qemu: refresh vcpu halted state only via query-cpus-fast

 src/qemu/qemu_capabilities.c   |   4 +-
 src/qemu/qemu_capabilities.h   |   1 +
 src/qemu/qemu_domain.c |   9 +-
 src/qemu/qemu_monitor.c|  14 +-
 src/qemu/qemu_monitor_json.c   | 149 +++--
 src/qemu/qemu_monitor_json.h   |   3 +-
 .../qemumonitorjson-cpuinfo-ppc64-hotplug-4.data   |   8 ++
 .../qemumonitorjson-cpuinfo-s390-fast-cpus.json|  21 +++
 .../qemumonitorjson-cpuinfo-s390-fast-hotplug.json |  21 +++
 .../qemumonitorjson-cpuinfo-s390-fast.data |  19 +++
 ...emumonitorjson-cpuinfo-x86-basic-pluggable.data |   5 +
 ...qemumonitorjson-cpuinfo-x86-full-fast-cpus.json |  71 ++
 ...umonitorjson-cpuinfo-x86-full-fast-hotplug.json | 115 
 .../qemumonitorjson-cpuinfo-x86-full-fast.data | 109 +++
 .../qemumonitorjson-cpuinfo-x86-node-full.data |   2 +
 tests/qemumonitorjsontest.c| 121 +
 tests/qemumonitortestutils.c   |   6 +
 tests/qemumonitortestutils.h   |   1 +
 18 files changed, 637 insertions(+), 42 deletions(-)
 create mode 100644 
tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-s390-fast-cpus.json
 create mode 100644 
tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-s390-fast-hotplug.json
 create mode 100644 
tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-s390-fast.data
 create mode 100644 
tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast-cpus.json
 create mode 100644 
tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast-hotplug.json
 create mode 100644 
tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-full-fast.data

-- 
1.9.1

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

[libvirt] [PATCH 2/6] qemu: use query-cpus-fast in JSON monitor

2018-03-02 Thread Viktor Mihajlovski

Use query-cpus-fast instead of query-cpus if supported by QEMU.
Based on the QEMU_CAPS_QUERY_CPUS_FAST capability.

Signed-off-by: Viktor Mihajlovski 
Reviewed-by: Boris Fiuczynski 
---
 src/qemu/qemu_monitor.c  | 10 --
 src/qemu/qemu_monitor_json.c | 26 +-
 src/qemu/qemu_monitor_json.h |  3 ++-
 tests/qemumonitorjsontest.c  |  2 +-
 4 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index ad5c572..dad1383 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2012,6 +2012,7 @@ qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
 int ret = -1;
 int rc;
 qemuMonitorCPUInfoPtr info = NULL;
+bool fast;
 
 QEMU_CHECK_MONITOR(mon);
 
@@ -2028,8 +2029,12 @@ qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
 (qemuMonitorJSONGetHotpluggableCPUs(mon, , )) 
< 0)
 goto cleanup;
 
+fast = virQEMUCapsGet(QEMU_DOMAIN_PRIVATE(mon->vm)->qemuCaps,
+  QEMU_CAPS_QUERY_CPUS_FAST);
+
 if (mon->json)
-rc = qemuMonitorJSONQueryCPUs(mon, , , hotplug);
+rc = qemuMonitorJSONQueryCPUs(mon, , , hotplug,
+  fast);
 else
 rc = qemuMonitorTextQueryCPUs(mon, , );
 
@@ -2082,7 +2087,8 @@ qemuMonitorGetCpuHalted(qemuMonitorPtr mon,
 QEMU_CHECK_MONITOR_NULL(mon);
 
 if (mon->json)
-rc = qemuMonitorJSONQueryCPUs(mon, , , false);
+rc = qemuMonitorJSONQueryCPUs(mon, , , false,
+  false);
 else
 rc = qemuMonitorTextQueryCPUs(mon, , );
 
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index a09e93e..2ecdf0a 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -1466,7 +1466,8 @@ int qemuMonitorJSONSystemReset(qemuMonitorPtr mon)
 static int
 qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data,
   struct qemuMonitorQueryCpusEntry **entries,
-  size_t *nentries)
+  size_t *nentries,
+  bool fast)
 {
 struct qemuMonitorQueryCpusEntry *cpus = NULL;
 int ret = -1;
@@ -1491,11 +1492,13 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data,
 }
 
 /* Some older qemu versions don't report the thread_id so treat this as
- * non-fatal, simply returning no data */
-ignore_value(virJSONValueObjectGetNumberInt(entry, "CPU", ));
-ignore_value(virJSONValueObjectGetNumberInt(entry, "thread_id", 
));
+ * non-fatal, simply returning no data.
+ * The return data of query-cpus-fast has different field names
+ */
+ignore_value(virJSONValueObjectGetNumberInt(entry, fast ? "cpu-index" 
: "CPU", ));
+ignore_value(virJSONValueObjectGetNumberInt(entry, fast ? "thread-id" 
: "thread_id", ));
 ignore_value(virJSONValueObjectGetBoolean(entry, "halted", ));
-qom_path = virJSONValueObjectGetString(entry, "qom_path");
+qom_path = virJSONValueObjectGetString(entry, fast ? "qom-path" : 
"qom_path");
 
 cpus[i].qemu_id = cpuid;
 cpus[i].tid = thread;
@@ -1520,10 +1523,12 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data,
  * @mon: monitor object
  * @entries: filled with detected entries on success
  * @nentries: number of entries returned
+ * @force: force exit on error
+ * @fast: use query-cpus-fast
  *
  * Queries qemu for cpu-related information. Failure to execute the command or
  * extract results does not produce an error as libvirt can continue without
- * this information.
+ * this information, unless the caller has specified @force == true.
  *
  * Returns 0 on success, -1 on a fatal error (oom ...) and -2 if the
  * query failed gracefully.
@@ -1532,10 +1537,13 @@ int
 qemuMonitorJSONQueryCPUs(qemuMonitorPtr mon,
  struct qemuMonitorQueryCpusEntry **entries,
  size_t *nentries,
- bool force)
+ bool force,
+ bool fast)
 {
 int ret = -1;
-virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-cpus", NULL);
+virJSONValuePtr cmd =
+qemuMonitorJSONMakeCommand(fast ? "query-cpus-fast" : "query-cpus",
+   NULL);
 virJSONValuePtr reply = NULL;
 virJSONValuePtr data;
 
@@ -1553,7 +1561,7 @@ qemuMonitorJSONQueryCPUs(qemuMonitorPtr mon,
 goto cleanup;
 }
 
-ret = qemuMonitorJSONExtractCPUInfo(data, entries, nentries);
+ret = qemuMonitorJSONExtractCPUInfo(data, entries, nentries, fast);
 
  cleanup:
 virJSONValueFree(cmd);
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index ec243be..e03299a 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -60,7 +60,8 @@ int

[libvirt] [PATCH 6/6] qemu: refresh vcpu halted state only via query-cpus-fast

2018-03-02 Thread Viktor Mihajlovski

In order to not affect running VMs, refreshing the halted state
is only performed if QEMU supports the query-cpus-fast QAPI.

Signed-off-by: Viktor Mihajlovski 
Reviewed-by: Boris Fiuczynski 
Reviewed-by: Marc Hartmayer 
---
 src/qemu/qemu_domain.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 8b4efc8..2eda5c8 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8796,8 +8796,13 @@ qemuDomainRefreshVcpuHalted(virQEMUDriverPtr driver,
 return 0;
 
 /* The halted state is interresting only on s390(x). On other platforms
- * the data would be stale at the time when it would be used. */
-if (!ARCH_IS_S390(vm->def->os.arch))
+ * the data would be stale at the time when it would be used.
+ * Calling qemuMonitorGetCpuHalted() can adversely affect the running
+ * VM's performance unless QEMU supports query-cpus-fast.
+ */
+if (!ARCH_IS_S390(vm->def->os.arch) ||
+!virQEMUCapsGet(QEMU_DOMAIN_PRIVATE(vm)->qemuCaps,
+QEMU_CAPS_QUERY_CPUS_FAST))
 return 0;
 
 if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
-- 
1.9.1

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

[libvirt] [PATCH 5/6] tests: add testcase for s390 query-cpus-fast

2018-03-02 Thread Viktor Mihajlovski

The s390 testcase verifies that the s390-specific cpu-state
field is correctly mapped to the halted property.

Since a few of the x86 and ppc testcases return "halted": "true"
it was necessary to update the respective .data files.

Signed-off-by: Viktor Mihajlovski 
Reviewed-by: Boris Fiuczynski 
---
 .../qemumonitorjson-cpuinfo-ppc64-hotplug-4.data|  8 
 .../qemumonitorjson-cpuinfo-s390-fast-cpus.json | 21 +
 .../qemumonitorjson-cpuinfo-s390-fast-hotplug.json  | 21 +
 .../qemumonitorjson-cpuinfo-s390-fast.data  | 19 +++
 ...qemumonitorjson-cpuinfo-x86-basic-pluggable.data |  5 +
 .../qemumonitorjson-cpuinfo-x86-node-full.data  |  2 ++
 tests/qemumonitorjsontest.c |  5 +
 7 files changed, 81 insertions(+)
 create mode 100644 
tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-s390-fast-cpus.json
 create mode 100644 
tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-s390-fast-hotplug.json
 create mode 100644 
tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-s390-fast.data

diff --git 
a/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-ppc64-hotplug-4.data 
b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-ppc64-hotplug-4.data
index 7c90889..5f6b865 100644
--- a/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-ppc64-hotplug-4.data
+++ b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-ppc64-hotplug-4.data
@@ -52,41 +52,49 @@
 alias='vcpu0'
 qom_path='/machine/peripheral/vcpu0'
 topology: core='8' vcpus='8'
+halted
 [vcpu libvirt-id='9']
 online=yes
 hotpluggable=yes
 thread-id='23171'
 query-cpus-id='17'
+halted
 [vcpu libvirt-id='10']
 online=yes
 hotpluggable=yes
 thread-id='23172'
 query-cpus-id='18'
+halted
 [vcpu libvirt-id='11']
 online=yes
 hotpluggable=yes
 thread-id='23173'
 query-cpus-id='19'
+halted
 [vcpu libvirt-id='12']
 online=yes
 hotpluggable=yes
 thread-id='23174'
 query-cpus-id='20'
+halted
 [vcpu libvirt-id='13']
 online=yes
 hotpluggable=yes
 thread-id='23175'
 query-cpus-id='21'
+halted
 [vcpu libvirt-id='14']
 online=yes
 hotpluggable=yes
 thread-id='23176'
 query-cpus-id='22'
+halted
 [vcpu libvirt-id='15']
 online=yes
 hotpluggable=yes
 thread-id='23177'
 query-cpus-id='23'
+halted
 [vcpu libvirt-id='16']
 online=yes
 hotpluggable=yes
diff --git 
a/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-s390-fast-cpus.json 
b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-s390-fast-cpus.json
new file mode 100644
index 000..4082e0e
--- /dev/null
+++ b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-s390-fast-cpus.json
@@ -0,0 +1,21 @@
+{
+  "return": [
+{
+  "arch": "s390",
+  "cpu-index": 0,
+  "props": {"core-id": 0},
+  "qom-path": "/machine/unattached/device[0]",
+  "thread-id": 89504,
+  "cpu-state": "operating"
+},
+{
+  "arch": "s390",
+  "cpu-index": 1,
+  "props": {"core-id": 1},
+  "qom-path": "/machine/unattached/device[1]",
+  "thread-id": 89505,
+  "cpu-state": "stopped"
+}
+  ],
+  "id": "libvirt-42"
+}
diff --git 
a/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-s390-fast-hotplug.json 
b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-s390-fast-hotplug.json
new file mode 100644
index 000..8016b5b
--- /dev/null
+++ b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-s390-fast-hotplug.json
@@ -0,0 +1,21 @@
+{
+  "return": [
+{
+  "props": {
+"core-id": 1
+  },
+  "vcpus-count": 1,
+  "qom-path": "/machine/unattached/device[1]",
+  "type": "host-s390x-cpu"
+},
+{
+  "props": {
+"core-id": 0
+  },
+  "vcpus-count": 1,
+  "qom-path": "/machine/unattached/device[0]",
+  "type": "host-s390x-cpu"
+}
+  ],
+  "id": "libvirt-41"
+}
diff --git a/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-s390-fast.data 
b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-s390-fast.data
new file mode 100644
index 000..9fc7041
--- /dev/null
+++ b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-s390-fast.data
@@ -0,0 +1,19 @@
+[vcpu libvirt-id='0']
+online=yes
+hotpluggable=no
+thread-id='89504'
+enable-id='1'
+query-cpus-id='0'
+type='host-s390x-cpu'
+qom_path='/machine/unattached/device[0]'
+topology: core='0' vcpus='1'
+[vcpu libvirt-id='1']
+online=yes
+hotpluggable=no
+thread-id='89505'
+enable-id='2'
+query-cpus-id='1'
+type='host-s390x-cpu'
+qom_path='/machine/unattached/device[1]'
+topology: core='1' vcpus='1'
+halted
diff --git 
a/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-basic-pluggable.data 
b/tests/qemumonitorjsondata/qemumonitorjson-cpuinfo-x86-basic-pluggable.data
index 93cefb9..9a1788d 100644
---

[libvirt] [PATCH 4/6] qemu: add architecture-specific CPU info handling

2018-03-02 Thread Viktor Mihajlovski

Extract architecture specific data from query-cpus[-fast] if
available. A new function qemuMonitorJSONExtractCPUArchInfo()
uses a call-back table to find and call architecture-specific
extraction handlers.

Initially, there's a handler for s390 cpu info to
set the halted property depending on the s390 cpu state
returned by QEMU. With this it's still possible to report
the halted condition even when using query-cpus-fast.

Signed-off-by: Viktor Mihajlovski 
Reviewed-by: Boris Fiuczynski 
---
 src/qemu/qemu_monitor.c  |   6 ++-
 src/qemu/qemu_monitor_json.c | 123 ++-
 2 files changed, 126 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index dad1383..5a0e8a7 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2083,12 +2083,16 @@ qemuMonitorGetCpuHalted(qemuMonitorPtr mon,
 size_t i;
 int rc;
 virBitmapPtr ret = NULL;
+bool fast;
 
 QEMU_CHECK_MONITOR_NULL(mon);
 
+fast = virQEMUCapsGet(QEMU_DOMAIN_PRIVATE(mon->vm)->qemuCaps,
+  QEMU_CAPS_QUERY_CPUS_FAST);
+
 if (mon->json)
 rc = qemuMonitorJSONQueryCPUs(mon, , , false,
-  false);
+  fast);
 else
 rc = qemuMonitorTextQueryCPUs(mon, , );
 
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 2ecdf0a..a408cfd 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -1451,15 +1451,128 @@ int qemuMonitorJSONSystemReset(qemuMonitorPtr mon)
 }
 
 
-/*
+typedef struct {
+const char *state;
+bool halted;
+} s390CpuStateMap;
+
+/**
+ * qemuMonitorJSONExtractCPUS390Info:
+ * @jsoncpu: pointer to a single JSON cpu entry
+ * @cpu: pointer to a single cpu entry
+ *
+ * Derive the legacy cpu info 'halted' information
+ * from the more accurate s390 cpu state. @cpu is
+ * modified only on success.
+ *
+ * Note: the 'uninitialized' s390 cpu state can't be
+ *   mapped to halted yes/no.
+ *
+ * A s390 cpu entry could look like this
+ *  { "arch": "s390",
+ *"cpu-index": 0,
+ *"qom-path": "/machine/unattached/device[0]",
+ *"thread_id": 3081,
+ *"cpu-state": "operating" }
+ *
+ * Returns 0 on success, and -2 if no cpu-state field was
+ * found or the state value is unknown.
+ */
+static int
+qemuMonitorJSONExtractCPUS390Info(virJSONValuePtr jsoncpu,
+  struct qemuMonitorQueryCpusEntry *cpu)
+{
+const char *cpu_state;
+s390CpuStateMap states[] = {
+{ "operating", false},
+{ "load", false},
+{ "stopped", true},
+{ "check-stop", true},
+};
+size_t i;
+
+if ((cpu_state = virJSONValueObjectGetString(jsoncpu, "cpu-state"))) {
+for (i = 0; i < ARRAY_CARDINALITY(states); i++) {
+if (STREQ(states[i].state, cpu_state)) {
+cpu->halted = states[i].halted;
+return 0;
+}
+}
+}
+
+return -2;
+}
+
+typedef struct {
+const char *arch;
+int (*extract)(virJSONValuePtr jsoncpu,
+   struct qemuMonitorQueryCpusEntry *cpu);
+} qemuCpuArchInfoHandler;
+
+
+/**
+ * qemuMonitorJSONExtractCPUArchInfo:
+ * @arch: virtual CPU's architecture
+ * @jsoncpu: pointer to a single JSON cpu entry
+ * @cpu: pointer to a single cpu entry
+ *
+ * Extracts architecure specific virtual CPU data for a single
+ * CPU from the QAPI response using an architecture specific
+ * callback.
  *
+ * To add a support for a new architecture, extend the array
+ * 'handlers' with a line like
+ * ...
+ *   { "newarch", qemuMonitorJSONExtractCPUNewarch },
+ * ...
+ * and implement the extraction callback.
+ * Check the QEMU QAPI schema for valid architecture names.
+ *
+ * Returns the called handler's return value or 0, if no handler
+ * was called.
+ */
+static int
+qemuMonitorJSONExtractCPUArchInfo(const char *arch, virJSONValuePtr jsoncpu,
+  struct qemuMonitorQueryCpusEntry *cpu)
+{
+qemuCpuArchInfoHandler handlers[] = {
+{ "s390", qemuMonitorJSONExtractCPUS390Info },
+};
+size_t i;
+
+for (i = 0; i < ARRAY_CARDINALITY(handlers); i++) {
+if (STREQ(handlers[i].arch, arch))
+return handlers[i].extract(jsoncpu, cpu);
+}
+
+return 0;
+}
+
+/**
+ * qemuMonitorJSONExtractCPUArchInfo:
+ * @data: JSON response data
+ * @entries: filled with detected cpu entries on success
+ * @nentries: number of entries returned
+ * @fast: true if this is a response from query-cpus-fast
+ *
+ * The JSON response @data will have the following format
+ * in case @fast == false
  * [{ "arch": "x86",
  *"current": true,
  *"CPU": 0,
  *"qom_path": "/machine/unattached/device[0]",
  *"pc": -2130415978,
  *"halted": true,
- *"thread_id": 2631237},
+ *"thread_id": 2631237,
+ *...},

[libvirt] [PATCH 1/6] qemu: add capability detection for query-cpus-fast

2018-03-02 Thread Viktor Mihajlovski

Detect whether QEMU supports the QMP query-cpus-fast API
and set QEMU_CAPS_QUERY_CPUS_FAST in this case.

Signed-off-by: Viktor Mihajlovski 
Reviewed-by: Boris Fiuczynski 
Reviewed-by: Marc Hartmayer 
---
 src/qemu/qemu_capabilities.c | 4 +++-
 src/qemu/qemu_capabilities.h | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index b5eb8cf..6635f5e 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -459,6 +459,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
   "pl011",
   "machine.pseries.max-cpu-compat",
   "dump-completed",
+  "query-cpus-fast",
 );
 
 
@@ -1579,7 +1580,8 @@ struct virQEMUCapsStringFlags virQEMUCapsCommands[] = {
 { "query-qmp-schema", QEMU_CAPS_QUERY_QMP_SCHEMA },
 { "query-cpu-model-expansion", QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION},
 { "query-cpu-definitions", QEMU_CAPS_QUERY_CPU_DEFINITIONS},
-{ "query-named-block-nodes", QEMU_CAPS_QUERY_NAMED_BLOCK_NODES}
+{ "query-named-block-nodes", QEMU_CAPS_QUERY_NAMED_BLOCK_NODES},
+{ "query-cpus-fast", QEMU_CAPS_QUERY_CPUS_FAST}
 };
 
 struct virQEMUCapsStringFlags virQEMUCapsMigration[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index c2ec2be..e3c31ab 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -444,6 +444,7 @@ typedef enum {
 QEMU_CAPS_DEVICE_PL011, /* -device pl011 (not user-instantiable) */
 QEMU_CAPS_MACHINE_PSERIES_MAX_CPU_COMPAT, /* -machine 
pseries,max-cpu-compat= */
 QEMU_CAPS_DUMP_COMPLETED, /* DUMP_COMPLETED event */
+QEMU_CAPS_QUERY_CPUS_FAST, /* query-cpus-fast command */
 
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
-- 
1.9.1

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

80 matches

Mail list logo