Thanks, I know some people have been waiting for this for quite a while.

Please make sure Jan Kara who's the new udf maintainer and linux-fsdevel

where we discuss general filesystem related issue for future revisions
isofs might not be the very best example since it's an odd filesystem,

but then so is udf.  I'll go through your patch in a little more detail
Any reason this is not called udf_get_parent to follow the scheme
This if/else block looks little odd, and following the locking is a bit

hard.  What about doing it like the following:
	lock_kernel();

	if (!udf_find_entry(child->d_inode, &dotdot, &fibh, &cfi))

		goto out_unlock;
	if (fibh.sbh != fibh.ebh)

		brelse(fibh.ebh);

	brelse(fibh.sbh);
	inode = udf_iget(child->d_inode->i_sb,

			 lelb_to_cpu(cfi.icb.extLocation));

	if (!inode)

		goto out_unlock;

	unlock_kernel();
	parent = d_alloc_anon(inode);

	if (!parent) {

		iput(inode);

		parent = ERR_PTR(-ENOMEM);

	}
	return parent;

 out_unlock:

	unlock_kernel();

	return ERR_PTR(-EACCESS);
to follow other filesystems this should be called

udf_nfs_get_inode.  Also please decide if you want to put the static

and return type on the same line or on a separate one.  Having it on

the same one is documented in Documentation/Codingstyle but separate
it would be better to introduce a version of udf_iget that can check

the generation and return an error instead of having to check this

later.  If you don't think you're up to modifying code we could do

this later on, though.  In this case please note this in the patch

description and fix up the above formatting to read something like:
	if (is_bad_inode(inode) ||
It would be useful if you could add symbolic constants for the

two fh types you add and chose values not yet used by other filesystems,

e.g. 0x51 and 0x52.  This will help people sniffing the nfs on the

wire protocol to understand what file handle they're dealing with.
Also you migh want to make the fh_len check more explicit and...
[ message continues ]

Most of the code is copied from udf_lookup(..)

To me the locking is simple. Before the two returns you have an unlock.

It's pretty clear that all control-paths are covered. However changing

Ok.
Attached is a new attempt at a patch.
Signed-off-by: Rasmus Rohde <rohde@duff.dk>
--- fs/udf/namei.c.orig	2007-10-10 16:22:30.000000000 +0200

+++ fs/udf/namei.c	2008-02-05 18:28:13.000000000 +0100

@@ -31,6 +31,7 @@

 #include <linux/smp_lock.h>

 #include <linux/buffer_head.h>

 #include <linux/sched.h>

+#include <linux/exportfs.h>
 static inline int udf_match(int len1, const char *name1, int len2,

 			    const char *name2)

@@ -315,9 +316,8 @@ static struct dentry *udf_lookup(struct

 		}

 	}

 	unlock_kernel();

-	d_add(dentry, inode);
-	return NULL;

+	return d_splice_alias(inode, dentry);

 }
 static struct fileIdentDesc *udf_add_entry(struct inode *dir,

@@ -1231,6 +1231,135 @@ end_rename:

 	return retval;

 }
+static struct dentry *udf_get_parent(struct dentry *child)

+{

+	struct dentry *parent;

+	struct inode *inode = NULL;

+	struct dentry dotdot;

+	struct fileIdentDesc cfi;

+	struct udf_fileident_bh fibh;

+

+	dotdot.d_name.name = "..";

+	dotdot.d_name.len = 2;

+

+	lock_kernel();

+	if (!udf_find_entry(child->d_inode, &dotdot, &fibh, &cfi))

+		goto out_unlock;

+

+	if (fibh.sbh != fibh.ebh)

+		brelse(fibh.ebh);

+	brelse(fibh.sbh);

+

+	inode = udf_iget(child->d_inode->i_sb,

+			 lelb_to_cpu(cfi.icb.extLocation));

+	if (!inode)

+		goto out_unlock;

+	unlock_kernel();

+

+	parent = d_alloc_anon(inode);

+	if (!parent) {

+		iput(inode);

+		parent = ERR_PTR(-ENOMEM);

+	}

+

+	return parent;

+out_unlock:

+	unlock_kernel();

+	return ERR_PTR(-EACCES);

+}

+

+

+static struct dentry *udf_nfs_get_inode(struct super_block *sb, u32 block,

+					u16 partref, __u32 generation)

+{

+	struct inode *inode;

+	struct dentry *result;

+	kernel_lb_addr loc;

+

+	if (block == 0)

+		return ERR_PTR(-ESTALE);

+

+...
[ message continues ]

Have you ever tried this? I think this could never work. UDF doesn't have

entry named .. in a directory. You have to search for an entry that has

in fileCharacteristics set bit FID_FILE_CHAR_PARENT. Maybe you could

hack-around udf_find_entry() to recognize .. dentry and do the search

  Otherwise the patch looks fine. But please rediff the patch against

Andrew's development tree (or -mm) because there are some cleanups there...

Thanks.
									Honza

--

Jan Kara <jack@suse.cz>

SUSE Labs, CR

-

Probably not. I just tested that I could read files and navigate the

directory structure. However looking into UDF I think you are right - it

will fail.

I have extended udf_find_entry() to do an explicit check based on

fileCharacteristics as you propose.

Certainly there are. New patch against 2.6.24-mm1:
Signed-off-by: Rasmus Rohde <rohde@duff.dk>
diff -uprN -X linux-2.6.24-mm1-vanilla/Documentation/dontdiff linux-2.6.24-mm1-vanilla/fs/udf/namei.c linux-2.6.24-mm1/fs/udf/namei.c

--- linux-2.6.24-mm1-vanilla/fs/udf/namei.c	2008-02-06 21:23:36.000000000 +0100

+++ linux-2.6.24-mm1/fs/udf/namei.c	2008-02-06 21:41:38.000000000 +0100

@@ -31,6 +31,7 @@

 #include <linux/smp_lock.h>

 #include <linux/buffer_head.h>

 #include <linux/sched.h>

+#include <linux/exportfs.h>
 static inline int udf_match(int len1, const char *name1, int len2,

 			    const char *name2)

@@ -159,6 +160,8 @@ static struct fileIdentDesc *udf_find_en

 	sector_t offset;

 	struct extent_position epos = {};

 	struct udf_inode_info *dinfo = UDF_I(dir);

+	int isdotdot = dentry->d_name.len == 2 &&

+		dentry->d_name.name[0] == '.' && dentry->d_name.name[1] == '.';
 	size = udf_ext0_offset(dir) + dir->i_size;

 	f_pos = udf_ext0_offset(dir);

@@ -232,6 +235,12 @@ static struct fileIdentDesc *udf_find_en

 				continue;

 		}
+		if ((cfi->fileCharacteristics & FID_FILE_CHAR_PARENT) &&

+		    isdotdot) {

+			brelse(epos.bh);

+			return fi;

+		}

+

 		if (!lfi)

 			continue;
@@ -324,9 +333,8 @@ static struct dentry *udf_lookup(struct

 		}

 	}

 	unlock_kernel();

-	d_add(dentry, inode);
-	return NULL;

+	return d_splice_alias(inode, dentry);

 }
 static struct fileIdentDesc *udf_add_entry(struct inode *dir,

@@ -1298,6 +1306,134 @@ end_rename:

 	return retval;

 }
+static struct dentry *udf_get_parent(struct dentry *child)

+{

+	struct dentry *parent;

+	struct inode *inode = NULL;

+	struct dentry dotdot;

+	struct fileIdentDe...
[ message continues ]

There's still a few trivial warnings from scripts/checkpatch.pl that

should be fixed up:
ERROR: trailing whitespace

#88: FILE: fs/udf/namei.c:1323:

+^I$
ERROR: trailing whitespace

#92: FILE: fs/udf/namei.c:1327:

+^I^I$
ERROR: trailing whitespace

#185: FILE: fs/udf/namei.c:1420:

+^I^Ifid->udf.parent_partref = location.partitionReferenceNum;^I$
WARNING: externs should be avoided in .c files

#212: FILE: fs/udf/super.c:79:

+extern struct export_operations udf_export_ops;
total: 3 errors, 1 warnings, 218 lines checked
-

Ok - I have checked get_parent and it works as expected.

I used the "Neil Brown"-test mentioned elsewhere in this thread and

Fixed that. Sorry for not running checkpatch.pl before submitting.
Before posting the last and hopefully final patch I'd like to know what

Jan says about open coding the lookup for ..

It will mean a lot of code duplication and I think it makes good sense

for udf_find_entry to be able to handle ..
-

Yes, I think opencoding it would really lead to larger code duplication

than I'd like so please keep the change in udf_find_entry(). Thanks.
								Honza

--

Jan Kara <jack@suse.cz>

SUSE Labs, CR

-

Great - then I think we are hopefully at a patch that can be accepted.
Signed-off-by: Rasmus Rohde <rohde@duff.dk>
diff -uprN -X linux-2.6.24-mm1-vanilla/Documentation/dontdiff linux-2.6.24-mm1-vanilla/fs/udf/namei.c linux-2.6.24-mm1/fs/udf/namei.c

--- linux-2.6.24-mm1-vanilla/fs/udf/namei.c	2008-02-06 21:23:36.000000000 +0100

+++ linux-2.6.24-mm1/fs/udf/namei.c	2008-02-07 07:13:04.000000000 +0100

@@ -31,6 +31,7 @@

 #include <linux/smp_lock.h>

 #include <linux/buffer_head.h>

 #include <linux/sched.h>

+#include <linux/exportfs.h>
 static inline int udf_match(int len1, const char *name1, int len2,

 			    const char *name2)

@@ -159,6 +160,8 @@ static struct fileIdentDesc *udf_find_en

 	sector_t offset;

 	struct extent_position epos = {};

 	struct udf_inode_info *dinfo = UDF_I(dir);

+	int isdotdot = dentry->d_name.len == 2 &&

+		dentry->d_name.name[0] == '.' && dentry->d_name.name[1] == '.';
 	size = udf_ext0_offset(dir) + dir->i_size;

 	f_pos = udf_ext0_offset(dir);

@@ -232,6 +235,12 @@ static struct fileIdentDesc *udf_find_en

 				continue;

 		}
+		if ((cfi->fileCharacteristics & FID_FILE_CHAR_PARENT) &&

+		    isdotdot) {

+			brelse(epos.bh);

+			return fi;

+		}

+

 		if (!lfi)

 			continue;
@@ -324,9 +333,8 @@ static struct dentry *udf_lookup(struct

 		}

 	}

 	unlock_kernel();

-	d_add(dentry, inode);
-	return NULL;

+	return d_splice_alias(inode, dentry);

 }
 static struct fileIdentDesc *udf_add_entry(struct inode *dir,

@@ -1298,6 +1306,134 @@ end_rename:

 	return retval;

 }
+static struct dentry *udf_get_parent(struct dentry *child)

+{

+	struct dentry *parent;

+	struct inode *inode = NULL;

+	struct dentry dotdot;

+	struct fileIdentDesc cfi;

+	struct udf_fileident_bh fibh;

+

+	dotdot.d_name.name = "..";

+	dotdot.d_name.len = 2;

+

+	lock_kernel();

+	if (!udf_find_entry(child->d_inode, &dotdot, &fibh, &cfi))

+		goto out_unlock;

+

+	if (fibh.sbh !=...
[ message continues ]

Jan, any reason this patch didn't go in with the last merge?  I'd really

---end quoted text---

--

Thanks for catching this. It seems I missed the patch when merging all

the UDF patches I had in my mailbox. I have it merged in my git tree and

will push it to Linus with other patches.
--

Jan Kara <jack@suse.cz>

SUSE Labs, CR

--

Honza

--

Jan Kara <jack@suse.cz>

SUSE Labs, CR

-

Testing this is pretty hard.  You export a filesystem, then cd somewhere

deep into a directory hiearchy in there.  Then unexport the filesystem

and unmount on the server.  mount it back on the server, export it again

and do something with a file from the directory you've cd into before

the unmount.  Make sure you have a printk in your get_parent method

to make sure you're really hitting it.
Btw, I think it would be nicer to opencode the .. lookup in get_parent

instead of changing udf_find_entry.  The lookup for .. is not needed

by anything else, and get_parent only looks for it so it's a natural

place to opencode it there.
-

^^^^^^^
Argh - this should have been WITH so the line reads:
if (fh_len != 5 || fh_type != FILEID_UDF_WITH_PARENT)
I'll just hold back a little reposting a new patch if other comments

should happen to show up.
-