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

[Eric Blake <eblake@redhat.com>]

[-- 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 --]