Date: Mon, 22 Dec 2008 16:13:21 -0800 From: "Eugene M. Kim" <20080111.freebsd.org@ab.ote.we.lv> To: Luigi Rizzo <luigi@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r185746 - head/sys/boot/forth Message-ID: <49502D21.5040604@ab.ote.we.lv> In-Reply-To: <200812071942.mB7JgK94034137@svn.freebsd.org> References: <200812071942.mB7JgK94034137@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?49502D21.5040604>