[libvirt] [PATCH] qemu: Remove redundant DomainObjIsActive check
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; - 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; } -- 2.7.3
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. Jan
- 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; } -- 2.7.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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
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
participants (2)
-
Cole Robinson -
Ján Tomko