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