On Thu, 5 Jan 2012 12:46:56 +0000
"Daniel P. Berrange" <berra...@redhat.com> 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
