From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:37444) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ThwlK-0000ON-Lr for qemu-devel@nongnu.org; Mon, 10 Dec 2012 01:22:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ThwlJ-0003tU-AA for qemu-devel@nongnu.org; Mon, 10 Dec 2012 01:22:46 -0500 Received: from mail4.hitachi.co.jp ([133.145.228.5]:50149) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ThwlI-0003sv-Qq for qemu-devel@nongnu.org; Mon, 10 Dec 2012 01:22:45 -0500 Message-ID: <50C57FE5.4020705@hitachi.com> Date: Mon, 10 Dec 2012 15:23:33 +0900 From: Tomoki Sekiyama MIME-Version: 1.0 References: <20121207083922.10624.97583.stgit@melchior2.sdl.hitachi.co.jp> <20121207083932.10624.8173.stgit@melchior2.sdl.hitachi.co.jp> <50C236CA.5050801@redhat.com> In-Reply-To: <50C236CA.5050801@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v7 2/2] qemu-ga: sample fsfreeze hooks List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: eblake@redhat.com Cc: lcapitulino@redhat.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com 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 >> --- > >> @@ -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 Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory