| Message ID | 1473344745-20634-2-git-send-email-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 8, 2016 at 4:25 PM, Jan Kara <jack@suse.cz> 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@suse.cz> > --- > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri 09-09-16 15:01:46, Miklos Szeredi wrote: > On Thu, Sep 8, 2016 at 4:25 PM, Jan Kara <jack@suse.cz> 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). Fair enough, I'll fix this up and resend the series (likely early next week). Honza
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 */
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. Signed-off-by: Jan Kara <jack@suse.cz> --- 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(-)