Re: [PATCH] Add basic export support to HFS+.

[Diego E. 'Flameeyes']

On Tue, 2008-12-09 at 05:06 -0500, Christoph Hellwig wrote:
> > +static struct inode *hfsplus_export_get_inode(struct super_block *sb,
> > +					      u64 ino, u32 generation)
> > +{
> > +	struct inode *inode;
> > +
> > +	if (ino < HFSPLUS_FIRSTUSER_CNID && ino != HFSPLUS_ROOT_CNID)
> > +		return ERR_PTR(-ESTALE);
> > +
> > +	/* iget isn't really right if the inode is currently unallocated!!
> > +	 */
> 
> I don't understand this comment.  This interface is only used when
> getting the FH for an already allocated inode.  hfsplus_iget is what
> all other operations use in hfsplus, so it seems fine.  hfs experts
> might chime in that something else might be needed due to the strange
> ways links work in hfs, but from the VFS POW it seems fine.

Sincerely, this is just blatantly copied over from the equivalent ext2
function.

> 
> > +	inode = hfsplus_iget(sb, ino);
> > +	if (IS_ERR(inode))
> > +		return ERR_CAST(inode);
> > +	if (generation && inode->i_generation != generation) {
> > +		/* we didn't find the right inode.. */
> > +		iput(inode);
> > +		return ERR_PTR(-ESTALE);
> > +	}
> 
> hfsplus never updates i_generation, so this check is superflous.  Brad,
> Roman: is there something that can help to guard against inode number
> reuse on HFS/HFSplus?

As above, just copied over, I tried to keep the code as similar as
possible so to make it clear that it's a copy-paste.

> 
> 
> > +/* Yes, most of these are left as NULL!!
> > + * A NULL value implies the default, which (hopefully) works with
> > + * hfs+-like file systems, but can be improved upon.
> > + * Currently only fh_to_dentry is required.
> > + */
> 
> You _do_ need a get_parent, otherwise access will magically fail once
> any directory inode in a path is out of the cache.  This can be
> trivially reproduced by unexporting and unmounting a filesystem while
> a client has a cwd deep inside the exported filesystem.

Okay, I think I was able to reproduce this both in "production" (as in,
making use of this in my real system) and on my testing virtual machine.
I'll get it fixed.

> And otherwise I don't think this comment is helpful in the code,
> fh_to_parent / fh_To_dentry and get_parent is what most simpler
> filesystems have.

Yes I noticed, my reason to change the comment was that the code
actually only tests fh_to_dentry (while the comment on ext2 structure
says that get_parent is the only needed one).

-- 
Diego "Flameeyes" PettenÃ
http://blog.flameeyes.eu/

Attachment: signature.asc
Description: This is a digitally signed message part