Message ID | 509BA754.3090403@hitachi.com |
---|---|
State | New |
Headers | show |
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.qu@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? > +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? > + flush_and_wait | $MYSQL & > + # wait until every block is flushed > + while [ "`echo 'SHOW STATUS LIKE "Key_blocks_not_flushed"' |\ I prefer $() over `` > + $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. > + 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 `` > + 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. > + sleep 1 > + done > + > +elif [ "$1" = "thaw" ]; then > + > + if [ -p $FIFO ]; then > + echo > $FIFO > + rm $FIFO > + fi > + > +fi > + > 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. > + > +# This script is executed when a guest agent receives fsfreeze-freeze and > +# fsfreeze-thaw command when it is specified in --fsfreeze-script/-F option > +# of qemu-ga or placed in default path (/etc/qemu/fsfreeze-script.sh). > +# When the agent receives fsfreeze-freeze command, the script is issued with > +# "freeze" argument before the filesystem is freezed.. And for fsfreeze-thaw, > +# it is issued with "thaw" argument after filesystem is thawed. > +# > +# This script iterates executables in directory "fsfreeze.d" with the > +# specified argument. > + > +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? > +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" "$@" > +done >
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.qu@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,
diff --git a/.gitignore b/.gitignore index bd6ba1c..867cb86 100644 --- a/.gitignore +++ b/.gitignore @@ -68,6 +68,7 @@ fsdev/virtfs-proxy-helper.pod *.tp *.vr *.d +!fsfreeze.d *.o *.lo *.la diff --git a/docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh b/docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh new file mode 100755 index 0000000..1704fb0 --- /dev/null +++ b/docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh @@ -0,0 +1,41 @@ +#!/bin/bash +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 + flush_and_wait | $MYSQL & + # wait until every block is flushed + while [ "`echo 'SHOW STATUS LIKE "Key_blocks_not_flushed"' |\ + $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 + 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` + rm $INNODB_STATUS + [ $LOG_CURRENT = $LOG_FLUSHED ] && break + sleep 1 + done + +elif [ "$1" = "thaw" ]; then + + if [ -p $FIFO ]; then + echo > $FIFO + rm $FIFO + fi + +fi + 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 + +# This script is executed when a guest agent receives fsfreeze-freeze and +# fsfreeze-thaw command when it is specified in --fsfreeze-script/-F option +# of qemu-ga or placed in default path (/etc/qemu/fsfreeze-script.sh). +# When the agent receives fsfreeze-freeze command, the script is issued with +# "freeze" argument before the filesystem is freezed.. And for fsfreeze-thaw, +# it is issued with "thaw" argument after filesystem is thawed. +# +# This script iterates executables in directory "fsfreeze.d" with the +# specified argument. + +cd `dirname $0` +cd fsfreeze.d +for x in *; do + [ -x ./$x ] && ./$x $1 +done
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.qu@hitachi.com> --- .gitignore | 1 + .../fsfreeze.d/mysql-flush.sh | 41 ++++++++++++++++++++++ .../fsfreeze-script-sample/fsfreeze.sh | 17 +++++++++ 3 files changed, 59 insertions(+) create mode 100755 docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.d/mysql-flush.sh create mode 100755 docs/qemu-guest-agent/fsfreeze-script-sample/fsfreeze.sh