From a20c3aafbe501cc1756341e82d33728a4d9c5b8f Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 14 Nov 2014 09:44:40 -0700 Subject: [PATCH] qemu: refactor blockinfo job handling In order for a future patch to virDomainListGetStats to reuse some code for determining disk usage of offline domains, we need to make it easier to pull out part of the guts of grabbing blockinfo. The current implementation grabs a job fairly late in the game, while getstats will already own a job; reordering things so that the job is always grabbed up front in both functions will make it easier to pull out the common code. This patch results in grabbing a job in cases where one was not previously needed, but as it is a query job, it should not be noticeably slower. This patch touches the same code as the fix for CVE-2014-6458 (commit b799259); in that patch, we avoided hotplug changing a disk reference during the time of obtaining a monitor lock by copying all data we needed and no longer referencing disk; this patch goes the other way and ensures that by holding the job, the disk cannot be changed so we no longer need to worry about the disk being invalidated across the monitor lock. * src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Rearrange job control to be outside of disk information. Signed-off-by: Eric Blake --- src/qemu/qemu_driver.c | 65 ++++++++++++++++++++---------------------- 1 file changed, 31 insertions(+), 34 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9aba4c87ae..9b399d0259 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11021,7 +11021,6 @@ qemuDomainGetBlockInfo(virDomainPtr dom, int format; bool activeFail = false; virQEMUDriverConfigPtr cfg = NULL; - char *alias = NULL; char *buf = NULL; ssize_t len; @@ -11040,11 +11039,19 @@ qemuDomainGetBlockInfo(virDomainPtr dom, goto cleanup; } + /* Technically, we only need a job if we are going to query the + * monitor, which is only for active domains that are using + * non-raw block devices. But it is easier to share code if we + * always grab a job; furthermore, grabbing the job ensures that + * hot-plug won't change disk behind our backs. */ + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + goto cleanup; + /* Check the path belongs to this domain. */ if ((idx = virDomainDiskIndexByName(vm->def, path, false)) < 0) { virReportError(VIR_ERR_INVALID_ARG, _("invalid path %s not assigned to domain"), path); - goto cleanup; + goto endjob; } disk = vm->def->disks[idx]; @@ -11054,36 +11061,36 @@ qemuDomainGetBlockInfo(virDomainPtr dom, virReportError(VIR_ERR_INVALID_ARG, _("disk '%s' does not currently have a source assigned"), path); - goto cleanup; + goto endjob; } if ((fd = qemuOpenFile(driver, vm, disk->src->path, O_RDONLY, NULL, NULL)) == -1) - goto cleanup; + goto endjob; if (fstat(fd, &sb) < 0) { virReportSystemError(errno, _("cannot stat file '%s'"), disk->src->path); - goto cleanup; + goto endjob; } if ((len = virFileReadHeaderFD(fd, VIR_STORAGE_MAX_HEADER, &buf)) < 0) { virReportSystemError(errno, _("cannot read header '%s'"), disk->src->path); - goto cleanup; + goto endjob; } } else { if (virStorageFileInitAs(disk->src, cfg->user, cfg->group) < 0) - goto cleanup; + goto endjob; if ((len = virStorageFileReadHeader(disk->src, VIR_STORAGE_MAX_HEADER, &buf)) < 0) - goto cleanup; + goto endjob; if (virStorageFileStat(disk->src, &sb) < 0) { virReportSystemError(errno, _("failed to stat remote file '%s'"), NULLSTR(disk->src->path)); - goto cleanup; + goto endjob; } } @@ -11095,17 +11102,17 @@ qemuDomainGetBlockInfo(virDomainPtr dom, virReportError(VIR_ERR_INTERNAL_ERROR, _("no disk format for %s and probing is disabled"), path); - goto cleanup; + goto endjob; } if ((format = virStorageFileProbeFormatFromBuf(disk->src->path, buf, len)) < 0) - goto cleanup; + goto endjob; } if (!(meta = virStorageFileGetMetadataFromBuf(disk->src->path, buf, len, format, NULL))) - goto cleanup; + goto endjob; /* Get info for normal formats */ if (S_ISREG(sb.st_mode) || fd == -1) { @@ -11127,7 +11134,7 @@ qemuDomainGetBlockInfo(virDomainPtr dom, if (end == (off_t)-1) { virReportSystemError(errno, _("failed to seek to end of %s"), path); - goto cleanup; + goto endjob; } info->physical = end; info->capacity = end; @@ -11155,35 +11162,24 @@ qemuDomainGetBlockInfo(virDomainPtr dom, if (!virDomainObjIsActive(vm)) { activeFail = true; ret = 0; - goto cleanup; + goto endjob; } - if (VIR_STRDUP(alias, disk->info.alias) < 0) - goto cleanup; + qemuDomainObjEnterMonitor(driver, vm); + ret = qemuMonitorGetBlockExtent(priv->mon, + disk->info.alias, + &info->allocation); + qemuDomainObjExitMonitor(driver, vm); - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) - goto cleanup; - - if (virDomainObjIsActive(vm)) { - qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorGetBlockExtent(priv->mon, - alias, - &info->allocation); - qemuDomainObjExitMonitor(driver, vm); - } else { - activeFail = true; - ret = 0; - } - - if (!qemuDomainObjEndJob(driver, vm)) - vm = NULL; } else { ret = 0; } + endjob: + if (!qemuDomainObjEndJob(driver, vm)) + vm = NULL; cleanup: VIR_FREE(buf); - VIR_FREE(alias); virStorageSourceFree(meta); VIR_FORCE_CLOSE(fd); if (disk) @@ -11197,7 +11193,8 @@ qemuDomainGetBlockInfo(virDomainPtr dom, _("domain is not running")); ret = -1; } - virObjectUnlock(vm); + if (vm) + virObjectUnlock(vm); virObjectUnref(cfg); return ret; }