Date: Tue, 7 Dec 1999 13:00:07 +0200 (IST) From: Eli Zaretskii X-Sender: eliz AT is To: Laurynas Biveinis cc: djgpp-workers AT delorie DOT com Subject: Re: Second symlink patch In-Reply-To: <384AA280.42326B2@softhome.net> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Reply-To: djgpp-workers AT delorie DOT com X-Mailing-List: djgpp-workers AT delorie DOT com X-Unsubscribes-To: listserv AT delorie DOT com Precedence: bulk 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 > #include > #include > +#include > #include > #include > +#include > > 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 , 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 You need to keep 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 . > +char _SYMLINK_PREFIX[] = "!"; > + > +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.