Re: [PATCH 2/6] shmem: make shmem_get_inode() return ERR_PTR instead of NULL

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi!

On Tue 11-04-23 09:47:08, Carlos Maiolino wrote:
> My apologies it took a while to get back to you on this one, I was
> accumulating reviews before running through all of them.

Sure, that is fine.

> > >  	inode = shmem_get_inode(idmap, dir->i_sb, dir, mode, 0, VM_NORESERVE);
> > > -	if (inode) {
> > > -		error = security_inode_init_security(inode, dir,
> > > -						     NULL,
> > > -						     shmem_initxattrs, NULL);
> > > -		if (error && error != -EOPNOTSUPP)
> > > -			goto out_iput;
> > > -		error = simple_acl_create(dir, inode);
> > > -		if (error)
> > > -			goto out_iput;
> > > -		d_tmpfile(file, inode);
> > > -	}
> > > +
> > > +	if (IS_ERR(inode))
> > > +		return PTR_ERR(inode);
> > 
> > This doesn't look correct. Previously, we've called
> > finish_open_simple(file, error), now you just return error... Otherwise the
> > patch looks good to me.
> 
> I see what you mean. But, finish_open_simple() simply does:
> 
> 	if (error)
> 		return error;
> 
> So, calling it with a non-zero value for error at most will just add another
> function call into the stack.

I see, I didn't look inside finish_open_simple() :). Well, it is at least
inline function so actually no function call.

> I'm not opposed to still call finish_open_simple() but I don't think it adds
> anything.

Yeah, the value of the call is questionable but given it takes 'error'
argument only so that it could do that if (error) return error; dance, it
seems to be very much expected it is called in error cases as well.

> If you prefer it being called. I thought about adding a new label, something
> like:
> 
> 	if (IS_ERR(inode))
> 		goto err_out;
> 	.
> 	.
> 	.
> 	d_tmpfile(file, inode)
> err_out:
> 	return finish_open_simple(file, error)
> .
> .
> .
> 
> Would it work for you?

Yeah, I'd prefer we keep calling finish_open_simple(). Adding a label for
it is fine by me.

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux