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