10:44 a.m.
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.