Re: [PATCH] usbip: handle length at sysfs show() functions

[Greg KH]

On Wed, Jun 01, 2011 at 07:14:07AM +0200, Németh Márton wrote:
> From: Márton Németh <nm127@xxxxxxxxxxx>
> 
> The sysfs show() functions shall return the actual content length of
> the result buffer. According to Documentation/filesystems/sysfs.txt:215
> the scnprintf() function is preferred.
> 
> See also the article titled "snprintf() confusion" at
> http://lwn.net/Articles/69419/ .
> 
> Signed-off-by: Márton Németh <nm127@xxxxxxxxxxx>
> ---
> diff --git a/drivers/staging/usbip/stub_dev.c b/drivers/staging/usbip/stub_dev.c
> index 6e99ec8..af5f107 100644
> --- a/drivers/staging/usbip/stub_dev.c
> +++ b/drivers/staging/usbip/stub_dev.c
> @@ -80,7 +80,7 @@ static ssize_t show_status(struct device *dev, struct device_attribute *attr,
>  	status = sdev->ud.status;
>  	spin_unlock(&sdev->ud.lock);
> 
> -	return snprintf(buf, PAGE_SIZE, "%d\n", status);
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", status);

Actually, this can be changed to just be:
	return sprintf(buf, "%d\n", status);
as obviously an integer will never be bigger than the sysfs buffer.

Good rule of thumb, if you care about the size of the sysfs buffer, you
are doing something wrong.

Case in point:

> --- a/drivers/staging/usbip/stub_main.c
> +++ b/drivers/staging/usbip/stub_main.c
> @@ -79,18 +79,22 @@ static ssize_t show_match_busid(struct device_driver *drv, char *buf)
>  {
>  	int i;
>  	char *out = buf;
> +	int count = 0;
> 
>  	spin_lock(&busid_table_lock);
> 
>  	for (i = 0; i < MAX_BUSID; i++)
> -		if (busid_table[i].name[0])
> -			out += sprintf(out, "%s ", busid_table[i].name);
> +		if (busid_table[i].name[0]) {
> +			count += scnprintf(out, PAGE_SIZE - count,
> +					"%s ", busid_table[i].name);
> +			out = buf + count;
> +		}

Here we are doing lots of work to try to put more than one value in the
sysfs file, and return the proper data to the kernel about how big the
buffer we used.

That's wrong, and violates the "one value per file" sysfs rule, so that
should be fixed instead of trying to change the sprintf() call.

Same goes for other places in this patch.

Care to redo that instead?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/