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

sys/net/sock_util: add sock_tl_name2ep() to optionally perform DNS lookups #17510

Merged
merged 3 commits into from Mar 24, 2022

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Jan 13, 2022

Contribution description

This adds a new helper function sock_tl_name2ep() that will perform a DNS lookup if the sock_dns is used.
Otherwise it behaves exactly as sock_tl_str2ep()

Testing procedure

Tests were added to tests/netutils

Issues/PRs references

miri64
miri64 previously requested changes Jan 13, 2022
sys/Makefile.include Outdated Show resolved Hide resolved
@miri64
Copy link
Member

miri64 commented Jan 13, 2022

Can we make this its own function that then calls sock_tl_str2ep(), please? str2ep IMHO should only convert an actual string representing an address-port-pair to an endpoint. This way we do not muddy functionality and it is clearer what which function does. E.g. sock_tl_name2ep() or something.

@miri64
Copy link
Member

miri64 commented Jan 13, 2022

(you also might not want to fallback to a name in some use case and explicitly want to call sock_tl_str2ep() without any implicit network operation)

sys/net/sock/sock_util.c Outdated Show resolved Hide resolved
@benpicco
Copy link
Contributor Author

Can we make this its own function that then calls sock_tl_str2ep(), please?

Mind you that sock_tl_str2ep() is not called directly but always by a wrapper (sock_udp_str2ep()/sock_tcp_str2ep()).

The idea was that all users (e.g. suit_coap_get_blockwise_url()) will get this for 'free' when sock_dns is enabled.

@miri64
Copy link
Member

miri64 commented Jan 14, 2022

Mind you that sock_tl_str2ep() is not called directly but always by a wrapper (sock_udp_str2ep()/sock_tcp_str2ep()).

I don't see how this is relevant... they are still the conversion functions from string to endpoint.

The idea was that all users (e.g. suit_coap_get_blockwise_url()) will get this for 'free' when sock_dns is enabled.

If you get something for free with unforeseen consequences, it is never really for free. IMHO we should change the usage of sock_udp_str2ep() in those instances to sock_udp_name2ep() where it is sensible. A classic example where it is not sensible, for instance, is when you want to set a DNS server's endpoint ;-).

@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Jan 26, 2022
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 26, 2022
@benpicco benpicco changed the title sys/net/sock_util: add support for DNS to sock_tl_str2ep() sys/net/sock_util: add sock_tl_name2ep() to optionally perform DNS lookups Jan 26, 2022
@benpicco
Copy link
Contributor Author

I added a new sock_tl_name2ep() function as well as UDP and TCP wrappers.
I would then convert users of sock_udp_str2ep() as a follow-up.

@benpicco benpicco requested a review from miri64 February 10, 2022 13:59
@benpicco benpicco added this to Backlog / Proposals in RIOT Sprint Day via automation Mar 21, 2022
sys/net/sock/sock_util.c Outdated Show resolved Hide resolved
Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK (after that fix).

@benpicco
Copy link
Contributor Author

benpicco commented Mar 24, 2022

Fix applied.
ping @miri64

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@miri64 miri64 dismissed their stale review March 24, 2022 14:59

:checkmark:

RIOT Sprint Day automation moved this from Backlog / Proposals to Waiting For Murdock Mar 24, 2022
@miri64 miri64 merged commit 76935dd into RIOT-OS:master Mar 24, 2022
RIOT Sprint Day automation moved this from Waiting For Murdock to Done Mar 24, 2022
@benpicco benpicco deleted the sock_tl_str2ep-dns branch March 24, 2022 15:02
@OlegHahm OlegHahm added this to the Release 2022.04 milestone Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants