Messages in this thread |  | | | Date | Tue, 6 Jan 2009 18:31:53 -0500 | | From | Christoph Hellwig <> | | Subject | Re: 2.6.29 -mm merge plans | |
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.
|  |