[Crash-utility] [PATCH 0/3] ikconfig and load module helper patches.

[Dave Anderson]

Hello Toshi,

Here are my comments and further suggestions for this patchset:

0001-new-ikconfig-API.patch
0002-dump_kernel_table.patch

  In the interest of simplification, let me suggest the following.

  With your current scheme, this is required:

    crash> extend module.so
    ...
    the module's _init() function gets called, which does:
      read_in_kernel_config(IKCFG_SETUP);
      ret = get_kernel_config("XXX", str);
      read_in_kernel_config(IKCFG_FREE);  
    ...
    crash>
  
  And then you have a bunch of stuff for handling multiple extension
  modules, reference counting, etc.

  But extension modules should not have to worry about having to make
  the two read_in_kernel_config() calls -- it should simply be a matter
  of doing this to get a kernel configuration item:
 
     ret = get_kernel_config("XXX", str);

  This would typically be called one (or more) times in their _init() 
  function, but it could also be called in a registered function as well.

  Anyway, let the read_in_kernel_config(IKCFG_SETUP) and (IKCFG_FREE) 
  mechanisms be handled by the static crash code:

    - read_in_kernel_config(IKCFG_SETUP) should be called by
      the exported get_kernel_config() function.  (see below)

    - read_in_kernel_config(IKCFG_FREE) should be called by the
      restore_sanity() function, which gets called just before 
      the "crash>" prompt for the *next* crash command is displayed.
      (see below)

  That way, the ikconfig data is only available for the life-time of
  a particular command, for example, while the crash "extend" command 
  is running, or perhaps later on when a module's registered command
  is run.

  That being the case, all of the stuff you have for handling multiple 
  extension modules can be removed, because that could never happen.

  The kt->ikconfig_setup counter can be removed, and replaced by 
  a new kt->ikconfig_flags field, that has at least two flags:

    #define IKCONFIG_AVAIL  0x1   /* kernel contains ikconfig data */
    #define IKCONFIG_LOADED 0x2   /* ikconfig data is currently loaded */

  And the would be set/cleared like so:

  - When the initialization-time read_in_kernel_config(IKCFG_INIT) call is 
    made, IKCFG_AVAIL would get set if the ikconfig data is available in
    the kernel.

  - When read_in_kernel_config(IKCFG_SETUP) successfully reads the ikconfig
    data, then IKCFG_LOADED could be be set (temporarily).  

  - When read_in_kernel_config(IKCFG_FREE) is called, IKCFG_LOADED gets cleared.

  That being the case, get_kernel_config() could have this at the top:

	switch (kt->ikconfig_flags & (IKCFG_LOADED|IKCFG_AVAIL))
	{
	case IKCFG_AVAIL:
                read_in_kernel_config(IKCFG_SETUP);
		if (!kt->config_flags & IKCFG_LOADED)
			return IKCONFIG_N;
		break;

	case (IKCFG_LOADED|IKCFG_AVAIL):  /* already loaded */
		break;

	default:
		return IKCFG_N;
	}

   And restore_sanity() could just do this:

	if (kt->ikconfig_flags & IKCFG_LOADED)
		read_in_kernel_config(IKCFG_FREE);

  And lastly, I see no need for kt->ikconfig_refs.  You increment
  it in get_kernel_config() each time it is called, and then set
  it back to 0 when read_in_kernel_config(IKCFG_FREE) frees everything.
  But nobody else reads it -- and I don't see why an extension module
  would ever need to read it?  

0003-load_module_symbols_helper.patch

  Queued for the next release.

Thanks again,
  Dave


----- Original Message -----
> Declare get_kernel_config(), load_module_symbols_helper()
> for crash outside extension users.
> CRASH_MODULE_PATH valiable can apply to search non-standard module path
> from load_module_symbols_helper() and "mod" command.
> 
> - get_kernel_config("config name", &val)
> Return one of target kernel configurations.
> If val == NULL, return value is poinited whether config is valid or
> not.
> IKCONFIG_N: kernel config is not set (not valid).
> IKCONFIG_Y: kernel config is set y (valid).
> IKCONFIG_M: kernel config is set m (valid).
> IKCONFIG_STR: kernel config is values or strings, etc (valid).
> 
> "help -k" can display ikconfig setup state,
> number of valid ikconfig entries, function calls.
> 
> Notes:
> 1. How to activate get_kernel_config().
> read_in_kernel_config(IKCFG_SETUP)
> -> get_kernel_config() become active.
> read_in_kernel_config(IKCFG_FREE)
> -> get_kernel_config() become iniactive.
> 
> 2. This function require CONFIG_IKCONFIG=y. Otherwise user is warned.
> 
> Functional change from previous one.
> "config name" can be allowed both with or without CONFIG_ prefix strings.
> [ Dave's recommendation ]
> 
> - load_module_symbols_helper("module name")
> Load specified kernel module symbols with argument.
> This is simplified usage from original load_module_symbols().
> 
> Add "CRASH_MODULE_PATH" valiable which can use to resolve
> non-standard module path.
> 
> Functional change from previous one.
> "CRASH_MODULE_PATH" could be used by the "mod -s" command as well.
> [ Dave's recommendation ]
> Example usage:
> < in .crashrc >
> mod -s ext3
> mod -s jbd
> :
> :
> 
> export CRASH_MODULE_PATH="your module's root directory"
> /usr/sbin/crash
> 
> Even if target kernel is changed,
> crash will load appropriate ext3 or jbd modules by this valiable.
> 
> Toshikazu Nakayama (3):
> new ikconfig API.
> dump_kernel_table()
> load_module_symbols_helper().
> 
> crash-5.1.1/defs.h | 14 +++++
> crash-5.1.1/kernel.c | 148
> ++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 162 insertions(+), 0 deletions(-)
> 
> --
> 1.7.4.rc2.3.g60a2e
> 
> 
> --
> Crash-utility mailing list
> Crash-utility at redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility