From efdc4c434a01fc78286a49d4ac605a765630882d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jos=C3=A9=20Rebelo?= <joserebelo@outlook.com>
Date: Sat, 6 Aug 2022 00:31:43 +0100
Subject: [PATCH] Correctly disable file logging if initialization fails

If logging initialization failed, the file logger would not be removed
correctly, and it would log to a GB_LOGFILES_DIR_IS_UNDEFINED directory.

Remove the file logger from the root appender, regardless of it being
null or not.

The issue can be easily reproduced before the fix by throwing an
exception in FileUtils#getExternalFilesDir. Before the fix, it would
still log to the aforementioned directory.

Aditionally, update the Settings screen to reflect that writing log
files is not available in such cases.
---
 .../freeyourgadget/gadgetbridge/Logging.java  | 21 +++++++++++++------
 .../activities/SettingsActivity.java          |  6 +++++-
 app/src/main/res/values/strings.xml           |  1 +
 3 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/app/src/main/java/nodomain/freeyourgadget/gadgetbridge/Logging.java b/app/src/main/java/nodomain/freeyourgadget/gadgetbridge/Logging.java
index e18069ff4..ca4d36751 100644
--- a/app/src/main/java/nodomain/freeyourgadget/gadgetbridge/Logging.java
+++ b/app/src/main/java/nodomain/freeyourgadget/gadgetbridge/Logging.java
@@ -76,13 +76,17 @@ public abstract class Logging {
     protected abstract String createLogDirectory() throws IOException;
 
     protected void init() throws IOException {
+        rememberFileLogger();
         String dir = createLogDirectory();
         if (dir == null) {
             throw new IllegalArgumentException("log directory must not be null");
         }
         // used by assets/logback.xml since the location cannot be statically determined
         System.setProperty(PROP_LOGFILES_DIR, dir);
-        rememberFileLogger();
+    }
+
+    public boolean isInitialized() {
+        return System.getProperty(PROP_LOGFILES_DIR) != null;
     }
 
     private Logger getLogger() {
@@ -100,13 +104,18 @@ public abstract class Logging {
     private void stopFileLogger() {
         if (fileLogger != null && fileLogger.isStarted()) {
             fileLogger.stop();
-            removeFileLogger(fileLogger);
         }
+        // We still need to remove the logger from the root appender, otherwise slf4j will log to
+        // a GB_LOGFILES_DIR_IS_UNDEFINED directory (especially if something failed before we got
+        // the fileLogger
+        removeFileLogger();
     }
 
     private void rememberFileLogger() {
-        ch.qos.logback.classic.Logger root = (ch.qos.logback.classic.Logger) LoggerFactory.getLogger(Logger.ROOT_LOGGER_NAME);
-        fileLogger = (FileAppender<ILoggingEvent>) root.getAppender("FILE");
+        if (fileLogger == null) {
+            ch.qos.logback.classic.Logger root = (ch.qos.logback.classic.Logger) LoggerFactory.getLogger(Logger.ROOT_LOGGER_NAME);
+            fileLogger = (FileAppender<ILoggingEvent>) root.getAppender("FILE");
+        }
     }
 
     private void addFileLogger(Appender<ILoggingEvent> fileLogger) {
@@ -120,10 +129,10 @@ public abstract class Logging {
         }
     }
 
-    private void removeFileLogger(Appender<ILoggingEvent> fileLogger) {
+    private void removeFileLogger() {
         try {
             ch.qos.logback.classic.Logger root = (ch.qos.logback.classic.Logger) LoggerFactory.getLogger(Logger.ROOT_LOGGER_NAME);
-            if (root.isAttached(fileLogger)) {
+            if (fileLogger != null && root.isAttached(fileLogger)) {
                 root.detachAppender(fileLogger);
             }
         } catch (Throwable ex) {
diff --git a/app/src/main/java/nodomain/freeyourgadget/gadgetbridge/activities/SettingsActivity.java b/app/src/main/java/nodomain/freeyourgadget/gadgetbridge/activities/SettingsActivity.java
index 35cb2e18a..9c823efa3 100644
--- a/app/src/main/java/nodomain/freeyourgadget/gadgetbridge/activities/SettingsActivity.java
+++ b/app/src/main/java/nodomain/freeyourgadget/gadgetbridge/activities/SettingsActivity.java
@@ -184,7 +184,11 @@ public class SettingsActivity extends AbstractSettingsActivity {
 
         });
 
-
+        // If we didn't manage to initialize file logging, disable the preference
+        if (!GBApplication.getLogging().isInitialized()) {
+            pref.setEnabled(false);
+            pref.setSummary(R.string.pref_write_logfiles_not_available);
+        }
 
         pref = findPreference("language");
         pref.setOnPreferenceChangeListener(new Preference.OnPreferenceChangeListener() {
diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml
index eb1e8faee..226df4279 100644
--- a/app/src/main/res/values/strings.xml
+++ b/app/src/main/res/values/strings.xml
@@ -480,6 +480,7 @@
     <string name="watch9_pairing_tap_hint">When your watch vibrates, shake the device or press its button.</string>
     <string name="title_activity_sleepmonitor">Sleep monitor</string>
     <string name="pref_write_logfiles">Write log files</string>
+    <string name="pref_write_logfiles_not_available">File logging initialization failed, writing log files is not available</string>
     <string name="initializing">Initializing</string>
     <string name="busy_task_fetch_activity_data">Fetching activity data</string>
     <string name="sleep_activity_date_range">From %1$s to %2$s</string>