Merge remote-tracking branch 'mdroth/qga-pull-4-27-12' into staging

* mdroth/qga-pull-4-27-12:
  qemu-ga: persist tracking of fsfreeze state via filesystem
  qemu-ga: add a whitelist for fsfreeze-safe commands
  qemu-ga: improve recovery options for fsfreeze

6 files changed, 409 insertions, 138 deletions

diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
index cf18876c57..d7a073ee70 100644
--- a/qapi-schema-guest.json
+++ b/qapi-schema-guest.json
@@ -296,14 +296,10 @@
 #
 # @frozen: all non-network guest filesystems frozen
 #
-# @error: failure to thaw 1 or more
-#         previously frozen filesystems, or failure to open a previously
-#         cached filesytem (filesystem unmounted/directory changes, etc).
-#
 # Since: 0.15.0
 ##
 { 'enum': 'GuestFsfreezeStatus',
-  'data': [ 'thawed', 'frozen', 'error' ] }
+  'data': [ 'thawed', 'frozen' ] }
 
 ##
 # @guest-fsfreeze-status:
@@ -312,6 +308,9 @@
 #
 # Returns: GuestFsfreezeStatus ("thawed", "frozen", etc., as defined below)
 #
+# Note: This may fail to properly report the current state as a result of
+# some other guest processes having issued an fs freeze/thaw.
+#
 # Since: 0.15.0
 ##
 { 'command': 'guest-fsfreeze-status',
@@ -320,9 +319,10 @@
 ##
 # @guest-fsfreeze-freeze:
 #
-# Sync and freeze all non-network guest filesystems
+# Sync and freeze all freezable, local guest filesystems
 #
-# Returns: Number of file systems frozen on success
+# Returns: Number of file systems currently frozen. On error, all filesystems
+# will be thawed.
 #
 # Since: 0.15.0
 ##
@@ -332,10 +332,15 @@
 ##
 # @guest-fsfreeze-thaw:
 #
-# Unfreeze frozen guest fileystems
+# Unfreeze all frozen guest filesystems
+#
+# Returns: Number of file systems thawed by this call
 #
-# Returns: Number of file systems thawed
-#          If error, -1 (unknown error) or -errno
+# Note: if return value does not match the previous call to
+#       guest-fsfreeze-freeze, this likely means some freezable
+#       filesystems were unfrozen before this call, and that the
+#       filesystem state may have changed before issuing this
+#       command.
 #
 # Since: 0.15.0
 ##
diff --git a/qapi/qmp-core.h b/qapi/qmp-core.h
index 3bb3acb589..431ddbb337 100644
--- a/qapi/qmp-core.h
+++ b/qapi/qmp-core.h
@@ -38,6 +38,7 @@ void qmp_register_command(const char *name, QmpCommandFunc *fn);
 QmpCommand *qmp_find_command(const char *name);
 QObject *qmp_dispatch(QObject *request);
 void qmp_disable_command(const char *name);
+void qmp_enable_command(const char *name);
 bool qmp_command_is_enabled(const char *name);
 char **qmp_get_command_list(void);
 
diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c
index 25c89ad098..43d5cdeb64 100644
--- a/qapi/qmp-registry.c
+++ b/qapi/qmp-registry.c
@@ -40,18 +40,28 @@ QmpCommand *qmp_find_command(const char *name)
     return NULL;
 }
 
-void qmp_disable_command(const char *name)
+static void qmp_toggle_command(const char *name, bool enabled)
 {
     QmpCommand *cmd;
 
     QTAILQ_FOREACH(cmd, &qmp_commands, node) {
         if (strcmp(cmd->name, name) == 0) {
-            cmd->enabled = false;
+            cmd->enabled = enabled;
             return;
         }
     }
 }
 
+void qmp_disable_command(const char *name)
+{
+    qmp_toggle_command(name, false);
+}
+
+void qmp_enable_command(const char *name)
+{
+    qmp_toggle_command(name, true);
+}
+
 bool qmp_command_is_enabled(const char *name)
 {
     QmpCommand *cmd;
diff --git a/qemu-ga.c b/qemu-ga.c
index 74a1b02c68..216be39072 100644
--- a/qemu-ga.c
+++ b/qemu-ga.c
@@ -18,6 +18,7 @@
 #ifndef _WIN32
 #include <syslog.h>
 #include <sys/wait.h>
+#include <sys/stat.h>
 #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 {
@@ -56,10 +58,27 @@ struct GAState {
     GAService service;
 #endif
     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;
 
+/* commands that are safe to issue while filesystems are frozen */
+static const char *ga_freeze_whitelist[] = {
+    "guest-ping",
+    "guest-info",
+    "guest-sync",
+    "guest-fsfreeze-status",
+    "guest-fsfreeze-thaw",
+    NULL
+};
+
 #ifdef _WIN32
 DWORD WINAPI service_ctrl_handler(DWORD ctrl, DWORD type, LPVOID data,
                                   LPVOID ctx);
@@ -68,6 +87,15 @@ VOID WINAPI service_main(DWORD argc, TCHAR *argv[]);
 
 static void quit_handler(int sig)
 {
+    /* if we're frozen, don't exit unless we're absolutely forced to,
+     * because it's basically impossible for graceful exit to complete
+     * unless all log/pid files are on unfreezable filesystems. there's
+     * also a very likely chance killing the agent before unfreezing
+     * the filesystems is a mistake (or will be viewed as one later).
+     */
+    if (ga_is_frozen(ga_state)) {
+        return;
+    }
     g_debug("received signal num %d, quitting", sig);
 
     if (g_main_loop_is_running(ga_state->main_loop)) {
@@ -126,6 +154,8 @@ static void usage(const char *cmd)
 "                    %s)\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"
@@ -137,7 +167,8 @@ static void usage(const char *cmd)
 "  -h, --help        display this help and exit\n"
 "\n"
 "Report bugs to <mdroth@linux.vnet.ibm.com>\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)
@@ -207,11 +238,171 @@ void ga_set_response_delimited(GAState *s)
 }
 
 #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);
+}
+
+/* disable commands that aren't safe for fsfreeze */
+static void ga_disable_non_whitelisted(void)
+{
+    char **list_head, **list;
+    bool whitelisted;
+    int i;
+
+    list_head = list = qmp_get_command_list();
+    while (*list != NULL) {
+        whitelisted = false;
+        i = 0;
+        while (ga_freeze_whitelist[i] != NULL) {
+            if (strcmp(*list, ga_freeze_whitelist[i]) == 0) {
+                whitelisted = true;
+            }
+            i++;
+        }
+        if (!whitelisted) {
+            g_debug("disabling command: %s", *list);
+            qmp_disable_command(*list);
+        }
+        g_free(*list);
+        list++;
+    }
+    g_free(list_head);
+}
+
+/* [re-]enable all commands, except those explictly blacklisted by user */
+static void ga_enable_non_blacklisted(GList *blacklist)
+{
+    char **list_head, **list;
+
+    list_head = list = qmp_get_command_list();
+    while (*list != NULL) {
+        if (g_list_find_custom(blacklist, *list, ga_strcmp) == NULL &&
+            !qmp_command_is_enabled(*list)) {
+            g_debug("enabling command: %s", *list);
+            qmp_enable_command(*list);
+        }
+        g_free(*list);
+        list++;
+    }
+    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;
+}
+
+void ga_set_frozen(GAState *s)
+{
+    if (ga_is_frozen(s)) {
+        return;
+    }
+    /* disable all non-whitelisted (for frozen state) commands */
+    ga_disable_non_whitelisted();
+    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)
+{
+    if (!ga_is_frozen(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 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);
+    }
+}
+
 static void become_daemon(const char *pidfile)
 {
+#ifndef _WIN32
     pid_t pid, sid;
-    int pidfd;
-    char *pidstr = NULL;
 
     pid = fork();
     if (pid < 0) {
@@ -221,20 +412,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);
@@ -249,15 +431,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)
 {
@@ -495,9 +676,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
@@ -513,12 +696,13 @@ int main(int argc, char **argv)
         { "blacklist", 1, NULL, 'b' },
 #ifdef _WIN32
         { "service", 1, NULL, 's' },
-#endif        
+#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;
 
     module_call_init(MODULE_INIT_QAPI);
@@ -532,17 +716,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;
@@ -568,14 +749,12 @@ int main(int argc, char **argv)
             for (j = 0, i = 0, len = strlen(optarg); i < len; i++) {
                 if (optarg[i] == ',') {
                     optarg[i] = 0;
-                    qmp_disable_command(&optarg[j]);
-                    g_debug("disabling command: %s", &optarg[j]);
+                    blacklist = g_list_append(blacklist, &optarg[j]);
                     j = i + 1;
                 }
             }
             if (j < i) {
-                qmp_disable_command(&optarg[j]);
-                g_debug("disabling command: %s", &optarg[j]);
+                blacklist = g_list_append(blacklist, &optarg[j]);
             }
             break;
         }
@@ -583,7 +762,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 {
@@ -602,19 +781,78 @@ 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 {
+            g_debug("disabling command: %s", (char *)blacklist->data);
+            qmp_disable_command(blacklist->data);
+            blacklist = g_list_next(blacklist);
+        } while (blacklist);
+    }
     s->command_state = ga_command_state_new();
     ga_command_state_init(s, s->command_state);
     ga_command_state_init_all(s->command_state);
@@ -648,13 +886,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;
 }
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 087c3af7ff..d58730ad80 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -316,44 +316,40 @@ static void guest_file_init(void)
 
 #if defined(CONFIG_FSFREEZE)
 
-static void disable_logging(void)
-{
-    ga_disable_logging(ga_state);
-}
-
-static void enable_logging(void)
-{
-    ga_enable_logging(ga_state);
-}
-
 typedef struct GuestFsfreezeMount {
     char *dirname;
     char *devtype;
     QTAILQ_ENTRY(GuestFsfreezeMount) next;
 } GuestFsfreezeMount;
 
-struct {
-    GuestFsfreezeStatus status;
-    QTAILQ_HEAD(, GuestFsfreezeMount) mount_list;
-} guest_fsfreeze_state;
+typedef QTAILQ_HEAD(, GuestFsfreezeMount) GuestFsfreezeMountList;
+
+static void guest_fsfreeze_free_mount_list(GuestFsfreezeMountList *mounts)
+{
+     GuestFsfreezeMount *mount, *temp;
+
+     if (!mounts) {
+         return;
+     }
+
+     QTAILQ_FOREACH_SAFE(mount, mounts, next, temp) {
+         QTAILQ_REMOVE(mounts, mount, next);
+         g_free(mount->dirname);
+         g_free(mount->devtype);
+         g_free(mount);
+     }
+}
 
 /*
  * Walk the mount table and build a list of local file systems
  */
-static int guest_fsfreeze_build_mount_list(void)
+static int guest_fsfreeze_build_mount_list(GuestFsfreezeMountList *mounts)
 {
     struct mntent *ment;
-    GuestFsfreezeMount *mount, *temp;
+    GuestFsfreezeMount *mount;
     char const *mtab = MOUNTED;
     FILE *fp;
 
-    QTAILQ_FOREACH_SAFE(mount, &guest_fsfreeze_state.mount_list, next, temp) {
-        QTAILQ_REMOVE(&guest_fsfreeze_state.mount_list, mount, next);
-        g_free(mount->dirname);
-        g_free(mount->devtype);
-        g_free(mount);
-    }
-
     fp = setmntent(mtab, "r");
     if (!fp) {
         g_warning("fsfreeze: unable to read mtab");
@@ -377,7 +373,7 @@ static int guest_fsfreeze_build_mount_list(void)
         mount->dirname = g_strdup(ment->mnt_dir);
         mount->devtype = g_strdup(ment->mnt_type);
 
-        QTAILQ_INSERT_TAIL(&guest_fsfreeze_state.mount_list, mount, next);
+        QTAILQ_INSERT_TAIL(mounts, mount, next);
     }
 
     endmntent(fp);
@@ -390,7 +386,11 @@ static int guest_fsfreeze_build_mount_list(void)
  */
 GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err)
 {
-    return guest_fsfreeze_state.status;
+    if (ga_is_frozen(ga_state)) {
+        return GUEST_FSFREEZE_STATUS_FROZEN;
+    }
+
+    return GUEST_FSFREEZE_STATUS_THAWED;
 }
 
 /*
@@ -400,61 +400,61 @@ GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err)
 int64_t qmp_guest_fsfreeze_freeze(Error **err)
 {
     int ret = 0, i = 0;
-    struct GuestFsfreezeMount *mount, *temp;
+    GuestFsfreezeMountList mounts;
+    struct GuestFsfreezeMount *mount;
     int fd;
     char err_msg[512];
 
     slog("guest-fsfreeze called");
 
-    if (guest_fsfreeze_state.status == GUEST_FSFREEZE_STATUS_FROZEN) {
-        return 0;
-    }
-
-    ret = guest_fsfreeze_build_mount_list();
+    QTAILQ_INIT(&mounts);
+    ret = guest_fsfreeze_build_mount_list(&mounts);
     if (ret < 0) {
         return ret;
     }
 
     /* cannot risk guest agent blocking itself on a write in this state */
-    disable_logging();
+    ga_set_frozen(ga_state);
 
-    QTAILQ_FOREACH_SAFE(mount, &guest_fsfreeze_state.mount_list, next, temp) {
+    QTAILQ_FOREACH(mount, &mounts, next) {
         fd = qemu_open(mount->dirname, O_RDONLY);
         if (fd == -1) {
-            sprintf(err_msg, "failed to open %s, %s", mount->dirname, strerror(errno));
+            sprintf(err_msg, "failed to open %s, %s", mount->dirname,
+                    strerror(errno));
             error_set(err, QERR_QGA_COMMAND_FAILED, err_msg);
             goto error;
         }
 
         /* we try to cull filesytems we know won't work in advance, but other
          * filesytems may not implement fsfreeze for less obvious reasons.
-         * these will report EOPNOTSUPP, so we simply ignore them. when
-         * thawing, these filesystems will return an EINVAL instead, due to
-         * not being in a frozen state. Other filesystem-specific
-         * errors may result in EINVAL, however, so the user should check the
-         * number * of filesystems returned here against those returned by the
-         * thaw operation to determine whether everything completed
-         * successfully
+         * these will report EOPNOTSUPP. we simply ignore these when tallying
+         * the number of frozen filesystems.
+         *
+         * any other error means a failure to freeze a filesystem we
+         * expect to be freezable, so return an error in those cases
+         * and return system to thawed state.
          */
         ret = ioctl(fd, FIFREEZE);
-        if (ret < 0 && errno != EOPNOTSUPP) {
-            sprintf(err_msg, "failed to freeze %s, %s", mount->dirname, strerror(errno));
-            error_set(err, QERR_QGA_COMMAND_FAILED, err_msg);
-            close(fd);
-            goto error;
+        if (ret == -1) {
+            if (errno != EOPNOTSUPP) {
+                sprintf(err_msg, "failed to freeze %s, %s",
+                        mount->dirname, strerror(errno));
+                error_set(err, QERR_QGA_COMMAND_FAILED, err_msg);
+                close(fd);
+                goto error;
+            }
+        } else {
+            i++;
         }
         close(fd);
-
-        i++;
     }
 
-    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_FROZEN;
+    guest_fsfreeze_free_mount_list(&mounts);
     return i;
 
 error:
-    if (i > 0) {
-        qmp_guest_fsfreeze_thaw(NULL);
-    }
+    guest_fsfreeze_free_mount_list(&mounts);
+    qmp_guest_fsfreeze_thaw(NULL);
     return 0;
 }
 
@@ -464,47 +464,61 @@ error:
 int64_t qmp_guest_fsfreeze_thaw(Error **err)
 {
     int ret;
-    GuestFsfreezeMount *mount, *temp;
-    int fd, i = 0;
-    bool has_error = false;
+    GuestFsfreezeMountList mounts;
+    GuestFsfreezeMount *mount;
+    int fd, i = 0, logged;
+
+    QTAILQ_INIT(&mounts);
+    ret = guest_fsfreeze_build_mount_list(&mounts);
+    if (ret) {
+        error_set(err, QERR_QGA_COMMAND_FAILED,
+                  "failed to enumerate filesystems");
+        return 0;
+    }
 
-    QTAILQ_FOREACH_SAFE(mount, &guest_fsfreeze_state.mount_list, next, temp) {
+    QTAILQ_FOREACH(mount, &mounts, next) {
+        logged = false;
         fd = qemu_open(mount->dirname, O_RDONLY);
         if (fd == -1) {
-            has_error = true;
-            continue;
-        }
-        ret = ioctl(fd, FITHAW);
-        if (ret < 0 && errno != EOPNOTSUPP && errno != EINVAL) {
-            has_error = true;
-            close(fd);
             continue;
         }
+        /* we have no way of knowing whether a filesystem was actually unfrozen
+         * as a result of a successful call to FITHAW, only that if an error
+         * was returned the filesystem was *not* unfrozen by that particular
+         * call.
+         *
+         * since multiple preceeding FIFREEZEs require multiple calls to FITHAW
+         * to unfreeze, continuing issuing FITHAW until an error is returned,
+         * in which case either the filesystem is in an unfreezable state, or,
+         * more likely, it was thawed previously (and remains so afterward).
+         *
+         * also, since the most recent successful call is the one that did
+         * the actual unfreeze, we can use this to provide an accurate count
+         * of the number of filesystems unfrozen by guest-fsfreeze-thaw, which
+         * may * be useful for determining whether a filesystem was unfrozen
+         * during the freeze/thaw phase by a process other than qemu-ga.
+         */
+        do {
+            ret = ioctl(fd, FITHAW);
+            if (ret == 0 && !logged) {
+                i++;
+                logged = true;
+            }
+        } while (ret == 0);
         close(fd);
-        i++;
     }
 
-    if (has_error) {
-        guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_ERROR;
-    } else {
-        guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_THAWED;
-    }
-    enable_logging();
+    ga_unset_frozen(ga_state);
+    guest_fsfreeze_free_mount_list(&mounts);
     return i;
 }
 
-static void guest_fsfreeze_init(void)
-{
-    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_THAWED;
-    QTAILQ_INIT(&guest_fsfreeze_state.mount_list);
-}
-
 static void guest_fsfreeze_cleanup(void)
 {
     int64_t ret;
     Error *err = NULL;
 
-    if (guest_fsfreeze_state.status == GUEST_FSFREEZE_STATUS_FROZEN) {
+    if (ga_is_frozen(ga_state) == GUEST_FSFREEZE_STATUS_FROZEN) {
         ret = qmp_guest_fsfreeze_thaw(&err);
         if (ret < 0 || err) {
             slog("failed to clean up frozen filesystems");
@@ -933,7 +947,7 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
 void ga_command_state_init(GAState *s, GACommandState *cs)
 {
 #if defined(CONFIG_FSFREEZE)
-    ga_command_state_add(cs, guest_fsfreeze_init, guest_fsfreeze_cleanup);
+    ga_command_state_add(cs, NULL, guest_fsfreeze_cleanup);
 #endif
     ga_command_state_add(cs, guest_file_init, NULL);
 }
diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
index 304525d3c2..bbb8b9b125 100644
--- a/qga/guest-agent-core.h
+++ b/qga/guest-agent-core.h
@@ -32,3 +32,6 @@ void ga_disable_logging(GAState *s);
 void ga_enable_logging(GAState *s);
 void slog(const gchar *fmt, ...);
 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);