diff mbox series

[v2,02/16] fsnotify: factor helpers fsnotify_dentry() and fsnotify_file()

Message ID 20200217131455.31107-3-amir73il@gmail.com (mailing list archive)
State New, archived
Headers show
Series Fanotify event with name info | expand

Commit Message

Amir Goldstein Feb. 17, 2020, 1:14 p.m. UTC 
Most of the code in fsnotify hooks is boiler plate of one or the other.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 include/linux/fsnotify.h | 96 +++++++++++++++-------------------------
 1 file changed, 36 insertions(+), 60 deletions(-)

Comments

Jan Kara Feb. 25, 2020, 1:46 p.m. UTC | #1 
On Mon 17-02-20 15:14:41, Amir Goldstein wrote:
> Most of the code in fsnotify hooks is boiler plate of one or the other.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  include/linux/fsnotify.h | 96 +++++++++++++++-------------------------
>  1 file changed, 36 insertions(+), 60 deletions(-)

Nice cleanup. Just two comments below.

> @@ -58,8 +78,6 @@ static inline int fsnotify_path(struct inode *inode, const struct path *path,
>  static inline int fsnotify_perm(struct file *file, int mask)
>  {
>  	int ret;
> -	const struct path *path = &file->f_path;
> -	struct inode *inode = file_inode(file);
>  	__u32 fsnotify_mask = 0;
>  
>  	if (file->f_mode & FMODE_NONOTIFY)

I guess you can drop the NONOTIFY check from here. You've moved it to
fsnotify_file() and there's not much done in this function to be worth
skipping...

> @@ -70,7 +88,7 @@ static inline int fsnotify_perm(struct file *file, int mask)
>  		fsnotify_mask = FS_OPEN_PERM;
>  
>  		if (file->f_flags & __FMODE_EXEC) {
> -			ret = fsnotify_path(inode, path, FS_OPEN_EXEC_PERM);
> +			ret = fsnotify_file(file, FS_OPEN_EXEC_PERM);
>  
>  			if (ret)
>  				return ret;

Hum, I think we could simplify fsnotify_perm() even further by having:

	if (mask & MAY_OPEN) {
		if (file->f_flags & __FMODE_EXEC)
			fsnotify_mask = FS_OPEN_EXEC_PERM;
		else
			fsnotify_mask = FS_OPEN_PERM;
	} ...

Otherwise the patch looks good to me.

								Honza
Amir Goldstein Feb. 25, 2020, 2:27 p.m. UTC | #2 
On Tue, Feb 25, 2020 at 3:46 PM Jan Kara <jack@suse.cz> wrote:
>
> On Mon 17-02-20 15:14:41, Amir Goldstein wrote:
> > Most of the code in fsnotify hooks is boiler plate of one or the other.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  include/linux/fsnotify.h | 96 +++++++++++++++-------------------------
> >  1 file changed, 36 insertions(+), 60 deletions(-)
>
> Nice cleanup. Just two comments below.
>
> > @@ -58,8 +78,6 @@ static inline int fsnotify_path(struct inode *inode, const struct path *path,
> >  static inline int fsnotify_perm(struct file *file, int mask)
> >  {
> >       int ret;
> > -     const struct path *path = &file->f_path;
> > -     struct inode *inode = file_inode(file);
> >       __u32 fsnotify_mask = 0;
> >
> >       if (file->f_mode & FMODE_NONOTIFY)
>
> I guess you can drop the NONOTIFY check from here. You've moved it to
> fsnotify_file() and there's not much done in this function to be worth
> skipping...

True.

>
> > @@ -70,7 +88,7 @@ static inline int fsnotify_perm(struct file *file, int mask)
> >               fsnotify_mask = FS_OPEN_PERM;
> >
> >               if (file->f_flags & __FMODE_EXEC) {
> > -                     ret = fsnotify_path(inode, path, FS_OPEN_EXEC_PERM);
> > +                     ret = fsnotify_file(file, FS_OPEN_EXEC_PERM);
> >
> >                       if (ret)
> >                               return ret;
>
> Hum, I think we could simplify fsnotify_perm() even further by having:
>
>         if (mask & MAY_OPEN) {
>                 if (file->f_flags & __FMODE_EXEC)
>                         fsnotify_mask = FS_OPEN_EXEC_PERM;
>                 else
>                         fsnotify_mask = FS_OPEN_PERM;
>         } ...
>

But the current code sends both FS_OPEN_EXEC_PERM and FS_OPEN_PERM
on an open for exec. I believe that is what was discussed when Matthew wrote
the OPEN_EXEC patches, so existing receivers of OPEN_PERM event on exec
will not regress..

Thanks,
Amir.
Jan Kara Feb. 26, 2020, 1:59 p.m. UTC | #3 
On Tue 25-02-20 16:27:02, Amir Goldstein wrote:
> On Tue, Feb 25, 2020 at 3:46 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Mon 17-02-20 15:14:41, Amir Goldstein wrote:
> > > Most of the code in fsnotify hooks is boiler plate of one or the other.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > ---
> > >  include/linux/fsnotify.h | 96 +++++++++++++++-------------------------
> > >  1 file changed, 36 insertions(+), 60 deletions(-)
> >
> > Nice cleanup. Just two comments below.
> >
> > > @@ -58,8 +78,6 @@ static inline int fsnotify_path(struct inode *inode, const struct path *path,
> > >  static inline int fsnotify_perm(struct file *file, int mask)
> > >  {
> > >       int ret;
> > > -     const struct path *path = &file->f_path;
> > > -     struct inode *inode = file_inode(file);
> > >       __u32 fsnotify_mask = 0;
> > >
> > >       if (file->f_mode & FMODE_NONOTIFY)
> >
> > I guess you can drop the NONOTIFY check from here. You've moved it to
> > fsnotify_file() and there's not much done in this function to be worth
> > skipping...
> 
> True.
> 
> >
> > > @@ -70,7 +88,7 @@ static inline int fsnotify_perm(struct file *file, int mask)
> > >               fsnotify_mask = FS_OPEN_PERM;
> > >
> > >               if (file->f_flags & __FMODE_EXEC) {
> > > -                     ret = fsnotify_path(inode, path, FS_OPEN_EXEC_PERM);
> > > +                     ret = fsnotify_file(file, FS_OPEN_EXEC_PERM);
> > >
> > >                       if (ret)
> > >                               return ret;
> >
> > Hum, I think we could simplify fsnotify_perm() even further by having:
> >
> >         if (mask & MAY_OPEN) {
> >                 if (file->f_flags & __FMODE_EXEC)
> >                         fsnotify_mask = FS_OPEN_EXEC_PERM;
> >                 else
> >                         fsnotify_mask = FS_OPEN_PERM;
> >         } ...
> >
> 
> But the current code sends both FS_OPEN_EXEC_PERM and FS_OPEN_PERM
> on an open for exec. I believe that is what was discussed when Matthew wrote
> the OPEN_EXEC patches, so existing receivers of OPEN_PERM event on exec
> will not regress..

Ah, my bad. You're right.

								Honza
diff mbox series

Patch

diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index a2d5d175d3c1..a87d4ab98da7 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -41,16 +41,36 @@  static inline int fsnotify_parent(const struct path *path,
 }
 
 /*
- * Simple wrapper to consolidate calls fsnotify_parent()/fsnotify() when
- * an event is on a path.
+ * Simple wrappers to consolidate calls fsnotify_parent()/fsnotify() when
+ * an event is on a file/dentry.
  */
-static inline int fsnotify_path(struct inode *inode, const struct path *path,
-				__u32 mask)
+static inline void fsnotify_dentry(struct dentry *dentry, __u32 mask)
 {
-	int ret = fsnotify_parent(path, NULL, mask);
+	struct inode *inode = d_inode(dentry);
 
+	if (S_ISDIR(inode->i_mode))
+		mask |= FS_ISDIR;
+
+	fsnotify_parent(NULL, dentry, mask);
+	fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0);
+}
+
+static inline int fsnotify_file(struct file *file, __u32 mask)
+{
+	const struct path *path = &file->f_path;
+	struct inode *inode = file_inode(file);
+	int ret;
+
+	if (file->f_mode & FMODE_NONOTIFY)
+		return 0;
+
+	if (S_ISDIR(inode->i_mode))
+		mask |= FS_ISDIR;
+
+	ret = fsnotify_parent(path, NULL, mask);
 	if (ret)
 		return ret;
+
 	return fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0);
 }
 
@@ -58,8 +78,6 @@  static inline int fsnotify_path(struct inode *inode, const struct path *path,
 static inline int fsnotify_perm(struct file *file, int mask)
 {
 	int ret;
-	const struct path *path = &file->f_path;
-	struct inode *inode = file_inode(file);
 	__u32 fsnotify_mask = 0;
 
 	if (file->f_mode & FMODE_NONOTIFY)
@@ -70,7 +88,7 @@  static inline int fsnotify_perm(struct file *file, int mask)
 		fsnotify_mask = FS_OPEN_PERM;
 
 		if (file->f_flags & __FMODE_EXEC) {
-			ret = fsnotify_path(inode, path, FS_OPEN_EXEC_PERM);
+			ret = fsnotify_file(file, FS_OPEN_EXEC_PERM);
 
 			if (ret)
 				return ret;
@@ -79,10 +97,7 @@  static inline int fsnotify_perm(struct file *file, int mask)
 		fsnotify_mask = FS_ACCESS_PERM;
 	}
 
-	if (S_ISDIR(inode->i_mode))
-		fsnotify_mask |= FS_ISDIR;
-
-	return fsnotify_path(inode, path, fsnotify_mask);
+	return fsnotify_file(file, fsnotify_mask);
 }
 
 /*
@@ -229,15 +244,7 @@  static inline void fsnotify_rmdir(struct inode *dir, struct dentry *dentry)
  */
 static inline void fsnotify_access(struct file *file)
 {
-	const struct path *path = &file->f_path;
-	struct inode *inode = file_inode(file);
-	__u32 mask = FS_ACCESS;
-
-	if (S_ISDIR(inode->i_mode))
-		mask |= FS_ISDIR;
-
-	if (!(file->f_mode & FMODE_NONOTIFY))
-		fsnotify_path(inode, path, mask);
+	fsnotify_file(file, FS_ACCESS);
 }
 
 /*
@@ -245,15 +252,7 @@  static inline void fsnotify_access(struct file *file)
  */
 static inline void fsnotify_modify(struct file *file)
 {
-	const struct path *path = &file->f_path;
-	struct inode *inode = file_inode(file);
-	__u32 mask = FS_MODIFY;
-
-	if (S_ISDIR(inode->i_mode))
-		mask |= FS_ISDIR;
-
-	if (!(file->f_mode & FMODE_NONOTIFY))
-		fsnotify_path(inode, path, mask);
+	fsnotify_file(file, FS_MODIFY);
 }
 
 /*
@@ -261,16 +260,12 @@  static inline void fsnotify_modify(struct file *file)
  */
 static inline void fsnotify_open(struct file *file)
 {
-	const struct path *path = &file->f_path;
-	struct inode *inode = file_inode(file);
 	__u32 mask = FS_OPEN;
 
-	if (S_ISDIR(inode->i_mode))
-		mask |= FS_ISDIR;
 	if (file->f_flags & __FMODE_EXEC)
 		mask |= FS_OPEN_EXEC;
 
-	fsnotify_path(inode, path, mask);
+	fsnotify_file(file, mask);
 }
 
 /*
@@ -278,16 +273,10 @@  static inline void fsnotify_open(struct file *file)
  */
 static inline void fsnotify_close(struct file *file)
 {
-	const struct path *path = &file->f_path;
-	struct inode *inode = file_inode(file);
-	fmode_t mode = file->f_mode;
-	__u32 mask = (mode & FMODE_WRITE) ? FS_CLOSE_WRITE : FS_CLOSE_NOWRITE;
-
-	if (S_ISDIR(inode->i_mode))
-		mask |= FS_ISDIR;
+	__u32 mask = (file->f_mode & FMODE_WRITE) ? FS_CLOSE_WRITE :
+						    FS_CLOSE_NOWRITE;
 
-	if (!(file->f_mode & FMODE_NONOTIFY))
-		fsnotify_path(inode, path, mask);
+	fsnotify_file(file, mask);
 }
 
 /*
@@ -295,14 +284,7 @@  static inline void fsnotify_close(struct file *file)
  */
 static inline void fsnotify_xattr(struct dentry *dentry)
 {
-	struct inode *inode = dentry->d_inode;
-	__u32 mask = FS_ATTRIB;
-
-	if (S_ISDIR(inode->i_mode))
-		mask |= FS_ISDIR;
-
-	fsnotify_parent(NULL, dentry, mask);
-	fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0);
+	fsnotify_dentry(dentry, FS_ATTRIB);
 }
 
 /*
@@ -311,7 +293,6 @@  static inline void fsnotify_xattr(struct dentry *dentry)
  */
 static inline void fsnotify_change(struct dentry *dentry, unsigned int ia_valid)
 {
-	struct inode *inode = dentry->d_inode;
 	__u32 mask = 0;
 
 	if (ia_valid & ATTR_UID)
@@ -332,13 +313,8 @@  static inline void fsnotify_change(struct dentry *dentry, unsigned int ia_valid)
 	if (ia_valid & ATTR_MODE)
 		mask |= FS_ATTRIB;
 
-	if (mask) {
-		if (S_ISDIR(inode->i_mode))
-			mask |= FS_ISDIR;
-
-		fsnotify_parent(NULL, dentry, mask);
-		fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0);
-	}
+	if (mask)
+		fsnotify_dentry(dentry, mask);
 }
 
 #endif	/* _LINUX_FS_NOTIFY_H */