[PATCH] use S_ISDIR() in link_path_walk() to decide whether the last path component is a directory

[PATCH] use S_ISDIR() in link_path_walk() to decide whether the last path component is a directory

[Alex Zarochentsev]

Hi,

This patch does not allow open(name, O_DIRECTORY) to be successful for
non-directories in reiser4.  It replaces ->i_op->lookup != NULL "is dir" check
for the last path component by explicit S_ISDIR(->i_mode) check. 

Regardless to reiser4, S_ISDIR() looks more clear there.

Thanks in advance.

===== fs/namei.c 1.104 vs edited =====
--- 1.104/fs/namei.c	Tue Aug 10 16:59:38 2004
+++ edited/fs/namei.c	Sun Sep 12 11:00:18 2004
@@ -816,7 +816,7 @@
 			break;
 		if (lookup_flags & LOOKUP_DIRECTORY) {
 			err = -ENOTDIR; 
-			if (!inode->i_op || !inode->i_op->lookup)
+			if (!S_ISDIR(inode->i_mode))
 				break;
 		}
 		goto return_base;

Re: [PATCH] use S_ISDIR() in link_path_walk() to decide whether the last path component is a directory

[viro]

On Mon, Sep 13, 2004 at 11:49:22AM +0400, Alex Zarochentsev wrote:
> Hi,
> 
> This patch does not allow open(name, O_DIRECTORY) to be successful for
> non-directories in reiser4.  It replaces ->i_op->lookup != NULL "is dir" check
> for the last path component by explicit S_ISDIR(->i_mode) check. 
> 
> Regardless to reiser4, S_ISDIR() looks more clear there.

The only objection here is that right now we are guaranteed that cwd and
root of every task have non-NULL ->lookup().  With your patch all we have
is S_ISDIR().

So we either need to check for non-NULL ->lookup() before the beginning of
loop in link_path_walk() or split the flag in two.  I would rather do the
former...

Re: [PATCH] use S_ISDIR() in link_path_walk() to decide whether the last path component is a directory

[David Dabbs]

>viro wrote
>On Mon, Sep 13, 2004 at 11:49:22AM +0400, Alex Zarochentsev wrote:
> Hi,
> 
> This patch does not allow open(name, O_DIRECTORY) to be successful for
> non-directories in reiser4.  It replaces ->i_op->lookup != NULL "is dir"
> check for the last path component by explicit S_ISDIR(->i_mode) check. 
> 
> Regardless to reiser4, S_ISDIR() looks more clear there.
>
>The only objection here is that right now we are guaranteed that cwd and
>root of every task have non-NULL ->lookup().  With your patch all we have
>is S_ISDIR().
>
>So we either need to check for non-NULL ->lookup() before the beginning of
>loop in link_path_walk() or split the flag in two.  I would rather do the
>former...
> 

I'm working on something similar, but with alternate pathname resolution
when the path begins with exactly two slashes. Only pseudocode here because
I do not have access to my box:

In path_lookup()

    if (*name == '/') {
       if (*(name+1)=='/' && *(name+2)==':') {
          name+=3;
          nd->flags &= LOOKUP_SLASHSLASH
          if (*name!='/')
            goto relative;
       }
    

In link_path_walk()
    When LOOKUP_SLASHSLASH, handle names as follows:

    If this !S_ISDIR()

       next.name  Behavior 
       ------------------------------------------------------
       .          Same if i_op && i_op->lookup, else -ENOTDIR.
       ..         Same.
       ...        Okay if i_op && i_op->lookup, else -ENOTDIR.
       otherwise  Fails with -ENOENT.
       
    If S_ISDIR()
       any        Same behavior


I tested the //: & flag code and this works (in my limited testing yesterday
with bash and some test programs). 

This should limit "hybrid" file-directories to only one valid subdirectory,
the "special metadata" directory. Thus, if a new/modified app wants to
create/access metadata, it would do something like the following:

   # relative path
   cd /test
   touch foo.txt	
   mkdir foo.txt/...     # FAILS
   mkdir //:foo.txt/...
   echo JayRandom > //:foo.txt/.../Author

   # absolute path
   echo blahblah > //:/test/foo.txt/.../Title

   mkdir testdir
   mkdir //:testdir/...
   echo no > //:testdir/.../VirusScan

Yes, this means that a) ... is "removed" from the namespace and b) directory
metadata directories are visible to naïve applications/users while those for
files are not. But it does provide "metadata-aware" apps/users a consistent
way to access this info for both files and directories without resorting to
openat(). Since SuS provides for "implementation-specific" pathname
resolution when pathnames begin with exactly two slashes this should be
legal(?), if desired.

This doesn't address the issue of "active" metadata objects such as reiser4
provides e.g. foo.txt/metas/perms. That's a different discussion. 


David

Re: [PATCH] use S_ISDIR() in link_path_walk() to decide whether the last path component is a directory

[viro]

On Mon, Sep 13, 2004 at 04:34:15PM -0500, David Dabbs wrote:
> I'm working on something similar, but with alternate pathname resolution
> when the path begins with exactly two slashes. Only pseudocode here because
> I do not have access to my box:
> 
>     if (*name == '/') {
>        if (*(name+1)=='/' && *(name+2)==':') {
>           name+=3;

	Pathname resolution is a hell of a fundamental thing and kludges
like that are too ugly to be acceptable.  If you can't make that clean
and have to resort to stuffing "special cases" (read: barfbag of ioctl
magnitude) into the areas that might be unspecified by POSIX, don't do it
at all.

	I don't like the amount of handwaving from Hans, but *that* is far
worse.  Vetoed.