From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:42474) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rar6P-0002jt-Ot for qemu-devel@nongnu.org; Wed, 14 Dec 2011 10:50:46 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Rar6K-0006dS-0t for qemu-devel@nongnu.org; Wed, 14 Dec 2011 10:50:41 -0500 Received: from e33.co.us.ibm.com ([32.97.110.151]:57470) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rar6J-0006d0-O5 for qemu-devel@nongnu.org; Wed, 14 Dec 2011 10:50:35 -0500 Received: from /spool/local by e33.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 14 Dec 2011 08:50:29 -0700 Received: from d03av04.boulder.ibm.com (d03av04.boulder.ibm.com [9.17.195.170]) by d03relay03.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id pBEFoGf5089634 for ; Wed, 14 Dec 2011 08:50:16 -0700 Received: from d03av04.boulder.ibm.com (loopback [127.0.0.1]) by d03av04.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id pBEFoECv018553 for ; Wed, 14 Dec 2011 08:50:14 -0700 Message-ID: <4EE8C5AC.8010505@linux.vnet.ibm.com> Date: Wed, 14 Dec 2011 09:50:04 -0600 From: Michael Roth MIME-Version: 1.0 References: <20111213162850.4cd135a3@doriath> <4EE7B54C.2050607@linux.vnet.ibm.com> <20111214110728.6b7bcf1e@doriath> In-Reply-To: <20111214110728.6b7bcf1e@doriath> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] qemu-ga: Add the guest-suspend command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: Amit Shah , aliguori@us.ibm.com, qemu-devel On 12/14/2011 07:07 AM, Luiz Capitulino wrote: > On Tue, 13 Dec 2011 14:27:56 -0600 > Michael Roth wrote: > >> On 12/13/2011 12:28 PM, Luiz Capitulino wrote: >>> It supports two modes: "hibernate" (which corresponds to S4) and >>> "sleep" (which corresponds to S3). It will try to execute the >>> pm-hibernate or pm-suspend scripts, if the scripts don't exist >>> the command will try to suspend by directly writing to the >>> "/sys/power/state" file. >>> >>> An interesting implementation detail is how to cleanup the child's >>> status on termination, so that we don't create zombies. I've >>> choosen to ignore the SIGCHLD signal. This will cause the kernel to >>> automatically cleanup the child's status on its termination. >>> >>> Signed-off-by: Luiz Capitulino >>> --- >>> >>> I've tested this w/o any virtio driver, as they don't support S4 yet. For >>> S4 it seems to work ok. I couldn't fully test S3 because we lack a way to >>> resume from it, but by checking the logs it seems to work fine. >>> >>> changelog >>> --------- >>> >>> v2 >>> >>> o Rename the command to 'guest-suspend' >>> o Add 'mode' parameter >>> o Use pm-utils scripts >>> o Cleanup child termination status >>> >>> qapi-schema-guest.json | 17 +++++++++++ >>> qemu-ga.c | 11 +++++++- >>> qga/guest-agent-commands.c | 64 ++++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 91 insertions(+), 1 deletions(-) >>> >>> diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json >>> index 29989fe..656bde9 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) >> >> 'suspend' is generally associated with sleep, so maybe we should make >> the mode optional and default to 'sleep'? > > I'm not sure I like having defaults because we're choosing for the client. > If this were a human interface then it would make sense, but for a machine > one I don't think it does. Besides we don't seem to have a way to resume > from S3 so we're probably going to get reports about this command not working > (and that reminds me to add a note about that in the doc). True, defaulting to broken behavior probably isn't a good idea :) > >> >>> +# >>> +# 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/qemu-ga.c b/qemu-ga.c >>> index 60d4972..b32e96c 100644 >>> --- a/qemu-ga.c >>> +++ b/qemu-ga.c >>> @@ -61,7 +61,7 @@ static void quit_handler(int sig) >>> >>> static void register_signal_handlers(void) >>> { >>> - struct sigaction sigact; >>> + struct sigaction sigact, sigact_chld; >>> int ret; >>> >>> memset(&sigact, 0, sizeof(struct sigaction)); >>> @@ -76,6 +76,15 @@ static void register_signal_handlers(void) >>> if (ret == -1) { >>> g_error("error configuring signal handler: %s", strerror(errno)); >>> } >>> + >>> + /* This should cause the kernel to automatically cleanup child >>> + termination status */ >>> + memset(&sigact_chld, 0, sizeof(struct sigaction)); >>> + sigact_chld.sa_handler = SIG_IGN; >>> + ret = sigaction(SIGCHLD,&sigact_chld, NULL); >>> + if (ret == -1) { >>> + g_error("error configuring signal handler: %s", strerror(errno)); >>> + } >>> } >>> >>> static void usage(const char *cmd) >>> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c >>> index a09c8ca..4799638 100644 >>> --- a/qga/guest-agent-commands.c >>> +++ b/qga/guest-agent-commands.c >>> @@ -574,6 +574,70 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err) >>> } >>> #endif >>> >>> +#define LINUX_PM_UTILS_PATH "/usr/sbin" >>> +#define LINUX_SYS_STATE_FILE "/sys/power/state" >>> + >>> +void qmp_guest_suspend(const char *mode, Error **err) >>> +{ >>> + int ret, fd = -1; >>> + const char *pmutils_bin; >>> + char pmutils_bin_path[PATH_MAX]; >>> + >>> + if (strcmp(mode, "hibernate") == 0) { >>> + pmutils_bin = "pm-hibernate"; >>> + } else if (strcmp(mode, "sleep") == 0) { >>> + pmutils_bin = "pm-suspend"; >>> + } else { >>> + error_set(err, QERR_INVALID_PARAMETER, "mode"); >>> + return; >>> + } >>> + >>> + snprintf(pmutils_bin_path, sizeof(pmutils_bin_path), "%s/%s", >>> + LINUX_PM_UTILS_PATH, pmutils_bin); >> >> I'd be surprised to find any distros where this isn't the case, but for >> situations where the scripts aren't in /usr/sbin, maybe we'd be better >> off just passing the command name to execlp() and letting it do the >> search via PATH? The normal use case is that qemu-ga will get launched >> as a service, so PATH should have the good stuff. > > Ok, I'll make that change. > >> >>> + >>> + if (access(pmutils_bin_path, X_OK) != 0) { >>> + pmutils_bin = NULL; >>> + fd = open(LINUX_SYS_STATE_FILE, O_WRONLY); >>> + if (fd< 0) { >>> + error_set(err, QERR_OPEN_FILE_FAILED, LINUX_SYS_STATE_FILE); >>> + return; >>> + } >>> + } >>> + >>> + ret = fork(); >>> + if (ret == 0) { >>> + /* child */ >>> + setsid(); >>> + fclose(stdin); >>> + fclose(stdout); >>> + fclose(stderr); >>> + >>> + if (pmutils_bin) { >>> + ret = execl(pmutils_bin_path, pmutils_bin, NULL); >>> + if (ret) { >>> + slog("%s failed: %s", pmutils_bin_path, strerror(errno)); >>> + } >>> + } else { >>> + const char *cmd = strcmp(mode, "sleep") == 0 ? "mem" : "disk"; >>> + ret = write(fd, cmd, strlen(cmd)); >>> + if (ret< 0) { >>> + slog("can't write to %s: %s\n", LINUX_SYS_STATE_FILE, >>> + strerror(errno)); >>> + } >>> + close(fd); >>> + } >>> + >>> + exit(!!ret); >>> + } else if (ret< 0) { >>> + error_set(err, QERR_UNDEFINED_ERROR); >>> + return; >>> + } >>> + >>> + if (!pmutils_bin) { >>> + close(fd); >>> + } >>> +} >>> + >>> /* register init/cleanup routines for stateful command groups */ >>> void ga_command_state_init(GAState *s, GACommandState *cs) >>> { >> >