The patch looks good. Your arguments are valid too. I'd take approach #2
if I were implementing newlib, but your argument #3 "why do we care" is
certainly valid.
The only thing I need to commit this is a ChangeLog entry.
Danny
On Fri, 2008-08-29 at 13:34 -0700, Pawel Veselov wrote:
> 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
> >
> >
>
>
>
> -------------------------------------------------------------------------
> 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
--
Danny Backx ; danny.backx - at - scarlet.be ; http://danny.backx.info
-------------------------------------------------------------------------
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
