Re: [PATCH] Fix d_path for lazy unmounts

[Neil Brown]

On Friday February 2, agruen@xxxxxxx wrote:
> Hello,
> 
> here is a bugfix to d_path. Please apply (after 2.6.20).

Looks good!  Just a couple of little comments (to prove that I have
really read it and thought about it :-)

> 
> Signed-off-by: Andreas Gruenbacher <agruen@xxxxxxx>

Reviewed-by: NeilBrown <neilb@xxxxxxx>

> 
> Index: linux-2.6/fs/dcache.c
> ===================================================================
> --- linux-2.6.orig/fs/dcache.c
> +++ linux-2.6/fs/dcache.c
> @@ -1739,45 +1739,43 @@ shouldnt_be_hashed:
>   * @rootmnt: vfsmnt to which the root dentry belongs
>   * @buffer: buffer to return value in
>   * @buflen: buffer length
> + * @fail_deleted: what to return for deleted files
>   *
> - * Convert a dentry into an ASCII path name. If the entry has been deleted
> - * the string " (deleted)" is appended. Note that this is ambiguous.
> + * Convert a dentry into an ASCII path name. If the entry has been deleted,
> + * then if @fail_deleted is true, ERR_PTR(-ENOENT) is returned. Otherwise,
> + * the the string " (deleted)" is appended. Note that this is ambiguous.

The behaviour in the face of a lazy unmount should be clarified in
this comment.

And I cannot help wondering if that behaviour should - in some cases -
be 'fail'.

i.e. if sys_getcwd is called on a directory that is no longer
connected to the root, it isn't clear to me that it should return
without an error.
Without your patch it can return garbage which is clearly wrong.
With you patch it will return a relative path name, which is also
wrong (it isn't a valid path that leads to the current working directory).
I would suggest that 'fail_deleted' be (e.g.) changed to
'fail_condition' where two conditions are defined
#define DPATH_FAIL_DELETED 1
#define DPATH_FAIL_DISCONNECTED 2

and both these are passed in by sys_getcwd.


> @@ -1791,33 +1789,49 @@ static char * __d_path( struct dentry *d
>  		parent = dentry->d_parent;
>  		prefetch(parent);
>  		namelen = dentry->d_name.len;
> -		buflen -= namelen + 1;
> -		if (buflen < 0)
> +		if (buflen <= namelen)
>  			goto Elong;

This bothers me.  You appear to be comparing buflen with namelen, but
are about the change buflen by 'namelen + 1'.  It looks like a bug.

In reality, you are comparing "buflen < namelen+1" but spelling it as
"buflen <= namelen".  I would prefer the full spelling with least room
for confusion.


> -		end -= namelen;
> -		memcpy(end, dentry->d_name.name, namelen);
> -		*--end = '/';
> -		retval = end;
> +		buflen -= namelen + 1;
> +		buffer -= namelen;
> +		memcpy(buffer, dentry->d_name.name, namelen);
> +		*--buffer = '/';

This wouldn't be my preference either.  It is important that 'buflen'
and 'buffer' move in step, but when I see
		buflen -= namelen + 1;
		buffer -= namelen;
it looks like they aren't.  Maybe:
> +		buflen -= namelen + 1;
> +		buffer -= namelen + 1;
> +		memcpy(buffer+1, dentry->d_name.name, namelen);
> +		*buffer = '/';

or am I being too picky?

NeilBrown
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/