[Qemu-devel] [PATCH v4 0/2] qemu-ga: add hook to quiesce the guest on fsfreeze-freeze/thaw

[Qemu-devel] [PATCH v4 0/2] qemu-ga: add hook to quiesce the guest on fsfreeze-freeze/thaw

[Tomoki Sekiyama]

Hi,

This is version 4 of the qemu-ga fsfreeze hook patchset.

*Changes from v3: ( https://lists.gnu.org/archive/html/qemu-devel/2012-11/msg01041.html )
 1/2: Fix help message of --fsfreeze-hook option.
      Use enum for an argument of execute_fsfreeze_hook().
      Abort fsfreeze and report an error if pre-fsfreeze hook exits abnormally.
 2/2: Not changed.

---

Tomoki Sekiyama (2):
      qemu-ga: execute hook to quiesce the guest on fsfreeze-freeze/thaw
      qemu-ga: sample fsfreeze hooks


 docs/qemu-guest-agent/fsfreeze-hook                |   31 +++++++++
 .../fsfreeze-hook.d.sample/mysql-flush.sh          |   47 ++++++++++++++
 qemu-ga.c                                          |   42 ++++++++++++-
 qga/commands-posix.c                               |   66 ++++++++++++++++++++
 qga/guest-agent-core.h                             |    1 
 5 files changed, 186 insertions(+), 1 deletion(-)
 create mode 100755 docs/qemu-guest-agent/fsfreeze-hook
 create mode 100755 docs/qemu-guest-agent/fsfreeze-hook.d.sample/mysql-flush.sh


Thanks,
-- 
Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com>
Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory

Re: [Qemu-devel] [PATCH v4 1/2] qemu-ga: execute hook to quiesce the guest on fsfreeze-freeze/thaw

[Tomoki Sekiyama]

To use the online disk snapshot for online-backup, application-level
consistency of the snapshot image is required. However, currently the
guest agent can provide only filesystem-level consistency, and the
snapshot may contain dirty data, for example, incomplete transactions.
This patch provides the opportunity to quiesce applications before
snapshot is taken.

When the qemu-ga receives fsfreeze-freeze command, the hook script
specified in --fsfreeze-hook option is executed with "freeze" argument
before the filesystem is frozen. For fsfreeze-thaw command, the hook
script is executed with "thaw" argument after the filesystem is thawed.

Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com>
---
 qemu-ga.c              |   42 ++++++++++++++++++++++++++++++-
 qga/commands-posix.c   |   66 ++++++++++++++++++++++++++++++++++++++++++++++++
 qga/guest-agent-core.h |    1 +
 3 files changed, 108 insertions(+), 1 deletion(-)

diff --git a/qemu-ga.c b/qemu-ga.c
index 9b59a52..240f6e2 100644
--- a/qemu-ga.c
+++ b/qemu-ga.c
@@ -34,6 +34,12 @@
 #include "qga/service-win32.h"
 #include <windows.h>
 #endif
+#ifdef __linux__
+#include <linux/fs.h>
+#ifdef FIFREEZE
+#define CONFIG_FSFREEZE
+#endif
+#endif
 
 #ifndef _WIN32
 #define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent.0"
@@ -42,6 +48,9 @@
 #endif
 #define QGA_STATEDIR_DEFAULT CONFIG_QEMU_LOCALSTATEDIR "/run"
 #define QGA_PIDFILE_DEFAULT QGA_STATEDIR_DEFAULT "/qemu-ga.pid"
+#ifdef CONFIG_FSFREEZE
+#define QGA_FSFREEZE_HOOK_DEFAULT CONFIG_QEMU_CONFDIR "/fsfreeze-hook"
+#endif
 #define QGA_SENTINEL_BYTE 0xFF
 
 struct GAState {
@@ -64,6 +73,9 @@ struct GAState {
         const char *log_filepath;
         const char *pid_filepath;
     } deferred_options;
+#ifdef CONFIG_FSFREEZE
+    const char *fsfreeze_hook;
+#endif
 };
 
 struct GAState *ga_state;
@@ -153,6 +165,10 @@ 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"
+#ifdef CONFIG_FSFREEZE
+"  -F, --fsfreeze-hook\n"
+"                    specify fsfreeze hook (default is %s)\n"
+#endif
 "  -t, --statedir    specify dir to store state information (absolute paths\n"
 "                    only, default is %s)\n"
 "  -v, --verbose     log extra debugging information\n"
@@ -167,6 +183,9 @@ static void usage(const char *cmd)
 "\n"
 "Report bugs to <mdroth@linux.vnet.ibm.com>\n"
     , cmd, QEMU_VERSION, QGA_VIRTIO_PATH_DEFAULT, QGA_PIDFILE_DEFAULT,
+#ifdef CONFIG_FSFREEZE
+    QGA_FSFREEZE_HOOK_DEFAULT,
+#endif
     QGA_STATEDIR_DEFAULT);
 }
 
@@ -401,6 +420,13 @@ void ga_unset_frozen(GAState *s)
     }
 }
 
+#ifdef CONFIG_FSFREEZE
+const char *ga_fsfreeze_hook(GAState *s)
+{
+    return s->fsfreeze_hook;
+}
+#endif
+
 static void become_daemon(const char *pidfile)
 {
 #ifndef _WIN32
@@ -678,10 +704,13 @@ VOID WINAPI service_main(DWORD argc, TCHAR *argv[])
 
 int main(int argc, char **argv)
 {
-    const char *sopt = "hVvdm:p:l:f:b:s:t:";
+    const char *sopt = "hVvdm:p:l:f:F:b:s:t:";
     const char *method = NULL, *path = NULL;
     const char *log_filepath = NULL;
     const char *pid_filepath = QGA_PIDFILE_DEFAULT;
+#ifdef CONFIG_FSFREEZE
+    const char *fsfreeze_hook = QGA_FSFREEZE_HOOK_DEFAULT;
+#endif
     const char *state_dir = QGA_STATEDIR_DEFAULT;
 #ifdef _WIN32
     const char *service = NULL;
@@ -691,6 +720,9 @@ int main(int argc, char **argv)
         { "version", 0, NULL, 'V' },
         { "logfile", 1, NULL, 'l' },
         { "pidfile", 1, NULL, 'f' },
+#ifdef CONFIG_FSFREEZE
+        { "fsfreeze-hook", 1, NULL, 'F' },
+#endif
         { "verbose", 0, NULL, 'v' },
         { "method", 1, NULL, 'm' },
         { "path", 1, NULL, 'p' },
@@ -723,6 +755,11 @@ int main(int argc, char **argv)
         case 'f':
             pid_filepath = optarg;
             break;
+#ifdef CONFIG_FSFREEZE
+        case 'F':
+            fsfreeze_hook = optarg;
+            break;
+#endif
         case 't':
              state_dir = optarg;
              break;
@@ -786,6 +823,9 @@ int main(int argc, char **argv)
     s = g_malloc0(sizeof(GAState));
     s->log_level = log_level;
     s->log_file = stderr;
+#ifdef CONFIG_FSFREEZE
+    s->fsfreeze_hook = fsfreeze_hook;
+#endif
     g_log_set_default_handler(ga_log, s);
     g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR);
     ga_enable_logging(s);
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 726930a..9849c10 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -396,6 +396,62 @@ GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err)
     return GUEST_FSFREEZE_STATUS_THAWED;
 }
 
+typedef enum {
+    FSFREEZE_HOOK_THAW = 0,
+    FSFREEZE_HOOK_FREEZE,
+} FsfreezeHookArg;
+
+const char *fsfreeze_hook_arg_string[] = {
+    "thaw",
+    "freeze",
+};
+
+/*
+ * Return -1 if hook is configured and exited abnormally. Otherwise return 0.
+ */
+static int execute_fsfreeze_hook(FsfreezeHookArg arg)
+{
+    int status;
+    pid_t pid, rpid;
+    const char *hook;
+    const char *arg_str = fsfreeze_hook_arg_string[arg];
+
+    hook = ga_fsfreeze_hook(ga_state);
+    if (!hook || access(hook, X_OK) != 0) {
+        return 0;
+    }
+
+    slog("executing fsfreeze hook with arg `%s'", arg_str);
+    pid = fork();
+    if (pid == 0) {
+        setsid();
+        reopen_fd_to_null(0);
+        reopen_fd_to_null(1);
+        reopen_fd_to_null(2);
+
+        execle(hook, hook, arg_str, NULL, environ);
+        _exit(EXIT_FAILURE);
+    } else if (pid < 0) {
+        slog("execution of fsfreeze hook failed: %s", strerror(errno));
+        return -1;
+    }
+
+    do {
+        rpid = waitpid(pid, &status, 0);
+    } while (rpid == -1 && errno == EINTR);
+    if (rpid == pid && WIFEXITED(status)) {
+        int st = WEXITSTATUS(status);
+        if (st) {
+            slog("fsfreeze hook failed with status %d", st);
+            return -1;
+        }
+    } else if (rpid == pid && WIFSIGNALED(status)) {
+        slog("fsfreeze hook killed by signal %d", WTERMSIG(status));
+        return -1;
+    }
+    return 0;
+}
+
 /*
  * Walk list of mounted file systems in the guest, and freeze the ones which
  * are real local file systems.
@@ -410,6 +466,13 @@ int64_t qmp_guest_fsfreeze_freeze(Error **err)
 
     slog("guest-fsfreeze called");
 
+    ret = execute_fsfreeze_hook(FSFREEZE_HOOK_FREEZE);
+    if (ret < 0) {
+        sprintf(err_msg, "execution of fsfreeze hook failed");
+        error_set(err, QERR_QGA_COMMAND_FAILED, err_msg);
+        return ret;
+    }
+
     QTAILQ_INIT(&mounts);
     ret = build_fs_mount_list(&mounts);
     if (ret < 0) {
@@ -513,6 +576,9 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
 
     ga_unset_frozen(ga_state);
     free_fs_mount_list(&mounts);
+
+    execute_fsfreeze_hook(FSFREEZE_HOOK_THAW);
+
     return i;
 }
 
diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
index 49a7abe..c6e8de0 100644
--- a/qga/guest-agent-core.h
+++ b/qga/guest-agent-core.h
@@ -34,6 +34,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);
+const char *ga_fsfreeze_hook(GAState *s);
 
 #ifndef _WIN32
 void reopen_fd_to_null(int fd);

Re: [Qemu-devel] [PATCH v4 2/2] qemu-ga: sample fsfreeze hooks

[Tomoki Sekiyama]

Adds sample hook scripts for --fsfreeze-hook option of qemu-ga.
  - fsfreeze-hook : execute scripts in fsfreeze-hook.d/
  - fsfreeze-hook.d.sample/mysql-flush.sh : quiesce MySQL before snapshot

Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com>
---
 docs/qemu-guest-agent/fsfreeze-hook                |   31 +++++++++++++
 .../fsfreeze-hook.d.sample/mysql-flush.sh          |   47 ++++++++++++++++++++
 2 files changed, 78 insertions(+)
 create mode 100755 docs/qemu-guest-agent/fsfreeze-hook
 create mode 100755 docs/qemu-guest-agent/fsfreeze-hook.d.sample/mysql-flush.sh

diff --git a/docs/qemu-guest-agent/fsfreeze-hook b/docs/qemu-guest-agent/fsfreeze-hook
new file mode 100755
index 0000000..319f68c
--- /dev/null
+++ b/docs/qemu-guest-agent/fsfreeze-hook
@@ -0,0 +1,31 @@
+#!/bin/sh
+
+# This script is executed when a guest agent receives fsfreeze-freeze and
+# fsfreeze-thaw command, if it is specified in --fsfreeze-hook (-F)
+# option of qemu-ga or placed in default path (/etc/qemu/fsfreeze-hook).
+# When the agent receives fsfreeze-freeze request, this script is issued with
+# "freeze" argument before the filesystem is freezed. And for fsfreeze-thaw
+# request, it is issued with "thaw" argument after filesystem is thawed.
+
+LOGFILE=/var/log/qga-fsfreeze-hook.log
+FSFREEZE_D=$(dirname -- "$0")/fsfreeze-hook.d
+
+# Check whether file $1 is a backup or rpm-generated file and should be ignored
+is_ignored_file() {
+    case "$1" in
+        *~ | *.bak | *.orig | *.rpmnew | *.rpmorig | *.rpmsave)
+            return 0 ;;
+    esac
+    return 1
+}
+
+# Iterate executables in directory "fsfreeze-hook.d" with the specified args
+[ ! -d "$FSFREEZE_D" ] && exit 1
+for file in "$FSFREEZE_D"/* ; do
+    is_ignored_file "$file" && continue
+    [ -x "$file" ] || continue
+    echo "$(date): execute $file $@" >>$LOGFILE
+    "$file" "$@" >>$LOGFILE 2>&1
+    STATUS=$?
+    echo "$(date): $file finished with status=$STATUS" >>$LOGFILE
+done
diff --git a/docs/qemu-guest-agent/fsfreeze-hook.d.sample/mysql-flush.sh b/docs/qemu-guest-agent/fsfreeze-hook.d.sample/mysql-flush.sh
new file mode 100755
index 0000000..e6d7998
--- /dev/null
+++ b/docs/qemu-guest-agent/fsfreeze-hook.d.sample/mysql-flush.sh
@@ -0,0 +1,47 @@
+#!/bin/sh
+
+# Flush MySQL tables to the disk before the filesystem is freezed.
+# At the same time, this keeps a read lock while the filesystem is freezed
+# in order to avoid write accesses by the other clients.
+
+MYSQL="mysql -uroot" #"-prootpassword"
+FIFO=/tmp/mysql-flush.fifo
+
+flush_and_wait() {
+    printf "FLUSH TABLES WITH READ LOCK \\G\n"
+    read < $FIFO
+    printf "UNLOCK TABLES \\G\n"
+}
+
+case "$1" in
+    freeze)
+        mkfifo $FIFO || exit 1
+        flush_and_wait | $MYSQL &
+        # wait until every block is flushed
+        while [ "$(echo 'SHOW STATUS LIKE "Key_blocks_not_flushed"' |\
+                 $MYSQL | tail -1 | cut -f 2)" -gt 0 ]; do
+            sleep 1
+        done
+        # for InnoDB, wait until every log is flushed
+        INNODB_STATUS=$(mktemp /tmp/mysql-flush.XXXXXX)
+        [ $? -ne 0 ] && exit 2
+        trap "rm -f $INNODB_STATUS" SIGINT
+        while :; do
+            printf "SHOW ENGINE INNODB STATUS \\G" | $MYSQL > $INNODB_STATUS
+            LOG_CURRENT=$(grep 'Log sequence number' $INNODB_STATUS |\
+                          tr -s ' ' | cut -d' ' -f4)
+            LOG_FLUSHED=$(grep 'Log flushed up to' $INNODB_STATUS |\
+                          tr -s ' ' | cut -d' ' -f5)
+            [ "$LOG_CURRENT" = "$LOG_FLUSHED" ] && break
+            sleep 1
+        done
+        rm -f $INNODB_STATUS
+        ;;
+
+    thaw)
+        [ ! -p $FIFO ] && exit 1
+        echo > $FIFO
+        rm -f $FIFO
+        ;;
+esac
+

Re: [Qemu-devel] [PATCH v4 1/2] qemu-ga: execute hook to quiesce the guest on fsfreeze-freeze/thaw

[Luiz Capitulino]

On Thu, 22 Nov 2012 11:15:46 +0900
Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com> wrote:

> To use the online disk snapshot for online-backup, application-level
> consistency of the snapshot image is required. However, currently the
> guest agent can provide only filesystem-level consistency, and the
> snapshot may contain dirty data, for example, incomplete transactions.
> This patch provides the opportunity to quiesce applications before
> snapshot is taken.
> 
> When the qemu-ga receives fsfreeze-freeze command, the hook script
> specified in --fsfreeze-hook option is executed with "freeze" argument
> before the filesystem is frozen. For fsfreeze-thaw command, the hook
> script is executed with "thaw" argument after the filesystem is thawed.
> 
> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com>
> ---
>  qemu-ga.c              |   42 ++++++++++++++++++++++++++++++-
>  qga/commands-posix.c   |   66 ++++++++++++++++++++++++++++++++++++++++++++++++
>  qga/guest-agent-core.h |    1 +
>  3 files changed, 108 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-ga.c b/qemu-ga.c
> index 9b59a52..240f6e2 100644
> --- a/qemu-ga.c
> +++ b/qemu-ga.c
> @@ -34,6 +34,12 @@
>  #include "qga/service-win32.h"
>  #include <windows.h>
>  #endif
> +#ifdef __linux__
> +#include <linux/fs.h>
> +#ifdef FIFREEZE
> +#define CONFIG_FSFREEZE
> +#endif
> +#endif
>  
>  #ifndef _WIN32
>  #define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent.0"
> @@ -42,6 +48,9 @@
>  #endif
>  #define QGA_STATEDIR_DEFAULT CONFIG_QEMU_LOCALSTATEDIR "/run"
>  #define QGA_PIDFILE_DEFAULT QGA_STATEDIR_DEFAULT "/qemu-ga.pid"
> +#ifdef CONFIG_FSFREEZE
> +#define QGA_FSFREEZE_HOOK_DEFAULT CONFIG_QEMU_CONFDIR "/fsfreeze-hook"
> +#endif
>  #define QGA_SENTINEL_BYTE 0xFF
>  
>  struct GAState {
> @@ -64,6 +73,9 @@ struct GAState {
>          const char *log_filepath;
>          const char *pid_filepath;
>      } deferred_options;
> +#ifdef CONFIG_FSFREEZE
> +    const char *fsfreeze_hook;
> +#endif
>  };
>  
>  struct GAState *ga_state;
> @@ -153,6 +165,10 @@ 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"
> +#ifdef CONFIG_FSFREEZE
> +"  -F, --fsfreeze-hook\n"
> +"                    specify fsfreeze hook (default is %s)\n"
> +#endif
>  "  -t, --statedir    specify dir to store state information (absolute paths\n"
>  "                    only, default is %s)\n"
>  "  -v, --verbose     log extra debugging information\n"
> @@ -167,6 +183,9 @@ static void usage(const char *cmd)
>  "\n"
>  "Report bugs to <mdroth@linux.vnet.ibm.com>\n"
>      , cmd, QEMU_VERSION, QGA_VIRTIO_PATH_DEFAULT, QGA_PIDFILE_DEFAULT,
> +#ifdef CONFIG_FSFREEZE
> +    QGA_FSFREEZE_HOOK_DEFAULT,
> +#endif
>      QGA_STATEDIR_DEFAULT);
>  }
>  
> @@ -401,6 +420,13 @@ void ga_unset_frozen(GAState *s)
>      }
>  }
>  
> +#ifdef CONFIG_FSFREEZE
> +const char *ga_fsfreeze_hook(GAState *s)
> +{

Argument can be const.

> +    return s->fsfreeze_hook;
> +}
> +#endif
> +
>  static void become_daemon(const char *pidfile)
>  {
>  #ifndef _WIN32
> @@ -678,10 +704,13 @@ VOID WINAPI service_main(DWORD argc, TCHAR *argv[])
>  
>  int main(int argc, char **argv)
>  {
> -    const char *sopt = "hVvdm:p:l:f:b:s:t:";
> +    const char *sopt = "hVvdm:p:l:f:F:b:s:t:";
>      const char *method = NULL, *path = NULL;
>      const char *log_filepath = NULL;
>      const char *pid_filepath = QGA_PIDFILE_DEFAULT;
> +#ifdef CONFIG_FSFREEZE
> +    const char *fsfreeze_hook = QGA_FSFREEZE_HOOK_DEFAULT;
> +#endif
>      const char *state_dir = QGA_STATEDIR_DEFAULT;
>  #ifdef _WIN32
>      const char *service = NULL;
> @@ -691,6 +720,9 @@ int main(int argc, char **argv)
>          { "version", 0, NULL, 'V' },
>          { "logfile", 1, NULL, 'l' },
>          { "pidfile", 1, NULL, 'f' },
> +#ifdef CONFIG_FSFREEZE
> +        { "fsfreeze-hook", 1, NULL, 'F' },
> +#endif
>          { "verbose", 0, NULL, 'v' },
>          { "method", 1, NULL, 'm' },
>          { "path", 1, NULL, 'p' },
> @@ -723,6 +755,11 @@ int main(int argc, char **argv)
>          case 'f':
>              pid_filepath = optarg;
>              break;
> +#ifdef CONFIG_FSFREEZE
> +        case 'F':
> +            fsfreeze_hook = optarg;
> +            break;
> +#endif
>          case 't':
>               state_dir = optarg;
>               break;
> @@ -786,6 +823,9 @@ int main(int argc, char **argv)
>      s = g_malloc0(sizeof(GAState));
>      s->log_level = log_level;
>      s->log_file = stderr;
> +#ifdef CONFIG_FSFREEZE
> +    s->fsfreeze_hook = fsfreeze_hook;
> +#endif
>      g_log_set_default_handler(ga_log, s);
>      g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR);
>      ga_enable_logging(s);
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 726930a..9849c10 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -396,6 +396,62 @@ GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err)
>      return GUEST_FSFREEZE_STATUS_THAWED;
>  }
>  
> +typedef enum {
> +    FSFREEZE_HOOK_THAW = 0,
> +    FSFREEZE_HOOK_FREEZE,
> +} FsfreezeHookArg;
> +
> +const char *fsfreeze_hook_arg_string[] = {
> +    "thaw",
> +    "freeze",
> +};
> +
> +/*
> + * Return -1 if hook is configured and exited abnormally. Otherwise return 0.
> + */
> +static int execute_fsfreeze_hook(FsfreezeHookArg arg)

Why don't you allow the argument to be the execle()'s "arg" string, as it's
only used by execle() itself? This way you can drop the enum and the array
pointer.

> +{
> +    int status;
> +    pid_t pid, rpid;
> +    const char *hook;
> +    const char *arg_str = fsfreeze_hook_arg_string[arg];
> +
> +    hook = ga_fsfreeze_hook(ga_state);
> +    if (!hook || access(hook, X_OK) != 0) {

Can hook be NULL? If it can't, then this should be an assert(), although
I think it would be nice to allow the hook to be disabled.

Also, you shouldn't silently fail if the execution bit is not set. At a
minimum you should emit an warning. I guess I would return an error though.

> +        return 0;
> +    }
> +
> +    slog("executing fsfreeze hook with arg `%s'", arg_str);
> +    pid = fork();
> +    if (pid == 0) {
> +        setsid();
> +        reopen_fd_to_null(0);
> +        reopen_fd_to_null(1);
> +        reopen_fd_to_null(2);
> +
> +        execle(hook, hook, arg_str, NULL, environ);
> +        _exit(EXIT_FAILURE);
> +    } else if (pid < 0) {
> +        slog("execution of fsfreeze hook failed: %s", strerror(errno));
> +        return -1;

Please, return a good error here. You could change this function
to take an Error object and do:

error_setg_errno(err, errno, "failed to wait for child (pid: %d)", pid);

> +    }
> +
> +    do {
> +        rpid = waitpid(pid, &status, 0);
> +    } while (rpid == -1 && errno == EINTR);

I was refactoring error messages & handling in qemu-ga and added a
function called ga_wait_child(), so that we avoid duplicating the
loop above and also have good error reporting.

You can check it here and grab it if you want:

http://repo.or.cz/w/qemu/qmp-unstable.git/commitdiff/036e1cb5a3d9f8d797c7220e049fa1e0042df4dc

Also note that you're ignoring waitpid() failures != EINTR.

> +    if (rpid == pid && WIFEXITED(status)) {
> +        int st = WEXITSTATUS(status);
> +        if (st) {
> +            slog("fsfreeze hook failed with status %d", st);

We need this on a error message back to the client otherwise it will
be harder to debug freeze failures (ie. use error_setg() here).

> +            return -1;
> +        }
> +    } else if (rpid == pid && WIFSIGNALED(status)) {
> +        slog("fsfreeze hook killed by signal %d", WTERMSIG(status));
> +        return -1;
> +    }

If pid > 0 and waitpid() didn't fail, then rpid == pid.

> +    return 0;
> +}
> +
>  /*
>   * Walk list of mounted file systems in the guest, and freeze the ones which
>   * are real local file systems.
> @@ -410,6 +466,13 @@ int64_t qmp_guest_fsfreeze_freeze(Error **err)
>  
>      slog("guest-fsfreeze called");
>  
> +    ret = execute_fsfreeze_hook(FSFREEZE_HOOK_FREEZE);
> +    if (ret < 0) {
> +        sprintf(err_msg, "execution of fsfreeze hook failed");
> +        error_set(err, QERR_QGA_COMMAND_FAILED, err_msg);
> +        return ret;

If you propagate errors from execute_fsfreeze_hook() you can drop all this
and fix the generic error message.

> +    }
> +
>      QTAILQ_INIT(&mounts);
>      ret = build_fs_mount_list(&mounts);
>      if (ret < 0) {
> @@ -513,6 +576,9 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
>  
>      ga_unset_frozen(ga_state);
>      free_fs_mount_list(&mounts);
> +
> +    execute_fsfreeze_hook(FSFREEZE_HOOK_THAW);
> +
>      return i;
>  }
>  
> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> index 49a7abe..c6e8de0 100644
> --- a/qga/guest-agent-core.h
> +++ b/qga/guest-agent-core.h
> @@ -34,6 +34,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);
> +const char *ga_fsfreeze_hook(GAState *s);
>  
>  #ifndef _WIN32
>  void reopen_fd_to_null(int fd);
> 
> 

Re: [Qemu-devel] [PATCH v4 2/2] qemu-ga: sample fsfreeze hooks

[Luiz Capitulino]

On Thu, 22 Nov 2012 11:15:49 +0900
Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com> wrote:

> Adds sample hook scripts for --fsfreeze-hook option of qemu-ga.
>   - fsfreeze-hook : execute scripts in fsfreeze-hook.d/
>   - fsfreeze-hook.d.sample/mysql-flush.sh : quiesce MySQL before snapshot
> 
> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com>
> ---
>  docs/qemu-guest-agent/fsfreeze-hook                |   31 +++++++++++++
>  .../fsfreeze-hook.d.sample/mysql-flush.sh          |   47 ++++++++++++++++++++
>  2 files changed, 78 insertions(+)
>  create mode 100755 docs/qemu-guest-agent/fsfreeze-hook
>  create mode 100755 docs/qemu-guest-agent/fsfreeze-hook.d.sample/mysql-flush.sh
> 
> diff --git a/docs/qemu-guest-agent/fsfreeze-hook b/docs/qemu-guest-agent/fsfreeze-hook
> new file mode 100755
> index 0000000..319f68c
> --- /dev/null
> +++ b/docs/qemu-guest-agent/fsfreeze-hook
> @@ -0,0 +1,31 @@
> +#!/bin/sh
> +
> +# This script is executed when a guest agent receives fsfreeze-freeze and
> +# fsfreeze-thaw command, if it is specified in --fsfreeze-hook (-F)
> +# option of qemu-ga or placed in default path (/etc/qemu/fsfreeze-hook).
> +# When the agent receives fsfreeze-freeze request, this script is issued with
> +# "freeze" argument before the filesystem is freezed. And for fsfreeze-thaw
> +# request, it is issued with "thaw" argument after filesystem is thawed.
> +
> +LOGFILE=/var/log/qga-fsfreeze-hook.log
> +FSFREEZE_D=$(dirname -- "$0")/fsfreeze-hook.d
> +
> +# Check whether file $1 is a backup or rpm-generated file and should be ignored
> +is_ignored_file() {
> +    case "$1" in
> +        *~ | *.bak | *.orig | *.rpmnew | *.rpmorig | *.rpmsave)
> +            return 0 ;;
> +    esac
> +    return 1
> +}
> +
> +# Iterate executables in directory "fsfreeze-hook.d" with the specified args
> +[ ! -d "$FSFREEZE_D" ] && exit 1
> +for file in "$FSFREEZE_D"/* ; do
> +    is_ignored_file "$file" && continue
> +    [ -x "$file" ] || continue
> +    echo "$(date): execute $file $@" >>$LOGFILE
> +    "$file" "$@" >>$LOGFILE 2>&1
> +    STATUS=$?
> +    echo "$(date): $file finished with status=$STATUS" >>$LOGFILE
> +done

execute_fsfreeze_hook() will fail the freeze process if this script fails. Two
comments:

 1. Do we want to fail the freeze process if one of the sub-scripts fails?
    If yes, then we have to exit 1 in the first failure

 2. The exit status of the script will echo's exit status. I doubt we want that

> diff --git a/docs/qemu-guest-agent/fsfreeze-hook.d.sample/mysql-flush.sh b/docs/qemu-guest-agent/fsfreeze-hook.d.sample/mysql-flush.sh
> new file mode 100755
> index 0000000..e6d7998
> --- /dev/null
> +++ b/docs/qemu-guest-agent/fsfreeze-hook.d.sample/mysql-flush.sh
> @@ -0,0 +1,47 @@
> +#!/bin/sh
> +
> +# Flush MySQL tables to the disk before the filesystem is freezed.
> +# At the same time, this keeps a read lock while the filesystem is freezed
> +# in order to avoid write accesses by the other clients.
> +
> +MYSQL="mysql -uroot" #"-prootpassword"
> +FIFO=/tmp/mysql-flush.fifo
> +
> +flush_and_wait() {
> +    printf "FLUSH TABLES WITH READ LOCK \\G\n"
> +    read < $FIFO
> +    printf "UNLOCK TABLES \\G\n"
> +}
> +
> +case "$1" in
> +    freeze)
> +        mkfifo $FIFO || exit 1
> +        flush_and_wait | $MYSQL &

Honest question: what happens if I don't have mysql installed?

> +        # wait until every block is flushed
> +        while [ "$(echo 'SHOW STATUS LIKE "Key_blocks_not_flushed"' |\
> +                 $MYSQL | tail -1 | cut -f 2)" -gt 0 ]; do
> +            sleep 1
> +        done
> +        # for InnoDB, wait until every log is flushed
> +        INNODB_STATUS=$(mktemp /tmp/mysql-flush.XXXXXX)
> +        [ $? -ne 0 ] && exit 2
> +        trap "rm -f $INNODB_STATUS" SIGINT
> +        while :; do
> +            printf "SHOW ENGINE INNODB STATUS \\G" | $MYSQL > $INNODB_STATUS
> +            LOG_CURRENT=$(grep 'Log sequence number' $INNODB_STATUS |\
> +                          tr -s ' ' | cut -d' ' -f4)
> +            LOG_FLUSHED=$(grep 'Log flushed up to' $INNODB_STATUS |\
> +                          tr -s ' ' | cut -d' ' -f5)
> +            [ "$LOG_CURRENT" = "$LOG_FLUSHED" ] && break
> +            sleep 1
> +        done
> +        rm -f $INNODB_STATUS
> +        ;;
> +
> +    thaw)
> +        [ ! -p $FIFO ] && exit 1
> +        echo > $FIFO
> +        rm -f $FIFO
> +        ;;
> +esac
> +
> 
> 

Re: [Qemu-devel] [PATCH v4 1/2] qemu-ga: execute hook to quiesce the guest on fsfreeze-freeze/thaw

[Tomoki Sekiyama]

Hi, thanks for your review.

On 2012/11/23 0:52, Luiz Capitulino wrote:
> On Thu, 22 Nov 2012 11:15:46 +0900
> Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com> wrote:
> 
>> To use the online disk snapshot for online-backup, application-level
>> consistency of the snapshot image is required. However, currently the
>> guest agent can provide only filesystem-level consistency, and the
>> snapshot may contain dirty data, for example, incomplete transactions.
>> This patch provides the opportunity to quiesce applications before
>> snapshot is taken.
>>
>> When the qemu-ga receives fsfreeze-freeze command, the hook script
>> specified in --fsfreeze-hook option is executed with "freeze" argument
>> before the filesystem is frozen. For fsfreeze-thaw command, the hook
>> script is executed with "thaw" argument after the filesystem is thawed.
>>
>> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com>
>> ---
>>  qemu-ga.c              |   42 ++++++++++++++++++++++++++++++-
>>  qga/commands-posix.c   |   66 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  qga/guest-agent-core.h |    1 +
>>  3 files changed, 108 insertions(+), 1 deletion(-)
<snip>
>> @@ -401,6 +420,13 @@ void ga_unset_frozen(GAState *s)
>>      }
>>  }
>>  
>> +#ifdef CONFIG_FSFREEZE
>> +const char *ga_fsfreeze_hook(GAState *s)
>> +{
> 
> Argument can be const.

OK, should be fixed at next patch.

>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -396,6 +396,62 @@ GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err)
>>      return GUEST_FSFREEZE_STATUS_THAWED;
>>  }
>>  
>> +typedef enum {
>> +    FSFREEZE_HOOK_THAW = 0,
>> +    FSFREEZE_HOOK_FREEZE,
>> +} FsfreezeHookArg;
>> +
>> +const char *fsfreeze_hook_arg_string[] = {
>> +    "thaw",
>> +    "freeze",
>> +};
>> +
>> +/*
>> + * Return -1 if hook is configured and exited abnormally. Otherwise return 0.
>> + */
>> +static int execute_fsfreeze_hook(FsfreezeHookArg arg)
> 
> Why don't you allow the argument to be the execle()'s "arg" string, as it's
> only used by execle() itself? This way you can drop the enum and the array
> pointer.

This is based on Michael's comment that enum style is better in terms of
documenting the interface.

>> +{
>> +    int status;
>> +    pid_t pid, rpid;
>> +    const char *hook;
>> +    const char *arg_str = fsfreeze_hook_arg_string[arg];
>> +
>> +    hook = ga_fsfreeze_hook(ga_state);
>> +    if (!hook || access(hook, X_OK) != 0) {
> 
> Can hook be NULL? If it can't, then this should be an assert(), 
> although I think it would be nice to allow the hook to be disabled.

Currently whether the hook is executed is determined by whether it is
installed at default path (or specified path by the option).
I think it is easier to configure, but it could make the hook disabled
by default and enable it only if --fsfreeze-hook option is specified.
How do you think of that?

> Also, you shouldn't silently fail if the execution bit is not set. At a
> minimum you should emit an warning. I guess I would return an error though.

I agree.

>> +        return 0;
>> +    }
>> +
>> +    slog("executing fsfreeze hook with arg `%s'", arg_str);
>> +    pid = fork();
>> +    if (pid == 0) {
>> +        setsid();
>> +        reopen_fd_to_null(0);
>> +        reopen_fd_to_null(1);
>> +        reopen_fd_to_null(2);
>> +
>> +        execle(hook, hook, arg_str, NULL, environ);
>> +        _exit(EXIT_FAILURE);
>> +    } else if (pid < 0) {
>> +        slog("execution of fsfreeze hook failed: %s", strerror(errno));
>> +        return -1;
> 
> Please, return a good error here. You could change this function
> to take an Error object and do:
> 
> error_setg_errno(err, errno, "failed to wait for child (pid: %d)", pid);

OK, I will try this.

>> +    }
>> +
>> +    do {
>> +        rpid = waitpid(pid, &status, 0);
>> +    } while (rpid == -1 && errno == EINTR);
> 
> I was refactoring error messages & handling in qemu-ga and added a
> function called ga_wait_child(), so that we avoid duplicating the
> loop above and also have good error reporting.
> 
> You can check it here and grab it if you want:
> 
> http://repo.or.cz/w/qemu/qmp-unstable.git/commitdiff/036e1cb5a3d9f8d797c7220e049fa1e0042df4dc

Thanks for the information. I will check your patch.
I also inspect the issues below at the same time.

> Also note that you're ignoring waitpid() failures != EINTR.
>
>> +    if (rpid == pid && WIFEXITED(status)) {
>> +        int st = WEXITSTATUS(status);
>> +        if (st) {
>> +            slog("fsfreeze hook failed with status %d", st);
> 
> We need this on a error message back to the client otherwise it will
> be harder to debug freeze failures (ie. use error_setg() here).
>
>> +            return -1;
>> +        }
>> +    } else if (rpid == pid && WIFSIGNALED(status)) {
>> +        slog("fsfreeze hook killed by signal %d", WTERMSIG(status));
>> +        return -1;
>> +    }
> 
> If pid > 0 and waitpid() didn't fail, then rpid == pid.
> 
>> +    return 0;
>> +}
>> +
>>  /*
>>   * Walk list of mounted file systems in the guest, and freeze the ones which
>>   * are real local file systems.
>> @@ -410,6 +466,13 @@ int64_t qmp_guest_fsfreeze_freeze(Error **err)
>>  
>>      slog("guest-fsfreeze called");
>>  
>> +    ret = execute_fsfreeze_hook(FSFREEZE_HOOK_FREEZE);
>> +    if (ret < 0) {
>> +        sprintf(err_msg, "execution of fsfreeze hook failed");
>> +        error_set(err, QERR_QGA_COMMAND_FAILED, err_msg);
>> +        return ret;
> 
> If you propagate errors from execute_fsfreeze_hook() you can drop all this
> and fix the generic error message.

Thanks,
-- 
Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com>
Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory

Re: [Qemu-devel] [PATCH v4 2/2] qemu-ga: sample fsfreeze hooks

[Tomoki Sekiyama]

On 2012/11/23 1:03, Luiz Capitulino wrote:
> On Thu, 22 Nov 2012 11:15:49 +0900
> Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com> wrote:
> 
>> Adds sample hook scripts for --fsfreeze-hook option of qemu-ga.
>>   - fsfreeze-hook : execute scripts in fsfreeze-hook.d/
>>   - fsfreeze-hook.d.sample/mysql-flush.sh : quiesce MySQL before snapshot
<snip>
>> +# Iterate executables in directory "fsfreeze-hook.d" with the specified args
>> +[ ! -d "$FSFREEZE_D" ] && exit 1
>> +for file in "$FSFREEZE_D"/* ; do
>> +    is_ignored_file "$file" && continue
>> +    [ -x "$file" ] || continue
>> +    echo "$(date): execute $file $@" >>$LOGFILE
>> +    "$file" "$@" >>$LOGFILE 2>&1
>> +    STATUS=$?
>> +    echo "$(date): $file finished with status=$STATUS" >>$LOGFILE
>> +done
> 
> execute_fsfreeze_hook() will fail the freeze process if this script fails. Two
> comments:
> 
>  1. Do we want to fail the freeze process if one of the sub-scripts fails?
>     If yes, then we have to exit 1 in the first failure

I originally thought the hooks are optional; even they failed, filesystem-level
consistency are still kept in the snapshot.
However, if we are going to fail fsfreeze process by one of sub-scripts'
failure, we also need to notify scripts which already succeeded to pre-freeze
to thaw (or abort) freezing, before exit 1 here.

>  2. The exit status of the script will echo's exit status. I doubt we want that

Do you mean $STATUS (not $status) is specialized in some shell environments?

>> diff --git a/docs/qemu-guest-agent/fsfreeze-hook.d.sample/mysql-flush.sh b/docs/qemu-guest-agent/fsfreeze-hook.d.sample/mysql-flush.sh
>> new file mode 100755
>> index 0000000..e6d7998
>> --- /dev/null
>> +++ b/docs/qemu-guest-agent/fsfreeze-hook.d.sample/mysql-flush.sh
>> @@ -0,0 +1,47 @@
>> +#!/bin/sh
>> +
>> +# Flush MySQL tables to the disk before the filesystem is freezed.
>> +# At the same time, this keeps a read lock while the filesystem is freezed
>> +# in order to avoid write accesses by the other clients.
>> +
>> +MYSQL="mysql -uroot" #"-prootpassword"
>> +FIFO=/tmp/mysql-flush.fifo
>> +
>> +flush_and_wait() {
>> +    printf "FLUSH TABLES WITH READ LOCK \\G\n"
>> +    read < $FIFO
>> +    printf "UNLOCK TABLES \\G\n"
>> +}
>> +
>> +case "$1" in
>> +    freeze)
>> +        mkfifo $FIFO || exit 1
>> +        flush_and_wait | $MYSQL &
> 
> Honest question: what happens if I don't have mysql installed?

Ah OK, availability of mysql should be checked in advance.

Thanks,
-- 
Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com>
Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory

Re: [Qemu-devel] [PATCH v4 1/2] qemu-ga: execute hook to quiesce the guest on fsfreeze-freeze/thaw

[Luiz Capitulino]

On Mon, 26 Nov 2012 20:49:08 +0900
Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com> wrote:

> Hi, thanks for your review.
> 
> On 2012/11/23 0:52, Luiz Capitulino wrote:
> > On Thu, 22 Nov 2012 11:15:46 +0900
> > Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com> wrote:
> > 
> >> To use the online disk snapshot for online-backup, application-level
> >> consistency of the snapshot image is required. However, currently the
> >> guest agent can provide only filesystem-level consistency, and the
> >> snapshot may contain dirty data, for example, incomplete transactions.
> >> This patch provides the opportunity to quiesce applications before
> >> snapshot is taken.
> >>
> >> When the qemu-ga receives fsfreeze-freeze command, the hook script
> >> specified in --fsfreeze-hook option is executed with "freeze" argument
> >> before the filesystem is frozen. For fsfreeze-thaw command, the hook
> >> script is executed with "thaw" argument after the filesystem is thawed.
> >>
> >> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com>
> >> ---
> >>  qemu-ga.c              |   42 ++++++++++++++++++++++++++++++-
> >>  qga/commands-posix.c   |   66 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>  qga/guest-agent-core.h |    1 +
> >>  3 files changed, 108 insertions(+), 1 deletion(-)
> <snip>
> >> @@ -401,6 +420,13 @@ void ga_unset_frozen(GAState *s)
> >>      }
> >>  }
> >>  
> >> +#ifdef CONFIG_FSFREEZE
> >> +const char *ga_fsfreeze_hook(GAState *s)
> >> +{
> > 
> > Argument can be const.
> 
> OK, should be fixed at next patch.
> 
> >> --- a/qga/commands-posix.c
> >> +++ b/qga/commands-posix.c
> >> @@ -396,6 +396,62 @@ GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err)
> >>      return GUEST_FSFREEZE_STATUS_THAWED;
> >>  }
> >>  
> >> +typedef enum {
> >> +    FSFREEZE_HOOK_THAW = 0,
> >> +    FSFREEZE_HOOK_FREEZE,
> >> +} FsfreezeHookArg;
> >> +
> >> +const char *fsfreeze_hook_arg_string[] = {
> >> +    "thaw",
> >> +    "freeze",
> >> +};
> >> +
> >> +/*
> >> + * Return -1 if hook is configured and exited abnormally. Otherwise return 0.
> >> + */
> >> +static int execute_fsfreeze_hook(FsfreezeHookArg arg)
> > 
> > Why don't you allow the argument to be the execle()'s "arg" string, as it's
> > only used by execle() itself? This way you can drop the enum and the array
> > pointer.
> 
> This is based on Michael's comment that enum style is better in terms of
> documenting the interface.

Well, I don't fully agree but I'm not strong about this either.

> >> +{
> >> +    int status;
> >> +    pid_t pid, rpid;
> >> +    const char *hook;
> >> +    const char *arg_str = fsfreeze_hook_arg_string[arg];
> >> +
> >> +    hook = ga_fsfreeze_hook(ga_state);
> >> +    if (!hook || access(hook, X_OK) != 0) {
> > 
> > Can hook be NULL? If it can't, then this should be an assert(), 
> > although I think it would be nice to allow the hook to be disabled.
> 
> Currently whether the hook is executed is determined by whether it is
> installed at default path (or specified path by the option).

Yes.

> I think it is easier to configure, but it could make the hook disabled
> by default and enable it only if --fsfreeze-hook option is specified.
> How do you think of that?

I agree that the way you coded it makes it easier to the user, just
two comments:

 1. As I noted below, you should issue a warning if the hook file
    doesn't exist or doesn't have the execution bit set

 2. There should be a way to disable it at the command-line. Tap scripts
    for example can be disabled by specifying "no" as the script path, like
	script=no. We could do that for qemu-ga too.

> 
> > Also, you shouldn't silently fail if the execution bit is not set. At a
> > minimum you should emit an warning. I guess I would return an error though.
> 
> I agree.
> 
> >> +        return 0;
> >> +    }
> >> +
> >> +    slog("executing fsfreeze hook with arg `%s'", arg_str);
> >> +    pid = fork();
> >> +    if (pid == 0) {
> >> +        setsid();
> >> +        reopen_fd_to_null(0);
> >> +        reopen_fd_to_null(1);
> >> +        reopen_fd_to_null(2);
> >> +
> >> +        execle(hook, hook, arg_str, NULL, environ);
> >> +        _exit(EXIT_FAILURE);
> >> +    } else if (pid < 0) {
> >> +        slog("execution of fsfreeze hook failed: %s", strerror(errno));
> >> +        return -1;
> > 
> > Please, return a good error here. You could change this function
> > to take an Error object and do:
> > 
> > error_setg_errno(err, errno, "failed to wait for child (pid: %d)", pid);
> 
> OK, I will try this.
> 
> >> +    }
> >> +
> >> +    do {
> >> +        rpid = waitpid(pid, &status, 0);
> >> +    } while (rpid == -1 && errno == EINTR);
> > 
> > I was refactoring error messages & handling in qemu-ga and added a
> > function called ga_wait_child(), so that we avoid duplicating the
> > loop above and also have good error reporting.
> > 
> > You can check it here and grab it if you want:
> > 
> > http://repo.or.cz/w/qemu/qmp-unstable.git/commitdiff/036e1cb5a3d9f8d797c7220e049fa1e0042df4dc
> 
> Thanks for the information. I will check your patch.
> I also inspect the issues below at the same time.

Well, I'm going to rebase this series and will probably post it later
today or tomorrow. So, you could rebase on top of it or just forget about
my comment above so that we avoid conflicts.

> 
> > Also note that you're ignoring waitpid() failures != EINTR.
> >
> >> +    if (rpid == pid && WIFEXITED(status)) {
> >> +        int st = WEXITSTATUS(status);
> >> +        if (st) {
> >> +            slog("fsfreeze hook failed with status %d", st);
> > 
> > We need this on a error message back to the client otherwise it will
> > be harder to debug freeze failures (ie. use error_setg() here).
> >
> >> +            return -1;
> >> +        }
> >> +    } else if (rpid == pid && WIFSIGNALED(status)) {
> >> +        slog("fsfreeze hook killed by signal %d", WTERMSIG(status));
> >> +        return -1;
> >> +    }
> > 
> > If pid > 0 and waitpid() didn't fail, then rpid == pid.
> > 
> >> +    return 0;
> >> +}
> >> +
> >>  /*
> >>   * Walk list of mounted file systems in the guest, and freeze the ones which
> >>   * are real local file systems.
> >> @@ -410,6 +466,13 @@ int64_t qmp_guest_fsfreeze_freeze(Error **err)
> >>  
> >>      slog("guest-fsfreeze called");
> >>  
> >> +    ret = execute_fsfreeze_hook(FSFREEZE_HOOK_FREEZE);
> >> +    if (ret < 0) {
> >> +        sprintf(err_msg, "execution of fsfreeze hook failed");
> >> +        error_set(err, QERR_QGA_COMMAND_FAILED, err_msg);
> >> +        return ret;
> > 
> > If you propagate errors from execute_fsfreeze_hook() you can drop all this
> > and fix the generic error message.
> 
> Thanks,

Re: [Qemu-devel] [PATCH v4 2/2] qemu-ga: sample fsfreeze hooks

[Luiz Capitulino]

On Mon, 26 Nov 2012 20:49:17 +0900
Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com> wrote:

> On 2012/11/23 1:03, Luiz Capitulino wrote:
> > On Thu, 22 Nov 2012 11:15:49 +0900
> > Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com> wrote:
> > 
> >> Adds sample hook scripts for --fsfreeze-hook option of qemu-ga.
> >>   - fsfreeze-hook : execute scripts in fsfreeze-hook.d/
> >>   - fsfreeze-hook.d.sample/mysql-flush.sh : quiesce MySQL before snapshot
> <snip>
> >> +# Iterate executables in directory "fsfreeze-hook.d" with the specified args
> >> +[ ! -d "$FSFREEZE_D" ] && exit 1
> >> +for file in "$FSFREEZE_D"/* ; do
> >> +    is_ignored_file "$file" && continue
> >> +    [ -x "$file" ] || continue
> >> +    echo "$(date): execute $file $@" >>$LOGFILE
> >> +    "$file" "$@" >>$LOGFILE 2>&1
> >> +    STATUS=$?
> >> +    echo "$(date): $file finished with status=$STATUS" >>$LOGFILE
> >> +done
> > 
> > execute_fsfreeze_hook() will fail the freeze process if this script fails. Two
> > comments:
> > 
> >  1. Do we want to fail the freeze process if one of the sub-scripts fails?
> >     If yes, then we have to exit 1 in the first failure
> 
> I originally thought the hooks are optional; even they failed, filesystem-level
> consistency are still kept in the snapshot.
> However, if we are going to fail fsfreeze process by one of sub-scripts'
> failure, we also need to notify scripts which already succeeded to pre-freeze
> to thaw (or abort) freezing, before exit 1 here.

Right, which makes things more complex. I vote for doing it the simpler way
for now then, which is to ignore subscripts exit status. But then you should
add exit 0 after the loop to avoid this:

> >  2. The exit status of the script will echo's exit status. I doubt we want that
> 
> Do you mean $STATUS (not $status) is specialized in some shell environments?

No, what I meant is that (afaik) when the script finishes, its return status to
qemu-ga is actually going to be the latest call echo exit status because it's
the last command executed in the loop. If echo fails (say no space) then
the script will fail.

We either, ignore any failures in qemu-ga itself (although we should at least
print a warning there) and/or add exit 0 as the last line of the script.

> 
> >> diff --git a/docs/qemu-guest-agent/fsfreeze-hook.d.sample/mysql-flush.sh b/docs/qemu-guest-agent/fsfreeze-hook.d.sample/mysql-flush.sh
> >> new file mode 100755
> >> index 0000000..e6d7998
> >> --- /dev/null
> >> +++ b/docs/qemu-guest-agent/fsfreeze-hook.d.sample/mysql-flush.sh
> >> @@ -0,0 +1,47 @@
> >> +#!/bin/sh
> >> +
> >> +# Flush MySQL tables to the disk before the filesystem is freezed.
> >> +# At the same time, this keeps a read lock while the filesystem is freezed
> >> +# in order to avoid write accesses by the other clients.
> >> +
> >> +MYSQL="mysql -uroot" #"-prootpassword"
> >> +FIFO=/tmp/mysql-flush.fifo
> >> +
> >> +flush_and_wait() {
> >> +    printf "FLUSH TABLES WITH READ LOCK \\G\n"
> >> +    read < $FIFO
> >> +    printf "UNLOCK TABLES \\G\n"
> >> +}
> >> +
> >> +case "$1" in
> >> +    freeze)
> >> +        mkfifo $FIFO || exit 1
> >> +        flush_and_wait | $MYSQL &
> > 
> > Honest question: what happens if I don't have mysql installed?
> 
> Ah OK, availability of mysql should be checked in advance.
> 
> Thanks,

Re: [Qemu-devel] [PATCH v4 2/2] qemu-ga: sample fsfreeze hooks

[Tomoki Sekiyama]

On 2012/11/26 21:40, Luiz Capitulino wrote:
> On Mon, 26 Nov 2012 20:49:17 +0900
> Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com> wrote:
> 
>> On 2012/11/23 1:03, Luiz Capitulino wrote:
>>> On Thu, 22 Nov 2012 11:15:49 +0900
>>> Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com> wrote:
>>>
>>>> Adds sample hook scripts for --fsfreeze-hook option of qemu-ga.
>>>>   - fsfreeze-hook : execute scripts in fsfreeze-hook.d/
>>>>   - fsfreeze-hook.d.sample/mysql-flush.sh : quiesce MySQL before snapshot
>> <snip>
>>>> +# Iterate executables in directory "fsfreeze-hook.d" with the specified args
>>>> +[ ! -d "$FSFREEZE_D" ] && exit 1
>>>> +for file in "$FSFREEZE_D"/* ; do
>>>> +    is_ignored_file "$file" && continue
>>>> +    [ -x "$file" ] || continue
>>>> +    echo "$(date): execute $file $@" >>$LOGFILE
>>>> +    "$file" "$@" >>$LOGFILE 2>&1
>>>> +    STATUS=$?
>>>> +    echo "$(date): $file finished with status=$STATUS" >>$LOGFILE
>>>> +done
>>>
>>> execute_fsfreeze_hook() will fail the freeze process if this script fails. Two
>>> comments:
>>>
>>>  1. Do we want to fail the freeze process if one of the sub-scripts fails?
>>>     If yes, then we have to exit 1 in the first failure
>>
>> I originally thought the hooks are optional; even they failed, filesystem-level
>> consistency are still kept in the snapshot.
>> However, if we are going to fail fsfreeze process by one of sub-scripts'
>> failure, we also need to notify scripts which already succeeded to pre-freeze
>> to thaw (or abort) freezing, before exit 1 here.
> 
> Right, which makes things more complex. I vote for doing it the simpler way
> for now then, which is to ignore subscripts exit status. But then you should
> add exit 0 after the loop to avoid this:

I agree this way.

>>>  2. The exit status of the script will echo's exit status. I doubt we want that
>>
>> Do you mean $STATUS (not $status) is specialized in some shell environments?
> 
> No, what I meant is that (afaik) when the script finishes, its return status to
> qemu-ga is actually going to be the latest call echo exit status because it's
> the last command executed in the loop. If echo fails (say no space) then
> the script will fail.
> 
> We either, ignore any failures in qemu-ga itself (although we should at least
> print a warning there) and/or add exit 0 as the last line of the script.

Now I got the point. I will fix this at the next version.

Thanks,
-- 
Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com>
Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory