8:42 a.m.
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;
>> }
>>