Message-ID: <384EA0DA.5C05DD67@softhome.net>
Date: Wed, 08 Dec 1999 20:18:02 +0200
From: Laurynas Biveinis <lauras AT softhome DOT net>
X-Mailer: Mozilla 4.7 [en] (Win98; I)
X-Accept-Language: en
MIME-Version: 1.0
To: Eli Zaretskii <eliz AT is DOT elta DOT co DOT il>
CC: DJGPP Workers <djgpp-workers AT delorie DOT com>
Subject: Re: Second symlink patch
References: <Pine DOT SUN DOT 3 DOT 91 DOT 991207125819 DOT 29997A-100000 AT is>
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Reply-To: djgpp-workers AT delorie DOT com

Eli Zaretskii wrote:
> 
> Some comments, based on reading the patches.

Thanks very much for feedback.

> > diff -u -r -d -N djgpp.old/src/libc/posix/sys/stat/is_exec.c djgpp/src/libc/posix/sys/stat/is_exec.c
> Here, you need to open the file in O_BINARY mode, since that is what
> _open does (you are going to read a possibly binary file).
> 
> Btw, it seems easier to simply call _solve_symlinks and leave the
> _open call alone.

OK, I did it your way.

> > +  if (strlen(fn) > FILENAME_MAX - 1)
> > +  {
> > +     errno = ENAMETOOLONG;
> > +     return -1;
> > +  }

> It seems like many functions do this test before calling
> _solve_symlinks.  I suggest to move this fragment into _solve_symlinks
> itself.

Moved. Also added a check for NULL pointers passed as args.

> Here, you need to include <libc/stubs.h>, so that non-ANSI and
> non-Posix functions like findfirst and open don't pollute the
> namespace, like in the fragment above.
> 
> Also, readlink needs to be added to the stubs list, since ANSI and
> Posix functions call it, directly or indirectly.

So I added a #define readlink __readlink in <libc/stubs.h> under
section ``DJGPP functions'' and #include <libc/stubs.h> to the top
of readlink.c. Is it correct? (I haven't dealed with it before)

> > +   for (buf_ptr = buffer; (read_now > 0) && ((unsigned)bytes_read < size); buf_ptr++)
> > +   {
> > +       read_now = read(fd, &byte, 1);
> 
> Isn't it better to read the whole _SYMLINK_FILE_LEN worth of bytes in
> one swell whoop and then walk the buffer, instead of reading the link
> byte by byte?

Clearly an oversight. Rewrote that piece, now it looks many times better.

> Please use either @emph{not} or @strong{not}.  The former produces
> "_not_" in the Info output, but I generally find @strong{} be better
> in the printed output.

> Please use @code{errno}, since it's a C identifier.

> Should be @var{size}.

Everything corrected as suggested. 

> You need to keep <libc/stubs.h> here, since you call non-ANSI
> functions.

OK.

> > +   symlink_file = _creat(real_dest, 0);
> > +   close(symlink_file);
> 
> Since you use _creat, I suggest to use _close.

OK.

> > +MSDOS doesn't support symbolic links.  However, DJGPP has two ways for
> > +supporting them - it supports ``symlinks'' to DJGPP programs so they're
> > +accessible from plain DOS and emulates symlinks to everything else,
> > +however only DJGPP programs are able to use the.  This function simulates
> > +a symlink between two @file{.exe} files by creating a program whose name
> > +is pointed to by @var{new} which, when run, will actually execute the
> > +program @var{exists} passing it the string pointed by @var{new} in
> > +@code{argv[0]} (some programs change their behavior depending on what's
> > +passed in @code{argv[0]}). The file referred to by @var{exists} doesn't
> > +really have to exist when this function is called. If @var{exists} points
> > +to an @emph{existing} file, the function checks that it is a DJGPP
> > +executable;
> 
> This seems to imply that a symlink is only between two .exe files.
> The second sentence does say that other files are supported, but the
> rest of the text only talks about programs.  I suggest to rewrite this
> fragment.

Surely. It was written when I thought that I would only support symlinks to
non exes, and leave current exe symlinks as they are.

> > +++ djgpp/src/libc/posix/unistd/xsymlink.c    Sat Dec  4 10:07:26 1999
> This needs to include <libc/stubs.h>.

Included.

> > +char _SYMLINK_PREFIX[] = "!<symlink>";
> > +
> > +unsigned _SYMLINK_PREFIX_LEN = 10; /* strlen(_SYMLINK_PREFIX) */
> 
> It is better to use "sizeof(_SYMLINK_PREFIX) - 1", so that it changes
> automatically if the prefix is changed.

Changed. How could I skip such things?

> This might be incompatible with the proposed chroot changes, since the
> root might be simulated.  Please put a FIXME comment here, so that
> this is tested in due time.

What changes? I'm on this mailing list since April or so, but can't 
recall anything. Also quick search through my mailbox gived nothing.

> Please add documentation for the _solve_symlinks function.

Will do.

Thanks,

Laurynas Biveinis