Skip to contentSkip to conversation

befs: iget_locked() doesn't return an ERR_PTR

PATCH Applied Rakesh Pandit 2 participants 2 messages Jan 15, 2014 – Jan 16, 2014

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 befs: iget_locked() doesn't return an ERR_PTR

Also fix befs_iget return value if iget_locked fails.

---
 fs/befs/linuxvfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
fs/befs/linuxvfs.c +2 -2
index daa15d6..845d2d6 100644
@@ -324,8 +324,8 @@ static struct inode *befs_iget(struct super_block *sb, unsigned long ino)
 	befs_debug(sb, "---> befs_read_inode() " "inode = %lu", ino);
 
 	inode = iget_locked(sb, ino);
-	if (IS_ERR(inode))
-		return inode;
+	if (!inode)
+		return ERR_PTR(-ENOMEM);
 	if (!(inode->i_state & I_NEW))
 		return inode;
 
Signature · 1 line
1.7.11.7
maintainer Applied
#1 lore raw
Quoted 1 line · Rakesh Pandit

Also fix befs_iget return value if iget_locked fails.

Applied, but I really wonder if we'd made a mistake with iget{5}_locked() API - perhaps the calling conventions returning ERR_PTR(-ENOMEM) would've been better... Most of the callers are immediately followed by

	if (!inode)
		return ERR_PTR(-ENOMEM);

and in fact something like
inode_is_new(inode) = !ERR_PTR(inode) && (inode->i_flags & I_NEW) might've killed even more boilerplate in callers, so it might make sense to introduce a new variants of iget{,5}_locked that would return ERR_PTR(-ENOMEM) on out-of-memory and mark the original variants obsolete after a while...

Anyway, for now your patch is obviously the right fix.

BTW, the callers of iget{,5}_locked() that do not do that return ERR_PTR(-ENOMEM) on NULL thing are worth looking into:
* sysfs_get_inode() - callers of _that_ do the same thing; might
as well have done it there.
* nilfs_iget_locked() - ditto.
* fuse_iget() - ditto.
* btrfs_iget_locked() - ditto.
* udf_iget() - callers galore, with very inconsistent error values
propagated on that failure (EACCES, EIO, etc.); quite a few of those would be better off with ENOMEM (e.g. ->lookup() failing with EACCES on oom isn't right).
* __ecryptfs_get_inode() - EACCES instead of ENOMEM, for no good
reason I can see...
* gfs2_iget() - ENOBUFS instead of ENOMEM. ENOBUFS is
"No buffer space available (POSIX.1 (XSI STREAMS option))" and since we don't support STREAMS it's probably fair game, but... what the hell?
* ll_iget() - this one definitely ought to return ERR_PTR(), since
it does that on other errors; callers check for both. One of them converts NULL to ERR_PTR(-ENOMEM), another (in mount guts) treats it as EBADF (again, very odd return value when there's no file descriptors in sight).
* befs_iget() - the one you'd caught.
* hfs_iget() - one caller treats it as EACCES, another - as EINVAL.
To make things more complicated, there's another failure exit, which probably should be EINVAL. Overall, I'd make it return ERR_PTR()...
* cifs_iget() - probably would be better off with ERR_PTR(); most
of the callers treat NULL as ENOMEM, but cifs_posix_mkdir() is really odd in that respect... No sure what's going on there.
* hfs_btree_open() - callers end up treating that as EIO (and do that
to kmalloc() failures right in there)
* hpfs_lookup() - treats all errors as ENOENT, that one included...
* hpfs_fill_super() - same, but treats everything as EINVAL
* bdget() and bdget_disk() - a bunch of callers forget to check for
allocation failures at all; some are doing that legitimately, some are probably oopsable...