Re: [libvirt] [v2 RESEND 3/3] qemu: Switch code to use new agent job APIs

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(a)redhat.com&gt; > --- > 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.