Opened 3 weeks ago

Last modified 8 days ago

#32181 new defect

remove custom Android logging, use syslog

Reported by: eighthave Owned by:
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: Android, 043-can
Cc: n8fr8, ahf, sysrqb, sisbell Actual Points:
Parent ID: Points: 1
Reviewer: Sponsor:

Description

As far as I can tell, syslog.h is fully supported for logging to Android's logcat. syslog.h is present as early as NDK r10e, if not earlier. It is also included on android-9 (2.3). My quick tests show syslog working fine on android-22 (5.1) through android-29 (10) emulators. android-24 (7.0) is currently the oldest version of Android supported by Google. syslog severity and identity tag are already mapped to the logcat versions.

@ahf is there some detail I'm missing? Were you having problems with syslog on Android?

Since the "android" logging option was never documented, I think it should just be removed, long with the add_android_log() code that was added in #24362 https://github.com/torproject/tor/commit/b0b8f7c30c8296344c77d06a24821f35b667e3dd

FYI, since syslog support is added about the same time as logcat support, #32036 is still an issue when using syslog with Android.

Child Tickets

Change History (8)

comment:1 Changed 3 weeks ago by nickm

We can deprecate direct calls to the logcat interface, or we can change "android" to be an alias for syslog, but we can't remove "android." We try very hard not to break code that uses Tor.

comment:2 Changed 3 weeks ago by eighthave

I've been working on Tor on Android for 10 years, and conducted a review of the Android apps that I know that include tor. None of them use this "android" log feature. Also, the "android" keyword was never documented. I think its safe to remove. But if you don't want to, just make "android" an alias for "syslog" and "AndroidIdentityTag" an alias for "SyslogIdentityTag".

comment:4 Changed 2 weeks ago by eighthave

In case it wasn't clear, logging to Log info android or Log info syslog produce the same messages to logcat, from what I've seen. That's one key reason why I think the code for the "android" logging method can be safely removed: it provides a duplicate code path ending in the same result.

comment:5 Changed 2 weeks ago by nickm

I'm still okay with removing the logcat code, but we do need to keep the deprecated option working for a couple of releases. We have a pretty firm policy that we try not to break working configurations without an extremely good reason. This policy might be annoying to work with some times, but it is a big part of what has kept us from breaking our downstream packages.

I believe you when you say that you've checked out all the Android apps that you know of that include Tor, but my experience with options is that there is always some super-obscure tool that we don't know about until we break it.

comment:6 Changed 2 weeks ago by eighthave

Fine by me if you want to keep it, as long as it remains undocumented and deprecated. No one should stuble upon it and try to use it, it just adds confusion. I myself wasted a chunk of time figuring out what was happening between the various logging methods, with the added pain of things like #32036

comment:7 Changed 2 weeks ago by eighthave

Here's a nice diagram and description of the Android logging system which could be helpful:
https://elinux.org/Android_Logging_System

Also, with Android, there could always be some weird edge case where the __android_log_printf() does something different than syslog, it is not a very

My personal preference when maintaining cross-platform systems is to use common APIs as much as possible that's where this bug report comes from. And I generally consider undocumented APIs as breakable, with good reason..

comment:8 Changed 8 days ago by teor

Keywords: 043-can added
Milestone: Tor: 0.4.3.x-final
Points: 1
Note: See TracTickets for help on using tickets.