Re: [RFC][PATCH] fswatch + audit + inotify status patch
Hi Tim,
Thanks for the update.
On Wed, Jul 20, 2005 at 04:56:39PM -0500, Timothy R. Chavez wrote:
Here's an update with a quick blurb in the form of a patch to
just
peruse and perhaps comment on to keep things going...
At first look, this patch seems to involve a non-trivial set of data
structures. I see watch, watch_inode_data, audit_watch,
audit_watch_info, audit_aux_data_watched, etc. Perhaps you could note
the purpose of each of these and explain how they are linked together.
It would also be helpful if you could summarize the key decisions made
in the data structure design. For instance, what are your locking
strategies for the fswatch and auditfs code? How have they changed
since your last patch?
How many watches can we expect in a given list? I see several
situations where you're walking a list and doing a string comparison
on each element. Looks like a potential performance issue.
Currently the framework is trying to support both statically
allocated, embedded watches and dynamically allocated stand-alone
watches (though I've not really tested the dynamically allocated
aspect of the system yet). I'm thinking about ditching the
dynamically allocated watches... but perhaps this feature makes the
framework more robust or at the very least, appealing?
I think supporting both creates an unnecessarily complicated
interface, which is the opposite of robust and appealing. Do you
think there is a good reason for supporting both?
+void auditfs_attach_wdata(struct watch *w, const void *name,
+ struct inode *inode, uint action)
+{
+ int disabled;
+ struct audit_context *ctx = current->audit_context;
+ struct audit_aux_data_watched *ax;
+ struct audit_watch_info *this, *winfo;
+ struct hlist_node *pos, *tmp;
+
+ printk("BEEP!\n");
+
+ if (!ctx || (w->w_user != FSW_USER_AUDIT))
+ return;
We shouldn't have to check w_user. The fswatch_notify_users() code
should be checking that and should not call us if this wasn't our
watch.
Then there's the necessity to change dnotify and inotify to use
this new
framework which I would be interested in recruiting help for.
I must admit I'm concerned about the current approach, especially now
that inotify is in the kernel. I think we should be basing the
fswatch work off of the inotify code, instead of introducing something
new that inotify has to adapt to use.
I anticipate some work around making it so this system interfaces
nicely with Inotify without Inotify having to make any drastic (or
more drastic) changes then auditfs has made.
If you don't want to make drastic changes to inotify, it's really best
to start there.
If I could suggest a different approach:
1. Move filesystem watching code from inotify.c to fswatch.c, so
it can be used by other components.
2. Add any must-have features needed by auditfs.
3. Add auditfs patch.
4. Propose any wanted-but-not-needed features.
This would give us an incremental set of patches that could be
independently developed, versus one giant patch. It would make the
code easier to review, and would be more appropriate to send to LKML.
It would also provide more stability to our testing efforts, as there
would be a running kernel available more quickly, with fewer changes
per-patch. This way we could focus on testing functionality subsets.
Thanks for the effort you've put into writing this patch so far. I
hope you'll consider my suggestions above.
I'd love to see more design discussions on this list as well.
Regards,
Amy