Re: [brauner-vfs:vfs.tmpfs 15/19] mm/shmem.c:3611 shmem_xattr_handler_set() warn: passing zero to 'PTR_ERR'

[Hugh Dickins <hughd@google.com>]

On Fri, 11 Aug 2023, Dan Carpenter wrote:
...
> smatch warnings:
> mm/shmem.c:3611 shmem_xattr_handler_set() warn: passing zero to 'PTR_ERR'
...
> 46170cc50828c3 Hugh Dickins        2023-08-08  3604  	old_xattr = simple_xattr_set(&info->xattrs, name, value, size, flags);
> 46170cc50828c3 Hugh Dickins        2023-08-08  3605  	if (!IS_ERR(old_xattr)) {
> 46170cc50828c3 Hugh Dickins        2023-08-08  3606  		simple_xattr_free(old_xattr);
> 46170cc50828c3 Hugh Dickins        2023-08-08  3607  		old_xattr = NULL;
> 36f05cab0a2c97 Jeff Layton         2022-09-09  3608  		inode->i_ctime = current_time(inode);
> 36f05cab0a2c97 Jeff Layton         2022-09-09  3609  		inode_inc_iversion(inode);
> 36f05cab0a2c97 Jeff Layton         2022-09-09  3610  	}
> 46170cc50828c3 Hugh Dickins        2023-08-08 @3611  	return PTR_ERR(old_xattr);
> 
> This code is correct but it would be nicer to reverse the checking:
> 
> 	old_xattr = simple_xattr_set(&info->xattrs, name, value, size, flags);
> 	if (IS_ERR(old_xattr))
> 		return PTR_ERR(old_xattr);
> 
> 	simple_xattr_free(old_xattr);
> 	inode->i_ctime = current_time(inode);
> 	inode_inc_iversion(inode);
> 
> 	return 0;

Thanks, Dan, but I'm going to resist making any change here.

As you observe, the code is correct as is.  And I did have a version
(unposted) which laid it out exactly as you have it there.  But the
collisions between different branches of vfs.git (and subsequent mods
here in vfs.tmpfs) was such that I aimed to minimize the interference.

Please let's leave it as is: there is nothing wrong with PTR_ERR(NULL).

Hugh