* Masami Hiramatsu <mhira...@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
