Re: [libvirt PATCHv4 10/15] qemu: forbid migration with vhost-user-fs device

[Peter Krempa]

On Thu, Feb 20, 2020 at 15:32:47 +0100, Ján Tomko wrote:
> This is not yet supported.
> 
> Signed-off-by: Ján Tomko 
> Reviewed-by: Daniel P. Berrangé 
> ---
>  src/qemu/qemu_migration.c | 10 ++
>  1 file changed, 10 insertions(+)

Reviewed-by: Peter Krempa 

Re: [libvirt PATCHv4 09/15] qemu: validate virtiofs filesystems

[Peter Krempa]

On Thu, Feb 20, 2020 at 15:32:46 +0100, Ján Tomko wrote:
> Reject unsupported configurations.
> 
> Signed-off-by: Ján Tomko 
> ---
>  src/qemu/qemu_domain.c | 61 +++---
>  1 file changed, 58 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index c3fc3fed1c..7cb283123d 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -8266,10 +8266,44 @@ qemuDomainDeviceDefValidateIOMMU(const 
> virDomainIOMMUDef *iommu,
>  return 0;
>  }
>  
> +static int
> +qemuDomainDefValidateVirtioFSSharedMemory(const virDomainDef *def)
> +{
> +size_t numa_nodes = virDomainNumaGetNodeCount(def->numa);
> +size_t i;
> +
> +for (i = 0; i < numa_nodes; i++) {

This won't catch guests with no numa configured ...

> +virDomainMemoryAccess node_access =
> +virDomainNumaGetNodeMemoryAccessMode(def->numa, i);
> +
> +switch (node_access) {
> +case VIR_DOMAIN_MEMORY_ACCESS_DEFAULT:
> +if (def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_SHARED) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("virtiofs requires shared memory"));

... so this error won't be reported.

Also must all nodes have shared memory? Isn't one enough?

> +return -1;
> +}
> +break;
> +case VIR_DOMAIN_MEMORY_ACCESS_SHARED:
> +break;
> +case VIR_DOMAIN_MEMORY_ACCESS_PRIVATE:
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("virtiofs requires shared memory"));
> +return -1;
> +
> +case VIR_DOMAIN_MEMORY_ACCESS_LAST:
> +default:
> +virReportEnumRangeError(virDomainMemoryAccess, node_access);
> +return -1;
> +
> +}
> +}
> +return 0;
> +}

Re: [libvirt PATCHv4 08/15] qemu: add virtiofsd_debug to qemu.conf

[Peter Krempa]

On Thu, Feb 20, 2020 at 15:32:45 +0100, Ján Tomko wrote:
> Add a 'virtiofsd_debug' option for tuning whether to run virtiofsd
> in debug mode.
> 
> Signed-off-by: Ján Tomko 
> Reviewed-by: Daniel P. Berrangé 
> ---
>  src/qemu/libvirtd_qemu.aug | 1 +
>  src/qemu/qemu.conf | 7 +++
>  src/qemu/qemu_conf.c   | 2 ++
>  src/qemu/qemu_conf.h   | 1 +
>  src/qemu/test_libvirtd_qemu.aug.in | 1 +
>  5 files changed, 12 insertions(+)

[...]

> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> index b6805ffc41..e82c1b5bd5 100644
> --- a/src/qemu/qemu.conf
> +++ b/src/qemu/qemu.conf
> @@ -809,6 +809,13 @@
>  #
>  #gluster_debug_level = 9
>  
> +# virtiofsd debug
> +#
> +# Whether to enable the debugging output of the virtiofsd daemon.
> +# Possible values are 0 or 1.

Please mention that the default is 'disabled' or 0.

> +#
> +#virtiofsd_debug = 1
> +
>  # To enhance security, QEMU driver is capable of creating private namespaces
>  # for each domain started. Well, so far only "mount" namespace is supported. 
> If
>  # enabled it means qemu process is unable to see all the devices on the 
> system,

Reviewed-by: Peter Krempa 

Re: [libvirt PATCHv4 07/15] conf: add virtiofs-related elements and attributes

[Peter Krempa]

On Thu, Feb 20, 2020 at 15:32:44 +0100, Ján Tomko wrote:
> Add more elements for tuning the virtiofsd daemon
> and the vhost-user-fs device:
> 
>   
> 
>   
>   
> 
>   
> 
> Signed-off-by: Ján Tomko 
> Reviewed-by: Daniel P. Berrangé 
> Reviewed-by: Masayoshi Mizuma 
> ---
>  docs/formatdomain.html.in |  25 +++-
>  docs/schemas/domaincommon.rng |  48 
>  src/conf/domain_conf.c| 107 +-
>  src/conf/domain_conf.h|  15 +++
>  src/libvirt_private.syms  |   1 +
>  .../vhost-user-fs-fd-memory.xml   |   6 +-
>  .../vhost-user-fs-hugepages.xml   |   1 +
>  7 files changed, 200 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index dab8fb8f6b..7c4153c7ce 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -3936,10 +3936,15 @@
>  
>
>
> -  
> +  
> +  
> + 
> + 
> +  
>
>
>
> +

Spurious whitespace?

>...
>  
>  ...
> @@ -4063,9 +4068,27 @@
>Virtio-specific options can also be
>set. (Since 3.5.0)
>
> +  
> +For virtiofs, the queue attribute can 
> be used
> +to specify the queue size (i.e. how many requests can the queue 
> fit).
> +(Since 6.1.0)

Version.

> +  
>  
>
>  
> +  binary
> +  
> +The optional binary element can tune the options for 
> virtiofsd.

I think you should state that any of the following attributes/elements
are optional, so that it's obvious that e.g. 'path' can be omitted if
the user wishes to configure locking.

> +The attribute path can be used to override the path to 
> the daemon.
> +Attribute xattr enables the use of filesystem extended 
> attributes.
> +Caching can be tuned via the cache element, possible 
> mode
> +values being none and always.
> +Locking can be controlled via the lock
> +element - attributes posix and flock both 
> accepting
> +values yes or no.
> +(Since 6.1.0)

Version.

> +  
> +
>source
>
>  The resource on the host that is being accessed in the guest. The

[...]

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index d78ea92ead..8e7400294b 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c

[...]

> @@ -11311,6 +11320,64 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
>  }
>  }
>  
> +if (def->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS) {
> +g_autofree char *queue_size = 
> virXPathString("string(./driver/@queue)", ctxt);
> +g_autofree char *binary = virXPathString("string(./binary/@path)", 
> ctxt);
> +g_autofree char *xattr = virXPathString("string(./binary/@xattr)", 
> ctxt);
> +g_autofree char *cache = 
> virXPathString("string(./binary/cache/@mode)", ctxt);
> +g_autofree char *posix_lock = 
> virXPathString("string(./binary/lock/@posix)", ctxt);
> +g_autofree char *flock = 
> virXPathString("string(./binary/lock/@flock)", ctxt);
> +int val;
> +
> +

Too many newlines.

> +if (queue_size && virStrToLong_ull(queue_size, NULL, 10, 
> &def->queue_size) < 0) {
> +virReportError(VIR_ERR_XML_ERROR,
> +   _("cannot parse queue size '%s' for virtiofs"),
> +   queue_size);
> +goto error;

[...]

> @@ -25142,6 +25209,9 @@ virDomainFSDefFormat(virBufferPtr buf,
>  const char *wrpolicy = virDomainFSWrpolicyTypeToString(def->wrpolicy);
>  const char *src = def->src->path;
>  g_auto(virBuffer) driverAttrBuf = VIR_BUFFER_INITIALIZER;
> +g_auto(virBuffer) driverBuf = VIR_BUFFER_INIT_CHILD(buf);
> +g_auto(virBuffer) binaryAttrBuf = VIR_BUFFER_INITIALIZER;
> +g_auto(virBuffer) binaryBuf = VIR_BUFFER_INIT_CHILD(buf);
>  
>  if (!type) {
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -25165,6 +25235,8 @@ virDomainFSDefFormat(virBufferPtr buf,
>  virBufferAddLit(buf, ">\n");
>  
>  virBufferAdjustIndent(buf, 2);
> +virBufferAdjustIndent(&driverBuf, 2);
> +virBufferAdjustIndent(&binaryBuf, 2);

Eww. Something is wrong if you need to tweak the indentation after using
VIR_BUFFER_INIT_CHILD.

>  if (def->fsdriver) {
>  virBufferAsprintf(&driverAttrBuf, " type='%s'", fsdriver);
>  
> @@ -25176,11 +25248,44 @@ virDomainFSDefFormat(virBufferPtr buf,
>  if (def->wrpolicy)
>  virBuff

Re: [libvirt PATCHv4 06/15] conf: qemu: add virtiofs fsdriver type

[Peter Krempa]

On Thu, Feb 20, 2020 at 15:32:43 +0100, Ján Tomko wrote:
> Introduce a new 'virtiofs' driver type for filesystem.
> 
> 
>   
>   
>   
>   
> 
> 
> Signed-off-by: Ján Tomko 
> Reviewed-by: Daniel P. Berrangé 
> ---

[...]

> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index f4af65f13f..dab8fb8f6b 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
[...]

> @@ -3963,6 +3968,9 @@
>  while the value immediate means that a host writeback
>  is immediately triggered for all pages touched during a guest file
>  write operation (since 0.9.10).
> +Since 6.1.0, type='virtiofs'

Might require update now.

> +is also supported. Using virtiofs requires setting up shared memory,
> +see the guide: Virtio-FS
>  
>  template
>  

Reviewed-by: Peter Krempa 

Re: [libvirt PATCH 5/6] Make PATHs unique for a VM object instance

[Shaju Abraham]

On Tue, Feb 25, 2020 at 6:29 PM Daniel P. Berrangé 
wrote:

> On Wed, Feb 19, 2020 at 02:39:22AM +, Shaju Abraham wrote:
> >
> >
> > On 2/11/20, 7:06 PM, "Daniel P. Berrangé"  wrote:
> >
> > On Tue, Feb 11, 2020 at 10:05:53AM +0100, Martin Kletzander wrote:
> >   >  > On Wed, Feb 05, 2020 at 05:32:50PM +, Daniel P. Berrangé
> wrote:
> > > > > On Mon, Feb 03, 2020 at 12:43:32PM +, Daniel P. Berrangé
> wrote:
> > > > > > From: Shaju Abraham 
> > > > > >
> > > > > There are various config paths that a VM uses. The monitor
> paths and
> > > > > > other lib paths are examples. These paths are tied to the VM
> name or
> > > > > > UUID. The local migration breaks the assumption that there
> will be only
> > > > > > one VM with a unique UUID and name. During local migrations
> there can be
> > > > > > multiple VMs with same name and UUID in the same host.
> Append the
> > > > > > domain-id field to the path so that there is no duplication
> of path
> > > > > names.
> > > > >
> > > > >This is the really critical problem with localhost migration.
> > > > >
> > > > >Appending the domain-id looks "simple" but this is a significant
> > > > >behavioural / functional change for applications, and I don't
> think
> > > > >it can fully solve the problem either.
> > > > >
> > > > >This is changing thue paths used in various places where libvirt
> > > > >internally generates unique paths (eg QMP socket, huge page or
> > > > >file based memory paths, and defaults used for auto-filling
> > > > >device paths ( if not specified).
> > > > >
> > > > >Some of these paths are functionally important to external
> > > > >applications and cannot be changed in this way. eg stuff
> > > > >integrating with QEMU can be expecting certain memory backing
> > > > >file paths, or certain  paths & is liable to break
> > > > >if we change the naming convention.
> > > > >
> > > > >For sake of argument, lets assume we can changing the naming
> > > > >convention without breaking anything...
> > > > >
> > > >
> > > >This was already done in (I would say) most places as they use
> > > >virDomainDefGetShortName() to get a short, unique name of a
> directory -- it uses
> > > >the domain ID as a prefix.
> > > >
> > > > > This only applies to paths libvirt generates at VM startup.
> > > > >
> > > > >There are plenty of configuration elements in the guest XML
> > > > >that are end user / mgmt app defined, and reference paths in
> > > > >the host OS.
> > > > >
> > > > >For example , , , ,
> > > > >all support UNIX domain sockets and TCP sockets. A UNIX
> > > > >domain socket cannot be listened on by multiple VMs
> > > > >at once. If the UNIX socket is in client mode, we cannot
> > > > >assume the thing QEMU is connecting to allows multiple
> > > > >concurrent connections. eg 2 QEMU's could have their
> > > > > connected together over a UNIX socket pair.
> > > > >Similarly if automatic TCP port assignment is not used
> > > > >we cannot have multiple QEMU's listening on the same
> > > > >host.
> > > > >
> > > > >One answer is to say that localhost migration is just
> > > > >not supported for such VMs, but I don't find that very
> > > > >convincing because the UNIX domain socket configs
> > > > >affected are in common use.
> > > > >
> > > >
> > > >I would be okay with saying that these either need to be changed
> in a provided
> > > >destination XML or the migration will probably break.  I do not
> think it is
> > > >unreasonable to say that if users are trying to shoot themselves,
> we should not
> > > >spend a ridiculous time just so we can prevent that.  Otherwise
> we will get to
> > > >the same point as we are now -- there might be a case where a
> local migration
> > > >would work, but users cannot execute it even if they were very
> cautious and went
> > > >through all the things that can prevent it from the technical
> point of view,
> > > >libvirt will still disallow that.
> >
> > >If there are clashing resources, we can't rely on QEMU reporting an
> > >error. For example with a UNIX domain socket, the first thing QEMU
> > >does is unlink(/socket/path), which will blow away the UNIX domain
> > >socket belonging to the original QEMU. As a result if migration
> > >fails, and we rollback, the original QEMU will be broken.
> >
> >By appending the id field to the path, we are effectively nullifying
> this particular
> >concern. Each qemu instance will get its own unique path and monitor.
> If a migration
> >fails, we can roll back.
>
> No, you've not nullified the problem. This only helps the cases where
> libvirt generates the path. This is only a subset of affected cases.
> Just one example:
>
>
>
> there are many other parts of the domain 

[PATCH] docs: document port isolated property in domain/network/networkport

[Laine Stump]

Signed-off-by: Laine Stump 
---

I had thought I'd included documentation with the patch that added
parsing/formatting for this, but after crobinso noticed it was
missing, I realized that I had only put documentation in an earlier
version of the patches (that put the option inside
). Oops :-/


 docs/formatdomain.html.in  | 31 +++
 docs/formatnetwork.html.in | 25 +
 docs/formatnetworkport.html.in | 11 +++
 3 files changed, 67 insertions(+)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 4fef2a0a97..28770188dd 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -6539,6 +6539,37 @@ qemu-kvm -net nic,model=? /dev/null
   traffic for that VLAN will be tagged.
 
 
+Isolating guests's network traffic from each 
other
+
+
+...
+
+  
+
+
+  
+
+...
+
+
+  Since 6.1.0. The port
+  element property isolated, when set
+  to yes (default setting is no) is used
+  to isolate this interface's network traffic from that of other
+  guest interfaces connected to the same network that also
+  have .  This setting is
+  only supported for emulated interface devices that use a
+  standard tap device to connect to the network via a Linux host
+  bridge. This property can be inherited from a libvirt network,
+  so if all guests that will be connected to the network should be
+  isolated, it is better to put the setting in the network
+  configuration. (NB: this only prevents guests that
+  have isolated='yes' from communicating with each
+  other; if there is a guest on the same bridge that doesn't
+  have isolated='yes', even the isolated guests will
+  be able to communicate with it.)
+
+
 Modifying virtual link state
 
 ...
diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
index 3d807ecab6..f1e7ce5e4e 100644
--- a/docs/formatnetwork.html.in
+++ b/docs/formatnetwork.html.in
@@ -729,6 +729,31 @@
   or .
 
 
+Isolating ports from one another
+
+
+
+  isolated-ports
+  
+  
+  
+
+
+
+
+  Since 6.1.0. The port
+  element property isolated, when set
+  to yes (default setting is no) is used
+  to isolate the network traffic of each guest on the network from
+  all other guests connected to the network; it does not have an
+  effect on communication between the guests and the host, or
+  between the guests and destinations beyond this network. This
+  setting is only supported for networks that use a Linux host
+  bridge to connect guest interfaces via a standard tap device
+  (i.e. those with a forward mode of nat, route, open, bridge, or
+  no forward mode).
+
+
 Portgroups
 
 
diff --git a/docs/formatnetworkport.html.in b/docs/formatnetworkport.html.in
index 0425e069ce..199a05f929 100644
--- a/docs/formatnetworkport.html.in
+++ b/docs/formatnetworkport.html.in
@@ -84,6 +84,7 @@
 
   
   
+  
   
 
   
@@ -110,6 +111,16 @@
 only supported for the virtio device model and for macvtap
 connections on the host.
   
+  port
+   Since 6.1.0.
+The port element property
+isolated, when set to yes (default
+setting is no) is used to isolate this port's
+network traffic from other ports on the same network that also
+have . This setting
+is only supported for emulated network devices connected to a
+Linux host bridge via a standard tap device.
+  
   virtualport
   The virtualport element describes metadata that
 needs to be provided to the underlying network subsystem. It
-- 
2.24.1

Re: [PATCH] network: bridge_driver: Use new helpers for storing libvirt errors

[Laine Stump]

On 2/25/20 1:10 PM, Gaurav Agrawal wrote:

Hi Laine,

I am sure that I did --subject-prefix , not sure why it did not landed.

Now am wondering about this situation do I still need a PATCH-3 or it's 
handled ?


Nah, Jano pointed out what I thought of late last night - this is 
different from the traditional use cases that the task-list wanted to 
have removed - it saves the error to re-use it multiple times in the 
future, so  virErrorRestore() would actually do the *wrong* thing here. 
And since virErrorPreserveLast() is intended to be used in a pair with 
virErrorRestore(), there is actually nothing to do here.


And even beyond that, when I looked at the list of remaining cases of 
virSaveLastError() for more than the 10 seconds I looked last night, I 
see (as Jano pointed out) that all the cases that should be converted, 
already *were* converted.


The only problem is that the person who did the converted didn't know 
that was a task on the bite-sized tasks list, so they didn't remove it.


Jano has now updated the task list to remove stale entries. So maybe you 
can find something else on that list to fix.


Sorry for sending you through all this. On the upside, your .gitconfig 
is now in order :-)





Thanks for giving your one last pass!

Best Regards
Gaurav

On Tue, Feb 25, 2020, 22:46 Laine Stump > wrote:


On 2/25/20 7:37 AM, Ján Tomko wrote:
 > On Mon, Feb 24, 2020 at 10:09:05PM -0500, Laine Stump wrote:
 >> On 2/24/20 1:58 PM, Gaurav Agrawal wrote:
 >>
 >
 > Yes, networkSetupPrivateChains is only called once (via virOnce, as
 > suggested by the comment on the top of the function) on
initialization
 > and if either IPv4 or IPv6 chains could not be created, it sets the
 > dirver-global error, which is then called on any subsequent attempt
 > to use it.
 >
 > So this is not really a case that needs to be converted.
 > In fact, glancing at git gre virSetError it seems we already got rid
 > of all the ones worth converting.


I could have sworn that when I looked last night there were a
handful of
virSaveLastError() calls, and that I looked at one that was paired with
virSetError(). But when I look now I see that all the
virSaveLastError()
calls remaining are strange "save this for later" type things rather
than "save this for a second while we clean up the mess".


It looks like jferlan pushed a bunch of patches in Dec 2018 to do all
the valid replacements, but the item was never removed from the
bite-sized tasks list (he might have fixed them without even knowing
about the item on the list). I just went to the wiki to remove it and
see that Jano has already taken care of it!


 >
 >
 >>
 >> Your patch has replaced the virSaveLastError() of the earlier part
 >> with virErrorPreserveLast(), but hasn't replaced the
virSetError() of
 >> the later part (which is down in networkAddFirewallRules()) with
 >> virErrorRestore().
 >
 > virErrorRestore resets the error, which is not what we want here -
 > any subsequent calls should report the same error we caught when
 > initializing.


Yeah, I realized that was probably the case in the middle of the night
last night, but wasn't at the keyboard to chastise myself. I knew I
should have just gone to bed instead of sitting down for one last
pass...


But anyway the upside is that Guarav got git send-email configured
properly to send future patches (while we're on the topic of workflow -
when you send a modified/updated version of a patch, be sure to note
that in the subject, e.g. with "--subject-prefix="PATCHv2").

Plans for next release

[Daniel Veillard]

  I'm a bit late, but it seems that if we freeze tomorrow, then RC2
could be provided on Friday and if everythig looks fine we can push 6.1.0
on Monday 2nd,

   so that's the plan, I hope this works for everyone !

Daniel

-- 
Daniel Veillard  | Red Hat Developers Tools http://developer.redhat.com/
veill...@redhat.com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | virtualization library  http://libvirt.org/

Re: [PATCH] network: bridge_driver: Use new helpers for storing libvirt errors

[Gaurav Agrawal]

Hi Laine,

I am sure that I did --subject-prefix , not sure why it did not landed.

Now am wondering about this situation do I still need a PATCH-3 or it's
handled ?

Thanks for giving your one last pass!

Best Regards
Gaurav

On Tue, Feb 25, 2020, 22:46 Laine Stump  wrote:

> On 2/25/20 7:37 AM, Ján Tomko wrote:
> > On Mon, Feb 24, 2020 at 10:09:05PM -0500, Laine Stump wrote:
> >> On 2/24/20 1:58 PM, Gaurav Agrawal wrote:
> >>
> >
> > Yes, networkSetupPrivateChains is only called once (via virOnce, as
> > suggested by the comment on the top of the function) on initialization
> > and if either IPv4 or IPv6 chains could not be created, it sets the
> > dirver-global error, which is then called on any subsequent attempt
> > to use it.
> >
> > So this is not really a case that needs to be converted.
> > In fact, glancing at git gre virSetError it seems we already got rid
> > of all the ones worth converting.
>
>
> I could have sworn that when I looked last night there were a handful of
> virSaveLastError() calls, and that I looked at one that was paired with
> virSetError(). But when I look now I see that all the virSaveLastError()
> calls remaining are strange "save this for later" type things rather
> than "save this for a second while we clean up the mess".
>
>
> It looks like jferlan pushed a bunch of patches in Dec 2018 to do all
> the valid replacements, but the item was never removed from the
> bite-sized tasks list (he might have fixed them without even knowing
> about the item on the list). I just went to the wiki to remove it and
> see that Jano has already taken care of it!
>
>
> >
> >
> >>
> >> Your patch has replaced the virSaveLastError() of the earlier part
> >> with virErrorPreserveLast(), but hasn't replaced the virSetError() of
> >> the later part (which is down in networkAddFirewallRules()) with
> >> virErrorRestore().
> >
> > virErrorRestore resets the error, which is not what we want here -
> > any subsequent calls should report the same error we caught when
> > initializing.
>
>
> Yeah, I realized that was probably the case in the middle of the night
> last night, but wasn't at the keyboard to chastise myself. I knew I
> should have just gone to bed instead of sitting down for one last pass...
>
>
> But anyway the upside is that Guarav got git send-email configured
> properly to send future patches (while we're on the topic of workflow -
> when you send a modified/updated version of a patch, be sure to note
> that in the subject, e.g. with "--subject-prefix="PATCHv2").
>
>
>
>

Re: [PATCH] network: bridge_driver: Use new helpers for storing libvirt errors

[Laine Stump]

On 2/25/20 7:37 AM, Ján Tomko wrote:

On Mon, Feb 24, 2020 at 10:09:05PM -0500, Laine Stump wrote:

On 2/24/20 1:58 PM, Gaurav Agrawal wrote:



Yes, networkSetupPrivateChains is only called once (via virOnce, as
suggested by the comment on the top of the function) on initialization
and if either IPv4 or IPv6 chains could not be created, it sets the
dirver-global error, which is then called on any subsequent attempt
to use it.

So this is not really a case that needs to be converted.
In fact, glancing at git gre virSetError it seems we already got rid
of all the ones worth converting.



I could have sworn that when I looked last night there were a handful of 
virSaveLastError() calls, and that I looked at one that was paired with 
virSetError(). But when I look now I see that all the virSaveLastError() 
calls remaining are strange "save this for later" type things rather 
than "save this for a second while we clean up the mess".



It looks like jferlan pushed a bunch of patches in Dec 2018 to do all 
the valid replacements, but the item was never removed from the 
bite-sized tasks list (he might have fixed them without even knowing 
about the item on the list). I just went to the wiki to remove it and 
see that Jano has already taken care of it!








Your patch has replaced the virSaveLastError() of the earlier part 
with virErrorPreserveLast(), but hasn't replaced the virSetError() of 
the later part (which is down in networkAddFirewallRules()) with 
virErrorRestore().


virErrorRestore resets the error, which is not what we want here -
any subsequent calls should report the same error we caught when
initializing.



Yeah, I realized that was probably the case in the middle of the night 
last night, but wasn't at the keyboard to chastise myself. I knew I 
should have just gone to bed instead of sitting down for one last pass...



But anyway the upside is that Guarav got git send-email configured 
properly to send future patches (while we're on the topic of workflow - 
when you send a modified/updated version of a patch, be sure to note 
that in the subject, e.g. with "--subject-prefix="PATCHv2").

Re: [libvirt PATCH 2/8] bridge: include netdev_bandwidth_conf.h

[Ján Tomko]

On Tue, Feb 25, 2020 at 04:37:28PM +0100, Michal Privoznik wrote:

On 2/22/20 5:31 PM, Ján Tomko wrote:

This file uses the virNetDevBandwidth*Floor helpers
without including the correct include,
relying on virnetworkportdef.h to include it.

Signed-off-by: Ján Tomko 
Fixes: 17f430eb5cfaaa3388077f2b0856f011f0b2a4c3


I have a question about this "Fixes" field.


Feel free to ask it ;)

I've noticed you put it 
into some commit messages, but I can't figure out the pattern. I 
thought that you are blaming commits from previous releases (to help 
distro maintainers), but the commit you are referencing here is in the 
same release as this one. And some commits in this series don't have 
the field at all.


It is to leave a trace for future readers.

The pattern is - I put it in in cases where I was bothered to look for
it - here it was because rebasing on master actually broke the build
for me. For other bugs, I'm usually curious how long that has been
broken or in which releases we need to fix this.

Of course,
a) the wording "Fixes" in this case is similar to using "Resolves"
for a related bugzilla link - Fixes somehow implies it was "broken".
We cannot really consider a patch that complies correctly broken.
The more accurate wording would be 'I-would-have-squashed-this-into:'
b) I assume nobody is going to backport an include cleanup, so it is
not useful in that regard either.

Hope this answers your untold question.

Jano



Michal



signature.asc
Description: PGP signature

[libvirt PATCH 1/2] conf: only allow virtio bus for input passthrough

[Ján Tomko]

Other buses are not supported.

Signed-off-by: Ján Tomko 
https://bugzilla.redhat.com/show_bug.cgi?id=1724928
---
 src/conf/domain_conf.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 17867eeece..e7d370e9c6 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6610,6 +6610,12 @@ virDomainInputDefValidate(const virDomainInputDef *input)
 break;
 
 case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH:
+if (input->bus != VIR_DOMAIN_INPUT_BUS_VIRTIO) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("only bus 'virtio' is supported for 
'passthrough' "
+ "input devices"));
+return -1;
+}
 break;
 
 case VIR_DOMAIN_INPUT_TYPE_LAST:
-- 
2.24.1

[libvirt PATCH 0/2] conf: only allow virtio bus for input passthrough

[Ján Tomko]

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

Ján Tomko (2):
  conf: only allow virtio bus for input passthrough
  conf: default to virtio bus for input passthrough

 src/conf/domain_conf.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

-- 
2.24.1

[libvirt PATCH 2/2] conf: default to virtio bus for input passthrough

[Ján Tomko]

Other buses are not supported.

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

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e7d370e9c6..ef5809ee54 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13610,7 +13610,8 @@ virDomainInputDefParseXML(virDomainXMLOptionPtr xmlopt,
 def->type == VIR_DOMAIN_INPUT_TYPE_KBD) &&
 (ARCH_IS_X86(dom->os.arch) || dom->os.arch == VIR_ARCH_NONE)) {
 def->bus = VIR_DOMAIN_INPUT_BUS_PS2;
-} else if (ARCH_IS_S390(dom->os.arch)) {
+} else if (ARCH_IS_S390(dom->os.arch) ||
+   def->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH) {
 def->bus = VIR_DOMAIN_INPUT_BUS_VIRTIO;
 } else {
 def->bus = VIR_DOMAIN_INPUT_BUS_USB;
-- 
2.24.1

Re: [libvirt PATCH] qemu: Do not set default CPU for archs without CPU driver

[Daniel P . Berrangé]

On Tue, Feb 25, 2020 at 04:45:50PM +0100, Jiri Denemark wrote:
> Whenever there is a guest CPU configured in domain XML, we will call
> some CPU driver APIs to validate the CPU definition and check its
> compatibility with the hypervisor. Thus domains with guest CPU
> specification can only be started if the guest architecture is supported
> by the CPU driver. But we would add a default CPU to any domain as long
> as QEMU reports it causing failures to start any domain on affected
> architectures.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1805755
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/cpu/cpu.c| 24 
>  src/cpu/cpu.h|  3 +++
>  src/libvirt_private.syms |  1 +
>  src/qemu/qemu_domain.c   |  3 +++
>  4 files changed, 31 insertions(+)

Reviewed-by: Daniel P. Berrangé 


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

[libvirt PATCH] docs: fix docs about bandwidth setting with bridge networks

[Daniel P . Berrangé]

We now support setting bandwidth on networks with type bridge.

Signed-off-by: Daniel P. Berrangé 
---
 docs/formatnetwork.html.in | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
index 3d807ecab6..ec055c8360 100644
--- a/docs/formatnetwork.html.in
+++ b/docs/formatnetwork.html.in
@@ -548,10 +548,10 @@
 (since 0.9.4). Setting
 bandwidth for a network is supported only
 for networks with a  mode
-of route, nat, or no mode at all
-(i.e. an "isolated" network). Setting bandwidth
-is not supported for forward modes
-of bridge, passthrough, private,
+of route, nat, bridge,
+or no mode at all (i.e. an "isolated" network). Setting
+bandwidth is not supported for forward modes
+passthrough, private,
 or hostdev. Attempts to do this will lead to
 a failure to define the network or to create a transient network.
   
-- 
2.24.1

[libvirt PATCH] qemu: Do not set default CPU for archs without CPU driver

[Jiri Denemark]

Whenever there is a guest CPU configured in domain XML, we will call
some CPU driver APIs to validate the CPU definition and check its
compatibility with the hypervisor. Thus domains with guest CPU
specification can only be started if the guest architecture is supported
by the CPU driver. But we would add a default CPU to any domain as long
as QEMU reports it causing failures to start any domain on affected
architectures.

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

Signed-off-by: Jiri Denemark 
---
 src/cpu/cpu.c| 24 
 src/cpu/cpu.h|  3 +++
 src/libvirt_private.syms |  1 +
 src/qemu/qemu_domain.c   |  3 +++
 4 files changed, 31 insertions(+)

diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c
index ae3a0acc10..6d6191fe4e 100644
--- a/src/cpu/cpu.c
+++ b/src/cpu/cpu.c
@@ -1096,3 +1096,27 @@ virCPUDataAddFeature(virCPUDataPtr cpuData,
 
 return driver->dataAddFeature(cpuData, name);
 }
+
+
+/**
+ * virCPUArchIsSupported:
+ *
+ * @arch: CPU architecture
+ *
+ * Returns true if the architecture is supported by any CPU driver.
+ */
+bool
+virCPUArchIsSupported(virArch arch)
+{
+size_t i;
+size_t j;
+
+for (i = 0; i < G_N_ELEMENTS(drivers); i++) {
+for (j = 0; j < drivers[i]->narch; j++) {
+if (arch == drivers[i]->arch[j])
+return true;
+}
+}
+
+return false;
+}
diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h
index 2e8b8923ae..f779d2be17 100644
--- a/src/cpu/cpu.h
+++ b/src/cpu/cpu.h
@@ -265,6 +265,9 @@ int
 virCPUDataAddFeature(virCPUDataPtr cpuData,
  const char *name);
 
+bool
+virCPUArchIsSupported(virArch arch);
+
 /* virCPUDataFormat and virCPUDataParse are implemented for unit tests only and
  * have no real-life usage
  */
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 9d172d3bd0..e27b6f29bc 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1309,6 +1309,7 @@ virStoragePoolObjVolumeListExport;
 # cpu/cpu.h
 cpuDecode;
 cpuEncode;
+virCPUArchIsSupported;
 virCPUBaseline;
 virCPUCheckFeature;
 virCPUCompare;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 446a517df9..7d274a4fa5 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4576,6 +4576,9 @@ qemuDomainDefSetDefaultCPU(virDomainDefPtr def,
  def->cpu->model))
 return 0;
 
+if (!virCPUArchIsSupported(def->os.arch))
+return 0;
+
 /* Default CPU model info from QEMU is usable for TCG only except for
  * x86, s390, and ppc64. */
 if (!ARCH_IS_X86(def->os.arch) &&
-- 
2.25.1

Re: [libvirt PATCH 0/8] Include networkportdef.h in domain_conf.h

[Michal Privoznik]

On 2/22/20 5:31 PM, Ján Tomko wrote:

The netdev_bandwidth_conf module contains
XML parsing and formatting functions operating
on types from util/virnetdevbandwidth.h
as well as helper functions using types
from domain_conf.h and network_conf.h

It does not, however, introduce any new types,
so there's no need to include its header in
other header files.

Move its inclusion in networkportdef.h to the
corresponding networkportdef.c file, where it's
used, which clears the path for networkportdef.h
inclusion in domain_conf.h.

Patch 1 is unrelated;

Patch 5 was intended to help remove the dependency
of the header file on network_conf.h (by passing int
instead of enum) and patch 6 would lessen the
dependency from domain_conf.h to virconftypes.h,
but later I realized this might not be necessary.
(Thanks, Pavel!)

Ján Tomko (8):
   conf: virnwfilterbindingdef: include virxml.h
   bridge: include netdev_bandwidth_conf.h
   conf: virnetworkportdef: include virnetdevmacvlan
   conf: rename virNetDevSupportBandwidth to virNetDevSupportsBandwidth
   conf: virNetDevSupportsBandwidth: move into the C file
   conf: do not pass vm object to virDomainClearNetBandwidth
   conf: reduce includes in virnetworkportdef.h
   conf: include virnetworkportdef.h in domain_conf.h

  src/conf/domain_conf.h   |  6 +-
  src/conf/netdev_bandwidth_conf.c | 35 ++--
  src/conf/netdev_bandwidth_conf.h | 26 ++--
  src/conf/virnetworkportdef.c |  5 +
  src/conf/virnetworkportdef.h |  3 ---
  src/conf/virnwfilterbindingdef.h |  1 +
  src/libvirt_private.syms |  1 +
  src/lxc/lxc_driver.c |  4 ++--
  src/lxc/lxc_process.c|  2 +-
  src/network/bridge_driver.c  |  2 ++
  src/qemu/qemu_command.c  |  2 +-
  src/qemu/qemu_driver.c   |  4 ++--
  src/qemu/qemu_hotplug.c  |  4 ++--
  src/qemu/qemu_process.c  |  2 +-
  14 files changed, 50 insertions(+), 47 deletions(-)



Reviewed-by: Michal Privoznik 

Michal

Re: [PATCH] virt-aa-helper: Fix build by including virutil.h

[Jim Fehlig]

On 2/25/20 4:35 AM, Michal Privoznik wrote:

On 2/25/20 10:06 AM, Christian Ehrhardt wrote:



On Tue, Feb 25, 2020 at 12:36 AM Jim Fehlig > wrote:


    Commit fb01e1a44d missed including virutil.h, causing the following
    compilation error

    ../../src/security/virt-aa-helper.c:1055:43: error: implicit
    declaration of
    function 'virHostGetDRMRenderNode'
    [-Werror=implicit-function-declaration]
    1055 |                 char *defaultRenderNode =
    virHostGetDRMRenderNode();

    Signed-off-by: Jim Fehlig mailto:jfeh...@suse.com>>
    ---

    Pushing under the build-breaker rule.


Thanks Jim,
it always built for me in all pre-tests which is odd.
Thanks for fixing it so fast!


In fact I think the commit that caused the problem was a different one (I blame 
the latest virutil.h include cleanup). The problem was that some header files 
included virutil.h directly and thus aa-helper code got it for free. After the 
include was removed, the helper needs to include virtuil.h explicitly. I haven't 
look at which commit exactly caused the problem. It doesn't really matter, does it?


Yep, I think you are right. It was likely commit b11e8ccc. But I also think you 
are right in that it really doesn't matter at this point :-).


Regards,
Jim

Re: [libvirt PATCH 2/8] bridge: include netdev_bandwidth_conf.h

[Michal Privoznik]

On 2/22/20 5:31 PM, Ján Tomko wrote:

This file uses the virNetDevBandwidth*Floor helpers
without including the correct include,
relying on virnetworkportdef.h to include it.

Signed-off-by: Ján Tomko 
Fixes: 17f430eb5cfaaa3388077f2b0856f011f0b2a4c3


I have a question about this "Fixes" field. I've noticed you put it into 
some commit messages, but I can't figure out the pattern. I thought that 
you are blaming commits from previous releases (to help distro 
maintainers), but the commit you are referencing here is in the same 
release as this one. And some commits in this series don't have the 
field at all.


Michal

Re: [libvirt PATCH] tests: fix missing test data for network port XML

[Michal Privoznik]

On 2/25/20 12:09 PM, Daniel P. Berrangé wrote:

The network port XML files were not including any usage of vlan
tags or port options, and one of the files was not even processed.

Signed-off-by: Daniel P. Berrangé 
---
  tests/virnetworkportxml2xmldata/plug-network.xml | 5 +
  tests/virnetworkportxml2xmltest.c| 1 +
  2 files changed, 6 insertions(+)


Reviewed-by: Michal Privoznik 

Michal

[libvirt PATCH] ci: Drop handling of $PKG_CONFIG_LIBDIR

[Andrea Bolognani]

As of libvirt-jenkins-ci commit e41e341f0d8f, we no longer bake
this environment variable into our container images.

Signed-off-by: Andrea Bolognani 
---
 ci/Makefile | 4 
 1 file changed, 4 deletions(-)

diff --git a/ci/Makefile b/ci/Makefile
index 577b130d2f..bc1dac11e3 100644
--- a/ci/Makefile
+++ b/ci/Makefile
@@ -216,15 +216,11 @@ ci-run-command@%: ci-prepare-tree
$(CI_ENGINE) run $(CI_ENGINE_ARGS) $(CI_IMAGE_PREFIX)$*$(CI_IMAGE_TAG) \
/bin/bash -c ' \
$(CI_USER_HOME)/prepare || exit 1; \
-   if test "$$PKG_CONFIG_LIBDIR"; then \
-   pkgconfig_env="PKG_CONFIG_LIBDIR=$$PKG_CONFIG_LIBDIR"; \
-   fi; \
sudo \
  --login \
  --user="#$(CI_UID)" \
  --group="#$(CI_GID)" \
  CONFIGURE_OPTS="$$CONFIGURE_OPTS" \
- $$pkgconfig_env \
  CI_CONT_SRCDIR="$(CI_CONT_SRCDIR)" \
  CI_CONT_BUILDDIR="$(CI_CONT_BUILDDIR)" \
  CI_SMP="$(CI_SMP)" \
-- 
2.24.1

Re: [libvirt PATCH v2 6/9] qemu: prepare and stop the dbus daemon

[Marc-André Lureau]

Hi

On Tue, Feb 25, 2020 at 12:49 PM Daniel P. Berrangé  wrote:
>
> On Tue, Feb 25, 2020 at 10:55:10AM +0100, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau 
> >
> > Signed-off-by: Marc-André Lureau 
> > ---
> >  src/qemu/qemu_process.c | 6 ++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index 8c1ed76677..3a6cb4b2b0 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> > @@ -58,6 +58,7 @@
> >  #include "qemu_extdevice.h"
> >  #include "qemu_firmware.h"
> >  #include "qemu_backup.h"
> > +#include "qemu_dbus.h"
> >
> >  #include "cpu/cpu.h"
> >  #include "cpu/cpu_x86.h"
> > @@ -6480,6 +6481,9 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver,
> >  qemuDomainObjPrivatePtr priv = vm->privateData;
> >  g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
> >
> > +if (qemuDBusPrepareHost(driver) < 0)
> > +return -1;
>
> This launches dbus unconditionally for every VM regardless of whether
> its actually going to be used.

No, it is qemuDBusStart()

>
> There is certainly a nice conceptual simplicity in doing this, but
> I'm pretty concerned that there are going to be mgmt apps which
> do not like this extra overhead being added for every VM. I'm
> thinking KubeVirt and Kata containers in particular. This is also
> relevant for the libvirt embedded driver which is trying to eliminate
> all libvirt added overhead on managing QEMU, so that our fastest
> QEMU startup time can match that achieved by running QEMU directly.
> Unconditionally starting dbus will make that much more challenging.
>
> This is a long winded way of saying I think we need to do this
> only when it is actually required. This will certainly add complexity
> as we'll need to cope with dynamically launching DBus when we hotplug
> certain types of device which require it. I think we can ignore the
> hot-unplug case at least, as once you've taken the overhead for DBus
> for the VM, I don't think there's much to complain about, even if the
> device using it is unplugged.

It is done on-demand, by qemuSlirpStart() at the moment, and actually
only when the backend has dbus support.

[libvirt-dockerfiles PATCH] Refresh after changes to cross-building environments

[Andrea Bolognani]

The new configurations are simpler and more reliable.

The corresponding libvirt-jenkins-ci commit is 483dfc62c86f.

Signed-off-by: Andrea Bolognani 
---
As usual, this is merely the plain-text representation of a
binary patch.

Pushed under the Dockerfile update rule.

 ...denv-libosinfo-fedora-30-cross-mingw32.zip | Bin 687 -> 629 bytes
 ...denv-libosinfo-fedora-30-cross-mingw64.zip | Bin 689 -> 631 bytes
 buildenv-libvirt-debian-10-cross-aarch64.zip  | Bin 1021 -> 982 bytes
 buildenv-libvirt-debian-10-cross-armv6l.zip   | Bin 1014 -> 974 bytes
 buildenv-libvirt-debian-10-cross-armv7l.zip   | Bin 1019 -> 983 bytes
 buildenv-libvirt-debian-10-cross-i686.zip | Bin 1016 -> 981 bytes
 buildenv-libvirt-debian-10-cross-mips.zip | Bin 1011 -> 974 bytes
 buildenv-libvirt-debian-10-cross-mips64el.zip | Bin 1022 -> 985 bytes
 buildenv-libvirt-debian-10-cross-mipsel.zip   | Bin 1016 -> 982 bytes
 buildenv-libvirt-debian-10-cross-ppc64le.zip  | Bin 1024 -> 985 bytes
 buildenv-libvirt-debian-10-cross-s390x.zip| Bin 1011 -> 977 bytes
 buildenv-libvirt-debian-9-cross-aarch64.zip   | Bin 1053 -> 1018 bytes
 buildenv-libvirt-debian-9-cross-armv6l.zip| Bin 1045 -> 1009 bytes
 buildenv-libvirt-debian-9-cross-armv7l.zip| Bin 1050 -> 1014 bytes
 buildenv-libvirt-debian-9-cross-mips.zip  | Bin 1044 -> 1010 bytes
 buildenv-libvirt-debian-9-cross-mips64el.zip  | Bin 1056 -> 1021 bytes
 buildenv-libvirt-debian-9-cross-mipsel.zip| Bin 1048 -> 1014 bytes
 buildenv-libvirt-debian-9-cross-ppc64le.zip   | Bin 1057 -> 1017 bytes
 buildenv-libvirt-debian-9-cross-s390x.zip | Bin 1045 -> 1013 bytes
 buildenv-libvirt-debian-sid-cross-aarch64.zip | Bin 1020 -> 986 bytes
 buildenv-libvirt-debian-sid-cross-armv6l.zip  | Bin 1013 -> 978 bytes
 buildenv-libvirt-debian-sid-cross-armv7l.zip  | Bin 1019 -> 982 bytes
 buildenv-libvirt-debian-sid-cross-i686.zip| Bin 1016 -> 981 bytes
 ...denv-libvirt-debian-sid-cross-mips64el.zip | Bin 1022 -> 985 bytes
 buildenv-libvirt-debian-sid-cross-mipsel.zip  | Bin 1013 -> 974 bytes
 buildenv-libvirt-debian-sid-cross-ppc64le.zip | Bin 1023 -> 986 bytes
 buildenv-libvirt-debian-sid-cross-s390x.zip   | Bin 1011 -> 977 bytes
 buildenv-libvirt-fedora-30-cross-mingw32.zip  | Bin 958 -> 897 bytes
 buildenv-libvirt-fedora-30-cross-mingw64.zip  | Bin 960 -> 899 bytes
 29 files changed, 0 insertions(+), 0 deletions(-)

diff --git a/buildenv-libosinfo-fedora-30-cross-mingw32.zip 
b/buildenv-libosinfo-fedora-30-cross-mingw32.zip
index f70be8d..ff8776c 100644
--- a/buildenv-libosinfo-fedora-30-cross-mingw32.zip
+++ b/buildenv-libosinfo-fedora-30-cross-mingw32.zip
@@ -65,6 +65,4 @@ RUN dnf install -y \
 ENV LANG "en_US.UTF-8"
 
 ENV ABI "i686-w64-mingw32"
-ENV CONFIGURE_OPTS "--host=i686-w64-mingw32 \
---target=i686-w64-mingw32"
-ENV PKG_CONFIG_LIBDIR 
"/usr/i686-w64-mingw32/sys-root/mingw/lib/pkgconfig:/usr/i686-w64-mingw32/sys-root/mingw/share/pkgconfig"
+ENV CONFIGURE_OPTS "--host=i686-w64-mingw32"
diff --git a/buildenv-libosinfo-fedora-30-cross-mingw64.zip 
b/buildenv-libosinfo-fedora-30-cross-mingw64.zip
index 7f75981..24f38bc 100644
--- a/buildenv-libosinfo-fedora-30-cross-mingw64.zip
+++ b/buildenv-libosinfo-fedora-30-cross-mingw64.zip
@@ -65,6 +65,4 @@ RUN dnf install -y \
 ENV LANG "en_US.UTF-8"
 
 ENV ABI "x86_64-w64-mingw32"
-ENV CONFIGURE_OPTS "--host=x86_64-w64-mingw32 \
---target=x86_64-w64-mingw32"
-ENV PKG_CONFIG_LIBDIR 
"/usr/x86_64-w64-mingw32/sys-root/mingw/lib/pkgconfig:/usr/x86_64-w64-mingw32/sys-root/mingw/share/pkgconfig"
+ENV CONFIGURE_OPTS "--host=x86_64-w64-mingw32"
diff --git a/buildenv-libvirt-debian-10-cross-aarch64.zip 
b/buildenv-libvirt-debian-10-cross-aarch64.zip
index 6ae4014..9bdfe9c 100644
--- a/buildenv-libvirt-debian-10-cross-aarch64.zip
+++ b/buildenv-libvirt-debian-10-cross-aarch64.zip
@@ -64,6 +64,7 @@ RUN export DEBIAN_FRONTEND=noninteractive && \
 dpkg --add-architecture arm64 && \
 apt-get update && \
 apt-get dist-upgrade -y && \
+apt-get install --no-install-recommends -y dpkg-dev && \
 apt-get install --no-install-recommends -y \
 gcc-aarch64-linux-gnu \
 libacl1-dev:arm64 \
@@ -108,6 +109,4 @@ RUN export DEBIAN_FRONTEND=noninteractive && \
 ENV LANG "en_US.UTF-8"
 
 ENV ABI "aarch64-linux-gnu"
-ENV CONFIGURE_OPTS "--host=aarch64-linux-gnu \
---target=aarch64-linux-gnu"
-ENV PKG_CONFIG_LIBDIR "/usr/lib/aarch64-linux-gnu/pkgconfig"
+ENV CONFIGURE_OPTS "--host=aarch64-linux-gnu"
diff --git a/buildenv-libvirt-debian-10-cross-armv6l.zip 
b/buildenv-libvirt-debian-10-cross-armv6l.zip
index bc4512a..84df535 100644
--- a/buildenv-libvirt-debian-10-cross-armv6l.zip
+++ b/buildenv-libvirt-debian-10-cross-armv6l.zip
@@ -64,6 +64,7 @@ RUN export DEBIAN_FRONTEND=noninteractive && \
 dpkg --add-architecture armel && \
 apt-get update && \
 apt-get dist-upgrade -y && \
+apt-get install --no-install-recommends -y dpkg-dev && \
 apt-

Re: [PATCH v2 3/3] kbase: backing_chains: Clarify some aspects of image probing

[Daniel P . Berrangé]

On Tue, Feb 25, 2020 at 02:25:45PM +0100, Peter Krempa wrote:
> Signed-off-by: Peter Krempa 
> ---
>  docs/kbase/backing_chains.rst | 16 ++--
>  1 file changed, 14 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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

Re: [PATCH v2 2/3] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances

[Daniel P . Berrangé]

On Tue, Feb 25, 2020 at 02:25:44PM +0100, Peter Krempa wrote:
> Allow format probing to work around lazy clients which did not specify
> their format in the overlay. Format probing will be allowed only, if we
> are able to probe the image, the probing result was successful and the
> probed image does not have any backing or data file.

Ok, so we're limiting probing to 1 level of recursion, so we're
not newly enabling 3 level chains

> 
> This relaxes the restrictions which were imposed in commit 3615e8b39bad
> in cases when we know that the image probing will not result in security
> issues or data corruption.
> 
> We perform the image format detection and in the case that we were able
> to probe the format and the format does not specify a backing store (or
> doesn't support backing store) we can use this format.
> 
> With pre-blockdev configurations this will restore the previous
> behaviour for the images mentioned above as qemu would probe the format
> anyways. It also improves error reporting compared to the old state as
> we now report that the backing chain will be broken in case when there
> is a backing file.
> 
> In blockdev configurations this ensures that libvirt will not cause data
> corruption by ending the chain prematurely without notifying the user,
> but still allows the old semantics when the users forgot to specify the
> format.
> 
> Users thus don't have to re-invent when image format detection is safe
> to do.
> 
> The price for this is that libvirt will need to keep the image format
> detector still current and working or replace it by invocation of
> qemu-img.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/util/virstoragefile.c | 52 ++-
>  1 file changed, 30 insertions(+), 22 deletions(-)
> 
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index d75d2a689a..b133cf17f1 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -4986,6 +4986,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr 
> src,
>   virHashTablePtr cycle,
>   unsigned int depth)
>  {
> +virStorageFileFormat orig_format = src->format;
>  size_t headerLen;
>  int rv;
>  g_autofree char *buf = NULL;
> @@ -4995,10 +4996,17 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr 
> src,
>NULLSTR(src->path), src->format,
>(unsigned int)uid, (unsigned int)gid);
> 
> +if (src->format == VIR_STORAGE_FILE_AUTO_SAFE)
> +src->format = VIR_STORAGE_FILE_AUTO;
> +
>  /* exit if we can't load information about the current image */
>  rv = virStorageFileSupportsBackingChainTraversal(src);
> -if (rv <= 0)
> +if (rv <= 0) {
> +if (orig_format == VIR_STORAGE_FILE_AUTO)
> +return -2;
> +
>  return rv;
> +}
> 
>  if (virStorageFileGetMetadataRecurseReadHeader(src, parent, uid, gid,
> &buf, &headerLen, cycle) 
> < 0)
> @@ -5007,6 +5015,18 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr 
> src,
>  if (virStorageFileGetMetadataInternal(src, buf, headerLen) < 0)
>  return -1;
> 
> +/* If we probed the format we MUST ensure that nothing else than the 
> current
> + * image (this includes both backing files and external data store) is
> + * considered for security labelling and/or recursion. */
> +if (orig_format == VIR_STORAGE_FILE_AUTO) {
> +if (src->backingStoreRaw || src->externalDataStoreRaw) {
> +src->format = VIR_STORAGE_FILE_RAW;
> +VIR_FREE(src->backingStoreRaw);
> +VIR_FREE(src->externalDataStoreRaw);
> +return -2;
> +}
> +}

Ok, this is where we limit recursion to 1 level to avoid enabling
deep chains.

> +
>  if (src->backingStoreRaw) {
>  if ((rv = virStorageSourceNewFromBacking(src, &backingStore)) < 0)
>  return -1;
> @@ -5015,33 +5035,21 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr 
> src,
>  if (rv == 1)
>  return 0;
> 
> -if (backingStore->format == VIR_STORAGE_FILE_AUTO) {
> -/* Assuming the backing store to be raw can lead to failures. We 
> do
> - * it only when we must not report an error to prevent losing 
> VMs.
> - * Otherwise report an error.
> - */
> -if (report_broken) {
> +if ((rv = virStorageFileGetMetadataRecurse(backingStore, parent,
> +   uid, gid,
> +   report_broken,
> +   cycle, depth + 1)) < 0) {
> +if (!report_broken)
> +return 0;
> +
> +if (rv == -2) {
>  virReportError(VIR_ERR_OPERATION_INVALID,
> _("format of backing image '%s' of imag

Re: [PATCH v2 1/3] qemu: domain: Convert detected 'iso' image format into 'raw'

[Daniel P . Berrangé]

On Tue, Feb 25, 2020 at 02:25:43PM +0100, Peter Krempa wrote:
> While our code can detect ISO as a separate format, qemu does not use it
> as such and just passes it through as raw. Add conversion for detected
> parts of the backing chain so that the validation code does not reject
> it right away.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_domain.c | 4 
>  1 file changed, 4 insertions(+)

Reviewed-by: Daniel P. Berrangé 


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

Re: [libvirt-jenkins-ci PATCH 0/4] Fixes and improvements to cross-build environments

[Fabiano Fidêncio]

On Tue, Feb 25, 2020 at 1:17 PM Andrea Bolognani  wrote:
>
> The first two commits are necessary to build container images that
> can successfully be used in libosinfo's GitLab CI setup, which is
> currently not performing MinGW builds because of the issues they
> address.
>
> Andrea Bolognani (4):
>   lcitool: Install dpkg-dev when doing cross-builds on Debian
>   Don't set $PKG_CONFIG_LIBDIR anywhere
>   lcitool: Drop duplicated code
>   lcitool: Don't specify --target in $CONFIGURE_OPTS
>
>  guests/lcitool   | 15 +++
>  guests/playbooks/build/jobs/defaults.yml |  2 --
>  jenkins/jobs/defaults.yaml   |  2 --
>  3 files changed, 3 insertions(+), 16 deletions(-)
>
> --
> 2.24.1
>

Reviewed-by: Fabiano Fidêncio 

[PATCH v2 0/3] Re-think stance towards image format probing

[Peter Krempa]

We decided that use of qemu-img would not be possible for this case. I'm
thus re-sending the patch with fixes to docs and the ISO image format
probe.

This approach is the simplest and most straightforward and deals with
most cases. Specifically we don't have to fix half of blockjob code by
doing this as opposed if we wanted to have qemu open the image itself by
looking into the overlay's metadata.

Peter Krempa (3):
  qemu: domain: Convert detected 'iso' image format into 'raw'
  virStorageFileGetMetadataRecurse: Allow format probing under special
circumstances
  kbase: backing_chains: Clarify some aspects of image probing

 docs/kbase/backing_chains.rst | 16 +--
 src/qemu/qemu_domain.c|  4 +++
 src/util/virstoragefile.c | 52 ---
 3 files changed, 48 insertions(+), 24 deletions(-)

-- 
2.24.1

[PATCH v2 2/3] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances

[Peter Krempa]

Allow format probing to work around lazy clients which did not specify
their format in the overlay. Format probing will be allowed only, if we
are able to probe the image, the probing result was successful and the
probed image does not have any backing or data file.

This relaxes the restrictions which were imposed in commit 3615e8b39bad
in cases when we know that the image probing will not result in security
issues or data corruption.

We perform the image format detection and in the case that we were able
to probe the format and the format does not specify a backing store (or
doesn't support backing store) we can use this format.

With pre-blockdev configurations this will restore the previous
behaviour for the images mentioned above as qemu would probe the format
anyways. It also improves error reporting compared to the old state as
we now report that the backing chain will be broken in case when there
is a backing file.

In blockdev configurations this ensures that libvirt will not cause data
corruption by ending the chain prematurely without notifying the user,
but still allows the old semantics when the users forgot to specify the
format.

Users thus don't have to re-invent when image format detection is safe
to do.

The price for this is that libvirt will need to keep the image format
detector still current and working or replace it by invocation of
qemu-img.

Signed-off-by: Peter Krempa 
---
 src/util/virstoragefile.c | 52 ++-
 1 file changed, 30 insertions(+), 22 deletions(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index d75d2a689a..b133cf17f1 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -4986,6 +4986,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
  virHashTablePtr cycle,
  unsigned int depth)
 {
+virStorageFileFormat orig_format = src->format;
 size_t headerLen;
 int rv;
 g_autofree char *buf = NULL;
@@ -4995,10 +4996,17 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr 
src,
   NULLSTR(src->path), src->format,
   (unsigned int)uid, (unsigned int)gid);

+if (src->format == VIR_STORAGE_FILE_AUTO_SAFE)
+src->format = VIR_STORAGE_FILE_AUTO;
+
 /* exit if we can't load information about the current image */
 rv = virStorageFileSupportsBackingChainTraversal(src);
-if (rv <= 0)
+if (rv <= 0) {
+if (orig_format == VIR_STORAGE_FILE_AUTO)
+return -2;
+
 return rv;
+}

 if (virStorageFileGetMetadataRecurseReadHeader(src, parent, uid, gid,
&buf, &headerLen, cycle) < 
0)
@@ -5007,6 +5015,18 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
 if (virStorageFileGetMetadataInternal(src, buf, headerLen) < 0)
 return -1;

+/* If we probed the format we MUST ensure that nothing else than the 
current
+ * image (this includes both backing files and external data store) is
+ * considered for security labelling and/or recursion. */
+if (orig_format == VIR_STORAGE_FILE_AUTO) {
+if (src->backingStoreRaw || src->externalDataStoreRaw) {
+src->format = VIR_STORAGE_FILE_RAW;
+VIR_FREE(src->backingStoreRaw);
+VIR_FREE(src->externalDataStoreRaw);
+return -2;
+}
+}
+
 if (src->backingStoreRaw) {
 if ((rv = virStorageSourceNewFromBacking(src, &backingStore)) < 0)
 return -1;
@@ -5015,33 +5035,21 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr 
src,
 if (rv == 1)
 return 0;

-if (backingStore->format == VIR_STORAGE_FILE_AUTO) {
-/* Assuming the backing store to be raw can lead to failures. We do
- * it only when we must not report an error to prevent losing VMs.
- * Otherwise report an error.
- */
-if (report_broken) {
+if ((rv = virStorageFileGetMetadataRecurse(backingStore, parent,
+   uid, gid,
+   report_broken,
+   cycle, depth + 1)) < 0) {
+if (!report_broken)
+return 0;
+
+if (rv == -2) {
 virReportError(VIR_ERR_OPERATION_INVALID,
_("format of backing image '%s' of image '%s' 
was not specified in the image metadata "
  "(See 
https://libvirt.org/kbase/backing_chains.html for troubleshooting)"),
src->backingStoreRaw, NULLSTR(src->path));
-return -1;
 }

-backingStore->format = VIR_STORAGE_FILE_RAW;
-}
-
-if (backingStore->format == VIR_STORAGE_FILE_AUTO_SAFE)
-backingStore->format = VIR_STORAGE_FILE_AUTO;

[PATCH v2 1/3] qemu: domain: Convert detected 'iso' image format into 'raw'

[Peter Krempa]

While our code can detect ISO as a separate format, qemu does not use it
as such and just passes it through as raw. Add conversion for detected
parts of the backing chain so that the validation code does not reject
it right away.

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

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 446a517df9..a28b51c10e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -11493,6 +11493,10 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
 return -1;

 for (n = src->backingStore; virStorageSourceIsBacking(n); n = 
n->backingStore) {
+/* convert detected ISO format to 'raw' as qemu would not understand 
it */
+if (n->format == VIR_STORAGE_FILE_ISO)
+n->format = VIR_STORAGE_FILE_RAW;
+
 if (qemuDomainValidateStorageSource(n, priv->qemuCaps) < 0)
 return -1;

-- 
2.24.1

[PATCH v2 3/3] kbase: backing_chains: Clarify some aspects of image probing

[Peter Krempa]

Signed-off-by: Peter Krempa 
---
 docs/kbase/backing_chains.rst | 16 ++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/docs/kbase/backing_chains.rst b/docs/kbase/backing_chains.rst
index 3b3f0583e5..12ed6253ac 100644
--- a/docs/kbase/backing_chains.rst
+++ b/docs/kbase/backing_chains.rst
@@ -46,14 +46,17 @@ system used on the host so that the hypervisor can access 
the files and possibly
 also directly to configure the hypervisor to use the appropriate images. Thus
 it's important to properly setup the formats and paths of the backing images.

+Any externally created image should always use the -F switch of ``qemu-img``
+to specify the format of the backing file to avoid probing.
+
 Image detection caveats
 ---

 Detection of the backing chain requires libvirt to read and understand the
 ``backing file`` field recorded in the image metadata and also being able to
 recurse and read the backing file. Due to security implications libvirt
-will not attempt to detect the format of the backing image if the image 
metadata
-doesn't contain it.
+will refuse to use backing images of any image whose format was not specified
+explicitly in the XML or the overlay image itself.

 Libvirt also might lack support for a network disk storage technology and thus
 may be unable to visit and detect backing chains on such storage. This may
@@ -104,6 +107,8 @@ Note that it's also possible to partially specify the chain 
in the XML but omit
 the terminating element. This will result into probing from the last specified
 

+Any image specified explicitly will not be probed for backing file or format.
+

 Manual image creation
 =
@@ -113,6 +118,13 @@ them properly so that they work with libvirt as expected. 
The created disk
 images must contain the format of the backing image in the metadata. This
 means that the **-F** parameter of ``qemu-img`` must always be used.

+::
+
+  qemu-img -f qcow2 -F qcow2 -b /path/to/backing /path/to/overlay
+
+Note that if '/path/to/backing' is relative the path is considered relative to
+the location of '/path/to/overlay'.
+
 Troubleshooting
 ===

-- 
2.24.1

Re: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances

[Peter Krempa]

On Tue, Feb 25, 2020 at 12:50:21 +, Daniel Berrange wrote:
> On Mon, Feb 24, 2020 at 06:10:46PM +0100, Peter Krempa wrote:
> > On Mon, Feb 24, 2020 at 14:24:15 +, Daniel Berrange wrote:
> > > On Mon, Feb 24, 2020 at 02:34:16PM +0100, Peter Krempa wrote:

[...]

> > > Would there be any downsides to this that did not already exist in the
> > > non-blockdev days ?
> > 
> > We can, but the price is that:
> > 1) we won't allow blockjobs and anything blockdev-related because node
> > name would be out of our control. This was possible in pre-blockdev era.
> 
> Ok, that's not viable then. We can't switch one regression for a different
> regression.
> 
> > 2) we will lose control of actually telling qemu to NOT open the backing
> > file in that case. Distros using only unix permission still have
> > arbitrary file access under permissions of the qemu process.
> 
> True, but that is at least historically expected behaviour, which can
> be fixed by setting  in the XML file IIUC.

Yes, but if you specify any  including the terminator you
basically configure the image chain yourselves. The part which is
configured explicitly will not undergo any form of detection, not even
looking for the backing file.

> > 3) weird special-case code, because we need to keep some metadata about
> > the image to do security labelling
> > 
> > > I don't think we can solve the regressions in behaviour of backing files
> > > by doing probing of the backing files in libvirt, because that only works
> > > for the case where libvirt can actually open the file. ie a local file on
> > > disk. We don't have logic for opening backing files on RBD, GlusterFS,
> > > iSCSI, HTTP, SSH, etc, and nor do we want todo that.
> > 
> > Now we are back in the teritory where we actually do match what would
> > happen with previously. We don't specify these on the command line with
> > ehaviour matching what's described above, with the caveats as above.
> > 
> > I kept this behaviour because we couldn't do better. This is in place
> > even now if the last introspectable image has valid format specified.
> > 
> > We can reconsider how to approach this but ideally separately.
> 
> I'm a little lost as to exactly which scenarios are broken, and which
> we're fixing.
> 
>  1. file:top.qcow2 -> file:base.raw
> 
>  2. file:top.qcow2 -> file:base.qcow2
> 
>  3. file:top.qcow2 -> file:middle.qcow2 -> file:base.raw
>  
>  4. file:top.qcow2 -> file:middle.qcow2 -> file:base.qcow2

I assume you meant that none of the 'qcow2' files have format of the
backing file recorded in the metadata.

> IIUC, (1) is working before and now, (2) is working before but
> broken now, and (3) and (4) were broken before and now.

1) was working before, but is now forbidden
2) was working before, and is now forbidden
3,4) were possibly working (without sVirt), or would get permission
denied reported by qemu during startup, currently they are forbidden by
a libvirt error message

After this patch:
1) will be fixed by this patch
2) will be fixed by this patch
3, 4) will report a libvirt error rather than relying on sVirt or others

> So (2) is the only one we /must/ to fix
> 
> Am I right that by doing probing in libvirt as per this patch,
> as well as fixing (2) though, we'll accidentally fix (3) and (4)
> even though they were always broken before ?
>
> This all talks about qcow2 images on the file: protocol driver.
> What is the situation for, say, the iscsi: protocol driver
> 
> 
>  1. iscsi:top.qcow2 -> iscsi:base.raw
> 
>  2. iscsi:top.qcow2 -> iscsi:base.qcow2
> 
>  3. iscsi:top.qcow2 -> iscsi:middle.qcow2 -> iscsi:base.raw
>  
>  4. iscsi:top.qcow2 -> iscsi:middle.qcow2 -> iscsi:base.qcow2
> 
> What was the behaviour of this in the pre-blockdev days and
> vs current git master ? Is it the same with (1) is working
> before and now, (2) is working before but broken now, and
> (3) and (4) were broken before and now.

Pre-blockdev and post-blockdev every scenario of the above is working.
We can't even inspect the top file for backing store so everything is
just ignored.

Now what partially changes is when we have something introspectable in
the way:

1,2) file:top.qcow2 -> iscsi:base.whatever

The current code would report an error if the format was not recorded
in the overlay (top.qcow2) and we can't introspect the backing file
we'll reject it.

This can be relaxed though.

3,4) will work if top.qcow2 has the format but any subsequent don't since
we don't introspect it

> I'm assuming the libvirt probing cannot fix any case other
> than file: protocol, or host-device: protocol, since we're
> unable to open any other type of storage.

It fixes also gluster since the backends for that are already
implemented.

I'll post another rebased version with some updated docs and one fix. We
can continue there perhaps.

Re: [libvirt PATCH 5/6] Make PATHs unique for a VM object instance

[Daniel P . Berrangé]

On Wed, Feb 19, 2020 at 02:39:22AM +, Shaju Abraham wrote:
> 
> 
> On 2/11/20, 7:06 PM, "Daniel P. Berrangé"  wrote:
> 
> On Tue, Feb 11, 2020 at 10:05:53AM +0100, Martin Kletzander wrote:
>   >  > On Wed, Feb 05, 2020 at 05:32:50PM +, Daniel P. Berrangé wrote:
> > > > On Mon, Feb 03, 2020 at 12:43:32PM +, Daniel P. Berrangé wrote:
> > > > > From: Shaju Abraham 
> > > > > 
> > > > There are various config paths that a VM uses. The monitor paths and
> > > > > other lib paths are examples. These paths are tied to the VM name 
> or
> > > > > UUID. The local migration breaks the assumption that there will 
> be only
> > > > > one VM with a unique UUID and name. During local migrations there 
> can be
> > > > > multiple VMs with same name and UUID in the same host. Append the
> > > > > domain-id field to the path so that there is no duplication of 
> path
> > > > names.
> > > > 
> > > >This is the really critical problem with localhost migration.
> > > >
> > > >Appending the domain-id looks "simple" but this is a significant
> > > >behavioural / functional change for applications, and I don't think
> > > >it can fully solve the problem either.
> > > > 
> > > >This is changing thue paths used in various places where libvirt
> > > >internally generates unique paths (eg QMP socket, huge page or
> > > >file based memory paths, and defaults used for auto-filling
> > > >device paths ( if not specified).
> > > >
> > > >Some of these paths are functionally important to external
> > > >applications and cannot be changed in this way. eg stuff
> > > >integrating with QEMU can be expecting certain memory backing
> > > >file paths, or certain  paths & is liable to break
> > > >if we change the naming convention.
> > > >
> > > >For sake of argument, lets assume we can changing the naming
> > > >convention without breaking anything...
> > > >
> > >
> > >This was already done in (I would say) most places as they use
> > >virDomainDefGetShortName() to get a short, unique name of a directory 
> -- it uses
> > >the domain ID as a prefix.
> > >
> > > > This only applies to paths libvirt generates at VM startup.
> > > >
> > > >There are plenty of configuration elements in the guest XML
> > > >that are end user / mgmt app defined, and reference paths in
> > > >the host OS.
> > > >
> > > >For example , , , ,
> > > >all support UNIX domain sockets and TCP sockets. A UNIX
> > > >domain socket cannot be listened on by multiple VMs
> > > >at once. If the UNIX socket is in client mode, we cannot
> > > >assume the thing QEMU is connecting to allows multiple
> > > >concurrent connections. eg 2 QEMU's could have their
> > > > connected together over a UNIX socket pair.
> > > >Similarly if automatic TCP port assignment is not used
> > > >we cannot have multiple QEMU's listening on the same
> > > >host.
> > > >
> > > >One answer is to say that localhost migration is just
> > > >not supported for such VMs, but I don't find that very
> > > >convincing because the UNIX domain socket configs
> > > >affected are in common use.
> > > >
> > >
> > >I would be okay with saying that these either need to be changed in a 
> provided
> > >destination XML or the migration will probably break.  I do not think 
> it is
> > >unreasonable to say that if users are trying to shoot themselves, we 
> should not
> > >spend a ridiculous time just so we can prevent that.  Otherwise we 
> will get to
> > >the same point as we are now -- there might be a case where a local 
> migration
> > >would work, but users cannot execute it even if they were very 
> cautious and went
> > >through all the things that can prevent it from the technical point of 
> view,
> > >libvirt will still disallow that.
> 
> >If there are clashing resources, we can't rely on QEMU reporting an
> >error. For example with a UNIX domain socket, the first thing QEMU
> >does is unlink(/socket/path), which will blow away the UNIX domain
> >socket belonging to the original QEMU. As a result if migration
> >fails, and we rollback, the original QEMU will be broken.
> 
>By appending the id field to the path, we are effectively nullifying this 
> particular
>concern. Each qemu instance will get its own unique path and monitor. If a 
> migration
>fails, we can roll back.

No, you've not nullified the problem. This only helps the cases where
libvirt generates the path. This is only a subset of affected cases.
Just one example:

   

there are many other parts of the domain XML that accept UNIX socket
paths where the mgmt app picks the socket path. Nothing is validating
this to prevent clashes between the src+dst QEMU on the same host,
meaning on migration rollback, src QEMU is broken.


Regards

Re: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances

[Daniel P . Berrangé]

On Mon, Feb 24, 2020 at 06:10:46PM +0100, Peter Krempa wrote:
> On Mon, Feb 24, 2020 at 14:24:15 +, Daniel Berrange wrote:
> > On Mon, Feb 24, 2020 at 02:34:16PM +0100, Peter Krempa wrote:
> > > On Wed, Feb 19, 2020 at 13:12:53 -0600, Eric Blake wrote:
> 
> [...]
> 
> > > I'll reiterate the historical state of the problem because I think it's
> > > important:
> > > 
> > > Pre-blockdev:
> > >   - we internally assumed that if the image format of an backing image
> > > was not present in the overlay it is 'raw'
> > >   - this influenced security labelling but not actually how qemu viewed
> > > or probed the file. If it was qcow2 probed as qcow2 qemu opened it
> > > as qcow2 possibly even including the backing file if selinux or
> > > other mechanism didn't prevent it.
> > > 
> > > post-blockdev:
> > >   - the assumption of 'raw' would now be expressed into the qemu
> > > configuration. This assumption turned into data corruption since we
> > > no longer allowed qemu to probe the format and forced it as raw.
> > >   - fix was to always require the format to be recorded in the overlay
> > >   - this made users unhappy who neglected to record the format into the
> > > overlay when creating it manually
> > 
> > So the key problem we have is that with -blockdev we are always explicitly
> > telling QEMU what the backing file is for every image.
> > 
> > Can we fix this to have the exact same behaviour as before by *not* telling
> > QEMU anything about the backing file when using -blockdev, if there is no
> > well defined backing format present. ie, use -blockdev, but let QEMU probe
> > just as it did in non-blockdev days.
> > 
> > Would there be any downsides to this that did not already exist in the
> > non-blockdev days ?
> 
> We can, but the price is that:
> 1) we won't allow blockjobs and anything blockdev-related because node
> name would be out of our control. This was possible in pre-blockdev era.

Ok, that's not viable then. We can't switch one regression for a different
regression.

> 2) we will lose control of actually telling qemu to NOT open the backing
> file in that case. Distros using only unix permission still have
> arbitrary file access under permissions of the qemu process.

True, but that is at least historically expected behaviour, which can
be fixed by setting  in the XML file IIUC.

> 3) weird special-case code, because we need to keep some metadata about
> the image to do security labelling
> 
> > I don't think we can solve the regressions in behaviour of backing files
> > by doing probing of the backing files in libvirt, because that only works
> > for the case where libvirt can actually open the file. ie a local file on
> > disk. We don't have logic for opening backing files on RBD, GlusterFS,
> > iSCSI, HTTP, SSH, etc, and nor do we want todo that.
> 
> Now we are back in the teritory where we actually do match what would
> happen with previously. We don't specify these on the command line with
> ehaviour matching what's described above, with the caveats as above.
> 
> I kept this behaviour because we couldn't do better. This is in place
> even now if the last introspectable image has valid format specified.
> 
> We can reconsider how to approach this but ideally separately.

I'm a little lost as to exactly which scenarios are broken, and which
we're fixing.

 1. file:top.qcow2 -> file:base.raw

 2. file:top.qcow2 -> file:base.qcow2

 3. file:top.qcow2 -> file:middle.qcow2 -> file:base.raw
 
 4. file:top.qcow2 -> file:middle.qcow2 -> file:base.qcow2

IIUC, (1) is working before and now, (2) is working before but
broken now, and (3) and (4) were broken before and now.

So (2) is the only one we /must/ to fix

Am I right that by doing probing in libvirt as per this patch,
as well as fixing (2) though, we'll accidentally fix (3) and (4)
even though they were always broken before ?

This all talks about qcow2 images on the file: protocol driver.
What is the situation for, say, the iscsi: protocol driver


 1. iscsi:top.qcow2 -> iscsi:base.raw

 2. iscsi:top.qcow2 -> iscsi:base.qcow2

 3. iscsi:top.qcow2 -> iscsi:middle.qcow2 -> iscsi:base.raw
 
 4. iscsi:top.qcow2 -> iscsi:middle.qcow2 -> iscsi:base.qcow2

What was the behaviour of this in the pre-blockdev days and
vs current git master ? Is it the same with (1) is working
before and now, (2) is working before but broken now, and
(3) and (4) were broken before and now.

I'm assuming the libvirt probing cannot fix any case other
than file: protocol, or host-device: protocol, since we're
unable to open any other type of storage.


> > > This boils down to whether we want to accept some possibility of image
> > > corruption in trade for avoiding regression of behaviour in the secure
> > > cases as well as management apps and users not having to re-invent when
> > > probing an image is actually safe.
> > 
> > I feel like the risk of image corruption is pretty minor. Our probing
> > handles all normal cases the sam

Re: [PATCH] network: bridge_driver: Use new helpers for storing libvirt errors

[Ján Tomko]

On Mon, Feb 24, 2020 at 10:09:05PM -0500, Laine Stump wrote:

On 2/24/20 1:58 PM, Gaurav Agrawal wrote:

From: GAURAV AGRAWAL 

Signed-off-by: Gaurav Agrawal 
---
 src/network/bridge_driver_linux.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)



Yay! It applied properly, so you have all the pieces of patch 
submission properly in line! :-)



(Welcome to the neighborhood, BTW.)




diff --git a/src/network/bridge_driver_linux.c 
b/src/network/bridge_driver_linux.c
index 7bbde5c6a9..fde33b5d38 100644
--- a/src/network/bridge_driver_linux.c
+++ b/src/network/bridge_driver_linux.c
@@ -22,6 +22,7 @@
 #include 
 #include "viralloc.h"
+#include "virerror.h"
 #include "virfile.h"
 #include "viriptables.h"
 #include "virstring.h"
@@ -53,7 +54,7 @@ static void networkSetupPrivateChains(void)
 if (rc < 0) {
 VIR_DEBUG("Failed to create global IPv4 chains: %s",
   virGetLastErrorMessage());
-errInitV4 = virSaveLastError();
+virErrorPreserveLast(&errInitV4);
 virResetLastError();
 } else {
 virFreeError(errInitV4);
@@ -70,7 +71,7 @@ static void networkSetupPrivateChains(void)
 if (rc < 0) {
 VIR_DEBUG("Failed to create global IPv6 chains: %s",
   virGetLastErrorMessage());
-errInitV6 = virSaveLastError();
+virErrorPreserveLast(&errInitV6);
 virResetLastError();
 } else {
 virFreeError(errInitV6);



For anyone who didn't notice, this patch is for one of the "bite sized 
tasks" here:


https://wiki.libvirt.org/page/BiteSizedTasks#More_conversions_to_virErrorPreserveLast.2FvirErrorRestore


This is a very strange case of virSaveLastError() / virSetError() to 
pick - usually they are in pairs within the same function, but in this 
case when the original error occurs during the driver init, it is 
squanched away in the static errInitV[46] with virSaveLastError(), and 
later, in a completely different context, whenever something tries to 
add a firewall rule of the given type, *then* it sets the error (with 
virSetError()) to what was earlier encountered during init.




Yes, networkSetupPrivateChains is only called once (via virOnce, as
suggested by the comment on the top of the function) on initialization
and if either IPv4 or IPv6 chains could not be created, it sets the
dirver-global error, which is then called on any subsequent attempt
to use it.

So this is not really a case that needs to be converted.
In fact, glancing at git gre virSetError it seems we already got rid
of all the ones worth converting.



Your patch has replaced the virSaveLastError() of the earlier part 
with virErrorPreserveLast(), but hasn't replaced the virSetError() of 
the later part (which is down in networkAddFirewallRules()) with 
virErrorRestore().


virErrorRestore resets the error, which is not what we want here -
any subsequent calls should report the same error we caught when
initializing.

Jano



I can make those two minor changes and push if you like, or you're 
free to send again with --subject-prefix="PATCHv2". (Or, maybe an even 
better idea - you could expand the patch to replace all the other uses 
of virSaveLastError()/virSetError()). The choice is yours :-)



(Oh, and BTW, no need to Cc: crobinso - he'll see it anyway)



signature.asc
Description: PGP signature

Re: [PATCH v4 5/5] lxc: Count max VCPUs based on cpuset.cpus in native config

[Daniel P . Berrangé]

On Mon, Feb 24, 2020 at 11:24:29AM -0300, Julio Faracco wrote:
> Native config files sometimes can setup cpuset.cpus to pin some CPUs.
> Before this, LXC was using a fixed number of 1 VCPU. After this commit,
> XML definition will generate a dynamic number of VCPUs based on that
> cgroup attribute.

I'm not sure whether this is worth doing - mapping between VCPU count
and CPUset doesn't really work during startup. Historically we've just
completely ignored  for containers. Ideally that element should
have been optional in the XML parser as it is not a good conceptual
fit for LXC.

> 
> Signed-off-by: Julio Faracco 
> ---
>  src/lxc/lxc_container.c   | 23 ++
>  src/lxc/lxc_container.h   |  2 ++
>  src/lxc/lxc_native.c  | 24 +--
>  .../lxcconf2xml-cpusettune.xml|  2 +-
>  4 files changed, 48 insertions(+), 3 deletions(-)
> 
> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index 88e27f3060..c5788e5c32 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -2487,3 +2487,26 @@ int lxcContainerChown(virDomainDefPtr def, const char 
> *path)
>  
>  return 0;
>  }
> +
> +
> +int lxcContainerGetMaxCpusInCpuset(const char *cpuset)
> +{
> +const char *c = cpuset;
> +int max_cpu = 0;
> +
> +while (c) {
> +int a, b, ret;
> +
> +ret = sscanf(c, "%d-%d", &a, &b);
> +if (ret == 1)
> +max_cpu++;
> +else if (ret == 2)
> +max_cpu +=  a > b ? a - b + 1 : b - a + 1;
> +
> +if (!(c = strchr(c+1, ',')))
> +break;
> +c++;
> +}
> +
> +return max_cpu;
> +}
> diff --git a/src/lxc/lxc_container.h b/src/lxc/lxc_container.h
> index 94a6c5309c..6f112e0667 100644
> --- a/src/lxc/lxc_container.h
> +++ b/src/lxc/lxc_container.h
> @@ -63,3 +63,5 @@ virArch lxcContainerGetAlt32bitArch(virArch arch);
>  int lxcContainerChown(virDomainDefPtr def, const char *path);
>  
>  bool lxcIsBasicMountLocation(const char *path);
> +
> +int lxcContainerGetMaxCpusInCpuset(const char *cpuset);
> diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
> index 02d2bf33e4..409bf00bd2 100644
> --- a/src/lxc/lxc_native.c
> +++ b/src/lxc/lxc_native.c
> @@ -993,6 +993,24 @@ lxcSetCpusetTune(virDomainDefPtr def, virConfPtr 
> properties)
>  return 0;
>  }
>  
> +
> +static int
> +lxcGetVCpuMax(virConfPtr properties)
> +{
> +g_autofree char *value = NULL;
> +int vcpumax = 1;
> +
> +if (virConfGetValueString(properties, "lxc.cgroup.cpuset.cpus",
> +  &value) > 0) {
> +vcpumax = lxcContainerGetMaxCpusInCpuset(value);
> +if (vcpumax > 0)
> +return vcpumax;
> +}
> +
> +return vcpumax;
> +}
> +
> +
>  static int
>  lxcBlkioDeviceWalkCallback(const char *name, virConfValuePtr value, void 
> *data)
>  {
> @@ -1132,6 +1150,7 @@ lxcParseConfigString(const char *config,
>  virDomainDefPtr vmdef = NULL;
>  g_autoptr(virConf) properties = NULL;
>  g_autofree char *value = NULL;
> +int vcpumax;
>  
>  if (!(properties = virConfReadString(config, VIR_CONF_FLAG_LXC_FORMAT)))
>  return NULL;
> @@ -1155,10 +1174,11 @@ lxcParseConfigString(const char *config,
>  
>  /* Value not handled by the LXC driver, setting to
>   * minimum required to make XML parsing pass */
> -if (virDomainDefSetVcpusMax(vmdef, 1, xmlopt) < 0)
> +vcpumax = lxcGetVCpuMax(properties);
> +if (virDomainDefSetVcpusMax(vmdef, vcpumax, xmlopt) < 0)
>  goto error;
>  
> -if (virDomainDefSetVcpus(vmdef, 1) < 0)
> +if (virDomainDefSetVcpus(vmdef, vcpumax) < 0)
>  goto error;
>  
>  vmdef->nfss = 0;
> diff --git a/tests/lxcconf2xmldata/lxcconf2xml-cpusettune.xml 
> b/tests/lxcconf2xmldata/lxcconf2xml-cpusettune.xml
> index 6df089d00f..a1fec12d9b 100644
> --- a/tests/lxcconf2xmldata/lxcconf2xml-cpusettune.xml
> +++ b/tests/lxcconf2xmldata/lxcconf2xml-cpusettune.xml
> @@ -3,7 +3,7 @@
>c7a5fdbd-edaf-9455-926a-d65c16db1809
>65536
>65536
> -  1
> +  5
>
>  
>
> -- 
> 2.20.1
> 
> 

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 v4 3/5] lxc: Replacing default strings definitions by g_autofree statement

[Daniel P . Berrangé]

On Mon, Feb 24, 2020 at 11:24:27AM -0300, Julio Faracco wrote:
> There are a lots of strings being handled inside some LXC functions.
> They can be moved to g_autofree to avoid declaring a return value to get
> proper code cleanups. This commit is changing functions from
> lxc_{controller,cgroup,fuse} only.
> 
> Signed-off-by: Julio Faracco 
> ---
>  src/lxc/lxc_cgroup.c | 15 +++
>  src/lxc/lxc_controller.c | 96 ++--
>  src/lxc/lxc_fuse.c   | 23 +++---
>  3 files changed, 44 insertions(+), 90 deletions(-)

Reviewed-by: Daniel P. Berrangé 


I'll push this one since its independant of the rest of the series.


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 v4 2/5] lxc: Add HPET device into allowed devices

[Daniel P . Berrangé]

On Mon, Feb 24, 2020 at 11:24:25AM -0300, Julio Faracco wrote:
> This commit is related to RTC timer device too. HPET is being shared
> from host device through `localtime` clock. This timer is available
> creating a new timer using `hpet` name.
> 
> Signed-off-by: Julio Faracco 
> ---
>  docs/formatdomain.html.in |  2 +-
>  src/lxc/lxc_cgroup.c  | 17 +
>  src/lxc/lxc_controller.c  | 33 +
>  3 files changed, 43 insertions(+), 9 deletions(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 5598bf41b4..8571db89dc 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -2464,7 +2464,7 @@
>  The name attribute selects which timer is
>  being modified, and can be one of
>  "platform" (currently unsupported),
> -"hpet" (libxl, xen, qemu), "kvmclock" (qemu),
> +"hpet" (libxl, xen, qemu, lxc), "kvmclock" (qemu),
>  "pit" (qemu), "rtc" (qemu, lxc), "tsc" (libxl, qemu -
>  since 3.2.0), "hypervclock"
>  (qemu - since 1.2.2) or
> diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c
> index 6a103055a4..997a5c3dfa 100644
> --- a/src/lxc/lxc_cgroup.c
> +++ b/src/lxc/lxc_cgroup.c
> @@ -344,20 +344,19 @@ static int virLXCCgroupSetupDeviceACL(virDomainDefPtr 
> def,
>  for (i = 0; i < def->clock.ntimers; i++) {
>  virDomainTimerDefPtr timer = def->clock.timers[i];
>  
> +if (!timer->present)
> +break;
> +
>  switch ((virDomainTimerNameType)timer->name) {
>  case VIR_DOMAIN_TIMER_NAME_PLATFORM:
>  case VIR_DOMAIN_TIMER_NAME_TSC:
>  case VIR_DOMAIN_TIMER_NAME_KVMCLOCK:
>  case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK:
>  case VIR_DOMAIN_TIMER_NAME_PIT:
> -case VIR_DOMAIN_TIMER_NAME_HPET:
>  case VIR_DOMAIN_TIMER_NAME_ARMVTIMER:
>  case VIR_DOMAIN_TIMER_NAME_LAST:
>  break;
>  case VIR_DOMAIN_TIMER_NAME_RTC:
> -if (!timer->present)
> -break;

Instead of moving the check here, just put it in the right place
immediately in the previous patch. Note the comment about this not
being a boolean, but rather a tri-state.

> -
>  if (virFileExists("/dev/rtc")) {
>  if (virCgroupAllowDevicePath(cgroup, "/dev/rtc",
>   VIR_CGROUP_DEVICE_READ,
> @@ -367,6 +366,16 @@ static int virLXCCgroupSetupDeviceACL(virDomainDefPtr 
> def,
>  VIR_DEBUG("Ignoring non-existent device /dev/rtc");
>  }
>  break;
> +case VIR_DOMAIN_TIMER_NAME_HPET:
> +if (virFileExists("/dev/hpet")) {
> +if (virCgroupAllowDevicePath(cgroup, "/dev/hpet",
> + VIR_CGROUP_DEVICE_READ,
> + false) < 0)
> +return -1;
> +} else {
> +VIR_DEBUG("Ignoring non-existent device /dev/hpet");
> +}

Same comment about needing to report an error here.


> +case VIR_DOMAIN_TIMER_NAME_HPET:
> +if (stat("/dev/hpet", &sb) < 0) {
> +if (errno == EACCES)
> +return -1;

Same strange special case missing error message reporting.

> +
> +virReportSystemError(errno,
> + _("Path '%s' is not accessible"),
> + path);
> +return -1;
> +}


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 v4 1/5] lxc: Add Real Time Clock device into allowed devices

[Daniel P . Berrangé]

On Mon, Feb 24, 2020 at 11:24:24AM -0300, Julio Faracco wrote:
> This commit share host Real Time Clock device (rtc) into LXC containers
> to support hardware clock. This should be available setting up a `rtc`
> timer under clock section. Since this option is not emulated, it should
> be available only for `localtime` clock. This option should be readonly
> due to security reasons.
> 
> Before:
> root# hwclock --verbose
> hwclock from util-linux 2.32.1
> System Time: 1581877557.598365
> Trying to open: /dev/rtc0
> Trying to open: /dev/rtc
> Trying to open: /dev/misc/rtc
> No usable clock interface found.
> hwclock: Cannot access the Hardware Clock via any known method.
> 
> Now:
> root# hwclock
> 2020-02-16 18:23:55.374134+00:00
> root# hwclock -w
> hwclock: ioctl(RTC_SET_TIME) to /dev/rtc to set the time failed:
> Permission denied
> 
> Signed-off-by: Julio Faracco 
> ---
>  docs/formatdomain.html.in |  2 +-
>  src/lxc/lxc_cgroup.c  | 36 +
>  src/lxc/lxc_controller.c  | 68 +++
>  3 files changed, 105 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 4fef2a0a97..5598bf41b4 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -2465,7 +2465,7 @@
>  being modified, and can be one of
>  "platform" (currently unsupported),
>  "hpet" (libxl, xen, qemu), "kvmclock" (qemu),
> -"pit" (qemu), "rtc" (qemu), "tsc" (libxl, qemu -
> +"pit" (qemu), "rtc" (qemu, lxc), "tsc" (libxl, qemu -
>  since 3.2.0), "hypervclock"
>  (qemu - since 1.2.2) or
>  "armvtimer" (qemu - since 6.1.0).
> diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c
> index 4ebe5ef467..6a103055a4 100644
> --- a/src/lxc/lxc_cgroup.c
> +++ b/src/lxc/lxc_cgroup.c
> @@ -337,6 +337,42 @@ static int virLXCCgroupSetupDeviceACL(virDomainDefPtr 
> def,
>   VIR_CGROUP_DEVICE_RWM) < 0)
>  return -1;
>  
> +VIR_DEBUG("Allowing timers char devices");
> +
> +/* Sync'ed with Host clock */
> +if (def->clock.offset == VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME) {
> +for (i = 0; i < def->clock.ntimers; i++) {
> +virDomainTimerDefPtr timer = def->clock.timers[i];
> +
> +switch ((virDomainTimerNameType)timer->name) {
> +case VIR_DOMAIN_TIMER_NAME_PLATFORM:
> +case VIR_DOMAIN_TIMER_NAME_TSC:
> +case VIR_DOMAIN_TIMER_NAME_KVMCLOCK:
> +case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK:
> +case VIR_DOMAIN_TIMER_NAME_PIT:
> +case VIR_DOMAIN_TIMER_NAME_HPET:
> +case VIR_DOMAIN_TIMER_NAME_ARMVTIMER:
> +case VIR_DOMAIN_TIMER_NAME_LAST:
> +break;
> +case VIR_DOMAIN_TIMER_NAME_RTC:
> +if (!timer->present)
> +break;

This field is not a boolean, it is a tri-state.

 * -1 == the hypervisor default - historically with LXC this means
 no /dev/rtc device
 * 0 == disable it  (matches the default for LXC historically)
 * 1 == enable it

So this check needs to be "if (timer->present != 1) "

> +
> +if (virFileExists("/dev/rtc")) {
> +if (virCgroupAllowDevicePath(cgroup, "/dev/rtc",
> + VIR_CGROUP_DEVICE_READ,
> + false) < 0)
> +return -1;
> +} else {
> +VIR_DEBUG("Ignoring non-existent device /dev/rtc");

We should be raising an error if the user requested RTC and it doesn't
exist.

> +}
> +break;
> +}
> +}
> +} else {
> +VIR_DEBUG("Ignoring non-localtime clock");
> +}

The setup for  elements should be independant of whether or
not localtime is set.

Ideally we would raise an unsupported config error for containers
which don't have localtime set, but there's way too large a risk
of causing regressions if we do this.

I'd suggest we just don't check the clock offset at all, or at
least do the check independantly of the timer setup.

> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
> index c3dec0859c..eba6bfe0bf 100644
> --- a/src/lxc/lxc_controller.c
> +++ b/src/lxc/lxc_controller.c
> @@ -1550,6 +1550,71 @@ static int 
> virLXCControllerPopulateDevices(virLXCControllerPtr ctrl)
>  }
>  
>  
> +static int
> +virLXCControllerSetupTimers(virLXCControllerPtr ctrl)
> +{
> +g_autofree char *path = NULL;
> +size_t i;
> +struct stat sb;
> +virDomainDefPtr def = ctrl->def;
> +
> +/* Not sync'ed with Host clock */
> +if (def->clock.offset != VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME)
> +return 0;
> +
> +for (i = 0; i < def->clock.ntimers; i++) {
> +dev_t dev;
> +virDo

[libvirt-jenkins-ci PATCH 1/4] lcitool: Install dpkg-dev when doing cross-builds on Debian

[Andrea Bolognani]

When invoked as $triplet-pkg-config, pkg-config is smart enough
to search the paths corresponding to the target architecture; on
Debian, however, this currently only works when dpkg-dev is
installed, which by default it is not.

Work around the issue on our side by installing dpkg-dev
explicitly in our cross-building container images.

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=952526

Signed-off-by: Andrea Bolognani 
---
 guests/lcitool | 1 +
 1 file changed, 1 insertion(+)

diff --git a/guests/lcitool b/guests/lcitool
index 771402e..86416c4 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -811,6 +811,7 @@ class Application:
 dpkg --add-architecture {cross_arch} && \\
 {package_manager} update && \\
 {package_manager} dist-upgrade -y && \\
+{package_manager} install --no-install-recommends -y 
dpkg-dev && \\
 {package_manager} install --no-install-recommends -y 
{cross_pkgs} && \\
 {package_manager} autoremove -y && \\
 {package_manager} autoclean -y
-- 
2.24.1

[libvirt-jenkins-ci PATCH 2/4] Don't set $PKG_CONFIG_LIBDIR anywhere

[Andrea Bolognani]

When invoked as $triplet-pkg-config, pkg-config is smart enough
to search the paths corresponding to the target architecture, so
we don't need to point it to them directly.

This works for both MinGW builds and non-x86_64 Linux builds, and
avoids the problems that come from setting $PKG_CONFIG_LIBDIR in
containers, mainly that you can't use the same container image to
perform both native builds and cross builds - something that
libosinfo is alreayd doing today in their CI.

Signed-off-by: Andrea Bolognani 
---
 guests/lcitool   | 2 --
 guests/playbooks/build/jobs/defaults.yml | 2 --
 jenkins/jobs/defaults.yaml   | 2 --
 3 files changed, 6 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index 86416c4..053e46a 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -899,7 +899,6 @@ class Application:
 ENV ABI "{cross_abi}"
 ENV CONFIGURE_OPTS "--host={cross_abi} \\
 --target={cross_abi}"
-ENV PKG_CONFIG_LIBDIR "/usr/lib/{cross_lib}/pkgconfig"
 """).format(**varmap))
 
 if cross_arch and package_format == "rpm":
@@ -907,7 +906,6 @@ class Application:
 ENV ABI "{cross_abi}"
 ENV CONFIGURE_OPTS "--host={cross_abi} \\
 --target={cross_abi}"
-ENV PKG_CONFIG_LIBDIR 
"/usr/{cross_abi}/sys-root/mingw/lib/pkgconfig:/usr/{cross_abi}/sys-root/mingw/share/pkgconfig"
 """).format(**varmap))
 
 def _action_dockerfile(self, args):
diff --git a/guests/playbooks/build/jobs/defaults.yml 
b/guests/playbooks/build/jobs/defaults.yml
index 65dee78..3d5eaa0 100644
--- a/guests/playbooks/build/jobs/defaults.yml
+++ b/guests/playbooks/build/jobs/defaults.yml
@@ -37,13 +37,11 @@ strip_buildrequires: |
 mingw32_local_env: |
   export VIRT_PREFIX="$VIRT_PREFIX/i686-w64-mingw32/sys-root/mingw"
   export PKG_CONFIG_PATH="$VIRT_PREFIX/lib/pkgconfig"
-  export 
PKG_CONFIG_LIBDIR="/usr/i686-w64-mingw32/sys-root/mingw/lib/pkgconfig:/usr/i686-w64-mingw32/sys-root/mingw/share/pkgconfig"
 mingw32_autogen_args: --host=i686-w64-mingw32
 mingw32_meson_args: --cross-file=/usr/share/mingw/toolchain-mingw32.meson
 mingw64_local_env: |
   export VIRT_PREFIX="$VIRT_PREFIX/x86_64-w64-mingw32/sys-root/mingw"
   export PKG_CONFIG_PATH="$VIRT_PREFIX/lib/pkgconfig"
-  export 
PKG_CONFIG_LIBDIR="/usr/x86_64-w64-mingw32/sys-root/mingw/lib/pkgconfig:/usr/x86_64-w64-mingw32/sys-root/mingw/share/pkgconfig"
 mingw64_autogen_args: --host=x86_64-w64-mingw32
 mingw64_meson_args: --cross-file=/usr/share/mingw/toolchain-mingw64.meson
 git_urls:
diff --git a/jenkins/jobs/defaults.yaml b/jenkins/jobs/defaults.yaml
index ec429ed..8e9486d 100644
--- a/jenkins/jobs/defaults.yaml
+++ b/jenkins/jobs/defaults.yaml
@@ -33,13 +33,11 @@
 mingw32_local_env: |
   export VIRT_PREFIX="$VIRT_PREFIX/i686-w64-mingw32/sys-root/mingw"
   export PKG_CONFIG_PATH="$VIRT_PREFIX/lib/pkgconfig"
-  export 
PKG_CONFIG_LIBDIR="/usr/i686-w64-mingw32/sys-root/mingw/lib/pkgconfig:/usr/i686-w64-mingw32/sys-root/mingw/share/pkgconfig"
 mingw32_autogen_args: --host=i686-w64-mingw32
 mingw32_meson_args: --cross-file=/usr/share/mingw/toolchain-mingw32.meson
 mingw64_local_env: |
   export VIRT_PREFIX="$VIRT_PREFIX/x86_64-w64-mingw32/sys-root/mingw"
   export PKG_CONFIG_PATH="$VIRT_PREFIX/lib/pkgconfig"
-  export 
PKG_CONFIG_LIBDIR="/usr/x86_64-w64-mingw32/sys-root/mingw/lib/pkgconfig:/usr/x86_64-w64-mingw32/sys-root/mingw/share/pkgconfig"
 mingw64_autogen_args: --host=x86_64-w64-mingw32
 mingw64_meson_args: --cross-file=/usr/share/mingw/toolchain-mingw64.meson
 git_urls:
-- 
2.24.1

[libvirt-jenkins-ci PATCH 0/4] Fixes and improvements to cross-build environments

[Andrea Bolognani]

The first two commits are necessary to build container images that
can successfully be used in libosinfo's GitLab CI setup, which is
currently not performing MinGW builds because of the issues they
address.

Andrea Bolognani (4):
  lcitool: Install dpkg-dev when doing cross-builds on Debian
  Don't set $PKG_CONFIG_LIBDIR anywhere
  lcitool: Drop duplicated code
  lcitool: Don't specify --target in $CONFIGURE_OPTS

 guests/lcitool   | 15 +++
 guests/playbooks/build/jobs/defaults.yml |  2 --
 jenkins/jobs/defaults.yaml   |  2 --
 3 files changed, 3 insertions(+), 16 deletions(-)

-- 
2.24.1

[libvirt-jenkins-ci PATCH 4/4] lcitool: Don't specify --target in $CONFIGURE_OPTS

[Andrea Bolognani]

If --target is not specified, the value of --host will be used,
so having both is unnecessary; --target is mostly intended for
building toolchains, which we're not doing.

In fact, we're already not using --target in either the Jenkins
and Ansible build scripts, thus proving that we don't need it.

Signed-off-by: Andrea Bolognani 
---
 guests/lcitool | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index 80e725c..209380a 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -897,8 +897,7 @@ class Application:
 if cross_arch:
 sys.stdout.write(textwrap.dedent("""
 ENV ABI "{cross_abi}"
-ENV CONFIGURE_OPTS "--host={cross_abi} \\
---target={cross_abi}"
+ENV CONFIGURE_OPTS "--host={cross_abi}"
 """).format(**varmap))
 
 def _action_dockerfile(self, args):
-- 
2.24.1

[libvirt-jenkins-ci PATCH 3/4] lcitool: Drop duplicated code

[Andrea Bolognani]

Now that we no longer set $PKG_CONFIG_LIBDIR, there is no need to
have separate code paths for Debian and Fedora.

Signed-off-by: Andrea Bolognani 
---
 guests/lcitool | 9 +
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index 053e46a..80e725c 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -894,14 +894,7 @@ class Application:
 ENV LANG "en_US.UTF-8"
 """).format(**varmap))
 
-if cross_arch and package_format == "deb":
-sys.stdout.write(textwrap.dedent("""
-ENV ABI "{cross_abi}"
-ENV CONFIGURE_OPTS "--host={cross_abi} \\
---target={cross_abi}"
-""").format(**varmap))
-
-if cross_arch and package_format == "rpm":
+if cross_arch:
 sys.stdout.write(textwrap.dedent("""
 ENV ABI "{cross_abi}"
 ENV CONFIGURE_OPTS "--host={cross_abi} \\
-- 
2.24.1

Re: [libvirt PATCH v2 6/9] qemu: prepare and stop the dbus daemon

[Daniel P . Berrangé]

On Tue, Feb 25, 2020 at 10:55:10AM +0100, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau 
> 
> Signed-off-by: Marc-André Lureau 
> ---
>  src/qemu/qemu_process.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 8c1ed76677..3a6cb4b2b0 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -58,6 +58,7 @@
>  #include "qemu_extdevice.h"
>  #include "qemu_firmware.h"
>  #include "qemu_backup.h"
> +#include "qemu_dbus.h"
>  
>  #include "cpu/cpu.h"
>  #include "cpu/cpu_x86.h"
> @@ -6480,6 +6481,9 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver,
>  qemuDomainObjPrivatePtr priv = vm->privateData;
>  g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
>  
> +if (qemuDBusPrepareHost(driver) < 0)
> +return -1;

This launches dbus unconditionally for every VM regardless of whether
its actually going to be used.

There is certainly a nice conceptual simplicity in doing this, but
I'm pretty concerned that there are going to be mgmt apps which
do not like this extra overhead being added for every VM. I'm
thinking KubeVirt and Kata containers in particular. This is also
relevant for the libvirt embedded driver which is trying to eliminate
all libvirt added overhead on managing QEMU, so that our fastest
QEMU startup time can match that achieved by running QEMU directly.
Unconditionally starting dbus will make that much more challenging.

This is a long winded way of saying I think we need to do this
only when it is actually required. This will certainly add complexity
as we'll need to cope with dynamically launching DBus when we hotplug
certain types of device which require it. I think we can ignore the
hot-unplug case at least, as once you've taken the overhead for DBus
for the VM, I don't think there's much to complain about, even if the
device using it is unplugged.


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

[PATCH] network: bridge_driver: Use new helpers for storing libvirt errors

[Gaurav Agrawal]

From: GAURAV AGRAWAL 

Signed-off-by: Gaurav Agrawal 
---
 src/network/bridge_driver_linux.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/network/bridge_driver_linux.c 
b/src/network/bridge_driver_linux.c
index 7bbde5c6a9..ac92d884b9 100644
--- a/src/network/bridge_driver_linux.c
+++ b/src/network/bridge_driver_linux.c
@@ -22,6 +22,7 @@
 #include 
 
 #include "viralloc.h"
+#include "virerror.h"
 #include "virfile.h"
 #include "viriptables.h"
 #include "virstring.h"
@@ -53,7 +54,7 @@ static void networkSetupPrivateChains(void)
 if (rc < 0) {
 VIR_DEBUG("Failed to create global IPv4 chains: %s",
   virGetLastErrorMessage());
-errInitV4 = virSaveLastError();
+virErrorPreserveLast(&errInitV4);
 virResetLastError();
 } else {
 virFreeError(errInitV4);
@@ -70,7 +71,7 @@ static void networkSetupPrivateChains(void)
 if (rc < 0) {
 VIR_DEBUG("Failed to create global IPv6 chains: %s",
   virGetLastErrorMessage());
-errInitV6 = virSaveLastError();
+virErrorPreserveLast(&errInitV6);
 virResetLastError();
 } else {
 virFreeError(errInitV6);
@@ -790,7 +791,7 @@ int networkAddFirewallRules(virNetworkDefPtr def)
 if (errInitV4 &&
 (virNetworkDefGetIPByIndex(def, AF_INET, 0) ||
  virNetworkDefGetRouteByIndex(def, AF_INET, 0))) {
-virSetError(errInitV4);
+virErrorRestore(&errInitV4);
 return -1;
 }
 
@@ -798,7 +799,7 @@ int networkAddFirewallRules(virNetworkDefPtr def)
 (virNetworkDefGetIPByIndex(def, AF_INET6, 0) ||
  virNetworkDefGetRouteByIndex(def, AF_INET6, 0) ||
  def->ipv6nogw)) {
-virSetError(errInitV6);
+virErrorRestore(&errInitV6);
 return -1;
 }
 
-- 
2.24.1

Re: [PATCH] virt-aa-helper: Fix build by including virutil.h

[Michal Privoznik]

On 2/25/20 10:06 AM, Christian Ehrhardt wrote:



On Tue, Feb 25, 2020 at 12:36 AM Jim Fehlig > wrote:


Commit fb01e1a44d missed including virutil.h, causing the following
compilation error

../../src/security/virt-aa-helper.c:1055:43: error: implicit
declaration of
function 'virHostGetDRMRenderNode'
[-Werror=implicit-function-declaration]
1055 |                 char *defaultRenderNode =
virHostGetDRMRenderNode();

Signed-off-by: Jim Fehlig mailto:jfeh...@suse.com>>
---

Pushing under the build-breaker rule.


Thanks Jim,
it always built for me in all pre-tests which is odd.
Thanks for fixing it so fast!


In fact I think the commit that caused the problem was a different one 
(I blame the latest virutil.h include cleanup). The problem was that 
some header files included virutil.h directly and thus aa-helper code 
got it for free. After the include was removed, the helper needs to 
include virtuil.h explicitly. I haven't look at which commit exactly 
caused the problem. It doesn't really matter, does it?


Michal

Re: [PATCH v4 4/5] lxc: Implement virtual /proc/cpuinfo via LXC fuse

[Daniel P . Berrangé]

On Mon, Feb 24, 2020 at 11:24:28AM -0300, Julio Faracco wrote:
> This commit tries to fix lots of issues related to LXC VCPUs. One of
> them is related to /proc/cpuinfo content. If only 1 VCPU is set, LXC
> containers will show all CPUs available for host. The second one is
> related to CPU share, if an user set only 1 VCPU, the container/process
> will use all available CPUs. (This is not the case when `cpuset`
> attribute is declared). So, this commit adds a virtual cpuinfo based on
> VCPU mapping and it automatically limits the CPU usage according VCPU
> count.
> 
> Example (now):
> LXC container - 8 CPUS with 2 VCPU:
> lxc-root# stress --cpu 8
> 
> On host machine, only CPU 0 and 1 have 100% usage.
> 
> Signed-off-by: Julio Faracco 
> ---
>  src/lxc/lxc_cgroup.c| 31 ++
>  src/lxc/lxc_container.c | 39 ++---
>  src/lxc/lxc_fuse.c  | 95 ++---
>  3 files changed, 145 insertions(+), 20 deletions(-)
> 
> diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c
> index d29b65092a..912a252473 100644
> --- a/src/lxc/lxc_cgroup.c
> +++ b/src/lxc/lxc_cgroup.c
> @@ -50,6 +50,34 @@ static int virLXCCgroupSetupCpuTune(virDomainDefPtr def,
>  }
>  
>  
> +static int virLXCCgroupSetupVcpuAuto(virDomainDefPtr def,
> + virCgroupPtr cgroup)
> +{
> +size_t i;
> +int vcpumax;
> +virBuffer buffer = VIR_BUFFER_INITIALIZER;
> +virBufferPtr cpuset = &buffer;
> +
> +vcpumax = virDomainDefGetVcpusMax(def);
> +for (i = 0; i < vcpumax; i++) {
> +virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(def, i);
> +/* Cgroup is smart enough to convert numbers separated
> + * by comma into ranges. Example: "0,1,2,5," -> "0-2,5".
> + * Libvirt does not need to process it here. */
> +if (vcpu)
> +virBufferAsprintf(cpuset, "%zu,", i);
> +}

If I have 4 containers, each with vcpu==2, then all 4 containers
are going to be taskset onto host CPUs 0 & 1, and host CPUs 2,3,4,5,6,7
are going to be unused. This is not viable as a strategy.

> +if (virCgroupSetCpusetCpus(cgroup,
> +   virBufferCurrentContent(cpuset)) < 0) {
> +virBufferFreeAndReset(cpuset);
> +return -1;
> +}
> +
> +virBufferFreeAndReset(cpuset);
> +return 0;
> +}

> +static int
> +lxcProcReadCpuinfoParse(virDomainDefPtr def, char *base,
> +virBufferPtr new_cpuinfo)
> +{
> +char *procline = NULL;
> +char *saveptr = base;
> +size_t cpu;
> +size_t nvcpu;
> +size_t curcpu = 0;
> +bool get_proc = false;
> +
> +nvcpu = virDomainDefGetVcpus(def);
> +while ((procline = strtok_r(NULL, "\n", &saveptr))) {
> +if (sscanf(procline, "processor\t: %zu", &cpu) == 1) {

This doesn't work because it is assuming the /proc/cpuinfo data format
for x86 architecture. Every architecture puts different info in this
file, and only some of them using "processor: NN" as a delimiter for
new entries. There's many examples in tests/sysinfodata/ and
in tests/virhostcpudata/ showing this problem

We had considered taking /proc/cpuinfo in the past, but decided against
it. Since this file is non-standard data format varying across arches,
well written apps won't actually look at /proc/cpuinfo at all. Instead
they'll use /sys/devices/system/cpu instead.  I don't think we want to
make /proc/cpuinfo be inconsistent with data in sysfs.


> +virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(def, cpu);
> +/* VCPU is mapped */
> +if (vcpu) {
> +if (curcpu == nvcpu)
> +break;
> +
> +if (curcpu > 0)
> +virBufferAddLit(new_cpuinfo, "\n");
> +
> +virBufferAsprintf(new_cpuinfo, "processor\t: %zu\n",
> +  curcpu);
> +curcpu++;
> +get_proc = true;
> +} else {
> +get_proc = false;
> +}
> +} else {
> +/* It is not a processor index */
> +if (get_proc)
> +virBufferAsprintf(new_cpuinfo, "%s\n", procline);
> +}
> +}
> +
> +virBufferAddLit(new_cpuinfo, "\n");
> +
> +return strlen(virBufferCurrentContent(new_cpuinfo));
> +}

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 5/8] domain: save/restore the state of dbus-daemon running

[Michal Privoznik]

On 2/24/20 4:58 PM, Marc-André Lureau wrote:

Hi

On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik  wrote:


On 1/14/20 2:46 PM, marcandre.lur...@redhat.com wrote:

From: Marc-André Lureau 

This avoids trying to start a dbus-daemon when its already running.

Signed-off-by: Marc-André Lureau 
---
   src/qemu/qemu_domain.c | 5 +
   1 file changed, 5 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 7722a53c62..dda3cb781f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2916,6 +2916,9 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf,
 virDomainChrTypeToString(priv->monConfig->type));
   }

+if (priv->dbusDaemonRunning)
+virBufferAddLit(buf, "\n");
+
   if (priv->namespaces) {
   ssize_t ns = -1;

@@ -3697,6 +3700,8 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
   goto error;
   }

+priv->dbusDaemonRunning = virXPathBoolean("boolean(./dbusDaemon)", ctxt) > 
0;
+
   if ((node = virXPathNode("./namespaces", ctxt))) {
   xmlNodePtr next;




I'd push these a bit down - closer to PR daemon and slirp so that they
are grouped together.


Well, as we introduce DBus bus for the VM, it would be a foundation
for IPC/multi-process communication, not specific to slirp. So I'd
leave it near the top.



My reasoning was that while in private data the order doesn't matter 
really, but so far we keep internal flags towards the beginning and 
helper daemons related flags towards the end. That is not to say that 
slirp and dbus have something in common. But apparently, our 
parser/formater are out of order anyway. Ideally, we should have the 
same order for parsing/formatting as defined in the struct. But that 
ship sailed long ago.


Michal

Re: [PATCH 3/8] qemu-conf: add dbusStateDir

[Michal Privoznik]

On 2/24/20 4:57 PM, Marc-André Lureau wrote:

Hi

On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik  wrote:


On 1/14/20 2:46 PM, marcandre.lur...@redhat.com wrote:

From: Marc-André Lureau 

Location of DBus daemon state configuration, socket, pid...

Signed-off-by: Marc-André Lureau 
---
   src/qemu/qemu_conf.c | 4 
   src/qemu/qemu_conf.h | 1 +
   2 files changed, 5 insertions(+)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index e1fea390c7..ade12dac6c 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -144,6 +144,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool 
privileged)

   cfg->cacheDir = g_strdup_printf("%s/cache/libvirt/qemu", 
LOCALSTATEDIR);

+cfg->dbusStateDir = g_strdup_printf("%s/run/libvirt/qemu/dbus", 
LOCALSTATEDIR);
+
   cfg->libDir = g_strdup_printf("%s/lib/libvirt/qemu", LOCALSTATEDIR);
   cfg->saveDir = g_strdup_printf("%s/save", cfg->libDir);
   cfg->snapshotDir = g_strdup_printf("%s/snapshot", cfg->libDir);
@@ -174,6 +176,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool 
privileged)
   cfg->stateDir = g_strdup_printf("%s/qemu/run", rundir);

   cfg->swtpmStateDir = g_strdup_printf("%s/swtpm", cfg->stateDir);
+cfg->dbusStateDir = g_strdup_printf("%s/dbus", cfg->stateDir);

   cfg->configBaseDir = virGetUserConfigDirectory();


Instead of doing practically the same in two branches, you can achieve
the same result with just one line if you put the call just below
cfg->slirpStateDir init.

However, do we need this to be in a special directory instead of per VM
private directory? The way I implemented PR helper was that the socket
it creates and to which qemu connects is stored under vm->priv->libDir
which is initialized in qemuDomainSetPrivatePaths() to:

$cfg->libDir/domain-$shortName/

This way you wouldn't need to care about domain's shortname in the next
patch.


Makes sense. I think I based this on slirpStateDir code. Any reason
it's not using vm->priv->libdir too?



I don't know. But since there are some releases which would store data 
under slirpStateDir I don't think we can change this. On daemon restart 
(with newer version) the new libvirtd wouldn't see the old dir.


Michal

Re: [libvirt PATCH] docs: add page describing the libvirt daemons

[Daniel P . Berrangé]

On Tue, Feb 25, 2020 at 10:26:32AM +0100, Erik Skultety wrote:
> On Mon, Feb 24, 2020 at 06:20:54PM +, Daniel P. Berrangé wrote:
> > Now that we have more than just the libvirtd daemon, we should be
> > explaining to users what they are all for & important aspects of their
> > configuration.
> >
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  docs/daemons.rst  | 682 ++
> >  docs/docs.html.in |   3 +
> >  2 files changed, 685 insertions(+)
> >  create mode 100644 docs/daemons.rst
> >
> > diff --git a/docs/daemons.rst b/docs/daemons.rst
> > new file mode 100644
> > index 00..a74b228025
> > --- /dev/null
> > +++ b/docs/daemons.rst
> > @@ -0,0 +1,682 @@
> > +===
> > +Libvirt Daemons
> > +===
> > +
> > +.. contents::
> > +
> > +A libvirt deployment for accessing one of the stateful drivers will require
> > +one or more daemons to be deployed on the virtualization host. There are a
> > +number of ways the daemons can be configured which will be outlined in this
> > +page.
> > +
> > +Architectural options
> > +=
> > +
> > +Monolithic vs modular daemons
> > +-
> > +
> > +Traditionally libvirt provided a single monolithic daemon called 
> > ``libvirtd``
> > +which exposed support for all the stateful drivers, both primary hypervisor
> > +drivers and secondary supporting drivers. It also enables secure remote 
> > access
> > +from clients running off host.
> > +
> > +Work is underway for the monolithic daemon to be replaced by a new set of
> > +modular daemons ``virt${DRIVER}d``, each one servicing a single stateful
> > +driver. A further ``virtproxyd`` daemon will provide secure remote access, 
> > as
> > +well as backcompatibility for clients using the UNIX socket path of the
> > +monolithic daemon.
> > +
> > +The change to modular daemons should not affect API functionality used by
> > +management applications. It will, however, have an impact on host 
> > provisioning
> > +tools since there are new systemd services and configuration files to be
> > +managed.
> > +
> > +Currently both monolithic and modular daemons are built by default, but 
> > the RPC
> > +client still prefers connecting to the monolithic daemon. It is intended to
> > +switch the RPC client to prefer the modular daemons in the near future. At
> > +least 1 year after this switch (but not more than 2 years), the monolithic
> > +daemon will be deleted entirely.
> > +
> > +Operating modes
> > +---
> > +
> > +The libvirt daemons, whether monolithic or modular, can often operate in 
> > two
> > +modes
> > +
> > +* *System mode* - the daemon is running as the root user account, enabling
> > +  access to its full range of functionality. A read-write connection to
> > +  daemons in system mode **typically implies privileges equivalent to 
> > having
> > +  a root shell**. Suitable `authentication mechanisms `__ **must
> > +  be enabled** to secure it against untrustworthy clients/users.
> > +
> > +* *Session mode* - the daemon is running as any non-root user account,
> > +  providing access to a more restricted range of functionality. Only client
> > +  apps/users running under **the same UID are permitted to connect**, thus 
> > a
> > +  connection does not imply any elevation of privileges.
> > +
> > +  Not all drivers support session mode and as such the corresponding
> > +  modular daemon may not support running in this mode
> > +
> > +
> > +Monolithic driver daemon
> > +
> > +
> > +The monolithic daemon is known as ``libvirtd`` and has historically been 
> > the
> > +default in libvirt. It is configured via the file 
> > ``/etc/libvirt/libvirtd.conf``
> > +
> > +
> > +Monolithic sockets
> > +--
> > +
> > +When running in system mode, ``libvirtd`` exposes three UNIX domain 
> > sockets, and
> > +optionally, one or two TCP sockets
> > +
> > +* ``/var/run/libvirt/libvirt-sock`` - the primary socket for accessing 
> > libvirt
> > +  APIs, with full read-write privileges. A connection to this socket gives 
> > the
> > +  client privileges that are equivalent to having a root shell. This is the
> > +  socket that most management applications connect to by default.
> > +
> > +* ``/var/run/libvirt/libvirt-sock-ro`` - the secondary socket for accessing
> > +  libvirt APIs, with limited read-only privileges. A connection to this 
> > socket
> > +  gives the ability to query the existance of objects and monitor some 
> > aspects
> 
> ^These paragraphs have essentially been copy-pasted to multiple sections of
> this document. I'd suggest explaining it once and reference the definition 
> from
> all the other respective sections, so that we'll need to add new/fix
> existing contents on a single spot only.

I considered that, but except in the case of virtproxyd and libvirtd, the
actual socket paths are different in all the other examples. I think it
is confusing to refer people ba

[libvirt PATCH] tests: fix missing test data for network port XML

[Daniel P . Berrangé]

The network port XML files were not including any usage of vlan
tags or port options, and one of the files was not even processed.

Signed-off-by: Daniel P. Berrangé 
---
 tests/virnetworkportxml2xmldata/plug-network.xml | 5 +
 tests/virnetworkportxml2xmltest.c| 1 +
 2 files changed, 6 insertions(+)

diff --git a/tests/virnetworkportxml2xmldata/plug-network.xml 
b/tests/virnetworkportxml2xmldata/plug-network.xml
index a3a8899148..8e7fc6d010 100644
--- a/tests/virnetworkportxml2xmldata/plug-network.xml
+++ b/tests/virnetworkportxml2xmldata/plug-network.xml
@@ -10,6 +10,11 @@
 
 
   
+  
+
+
+  
+  
   
   
 
diff --git a/tests/virnetworkportxml2xmltest.c 
b/tests/virnetworkportxml2xmltest.c
index 1b2175dd9d..039da96490 100644
--- a/tests/virnetworkportxml2xmltest.c
+++ b/tests/virnetworkportxml2xmltest.c
@@ -94,6 +94,7 @@ mymain(void)
 DO_TEST("plug-bridge-mactbl");
 DO_TEST("plug-direct");
 DO_TEST("plug-hostdev-pci");
+DO_TEST("plug-network");
 
 return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
 }
-- 
2.24.1

[libvirt PATCH] src: add virutil.h to more source files for geteuid() compat

[Daniel P . Berrangé]

The virutil.h header defines a geteuid() macro for Windows platforms.
This fixes a few missed cases from:

  commit b11e8cccdd5163727fd4cecda0076ac2b63fe32d
  Author: Ján Tomko 
  Date:   Sun Feb 16 23:09:15 2020 +0100

Remove virutil.h from all header files

Signed-off-by: Daniel P. Berrangé 
---

Pushed to fix Windows build

 src/admin/admin_server_dispatch.c | 1 +
 src/driver.c  | 1 +
 src/util/virnetdevbandwidth.c | 1 +
 src/util/virstoragefile.c | 1 +
 tests/virlockspacetest.c  | 1 +
 5 files changed, 5 insertions(+)

diff --git a/src/admin/admin_server_dispatch.c 
b/src/admin/admin_server_dispatch.c
index 485f7d967c..7b3bd697f3 100644
--- a/src/admin/admin_server_dispatch.c
+++ b/src/admin/admin_server_dispatch.c
@@ -34,6 +34,7 @@
 #include "virstring.h"
 #include "virthreadjob.h"
 #include "virtypedparam.h"
+#include "virutil.h"
 
 #define VIR_FROM_THIS VIR_FROM_ADMIN
 
diff --git a/src/driver.c b/src/driver.c
index 2392fd7d5f..a2047beaef 100644
--- a/src/driver.c
+++ b/src/driver.c
@@ -31,6 +31,7 @@
 #include "virmodule.h"
 #include "virstring.h"
 #include "virthread.h"
+#include "virutil.h"
 #include "configmake.h"
 
 VIR_LOG_INIT("driver");
diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c
index d00ef57606..5fd7186760 100644
--- a/src/util/virnetdevbandwidth.c
+++ b/src/util/virnetdevbandwidth.c
@@ -24,6 +24,7 @@
 #include "viralloc.h"
 #include "virerror.h"
 #include "virstring.h"
+#include "virutil.h"
 
 #define VIR_FROM_THIS VIR_FROM_NONE
 
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 831ce30d4d..d75d2a689a 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -39,6 +39,7 @@
 #include "virjson.h"
 #include "virstorageencryption.h"
 #include "virsecret.h"
+#include "virutil.h"
 
 #define VIR_FROM_THIS VIR_FROM_STORAGE
 
diff --git a/tests/virlockspacetest.c b/tests/virlockspacetest.c
index 1f156ba3d6..82aef8bc2b 100644
--- a/tests/virlockspacetest.c
+++ b/tests/virlockspacetest.c
@@ -27,6 +27,7 @@
 #include "viralloc.h"
 #include "virfile.h"
 #include "virlog.h"
+#include "virutil.h"
 
 #include "virlockspace.h"
 
-- 
2.24.1

[libvirt PATCH v2 6/9] qemu: prepare and stop the dbus daemon

[marcandre . lureau]

From: Marc-André Lureau 

Signed-off-by: Marc-André Lureau 
---
 src/qemu/qemu_process.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 8c1ed76677..3a6cb4b2b0 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -58,6 +58,7 @@
 #include "qemu_extdevice.h"
 #include "qemu_firmware.h"
 #include "qemu_backup.h"
+#include "qemu_dbus.h"
 
 #include "cpu/cpu.h"
 #include "cpu/cpu_x86.h"
@@ -6480,6 +6481,9 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver,
 qemuDomainObjPrivatePtr priv = vm->privateData;
 g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
 
+if (qemuDBusPrepareHost(driver) < 0)
+return -1;
+
 if (qemuPrepareNVRAM(cfg, vm) < 0)
 return -1;
 
@@ -7408,6 +7412,8 @@ void qemuProcessStop(virQEMUDriverPtr driver,
 
 qemuExtDevicesStop(driver, vm);
 
+qemuDBusStop(driver, vm);
+
 vm->def->id = -1;
 
 /* Stop autodestroy in case guest is restarted */
-- 
2.25.0.rc2.1.g09a9a1a997

[libvirt PATCH v2 8/9] qemu-slirp: register helper for migration

[marcandre . lureau]

From: Marc-André Lureau 

When the helper supports DBus, connect it to the bus and set its ID.

If the helper supports migration, register its ID to the list of
dbus-vmstate ID to migrate, and specify --dbus-incoming when
restoring the VM.

Signed-off-by: Marc-André Lureau 
---
 src/qemu/qemu_slirp.c | 36 
 1 file changed, 36 insertions(+)

diff --git a/src/qemu/qemu_slirp.c b/src/qemu/qemu_slirp.c
index 8e001f0d10..e9b23f72a5 100644
--- a/src/qemu/qemu_slirp.c
+++ b/src/qemu/qemu_slirp.c
@@ -18,6 +18,7 @@
 
 #include 
 
+#include "qemu_dbus.h"
 #include "qemu_extdevice.h"
 #include "qemu_security.h"
 #include "qemu_slirp.h"
@@ -202,6 +203,16 @@ qemuSlirpGetFD(qemuSlirpPtr slirp)
 }
 
 
+static char *
+qemuSlirpGetDBusVMStateId(virDomainNetDefPtr net)
+{
+char macstr[VIR_MAC_STRING_BUFLEN] = "";
+
+/* can't use alias, because it's not stable across restarts */
+return g_strdup_printf("slirp-%s", virMacAddrFormat(&net->mac, macstr));
+}
+
+
 void
 qemuSlirpStop(qemuSlirpPtr slirp,
   virDomainObjPtr vm,
@@ -209,11 +220,14 @@ qemuSlirpStop(qemuSlirpPtr slirp,
   virDomainNetDefPtr net)
 {
 g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
+g_autofree char *id = qemuSlirpGetDBusVMStateId(net);
 g_autofree char *pidfile = NULL;
 virErrorPtr orig_err;
 pid_t pid;
 int rc;
 
+qemuDBusVMStateRemove(vm, id);
+
 if (!(pidfile = qemuSlirpCreatePidFilename(cfg, vm->def, 
net->info.alias))) {
 VIR_WARN("Unable to construct slirp pidfile path");
 return;
@@ -310,6 +324,28 @@ qemuSlirpStart(qemuSlirpPtr slirp,
 }
 }
 
+if (qemuSlirpHasFeature(slirp, QEMU_SLIRP_FEATURE_DBUS_ADDRESS)) {
+g_autofree char *id = qemuSlirpGetDBusVMStateId(net);
+g_autofree char *dbus_addr = qemuDBusGetAddress(driver, vm);
+
+if (qemuDBusStart(driver, vm) < 0)
+return -1;
+
+virCommandAddArgFormat(cmd, "--dbus-id=%s", id);
+
+virCommandAddArgFormat(cmd, "--dbus-address=%s", dbus_addr);
+
+if (qemuSlirpHasFeature(slirp, QEMU_SLIRP_FEATURE_MIGRATE)) {
+if (qemuDBusVMStateAdd(vm, id) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Failed to register slirp migration"));
+return -1;
+}
+if (incoming)
+virCommandAddArg(cmd, "--dbus-incoming");
+}
+}
+
 if (qemuSlirpHasFeature(slirp, QEMU_SLIRP_FEATURE_EXIT_WITH_PARENT))
 virCommandAddArg(cmd, "--exit-with-parent");
 
-- 
2.25.0.rc2.1.g09a9a1a997

[libvirt PATCH v2 2/9] qemu-conf: add configurable dbus-daemon location

[marcandre . lureau]

From: Marc-André Lureau 

Signed-off-by: Marc-André Lureau 
---
 m4/virt-driver-qemu.m4 | 6 ++
 src/qemu/libvirtd_qemu.aug | 1 +
 src/qemu/qemu.conf | 3 +++
 src/qemu/qemu_conf.c   | 5 +
 src/qemu/qemu_conf.h   | 1 +
 src/qemu/test_libvirtd_qemu.aug.in | 1 +
 6 files changed, 17 insertions(+)

diff --git a/m4/virt-driver-qemu.m4 b/m4/virt-driver-qemu.m4
index a1d2c66bba..886261fce5 100644
--- a/m4/virt-driver-qemu.m4
+++ b/m4/virt-driver-qemu.m4
@@ -110,6 +110,12 @@ AC_DEFUN([LIBVIRT_DRIVER_CHECK_QEMU], [
[/usr/bin:/usr/libexec])
   AC_DEFINE_UNQUOTED([QEMU_SLIRP_HELPER], ["$QEMU_SLIRP_HELPER"],
  [QEMU slirp helper])
+
+  AC_PATH_PROG([QEMU_DBUS_DAEMON], [dbus-daemon],
+   [/usr/bin/dbus-daemon],
+   [/usr/bin:/usr/libexec])
+  AC_DEFINE_UNQUOTED([QEMU_DBUS_DAEMON], ["$QEMU_DBUS_DAEMON"],
+ [QEMU dbus daemon])
 ])
 
 AC_DEFUN([LIBVIRT_DRIVER_RESULT_QEMU], [
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index 557b6f38f8..58a2b6416f 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -89,6 +89,7 @@ module Libvirtd_qemu =
  | str_entry "bridge_helper"
  | str_entry "pr_helper"
  | str_entry "slirp_helper"
+ | str_entry "dbus_daemon"
  | bool_entry "set_process_name"
  | int_entry "max_processes"
  | int_entry "max_files"
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index b6805ffc41..72ad4d4a10 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -828,6 +828,9 @@
 # Path to the SLIRP networking helper.
 #slirp_helper = "/usr/bin/slirp-helper"
 
+# Path to the dbus-daemon
+#dbus_daemon = "/usr/bin/dbus-daemon"
+
 # User for the swtpm TPM Emulator
 #
 # Default is 'tss'; this is the same user that tcsd (TrouSerS) installs
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 0357501dc6..c0be78261f 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -261,6 +261,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool 
privileged,
 cfg->bridgeHelperName = g_strdup(QEMU_BRIDGE_HELPER);
 cfg->prHelperName = g_strdup(QEMU_PR_HELPER);
 cfg->slirpHelperName = g_strdup(QEMU_SLIRP_HELPER);
+cfg->dbusDaemonName = g_strdup(QEMU_DBUS_DAEMON);
 
 cfg->securityDefaultConfined = true;
 cfg->securityRequireConfined = false;
@@ -347,6 +348,7 @@ static void virQEMUDriverConfigDispose(void *obj)
 VIR_FREE(cfg->bridgeHelperName);
 VIR_FREE(cfg->prHelperName);
 VIR_FREE(cfg->slirpHelperName);
+VIR_FREE(cfg->dbusDaemonName);
 
 VIR_FREE(cfg->saveImageFormat);
 VIR_FREE(cfg->dumpImageFormat);
@@ -638,6 +640,9 @@ virQEMUDriverConfigLoadProcessEntry(virQEMUDriverConfigPtr 
cfg,
 if (virConfGetValueString(conf, "slirp_helper", &cfg->slirpHelperName) < 0)
 return -1;
 
+if (virConfGetValueString(conf, "dbus_daemon", &cfg->dbusDaemonName) < 0)
+return -1;
+
 if (virConfGetValueBool(conf, "set_process_name", &cfg->setProcessName) < 
0)
 return -1;
 if (virConfGetValueUInt(conf, "max_processes", &cfg->maxProcesses) < 0)
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index cedf232223..07c1944998 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -155,6 +155,7 @@ struct _virQEMUDriverConfig {
 char *bridgeHelperName;
 char *prHelperName;
 char *slirpHelperName;
+char *dbusDaemonName;
 
 bool macFilter;
 
diff --git a/src/qemu/test_libvirtd_qemu.aug.in 
b/src/qemu/test_libvirtd_qemu.aug.in
index dd90edf687..95540a4b5e 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -104,6 +104,7 @@ module Test_libvirtd_qemu =
 { "memory_backing_dir" = "/var/lib/libvirt/qemu/ram" }
 { "pr_helper" = "/usr/bin/qemu-pr-helper" }
 { "slirp_helper" = "/usr/bin/slirp-helper" }
+{ "dbus_daemon" = "/usr/bin/dbus-daemon" }
 { "swtpm_user" = "tss" }
 { "swtpm_group" = "tss" }
 { "capability_filters"
-- 
2.25.0.rc2.1.g09a9a1a997

[libvirt PATCH v2 7/9] qemu: add dbus-vmstate helper migration support

[marcandre . lureau]

From: Marc-André Lureau 

Helper processes may have their state migrated with QEMU data stream
thanks to the QEMU "dbus-vmstate".

libvirt maintains the list of helpers to be migrated. The
"dbus-vmstate" is added when required, and given the list of helper
Ids that must be migrated, on save & load sides.

See also:
https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/dbus-vmstate.rst

Signed-off-by: Marc-André Lureau 
---
 src/qemu/qemu_alias.c|  7 +++
 src/qemu/qemu_alias.h|  2 +
 src/qemu/qemu_command.c  | 54 
 src/qemu/qemu_command.h  |  3 ++
 src/qemu/qemu_dbus.c | 14 ++
 src/qemu/qemu_dbus.h |  4 ++
 src/qemu/qemu_domain.c   | 12 ++
 src/qemu/qemu_domain.h   |  5 +++
 src/qemu/qemu_hotplug.c  | 82 
 src/qemu/qemu_hotplug.h  |  8 
 src/qemu/qemu_migration.c| 51 ++
 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 +++
 15 files changed, 286 insertions(+)

diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
index 61f8ce05c9..d2e1ce155e 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -840,3 +840,10 @@ qemuDomainGetUnmanagedPRAlias(const char *parentalias)
 
 return ret;
 }
+
+
+const char *
+qemuDomainGetDBusVMStateAlias(void)
+{
+return "dbus-vmstate0";
+}
diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h
index aaac09a1d1..e3492116c5 100644
--- a/src/qemu/qemu_alias.h
+++ b/src/qemu/qemu_alias.h
@@ -95,3 +95,5 @@ char *qemuAliasChardevFromDevAlias(const char *devAlias)
 const char *qemuDomainGetManagedPRAlias(void);
 
 char *qemuDomainGetUnmanagedPRAlias(const char *parentalias);
+
+const char *qemuDomainGetDBusVMStateAlias(void);
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 7eec7b7577..4696fd715c 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -24,6 +24,7 @@
 #include "qemu_command.h"
 #include "qemu_hostdev.h"
 #include "qemu_capabilities.h"
+#include "qemu_dbus.h"
 #include "qemu_interface.h"
 #include "qemu_alias.h"
 #include "qemu_security.h"
@@ -9516,6 +9517,56 @@ qemuBuildPflashBlockdevCommandLine(virCommandPtr cmd,
 }
 
 
+virJSONValuePtr
+qemuBuildDBusVMStateInfoProps(virQEMUDriverPtr driver,
+  virDomainObjPtr vm)
+{
+virJSONValuePtr ret = NULL;
+const char *alias = qemuDomainGetDBusVMStateAlias();
+g_autofree char *addr = qemuDBusGetAddress(driver, vm);
+
+if (!addr)
+return NULL;
+
+qemuMonitorCreateObjectProps(&ret,
+ "dbus-vmstate", alias,
+ "s:addr", addr, NULL);
+return ret;
+}
+
+
+static int
+qemuBuildDBusVMStateCommandLine(virCommandPtr cmd,
+virQEMUDriverPtr driver,
+virDomainObjPtr vm)
+{
+g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
+g_autoptr(virJSONValue) props = NULL;
+qemuDomainObjPrivatePtr priv = QEMU_DOMAIN_PRIVATE(vm);
+
+if (virStringListLength((const char **)priv->dbusVMStateIds) == 0)
+return 0;
+
+if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DBUS_VMSTATE)) {
+VIR_INFO("dbus-vmstate object is not supported by this QEMU binary");
+return 0;
+}
+
+if (!(props = qemuBuildDBusVMStateInfoProps(driver, vm)))
+return -1;
+
+if (virQEMUBuildObjectCommandlineFromJSON(&buf, props) < 0)
+return -1;
+
+virCommandAddArg(cmd, "-object");
+virCommandAddArgBuffer(cmd, &buf);
+
+priv->dbusVMState = true;
+
+return 0;
+}
+
+
 /**
  * qemuBuildCommandLineValidate:
  *
@@ -9747,6 +9798,9 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
 if (qemuBuildMasterKeyCommandLine(cmd, priv) < 0)
 return NULL;
 
+if (qemuBuildDBusVMStateCommandLine(cmd, driver, vm) < 0)
+return NULL;
+
 if (qemuBuildManagedPRCommandLine(cmd, def, priv) < 0)
 return NULL;
 
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index d4927d2191..bc3ba44fb3 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -59,6 +59,9 @@ virCommandPtr qemuBuildCommandLine(virQEMUDriverPtr driver,
 virJSONValuePtr qemuBuildPRManagerInfoProps(virStorageSourcePtr src);
 virJSONValuePtr qemuBuildPRManagedManagerInfoProps(qemuDomainObjPrivatePtr 
priv);
 
+virJSONValuePtr qemuBuildDBusVMStateInfoProps(virQEMUDriverPtr driver,
+  virDomainObjPtr vm);
+
 /* Generate the object properties for a secret */
 int qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo,
  virJSONValuePtr *propsret);
diff --git a/src/qemu/qemu_dbus.c b/src/qemu/qemu_dbus.c
index 383efa0209..1bce6ffc69 100644
--- a/src/qemu/qemu_dbus.c
+++ b/src/qemu/qemu_dbus.c
@@ -280,3 +280,17 @@ qemuDBusStart(virQEMUDriverPtr 

[libvirt PATCH v2 9/9] WIP: qemu-slirp: update to follow current spec

[marcandre . lureau]

From: Marc-André Lureau 

The WIP specification is hosted on slirp wiki at this point:
https://gitlab.freedesktop.org/slirp/libslirp/-/wikis/Slirp-Helper

We would need more feedback from various parties (including libvirt,
podman, and other developpers) before declaring a frozen version.

So for now, follow it, and feedback welcome!

Signed-off-by: Marc-André Lureau 
---
 src/qemu/qemu_slirp.c | 32 +---
 1 file changed, 9 insertions(+), 23 deletions(-)

diff --git a/src/qemu/qemu_slirp.c b/src/qemu/qemu_slirp.c
index e9b23f72a5..5e1fc1cf47 100644
--- a/src/qemu/qemu_slirp.c
+++ b/src/qemu/qemu_slirp.c
@@ -293,35 +293,21 @@ qemuSlirpStart(qemuSlirpPtr slirp,
 const virNetDevIPAddr *ip = net->guestIP.ips[i];
 g_autofree char *addr = NULL;
 const char *opt = "";
+unsigned prefix = ip->prefix;
 
 if (!(addr = virSocketAddrFormat(&ip->address)))
 return -1;
 
-if (VIR_SOCKET_ADDR_IS_FAMILY(&ip->address, AF_INET))
+if (VIR_SOCKET_ADDR_IS_FAMILY(&ip->address, AF_INET)) {
 opt = "--net";
-if (VIR_SOCKET_ADDR_IS_FAMILY(&ip->address, AF_INET6))
-opt = "--prefix-ipv6";
-
-virCommandAddArgFormat(cmd, "%s=%s", opt, addr);
-
-if (ip->prefix) {
-if (VIR_SOCKET_ADDR_IS_FAMILY(&ip->address, AF_INET)) {
-virSocketAddr netmask;
-g_autofree char *netmaskStr = NULL;
-
-if (virSocketAddrPrefixToNetmask(ip->prefix, &netmask, 
AF_INET) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("Failed to translate prefix %d to 
netmask"),
-   ip->prefix);
-return -1;
-}
-if (!(netmaskStr = virSocketAddrFormat(&netmask)))
-return -1;
-virCommandAddArgFormat(cmd, "--mask=%s", netmaskStr);
-}
-if (VIR_SOCKET_ADDR_IS_FAMILY(&ip->address, AF_INET6))
-virCommandAddArgFormat(cmd, "--prefix-length-ipv6=%u", 
ip->prefix);
+prefix = prefix ?: 24;
+}
+if (VIR_SOCKET_ADDR_IS_FAMILY(&ip->address, AF_INET6)) {
+opt = "--net6";
+prefix = prefix ?: 64;
 }
+
+virCommandAddArgFormat(cmd, "%s=%s/%u", opt, addr, prefix);
 }
 
 if (qemuSlirpHasFeature(slirp, QEMU_SLIRP_FEATURE_DBUS_ADDRESS)) {
-- 
2.25.0.rc2.1.g09a9a1a997

[libvirt PATCH v2 5/9] domain: save/restore the state of dbus-daemon running

[marcandre . lureau]

From: Marc-André Lureau 

This avoids trying to start a dbus-daemon when its already running.

Signed-off-by: Marc-André Lureau 
---
 src/qemu/qemu_domain.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 4c6814a676..663938c9ff 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2882,6 +2882,9 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf,
   virDomainChrTypeToString(priv->monConfig->type));
 }
 
+if (priv->dbusDaemonRunning)
+virBufferAddLit(buf, "\n");
+
 if (priv->namespaces) {
 ssize_t ns = -1;
 
@@ -3634,6 +3637,8 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
 goto error;
 }
 
+priv->dbusDaemonRunning = virXPathBoolean("boolean(./dbusDaemon)", ctxt) > 
0;
+
 if ((node = virXPathNode("./namespaces", ctxt))) {
 xmlNodePtr next;
 
-- 
2.25.0.rc2.1.g09a9a1a997

[libvirt PATCH v2 4/9] qemu: add a DBus daemon helper unit

[marcandre . lureau]

From: Marc-André Lureau 

Add a unit to start & stop a private dbus-daemon.

The daemon is meant to be started on demand, and associated with a
QEMU process. It should be stopped when the QEMU process is stopped.

The current policy is permissive like a session bus. Stricter
policies can be added later, following recommendations from:
https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/dbus.rst

Signed-off-by: Marc-André Lureau 
---
 po/POTFILES.in   |   1 +
 src/qemu/Makefile.inc.am |   2 +
 src/qemu/qemu_dbus.c | 282 +++
 src/qemu/qemu_dbus.h |  33 +
 src/qemu/qemu_domain.h   |   2 +
 5 files changed, 320 insertions(+)
 create mode 100644 src/qemu/qemu_dbus.c
 create mode 100644 src/qemu/qemu_dbus.h

diff --git a/po/POTFILES.in b/po/POTFILES.in
index 2d54623dc7..fe361204bb 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -150,6 +150,7 @@
 @SRCDIR@/src/qemu/qemu_checkpoint.c
 @SRCDIR@/src/qemu/qemu_command.c
 @SRCDIR@/src/qemu/qemu_conf.c
+@SRCDIR@/src/qemu/qemu_dbus.c
 @SRCDIR@/src/qemu/qemu_domain.c
 @SRCDIR@/src/qemu/qemu_domain_address.c
 @SRCDIR@/src/qemu/qemu_driver.c
diff --git a/src/qemu/Makefile.inc.am b/src/qemu/Makefile.inc.am
index 2b8517ecff..94a333f855 100644
--- a/src/qemu/Makefile.inc.am
+++ b/src/qemu/Makefile.inc.am
@@ -13,6 +13,8 @@ QEMU_DRIVER_SOURCES = \
qemu/qemu_capabilities.h \
qemu/qemu_command.c \
qemu/qemu_command.h \
+   qemu/qemu_dbus.c \
+   qemu/qemu_dbus.h \
qemu/qemu_domain.c \
qemu/qemu_domain.h \
qemu/qemu_domain_address.c \
diff --git a/src/qemu/qemu_dbus.c b/src/qemu/qemu_dbus.c
new file mode 100644
index 00..383efa0209
--- /dev/null
+++ b/src/qemu/qemu_dbus.c
@@ -0,0 +1,282 @@
+/*
+ * qemu_dbus.c: QEMU dbus daemon
+ *
+ * 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 "qemu_extdevice.h"
+#include "qemu_dbus.h"
+#include "qemu_security.h"
+
+#include "viralloc.h"
+#include "virlog.h"
+#include "virstring.h"
+#include "virtime.h"
+#include "virpidfile.h"
+
+#define VIR_FROM_THIS VIR_FROM_NONE
+
+VIR_LOG_INIT("qemu.dbus");
+
+
+int
+qemuDBusPrepareHost(virQEMUDriverPtr driver)
+{
+g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
+
+return virDirCreate(cfg->dbusStateDir, 0770, cfg->user, cfg->group,
+VIR_DIR_CREATE_ALLOW_EXIST);
+}
+
+
+static char *
+qemuDBusCreatePidFilename(virQEMUDriverConfigPtr cfg,
+  const char *shortName)
+{
+g_autofree char *name = g_strdup_printf("%s-dbus", shortName);
+
+return virPidFileBuildPath(cfg->dbusStateDir, name);
+}
+
+
+static char *
+qemuDBusCreateFilename(const char *stateDir,
+   const char *shortName,
+   const char *ext)
+{
+g_autofree char *name = g_strdup_printf("%s-dbus", shortName);
+
+return virFileBuildPath(stateDir, name,  ext);
+}
+
+
+static char *
+qemuDBusCreateSocketPath(virQEMUDriverConfigPtr cfg,
+ const char *shortName)
+{
+return qemuDBusCreateFilename(cfg->dbusStateDir, shortName, ".sock");
+}
+
+
+static char *
+qemuDBusCreateConfPath(virQEMUDriverConfigPtr cfg,
+   const char *shortName)
+{
+return qemuDBusCreateFilename(cfg->dbusStateDir, shortName, ".conf");
+}
+
+
+char *
+qemuDBusGetAddress(virQEMUDriverPtr driver,
+   virDomainObjPtr vm)
+{
+g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
+g_autofree char *shortName = virDomainDefGetShortName(vm->def);
+g_autofree char *path = NULL;
+
+if (!shortName)
+return NULL;
+
+path = qemuDBusCreateSocketPath(cfg, shortName);
+
+return g_strdup_printf("unix:path=%s", path);
+}
+
+
+static int
+qemuDBusWriteConfig(const char *filename, const char *path)
+{
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+g_autofree char *config = NULL;
+
+virBufferAddLit(&buf, "http://www.freedesktop.org/standards/dbus/1.0/busconfig.dtd\";>\n");
+virBufferAddLit(&buf, "\n");
+virBufferAdjustIndent(&buf, 2);
+
+virBufferAddLit(&buf, "org.libvirt.qemu\n");
+virBufferAsprintf(&buf, "unix:path=%s\n", path);
+virBufferAddLit(&buf, "EXTERNAL\n");
+
+virBufferAddLit(&buf, "\n");
+virBufferAdjustIndent(&buf, 2);
+virBufferAddLit(&b

[libvirt PATCH v2 3/9] qemu-conf: add dbusStateDir

[marcandre . lureau]

From: Marc-André Lureau 

Location of DBus daemon state configuration, socket, pid...

Signed-off-by: Marc-André Lureau 
---
 src/qemu/qemu_conf.c | 2 ++
 src/qemu/qemu_conf.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index c0be78261f..2309086576 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -225,6 +225,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool 
privileged,
 cfg->configDir = g_strdup_printf("%s/qemu", cfg->configBaseDir);
 cfg->autostartDir = g_strdup_printf("%s/qemu/autostart", 
cfg->configBaseDir);
 cfg->slirpStateDir = g_strdup_printf("%s/slirp", cfg->stateDir);
+cfg->dbusStateDir = g_strdup_printf("%s/dbus", cfg->stateDir);
 
 /* Set the default directory to find TLS X.509 certificates.
  * This will then be used as a fallback if the service specific
@@ -308,6 +309,7 @@ static void virQEMUDriverConfigDispose(void *obj)
 VIR_FREE(cfg->stateDir);
 VIR_FREE(cfg->swtpmStateDir);
 VIR_FREE(cfg->slirpStateDir);
+VIR_FREE(cfg->dbusStateDir);
 
 VIR_FREE(cfg->libDir);
 VIR_FREE(cfg->cacheDir);
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 07c1944998..e67b4cc342 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -97,6 +97,7 @@ struct _virQEMUDriverConfig {
 char *stateDir;
 char *swtpmStateDir;
 char *slirpStateDir;
+char *dbusStateDir;
 /* These two directories are ones QEMU processes use (so must match
  * the QEMU user/group */
 char *libDir;
-- 
2.25.0.rc2.1.g09a9a1a997

[libvirt PATCH v2 0/9] Second take on slirp-helper & dbus-vmstate

[marcandre . lureau]

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

v2:
- merge most suggestions/changes from Michal Privoznik review of v1.
- added "WIP: qemu_slirp: update to follow current spec"

Marc-André Lureau (9):
  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
  WIP: qemu-slirp: update to follow current spec

 m4/virt-driver-qemu.m4 |   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|  81 +++--
 src/qemu/qemu_command.h|   6 +-
 src/qemu/qemu_conf.c   |   7 +
 src/qemu/qemu_conf.h   |   2 +
 src/qemu/qemu_dbus.c   | 264 +
 src/qemu/qemu_dbus.h   |  25 ++-
 src/qemu/qemu_domain.c |  30 ++--
 src/qemu/qemu_domain.h |   8 +-
 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  | 157 +++--
 src/qemu/qemu_slirp.h  |   4 +-
 src/qemu/test_libvirtd_qemu.aug.in |   1 +
 25 files changed, 544 insertions(+), 364 deletions(-)

-- 
2.25.0.rc2.1.g09a9a1a997

[libvirt PATCH v2 1/9] qemu: remove dbus-vmstate code

[marcandre . lureau]

From: Marc-André Lureau 

This code was based on a per-helper instance and peer-to-peer
connections. The code that landed in qemu master for v5.0 is relying
on a single instance and DBus bus.

Instead of trying to adapt the existing dbus-vmstate code, let's
remove it and resubmit. That should make reviewing easier.

Signed-off-by: Marc-André Lureau 
---
 po/POTFILES.in|   1 -
 src/qemu/Makefile.inc.am  |   2 -
 src/qemu/qemu_alias.c |  16 -
 src/qemu/qemu_alias.h |   3 -
 src/qemu/qemu_command.c   |  83 -
 src/qemu/qemu_command.h   |   3 -
 src/qemu/qemu_dbus.c  |  94 
 src/qemu/qemu_dbus.h  |  42 -
 src/qemu/qemu_domain.c|  13 
 src/qemu/qemu_domain.h|   1 -
 src/qemu/qemu_extdevice.c |   4 +-
 src/qemu/qemu_hotplug.c   |  83 +
 src/qemu/qemu_hotplug.h   |  11 
 src/qemu/qemu_migration.c |   8 ---
 src/qemu/qemu_slirp.c | 125 +-
 src/qemu/qemu_slirp.h |   4 +-
 16 files changed, 7 insertions(+), 486 deletions(-)
 delete mode 100644 src/qemu/qemu_dbus.c
 delete mode 100644 src/qemu/qemu_dbus.h

diff --git a/po/POTFILES.in b/po/POTFILES.in
index fe361204bb..2d54623dc7 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -150,7 +150,6 @@
 @SRCDIR@/src/qemu/qemu_checkpoint.c
 @SRCDIR@/src/qemu/qemu_command.c
 @SRCDIR@/src/qemu/qemu_conf.c
-@SRCDIR@/src/qemu/qemu_dbus.c
 @SRCDIR@/src/qemu/qemu_domain.c
 @SRCDIR@/src/qemu/qemu_domain_address.c
 @SRCDIR@/src/qemu/qemu_driver.c
diff --git a/src/qemu/Makefile.inc.am b/src/qemu/Makefile.inc.am
index 94a333f855..2b8517ecff 100644
--- a/src/qemu/Makefile.inc.am
+++ b/src/qemu/Makefile.inc.am
@@ -13,8 +13,6 @@ QEMU_DRIVER_SOURCES = \
qemu/qemu_capabilities.h \
qemu/qemu_command.c \
qemu/qemu_command.h \
-   qemu/qemu_dbus.c \
-   qemu/qemu_dbus.h \
qemu/qemu_domain.c \
qemu/qemu_domain.h \
qemu/qemu_domain_address.c \
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
index 93bdcb7548..61f8ce05c9 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -840,19 +840,3 @@ qemuDomainGetUnmanagedPRAlias(const char *parentalias)
 
 return ret;
 }
-
-char *
-qemuAliasDBusVMStateFromId(const char *id)
-{
-char *ret;
-size_t i;
-
-ret = g_strdup_printf("dbus-vms-%s", id);
-
-for (i = 0; ret[i]; i++) {
-if (ret[i] == ':')
-ret[i] = '_';
-}
-
-return ret;
-}
diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h
index ae2fce16bc..aaac09a1d1 100644
--- a/src/qemu/qemu_alias.h
+++ b/src/qemu/qemu_alias.h
@@ -95,6 +95,3 @@ char *qemuAliasChardevFromDevAlias(const char *devAlias)
 const char *qemuDomainGetManagedPRAlias(void);
 
 char *qemuDomainGetUnmanagedPRAlias(const char *parentalias);
-
-char *qemuAliasDBusVMStateFromId(const char *id)
-ATTRIBUTE_NONNULL(1);
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6d5b53d30a..7eec7b7577 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -27,7 +27,6 @@
 #include "qemu_interface.h"
 #include "qemu_alias.h"
 #include "qemu_security.h"
-#include "qemu_dbus.h"
 #include "qemu_slirp.h"
 #include "qemu_block.h"
 #include "cpu/cpu.h"
@@ -9517,85 +9516,6 @@ qemuBuildPflashBlockdevCommandLine(virCommandPtr cmd,
 }
 
 
-static virJSONValuePtr
-qemuBuildDBusVMStateInfoPropsInternal(const char *alias,
-  const char *addr)
-{
-virJSONValuePtr ret = NULL;
-
-if (qemuMonitorCreateObjectProps(&ret,
- "dbus-vmstate", alias,
- "s:addr", addr, NULL) < 0)
-return NULL;
-
-return ret;
-}
-
-
-virJSONValuePtr
-qemuBuildDBusVMStateInfoProps(const char *id,
-  const char *addr)
-{
-g_autofree char *alias = qemuAliasDBusVMStateFromId(id);
-
-if (!alias)
-return NULL;
-
-return qemuBuildDBusVMStateInfoPropsInternal(alias, addr);
-}
-
-
-typedef struct qemuBuildDBusVMStateCommandLineData {
-virCommandPtr cmd;
-} qemuBuildDBusVMStateCommandLineData;
-
-
-static int
-qemuBuildDBusVMStateCommandLineEach(void *payload,
-const void *id,
-void *user_data)
-{
-qemuBuildDBusVMStateCommandLineData *data = user_data;
-qemuDBusVMStatePtr vms = payload;
-g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
-g_autoptr(virJSONValue) props = NULL;
-
-if (!(props = qemuBuildDBusVMStateInfoProps(id, vms->addr)))
-return -1;
-
-if (virQEMUBuildObjectCommandlineFromJSON(&buf, props) < 0)
-return -1;
-
-virCommandAddArg(data->cmd, "-object");
-virCommandAddArgBuffer(data->cmd, &buf);
-
-return 0;
-}
-
-static int
-qemuBuildDBusVMStateCommandLine(virCommandPtr cmd,
-qemuDomainObjPrivatePtr priv)
-{
-qemuBuildDBus

Re: [libvirt PATCH] docs: add page describing the libvirt daemons

[Erik Skultety]

On Mon, Feb 24, 2020 at 06:20:54PM +, Daniel P. Berrangé wrote:
> Now that we have more than just the libvirtd daemon, we should be
> explaining to users what they are all for & important aspects of their
> configuration.
>
> Signed-off-by: Daniel P. Berrangé 
> ---
>  docs/daemons.rst  | 682 ++
>  docs/docs.html.in |   3 +
>  2 files changed, 685 insertions(+)
>  create mode 100644 docs/daemons.rst
>
> diff --git a/docs/daemons.rst b/docs/daemons.rst
> new file mode 100644
> index 00..a74b228025
> --- /dev/null
> +++ b/docs/daemons.rst
> @@ -0,0 +1,682 @@
> +===
> +Libvirt Daemons
> +===
> +
> +.. contents::
> +
> +A libvirt deployment for accessing one of the stateful drivers will require
> +one or more daemons to be deployed on the virtualization host. There are a
> +number of ways the daemons can be configured which will be outlined in this
> +page.
> +
> +Architectural options
> +=
> +
> +Monolithic vs modular daemons
> +-
> +
> +Traditionally libvirt provided a single monolithic daemon called ``libvirtd``
> +which exposed support for all the stateful drivers, both primary hypervisor
> +drivers and secondary supporting drivers. It also enables secure remote 
> access
> +from clients running off host.
> +
> +Work is underway for the monolithic daemon to be replaced by a new set of
> +modular daemons ``virt${DRIVER}d``, each one servicing a single stateful
> +driver. A further ``virtproxyd`` daemon will provide secure remote access, as
> +well as backcompatibility for clients using the UNIX socket path of the
> +monolithic daemon.
> +
> +The change to modular daemons should not affect API functionality used by
> +management applications. It will, however, have an impact on host 
> provisioning
> +tools since there are new systemd services and configuration files to be
> +managed.
> +
> +Currently both monolithic and modular daemons are built by default, but the 
> RPC
> +client still prefers connecting to the monolithic daemon. It is intended to
> +switch the RPC client to prefer the modular daemons in the near future. At
> +least 1 year after this switch (but not more than 2 years), the monolithic
> +daemon will be deleted entirely.
> +
> +Operating modes
> +---
> +
> +The libvirt daemons, whether monolithic or modular, can often operate in two
> +modes
> +
> +* *System mode* - the daemon is running as the root user account, enabling
> +  access to its full range of functionality. A read-write connection to
> +  daemons in system mode **typically implies privileges equivalent to having
> +  a root shell**. Suitable `authentication mechanisms `__ **must
> +  be enabled** to secure it against untrustworthy clients/users.
> +
> +* *Session mode* - the daemon is running as any non-root user account,
> +  providing access to a more restricted range of functionality. Only client
> +  apps/users running under **the same UID are permitted to connect**, thus a
> +  connection does not imply any elevation of privileges.
> +
> +  Not all drivers support session mode and as such the corresponding
> +  modular daemon may not support running in this mode
> +
> +
> +Monolithic driver daemon
> +
> +
> +The monolithic daemon is known as ``libvirtd`` and has historically been the
> +default in libvirt. It is configured via the file 
> ``/etc/libvirt/libvirtd.conf``
> +
> +
> +Monolithic sockets
> +--
> +
> +When running in system mode, ``libvirtd`` exposes three UNIX domain sockets, 
> and
> +optionally, one or two TCP sockets
> +
> +* ``/var/run/libvirt/libvirt-sock`` - the primary socket for accessing 
> libvirt
> +  APIs, with full read-write privileges. A connection to this socket gives 
> the
> +  client privileges that are equivalent to having a root shell. This is the
> +  socket that most management applications connect to by default.
> +
> +* ``/var/run/libvirt/libvirt-sock-ro`` - the secondary socket for accessing
> +  libvirt APIs, with limited read-only privileges. A connection to this 
> socket
> +  gives the ability to query the existance of objects and monitor some 
> aspects

^These paragraphs have essentially been copy-pasted to multiple sections of
this document. I'd suggest explaining it once and reference the definition from
all the other respective sections, so that we'll need to add new/fix
existing contents on a single spot only.


s/existance/existence

...

> +NB, some distros will use ``/run`` instead of ``/var/run``.
> +
> +When running in session mode, ``libvirtd`` exposes two UNIX domain sockets
> +
> +* ``$XDG_RUNTIME_DIR/libvirt/libvirt-sock`` - the primary socket for 
> accessing
> +  libvirt APIs, with full read-write privileges. A connection to this socket
> +  does not alter the privileges that the client already has. This is the
> +  socket that most management applications connect to by default.
> +
> +* ``

Re: [PATCH] virt-aa-helper: Fix build by including virutil.h

[Christian Ehrhardt]

On Tue, Feb 25, 2020 at 12:36 AM Jim Fehlig  wrote:

> Commit fb01e1a44d missed including virutil.h, causing the following
> compilation error
>
> ../../src/security/virt-aa-helper.c:1055:43: error: implicit declaration of
> function 'virHostGetDRMRenderNode' [-Werror=implicit-function-declaration]
> 1055 | char *defaultRenderNode = virHostGetDRMRenderNode();
>
> Signed-off-by: Jim Fehlig 
> ---
>
> Pushing under the build-breaker rule.
>

Thanks Jim,
it always built for me in all pre-tests which is odd.
Thanks for fixing it so fast!


>  src/security/virt-aa-helper.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index 6f36652c7c..b6f58efdea 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -41,6 +41,7 @@
>  #include "virxml.h"
>  #include "viruuid.h"
>  #include "virusb.h"
> +#include "virutil.h"
>  #include "virpci.h"
>  #include "virfile.h"
>  #include "configmake.h"
> --
> 2.25.0
>
>
>

-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH 0/3] security: Don't fail if locking a file on NFS mount fails

[Peter Krempa]

On Fri, Feb 21, 2020 at 10:33:00 +0100, Michal Privoznik wrote:
> *** BLURB HERE ***
> 
> Michal Prívozník (3):
>   virSecurityManagerMetadataLock: Store locked paths
>   security: Don't remember seclabel for paths we haven't locked
> successfully
>   security: Don't fail if locking a file on NFS mount fails

I'm not quite happy about the technical debt this series is adding and
the code itself even had before. Tracking the paths and filedescriptors
in two separate arrays is one of them. The abduance of existing and
added n^2 algorithms is another. The fact that the metadata locking
function is modifying the order of the paths in the argument is also
weird and undocumented.

series:

Reviewed-by: Peter Krempa 

as this is fixing a real bug oVirt is hitting.