From mboxrd@z Thu Jan  1 00:00:00 1970
From: Christoph Hellwig <hch@infradead.org>
Subject: Re: [PATCH 3/4] switch all filesystems over to d_obtain_alias
Date: Tue, 9 Sep 2008 11:31:08 -0400
Message-ID: <20080909153108.GB29279@infradead.org>
References: <20080811134904.GE21264@lst.de> <E1Kd3GQ-0001v9-4u@pomaz-ex.szeredi.hu>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: hch@lst.de, viro@zeniv.linux.org.uk, linux-fsdevel@vger.kernel.org
To: Miklos Szeredi <miklos@szeredi.hu>
Return-path: <linux-fsdevel-owner@vger.kernel.org>
Received: from bombadil.infradead.org ([18.85.46.34]:49005 "EHLO
	bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1756507AbYIIPbL (ORCPT
	<rfc822;linux-fsdevel@vger.kernel.org>);
	Tue, 9 Sep 2008 11:31:11 -0400
Content-Disposition: inline
In-Reply-To: <E1Kd3GQ-0001v9-4u@pomaz-ex.szeredi.hu>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

On Tue, Sep 09, 2008 at 03:28:14PM +0200, Miklos Szeredi wrote:
> > -	if (get_node_id(inode) != FUSE_ROOT_ID) {
> > +	entry = d_obtain_alias(inode);
> > +	if (entry && !IS_ERR(entry) && get_node_id(inode) != FUSE_ROOT_ID) {
> 
> checking for entry != NULL superfluous here.

True.

> >  	}
> > @@ -696,17 +692,14 @@ static struct dentry *fuse_get_parent(st
> >  	name.name = "..";
> >  	err = fuse_lookup_name(child_inode->i_sb, get_node_id(child_inode),
> >  			       &name, &outarg, &inode);
> > -	if (err && err != -ENOENT)
> > -		return ERR_PTR(err);
> > -	if (err || !inode)
> > -		return ERR_PTR(-ESTALE);
> > -
> > -	parent = d_alloc_anon(inode);
> > -	if (!parent) {
> > -		iput(inode);
> > -		return ERR_PTR(-ENOMEM);
> > +	if (err) {
> > +		if (err == -ENOENT)
> > +			return -ESTALE;
> > +		return err;
> >  	}
> > -	if (get_node_id(inode) != FUSE_ROOT_ID) {
> > +
> > +	parent = d_obtain_alias(inode);
> > +	if (parent && !IS_ERR(parent) && get_node_id(inode) != FUSE_ROOT_ID) {
> 
> This is buggy: we want to return ERR_PTR(-ESTALE) for !inode, not
> NULL. So again, either d_obtain_alias() should turn !inode into
> -ENOENT, (which would gain nothing in this case) or it should simply
> expect inode to be not NULL.

ESTALE doesn't really help us here, but I agree that we want an error
return.

> Doing the !inode check in d_obtain_alias() seems only to confuse
> things, instead of simplifying them.

Doing the !inode check here means we can do a return
d_obtain_alias(foo_iget())) tailcall which is quite useful to reduce the
mess in the export operations.