Date: Mon, 27 Nov 2000 13:44:20 +0200 (IST) From: Eli Zaretskii X-Sender: eliz AT is To: "Peter J. Farley III" cc: djgpp-workers AT delorie DOT com Subject: Re: Locking fcntl() and flock() patches In-Reply-To: <5.0.1.4.0.20001127000014.00a46b50@pop5.banet.net> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Reply-To: djgpp-workers AT delorie DOT com Errors-To: nobody AT delorie DOT com X-Mailing-List: djgpp-workers AT delorie DOT com X-Unsubscribes-To: listserv AT delorie DOT com Precedence: bulk On Mon, 27 Nov 2000, Peter J. Farley III wrote: > Attached are three diff files which I *think* should come through as > text and not mime-encoded, but *please* let me know how they reach you. They seem to be received okay. > src.dif contains all the patches for the /src tree, include.dif > contains all the patches for the include tree (including the file I > missed the last time, /include/libc/getdinfo.h) and tests.dif contains > all the patches for the /tests tree. Thanks! Please don't be discouraged by the comments below: they are mostly minor technicalities. > If anyone here thinks we should just implement it anyway, I'm prepared > to hear arguments in favor, but I won't be able to do a table-based > implementation myself, as my copious spare time :) is severely > depleted. Perhaps it would be a good idea to implement lockf as a simple wrapper around fcntl. We could add the tables later, if they are needed. Unless programs which use lockf *assume* that the tables are available and *rely* on them, that is. (Btw, the patch you sent does include wc204 changes that tell lockf _is_ available.) > +v2.03) now work, with two exceptions: (1) command F_GETLK will not Preprocessor symbols such as F_GETLK should have a @code markup... > +return values in the 'struct flock' parameter identifying what lock ...and so should "struct flock". In general, anything that is put into a source code verbatim should be in @code{}. > +@itemize @plus{} > +@item > +lock_req->l_whence = SEEK_SET > +@item > +lock_req->l_start == 0 > +@item > +lock_req->l_len == 0 > +@end itemize I think "@table @code" would be better here. Also, why didn't you use "==" in the first condition as well? > +This is because DOS/Windows only supports read locks on a per-file > +basis, so any attempted use of a read lock is an error, UNLESS all of In Texinfo, there's no need to use CAPS for emphasis; use @strong{unless} instead. > +@cindex fcntl commands F_GETFD and F_SETFD Please add @code markup to F_SETFD and friends in @cindex entries also (@findex and @vindex entries already have an implicit @code markup). > +@findex fcntl commands F_GETFL and F_SETFL The @findex entries which combine C symbols and plain text should arrange for the plain text to be in the normal typeface, like this: @findex fcntl AT r{ commands }F_GETFL AT r{ and }F_SETFL @r{} causes an escape into the normal Roman typeface. > +these functions are still effective no-ops, since DOS/Windows does not > +allow flags to be modified on an open file, except for F_SETFL to remove > +the O_NONBLOCK flag, which is not supported by DJGPP anyway. > + > +@item > +Documentation has been updated to reflect the new functionality and a > +test program has been provided. > +@end itemize This piece of docs is too detailed for wc204.txi, which should only have short descriptions of each change. Please move these details into fcntl.txh (which doesn't mention some of the information you put in wcfcntl.txi). > -int dosexterr(struct DOSERROR *p_error); > +int dosexterr(struct _DOSERROR *p_error); Why this change? `dosexterr' is a compatibility function: it mimics its Borland namesake, which uses DOSERROR, not _DOSERROR. Using _DOSERROR would be incompatible. > -void main(void) > +int main(void) > @{ > FILE *fp; > struct _DOSERROR de; This should be changed to DOSERROR, for the same reason (yes, I know: it's not your bug ;-). > +This function accepts the extended error number from DOS (e.g., from > +the return value from function @ref{dosexterr} or from the value of > +@ref{_doserrno} after a failed DOS call) Please don't use @ref{} like this. It might appear as a nifty trick (you combine a cross-reference with the text), but it looks awkward in Info format: "...from the value of *Note {_doserrno} after a failed DOS call" and simply unreadable in the printed output: "...from the value of Section 2.10 [_doserrno], page 123, after a failed DOS call" Also, you need a comma or a period after @ref{}, or else some Info browsers will become confused. Here's how I suggest to write such fragments: This function accepts the extended error number from DOS (e.g., from the return value from function @ref{dosexterr} or from the value of @code{_doserrno}, see @ref{_doserrno}, after a failed DOS call) > +For a list of the strings returned for each error number, see the > +list at @ref{dosexterr}. Same here (and in a few more places). > +@example > +#include > + > +char * > +dostrcls(doserrclass); > +@end example Please don't put a newline between the function return type and its name in .txh files. Instead, write this: @example char *dostrcls (doserrclass); @end example > +This function accepts the extended error class number from DOS (i.e., > +from the returned error class value from function @ref{dosexterr} after > +a failed DOS call) and returns the string which describes that error > +class number. This function is a DOS analogue of the ANSI function > +@ref{strerror}, and can be used to print a descriptive message > +corresponding to the error class. I'm not sure you can say that dostrcls is an analogue of strerror. dostrerr is such an analogue, but dostrcls has no direct analogue; it's an extension. The same pertains to dostract and dostrloc. > +const char *__dos_errlist [] = { > +/* 00h (0) */ "No error", > +/* 01h (1) */ "Function number invalid", [snip] > +/* 67h (103)*/ "(MSCDEX) Not High Sierra or ISO-9660 format", > +/* 68h (104)*/ "(MSCDEX) Door open" > +/* (End of list) */ }; This list is incomplete; there are many more error codes, all the way to FFh. They are all present in version 61 of RBIL, which I suggest to use as the source. > +const char *__dos_errclass [] = { > +/* 01h (01) */ "Out of resource (storage space or I/O channels)", > +/* 02h (02) */ "Temporary situation (file or record lock)", > +/* 03h (03) */ "Authorization (denied access)", > +/* 04h (04) */ "Internal (system software bug)", > +/* 05h (05) */ "Hardware failure", > +/* 06h (06) */ "System failure (configuration file missing or incorrect)", > +/* 07h (07) */ "Application program error", > +/* 08h (08) */ "Not found", > +/* 09h (09) */ "Bad format", > +/* 0Ah (10) */ "Locked", > +/* 0Bh (11) */ "Media error", > +/* 0Ch (12) */ "Already exists", > +/* 0Dh (13) */ "Unknown" > +/* (End of list) */ }; Same here: there are two more entries in RBIL v61. > +static char * > +_err_unknown(int errnum) > +{ > + static char ebuf[40]; /* 64-bit number + slop */ > + char *cp; > + int v=1000000, lz=0; > + > + strcpy(ebuf, "Unknown error: "); > + cp = ebuf + 15; Please avoid magic constants such as 15. Instead, make the string "Unknown error: " be a const char array and use sizeof. This prevents possible bugs due to miscounts, and also automagically DTRT if the constant string is ever modified. > +dostrerr(int errnum) > +{ > + if (errnum >= 0 && errnum < __dos_nerr) > + return(unconst(__dos_errlist[errnum], char *)); > + else > + return(_err_unknown(errnum)); > +} > + > +char * > +dostrcls(int errnum) > +{ > + if (errnum >= 0 && errnum < __dos_ncls) > + return(unconst(__dos_errclass[errnum], char *)); > + else > + return(_err_unknown(errnum)); > +} I wonder whether it will be a better design to return all this info as a single struct, instead of having 4 separate functions. It strikes me that whoever wants the human-redable description of what _dosexterr returns, will want all of the info, not parts of it. It also seems to be more consistent with _dosexterr itself, which returns all of the codes at once. Thoughts? > +/* Changed 11/25/2000, 21h (33) EPERM -> EACCES */ > +/* 21h (33) lock violation needs EACCES errno in fcntl */ > +/* Peter J. Farley III + /* Get the JFT entry address for this handle. */ > + regs.x.ax = 0x1220; > regs.x.bx = fd; > - __dpmi_int(0x21, ®s); > + __dpmi_int(0x2f, ®s); > + > + > if (regs.x.flags & 1) > + { > + errno = ENOSYS; I don't think it's right to set errno to ENOSYS here. This call is supported by DOS 3.0 and later, so ENOSYS is inappropriate (since DJGPP doesn't work on DOS versions before 3). RBIL says that 2F1220 can return 6 in AL (invalid file handle), which should be mapped to EBADF. Simply use __doserr_to_errno, it will DTRT. There are several more places where I think you need to make such a change. > + /* Get the SFT entry number for this handle. */ > + es = regs.x.es; > + di = regs.x.di; > + index = _farpeekb(_dos_ds, es * 16 + di); > > - return 1; > + > + /* Now get the address of the entry. */ > + regs.x.ax = 0x1216; > + regs.x.bx = index; > + __dpmi_int (0x2f, ®s); Careful: if index is 255 (FFh), it means the file handle is not open (see RBIL), and you should return EBADF in errno instead of feeding 2F1216 with an invalid index. (It is true that 2F1216 is documented to fail if the index is grater than what FILES= says, but we are dealing with undocumented internal OS calls, so playing safe is always a good idea.) > + new_dev_info = _get_dev_info(fd); > + > + /* If the dev info words are equal, then the documented > + interface can't be used to set the inheritance bit. */ > + return (new_dev_info == dev_info) ? 1 : 0; The above comment seems to be misleading: acrually, if the info words are equal, the documented interface *can* be used to set inheritance bit. Or am I missing something? > + case F_SETLKW: > + { > + struct flock *lock_req = NULL; /* shut up -Wall */ > + int ret = -1; > + off_t pos, cur_pos, lock_pos; > + off_t len; > + > + > + cur_pos = lseek(fd, 0, SEEK_CUR); This should check if cur_pos is -1, and if so, bail out. This might happen, e.g., if fd is invalid. > + va_start (ap, cmd); > + lock_req = va_arg(ap, struct flock *); > + va_end (ap); > + lock_pos = lseek (fd, lock_req->l_start, lock_req->l_whence); lock_pos can also be -1 (if the file is a device, for example). > + lseek (fd, cur_pos, SEEK_SET); This doesn't check for errors, but should IMHO. > + /* DOS/Windows only support read locks on a per-file basis, > + so any attempted use of a read lock is an error, > + UNLESS all of these conditions are true, which makes the > + request into a per-file lock equivalent to F_WRLCK: > + > + lock_req->l_whence = SEEK_SET > + lock_req->l_start == 0 > + lock_req->l_len == 0 > + */ Hmm... I cannot figure out this comment, in particular what do you mean by ``only supports use of read locks on a per-file basis''. It is also not clear why do you insist on l_whence being SEEK_SET: clearly, the same conditions, whatever they are, can be created by a suitably chosen values of an offset with SEEK_CUR and SEEK_END, no? Could you please explain this some more? > @item F_SETFL > Set the open mode and status flags associated with the handle @var{fd}. > -This always fails and sets @code{errno} to @code{ENOSYS}, since DOS and > -Windows don't allow to change the descriptor flags after the file is > -open. > +This fails in all but one case, and sets @code{errno} to @code{ENOSYS}, > +since DOS and Windows don't allow changing the descriptor flags after > +the file is open. The one allowed case is for @code{O_NONBLOCK}, since > +DJGPP doesn't support it anyway. This doesn't explain what is the result when O_NONBLOCK is used. Does the function always succeed or always fail? Let's not ask the user to guess (they will guess wrong ;-). > @item F_SETLKW > Same as @code{F_SETLK}, but if the lock is blocked, the call will wait > -until it is unblocked and the lock can be applied. This is unsupported > -and will always fail. > +(using @code{__dpmi_yield()}) until it is unblocked and the lock can be Please add a cross-reference to __dpmi_yield's docs. > +@item LOCK_SH > +Shared lock. More than one process may hold a shared lock for a given > +file at a given time. > + > +@item LOCK_EX > +Exclusive lock. Only one process may hold an exclusive lock for a given > +file at a given time. But DOS doesn't really distinguish between shared and exclusive locks, does it? If it doesn't, please say so in the docs. Thanks again for your efforts.