[Cluster-devel] [PATCH] gfs2 init script

[Fabio M. Di Nitto <fdinitto@redhat.com>]

Hi Marian,

thanks for your patch. I have a few comments on it.

git-am ../init.diff
Applying gfs2 init script
fatal: corrupt patch at line 18
Patch failed at 0001.

first I can't apply the patch.. the patch is corrupted. So I can only 
comment on what I can read from it but I was not able to test it.

> diff --git a/gfs2/init.d/gfs2.in b/gfs2/init.d/gfs2.in
> old mode 100644
> new mode 100755
> index 35688a0..cb9bee9
> --- a/gfs2/init.d/gfs2.in
> +++ b/gfs2/init.d/gfs2.in
> @@ -2,7 +2,7 @@
> #
> # gfs2 mount/unmount helper
> #
> -# chkconfig: - 26 74
> +# chkconfig: 2345 26 74
> # description: mount/unmount gfs2 filesystems configured in /etc/fstab

This is a "no-no". most distribution have slightly different 
interpretations of what runlevels should do. Let them decide where they 
want to run the scripts. Running by default has also a danger of making 
the boot problematic if somebody installed an RPM without configuring the 
stack properly. Better that they enable it after testing.

>
> ### BEGIN INIT INFO
> @@ -15,6 +15,15 @@
> # Description:         mount/unmount gfs2 filesystems configured
> in /etc/fstab
> ### END INIT INFO
>
> +# set secure PATH
> +PATH="/bin:/usr/bin:/sbin:/usr/sbin"

While this is generally a good point, we might have to add $sbindir from 
the build system here. There is no guarantee that our binaries or system 
binaries are installed in those paths (/usr/local... /opt..) etc.

> +
> +# Check privileges
> +if [ "$1" != 'status' ] && [ "$(whoami)" != 'root' ]; then
> +       echo "You are not allowed to run this script."
> +       exit 4;
> +fi

This is another "no-no". It's entirely possible to delegate mounting of 
disks to unprivilged users or users that are not root. There are system 
groups like "disk" for it as well. The number of checks to do are simply 
too complex with very little benefit for an init script.

mount will simply fail if you are not allowed to do the operation.

> +
> # rpm based distros
> if [ -d /etc/sysconfig ]; then
>        [ -f @INITDDIR@/functions ] && . @INITDDIR@/functions
> @@ -34,14 +43,16 @@ if [ -d /etc/default ]; then
>        failure=failure
> fi
>
> -local_success()
> -{
> -    echo -ne "[  OK  ]\r"
> +function local_success() {
> +    echo -ne "[  OK  ]\n"
> }
>
> -local_failure()
> -{
> +function local_failure() {
>     echo -ne "[FAILED]\r"
> +       # if we have exit code, use it
> +       if [ "$1" = [0-9] ]; then
> +               exit $1
> +       fi
> }

Careful here. local_failure is the equivalent of failure in 
/etc/init.d/functions. It only performs an echo. So should this one. The 
return code should be issues after by the script and not embedded (see 
also the other comments before about using local_*

> +function start_gfs() {
> +       # check if we have /proc/modules
> +       # check if gfs2 module is loaded
> +       # check if we have gfs2 as a module
> +       if [ -f /proc/modules ] &&
> +               ( ! grep gfs2 /proc/modules > /dev/null ) &&
> +               ( modprobe -l|grep gfs2 > /dev/null ); then
> +                       echo -n "Loading gfs2 module: "
> +                       if ( modprobe gfs2 ); then
> +                               local_success
> +                       else
> +                               local_failure 6
> +                       fi

both local_success and local_failure should be $success and $failure.

We are aliasing them according to the distro we are running. rpm based 
distro have those functions embedded in /etc/init.d/functions. and they do 
"pretty output".

In case of deb based distros, then there is no such thing. So we alias 
them to the local version.

I did try to be careful not to change behaviour from system and local. I 
would prefer to respect that from this patch as well.

> +       if ( modprobe -r gfs2 && rm -f $LOCK_FILE ) ; then
> +               exit 0
> +       else
> +               exit 1
> +       fi
> +}

I really don't think you want to fail if you can't remove the LOCK_FILE or 
the module. Try to do both, but it's not a condition where you want to 
return an error.

Can you plese resend it as attachment? maybe either your or my MUA is 
corrupting it.

Thanks
Fabio

--
I'm going to make him an offer he can't refuse.