select()'s first argument should be zero, if no file descriptors are selected #1786
Conversation
|
Yes, adding an extra flag should solve the problem too. ACK |
|
This does not fix 10022 spam on Windows, as But I think it is another fix worth to merge in addition to #1772, so ACK. |
| int nSelect = select(hSocketMax + 1, &fdsetRecv, &fdsetSend, &fdsetError, &timeout); | ||
| if (have_fds) | ||
| nSelect = select(hSocketMax + 1, | ||
| &fdsetRecv, &fdsetSend, &fdsetError, &timeout); |
There was a problem hiding this comment.
What adds such a line-break to readability?
|
@Diapolo so you actually tested this on Windows, for the net and !net cases? I'm not sure the Windows expected behavior matches what you describe... |
|
Indeed, that So yes on Windows you still need the check to prevent error spam. It appears that windows does not allow for using select as simply a timeout. However, now you can check for have_fds instead of hSocketMax!=0, which is at least correct. |
|
Yes, I understand it comes from the documentation, but is it actually seen after applying this patch? |
|
The error 10022 is still logged with this and without my patch, as select() returns WSAEINVAL. I tried it :). @laanwj Yes, a check for |
|
It makes sense that it still occurs because windows ignores the nfds argument. So this change is a no-op from the viewpoint of winsock. We could spare ourselves worrying about all these kinds of low-level OS details if we used an abstraction such as boost::asio (which will also use more efficient mechanisms such as epoll on platforms that support them). |
|
That is what I tried to say @jgarzik, his patch does not prevent the 10022 spam, I read the select() documentation on MSDN more than once to understand what is happening ^^. I rather sure most part of that network code are still from Satoshi (at least he was the one who merged them to Github at first). |
|
| { | ||
| int nErr = WSAGetLastError(); |
There was a problem hiding this comment.
When making my pulls obsolete, which makes sense here to have 1 instead of 2 to fix this, could you please merge the WSAGetLastError() into the printf :)?
There was a problem hiding this comment.
It is nicer this way, especially if you are stepping through with a debugger.
|
Verified to work and fix the 10022 spam on Windows, tried it and closed #1772 in favor of your patch. |
|
ACK |
select()'s first argument should be zero, if no file descriptors are selected
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/2387944782fa61a3137afda91e9e8105d8cc5ddf for binaries and test log. |
e04d2bd Cleanup: removing unused mapRelay and cs_mapRelay. (furszy) Pull request description: Cleaning the currently not unused `mapRelay` and `cs_mapRelay`. Making bitcoin#1786 obsolete. ACKs for top commit: random-zebra: utACK e04d2bd Fuzzbawls: utACK e04d2bd Tree-SHA512: ccbbec9f5e04995982e891191c844c3f81d4c06798d35f80ab52c0e07c05dd5ab15bf3a8c3e2476ae282794c00cb533ba807116142db0d455cf3791424ec5ab8
The most portable invocation for the zero-fds case, a widely used
portable hack for sleeping, generally involves passing NULL arguments
when the fd sets are unused. This form is used here.
Intended to supercede #1772.