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

[Daniel P. Berrange]

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.

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.

> +
> +        /* 
> +         * 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.

The impl of slog() doesn't look too safe to me either.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|