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;

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(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; > > 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.

But this patch is consistent with the others, so Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt;