[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2] qemu: Implement DomainPMSuspendForDuration



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.


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

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

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]