On 11/24/2016 04:19 AM, Nikolay Shirokovskiy wrote:

In case of 0 filesystems *info is not set while according to virDomainGetFSInfo contract user should call free on it even in case of 0 filesystems. Thus we need to properly set it. NULL will be enough as free eats NULLs ok. --- src/qemu/qemu_agent.c | 1 + 1 file changed, 1 insertion(+)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index ec8d47e..c5cf403 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1872,6 +1872,7 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info, ndata = virJSONValueArraySize(data); if (!ndata) { ret = 0; + *info = NULL;

ACK - although there are more ways above this hunk that allow us to get to cleanup without setting *info = NULL; Currently each of the callers sets the input info to NULL before calling here IOW: We could also move that *info = NULL up before the call to virAgentMakeCommand John

goto cleanup; } if (VIR_ALLOC_N(info_ret, ndata) < 0)

On 12/09/2016 02:27 AM, Nikolay Shirokovskiy wrote:

On 08.12.2016 19:38, John Ferlan wrote:

...

On 11/24/2016 04:19 AM, Nikolay Shirokovskiy wrote:

...

In case of 0 filesystems *info is not set while according to virDomainGetFSInfo contract user should call free on it even in case of 0 filesystems. Thus we need to properly set it. NULL will be enough as free eats NULLs ok. --- src/qemu/qemu_agent.c | 1 + 1 file changed, 1 insertion(+)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index ec8d47e..c5cf403 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1872,6 +1872,7 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info, ndata = virJSONValueArraySize(data); if (!ndata) { ret = 0; + *info = NULL;

ACK - although there are more ways above this hunk that allow us to get to cleanup without setting *info = NULL; Currently each of the callers

These are error paths. The caller have no obligations to free info in these cases.

...

sets the input info to NULL before calling here

qemuDomainGetFSInfo does not set...

...

IOW: We could also move that *info = NULL up before the call to virAgentMakeCommand

I looked here and there in the libvirt code and found out that it does not zero out output parameter immediately. I guess it makes sense. Output parameter can be actually filled in deep below the call stack. Thus if one starts with convention to zero out immediately one have to do it in every function on the path.

I have a recollection of having it mentioned for one of my code reviews, hence why setting *info = NULL unconditionally early on has stuck with me... See virDomainGetIOThreadInfo. Ironically virDomainGetFSInfo also has a *info = NULL prior to calling domainGetFSInfo which calls into qemuDomainGetFSInfo. As for testQemuAgentGetFSInfo (the other consumer of qemuAgentGetFSInfo), it too has a qemuAgentFsInfoPtr *info = NULL; at the top so well it should be happy, except that I see/note qemuAgentGetFSInfo is called a second time, but in this case it expects some sort of failure (it's not all that clear what's being tested, but that's a different issue). In any case, prior to that call info was not freed nor set to NULL (maybe that's the test case - I didn't think too long and hard about it). In any case, whether you decide in your v2 to put the *info at the top or keep it where it is - doesn't matter to retain the ACK. John

I guess caller itself can zero out output parameter to simplify its cleanup logic. And some callers are not zero out, they cleanup conditionally for example - these rely on function to properly set output parameter.

Nikolay

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

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

On 11/24/2016 04:19 AM, Nikolay Shirokovskiy wrote:

qemuAgentNotifyEvent notify on a lock condition without taking the lock. This works but it is a subject to race conditions. --- src/qemu/qemu_agent.c | 4 ++++ 1 file changed, 4 insertions(+)

But the vm is locked prior to any priv->agent dereference and call - so what path could free priv->agent before we get into this NotifyEvent code? I suppose it wouldn't hurt, but we're not entering the agent and the AgentEOF would require vm lock to clear agent. John

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 5230cbc..ad031d0 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1248,6 +1248,8 @@ qemuAgentMakeStringsArray(const char **strings, unsigned int len) void qemuAgentNotifyEvent(qemuAgentPtr mon, qemuAgentEvent event) { + virObjectLock(mon); + VIR_DEBUG("mon=%p event=%d await_event=%d", mon, event, mon->await_event); if (mon->await_event == event) { mon->await_event = QEMU_AGENT_EVENT_NONE; @@ -1257,6 +1259,8 @@ void qemuAgentNotifyEvent(qemuAgentPtr mon, virCondSignal(&mon->notify); } } + + virObjectUnlock(mon); }

VIR_ENUM_DECL(qemuAgentShutdownMode);

On 12/09/2016 03:55 AM, Nikolay Shirokovskiy wrote:

On 08.12.2016 19:40, John Ferlan wrote:

...

On 11/24/2016 04:19 AM, Nikolay Shirokovskiy wrote:

...

qemuAgentNotifyEvent notify on a lock condition without taking the lock. This works but it is a subject to race conditions. --- src/qemu/qemu_agent.c | 4 ++++ 1 file changed, 4 insertions(+)

But the vm is locked prior to any priv->agent dereference and call - so what path could free priv->agent before we get into this NotifyEvent

This patche does not fix NULL or dangling pointer reference...

...

code? I suppose it wouldn't hurt, but we're not entering the agent and the AgentEOF would require vm lock to clear agent.

It is just we can not deliver signal reliably if lock is not taken. It is race condition. Acessing monitor fields without lock in general is risky too.

For example imagine we are in the process of sending agent command, vm lock is dropped, sending thread holds agent lock and at the same time reset comes - it is possible as nobody holds vm lock, so we come here and now we have dangerous simultaneous access from both threads - sending and that notifies us of a reset.

OK - and this is the type of information/details that should go into the commit message. When you chase something down, leave a few bread crumbs of details. Anyway the details are, await_event is only set for qemuAgentShutdown and qemuAgentSuspend. Both have the agent lock to do so and their callers are the only ones that care. So the issue is that one of those two is called and at some time after releasing the domain lock and before setting await_event to RESET, SHUTDOWN, or SUSPEND that qemuAgentNotifyEvent is called from qemuProcessHandleReset, qemuProcessHandleShutdown, qemuProcessHandlePMSuspend, or qemuProcessHandlePMSuspendDisk which only take the vmobj lock before looking at agent fields. With that kind of detail provided in the commit message wouldn't leave me wondering what was being chased. As you saw my first assumption was the race you were referring to is the agent free race. With the extra details I think I'm able to piece together what race you are referring to. So ACK to the patch, but when you create your v2 for this series, please update the commit message with more details. Feel free to liberally steal the above paragraph. John

Nikolay

...

...

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 5230cbc..ad031d0 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1248,6 +1248,8 @@ qemuAgentMakeStringsArray(const char **strings, unsigned int len) void qemuAgentNotifyEvent(qemuAgentPtr mon, qemuAgentEvent event) { + virObjectLock(mon); + VIR_DEBUG("mon=%p event=%d await_event=%d", mon, event, mon->await_event); if (mon->await_event == event) { mon->await_event = QEMU_AGENT_EVENT_NONE; @@ -1257,6 +1259,8 @@ void qemuAgentNotifyEvent(qemuAgentPtr mon, virCondSignal(&mon->notify); } } + + virObjectUnlock(mon); }

VIR_ENUM_DECL(qemuAgentShutdownMode);