gnrc_ipv6_nib: Add capability to handle and send RDNSS option by miri64 · Pull Request #8646 · RIOT-OS/RIOT

miri64 · 2018-02-27T09:33:04Z

Contribution description

With the RDNSS option sock_dns can be auto-configured by the administrative router (either the border router or the upstream router configuring the border router).

Also some fixes/convenience enhancements to sock_dns and sock_util piggy-backed.

Issues/PRs references

None

miri64 · 2018-06-04T13:21:22Z

Ping @bergzand @kaspar030 @smlng this would allow us to finally include DNS into gnrc_networking ;-).

smlng · 2018-06-04T17:34:20Z

Makefile.dep

  USEMODULE += sock_util
 endif

+ifneq (,$(filter sock_util,$(USEMODULE)))


seems unrelated, at least for this PR its usage is unclear

The commit is describing the change sufficiently enough IMHO: 3b79b2c

But to explain it more in-depth: sock_util requires POSIX, but this wasn't declared when it was introduced. This PR uses sock_util via sock_dns and thus the module is not compilable if the user does not explicitly include posix.

smlng · 2018-06-04T18:57:36Z

sys/net/gnrc/network_layer/ndp/gnrc_ndp.c

    return pkt;
 }

+gnrc_pktsnip_t *gnrc_ndp_opt_rdnss_build(uint32_t ltime, ipv6_addr_t *addrs,


this function is only used with GNRC_IPV6_NIB_CONF_DNS, IMHO the internal #if is not needed, but compiler might complain about unused function if DNS is not used?!

This is a public function. The unused function warning does not apply here.

I tried without the '#if` guard and it works so no need for this ugly construct. Hence, you can remove the preprocessor if and omit the else path for unused params.

But then it will be compiled in, when we don't use DNS...

(aka the code size will be affected)

Ok. It isn't. Will remove.

yep, just wanted to report the same:

this PR 55104 508 14972 70584 113b8 /RIOT/tests/gnrc_sock_dns/bin/samr21-xpro/tests_gnrc_sock_dns.elf 47108 144 7252 54504 d4e8 /RIOT/tests/gnrc_ipv6_nib/bin/samr21-xpro/tests_gnrc_ipv6_nib.elf without pre-if 55104 508 14972 70584 113b8 /RIOT/tests/gnrc_sock_dns/bin/samr21-xpro/tests_gnrc_sock_dns.elf 47108 144 7252 54504 d4e8 /RIOT/tests/gnrc_ipv6_nib/bin/samr21-xpro/tests_gnrc_ipv6_nib.elf

smlng · 2018-06-04T19:07:24Z

sys/net/application_layer/dns/dns.c

 #define DNS_MIN_REPLY_LEN   (unsigned)(sizeof(sock_dns_hdr_t ) + 7)

+/* global DNS server UDP endpoint */
+sock_udp_ep_t sock_dns_server;


is one global DNS server sufficient?

I hoped that this could be const for some use-cases, but I'm OK with moving it here for now.

is one global DNS server sufficient?

For now it should be. Your local LAN usually also is happy with just one ;-).

smlng · 2018-06-04T19:08:13Z

sys/net/gnrc/network_layer/ipv6/nib/nib.c

+        }
+        else {
+            evtimer_del(&_nib_evtimer, &_rdnss_timeout.event);
+            memset(&sock_dns_server, 0, sizeof(sock_dns_server));


shouldn't this use _handle_rdnss_timeout too?

I'm not so sure, if replacing one function with another calling that function makes a difference, but if you insist...

kaspar030

ACK for the changes to sock_dns.

kaspar030 · 2018-06-04T21:52:05Z

sys/net/application_layer/dns/dns.c

 #define DNS_MIN_REPLY_LEN   (unsigned)(sizeof(sock_dns_hdr_t ) + 7)

+/* global DNS server UDP endpoint */
+sock_udp_ep_t sock_dns_server;


I hoped that this could be const for some use-cases, but I'm OK with moving it here for now.

miri64 · 2018-06-05T07:52:40Z

Adressed comments.

tcschmidt · 2018-06-21T12:10:29Z

@smlng agree?

miri64 · 2018-06-21T12:38:27Z

Rebased

smlng · 2018-06-22T06:36:59Z

please squash

smlng · 2018-06-22T06:55:11Z

sys/net/gnrc/network_layer/ipv6/nib/nib.c

+#if GNRC_IPV6_NIB_CONF_DNS
+static void _handle_rdnss_timeout(sock_udp_ep_t *dns_server)
+{
+    memset(&dns_server, 0, sizeof(sock_udp_ep_t));


dns_server is a pointer already, hence should be memset(dns_server, 0, sizeof(sock_udp_ep_t));? at least on macOS I get compile error:

/RIOT/sys/net/gnrc/network_layer/ipv6/nib/nib.c:1255:5: error: '__builtin___memset_chk' will always overflow destination buffer [-Werror,-Wbuiltin-memcpy-chk-size] memset(&dns_server, 0, sizeof(sock_udp_ep_t)); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /usr/include/secure/_string.h:76:3: note: expanded from macro 'memset' __builtin___memset_chk (dest, __VA_ARGS__, __darwin_obsz0 (dest)) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 error generated.

Oops (but what did you use to compile that finds this type confusion)

macOS -> clang

We really should start having a clang config for the CI ;-).

smlng · 2018-06-22T06:58:26Z

sys/net/gnrc/network_layer/ipv6/nib/nib.c

 */
 static void _handle_mtuo(gnrc_netif_t *netif, const icmpv6_hdr_t *icmpv6,
                         const ndp_opt_mtu_t *mtuo);
+static uint32_t _handle_rdnsso(gnrc_netif_t *netif, const icmpv6_hdr_t *icmpv6,


needs to be guarded with #if GNRC_IPV6_NIB_CONF_DNS, got error:

/RIOT/sys/net/gnrc/network_layer/ipv6/nib/nib.c:405:17: error: unused function '_handle_rdnsso' [-Werror,-Wunused-function] static uint32_t _handle_rdnsso(gnrc_netif_t *netif, const icmpv6_hdr_t *icmpv6, ^ 1 error generated.

otherwise

smlng · 2018-06-22T07:00:23Z

sys/net/gnrc/network_layer/ndp/gnrc_ndp.c

    return pkt;
 }

+gnrc_pktsnip_t *gnrc_ndp_opt_rdnss_build(uint32_t ltime, ipv6_addr_t *addrs,


I tried without the '#if` guard and it works so no need for this ugly construct. Hence, you can remove the preprocessor if and omit the else path for unused params.

smlng · 2018-06-22T07:01:29Z

sys/net/gnrc/network_layer/ipv6/nib/nib.c

+            }
+        }
+    }
+#if SOCK_HAS_IPV6


does this make sense, I would assume if GNRC_IPV6_NIB_CONF_DNS is true, we have IPv6 support?

IPv6-support for sock is independent of GNRC's inclusion of GNRC. So it makes sense.

from sys/Makefile.include:

ifneq (,$(filter gnrc_sock,$(USEMODULE))) USEMODULE_INCLUDES += $(RIOTBASE)/sys/net/gnrc/sock/include ifneq (,$(filter gnrc_ipv6,$(USEMODULE))) CFLAGS += -DSOCK_HAS_IPV6 endif endif

Hence, I would (recursively) resolve/deduce GNRC_IPV6_NIB_CONF_DNS -> GNRC_IPV6 -> SOCK_HAS_IPV6.

I'd rather prefer to keep the dependency explicit as it is instead of implicit, which addressing this change would make it.

miri64 · 2018-06-22T08:10:19Z

Adressed comments.

miri64 · 2018-06-22T08:38:50Z

May I squash?

miri64 · 2018-07-04T15:37:27Z

Ping?

smlng

Tested ACK, works with tests/gnrc_sock_dns on native according to readme. Note: one problem I had was that the firewall on Fedora is active by default and blocks DNS, after explicitly allowing DNS on tap0 it work as expected.

smlng · 2018-07-05T07:40:51Z

please squash, and trigger CI when ready.

miri64 · 2018-07-05T08:20:42Z

Should I either rebase to #9498 now or close #9498?

miri64 · 2018-07-05T08:23:20Z

~~Rebased~~ Squashed. I really don't care either way regarding my question.

smlng · 2018-07-05T08:43:25Z

I would rather go for rebase, I just ACKed #9498 so we can move both forward. IMHO its better to have the dependency update separate, okay?

miri64 · 2018-07-05T09:01:05Z

Okay!

miri64 · 2018-07-05T09:01:35Z

Rebased! :-)

miri64 · 2018-07-05T09:09:57Z

Fixed and squashed error reported by Travis.

miri64 · 2018-07-05T10:26:41Z

@smlng Ok to merge?

miri64 · 2018-07-05T11:09:32Z

🎉

miri64 · 2018-07-05T11:16:12Z

@danpetry @bergzand now that this is merged, this might help you with your respective works (if the RAdvD is configured correctly [@smlng maybe something to consider to put in the example config @RIOT-Makers?] and the applications you write use sock_dns).

miri64 assigned kaspar030, smlng and bergzand and unassigned kaspar030 and smlng Feb 27, 2018

miri64 force-pushed the gnrc/enh/dns-config branch from bf067c5 to 44529ea Compare June 4, 2018 13:20

smlng suggested changes Jun 4, 2018

View reviewed changes

kaspar030 reviewed Jun 4, 2018

View reviewed changes

miri64 force-pushed the gnrc/enh/dns-config branch from 8bfc522 to b365d4a Compare June 21, 2018 12:37

smlng approved these changes Jun 22, 2018

View reviewed changes

smlng suggested changes Jun 22, 2018

View reviewed changes

miri64 mentioned this pull request Jun 22, 2018

murdock: also compile with LLVM/clang #9398

Merged

miri64 mentioned this pull request Jul 4, 2018

net: add Asymcute, an asynchronous MQTT-SN client implementation #9464

Merged

miri64 mentioned this pull request Jul 4, 2018

sock_util: add posix dependency #9498

Merged

smlng approved these changes Jul 5, 2018

View reviewed changes

miri64 force-pushed the gnrc/enh/dns-config branch from f050cb6 to fa9315a Compare July 5, 2018 08:23

miri64 added 3 commits July 5, 2018 11:01

ndp: define RDNSS option

8eab80c

gnrc_sock_dns: Move DNS server end point to implementation

fbf216f

gnrc_ndp: add support for building RDNSS option

0ef62b9

miri64 force-pushed the gnrc/enh/dns-config branch from fa9315a to b0fb99d Compare July 5, 2018 09:01

miri64 added 4 commits July 5, 2018 11:09

gnrc_ipv6_nib: add support to handle RDNSSO

6d1f012

gnrc_ipv6_nib_router: add support to send RDNSSO

1916c45

sock_dns: return error if DNS server is not set

5361c4c

tests: adapt gnrc_sock_dns for auto-configured DNS

95c1992

miri64 force-pushed the gnrc/enh/dns-config branch from b0fb99d to 95c1992 Compare July 5, 2018 09:09

miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 5, 2018

smlng merged commit dd16562 into RIOT-OS:master Jul 5, 2018

miri64 deleted the gnrc/enh/dns-config branch July 5, 2018 11:09

cladmi added this to the Release 2018.07 milestone Jul 10, 2018

Conversation

miri64 commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Issues/PRs references

Uh oh!

miri64 commented Jun 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspar030 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Jun 5, 2018

Uh oh!

tcschmidt commented Jun 21, 2018

Uh oh!

miri64 commented Jun 21, 2018

Uh oh!

smlng commented Jun 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Jun 22, 2018

Uh oh!

miri64 commented Jun 22, 2018

miri64 commented Feb 27, 2018 •

edited

Loading

miri64 commented Jul 5, 2018 •

edited

Loading