pppoe: padi-timeout & padi-attempts config options not used

[ipaton1]

commit 8e77984 rp-pppoe plugin: Add options to tune discovery timeout and number of attempts added config options padi-timeout & padi-attempts. However what's actually used during the discovery phase are conn->discoveryTimeout & conn->discoveryAttempts

The commit appears to copy the values into the conn structure with a change to PPPOEInitDevice, however this happens before the config file is read meaning that the used values are just the fixed defaults.

What's required is to copy the values in PPPOEConnectDevice as that happens after the config as been parsed. Something like this alongside the other values from the config file

diff --git a/pppd/plugins/pppoe/plugin.c b/pppd/plugins/pppoe/plugin.c
index 7d4709e..4af680e 100644
--- a/pppd/plugins/pppoe/plugin.c
+++ b/pppd/plugins/pppoe/plugin.c
@@ -195,6 +195,9 @@ PPPOEConnectDevice(void)
        memcpy(conn->hostUniq.payload, &pid, sizeof(pid));
     }
 
+    conn->discoveryTimeout=pppoe_padi_timeout;
+    conn->discoveryAttempts=pppoe_padi_attempts;
+
     conn->acName = acName;
     conn->serviceName = pppd_pppoe_service;
     ppp_set_pppdevnam(devnam);

discovered when tracking down the timeout issue in #484 and trying to change the value in the config that looked like it should alter the 5s timeout I was hitting but didn't

[Neustradamus]

@paulusmack, @enaess: What do you think?

[paulusmack]

I agree there's a problem - PPPOEInitDevice() is called from PPPoEDevnameHook(), which is called when the ethernet device name is seen while parsing the arguments. So if the pppoe-padi-timeout or pppoe-padi-attempts options come before the ethernet device name, they'll take effect, but if they come after, they'll be ignored. Not ideal.

[paulusmack]

I pushed a proposed fix to the pppoe-options branch in this repository. @ipaton1 please test it and confirm whether it fixes the problem. If it does I'll merge it.

[Neustradamus]

@ipaton1: @paulusmack has done it here: https://github.com/ppp-project/ppp/tree/pppoe-options
It is good for you? It can be merged?