On Thu, 2019-11-07 at 14:19 +0100, Michal Privoznik wrote:
The qemuDomainGetStatsIOThread() accesses the monitor by calling qemuDomainGetIOThreadsMon(). And it's also marked as "need monitor" in qemuDomainGetStatsWorkers[]. However, it's not checking if acquiring job was successful.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d17c18705b..146b4ee319 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -21194,7 +21194,7 @@ static int qemuDomainGetStatsIOThread(virQEMUDriverPtr driver, virDomainObjPtr dom, virTypedParamListPtr params, - unsigned int privflags G_GNUC_UNUSED) + unsigned int privflags) { qemuDomainObjPrivatePtr priv = dom->privateData; size_t i; @@ -21202,7 +21202,7 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr driver, int niothreads; int ret = -1;
- if (!virDomainObjIsActive(dom)) + if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) return 0;
It seems to me that not having acquired a job would be an error condition. Here you return 0 indicating that the query was successful. Also, although this change doesn't really harm anything, it doesn't quite seem like the proper fix to me. You're essentially calling a function that requires a job, and passing it a flag indicating whether we've acquired a job or not. That's a bit of an odd pattern; a flag parameter indicating whether the function should actually execute or not: void do_something(bool yes_i_really_mean_it) :) It seems like the proper fix would be to simply not call the function if we haven't acquired a job. Of course, you could still keep the above patch if you want to be cautious, but perhaps the real fix should be to filter out monitor- requiring jobs in qemuDomainGetStats() when we failed to acquire a job? Something like: diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 42922d2f04..a935ef6d97 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -21456,7 +21456,10 @@ qemuDomainGetStats(virConnectPtr conn, return -1; for (i = 0; qemuDomainGetStatsWorkers[i].func; i++) { - if (stats & qemuDomainGetStatsWorkers[i].stats) { + if (stats & qemuDomainGetStatsWorkers[i].stats && + (!qemuDomainGetStatsWorkers[i].monitor || HAVE_JOB(flags))) { if (qemuDomainGetStatsWorkers[i].func(conn->privateData, dom, params, flags) < 0) return -1; (Note: untested)
if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD))