- Devel - libvirt List Archives

[libvirt] [PATCH v2] qemu: Introduce a wrapper over virFileWrapperFdClose
by Michal Privoznik 21 Sep '17

21 Sep '17

https://bugzilla.redhat.com/show_bug.cgi?id=1448268 When migrating to a file (e.g. when doing 'virsh save file'), couple of things are happening in the thread that is executing the API: 1) the domain obj is locked 2) iohelper is spawned as a separate process to handle all I/O 3) the thread waits for iohelper to finish 4) the domain obj is unlocked Now, the problem is that while the thread waits in step 3 for iohelper to finish this may take ages because iohelper calls fdatasync(). And unfortunately, we are waiting the whole time with the domain locked. So if another thread wants to jump in and say copy the domain name ('virsh list' for instance), they are stuck. The solution is to unlock the domain whenever waiting for I/O and lock it back again when it finished. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- diff to v1: - check after locking the domain obj back if the domain is still alive, since it was unlocked it might have changed its state. src/qemu/qemu_driver.c | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e1a0dd553..6095a5ec5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3216,6 +3216,31 @@ qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid, goto cleanup; } + +static int +qemuFileWrapperFDClose(virDomainObjPtr vm, + virFileWrapperFdPtr fd) +{ + int ret; + + /* virFileWrapperFd uses iohelper to write data onto disk. + * However, iohelper calls fdatasync() which may take ages to + * finish. Therefore, we shouldn't be waiting with the domain + * object locked. */ + + virObjectUnlock(vm); + ret = virFileWrapperFdClose(fd); + virObjectLock(vm); + if (!virDomainObjIsActive(vm)) { + if (!virGetLastError()) + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("domain is no longer running")); + ret = -1; + } + return ret; +} + + /* Helper function to execute a migration to file with a correct save header * the caller needs to make sure that the processors are stopped and do all other * actions besides saving memory */ @@ -3276,7 +3301,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver, goto cleanup; } - if (virFileWrapperFdClose(wrapperFd) < 0) + if (qemuFileWrapperFDClose(vm, wrapperFd) < 0) goto cleanup; if ((fd = qemuOpenFile(driver, vm, path, O_WRONLY, NULL, NULL)) < 0 || @@ -3827,7 +3852,7 @@ doCoreDump(virQEMUDriverPtr driver, path); goto cleanup; } - if (virFileWrapperFdClose(wrapperFd) < 0) + if (qemuFileWrapperFDClose(vm, wrapperFd) < 0) goto cleanup; ret = 0; @@ -6687,7 +6712,7 @@ qemuDomainRestoreFlags(virConnectPtr conn, ret = qemuDomainSaveImageStartVM(conn, driver, vm, &fd, data, path, false, QEMU_ASYNC_JOB_START); - if (virFileWrapperFdClose(wrapperFd) < 0) + if (qemuFileWrapperFDClose(vm, wrapperFd) < 0) VIR_WARN("Failed to close %s", path); qemuProcessEndJob(driver, vm); @@ -6962,7 +6987,7 @@ qemuDomainObjRestore(virConnectPtr conn, ret = qemuDomainSaveImageStartVM(conn, driver, vm, &fd, data, path, start_paused, asyncJob); - if (virFileWrapperFdClose(wrapperFd) < 0) + if (qemuFileWrapperFDClose(vm, wrapperFd) < 0) VIR_WARN("Failed to close %s", path); cleanup: -- 2.13.5

2 2

[libvirt] [PATCH 0/2] qemu: Order PCI controllers on the command line
by Andrea Bolognani 21 Sep '17

21 Sep '17

All the details are in the commit message for patch 2/2. Andrea Bolognani (2): qemu: Clean up qemuBuildControllerDevCommandLine() qemu: Order PCI controllers on the command line src/qemu/qemu_command.c | 91 +++++++++++++++------- .../qemuxml2argv-pci-autoadd-idx.args | 2 +- .../qemuxml2argv-pseries-many-buses-2.args | 4 +- 3 files changed, 68 insertions(+), 29 deletions(-) -- 2.13.5

3 8

[libvirt] [PATCH 0/6] Fix guest CPU updates for inactive domains
by Jiri Denemark 21 Sep '17

21 Sep '17

This series originally started as a cleanup work, but it turned out two real bugs got fixed with this cleanup :-) Since commit v2.2.0-199-g7ce711a30e libvirt stores an updated guest CPU in domain's live definition and there's no need to update it every time we want to format the definition. Thus libvirt should never internally request a guest CPU update when formatting domain XML. But if it is asked by a user to do so, it should use the same host CPU model which is used when starting a domain to provide meaningful results. https://bugzilla.redhat.com/show_bug.cgi?id=1481309 https://bugzilla.redhat.com/show_bug.cgi?id=1485022 Jiri Denemark (6): qemuxml2xmltest: Add tests for Power CPUs cpu_conf: Drop updateCPU from virCPUDefFormat cpu_conf: Simplify formatting of guest CPU attributes conf: Drop unused VIR_DOMAIN_DEF_FORMAT_UPDATE_CPU qemu: Use correct host model for updating guest cpu qemu: Don't update CPU when formatting live def src/bhyve/bhyve_driver.c | 2 +- src/conf/capabilities.c | 2 +- src/conf/cpu_conf.c | 38 +++++++++------------ src/conf/cpu_conf.h | 9 ++--- src/conf/domain_capabilities.c | 2 +- src/conf/domain_conf.c | 6 +--- src/conf/domain_conf.h | 1 - src/conf/snapshot_conf.c | 3 +- src/libxl/libxl_driver.c | 2 +- src/qemu/qemu_domain.c | 17 +++++++--- src/qemu/qemu_domain.h | 3 +- src/qemu/qemu_driver.c | 9 ++++- src/qemu/qemu_migration_cookie.c | 2 +- src/test/test_driver.c | 2 +- src/vz/vz_driver.c | 2 +- tests/cputest.c | 15 ++++----- .../ppc64-host+guest-compat-incompatible.xml | 2 +- .../ppc64-host+guest-compat-invalid.xml | 2 +- .../cputestdata/ppc64-host+guest-compat-valid.xml | 2 +- .../qemuxml2xmlout-pseries-cpu-compat-power9.xml | 38 +++++++++++++++++++++ .../qemuxml2xmlout-pseries-cpu-compat.xml | 38 +++++++++++++++++++++ .../qemuxml2xmlout-pseries-cpu-exact.xml | 39 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 4 +++ 23 files changed, 178 insertions(+), 62 deletions(-) create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-cpu-compat-power9.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-cpu-compat.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-cpu-exact.xml -- 2.14.1

2 9

[libvirt] [PATCH 0/5] Reject parallel ports on s390, and pseries
by Pino Toscano 21 Sep '17

21 Sep '17

Hi, this is a simple series to reject parallel ports on s390 architectures, and pseries machines -- both simply do not support them. This fixes https://bugzilla.redhat.com/show_bug.cgi?id=1487499 Pino Toscano (5): tests: qemuxml2argv: fix expected type for usb-bus-missing tests: qemuxml2argv: fail also on unexpected pass qemu: pass the virDomainDef to qemuDomainChrDefValidate qemu: reject parallel ports for s390 archs qemu: reject parallel ports for pseries machines src/qemu/qemu_domain.c | 16 ++++++++++++---- .../qemuxml2argv-pseries-no-parallel.xml | 18 ++++++++++++++++++ .../qemuxml2argv-s390-no-parallel.xml | 22 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 20 +++++++++++++++++--- 4 files changed, 69 insertions(+), 7 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-no-parallel.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-s390-no-parallel.xml -- 2.13.5

4 10

[libvirt] [PATCH] qemu: capabilities: Remove support for downstream-only QMP monitor backport
by Peter Krempa 21 Sep '17

21 Sep '17

Some distros (see diff) chose to backport QMP support rather than rebase to newer version of qemu. As a hack they added the string 'libvirt' to the qemu -help output. Remove this as downstream-only hacks should be carried by downstream and not litter upstream. This effectively reverts commit ff88cd590572277f10ecee4ebb1174d9b70fc0d7 --- src/qemu/qemu_capabilities.c | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 7bfb71420..085910dd4 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -1335,8 +1335,7 @@ virQEMUCapsComputeCmdFlags(const char *help, if ((netdev = strstr(help, "-netdev"))) { /* Disable -netdev on 0.12 since although it exists, * the corresponding netdev_add/remove monitor commands - * do not, and we need them to be able to do hotplug. - * But see below about RHEL build. */ + * do not, and we need them to be able to do hotplug. */ if (version >= 13000) { if (strstr(netdev, "bridge")) virQEMUCapsSet(qemuCaps, QEMU_CAPS_NETDEV_BRIDGE); @@ -1369,20 +1368,10 @@ virQEMUCapsComputeCmdFlags(const char *help, /* While JSON mode was available in 0.12.0, it was too * incomplete to contemplate using. The 0.13.0 release * is good enough to use, even though it lacks one or - * two features. This is also true of versions of qemu - * built for RHEL, labeled 0.12.1, but with extra text - * in the help output that mentions that features were - * backported for libvirt. The benefits of JSON mode now - * outweigh the downside. - */ + * two features. */ #if WITH_YAJL - if (version >= 13000) { - virQEMUCapsSet(qemuCaps, QEMU_CAPS_MONITOR_JSON); - } else if (version >= 12000 && - strstr(help, "libvirt")) { + if (version >= 13000) virQEMUCapsSet(qemuCaps, QEMU_CAPS_MONITOR_JSON); - virQEMUCapsSet(qemuCaps, QEMU_CAPS_NETDEV); - } #else /* Starting with qemu 0.15 and newer, upstream qemu no longer * promises to keep the human interface stable, but requests that @@ -1392,8 +1381,7 @@ virQEMUCapsComputeCmdFlags(const char *help, * telling them to recompile (the spec file includes the * dependency, so distros won't hit this). This check is * also in m4/virt-yajl.m4 (see $with_yajl). */ - if (version >= 15000 || - (version >= 12000 && strstr(help, "libvirt"))) { + if (version >= 15000) { if (check_yajl) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("this qemu binary requires libvirt to be " -- 2.13.5

2 1

[libvirt] [PATCH] Fix commandhelper build on win32
by Daniel P. Berrange 21 Sep '17

21 Sep '17

For win32 we need EXIT_AM_SKIP which is in testutils.h. We must define NO_LIBVIRT to prevent replacement of fprintf with virFilePrintf as we can't link to libvirt_util.la Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com> --- Pushed as a build fix tests/commandhelper.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index e9e6353f3..1da2834aa 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -28,6 +28,8 @@ #include <sys/stat.h> #include "internal.h" +#define NO_LIBVIRT +#include "testutils.h" #ifndef WIN32 -- 2.13.5

1 0

[libvirt] [PATCH libvirt-glib 0/5] Portability fixes
by Andrea Bolognani 21 Sep '17

21 Sep '17

Currently, trying to build libvirt-glib on FreeBSD results in all sorts of interesting failures. After applying this series, it's possible to build libvirt-glib and run the test suite successfully. There's even a couple of clean-ups thrown in for good measure :) Andrea Bolognani (5): maint: Drop autobuild.sh configure: Bump required libvirt version to 1.2.5 configure: Look for Perl interpreter path tests: Don't rely on non-portable paths scripts: Fix sha-bang lines autobuild.sh | 91 ------------------------------------------- build-aux/check-symfile.pl | 2 +- build-aux/check-symsorting.pl | 2 +- configure.ac | 4 +- examples/config-demo.py | 2 +- examples/conn-test.js | 2 +- examples/conn-test.py | 2 +- examples/event-test.py | 1 + tap-driver.sh | 2 +- tap-test | 2 +- tests/test-events.c | 2 +- 11 files changed, 12 insertions(+), 100 deletions(-) delete mode 100755 autobuild.sh -- 2.13.5

2 6

[libvirt] Questions about function virPCIDeviceIsBehindSwitchLackingACS in virpci.c
by Wuzongyong (Euler Dept) 21 Sep '17

21 Sep '17

Hi, In function virPCIDeviceIsBehindSwitchLackingACS, I noticed that(line 8): 1 if (virPCIDeviceGetParent(dev, &parent) < 0) 2 return -1; 3 if (!parent) { 4 /* if we have no parent, and this is the root bus, ACS doesn't come 5 * into play since devices on the root bus can't P2P without going 6 * through the root IOMMU. 7 */ 8 if (dev->address.bus == 0) { 9 return 0; 10 } else { 11 virReportError(VIR_ERR_INTERNAL_ERROR, 12 _("Failed to find parent device for %s"), 13 dev->name); 14 return -1; 15 } 16 } Why we just return 0 only if device's bus is 0? In my server, I can see a root bus which bus number is greater than 0, see the results(just a part) after I run lspci -t: +-[0000:80]-+-02.0-[81-83]--+-00.0 | | \-00.1 | +-05.0 | +-05.1 | +-05.2 | \-05.4 +-[0000:7f]-+-08.0 | +-08.2 | +-08.3 | + . . . | \-1f.2 \-[0000:00]-+-00.0 +-01.0-[01]----00.0 +-02.0-[02]--+-00.0 | +-00.1 | +-00.2 | \-00.3 +-02.2-[03]-- +-03.0-[04-0b]----00.0-[05-0b]--+-08.0-[06-08]----00.0 | \-10.0-[09-0b]----00.0 +-05.0 +-05.1 +-05.2 +-05.4 +-11.0 +-11.4 +-16.0 +-16.1 +-1a.0 If I assign the device 0000:81:00.0 to a VM, I get "Failed to find parent device". I think I should get no error with return value 0 just like bus number is 0, because bus 80 is the root bus as well in my case. In the <<Intel C610 Series Chipset and Intel X99 Chipset Platform Controller Hub(PCH)>> Datasheet, I found that(Chapter 9.1): For some server platforms, it may be desirable to have multiple PCHs in the system Which means some PCH's may reside on a bus greater than 0. So, is this a bug? Thanks, Zongyong Wu

4 6

[libvirt] [PATCH] iohelper: avoid calling read() with misaligned buffers for O_DIRECT
by Daniel P. Berrange 20 Sep '17

20 Sep '17

The iohelper currently calls saferead() to get data from the underlying file. This has a problem with O_DIRECT when hitting end-of-file. saferead() is asked to read 1MB, but the first read() it does may return only a few KB, so it'll try another read() to fill the remaining buffer. Unfortunately the buffer pointer passed into this 2nd read() is likely not aligned to the extent that O_DIRECT requires, so rather than seeing '0' for end-of-file, we'll get -1 + EINVAL due to misaligned buffer. The way the iohelper is currently written, it already handles getting short reads, so there is actually no need to use saferead() at all. We can simply call read() directly. The benefit of this is that we can now write() the data immediately so when we go into the subsequent reads() we'll always have a correctly aligned buffer. Technically the file position ought to be aligned for O_DIRECT too, but this does not appear to matter when at end-of-file. Tested-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com> Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com> --- src/util/iohelper.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/util/iohelper.c b/src/util/iohelper.c index fe15a92e6..5416d4506 100644 --- a/src/util/iohelper.c +++ b/src/util/iohelper.c @@ -109,7 +109,9 @@ runIO(const char *path, int fd, int oflags) while (1) { ssize_t got; - if ((got = saferead(fdin, buf, buflen)) < 0) { + if ((got = read(fdin, buf, buflen)) < 0) { + if (errno == EINTR) + continue; virReportSystemError(errno, _("Unable to read %s"), fdinname); goto cleanup; } -- 2.13.5

2 1

[libvirt] [PATCH v2] iohelper: fix reading with O_DIRECT
by Nikolay Shirokovskiy 20 Sep '17

20 Sep '17

saferead is not suitable for direct reads. If file size is not multiple of align size then we get EINVAL on the read(2) that is supposed to return 0 because read buffer will not be aligned at this point. Let's not read again after partial read and check that we read everything by comparing the number of total bytes read against file size. --- Diff from v1: - a couple of cosmetic changes src/util/iohelper.c | 42 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 37 insertions(+), 5 deletions(-) diff --git a/src/util/iohelper.c b/src/util/iohelper.c index fe15a92..9aa6ae0 100644 --- a/src/util/iohelper.c +++ b/src/util/iohelper.c @@ -56,6 +56,7 @@ runIO(const char *path, int fd, int oflags) unsigned long long total = 0; bool direct = O_DIRECT && ((oflags & O_DIRECT) != 0); off_t end = 0; + off_t cur; #if HAVE_POSIX_MEMALIGN if (posix_memalign(&base, alignMask + 1, buflen)) { @@ -78,10 +79,22 @@ runIO(const char *path, int fd, int oflags) fdoutname = "stdout"; /* To make the implementation simpler, we give up on any * attempt to use O_DIRECT in a non-trivial manner. */ - if (direct && ((end = lseek(fd, 0, SEEK_CUR)) != 0)) { - virReportSystemError(end < 0 ? errno : EINVAL, "%s", - _("O_DIRECT read needs entire seekable file")); - goto cleanup; + if (direct) { + if ((cur = lseek(fd, 0, SEEK_CUR)) != 0) { + virReportSystemError(cur < 0 ? errno : EINVAL, "%s", + _("O_DIRECT read needs entire seekable file")); + goto cleanup; + } + + if ((end = lseek(fd, 0, SEEK_END)) < 0) { + virReportSystemError(errno, "%s", _("can not seek file end")); + goto cleanup; + } + + if (lseek(fd, cur, SEEK_SET) < 0) { + virReportSystemError(errno, "%s", _("can not seek file begin")); + goto cleanup; + } } break; case O_WRONLY: @@ -109,7 +122,26 @@ runIO(const char *path, int fd, int oflags) while (1) { ssize_t got; - if ((got = saferead(fdin, buf, buflen)) < 0) { + /* in case of O_DIRECT we cannot read again to check for EOF + * after partial buffer read as it is done in saferead */ + if (direct && fdin == fd && end - total < buflen) { + if (total == end) + break; + + while ((got = read(fd, buf, buflen)) < 0 && errno == EINTR) + ; + + if (got < end - total) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to read last chunk from %s"), + fdinname); + goto cleanup; + } + } else { + got = saferead(fdin, buf, buflen); + } + + if (got < 0) { virReportSystemError(errno, _("Unable to read %s"), fdinname); goto cleanup; } -- 1.8.3.1

2 9