bin/182098: [patch] Change kldxref fts_open ordering so it produces a consistent linker.hints between machines of the same architecture.
Derek Schrock
dereks at lifeofadishwasher.com
Sat Sep 28 21:10:01 UTC 2013
The following reply was made to PR bin/182098; it has been noted by GNATS.
From: Derek Schrock <dereks at lifeofadishwasher.com>
To: Jilles Tjoelker <jilles at stack.nl>
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 17:06:00 -0400
Looks like I wanted to postpone the last message instead of sending it:
Ok I see what you mean by using FTS_D. Everything looks good to me, I
compared the linker.hints generated by the two system from the initial
description. They're the same.
However, I slightly modified the testing script from your last comment
and found three .ko files that don't seem to generate an entry in the
linker.hints files:
Patched kldxref:
$ /usr/src/usr.sbin/kldxref/kldxref -R /boot/
$ dir="kernel"; strings="$(strings /boot/$dir/linker.hints)"; for f in /boot/"$dir"/*.ko ; do f="${f##*/}"; case "$strings" in *"$f"*) ;; *) echo "what? $f" ;; esac; done
what? musb.ko
what? scc.ko
what? uss820dci.ko
System kldxref:
$ kldxref -R /boot/
$ dir="kernel"; strings="$(strings /boot/$dir/linker.hints)"; for f in /boot/"$dir"/*.ko ; do f="${f##*/}"; case "$strings" in *"$f"*) ;; *) echo "what? $f" ;; esac; done
what? musb.ko
what? scc.ko
what? uss820dci.ko
Can we assume this isn't a problem with patch since the system kldxref
doesn't generate a linker.hints with strings that reference those ko
files?
On Sat, Sep 28, 2013 at 04:50:10PM -0400, Derek Schrock wrote:
> Ok I see what you mean by using FTS_D. Everything looks good to me, I
> compared the linker.hints generated by the two system from
>
> On Sat, Sep 28, 2013 at 08:39:53PM +0200, Jilles Tjoelker wrote:
> > 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
More information about the freebsd-bugs
mailing list