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

[Qemu-devel] [PATCH 1/2] qemu-ga: execute script 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 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 <tomoki.sekiyama.qu@hitachi.com>
---
 qemu-ga.c              | 42 +++++++++++++++++++++++++++++++++++++++++-
 qga/commands-posix.c   | 34 ++++++++++++++++++++++++++++++++++
 qga/guest-agent-core.h |  2 ++
 3 files changed, 77 insertions(+), 1 deletion(-)

diff --git a/qemu-ga.c b/qemu-ga.c
index 9b59a52..7cb682e 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_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 <mdroth@linux.vnet.ibm.com>\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);
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 726930a..007c0a3 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -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;
 }
 
diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
index 49a7abe..b466bfd 100644
--- a/qga/guest-agent-core.h
+++ b/qga/guest-agent-core.h
@@ -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);
-- 
1.7.11.7

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

[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 <tomoki.sekiyama.qu@hitachi.com>
---
 .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

diff --git a/.gitignore b/.gitignore
index bd6ba1c..867cb86 100644
--- a/.gitignore
+++ b/.gitignore
@@ -68,6 +68,7 @@ fsdev/virtfs-proxy-helper.pod
 *.tp
 *.vr
 *.d
+!fsfreeze.d
 *.o
 *.lo
 *.la
diff --git a/docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh b/docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh
new file mode 100755
index 0000000..1704fb0
--- /dev/null
+++ b/docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh
@@ -0,0 +1,41 @@
+#!/bin/bash
+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
+	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
+	while :; do
+		INNODB_STATUS=/tmp/mysql-innodb.status
+		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`
+		rm $INNODB_STATUS
+		[ $LOG_CURRENT = $LOG_FLUSHED ] && break
+		sleep 1
+	done
+
+elif [ "$1" = "thaw" ]; then
+
+	if [ -p $FIFO ]; then
+		echo > $FIFO
+		rm $FIFO
+	fi
+
+fi
+
diff --git a/docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh b/docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh
new file mode 100755
index 0000000..b402107
--- /dev/null
+++ b/docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh
@@ -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
-- 
1.7.11.7

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

[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 <tomoki.sekiyama.qu@hitachi.com>
---
 .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

diff --git a/.gitignore b/.gitignore
index bd6ba1c..867cb86 100644
--- a/.gitignore
+++ b/.gitignore
@@ -68,6 +68,7 @@ fsdev/virtfs-proxy-helper.pod
 *.tp
 *.vr
 *.d
+!fsfreeze.d
 *.o
 *.lo
 *.la
diff --git a/docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh b/docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh
new file mode 100755
index 0000000..1704fb0
--- /dev/null
+++ b/docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh
@@ -0,0 +1,41 @@
+#!/bin/bash
+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
+	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
+	while :; do
+		INNODB_STATUS=/tmp/mysql-innodb.status
+		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`
+		rm $INNODB_STATUS
+		[ $LOG_CURRENT = $LOG_FLUSHED ] && break
+		sleep 1
+	done
+
+elif [ "$1" = "thaw" ]; then
+
+	if [ -p $FIFO ]; then
+		echo > $FIFO
+		rm $FIFO
+	fi
+
+fi
+
diff --git a/docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh b/docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh
new file mode 100755
index 0000000..b402107
--- /dev/null
+++ b/docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh
@@ -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
-- 
1.7.11.7

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

[Eric Blake]

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

On 11/08/2012 05:36 AM, Tomoki Sekiyama wrote:
> 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 <tomoki.sekiyama.qu@hitachi.com>
> ---
> +++ b/docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh
> @@ -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?

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

> +	flush_and_wait | $MYSQL &
> +	# wait until every block is flushed
> +	while [ "`echo 'SHOW STATUS LIKE "Key_blocks_not_flushed"' |\

I prefer $() over ``

> +		$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.

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

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

> +		sleep 1
> +	done
> +
> +elif [ "$1" = "thaw" ]; then
> +
> +	if [ -p $FIFO ]; then
> +		echo > $FIFO
> +		rm $FIFO
> +	fi
> +
> +fi
> +
> diff --git a/docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh b/docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh
> new file mode 100755
> index 0000000..b402107
> --- /dev/null
> +++ b/docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh
> @@ -0,0 +1,17 @@
> +#!/bin/bash

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

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

> +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" "$@"

> +done
> 

-- 
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: 617 bytes --]

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

[Eric Blake]

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

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]

> 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 <tomoki.sekiyama.qu@hitachi.com>
> ---

> @@ -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)) {

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

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

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

> +                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().

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

> +            } 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.

> +            }
> +        }
> +    }
> +    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.

-- 
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: 617 bytes --]

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

[Tomoki Sekiyama]

Hi Eric, thanks for your review.

On 2012/11/09 3:38, Eric Blake wrote:
> 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.

>> 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 <tomoki.sekiyama.qu@hitachi.com>
>> ---
> 
>> @@ -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)) {
> 
> 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>
>> +                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.

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

> 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,
-- 
Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com>
Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory

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

[Tomoki Sekiyama]

On 2012/11/09 3:25, Eric Blake wrote:
> On 11/08/2012 05:36 AM, Tomoki Sekiyama wrote:
>> 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 <tomoki.sekiyama.qu@hitachi.com>
>> ---
>> +++ b/docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh
>> @@ -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.

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

I will add the check.

>> +	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 $().

>> +		$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.

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

>> +		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>
>> diff --git a/docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh b/docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh
>> new file mode 100755
>> index 0000000..b402107
>> --- /dev/null
>> +++ b/docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh
>> @@ -0,0 +1,17 @@
>> +#!/bin/bash
> 
> Again, I see no bash-isms, why not use /bin/sh.

I will use /bin/sh here too.

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

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

>> +done

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

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