dkms - module build reliability patch

[Matt Domsch]

On Tue, May 29, 2007 at 12:48:10PM +0400, Vladimir Simonov wrote:
> Hello Matt,
> 
> We have been using dkms a couple years and have
> experience with its work on width range of distributions.
> 
> We'd like to incorporate our patches into dkms mainstream.
> For brief review and discussion I've summarized them into
> single patch (dkms-final.patch). All others are its chunks.
> The patches should be applied against dkms 2.0.16.1.

Thanks for the great writeup and patches.  Here's my initial take:


> Brief description.
> 
> 1. dkms-sbin-in-path - adds /sbin & /usr/sbin into PATH.
> It fixes "dkms build ..." via sudo work. Without it
> depmod can't be found. Checking for sbin in PATH is not
> too reliable but enough in our opinion.

> +if [ -n `echo $PATH |grep /sbin` ]; then
> +  PATH=$PATH:/sbin:/usr/sbin
> +fi
> +

yeah, not too reliable.  Would be better to parse $PATH properly.  Why
add /usr/sbin if depmod is always in /sbin ?  Maybe just change the
calls to depmod to be /sbin/depmod ?  There are only 3 invocations of it.


 
> 2. dkms-nomktemp.patch - some distributions do not
> support "mktemp -d". Function mktempdir uses "mktemp -d"
> if possible.

+function mktempdir ()
...
+    tmp="`mktemp $1 2> /dev/null`"
+    [ $? -ne 0 ] && return 1
+    rm -f $tmp
+    mkdir -p $tmp
+    echo $tmp
+    return 0

This is still racy.  What OS do you have that doesn't know 'mktemp
-d'?  Michael Brown's got a cleaner way, but relies on mkdir having a
-m 0700 option, which if you don't have mktemp -d, you may not have
mkdir -m.  Do you?  My Red Hat Linux 7.3 server had mkdir -m and
mktemp -d.  I don't have anything older anymore to look.

 
> 3. dkms-symlink_check - adds some harmless checks.
> I don't remember the problem related to this patch
> but it really existed :).

-	if ! [ -L "$dkms_tree/$module/kernel-${kernelver_array[0]}-${arch_array[0]}" ]; then
+	if ! [ -L "$dkms_tree/$module/kernel-${kernelver_array[0]}-${arch_array[0]}" \
+	    -a -d "$dkms_tree/$module/kernel-${kernelver_array[0]}-${arch_array[0]}" ]; then


Not really harmless.  [ -L foo -a -d foo ] will always evaluate to
false, as it can't be both a symlink and a directory.  So this if/then
path is always taken.

-	elif [ -L "$dkms_tree/$module/kernel-${kernelver_array[0]}-${arch_array[0]}" \
                -a -e "$dkms_tree/$module/original_module/${kernelver_array[0]}/${arch_array[0]}/${dest_module_name[$count]}$module_suffix" ]; then
+	elif [ -L "$dkms_tree/$module/kernel-${kernelver_array[0]}-${arch_array[0]}" \
+		-a -d "$dkms_tree/$module/kernel-${kernelver_array[0]}-${arch_array[0]}" \
+		-a -e "$dkms_tree/$module/original_module/${kernelver_array[0]}/${arch_array[0]}/${dest_module_name[$count]}$module_suffix" ]; then

Likewise here, -L and -d are exclusive, thus this is always false.


 
> 4. dkms-no-prepare-on-suse - fixes build on SuSE 9.1.
> Really, the removed code looks very artificial fo me.

-    if [[ (! ( $(VER $1) < $(VER 2.6.5) ) || (-d /etc/SuSEconfig)) && \
-       -d "$kernel_source_dir" && \
-       -z "$ksourcedir_fromcli" ]]; then
-	echo $""
-	echo $"Kernel preparation unnecessary for this kernel.  Skipping..."
-	no_clean_kernel="no-clean-kernel"
-	return 1
-    fi

The idea was, fixes for make prepare-all to enable building
out-of-tree modules was added in 2.6.5, at our request. Kernels less
than this needed to run make prepare-all.  But, SLES9 got the fix too
(hence, the -d /etc/SuSEconfig).

We could perhaps do something special for SuSE 9.1 here...

 
> 5. dkms-config-priority-to21 - the most complex and probably
> the most questionable patch. It introduces:
> 5.1 Support for 2.6.21 kernel - the kernel depricates
> prepare-all make option.

that's fair.

> 5.2 The following logic for kernel config file detection:
> a) Never guess config file name if it is specified
> in command line option.

Agreed.
 
> If no config file name specified then
> b) If .config file exists then use it witout any further
> guessing.
> c) If /proc/config.gz is found - use it (and unpack it if
> necessary).
> d) If /boot/config-KERNEL_VERSION exists - use it.

Seems sane.  I need to read the code closely to make sure this is what
it does; that section is pretty unreadable before your patch is
applied; it's not any prettier afterwards. :-)


Thanks for the patches.  I hope you can address the comments above.

-Matt

-- 
Matt Domsch
Software Architect
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com