Re: [libvirt] [PATCH v3 8/9] qemu: Implement virDomainGetGuestInfo()
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(a)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