Date: Sat, 28 Sep 2013 18:50:01 GMT From: Jilles Tjoelker <jilles@stack.nl> To: freebsd-bugs@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. Message-ID: <201309281850.r8SIo1pV069514@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR bin/182098; it has been noted by GNATS.
From: Jilles Tjoelker <jilles@stack.nl>
To: Derek Schrock <dereks@lifeofadishwasher.com>
Cc: bug-followup@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201309281850.r8SIo1pV069514>