On 12/09/2016 03:37 AM, Nikolay Shirokovskiy wrote: > > > On 08.12.2016 19:40, John Ferlan wrote: >> >> >> 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 > > Well I'll just set it to zero then... > Right the adjustment was obvious, but nonetheless ironic given patch 1. >> >>> 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. > > Why? _qemuAgentFsDiskAlias does not have anything to be freed. > and devAlias itself is an array of elements not array of pointers. > Perhaps it's because I read those VIR_*_N macros and I think you have an array of strings... When I see defs that have: size_t ndevAlias; /* number of elements in devAlias */ qemuAgentFsDiskAliasPtr devAlias; /* array of disk device aliases */ I'm thinking devAlias is an array of qemuAgentFsDiskAliasPtr - where each entry in the array is allocated, thus the entries and the array need to be freed. Although I think in this case you have one hunk of memory which isn't an array, but rather a ndevAlias elems in which pointer arithmetic is used to get the offset into devAlias for each element. It's the optical/mind illusion. In any case, I think my suggestion of using virDomainDefCopy will make this a moot point. >> >>> + >>> + 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 > > Well it saves a few cpu cycles. But we definetely need to change function name then. > Like qemuDomainFsInfoSteal or something to reflect the disruptive changes to the arguments. > >> >>> + >>> + 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 > > Well yes, expand does not give here much of its benefits. I can just > use alloc, but what wrong with shrink? The loop below can skip elements > so to keep memory tight shrink is useful. > >> >>> + >>> + 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; ? > > VIR_APPEND_ELEMENT reallocates on every append... > > Well I don't think stealing from domain config is a good idea)) > >> >> >>> + } >>> + >>> + 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. > > I have no objections in general to such approach and it would take much > less changes too. But I guess qemuAgentGetFSInfo is the only function in monitor > that takes domain configuration into account and odd for this reason. > I mean look at the tests - there is no more stub configurations to test monitor > except for this function. > I think this would be better too... I was trying to think if there'd be any issues if the agent or domain crashed in the mean time, but didn't come up with any. I think it's just cleaner. I don't think you want to be mucking with making just a virDomainDeviceDefCopy and modifying the existing virDomainDisk*ByAddress code to handle using that instead either. John >> >> >>> + 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; >>> } >>> -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list