Date: Wed, 14 Jun 2000 12:11:17 +0300 (IDT)
From: Eli Zaretskii <eliz AT is DOT elta DOT co DOT il>
X-Sender: eliz AT is
To: Laurynas Biveinis <lauras AT softhome DOT net>
cc: DJGPP Workers <djgpp-workers AT delorie DOT com>
Subject: Re: Another one symlink patch
In-Reply-To: <3944ECA6.6DA1D8E3@softhome.net>
Message-ID: <Pine.SUN.3.91.1000614120950.29091B-100000@is>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
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


On Mon, 12 Jun 2000, Laurynas Biveinis wrote:

> This is the n-th (lost the number) patch to add symlink support.

It's 6th, according to my count ;-).  (Btw, I still have the last one you 
posted, in case you need that.)

Thanks for working on this.  My comments are below.

    -#define ENMFILE		38
    +#define ELOOP           38
    +#define ENMFILE		39

Why is it a good idea to change the numerical value of ENMFILE?  It
might break some libraries compiled with older headers and create an
unnecessary incompatibility.  Can't we simply add ELOOP with the value
of 39?

Also, the definition of O_NOLINK is in Posix section of fcntl.h.  Is
O_NOLINK defined by Posix?  I doubt that.

     @item 17

    +ELOOP -- Too many levels of symbolic links.  Usually it means
    +encountered link loop (link1 -> link2; link2 -> link1).
    +
    +@item 18
    +
     EMFILE -- Too many open files in system (no more handles available).
     This usually means that the number specified by the @samp{FILES=}
     directive in @file{CONFIG.SYS} is too small.

This is wrong: those numbers in "@item 17" etc. are not ordinal
numbers, they are numerical values of the errno variable.  So ELOOP
should go under "@item 39" and you should leave the existing numbers
intact.

About `lstat': since `_is_executable' always resolves links, doesn't
it mean that the execute bit returned by `lstat' would be incorrect,
because it is taken from the target of a link?

    -  if (mydirname == 0)
    +  if (!__solve_symlinks(mydirname, real_name))
    +     return -1;
    +  _fixpath(real_name, path);
    +
    +  if (path == 0)

Why did you need to add this call to _fixpath in chdir.c?

    +This function always returns zero if the file exists (it does not
    +follow symbolic links), else it returns -1 and sets @var{errno} 
    +to @code{ENOENT}.

`errno' should have the @code markup, not @var.  @var is for
meta-variables, such as formal parameters of a function.  In this
case, `errno' is the actual name of an existing variable, so @var is
inappropriate.  (Among other problems, the argument of @var is
up-cased by makeinfo, so users might think they need to type it in
upper case.)

    +int readlink(const char * filename, char * buffer, size_t size)
    +{
    +   ssize_t      bytes_read = 0;
    +   char         buf[FILENAME_MAX];
    +   int          fd;
    +   struct ffblk file_info;
    +   int          old_errno;
    +   char         data_buf[_SYMLINK_FILE_LEN];
    +
    +   /* Now do the real job */
    +   /* First DJGPP symlink check - is file size a fixed ``magic value''? */
    +   if (findfirst(filename, &file_info, 0) ||
    +      (file_info.ff_fsize != _SYMLINK_FILE_LEN))
    +   {
    +      errno = ENOENT;
    +      return -1;
    +   }

My references indicate that if the argument is not a symlink,
`readlink' should return EINVAL, not ENOENT.  (That's from a SunOS
box; could someone please see what happens on Linux?)  So in the above
clause, errno should be set to EINVAL if the size doesn't fit, but if
`findfirst' failed, you should leave errno intact, as `findfirst' sets
it.

    +   fd = open(filename, O_NOLINK | O_RDONLY | O_BINARY);
    +   if (fd < 0)
    +      return -1; /* errno from open() call */
    +   bytes_read = read(fd, buf, _SYMLINK_PREFIX_LEN);

Isn't it better to call `_open' and `_read' instead, and avoid
unnecessary overhead?

    +@subheading Description
    +MSDOS doesn't support symbolic links but DJGPP emulates them.
    +This function checks if @var{filename} is a DJGPP symlink and
    +the file name that the links points to is copied into buffer,
    +up to maximum @var{size} characters. Only the last file name
    +is resolved.

Please add the @var markup to "buffer" in the second sentence.

I have difficulty understanding the last sentence above.  What does it
mean?

    +   symlink_file = _creat(real_dest, 0);
    +   if (symlink_file < 0)
    +      return -1; /* Return errno from creat() call */
    +   write(symlink_file, _SYMLINK_PREFIX, _SYMLINK_PREFIX_LEN);

The above is from `symlink': isn't it better/faster to use _write
here?

    +      do
    +      {
    +         bytes_copied = readlink(real_path, fn_buf, FILENAME_MAX);
    +         if (bytes_copied != -1)
    +         {
    +            link_level++;
    +            fn_buf[bytes_copied] = '\0';
    +            strcpy(real_path, fn_buf);

Your docs for `readlink' says that if its return value is equal to its
last argument, the caller should enlarge the buffer and try again.
And yet in the above fragment you don't do anything about that case.

    +This function fully resolves given symlink in @var{symlink_path}---
    +all path components and all symlink levels are resolved. The
    +returned path in @var{real_path} is guaranteed to be symlink-clean
    +and understandable by DOS. If @var{symlink_path} does not contain
    +symlinks at all, it is simply copied to @var{real_path}.
    +@subheading Return Value

Please leave a blank line before each @subheading.

A general comment about the *.txh files: please use two spaces between
sentences.  Also, don't break a line after "---", as in the following
fragment:

    +This function fully resolves given symlink in @var{symlink_path}---
    +all path components and all symlink levels are resolved. The

Instead, write the next word after "---" without any intervening
whitespace, like this:

    ...resolves given symlink in @var{symlink_path}---all path...

Btw, do we need an FSEXT value for `readlink' and `symlink'?

Last, but not least: please provide an entry for wc204.txi and short
test programs for the new functions to be included in djtst.