On 02/26/2014 06:36 PM, Erich E. Hoover wrote:
On Wed, Feb 26, 2014 at 4:26 PM, Max Woodbury mtewoodbury@gmail.com wrote:
Please explain in words of one syllable why this has been rejected?
Probably for the reasons Ken already mentioned:
- Strange blank line after "if (n == -1)"
- Unnecessary style change to K&R style ("if (ret >= 0) {")
- "[PATCH 2/2]" with no "[PATCH 1/2]"
- No explanation for his comment that EAGAIN is probably not needed.
EINTR might want this behavior, but EGAIN should not. 5) If you bring this code "down" to where you have then you need to remove it from the "higher" level: http://source.winehq.org/source/dlls/ws2_32/socket.c#L4494 http://source.winehq.org/source/dlls/ws2_32/socket.c#L6500 6) It should be in a loop
Does that help?
Best, Erich
So it's mainly on style:
- I've been reading other peoples code for 50 years and that aspect of K&R is much easier to handle in my opinion. Same with the extra blank line. Other parts of wine use the style and are much easier to read as a result.
- And the miscount can't simply be ignored?
If you read kernel code, you'll find that just returning to application level is sometimes enough to free the missing resource. That happens often enough that a single quick retry on EAGAIN is warranted.
Bringing the relevant part of the retry code down and applying it to only those parts where is has a strong likelihood of doing the most good is exactly what the patch does. If one retry is not enough, then higher level strategy is needed so no changes are called for on the higher level. A loop at this level is _not_ a good idea since there would have to be something to break the loop if it runs too long.
In other words, this fiddle is based on several decades of experience with kernel and bottom level application debugging experience. At least look at it and test it before rejecting it on superficial grounds.
max