[PATCH 2/3] qemu: don't use vmdef without domain lock

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Current call to qemuAgentGetFSInfo in qemuDomainGetFSInfo is
unsafe. Domain lock is dropped and we use vm->def. To fix that
let's fill in intermediate qemuAgentFsInfo structure in
qemuAgentGetFSInfo and use vm->def to convert result later when
lock is hold.
---
 src/qemu/qemu_agent.c                    | 52 +++++++++++--------
 src/qemu/qemu_agent.h                    | 25 ++++++++-
 src/qemu/qemu_driver.c                   | 88 +++++++++++++++++++++++++++++++-
 tests/Makefile.am                        |  1 -
 tests/qemuagentdata/qemuagent-fsinfo.xml | 39 --------------
 tests/qemuagenttest.c                    | 47 +++++++----------
 6 files changed, 159 insertions(+), 93 deletions(-)
 delete mode 100644 tests/qemuagentdata/qemuagent-fsinfo.xml

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index c5cf403..5230cbc 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -1835,18 +1835,15 @@ qemuAgentSetTime(qemuAgentPtr mon,
 
 
 int
-qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info,
-                   virDomainDefPtr vmdef)
+qemuAgentGetFSInfo(qemuAgentPtr mon, qemuAgentFsInfoPtr **info)
 {
     size_t i, j, k;
     int ret = -1;
     ssize_t ndata = 0, ndisk;
-    char **alias;
     virJSONValuePtr cmd;
     virJSONValuePtr reply = NULL;
     virJSONValuePtr data;
-    virDomainFSInfoPtr *info_ret = NULL;
-    virPCIDeviceAddress pci_address;
+    qemuAgentFsInfoPtr *info_ret = NULL;
 
     cmd = qemuAgentMakeCommand("guest-get-fsinfo", NULL);
     if (!cmd)
@@ -1879,6 +1876,8 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info,
         goto cleanup;
 
     for (i = 0; i < ndata; i++) {
+        qemuAgentFsDiskAliasPtr alias;
+
         /* Reverse the order to arrange in mount order */
         virJSONValuePtr entry = virJSONValueArrayGet(data, ndata - 1 - i);
 
@@ -1941,7 +1940,6 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info,
             int diskaddr[3], pciaddr[4];
             const char *diskaddr_comp[] = {"bus", "target", "unit"};
             const char *pciaddr_comp[] = {"domain", "bus", "slot", "function"};
-            virDomainDiskDefPtr diskDef;
 
             if (!disk) {
                 virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -1967,6 +1965,11 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info,
                     goto cleanup;
                 }
             }
+
+            alias->bus = diskaddr[0];
+            alias->target = diskaddr[1];
+            alias->unit = diskaddr[2];
+
             for (k = 0; k < 4; k++) {
                 if (virJSONValueObjectGetNumberInt(
                         pci, pciaddr_comp[k], &pciaddr[k]) < 0) {
@@ -1977,22 +1980,13 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info,
                 }
             }
 
-            pci_address.domain = pciaddr[0];
-            pci_address.bus = pciaddr[1];
-            pci_address.slot = pciaddr[2];
-            pci_address.function = pciaddr[3];
-            if (!(diskDef = virDomainDiskByAddress(
-                     vmdef, &pci_address,
-                     diskaddr[0], diskaddr[1], diskaddr[2])))
-                continue;
+            alias->address.domain = pciaddr[0];
+            alias->address.bus = pciaddr[1];
+            alias->address.slot = pciaddr[2];
+            alias->address.function = pciaddr[3];
 
-            if (VIR_STRDUP(*alias, diskDef->dst) < 0)
-                goto cleanup;
-
-            if (*alias) {
-                alias++;
-                info_ret[i]->ndevAlias++;
-            }
+            alias++;
+            info_ret[i]->ndevAlias++;
         }
     }
 
@@ -2003,7 +1997,7 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info,
  cleanup:
     if (info_ret) {
         for (i = 0; i < ndata; i++)
-            virDomainFSInfoFree(info_ret[i]);
+            qemuAgentFsInfoFree(info_ret[i]);
         VIR_FREE(info_ret);
     }
     virJSONValueFree(cmd);
@@ -2242,3 +2236,17 @@ qemuAgentSetUserPassword(qemuAgentPtr mon,
     VIR_FREE(password64);
     return ret;
 }
+
+void
+qemuAgentFsInfoFree(qemuAgentFsInfoPtr info)
+{
+    if (!info)
+        return;
+
+    VIR_FREE(info->mountpoint);
+    VIR_FREE(info->name);
+    VIR_FREE(info->fstype);
+    VIR_FREE(info->devAlias);
+
+    VIR_FREE(info);
+}
diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
index 6dd9c70..4b2277c 100644
--- a/src/qemu/qemu_agent.h
+++ b/src/qemu/qemu_agent.h
@@ -75,8 +75,29 @@ int qemuAgentShutdown(qemuAgentPtr mon,
 int qemuAgentFSFreeze(qemuAgentPtr mon,
                       const char **mountpoints, unsigned int nmountpoints);
 int qemuAgentFSThaw(qemuAgentPtr mon);
-int qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info,
-                       virDomainDefPtr vmdef);
+
+typedef struct _qemuAgentFsDiskAlias qemuAgentFsDiskAlias;
+typedef qemuAgentFsDiskAlias *qemuAgentFsDiskAliasPtr;
+struct _qemuAgentFsDiskAlias {
+    virPCIDeviceAddress address;
+    unsigned int bus;
+    unsigned int target;
+    unsigned int unit;
+};
+
+typedef struct _qemuAgentFsInfo qemuAgentFsInfo;
+typedef qemuAgentFsInfo *qemuAgentFsInfoPtr;
+struct _qemuAgentFsInfo {
+    char *mountpoint; /* path to mount point */
+    char *name;       /* device name in the guest (e.g. "sda1") */
+    char *fstype;     /* filesystem type */
+    size_t ndevAlias; /* number of elements in devAlias */
+    qemuAgentFsDiskAliasPtr devAlias;  /* array of disk device aliases */
+};
+
+void qemuAgentFsInfoFree(qemuAgentFsInfoPtr info);
+
+int qemuAgentGetFSInfo(qemuAgentPtr mon, qemuAgentFsInfoPtr **info);
 
 int qemuAgentSuspend(qemuAgentPtr mon,
                      unsigned int target);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index fdfe912..1bc5dc6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -19672,14 +19672,89 @@ qemuNodeAllocPages(virConnectPtr conn,
 
 
 static int
+qemuDomainConvertFsInfo(qemuAgentFsInfoPtr *agent_info, int num,
+                        virDomainDefPtr def,
+                        virDomainFSInfoPtr **info)
+{
+    int ret = -1;
+    size_t i;
+    virDomainFSInfoPtr *info_ret = NULL;
+
+    if (num == 0) {
+        *info = NULL;
+        return 0;
+    }
+
+    if (VIR_ALLOC_N(info_ret, num) < 0)
+        return -1;
+
+    for (i = 0; i < num; i++) {
+        size_t j;
+        int devnum;
+
+        if (VIR_ALLOC(info_ret[i]) < 0)
+            goto cleanup;
+
+        if (VIR_STRDUP(info_ret[i]->mountpoint, agent_info[i]->mountpoint) < 0 ||
+            VIR_STRDUP(info_ret[i]->name, agent_info[i]->name) < 0 ||
+            VIR_STRDUP(info_ret[i]->fstype, agent_info[i]->fstype) < 0)
+            goto cleanup;
+
+        if (agent_info[i]->ndevAlias == 0)
+            continue;
+
+        if (VIR_EXPAND_N(info_ret[i]->devAlias,
+                         info_ret[i]->ndevAlias,
+                         agent_info[i]->ndevAlias) < 0)
+            goto cleanup;
+
+        devnum = 0;
+        for (j = 0; j < agent_info[i]->ndevAlias; j++) {
+            virDomainDiskDefPtr diskDef;
+            qemuAgentFsDiskAliasPtr alias = &agent_info[i]->devAlias[j];
+
+            if (!(diskDef = virDomainDiskByAddress(def, &alias->address,
+                                                   alias->bus, alias->target,
+                                                   alias->unit)))
+                continue;
+
+            if (VIR_STRDUP(info_ret[i]->devAlias[devnum++], diskDef->dst) < 0)
+                goto cleanup;
+        }
+
+        if (devnum < info_ret[i]->ndevAlias)
+            VIR_SHRINK_N(info_ret[i]->devAlias,
+                         info_ret[i]->ndevAlias,
+                         info_ret[i]->ndevAlias - devnum);
+    }
+
+    *info = info_ret;
+    info_ret = NULL;
+    ret = num;
+
+ cleanup:
+    if (info_ret) {
+        for (i = 0; i < num; i++)
+            virDomainFSInfoFree(info_ret[i]);
+        VIR_FREE(info_ret);
+    }
+
+    return ret;
+}
+
+
+static int
 qemuDomainGetFSInfo(virDomainPtr dom,
                     virDomainFSInfoPtr **info,
                     unsigned int flags)
 {
     virQEMUDriverPtr driver = dom->conn->privateData;
+    qemuAgentFsInfoPtr *agent_info = NULL;
     virDomainObjPtr vm;
     qemuAgentPtr agent;
     int ret = -1;
+    int num;
+    size_t i;
 
     virCheckFlags(0, ret);
 
@@ -19702,13 +19777,24 @@ qemuDomainGetFSInfo(virDomainPtr dom,
         goto endjob;
 
     agent = qemuDomainObjEnterAgent(vm);
-    ret = qemuAgentGetFSInfo(agent, info, vm->def);
+    num = qemuAgentGetFSInfo(agent, &agent_info);
     qemuDomainObjExitAgent(vm, agent);
 
+    if (num < 0)
+        goto endjob;
+
+    ret = qemuDomainConvertFsInfo(agent_info, num, vm->def, info);
+
  endjob:
     qemuDomainObjEndJob(driver, vm);
 
  cleanup:
+    if (agent_info) {
+        for (i = 0; i < num; i++)
+            qemuAgentFsInfoFree(agent_info[i]);
+        VIR_FREE(agent_info);
+    }
+
     virDomainObjEndAPI(&vm);
     return ret;
 }
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 924029a..d34cc95 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -116,7 +116,6 @@ EXTRA_DIST =		\
 	nwfilterxml2xmlin \
 	nwfilterxml2xmlout \
 	oomtrace.pl \
-	qemuagentdata \
 	qemuargv2xmldata \
 	qemucapabilitiesdata \
 	qemucaps2xmldata \
diff --git a/tests/qemuagentdata/qemuagent-fsinfo.xml b/tests/qemuagentdata/qemuagent-fsinfo.xml
deleted file mode 100644
index 9638feb..0000000
--- a/tests/qemuagentdata/qemuagent-fsinfo.xml
+++ /dev/null
@@ -1,39 +0,0 @@
-<domain type='qemu'>
-  <name>QEMUGuest1</name>
-  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
-  <memory unit='KiB'>219136</memory>
-  <currentMemory unit='KiB'>219136</currentMemory>
-  <vcpu placement='static'>1</vcpu>
-  <os>
-    <type arch='i686' machine='pc'>hvm</type>
-    <boot dev='hd'/>
-  </os>
-  <clock offset='utc'/>
-  <on_poweroff>destroy</on_poweroff>
-  <on_reboot>restart</on_reboot>
-  <on_crash>destroy</on_crash>
-  <devices>
-    <emulator>/usr/bin/qemu</emulator>
-    <disk type='file' device='disk'>
-      <source file='/tmp/idedisk.img'/>
-      <target dev='hdc' bus='ide'/>
-      <address type='drive' controller='0' bus='1' target='0' unit='0'/>
-    </disk>
-    <disk type='file' device='disk'>
-      <driver name='qemu' type='qcow2'/>
-      <source file='/tmp/virtio-blk1.qcow2'/>
-      <target dev='vda' bus='virtio'/>
-      <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/>
-    </disk>
-    <disk type='file' device='disk'>
-      <driver name='qemu' type='qcow2'/>
-      <source file='/tmp/virtio-blk2.qcow2'/>
-      <target dev='vdb' bus='virtio'/>
-      <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/>
-    </disk>
-    <controller type='ide' index='0'>
-      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
-    </controller>
-    <memballoon model='virtio'/>
-  </devices>
-</domain>
diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c
index 5dfa58e..97f340c 100644
--- a/tests/qemuagenttest.c
+++ b/tests/qemuagenttest.c
@@ -169,22 +169,16 @@ testQemuAgentGetFSInfo(const void *data)
     virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data;
     virCapsPtr caps = testQemuCapsInit();
     qemuMonitorTestPtr test = qemuMonitorTestNewAgent(xmlopt);
-    char *domain_filename = NULL;
-    virDomainDefPtr def = NULL;
-    virDomainFSInfoPtr *info = NULL;
+    qemuAgentFsInfoPtr *info = NULL;
     int ret = -1, ninfo = 0, i;
 
+    qemuAgentFsDiskAlias alias_sda1 = { { 0, 0, 1, 1, 0 }, 1, 0, 0 };
+    qemuAgentFsDiskAlias alias_vda = { { 0, 0, 6, 0, 0 }, 0, 0, 0 };
+    qemuAgentFsDiskAlias alias_vdb = { { 0, 0, 7, 0, 0 }, 0, 0, 0 };
+
     if (!test)
         return -1;
 
-    if (virAsprintf(&domain_filename, "%s/qemuagentdata/qemuagent-fsinfo.xml",
-                    abs_srcdir) < 0)
-        goto cleanup;
-
-    if (!(def = virDomainDefParseFile(domain_filename, caps, xmlopt,
-                                      NULL, VIR_DOMAIN_DEF_PARSE_INACTIVE)))
-        goto cleanup;
-
     if (qemuMonitorTestAddAgentSyncResponse(test) < 0)
         goto cleanup;
 
@@ -220,8 +214,7 @@ testQemuAgentGetFSInfo(const void *data)
                                "   \"disk\": [], \"type\": \"xfs\"}]}") < 0)
         goto cleanup;
 
-    if ((ninfo = qemuAgentGetFSInfo(qemuMonitorTestGetAgent(test),
-                                    &info, def)) < 0)
+    if ((ninfo = qemuAgentGetFSInfo(qemuMonitorTestGetAgent(test), &info)) < 0)
         goto cleanup;
 
     if (ninfo != 3) {
@@ -234,11 +227,11 @@ testQemuAgentGetFSInfo(const void *data)
         STRNEQ(info[2]->mountpoint, "/") ||
         STRNEQ(info[2]->fstype, "ext4") ||
         info[2]->ndevAlias != 1 ||
-        !info[2]->devAlias || !info[2]->devAlias[0] ||
-        STRNEQ(info[2]->devAlias[0], "hdc")) {
+        !info[2]->devAlias ||
+        memcmp(&info[2]->devAlias[0], &alias_sda1, sizeof(alias_sda1)) != 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
-            "unexpected filesystems information returned for sda1 (%s,%s)",
-            info[2]->name, info[2]->devAlias ? info[2]->devAlias[0] : "null");
+            "unexpected filesystems information returned for sda1 (%s)",
+            info[2]->name);
         ret = -1;
         goto cleanup;
     }
@@ -246,12 +239,12 @@ testQemuAgentGetFSInfo(const void *data)
         STRNEQ(info[1]->mountpoint, "/opt") ||
         STRNEQ(info[1]->fstype, "vfat") ||
         info[1]->ndevAlias != 2 ||
-        !info[1]->devAlias || !info[1]->devAlias[0] || !info[1]->devAlias[1] ||
-        STRNEQ(info[1]->devAlias[0], "vda") ||
-        STRNEQ(info[1]->devAlias[1], "vdb")) {
+        !info[1]->devAlias ||
+        memcmp(&info[1]->devAlias[0], &alias_vda, sizeof(alias_vda)) != 0 ||
+        memcmp(&info[1]->devAlias[1], &alias_vdb, sizeof(alias_vdb)) != 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
-            "unexpected filesystems information returned for dm-1 (%s,%s)",
-            info[0]->name, info[0]->devAlias ? info[0]->devAlias[0] : "null");
+            "unexpected filesystems information returned for dm-1 (%s)",
+            info[0]->name);
         ret = -1;
         goto cleanup;
     }
@@ -260,8 +253,8 @@ testQemuAgentGetFSInfo(const void *data)
         STRNEQ(info[0]->fstype, "xfs") ||
         info[0]->ndevAlias != 0 || info[0]->devAlias) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
-            "unexpected filesystems information returned for sdb1 (%s,%s)",
-            info[0]->name, info[0]->devAlias ? info[0]->devAlias[0] : "null");
+            "unexpected filesystems information returned for sdb1 (%s)",
+            info[0]->name);
         ret = -1;
         goto cleanup;
     }
@@ -280,7 +273,7 @@ testQemuAgentGetFSInfo(const void *data)
                                "}") < 0)
         goto cleanup;
 
-    if (qemuAgentGetFSInfo(qemuMonitorTestGetAgent(test), &info, def) != -1) {
+    if (qemuAgentGetFSInfo(qemuMonitorTestGetAgent(test), &info) != -1) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        "agent get-fsinfo command should have failed");
         goto cleanup;
@@ -290,11 +283,9 @@ testQemuAgentGetFSInfo(const void *data)
 
  cleanup:
     for (i = 0; i < ninfo; i++)
-        virDomainFSInfoFree(info[i]);
+        qemuAgentFsInfoFree(info[i]);
     VIR_FREE(info);
-    VIR_FREE(domain_filename);
     virObjectUnref(caps);
-    virDomainDefFree(def);
     qemuMonitorTestFree(test);
     return ret;
 }
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux