[libvirt] [PATCH 0/3] qemu: cleanup duplicate calls to DomainObjIsActive
Some API entry points have duplicate calls to DomainObjIsActive. Remove the redundant ones, and document the valid ones that confused me :) Cole Robinson (3): qemu: Remove redundant DomainObjIsActive calls qemu: Clarify usage of DomainObjIsActive for PMSuspended qemu: Clarify usage of DomainObjIsActive for SetTime src/qemu/qemu_driver.c | 43 ++++++++++++++++--------------------------- 1 file changed, 16 insertions(+), 27 deletions(-) -- 2.7.3
The common idiom in the driver API implementations is roughly: - ACL check - BeginJob (if needed) - AgentAvailable (if needed) - !IsActive A few calls had an extra !IsActive before BeginJob, which doesn't seem to serve much use. Drop them --- src/qemu/qemu_driver.c | 24 ------------------------ 1 file changed, 24 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e795062..9a3d46b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1884,12 +1884,6 @@ static int qemuDomainSuspend(virDomainPtr dom) if (virDomainSuspendEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is not running")); - goto cleanup; - } - cfg = virQEMUDriverGetConfig(driver); priv = vm->privateData; @@ -2566,12 +2560,6 @@ static int qemuDomainInjectNMI(virDomainPtr domain, unsigned int flags) if (virDomainInjectNMIEnsureACL(domain->conn, vm->def) < 0) goto cleanup; - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is not running")); - goto cleanup; - } - priv = vm->privateData; if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) @@ -15901,12 +15889,6 @@ static int qemuDomainQemuMonitorCommand(virDomainPtr domain, const char *cmd, if (virDomainQemuMonitorCommandEnsureACL(domain->conn, vm->def) < 0) goto cleanup; - 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; @@ -18510,12 +18492,6 @@ qemuDomainFSTrim(virDomainPtr dom, if (virDomainFSTrimEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - 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; -- 2.7.3
On Fri, Apr 15, 2016 at 10:00:12AM -0400, Cole Robinson wrote:
The common idiom in the driver API implementations is roughly:
- ACL check - BeginJob (if needed) - AgentAvailable (if needed) - !IsActive
A few calls had an extra !IsActive before BeginJob, which doesn't seem to serve much use. Drop them --- src/qemu/qemu_driver.c | 24 ------------------------ 1 file changed, 24 deletions(-)
ACK The call would help us error out early if: * the domain is not alive AND * we would not be able to get a job on it But we already acquired the domain object lock in qemuDomObjFromDomain. This means that either: * there is no job on the domain OR * there is a job that does unlock the domain object Since a job on inactive domain has no reason to unlock the object to enter the monitor or agent, this extra early call is unlikely to protect us from any unnecessary waiting for the job. Jan
--- src/qemu/qemu_driver.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9a3d46b..f8dfa27 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18237,6 +18237,9 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom, if (virDomainPMSuspendForDurationEnsureACL(dom->conn, vm->def) < 0) goto cleanup; + /* We also perform this check further down after grabbing the + job lock, but do it here too so we can throw an error for + an invalid config, before trying to talk to the guest agent */ if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running")); -- 2.7.3
On Fri, Apr 15, 2016 at 10:00:13AM -0400, Cole Robinson wrote:
--- src/qemu/qemu_driver.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9a3d46b..f8dfa27 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18237,6 +18237,9 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom, if (virDomainPMSuspendForDurationEnsureACL(dom->conn, vm->def) < 0) goto cleanup;
+ /* We also perform this check further down after grabbing the + job lock, but do it here too so we can throw an error for + an invalid config, before trying to talk to the guest agent */
This is wrong. Even without this check, we would not talk to the guest agent. The real reason for this check is that for an inactive domain, priv->qemuCaps is NULL and we would report QEMU_CAPS_WAKEUP as missing. Jan
if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running")); -- 2.7.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
This converts SetTime to the common obj->agent->isactive pattern (which adds a _third_ IsActive check), and documents the extra checks --- src/qemu/qemu_driver.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f8dfa27..43f22f1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18725,9 +18725,9 @@ qemuDomainSetTime(virDomainPtr dom, priv = vm->privateData; - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) - goto cleanup; - + /* We also perform this check further down after grabbing the + job lock, but do it here too so we can throw an error for + an invalid config, before trying to talk to the guest agent */ if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running")); @@ -18746,9 +18746,18 @@ qemuDomainSetTime(virDomainPtr dom, goto endjob; } + 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); rv = qemuAgentSetTime(priv->agent, seconds, nseconds, rtcSync); qemuDomainObjExitAgent(vm); @@ -18756,6 +18765,7 @@ qemuDomainSetTime(virDomainPtr dom, if (rv < 0) goto endjob; + /* The VM may have shut down inbetween, check state again */ if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running")); -- 2.7.3
On Fri, Apr 15, 2016 at 10:00:14AM -0400, Cole Robinson wrote:
This converts SetTime to the common obj->agent->isactive pattern (which adds a _third_ IsActive check), and documents the extra checks
I would not expect a commit with the summary 'Clarify usage' to change job handling.
--- src/qemu/qemu_driver.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f8dfa27..43f22f1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18725,9 +18725,9 @@ qemuDomainSetTime(virDomainPtr dom,
priv = vm->privateData;
- if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) - goto cleanup; -
This will let us report the capability error without even getting the job.
+ /* We also perform this check further down after grabbing the + job lock, but do it here too so we can throw an error for + an invalid config, before trying to talk to the guest agent */
s/trying to talk to the guest agent/acquiring the job/
if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running")); @@ -18746,9 +18746,18 @@ qemuDomainSetTime(virDomainPtr dom, goto endjob;
This goto would end the job before it was even started.
}
+ 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); rv = qemuAgentSetTime(priv->agent, seconds, nseconds, rtcSync); qemuDomainObjExitAgent(vm); @@ -18756,6 +18765,7 @@ qemuDomainSetTime(virDomainPtr dom, if (rv < 0) goto endjob;
+ /* The VM may have shut down inbetween, check state again */
This pattern is so common that commenting it is pointless. Maybe the IsActive check could be moved inside qemuDomainObjExitAgent as I've done for qemuDomainObjExitMonitor. Jan
if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running")); -- 2.7.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 04/19/2016 10:30 AM, Ján Tomko wrote:
On Fri, Apr 15, 2016 at 10:00:14AM -0400, Cole Robinson wrote:
This converts SetTime to the common obj->agent->isactive pattern (which adds a _third_ IsActive check), and documents the extra checks
I would not expect a commit with the summary 'Clarify usage' to change job handling.
--- src/qemu/qemu_driver.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f8dfa27..43f22f1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18725,9 +18725,9 @@ qemuDomainSetTime(virDomainPtr dom,
priv = vm->privateData;
- if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) - goto cleanup; -
This will let us report the capability error without even getting the job.
+ /* We also perform this check further down after grabbing the + job lock, but do it here too so we can throw an error for + an invalid config, before trying to talk to the guest agent */
s/trying to talk to the guest agent/acquiring the job/
if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running")); @@ -18746,9 +18746,18 @@ qemuDomainSetTime(virDomainPtr dom, goto endjob;
This goto would end the job before it was even started.
}
+ 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); rv = qemuAgentSetTime(priv->agent, seconds, nseconds, rtcSync); qemuDomainObjExitAgent(vm); @@ -18756,6 +18765,7 @@ qemuDomainSetTime(virDomainPtr dom, if (rv < 0) goto endjob;
+ /* The VM may have shut down inbetween, check state again */
This pattern is so common that commenting it is pointless. Maybe the IsActive check could be moved inside qemuDomainObjExitAgent as I've done for qemuDomainObjExitMonitor.
Thanks for your comments, I'll look into that - Cole
participants (2)
-
Cole Robinson -
Ján Tomko