On Fri, 27 Apr 2018 09:14:17 +0200 Ingo Molnar mingo@kernel.org wrote:
- 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?
It's a typo. I mean "show it. Or, it shows..." anyway,
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:*/
OK, look good to me.
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()...
Would you really think we should handle all the address with 'void *'? IOW, are there any policy that we handle the generic address by 'void *' or 'unsigned long'? For example, other address checker like kernel_text_address(), module_text_address(), and ftrace_location() receive 'unsigned long'. (only jump_label_text_reserved() using 'void *')
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);
?
Both are OK to me. I just didn't want to repeat the printk format string there.
This variant eliminates the unnecessary complication over local variables and makes it abundantly clear what gets printed and how.
Agreed, it may shorten the patch.
( Note that the kprobe_blacklist_entry type cleanup should still be done, regardless of code flow choices. )
Thanks,
Ingo
Thank you,