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

[Carlos Maiolino <cem@kernel.org>]

Hi Honza.

My apologies it took a while to get back to you on this one, I was accumulating
reviews before running through all of them.


> >  	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'm not opposed to still call finish_open_simple() but I don't think it adds
anything.

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?

> 
> 								Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

-- 
Carlos Maiolino