[libvirt] [PATCH] qemu: fix hang in p2p + xbzrle compression + parallel migration

[Lin Ma]

When we do parallel migration, The multifd-channels migration parameter
needs to be set on the destination side as well before incoming migration
URI, unless we accept the default number of connections(2).

Usually, This can be correctly handled by libvirtd. But in this case if
we use p2p + xbzrle compression without parameter '--comp-xbzrle-cache',
qemuMigrationParamsDump returns too early, The corresponding migration 
parameter will not be set on the destination side, It results QEMU hangs.

Reproducer:
virsh migrate --live --p2p --comp-methods xbzrle \
--parallel --parallel-connections 3 GUEST qemu+ssh://dsthost/system

or

virsh migrate --live --p2p --compressed \
--parallel --parallel-connections 3 GUEST qemu+ssh://dsthost/system

Signed-off-by: Lin Ma 
---
 src/qemu/qemu_migration_params.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c
index dd5e2ce1b6..810199370f 100644
--- a/src/qemu/qemu_migration_params.c
+++ b/src/qemu/qemu_migration_params.c
@@ -630,7 +630,6 @@ qemuMigrationParamsDump(qemuMigrationParamsPtr migParams,
 if (migParams->compMethods == 1ULL << QEMU_MIGRATION_COMPRESS_XBZRLE &&
 !migParams->params[QEMU_MIGRATION_PARAM_XBZRLE_CACHE_SIZE].set) {
 *flags |= VIR_MIGRATE_COMPRESSED;
-return 0;
 }
 
 for (i = 0; i < QEMU_MIGRATION_COMPRESS_LAST; ++i) {
-- 
2.23.0

Re: [libvirt PATCH] docs: kbase: add a page about debugging

[Han Han]

On Wed, Apr 15, 2020 at 11:13 PM Ján Tomko  wrote:

> Migrate the following wiki articles:
> https://wiki.libvirt.org/page/DebugLogs
> https://wiki.libvirt.org/page/Debugging
>
> With the syntax changed to rST and rewrapped.
>
> Signed-off-by: Ján Tomko 
> ---
> I might've changed an article or two, too.
> Or rather: the article 'the'.
>
>  docs/kbase.html.in   |   3 +
>  docs/kbase/debugging.rst | 303 +++
>  2 files changed, 306 insertions(+)
>  create mode 100644 docs/kbase/debugging.rst
>
> diff --git a/docs/kbase.html.in b/docs/kbase.html.in
> index c586e0f676..1a50428805 100644
> --- a/docs/kbase.html.in
> +++ b/docs/kbase.html.in
> @@ -36,6 +36,9 @@
>
>  Virtio-FS
>  Share a filesystem between the guest and the host
> +
> +Debugging libvirt
> +How to capture debug logs and use a debugger on libvirt
>
>  
>
> diff --git a/docs/kbase/debugging.rst b/docs/kbase/debugging.rst
> new file mode 100644
> index 00..a9bc02aa55
> --- /dev/null
> +++ b/docs/kbase/debugging.rst
> @@ -0,0 +1,303 @@
> +=
> +Debugging libvirt
> +=
> +
> +.. contents::
> +
> +
> +If you `report a bug `_ against libvirt,
> in most
> +cases you will be asked to attach debug logs. These are bare text files
> which
> +tracks transition between different states of libvirtd, what it has tried
> to
> +achieve, etc. Because of client -- server schema used in libvirt, the
> logs can
> +be either client or server too. Usually, it's server side that matters as
> +nearly all interesting work takes place there. Moreover, libvirt catches
> stderr
> +of all running domains. These can be useful as well.
> +
> +
> +How to turn on debug logs for libvirt
> +=
> +
> +Persistent setting
> +--
> +
> +The daemon configuration files location is dependent on the `connection
> URI
> +`_. For ``qemu:///system``:
> +
> +* open ``/etc/libvirt/libvirtd.conf`` in your favourite editor
> +* find & replace, or set these variables::
> +
> +   # LEGACY SETTINGS PRIOR LIBVIRT 4.4.0 SEE BELOW! #
> +   log_level = 1
> +   log_filters="1:qemu 3:remote 4:event 3:util.json 3:rpc"
> +   log_outputs="1:file:/var/log/libvirt/libvirtd.log"
> +
> +
> +   # PREFERRED SETTINGS AFTER LIBVIRT 4.4.0 #
> +   log_filters="3:remote 4:event 3:util.json 3:rpc 1:*"
> +   log_outputs="1:file:/var/log/libvirt/libvirtd.log"
> +
> +* save and exit
> +* restart libvirtd service::
> +
> +   systemctl restart libvirtd.service
> +
> +In the config variables above, we have set logging level to 1 (debug
> level),
> +set some filters (to filter out noise), e.g. from rpc only warnings
> (=level 3)
>
I think it is worth explaining the show all the filters and the field of
logs they gathered.
For example, if you will report an issue on storage pool, the filter should
be '1:storage'

> +and above will be reported. The logs are saved into
> +``/var/log/libvirt/libvirtd.log``. Since libvirt *4.4.0* log filters
> +support shell globbing, therefore the usage of ``log_level`` is considered
> +deprecated in favour of pure usage of ``log_filters``.
> +
> +In case you want to get the client logs, you need to set this environment
> variable::
> +
> + export LIBVIRT_LOG_OUTPUTS="1:file:/tmp/libvirt_client.log"
> +
> +However, when you are using the session mode ``qemu:///session`` or you
> run the
> +``libvirtd`` as unprivileged user you will find configuration file under
> +``$XDG_CONFIG_HOME/libvirt/libvirt.conf``.
> +
> +
> +Runtime setting
> +---
> +
> +Debugging anomalies can be very painful, especially when trying to
> reproduce it
> +after the daemon restarts, since the new session can make the anomaly
> +"disappear". Therefore, it's possible to enable the debug logs during
> runtime
> +using libvirt administration API. To use it conveniently, there's a
> ``virt-admin``
> +client provided by the ``libvirt-admin`` package. Use the package manager
> provided
> +by your distribution to install this package. Once you have it installed,
> run
> +the following as root to see the set of log filters currently being
> active::
> +
> +   # virt-admin daemon-log-filters
> +   Logging filters: 3:remote 4:util.json 4:rpc
> +
> +In order to change this set, run the same command as root, this time with
> your own set of filters::
> +
> +   ## LEGACY APPROACH ENUMERATING ALL THE DESIRED MODULES ##
> +   # virt-admin daemon-log-filters "1:util 1:libvirt 1:storage 1:network
> 1:nodedev 1:qemu"
> +
> +   ## CURRENT APPROACH USING SHELL GLOBBING ##
> +   # virt-admin daemon-log-filters "3:remote 4:util.json 4:rpc 1:*"
> +
> +Analogically, the same procedure can be performed with log outputs::
> +
> +   # virt-admin daemon-log-outputs
> +   Logging outputs: 3:syslog:libvirtd
> +   # virt-admin daemon-log-outputs "1:file:/var/log/libvirt/libvirtd.log"
> +
> +NOTE: It's always good prac

Re: [libvirt PATCH 4/4] docs: Remove one example from pci-addresses.rst

[Cornelia Huck]

On Wed, 15 Apr 2020 19:31:36 +0200
Andrea Bolognani  wrote:

> The idea behind this document is to show, with actual examples,
> that users should not expect PCI addresses in the domain XML and
> in the guest OS to match.
> 
> The first zPCI example already serves this purpose perfectly, so
> in the interest of keeping the page as brief and easy to digest
> as possible the second one is removed.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  docs/pci-addresses.rst | 19 ---
>  1 file changed, 19 deletions(-)
> 
> diff --git a/docs/pci-addresses.rst b/docs/pci-addresses.rst
> index 86a41df6ce..1d2dc8e5fc 100644
> --- a/docs/pci-addresses.rst
> +++ b/docs/pci-addresses.rst
> @@ -204,25 +204,6 @@ will result in the exactly same view in the guest, as 
> the addresses there
>  are generated from the information provided via the ``zpci`` element (in
>  fact, from the ``uid``).
>  
> -Therefore, replacing the virtio-net device definition with the following XML
> -snippet
> -
> -::
> -
> -  
> -
> -
> - function='0x3'>
> -  
> -
> -  
> -
> -will yield the following result in a Linux guest:
> -
> -::
> -
> -  0007:00:00.0 Ethernet controller: Red Hat, Inc. Virtio network device
> -
>  
>  Device assignment
>  =

Hm, should that rather go somewhere else? What I wanted to show is "you
can have the same PCI address in the XML and still get a different PCI
address in the guest, if you change the zpci values", as that might be
another source of confusion.

Re: [libvirt PATCH 3/4] docs: Remove MAC addresses from pci-addresses.rst

[Cornelia Huck]

On Wed, 15 Apr 2020 19:31:35 +0200
Andrea Bolognani  wrote:

> Signed-off-by: Andrea Bolognani 
> ---
>  docs/pci-addresses.rst | 2 --
>  1 file changed, 2 deletions(-)

Yeah, not that useful.

Reviewed-by: Cornelia Huck 

Re: [libvirt PATCH 1/4] docs: Use consistent style in pci-addresses.rst

[Cornelia Huck]

On Wed, 15 Apr 2020 19:31:33 +0200
Andrea Bolognani  wrote:

> Indent all code snippets by the same number of spaces, and don't
> embed the :: marker in the line preceding a code block.
> 
> This commit is best viewed with 'git show -w'.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  docs/pci-addresses.rst | 60 ++
>  1 file changed, 32 insertions(+), 28 deletions(-)

Reviewed-by: Cornelia Huck 

Re: [libvirt PATCH 2/4] docs: Move sections around in pci-addresses.rst

[Cornelia Huck]

On Wed, 15 Apr 2020 19:31:34 +0200
Andrea Bolognani  wrote:

> The section about VFIO devices is kept separate from the rest
> because it's less about domain XML and guest OS disagreeing on the
> PCI address of a device, and more about which of the two PCI
> addresses in the domain XML is even relevant to the guest OS.
> 
> The section on zPCI addresses, on the other hand, falls squarely
> in the "more complex cases" category, so it should live in the
> corresponding section.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  docs/pci-addresses.rst | 57 +-
>  1 file changed, 28 insertions(+), 29 deletions(-)

Not including a 'wonky cases' category is fine with me :)

Reviewed-by: Cornelia Huck 

Re: [PATCH] docs: add zpci information to pci-addresses.rst

[Andrea Bolognani]

On Wed, 2020-04-15 at 12:51 +0200, Cornelia Huck wrote:
> Add some information on how pci address work on s390x.
> 
> Signed-off-by: Cornelia Huck 
> ---
>  docs/pci-addresses.rst | 63 ++
>  1 file changed, 63 insertions(+)

There are a few comments that I didn't have a chance to raise before
the patch was pushed, so I just posted a follow-up series addressing
them:

  https://www.redhat.com/archives/libvir-list/2020-April/msg00749.html

Please take a look and let me know whether you're okay with them.

-- 
Andrea Bolognani / Red Hat / Virtualization

[libvirt PATCH 3/4] docs: Remove MAC addresses from pci-addresses.rst

[Andrea Bolognani]

Signed-off-by: Andrea Bolognani 
---
 docs/pci-addresses.rst | 2 --
 1 file changed, 2 deletions(-)

diff --git a/docs/pci-addresses.rst b/docs/pci-addresses.rst
index 566c81d957..86a41df6ce 100644
--- a/docs/pci-addresses.rst
+++ b/docs/pci-addresses.rst
@@ -177,7 +177,6 @@ In the simplest case, the following XML snippet
 
   
   
-
 
 
 
@@ -211,7 +210,6 @@ snippet
 ::
 
   
-
 
 
 
-- 
2.25.2

[libvirt PATCH 4/4] docs: Remove one example from pci-addresses.rst

[Andrea Bolognani]

The idea behind this document is to show, with actual examples,
that users should not expect PCI addresses in the domain XML and
in the guest OS to match.

The first zPCI example already serves this purpose perfectly, so
in the interest of keeping the page as brief and easy to digest
as possible the second one is removed.

Signed-off-by: Andrea Bolognani 
---
 docs/pci-addresses.rst | 19 ---
 1 file changed, 19 deletions(-)

diff --git a/docs/pci-addresses.rst b/docs/pci-addresses.rst
index 86a41df6ce..1d2dc8e5fc 100644
--- a/docs/pci-addresses.rst
+++ b/docs/pci-addresses.rst
@@ -204,25 +204,6 @@ will result in the exactly same view in the guest, as the 
addresses there
 are generated from the information provided via the ``zpci`` element (in
 fact, from the ``uid``).
 
-Therefore, replacing the virtio-net device definition with the following XML
-snippet
-
-::
-
-  
-
-
-
-  
-
-  
-
-will yield the following result in a Linux guest:
-
-::
-
-  0007:00:00.0 Ethernet controller: Red Hat, Inc. Virtio network device
-
 
 Device assignment
 =
-- 
2.25.2

[libvirt PATCH 2/4] docs: Move sections around in pci-addresses.rst

[Andrea Bolognani]

The section about VFIO devices is kept separate from the rest
because it's less about domain XML and guest OS disagreeing on the
PCI address of a device, and more about which of the two PCI
addresses in the domain XML is even relevant to the guest OS.

The section on zPCI addresses, on the other hand, falls squarely
in the "more complex cases" category, so it should live in the
corresponding section.

Signed-off-by: Andrea Bolognani 
---
 docs/pci-addresses.rst | 57 +-
 1 file changed, 28 insertions(+), 29 deletions(-)

diff --git a/docs/pci-addresses.rst b/docs/pci-addresses.rst
index 0b83bb231f..566c81d957 100644
--- a/docs/pci-addresses.rst
+++ b/docs/pci-addresses.rst
@@ -158,36 +158,8 @@ Once again, while the PCI addresses seen in the domain XML 
and those
 seen by the guest OS do not match, the relationships between the
 various devices are preserved.
 
-
-Device assignment
-=
-
-When using VFIO to assign host devices to a guest, an additional
-caveat to keep in mind that the guest OS will base its decisions upon
-the *target address* (guest side) rather than the *source address*
-(host side).
-
-For example, the domain XML snippet
-
-::
-
-  
-
-
-  
-
-
-  
-
-will result in the device showing up as ``:00:01.0`` in the
-guest OS rather than as ``0001:08:00.1``, which is the address of the
-device on the host.
-
-Of course, all the rules and behaviors described above still apply.
-
-
 zPCI addresses
-==
+--
 
 For s390x machines, PCI addresses are handled yet differently. No
 topology information is relayed in the PCI addresses; instead, the
@@ -252,3 +224,30 @@ will yield the following result in a Linux guest:
 ::
 
   0007:00:00.0 Ethernet controller: Red Hat, Inc. Virtio network device
+
+
+Device assignment
+=
+
+When using VFIO to assign host devices to a guest, an additional
+caveat to keep in mind that the guest OS will base its decisions upon
+the *target address* (guest side) rather than the *source address*
+(host side).
+
+For example, the domain XML snippet
+
+::
+
+  
+
+
+  
+
+
+  
+
+will result in the device showing up as ``:00:01.0`` in the
+guest OS rather than as ``0001:08:00.1``, which is the address of the
+device on the host.
+
+Of course, all the rules and behaviors described above still apply.
-- 
2.25.2

[libvirt PATCH 1/4] docs: Use consistent style in pci-addresses.rst

[Andrea Bolognani]

Indent all code snippets by the same number of spaces, and don't
embed the :: marker in the line preceding a code block.

This commit is best viewed with 'git show -w'.

Signed-off-by: Andrea Bolognani 
---
 docs/pci-addresses.rst | 60 ++
 1 file changed, 32 insertions(+), 28 deletions(-)

diff --git a/docs/pci-addresses.rst b/docs/pci-addresses.rst
index fbc741e344..0b83bb231f 100644
--- a/docs/pci-addresses.rst
+++ b/docs/pci-addresses.rst
@@ -196,24 +196,26 @@ In the simplest case, the following XML snippet
 
 ::
 
-  
-  
-
-
-
-  
-
-  
-  
-
-
-
-
-  
-
-  
-
-will result in the following in a Linux guest::
+  
+  
+
+
+
+  
+
+  
+  
+
+
+
+
+  
+
+  
+
+will result in the following in a Linux guest:
+
+::
 
   0001:00:00.0 Ethernet controller: Red Hat, Inc. Virtio network device
 
@@ -221,7 +223,7 @@ Note that the PCI bridge is not visible in the guest; s390x 
always has a flat
 topology.
 
 Neither are any changes in the PCI address visible in the guest; replacing
-the PCI address for the virtio-net device with
+the PCI address for the ``virtio-net`` device with
 
 ::
 
@@ -236,15 +238,17 @@ snippet
 
 ::
 
-  
-
-
-
-
-  
-
-  
+  
+
+
+
+
+  
+
+  
 
-will yield the following result in a Linux guest::
+will yield the following result in a Linux guest:
+
+::
 
   0007:00:00.0 Ethernet controller: Red Hat, Inc. Virtio network device
-- 
2.25.2

[libvirt PATCH 0/4] docs: A few adjustments to pci-addresses.rst

[Andrea Bolognani]

Andrea Bolognani (4):
  docs: Use consistent style in pci-addresses.rst
  docs: Move sections around in pci-addresses.rst
  docs: Remove MAC addresses from pci-addresses.rst
  docs: Remove one example from pci-addresses.rst

 docs/pci-addresses.rst | 102 +
 1 file changed, 42 insertions(+), 60 deletions(-)

-- 
2.25.2

Re: [PATCH] docs: add zpci information to pci-addresses.rst

[Cornelia Huck]

On Wed, 15 Apr 2020 18:38:06 +0200
Andrea Bolognani  wrote:

> On Wed, 2020-04-15 at 16:45 +0200, Cornelia Huck wrote:
> > On Wed, 15 Apr 2020 16:23:46 +0200
> > Boris Fiuczynski  wrote:  
> > > Once I understand you confusion above I will provide a patch...  
> > 
> > I'd say just go ahead.  
> 
> While I appreciate the sentiment, I would rather not have an
> excessive amount of detail added to this page.
> 
> The idea behind it was to show users that they shouldn't expect the
> address in the domain XML to match the one in the guest OS, with a
> few real-life examples to illustrate the point. So, I don't think
> the details of how exactly zPCI IDs translate to guest-visible PCI
> addresses is in scope.
> 
> It would be great information to have in some other document, though!
> Is there something like that in qemu.git, or in the QEMU wiki? Those
> are the places where I would expect it to live, since it's not really
> tied to libvirt...

I don't think there's much in QEMU documentation yet; someone(tm) wrote
https://virtualpenguins.blogspot.com/2018/02/notes-on-pci-on-s390x.html,
which might provide a starting point.

Re: [PATCH] docs: add zpci information to pci-addresses.rst

[Andrea Bolognani]

On Wed, 2020-04-15 at 16:45 +0200, Cornelia Huck wrote:
> On Wed, 15 Apr 2020 16:23:46 +0200
> Boris Fiuczynski  wrote:
> > Once I understand you confusion above I will provide a patch...
> 
> I'd say just go ahead.

While I appreciate the sentiment, I would rather not have an
excessive amount of detail added to this page.

The idea behind it was to show users that they shouldn't expect the
address in the domain XML to match the one in the guest OS, with a
few real-life examples to illustrate the point. So, I don't think
the details of how exactly zPCI IDs translate to guest-visible PCI
addresses is in scope.

It would be great information to have in some other document, though!
Is there something like that in qemu.git, or in the QEMU wiki? Those
are the places where I would expect it to live, since it's not really
tied to libvirt...

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH 0/2] Include lease time option into DHCP settings

[Julio Faracco]

I resubmitted this series because our team needs to hack dnsmasq
settings to change lease time. This feature would be so important to
us to avoid workarounds.

It is based on Alberto's patch from 2017. But personally I don't like
this approach.
IMHO, it would be nice to have specific attributes to configure lease time.
For example:



They can be different from each other.
I still think that the idea should be better developed.
I don't like that my example also (it is just an example).
That's why I submitted... To listen opinions from others.

--
Julio Cesar Faracco

Em qua., 15 de abr. de 2020 às 13:19, Julio Faracco
 escreveu:
>
> This series is based on latest series from Alberto. It includes a new
> entry called  under  scope to add a default lease
> time for range and host options for dnsmasq. There is no point to
> configure both separately. If they are defined (range and/or host), they
> should have the same lease time value.
>
> This series includes some test cases to cover lease time XML syntax
> also.
>
> Julio Faracco (2):
>   conf: Add  option for  settings
>   tests: Add tests for  to cover dnsmasq settings
>
>  docs/schemas/basictypes.rng   |  9 +++
>  docs/schemas/network.rng  | 11 
>  src/conf/network_conf.c   | 62 ++-
>  src/conf/network_conf.h   | 14 +
>  src/libvirt_private.syms  |  2 +
>  src/network/bridge_driver.c   | 37 ++-
>  src/util/virdnsmasq.c | 40 ++--
>  src/util/virdnsmasq.h |  1 +
>  .../networkxml2confdata/leasetime-hours.conf  | 16 +
>  tests/networkxml2confdata/leasetime-hours.xml | 12 
>  .../leasetime-infinite.conf   | 16 +
>  .../leasetime-infinite.xml| 12 
>  .../leasetime-minutes.conf| 16 +
>  .../networkxml2confdata/leasetime-minutes.xml | 12 
>  .../leasetime-seconds.conf| 16 +
>  .../networkxml2confdata/leasetime-seconds.xml | 12 
>  tests/networkxml2conftest.c   |  4 ++
>  tests/networkxml2xmlin/leasetime-hours.xml| 12 
>  tests/networkxml2xmlin/leasetime-infinite.xml | 12 
>  tests/networkxml2xmlin/leasetime-minutes.xml  | 12 
>  tests/networkxml2xmlin/leasetime-seconds.xml  | 12 
>  tests/networkxml2xmlout/leasetime-hours.xml   | 14 +
>  .../networkxml2xmlout/leasetime-infinite.xml  | 14 +
>  tests/networkxml2xmlout/leasetime-minutes.xml | 14 +
>  tests/networkxml2xmlout/leasetime-seconds.xml | 14 +
>  tests/networkxml2xmltest.c|  4 ++
>  26 files changed, 376 insertions(+), 24 deletions(-)
>  create mode 100644 tests/networkxml2confdata/leasetime-hours.conf
>  create mode 100644 tests/networkxml2confdata/leasetime-hours.xml
>  create mode 100644 tests/networkxml2confdata/leasetime-infinite.conf
>  create mode 100644 tests/networkxml2confdata/leasetime-infinite.xml
>  create mode 100644 tests/networkxml2confdata/leasetime-minutes.conf
>  create mode 100644 tests/networkxml2confdata/leasetime-minutes.xml
>  create mode 100644 tests/networkxml2confdata/leasetime-seconds.conf
>  create mode 100644 tests/networkxml2confdata/leasetime-seconds.xml
>  create mode 100644 tests/networkxml2xmlin/leasetime-hours.xml
>  create mode 100644 tests/networkxml2xmlin/leasetime-infinite.xml
>  create mode 100644 tests/networkxml2xmlin/leasetime-minutes.xml
>  create mode 100644 tests/networkxml2xmlin/leasetime-seconds.xml
>  create mode 100644 tests/networkxml2xmlout/leasetime-hours.xml
>  create mode 100644 tests/networkxml2xmlout/leasetime-infinite.xml
>  create mode 100644 tests/networkxml2xmlout/leasetime-minutes.xml
>  create mode 100644 tests/networkxml2xmlout/leasetime-seconds.xml
>
> --
> 2.24.1
>

[PATCH 2/2] tests: Add tests for to cover dnsmasq settings

[Julio Faracco]

New tests are required to cover some new XML syntax entry for
 option. This includes schema testing and other features
like unit attribute and leasetime value.

Signed-off-by: Julio Faracco 
---
 tests/networkxml2confdata/leasetime-hours.conf   | 16 
 tests/networkxml2confdata/leasetime-hours.xml| 12 
 .../networkxml2confdata/leasetime-infinite.conf  | 16 
 tests/networkxml2confdata/leasetime-infinite.xml | 12 
 tests/networkxml2confdata/leasetime-minutes.conf | 16 
 tests/networkxml2confdata/leasetime-minutes.xml  | 12 
 tests/networkxml2confdata/leasetime-seconds.conf | 16 
 tests/networkxml2confdata/leasetime-seconds.xml  | 12 
 tests/networkxml2conftest.c  |  4 
 tests/networkxml2xmlin/leasetime-hours.xml   | 12 
 tests/networkxml2xmlin/leasetime-infinite.xml| 12 
 tests/networkxml2xmlin/leasetime-minutes.xml | 12 
 tests/networkxml2xmlin/leasetime-seconds.xml | 12 
 tests/networkxml2xmlout/leasetime-hours.xml  | 14 ++
 tests/networkxml2xmlout/leasetime-infinite.xml   | 14 ++
 tests/networkxml2xmlout/leasetime-minutes.xml| 14 ++
 tests/networkxml2xmlout/leasetime-seconds.xml| 14 ++
 tests/networkxml2xmltest.c   |  4 
 18 files changed, 224 insertions(+)
 create mode 100644 tests/networkxml2confdata/leasetime-hours.conf
 create mode 100644 tests/networkxml2confdata/leasetime-hours.xml
 create mode 100644 tests/networkxml2confdata/leasetime-infinite.conf
 create mode 100644 tests/networkxml2confdata/leasetime-infinite.xml
 create mode 100644 tests/networkxml2confdata/leasetime-minutes.conf
 create mode 100644 tests/networkxml2confdata/leasetime-minutes.xml
 create mode 100644 tests/networkxml2confdata/leasetime-seconds.conf
 create mode 100644 tests/networkxml2confdata/leasetime-seconds.xml
 create mode 100644 tests/networkxml2xmlin/leasetime-hours.xml
 create mode 100644 tests/networkxml2xmlin/leasetime-infinite.xml
 create mode 100644 tests/networkxml2xmlin/leasetime-minutes.xml
 create mode 100644 tests/networkxml2xmlin/leasetime-seconds.xml
 create mode 100644 tests/networkxml2xmlout/leasetime-hours.xml
 create mode 100644 tests/networkxml2xmlout/leasetime-infinite.xml
 create mode 100644 tests/networkxml2xmlout/leasetime-minutes.xml
 create mode 100644 tests/networkxml2xmlout/leasetime-seconds.xml

diff --git a/tests/networkxml2confdata/leasetime-hours.conf 
b/tests/networkxml2confdata/leasetime-hours.conf
new file mode 100644
index 00..1599d4633e
--- /dev/null
+++ b/tests/networkxml2confdata/leasetime-hours.conf
@@ -0,0 +1,16 @@
+##WARNING:  THIS IS AN AUTO-GENERATED FILE. CHANGES TO IT ARE LIKELY TO BE
+##OVERWRITTEN AND LOST.  Changes to this configuration should be made using:
+##virsh net-edit default
+## or other application using the libvirt API.
+##
+## dnsmasq conf file created by libvirt
+strict-order
+except-interface=lo
+bind-dynamic
+interface=virbr0
+dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0,1h
+dhcp-no-override
+dhcp-authoritative
+dhcp-lease-max=253
+dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile
+addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts
diff --git a/tests/networkxml2confdata/leasetime-hours.xml 
b/tests/networkxml2confdata/leasetime-hours.xml
new file mode 100644
index 00..8df196e84e
--- /dev/null
+++ b/tests/networkxml2confdata/leasetime-hours.xml
@@ -0,0 +1,12 @@
+http://libvirt.org/schemas/network/dnsmasq/1.0";>
+  default
+  81ff0d90-c91e-6742-64da-4a736edb9a9b
+  
+  
+  
+
+  
+  1
+
+  
+
diff --git a/tests/networkxml2confdata/leasetime-infinite.conf 
b/tests/networkxml2confdata/leasetime-infinite.conf
new file mode 100644
index 00..883ced6ade
--- /dev/null
+++ b/tests/networkxml2confdata/leasetime-infinite.conf
@@ -0,0 +1,16 @@
+##WARNING:  THIS IS AN AUTO-GENERATED FILE. CHANGES TO IT ARE LIKELY TO BE
+##OVERWRITTEN AND LOST.  Changes to this configuration should be made using:
+##virsh net-edit default
+## or other application using the libvirt API.
+##
+## dnsmasq conf file created by libvirt
+strict-order
+except-interface=lo
+bind-dynamic
+interface=virbr0
+dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0,infinite
+dhcp-no-override
+dhcp-authoritative
+dhcp-lease-max=253
+dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile
+addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts
diff --git a/tests/networkxml2confdata/leasetime-infinite.xml 
b/tests/networkxml2confdata/leasetime-infinite.xml
new file mode 100644
index 00..9ae3c4f7f7
--- /dev/null
+++ b/tests/networkxml2confdata/leasetime-infinite.xml
@@ -0,0 +1,12 @@
+http://libvirt.org/schemas/network/dnsmasq/1.0";>
+  default
+  81ff0d90-c91e-6742-64da-4a736edb9a9b
+  
+  
+  
+
+  
+  
+
+  
+
diff --git a/tests/networ

[PATCH 1/2] conf: Add option for settings

[Julio Faracco]

If an user is trying to configure a dhcp neetwork settings, it is not
possible to change the leasetime of a range or a host entry. This is
available using dnsmasq extra options, but they are associated with
dhcp-range or dhcp-hosts fields. This patch implements a default
leasetime for both. If this XML entry is defined, it applies leasetime
for each range or host defined under DHCP scope.

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

Signed-off-by: Julio Faracco 
---
 docs/schemas/basictypes.rng |  9 ++
 docs/schemas/network.rng| 11 +++
 src/conf/network_conf.c | 62 -
 src/conf/network_conf.h | 14 +
 src/libvirt_private.syms|  2 ++
 src/network/bridge_driver.c | 37 --
 src/util/virdnsmasq.c   | 40 
 src/util/virdnsmasq.h   |  1 +
 8 files changed, 152 insertions(+), 24 deletions(-)

diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng
index 81465273c8..12f085c101 100644
--- a/docs/schemas/basictypes.rng
+++ b/docs/schemas/basictypes.rng
@@ -607,4 +607,13 @@
 
   
 
+  
+
+  seconds
+  minutes
+  hours
+  infinite
+
+  
+
 
diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
index 60453225d6..9a93529d52 100644
--- a/docs/schemas/network.rng
+++ b/docs/schemas/network.rng
@@ -398,6 +398,17 @@
 
   
 
+
+  
+
+  
+
+
+  
+  
+
+  
+
   
 
   
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 819b645df7..e6e82ee9fc 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -70,6 +70,14 @@ VIR_ENUM_IMPL(virNetworkTaint,
   "hook-script",
 );
 
+VIR_ENUM_IMPL(virNetworkDHCPLeaseTimeUnit,
+  VIR_NETWORK_DHCP_LEASETIME_UNIT_LAST,
+  "seconds",
+  "minutes",
+  "hours",
+  "infinite",
+);
+
 static virClassPtr virNetworkXMLOptionClass;
 
 static void
@@ -552,9 +560,50 @@ virNetworkDHCPHostDefParseXML(const char *networkName,
 }
 
 
+static int
+virNetworkDHCPLeaseTimeDefParseXML(virNetworkIPDefPtr def,
+   xmlNodePtr node,
+   xmlXPathContextPtr ctxt)
+{
+g_autofree char *leasetime = NULL, *leaseunit = NULL;
+
+if (!(leaseunit = virXMLPropString(node, "unit")))
+def->leaseunit = VIR_NETWORK_DHCP_LEASETIME_UNIT_SECONDS;
+else
+def->leaseunit = virNetworkDHCPLeaseTimeUnitTypeFromString(leaseunit);
+
+if (def->leaseunit == VIR_NETWORK_DHCP_LEASETIME_UNIT_INFINITE)
+return 0;
+
+if (!(leasetime = virXPathString("string(./dhcp/leasetime)", ctxt)))
+return -1;
+
+if (virStrToLong_ul(leasetime, NULL, 10, &def->leasetime) < 0)
+return -1;
+
+/* This boundary check is related to dnsmasq man page settings:
+ * "The minimum lease time is two minutes." */
+if ((def->leaseunit == VIR_NETWORK_DHCP_LEASETIME_UNIT_SECONDS &&
+ def->leasetime < 120) ||
+(def->leaseunit == VIR_NETWORK_DHCP_LEASETIME_UNIT_MINUTES &&
+ def->leasetime < 2)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("The minimum lease time should be greater "
+ "than 2 minutes"));
+return -1;
+}
+
+if (def->leasetime > 0)
+return 0;
+
+return -1;
+}
+
+
 static int
 virNetworkDHCPDefParseXML(const char *networkName,
   xmlNodePtr node,
+  xmlXPathContextPtr ctxt,
   virNetworkIPDefPtr def)
 {
 int ret = -1;
@@ -583,7 +632,11 @@ virNetworkDHCPDefParseXML(const char *networkName,
 goto cleanup;
 if (VIR_APPEND_ELEMENT(def->hosts, def->nhosts, host) < 0)
 goto cleanup;
+} else if (cur->type == XML_ELEMENT_NODE &&
+virXMLNodeNameEqual(cur, "leasetime")) {
 
+if (virNetworkDHCPLeaseTimeDefParseXML(def, cur, ctxt) < 0)
+goto cleanup;
 } else if (VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET) &&
cur->type == XML_ELEMENT_NODE &&
virXMLNodeNameEqual(cur, "bootp")) {
@@ -1143,7 +1196,7 @@ virNetworkIPDefParseXML(const char *networkName,
 }
 
 if ((dhcp = virXPathNode("./dhcp[1]", ctxt)) &&
-virNetworkDHCPDefParseXML(networkName, dhcp, def) < 0)
+virNetworkDHCPDefParseXML(networkName, dhcp, ctxt, def) < 0)
 goto cleanup;
 
 if (virXPathNode("./tftp[1]", ctxt)) {
@@ -2343,6 +2396,13 @@ virNetworkIPDefFormat(virBufferPtr buf,
  

[PATCH 0/2] Include lease time option into DHCP settings

[Julio Faracco]

This series is based on latest series from Alberto. It includes a new
entry called  under  scope to add a default lease
time for range and host options for dnsmasq. There is no point to
configure both separately. If they are defined (range and/or host), they
should have the same lease time value.

This series includes some test cases to cover lease time XML syntax
also.

Julio Faracco (2):
  conf: Add  option for  settings
  tests: Add tests for  to cover dnsmasq settings

 docs/schemas/basictypes.rng   |  9 +++
 docs/schemas/network.rng  | 11 
 src/conf/network_conf.c   | 62 ++-
 src/conf/network_conf.h   | 14 +
 src/libvirt_private.syms  |  2 +
 src/network/bridge_driver.c   | 37 ++-
 src/util/virdnsmasq.c | 40 ++--
 src/util/virdnsmasq.h |  1 +
 .../networkxml2confdata/leasetime-hours.conf  | 16 +
 tests/networkxml2confdata/leasetime-hours.xml | 12 
 .../leasetime-infinite.conf   | 16 +
 .../leasetime-infinite.xml| 12 
 .../leasetime-minutes.conf| 16 +
 .../networkxml2confdata/leasetime-minutes.xml | 12 
 .../leasetime-seconds.conf| 16 +
 .../networkxml2confdata/leasetime-seconds.xml | 12 
 tests/networkxml2conftest.c   |  4 ++
 tests/networkxml2xmlin/leasetime-hours.xml| 12 
 tests/networkxml2xmlin/leasetime-infinite.xml | 12 
 tests/networkxml2xmlin/leasetime-minutes.xml  | 12 
 tests/networkxml2xmlin/leasetime-seconds.xml  | 12 
 tests/networkxml2xmlout/leasetime-hours.xml   | 14 +
 .../networkxml2xmlout/leasetime-infinite.xml  | 14 +
 tests/networkxml2xmlout/leasetime-minutes.xml | 14 +
 tests/networkxml2xmlout/leasetime-seconds.xml | 14 +
 tests/networkxml2xmltest.c|  4 ++
 26 files changed, 376 insertions(+), 24 deletions(-)
 create mode 100644 tests/networkxml2confdata/leasetime-hours.conf
 create mode 100644 tests/networkxml2confdata/leasetime-hours.xml
 create mode 100644 tests/networkxml2confdata/leasetime-infinite.conf
 create mode 100644 tests/networkxml2confdata/leasetime-infinite.xml
 create mode 100644 tests/networkxml2confdata/leasetime-minutes.conf
 create mode 100644 tests/networkxml2confdata/leasetime-minutes.xml
 create mode 100644 tests/networkxml2confdata/leasetime-seconds.conf
 create mode 100644 tests/networkxml2confdata/leasetime-seconds.xml
 create mode 100644 tests/networkxml2xmlin/leasetime-hours.xml
 create mode 100644 tests/networkxml2xmlin/leasetime-infinite.xml
 create mode 100644 tests/networkxml2xmlin/leasetime-minutes.xml
 create mode 100644 tests/networkxml2xmlin/leasetime-seconds.xml
 create mode 100644 tests/networkxml2xmlout/leasetime-hours.xml
 create mode 100644 tests/networkxml2xmlout/leasetime-infinite.xml
 create mode 100644 tests/networkxml2xmlout/leasetime-minutes.xml
 create mode 100644 tests/networkxml2xmlout/leasetime-seconds.xml

-- 
2.24.1

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

[Andrea Bolognani]

On Wed, 2020-04-15 at 13:52 +0100, Daniel P. Berrangé wrote:
> On Tue, Apr 14, 2020 at 12:42:26PM +0200, Andrea Bolognani wrote:
> > 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 don't see that as a big deal really. The older platforms are
> generally more interesting, as they're more likely to be lacking
> features we depend on. Being able to validate multiple XCode
> versions is interesting as they're different compiler toolchain
> versions and so likely to spit out different warnings.

I think coverage of older platforms is useful if we still target
them, and a nuisance otherwise.

As for older toolchains, we obviously need that for platforms where
the toolchain can't be upgraded separately from the OS, but that's
not the case on macOS, and given that Xcode updates are free of
charge and introduce significant new features, I doubt many
developers stick with older releases for longer than they need to.

> > 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.
> 
> Cirrus only seems to offer latest version of XCode, so that's not
> especially appealing IMHO.

That version of Xcode is running on the latest macOS version, the
one we actually target. That's a big plus in my book.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH 2/2] gitlab: Enable improved ccache usage

[Andrea Bolognani]

On Wed, 2020-04-15 at 13:38 +0100, Daniel P. Berrangé wrote:
> On Mon, Mar 30, 2020 at 07:31:03PM +0200, Andrea Bolognani wrote:
> > Since we're touching this anyway, we make a few more changes:
> > $CCACHE_DIR is no longer created manually, because ccache will
> > take care of creating it for us if it doesn't already exist; the
> 
> I could have sworn I had a build failure when I didn't have the
> mkdir present when originally doing this recipe. It might have
> been from gitlab itself, rather than ccache, eg when unpacking
> the cache archive to the non-existant dir.  But assuming the
> CI pipeline works for you...

I tried again to make sure, because I had not considered the GitLab
angle. If you look at any of the build on

  https://gitlab.com/abologna/libvirt/pipelines/136291992

you'll see that the cache gets restored successfully.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[Michal Privoznik]

On 4/15/20 2:09 PM, Pavel Mores wrote:

On Tue, Apr 14, 2020 at 11:32:08AM +0200, Michal Privoznik wrote:

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"));


Is the revocation code correctly placed though?  I mean, it seems to follow
from the patch that we need to revoke as soon as
qemuDomainStorageSourceChainAccessAllow() succeeds.  However, the revocation
call is conditioned by there being 'data' or 'crdata' (among other things).

What happens if qemuDomainStorageSourceChainAccessAllow() succeeds but the
first subsequent attempt to retrieve 'data' or 'crdata' (whichever comes first)
fails?  These failures are handled by 'goto endjob' which reaches the clean-up
section apparently with both 'data' and 'crdata' still being NULL but
'need_revoke' true. If the above is correct, that would mean the 'if
(need_revoke)' code is never reached and no revocation performed.

What am I missing?


Nothing, my patch is broken. I will send v2.

Michal

[libvirt PATCH] docs: kbase: add a page about debugging

[Ján Tomko]

Migrate the following wiki articles:
https://wiki.libvirt.org/page/DebugLogs
https://wiki.libvirt.org/page/Debugging

With the syntax changed to rST and rewrapped.

Signed-off-by: Ján Tomko 
---
I might've changed an article or two, too.
Or rather: the article 'the'.

 docs/kbase.html.in   |   3 +
 docs/kbase/debugging.rst | 303 +++
 2 files changed, 306 insertions(+)
 create mode 100644 docs/kbase/debugging.rst

diff --git a/docs/kbase.html.in b/docs/kbase.html.in
index c586e0f676..1a50428805 100644
--- a/docs/kbase.html.in
+++ b/docs/kbase.html.in
@@ -36,6 +36,9 @@
 
 Virtio-FS
 Share a filesystem between the guest and the host
+
+Debugging libvirt
+How to capture debug logs and use a debugger on libvirt
   
 
 
diff --git a/docs/kbase/debugging.rst b/docs/kbase/debugging.rst
new file mode 100644
index 00..a9bc02aa55
--- /dev/null
+++ b/docs/kbase/debugging.rst
@@ -0,0 +1,303 @@
+=
+Debugging libvirt
+=
+
+.. contents::
+
+
+If you `report a bug `_ against libvirt, in most
+cases you will be asked to attach debug logs. These are bare text files which
+tracks transition between different states of libvirtd, what it has tried to
+achieve, etc. Because of client -- server schema used in libvirt, the logs can
+be either client or server too. Usually, it's server side that matters as
+nearly all interesting work takes place there. Moreover, libvirt catches stderr
+of all running domains. These can be useful as well.
+
+
+How to turn on debug logs for libvirt
+=
+
+Persistent setting
+--
+
+The daemon configuration files location is dependent on the `connection URI
+`_. For ``qemu:///system``:
+
+* open ``/etc/libvirt/libvirtd.conf`` in your favourite editor
+* find & replace, or set these variables::
+
+   # LEGACY SETTINGS PRIOR LIBVIRT 4.4.0 SEE BELOW! #
+   log_level = 1
+   log_filters="1:qemu 3:remote 4:event 3:util.json 3:rpc"
+   log_outputs="1:file:/var/log/libvirt/libvirtd.log"
+
+
+   # PREFERRED SETTINGS AFTER LIBVIRT 4.4.0 #
+   log_filters="3:remote 4:event 3:util.json 3:rpc 1:*"
+   log_outputs="1:file:/var/log/libvirt/libvirtd.log"
+
+* save and exit
+* restart libvirtd service::
+
+   systemctl restart libvirtd.service
+
+In the config variables above, we have set logging level to 1 (debug level),
+set some filters (to filter out noise), e.g. from rpc only warnings (=level 3)
+and above will be reported. The logs are saved into
+``/var/log/libvirt/libvirtd.log``. Since libvirt *4.4.0* log filters
+support shell globbing, therefore the usage of ``log_level`` is considered
+deprecated in favour of pure usage of ``log_filters``.
+
+In case you want to get the client logs, you need to set this environment 
variable::
+
+ export LIBVIRT_LOG_OUTPUTS="1:file:/tmp/libvirt_client.log"
+
+However, when you are using the session mode ``qemu:///session`` or you run the
+``libvirtd`` as unprivileged user you will find configuration file under
+``$XDG_CONFIG_HOME/libvirt/libvirt.conf``.
+
+
+Runtime setting
+---
+
+Debugging anomalies can be very painful, especially when trying to reproduce it
+after the daemon restarts, since the new session can make the anomaly
+"disappear". Therefore, it's possible to enable the debug logs during runtime
+using libvirt administration API. To use it conveniently, there's a 
``virt-admin``
+client provided by the ``libvirt-admin`` package. Use the package manager 
provided
+by your distribution to install this package. Once you have it installed, run
+the following as root to see the set of log filters currently being active::
+
+   # virt-admin daemon-log-filters
+   Logging filters: 3:remote 4:util.json 4:rpc
+
+In order to change this set, run the same command as root, this time with your 
own set of filters::
+
+   ## LEGACY APPROACH ENUMERATING ALL THE DESIRED MODULES ##
+   # virt-admin daemon-log-filters "1:util 1:libvirt 1:storage 1:network 
1:nodedev 1:qemu"
+
+   ## CURRENT APPROACH USING SHELL GLOBBING ##
+   # virt-admin daemon-log-filters "3:remote 4:util.json 4:rpc 1:*"
+
+Analogically, the same procedure can be performed with log outputs::
+
+   # virt-admin daemon-log-outputs
+   Logging outputs: 3:syslog:libvirtd
+   # virt-admin daemon-log-outputs "1:file:/var/log/libvirt/libvirtd.log"
+
+NOTE: It's always good practice to return the settings to the original state
+once you're finished debugging, just remember to save the original sets of
+filters and outputs and restore them at the end the same way as described
+above.
+
+
+Removing filters and outputs
+
+
+It's also possible to remove all the filters and produce an enormous log file,
+but it is not recommended since some of libvirt's modules can produce a large
+amount of noise. However, should you really want to do this, you can specif

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

[Pavel Mores]

On Wed, Apr 15, 2020 at 03:58:55PM +0100, Daniel P. Berrangé wrote:
> On Wed, Apr 15, 2020 at 04:11:30PM +0200, Pavel Mores wrote:
> > On Wed, Apr 15, 2020 at 01:55:34PM +0100, Daniel P. Berrangé wrote:
> > > On Tue, Apr 14, 2020 at 05:24:50PM +0200, Pavel Mores wrote:
> > > > 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.
> > > 
> > > Honestly, none of the reasons mutex init can fail are especially
> > > interesting to callers. There's essentially nothing useful callers
> > > can do when a mutex init fails, as its symptomatic of much bigger
> > > problems. Thus I think abort'ing is a reasonable approach. Likewise
> > > for lock/unlock.
> > 
> > Yes, agreed.  At the risk of going off-topic, what makes me wonder is that
> > phtread_mutex_init() POSIX manual page lists also EPERM among the reasons 
> > why
> > the call might fail.  I've never heard of that, let alone encountered it, 
> > and
> > it might not actually be implemented by OS's in practice.  However if EPERM 
> > can
> > happen, then I guess that could be worth reporting to the user.
> 
> Any OS that is crazy enough to report EPERM for initializing mutex is not
> an OS I wish to support :-)

That seems fair enough. :-)

pvl

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

[Daniel P . Berrangé]

On Wed, Apr 15, 2020 at 04:11:30PM +0200, Pavel Mores wrote:
> On Wed, Apr 15, 2020 at 01:55:34PM +0100, Daniel P. Berrangé wrote:
> > On Tue, Apr 14, 2020 at 05:24:50PM +0200, Pavel Mores wrote:
> > > 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.
> > 
> > Honestly, none of the reasons mutex init can fail are especially
> > interesting to callers. There's essentially nothing useful callers
> > can do when a mutex init fails, as its symptomatic of much bigger
> > problems. Thus I think abort'ing is a reasonable approach. Likewise
> > for lock/unlock.
> 
> Yes, agreed.  At the risk of going off-topic, what makes me wonder is that
> phtread_mutex_init() POSIX manual page lists also EPERM among the reasons why
> the call might fail.  I've never heard of that, let alone encountered it, and
> it might not actually be implemented by OS's in practice.  However if EPERM 
> can
> happen, then I guess that could be worth reporting to the user.

Any OS that is crazy enough to report EPERM for initializing mutex is not
an OS I wish to support :-)

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: Wiki page audit - possible page deleteion

[Daniel P . Berrangé]

On Wed, Apr 15, 2020 at 04:08:05PM +0200, Ján Tomko wrote:
> On a Wednesday in 2020, Daniel P. Berrangé wrote:
> > Ideally I would like to decomission the current wiki.libvirt.org
> > site. It is based on mediawiki running on openshift and has myself
> > as a single point of failure.
> > 
> > GitLab provides a wiki, but I don't think we need to use that
> > either, as I think that it is desirable to bring the content into
> > our main website.
> > 
> > By putting an "edit this page" link on each page of our website,
> > users can quickly see what source to change. There is still the
> > burden of submitting a merge request & having review feedback,
> > but I think we could mitigate this by having the reviewer
> > actually make the changes they want directly, avoiding the
> > tedious back & forth updates in easy cases. GitLab allows
> > this if the person opening the merge request selects the
> > option to allow maintainers to edit code.
> > 
> > In any case, to remove wiki.libvirt.org we need to do something
> > with its current content. The majority of content was first
> > created 5-10 years ago, only a handful of pages get frequent
> > edits right now:
> > 
> >   
> > https://wiki.libvirt.org/index.php?title=Special:RecentChanges&limit=500&days=90
> > 
> > 
> > Looking at the pages I see some key groups, so I'll talk
> > about them separately...
> > 
> 
> Is this a list of the recently changed pages? Or all of them?

This was all the pages shown under

https://wiki.libvirt.org/page/Special:AllPages

but I've already deleted a bunch of the  irrelevant cruft mentioned
in this original mail

> > Various misc pages that I've not spent too much time looking at each
> > page, but as a rough approx I'd keep the following list, pulling it
> > into the main website
> > 
> >BiteSizedTasks
> 
> >DebugLogs
> >Debugging
> 
> I'll put these two into a kbase article.

BTW, we already have this page:

   https://libvirt.org/logging.html

so at least some content from the wiki might fold into that page.



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] docs: add zpci information to pci-addresses.rst

[Cornelia Huck]

On Wed, 15 Apr 2020 16:23:46 +0200
Boris Fiuczynski  wrote:

> On 4/15/20 3:42 PM, Cornelia Huck wrote:
> > On Tue, 14 Apr 2020 23:06:47 +0200
> > Boris Fiuczynski  wrote:
> >   
> >> On 4/15/20 12:51 PM, Cornelia Huck wrote:  

> >>> +In the simplest case, the following XML snippet
> >>> +
> >>> +::
> >>> +
> >>> +  
> >>> +  
> >>> +
> >>> +
> >>> + >>> function='0x0'>
> >>> +  
> >>> +
> >>> +  
> >>> +  
> >>> +
> >>> +
> >>> +
> >>> + >>> function='0x0'>
> >>> +  
> >>> +
> >>> +  
> >>> +
> >>> +will result in the following in a Linux guest::
> >>> +
> >>> +  0001:00:00.0 Ethernet controller: Red Hat, Inc. Virtio network device
> >>> +
> >>> +Note that the PCI bridge is not visible in the guest; s390x always has a 
> >>> flat
> >>> +topology.
> >>> +
> >>> +Neither are any changes in the PCI address visible in the guest; 
> >>> replacing
> >>> +the PCI address for the virtio-net device with
> >>> +
> >>> +::
> >>> +
> >>> +   >>> function='0x3'>
> >>> +
> >>> +will result in the exactly same view in the guest, as the addresses there
> >>> +are generated from the information provided via the ``zpci`` element (in
> >>> +fact, from the ``uid``).
> >>> +  
> >> This explains that the uid is used by the guest to define the pci domain
> >> of the guest device.
> >> How about an addition for the fid? Something like:
> >>
> >> The function id 'fid' defines the slot address of the pci card in the
> >> guest. It can be observed in the guest at /sys/bus/pci/slots. In the
> >> example given above the card would be at /sys/bus/pci/slots/.  
> > 
> > Hm, is it intentional that this does not show up in the actual pci
> > address? But maybe I'm confused.  
> Are you referring to the slot of the pci address?

Yes.

> 
> If yes, the pci slot does not provided the required address space for 
> the function id. Also we once said that the pci bus in qemu would be 
> like the pci bus of the CEC and the firmware does provide a a function 
> id for every pci function. The zpci fid does one allow to do the same.

Ok, now I dimly remember something like that. Still confusing, but
makes sense.

> 
> >   
> >>
> >>  
> >>> +Therefore, replacing the virtio-net device definition with the following 
> >>> XML
> >>> +snippet
> >>> +
> >>> +::
> >>> +
> >>> +  
> >>> +
> >>> +
> >>> +
> >>> + >>> function='0x3'>
> >>> +  
> >>> +
> >>> +  
> >>> +
> >>> +will yield the following result in a Linux guest::
> >>> +
> >>> +  0007:00:00.0 Ethernet controller: Red Hat, Inc. Virtio network device
> >>>  
> >> and the card would be at /sys/bus/pci/slots/0003.  
> > 
> > Do you want to explain the fid/slot stuff in a patch on top? Despite
> > your attempts at time travel, this patch has already been pushed :)   
> It seems to be a very pushy time... :( no matter how many breaks are 
> produced... ;)  11 minutes from patch production time to commit time. 
> Why even send it for review? :D

Well, I don't have commit rights O:-)

> Once I understand you confusion above I will provide a patch...

I'd say just go ahead.

Re: [PATCH 03/43] conf: nwfilter_ipaddrmap: convert virMutex to GMutex

[Laine Stump]

On 4/15/20 8:43 AM, Pavel Mores wrote:

On Sat, Apr 11, 2020 at 08:15:15PM +0200, Rafael Fonseca wrote:

On Fri, Apr 10, 2020 at 7:06 PM Laine Stump  wrote:

Beyond that, why not just use the G_*() macros for *all* locks in
libvirt instead of only using them in cases of static locks? It seems
strange to use the convenience macros in some cases and the raw
functions in others. (I'm looking at this with 0 experience using the
Glib locks, so maybe there's something basic I'm just not aware of -
maybe something necessary is missing from the G_LOCK() macros?).


Okay, I already see that the G_LOCK macros don't cover everything - no
recursive mutexes and no RW mutexes for example. Too bad - it would be
good to be consistent.

Yes, that's one issue. Another is: how do you use those macros with
locks inside structs? You can't do `G_LOCK(obj->parent.lock)` because
it'll result in `g_mutex_lock(&g__obj->parent.lock_lock)` which is
wrong. You'd have to use the raw function
`g_mutex_lock(obj->parent.LOCK_NAME(lock))` anyway, which imho, is
even worse than `g_mutex_lock(&obj->parent.lock)`. The same issue
happens when using mutexes with conditions: `g_cond_wait(cond,
obj->parent.LOCK_NAME(lock))
` instead of just `g_cond_wait(cond, obj->parent.lock)`. So they work
better for statically-defined locks

I don't mind doing whichever you guys prefer, just let me know.

Looking at the source code, the name mangling is pretty much all that the
G_LOCK_DEFINE* macros do.  So - besides some logging - the only advantage to
using them is that you don't have to mangle the lock names manually and can use
names of existing variables as the macros' arguments.

Considering the above, I'd say either use the macros and don't mangle the lock
names in their arguments manually, or don't use the macros.  If consistent
style is a priority I'd lean towards raw functions



I agree with this - consistency is king :-)



  - unlike the macros, they
can be used everywhere, and having to mangle the lock names by hand doesn't
seem a huge burden to me.  We do loose the logging that the macros do but in my
experience, mutex logging often doesn't turn out as useful in practice as it
might first appear...

pvl

Re: [PATCH] docs: add zpci information to pci-addresses.rst

[Boris Fiuczynski]

On 4/15/20 3:42 PM, Cornelia Huck wrote:

On Tue, 14 Apr 2020 23:06:47 +0200
Boris Fiuczynski  wrote:


On 4/15/20 12:51 PM, Cornelia Huck wrote:



+zPCI addresses
+==
+
+For s390x machines, PCI addresses are handled yet differently. No
+topology information is relayed in the PCI addresses; instead, the
+``fid`` and ``uid`` elements of the ``zpci`` device convey information.

Should it be mentioned that qemu uses the pci address internally to plug
the device into its pci bus since the pci address as far as I know must
still be properly provided or generated.


Not sure how much is autogenerated; however, as this document talks
about how pci addresses in the xml translate to pci addresses in the
guest, I think we can simply ignore it?
ok. I just wanted to explain why the pci address part still exists and 
why libvirt also will autogenerate all other required bridges.








+In the simplest case, the following XML snippet
+
+::
+
+  
+  
+
+
+
+  
+
+  
+  
+
+
+
+
+  
+
+  
+
+will result in the following in a Linux guest::
+
+  0001:00:00.0 Ethernet controller: Red Hat, Inc. Virtio network device
+
+Note that the PCI bridge is not visible in the guest; s390x always has a flat
+topology.
+
+Neither are any changes in the PCI address visible in the guest; replacing
+the PCI address for the virtio-net device with
+
+::
+
+  
+
+will result in the exactly same view in the guest, as the addresses there
+are generated from the information provided via the ``zpci`` element (in
+fact, from the ``uid``).
+

This explains that the uid is used by the guest to define the pci domain
of the guest device.
How about an addition for the fid? Something like:

The function id 'fid' defines the slot address of the pci card in the
guest. It can be observed in the guest at /sys/bus/pci/slots. In the
example given above the card would be at /sys/bus/pci/slots/.


Hm, is it intentional that this does not show up in the actual pci
address? But maybe I'm confused.

Are you referring to the slot of the pci address?

If yes, the pci slot does not provided the required address space for 
the function id. Also we once said that the pci bus in qemu would be 
like the pci bus of the CEC and the firmware does provide a a function 
id for every pci function. The zpci fid does one allow to do the same.








+Therefore, replacing the virtio-net device definition with the following XML
+snippet
+
+::
+
+  
+
+
+
+
+  
+
+  
+
+will yield the following result in a Linux guest::
+
+  0007:00:00.0 Ethernet controller: Red Hat, Inc. Virtio network device
   

and the card would be at /sys/bus/pci/slots/0003.


Do you want to explain the fid/slot stuff in a patch on top? Despite
your attempts at time travel, this patch has already been pushed :) 
It seems to be a very pushy time... :( no matter how many breaks are 
produced... ;)  11 minutes from patch production time to commit time. 
Why even send it for review? :D

Once I understand you confusion above I will provide a patch...


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

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

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

[Pavel Mores]

On Wed, Apr 15, 2020 at 01:55:34PM +0100, Daniel P. Berrangé wrote:
> On Tue, Apr 14, 2020 at 05:24:50PM +0200, Pavel Mores wrote:
> > 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.
> 
> Honestly, none of the reasons mutex init can fail are especially
> interesting to callers. There's essentially nothing useful callers
> can do when a mutex init fails, as its symptomatic of much bigger
> problems. Thus I think abort'ing is a reasonable approach. Likewise
> for lock/unlock.

Yes, agreed.  At the risk of going off-topic, what makes me wonder is that
phtread_mutex_init() POSIX manual page lists also EPERM among the reasons why
the call might fail.  I've never heard of that, let alone encountered it, and
it might not actually be implemented by OS's in practice.  However if EPERM can
happen, then I guess that could be worth reporting to the user.

Well it's not like we need to solve this problem on this list anyway...

pvl

Re: Wiki page audit - possible page deleteion

[Ján Tomko]

On a Wednesday in 2020, Daniel P. Berrangé wrote:

Ideally I would like to decomission the current wiki.libvirt.org
site. It is based on mediawiki running on openshift and has myself
as a single point of failure.

GitLab provides a wiki, but I don't think we need to use that
either, as I think that it is desirable to bring the content into
our main website.

By putting an "edit this page" link on each page of our website,
users can quickly see what source to change. There is still the
burden of submitting a merge request & having review feedback,
but I think we could mitigate this by having the reviewer
actually make the changes they want directly, avoiding the
tedious back & forth updates in easy cases. GitLab allows
this if the person opening the merge request selects the
option to allow maintainers to edit code.

In any case, to remove wiki.libvirt.org we need to do something
with its current content. The majority of content was first
created 5-10 years ago, only a handful of pages get frequent
edits right now:

  
https://wiki.libvirt.org/index.php?title=Special:RecentChanges&limit=500&days=90


Looking at the pages I see some key groups, so I'll talk
about them separately...



Is this a list of the recently changed pages? Or all of them?

[...]


Various misc pages that I've not spent too much time looking at each
page, but as a rough approx I'd keep the following list, pulling it
into the main website

   BiteSizedTasks



   DebugLogs
   Debugging


I'll put these two into a kbase article.


   FAQ
   Libvirt-snmp
   Libvirtd and dnsmasq
   Live-disk-backup-with-active-blockcommit
   Live-merge-an-entire-disk-image-chain-including-current-active-disk
   Maintenance Releases
   NPIV in libvirt
   Net.bridge-nf-call and sysctl.conf
   Net.bridge.bridge-nf-call and sysctl.conf
   Networking
   Qemu guest agent
   SSHPolicyKitSetup
   VM lifecycle
   Vhost-scsi target


And probably delete the following (though some might want
preserving upon closer inspection)

   AprilFools'


This one does not belong to a git repo IMO. I'll just keep it to myself
:)

Jano


signature.asc
Description: PGP signature

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

[Pavel Mores]

On Wed, Apr 15, 2020 at 01:59:46PM +0100, Daniel P. Berrangé wrote:
> On Wed, Apr 15, 2020 at 02:15:35PM +0200, Pavel Mores wrote:
> > On Tue, Apr 14, 2020 at 07:05:03PM +0200, Rafael Fonseca wrote:
> > > 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`?
> > 
> > Yes - I mean, I'm not familiar enough with this to be sure off-hand that 
> > just
> > doing a literal find & replace would work with no undesired side-effects, 
> > but
> > conceptually yes, that's the idea.
> > 
> > That's just a thought though - taking that approach would have broken the
> > refactor into two more manageable & testable chunks but seeing as you've 
> > done
> > the hard work already, there's no need to rework the series just because of 
> > me.
> > :-)
> 
> Replacing the virMutex calls with GMutex APis in all callers is the
> desirable approach.  The goal of using GLib APIs is to remove any
> libvirt specfic APIs which duplicate GLib.  Thus re-writing virMutex
> APIs impls to call GMutex is just delaying the desired end state where
> virMutex ceases to exist.

Sure, I agree with the goal, no question about that.  I was just thinking about
getting there in two steps (with no delay between them implied) which might
have been at least easier to review.  Also bisecting would be easier that way
if we later suspect that there might have been an unexpected change of
behaviour after all, possibly related to the migration to glib sync primitives.

Anyway, the work has been pretty much done already so the point seems moot.

Thanks,

pvl

Re: [PATCH] docs: add zpci information to pci-addresses.rst

[Cornelia Huck]

On Tue, 14 Apr 2020 23:06:47 +0200
Boris Fiuczynski  wrote:

> On 4/15/20 12:51 PM, Cornelia Huck wrote:

> > +zPCI addresses
> > +==
> > +
> > +For s390x machines, PCI addresses are handled yet differently. No
> > +topology information is relayed in the PCI addresses; instead, the
> > +``fid`` and ``uid`` elements of the ``zpci`` device convey information.  
> Should it be mentioned that qemu uses the pci address internally to plug 
> the device into its pci bus since the pci address as far as I know must 
> still be properly provided or generated.

Not sure how much is autogenerated; however, as this document talks
about how pci addresses in the xml translate to pci addresses in the
guest, I think we can simply ignore it?

> 
> 
> > +In the simplest case, the following XML snippet
> > +
> > +::
> > +
> > +  
> > +  
> > +
> > +
> > + > function='0x0'>
> > +  
> > +
> > +  
> > +  
> > +
> > +
> > +
> > + > function='0x0'>
> > +  
> > +
> > +  
> > +
> > +will result in the following in a Linux guest::
> > +
> > +  0001:00:00.0 Ethernet controller: Red Hat, Inc. Virtio network device
> > +
> > +Note that the PCI bridge is not visible in the guest; s390x always has a 
> > flat
> > +topology.
> > +
> > +Neither are any changes in the PCI address visible in the guest; replacing
> > +the PCI address for the virtio-net device with
> > +
> > +::
> > +
> > +   > function='0x3'>
> > +
> > +will result in the exactly same view in the guest, as the addresses there
> > +are generated from the information provided via the ``zpci`` element (in
> > +fact, from the ``uid``).
> > +  
> This explains that the uid is used by the guest to define the pci domain 
> of the guest device.
> How about an addition for the fid? Something like:
> 
> The function id 'fid' defines the slot address of the pci card in the 
> guest. It can be observed in the guest at /sys/bus/pci/slots. In the 
> example given above the card would be at /sys/bus/pci/slots/.

Hm, is it intentional that this does not show up in the actual pci
address? But maybe I'm confused.

> 
> 
> > +Therefore, replacing the virtio-net device definition with the following 
> > XML
> > +snippet
> > +
> > +::
> > +
> > +  
> > +
> > +
> > +
> > + > function='0x3'>
> > +  
> > +
> > +  
> > +
> > +will yield the following result in a Linux guest::
> > +
> > +  0007:00:00.0 Ethernet controller: Red Hat, Inc. Virtio network device
> >   
> and the card would be at /sys/bus/pci/slots/0003.

Do you want to explain the fid/slot stuff in a patch on top? Despite
your attempts at time travel, this patch has already been pushed :)

Re: [RFC] Adding docker driver to libvirt

[Daniel P . Berrangé]

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.

The container based drivers in libvirt have been a bit of a square-peg /
round-hole thing. Given that we have a couple of them already (LXC,
OpenVZ, VZ), I wouldn't say no to adding a docker one too. The only
real issue is having people willing to do the work to implement it and
then maintain it thereafter.

Describing the scope of the desired work is probably useful. With docker,
a big part is in the image download/listing/upload and build process.
The container lifecycle is only a small part of the API coverage. The
image parts have no mapping in libvirt, and I'm not sure whether we
should to expand libvirt scope to that too.

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 02/43] bhyve: convert virMutex to GMutex

[Daniel P . Berrangé]

On Wed, Apr 15, 2020 at 02:15:35PM +0200, Pavel Mores wrote:
> On Tue, Apr 14, 2020 at 07:05:03PM +0200, Rafael Fonseca wrote:
> > 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`?
> 
> Yes - I mean, I'm not familiar enough with this to be sure off-hand that just
> doing a literal find & replace would work with no undesired side-effects, but
> conceptually yes, that's the idea.
> 
> That's just a thought though - taking that approach would have broken the
> refactor into two more manageable & testable chunks but seeing as you've done
> the hard work already, there's no need to rework the series just because of 
> me.
> :-)

Replacing the virMutex calls with GMutex APis in all callers is the
desirable approach.  The goal of using GLib APIs is to remove any
libvirt specfic APIs which duplicate GLib.  Thus re-writing virMutex
APIs impls to call GMutex is just delaying the desired end state where
virMutex ceases to exist.

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 01/43] admin: convert virMutex to GMutex

[Daniel P . Berrangé]

On Tue, Apr 14, 2020 at 05:24:50PM +0200, Pavel Mores wrote:
> 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.

Honestly, none of the reasons mutex init can fail are especially
interesting to callers. There's essentially nothing useful callers
can do when a mutex init fails, as its symptomatic of much bigger
problems. Thus I think abort'ing is a reasonable approach. Likewise
for lock/unlock.


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

[Daniel P . Berrangé]

On Tue, Apr 14, 2020 at 12:42:26PM +0200, Andrea Bolognani wrote:
> 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 don't see that as a big deal really. The older platforms are
generally more interesting, as they're more likely to be lacking
features we depend on. Being able to validate multiple XCode
versions is interesting as they're different compiler toolchain
versions and so likely to spit out different warnings.

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

Cirrus only seems to offer latest version of XCode, so that's not
especially appealing IMHO.

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 03/43] conf: nwfilter_ipaddrmap: convert virMutex to GMutex

[Pavel Mores]

On Sat, Apr 11, 2020 at 08:15:15PM +0200, Rafael Fonseca wrote:
> On Fri, Apr 10, 2020 at 7:06 PM Laine Stump  wrote:
> > > Beyond that, why not just use the G_*() macros for *all* locks in
> > > libvirt instead of only using them in cases of static locks? It seems
> > > strange to use the convenience macros in some cases and the raw
> > > functions in others. (I'm looking at this with 0 experience using the
> > > Glib locks, so maybe there's something basic I'm just not aware of -
> > > maybe something necessary is missing from the G_LOCK() macros?).
> >
> >
> > Okay, I already see that the G_LOCK macros don't cover everything - no
> > recursive mutexes and no RW mutexes for example. Too bad - it would be
> > good to be consistent.
> 
> Yes, that's one issue. Another is: how do you use those macros with
> locks inside structs? You can't do `G_LOCK(obj->parent.lock)` because
> it'll result in `g_mutex_lock(&g__obj->parent.lock_lock)` which is
> wrong. You'd have to use the raw function
> `g_mutex_lock(obj->parent.LOCK_NAME(lock))` anyway, which imho, is
> even worse than `g_mutex_lock(&obj->parent.lock)`. The same issue
> happens when using mutexes with conditions: `g_cond_wait(cond,
> obj->parent.LOCK_NAME(lock))
> ` instead of just `g_cond_wait(cond, obj->parent.lock)`. So they work
> better for statically-defined locks
> 
> I don't mind doing whichever you guys prefer, just let me know.

Looking at the source code, the name mangling is pretty much all that the
G_LOCK_DEFINE* macros do.  So - besides some logging - the only advantage to
using them is that you don't have to mangle the lock names manually and can use
names of existing variables as the macros' arguments.

Considering the above, I'd say either use the macros and don't mangle the lock
names in their arguments manually, or don't use the macros.  If consistent
style is a priority I'd lean towards raw functions - unlike the macros, they
can be used everywhere, and having to mangle the lock names by hand doesn't
seem a huge burden to me.  We do loose the logging that the macros do but in my
experience, mutex logging often doesn't turn out as useful in practice as it
might first appear...

pvl

Re: [libvirt PATCH 2/2] gitlab: Enable improved ccache usage

[Daniel P . Berrangé]

On Mon, Mar 30, 2020 at 07:31:03PM +0200, Andrea Bolognani wrote:
> Setting CC="ccache cc" works in most cases, but sometimes it will
> break the build: in particular, we have experienced issues in the
> past with that approach when using cgo to build our Go bindings.
> 
> A more robust approach is to have a directory containing symlinks
> from the compiler name to the ccache binary: in that case, ccache
> itself will invoke the compiler, and the build system will be none
> the wiser.
> 
> Since libvirt-jenkins-ci commit 2563aebb6c5c, container images
> contain a suitable symlink directory, so all that's needed to
> enable the new approach is to add this directory to $PATH.
> 
> Since we're touching this anyway, we make a few more changes:
> $CCACHE_DIR is no longer created manually, because ccache will
> take care of creating it for us if it doesn't already exist; the

I could have sworn I had a build failure when I didn't have the
mkdir present when originally doing this recipe. It might have
been from gitlab itself, rather than ccache, eg when unpacking
the cache archive to the non-existant dir.  But assuming the
CI pipeline works for you...

> ccache setup is moved out of the job template and into
> script_variables, removing unnecessary duplication; a limit is
> set on the size of the cache (500 MB, which is twice the amount
> used by a fresh build on my Fedora 31 machine).
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  .gitlab-ci.yml | 12 
>  1 file changed, 4 insertions(+), 8 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 1/2] gitlab: Don't define $MAKE

[Daniel P . Berrangé]

On Mon, Mar 30, 2020 at 07:31:02PM +0200, Andrea Bolognani wrote:
> Since libvirt-jenkins-ci commit 27cfddee8835, paths to build tools
> such as ninja and make are exported in the container's environment
> and can be used directly.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  .gitlab-ci.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: [PATCH] docs: add zpci information to pci-addresses.rst

[Boris Fiuczynski]

Sorry about the time travel.
I will try to give up that super power... :(

On 4/14/20 11:06 PM, Boris Fiuczynski wrote:

On 4/15/20 12:51 PM, Cornelia Huck wrote:

Add some information on how pci address work on s390x.

Signed-off-by: Cornelia Huck 

Conny, thanks for catching this "wacky case"... :)
I took the liberty to remove "completely" because there needs to be room 
for pci multifunction support... :D



---
  docs/pci-addresses.rst | 63 ++
  1 file changed, 63 insertions(+)

diff --git a/docs/pci-addresses.rst b/docs/pci-addresses.rst
index 923783a151b0..9e241a24fcfb 100644
--- a/docs/pci-addresses.rst
+++ b/docs/pci-addresses.rst
@@ -184,3 +184,66 @@ guest OS rather than as ``0001:08:00.1``, which 
is the address of the

  device on the host.
  Of course, all the rules and behaviors described above still apply.
+
+zPCI addresses
+==
+
+For s390x machines, PCI addresses are handled yet differently. No
+topology information is relayed in the PCI addresses; instead, the
+``fid`` and ``uid`` elements of the ``zpci`` device convey information.
Should it be mentioned that qemu uses the pci address internally to plug 
the device into its pci bus since the pci address as far as I know must 
still be properly provided or generated.




+In the simplest case, the following XML snippet
+
+::
+
+  
+  
+    
+    
+    function='0x0'>

+  
+    
+  
+  
+    
+    
+    
+    function='0x0'>

+  
+    
+  
+
+will result in the following in a Linux guest::
+
+  0001:00:00.0 Ethernet controller: Red Hat, Inc. Virtio network device
+
+Note that the PCI bridge is not visible in the guest; s390x always 
has a flat

+topology.
+
+Neither are any changes in the PCI address visible in the guest; 
replacing

+the PCI address for the virtio-net device with
+
+::
+
+  function='0x3'>

+
+will result in the exactly same view in the guest, as the addresses 
there

+are generated from the information provided via the ``zpci`` element (in
+fact, from the ``uid``).
+
This explains that the uid is used by the guest to define the pci domain 
of the guest device.

How about an addition for the fid? Something like:

The function id 'fid' defines the slot address of the pci card in the 
guest. It can be observed in the guest at /sys/bus/pci/slots. In the 
example given above the card would be at /sys/bus/pci/slots/.



+Therefore, replacing the virtio-net device definition with the 
following XML

+snippet
+
+::
+
+  
+    
+    
+    
+    function='0x3'>

+  
+    
+  
+
+will yield the following result in a Linux guest::
+
+  0007:00:00.0 Ethernet controller: Red Hat, Inc. Virtio network device


and the card would be at /sys/bus/pci/slots/0003.





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

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

Re: [PATCH] docs: add zpci information to pci-addresses.rst

[Boris Fiuczynski]

On 4/15/20 12:51 PM, Cornelia Huck wrote:

Add some information on how pci address work on s390x.

Signed-off-by: Cornelia Huck 

Conny, thanks for catching this "wacky case"... :)
I took the liberty to remove "completely" because there needs to be room 
for pci multifunction support... :D



---
  docs/pci-addresses.rst | 63 ++
  1 file changed, 63 insertions(+)

diff --git a/docs/pci-addresses.rst b/docs/pci-addresses.rst
index 923783a151b0..9e241a24fcfb 100644
--- a/docs/pci-addresses.rst
+++ b/docs/pci-addresses.rst
@@ -184,3 +184,66 @@ guest OS rather than as ``0001:08:00.1``, which is the 
address of the
  device on the host.
  
  Of course, all the rules and behaviors described above still apply.

+
+zPCI addresses
+==
+
+For s390x machines, PCI addresses are handled yet differently. No
+topology information is relayed in the PCI addresses; instead, the
+``fid`` and ``uid`` elements of the ``zpci`` device convey information.
Should it be mentioned that qemu uses the pci address internally to plug 
the device into its pci bus since the pci address as far as I know must 
still be properly provided or generated.




+In the simplest case, the following XML snippet
+
+::
+
+  
+  
+
+
+
+  
+
+  
+  
+
+
+
+
+  
+
+  
+
+will result in the following in a Linux guest::
+
+  0001:00:00.0 Ethernet controller: Red Hat, Inc. Virtio network device
+
+Note that the PCI bridge is not visible in the guest; s390x always has a flat
+topology.
+
+Neither are any changes in the PCI address visible in the guest; replacing
+the PCI address for the virtio-net device with
+
+::
+
+  
+
+will result in the exactly same view in the guest, as the addresses there
+are generated from the information provided via the ``zpci`` element (in
+fact, from the ``uid``).
+
This explains that the uid is used by the guest to define the pci domain 
of the guest device.

How about an addition for the fid? Something like:

The function id 'fid' defines the slot address of the pci card in the 
guest. It can be observed in the guest at /sys/bus/pci/slots. In the 
example given above the card would be at /sys/bus/pci/slots/.




+Therefore, replacing the virtio-net device definition with the following XML
+snippet
+
+::
+
+  
+
+
+
+
+  
+
+  
+
+will yield the following result in a Linux guest::
+
+  0007:00:00.0 Ethernet controller: Red Hat, Inc. Virtio network device


and the card would be at /sys/bus/pci/slots/0003.


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

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

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

[Pavel Mores]

On Tue, Apr 14, 2020 at 07:05:03PM +0200, Rafael Fonseca wrote:
> 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`?

Yes - I mean, I'm not familiar enough with this to be sure off-hand that just
doing a literal find & replace would work with no undesired side-effects, but
conceptually yes, that's the idea.

That's just a thought though - taking that approach would have broken the
refactor into two more manageable & testable chunks but seeing as you've done
the hard work already, there's no need to rework the series just because of me.
:-)

pvl

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

[Pavel Mores]

On Tue, Apr 14, 2020 at 11:32:08AM +0200, Michal Privoznik wrote:
> 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"));

Is the revocation code correctly placed though?  I mean, it seems to follow
from the patch that we need to revoke as soon as
qemuDomainStorageSourceChainAccessAllow() succeeds.  However, the revocation
call is conditioned by there being 'data' or 'crdata' (among other things).

What happens if qemuDomainStorageSourceChainAccessAllow() succeeds but the
first subsequent attempt to retrieve 'data' or 'crdata' (whichever comes first)
fails?  These failures are handled by 'goto endjob' which reaches the clean-up
section apparently with both 'data' and 'crdata' still being NULL but
'need_revoke' true. If the above is correct, that would mean the 'if
(need_revoke)' code is never reached and no revocation performed.

What am I missing?

pvl

[PATCH 1/3] Improve blockpull man entry

[Sebastian Mitterle]

1. Mention usage of `--base` and `--bandwidth` and fix cmd syntax.
2. Explain valid arguments for `base`.
3. Move explanation for `--keep-relative` to end considering it
   less frequent use case because libvirt doesn't create relative
   backing chain names.
4. Add reference to documentation for relative paths in backing chains.

Signed-off-by: Sebastian Mitterle 
---
 docs/manpages/virsh.rst | 19 +--
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
index dc404ddfe8..27ecc53d56 100644
--- a/docs/manpages/virsh.rst
+++ b/docs/manpages/virsh.rst
@@ -1345,7 +1345,7 @@ blockpull
 
 .. code-block::
 
-   blockpull domain path [bandwidth] [--bytes] [base]
+   blockpull domain path { [bandwidth [--bytes] [base]] | [--bandwidth 
[--bytes]] [--base] }
   [--wait [--verbose] [--timeout seconds] [--async]]
   [--keep-relative]
 
@@ -1353,7 +1353,7 @@ Populate a disk from its backing image chain. By default, 
this command
 flattens the entire chain; but if *base* is specified, containing the
 name of one of the backing files in the chain, then that file becomes
 the new backing file and only the intermediate portion of the chain is
-pulled.  Once all requested data from the backing image chain has been
+pulled. Once all requested data from the backing image chain has been
 pulled, the disk no longer depends on that portion of the backing chain.
 
 By default, this command returns as soon as possible, and data for
@@ -1367,16 +1367,23 @@ is triggered, *--async* will return control to the user 
as fast as
 possible, otherwise the command may continue to block a little while
 longer until the job is done cleaning up.
 
-Using the *--keep-relative* flag will keep the backing chain names
-relative.
-
 *path* specifies fully-qualified path of the disk; it corresponds
 to a unique target name () or source file () for one of the disk devices attached to *domain* (see
 also ``domblklist`` for listing these names).
+
 *bandwidth* specifies copying bandwidth limit in MiB/s. For further information
 on the *bandwidth* argument see the corresponding section for the ``blockjob``
-command.
+command. Using *--bytes* flag indicates the value in *bandwidth* is given in
+bytes.
+
+*base* specifies fully-qualified path of the backing file; it corresponds
+to a unique indexed target name 'name[i]' (..
+) or source file 'name' ().
+
+Using the *--keep-relative* flag will keep the backing chain names
+relative (details on `https://www.libvirt.org/kbase/backing_chains.html
+`__).
 
 
 blockresize
-- 
2.25.2

[PATCH 0/3] doc: minor improvements

[Sebastian Mitterle]

Minor man/doc improvements related to blockpull.

Sebastian Mitterle (3):
  Improve blockpull man entry
  Improve Disk image chains documentation
  Add version info on domaincaps doc

 docs/formatdomaincaps.html.in |  3 ++-
 docs/kbase/backing_chains.rst | 36 +--
 docs/manpages/virsh.rst   | 19 --
 3 files changed, 33 insertions(+), 25 deletions(-)

-- 
2.25.2

[PATCH 2/3] Improve Disk image chains documentation

[Sebastian Mitterle]

1. Use 'setup' consistently as noun, 'set up' as verb
2. Use path variables like '$IMAGE_PATH' consistently
   like in Troubleshooting to improve readability
3. Remove ':' from field names
4. Change phrasing in sentences I stumbled upon several
   times to improve readability.
5. Minor grammar/vocab fixes.

Signed-off-by: Sebastian Mitterle 
---
 docs/kbase/backing_chains.rst | 36 +--
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/docs/kbase/backing_chains.rst b/docs/kbase/backing_chains.rst
index af848ccb14..c112f2bc82 100644
--- a/docs/kbase/backing_chains.rst
+++ b/docs/kbase/backing_chains.rst
@@ -38,13 +38,13 @@ image of the backing chain.
 This configuration will prompt libvirt to detect the backing image of the 
source
 image and recursively do the same thing until the end of the chain.
 
-Importance of properly setup backing chain
---
+Importance of proper backing chain setup
+
 
-The disk image locations are used by libvirt to properly setup the security
-system used on the host so that the hypervisor can access the files and 
possibly
-also directly to configure the hypervisor to use the appropriate images. Thus
-it's important to properly setup the formats and paths of the backing images.
+The disk image locations are used by libvirt to properly set up the security
+system used on the host so that the hypervisor can access the files; it can
+also be used to configure the hypervisor to use the appropriate images. Thus
+it's important to properly set up the formats and paths of the backing images.
 
 Any externally created image should always use the -F switch of ``qemu-img``
 to specify the format of the backing file to avoid probing.
@@ -60,7 +60,7 @@ explicitly in the XML or the overlay image itself.
 
 Libvirt also might lack support for a network disk storage technology and thus
 may be unable to visit and detect backing chains on such storage. This may
-result in the backing chain present in the live XML to look incomplete or some
+result in the backing chain in the live XML looking incomplete or some
 operations not being possible. To prevent this it's possible to specify the
 image metadata explicitly in the XML.
 
@@ -104,7 +104,7 @@ An empty  element signals the end of the 
chain. Using this
 will prevent libvirt or qemu from probing the backing chain.
 
 Note that it's also possible to partially specify the chain in the XML but omit
-the terminating element. This will result into probing from the last specified
+the terminating element. This will result in probing from the last specified
 
 
 Any image specified explicitly will not be probed for backing file or format.
@@ -120,10 +120,10 @@ means that the **-F** parameter of ``qemu-img`` must 
always be used.
 
 ::
 
-  qemu-img -f qcow2 -F qcow2 -b /path/to/backing /path/to/overlay
+  qemu-img -f qcow2 -F qcow2 -b $BACKING_IMAGE_PATH $IMAGE_PATH
 
-Note that if '/path/to/backing' is relative the path is considered relative to
-the location of '/path/to/overlay'.
+Note that if ``$BACKING_IMAGE_PATH`` is relative the path is considered 
relative to
+the location of ``$IMAGE_PATH``.
 
 Troubleshooting
 ===
@@ -164,7 +164,7 @@ the image has a backing image format specified:
  refcount bits: 16
  corrupt: false
 
-If the ``backing file format:`` field is missing above the format was not
+If the ``backing file format`` field is missing above the format was not
 specified properly. The image can be fixed by the following command:
 
 ::
@@ -177,22 +177,22 @@ If relative referencing of the backing image is desired, 
the path must be
 relative to the location of image described by ``$IMAGE_PATH``.
 
 **Important:** If the ``$BACKING_IMAGE_FORMAT`` is not known it can be queried
-using ``qemu-img info $BACKING_IMAGE_PATH`` and looking for the ``file 
format:``
-field, but for security reasons should be used *only* if at least one of the
-following criteria is met:
+using ``qemu-img info $BACKING_IMAGE_PATH`` and looking for the ``file format``
+field, but for security reasons the value should be used *only* if at least one
+of the following criteria is met:
 
 - ``file format`` is ``raw``
 - ``backing file`` is NOT present
 - ``backing file`` is present AND is correct/trusted
 
-Note that the last criteria may require manual inspection and thus should not
+Note that the last criterion may require manual inspection and thus should not
 be scripted unless the trust for the image can be expressed programatically.
 
 Also note that the above steps may need to be repeated recursively for any
 subsequent backing images.
 
-Missing images reported after after moving disk images into a different path
-
+Missing images reported after moving disk images into a different path
+---

[PATCH 3/3] Add version info on domaincaps doc

[Sebastian Mitterle]

1. Add 'since 5.10' as commonly used in formatdomain to avoid
   misunderstandings if element is not present (Is it not supported
   because of my version or because of my environment?)

Signed-off-by: Sebastian Mitterle 
---
 docs/formatdomaincaps.html.in | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in
index 4056e0bd31..70b1d2a8f9 100644
--- a/docs/formatdomaincaps.html.in
+++ b/docs/formatdomaincaps.html.in
@@ -561,7 +561,8 @@
 
 Reports whether the hypervisor will obey the 
 elements configured for a  when booting the guest, hotplugging
-the disk to a running guest, or similar.
+the disk to a running guest, or similar, since 5.10
+.
 
 
 backup
-- 
2.25.2

Re: [PATCH] docs: add zpci information to pci-addresses.rst

[Ján Tomko]

On a Wednesday in 2020, Cornelia Huck wrote:

Add some information on how pci address work on s390x.

Signed-off-by: Cornelia Huck 
---
docs/pci-addresses.rst | 63 ++
1 file changed, 63 insertions(+)

diff --git a/docs/pci-addresses.rst b/docs/pci-addresses.rst
index 923783a151b0..9e241a24fcfb 100644
--- a/docs/pci-addresses.rst
+++ b/docs/pci-addresses.rst
@@ -184,3 +184,66 @@ guest OS rather than as ``0001:08:00.1``, which is the 
address of the
device on the host.

Of course, all the rules and behaviors described above still apply.
+


I added one extra blank line to match what we use elsehwere in this file
for this level of headings.


+zPCI addresses
+==
+


Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

[PATCH] docs: add zpci information to pci-addresses.rst

[Cornelia Huck]

Add some information on how pci address work on s390x.

Signed-off-by: Cornelia Huck 
---
 docs/pci-addresses.rst | 63 ++
 1 file changed, 63 insertions(+)

diff --git a/docs/pci-addresses.rst b/docs/pci-addresses.rst
index 923783a151b0..9e241a24fcfb 100644
--- a/docs/pci-addresses.rst
+++ b/docs/pci-addresses.rst
@@ -184,3 +184,66 @@ guest OS rather than as ``0001:08:00.1``, which is the 
address of the
 device on the host.
 
 Of course, all the rules and behaviors described above still apply.
+
+zPCI addresses
+==
+
+For s390x machines, PCI addresses are handled yet differently. No
+topology information is relayed in the PCI addresses; instead, the
+``fid`` and ``uid`` elements of the ``zpci`` device convey information.
+In the simplest case, the following XML snippet
+
+::
+
+  
+  
+
+
+
+  
+
+  
+  
+
+
+
+
+  
+
+  
+
+will result in the following in a Linux guest::
+
+  0001:00:00.0 Ethernet controller: Red Hat, Inc. Virtio network device
+
+Note that the PCI bridge is not visible in the guest; s390x always has a flat
+topology.
+
+Neither are any changes in the PCI address visible in the guest; replacing
+the PCI address for the virtio-net device with
+
+::
+
+  
+
+will result in the exactly same view in the guest, as the addresses there
+are generated from the information provided via the ``zpci`` element (in
+fact, from the ``uid``).
+
+Therefore, replacing the virtio-net device definition with the following XML
+snippet
+
+::
+
+  
+
+
+
+
+  
+
+  
+
+will yield the following result in a Linux guest::
+
+  0007:00:00.0 Ethernet controller: Red Hat, Inc. Virtio network device
-- 
2.21.1

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

[Daniel Henrique Barboza]

On 4/14/20 6:32 AM, Michal Privoznik wrote:

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


Reviewed-by: Daniel Henrique Barboza 

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

[Andrea Bolognani]

On Wed, 2020-04-15 at 10:46 +0200, Christian Ehrhardt wrote:
> Hi Andrea,
> I saw this change committed and a latter push of mine has reported the
> following while running the pipelines:
> 
> ../docs/pci-addresses.rst:7:the the
> build-aux/syntax-check.mk: doubled words
> make: *** [../build-aux/syntax-check.mk:1727: sc_prohibit_doubled_word] Error 
> 1
> 
> This is due to:
>   2923e7a3 docs: Add pci-addresses.rst
> 
> Would you mind to provide a fixup as otherwise all other commits
> pushed will crash on that pipeline?

Done.

  commit e767f509b2bd2ec7a927515e37ee14b10e338313
  Author: Andrea Bolognani 
  Date:   Wed Apr 15 10:49:42 2020 +0200

docs: Fix word repetition in pci-addresses.rst

Fixes: 2923e7a3dd984c46202703d390dce3ff4ea4048c
Reported-by: Ján Tomko 
Signed-off-by: Andrea Bolognani 

I had not seen your message before now, sorry O:-)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH v5 3/4] vfio/mdev: add migration_version attribute for mdev (under mdev device node)

[Yan Zhao]

On Wed, Apr 15, 2020 at 03:42:58PM +0800, Erik Skultety wrote:
> On Mon, Apr 13, 2020 at 01:55:04AM -0400, Yan Zhao wrote:
> > migration_version attribute is used to check migration compatibility
> > between two mdev devices of the same mdev type.
> > The key is that it's rw and its data is opaque to userspace.
> >
> > Userspace reads migration_version of mdev device at source side and
> > writes the value to migration_version attribute of mdev device at target
> > side. It judges migration compatibility according to whether the read
> > and write operations succeed or fail.
> >
> > Currently, it is able to read/write migration_version attribute under two
> > places:
> >
> > (1) under mdev_type node
> > userspace is able to know whether two mdev devices are compatible before
> > a mdev device is created.
> >
> > userspace also needs to check whether the two mdev devices are of the same
> > mdev type before checking the migration_version attribute. It also needs
> > to check device creation parameters if aggregation is supported in future.
> >
> > (2) under mdev device node
> > userspace is able to know whether two mdev devices are compatible after
> > they are all created. But it does not need to check mdev type and device
> > creation parameter for aggregation as device vendor driver would have
> > incorporated those information into the migration_version attribute.
> >
> >  __userspace
> >   /\  \
> >  / \write
> > / read  \
> >/__   ___\|/_
> >   | migration_version | | migration_version |-->check migration
> >   - -   compatibility
> > mdev device A   mdev device B
> >
> > This patch is for mdev documentation about the second place (under
> > mdev device node)
> >
> > Cc: Alex Williamson 
> > Cc: Erik Skultety 
> > Cc: "Dr. David Alan Gilbert" 
> > Cc: Cornelia Huck 
> > Cc: "Tian, Kevin" 
> > Cc: Zhenyu Wang 
> > Cc: "Wang, Zhi A" 
> > Cc: Neo Jia 
> > Cc: Kirti Wankhede 
> > Cc: Daniel P. Berrangé 
> > Cc: Christophe de Dinechin 
> >
> > Signed-off-by: Yan Zhao 
> > ---
> >  .../driver-api/vfio-mediated-device.rst   | 70 +++
> >  1 file changed, 70 insertions(+)
> >
> > diff --git a/Documentation/driver-api/vfio-mediated-device.rst 
> > b/Documentation/driver-api/vfio-mediated-device.rst
> > index 2d1f3c0f3c8f..efbadfd51b7e 100644
> > --- a/Documentation/driver-api/vfio-mediated-device.rst
> > +++ b/Documentation/driver-api/vfio-mediated-device.rst
> > @@ -383,6 +383,7 @@ Directories and Files Under the sysfs for Each mdev 
> > Device
> >   |--- remove
> >   |--- mdev_type {link to its type}
> >   |--- vendor-specific-attributes [optional]
> > + |--- migration_verion [optional]
> >
> >  * remove (write only)
> >
> > @@ -394,6 +395,75 @@ Example::
> >
> > # echo 1 > /sys/bus/mdev/devices/$mdev_UUID/remove
> >
> > +* migration_version (rw, optional)
> 
> Hmm, ^this is not consistent with how patch 1/5 reports this information, but
> looking at the existing docs we're not doing very well in terms of consistency
> there either.
> 
> I suggest we go with "(read-write)" in both patch 1/5 and here and then start
> the paragraph with "This is an optional attribute."
>
ok. got it.

> > +  It is used to check migration compatibility between two mdev devices.
> > +  Absence of this attribute means the mdev device does not support 
> > migration.
> > +
> > +  This attribute provides a way to check migration compatibility between 
> > two
> > +  mdev devices from userspace after device created. The intended usage is
> 
> after the target device has been created.
> 
> side note: maybe add something like "(see the migration_version attribute of
> the device node if the target device already exists)" in the same section in
> patch 1/5.

ok. good idea.
> 
> > +  for userspace to read the migration_version attribute from one mdev 
> > device and
> > +  then writing that value to the migration_version attribute of the other 
> > mdev
> > +  device. The second mdev device indicates compatibility via the return 
> > code of
> > +  the write operation. This makes compatibility between mdev devices 
> > completely
> > +  vendor-defined and opaque to userspace. Userspace should do nothing more
> > +  than use the migration_version attribute to confirm source to target
> > +  compatibility.
> 
> ...
> 
> > +
> > +  Reading/Writing Attribute Data:
> > +  read(2) will fail if a mdev device does not support migration and 
> > otherwise
> > +succeed and return migration_version string of the mdev device.
> > +
> > +This migration_version string is vendor defined and opaque to the
> > +userspace. Vendor is free to include whatever they feel is 
> > relevant.
> > +e.g. -.
> > +
> > +Restrictions on this migration_version string:
> > +1.

Re: [PATCH v5 1/4] vfio/mdev: add migration_version attribute for mdev (under mdev_type node)

[Yan Zhao]

On Wed, Apr 15, 2020 at 03:28:51PM +0800, Erik Skultety wrote:
> On Mon, Apr 13, 2020 at 01:54:03AM -0400, Yan Zhao wrote:
> > migration_version attribute is used to check migration compatibility
> > between two mdev devices of the same mdev type.
> > The key is that it's rw and its data is opaque to userspace.
> >
> > Userspace reads migration_version of mdev device at source side and
> > writes the value to migration_version attribute of mdev device at target
> > side. It judges migration compatibility according to whether the read
> > and write operations succeed or fail.
> >
> > Currently, it is able to read/write migration_version attribute under two
> > places:
> >
> > (1) under mdev_type node
> > userspace is able to know whether two mdev devices are compatible before
> > a mdev device is created.
> >
> > userspace also needs to check whether the two mdev devices are of the same
> > mdev type before checking the migration_version attribute. It also needs
> > to check device creation parameters if aggregation is supported in future.
> >
> > (2) under mdev device node
> > userspace is able to know whether two mdev devices are compatible after
> > they are all created. But it does not need to check mdev type and device
> > creation parameter for aggregation as device vendor driver would have
> > incorporated those information into the migration_version attribute.
> >
> >  __userspace
> >   /\  \
> >  / \write
> > / read  \
> >/__   ___\|/_
> >   | migration_version | | migration_version |-->check migration
> >   - -   compatibility
> > mdev device A   mdev device B
> >
> > This patch is for mdev documentation about the first place (under
> > mdev_type node)
> >
> > Cc: Alex Williamson 
> > Cc: Erik Skultety 
> > Cc: "Dr. David Alan Gilbert" 
> > Cc: Cornelia Huck 
> > Cc: "Tian, Kevin" 
> > Cc: Zhenyu Wang 
> > Cc: "Wang, Zhi A" 
> > Cc: Neo Jia 
> > Cc: Kirti Wankhede 
> > Cc: Daniel P. Berrangé 
> > Cc: Christophe de Dinechin 
> >
> > Reviewed-by: Cornelia Huck 
> > Signed-off-by: Yan Zhao 
> >
> > ---
> > v5:
> > updated commit message a little to indicate this patch is for
> > migration_version attribute under mdev_type node
> >
> > v4:
> > fixed a typo. (Cornelia Huck)
> >
> > v3:
> > 1. renamed version to migration_version
> > (Christophe de Dinechin, Cornelia Huck, Alex Williamson)
> > 2. let errno to be freely defined by vendor driver
> > (Alex Williamson, Erik Skultety, Cornelia Huck, Dr. David Alan Gilbert)
> > 3. let checking mdev_type be prerequisite of migration compatibility
> > check. (Alex Williamson)
> > 4. reworded example usage section.
> > (most of this section came from Alex Williamson)
> > 5. reworded attribute intention section (Cornelia Huck)
> >
> > v2:
> > 1. added detailed intent and usage
> > 2. made definition of version string completely private to vendor driver
> >(Alex Williamson)
> > 3. abandoned changes to sample mdev drivers (Alex Williamson)
> > 4. mandatory --> optional (Cornelia Huck)
> > 5. added description for errno (Cornelia Huck)
> > ---
> >  .../driver-api/vfio-mediated-device.rst   | 113 ++
> >  1 file changed, 113 insertions(+)
> >
> > diff --git a/Documentation/driver-api/vfio-mediated-device.rst 
> > b/Documentation/driver-api/vfio-mediated-device.rst
> > index 25eb7d5b834b..2d1f3c0f3c8f 100644
> > --- a/Documentation/driver-api/vfio-mediated-device.rst
> > +++ b/Documentation/driver-api/vfio-mediated-device.rst
> > @@ -202,6 +202,7 @@ Directories and files under the sysfs for Each Physical 
> > Device
> >| |   |--- available_instances
> >| |   |--- device_api
> >| |   |--- description
> > +  | |   |--- migration_version
> >| |   |--- [devices]
> >| |--- []
> >| |   |--- create
> > @@ -209,6 +210,7 @@ Directories and files under the sysfs for Each Physical 
> > Device
> >| |   |--- available_instances
> >| |   |--- device_api
> >| |   |--- description
> > +  | |   |--- migration_version
> >| |   |--- [devices]
> >| |--- []
> >|  |--- create
> > @@ -216,6 +218,7 @@ Directories and files under the sysfs for Each Physical 
> > Device
> >|  |--- available_instances
> >|  |--- device_api
> >|  |--- description
> > +  |  |--- migration_version
> >|  |--- [devices]
> >
> >  * [mdev_supported_types]
> > @@ -246,6 +249,116 @@ Directories and files under the sysfs for Each 
> > Physical Device
> >This attribute should show the number of devices of type  that 
> > can be
> >created.
> 
> I've got only a few suggestions to improve to wording in the documentation
> (feel free to disagree):
> 
hi Erik,
Thanks for your good suggestions. They are better to understand than
the original ones:

[libvirt PATCH] docs: Fix word repetition in pci-addresses.rst

[Andrea Bolognani]

Fixes: 2923e7a3dd984c46202703d390dce3ff4ea4048c
Reported-by: Ján Tomko 
Signed-off-by: Andrea Bolognani 
---
Pushed under the build fix rule.

 docs/pci-addresses.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/pci-addresses.rst b/docs/pci-addresses.rst
index 991cb84aa1..923783a151 100644
--- a/docs/pci-addresses.rst
+++ b/docs/pci-addresses.rst
@@ -4,7 +4,7 @@ PCI addresses in domain XML and guest OS
 
 .. contents::
 
-When discussing PCI addresses, it's important to understand the the
+When discussing PCI addresses, it's important to understand the
 relationship between the addresses that can be seen in the domain XML
 and those that are visible inside the guest OS.
 
-- 
2.25.2

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

[Christian Ehrhardt]

Hi Andrea,
I saw this change committed and a latter push of mine has reported the
following while running the pipelines:

../docs/pci-addresses.rst:7:the the
build-aux/syntax-check.mk: doubled words
make: *** [../build-aux/syntax-check.mk:1727: sc_prohibit_doubled_word] Error 1

This is due to:
  2923e7a3 docs: Add pci-addresses.rst

Would you mind to provide a fixup as otherwise all other commits
pushed will crash on that pipeline?



On Wed, Apr 15, 2020 at 9:46 AM Andrea Bolognani  wrote:
>
> On Wed, 2020-04-15 at 08:47 +0200, Cornelia Huck wrote:
> > On Tue, 14 Apr 2020 19:53:05 +0200
> > Andrea Bolognani  wrote:
> > > +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?
>
> It could be an interesting example, sure! Would you mind writing a
> few lines about it? I don't have easy access to zPCI-capable s390x
> hardware.
>
> --
> Andrea Bolognani / Red Hat / Virtualization
>


--
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH v2] apparmor: avoid denials on libpmem initialization

[Christian Ehrhardt]

On Thu, Apr 9, 2020 at 6:57 PM Jamie Strandboge  wrote:
>
> On Thu, 09 Apr 2020, Christian Ehrhardt wrote:
>
> > With libpmem support compiled into qemu it will trigger the following
> > denials on every startup.
> >   apparmor="DENIED" operation="open" name="/"
> >   apparmor="DENIED" operation="open" name="/sys/bus/nd/devices/"
> >
> > This is due to [1] that tries to auto-detect if the platform supports
> > auto flush for all region.
> >
> > Once we know all the paths that are potentially needed if this feature
> > is really used we can add them conditionally in virt-aa-helper and labelling
> > calls in case  is enabled.
> >
> > But until then the change here silences the denial warnings seen above.
> >
> > [1]: 
> > https://github.com/pmem/pmdk/blob/master/src/libpmem2/auto_flush_linux.c#L131
> >
> > Bug: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1871354
> >
> > Signed-off-by: Christian Ehrhardt 
> > ---
> >  src/security/apparmor/libvirt-qemu | 5 +
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/src/security/apparmor/libvirt-qemu 
> > b/src/security/apparmor/libvirt-qemu
> > index 80986aec61..1a4b226612 100644
> > --- a/src/security/apparmor/libvirt-qemu
> > +++ b/src/security/apparmor/libvirt-qemu
> > @@ -227,3 +227,8 @@
> ># required for sasl GSSAPI plugin
> >/etc/gss/mech.d/ r,
> >/etc/gss/mech.d/* r,
> > +
> > +  # required by libpmem init to fts_open()/fts_read() the symlinks in
> > +  # /sys/bus/nd/devices
> > +  / r, # harmless on any lsb compliant system
> > +  /sys/bus/nd/devices/{,**/} r,
>
> LGTM. Thanks!

Thanks, it also works fine in all my tests and there was no other
negative feedback.
Added your acked-by and pushing to the repo now ...

> --
> Jamie Strandboge | http://www.canonical.com



-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

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

[Andrea Bolognani]

On Wed, 2020-04-15 at 08:47 +0200, Cornelia Huck wrote:
> On Tue, 14 Apr 2020 19:53:05 +0200
> Andrea Bolognani  wrote:
> > +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?

It could be an interesting example, sure! Would you mind writing a
few lines about it? I don't have easy access to zPCI-capable s390x
hardware.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH v5 3/4] vfio/mdev: add migration_version attribute for mdev (under mdev device node)

[Erik Skultety]

On Mon, Apr 13, 2020 at 01:55:04AM -0400, Yan Zhao wrote:
> migration_version attribute is used to check migration compatibility
> between two mdev devices of the same mdev type.
> The key is that it's rw and its data is opaque to userspace.
>
> Userspace reads migration_version of mdev device at source side and
> writes the value to migration_version attribute of mdev device at target
> side. It judges migration compatibility according to whether the read
> and write operations succeed or fail.
>
> Currently, it is able to read/write migration_version attribute under two
> places:
>
> (1) under mdev_type node
> userspace is able to know whether two mdev devices are compatible before
> a mdev device is created.
>
> userspace also needs to check whether the two mdev devices are of the same
> mdev type before checking the migration_version attribute. It also needs
> to check device creation parameters if aggregation is supported in future.
>
> (2) under mdev device node
> userspace is able to know whether two mdev devices are compatible after
> they are all created. But it does not need to check mdev type and device
> creation parameter for aggregation as device vendor driver would have
> incorporated those information into the migration_version attribute.
>
>  __userspace
>   /\  \
>  / \write
> / read  \
>/__   ___\|/_
>   | migration_version | | migration_version |-->check migration
>   - -   compatibility
> mdev device A   mdev device B
>
> This patch is for mdev documentation about the second place (under
> mdev device node)
>
> Cc: Alex Williamson 
> Cc: Erik Skultety 
> Cc: "Dr. David Alan Gilbert" 
> Cc: Cornelia Huck 
> Cc: "Tian, Kevin" 
> Cc: Zhenyu Wang 
> Cc: "Wang, Zhi A" 
> Cc: Neo Jia 
> Cc: Kirti Wankhede 
> Cc: Daniel P. Berrangé 
> Cc: Christophe de Dinechin 
>
> Signed-off-by: Yan Zhao 
> ---
>  .../driver-api/vfio-mediated-device.rst   | 70 +++
>  1 file changed, 70 insertions(+)
>
> diff --git a/Documentation/driver-api/vfio-mediated-device.rst 
> b/Documentation/driver-api/vfio-mediated-device.rst
> index 2d1f3c0f3c8f..efbadfd51b7e 100644
> --- a/Documentation/driver-api/vfio-mediated-device.rst
> +++ b/Documentation/driver-api/vfio-mediated-device.rst
> @@ -383,6 +383,7 @@ Directories and Files Under the sysfs for Each mdev Device
>   |--- remove
>   |--- mdev_type {link to its type}
>   |--- vendor-specific-attributes [optional]
> + |--- migration_verion [optional]
>
>  * remove (write only)
>
> @@ -394,6 +395,75 @@ Example::
>
>   # echo 1 > /sys/bus/mdev/devices/$mdev_UUID/remove
>
> +* migration_version (rw, optional)

Hmm, ^this is not consistent with how patch 1/5 reports this information, but
looking at the existing docs we're not doing very well in terms of consistency
there either.

I suggest we go with "(read-write)" in both patch 1/5 and here and then start
the paragraph with "This is an optional attribute."

> +  It is used to check migration compatibility between two mdev devices.
> +  Absence of this attribute means the mdev device does not support migration.
> +
> +  This attribute provides a way to check migration compatibility between two
> +  mdev devices from userspace after device created. The intended usage is

after the target device has been created.

side note: maybe add something like "(see the migration_version attribute of
the device node if the target device already exists)" in the same section in
patch 1/5.

> +  for userspace to read the migration_version attribute from one mdev device 
> and
> +  then writing that value to the migration_version attribute of the other 
> mdev
> +  device. The second mdev device indicates compatibility via the return code 
> of
> +  the write operation. This makes compatibility between mdev devices 
> completely
> +  vendor-defined and opaque to userspace. Userspace should do nothing more
> +  than use the migration_version attribute to confirm source to target
> +  compatibility.

...

> +
> +  Reading/Writing Attribute Data:
> +  read(2) will fail if a mdev device does not support migration and otherwise
> +succeed and return migration_version string of the mdev device.
> +
> +This migration_version string is vendor defined and opaque to the
> +userspace. Vendor is free to include whatever they feel is relevant.
> +e.g. -.
> +
> +Restrictions on this migration_version string:
> +1. It should only contain ascii characters
> +2. MAX Length is PATH_MAX (4096)
> +
> +  write(2) expects migration_version string of source mdev device, and will
> + succeed if it is determined to be compatible and otherwise fail with
> + vendor specific errno.
> +
> +  Errno:
> +  -An errno on read(2) indicates the mdev dev

Re: [PATCH v5 1/4] vfio/mdev: add migration_version attribute for mdev (under mdev_type node)

[Erik Skultety]

On Mon, Apr 13, 2020 at 01:54:03AM -0400, Yan Zhao wrote:
> migration_version attribute is used to check migration compatibility
> between two mdev devices of the same mdev type.
> The key is that it's rw and its data is opaque to userspace.
>
> Userspace reads migration_version of mdev device at source side and
> writes the value to migration_version attribute of mdev device at target
> side. It judges migration compatibility according to whether the read
> and write operations succeed or fail.
>
> Currently, it is able to read/write migration_version attribute under two
> places:
>
> (1) under mdev_type node
> userspace is able to know whether two mdev devices are compatible before
> a mdev device is created.
>
> userspace also needs to check whether the two mdev devices are of the same
> mdev type before checking the migration_version attribute. It also needs
> to check device creation parameters if aggregation is supported in future.
>
> (2) under mdev device node
> userspace is able to know whether two mdev devices are compatible after
> they are all created. But it does not need to check mdev type and device
> creation parameter for aggregation as device vendor driver would have
> incorporated those information into the migration_version attribute.
>
>  __userspace
>   /\  \
>  / \write
> / read  \
>/__   ___\|/_
>   | migration_version | | migration_version |-->check migration
>   - -   compatibility
> mdev device A   mdev device B
>
> This patch is for mdev documentation about the first place (under
> mdev_type node)
>
> Cc: Alex Williamson 
> Cc: Erik Skultety 
> Cc: "Dr. David Alan Gilbert" 
> Cc: Cornelia Huck 
> Cc: "Tian, Kevin" 
> Cc: Zhenyu Wang 
> Cc: "Wang, Zhi A" 
> Cc: Neo Jia 
> Cc: Kirti Wankhede 
> Cc: Daniel P. Berrangé 
> Cc: Christophe de Dinechin 
>
> Reviewed-by: Cornelia Huck 
> Signed-off-by: Yan Zhao 
>
> ---
> v5:
> updated commit message a little to indicate this patch is for
> migration_version attribute under mdev_type node
>
> v4:
> fixed a typo. (Cornelia Huck)
>
> v3:
> 1. renamed version to migration_version
> (Christophe de Dinechin, Cornelia Huck, Alex Williamson)
> 2. let errno to be freely defined by vendor driver
> (Alex Williamson, Erik Skultety, Cornelia Huck, Dr. David Alan Gilbert)
> 3. let checking mdev_type be prerequisite of migration compatibility
> check. (Alex Williamson)
> 4. reworded example usage section.
> (most of this section came from Alex Williamson)
> 5. reworded attribute intention section (Cornelia Huck)
>
> v2:
> 1. added detailed intent and usage
> 2. made definition of version string completely private to vendor driver
>(Alex Williamson)
> 3. abandoned changes to sample mdev drivers (Alex Williamson)
> 4. mandatory --> optional (Cornelia Huck)
> 5. added description for errno (Cornelia Huck)
> ---
>  .../driver-api/vfio-mediated-device.rst   | 113 ++
>  1 file changed, 113 insertions(+)
>
> diff --git a/Documentation/driver-api/vfio-mediated-device.rst 
> b/Documentation/driver-api/vfio-mediated-device.rst
> index 25eb7d5b834b..2d1f3c0f3c8f 100644
> --- a/Documentation/driver-api/vfio-mediated-device.rst
> +++ b/Documentation/driver-api/vfio-mediated-device.rst
> @@ -202,6 +202,7 @@ Directories and files under the sysfs for Each Physical 
> Device
>| |   |--- available_instances
>| |   |--- device_api
>| |   |--- description
> +  | |   |--- migration_version
>| |   |--- [devices]
>| |--- []
>| |   |--- create
> @@ -209,6 +210,7 @@ Directories and files under the sysfs for Each Physical 
> Device
>| |   |--- available_instances
>| |   |--- device_api
>| |   |--- description
> +  | |   |--- migration_version
>| |   |--- [devices]
>| |--- []
>|  |--- create
> @@ -216,6 +218,7 @@ Directories and files under the sysfs for Each Physical 
> Device
>|  |--- available_instances
>|  |--- device_api
>|  |--- description
> +  |  |--- migration_version
>|  |--- [devices]
>
>  * [mdev_supported_types]
> @@ -246,6 +249,116 @@ Directories and files under the sysfs for Each Physical 
> Device
>This attribute should show the number of devices of type  that 
> can be
>created.

I've got only a few suggestions to improve to wording in the documentation
(feel free to disagree):

>
> +* migration_version
> +
> +  This attribute is rw, and is optional.

IMO better wording: "This is an optional, RW attribute."

> +  It is used to check migration compatibility between two mdev devices of the
> +  same mdev type. Absence of this attribute means the device of type 
> 
> +  does not support migration.
> +  This attribute provides a way to check migration compatibility between two
> +  md