Re: [libvirt] [PATCH] qemu: Remove redundant DomainObjIsActive check
On 04/15/2016 08:59 AM, Ján Tomko wrote:
On Fri, Apr 15, 2016 at 08:54:23AM -0400, Cole Robinson wrote:
> We perform the same check several lines earlier
> ---
> src/qemu/qemu_driver.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index e795062..ced808a 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -18403,32 +18403,26 @@ qemuDomainQemuAgentCommand(virDomainPtr domain,
>
> if (!virDomainObjIsActive(vm)) {
> virReportError(VIR_ERR_OPERATION_INVALID,
> "%s", _("domain is not running"));
> goto cleanup;
> }
>
> if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> goto cleanup;
>
> if (!qemuDomainAgentAvailable(vm, true))
> goto endjob;
>
This is not redundant, the domain might stop running while we wait for
the job with the domain lock unlocked.
Okay, I dug a bit deeper.
I grepped through all "domain is not running" errors and the only functions
that have duplicate IsActive calls are qemuDomainSuspend, InjectNMI,
qemuDomainQemuMonitorCommand, qemuDomainPMSuspendForDuration,
qemuDomainFSTrim, qemuDomainSetTime, which all have an additional one _before_
the BeginJob check, which AFAICT only errors a bit earlier instead of waiting
for the job lock, which isn't that useful. I looked at git history for
DomainSuspend and DomainQemuMonitorCommand and it looks like both were added
basically accidentally without any explicit purpose.
The pattern that most functions call, and which seems ideal to me, is:
- ACL check
- BeginJob (if needed)
- AgentAvailable (if needed, which btw checks domain is RUNNING btw)
- !IsActive
I'll send a patch to fix those up, seems like most cases were cargo culted anyways
- Cole