[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Cluster-devel] [PATCH] gfs2 init script

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.

@@ -15,6 +15,15 @@
# Description:         mount/unmount gfs2 filesystems configured
in /etc/fstab

+# set secure PATH

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;

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

-    echo -ne "[  OK  ]\r"
+function local_success() {
+    echo -ne "[  OK  ]\n"

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


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

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]