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