[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/2] qemu-ga: Add the guest-suspend command
From: |
Luiz Capitulino |
Subject: |
Re: [Qemu-devel] [PATCH 2/2] qemu-ga: Add the guest-suspend command |
Date: |
Tue, 17 Jan 2012 10:18:34 -0200 |
On Mon, 16 Jan 2012 21:06:27 +0000
"Daniel P. Berrange" <address@hidden> 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