stat_starting is always 0 in pppoe

[psgreco]

61862862a9fa24db4f16c24db1aed1f1a5f0be19 implemented connection limiting in pppoe, but since stat_starting is never incremented/decremented, the code is never triggered.

[DmitriyEshenko]

Hi @psgreco did you test this [pppoe]max-starting=1 and connect 2 clients simultaneously?

[psgreco]

Hi @psgreco did you test this [pppoe]max-starting=1 and connect 2 clients simultaneously?

Yes, exactly that, and the pppoe: Count of starting sessions > conf_max_starting, droping connection... message is never logged. Since I'm only using pppoe, this patch works for me as a temporary measure.

diff --git a/accel-pppd/ctrl/pppoe/pppoe.c b/accel-pppd/ctrl/pppoe/pppoe.c
index 16b4dfb..671233d 100644
--- a/accel-pppd/ctrl/pppoe/pppoe.c
+++ b/accel-pppd/ctrl/pppoe/pppoe.c
@@ -968,7 +968,7 @@ static void pppoe_recv_PADI(struct pppoe_serv_t *serv, uint8_t *pack, int size)

        if (conf_max_sessions && ap_session_stat.active + ap_session_stat.starting >= conf_max_sessions)
                return;
-       if (conf_max_starting > 0 && stat_starting >= conf_max_starting) {
+       if (conf_max_starting > 0 && ap_session_stat.starting >= conf_max_starting) {
                log_warn("pppoe: Count of starting sessions  >  conf_max_starting, droping connection...\n");
                return;

[themiron]

@psgreco right, commits 02008c74a19c538ff7d9ce643c8cd4c738886196 and 61862862a9fa24db4f16c24db1aed1f1a5f0be19 make no really sense and done in quite wrong and undocumented way. I'm going to drop them unless reimplement in different way. please do not use it at the moment

[themiron]

@psgreco please try #121 patch

[psgreco]

I can try the patch, and I like it as a "global max-starting", but currently, the only one that doesn't track stat_starting is pppoe, so this patch would break the functionality for those who want per-technology limit. I think the correct solution would be to keep track of stat_starting in pppoe too, and maybe use this as a global limit

[themiron]

yes, but there’s none who want it per-proto afaik, it was neither announced nor documented