From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:33116) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rimtx-0000wT-TD for qemu-devel@nongnu.org; Thu, 05 Jan 2012 07:58:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Rimtw-0002qS-KJ for qemu-devel@nongnu.org; Thu, 05 Jan 2012 07:58:37 -0500 Received: from mx1.redhat.com ([209.132.183.28]:1292) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rimtw-0002oZ-6h for qemu-devel@nongnu.org; Thu, 05 Jan 2012 07:58:36 -0500 Date: Thu, 5 Jan 2012 10:58:29 -0200 From: Luiz Capitulino Message-ID: <20120105105829.5340a377@doriath> In-Reply-To: <20120105124656.GK31797@redhat.com> References: <1325706313-21936-1-git-send-email-lcapitulino@redhat.com> <1325706313-21936-3-git-send-email-lcapitulino@redhat.com> <20120105124656.GK31797@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] qemu-ga: Add the guest-suspend command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" Cc: amit.shah@redhat.com, jcody@redhat.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com On Thu, 5 Jan 2012 12:46:56 +0000 "Daniel P. Berrange" wrote: > On Wed, Jan 04, 2012 at 05:45:13PM -0200, Luiz Capitulino wrote: > > diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c > > index a09c8ca..19f29c6 100644 > > --- a/qga/guest-agent-commands.c > > +++ b/qga/guest-agent-commands.c > > @@ -574,6 +574,61 @@ 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) > > +{ > > + pid_t pid; > > + const char *pmutils_bin; > > + > > + /* TODO implement 'sleep' and 'hybrid' modes once qemu is fixed to > > + support them */ > > + if (strcmp(mode, "hibernate") == 0) { > > + pmutils_bin = "pm-hibernate"; > > + } else { > > + error_set(err, QERR_INVALID_PARAMETER, "mode"); > > + return; > > + } > > + > > + pid = fork(); > > + if (pid == 0) { > > + /* child */ > > + int fd; > > + > > + setsid(); > > + fclose(stdin); > > + fclose(stdout); > > + fclose(stderr); > > + > > + execlp(pmutils_bin, pmutils_bin, NULL); > > Strictly speaking, in multi-threaded programs, between fork() and > exec() you are restricted to calling functions which are marked as > async-signal safe in POSIX spec - fclose() is not. > > Also, if there was unflushed buffered output on stdout, calling > fclose() in the child will flush that output, but then the parent > process will also flush it some time later, causing duplicated > stdout data. > > NB, you might not think qemu-ga is multi-threaded, but depending on > which GLib APIs you're calling, you might find you are in fact using > threads behind the scenes without realizing, so I think it is wise > to be conservative here & assume threads are possible. All good points. > Thus you really want to use a plain old 'close()' call, and then > re-open to /dev/null as Eric suggests, leaving stdin/out/err FILE* > alone. I'm going to use dup2(), which seems to be ok in that regard. > > > + > > + /* > > + * The exec call should not return, if it does something went wrong. > > + * In this case we try to suspend manually if 'mode' is 'hibernate' > > + */ > > + slog("could not execute %s: %s\n", pmutils_bin, strerror(errno)); > > + slog("trying to suspend using the manual method...\n"); > > + > > + fd = open(LINUX_SYS_STATE_FILE, O_WRONLY); > > + if (fd < 0) { > > + slog("can't open file %s: %s\n", LINUX_SYS_STATE_FILE, > > + strerror(errno)); > > + exit(1); > > + } > > + > > + if (write(fd, "disk", 4) < 0) { > > + slog("can't write to %s: %s\n", LINUX_SYS_STATE_FILE, > > + strerror(errno)); > > + exit(1); > > + } > > + > > + exit(0); > > exit() is also not async-signal safe, because it calls into stdio > to flush buffers. So you want to use _exit() instead for these. Ok, I'll change and will fix qmp_guest_shutdown() in a different patch. > > The impl of slog() doesn't look too safe to me either. > > Regards, > Daniel