Mail Archives: djgpp-workers/2000/12/12/21:20:29
At 11:30 AM 12/12/00 +0200, Eli Zaretskii wrote:
>On Mon, 11 Dec 2000, Peter J. Farley III wrote:
>> OK, Real Soon Now has arrived. I think I have incorporated all of
>> the suggestions and corrections that were made.
>
>Thanks!
>
>> In addition, after
>> these patches are accepted and applied, someone needs to test the
>> FAT32 functions with real 32-bit offsets on a file whose size is
>> 2GB.
>
>Actually, I would be glad if this were tested _before_ the changes
are
>committed, if someone with access to a FAT32 machine can do that. If
>not, we will have to keep our fingers crossed...
I would feel better about that as well, but see my next answer.
>> All coded tests just use zero offsets.
>
>Isn't it better to try non-zero offsets as well? You could get them
>from `rand', for example.
>
>That would also allow an easy test on FAT32 systems, if you probe for
>FAT32 support and provide a special test for that, which actually
>tries to lock after 2GB.
Except we really need to create (say) a 3GB file to test that the code
correctly performs the function, don't we? And I don't have 3GB disk
space to spare at the moment.
However, I can probably put in a few multi-region tests by hand without
too much trouble. I was just trying to get this magilla off my chest
sooner rather than later. I'll see what I can work up. Dividing the
file length by 4 and locking/unlocking each quarter of the file opened
should be enough of a test, right?
>> +struct _DOSERROR_STR {
>> + char *exterror_str;
>> + #ifdef __cplusplus
>> + char *errclass_str;
>> + #else
>> + char *class_str;
>> + #endif
>> + char *action_str;
>> + char *locus_str;
>
>Why do we need the special __cplusplus case? Can't we always call
>that member errclass_str (and rename the other two to erraction_str
>and errlocus_str)?
Well, I just copied the existing struct flock, which has that identical
check for C++ mode. I am presuming that "class" is a reserved word in
C++, so using "class" in the original struct won't work if it's being
used in C++, hence it provides an alternate structure member name.
I have no problem just using "errclass_str", "erraction_str" and
"errlocus_str", if that's what seems to work better.
<Snipped>
>I think the following should do what you want (but I didn't test
>this):
<Code snipped>
OK, I'll give it a try after all the other changes are done. I want to
get something that folk can really test out first, and worry about the
tough cases second.
>> + case F_TEST:
>> + lock_req.l_type = F_WRLCK;
>> + /* Test the lock: return 0 if FD is unlocked;
>> + return -1, set errno to EACCES, if the lock cannot be
obtained. */
>> + if (fcntl (_fildes, F_GETLK64, &lock_req) < 0)
>> + return -1;
>> + if (lock_req.l_type == F_UNLCK)
>> + return 0;
>> + errno = EACCES;
>> + return -1;
>
>Some systems return EAGAIN if the region is already locked. I'm not
>sure what does the majority of systems do; could people please look
on
>the Unix and GNU/Linux boxes they have access to and tell what they
>see in "man lockf"?
I'll go with the majority usage, assuming we can find it out. FWIW,
the man page on Debian/GNU systems says it returns "EACCES or EAGAIN"
and doesn't tell you which is given under what circumstances.
>> +The @code{llockf} command is a simplified interface to the locking
>> +facilities of @code{fcntl} (see @ref{fcntl} for more detailed
>> +information).
>
>"@ref{fcntl}" should be followed by a comma or a period (comma in
this
>case). Didn't makeinfo print a warning?
Nope, no warning that I saw. Just a count of the number of nodes. But
I'll change it anyway.
>> +The @code{llockf} command performs exactly the same functions as
>> +@code{lockf}
>
>Please add an @xref to lockf here.
OK.
>> +The @code{llockf} command is intended to permit using @code{fcntl}
>> +functions with @var{size} values greater than 2^31 - 1.
>
>It is better to use @math{2^31 - 1}, in case someone prints the libc
>reference: it looks nicer.
OK.
>> +@var{size} is the number of contiguous bytes to be locked or
>unlocked.
>> +The resource to be locked starts at the current offset in the file
>and
>> +extends forward for a positive size and backward for a negative
size
> ^^^^
^^^^
>These two occurences of "size" should have the @var markup, since you
>are talking about the function's argument.
OK.
>> +(the preceding bytes up to but not including the current
offset).
>If
>> +size is zero
> ^^^^
>And also here.
OK.
>> +@table @code
>> +@item F_TEST
>> +This function is used to detect if the specified section is
already
>> +locked. @code{F_LOCK} and @code{F_TLOCK} both lock a section of a
>file
>> +if the section is available. @code{F_ULOCK} removes locks from a
>> +section of the file.
>
>The last two sentences don't belong to the description of F_TEST.
No, they certainly don't.
>> +@item F_LOCK
>> +@item F_TLOCK
>
>In a @table, all @item's but the first should be @itemx.
OK.
>> +The @code{llockf} utility will fail
>
>llockf is not a utility.
OK. Use "command" instead, right?
>> if one or more of the following are
>> +true:
>> +
>> +@table @code
>> +@item EBADF
>> +Parameter @var{fildes} is not a valid open file.
>> +
>> +@item EACCES
>> +Cmd is @code{F_TLOCK} or @code{F_TEST} and the section is already
>> +locked.
>> +
>> +@item EINVAL
>> +Parameter @var{function} is not one of the implemented functions.
>> +@end table
>
>This doesn't make clear that EBADF etc. are the values of `errno'.
>(The last tow comments pertain to both llockf and lockf's docs.)
OK. I'll make it clearer.
>> +This function accepts the extended error structure from DOS (e.g.,
>from
>> +the returned parameter from function @code{dosexterr}, see
>> +@ref{dosexterr}) and returns
>
>Need a comma after @ref{}.
OK.
>> +structure in the structure pointed to by the second
parameter. This
>> +function is a DOS analogue of the ANSI function @ref{strerror},
>
>This use of @ref{} is one of the bad ones I wrote in reply to your
>previous patches.
Yes, it is. I'll fix it.
>> +/* B4h (180) */ "(MS-DOS 7.0) Lock count has been exceeded",
>> +/* B4h (180) */ "(NetWare4) Invalid segment number",
>
>One of these two strings should not appear: they are for the same
>code. The first one should be retained, I think (we don't care about
>NetWare).
Missed that one. I merged several other "multiple version" messages to
put out both as one message, so I'll do the same for this one.
>> +/* B5h (181) */ "(MS-DOS 7.0) A valid eject request failed",
>> +/* B5h (181) */ "(DOS 5.0-6.0,NetWare4) Invalid call gate",
>
>Same here.
And that one.
>> +/* FEh (254) */ "Reserved",
>
>Should all those "Reserved" be replaced by "Unknow error: XX"?
I'm *not* going to manually code all those items as unknown. But I
*can* easily code the function to replace "Reserved" with the "Unknown"
message. Will that do?
>> + /* If l_len is zero, then the lock is to be set from l_start
>> + until the end-of-file. */
>> + if (len == 0)
>> + {
>> + len = filelength(fd) - cur_pos;
>> + /* This should probably be an error. */
>> + if (len <= 0)
>> + len = 1;
>> + }
>
>Perhaps the case where filelength returns a negative value (can it?)
>should indeed return an error, but if filelength returns zero
(meaning
>the file is empty), it should succeed, I think, because fcntl allows
>to lock regions beyond the current EOF. The question is: does
>DOS/Windows allow that? Otherwise, it is not clear to me why the
>above snippet bumps len to 1.
Because that's the way Mark originally coded it. I didn't change it at
all from his version.
If file position has been set beyond EOF (does [l]lseek allow that?),
you will get negative values. If file position has been set *at* EOF,
you will get zero, but a zero length is not, I think, a valid length
argument to the interrupt, which is why I suspect Mark coded it that
way.
Let me review that code again. Maybe we just need to interchange the
position and abs(length) values when position is beyond EOF to get the
correct result. The zero case is a problem, I think.
More after I have looked at it more carefully.
>> + len = filelength(fd) - cur_pos;
>> + /* This should probably be an error. */
>> + if (len <= 0L)
>> + len = 1L;
>
>Does filelength in the current CVS support FAT32?
No idea. I didn't even think to check.
<Wide example snipped>
>This should use @smallexample (its lines are too long for @example).
OK.
>> +A file is locked, not the file descriptor. So, dup (2) does not
>create
>> +multiple instances of a lock.
>
>"dup" should have a @code markup, and please also add an
@xref. Also,
>we don't use the (n) notation for commands and functions (because the
>references aren't man pages), so please remove "(2)" in the above.
OK. That text was copied from a man page, so I'll fix it as you
suggest.
>> +Dos/Win9x does not support shared locks, but the underlying
>> +implementation (which uses the @code{F_SETLK} (non-blocking) or
>> +@code{F_SETLKW} (blocking) commands to @code{fcntl}, see
>@ref{fcntl})
>
>This should use "@pxref", not "see @ref", because the latter needs a
>period or a comma after it.
Could I replace all of the erroneous @ref{}'s I have used without
commas or periods with @pxref{}'s to avoid that problem?
>Last, but not least: thanks for all the work!
Labors of love are a *lot* more work than they ever appear at the
start... :)
At DJ's suggestion, I will also break this monster up into more
manageable pieces so I don't overload his server, and (more
importantly) to make the pieces more digestible by the folk on the
list.
Corrections in progress, more later.
---------------------------------------------------------
Peter J. Farley III (pjfarley AT dorsai DOT org OR
pjfarley AT banet DOT net)
- Raw text -