Patchwork [v7,2/2] qemu-ga: sample fsfreeze hooks

login
register
mail settings
Submitter Tomoki Sekiyama
Date Dec. 7, 2012, 8:39 a.m.
Message ID <20121207083932.10624.8173.stgit@melchior2.sdl.hitachi.co.jp>
Download mbox | patch
Permalink /patch/204426/
State New
Headers show

Comments

Tomoki Sekiyama - Dec. 7, 2012, 8:39 a.m.
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>
---
 .gitignore                                         |    1 
 Makefile                                           |    2 -
 scripts/qemu-guest-agent/fsfreeze-hook             |   33 ++++++++++++
 .../fsfreeze-hook.d/mysql-flush.sh.sample          |   55 ++++++++++++++++++++
 4 files changed, 90 insertions(+), 1 deletion(-)
 create mode 100755 scripts/qemu-guest-agent/fsfreeze-hook
 create mode 100755 scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample
Luiz Capitulino - Dec. 7, 2012, 1:55 p.m.
On Fri, 07 Dec 2012 17:39:32 +0900
Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com> 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>
> ---
>  .gitignore                                         |    1 
>  Makefile                                           |    2 -
>  scripts/qemu-guest-agent/fsfreeze-hook             |   33 ++++++++++++
>  .../fsfreeze-hook.d/mysql-flush.sh.sample          |   55 ++++++++++++++++++++
>  4 files changed, 90 insertions(+), 1 deletion(-)
>  create mode 100755 scripts/qemu-guest-agent/fsfreeze-hook
>  create mode 100755 scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample
> 
> diff --git a/.gitignore b/.gitignore
> index bd6ba1c..286822d 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -93,3 +93,4 @@ cscope.*
>  tags
>  TAGS
>  *~
> +!scripts/qemu-guest-agent/fsfreeze-hook.d

Why? Do we expect to have *~ files in there?

> diff --git a/Makefile b/Makefile
> index 9ecbcbb..466dcd7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -248,7 +248,7 @@ clean:
>  # avoid old build problems by removing potentially incorrect old files
>  	rm -f config.mak op-i386.h opc-i386.h gen-op-i386.h op-arm.h opc-arm.h gen-op-arm.h
>  	rm -f qemu-options.def
> -	find . -name '*.[od]' -exec rm -f {} +
> +	find . -name '*.[od]' -type f -exec rm -f {} +

What does this change have to do with this patch?

>  	rm -f *.a *.lo $(TOOLS) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod *~ */*~
>  	rm -Rf .libs
>  	rm -f qemu-img-cmds.h
> diff --git a/scripts/qemu-guest-agent/fsfreeze-hook b/scripts/qemu-guest-agent/fsfreeze-hook
> new file mode 100755
> index 0000000..4f7ff15
> --- /dev/null
> +++ b/scripts/qemu-guest-agent/fsfreeze-hook
> @@ -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
> +# request, it is issued with "thaw" argument after filesystem is thawed.
> +
> +LOGFILE=/var/log/qga-fsfreeze-hook.log
> +FSFREEZE_D=$(dirname -- "$0")/fsfreeze-hook.d
> +
> +# Check whether file $1 is a backup or rpm-generated file and should be ignored
> +is_ignored_file() {
> +    case "$1" in
> +        *~ | *.bak | *.orig | *.rpmnew | *.rpmorig | *.rpmsave | *.sample)
> +            return 0 ;;
> +    esac
> +    return 1
> +}
> +
> +# Iterate executables in directory "fsfreeze-hook.d" with the specified args
> +[ ! -d "$FSFREEZE_D" ] && exit 1
> +for file in "$FSFREEZE_D"/* ; do
> +    is_ignored_file "$file" && continue
> +    [ -x "$file" ] || continue
> +    echo "$(date): execute $file $@" >>$LOGFILE
> +    "$file" "$@" >>$LOGFILE 2>&1
> +    STATUS=$?
> +    echo "$(date): $file finished with status=$STATUS" >>$LOGFILE
> +done
> +
> +exit 0
> diff --git a/scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample b/scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample
> new file mode 100755
> index 0000000..c69b8ad
> --- /dev/null
> +++ b/scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample
> @@ -0,0 +1,55 @@
> +#!/bin/sh
> +
> +# Flush MySQL tables to the disk before the filesystem is freezed.
> +# 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
> +
> +flush_and_wait() {
> +    printf "FLUSH TABLES WITH READ LOCK \\G\n"
> +    read < $FIFO
> +    printf "UNLOCK TABLES \\G\n"
> +}
> +
> +case "$1" in
> +    freeze)
> +        mkfifo $FIFO || exit 1
> +        flush_and_wait | $MYSQL_CMD &
> +        # wait until every block is flushed
> +        while [ "$(echo 'SHOW STATUS LIKE "Key_blocks_not_flushed"' |\
> +                 $MYSQL_CMD | tail -1 | cut -f 2)" -gt 0 ]; do
> +            sleep 1
> +        done
> +        # 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
> +        while :; do
> +            printf "SHOW ENGINE INNODB STATUS \\G" | $MYSQL_CMD > $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)
> +            [ "$LOG_CURRENT" = "$LOG_FLUSHED" ] && break
> +            sleep 1
> +        done
> +        rm -f $INNODB_STATUS
> +        ;;
> +
> +    thaw)
> +        [ ! -p $FIFO ] && exit 1
> +        echo > $FIFO
> +        rm -f $FIFO
> +        ;;
> +
> +    *)
> +        exit 1
> +        ;;
> +esac
>
Michael Roth - Dec. 7, 2012, 5:47 p.m.
On Fri, Dec 07, 2012 at 11:55:16AM -0200, Luiz Capitulino wrote:
> On Fri, 07 Dec 2012 17:39:32 +0900
> Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com> 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>
> > ---
> >  .gitignore                                         |    1 
> >  Makefile                                           |    2 -
> >  scripts/qemu-guest-agent/fsfreeze-hook             |   33 ++++++++++++
> >  .../fsfreeze-hook.d/mysql-flush.sh.sample          |   55 ++++++++++++++++++++
> >  4 files changed, 90 insertions(+), 1 deletion(-)
> >  create mode 100755 scripts/qemu-guest-agent/fsfreeze-hook
> >  create mode 100755 scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample
> > 
> > diff --git a/.gitignore b/.gitignore
> > index bd6ba1c..286822d 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -93,3 +93,4 @@ cscope.*
> >  tags
> >  TAGS
> >  *~
> > +!scripts/qemu-guest-agent/fsfreeze-hook.d
> 
> Why? Do we expect to have *~ files in there?

I think default vim configurations will copy <file> to <file>~ prior to
editing.

> 
> > diff --git a/Makefile b/Makefile
> > index 9ecbcbb..466dcd7 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -248,7 +248,7 @@ clean:
> >  # avoid old build problems by removing potentially incorrect old files
> >  	rm -f config.mak op-i386.h opc-i386.h gen-op-i386.h op-arm.h opc-arm.h gen-op-arm.h
> >  	rm -f qemu-options.def
> > -	find . -name '*.[od]' -exec rm -f {} +
> > +	find . -name '*.[od]' -type f -exec rm -f {} +
> 
> What does this change have to do with this patch?

Looks like it'll try (and fail, due to lack of -r option to rm) to
delete the fsfreeze-hook.d directory on `make clean`. For in-tree builds
at least.
Eric Blake - Dec. 7, 2012, 6:22 p.m.
On 12/07/2012 06:55 AM, Luiz Capitulino wrote:
> On Fri, 07 Dec 2012 17:39:32 +0900
> Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com> 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>
>> ---
>>  .gitignore                                         |    1 
>>  Makefile                                           |    2 -
>>  scripts/qemu-guest-agent/fsfreeze-hook             |   33 ++++++++++++
>>  .../fsfreeze-hook.d/mysql-flush.sh.sample          |   55 ++++++++++++++++++++
>>  4 files changed, 90 insertions(+), 1 deletion(-)
>>  create mode 100755 scripts/qemu-guest-agent/fsfreeze-hook
>>  create mode 100755 scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample
>>
>> diff --git a/.gitignore b/.gitignore
>> index bd6ba1c..286822d 100644
>> --- a/.gitignore
>> +++ b/.gitignore
>> @@ -93,3 +93,4 @@ cscope.*
>>  tags
>>  TAGS
>>  *~
>> +!scripts/qemu-guest-agent/fsfreeze-hook.d
> 
> Why? Do we expect to have *~ files in there?

What does your question have to do with the patch, which isn't even
touching the pre-existing *~ line?

> 
>> diff --git a/Makefile b/Makefile
>> index 9ecbcbb..466dcd7 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -248,7 +248,7 @@ clean:
>>  # avoid old build problems by removing potentially incorrect old files
>>  	rm -f config.mak op-i386.h opc-i386.h gen-op-i386.h op-arm.h opc-arm.h gen-op-arm.h
>>  	rm -f qemu-options.def
>> -	find . -name '*.[od]' -exec rm -f {} +
>> +	find . -name '*.[od]' -type f -exec rm -f {} +
> 
> What does this change have to do with this patch?

Because this patch introduces a directory
scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample that
belongs to the repo and therefore must not be cleaned.
Luiz Capitulino - Dec. 7, 2012, 6:29 p.m.
On Fri, 7 Dec 2012 11:47:24 -0600
mdroth <mdroth@linux.vnet.ibm.com> wrote:

> On Fri, Dec 07, 2012 at 11:55:16AM -0200, Luiz Capitulino wrote:
> > On Fri, 07 Dec 2012 17:39:32 +0900
> > Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com> 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>
> > > ---
> > >  .gitignore                                         |    1 
> > >  Makefile                                           |    2 -
> > >  scripts/qemu-guest-agent/fsfreeze-hook             |   33 ++++++++++++
> > >  .../fsfreeze-hook.d/mysql-flush.sh.sample          |   55 ++++++++++++++++++++
> > >  4 files changed, 90 insertions(+), 1 deletion(-)
> > >  create mode 100755 scripts/qemu-guest-agent/fsfreeze-hook
> > >  create mode 100755 scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample
> > > 
> > > diff --git a/.gitignore b/.gitignore
> > > index bd6ba1c..286822d 100644
> > > --- a/.gitignore
> > > +++ b/.gitignore
> > > @@ -93,3 +93,4 @@ cscope.*
> > >  tags
> > >  TAGS
> > >  *~
> > > +!scripts/qemu-guest-agent/fsfreeze-hook.d
> > 
> > Why? Do we expect to have *~ files in there?
> 
> I think default vim configurations will copy <file> to <file>~ prior to
> editing.

But should that be tracked by git specifically in that dir?

> > > diff --git a/Makefile b/Makefile
> > > index 9ecbcbb..466dcd7 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -248,7 +248,7 @@ clean:
> > >  # avoid old build problems by removing potentially incorrect old files
> > >  	rm -f config.mak op-i386.h opc-i386.h gen-op-i386.h op-arm.h opc-arm.h gen-op-arm.h
> > >  	rm -f qemu-options.def
> > > -	find . -name '*.[od]' -exec rm -f {} +
> > > +	find . -name '*.[od]' -type f -exec rm -f {} +
> > 
> > What does this change have to do with this patch?
> 
> Looks like it'll try (and fail, due to lack of -r option to rm) to
> delete the fsfreeze-hook.d directory on `make clean`. For in-tree builds
> at least.

I can't see it, but well, I don't want to hold this patch for minor things
either... It looks good for me apart from these two nitpicks.
Luiz Capitulino - Dec. 7, 2012, 6:31 p.m.
On Fri, 07 Dec 2012 11:22:54 -0700
Eric Blake <eblake@redhat.com> wrote:

> On 12/07/2012 06:55 AM, Luiz Capitulino wrote:
> > On Fri, 07 Dec 2012 17:39:32 +0900
> > Tomoki Sekiyama <tomoki.sekiyama.qu@hitachi.com> 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>
> >> ---
> >>  .gitignore                                         |    1 
> >>  Makefile                                           |    2 -
> >>  scripts/qemu-guest-agent/fsfreeze-hook             |   33 ++++++++++++
> >>  .../fsfreeze-hook.d/mysql-flush.sh.sample          |   55 ++++++++++++++++++++
> >>  4 files changed, 90 insertions(+), 1 deletion(-)
> >>  create mode 100755 scripts/qemu-guest-agent/fsfreeze-hook
> >>  create mode 100755 scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample
> >>
> >> diff --git a/.gitignore b/.gitignore
> >> index bd6ba1c..286822d 100644
> >> --- a/.gitignore
> >> +++ b/.gitignore
> >> @@ -93,3 +93,4 @@ cscope.*
> >>  tags
> >>  TAGS
> >>  *~
> >> +!scripts/qemu-guest-agent/fsfreeze-hook.d
> > 
> > Why? Do we expect to have *~ files in there?
> 
> What does your question have to do with the patch, which isn't even
> touching the pre-existing *~ line?

As far I could understand the ! prefix in gitignore documention, it's
changing the *~ meaning for the fsfreeze-hoo.d directory, and I wanted
to understand why.

> >> diff --git a/Makefile b/Makefile
> >> index 9ecbcbb..466dcd7 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -248,7 +248,7 @@ clean:
> >>  # avoid old build problems by removing potentially incorrect old files
> >>  	rm -f config.mak op-i386.h opc-i386.h gen-op-i386.h op-arm.h opc-arm.h gen-op-arm.h
> >>  	rm -f qemu-options.def
> >> -	find . -name '*.[od]' -exec rm -f {} +
> >> +	find . -name '*.[od]' -type f -exec rm -f {} +
> > 
> > What does this change have to do with this patch?
> 
> Because this patch introduces a directory
> scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample that
> belongs to the repo and therefore must not be cleaned.
Eric Blake - Dec. 7, 2012, 6:34 p.m.
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 - Dec. 7, 2012, 6:37 p.m.
On 12/07/2012 11:31 AM, Luiz Capitulino wrote:
>>>> +++ b/.gitignore
>>>> @@ -93,3 +93,4 @@ cscope.*
>>>>  tags
>>>>  TAGS
>>>>  *~
>>>> +!scripts/qemu-guest-agent/fsfreeze-hook.d
>>>
>>> Why? Do we expect to have *~ files in there?
>>
>> What does your question have to do with the patch, which isn't even
>> touching the pre-existing *~ line?
> 
> As far I could understand the ! prefix in gitignore documention, it's
> changing the *~ meaning for the fsfreeze-hoo.d directory, and I wanted
> to understand why.
> 

No, it is changing a much earlier line:

*.d

to say that this _particular_ .d is allowed to be committed.  It has
nothing to do with *~.

Still, I have to wonder if we really want to store these files in a .d
in the repository itself, or if we should store them under some other
file name and only at 'make install' time insert them into a .d at the
install destination.  It would clean up this confusion about the
.gitignore as well as the change to the 'find' command during 'make clean'.
Luiz Capitulino - Dec. 7, 2012, 6:57 p.m.
On Fri, 07 Dec 2012 11:37:44 -0700
Eric Blake <eblake@redhat.com> wrote:

> On 12/07/2012 11:31 AM, Luiz Capitulino wrote:
> >>>> +++ b/.gitignore
> >>>> @@ -93,3 +93,4 @@ cscope.*
> >>>>  tags
> >>>>  TAGS
> >>>>  *~
> >>>> +!scripts/qemu-guest-agent/fsfreeze-hook.d
> >>>
> >>> Why? Do we expect to have *~ files in there?
> >>
> >> What does your question have to do with the patch, which isn't even
> >> touching the pre-existing *~ line?
> > 
> > As far I could understand the ! prefix in gitignore documention, it's
> > changing the *~ meaning for the fsfreeze-hoo.d directory, and I wanted
> > to understand why.
> > 
> 
> No, it is changing a much earlier line:
> 
> *.d
> 
> to say that this _particular_ .d is allowed to be committed.  It has
> nothing to do with *~.

Ah, now it makes a lot of sense, thanks Eric.

It would be nice to move it right below *.d, to avoid stupid comments :)

> Still, I have to wonder if we really want to store these files in a .d
> in the repository itself, or if we should store them under some other
> file name and only at 'make install' time insert them into a .d at the
> install destination.  It would clean up this confusion about the
> .gitignore as well as the change to the 'find' command during 'make clean'.

Well, now that I understand it both ways are fine with me (ie. what you
suggest and what's been implemented in this patch).
Tomoki Sekiyama - Dec. 10, 2012, 6:23 a.m.
Hi, sorry for my late reply.

On 2012/12/08 3:57, Luiz Capitulino wrote:
> On Fri, 07 Dec 2012 11:37:44 -0700
> Eric Blake <eblake@redhat.com> wrote:
> 
>> On 12/07/2012 11:31 AM, Luiz Capitulino wrote:
>>>>>> +++ b/.gitignore
>>>>>> @@ -93,3 +93,4 @@ cscope.*
>>>>>>  tags
>>>>>>  TAGS
>>>>>>  *~
>>>>>> +!scripts/qemu-guest-agent/fsfreeze-hook.d
>>>>>
>>>>> Why? Do we expect to have *~ files in there?
>>>>
>>>> What does your question have to do with the patch, which isn't even
>>>> touching the pre-existing *~ line?
>>>
>>> As far I could understand the ! prefix in gitignore documention, it's
>>> changing the *~ meaning for the fsfreeze-hoo.d directory, and I wanted
>>> to understand why.
>>>
>>
>> No, it is changing a much earlier line:
>>
>> *.d
>>
>> to say that this _particular_ .d is allowed to be committed.  It has
>> nothing to do with *~.

Yes, it's for adding fsfreeze-hook.d into the repo.

> Ah, now it makes a lot of sense, thanks Eric.
> 
> It would be nice to move it right below *.d, to avoid stupid comments :)

And OK, I will move this there.

>> Still, I have to wonder if we really want to store these files in a .d
>> in the repository itself, or if we should store them under some other
>> file name and only at 'make install' time insert them into a .d at the
>> install destination.  It would clean up this confusion about the
>> .gitignore as well as the change to the 'find' command during 'make clean'.
> 
> Well, now that I understand it both ways are fine with me (ie. what you
> suggest and what's been implemented in this patch).
Then I'd like to keep current implementation.

Thanks,
Tomoki Sekiyama - Dec. 10, 2012, 6:23 a.m.
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,

Patch

diff --git a/.gitignore b/.gitignore
index bd6ba1c..286822d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -93,3 +93,4 @@  cscope.*
 tags
 TAGS
 *~
+!scripts/qemu-guest-agent/fsfreeze-hook.d
diff --git a/Makefile b/Makefile
index 9ecbcbb..466dcd7 100644
--- a/Makefile
+++ b/Makefile
@@ -248,7 +248,7 @@  clean:
 # avoid old build problems by removing potentially incorrect old files
 	rm -f config.mak op-i386.h opc-i386.h gen-op-i386.h op-arm.h opc-arm.h gen-op-arm.h
 	rm -f qemu-options.def
-	find . -name '*.[od]' -exec rm -f {} +
+	find . -name '*.[od]' -type f -exec rm -f {} +
 	rm -f *.a *.lo $(TOOLS) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod *~ */*~
 	rm -Rf .libs
 	rm -f qemu-img-cmds.h
diff --git a/scripts/qemu-guest-agent/fsfreeze-hook b/scripts/qemu-guest-agent/fsfreeze-hook
new file mode 100755
index 0000000..4f7ff15
--- /dev/null
+++ b/scripts/qemu-guest-agent/fsfreeze-hook
@@ -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
+# request, it is issued with "thaw" argument after filesystem is thawed.
+
+LOGFILE=/var/log/qga-fsfreeze-hook.log
+FSFREEZE_D=$(dirname -- "$0")/fsfreeze-hook.d
+
+# Check whether file $1 is a backup or rpm-generated file and should be ignored
+is_ignored_file() {
+    case "$1" in
+        *~ | *.bak | *.orig | *.rpmnew | *.rpmorig | *.rpmsave | *.sample)
+            return 0 ;;
+    esac
+    return 1
+}
+
+# Iterate executables in directory "fsfreeze-hook.d" with the specified args
+[ ! -d "$FSFREEZE_D" ] && exit 1
+for file in "$FSFREEZE_D"/* ; do
+    is_ignored_file "$file" && continue
+    [ -x "$file" ] || continue
+    echo "$(date): execute $file $@" >>$LOGFILE
+    "$file" "$@" >>$LOGFILE 2>&1
+    STATUS=$?
+    echo "$(date): $file finished with status=$STATUS" >>$LOGFILE
+done
+
+exit 0
diff --git a/scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample b/scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample
new file mode 100755
index 0000000..c69b8ad
--- /dev/null
+++ b/scripts/qemu-guest-agent/fsfreeze-hook.d/mysql-flush.sh.sample
@@ -0,0 +1,55 @@ 
+#!/bin/sh
+
+# Flush MySQL tables to the disk before the filesystem is freezed.
+# 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
+
+flush_and_wait() {
+    printf "FLUSH TABLES WITH READ LOCK \\G\n"
+    read < $FIFO
+    printf "UNLOCK TABLES \\G\n"
+}
+
+case "$1" in
+    freeze)
+        mkfifo $FIFO || exit 1
+        flush_and_wait | $MYSQL_CMD &
+        # wait until every block is flushed
+        while [ "$(echo 'SHOW STATUS LIKE "Key_blocks_not_flushed"' |\
+                 $MYSQL_CMD | tail -1 | cut -f 2)" -gt 0 ]; do
+            sleep 1
+        done
+        # 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
+        while :; do
+            printf "SHOW ENGINE INNODB STATUS \\G" | $MYSQL_CMD > $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)
+            [ "$LOG_CURRENT" = "$LOG_FLUSHED" ] && break
+            sleep 1
+        done
+        rm -f $INNODB_STATUS
+        ;;
+
+    thaw)
+        [ ! -p $FIFO ] && exit 1
+        echo > $FIFO
+        rm -f $FIFO
+        ;;
+
+    *)
+        exit 1
+        ;;
+esac