Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

luci-proto-3g/ppp/pppossh: fix being unable to set keepalive to 0 #7392

Closed
wants to merge 1 commit into from

Conversation

Preport
Copy link
Contributor

@Preport Preport commented Nov 14, 2024

Since on openwrt keepalive option defaults to "5 1" when it's not defined
(see https://github.com/openwrt/openwrt/blob/6720c4ccba256186bf2f1b1edadb851c447e62a5/package/network/services/ppp/files/ppp.sh#L128).
Users must be able to set it to 0 to ignore connection failures.

  • This PR is not from my main or master branch 💩, but a separate branch ✅
  • Each commit has a valid ✒️ Signed-off-by: <[email protected]> row (via git commit --signoff)
  • Each commit and PR title has a valid 📝 <package name>: title first line subject for packages
  • Tested on: (mt7621 mipsel, openwrt 23.05.04 firefox) ✅
  • Mention: @jow-
  • Closes: LCP echo failure threshold 0 does not behave as described in LuCI #2112
  • Description: (describe the changes proposed in this PR)

Since on openwrt keepalive option defaults to "5 1" when it's not defined (https://github.com/openwrt/openwrt/blob/6720c4ccba256186bf2f1b1edadb851c447e62a5/package/network/services/ppp/files/ppp.sh#L128).
Users must be able to set it to 0 to ignore connection failures.

Signed-off-by: Erdem Gez <[email protected]>
if (f == null || f == '' || isNaN(f))
f = 0;
if (f === '' || isNaN(f))
f = null;

if (i == null || i == '' || isNaN(i) || i < 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't interval be zero also? I don't know for sure. I get the impression that setting it to zero should turn it off. Does that happen in the init scripts?

https://github.com/ppp-project/ppp/blob/616102e93be1c629821cafd83a89d9e6c0467033/pppd/lcp.c#L2492-L2493

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as failure is less than zero it should work.

https://github.com/openwrt/openwrt/blob/6720c4ccba256186bf2f1b1edadb851c447e62a5/package/network/services/ppp/files/ppp.sh#L133

This just means that setting it to zero disables it (in the init scipt) - which is inaccessible with thei < 1 behaviour. I'm not nit-picking your code- just what's going on here in general. That whole logic could do with a rewrite. Are you game?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can't be less than 1 on luci since there's a check for that

But if we want users to be able to disable keepalive through the interface by either setting the failure or/and the interval to 0 then yes we'd need to edit the init script too.

I think the init script should be edited regardless to always set the keepalive instead of omitting it when it's 0 since that way pppd may use whatever it's set to on options file.
https://github.com/openwrt/openwrt/blob/6720c4ccba256186bf2f1b1edadb851c447e62a5/package/network/services/ppp/files/etc/ppp/options

@systemcrash
Copy link
Contributor

@feckert do you also use some flavour of PPP?

@systemcrash
Copy link
Contributor

Closed by f3d26a2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LCP echo failure threshold 0 does not behave as described in LuCI
2 participants