Re: [libvirt] [RFC PATCH 3/3] news: Update news for vfio-ap support

2018-10-08 Thread Boris Fiuczynski

On 10/9/18 7:10 AM, Bjoern Walk wrote:

+  The qemu driver now has support to passthrough adjunct processors
+  into qemu guests on S390.

s/qemu/QEMU


Ok, changed.

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

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

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


Re: [libvirt] [RFC PATCH 3/3] news: Update news for vfio-ap support

2018-10-08 Thread Bjoern Walk
Boris Fiuczynski  [2018-10-08, 06:25PM +0200]:
> Signed-off-by: Boris Fiuczynski 
> ---
>  docs/news.xml | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/docs/news.xml b/docs/news.xml
> index dc08c96352..b47cb979f2 100644
> --- a/docs/news.xml
> +++ b/docs/news.xml
> @@ -35,6 +35,15 @@
>  
>
>  
> +  
> +
> +  qemu: Add vfio AP support
> +
> +
> +  The qemu driver now has support to passthrough adjunct processors
> +  into qemu guests on S390.

s/qemu/QEMU

> +
> +  
>  
>  
>  
> -- 
> 2.17.0
> 


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

Re: [libvirt] [RFC PATCH 2/3] qemu: vfio-ap device support

2018-10-08 Thread Bjoern Walk
Boris Fiuczynski  [2018-10-08, 06:25PM +0200]:
> Adjusting domain format documentation, adding device address
> support and adding command line generation for vfio-ap.
> 
> Signed-off-by: Boris Fiuczynski 
> ---
>  docs/formatdomain.html.in  | 3 ++-
>  docs/schemas/domaincommon.rng  | 1 +
>  src/qemu/qemu_command.c| 8 
>  src/qemu/qemu_domain_address.c | 4 
>  src/util/virmdev.c | 3 ++-
>  src/util/virmdev.h | 1 +
>  6 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 8189959773..269741a690 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -4616,8 +4616,9 @@
>For mediated devices (Since 3.2.0)
>the model attribute specifies the device API which
>determines how the host's vfio driver will expose the device to the
> -  guest. Currently, model='vfio-pci' and
> +  guest. Currently, model='vfio-pci',
>model='vfio-ccw' (Since 
> 4.4.0)
> +  and model='vfio-ap' (Since 
> 4.9.0)
>is supported. MDEV section
>provides more information about mediated devices as well as how to
>create mediated devices on the host.

Maybe it is time to explain what the models actually are? Or is this not
in the scope of libvirt's documentation?

> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 099a949cf8..b9ac5df479 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -4618,6 +4618,7 @@
>
>  vfio-pci
>  vfio-ccw
> +vfio-ap
>
>  
>  
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index d77cf8c2d6..83569d70ab 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -5476,6 +5476,14 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
>  return -1;
>  }
>  break;
> +case VIR_MDEV_MODEL_TYPE_VFIO_AP:
> +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_AP)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("VFIO AP device assignment is not "
> + "supported by this version of QEMU"));
> +return -1;
> +}
> +break;
>  case VIR_MDEV_MODEL_TYPE_LAST:
>  default:
>  virReportEnumRangeError(virMediatedDeviceModelType,
> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index 8a8764cff5..24dd7c1a58 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -294,6 +294,10 @@ qemuDomainPrimeVfioDeviceAddresses(virDomainDefPtr def,
>  subsys->u.mdev.model == VIR_MDEV_MODEL_TYPE_VFIO_CCW &&
>  def->hostdevs[i]->info->type == 
> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
>  def->hostdevs[i]->info->type = type;
> +
> +if (virHostdevIsMdevDevice(def->hostdevs[i]) &&
> +subsys->u.mdev.model == VIR_MDEV_MODEL_TYPE_VFIO_AP)
> +def->hostdevs[i]->info->type = 
> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
>  }
>  }
>  
> diff --git a/src/util/virmdev.c b/src/util/virmdev.c
> index 10a2b08337..3e11e38802 100644
> --- a/src/util/virmdev.c
> +++ b/src/util/virmdev.c
> @@ -48,7 +48,8 @@ struct _virMediatedDeviceList {
>  
>  VIR_ENUM_IMPL(virMediatedDeviceModel, VIR_MDEV_MODEL_TYPE_LAST,
>"vfio-pci",
> -  "vfio-ccw")
> +  "vfio-ccw",
> +  "vfio-ap")
>  
>  static virClassPtr virMediatedDeviceListClass;
>  
> diff --git a/src/util/virmdev.h b/src/util/virmdev.h
> index 7c93c4d390..c856ff5bdb 100644
> --- a/src/util/virmdev.h
> +++ b/src/util/virmdev.h
> @@ -27,6 +27,7 @@
>  typedef enum {
>  VIR_MDEV_MODEL_TYPE_VFIO_PCI = 0,
>  VIR_MDEV_MODEL_TYPE_VFIO_CCW = 1,
> +VIR_MDEV_MODEL_TYPE_VFIO_AP  = 2,
>  
>  VIR_MDEV_MODEL_TYPE_LAST
>  } virMediatedDeviceModelType;
> -- 
> 2.17.0
> 

Looks good.

Reviewed-by: Bjoern Walk 


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

Re: [libvirt] [RFC PATCH 1/3] qemu: add vfio-ap capability

2018-10-08 Thread Bjoern Walk
Boris Fiuczynski  [2018-10-08, 06:25PM +0200]:
> Introduce vfio-ap capability.
> 
> Signed-off-by: Boris Fiuczynski 
> ---
>  src/qemu/qemu_capabilities.c | 2 ++
>  src/qemu/qemu_capabilities.h | 1 +
>  2 files changed, 3 insertions(+)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index e228f52ec0..2ca5af3297 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -508,6 +508,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>/* 315 */
>"vfio-pci.display",
>"blockdev",
> +  "vfio-ap",
>  );
>  
>  
> @@ -1092,6 +1093,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] 
> = {
>  { "vhost-vsock-device", QEMU_CAPS_DEVICE_VHOST_VSOCK },
>  { "mch", QEMU_CAPS_DEVICE_MCH },
>  { "sev-guest", QEMU_CAPS_SEV_GUEST },
> +{ "vfio-ap", QEMU_CAPS_DEVICE_VFIO_AP },
>  };
>  
>  static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsVirtioBalloon[] = 
> {
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index 934620ed31..6bb9a2c8f0 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -492,6 +492,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
> syntax-check */
>  /* 315 */
>  QEMU_CAPS_VFIO_PCI_DISPLAY, /* -device vfio-pci.display */
>  QEMU_CAPS_BLOCKDEV, /* -blockdev and blockdev-add are supported */
> +QEMU_CAPS_DEVICE_VFIO_AP, /* -device vfio-ap */
>  
>  QEMU_CAPS_LAST /* this must always be the last item */
>  } virQEMUCapsFlags;
> -- 
> 2.17.0
> 

Reviewed-by: Bjoern Walk 


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

[libvirt] [PATCH v2 1/1] Add attribute single_usage_restriction for mdev type-id

2018-10-08 Thread Kirti Wankhede
Generally a single instance of mdev device, a share of physical device, is
assigned to user space application or a VM. There are cases when multiple
instances of mdev devices of same or different types are required by user
space application or VM. For example in case of vGPU, multiple mdev devices
of type which represents whole GPU can be assigned to one instance of
application or VM.

All types of mdev devices may not support assigning multiple mdev devices
to a user space application. In that case vendor driver can fail open()
call of mdev device. But there is no way to know User space application to
about the configuration supported by vendor driver.

To expose supported configuration, vendor driver should add
'single_usage_restriction' attribute to type-id directory. Returning Y for
this attribute indicates vendor driver has restriction of single mdev
device of particular  assigned to one user space application.
Returning N indicates that multiple mdev devices of particular 
can be assigned to one user space application.

User space application should read if 'single_usage_restriction' attibute
is present in  directory of all mdev devices which are going to be
used. If all read N then user space application can proceed with multiple
mdev devices.

This is optional and readonly attribute.

Signed-off-by: Kirti Wankhede 
Reviewed-by: Neo Jia 
---
 Documentation/ABI/testing/sysfs-bus-vfio-mdev | 16 
 1 file changed, 16 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-vfio-mdev 
b/Documentation/ABI/testing/sysfs-bus-vfio-mdev
index 452dbe39270e..3aca352a70e5 100644
--- a/Documentation/ABI/testing/sysfs-bus-vfio-mdev
+++ b/Documentation/ABI/testing/sysfs-bus-vfio-mdev
@@ -85,6 +85,22 @@ Users:
a particular  that can help in understanding the
features provided by that type of mediated device.
 
+What:  /sys/.../mdev_supported_types//single_usage_restriction
+Date:   October 2018
+Contact:Kirti Wankhede 
+Description:
+   Reading this attribute will return Y or N. Returning Y indicates
+   vendor driver has restriction of single mdev device of this
+   particular  assigned to one user space application.
+   Returning N indicates that multiple mdev devices of particular
+can be assigned to one user space application.
+   This is optional and readonly attribute.
+Users:
+   User space application should read if 'single_usage_restriction'
+   attibute is present in  directory of all mdev devices
+   which are going to be used. If all read N then user space
+   application can proceed with multiple mdev devices.
+
 What:   /sys/...///
 Date:   October 2016
 Contact:Kirti Wankhede 
-- 
2.7.0

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


[libvirt] [RFC] virfile: fix cast-align error

2018-10-08 Thread Marc Hartmayer
Use the correct type in order to fix the following error on s390x:

In function 'virFileIsSharedFSType':
../../src/util/virfile.c:3578:38: error: cast increases required alignment of 
target type [-Werror=cast-align]
 virFileIsSharedFixFUSE(path, (long *) _type);

Signed-off-by: Marc Hartmayer 
---
 src/util/virfile.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/util/virfile.c b/src/util/virfile.c
index 2a7e87102a25..832d832696d5 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -3466,7 +3466,7 @@ int virFilePrintf(FILE *fp, const char *msg, ...)
 
 static int
 virFileIsSharedFixFUSE(const char *path,
-   long *f_type)
+   unsigned int *f_type)
 {
 char *dirpath = NULL;
 const char **mounts = NULL;
@@ -3575,7 +3575,7 @@ virFileIsSharedFSType(const char *path,
 
 if (sb.f_type == FUSE_SUPER_MAGIC) {
 VIR_DEBUG("Found FUSE mount for path=%s. Trying to fix it", path);
-virFileIsSharedFixFUSE(path, (long *) _type);
+virFileIsSharedFixFUSE(path, _type);
 }
 
 VIR_DEBUG("Check if path %s with FS magic %lld is shared",
-- 
2.17.0

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


Re: [libvirt] [PATCH] qemu: use "id" instead of deprecated "name" for -net

2018-10-08 Thread Laine Stump
On 10/08/2018 10:54 AM, Ján Tomko wrote:
> -net name= will be deprecated in QEMU 3.1:
> commit 101625a4d4ac7e96227a156bc5f6d21a9cc383cd
> net: Deprecate the "name" parameter of -net
> git describe: v3.0.0-791-g101625a4d4
>
> Use the id option instead, supported since QEMU 1.2:
> commit 6687b79d636cd60ed9adb1177d0d946b58fa7717
> convert net_client_init() to OptsVisitor
> git describe: v1.0-3564-g6687b79d63 contains: v1.2.0-rc0~142^2~8
>
> Thankfully, libvirt only uses -net for non-PCI, non-virtio NICs
> on ARM.
>
> Signed-off-by: Ján Tomko 
> ---
>  src/qemu/qemu_command.c  | 2 +-
>  tests/qemuxml2argvdata/arm-vexpressa9-basic.args | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index d77cf8c2d6..269276f2f9 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3516,7 +3516,7 @@ qemuBuildLegacyNicStr(virDomainNetDefPtr net)
>   net->info.alias,
>   (net->model ? ",model=" : ""),
>   (net->model ? net->model : ""),
> - (net->info.alias ? ",name=" : ""),
> + (net->info.alias ? ",id=" : ""),
>   (net->info.alias ? net->info.alias : "")));
>  return str;
>  }
> diff --git a/tests/qemuxml2argvdata/arm-vexpressa9-basic.args 
> b/tests/qemuxml2argvdata/arm-vexpressa9-basic.args
> index 90661d8b55..b925baa0e0 100644
> --- a/tests/qemuxml2argvdata/arm-vexpressa9-basic.args
> +++ b/tests/qemuxml2argvdata/arm-vexpressa9-basic.args
> @@ -27,6 +27,6 @@ server,nowait \
>  -usb \
>  -drive file=/arm.raw,format=raw,if=sd,index=0 \
>  -netdev user,id=hostnet0 \
> --net nic,macaddr=52:54:00:09:a4:37,netdev=hostnet0,model=lan9118,name=net0 \
> +-net nic,macaddr=52:54:00:09:a4:37,netdev=hostnet0,model=lan9118,id=net0 \
>  -chardev pty,id=charserial0 \
>  -serial chardev:charserial0


Reviewed-by: Laine Stump 


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

[libvirt] [RFC PATCH 0/3] qemu: guest dedicated crypto adapters

2018-10-08 Thread Boris Fiuczynski
This RFC patch series introduces initial libvirt support for guest
dedicated crypto adapters on S390.
It essentially allows to specify a vfio-ap mediated device in a domain.
Extensive documentation about AP is available in patch 6 of
the QEMU patch series.

KVM/kernel: guest dedicated crypto adapters
https://lkml.org/lkml/2018/9/26/25

QEMU: s390x: vfio-ap: guest dedicated crypto adapters
https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg03538.html

Boris Fiuczynski (3):
  qemu: add vfio-ap capability
  qemu: vfio-ap device support
  news: Update news for vfio-ap support

 docs/formatdomain.html.in  | 3 ++-
 docs/news.xml  | 9 +
 docs/schemas/domaincommon.rng  | 1 +
 src/qemu/qemu_capabilities.c   | 2 ++
 src/qemu/qemu_capabilities.h   | 1 +
 src/qemu/qemu_command.c| 8 
 src/qemu/qemu_domain_address.c | 4 
 src/util/virmdev.c | 3 ++-
 src/util/virmdev.h | 1 +
 9 files changed, 30 insertions(+), 2 deletions(-)

-- 
2.17.0

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


[libvirt] [RFC PATCH 2/3] qemu: vfio-ap device support

2018-10-08 Thread Boris Fiuczynski
Adjusting domain format documentation, adding device address
support and adding command line generation for vfio-ap.

Signed-off-by: Boris Fiuczynski 
---
 docs/formatdomain.html.in  | 3 ++-
 docs/schemas/domaincommon.rng  | 1 +
 src/qemu/qemu_command.c| 8 
 src/qemu/qemu_domain_address.c | 4 
 src/util/virmdev.c | 3 ++-
 src/util/virmdev.h | 1 +
 6 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 8189959773..269741a690 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -4616,8 +4616,9 @@
   For mediated devices (Since 3.2.0)
   the model attribute specifies the device API which
   determines how the host's vfio driver will expose the device to the
-  guest. Currently, model='vfio-pci' and
+  guest. Currently, model='vfio-pci',
   model='vfio-ccw' (Since 
4.4.0)
+  and model='vfio-ap' (Since 
4.9.0)
   is supported. MDEV section
   provides more information about mediated devices as well as how to
   create mediated devices on the host.
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 099a949cf8..b9ac5df479 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -4618,6 +4618,7 @@
   
 vfio-pci
 vfio-ccw
+vfio-ap
   
 
 
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index d77cf8c2d6..83569d70ab 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5476,6 +5476,14 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
 return -1;
 }
 break;
+case VIR_MDEV_MODEL_TYPE_VFIO_AP:
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_AP)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("VFIO AP device assignment is not "
+ "supported by this version of QEMU"));
+return -1;
+}
+break;
 case VIR_MDEV_MODEL_TYPE_LAST:
 default:
 virReportEnumRangeError(virMediatedDeviceModelType,
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 8a8764cff5..24dd7c1a58 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -294,6 +294,10 @@ qemuDomainPrimeVfioDeviceAddresses(virDomainDefPtr def,
 subsys->u.mdev.model == VIR_MDEV_MODEL_TYPE_VFIO_CCW &&
 def->hostdevs[i]->info->type == 
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
 def->hostdevs[i]->info->type = type;
+
+if (virHostdevIsMdevDevice(def->hostdevs[i]) &&
+subsys->u.mdev.model == VIR_MDEV_MODEL_TYPE_VFIO_AP)
+def->hostdevs[i]->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
 }
 }
 
diff --git a/src/util/virmdev.c b/src/util/virmdev.c
index 10a2b08337..3e11e38802 100644
--- a/src/util/virmdev.c
+++ b/src/util/virmdev.c
@@ -48,7 +48,8 @@ struct _virMediatedDeviceList {
 
 VIR_ENUM_IMPL(virMediatedDeviceModel, VIR_MDEV_MODEL_TYPE_LAST,
   "vfio-pci",
-  "vfio-ccw")
+  "vfio-ccw",
+  "vfio-ap")
 
 static virClassPtr virMediatedDeviceListClass;
 
diff --git a/src/util/virmdev.h b/src/util/virmdev.h
index 7c93c4d390..c856ff5bdb 100644
--- a/src/util/virmdev.h
+++ b/src/util/virmdev.h
@@ -27,6 +27,7 @@
 typedef enum {
 VIR_MDEV_MODEL_TYPE_VFIO_PCI = 0,
 VIR_MDEV_MODEL_TYPE_VFIO_CCW = 1,
+VIR_MDEV_MODEL_TYPE_VFIO_AP  = 2,
 
 VIR_MDEV_MODEL_TYPE_LAST
 } virMediatedDeviceModelType;
-- 
2.17.0

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


[libvirt] [RFC PATCH 3/3] news: Update news for vfio-ap support

2018-10-08 Thread Boris Fiuczynski
Signed-off-by: Boris Fiuczynski 
---
 docs/news.xml | 9 +
 1 file changed, 9 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index dc08c96352..b47cb979f2 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -35,6 +35,15 @@
 
   
 
+  
+
+  qemu: Add vfio AP support
+
+
+  The qemu driver now has support to passthrough adjunct processors
+  into qemu guests on S390.
+
+  
 
 
 
-- 
2.17.0

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


[libvirt] [RFC PATCH 1/3] qemu: add vfio-ap capability

2018-10-08 Thread Boris Fiuczynski
Introduce vfio-ap capability.

Signed-off-by: Boris Fiuczynski 
---
 src/qemu/qemu_capabilities.c | 2 ++
 src/qemu/qemu_capabilities.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index e228f52ec0..2ca5af3297 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -508,6 +508,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
   /* 315 */
   "vfio-pci.display",
   "blockdev",
+  "vfio-ap",
 );
 
 
@@ -1092,6 +1093,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
 { "vhost-vsock-device", QEMU_CAPS_DEVICE_VHOST_VSOCK },
 { "mch", QEMU_CAPS_DEVICE_MCH },
 { "sev-guest", QEMU_CAPS_SEV_GUEST },
+{ "vfio-ap", QEMU_CAPS_DEVICE_VFIO_AP },
 };
 
 static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsVirtioBalloon[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 934620ed31..6bb9a2c8f0 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -492,6 +492,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
syntax-check */
 /* 315 */
 QEMU_CAPS_VFIO_PCI_DISPLAY, /* -device vfio-pci.display */
 QEMU_CAPS_BLOCKDEV, /* -blockdev and blockdev-add are supported */
+QEMU_CAPS_DEVICE_VFIO_AP, /* -device vfio-ap */
 
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
-- 
2.17.0

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


[libvirt] [PATCH] qemu: use "id" instead of deprecated "name" for -net

2018-10-08 Thread Ján Tomko
-net name= will be deprecated in QEMU 3.1:
commit 101625a4d4ac7e96227a156bc5f6d21a9cc383cd
net: Deprecate the "name" parameter of -net
git describe: v3.0.0-791-g101625a4d4

Use the id option instead, supported since QEMU 1.2:
commit 6687b79d636cd60ed9adb1177d0d946b58fa7717
convert net_client_init() to OptsVisitor
git describe: v1.0-3564-g6687b79d63 contains: v1.2.0-rc0~142^2~8

Thankfully, libvirt only uses -net for non-PCI, non-virtio NICs
on ARM.

Signed-off-by: Ján Tomko 
---
 src/qemu/qemu_command.c  | 2 +-
 tests/qemuxml2argvdata/arm-vexpressa9-basic.args | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index d77cf8c2d6..269276f2f9 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3516,7 +3516,7 @@ qemuBuildLegacyNicStr(virDomainNetDefPtr net)
  net->info.alias,
  (net->model ? ",model=" : ""),
  (net->model ? net->model : ""),
- (net->info.alias ? ",name=" : ""),
+ (net->info.alias ? ",id=" : ""),
  (net->info.alias ? net->info.alias : "")));
 return str;
 }
diff --git a/tests/qemuxml2argvdata/arm-vexpressa9-basic.args 
b/tests/qemuxml2argvdata/arm-vexpressa9-basic.args
index 90661d8b55..b925baa0e0 100644
--- a/tests/qemuxml2argvdata/arm-vexpressa9-basic.args
+++ b/tests/qemuxml2argvdata/arm-vexpressa9-basic.args
@@ -27,6 +27,6 @@ server,nowait \
 -usb \
 -drive file=/arm.raw,format=raw,if=sd,index=0 \
 -netdev user,id=hostnet0 \
--net nic,macaddr=52:54:00:09:a4:37,netdev=hostnet0,model=lan9118,name=net0 \
+-net nic,macaddr=52:54:00:09:a4:37,netdev=hostnet0,model=lan9118,id=net0 \
 -chardev pty,id=charserial0 \
 -serial chardev:charserial0
-- 
2.16.4

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

Re: [libvirt] [PATCH 08/11] qemu: Simplify QEMU binary search

2018-10-08 Thread Daniel P . Berrangé
On Mon, Oct 08, 2018 at 02:35:30PM +0200, Andrea Bolognani wrote:
> On Mon, 2018-10-08 at 13:09 +0100, Daniel P. Berrangé wrote:
> > This patch is doing two things. It is moving the code block earlier,
> > to let you drop the duplicated virQEMUCapsCacheLookup(). Second it is
> > removing the array iteration & just checking one single path instead.
> > 
> > I'd suggest we keep the array iteration, and just move the code.
> 
> The patch has already been merged, so you'd have to partially revert
> it to achieve what you suggest.
> 
> As explained elsewhere in the thread, the probability we would ever
> need more than one entry in the array is basically zero, so why have
> it? If it ever comes the time when we actually need a second entry,
> then sure, but now? Just in case? That's pretty much a textbook
> example of over-engineering IMHO.
> 
> Anyway, I feel like I've spent way too much time arguing over what
> is ultimately a very, very minor detail already, and at the end of
> the day I just don't care enough to spend more energy on it. If
> either you or Peter want to reintroduce the array, then by all means
> go ahead.

Oh, I missed that it was already merged. I don't care enough to change
it post-merge.

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 08/11] qemu: Simplify QEMU binary search

2018-10-08 Thread Andrea Bolognani
On Mon, 2018-10-08 at 13:09 +0100, Daniel P. Berrangé wrote:
> This patch is doing two things. It is moving the code block earlier,
> to let you drop the duplicated virQEMUCapsCacheLookup(). Second it is
> removing the array iteration & just checking one single path instead.
> 
> I'd suggest we keep the array iteration, and just move the code.

The patch has already been merged, so you'd have to partially revert
it to achieve what you suggest.

As explained elsewhere in the thread, the probability we would ever
need more than one entry in the array is basically zero, so why have
it? If it ever comes the time when we actually need a second entry,
then sure, but now? Just in case? That's pretty much a textbook
example of over-engineering IMHO.

Anyway, I feel like I've spent way too much time arguing over what
is ultimately a very, very minor detail already, and at the end of
the day I just don't care enough to spend more energy on it. If
either you or Peter want to reintroduce the array, then by all means
go ahead.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 08/11] qemu: Simplify QEMU binary search

2018-10-08 Thread Daniel P . Berrangé
On Thu, Sep 20, 2018 at 05:25:26PM +0200, Andrea Bolognani wrote:
> Now that we have reduced the number of sensible options down
> to either the native QEMU binary or RHEL's qemu-kvm, we can
> make virQEMUCapsInitGuest() a bit simpler.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_capabilities.c | 29 +++--
>  1 file changed, 7 insertions(+), 22 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index fd8badc60b..72fa19a2b7 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -746,7 +746,6 @@ virQEMUCapsInitGuest(virCapsPtr caps,
>   virArch hostarch,
>   virArch guestarch)
>  {
> -size_t i;
>  char *binary = NULL;
>  virQEMUCapsPtr qemubinCaps = NULL;
>  int ret = -1;
> @@ -756,6 +755,13 @@ virQEMUCapsInitGuest(virCapsPtr caps,
>   */
>  binary = virQEMUCapsFindBinaryForArch(hostarch, guestarch);
>  
> +/* RHEL doesn't follow the usual naming for QEMU binaries and ships
> + * a single binary named qemu-kvm outside of $PATH instead */
> +if (virQEMUCapsGuestIsNative(hostarch, guestarch) && !binary) {
> +if (VIR_STRDUP(binary, "/usr/libexec/qemu-kvm") < 0)
> +return -1;
> +}
> +
>  /* Ignore binary if extracting version info fails */
>  if (binary) {
>  if (!(qemubinCaps = virQEMUCapsCacheLookup(cache, binary))) {
> @@ -764,27 +770,6 @@ virQEMUCapsInitGuest(virCapsPtr caps,
>  }
>  }
>  
> -if (virQEMUCapsGuestIsNative(hostarch, guestarch) && !binary) {
> -const char *kvmbins[] = {
> -"/usr/libexec/qemu-kvm", /* RHEL */
> -};
> -
> -for (i = 0; i < ARRAY_CARDINALITY(kvmbins); ++i) {
> -binary = virFindFileInPath(kvmbins[i]);
> -
> -if (!binary)
> -continue;
> -
> -if (!(qemubinCaps = virQEMUCapsCacheLookup(cache, binary))) {
> -virResetLastError();
> -VIR_FREE(binary);
> -continue;
> -}
> -
> -break;
> -}
> -}

This patch is doing two things. It is moving the code block earlier,
to let you drop the duplicated virQEMUCapsCacheLookup(). Second it is
removing the array iteration & just checking one single path instead.

I'd suggest we keep the array iteration, and just move the code.

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 08/11] qemu: Simplify QEMU binary search

2018-10-08 Thread Andrea Bolognani
On Mon, 2018-10-08 at 13:10 +0200, Peter Krempa wrote:
> On Mon, Oct 08, 2018 at 13:04:17 +0200, Andrea Bolognani wrote:
> > On Mon, 2018-10-08 at 12:59 +0200, Peter Krempa wrote:
> > > Well, if we are going to bend the rules and make a hack for the mess
> > > some downstream distro made, then we should keep the original code which
> > > allows to add mess for other distros as well.
> > 
> > The downstream path names I dropped are no longer necessary because
> > Debian and friends are using the standard binary names in all
> > releases we support; the same is not true of RHEL, which is why that
> > one path has not been removed along with the rest.
> 
> That is perfectly fine with me. I'm arguing that we should keep the
> whole function that iterates the array of override paths even if it
> contains the exception only for RHEL.
> 
> Doing a tailored special case for RHEL seems like we are favoriting it
> somehow which we should not. So either the loop is deleted without
> replacement and RHEL maintainers fix their mess or we keep it as-is so
> that other distros can add their mess as well.

I don't expect distros that have gotten rid of their own incompatible
binary names (eg. all except RHEL) will reintroduce them, so the loop
would be needlessly verbose dead code right now and most likely going
forward.

If anyone will seriously accuse us of "favoring RHEL" because of how
that piece of code is written, we can just point at the git history
and show them that it was simply the most concise way to achieve its
goal given the known constraints.

Of course if at any point down the line I'm proven wrong about the
assumption described above and we end up needing to special-case
another binary name, then reintroducing the loop will be the correct
solution.

tl;dr I stand behind my changes and won't support reverting them
  unless a purely technical need motivates doing so.

-- 
Andrea Bolognani / Red Hat / Virtualization

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


[libvirt] [PATCH] libvirt: add daemon itself as shutdown reason

2018-10-08 Thread Nikolay Shirokovskiy
Let's introduce shutdown reason "daemon" which means we have to
kill running domain ourselves as the best action we can take at
that moment. Failure to pick up domain on daemon restart is
one example of such case. Using reason "crashed" is a bit misleading
as it is used when qemu is actually crashed or qemu destroyed on
guest panic as result of user choice that is the problem was
in qemu/guest itself. So I propose to use "crashed" only for
qemu side issues and introduce "daemon" when we have to kill the qemu
for good.

There is another example where "daemon" will be useful. If we can
not reboot domain we kill it and got "crashed" reason now. Looks
like good candidate for "daemon" reason.

Signed-off-by: Nikolay Shirokovskiy 
---
 include/libvirt/libvirt-domain.h |  1 +
 src/conf/domain_conf.c   |  3 ++-
 src/qemu/qemu_process.c  | 11 ---
 tools/virsh-domain-monitor.c |  3 ++-
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index fdd2d6b..11fdab5 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -145,6 +145,7 @@ typedef enum {
 VIR_DOMAIN_SHUTOFF_FAILED = 6,  /* domain failed to start */
 VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT = 7, /* restored from a snapshot which was
* taken while domain was shutoff */
+VIR_DOMAIN_SHUTOFF_DAEMON = 8,  /* daemon have to kill domain */
 # ifdef VIR_ENUM_SENTINELS
 VIR_DOMAIN_SHUTOFF_LAST
 # endif
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9911d56..e441723 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -779,7 +779,8 @@ VIR_ENUM_IMPL(virDomainShutoffReason, 
VIR_DOMAIN_SHUTOFF_LAST,
   "migrated",
   "saved",
   "failed",
-  "from snapshot")
+  "from snapshot",
+  "daemon")
 
 VIR_ENUM_IMPL(virDomainCrashedReason, VIR_DOMAIN_CRASHED_LAST,
   "unknown",
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index e9c7618..c4bc9ca 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -7988,15 +7988,12 @@ qemuProcessReconnect(void *opaque)
 /* We can't get the monitor back, so must kill the VM
  * to remove danger of it ending up running twice if
  * user tries to start it again later
- * If we couldn't get the monitor since QEMU supports
- * no-shutdown, we can safely say that the domain
- * crashed ... */
-state = VIR_DOMAIN_SHUTOFF_CRASHED;
-/* If BeginJob failed, we jumped here without a job, let's hope another
+ * If BeginJob failed, we jumped here without a job, let's hope another
  * thread didn't have a chance to start playing with the domain yet
  * (it's all we can do anyway).
  */
-qemuProcessStop(driver, obj, state, QEMU_ASYNC_JOB_NONE, stopFlags);
+qemuProcessStop(driver, obj, VIR_DOMAIN_SHUTOFF_DAEMON,
+QEMU_ASYNC_JOB_NONE, stopFlags);
 }
 goto cleanup;
 }
@@ -8035,7 +8032,7 @@ qemuProcessReconnectHelper(virDomainObjPtr obj,
  * is no thread that could be doing anything else with the same domain
  * object.
  */
-qemuProcessStop(src->driver, obj, VIR_DOMAIN_SHUTOFF_FAILED,
+qemuProcessStop(src->driver, obj, VIR_DOMAIN_SHUTOFF_DAEMON,
 QEMU_ASYNC_JOB_NONE, 0);
 qemuDomainRemoveInactiveJobLocked(src->driver, obj);
 
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index 3a26363..f0ad558 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -212,7 +212,8 @@ VIR_ENUM_IMPL(virshDomainShutoffReason,
   N_("migrated"),
   N_("saved"),
   N_("failed"),
-  N_("from snapshot"))
+  N_("from snapshot"),
+  N_("daemon"))
 
 VIR_ENUM_DECL(virshDomainCrashedReason)
 VIR_ENUM_IMPL(virshDomainCrashedReason,
-- 
1.8.3.1

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


Re: [libvirt] [PATCH v2 04/12] qemu: domain: Assume 'raw' default storage format also for network storage

2018-10-08 Thread Peter Krempa
On Mon, Oct 08, 2018 at 07:10:28 -0400, John Ferlan wrote:
> 
> 
> On 10/5/18 8:14 AM, Peter Krempa wrote:
> > Post parse callback adds the 'raw' type only for local files. Remote
> > files can also have backing store (even local) so we should do this also
> > for network backed storage.
> 
> NIT: The .xml/.args in this patch are not all remote storage - they are
> volume storage...
> 
> The disk-source-pool-mode.args gets the look/feel of remote from
> virDomainDiskTranslateISCSIDirect

Oh! actually you are right. For disk type "VOLUME" we should not add any
default format in the parser since the function that fetches the volume
from the storage driver should do that.

I'll investigate slightly further and change the condition to only
exclude "VOLUME" disks rather than try to include a variety of the types
which require this.


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 04/12] qemu: domain: Assume 'raw' default storage format also for network storage

2018-10-08 Thread John Ferlan



On 10/5/18 8:14 AM, Peter Krempa wrote:
> Post parse callback adds the 'raw' type only for local files. Remote
> files can also have backing store (even local) so we should do this also
> for network backed storage.

NIT: The .xml/.args in this patch are not all remote storage - they are
volume storage...

The disk-source-pool-mode.args gets the look/feel of remote from
virDomainDiskTranslateISCSIDirect

John

> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_domain.c | 4 +---
>  tests/qemuxml2argvdata/disk-source-pool-mode.args  | 6 +++---
>  tests/qemuxml2argvdata/disk-source-pool.args   | 4 ++--
>  tests/qemuxml2xmloutdata/disk-source-pool-mode.xml | 6 +++---
>  tests/qemuxml2xmloutdata/disk-source-pool.xml  | 2 +-
>  5 files changed, 10 insertions(+), 12 deletions(-)
> 

[...]

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


Re: [libvirt] [PATCH 08/11] qemu: Simplify QEMU binary search

2018-10-08 Thread Peter Krempa
On Mon, Oct 08, 2018 at 13:04:17 +0200, Andrea Bolognani wrote:
> On Mon, 2018-10-08 at 12:59 +0200, Peter Krempa wrote:
> > On Fri, Oct 05, 2018 at 17:18:44 +0200, Andrea Bolognani wrote:
> > > On Fri, 2018-10-05 at 16:43 +0200, Ján Tomko wrote:
> > > > On Thu, Sep 20, 2018 at 05:25:26PM +0200, Andrea Bolognani wrote:
> > > > > @@ -756,6 +755,13 @@ virQEMUCapsInitGuest(virCapsPtr caps,
> > > > >  */
> > > > > binary = virQEMUCapsFindBinaryForArch(hostarch, guestarch);
> > > > > 
> > > > > +/* RHEL doesn't follow the usual naming for QEMU binaries and 
> > > > > ships
> > > > > + * a single binary named qemu-kvm outside of $PATH instead */
> > > > 
> > > > This does not look like something upstream libvirt should worry about.
> > > 
> > > I would be simple enough to turn this into a downstream-only patch;
> > > however, by doing so we would lose the ability to run unchanged
> > > upstream libvirt on top of RHEL, so I think it's preferable to
> > > leave it alone.
> > 
> > Well, if we are going to bend the rules and make a hack for the mess
> > some downstream distro made, then we should keep the original code which
> > allows to add mess for other distros as well.
> 
> The downstream path names I dropped are no longer necessary because
> Debian and friends are using the standard binary names in all
> releases we support; the same is not true of RHEL, which is why that
> one path has not been removed along with the rest.

That is perfectly fine with me. I'm arguing that we should keep the
whole function that iterates the array of override paths even if it
contains the exception only for RHEL.

Doing a tailored special case for RHEL seems like we are favoriting it
somehow which we should not. So either the loop is deleted without
replacement and RHEL maintainers fix their mess or we keep it as-is so
that other distros can add their mess as well.


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

Re: [libvirt] [PATCH 08/11] qemu: Simplify QEMU binary search

2018-10-08 Thread Andrea Bolognani
On Mon, 2018-10-08 at 12:59 +0200, Peter Krempa wrote:
> On Fri, Oct 05, 2018 at 17:18:44 +0200, Andrea Bolognani wrote:
> > On Fri, 2018-10-05 at 16:43 +0200, Ján Tomko wrote:
> > > On Thu, Sep 20, 2018 at 05:25:26PM +0200, Andrea Bolognani wrote:
> > > > @@ -756,6 +755,13 @@ virQEMUCapsInitGuest(virCapsPtr caps,
> > > >  */
> > > > binary = virQEMUCapsFindBinaryForArch(hostarch, guestarch);
> > > > 
> > > > +/* RHEL doesn't follow the usual naming for QEMU binaries and ships
> > > > + * a single binary named qemu-kvm outside of $PATH instead */
> > > 
> > > This does not look like something upstream libvirt should worry about.
> > 
> > I would be simple enough to turn this into a downstream-only patch;
> > however, by doing so we would lose the ability to run unchanged
> > upstream libvirt on top of RHEL, so I think it's preferable to
> > leave it alone.
> 
> Well, if we are going to bend the rules and make a hack for the mess
> some downstream distro made, then we should keep the original code which
> allows to add mess for other distros as well.

The downstream path names I dropped are no longer necessary because
Debian and friends are using the standard binary names in all
releases we support; the same is not true of RHEL, which is why that
one path has not been removed along with the rest.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 08/11] qemu: Simplify QEMU binary search

2018-10-08 Thread Peter Krempa
On Fri, Oct 05, 2018 at 17:18:44 +0200, Andrea Bolognani wrote:
> On Fri, 2018-10-05 at 16:43 +0200, Ján Tomko wrote:
> > On Thu, Sep 20, 2018 at 05:25:26PM +0200, Andrea Bolognani wrote:
> > > @@ -756,6 +755,13 @@ virQEMUCapsInitGuest(virCapsPtr caps,
> > >  */
> > > binary = virQEMUCapsFindBinaryForArch(hostarch, guestarch);
> > > 
> > > +/* RHEL doesn't follow the usual naming for QEMU binaries and ships
> > > + * a single binary named qemu-kvm outside of $PATH instead */
> > 
> > This does not look like something upstream libvirt should worry about.
> 
> I would be simple enough to turn this into a downstream-only patch;
> however, by doing so we would lose the ability to run unchanged
> upstream libvirt on top of RHEL, so I think it's preferable to
> leave it alone.

Well, if we are going to bend the rules and make a hack for the mess
some downstream distro made, then we should keep the original code which
allows to add mess for other distros as well.


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 00/12] qemu: Fix media changing problems

2018-10-08 Thread Michal Privoznik
On 10/05/2018 02:13 PM, Peter Krempa wrote:
> Fixes regression in media changing/disk hotplug as the ordering of the
> alias allocation and disk preparation was bad.
> 
> v2:
> - be more explicit about old and new definitions used in certain steps
> - clean up legacy hotplug to not access old disk source definition
> -  also treat network disks as 'raw' if the format was not provided
> 
> Peter Krempa (12):
>   Revert "qemu: hotplug: Prepare disk source in
> qemuDomainAttachDeviceDiskLive"
>   Revert "qemu: hotplug: consolidate media change code paths"
>   qemu: hotplug: Don't pretend that we support secrets for media change
>   qemu: domain: Assume 'raw' default storage format also for network
> storage
>   qemu: hotplug: Remove code handling possible missing disk source
> format
>   qemu: hotplug: Allow specifying explicit source for disk backend
> hotplug code
>   qemu: hotplug: Be explicit about old/new sources when changing media
>   qemu: hotplug: Prepare disk source for media changing
>   qemu: hotplug: Add wrapper for disk hotplug code
>   qemu: conf: Export qemuAddSharedDisk
>   qemu: hotplug: Split out media change code from disk hotplug
>   qemu: hotplug: Refactor qemuDomainAttachDeviceDiskLiveInternal
> 
>  src/qemu/qemu_conf.c  |   2 +-
>  src/qemu/qemu_conf.h  |   5 +
>  src/qemu/qemu_domain.c|   4 +-
>  src/qemu/qemu_driver.c|   7 +-
>  src/qemu/qemu_hotplug.c   | 276 ++
>  src/qemu/qemu_hotplug.h   |   9 +-
>  tests/qemuhotplugtest.c   |   2 +-
>  .../disk-source-pool-mode.args|   6 +-
>  tests/qemuxml2argvdata/disk-source-pool.args  |   4 +-
>  .../disk-source-pool-mode.xml |   6 +-
>  tests/qemuxml2xmloutdata/disk-source-pool.xml |   2 +-
>  11 files changed, 185 insertions(+), 138 deletions(-)
> 


ACK series. I can also confirm that this passes my testing which caused
this in the first place.

Michal

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


Re: [libvirt] [osinfo-db-tools PATCH v2 0/2] RFC: Make osinfo-db-import aware of URLs

2018-10-08 Thread Fabiano Fidêncio
On Mon, 2018-10-08 at 10:53 +0200, Fabiano Fidêncio wrote:

> This is a simple draft of a work that has been discussed back in July
> as
> part of this thread[0].
> 
> There are a few things which are not clear to me whether we want (or
> whether we do *not* want).
> 
> For now I'm not using any progress callback to print the download
> status
> and I don't even know whether it would be desired to have. But this
> is
> something that could be easily done.
> 
> Also, for now the operation is synchronous and there's no cancellable
> used ... but that's something that could be changed (although I don't
> see the need for that right now).
> 
> In order to give it a try, please, just build our source with the
> patch
> included and try:
> ./tools/osinfo-db-import 
> https://releases.pagure.org/libosinfo/osinfo-db-20180920.tar.xz
> ./tools/osinfo-db-import https://foo.bar/foo.bar.tar.xz
> 
> [0]: 
> https://www.redhat.com/archives/libosinfo/2018-July/msg2.html
> 
> Changes since v1:
> - Use Gio instead of curl;
> - An additional patch removing a unused var;
> 
> Fabiano Fidêncio (2):
>   import: Learn how to deal with URLs
>   import: Remove unused variable
> 
>  tools/osinfo-db-import.c | 91 
> 
>  1 file changed, 84 insertions(+), 7 deletions(-)
> 

Oops, please, ignore those patches as they've been sent to wrong list,
sorry for the noise!


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

[libvirt] [osinfo-db-tools PATCH v2 2/2] import: Remove unused variable

2018-10-08 Thread Fabiano Fidêncio
Signed-off-by: Fabiano Fidêncio 
---
 tools/osinfo-db-import.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tools/osinfo-db-import.c b/tools/osinfo-db-import.c
index 289a85d..c0b4931 100644
--- a/tools/osinfo-db-import.c
+++ b/tools/osinfo-db-import.c
@@ -193,7 +193,6 @@ static int osinfo_db_import_extract(GFile *target,
 int ret = -1;
 int r;
 GFile *file = NULL;
-GError *err = NULL;
 gchar *source_file = NULL;
 const gchar *uri_schemes[] = {"ftp", "http", NULL};
 
@@ -266,8 +265,6 @@ static int osinfo_db_import_extract(GFile *target,
 ret = 0;
  cleanup:
 archive_read_free(arc);
-if (err)
-g_error_free(err);
 if (file)
 g_object_unref(file);
 g_free(source_file);
-- 
2.19.1

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

[libvirt] [osinfo-db-tools PATCH v2 1/2] import: Learn how to deal with URLs

2018-10-08 Thread Fabiano Fidêncio
Signed-off-by: Fabiano Fidêncio 
---
 tools/osinfo-db-import.c | 88 ++--
 1 file changed, 84 insertions(+), 4 deletions(-)

diff --git a/tools/osinfo-db-import.c b/tools/osinfo-db-import.c
index 0d0bdd9..289a85d 100644
--- a/tools/osinfo-db-import.c
+++ b/tools/osinfo-db-import.c
@@ -135,6 +135,55 @@ static GFile *osinfo_db_import_get_file(GFile *target,
 return g_file_resolve_relative_path(target, tmp);
 }
 
+static int
+osinfo_db_import_download_file(const gchar *url,
+   gchar **source_file)
+{
+GFile *in = NULL;
+GFile *out = NULL;
+GError *err = NULL;
+gchar *filename = NULL;
+GFileCopyFlags flags = G_FILE_COPY_OVERWRITE | G_FILE_COPY_BACKUP;
+int ret = -1;
+
+in = g_file_new_for_uri(url);
+if (in == NULL)
+return -1;
+
+filename = g_file_get_basename(in);
+if (filename == NULL)
+goto cleanup;
+
+*source_file = g_strdup_printf("%s/%s", g_get_user_cache_dir(), filename);
+if (*source_file == NULL)
+goto cleanup;
+
+out = g_file_new_for_path(*source_file);
+if (out == NULL)
+goto cleanup;
+
+if (!g_file_copy(in, out, flags, NULL, NULL, NULL, )) {
+g_printerr("Could not download the \"%s\" file: %s\n",
+   url, err->message);
+goto cleanup;
+}
+
+ret = 0;
+
+ cleanup:
+g_free(filename);
+if (in != NULL)
+g_object_unref(in);
+if (out != NULL)
+g_object_unref(out);
+if (err != NULL)
+g_error_free(err);
+if (ret != 0)
+unlink(*source_file);
+
+return ret;
+}
+
 static int osinfo_db_import_extract(GFile *target,
 const char *source,
 gboolean verbose)
@@ -145,6 +194,8 @@ static int osinfo_db_import_extract(GFile *target,
 int r;
 GFile *file = NULL;
 GError *err = NULL;
+gchar *source_file = NULL;
+const gchar *uri_schemes[] = {"ftp", "http", NULL};
 
 arc = archive_read_new();
 
@@ -154,9 +205,37 @@ static int osinfo_db_import_extract(GFile *target,
 if (source != NULL && g_str_equal(source, "-"))
 source = NULL;
 
-if ((r = archive_read_open_filename(arc, source, 10240)) != ARCHIVE_OK) {
+if (source != NULL) {
+gboolean download_file = FALSE;
+
+file = g_file_new_for_uri(source);
+if (file == NULL)
+goto cleanup;
+
+/* The supported uri schemes here are "ftp", "http" and "https".
+ * However, "https" is not set as part of the array as it matches with
+ * "http". */
+for (gint i = 0; uri_schemes[i] != NULL; i++) {
+if (g_file_has_uri_scheme(file, uri_schemes[i])) {
+download_file = TRUE;
+break;
+}
+}
+
+if (download_file) {
+if (osinfo_db_import_download_file(source, _file) < 0)
+goto cleanup;
+} else {
+source_file = g_strdup(source);
+}
+
+if (source_file == NULL)
+goto cleanup;
+}
+
+if ((r = archive_read_open_filename(arc, source_file, 10240)) != 
ARCHIVE_OK) {
 g_printerr("%s: cannot open archive %s: %s\n",
-   argv0, source, archive_error_string(arc));
+   argv0, source_file, archive_error_string(arc));
 goto cleanup;
 }
 
@@ -166,7 +245,7 @@ static int osinfo_db_import_extract(GFile *target,
 break;
 if (r != ARCHIVE_OK) {
 g_printerr("%s: cannot read next archive entry in %s: %s\n",
-   argv0, source, archive_error_string(arc));
+   argv0, source_file, archive_error_string(arc));
 goto cleanup;
 }
 
@@ -180,7 +259,7 @@ static int osinfo_db_import_extract(GFile *target,
 
 if (archive_read_close(arc) != ARCHIVE_OK) {
 g_printerr("%s: cannot finish reading archive %s: %s\n",
-   argv0, source, archive_error_string(arc));
+   argv0, source_file, archive_error_string(arc));
 goto cleanup;
 }
 
@@ -191,6 +270,7 @@ static int osinfo_db_import_extract(GFile *target,
 g_error_free(err);
 if (file)
 g_object_unref(file);
+g_free(source_file);
 return ret;
 }
 
-- 
2.19.1

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

[libvirt] [osinfo-db-tools PATCH v2 0/2] RFC: Make osinfo-db-import aware of URLs

2018-10-08 Thread Fabiano Fidêncio
This is a simple draft of a work that has been discussed back in July as
part of this thread[0].

There are a few things which are not clear to me whether we want (or
whether we do *not* want).

For now I'm not using any progress callback to print the download status
and I don't even know whether it would be desired to have. But this is
something that could be easily done.

Also, for now the operation is synchronous and there's no cancellable
used ... but that's something that could be changed (although I don't
see the need for that right now).

In order to give it a try, please, just build our source with the patch
included and try:
./tools/osinfo-db-import 
https://releases.pagure.org/libosinfo/osinfo-db-20180920.tar.xz
./tools/osinfo-db-import https://foo.bar/foo.bar.tar.xz

[0]: https://www.redhat.com/archives/libosinfo/2018-July/msg2.html

Changes since v1:
- Use Gio instead of curl;
- An additional patch removing a unused var;

Fabiano Fidêncio (2):
  import: Learn how to deal with URLs
  import: Remove unused variable

 tools/osinfo-db-import.c | 91 
 1 file changed, 84 insertions(+), 7 deletions(-)

-- 
2.19.1

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

Re: [libvirt] [PATCH 00/15] Move unix socket creation out of qemuBuildCommandLine

2018-10-08 Thread Michal Privoznik
On 10/04/2018 09:22 PM, Ján Tomko wrote:
> Add test coverage for all the possible devices and move all the
> socket creation to qemuProcessPrepareHost.
> TBD: remove the racy qemuSecuritySetSocketLabel call
> as well as the rest of host state modification out of
> qemuBuildCommandLine
> 
> Ján Tomko (15):
>   tests: add smartcard-passthrough-unix
>   tests: add parallel-unix-chardev
>   tests: add channel-unix-guestfwd
>   tests: add console-virtio-unix
>   tests: add usb-redir-unix
>   tests: add virtio-rng-egd-unix
>   tests: use real capabilities for net-vhostuser
>   qemuProcessPrepareDomain: pass xmlopt when creating monConfig
>   qemu: add fd to qemuDomainChrSourcePrivate
>   qemuBuildChrChardevStr: split attribute formatting
>   qemuBuildChrChardevStr: increase scope of qemuBuildChrChardevStr
>   qemuBuildChrChardevStr: pass fd from private data
>   qemu: remove QEMU_BUILD_CHARDEV_UNIX_FD_PASS
>   qemuBuildCommandLine: do not pass security manager
>   qemuOpenChrChardevUNIXSocket: move to qemu_process
> 
>  src/qemu/qemu_command.c| 192 
> +
>  src/qemu/qemu_command.h|   5 -
>  src/qemu/qemu_domain.c |   3 +
>  src/qemu/qemu_domain.h |   2 +
>  src/qemu/qemu_process.c| 188 +++-
>  src/qemu/qemu_process.h|   3 +
>  .../channel-unix-guestfwd.x86_64-2.5.0.args|  33 
>  .../channel-unix-guestfwd.x86_64-latest.args   |  36 
>  tests/qemuxml2argvdata/channel-unix-guestfwd.xml   |  37 
>  .../console-virtio-unix.x86_64-2.5.0.args  |  34 
>  .../console-virtio-unix.x86_64-latest.args |  37 
>  tests/qemuxml2argvdata/console-virtio-unix.xml |  33 
>  .../net-vhostuser.x86_64-2.5.0.args|  39 +
>  .../net-vhostuser.x86_64-latest.args   |  42 +
>  .../parallel-unix-chardev.x86_64-2.5.0.args|  33 
>  .../parallel-unix-chardev.x86_64-latest.args   |  36 
>  tests/qemuxml2argvdata/parallel-unix-chardev.xml   |  35 
>  .../smartcard-passthrough-unix.x86_64-2.5.0.args   |  30 
>  .../smartcard-passthrough-unix.x86_64-latest.args  |  33 
>  .../smartcard-passthrough-unix.xml |  19 ++
>  .../usb-redir-unix.x86_64-2.5.0.args   |  35 
>  .../usb-redir-unix.x86_64-latest.args  |  38 
>  tests/qemuxml2argvdata/usb-redir-unix.xml  |  45 +
>  .../virtio-rng-egd-unix.x86_64-2.5.0.args  |  30 
>  .../virtio-rng-egd-unix.x86_64-latest.args |  33 
>  tests/qemuxml2argvdata/virtio-rng-egd-unix.xml |  29 
>  tests/qemuxml2argvmock.c   |   3 +-
>  tests/qemuxml2argvtest.c   |  14 ++
>  28 files changed, 939 insertions(+), 158 deletions(-)
>  create mode 100644 
> tests/qemuxml2argvdata/channel-unix-guestfwd.x86_64-2.5.0.args
>  create mode 100644 
> tests/qemuxml2argvdata/channel-unix-guestfwd.x86_64-latest.args
>  create mode 100644 tests/qemuxml2argvdata/channel-unix-guestfwd.xml
>  create mode 100644 
> tests/qemuxml2argvdata/console-virtio-unix.x86_64-2.5.0.args
>  create mode 100644 
> tests/qemuxml2argvdata/console-virtio-unix.x86_64-latest.args
>  create mode 100644 tests/qemuxml2argvdata/console-virtio-unix.xml
>  create mode 100644 tests/qemuxml2argvdata/net-vhostuser.x86_64-2.5.0.args
>  create mode 100644 tests/qemuxml2argvdata/net-vhostuser.x86_64-latest.args
>  create mode 100644 
> tests/qemuxml2argvdata/parallel-unix-chardev.x86_64-2.5.0.args
>  create mode 100644 
> tests/qemuxml2argvdata/parallel-unix-chardev.x86_64-latest.args
>  create mode 100644 tests/qemuxml2argvdata/parallel-unix-chardev.xml
>  create mode 100644 
> tests/qemuxml2argvdata/smartcard-passthrough-unix.x86_64-2.5.0.args
>  create mode 100644 
> tests/qemuxml2argvdata/smartcard-passthrough-unix.x86_64-latest.args
>  create mode 100644 tests/qemuxml2argvdata/smartcard-passthrough-unix.xml
>  create mode 100644 tests/qemuxml2argvdata/usb-redir-unix.x86_64-2.5.0.args
>  create mode 100644 tests/qemuxml2argvdata/usb-redir-unix.x86_64-latest.args
>  create mode 100644 tests/qemuxml2argvdata/usb-redir-unix.xml
>  create mode 100644 
> tests/qemuxml2argvdata/virtio-rng-egd-unix.x86_64-2.5.0.args
>  create mode 100644 
> tests/qemuxml2argvdata/virtio-rng-egd-unix.x86_64-latest.args
>  create mode 100644 tests/qemuxml2argvdata/virtio-rng-egd-unix.xml
> 


ACK series

Michal

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

[libvirt] [PATCH RFC v2] qemu: fix deadlock when waiting in non async jobs

2018-10-08 Thread Nikolay Shirokovskiy
Block job abort operation can not handle properly qemu crashes when waiting for
abort/pivot completion. Deadlock scenario is next:

- qemuDomainBlockJobAbort waits for pivot/abort completion
- qemu crashes, then qemuProcessBeginStopJob broadcasts for VM condition and
  then waits for job condition (taken by qemuDomainBlockJobAbort)
- qemuDomainBlockJobAbort awakes but nothing really changed, VM is still
  active (vm->def->id != -1) so thread starts waiting for completion again.
  Now two threads are in deadlock.

First let's remove broadcast in qemuProcessBeginStopJob. It is simply wrong
because it is not set any condition before broadcast so that awaked threads can
not detect any changes. Crashing domain during async job will continue to be
handled properly because destroy job can run concurrently with async job and
destroy job calls qemuProcessStop which sets vm->def->id to -1 and broadcasts.

Second let's introduce flag that EOF is received and broadcast after that.
Now non async jobs can check this flag in wait loop.

Signed-off-by: Nikolay Shirokovskiy 

---

Diff from v1:

- patches 1 and 2 are already merged
- don't bother with reporting monitor EOF reason to user as most of
  time it is simply "unexpected eof" (this implies dropping patch 3)
- drop patch 5 as we now always report "domain is being stopped"
  in qemuDomainObjWait
- don't signal on monitor error for simplicity (otherwise we need to report
  something more elaborate that "domain is being stopped" as we don't
  kill domain on monitor errors. On the other hand I guess monitor
  error is rare case to handle it right now)
- keep virDomainObjWait for async jobs

It's a bit uneven that for async jobs domain is destroyed concurrently and for
non async jobs it will be actually destroyed after job get completed.  Also if
non async job needs issuing commands to qemu on cleanup then we will send these
commands in vain polluting logs etc because qemu process in not running at this
moment but typical check (virDomainObjIsActive) will think it is still running.

Domain is destroyed (qemuProcessStop) in a job due to patches [1] and [2].
However AFAIU it is not neccessary. If qemuProcessStop does not drop VM lock
then we don't need extra job to make qemuProcessStop and main job not
interleave. And we can drop the lock now only in qemuDomainObjBeginNestedJob in
qemuProcessStop which is introduced in [2]. AFAIU we can fix issues mentioned in
[2] the other way for example like it is done for qemu agent - we save agent
monitor reference on stack for entering/exiting agent monitor.

So I wonder can we instead of this fix remove job for qemuProcessStop and run
destroying domain cuncurrently for non async jobs too.

[1]
commit 8c9ff9960b29d4703a99efdd1cadcf6f48799cc0
Author: Jiri Denemark 
Date:   Thu Feb 11 15:32:48 2016 +0100

qemu: Process monitor EOF in a job

[2]
commit 81f50cb92d16643bcd749e3ab5b404b8b7cec643
Author: Jiri Denemark 
Date:   Thu Feb 11 11:20:28 2016 +0100

qemu: Avoid calling qemuProcessStop without a job

 src/qemu/qemu_domain.c  | 39 +++
 src/qemu/qemu_domain.h  |  4 
 src/qemu/qemu_driver.c  |  2 +-
 src/qemu/qemu_hotplug.c |  4 ++--
 src/qemu/qemu_process.c |  9 +
 5 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 939b2a3..aead72b 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -13534,3 +13534,42 @@ 
qemuDomainRunningReasonToResumeEvent(virDomainRunningReason reason)
 
 return VIR_DOMAIN_EVENT_RESUMED_UNPAUSED;
 }
+
+
+/**
+ * Waits for domain condition to be triggered for a specific period of time.
+ * if @until is 0 then waits indefinetely.
+ *
+ * Returns:
+ *  -1 on error
+ *   0 on success
+ *   1 on timeout
+ */
+int
+qemuDomainObjWait(virDomainObjPtr vm, unsigned long long until)
+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+int rc;
+
+if (until)
+rc = virCondWaitUntil(>cond, >parent.lock, until);
+else
+rc = virCondWait(>cond, >parent.lock);
+
+if (rc < 0) {
+if (until && errno == ETIMEDOUT)
+return 1;
+
+virReportSystemError(errno, "%s",
+ _("failed to wait for domain condition"));
+return -1;
+}
+
+if (priv->monEOF) {
+virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+ _("domain is being stopped"));
+return -1;
+}
+
+return 0;
+}
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 2f8a1bf..36ab294 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -281,6 +281,7 @@ struct _qemuDomainObjPrivate {
 virDomainChrSourceDefPtr monConfig;
 bool monJSON;
 bool monError;
+bool monEOF;
 unsigned long long monStart;
 
 qemuAgentPtr agent;
@@ -1085,4 +1086,7 @@ void qemuDomainStorageIdReset(qemuDomainObjPrivatePtr 
priv);
 virDomainEventResumedDetailType
 

Re: [libvirt] [PATCH] virresctrl: remove bogus virResetLastError

2018-10-08 Thread Michal Privoznik
On 10/05/2018 04:42 PM, Ján Tomko wrote:
> virFileReadValueUint does not log errors for non-existient files,
> it merely returns -2.
> 
> Commit 12093f1 introduced this.
> 
> Signed-off-by: Ján Tomko 
> ---
>  src/util/virresctrl.c | 2 --
>  1 file changed, 2 deletions(-)
> 


ACK

Michal

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

Re: [libvirt] [PATCH] virt-host-validate: Fix build on non-Linux

2018-10-08 Thread Pavel Hrdina
On Mon, Oct 08, 2018 at 09:34:48AM +0200, Michal Privoznik wrote:
> For non-Linux platforms we have
> virHostValidateCGroupControllers() stub which only reports an
> error. But we are not marking the ignored arguments the way we
> should.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  tools/virt-host-validate-common.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Pavel Hrdina 


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

[libvirt] [PATCH] virt-host-validate: Fix build on non-Linux

2018-10-08 Thread Michal Privoznik
For non-Linux platforms we have
virHostValidateCGroupControllers() stub which only reports an
error. But we are not marking the ignored arguments the way we
should.

Signed-off-by: Michal Privoznik 
---
 tools/virt-host-validate-common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/virt-host-validate-common.c 
b/tools/virt-host-validate-common.c
index 4e70fe9e9c..73e3bdb34c 100644
--- a/tools/virt-host-validate-common.c
+++ b/tools/virt-host-validate-common.c
@@ -322,8 +322,8 @@ int virHostValidateCGroupControllers(const char *hvname,
 return ret;
 }
 #else /*  !__linux__ */
-int virHostValidateCGroupControllers(const char *hvname,
- int controllers,
+int virHostValidateCGroupControllers(const char *hvname ATTRIBUTE_UNUSED,
+ int controllers ATTRIBUTE_UNUSED,
  virHostValidateLevel level)
 {
 virHostMsgFail(level, "%s", "This platform does not support cgroups");
-- 
2.18.0

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


Re: [libvirt] [PATCH v2] qemu: agent: Avoid agentError when closing the QEMU agent

2018-10-08 Thread wang.yechao255
> On 9/28/18 5:36 AM, Wang Yechao wrote:
> > After calling qemuAgentClose(), it is still possible for
> > the QEMU Agent I/O event callback to get invoked. This
> > will trigger an agent error because mon->fd has been set
> > to -1 at this point.  Then vm->privateData->agentError is
> > always 'true' except that restart libvirtd or restart
> > qemu-guest-agent process in guest.
> >
> > Silently ignore the case where mon->fd is -1, likewise for
> > mon->watch being zero.
> >
> > Signed-off-by: Wang Yechao 
> > ---
> > v1 patch:
> > https://www.redhat.com/archives/libvir-list/2018-September/msg01382.html
> >
> > Changes in v2:
> >  - do not set agentError, let agent state as disconnected instead of error.
> > ---
> >  src/qemu/qemu_agent.c | 13 -
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >
>
> Since the v1 review referenced commit 89563efc which it seems you
> applied or liberally copied here, I think it would be a "good idea" to
> reference that commit in your commit message.  Imitation is a form of
> flattery and so as is attribution.
>
> John

Thanks for your advice, I should reference commit 89563efc in my
commit message.

> > diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> > index 97ad0e7..d842b0e 100644
> > --- a/src/qemu/qemu_agent.c
> > +++ b/src/qemu/qemu_agent.c
> > @@ -530,6 +530,9 @@ static void qemuAgentUpdateWatch(qemuAgentPtr mon)
> >  VIR_EVENT_HANDLE_HANGUP |
> >  VIR_EVENT_HANDLE_ERROR;
> >
> > +if (!mon->watch)
> > +return;
> > +
> >  if (mon->lastError.code == VIR_ERR_OK) {
> >  events |= VIR_EVENT_HANDLE_READABLE;
> >
> > @@ -555,6 +558,12 @@ qemuAgentIO(int watch, int fd, int events, void 
> > *opaque)
> >  VIR_DEBUG("Agent %p I/O on watch %d fd %d events %d", mon, watch, fd, 
> > events);
> >  #endif
> >
> > +if (mon->fd == -1 || mon->watch == 0) {
> > +virObjectUnlock(mon);
> > +virObjectUnref(mon);
> > +return;
> > +}
> > +
> >  if (mon->fd != fd || mon->watch != watch) {
> >  if (events & (VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR))
> >  eof = true;
> > @@ -788,8 +797,10 @@ void qemuAgentClose(qemuAgentPtr mon)
> >  virObjectLock(mon);
> >
> >  if (mon->fd >= 0) {
> > -if (mon->watch)
> > +if (mon->watch) {
> >  virEventRemoveHandle(mon->watch);
> > +mon->watch = 0;
> > +}
> >  VIR_FORCE_CLOSE(mon->fd);
> >  }
> >
>
---
Best wishes
Wang Yechao--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] qemu: agent: Avoid agentError when closing the QEMU agent

2018-10-08 Thread wang.yechao255
> On 09/28/2018 11:36 AM, Wang Yechao wrote:
> > After calling qemuAgentClose(), it is still possible for
> > the QEMU Agent I/O event callback to get invoked. This
> > will trigger an agent error because mon->fd has been set
> > to -1 at this point.  Then vm->privateData->agentError is
> > always 'true' except that restart libvirtd or restart
> > qemu-guest-agent process in guest.
> >
> > Silently ignore the case where mon->fd is -1, likewise for
> > mon->watch being zero.
> >
> > Signed-off-by: Wang Yechao 
> > ---
> > v1 patch:
> > https://www.redhat.com/archives/libvir-list/2018-September/msg01382.html
> >
> > Changes in v2:
> >  - do not set agentError, let agent state as disconnected instead of error.
> > ---
> >  src/qemu/qemu_agent.c | 13 -
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> > index 97ad0e7..d842b0e 100644
> > --- a/src/qemu/qemu_agent.c
> > +++ b/src/qemu/qemu_agent.c
> > @@ -530,6 +530,9 @@ static void qemuAgentUpdateWatch(qemuAgentPtr mon)
> >  VIR_EVENT_HANDLE_HANGUP |
> >  VIR_EVENT_HANDLE_ERROR;
> >
> > +if (!mon->watch)
> > +return;
> > +
> >  if (mon->lastError.code == VIR_ERR_OK) {
> >  events |= VIR_EVENT_HANDLE_READABLE;
> >
> > @@ -555,6 +558,12 @@ qemuAgentIO(int watch, int fd, int events, void 
> > *opaque)
> >  VIR_DEBUG("Agent %p I/O on watch %d fd %d events %d", mon, watch, fd, 
> > events);
> >  #endif
> >
> > +if (mon->fd == -1 || mon->watch == 0) {
> > +virObjectUnlock(mon);
> > +virObjectUnref(mon);
> > +return;
> > +}
> > +
>
> Is this safe to do? What if there's a thread waiting for a message from
> the agent? Shouldn't @eof variable be set in this case so that
> appropriate code is run?

It is safe to do. The waiting thread is waked up when qemuAgentClose
invoked, and it cannot get any message from the agent at this time.
There is no need to set the @eof variable, because the EOF callback's job
has been done when closing the agent.

> >  if (mon->fd != fd || mon->watch != watch) {
> >  if (events & (VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR))
> >  eof = true;
> > @@ -788,8 +797,10 @@ void qemuAgentClose(qemuAgentPtr mon)
> >  virObjectLock(mon);
> >
> >  if (mon->fd >= 0) {
> > -if (mon->watch)
> > +if (mon->watch) {
> >  virEventRemoveHandle(mon->watch);
> > +mon->watch = 0;
> > +}
> >  VIR_FORCE_CLOSE(mon->fd);
> >  }
> >
> >
>
> Michal
>
---
Best wishes
Wang Yechao--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list