Bug 1023503
Summary: | Port RHEL 6.5 PPPoE reconnect delay to git master | ||
---|---|---|---|
Product: | Red Hat Enterprise Linux 7 | Reporter: | Dan Williams <dcbw> |
Component: | NetworkManager | Assignee: | Jirka Klimes <jklimes> |
Status: | CLOSED CURRENTRELEASE | QA Contact: | Desktop QE <desktop-qa-list> |
Severity: | unspecified | Docs Contact: | |
Priority: | unspecified | ||
Version: | 7.0 | CC: | dcbw, kdube, rkhan, thaller, tpelka, vbenes |
Target Milestone: | rc | ||
Target Release: | --- | ||
Hardware: | Unspecified | ||
OS: | Unspecified | ||
Whiteboard: | |||
Fixed In Version: | NetworkManager-0.9.9.0-28.git20131212.el7 | Doc Type: | Bug Fix |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2014-06-13 10:18:18 UTC | Type: | Bug |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Attachments: |
Description
Dan Williams
2013-10-25 14:48:25 UTC
Created attachment 816194 [details]
rh602265-pppoe-reconnect-delay.patch
Created attachment 834237 [details]
[PATCH] ethernet: add reconnect delay for PPPoE connections (for master)
Previous patch rebased to upstream master. (You can also see it in jk/rh1023503-pppoe).
Looks good to me; I can test it tomorrow, but if you want to test it out yourself, you can install the rp-pppoe package on a different machine on the same LAN and then follow these directions: http://darmawan-salihun.blogspot.com/2008/12/setting-up-basic-pppoe-server-in-linux.html starting at the "Let's proceed to configure the machines one by one." part, and stopping at the "For the pppoe client machine, the important configuration files are" part since NetworkManager handles that. Then you can run the pppoe server with a command like: pppoe-server -k -I eth0 -F and then let NetworkManager attempt to connect. As long as the two machines are on the same LAN, the client will broadcast for a PPPoE server, the server will reply, and they will negotiate the connection. Then you could do something like "nmcli dev eth0 disconnect; nmcli con up my-pppoe-test" and make sure you see the log messages about waiting. (In reply to Jirka Klimes from comment #3) > Created attachment 834237 [details] > [PATCH] ethernet: add reconnect delay for PPPoE connections (for master) > > Previous patch rebased to upstream master. (You can also see it in > jk/rh1023503-pppoe). I think, using time() is wrong, because it's affected by daylight saving time and resetting of the clock. I'd add a function nm_utils_get_monotonic_timestamp_ns(), that should be used whenever calculating time offsets. See th/review/jk/rh1023503-pppoe. It includes two minor fixups. (In reply to Thomas Haller from comment #5) > I think, using time() is wrong, because it's affected by daylight saving > time and resetting of the clock. aehh..., partly wrong. time() returns UTC, so no problem with DST. (In reply to Thomas Haller from comment #6) > (In reply to Thomas Haller from comment #5) > > > I think, using time() is wrong, because it's affected by daylight saving > > time and resetting of the clock. > > aehh..., partly wrong. time() returns UTC, so no problem with DST. Yeah, it might be wrong if the system suspends; so technically it should be something like clock_gettime(CLOCK_MONOTONIC). Though in this case it shouldn't be too large of an issue since it's a very short timeout, and the systems that are running PPPoE here won't usually be suspended. I'm fine with either option: (a) change to use clock_gettime() or (b) leave as-is for now. (In reply to Dan Williams from comment #4) > Looks good to me; I can test it tomorrow, but if you want to test it out > yourself, you can install the rp-pppoe package on a different machine on the > same LAN and then follow these directions: > > http://darmawan-salihun.blogspot.com/2008/12/setting-up-basic-pppoe-server- > in-linux.html > Dan, thanks for the testing instructions. It helped to set the testing environment a lot. I've found that there was a problem setting last_pppoe_time in device_state_changed(), because in that function activation request is already NULL and thus device_get_setting (dev, NM_TYPE_SETTING_PPPOE) returns NULL. So I moved setting last_pppoe_time to deactivate(), which works good, because it is called *before* clear_act_request() in device.c:nm_device_deactivate(). Also, I adapted the delay in g_timeout_add_seconds() to be the difference between now and the last time, because we don't probably want to wait the whole PPPOE_RECONNECT_DELAY interval, but only the remaining time. As for time changes Thomas proposes, I would prefer not to do changes for this patch. Rather we could adapt timestamp handling in a separate branch for whole NM if we decide to go that way. Created attachment 835238 [details]
[PATCH v2] ethernet: add reconnect delay for PPPoE connections (rh #602265)
Version 2 of the previous patch. Changes:
* setting last_pppoe_time in deactivate()
* waiting only remaining time of PPPOE_RECONNECT_DELAY
The updated patch is also at upstream branch jk/rh1023503-pppoe.
Updated commit message in the branch to reference bug 1023503 rather than 602265. Created attachment 835245 [details] [PATCH] fixup! ethernet: add reconnect delay for PPPoE connections (rh #602265) (In reply to Jirka Klimes from comment #8) > As for time changes Thomas proposes, I would prefer not to do changes for > this patch. Rather we could adapt timestamp handling in a separate branch > for whole NM if we decide to go that way. well, ok about the usage of time() or not, but what about the other bits? Notice, that the commit message does not reference this bug 1023503, but bug 602265. Current branch works well in my test setup. Thomas' fixups look good and work correctly, with the exception to the changes in act_stage1_prepare(). "if (priv->last_pppoe_time)" would cause a normal Ethernet connection to delay for up to PPPOE_RECONNECT_DELAY if the last connection was PPPoE but this connection is not PPPoE. That could be changed to: if (device_get_setting (dev, NM_TYPE_SETTING_PPPOE) && priv->last_pppoe_time) (In reply to Dan Williams from comment #12) > Current branch works well in my test setup. > > Thomas' fixups look good and work correctly, with the exception to the > changes in act_stage1_prepare(). "if (priv->last_pppoe_time)" would cause a > normal Ethernet connection to delay for up to PPPOE_RECONNECT_DELAY if the > last connection was PPPoE but this connection is not PPPoE. That could be > changed to: > > if (device_get_setting (dev, NM_TYPE_SETTING_PPPOE) && priv->last_pppoe_time) Actually, it _would be_ correct though, because last_pppoe_time only gets set, if the device has such a setting. + if (device_get_setting (device, NM_TYPE_SETTING_PPPOE)) + NM_DEVICE_ETHERNET_GET_PRIVATE (device)->last_pppoe_time = time (NULL); But it might be more explicit to check it again. I would propose a different order: if (priv->last_pppoe_time && device_get_setting (dev, NM_TYPE_SETTING_PPPOE)) because the previous is much cheaper to evaluate. (In reply to Thomas Haller from comment #13) > (In reply to Dan Williams from comment #12) > > Current branch works well in my test setup. > > > > Thomas' fixups look good and work correctly, with the exception to the > > changes in act_stage1_prepare(). "if (priv->last_pppoe_time)" would cause a > > normal Ethernet connection to delay for up to PPPOE_RECONNECT_DELAY if the > > last connection was PPPoE but this connection is not PPPoE. That could be > > changed to: > > > > if (device_get_setting (dev, NM_TYPE_SETTING_PPPOE) && priv->last_pppoe_time) > > > > Actually, it _would be_ correct though, because last_pppoe_time only gets > set, if the device has such a setting. > + if (device_get_setting (device, NM_TYPE_SETTING_PPPOE)) > + NM_DEVICE_ETHERNET_GET_PRIVATE (device)->last_pppoe_time = time > (NULL); > > But it might be more explicit to check it again. > > I would propose a different order: > if (priv->last_pppoe_time && device_get_setting (dev, > NM_TYPE_SETTING_PPPOE)) > because the previous is much cheaper to evaluate. Ok, you are right :) It is really needed. Never mind :D Cheaper order is fine with me. Fix pushed to upstream master: 5fad262 ethernet: add reconnect delay for PPPoE connections (rh #1023503) This request was resolved in Red Hat Enterprise Linux 7.0. Contact your manager or support representative in case you have further questions about the request. |