Some questions

[Joerg Sommer <joerg@alea.gnuu.de>]

Hi,

I've downloaded hotplug-2003_08_05 and have some questions about the code:

* Why not remove the bashism?

sbin/hotplug:
* Why not include the test for -x in the if expression?
  	if [ -f $I -a -x $I ]; then
		$I $1 ;
	fi
* Why returns this script with exit code 1? Doesn't this stand for an
  error?

#v+
diff -ur hotplug-2003_08_05.orig/sbin/hotplug hotplug-2003_08_05/sbin/hotplug
--- hotplug-2003_08_05.orig/sbin/hotplug        2003-06-28 02:13:10.000000000 +0
+++ hotplug-2003_08_05/sbin/hotplug     2003-08-19 16:21:38.000000000 +0200
@@ -28,8 +28,8 @@
 DIR="/etc/hotplug.d"
 
 for I in "${DIR}/$1/"*.hotplug "${DIR}/"default/*.hotplug ; do
-       if [ -f $I ]; then
-               test -x $I && $I $1 ;
+       if [ -f $I -a -x $I ]; then
+               $I $1
        fi
 done
 
#v-

etc/hotplug.d/default/default.hotplug:
* l.46: Why use sed to strip the tailing .agent? This causes a pipe
  and another command, even though, this could be done by basename.
* l.67: Why do you debug handling by hand? There is a function
  debug_mesg, so this shouldn't be necessary.
* If default.hotplug is called in the right way, you check, if the agent
  is executable. If not, it fails. So you can't name nonexecutable agents
  in l.53.
* I think a $(set -e) is essential, because a cd to /etc/hotplug could
  fail.
* To source hotplug.functions a path must be named, otherwise
  hotplug.functions is searched in the directories of $PATH, which should
  fail. See Posix 1003.2 3.14.4.

#v+
diff -ur hotplug-2003_08_05.orig/etc/hotplug.d/default/default.hotplug hotplug-2003_08_05/etc/hotplug.d/default/default.hotplug
--- hotplug-2003_08_05.orig/etc/hotplug.d/default/default.hotplug	2003-05-02 07:01:45.000000000 +0200
+++ hotplug-2003_08_05/etc/hotplug.d/default/default.hotplug	2003-08-19 16:26:07.000000000 +0200
@@ -23,10 +23,12 @@
 # $Id: default.hotplug,v 1.1 2003/05/02 05:01:45 kroah Exp $
 #
 
-cd /etc/hotplug
-. hotplug.functions
+set -e
+
+# export DEBUG=yes
 
-# DEBUG=yes export DEBUG
+cd /etc/hotplug
+. ./hotplug.functions
 
 debug_mesg "arguments ($*) env (`env`)"
 
@@ -38,19 +40,16 @@
 # through argv.
 # 
 if [ $# -lt 1 -o "$1" = "help" -o "$1" = "--help" ]; then
-    if [ -t ]; then
+    if [ -t 1 ]; then
 	echo "Usage: $0 AgentName [AgentArguments]"
 
-	AGENTS=""
-	for AGENT in /etc/hotplug/*.agent ; do
-	    TYPE=`basename $AGENT | sed s/.agent//`
+	printf "AgentName values on this system:"
+	for AGENT in $HOTPLUG_DIR/*.agent ; do
 	    if [ -x $AGENT ]; then
-		AGENTS="$AGENTS $TYPE"
-	    else
-		AGENTS="$AGENTS ($TYPE)"
+		printf " `basename $AGENT .agent`"
 	    fi
 	done
-	echo "AgentName values on this system: $AGENTS" 
+	echo
     else
 	mesg "illegal usage $*"
     fi
@@ -61,12 +60,10 @@
 # Delegate event handling:
 #   /sbin/hotplug FOO ..args.. ==> /etc/hotplug/FOO.agent ..args..
 #
-AGENT=/etc/hotplug/$1.agent
+AGENT=$HOTPLUG_DIR/$1.agent
 if [ -x $AGENT ]; then
     shift
-    if [ "$DEBUG" != "" ]; then
-	mesg "invoke $AGENT ($@)"
-    fi
+    debug_mesg "invoke $AGENT ($@)"
     exec $AGENT "$@"
     mesg "couldn't exec $AGENT"
     exit 1
#v-

etc/hotplug/hotplug.functions:
* Posix logger doesn't know the option -t. If you depend on BSD logger,
  it may be possible to specify the priority of the messages. In this
  case, I would propose to add a function error_mesg(), hat logs on
  stderr or use level debug for logger. As a whole, I would propose in
  this case, to log to facility daemon.

#v+
diff -ur hotplug-2003_08_05.orig/etc/hotplug/hotplug.functions hotplug-2003_08_05/etc/hotplug/hotplug.functions
--- hotplug-2003_08_05.orig/etc/hotplug/hotplug.functions	2003-06-28 02:13:10.000000000 +0200
+++ hotplug-2003_08_05/etc/hotplug/hotplug.functions	2003-08-19 17:36:55.000000000 +0200
@@ -10,7 +10,7 @@
 #
 #
 
-# DEBUG=yes; export DEBUG
+# export DEBUG=yes
 PATH=/bin:/sbin:/usr/sbin:/usr/bin
 
 KERNEL=`uname -r`
@@ -22,31 +22,31 @@
     . /etc/sysconfig/hotplug
 fi
 
-if [ -x /usr/bin/logger ]; then
-    LOGGER=/usr/bin/logger
-elif [ -x /bin/logger ]; then
-    LOGGER=/bin/logger
-else
-    unset LOGGER
-fi
 #
 # for diagnostics
 #
-if [ -t -o -z "$LOGGER" ]; then
+if [ -t 1 -o ! \( -x /usr/bin/logger -o -x /bin/logger \) ]; then
     mesg () {
 	echo "$@"
     }
+    if [ "$DEBUG" = yes ]; then
+	debug_mesg () {
+	    echo "$@"
+	}
+    else
+	debug_mesg () {
+	    :
+	}
+    fi
 else
     mesg () {
-	$LOGGER -t $(basename $0)"[$$]" "$@"
+	logger $(basename $0)"[$$] $@"
+    }
+    debug_mesg () {
+	logger $(basename $0)"[$$] $@"
     }
 fi
 
-debug_mesg () {
-    test "$DEBUG" = "" -o "$DEBUG" = no && return
-    mesg "$@"
-}
-
 #
 # The modules.*map parsing uses BASH ("declare -i") and some version
 # of AWK, typically /bin/gawk.  Most GNU/Linux distros have these,
@@ -80,23 +80,18 @@
 #
 load_drivers ()
 {
-    local LOADED TYPE FILENAME DESCRIPTION LISTER
-    DRIVERS=""
-
     # make this routine more readable
     TYPE=$1
     FILENAME=$2
     DESCRIPTION=$3
 
+    DRIVERS=""
     # should we use usbmodules, pcimodules?  not on 2.5+, because sysfs
     # ought to expose the data we need to find all candidate drivers.
     # (on 2.5.48 it does for usb; but maybe not yet for pci.)
     case "$KERNEL" in
-    2.2*|2.3*|2.4*)	LISTER=`type -p ${TYPE}modules` ;;
-    *)			LISTER="" ;;
-    esac
-
-    if [ "$LISTER" != "" ]; then
+    2.2*|2.3*|2.4*)
+        LISTER=`type -p ${TYPE}modules`
 	# lister programs MIGHT be preferable to parsing from shell scripts:
 	# - usbmodules used for (a) multi-interface devices, (b) coldplug
 	# - pcimodules used only for coldplug
@@ -106,27 +101,26 @@
 	    # only works if we have usbfs
 	    # ... reads more descriptors than are passed in env
 	    # ... doesn't handle comment syntax either
-	    if [ "$DEVICE" = "" -o ! -f "$DEVICE" ]; then
-		LISTER=
-	    else
+	    if [ -f "$DEVICE" ]; then
 		DRIVERS=`$LISTER --mapfile $FILENAME --device $DEVICE`
-	    fi ;;
-
+	    fi
+	    ;;
 	pci)
 	    debug_mesg "pcimodules is scanning more than $PCI_SLOT ..."
 	    DRIVERS=`$LISTER`
 	    ;;
 	esac
-    fi
+	;;
+    esac
 
     # try parsing by shell scripts if no luck yet
-    if [ "$DRIVERS" = "" ]; then
+    if [ -z "$DRIVERS" ]; then
 	${TYPE}_map_modules < $FILENAME
     fi
 
     # FIXME remove dups and blacklisted modules from $DRIVERS here
 
-    if [ "$DRIVERS" = "" ]; then
+    if [ -z "$DRIVERS" ]; then
 	return
     fi
 
@@ -139,9 +133,8 @@
     do
 	# maybe driver modules need loading
         LOADED=false
-	if ! lsmod | grep -q "^$MODULE " > /dev/null 2>&1; then
-	    if grep -q "^$MODULE\$" $HOTPLUG_DIR/blacklist \
-		    >/dev/null 2>&1; then
+	if ! lsmod | grep -q "^$MODULE "; then
+	    if grep -q "^$MODULE\$" $HOTPLUG_DIR/blacklist; then
 		debug_mesg "... blacklisted module:  $MODULE"
 		continue
 	    fi
@@ -167,8 +160,7 @@
 	# giving kernel code another chance.
 	if [ -x $HOTPLUG_DIR/$TYPE/$MODULE ]; then
 	    debug_mesg Module setup $MODULE for $DESCRIPTION
-	    $HOTPLUG_DIR/$TYPE/$MODULE
-	    LOADED=true
+	    $HOTPLUG_DIR/$TYPE/$MODULE && LOADED=true
 	fi
 
 	if [ $LOADED = false ]; then
#v-

Bye, Joerg.

-- 
»Perl - the only language that looks the same
 before and after RSA encryption.«           -- Keith Bostic


-------------------------------------------------------
This SF.net email is sponsored by Dice.com.
Did you know that Dice has over 25,000 tech jobs available today? From
careers in IT to Engineering to Tech Sales, Dice has tech jobs from the
best hiring companies. http://www.dice.com/index.epl?rel_code\x104
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel