On Thu, 2019-11-07 at 14:18 +0100, Michal Privoznik wrote:

The qemuDomainObjEnterMonitor() should not be called from without a job set. Catch this error and produce a warning message if such call occurred.

s/from// Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 5 +++++ 1 file changed, 5 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 667cc89072..4661deb027 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8682,6 +8682,11 @@ qemuDomainObjEnterMonitorInternal(virQEMUDriverPtr driver, } else if (priv->job.asyncOwner == virThreadSelfID()) { VIR_WARN("This thread seems to be the async job owner; entering" " monitor without asking for a nested job is dangerous"); + } else if (priv->job.owner != virThreadSelfID()) { + VIR_WARN("Entering a monitor without owning a job. " + "Job %s owner %s (%llu)", + qemuDomainJobTypeToString(priv->job.active), + priv->job.ownerAPI, priv->job.owner); }

VIR_DEBUG("Entering monitor (mon=%p vm=%p name=%s)",

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))

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@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;

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@redhat.com>

As for returning success, the virConnectGetAllDomainStats() API is document as:

Note that entire stats groups or individual stat fields may be missing from the output in case they are not supported by the given hypervisor, are not applicable for the current state of the guest domain, or their retrieval was not successful.

The way I read this is: if unable to acquire job, omit iothread stats without reporting error. In fact, this is what other callbacks do too.

Michal