[Crash-utility] [PATCH 0/2] The new API about ikconfig.

[Toshikazu Nakayama]

Hi Dave,

I shuold agree with your comments at all, thanks a lot.

>Hello Toshi,
>
>I have a few issues/questions with your patch:
>
>1. This should be impossible, because even if preload_extensions() 
>   is used, that function is called long after the init-time call 
>   to read_in_kernel_config(IKCFG_INIT) is done:

You're right. I'll remove this condition because this never hit as TRUE.

>+       if (command == IKCFG_INIT && kt->ikconfig_setup) {
>+               /* fastpath */
>+               if (get_kernel_config("DEBUG_BUGVERBOSE", NULL) == IKCONFIG_N)
>+                       kt->flags |= BUGVERBOSE_OFF;
>+
>+               if (get_kernel_config("NR_CPUS", &val) == IKCONFIG_STR) {
>+                       kt->kernel_NR_CPUS = atoi(val);
>+                       if (CRASHDEBUG(1))
>+                               error(INFO, "CONFIG_NR_CPUS: %d\n",
>+                                     kt->kernel_NR_CPUS);
>+               }
>+               if (get_kernel_config("PGTABLE_4", NULL) == IKCONFIG_Y) {
>+                       machdep->flags |= VM_4_LEVEL;
>+                       if (CRASHDEBUG(1))
>+                               error(INFO, "CONFIG_PGTABLE_4\n");
>+               }
>+               if (get_kernel_config("HZ", &val) == IKCONFIG_STR) {
>+                       machdep->hz = atoi(val);
>+                       if (CRASHDEBUG(1))
>+                               error(INFO, "CONFIG_HZ: %d\n", machdep->hz);
>+               }
>+
>+               goto out1;
>+       }
>
>2. I didn't test this, but I think this will be a problem because it will
>   not return back to the _init() function in the extension module:  
>
>        if ((sp = symbol_search("kernel_config_data")) == NULL) {
>-               if (command == IKCFG_READ)
>+               if (command == IKCFG_READ || command == IKCFG_SETUP)
>                        error(FATAL,
>                            "kernel_config_data does not exist in this kernel\n");
>                return;
>
>   It should do this:
>
>     If IKCONFIG_INIT:  return quietly with no error message
>     If IKCONFIG_READ:  error(FATAL, ...
>     If IKCONFIG_SETUP: error(WARNING, ...

I'll change with this manner.

>2. Can you explain the need for the IKCONFIG_SORT_INTERVAL stuff?

Example for CONFIG_X86(_64) which is probably referenced many times from
extensions, it shuold be in front position of config list for rapid loop break.
In my environs, kernel owned around 400 valid kernel configs.

However, I'll remove them except kt->ikconfig_refs.

Extensions may not repeat get_kernel_config(), it's only in their initialization.
Also such a major configs are already existing at front of kernel_config_data.
(In addition to these reasons, can not work for "is not set" configs.)

The kt->ikconfig_refs is remained as get_kernel_config() call (reference)
counters including the case of IKCONFIG_N.

>3. When adding items to any global structure (such as to the kernel_table),
>   I always add them to the end of the structure so that any pre-existing
>   extension modules that use something in the unmodified structure will 
>   still work even if additional entries are added to the end.

I could not be aware of this issue, will fix my bad-mannered.
In my understanding, this is not indicating position
at dump_kernel_table(), correct?

>4. Just a preference on my part, but I would allow get_kernel_config()
>   to accept both "CONFIG_XXXX" and "XXX" as a "name" argument, and then
>   just strip off "CONFIG_" if it's there.

I also prefer this idea, I'll update patch to support "CONFIG_name" argument.

Thanks,
Toshi.
 
>Dave
>
>
>> -----------------------------------------------------------
>> 
>> Example usage: Resolve RADIX_TREE_MAP_SHIFT from config depends.
>> 
>> [ code of kernel-2.6.35 ]
>> 
>> #define RADIX_TREE_MAP_SHIFT (CONFIG_BASE_SMALL ? 4 : 6)
>> 
>> [ code of crash utility or extension modules ]
>> 
>> static int radix_tree_map_shift = 6;
>> #define RADIX_TREE_MAP_SHIFT radix_tree_map_shift
>> 
>> int _init(void)
>> {
>> read_in_kernel_config(IKCFG_SETUP);
>> 
>> if (get_kernel_config("BASE_SMALL", NULL) == IKCONFIG_Y)
>> radix_tree_map_shift = 4;
>> 
>> return 0;
>> }
>> 
>> int int _fini(void)
>> {
>> read_in_kernel_config(IKCFG_FREE);
>> 
>> return 0;
>> }
>> 
>> Toshikazu Nakayama (2):
>> new ikconfig API.
>> dump_kernel_table()
>> 
>> crash-5.1.1/defs.h | 13 ++++
>> crash-5.1.1/kernel.c | 166
>> +++++++++++++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 178 insertions(+), 1 deletions(-)
>> 
>> --
>> 1.7.4.rc2
>> 
>> --
>> Crash-utility mailing list
>> Crash-utility at redhat.com
>> https://www.redhat.com/mailman/listinfo/crash-utility
>
>--
>Crash-utility mailing list
>Crash-utility at redhat.com
>https://www.redhat.com/mailman/listinfo/crash-utility