Move CloseSocket out of SetSocketNonBlocking and pass socket as const reference #10865

pull bytting wants to merge 1 commits into bitcoin:master from bytting:20170718-refactor-1 changing 4 files +10 −9

bytting commented at 2:33 PM on July 18, 2017: contributor

Rationale:

Readability, SetSocketNonBlocking does what it says on the tin.

Consistency, More consistent with the rest of the API in this unit.

Reusability, SetSocketNonBlocking can also be used by clients that may not want to close the socket on failure.

This also moves the responsibility of closing the socket back to the caller that opened it, which in general should know better how and when to close it.
jonasschnelli assigned theuni on Jul 19, 2017
jonasschnelli added the label P2P on Jul 19, 2017
theuni approved
theuni commented at 5:33 PM on July 19, 2017: member

utACK. Can you please squash these together?
bytting commented at 7:34 AM on July 20, 2017: contributor

@theuni, I assume you mean squashing while merging, so nothing for me to do here?
theuni commented at 4:33 PM on July 20, 2017: member

@corebob No, I mean combining the two commits into a single one, and force-pushing the result. Otherwise the first commit would technically be a regression.
in src/netbase.cpp:694 in 5948598ba7 outdated
```
 693 | @@ -692,7 +694,6 @@ bool SetSocketNonBlocking(SOCKET& hSocket, bool fNonBlocking)
 694 |          int fFlags = fcntl(hSocket, F_GETFL, 0);
```
laanwj commented at 7:39 PM on July 20, 2017:

I guess the argument (to SetSocketNonBlocking) can be just SOCKET , or even const SOCKET now that it's no longer changed?
bytting commented at 8:53 PM on July 20, 2017: contributor

I think I got the squashing right, thanks for the assistance :smiley:

As far as I can see, SOCKET is defined as an unsigned int on both platforms, so passing it by value seems reasonable.

It seems to me that both SetSocketNoDelay, InterrupibleRecv and IsSelectableSocket could benefit from const arguments as well.

PS. I just realized I updated the patch with more code after approval. Maybe that was a bad idea.
Move CloseSocket out of SetSocketNonBlocking and pass SOCKET by const reference in SetSocket* functions 05e023f2ec
bytting commented at 7:18 AM on July 22, 2017: contributor

@theuni I made an update to this patch. It should now move CloseSocket out of SetSocketNonBlocking and pass sockets as const reference to some utility functions.

Please comment if you have any issues with this.
bytting renamed this:
~~Move CloseSocket out of SetSocketNonBlocking in netbase.cpp~~
Move CloseSocket out of SetSocketNonBlocking and pass socket as const reference
on Jul 22, 2017
theuni commented at 3:16 PM on July 22, 2017: member

This makes it more clear what's happening, looks good to me. utACK 05e023f2ec8d8dc37bb0f20db1c606f06aea69f5
laanwj merged this on Jul 24, 2017
laanwj closed this on Jul 24, 2017
laanwj referenced this in commit 0c70e845aa on Jul 24, 2017
laanwj commented at 7:22 PM on July 25, 2017: member

ACK
TheBlueMatt commented at 7:50 PM on July 25, 2017: member

Postumous utACK
PastaPastaPasta referenced this in commit 4751ea05f8 on Aug 6, 2019
PastaPastaPasta referenced this in commit 5e39bedadd on Aug 6, 2019
PastaPastaPasta referenced this in commit 8bec6bc976 on Aug 6, 2019
PastaPastaPasta referenced this in commit a85f13c7bf on Aug 7, 2019
PastaPastaPasta referenced this in commit f9c4cdeac4 on Aug 8, 2019
PastaPastaPasta referenced this in commit 0a0ba2837e on Aug 12, 2019
barrystyle referenced this in commit 2b5c2f625c on Jan 22, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Labels