Date: Tue, 15 Aug 2000 22:37:38 +0200
From: "Eli Zaretskii" <eliz AT is DOT elta DOT co DOT il>
Sender: halo1 AT zahav DOT net DOT il
To: lauras AT softhome DOT net
Message-Id: <5137-Tue15Aug2000223737+0300-eliz@is.elta.co.il>
X-Mailer: Emacs 20.6 (via feedmail 8.2.emacs20_6 I) and Blat ver 1.8.5b
CC: djgpp-workers AT delorie DOT com
In-reply-to: <39998D7D.70F85B9E@softhome.net> (message from Laurynas Biveinis
	on Tue, 15 Aug 2000 20:35:41 +0200)
Subject: Re: Patch: open() adjustment for symlinks
References:  <39998D7D DOT 70F85B9E AT softhome DOT net>
Reply-To: djgpp-workers AT delorie DOT com
Errors-To: nobody AT delorie DOT com
X-Mailing-List: djgpp-workers AT delorie DOT com
X-Unsubscribes-To: listserv AT delorie DOT com
Precedence: bulk

> Date: Tue, 15 Aug 2000 20:35:41 +0200
> From: Laurynas Biveinis <lauras AT softhome DOT net>
>  
> +@findex open AT r{, changed behaviour}

Too many "changed behavior" entries are not very useful (what will you
write next time when the behavior is changed?).  I suggest to write
this instead:

  @findex open AT r{, accepts @code{O_NOLINK} and @code{O_NOFOLLOW} flags}

>  int
>  open(const char* filename, int oflag, ...)
>  {
>    int fd, dmode, bintext, dont_have_share;
> +  char real_name[FILENAME_MAX + 1];
>    int should_create = (oflag & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL);
>  
> +  /* Solve symlinks and honor O_NOLINK flag  */
> +  if (oflag & O_NOLINK)
> +      strcpy(real_name, filename);
> +  else
> +  {
> +     if (!__solve_symlinks(filename, real_name))
> +        return -1; /* errno from from __solve_symlinks() */
> +  }

Bother.  __solve_symlinks doesn't have an FSEXT, does it?  So what
does a file extension do if it wants to catch this new code?

Also note that the special precautions that `open' takes not to fail
in the case of a file that's open by another program on Windows, in
this fragment:

> -      if (__file_exists(filename))
> +      if (__file_exists(real_name))
>        {
>  	/* Under multi-taskers, such as Windows, our file might be
>  	   open by some other program with DENY-NONE sharing bit,
> @@ -75,12 +126,12 @@
>  	   DENY-NONE bit set, unless some sharing bits were already
>  	   set in the initial call.  */
>  	if (dont_have_share)
> -	  fd = _open(filename, oflag | SH_DENYNO);
> +	  fd = _open(real_name, oflag | SH_DENYNO);
>        }

aren't in __solve_symlinks, sop `open' might fail inside
__solve_symlinks in these cases.

> +static char *find_last_sep(const char * str)
> +{
> +   char * res = strrchr(str, '/');
> +   if (!res)
> +      res = strrchr(str, '\\');
> +   if (!res)
> +      res = unconst(str, char*);
> +   return res;

Why not use `basename'?  Don't you need to handle the case of "d:foo"?
(If you do use `basename', you need to make a stub for it.)

> +@code{open} will fail with errno set to ELOOP, if the last patch component
> +in @var{file} is symlink.

Please put ELOOP inside @code{}.