| Message ID | 20250313142557.36936-2-thorsten.blum@linux.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND] scsi: sd: Use str_on_off() helper in sd_read_write_protect_flag() | expand |
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.
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
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.
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); } }
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(-)