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

[Michal Privoznik]

On 8/26/19 6:08 PM, Jonathon Jongsma wrote:
> 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(+)
>>>

> 
> 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?

Very good point. Yeah, they should be traeted the same.

> 
> 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?

Agreed, it adds needless complexity.

> 
> 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()?

D'oh! Yes it is not necessary, we have jobs to prevent domain object 
modifications when the lock is dropped. Let me post a patch for that.
The domain def copying was introduced in v3.0.0-rc1~336 but the commit 
message is unclear why the copy is needed. Anyway, we have jobs so that 
we don't need to create duplicates.

BTW: jobs are again a complex idea. But put simply: it's an integer 
inside qemuDomainObjPrivatePtr (which can be accessed via 
vm->privateData) and acquiring a job means setting the integer to a 
non-zero value (with optional wait if the integer is set) and releasing 
the job then means setting the integer to zero. This means that if two 
threads fight over access of the same domain only one will succeed in 
setting the integer and the other has to wait until the job is released.
But this means that the domain can be locked and unlocked at will if it 
has a job acquired - no one else will modify the domain object (nor its 
definition since it's part of domain object).

This is documented in src/qemu/THREADS.txt

Michal