[PATCH 1/3] hpsa: convert show method snprintf usage to scnprintf

[PATCH 1/3] hpsa: convert show method snprintf usage to scnprintf

[Seymour, Shane M]

Changed all show method snprintf usage to scnprintf per
Documentation/filesystems/sysfs.txt.

Signed-off-by: Shane Seymour <shane.seymour-VXdhtT5mjnY@public.gmane.org>
---
Please let me know if this is not the correct way to submit
patches by separating them but keeping them logically
together.
--- a/drivers/scsi/hpsa.c	2015-06-25 15:52:15.633031319 -0500
+++ b/drivers/scsi/hpsa.c	2015-06-30 16:12:58.125990687 -0500
@@ -460,7 +460,7 @@ static ssize_t host_show_firmware_revisi
 	if (!h->hba_inquiry_data)
 		return 0;
 	fwrev = &h->hba_inquiry_data[32];
-	return snprintf(buf, 20, "%c%c%c%c\n",
+	return scnprintf(buf, 20, "%c%c%c%c\n",
 		fwrev[0], fwrev[1], fwrev[2], fwrev[3]);
 }
 
@@ -470,7 +470,7 @@ static ssize_t host_show_commands_outsta
 	struct Scsi_Host *shost = class_to_shost(dev);
 	struct ctlr_info *h = shost_to_hba(shost);
 
-	return snprintf(buf, 20, "%d\n",
+	return scnprintf(buf, 20, "%d\n",
 			atomic_read(&h->commands_outstanding));
 }
 
@@ -481,7 +481,7 @@ static ssize_t host_show_transport_mode(
 	struct Scsi_Host *shost = class_to_shost(dev);
 
 	h = shost_to_hba(shost);
-	return snprintf(buf, 20, "%s\n",
+	return scnprintf(buf, 20, "%s\n",
 		h->transMethod & CFGTBL_Trans_Performant ?
 			"performant" : "simple");
 }
@@ -493,7 +493,7 @@ static ssize_t host_show_hp_ssd_smart_pa
 	struct Scsi_Host *shost = class_to_shost(dev);
 
 	h = shost_to_hba(shost);
-	return snprintf(buf, 30, "HP SSD Smart Path %s\n",
+	return scnprintf(buf, 30, "HP SSD Smart Path %s\n",
 		(h->acciopath_status == 1) ?  "enabled" : "disabled");
 }
 
@@ -589,7 +589,7 @@ static ssize_t host_show_resettable(stru
 	struct Scsi_Host *shost = class_to_shost(dev);
 
 	h = shost_to_hba(shost);
-	return snprintf(buf, 20, "%d\n", ctlr_is_resettable(h->board_id));
+	return scnprintf(buf, 20, "%d\n", ctlr_is_resettable(h->board_id));
 }
 
 static inline int is_logical_dev_addr_mode(unsigned char scsi3addr[])
@@ -631,7 +631,7 @@ static ssize_t raid_level_show(struct de
 	/* Is this even a logical drive? */
 	if (!is_logical_dev_addr_mode(hdev->scsi3addr)) {
 		spin_unlock_irqrestore(&h->lock, flags);
-		l = snprintf(buf, PAGE_SIZE, "N/A\n");
+		l = scnprintf(buf, PAGE_SIZE, "N/A\n");
 		return l;
 	}
 
@@ -639,7 +639,7 @@ static ssize_t raid_level_show(struct de
 	spin_unlock_irqrestore(&h->lock, flags);
 	if (rlevel > RAID_UNKNOWN)
 		rlevel = RAID_UNKNOWN;
-	l = snprintf(buf, PAGE_SIZE, "RAID %s\n", raid_label[rlevel]);
+	l = scnprintf(buf, PAGE_SIZE, "RAID %s\n", raid_label[rlevel]);
 	return l;
 }
 
@@ -662,7 +662,7 @@ static ssize_t lunid_show(struct device
 	}
 	memcpy(lunid, hdev->scsi3addr, sizeof(lunid));
 	spin_unlock_irqrestore(&h->lock, flags);
-	return snprintf(buf, 20, "0x%02x%02x%02x%02x%02x%02x%02x%02x\n",
+	return scnprintf(buf, 20, "0x%02x%02x%02x%02x%02x%02x%02x%02x\n",
 		lunid[0], lunid[1], lunid[2], lunid[3],
 		lunid[4], lunid[5], lunid[6], lunid[7]);
 }
@@ -686,7 +686,7 @@ static ssize_t unique_id_show(struct dev
 	}
 	memcpy(sn, hdev->device_id, sizeof(sn));
 	spin_unlock_irqrestore(&h->lock, flags);
-	return snprintf(buf, 16 * 2 + 2,
+	return scnprintf(buf, 16 * 2 + 2,
 			"%02X%02X%02X%02X%02X%02X%02X%02X"
 			"%02X%02X%02X%02X%02X%02X%02X%02X\n",
 			sn[0], sn[1], sn[2], sn[3],
@@ -714,7 +714,7 @@ static ssize_t host_show_hp_ssd_smart_pa
 	}
 	offload_enabled = hdev->offload_enabled;
 	spin_unlock_irqrestore(&h->lock, flags);
-	return snprintf(buf, 20, "%d\n", offload_enabled);
+	return scnprintf(buf, 20, "%d\n", offload_enabled);
 }
 
 static DEVICE_ATTR(raid_level, S_IRUGO, raid_level_show, NULL);

Re: [PATCH 1/3] hpsa: convert show method snprintf usage to scnprintf

[James Bottomley]

On Wed, 2015-07-01 at 03:45 +0000, Seymour, Shane M wrote:
> Changed all show method snprintf usage to scnprintf per
> Documentation/filesystems/sysfs.txt.
> 
> Signed-off-by: Shane Seymour <shane.seymour@hp.com>

There's been no ack on this one.  However, there's no actual reason to
prefer scnprintf over snprintf: the former will zero terminate, the
latter won't if the write length is over the buffer length, but this is
a file buffer: the routine will return as many bytes to userspace as are
specified in the count (including zeros if they're within the count), so
zero termination of a string in sysfs is unnecessary.

James


> Please let me know if this is not the correct way to submit
> patches by separating them but keeping them logically
> together.
> --- a/drivers/scsi/hpsa.c	2015-06-25 15:52:15.633031319 -0500
> +++ b/drivers/scsi/hpsa.c	2015-06-30 16:12:58.125990687 -0500
> @@ -460,7 +460,7 @@ static ssize_t host_show_firmware_revisi
>  	if (!h->hba_inquiry_data)
>  		return 0;
>  	fwrev = &h->hba_inquiry_data[32];
> -	return snprintf(buf, 20, "%c%c%c%c\n",
> +	return scnprintf(buf, 20, "%c%c%c%c\n",
>  		fwrev[0], fwrev[1], fwrev[2], fwrev[3]);
>  }
>  
> @@ -470,7 +470,7 @@ static ssize_t host_show_commands_outsta
>  	struct Scsi_Host *shost = class_to_shost(dev);
>  	struct ctlr_info *h = shost_to_hba(shost);
>  
> -	return snprintf(buf, 20, "%d\n",
> +	return scnprintf(buf, 20, "%d\n",
>  			atomic_read(&h->commands_outstanding));
>  }
>  
> @@ -481,7 +481,7 @@ static ssize_t host_show_transport_mode(
>  	struct Scsi_Host *shost = class_to_shost(dev);
>  
>  	h = shost_to_hba(shost);
> -	return snprintf(buf, 20, "%s\n",
> +	return scnprintf(buf, 20, "%s\n",
>  		h->transMethod & CFGTBL_Trans_Performant ?
>  			"performant" : "simple");
>  }
> @@ -493,7 +493,7 @@ static ssize_t host_show_hp_ssd_smart_pa
>  	struct Scsi_Host *shost = class_to_shost(dev);
>  
>  	h = shost_to_hba(shost);
> -	return snprintf(buf, 30, "HP SSD Smart Path %s\n",
> +	return scnprintf(buf, 30, "HP SSD Smart Path %s\n",
>  		(h->acciopath_status == 1) ?  "enabled" : "disabled");
>  }
>  
> @@ -589,7 +589,7 @@ static ssize_t host_show_resettable(stru
>  	struct Scsi_Host *shost = class_to_shost(dev);
>  
>  	h = shost_to_hba(shost);
> -	return snprintf(buf, 20, "%d\n", ctlr_is_resettable(h->board_id));
> +	return scnprintf(buf, 20, "%d\n", ctlr_is_resettable(h->board_id));
>  }
>  
>  static inline int is_logical_dev_addr_mode(unsigned char scsi3addr[])
> @@ -631,7 +631,7 @@ static ssize_t raid_level_show(struct de
>  	/* Is this even a logical drive? */
>  	if (!is_logical_dev_addr_mode(hdev->scsi3addr)) {
>  		spin_unlock_irqrestore(&h->lock, flags);
> -		l = snprintf(buf, PAGE_SIZE, "N/A\n");
> +		l = scnprintf(buf, PAGE_SIZE, "N/A\n");
>  		return l;
>  	}
>  
> @@ -639,7 +639,7 @@ static ssize_t raid_level_show(struct de
>  	spin_unlock_irqrestore(&h->lock, flags);
>  	if (rlevel > RAID_UNKNOWN)
>  		rlevel = RAID_UNKNOWN;
> -	l = snprintf(buf, PAGE_SIZE, "RAID %s\n", raid_label[rlevel]);
> +	l = scnprintf(buf, PAGE_SIZE, "RAID %s\n", raid_label[rlevel]);
>  	return l;
>  }
>  
> @@ -662,7 +662,7 @@ static ssize_t lunid_show(struct device
>  	}
>  	memcpy(lunid, hdev->scsi3addr, sizeof(lunid));
>  	spin_unlock_irqrestore(&h->lock, flags);
> -	return snprintf(buf, 20, "0x%02x%02x%02x%02x%02x%02x%02x%02x\n",
> +	return scnprintf(buf, 20, "0x%02x%02x%02x%02x%02x%02x%02x%02x\n",
>  		lunid[0], lunid[1], lunid[2], lunid[3],
>  		lunid[4], lunid[5], lunid[6], lunid[7]);
>  }
> @@ -686,7 +686,7 @@ static ssize_t unique_id_show(struct dev
>  	}
>  	memcpy(sn, hdev->device_id, sizeof(sn));
>  	spin_unlock_irqrestore(&h->lock, flags);
> -	return snprintf(buf, 16 * 2 + 2,
> +	return scnprintf(buf, 16 * 2 + 2,
>  			"%02X%02X%02X%02X%02X%02X%02X%02X"
>  			"%02X%02X%02X%02X%02X%02X%02X%02X\n",
>  			sn[0], sn[1], sn[2], sn[3],
> @@ -714,7 +714,7 @@ static ssize_t host_show_hp_ssd_smart_pa
>  	}
>  	offload_enabled = hdev->offload_enabled;
>  	spin_unlock_irqrestore(&h->lock, flags);
> -	return snprintf(buf, 20, "%d\n", offload_enabled);
> +	return scnprintf(buf, 20, "%d\n", offload_enabled);
>  }
>  
>  static DEVICE_ATTR(raid_level, S_IRUGO, raid_level_show, NULL);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

RE: [PATCH 1/3] hpsa: convert show method snprintf usage to scnprintf

[Seymour, Shane M]

Hi James,

> There's been no ack on this one.  However, there's no actual reason to
> prefer scnprintf over snprintf: the former will zero terminate, the
> latter won't if the write length is over the buffer length, but this is
> a file buffer: the routine will return as many bytes to userspace as are
> specified in the count (including zeros if they're within the count), so
> zero termination of a string in sysfs is unnecessary.

There's a patch in Greg's driver tree for the next merge that changes
the documentation about the usage of the s*printf() functions in sysfs
show() methods from/to (in Documentation/filesystems/sysfs.txt):

-- show() should always use scnprintf().
+- show() must not use snprintf() when formatting the value to be
+  returned to user space. If you can guarantee that an overflow
+  will never happen you can use sprintf() otherwise you must use
+  scnprintf().

It currently says you should use scnprintf() but will become more
explicit about what you must not use and what you can or must use.
That's probably the best reason I can offer about why to prefer one
function over the other.

This is my understanding of the difference between snprintf() and
scnprintf() in terms of sysfs show() methods - there is a subtle
difference between the two functions in the return value.

The snprintf() function returns the number of bytes that it would
have formatted given sufficient space. It doesn't matter what the
size argument was. If the size passed in is 4096 and the number of
bytes that it would have formatted is 4200 then 4200 will be what is
returned from snprintf() even though it did not modify anything
after byte 4096 in the buffer.

The scnprintf() function returns the number of bytes it actually
formatted (excluding the zero termination). Using the above data
if 4096 was the size passed in then the return value will never be
more than 4095.

There is code in sysfs_kf_seq_show() to make sure that the count
returned from the show() method is not >= PAGE_SIZE and
reduce it to PAGE_SIZE-1 if it was. I don't think user space will 
ever get more than PAGE_SIZE-1 bytes regardless of which
function is used.

I don't mind if the patch isn't accepted but I thought I should at
least explain my rationale behind the change.

Thanks
Shane