sd: fix sysfs writes to "provisioning_mode" and "zeroing_mode"

Commit Message

Ewan Milne May 15, 2017, 7:13 p.m. UTC

From: "Ewan D. Milne" <[e-mail]>

Change to use strlen() of the desired string for the length
parameter to strncmp().  Otherwise one cannot simply use a
command like 'echo "writesame_16" > .../provisioning_mode'.
This patch makes sysfs writes consistent with other usage.

Signed-off-by: Ewan D. Milne <[e-mail]>
---
 drivers/scsi/sd.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

Comments

Bart Van Assche May 15, 2017, 8:14 p.m. UTC | #1

On Mon, 2017-05-15 at 15:13 -0400, Ewan D. Milne wrote:
> From: "Ewan D. Milne" <[e-mail]>
> 
> Change to use strlen() of the desired string for the length
> parameter to strncmp().  Otherwise one cannot simply use a
> command like 'echo "writesame_16" > .../provisioning_mode'.
> This patch makes sysfs writes consistent with other usage.

Hello Ewan,

Sorry but I don't like the approach of this patch. Have you considered
to strip the whitespace from 'buf' with e.g. strim() such that strcmp()
can be used instead of strncmp(buf, ..., strlen(...))?

Thanks,

Bart.

Ewan Milne May 15, 2017, 9:02 p.m. UTC | #2

On Mon, 2017-05-15 at 20:14 +0000, Bart Van Assche wrote:
> On Mon, 2017-05-15 at 15:13 -0400, Ewan D. Milne wrote:
> > From: "Ewan D. Milne" <[e-mail]>
> > 
> > Change to use strlen() of the desired string for the length
> > parameter to strncmp().  Otherwise one cannot simply use a
> > command like 'echo "writesame_16" > .../provisioning_mode'.
> > This patch makes sysfs writes consistent with other usage.
> 
> Hello Ewan,
> 
> Sorry but I don't like the approach of this patch. Have you considered
> to strip the whitespace from 'buf' with e.g. strim() such that strcmp()
> can be used instead of strncmp(buf, ..., strlen(...))?
> 
> Thanks,
> 
> Bart.

I think it's the '\n' that actually causes the problem.
But, what I think we should do is use the same technique everywhere.
So, are you suggesting we change all the other sysfs store routines?

The problem arises when scripts can manipulate some sysfs properties
but not others, and its not obvious why not.  It should be consistent.

Also any change has to be careful to avoid breaking existing scripts.

-Ewan

Bart Van Assche May 15, 2017, 9:14 p.m. UTC | #3

On Mon, 2017-05-15 at 17:02 -0400, Ewan D. Milne wrote:
> I think it's the '\n' that actually causes the problem.

Agreed, and that's why I proposed to strip trailing whitespace.

> But, what I think we should do is use the same technique everywhere.

If you have a look at the block layer you will see that the block layer
sysfs code is already using strim() and strstrip().

> So, are you suggesting we change all the other sysfs store routines?

I will leave it to Martin and James to comment on this.

Bart.

Martin K. Petersen May 15, 2017, 9:18 p.m. UTC | #4

Bart,

>> So, are you suggesting we change all the other sysfs store routines?
>
> I will leave it to Martin and James to comment on this.

As I mentioned a few weeks ago, I have a patch that converts all these
to sysfs_match_string(). Now that the latter is upstream I intend to
post the patch.

Ewan Milne May 16, 2017, 12:52 p.m. UTC | #5

On Mon, 2017-05-15 at 17:18 -0400, Martin K. Petersen wrote:
> Bart,
> 
> >> So, are you suggesting we change all the other sysfs store routines?
> >
> > I will leave it to Martin and James to comment on this.
> 
> As I mentioned a few weeks ago, I have a patch that converts all these
> to sysfs_match_string(). Now that the latter is upstream I intend to
> post the patch.
> 

OK good.  One other thing I wanted to check -- I assume it is
intentional that the provisioning mode cannot be set back to "full"
once the device probe has set it and/or it has been manually set via
sysfs?

-Ewan

Patch

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index f9d1432..a5eacea 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -402,15 +402,20 @@  provisioning_mode_store(struct device *dev, struct device_attribute *attr,
 	if (sdp->type != TYPE_DISK)
 		return -EINVAL;
 
-	if (!strncmp(buf, lbp_mode[SD_LBP_UNMAP], 20))
+	if (!strncmp(buf, lbp_mode[SD_LBP_UNMAP],
+		     strlen(lbp_mode[SD_LBP_UNMAP])))
 		sd_config_discard(sdkp, SD_LBP_UNMAP);
-	else if (!strncmp(buf, lbp_mode[SD_LBP_WS16], 20))
+	else if (!strncmp(buf, lbp_mode[SD_LBP_WS16],
+			  strlen(lbp_mode[SD_LBP_WS16])))
 		sd_config_discard(sdkp, SD_LBP_WS16);
-	else if (!strncmp(buf, lbp_mode[SD_LBP_WS10], 20))
+	else if (!strncmp(buf, lbp_mode[SD_LBP_WS10],
+			  strlen(lbp_mode[SD_LBP_WS10])))
 		sd_config_discard(sdkp, SD_LBP_WS10);
-	else if (!strncmp(buf, lbp_mode[SD_LBP_ZERO], 20))
+	else if (!strncmp(buf, lbp_mode[SD_LBP_ZERO],
+			  strlen(lbp_mode[SD_LBP_ZERO])))
 		sd_config_discard(sdkp, SD_LBP_ZERO);
-	else if (!strncmp(buf, lbp_mode[SD_LBP_DISABLE], 20))
+	else if (!strncmp(buf, lbp_mode[SD_LBP_DISABLE],
+			  strlen(lbp_mode[SD_LBP_DISABLE])))
 		sd_config_discard(sdkp, SD_LBP_DISABLE);
 	else
 		return -EINVAL;
@@ -444,13 +449,17 @@  zeroing_mode_store(struct device *dev, struct device_attribute *attr,
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;
 
-	if (!strncmp(buf, zeroing_mode[SD_ZERO_WRITE], 20))
+	if (!strncmp(buf, zeroing_mode[SD_ZERO_WRITE],
+		     strlen(zeroing_mode[SD_ZERO_WRITE])))
 		sdkp->zeroing_mode = SD_ZERO_WRITE;
-	else if (!strncmp(buf, zeroing_mode[SD_ZERO_WS], 20))
+	else if (!strncmp(buf, zeroing_mode[SD_ZERO_WS],
+			  strlen(zeroing_mode[SD_ZERO_WS])))
 		sdkp->zeroing_mode = SD_ZERO_WS;
-	else if (!strncmp(buf, zeroing_mode[SD_ZERO_WS16_UNMAP], 20))
+	else if (!strncmp(buf, zeroing_mode[SD_ZERO_WS16_UNMAP],
+			  strlen(zeroing_mode[SD_ZERO_WS16_UNMAP])))
 		sdkp->zeroing_mode = SD_ZERO_WS16_UNMAP;
-	else if (!strncmp(buf, zeroing_mode[SD_ZERO_WS10_UNMAP], 20))
+	else if (!strncmp(buf, zeroing_mode[SD_ZERO_WS10_UNMAP],
+			  strlen(zeroing_mode[SD_ZERO_WS10_UNMAP])))
 		sdkp->zeroing_mode = SD_ZERO_WS10_UNMAP;
 	else
 		return -EINVAL;