Re: [PATCH] libfs: add simple attribute files

[Arnd Bergmann <arnd@arndb.de>]

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.

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