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

dns: provide generic dns_query() function #18378

Merged
merged 3 commits into from
Aug 1, 2022
Merged

Conversation

benpicco
Copy link
Contributor

Contribution description

Since we can have multiple DNS backends now, provide a common dns_query() wrapper function that allows to write backend agnostic applications.

Testing procedure

Issues/PRs references

@github-actions github-actions bot added Area: network Area: Networking Area: sys Area: System labels Jul 27, 2022
@benpicco benpicco added the Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer label Jul 27, 2022
sys/include/net/dns.h Outdated Show resolved Hide resolved
sys/include/net/dns.h Outdated Show resolved Hide resolved
@miri64 miri64 added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label Jul 27, 2022
@benpicco benpicco removed the Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer label Jul 27, 2022
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.

sock_dtls is a bit fiddly when it comes to this (should be fixed long-term!), but does this work? If yes it makes the code a bit more easy to read. If does not work, IS_USED is preferred in any case over defined to check the usage of a module.

sys/include/net/dns.h Outdated Show resolved Hide resolved
sys/include/net/dns.h Outdated Show resolved Hide resolved
sys/include/net/dns.h Outdated Show resolved Hide resolved
sys/include/net/dns.h Outdated Show resolved Hide resolved
sys/include/net/dns.h Outdated Show resolved Hide resolved
sys/include/net/dns.h Outdated Show resolved Hide resolved
sys/include/net/dns.h Outdated Show resolved Hide resolved
@RIOT-OS RIOT-OS deleted a comment from miri64 Jul 27, 2022
@benpicco
Copy link
Contributor Author

Including dodtls.h unconditionally fails:

sys/include/net/sock/dtls.h:1001:10: fatal error: sock_dtls_types.h: No such file or directory
 1001 | #include "sock_dtls_types.h"

If does not work, IS_USED is preferred in any case over defined to check the usage of a module.

But why? I see no advantage of using IS_USED outside C code, in fact it makes the code less idiomatic.

benpicco deleted a comment from miri64 18 minutes ago

Huh I think I did delete a comment of mine 😅

@miri64
Copy link
Member

miri64 commented Jul 28, 2022

But why? I see no advantage of using IS_USED outside C code, in fact it makes the code less idiomatic.

AFAIK, defined merely checks if the macro is defined, IS_USED also checks if the macro is either 0 or 1.

@benpicco
Copy link
Contributor Author

Yes but under which conditions would be have MODULE_GCOAP_DNS defined to 0?

@miri64
Copy link
Member

miri64 commented Jul 28, 2022

Yes but under which conditions would be have MODULE_GCOAP_DNS defined to 0?

Maybe testing, where you want to have that module in, but you don't want the behavior it implies in other modules, so you set CFLAGS += -DMODULE_GCOAP_DNS=0... but granted this very pretty constructed and might lead to other conflicts... so idk.

@miri64
Copy link
Member

miri64 commented Jul 28, 2022

Yes but under which conditions would be have MODULE_GCOAP_DNS defined to 0?

Maybe testing, where you want to have that module in, but you don't want the behavior it implies in other modules, so you set CFLAGS += -DMODULE_GCOAP_DNS=0... but granted this very pretty constructed and might lead to other conflicts... so idk.

But to wrap up this discussion: using C-conditionals is not possible, because sock/dtls.h (which is included by gcoap_dtls and sock_dodtls headers) requires a sock_dtls_types.h to be present, which is only provided if a sock_dtls implementation is present. As such, C-conditionals can't be used at the moment (will try if I can do a quick fix). This only leaves #18378 (review).

@miri64
Copy link
Member

miri64 commented Jul 28, 2022

(will try if I can do a quick fix)

See #18380. @benpicco can you try, if with that C-conditionals work?

@benpicco benpicco requested a review from maribu as a code owner July 28, 2022 12:27
@benpicco
Copy link
Contributor Author

Yup that works

@miri64 miri64 added the State: waiting for other PR State: The PR requires another PR to be merged first label Jul 28, 2022
@miri64
Copy link
Member

miri64 commented Jul 28, 2022

Yup that works

Than I would be very happy about an ACK in #18380 ;-)

@benpicco
Copy link
Contributor Author

It's a two line change, can't I just include it here?

@miri64
Copy link
Member

miri64 commented Jul 28, 2022

As long as authorship is preserved.. But even for 2 line change: I won't review my own code.

@benpicco
Copy link
Contributor Author

benpicco commented Jul 28, 2022

But even for 2 line change: I won't review my own code.

Are you being serious? 😄
And anyway, if I cherry pick the commit, isn't that pretty much me giving it an ACK?

@miri64
Copy link
Member

miri64 commented Jul 28, 2022

But even for 2 line change: I won't review my own code.

Are you being serious? 😄
And anyway, if I cherry pick the commit, isn't that pretty much me giving it an ACK?

Yes and yes. If I f*** up my own code and the reviewer did not test properly, the responsibility does not only lie with me. If I f*** my own code and ACK my own code with only the testing I did, the responsibility is completely with me. So, knowing myself, I'd rather avoid the risk of that 😉.

@miri64 miri64 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Jul 28, 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 Jul 29, 2022
@benpicco
Copy link
Contributor Author

Now this PR only contains commits by yours truly, you are free to review it 😉

sys/net/netutils/util.c Show resolved Hide resolved
sys/net/sock/sock_util.c Show resolved Hide resolved
sys/Makefile.dep Show resolved Hide resolved
@miri64 miri64 enabled auto-merge August 1, 2022 09:23
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.

ACK

@miri64 miri64 merged commit 24ff493 into RIOT-OS:master Aug 1, 2022
@benpicco benpicco deleted the dns_query branch August 1, 2022 09:42
@maribu maribu added this to the Release 2022.10 milestone Oct 14, 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 CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants