[libvirt] [PATCH v3 8/9] qemu: Implement virDomainGetGuestInfo()

[Michal Privoznik]

On 8/23/19 6:31 PM, Jonathon Jongsma wrote:
> Iimplements the new guest information API by querying requested
> information via the guest agent.
> 
> Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> ---
>   src/qemu/qemu_driver.c | 110 +++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 110 insertions(+)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 1b051a9424..446266e66b 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -23190,6 +23190,115 @@ qemuDomainGetLaunchSecurityInfo(virDomainPtr domain,
>       return ret;
>   }
>   
> +static unsigned int supportedGuestInfoTypes =

This can be const since it's never modified.

> +    VIR_DOMAIN_GUEST_INFO_USERS |
> +    VIR_DOMAIN_GUEST_INFO_OS |
> +    VIR_DOMAIN_GUEST_INFO_TIMEZONE |
> +    VIR_DOMAIN_GUEST_INFO_HOSTNAME |
> +    VIR_DOMAIN_GUEST_INFO_FILESYSTEM;
> +
> +static void
> +qemuDomainGetGuestInfoCheckSupport(unsigned int *types)
> +{
> +    if (*types == 0)
> +        *types = supportedGuestInfoTypes;
> +
> +    *types = *types & supportedGuestInfoTypes;
> +}
> +
> +static int
> +qemuDomainGetGuestInfo(virDomainPtr dom,
> +                       unsigned int types,
> +                       virTypedParameterPtr *params,
> +                       int *nparams,
> +                       unsigned int flags)
> +{
> +    virQEMUDriverPtr driver = dom->conn->privateData;
> +    virDomainObjPtr vm = NULL;
> +    qemuAgentPtr agent;
> +    int ret = -1;
> +    int rv = -1;

Huh, this @rv and @ret are used a bit misleadinly :-D

> +    int maxparams = 0;
> +    char *hostname = NULL;

VIR_AUTOFREE()

> +    virDomainDefPtr def = NULL;
> +    virCapsPtr caps = NULL;
> +    unsigned int supportedTypes = types;
> +
> +    virCheckFlags(0, ret);
> +    qemuDomainGetGuestInfoCheckSupport(&supportedTypes);
> +
> +    if (!(vm = qemuDomObjFromDomain(dom)))
> +        goto cleanup;
> +
> +    if (virDomainGetGuestInfoEnsureACL(dom->conn, vm->def) < 0)
> +        goto cleanup;
> +
> +    if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_QUERY) < 0)
> +        goto cleanup;
> +
> +    if (!qemuDomainAgentAvailable(vm, true))
> +        goto endjob;
> +
> +    agent = qemuDomainObjEnterAgent(vm);
> +
> +    /* Although the libvirt qemu driver supports all of these guest info types,
> +     * some guest agents might be too old to support these commands. If these
> +     * info categories were explicitly requested (i.e. 'types' is non-zero),
> +     * abort and report an error on any failures, otherwise continue and return
> +     * as much info as is supported by the guest agent. */
> +    if (supportedTypes & VIR_DOMAIN_GUEST_INFO_USERS) {
> +        if (qemuAgentGetUsers(agent, params, nparams, &maxparams) < 0 &&

This might not do what you think it does. qemuAgentGetUsers() (and the 
rest of qemuAgentXXX() for that matter) can fail because of variety of 
reasons. We want to continue if and only if command was not found and 
halt in all other cases. But I'll leave this as is for now and probably 
fix this in a follow up patch. Unless you want to work on that. If you 
do, then you might want to take a look how we do that in qemu monitor code:

   qemuMonitorJSONHasError(reply, "CommandNotFound"))

> +            types != 0)
> +            goto exitagent;
> +    }
> +    if (supportedTypes & VIR_DOMAIN_GUEST_INFO_OS) {
> +        if (qemuAgentGetOSInfo(agent, params, nparams, &maxparams) < 0
> +            && types != 0)
> +            goto exitagent;
> +    }
> +    if (supportedTypes & VIR_DOMAIN_GUEST_INFO_TIMEZONE) {
> +        if (qemuAgentGetTimezone(agent, params, nparams, &maxparams) < 0
> +            && types != 0)
> +            goto exitagent;
> +    }
> +    if (supportedTypes & VIR_DOMAIN_GUEST_INFO_HOSTNAME) {
> +        if (qemuAgentGetHostname(agent, &hostname) < 0) {
> +            if (types != 0)
> +                goto exitagent;
> +        } else {
> +            if (virTypedParamsAddString(params, nparams, &maxparams, "hostname",
> +                                        hostname) < 0)
> +                goto exitagent;
> +        }
> +    }
> +    if (supportedTypes & VIR_DOMAIN_GUEST_INFO_FILESYSTEM) {
> +        if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
> +            goto exitagent;
> +
> +        if (!(def = virDomainDefCopy(vm->def, caps, driver->xmlopt, NULL, false)))
> +            goto exitagent;
> +

No need to create the copy of the domain def (which is really expensive).

> +        if (qemuAgentGetFSInfoParams(agent, params, nparams, &maxparams, def) < 0 &&
> +            types != 0)
> +            goto exitagent;
> +    }
> +
> +    rv = 0;
> +
> + exitagent:
> +    qemuDomainObjExitAgent(vm, agent);
> +
> + endjob:
> +    qemuDomainObjEndAgentJob(vm);
> +
> + cleanup:
> +    virDomainObjEndAPI(&vm);
> +    virDomainDefFree(def);
> +    virObjectUnref(caps);
> +    VIR_FREE(hostname);
> +    return rv;
> +}
> +
>   static virHypervisorDriver qemuHypervisorDriver = {
>       .name = QEMU_DRIVER_NAME,
>       .connectURIProbe = qemuConnectURIProbe,
> @@ -23425,6 +23534,7 @@ static virHypervisorDriver qemuHypervisorDriver = {
>       .domainCheckpointLookupByName = qemuDomainCheckpointLookupByName, /* 5.6.0 */
>       .domainCheckpointGetParent = qemuDomainCheckpointGetParent, /* 5.6.0 */
>       .domainCheckpointDelete = qemuDomainCheckpointDelete, /* 5.6.0 */
> +    .domainGetGuestInfo = qemuDomainGetGuestInfo, /* 5.6.0 */

s/6/7/

Michal