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-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
