linux-kernel - Re: [PATCH] proc: Use str_yes_no() helper in proc_pid_ksm

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] [day] [month] [year] [list]

Message-ID: <20250212160451.GB106698@mit.edu>
Date: Wed, 12 Feb 2025 11:04:51 -0500
From: "Theodore Ts'o" <tytso@....edu>
To: Al Viro <viro@...iv.linux.org.uk>
Cc: Thorsten Blum <thorsten.blum@...ux.dev>,
        Christian Brauner <brauner@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jeff Layton <jlayton@...nel.org>,
        Adrian Ratiu <adrian.ratiu@...labora.com>,
        xu xin <xu.xin16@....com.cn>,
        Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
        Felix Moessbauer <felix.moessbauer@...mens.com>,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] proc: Use str_yes_no() helper in proc_pid_ksm_stat()

On Wed, Feb 12, 2025 at 12:36:09PM +0000, Al Viro wrote:
> On Wed, Feb 12, 2025 at 01:11:08PM +0100, Thorsten Blum wrote:
> > /*
> >  * Here provide a series of helpers in the str_$TRUE_$FALSE format (you can
> >  * also expand some helpers as needed), where $TRUE and $FALSE are their
> >  * corresponding literal strings. These helpers can be used in the printing
> >  * and also in other places where constant strings are required. Using these
> >  * helpers offers the following benefits:
> >  *  1) Reducing the hardcoding of strings, which makes the code more elegant
> >  *     through these simple literal-meaning helpers.
> >  *  2) Unifying the output, which prevents the same string from being printed
> >  *     in various forms, such as enable/disable, enabled/disabled, en/dis.
> >  *  3) Deduping by the linker, which results in a smaller binary file.
> >  */
> 
> Printf modifiers would've covered all of that, though...
> 
> The thing is, <expr> ? "yes" : "no" is visually easier to distinguish than
> str_yes_no(<expr>), especially when expression itself is a function call, etc.
> So I'd question elegance, actually...

Yeah, personally I think str_yes_no() is a bit of a toss-up from an
elegance perspective.  It's a fewer number of characters, sure, but
it's a bit more cognitive load where the people reading the code need
to map str_yes_no() to semantic meaning.  For someone who is doing
wholesale conversions, they probably don't notice it, but for someone
who isn't dealing with it every day, it's One More Thing that they
have to track.

Using printf modifiers would probably actually save more bytes from a
"size of the binary" perspective, at the cost of forcing the code
reader to deal with some line-noise perl- or Rust-like construction
(e.g., if %pcy is str_yes_no, %pco is str_on_off, %pct is
str_true_false, etc. is the increased cognitive load worth it?)

Bottom line, I don't *hate* the string choices, but I really don't
think it's an obvious improvement.  Thorsten, this is why I hadn't
processed your ext4 patch yet.  I was super busy over the holidays,
and from a prioritization perspective, I didn't consider it high
priority.  Thinking about it now, to be honest, I'm feeling really
ambivilent about string_choices.h

Cheers,

					- Ted