networkd: configure link even if no routes have been received by dhcp #6886

ssahani · 2017-09-22T09:00:14Z

 networkctl 
IDX LINK             TYPE               OPERATIONAL SETUP     
  1 lo               loopback           carrier     unmanaged 
  2 eth0             ether              no-carrier  configuring 
  5 host             ether              routable    configured <==========

5 links listed.

sitsofe · 2017-09-22T17:05:39Z

src/network/networkd-dhcp4.c

        if (n < 0)
-                return log_link_warning_errno(link, n, "DHCP error: could not get routes: %m");
+                log_link_warning_errno(link, n, "DHCP error: could not get routes: %m");


It's not really an error for DHCP not to send a route. It could be just interface is unroutable to the outside....

This pr is about dhcp not sending any routes. So what I am missing here. It's waring message.

If sd_dhcp_lease_get_routes returns -ENODATA, this will still print an error. You have to exclude this case like above:
if (n < 0 && n != -ENODATA)

I think we should log the warning message .

This comment is about this change and the one in line 90.

If I got the intend of this PR right this is about DHCP responses that do not carry a ROUTER attribute aka a default gateway.

IMHO logging this situation is definitely required. I am just unsure if warning is the right level here. warning causes me to think that there might be something wrong within networkd but it was able to continue vs this is a (valid) edge case that networkd did handle, but you might be wondering why there is no default route.

Wouldn't log_link_info_errno be better suited in that case? In any case the logged message could probably made more descriptive.

For the first message in Line 90 logging beased on one of the two return codes (-EINVAL, -ENODATA) could probably be something like

switch (n) { case -ENODATA: log_info(…, "DHCP {nfo: No routes received from DHCP server: %m"); break ; case -EINVAL: default: log_{warrning,error}(…, "DHCP {warning|error}: could not get routes: %m"); break ; }

I have had many frustrating hours with software that just logged errors like Subsystem: error: ${errno} where I had to start reading the source code to actually get why or what is going wrong. Getting this right in networkd would be awesome :-)

Hmm, I may misunderstand the code or what the routes here are. But on my laptop, I got

wlp2s0: DHCP error: could not get routes: No data available

however I can connect internet. I thought that even if we do not get routes from DHCP, getting default gateway is sufficient to connect outside. Is it correct, or not?

Yes that is correct. But we can't really keep the log in info level.

Perhaps "DHCP server did not provide any additional routes" would be a nicer message...

@ssahani Then, I still do not understand why you stick to warn here. I think we should warn if DHCP server do not provide either default gateway or additional routes. What do you think about the following?

if (n == -ENODATA) log_link_info(link, "DHCP: No routes received from DHCP server: %m"); else if (n < 0) log_link_warning_errno(link, n, "DHCP error: could not get routes: %m"); if (r < 0 && n < 0) log_link_warning(link, "DHCP error: No routes received from DHCP server: %m");

Agree this is more informative. When the asset fails for get_router we should warn.

sitsofe

configure linke
Minor but you're missing an r on the word linker.

ssahani · 2017-09-22T17:18:03Z

I can't follow you sorry

flo-wer · 2017-09-22T19:23:40Z

He is referring to the title, it is meant to be "configure link", not "linke" or "linker".

ssahani · 2017-09-23T02:13:41Z

Ohh fixed thanks :)

yuwata · 2017-09-30T14:44:17Z

src/network/networkd-dhcp4.c

-                return log_link_warning_errno(link, r, "DHCP error: could not get gateway: %m");
+        if (r == -ENODATA)
+                log_link_info(link, "DHCP: No routes received from DHCP server: %m");
+        else


Should be else if (r < 0) here? With this PR, I got

wlp2s0: DHCP error: could not get gateway: Success

Yes right . updated. thanks !

Updated commit fixes my point. Thanks.

yuwata · 2017-10-02T02:28:04Z

Thanks. lgtm.

poettering · 2017-10-02T15:51:43Z

src/network/networkd-dhcp4.c

+        else if (n < 0)
+                log_link_warning_errno(link, n, "DHCP error: could not get routes: %m");
+
+        if (n < 0 && r < 0) {


hmm, i really don't like that you check r here, after all it has possibly been updated in the mean time with lots of other values since it was set to -ENODATA because no routes were discovered.

Why not just use the the test "link->dhcp4_messages == 0" here instead? that's a much more correct and explicit test under the same conditions, or am I wrong?

yes that is much cleaner. thanks !

poettering · 2017-10-02T15:52:49Z

src/network/networkd-dhcp4.c

+
+                link->dhcp4_configured = true;
+                link_check_ready(link);
+        }


so, even though it's not strictly necessary either you should do "return 0" here i figure, or just move the whole block to the end of the function... otherwise it's weird, if you still go through the loop even though you already decided its unnecessary...

or am i missing something here?

Fixes systemd#3752 networkctl IDX LINK TYPE OPERATIONAL SETUP 1 lo loopback carrier unmanaged 2 eth0 ether no-carrier configuring 5 host ether routable configured <========== 5 links listed.

ssahani · 2017-10-02T17:34:47Z

updated

…arspec_from_time_t() gmtime_r() will return NULL in that case, and we would crash. I committed the reproducer case in fuzz-regressions/, even though we don't have ubsan hooked up yet. Let's add it anyway in case it is useful in the future. We actually crash anyway when compiled with asserts, so this can be easily reproduced without ubsan. oss-fuzz systemd#6886.

ssahani mentioned this pull request Sep 22, 2017

systemd-networkd can't configure interface if dhcp server does not provide routers #3752

Closed

2 tasks

sitsofe reviewed Sep 22, 2017

View reviewed changes

ssahani changed the title ~~networkd: configure linke even if no routes have been received by dhcp~~ networkd: configure link even if no routes have been received by dhcp Sep 23, 2017

ssahani force-pushed the issue-3752 branch from 28a8d91 to 435f812 Compare September 23, 2017 02:07

ssahani force-pushed the issue-3752 branch from 435f812 to 8c1b2dc Compare September 29, 2017 13:04

yuwata reviewed Sep 30, 2017

View reviewed changes

yuwata added network reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Sep 30, 2017

ssahani force-pushed the issue-3752 branch 2 times, most recently from 5b59a5d to 91ba098 Compare October 2, 2017 02:21

yuwata removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Oct 2, 2017

yuwata added the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Oct 2, 2017

poettering reviewed Oct 2, 2017

View reviewed changes

poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels Oct 2, 2017

networkd: configure link even if no routes have been received by dhcp

8349c07

Fixes systemd#3752 networkctl IDX LINK TYPE OPERATIONAL SETUP 1 lo loopback carrier unmanaged 2 eth0 ether no-carrier configuring 5 host ether routable configured <========== 5 links listed.

ssahani force-pushed the issue-3752 branch from 91ba098 to 8349c07 Compare October 2, 2017 17:33

poettering merged commit 444b017 into systemd:master Nov 20, 2017

ssahani deleted the issue-3752 branch November 21, 2017 09:15

yuwata removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Jun 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

networkd: configure link even if no routes have been received by dhcp #6886

networkd: configure link even if no routes have been received by dhcp #6886

ssahani commented Sep 22, 2017

sitsofe Sep 22, 2017

ssahani Sep 22, 2017

flo-wer Sep 22, 2017

ssahani Sep 23, 2017

andir Sep 29, 2017

yuwata Oct 1, 2017

ssahani Oct 1, 2017

sitsofe Oct 1, 2017

yuwata Oct 2, 2017

ssahani Oct 2, 2017 •

edited

Loading

sitsofe left a comment

ssahani commented Sep 22, 2017

flo-wer commented Sep 22, 2017

ssahani commented Sep 23, 2017

yuwata Sep 30, 2017

ssahani Sep 30, 2017

yuwata Oct 1, 2017

yuwata commented Oct 2, 2017

poettering Oct 2, 2017

ssahani Oct 2, 2017

poettering Oct 2, 2017

ssahani Oct 2, 2017

ssahani commented Oct 2, 2017

networkd: configure link even if no routes have been received by dhcp #6886

networkd: configure link even if no routes have been received by dhcp #6886

Conversation

ssahani commented Sep 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssahani Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

sitsofe left a comment

Choose a reason for hiding this comment

ssahani commented Sep 22, 2017

flo-wer commented Sep 22, 2017

ssahani commented Sep 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuwata commented Oct 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssahani commented Oct 2, 2017

ssahani Oct 2, 2017 •

edited

Loading