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.