Skip site navigation (1)Skip section navigation (2)
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>