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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list