[RESEND PATCH] scsi: sd: Use str_on_off() helper in sd_read_write_protect_flag()

[RESEND PATCH] scsi: sd: Use str_on_off() helper in sd_read_write_protect_flag()

[Thorsten Blum]

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

Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
 drivers/scsi/sd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 950d8c9fb884..96f64237360f 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -50,6 +50,7 @@
 #include <linux/rw_hint.h>
 #include <linux/major.h>
 #include <linux/mutex.h>
+#include <linux/string_choices.h>
 #include <linux/string_helpers.h>
 #include <linux/slab.h>
 #include <linux/sed-opal.h>
@@ -3004,7 +3005,7 @@ sd_read_write_protect_flag(struct scsi_disk *sdkp, unsigned char *buffer)
 		set_disk_ro(sdkp->disk, sdkp->write_prot);
 		if (sdkp->first_scan || old_wp != sdkp->write_prot) {
 			sd_printk(KERN_NOTICE, sdkp, "Write Protect is %s\n",
-				  sdkp->write_prot ? "on" : "off");
+				  str_on_off(sdkp->write_prot));
 			sd_printk(KERN_DEBUG, sdkp, "Mode Sense: %4ph\n", buffer);
 		}
 	}
-- 
2.48.1

Re: [RESEND PATCH] scsi: sd: Use str_on_off() helper in sd_read_write_protect_flag()

[Bart Van Assche]

On 3/13/25 7:25 AM, Thorsten Blum wrote:
> Remove hard-coded strings by using the str_on_off() helper function.

Shouldn't a patch description explain two things: what has been changed
and also why a change has been made? I don't see an explanation of why
this change has been made.

> @@ -3004,7 +3005,7 @@ sd_read_write_protect_flag(struct scsi_disk *sdkp, unsigned char *buffer)
>   		set_disk_ro(sdkp->disk, sdkp->write_prot);
>   		if (sdkp->first_scan || old_wp != sdkp->write_prot) {
>   			sd_printk(KERN_NOTICE, sdkp, "Write Protect is %s\n",
> -				  sdkp->write_prot ? "on" : "off");
> +				  str_on_off(sdkp->write_prot));
>   			sd_printk(KERN_DEBUG, sdkp, "Mode Sense: %4ph\n", buffer);
>   		}
>   	}

I prefer the current code (without this patch). Others may have another
opinion.

Thanks,

Bart.

Re: [RESEND PATCH] scsi: sd: Use str_on_off() helper in sd_read_write_protect_flag()

[Thorsten Blum]

Hi Bart,

On 13. Mar 2025, at 17:25, Bart Van Assche wrote:
> On 3/13/25 7:25 AM, Thorsten Blum wrote:
>> Remove hard-coded strings by using the str_on_off() helper function.
> 
> Shouldn't a patch description explain two things: what has been changed
> and also why a change has been made? I don't see an explanation of why
> this change has been made.

The benefits are explained in linux/string_choices.h and I didn't think
it would be necessary to repeat them in the commit message:

/*
 * 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: [RESEND PATCH] scsi: sd: Use str_on_off() helper in sd_read_write_protect_flag()

[Bart Van Assche]

On 3/13/25 10:05 AM, Thorsten Blum wrote:
>   *  3) Deduping by the linker, which results in a smaller binary file.

Hmm ... I'm not sure that using functions like str_on_off() will result
in a smaller binary file since the str_on_off() function (and other
similar helper functions) have been declared inline. Additionally, isn't
a linker supposed to deduplicate literal strings even if str_on_off()
is not used? Isn't the compiler assumed to merge identical literal
strings if -fmerge-constants has been specified? From the GCC
documentation about that option: "Enabled at levels -O1, -O2, -O3, -Os."

Bart.