sony / nmos-cpp

An NMOS (Networked Media Open Specifications) Registry and Node in C++ (IS-04, IS-05)
Apache License 2.0
141 stars 80 forks source link

Open getaddrinfo for public use #350

Closed lo-simon closed 11 months ago

lo-simon commented 11 months ago

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

garethsb commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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

https://github.com/sony/nmos-cpp/blob/f7bd2f7317358123cb0685ef121ed3606c632952/Development/cpprest/host_utils.cpp#L313

lo-simon commented 11 months ago

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

https://github.com/sony/nmos-cpp/blob/f7bd2f7317358123cb0685ef121ed3606c632952/Development/cpprest/host_utils.cpp#L313

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 commented 11 months ago

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

garethsb commented 11 months ago

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:

lo-simon commented 11 months ago

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.