November 2019 - Devel - libvirt List Archives

[libvirt] [PATCH] remote_daemon_stream: Hold an extra reference to stream in daemonStreamFilter
by Michal Privoznik 29 Nov '19

29 Nov '19

In v5.9.0-273-g8ecab214de I've tried to fix a lock ordering problem, but introduced a crasher. Problem is that because the client lock is unlocked (in order to honour lock ordering) the stream we are currently checking in daemonStreamFilter() might be freed and thus stream->priv might not even exist when the control get to virMutexLock() call. To resolve this, grab an extra reference to the stream and handle its cleanup should the refcounter reach zero after the deref. If that's the case and we are the only ones holding a reference to the stream, we MUST return a positive value to make virNetServerClientDispatchRead() break its loop where it iterates over filters. The problem is, if we did not do so, then "filter = filter->next" line will read from a memory that was just freed (freeing a stream also unregisters its filter). Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- Reproducing this issue is very easy: 1) put sleep(5) right after virObjectUnlock(client) in the fist hunk, 2) virsh console --force $dom and type something so that the stream has some data to process 3) while 2) is still running, run the same command from another terminal 4) observe libvirtd crash src/remote/remote_daemon_stream.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/remote/remote_daemon_stream.c b/src/remote/remote_daemon_stream.c index 82cadb67ac..73e4d7befb 100644 --- a/src/remote/remote_daemon_stream.c +++ b/src/remote/remote_daemon_stream.c @@ -293,10 +293,25 @@ daemonStreamFilter(virNetServerClientPtr client, daemonClientStream *stream = opaque; int ret = 0; + /* We must honour lock ordering here. Client private data lock must + * be acquired before client lock. Bu we are already called with + * client locked. To avoid stream disappearing while we unlock + * everything, let's increase its refcounter. This has some + * implications though. */ + stream->refs++; virObjectUnlock(client); virMutexLock(&stream->priv->lock); virObjectLock(client); + if (stream->refs == 1) { + /* So we are the only ones holding the reference to the stream. + * Return 1 to signal to the caller that we've processed the + * message. And to "process" means free. */ + virNetMessageFree(msg); + ret = 1; + goto cleanup; + } + if (msg->header.type != VIR_NET_STREAM && msg->header.type != VIR_NET_STREAM_HOLE) goto cleanup; @@ -318,6 +333,10 @@ daemonStreamFilter(virNetServerClientPtr client, cleanup: virMutexUnlock(&stream->priv->lock); + /* Don't pass client here, because client is locked here and this + * function might try to lock it again which would result in a + * deadlock. */ + daemonFreeClientStream(NULL, stream); return ret; } -- 2.23.0

3 8

[libvirt] [PATCH 0/3] qemu: fix regression and design deficiencies of VIR_DOMAIN_STATS_IOTHREAD
by Peter Krempa 29 Nov '19

29 Nov '19

Fix regression of numbering of the output in the statistics for iothreads and improve few aspects so that the numbering is usable. Peter Krempa (3): lib: Fix documentation for the count field of VIR_DOMAIN_STATS_IOTHREAD qemu: Fix indexes in statistics of iothreads qemu: Report which iothread IDs are actually returned with VIR_DOMAIN_STATS_IOTHREAD src/libvirt-domain.c | 12 +++++++----- src/qemu/qemu_driver.c | 20 +++++++++++++++++--- 2 files changed, 24 insertions(+), 8 deletions(-) -- 2.23.0

2 7

[libvirt] [PATCH] storage: backend_disk: Implement the deletePool function
by Erik Skultety 29 Nov '19

29 Nov '19

This is a very simple and straightforward implementation of the opposite what buildPool does for the disk backend. The background for this change comes from an existing test case in TCK which does use the delete method for a pool of type disk, but it truly could not have ever worked since the implementation simply wasn't there for the pool of type disk. Signed-off-by: Erik Skultety <eskultet(a)redhat.com> --- src/storage/storage_backend_disk.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index d971530cd8..45d1257f3d 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -531,6 +531,25 @@ virStorageBackendDiskBuildPool(virStoragePoolObjPtr pool, } +/** + * Wipe the existing partition table + */ +static int +virStorageBackendDiskDeletePool(virStoragePoolObjPtr pool, + unsigned int flags) +{ + virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); + + virCheckFlags(0, -1); + + if (virStorageBackendZeroPartitionTable(def->source.devices[0].path, + 1024 * 1024) < 0) + return -1; + + return 0; +} + + struct virStorageVolNumData { int count; }; @@ -954,6 +973,7 @@ virStorageBackend virStorageBackendDisk = { .startPool = virStorageBackendDiskStartPool, .buildPool = virStorageBackendDiskBuildPool, .refreshPool = virStorageBackendDiskRefreshPool, + .deletePool = virStorageBackendDiskDeletePool, .createVol = virStorageBackendDiskCreateVol, .deleteVol = virStorageBackendDiskDeleteVol, -- 2.23.0

2 1

[libvirt] RFC: stop clearing QEMU emulator capabilities
by Daniel P. Berrangé 29 Nov '19

29 Nov '19

We have an RFE from libguestfs to provide a way to run as root *with* capabilities. I looked integrating this into the DAC security manager as a new flag in the security label, but then I started thinking about the whole idea of clearing capabilities https://bugzilla.redhat.com/show_bug.cgi?id=1045039 Pretty much forever we have explicitly cleared QEMU emulator capabilities when starting it. When QEMU uid/gid is set to non-root this is pointless as if we just used a regular setuid/setgid call, the process will have all its capabilities cleared anyway by the kernel. When QEMU uid/gid is set to root, this is almost (always?) never what people actually want. People make QEMU run as root in order to access some privileged resource that libvirt doesn't support yet and this often requires capabilities. As a result they have to go find the qemu.conf param to turn this off. This is not viable for libguestfs - they want to control everything via thue XML security label to request running as root regardless of the qemu.conf settings for user/group. Clearing capabilities was implemented originally because there was a proposal in Fedora to change permissions such that root, with no capabilities would not be able to compromise the system. ie a locked down root account. This never went anywhere though, and as a result clearing capabilities when running as root does not really get us any security benefit AFAICT. The root user can just do something like create a cronjob, which will then faithfully be run with full capabilities, trivially bypassing the restriction we place. IOW, our clearing of capabilities is both useless from a security POV, and breaks valid use cases when people need to run as root. I'm thinking we should just rip out the code which clears capabilities and allow default loggic to run - If uid/gid is non-root, then no capabilities are present - If uid/gid is root, then full capabilities are present 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 :|

4 3

[libvirt] [PATCH] check-symfile: Use pythonesque string formatting instead of perl
by Peter Krempa 29 Nov '19

29 Nov '19

Commit d30a1ad0443 translated the symbol file checker from perl to python by doing a literal translation in most cases. Unfortunately one string formatting operation was not really translated into python leaving users with non-helpful error: 'Symbol $1 is listed twice' Signed-off-by: Peter Krempa <pkrempa(a)redhat.com> --- scripts/check-symfile.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/check-symfile.py b/scripts/check-symfile.py index 0c02591991..34396b8623 100755 --- a/scripts/check-symfile.py +++ b/scripts/check-symfile.py @@ -52,7 +52,7 @@ with open(symfile, "r") as fh: line = line.strip(";") if line in wantsyms: - print("Symbol $1 is listed twice", file=sys.stderr) + print("Symbol %s is listed twice" % line ,file=sys.stderr) ret = 1 else: wantsyms[line] = True -- 2.23.0

9 14

[libvirt] Entering freeze for libvirt 5.10.0
by Daniel Veillard 28 Nov '19

28 Nov '19

As planned this is now tagged in git head and signed source tarball and rpm are available at the usual place: https://libvirt.org/sources/ there is a few weirdness on CI, some apparently related to freebsd, others to python and go bindings, worth having a look, some are broken for a while now. Otherwise this passes my very minimal testing, but don't rely on this ! I plan on making a RC2 for Thanksgiving, and if everything looks fine push the final release next Monday. Please make sure we have the docs in the news file :-) thanks ! 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/

2 2

[libvirt] [PATCH] qemu: make 'xz' image compression viable by using -3
by Daniel P. Berrangé 28 Nov '19

28 Nov '19

For managed save we can choose between various compression methods. I randomly tested the 'xz' program on a 8 GB guest and was surprised to have to wait > 50 minutes for it to finish compressing, with 'xz' burning 100% cpu for the entire time. Despite the impressive compression, this is completely useless in the real world as it is far too long to wait to save the VM. The 'xz' binary defaults to '-6' optimization level which aims for high compression, with moderate memory usage, at the expense of speed. This change switches it to use the '-3' optimization level which is documented as being the one that optimizes speed at expense of compression. Even with this, it will still outperform all the other options in terms of compression level. It is a little less than x4 faster than '-6' which means it starts to be a viable choice to use 'xz' for people who really want best compression. The test results on a 1 GB, fairly freshly booted VM are as follows format | save | restore size =======+=======+============= raw | 05s | 1s | 428 MB lzop | 05s | 3s | 160 MB gzip | 29s | 5s | 118 MB bz2 | 54s | 22s | 114 MB xz | 4m37s | 13s | 86 MB xz -3 | 1m20s | 12s | 95 MB Based on this we can say * For moderate compression with no noticable loss in speed => use lzop * For high compression with moderate loss in speed => use gzip * For best compression with significant loss in speed => use xz Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com> --- src/qemu/qemu_driver.c | 46 ++++++++++++++++++++++----------------- src/qemu/qemu_migration.c | 28 ++++++++---------------- src/qemu/qemu_migration.h | 2 +- 3 files changed, 36 insertions(+), 40 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8c2670d377..ba50701d3a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3177,7 +3177,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver, virDomainObjPtr vm, const char *path, virQEMUSaveDataPtr data, - const char *compressedpath, + virCommandPtr compressor, unsigned int flags, qemuDomainAsyncJob asyncJob) { @@ -3216,7 +3216,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver, goto cleanup; /* Perform the migration */ - if (qemuMigrationSrcToFile(driver, vm, fd, compressedpath, asyncJob) < 0) + if (qemuMigrationSrcToFile(driver, vm, fd, compressor, asyncJob) < 0) goto cleanup; /* Touch up file header to mark image complete. */ @@ -3259,7 +3259,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver, static int qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainObjPtr vm, const char *path, - int compressed, const char *compressedpath, + int compressed, virCommandPtr compressor, const char *xmlin, unsigned int flags) { g_autofree char *xml = NULL; @@ -3345,7 +3345,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, goto endjob; xml = NULL; - ret = qemuDomainSaveMemory(driver, vm, path, data, compressedpath, + ret = qemuDomainSaveMemory(driver, vm, path, data, compressor, flags, QEMU_ASYNC_JOB_SAVE); if (ret < 0) goto endjob; @@ -3406,13 +3406,14 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, */ static int ATTRIBUTE_NONNULL(2) qemuGetCompressionProgram(const char *imageFormat, - char **compresspath, + virCommandPtr *compressor, const char *styleFormat, bool use_raw_on_fail) { int ret; + const char *prog; - *compresspath = NULL; + *compressor = NULL; if (!imageFormat) return QEMU_SAVE_FORMAT_RAW; @@ -3423,9 +3424,14 @@ qemuGetCompressionProgram(const char *imageFormat, if (ret == QEMU_SAVE_FORMAT_RAW) return QEMU_SAVE_FORMAT_RAW; - if (!(*compresspath = virFindFileInPath(imageFormat))) + if (!(prog = virFindFileInPath(imageFormat))) goto error; + *compressor = virCommandNew(prog); + virCommandAddArg(*compressor, "-c"); + if (ret == QEMU_SAVE_FORMAT_XZ) + virCommandAddArg(*compressor, "-3"); + return ret; error: @@ -3466,7 +3472,7 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, { virQEMUDriverPtr driver = dom->conn->privateData; int compressed; - g_autofree char *compressedpath = NULL; + g_autoptr(virCommand) compressor = NULL; int ret = -1; virDomainObjPtr vm = NULL; g_autoptr(virQEMUDriverConfig) cfg = NULL; @@ -3477,7 +3483,7 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, cfg = virQEMUDriverGetConfig(driver); if ((compressed = qemuGetCompressionProgram(cfg->saveImageFormat, - &compressedpath, + &compressor, "save", false)) < 0) goto cleanup; @@ -3491,7 +3497,7 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, goto cleanup; ret = qemuDomainSaveInternal(driver, vm, path, compressed, - compressedpath, dxml, flags); + compressor, dxml, flags); cleanup: virDomainObjEndAPI(&vm); @@ -3522,7 +3528,7 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) virQEMUDriverPtr driver = dom->conn->privateData; g_autoptr(virQEMUDriverConfig) cfg = NULL; int compressed; - g_autofree char *compressedpath = NULL; + g_autoptr(virCommand) compressor = NULL; virDomainObjPtr vm; g_autofree char *name = NULL; int ret = -1; @@ -3548,7 +3554,7 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) cfg = virQEMUDriverGetConfig(driver); if ((compressed = qemuGetCompressionProgram(cfg->saveImageFormat, - &compressedpath, + &compressor, "save", false)) < 0) goto cleanup; @@ -3558,7 +3564,7 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) VIR_INFO("Saving state of domain '%s' to '%s'", vm->def->name, name); ret = qemuDomainSaveInternal(driver, vm, name, compressed, - compressedpath, NULL, flags); + compressor, NULL, flags); if (ret == 0) vm->hasManagedSave = true; @@ -3754,14 +3760,14 @@ doCoreDump(virQEMUDriverPtr driver, unsigned int flags = VIR_FILE_WRAPPER_NON_BLOCKING; const char *memory_dump_format = NULL; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); - g_autofree char *compressedpath = NULL; + g_autoptr(virCommand) compressor = NULL; /* We reuse "save" flag for "dump" here. Then, we can support the same * format in "save" and "dump". This path doesn't need the compression * program to exist and can ignore the return value - it only cares to - * get the compressedpath */ + * get the compressor */ ignore_value(qemuGetCompressionProgram(cfg->dumpImageFormat, - &compressedpath, + &compressor, "dump", true)); /* Create an empty file with appropriate ownership. */ @@ -3809,7 +3815,7 @@ doCoreDump(virQEMUDriverPtr driver, if (!qemuMigrationSrcIsAllowed(driver, vm, false, 0)) goto cleanup; - ret = qemuMigrationSrcToFile(driver, vm, fd, compressedpath, + ret = qemuMigrationSrcToFile(driver, vm, fd, compressor, QEMU_ASYNC_JOB_DUMP); } @@ -15586,7 +15592,7 @@ qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver, int thaw = 0; /* 1 if freeze succeeded, -1 if freeze failed */ bool pmsuspended = false; int compressed; - g_autofree char *compressedpath = NULL; + g_autoptr(virCommand) compressor = NULL; virQEMUSaveDataPtr data = NULL; /* If quiesce was requested, then issue a freeze command, and a @@ -15656,7 +15662,7 @@ qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver, JOB_MASK(QEMU_JOB_MIGRATION_OP))); if ((compressed = qemuGetCompressionProgram(cfg->snapshotImageFormat, - &compressedpath, + &compressor, "snapshot", false)) < 0) goto cleanup; @@ -15673,7 +15679,7 @@ qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver, xml = NULL; if ((ret = qemuDomainSaveMemory(driver, vm, snapdef->file, data, - compressedpath, 0, + compressor, 0, QEMU_ASYNC_JOB_SNAPSHOT)) < 0) goto cleanup; diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index c38bf342d7..dabdda2715 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -5170,13 +5170,12 @@ qemuMigrationDstFinish(virQEMUDriverPtr driver, int qemuMigrationSrcToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, int fd, - const char *compressor, + virCommandPtr compressor, qemuDomainAsyncJob asyncJob) { qemuDomainObjPrivatePtr priv = vm->privateData; int rc; int ret = -1; - virCommandPtr cmd = NULL; int pipeFD[2] = { -1, -1 }; unsigned long saveMigBandwidth = priv->migMaxBandwidth; char *errbuf = NULL; @@ -5221,25 +5220,17 @@ qemuMigrationSrcToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, QEMU_MONITOR_MIGRATE_BACKGROUND, fd); } else { - const char *prog = compressor; - const char *args[] = { - prog, - "-c", - NULL - }; - - cmd = virCommandNewArgs(args); - virCommandSetInputFD(cmd, pipeFD[0]); - virCommandSetOutputFD(cmd, &fd); - virCommandSetErrorBuffer(cmd, &errbuf); - virCommandDoAsyncIO(cmd); + virCommandSetInputFD(compressor, pipeFD[0]); + virCommandSetOutputFD(compressor, &fd); + virCommandSetErrorBuffer(compressor, &errbuf); + virCommandDoAsyncIO(compressor); if (virSetCloseExec(pipeFD[1]) < 0) { virReportSystemError(errno, "%s", _("Unable to set cloexec flag")); ignore_value(qemuDomainObjExitMonitor(driver, vm)); goto cleanup; } - if (virCommandRunAsync(cmd, NULL) < 0) { + if (virCommandRunAsync(compressor, NULL) < 0) { ignore_value(qemuDomainObjExitMonitor(driver, vm)); goto cleanup; } @@ -5260,7 +5251,7 @@ qemuMigrationSrcToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, if (rc < 0) { if (rc == -2) { virErrorPreserveLast(&orig_err); - virCommandAbort(cmd); + virCommandAbort(compressor); if (virDomainObjIsActive(vm) && qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) == 0) { qemuMonitorMigrateCancel(priv->mon); @@ -5270,7 +5261,7 @@ qemuMigrationSrcToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, goto cleanup; } - if (cmd && virCommandWait(cmd, NULL) < 0) + if (compressor && virCommandWait(compressor, NULL) < 0) goto cleanup; qemuDomainEventEmitJobCompleted(driver, vm); @@ -5290,10 +5281,9 @@ qemuMigrationSrcToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, VIR_FORCE_CLOSE(pipeFD[0]); VIR_FORCE_CLOSE(pipeFD[1]); - if (cmd) { + if (errbuf) { VIR_DEBUG("Compression binary stderr: %s", NULLSTR(errbuf)); VIR_FREE(errbuf); - virCommandFree(cmd); } virErrorRestore(&orig_err); diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index 2daae65191..d98fe9f80a 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -204,7 +204,7 @@ int qemuMigrationSrcToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, int fd, - const char *compressor, + virCommandPtr compressor, qemuDomainAsyncJob asyncJob) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; -- 2.21.0

2 1

[libvirt] [PATCH for 5.10.0] nss: Don't fail on empty files
by Michal Privoznik 28 Nov '19

28 Nov '19

Before we rewrote nss plugin so that it doesn't use libvirt's internal functions it used virLeaseReadCustomLeaseFile() to parse .status files. After the rewrite it's using read() + yajl_parse() + yajl_complete_parse(). There's one catch though, virLeaseReadCustomLeaseFile() skipped over empty files. An empty .status file is created when a network is started. This is because we configure dnsmasq to use our leasehelper. So the first thing it does it calls it as follows: DNSMASQ_INTERFACE=virbr0 /usr/libexec/libvirt_leaseshelper init which causes the leasehelper to create empty virbr0.status file. If there is only one libvirt network then that is no problem - there are no other .status files to parse anyway. But if there are two or more networks then the first empty .status file causes whole parsing process and subsequently the whole name lookup process to fail. Reported-by: Pavel Hrdina <phrdina(a)redhat.com> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- tools/nss/libvirt_nss_leases.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tools/nss/libvirt_nss_leases.c b/tools/nss/libvirt_nss_leases.c index e96e260da8..7c431e4d53 100644 --- a/tools/nss/libvirt_nss_leases.c +++ b/tools/nss/libvirt_nss_leases.c @@ -379,6 +379,7 @@ findLeases(const char *file, }; yajl_handle parser = NULL; char line[1024]; + ssize_t nreadTotal = 0; int rv; if ((fd = open(file, O_RDONLY)) < 0) { @@ -398,6 +399,7 @@ findLeases(const char *file, goto cleanup; if (rv == 0) break; + nreadTotal += rv; if (yajl_parse(parser, (const unsigned char *)line, rv) != yajl_status_ok) { @@ -409,7 +411,8 @@ findLeases(const char *file, } } - if (yajl_complete_parse(parser) != yajl_status_ok) { + if (nreadTotal > 0 && + yajl_complete_parse(parser) != yajl_status_ok) { ERROR("Parse failed %s", yajl_get_error(parser, 1, NULL, 0)); goto cleanup; -- 2.23.0

2 2

[libvirt] [PATCH] qemu: Fix migration without parameters
by Jiri Denemark 28 Nov '19

28 Nov '19

The virTypedParamsFilter function doesn't mind params == NULL if nparams is zero. And there's no need to check for params == NULL && nparams > 0 because this is checked higher in the stack. In fact all the virCheckNonNull* checks in virTypedParamsFilter are useless. https://bugzilla.redhat.com/show_bug.cgi?id=1777094 Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com> --- src/util/virtypedparam.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index 603fcf213a..be3dac1571 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -403,10 +403,6 @@ virTypedParamsFilter(virTypedParameterPtr params, { size_t i, n = 0; - virCheckNonNullArgGoto(params, error); - virCheckNonNullArgGoto(name, error); - virCheckNonNullArgGoto(ret, error); - if (VIR_ALLOC_N(*ret, nparams) < 0) goto error; -- 2.24.0

3 2

[libvirt] [PATCH 1/1] qemu: sync blockjob finishing in qemuDomainGetBlockJobInfo
by Nikolay Shirokovskiy 28 Nov '19

28 Nov '19

Due to race qemuDomainGetBlockJobInfo can return there is no block job for disk but later call to spawn new blockjob can fail because libvirt internally still not process blockjob finishing. Thus let's wait for blockjob finishing if we report there is no more blockjob. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com> --- src/qemu/qemu_driver.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 669c12d6ca..b148df3a57 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17785,12 +17785,26 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, goto endjob; } + qemuBlockJobSyncBegin(job); + qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorGetBlockJobInfo(qemuDomainGetMonitor(vm), job->name, &rawInfo); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; - if (ret <= 0) + if (ret < 0) + goto endjob; + + if (ret == 0) { + qemuBlockJobUpdate(vm, job, QEMU_ASYNC_JOB_NONE); + while (qemuBlockJobIsRunning(job)) { + if (virDomainObjWait(vm) < 0) { + ret = -1; + goto endjob; + } + qemuBlockJobUpdate(vm, job, QEMU_ASYNC_JOB_NONE); + } goto endjob; + } if (qemuBlockJobInfoTranslate(&rawInfo, info, disk, flags & VIR_DOMAIN_BLOCK_JOB_INFO_BANDWIDTH_BYTES) < 0) { @@ -17799,6 +17813,8 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, } endjob: + if (job) + qemuBlockJobSyncEnd(vm, job, QEMU_ASYNC_JOB_NONE); qemuDomainObjEndJob(driver, vm); cleanup: -- 2.23.0

2 6