Am integration into Termux #2458
Conversation
Client to use the new am implementation from this PR: termux/termux-app#2458
|
The test seems to fail because the native library for termux-shared cannot be found during the test. It works fine when running though. |
|
I am currently busy with work and lot of other stuff so will look into this in next few days. This and other changes in The new design itself is good and should be merge-able, just need time. |
|
@tareksander so with this, and the corresponding changes in termux-api changes, termux-am-socket and the termux-api-package changes, we can drop termux-am? Just want to check so that I understand it correctly (and if yes, then dependency of termux-tools on termux-am needs to be removed, or rather changed to termux-am-socket) |
|
In my opinion |
I made the changes to termux-api to make it faster, but then I realized that Replacing the use of
I'm also against removing |
|
Alright, I have been confusing the two features here, we have: Integration of termuxAm into termux-app:
We then also have the improvement of making termux-api use a unix socket instead of (termux-)am broadcast:
Please let me know if I am totally misunderstanding how something here works |
|
Basically you got everything right, but this PR can not only be used with termux-api-package, but can also speed up the communication with the Termux app for things like termux-wake-lock, termux-wake-unlock ..., basically all communication that uses intents. That's why I would prefer this to be merged, as it is a more general solution. |
|
@tareksander aha, great! I have been using a build with this PR, and the termux-api{,-package} PRs, merged for the last ~two weeks and haven't noticed any issues, so LGTM. I'll merge the other parts, and upload the termux-am-socket package. Leaving this PR for @agnostic-apollo to merge though, as it might conflict will the other pending changes |
Client to use the new am implementation from this PR: termux/termux-app#2458
Client to use the new am implementation from this PR: termux/termux-app#2458
Grimler91
force-pushed
the
am-integration
branch
from
87bb0ff
to
2c4df48
Compare
agnostic-apollo
force-pushed
the
master
branch
from
85376e2
to
6631599
Compare
|
@tareksander Is there a specific reason why you have enforced a
|
|
I wanted some sort of timeout so if something goes wrong, it doesn't block until the Termux process is restarted. I just picked some timeout with no particular reason, you can set it higher. 1s was more than enough for me, but that was on an emulator and with practically no load. |
|
Okay. Yeah, will set it at a higher value. |
agnostic-apollo
force-pushed
the
am-integration
branch
from
2c4df48
to
4392e57
Compare
|
Hey @tareksander and @Grimler91, so did some refactoring and some variable name changes. Also pushed changes to https://github.com/termux/termux-am-socket Read the commit messages for details, specially b2a6487, 3054836, 4392e57 and termux/termux-am-socket@f0473023. Test is passing now too. This should be tested and reviewed before its merged. You can get apk from https://github.com/termux/termux-app/actions/runs/2192198078 and I tested from within termux and root and it worked on Android 7 and 11. From other app with |
|
Great! Making the socket API more general is something I would have needed for the new plugin system anyways. I'll try and do better with variable naming in the future, this is the first time I really coded something others have to understand outside of class. I can't test anything new, my devices are also Android 7 and 11. I do have some Android emulators set up though, but those are x86. |
|
After Installing latest termux-am-socket (and termux-api, and termux-api-package), and then installing app from this branch I get an error on socket creation which I can't seem to figure out the reason for: All log outputReport InfoUser Action: TermuxAm Socket Server ErrorError Code: TermuxAm Socket Server Run ConfigPath: Termux App InfoAPP_NAME: Device InfoSoftwareOS_VERSION: HardwareMANUFACTURER: Have I perhaps missed a pending PR somewhere? |
Don't feel taken down by the refactor even a bit, it was seriously a great first version, specially if it was the first time after uni. As for the non-general design, that was an issue but writing better designs takes experience and time to learn, read other people's code and see how they do it, eventually you get better. One should also think a few steps ahead for how else the code may be used in future, so that you need fewer changes in future and don't waste time making lot of changes. Even current design may have issues, I guess time will tell.
Yeah, variable names should be more descriptive. Deciding on variable names is like one of the hardest and time taking part.
The testing is done for missed cases that I may not have tested or for device or build specific issues. I will do a basic test on android 12 and 13 as well in avd. |
Proguard was removing specific methods only used by JNI for The And https://github.com/termux/termux-api-package/blob/v0.57/termux-api.c#L25
And someone builds their own lineageOS. :D I need to start doing that, but first need to get a 1TB SSD. :p |
Thanks! Seems to work fine both on my aarch64 phone and an arm tablet as normal user Running a termux-api command from a su shell seem to work as well, but not when run from a tsu shell (then it just hangs), due to some env var I guess.
Yeah, so I have only myself to blame for all the bugs in the ROM ;) yeah, it takes an ridiculous amount of disk space, especially for later android versions.. |
Great.
I installed I guess pull request should be mergable, unless someone has an objection.
Your security patch is even older than mine! :p
Yeah, that's why I am gonna wait for the 1TB SSD, gonna need lot of space to build different test versions. |
…ilesystem. The UID of connecting programs is automatically checked against the processes UID and connections where the UID doesn't match are automatically rejected and logged. Changed: LocalSocketListener now uses sockets in the filesystem.
…s now part of the Termux github organization.
Using the TermuxConstants.TERMUX_FILES_DIR variable to get full path TermuxConstants.TERMUX_FILES_DIR_PATH/api/am-socket.
…d.termux.plugins.TermuxPluginUtils` This will allow plugins and `termux-shared` library to trigger plugin error notifications too and process plugin command results.
…() to `sendCrashReportNotification()`
…ring()` and `getArgumentsMarkdownString()` functions for external usage
…by `CrashHandler` receives an exception on a non main thread Rename function that should be used by main thread of apps to `setDefaultCrashHandler()`. Functions for other threads will be added in a later commit.
…shHandler` as the `UncaughtExceptionHandler`
…eFile()` in addition to allowed file types
… log errors that may contain potentially private info unless log level is debug or higher Execution commands and other errors that may contain potentially private info should not be logged unless user has explicitly allowed it since apps with `READ_LOGS` permission would be able to read the data. A notification for failed executions commands would still be shown if enabled and required.
…create them at application startup. The termux files directory will also be checked and created if required at startup and code related to it will only be run if it is accessible. This can later also be used for init execution commands. The `TERMUX_APP.APPS_DIR_PATH` will act as app specific directory for `termux-app` app related files. Other plugin apps will have their own directories under `TERMUX_APPS_DIR_PATH` if required.
… `PackageManager` and `Libcore`
…id from `ActivityManager`
The user can add `run-termux-am-socket-server=false` entry to `termux.properties` file to disable the `termux-am` server to run at app startup which is connected to by `$PREFIX/bin/termux-am` from the `termux-am-socket` package. The default value is `true`. Changes require `termux-app` to be force stopped and restarted to provide consistent state for all termux sessions and tasks. The prop will be used in a later commit.
…make client handling abstract - Added `LocalSocketManager` to manage the server, `LocalServerSocket` to represent server socket, `LocalClientSocket` to represent client socket, `LocalSocketRunConfig` to store server run config and `ILocalSocketManager` as interface for the `LocalSocketManager` to handle callbacks from the server to handle clients. - Added support to get full `PeerCred` for client socket, including `pid`, `pname`, `uid`, `uname`, `gid`, `gname` and `cmdline` instead of just `uid`. This should provide more info for error logs about which client failed or tried to connect in case of disallowed clients. Some data is filled in native code and some in java. Native support for added to get process name and `cmdline` of a process with a specific pid. - Added `JniResult` to get results for JNI calls. Previously only an int was returned and incomplete errors logged. With `JniResult`, both `retval` and `errno` will be returned and full error messages in `errmsg`, including all `strerror()` output for `errno`s. This would provide more helpful info on errors. - Added `Error` support via `LocalSocketErrno` which contains full error messages and stacktraces for all native and java calls, allowing much better error reporting to users and devs. The errors will be logged by `LocalSocketManagerClientBase` if log level is debug or higher since `PeerCred` `cmdline` may contain private info of users. - Added support in java to check if socket path was an absolute path and not greater than `108` bytes, after canonicalizing it since otherwise it would result in creation of useless parent directories on failure. - Added `readDataOnInputStream()` and `sendDataToOutputStream()` functions to `LocalClientSocket` so that server manager client can easily read and send data. - Renamed the variables and functions as per convention, specially one letter variables. https://source.android.com/setup/contribute/code-style#follow-field-naming-conventions - Rename `local-filesystem-socket` to `local-filesystem` since abstract namespace sockets can also be created. - Previously, it was assumed that all local server would expect a shell command string that should be converted to command args with `ArgumentTokenizer` and then should be passed to `LocalSocketHandler.handle()` and then result sent back to client with exit code, stdout and stderr, but there could be any kind of servers in which behaviour is different. Such client handling should not be hard coded and the server manager client should handle the client themselves however they like, including closing the client socket. This will now be done with `ILocalSocketManager. onClientAccepted(LocalSocketManager, LocalClientSocket)`. - Ensure app does not crash if `local-socket` library is not found or for any other exceptions in the server since anything running in the `Application` class is critical that it does not fail since user would not be able to recover from it, specially non rooted users without SAF support to disable the server with a prop. - Make sure all reasonable JNI exceptions are caught instead of crashing the app. - Fixed issue where client logic (`LocalSocketHandler.handle()` was being run in the same thread as the new client acceptable thread, basically blocking new clients until previous client's am command was fully processed. Now all client interface callbacks are started in new threads by `LocalSocketManager`. - Fix bug where timeout would not be greater than `1000ms` due to only using `tv_usec` which caps at `999,999`.
The `AmSocketServer` now handles the entire logic for processing of am commands sent by clients and its results. This can be used by other apps as well to run their own am servers. The server started by `termux-app` will be managed by `TermuxAmSocketServer`. Read their javadocs for details. The extended implementation `TermuxAmSocketServerClient` of `AmSocketServer.AmSocketServerClient`/`ILocalSocketManager` will also send a plugin error notification for all errors to the user instead of just logging to logcat since users are not very good at checking those, this should save dev time debugging problems. We may need to ignore notifications for some errors like broken pipe, based on their `Error` objects if they are normally expected, this requires further investigation. The `TERMUX_APP_AM_SOCKET_SERVER_ENABLED` env variable will also be exported for all shell sessions and tasks for whether the server was successfully started on app startup. The user can disable the server by adding "run-termux-am-socket-server=false" to the "~/.termux/termux.properties" as implemented in 5f8a922. The env variable will be checked by `$PREFIX/bin/termux-am` before attempting to connect. The new path for the server socket is `/data/data/com.termux/files/apps/termux-app/termux-am/am.sock` as per `TERMUX_APP.APPS_DIR_PATH` added in bcd8f4c.
``` Exception in createServerSocketNative(): java.lang.NoSuchMethodError: no non-static method "Lcom/termux/shared/jni/models/JniResult;.<init>(IILjava/lang/String;I)V" at com.termux.shared.net.socket.local.LocalSocketManager.createServerSocketNative(Native Method) at com.termux.shared.net.socket.local.LocalSocketManager.createServerSocket(LocalSocketManager.java:125) at com.termux.shared.net.socket.local.LocalServerSocket.start(LocalServerSocket.java:100) at com.termux.shared.net.socket.local.LocalSocketManager.start(LocalSocketManager.java:84) at com.termux.shared.shell.am.AmSocketServer.start(AmSocketServer.java:68) at com.termux.shared.termux.shell.am.TermuxAmSocketServer.start(TermuxAmSocketServer.java:101) at com.termux.shared.termux.shell.am.TermuxAmSocketServer.setupTermuxAmSocketServer(TermuxAmSocketServer.java:77) at com.termux.app.TermuxApplication.onCreate(TermuxApplication.java:53) at android.app.Instrumentation.callApplicationOnCreate(Instrumentation.java:1192) at android.app.ActivityThread.handleBindApplication(ActivityThread.java:6719) at android.app.ActivityThread.access$1300(ActivityThread.java:237) at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1913) at android.os.Handler.dispatchMessage(Handler.java:106) at android.os.Looper.loop(Looper.java:223) at android.app.ActivityThread.main(ActivityThread.java:7664) at java.lang.reflect.Method.invoke(Native Method) at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:592) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:947) ```
agnostic-apollo
force-pushed
the
am-integration
branch
from
45450eb
to
7f7d889
Compare
Client to use the new am implementation from this PR: termux/termux-app#2458
This PR includes:
amusing the Android API (Am.java)amimplementationThe new
amimplementation lacks some features (some warnings can't be displayed because the Android API doesn't give them), but is about 10x faster, because there is no need to create a dalvik VM foramto run, the process is already there.Because the new implementation isn't a 1:1 copy of
amit shouldn't replaceam, but can be used as a replacement if tested, e.g. for termux-wake-lock, termux-notification... to get better speed.More details can be found in this PR that gave me the idea.