Re: 2.6.29 -mm merge plans

Christoph Hellwig
2009-01-06T23:32:23Z

Yes, the version you attached looks much better, and correct.

Just some minor comments:

> +++ b/fs/hfsplus/export.c
> @@ -0,0 +1,118 @@
> +/*
> + * linux/fs/hfsplus/export.c
> + *

Please don't put filenames in top of file comments. They don't serve
any purpose and easily get out of date.

> + if ( be16_to_cpu(entry.type) != HFSPLUS_FOLDER_THREAD) {

no space after the opening brace, please/

> + inode = hfsplus_iget(child->d_sb, be32_to_cpu(entry.thread.parentID));
> + if (IS_ERR(inode)) {
> + printk(KERN_ERR "hfs: unable to find parent, call the social services\n");
> + return ERR_CAST(inode);
> + }

no lines longer than 80 characters please.

> + inode = hfsplus_iget(sb, ino);
> + if (IS_ERR(inode))
> + return ERR_CAST(inode);
> +
> + /* probably superfluous but it does not seem to hurt */
> + if (generation && inode->i_generation != generation) {
> + /* we didn't find the right inode.. */
> + iput(inode);
> + return ERR_PTR(-ESTALE);
> + }
> + return inode;

Given that hfsplus never sets i_generation it's superflous. So just
write the above as

return hfsplus_iget(sb, ino);

And maybe write a little comment that there is no generation number
in hfsplus.

> +/* Yes, most of these are left as NULL!!
> + * A NULL value implies the default, which works with hfs+-like file
> + * systems, but can be improved upon.
> + */

No need for this comment I think. All this is pretty well documented
in Documentation/filesystems/Exporting, and all the other filesystems
that just fill out these three methods don't comment on it either.