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

On Thu, 2019-11-07 at 14:19 +0100, Michal Privoznik wrote: 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)