[PATCH] proc: Use str_yes_no() helper in proc_pid_ksm_stat()

[PATCH] proc: Use str_yes_no() helper in proc_pid_ksm_stat()

[Thorsten Blum]

Remove hard-coded strings by using the str_yes_no() helper function.

Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
 fs/proc/base.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index cd89e956c322..f28acc5d5ec8 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -60,6 +60,7 @@
 #include <linux/file.h>
 #include <linux/generic-radix-tree.h>
 #include <linux/string.h>
+#include <linux/string_choices.h>
 #include <linux/seq_file.h>
 #include <linux/namei.h>
 #include <linux/mnt_namespace.h>
@@ -3280,14 +3281,14 @@ static int proc_pid_ksm_stat(struct seq_file *m, struct pid_namespace *ns,
 		seq_printf(m, "ksm_merging_pages %lu\n", mm->ksm_merging_pages);
 		seq_printf(m, "ksm_process_profit %ld\n", ksm_process_profit(mm));
 		seq_printf(m, "ksm_merge_any: %s\n",
-				test_bit(MMF_VM_MERGE_ANY, &mm->flags) ? "yes" : "no");
+				str_yes_no(test_bit(MMF_VM_MERGE_ANY, &mm->flags)));
 		ret = mmap_read_lock_killable(mm);
 		if (ret) {
 			mmput(mm);
 			return ret;
 		}
 		seq_printf(m, "ksm_mergeable: %s\n",
-				ksm_process_mergeable(mm) ? "yes" : "no");
+				str_yes_no(ksm_process_mergeable(mm)));
 		mmap_read_unlock(mm);
 		mmput(mm);
 	}
-- 
2.48.1

Re: [PATCH] proc: Use str_yes_no() helper in proc_pid_ksm_stat()

[Al Viro]

On Wed, Feb 12, 2025 at 12:59:52PM +0100, Thorsten Blum wrote:
> Remove hard-coded strings by using the str_yes_no() helper function.

>  		seq_printf(m, "ksm_merge_any: %s\n",
> -				test_bit(MMF_VM_MERGE_ANY, &mm->flags) ? "yes" : "no");
> +				str_yes_no(test_bit(MMF_VM_MERGE_ANY, &mm->flags)));
>  		ret = mmap_read_lock_killable(mm);
>  		if (ret) {
>  			mmput(mm);
>  			return ret;
>  		}
>  		seq_printf(m, "ksm_mergeable: %s\n",
> -				ksm_process_mergeable(mm) ? "yes" : "no");
> +				str_yes_no(ksm_process_mergeable(mm)));

Is that any more readable?  If anything, that might be better off with something
like a printf modifier...

Re: [PATCH] proc: Use str_yes_no() helper in proc_pid_ksm_stat()

[Thorsten Blum]

On 12. Feb 2025, at 13:04, Al Viro wrote:
> On Wed, Feb 12, 2025 at 12:59:52PM +0100, Thorsten Blum wrote:
>> Remove hard-coded strings by using the str_yes_no() helper function.
> 
>> seq_printf(m, "ksm_merge_any: %s\n",
>> - test_bit(MMF_VM_MERGE_ANY, &mm->flags) ? "yes" : "no");
>> + str_yes_no(test_bit(MMF_VM_MERGE_ANY, &mm->flags)));
>> ret = mmap_read_lock_killable(mm);
>> if (ret) {
>> mmput(mm);
>> return ret;
>> }
>> seq_printf(m, "ksm_mergeable: %s\n",
>> - ksm_process_mergeable(mm) ? "yes" : "no");
>> + str_yes_no(ksm_process_mergeable(mm)));
> 
> Is that any more readable?  If anything, that might be better off with something
> like a printf modifier...

The helpers have other benefits (from include/linux/string_choices.h):

/*
 * 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.
 */

Thanks,
Thorsten

Re: [PATCH] proc: Use str_yes_no() helper in proc_pid_ksm_stat()

[Al Viro]

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...

Re: [PATCH] proc: Use str_yes_no() helper in proc_pid_ksm_stat()

[Theodore Ts'o]

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