Messages in this thread Patch in this message | | | From | Arnd Bergmann <> | Subject | Re: [PATCH] libfs: add simple attribute files | Date | Thu, 19 May 2005 10:29:06 +0200 |
| |
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/
| |