[libvirt] [PATCH v2 3/7] qemu: refactor blockinfo job handling

[Eric Blake]

In order for a future patch to actually implement the new
VIR_DOMAIN_XML_BLOCK_INFO flag, we want to reuse the code for
grabbing block information, including the use of a monitor
command.  But grabbing a job multiple times when getting XML
is not wise, so it is easier to reuse the query code if it
can always assume the job is already owned.  For that to work,
this patch refactors the job handling to have a larger scope
when getting information on a single block.  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, in order to make future
patch for reusing disk information easier to manage.

Signed-off-by: Eric Blake <eblake at redhat.com>
---
 src/qemu/qemu_driver.c | 62 +++++++++++++++++++++++---------------------------
 1 file changed, 29 insertions(+), 33 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9b77b00..cd70fde 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10950,7 +10950,6 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
     int format;
     int activeFail = false;
     virQEMUDriverConfigPtr cfg = NULL;
-    char *alias = NULL;
     char *buf = NULL;
     ssize_t len;

@@ -10969,11 +10968,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];
@@ -10983,36 +10990,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;
         }
     }

@@ -11024,17 +11031,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) {
@@ -11056,7 +11063,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;
@@ -11084,35 +11091,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)
-- 
1.9.3