From owner-svn-src-all@FreeBSD.ORG Tue Dec 23 00:45:40 2008 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 21CB81065670; Tue, 23 Dec 2008 00:45:40 +0000 (UTC) (envelope-from 20080111.freebsd.org@ab.ote.we.lv) Received: from mx2.nttmcl.com (MX2.nttmcl.com [216.69.68.200]) by mx1.freebsd.org (Postfix) with ESMTP id F1E6E8FC12; Tue, 23 Dec 2008 00:45:39 +0000 (UTC) (envelope-from 20080111.freebsd.org@ab.ote.we.lv) Received: from localhost (localhost [127.0.0.1]) by mx2.nttmcl.com (Postfix) with ESMTP id 958414DD4E; Mon, 22 Dec 2008 16:13:22 -0800 (PST) X-Spam-Score: -2.9 X-Spam-Level: X-Spam-Status: No, score=-2.9 tagged_above=-999 required=5 tests=[ALL_TRUSTED=-1.8, BAYES_00=-2.599, FROM_STARTS_WITH_NUMS=1.499] Received: from mx2.nttmcl.com ([127.0.0.1]) by localhost (mx2.nttmcl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id nsaoH66P7+9E; Mon, 22 Dec 2008 16:13:21 -0800 (PST) Received: from [216.69.69.252] (dhcp252.nttmcl.com [216.69.69.252]) by mx2.nttmcl.com (Postfix) with ESMTP id E771A4DD4D; Mon, 22 Dec 2008 16:13:21 -0800 (PST) Message-ID: <49502D21.5040604@ab.ote.we.lv> Date: Mon, 22 Dec 2008 16:13:21 -0800 From: "Eugene M. Kim" <20080111.freebsd.org@ab.ote.we.lv> User-Agent: Thunderbird 2.0.0.18 (Windows/20081105) MIME-Version: 1.0 To: Luigi Rizzo References: <200812071942.mB7JgK94034137@svn.freebsd.org> In-Reply-To: <200812071942.mB7JgK94034137@svn.freebsd.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r185746 - head/sys/boot/forth X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 23 Dec 2008 00:45:40 -0000 Luigi Rizzo wrote: > Author: luigi > Date: Sun Dec 7 19:42:20 2008 > New Revision: 185746 > URL: http://svn.freebsd.org/changeset/base/185746 > > Log: > PROBLEM: putting in a loader config file a line of the form > > loader_conf_files="foo bar baz" > > should cause loading the files listed, and then resume with the > remaining config files (from previous values of the variable). > Unfortunately, sometimes the line was ignored -- actually even > modifying the line in /boot/default/loader.conf sometimes doesn't work. > > ANALYSIS: After much investigation, turned out to be a bug in the logic. > The existing code detected a new assignment by looking at the address > of the the variable containing the string. This only worked by pure > chance, i.e. if the new string is longer than the previous value > then the memory allocator may return a different address > to store the string hence triggering the detection. > > SOLUTION: This commit contains a minimal change to fix the problem, > without altering too much the existing structure of the code. > However, as a step towards improving the quality and reliability of > this code, I have introduced a handful of one-line functions > (strget, strset, strfree, string= ) that could be used in dozens > of places in the existing code. > > HOWEVER: > There is a much bigger problem here. Even though I am no Forth > expert (as most fellow src committers) I can tell that much of the > forth code (in support.4th at least) is in severe need of a > review/refactoring: > > + pieces of code are replicated multiple times instead of writing > functions (see e.g. set_module_*); > > + a lot of stale code (e.g. "structure" definitions for > preloaded_files, kernel_module, pnp stuff) which is not used > or at least belongs elsewhere. > The code bload is extremely bad as the loader runs with very small > memory constraints, and we already hit the limit once (see > > http://svn.freebsd.org/viewvc/base?view=revision&revision=185132 > Reducing the footprint of the forth files is critical. > > + two different styles of coding, one using pure stack functions > (maybe beautiful but surely highly unreadable), one using > high level mechanisms to give names to arguments and local > variables (which leads to readable code). > > Note that this code is used by default by all FreeBSD installations, > so the fragility and the code bloat are extremely damaging. > I will try to work fixing the three items above, but if others have > time, please have a look at these issues. > > MFC after: 4 weeks > > Modified: > head/sys/boot/forth/support.4th > > Modified: head/sys/boot/forth/support.4th > ============================================================================== > --- head/sys/boot/forth/support.4th Sun Dec 7 19:29:11 2008 (r185745) > +++ head/sys/boot/forth/support.4th Sun Dec 7 19:42:20 2008 (r185746) > @@ -288,6 +288,17 @@ only forth also support-functions defini > > : free-memory free if free_error throw then ; > > +: strget { var -- addr len } var .addr @ var .len @ ; > + > +\ assign addr len to variable. > +: strset { addr len var -- } addr var .addr ! len var .len ! ; > + > +\ free memory and reset fields > +: strfree { var -- } var .addr @ ?dup if free-memory 0 0 var strset then ; > + > +\ free old content, make a copy of the string and assign to variable > +: string= { addr len var -- } var strfree addr len strdup var strset ; > + > \ Assignment data temporary storage > > string name_buffer > @@ -712,19 +723,6 @@ only forth also support-functions also f > module_loaderror_suffix suffix_type? > ; > > -: set_conf_files > - conf_files .addr @ ?dup if > - free-memory > - then > - value_buffer .addr @ c@ [char] " = if > - value_buffer .addr @ char+ value_buffer .len @ 2 chars - > - else > - value_buffer .addr @ value_buffer .len @ > - then > - strdup > - conf_files .len ! conf_files .addr ! > -; > - > : set_nextboot_conf > nextboot_conf_file .addr @ ?dup if > free-memory > @@ -888,6 +886,11 @@ only forth also support-functions also f > then > ; > > +: set_conf_files > + set_environment_variable > + s" loader_conf_files" getenv conf_files string= > +; > + > : set_nextboot_flag > yes_value? to nextboot? > ; > @@ -1045,7 +1048,6 @@ only forth also support-functions defini > \ Variables used for processing multiple conf files > > string current_file_name > -variable current_conf_files > > \ Indicates if any conf file was succesfully read > > @@ -1053,16 +1055,8 @@ variable current_conf_files > > \ loader_conf_files processing support functions > > -: set_current_conf_files > - conf_files .addr @ current_conf_files ! > -; > - > -: get_conf_files > - conf_files .addr @ conf_files .len @ strdup > -; > - > -: recurse_on_conf_files? > - current_conf_files @ conf_files .addr @ <> > +: get_conf_files ( -- addr len ) \ put addr/len on stack, reset var > + conf_files strget 0 0 conf_files strset > ; > > : skip_leading_spaces { addr len pos -- addr len pos' } > @@ -1133,7 +1127,6 @@ variable current_conf_files > \ Interface to loader_conf_files processing > > : include_conf_files > - set_current_conf_files > get_conf_files 0 > begin > get_next_file ?dup > @@ -1141,7 +1134,7 @@ variable current_conf_files > set_current_file_name > ['] load_conf catch > process_conf_errors > - recurse_on_conf_files? if recurse then > + conf_files .addr @ if recurse then > repeat > ; > > _______________________________________________ > svn-src-all@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/svn-src-all > To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org" > I believe this fixes bin/120084. I will test this soon (my -CURRENT machine runs Windows for the time being :-p). Cheers, Eugene