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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list