[7/7] fs: Do not overload update_time
diff mbox series

Message ID 20191130053030.7868-8-deepa.kernel@gmail.com
State New
Headers show
Series
  • Delete timespec64_trunc()
Related show

Commit Message

Deepa Dinamani Nov. 30, 2019, 5:30 a.m. UTC
update_time() also has an internal function pointer
update_time. Even though this works correctly, it is
confusing to the readers.

Use a different name for the local variable.

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
---
 fs/inode.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Dave Chinner Dec. 1, 2019, 8:51 p.m. UTC | #1
On Fri, Nov 29, 2019 at 09:30:30PM -0800, Deepa Dinamani wrote:
> update_time() also has an internal function pointer
> update_time. Even though this works correctly, it is
> confusing to the readers.
> 
> Use a different name for the local variable.
> 
> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
> ---
>  fs/inode.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 12c9e38529c9..0be58a680457 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1675,12 +1675,12 @@ EXPORT_SYMBOL(generic_update_time);
>   */
>  static int update_time(struct inode *inode, struct timespec64 *time, int flags)
>  {
> -	int (*update_time)(struct inode *, struct timespec64 *, int);
> +	int (*cb)(struct inode *, struct timespec64 *, int);
>  
> -	update_time = inode->i_op->update_time ? inode->i_op->update_time :
> +	cb = inode->i_op->update_time ? inode->i_op->update_time :
>  		generic_update_time;
>  
> -	return update_time(inode, time, flags);
> +	return cb(inode, time, flags);

What's wrong with a simple if() like we use everywhere else for this
sort of thing?

	if (inode->i_op->update_time)
		return inode->i_op->update_time(inode, time, flags);
	return generic_update_time(inode, time, flags);

-Dave.
Christoph Hellwig Dec. 2, 2019, 5:48 p.m. UTC | #2
On Fri, Nov 29, 2019 at 09:30:30PM -0800, Deepa Dinamani wrote:
> -	int (*update_time)(struct inode *, struct timespec64 *, int);
> +	int (*cb)(struct inode *, struct timespec64 *, int);
>  
> -	update_time = inode->i_op->update_time ? inode->i_op->update_time :
> +	cb = inode->i_op->update_time ? inode->i_op->update_time :
>  		generic_update_time;
>  
> -	return update_time(inode, time, flags);
> +	return cb(inode, time, flags);

Just killing the pointless local variable cleans the function op, and
also avoids the expensive indirect call for the common case:

	if (inode->i_op->update_time)
		return inode->i_op->update_time(inode, time, flags);
	return generic_update_time(inode, time, flags);
Deepa Dinamani Dec. 2, 2019, 11:13 p.m. UTC | #3
On Mon, Dec 2, 2019 at 9:48 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Fri, Nov 29, 2019 at 09:30:30PM -0800, Deepa Dinamani wrote:
> > -     int (*update_time)(struct inode *, struct timespec64 *, int);
> > +     int (*cb)(struct inode *, struct timespec64 *, int);
> >
> > -     update_time = inode->i_op->update_time ? inode->i_op->update_time :
> > +     cb = inode->i_op->update_time ? inode->i_op->update_time :
> >               generic_update_time;
> >
> > -     return update_time(inode, time, flags);
> > +     return cb(inode, time, flags);
>
> Just killing the pointless local variable cleans the function op, and
> also avoids the expensive indirect call for the common case:

Will update this in v2. Thanks.

-Deepa

Patch
diff mbox series

diff --git a/fs/inode.c b/fs/inode.c
index 12c9e38529c9..0be58a680457 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1675,12 +1675,12 @@  EXPORT_SYMBOL(generic_update_time);
  */
 static int update_time(struct inode *inode, struct timespec64 *time, int flags)
 {
-	int (*update_time)(struct inode *, struct timespec64 *, int);
+	int (*cb)(struct inode *, struct timespec64 *, int);
 
-	update_time = inode->i_op->update_time ? inode->i_op->update_time :
+	cb = inode->i_op->update_time ? inode->i_op->update_time :
 		generic_update_time;
 
-	return update_time(inode, time, flags);
+	return cb(inode, time, flags);
 }
 
 /**