Skip to content

Conversation

vasild
Copy link
Contributor

@vasild vasild commented Jun 20, 2022

This is a piece of #21878, chopped off to ease review.

Wrap the syscall getsockname() in Sock::GetSockName() and change GetBindAddress() to take a Sock argument so that it can use the wrapper.

This further encapsulates syscalls inside the Sock class and makes the callers mockable.

vasild added 2 commits June 20, 2022 14:51
This will help to increase `Sock` usage and make more code mockable.
This avoids the direct call to `getsockname()` and allows mocking.
@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 20, 2022

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #25421 (net: convert standalone IsSelectableSocket() and SetSocketNonBlocking() to Sock methods by vasild)
  • #23531 (Add Yggdrasil support by prusnak)
  • #21878 (Make all networking code mockable by vasild)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@laanwj
Copy link
Member

laanwj commented Jun 20, 2022

Code review ACK a8d6abb
I was about to propose a different return value strategy for GetSockName (eg, return the data instead of take out-arg pointers) but i don't think it's worth it. At least this change is trivial to review like this.

@DrahtBot DrahtBot mentioned this pull request Jun 20, 2022
@laanwj laanwj merged commit ba29911 into bitcoin:master Jun 28, 2022
@vasild vasild deleted the getsockname branch June 28, 2022 11:42
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 28, 2022
…ps getsockname() and use it in GetBindAddress()

a8d6abb net: change GetBindAddress() to take Sock argument (Vasil Dimov)
748dbcd net: add new method Sock::GetSockName() that wraps getsockname() (Vasil Dimov)

Pull request description:

  _This is a piece of bitcoin#21878, chopped off to ease review._

  Wrap the syscall `getsockname()` in `Sock::GetSockName()` and change `GetBindAddress()` to take a `Sock` argument so that it can use the wrapper.

  This further encapsulates syscalls inside the `Sock` class and makes the callers mockable.

ACKs for top commit:
  laanwj:
    Code review ACK a8d6abb

Tree-SHA512: 3a73463258c0057487fb3fd67215816b03a1c5160f45e45930eaeef86bb3611ec385794cdb08339aa074feba8ad67cd2bfd3836f6cbd40834e15d933214a05dc
@bitcoin bitcoin locked and limited conversation to collaborators Jun 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants