Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20181221175618.968519903@goodmis.org> <20181221175659.208858193@goodmis.org>
In-Reply-To: <20181221175659.208858193@goodmis.org>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Fri, 21 Dec 2018 10:51:29 -0800
Message-ID: <CAHk-=wg_sR-UEC1ggmkZpypOUYanL5CMX4R7ceuaV4QMf5jBtg@mail.gmail.com>
Subject: Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
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>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Dec 21, 2018 at 9:57 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> I figured the best thing to do is to create a helper macro and place it
> into include/linux/string.h. And go around and fix all the open coded
> versions of it later.
>
> I plan on only applying this patch and updating the tracing hooks for
> this merge window. And perhaps use it to fix some of the bugs that were
> found.

I like the helper function concept, but as they say about CS: "There
is only one hard problem in computer science: naming and off-by-one
errors".

And this one has that problem. The name makes absolutely no sense.

Calling it "strncmp_prefix()" when there is no "n" there makes no sense.

So drop the "n" from the name.

And honestly, maybe it would be better to use a different naming
pattern entirely, and avoid the crazy non-boolean "str*cmp()" model.
That thing is useful for search comparisons (where "before/after"
matters), but it's horrible for the actual "is this true or not",
where the code ends up being

        if (!strcmp_prefix(a, "prefix")) {
                // This is the "Yes, prefix matched" case, despite the
"if !" syntax

which is just confusing.

So I'd really prefer more of a

        #define has_prefix(str, prefix) ...

model that actually returns a boolean (true if it has a prefix, false
if it doesn't), rather than use the "str*cmp" naming and return
values.

(But I agree that *if* you use the "strcmp" naming, then you do need
to hold to the traditional strcmp return value semantics).

Hmm?

Finally, I also suspect that your helper might be slightly fragile.
Doing sizeof() seems broken. I could see somebody using some prefix
define for arrays with constant sizes, and doing

   #define PFX1 "cpp\0"
   #define PFX2 "c\0\0\0"
   #define PFX3 "h\0\0\0"

or similar. Also, is there a reason you use "&prefix" for the constant
test? I don't see that pattern anywhere else. Plus it looks entirely
wrong without the parenthesis (ie "&(prefix)" to make sure we group
things right).

So a lot of small problems, but the naming one is big.

                  Linus