Message-Id: <5.0.1.4.0.20001127210039.034a5360@pop5.banet.net> X-Sender: usbanet DOT farley3 AT pop5 DOT banet DOT net X-Mailer: QUALCOMM Windows Eudora Version 5.0.1 Date: Mon, 27 Nov 2000 23:21:14 -0500 To: Eli Zaretskii From: "Peter J. Farley III" Subject: Re: Locking fcntl() and flock() patches Cc: djgpp-workers AT delorie DOT com In-Reply-To: <5.0.1.4.0.20001127203612.00a3d2a0@pop5.banet.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; format=flowed 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 At 08:38 PM 11/27/00 -0500, you wrote: >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. Thanks for the feedback. > > 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! You're welcome. It has been more work than I anticipated (of course), but rewarding to the mind. >Please don't be discouraged by the comments below: they are mostly >minor technicalities. Whew! But there sure are a lot of them... I will try to keep my responses brief. Let me say ahead of time, Thank You for all of it, so I am not constantly repeating myself. > > 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. I will defer to anyone else who wishes to implement this as a wrapper, building on these changes after they're committed. >(Btw, the patch you sent does include wc204 changes that tell lockf >_is_ available.) Oops. Removed everything else, forgot that. > > +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{}. Got it. > > +@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? OK, I will convert that to a table. And the "=" is a typo, it should have been "==". Copied from the comment in the code, which is also a typo. > > +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 > >In Texinfo, there's no need to use CAPS for emphasis; use >@strong{unless} instead. OK. My lack of texinfo knowledge is obviously showing. > > +@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). Ditto. > > +@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. And again, ditto. > > +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). OK. > > -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. My ignorance. I did not know that this was the way it should be. I will change it back. > > -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 ;-). OK :) And this is probably where I got the bad idea for the other change. >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). OK, I'll try to follow those instructions. > > +@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: OK, I was just copying the actual code there. >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. Yes, but in the sense that they return a string in response to an error code, they are an analogue, IMHO. However, I can see your point. I have no real problem changing the class/action/locus descriptions to say something like "... returns a string describing the ..., similar to the way strerror returns a string for values of @code{errno}...". Does that sound OK? >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. Well, I used the comments in D_EXTERR.C as the source. I will try to find compatible text in RBIL 61. >Same here: there are two more entries in RBIL v61. Ditto. > > +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. Well, this is an exact copy of the code for strerror(), so I guess that needs fixing too. >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? I actually considered setting up a new struct with four char pointers as members, but I didn't want to introduce more strangeness than I was already doing. I thought staying with the strerror model would be more familiar, and thus make it easier to remember how to use it. Introducing a struct forces looking up the names of the members of the struct to get at the string values. OTOH, if someone has bothered to look up how to call dosexterr(), maybe it's not so much to ask to use another struct to get the strings describing the errors, especially if the member names are similar to the ones returned by dosexterr(). For example, struct DOSERROR_STR { char *exterror_str; char *class_str; char *action_str; char *locus_str; }; Does anyone else have thoughts or input on this? > > +/* Changed 11/25/2000, 21h (33) EPERM -> EACCES */ > > +/* 21h (33) lock violation needs EACCES errno in fcntl */ > > +/* Peter J. Farley III >There's no need for these comments: that's what CVS is for. If you >think it's important to mention why 21h is mapped to EACCES, please >put a comment near the code which does that, saying that fcntl needs >it. Since I don't have access to CVS, nor have I ever used it, so I didn't know that. My practice has always been to comment what I change where I change it. I put it at the front because the change is in the initialization of the array which immediately follows. However, if CVS handles this function, I can remove the comment. >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. This is Mark E.'s original code, I did not change it. I can make the changes you suggest. > > + /* 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.) Again, this is Mark E.'s code, I have *not* done the research to have this knowledge. However, if what you are saying is that the value needs to be tested for x'FF' and to return EBADF if equal, that can be done. > > + 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? I saw that Mark E. already replied about this agreed that it should say "can" and not "can't". >This should check if cur_pos is -1, and if so, bail out. This might >happen, e.g., if fd is invalid. Agreed. More of Mark's code that I just left alone. > > + 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. Ditto, and ditto. And I already saw the subsequent comments about using llseek() and Offset_t, so I'll change the calls and variables appropriately. That will also mean introducing some intermediate variables, since I don't think we should change the types of the members of the struct flock. > > + /* 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? The first part (the part you don't understand) is Mark's comment, not mine. I took it as truth and did not change it. What I added (from UNLESS onwards) was a simple allowance for what the comment describes, i.e., allow a read lock anyway if it covers the whole file. I did not think of the other ways you can "cover the whole file", only the simplest and most direct one. What I do *not* know is whether the *first* part of the comment is true or false. RBIL's info on int21/5C do not seem to indicate either way whether it is true or not, so I left it as it was. How would one go about determining whether an arbitrary combination "covered the whole file"? Would one use f_tell() to get the file position or some internal value/function saying where the file is positioned? Would one use filelength() to determine whether the file was at it's end position? More importantly, is Mark's comment true? If it is not, none of this code is needed. I don't know the answer to that question. > > @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 ;-). Hmm. Agreed, I worded that poorly (trust a programmer not to be a wordsmith). I'll give it another try. > > @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. OK. > > +@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. I copied most of this (with changes) from a man page I had. You're right, it needs to be changed for DOS. >Thanks again for your efforts And thank you for your diligence in reviewing it all. Forgive me in advance if I don't catch everything in pass #2. Hopefully, all will be right by pass #3. Right, back to work now... --------------------------------------------------------- Peter J. Farley III (pjfarley AT dorsai DOT org OR pjfarley AT banet DOT net)