* Masami Hiramatsu mhiramat@kernel.org wrote:
- /*
* As long as kallsyms shows the address, kprobes blacklist also* show it, Or, it shows null address and symbol.*/
Please _read_ the comments you write!
In which universe does a capitalized 'Or' make sense, even if we ignore the various other spelling mistakes?
Also, that sentence is unnecessarily complex, just say this:
- /*
* If /proc/kallsyms is not showing kernel addresses then we won't show* them here either:*/
But I'm unhappy about the messy typing and the messy code flow:
+ void *start = (void *)ent->start_addr, *end = (void *)ent->end_addr;
+ /* + * As long as kallsyms shows the address, kprobes blacklist also + * show it, Or, it shows null address and symbol. + */ + if (!kallsyms_show_value()) + start = end = NULL; + + seq_printf(m, "0x%px-0x%px\t%ps\n", start, end, + (void *)ent->start_addr);
All three 'void *' type casts here are due to the bad type choices here:
struct kprobe_blacklist_entry { struct list_head list; unsigned long start_addr; unsigned long end_addr; };
The natural type of ->start_addr and ->end_addr is 'void *', AFAICS this would remove some other type casts from the kprobes code as well, such as from the arch_deref_entry_point()...
But the whole code flow introduced by this patch is messy as hell as well. Why cannot this do the obvious thing:
if (!kallsyms_show_value()) seq_printf(m, "0x%px-0x%px\t%ps\n", NULL, NULL, ent->start_addr); else seq_printf(m, "0x%px-0x%px\t%ps\n", ent->start_addr, ent->end_addr, ent->start_addr);
?
This variant eliminates the unnecessary complication over local variables and makes it abundantly clear what gets printed and how.
( Note that the kprobe_blacklist_entry type cleanup should still be done, regardless of code flow choices. )
Thanks,
Ingo