bin/182098: [patch] Change kldxref fts_open ordering so it produces a consistent linker.hints between machines of the same architecture.

[Jilles Tjoelker]

The following reply was made to PR bin/182098; it has been noted by GNATS.

From: Jilles Tjoelker <jilles at stack.nl>
To: Derek Schrock <dereks at lifeofadishwasher.com>
Cc: bug-followup at FreeBSD.org
Subject: Re: bin/182098: [patch] Change kldxref fts_open ordering so it
 produces a consistent linker.hints between machines of the same
 architecture.
Date: Sat, 28 Sep 2013 20:39:53 +0200

 On Tue, Sep 24, 2013 at 09:32:42PM -0400, Derek Schrock wrote:
 > On Mon, Sep 16, 2013 at 12:39:58AM +0200, Jilles Tjoelker wrote:
 > > In the interest of reproducible builds, your patch seems a good idea. It
 > > seems unattractive to run kldxref /boot/kernel on every machine.
 > > 
 > > The implementation of compare() seems unnecessarily complex though. In
 > > find -s, the fts_names are simply passed to strcoll() (here, strcmp()
 > > would be better). The trickery with the length may cause inconsistent
 > > results if one filename is a prefix of another (rare).
 
 > Fair enough after reading more of the fts(3) man fts_name is always
 > null terminated.
 
 > > This change may also expose a latent bug with kldxref -R: it does not
 > > work properly if a directory contains both files that need a mention in
 > > a hints file and subdirectories, and at least one such file appears
 > > after a subdirectory. Because your change alters the traversal order, it
 > > might break a use of kldxref -R that previously happened to work. You
 > > can make it work reliably by sorting FTS_D entries after other entries.
 
 > Yep, after some additional testing with the patched kldxref it
 > produced different linker.hints files. 
 
 Hmm, that is unexpected. The sorting happens on entries of each
 directory, and find -s does the same.
 
 > The new patch uses fts_parent's fts_name (the directory's name).
 > First compare the parent's name, if the same compare the passed
 > FTSENTs.
 
 The comparison on the parent's name seems redundant due to the way
 fts(3) does the sorting.
 
 I tried my idea using FTS_D.
 
 Here is a script to test kldxref on this:
 
 #!/bin/sh
 
 kldxref=${1:-kldxref}
 kldxrefpath=$(command -v "$kldxref" 2>/dev/null) || {
 	printf "%s not found\n" "$kldxref"
 	exit
 }
 printf "Testing %s\n" "$kldxrefpath"
 tmpdir=$(mktemp -d -t kldxreftest) || exit
 cd "$tmpdir" || exit
 cp /boot/kernel/if_re.ko . || exit
 mkdir subdir1 || exit
 cp /boot/kernel/if_faith.ko . || exit
 cp /boot/kernel/zlib.ko . || exit
 cp /boot/kernel/unionfs.ko subdir1 || exit
 printf "Directory structure created in %s\n" "$tmpdir"
 "$kldxref" -R "$tmpdir"
 mainstrings=$(strings linker.hints)
 subdir1strings=$(strings subdir1/linker.hints)
 bad=
 for m in if_re.ko if_faith.ko zlib.ko; do
 	case $mainstrings in
 	*"$m"*) ;;
 	*) printf "%s is missing from main\n" "$m"; bad=1 ;;
 	esac
 	case $subdir1strings in
 	*"$m"*) printf "subdir1 wrongly has %s\n" "$m"; bad=1 ;;
 	esac
 done
 for m in unionfs.ko; do
 	case $subdir1strings in
 	*"$m"*) ;;
 	*) printf "%s is missing from subdir1\n" "$m"; bad=1 ;;
 	esac
 	case $mainstrings in
 	*"$m"*) printf "main wrongly has %s\n" "$m"; bad=1 ;;
 	esac
 done
 if [ -z "$bad" ]; then
 	printf "%s appears to be working correctly\n" "$kldxrefpath"
 fi
 
 My suggested patch:
 
 Index: usr.sbin/kldxref/kldxref.c
 ===================================================================
 --- usr.sbin/kldxref/kldxref.c	(revision 255570)
 +++ usr.sbin/kldxref/kldxref.c	(working copy)
 @@ -274,6 +274,16 @@
  	exit(1);
  }
  
 +int 
 +compare(const FTSENT* const* a, const FTSENT* const* b)
 +{
 +	if ((*a)->fts_info == FTS_D && (*b)->fts_info != FTS_D)
 +		return 1;
 +	if ((*a)->fts_info != FTS_D && (*b)->fts_info == FTS_D)
 +		return -1;
 +	return strcmp((*a)->fts_name, (*b)->fts_name);
 +}
 +
  int
  main(int argc, char *argv[])
  {
 @@ -315,7 +325,7 @@
  		err(1, "%s", argv[0]);
  	}
  
 -	ftsp = fts_open(argv, fts_options, 0);
 +	ftsp = fts_open(argv, fts_options, compare);
  	if (ftsp == NULL)
  		exit(1);
  
 
 -- 
 Jilles Tjoelker