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

[Jonathon Jongsma]

On Mon, 2019-08-26 at 17:29 +0200, Michal Privoznik wrote:
> 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"))

I actually debated about whether to differentiate based on the error
response, but ended up choosing not to. But I agree that it probably is
better if we do. Should I treat CommandNotFound and CommandDisabled the
same for this purpose?

I also debated whether to first query the 'guest-info' command (which
returns an array of 'supported_commands') to see which commands are
supported by the guest agent. Then I could use that response to set the
value of 'supportedGuestInfoTypes'. In the end I decided that this
added more complication than benefit and decided to simply execute the
command and see whether it failed. Do you have an opionion on this
approach?

I'll revise the patch in either case.



> 
> > +            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).

Hmm, perhaps this is a case of reusing existing code without thinking
about it carefully enough. Does that also mean that it's not necessary
to copy in qemuDomainGetFSInfo()?

> 
> > +        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
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list