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

[Jonathon Jongsma <jjongsma@xxxxxxxxxx>]

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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list