[libvirt] [PATCH 2/3] qemu: don't use vmdef without domain lock
John Ferlan
jferlan at redhat.com
Thu Dec 8 16:40:10 UTC 2016
On 11/24/2016 04:19 AM, Nikolay Shirokovskiy wrote:
> 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
>
This failed to compile for me as 'num' wasn't initialized in
qemuDomainGetFSInfo
> 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);
You'd have to free each 'ndevAlias' here.
> +
> + 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)
qemuDomainFsInfoConvert
> +{
> + 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;
I think you could certainly make use of VIR_STEAL_PTR here
> +
> + 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;
Not sure EXPAND/SHRINK will be necessary
> +
> + 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;
Would it be possible to use something like VIR_APPEND_ELEMENT of a
VIR_STEAL_PTR on diskDef->dst; ?
> + }
> +
> + 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;
num needs to be initialized since we can jump to cleanup without setting it.
> + 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);
>
Wouldn't it be possible to pass a copy (eg virDomainDefCopy prior to
dropping the vm lock) of the vm->def that virDomainDiskByAddress is
going to need?
In the long run all it's using the data for is a frozen view in time.
> + 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;
> }
Depending on the above, could mean changes below, so I'll leave it for
now...
John
> 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;
> }
>
More information about the libvir-list
mailing list