This attached patch should take care of the _close_r() problem. As you
suggested, I extended FD_CHECK to take 2 arguments.
Regarding protecthing the _fdtab[] contents... Locking the section
completely during read/write operations will serialize IO of any MT
program. As it now happens with _write_r(), only one thread can write
to any file at a time. That doesn't sound right.
I see three potential ways out of this:
1. In the beginning of a function, lock the section, copy the contents
of _fdtab[fd] to a local variable, and release the lock. It doesn't
look like any IO involved methods change the contents of the fd
structure, but I haven't really looked at what happens to the "cxt",
and whether we need to worry about applications that try to access the
same fd simultaneously from multiple threads. There seems to be no
POSIX statements about MT safety of read()/write() system calls.
2. add critical section structure as a member of _fdent_s structure.
Here, I'm not sure how big the structure actually is in memory, and
whether it is going present significant memory consumption, and
whether there are any other vital resources that are allocated for the
initialized critical section, but MSDN says that
InitializeCriticalSection may return OUT_OF_MEMORY, so there must be
some other resources that are being allocated, and this may not turn
out to be feasible.
3. Why do we care about locking? :) It seems that if an application is
careless enough to execute IO operations on the same file descriptor
simultaneously from multiple threads, it's an application issue, and
it should lead to unpredictable results. The library should only care
about not corrupting it's structures to the point where accessing them
will lead to a system exception.
Thanks,
Pawel.
P.S. The source file seems to be in DOS format, and there is no
svn:eol-style property set... Shouldn't it be? The patch therefore is
half in DOS format :) (generated using svn diff)
On Fri, Aug 29, 2008 at 11:04 AM, Danny Backx <[EMAIL PROTECTED]> wrote:
> On Thu, 2008-08-28 at 19:29 -0700, Pawel Veselov wrote:
>> Hi,
>>
>> Was looking at the _close_r() in
>> src/newlib/newlib/libc/sys/wince/io.c, at the top:
>>
>> EnterCriticalSection(&critsect);
>> FDCHECK(fd);
>>
>> FDCHECK() macro can return from the method if file handle is invalid,
>> and if it does, wouldn't that leave the lock, and cause deadlock for I/O
>> operations?
>
> That's a very sharp observation ! Thanks for pointing this out.
>
>> P.S. What does critsect protect there? Changes to fd[*].fd ?
>
> I'd say all of the _fdtab array. However, as you point out, in some
> places it's not coded very well. More browsing through that source make
> wonder about the quality of that code. The _read_r() function uses this
> same _fdtab[] but never uses the critical section.
>
> Would you be willing to submit a fix for this ?
>
> My first idea would be to add a second argument to FDCHECK which could
> be the statement to run before the return.
>
> Danny
> --
> Danny Backx ; danny.backx - at - scarlet.be ; http://danny.backx.info
>
>
--
With best of best regards
Pawel S. Veselov
Index: src/newlib/newlib/libc/sys/wince/sys/io.h
===================================================================
--- src/newlib/newlib/libc/sys/wince/sys/io.h (revision 1168)
+++ src/newlib/newlib/libc/sys/wince/sys/io.h (working copy)
@@ -34,11 +34,12 @@
_DEVOPS devops;
} _fdent_t;
-#define FDCHECK(F) \
+#define FDCHECK(F, CS) \
do { \
if (F < 0 || F >= MAXFDS || _fdtab[F].fd == -1) { \
WCETRACE(WCE_IO, "Invalid file handle: %d", F); \
errno = EBADF; \
+ if (CS) { LeaveCriticalSection(CS); } \
return(-1); \
} \
} while (0)
Index: src/newlib/newlib/libc/sys/wince/io.c
===================================================================
--- src/newlib/newlib/libc/sys/wince/io.c (revision 1168)
+++ src/newlib/newlib/libc/sys/wince/io.c (working copy)
@@ -72,7 +72,7 @@
int getfiletype(int fd)
{
- FDCHECK(fd);
+ FDCHECK(fd, 0);
return _fdtab[fd].type;
}
@@ -418,7 +418,7 @@
_fdtab[fd].type, _fdtab[fd].flags, _fdtab[fd].hnd, _fdtab[fd].cxt);
EnterCriticalSection(&critsect);
- FDCHECK(fd);
+ FDCHECK(fd, &critsect);
if (_fdtab[fd].devops == NULL) {
if (_fdtab[fd].type == IO_FILE_TYPE_FILE) {
@@ -458,7 +458,7 @@
return count;
}
- FDCHECK(fd);
+ FDCHECK(fd, 0);
if (_fdtab[fd].devops == NULL) {
if (_fdtab[fd].type == IO_FILE_TYPE_FILE || _fdtab[fd].type == IO_FILE_TYPE_CONSOLE) {
@@ -560,7 +560,7 @@
int method;
WCETRACE(WCE_IO, "lseek(%d, %d, %d)", fd, offset, whence);
- FDCHECK(fd);
+ FDCHECK(fd, 0);
if (_fdtab[fd].devops == NULL) {
switch (whence) {
@@ -653,7 +653,7 @@
for (i = 0; i < set->fd_count; i++) {
int fd = (int)set->fd_array[i];
- FDCHECK(fd);
+ FDCHECK(fd, 0);
/* On WINCE, only IO_FILE_TYPE_SOCKET is handled */
if (_fdtab[fd].type == IO_FILE_TYPE_SOCKET) {
@@ -773,7 +773,7 @@
WCETRACE(WCE_IO, "ioctl(%d, %p %p)", fd, request, arg);
- FDCHECK(fd);
+ FDCHECK(fd, 0);
if (_fdtab[fd].devops == NULL) {
if (_fdtab[fd].type == IO_FILE_TYPE_FILE) {
@@ -858,7 +858,7 @@
{
DWORD newpos;
- FDCHECK(fd);
+ FDCHECK(fd, 0);
if (_fdtab[fd].type != IO_FILE_TYPE_FILE) {
errno = EBADF;
Index: src/newlib/newlib/libc/sys/wince/stat.c
===================================================================
--- src/newlib/newlib/libc/sys/wince/stat.c (revision 1168)
+++ src/newlib/newlib/libc/sys/wince/stat.c (working copy)
@@ -21,7 +21,7 @@
WCETRACE(WCE_IO, "fstat(%d)", fd);
- FDCHECK(fd);
+ FDCHECK(fd, 0);
#if 0
static int first_time = 1;
-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Cegcc-devel mailing list
Cegcc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/cegcc-devel
