LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Neil Brown <neilb@suse.de>
To: linux-kernel@vger.kernel.org
Cc: linux-fsdevel@vger.kernel.org, Andrew Morton <akpm@osdl.org>,
	Tony Jones <tonyj@suse.de>
Subject: Re: [PATCH] Fix d_path for lazy unmounts
Date: Mon, 5 Feb 2007 11:15:21 +1100	[thread overview]
Message-ID: <17862.30489.694079.138765@notabene.brown> (raw)
In-Reply-To: message from Andreas Gruenbacher on Friday February 2

On Friday February 2, agruen@suse.de 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@suse.de>

Reviewed-by: NeilBrown <neilb@suse.de>

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

  reply	other threads:[~2007-02-05  0:15 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-02-03  3:23 Andreas Gruenbacher
2007-02-05  0:15 ` Neil Brown [this message]
2007-02-15  2:43   ` Andreas Gruenbacher
2007-02-05  8:32 ` Andreas Gruenbacher
2007-02-05 18:37   ` [PATCH]Add memory barrier before clear bit in unlock_buffer() Mingming Cao
2007-02-14  8:19   ` [PATCH] Fix d_path for lazy unmounts Andreas Gruenbacher
2007-02-14  8:29     ` Olaf Hering
2007-02-14  8:42       ` Andreas Gruenbacher
2007-02-14 15:37     ` Linus Torvalds
2007-02-14 19:39       ` Andreas Gruenbacher
2007-02-14 22:57         ` Andreas Gruenbacher
2007-02-15  3:13           ` Andreas Gruenbacher
2007-02-17 13:30             ` Andreas Gruenbacher
2007-02-15 12:53           ` Jan Engelhardt
2007-02-15 13:19             ` Andreas Gruenbacher

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=17862.30489.694079.138765@notabene.brown \
    --to=neilb@suse.de \
    --cc=akpm@osdl.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tonyj@suse.de \
    --subject='Re: [PATCH] Fix d_path for lazy unmounts' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).