qemu-ga: execute script to quiesce the guest on fsfreeze-freeze/thaw

PATCH In review Tomoki Sekiyama 2 participants 6 messages Nov 8, 2012 – Nov 12, 2012

Share & tools

Tomoki Sekiyama

author

2012-11-08 12:04 UTC #0 lore raw

PATCH 1/2 qemu-ga: execute script to quiesce the guest on fsfreeze-freeze/thaw

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 script specified in --fsfreeze-script option is executed with "freeze" argument before the filesystem is frozen. For fsfreeze-thaw command, the script is executed with "thaw" argument after the filesystem is thawed.

Signed-off-by Tomoki Sekiyama <[email protected]>

---
 qemu-ga.c              | 42 +++++++++++++++++++++++++++++++++++++++++-
 qga/commands-posix.c   | 34 ++++++++++++++++++++++++++++++++++
 qga/guest-agent-core.h |  2 ++
 3 files changed, 77 insertions(+), 1 deletion(-)

qemu-ga.c +41 -1

index 9b59a52..7cb682e 100644
@@ -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_SCRIPT_DEFAULT CONFIG_QEMU_CONFDIR "/fsfreeze.sh"
+#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_script;
+#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_script\n"
+"                    specify fsfreeze script (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 <[email protected]>\n"
     , cmd, QEMU_VERSION, QGA_VIRTIO_PATH_DEFAULT, QGA_PIDFILE_DEFAULT,
+#ifdef CONFIG_FSFREEZE
+    QGA_FSFREEZE_SCRIPT_DEFAULT,
+#endif
     QGA_STATEDIR_DEFAULT);
 }

@@ -401,6 +420,13 @@ void ga_unset_frozen(GAState *s)
     }
 }
 
+#ifdef CONFIG_FSFREEZE
+const char *ga_fsfreeze_script(GAState *s)
+{
+    return s->fsfreeze_script;
+}
+#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_script = QGA_FSFREEZE_SCRIPT_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-script", 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_script = 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_script = fsfreeze_script;
+#endif
     g_log_set_default_handler(ga_log, s);
     g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR);
     ga_enable_logging(s);

qga/commands-posix.c +34 -0

index 726930a..007c0a3 100644
@@ -13,6 +13,7 @@
 
 #include <glib.h>
 #include <sys/types.h>
+#include <sys/stat.h>
 #include <sys/ioctl.h>
 #include <sys/wait.h>
 #include "qga/guest-agent-core.h"

@@ -396,6 +397,34 @@ GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err)
     return GUEST_FSFREEZE_STATUS_THAWED;
 }
 
+int execute_fsfreeze_script(const char *arg)
+{
+    int ret = -1;
+    const char *fsfreeze_script;
+    char *cmdline;
+    struct stat st;
+
+    fsfreeze_script = ga_fsfreeze_script(ga_state);
+    if (fsfreeze_script && stat(fsfreeze_script, &st) == 0) {
+        if (S_ISREG(st.st_mode) && (st.st_mode & S_IXUSR)) {
+            slog("executing fsfreeze script with arg `%s'", arg);
+            cmdline = malloc(strlen(fsfreeze_script) + strlen(arg) + 2);
+            if (cmdline) {
+                sprintf(cmdline, "%s %s", fsfreeze_script, arg);
+                ret = system(cmdline);
+                free(cmdline);
+            }
+            if (ret > 0) {
+                g_warning("fsfreeze script failed with status=%d", ret);
+            } else if (ret == -1) {
+                g_warning("execution of fsfreeze script failed: %s",
+                          strerror(errno));
+            }
+        }
+    }
+    return ret;
+}
+
 /*
  * Walk list of mounted file systems in the guest, and freeze the ones which
  * are real local file systems.

@@ -410,6 +439,8 @@ int64_t qmp_guest_fsfreeze_freeze(Error **err)
 
     slog("guest-fsfreeze called");
 
+    execute_fsfreeze_script("freeze");
+
     QTAILQ_INIT(&mounts);
     ret = build_fs_mount_list(&mounts);
     if (ret < 0) {

@@ -513,6 +544,9 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
 
     ga_unset_frozen(ga_state);
     free_fs_mount_list(&mounts);
+
+    execute_fsfreeze_script("thaw");
+
     return i;
 }

qga/guest-agent-core.h +2 -0

index 49a7abe..b466bfd 100644
@@ -34,6 +34,8 @@ 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_script(GAState *s);
+int execute_fsfreeze_script(const char *arg);
 
 #ifndef _WIN32
 void reopen_fd_to_null(int fd);

Signature · 1 line

1.7.11.7

Tomoki Sekiyama

author

2012-11-08 12:36 UTC #1 lore raw

PATCH 2/2 qemu-ga: sample fsfreeze-script

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

Signed-off-by Tomoki Sekiyama <[email protected]>

---
 .gitignore                                         |  1 +
 .../fsfreeze.d/mysql-flush.sh                      | 41 ++++++++++++++++++++++
 .../fsfreeze-script-sample/fsfreeze.sh             | 17 +++++++++
 3 files changed, 59 insertions(+)
 create mode 100755 docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh
 create mode 100755 docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh

.gitignore +1 -0

index bd6ba1c..867cb86 100644
@@ -68,6 +68,7 @@ fsdev/virtfs-proxy-helper.pod
 *.tp
 *.vr
 *.d
+!fsfreeze.d
 *.o
 *.lo
 *.la

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh new file +41 -0

41 lines not on this page · raw message

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh new file +17 -0

new file mode 100755
index 0000000..b402107
@@ -0,0 +1,17 @@
+#!/bin/bash
+
+# This script is executed when a guest agent receives fsfreeze-freeze and
+# fsfreeze-thaw command when it is specified in --fsfreeze-script/-F option
+# of qemu-ga or placed in default path (/etc/qemu/fsfreeze-script.sh).
+# When the agent receives fsfreeze-freeze command, the script is issued with
+# "freeze" argument before the filesystem is freezed.. And for fsfreeze-thaw,
+# it is issued with "thaw" argument after filesystem is thawed.
+#
+# This script iterates executables in directory "fsfreeze.d" with the
+# specified argument.
+
+cd `dirname $0`
+cd fsfreeze.d
+for x in *; do
+	[ -x ./$x ] && ./$x $1
+done

Signature · 1 line

1.7.11.7

Eric Blake

2012-11-08 18:25 UTC #2 lore raw

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

Review line 1

Quoted 9 lines · Tomoki Sekiyama

Adds sample scripts for --fsfreeze-script option of qemu-ga.
-fsfreeze.sh: iterates and execute scripts in fsfreeze.d/ -fsfreeze.d/mysql-flush.sh: quiesce MySQL before snapshot
Signed-off-by Tomoki Sekiyama <[email protected]>
---
+++ b/docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh
(quoted diff) +1 -0
@@ -0,0 +1,41 @@
+#!/bin/bash

Any particular reason you have to use a bash-ism, or would it be appropriate to use /bin/sh here?

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh line 13

Quoted 12 lines

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh new file +12 -0

+MYSQL="mysql -uroot" #"-prootpassword"
+FIFO=/tmp/mysql-flush.fifo
+
+flush_and_wait() {
+	echo 'FLUSH TABLES WITH READ LOCK \G'
+	read < $FIFO
+	echo 'UNLOCK TABLES \G'
+}
+
+if [ "$1" = "freeze" ]; then
+
+	mkfifo $FIFO

No error checking?

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh line 16

Quoted 3 lines

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh new file +3 -0
+	flush_and_wait | $MYSQL &
+	# wait until every block is flushed
+	while [ "`echo 'SHOW STATUS LIKE "Key_blocks_not_flushed"' |\

I prefer $() over ``

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh line 22

Quoted 6 lines

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh new file +6 -0
+		$MYSQL | tail -1 | cut -f 2`" -gt 0 ]; do
+		sleep 1
+	done
+	# for InnoDB, wait until every log is flushed
+	while :; do
+		INNODB_STATUS=/tmp/mysql-innodb.status

This name is highly predictable, and therefore highly insecure. I hope I'm never caught installing something this insecure on my system.

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh line 27

Quoted 5 lines

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh new file +5 -0

+		echo '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`

More instances where $() is nicer than ``

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh line 29

Quoted 2 lines

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh new file +2 -0
+		rm $INNODB_STATUS
+		[ $LOG_CURRENT = $LOG_FLUSHED ] && break

Are you sure that $LOG_CURRENT and $LOG_FLUSHED will always be non-empty and contain no whitespace? If not, you are missing quoting here.

Quoted 18 lines

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh new file +12 -0
+		sleep 1
+	done
+
+elif [ "$1" = "thaw" ]; then
+
+	if [ -p $FIFO ]; then
+		echo > $FIFO
+		rm $FIFO
+	fi
+
+fi
+
docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh new file +0 -0
new file mode 100755
index 0000000..b402107

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh line 1

Quoted 1 line

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh new file +1 -0
@@ -0,0 +1,17 @@
+#!/bin/bash

Again, I see no bash-isms, why not use /bin/sh.

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh line 14

Quoted 13 lines

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh new file +13 -0

+
+# This script is executed when a guest agent receives fsfreeze-freeze and
+# fsfreeze-thaw command when it is specified in --fsfreeze-script/-F option
+# of qemu-ga or placed in default path (/etc/qemu/fsfreeze-script.sh).
+# When the agent receives fsfreeze-freeze command, the script is issued with
+# "freeze" argument before the filesystem is freezed.. And for fsfreeze-thaw,
+# it is issued with "thaw" argument after filesystem is thawed.
+#
+# This script iterates executables in directory "fsfreeze.d" with the
+# specified argument.
+
+cd `dirname $0`
+cd fsfreeze.d

Unsafe if $0 contains spaces or starts with '-'. Although you could argue that either of those situations represents installation error, it never hurts to be robust. Also, why bother with two cd when one would do, and where is your error checking?

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh line 16

Quoted 2 lines

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh new file +2 -0
+for x in *; do
+	[ -x ./$x ] && ./$x $1

Should you be filtering out files such as *~ or *.bak or ~.rpmsave, and so forth? This is insecure if $x contains spaces. And rather than unquoted $1, it is better to pass "$@", as in:

[ -x "$x" ] && "./$x" "$@"

Quoted 2 lines

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh new file +1 -0
+done

Signature · 2 lines

Eric Blake   [email protected]    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Eric Blake

2012-11-08 18:38 UTC #4 lore raw

In reply to Tomoki Sekiyama

On 11/08/2012 05:05 AM, Tomoki Sekiyama wrote:

[Recoding to UTF-8, as ISO-2022-JP is not universally installed these days - you may want to reconsider your mailer's defaults]

Quoted 19 lines

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 script specified in --fsfreeze-script option is executed with "freeze" argument before the filesystem is frozen. For fsfreeze-thaw command, the script is executed with "thaw" argument after the filesystem is thawed.
Signed-off-by Tomoki Sekiyama <[email protected]>
---
qga/commands-posix.c +0 -0
@@ -396,6 +397,34 @@ GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err)
     return GUEST_FSFREEZE_STATUS_THAWED;
 }
 

qga/commands-posix.c GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err) · line 409

Quoted 10 lines

qga/commands-posix.c +10 -0

+int execute_fsfreeze_script(const char *arg)
+{
+    int ret = -1;
+    const char *fsfreeze_script;
+    char *cmdline;
+    struct stat st;
+
+    fsfreeze_script = ga_fsfreeze_script(ga_state);
+    if (fsfreeze_script && stat(fsfreeze_script, &st) == 0) {
+        if (S_ISREG(st.st_mode) && (st.st_mode & S_IXUSR)) {

Is it any simpler to use access(fsfreeze_script, X_OK) to check if the script exists and is executable?

qga/commands-posix.c GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err) · line 411

Quoted 2 lines

qga/commands-posix.c +2 -0

+            slog("executing fsfreeze script with arg `%s'", arg);
+            cmdline = malloc(strlen(fsfreeze_script) + strlen(arg) + 2);

Don't we prefer g_malloc over malloc in qemu-ga?

qga/commands-posix.c GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err) · line 413

Quoted 2 lines

qga/commands-posix.c +2 -0

+            if (cmdline) {
+                sprintf(cmdline, "%s %s", fsfreeze_script, arg);

Thankfully, this doesn't overflow, but isn't there a glib function that makes it easier to create a malloc'd concatenated string in one call rather than pairing a malloc and sprintf? That said, why do you even need a single string, given that...

qga/commands-posix.c GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err) · line 414

Quoted 1 line

qga/commands-posix.c +1 -0
+                ret = system(cmdline);

...system() is not required to be thread-safe, but we should assume that qemu-ga is multi-threaded, and therefore we should not use system. Besides executing things via an extra layer of shell opens doors for further problems; for example, if the user starts qemu-ga with --fsfreeze-script '/path/with spaces/script', your command line is horribly broken when passed through the shell. It would be much better to directly fork() and exec() the script ourselves instead of relying on system().

qga/commands-posix.c GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err) · line 418

Quoted 4 lines

qga/commands-posix.c +4 -0

+                free(cmdline);
+            }
+            if (ret > 0) {
+                g_warning("fsfreeze script failed with status=%d", ret);

This is a potentially misleading message; you should be using macros such as WEXITSTATUS when interpreting the result of system(), since not all systems return exit status 1 in the same bit position.

qga/commands-posix.c GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err) · line 421

Quoted 3 lines

qga/commands-posix.c +3 -0

+            } else if (ret == -1) {
+                g_warning("execution of fsfreeze script failed: %s",
+                          strerror(errno));

free() is allowed to clobber errno, which means you may be reporting the wrong error if system() failed with -1.

qga/commands-posix.c GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err) · line 427

Quoted 6 lines

qga/commands-posix.c +6 -0

+            }
+        }
+    }
+    return ret;
+}
+

The idea of having the freeze and thaw actions hook out to user-specified actions for additional steps seems nice, but this patch series needs a lot more work.

Signature · 2 lines

Eric Blake   [email protected]    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Tomoki Sekiyama

author

2012-11-12 04:09 UTC #5 lore raw

Hi Eric, thanks for your review.

Quoted 4 lines · Eric Blake

On 11/08/2012 05:05 AM, Tomoki Sekiyama wrote:
[Recoding to UTF-8, as ISO-2022-JP is not universally installed these days - you may want to reconsider your mailer's defaults]

Now this should be UTF-8, sorry.

Quoted 19 lines

Quoted 19 lines
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 script specified in --fsfreeze-script option is executed with "freeze" argument before the filesystem is frozen. For fsfreeze-thaw command, the script is executed with "thaw" argument after the filesystem is thawed.
Signed-off-by Tomoki Sekiyama <[email protected]>
---
qga/commands-posix.c +0 -0
@@ -396,6 +397,34 @@ GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err)
     return GUEST_FSFREEZE_STATUS_THAWED;
 }
 

Quoted 13 lines

qga/commands-posix.c GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err) · line 409
Quoted 10 lines
qga/commands-posix.c +10 -0
+int execute_fsfreeze_script(const char *arg)
+{
+    int ret = -1;
+    const char *fsfreeze_script;
+    char *cmdline;
+    struct stat st;
+
+    fsfreeze_script = ga_fsfreeze_script(ga_state);
+    if (fsfreeze_script && stat(fsfreeze_script, &st) == 0) {
+        if (S_ISREG(st.st_mode) && (st.st_mode & S_IXUSR)) {
Is it any simpler to use access(fsfreeze_script, X_OK) to check if the script exists and is executable?

OK, I will use access() here.

<snip>

Quoted 10 lines

qga/commands-posix.c GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err) · line 414
Quoted 1 line
qga/commands-posix.c +1 -0
+                ret = system(cmdline);
...system() is not required to be thread-safe, but we should assume that qemu-ga is multi-threaded, and therefore we should not use system. Besides executing things via an extra layer of shell opens doors for further problems; for example, if the user starts qemu-ga with --fsfreeze-script '/path/with spaces/script', your command line is horribly broken when passed through the shell. It would be much better to directly fork() and exec() the script ourselves instead of relying on system().

I will use fork()/exec() method instead of system(), as in shutdown, so I can remove malloc/free.

Quoted 6 lines

qga/commands-posix.c GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err) · line 418
Quoted 2 lines
qga/commands-posix.c +2 -0
+            if (ret > 0) {
+                g_warning("fsfreeze script failed with status=%d", ret);
This is a potentially misleading message; you should be using macros such as WEXITSTATUS when interpreting the result of system(), since not all systems return exit status 1 in the same bit position.

OK. I will refine error messages.

Quoted 3 lines

The idea of having the freeze and thaw actions hook out to user-specified actions for additional steps seems nice, but this patch series needs a lot more work.

Regards,

Signature · 3 lines

Tomoki Sekiyama <[email protected]>
Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory

Tomoki Sekiyama

author

2012-11-12 04:10 UTC #3 lore raw

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

In reply to Eric Blake

Quoted 5 lines · Eric Blake

Quoted 4 lines · Tomoki Sekiyama
Adds sample scripts for --fsfreeze-script option of qemu-ga.
-fsfreeze.sh: iterates and execute scripts in fsfreeze.d/ -fsfreeze.d/mysql-flush.sh: quiesce MySQL before snapshot

Quoted 8 lines

Review line 1
Quoted 5 lines
Signed-off-by Tomoki Sekiyama <[email protected]>
---
+++ b/docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh
(quoted diff) +1 -0
@@ -0,0 +1,41 @@
+#!/bin/bash
Any particular reason you have to use a bash-ism, or would it be appropriate to use /bin/sh here?

No, I will just replace this to /bin/sh.

Quoted 6 lines

Quoted 6 lines
docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh new file +6 -0
+MYSQL="mysql -uroot" #"-prootpassword"
+FIFO=/tmp/mysql-flush.fifo
+
+flush_and_wait() {
+	echo 'FLUSH TABLES WITH READ LOCK \G'
+	read < $FIFO

Quoted 8 lines

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh line 13
Quoted 6 lines
docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh new file +6 -0
+	echo 'UNLOCK TABLES \G'
+}
+
+if [ "$1" = "freeze" ]; then
+
+	mkfifo $FIFO
No error checking?

I will add the check.

Quoted 5 lines

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh line 16
Quoted 3 lines
docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh new file +3 -0
+	flush_and_wait | $MYSQL &
+	# wait until every block is flushed
+	while [ "`echo 'SHOW STATUS LIKE "Key_blocks_not_flushed"' |\
I prefer $() over ``

OK, will replace `` by $().

Quoted 9 lines

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh line 22
Quoted 6 lines
docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh new file +6 -0
+		$MYSQL | tail -1 | cut -f 2`" -gt 0 ]; do
+		sleep 1
+	done
+	# for InnoDB, wait until every log is flushed
+	while :; do
+		INNODB_STATUS=/tmp/mysql-innodb.status
This name is highly predictable, and therefore highly insecure. I hope I'm never caught installing something this insecure on my system.

Usually this doesn't contain critical data, but I will use mktemp to randomize the filename and to make this only accessible from the qemu-ga user.

Quoted 7 lines

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh line 27
Quoted 5 lines
docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh new file +5 -0
+		echo '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`
More instances where $() is nicer than ``

OK.

Quoted 5 lines

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh line 29
Quoted 2 lines
docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh new file +2 -0
+		rm $INNODB_STATUS
+		[ $LOG_CURRENT = $LOG_FLUSHED ] && break
Are you sure that $LOG_CURRENT and $LOG_FLUSHED will always be non-empty and contain no whitespace? If not, you are missing quoting here.

I will add quotes, just to make it robust.

<snip>

Quoted 9 lines

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh line 1
Quoted 7 lines
docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh new file +1 -0
new file mode 100755
index 0000000..b402107
@@ -0,0 +1,17 @@
+#!/bin/bash
Again, I see no bash-isms, why not use /bin/sh.

I will use /bin/sh here too.

Quoted 7 lines

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh line 14
Quoted 2 lines
docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh new file +2 -0
+cd `dirname $0`
+cd fsfreeze.d
Unsafe if $0 contains spaces or starts with '-'. Although you could argue that either of those situations represents installation error, it never hurts to be robust. Also, why bother with two cd when one would do, and where is your error checking?

OK, I will add quotes and -- to be make it safer and use full path below.

Quoted 8 lines

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh line 16
Quoted 2 lines
docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh new file +2 -0
+for x in *; do
+	[ -x ./$x ] && ./$x $1
Should you be filtering out files such as *~ or *.bak or ~.rpmsave, and so forth? This is insecure if $x contains spaces. And rather than unquoted $1, it is better to pass "$@", as in:
[ -x "$x" ] && "./$x" "$@"

I will add quotes and checking for files to be ignored.

Quoted 1 line

docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh line 6
Quoted 1 line
docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh new file +1 -0
+done

Again, thank you for your detailed review.
I will fix the issues and send new patchset soon.

Regards,

Signature · 3 lines

Tomoki Sekiyama <[email protected]>
Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory