Re: Questions about using qemuProcess API within a libvirt test

[Collin Walling]

Much appreciated to you both for the well-written explanations. I'll keep my 
libvirt tests 
to mock input/output responses and find an alternative test suite for using 
libvirt with a 
real QEMU instance.

Thank you for your time!

-- 
Respectfully,
- Collin Walling

Re: [PATCH] apparmor: allow to call vhost-user-gpu

[Jim Fehlig]

On 2/14/20 1:14 PM, Christian Ehrhardt wrote:



On Fri, Feb 14, 2020 at 6:00 PM Jim Fehlig > wrote:


On 2/13/20 4:32 AM, Christian Ehrhardt wrote:
 > Configuring vhost-user-gpu like:
 >      
 >        
 >        
 >      
 > Triggers an apparmor denial like:
 >      apparmor="DENIED" operation="exec" profile="libvirtd"
 >      name="/usr/lib/qemu/vhost-user-gpu" pid=888257 comm="libvirtd"
 >      requested_mask="x" denied_mask="x" fsuid=0 ouid=0
 >
 > This helper is provided by qemu for vhost-user-gpu and thereby being
 > in the same path as qemu_bridge_helper. Due to that adding a rule 
allowing
 > to call uses the same path list.

Does the vhost-usr-gpu helper need a profile to restrict its access, 
similar to
the bridge helper?

Hi Jim,
Yes - we can later on add one, as soon as someone did the work to trace all the 
things that will be needed.
I had no full setup - and I'm not sure about the multitude of potential 
configurations - so I didn't go that far.

I didn't have that yet, but if anyone has please just add a follow on patch.

The P in PUx allows that someone defines an external profile to guard it - and 
it would be used, but without one existing the U allows it to fall back to 
unconfined.


Nod. That's reasonable behavior in the absence of an internal profile.

If/Once we add an internal profile like we do for bridge helper P can be changed 
to C, but as I said I have no useful profile and no full setup to test one 
at the moment.


With a bit of searching I could probably find a setup, but getting access is 
another thing. In the meantime your patch is a fine alternative IMO


Reviewed-by: Jim Fehlig 

Regards,
Jim


 >
 > Signed-off-by: Christian Ehrhardt mailto:christian.ehrha...@canonical.com>>
 > ---
 >   src/security/apparmor/usr.sbin.libvirtd.in
 | 1 +
 >   1 file changed, 1 insertion(+)
 >
 > diff --git a/src/security/apparmor/usr.sbin.libvirtd.in
 b/src/security/apparmor/usr.sbin.libvirtd.in

 > index b384b7213b..1e137039e9 100644
 > --- a/src/security/apparmor/usr.sbin.libvirtd.in

 > +++ b/src/security/apparmor/usr.sbin.libvirtd.in

 > @@ -86,6 +86,7 @@ profile libvirtd @sbindir@/libvirtd
flags=(attach_disconnected) {
 >     /usr/{lib,lib64}/xen-common/bin/xen-toolstack PUx,
 >     /usr/{lib,lib64}/xen/bin/* Ux,
 >     /usr/lib/xen-*/bin/libxl-save-helper PUx,
 > +  /usr/{lib,lib64,lib/qemu,libexec}/vhost-user-gpu PUx,
 >
 >     # Required by 
nwfilter_ebiptables_driver.c:ebiptablesWriteToTempFile() to
 >     # read and run an ebtables script.
 >



--
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH] apparmor: allow to call vhost-user-gpu

[Christian Ehrhardt]

On Fri, Feb 14, 2020 at 6:00 PM Jim Fehlig  wrote:

> On 2/13/20 4:32 AM, Christian Ehrhardt wrote:
> > Configuring vhost-user-gpu like:
> >  
> >
> >
> >  
> > Triggers an apparmor denial like:
> >  apparmor="DENIED" operation="exec" profile="libvirtd"
> >  name="/usr/lib/qemu/vhost-user-gpu" pid=888257 comm="libvirtd"
> >  requested_mask="x" denied_mask="x" fsuid=0 ouid=0
> >
> > This helper is provided by qemu for vhost-user-gpu and thereby being
> > in the same path as qemu_bridge_helper. Due to that adding a rule
> allowing
> > to call uses the same path list.
>
> Does the vhost-usr-gpu helper need a profile to restrict its access,
> similar to
> the bridge helper?
>

Hi Jim,
Yes - we can later on add one, as soon as someone did the work to trace all
the things that will be needed.
I had no full setup - and I'm not sure about the multitude of potential
configurations - so I didn't go that far.
I didn't have that yet, but if anyone has please just add a follow on patch.

The P in PUx allows that someone defines an external profile to guard it -
and it would be used, but without one existing the U allows it to fall back
to unconfined.
If/Once we add an internal profile like we do for bridge helper P can be
changed to C, but as I said I have no useful profile and no full setup to
test one at the moment.

Regards,
> Jim
>
> >
> > Signed-off-by: Christian Ehrhardt 
> > ---
> >   src/security/apparmor/usr.sbin.libvirtd.in | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/src/security/apparmor/usr.sbin.libvirtd.in
> b/src/security/apparmor/usr.sbin.libvirtd.in
> > index b384b7213b..1e137039e9 100644
> > --- a/src/security/apparmor/usr.sbin.libvirtd.in
> > +++ b/src/security/apparmor/usr.sbin.libvirtd.in
> > @@ -86,6 +86,7 @@ profile libvirtd @sbindir@/libvirtd
> flags=(attach_disconnected) {
> > /usr/{lib,lib64}/xen-common/bin/xen-toolstack PUx,
> > /usr/{lib,lib64}/xen/bin/* Ux,
> > /usr/lib/xen-*/bin/libxl-save-helper PUx,
> > +  /usr/{lib,lib64,lib/qemu,libexec}/vhost-user-gpu PUx,
> >
> > # Required by
> nwfilter_ebiptables_driver.c:ebiptablesWriteToTempFile() to
> > # read and run an ebtables script.
> >
>
>

-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [libvirt PATCH 05/11] src: introduce an abstraction for running event loops

[Daniel P . Berrangé]

On Fri, Feb 14, 2020 at 12:52:03PM +, Daniel P. Berrangé wrote:
> We want a way to easily run a private GMainContext in a
> thread, with correct synchronization between startup
> and shutdown of the thread.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  po/POTFILES.in|   1 +
>  src/libvirt_private.syms  |   5 ++
>  src/util/Makefile.inc.am  |   2 +
>  src/util/vireventthread.c | 175 ++
>  src/util/vireventthread.h |  31 +++
>  5 files changed, 214 insertions(+)
>  create mode 100644 src/util/vireventthread.c
>  create mode 100644 src/util/vireventthread.h
> 
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index dba0d3a12e..d49c10407a 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -238,6 +238,7 @@
>  @SRCDIR@/src/util/virerror.c
>  @SRCDIR@/src/util/virerror.h
>  @SRCDIR@/src/util/virevent.c
> +@SRCDIR@/src/util/vireventthread.c
>  @SRCDIR@/src/util/virfcp.c
>  @SRCDIR@/src/util/virfdstream.c
>  @SRCDIR@/src/util/virfile.c
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 375e6ea000..361c9d6c13 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1938,6 +1938,11 @@ virEventGLibRegister;
>  virEventGLibRunOnce;
>  
>  
> +# util/vireventthread.h
> +virEventThreadGetContext;
> +virEventThreadNew;
> +
> +
>  # util/virfcp.h
>  virFCIsCapableRport;
>  virFCReadRportValue;
> diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am
> index fbe67090d3..35629808c9 100644
> --- a/src/util/Makefile.inc.am
> +++ b/src/util/Makefile.inc.am
> @@ -65,6 +65,8 @@ UTIL_SOURCES = \
>   util/vireventglib.h \
>   util/vireventglibwatch.c \
>   util/vireventglibwatch.h \
> + util/vireventthread.c \
> + util/vireventthread.h \
>   util/virfcp.c \
>   util/virfcp.h \
>   util/virfdstream.c \
> diff --git a/src/util/vireventthread.c b/src/util/vireventthread.c
> new file mode 100644
> index 00..aed376bc7c
> --- /dev/null
> +++ b/src/util/vireventthread.c
> @@ -0,0 +1,175 @@
> +/*
> + * vireventthread.c: thread running a dedicated GMainLoop
> + *
> + * Copyright (C) 2020 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library.  If not, see
> + * .
> + */
> +
> +#include 
> +
> +#include "vireventthread.h"
> +#include "virthread.h"
> +#include "virerror.h"
> +
> +struct _virEventThread {
> +GObject parent;
> +
> +GCond cond;
> +GMutex lock;
> +bool running;
> +
> +GThread *thread;
> +GMainContext *context;
> +GMainLoop *loop;
> +};
> +
> +G_DEFINE_TYPE(virEventThread, vir_event_thread, G_TYPE_OBJECT)
> +
> +#define VIR_FROM_THIS VIR_FROM_EVENT
> +
> +static void
> +vir_event_thread_finalize(GObject *object)
> +{
> +virEventThread *evt = VIR_EVENT_THREAD(object);
> +
> +if (evt->thread) {
> +g_main_loop_quit(evt->loop);
> +g_thread_unref(evt->thread);
> +}
> +
> +g_main_loop_unref(evt->loop);
> +g_main_context_unref(evt->context);
> +
> +g_mutex_clear(>lock);
> +g_cond_clear(>cond);
> +
> +G_OBJECT_CLASS(vir_event_thread_parent_class)->finalize(object);
> +}
> +
> +
> +static void
> +vir_event_thread_init(virEventThread *evt)
> +{
> +g_cond_init(>cond);
> +g_mutex_init(>lock);
> +evt->running = false;
> +evt->context = g_main_context_new();
> +evt->loop = g_main_loop_new(evt->context, FALSE);
> +}
> +
> +
> +static void
> +vir_event_thread_class_init(virEventThreadClass *klass)
> +{
> +GObjectClass *obj = G_OBJECT_CLASS(klass);
> +
> +obj->finalize = vir_event_thread_finalize;
> +}
> +
> +
> +static gboolean
> +virEventThreadNotify(void *opaque)
> +{
> +virEventThread *evt = opaque;
> +
> +g_mutex_lock(>lock);
> +evt->running = TRUE;
> +g_mutex_unlock(>lock);
> +g_cond_signal(>cond);
> +
> +return G_SOURCE_REMOVE;
> +}
> +
> +
> +static void *
> +virEventThreadWorker(void *opaque)
> +{
> +virEventThread *evt = opaque;
> +g_autoptr(GSource) running = g_idle_source_new();
> +
> +g_source_set_callback(running, virEventThreadNotify, evt, NULL);
> +
> +g_source_attach(running, evt->context);
> +
> +g_main_loop_run(evt->loop);
> +
> +g_main_loop_unref(evt->loop);
> +g_main_context_unref(evt->context);

self-NACK, this has use-after-free on the 'evt' object,
because we only kept a 

Re: [libvirt PATCH] docs: add news item about GNULIB removal

[Andrea Bolognani]

On Fri, 2020-02-14 at 16:37 +, Daniel P. Berrangé wrote:
> On Fri, Feb 07, 2020 at 06:18:59PM +0100, Andrea Bolognani wrote:
> > On Fri, 2020-02-07 at 16:21 +, Daniel P. Berrangé wrote:
> > > +  Other Linux distros of a similar vintage using GLibC are 
> > > expected
> > > +  to work. Linux distros using non-GLibC packages, and other
> > > +  non-Linux platforms may encounter regressions when building 
> > > this
> > > +  release. Please report any build problems encountered back to 
> > > the
> > > +  project maintainers for resolution.
> > 
> > Should we include the caveat that we're still following our platform
> > compatibility guidelines?
> 
> I don't want to discourage people from reporting issues. If someone
> reports an issue with a platform that's unsupported, we can consider
> on a case by case basis whether to accept the fix and/or add to the
> supported platforms.

Fair enough.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH] apparmor: allow to call vhost-user-gpu

[Jim Fehlig]

On 2/13/20 4:32 AM, Christian Ehrhardt wrote:

Configuring vhost-user-gpu like:
 
   
   
 
Triggers an apparmor denial like:
 apparmor="DENIED" operation="exec" profile="libvirtd"
 name="/usr/lib/qemu/vhost-user-gpu" pid=888257 comm="libvirtd"
 requested_mask="x" denied_mask="x" fsuid=0 ouid=0

This helper is provided by qemu for vhost-user-gpu and thereby being
in the same path as qemu_bridge_helper. Due to that adding a rule allowing
to call uses the same path list.


Does the vhost-usr-gpu helper need a profile to restrict its access, similar to 
the bridge helper?


Regards,
Jim



Signed-off-by: Christian Ehrhardt 
---
  src/security/apparmor/usr.sbin.libvirtd.in | 1 +
  1 file changed, 1 insertion(+)

diff --git a/src/security/apparmor/usr.sbin.libvirtd.in 
b/src/security/apparmor/usr.sbin.libvirtd.in
index b384b7213b..1e137039e9 100644
--- a/src/security/apparmor/usr.sbin.libvirtd.in
+++ b/src/security/apparmor/usr.sbin.libvirtd.in
@@ -86,6 +86,7 @@ profile libvirtd @sbindir@/libvirtd 
flags=(attach_disconnected) {
/usr/{lib,lib64}/xen-common/bin/xen-toolstack PUx,
/usr/{lib,lib64}/xen/bin/* Ux,
/usr/lib/xen-*/bin/libxl-save-helper PUx,
+  /usr/{lib,lib64,lib/qemu,libexec}/vhost-user-gpu PUx,
  
# Required by nwfilter_ebiptables_driver.c:ebiptablesWriteToTempFile() to

# read and run an ebtables script.

Re: [libvirt PATCH] docs: add news item about GNULIB removal

[Daniel P . Berrangé]

On Fri, Feb 07, 2020 at 06:18:59PM +0100, Andrea Bolognani wrote:
> On Fri, 2020-02-07 at 16:21 +, Daniel P. Berrangé wrote:
> > While we have CI testing coverage for many platforms, we don't test any
> > non-GLibC based Linux and there are other non-Linux platforms we don't
> 
> It's "glibc", not "GLibC".
> 
> > officially target, both of which might hit regressions.
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  docs/news.xml | 19 +++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/docs/news.xml b/docs/news.xml
> > index f567a1182e..54ccc31abe 100644
> > --- a/docs/news.xml
> > +++ b/docs/news.xml
> > @@ -84,6 +84,25 @@
> >
> >  
> >  
> 
> This belongs quite squarely in the "Packaging changes" section IMHO.
> 
> > +  
> > +
> > +  use of GNULIB has been completely eliminated
> 
> Looking at the website and the git repository, it's either "gnulib"
> or "Gnulib", never "GNULIB".
> 
> > +
> > +
> > +  Historically libvirt has embedded GNULIB to provide fixes for
> > +  various platform portability problems. This usage has now been
> > +  eliminated and alternative approaches for platform portability
> > +  problems adopted where required. This has been validated on the
> > +  set of platforms covered by automated CI build testing: Fedora
> > +  30, 31 and rawhide; CentOS 7 and 8; Debian 9 and 10; Ubuntu 
> > 18.04;
> > +  FreeBSD 11 and 12; Mingw-w64; macOS 10.14 with XCode 10.3 and 
> > 11.3.
> 
> I think listing all targets is a bit excessive. Also note that we
> don't actually have CentOS 8 CI coverage yet.
> 
> > +  Other Linux distros of a similar vintage using GLibC are expected
> > +  to work. Linux distros using non-GLibC packages, and other
> > +  non-Linux platforms may encounter regressions when building this
> > +  release. Please report any build problems encountered back to the
> > +  project maintainers for resolution.
> 
> Should we include the caveat that we're still following our platform
> compatibility guidelines?

I don't want to discourage people from reporting issues. If someone
reports an issue with a platform that's unsupported, we can consider
on a case by case basis whether to accept the fix and/or add to the
supported platforms.

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

[libvirt PATCH v2] docs: add news item about gnulib removal

[Daniel P . Berrangé]

While we have CI testing coverage for many platforms, we don't test any
non-glibc based Linux and there are other non-Linux platforms we don't
officially target, both of which might hit regressions.

Signed-off-by: Daniel P. Berrangé 
---
 docs/news.xml | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index 5aa9d081a7..13812a1234 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -97,6 +97,25 @@
 
 
 
+
+  
+
+  use of gnulib has been completely eliminated
+
+
+  Historically libvirt has embedded gnulib to provide fixes for
+  various platform portability problems. This usage has now been
+  eliminated and alternative approaches for platform portability
+  problems adopted where required. This has been validated on the
+  set of platforms covered by automated CI build testing. Other
+  modern Linux distros using glibc are expected to work. Linux
+  distros using non-glibc packages, and other non-Linux platforms
+  may encounter regressions when building this release. Please
+  report any build problems encountered back to the project
+  maintainers for evaluation.
+
+  
+
   
   
 
-- 
2.24.1

[libvirt PATCH v2 1/6] qemu: test if bandwidth has 'floor' factored out to separate function

[Pavel Mores]

This compound condition will be useful in several places so it makes sense
to give it a name for better readability.

Signed-off-by: Pavel Mores 
---
 src/conf/netdev_bandwidth_conf.h | 7 +++
 src/network/bridge_driver.c  | 8 
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/src/conf/netdev_bandwidth_conf.h b/src/conf/netdev_bandwidth_conf.h
index 0004e48a4a..5b5ed52566 100644
--- a/src/conf/netdev_bandwidth_conf.h
+++ b/src/conf/netdev_bandwidth_conf.h
@@ -23,6 +23,7 @@
 #include "virbuffer.h"
 #include "virxml.h"
 #include "domain_conf.h"
+#include "network_conf.h"
 
 int virNetDevBandwidthParse(virNetDevBandwidthPtr *bandwidth,
 unsigned int *class_id,
@@ -57,3 +58,9 @@ static inline bool virNetDevSupportBandwidth(virDomainNetType 
type)
 }
 return false;
 }
+
+
+static inline bool virNetDevBandwidthHasFloor(const virNetDevBandwidth *b)
+{
+return b && b->in && b->in->floor != 0;
+}
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 94212eec77..14976c9821 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -5065,8 +5065,7 @@ networkCheckBandwidth(virNetworkObjPtr obj,
 
 virMacAddrFormat(ifaceMac, ifmac);
 
-if (ifaceBand && ifaceBand->in && ifaceBand->in->floor &&
-!(netBand && netBand->in)) {
+if (virNetDevBandwidthHasFloor(ifaceBand) && !(netBand && netBand->in)) {
 virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("Invalid use of 'floor' on interface with MAC "
  "address %s - network '%s' has no inbound QoS set"),
@@ -5079,8 +5078,9 @@ networkCheckBandwidth(virNetworkObjPtr obj,
 /* no QoS required, claim success */
 return 1;
 }
-if (((!ifaceBand || !ifaceBand->in || !ifaceBand->in->floor) &&
- (!oldBandwidth || !oldBandwidth->in || !oldBandwidth->in->floor))) {
+if (!virNetDevBandwidthHasFloor(ifaceBand) &&
+!virNetDevBandwidthHasFloor(oldBandwidth)) {
+
 VIR_DEBUG("No old/new interface bandwidth floor");
 /* no QoS required, claim success */
 return 1;
-- 
2.24.1

[libvirt PATCH v2 4/6] qemu: check if 'floor' is supported for given interface and network

[Pavel Mores]

Even if an interface of type 'network', setting 'floor' is only supported
if the network's forward type is nat, route, open or none.

Signed-off-by: Pavel Mores 
---
 src/network/bridge_driver.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 14976c9821..72220e1c64 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -5065,6 +5065,16 @@ networkCheckBandwidth(virNetworkObjPtr obj,
 
 virMacAddrFormat(ifaceMac, ifmac);
 
+if (virNetDevBandwidthHasFloor(ifaceBand) &&
+!virNetDevSupportBandwidthFloor(def->forward.type)) {
+
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+   _("Invalid use of 'floor' on interface with MAC address 
%s "
+ "- 'floor' is only supported for interface type 
'network' with forward type 'nat', 'route', 'open' or none"),
+   ifmac);
+return -1;
+}
+
 if (virNetDevBandwidthHasFloor(ifaceBand) && !(netBand && netBand->in)) {
 virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("Invalid use of 'floor' on interface with MAC "
-- 
2.24.1

[libvirt PATCH v2 6/6] docs: QoS parameter 'floor' is supported for 'open' networks too

[Pavel Mores]

Relevant code seems to treat forward modes 'route', 'nat', 'open' and 'none'
the same but documentation hasn't reflected that so far.

Signed-off-by: Pavel Mores 
---
 docs/formatnetwork.html.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
index 2448fb09e7..3d807ecab6 100644
--- a/docs/formatnetwork.html.in
+++ b/docs/formatnetwork.html.in
@@ -631,7 +631,7 @@
   goes through one point where QoS decisions can take place, hence
   why this attribute works only for virtual networks for now
   (that is interface type='network'/ with a
-  forward type of route, nat, or no forward at all). Moreover, the
+  forward type of route, nat, open or no forward at all). Moreover, the
   virtual network the interface is connected to is required to have
   at least inbound QoS set (average at least). If
   using the floor attribute users don't need to specify
-- 
2.24.1

[libvirt PATCH v2 5/6] qemu: call networkPlugBandwidth() for all types of network

[Pavel Mores]

To fix the actual bug, it was necessary to make networkPlugBandwidth() be
called also for 'bridge'-type networks implemented using macvtap's 'bridge'
mode (previously it was only called for those implemented on top of an
existing bridge).

However, it seems beneficial to call it for other network types as well, at
least because it removes an inconsistency in types of bandwidth configuration
changes permissible in inactive and active domain configs.  It should also be
safe as the function pretty much amounts to NOP if no QoS is requested and the
new behaviour should not be any worse than before if it is.

Signed-off-by: Pavel Mores 
---
 src/network/bridge_driver.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 72220e1c64..c8f7f07acb 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -4571,8 +4571,6 @@ networkAllocatePort(virNetworkObjPtr obj,
 return -1;
 }
 
-if (networkPlugBandwidth(obj, >mac, port->bandwidth, 
>class_id) < 0)
-return -1;
 break;
 
 case VIR_NETWORK_FORWARD_HOSTDEV: {
@@ -4637,8 +4635,6 @@ networkAllocatePort(virNetworkObjPtr obj,
 }
 }
 
-if (networkPlugBandwidth(obj, >mac, port->bandwidth, 
>class_id) < 0)
-return -1;
 break;
 }
 
@@ -4736,6 +4732,11 @@ networkAllocatePort(virNetworkObjPtr obj,
 return -1;
 }
 
+
+if (networkPlugBandwidth(obj, >mac, port->bandwidth,
+ >class_id) < 0)
+return -1;
+
 if (virNetworkObjMacMgrAdd(obj, driver->dnsmasqStateDir,
port->ownername, >mac) < 0)
 return -1;
-- 
2.24.1

[libvirt PATCH v2 2/6] qemu: add function to test if network supports setting 'floor'

[Pavel Mores]

This function will be useful in upcoming commits when code to check whether
a network can support 'floor' in the first place is introduced.

Signed-off-by: Pavel Mores 
---
 src/conf/netdev_bandwidth_conf.h | 20 
 1 file changed, 20 insertions(+)

diff --git a/src/conf/netdev_bandwidth_conf.h b/src/conf/netdev_bandwidth_conf.h
index 5b5ed52566..0596d555e5 100644
--- a/src/conf/netdev_bandwidth_conf.h
+++ b/src/conf/netdev_bandwidth_conf.h
@@ -60,6 +60,26 @@ static inline bool 
virNetDevSupportBandwidth(virDomainNetType type)
 }
 
 
+static inline bool virNetDevSupportBandwidthFloor(virNetworkForwardType type)
+{
+switch (type) {
+case VIR_NETWORK_FORWARD_NONE:
+case VIR_NETWORK_FORWARD_NAT:
+case VIR_NETWORK_FORWARD_ROUTE:
+case VIR_NETWORK_FORWARD_OPEN:
+return true;
+case VIR_NETWORK_FORWARD_BRIDGE:
+case VIR_NETWORK_FORWARD_PRIVATE:
+case VIR_NETWORK_FORWARD_VEPA:
+case VIR_NETWORK_FORWARD_PASSTHROUGH:
+case VIR_NETWORK_FORWARD_HOSTDEV:
+case VIR_NETWORK_FORWARD_LAST:
+break;
+}
+return false;
+}
+
+
 static inline bool virNetDevBandwidthHasFloor(const virNetDevBandwidth *b)
 {
 return b && b->in && b->in->floor != 0;
-- 
2.24.1

[libvirt PATCH v2 3/6] qemu: fail on attempt to set 'floor' if interface type is not 'network'

[Pavel Mores]

QoS 'floor' setting is documented to be only supported for interfaces of
type 'network'.  Fail with an error message on attempt to set 'floor' on
an interface of any other type.

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

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2813f084cd..f686b858cf 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11672,9 +11672,21 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom,
sizeof(*newBandwidth->out));
 }
 
-if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
-virDomainNetBandwidthUpdate(net, newBandwidth) < 0)
-goto endjob;
+if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
+if (virDomainNetBandwidthUpdate(net, newBandwidth) < 0)
+goto endjob;
+} else {
+if (virNetDevBandwidthHasFloor(bandwidth)) {
+char ifmac[VIR_MAC_STRING_BUFLEN];
+
+virMacAddrFormat(>mac, ifmac);
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+   _("Invalid use of 'floor' on interface with MAC 
address %s "
+   "- 'floor' is only supported for interface type 
'network' with forward type 'nat', 'route', 'open' or none"),
+   ifmac);
+goto endjob;
+}
+}
 
 if (virNetDevBandwidthSet(net->ifname, newBandwidth, false,
   !virDomainNetTypeSharesHostView(net)) < 0) {
-- 
2.24.1

[libvirt PATCH v2 0/6] qemu: add stricter checks of permissibility of the QoS parameter 'floor'

[Pavel Mores]

v2 is mostly just integrating requests from Michal's review.  The initial two
commits introduce new utility functions to be used in the following two
commits.  The final two commits have no substantial changes since v1.

The only exception are long lines caused by error messages which stay unbroken
in v2 as per libvirt's contributor's guidelines (as was also pointed out during
review).

Pavel Mores (6):
  qemu: test if bandwidth has 'floor' factored out to separate function
  qemu: add function to test if network supports setting 'floor'
  qemu: fail on attempt to set 'floor' if interface type is not
'network'
  qemu: check if 'floor' is supported for given interface and network
  qemu: call networkPlugBandwidth() for all types of network
  docs: QoS parameter 'floor' is supported for 'open' networks too

 docs/formatnetwork.html.in   |  2 +-
 src/conf/netdev_bandwidth_conf.h | 27 +++
 src/network/bridge_driver.c  | 27 +++
 src/qemu/qemu_driver.c   | 18 +++---
 4 files changed, 62 insertions(+), 12 deletions(-)

-- 
2.24.1

Re: [PATCH v3 12/15] tests: qemublock: Add cases for creating image overlays on top of disks with

[Ján Tomko]

On Fri, Feb 14, 2020 at 01:50:22PM +0100, Peter Krempa wrote:

On Thu, Feb 13, 2020 at 15:54:53 +0100, Ján Tomko wrote:

On Wed, Feb 12, 2020 at 07:03:23PM +0100, Peter Krempa wrote:
> Add a set of test data to see whether the backing store strings are
> formatted reasonably. Note that we don't support direct creation of such
> images so those tests are not enabled.
>
> Signed-off-by: Peter Krempa 
> ---
> tests/qemublocktest.c |  2 ++
> .../imagecreate/qcow2-backing-qcow2-slice.json| 15 +++
> .../imagecreate/qcow2-backing-qcow2-slice.xml |  1 +
> .../imagecreate/qcow2-backing-raw-slice.json  | 15 +++
> .../imagecreate/qcow2-backing-raw-slice.xml   |  1 +
> .../qemublocktestdata/imagecreate/qcow2-slice.xml | 14 ++
> tests/qemublocktestdata/imagecreate/raw-slice.xml | 14 ++
> 7 files changed, 62 insertions(+)
> create mode 100644 
tests/qemublocktestdata/imagecreate/qcow2-backing-qcow2-slice.json
> create mode 12 
tests/qemublocktestdata/imagecreate/qcow2-backing-qcow2-slice.xml
> create mode 100644 
tests/qemublocktestdata/imagecreate/qcow2-backing-raw-slice.json
> create mode 12 
tests/qemublocktestdata/imagecreate/qcow2-backing-raw-slice.xml
> create mode 100644 tests/qemublocktestdata/imagecreate/qcow2-slice.xml
> create mode 100644 tests/qemublocktestdata/imagecreate/raw-slice.xml
>
> diff --git a/tests/qemublocktestdata/imagecreate/qcow2-slice.xml 
b/tests/qemublocktestdata/imagecreate/qcow2-slice.xml
> new file mode 100644
> index 00..6c5ae3107b
> --- /dev/null
> +++ b/tests/qemublocktestdata/imagecreate/qcow2-slice.xml
> @@ -0,0 +1,14 @@
> +
> +  
> +  
> +
> +  
> +
> +
> +  
> +

> +

This is not parsed.

> +  
> +
> +  
> +
> diff --git a/tests/qemublocktestdata/imagecreate/raw-slice.xml 
b/tests/qemublocktestdata/imagecreate/raw-slice.xml
> new file mode 100644
> index 00..adc7a175ce
> --- /dev/null
> +++ b/tests/qemublocktestdata/imagecreate/raw-slice.xml
> @@ -0,0 +1,14 @@
> +
> +  
> +  
> +
> +  
> +
> +
> +  
> +

> +

This is not parsed either.


Both are parsed. These are nodenames of the storage and format layer
when instantiating storage via -blockdev.



I sit corrected.

Re-reviewed-by: Ján Tomko 

Jano




> +  


signature.asc
Description: PGP signature

Re: [libvirt PATCH 07/11] qemu: start/stop an event thread for QMP probing

[Daniel P . Berrangé]

On Fri, Feb 14, 2020 at 02:03:59PM +0100, Ján Tomko wrote:
> On Fri, Feb 14, 2020 at 12:52:05PM +, Daniel P. Berrangé wrote:
> > In common with regular QEMU guests, the QMP probing
> > will need an event loop for handling monitor I/O
> > operations.
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> > src/qemu/qemu_process.c | 16 
> > src/qemu/qemu_process.h |  2 ++
> > 2 files changed, 18 insertions(+)
> > 
> > @@ -8408,6 +8411,9 @@ qemuProcessQMPNew(const char *binary,
> > {
> > qemuProcessQMPPtr ret = NULL;
> > qemuProcessQMPPtr proc = NULL;
> > +g_autoptr(GError) gerr = NULL;
> 
> Fails to build with Clang:
> 
> ../../src/qemu/qemu_process.c:8416:23: error: unused variable 'gerr' 
> [-Werror,-Wunused-variable]
> g_autoptr(GError) gerr = NULL;
>   ^

Opps, left over from earlier version


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

Re: [libvirt PATCH 08/11] tests: start/stop an event thread for QEMU monitor/agent tests

[Daniel P . Berrangé]

On Fri, Feb 14, 2020 at 02:04:48PM +0100, Ján Tomko wrote:
> On Fri, Feb 14, 2020 at 12:52:06PM +, Daniel P. Berrangé wrote:
> > Tests which are using the QEMU monitor / agent need to have an
> > event thread running a private GMainContext.
> > 
> > There is already a thread running the main libvirt event loop
> > but this can't be eliminated yet as it is used for more than
> > just the monitor client I/O.
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> > tests/qemumonitortestutils.c | 13 +
> > 1 file changed, 13 insertions(+)
> > 
> > diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c
> > index b29e5d8cd2..a1641050ea 100644
> > --- a/tests/qemumonitortestutils.c
> > +++ b/tests/qemumonitortestutils.c
> > @@ -1389,12 +1398,16 @@ qemuMonitorTestNewAgent(virDomainXMLOptionPtr 
> > xmlopt)
> > {
> > qemuMonitorTestPtr test = NULL;
> > virDomainChrSourceDef src;
> > +g_autofree char *threadName = NULL;
> > 
> 
> ../../tests/qemumonitortestutils.c:1402:22: error: unused variable 
> 'threadName' [-Werror,-Wunused-variable]
> g_autofree char *threadName = NULL;
>  ^

ANother left over


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

Re: [PATCH] qemu_domain: Modify access to a NVMe disk iff needed

[Ján Tomko]

On Fri, Feb 14, 2020 at 02:25:15PM +0100, Michal Privoznik wrote:

If a domain has a NVMe disk it already has the access configured.
Trying to configure it again on a commit or some other operation
is wrong and condemned to failure.

Signed-off-by: Michal Privoznik 
---
src/qemu/qemu_domain.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 72f03c3a35..b0e90f818d 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -11676,13 +11676,13 @@ qemuDomainStorageSourceAccessModify(virQEMUDriverPtr 
driver,

revoke_lockspace = true;

-if (qemuDomainStorageSourceAccessModifyNVMe(driver, vm, src, false) < 0)
-goto revoke;
-
-revoke_nvme = true;
-



/* When modifying access of existing @src namespace does not need update */


This renders the above comment incomplete. I suggest deleting it :)


if (!(flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_MODIFY_ACCESS)) {
+if (qemuDomainStorageSourceAccessModifyNVMe(driver, vm, src, false) < 
0)
+goto revoke;
+
+revoke_nvme = true;
+
if (qemuDomainNamespaceSetupDisk(vm, src) < 0)
goto revoke;

--


Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

[PATCH] qemu_domain: Modify access to a NVMe disk iff needed

[Michal Privoznik]

If a domain has a NVMe disk it already has the access configured.
Trying to configure it again on a commit or some other operation
is wrong and condemned to failure.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_domain.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 72f03c3a35..b0e90f818d 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -11676,13 +11676,13 @@ qemuDomainStorageSourceAccessModify(virQEMUDriverPtr 
driver,
 
 revoke_lockspace = true;
 
-if (qemuDomainStorageSourceAccessModifyNVMe(driver, vm, src, false) < 0)
-goto revoke;
-
-revoke_nvme = true;
-
 /* When modifying access of existing @src namespace does not need update */
 if (!(flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_MODIFY_ACCESS)) {
+if (qemuDomainStorageSourceAccessModifyNVMe(driver, vm, src, false) < 
0)
+goto revoke;
+
+revoke_nvme = true;
+
 if (qemuDomainNamespaceSetupDisk(vm, src) < 0)
 goto revoke;
 
-- 
2.24.1

Re: [libvirt PATCH 08/11] tests: start/stop an event thread for QEMU monitor/agent tests

[Ján Tomko]

On Fri, Feb 14, 2020 at 12:52:06PM +, Daniel P. Berrangé wrote:

Tests which are using the QEMU monitor / agent need to have an
event thread running a private GMainContext.

There is already a thread running the main libvirt event loop
but this can't be eliminated yet as it is used for more than
just the monitor client I/O.

Signed-off-by: Daniel P. Berrangé 
---
tests/qemumonitortestutils.c | 13 +
1 file changed, 13 insertions(+)

diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c
index b29e5d8cd2..a1641050ea 100644
--- a/tests/qemumonitortestutils.c
+++ b/tests/qemumonitortestutils.c
@@ -1389,12 +1398,16 @@ qemuMonitorTestNewAgent(virDomainXMLOptionPtr xmlopt)
{
qemuMonitorTestPtr test = NULL;
virDomainChrSourceDef src;
+g_autofree char *threadName = NULL;



../../tests/qemumonitortestutils.c:1402:22: error: unused variable 'threadName' 
[-Werror,-Wunused-variable]
g_autofree char *threadName = NULL;
 ^
1 error generated.

Jano


memset(, 0, sizeof(src));

if (!(test = qemuMonitorCommonTestNew(xmlopt, NULL, )))
goto error;

+if (!(test->eventThread = virEventThreadNew("agent-test")))
+goto error;
+
if (!(test->agent = qemuAgentOpen(test->vm,
  ,
  )))
--
2.24.1



signature.asc
Description: PGP signature

Re: [libvirt PATCH 07/11] qemu: start/stop an event thread for QMP probing

[Ján Tomko]

On Fri, Feb 14, 2020 at 12:52:05PM +, Daniel P. Berrangé wrote:

In common with regular QEMU guests, the QMP probing
will need an event loop for handling monitor I/O
operations.

Signed-off-by: Daniel P. Berrangé 
---
src/qemu/qemu_process.c | 16 
src/qemu/qemu_process.h |  2 ++
2 files changed, 18 insertions(+)

@@ -8408,6 +8411,9 @@ qemuProcessQMPNew(const char *binary,
{
qemuProcessQMPPtr ret = NULL;
qemuProcessQMPPtr proc = NULL;
+g_autoptr(GError) gerr = NULL;


Fails to build with Clang:

../../src/qemu/qemu_process.c:8416:23: error: unused variable 'gerr' 
[-Werror,-Wunused-variable]
g_autoptr(GError) gerr = NULL;
  ^
1 error generated.

Jano


+const char *threadSuffix;
+g_autofree char *threadName = NULL;

VIR_DEBUG("exec=%s, libDir=%s, runUid=%u, runGid=%u, forceTCG=%d",
  binary, libDir, runUid, runGid, forceTCG);
@@ -8422,6 +8428,16 @@ qemuProcessQMPNew(const char *binary,
proc->runGid = runGid;
proc->forceTCG = forceTCG;

+threadSuffix = strrchr(binary, '-');
+if (threadSuffix)
+threadSuffix++;
+else
+threadSuffix = binary;
+threadName = g_strdup_printf("qmp-%s", threadSuffix);
+
+if (!(proc->eventThread = virEventThreadNew(threadName)))
+goto cleanup;
+
ret = g_steal_pointer();

 cleanup:


signature.asc
Description: PGP signature

[libvirt PATCH 11/11] qemu: convert agent to use the per-VM event loop

[Daniel P . Berrangé]

This converts the QEMU agent APIs to use the per-VM
event loop, which involves switching from virEvent APIs
to GMainContext / GSource APIs.

A GSocket is used as a convenient way to create a GSource
for a socket, but is not yet used for actual I/O.

Signed-off-by: Daniel P. Berrangé 
---
 src/qemu/qemu_agent.c| 146 +++
 src/qemu/qemu_agent.h|   1 +
 src/qemu/qemu_process.c  |   1 +
 tests/qemumonitortestutils.c |   1 +
 4 files changed, 84 insertions(+), 65 deletions(-)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index da1081b60b..ecc9eb23d1 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -25,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "qemu_agent.h"
 #include "qemu_domain.h"
@@ -100,7 +101,10 @@ struct _qemuAgent {
 virCond notify;
 
 int fd;
-int watch;
+
+GMainContext *context;
+GSocket *socket;
+GSource *watch;
 
 bool running;
 
@@ -171,6 +175,7 @@ static void qemuAgentDispose(void *obj)
 (agent->cb->destroy)(agent, agent->vm);
 virCondDestroy(>notify);
 VIR_FREE(agent->buffer);
+g_main_context_unref(agent->context);
 virResetError(>lastError);
 }
 
@@ -187,13 +192,6 @@ qemuAgentOpenUnix(const char *socketpath)
 return -1;
 }
 
-if (virSetNonBlock(agentfd) < 0) {
-virReportSystemError(errno, "%s",
- _("Unable to put monitor "
-   "into non-blocking mode"));
-goto error;
-}
-
 if (virSetCloseExec(agentfd) < 0) {
 virReportSystemError(errno, "%s",
  _("Unable to set agent "
@@ -497,28 +495,62 @@ qemuAgentIORead(qemuAgentPtr agent)
 }
 
 
-static void qemuAgentUpdateWatch(qemuAgentPtr agent)
-{
-int events =
-VIR_EVENT_HANDLE_HANGUP |
-VIR_EVENT_HANDLE_ERROR;
+static gboolean
+qemuAgentIO(GSocket *socket,
+GIOCondition cond,
+gpointer opaque);
 
-if (!agent->watch)
-return;
+
+static void
+qemuAgentRegister(qemuAgentPtr agent)
+{
+GIOCondition cond = 0;
 
 if (agent->lastError.code == VIR_ERR_OK) {
-events |= VIR_EVENT_HANDLE_READABLE;
+cond |= G_IO_IN;
 
 if (agent->msg && agent->msg->txOffset < agent->msg->txLength)
-events |= VIR_EVENT_HANDLE_WRITABLE;
+cond |= G_IO_OUT;
 }
 
-virEventUpdateHandle(agent->watch, events);
+agent->watch = g_socket_create_source(agent->socket,
+cond,
+NULL);
+
+virObjectRef(agent);
+g_source_set_callback(agent->watch,
+  (GSourceFunc)qemuAgentIO,
+  agent,
+  NULL);
+
+g_source_attach(agent->watch,
+agent->context);
 }
 
 
 static void
-qemuAgentIO(int watch, int fd, int events, void *opaque)
+qemuAgentUnregister(qemuAgentPtr agent)
+{
+if (agent->watch) {
+g_source_destroy(agent->watch);
+g_source_unref(agent->watch);
+agent->watch = NULL;
+}
+}
+
+
+static void qemuAgentUpdateWatch(qemuAgentPtr agent)
+{
+qemuAgentUnregister(agent);
+if (agent->socket)
+qemuAgentRegister(agent);
+}
+
+
+static gboolean
+qemuAgentIO(GSocket *socket G_GNUC_UNUSED,
+GIOCondition cond,
+gpointer opaque)
 {
 qemuAgentPtr agent = opaque;
 bool error = false;
@@ -528,45 +560,36 @@ qemuAgentIO(int watch, int fd, int events, void *opaque)
 /* lock access to the agent and protect fd */
 virObjectLock(agent);
 #if DEBUG_IO
-VIR_DEBUG("Agent %p I/O on watch %d fd %d events %d", agent, watch, fd, 
events);
+VIR_DEBUG("Agent %p I/O on watch %d socket %p cond %d", agent, 
agent->socket, cond);
 #endif
 
-if (agent->fd == -1 || agent->watch == 0) {
+if (agent->fd == -1 || !agent->watch) {
 virObjectUnlock(agent);
 virObjectUnref(agent);
-return;
+return G_SOURCE_REMOVE;
 }
 
-if (agent->fd != fd || agent->watch != watch) {
-if (events & (VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR))
-eof = true;
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("event from unexpected fd %d!=%d / watch %d!=%d"),
-   agent->fd, fd, agent->watch, watch);
-error = true;
-} else if (agent->lastError.code != VIR_ERR_OK) {
-if (events & (VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR))
+if (agent->lastError.code != VIR_ERR_OK) {
+if (cond & (G_IO_HUP | G_IO_ERR))
 eof = true;
 error = true;
 } else {
-if (events & VIR_EVENT_HANDLE_WRITABLE) {
+if (cond & G_IO_OUT) {
 if (qemuAgentIOWrite(agent) < 0)
 error = true;
-events &= ~VIR_EVENT_HANDLE_WRITABLE;
 }
 
 if (!error &&
-events & 

[libvirt PATCH 10/11] qemu: fix variable naming in agent code

[Daniel P . Berrangé]

We are dealing with the QEMU agent, not the monitor.

Signed-off-by: Daniel P. Berrangé 
---
 src/qemu/qemu_agent.c | 498 +-
 1 file changed, 249 insertions(+), 249 deletions(-)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 7ca5975a76..da1081b60b 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -64,7 +64,7 @@ VIR_LOG_INIT("qemu.qemu_agent");
  *
 static struct {
 const char *type;
-void (*handler)(qemuAgentPtr mon, virJSONValuePtr data);
+void (*handler)(qemuAgentPtr agent, virJSONValuePtr data);
 } eventHandlers[] = {
 };
 */
@@ -77,13 +77,13 @@ struct _qemuAgentMessage {
 int txOffset;
 int txLength;
 
-/* Used by the JSON monitor to hold reply / error */
+/* Used by the JSON agent to hold reply / error */
 char *rxBuffer;
 int rxLength;
 void *rxObject;
 
 /* True if rxBuffer / rxObject are ready, or a
- * fatal error occurred on the monitor channel
+ * fatal error occurred on the agent channel
  */
 bool finished;
 /* true for sync command */
@@ -112,18 +112,18 @@ struct _qemuAgent {
  * non-NULL */
 qemuAgentMessagePtr msg;
 
-/* Buffer incoming data ready for Agent monitor
+/* Buffer incoming data ready for agent
  * code to process & find message boundaries */
 size_t bufferOffset;
 size_t bufferLength;
 char *buffer;
 
 /* If anything went wrong, this will be fed back
- * the next monitor msg */
+ * the next agent msg */
 virError lastError;
 
 /* Some guest agent commands don't return anything
- * but fire up an event on qemu monitor instead.
+ * but fire up an event on qemu agent instead.
  * Take that as indication of successful completion */
 qemuAgentEvent await_event;
 int timeout;
@@ -165,71 +165,71 @@ qemuAgentEscapeNonPrintable(const char *text)
 
 static void qemuAgentDispose(void *obj)
 {
-qemuAgentPtr mon = obj;
-VIR_DEBUG("mon=%p", mon);
-if (mon->cb && mon->cb->destroy)
-(mon->cb->destroy)(mon, mon->vm);
-virCondDestroy(>notify);
-VIR_FREE(mon->buffer);
-virResetError(>lastError);
+qemuAgentPtr agent = obj;
+VIR_DEBUG("agent=%p", agent);
+if (agent->cb && agent->cb->destroy)
+(agent->cb->destroy)(agent, agent->vm);
+virCondDestroy(>notify);
+VIR_FREE(agent->buffer);
+virResetError(>lastError);
 }
 
 static int
-qemuAgentOpenUnix(const char *monitor)
+qemuAgentOpenUnix(const char *socketpath)
 {
 struct sockaddr_un addr;
-int monfd;
+int agentfd;
 int ret = -1;
 
-if ((monfd = socket(AF_UNIX, SOCK_STREAM, 0)) < 0) {
+if ((agentfd = socket(AF_UNIX, SOCK_STREAM, 0)) < 0) {
 virReportSystemError(errno,
  "%s", _("failed to create socket"));
 return -1;
 }
 
-if (virSetNonBlock(monfd) < 0) {
+if (virSetNonBlock(agentfd) < 0) {
 virReportSystemError(errno, "%s",
  _("Unable to put monitor "
"into non-blocking mode"));
 goto error;
 }
 
-if (virSetCloseExec(monfd) < 0) {
+if (virSetCloseExec(agentfd) < 0) {
 virReportSystemError(errno, "%s",
- _("Unable to set monitor "
+ _("Unable to set agent "
"close-on-exec flag"));
 goto error;
 }
 
 memset(, 0, sizeof(addr));
 addr.sun_family = AF_UNIX;
-if (virStrcpyStatic(addr.sun_path, monitor) < 0) {
+if (virStrcpyStatic(addr.sun_path, socketpath) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("Agent path %s too big for destination"), monitor);
+   _("Socket path %s too big for destination"), 
socketpath);
 goto error;
 }
 
-ret = connect(monfd, (struct sockaddr *), sizeof(addr));
+ret = connect(agentfd, (struct sockaddr *), sizeof(addr));
 if (ret < 0) {
 virReportSystemError(errno, "%s",
- _("failed to connect to monitor socket"));
+ _("failed to connect to agent socket"));
 goto error;
 }
 
-return monfd;
+return agentfd;
 
  error:
-VIR_FORCE_CLOSE(monfd);
+VIR_FORCE_CLOSE(agentfd);
 return -1;
 }
 
 
 static int
-qemuAgentIOProcessEvent(qemuAgentPtr mon,
+qemuAgentIOProcessEvent(qemuAgentPtr agent,
 virJSONValuePtr obj)
 {
 const char *type;
-VIR_DEBUG("mon=%p obj=%p", mon, obj);
+VIR_DEBUG("agent=%p obj=%p", agent, obj);
 
 type = virJSONValueObjectGetString(obj, "event");
 if (!type) {
@@ -244,7 +244,7 @@ qemuAgentIOProcessEvent(qemuAgentPtr mon,
 virJSONValuePtr data = virJSONValueObjectGet(obj, "data");
 VIR_DEBUG("handle %s handler=%p data=%p", type,
   eventHandlers[i].handler, data);
-

[libvirt PATCH 08/11] tests: start/stop an event thread for QEMU monitor/agent tests

[Daniel P . Berrangé]

Tests which are using the QEMU monitor / agent need to have an
event thread running a private GMainContext.

There is already a thread running the main libvirt event loop
but this can't be eliminated yet as it is used for more than
just the monitor client I/O.

Signed-off-by: Daniel P. Berrangé 
---
 tests/qemumonitortestutils.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c
index b29e5d8cd2..a1641050ea 100644
--- a/tests/qemumonitortestutils.c
+++ b/tests/qemumonitortestutils.c
@@ -36,6 +36,7 @@
 #include "virlog.h"
 #include "virerror.h"
 #include "virstring.h"
+#include "vireventthread.h"
 
 #define VIR_FROM_THIS VIR_FROM_NONE
 
@@ -66,6 +67,8 @@ struct _qemuMonitorTest {
 virNetSocketPtr server;
 virNetSocketPtr client;
 
+virEventThread *eventThread;
+
 qemuMonitorPtr mon;
 qemuAgentPtr agent;
 
@@ -389,6 +392,8 @@ qemuMonitorTestFree(qemuMonitorTestPtr test)
 qemuAgentClose(test->agent);
 }
 
+g_object_unref(test->eventThread);
+
 virObjectUnref(test->vm);
 
 if (test->started)
@@ -1142,6 +1147,7 @@ qemuMonitorCommonTestInit(qemuMonitorTestPtr test)
 "}"
 /* We skip the normal handshake reply of "{\"execute\":\"qmp_capabilities\"}" 
*/
 
+
 qemuMonitorTestPtr
 qemuMonitorTestNew(virDomainXMLOptionPtr xmlopt,
virDomainObjPtr vm,
@@ -1157,6 +1163,9 @@ qemuMonitorTestNew(virDomainXMLOptionPtr xmlopt,
 if (!(test = qemuMonitorCommonTestNew(xmlopt, vm, )))
 goto error;
 
+if (!(test->eventThread = virEventThreadNew("mon-test")))
+goto error;
+
 test->qapischema = schema;
 if (!(test->mon = qemuMonitorOpen(test->vm,
   ,
@@ -1389,12 +1398,16 @@ qemuMonitorTestNewAgent(virDomainXMLOptionPtr xmlopt)
 {
 qemuMonitorTestPtr test = NULL;
 virDomainChrSourceDef src;
+g_autofree char *threadName = NULL;
 
 memset(, 0, sizeof(src));
 
 if (!(test = qemuMonitorCommonTestNew(xmlopt, NULL, )))
 goto error;
 
+if (!(test->eventThread = virEventThreadNew("agent-test")))
+goto error;
+
 if (!(test->agent = qemuAgentOpen(test->vm,
   ,
   )))
-- 
2.24.1

[libvirt PATCH 01/11] qemu: drop support for agent connections on PTYs

[Daniel P . Berrangé]

Libvirt has never configured the QEMU agent to support
running on a PTY implicitly. In theory an end user may
have written such an XML config, but this is reasonably
unlikely since when a bare  is provided, libvirt
will auto-expand it to a UNIX socket backend.

With this change a user who has use the PTY backend will
have to switch to the UNIX backend if they wish to use
libvirt APIs for interacting with the agent. This will
not have guest ABI impact.

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

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 7d01d21a11..7ca5975a76 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -223,30 +223,6 @@ qemuAgentOpenUnix(const char *monitor)
 return -1;
 }
 
-static int
-qemuAgentOpenPty(const char *monitor)
-{
-int monfd;
-
-if ((monfd = open(monitor, O_RDWR | O_NONBLOCK)) < 0) {
-virReportSystemError(errno,
- _("Unable to open monitor path %s"), monitor);
-return -1;
-}
-
-if (virSetCloseExec(monfd) < 0) {
-virReportSystemError(errno, "%s",
- _("Unable to set monitor close-on-exec flag"));
-goto error;
-}
-
-return monfd;
-
- error:
-VIR_FORCE_CLOSE(monfd);
-return -1;
-}
-
 
 static int
 qemuAgentIOProcessEvent(qemuAgentPtr mon,
@@ -705,22 +681,14 @@ qemuAgentOpen(virDomainObjPtr vm,
 mon->vm = vm;
 mon->cb = cb;
 
-switch (config->type) {
-case VIR_DOMAIN_CHR_TYPE_UNIX:
-mon->fd = qemuAgentOpenUnix(config->data.nix.path);
-break;
-
-case VIR_DOMAIN_CHR_TYPE_PTY:
-mon->fd = qemuAgentOpenPty(config->data.file.path);
-break;
-
-default:
+if (config->type != VIR_DOMAIN_CHR_TYPE_UNIX) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("unable to handle monitor type: %s"),
virDomainChrTypeToString(config->type));
 goto cleanup;
 }
 
+mon->fd = qemuAgentOpenUnix(config->data.nix.path);
 if (mon->fd == -1)
 goto cleanup;
 
-- 
2.24.1

[libvirt PATCH 09/11] qemu: convert monitor to use the per-VM event loop

[Daniel P . Berrangé]

This converts the QEMU monitor APIs to use the per-VM
event loop, which involves switching from virEvent APIs
to GMainContext / GSource APIs.

A GSocket is used as a convenient way to create a GSource
for a socket, but is not yet used for actual I/O.

Signed-off-by: Daniel P. Berrangé 
---
 src/qemu/qemu_monitor.c  | 145 ---
 src/qemu/qemu_monitor.h  |   3 +-
 src/qemu/qemu_process.c  |   6 +-
 tests/qemumonitortestutils.c |   1 +
 4 files changed, 71 insertions(+), 84 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index bf53962872..d969853963 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -24,6 +24,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "qemu_monitor.h"
 #include "qemu_monitor_text.h"
@@ -71,12 +72,9 @@ struct _qemuMonitor {
 
 int fd;
 
-/* Represents the watch number to be used for updating and
- * unregistering the monitor @fd for events in the event loop:
- * > 0: valid watch number
- * = 0: not registered
- * < 0: an error occurred during the registration of @fd */
-int watch;
+GMainContext *context;
+GSocket *socket;
+GSource *watch;
 
 virDomainObjPtr vm;
 
@@ -226,6 +224,7 @@ qemuMonitorDispose(void *obj)
 (mon->cb->destroy)(mon, mon->vm, mon->callbackOpaque);
 virObjectUnref(mon->vm);
 
+g_main_context_unref(mon->context);
 virResetError(>lastError);
 virCondDestroy(>notify);
 VIR_FREE(mon->buffer);
@@ -509,27 +508,16 @@ qemuMonitorIORead(qemuMonitorPtr mon)
 static void
 qemuMonitorUpdateWatch(qemuMonitorPtr mon)
 {
-int events =
-VIR_EVENT_HANDLE_HANGUP |
-VIR_EVENT_HANDLE_ERROR;
-
-if (!mon->watch)
-return;
-
-if (mon->lastError.code == VIR_ERR_OK) {
-events |= VIR_EVENT_HANDLE_READABLE;
-
-if ((mon->msg && mon->msg->txOffset < mon->msg->txLength) &&
-!mon->waitGreeting)
-events |= VIR_EVENT_HANDLE_WRITABLE;
-}
-
-virEventUpdateHandle(mon->watch, events);
+qemuMonitorUnregister(mon);
+if (mon->socket)
+qemuMonitorRegister(mon);
 }
 
 
-static void
-qemuMonitorIO(int watch, int fd, int events, void *opaque)
+static gboolean
+qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
+  GIOCondition cond,
+  gpointer opaque)
 {
 qemuMonitorPtr mon = opaque;
 bool error = false;
@@ -541,39 +529,29 @@ qemuMonitorIO(int watch, int fd, int events, void *opaque)
 /* lock access to the monitor and protect fd */
 virObjectLock(mon);
 #if DEBUG_IO
-VIR_DEBUG("Monitor %p I/O on watch %d fd %d events %d", mon, watch, fd, 
events);
+VIR_DEBUG("Monitor %p I/O on socket %p cond %d", mon, socket, cond);
 #endif
-if (mon->fd == -1 || mon->watch == 0) {
+if (mon->fd == -1 || !mon->watch) {
 virObjectUnlock(mon);
 virObjectUnref(mon);
-return;
+return G_SOURCE_REMOVE;
 }
 
-if (mon->fd != fd || mon->watch != watch) {
-if (events & (VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR))
-eof = true;
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("event from unexpected fd %d!=%d / watch %d!=%d"),
-   mon->fd, fd, mon->watch, watch);
-error = true;
-} else if (mon->lastError.code != VIR_ERR_OK) {
-if (events & (VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR))
+if (mon->lastError.code != VIR_ERR_OK) {
+if (cond & (G_IO_HUP | G_IO_ERR))
 eof = true;
 error = true;
 } else {
-if (events & VIR_EVENT_HANDLE_WRITABLE) {
+if (cond & G_IO_OUT) {
 if (qemuMonitorIOWrite(mon) < 0) {
 error = true;
 if (errno == ECONNRESET)
 hangup = true;
 }
-events &= ~VIR_EVENT_HANDLE_WRITABLE;
 }
 
-if (!error &&
-events & VIR_EVENT_HANDLE_READABLE) {
+if (!error && cond & G_IO_IN) {
 int got = qemuMonitorIORead(mon);
-events &= ~VIR_EVENT_HANDLE_READABLE;
 if (got < 0) {
 error = true;
 if (errno == ECONNRESET)
@@ -581,37 +559,29 @@ qemuMonitorIO(int watch, int fd, int events, void *opaque)
 } else if (got == 0) {
 eof = true;
 } else {
-/* Ignore hangup/error events if we read some data, to
+/* Ignore hangup/error cond if we read some data, to
  * give time for that data to be consumed */
-events = 0;
+cond = 0;
 
 if (qemuMonitorIOProcess(mon) < 0)
 error = true;
 }
 }
 
-if (events & VIR_EVENT_HANDLE_HANGUP) {
+if (cond & G_IO_HUP) {
 hangup = true;
 if (!error) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
 

[libvirt PATCH 03/11] src: set the OS level thread name

[Daniel P . Berrangé]

Setting the thread name makes it easier to debug libvirtd
when many threads are running.

Signed-off-by: Daniel P. Berrangé 
---
 src/libvirt_private.syms |  1 +
 src/util/virthread.c | 44 +++-
 src/util/virthread.h |  4 +++-
 3 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index dc0449d1d8..375e6ea000 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3258,6 +3258,7 @@ virThreadCreateFull;
 virThreadID;
 virThreadIsSelf;
 virThreadJoin;
+virThreadMaxName;
 virThreadSelf;
 virThreadSelfID;
 
diff --git a/src/util/virthread.c b/src/util/virthread.c
index cdc5cab604..750e8d5655 100644
--- a/src/util/virthread.c
+++ b/src/util/virthread.c
@@ -175,23 +175,57 @@ void virCondBroadcast(virCondPtr c)
 
 struct virThreadArgs {
 virThreadFunc func;
-const char *funcName;
+char *name;
 bool worker;
 void *opaque;
 };
 
+size_t virThreadMaxName(void)
+{
+#if defined(__FreeBSD__) || defined(__APPLE__)
+return 63;
+#else
+# ifdef __linux__
+return 15;
+# else
+return 0; /* unlimited */
+# endif
+#endif
+}
+
 static void *virThreadHelper(void *data)
 {
 struct virThreadArgs *args = data;
 struct virThreadArgs local = *args;
+g_autofree char *thname = NULL;
+size_t maxname = virThreadMaxName();
 
 /* Free args early, rather than tying it up during the entire thread.  */
 VIR_FREE(args);
 
 if (local.worker)
-virThreadJobSetWorker(local.funcName);
+virThreadJobSetWorker(local.name);
 else
-virThreadJobSet(local.funcName);
+virThreadJobSet(local.name);
+
+if (maxname) {
+thname = g_strndup(local.name, maxname);
+} else {
+thname = g_strdup(local.name);
+}
+g_free(local.name);
+
+#if defined(__linux__) || defined(WIN32)
+pthread_setname_np(pthread_self(), thname);
+#else
+# ifdef __FreeBSD__
+pthread_set_name_np(pthread_self(), thname);
+# else
+#  ifdef __APPLE__
+pthread_setname_np(thname);
+#  endif
+# endif
+#endif
 
 local.func(local.opaque);
 
@@ -204,7 +238,7 @@ static void *virThreadHelper(void *data)
 int virThreadCreateFull(virThreadPtr thread,
 bool joinable,
 virThreadFunc func,
-const char *funcName,
+const char *name,
 bool worker,
 void *opaque)
 {
@@ -221,7 +255,7 @@ int virThreadCreateFull(virThreadPtr thread,
 }
 
 args->func = func;
-args->funcName = funcName;
+args->name = g_strdup(name);
 args->worker = worker;
 args->opaque = opaque;
 
diff --git a/src/util/virthread.h b/src/util/virthread.h
index a7960e444a..c227951ddd 100644
--- a/src/util/virthread.h
+++ b/src/util/virthread.h
@@ -90,13 +90,15 @@ typedef void (*virThreadFunc)(void *opaque);
 int virThreadCreateFull(virThreadPtr thread,
 bool joinable,
 virThreadFunc func,
-const char *funcName,
+const char *name,
 bool worker,
 void *opaque) G_GNUC_WARN_UNUSED_RESULT;
 void virThreadSelf(virThreadPtr thread);
 bool virThreadIsSelf(virThreadPtr thread);
 void virThreadJoin(virThreadPtr thread);
 
+size_t virThreadMaxName(void);
+
 /* This API is *NOT* for general use. It exists solely as a stub
  * for integration with libselinux AVC callbacks */
 void virThreadCancel(virThreadPtr thread);
-- 
2.24.1

[libvirt PATCH 04/11] src: improve thread naming with human targetted names

[Daniel P . Berrangé]

Historically threads are given a name based on the C function,
and this name is just used inside libvirt. With OS level thread
naming this name is now visible to debuggers, but also has to
fit in 15 characters on Linux, so function names are too long
in some cases.

Signed-off-by: Daniel P. Berrangé 
---
 src/libxl/libxl_domain.c| 10 ++
 src/libxl/libxl_migration.c | 23 ++-
 src/lxc/lxc_fuse.c  |  4 ++--
 src/node_device/node_device_udev.c  |  7 ---
 src/nwfilter/nwfilter_dhcpsnoop.c   | 11 ++-
 src/nwfilter/nwfilter_learnipaddr.c | 10 ++
 src/qemu/qemu_driver.c  |  3 ++-
 src/qemu/qemu_migration.c   |  8 +---
 src/qemu/qemu_process.c | 17 -
 src/remote/remote_daemon.c  |  9 ++---
 src/rpc/virnetserver.c  |  9 +
 src/storage/storage_backend_scsi.c  |  4 ++--
 src/storage/storage_driver.c|  4 ++--
 src/util/vircommand.c   |  5 +++--
 src/util/virfdstream.c  | 10 ++
 src/util/virnodesuspend.c   |  8 +---
 src/util/virthreadpool.c| 14 ++
 src/util/virthreadpool.h|  2 +-
 18 files changed, 101 insertions(+), 57 deletions(-)

diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index c8b68665af..e3da9f777d 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -664,6 +664,7 @@ libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST 
libxl_event *event)
 virThread thread;
 g_autoptr(libxlDriverConfig) cfg = NULL;
 int ret = -1;
+g_autofree char *name = NULL;
 
 if (event->type != LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN &&
 event->type != LIBXL_EVENT_TYPE_DOMAIN_DEATH) {
@@ -687,12 +688,13 @@ libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST 
libxl_event *event)
 
 shutdown_info->driver = driver;
 shutdown_info->event = (libxl_event *)event;
+name = g_strdup_printf("ev-%d", event->domid);
 if (event->type == LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN)
-ret = virThreadCreate(, false, libxlDomainShutdownThread,
-  shutdown_info);
+ret = virThreadCreateFull(, false, libxlDomainShutdownThread,
+  name, false, shutdown_info);
 else if (event->type == LIBXL_EVENT_TYPE_DOMAIN_DEATH)
-ret = virThreadCreate(, false, libxlDomainDeathThread,
-  shutdown_info);
+ret = virThreadCreateFull(, false, libxlDomainDeathThread,
+  name, false, shutdown_info);
 
 if (ret < 0) {
 /*
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 873b2b3e01..e5f39cfc40 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -293,6 +293,7 @@ libxlMigrateDstReceive(virNetSocketPtr sock,
 virNetSocketPtr client_sock;
 int recvfd = -1;
 size_t i;
+g_autofree char *name = NULL;
 
 /* Accept migration connection */
 if (virNetSocketAccept(sock, _sock) < 0 || !client_sock) {
@@ -313,8 +314,13 @@ libxlMigrateDstReceive(virNetSocketPtr sock,
 VIR_FREE(priv->migrationDstReceiveThr);
 if (VIR_ALLOC(priv->migrationDstReceiveThr) < 0)
 goto fail;
-if (virThreadCreate(priv->migrationDstReceiveThr, true,
-libxlDoMigrateDstReceive, args) < 0) {
+
+name = g_strdup_printf("mig-%s", args->vm->def->name);
+if (virThreadCreateFull(priv->migrationDstReceiveThr, true,
+libxlDoMigrateDstReceive,
+name,
+false,
+args) < 0) {
 virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("Failed to create thread for receiving migration 
data"));
 goto fail;
@@ -553,6 +559,7 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn,
 char *xmlout = NULL;
 int dataFD[2] = { -1, -1 };
 int ret = -1;
+g_autofree char *name = NULL;
 
 if (libxlDomainMigrationPrepareAny(dconn, def, cookiein, cookieinlen,
, , _hook) < 0)
@@ -610,7 +617,10 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn,
 VIR_FREE(priv->migrationDstReceiveThr);
 if (VIR_ALLOC(priv->migrationDstReceiveThr) < 0)
 goto error;
-if (virThreadCreate(priv->migrationDstReceiveThr, true, 
libxlDoMigrateDstReceive, args) < 0) {
+name = g_strdup_printf("mig-%s", args->vm->def->name);
+if (virThreadCreateFull(priv->migrationDstReceiveThr, true,
+libxlDoMigrateDstReceive,
+name, false, args) < 0) {
 virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("Failed to create thread for receiving migration 
data"));
 goto endjob;
@@ -909,6 +919,7 @@ libxlMigrationSrcStartTunnel(libxlDriverPrivatePtr 

[libvirt PATCH 05/11] src: introduce an abstraction for running event loops

[Daniel P . Berrangé]

We want a way to easily run a private GMainContext in a
thread, with correct synchronization between startup
and shutdown of the thread.

Signed-off-by: Daniel P. Berrangé 
---
 po/POTFILES.in|   1 +
 src/libvirt_private.syms  |   5 ++
 src/util/Makefile.inc.am  |   2 +
 src/util/vireventthread.c | 175 ++
 src/util/vireventthread.h |  31 +++
 5 files changed, 214 insertions(+)
 create mode 100644 src/util/vireventthread.c
 create mode 100644 src/util/vireventthread.h

diff --git a/po/POTFILES.in b/po/POTFILES.in
index dba0d3a12e..d49c10407a 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -238,6 +238,7 @@
 @SRCDIR@/src/util/virerror.c
 @SRCDIR@/src/util/virerror.h
 @SRCDIR@/src/util/virevent.c
+@SRCDIR@/src/util/vireventthread.c
 @SRCDIR@/src/util/virfcp.c
 @SRCDIR@/src/util/virfdstream.c
 @SRCDIR@/src/util/virfile.c
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 375e6ea000..361c9d6c13 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1938,6 +1938,11 @@ virEventGLibRegister;
 virEventGLibRunOnce;
 
 
+# util/vireventthread.h
+virEventThreadGetContext;
+virEventThreadNew;
+
+
 # util/virfcp.h
 virFCIsCapableRport;
 virFCReadRportValue;
diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am
index fbe67090d3..35629808c9 100644
--- a/src/util/Makefile.inc.am
+++ b/src/util/Makefile.inc.am
@@ -65,6 +65,8 @@ UTIL_SOURCES = \
util/vireventglib.h \
util/vireventglibwatch.c \
util/vireventglibwatch.h \
+   util/vireventthread.c \
+   util/vireventthread.h \
util/virfcp.c \
util/virfcp.h \
util/virfdstream.c \
diff --git a/src/util/vireventthread.c b/src/util/vireventthread.c
new file mode 100644
index 00..aed376bc7c
--- /dev/null
+++ b/src/util/vireventthread.c
@@ -0,0 +1,175 @@
+/*
+ * vireventthread.c: thread running a dedicated GMainLoop
+ *
+ * Copyright (C) 2020 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ */
+
+#include 
+
+#include "vireventthread.h"
+#include "virthread.h"
+#include "virerror.h"
+
+struct _virEventThread {
+GObject parent;
+
+GCond cond;
+GMutex lock;
+bool running;
+
+GThread *thread;
+GMainContext *context;
+GMainLoop *loop;
+};
+
+G_DEFINE_TYPE(virEventThread, vir_event_thread, G_TYPE_OBJECT)
+
+#define VIR_FROM_THIS VIR_FROM_EVENT
+
+static void
+vir_event_thread_finalize(GObject *object)
+{
+virEventThread *evt = VIR_EVENT_THREAD(object);
+
+if (evt->thread) {
+g_main_loop_quit(evt->loop);
+g_thread_unref(evt->thread);
+}
+
+g_main_loop_unref(evt->loop);
+g_main_context_unref(evt->context);
+
+g_mutex_clear(>lock);
+g_cond_clear(>cond);
+
+G_OBJECT_CLASS(vir_event_thread_parent_class)->finalize(object);
+}
+
+
+static void
+vir_event_thread_init(virEventThread *evt)
+{
+g_cond_init(>cond);
+g_mutex_init(>lock);
+evt->running = false;
+evt->context = g_main_context_new();
+evt->loop = g_main_loop_new(evt->context, FALSE);
+}
+
+
+static void
+vir_event_thread_class_init(virEventThreadClass *klass)
+{
+GObjectClass *obj = G_OBJECT_CLASS(klass);
+
+obj->finalize = vir_event_thread_finalize;
+}
+
+
+static gboolean
+virEventThreadNotify(void *opaque)
+{
+virEventThread *evt = opaque;
+
+g_mutex_lock(>lock);
+evt->running = TRUE;
+g_mutex_unlock(>lock);
+g_cond_signal(>cond);
+
+return G_SOURCE_REMOVE;
+}
+
+
+static void *
+virEventThreadWorker(void *opaque)
+{
+virEventThread *evt = opaque;
+g_autoptr(GSource) running = g_idle_source_new();
+
+g_source_set_callback(running, virEventThreadNotify, evt, NULL);
+
+g_source_attach(running, evt->context);
+
+g_main_loop_run(evt->loop);
+
+g_main_loop_unref(evt->loop);
+g_main_context_unref(evt->context);
+
+return NULL;
+}
+
+
+static int
+virEventThreadStart(virEventThread *evt, const char *name)
+{
+g_autoptr(GError) gerr = NULL;
+g_autofree char *thname = NULL;
+size_t maxname = virThreadMaxName();
+
+if (maxname)
+thname = g_strndup(name, maxname);
+else
+thname = g_strdup(name);
+
+if (evt->thread) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Event thread is already running"));
+   

[libvirt PATCH 07/11] qemu: start/stop an event thread for QMP probing

[Daniel P . Berrangé]

In common with regular QEMU guests, the QMP probing
will need an event loop for handling monitor I/O
operations.

Signed-off-by: Daniel P. Berrangé 
---
 src/qemu/qemu_process.c | 16 
 src/qemu/qemu_process.h |  2 ++
 2 files changed, 18 insertions(+)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 73158e29e6..7475813e9f 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8377,6 +8377,9 @@ qemuProcessQMPFree(qemuProcessQMPPtr proc)
 return;
 
 qemuProcessQMPStop(proc);
+
+g_object_unref(proc->eventThread);
+
 VIR_FREE(proc->binary);
 VIR_FREE(proc->libDir);
 VIR_FREE(proc->uniqDir);
@@ -8408,6 +8411,9 @@ qemuProcessQMPNew(const char *binary,
 {
 qemuProcessQMPPtr ret = NULL;
 qemuProcessQMPPtr proc = NULL;
+g_autoptr(GError) gerr = NULL;
+const char *threadSuffix;
+g_autofree char *threadName = NULL;
 
 VIR_DEBUG("exec=%s, libDir=%s, runUid=%u, runGid=%u, forceTCG=%d",
   binary, libDir, runUid, runGid, forceTCG);
@@ -8422,6 +8428,16 @@ qemuProcessQMPNew(const char *binary,
 proc->runGid = runGid;
 proc->forceTCG = forceTCG;
 
+threadSuffix = strrchr(binary, '-');
+if (threadSuffix)
+threadSuffix++;
+else
+threadSuffix = binary;
+threadName = g_strdup_printf("qmp-%s", threadSuffix);
+
+if (!(proc->eventThread = virEventThreadNew(threadName)))
+goto cleanup;
+
 ret = g_steal_pointer();
 
  cleanup:
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index 9af9f967fd..3077d3ef9e 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -24,6 +24,7 @@
 #include "qemu_conf.h"
 #include "qemu_domain.h"
 #include "virstoragefile.h"
+#include "vireventthread.h"
 
 int qemuProcessPrepareMonitorChr(virDomainChrSourceDefPtr monConfig,
  const char *domainDir);
@@ -217,6 +218,7 @@ struct _qemuProcessQMP {
 char *monpath;
 char *pidfile;
 char *uniqDir;
+virEventThread *eventThread;
 virCommandPtr cmd;
 qemuMonitorPtr mon;
 pid_t pid;
-- 
2.24.1

[libvirt PATCH 06/11] qemu: start/stop an event loop thread for domains

[Daniel P . Berrangé]

The event loop thread will be responsible for handling
any per-domain I/O operations, most notably the QEMU
monitor and agent sockets.

We start this event loop when launching QEMU, but stopping
the event loop is a little more complicated. The obvious
idea is to stop it in qemuProcessStop(), but if we do that
we risk loosing the final events from the QEMU monitor, as
they might not have been read by the event thread at the
time we tell the thread to stop.

The solution is to delay shutdown of the event thread until
we have seen EOF from the QEMU monitor, and thus we know
there are no further events to process.

Note that this assumes that we don't have events to process
from the QEMU agent.

Signed-off-by: Daniel P. Berrangé 
---
 src/qemu/qemu_domain.c  | 33 +
 src/qemu/qemu_domain.h  |  6 ++
 src/qemu/qemu_process.c | 21 +
 3 files changed, 60 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 6fc0bd4e68..916a21b5f2 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2146,6 +2146,33 @@ dbusVMStateHashFree(void *opaque)
 }
 
 
+int
+qemuDomainObjStartWorker(virDomainObjPtr dom)
+{
+qemuDomainObjPrivatePtr priv = dom->privateData;
+
+if (!priv->eventThread) {
+g_autofree char *threadName = g_strdup_printf("vm-%s", dom->def->name);
+if (!(priv->eventThread = virEventThreadNew(threadName)))
+return -1;
+}
+
+return 0;
+}
+
+
+void
+qemuDomainObjStopWorker(virDomainObjPtr dom)
+{
+qemuDomainObjPrivatePtr priv = dom->privateData;
+
+if (priv->eventThread) {
+g_object_unref(priv->eventThread);
+priv->eventThread = NULL;
+}
+}
+
+
 static void *
 qemuDomainObjPrivateAlloc(void *opaque)
 {
@@ -2285,6 +2312,12 @@ qemuDomainObjPrivateFree(void *data)
 virHashFree(priv->blockjobs);
 virHashFree(priv->dbusVMStates);
 
+/* This should never be non-NULL if we get here, but just in case... */
+if (priv->eventThread) {
+VIR_ERROR(_("Unexpected event thread still active during domain 
deletion"));
+g_object_unref(priv->eventThread);
+}
+
 VIR_FREE(priv);
 }
 
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index f8fb48f2ff..9ce27fbdda 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -40,6 +40,7 @@
 #include "logging/log_manager.h"
 #include "virdomainmomentobjlist.h"
 #include "virenum.h"
+#include "vireventthread.h"
 
 #define QEMU_DOMAIN_FORMAT_LIVE_FLAGS \
 (VIR_DOMAIN_XML_SECURE)
@@ -300,6 +301,8 @@ struct _qemuDomainObjPrivate {
 
 virBitmapPtr namespaces;
 
+virEventThread *eventThread;
+
 qemuMonitorPtr mon;
 virDomainChrSourceDefPtr monConfig;
 bool monError;
@@ -630,6 +633,9 @@ struct _qemuDomainXmlNsDef {
 char **capsdel;
 };
 
+int qemuDomainObjStartWorker(virDomainObjPtr dom);
+void qemuDomainObjStopWorker(virDomainObjPtr dom);
+
 virDomainObjPtr qemuDomainObjFromDomain(virDomainPtr domain);
 
 qemuDomainSaveCookiePtr qemuDomainSaveCookieNew(virDomainObjPtr vm);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index e36d1dd7c7..73158e29e6 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -319,6 +319,9 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon,
 qemuDomainDestroyNamespace(driver, vm);
 
  cleanup:
+/* Now we got EOF we're not expecting more I/O, so we
+ * can finally kill the event thread */
+qemuDomainObjStopWorker(vm);
 virObjectUnlock(vm);
 }
 
@@ -6912,6 +6915,9 @@ qemuProcessLaunch(virConnectPtr conn,
 if (rv == -1) /* The VM failed to start */
 goto cleanup;
 
+if (qemuDomainObjStartWorker(vm) < 0)
+goto cleanup;
+
 VIR_DEBUG("Waiting for monitor to show up");
 if (qemuProcessWaitForMonitor(driver, vm, asyncJob, logCtxt) < 0)
 goto cleanup;
@@ -7394,6 +7400,18 @@ void qemuProcessStop(virQEMUDriverPtr driver,
 priv->monConfig = NULL;
 }
 
+/*
+ * We cannot stop the event thread at this time. When
+ * we are in this code, we may not yet have processed the
+ * STOP event or EOF from the monitor. So the event loop
+ * may have pending input that we need to process still.
+ * The qemuProcessHandleMonitorEOF method will kill
+ * the event thread because at that point we don't
+ * expect any more I/O from the QEMU monitor. We are
+ * assuming we don't need to get any more events from the
+ * QEMU agent at that time.
+ */
+
 /* Remove the master key */
 qemuDomainMasterKeyRemove(priv);
 
@@ -7985,6 +8003,9 @@ qemuProcessReconnect(void *opaque)
 virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS))
 retry = false;
 
+if (qemuDomainObjStartWorker(obj) < 0)
+goto error;
+
 VIR_DEBUG("Reconnect monitor to def=%p name='%s' retry=%d",
   obj, obj->def->name, retry);
 
-- 
2.24.1

[libvirt PATCH 02/11] qemu: drop ability to open monitor from FD

[Daniel P . Berrangé]

The qemuMonitorOpenFD method has not been used since it
was first introduced.

Signed-off-by: Daniel P. Berrangé 
---
 src/qemu/qemu_monitor.c | 10 --
 src/qemu/qemu_monitor.h |  5 -
 2 files changed, 15 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 008d4a0e75..bf53962872 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -824,16 +824,6 @@ qemuMonitorOpen(virDomainObjPtr vm,
 }
 
 
-qemuMonitorPtr
-qemuMonitorOpenFD(virDomainObjPtr vm,
-  int sockfd,
-  qemuMonitorCallbacksPtr cb,
-  void *opaque)
-{
-return qemuMonitorOpenInternal(vm, sockfd, cb, opaque);
-}
-
-
 /**
  * qemuMonitorRegister:
  * @mon: QEMU monitor
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 8cf9e11899..c84cd425df 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -394,11 +394,6 @@ qemuMonitorPtr qemuMonitorOpen(virDomainObjPtr vm,
qemuMonitorCallbacksPtr cb,
void *opaque)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5);
-qemuMonitorPtr qemuMonitorOpenFD(virDomainObjPtr vm,
- int sockfd,
- qemuMonitorCallbacksPtr cb,
- void *opaque)
-ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4);
 
 bool qemuMonitorRegister(qemuMonitorPtr mon)
 ATTRIBUTE_NONNULL(1);
-- 
2.24.1

[libvirt PATCH 00/11] qemu: introduce a per-VM event loop thread

[Daniel P . Berrangé]

This series changes the way we manage the QEMU monitor and
QEMU agent, such that all I/O is processed by a dedicated
event loop thread.

Many times in the past years people are reported issues
where long running monitor event callbacks block the main
libvirtd event loop for an unacceptably long period of
time. In the best case, this delays other work being
completed, but in bad cases it leads to mgmt app failures
when keepalive times trigger a client disconnect.

With this series, when we spawn QEMU, we also spawn a
dedicated thread running a GMainLoop instance. Then QEMU
monitor and QEMU agent UNIX sockets are switched to use
GMainContext for events instead of the traditional libvirt
event loop APIs. We kill off the event thread when we see
EOF on the QEMU monitor during shutdown.

The cost of this approach is one extra thread per VM,
which incurs a new OS process and a new stack allocation.

The QEMU driver already delegates some QMP event handling
to a thread pool for certain types of event. This was a
previous hack to mitigate the impact on the main event
loop. It is likely that we can remove this thread pool
from the QEMU driver & rely on the per-VM event threads
to do all the work. This will, however, require careful
analysis of each handler we pushed into the thread pool
to make sure its work doesn't have a dependency on the
event loop running in parallel.

This should also eliminate the need to have the libvirt
event loop registered when using the embedded QEMU driver.
This has not yet been validated, however, so it is left
for a future patch to relax the constraint.

Daniel P. Berrangé (11):
  qemu: drop support for agent connections on PTYs
  qemu: drop ability to open monitor from FD
  src: set the OS level thread name
  src: improve thread naming with human targetted names
  src: introduce an abstraction for running event loops
  qemu: start/stop an event loop thread for domains
  qemu: start/stop an event thread for QMP probing
  tests: start/stop an event thread for QEMU monitor/agent tests
  qemu: convert monitor to use the per-VM event loop
  qemu: fix variable naming in agent code
  qemu: convert agent to use the per-VM event loop

 po/POTFILES.in  |   1 +
 src/libvirt_private.syms|   6 +
 src/libxl/libxl_domain.c|  10 +-
 src/libxl/libxl_migration.c |  23 +-
 src/lxc/lxc_fuse.c  |   4 +-
 src/node_device/node_device_udev.c  |   7 +-
 src/nwfilter/nwfilter_dhcpsnoop.c   |  11 +-
 src/nwfilter/nwfilter_learnipaddr.c |  10 +-
 src/qemu/qemu_agent.c   | 634 ++--
 src/qemu/qemu_agent.h   |   1 +
 src/qemu/qemu_domain.c  |  33 ++
 src/qemu/qemu_domain.h  |   6 +
 src/qemu/qemu_driver.c  |   3 +-
 src/qemu/qemu_migration.c   |   8 +-
 src/qemu/qemu_monitor.c | 155 +++
 src/qemu/qemu_monitor.h |   8 +-
 src/qemu/qemu_process.c |  61 ++-
 src/qemu/qemu_process.h |   2 +
 src/remote/remote_daemon.c  |   9 +-
 src/rpc/virnetserver.c  |   9 +-
 src/storage/storage_backend_scsi.c  |   4 +-
 src/storage/storage_driver.c|   4 +-
 src/util/Makefile.inc.am|   2 +
 src/util/vircommand.c   |   5 +-
 src/util/vireventthread.c   | 175 
 src/util/vireventthread.h   |  31 ++
 src/util/virfdstream.c  |  10 +-
 src/util/virnodesuspend.c   |   8 +-
 src/util/virthread.c|  44 +-
 src/util/virthread.h|   4 +-
 src/util/virthreadpool.c|  14 +-
 src/util/virthreadpool.h|   2 +-
 tests/qemumonitortestutils.c|  15 +
 33 files changed, 832 insertions(+), 487 deletions(-)
 create mode 100644 src/util/vireventthread.c
 create mode 100644 src/util/vireventthread.h

-- 
2.24.1

Re: [PATCH v3 12/15] tests: qemublock: Add cases for creating image overlays on top of disks with

[Peter Krempa]

On Thu, Feb 13, 2020 at 15:54:53 +0100, Ján Tomko wrote:
> On Wed, Feb 12, 2020 at 07:03:23PM +0100, Peter Krempa wrote:
> > Add a set of test data to see whether the backing store strings are
> > formatted reasonably. Note that we don't support direct creation of such
> > images so those tests are not enabled.
> > 
> > Signed-off-by: Peter Krempa 
> > ---
> > tests/qemublocktest.c |  2 ++
> > .../imagecreate/qcow2-backing-qcow2-slice.json| 15 +++
> > .../imagecreate/qcow2-backing-qcow2-slice.xml |  1 +
> > .../imagecreate/qcow2-backing-raw-slice.json  | 15 +++
> > .../imagecreate/qcow2-backing-raw-slice.xml   |  1 +
> > .../qemublocktestdata/imagecreate/qcow2-slice.xml | 14 ++
> > tests/qemublocktestdata/imagecreate/raw-slice.xml | 14 ++
> > 7 files changed, 62 insertions(+)
> > create mode 100644 
> > tests/qemublocktestdata/imagecreate/qcow2-backing-qcow2-slice.json
> > create mode 12 
> > tests/qemublocktestdata/imagecreate/qcow2-backing-qcow2-slice.xml
> > create mode 100644 
> > tests/qemublocktestdata/imagecreate/qcow2-backing-raw-slice.json
> > create mode 12 
> > tests/qemublocktestdata/imagecreate/qcow2-backing-raw-slice.xml
> > create mode 100644 tests/qemublocktestdata/imagecreate/qcow2-slice.xml
> > create mode 100644 tests/qemublocktestdata/imagecreate/raw-slice.xml
> > 
> > diff --git a/tests/qemublocktestdata/imagecreate/qcow2-slice.xml 
> > b/tests/qemublocktestdata/imagecreate/qcow2-slice.xml
> > new file mode 100644
> > index 00..6c5ae3107b
> > --- /dev/null
> > +++ b/tests/qemublocktestdata/imagecreate/qcow2-slice.xml
> > @@ -0,0 +1,14 @@
> > +
> > +  
> > +  
> > +
> > +  
> > +
> > +
> > +  
> > +
> 
> > +
> 
> This is not parsed.
> 
> > +  
> > +
> > +  
> > +
> > diff --git a/tests/qemublocktestdata/imagecreate/raw-slice.xml 
> > b/tests/qemublocktestdata/imagecreate/raw-slice.xml
> > new file mode 100644
> > index 00..adc7a175ce
> > --- /dev/null
> > +++ b/tests/qemublocktestdata/imagecreate/raw-slice.xml
> > @@ -0,0 +1,14 @@
> > +
> > +  
> > +  
> > +
> > +  
> > +
> > +
> > +  
> > +
> 
> > +
> 
> This is not parsed either.

Both are parsed. These are nodenames of the storage and format layer
when instantiating storage via -blockdev.

> 
> 
> > +  
> 
> Reviewed-by: Ján Tomko 
> 
> Jano

Re: [PATCH v3 06/15] qemuDomainValidateStorageSource: Reject unsupported slices

[Peter Krempa]

On Thu, Feb 13, 2020 at 14:51:20 +0100, Ján Tomko wrote:
> On Wed, Feb 12, 2020 at 07:03:17PM +0100, Peter Krempa wrote:
> > We will currently support slice only for the 'raw' format slice reject
> 
> format slice.
> Reject any other option.

Actually I completely forgot to fix this commit message and would not be
accurate. For the state this commit implemets the commit message should
read:

We support explicit storage slices only when using blockdev. Storage
slices expressed via the backing store string are left to qemu to
open correctly.

Reject storage slices configured via the XML for non-blockdev usage.

Re: [libvirt] [PATCH 0/8] Second take on slirp-helper & dbus-vmstate

[Marc-André Lureau]

Hi

On Tue, Jan 14, 2020 at 2:47 PM  wrote:
>
> From: Marc-André Lureau 
>
> Hi,
>
> The series "[libvirt] [PATCH v2 00/23] Use a slirp helper process" has
> been merged and partially reverted. Meanwhile, qemu dbus-vmstate
> design has been changed and merged upstream.
>
> This new series fixes the slirp-helper support. The significant change
> is that dbus-vmstate now requires a bus (instead of the earlier
> peer-to-peer connection). The current series doesn't attempt to
> enforce strict policies on the bus. As long as you can connect to the
> bus, you can send/receive from/to anyone. A follow-up series should
> implement the recommendations from
> https://qemu.readthedocs.io/en/latest/interop/dbus.html#security.
>
> The libslirp-rs slirp-helper hasn't yet received an official release.
> For testing, you may:
> $ cargo install --features=all --git 
> https://gitlab.freedesktop.org/slirp/libslirp-rs
>
> The resulting binary should be ~/.cargo/bin/slirp-helper, so qemu.conf
> slirp_helper location should be adjusted. With that in place, a VM
> with user networking (slirp) should now start with the helper process.
>
> thanks
>
> Marc-André Lureau (8):
>   qemu: remove dbus-vmstate code
>   qemu-conf: add configurable dbus-daemon location
>   qemu-conf: add dbusStateDir
>   qemu: add a DBus daemon helper unit
>   domain: save/restore the state of dbus-daemon running
>   qemu: prepare and stop the dbus daemon
>   qemu: add dbus-vmstate helper migration support
>   qemu-slirp: register helper for migration

Can I get some feedback/review on the series?

Fwiw, in the past month, we have seen a new implementation emerge
(https://github.com/majek/slirpnetstack) and this made me reconsider a
number of CLI/capabilities things from the spec. I moved the spec on a
wiki for now: 
https://gitlab.freedesktop.org/slirp/libslirp/-/wikis/Slirp-Helper.
We are also discussing with podman developpers about it.

Given that this is not frozen, and no helper was released so far, I
understand that libvirt may want to back off a little. Yet, your
feedback could also help shape/define the helper behaviour!

So please, take a look :)
thanks

>
>  m4/virt-driver-qemu.m4 |   6 +
>  src/qemu/Makefile.inc.am   |   6 +-
>  src/qemu/libvirtd_qemu.aug |   1 +
>  src/qemu/qemu.conf |   3 +
>  src/qemu/qemu_alias.c  |  17 +-
>  src/qemu/qemu_alias.h  |   3 +-
>  src/qemu/qemu_command.c|  65 +++
>  src/qemu/qemu_command.h|   6 +-
>  src/qemu/qemu_conf.c   |   9 +
>  src/qemu/qemu_conf.h   |   2 +
>  src/qemu/qemu_dbus.c   | 283 +
>  src/qemu/qemu_dbus.h   |  30 +--
>  src/qemu/qemu_domain.c |  30 +--
>  src/qemu/qemu_domain.h |   9 +-
>  src/qemu/qemu_extdevice.c  |   4 +-
>  src/qemu/qemu_hotplug.c| 165 +
>  src/qemu/qemu_hotplug.h|  17 +-
>  src/qemu/qemu_migration.c  |  57 +-
>  src/qemu/qemu_monitor.c|  21 +++
>  src/qemu/qemu_monitor.h|   3 +
>  src/qemu/qemu_monitor_json.c   |  15 ++
>  src/qemu/qemu_monitor_json.h   |   5 +
>  src/qemu/qemu_process.c|   6 +
>  src/qemu/qemu_slirp.c  | 126 ++---
>  src/qemu/qemu_slirp.h  |   4 +-
>  src/qemu/test_libvirtd_qemu.aug.in |   1 +
>  tests/Makefile.am  |   1 +
>  27 files changed, 564 insertions(+), 331 deletions(-)
>
> --
> 2.25.0.rc2.1.g09a9a1a997
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list



-- 
Marc-André Lureau

Re: [libvirt PATCH v2 3/8] conf: Introduce VIR_DOMAIN_TIMER_NAME_ARMVTIMER

[Andrea Bolognani]

On Fri, 2020-02-14 at 11:19 +0100, Andrew Jones wrote:
> On Thu, Feb 13, 2020 at 05:30:21PM +0100, Andrea Bolognani wrote:
> > On Thu, 2020-02-13 at 14:26 +0100, Ján Tomko wrote:
> > > Okay, so this name is a libvirt invention.
> > 
> > I believe "ARM virtual timer" is the official name for the device;
> > the name "armvtimer" is an abbreviation that me and Drew (CC'd) have
> > agreed upon, and I understand that's fairly commonly used too.
> > 
> > Drew can confirm whether this is actually the case.
> 
> To be precise the ARM architecture reference manual refers to the CPU's
> builtin timer as the "Generic Timer". All CPUs which the QEMU arm 'virt'
> machine type supports implement the Generic Timer, and all implementations
> of the Generic Timer must include the virtual counter. The virtual counter
> is used to indicate virtual time. In both QEMU and KVM when we want to
> refer to the virtual counter based timer we call it the "vtimer".
> 
> So, while libvirt is indeed inventing the name "armvtimer", I think it's
> appropriate.

There's plenty of precedents for carrying a name used in KVM/QEMU
over to libvirt, so we're good on that front.

> > > And the timer itself is present on all ARM/virt guests and cannot be
> > > disabled, correct?
> > 
> > I believe so but, once again, it'd be better if Drew confirmed it :)
> 
> Correct. While the Generic Timer is an optional feature of the CPU, all
> CPUs that the ARM/virt machine type support have it and it cannot be
> removed.
> 
> So, all ARM/virt guests have vtimers, but not all ARM guests must have
> vtimers. If libvirt can start non-'virt' ARM guests, which have different
> CPUs, then there's a chance that the guest will not have a vtimer. I'm
> not sure if we want to try and incorporate that much specificity
> into the libvirt name, though. I think if a machine/CPU doesn't have a
> vtimer than this XML element should just not be valid.

We're explicitly preventing non-virt guests to configure the timer,
so no problem here.


Since all outstanding questions have been answered, I'm going to push
the series now. Thanks Drew for providing these last nuggets of
information, and of course thanks Jano for the review :)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH v2 3/8] conf: Introduce VIR_DOMAIN_TIMER_NAME_ARMVTIMER

[Andrew Jones]

On Thu, Feb 13, 2020 at 05:30:21PM +0100, Andrea Bolognani wrote:
> On Thu, 2020-02-13 at 14:26 +0100, Ján Tomko wrote:
> > On Fri, Feb 07, 2020 at 03:27:03PM +0100, Andrea Bolognani wrote:
> > > +++ b/src/conf/domain_conf.c
> > > @@ -1063,6 +1063,7 @@ VIR_ENUM_IMPL(virDomainTimerName,
> > >   "tsc",
> > >   "kvmclock",
> > >   "hypervclock",
> > > +  "armvtimer",
> > > );
> > 
> > Okay, so this name is a libvirt invention.
> 
> I believe "ARM virtual timer" is the official name for the device;
> the name "armvtimer" is an abbreviation that me and Drew (CC'd) have
> agreed upon, and I understand that's fairly commonly used too.
> 
> Drew can confirm whether this is actually the case.

To be precise the ARM architecture reference manual refers to the CPU's
builtin timer as the "Generic Timer". All CPUs which the QEMU arm 'virt'
machine type supports implement the Generic Timer, and all implementations
of the Generic Timer must include the virtual counter. The virtual counter
is used to indicate virtual time. In both QEMU and KVM when we want to
refer to the virtual counter based timer we call it the "vtimer".

So, while libvirt is indeed inventing the name "armvtimer", I think it's
appropriate.

> 
> > And the timer itself is present on all ARM/virt guests and cannot be
> > disabled, correct?
> 
> I believe so but, once again, it'd be better if Drew confirmed it :)

Correct. While the Generic Timer is an optional feature of the CPU, all
CPUs that the ARM/virt machine type support have it and it cannot be
removed.

So, all ARM/virt guests have vtimers, but not all ARM guests must have
vtimers. If libvirt can start non-'virt' ARM guests, which have different
CPUs, then there's a chance that the guest will not have a vtimer. I'm
not sure if we want to try and incorporate that much specificity
into the libvirt name, though. I think if a machine/CPU doesn't have a
vtimer than this XML element should just not be valid.

Thanks,
drew

Re: [libvirt PATCH v2 4/8] qemu: Validate configuration for the armvtimer timer

[Ján Tomko]

On Thu, Feb 13, 2020 at 05:45:14PM +0100, Andrea Bolognani wrote:

On Thu, 2020-02-13 at 14:28 +0100, Ján Tomko wrote:

On Fri, Feb 07, 2020 at 03:27:04PM +0100, Andrea Bolognani wrote:
> +++ b/src/qemu/qemu_domain.c
> @@ -5430,6 +5430,39 @@ qemuDomainDefValidateClockTimers(const virDomainDef 
*def,
> break;
>
> case VIR_DOMAIN_TIMER_NAME_ARMVTIMER:

Missing check for present == 0.


Good catch! Assuming Drew confirms the timer can't be disabled, then
we should definitely error out for present='no'.

Do you want me to respin, or can I just squash in the diff below?


diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 29ec203413..143ddc508e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5441,6 +5441,12 @@ qemuDomainDefValidateClockTimers(const virDomainDef *def,
   def->os.machine);
return -1;
}
+if (timer->present == 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("The '%s' timer can't be disabled"),
+   virDomainTimerNameTypeToString(timer->name));
+return -1;
+}
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_CPU_KVM_NO_ADJVTIME)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
   _("Configuring the '%s' timer is not supported "


Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature