From d34b867d816585900b72d09d42a34cea3057903d Mon Sep 17 00:00:00 2001 From: Luiz Capitulino Date: Tue, 8 May 2012 14:24:44 -0300 Subject: qapi: add support for command options Options allow for changes in commands behavior. This commit introduces the QCO_NO_SUCCESS_RESP option, which causes a command to not emit a success response. This is needed by commands such as qemu-ga's guest-shutdown, which may not be able to complete before the VM vanishes. In this case, it's useful and simpler not to bother sending a success response. Signed-off-by: Luiz Capitulino Signed-off-by: Michael Roth --- qapi/qmp-core.h | 10 +++++++++- qapi/qmp-dispatch.c | 8 ++++++-- qapi/qmp-registry.c | 4 +++- scripts/qapi-commands.py | 14 ++++++++++++-- 4 files changed, 30 insertions(+), 6 deletions(-) diff --git a/qapi/qmp-core.h b/qapi/qmp-core.h index 431ddbb337..b0f64ba1ee 100644 --- a/qapi/qmp-core.h +++ b/qapi/qmp-core.h @@ -25,16 +25,24 @@ typedef enum QmpCommandType QCT_NORMAL, } QmpCommandType; +typedef enum QmpCommandOptions +{ + QCO_NO_OPTIONS = 0x0, + QCO_NO_SUCCESS_RESP = 0x1, +} QmpCommandOptions; + typedef struct QmpCommand { const char *name; QmpCommandType type; QmpCommandFunc *fn; + QmpCommandOptions options; QTAILQ_ENTRY(QmpCommand) node; bool enabled; } QmpCommand; -void qmp_register_command(const char *name, QmpCommandFunc *fn); +void qmp_register_command(const char *name, QmpCommandFunc *fn, + QmpCommandOptions options); QmpCommand *qmp_find_command(const char *name); QObject *qmp_dispatch(QObject *request); void qmp_disable_command(const char *name); diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c index 43f640a95e..122c1a29ba 100644 --- a/qapi/qmp-dispatch.c +++ b/qapi/qmp-dispatch.c @@ -94,8 +94,12 @@ static QObject *do_qmp_dispatch(QObject *request, Error **errp) switch (cmd->type) { case QCT_NORMAL: cmd->fn(args, &ret, errp); - if (!error_is_set(errp) && ret == NULL) { - ret = QOBJECT(qdict_new()); + if (!error_is_set(errp)) { + if (cmd->options & QCO_NO_SUCCESS_RESP) { + g_assert(!ret); + } else if (!ret) { + ret = QOBJECT(qdict_new()); + } } break; } diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c index 43d5cdeb64..5414613377 100644 --- a/qapi/qmp-registry.c +++ b/qapi/qmp-registry.c @@ -17,7 +17,8 @@ static QTAILQ_HEAD(QmpCommandList, QmpCommand) qmp_commands = QTAILQ_HEAD_INITIALIZER(qmp_commands); -void qmp_register_command(const char *name, QmpCommandFunc *fn) +void qmp_register_command(const char *name, QmpCommandFunc *fn, + QmpCommandOptions options) { QmpCommand *cmd = g_malloc0(sizeof(*cmd)); @@ -25,6 +26,7 @@ void qmp_register_command(const char *name, QmpCommandFunc *fn) cmd->type = QCT_NORMAL; cmd->fn = fn; cmd->enabled = true; + cmd->options = options; QTAILQ_INSERT_TAIL(&qmp_commands, cmd, node); } diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py index 0b4f0a0fe1..9eed40e18a 100644 --- a/scripts/qapi-commands.py +++ b/scripts/qapi-commands.py @@ -291,14 +291,24 @@ out: return ret +def option_value_matches(opt, val, cmd): + if opt in cmd and cmd[opt] == val: + return True + return False + def gen_registry(commands): registry="" push_indent() for cmd in commands: + options = 'QCO_NO_OPTIONS' + if option_value_matches('success-response', 'no', cmd): + options = 'QCO_NO_SUCCESS_RESP' + registry += mcgen(''' -qmp_register_command("%(name)s", qmp_marshal_input_%(c_name)s); +qmp_register_command("%(name)s", qmp_marshal_input_%(c_name)s, %(opts)s); ''', - name=cmd['command'], c_name=c_fun(cmd['command'])) + name=cmd['command'], c_name=c_fun(cmd['command']), + opts=options) pop_indent() ret = mcgen(''' static void qmp_init_marshal(void) -- cgit v1.2.3 From ce8c8b7bd8958fde291f7736016015864e7638a2 Mon Sep 17 00:00:00 2001 From: Luiz Capitulino Date: Tue, 8 May 2012 14:24:45 -0300 Subject: qemu-ga: don't warn on no command return This is a valid condition when a command chooses to not emit a success response. Signed-off-by: Luiz Capitulino Signed-off-by: Michael Roth --- qemu-ga.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/qemu-ga.c b/qemu-ga.c index 680997ebd3..cf61cb95e5 100644 --- a/qemu-ga.c +++ b/qemu-ga.c @@ -488,8 +488,6 @@ static void process_command(GAState *s, QDict *req) g_warning("error sending response: %s", strerror(ret)); } qobject_decref(rsp); - } else { - g_warning("error getting response"); } } -- cgit v1.2.3 From 8926817219d18403e04625afddd29e6ad25c3162 Mon Sep 17 00:00:00 2001 From: Luiz Capitulino Date: Tue, 8 May 2012 14:24:46 -0300 Subject: qemu-ga: guest-shutdown: don't emit a success response Today, qemu-ga may not be able to emit a success response when guest-shutdown completes. This happens because the VM may vanish before qemu-ga is able to emit a response. This semantic is a bit confusing, as it's not clear for clients if they should wait for a response or how they should check for success. This commit solves that problem by changing guest-shutdown to never emit a success response and suggests in the documentation what clients could do to check for success. Signed-off-by: Luiz Capitulino Signed-off-by: Michael Roth --- qapi-schema-guest.json | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json index 692b570675..1dd3454555 100644 --- a/qapi-schema-guest.json +++ b/qapi-schema-guest.json @@ -131,11 +131,15 @@ # # @mode: #optional "halt", "powerdown" (default), or "reboot" # -# Returns: Nothing on success +# This command does NOT return a response on success. Success condition +# is indicated by the VM exiting with a zero exit status or, when +# running with --no-shutdown, by issuing the query-status QMP command +# to confirm the VM status is "shutdown". # # Since: 0.15.0 ## -{ 'command': 'guest-shutdown', 'data': { '*mode': 'str' } } +{ 'command': 'guest-shutdown', 'data': { '*mode': 'str' }, + 'success-response': 'no' } ## # @guest-file-open: -- cgit v1.2.3 From c6fcc10ab31d22e93eb169c451025ac9636ec84b Mon Sep 17 00:00:00 2001 From: Luiz Capitulino Date: Tue, 8 May 2012 14:24:47 -0300 Subject: qemu-ga: guest-suspend-disk: don't emit a success response Today, qemu-ga may not be able to emit a success response when guest-suspend-disk completes. This happens because the VM may vanish before qemu-ga is able to emit a response. This semantic is a bit confusing, as it's not clear for clients if they should wait for a response or how they should check for success. This commit solves that problem by changing guest-suspend-disk to never emit a success response and suggests in the documentation what clients could do to check for success. Signed-off-by: Luiz Capitulino Signed-off-by: Michael Roth --- qapi-schema-guest.json | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json index 1dd3454555..88b165cac0 100644 --- a/qapi-schema-guest.json +++ b/qapi-schema-guest.json @@ -363,17 +363,21 @@ # For the best results it's strongly recommended to have the pm-utils # package installed in the guest. # -# Returns: nothing on success +# This command does NOT return a response on success. There is a high chance +# the command succeeded if the VM exits with a zero exit status or, when +# running with --no-shutdown, by issuing the query-status QMP command to +# to confirm the VM status is "shutdown". However, the VM could also exit +# (or set its status to "shutdown") due to other reasons. +# +# The following errors may be returned: # If suspend to disk is not supported, Unsupported # -# Notes: o This is an asynchronous request. There's no guarantee a response -# will be sent -# o It's strongly recommended to issue the guest-sync command before -# sending commands when the guest resumes +# Notes: It's strongly recommended to issue the guest-sync command before +# sending commands when the guest resumes # # Since: 1.1 ## -{ 'command': 'guest-suspend-disk' } +{ 'command': 'guest-suspend-disk', 'success-response': 'no' } ## # @guest-suspend-ram -- cgit v1.2.3 From 432d29db0db9d08fe34a57b8c03af20c9f759d77 Mon Sep 17 00:00:00 2001 From: Luiz Capitulino Date: Tue, 8 May 2012 14:24:48 -0300 Subject: qemu-ga: guest-suspend-ram: don't emit a success response Today, qemu-ga may not be able to emit a success response when guest-suspend-ram completes. This happens because the VM may suspend before qemu-ga is able to emit a response. This semantic is a bit confusing, as it's not clear for clients if they should wait for a response or how they should check for success. This commit solves that problem by changing guest-suspend-ram to never emit a success response and suggests in the documentation what clients should do to check for success. Signed-off-by: Luiz Capitulino Signed-off-by: Michael Roth --- qapi-schema-guest.json | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json index 88b165cac0..50e2815fb1 100644 --- a/qapi-schema-guest.json +++ b/qapi-schema-guest.json @@ -395,17 +395,21 @@ # command. Thus, it's *required* to query QEMU for the presence of the # 'system_wakeup' command before issuing guest-suspend-ram. # -# Returns: nothing on success +# This command does NOT return a response on success. There are two options +# to check for success: +# 1. Wait for the SUSPEND QMP event from QEMU +# 2. Issue the query-status QMP command to confirm the VM status is +# "suspended" +# +# The following errors may be returned: # If suspend to ram is not supported, Unsupported # -# Notes: o This is an asynchronous request. There's no guarantee a response -# will be sent -# o It's strongly recommended to issue the guest-sync command before -# sending commands when the guest resumes +# Notes: It's strongly recommended to issue the guest-sync command before +# sending commands when the guest resumes # # Since: 1.1 ## -{ 'command': 'guest-suspend-ram' } +{ 'command': 'guest-suspend-ram', 'success-response': 'no' } ## # @guest-suspend-hybrid -- cgit v1.2.3 From d9fcd2a1c825791cec9b21e634013b728422972f Mon Sep 17 00:00:00 2001 From: Luiz Capitulino Date: Tue, 8 May 2012 14:24:49 -0300 Subject: qemu-ga: guest-suspend-hybrid: don't emit a success response Today, qemu-ga may not be able to emit a success response when guest-suspend-hybrid completes. This happens because the VM may suspend before qemu-ga is able to emit a response. This semantic is a bit confusing, as it's not clear for clients if they should wait for a response or how they should check for success. This commit solves that problem by changing guest-suspend-hybrid to never emit a success response and suggests in the documentation what clients should do to check for success. Signed-off-by: Luiz Capitulino Signed-off-by: Michael Roth --- qapi-schema-guest.json | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json index 50e2815fb1..7c6cb21a79 100644 --- a/qapi-schema-guest.json +++ b/qapi-schema-guest.json @@ -422,17 +422,21 @@ # command. Thus, it's *required* to query QEMU for the presence of the # 'system_wakeup' command before issuing guest-suspend-hybrid. # -# Returns: nothing on success +# This command does NOT return a response on success. There are two options +# to check for success: +# 1. Wait for the SUSPEND QMP event from QEMU +# 2. Issue the query-status QMP command to confirm the VM status is +# "suspended" +# +# The following errors may be returned: # If hybrid suspend is not supported, Unsupported # -# Notes: o This is an asynchronous request. There's no guarantee a response -# will be sent -# o It's strongly recommended to issue the guest-sync command before -# sending commands when the guest resumes +# Notes: It's strongly recommended to issue the guest-sync command before +# sending commands when the guest resumes # # Since: 1.1 ## -{ 'command': 'guest-suspend-hybrid' } +{ 'command': 'guest-suspend-hybrid', 'success-response': 'no' } ## # @GuestIpAddressType: -- cgit v1.2.3 From 04b4e75f33ae0775d70b8e33080f46d66275cdcc Mon Sep 17 00:00:00 2001 From: Luiz Capitulino Date: Thu, 10 May 2012 16:50:41 -0300 Subject: qemu-ga: make reopen_fd_to_null() public The next commit wants to use it. Signed-off-by: Luiz Capitulino Reviewed-by: Eric Blake Signed-off-by: Michael Roth --- qemu-ga.c | 17 +++++++++++++++++ qga/commands-posix.c | 19 ------------------- qga/guest-agent-core.h | 4 ++++ 3 files changed, 21 insertions(+), 19 deletions(-) diff --git a/qemu-ga.c b/qemu-ga.c index cf61cb95e5..8d53e04a0f 100644 --- a/qemu-ga.c +++ b/qemu-ga.c @@ -140,6 +140,23 @@ static gboolean register_signal_handlers(void) return true; } + +/* TODO: use this in place of all post-fork() fclose(std*) callers */ +void reopen_fd_to_null(int fd) +{ + int nullfd; + + nullfd = open("/dev/null", O_RDWR); + if (nullfd < 0) { + return; + } + + dup2(nullfd, fd); + + if (nullfd != fd) { + close(nullfd); + } +} #endif static void usage(const char *cmd) diff --git a/qga/commands-posix.c b/qga/commands-posix.c index e448431c66..adb9b3db8d 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -34,25 +34,6 @@ #endif #endif -#if defined(__linux__) -/* TODO: use this in place of all post-fork() fclose(std*) callers */ -static void reopen_fd_to_null(int fd) -{ - int nullfd; - - nullfd = open("/dev/null", O_RDWR); - if (nullfd < 0) { - return; - } - - dup2(nullfd, fd); - - if (nullfd != fd) { - close(nullfd); - } -} -#endif /* defined(__linux__) */ - void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err) { int ret; diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h index bbb8b9b125..6dba10484d 100644 --- a/qga/guest-agent-core.h +++ b/qga/guest-agent-core.h @@ -35,3 +35,7 @@ void ga_set_response_delimited(GAState *s); bool ga_is_frozen(GAState *s); void ga_set_frozen(GAState *s); void ga_unset_frozen(GAState *s); + +#ifndef _WIN32 +void reopen_fd_to_null(int fd); +#endif -- cgit v1.2.3 From 226a48949cf74006a94b5931e50352b2af801ac7 Mon Sep 17 00:00:00 2001 From: Luiz Capitulino Date: Thu, 10 May 2012 16:50:42 -0300 Subject: qemu-ga: become_daemon(): reopen standard fds to /dev/null This fixes a bug where qemu-ga doesn't suspend the guest because it fails to detect suspend support even when the guest does support suspend. This happens because of the way qemu-ga fds are managed in daemon mode. When starting qemu-ga with --daemon, become_daemon() will close all standard fds. This will cause qemu-ga to end up with the following fds (if started with 'qemu-ga --daemon'): 0 -> /dev/vport0p1 3 -> /run/qemu-ga.pid Then a guest-suspend-* function is issued. They call bios_supports_mode(), which will call pipe(), and qemu-ga's fd will be: 0 -> /dev/vport0p1 1 -> pipe:[16247] 2 -> pipe:[16247] 3 -> /run/qemu-ga.pid bios_supports_mode() forks off a child and blocks waiting for the child to write something to the pipe. The child, however, closes its reading end of the pipe _and_ reopen all standard fds to /dev/null. This will cause the child's fds to be: 0 -> /dev/null 1 -> /dev/null 2 -> /dev/null 3 -> /run/qemu-ga.pid In other words, the child's writing end of the pipe is now /dev/null. It writes there and exits. The parent process (blocked on read()) will get an EOF and interpret this as "something unexpected happened in the child, let's assume the guest doesn't support suspend". And suspend will fail. To solve this problem we have to reopen standard fds to /dev/null in become_daemon(), instead of closing them. Signed-off-by: Luiz Capitulino Reviewed-by: Eric Blake Signed-off-by: Michael Roth --- qemu-ga.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/qemu-ga.c b/qemu-ga.c index 8d53e04a0f..6e7caedc41 100644 --- a/qemu-ga.c +++ b/qemu-ga.c @@ -445,9 +445,9 @@ static void become_daemon(const char *pidfile) goto fail; } - close(STDIN_FILENO); - close(STDOUT_FILENO); - close(STDERR_FILENO); + reopen_fd_to_null(STDIN_FILENO); + reopen_fd_to_null(STDOUT_FILENO); + reopen_fd_to_null(STDERR_FILENO); return; fail: -- cgit v1.2.3 From dc8764f06155a7b3e635e02281b747a9e292127e Mon Sep 17 00:00:00 2001 From: Luiz Capitulino Date: Fri, 11 May 2012 16:19:46 -0300 Subject: qemu-ga: guest-suspend: make the API synchronous Currently, qemu-ga has a SIGCHLD handler that automatically reaps terminated children processes. The idea is to avoid having qemu-ga commands blocked waiting for children to terminate. That approach has two problems: 1. qemu-ga is unable to detect errors in the child, meaning that qemu-ga returns success even if the child fails to perform its task 2. if a command does depend on the child exit status, the command has to play tricks to bypass the automatic reaper Case 2 impacts the guest-suspend-* API, because it has to execute an external program to check for suspend support. Today, to bypass the automatic reaper, suspend code has to double fork and pass exit status information through a pipe. Besides being complex, this is prone to race condition bugs. Indeed, the current code does have such bugs. Making the guest-suspend-* API synchronous (ie. by dropping the SIGCHLD handler and calling waitpid() from commands) is a much simpler approach, which fixes current race conditions bugs and enables commands to detect errors in the child. This commit does just that. There's a side effect though, guest-shutdown will generate zombies if shutting down fails. This will be fixed by the next commit. Signed-off-by: Luiz Capitulino Reviewed-by: Eric Blake Signed-off-by: Michael Roth --- qemu-ga.c | 17 +------ qga/commands-posix.c | 128 ++++++++++++++++++++++----------------------------- 2 files changed, 55 insertions(+), 90 deletions(-) diff --git a/qemu-ga.c b/qemu-ga.c index 6e7caedc41..3a88333a17 100644 --- a/qemu-ga.c +++ b/qemu-ga.c @@ -104,16 +104,9 @@ static void quit_handler(int sig) } #ifndef _WIN32 -/* reap _all_ terminated children */ -static void child_handler(int sig) -{ - int status; - while (waitpid(-1, &status, WNOHANG) > 0) /* NOTHING */; -} - static gboolean register_signal_handlers(void) { - struct sigaction sigact, sigact_chld; + struct sigaction sigact; int ret; memset(&sigact, 0, sizeof(struct sigaction)); @@ -130,14 +123,6 @@ static gboolean register_signal_handlers(void) return false; } - memset(&sigact_chld, 0, sizeof(struct sigaction)); - sigact_chld.sa_handler = child_handler; - sigact_chld.sa_flags = SA_NOCLDSTOP; - ret = sigaction(SIGCHLD, &sigact_chld, NULL); - if (ret == -1) { - g_error("error configuring signal handler: %s", strerror(errno)); - } - return true; } diff --git a/qga/commands-posix.c b/qga/commands-posix.c index adb9b3db8d..76c8235272 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -512,117 +512,88 @@ static void guest_fsfreeze_cleanup(void) #define SUSPEND_SUPPORTED 0 #define SUSPEND_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 void bios_supports_mode(const char *pmutils_bin, const char *pmutils_arg, const char *sysfile_str, Error **err) { - pid_t pid; - ssize_t ret; char *pmutils_path; - int status, pipefds[2]; - - if (pipe(pipefds) < 0) { - error_set(err, QERR_UNDEFINED_ERROR); - return; - } + pid_t pid, rpid; + int status; pmutils_path = g_find_program_in_path(pmutils_bin); pid = fork(); if (!pid) { - struct sigaction act; - - memset(&act, 0, sizeof(act)); - act.sa_handler = SIG_DFL; - sigaction(SIGCHLD, &act, NULL); + char buf[32]; /* hopefully big enough */ + ssize_t ret; + int fd; 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 */ - - if (pmutils_path) { - execle(pmutils_path, pmutils_bin, pmutils_arg, NULL, environ); - } - - /* - * If we get here either pm-utils is not installed or execle() has - * failed. Let's try the manual method if the caller wants it. - */ - - if (!sysfile_str) { - _exit(SUSPEND_NOT_SUPPORTED); - } - - fd = open(LINUX_SYS_STATE_FILE, O_RDONLY); - if (fd < 0) { - _exit(SUSPEND_NOT_SUPPORTED); - } + if (pmutils_path) { + execle(pmutils_path, pmutils_bin, pmutils_arg, NULL, environ); + } - ret = read(fd, buf, sizeof(buf)-1); - if (ret <= 0) { - _exit(SUSPEND_NOT_SUPPORTED); - } - buf[ret] = '\0'; + /* + * If we get here either pm-utils is not installed or execle() has + * failed. Let's try the manual method if the caller wants it. + */ - if (strstr(buf, sysfile_str)) { - _exit(SUSPEND_SUPPORTED); - } + if (!sysfile_str) { + _exit(SUSPEND_NOT_SUPPORTED); + } + fd = open(LINUX_SYS_STATE_FILE, O_RDONLY); + if (fd < 0) { _exit(SUSPEND_NOT_SUPPORTED); } - if (pid > 0) { - wait(&status); - } else { - status = SUSPEND_NOT_SUPPORTED; + ret = read(fd, buf, sizeof(buf)-1); + if (ret <= 0) { + _exit(SUSPEND_NOT_SUPPORTED); } + buf[ret] = '\0'; - ret = write(pipefds[1], &status, sizeof(status)); - if (ret != sizeof(status)) { - _exit(EXIT_FAILURE); + if (strstr(buf, sysfile_str)) { + _exit(SUSPEND_SUPPORTED); } - _exit(EXIT_SUCCESS); + _exit(SUSPEND_NOT_SUPPORTED); } - close(pipefds[1]); g_free(pmutils_path); if (pid < 0) { - error_set(err, QERR_UNDEFINED_ERROR); - goto out; - } - - ret = read(pipefds[0], &status, sizeof(status)); - if (ret == sizeof(status) && WIFEXITED(status) && - WEXITSTATUS(status) == SUSPEND_SUPPORTED) { - goto out; + goto undef_err; + } + + do { + rpid = waitpid(pid, &status, 0); + } while (rpid == -1 && errno == EINTR); + if (rpid == pid && WIFEXITED(status)) { + switch (WEXITSTATUS(status)) { + case SUSPEND_SUPPORTED: + return; + case SUSPEND_NOT_SUPPORTED: + error_set(err, QERR_UNSUPPORTED); + return; + default: + goto undef_err; + } } - error_set(err, QERR_UNSUPPORTED); - -out: - close(pipefds[0]); +undef_err: + error_set(err, QERR_UNDEFINED_ERROR); } static void guest_suspend(const char *pmutils_bin, const char *sysfile_str, Error **err) { - pid_t pid; char *pmutils_path; + pid_t rpid, pid; + int status; pmutils_path = g_find_program_in_path(pmutils_bin); @@ -664,9 +635,18 @@ static void guest_suspend(const char *pmutils_bin, const char *sysfile_str, g_free(pmutils_path); if (pid < 0) { - error_set(err, QERR_UNDEFINED_ERROR); + goto exit_err; + } + + do { + rpid = waitpid(pid, &status, 0); + } while (rpid == -1 && errno == EINTR); + if (rpid == pid && WIFEXITED(status) && !WEXITSTATUS(status)) { return; } + +exit_err: + error_set(err, QERR_UNDEFINED_ERROR); } void qmp_guest_suspend_disk(Error **err) -- cgit v1.2.3 From d5dd3498ebf4d6dc8661f6a9a69ae10b50f3a6a6 Mon Sep 17 00:00:00 2001 From: Luiz Capitulino Date: Fri, 11 May 2012 16:19:47 -0300 Subject: qemu-ga: guest-shutdown: become synchronous Last commit dropped qemu-ga's SIGCHLD handler, used to automatically reap terminated children processes. This introduced a bug to qmp_guest_shutdown(): it will generate zombies. This problem probably doesn't matter in the success case, as the VM will shutdown anyway, but let's do the right thing and reap the created process. This ultimately means that guest-shutdown is now a synchronous command. An interesting side effect is that guest-shutdown is now able to report an error to the client if shutting down fails. Signed-off-by: Luiz Capitulino Reviewed-by: Eric Blake Signed-off-by: Michael Roth --- qga/commands-posix.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/qga/commands-posix.c b/qga/commands-posix.c index 76c8235272..68da71f232 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -36,8 +36,9 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err) { - int ret; const char *shutdown_flag; + int ret, status; + pid_t rpid, pid; slog("guest-shutdown called, mode: %s", mode); if (!has_mode || strcmp(mode, "powerdown") == 0) { @@ -52,8 +53,8 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err) return; } - ret = fork(); - if (ret == 0) { + pid = fork(); + if (pid == 0) { /* child, start the shutdown */ setsid(); fclose(stdin); @@ -66,9 +67,19 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err) slog("guest-shutdown failed: %s", strerror(errno)); } exit(!!ret); - } else if (ret < 0) { - error_set(err, QERR_UNDEFINED_ERROR); + } else if (pid < 0) { + goto exit_err; } + + do { + rpid = waitpid(pid, &status, 0); + } while (rpid == -1 && errno == EINTR); + if (rpid == pid && WIFEXITED(status) && !WEXITSTATUS(status)) { + return; + } + +exit_err: + error_set(err, QERR_UNDEFINED_ERROR); } typedef struct GuestFileHandle { -- cgit v1.2.3 From 3674838cd05268954bb6473239cd7f700a79bf0f Mon Sep 17 00:00:00 2001 From: Luiz Capitulino Date: Mon, 14 May 2012 15:25:20 -0300 Subject: qemu-ga: guest-shutdown: use only async-signal-safe functions POSIX mandates[1] that a child process of a multi-thread program uses only async-signal-safe functions before exec(). We consider qemu-ga to be multi-thread, because it uses glib. However, qmp_guest_shutdown() uses functions that are not async-signal-safe. Fix it the following way: - fclose() -> reopen_fd_to_null() - execl() -> execle() - exit() -> _exit() - drop slog() usage (which is not safe) [1] http://pubs.opengroup.org/onlinepubs/009695399/functions/fork.html Signed-off-by: Luiz Capitulino Reviewed-by: Eric Blake Signed-off-by: Michael Roth --- qapi-schema-guest.json | 3 +-- qga/commands-posix.c | 19 ++++++++----------- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json index 7c6cb21a79..d4055d262a 100644 --- a/qapi-schema-guest.json +++ b/qapi-schema-guest.json @@ -126,8 +126,7 @@ # @guest-shutdown: # # Initiate guest-activated shutdown. Note: this is an asynchronous -# shutdown request, with no guaruntee of successful shutdown. Errors -# will be logged to guest's syslog. +# shutdown request, with no guarantee of successful shutdown. # # @mode: #optional "halt", "powerdown" (default), or "reboot" # diff --git a/qga/commands-posix.c b/qga/commands-posix.c index 68da71f232..7664be10a0 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -37,8 +37,8 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err) { const char *shutdown_flag; - int ret, status; pid_t rpid, pid; + int status; slog("guest-shutdown called, mode: %s", mode); if (!has_mode || strcmp(mode, "powerdown") == 0) { @@ -57,16 +57,13 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err) if (pid == 0) { /* child, start the shutdown */ setsid(); - fclose(stdin); - fclose(stdout); - fclose(stderr); - - ret = execl("/sbin/shutdown", "shutdown", shutdown_flag, "+0", - "hypervisor initiated shutdown", (char*)NULL); - if (ret) { - slog("guest-shutdown failed: %s", strerror(errno)); - } - exit(!!ret); + reopen_fd_to_null(0); + reopen_fd_to_null(1); + reopen_fd_to_null(2); + + execle("/sbin/shutdown", "shutdown", shutdown_flag, "+0", + "hypervisor initiated shutdown", (char*)NULL, environ); + _exit(EXIT_FAILURE); } else if (pid < 0) { goto exit_err; } -- cgit v1.2.3 From 6c615ec57e83bf8cc7b1721bcd58c7d1ed93ef65 Mon Sep 17 00:00:00 2001 From: Michael Roth Date: Mon, 14 May 2012 16:42:35 -0500 Subject: qemu-ga: fix segv after failure to open log file Currently, if we fail to open the specified log file (generally due to a permissions issue), we'll assign NULL to the logfile handle (stderr, initially) used by the logging routines, which can cause a segfault to occur when we attempt to report the error before exiting. Instead, only re-assign if the open() was successful. Reviewed-by: Michal Privoznik Signed-off-by: Michael Roth --- qemu-ga.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/qemu-ga.c b/qemu-ga.c index 3a88333a17..42e271f35f 100644 --- a/qemu-ga.c +++ b/qemu-ga.c @@ -836,12 +836,13 @@ int main(int argc, char **argv) become_daemon(pid_filepath); } if (log_filepath) { - s->log_file = fopen(log_filepath, "a"); - if (!s->log_file) { + FILE *log_file = fopen(log_filepath, "a"); + if (!log_file) { g_critical("unable to open specified log file: %s", strerror(errno)); goto out_bad; } + s->log_file = log_file; } } -- cgit v1.2.3 From 8efacc43aef2c8fffceb0aaa52b8b7658a3e41ff Mon Sep 17 00:00:00 2001 From: Michael Roth Date: Mon, 14 May 2012 09:33:48 -0500 Subject: qemu-ga: align versioning with QEMU_VERSION Previously qemu-ga version was defined seperately. Since it is aligned with QEMU releases, use QEMU_VERSION instead. This also implies the version bump for 1.1[-rcN] release of qemu-ga. Reviewed-by: Michal Privoznik Acked-by: Luiz Capitulino Signed-off-by: Michael Roth --- qemu-ga.c | 4 ++-- qga/commands.c | 2 +- qga/guest-agent-core.h | 1 - 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/qemu-ga.c b/qemu-ga.c index 42e271f35f..8199da789c 100644 --- a/qemu-ga.c +++ b/qemu-ga.c @@ -169,7 +169,7 @@ static void usage(const char *cmd) " -h, --help display this help and exit\n" "\n" "Report bugs to \n" - , cmd, QGA_VERSION, QGA_VIRTIO_PATH_DEFAULT, QGA_PIDFILE_DEFAULT, + , cmd, QEMU_VERSION, QGA_VIRTIO_PATH_DEFAULT, QGA_PIDFILE_DEFAULT, QGA_STATEDIR_DEFAULT); } @@ -729,7 +729,7 @@ int main(int argc, char **argv) log_level = G_LOG_LEVEL_MASK; break; case 'V': - printf("QEMU Guest Agent %s\n", QGA_VERSION); + printf("QEMU Guest Agent %s\n", QEMU_VERSION); return 0; case 'd': daemonize = 1; diff --git a/qga/commands.c b/qga/commands.c index 5bcceaae34..46b0b083bc 100644 --- a/qga/commands.c +++ b/qga/commands.c @@ -52,7 +52,7 @@ struct GuestAgentInfo *qmp_guest_info(Error **err) GuestAgentCommandInfoList *cmd_info_list; char **cmd_list_head, **cmd_list; - info->version = g_strdup(QGA_VERSION); + info->version = g_strdup(QEMU_VERSION); cmd_list_head = cmd_list = qmp_get_command_list(); if (*cmd_list_head == NULL) { diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h index 6dba10484d..49a7abee95 100644 --- a/qga/guest-agent-core.h +++ b/qga/guest-agent-core.h @@ -13,7 +13,6 @@ #include "qapi/qmp-core.h" #include "qemu-common.h" -#define QGA_VERSION "1.0" #define QGA_READ_COUNT_DEFAULT 4096 typedef struct GAState GAState; -- cgit v1.2.3