All of lore.kernel.org
 help / color / mirror / Atom feed
From: Linus Torvalds <torvalds@linux-foundation.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Linux List Kernel Mailing <linux-kernel@vger.kernel.org>,
	Ingo Molnar <mingo@kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Namhyung Kim <namhyung@kernel.org>,
	Masami Hiramatsu <mhiramat@kernel.org>,
	Joe Perches <joe@perches.com>, Tom Zanussi <zanussi@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
Date: Fri, 21 Dec 2018 14:08:11 -0800	[thread overview]
Message-ID: <CAHk-=wjaNWzXSW9XT-Z=9OF=5+2ft6UqjoUgaWeEWx8PUmS9xg@mail.gmail.com> (raw)
In-Reply-To: <20181221155513.11450ca6@gandalf.local.home>

On Fri, Dec 21, 2018 at 12:55 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 21 Dec 2018 12:41:17 -0800
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> > Parentheses....
>
> ?

Your patch actually had them, but in the body of your email you did

> #define have_prefix(str, prefix) ({ \
>       const char *__pfx = (const char *)prefix; \

which is just completely wrong.

Considering your _old_ patch had the exact same bug, I really think
you need to start internalizing the whole "macro arguments *have* to
be properly protected" thing.

And I agree with Joe that using a million underscores just makes code
less legible. Two underscores at the beginning is the standard
namespace protection. Underscores at the end do nothing. And using
*more* than two is just crazy.

Finally, I think the cast is actually bogus and wrong. Why would the
prefix ever be anything but "const char *"? Adding the cast only makes
it more possible that somebody uses a truly wrong type ("unsigned long
*" ?), and then the cast just silently hides it.

If somebody uses "unsigned char *" for this, they'd get the exact same
warning if they were using strncmp and do this by hand.

So why would that helper function act any differently? Particularly
when it then has the huge downside that it will also accept absolute
garbage?

Anyway, that was a long and winding NAK for your patch.

             Linus

  reply	other threads:[~2018-12-21 22:08 UTC|newest]

Thread overview: 62+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-21 17:56 [for-next][PATCH 00/24] tracing: Updates for the next (coming soon) merge window Steven Rostedt
2018-12-21 17:56 ` [for-next][PATCH 01/24] ftrace: Allow ftrace_replace_code() to be schedulable Steven Rostedt
2018-12-21 17:56 ` [for-next][PATCH 02/24] arm64: ftrace: Set FTRACE_MAY_SLEEP before ftrace_modify_all_code() Steven Rostedt
2018-12-21 17:56 ` [for-next][PATCH 03/24] fgraph: Add comment to describe ftrace_graph_get_ret_stack Steven Rostedt
2018-12-21 17:56 ` [for-next][PATCH 04/24] x86/ftrace: Do not call function graph from dynamic trampolines Steven Rostedt
2018-12-21 17:56 ` [for-next][PATCH 05/24] powerpc/frace: Use ftrace_graph_get_ret_stack() instead of curr_ret_stack Steven Rostedt
2018-12-21 17:56   ` Steven Rostedt
2018-12-22  9:51   ` Michael Ellerman
2018-12-22  9:51     ` Michael Ellerman
2018-12-22 12:24     ` Steven Rostedt
2018-12-22 12:24       ` Steven Rostedt
2018-12-21 17:56 ` [for-next][PATCH 06/24] sparc64: " Steven Rostedt
2018-12-21 17:56   ` Steven Rostedt
2018-12-21 18:24   ` David Miller
2018-12-21 18:24     ` David Miller
2018-12-21 19:29     ` Steven Rostedt
2018-12-21 19:29       ` Steven Rostedt
2018-12-21 17:56 ` [for-next][PATCH 07/24] sh: ftrace: " Steven Rostedt
2018-12-21 17:56   ` Steven Rostedt
2018-12-21 17:56 ` [for-next][PATCH 08/24] arm64: " Steven Rostedt
2018-12-21 17:56   ` Steven Rostedt
2018-12-21 17:56 ` [for-next][PATCH 10/24] seq_buf: Use size_t for len in seq_buf_puts() Steven Rostedt
2018-12-21 17:56 ` [for-next][PATCH 11/24] tracing: Fix ftrace_graph_get_ret_stack() to use task and not current Steven Rostedt
2018-12-21 17:56 ` [for-next][PATCH 12/24] tracing: Remove unnecessary hist trigger struct field Steven Rostedt
2018-12-21 17:56 ` [for-next][PATCH 13/24] tracing: Change strlen to sizeof for hist trigger static strings Steven Rostedt
2018-12-21 17:56 ` [for-next][PATCH 14/24] tracing: Use var_refs[] for hist trigger reference checking Steven Rostedt
2018-12-21 17:56 ` [for-next][PATCH 15/24] tracing: Remove open-coding of hist trigger var_ref management Steven Rostedt
2018-12-21 17:56 ` [for-next][PATCH 16/24] tracing: Use hist triggers var_ref array to destroy var_refs Steven Rostedt
2018-12-21 17:56 ` [for-next][PATCH 17/24] tracing: Remove hist trigger synth_var_refs Steven Rostedt
2018-12-21 17:56 ` [for-next][PATCH 18/24] tracing: Add hist trigger comments for variable-related fields Steven Rostedt
2018-12-21 17:56 ` [for-next][PATCH 19/24] tracing: Merge seq_print_sym_short() and seq_print_sym_offset() Steven Rostedt
2018-12-21 17:56 ` [for-next][PATCH 21/24] tracing: Simplify printfing in seq_print_sym Steven Rostedt
2018-12-21 17:56 ` [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro Steven Rostedt
2018-12-21 18:51   ` Linus Torvalds
2018-12-21 19:40     ` Steven Rostedt
2018-12-21 20:01       ` Linus Torvalds
2018-12-21 20:35         ` Steven Rostedt
2018-12-21 20:46           ` Joe Perches
2018-12-21 20:54             ` Steven Rostedt
2018-12-21 20:58               ` Andreas Schwab
2018-12-21 21:08                 ` Steven Rostedt
2018-12-21 22:20                   ` Joe Perches
2018-12-21 22:29                     ` Linus Torvalds
2018-12-21 22:58                       ` Steven Rostedt
2018-12-21 22:55                     ` Steven Rostedt
2018-12-23 22:01                     ` Rasmus Villemoes
2018-12-23 22:56                       ` Steven Rostedt
2018-12-23 23:52                         ` Rasmus Villemoes
2018-12-24  0:05                           ` Steven Rostedt
2018-12-23 23:01                       ` Joe Perches
2018-12-21 20:58             ` Steven Rostedt
     [not found]           ` <CAHk-=wjtkvFUuRNZU67KccuUKYHw=pYoDMQJ_9OVDFxOwmK9zQ@mail.gmail.com>
2018-12-21 20:55             ` Steven Rostedt
2018-12-21 22:08               ` Linus Torvalds [this message]
2018-12-21 22:48                 ` Steven Rostedt
2018-12-21 22:57                   ` Linus Torvalds
2018-12-21 23:03                     ` Steven Rostedt
2018-12-22 10:20     ` Malcolm Priestley
2018-12-22 12:26       ` Steven Rostedt
2018-12-21 17:56 ` [for-next][PATCH 24/24] tracing: Use strncmp_prefix() helper for histogram code Steven Rostedt
     [not found] ` <20181221175656.827708767@goodmis.org>
2018-12-21 18:00   ` [for-next][PATCH 09/24] seq_buf: Make seq_buf_puts() null-terminate the buffer Steven Rostedt
2018-12-21 18:05     ` Steven Rostedt
2018-12-27  5:25       ` Michael Ellerman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAHk-=wjaNWzXSW9XT-Z=9OF=5+2ft6UqjoUgaWeEWx8PUmS9xg@mail.gmail.com' \
    --to=torvalds@linux-foundation.org \
    --cc=akpm@linux-foundation.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=joe@perches.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mhiramat@kernel.org \
    --cc=mingo@kernel.org \
    --cc=namhyung@kernel.org \
    --cc=rostedt@goodmis.org \
    --cc=zanussi@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.