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

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