From owner-freebsd-bugs@FreeBSD.ORG Sat Sep 28 21:10:01 2013 Return-Path: Delivered-To: freebsd-bugs@smarthost.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTP id C0485BCA for ; Sat, 28 Sep 2013 21:10:01 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:1900:2254:206c::16:87]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 9A80522C4 for ; Sat, 28 Sep 2013 21:10:01 +0000 (UTC) Received: from freefall.freebsd.org (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.7/8.14.7) with ESMTP id r8SLA1aP098610 for ; Sat, 28 Sep 2013 21:10:01 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.7/8.14.7/Submit) id r8SLA1tu098609; Sat, 28 Sep 2013 21:10:01 GMT (envelope-from gnats) Date: Sat, 28 Sep 2013 21:10:01 GMT Message-Id: <201309282110.r8SLA1tu098609@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org Cc: From: Derek Schrock Subject: Re: bin/182098: [patch] Change kldxref fts_open ordering so it produces a consistent linker.hints between machines of the same architecture. X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: Derek Schrock List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 28 Sep 2013 21:10:01 -0000 The following reply was made to PR bin/182098; it has been noted by GNATS. From: Derek Schrock To: Jilles Tjoelker 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 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