From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:33361) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TXlLc-00050N-TW for qemu-devel@nongnu.org; Sun, 11 Nov 2012 23:10:11 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TXlLZ-0006Ch-Ib for qemu-devel@nongnu.org; Sun, 11 Nov 2012 23:10:08 -0500 Received: from mail4.hitachi.co.jp ([133.145.228.5]:56873) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TXlLZ-00067P-0m for qemu-devel@nongnu.org; Sun, 11 Nov 2012 23:10:05 -0500 Message-ID: <50A076BF.5040500@hitachi.com> Date: Mon, 12 Nov 2012 13:10:39 +0900 From: Tomoki Sekiyama MIME-Version: 1.0 References: <509B9FEF.4040604@hitachi.com> <509BA754.3090403@hitachi.com> <509BF900.2050700@redhat.com> In-Reply-To: <509BF900.2050700@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] qemu-ga: sample fsfreeze-script List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: eblake@redhat.com Cc: qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com 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 >> --- >> +++ 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. >> 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 Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory