3:25 p.m.
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org