From patchwork Wed Apr 18 22:04:11 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Michael Roth X-Patchwork-Id: 153615 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id B6407B6EE7 for ; Thu, 19 Apr 2012 08:04:48 +1000 (EST) Received: from localhost ([::1]:46380 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SKczW-0008TU-Ip for incoming@patchwork.ozlabs.org; Wed, 18 Apr 2012 18:04:46 -0400 Received: from eggs.gnu.org ([208.118.235.92]:47065) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SKczE-0008Lx-Rx for qemu-devel@nongnu.org; Wed, 18 Apr 2012 18:04:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SKczB-00052D-Og for qemu-devel@nongnu.org; Wed, 18 Apr 2012 18:04:28 -0400 Received: from mail-yx0-f173.google.com ([209.85.213.173]:38174) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SKczB-000526-Gh for qemu-devel@nongnu.org; Wed, 18 Apr 2012 18:04:25 -0400 Received: by yenr5 with SMTP id r5so5272928yen.4 for ; Wed, 18 Apr 2012 15:04:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:from:to:cc:subject:date:message-id:x-mailer:in-reply-to :references; bh=pM2Qzxz6Wza0Fh6KeYvjH+AYj+ggf8UZZqUy9HHLtaY=; b=IrSnd+fK/y6YtTS+4zHx7BScuVI2/XPMB12RxSgEiTxXEw3UjlBUcwkCzMFpwFYOZx +l/K0N6K7PYh5MYmArabiqfyxvFCbBoJ1kNYGp7iGucXCzEsJtB3SE8V+8TM9NjmE85o wpj8DUCH6DdfgsByvYTfAwxZPc0MzmS5a6a/OWFOfP+CTKodPNYO3SQiFFkOYX5TYRGv 5kB+EQ79dCdB7z6x7tMrGO0dGBheCiffW+88O3XoOPxwNi8ouoiFyqOmv9uz3Om2/agM fcfHYFfi2LCa8MfBHZ8jNfzqmjdJyG48CNh+MM979tyECbahRvpvvHoJhgnWTKSsSLRj li7w== Received: by 10.60.24.164 with SMTP id v4mr5289372oef.51.1334786663656; Wed, 18 Apr 2012 15:04:23 -0700 (PDT) Received: from localhost.localdomain ([32.97.110.59]) by mx.google.com with ESMTPS id d9sm286497obz.6.2012.04.18.15.04.21 (version=TLSv1/SSLv3 cipher=OTHER); Wed, 18 Apr 2012 15:04:22 -0700 (PDT) From: Michael Roth To: qemu-devel@nongnu.org Date: Wed, 18 Apr 2012 17:04:11 -0500 Message-Id: <1334786651-30519-3-git-send-email-mdroth@linux.vnet.ibm.com> X-Mailer: git-send-email 1.7.4.1 In-Reply-To: <1334786651-30519-1-git-send-email-mdroth@linux.vnet.ibm.com> References: <1334786651-30519-1-git-send-email-mdroth@linux.vnet.ibm.com> X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.85.213.173 Cc: mprivozn@redhat.com, jcody@redhat.com Subject: [Qemu-devel] [PATCH 3/3] qemu-ga: persist tracking of fsfreeze state via filesystem X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Currently, qemu-ga may die/get killed/go away for whatever reason after guest-fsfreeze-freeze has been issued, and before guest-fsfreeze-thaw has been issued. This means the only way to unfreeze the guest is via VNC/network/console access, but obtaining that access after-the-fact can often be very difficult when filesystems are frozen. Logins will almost always hang, for instance. In many cases the only recourse would be to reboot the guest without any quiescing of volatile state, which makes this a corner-case worth giving some attention to. A likely failsafe for this situation would be to use a watchdog to restart qemu-ga if it goes away. There are some precautions qemu-ga needs to take in order to avoid immediately hanging itself on I/O, however, namely, we must disable logging and defer to processing/creation of user-specific logfiles, along with creation of the pid file if we're running as a daemon. We also need to disable non-fsfreeze-safe commands, as we normally would when processing the guest-fsfreeze-freeze command. To track when we need to do this in a way that persists between multiple invocations of qemu-ga, we create a file on the guest filesystem before issuing the fsfreeze, and delete it when doing the thaw. On qemu-ga startup, we check for the existance of this file to determine the need to take the above precautions. We're forced to do it this way since a more traditional approach such as reading/writing state to a dedicated state file will cause access/modification time updates, respectively, both of which will hang if the file resides on a frozen filesystem. Both can occur even if relatime is enabled. Checking for file existence will not update the access time, however, so it's a safe way to check for fsfreeze state. An actual watchdog-based restart of qemu-ga can itself cause an access time update that would thus hang the invocation of qemu-ga, but the logic to workaround that can be handled via the watchdog, so we don't address that here (for relatime we'd periodically touch the qemu-ga binary if the file $qga_statedir/qga.state.isfrozen is not present, this avoids qemu-ga updates or the 1 day relatime threshold causing an access-time update if we try to respawn qemu-ga shortly after it goes away) Signed-off-by: Michael Roth --- qapi-schema-guest.json | 8 +- qemu-ga.c | 218 ++++++++++++++++++++++++++++++++++++++---------- 2 files changed, 177 insertions(+), 49 deletions(-) diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json index dc25be0..11488b4 100644 --- a/qapi-schema-guest.json +++ b/qapi-schema-guest.json @@ -309,10 +309,10 @@ # Returns: GuestFsfreezeStatus ("thawed", "frozen", etc., as defined below) # # Note: This may fail to properly report the current state as a result of -# qemu-ga having been restarted, or other guest processes having issued -# an fsfreeze. To be safe, rather than relying on status reported here, users -# should always issue freeze/unfreeze in pairs to allow a guest to return to -# a fully-functional state: an extra unfreeze will simply result in a no-op. +# some other guest processes having issued an fsfreeze. To be safe, rather +# than relying on status reported here, users should always issue +# freeze/unfreeze in pairs to allow a guest to return to a fully-functional +# state: an extra unfreeze will simply result in a no-op. # # Since: 0.15.0 ## diff --git a/qemu-ga.c b/qemu-ga.c index 5ebd1c1..792f0eb 100644 --- a/qemu-ga.c +++ b/qemu-ga.c @@ -18,6 +18,7 @@ #ifndef _WIN32 #include #include +#include #endif #include "json-streamer.h" #include "json-parser.h" @@ -41,6 +42,7 @@ #define QGA_VIRTIO_PATH_DEFAULT "\\\\.\\Global\\org.qemu.guest_agent.0" #endif #define QGA_PIDFILE_DEFAULT "/var/run/qemu-ga.pid" +#define QGA_STATEDIR_DEFAULT "/tmp" #define QGA_SENTINEL_BYTE 0xFF struct GAState { @@ -58,6 +60,11 @@ struct GAState { bool delimit_response; bool frozen; GList *blacklist; + const char *state_filepath_isfrozen; + struct { + const char *log_filepath; + const char *pid_filepath; + } deferred_options; }; struct GAState *ga_state; @@ -146,6 +153,8 @@ static void usage(const char *cmd) " -p, --path device/socket path (%s is the default for virtio-serial)\n" " -l, --logfile set logfile path, logs to stderr by default\n" " -f, --pidfile specify pidfile (default is %s)\n" +" -t, --statedir specify dir to store state information (absolute paths\n" +" only, default is %s)\n" " -v, --verbose log extra debugging information\n" " -V, --version print version information and exit\n" " -d, --daemonize become a daemon\n" @@ -157,7 +166,8 @@ 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, QGA_VERSION, QGA_VIRTIO_PATH_DEFAULT, QGA_PIDFILE_DEFAULT, + QGA_STATEDIR_DEFAULT); } static const char *ga_log_level_str(GLogLevelFlags level) @@ -226,6 +236,41 @@ void ga_set_response_delimited(GAState *s) s->delimit_response = true; } +#ifndef _WIN32 +static bool ga_open_pidfile(const char *pidfile) +{ + int pidfd; + char pidstr[32]; + + pidfd = open(pidfile, O_CREAT|O_WRONLY, S_IRUSR|S_IWUSR); + if (pidfd == -1 || lockf(pidfd, F_TLOCK, 0)) { + g_critical("Cannot lock pid file, %s", strerror(errno)); + return false; + } + + if (ftruncate(pidfd, 0) || lseek(pidfd, 0, SEEK_SET)) { + g_critical("Failed to truncate pid file"); + goto fail; + } + sprintf(pidstr, "%d", getpid()); + if (write(pidfd, pidstr, strlen(pidstr)) != strlen(pidstr)) { + g_critical("Failed to write pid file"); + goto fail; + } + + return true; + +fail: + unlink(pidfile); + return false; +} +#else /* _WIN32 */ +static bool ga_open_pidfile(const char *pidfile) +{ + return true; +} +#endif + static gint ga_strcmp(gconstpointer str1, gconstpointer str2) { return strcmp(str1, str2); @@ -276,6 +321,28 @@ static void ga_enable_non_blacklisted(GList *blacklist) g_free(list_head); } +static bool ga_create_file(const char *path) +{ + int fd = open(path, O_CREAT | O_WRONLY, S_IWUSR | S_IRUSR); + if (fd == -1) { + g_warning("unable to open/create file %s: %s", path, strerror(errno)); + return false; + } + close(fd); + return true; +} + +static bool ga_delete_file(const char *path) +{ + int ret = unlink(path); + if (ret == -1) { + g_warning("unable to delete file: %s: %s", path, strerror(errno)); + return false; + } + + return true; +} + bool ga_is_frozen(GAState *s) { return s->frozen; @@ -291,6 +358,10 @@ void ga_set_frozen(GAState *s) g_warning("disabling logging due to filesystem freeze"); ga_disable_logging(s); s->frozen = true; + if (!ga_create_file(s->state_filepath_isfrozen)) { + g_warning("unable to create %s, fsfreeze may not function properly", + s->state_filepath_isfrozen); + } } void ga_unset_frozen(GAState *s) @@ -299,20 +370,38 @@ void ga_unset_frozen(GAState *s) return; } + /* if we delayed creation/opening of pid/log files due to being + * in a frozen state at start up, do it now + */ + if (s->deferred_options.log_filepath) { + s->log_file = fopen(s->deferred_options.log_filepath, "a"); + if (!s->log_file) { + s->log_file = stderr; + } + s->deferred_options.log_filepath = NULL; + } ga_enable_logging(s); - g_warning("logging re-enabled"); + g_warning("logging re-enabled due to filesystem unfreeze"); + if (s->deferred_options.pid_filepath) { + if (!ga_open_pidfile(s->deferred_options.pid_filepath)) { + g_warning("failed to create/open pid file"); + } + s->deferred_options.pid_filepath = NULL; + } /* enable all disabled, non-blacklisted commands */ ga_enable_non_blacklisted(s->blacklist); s->frozen = false; + if (!ga_delete_file(s->state_filepath_isfrozen)) { + g_warning("unable to delete %s, fsfreeze may not function properly", + s->state_filepath_isfrozen); + } } -#ifndef _WIN32 static void become_daemon(const char *pidfile) { +#ifndef _WIN32 pid_t pid, sid; - int pidfd; - char *pidstr = NULL; pid = fork(); if (pid < 0) { @@ -322,20 +411,11 @@ static void become_daemon(const char *pidfile) exit(EXIT_SUCCESS); } - pidfd = open(pidfile, O_CREAT|O_WRONLY|O_EXCL, S_IRUSR|S_IWUSR); - if (pidfd == -1) { - g_critical("Cannot create pid file, %s", strerror(errno)); - exit(EXIT_FAILURE); - } - - if (asprintf(&pidstr, "%d", getpid()) == -1) { - g_critical("Cannot allocate memory"); - goto fail; - } - if (write(pidfd, pidstr, strlen(pidstr)) != strlen(pidstr)) { - free(pidstr); - g_critical("Failed to write pid file"); - goto fail; + if (pidfile) { + if (!ga_open_pidfile(pidfile)) { + g_critical("failed to create pidfile"); + exit(EXIT_FAILURE); + } } umask(0); @@ -350,15 +430,14 @@ static void become_daemon(const char *pidfile) close(STDIN_FILENO); close(STDOUT_FILENO); close(STDERR_FILENO); - free(pidstr); return; fail: unlink(pidfile); g_critical("failed to daemonize"); exit(EXIT_FAILURE); -} #endif +} static int send_response(GAState *s, QObject *payload) { @@ -596,9 +675,11 @@ VOID WINAPI service_main(DWORD argc, TCHAR *argv[]) int main(int argc, char **argv) { - const char *sopt = "hVvdm:p:l:f:b:s:"; - const char *method = NULL, *path = NULL, *pidfile = QGA_PIDFILE_DEFAULT; - const char *log_file_name = NULL; + const char *sopt = "hVvdm:p:l:f:b:s:t:"; + const char *method = NULL, *path = NULL; + const char *log_filepath = NULL; + const char *pid_filepath = QGA_PIDFILE_DEFAULT; + const char *state_dir = QGA_STATEDIR_DEFAULT; #ifdef _WIN32 const char *service = NULL; #endif @@ -615,11 +696,11 @@ int main(int argc, char **argv) #ifdef _WIN32 { "service", 1, NULL, 's' }, #endif + { "statedir", 1, NULL, 't' }, { NULL, 0, NULL, 0 } }; int opt_ind = 0, ch, daemonize = 0, i, j, len; GLogLevelFlags log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL; - FILE *log_file = stderr; GList *blacklist = NULL; GAState *s; @@ -634,17 +715,14 @@ int main(int argc, char **argv) path = optarg; break; case 'l': - log_file_name = optarg; - log_file = fopen(log_file_name, "a"); - if (!log_file) { - g_critical("unable to open specified log file: %s", - strerror(errno)); - return EXIT_FAILURE; - } + log_filepath = optarg; break; case 'f': - pidfile = optarg; + pid_filepath = optarg; break; + case 't': + state_dir = optarg; + break; case 'v': /* enable all log levels */ log_level = G_LOG_LEVEL_MASK; @@ -683,7 +761,7 @@ int main(int argc, char **argv) case 's': service = optarg; if (strcmp(service, "install") == 0) { - return ga_install_service(path, log_file_name); + return ga_install_service(path, log_filepath); } else if (strcmp(service, "uninstall") == 0) { return ga_uninstall_service(); } else { @@ -702,20 +780,70 @@ int main(int argc, char **argv) } } -#ifndef _WIN32 - if (daemonize) { - g_debug("starting daemon"); - become_daemon(pidfile); - } -#endif - s = g_malloc0(sizeof(GAState)); - s->log_file = log_file; s->log_level = log_level; + s->log_file = stderr; g_log_set_default_handler(ga_log, s); g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR); - s->logging_enabled = true; + ga_enable_logging(s); + s->state_filepath_isfrozen = g_strdup_printf("%s/qga.state.isfrozen", + state_dir); s->frozen = false; +#ifndef _WIN32 + /* check if a previous instance of qemu-ga exited with filesystems' state + * marked as frozen. this could be a stale value (a non-qemu-ga process + * or reboot may have since unfrozen them), but better to require an + * uneeded unfreeze than to risk hanging on start-up + */ + struct stat st; + if (stat(s->state_filepath_isfrozen, &st) == -1) { + /* it's okay if the file doesn't exist, but if we can't access for + * some other reason, such as permissions, there's a configuration + * that needs to be addressed. so just bail now before we get into + * more trouble later + */ + if (errno != ENOENT) { + g_critical("unable to access state file at path %s: %s", + s->state_filepath_isfrozen, strerror(errno)); + return EXIT_FAILURE; + } + } else { + g_warning("previous instance appears to have exited with frozen" + " filesystems. deferring logging/pidfile creation and" + " disabling non-fsfreeze-safe commands until" + " guest-fsfreeze-thaw is issued, or filesystems are" + " manually unfrozen and the file %s is removed", + s->state_filepath_isfrozen); + s->frozen = true; + } +#endif + + if (ga_is_frozen(s)) { + if (daemonize) { + /* delay opening/locking of pidfile till filesystem are unfrozen */ + s->deferred_options.pid_filepath = pid_filepath; + become_daemon(NULL); + } + if (log_filepath) { + /* delay opening the log file till filesystems are unfrozen */ + s->deferred_options.log_filepath = log_filepath; + } + ga_disable_logging(s); + ga_disable_non_whitelisted(); + } else { + if (daemonize) { + become_daemon(pid_filepath); + } + if (log_filepath) { + s->log_file = fopen(log_filepath, "a"); + if (!s->log_file) { + g_critical("unable to open specified log file: %s", + strerror(errno)); + goto out_bad; + } + } + } + if (blacklist) { s->blacklist = blacklist; do { @@ -757,13 +885,13 @@ int main(int argc, char **argv) ga_channel_free(ga_state->channel); if (daemonize) { - unlink(pidfile); + unlink(pid_filepath); } return 0; out_bad: if (daemonize) { - unlink(pidfile); + unlink(pid_filepath); } return EXIT_FAILURE; }