[libvirt] [PATCH 2/2] qemu: Check for job being set when getting iothread stats
Jonathon Jongsma
jjongsma at redhat.com
Thu Nov 7 17:29:37 UTC 2019
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 at 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))
More information about the libvir-list
mailing list