Patchwork [2/2] qemu-ga: sample fsfreeze-script

login
register
mail settings
Submitter Tomoki Sekiyama
Date Nov. 8, 2012, 12:36 p.m.
Message ID <509BA754.3090403@hitachi.com>
Download mbox | patch
Permalink /patch/197830/
State New
Headers show

Comments

Tomoki Sekiyama - Nov. 8, 2012, 12:36 p.m.
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
Eric Blake - Nov. 8, 2012, 6:25 p.m.
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
>
Tomoki Sekiyama - Nov. 12, 2012, 4:10 a.m.
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,

Patch

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