regression: pppoe discovery fails when pppoe_verbose is set

[ipaton1]

commit 1c082acf77e6256d9e47a3c9490b7615d591e67b pppoe: Show verbose information about all concentrator when pppoe-verbose option is set completely changes the behaviour of how discovery works if pppoe_verbose is set >= 1

what happens is that the plugin sends PADI, receives PADO from the AC and then waits 5 seconds before sending PADR.

In my case this completely breaks connectivity as the AC appears to timeout the PADO after something less than 5 seconds and then does not reply to the, delayed, PADR. Can't tell exactly what the AC is doing as that belongs to the ISP which is BT in this instance.

This part at the bottom of the commit, in waitForPADO is the issue

            conn->numPADOs++;
-           if (pc.acNameOK && pc.serviceNameOK) {
+           if (pc.acNameOK && pc.serviceNameOK && conn->discoveryState != STATE_RECEIVED_PADO) {
                memcpy(conn->peerEth, packet.ethHdr.h_source, ETH_ALEN);
                conn->discoveryState = STATE_RECEIVED_PADO;
-               break;
            }
        }
-    } while (conn->discoveryState != STATE_RECEIVED_PADO);
+    } while (pppoe_verbose >= 1 || conn->discoveryState != STATE_RECEIVED_PADO);
 }

by removing the break and causing the while to always be true whenever pppoe_verbose is set you're left with the only way out of the loop being to hit the timeout.

The timeout that's passed in comes from conn->discoveryTimeout in discovery1 which derives from the default value for padi-timeout which is 5 seconds.

It sort of appears that this cahnge was made to allow pppoe-discovery.c to use discovery1 instead of duplicating some code. However it's meeting it's goals by overloading a different meaning onto pppoe_verbose and when that causes a drastic change of behaviour in pppd when using the plugin then I don't think that's acceptable.

If pppoe-discovery.c wants to change the behaviour of the loop to do something different when linked with pppoe-discovery that's fine, but it should enable that functionality without overloading pppoe_verbose and breaking connectivity when used in the plugin.

[ipaton1]

should have added that 2.4.8 & 2.4.9 work fine, we were on 2.4.8 and the upgrade to 2.5.0 broke everything so we went back and forward for a while trying to pin down what had changed as the upgrade to 2.5.0 happened almost six months ago. Aparrently the DSL line is quite stable and hadn't dropped once in all that time so 2.4.8 was still running until a recent power outage.

Issue still aparrent up to todays head.

[Neustradamus]

@paulusmack, @enaess: What do you think?

[paulusmack]

@pali any comment?

I guess that if you want to know about all concentrators, it is reasonable to wait a little while after the first response to see if any others come in. But it sounds like 5 seconds is too long, at least with this ISP. Maybe 1 or 2 seconds would work better.

[paulusmack]

I got a reply from @pali via email, with a patch.

@ipaton1 please test the pppoe-verbose-fix branch in this repository and tell me whether it fixes your problem.

[Neustradamus]

@ipaton1: @paulusmack has done a commit with @pali patch here: https://github.com/ppp-project/ppp/tree/pppoe-verbose-fix It is good for you? It can be merged?

[andrewbaxter]

I just found this issue after a long night trying to get ppp working on Arch with 2.5.0.

I tried master (which I believe includes #492) with pppoe-verbose 10. With the system 2.5.0 I was seeing the 5s delay and PADS was never sent by the remote end. With the git version it connected immediately.

Would it be possible to cut a release with this fix?

[Neustradamus]

@andrewbaxter: A 2.5.1 release will arrive soon by @paulusmack:

• https://github.com/ppp-project/ppp/issues/460