Hey, John.
On top of the patch I just posted (see "[patch] add class_device to
miscdevice") this patch uses the newly attainable class_device value to
correctly add our sysfs attributes.
The problem with the current solution is that we were adding attributes
to the device. But we did not have a device entry, only the fake
inotify entry in class/misc. So we need to attach the attributes to the
class device and not the nonexistent physical device.
In order to attach attributes to the class_device, I needed to make the
changes I sent to Greg. I would suggest merging that patch and this and
hopefully the miscdevice bits will find their way into mainline and we
can then drop them from inotify.
Thanks,
Robert Love
Make our /sys/class/misc/inotify entries show up and work.
Signed-Off-By: Robert Love <[email protected]>
inotify.c | 57 +++++++++++++++++++++++----------------------------------
1 files changed, 23 insertions(+), 34 deletions(-)
diff -u linux/drivers/char/inotify.c linux/drivers/char/inotify.c
--- linux/drivers/char/inotify.c 2004-11-15 15:28:34.951248696 -0500
+++ linux/drivers/char/inotify.c 2004-11-16 14:42:11.929575168 -0500
@@ -80,60 +80,48 @@
#define inotify_watch_i_list(pos) list_entry((pos), struct inotify_watch, i_list)
#define inotify_watch_u_list(pos) list_entry((pos), struct inotify_watch, u_list)
-static ssize_t show_max_queued_events(struct device *dev, char *buf)
+static ssize_t show_max_queued_events(struct class_device *class, char *buf)
{
sprintf(buf, "%d", sysfs_attrib_max_queued_events);
-
return strlen(buf) + 1;
}
-static ssize_t store_max_queued_events(struct device *dev, const char *buf,
- size_t count)
+static ssize_t store_max_queued_events(struct class_device *class,
+ const char *buf, size_t count)
{
return 0;
}
-static ssize_t show_max_user_devices(struct device *dev, char *buf)
+static ssize_t show_max_user_devices(struct class_device *class, char *buf)
{
sprintf(buf, "%d", sysfs_attrib_max_user_devices);
-
return strlen(buf) + 1;
}
-static ssize_t store_max_user_devices(struct device *dev, const char *buf,
- size_t count)
+static ssize_t store_max_user_devices(struct class_device *class,
+ const char *buf, size_t count)
{
return 0;
}
-static ssize_t show_max_user_watches(struct device *dev, char *buf)
+static ssize_t show_max_user_watches(struct class_device *class, char *buf)
{
sprintf(buf, "%d", sysfs_attrib_max_user_watches);
-
return strlen(buf) + 1;
}
-static ssize_t store_max_user_watches(struct device *dev, const char *buf,
- size_t count)
+static ssize_t store_max_user_watches(struct class_device *class,
+ const char *buf, size_t count)
{
return 0;
}
-
-static DEVICE_ATTR(max_queued_events, S_IRUGO | S_IWUSR, show_max_queued_events,
- store_max_queued_events);
-static DEVICE_ATTR(max_user_devices, S_IRUGO | S_IWUSR, show_max_user_devices,
- store_max_user_devices);
-static DEVICE_ATTR(max_user_watches, S_IRUGO | S_IWUSR, show_max_user_watches,
- store_max_user_watches);
-
-static struct device_attribute *inotify_device_attrs[] = {
- &dev_attr_max_queued_events,
- &dev_attr_max_user_devices,
- &dev_attr_max_user_watches,
- NULL
-};
-
+static CLASS_DEVICE_ATTR(max_queued_events, S_IRUGO | S_IWUSR,
+ show_max_queued_events, store_max_queued_events);
+static CLASS_DEVICE_ATTR(max_user_devices, S_IRUGO | S_IWUSR,
+ show_max_user_devices, store_max_user_devices);
+static CLASS_DEVICE_ATTR(max_user_watches, S_IRUGO | S_IWUSR,
+ show_max_user_watches, store_max_user_watches);
/*
* A list of these is attached to each instance of the driver
@@ -307,7 +295,8 @@
{
int ret;
- if (atomic_read(&dev->user->inotify_watches) >= sysfs_attrib_max_user_watches)
+ if (atomic_read(&dev->user->inotify_watches) >=
+ sysfs_attrib_max_user_watches)
return -ENOSPC;
repeat:
@@ -968,7 +957,8 @@
static int __init inotify_init(void)
{
- int ret,i;
+ struct class_device *class;
+ int ret;
ret = misc_register(&inotify_device);
if (ret)
@@ -978,11 +968,10 @@
sysfs_attrib_max_user_devices = 64;
sysfs_attrib_max_user_watches = 16384;
- for (i = 0; inotify_device_attrs[i]; i++) {
- int res;
- res = device_create_file (inotify_device.dev, inotify_device_attrs[i]);
- printk(KERN_INFO "Inotify sysfs create file = %d\n", res);
- }
+ class = inotify_device.class;
+ class_device_create_file(class, &class_device_attr_max_queued_events);
+ class_device_create_file(class, &class_device_attr_max_user_devices);
+ class_device_create_file(class, &class_device_attr_max_user_watches);
atomic_set(&inotify_cookie, 0);
John,
In 2.6.10-rc, vfs_permission() was replaced by generic_permission(),
which also has a slightly changed behavior and argument list.
Robert Love
vfs_permission was replaced by generic_permission in 2.6.10-rc.
Signed-Off-By: Robert Love <[email protected]>
inotify.c | 2 +-
1 files changed, 1 insertion(+), 1 deletion(-)
diff -u linux/drivers/char/inotify.c linux/drivers/char/inotify.c
--- linux/drivers/char/inotify.c 2004-11-15 15:28:34.951248696 -0500
+++ linux/drivers/char/inotify.c 2004-11-16 14:42:11.929575168 -0500
@@ -163,7 +151,7 @@
inode = nd.dentry->d_inode;
/* you can only watch an inode if you have read permissions on it */
- error = vfs_permission(inode, MAY_READ);
+ error = generic_permission(inode, MAY_READ, NULL);
if (error) {
inode = ERR_PTR(error);
goto release_and_out;
On Wed, Nov 17, 2004 at 01:02:40PM -0500, Robert Love wrote:
> John,
>
> In 2.6.10-rc, vfs_permission() was replaced by generic_permission(),
> which also has a slightly changed behavior and argument list.
And using either of them was/is totally bogus. It's a helper for filesystems,
not a general API.
On Wed, 2004-11-17 at 19:18 +0000, Christoph Hellwig wrote:
> No it doesn't. Please try to understand the APIs before you're using them.
> Just looking at the callers should give you an immediate clue.
Maybe you should look at the code in question. We actually want to
perform the exact same sort of permission checks that, say, read
performs.
Robert Love
On Wed, Nov 17, 2004 at 02:10:01PM -0500, Robert Love wrote:
> On Wed, 2004-11-17 at 19:08 +0000, Christoph Hellwig wrote:
>
> > And using either of them was/is totally bogus. It's a helper for filesystems,
> > not a general API.
>
> Is there some specific reason you have a problem with its use? It works
> quite nicely for what we need.
No it doesn't. Please try to understand the APIs before you're using them.
Just looking at the callers should give you an immediate clue.
On Wed, 2004-11-17 at 19:08 +0000, Christoph Hellwig wrote:
> And using either of them was/is totally bogus. It's a helper for filesystems,
> not a general API.
Is there some specific reason you have a problem with its use? It works
quite nicely for what we need.
Robert Love
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Robert Love wrote:
> On Wed, 2004-11-17 at 19:18 +0000, Christoph Hellwig wrote:
>
>
>>No it doesn't. Please try to understand the APIs before you're using them.
>>Just looking at the callers should give you an immediate clue.
>
>
> Maybe you should look at the code in question. We actually want to
> perform the exact same sort of permission checks that, say, read
> performs.
>
> Robert Love
>
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
use permission()
- --
Mike Waychison
Sun Microsystems, Inc.
1 (650) 352-5299 voice
1 (416) 202-8336 voice
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
NOTICE: The opinions expressed in this email are held by me,
and may not represent the views of Sun Microsystems, Inc.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org
iD8DBQFBm6/hdQs4kOxk3/MRAjFPAJ9tmyglXGvlhP4aYFzbX4uAmXIZkwCgjs86
mUksZfBEDIdncxVMmutvVGA=
=ei8c
-----END PGP SIGNATURE-----
Attached patch implements the final chunk of our sysfs solution: store
support. I added basic write support with some simple checking (do not
allow zero) to the existing sysfs attributes.
I made a few other changes. I added a newline after the values in the
show function. The other sysfs attributes do this and it makes
sense--do a "cat *" in our sysfs directory before and after. I also
just return sprintf() directly instead of the strlen(). I also made
max_queued_events unsigned, since dev->max_events is unsigned. If we
don't do this we need to add checking in store_max_queued_events to
ensure that the given value is less than or equal to INT_MAX so this
seems easier and more optimal anyhow. The other values I kept at int
since that is the range of atomic_t's.
I am running it now. I can read and write the values fine. Works
great.
Robert Love
Add store support to our sysfs attributes and a few other changes.
inotify.c | 35 +++++++++++++++++++++++++----------
1 files changed, 25 insertions(+), 10 deletions(-)
diff -u linux/drivers/char/inotify.c linux/drivers/char/inotify.c
--- linux/drivers/char/inotify.c 2004-11-16 14:42:11.929575168 -0500
+++ linux/drivers/char/inotify.c 2004-11-17 12:28:27.921136656 -0500
@@ -40,7 +40,7 @@
static int sysfs_attrib_max_user_devices;
static int sysfs_attrib_max_user_watches;
-static int sysfs_attrib_max_queued_events;
+static unsigned int sysfs_attrib_max_queued_events;
/*
* struct inotify_device - represents an open instance of an inotify device
@@ -82,38 +82,53 @@
static ssize_t show_max_queued_events(struct class_device *class, char *buf)
{
- sprintf(buf, "%d", sysfs_attrib_max_queued_events);
- return strlen(buf) + 1;
+ return sprintf(buf, "%d\n", sysfs_attrib_max_queued_events);
}
static ssize_t store_max_queued_events(struct class_device *class,
const char *buf, size_t count)
{
- return 0;
+ unsigned int max;
+
+ if (sscanf(buf, "%u", &max) > 0 && max > 0) {
+ sysfs_attrib_max_queued_events = max;
+ return strlen(buf);
+ }
+ return -EINVAL;
}
static ssize_t show_max_user_devices(struct class_device *class, char *buf)
{
- sprintf(buf, "%d", sysfs_attrib_max_user_devices);
- return strlen(buf) + 1;
+ return sprintf(buf, "%d\n", sysfs_attrib_max_user_devices);
}
static ssize_t store_max_user_devices(struct class_device *class,
const char *buf, size_t count)
{
- return 0;
+ int max;
+
+ if (sscanf(buf, "%d", &max) > 0 && max > 0) {
+ sysfs_attrib_max_user_devices = max;
+ return strlen(buf);
+ }
+ return -EINVAL;
}
static ssize_t show_max_user_watches(struct class_device *class, char *buf)
{
- sprintf(buf, "%d", sysfs_attrib_max_user_watches);
- return strlen(buf) + 1;
+ return sprintf(buf, "%d\n", sysfs_attrib_max_user_watches);
}
static ssize_t store_max_user_watches(struct class_device *class,
const char *buf, size_t count)
{
- return 0;
+ int max;
+
+ if (sscanf(buf, "%d", &max) > 0 && max > 0) {
+ sysfs_attrib_max_user_watches = max;
+ return strlen(buf);
+ }
+ return -EINVAL;
}
static CLASS_DEVICE_ATTR(max_queued_events, S_IRUGO | S_IWUSR,
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Robert Love wrote:
> On Wed, 2004-11-17 at 15:09 -0500, Mike Waychison wrote:
>
>
>>use permission()
>
>
> I appreciate the constructive comment.
>
> John, attached patch replaces vfs_permission() with permission().
>
> Thanks, Mike.
>
> Robert Love
>
>
> Use permission() instead of generic_permission().
>
> Signed-Off-By: Robert Love <[email protected]>
>
> diff -u linux/drivers/char/inotify.c linux/drivers/char/inotify.c
> --- linux/drivers/char/inotify.c 2004-11-17 12:28:27.921136656 -0500
> +++ linux/drivers/char/inotify.c 2004-11-17 12:28:27.921136656 -0500
> @@ -166,7 +166,7 @@
> inode = nd.dentry->d_inode;
>
> /* you can only watch an inode if you have read permissions on it */
> - error = generic_permission(inode, MAY_READ, NULL);
> + error = permission(inode, MAY_READ);
> if (error) {
> inode = ERR_PTR(error);
> goto release_and_out;
>
>
permission() still takes 3 arguments though. I think it is safe to pass
NULL for the nameidata.
- --
Mike Waychison
Sun Microsystems, Inc.
1 (650) 352-5299 voice
1 (416) 202-8336 voice
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
NOTICE: The opinions expressed in this email are held by me,
and may not represent the views of Sun Microsystems, Inc.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org
iD8DBQFBm7O2dQs4kOxk3/MRApYgAJ0c1XGD99i8Yir2LYTAtosJrOtvIACfT5BE
C3T8DDq+L9NskVe1sI47IKM=
=BZA8
-----END PGP SIGNATURE-----
On Wed, 2004-11-17 at 15:25 -0500, Mike Waychison wrote:
> permission() still takes 3 arguments though. I think it is safe to pass
> NULL for the nameidata.
Ugh, I compile-tested the wrong tree. Thanks.
It is safe to pass NULL.
Patch below.
Robert Love
Use permission() instead of generic_permission().
Signed-Off-By: Robert Love <[email protected]>
diff -u linux/drivers/char/inotify.c linux/drivers/char/inotify.c
--- linux/drivers/char/inotify.c 2004-11-17 12:28:27.921136656 -0500
+++ linux/drivers/char/inotify.c 2004-11-17 12:28:27.921136656 -0500
@@ -166,7 +166,7 @@
inode = nd.dentry->d_inode;
/* you can only watch an inode if you have read permissions on it */
- error = vfs_permission(inode, MAY_READ);
+ error = permission(inode, MAY_READ, NULL);
if (error) {
inode = ERR_PTR(error);
goto release_and_out;
On Wed, 2004-11-17 at 15:09 -0500, Mike Waychison wrote:
> use permission()
I appreciate the constructive comment.
John, attached patch replaces vfs_permission() with permission().
Thanks, Mike.
Robert Love
Use permission() instead of generic_permission().
Signed-Off-By: Robert Love <[email protected]>
diff -u linux/drivers/char/inotify.c linux/drivers/char/inotify.c
--- linux/drivers/char/inotify.c 2004-11-17 12:28:27.921136656 -0500
+++ linux/drivers/char/inotify.c 2004-11-17 12:28:27.921136656 -0500
@@ -166,7 +166,7 @@
inode = nd.dentry->d_inode;
/* you can only watch an inode if you have read permissions on it */
- error = generic_permission(inode, MAY_READ, NULL);
+ error = permission(inode, MAY_READ);
if (error) {
inode = ERR_PTR(error);
goto release_and_out;
Awesome work! But I am going to hold off applying this, until the
mainline kernel gets the misc device changes.
John
On Wed, 2004-11-17 at 15:10 -0500, Robert Love wrote:
> Attached patch implements the final chunk of our sysfs solution: store
> support. I added basic write support with some simple checking (do not
> allow zero) to the existing sysfs attributes.
>
> I made a few other changes. I added a newline after the values in the
> show function. The other sysfs attributes do this and it makes
> sense--do a "cat *" in our sysfs directory before and after. I also
> just return sprintf() directly instead of the strlen(). I also made
> max_queued_events unsigned, since dev->max_events is unsigned. If we
> don't do this we need to add checking in store_max_queued_events to
> ensure that the given value is less than or equal to INT_MAX so this
> seems easier and more optimal anyhow. The other values I kept at int
> since that is the range of atomic_t's.
>
> I am running it now. I can read and write the values fine. Works
> great.
>
> Robert Love
>
>
> Add store support to our sysfs attributes and a few other changes.
>
> inotify.c | 35 +++++++++++++++++++++++++----------
> 1 files changed, 25 insertions(+), 10 deletions(-)
>
> diff -u linux/drivers/char/inotify.c linux/drivers/char/inotify.c
> --- linux/drivers/char/inotify.c 2004-11-16 14:42:11.929575168 -0500
> +++ linux/drivers/char/inotify.c 2004-11-17 12:28:27.921136656 -0500
> @@ -40,7 +40,7 @@
>
> static int sysfs_attrib_max_user_devices;
> static int sysfs_attrib_max_user_watches;
> -static int sysfs_attrib_max_queued_events;
> +static unsigned int sysfs_attrib_max_queued_events;
>
> /*
> * struct inotify_device - represents an open instance of an inotify device
> @@ -82,38 +82,53 @@
>
> static ssize_t show_max_queued_events(struct class_device *class, char *buf)
> {
> - sprintf(buf, "%d", sysfs_attrib_max_queued_events);
> - return strlen(buf) + 1;
> + return sprintf(buf, "%d\n", sysfs_attrib_max_queued_events);
> }
>
> static ssize_t store_max_queued_events(struct class_device *class,
> const char *buf, size_t count)
> {
> - return 0;
> + unsigned int max;
> +
> + if (sscanf(buf, "%u", &max) > 0 && max > 0) {
> + sysfs_attrib_max_queued_events = max;
> + return strlen(buf);
> + }
> + return -EINVAL;
> }
>
> static ssize_t show_max_user_devices(struct class_device *class, char *buf)
> {
> - sprintf(buf, "%d", sysfs_attrib_max_user_devices);
> - return strlen(buf) + 1;
> + return sprintf(buf, "%d\n", sysfs_attrib_max_user_devices);
> }
>
> static ssize_t store_max_user_devices(struct class_device *class,
> const char *buf, size_t count)
> {
> - return 0;
> + int max;
> +
> + if (sscanf(buf, "%d", &max) > 0 && max > 0) {
> + sysfs_attrib_max_user_devices = max;
> + return strlen(buf);
> + }
> + return -EINVAL;
> }
>
> static ssize_t show_max_user_watches(struct class_device *class, char *buf)
> {
> - sprintf(buf, "%d", sysfs_attrib_max_user_watches);
> - return strlen(buf) + 1;
> + return sprintf(buf, "%d\n", sysfs_attrib_max_user_watches);
> }
>
> static ssize_t store_max_user_watches(struct class_device *class,
> const char *buf, size_t count)
> {
> - return 0;
> + int max;
> +
> + if (sscanf(buf, "%d", &max) > 0 && max > 0) {
> + sysfs_attrib_max_user_watches = max;
> + return strlen(buf);
> + }
> + return -EINVAL;
> }
>
> static CLASS_DEVICE_ATTR(max_queued_events, S_IRUGO | S_IWUSR,
>
>
--
John McCutchan <[email protected]>
On Wed, 2004-11-17 at 20:00 -0500, John McCutchan wrote:
> Awesome work!
Thanks.
> But I am going to hold off applying this, until the
> mainline kernel gets the misc device changes.
OK, but it is going to be awhile. It is too late for 2.6.10, so it
won't go in until 2.6.11.
Robert Love
This looks good. Thanks.
On Wed, 2004-11-17 at 15:28 -0500, Robert Love wrote:
> On Wed, 2004-11-17 at 15:25 -0500, Mike Waychison wrote:
>
> > permission() still takes 3 arguments though. I think it is safe to pass
> > NULL for the nameidata.
>
> Ugh, I compile-tested the wrong tree. Thanks.
>
> It is safe to pass NULL.
>
> Patch below.
>
> Robert Love
>
>
> Use permission() instead of generic_permission().
>
> Signed-Off-By: Robert Love <[email protected]>
>
> diff -u linux/drivers/char/inotify.c linux/drivers/char/inotify.c
> --- linux/drivers/char/inotify.c 2004-11-17 12:28:27.921136656 -0500
> +++ linux/drivers/char/inotify.c 2004-11-17 12:28:27.921136656 -0500
> @@ -166,7 +166,7 @@
> inode = nd.dentry->d_inode;
>
> /* you can only watch an inode if you have read permissions on it */
> - error = vfs_permission(inode, MAY_READ);
> + error = permission(inode, MAY_READ, NULL);
> if (error) {
> inode = ERR_PTR(error);
> goto release_and_out;
>
>
--
John McCutchan <[email protected]>
On Wed, Nov 17, 2004 at 02:17:32PM -0500, Robert Love wrote:
> On Wed, 2004-11-17 at 19:18 +0000, Christoph Hellwig wrote:
>
> > No it doesn't. Please try to understand the APIs before you're using them.
> > Just looking at the callers should give you an immediate clue.
>
> Maybe you should look at the code in question. We actually want to
> perform the exact same sort of permission checks that, say, read
> performs.
Maybe you should look at the code in kernel - e.g. aforementioned sys_read().
Or sys_open().
The only fs-independent code that has any business calling that puppy is
permission(9).
John,
Ah, another part of my locking rewrite I remember and should pull out
and send now.
We need to grab inode_lock before calling __iget().
Robert Love
need to hold inode_lock around __iget()
drivers/char/inotify.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletion(-)
diff -u linux/drivers/char/inotify.c linux/drivers/char/inotify.c
--- linux/drivers/char/inotify.c 2004-11-18 12:31:14.294242616 -0500
+++ linux/drivers/char/inotify.c 2004-11-18 13:59:33.400655992 -0500
@@ -171,7 +171,9 @@
goto release_and_out;
}
+ spin_lock(&inode_lock);
__iget(inode);
+ spin_unlock(&inode_lock);
release_and_out:
path_release(&nd);
out:
@@ -790,7 +792,7 @@
*/
static void inotify_release_all_watches(struct inotify_device *dev)
{
- struct inotify_watch *watch,*next;
+ struct inotify_watch *watch, *next;
list_for_each_entry_safe(watch, next, &dev->watches, d_list)
ignore_helper(watch, 0);