On 11/8/19 4:05 PM, Jonathon Jongsma wrote:
On Fri, 2019-11-08 at 10:44 +0100, Michal Privoznik wrote:
On 11/7/19 6:29 PM, Jonathon Jongsma wrote:
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@xxxxxxxxxx>
---
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;
These stats querying callbacks work in a different way than the rest
of
the code. The entry point is qemuConnectGetAllDomainStats(). It
iterates
over the list of domains and tries to acquire job for each domain
that
it's currently iterating over. This may of course fail (or, if
NOWAIT
flag is specified then user specifically requested to skip domains
"blocked" with a job). Anyway, there are some stats that require
job,
some that don't, and some that are somewhere in the middle (e.g.
qemuDomainGetStatsVcpu), i.e. they can get some basic info and if job
is
acquired they can gather even more info. Therefore, we can't check
for
HAVE_JOB() in qemuDomainGetStats() but rather in each callback
individually.
Aha, I had missed the part where some functions can get a reduced set
of stats despite having .monitor=true. Sorry for that.
It still seems a bit of an odd pattern to me. In some ways, I think it
would be cleaner to change the qemuDomainGetStatsWorker.monitor
variable from a boolean to an enumeration that can be one of
MONITOR_NONE, MONITOR_OPTIONAL, MONITOR_REQUIRED, so we have enough
information to decide whether to call the function or not.
Well, if we had such variable, it wouldn't help really, would it? I
mean, maybe for MONITOR_REQUIRED we could skip calling the function
completely, but for the rest, we would still need to call the function
and check the state inside anyways.
But this patch is consistent with the others, so
Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
Thanks, I've pushed both.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
