diff mbox

qemu: Remove redundant DomainObjIsActive check

Message ID fc20d29e35d2bb99f9f5d35f832fb3b70b39d832.1460724858.git.crobinso@redhat.com
State New
Headers show

Commit Message

Cole Robinson April 15, 2016, 12:54 p.m. UTC 
We perform the same check several lines earlier
---
 src/qemu/qemu_driver.c | 6 ------
 1 file changed, 6 deletions(-)

-- 
2.7.3

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

Comments

Cole Robinson April 15, 2016, 1:22 p.m. UTC | #1 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson April 15, 2016, 1:29 p.m. UTC | #2 
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

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

Patch

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;
 
-    if (!virDomainObjIsActive(vm)) {
-        virReportError(VIR_ERR_OPERATION_INVALID,
-                       "%s", _("domain is not running"));
-        goto endjob;
-    }
-
     qemuDomainObjEnterAgent(vm);
     ret = qemuAgentArbitraryCommand(priv->agent, cmd, &result, timeout);
     qemuDomainObjExitAgent(vm);
     if (ret < 0)
         VIR_FREE(result);
 
  endjob:
     qemuDomainObjEndJob(driver, vm);
 
  cleanup:
     virDomainObjEndAPI(&vm);
     return result;
 }