Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open getaddrinfo for public use #350

Merged
merged 4 commits into from
Nov 2, 2023
Merged

Conversation

lo-simon
Copy link
Contributor

Open the getaddrinfo for public use. It is a beneficial function which can look up IP address from the hostname.

@garethsb
Copy link
Contributor

Exposes quite a lot of implementation detail... unless this is only for test code, maybe it might be better to put a getaddrinfo on the service_discovery interface in the style of resolve?

@lo-simon
Copy link
Contributor Author

Thanks, @garethsb, we want it to be opened, so it is not for test code. I was thinking the same, but not sure whether to bring it into service_discovery or some other class. Okay let's do it in service_discovery, will do an update shortly.

@garethsb
Copy link
Contributor

@lo-simon depending on use case, would host_addresses be sufficient?

std::vector<utility::string_t> host_addresses(const utility::string_t& host_name)

@lo-simon
Copy link
Contributor Author

@lo-simon depending on use case, would host_addresses be sufficient?

std::vector<utility::string_t> host_addresses(const utility::string_t& host_name)

Using the host_addresses will not be making the hostname resolution via the DNS server specified in resolv.conf configured on the device. Therefore opening up the getaddrinfo is necessary.

@garethsb
Copy link
Contributor

Hmm, that is surprising, I thought it should use the system resolver that would use resolv.conf if configured correctly.

@garethsb
Copy link
Contributor

garethsb commented Oct 31, 2023

As discussed earlier today, it would be good to have a clear, demonstrated, use case, where using DNSServiceGetAddrInfo produces the required result but POSIX getaddrinfo (which does use nsswitch.conf and resolv.conf on Linux) via Boost.Asio via our host_addresses function doesn't.

If that is the case, this revised PR looks nice. Just a couple of questions:

  • can the DNSServiceGetAddrInfo callback be called multiple times to return more than one address, i.e. should the result be vector-of-address_result like the results of resolve and browse are vectors of their respective types?
  • is it OK to require internet access in the test code (resolving Google DNS to 8.8.8.8)?

@lo-simon
Copy link
Contributor Author

lo-simon commented Nov 1, 2023

can the DNSServiceGetAddrInfo callback be called multiple times to return more than one address, i.e. should the result be vector-of-address_result like the results of resolve and browse are vectors of their respective types?

The function wrapped the mdns_details::getaddrinfo, which executes the DNSServiceRefDeallocate after receiving the 1st addrinfo to prevent any more DNSServiceGetAddrInfo callback. But the wrapped function should also be returning a vector of results as in case no mDNSResponder is used, host_addresses will be used which will be returning a list of ipaddresses.

is it OK to require internet access in the test code (resolving Google DNS to 8.8.8.8)?

As it is already passing the testDnsGetAddrInfo unit test, the runner must be able to access the internet.

@lo-simon lo-simon merged commit 4bdd15a into sony:master Nov 2, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants