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

Load ffi symbols individually, in protected mode #10

Merged
merged 2 commits into from
Feb 10, 2023

Conversation

NiLuJe
Copy link
Member

@NiLuJe NiLuJe commented Feb 9, 2023

To avoid conflicts with koreader-base once and for all...


This change is Reviewable

To avoid conflicts with koreader-base once and for all...
Copy link
Member

@Frenzie Frenzie left a comment

Choose a reason for hiding this comment

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

Heh, I hadn't expected this to actually work. Do you have any idea of the performance impact?

@NiLuJe
Copy link
Member Author

NiLuJe commented Feb 9, 2023

Do you have any idea of the performance impact?

IIRC, error handling in Lua involves a setjmp/longjmp pair, so, depends on how good the CPUs are at jumps (plus the extra context switches it involves) ;).

But even regardless of the pcall implementation details, it is mechanically slower, if only because of the multiplication of function calls ;).

That said, we're, comparatively, doing this for a limited number of calls, once (at require-time), so I don't expect a significant impact (and it essentially future-proofs this against future conflicting additions in base).

(i.e., I'd be warier if we were doing this globally).

@Frenzie
Copy link
Member

Frenzie commented Feb 9, 2023

Well yeah, even if it were more than a hundred times slower it'd probably still be fast and unproblematic. Just curious. ;-)

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 7 of 7 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)

@NiLuJe NiLuJe merged commit 2f93beb into koreader:master Feb 10, 2023
NiLuJe added a commit to NiLuJe/koreader-base that referenced this pull request Feb 10, 2023
NiLuJe added a commit to koreader/koreader-base that referenced this pull request Feb 10, 2023
And bump lj-wpaclient to better deal with conflicts (koreader/lj-wpaclient#10)
NiLuJe added a commit to koreader/koreader that referenced this pull request Feb 10, 2023
Much easier to deal with thanks to the cleanup work done in #10062 ;).

* `carrier` is set to 1 as soon as the device is *administratively* up (in practice, as soon as we run `ifconfig up`). This is perfectly fine for `isWifiOn`, but absolutely not for `isConnected`, because we are not, actually, connected to *anything*, no attempt at associating has even been made at that point. Besides being semantically wrong, in practice, this will horribly break the connectivity check, because it expects that `isConnected` means we can talk to at least the LAN.
* Delving into the Linux docs reveals that `operstate` looks like a better candidate, as it reflects *operational status*; for Wi-Fi, that means associated and successfully authenticated. That's... closer, but still not it, because we still don't have an IP, so we technically can't talk to anything other than the AP.
* So, I've brought out the big guns (`getifaddrs`), and replicated a bit of code that I already use in the USBNetwork hack on Kindle, to detect whether we actually have an IP assigned. (Other approaches, like `/proc/net/route`, may not be entirely fool-proof, and/or get complicated when IPv6 enters the fray (which it does, on Kobo, Mk. 8+ devices are IPv6-enabled)).

TL;DR: Bunch of C via ffi, and `isConnected` now returns true only when the device is operationally up *and* we have an IP assigned.

Pulls in koreader/koreader-base#1579 & koreader/lj-wpaclient#10
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.

2 participants