On Tue, Jun 19, 2018 at 08:38:02 +0200, Michal Privoznik wrote:
> There are two sets of functions here:
> 1) some functions talk on both monitor and agent monitor,
> 2) some functions only talk on agent monitor.
>
> For functions from set 1) we need to use
> qemuDomainObjBeginJobWithAgent() and for functions from set 2) we
> need to use qemuDomainObjBeginAgentJob() only.
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> src/qemu/qemu_driver.c | 91 ++++++++++++++++++++++++++++++++------------------
> 1 file changed, 58 insertions(+), 33 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 3abbe41895..cffd4c928a 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1954,6 +1954,7 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags)
> bool useAgent = false, agentRequested, acpiRequested;
> bool isReboot = false;
> bool agentForced;
> + bool agentJob = false;
> int agentFlag = QEMU_AGENT_SHUTDOWN_POWERDOWN;
>
> virCheckFlags(VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN |
> @@ -1980,9 +1981,14 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags)
> if (virDomainShutdownFlagsEnsureACL(dom->conn, vm->def, flags) < 0)
> goto cleanup;
>
> - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> + if ((!useAgent && qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) ||
> + (useAgent && qemuDomainObjBeginJobWithAgent(driver, vm,
> + QEMU_JOB_MODIFY,
> + QEMU_AGENT_JOB_MODIFY) < 0))
Looks a bit hard to parse, it would be easier to just call
qemuDomainObjBeginJobWithAgent:
qemuDomainAgentJob agentJob = QEMU_AGENT_JOB_NONE;
if (useAgent)
agentJob = QEMU_AGENT_JOB_MODIFY;
if (qemuDomainObjBeginJobWithAgent(driver, vm, QEMU_JOB_MODIFY,
agentJob) < 0)
goto cleanup;
Perhaps it would even make sense to document this usage if the caller
does not always need to talk to the agent.
> @@ -2026,7 +2032,10 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags)
> }
>
> endjob:
> - qemuDomainObjEndJob(driver, vm);
> + if (agentJob)
> + qemuDomainObjEndJobWithAgent(driver, vm);
> + else
> + qemuDomainObjEndJob(driver, vm);
And this would still work even with the suggested changes.
>
> cleanup:
> virDomainObjEndAPI(&vm);
> @@ -2049,6 +2058,7 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags)
> bool useAgent = false, agentRequested, acpiRequested;
> bool isReboot = true;
> bool agentForced;
> + bool agentJob = false;
> int agentFlag = QEMU_AGENT_SHUTDOWN_REBOOT;
>
> virCheckFlags(VIR_DOMAIN_REBOOT_ACPI_POWER_BTN |
> @@ -2075,9 +2085,14 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags)
> if (virDomainRebootEnsureACL(dom->conn, vm->def, flags) < 0)
> goto cleanup;
>
> - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> + if ((!useAgent && qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) ||
> + (useAgent && qemuDomainObjBeginJobWithAgent(driver, vm,
> + QEMU_JOB_MODIFY,
> + QEMU_AGENT_JOB_MODIFY) < 0))
The same pattern could be used here.
> goto cleanup;
>
> + agentJob = useAgent;
> +
> agentForced = agentRequested && !acpiRequested;
> if (!qemuDomainAgentAvailable(vm, agentForced)) {
> if (agentForced)
> @@ -2115,7 +2130,10 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags)
> }
>
> endjob:
> - qemuDomainObjEndJob(driver, vm);
> + if (agentJob)
> + qemuDomainObjEndJobWithAgent(driver, vm);
> + else
> + qemuDomainObjEndJob(driver, vm);
>
> cleanup:
> virDomainObjEndAPI(&vm);
> @@ -4949,6 +4967,7 @@ qemuDomainSetVcpusFlags(virDomainPtr dom,
> virDomainDefPtr def;
> virDomainDefPtr persistentDef;
> bool hotpluggable = !!(flags & VIR_DOMAIN_VCPU_HOTPLUGGABLE);
> + bool useAgent = !!(flags & VIR_DOMAIN_VCPU_GUEST);
> int ret = -1;
>
> virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> @@ -4963,13 +4982,14 @@ qemuDomainSetVcpusFlags(virDomainPtr dom,
> if (virDomainSetVcpusFlagsEnsureACL(dom->conn, vm->def, flags) < 0)
> goto cleanup;
>
> - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> + if ((!useAgent && qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) ||
> + (useAgent && qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_MODIFY) < 0))
> goto cleanup;
And here.
>
> if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0)
> goto endjob;
>
> - if (flags & VIR_DOMAIN_VCPU_GUEST)
> + if (useAgent)
> ret = qemuDomainSetVcpusAgent(vm, nvcpus);
> else if (flags & VIR_DOMAIN_VCPU_MAXIMUM)
> ret = qemuDomainSetVcpusMax(driver, def, persistentDef, nvcpus);
> @@ -4978,7 +4998,10 @@ qemuDomainSetVcpusFlags(virDomainPtr dom,
> nvcpus, hotpluggable);
>
> endjob:
> - qemuDomainObjEndJob(driver, vm);
> + if (useAgent)
> + qemuDomainObjEndAgentJob(vm);
> + else
> + qemuDomainObjEndJob(driver, vm);
>
> cleanup:
> virDomainObjEndAPI(&vm);
> @@ -5429,7 +5452,7 @@ qemuDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags)
> goto cleanup;
>
> if (flags & VIR_DOMAIN_VCPU_GUEST) {
> - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
> + if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_QUERY) < 0)
> goto cleanup;
>
> if (!virDomainObjIsActive(vm)) {
> @@ -5447,7 +5470,7 @@ qemuDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags)
> qemuDomainObjExitAgent(vm, agent);
>
> endjob:
> - qemuDomainObjEndJob(driver, vm);
> + qemuDomainObjEndAgentJob(vm);
>
> if (ncpuinfo < 0)
> goto cleanup;
I'd expect changes to qemuDomainSnapshotCreateActiveExternal here since
it calls qemuDomainSnapshotFSFreeze and qemuDomainSnapshotFSThaw from an
async job without acquiring a nested job, which looks wrong. And this
patch should change it to acquire an agent job before talking to the
agent.
> @@ -18954,7 +18977,7 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom,
> if (virDomainPMSuspendForDurationEnsureACL(dom->conn, vm->def) < 0)
> goto cleanup;
>
> - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> + if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_MODIFY) < 0)
> goto cleanup;
>
> if (virDomainObjCheckActive(vm) < 0)
...
The rest looks OK.
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
