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

[Jiri Denemark]

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 at redhat.com>
> ---
>  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