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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2930 bytes --]

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/

> +
> +# 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?

> +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.

> +    "$file" "$@" >>$LOGFILE 2>&1
> +    STATUS=$?
> +    echo "$(date): $file finished with status=$STATUS" >>$LOGFILE

Again.


> @@ -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.

> +        # 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?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]