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

[Dave Anderson]

----- Original Message -----
> Hi,
> 
> I would like to suggest new ikconfig's API from defs.h.
> 
> The get_kernel_config("config name without CONFIG_ prefix", &val)
> will 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).
> 
> API require CONFIG_IKCONFIG=y. Otherwise user is warned.
> 
> I think it is useful for such as extension module developers
> to write kernel config switch code.
> 
> Although ifdef or symbol APIs can be used instead,
> I believe using this API can write more impressive or flexible code
> about kernel config depended parts (over my senses...).
> 
> Thanks,
> Toshi.

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:

+       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, ...

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

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.

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.
 
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