On Thu, Sep 8, 2016 at 4:25 PM, Jan Kara <jack@xxxxxxx> wrote:
> Currently there are three possible returns from fsnotify_add_event(). We
> will be adding a fourth one. Let's change magic numbers to enum to make
> things clearer.
This cleanup should go last, otherwise it'll just make backporting the
fix harder.
Also I don't really see the value of having a big enum with 3 of the
values having the same meaning. Yeah, there's that BUG_ON for having
merged a permission event. But it checks the very obvious condition
at the top of fanotify_merge() with the big comment, not very
interesting.
So I'd kill the BUG_ON, drop the merged/overflowed/shutdown
distinction and return a two way value (doing it with an enum still
makes the code more readable, though).
Thanks,
Miklos
>
> Signed-off-by: Jan Kara <jack@xxxxxxx>
> ---
> fs/notify/fanotify/fanotify.c | 7 ++++---
> fs/notify/inotify/inotify_fsnotify.c | 4 ++--
> fs/notify/notification.c | 20 +++++++++++---------
> include/linux/fsnotify_backend.h | 14 ++++++++++----
> 4 files changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index d2f97ecca6a5..3c6c81e0c2c8 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -192,6 +192,7 @@ static int fanotify_handle_event(struct fsnotify_group *group,
> const unsigned char *file_name, u32 cookie)
> {
> int ret = 0;
> + enum fsn_add_event_ret ae_ret;
> struct fanotify_event_info *event;
> struct fsnotify_event *fsn_event;
>
> @@ -218,10 +219,10 @@ static int fanotify_handle_event(struct fsnotify_group *group,
> return -ENOMEM;
>
> fsn_event = &event->fse;
> - ret = fsnotify_add_event(group, fsn_event, fanotify_merge);
> - if (ret) {
> + ae_ret = fsnotify_add_event(group, fsn_event, fanotify_merge);
> + if (ae_ret != AE_INSERTED) {
> /* Permission events shouldn't be merged */
> - BUG_ON(ret == 1 && mask & FAN_ALL_PERM_EVENTS);
> + BUG_ON(ae_ret == AE_MERGED && mask & FAN_ALL_PERM_EVENTS);
> /* Our event wasn't used in the end. Free it. */
> fsnotify_destroy_event(group, fsn_event);
>
> diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
> index 2cd900c2c737..ac37192c59c5 100644
> --- a/fs/notify/inotify/inotify_fsnotify.c
> +++ b/fs/notify/inotify/inotify_fsnotify.c
> @@ -72,7 +72,7 @@ int inotify_handle_event(struct fsnotify_group *group,
> struct inotify_inode_mark *i_mark;
> struct inotify_event_info *event;
> struct fsnotify_event *fsn_event;
> - int ret;
> + enum fsn_add_event_ret ret;
> int len = 0;
> int alloc_len = sizeof(struct inotify_event_info);
>
> @@ -109,7 +109,7 @@ int inotify_handle_event(struct fsnotify_group *group,
> strcpy(event->name, file_name);
>
> ret = fsnotify_add_event(group, fsn_event, inotify_merge);
> - if (ret) {
> + if (ret != AE_INSERTED) {
> /* Our event wasn't used in the end. Free it. */
> fsnotify_destroy_event(group, fsn_event);
> }
> diff --git a/fs/notify/notification.c b/fs/notify/notification.c
> index a95d8e037aeb..12bfd6790fc4 100644
> --- a/fs/notify/notification.c
> +++ b/fs/notify/notification.c
> @@ -84,12 +84,12 @@ void fsnotify_destroy_event(struct fsnotify_group *group,
> * added to the queue, 1 if the event was merged with some other queued event,
> * 2 if the queue of events has overflown.
> */
> -int fsnotify_add_event(struct fsnotify_group *group,
> - struct fsnotify_event *event,
> - int (*merge)(struct list_head *,
> - struct fsnotify_event *))
> +enum fsn_add_event_ret fsnotify_add_event(
> + struct fsnotify_group *group,
> + struct fsnotify_event *event,
> + int (*merge)(struct list_head *, struct fsnotify_event *))
> {
> - int ret = 0;
> + enum fsn_add_event_ret ret = AE_INSERTED;
> struct list_head *list = &group->notification_list;
>
> pr_debug("%s: group=%p event=%p\n", __func__, group, event);
> @@ -97,7 +97,7 @@ int fsnotify_add_event(struct fsnotify_group *group,
> mutex_lock(&group->notification_mutex);
>
> if (group->q_len >= group->max_events) {
> - ret = 2;
> + ret = AE_OVERFLOW;
> /* Queue overflow event only if it isn't already queued */
> if (!list_empty(&group->overflow_event->list)) {
> mutex_unlock(&group->notification_mutex);
> @@ -108,10 +108,12 @@ int fsnotify_add_event(struct fsnotify_group *group,
> }
>
> if (!list_empty(list) && merge) {
> - ret = merge(list, event);
> - if (ret) {
> + int merge_ret;
> +
> + merge_ret = merge(list, event);
> + if (merge_ret) {
> mutex_unlock(&group->notification_mutex);
> - return ret;
> + return AE_MERGED;
> }
> }
>
> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index 58205f33af02..b948a52ce65f 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -299,11 +299,17 @@ extern int fsnotify_fasync(int fd, struct file *file, int on);
> /* Free event from memory */
> extern void fsnotify_destroy_event(struct fsnotify_group *group,
> struct fsnotify_event *event);
> +/* Return values of fsnotify_add_event() */
> +enum fsn_add_event_ret {
> + AE_INSERTED, /* Event was added in the queue */
> + AE_MERGED, /* Event was merged with another event, passed event unused */
> + AE_OVERFLOW, /* Queue overflow, passed event unused */
> +};
> /* attach the event to the group notification queue */
> -extern int fsnotify_add_event(struct fsnotify_group *group,
> - struct fsnotify_event *event,
> - int (*merge)(struct list_head *,
> - struct fsnotify_event *));
> +extern enum fsn_add_event_ret fsnotify_add_event(
> + struct fsnotify_group *group,
> + struct fsnotify_event *event,
> + int (*merge)(struct list_head *, struct fsnotify_event *));
> /* Remove passed event from groups notification queue */
> extern void fsnotify_remove_event(struct fsnotify_group *group, struct fsnotify_event *event);
> /* true if the group notification queue is empty */
> --
> 2.6.6
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
