[PATCH] st: convert DRIVER_ATTR macros to DRIVER_ATTR_RO

[PATCH] st: convert DRIVER_ATTR macros to DRIVER_ATTR_RO

[Seymour, Shane M]

Convert DRIVER_ATTR macros to DRIVER_ATTR_RO as requested by
Greg KH. Also switched to using sprintf as nothing printed should
exceed PAGE_SIZE - based on feedback from Greg when implementing
show functions for tape stats.

Suggested-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
Signed-off-by: Shane Seymour <shane.seymour-VXdhtT5mjnY@public.gmane.org>
---
This patch was implemented on top of the previous patch to
convert to using driver attr groups.

Resending with [PATCH] at the front since I forgot to add it.
--- a/drivers/scsi/st.c	2015-06-22 20:38:16.061386739 -0500
+++ b/drivers/scsi/st.c	2015-06-23 17:29:03.547867682 -0500
@@ -4427,29 +4427,29 @@ module_exit(exit_st);
 
 
 /* The sysfs driver interface. Read-only at the moment */
-static ssize_t st_try_direct_io_show(struct device_driver *ddp, char *buf)
+static ssize_t try_direct_io_show(struct device_driver *ddp, char *buf)
 {
-	return snprintf(buf, PAGE_SIZE, "%d\n", try_direct_io);
+	return sprintf(buf, "%d\n", try_direct_io);
 }
-static DRIVER_ATTR(try_direct_io, S_IRUGO, st_try_direct_io_show, NULL);
+static DRIVER_ATTR_RO(try_direct_io);
 
-static ssize_t st_fixed_buffer_size_show(struct device_driver *ddp, char *buf)
+static ssize_t fixed_buffer_size_show(struct device_driver *ddp, char *buf)
 {
-	return snprintf(buf, PAGE_SIZE, "%d\n", st_fixed_buffer_size);
+	return sprintf(buf, "%d\n", st_fixed_buffer_size);
 }
-static DRIVER_ATTR(fixed_buffer_size, S_IRUGO, st_fixed_buffer_size_show, NULL);
+static DRIVER_ATTR_RO(fixed_buffer_size);
 
-static ssize_t st_max_sg_segs_show(struct device_driver *ddp, char *buf)
+static ssize_t max_sg_segs_show(struct device_driver *ddp, char *buf)
 {
-	return snprintf(buf, PAGE_SIZE, "%d\n", st_max_sg_segs);
+	return sprintf(buf, "%d\n", st_max_sg_segs);
 }
-static DRIVER_ATTR(max_sg_segs, S_IRUGO, st_max_sg_segs_show, NULL);
+static DRIVER_ATTR_RO(max_sg_segs);
 
-static ssize_t st_version_show(struct device_driver *ddd, char *buf)
+static ssize_t version_show(struct device_driver *ddd, char *buf)
 {
-	return snprintf(buf, PAGE_SIZE, "[%s]\n", verstr);
+	return sprintf(buf, "[%s]\n", verstr);
 }
-static DRIVER_ATTR(version, S_IRUGO, st_version_show, NULL);
+static DRIVER_ATTR_RO(version);
 
 static struct attribute *st_drv_attrs[] = {
 	&driver_attr_try_direct_io.attr,
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] st: convert DRIVER_ATTR macros to DRIVER_ATTR_RO

[Sergey Senozhatsky]

On (06/24/15 06:10), Seymour, Shane M wrote:
[..]
>  
>  /* The sysfs driver interface. Read-only at the moment */
> -static ssize_t st_try_direct_io_show(struct device_driver *ddp, char *buf)
> +static ssize_t try_direct_io_show(struct device_driver *ddp, char *buf)
>  {
> -	return snprintf(buf, PAGE_SIZE, "%d\n", try_direct_io);
> +	return sprintf(buf, "%d\n", try_direct_io);
>  }

a nitpick,

per Documentation/filesystems/sysfs.txt

:
: - show() should always use scnprintf().
:

  -ss

> -static DRIVER_ATTR(try_direct_io, S_IRUGO, st_try_direct_io_show, NULL);
> +static DRIVER_ATTR_RO(try_direct_io);
>  
> -static ssize_t st_fixed_buffer_size_show(struct device_driver *ddp, char *buf)
> +static ssize_t fixed_buffer_size_show(struct device_driver *ddp, char *buf)
>  {
> -	return snprintf(buf, PAGE_SIZE, "%d\n", st_fixed_buffer_size);
> +	return sprintf(buf, "%d\n", st_fixed_buffer_size);
>  }
> -static DRIVER_ATTR(fixed_buffer_size, S_IRUGO, st_fixed_buffer_size_show, NULL);
> +static DRIVER_ATTR_RO(fixed_buffer_size);
>  
> -static ssize_t st_max_sg_segs_show(struct device_driver *ddp, char *buf)
> +static ssize_t max_sg_segs_show(struct device_driver *ddp, char *buf)
>  {
> -	return snprintf(buf, PAGE_SIZE, "%d\n", st_max_sg_segs);
> +	return sprintf(buf, "%d\n", st_max_sg_segs);
>  }
> -static DRIVER_ATTR(max_sg_segs, S_IRUGO, st_max_sg_segs_show, NULL);
> +static DRIVER_ATTR_RO(max_sg_segs);
>  
> -static ssize_t st_version_show(struct device_driver *ddd, char *buf)
> +static ssize_t version_show(struct device_driver *ddd, char *buf)
>  {
> -	return snprintf(buf, PAGE_SIZE, "[%s]\n", verstr);
> +	return sprintf(buf, "[%s]\n", verstr);
>  }
> -static DRIVER_ATTR(version, S_IRUGO, st_version_show, NULL);
> +static DRIVER_ATTR_RO(version);
>  
>  static struct attribute *st_drv_attrs[] = {
>  	&driver_attr_try_direct_io.attr,
> --

Re: [PATCH] st: convert DRIVER_ATTR macros to DRIVER_ATTR_RO

[Greg KH <gregkh@linuxfoundation.org> (gregkh@linuxfoundation.org)]

On Wed, Jun 24, 2015 at 03:25:57PM +0900, Sergey Senozhatsky wrote:
> On (06/24/15 06:10), Seymour, Shane M wrote:
> [..]
> >  
> >  /* The sysfs driver interface. Read-only at the moment */
> > -static ssize_t st_try_direct_io_show(struct device_driver *ddp, char *buf)
> > +static ssize_t try_direct_io_show(struct device_driver *ddp, char *buf)
> >  {
> > -	return snprintf(buf, PAGE_SIZE, "%d\n", try_direct_io);
> > +	return sprintf(buf, "%d\n", try_direct_io);
> >  }
> 
> a nitpick,
> 
> per Documentation/filesystems/sysfs.txt
> 
> :
> : - show() should always use scnprintf().
> :

Don't believe everything you read, this change is just fine.

But, you are doing something here that you did not say you were doing in
the changelog, so for that reason, the patch should be redone.

thanks,

greg k-h

Re: [PATCH] st: convert DRIVER_ATTR macros to DRIVER_ATTR_RO

[Greg KH <gregkh@linuxfoundation.org> (gregkh@linuxfoundation.org)]

On Wed, Jun 24, 2015 at 03:25:57PM +0900, Sergey Senozhatsky wrote:
> On (06/24/15 06:10), Seymour, Shane M wrote:
> [..]
> >  
> >  /* The sysfs driver interface. Read-only at the moment */
> > -static ssize_t st_try_direct_io_show(struct device_driver *ddp, char *buf)
> > +static ssize_t try_direct_io_show(struct device_driver *ddp, char *buf)
> >  {
> > -	return snprintf(buf, PAGE_SIZE, "%d\n", try_direct_io);
> > +	return sprintf(buf, "%d\n", try_direct_io);
> >  }
> 
> a nitpick,
> 
> per Documentation/filesystems/sysfs.txt
> 
> :
> : - show() should always use scnprintf().
> :

That should be rewritten to say, "don't use snprintf(), but scnprintf(),
if you want to.  Otherwise sprintf() should be fine as you obviously are
only returning a single value to userspace"

Or something like that.

thanks,

greg k-h

Re: [PATCH] st: convert DRIVER_ATTR macros to DRIVER_ATTR_RO

[Sergey Senozhatsky]

On (06/24/15 08:10), Greg KH <gregkh@linuxfoundation.org> (gregkh@linuxfoundation.org) wrote:
> On Wed, Jun 24, 2015 at 03:25:57PM +0900, Sergey Senozhatsky wrote:
> > On (06/24/15 06:10), Seymour, Shane M wrote:
> > [..]
> > >  
> > >  /* The sysfs driver interface. Read-only at the moment */
> > > -static ssize_t st_try_direct_io_show(struct device_driver *ddp, char *buf)
> > > +static ssize_t try_direct_io_show(struct device_driver *ddp, char *buf)
> > >  {
> > > -	return snprintf(buf, PAGE_SIZE, "%d\n", try_direct_io);
> > > +	return sprintf(buf, "%d\n", try_direct_io);
> > >  }
> > 
> > a nitpick,
> > 
> > per Documentation/filesystems/sysfs.txt
> > 
> > :
> > : - show() should always use scnprintf().
> > :
> 
> That should be rewritten to say, "don't use snprintf(), but scnprintf(),
> if you want to.  Otherwise sprintf() should be fine as you obviously are
> only returning a single value to userspace"
> 

Sure, that was just a nitpick. For '%d' it's totally fine, I agree.
It was more of a 'do we strictly obey the rules' thing.

	-ss