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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/crash-utility
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility
