Mail Archives: djgpp-workers/2000/06/14/10:19:10.1
Eli Zaretskii wrote:
>
> 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.)
Oh, I really have lost the count :) I though it was 7th.
> -#define ENMFILE 38
> +#define ELOOP 38
> +#define ENMFILE 39
>
> Why is it a good idea to change the numerical value of ENMFILE? It
No. It seems that I've thought of 38/39 like about meaningless enumeration.
> might break some libraries compiled with older headers and create an
> unnecessary incompatibility. Can't we simply add ELOOP with the value
> of 39?
OK.
> Also, the definition of O_NOLINK is in Posix section of fcntl.h. Is
> O_NOLINK defined by Posix? I doubt that.
I've just looked up in glibc-2.1.3 docs and they state that O_NOLINK is
a GNU extension. Will correct 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.
As I understand texinfo, @item specifies just a label in a table layout
and the real EWHATEVER value does not matter there, does it?
> 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?
I was 110% sure that I've included missing S_ISLNK *this time*.
Guess if it is really so. But the real code sets all bits correctly, it
looks like this:
if (readlink(......
{
statbuf->st_mode |= S_ISLNK;
...
}
else
{
... _is_executable and so on.
}
>
> - 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?
I think it is remaining from the ancient symlink times when
chdir() was keeping directory name for getcwd() to return
symlink instead of real dir. (Remember my 1st patch?).
This is obsolete now and will be removed.
> +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.)
It will be corrected.
> +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.
Again glibc doc proves your point. BTW, I think I will extract that
code into __is_symlink() if I haven't done it before and use it,
because I may have written if ((readlink(...) == -1) && (errno = ENOENT))
to check if given file is a symlink.
>
> + 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?
OK.
>
> +@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.
OK.
> I have difficulty understanding the last sentence above. What does it
> mean?
Since the symlink file itself won't be checked for symlinks, this means
that readlink("c:/dir/link.to.dir/link"...) will fail because it won't
resolve 'link.to.dir'. I will try to express it better.
>
> + 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?
OK.
>
> + 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.
Note that I'm already asking for FILENAME_MAX filename length.
I think I will do something like
char fn_buf[FILENAME_MAX + 1];
...
do
{
bytes_copied = readlink(real_path, fn_buf, FILENAME_MAX + 1);
if (bytes_copied == FILENAME_MAX + 1)
{
errno = ENAMETOOLONG;
return -1;
}
...
>
> +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.
OK.
> 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:
OK.
> Btw, do we need an FSEXT value for `readlink' and `symlink'?
I can't think right now why application could want to hook those
functions.
> Last, but not least: please provide an entry for wc204.txi and short
I will provide it when actual symlink integration starts.
> test programs for the new functions to be included in djtst.
Oh yes, I should have quite a few. I think it was the last
full patch, from now on I will post indvidual pieces for review
before commiting.
Laurynas
- Raw text -