Mail Archives: djgpp-workers/1999/12/08/13:39:39
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
- Raw text -