linux-kernel - Re: [PATCH 1/3] tracing: merge seq_print_sym_short() and seq_print_sym

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 26 Oct 2018 17:41:33 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Rasmus Villemoes <linux@...musvillemoes.dk>
Cc:     Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] tracing: merge seq_print_sym_short() and
 seq_print_sym_offset()


Hi Ramus,

Thanks for sending these patches. I have some small nits though.

First, please send a cover letter whenever sending more than one patch.
It just groups them better in my inbox.

The second is embedded below.

On Fri, 26 Oct 2018 23:13:44 +0200
Rasmus Villemoes <linux@...musvillemoes.dk> wrote:

> These two functions are nearly identical, so we can avoid some code
> duplication by moving the conditional into a common implementation.
> 
> Signed-off-by: Rasmus Villemoes <linux@...musvillemoes.dk>
> ---
>  kernel/trace/trace_output.c | 34 +++++++---------------------------
>  1 file changed, 7 insertions(+), 27 deletions(-)
> 
> diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
> index 6e6cc64faa38..501311dcfca6 100644
> --- a/kernel/trace/trace_output.c
> +++ b/kernel/trace/trace_output.c
> @@ -339,34 +339,17 @@ static inline const char *kretprobed(const char *name)
>  #endif /* CONFIG_KRETPROBES */
>  
>  static void
> -seq_print_sym_short(struct trace_seq *s, const char *fmt, unsigned long address)
> +seq_print_sym(struct trace_seq *s, const char *fmt, unsigned long address,
> +	      bool with_offset)

Just call the variable "offset". It's rather obvious what that means.

The other patches look good, but can you send a v2 of the series, with
these updates?

Thanks!

-- Steve


>  {
>  	char str[KSYM_SYMBOL_LEN];
>  #ifdef CONFIG_KALLSYMS
>  	const char *name;
>  
> -	kallsyms_lookup(address, NULL, NULL, NULL, str);
> -
> -	name = kretprobed(str);
> -
> -	if (name && strlen(name)) {
> -		trace_seq_printf(s, fmt, name);
> -		return;
> -	}
> -#endif
> -	snprintf(str, KSYM_SYMBOL_LEN, "0x%08lx", address);
> -	trace_seq_printf(s, fmt, str);
> -}
> -
> -static void
> -seq_print_sym_offset(struct trace_seq *s, const char *fmt,
> -		     unsigned long address)
> -{
> -	char str[KSYM_SYMBOL_LEN];
> -#ifdef CONFIG_KALLSYMS
> -	const char *name;
> -
> -	sprint_symbol(str, address);
> +	if (with_offset)
> +		sprint_symbol(str, address);
> +	else
> +		kallsyms_lookup(address, NULL, NULL, NULL, str);
>  	name = kretprobed(str);
>  
>  	if (name && strlen(name)) {
> @@ -424,10 +407,7 @@ seq_print_ip_sym(struct trace_seq *s, unsigned long ip, unsigned long sym_flags)
>  		goto out;
>  	}
>  
> -	if (sym_flags & TRACE_ITER_SYM_OFFSET)
> -		seq_print_sym_offset(s, "%s", ip);
> -	else
> -		seq_print_sym_short(s, "%s", ip);
> +	seq_print_sym(s, "%s", ip, sym_flags & TRACE_ITER_SYM_OFFSET);
>  
>  	if (sym_flags & TRACE_ITER_SYM_ADDR)
>  		trace_seq_printf(s, " <" IP_FMT ">", ip);