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

Device: Handle network info data gathering ourselves #10139

Merged
merged 40 commits into from
Feb 20, 2023

Conversation

NiLuJe
Copy link
Member

@NiLuJe NiLuJe commented Feb 19, 2023

i.e., we now query routes, interfaces, wireless extensions & ping ourselves, dropping the dependency on specific CLI tools altogether.

Depends on koreader/koreader-base#1584


This change is Reviewable

Copy link
Member Author

@NiLuJe NiLuJe left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @NiLuJe)

@Frenzie Frenzie added this to the 2023.02 milestone Feb 19, 2023
@NiLuJe
Copy link
Member Author

NiLuJe commented Feb 20, 2023

I figured why stop there, and added ping support ;D.

On the upside, I went the extra mile with unprivileged ICMP socket support (like iputils' ping). That may help on unrooted platforms, assuming the sysctl knob doesn't block its use ;p.
(Otherwise, we, like busybox's ping, use a raw socket, and creating one is a privileged operation).

Copy link
Member Author

@NiLuJe NiLuJe left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @NiLuJe)

@NiLuJe
Copy link
Member Author

NiLuJe commented Feb 20, 2023

Also, because I asked myself this yesterday:

This is in Device instead of NetworkMgr mainly because it predates NetworkMgr ^^, and this may be platform-specific (e.g., overloaded by specific Device implementation; while Generic does not setup custom NetworkMgr methods by default, which would make this slightly less straightforward).

@NiLuJe
Copy link
Member Author

NiLuJe commented Feb 20, 2023

Here's the money shot
netinfo

Copy link
Member Author

@NiLuJe NiLuJe left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @NiLuJe)

Copy link
Member Author

@NiLuJe NiLuJe left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @NiLuJe)

@NiLuJe NiLuJe merged commit 5330c24 into koreader:master Feb 20, 2023
@poire-z
Copy link
Contributor

poire-z commented Feb 20, 2023

Not that I bother, but on the emulator (not running as root), it crashes:

02/21/23-00:56:11 ERROR Device:retrieveNetworkInfo: SIOCGIWESSID ioctl: Operation not supported
02/21/23-00:56:11 DEBUG Device:ping4: unprivileged ICMP socket: Permission denied
02/21/23-00:56:11 DEBUG Device:ping4: Opening a RAW ICMP socket requires CAP_NET_RAW capabilities!
./luajit: frontend/device/generic/device.lua:844: attempt to perform arithmetic on local 'rtt' (a nil value)
stack traceback:
        frontend/device/generic/device.lua:844: in function 'retrieveNetworkInfo'
        frontend/ui/network/networklistener.lua:243: in function 'handleEvent'

@NiLuJe
Copy link
Member Author

NiLuJe commented Feb 21, 2023

Oh, duh'; the usual "did my testing before implementing the final stupid idea" ;p.

Fix is trivial, will do so later tonight ;).

NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Feb 21, 2023
Don't blow up if we fall back to exec'ing ping
Fix koreader#10139 (comment)
NiLuJe added a commit that referenced this pull request Feb 21, 2023
Don't blow up if we fall back to exec'ing ping
Fix #10139 (comment)
table.insert(results, T(_("SSID: \"%1\""), ffi.string(essid)))
end
else
table.insert(results, _("SSID: off/any"))
Copy link
Member

Choose a reason for hiding this comment

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

What exactly does any mean here?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a fair question, I'm assuming that means a hidden SSID (that's the same string as iwconfig, FWIW).

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I didn't realize it was just copying the logic more or less verbatim. (first search result with the source, probably a bit outdated)

https://github.com/HewlettPackard/wireless-tools/blob/c1074342112d8a4cdc44275b1bc15701aaf7f30b/wireless_tools/iwconfig.c#L159-L185

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.

None yet

3 participants