Re: [libvirt PATCH] docs: Add pci-addresses.rst

2020-04-14 Thread Cornelia Huck
On Tue, 14 Apr 2020 19:53:05 +0200
Andrea Bolognani  wrote:

> This document describes the relationship between PCI addresses as
> seen in the domain XML and by the guest OS, which is a topic that
> people get confused by time and time again.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  docs/formatdomain.html.in |   6 +-
>  docs/pci-addresses.rst| 184 ++
>  2 files changed, 189 insertions(+), 1 deletion(-)
>  create mode 100644 docs/pci-addresses.rst
> 

(...)

> diff --git a/docs/pci-addresses.rst b/docs/pci-addresses.rst
> new file mode 100644
> index 00..96c6466899
> --- /dev/null
> +++ b/docs/pci-addresses.rst
> @@ -0,0 +1,184 @@
> +
> +PCI addresses in domain XML and guest OS
> +
> +
> +.. contents::
> +
> +When discussing PCI addresses, it's important to understand the the
> +relationship between the addresses that can be seen in the domain XML
> +and those that are visible inside the guest OS.
> +
> +
> +Simple cases
> +

(...)

> +More complex cases
> +==

(...)

I'm wondering whether it is worth mentioning zPCI under 'More complex
cases', or maybe under 'Completely wacky cases', as the PCI addresses a
Linux guest will generate do not have any relation to whatever
addresses are used in the XML at all, but only to the zPCI attributes?



Re:Re: what a correct use for virConnectDomainEventRegisterAny API, how to Obtain a stable expected result

2020-04-14 Thread thomas.kuang
Daniel, thanks for your help.


If it cannot receive a stable expected callback, it need a timer to handle the 
timeout, in the timer timeout to release memory passed to  
virConnectDomainEventRegisterAny
or, maybe memory-leak now ...





At 2020-04-14 18:00:49, "Daniel P. Berrangé"  wrote:
>On Mon, Apr 13, 2020 at 05:32:38PM +0800, thomas.kuang wrote:
>> HI, everyone:
>> 
>> 
>> My target  deal with network hotplug use virDomainDetachDeviceFlags. Because 
>> when the API return ,the network maybe doesn’t remove from my vm guest os.
>> 
>> So I use virConnectDomainEventRegisterAny  to register an event ID: 
>> VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED ,
>> 
>>  my process as follow:
>> 
>> cb_para->call_id=virConnectDomainEventRegisterAny(cb_para->conn,cb_para->dom,VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED,
>>  VIR_DOMAIN_EVENT_CALLBACK(vnf_control_del_network_cb), cb_para, 
>> vnf_control_del_network_cb_free); 
>> 
>> 
>> 
>> flags |= VIR_DOMAIN_AFFECT_CONFIG;
>> if (virDomainIsActive(dom) == 1) {
>> flags |= VIR_DOMAIN_AFFECT_LIVE;
>> }
>> 
>> ret = virDomainDetachDeviceFlags(dom, xml, flags);
>> 
>> 
>>  
>> 
>> above code write in thread loop ,then in the same loop :
>> 
>>  
>> 
>>while (1) {
>> 
>>   mission = vnf_mission_queue_get(task);
>> 
>>   if (mission == NULL) {
>> 
>>  sleep(1);
>> 
>>  continue;
>> 
>>   } 
>> 
>>   vnf_op_process(&mission->info); // this will deal with network 
>> hotplug,will call virConnectDomainEventRegisterAny  then call  
>> virDomainDetachDeviceFlags
>> 
>>   if (mission) {
>> 
>>  vnf_mission_free(mission);
>> 
>>   } 
>> 
>>   if(virEventRunDefaultImpl() < 0) { // at here process  the 
>> registered callback for event-registered
>> 
>> printf();
>> 
>> 
>>   } 
>> 
>>}
>> 
>> My problem is: some time  , the virEventRunDefaultImpl can trigger the
>> vnf_control_del_network_cb  callback ,but some time  there is nothing,
>> as if the VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED has lost.
>> what cause the Unpredictable behavior ?  what is a correct use for
>> virConnectDomainEventRegisterAny ?
>
>The virDomainDetachDeviceFlags API is asynchronous and requires a cooperative
>guest to complete. It merely injects an ACPI PCI unplug request to the guest
>OS.  If the guest OS doesn't honour this request (because it is in early
>bootup, or is in BIOS, or is crashed, or is configured to disable hotplug,
>or is maliciously not responding), then the device will never be unplugged,
>and so you'll never get an event.
>
>So the first thing for you todo is to validate that the device really is
>being unplugged in the guest OS, and in QEMU.
>
>To check QEMU run:
>
>  virsh qemu-monitor-command --hmp $GUEST "info pci"
>
>and look to see if the PCI device is still listed in the output.
>
>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] libxl: enable Xen's e820_host setting

2020-04-14 Thread Jim Fehlig

On 4/14/20 4:07 PM, Marek Marczykowski-Górecki wrote:

On Tue, Apr 14, 2020 at 03:56:47PM -0600, Jim Fehlig wrote:

On 4/13/20 1:17 PM, Marek Marczykowski-Górecki wrote:

FWIW, in Qubes we have a patches adding e820_host setting here:
https://github.com/QubesOS/qubes-core-libvirt/
(patches 8-11)
Not submitted before, exactly to avoid adding temporary options. But
since 8+ years later it is still there, I think it's safe to assume it
will be there for some more. Or at least it's worth to unbreak some
configurations in the meantime.


BTW, the other piece of the puzzle for PCI passthrough starting with xen
4.13 is the xl.cfg 'passthrough' option. See xen commit babde47a3fe. Without
that enabled, hotplugging a PCI device to an HVM or PV domain is not
possible, with or without e820_host. AFAICT there are no patches in your
queue for the 'passthrough' option. Perhaps you haven't rebased to xen 4.13.


TBH, I don't do PCI hotplugging.


Lucky you :-).


Do you have any thoughts on modeling that option?

My thoughts are adding another xen hypervisor feature (similar to e820_host)
or map it to the new IOMMU device in libvirt [1]. I suppose it depends on
the need to support the {sync,share}_pt values vs a simple enable/disable.
If enabling and disabling passthrough is enough, I'd lean towards another
xen hypervisor feature. Otherwise we'll need to explore the IOMMU device?


Isn't IOMMU device about allowing the guest to control (v)IOMMU? I've seen
some discussion about it in context of Xen, so IMO we should not use it
now for something else.
What about adding  ?
I'm not sure about {sync,share}_pt, but probably could be squeezed there
if needed.


I suppose adding a PCI controller is one way of configuring the guest to later 
hotplug PCI devices, although I'm not sure if (sync,share}_pt could be 
considered attributes of a PCI controller. Also, xen has no notion of a PCI 
controller that I'm aware of, so it would only exist in libvirt. It seems like a 
heavy-weight approach for a setting that essentially enables or disables PCI 
passthrough.


Daniel, do you have any ideas about modeling the xl.cfg(5) 'passthrough' option? 
For convenience, see 'passthrough' under "Other Options"


https://xenbits.xen.org/docs/unstable/man/xl.cfg.5.html#Other-Options

Regards,
Jim




Re: [PATCH] libxl: enable Xen's e820_host setting

2020-04-14 Thread Marek Marczykowski-Górecki
On Tue, Apr 14, 2020 at 03:56:47PM -0600, Jim Fehlig wrote:
> On 4/13/20 1:17 PM, Marek Marczykowski-Górecki wrote:
> > FWIW, in Qubes we have a patches adding e820_host setting here:
> > https://github.com/QubesOS/qubes-core-libvirt/
> > (patches 8-11)
> > Not submitted before, exactly to avoid adding temporary options. But
> > since 8+ years later it is still there, I think it's safe to assume it
> > will be there for some more. Or at least it's worth to unbreak some
> > configurations in the meantime.
> 
> BTW, the other piece of the puzzle for PCI passthrough starting with xen
> 4.13 is the xl.cfg 'passthrough' option. See xen commit babde47a3fe. Without
> that enabled, hotplugging a PCI device to an HVM or PV domain is not
> possible, with or without e820_host. AFAICT there are no patches in your
> queue for the 'passthrough' option. Perhaps you haven't rebased to xen 4.13.

TBH, I don't do PCI hotplugging.

> Do you have any thoughts on modeling that option?
>
> My thoughts are adding another xen hypervisor feature (similar to e820_host)
> or map it to the new IOMMU device in libvirt [1]. I suppose it depends on
> the need to support the {sync,share}_pt values vs a simple enable/disable.
> If enabling and disabling passthrough is enough, I'd lean towards another
> xen hypervisor feature. Otherwise we'll need to explore the IOMMU device?

Isn't IOMMU device about allowing the guest to control (v)IOMMU? I've seen
some discussion about it in context of Xen, so IMO we should not use it
now for something else.
What about adding  ?
I'm not sure about {sync,share}_pt, but probably could be squeezed there
if needed.

> Regards,
> Jim
> 
> [1] https://libvirt.org/formatdomain.html#elementsIommu

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?


signature.asc
Description: PGP signature


Re: [PATCH v2 1/4] conf: add xen specific feature: e820_host

2020-04-14 Thread Jim Fehlig

On 4/13/20 8:37 PM, Marek Marczykowski-Górecki wrote:

e820_host is a Xen-specific option, only available for PV domains, that
provides the domain a virtual e820 memory map based on the host one. It
is required when using PCI passthrough and is generally considered safe
for any PV kernel. e820_host is silently ignored if set in HVM domain
configuration. See xl.cfg(5) man page in the Xen documentation for more
details.

Signed-off-by: Marek Marczykowski-Górecki 


This looks good to me now so

Reviewed-by: Jim Fehlig 

But before pushing I'd like to settle on mapping the new 'passthrough' xl.cfg 
option to libvirt. See 
https://www.redhat.com/archives/libvir-list/2020-April/msg00696.html


On xen 4.13 it seems 'passthrough' is enough for PV domains. Enabling it, and 
not e820_host, allows me to successfully hotplug a PCI device after PV domain 
creation. I haven't checked but I assume e820_host was needed to hotplug PCI 
devices with xen < 4.13?


Regards,
Jim




Re: [PATCH] libxl: enable Xen's e820_host setting

2020-04-14 Thread Jim Fehlig

On 4/13/20 1:17 PM, Marek Marczykowski-Górecki wrote:

FWIW, in Qubes we have a patches adding e820_host setting here:
https://github.com/QubesOS/qubes-core-libvirt/
(patches 8-11)
Not submitted before, exactly to avoid adding temporary options. But
since 8+ years later it is still there, I think it's safe to assume it
will be there for some more. Or at least it's worth to unbreak some
configurations in the meantime.


BTW, the other piece of the puzzle for PCI passthrough starting with xen 4.13 is 
the xl.cfg 'passthrough' option. See xen commit babde47a3fe. Without that 
enabled, hotplugging a PCI device to an HVM or PV domain is not possible, with 
or without e820_host. AFAICT there are no patches in your queue for the 
'passthrough' option. Perhaps you haven't rebased to xen 4.13. Do you have any 
thoughts on modeling that option?


My thoughts are adding another xen hypervisor feature (similar to e820_host) or 
map it to the new IOMMU device in libvirt [1]. I suppose it depends on the need 
to support the {sync,share}_pt values vs a simple enable/disable. If enabling 
and disabling passthrough is enough, I'd lean towards another xen hypervisor 
feature. Otherwise we'll need to explore the IOMMU device?


Regards,
Jim

[1] https://libvirt.org/formatdomain.html#elementsIommu




Re: [PATCH 1/2] docs: backup: Remove references to push backup to network disk

2020-04-14 Thread Eric Blake

On 4/14/20 2:06 PM, Peter Krempa wrote:

On Tue, Apr 14, 2020 at 12:36:56 -0500, Eric Blake wrote:

On 4/14/20 4:22 AM, Peter Krempa wrote:

It was never implemented and for now I don't think there's demand to do
it. Remove the reference.

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

Signed-off-by: Peter Krempa 
---
   docs/formatbackup.html.in | 7 +++
   1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/docs/formatbackup.html.in b/docs/formatbackup.html.in
index 55acd13ddc..87744bac98 100644
--- a/docs/formatbackup.html.in
+++ b/docs/formatbackup.html.in
@@ -97,12 +97,11 @@
 type
 A mandatory attribute to describe the type of the
   disk, except when backup='no' is
-used. Valid values include file,
-block, or network.
+used. Valid values include file, or
+block.


I think we should implement block, rather than delete it.  It matters for
the same reason that it matters in the destination of block copy: if you


I'm deleting 'network'. Block is implemented, working and tested.


Then shame on me for misreading the patch.

Okay, we may someday add network, but for now making the documentation 
match what we have as existing implementation makes sense.





want to set a highest-byte watermark threshold (to be warned by qemu when it
is time to resize the disk larger), you NEED a block device, not a file.


You can do this on a file too.


I seem to recall differences in how the two behave at the qemu level; 
but it doesn't affect this patch if we have the means for telling 
libvirt which of the two (file or block) we meant.




I'm inclined to NACK this patch.


Wouldn't mean that much since it's necessary to add schema if you want
it.


NACK withdrawn; you've convinced me that the patch (dropping 'network', 
not 'block') is reasonable for matching what we have.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



Re: [libvirt PATCH] docs: Add pci-addresses.rst

2020-04-14 Thread Laine Stump

On 4/14/20 1:53 PM, Andrea Bolognani wrote:

This document describes the relationship between PCI addresses as
seen in the domain XML and by the guest OS, which is a topic that
people get confused by time and time again.

Signed-off-by: Andrea Bolognani 
---
  docs/formatdomain.html.in |   6 +-
  docs/pci-addresses.rst| 184 ++
  2 files changed, 189 insertions(+), 1 deletion(-)
  create mode 100644 docs/pci-addresses.rst

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 6f43976815..0077666862 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -4286,7 +4286,11 @@
  element with no other attributes as an explicit request to
  assign a PCI address for the device rather than some other
  type of address that may also be appropriate for that same
-device (e.g. virtio-mmio).
+device (e.g. virtio-mmio).
+The relationship between the PCI addresses configured in the domain
+XML and those seen by the guest OS can sometime seem confusing: a
+separate document describes how PCI
+addresses work in more detail.

drive
Drive addresses have the following additional
diff --git a/docs/pci-addresses.rst b/docs/pci-addresses.rst
new file mode 100644
index 00..96c6466899
--- /dev/null
+++ b/docs/pci-addresses.rst
@@ -0,0 +1,184 @@
+
+PCI addresses in domain XML and guest OS
+
+
+.. contents::
+
+When discussing PCI addresses, it's important to understand the the
+relationship between the addresses that can be seen in the domain XML
+and those that are visible inside the guest OS.
+
+
+Simple cases
+
+
+When the PCI topology of the VM is very simple, the PCI addresses
+will usually match.
+
+For example, the domain XML snippet
+
+::
+
+  
+  
+
+
+
+  
+  
+
+
+
+  
+
+will result in the PCI topology
+
+::
+
+  :00:00.0 Host bridge: Intel Corporation 82G33/G31/P35/P31 Express DRAM 
Controller
+  :00:01.0 PCI bridge: Red Hat, Inc. QEMU PCIe Root port
+  :01:00.0 Ethernet controller: Red Hat, Inc. Virtio network device (rev 
01)
+
+showing up in the guest OS.
+
+The PCI address of the ``virtio-net`` adapter, ``:01:00.0``, is
+the same in both cases, so there's no confusion.
+
+
+More complex cases
+==
+
+In more complex cases, the PCI address visible in the domain XML will
+correlate to the one seen by the guest OS in a less obvious way.
+
+pcie-expander-bus
+-
+
+This fairly uncommon device, which can be used with ``x86_64/q35``
+guests, will help illustrate one such scenario.
+
+For example, the domain XML snippet
+
+::
+
+  
+  
+
+
+
+  
+  
+
+
+
+  
+  
+
+
+
+  
+
+will result in the PCI topology
+
+::
+
+  :00:00.0 Host bridge: Intel Corporation 82G33/G31/P35/P31 Express DRAM 
Controller
+  :00:01.0 Host bridge: Red Hat, Inc. QEMU PCIe Expander bridge
+  :fe:00.0 PCI bridge: Red Hat, Inc. QEMU PCIe Root port
+  :ff:00.0 Ethernet controller: Red Hat, Inc. Virtio network device (rev 
01)
+
+showing up in the guest OS.
+
+This time the addresses don't match: this is because the ``busNr``
+property for the ``pcie-expander-bus`` controller causes it to show
+up as bus 254 (``0xfe`` in hexadecimal) instead of bus 1 as one might
+expect based on its ``index`` property.
+
+How can the domain XML shown above work at all, then? Surely the
+``pcie-root-port`` controller and the ``virtio-net`` adapter should
+use ``bus=0xfe`` and ``bus=0xff`` respectively for the configuration
+to be accepted by libvirt?
+
+As it turns out, that's not the case. The reason for this is that
+QEMU, and consequently libvirt, uses the ``bus`` property of a
+device's PCI address only to match it with the PCI controller that
+has the same ``index`` property, and not to set the actual PCI
+address, which is decided by the guest OS.
+
+So, by looking at the XML snippet above, we can see that the
+``virtio-net`` adapter plugs into the ``pcie-root-port`` controller,
+which plugs into the ``pcie-expander-bus`` controller, which plugs
+into ``pcie-root``: the guest OS sees the same topology, but assigns
+different PCI addresses to some of its component.
+
+The takeaway is that the *relationship* between controllers are the
+very same whether you look at the domain XML or at the guest OS, but
+the *actual PCI addresses* are not guaranteed to match and in fact,
+except for the very simplest cases, they usually will not.



and it doesn't necessarily take a pcie-expander-bus to make the 
numbering appear "off". It really is 100% up to the guest OS what bus 
number is given to each bus that it discovers, and there's nothing the 
host can do about it. This is similar to the target device name for 
block devices - you can put  in the config for a disk 
as much as you w

Re: [PATCH 1/2] docs: backup: Remove references to push backup to network disk

2020-04-14 Thread Peter Krempa
On Tue, Apr 14, 2020 at 12:36:56 -0500, Eric Blake wrote:
> On 4/14/20 4:22 AM, Peter Krempa wrote:
> > It was never implemented and for now I don't think there's demand to do
> > it. Remove the reference.
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1812100
> > 
> > Signed-off-by: Peter Krempa 
> > ---
> >   docs/formatbackup.html.in | 7 +++
> >   1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/docs/formatbackup.html.in b/docs/formatbackup.html.in
> > index 55acd13ddc..87744bac98 100644
> > --- a/docs/formatbackup.html.in
> > +++ b/docs/formatbackup.html.in
> > @@ -97,12 +97,11 @@
> > type
> > A mandatory attribute to describe the type of the
> >   disk, except when backup='no' is
> > -used. Valid values include file,
> > -block, or network.
> > +used. Valid values include file, or
> > +block.
> 
> I think we should implement block, rather than delete it.  It matters for
> the same reason that it matters in the destination of block copy: if you

I'm deleting 'network'. Block is implemented, working and tested.

> want to set a highest-byte watermark threshold (to be warned by qemu when it
> is time to resize the disk larger), you NEED a block device, not a file.

You can do this on a file too.

> But libvirt treats all  as files, even when opening
> /path/to/device; you HAVE to use  when specifying a block
> device to get the behaviors needed for handling it as a block device rather
> than a file.
> 
> >   Similar to a disk declaration for a domain, the choice of 
> > type
> >   controls what additional sub-elements are needed to 
> > describe
> > -the destination (such as protocol for a
> > -network destination).
> > +the destination.
> > target
> > Valid only for push mode backups, this is the
> >   primary sub-element that describes the file name of
> > 
> 
> I'm inclined to NACK this patch.

Wouldn't mean that much since it's necessary to add schema if you want
it.

> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.   +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org



Re: [RFC] Adding docker driver to libvirt

2020-04-14 Thread Andrea Bolognani
On Tue, 2020-04-14 at 18:34 +0200, Martin Kletzander wrote:
> On Tue, Apr 14, 2020 at 09:56:24AM +0300, nshirokovskiy wrote:
> > We wanted to add Windows containers to the libvirt API. They are available
> > under docker API thus the idea to add a docker driver. The docker itself
> > uses some API to manage Windows containers but this API lacks documentation
> > thus again the willingness to use just docker API to bring Windows 
> > containers
> > to libvirt.
> 
> Oh, so with that we would be able to manage native containers on Windows?  
> That
> might be interesting then.  I did not know about that because last time I 
> heard
> about containers on Windows docker was running a Linux VM in which it spawned
> the containers =D  I did not know there's something new now.

Note that we don't build the daemon on Windows, so this would have
to be a stateless driver.

I think that's probably fine, since Docker already has its own daemon
which supports remote connections (see DOCKER_HOST in [1]).


[1] 
https://docs.docker.com/engine/reference/commandline/cli/#environment-variables
-- 
Andrea Bolognani / Red Hat / Virtualization



[libvirt PATCH] docs: Add pci-addresses.rst

2020-04-14 Thread Andrea Bolognani
This document describes the relationship between PCI addresses as
seen in the domain XML and by the guest OS, which is a topic that
people get confused by time and time again.

Signed-off-by: Andrea Bolognani 
---
 docs/formatdomain.html.in |   6 +-
 docs/pci-addresses.rst| 184 ++
 2 files changed, 189 insertions(+), 1 deletion(-)
 create mode 100644 docs/pci-addresses.rst

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 6f43976815..0077666862 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -4286,7 +4286,11 @@
 element with no other attributes as an explicit request to
 assign a PCI address for the device rather than some other
 type of address that may also be appropriate for that same
-device (e.g. virtio-mmio).
+device (e.g. virtio-mmio).
+The relationship between the PCI addresses configured in the domain
+XML and those seen by the guest OS can sometime seem confusing: a
+separate document describes how PCI
+addresses work in more detail.
   
   drive
   Drive addresses have the following additional
diff --git a/docs/pci-addresses.rst b/docs/pci-addresses.rst
new file mode 100644
index 00..96c6466899
--- /dev/null
+++ b/docs/pci-addresses.rst
@@ -0,0 +1,184 @@
+
+PCI addresses in domain XML and guest OS
+
+
+.. contents::
+
+When discussing PCI addresses, it's important to understand the the
+relationship between the addresses that can be seen in the domain XML
+and those that are visible inside the guest OS.
+
+
+Simple cases
+
+
+When the PCI topology of the VM is very simple, the PCI addresses
+will usually match.
+
+For example, the domain XML snippet
+
+::
+
+  
+  
+
+
+
+  
+  
+
+
+
+  
+
+will result in the PCI topology
+
+::
+
+  :00:00.0 Host bridge: Intel Corporation 82G33/G31/P35/P31 Express DRAM 
Controller
+  :00:01.0 PCI bridge: Red Hat, Inc. QEMU PCIe Root port
+  :01:00.0 Ethernet controller: Red Hat, Inc. Virtio network device (rev 
01)
+
+showing up in the guest OS.
+
+The PCI address of the ``virtio-net`` adapter, ``:01:00.0``, is
+the same in both cases, so there's no confusion.
+
+
+More complex cases
+==
+
+In more complex cases, the PCI address visible in the domain XML will
+correlate to the one seen by the guest OS in a less obvious way.
+
+pcie-expander-bus
+-
+
+This fairly uncommon device, which can be used with ``x86_64/q35``
+guests, will help illustrate one such scenario.
+
+For example, the domain XML snippet
+
+::
+
+  
+  
+
+
+
+  
+  
+
+
+
+  
+  
+
+
+
+  
+
+will result in the PCI topology
+
+::
+
+  :00:00.0 Host bridge: Intel Corporation 82G33/G31/P35/P31 Express DRAM 
Controller
+  :00:01.0 Host bridge: Red Hat, Inc. QEMU PCIe Expander bridge
+  :fe:00.0 PCI bridge: Red Hat, Inc. QEMU PCIe Root port
+  :ff:00.0 Ethernet controller: Red Hat, Inc. Virtio network device (rev 
01)
+
+showing up in the guest OS.
+
+This time the addresses don't match: this is because the ``busNr``
+property for the ``pcie-expander-bus`` controller causes it to show
+up as bus 254 (``0xfe`` in hexadecimal) instead of bus 1 as one might
+expect based on its ``index`` property.
+
+How can the domain XML shown above work at all, then? Surely the
+``pcie-root-port`` controller and the ``virtio-net`` adapter should
+use ``bus=0xfe`` and ``bus=0xff`` respectively for the configuration
+to be accepted by libvirt?
+
+As it turns out, that's not the case. The reason for this is that
+QEMU, and consequently libvirt, uses the ``bus`` property of a
+device's PCI address only to match it with the PCI controller that
+has the same ``index`` property, and not to set the actual PCI
+address, which is decided by the guest OS.
+
+So, by looking at the XML snippet above, we can see that the
+``virtio-net`` adapter plugs into the ``pcie-root-port`` controller,
+which plugs into the ``pcie-expander-bus`` controller, which plugs
+into ``pcie-root``: the guest OS sees the same topology, but assigns
+different PCI addresses to some of its component.
+
+The takeaway is that the *relationship* between controllers are the
+very same whether you look at the domain XML or at the guest OS, but
+the *actual PCI addresses* are not guaranteed to match and in fact,
+except for the very simplest cases, they usually will not.
+
+spapr-pci-host-bridge
+-
+
+This device, which is unique to ``ppc64/pseries`` guests, will help
+illustrate another scenario.
+
+For example, the domain XML snippet
+
+::
+
+  
+ 
+ 
+   
+   
+ 
+ 
+   
+   
+ 
+ 
+ 
+   
+
+will result in the PCI topology
+
+::
+
+  0001:00:01.0 Ethernet controller: Red Hat, Inc. Virtio network device
+
+showing up in 

Re: [libvirt PATCH 0/2] gitlab: Clean up and improve ccache usage

2020-04-14 Thread Andrea Bolognani
On Mon, 2020-03-30 at 19:31 +0200, Andrea Bolognani wrote:
> Test pipeline: https://gitlab.com/abologna/libvirt/pipelines/131128028
> 
> Andrea Bolognani (2):
>   gitlab: Don't define $MAKE
>   gitlab: Enable improved ccache usage
> 
>  .gitlab-ci.yml | 13 -
>  1 file changed, 4 insertions(+), 9 deletions(-)

alias ping="ccache ping"

-- 
Andrea Bolognani / Red Hat / Virtualization



Re: [PATCH 1/2] docs: backup: Remove references to push backup to network disk

2020-04-14 Thread Eric Blake

On 4/14/20 4:22 AM, Peter Krempa wrote:

It was never implemented and for now I don't think there's demand to do
it. Remove the reference.

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

Signed-off-by: Peter Krempa 
---
  docs/formatbackup.html.in | 7 +++
  1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/docs/formatbackup.html.in b/docs/formatbackup.html.in
index 55acd13ddc..87744bac98 100644
--- a/docs/formatbackup.html.in
+++ b/docs/formatbackup.html.in
@@ -97,12 +97,11 @@
type
A mandatory attribute to describe the type of the
  disk, except when backup='no' is
-used. Valid values include file,
-block, or network.
+used. Valid values include file, or
+block.


I think we should implement block, rather than delete it.  It matters 
for the same reason that it matters in the destination of block copy: if 
you want to set a highest-byte watermark threshold (to be warned by qemu 
when it is time to resize the disk larger), you NEED a block device, not 
a file.  But libvirt treats all  as files, even when 
opening /path/to/device; you HAVE to use  when 
specifying a block device to get the behaviors needed for handling it as 
a block device rather than a file.



  Similar to a disk declaration for a domain, the choice of type
  controls what additional sub-elements are needed to describe
-the destination (such as protocol for a
-network destination).
+the destination.
target
Valid only for push mode backups, this is the
  primary sub-element that describes the file name of



I'm inclined to NACK this patch.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



Re: [PATCH 02/43] bhyve: convert virMutex to GMutex

2020-04-14 Thread Rafael Fonseca
On Tue, Apr 14, 2020 at 6:06 PM Pavel Mores  wrote:
>
> By the way, the approach taken here with bhyveDriver{Lock,Unlock}() might make
> sense with the whole series - implement e.g. virMutexInit() in terms of
> g_mutex_init() in the first phase and only then replace the actual
> virMutexInit() calls if considered beneficial...

So you mean one patch doing 's/virMutex/GMutex' and then inside
virMutex*() we call the g_mutex_*() equivalent? And maybe make
virMutex*() `inline`?


Att.
-- 
Rafael Fonseca




Re: [PATCH 20/43] network: bridge_driver: convert virMutex to GMutex

2020-04-14 Thread Pavel Mores
On Fri, Apr 10, 2020 at 03:54:47PM +0200, Rafael Fonseca wrote:
> Signed-off-by: Rafael Fonseca 
> ---
>  src/network/bridge_driver.c  | 11 ---
>  src/network/bridge_driver_platform.h |  2 +-
>  2 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index f06099297a..2eaab9c667 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -101,14 +101,14 @@ networkGetDriver(void)
>  static void
>  networkDriverLock(virNetworkDriverStatePtr driver)
>  {
> -virMutexLock(&driver->lock);
> +g_mutex_lock(&driver->lock);
>  }
>  
>  
>  static void
>  networkDriverUnlock(virNetworkDriverStatePtr driver)
>  {
> -virMutexUnlock(&driver->lock);
> +g_mutex_unlock(&driver->lock);
>  }
>  
>  
> @@ -726,10 +726,7 @@ networkStateInitialize(bool privileged,
>  goto error;
>  
>  network_driver->lockFD = -1;
> -if (virMutexInit(&network_driver->lock) < 0) {
> -VIR_FREE(network_driver);
> -goto error;
> -}
> +g_mutex_init(&network_driver->lock);
>  
>  network_driver->privileged = privileged;
>  
> @@ -907,7 +904,7 @@ networkStateCleanup(void)
>  
>  virObjectUnref(network_driver->dnsmasqCaps);
>  
> -virMutexDestroy(&network_driver->lock);
> +g_mutex_clear(&network_driver->lock);
>  
>  VIR_FREE(network_driver);
>  
> diff --git a/src/network/bridge_driver_platform.h 
> b/src/network/bridge_driver_platform.h
> index 169417a6c0..6528bf6647 100644
> --- a/src/network/bridge_driver_platform.h
> +++ b/src/network/bridge_driver_platform.h
> @@ -29,7 +29,7 @@
>  
>  /* Main driver state */
>  struct _virNetworkDriverState {
> -virMutex lock;
> +GMutex lock;
>  
>  /* Read-only */
>  bool privileged;
> -- 
> 2.25.2
> 
> 

Reviewed-by: Pavel Mores 



Re: [PATCH 06/43] util: virtpm: convert virMutex to GMutex

2020-04-14 Thread Pavel Mores
On Fri, Apr 10, 2020 at 03:54:33PM +0200, Rafael Fonseca wrote:
> Signed-off-by: Rafael Fonseca 
> ---
>  src/util/virtpm.c | 18 +-
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/src/util/virtpm.c b/src/util/virtpm.c
> index c734bf941a..5fd6396f2f 100644
> --- a/src/util/virtpm.c
> +++ b/src/util/virtpm.c
> @@ -91,7 +91,7 @@ virTPMCreateCancelPath(const char *devpath)
>   * executables for the swtpm; to be found on the host along with
>   * capabilties bitmap
>   */
> -static virMutex swtpm_tools_lock = VIR_MUTEX_INITIALIZER;
> +G_LOCK_DEFINE_STATIC(swtpm_tools_lock);
>  static char *swtpm_path;
>  static struct stat swtpm_stat;
>  static virBitmapPtr swtpm_caps;
> @@ -113,9 +113,9 @@ virTPMGetSwtpm(void)
>  if (!swtpm_path && virTPMEmulatorInit() < 0)
>  return NULL;
>  
> -virMutexLock(&swtpm_tools_lock);
> +G_LOCK(swtpm_tools_lock);
>  s = g_strdup(swtpm_path);
> -virMutexUnlock(&swtpm_tools_lock);
> +G_UNLOCK(swtpm_tools_lock);
>  
>  return s;
>  }
> @@ -128,9 +128,9 @@ virTPMGetSwtpmSetup(void)
>  if (!swtpm_setup && virTPMEmulatorInit() < 0)
>  return NULL;
>  
> -virMutexLock(&swtpm_tools_lock);
> +G_LOCK(swtpm_tools_lock);
>  s = g_strdup(swtpm_setup);
> -virMutexUnlock(&swtpm_tools_lock);
> +G_UNLOCK(swtpm_tools_lock);
>  
>  return s;
>  }
> @@ -143,9 +143,9 @@ virTPMGetSwtpmIoctl(void)
>  if (!swtpm_ioctl && virTPMEmulatorInit() < 0)
>  return NULL;
>  
> -virMutexLock(&swtpm_tools_lock);
> +G_LOCK(swtpm_tools_lock);
>  s = g_strdup(swtpm_ioctl);
> -virMutexUnlock(&swtpm_tools_lock);
> +G_UNLOCK(swtpm_tools_lock);
>  
>  return s;
>  }
> @@ -284,7 +284,7 @@ virTPMEmulatorInit(void)
>  };
>  size_t i;
>  
> -virMutexLock(&swtpm_tools_lock);
> +G_LOCK(swtpm_tools_lock);
>  
>  for (i = 0; i < G_N_ELEMENTS(prgs); i++) {
>  g_autofree char *path = NULL;
> @@ -341,7 +341,7 @@ virTPMEmulatorInit(void)
>  ret = 0;
>  
>   cleanup:
> -virMutexUnlock(&swtpm_tools_lock);
> +G_UNLOCK(swtpm_tools_lock);
>  
>  return ret;
>  }
> -- 
> 2.25.2
> 
> 

Reviewed-by: Pavel Mores 



Re: [PATCH 04/43] conf: virchrdev: convert virMutex to GMutex

2020-04-14 Thread Pavel Mores
On Fri, Apr 10, 2020 at 03:54:31PM +0200, Rafael Fonseca wrote:
> Signed-off-by: Rafael Fonseca 
> ---
>  src/conf/virchrdev.c | 27 +++
>  1 file changed, 7 insertions(+), 20 deletions(-)
> 
> diff --git a/src/conf/virchrdev.c b/src/conf/virchrdev.c
> index 800e82869e..8280f8e188 100644
> --- a/src/conf/virchrdev.c
> +++ b/src/conf/virchrdev.c
> @@ -44,7 +44,7 @@ VIR_LOG_INIT("conf.chrdev");
>  /* structure holding information about character devices
>   * open in a given domain */
>  struct _virChrdevs {
> -virMutex lock;
> +GMutex lock;
>  virHashTablePtr hash;
>  };
>  
> @@ -238,12 +238,10 @@ static void virChrdevFDStreamCloseCb(virStreamPtr st 
> G_GNUC_UNUSED,
>void *opaque)
>  {
>  virChrdevStreamInfoPtr priv = opaque;
> -virMutexLock(&priv->devs->lock);
> +g_autoptr(GMutexLocker) locker = g_mutex_locker_new(&priv->devs->lock);
>  
>  /* remove entry from hash */
>  virHashRemoveEntry(priv->devs->hash, priv->path);
> -
> -virMutexUnlock(&priv->devs->lock);
>  }
>  
>  /**
> @@ -258,12 +256,7 @@ virChrdevsPtr virChrdevAlloc(void)
>  if (VIR_ALLOC(devs) < 0)
>  return NULL;
>  
> -if (virMutexInit(&devs->lock) < 0) {
> -virReportSystemError(errno, "%s",
> - _("Unable to init device stream mutex"));
> -VIR_FREE(devs);
> -return NULL;
> -}
> +g_mutex_init(&devs->lock);
>  
>  /* there will hardly be any devices most of the time, the hash
>   * does not have to be huge */
> @@ -299,11 +292,11 @@ void virChrdevFree(virChrdevsPtr devs)
>  if (!devs)
>  return;
>  
> -virMutexLock(&devs->lock);
> +g_mutex_lock(&devs->lock);
>  virHashForEach(devs->hash, virChrdevFreeClearCallbacks, NULL);
>  virHashFree(devs->hash);
> -virMutexUnlock(&devs->lock);
> -virMutexDestroy(&devs->lock);
> +g_mutex_unlock(&devs->lock);
> +g_mutex_clear(&devs->lock);
>  
>  VIR_FREE(devs);
>  }
> @@ -334,6 +327,7 @@ int virChrdevOpen(virChrdevsPtr devs,
>  char *path;
>  int ret;
>  bool added = false;
> +g_autoptr(GMutexLocker) locker = g_mutex_locker_new(&devs->lock);
>  
>  switch (source->type) {
>  case VIR_DOMAIN_CHR_TYPE_PTY:
> @@ -354,12 +348,9 @@ int virChrdevOpen(virChrdevsPtr devs,
>  return -1;
>  }
>  
> -virMutexLock(&devs->lock);
> -
>  if ((ent = virHashLookup(devs->hash, path))) {
>  if (!force) {
>   /* entry found, device is busy */
> -virMutexUnlock(&devs->lock);
>  return 1;
> } else {
> /* terminate existing connection */
> @@ -378,13 +369,11 @@ int virChrdevOpen(virChrdevsPtr devs,
>  
>  /* create the lock file */
>  if ((ret = virChrdevLockFileCreate(path)) < 0) {
> -virMutexUnlock(&devs->lock);
>  return ret;
>  }
>  
>  /* obtain a reference to the stream */
>  if (virStreamRef(st) < 0) {
> -virMutexUnlock(&devs->lock);
>  return -1;
>  }
>  
> @@ -428,7 +417,6 @@ int virChrdevOpen(virChrdevsPtr devs,
>cbdata,
>virChrdevFDStreamCloseCbFree);
>  
> -virMutexUnlock(&devs->lock);
>  return 0;
>  
>   error:
> @@ -440,7 +428,6 @@ int virChrdevOpen(virChrdevsPtr devs,
>  if (cbdata)
>  VIR_FREE(cbdata->path);
>  VIR_FREE(cbdata);
> -virMutexUnlock(&devs->lock);
>  virChrdevHashEntryFree(ent);
>  return -1;
>  }
> -- 
> 2.25.2
> 
> 

Reviewed-by: Pavel Mores 



Re: [RFC] Adding docker driver to libvirt

2020-04-14 Thread Martin Kletzander

On Tue, Apr 14, 2020 at 09:56:24AM +0300, nshirokovskiy wrote:



On 12.04.2020 12:39, Martin Kletzander wrote:

On Thu, Apr 09, 2020 at 03:30:11PM +0300, nshirokovskiy wrote:

Hi, all.

Does it make sense to add such a driver? I can't say I have a big picture
of docker functionality in mind but at least container lifecycle management
and container networking are common to both.



I think we had something in virt-tools that was able to pull an image from
docker hub and run it with lxc.  Or was it part of sandbox?  I don't know.

Anyway, what would be the benefit of that?



We wanted to add Windows containers to the libvirt API. They are available
under docker API thus the idea to add a docker driver. The docker itself
uses some API to manage Windows containers but this API lacks documentation
thus again the willingness to use just docker API to bring Windows containers
to libvirt.



Oh, so with that we would be able to manage native containers on Windows?  That
might be interesting then.  I did not know about that because last time I heard
about containers on Windows docker was running a Linux VM in which it spawned
the containers =D  I did not know there's something new now.


Nikolay



signature.asc
Description: PGP signature


Re: [PATCH 02/43] bhyve: convert virMutex to GMutex

2020-04-14 Thread Pavel Mores
On Fri, Apr 10, 2020 at 03:54:29PM +0200, Rafael Fonseca wrote:
> Signed-off-by: Rafael Fonseca 
> ---
>  src/bhyve/bhyve_driver.c | 11 ---
>  src/bhyve/bhyve_utils.h  |  2 +-
>  2 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
> index b6204c7fb9..6e9a79cb52 100644
> --- a/src/bhyve/bhyve_driver.c
> +++ b/src/bhyve/bhyve_driver.c
> @@ -73,13 +73,13 @@ bhyveConnPtr bhyve_driver = NULL;
>  void
>  bhyveDriverLock(bhyveConnPtr driver)
>  {
> -virMutexLock(&driver->lock);
> +g_mutex_lock(&driver->lock);
>  }
>  
>  void
>  bhyveDriverUnlock(bhyveConnPtr driver)
>  {
> -virMutexUnlock(&driver->lock);
> +g_mutex_unlock(&driver->lock);
>  }
>  
>  static int
> @@ -1199,7 +1199,7 @@ bhyveStateCleanup(void)
>  if (bhyve_driver->lockFD != -1)
>  virPidFileRelease(BHYVE_STATE_DIR, "driver", bhyve_driver->lockFD);
>  
> -virMutexDestroy(&bhyve_driver->lock);
> +g_mutex_clear(&bhyve_driver->lock);
>  VIR_FREE(bhyve_driver);
>  
>  return 0;
> @@ -1228,10 +1228,7 @@ bhyveStateInitialize(bool privileged,
>  return VIR_DRV_STATE_INIT_ERROR;
>  
>  bhyve_driver->lockFD = -1;
> -if (virMutexInit(&bhyve_driver->lock) < 0) {
> -VIR_FREE(bhyve_driver);
> -return VIR_DRV_STATE_INIT_ERROR;
> -}
> +g_mutex_init(&bhyve_driver->lock);
>  
>  if (!(bhyve_driver->closeCallbacks = virCloseCallbacksNew()))
>  goto cleanup;
> diff --git a/src/bhyve/bhyve_utils.h b/src/bhyve/bhyve_utils.h
> index f3e80b6121..a92ecb48c4 100644
> --- a/src/bhyve/bhyve_utils.h
> +++ b/src/bhyve/bhyve_utils.h
> @@ -44,7 +44,7 @@ struct _virBhyveDriverConfig {
>  };
>  
>  struct _bhyveConn {
> -virMutex lock;
> +GMutex lock;
>  
>  virBhyveDriverConfigPtr config;
>  
> -- 
> 2.25.2
> 
> 

By the way, the approach taken here with bhyveDriver{Lock,Unlock}() might make
sense with the whole series - implement e.g. virMutexInit() in terms of
g_mutex_init() in the first phase and only then replace the actual
virMutexInit() calls if considered beneficial...

Reviewed-by: Pavel Mores 



Re: [PATCH 01/43] admin: convert virMutex to GMutex

2020-04-14 Thread Pavel Mores
On Fri, Apr 10, 2020 at 03:54:28PM +0200, Rafael Fonseca wrote:
> Signed-off-by: Rafael Fonseca 
> ---
>  src/admin/admin_server_dispatch.c | 13 -
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/src/admin/admin_server_dispatch.c 
> b/src/admin/admin_server_dispatch.c
> index b3da577995..2515528779 100644
> --- a/src/admin/admin_server_dispatch.c
> +++ b/src/admin/admin_server_dispatch.c
> @@ -45,7 +45,7 @@ typedef daemonAdmClientPrivate *daemonAdmClientPrivatePtr;
>  /* Separate private data for admin connection */
>  struct daemonAdmClientPrivate {
>  /* Just a placeholder, not that there is anything to be locked */
> -virMutex lock;
> +GMutex lock;
>  
>  virNetDaemonPtr dmn;
>  };
> @@ -55,7 +55,7 @@ remoteAdmClientFree(void *data)
>  {
>  struct daemonAdmClientPrivate *priv = data;
>  
> -virMutexDestroy(&priv->lock);
> +g_mutex_clear(&priv->lock);
>  virObjectUnref(priv->dmn);
>  VIR_FREE(priv);
>  }
> @@ -91,11 +91,7 @@ remoteAdmClientNew(virNetServerClientPtr client 
> G_GNUC_UNUSED,
>  if (VIR_ALLOC(priv) < 0)
>  return NULL;
>  
> -if (virMutexInit(&priv->lock) < 0) {
> -VIR_FREE(priv);
> -virReportSystemError(errno, "%s", _("unable to init mutex"));
> -return NULL;
> -}
> +g_mutex_init(&priv->lock);
>  
>  /*
>   * We don't necessarily need to ref this object right now as there
> @@ -167,9 +163,9 @@ adminDispatchConnectOpen(virNetServerPtr server 
> G_GNUC_UNUSED,
>  struct daemonAdmClientPrivate *priv =
>  virNetServerClientGetPrivateData(client);
>  int ret = -1;
> +g_autoptr(GMutexLocker) locker = g_mutex_locker_new(&priv->lock);
>  
>  VIR_DEBUG("priv=%p dmn=%p", priv, priv->dmn);
> -virMutexLock(&priv->lock);
>  
>  flags = args->flags;
>  virCheckFlagsGoto(0, cleanup);
> @@ -178,7 +174,6 @@ adminDispatchConnectOpen(virNetServerPtr server 
> G_GNUC_UNUSED,
>   cleanup:
>  if (ret < 0)
>  virNetMessageSaveError(rerr);
> -virMutexUnlock(&priv->lock);
>  return ret;
>  }
>  
> -- 
> 2.25.2

I was wondering why virMutexInit() returns a value whereas g_mutex_init() does
not, to make sure there weren't any additional adjustments necessary, but it's
probably OK.

I mean, it does feel slightly dubious as virMutexInit() fails if
pthread_mutex_init() fails which can happen under a bunch of seemingly fairly
realistic scenarios.  I assume g_mutex_init() ultimately calls
pthread_mutex_init() as well so these scenarios should still apply.

However, this seems ultimately a problem of glib API designers to decide how
realistic the scenarios are (at least some of them seem to be related to memory
allocation which glib solves by aborting) and whether to report them to their
users, and they made the decision that they made, hopefully for good reasons.

Reviewed-by: Pavel Mores 



Re: [PATCH 2/2] backup: Allow 'encryption' of backups and scratch images

2020-04-14 Thread Erik Skultety
On Tue, Apr 14, 2020 at 11:22:44AM +0200, Peter Krempa wrote:
> Add the appropriate entries into the schema to allow encryption of the
> backup or scratch image. Since we use blockdev internals for everything
> no changes to the code are actually necessary.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1811906
>
> Signed-off-by: Peter Krempa 
> ---
>  docs/formatbackup.html.in | 14 +++-
>  docs/schemas/domainbackup.rng | 65 +++
>  .../backup-pull-encrypted.xml | 30 +
>  .../backup-push-encrypted.xml | 29 +
>  .../backup-pull-encrypted.xml | 30 +
>  .../backup-push-encrypted.xml | 29 +
>  tests/genericxml2xmltest.c|  3 +
>  7 files changed, 185 insertions(+), 15 deletions(-)
>  create mode 100644 tests/domainbackupxml2xmlin/backup-pull-encrypted.xml
>  create mode 100644 tests/domainbackupxml2xmlin/backup-push-encrypted.xml
>  create mode 100644 tests/domainbackupxml2xmlout/backup-pull-encrypted.xml
>  create mode 100644 tests/domainbackupxml2xmlout/backup-push-encrypted.xml
>
> diff --git a/docs/formatbackup.html.in b/docs/formatbackup.html.in
> index 87744bac98..9e69d8f7d3 100644
> --- a/docs/formatbackup.html.in
> +++ b/docs/formatbackup.html.in
> @@ -101,7 +101,7 @@
>  block.
>  Similar to a disk declaration for a domain, the choice of 
> type
>  controls what additional sub-elements are needed to describe
> -the destination.
> +the destination.

^This hunk needs to go into the previous commit, otherwise the previous patch
breaks the build.

With that fixed (to both patches):
Reviewed-by: Erik Skultety 



Re: [PATCH 0/2] backup: Fix docs and add schema for LUKS encrypted backups

2020-04-14 Thread Ján Tomko

On a Tuesday in 2020, Peter Krempa wrote:

Peter Krempa (2):
 docs: backup: Remove references to push backup to network disk
 backup: Allow 'encryption' of backups and scratch images

docs/formatbackup.html.in | 19 --
docs/schemas/domainbackup.rng | 65 +++
.../backup-pull-encrypted.xml | 30 +
.../backup-push-encrypted.xml | 29 +
.../backup-pull-encrypted.xml | 30 +
.../backup-push-encrypted.xml | 29 +
tests/genericxml2xmltest.c|  3 +
7 files changed, 187 insertions(+), 18 deletions(-)
create mode 100644 tests/domainbackupxml2xmlin/backup-pull-encrypted.xml
create mode 100644 tests/domainbackupxml2xmlin/backup-push-encrypted.xml
create mode 100644 tests/domainbackupxml2xmlout/backup-pull-encrypted.xml
create mode 100644 tests/domainbackupxml2xmlout/backup-push-encrypted.xml



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature


Re: [libvirt PATCH] README: rewrite in rst

2020-04-14 Thread Michal Privoznik

On 4/14/20 11:11 AM, Ján Tomko wrote:

Signed-off-by: Ján Tomko 
---
  README  |  2 +-
  README.md => README.rst | 53 +
  2 files changed, 28 insertions(+), 27 deletions(-)
  rename README.md => README.rst (65%)


Reviewed-by: Michal Privoznik 

Michal



[libvirt PATCH] README: Add GitLab CI badge

2020-04-14 Thread Andrea Bolognani
We've moved most of our CI jobs to GitLab, so we should display
the corresponding badge prominently.

Signed-off-by: Andrea Bolognani 
---
 README.rst | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/README.rst b/README.rst
index 080352ac77..cdcc97b5b2 100644
--- a/README.rst
+++ b/README.rst
@@ -1,3 +1,6 @@
+.. image:: https://gitlab.com/libvirt/libvirt/badges/master/pipeline.svg
+ :target: https://gitlab.com/libvirt/libvirt/pipelines
+ :alt: GitLab CI Build Status
 .. image:: https://travis-ci.org/libvirt/libvirt.svg
  :target: https://travis-ci.org/libvirt/libvirt
  :alt: Travis CI Build Status
-- 
2.25.2



Re: [libvirt PATCH] Convert all remaining Markdown files to reStructuredText

2020-04-14 Thread Andrea Bolognani
On Tue, 2020-04-14 at 15:59 +0200, Ján Tomko wrote:
> On a Tuesday in 2020, Andrea Bolognani wrote:
> > -```
> > -$ mkdir build && cd build
> > -$ ../configure --prefix=/usr --sysconfdir=/etc --localstatedir=/var
> > -$ make
> > -$ sudo make install
> > -```
> > +::
> 
> Side question:
> Are we against putting :: at the end of the previous line? I think it
> looks slightly better that way.

There are no examples of that pattern in our repository at the
moment. For what it's worth, I think it looks significantly worse
than having the :: on a separate line.

-- 
Andrea Bolognani / Red Hat / Virtualization



Re: [libvirt PATCH] Convert all remaining Markdown files to reStructuredText

2020-04-14 Thread Ján Tomko

On a Tuesday in 2020, Andrea Bolognani wrote:

We've adopted reStructuredText as the primary markup language for
our documentation and, given that both GitLab and GitHub can render
documents in this format just fine, it makes sense to get rid of
the few last remaining bits of Markdown and standardize on
reStructuredText across the board.

Signed-off-by: Andrea Bolognani 
---
Note that I've taken a few liberties during the conversion when it
comes to formatting, with the goal of staying close to the style
used for existing reStructuredText documents, but I have not altered
the contents beyond that.

ABOUT-NLS |  2 +-
Makefile.am   |  2 +-
README|  2 +-
README.md => README.rst   | 55 ---
docs/Makefile.am  |  2 +-
docs/fonts/{LICENSE.md => LICENSE.rst}| 30 +
libvirt.spec.in   |  2 +-
po/{README.md => README.rst}  | 32 -
tools/wireshark/{README.md => README.rst} | 28 
9 files changed, 97 insertions(+), 58 deletions(-)
rename README.md => README.rst (65%)
rename docs/fonts/{LICENSE.md => LICENSE.rst} (94%)
rename po/{README.md => README.rst} (86%)
rename tools/wireshark/{README.md => README.rst} (52%)




diff --git a/README.md b/README.rst
similarity index 65%
rename from README.md
rename to README.rst
index 44b0dd87c5..663fba4510 100644
--- a/README.md
+++ b/README.rst


[...]


@@ -1,6 +1,9 @@
-[![Build 
Status](https://travis-ci.org/libvirt/libvirt.svg)](https://travis-ci.org/libvirt/libvirt)
- [![CII Best 
Practices](https://bestpractices.coreinfrastructure.org/projects/355/badge)](https://bestpractices.coreinfrastructure.org/projects/355)
+.. image:: https://travis-ci.org/libvirt/libvirt.svg
+ :alt: Travis CI Build Status
+.. image:: https://bestpractices.coreinfrastructure.org/projects/355/badge
+ :alt: CII Best Practices

+==
Libvirt API for virtualization
==

@@ -21,66 +24,66 @@ mappings into object systems such as GObject, CIM and SNMP.
Further information about the libvirt project can be found on the
website:

-[https://libvirt.org](https://libvirt.org)
+https://libvirt.org


License

+===

The libvirt C API is distributed under the terms of GNU Lesser General
Public License, version 2.1 (or later). Some parts of the code that are
not part of the C library may have the more restrictive GNU General
-Public License, version 2.0 (or later). See the files `COPYING.LESSER`
-and `COPYING` for full license terms & conditions.
+Public License, version 2.0 (or later). See the files ``COPYING.LESSER``
+and ``COPYING`` for full license terms & conditions.


Installation
-
+

Libvirt uses the GNU Autotools build system, so in general can be built
and installed with the usual commands, however, we mandate to have the
build directory different than the source directory. For example, to build
in a manner that is suitable for installing as root, use:

-```
-$ mkdir build && cd build
-$ ../configure --prefix=/usr --sysconfdir=/etc --localstatedir=/var
-$ make
-$ sudo make install
-```
+::


Side question:
Are we against putting :: at the end of the previous line? I think it
looks slightly better that way.


+
+  $ mkdir build && cd build
+  $ ../configure --prefix=/usr --sysconfdir=/etc --localstatedir=/var
+  $ make
+  $ sudo make install

While to build & install as an unprivileged user

-```
-$ mkdir build && cd build
-$ ../configure --prefix=$HOME/usr
-$ make
-$ make install
-```
+::
+
+  $ mkdir build && cd build
+  $ ../configure --prefix=$HOME/usr
+  $ make
+  $ make install

The libvirt code relies on a large number of 3rd party libraries. These will
-be detected during execution of the `configure` script and a summary printed
+be detected during execution of the ``configure`` script and a summary printed
which lists any missing (optional) dependencies.


Contributing
-
+

The libvirt project welcomes contributions in many ways. For most components
the best way to contribute is to send patches to the primary development
mailing list. Further guidance on this can be found on the website:

-[https://libvirt.org/contribute.html](https://libvirt.org/contribute.html)
+https://libvirt.org/contribute.html


Contact

+===

The libvirt project has two primary mailing lists:

-  * libvirt-us...@redhat.com (**for user discussions**)
-  * libvir-list@redhat.com (**for development only**)
+* libvirt-us...@redhat.com (**for user discussions**)
+* libvir-list@redhat.com (**for development only**)

Further details on contacting the project are available on the website:

-[https://libvirt.org/contact.html](https://libvirt.org/contact.html)
+https://libvirt.org/contact.html
diff --git a/docs/Makefile.am b/docs/Makefile.am
index 404121a2a2..6860efc888 100644
--- a/d

Re: [libvirt PATCH] Convert all remaining Markdown files to reStructuredText

2020-04-14 Thread Ján Tomko

On a Tuesday in 2020, Andrea Bolognani wrote:

On Tue, 2020-04-14 at 13:52 +0200, Andrea Bolognani wrote:

+++ b/README.rst
@@ -1,6 +1,9 @@
-[![Build 
Status](https://travis-ci.org/libvirt/libvirt.svg)](https://travis-ci.org/libvirt/libvirt)
- [![CII Best 
Practices](https://bestpractices.coreinfrastructure.org/projects/355/badge)](https://bestpractices.coreinfrastructure.org/projects/355)
+.. image:: https://travis-ci.org/libvirt/libvirt.svg
+ :alt: Travis CI Build Status
+.. image:: https://bestpractices.coreinfrastructure.org/projects/355/badge
+ :alt: CII Best Practices


The fact that the badges are clickable got lost in translation.
Consider the diff below to be squashed in.

diff --git a/README.rst b/README.rst
index 663fba4510..080352ac77 100644
--- a/README.rst
+++ b/README.rst
@@ -1,6 +1,8 @@
.. image:: https://travis-ci.org/libvirt/libvirt.svg
+ :target: https://travis-ci.org/libvirt/libvirt
 :alt: Travis CI Build Status
.. image:: https://bestpractices.coreinfrastructure.org/projects/355/badge
+ :target: https://bestpractices.coreinfrastructure.org/projects/355
 :alt: CII Best Practices

==


Signed-off-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature


Re: [PATCH 2/2] conf: Don't format http cookies unless VIR_DOMAIN_DEF_FORMAT_SECURE is used

2020-04-14 Thread Erik Skultety
On Tue, Apr 14, 2020 at 10:38:48AM +0200, Peter Krempa wrote:
> Starting from 3b076391befc3fe72deb0c244ac6c2b4c100b410

s/from/with

> (v6.1.0-122-g3b076391be) we support http cookies. Since they may contain
> somewhat sensitive information we should not format into the XML unless

s/format/format them/

--
Erik Skultety



Re: [PATCH 0/2] Fix formatting of http cookies into XML

2020-04-14 Thread Erik Skultety
On Tue, Apr 14, 2020 at 10:38:46AM +0200, Peter Krempa wrote:
> See 2/2.
>
> Peter Krempa (2):
>   virstoragetest: testBackingParse: Use VIR_DOMAIN_DEF_FORMAT_SECURE
> when formatting xml
>   conf: Don't format http cookies unless VIR_DOMAIN_DEF_FORMAT_SECURE is
> used
>
>  src/conf/domain_conf.c | 8 ++--
>  tests/virstoragetest.c | 3 ++-
>  2 files changed, 8 insertions(+), 3 deletions(-)
>
> --
> 2.26.0
>

Reviewed-by: Erik Skultety 



Re: [libvirt PATCH] Convert all remaining Markdown files to reStructuredText

2020-04-14 Thread Andrea Bolognani
On Tue, 2020-04-14 at 13:52 +0200, Andrea Bolognani wrote:
> +++ b/README.rst
> @@ -1,6 +1,9 @@
> -[![Build 
> Status](https://travis-ci.org/libvirt/libvirt.svg)](https://travis-ci.org/libvirt/libvirt)
> - [![CII Best 
> Practices](https://bestpractices.coreinfrastructure.org/projects/355/badge)](https://bestpractices.coreinfrastructure.org/projects/355)
> +.. image:: https://travis-ci.org/libvirt/libvirt.svg
> + :alt: Travis CI Build Status
> +.. image:: https://bestpractices.coreinfrastructure.org/projects/355/badge
> + :alt: CII Best Practices

The fact that the badges are clickable got lost in translation.
Consider the diff below to be squashed in.

diff --git a/README.rst b/README.rst
index 663fba4510..080352ac77 100644
--- a/README.rst
+++ b/README.rst
@@ -1,6 +1,8 @@
 .. image:: https://travis-ci.org/libvirt/libvirt.svg
+ :target: https://travis-ci.org/libvirt/libvirt
  :alt: Travis CI Build Status
 .. image:: https://bestpractices.coreinfrastructure.org/projects/355/badge
+ :target: https://bestpractices.coreinfrastructure.org/projects/355
  :alt: CII Best Practices

 ==
-- 
Andrea Bolognani / Red Hat / Virtualization



Re: libvirt python console

2020-04-14 Thread Michal Privoznik

On 4/12/20 7:18 PM, PunkGeek wrote:

Hello,
I see this example about VM console serial, Which I could connect to the 
guest VM successfully.

however, I could not understand how to send commands to the guest VM?
https://github.com/libvirt/libvirt-python/blob/master/examples/consolecallback.py
Best regards


The example is basically what 'virsh console' does. This simply opens 
serial console to virtual machine. But then you need to enable agetty 
for the serial console. In systemd distributions it's sufficient to 
append 'console=ttyS0' to kernel cmd line and systemd should do the rest 
automagically.


Michal



Re: Wiki page audit - possible page deleteion

2020-04-14 Thread Michal Privoznik

On 4/9/20 12:10 PM, Daniel P. Berrangé wrote:

On Wed, Apr 08, 2020 at 04:45:46PM +0100, Daniel P. Berrangé wrote:


Various GSoC pages, the per-year pages, and for 2016 only
some reports from the contributors. Most of the latter is
outdated info that's no longer really relevant, so I'd
probablyjust delete the last 4 pages, and move the rest
to the main site.

 Google Summer of Code 2016
 Google Summer of Code 2017
 Google Summer of Code 2018
 Google Summer of Code 2019
 Google Summer of Code FAQ
 Google Summer of Code Ideas
 Google Summer of Code 2016/Abstracting device address allocation
 Google Summer of Code 2016/Asynchronous lifecycle events for storage 
objects
 Google Summer of Code 2016/Making virsh more bash like
 Google Summer of Code 2016/lxc migration


What do people think of using the GitLab issue tracker for main Google
Summer of Code ideas page ?

We could have a "gsoc" label that we apply to issues to make it easy
to view them all. The initial issue description would describe the high
level of the project. The issue tracker can be used for further discussions
as the project is planned and then gets underway, as well as letting us
track progress.

We could create further tags  "gsoc::2018" when issues are picked for
a particular year.


Yes, makes sense. I'm okay with this approach.

Michal



[libvirt PATCH] CONTRIBUTING: Include in release archives

2020-04-14 Thread Andrea Bolognani
The file, added with commit

  commit 874952f80c6d68c1a7a75e71c11a576f96f75dc2
  Author: Andrea Bolognani 
  Date:   Mon Apr 6 11:56:58 2020 +0200

CONTRIBUTING: Add entry point for new contributors

should be included in release archives.

Signed-off-by: Andrea Bolognani 
---
 Makefile.am | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile.am b/Makefile.am
index 5590c88e4d..01a9a5aa83 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -45,6 +45,7 @@ EXTRA_DIST = \
   run.in \
   README.md \
   AUTHORS.in \
+  CONTRIBUTING.rst \
   scripts/apibuild.py \
   scripts/augeas-gentest.py \
   build-aux/check-spacing.pl \
-- 
2.25.2



[libvirt PATCH] Convert all remaining Markdown files to reStructuredText

2020-04-14 Thread Andrea Bolognani
We've adopted reStructuredText as the primary markup language for
our documentation and, given that both GitLab and GitHub can render
documents in this format just fine, it makes sense to get rid of
the few last remaining bits of Markdown and standardize on
reStructuredText across the board.

Signed-off-by: Andrea Bolognani 
---
Note that I've taken a few liberties during the conversion when it
comes to formatting, with the goal of staying close to the style
used for existing reStructuredText documents, but I have not altered
the contents beyond that.

 ABOUT-NLS |  2 +-
 Makefile.am   |  2 +-
 README|  2 +-
 README.md => README.rst   | 55 ---
 docs/Makefile.am  |  2 +-
 docs/fonts/{LICENSE.md => LICENSE.rst}| 30 +
 libvirt.spec.in   |  2 +-
 po/{README.md => README.rst}  | 32 -
 tools/wireshark/{README.md => README.rst} | 28 
 9 files changed, 97 insertions(+), 58 deletions(-)
 rename README.md => README.rst (65%)
 rename docs/fonts/{LICENSE.md => LICENSE.rst} (94%)
 rename po/{README.md => README.rst} (86%)
 rename tools/wireshark/{README.md => README.rst} (52%)

diff --git a/ABOUT-NLS b/ABOUT-NLS
index b583e276a7..91a3266ed6 12
--- a/ABOUT-NLS
+++ b/ABOUT-NLS
@@ -1 +1 @@
-po/README.md
\ No newline at end of file
+po/README.rst
\ No newline at end of file
diff --git a/Makefile.am b/Makefile.am
index 5590c88e4d..06475344c1 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -43,7 +43,7 @@ EXTRA_DIST = \
   autogen.sh \
   GNUmakefile \
   run.in \
-  README.md \
+  README.rst \
   AUTHORS.in \
   scripts/apibuild.py \
   scripts/augeas-gentest.py \
diff --git a/README b/README
index 42061c01a1..92cacd2853 12
--- a/README
+++ b/README
@@ -1 +1 @@
-README.md
\ No newline at end of file
+README.rst
\ No newline at end of file
diff --git a/README.md b/README.rst
similarity index 65%
rename from README.md
rename to README.rst
index 44b0dd87c5..663fba4510 100644
--- a/README.md
+++ b/README.rst
@@ -1,6 +1,9 @@
-[![Build 
Status](https://travis-ci.org/libvirt/libvirt.svg)](https://travis-ci.org/libvirt/libvirt)
- [![CII Best 
Practices](https://bestpractices.coreinfrastructure.org/projects/355/badge)](https://bestpractices.coreinfrastructure.org/projects/355)
+.. image:: https://travis-ci.org/libvirt/libvirt.svg
+ :alt: Travis CI Build Status
+.. image:: https://bestpractices.coreinfrastructure.org/projects/355/badge
+ :alt: CII Best Practices
 
+==
 Libvirt API for virtualization
 ==
 
@@ -21,66 +24,66 @@ mappings into object systems such as GObject, CIM and SNMP.
 Further information about the libvirt project can be found on the
 website:
 
-[https://libvirt.org](https://libvirt.org)
+https://libvirt.org
 
 
 License

+===
 
 The libvirt C API is distributed under the terms of GNU Lesser General
 Public License, version 2.1 (or later). Some parts of the code that are
 not part of the C library may have the more restrictive GNU General
-Public License, version 2.0 (or later). See the files `COPYING.LESSER`
-and `COPYING` for full license terms & conditions.
+Public License, version 2.0 (or later). See the files ``COPYING.LESSER``
+and ``COPYING`` for full license terms & conditions.
 
 
 Installation
-
+
 
 Libvirt uses the GNU Autotools build system, so in general can be built
 and installed with the usual commands, however, we mandate to have the
 build directory different than the source directory. For example, to build
 in a manner that is suitable for installing as root, use:
 
-```
-$ mkdir build && cd build
-$ ../configure --prefix=/usr --sysconfdir=/etc --localstatedir=/var
-$ make
-$ sudo make install
-```
+::
+
+  $ mkdir build && cd build
+  $ ../configure --prefix=/usr --sysconfdir=/etc --localstatedir=/var
+  $ make
+  $ sudo make install
 
 While to build & install as an unprivileged user
 
-```
-$ mkdir build && cd build
-$ ../configure --prefix=$HOME/usr
-$ make
-$ make install
-```
+::
+
+  $ mkdir build && cd build
+  $ ../configure --prefix=$HOME/usr
+  $ make
+  $ make install
 
 The libvirt code relies on a large number of 3rd party libraries. These will
-be detected during execution of the `configure` script and a summary printed
+be detected during execution of the ``configure`` script and a summary printed
 which lists any missing (optional) dependencies.
 
 
 Contributing
-
+
 
 The libvirt project welcomes contributions in many ways. For most components
 the best way to contribute is to send patches to the primary development
 mailing list. Further guidance on this can be found on the website:
 
-[https://libvirt.org/contribute.html](https://libvirt.org/contribute.html)
+https://libvirt.org/contribute.html
 
 
 Contact

+=

Re: [PATCH] libvirt-stream.c: Use g_autofree

2020-04-14 Thread Erik Skultety
On Mon, Apr 13, 2020 at 06:19:26PM +0530, Seeteena Thoufeek wrote:
> Signed-off-by: Seeteena Thoufeek 
> ---
Reviewed-by: Erik Skultety 



Re: [libvirt PATCH 1/2] CONTRIBUTING: Add entry point for new contributors

2020-04-14 Thread Andrea Bolognani
On Tue, 2020-04-14 at 10:14 +0200, Ján Tomko wrote:
> On a Monday in 2020, Andrea Bolognani wrote:
> > diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
> > new file mode 100644
> > index 00..a8551965bd
> > --- /dev/null
> > +++ b/CONTRIBUTING.md
> 
> s/md/rst/

Good point.

I've verified that both GitLab and GitHub can render reStructuredText
correctly; I'll look into converting the few remaining Markdown files
in the repository to reStructuredText as well.

-- 
Andrea Bolognani / Red Hat / Virtualization



Re: [libvirt-jenkins-ci PATCH v3 4/5] guests: Introduce the new 'gitlab' flavor

2020-04-14 Thread Erik Skultety
On Tue, Apr 14, 2020 at 12:31:26PM +0200, Andrea Bolognani wrote:
> On Tue, 2020-04-14 at 10:17 +0200, Erik Skultety wrote:
> > On Thu, Apr 09, 2020 at 12:28:50PM +0200, Andrea Bolognani wrote:
> > > On Thu, 2020-04-09 at 06:23 +0200, Erik Skultety wrote:
> > > > +++ b/guests/playbooks/update/tasks/gitlab.yml
> > > > +- name: Make {{ gitlab_runner_config_dir }} world readable
> > > > +  file:
> > > > +path: '{{ gitlab_runner_config_dir }}'
> > > > +mode: '0755'
> > > > +
> > > > +- name: Make {{ gitlab_runner_config_dir }}/config.toml world readable
> > > > +  file:
> > > > +path: '{{ gitlab_runner_config_dir }}/config.toml'
> > > > +mode: '0644'
> > >
> > > The message for these tasks is unnecessarily detailed: I'd just use
> > > something like
> > >
> > >   Make gitlab-runner configuration readable
> >
> > Okay, however...
> >
> > > for both.
> > >
> > > Additionally, even though the gitlab user is going to be the only one
> > > on the system so it doesn't make much of a difference in practice, I
> > > think we should have config.toml
> > >
> >
> > ...here you suggest the following adjustment. I feel like the messages above
> > will then become confusing and misleading, since who are we making it 
> > readable
> > for? Well, only for the gitlab user, so I think a little more detail in 
> > them is
> > justifiable.
> >
> > >   owner: root
> > >   group: gitlab
> > >   mode: '0640'
> >
> > So how about:
> > "Make gitlab-runner config dir readable" for the former and
> > "Make gitlab-runner config.toml owned by the gitlab group" for the latter
>
> I still think that's an unnecessary amount of detail, and we have
> plenty of existing examples in the repository such as
>
>   - name: Update installed packages
> package:
>   name: fedora-gpg-keys
>   state: latest
>   disable_gpg_check: yes
> when:
>   - os_name == 'Fedora'
>   - os_version == 'Rawhide'
>
>   - name: Update installed packages
> command: '{{ package_manager }} update --refresh --exclude "kernel*" -y'
> args:
>   warn: no
> when:
>   - os_name == 'Fedora'
>   - os_version == 'Rawhide'
>
>   - name: Update installed packages
> command: '{{ package_manager }} update --disablerepo="*" 
> --enablerepo=fedora-rawhide-kernel-nodebug "kernel*" -y'
> args:
>   warn: no
> when:
>   - os_name == 'Fedora'
>   - os_version == 'Rawhide'

Fair enough, ^this is a compelling counter-argument, I proceeded with the
original suggestion and merged the series, thanks for the review.

--
Erik Skultety



Re: [libvirt PATCH 2/3] travis: Reduce test matrix

2020-04-14 Thread Andrea Bolognani
On Tue, 2020-04-14 at 10:54 +0100, Daniel P. Berrangé wrote:
> On Fri, Apr 10, 2020 at 11:10:33AM +0200, Andrea Bolognani wrote:
> > The xcode10.3 build has recently started failing because of issues
> > that are not in libvirt, so get rid of it. It didn't buy us
> > additionaly platform coverage anyway, since underneath it's using
> > the same macOS 10.14 version as xcode11.3.
> 
> The point was to test both xcode versions, since they're both commonly
> used.  I was testing this over the weekend and determined that 10.1
> will still work correctly. Travis broke something in their 10.2/10.3
> images that inserts a bogus -I path.

Considering that almost nobody in the libvirt community is actually
focused on macOS support, trying to chase after two SDKs instead of
a single one doesn't necessarily sound like a good idea to me,
especially when the older one doesn't target the current version of
macOS.

It's already pretty bad that we're stuck on 10.14, as opposed to
10.15 which is the version we actually target according to our
platform support policy, because Travis CI always takes forever to
prepare new images.

I was looking into Cirrus CI the other day and they support macOS
too, and they even already have a 10.15 image. I didn't dig deeper,
but maybe if we're stuck using an external service for macOS builds
it should be Cirrus CI instead of Travis CI.

-- 
Andrea Bolognani / Red Hat / Virtualization



Re: [PATCH ocaml 1/1] Add dune build system

2020-04-14 Thread Richard W.M. Jones
On Tue, Apr 14, 2020 at 12:22:49PM +0200, Pino Toscano wrote:
> Introduce the dune build system to build ocaml-libvirt, providing
> everything that the current autoconf-based build system has.
> 
> Add also an opam configuration files, as it is mandatory with dune.
> 
> Signed-off-by: Pino Toscano 

ACK

You might want to drop -safe-string since it's been the default
for a while (since OCaml 4.06 / Nov 2017).

Rich.

>  .gitignore |  3 +++
>  dune   | 18 +
>  dune-project   |  1 +
>  dune.inc   | 22 
>  examples/dune  | 50 
>  libvirt/discover.ml| 40 +
>  libvirt/dune   | 57 ++
>  libvirt/dune-project   |  2 ++
>  libvirt/mllibvirt.opam | 33 
>  9 files changed, 226 insertions(+)
>  create mode 100644 dune
>  create mode 100644 dune-project
>  create mode 100644 dune.inc
>  create mode 100644 examples/dune
>  create mode 100644 libvirt/discover.ml
>  create mode 100644 libvirt/dune
>  create mode 100644 libvirt/dune-project
>  create mode 100644 libvirt/mllibvirt.opam
> 
> diff --git a/.gitignore b/.gitignore
> index c52c1b8..814457b 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -10,10 +10,12 @@
>  *.opt
>  *.so
>  *~
> +.merlin
>  Make.rules
>  Makefile
>  core
>  core.*
> +/_build/
>  /META
>  /aclocal.m4
>  /autom4te.cache
> @@ -30,5 +32,6 @@ core.*
>  /html/
>  /libvirt/libvirt_generated.c
>  /libvirt/libvirt_version.ml
> +/libvirt/mllibvirt.install
>  /ocaml-libvirt-*.exe
>  /ocaml-libvirt-*.tar.gz
> diff --git a/dune b/dune
> new file mode 100644
> index 000..5002345
> --- /dev/null
> +++ b/dune
> @@ -0,0 +1,18 @@
> +; OCaml bindings for libvirt.
> +; (C) Copyright 2020 Pino Toscano, Red Hat Inc.
> +;
> +; This library is free software; you can redistribute it and/or
> +; modify it under the terms of the GNU Lesser General Public
> +; License as published by the Free Software Foundation; either
> +; version 2 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, write to the Free Software
> +; Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301  
> USA
> +
> +(include dune.inc)
> diff --git a/dune-project b/dune-project
> new file mode 100644
> index 000..de4fc20
> --- /dev/null
> +++ b/dune-project
> @@ -0,0 +1 @@
> +(lang dune 1.0)
> diff --git a/dune.inc b/dune.inc
> new file mode 100644
> index 000..2cfb767
> --- /dev/null
> +++ b/dune.inc
> @@ -0,0 +1,22 @@
> +; OCaml bindings for libvirt.
> +; (C) Copyright 2020 Pino Toscano, Red Hat Inc.
> +;
> +; This library is free software; you can redistribute it and/or
> +; modify it under the terms of the GNU Lesser General Public
> +; License as published by the Free Software Foundation; either
> +; version 2 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, write to the Free Software
> +; Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301  
> USA
> +
> +(env
> +  (dev
> +(flags (:standard -g -warn-error CDEFLMPSUVYZX-3 -w -9-27-34-37-50 
> -safe-string)))
> +  (release
> +(flags (:standard -g -warn-error CDEFLMPSUVYZX-3 -w -9-27-34-37-50 
> -safe-string
> diff --git a/examples/dune b/examples/dune
> new file mode 100644
> index 000..219727c
> --- /dev/null
> +++ b/examples/dune
> @@ -0,0 +1,50 @@
> +; OCaml bindings for libvirt.
> +; (C) Copyright 2020 Pino Toscano, Red Hat Inc.
> +;
> +; This library is free software; you can redistribute it and/or
> +; modify it under the terms of the GNU Lesser General Public
> +; License as published by the Free Software Foundation; either
> +; version 2 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, write to the Free Software
> +; Foundation, Inc., 51 Franklin Street,

Re: [libvirt-jenkins-ci PATCH v3 4/5] guests: Introduce the new 'gitlab' flavor

2020-04-14 Thread Andrea Bolognani
On Tue, 2020-04-14 at 10:17 +0200, Erik Skultety wrote:
> On Thu, Apr 09, 2020 at 12:28:50PM +0200, Andrea Bolognani wrote:
> > On Thu, 2020-04-09 at 06:23 +0200, Erik Skultety wrote:
> > > +++ b/guests/playbooks/update/tasks/gitlab.yml
> > > +- name: Make {{ gitlab_runner_config_dir }} world readable
> > > +  file:
> > > +path: '{{ gitlab_runner_config_dir }}'
> > > +mode: '0755'
> > > +
> > > +- name: Make {{ gitlab_runner_config_dir }}/config.toml world readable
> > > +  file:
> > > +path: '{{ gitlab_runner_config_dir }}/config.toml'
> > > +mode: '0644'
> > 
> > The message for these tasks is unnecessarily detailed: I'd just use
> > something like
> > 
> >   Make gitlab-runner configuration readable
> 
> Okay, however...
> 
> > for both.
> > 
> > Additionally, even though the gitlab user is going to be the only one
> > on the system so it doesn't make much of a difference in practice, I
> > think we should have config.toml
> > 
> 
> ...here you suggest the following adjustment. I feel like the messages above
> will then become confusing and misleading, since who are we making it readable
> for? Well, only for the gitlab user, so I think a little more detail in them 
> is
> justifiable.
> 
> >   owner: root
> >   group: gitlab
> >   mode: '0640'
> 
> So how about:
> "Make gitlab-runner config dir readable" for the former and
> "Make gitlab-runner config.toml owned by the gitlab group" for the latter

I still think that's an unnecessary amount of detail, and we have
plenty of existing examples in the repository such as

  - name: Update installed packages
package:
  name: fedora-gpg-keys
  state: latest
  disable_gpg_check: yes
when:
  - os_name == 'Fedora'
  - os_version == 'Rawhide'

  - name: Update installed packages
command: '{{ package_manager }} update --refresh --exclude "kernel*" -y'
args:
  warn: no
when:
  - os_name == 'Fedora'
  - os_version == 'Rawhide'

  - name: Update installed packages
command: '{{ package_manager }} update --disablerepo="*" 
--enablerepo=fedora-rawhide-kernel-nodebug "kernel*" -y'
args:
  warn: no
when:
  - os_name == 'Fedora'
  - os_version == 'Rawhide'

where we provide the high-level information as feedback to the user,
without going too much into detail - in this case, that we're
updating the system in three steps instead of a single one because
some packages require special handling.

But I don't want to hold up the series because of bikeshedding, so
if you are very keen on having the extra detail I'll take it as-is :)

-- 
Andrea Bolognani / Red Hat / Virtualization



[PATCH ocaml 0/1] RFC: add/switch to dune build system

2020-04-14 Thread Pino Toscano
ocaml-libvirt is currently built using manually written makefiles, and
an autoconf script to detect libvirt (using pkg-config). While this
seems OK, it has a lot of downsides:
- all the OCaml rules are manually written
- there is some form of duplication in the OCaml rules
- the bytecode vs native build is fragile

Initially named jhbuild, dune is a native OCaml build system which plans
to eventually replace the others (omake, oasis, ocamlbuild), with a
growing user base in the OCaml community.

This is a very simple patch adding support for dune. The idea is to
replace the current autoconf+makefile build system, which can be
removed more or less easily. I'd prefer to not maintain two build
systems at once, so this patch is also a way to start a discussion about
the proposed approach.

Things not yet done (pending the discussion):
- rewrite/simplify the release bits, currently interwonen in the
  makefiles
- update the documentation
- update libvirt-ci
- remove the autoconf+makefile build system

Pino Toscano (1):
  Add dune build system

 .gitignore |  3 +++
 dune   | 18 +
 dune-project   |  1 +
 dune.inc   | 22 
 examples/dune  | 50 
 libvirt/discover.ml| 40 +
 libvirt/dune   | 57 ++
 libvirt/dune-project   |  2 ++
 libvirt/mllibvirt.opam | 33 
 9 files changed, 226 insertions(+)
 create mode 100644 dune
 create mode 100644 dune-project
 create mode 100644 dune.inc
 create mode 100644 examples/dune
 create mode 100644 libvirt/discover.ml
 create mode 100644 libvirt/dune
 create mode 100644 libvirt/dune-project
 create mode 100644 libvirt/mllibvirt.opam

-- 
2.25.2



[PATCH ocaml 1/1] Add dune build system

2020-04-14 Thread Pino Toscano
Introduce the dune build system to build ocaml-libvirt, providing
everything that the current autoconf-based build system has.

Add also an opam configuration files, as it is mandatory with dune.

Signed-off-by: Pino Toscano 
---
 .gitignore |  3 +++
 dune   | 18 +
 dune-project   |  1 +
 dune.inc   | 22 
 examples/dune  | 50 
 libvirt/discover.ml| 40 +
 libvirt/dune   | 57 ++
 libvirt/dune-project   |  2 ++
 libvirt/mllibvirt.opam | 33 
 9 files changed, 226 insertions(+)
 create mode 100644 dune
 create mode 100644 dune-project
 create mode 100644 dune.inc
 create mode 100644 examples/dune
 create mode 100644 libvirt/discover.ml
 create mode 100644 libvirt/dune
 create mode 100644 libvirt/dune-project
 create mode 100644 libvirt/mllibvirt.opam

diff --git a/.gitignore b/.gitignore
index c52c1b8..814457b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -10,10 +10,12 @@
 *.opt
 *.so
 *~
+.merlin
 Make.rules
 Makefile
 core
 core.*
+/_build/
 /META
 /aclocal.m4
 /autom4te.cache
@@ -30,5 +32,6 @@ core.*
 /html/
 /libvirt/libvirt_generated.c
 /libvirt/libvirt_version.ml
+/libvirt/mllibvirt.install
 /ocaml-libvirt-*.exe
 /ocaml-libvirt-*.tar.gz
diff --git a/dune b/dune
new file mode 100644
index 000..5002345
--- /dev/null
+++ b/dune
@@ -0,0 +1,18 @@
+; OCaml bindings for libvirt.
+; (C) Copyright 2020 Pino Toscano, Red Hat Inc.
+;
+; This library is free software; you can redistribute it and/or
+; modify it under the terms of the GNU Lesser General Public
+; License as published by the Free Software Foundation; either
+; version 2 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, write to the Free Software
+; Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301  USA
+
+(include dune.inc)
diff --git a/dune-project b/dune-project
new file mode 100644
index 000..de4fc20
--- /dev/null
+++ b/dune-project
@@ -0,0 +1 @@
+(lang dune 1.0)
diff --git a/dune.inc b/dune.inc
new file mode 100644
index 000..2cfb767
--- /dev/null
+++ b/dune.inc
@@ -0,0 +1,22 @@
+; OCaml bindings for libvirt.
+; (C) Copyright 2020 Pino Toscano, Red Hat Inc.
+;
+; This library is free software; you can redistribute it and/or
+; modify it under the terms of the GNU Lesser General Public
+; License as published by the Free Software Foundation; either
+; version 2 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, write to the Free Software
+; Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301  USA
+
+(env
+  (dev
+(flags (:standard -g -warn-error CDEFLMPSUVYZX-3 -w -9-27-34-37-50 
-safe-string)))
+  (release
+(flags (:standard -g -warn-error CDEFLMPSUVYZX-3 -w -9-27-34-37-50 
-safe-string
diff --git a/examples/dune b/examples/dune
new file mode 100644
index 000..219727c
--- /dev/null
+++ b/examples/dune
@@ -0,0 +1,50 @@
+; OCaml bindings for libvirt.
+; (C) Copyright 2020 Pino Toscano, Red Hat Inc.
+;
+; This library is free software; you can redistribute it and/or
+; modify it under the terms of the GNU Lesser General Public
+; License as published by the Free Software Foundation; either
+; version 2 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, write to the Free Software
+; Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301  USA
+
+(executable
+  (name domain_events)
+  (modules Domain_events)
+  (libraries mllibvirt))
+
+(executable
+  (name get_all_domain_stats)
+  (modules Get_all_domain_stats)
+  (libraries mllibvirt unix))
+
+(executable
+  (name get_cpu_stats)
+  (modules Get_cpu_stats)
+  (libraries mllibvirt))
+
+(executable
+  (name list_domains)
+  (modules List_domains)
+  (libraries mllibvirt))
+
+(executable
+  (name

Re: [libvirt PATCH 2/3] travis: Reduce test matrix

2020-04-14 Thread Daniel P . Berrangé
On Tue, Apr 14, 2020 at 10:54:16AM +0100, Daniel P. Berrangé wrote:
> On Fri, Apr 10, 2020 at 11:10:33AM +0200, Andrea Bolognani wrote:
> > The xcode10.3 build has recently started failing because of issues
> > that are not in libvirt, so get rid of it. It didn't buy us
> > additionaly platform coverage anyway, since underneath it's using
> > the same macOS 10.14 version as xcode11.3.
> 
> The point was to test both xcode versions, since they're both commonly
> used.  I was testing this over the weekend and determined that 10.1
> will still work correctly. Travis broke something in their 10.2/10.3
> images that inserts a bogus -I path.

It appears they realized their mistake, as the latest build this morning
on 10.3 has succeeeded again. So we don't need to drop / change anything.

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: what a correct use for virConnectDomainEventRegisterAny API, how to Obtain a stable expected result

2020-04-14 Thread Daniel P . Berrangé
On Mon, Apr 13, 2020 at 05:32:38PM +0800, thomas.kuang wrote:
> HI, everyone:
> 
> 
> My target  deal with network hotplug use virDomainDetachDeviceFlags. Because 
> when the API return ,the network maybe doesn’t remove from my vm guest os.
> 
> So I use virConnectDomainEventRegisterAny  to register an event ID: 
> VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED ,
> 
>  my process as follow:
> 
> cb_para->call_id=virConnectDomainEventRegisterAny(cb_para->conn,cb_para->dom,VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED,
>  VIR_DOMAIN_EVENT_CALLBACK(vnf_control_del_network_cb), cb_para, 
> vnf_control_del_network_cb_free); 
> 
> 
> 
> flags |= VIR_DOMAIN_AFFECT_CONFIG;
> if (virDomainIsActive(dom) == 1) {
> flags |= VIR_DOMAIN_AFFECT_LIVE;
> }
> 
> ret = virDomainDetachDeviceFlags(dom, xml, flags);
> 
> 
>  
> 
> above code write in thread loop ,then in the same loop :
> 
>  
> 
>while (1) {
> 
>   mission = vnf_mission_queue_get(task);
> 
>   if (mission == NULL) {
> 
>  sleep(1);
> 
>  continue;
> 
>   } 
> 
>   vnf_op_process(&mission->info); // this will deal with network 
> hotplug,will call virConnectDomainEventRegisterAny  then call  
> virDomainDetachDeviceFlags
> 
>   if (mission) {
> 
>  vnf_mission_free(mission);
> 
>   } 
> 
>   if(virEventRunDefaultImpl() < 0) { // at here process  the 
> registered callback for event-registered
> 
> printf();
> 
> 
>   } 
> 
>}
> 
> My problem is: some time  , the virEventRunDefaultImpl can trigger the
> vnf_control_del_network_cb  callback ,but some time  there is nothing,
> as if the VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED has lost.
> what cause the Unpredictable behavior ?  what is a correct use for
> virConnectDomainEventRegisterAny ?

The virDomainDetachDeviceFlags API is asynchronous and requires a cooperative
guest to complete. It merely injects an ACPI PCI unplug request to the guest
OS.  If the guest OS doesn't honour this request (because it is in early
bootup, or is in BIOS, or is crashed, or is configured to disable hotplug,
or is maliciously not responding), then the device will never be unplugged,
and so you'll never get an event.

So the first thing for you todo is to validate that the device really is
being unplugged in the guest OS, and in QEMU.

To check QEMU run:

  virsh qemu-monitor-command --hmp $GUEST "info pci"

and look to see if the PCI device is still listed in the output.

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 python console

2020-04-14 Thread PunkGeek
Hello,
I see this example about VM console serial, Which I could connect to the
guest VM successfully.
however, I could not understand how to send commands to the guest VM?
https://github.com/libvirt/libvirt-python/blob/master/examples/consolecallback.py
Best regards


Re: [libvirt PATCH 3/3] travis: Remove usage of 'sudo'

2020-04-14 Thread Daniel P . Berrangé
On Fri, Apr 10, 2020 at 11:10:34AM +0200, Andrea Bolognani wrote:
> Travis CI reports
> 
>   root: deprecated key sudo (The key `sudo` has no effect anymore.)
> 
> so let's drop it.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  .travis.yml | 1 -
>  1 file changed, 1 deletion(-)

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 PATCH 1/3] travis: Deduplicate build instructions

2020-04-14 Thread Daniel P . Berrangé
On Fri, Apr 10, 2020 at 11:10:32AM +0200, Andrea Bolognani wrote:
> All information, except for osx_image image, is identical between
> the two jobs so we can avoid repeating it.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  .travis.yml | 51 ---
>  1 file changed, 20 insertions(+), 31 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: [libvirt PATCH 2/3] travis: Reduce test matrix

2020-04-14 Thread Daniel P . Berrangé
On Fri, Apr 10, 2020 at 11:10:33AM +0200, Andrea Bolognani wrote:
> The xcode10.3 build has recently started failing because of issues
> that are not in libvirt, so get rid of it. It didn't buy us
> additionaly platform coverage anyway, since underneath it's using
> the same macOS 10.14 version as xcode11.3.

The point was to test both xcode versions, since they're both commonly
used.  I was testing this over the weekend and determined that 10.1
will still work correctly. Travis broke something in their 10.2/10.3
images that inserts a bogus -I path.


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] driver.c: Use g_autofree

2020-04-14 Thread Erik Skultety
On Mon, Apr 13, 2020 at 06:18:57PM +0530, Seeteena Thoufeek wrote:
> This is the only instance of g_autofree change applicable for
> driver.c

I'd say something like: "This is the last missing g_autofree change to convert
the module after commit 1e2ae2e311c took care of the VIR_AUTOFREE conversion".

Reviewed-by: Erik Skultety 

I'll merge this with the adjusted commit message.



[PATCH] qemu: Revoke access to mirror on failed blockcopy

2020-04-14 Thread Michal Privoznik
When preparing to do a blockcopy, the mirror image is modified so
that QEMU can access it. For instance, the mirror has seclabels
set, if it is a NVMe disk it is detached from the host and so on.
And usually, the restore is done upon successful finish of the
blockcopy operation. But, if something fails then we need to
explicitly revoke the access to the mirror image (and thus
reattach NVMe disk back to the host).

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

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_driver.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 31f199fdef..9475235f01 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17950,6 +17950,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
 virDomainDiskDefPtr disk = NULL;
 int ret = -1;
 bool need_unlink = false;
+bool need_revoke = false;
 g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
 const char *format = NULL;
 bool mirror_reuse = !!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT);
@@ -18124,6 +18125,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
 
 if (qemuDomainStorageSourceChainAccessAllow(driver, vm, mirror) < 0)
 goto endjob;
+need_revoke = true;
 
 if (blockdev) {
 if (mirror_reuse) {
@@ -18240,6 +18242,8 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
 if (crdata)
 qemuBlockStorageSourceAttachRollback(priv->mon, 
crdata->srcdata[0]);
 ignore_value(qemuDomainObjExitMonitor(driver, vm));
+if (need_revoke)
+qemuDomainStorageSourceChainAccessRevoke(driver, vm, mirror);
 }
 if (need_unlink && virStorageFileUnlink(mirror) < 0)
 VIR_WARN("%s", _("unable to remove just-created copy target"));
-- 
2.24.1



[PATCH 0/2] backup: Fix docs and add schema for LUKS encrypted backups

2020-04-14 Thread Peter Krempa
Peter Krempa (2):
  docs: backup: Remove references to push backup to network disk
  backup: Allow 'encryption' of backups and scratch images

 docs/formatbackup.html.in | 19 --
 docs/schemas/domainbackup.rng | 65 +++
 .../backup-pull-encrypted.xml | 30 +
 .../backup-push-encrypted.xml | 29 +
 .../backup-pull-encrypted.xml | 30 +
 .../backup-push-encrypted.xml | 29 +
 tests/genericxml2xmltest.c|  3 +
 7 files changed, 187 insertions(+), 18 deletions(-)
 create mode 100644 tests/domainbackupxml2xmlin/backup-pull-encrypted.xml
 create mode 100644 tests/domainbackupxml2xmlin/backup-push-encrypted.xml
 create mode 100644 tests/domainbackupxml2xmlout/backup-pull-encrypted.xml
 create mode 100644 tests/domainbackupxml2xmlout/backup-push-encrypted.xml

-- 
2.26.0



[PATCH 1/2] docs: backup: Remove references to push backup to network disk

2020-04-14 Thread Peter Krempa
It was never implemented and for now I don't think there's demand to do
it. Remove the reference.

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

Signed-off-by: Peter Krempa 
---
 docs/formatbackup.html.in | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/docs/formatbackup.html.in b/docs/formatbackup.html.in
index 55acd13ddc..87744bac98 100644
--- a/docs/formatbackup.html.in
+++ b/docs/formatbackup.html.in
@@ -97,12 +97,11 @@
   type
   A mandatory attribute to describe the type of the
 disk, except when backup='no' is
-used. Valid values include file,
-block, or network.
+used. Valid values include file, or
+block.
 Similar to a disk declaration for a domain, the choice of type
 controls what additional sub-elements are needed to describe
-the destination (such as protocol for a
-network destination).
+the destination.
   target
   Valid only for push mode backups, this is the
 primary sub-element that describes the file name of
-- 
2.26.0



[PATCH 2/2] backup: Allow 'encryption' of backups and scratch images

2020-04-14 Thread Peter Krempa
Add the appropriate entries into the schema to allow encryption of the
backup or scratch image. Since we use blockdev internals for everything
no changes to the code are actually necessary.

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

Signed-off-by: Peter Krempa 
---
 docs/formatbackup.html.in | 14 +++-
 docs/schemas/domainbackup.rng | 65 +++
 .../backup-pull-encrypted.xml | 30 +
 .../backup-push-encrypted.xml | 29 +
 .../backup-pull-encrypted.xml | 30 +
 .../backup-push-encrypted.xml | 29 +
 tests/genericxml2xmltest.c|  3 +
 7 files changed, 185 insertions(+), 15 deletions(-)
 create mode 100644 tests/domainbackupxml2xmlin/backup-pull-encrypted.xml
 create mode 100644 tests/domainbackupxml2xmlin/backup-push-encrypted.xml
 create mode 100644 tests/domainbackupxml2xmlout/backup-pull-encrypted.xml
 create mode 100644 tests/domainbackupxml2xmlout/backup-push-encrypted.xml

diff --git a/docs/formatbackup.html.in b/docs/formatbackup.html.in
index 87744bac98..9e69d8f7d3 100644
--- a/docs/formatbackup.html.in
+++ b/docs/formatbackup.html.in
@@ -101,7 +101,7 @@
 block.
 Similar to a disk declaration for a domain, the choice of type
 controls what additional sub-elements are needed to describe
-the destination.
+the destination.
   target
   Valid only for push mode backups, this is the
 primary sub-element that describes the file name of
@@ -110,7 +110,8 @@
 disk. An optional sub-element driver can
 also be used, with an attribute type to
 specify a destination format different from
-qcow2. 
+qcow2. See documentation for scratch below for
+additional configuration.
   scratch
   Valid only for pull mode backups, this is the
 primary sub-element that describes the file name of
@@ -130,7 +131,14 @@
 used without modification. The file is not deleted after the
 backup but the contents of the file don't make sense outside
 of the backup. The same applies for the block device which
-must be formatted appropriately.
+must be formatted appropriately.
+
+Similarly to the domain
+disk
+definition scratch and target can
+contain seclabel and/or encryption
+subelements to configure the corresponding properties.
+  
 
   
 
diff --git a/docs/schemas/domainbackup.rng b/docs/schemas/domainbackup.rng
index 395ea841f9..ac5b12c463 100644
--- a/docs/schemas/domainbackup.rng
+++ b/docs/schemas/domainbackup.rng
@@ -7,6 +7,27 @@

   

+  
+
+  
+
+  luks
+
+  
+  
+
+
+  
+
+  
+  
+
+  
+
+  
+
+  
+
   
 
   
@@ -123,9 +144,14 @@
   
 
   
-  
-
-  
+  
+
+  
+
+
+  
+
+  
 
   
   
@@ -142,9 +168,14 @@
   
 
   
-  
-
-  
+  
+
+  
+
+
+  
+
+  
 
   
   
@@ -192,9 +223,14 @@
   
 
   
-  
-
-  
+  
+
+  
+
+
+  
+
+  
 
 
   
@@ -210,9 +246,14 @@
 
   
 
-
-  
-
+
+  
+
+  
+  
+
+  
+
   
   
 
diff --git a/tests/domainbackupxml2xmlin/backup-pull-encrypted.xml 
b/tests/do

[libvirt PATCH] README: rewrite in rst

2020-04-14 Thread Ján Tomko
Signed-off-by: Ján Tomko 
---
 README  |  2 +-
 README.md => README.rst | 53 +
 2 files changed, 28 insertions(+), 27 deletions(-)
 rename README.md => README.rst (65%)

diff --git a/README b/README
index 42061c01a1..92cacd2853 12
--- a/README
+++ b/README
@@ -1 +1 @@
-README.md
\ No newline at end of file
+README.rst
\ No newline at end of file
diff --git a/README.md b/README.rst
similarity index 65%
rename from README.md
rename to README.rst
index 44b0dd87c5..222ba55e78 100644
--- a/README.md
+++ b/README.rst
@@ -1,6 +1,11 @@
-[![Build 
Status](https://travis-ci.org/libvirt/libvirt.svg)](https://travis-ci.org/libvirt/libvirt)
- [![CII Best 
Practices](https://bestpractices.coreinfrastructure.org/projects/355/badge)](https://bestpractices.coreinfrastructure.org/projects/355)
+.. image:: https://travis-ci.org/libvirt/libvirt.svg
+   :alt: Build Status
+   :target: https://travis-ci.org/libvirt/libvirt
+.. image:: https://bestpractices.coreinfrastructure.org/projects/355/badge
+   :alt: CII Best Practices
+   :target: https://bestpractices.coreinfrastructure.org/projects/355
 
+==
 Libvirt API for virtualization
 ==
 
@@ -21,60 +26,56 @@ mappings into object systems such as GObject, CIM and SNMP.
 Further information about the libvirt project can be found on the
 website:
 
-[https://libvirt.org](https://libvirt.org)
+https://libvirt.org
 
 
 License

+===
 
 The libvirt C API is distributed under the terms of GNU Lesser General
 Public License, version 2.1 (or later). Some parts of the code that are
 not part of the C library may have the more restrictive GNU General
-Public License, version 2.0 (or later). See the files `COPYING.LESSER`
-and `COPYING` for full license terms & conditions.
+Public License, version 2.0 (or later). See the files ``COPYING.LESSER``
+and ``COPYING`` for full license terms & conditions.
 
 
 Installation
-
+
 
 Libvirt uses the GNU Autotools build system, so in general can be built
 and installed with the usual commands, however, we mandate to have the
 build directory different than the source directory. For example, to build
-in a manner that is suitable for installing as root, use:
+in a manner that is suitable for installing as root, use::
 
-```
-$ mkdir build && cd build
-$ ../configure --prefix=/usr --sysconfdir=/etc --localstatedir=/var
-$ make
-$ sudo make install
-```
+   $ mkdir build && cd build
+   $ ../configure --prefix=/usr --sysconfdir=/etc --localstatedir=/var
+   $ make
+   $ sudo make install
 
-While to build & install as an unprivileged user
+While to build & install as an unprivileged user::
 
-```
-$ mkdir build && cd build
-$ ../configure --prefix=$HOME/usr
-$ make
-$ make install
-```
+   $ mkdir build && cd build
+   $ ../configure --prefix=$HOME/usr
+   $ make
+   $ make install
 
 The libvirt code relies on a large number of 3rd party libraries. These will
-be detected during execution of the `configure` script and a summary printed
+be detected during execution of the ``configure`` script and a summary printed
 which lists any missing (optional) dependencies.
 
 
 Contributing
-
+
 
 The libvirt project welcomes contributions in many ways. For most components
 the best way to contribute is to send patches to the primary development
 mailing list. Further guidance on this can be found on the website:
 
-[https://libvirt.org/contribute.html](https://libvirt.org/contribute.html)
+https://libvirt.org/contribute.html
 
 
 Contact

+===
 
 The libvirt project has two primary mailing lists:
 
@@ -83,4 +84,4 @@ The libvirt project has two primary mailing lists:
 
 Further details on contacting the project are available on the website:
 
-[https://libvirt.org/contact.html](https://libvirt.org/contact.html)
+https://libvirt.org/contact.html
-- 
2.25.1



[PATCH 0/2] Fix formatting of http cookies into XML

2020-04-14 Thread Peter Krempa
See 2/2.

Peter Krempa (2):
  virstoragetest: testBackingParse: Use VIR_DOMAIN_DEF_FORMAT_SECURE
when formatting xml
  conf: Don't format http cookies unless VIR_DOMAIN_DEF_FORMAT_SECURE is
used

 src/conf/domain_conf.c | 8 ++--
 tests/virstoragetest.c | 3 ++-
 2 files changed, 8 insertions(+), 3 deletions(-)

-- 
2.26.0



[PATCH 1/2] virstoragetest: testBackingParse: Use VIR_DOMAIN_DEF_FORMAT_SECURE when formatting xml

2020-04-14 Thread Peter Krempa
We want to format even the secure information in tests.

Signed-off-by: Peter Krempa 
---
 tests/virstoragetest.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index 6e8ebeba13..6d2b21c25f 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -594,6 +594,7 @@ testBackingParse(const void *args)
 g_autoptr(virStorageSource) src = NULL;
 int rc;
 int erc = data->rv;
+unsigned int xmlformatflags = VIR_DOMAIN_DEF_FORMAT_SECURE;

 /* expect failure return code with NULL expected data */
 if (!data->expect)
@@ -613,7 +614,7 @@ testBackingParse(const void *args)
 return -1;
 }

-if (virDomainDiskSourceFormat(&buf, src, "source", 0, false, 0, true, 
NULL) < 0 ||
+if (virDomainDiskSourceFormat(&buf, src, "source", 0, false, 
xmlformatflags, true, NULL) < 0 ||
 !(xml = virBufferContentAndReset(&buf))) {
 fprintf(stderr, "failed to format disk source xml\n");
 return -1;
-- 
2.26.0



[PATCH 2/2] conf: Don't format http cookies unless VIR_DOMAIN_DEF_FORMAT_SECURE is used

2020-04-14 Thread Peter Krempa
Starting from 3b076391befc3fe72deb0c244ac6c2b4c100b410
(v6.1.0-122-g3b076391be) we support http cookies. Since they may contain
somewhat sensitive information we should not format into the XML unless
VIR_DOMAIN_DEF_FORMAT_SECURE is asserted.

Reported-by: Han Han 
Signed-off-by: Peter Krempa 
---
 src/conf/domain_conf.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 8e8146374c..8700d56761 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -24636,11 +24636,15 @@ virDomainSourceDefFormatSeclabel(virBufferPtr buf,

 static void
 virDomainDiskSourceFormatNetworkCookies(virBufferPtr buf,
-virStorageSourcePtr src)
+virStorageSourcePtr src,
+unsigned int flags)
 {
 g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
 size_t i;

+if (!(flags & VIR_DOMAIN_DEF_FORMAT_SECURE))
+return;
+
 for (i = 0; i < src->ncookies; i++) {
 virBufferEscapeString(&childBuf, "", 
src->cookies[i]->name);
 virBufferEscapeString(&childBuf, "%s\n", 
src->cookies[i]->value);
@@ -24701,7 +24705,7 @@ virDomainDiskSourceFormatNetwork(virBufferPtr attrBuf,
   virTristateBoolTypeToString(src->sslverify));
 }

-virDomainDiskSourceFormatNetworkCookies(childBuf, src);
+virDomainDiskSourceFormatNetworkCookies(childBuf, src, flags);

 if (src->readahead)
 virBufferAsprintf(childBuf, "\n", 
src->readahead);
-- 
2.26.0



Re: [libvirt-jenkins-ci PATCH v3 4/5] guests: Introduce the new 'gitlab' flavor

2020-04-14 Thread Erik Skultety
On Thu, Apr 09, 2020 at 12:28:50PM +0200, Andrea Bolognani wrote:
> On Thu, 2020-04-09 at 06:23 +0200, Erik Skultety wrote:
> > +++ b/guests/playbooks/update/tasks/gitlab.yml
> > +- name: Make {{ gitlab_runner_config_dir }} world readable
> > +  file:
> > +path: '{{ gitlab_runner_config_dir }}'
> > +mode: '0755'
> > +
> > +- name: Make {{ gitlab_runner_config_dir }}/config.toml world readable
> > +  file:
> > +path: '{{ gitlab_runner_config_dir }}/config.toml'
> > +mode: '0644'
>
> The message for these tasks is unnecessarily detailed: I'd just use
> something like
>
>   Make gitlab-runner configuration readable

Okay, however...

>
> for both.
>
> Additionally, even though the gitlab user is going to be the only one
> on the system so it doesn't make much of a difference in practice, I
> think we should have config.toml
>

...here you suggest the following adjustment. I feel like the messages above
will then become confusing and misleading, since who are we making it readable
for? Well, only for the gitlab user, so I think a little more detail in them is
justifiable.

>   owner: root
>   group: gitlab
>   mode: '0640'

So how about:
"Make gitlab-runner config dir readable" for the former and
"Make gitlab-runner config.toml owned by the gitlab group" for the latter

--
Erik Skultety



Re: [libvirt PATCH 2/2] README-hacking: Drop from the git repository

2020-04-14 Thread Ján Tomko

On a Monday in 2020, Andrea Bolognani wrote:

The newly-introduced CONTRIBUTING.md serves the same purposes and
lives in the path where most people would look for it.

Signed-off-by: Andrea Bolognani 
---
README-hacking | 56 --
1 file changed, 56 deletions(-)
delete mode 100644 README-hacking



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature


Re: [libvirt PATCH 1/2] CONTRIBUTING: Add entry point for new contributors

2020-04-14 Thread Ján Tomko

On a Monday in 2020, Andrea Bolognani wrote:

It's generally expected that a git repository will contain this file,
which serves as an entry point for people interested in contributing
to the project.

In our case, we have extensive documentation available on the
website which we don't want to duplicate, so let's just point people
there.

Signed-off-by: Andrea Bolognani 
---
CONTRIBUTING.md | 18 ++
1 file changed, 18 insertions(+)
create mode 100644 CONTRIBUTING.md

diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
new file mode 100644
index 00..a8551965bd
--- /dev/null
+++ b/CONTRIBUTING.md


s/md/rst/


@@ -0,0 +1,18 @@
+Contributing to libvirt
+===
+
+Full, up to date information on how to contribute to libvirt can be
+found on the libvirt website:
+
+[https://libvirt.org/hacking.html](https://libvirt.org/hacking.html)


https://libvirt.org/contribute.html


+
+To build the same document locally, from the top level directory of
+your git clone run:
+
+```
+$ mkdir build && cd build
+$ ../autogen.sh
+$ make
+```
+
+You'll find the freshly-built document in `docs/hacking.html`.


s/hacking/contribute/


--


Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature


Re: [PATCH 05/43] esx: convert virMutex to GMutex

2020-04-14 Thread Rafael Fonseca
Pino,

thank you for the review. Could you please take a look at patch #42 in
this series? It's the one in which I had to add some explicit unlock
calls, so it'd be good for someone who knows the code to review this
part.

On Tue, Apr 14, 2020 at 8:15 AM Pino Toscano  wrote:
>
> On Friday, 10 April 2020 15:54:32 CEST Rafael Fonseca wrote:
> > @@ -346,11 +342,12 @@ esxStreamClose(virStreamPtr stream, bool finish)
> >  {
> >  int result = 0;
> >  esxStreamPrivate *priv = stream->privateData;
> > +g_autoptr(GMutexLocker) locker = NULL;
> >
> >  if (!priv)
> >  return 0;
> >
> > -virMutexLock(&priv->curl->lock);
> > +locker = g_mutex_locker_new(&priv->curl->lock);
> >
> >  if (finish && priv->backlog_used > 0) {
> >  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > @@ -360,8 +357,6 @@ esxStreamClose(virStreamPtr stream, bool finish)
> >
> >  stream->privateData = NULL;
> >
> > -virMutexUnlock(&priv->curl->lock);
> > -
> >  esxFreeStreamPrivate(&priv);
>
> Careful here, this is a problematic situation:
> - the lock is indirectly part of the @priv structure
> - esxFreeStreamPrivate calls esxVI_CURL_Free(priv->curl)
> - esxVI_CURL_Free calls virMutexDestroy(&item->lock)
> - lock is still locked, so it will deadlock (or crash, or something
>   not good anyway)
>
> You must unlock the mutex before esxFreeStreamPrivate is called.

Ops, nice catch.

> I did not check other patches of this long series for similar potential
> issues, please do check them.

Ok, will do.

> > diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
> > index 16690edfbe..ed6c6c28cd 100644
> > --- a/src/esx/esx_vi.c
> > +++ b/src/esx/esx_vi.c
> > @@ -433,14 +429,13 @@ int
> >  esxVI_CURL_Upload(esxVI_CURL *curl, const char *url, const char *content)
> >  {
> >  int responseCode = 0;
> > +g_autoptr(GMutexLocker) locker = g_mutex_locker_new(&curl->lock);
> >
> >  if (!content) {
> >  virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid 
> > argument"));
> >  return -1;
> >  }
> >
> > -virMutexLock(&curl->lock);
> > -
>
> Careful #2 here about locking earlier: while usually this is not an
> issue, it could be in case the code that was executed without the lock
> held can be called by other code branches with the lock held.
>
> Again, this must be thoroughly checked in the whole patch series.

I'll recheck but I tried to do it in places where it was obvious the
lock was not held because of an early return case.


Att.
-- 
Rafael Fonseca




FYI: Report about libvirt failing to compile on MSYS2 with an XDR error

2020-04-14 Thread Richard W.M. Jones


.. is here:

https://github.com/msys2/MINGW-packages/issues/6384

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v



Re: [libvirt PATCH 0/2] Improve entry point for new contributors

2020-04-14 Thread Andrea Bolognani
On Mon, 2020-04-06 at 12:07 +0200, Andrea Bolognani wrote:
> Use the path most people will look for, reduce building instructions
> to the bare minimum necessary to locally bootstrap the learning
> process, and ensure duplication doesn't creep in over time by
> pointing to the full documentation.
> 
> Andrea Bolognani (2):
>   CONTRIBUTING: Add entry point for new contributors
>   README-hacking: Drop from the git repository
> 
>  CONTRIBUTING.md | 18 
>  README-hacking  | 56 -
>  2 files changed, 18 insertions(+), 56 deletions(-)
>  create mode 100644 CONTRIBUTING.md
>  delete mode 100644 README-hacking

ping

-- 
Andrea Bolognani / Red Hat / Virtualization