svn commit: r185746 - head/sys/boot/forth

[Luigi Rizzo]

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
 ;