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

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; > } >