Re: [PATCH] libfs: add simple attribute files

[Greg KH]

On Thu, May 19, 2005 at 10:29:06AM +0200, Arnd Bergmann wrote:
> On Middeweken 18 Mai 2005 22:24, Greg KH wrote:
> 
> > Thanks for the patch.  I've cleaned it up a bit (drop the spufs
> > comments, changed the access check, and made the val be u64, and
> > exported the symbols and cleaned up the debugfs portion) and added it to
> > my tree.  It should show up in the next -mm release.  I've included the
> > patch below so you can see my
> > changes.
> 
> Great, thanks for cleaning up those mistakes.
> 
> I noticed one small problem with the change from 'long' to 'u64', in 
> that you did not change it in all places. In particular, using "%lu" to
> print a u64 value will always do the wrong thing on big-endian 32 bit 
> platforms and maybe on some others.
> Since 'u64' is '%llu' on most platforms but '%lu' on some 64 bit
> platforms, I'd either do explicit cast to unsigned long long in
> the printf or use unsigned long long throughout the code.
> 
> > void foo_set(void *data, long val); and
>                            ^^     u64
> > long foo_get(void *data);
>   ^^   u64
> 
> > +#define DEFINE_SIMPLE_ATTRIBUTE(__fops, __get, __set, __fmt)		\
> > +static int __fops ## _open(struct inode *inode, struct file *file)	\
> > +{									\
> > +	__simple_attr_check_format(__fmt, 0ul);				\
>                                          ^^^^    0ull
> 
> > +	else	  /* first read */
> > +		size = scnprintf(attr->get_buf, sizeof(attr->get_buf),
> > +				 attr->fmt,  attr->get(attr->data));
> 					   ^^ (unsigned long long)
> 
> > +DEFINE_SIMPLE_ATTRIBUTE(fops_u8, debugfs_u8_get, debugfs_u8_set, "%lu\n");
> > +DEFINE_SIMPLE_ATTRIBUTE(fops_u16, debugfs_u16_get, debugfs_u16_set, "%lu\n");
> > +DEFINE_SIMPLE_ATTRIBUTE(fops_u32, debugfs_u32_get, debugfs_u32_set, "%lu\n");
>                                                                  %llu   ^^^^ 
> 
> I also noticed that it is not possible to pass NULL operations to
> DEFINE_SIMPLE_ATTRIBUTE() unless you change
> 
> --- a/include/linux/fs.h	2005-05-19 10:17:53.000000000 +0200
> +++ b/include/linux/fs.h	2005-05-19 10:14:57.000000000 +0200
> @@ -1680,7 +1680,7 @@
>  static int __fops ## _open(struct inode *inode, struct file *file)	\
>  {									\
>  	__simple_attr_check_format(__fmt, 0ul);				\
> -	return simple_attr_open(inode, file, &__get, &__set, __fmt);	\
> +	return simple_attr_open(inode, file, __get, __set, __fmt);	\
>  }									\
>  static struct file_operations __fops = {				\
>  	.owner	 = THIS_MODULE,						\
> 
> I'm currently away from my test machine, so I think it's easier if you
> just update your patch yourself, but I could also send you an update
> patch later if you prefer.

Thanks for the updates, I've made them by hand to the patch, and will
show up in the next -mm release.

thanks again,

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/