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

[Carlos Maiolino <cem@kernel.org>]

On Tue, Apr 11, 2023 at 10:14:45AM +0200, Jan Kara wrote:
> 
> 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.

Great, I didn't notice it's an inline function :D

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

Ok, I'm updating the series today, hopefully I'll send the new version today or
tomorrow.

Thanks for the reviews!

-- 
Carlos Maiolino