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

[Tomoki Sekiyama]

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 <address@hidden>
>> ---
>> +++ 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 <address@hidden>
Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory