Re: [PATCH] News: Several apparmor improvements at v6.7.0

[Jianan Gao]

Hi Andrea,
Thanks for your advice, i'll try again with your suggestions!

Jianan Gao

On Thu, Nov 12, 2020 at 9:45 PM Andrea Bolognani 
wrote:

> Please don't CC individual developers when posting patches: we're all
> subscribed to the mailing list.
>
> On Mon, 2020-11-09 at 15:33 +0800, jgao wrote:
> > From: jgao 
> >
> > Add news about apparmor about the improvements.
> >
> > Signed-off-by: jgao 
>
> Please make sure both the Signed-off-by tag and git authorship
> information are in the form
>
>   FirstName LastName 
>
> We generally can't accept patches that don't have a S-o-b in that
> format.
>
> > +  * apparmor: Several improvements
> > +
> > +   Add support for virtiofs filesystem, allowing qemu load old shared
> objects after
> > +   upgrades.
>
> These are completely distinct changes, but the way you constructed
> your sentence makes it appear as if they were connected. I suggest
> something like
>
>   Add support for the virtiofs filesystem and allow QEMU to load old
>   modules after upgrade.
>
> Does that sound good to you?
>
> --
> Andrea Bolognani / Red Hat / Virtualization
>
>

Re: [PATCH] util: remove ATTRIBUTE_NONNULL from virDirClose declaration

[Laine Stump]

After short discussion on IRC, I pushed this as "trivial".

On 11/13/20 12:42 PM, Laine Stump wrote:

Before commit 24d8968c, virDirClose took a DIR**, and that was never
NULL, so its declaration included ATTRIBUTE_NONNULL(1). Since that
commit, virDirClose takes a DIR*, and it may be NULL (e.g. if the DIR*
is initialized to NULL and was never closed).

Even though virDirClose() is currently only called implicitly (as the
cleanup for a g_autoptr(DIR)), and (as I've just newly learned) the
autocleanup function g_autoptr will only be called if the pointer in
question is non-null (see the definition of
_GLIB_AUTOPTR_CLEAR_FUNC_NAME in
/usr/include/glib-2.0/glib/gmacros.h), it does still cause Coverity to
complain that it *could* be called with a NULL, and it's also possible
that in the future someone might add code that explicitly calls
virDirClose.

To eliminate the Coverity complaints, and protect against the
hypothetical future where someone both explicitly calls virDirClose()
with a potentially NULL value, *and* re-enables the nonnull directive
when not building with Coverity (disabled by commit eefb881) this
patch removes the ATTRIBUTE_NONNULL(1) from the declaration of
virDirClose().

Fixes: 24d8968cd0a718af4badbbc858b1b449fea7205a
Details-Research-by: Dan Berrange 
Signed-off-by: Laine Stump 
---
  src/util/virfile.h | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/util/virfile.h b/src/util/virfile.h
index ba31a78e58..32505826ee 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -269,8 +269,7 @@ int virDirOpenQuiet(DIR **dirp, const char *dirname)
  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT;
  int virDirRead(DIR *dirp, struct dirent **ent, const char *dirname)
  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT;
-void virDirClose(DIR *dirp)
-ATTRIBUTE_NONNULL(1);
+void virDirClose(DIR *dirp);
  G_DEFINE_AUTOPTR_CLEANUP_FUNC(DIR, virDirClose);
  
  int virFileMakePath(const char *path) G_GNUC_WARN_UNUSED_RESULT;

Re: [PATCH 1/1] qemu_driver.c: do not redefine 'event' in qemuDomainDefineXMLFlags()

[John Ferlan]

On 11/13/20 12:57 PM, Daniel Henrique Barboza wrote:
> A bad merge while rebasing 74b2834333a caused the @event variable
> to be defined twice, inside the 'cleanup' label, causing coverity
> errors.
> 
> This code was originally moved outside of the label by commit
> 773c7c43611a. Delete the unintended code in the 'cleanup'
> label.
> 
> Fixes: 74b2834333ab3bf500f870e0a6d4e8309379d96a
> Reported-by: John Ferlan 
> Signed-off-by: Daniel Henrique Barboza 
> ---
>  src/qemu/qemu_driver.c | 9 -
>  1 file changed, 9 deletions(-)
> 

Reviewed-by: John Ferlan 

Tks -

John

Re: [PATCH v1 02/10] qemu_domain.c: align memory modules before calculating 'initialmem'

[Daniel Henrique Barboza]

(Peter, I cc'ed you because I cited one of your commits and you might want
to weight in)

On 11/13/20 10:51 AM, Andrea Bolognani wrote:

On Fri, 2020-11-13 at 09:58 -0300, Daniel Henrique Barboza wrote:

On 11/13/20 7:30 AM, Andrea Bolognani wrote:

On Wed, 2020-11-11 at 19:07 -0300, Daniel Henrique Barboza wrote:

+++ b/tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma.args


[...]


I only skimmed the remaining patches and didn't dig into the code as
much, or as recently, as you have, but from a high-level perspective
I don't see why you wouldn't be able to simply move the existing
rounding logic from the command line generator to PostParse? It's not
like the former has access to additional information that the latter
can't get to, right?



I was looking into the code and I think that might have the wrong idea here.
Apparently we're not aligning memory during migration or snapshot restore.
This specific line in qemu_command.c got my attention:

-- qemuBuildCommandLine() --

if (!migrateURI && !snapshot && qemuDomainAlignMemorySizes(def) < 0)
return NULL;

--

I investigated the history behind this logic and found the following commit:

commit c7d7ba85a6242d789ba3f4dae313e950fbb638c5
Author: Peter Krempa 
Date:   Thu Sep 17 08:14:05 2015 +0200

qemu: command: Align memory sizes only on fresh starts

When we are starting a qemu process for an incomming migration or

snapshot reloading we should not modify the memory sizes in the domain
since we could potentially change the guest ABI that was tediously
checked before. Additionally the function now updates the initial memory
size according to the NUMA node size, which should not happen if we are
restoring state.

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


-


This means that the changes made in this series will not break migration, since
the alignment of 'initialmem' is not being triggered in those cases. Which
is good.

However, you also brought up in an earlier reply that these changes might break
"the guest ABI across guest boots (if libvirt is upgraded in between them)". 
This
can't be helped I think - an older ppc64 guest with 4GiB of 'currentMemory' in 
the
XML, that is ending up having 4.256GiB (extra 256MiB) due to the way alignment 
was
being done, will have exactly 4GiB of 'initialmem' after these changes. My 
point is
that we're giving the exact memory the guest is demanding, as intended by the 
domain
XML, in a fresh guest start. This might be considered an ABI break probably, but
why would one complain that Libvirt is now giving precisely what was asked for?
Avoiding granting extra 256MBs of mem for domains seems worth it, given that 
we're
not impacting live domains or migration.


Thanks,


DHB

Re: [PATCH 4/4] ci: Run test suite on macOS

[Andrea Bolognani]

On Fri, 2020-11-13 at 16:58 +0100, Michal Privoznik wrote:
> On 11/8/20 10:24 PM, Roman Bolshakov wrote:
> > -- if test "$(uname)" = "FreeBSD"; then ninja -C build dist; fi
> > -- if test "$(uname)" = "Darwin"; then ninja -C build && ninja -C build 
> > install; fi
> > +- ninja -C build dist
> 
> If we go with --timeout-multiplier= as I'm suggesting in 3/4 then this 
> can't be ninja, but meson. I'm not sure what the whole point of 'ninja' 
> is at this point, sorry.

ninja is the low-level tool, so in some cases (notably running the
test suite) having meson call ninja instead of invoking the latter
directly can enable additional features.

In this case, I assume 'ninja dist' will use 'ninja test' rather than
'meson test', so we might have to do something like

  - if test "$(uname)" = "FreeBSD"; then cd build && ninja dist; fi
  - if test "$(uname)" = "Darwin"; then cd build && ninja && meson test 
--timeout-multiplier=X && ninja install; fi

to run the test suite on macOS without hitting the timeout.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH 3/4] qemuxml2argvtest: Increase timeout on macOS

[Andrea Bolognani]

On Fri, 2020-11-13 at 16:58 +0100, Michal Privoznik wrote:
> On 11/8/20 10:24 PM, Roman Bolshakov wrote:
> > +  if data['name'] == 'qemuxml2argvtest' and host_machine.system() == 
> > 'darwin'
> > +timeout = 180
> > +  else
> > +# default meson timeout
> > +timeout = 30
> > +  endif
> > +  test(data['name'], test_bin, env: tests_env, timeout: timeout)
> 
> I think the last time I wanted to increase the timeout I was told that 
> it is machine specific and since I know my machine the best I should 
> use: meson test --timeout-multiplier=X

Agreed: in my local test environment, for example, the test suite
completes successfully without tweaking the timeout at all. If the
Cirrus CI builders are slower and require a larger timeout that's
perfectly fine, but we should apply the tweak for that environment
only using the command line argument.

-- 
Andrea Bolognani / Red Hat / Virtualization

[PATCH] virsh: Added attach-disk support for network disk

[Ryan Gahagan]

Related issue: https://gitlab.com/libvirt/libvirt/-/issues/16
Added in support for the following parameters in attach-disk:
--source-protocol
--source-host-name
--source-host-socket
--source-host-transport

Added documentation to virsh.rst specifying usage.

Signed-off-by: Ryan Gahagan 
---
 docs/manpages/virsh.rst | 26 ++---
 tools/virsh-domain.c| 85 ++---
 2 files changed, 100 insertions(+), 11 deletions(-)

diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
index bfd26e3120..a4d70e9211 100644
--- a/docs/manpages/virsh.rst
+++ b/docs/manpages/virsh.rst
@@ -4500,14 +4500,18 @@ attach-disk
   [--current]] | [--persistent]] [--targetbus bus]
   [--driver driver] [--subdriver subdriver] [--iothread iothread]
   [--cache cache] [--io io] [--type type] [--alias alias]
-  [--mode mode] [--sourcetype sourcetype] [--serial serial]
-  [--wwn wwn] [--rawio] [--address address] [--multifunction]
-  [--print-xml]
+  [--mode mode] [--sourcetype sourcetype]
+  [--source-protocol protocol] [--source-host-name hostname:port]
+  [--source-host-transport transport] [--source-host-socket socket]
+  [--serial serial] [--wwn wwn] [--rawio] [--address address]
+  [--multifunction] [--print-xml]
 
 Attach a new disk device to the domain.
-*source* is path for the files and devices. *target* controls the bus or
-device under which the disk is exposed to the guest OS. It indicates the
-"logical" device name; the optional *targetbus* attribute specifies the type
+*source* is path for the files and devices unless *--source-protocol*
+is specified, in which case *source* is the name of a network disk.
+*target* controls the bus or device under which the disk is exposed
+to the guest OS. It indicates the "logical" device name;
+the optional *targetbus* attribute specifies the type
 of disk device to emulate; possible values are driver specific, with typical
 values being *ide*, *scsi*, *virtio*, *xen*, *usb*, *sata*, or *sd*, if
 omitted, the bus type is inferred from the style of the device name (e.g.  a
@@ -4541,6 +4545,16 @@ ccw:cssid.ssid.devno. Virtio-ccw devices must have their 
cssid set to 0xfe.
 *multifunction* indicates specified pci address is a multifunction pci device
 address.
 
+There is also support for using a network disk. As specified, the user can
+provide a *--source-protocol* in which case the *source* parameter will
+be interpreted as the source name. *--source-protocol* must be provided
+if the user also wishes to provide host information.
+Host information can be provided using any of the tags
+*--source-host-name*, *--source-host-transport*, and *--source-host-socket*,
+which respectively denote the name of the host, the host's transport method,
+and the socket that the host uses. The *--source-host-name* parameter
+supports host:port syntax if the user wants to provide a port as well.
+
 If *--print-xml* is specified, then the XML of the disk that would be attached
 is printed instead.
 
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 12b35c037d..4c43da7a2c 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -222,7 +222,7 @@ static const vshCmdOptDef opts_attach_disk[] = {
 {.name = "source",
  .type = VSH_OT_DATA,
  .flags = VSH_OFLAG_REQ | VSH_OFLAG_EMPTY_OK,
- .help = N_("source of disk device")
+ .help = N_("source of disk device or name of network disk")
 },
 {.name = "target",
  .type = VSH_OT_DATA,
@@ -298,6 +298,22 @@ static const vshCmdOptDef opts_attach_disk[] = {
  .type = VSH_OT_BOOL,
  .help = N_("print XML document rather than attach the disk")
 },
+{.name = "source-protocol",
+ .type = VSH_OT_STRING,
+ .help = N_("protocol used by disk device source")
+},
+{.name = "source-host-name",
+ .type = VSH_OT_STRING,
+ .help = N_("host name for source of disk device")
+},
+{.name = "source-host-transport",
+ .type = VSH_OT_STRING,
+ .help = N_("host transport for source of disk device")
+},
+{.name = "source-host-socket",
+ .type = VSH_OT_STRING,
+ .help = N_("host socket for source of disk device")
+},
 VIRSH_COMMON_OPT_DOMAIN_PERSISTENT,
 VIRSH_COMMON_OPT_DOMAIN_CONFIG,
 VIRSH_COMMON_OPT_DOMAIN_LIVE,
@@ -567,6 +583,12 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
 *iothread = NULL, *cache = NULL, *io = NULL,
 *serial = NULL, *straddr = NULL, *wwn = NULL,
 *targetbus = NULL, *alias = NULL;
+const char *source_protocol = NULL;
+const char *host_name = NULL;
+const char *host_transport = NULL;
+const char *host_socket = NULL;
+char *host_name_copy = NULL;
+char *host_port = NULL;
 struct DiskAddress diskAddr;
 bool isFile = false, functionReturn = false;
 int ret;
@@ -604,7 +626,11 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
 

Re: [PATCH v1 01/10] qemu_driver.c: use g_autoptr() with virDomainDef pointers

[Daniel Henrique Barboza]

On 11/13/20 2:39 PM, John Ferlan wrote:


Coverity found a bad merge conflict resolution...



Thanks John. Just posted a fix. Guess I'll have to find a way to
run Coverity before pushing stuff ...


DHB




On 11/12/20 4:48 PM, Daniel Henrique Barboza wrote:

Signed-off-by: Daniel Henrique Barboza 
---
  src/qemu/qemu_driver.c | 54 ++
  1 file changed, 18 insertions(+), 36 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 05f8eb2cb7..fdbac9d21d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c


[...]


  ret = -1;
@@ -6689,8 +6682,8 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
   unsigned int flags)
  {
  virQEMUDriverPtr driver = conn->privateData;
-virDomainDefPtr def = NULL;
-virDomainDefPtr oldDef = NULL;
+g_autoptr(virDomainDef) def = NULL;
+g_autoptr(virDomainDef) oldDef = NULL;
  virDomainObjPtr vm = NULL;
  virDomainPtr dom = NULL;
  virObjectEventPtr event = NULL;
@@ -6751,8 +6744,6 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
  dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id);
  
   cleanup:

-virDomainDefFree(oldDef);
-virDomainDefFree(def);
  virDomainObjEndAPI();
  virObjectEventStateQueue(driver->domainEventState, event);
  return dom;
@@ -7755,7 +7746,7 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm,


The code that was pushed added:

+
+event = virDomainEventLifecycleNewFromObj(vm,
+ VIR_DOMAIN_EVENT_DEFINED,
+ !oldDef ?
+ VIR_DOMAIN_EVENT_DEFINED_ADDED :
+ VIR_DOMAIN_EVENT_DEFINED_UPDATED);
+
+VIR_INFO("Creating domain '%s'", vm->def->name);
+dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id);
+

within the cleanup: label even though recent commit 773c7c43611 had
moved the code up higher.

The coverity error is that @event gets overwritten...

Hopefully easy enough to figure out.

John
[...]

[PATCH 1/1] qemu_driver.c: do not redefine 'event' in qemuDomainDefineXMLFlags()

[Daniel Henrique Barboza]

A bad merge while rebasing 74b2834333a caused the @event variable
to be defined twice, inside the 'cleanup' label, causing coverity
errors.

This code was originally moved outside of the label by commit
773c7c43611a. Delete the unintended code in the 'cleanup'
label.

Fixes: 74b2834333ab3bf500f870e0a6d4e8309379d96a
Reported-by: John Ferlan 
Signed-off-by: Daniel Henrique Barboza 
---
 src/qemu/qemu_driver.c | 9 -
 1 file changed, 9 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 78ccf96df5..ac38edf009 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6748,15 +6748,6 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
 }
 }
 
-event = virDomainEventLifecycleNewFromObj(vm,
- VIR_DOMAIN_EVENT_DEFINED,
- !oldDef ?
- VIR_DOMAIN_EVENT_DEFINED_ADDED :
- VIR_DOMAIN_EVENT_DEFINED_UPDATED);
-
-VIR_INFO("Creating domain '%s'", vm->def->name);
-dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id);
-
 virDomainObjEndAPI();
 virObjectEventStateQueue(driver->domainEventState, event);
 return dom;
-- 
2.26.2

[PATCH] util: remove ATTRIBUTE_NONNULL from virDirClose declaration

[Laine Stump]

Before commit 24d8968c, virDirClose took a DIR**, and that was never
NULL, so its declaration included ATTRIBUTE_NONNULL(1). Since that
commit, virDirClose takes a DIR*, and it may be NULL (e.g. if the DIR*
is initialized to NULL and was never closed).

Even though virDirClose() is currently only called implicitly (as the
cleanup for a g_autoptr(DIR)), and (as I've just newly learned) the
autocleanup function g_autoptr will only be called if the pointer in
question is non-null (see the definition of
_GLIB_AUTOPTR_CLEAR_FUNC_NAME in
/usr/include/glib-2.0/glib/gmacros.h), it does still cause Coverity to
complain that it *could* be called with a NULL, and it's also possible
that in the future someone might add code that explicitly calls
virDirClose.

To eliminate the Coverity complaints, and protect against the
hypothetical future where someone both explicitly calls virDirClose()
with a potentially NULL value, *and* re-enables the nonnull directive
when not building with Coverity (disabled by commit eefb881) this
patch removes the ATTRIBUTE_NONNULL(1) from the declaration of
virDirClose().

Fixes: 24d8968cd0a718af4badbbc858b1b449fea7205a
Details-Research-by: Dan Berrange 
Signed-off-by: Laine Stump 
---
 src/util/virfile.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/util/virfile.h b/src/util/virfile.h
index ba31a78e58..32505826ee 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -269,8 +269,7 @@ int virDirOpenQuiet(DIR **dirp, const char *dirname)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT;
 int virDirRead(DIR *dirp, struct dirent **ent, const char *dirname)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT;
-void virDirClose(DIR *dirp)
-ATTRIBUTE_NONNULL(1);
+void virDirClose(DIR *dirp);
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(DIR, virDirClose);
 
 int virFileMakePath(const char *path) G_GNUC_WARN_UNUSED_RESULT;
-- 
2.28.0

Re: [PATCH v1 01/10] qemu_driver.c: use g_autoptr() with virDomainDef pointers

[John Ferlan]

Coverity found a bad merge conflict resolution...

On 11/12/20 4:48 PM, Daniel Henrique Barboza wrote:
> Signed-off-by: Daniel Henrique Barboza 
> ---
>  src/qemu/qemu_driver.c | 54 ++
>  1 file changed, 18 insertions(+), 36 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 05f8eb2cb7..fdbac9d21d 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c

[...]

>  ret = -1;
> @@ -6689,8 +6682,8 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
>   unsigned int flags)
>  {
>  virQEMUDriverPtr driver = conn->privateData;
> -virDomainDefPtr def = NULL;
> -virDomainDefPtr oldDef = NULL;
> +g_autoptr(virDomainDef) def = NULL;
> +g_autoptr(virDomainDef) oldDef = NULL;
>  virDomainObjPtr vm = NULL;
>  virDomainPtr dom = NULL;
>  virObjectEventPtr event = NULL;
> @@ -6751,8 +6744,6 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
>  dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id);
>  
>   cleanup:
> -virDomainDefFree(oldDef);
> -virDomainDefFree(def);
>  virDomainObjEndAPI();
>  virObjectEventStateQueue(driver->domainEventState, event);
>  return dom;
> @@ -7755,7 +7746,7 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm,

The code that was pushed added:

+
+event = virDomainEventLifecycleNewFromObj(vm,
+ VIR_DOMAIN_EVENT_DEFINED,
+ !oldDef ?
+ VIR_DOMAIN_EVENT_DEFINED_ADDED :
+ VIR_DOMAIN_EVENT_DEFINED_UPDATED);
+
+VIR_INFO("Creating domain '%s'", vm->def->name);
+dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id);
+

within the cleanup: label even though recent commit 773c7c43611 had
moved the code up higher.

The coverity error is that @event gets overwritten...

Hopefully easy enough to figure out.

John
[...]

[PATCH] virnetdevopenvswitch: Fix ATTRIBUTE_NONNULL() tag for virNetDevOpenvswitchGetVhostuserIfname()

[Michal Privoznik]

After e4c29e2904 the function has one argument more and the
argument that can't be NULL moved from second to third position.

Reported-by: John Ferlan 
Signed-off-by: Michal Privoznik 
---

Pushed as trivial.

 src/util/virnetdevopenvswitch.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/virnetdevopenvswitch.h b/src/util/virnetdevopenvswitch.h
index 5cd1d22ae3..8409aa92ac 100644
--- a/src/util/virnetdevopenvswitch.h
+++ b/src/util/virnetdevopenvswitch.h
@@ -63,7 +63,7 @@ int virNetDevOpenvswitchInterfaceGetMaster(const char 
*ifname, char **master)
 int virNetDevOpenvswitchGetVhostuserIfname(const char *path,
bool server,
char **ifname)
-ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT G_GNUC_NO_INLINE;
+ATTRIBUTE_NONNULL(3) G_GNUC_WARN_UNUSED_RESULT G_GNUC_NO_INLINE;
 
 int virNetDevOpenvswitchUpdateVlan(const char *ifname,
const virNetDevVlan *virtVlan)
-- 
2.26.2

Re: [libvirt PATCH] kbase: Shorten "less verbose QEMU logging" example

[Andrea Bolognani]

On Fri, 2020-11-13 at 17:09 +0100, Peter Krempa wrote:
> On Fri, Nov 13, 2020 at 16:34:25 +0100, Andrea Bolognani wrote:
> > Rationale for the changes:
> > 
> >   * util.dbus produces very little logging, so it doesn't really
> > make sense to filter it out;
> 
> IIRC it did log a lot before the switch to gdbus, thus I'd prefer to
> keep this since it's not that long since we switched to gdbus.

Fair enough! Re-added before pushing.

-- 
Andrea Bolognani / Red Hat / Virtualization

[libvirt PATCH 4/4] rpm: enable wireshark build for RHEL-8 and later

[Daniel P . Berrangé]

wireshark plugin was disabled in RHEL because RHEL-7 was too old, but we
forgot to enable it in RHEL-8 where it builds fine.

Signed-off-by: Daniel P. Berrangé 
---
 libvirt.spec.in | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 43955b0e09..de76878bc6 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -167,8 +167,8 @@
 %define with_libssh2 0%{!?_without_libssh2:1}
 %endif
 
-# Enable wireshark plugins for all distros shipping libvirt 1.2.2 or newer
-%if 0%{?fedora}
+# Enable wireshark plugins for all distros except RHEL-7
+%if 0%{?fedora} || 0%{?rhel} > 7
 %define with_wireshark 0%{!?_without_wireshark:1}
 %define wireshark_plugindir %(pkg-config --variable plugindir 
wireshark)/epan
 %endif
-- 
2.28.0

[libvirt PATCH 3/4] rpm: remove version checks for wireshark

[Daniel P . Berrangé]

We only turn on with_wireshark if we already know the distro is
guaranteed to have new enough packages. The versioned dep is thus not
required.

Signed-off-by: Daniel P. Berrangé 
---
 libvirt.spec.in | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 034982809d..43955b0e09 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -386,7 +386,7 @@ BuildRequires: numad
 %endif
 
 %if %{with_wireshark}
-BuildRequires: wireshark-devel >= 2.4.0
+BuildRequires: wireshark-devel
 %endif
 
 %if %{with_libssh}
@@ -928,7 +928,7 @@ Bash completion script stub.
 %if %{with_wireshark}
 %package wireshark
 Summary: Wireshark dissector plugin for libvirt RPC transactions
-Requires: wireshark >= 2.4.0
+Requires: wireshark
 Requires: %{name}-libs = %{version}-%{release}
 
 %description wireshark
-- 
2.28.0

[libvirt PATCH 0/4] Misc version updates

[Daniel P . Berrangé]

In both yajl and wireshark we can bump our min version per our platform
support matrix and thus eliminate some historical cruft.

Daniel P. Berrangé (4):
  meson: assume pkg-config support for yajl
  meson: bump min wireshark to 2.6.0
  rpm: remove version checks for wireshark
  rpm: enable wireshark build for RHEL-8 and later

 libvirt.spec.in |  8 +++
 meson.build | 63 ++---
 2 files changed, 22 insertions(+), 49 deletions(-)

-- 
2.28.0

[libvirt PATCH 1/4] meson: assume pkg-config support for yajl

[Daniel P . Berrangé]

Per the platform support rules, we no longer need to consider SLES 12 as
a target, and so can now assume pkg-config support in yajl.

Signed-off-by: Daniel P. Berrangé 
---
 meson.build | 20 +---
 1 file changed, 1 insertion(+), 19 deletions(-)

diff --git a/meson.build b/meson.build
index cecaad199d..816cf97a19 100644
--- a/meson.build
+++ b/meson.build
@@ -1460,25 +1460,7 @@ else
 endif
 
 yajl_version = '2.0.3'
-if not get_option('yajl').disabled()
-  yajl_dep = dependency('yajl', version: '>=' + yajl_version, required: false)
-  # 2.0.3 was the version where the pkg-config file was first added
-  # SLES 12 and openSUSE Leap 42.3 still use 2.0.1
-  # TODO: delete this in July 2020
-  if not yajl_dep.found()
-yajl_dep = cc.find_library('yajl', required: get_option('yajl'))
-if yajl_dep.found()
-  has_func = cc.has_function('yajl_tree_parse', dependencies: yajl_dep, 
prefix: '#include ')
-  if not has_func and get_option('yajl').enabled()
-error('yajl >= @0@ was not found'.format(yajl_version))
-  elif not has_func
-yajl_dep = dependency('', required: false)
-  endif
-endif
-  endif
-else
-  yajl_dep = dependency('', required: false)
-endif
+yajl_dep = dependency('yajl', version: '>=' + yajl_version, required: 
get_option('yajl'))
 if yajl_dep.found()
   conf.set('WITH_YAJL', 1)
 endif
-- 
2.28.0

[libvirt PATCH 2/4] meson: bump min wireshark to 2.6.0

[Daniel P . Berrangé]

If using the declared min version of wireshark, 2.4.0, libvirt plugin
fails to build. This min version isn't present in any supported distros
and thus not tested by CI.

We don't support wireshark on RHEL-7 since it has 1.x.x series. The next
oldest version present in supported distros is 2.6.2 on RHEL-8.

Thus we should bump the min version to 2.6.0. This also lets us assume
that the "plugindir" variable exists in pkg-config.

Signed-off-by: Daniel P. Berrangé 
---
 meson.build | 43 +--
 1 file changed, 17 insertions(+), 26 deletions(-)

diff --git a/meson.build b/meson.build
index 816cf97a19..565a5f12e2 100644
--- a/meson.build
+++ b/meson.build
@@ -1403,7 +1403,7 @@ else
   win32_link_flags = []
 endif
 
-wireshark_version = '2.4.0'
+wireshark_version = '2.6.0'
 wireshark_dep = dependency('wireshark', version: '>=' + wireshark_version, 
required: get_option('wireshark_dissector'))
 if wireshark_dep.found()
   wireshark_plugindir = get_option('wireshark_plugindir')
@@ -1411,31 +1411,22 @@ if wireshark_dep.found()
 wireshark_plugindir = wireshark_dep.get_pkgconfig_variable('plugindir')
   endif
 
-  # On some systems the plugindir variable may not be stored within pkg config.
-  # Fall back to older style of constructing the plugin dir path.
-  if wireshark_plugindir == ''
-wireshark_modversion = wireshark_dep.version()
-wireshark_plugindir = '@0@/wireshark/plugins/@1@'.format(
-  libdir, wireshark_modversion
-)
-  else
-wireshark_prefix = wireshark_dep.get_pkgconfig_variable('prefix')
-if wireshark_prefix == ''
-  # If wireshark's prefix cannot be retrieved from pkg-config,
-  # this is our best bet.
-  wireshark_prefix = '/usr'
-endif
-# Replace wireshark's prefix with our own.
-# There is no replace method in meson so we have to workaround it.
-rc = run_command(
-  'python3', '-c',
-  'print("@0@".replace("@1@", "@2@"))'.format(
-wireshark_plugindir, wireshark_prefix, prefix,
-  ),
-  check: true,
-)
-wireshark_plugindir = rc.stdout().strip()
-  endif
+  wireshark_prefix = wireshark_dep.get_pkgconfig_variable('prefix')
+  if wireshark_prefix == ''
+# If wireshark's prefix cannot be retrieved from pkg-config,
+# this is our best bet.
+wireshark_prefix = '/usr'
+  endif
+  # Replace wireshark's prefix with our own.
+  # There is no replace method in meson so we have to workaround it.
+  rc = run_command(
+'python3', '-c',
+'print("@0@".replace("@1@", "@2@"))'.format(
+  wireshark_plugindir, wireshark_prefix, prefix,
+),
+check: true,
+  )
+  wireshark_plugindir = rc.stdout().strip()
 
   # Since wireshark 2.5.0 plugins can't live in top level plugindir but have
   # to be under one of ["epan", "wiretap", "codecs"] subdir. The first one 
looks okay.
-- 
2.28.0

Re: [PATCH 1/2] virnetdevopenvswitch: Get names for dpdkvhostuserclient too

[John Ferlan]

On 11/11/20 3:38 AM, Michal Privoznik wrote:
> There are two type of vhostuser ports:
> 
>   dpdkvhostuser - OVS creates the socket and QEMU connects to it
>   dpdkvhostuserclient - QEMU creates the socket and OVS connects to it
> 
> But of course ovs-vsctl syntax for fetching ifname is different.
> So far, we've implemented the former. The lack of implementation
> for the latter means that we are not detecting the interface name
> and thus not reporting it in domain XML, or failing to get
> interface statistics.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1767013
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_command.c |  1 +
>  src/qemu/qemu_hotplug.c |  1 +
>  src/util/virnetdevopenvswitch.c | 60 +++--
>  src/util/virnetdevopenvswitch.h |  1 +
>  tests/qemuxml2argvmock.c|  1 +
>  5 files changed, 46 insertions(+), 18 deletions(-)
> 

[...]


> diff --git a/src/util/virnetdevopenvswitch.h b/src/util/virnetdevopenvswitch.h
> index c9ea592058..5cd1d22ae3 100644
> --- a/src/util/virnetdevopenvswitch.h
> +++ b/src/util/virnetdevopenvswitch.h
> @@ -61,6 +61,7 @@ int virNetDevOpenvswitchInterfaceGetMaster(const char 
> *ifname, char **master)
>  ATTRIBUTE_NONNULL(1) G_GNUC_WARN_UNUSED_RESULT;
>  
>  int virNetDevOpenvswitchGetVhostuserIfname(const char *path,
> +   bool server,
> char **ifname)
>  ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT G_GNUC_NO_INLINE;

^^  A coverity build breaker...  s/(2)/(3) will resolve.


John

[libvirt PATCH] docs: compiling.html: pass -d to xz to decompress

[Daniel P . Berrangé]

From: Jonathan Watt 

tar on macOS recognizes XZ compression automatically, but that is
not the case for GNU tar (1.32 at least).  On Fedora 33 the current
instructions result in the following error:

  $ xz -c libvirt-6.9.0.tar.xz | tar xvf -
  tar: Archive is compressed. Use -J option
  tar: Error is not recoverable: exiting now

Reviewed-by: Daniel P. Berrangé 
Signed-off-by: Jonathan Watt 
---

Pushed as a trivial patch from gitlab

 docs/compiling.html.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/compiling.html.in b/docs/compiling.html.in
index c898d16313..c2c9d9f0ed 100644
--- a/docs/compiling.html.in
+++ b/docs/compiling.html.in
@@ -14,7 +14,7 @@
 
 
 
-$ xz -c libvirt-x.x.x.tar.xz | tar xvf -
+$ xz -dc libvirt-x.x.x.tar.xz | tar xvf -
 $ cd libvirt-x.x.x
 $ meson build
 
-- 
2.28.0

Re: [libvirt PATCH] kbase: Shorten "less verbose QEMU logging" example

[Peter Krempa]

On Fri, Nov 13, 2020 at 16:34:25 +0100, Andrea Bolognani wrote:
> Rationale for the changes:
> 
>   * util.dbus produces very little logging, so it doesn't really
> make sense to filter it out;

IIRC it did log a lot before the switch to gdbus, thus I'd prefer to
keep this since it's not that long since we switched to gdbus.

>   * util.udev doesn't exist;

I have no idea where I got this though ...

>   * access can be filtered out entirely, as nothing very
> interesting is produced by the only other component in the
> same package (access.accessdriverpolkit).
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  docs/kbase/debuglogs.rst | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

If you keep dbus filtered out:

Reviewed-by: Peter Krempa 

Re: [libvirt PATCH] kbase: Shorten "less verbose QEMU logging" example

[Ján Tomko]

On a Friday in 2020, Andrea Bolognani wrote:

Rationale for the changes:

 * util.dbus produces very little logging, so it doesn't really
   make sense to filter it out;
 * util.udev doesn't exist;
 * access can be filtered out entirely, as nothing very
   interesting is produced by the only other component in the
   same package (access.accessdriverpolkit).

Signed-off-by: Andrea Bolognani 
---
docs/kbase/debuglogs.rst | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/docs/kbase/debuglogs.rst b/docs/kbase/debuglogs.rst
index 7945cf8bed..fed998efd8 100644
--- a/docs/kbase/debuglogs.rst
+++ b/docs/kbase/debuglogs.rst
@@ -178,13 +178,12 @@ Less verbose logging for QEMU VMs



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 2/6] qemu: conf: Clarify default of "vnc_tls_x509_verify"

[Michal Privoznik]

On 11/13/20 4:45 PM, Daniel P. Berrangé wrote:

On Fri, Nov 13, 2020 at 04:38:06PM +0100, Michal Privoznik wrote:

On 11/13/20 4:01 PM, Peter Krempa wrote:

If both "vnc_tls_x509_verify" and "default_tls_x509_verify" are missing
from the config file the client certificate validation is disabled. VNC
provides a layer of authentication so client certificate validation is
not strictly required.

Signed-off-by: Peter Krempa 
---
   src/qemu/qemu.conf | 3 ++-
   1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index 6f9d940477..f40963ce48 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -119,7 +119,8 @@
   # CA in the vnc_tls_x509_cert_dir (or default_tls_x509_cert_dir).
   #
   # If this option is not supplied, it will be set to the value of
-# "default_tls_x509_verify".
+# "default_tls_x509_verify". If "default_tls_x509_verify" is not supplied 
either


No native speaker, but perhaps s/either/neither/? Applies for next patches
too.


That wouldn't be right - what's there now is fine.


Okay, coming from a language that has double negatives so I'm never sure :-)

Thanks for clarification.

Michal

Re: [PATCH] Added attach-disk parameters for network disk support

[Peter Krempa]

On Thu, Nov 12, 2020 at 15:15:27 -0600, Ryan Gahagan wrote:
> Hey Peter,

Hi Ryan,

firstly I'd like to ask you to keep the conversation on-list. (use
reply-all all the time). This ensures that the response is public and
also other people can jump in and respont. At the very least it can be
used for future reference.

I'll be re-adding the mailing list in this response.

Also please avoid top-posting in techincal lists. It's always best to
respond inline at the proper context that is being discussed.

> Sorry about all the issues with our commit, we're new to this and trying to
> make our first contribution to this repo. We had a couple questions over
> your feedback and wanted to run them by you before making another request.

Sure thing, you can always ask on the mailing list even without patches.

> In the original code, the --source parameter is marked as required but okay
> to be empty. If the string is empty, then we think the parameter read in by
> vshCommandOptStringReq would be NULL, in which case no  tag would
> be generated at all. If this is the case, then why is --source required in
> the first place? We see that --source-name is mutually exclusive witha

That's a quirk of it being a positional argument (VSH_OT_DATA). A
positional argument is mandatory.

> --source, but is it also okay to be empty? For example, in the issue
>  this patch was based on,
> there's an example XML which has a  with neither a file nor a name.
> Is this example incorrect, or is it fine to have neither and only have a
> protocol?

That is correct for example with the NBD protocol where a missing name
actually means the default unnamed export.

The thing is that we can't really change --source to be something else
than VSH_OT_DATA since it would change the semantics of the virsh
command.

You probably will need to use it also for the 'name' attribute and
switch the name according to the presence of the 'protocol'
attribute/option.

> When it comes to making these mutually exclusive, we also were confused on
> what flags to give them. Would it be proper behavior to give them both no
> flags or only the VSH_OFLAG_EMPTY_OK flag and then check that one is
> defined in the cmdAttachDisk method (using VSH_EXCLUSIVE_OPTIONS_VAR) and
> error there instead?

Those are two separate things. If it makes sense for a flag to be
specified but empty and it impacts behaviour then you need to use
VSH_OFLAG_EMPTY_OK.

Any non-positional arguments (those which need --) can be made optional.

On the other hand that does not say anything about how they can or can't
be combined with other options and thus you need to declare that in the
code.

> If the user provides an empty source without any protocol, but provides
> --source-host-* parameters, what should be generated instead? Would this

This should be an error. --source-host must require the use of
--source-protocol

> cause an error, would it generate an in-line  tag, or would it
> generate a completely empty  tag followed by the host information?

The protocol option flag should be mandatory if you want to use the
host/socket.


> 
> -Ryan Gahagan
> 
> On Thu, Nov 12, 2020 at 2:57 AM Peter Krempa  wrote:
> 
> > On Wed, Nov 11, 2020 at 17:00:51 -0600, Ryan Gahagan wrote:
> > > Related issue: https://gitlab.com/libvirt/libvirt/-/issues/16
> > > Added in support for the following parameters in attach-disk:
> > > --source-name
> > > --source-protocol
> > > --source-host-name
> > > --source-host-socket
> > > --source-host-transport

Please trim parts of the message that are no longer relevant.

[...]

Re: [PATCH 4/4] ci: Run test suite on macOS

[Michal Privoznik]

On 11/8/20 10:24 PM, Roman Bolshakov wrote:

There's no need to have different CI process between macOS and FreeBSD
as test suite has been fixed on macOS.

Signed-off-by: Roman Bolshakov 
---
  ci/cirrus/build.yml | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/ci/cirrus/build.yml b/ci/cirrus/build.yml
index 912284b906..dc1cccd135 100644
--- a/ci/cirrus/build.yml
+++ b/ci/cirrus/build.yml
@@ -20,5 +20,4 @@ build_task:
  - git reset --hard "$CI_COMMIT_SHA"
build_script:
  - meson build --prefix=$(pwd)/install-root
-- if test "$(uname)" = "FreeBSD"; then ninja -C build dist; fi
-- if test "$(uname)" = "Darwin"; then ninja -C build && ninja -C build 
install; fi
+- ninja -C build dist



If we go with --timeout-multiplier= as I'm suggesting in 3/4 then this 
can't be ninja, but meson. I'm not sure what the whole point of 'ninja' 
is at this point, sorry.


Michal

Re: [PATCH 2/4] tests: Fix mock chaining on macOS

[Michal Privoznik]

On 11/8/20 10:24 PM, Roman Bolshakov wrote:

Some tests in qemuxml2argvtest need opendir() from virpcimock, others
need opendir() from virfilewrapper.

But as of now, only opendir() from virpcimock has an effect.
real_opendir in virpcimock has a pointer to opendir$INODE64 in
libsystem_kernel.dylib instead of pointing to opendir$INODE64 in
qemuxml2argvtest (from virfilewrapper). And because the second one is
never used, tests that rely on prefixes added by virFileWrapperAddPrefix
fail.

That can be fixed if dlsym(3) is asked explicitly to search symbols in
main executable with RTLD_MAIN_ONLY before going to other dylibs.
Existing RTLD_NEXT handle results into libsystem_kernel.dylib being
searched before main executable.

Signed-off-by: Roman Bolshakov 
---
  tests/virmock.h| 37 +++--
  tests/virpcimock.c |  1 +
  2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/tests/virmock.h b/tests/virmock.h
index dea5feb80f..db051f3636 100644
--- a/tests/virmock.h
+++ b/tests/virmock.h
@@ -284,8 +284,19 @@
  static void (*real_##name)(void); \
  void wrap_##name(void)
  
+#ifdef __APPLE__

+# define VIR_MOCK_REAL_INIT_MAIN(name, alias) \
+do { \
+if (real_##name == NULL) { \
+real_##name = dlsym(RTLD_MAIN_ONLY, alias); \
+} \
+} while (0)
+#else
+# define VIR_MOCK_REAL_INIT_MAIN(name, alias) \
+do {} while (0)
+#endif
  
-#define VIR_MOCK_REAL_INIT(name) \

+#define VIR_MOCK_REAL_INIT_NEXT(name) \
  do { \
  if (real_##name == NULL && \
  !(real_##name = dlsym(RTLD_NEXT, \
@@ -295,7 +306,7 @@
  } \
  } while (0)
  
-#define VIR_MOCK_REAL_INIT_ALIASED(name, alias) \

+#define VIR_MOCK_REAL_INIT_ALIASED_NEXT(name, alias) \
  do { \
  if (real_##name == NULL && \
  !(real_##name = dlsym(RTLD_NEXT, \
@@ -304,3 +315,25 @@
  abort(); \
  } \
  } while (0)
+
+#ifdef VIR_MOCK_LOOKUP_MAIN


I think we can do this even without this macro. If the 
VIR_MOCK_LOOKUP_MAIN macro is not defined then we are still guarded by 
VIR_MOCK_REAL_INIT_MAIN() being a NOP on !__APPLE__. Also ..



+# define VIR_MOCK_REAL_INIT(name) \
+do { \
+VIR_MOCK_REAL_INIT_MAIN(name, #name); \
+VIR_MOCK_REAL_INIT_NEXT(name); \
+} while (0)
+# define VIR_MOCK_REAL_INIT_ALIASED(name, alias) \
+do { \
+VIR_MOCK_REAL_INIT_MAIN(name, alias); \
+VIR_MOCK_REAL_INIT_ALIASED_NEXT(name, alias); \
+} while (0)
+#else
+# define VIR_MOCK_REAL_INIT(name) \
+do { \
+VIR_MOCK_REAL_INIT_NEXT(name); \
+} while (0)
+# define VIR_MOCK_REAL_INIT_ALIASED(name, alias) \
+do { \
+VIR_MOCK_REAL_INIT_ALIASED_NEXT(name, alias); \
+} while (0)
+#endif
diff --git a/tests/virpcimock.c b/tests/virpcimock.c
index 686f894e99..4aa96cae08 100644
--- a/tests/virpcimock.c
+++ b/tests/virpcimock.c
@@ -19,6 +19,7 @@
  #include 
  
  #if defined(__linux__) || defined(__FreeBSD__) || defined(__APPLE__)

+# define VIR_MOCK_LOOKUP_MAIN


.. it feels a bit odd that this is defined for virpcimock only.

Michal

Re: [PATCH 3/4] qemuxml2argvtest: Increase timeout on macOS

[Michal Privoznik]

On 11/8/20 10:24 PM, Roman Bolshakov wrote:

The test takes 100+ seconds if all test suite is run with meson and 45+
seconds if it's run directly. According to the output of sample tool,
most of the time (~72 seconds) is spent in poll():

Sort by top of stack, same collapsed (when >= 5):
 poll  (in libsystem_kernel.dylib) 72396
 tiny_free_no_lock  (in libsystem_malloc.dylib) 1726
 tiny_malloc_should_clear  (in libsystem_malloc.dylib)  1671
 free_tiny  (in libsystem_malloc.dylib) 1287
 tiny_free_list_add_ptr  (in libsystem_malloc.dylib)1258
 tiny_malloc_from_free_list  (in libsystem_malloc.dylib)1256

Closes https://gitlab.com/libvirt/libvirt/-/issues/58
Signed-off-by: Roman Bolshakov 
---
  tests/meson.build | 8 +++-
  1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tests/meson.build b/tests/meson.build
index 8decdfa20c..3b77a211bb 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -571,7 +571,13 @@ foreach data : tests
  ],
  export_dynamic: true,
)
-  test(data['name'], test_bin, env: tests_env)
+  if data['name'] == 'qemuxml2argvtest' and host_machine.system() == 'darwin'
+timeout = 180
+  else
+# default meson timeout
+timeout = 30
+  endif
+  test(data['name'], test_bin, env: tests_env, timeout: timeout)
  endforeach
  
  



I think the last time I wanted to increase the timeout I was told that 
it is machine specific and since I know my machine the best I should 
use: meson test --timeout-multiplier=X


Do you need this for the next cirrus patch? If so I think we should just 
add --timeout-multiplier= there.


Michal

Re: [PATCH 1/4] tests: Fix opendir mocks on macOS

[Michal Privoznik]

On 11/8/20 10:24 PM, Roman Bolshakov wrote:

opendir() mocks need to search for decorated function with $INODE64
suffix, like stat mocks.

Signed-off-by: Roman Bolshakov 
---
  tests/virfilewrapper.c | 4 
  tests/virpcimock.c | 4 
  2 files changed, 8 insertions(+)



Reviewed-by: Michal Privoznik 

Michal

Re: [PATCH v2 00/10] qemu: support renaming domains with snapshots/checkpoints

[Nikolay Shirokovskiy]

On 10.11.2020 10:58, Nikolay Shirokovskiy wrote:
> 
> 
> On 09.11.2020 16:51, Daniel Henrique Barboza wrote:
>>
>>
>> On 11/3/20 8:59 AM, Nikolay Shirokovskiy wrote:
>>> This is basically just rebase of [1] as it was not get any attention at that
>>> time.
>>>
>>> [1] [PATCH 0/8] qemu: support renaming domains with snapshots/checkpoints
>>> https://www.redhat.com/archives/libvir-list/2020-March/msg00018.html
>>
>> Code LGTM:
>>
>> Reviewed-by: Daniel Henrique Barboza 
>>
>>
>> Shouldn't you add some test cases for this new behavior though? I'm a bit
>> nervous with pushing this upstream without any coverage.
>>
>>
> 
> So basically we need to test how qemuDomainRenameCallback works. Currently
> there is no test for this function or virDomainRename. I spent some time
> looking at existing tests that need to mock driver/vm objects and it looks 
> like
> it requires a good deal of effort in order to prepare the test in this case.
> At the same time those tests has many inputs so it looks like worth heavy
> preparation. In case of rename the code we test does not depend greatly on
> inputs so may be it does not worth adding such test given heavy preparation we
> have to do.
> 
> Of course I give the patch series some manual testing.
> 

Thanx for the review!
Pushed with next hunk squashed into the last patch:

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c  

index c831ae6..fef0be6 100644   

--- a/src/qemu/qemu_migration.c 

+++ b/src/qemu/qemu_migration.c 

@@ -5137,7 +5137,7 @@ qemuMigrationDstPersist(virQEMUDriverPtr driver,  

priv->qemuCaps)))   

 goto error;



-if (!oldDef && qemuDomainNamePathsCleanup(cfg, vmdef->name, false) < 0)

+if (!oldPersist && qemuDomainNamePathsCleanup(cfg, vmdef->name, false) < 
0)
 goto error;



 if (virDomainDefSave(vmdef, driver->xmlopt, cfg->configDir) < 0 &&  

Nikolay

Re: [PATCH 2/6] qemu: conf: Clarify default of "vnc_tls_x509_verify"

[Daniel P . Berrangé]

On Fri, Nov 13, 2020 at 04:38:06PM +0100, Michal Privoznik wrote:
> On 11/13/20 4:01 PM, Peter Krempa wrote:
> > If both "vnc_tls_x509_verify" and "default_tls_x509_verify" are missing
> > from the config file the client certificate validation is disabled. VNC
> > provides a layer of authentication so client certificate validation is
> > not strictly required.
> > 
> > Signed-off-by: Peter Krempa 
> > ---
> >   src/qemu/qemu.conf | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> > index 6f9d940477..f40963ce48 100644
> > --- a/src/qemu/qemu.conf
> > +++ b/src/qemu/qemu.conf
> > @@ -119,7 +119,8 @@
> >   # CA in the vnc_tls_x509_cert_dir (or default_tls_x509_cert_dir).
> >   #
> >   # If this option is not supplied, it will be set to the value of
> > -# "default_tls_x509_verify".
> > +# "default_tls_x509_verify". If "default_tls_x509_verify" is not supplied 
> > either
> 
> No native speaker, but perhaps s/either/neither/? Applies for next patches
> too.

That wouldn't be right - what's there now is fine.

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 libvirt v2 03/11] nodedev: detect AP queues

[Jonathon Jongsma]

On Fri, 13 Nov 2020 15:19:11 +0100
Shalini Chellathurai Saroja  wrote:

> On 11/12/20 6:01 PM, Jonathon Jongsma wrote:
> >> diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in
> >> index d10a79e3..45281363 100644
> >> --- a/docs/formatnode.html.in
> >> +++ b/docs/formatnode.html.in
> >> @@ -439,6 +439,17 @@
> >> AP Card identifier.
> >>   
> >> 
> >> +  ap_queue
> >> +  Describes the AP Queue on a S390 host. An AP Queue
> >> is
> >> +  identified by it's ap-adapter and ap-domain id.  
> > "it's" should be "its". Is it worth a very brief description of
> > what an AP queue actually is?  
> 
> Sure, I will provide a brief description on AP queue.
> 
> ...
> 
> >  
> >>   
> >> +  
> >> +
> >> +  ap_queue
> >> +
> >> +
> >> +  
> >> +
> >> +
> >> +  
> >> +
> >> +
> > Let's use double quotes to keep the file consistent.  
> Sure:-)
> >  
> >> +
> >> 
> >>   
> >> 
> >> @@ -738,4 +751,16 @@
> >>   
> >> 
> >>   
> >> +  
> >> +
> >> +  
> >> +(0x)?[0-9a-fA-F]{1,4}
> >> +  
> >> +  
> >> +0
> >> +255  
> > Is 255 correct here? the hex pattern above implies that it is a 16
> > bit value, but here you're limiting it to 255. If it is a 16 bit
> > value, can we just use the already-defined 'uint16' type from
> > basictypes.rng?  
> 
> Yes, it is. Current architecture limit is 256 (0-255) for domains.
> 
> The address schema of domains was created in linux on z, with a
> future architectural change in mind.
> 
> ...


So, this is a bit confusing to me. The xml schema appears to be
trying to prevent me from specifying a value above 255 if I enter
it as an integer, yet it allows me to specify a value of 0x
if I specify it as a hex value. 

Side note: I can also enter plain integers beyond 255 because of the
fact that the '0x' prefix is optional.  will validate according to
the schema because it matches the hex string pattern. 

However I note that your parsing code does not enforce this 255 limit
anywhere.

> 
> >  
> >> diff --git a/src/node_device/node_device_udev.c
> >> b/src/node_device/node_device_udev.c index b4eb4553..6bbff571
> >> 100644 --- a/src/node_device/node_device_udev.c
> >> +++ b/src/node_device/node_device_udev.c
> >> @@ -1218,6 +1218,29 @@ udevProcessAPCard(struct udev_device
> >> *device, }
> >>   
> >>   
> >> +static int
> >> +udevProcessAPQueue(struct udev_device *device,
> >> +   virNodeDeviceDefPtr def)
> >> +{
> >> +char *c;
> >> +virNodeDevCapDataPtr data = >caps->data;
> >> +  
> > In the previous patch, you added a comment explaining the format of
> > the sysfs path. I found that helpful. Without knowing the format,
> > it's a bit difficult to judge whether this code below is correct.
> >  
> I will add the below comment
> 
> /* The sysfs path would be in the format /sys/bus/ap/devices/xx., 
> where xx is ap adapter id and  is ap domain id. 
> eg:/sys/bus/ap/devices/08.0001 */
> 

Re: [PATCH 2/6] qemu: conf: Clarify default of "vnc_tls_x509_verify"

[Ján Tomko]

On a Friday in 2020, Michal Privoznik wrote:

On 11/13/20 4:01 PM, Peter Krempa wrote:

If both "vnc_tls_x509_verify" and "default_tls_x509_verify" are missing
from the config file the client certificate validation is disabled. VNC
provides a layer of authentication so client certificate validation is
not strictly required.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu.conf | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index 6f9d940477..f40963ce48 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -119,7 +119,8 @@
 # CA in the vnc_tls_x509_cert_dir (or default_tls_x509_cert_dir).
 #
 # If this option is not supplied, it will be set to the value of
-# "default_tls_x509_verify".
+# "default_tls_x509_verify". If "default_tls_x509_verify" is not supplied 
either


No native speaker, but perhaps s/either/neither/? Applies for next 
patches too.




Not a native speaker either, but your suggested substitution sounds
weirder.

Jano


+# the default is "0".
 #
 #vnc_tls_x509_verify = 1



Michal



signature.asc
Description: PGP signature

Re: [PATCH] viridentitytest: Run more frequently

[Ján Tomko]

On a Monday in 2020, Michal Privoznik wrote:

The viridentitytest tests our viridentity module which is
compiled on all platforms and OSes. There is no need to have
SELinux secdriver as individual test cases are skipped if SELinux
is missing.

Signed-off-by: Michal Privoznik 
---

Successfully build here:

https://gitlab.com/MichalPrivoznik/libvirt/-/pipelines/213586713

While technically this is a v2 of:

https://www.redhat.com/archives/libvir-list/2020-November/msg00308.html

it's implementing completely different approach and thus I did not mark
it as v2.

tests/meson.build   | 2 +-
tests/viridentitytest.c | 3 +--
2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/tests/meson.build b/tests/meson.build
index 8decdfa20c..9f0051303d 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -312,6 +312,7 @@ tests += [
  { 'name': 'virfirewalltest' },
  { 'name': 'virhostcputest', 'link_whole': [ test_file_wrapper_lib ] },
  { 'name': 'virhostdevtest' },
+  { 'name': 'viridentitytest' },


This seems to drop the check for WITH_ATTR which was added in:
commit cdc5f3f1a3a960cbc988ad6a21078f135c936457
tests: build viridentitytest only WITH_ATTR.

Was it ever needed?


  { 'name': 'viriscsitest' },
  { 'name': 'virkeycodetest' },
  { 'name': 'virkmodtest' },


Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 1/6] qemu: conf: Allow individual control of default value for *_tls_x509_verify

[Michal Privoznik]

On 11/13/20 4:01 PM, Peter Krempa wrote:

Store whether "default_tls_x509_verify" was provided and enhance the
SET_TLS_VERIFY_DEFAULT macro so that indiviual users can provide their
own default if "default_tls_x509_verify" config option was not provided.

For now we keep setting it to 'false'.

Signed-off-by: Peter Krempa 
---
  src/qemu/qemu.conf   |  6 ++
  src/qemu/qemu_conf.c | 22 ++
  src/qemu/qemu_conf.h |  1 +
  3 files changed, 21 insertions(+), 8 deletions(-)




diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 2fb2f021c2..c3a61816a4 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -406,8 +406,10 @@ 
virQEMUDriverConfigLoadDefaultTLSEntry(virQEMUDriverConfigPtr cfg,
  if ((rv = virConfGetValueString(conf, "default_tls_x509_cert_dir", 
>defaultTLSx509certdir)) < 0)
  return -1;
  cfg->defaultTLSx509certdirPresent = (rv == 1);
-if (virConfGetValueBool(conf, "default_tls_x509_verify", 
>defaultTLSx509verify) < 0)
+if ((rv = virConfGetValueBool(conf, "default_tls_x509_verify", 
>defaultTLSx509verify)) < 0)
  return -1;
+if (rv == 1)
+cfg->defaultTLSx509verifyPresent = true;
  if (virConfGetValueString(conf, "default_tls_x509_secret_uuid",
>defaultTLSx509secretUUID) < 0)
  return -1;
@@ -1240,16 +1242,20 @@ virQEMUDriverConfigSetDefaults(virQEMUDriverConfigPtr 
cfg)

  #undef SET_TLS_X509_CERT_DEFAULT

-#define SET_TLS_VERIFY_DEFAULT(val) \
+#define SET_TLS_VERIFY_DEFAULT(val, defaultverify) \
  do { \
-if (!cfg->val## TLSx509verifyPresent) \
-cfg->val## TLSx509verify = cfg->defaultTLSx509verify; \
+if (!cfg->val## TLSx509verifyPresent) {\
+if (cfg->defaultTLSx509verifyPresent) \
+  cfg->val## TLSx509verify = cfg->defaultTLSx509verify; \
+else \
+cfg->val## TLSx509verify = defaultverify;\


Alignment.


+}\
  } while (0)



Michal

Re: [PATCH 0/6] qemu: Enable client TLS certificate validation by default for chardev, migration, and backup servers

[Michal Privoznik]

On 11/13/20 4:01 PM, Peter Krempa wrote:

See patches 6 for a explanation.

Peter Krempa (6):
   qemu: conf: Allow individual control of default value for
 *_tls_x509_verify
   qemu: conf: Clarify default of "vnc_tls_x509_verify"
   qemu: conf: Enable 'chardev_tls_x509_verify' by default
   qemu: conf: Enable 'migrate_tls_x509_verify' by default
   qemu: conf: Enable 'backup_tls_x509_verify' by default
   NEWS: Mention change of default for TLS certificate verification

  NEWS.rst | 11 +++
  src/qemu/qemu.conf   | 18 ++
  src/qemu/qemu_conf.c | 22 ++
  src/qemu/qemu_conf.h |  1 +
  4 files changed, 40 insertions(+), 12 deletions(-)



Reviewed-by: Michal Privoznik 

Michal

Re: [PATCH 0/9] docs: kbase style change and cleanups

[Michal Privoznik]

On 11/4/20 6:01 PM, Peter Krempa wrote:

Some patches are taken from an older series where we've discussed that
the kbase page should look more like 'docs.html' as more articles
appear, this series delivers that:

https://www.redhat.com/archives/libvir-list/2020-August/msg00172.html

... and a few other changes.

Final output:

https://pipo.sk.gitlab.io/-/libvirt/-/jobs/830387801/artifacts/website/docs.html

Peter Krempa (9):
   docs: Fix title of 'docs' page
   docs: xslt: Use 'Link' rather than 'Permalink' in header links
   docs: css: Add a gray box around table of contents of RST based docs
   docs: kbase: Move index page to docs/kbase
   docs: kbase: Remove extra container from index page
   docs: kbase: Split articles into sections
   docs: xsl: Unify stylability of main container element
   docs: css: Modify appearance of the kbase directory page
   docs: kbase: Reorder some articles in the 'Usage' section

  docs/docs.html.in  |  5 ++--
  docs/index.html.in |  4 +--
  docs/kbase.rst | 53 -
  docs/kbase/index.rst   | 57 +++
  docs/kbase/meson.build |  1 +
  docs/libvirt.css   | 67 +++---
  docs/meson.build   |  1 -
  docs/page.xsl  | 27 ++---
  scripts/hvsupport.py   |  2 +-
  9 files changed, 124 insertions(+), 93 deletions(-)
  delete mode 100644 docs/kbase.rst
  create mode 100644 docs/kbase/index.rst



Reviewed-by: Michal Privoznik 

Michal

[libvirt PATCH] kbase: Shorten "less verbose QEMU logging" example

[Andrea Bolognani]

Rationale for the changes:

  * util.dbus produces very little logging, so it doesn't really
make sense to filter it out;
  * util.udev doesn't exist;
  * access can be filtered out entirely, as nothing very
interesting is produced by the only other component in the
same package (access.accessdriverpolkit).

Signed-off-by: Andrea Bolognani 
---
 docs/kbase/debuglogs.rst | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/docs/kbase/debuglogs.rst b/docs/kbase/debuglogs.rst
index 7945cf8bed..fed998efd8 100644
--- a/docs/kbase/debuglogs.rst
+++ b/docs/kbase/debuglogs.rst
@@ -178,13 +178,12 @@ Less verbose logging for QEMU VMs
 
 Some subsystems are very noisy and usually not the culprit of the problems. 
They
 can be silenced individually for a less verbose log while still logging
-everything else. Usual suspects are the JSON code, udev, authentication and 
such.
+everything else. Usual suspects are the JSON code, RPC, authentication and 
such.
 A permissive filter is good for development use cases.
 
 ::
 
-3:remote 4:event 3:util.json 3:util.object 3:util.dbus 3:util.udev 
3:node_device 3:rpc 3:access.accessmanager 3:util.netlink 1:*
-
+3:remote 4:event 3:util.json 3:util.object 3:util.netlink 3:node_device 
3:rpc 3:access 1:*
 
 Minimalistic QEMU QMP monitor logging
 ~
-- 
2.26.2

Re: [PATCH 2/6] qemu: conf: Clarify default of "vnc_tls_x509_verify"

[Michal Privoznik]

On 11/13/20 4:01 PM, Peter Krempa wrote:

If both "vnc_tls_x509_verify" and "default_tls_x509_verify" are missing
from the config file the client certificate validation is disabled. VNC
provides a layer of authentication so client certificate validation is
not strictly required.

Signed-off-by: Peter Krempa 
---
  src/qemu/qemu.conf | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index 6f9d940477..f40963ce48 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -119,7 +119,8 @@
  # CA in the vnc_tls_x509_cert_dir (or default_tls_x509_cert_dir).
  #
  # If this option is not supplied, it will be set to the value of
-# "default_tls_x509_verify".
+# "default_tls_x509_verify". If "default_tls_x509_verify" is not supplied 
either


No native speaker, but perhaps s/either/neither/? Applies for next 
patches too.



+# the default is "0".
  #
  #vnc_tls_x509_verify = 1



Michal

Re: [PATCH libvirt v2 01/11] nodedev: detect AP card device

[Jonathon Jongsma]

On Fri, 13 Nov 2020 12:09:26 +0100
Shalini Chellathurai Saroja  wrote:

> Hi Jonathon,
> 
> Thank you for the quick review:-)
> 
> On 11/12/20 5:27 PM, Jonathon Jongsma wrote:
> >> +  
> >> +
> >> +  
> >> +  
> >> +  
> >> +   >> name='hexuint'/>  
> > It seems that you're unnecessarily changing double-quotes to
> > single-quotes here, which adds spurious changes to the diff. The
> > rest of the file uses double-quotes. Let's stick with that.  
> Sure.
> >  
> >>   
> >> 
> >>   
> >> @@ -716,4 +726,16 @@
> >>   
> >> 
> >>   
> >> +  
> >> +
> >> +  
> >> +(0x)?[0-9a-fA-F]{1,2}
> >> +  
> >> +  
> >> +0
> >> +255
> >> +  
> >> +
> >> +
> > As far as I can tell, this is identical to the definition of the
> > 'uint8' type in basictypes.rng. Is there a reason for defining a
> > different type?
> >  
> Yes, In definition apAdapterRange, the prefix '0x' is optional.
> 

Oh, I guess I missed that part. Now that I look at basictypes.rng, I
see that the '0x' is required for uint8 and uint24, but it is optional
for uint16 and uint32. Does anybody know the justification for these
differences?

That said, I don't believe that your parsing code actually supports an
optional '0x' prefix. In virNodeDevCapAPCardParseXML(), you call

virStrToLong_uip(adapter, NULL, 0, _card->ap_adapter)

But I'm quite sure that passing a value of e.g. 'ff' for adapter will
result in a parsing failure. Try changing the ap-adapter value in
tests/nodedevschemadata/ap_card07.xml to some different values and see
what happens.

Jonathon

Re: [PATCH 0/3] kbase: Port 'debuglogs' from wiki and convert directory to rst

[Andrea Bolognani]

On Fri, 2020-11-13 at 15:14 +0100, Ján Tomko wrote:
> On a Friday in 2020, Andrea Bolognani wrote:
> > Can someone with write access to the wiki please turn it into a short
> > stub that points people to the kbase article[2] instead?
> 
> Done. Hope it's short enough for your taste.

It definitely does the trick :) Thank you!

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH v1 00/10] more simple g_autoptr() cleanups

[Daniel Henrique Barboza]

On 11/12/20 7:21 PM, Jonathon Jongsma wrote:

On Thu, 12 Nov 2020 18:48:30 -0300
Daniel Henrique Barboza  wrote:


I intended to clean up the virDomainDefFree() calls that I
kept seeing in qemu_driver.c, ended up doing a bit more than
that.

Daniel Henrique Barboza (10):
   qemu_driver.c: use g_autoptr() with virDomainDef pointers
   qemu_driver.c: use g_autoptr() with qemuMigrationParams pointers
   qemu_driver.c: use g_autoptr() with virDomainDeviceDefPtr
   qemu_driver.c: remove unneeded 'cleanup' labels
   qemu_snapshot.c: remove uneeded 'cleanup' label in
 qemuSnapshotDelete()
   qemu_domain.c: use g_autoptr() with virDomainDef pointers
   qemu_domain.c: remove unneeded cleanup labels
   qemu_domain.c: modernize qemuDomainFixupCPUs()
   qemu_domain.c: modernize qemuDomainWriteMasterKeyFile()
   qemu_domain.c: modernize qemuMonitorGetCpuHalted()

  src/qemu/qemu_domain.c   | 100 ++
  src/qemu/qemu_driver.c   | 219
+++ src/qemu/qemu_snapshot.c |
3 +- 3 files changed, 119 insertions(+), 203 deletions(-)



Reviewed-by: Jonathon Jongsma 



Pushed. Thanks!


DHB

Re: [PATCH 0/4] qemu: hotplug: Fix check whether controler is used

[Ján Tomko]

On a Friday in 2020, Peter Krempa wrote:

Patch 2/4 is the actual fix, other patches are additional cleanups.

Peter Krempa (4):
 qemuDomain(Disk)ControllerIsBusy: Fix function header format
 qemuDomainDiskControllerIsBusy: Fix logic of matching disk bus to
   controller type
 qemuDomainDiskControllerIsBusy: Optimize checking for SCSI hostdevs
 qemuDomainControllerIsBusy: Fully populate switch statement

src/qemu/qemu_hotplug.c | 92 ++---
1 file changed, 69 insertions(+), 23 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH] qemu: backup: Install bitmap for incremental backup to appropriate node only

[Peter Krempa]

On Fri, Nov 13, 2020 at 09:07:24 -0600, Eric Blake wrote:
> On 11/13/20 8:55 AM, Peter Krempa wrote:
> > Libvirt's backup code has two modes:
> > 
> > 1) push - where qemu actively writes the difference since the checkpoint
> >   into the output file
> > 
> > 2) pull - where we instruct qemu to expose a frozen disk state along
> >   with a bitmap of blocks which changed since the checkpoint
> > 
> > For push mode qemu needs the temporary bitmap we use where we calculate
> > the actual changes to be present on the block node backing the disk.
> > 
> > For pull mode where we expose the bitmap via NBD qemu actually wants the
> > bitmap to be present for the exported block node which is the scratch
> > file.
> > 
> > Until now we've calculated the bitmap twice and installed it both to the
> > scratch file and to the disk node, but we don't need to since we know
> > when it's needed.
> > 
> > Pass in the 'pull' flag and decide where to install the bitmap according
> > to it and also when to register the bitmap name with the blockjob.
> > 
> > Signed-off-by: Peter Krempa 
> > ---
> >  src/qemu/qemu_backup.c | 42 ++
> >  1 file changed, 26 insertions(+), 16 deletions(-)
> 
> Reviewed-by: Eric Blake 
> 
> 
> > +/* For pull-mode backup, we need the bitmap to be present in the 
> > scratch
> > + * file as that will be exported. For push-mode backup the bitmap is
> > + * actually required on top of the image backing the disk */
> > 
> > -if (qemuBackupDiskPrepareOneBitmapsChain(dd->domdisk->src,
> > - dd->store,
> > - dd->incrementalBitmap,
> > - dd->backupdisk->incremental,
> > - actions,
> > - blockNamedNodeData) < 0)
> > -return -1;
> > +if (pull) {
> > +if (qemuBackupDiskPrepareOneBitmapsChain(dd->domdisk->src,
> > + dd->store,
> > + dd->incrementalBitmap,
> > + 
> > dd->backupdisk->incremental,
> > + actions,
> > + blockNamedNodeData) < 0)
> 
> Technically, with both qemu 5.0 and qemu 5.2, the bitmap can live in the
> backing store for BOTH modes, (in 5.2 and beyond, because qemu will
> chase through the backing chain to find the named bitmap if it is not
> present in the temporary store; in 5.0 because the temporary store is
> not a filter node).  But since qemu 5.1 used a filter node for the
> temporary store but was unable to chase through filter nodes, always
> placing the bitmap in the temporary node for pull mode is fine.

I can consider just putting it in one place. Since incremental backup in
libvirt requires 'blockdev-reopen' and that was not made stable yet in
qemu, libvirt will officially support incremental backups only with
qemu-6.0 at soonest.

Re: [PATCH 5/6] qemu: conf: Enable 'backup_tls_x509_verify' by default

[Eric Blake]

On 11/13/20 9:01 AM, Peter Krempa wrote:
> The NBD server used to export pull-mode backups doesn't have any other
> form of client authentication on top of the TLS transport, so the only
> way to authenticate clients is to verify their certificate.
> 
> Enable this option by defauilt when both 'backup_tls_x509_verify' and
> 'default_tls_x509_verify' were not configured.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1879477
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu.conf   | 3 ++-
>  src/qemu/qemu_conf.c | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> index d621dad53b..cc46a34ae2 100644
> --- a/src/qemu/qemu.conf
> +++ b/src/qemu/qemu.conf
> @@ -422,7 +422,8 @@
>  # CA in the backup_tls_x509_cert_dir (or default_tls_x509_cert_dir).
>  #
>  # If this option is not supplied, it will be set to the value of
> -# "default_tls_x509_verify".
> +# "default_tls_x509_verify". If "default_tls_x509_verify" is not supplied 
> either
> +# the default is "1".

s/either/either,/

Reviewed-by: Eric Blake 

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

Re: [PATCH 4/6] qemu: conf: Enable 'migrate_tls_x509_verify' by default

[Eric Blake]

On 11/13/20 9:01 AM, Peter Krempa wrote:
> The migration stream connection and also the NBD server for non-shared
> storage migration don't have any other form of client authentication on
> top of the TLS transport, so the only way to authenticate clients is to
> verify their certificate.
> 
> Enable this option by defauilt when both 'migrate_tls_x509_verify' and
> 'default_tls_x509_verify' were not configured.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1879477
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu.conf   | 3 ++-
>  src/qemu/qemu_conf.c | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> index 8a1a50d664..d621dad53b 100644
> --- a/src/qemu/qemu.conf
> +++ b/src/qemu/qemu.conf
> @@ -385,7 +385,8 @@
>  # CA in the migrate_tls_x509_cert_dir (or default_tls_x509_cert_dir).
>  #
>  # If this option is not supplied, it will be set to the value of
> -# "default_tls_x509_verify".
> +# "default_tls_x509_verify". If "default_tls_x509_verify" is not supplied
> +# either the default is "1".

s/either/either,/

Reviewed-by: Eric Blake 

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

Re: [PATCH] qemu: backup: Install bitmap for incremental backup to appropriate node only

[Eric Blake]

On 11/13/20 8:55 AM, Peter Krempa wrote:
> Libvirt's backup code has two modes:
> 
> 1) push - where qemu actively writes the difference since the checkpoint
>   into the output file
> 
> 2) pull - where we instruct qemu to expose a frozen disk state along
>   with a bitmap of blocks which changed since the checkpoint
> 
> For push mode qemu needs the temporary bitmap we use where we calculate
> the actual changes to be present on the block node backing the disk.
> 
> For pull mode where we expose the bitmap via NBD qemu actually wants the
> bitmap to be present for the exported block node which is the scratch
> file.
> 
> Until now we've calculated the bitmap twice and installed it both to the
> scratch file and to the disk node, but we don't need to since we know
> when it's needed.
> 
> Pass in the 'pull' flag and decide where to install the bitmap according
> to it and also when to register the bitmap name with the blockjob.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_backup.c | 42 ++
>  1 file changed, 26 insertions(+), 16 deletions(-)

Reviewed-by: Eric Blake 


> +/* For pull-mode backup, we need the bitmap to be present in the scratch
> + * file as that will be exported. For push-mode backup the bitmap is
> + * actually required on top of the image backing the disk */
> 
> -if (qemuBackupDiskPrepareOneBitmapsChain(dd->domdisk->src,
> - dd->store,
> - dd->incrementalBitmap,
> - dd->backupdisk->incremental,
> - actions,
> - blockNamedNodeData) < 0)
> -return -1;
> +if (pull) {
> +if (qemuBackupDiskPrepareOneBitmapsChain(dd->domdisk->src,
> + dd->store,
> + dd->incrementalBitmap,
> + dd->backupdisk->incremental,
> + actions,
> + blockNamedNodeData) < 0)

Technically, with both qemu 5.0 and qemu 5.2, the bitmap can live in the
backing store for BOTH modes, (in 5.2 and beyond, because qemu will
chase through the backing chain to find the named bitmap if it is not
present in the temporary store; in 5.0 because the temporary store is
not a filter node).  But since qemu 5.1 used a filter node for the
temporary store but was unable to chase through filter nodes, always
placing the bitmap in the temporary node for pull mode is fine.

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

Re: [PATCH 0/9] docs: kbase style change and cleanups

[Peter Krempa]

On Wed, Nov 04, 2020 at 18:01:52 +0100, Peter Krempa wrote:
> Some patches are taken from an older series where we've discussed that
> the kbase page should look more like 'docs.html' as more articles
> appear, this series delivers that:
> 
> https://www.redhat.com/archives/libvir-list/2020-August/msg00172.html
> 
> ... and a few other changes.
> 
> Final output:
> 
> https://pipo.sk.gitlab.io/-/libvirt/-/jobs/830387801/artifacts/website/docs.html
> 
> Peter Krempa (9):
>   docs: Fix title of 'docs' page
>   docs: xslt: Use 'Link' rather than 'Permalink' in header links
>   docs: css: Add a gray box around table of contents of RST based docs
>   docs: kbase: Move index page to docs/kbase
>   docs: kbase: Remove extra container from index page
>   docs: kbase: Split articles into sections
>   docs: xsl: Unify stylability of main container element
>   docs: css: Modify appearance of the kbase directory page
>   docs: kbase: Reorder some articles in the 'Usage' section

Ping?

[PATCH 5/6] qemu: conf: Enable 'backup_tls_x509_verify' by default

[Peter Krempa]

The NBD server used to export pull-mode backups doesn't have any other
form of client authentication on top of the TLS transport, so the only
way to authenticate clients is to verify their certificate.

Enable this option by defauilt when both 'backup_tls_x509_verify' and
'default_tls_x509_verify' were not configured.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1879477
Signed-off-by: Peter Krempa 
---
 src/qemu/qemu.conf   | 3 ++-
 src/qemu/qemu_conf.c | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index d621dad53b..cc46a34ae2 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -422,7 +422,8 @@
 # CA in the backup_tls_x509_cert_dir (or default_tls_x509_cert_dir).
 #
 # If this option is not supplied, it will be set to the value of
-# "default_tls_x509_verify".
+# "default_tls_x509_verify". If "default_tls_x509_verify" is not supplied 
either
+# the default is "1".
 #
 #backup_tls_x509_verify = 1

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 6f74766607..8ae7c682cb 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1255,7 +1255,7 @@ virQEMUDriverConfigSetDefaults(virQEMUDriverConfigPtr cfg)
 SET_TLS_VERIFY_DEFAULT(vnc, false);
 SET_TLS_VERIFY_DEFAULT(chardev, true);
 SET_TLS_VERIFY_DEFAULT(migrate, true);
-SET_TLS_VERIFY_DEFAULT(backup, false);
+SET_TLS_VERIFY_DEFAULT(backup, true);

 #undef SET_TLS_VERIFY_DEFAULT

-- 
2.28.0

Re: [PATCH] migration.html: Fix the spelling of the --persistent parameter

[Ján Tomko]

On a Friday in 2020, Thomas Huth wrote:

"--persist" is missing the "ent" at the end.

Signed-off-by: Thomas Huth 
---
Sorry, I just noticed this after my previous "--undefinesource"
patch had been merged - otherwise I had sent both fixes in one
patch together...


No need to:
a) be sorry. I can't see anything helpful coming out of that feeling
b) cc me - I watch libvir-list as often as my inbox
c) squashing them together if they fix different issues



docs/migration.html.in | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/migration.html.in b/docs/migration.html.in
index 194cf7d209..77731eeb37 100644
--- a/docs/migration.html.in
+++ b/docs/migration.html.in
@@ -253,7 +253,7 @@
  migration by default. The virsh command has two flags to
  influence this behaviour. The  --undefinesource flag
  will cause the configuration file to be removed on the source host
-  after a successful migration. The --persist flag will
+  after a successful migration. The --persistent flag will
  cause a configuration file to be created on the destination host
  after a successful migration. The following table summarizes the
  configuration file handling in all possible state and flag
@@ -272,7 +272,7 @@
  Source config
  Dest config
  --undefinesource
-  --persist
+  --persistent
  Dest type
  Source config
  Dest config


Reviewed-by: Ján Tomko 
and pushed.

Jano


signature.asc
Description: PGP signature

[PATCH 3/6] qemu: conf: Enable 'chardev_tls_x509_verify' by default

[Peter Krempa]

Chardevs don't have any other form of client authentication on top of
the TLS transport, so the only way to authenticate clients is to verify
their certificate.

Enable this option by defauilt when both 'chardev_tls_x509_verify' and
'default_tls_x509_verify' were not configured.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1879477
Signed-off-by: Peter Krempa 
---
 src/qemu/qemu.conf   | 3 ++-
 src/qemu/qemu_conf.c | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index f40963ce48..8a1a50d664 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -258,7 +258,8 @@
 # CA in the chardev_tls_x509_cert_dir (or default_tls_x509_cert_dir).
 #
 # If this option is not supplied, it will be set to the value of
-# "default_tls_x509_verify".
+# "default_tls_x509_verify". If "default_tls_x509_verify" is not supplied 
either
+# the default is "1".
 #
 #chardev_tls_x509_verify = 1

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index c3a61816a4..e8bad33a40 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1253,7 +1253,7 @@ virQEMUDriverConfigSetDefaults(virQEMUDriverConfigPtr cfg)
 } while (0)

 SET_TLS_VERIFY_DEFAULT(vnc, false);
-SET_TLS_VERIFY_DEFAULT(chardev, false);
+SET_TLS_VERIFY_DEFAULT(chardev, true);
 SET_TLS_VERIFY_DEFAULT(migrate, false);
 SET_TLS_VERIFY_DEFAULT(backup, false);

-- 
2.28.0

[PATCH 2/6] qemu: conf: Clarify default of "vnc_tls_x509_verify"

[Peter Krempa]

If both "vnc_tls_x509_verify" and "default_tls_x509_verify" are missing
from the config file the client certificate validation is disabled. VNC
provides a layer of authentication so client certificate validation is
not strictly required.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu.conf | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index 6f9d940477..f40963ce48 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -119,7 +119,8 @@
 # CA in the vnc_tls_x509_cert_dir (or default_tls_x509_cert_dir).
 #
 # If this option is not supplied, it will be set to the value of
-# "default_tls_x509_verify".
+# "default_tls_x509_verify". If "default_tls_x509_verify" is not supplied 
either
+# the default is "0".
 #
 #vnc_tls_x509_verify = 1

-- 
2.28.0

[PATCH 4/6] qemu: conf: Enable 'migrate_tls_x509_verify' by default

[Peter Krempa]

The migration stream connection and also the NBD server for non-shared
storage migration don't have any other form of client authentication on
top of the TLS transport, so the only way to authenticate clients is to
verify their certificate.

Enable this option by defauilt when both 'migrate_tls_x509_verify' and
'default_tls_x509_verify' were not configured.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1879477
Signed-off-by: Peter Krempa 
---
 src/qemu/qemu.conf   | 3 ++-
 src/qemu/qemu_conf.c | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index 8a1a50d664..d621dad53b 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -385,7 +385,8 @@
 # CA in the migrate_tls_x509_cert_dir (or default_tls_x509_cert_dir).
 #
 # If this option is not supplied, it will be set to the value of
-# "default_tls_x509_verify".
+# "default_tls_x509_verify". If "default_tls_x509_verify" is not supplied
+# either the default is "1".
 #
 #migrate_tls_x509_verify = 1

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index e8bad33a40..6f74766607 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1254,7 +1254,7 @@ virQEMUDriverConfigSetDefaults(virQEMUDriverConfigPtr cfg)

 SET_TLS_VERIFY_DEFAULT(vnc, false);
 SET_TLS_VERIFY_DEFAULT(chardev, true);
-SET_TLS_VERIFY_DEFAULT(migrate, false);
+SET_TLS_VERIFY_DEFAULT(migrate, true);
 SET_TLS_VERIFY_DEFAULT(backup, false);

 #undef SET_TLS_VERIFY_DEFAULT
-- 
2.28.0

[PATCH 6/6] NEWS: Mention change of default for TLS certificate verification

[Peter Krempa]

Signed-off-by: Peter Krempa 
---
 NEWS.rst | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/NEWS.rst b/NEWS.rst
index 3fd3ce4cb9..6fcfd4e26b 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -11,6 +11,17 @@ For a more fine-grained view, use the `git log`_.
 v6.10.0 (unreleased)
 

+* **Security**
+
+  * qemu: Enable client TLS certificate validation by default for ``chardev``,
+``migration``, and ``backup`` servers.
+
+  The default value if qemu.conf options ``chardev_tls_x509_verify``,
+  ``migrate_tls_x509_verify``, or  ``backup_tls_x509_verify`` are not specified
+  explicitly in the config file and also the ``default_tls_x509_verify`` config
+  option is missing are now '1'. This ensures that only legitimate clients
+  access servers, which don't have any additional form of authentication.
+
 * **New features**

   * hyperv: implement new APIs
-- 
2.28.0

[PATCH 1/6] qemu: conf: Allow individual control of default value for *_tls_x509_verify

[Peter Krempa]

Store whether "default_tls_x509_verify" was provided and enhance the
SET_TLS_VERIFY_DEFAULT macro so that indiviual users can provide their
own default if "default_tls_x509_verify" config option was not provided.

For now we keep setting it to 'false'.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu.conf   |  6 ++
 src/qemu/qemu_conf.c | 22 ++
 src/qemu/qemu_conf.h |  1 +
 3 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index 6f7d2b14f7..6f9d940477 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -40,6 +40,12 @@
 #  client-cert.pem - the client certificate signed with the ca-cert.pem
 #  client-key.pem - the client private key
 #
+# If this option is supplied it provides the default for the "_verify" option
+# of specific TLS users such as vnc, backups, migration, etc. The specific
+# users of TLS may override this by setting the specific "_verify" option.
+#
+# When not supplied the specific TLS users provide their own defaults.
+#
 #default_tls_x509_verify = 1

 #
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 2fb2f021c2..c3a61816a4 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -406,8 +406,10 @@ 
virQEMUDriverConfigLoadDefaultTLSEntry(virQEMUDriverConfigPtr cfg,
 if ((rv = virConfGetValueString(conf, "default_tls_x509_cert_dir", 
>defaultTLSx509certdir)) < 0)
 return -1;
 cfg->defaultTLSx509certdirPresent = (rv == 1);
-if (virConfGetValueBool(conf, "default_tls_x509_verify", 
>defaultTLSx509verify) < 0)
+if ((rv = virConfGetValueBool(conf, "default_tls_x509_verify", 
>defaultTLSx509verify)) < 0)
 return -1;
+if (rv == 1)
+cfg->defaultTLSx509verifyPresent = true;
 if (virConfGetValueString(conf, "default_tls_x509_secret_uuid",
   >defaultTLSx509secretUUID) < 0)
 return -1;
@@ -1240,16 +1242,20 @@ virQEMUDriverConfigSetDefaults(virQEMUDriverConfigPtr 
cfg)

 #undef SET_TLS_X509_CERT_DEFAULT

-#define SET_TLS_VERIFY_DEFAULT(val) \
+#define SET_TLS_VERIFY_DEFAULT(val, defaultverify) \
 do { \
-if (!cfg->val## TLSx509verifyPresent) \
-cfg->val## TLSx509verify = cfg->defaultTLSx509verify; \
+if (!cfg->val## TLSx509verifyPresent) {\
+if (cfg->defaultTLSx509verifyPresent) \
+  cfg->val## TLSx509verify = cfg->defaultTLSx509verify; \
+else \
+cfg->val## TLSx509verify = defaultverify;\
+}\
 } while (0)

-SET_TLS_VERIFY_DEFAULT(vnc);
-SET_TLS_VERIFY_DEFAULT(chardev);
-SET_TLS_VERIFY_DEFAULT(migrate);
-SET_TLS_VERIFY_DEFAULT(backup);
+SET_TLS_VERIFY_DEFAULT(vnc, false);
+SET_TLS_VERIFY_DEFAULT(chardev, false);
+SET_TLS_VERIFY_DEFAULT(migrate, false);
+SET_TLS_VERIFY_DEFAULT(backup, false);

 #undef SET_TLS_VERIFY_DEFAULT

diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index da03a184c1..8748212a82 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -108,6 +108,7 @@ struct _virQEMUDriverConfig {
 char *defaultTLSx509certdir;
 bool defaultTLSx509certdirPresent;
 bool defaultTLSx509verify;
+bool defaultTLSx509verifyPresent;
 char *defaultTLSx509secretUUID;

 bool vncAutoUnixSocket;
-- 
2.28.0

[PATCH 0/6] qemu: Enable client TLS certificate validation by default for chardev, migration, and backup servers

[Peter Krempa]

See patches 6 for a explanation.

Peter Krempa (6):
  qemu: conf: Allow individual control of default value for
*_tls_x509_verify
  qemu: conf: Clarify default of "vnc_tls_x509_verify"
  qemu: conf: Enable 'chardev_tls_x509_verify' by default
  qemu: conf: Enable 'migrate_tls_x509_verify' by default
  qemu: conf: Enable 'backup_tls_x509_verify' by default
  NEWS: Mention change of default for TLS certificate verification

 NEWS.rst | 11 +++
 src/qemu/qemu.conf   | 18 ++
 src/qemu/qemu_conf.c | 22 ++
 src/qemu/qemu_conf.h |  1 +
 4 files changed, 40 insertions(+), 12 deletions(-)

-- 
2.28.0

[PATCH 3/4] qemuDomainDiskControllerIsBusy: Optimize checking for SCSI hostdevs

[Peter Krempa]

Iterate through hostdevs only when the controller type is
VIR_DOMAIN_CONTROLLER_TYPE_SCSI.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_hotplug.c | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 90ed59a0ee..124f60912f 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5373,13 +5373,15 @@ qemuDomainDiskControllerIsBusy(virDomainObjPtr vm,
 return true;
 }

-for (i = 0; i < vm->def->nhostdevs; i++) {
-hostdev = vm->def->hostdevs[i];
-if (!virHostdevIsSCSIDevice(hostdev) ||
-detach->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI)
-continue;
-if (hostdev->info->addr.drive.controller == detach->idx)
-return true;
+if (detach->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
+for (i = 0; i < vm->def->nhostdevs; i++) {
+hostdev = vm->def->hostdevs[i];
+if (!virHostdevIsSCSIDevice(hostdev))
+continue;
+
+if (hostdev->info->addr.drive.controller == detach->idx)
+return true;
+}
 }

 return false;
-- 
2.28.0

[PATCH 4/4] qemuDomainControllerIsBusy: Fully populate switch statement

[Peter Krempa]

Typecast the controller type variable to the appropriate type and add
the missing controller types for future extension.

Note that we currently allow only unplug of
VIR_DOMAIN_CONTROLLER_TYPE_SCSI thus the other controller types which
are not implemented return false now.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_hotplug.c | 20 ++--
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 124f60912f..2c12ef60af 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5392,20 +5392,28 @@ static bool
 qemuDomainControllerIsBusy(virDomainObjPtr vm,
virDomainControllerDefPtr detach)
 {
-switch (detach->type) {
+switch ((virDomainControllerType) detach->type) {
 case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
 case VIR_DOMAIN_CONTROLLER_TYPE_FDC:
 case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
+case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
 return qemuDomainDiskControllerIsBusy(vm, detach);

-case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
 case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL:
 case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
+case VIR_DOMAIN_CONTROLLER_TYPE_USB:
+case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
+case VIR_DOMAIN_CONTROLLER_TYPE_ISA:
+/* detach of the controller types above is not yet supported */
+return false;
+
+case VIR_DOMAIN_CONTROLLER_TYPE_XENBUS:
+/* qemu driver doesn't support xenbus */
+return false;
+
+case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
 default:
-/* libvirt does not support sata controller, and does not support to
- * detach virtio and smart card controller.
- */
-return true;
+return false;
 }
 }

-- 
2.28.0

[PATCH 1/4] qemuDomain(Disk)ControllerIsBusy: Fix function header format

[Peter Krempa]

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

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 81ec44ffcd..00d908912f 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5312,8 +5312,9 @@ qemuDomainDetachPrepDisk(virDomainObjPtr vm,
 }


-static bool qemuDomainDiskControllerIsBusy(virDomainObjPtr vm,
-   virDomainControllerDefPtr detach)
+static bool
+qemuDomainDiskControllerIsBusy(virDomainObjPtr vm,
+   virDomainControllerDefPtr detach)
 {
 size_t i;
 virDomainDiskDefPtr disk;
@@ -5352,8 +5353,10 @@ static bool 
qemuDomainDiskControllerIsBusy(virDomainObjPtr vm,
 return false;
 }

-static bool qemuDomainControllerIsBusy(virDomainObjPtr vm,
-   virDomainControllerDefPtr detach)
+
+static bool
+qemuDomainControllerIsBusy(virDomainObjPtr vm,
+   virDomainControllerDefPtr detach)
 {
 switch (detach->type) {
 case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
@@ -5372,6 +5375,7 @@ static bool qemuDomainControllerIsBusy(virDomainObjPtr vm,
 }
 }

+
 static int
 qemuDomainDetachPrepController(virDomainObjPtr vm,
virDomainControllerDefPtr match,
-- 
2.28.0

[PATCH 2/4] qemuDomainDiskControllerIsBusy: Fix logic of matching disk bus to controller type

[Peter Krempa]

The tests which match the disk bus to the controller type were backwards
in this function. This meant that any disk bus type (such as
VIR_DOMAIN_DISK_BUS_SATA) would not skip the controller index comparison
even if the removed controller was of a different type.

Switch the internals to a switch statement with selects the controller
type in the first place and a proper type so that new controller types
are added in the future.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1870072
Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_hotplug.c | 44 +++--
 1 file changed, 38 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 00d908912f..90ed59a0ee 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5327,16 +5327,48 @@ qemuDomainDiskControllerIsBusy(virDomainObjPtr vm,
 continue;

 /* check whether the disk uses this type controller */
-if (disk->bus == VIR_DOMAIN_DISK_BUS_IDE &&
-detach->type != VIR_DOMAIN_CONTROLLER_TYPE_IDE)
+switch ((virDomainControllerType) detach->type) {
+case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
+if (disk->bus != VIR_DOMAIN_DISK_BUS_IDE)
+continue;
+break;
+
+case VIR_DOMAIN_CONTROLLER_TYPE_FDC:
+if (disk->bus != VIR_DOMAIN_DISK_BUS_FDC)
+continue;
+break;
+
+case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
+if (disk->bus != VIR_DOMAIN_DISK_BUS_SCSI)
+continue;
+break;
+
+case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
+if (disk->bus != VIR_DOMAIN_DISK_BUS_SATA)
+continue;
+break;
+
+case VIR_DOMAIN_CONTROLLER_TYPE_XENBUS:
+/* xenbus is not supported by the qemu driver */
 continue;
-if (disk->bus == VIR_DOMAIN_DISK_BUS_FDC &&
-detach->type != VIR_DOMAIN_CONTROLLER_TYPE_FDC)
+
+case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL:
+/* virtio-serial does not host any disks */
 continue;
-if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI &&
-detach->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI)
+
+case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
+case VIR_DOMAIN_CONTROLLER_TYPE_USB:
+case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
+case VIR_DOMAIN_CONTROLLER_TYPE_ISA:
+/* These buses have (also) other device types too so they need to
+ * be checked elsewhere */
 continue;

+case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
+default:
+continue;
+}
+
 if (disk->info.addr.drive.controller == detach->idx)
 return true;
 }
-- 
2.28.0

[PATCH 0/4] qemu: hotplug: Fix check whether controler is used

[Peter Krempa]

Patch 2/4 is the actual fix, other patches are additional cleanups.

Peter Krempa (4):
  qemuDomain(Disk)ControllerIsBusy: Fix function header format
  qemuDomainDiskControllerIsBusy: Fix logic of matching disk bus to
controller type
  qemuDomainDiskControllerIsBusy: Optimize checking for SCSI hostdevs
  qemuDomainControllerIsBusy: Fully populate switch statement

 src/qemu/qemu_hotplug.c | 92 ++---
 1 file changed, 69 insertions(+), 23 deletions(-)

-- 
2.28.0

[PATCH] qemu: backup: Install bitmap for incremental backup to appropriate node only

[Peter Krempa]

Libvirt's backup code has two modes:

1) push - where qemu actively writes the difference since the checkpoint
  into the output file

2) pull - where we instruct qemu to expose a frozen disk state along
  with a bitmap of blocks which changed since the checkpoint

For push mode qemu needs the temporary bitmap we use where we calculate
the actual changes to be present on the block node backing the disk.

For pull mode where we expose the bitmap via NBD qemu actually wants the
bitmap to be present for the exported block node which is the scratch
file.

Until now we've calculated the bitmap twice and installed it both to the
scratch file and to the disk node, but we don't need to since we know
when it's needed.

Pass in the 'pull' flag and decide where to install the bitmap according
to it and also when to register the bitmap name with the blockjob.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_backup.c | 42 ++
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c
index 99dee46c4f..09f7921ea7 100644
--- a/src/qemu/qemu_backup.c
+++ b/src/qemu/qemu_backup.c
@@ -109,6 +109,7 @@ struct qemuBackupDiskData {
 virStorageSourcePtr terminator;
 virStorageSourcePtr backingStore;
 char *incrementalBitmap;
+const char *domdiskIncrementalBitmap; /* name of temporary bitmap 
installed on disk source */
 qemuBlockStorageSourceChainDataPtr crdata;
 bool labelled;
 bool initialized;
@@ -201,6 +202,7 @@ qemuBackupDiskPrepareOneBitmapsChain(virStorageSourcePtr 
backingChain,
 static int
 qemuBackupDiskPrepareOneBitmaps(struct qemuBackupDiskData *dd,
 virJSONValuePtr actions,
+bool pull,
 GHashTable *blockNamedNodeData)
 {
 if (!qemuBlockBitmapChainIsValid(dd->domdisk->src,
@@ -212,21 +214,29 @@ qemuBackupDiskPrepareOneBitmaps(struct qemuBackupDiskData 
*dd,
 return -1;
 }

-if (qemuBackupDiskPrepareOneBitmapsChain(dd->domdisk->src,
- dd->domdisk->src,
- dd->incrementalBitmap,
- dd->backupdisk->incremental,
- actions,
- blockNamedNodeData) < 0)
-return -1;
+/* For pull-mode backup, we need the bitmap to be present in the scratch
+ * file as that will be exported. For push-mode backup the bitmap is
+ * actually required on top of the image backing the disk */

-if (qemuBackupDiskPrepareOneBitmapsChain(dd->domdisk->src,
- dd->store,
- dd->incrementalBitmap,
- dd->backupdisk->incremental,
- actions,
- blockNamedNodeData) < 0)
-return -1;
+if (pull) {
+if (qemuBackupDiskPrepareOneBitmapsChain(dd->domdisk->src,
+ dd->store,
+ dd->incrementalBitmap,
+ dd->backupdisk->incremental,
+ actions,
+ blockNamedNodeData) < 0)
+return -1;
+} else {
+if (qemuBackupDiskPrepareOneBitmapsChain(dd->domdisk->src,
+ dd->domdisk->src,
+ dd->incrementalBitmap,
+ dd->backupdisk->incremental,
+ actions,
+ blockNamedNodeData) < 0)
+return -1;
+
+dd->domdiskIncrementalBitmap = dd->backupdisk->incremental;
+}

 return 0;
 }
@@ -293,12 +303,12 @@ qemuBackupDiskPrepareDataOne(virDomainObjPtr vm,
 else
 dd->incrementalBitmap = g_strdup_printf("backup-%s", 
dd->domdisk->dst);

-if (qemuBackupDiskPrepareOneBitmaps(dd, actions, blockNamedNodeData) < 
0)
+if (qemuBackupDiskPrepareOneBitmaps(dd, actions, pull, 
blockNamedNodeData) < 0)
 return -1;
 }

 if (!(dd->blockjob = qemuBlockJobDiskNewBackup(vm, dd->domdisk, dd->store,
-   dd->incrementalBitmap)))
+   
dd->domdiskIncrementalBitmap)))
 return -1;

 /* use original disk as backing to prevent opening the backing chain */
-- 
2.28.0

[PATCH] migration.html: Fix the spelling of the --persistent parameter

[Thomas Huth]

"--persist" is missing the "ent" at the end.

Signed-off-by: Thomas Huth 
---
 Sorry, I just noticed this after my previous "--undefinesource"
 patch had been merged - otherwise I had sent both fixes in one
 patch together...

 docs/migration.html.in | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/migration.html.in b/docs/migration.html.in
index 194cf7d209..77731eeb37 100644
--- a/docs/migration.html.in
+++ b/docs/migration.html.in
@@ -253,7 +253,7 @@
   migration by default. The virsh command has two flags to
   influence this behaviour. The  --undefinesource flag
   will cause the configuration file to be removed on the source host
-  after a successful migration. The --persist flag will
+  after a successful migration. The --persistent flag will
   cause a configuration file to be created on the destination host
   after a successful migration. The following table summarizes the
   configuration file handling in all possible state and flag
@@ -272,7 +272,7 @@
   Source config
   Dest config
   --undefinesource
-  --persist
+  --persistent
   Dest type
   Source config
   Dest config
-- 
2.18.4

Re: [PATCH libvirt v2 06/11] nodedev: detect AP matrix device

[Shalini Chellathurai Saroja]

On 11/12/20 9:29 PM, Jonathon Jongsma wrote:

diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c index 6bbff571..5f57000e 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1241,6 +1241,25 @@ udevProcessAPQueue(struct udev_device *device,
  }
  
  
+static int

+udevProcessAPMatrix(struct udev_device *device,
+virNodeDeviceDefPtr def)
+{
+size_t i;
+virNodeDevCapDataPtr data = >caps->data;
+
+data->ap_matrix.addr =
g_strdup(udev_device_get_sysname(device));
+def->name = g_strdup_printf("ap_%s", data->ap_matrix.addr);

So you're setting this 'addr' field, but it is not used anywhere else in
this patch. Perhaps you'll use it in upcoming patches. But it is not
formatted into the node device xml or anything like that. Is that
intentional?


Yes, it is not formatted into the node device xml.

I will include this change in the patch 10.




+
+for (i = 0; i < strlen(def->name); i++) {
+if (!(g_ascii_isalnum(*(def->name + i
+*(def->name + i) = '_';
+}
+
+return 0;
+}

Out of curiosity, what's the reason that you're
hard-coding an "ap_" prefix to the nodedev name rather than just using
udevGenerateDeviceName() like all of the other device types?
The name generated with udevGenerateDeviceName() is matrix_matrix (as 
both udev_device_get_subsystem and udev_device_get_sysname returns 
matrix), which is not very helpful, so we decided to go with ap_matrix 
instead.


--
Kind regards
Shalini Chellathurai Saroja
Linux on Z and Virtualization Development
Vorsitzende des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH 0/6] Introduce OpenSSH authorized key file mgmt APIs

[Michal Privoznik]

On 11/13/20 9:23 AM, Vasiliy Tolstov wrote:

But how about selinux? I'm run qemu-ga in guest and want to modify the
authorized_keys file of some user? Do we need to extend the selinux
policy to allow modification of such files in all guests?


Yes we do. But since qemu-ga offers this under API it should be fairly 
easy to argue that it should be allowed. It would be much harder to 
advocate for selinux policy change using solely file APIs of qemu-ga.


Michal

Re: [PATCH v2] selinux label: restore all labels when some labels fail to set

[Michal Privoznik]

On 11/13/20 10:47 AM, Jin Yan wrote:


Hi Michal,
I found this problem while performing migration, based on
     libvirt version: 6.2.0
     SELinux mode: permissive

Steps:
1. start a vm configured with pipe-type serial port.
     
   
   
     
   
     
2. migrate vm to Dst-side where no '/tmp/test_pipe' exits.
3. migration failed in Dst-side qemuProcessLaunch, and the path's label 
that

has been set is not restored ('/var/lib/libvirt/qemu/nvram/XXX.fd').

I have no idea why 2)rollback you mentioned didn't work.




I'm not sure. I could not reproduce with the current master. Is it 
possible for you to try the master?


Michal

Re: [PATCH libvirt v2 10/11] node_device: mdev matrix support

[Shalini Chellathurai Saroja]

On 11/12/20 9:42 PM, Jonathon Jongsma wrote:

On Thu, 12 Nov 2020 13:15:18 +0100
Shalini Chellathurai Saroja  wrote:


Allow mdev devices to be created on the matrix device.

Signed-off-by: Shalini Chellathurai Saroja 
Reviewed-by: Bjoern Walk 
Reviewed-by: Boris Fiuczynski 
---
  src/node_device/node_device_driver.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/node_device/node_device_driver.c
b/src/node_device/node_device_driver.c index 65c647f5..e254b492 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -662,6 +662,10 @@ nodeDeviceFindAddressByName(const char *name)
  break;
  }
  
+case VIR_NODE_DEV_CAP_AP_MATRIX:

+addr = g_strdup(caps->data.ap_matrix.addr);
+break;
+
  case VIR_NODE_DEV_CAP_SYSTEM:
  case VIR_NODE_DEV_CAP_USB_DEV:
  case VIR_NODE_DEV_CAP_USB_INTERFACE:
@@ -680,7 +684,6 @@ nodeDeviceFindAddressByName(const char *name)
  case VIR_NODE_DEV_CAP_VDPA:
  case VIR_NODE_DEV_CAP_AP_CARD:
  case VIR_NODE_DEV_CAP_AP_QUEUE:
-case VIR_NODE_DEV_CAP_AP_MATRIX:
  case VIR_NODE_DEV_CAP_LAST:
  break;
  }


So here is where you use the saved address field. Perhaps the
field should be introduced in this patch instead?

ok, I will do so.



--
Kind regards
Shalini Chellathurai Saroja
Linux on Z and Virtualization Development
Vorsitzende des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH] migration.html: Fix the spelling of the --undefinesource parameter

[Ján Tomko]

On a Friday in 2020, Thomas Huth wrote:

There is no dash between "undefine" and "source" in this parameter.



If you think there should be one, we can add an alias for the command
and hide the undashed version.


Signed-off-by: Thomas Huth 
---
docs/migration.html.in | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)



Reviewed-by: Ján Tomko 

and pushed.

Jano


signature.asc
Description: PGP signature

Re: [PATCH libvirt v2 03/11] nodedev: detect AP queues

[Shalini Chellathurai Saroja]

On 11/12/20 6:01 PM, Jonathon Jongsma wrote:

diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in
index d10a79e3..45281363 100644
--- a/docs/formatnode.html.in
+++ b/docs/formatnode.html.in
@@ -439,6 +439,17 @@
AP Card identifier.
  

+  ap_queue
+  Describes the AP Queue on a S390 host. An AP Queue is
+  identified by it's ap-adapter and ap-domain id.

"it's" should be "its". Is it worth a very brief description of what an
AP queue actually is?


Sure, I will provide a brief description on AP queue.

...



  
+  

+
+  ap_queue
+
+
+  
+
+
+  
+
+  

Let's use double quotes to keep the file consistent.

Sure:-)



+

  

@@ -738,4 +751,16 @@
  

  
+  

+
+  
+(0x)?[0-9a-fA-F]{1,4}
+  
+  
+0
+255

Is 255 correct here? the hex pattern above implies that it is a 16
bit value, but here you're limiting it to 255. If it is a 16 bit value,
can we just use the already-defined 'uint16' type from basictypes.rng?


Yes, it is. Current architecture limit is 256 (0-255) for domains.

The address schema of domains was created in linux on z, with a future 
architectural change in mind.


...




diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c index b4eb4553..6bbff571 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1218,6 +1218,29 @@ udevProcessAPCard(struct udev_device *device,
  }
  
  
+static int

+udevProcessAPQueue(struct udev_device *device,
+   virNodeDeviceDefPtr def)
+{
+char *c;
+virNodeDevCapDataPtr data = >caps->data;
+

In the previous patch, you added a comment explaining the format of the
sysfs path. I found that helpful. Without knowing the format, it's
a bit difficult to judge whether this code below is correct.


I will add the below comment

/* The sysfs path would be in the format /sys/bus/ap/devices/xx., 
where xx is ap adapter id and  is ap domain id. 
eg:/sys/bus/ap/devices/08.0001 */


--
Kind regards
Shalini Chellathurai Saroja
Vorsitzende des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

[PATCH] migration.html: Fix the spelling of the --undefinesource parameter

[Thomas Huth]

There is no dash between "undefine" and "source" in this parameter.

Signed-off-by: Thomas Huth 
---
 docs/migration.html.in | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/migration.html.in b/docs/migration.html.in
index dd5eddd6f4..194cf7d209 100644
--- a/docs/migration.html.in
+++ b/docs/migration.html.in
@@ -251,7 +251,7 @@
 
   As mentioned above, libvirt will not modify configuration files during
   migration by default. The virsh command has two flags to
-  influence this behaviour. The  --undefine-source flag
+  influence this behaviour. The  --undefinesource flag
   will cause the configuration file to be removed on the source host
   after a successful migration. The --persist flag will
   cause a configuration file to be created on the destination host
@@ -271,7 +271,7 @@
   Source type
   Source config
   Dest config
-  --undefine-source
+  --undefinesource
   --persist
   Dest type
   Source config
-- 
2.18.4

Re: [PATCH 0/3] kbase: Port 'debuglogs' from wiki and convert directory to rst

[Ján Tomko]

On a Friday in 2020, Andrea Bolognani wrote:

On Thu, 2020-07-30 at 12:30 +0200, Peter Krempa wrote:

Peter Krempa (3):
  docs: kbase: Convert 'kbase' article registry to RST
  docs: kbase: Make kbase article directory wider
  docs: kbase: Port 'debuglogs' document from libvirt's wiki


I just realized that, despite this series having been merged for a
long time, the original wiki page[1] still exists and its contents
are the same as they were back in July.

Can someone with write access to the wiki please turn it into a short
stub that points people to the kbase article[2] instead?



Done. Hope it's short enough for your taste.

Jano


Thanks!


[1] https://wiki.libvirt.org/page/DebugLogs
[2] https://libvirt.org/kbase/debuglogs.html
--
Andrea Bolognani / Red Hat / Virtualization



signature.asc
Description: PGP signature

Re: [PATCH v1 02/10] qemu_domain.c: align memory modules before calculating 'initialmem'

[Daniel Henrique Barboza]

On 11/13/20 10:51 AM, Andrea Bolognani wrote:

On Fri, 2020-11-13 at 09:58 -0300, Daniel Henrique Barboza wrote:

On 11/13/20 7:30 AM, Andrea Bolognani wrote:

On Wed, 2020-11-11 at 19:07 -0300, Daniel Henrique Barboza wrote:

+++ b/tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma.args
@@ -11,7 +11,7 @@ QEMU_AUDIO_DRV=none \
   -name QEMUGuest1 \
   -S \
   -machine pseries,accel=kvm,usb=off,dump-guest-core=off \
--m size=1310720k,slots=16,maxmem=4194304k \
+-m size=1048576k,slots=16,maxmem=4194304k \
   -realtime mlock=off \
   -smp 1,sockets=1,cores=1,threads=1 \
   -object memory-backend-ram,id=memdimm0,size=536870912 \


This doesn't look right: AFAIK the initial memory size is
guest-visible, so by changing how the alignment is performed you
might both change the guest ABI across guest boots (if libvirt is
upgraded in between them) and break migration (if either the source
or destination host is running the newer libvirt but the other side
isn't).


Good point. I failed to consider ABI stability for ppc64 guest migration.
Yes, this will break older guests that happen to have extra memory. In
fact, this can be specially harmful for migration.

This means that I can't proceed with any other ppc64 changes made here.
Aligning ppc64 DIMMs in PostParse will achieve the same results even
without this patch - the DIMMs will be aligned before 
qemuDomainAlignMemorySizes()
and initialmem will be rounded to a more precise value of 'currentMemory'.
I can think of ways to align ppc64 DIMMs while not touching initialmem,
but all of them will require extra state in the domain definition. The
benefits are there (the DIMMs will be aligned in the live XML) but I'm
not sure it's worth the extra code.


I only skimmed the remaining patches and didn't dig into the code as
much, or as recently, as you have, but from a high-level perspective
I don't see why you wouldn't be able to simply move the existing
rounding logic from the command line generator to PostParse? It's not
like the former has access to additional information that the latter
can't get to, right?


Interesting. I suppose that moving the logic of "qemuDomainAlignMemorySizes"
to PostParse might allows us to align DIMMs while being able to not touch
'initialmem'. I'm not entirely sure if that code is dependent on runtime stuff
though (the NUMA related code for instance). I'll investigate.


DHB

Re: [PATCH 0/3] kbase: Port 'debuglogs' from wiki and convert directory to rst

[Andrea Bolognani]

On Thu, 2020-07-30 at 12:30 +0200, Peter Krempa wrote:
> Peter Krempa (3):
>   docs: kbase: Convert 'kbase' article registry to RST
>   docs: kbase: Make kbase article directory wider
>   docs: kbase: Port 'debuglogs' document from libvirt's wiki

I just realized that, despite this series having been merged for a
long time, the original wiki page[1] still exists and its contents
are the same as they were back in July.

Can someone with write access to the wiki please turn it into a short
stub that points people to the kbase article[2] instead?

Thanks!


[1] https://wiki.libvirt.org/page/DebugLogs
[2] https://libvirt.org/kbase/debuglogs.html
-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH v1 02/10] qemu_domain.c: align memory modules before calculating 'initialmem'

[Andrea Bolognani]

On Fri, 2020-11-13 at 09:58 -0300, Daniel Henrique Barboza wrote:
> On 11/13/20 7:30 AM, Andrea Bolognani wrote:
> > On Wed, 2020-11-11 at 19:07 -0300, Daniel Henrique Barboza wrote:
> > > +++ b/tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma.args
> > > @@ -11,7 +11,7 @@ QEMU_AUDIO_DRV=none \
> > >   -name QEMUGuest1 \
> > >   -S \
> > >   -machine pseries,accel=kvm,usb=off,dump-guest-core=off \
> > > --m size=1310720k,slots=16,maxmem=4194304k \
> > > +-m size=1048576k,slots=16,maxmem=4194304k \
> > >   -realtime mlock=off \
> > >   -smp 1,sockets=1,cores=1,threads=1 \
> > >   -object memory-backend-ram,id=memdimm0,size=536870912 \
> > 
> > This doesn't look right: AFAIK the initial memory size is
> > guest-visible, so by changing how the alignment is performed you
> > might both change the guest ABI across guest boots (if libvirt is
> > upgraded in between them) and break migration (if either the source
> > or destination host is running the newer libvirt but the other side
> > isn't).
> 
> Good point. I failed to consider ABI stability for ppc64 guest migration.
> Yes, this will break older guests that happen to have extra memory. In
> fact, this can be specially harmful for migration.
> 
> This means that I can't proceed with any other ppc64 changes made here.
> Aligning ppc64 DIMMs in PostParse will achieve the same results even
> without this patch - the DIMMs will be aligned before 
> qemuDomainAlignMemorySizes()
> and initialmem will be rounded to a more precise value of 'currentMemory'.
> I can think of ways to align ppc64 DIMMs while not touching initialmem,
> but all of them will require extra state in the domain definition. The
> benefits are there (the DIMMs will be aligned in the live XML) but I'm
> not sure it's worth the extra code.

I only skimmed the remaining patches and didn't dig into the code as
much, or as recently, as you have, but from a high-level perspective
I don't see why you wouldn't be able to simply move the existing
rounding logic from the command line generator to PostParse? It's not
like the former has access to additional information that the latter
can't get to, right?

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: regression in meson build, AC_PATH_PROG lost

[Andrea Bolognani]

On Fri, 2020-11-13 at 12:37 +0100, Pavel Hrdina wrote:
> On Thu, Nov 12, 2020 at 10:40:02PM +0100, Olaf Hering wrote:
> > Since meson does not support environment variables it seems the
> > only way to address this is to introduce an option in
> > meson_options.txt for each runtime executable.
> > 
> > Will such change be accepted?
> 
> This would be one way how to address runtime executables, currently we
> have a lot of them. These executables are checked in order to
> enable/disable some libvirt feature but is used only in runtime:
> 
> parted, bhyve, bhyvectl, bhyveload, mount, umount, mkfs, pvcreate,
> vgcreate, lvcreate, pvremove, vgremove, lvremove, lvchange, vgchange,
> vgscan, pvs, vgs, lvs, (collie|dog), vstorage, vstorage-mount, zfs,
> zpool, numad
> 
> This one is even more broken as if we don't find it during build time it
> is set to empty string and never checked in our code:
> 
> showmount
> 
> These are checked during build time but if not present they are set to
> known absolute path:
> 
> qemu-bridge-helper, qemu-pr-helper, slirp-helper, dbus-daemon
> 
> And we have a list of optional_programs that are checked during build
> time and if not present they are set to the name of the executable and
> resolved during runtime from $PATH.
> 
> The last executable, pkcheck, is not used during build and in runtime.
> We only check its presence to enable/disable polkit support. We should
> be able to simply drop this check and figure out the presence of polkit
> using DBus only as we do for machined or logind and other DBus services
> that we use.
> 
> All of this was copied from autoconf except for the fact that it is no
> longer possible to use ENV variables.
> 
> I think we need to unify the process how we handle runtime executable
> dependencies and possibly add meson options for them.

As another data point, Debian currently carries a patch[1] which
allows us to enable the ZFS driver without installing the ZFS
packages in the build environment: this is necessary because, due to
its license, ZFS is kept outside of the main Debian repository.

Being able to use something like

  -Dprog_zfs=/sbin/zfs -Dprog_zfspool=/sbin/zfspool

to achieve the same result would allow us to drop that patch, which I
would be *extremely* happy about :)


[1] 
https://salsa.debian.org/libvirt-team/libvirt/-/blob/debian/master/debian/patches/debian/Set-defaults-for-zfs-tools.patch
-- 
Andrea Bolognani / Red Hat / Virtualization

[PATCH v2 0/2] virnetdaemon: Wait for "daemon-stop" thread to finish before quitting

[Michal Privoznik]

This is a v2 of:

https://www.redhat.com/archives/libvir-list/2020-November/msg00639.html

but it implements a different approach per Nikolay's suggestion.

Michal Prívozník (2):
  DO NOT MERGE
  virnetdaemon: Wait for "daemon-stop" thread to finish before quitting

 src/libvirt_remote.syms|  1 +
 src/remote/remote_daemon.c | 73 +++---
 src/rpc/virnetdaemon.c | 17 +
 src/rpc/virnetdaemon.h |  3 ++
 4 files changed, 66 insertions(+), 28 deletions(-)

-- 
2.26.2

[PATCH v2 2/2] virnetdaemon: Wait for "daemon-stop" thread to finish before quitting

[Michal Privoznik]

When the host is shutting down then we get PrepareForShutdown
signal on DBus to which we react by creating a thread which
runs virStateStop() and thus qemuStateStop(). But if scheduling
the thread is delayed just a but it may happen that we receive
SIGTERM (sent by systemd) to which we respond by quitting our
event loop and cleaning up everything (including drivers). And
only after that the thread gets to run only to find qemu_driver
being NULL.

What we can do is to delay exiting event loop and join the thread
that's executing virStateStop(). If the join doesn't happen in
given timeout (currently 30 seconds) then libvirtd shuts down
forcefully anyways (see virNetDaemonRun()).

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

Signed-off-by: Michal Privoznik 
---
 src/libvirt_remote.syms|  1 +
 src/remote/remote_daemon.c | 15 ---
 src/rpc/virnetdaemon.c | 17 +
 src/rpc/virnetdaemon.h |  3 +++
 4 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index d398d20880..3cd84a0854 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -88,6 +88,7 @@ virNetDaemonQuit;
 virNetDaemonRemoveShutdownInhibition;
 virNetDaemonRun;
 virNetDaemonSetShutdownCallbacks;
+virNetDaemonSetStateStopWorkerThread;
 virNetDaemonUpdateServices;
 
 
diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
index 5b68c9d2f9..75bb15a439 100644
--- a/src/remote/remote_daemon.c
+++ b/src/remote/remote_daemon.c
@@ -506,11 +506,20 @@ static void daemonStop(virNetDaemonPtr dmn,
siginfo_t *sig G_GNUC_UNUSED,
void *opaque G_GNUC_UNUSED)
 {
-virThread thr;
+virThreadPtr thr;
 virObjectRef(dmn);
-if (virThreadCreateFull(, false, daemonStopWorker,
-"daemon-stop", false, dmn) < 0)
+
+thr = g_new0(virThread, 1);
+
+if (virThreadCreateFull(thr, true,
+daemonStopWorker,
+"daemon-stop", false, dmn) < 0) {
 virObjectUnref(dmn);
+g_free(thr);
+return;
+}
+
+virNetDaemonSetStateStopWorkerThread(dmn, );
 }
 
 
diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
index 5f0f078fac..e337ff1fde 100644
--- a/src/rpc/virnetdaemon.c
+++ b/src/rpc/virnetdaemon.c
@@ -71,6 +71,7 @@ struct _virNetDaemon {
 
 virNetDaemonShutdownCallback shutdownPrepareCb;
 virNetDaemonShutdownCallback shutdownWaitCb;
+virThreadPtr stateStopThread;
 int finishTimer;
 bool quit;
 bool finished;
@@ -108,6 +109,7 @@ virNetDaemonDispose(void *obj)
 #endif /* !WIN32 */
 
 VIR_FORCE_CLOSE(dmn->autoShutdownInhibitFd);
+VIR_FREE(dmn->stateStopThread);
 
 virHashFree(dmn->servers);
 
@@ -773,6 +775,9 @@ daemonShutdownWait(void *opaque)
 if (dmn->shutdownWaitCb && dmn->shutdownWaitCb() < 0)
 goto finish;
 
+if (dmn->stateStopThread)
+virThreadJoin(dmn->stateStopThread);
+
 graceful = true;
 
  finish:
@@ -891,6 +896,18 @@ virNetDaemonRun(virNetDaemonPtr dmn)
 }
 
 
+void
+virNetDaemonSetStateStopWorkerThread(virNetDaemonPtr dmn,
+ virThreadPtr *thr)
+{
+virObjectLock(dmn);
+
+VIR_DEBUG("Setting state stop worker thread on dmn=%p to thr=%p", dmn, 
thr);
+dmn->stateStopThread = g_steal_pointer(thr);
+virObjectUnlock(dmn);
+}
+
+
 void
 virNetDaemonQuit(virNetDaemonPtr dmn)
 {
diff --git a/src/rpc/virnetdaemon.h b/src/rpc/virnetdaemon.h
index 6ae5305e53..fcc6e1fdff 100644
--- a/src/rpc/virnetdaemon.h
+++ b/src/rpc/virnetdaemon.h
@@ -69,6 +69,9 @@ int virNetDaemonAddSignalHandler(virNetDaemonPtr dmn,
 void virNetDaemonUpdateServices(virNetDaemonPtr dmn,
 bool enabled);
 
+void virNetDaemonSetStateStopWorkerThread(virNetDaemonPtr dmn,
+  virThreadPtr *thr);
+
 void virNetDaemonRun(virNetDaemonPtr dmn);
 
 void virNetDaemonQuit(virNetDaemonPtr dmn);
-- 
2.26.2

[PATCH v2 1/2] DO NOT MERGE

[Michal Privoznik]

This is to help reproduce the race. Build and attach gdb and:

handle SIGUSR1 nostop pass
handle SIGINT nostop pass

and then:

kill -SIGUSR1 $(pgrep libvirtd); sleep 1; kill -SIGINT $(pgrep libvirtd)
---
 src/remote/remote_daemon.c | 64 +-
 1 file changed, 36 insertions(+), 28 deletions(-)

diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
index 7607da94be..5b68c9d2f9 100644
--- a/src/remote/remote_daemon.c
+++ b/src/remote/remote_daemon.c
@@ -483,6 +483,37 @@ static void daemonReloadHandler(virNetDaemonPtr dmn 
G_GNUC_UNUSED,
 }
 }
 
+
+static void daemonStopWorker(void *opaque)
+{
+virNetDaemonPtr dmn = opaque;
+
+sleep(10);
+
+VIR_DEBUG("Begin stop dmn=%p", dmn);
+
+ignore_value(virStateStop());
+
+VIR_DEBUG("Completed stop dmn=%p", dmn);
+
+/* Exit daemon cleanly */
+virNetDaemonQuit(dmn);
+}
+
+
+/* We do this in a thread to not block the main loop */
+static void daemonStop(virNetDaemonPtr dmn,
+   siginfo_t *sig G_GNUC_UNUSED,
+   void *opaque G_GNUC_UNUSED)
+{
+virThread thr;
+virObjectRef(dmn);
+if (virThreadCreateFull(, false, daemonStopWorker,
+"daemon-stop", false, dmn) < 0)
+virObjectUnref(dmn);
+}
+
+
 static int daemonSetupSignals(virNetDaemonPtr dmn)
 {
 if (virNetDaemonAddSignalHandler(dmn, SIGINT, daemonShutdownHandler, NULL) 
< 0)
@@ -493,6 +524,8 @@ static int daemonSetupSignals(virNetDaemonPtr dmn)
 return -1;
 if (virNetDaemonAddSignalHandler(dmn, SIGHUP, daemonReloadHandler, NULL) < 
0)
 return -1;
+if (virNetDaemonAddSignalHandler(dmn, SIGUSR1, daemonStop, NULL) < 0)
+return -1;
 return 0;
 }
 
@@ -511,32 +544,6 @@ static void daemonInhibitCallback(bool inhibit, void 
*opaque)
 static GDBusConnection *sessionBus;
 static GDBusConnection *systemBus;
 
-static void daemonStopWorker(void *opaque)
-{
-virNetDaemonPtr dmn = opaque;
-
-VIR_DEBUG("Begin stop dmn=%p", dmn);
-
-ignore_value(virStateStop());
-
-VIR_DEBUG("Completed stop dmn=%p", dmn);
-
-/* Exit daemon cleanly */
-virNetDaemonQuit(dmn);
-}
-
-
-/* We do this in a thread to not block the main loop */
-static void daemonStop(virNetDaemonPtr dmn)
-{
-virThread thr;
-virObjectRef(dmn);
-if (virThreadCreateFull(, false, daemonStopWorker,
-"daemon-stop", false, dmn) < 0)
-virObjectUnref(dmn);
-}
-
-
 static GDBusMessage *
 handleSessionMessageFunc(GDBusConnection *connection G_GNUC_UNUSED,
  GDBusMessage *message,
@@ -550,7 +557,7 @@ handleSessionMessageFunc(GDBusConnection *connection 
G_GNUC_UNUSED,
 if (virGDBusMessageIsSignal(message,
 "org.freedesktop.DBus.Local",
 "Disconnected"))
-daemonStop(dmn);
+daemonStop(dmn, NULL, NULL);
 
 return message;
 }
@@ -569,7 +576,7 @@ handleSystemMessageFunc(GDBusConnection *connection 
G_GNUC_UNUSED,
 
 VIR_DEBUG("dmn=%p", dmn);
 
-daemonStop(dmn);
+daemonStop(dmn, NULL, NULL);
 }
 
 
@@ -1247,5 +1254,6 @@ int main(int argc, char **argv) {
 VIR_FREE(remote_config_file);
 daemonConfigFree(config);
 
+sleep(10);
 return ret;
 }
-- 
2.26.2

Re: [PATCH v1 02/10] qemu_domain.c: align memory modules before calculating 'initialmem'

[Daniel Henrique Barboza]

On 11/13/20 7:30 AM, Andrea Bolognani wrote:

On Wed, 2020-11-11 at 19:07 -0300, Daniel Henrique Barboza wrote:

+++ b/tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma.args
@@ -11,7 +11,7 @@ QEMU_AUDIO_DRV=none \
  -name QEMUGuest1 \
  -S \
  -machine pseries,accel=kvm,usb=off,dump-guest-core=off \
--m size=1310720k,slots=16,maxmem=4194304k \
+-m size=1048576k,slots=16,maxmem=4194304k \
  -realtime mlock=off \
  -smp 1,sockets=1,cores=1,threads=1 \
  -object memory-backend-ram,id=memdimm0,size=536870912 \


This doesn't look right: AFAIK the initial memory size is
guest-visible, so by changing how the alignment is performed you
might both change the guest ABI across guest boots (if libvirt is
upgraded in between them) and break migration (if either the source
or destination host is running the newer libvirt but the other side
isn't).


Good point. I failed to consider ABI stability for ppc64 guest migration.
Yes, this will break older guests that happen to have extra memory. In
fact, this can be specially harmful for migration.

This means that I can't proceed with any other ppc64 changes made here.
Aligning ppc64 DIMMs in PostParse will achieve the same results even
without this patch - the DIMMs will be aligned before 
qemuDomainAlignMemorySizes()
and initialmem will be rounded to a more precise value of 'currentMemory'.
I can think of ways to align ppc64 DIMMs while not touching initialmem,
but all of them will require extra state in the domain definition. The
benefits are there (the DIMMs will be aligned in the live XML) but I'm
not sure it's worth the extra code.

Thanks for pointing this out. I'll evaluate if the x86 bits are still
valuable and re-send them, since they're not messing with initialmem
calculation of x86 guests.


DHB





Did I miss something that makes this okay?

Re: [PATCH v2 00/10] qemu: add option to process offloaded legacy blockjob event ealier

[Nikolay Shirokovskiy]

On 13.11.2020 10:24, Peter Krempa wrote:
> On Fri, Nov 13, 2020 at 09:53:28 +0300, Nikolay Shirokovskiy wrote:
>> This is successor to [1] but I changed the subject as in the review the patch
>> 'qemu: sync backing chain update in virDomainGetBlockJobInfo' was not
>> considered good one from design POV. However I think the basic patch is 
>> helpful
>> to address similar issues. Look at [*] for example, there it allows to sync
>> backing chains during query block stats.
>>
>> In discussion of [1] I stated that first patch will also allow to get rid of
>> stale block job events on reconnection to qemu. But this will require
>> additional work actually so with this patch series stale events are still
>> present. And in the discussion it was also mentioned by Peter that stale 
>> events
>> are not harmful for legacy blockjobs code. I also removed previous attempts 
>> to
>> eliminate some of stale events.
>>
>> I still keep patch for virDomainGetBlockJobInfo. May be in comparsion to [*]
>> the approach the patch takes will look not so bad :)
>>
>> [1] First version of the patch series
>> https://www.redhat.com/archives/libvir-list/2020-October/msg01133.html
> 
> Please allow for longer review time. I'm stuffed with other work and
> also as I've pointed out last time I'm not very in favor of modifying
> the old block job code since it's being phased out.
> 
> I still suggest the client application is fixed to use the events and
> stop polling virDomainGetBlockJobInfo.
> 

Of course, take your time, we all have our duties.

Note that there is new case in "[PATCH v2 09/10] qemu: fix race on legacy block 
completion and quering stats" that is similar to case in
virDomainGetBlockJobInfo. AFAIU modern blockjobs are also affected. 

Nikolay

Re: regression in meson build, AC_PATH_PROG lost

[Pavel Hrdina]

On Thu, Nov 12, 2020 at 10:40:02PM +0100, Olaf Hering wrote:
> autoconf allows to specify the path to a runtime binary that is not required 
> during build via an environment variable:
> AC_PATH_PROG([PARTED], [parted], [], [$LIBVIRT_SBIN_PATH])
> 
> meson lacks such essential feature. As a result the package build environment 
> needs to have more packages than necessary installed. This excessive list 
> also pushes libvirt closer to the end of the build dependency chain.
> 
> Was this considered while the buildsystem was switched to meson?

Hi,

I did not considered this specific use-case where the runtime executable
dependency is not available during build time. Mainly because we have
a lot of other executables that are runtime dependencies but are check
by meson to figure out if some libvirt feature should be enabled or
disabled.

> Since meson does not support environment variables it seems the only way to 
> address this is to introduce an option in meson_options.txt for each runtime 
> executable.
> 
> Will such change be accepted?

This would be one way how to address runtime executables, currently we
have a lot of them. These executables are checked in order to
enable/disable some libvirt feature but is used only in runtime:

parted, bhyve, bhyvectl, bhyveload, mount, umount, mkfs, pvcreate,
vgcreate, lvcreate, pvremove, vgremove, lvremove, lvchange, vgchange,
vgscan, pvs, vgs, lvs, (collie|dog), vstorage, vstorage-mount, zfs,
zpool, numad

This one is even more broken as if we don't find it during build time it
is set to empty string and never checked in our code:

showmount

These are checked during build time but if not present they are set to
known absolute path:

qemu-bridge-helper, qemu-pr-helper, slirp-helper, dbus-daemon

And we have a list of optional_programs that are checked during build
time and if not present they are set to the name of the executable and
resolved during runtime from $PATH.

The last executable, pkcheck, is not used during build and in runtime.
We only check its presence to enable/disable polkit support. We should
be able to simply drop this check and figure out the presence of polkit
using DBus only as we do for machined or logind and other DBus services
that we use.

All of this was copied from autoconf except for the fact that it is no
longer possible to use ENV variables.

I think we need to unify the process how we handle runtime executable
dependencies and possibly add meson options for them.

Pavel


signature.asc
Description: PGP signature

Re: [PATCH v1] wireshark: fix build with with 2.4.0

[Daniel P . Berrangé]

On Fri, Nov 13, 2020 at 12:04:43PM +0100, Olaf Hering wrote:
> Am Fri, 13 Nov 2020 10:53:41 +
> schrieb Daniel P. Berrangé :
> 
> > SLE12 is no longer considered a supported platform by libvirt, since it
> > has been more than 2 years since the release of SLE15.
> 
> That might be all true.
> 
> This patch is for wireshark. Not for SLE12.

The probem is simply that the min version is wrong - it should be 2.6.0
instead I believe, as it looks like that is the oldest version on the
targetted distros (RHEL-7 is excepted as we don't support wireshark 1.x
at all). Once I've validated the CI pipeline, I'll bump the min version
which will avoid the build failure.

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 libvirt v2 01/11] nodedev: detect AP card device

[Shalini Chellathurai Saroja]

Hi Jonathon,

Thank you for the quick review:-)

On 11/12/20 5:27 PM, Jonathon Jongsma wrote:

+  
+
+  
+  
+  
+  

It seems that you're unnecessarily changing double-quotes to
single-quotes here, which adds spurious changes to the diff. The rest of
the file uses double-quotes. Let's stick with that.

Sure.



  

  
@@ -716,4 +726,16 @@

  

  
+  

+
+  
+(0x)?[0-9a-fA-F]{1,2}
+  
+  
+0
+255
+  
+
+  

As far as I can tell, this is identical to the definition of the
'uint8' type in basictypes.rng. Is there a reason for defining a
different type?


Yes, In definition apAdapterRange, the prefix '0x' is optional.

--
Kind regards
Shalini Chellathurai Saroja
Linux on Z and Virtualization Development
Dept 1419
Vorsitzende des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH v1] wireshark: fix build with with 2.4.0

[Olaf Hering]

Am Fri, 13 Nov 2020 10:53:41 +
schrieb Daniel P. Berrangé :

> SLE12 is no longer considered a supported platform by libvirt, since it
> has been more than 2 years since the release of SLE15.

That might be all true.

This patch is for wireshark. Not for SLE12.

Olaf


pgpJmXJp43WLi.pgp
Description: Digitale Signatur von OpenPGP

[PATCH v2 09/10] qemu: fix race on legacy block completion and quering stats

[Nikolay Shirokovskiy]

At the time when we query qemu for block stats backing chain in qemu and
libvirt can be different and this will result in messy block stats. I guess
this can be noticable under load when thread that process events is busy so
that it can take some time before block job events are processed.

The modern block job have same issue I guess but I don't know them well enough
to apply this fix too. Also the approach for them can be different as there is
option to use autofinalize = no.

Signed-off-by: Nikolay Shirokovskiy 
---
 src/qemu/qemu_driver.c | 24 +++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 25466f6..05917eb 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18458,8 +18458,9 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
 int count_index = -1;
 size_t visited = 0;
 bool visitBacking = !!(privflags & QEMU_DOMAIN_STATS_BACKING);
+bool refresh = true;
 
-if (HAVE_JOB(privflags) && virDomainObjIsActive(dom)) {
+while (HAVE_JOB(privflags) && virDomainObjIsActive(dom) && refresh) {
 qemuDomainObjEnterMonitor(driver, dom);
 
 rc = qemuMonitorGetAllBlockStatsInfo(priv->mon, , visitBacking);
@@ -18481,6 +18482,27 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
 /* failure to retrieve stats is fine at this point */
 if (rc < 0 || (fetchnodedata && !nodedata))
 virResetLastError();
+
+refresh = false;
+if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) {
+for (i = 0; i < dom->def->ndisks; i++) {
+virDomainDiskDefPtr disk = dom->def->disks[i];
+g_autoptr(qemuBlockJobData) job = qemuBlockJobDiskGetJob(disk);
+
+if (!job)
+continue;
+
+/*
+ * If block job is completed then backing chain at time
+ * of quering stats in qemu and current backing chain in
+ * libvirt can be different. We need to sync them.
+ */
+if (job->newstate == QEMU_BLOCKJOB_STATE_COMPLETED) {
+qemuBlockJobUpdate(dom, job, QEMU_ASYNC_JOB_NONE);
+refresh = true;
+}
+}
+}
 }
 
 if (nodedata &&
-- 
1.8.3.1

[PATCH v2 08/10] qemu: move code that depends on backing chain appropriately

[Nikolay Shirokovskiy]

After the previous patch that moves backing chain detection later in code.

Signed-off-by: Nikolay Shirokovskiy 
---
 src/qemu/qemu_process.c | 47 +--
 1 file changed, 25 insertions(+), 22 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 0d50db5..0dadc69 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8372,22 +8372,6 @@ qemuProcessReconnect(void *opaque)
 if (qemuDomainInitializePflashStorageSource(obj) < 0)
 goto error;
 
-/* XXX: Need to change as long as lock is introduced for
- * qemu_driver->sharedDevices.
- */
-for (i = 0; i < obj->def->ndisks; i++) {
-virDomainDiskDefPtr disk = obj->def->disks[i];
-virDomainDeviceDef dev;
-
-if (virDomainDiskTranslateSourcePool(disk) < 0)
-goto error;
-
-dev.type = VIR_DOMAIN_DEVICE_DISK;
-dev.data.disk = disk;
-if (qemuAddSharedDevice(driver, , obj->def->name) < 0)
-goto error;
-}
-
 for (i = 0; i < obj->def->ngraphics; i++) {
 if (qemuProcessGraphicsReservePorts(obj->def->graphics[i], true) < 0)
 goto error;
@@ -8436,12 +8420,6 @@ qemuProcessReconnect(void *opaque)
 goto error;
 }
 
-/* if domain requests security driver we haven't loaded, report error, but
- * do not kill the domain
- */
-ignore_value(qemuSecurityCheckAllLabel(driver->securityManager,
-   obj->def));
-
 if (qemuProcessRefreshCPU(driver, obj) < 0)
 goto error;
 
@@ -8482,6 +8460,31 @@ qemuProcessReconnect(void *opaque)
 if (qemuProcessRefreshBlockjobs(driver, obj) < 0)
 goto error;
 
+/* XXX: Need to change as long as lock is introduced for
+ * qemu_driver->sharedDevices.
+ *
+ * Handling shared devices depends on top source and thus should
+ * follow refreshing blockjobs which can update it.
+ */
+for (i = 0; i < obj->def->ndisks; i++) {
+virDomainDiskDefPtr disk = obj->def->disks[i];
+virDomainDeviceDef dev;
+
+if (virDomainDiskTranslateSourcePool(disk) < 0)
+goto error;
+
+dev.type = VIR_DOMAIN_DEVICE_DISK;
+dev.data.disk = disk;
+if (qemuAddSharedDevice(driver, , obj->def->name) < 0)
+goto error;
+}
+
+/* if domain requests security driver we haven't loaded, report error, but
+ * do not kill the domain
+ */
+ignore_value(qemuSecurityCheckAllLabel(driver->securityManager,
+   obj->def));
+
 if (qemuProcessUpdateDevices(driver, obj) < 0)
 goto error;
 
-- 
1.8.3.1

[PATCH v2 07/10] qemu: refresh backing chain after block job reconnection

[Nikolay Shirokovskiy]

After [1] we basically ignore qemu block job event if there is no
correspondent block job object in libvirt. Thus we need to refresh backing
chain after we reconnect block jobs or we can miss events that change backing
chain.

There is also another reason for it. I'm not sure of result if both libvirt is
reading images metadata on refreshing backing chain and qemu is writing
metadata on block job completion.

Reconnection code that depends on backing chain will be moved appropriately in
a different patch.

[1] qemu: add option to process offloaded legacy blockjob event ealier

Signed-off-by: Nikolay Shirokovskiy 
---
 src/qemu/qemu_process.c | 37 +++--
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 46a39ac..0d50db5 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8193,9 +8193,15 @@ static int
 qemuProcessRefreshLegacyBlockjobs(virQEMUDriverPtr driver,
   virDomainObjPtr vm)
 {
+qemuDomainObjPrivatePtr priv = vm->privateData;
 g_autoptr(GHashTable) blockJobs = NULL;
 size_t i;
 
+if (priv->reconnectBlockjobs == VIR_TRISTATE_BOOL_NO) {
+VIR_DEBUG("skipping block job reconnection");
+return 0;
+}
+
 for (i = 0; i < vm->def->ndisks; i++) {
 virDomainDiskDefPtr disk = vm->def->disks[i];
 g_autoptr(qemuBlockJobData) job = NULL;
@@ -8221,9 +8227,25 @@ qemuProcessRefreshLegacyBlockjobs(virQEMUDriverPtr 
driver,
  */
 for (i = 0; i < vm->def->ndisks; i++) {
 virDomainDiskDefPtr disk = vm->def->disks[i];
-qemuBlockJobDataPtr job =  qemuBlockJobDiskGetJob(disk);
+g_autoptr(qemuBlockJobData) job =  qemuBlockJobDiskGetJob(disk);
 
 qemuBlockJobStartupFinalize(vm, job);
+
+if ((job = qemuBlockJobDiskGetJob(disk)))
+continue;
+
+if (!disk->mirror) {
+/* This should be the only place that calls
+ * qemuDomainDetermineDiskChain with @report_broken == false
+ * to guarantee best-effort domain reconnect */
+virStorageSourceBackingStoreClear(disk->src);
+if (qemuDomainDetermineDiskChain(driver, vm, disk, NULL, false) < 
0)
+return -1;
+}
+/*
+ * TODO we need code to properly reconnect for mirror blockjobs
+ * that completed while libvirtd was down.
+ */
 }
 
 return 0;
@@ -8360,19 +8382,6 @@ qemuProcessReconnect(void *opaque)
 if (virDomainDiskTranslateSourcePool(disk) < 0)
 goto error;
 
-/* backing chains need to be refreshed only if they could change */
-if (priv->reconnectBlockjobs != VIR_TRISTATE_BOOL_NO &&
-!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) {
-/* This should be the only place that calls
- * qemuDomainDetermineDiskChain with @report_broken == false
- * to guarantee best-effort domain reconnect */
-virStorageSourceBackingStoreClear(disk->src);
-if (qemuDomainDetermineDiskChain(driver, obj, disk, NULL, false) < 
0)
-goto error;
-} else {
-VIR_DEBUG("skipping backing chain detection for '%s'", disk->dst);
-}
-
 dev.type = VIR_DOMAIN_DEVICE_DISK;
 dev.data.disk = disk;
 if (qemuAddSharedDevice(driver, , obj->def->name) < 0)
-- 
1.8.3.1

[PATCH v2 10/10] qemu: sync backing chain update in virDomainGetBlockJobInfo

[Nikolay Shirokovskiy]

Some mgmt still use polling for block job completion. After job completion the
job failure/success is infered by inspecting domain xml. With legacy block job
processing this does not always work.

The issue deals with how libvirt processes events. If no other thread is
waiting for blockjob event then event processing if offloaded to worker thread.
If now virDomainGetBlockJobInfo API is called then as block job is already
dismissed in legacy scheme the API returns 0 but backing chain is not yet
updated as processing yet be done in worker thread. Now mgmt checks backing
chain right after return from the API call and detects error.

This happens quite often under load. I guess because of we have only one worker
thread for all the domains.

The event delivery is synchronous in qemu and block job completed event is sent
in job finalize step so if block job is absent the event is already delivered
and we just need to process it.

Signed-off-by: Nikolay Shirokovskiy 
---
 src/qemu/qemu_driver.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 05917eb..25f66df 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14740,8 +14740,15 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom,
 ret = qemuMonitorGetBlockJobInfo(qemuDomainGetMonitor(vm), job->name, 
);
 if (qemuDomainObjExitMonitor(driver, vm) < 0)
 ret = -1;
-if (ret <= 0)
+if (ret < 0)
+goto endjob;
+if (ret == 0) {
+qemuDomainObjPrivatePtr priv = vm->privateData;
+
+if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV))
+qemuBlockJobUpdate(vm, job, QEMU_ASYNC_JOB_NONE);
 goto endjob;
+}
 
 if (qemuBlockJobInfoTranslate(, info, disk,
   flags & 
VIR_DOMAIN_BLOCK_JOB_INFO_BANDWIDTH_BYTES) < 0) {
-- 
1.8.3.1

[PATCH v2 05/10] qemu: add note for outdated legacy block job events

[Nikolay Shirokovskiy]

Signed-off-by: Nikolay Shirokovskiy 
---
 src/qemu/qemu_process.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 9bbc9dc..8706de3 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8214,6 +8214,12 @@ qemuProcessRefreshLegacyBlockjobs(virQEMUDriverPtr 
driver,
 if (virHashForEach(blockJobs, qemuProcessRefreshLegacyBlockjob, vm) < 0)
 goto cleanup;
 
+/*
+ * At this point we can have outdated pending events in job->newstate.
+ * These events can arise from qemu events received before we get
+ * reply to query-block-jobs command. So the code that handle legacy
+ * block job events should handle outdated events correctly.
+ */
 for (i = 0; i < vm->def->ndisks; i++) {
 virDomainDiskDefPtr disk = vm->def->disks[i];
 qemuBlockJobDataPtr job =  qemuBlockJobDiskGetJob(disk);
-- 
1.8.3.1

[PATCH v2 06/10] qemu: use autoptr in qemuProcessRefreshLegacyBlockjobs

[Nikolay Shirokovskiy]

Signed-off-by: Nikolay Shirokovskiy 
---
 src/qemu/qemu_process.c | 13 -
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 8706de3..46a39ac 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8193,8 +8193,7 @@ static int
 qemuProcessRefreshLegacyBlockjobs(virQEMUDriverPtr driver,
   virDomainObjPtr vm)
 {
-GHashTable *blockJobs = NULL;
-int ret = -1;
+g_autoptr(GHashTable) blockJobs = NULL;
 size_t i;
 
 for (i = 0; i < vm->def->ndisks; i++) {
@@ -8209,10 +8208,10 @@ qemuProcessRefreshLegacyBlockjobs(virQEMUDriverPtr 
driver,
 qemuDomainObjEnterMonitor(driver, vm);
 blockJobs = qemuMonitorGetAllBlockJobInfo(qemuDomainGetMonitor(vm), true);
 if (qemuDomainObjExitMonitor(driver, vm) < 0 || !blockJobs)
-goto cleanup;
+return -1;
 
 if (virHashForEach(blockJobs, qemuProcessRefreshLegacyBlockjob, vm) < 0)
-goto cleanup;
+return -1;
 
 /*
  * At this point we can have outdated pending events in job->newstate.
@@ -8227,11 +8226,7 @@ qemuProcessRefreshLegacyBlockjobs(virQEMUDriverPtr 
driver,
 qemuBlockJobStartupFinalize(vm, job);
 }
 
-ret = 0;
-
- cleanup:
-virHashFree(blockJobs);
-return ret;
+return 0;
 }
 
 
-- 
1.8.3.1

[PATCH v2 03/10] qemu: remove extra block job finalize on reconnect

[Nikolay Shirokovskiy]

As we now call qemuBlockJobStartupFinalize in qemuProcessRefreshLegacyBlockjobs
for precreated block job of every disk anyway.

Signed-off-by: Nikolay Shirokovskiy 
---
 src/qemu/qemu_process.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 549c17a..49ee8fe 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8140,7 +8140,7 @@ qemuProcessRefreshLegacyBlockjob(void *payload,
 virDomainObjPtr vm = opaque;
 qemuMonitorBlockJobInfoPtr info = payload;
 virDomainDiskDefPtr disk;
-qemuBlockJobDataPtr job;
+g_autoptr(qemuBlockJobData) job = NULL;
 qemuBlockJobType jobtype = info->type;
 qemuDomainObjPrivatePtr priv = vm->privateData;
 
@@ -8187,7 +8187,6 @@ qemuProcessRefreshLegacyBlockjob(void *payload,
 qemuBlockJobStarted(job, vm);
 
  cleanup:
-qemuBlockJobStartupFinalize(vm, job);
 
 return 0;
 }
-- 
1.8.3.1

[PATCH v2 04/10] qemu: remove stale cleanup in qemuProcessRefreshLegacyBlockjob

[Nikolay Shirokovskiy]

Signed-off-by: Nikolay Shirokovskiy 
---
 src/qemu/qemu_process.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 49ee8fe..9bbc9dc 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8172,7 +8172,7 @@ qemuProcessRefreshLegacyBlockjob(void *payload,
 if (disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) {
 if (qemuDomainDetermineDiskChain(priv->driver, vm, disk,
  disk->mirror, true) < 0)
-goto cleanup;
+return 0;
 
 if (disk->mirror->format &&
 disk->mirror->format != VIR_STORAGE_FILE_RAW &&
@@ -8180,14 +8180,11 @@ qemuProcessRefreshLegacyBlockjob(void *payload,
  qemuSetupImageChainCgroup(vm, disk->mirror) < 0 ||
  qemuSecuritySetImageLabel(priv->driver, vm, disk->mirror,
true, true) < 0))
-goto cleanup;
+return 0;
 }
 }
 
 qemuBlockJobStarted(job, vm);
-
- cleanup:
-
 return 0;
 }
 
-- 
1.8.3.1

[PATCH v2 02/10] qemu: reconnect: precreate legacy blockjobs

[Nikolay Shirokovskiy]

Now we basically ignore qemu block job event if there is no correspondent block
job object in libvirt. So we need to precreate block job objects before we call
qemu's query-block-job because we can receive events right after receiving
query-block-job result but before we create block job object in libvirt and
miss this event.

Signed-off-by: Nikolay Shirokovskiy 
---
 src/qemu/qemu_process.c | 21 +++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index a810f38..549c17a 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8153,8 +8153,8 @@ qemuProcessRefreshLegacyBlockjob(void *payload,
 disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT)
 jobtype = disk->mirrorJob;
 
-if (!(job = qemuBlockJobDiskNew(vm, disk, jobtype, jobname)))
-return -1;
+job = qemuBlockJobDiskGetJob(disk);
+job->type = jobtype;
 
 if (disk->mirror) {
 if (info->ready == 1 ||
@@ -8199,6 +8199,16 @@ qemuProcessRefreshLegacyBlockjobs(virQEMUDriverPtr 
driver,
 {
 GHashTable *blockJobs = NULL;
 int ret = -1;
+size_t i;
+
+for (i = 0; i < vm->def->ndisks; i++) {
+virDomainDiskDefPtr disk = vm->def->disks[i];
+g_autoptr(qemuBlockJobData) job = NULL;
+g_autofree char *jobname = NULL;
+
+jobname = qemuAliasDiskDriveFromDisk(disk);
+job = qemuBlockJobDiskNew(vm, disk, QEMU_BLOCKJOB_TYPE_NONE, jobname);
+}
 
 qemuDomainObjEnterMonitor(driver, vm);
 blockJobs = qemuMonitorGetAllBlockJobInfo(qemuDomainGetMonitor(vm), true);
@@ -8208,6 +8218,13 @@ qemuProcessRefreshLegacyBlockjobs(virQEMUDriverPtr 
driver,
 if (virHashForEach(blockJobs, qemuProcessRefreshLegacyBlockjob, vm) < 0)
 goto cleanup;
 
+for (i = 0; i < vm->def->ndisks; i++) {
+virDomainDiskDefPtr disk = vm->def->disks[i];
+qemuBlockJobDataPtr job =  qemuBlockJobDiskGetJob(disk);
+
+qemuBlockJobStartupFinalize(vm, job);
+}
+
 ret = 0;
 
  cleanup:
-- 
1.8.3.1

[PATCH v2 01/10] qemu: add option to process offloaded legacy blockjob event ealier

[Nikolay Shirokovskiy]

Currently in qemuProcessHandleBlockJob we either offload blockjob event
processing to the worker thread or notify another thread that waits for
blockjob event and that thread processes the event. But sometimes after event
is offloaded to the worker thread we need to process the event in a different
thread.

To be able to to do it let's always set newstate/errmsg and then let whatever
thread is first process it.

Libvirt always creates blockjob object before starting blockjob but at least in
Virtuozzo we have been using virDomainQemuMonitorCommand for several years to
start push backups and relay on behaviour that libvirt sends block job events
even in this case. Thus let's continue to send notifications if block job
object is not found.

Note that we can have issue on reconnect when block jobs are not yet created
but this will addressed in following patches.

Signed-off-by: Nikolay Shirokovskiy 
---
 src/qemu/qemu_blockjob.c |  2 +-
 src/qemu/qemu_blockjob.h |  7 +++
 src/qemu/qemu_driver.c   | 17 -
 src/qemu/qemu_process.c  | 19 +++
 4 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 2a5a5e6..cf3939d 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -596,7 +596,7 @@ qemuBlockJobRefreshJobs(virQEMUDriverPtr driver,
  * Emits the VIR_DOMAIN_EVENT_ID_BLOCK_JOB and VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2
  * for a block job. The former event is emitted only for local disks.
  */
-static void
+void
 qemuBlockJobEmitEvents(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainDiskDefPtr disk,
diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h
index 9f73a35..5edb459 100644
--- a/src/qemu/qemu_blockjob.h
+++ b/src/qemu/qemu_blockjob.h
@@ -245,3 +245,10 @@ qemuBlockJobGetByDisk(virDomainDiskDefPtr disk)
 
 qemuBlockjobState
 qemuBlockjobConvertMonitorStatus(int monitorstatus);
+
+void
+qemuBlockJobEmitEvents(virQEMUDriverPtr driver,
+   virDomainObjPtr vm,
+   virDomainDiskDefPtr disk,
+   virDomainBlockJobType type,
+   virConnectDomainEventBlockJobStatus status);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 05f8eb2..25466f6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4116,9 +4116,7 @@ processSerialChangedEvent(virQEMUDriverPtr driver,
 static void
 processBlockJobEvent(virQEMUDriverPtr driver,
  virDomainObjPtr vm,
- const char *diskAlias,
- int type,
- int status)
+ const char *diskAlias)
 {
 virDomainDiskDefPtr disk;
 g_autoptr(qemuBlockJobData) job = NULL;
@@ -4137,14 +4135,10 @@ processBlockJobEvent(virQEMUDriverPtr driver,
 }
 
 if (!(job = qemuBlockJobDiskGetJob(disk))) {
-VIR_DEBUG("creating new block job object for '%s'", diskAlias);
-if (!(job = qemuBlockJobDiskNew(vm, disk, type, diskAlias)))
-goto endjob;
-job->state = QEMU_BLOCKJOB_STATE_RUNNING;
+VIR_DEBUG("disk %s job not found", diskAlias);
+goto endjob;
 }
 
-job->newstate = status;
-
 qemuBlockJobUpdate(vm, job, QEMU_ASYNC_JOB_NONE);
 
  endjob:
@@ -4319,10 +4313,7 @@ static void qemuProcessEventHandler(void *data, void 
*opaque)
   processEvent->action);
 break;
 case QEMU_PROCESS_EVENT_BLOCK_JOB:
-processBlockJobEvent(driver, vm,
- processEvent->data,
- processEvent->action,
- processEvent->status);
+processBlockJobEvent(driver, vm, processEvent->data);
 break;
 case QEMU_PROCESS_EVENT_JOB_STATUS_CHANGE:
 processJobStatusChangeEvent(driver, vm, processEvent->data);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 1963de9..a810f38 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -953,13 +953,18 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon 
G_GNUC_UNUSED,
 if (!(disk = qemuProcessFindDomainDiskByAliasOrQOM(vm, diskAlias, NULL)))
 goto cleanup;
 
-job = qemuBlockJobDiskGetJob(disk);
+if (!(job = qemuBlockJobDiskGetJob(disk))) {
+VIR_DEBUG("block job for '%s' not found", diskAlias);
+qemuBlockJobEmitEvents(driver, vm, disk, type, status);
+goto cleanup;
+}
 
-if (job && job->synchronous) {
-/* We have a SYNC API waiting for this event, dispatch it back */
-job->newstate = status;
-VIR_FREE(job->errmsg);
-job->errmsg = g_strdup(error);
+job->newstate = status;
+VIR_FREE(job->errmsg);
+job->errmsg = g_strdup(error);
+
+/* We have a SYNC API waiting for this event, dispatch it back */
+if (job->synchronous) {
 virDomainObjBroadcast(vm);
 } else {

[PATCH v2 00/10] qemu: add option to process offloaded legacy blockjob event ealier

[Nikolay Shirokovskiy]

This is successor to [1] but I changed the subject as in the review the patch
'qemu: sync backing chain update in virDomainGetBlockJobInfo' was not
considered good one from design POV. However I think the basic patch is helpful
to address similar issues. Look at [*] for example, there it allows to sync
backing chains during query block stats.

In discussion of [1] I stated that first patch will also allow to get rid of
stale block job events on reconnection to qemu. But this will require
additional work actually so with this patch series stale events are still
present. And in the discussion it was also mentioned by Peter that stale events
are not harmful for legacy blockjobs code. I also removed previous attempts to
eliminate some of stale events.

I still keep patch for virDomainGetBlockJobInfo. May be in comparsion to [*]
the approach the patch takes will look not so bad :)

[1] First version of the patch series
https://www.redhat.com/archives/libvir-list/2020-October/msg01133.html

Nikolay Shirokovskiy (10):
  qemu: add option to process offloaded legacy blockjob event ealier
  qemu: reconnect: precreate legacy blockjobs
  qemu: remove extra block job finalize on reconnect
  qemu: remove stale cleanup in qemuProcessRefreshLegacyBlockjob
  qemu: add note for outdated legacy block job events
  qemu: use autoptr in qemuProcessRefreshLegacyBlockjobs
  qemu: refresh backing chain after block job reconnection
  qemu: move code that depends on backing chain appropriately
  qemu: fix race on legacy block completion and quering stats [*]
  qemu: sync backing chain update in virDomainGetBlockJobInfo

 src/qemu/qemu_blockjob.c |   2 +-
 src/qemu/qemu_blockjob.h |   7 +++
 src/qemu/qemu_driver.c   |  50 +++-
 src/qemu/qemu_process.c  | 149 ---
 4 files changed, 132 insertions(+), 76 deletions(-)

-- 
1.8.3.1

Re: [PATCH v1] wireshark: fix build with with 2.4.0

[Daniel P . Berrangé]

On Fri, Nov 13, 2020 at 11:47:30AM +0100, Olaf Hering wrote:
> Am Fri, 13 Nov 2020 10:46:15 +
> schrieb Daniel P. Berrangé :
> 
> > What platform still ships this old 2.4.0 version ?
> 
> This version is what meson.build expects. SLE12 has it.

SLE12 is no longer considered a supported platform by libvirt, since it
has been more than 2 years since the release of SLE15.

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 v1] wireshark: fix build with with 2.4.0

[Olaf Hering]

Am Fri, 13 Nov 2020 10:46:15 +
schrieb Daniel P. Berrangé :

> What platform still ships this old 2.4.0 version ?

This version is what meson.build expects. SLE12 has it.

Olaf


pgpxs4hCINzfs.pgp
Description: Digitale Signatur von OpenPGP

Re: [PATCH v1] wireshark: fix build with with 2.4.0

[Daniel P . Berrangé]

On Fri, Nov 13, 2020 at 11:43:29AM +0100, Olaf Hering wrote:
> wireshark/epan/proto.h uses WS_NORETURN, which is defined in 
> wireshark/config.h,
> without including this header first.

What platform still ships this old 2.4.0 version ?

> 
> Fixes commit caa9560c150b3df46965582388d0a8a0bafa97ae
> 
> Signed-off-by: Olaf Hering 
> ---
>  tools/wireshark/src/packet-libvirt.c | 1 +
>  1 file changed, 1 insertion(+)

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

[PATCH v1] wireshark: fix build with with 2.4.0

[Olaf Hering]

wireshark/epan/proto.h uses WS_NORETURN, which is defined in wireshark/config.h,
without including this header first.

Fixes commit caa9560c150b3df46965582388d0a8a0bafa97ae

Signed-off-by: Olaf Hering 
---
 tools/wireshark/src/packet-libvirt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/wireshark/src/packet-libvirt.c 
b/tools/wireshark/src/packet-libvirt.c
index f43919b05d..5bddeb3763 100644
--- a/tools/wireshark/src/packet-libvirt.c
+++ b/tools/wireshark/src/packet-libvirt.c
@@ -18,6 +18,7 @@
  */
 #include 
 
+#include 
 #include 
 #include 
 #include 

Re: [PATCH 2/2] qemu: Check if driver is still available in qemuStateStop()

[Nikolay Shirokovskiy]

On 12.11.2020 21:45, Michal Privoznik wrote:
> When the host is shutting down then we get PrepareForShutdown
> signal on DBus to which we react by creating a thread which
> runs virStateStop() and thus qemuStateStop(). But if scheduling
> the thread is delayed just a but it may happen that we receive
> SIGTERM (sent by systemd) to which we respond by quitting our
> event loop and cleaning up everything (including drivers). And
> only after that the thread gets to run only to find qemu_driver
> being NULL. At this point there is nothing left to do anyways,
> the event loop is gone so no API call that qemuStateStop() does
> in attempt to save running domains can ever succeed.
> 
> But to be fair, if there was a domain running we would have
> registered shutdown inhibitor so we would not get killed by
> signal. So there is nothing left to do for qemuStateStop()
> anyway.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1895359
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_driver.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 05f8eb2cb7..9aa0ce4ec8 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1046,7 +1046,12 @@ qemuStateStop(void)
>  int state;
>  virDomainPtr *domains = NULL;
>  g_autofree unsigned int *flags = NULL;
> -g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(qemu_driver);
> +g_autoptr(virQEMUDriverConfig) cfg = NULL;
> +
> +if (!qemu_driver)
> +return -1;
> +
> +cfg = virQEMUDriverGetConfig(qemu_driver);
>  
>  if (!(conn = virConnectOpen(cfg->uri)))
>  goto cleanup;
> 

Hi, Michal.

There is a machinery to handle such issues now in libvirt. 
https://www.redhat.com/archives/libvir-list/2020-November/msg00667.html

Nikolay

Re: Races / crashes in shutdown of libvirtd daemon

[Nikolay Shirokovskiy]

On 12.11.2020 20:12, Michal Privoznik wrote:
> On 4/27/20 5:54 PM, Daniel P. Berrangé wrote:
>> We got a new BZ filed about a libvirtd crash in shutdown
>>
>>    https://bugzilla.redhat.com/show_bug.cgi?id=1828207
> 
> And there is another one:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1895359
> 
> The problem is that when host is shutting down we get PrepareForShutdown() 
> signal on dbus and spawn a thread that will eventually call qemuStateStop(). 
> But before it gets a chance to run the main thread quits the event loop and 
> calls qemuStateCleanup() freeing the qemu driver. After all this the dbus 
> signal handling thread gets to run only to find qemu_driver=0x0 and thus 
> crash.
> 
> From the fact that the event loop quit I deduct that we were sent a SIGTERM 
> which means there was no guest running otherwise we would inhibit the 
> shutdown, so qemuStateStop() would be a NOP anyway. So maybe the fix for this 
> particular case indeed is to check whether qemu_driver == 0x0.
> 
> Anyway, I think your idea is sound.
> 


Hi, Michal.

I added code to handle shutdown as Dan proposed in [1]. It handles most busy 
threads which are rpc and worker and per-VM event loop
threads in qemu driver. But there are still other drivers and short living 
threads here and there [2]. So thread that spawns 
daemonStop is on of these untamed threads.

So we need to join this thread in daemonShutdownWait.

[1] [PATCH v2 00/13] resolve hangs/crashes on libvirtd shutdown
https://www.redhat.com/archives/libvir-list/2020-July/msg01606.html
[2] Re: [PATCH v2 00/13] resolve hangs/crashes on libvirtd shutdown
https://www.redhat.com/archives/libvir-list/2020-September/msg00319.html

Nikolay