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

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

[Tomoki Sekiyama]

Hi,

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

*Changes from v6: ( http://patchwork.ozlabs.org/patch/202959/ )
 1/2: Improved an error message on failure of access(2) to fsfreeze-hook.
 2/2: Moved hook scripts from docs/ to scripts/.
      Move fsfreeze.d.sample/flush-mysql.sh to fsfreeze.d/flush-mysql.sh.sample.
      fsfreeze-hook now ignores fsfreeze.d/*.sample.

This patchset depends on Luiz Capitulino's patchset to improve error
handling, which is available at:
https://lists.gnu.org/archive/html/qemu-devel/2012-11/msg03016.html
---
Tomoki Sekiyama (2):
      qemu-ga: execute hook to quiesce the guest on fsfreeze-freeze/thaw
      qemu-ga: sample fsfreeze hooks

 .gitignore                                         |    1 
 Makefile                                           |    2 -
 qemu-ga.c                                          |   42 ++++++++++++
 qga/commands-posix.c                               |   69 ++++++++++++++++++++
 qga/guest-agent-core.h                             |    1 
 scripts/qemu-guest-agent/fsfreeze-hook             |   33 ++++++++++
 .../fsfreeze-hook.d/mysql-flush.sh.sample          |   55 ++++++++++++++++
 7 files changed, 201 insertions(+), 2 deletions(-)
 create mode 100755 scripts/qemu-guest-agent/fsfreeze-hook
 create mode 100755 scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample

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

Re: [Qemu-devel] [PATCH v7 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.

If --fsfreeze-hook option is specified, the hook is executed with
"freeze" argument before the filesystem is frozen by fsfreeze-freeze
command. As for fsfreeze-thaw command, the hook is executed with "thaw"
argument after the filesystem is thawed.

This patch depends on patchset to improve error reporting by Luiz Capitulino:
  http://lists.gnu.org/archive/html/qemu-devel/2012-11/msg03016.html

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

diff --git a/qemu-ga.c b/qemu-ga.c
index 9b59a52..53ce462 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"
+"                    enable 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 = NULL;
+#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", 2, 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 ? optarg : QGA_FSFREEZE_HOOK_DEFAULT;
+            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 2591a3c..f7c0d9a 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -410,6 +410,66 @@ static void build_fs_mount_list(FsMountList *mounts, Error **err)
 
 #if defined(CONFIG_FSFREEZE)
 
+typedef enum {
+    FSFREEZE_HOOK_THAW = 0,
+    FSFREEZE_HOOK_FREEZE,
+} FsfreezeHookArg;
+
+const char *fsfreeze_hook_arg_string[] = {
+    "thaw",
+    "freeze",
+};
+
+static void execute_fsfreeze_hook(FsfreezeHookArg arg, Error **err)
+{
+    int status;
+    pid_t pid;
+    const char *hook;
+    const char *arg_str = fsfreeze_hook_arg_string[arg];
+    Error *local_err = NULL;
+
+    hook = ga_fsfreeze_hook(ga_state);
+    if (!hook) {
+        return;
+    }
+    if (access(hook, X_OK) != 0) {
+        error_setg_errno(err, errno, "can't access fsfreeze hook '%s'", hook);
+        return;
+    }
+
+    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) {
+        error_setg_errno(err, errno, "failed to create child process");
+        return;
+    }
+
+    ga_wait_child(pid, &status, &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(err, local_err);
+        return;
+    }
+
+    if (!WIFEXITED(status)) {
+        error_setg(err, "fsfreeze hook has terminated abnormally");
+        return;
+    }
+
+    status = WEXITSTATUS(status);
+    if (status) {
+        error_setg(err, "fsfreeze hook has failed with status %d", status);
+        return;
+    }
+}
+
 /*
  * Return status of freeze/thaw
  */
@@ -436,6 +496,12 @@ int64_t qmp_guest_fsfreeze_freeze(Error **err)
 
     slog("guest-fsfreeze called");
 
+    execute_fsfreeze_hook(FSFREEZE_HOOK_FREEZE, &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(err, local_err);
+        return -1;
+    }
+
     QTAILQ_INIT(&mounts);
     build_fs_mount_list(&mounts, &local_err);
     if (error_is_set(&local_err)) {
@@ -537,6 +603,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, err);
+
     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 v7 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/mysql-flush.sh.sample : quiesce MySQL before snapshot

Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com>
---
 .gitignore                                         |    1 
 Makefile                                           |    2 -
 scripts/qemu-guest-agent/fsfreeze-hook             |   33 ++++++++++++
 .../fsfreeze-hook.d/mysql-flush.sh.sample          |   55 ++++++++++++++++++++
 4 files changed, 90 insertions(+), 1 deletion(-)
 create mode 100755 scripts/qemu-guest-agent/fsfreeze-hook
 create mode 100755 scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample

diff --git a/.gitignore b/.gitignore
index bd6ba1c..286822d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -93,3 +93,4 @@ cscope.*
 tags
 TAGS
 *~
+!scripts/qemu-guest-agent/fsfreeze-hook.d
diff --git a/Makefile b/Makefile
index 9ecbcbb..466dcd7 100644
--- a/Makefile
+++ b/Makefile
@@ -248,7 +248,7 @@ clean:
 # avoid old build problems by removing potentially incorrect old files
 	rm -f config.mak op-i386.h opc-i386.h gen-op-i386.h op-arm.h opc-arm.h gen-op-arm.h
 	rm -f qemu-options.def
-	find . -name '*.[od]' -exec rm -f {} +
+	find . -name '*.[od]' -type f -exec rm -f {} +
 	rm -f *.a *.lo $(TOOLS) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod *~ */*~
 	rm -Rf .libs
 	rm -f qemu-img-cmds.h
diff --git a/scripts/qemu-guest-agent/fsfreeze-hook b/scripts/qemu-guest-agent/fsfreeze-hook
new file mode 100755
index 0000000..4f7ff15
--- /dev/null
+++ b/scripts/qemu-guest-agent/fsfreeze-hook
@@ -0,0 +1,33 @@
+#!/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 | *.sample)
+            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
+
+exit 0
diff --git a/scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample b/scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample
new file mode 100755
index 0000000..c69b8ad
--- /dev/null
+++ b/scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample
@@ -0,0 +1,55 @@
+#!/bin/sh
+
+# Flush MySQL tables to the disk before the filesystem is freezed.
+# At the same time, this keeps a read lock in order to avoid write accesses
+# from the other clients until the filesystem is thawed.
+
+MYSQL="/usr/bin/mysql"
+MYSQL_OPTS="-uroot" #"-prootpassword"
+FIFO=/tmp/mysql-flush.fifo
+MYSQL_CMD="$MYSQL $MYSQL_OPTS"
+
+# Check mysql is installed and the server running
+[ -x $MYSQL ] && $MYSQL_CMD < /dev/null || exit 0
+
+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_CMD &
+        # wait until every block is flushed
+        while [ "$(echo 'SHOW STATUS LIKE "Key_blocks_not_flushed"' |\
+                 $MYSQL_CMD | 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_CMD > $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
+        ;;
+
+    *)
+        exit 1
+        ;;
+esac

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

[Luiz Capitulino]

On Fri, 07 Dec 2012 17:39:29 +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.
> 
> If --fsfreeze-hook option is specified, the hook is executed with
> "freeze" argument before the filesystem is frozen by fsfreeze-freeze
> command. As for fsfreeze-thaw command, the hook is executed with "thaw"
> argument after the filesystem is thawed.
> 
> This patch depends on patchset to improve error reporting by Luiz Capitulino:
>   http://lists.gnu.org/archive/html/qemu-devel/2012-11/msg03016.html
> 
> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com>

Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>

> ---
>  qemu-ga.c              |   42 +++++++++++++++++++++++++++++
>  qga/commands-posix.c   |   69 ++++++++++++++++++++++++++++++++++++++++++++++++
>  qga/guest-agent-core.h |    1 +
>  3 files changed, 111 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-ga.c b/qemu-ga.c
> index 9b59a52..53ce462 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"
> +"                    enable 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 = NULL;
> +#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", 2, 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 ? optarg : QGA_FSFREEZE_HOOK_DEFAULT;
> +            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 2591a3c..f7c0d9a 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -410,6 +410,66 @@ static void build_fs_mount_list(FsMountList *mounts, Error **err)
>  
>  #if defined(CONFIG_FSFREEZE)
>  
> +typedef enum {
> +    FSFREEZE_HOOK_THAW = 0,
> +    FSFREEZE_HOOK_FREEZE,
> +} FsfreezeHookArg;
> +
> +const char *fsfreeze_hook_arg_string[] = {
> +    "thaw",
> +    "freeze",
> +};
> +
> +static void execute_fsfreeze_hook(FsfreezeHookArg arg, Error **err)
> +{
> +    int status;
> +    pid_t pid;
> +    const char *hook;
> +    const char *arg_str = fsfreeze_hook_arg_string[arg];
> +    Error *local_err = NULL;
> +
> +    hook = ga_fsfreeze_hook(ga_state);
> +    if (!hook) {
> +        return;
> +    }
> +    if (access(hook, X_OK) != 0) {
> +        error_setg_errno(err, errno, "can't access fsfreeze hook '%s'", hook);
> +        return;
> +    }
> +
> +    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) {
> +        error_setg_errno(err, errno, "failed to create child process");
> +        return;
> +    }
> +
> +    ga_wait_child(pid, &status, &local_err);
> +    if (error_is_set(&local_err)) {
> +        error_propagate(err, local_err);
> +        return;
> +    }
> +
> +    if (!WIFEXITED(status)) {
> +        error_setg(err, "fsfreeze hook has terminated abnormally");
> +        return;
> +    }
> +
> +    status = WEXITSTATUS(status);
> +    if (status) {
> +        error_setg(err, "fsfreeze hook has failed with status %d", status);
> +        return;
> +    }
> +}
> +
>  /*
>   * Return status of freeze/thaw
>   */
> @@ -436,6 +496,12 @@ int64_t qmp_guest_fsfreeze_freeze(Error **err)
>  
>      slog("guest-fsfreeze called");
>  
> +    execute_fsfreeze_hook(FSFREEZE_HOOK_FREEZE, &local_err);
> +    if (error_is_set(&local_err)) {
> +        error_propagate(err, local_err);
> +        return -1;
> +    }
> +
>      QTAILQ_INIT(&mounts);
>      build_fs_mount_list(&mounts, &local_err);
>      if (error_is_set(&local_err)) {
> @@ -537,6 +603,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, err);
> +
>      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 v7 2/2] qemu-ga: sample fsfreeze hooks

[Luiz Capitulino]

On Fri, 07 Dec 2012 17:39:32 +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/mysql-flush.sh.sample : quiesce MySQL before snapshot
> 
> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com>
> ---
>  .gitignore                                         |    1 
>  Makefile                                           |    2 -
>  scripts/qemu-guest-agent/fsfreeze-hook             |   33 ++++++++++++
>  .../fsfreeze-hook.d/mysql-flush.sh.sample          |   55 ++++++++++++++++++++
>  4 files changed, 90 insertions(+), 1 deletion(-)
>  create mode 100755 scripts/qemu-guest-agent/fsfreeze-hook
>  create mode 100755 scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample
> 
> diff --git a/.gitignore b/.gitignore
> index bd6ba1c..286822d 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -93,3 +93,4 @@ cscope.*
>  tags
>  TAGS
>  *~
> +!scripts/qemu-guest-agent/fsfreeze-hook.d

Why? Do we expect to have *~ files in there?

> diff --git a/Makefile b/Makefile
> index 9ecbcbb..466dcd7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -248,7 +248,7 @@ clean:
>  # avoid old build problems by removing potentially incorrect old files
>  	rm -f config.mak op-i386.h opc-i386.h gen-op-i386.h op-arm.h opc-arm.h gen-op-arm.h
>  	rm -f qemu-options.def
> -	find . -name '*.[od]' -exec rm -f {} +
> +	find . -name '*.[od]' -type f -exec rm -f {} +

What does this change have to do with this patch?

>  	rm -f *.a *.lo $(TOOLS) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod *~ */*~
>  	rm -Rf .libs
>  	rm -f qemu-img-cmds.h
> diff --git a/scripts/qemu-guest-agent/fsfreeze-hook b/scripts/qemu-guest-agent/fsfreeze-hook
> new file mode 100755
> index 0000000..4f7ff15
> --- /dev/null
> +++ b/scripts/qemu-guest-agent/fsfreeze-hook
> @@ -0,0 +1,33 @@
> +#!/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 | *.sample)
> +            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
> +
> +exit 0
> diff --git a/scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample b/scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample
> new file mode 100755
> index 0000000..c69b8ad
> --- /dev/null
> +++ b/scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample
> @@ -0,0 +1,55 @@
> +#!/bin/sh
> +
> +# Flush MySQL tables to the disk before the filesystem is freezed.
> +# At the same time, this keeps a read lock in order to avoid write accesses
> +# from the other clients until the filesystem is thawed.
> +
> +MYSQL="/usr/bin/mysql"
> +MYSQL_OPTS="-uroot" #"-prootpassword"
> +FIFO=/tmp/mysql-flush.fifo
> +MYSQL_CMD="$MYSQL $MYSQL_OPTS"
> +
> +# Check mysql is installed and the server running
> +[ -x $MYSQL ] && $MYSQL_CMD < /dev/null || exit 0
> +
> +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_CMD &
> +        # wait until every block is flushed
> +        while [ "$(echo 'SHOW STATUS LIKE "Key_blocks_not_flushed"' |\
> +                 $MYSQL_CMD | 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_CMD > $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
> +        ;;
> +
> +    *)
> +        exit 1
> +        ;;
> +esac
> 

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

[mdroth]

On Fri, Dec 07, 2012 at 11:55:16AM -0200, Luiz Capitulino wrote:
> On Fri, 07 Dec 2012 17:39:32 +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/mysql-flush.sh.sample : quiesce MySQL before snapshot
> > 
> > Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com>
> > ---
> >  .gitignore                                         |    1 
> >  Makefile                                           |    2 -
> >  scripts/qemu-guest-agent/fsfreeze-hook             |   33 ++++++++++++
> >  .../fsfreeze-hook.d/mysql-flush.sh.sample          |   55 ++++++++++++++++++++
> >  4 files changed, 90 insertions(+), 1 deletion(-)
> >  create mode 100755 scripts/qemu-guest-agent/fsfreeze-hook
> >  create mode 100755 scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample
> > 
> > diff --git a/.gitignore b/.gitignore
> > index bd6ba1c..286822d 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -93,3 +93,4 @@ cscope.*
> >  tags
> >  TAGS
> >  *~
> > +!scripts/qemu-guest-agent/fsfreeze-hook.d
> 
> Why? Do we expect to have *~ files in there?

I think default vim configurations will copy <file> to <file>~ prior to
editing.

> 
> > diff --git a/Makefile b/Makefile
> > index 9ecbcbb..466dcd7 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -248,7 +248,7 @@ clean:
> >  # avoid old build problems by removing potentially incorrect old files
> >  	rm -f config.mak op-i386.h opc-i386.h gen-op-i386.h op-arm.h opc-arm.h gen-op-arm.h
> >  	rm -f qemu-options.def
> > -	find . -name '*.[od]' -exec rm -f {} +
> > +	find . -name '*.[od]' -type f -exec rm -f {} +
> 
> What does this change have to do with this patch?

Looks like it'll try (and fail, due to lack of -r option to rm) to
delete the fsfreeze-hook.d directory on `make clean`. For in-tree builds
at least.

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2091 bytes --]

On 12/07/2012 06:55 AM, Luiz Capitulino wrote:
> On Fri, 07 Dec 2012 17:39:32 +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/mysql-flush.sh.sample : quiesce MySQL before snapshot
>>
>> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com>
>> ---
>>  .gitignore                                         |    1 
>>  Makefile                                           |    2 -
>>  scripts/qemu-guest-agent/fsfreeze-hook             |   33 ++++++++++++
>>  .../fsfreeze-hook.d/mysql-flush.sh.sample          |   55 ++++++++++++++++++++
>>  4 files changed, 90 insertions(+), 1 deletion(-)
>>  create mode 100755 scripts/qemu-guest-agent/fsfreeze-hook
>>  create mode 100755 scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample
>>
>> diff --git a/.gitignore b/.gitignore
>> index bd6ba1c..286822d 100644
>> --- a/.gitignore
>> +++ b/.gitignore
>> @@ -93,3 +93,4 @@ cscope.*
>>  tags
>>  TAGS
>>  *~
>> +!scripts/qemu-guest-agent/fsfreeze-hook.d
> 
> Why? Do we expect to have *~ files in there?

What does your question have to do with the patch, which isn't even
touching the pre-existing *~ line?

> 
>> diff --git a/Makefile b/Makefile
>> index 9ecbcbb..466dcd7 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -248,7 +248,7 @@ clean:
>>  # avoid old build problems by removing potentially incorrect old files
>>  	rm -f config.mak op-i386.h opc-i386.h gen-op-i386.h op-arm.h opc-arm.h gen-op-arm.h
>>  	rm -f qemu-options.def
>> -	find . -name '*.[od]' -exec rm -f {} +
>> +	find . -name '*.[od]' -type f -exec rm -f {} +
> 
> What does this change have to do with this patch?

Because this patch introduces a directory
scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample that
belongs to the repo and therefore must not be cleaned.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

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

[Luiz Capitulino]

On Fri, 7 Dec 2012 11:47:24 -0600
mdroth <mdroth@linux.vnet.ibm.com> wrote:

> On Fri, Dec 07, 2012 at 11:55:16AM -0200, Luiz Capitulino wrote:
> > On Fri, 07 Dec 2012 17:39:32 +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/mysql-flush.sh.sample : quiesce MySQL before snapshot
> > > 
> > > Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com>
> > > ---
> > >  .gitignore                                         |    1 
> > >  Makefile                                           |    2 -
> > >  scripts/qemu-guest-agent/fsfreeze-hook             |   33 ++++++++++++
> > >  .../fsfreeze-hook.d/mysql-flush.sh.sample          |   55 ++++++++++++++++++++
> > >  4 files changed, 90 insertions(+), 1 deletion(-)
> > >  create mode 100755 scripts/qemu-guest-agent/fsfreeze-hook
> > >  create mode 100755 scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample
> > > 
> > > diff --git a/.gitignore b/.gitignore
> > > index bd6ba1c..286822d 100644
> > > --- a/.gitignore
> > > +++ b/.gitignore
> > > @@ -93,3 +93,4 @@ cscope.*
> > >  tags
> > >  TAGS
> > >  *~
> > > +!scripts/qemu-guest-agent/fsfreeze-hook.d
> > 
> > Why? Do we expect to have *~ files in there?
> 
> I think default vim configurations will copy <file> to <file>~ prior to
> editing.

But should that be tracked by git specifically in that dir?

> > > diff --git a/Makefile b/Makefile
> > > index 9ecbcbb..466dcd7 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -248,7 +248,7 @@ clean:
> > >  # avoid old build problems by removing potentially incorrect old files
> > >  	rm -f config.mak op-i386.h opc-i386.h gen-op-i386.h op-arm.h opc-arm.h gen-op-arm.h
> > >  	rm -f qemu-options.def
> > > -	find . -name '*.[od]' -exec rm -f {} +
> > > +	find . -name '*.[od]' -type f -exec rm -f {} +
> > 
> > What does this change have to do with this patch?
> 
> Looks like it'll try (and fail, due to lack of -r option to rm) to
> delete the fsfreeze-hook.d directory on `make clean`. For in-tree builds
> at least.

I can't see it, but well, I don't want to hold this patch for minor things
either... It looks good for me apart from these two nitpicks.

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

[Luiz Capitulino]

On Fri, 07 Dec 2012 11:22:54 -0700
Eric Blake <eblake@redhat.com> wrote:

> On 12/07/2012 06:55 AM, Luiz Capitulino wrote:
> > On Fri, 07 Dec 2012 17:39:32 +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/mysql-flush.sh.sample : quiesce MySQL before snapshot
> >>
> >> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com>
> >> ---
> >>  .gitignore                                         |    1 
> >>  Makefile                                           |    2 -
> >>  scripts/qemu-guest-agent/fsfreeze-hook             |   33 ++++++++++++
> >>  .../fsfreeze-hook.d/mysql-flush.sh.sample          |   55 ++++++++++++++++++++
> >>  4 files changed, 90 insertions(+), 1 deletion(-)
> >>  create mode 100755 scripts/qemu-guest-agent/fsfreeze-hook
> >>  create mode 100755 scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample
> >>
> >> diff --git a/.gitignore b/.gitignore
> >> index bd6ba1c..286822d 100644
> >> --- a/.gitignore
> >> +++ b/.gitignore
> >> @@ -93,3 +93,4 @@ cscope.*
> >>  tags
> >>  TAGS
> >>  *~
> >> +!scripts/qemu-guest-agent/fsfreeze-hook.d
> > 
> > Why? Do we expect to have *~ files in there?
> 
> What does your question have to do with the patch, which isn't even
> touching the pre-existing *~ line?

As far I could understand the ! prefix in gitignore documention, it's
changing the *~ meaning for the fsfreeze-hoo.d directory, and I wanted
to understand why.

> >> diff --git a/Makefile b/Makefile
> >> index 9ecbcbb..466dcd7 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -248,7 +248,7 @@ clean:
> >>  # avoid old build problems by removing potentially incorrect old files
> >>  	rm -f config.mak op-i386.h opc-i386.h gen-op-i386.h op-arm.h opc-arm.h gen-op-arm.h
> >>  	rm -f qemu-options.def
> >> -	find . -name '*.[od]' -exec rm -f {} +
> >> +	find . -name '*.[od]' -type f -exec rm -f {} +
> > 
> > What does this change have to do with this patch?
> 
> Because this patch introduces a directory
> scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample that
> belongs to the repo and therefore must not be cleaned.

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2930 bytes --]

On 12/07/2012 01:39 AM, Tomoki Sekiyama wrote:
> Adds sample hook scripts for --fsfreeze-hook option of qemu-ga.
>   - fsfreeze-hook : execute scripts in fsfreeze-hook.d/
>   - fsfreeze-hook.d/mysql-flush.sh.sample : quiesce MySQL before snapshot
> 
> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com>
> ---

> @@ -0,0 +1,33 @@
> +#!/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

s/freezed/frozen/

> +
> +# Iterate executables in directory "fsfreeze-hook.d" with the specified args
> +[ ! -d "$FSFREEZE_D" ] && exit 1

Do you really want to fail the entire operation if the directory doesn't
exist?  Shouldn't you instead exit 0 because there is nothing to do?

> +for file in "$FSFREEZE_D"/* ; do
> +    is_ignored_file "$file" && continue
> +    [ -x "$file" ] || continue
> +    echo "$(date): execute $file $@" >>$LOGFILE

This is unsafe (although the worst that will happen is a poor message to
the log file).  $file might contain backslash, and echo cannot portably
be mixed with backslash.  Use printf(1) instead.

> +    "$file" "$@" >>$LOGFILE 2>&1
> +    STATUS=$?
> +    echo "$(date): $file finished with status=$STATUS" >>$LOGFILE

Again.


> @@ -0,0 +1,55 @@
> +#!/bin/sh
> +
> +# Flush MySQL tables to the disk before the filesystem is freezed.

s/freezed/frozen/

> +# At the same time, this keeps a read lock in order to avoid write accesses
> +# from the other clients until the filesystem is thawed.
> +
> +MYSQL="/usr/bin/mysql"
> +MYSQL_OPTS="-uroot" #"-prootpassword"
> +FIFO=/tmp/mysql-flush.fifo
> +MYSQL_CMD="$MYSQL $MYSQL_OPTS"
> +
> +# Check mysql is installed and the server running
> +[ -x $MYSQL ] && $MYSQL_CMD < /dev/null || exit 0

Safe as written, since you just initialized $MYSQL above; but risky,
since the mere use of MYSQL in the initialization might encourage
someone to point to an alternate path that includes spaces.  Then again,
if they do that, then MYSQL_CMD is broken.  It might be better to be
explicit and write
 [ -x "$MYSQL" ] && "$MYSQL" $MYSQL_OPTS < /dev/null
but I won't insist.

> +        # 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

POSIX says that 'trap foo INT' is required to work, but 'trap foo
SIGINT' is optional.  Also, shouldn't you also worry about HUP, ALRM,
and TERM?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1151 bytes --]

On 12/07/2012 11:31 AM, Luiz Capitulino wrote:
>>>> +++ b/.gitignore
>>>> @@ -93,3 +93,4 @@ cscope.*
>>>>  tags
>>>>  TAGS
>>>>  *~
>>>> +!scripts/qemu-guest-agent/fsfreeze-hook.d
>>>
>>> Why? Do we expect to have *~ files in there?
>>
>> What does your question have to do with the patch, which isn't even
>> touching the pre-existing *~ line?
> 
> As far I could understand the ! prefix in gitignore documention, it's
> changing the *~ meaning for the fsfreeze-hoo.d directory, and I wanted
> to understand why.
> 

No, it is changing a much earlier line:

*.d

to say that this _particular_ .d is allowed to be committed.  It has
nothing to do with *~.

Still, I have to wonder if we really want to store these files in a .d
in the repository itself, or if we should store them under some other
file name and only at 'make install' time insert them into a .d at the
install destination.  It would clean up this confusion about the
.gitignore as well as the change to the 'find' command during 'make clean'.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

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

[Luiz Capitulino]

On Fri, 07 Dec 2012 11:37:44 -0700
Eric Blake <eblake@redhat.com> wrote:

> On 12/07/2012 11:31 AM, Luiz Capitulino wrote:
> >>>> +++ b/.gitignore
> >>>> @@ -93,3 +93,4 @@ cscope.*
> >>>>  tags
> >>>>  TAGS
> >>>>  *~
> >>>> +!scripts/qemu-guest-agent/fsfreeze-hook.d
> >>>
> >>> Why? Do we expect to have *~ files in there?
> >>
> >> What does your question have to do with the patch, which isn't even
> >> touching the pre-existing *~ line?
> > 
> > As far I could understand the ! prefix in gitignore documention, it's
> > changing the *~ meaning for the fsfreeze-hoo.d directory, and I wanted
> > to understand why.
> > 
> 
> No, it is changing a much earlier line:
> 
> *.d
> 
> to say that this _particular_ .d is allowed to be committed.  It has
> nothing to do with *~.

Ah, now it makes a lot of sense, thanks Eric.

It would be nice to move it right below *.d, to avoid stupid comments :)

> Still, I have to wonder if we really want to store these files in a .d
> in the repository itself, or if we should store them under some other
> file name and only at 'make install' time insert them into a .d at the
> install destination.  It would clean up this confusion about the
> .gitignore as well as the change to the 'find' command during 'make clean'.

Well, now that I understand it both ways are fine with me (ie. what you
suggest and what's been implemented in this patch).

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

[Tomoki Sekiyama]

Hi, sorry for my late reply.

On 2012/12/08 3:57, Luiz Capitulino wrote:
> On Fri, 07 Dec 2012 11:37:44 -0700
> Eric Blake <eblake@redhat.com> wrote:
> 
>> On 12/07/2012 11:31 AM, Luiz Capitulino wrote:
>>>>>> +++ b/.gitignore
>>>>>> @@ -93,3 +93,4 @@ cscope.*
>>>>>>  tags
>>>>>>  TAGS
>>>>>>  *~
>>>>>> +!scripts/qemu-guest-agent/fsfreeze-hook.d
>>>>>
>>>>> Why? Do we expect to have *~ files in there?
>>>>
>>>> What does your question have to do with the patch, which isn't even
>>>> touching the pre-existing *~ line?
>>>
>>> As far I could understand the ! prefix in gitignore documention, it's
>>> changing the *~ meaning for the fsfreeze-hoo.d directory, and I wanted
>>> to understand why.
>>>
>>
>> No, it is changing a much earlier line:
>>
>> *.d
>>
>> to say that this _particular_ .d is allowed to be committed.  It has
>> nothing to do with *~.

Yes, it's for adding fsfreeze-hook.d into the repo.

> Ah, now it makes a lot of sense, thanks Eric.
> 
> It would be nice to move it right below *.d, to avoid stupid comments :)

And OK, I will move this there.

>> Still, I have to wonder if we really want to store these files in a .d
>> in the repository itself, or if we should store them under some other
>> file name and only at 'make install' time insert them into a .d at the
>> install destination.  It would clean up this confusion about the
>> .gitignore as well as the change to the 'find' command during 'make clean'.
> 
> Well, now that I understand it both ways are fine with me (ie. what you
> suggest and what's been implemented in this patch).
Then I'd like to keep current implementation.

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

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

[Tomoki Sekiyama]

Hi Eric,

Thanks for your review.

On 2012/12/08 3:34, Eric Blake wrote:
> On 12/07/2012 01:39 AM, Tomoki Sekiyama wrote:
>> Adds sample hook scripts for --fsfreeze-hook option of qemu-ga.
>>   - fsfreeze-hook : execute scripts in fsfreeze-hook.d/
>>   - fsfreeze-hook.d/mysql-flush.sh.sample : quiesce MySQL before snapshot
>>
>> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com>
>> ---
> 
>> @@ -0,0 +1,33 @@
>> +#!/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
> 
> s/freezed/frozen/
Oops...

>> +
>> +# Iterate executables in directory "fsfreeze-hook.d" with the specified args
>> +[ ! -d "$FSFREEZE_D" ] && exit 1
> 
> Do you really want to fail the entire operation if the directory doesn't
> exist?  Shouldn't you instead exit 0 because there is nothing to do?

I thought that was installation failure, but this might be too cautious.
Exiting 0 is also reasonable, so I will change this.

>> +for file in "$FSFREEZE_D"/* ; do
>> +    is_ignored_file "$file" && continue
>> +    [ -x "$file" ] || continue
>> +    echo "$(date): execute $file $@" >>$LOGFILE
> 
> This is unsafe (although the worst that will happen is a poor message to
> the log file).  $file might contain backslash, and echo cannot portably
> be mixed with backslash.  Use printf(1) instead.

OK, I will use printf here, and

>> +    "$file" "$@" >>$LOGFILE 2>&1
>> +    STATUS=$?
>> +    echo "$(date): $file finished with status=$STATUS" >>$LOGFILE
> 
> Again.
here too.

>> @@ -0,0 +1,55 @@
>> +#!/bin/sh
>> +
>> +# Flush MySQL tables to the disk before the filesystem is freezed.
> 
> s/freezed/frozen/
> 
>> +# At the same time, this keeps a read lock in order to avoid write accesses
>> +# from the other clients until the filesystem is thawed.
>> +
>> +MYSQL="/usr/bin/mysql"
>> +MYSQL_OPTS="-uroot" #"-prootpassword"
>> +FIFO=/tmp/mysql-flush.fifo
>> +MYSQL_CMD="$MYSQL $MYSQL_OPTS"
>> +
>> +# Check mysql is installed and the server running
>> +[ -x $MYSQL ] && $MYSQL_CMD < /dev/null || exit 0
> 
> Safe as written, since you just initialized $MYSQL above; but risky,
> since the mere use of MYSQL in the initialization might encourage
> someone to point to an alternate path that includes spaces.  Then again,
> if they do that, then MYSQL_CMD is broken.  It might be better to be
> explicit and write
>  [ -x "$MYSQL" ] && "$MYSQL" $MYSQL_OPTS < /dev/null
> but I won't insist.

OK, I will replace $MYSQL_CMD with "$MYSQL" $MYSQL_OPTS.

>> +        # 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
> 
> POSIX says that 'trap foo INT' is required to work, but 'trap foo
> SIGINT' is optional.  Also, shouldn't you also worry about HUP, ALRM,
> and TERM?

I will fix this to
  trap "..." HUP INT QUIT ALRM TERM

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