Re: [Qemu-devel] [PATCH v2] qemu-ga: Add the guest-suspend command

[Michael Roth <mdroth@linux.vnet.ibm.com>]

On 12/14/2011 02:56 PM, Michael Roth wrote:
> On 12/14/2011 02:06 PM, Luiz Capitulino wrote:
>> On Wed, 14 Dec 2011 13:43:23 -0600
>> Michael Roth<mdroth@linux.vnet.ibm.com> wrote:
>>
>>> On 12/14/2011 12:17 PM, Luiz Capitulino wrote:
>>>> On Wed, 14 Dec 2011 14:38:55 -0200
>>>> Luiz Capitulino<lcapitulino@redhat.com> wrote:
>>>>
>>>>>>> I'm also wondering if we could use g_child_watch_add(), but it's
>>>>>>> not clear
>>>>>>> to me if it works with processes not created with g_spawn_*()
>>>>>>> functions.
>>>>>>
>>>>>> GPid's map to something other than PIDs on Windows, so I think
>>>>>> we'd have
>>>>>> issues there. But our fork() approach probably wouldn't work at
>>>>>> all on
>>>>>> Windows except maybe under cygwin, so at some point we'd probably
>>>>>> want
>>>>>> to switch over to g_spawn for this kind of stuff anyway...
>>>>>>
>>>>>> So this might be a good point to switch over to using the glib
>>>>>> functions.
>>>>>>
>>>>>> Would you mind trying to do the hibernate/zombie reaping stuff using
>>>>>> g_spawn+g_child_watch_add()? It might end up being the easiest route.
>>>>>> Otherwise I can take a look at it later today.
>>>>>
>>>>> Well, there are two problems with g_spawn wrt to the manual method of
>>>>> writing to the sysfs file. The first one is that I'm not sure if
>>>>> g_spawn()
>>>>> reports the file not found error synchronously. The other problem
>>>>> is that,
>>>>> I'd have to fork() anyway to write to the sysfs file (unless we
>>>>> decide that
>>>>> it's ok to do this synchronously, which seems ok to me).
>>>>
>>>> The version below uses g_spawn_async(). The code is a bit simpler
>>>> than previous
>>>> versions, but there are two important details about it:
>>>>
>>>> 1. I'm letting g_spawn_async() reap the child automatically. I don't
>>>> know
>>>> how it does it though. I'd guess it uses g_child_watch_add(), worst
>>>> case it ignores SIGCHLD (although I think this would be awful)
>>>
>>> Weird, doesn't seem to add a watcher or do anything with SIGCHLD. You're
>>> not seeing an error that's causing it to reap it immediately after
>>> the fork?
>>
>> No. I haven't tested it much though, but the basic use case seemed to
>> work
>> fine.
>>
>>>>
>>>> 2. The manual method of writing to the sysfs is done synchronously.
>>>> This
>>>> means that the command response will only be sent when the guest
>>>> resumes
>>>
>>> Want to avoid sync blocking calls as much as possible until timeouts are
>>> sorted out, particularly in this case. Currently, with hibernate at
>>> least, QEMU will exit (or is something being done to change that
>>> behavior?), libvirt will restart the VM later via some other interface,
>>> establish a connection to a new monitor and new agent channel, then gets
>>> a spurious response from the agent for a command it issue to a different
>>> QEMU process far in the past. So the common scenario would need nasty,
>>> special handling in libvirt and they'd probably hate us for it.
>>
>> Problem is: even with the other approach there's no way to guarantee that
>> the response will arrive before the guest suspends or shuts down, as it
>> depends on which process runs first.
>
>> If we want to guarantee that the client will see a response before the
>> guest suspends, then we need some form of synchronization. Like, the
>> child
>> should wait for the parent to send the response to the client before
>> exec()ing (or writing to the sysfs file).
>>
>
> There's no way to guarantee it though, even if we remove the window of

Rather, there's no way to guarantee the client will get a response (or 
that, given a response, the async command will succeed).

But you're that we could make it so that time-out/lack of response at 
least implies that the command *won't* be carried out...

That might be a nice guarantee, but your call on whether you want to do 
the signalling approach for hibernate. Otherwise, I think async will 
work well enough in the common case, and response timeout would imply 
neither success nor failure of the request, which I think is the norm 
for most things.

> opportunity for a child to complete a shutdown/suspend/etc request
> before we write out a response, there's still the potential scenario
> where an admin kills off/restarts qemu-ga while it's processing a request.
>
> So relying on client-side timeouts to handle these corner cases is
> acceptable, since being able to handle that is a requirement of a robust
> client, especially since we're talking to a potentially malicious
> guest/agent. We just need to do our best to make these situations corner
> cases rather than common ones.
>
>>>
>>> So probably best to stick with your previous approach for now, and look
>>> for a glib solution later.
>>>
>>>>
>>>> If you think this approach is acceptable, I'll test it more, update
>>>> its doc,
>>>> etc and post it again.
>>>>
>>>> diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
>>>> index 5f8a18d..63f65a6 100644
>>>> --- a/qapi-schema-guest.json
>>>> +++ b/qapi-schema-guest.json
>>>> @@ -219,3 +219,20 @@
>>>> ##
>>>> { 'command': 'guest-fsfreeze-thaw',
>>>> 'returns': 'int' }
>>>> +
>>>> +##
>>>> +# @guest-suspend
>>>> +#
>>>> +# Suspend guest execution by entering ACPI power state S3 or S4.
>>>> +#
>>>> +# @mode: 'hibernate' RAM content is saved in the disk and the guest is
>>>> +# powered down (this corresponds to ACPI S4)
>>>> +# 'sleep' execution is suspended but the RAM retains its contents
>>>> +# (this corresponds to ACPI S3)
>>>> +#
>>>> +# Notes: This is an asynchronous request. There's no guarantee it will
>>>> +# succeed. Errors will be logged to guest's syslog.
>>>> +#
>>>> +# Since: 1.1
>>>> +##
>>>> +{ 'command': 'guest-suspend', 'data': { 'mode': 'str' } }
>>>> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
>>>> index a09c8ca..0c6b78e 100644
>>>> --- a/qga/guest-agent-commands.c
>>>> +++ b/qga/guest-agent-commands.c
>>>> @@ -574,6 +574,56 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
>>>> }
>>>> #endif
>>>>
>>>> +#define LINUX_SYS_STATE_FILE "/sys/power/state"
>>>> +
>>>> +void qmp_guest_suspend(const char *mode, Error **err)
>>>> +{
>>>> + GError *error = NULL;
>>>> + gchar *argv[2];
>>>> +
>>>> + if (strcmp(mode, "hibernate") == 0) {
>>>> + argv[0] = (gchar *) "pm-hibernate";
>>>> + } else if (strcmp(mode, "sleep") == 0) {
>>>> + argv[0] = (gchar *) "pm-suspend";
>>>> + } else if (strcmp(mode, "hybrid") == 0) {
>>>> + argv[0] = (gchar *) "pm-hybrid";
>>>> + } else {
>>>> + error_set(err, QERR_INVALID_PARAMETER, "mode");
>>>> + return;
>>>> + }
>>>> +
>>>> + argv[1] = NULL;
>>>> + if (g_spawn_async(NULL, argv, NULL,G_SPAWN_SEARCH_PATH |
>>>> + G_SPAWN_FILE_AND_ARGV_ZERO,
>>>> + NULL, NULL, NULL,&error)< 0) {
>>>> + int fd;
>>>> + const char *cmd;
>>>> +
>>>> + slog("%s\n", error->message);
>>>> + g_error_free(error);
>>>> +
>>>> + if (strcmp(mode, "hybrid") == 0) {
>>>> + error_set(err, QERR_UNDEFINED_ERROR);
>>>> + return;
>>>> + }
>>>> +
>>>> + slog("trying to suspend using the manual method...\n");
>>>> +
>>>> + fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
>>>> + if (fd< 0) {
>>>> + error_set(err, QERR_OPEN_FILE_FAILED, LINUX_SYS_STATE_FILE);
>>>> + return;
>>>> + }
>>>> +
>>>> + cmd = strcmp(mode, "sleep") == 0 ? "mem" : "disk";
>>>> + if (write(fd, cmd, strlen(cmd))< 0) {
>>>> + error_set(err, QERR_IO_ERROR);
>>>> + }
>>>> +
>>>> + close(fd);
>>>> + }
>>>> +}
>>>> +
>>>> /* register init/cleanup routines for stateful command groups */
>>>> void ga_command_state_init(GAState *s, GACommandState *cs)
>>>> {
>>>
>>
>