[libvirt] [PATCH] qemu: Remove redundant DomainObjIsActive check

[Cole Robinson]

On 04/15/2016 09:22 AM, Cole Robinson wrote:
> 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
> 

Hmm, a couple have arguably valid use cases. I'll add a separate patch with
comments for those

- Cole