dkms - module build reliability patch

[Vladimir Simonov]

Hello Matt,

Sorry for delay - was too busy.
Please see inline

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

a) We did not check dkms body for all required third party utilities
in all code pathes.
b) What if depmod is bash script on some distro and requires
other system command?
c) dkms is in /usr/sbin so it looks reasonable to add /usr/sbin
to PATH (if I remember correct - some kind of recursion was used
in ealier dkms versions).

So adding /sbin:/usr/sbin to PATH looks acceptable for system
utility. Probably even without any "if".

>>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.
Yes, but
1) We restore functionality - not fixing race;
2) Race probability is too small in my opinion,
BTW current dkms code does not check mktemp -d
exit code at all.

Probably somebody suggests better implementation
for mktempdir later and all hooks will be ready...

>  What OS do you have that doesn't know 'mktemp
> -d'?  
Enterasys Dragon Slackware

> 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?

Unfortunatelly it is some customer's box and I have no access to it.

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

Agreed, it is mistake. Please ignore.

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

I think we should discuss this patch keeping in mind
the next one.

Don't think increasing special cases list is good idea...
The logic should be as simple as possible.

Anyway we have very width installation base and have no problems
without above checks. Of course we do not cover all use cases...


>>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. :-)
Yes, and we are trying to cleanup things.

We don't insist on our patches commitment as-is, but we'd
like to see some solutions of above issues in further dkms
release.

Thank you in advance.

Best regards
Vladimir Simonov