bug-hurd
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Questions about patches for hurdselect.c


From: Svante Signell
Subject: Re: Questions about patches for hurdselect.c
Date: Sun, 13 Jan 2013 21:18:07 +0100

Please don't use @telia.com, it should not be usable any longer, use
@gmail.com: I've changed ISP.
 
On Sun, 2013-01-13 at 20:53 +0100, Samuel Thibault wrote:
> Hello,
> 
> Svante Signell, le Wed 12 Dec 2012 20:11:55 +0100, a écrit :
> > As promised, attached is the first patch to split hurdselect.c code into
> > three cases: DELAY, POLL and SELECT, doing the first two parts as
> > promised in an earlier mail:
> > 1) Introducing three cases: DELAY, POLL, and SELECT
> > 2) Removing the unneeded code for each case statement
> > It does _not_ include any poll code updates yet, that will be in part 3,
> > i.e. the step 2 patch:
> > 3) Rewriting the poll case to be compliant with POSIX 2001.
> 
> Just to make sure: as I told you privately, make sure to perform step
> 2 in a separate patch to be applied on top of this one, so that we can
> actually review it.  We can't review a patch that does the three points
> altogether.

I will do that, I haven't had time yet. Give me a week more.

> > - Notes haver been added to where the code has a bug for the POLL case.
> > The updated POLL code will resolve mot of these bugs.
> 
> Ok, so these are the actual fixes? AIUI, the exact list is:
> 
> - 0-timeout should return immediately, but still manage to get some
> answer from servers.  That is being worked on by Richard through a
> rework of the protocol.

Good, then I don't have to bother with zero timeout problems.

> - In the poll case, on error on one descriptor, the loop shall mark
> POLLERR and continue, not fail altogether.
> 
> Is that right?

The current code errors out if it finds one FD not OK, which is a bug.
Irrespective of how many FDs there are, no checks are made of the input
data given.

> > - Hopefully the code is more readable now, without causing regressions.
> 
> But it duplicates things. As I told you, we might still keep the code
> as-is, and just apply your poll fixes over it. The 3-split still being
> useful for reviewing.

Well, slitting into three parts made the poll case faster (and the delay
case considerably simpler). With a zero timeout the FDs are not ready
with the new code, while they are ready with the old code. That was the
reason I introduced a minimum delay. Of course these problems will be
solved with Richards rewrite of the zero timeout solution. 

> > Comments welcomed, when hopefully accepted (after reviews) the POLL
> > update will follow.
> 
> Well, it's not us to accept it, but Roland.  I have to say I doubt he'll
> accept such a rewrite.  So just proceed with the poll fixes, since
> that's what we actually need.

Well, even if there is some code duplication, splitting into the three
cases I proposed increases readability and maintainability, at least in
my opinion. Doing a lot of if-then-else clutters the code too much, if
used often. Additionally, the poll case _is_ different from select, see
the POSIX specs. Of course, the libc maintainers have the last word on
this. 





reply via email to

[Prev in Thread] Current Thread [Next in Thread]