5:13 p.m.
Changes from v1:
1. patch 2: make vm def copy instead of making qemuAgentGetFSInfo independent of domain
conf.
2. patch 3: provide more relevant commit message
Nikolay Shirokovskiy (3):
qemu: agent: fix uninitialized var case in qemuAgentGetFSInfo
qemu: don't use vm when lock is dropped in qemuDomainGetFSInfo
qemu: agent: take monitor lock in qemuAgentNotifyEvent
src/qemu/qemu_agent.c | 5 +++++
src/qemu/qemu_driver.c | 12 +++++++++++-
2 files changed, 16 insertions(+), 1 deletion(-)
--
1.8.3.1
5:13 p.m.
New subject: [libvirt] [PATCH v2 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 1d677f7..c50f760 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
5:13 p.m.
New subject: [libvirt] [PATCH v2 2/3] qemu: don't use vm when lock is dropped in qemuDomainGetFSInfo
Current call to qemuAgentGetFSInfo in qemuDomainGetFSInfo is
unsafe. Domain lock is dropped and we use vm->def. Let's make
def copy to fix that.
---
src/qemu/qemu_driver.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 842de0a..976ccb5 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -19749,6 +19749,8 @@ qemuDomainGetFSInfo(virDomainPtr dom,
virQEMUDriverPtr driver = dom->conn->privateData;
virDomainObjPtr vm;
qemuAgentPtr agent;
+ virCapsPtr caps = NULL;
+ virDomainDefPtr def = NULL;
int ret = -1;
virCheckFlags(0, ret);
@@ -19771,8 +19773,14 @@ qemuDomainGetFSInfo(virDomainPtr dom,
if (!qemuDomainAgentAvailable(vm, true))
goto endjob;
+ if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
+ goto endjob;
+
+ if (!(def = virDomainDefCopy(vm->def, caps, driver->xmlopt, NULL, false)))
+ goto endjob;
+
agent = qemuDomainObjEnterAgent(vm);
- ret = qemuAgentGetFSInfo(agent, info, vm->def);
+ ret = qemuAgentGetFSInfo(agent, info, def);
qemuDomainObjExitAgent(vm, agent);
endjob:
@@ -19780,6 +19788,8 @@ qemuDomainGetFSInfo(virDomainPtr dom,
cleanup:
virDomainObjEndAPI(&vm);
+ virDomainDefFree(def);
+ virObjectUnref(caps);
return ret;
}
--
1.8.3.1
5:13 p.m.
New subject: [libvirt] [PATCH v2 3/3] qemu: agent: take monitor lock in qemuAgentNotifyEvent
qemuAgentNotifyEvent accesses monitor structure and is called on qemu
reset/shutdown/suspend events under domain lock. Other monitor
functions on the other hand take monitor lock and don't hold domain lock.
Thus it is possible to have risky simultaneous access to the structure
from 2 threads. Let's take monitor lock here to make access exclusive.
---
John, I decided to formulate patch purpuse this way as I doubt we
can have actual signalling race here becase shutdown/suspend functions
first set await_event and then send message to agent which in turn causes
the event that qemuAgentNotifyEvent handles.
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 c50f760..46cad53 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
6:58 a.m.
On 12/12/2016 04:13 AM, Nikolay Shirokovskiy wrote:
Changes from v1:
1. patch 2: make vm def copy instead of making qemuAgentGetFSInfo independent of domain
conf.
2. patch 3: provide more relevant commit message
Nikolay Shirokovskiy (3):
qemu: agent: fix uninitialized var case in qemuAgentGetFSInfo
qemu: don't use vm when lock is dropped in qemuDomainGetFSInfo
qemu: agent: take monitor lock in qemuAgentNotifyEvent
src/qemu/qemu_agent.c | 5 +++++
src/qemu/qemu_driver.c | 12 +++++++++++-
2 files changed, 16 insertions(+), 1 deletion(-)
ACK series and pushed.
John
BTW: Try to keep in mind for future patches we'd like to have features,
enhancements, fixes, etc. logged in news.html.in
If you think what I've pushed already should have some text, then create
a patch for that...
2:21 p.m.
On 13.12.2016 01:58, John Ferlan wrote:
On 12/12/2016 04:13 AM, Nikolay Shirokovskiy wrote:
> Changes from v1:
> 1. patch 2: make vm def copy instead of making qemuAgentGetFSInfo independent of
domain conf.
> 2. patch 3: provide more relevant commit message
>
> Nikolay Shirokovskiy (3):
> qemu: agent: fix uninitialized var case in qemuAgentGetFSInfo
> qemu: don't use vm when lock is dropped in qemuDomainGetFSInfo
> qemu: agent: take monitor lock in qemuAgentNotifyEvent
>
> src/qemu/qemu_agent.c | 5 +++++
> src/qemu/qemu_driver.c | 12 +++++++++++-
> 2 files changed, 16 insertions(+), 1 deletion(-)
>
ACK series and pushed.
Thanxs!
John
BTW: Try to keep in mind for future patches we'd like to have features,
enhancements, fixes, etc. logged in news.html.in
Ok.
If you think what I've pushed already should have some text, then create
a patch for that...
Looks like these changes don't need to be mentioned there.
Nikolay
2879
days inactive
2880
days old
5 comments
2 participants
participants (2)
-
John Ferlan -
Nikolay Shirokovskiy