August 2018 - Devel - libvirt List Archives

[libvirt] [PATCH] qemu: qemuDomainChangeNet: validity checks should be done before XML autocompletion
by Katerina Koukiou 06 Aug '18

06 Aug '18

We need to first perform the checks for changed/missing elements and then we can overwrite the missing ones. Otherwise we might falsely report successfull update, because some elements got overwritten before the validity checks. https://bugzilla.redhat.com/show_bug.cgi?id=1599513 Signed-off-by: Katerina Koukiou <kkoukiou(a)redhat.com> --- src/qemu/qemu_hotplug.c | 43 ++++++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1488f0a7c2..8d98c149e2 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3445,23 +3445,9 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, goto cleanup; } - /* info: if newdev->info is empty, fill it in from olddev, - * otherwise verify that it matches - nothing is allowed to - * change. (There is no helper function to do this, so - * individually check the few feidls of virDomainDeviceInfo that - * are relevant in this case). + /* info: Nothing is allowed to change. First check for changes and + * then fill the missing newdev->info from olddev. */ - if (!virDomainDeviceAddressIsValid(&newdev->info, - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) && - virDomainDeviceInfoCopy(&newdev->info, &olddev->info) < 0) { - goto cleanup; - } - if (!virPCIDeviceAddressEqual(&olddev->info.addr.pci, - &newdev->info.addr.pci)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("cannot modify network device guest PCI address")); - goto cleanup; - } /* grab alias from olddev if not set in newdev */ if (!newdev->info.alias && VIR_STRDUP(newdev->info.alias, olddev->info.alias) < 0) @@ -3469,26 +3455,51 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, /* device alias is checked already in virDomainDefCompatibleDevice */ + if (newdev->info.rombar == VIR_TRISTATE_BOOL_ABSENT) + newdev->info.rombar = olddev->info.rombar; if (olddev->info.rombar != newdev->info.rombar) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("cannot modify network device rom bar setting")); goto cleanup; } + + if (!newdev->info.romfile && + VIR_STRDUP(newdev->info.romfile, olddev->info.romfile) < 0) + goto cleanup; if (STRNEQ_NULLABLE(olddev->info.romfile, newdev->info.romfile)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("cannot modify network rom file")); goto cleanup; } + + if (newdev->info.bootIndex == 0) + newdev->info.bootIndex = olddev->info.bootIndex; if (olddev->info.bootIndex != newdev->info.bootIndex) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("cannot modify network device boot index setting")); goto cleanup; } + + if (newdev->info.romenabled == VIR_TRISTATE_BOOL_ABSENT) + newdev->info.romenabled = olddev->info.romenabled; if (olddev->info.romenabled != newdev->info.romenabled) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("cannot modify network device rom enabled setting")); goto cleanup; } + + /* if pci addr is missing or is invalid we overwrite all info struct */ + if (!virDomainDeviceAddressIsValid(&newdev->info, + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) && + virDomainDeviceInfoCopy(&newdev->info, &olddev->info) < 0) { + goto cleanup; + } + if (!virPCIDeviceAddressEqual(&olddev->info.addr.pci, + &newdev->info.addr.pci)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("cannot modify network device guest PCI address")); + goto cleanup; + } /* (end of device info checks) */ if (STRNEQ_NULLABLE(olddev->filter, newdev->filter) || -- 2.17.1

2 1

[libvirt] [PATCH] qemu: emit error when trying to update blkiotune group_name in qemuDomainChangeDiskLive
by Katerina Koukiou 06 Aug '18

06 Aug '18

All rest of blkiotune parameters are not updatable through UpdateDeviceFlags API. https://bugzilla.redhat.com/show_bug.cgi?id=1601677 Signed-off-by: Katerina Koukiou <kkoukiou(a)redhat.com> --- The BZ was requesting to add support for updating the group_name for blkdeviotune here, though, all the rest of blkdeviotune elements are not updatable though UpdateDeviceFlags API. I think emmiting error and keeping all blkiodevtune parameters updatable in a consistent way is the right way to go. src/qemu/qemu_domain.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index de056272e8..0aa440e0b5 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8791,6 +8791,9 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, CHECK_EQ(blkdeviotune.size_iops_sec, "blkdeviotune size_iops_sec", true); + CHECK_EQ(blkdeviotune.group_name, + "blkdeviotune group_name", + true); if (disk->serial && STRNEQ_NULLABLE(disk->serial, orig_disk->serial)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, -- 2.17.1

2 1

[libvirt] [RFC v2 1/2] process: wait longer on kill per assigned Hostdev
by Christian Ehrhardt 06 Aug '18

06 Aug '18

It was found that in cases with host devices virProcessKillPainfully might be able to send signal zero to the target PID for quite a while with the process already being gone from /proc/<PID>. That is due to cleanup and reset of devices which might include a secondary bus reset that on top of the actions taken has a 1s delay to let the bus settle. Due to that guests with plenty of Host devices could easily exceed the default timeouts. To solve that, this adds an extra delay of 2s per hostdev that is associated to a VM. Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com> --- src/qemu/qemu_process.c | 5 +++-- src/util/virprocess.c | 18 +++++++++++++++--- src/util/virprocess.h | 1 + 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index c4e33723d1..c20cc11e51 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6814,8 +6814,9 @@ qemuProcessKill(virDomainObjPtr vm, unsigned int flags) return 0; } - ret = virProcessKillPainfully(vm->pid, - !!(flags & VIR_QEMU_PROCESS_KILL_FORCE)); + ret = virProcessKillPainfullyDelay(vm->pid, + !!(flags & VIR_QEMU_PROCESS_KILL_FORCE) + vm->def->nhostdevs * 2); return ret; } diff --git a/src/util/virprocess.c b/src/util/virprocess.c index f92b0dce37..aefe655771 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -344,15 +344,19 @@ int virProcessKill(pid_t pid, int sig) * Returns 0 if it was killed gracefully, 1 if it * was killed forcibly, -1 if it is still alive, * or another error occurred. + * + * Callers can proide an extra delay to wait longer + * than the default. */ int -virProcessKillPainfully(pid_t pid, bool force) +virProcessKillPainfullyDelay(pid_t pid, bool force, unsigned int extradelay) { size_t i; int ret = -1; + unsigned int delay = 75 + (extradelay*5); const char *signame = "TERM"; - VIR_DEBUG("vpid=%lld force=%d", (long long)pid, force); + VIR_DEBUG("vpid=%lld force=%d delay=%u", (long long)pid, force, pid); /* This loop sends SIGTERM, then waits a few iterations (10 seconds) * to see if it dies. If the process still hasn't exited, and @@ -360,9 +364,12 @@ virProcessKillPainfully(pid_t pid, bool force) * wait up to 5 seconds more for the process to exit before * returning. * + * An extra delay can be specified for cases that are expected to clean + * up slower than usual. + * * Note that setting @force could result in dataloss for the process. */ - for (i = 0; i < 75; i++) { + for (i = 0; i < delay; i++) { int signum; if (i == 0) { signum = SIGTERM; /* kindly suggest it should exit */ @@ -405,6 +412,11 @@ virProcessKillPainfully(pid_t pid, bool force) } +int virProcessKillPainfully(pid_t pid, bool force) +{ + return virProcessKillPainfullyDelay(pid, force, 0); +} + #if HAVE_SCHED_GETAFFINITY int virProcessSetAffinity(pid_t pid, virBitmapPtr map) diff --git a/src/util/virprocess.h b/src/util/virprocess.h index 3c5a882772..b72603ca8e 100644 --- a/src/util/virprocess.h +++ b/src/util/virprocess.h @@ -55,6 +55,7 @@ virProcessWait(pid_t pid, int *exitstatus, bool raw) int virProcessKill(pid_t pid, int sig); int virProcessKillPainfully(pid_t pid, bool force); +int virProcessKillPainfullyDelay(pid_t pid, bool force, unsigned int); int virProcessSetAffinity(pid_t pid, virBitmapPtr map); -- 2.17.1

1 1

[libvirt] [RFC 0/2] Fix detection of slow guest shutdown
by Christian Ehrhardt 06 Aug '18

06 Aug '18

Hi, I was recently looking into a case which essentially looked like this: 1. virsh shutdown guest 2. after <1 second the qemu process was gone from /proc/ 3. but libvirt spun in virProcessKillPainfully because the process was still reachable via signals 4. virProcessKillPainfully eventually fails after 15 seconds and the guest stays in "in shutdown" state forever This is not one of the common cases I've found for virProcessKillPainfully to break: - bad I/O e.g. NFS gets qemu stuck - CPU overload stalls things to death - qemu not being reaped (by init) All of the above would have the process still available in /proc/<pid> as Zombie or in uninterruptible sleep, but that is not true in my case. It turned out that the case was dependent on the amount of hostdev resources passed to the guest. Debugging showed that with 8 and more likely 16 GPUs passed it took ~18 seconds from SIGTERM to "no more be reachable with signal 0". I haven't conducted much more tests but stayed on the 16 GPU case, but I'm rather sure more devices might make it take even longer. Discussion with a few kernel folks revealed that the kill(2) man page on signal 0 has to be taken very literal "check for the existence of a process ID" - you can read this as "the PID exists, but the Process is no more". I'm unsure why the kernel would take that much time to clean up as I thought taking away /proc/<PID> would be almost the last step in the cleanup of a task. patch 2: I happened to find that there seems to be no much better way than signal-0 to check, but finding that this isn't reliable if the kernel can still accept for quite some time even with the pid gone from all other interfaces that I could find - so I wanted to suggest a fallback in virProcessKillPainfully that considers the absence of /proc/<pid> as a valid "the process is gone" as well on top of the ESRCH of signal-0. We could also use the open FDs we have e.g. to the qemu monitor to see if the remote end is dead, but that didn't seem more readable/reliable to me and would have to cross quite some code to know about the FDs. But maybe someone else here has the insight what exactly would take the time in the kernel that I see and that might bring us to totally different solutions (therefore RFC). patch 1: Finally after working through this code for a while I got the feeling that if we are in a bad/non-responsive case after 10 seconds upgrading to SIGKILL we should give it some more time to take effect. We reach this in stressful cases only anyway and only if force is set, so then waiting a bit more helps to resolve some of the other cases that I found on the mailing list about virProcessKillPainfully being stuck. If one has a personal interest in the 15 seconds we had before lets add a VIR_WARN on 15 seconds if that would be better, but overall wait a bit more. P.S. Afer a short discussion with Daniel on IRC I'm also adding Alex explicitly for passthrough experience. P.P.S.: For now this really is only meant as an RFC to kick off the discussion. I got taken away the system that I could trigger this case easily on before I could complete a final verification. But the case is interesting enough to start the discussion now. Christian Ehrhardt (2): process: wait longer 5->30s on hard shutdown process: accept the lack of /proc/<pid> as valid process removal src/util/virprocess.c | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) -- 2.17.1

4 7

[libvirt] [PATCH v3 0/4] iscsi-direct: first part
by clem＠lse.epita.fr 06 Aug '18

06 Aug '18

From: Clementine Hayat <clem(a)lse.epita.fr> Hello, This is the implementation of the iscsi-direct backend storage pool version 3. v1: https://www.redhat.com/archives/libvir-list/2018-July/msg00918.html v2: https://www.redhat.com/archives/libvir-list/2018-July/msg01528.html Best Regards, -- Clementine Hayat Clementine Hayat (4): configure: Introduce libiscsi in build system storage: Introduce iscsi_direct pool type storage: Implement iscsi_direct pool backend news: add storage pool iscsi-direct configure.ac | 9 +- docs/news.xml | 9 + docs/schemas/storagepool.rng | 35 ++ docs/storage.html.in | 30 ++ m4/virt-libiscsi.m4 | 30 ++ m4/virt-storage-iscsi-direct.m4 | 44 ++ src/conf/domain_conf.c | 1 + src/conf/storage_conf.c | 22 +- src/conf/storage_conf.h | 1 + src/conf/virstorageobj.c | 2 + src/storage/Makefile.inc.am | 24 + src/storage/storage_backend.c | 6 + src/storage/storage_backend_iscsi_direct.c | 452 ++++++++++++++++++ src/storage/storage_backend_iscsi_direct.h | 6 + src/storage/storage_driver.c | 1 + .../pool-iscsi-direct-auth.xml | 14 + .../pool-iscsi-direct.xml | 12 + .../pool-iscsi-direct-auth.xml | 17 + .../pool-iscsi-direct.xml | 14 + tests/storagepoolxml2xmltest.c | 2 + tools/virsh-pool.c | 3 + 21 files changed, 728 insertions(+), 6 deletions(-) create mode 100644 m4/virt-libiscsi.m4 create mode 100644 m4/virt-storage-iscsi-direct.m4 create mode 100644 src/storage/storage_backend_iscsi_direct.c create mode 100644 src/storage/storage_backend_iscsi_direct.h create mode 100644 tests/storagepoolxml2xmlin/pool-iscsi-direct-auth.xml create mode 100644 tests/storagepoolxml2xmlin/pool-iscsi-direct.xml create mode 100644 tests/storagepoolxml2xmlout/pool-iscsi-direct-auth.xml create mode 100644 tests/storagepoolxml2xmlout/pool-iscsi-direct.xml -- 2.18.0

4 8

[libvirt] [RFC] secdrivers remembering original labels
by Michal Privoznik 06 Aug '18

06 Aug '18

Dear list, we have this very old bug [1] that I just keep pushing in front of me. I made several attempts to fix it. However, none of them made into the tree. I guess it's time to have discussion what to do about it. IIRC, the algorithm that I implemented last was to keep original label in XATTRs (among with some ref counter) and the last one to restore the label will look there and find the original label. There was a problem with two libvirtds fighting over a file on shared FS. So I guess my question is can we come up with a design that would work? Or at least work to the extent that we're satisfied with? Personally, I like the XATTRs approach. And to resolve the NFS race we can invent yet another lockspace to guard labelling - I also have a bug for that [2] (although, I'm not that familiar with lockspaces). I guess doing disk metadata locking is not going to be trivial, is it? Michal 1: https://bugzilla.redhat.com/show_bug.cgi?id=547546 2: https://bugzilla.redhat.com/show_bug.cgi?id=1524792

3 7

[libvirt] Release of libvirt-4.6.0
by Daniel Veillard 06 Aug '18

06 Aug '18

Okay it took a bit longer than usual but the relrase is out, it's tagged in git and signed tarball and rpms have been pushed to the usual place, but there was quite a few bug fixes going in after RC2 was cut. ftp://libvirt.org/libvirt/ I also made libvirt-python 4.6.0 bindings release in a similar fashion available at : ftp://libvirt.org/libvirt/python/ So though the release note don't list any bug fixes we know there is a batch of those in, otherwise this is mainly features and improvement targetting QEmu and virsh users: New features: - qemu: Implement the HTM pSeries feature Users can now decide whether HTM (Hardware Transactional Memory) support should be available to the guest. - qemu: Enable VNC console for mediated devices Host devices now support a new atribute 'display' which can be used to turn on frame buffer rendering on a vgpu mediated device instead of on an emulated GPU, like QXL. Improvements: - qemu: Introduce a new video model of type 'none' Introduce a new video model type that disables the automatic addition of a video device to domains with 'graphics' specified in their XML. This can be useful with GPU mediated devices which can serve as the only rendering devices within the guest. - virsh: Add --alias to attach-disk and attach-interface commands Add option --alias to set customized device alias name when using attach-disk or attach-interface commands. - virsh: Support usb and sata address to attach-disk Usb or sata address could be used when attach-disk with --address. For example, use usb address as usb:<bus>.<port>, use sata address as <controller>.<bus>.<unit>. Thanks everybody for your help with this release, hopefully the next one beginning of Sept will be more timely ! Enjoy, Daniel -- Daniel Veillard | Red Hat Developers Tools http://developer.redhat.com/ veillard(a)redhat.com | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | virtualization library http://libvirt.org/

1 0

[libvirt] [PATCH] Fix include for xattr.h
by Martin Kletzander 06 Aug '18

06 Aug '18

The proper file that should be included is `sys/xattr.h` as that comes from `glibc` and not `attr/xattr.h` which ships with the `attr` utility. We're most probably not the only ones because `attr/xattr.h` added a #warning to their include resulting in the following compilation errors: In file included from securityselinuxlabeltest.c:31:0: /usr/include/attr/xattr.h:5:2: error: #warning "Please change your <attr/xattr.h> includes to <sys/xattr.h>" [-Werror=cpp] #warning "Please change your <attr/xattr.h> includes to <sys/xattr.h>" ^~~~~~~ In file included from securityselinuxhelper.c:37:0: /usr/include/attr/xattr.h:5:2: error: #warning "Please change your <attr/xattr.h> includes to <sys/xattr.h>" [-Werror=cpp] #warning "Please change your <attr/xattr.h> includes to <sys/xattr.h>" ^~~~~~~ Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com> --- Not pushing as build-breaker because we are still in freeze and I'd still like someone with SELinux to check this patch. My VM with such system stopped working just now =( m4/virt-attr.m4 | 2 +- tests/securityselinuxhelper.c | 2 +- tests/securityselinuxlabeltest.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/m4/virt-attr.m4 b/m4/virt-attr.m4 index c55bd7dbd823..478549c17f12 100644 --- a/m4/virt-attr.m4 +++ b/m4/virt-attr.m4 @@ -5,7 +5,7 @@ AC_DEFUN([LIBVIRT_ARG_ATTR],[ ]) AC_DEFUN([LIBVIRT_CHECK_ATTR],[ - LIBVIRT_CHECK_LIB([ATTR], [attr], [getxattr], [attr/xattr.h]) + LIBVIRT_CHECK_LIB([ATTR], [attr], [getxattr], [sys/xattr.h]) ]) AC_DEFUN([LIBVIRT_RESULT_ATTR],[ diff --git a/tests/securityselinuxhelper.c b/tests/securityselinuxhelper.c index 22b6709554b4..fe6f2b5fd9b0 100644 --- a/tests/securityselinuxhelper.c +++ b/tests/securityselinuxhelper.c @@ -34,7 +34,7 @@ #include <string.h> #include <sys/vfs.h> #include <unistd.h> -#include <attr/xattr.h> +#include <sys/xattr.h> #ifndef NFS_SUPER_MAGIC # define NFS_SUPER_MAGIC 0x6969 diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c index c684989fd26e..48fee7cd285b 100644 --- a/tests/securityselinuxlabeltest.c +++ b/tests/securityselinuxlabeltest.c @@ -28,7 +28,7 @@ #include <selinux/selinux.h> #include <selinux/context.h> -#include <attr/xattr.h> +#include <sys/xattr.h> #include "internal.h" #include "testutils.h" -- 2.18.0

2 1

[libvirt] [PATCH] virrandom: Avoid undefined behaviour in virRandomBits
by Michal Privoznik 06 Aug '18

06 Aug '18

If nbits is 64 (or greater) then shifting 1ULL left is undefined. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- src/util/virrandom.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/util/virrandom.c b/src/util/virrandom.c index 3c011a8615..7915f6531e 100644 --- a/src/util/virrandom.c +++ b/src/util/virrandom.c @@ -68,7 +68,9 @@ uint64_t virRandomBits(int nbits) return 0; } - ret &= (1ULL << nbits) - 1; + if (nbits < 64) + ret &= (1ULL << nbits) - 1; + return ret; } -- 2.16.4

2 1

[libvirt] [PATCH] remote: daemon: Make sure that JSON symbols are properly loaded at startup
by Peter Krempa 06 Aug '18

06 Aug '18

Explicitly call virJSONInitialize at startup of the libvirt daemon so that we are sure that the symbols in the compat library are properly loaded. This will prevent any random failure from happening later on when the daemon would want to use the JSON parser. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com> --- src/libvirt_private.syms | 4 ++++ src/remote/remote_daemon.c | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fc386e1699..70dfcc5e29 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2135,6 +2135,10 @@ virJSONValueObjectStealObject; virJSONValueToString; +# util/virjsoncompat.h +virJSONInitialize; + + # util/virkeycode.h virKeycodeSetTypeFromString; virKeycodeSetTypeToString; diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c index 9f3a5f38ad..8bbc3818bb 100644 --- a/src/remote/remote_daemon.c +++ b/src/remote/remote_daemon.c @@ -59,6 +59,7 @@ #include "virutil.h" #include "virgettext.h" #include "util/virnetdevopenvswitch.h" +#include "virjsoncompat.h" #include "driver.h" @@ -1183,6 +1184,9 @@ int main(int argc, char **argv) { exit(EXIT_FAILURE); } + if (virJSONInitialize() < 0) + exit(EXIT_FAILURE); + daemonSetupNetDevOpenvswitch(config); if (daemonSetupAccessManager(config) < 0) { -- 2.16.2

2 1