Mail Archives: djgpp-workers/1999/12/07/06:09:40
Some comments, based on reading the patches.
> diff -u -r -d -N djgpp.old/src/libc/posix/sys/stat/is_exec.c djgpp/src/libc/posix/sys/stat/is_exec.c
> --- djgpp.old/src/libc/posix/sys/stat/is_exec.c Sat Mar 20 22:59:52 1999
> +++ djgpp/src/libc/posix/sys/stat/is_exec.c Fri Dec 3 19:12:20 1999
> @@ -1,3 +1,4 @@
> +/* Copyright (C) 1999 DJ Delorie, see COPYING.DJ for details */
> /* Copyright (C) 1996 DJ Delorie, see COPYING.DJ for details */
> /* Copyright (C) 1995 DJ Delorie, see COPYING.DJ for details */
> /* IS_EXEC.C
> @@ -24,8 +25,10 @@
> #include <dpmi.h>
> #include <go32.h>
> #include <io.h>
> +#include <fcntl.h>
> #include <libc/farptrgs.h>
> #include <libc/dosio.h>
> +#include <sys/stat.h>
>
> extern unsigned short _djstat_flags;
> unsigned short _get_magic(const char *, int);
> @@ -50,7 +53,7 @@
> if (s)
> {
> int handle;
> - if((handle = _open(s,0)) == -1)
> + if((handle = open(s, O_RDONLY)) == -1)
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.
> int access(const char *fn, int flags)
> {
> - int attr = _chmod(fn, 0);
> + int attr;
> + char file_name[FILENAME_MAX];
> +
> + if (strlen(fn) > FILENAME_MAX - 1)
> + {
> + errno = ENAMETOOLONG;
> + return -1;
> + }
> +
> + /* LB: symlink support here. */
> + if (!_solve_symlinks(fn, file_name))
> + return -1;
> +
> + attr = _chmod(file_name, 0);
It seems like many functions do this test before calling
_solve_symlinks. I suggest to move this fragment into _solve_symlinks
itself.
> +++ djgpp/src/libc/posix/unistd/readlink.c Wed Dec 1 19:50:32 1999
[snip]
> + if (findfirst(filename, &file_info, 0) ||
> + (file_info.ff_fsize != _SYMLINK_FILE_LEN))
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.
> + 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?
> +It is _not_ terminated @code{'\0'}
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.
> +error and errno is set. When value returned is equal to @var{size},
Please use @code{errno}, since it's a C identifier.
> +name. So increase size and try again.
^^^^
Should be @var{size}.
> --- djgpp.old/src/libc/posix/unistd/symlink.c Wed Aug 25 11:24:48 1999
> +++ djgpp/src/libc/posix/unistd/symlink.c Thu Dec 2 17:27:44 1999
> @@ -1,180 +1,51 @@
> +/* Copyright (C) 1999 DJ Delorie, see COPYING.DJ for details */
> /* Copyright (C) 1997 DJ Delorie, see COPYING.DJ for details */
> -#include <libc/stubs.h>
You need to keep <libc/stubs.h> here, since you call non-ANSI
functions.
> + 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);
> + write(symlink_file, source, strlen(source));
> + write(symlink_file, "\n", 1);
> + write(symlink_file, fill_buf, _SYMLINK_FILE_LEN - _SYMLINK_PREFIX_LEN - strlen(source) - 1);
> + close(symlink_file);
Since you use _creat, I suggest to use _close.
> +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.
> +++ djgpp/src/libc/posix/unistd/xsymlink.c Sat Dec 4 10:07:26 1999
> @@ -0,0 +1,80 @@
> +/* Copyright (C) 1999 DJ Delorie, see COPYING.DJ for details */
> +
> +/* Written by Laurynas Biveinis */
> +/* Internal source file specifying DJGPP symlink prefix and internal */
> +/* function which fully resolves given symlink. (Function readlink() */
> +/* resolves only last filename component and one symlink level.) */
> +
This needs to include <libc/stubs.h>.
> +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.
> + if (((bytes_copied > 2) && (real_path[1] == ':')) ||
> + ((bytes_copied > 0) && ((real_path[0] == '/') || (real_path[0] == '\\'))))
> + {
> + found_absolute_path = 1;
> + }
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.
Please add documentation for the _solve_symlinks function.
- Raw text -