[PATCH] Add basic export support to HFS+.

[PATCH] Add basic export support to HFS+.

[Diego Elio 'Flameeyes' Pettenò]

From: Diego E. 'Flameeyes' Pettenò <flameeyes@gmail.com>

The functions' skeleton is taken from ext2/super.c and seems to work
fine for R/W access to HFS+ non-journaled case-sensitive filesystems.

It's probably slow and has a lot of space for improvement but it's
still better than having no hope to export HFS+ filesystems.

Signed-off-by: Diego E. 'Flameeyes' Pettenò <flameeyes@gmail.com>
---

 fs/hfsplus/super.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 49 insertions(+), 0 deletions(-)

diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index eb74531..45b48cc 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -11,6 +11,7 @@
 #include <linux/init.h>
 #include <linux/pagemap.h>
 #include <linux/fs.h>
+#include <linux/exportfs.h>
 #include <linux/slab.h>
 #include <linux/vfs.h>
 #include <linux/nls.h>
@@ -281,6 +282,53 @@ static const struct super_operations hfsplus_sops = {
 	.show_options	= hfsplus_show_options,
 };
 
+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!!
+	 */
+	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);
+	}
+	return inode;
+}
+
+static struct dentry *hfsplus_fh_to_dentry(struct super_block *sb, struct fid *fid,
+					   int fh_len, int fh_type)
+
+{
+	return generic_fh_to_dentry(sb, fid, fh_len, fh_type,
+				    hfsplus_export_get_inode);
+}
+
+static struct dentry *hfsplus_fh_to_parent(struct super_block *sb, struct fid *fid,
+					   int fh_len, int fh_type)
+
+{
+	return generic_fh_to_parent(sb, fid, fh_len, fh_type,
+				    hfsplus_export_get_inode);
+}
+
+/* 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.
+ */
+static const struct export_operations hfsplus_export_ops = {
+	.fh_to_dentry = hfsplus_fh_to_dentry,
+	.fh_to_parent = hfsplus_fh_to_parent,
+};
+
 static int hfsplus_fill_super(struct super_block *sb, void *data, int silent)
 {
 	struct hfsplus_vh *vhdr;
@@ -345,6 +393,7 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent)
 
 	/* Set up operations so we can load metadata */
 	sb->s_op = &hfsplus_sops;
+	sb->s_export_op = &hfsplus_export_ops;
 	sb->s_maxbytes = MAX_LFS_FILESIZE;
 
 	if (!(vhdr->attributes & cpu_to_be32(HFSPLUS_VOL_UNMNT))) {

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

[Christoph Hellwig]

On Thu, Dec 04, 2008 at 06:47:06PM +0100, Diego Elio 'Flameeyes' Petten?? wrote:
> From: Diego E. 'Flameeyes' Petten?? <flameeyes@gmail.com>
> 
> The functions' skeleton is taken from ext2/super.c and seems to work
> fine for R/W access to HFS+ non-journaled case-sensitive filesystems.
> 
> It's probably slow and has a lot of space for improvement but it's
> still better than having no hope to export HFS+ filesystems.
> 
> Signed-off-by: Diego E. 'Flameeyes' Petten?? <flameeyes@gmail.com>
> ---
> 
>  fs/hfsplus/super.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 49 insertions(+), 0 deletions(-)
> 

> +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.

> +	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?


> +/* 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.

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.

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

[Diego E. 'Flameeyes' Pettenò]

[-- Attachment #1: Type: text/plain, Size: 2530 bytes --]

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 #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]