diff mbox series

scsi: hpsa: Use min() to simplify code

Message ID 20250212115557.111263-2-thorsten.blum@linux.dev (mailing list archive)
State Superseded
Headers show
Series scsi: hpsa: Use min() to simplify code | expand

Commit Message

Thorsten Blum Feb. 12, 2025, 11:55 a.m. UTC
Use min() to simplify the host_store_hp_ssd_smart_path_status() and
host_store_raid_offload_debug() functions.

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

Comments

Bart Van Assche Feb. 12, 2025, 6:48 p.m. UTC | #1
On 2/12/25 3:55 AM, Thorsten Blum wrote:
> Use min() to simplify the host_store_hp_ssd_smart_path_status() and
> host_store_raid_offload_debug() functions.
> 
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
>   drivers/scsi/hpsa.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index 84d8de07b7ae..1d19eb2ca1d3 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -460,7 +460,7 @@ static ssize_t host_store_hp_ssd_smart_path_status(struct device *dev,
>   
>   	if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
>   		return -EACCES;
> -	len = count > sizeof(tmpbuf) - 1 ? sizeof(tmpbuf) - 1 : count;
> +	len = min(count, sizeof(tmpbuf) - 1);
>   	strncpy(tmpbuf, buf, len);
>   	tmpbuf[len] = '\0';
>   	if (sscanf(tmpbuf, "%d", &status) != 1)
> @@ -484,7 +484,7 @@ static ssize_t host_store_raid_offload_debug(struct device *dev,
>   
>   	if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
>   		return -EACCES;
> -	len = count > sizeof(tmpbuf) - 1 ? sizeof(tmpbuf) - 1 : count;
> +	len = min(count, sizeof(tmpbuf) - 1);
>   	strncpy(tmpbuf, buf, len);
>   	tmpbuf[len] = '\0';
>   	if (sscanf(tmpbuf, "%d", &debug_level) != 1)

 From Documentation/process/deprecated.rst:

<quote>
strncpy() on NUL-terminated strings
-----------------------------------
Use of strncpy() does not guarantee that the destination buffer will
be NUL terminated. This can lead to various linear read overflows and
other misbehavior due to the missing termination. It also NUL-pads
the destination buffer if the source contents are shorter than the
destination buffer size, which may be a needless performance penalty
for callers using only NUL-terminated strings.

When the destination is required to be NUL-terminated, the replacement is
strscpy(), though care must be given to any cases where the return value
of strncpy() was used, since strscpy() does not return a pointer to the
destination, but rather a count of non-NUL bytes copied (or negative
errno when it truncates). Any cases still needing NUL-padding should
instead use strscpy_pad().

If a caller is using non-NUL-terminated strings, strtomem() should be
used, and the destinations should be marked with the `__nonstring
<https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html>`_
attribute to avoid future compiler warnings. For cases still needing
NUL-padding, strtomem_pad() can be used.
</quote>

Instead of only changing the calculation of 'len', please change the
strncpy() calls into strscpy() calls.

Thanks,

Bart.
Thorsten Blum Feb. 12, 2025, 10:28 p.m. UTC | #2
On 12. Feb 2025, at 19:48, Bart Van Assche wrote:
> On 2/12/25 3:55 AM, Thorsten Blum wrote:
>> Use min() to simplify the host_store_hp_ssd_smart_path_status() and
>> host_store_raid_offload_debug() functions.
>> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
>> ---
> 
> From Documentation/process/deprecated.rst:
> 
> <quote>
> strncpy() on NUL-terminated strings
> -----------------------------------
> Use of strncpy() does not guarantee that the destination buffer will
> be NUL terminated. This can lead to various linear read overflows and
> other misbehavior due to the missing termination. It also NUL-pads
> the destination buffer if the source contents are shorter than the
> destination buffer size, which may be a needless performance penalty
> for callers using only NUL-terminated strings.
> 
> When the destination is required to be NUL-terminated, the replacement is
> strscpy(), though care must be given to any cases where the return value
> of strncpy() was used, since strscpy() does not return a pointer to the
> destination, but rather a count of non-NUL bytes copied (or negative
> errno when it truncates). Any cases still needing NUL-padding should
> instead use strscpy_pad().
> 
> If a caller is using non-NUL-terminated strings, strtomem() should be
> used, and the destinations should be marked with the `__nonstring
> <https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html>`_
> attribute to avoid future compiler warnings. For cases still needing
> NUL-padding, strtomem_pad() can be used.
> </quote>
> 
> Instead of only changing the calculation of 'len', please change the
> strncpy() calls into strscpy() calls.

Thank you for the suggestion.

I just sent a new patch [1] essentially replacing this one.

Best,
Thorsten

[1] https://lore.kernel.org/r/20250212222214.86110-2-thorsten.blum@linux.dev/
diff mbox series

Patch

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 84d8de07b7ae..1d19eb2ca1d3 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -460,7 +460,7 @@  static ssize_t host_store_hp_ssd_smart_path_status(struct device *dev,
 
 	if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
 		return -EACCES;
-	len = count > sizeof(tmpbuf) - 1 ? sizeof(tmpbuf) - 1 : count;
+	len = min(count, sizeof(tmpbuf) - 1);
 	strncpy(tmpbuf, buf, len);
 	tmpbuf[len] = '\0';
 	if (sscanf(tmpbuf, "%d", &status) != 1)
@@ -484,7 +484,7 @@  static ssize_t host_store_raid_offload_debug(struct device *dev,
 
 	if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
 		return -EACCES;
-	len = count > sizeof(tmpbuf) - 1 ? sizeof(tmpbuf) - 1 : count;
+	len = min(count, sizeof(tmpbuf) - 1);
 	strncpy(tmpbuf, buf, len);
 	tmpbuf[len] = '\0';
 	if (sscanf(tmpbuf, "%d", &debug_level) != 1)