November 2020 - Devel - libvirt List Archives

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

[PATCH] qemu: domfsinfo should print a target name on s390x, not a complete device path
by Thomas Huth 13 Nov '20

13 Nov '20

While fixing domfsinfo for non-PCI (i.e. CCW) devices on s390x, I accidentally used the whole device path for the devAlias field. However, it should only contain the base target name. Currently we have the wrong output: $ virsh domfsinfo guestname Mountpoint Name Type Target --------------------------------------- / sda3 xfs /dev/sda3 /boot sda1 xfs /dev/sda1 It should look like this instead: $ virsh domfsinfo guestname Mountpoint Name Type Target ------------------------------------ / sda3 xfs sda /boot sda1 xfs sda Thus we have to strip the "/dev/" prefix and the partition number from the string. Fixes: f8333b3b0a ("qemu: Fix domfsinfo for non-PCI device information ...") Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1858771 Reported-by: Sebastian Mitterle <smitterl(a)redhat.com> --- src/qemu/qemu_driver.c | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 05f8eb2cb7..d92bee1d35 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18874,6 +18874,30 @@ qemuDomainGetFSInfoAgent(virQEMUDriverPtr driver, return ret; } +/* Turn device node string like "/dev/vda1" into a target name like "vda" */ +static char * +qemuAgentDevNodeToTarget(const char *devnode) +{ + char *str = g_strdup(devnode); + size_t len = strlen(str); + + /* Remove the "/dev/" prefix from the string */ + if (g_str_has_prefix(str, "/dev/")) { + len -= 5; + memmove(str, str + 5, len + 1); + } + + /* Remove the partition number from the end of the string */ + while (len > 0) { + len--; + if (!g_ascii_isdigit(str[len])) + break; + str[len] = 0; + } + + return str; +} + static virDomainFSInfoPtr qemuAgentFSInfoToPublic(qemuAgentFSInfoPtr agent, virDomainDefPtr vmdef) @@ -18903,7 +18927,7 @@ qemuAgentFSInfoToPublic(qemuAgentFSInfoPtr agent, if (diskDef != NULL) ret->devAlias[i] = g_strdup(diskDef->dst); else if (agentdisk->devnode != NULL) - ret->devAlias[i] = g_strdup(agentdisk->devnode); + ret->devAlias[i] = qemuAgentDevNodeToTarget(agentdisk->devnode); else VIR_DEBUG("Missing devnode name for '%s'.", ret->mountpoint); } -- 2.18.4

2 2

[PATCH 0/2] qemu: Check if driver is still available in qemuStateStop()
by Michal Privoznik 13 Nov '20

13 Nov '20

The second patch is the interesting one. Michal Prívozník (2): DO NOT MERGE qemu: Check if driver is still available in qemuStateStop() src/qemu/qemu_driver.c | 7 ++++- src/remote/remote_daemon.c | 64 +++++++++++++++++++++----------------- 2 files changed, 42 insertions(+), 29 deletions(-) -- 2.26.2

2 3

[libvirt PATCH] util: Make virFileClose() quiet on success
by Andrea Bolognani 12 Nov '20

12 Nov '20

While it's certainly good to log events like "failed to close fd" and "tried to close invalid fd", which are likely to be the consequence of some bug in libvirt, logging a message every single time a file descriptor is closed successfully is perhaps excessive and can lead to useful information being missed among the noise. Log filters don't help in this situation, because filtering out all of util.file is too big a hammer and would cause important messages to be left out as well. To give an idea of just how much noise this single debug statement can cause, here's a real life example from a quite large libvirtd log I had to look at recently: $ grep virFile libvirt.log | wc -l 1307 $ grep virFile libvirt.log | grep -v 'Closed fd' | wc -l 343 Signed-off-by: Andrea Bolognani <abologna(a)redhat.com> --- src/util/virfile.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 9a7dcfc9a6..f57272ca2f 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -131,8 +131,6 @@ int virFileClose(int *fdptr, virFileCloseFlags flags) VIR_DEBUG("Failed to close fd %d: %s", *fdptr, g_strerror(errno)); } - } else { - VIR_DEBUG("Closed fd %d", *fdptr); } } *fdptr = -1; -- 2.26.2

2 1

[PATCH v2 0/6] hyperv: implement vCPU APIs
by Matt Coleman 12 Nov '20

12 Nov '20

Changes since v1: * use an enum for the real CPU constants * remove the forward declaration Here's a GitLab merge request, if you'd prefer to review it there: https://gitlab.com/iammattcoleman/libvirt/-/merge_requests/11 Matt Coleman (6): domain: introduce constants for virVcpuInfo->cpu state values hyperv: implement domainGetVcpus hyperv: implement domainGetVcpusFlags hyperv: implement domainGetMaxVcpus hyperv: implement domainSetVcpus and domainSetVcpusFlags news: implement vCPU APIs in the Hyper-V driver NEWS.rst | 8 +- include/libvirt/libvirt-domain.h | 7 +- src/hyperv/hyperv_driver.c | 175 +++++++++++++++++++++++++++++++ 3 files changed, 186 insertions(+), 4 deletions(-) -- 2.27.0

2 7

[PATCH 0/6] hyperv: implement vCPU APIs
by Matt Coleman 12 Nov '20

12 Nov '20

Here's a GitLab merge request, if you'd prefer to review it there: https://gitlab.com/iammattcoleman/libvirt/-/merge_requests/10 Matt Coleman (6): domain: introduce constants for virVcpuInfo->cpu state values hyperv: implement domainGetVcpus hyperv: implement domainGetVcpusFlags hyperv: implement domainGetMaxVcpus hyperv: implement domainSetVcpus and domainSetVcpusFlags news: implement vCPU APIs in the Hyper-V driver NEWS.rst | 8 +- include/libvirt/libvirt-domain.h | 16 ++- src/hyperv/hyperv_driver.c | 180 +++++++++++++++++++++++++++++++ 3 files changed, 200 insertions(+), 4 deletions(-) -- 2.27.0

2 13

[PATCH 0/8] tools: vsh: Fix validation of command parameter lists
by Peter Krempa 12 Nov '20

12 Nov '20

Recent patch: https://www.redhat.com/archives/libvir-list/2020-November/msg00555.html passes build check but then virsh complains when started that the command list is wrong. Fix it by moving the check to the appropriate place. You can use the above patch as reproducer. Peter Krempa (8): tools: vsh: Unexport vshCmddefHelp tools: vshCmddefCheckInternals: Add parameter name to error message tools: vshCmddefCheckInternals: Port mandatory options check from vshCmddefOptParse tools: cmdSelfTest: Drop misleading comment tools: vshCmddefOptParse: Remove 'optional' command validation tools: vshCmddefHelp: Don't call vshCmddefOptParse tools: vshCmddefOptParse: Remove return value tools: virsh: Reset error when keepalive registration fails tools/virsh.c | 1 + tools/vsh.c | 108 +++++++++++++++++++++----------------------------- tools/vsh.h | 1 - 3 files changed, 47 insertions(+), 63 deletions(-) -- 2.28.0

2 11

[PATCH 00/28] a bunch of domain_conf cleanup
by Matt Coleman 12 Nov '20

12 Nov '20

Most of this is making functions void that unnecessarily return an int. It also includes some conversion to GLib. Feel free to squash related commits, if you'd like. I left them separate to make it easier to review. Matt Coleman (28): domain_conf: make virDomainDiskSetDriver() void domain_conf: make virDomainPostParseCheckISCSIPath() void domain_conf: use g_free() in virDomainPostParseCheckISCSIPath() domain_conf: make virDomainHostdevAssignAddress() void domain_conf: make virDomainChr/RNG/Video/VsockDefPostParse() and virDomainNVRAMDefFormat() void domain_conf: make virDomainDeviceInfoFormat() void domain_conf: make virDomainGraphicsDefParseXMLEGLHeadless() void domain_conf: make virDomainLeaseDefFormat() void domain_conf: make virDomainDiskSourceFormatNetwork() void domain_conf: make virDomainDiskDefFormatIotune() void domain_conf: make virDomainDiskDefFormatDriver() void domain_conf: make virDomainControllerDriverFormat() void domain_conf: make virDomainVirtioNetGuestOpts/HostOpts/DriverFormat() void domain_conf: make virDomainRedirFilterDefFormat() void domain_conf: make virDomainIOMMUDefFormat() void domain_conf: make virDomainDefFormatBlkiotune() void domain_conf: make virDomainChrSourceDefFormat() void domain_conf: make virDomainDiskSetBlockIOTune() void domain_conf: use g_free in virDomainDiskSetBlockIOTune() domain_conf: use g_renew in virDomainDiskInsert() and virDomainControllerInsert() domain_conf: make virDomainDiskInsert() void domain_conf: make virDomainControllerInsert() void domain_conf: use g_renew in virDomainLeaseInsertPreAlloc() domain_conf: make virDomainLeaseInsertPreAlloc() void domain_conf: make virDomainLeaseInsert() void domain_conf: make virDomainPanicDefFormat() void domain_conf: make virDomainShmemDefFormat() void domain_conf: make virDomainVsockDefFormat() void src/conf/domain_conf.c | 349 ++++++++++++++------------------------- src/conf/domain_conf.h | 21 +-- src/libxl/libxl_conf.c | 5 +- src/libxl/libxl_domain.c | 5 +- src/libxl/libxl_driver.c | 9 +- src/libxl/xen_xl.c | 12 +- src/libxl/xen_xm.c | 10 +- src/lxc/lxc_driver.c | 3 +- src/qemu/qemu_domain.c | 5 +- src/qemu/qemu_driver.c | 15 +- src/qemu/qemu_hotplug.c | 3 +- src/test/test_driver.c | 3 +- src/vz/vz_sdk.c | 9 +- tests/qemublocktest.c | 5 +- 14 files changed, 158 insertions(+), 296 deletions(-) -- 2.27.0

5 39

[PATCH] qemu: virtiofs: Stop virtiofsd when the qemu guest fails to start
by Masayoshi Mizuma 12 Nov '20

12 Nov '20

From: Masayoshi Mizuma <m.mizuma(a)jp.fujitsu.com> A qemu guest which has virtiofs config fails to start if the previous starting failed because of invalid option or something. For example of the reproduction: # virsh start guest error: Failed to start domain guest error: internal error: process exited while connecting to monitor: qemu-system-x86_64: -foo: invalid option ... fix the option ... # virsh start guest error: Failed to start domain guest error: Cannot open log file: '/var/log/libvirt/qemu/guest-fs0-virtiofsd.log': Device or resource busy # That's because the virtiofsd which was executed on the former staring remains and virtlogd keeps to opening the log file. Stop virtiofsd when the qemu guest fails to start. Signed-off-by: Masayoshi Mizuma <m.mizuma(a)jp.fujitsu.com> --- src/qemu/qemu_domain.h | 1 + src/qemu/qemu_virtiofs.c | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index ca041e207b..7d47c030bd 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -426,6 +426,7 @@ struct _qemuDomainFSPrivate { virObject parent; char *vhostuser_fs_sock; + pid_t virtiofsd_pid; }; diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c index 2e239cad66..8684219915 100644 --- a/src/qemu/qemu_virtiofs.c +++ b/src/qemu/qemu_virtiofs.c @@ -250,6 +250,7 @@ qemuVirtioFSStart(virLogManagerPtr logManager, } QEMU_DOMAIN_FS_PRIVATE(fs)->vhostuser_fs_sock = g_steal_pointer(&socket_path); + QEMU_DOMAIN_FS_PRIVATE(fs)->virtiofsd_pid = pid; ret = 0; cleanup: @@ -273,6 +274,7 @@ qemuVirtioFSStop(virQEMUDriverPtr driver G_GNUC_UNUSED, { g_autofree char *pidfile = NULL; virErrorPtr orig_err; + pid_t pid = QEMU_DOMAIN_FS_PRIVATE(fs)->virtiofsd_pid; virErrorPreserveLast(&orig_err); @@ -286,6 +288,9 @@ qemuVirtioFSStop(virQEMUDriverPtr driver G_GNUC_UNUSED, unlink(QEMU_DOMAIN_FS_PRIVATE(fs)->vhostuser_fs_sock); } + if (virProcessKill(pid, 0) == 0) + virProcessKillPainfully(pid, true); + cleanup: virErrorRestore(&orig_err); } -- 2.27.0

4 5