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