Re: [Crash-utility] [PATCH 0/3] ikconfig and load module helperpatches.

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