[libvirt] [PATCH v2] qemu: Implement DomainPMSuspendForDuration

[Michal Privoznik]

On 10.02.2012 22:25, Eric Blake wrote:
> On 02/10/2012 04:25 AM, Michal Privoznik wrote:
>> via user agent.
>> ---
>> diff to v1:
>> -allow mem and hybrid targets iff system_wakeup monitor command is presented
> 
> Before I review too much of this, remember that there is still a pending
> discussion that might rename the API slightly to add in the wakeup.
> 
>>
>>  src/qemu/qemu_agent.c        |   31 +++++++++++++++
>>  src/qemu/qemu_agent.h        |    2 +
>>  src/qemu/qemu_capabilities.c |    1 +
>>  src/qemu/qemu_capabilities.h |    1 +
>>  src/qemu/qemu_driver.c       |   87 ++++++++++++++++++++++++++++++++++++++++++
>>  src/qemu/qemu_monitor.c      |   21 +++++-----
>>  src/qemu/qemu_monitor.h      |    4 +-
>>  src/qemu/qemu_monitor_json.c |   15 ++++---
>>  src/qemu/qemu_monitor_json.h |    5 ++-
>>  src/qemu/qemu_process.c      |    2 +-
>>  10 files changed, 149 insertions(+), 20 deletions(-)
> 
> I'm wondering if it might be worth dividing this into two patches - the
> first that refactors qemu_capabilities and qemu_monitor to set
> QEMU_CAPS_WAKEUP, and the second that touches qemu_agent, qemu_driver,
> and qemu_monitor to wire up the suspend command.
> 
> Another thing that would be nice to do would be improving the
> qemuhelptest.c to allow the testsuite to start probing for JSON
> responses; I would add some new files qemuhelpdata/qemu-*-monitor, which
> contains the JSON output corresponding to the query-commands input for
> the varous qemu versions that support JSON, so we make sure we can
> enable caps according to what json strings we parse.

Yeah, but I'd like to have this in a separate patch, as it is not
trivial and a bit overkill for this functionality I am adding.
> 
> 
>> +static int
>> +qemuDomainPMSuspendForDuration(virDomainPtr dom,
>> +                               unsigned int target,
>> +                               unsigned long long duration,
>> +                               unsigned int flags)
>> +{
>> +    struct qemud_driver *driver = dom->conn->privateData;
>> +    qemuDomainObjPrivatePtr priv;
>> +    virDomainObjPtr vm;
>> +    int ret = -1;
>> +
>> +    virCheckFlags(0, -1);
>> +
>> +    if (duration) {
>> +        qemuReportError(VIR_ERR_INVALID_ARG, "%s",
> 
> VIR_ERR_ARGUMENT_UNSUPPORTED - the call was valid per the documentation,
> but we don't support it.
> 
>> +                        _("Duration not supported. Use 0 for now"));
>> +        return -1;
>> +    }
>> +
>> +    if (!(target == VIR_NODE_SUSPEND_TARGET_MEM ||
>> +          target == VIR_NODE_SUSPEND_TARGET_DISK ||
>> +          target == VIR_NODE_SUSPEND_TARGET_HYBRID)) {
>> +        qemuReportError(VIR_ERR_INVALID_ARG,
> 
> This one's fine - the call really is invalid per the documentation.
> 
>> +
>> +    if (priv->agentError) {
>> +        qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                        _("QEMU guest agent is not available due to an error"));
>> +        goto cleanup;
>> +    }
> 
> Meta-question - is it right to have a set-once priv->agentError flag?
> Or, since the agent can come and go at will, an error encountered
> previously might be overcome if this time around we send a new sync
> flag.  Thus, would it be better to have a dynamic function call, where
> the task of re-synchronizing to the guest is done at that point, and
> which either returns 0 if the guest is currently in sync or -1 if the
> resync failed for any reason?  But this would affect all uses of the
> guest agent, so for now, keeping this call consistent with other callers
> is okay.

Right. However, we are still pending a patch for issuing 'guest-sync'
command which can solve this issue for us. I mean, then we can drop
whole priv->agentError;

> 
>> +
>> +    if (!priv->agent) {
>> +        qemuReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
>> +                        _("QEMU guest agent is not configured"));
>> +        goto cleanup;
>> +    }
> 
> In fact, if you have a function call, then you could inline the
> priv->agent check into that function, for less redundancy in all callers
> that need an active agent.

Agree, but then again - a separate patch.
> 
>> +
>> +    if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
>> +        goto cleanup;
> 
> Hmm - if we do have a helper function to resync the agent, it would have
> to be done inside the job condition.
> 
>> @@ -1009,19 +1011,16 @@ int qemuMonitorSetCapabilities(qemuMonitorPtr mon)
>>  
>>      if (mon->json) {
>>          ret = qemuMonitorJSONSetCapabilities(mon);
>> -        if (ret == 0) {
>> -            int hmp = qemuMonitorJSONCheckHMP(mon);
>> -            if (hmp < 0) {
>> -                /* qemu may quited unexpectedly when we call
>> -                 * qemuMonitorJSONCheckHMP() */
>> -                ret = -1;
>> -            } else {
>> -                mon->json_hmp = hmp > 0;
>> -            }
>> -        }
>> +        if (ret)
>> +            goto cleanup;
>> +
>> +        ret = qemuMonitorJSONCheckCommands(mon, qemuCaps, &json_hmp);
>> +        mon->json_hmp = json_hmp > 0;
> 
> Looks nice.
> 

Okay, will send another version. Thanks.