Re: [libvirt] [PATCH 2/2] qemu: Check for job being set when getting iothread stats

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(a)redhat.com&gt; > --- > 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;