November 2020 - Devel - libvirt List Archives

[PATCH] qemu: backup: Install bitmap for incremental backup to appropriate node only
by Peter Krempa 13 Nov '20

13 Nov '20

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 <pkrempa(a)redhat.com> --- 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

2 2

[PATCH] migration.html: Fix the spelling of the --persistent parameter
by Thomas Huth 13 Nov '20

13 Nov '20

"--persist" is missing the "ent" at the end. Signed-off-by: Thomas Huth <thuth(a)redhat.com> --- 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 <code>virsh</code> command has two flags to influence this behaviour. The <code>--undefinesource</code> flag will cause the configuration file to be removed on the source host - after a successful migration. The <code>--persist</code> flag will + after a successful migration. The <code>--persistent</code> 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 @@ <th>Source config</th> <th>Dest config</th> <th>--undefinesource</th> - <th>--persist</th> + <th>--persistent</th> <th>Dest type</th> <th>Source config</th> <th>Dest config</th> -- 2.18.4

2 1

[PATCH 0/6] Introduce OpenSSH authorized key file mgmt APIs
by Michal Privoznik 13 Nov '20

13 Nov '20

Marc-André posted a patch that implements agent handling. I've written the rest. Marc-André Lureau (1): qemu_agent: add qemuAgentSSH{Add,Remove,Get}AuthorizedKeys Michal Prívozník (5): Introduce OpenSSH authorized key file mgmt APIs remote: Implement OpenSSH authorized key file mgmt APIs virsh: Expose OpenSSH authorized key file mgmt APIs qemu: Implement OpenSSH authorized key file mgmt APIs news: Document recent OpenSSH authorized key file mgmt APIs NEWS.rst | 6 ++ docs/manpages/virsh.rst | 37 +++++++ include/libvirt/libvirt-domain.h | 17 ++++ src/driver-hypervisor.h | 15 +++ src/libvirt-domain.c | 115 +++++++++++++++++++++ src/libvirt_public.syms | 6 ++ src/qemu/qemu_agent.c | 140 +++++++++++++++++++++++++ src/qemu/qemu_agent.h | 15 +++ src/qemu/qemu_driver.c | 81 +++++++++++++++ src/remote/remote_daemon_dispatch.c | 82 +++++++++++++++ src/remote/remote_driver.c | 87 ++++++++++++++++ src/remote/remote_protocol.x | 34 ++++++- src/remote_protocol-structs | 22 ++++ tests/qemuagenttest.c | 79 +++++++++++++++ tools/virsh-domain.c | 152 ++++++++++++++++++++++++++++ 15 files changed, 887 insertions(+), 1 deletion(-) -- 2.26.2

4 22

[PATCH] migration.html: Fix the spelling of the --undefinesource parameter
by Thomas Huth 13 Nov '20

13 Nov '20

There is no dash between "undefine" and "source" in this parameter. Signed-off-by: Thomas Huth <thuth(a)redhat.com> --- 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 @@ <p> As mentioned above, libvirt will not modify configuration files during migration by default. The <code>virsh</code> command has two flags to - influence this behaviour. The <code>--undefine-source</code> flag + influence this behaviour. The <code>--undefinesource</code> flag will cause the configuration file to be removed on the source host after a successful migration. The <code>--persist</code> flag will cause a configuration file to be created on the destination host @@ -271,7 +271,7 @@ <th>Source type</th> <th>Source config</th> <th>Dest config</th> - <th>--undefine-source</th> + <th>--undefinesource</th> <th>--persist</th> <th>Dest type</th> <th>Source config</th> -- 2.18.4

2 1

[PATCH v2 00/10] qemu: support renaming domains with snapshots/checkpoints
by Nikolay Shirokovskiy 13 Nov '20

13 Nov '20

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 Nikolay Shirokovskiy (10): qemu: qemuDomainRenameCallback: fix sending false undefined event qemu: rename: send events only on success qemu: rename: return instead of goto if no cleanup required qemu: remove duplicate code for removing remnant files qemu: rename: support renaming snapshots directory qemu: rename: support renaming checkpoints directory qemu: update name on reverting from snapshot qemu: rename: remove snapshot/checkpoint restriction qemu: qemuDomainDefineXMLFlags: move cleanup logic to cleanup section qemu: remove possible garbage left from previous rename/undefine src/qemu/qemu_checkpoint.c | 2 +- src/qemu/qemu_checkpoint.h | 6 ++ src/qemu/qemu_domain.c | 43 ++++++++++++ src/qemu/qemu_domain.h | 5 ++ src/qemu/qemu_driver.c | 158 ++++++++++++++++++++++++++------------------- src/qemu/qemu_migration.c | 3 + src/qemu/qemu_snapshot.c | 10 +++ 7 files changed, 160 insertions(+), 67 deletions(-) -- 1.8.3.1

2 15

[PATCH v1] wireshark: fix build with with 2.4.0
by Olaf Hering 13 Nov '20

13 Nov '20

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 <olaf(a)aepfle.de> --- 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 <config.h> +#include <wireshark/config.h> #include <wireshark/epan/proto.h> #include <wireshark/epan/packet.h> #include <wireshark/epan/dissectors/packet-tcp.h>

2 5

[PATCH v1] meson: fix yajl detection
by Olaf Hering 13 Nov '20

13 Nov '20

yajl_tree_parse is declared in yajl/yajl_tree.h autoconf is more forgiving, the error did not trigger because yajl_tree_parse is not actually used. Fixes commit 44b8df4cb4b3f0c143e0330f543812e6bcd5c9f0 Fixes commit 88ab32a4e555f67dae3204fbc3bcf63d3da8ad2b Signed-off-by: Olaf Hering <olaf(a)aepfle.de> --- meson.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/meson.build b/meson.build index 9ee8dffa60..2b2ce6dfb1 100644 --- a/meson.build +++ b/meson.build @@ -1471,7 +1471,7 @@ if not get_option('yajl').disabled() 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 <yajl/yajl_common.h>') + has_func = cc.has_function('yajl_tree_parse', dependencies: yajl_dep, prefix: '#include <yajl/yajl_tree.h>') if not has_func and get_option('yajl').enabled() error('yajl >= @0@ was not found'.format(yajl_version)) elif not has_func

3 2

[PATCH] kbase: Document minimal libvirt version for NUMA-less virtiofs
by Michal Privoznik 13 Nov '20

13 Nov '20

Using virtiofs without NUMA was implemented in v6.9.0-rc1~161 but our kbase document only mentions QEMU version which may confuse users. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- docs/kbase/virtiofs.rst | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/kbase/virtiofs.rst b/docs/kbase/virtiofs.rst index 01440420d7..c0bc07a68d 100644 --- a/docs/kbase/virtiofs.rst +++ b/docs/kbase/virtiofs.rst @@ -25,8 +25,9 @@ means that QEMU needs to allocate the backing memory for all the guest RAM as shared memory. As of QEMU 4.2, it is possible to explicitly specify a memory backend when specifying the NUMA topology. This method is however only viable for machine types that do support -NUMA. As of QEMU 5.0.0, it is possible to specify the memory backend -without NUMA (using the so called memobject interface). +NUMA. As of QEMU 5.0.0 and libvirt 6.9.0, it is possible to +specify the memory backend without NUMA (using the so called +memobject interface). Either of the following: -- 2.26.2

2 2

Races / crashes in shutdown of libvirtd daemon
by Daniel P. Berrangé 13 Nov '20

13 Nov '20

We got a new BZ filed about a libvirtd crash in shutdown https://bugzilla.redhat.com/show_bug.cgi?id=1828207 We can see from the stack trace that the "interface" driver is in the middle of servicing an RPC call for virConnectListAllInterfaces() Meanwhile the libvirtd daemon is doing virObjectUnref(dmn) on the virNetDaemonPtr object. The fact that it is doing this unref, means that it must have already call virStateCleanup(), given the code sequence: /* Run event loop. */ virNetDaemonRun(dmn); ret = 0; virHookCall(VIR_HOOK_DRIVER_DAEMON, "-", VIR_HOOK_DAEMON_OP_SHUTDOWN, 0, "shutdown", NULL, NULL); cleanup: /* Keep cleanup order in inverse order of startup */ virNetDaemonClose(dmn); virNetlinkEventServiceStopAll(); if (driversInitialized) { /* NB: Possible issue with timing window between driversInitialized * setting if virNetlinkEventServerStart fails */ driversInitialized = false; virStateCleanup(); } virObjectUnref(adminProgram); virObjectUnref(srvAdm); virObjectUnref(qemuProgram); virObjectUnref(lxcProgram); virObjectUnref(remoteProgram); virObjectUnref(srv); virObjectUnref(dmn); Unless I'm missing something non-obvious, this cleanup code path is inherantly broken & racy. When virNetDaemonRun() returns the RPC worker threads are all still active. They are all liable to still be executing RPC calls, which means any of the drivers may be in use. So calling virStateCleanup() is an inherantly dangerous thing to do. There is the further complication that once we have exitted the main loop we may prevent the RPC calls from ever completing, as they may be waiting on an event to be dispatched. I know we're had various patch proposals in the past to improve the robustness of shutdown cleanup but I can't remember the outcome of the reviews. Hopefully people involved in those threads can jump in here... IMHO the key problem here is the virNetDeamonRun() method which just looks at the "quit" flag and immediately returns if it is set. This needs to be changed so that when it sees quit == true, it takes the following actions 1. Call virNetDaemonClose() to drop all RPC clients and thus prevent new RPC calls arriving 2. Flush any RPC calls which are queued but not yet assigned to a worker thread 3. Tell worker threads to exit after finishing their current job 4. Wait for all worker threads to exit 5. Now virNetDaemonRun may return At this point we can call virStateCleanup and the various other things, as we know no drivers are still active in RPC calls. Having said that, there could be background threads in the the drivers which are doing work that uses the event loop thread. So we probably need a virStateClose() method that we call from virNetDaemonRun, *after* all worker threads are gone, which would cleanup any background threads while the event loop is still running. The issue is that step 4 above ("Wait for all worker threads to exit") may take too long, or indeed never complete. To deal with this, it will need a timeout. In the remote_daemon.c cleanup code path, if there are still worker threads present, then we need to skip all cleanup and simply call _exit(0) to terminate the process with no attempt at cleanup, since it would be unsafe to try anything else. 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 :|

3 4

[PATCH 0/2] char: Deprecate backend aliases
by Kevin Wolf 13 Nov '20

13 Nov '20

These aliases only work the command line, but not in QMP. Command line QAPIfication involves writing some compatibility glue for them, which I'm doing, but I think it's desirable to unify accepted values of both paths. So deprecate the aliases so that we can drop the compatibility glue later. In the deprecation documentation I assumed that this is for 6.0, but if we want to include it in 5.2 still, this can be changed, of course. Kevin Wolf (2): char: Skip CLI aliases in query-chardev-backends char: Deprecate backend aliases 'tty' and 'parport' docs/system/deprecated.rst | 6 ++++++ chardev/char.c | 32 ++++++++++++++++++++++++-------- 2 files changed, 30 insertions(+), 8 deletions(-) -- 2.28.0

3 6