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

[Michal Privoznik]

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

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.

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