svn commit: r185746 - head/sys/boot/forth
Eugene M. Kim
20080111.freebsd.org at ab.ote.we.lv
Mon Dec 22 16:45:40 PST 2008
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 at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/svn-src-all
> To unsubscribe, send any mail to "svn-src-all-unsubscribe at 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
More information about the svn-src-all
mailing list