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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Mon, 16 Jan 2012 21:06:27 +0000
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> On Mon, Jan 16, 2012 at 06:09:52PM -0200, Luiz Capitulino wrote:
> 
> > +/* Try to find executable file 'file'. If it's found, its absolute path is
> > +   returned in 'abs_path' and the function returns true. If it's not found,
> > +   the function will return false and 'abs_path' will contain zeros */
> > +static bool find_executable_file(const char *file, char *abs_path, size_t len)
> > +{
> > +    char *path, *saveptr;
> > +    const char *token, *delim = ":";
> > + 
> > +    if (!getenv("PATH")) {
> > +        return false;
> > +    }
> > +
> > +    path = g_strdup(getenv("PATH"));
> > +    if (!path) {
> > +        return false;
> > +    }
> > +
> > +    for (token = strtok_r(path, delim, &saveptr); token;
> > +         token = strtok_r(NULL, delim, &saveptr)) {
> > +        snprintf(abs_path, len, "%s/%s", token, file);
> > +        if (access(abs_path, X_OK) == 0) {
> > +            g_free(path);
> > +            return true;
> > +        }
> > +    }
> > +
> > +    g_free(path);
> > +    memset(abs_path, 0, len);
> > +
> > +    return false;
> > +}
> 
> I hope you don't hate me for now pointing out...
> 
>    g_find_program_in_path()
> 
> http://developer.gnome.org/glib/stable/glib-Miscellaneous-Utility-Functions.html#g-find-program-in-path

Oh, Thanks.

Note that it returns an allocated string. I won't free it in the child,
as it will be automatically freed by exec() or by exit().

> > +
> >  int64_t qmp_guest_sync(int64_t id, Error **errp)
> >  {
> >      return id;
> > @@ -574,6 +623,220 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
> >  }
> >  #endif
> >  
> > +#define LINUX_SYS_STATE_FILE "/sys/power/state"
> > +#define SUS_MODE_SUPPORTED 0
> > +#define SUS_MODE_NOT_SUPPORTED 1
> > +
> > +/**
> > + * This function forks twice and the information about the mode support
> > + * status is passed to the qemu-ga process via a pipe.
> > + *
> > + * This approach allows us to keep the way we reap terminated children
> > + * in qemu-ga quite simple.
> > + */
> > +static bool bios_supports_mode(const char *mode, Error **err)
> > +{
> > +    pid_t pid;
> > +    ssize_t ret;
> > +    bool has_pmutils;
> > +    int status, pipefds[2];
> > +    char pmutils_path[PATH_MAX];
> > +    const char *pmutils_bin = "pm-is-supported";
> > +
> > +    if (pipe(pipefds) < 0) {
> > +        error_set(err, QERR_UNDEFINED_ERROR);
> > +        return false;
> > +    }
> > +
> > +    has_pmutils = find_executable_file(pmutils_bin, pmutils_path,
> > +                                       sizeof(pmutils_path));
> > +
> > +    pid = fork();
> > +    if (!pid) {
> > +        struct sigaction act;
> > +
> > +        memset(&act, 0, sizeof(act));
> > +        act.sa_handler = SIG_DFL;
> > +        sigaction(SIGCHLD, &act, NULL);
> > +
> > +        setsid();
> > +        close(pipefds[0]);
> > +        reopen_fd_to_null(0);
> > +        reopen_fd_to_null(1);
> > +        reopen_fd_to_null(2);
> > +
> > +        pid = fork();
> > +        if (!pid) {
> > +            int fd;
> > +            char buf[32]; /* hopefully big enough */
> > +            const char *arg;
> > +
> > +            if (strcmp(mode, "hibernate") == 0) {
> > +                arg = "--hibernate";
> > +            } else if (strcmp(mode, "sleep") == 0) {
> > +                arg = "--suspend";
> > +            } else if (strcmp(mode, "hybrid") == 0) {
> > +                arg = "--suspend-hybrid";
> > +            } else {
> > +                _exit(SUS_MODE_NOT_SUPPORTED);
> > +            }
> > +
> > +            if (has_pmutils) {
> > +                execle(pmutils_path, pmutils_bin, arg, NULL, environ);
> > +            }
> > +
> > +            /*
> > +             * If we get here either pm-utils is not installed or  execle() has
> > +             * failed. Let's try the manual approach if mode is not hybrid (as
> > +             * it's only supported by pm-utils)
> > +             */
> > +
> > +            if (strcmp(mode, "hybrid") == 0) {
> > +                _exit(SUS_MODE_NOT_SUPPORTED);
> > +            }
> > +
> > +            fd = open(LINUX_SYS_STATE_FILE, O_RDONLY);
> > +            if (fd < 0) {
> > +                _exit(SUS_MODE_NOT_SUPPORTED);
> > +            }
> > +
> > +            ret = read(fd, buf, sizeof(buf));
> 
> You want "sizeof(buf)-1" here, otherwise if read return 'ret'
> set to exactly sizeof(buf)...

Good catch.

> 
> > +            if (ret <= 0) {
> > +                _exit(SUS_MODE_NOT_SUPPORTED);
> > +            }
> > +
> > +            buf[ret] = '\0';
> 
> ...this becomes a stack overflow.
> 
> > +    has_pmutils = find_executable_file(pmutils_bin, pmutils_path,
> > +                                       sizeof(pmutils_path));
> > +
> > +    pid = fork();
> > +    if (pid == 0) {
> > +        /* child */
> > +        int fd;
> > +        const char *cmd;
> > +
> > +        setsid();
> > +        reopen_fd_to_null(0);
> > +        reopen_fd_to_null(1);
> > +        reopen_fd_to_null(2);
> > +
> > +        if (has_pmutils) {
> > +            execle(pmutils_path, pmutils_bin, NULL, environ);
> 
> You could just use  execl()  and drop the trailing 'environ' here,
> since that is the default anyway.

execl() is not in the async-signal-safe list.

> 
> > +        }
> > +
> > +        /*
> > +         * If we get here either pm-utils is not installed or  execle() has
> > +         * failed. Let's try the manual approach if mode is not hybrid (as
> > +         * it's only supported by pm-utils)
> > +         */
> > +
> > +        slog("could not execute %s\n", pmutils_bin);
> > +        if (strcmp(mode, "hybrid") == 0) {
> > +            _exit(EXIT_FAILURE);
> > +        }
> > +
> > +        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(EXIT_FAILURE);
> > +        }
> > +
> > +        cmd = strcmp(mode, "sleep") == 0 ? "mem" : "disk";
> > +        if (write(fd, cmd, strlen(cmd)) < 0) {
> > +            slog("failued to write to %s\n", LINUX_SYS_STATE_FILE);
> > +            _exit(EXIT_FAILURE);
> > +        }
> > +
> > +        _exit(EXIT_SUCCESS);
> > +    } else if (pid < 0) {
> > +        error_set(err, QERR_UNDEFINED_ERROR);
> > +        return;
> > +    }
> > +}
> > +
> >  /* register init/cleanup routines for stateful command groups */
> >  void ga_command_state_init(GAState *s, GACommandState *cs)
> >  {
> 
> 
> Regards,
> Daniel