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

[Luiz Capitulino <lcapitulino@redhat.com>]

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