| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20090407160626.1ff81b1e.akpm@linux-foundation.org>
Date: Tue, 7 Apr 2009 16:06:26 -0700
From: Andrew Morton <akpm@...ux-foundation.org>
To: Eric Paris <eparis@...hat.com>
Cc: linux-kernel@...r.kernel.org, viro@...iv.linux.org.uk,
hch@...radead.org, alan@...rguk.ukuu.org.uk, sfr@...b.auug.org.au,
john@...nmccutchan.com, rlove@...ve.org
Subject: Re: [PATCH -V2 07/13] fsnotify: generic notification queue and
waitq
On Fri, 27 Mar 2009 16:05:43 -0400
Eric Paris <eparis@...hat.com> wrote:
> inotify needs to do asyc notification in which event information is stored
> on a queue until the listener is ready to receive it. This patch
> implements a generic notification queue for inotify (and later fanotify) to
> store events to be sent at a later time.
>
>
> ...
>
> +/* return 1 if something is available, return 0 otherwise */
> +int fsnotify_check_notif_queue(struct fsnotify_group *group)
> +{
> + BUG_ON(!mutex_is_locked(&group->notification_mutex));
> + return !list_empty(&group->notification_list);
> +}
It's a poorly named function, because the name ("check") doesn't convey
information about the return value.
Better would be
bool fsnotify_notif_queue_nonempty(...)
or
bool fsnotify_notif_queue_empty(...)
(and invert test in callers)
and the abbreviation of "notify" to "notif" just makes it harder to
remember its name.
best is
bool fsnotify_notify_queue_is_empty(...)
> void fsnotify_get_event(struct fsnotify_event *event)
> {
> @@ -45,26 +60,180 @@ void fsnotify_put_event(struct fsnotify_event *event)
> return;
>
> if (atomic_dec_and_test(&event->refcnt)) {
> - if (event->data_type == FSNOTIFY_EVENT_PATH) {
> + if (event->data_type == FSNOTIFY_EVENT_PATH)
> path_put(&event->path);
> - event->path.dentry = NULL;
> - event->path.mnt = NULL;
> - }
> + kmem_cache_free(event_kmem_cache, event);
> + }
> +}
>
> - event->mask = 0;
> +struct fsnotify_event_holder *alloc_event_holder(void)
> +{
> + return kmem_cache_alloc(event_holder_kmem_cache, GFP_KERNEL);
> +}
That's a pretty generic-sounding name for a global symbol.
> - kmem_cache_free(event_kmem_cache, event);
> +void fsnotify_destroy_event_holder(struct fsnotify_event_holder *holder)
> +{
> + kmem_cache_free(event_holder_kmem_cache, holder);
> +}
That one's better.
> +/*
> + * check if 2 events contain the same information.
> + */
> +static inline int event_compare(struct fsnotify_event *old, struct fsnotify_event *new)
> +{
> + if ((old->mask == new->mask) &&
> + (old->to_tell == new->to_tell) &&
> + (old->data_type == new->data_type)) {
> + switch (old->data_type) {
> + case (FSNOTIFY_EVENT_INODE):
> + if (old->inode == new->inode)
> + return 1;
> + break;
> + case (FSNOTIFY_EVENT_PATH):
> + if ((old->path.mnt == new->path.mnt) &&
> + (old->path.dentry == new->path.dentry))
> + return 1;
> + case (FSNOTIFY_EVENT_NONE):
> + return 1;
> + };
> }
> + return 0;
> }
The compiler would have inlined this.
> -struct fsnotify_event *fsnotify_create_event(struct inode *to_tell, __u32 mask, void *data, int data_type)
> +/*
> + * Add an event to the group notification queue. The group can later pull this
> + * event off the queue to deal with.
> + */
> +int fsnotify_add_notif_event(struct fsnotify_group *group, struct fsnotify_event *event)
s/notif/notify/
> +{
> + struct fsnotify_event_holder *holder = NULL;
> + struct list_head *list = &group->notification_list;
> + struct fsnotify_event_holder *last_holder;
> + struct fsnotify_event *last_event;
> +
> + /*
> + * Check if we expect to be able to use the in event holder. If not alloc
> + * a new holder.
> + * For the overflow event it's possible that something will use the in
> + * event holder before we get the lock so we may need to jump back and
> + * alloc a new holder.
> + */
The term "in event" is unclear to this reader.
> + if (!list_empty(&event->holder.event_list)) {
> +alloc_holder:
> + holder = alloc_event_holder();
> + if (!holder)
> + return -ENOMEM;
> + }
> +
> + mutex_lock(&group->notification_mutex);
> +
> + if (group->q_len >= group->max_events)
> + event = &q_overflow_event;
> +
> + spin_lock(&event->lock);
> +
> + if (list_empty(&event->holder.event_list)) {
> + if (unlikely(holder))
> + fsnotify_destroy_event_holder(holder);
> + holder = &event->holder;
> + } else if (unlikely(!holder)) {
> + /* between the time we checked above and got the lock the in
> + * event holder was used, go back and get a new one */
> + spin_unlock(&event->lock);
> + mutex_unlock(&group->notification_mutex);
> + goto alloc_holder;
> + }
> +
> + if (!list_empty(list)) {
> + last_holder = list_entry(list->prev, struct fsnotify_event_holder, event_list);
> + last_event = last_holder->event;
> + if (event_compare(last_event, event)) {
> + spin_unlock(&event->lock);
> + mutex_unlock(&group->notification_mutex);
> + if (holder != &event->holder)
> + fsnotify_destroy_event_holder(holder);
> + return 0;
> + }
> + }
> +
> + group->q_len++;
> + holder->event = event;
> +
> + fsnotify_get_event(event);
> + list_add_tail(&holder->event_list, list);
> + spin_unlock(&event->lock);
> + mutex_unlock(&group->notification_mutex);
> +
> + wake_up(&group->notification_waitq);
> + return 0;
> +}
> +
>
> ...
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists