3:19 a.m.
Patch 1 is a nitpick I found when working on Patch 2.
Patch 2 and 3 fix a couple of race conditions in code.
A lot of changes is inherent refactorings in Patch 2 actually.
Nikolay Shirokovskiy (3):
qemu: agent: fix uninitialized var case in qemuAgentGetFSInfo
qemu: don't use vmdef without domain lock
qemu: agent: take monitor lock in qemuAgentNotifyEvent
src/qemu/qemu_agent.c | 57 +++++++++++++--------
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, 164 insertions(+), 93 deletions(-)
delete mode 100644 tests/qemuagentdata/qemuagent-fsinfo.xml
--
1.8.3.1
3:19 a.m.
New subject: [libvirt] [PATCH 1/3] qemu: agent: fix uninitialized var case in qemuAgentGetFSInfo
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;
goto cleanup;
}
if (VIR_ALLOC_N(info_ret, ndata) < 0)
--
1.8.3.1
10:38 a.m.
New subject: [libvirt] [PATCH 1/3] qemu: agent: fix uninitialized var case in qemuAgentGetFSInfo
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)
1:27 a.m.
New subject: [libvirt] [PATCH 1/3] qemu: agent: fix uninitialized var case in qemuAgentGetFSInfo
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 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
8:27 a.m.
New subject: [libvirt] [PATCH 1/3] qemu: agent: fix uninitialized var case in qemuAgentGetFSInfo
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
3:19 a.m.
New subject: [libvirt] [PATCH 2/3] qemu: don't use vmdef without domain lock
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
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);
+
+ 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)
+{
+ 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;
+
+ 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;
+
+ 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;
+ }
+
+ 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;
+ 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);
+ 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;
}
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;
}
--
1.8.3.1
10:40 a.m.
New subject: [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;
}
2:37 a.m.
New subject: [libvirt] [PATCH 2/3] qemu: don't use vmdef without domain lock
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...
> 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.
> +
> + 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.
> + 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;
> }
>
8:42 a.m.
New subject: [libvirt] [PATCH 2/3] qemu: don't use vmdef without domain lock
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;
>> }
>>
3:19 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: agent: take monitor lock in qemuAgentNotifyEvent
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(+)
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);
--
1.8.3.1
10:40 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: agent: take monitor lock in qemuAgentNotifyEvent
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);
2:55 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: agent: take monitor lock in qemuAgentNotifyEvent
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.
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);
>
8:42 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: agent: take monitor lock in qemuAgentNotifyEvent
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);
>>
1:42 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: agent: take monitor lock in qemuAgentNotifyEvent
On 09.12.2016 17:42, John Ferlan wrote:
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.
Ok.
Honestly I did not think of a detailed race screnario when I send a patch.
It is just hit me that notify is called without a lock, I thought before it
is incorrect code. As it turns out it is not, one can call notify without a
lock. However there is a reason why it is worth taking lock anyway -
http://stackoverflow.com/questions/4544234/calling-pthread-cond-signal-wi...
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);
>>>
2901
days inactive
2919
days old
13 comments
2 participants
participants (2)
-
John Ferlan -
Nikolay Shirokovskiy