Logger: new class BZFile (#19959)

* add class BZFile

* inline

* cleanup includes

* stack bzfile

* log first error

* remove assert

* rename log_err to err_logged

* assert in destructor

* LOGE

* don't assert statement

* assert error of fclose

selfdrive/loggerd/bootlog.cc

@@ -1,12 +1,6 @@
 #include <assert.h>
 #include <string>
-#include <cstdio>
-
-#include <bzlib.h>
-
 #include "common/swaglog.h"
-#include "common/util.h"
-#include "messaging.hpp"
 #include "logger.h"
 
 int main(int argc, char** argv) {
@@ -26,30 +20,13 @@ int main(int argc, char** argv) {
   int r = logger_mkpath((char*)path.c_str());
   assert(r == 0);
 
-  FILE * file = fopen(path.c_str(), "wb");
-  assert(file != nullptr);
-
-  // Open as bz2
-  int bzerror;
-  BZFILE* bz_file = BZ2_bzWriteOpen(&bzerror, file, 9, 0, 30);
-  assert(bzerror == BZ_OK);
+  BZFile bz_file(path.c_str());
 
   // Write initdata
-  kj::Array<capnp::word> init_msg = logger_build_init_data();
-  auto bytes = init_msg.asBytes();
-  BZ2_bzWrite(&bzerror, bz_file, bytes.begin(), bytes.size());
-  assert(bzerror == BZ_OK);
+  bz_file.write(logger_build_init_data().asBytes());
 
   // Write bootlog
-  kj::Array<capnp::word> boot_msg = logger_build_boot();
-  bytes = boot_msg.asBytes();
-  BZ2_bzWrite(&bzerror, bz_file, bytes.begin(), bytes.size());
-  assert(bzerror == BZ_OK);
+  bz_file.write(logger_build_boot().asBytes());
 
-  // Close bz2 and file
-  BZ2_bzWriteClose(&bzerror, bz_file, 0, NULL, NULL);
-  assert(bzerror == BZ_OK);
-
-  fclose(file);
   return 0;
 }

selfdrive/loggerd/logger.cc

@@ -9,8 +9,6 @@
 #include <unistd.h>
 #include <sys/stat.h>
 
-#include <pthread.h>
-#include <bzlib.h>
 #include <iostream>
 #include <fstream>
 #include <streambuf>
@@ -20,7 +18,6 @@
 
 #include "common/swaglog.h"
 #include "common/params.h"
-#include "common/util.h"
 #include "common/version.h"
 #include "messaging.hpp"
 #include "logger.h"
@@ -200,46 +197,14 @@ static LoggerHandle* logger_open(LoggerState *s, const char* root_path) {
   if (lock_file == NULL) return NULL;
   fclose(lock_file);
 
-  h->log_file = fopen(h->log_path, "wb");
-  if (h->log_file == NULL) goto fail;
-
+  h->log = std::make_unique<BZFile>(h->log_path);
   if (s->has_qlog) {
-    h->qlog_file = fopen(h->qlog_path, "wb");
-    if (h->qlog_file == NULL) goto fail;
-  }
-
-  int bzerror;
-  h->bz_file = BZ2_bzWriteOpen(&bzerror, h->log_file, 9, 0, 30);
-  if (bzerror != BZ_OK) goto fail;
-
-  if (s->has_qlog) {
-    h->bz_qlog = BZ2_bzWriteOpen(&bzerror, h->qlog_file, 9, 0, 30);
-    if (bzerror != BZ_OK) goto fail;
+    h->q_log = std::make_unique<BZFile>(h->qlog_path);
   }
 
   pthread_mutex_init(&h->lock, NULL);
   h->refcnt++;
   return h;
-
-fail:
-  LOGE("logger failed to open files");
-  if (h->bz_file) {
-    BZ2_bzWriteClose(&bzerror, h->bz_file, 0, NULL, NULL);
-    h->bz_file = NULL;
-  }
-  if (h->bz_qlog) {
-    BZ2_bzWriteClose(&bzerror, h->bz_qlog, 0, NULL, NULL);
-    h->bz_qlog = NULL;
-  }
-  if (h->qlog_file) {
-    fclose(h->qlog_file);
-    h->qlog_file = NULL;
-  }
-  if (h->log_file) {
-    fclose(h->log_file);
-    h->log_file = NULL;
-  }
-  return NULL;
 }
 
 int logger_next(LoggerState *s, const char* root_path,
@@ -310,11 +275,9 @@ void logger_close(LoggerState *s) {
 void lh_log(LoggerHandle* h, uint8_t* data, size_t data_size, bool in_qlog) {
   pthread_mutex_lock(&h->lock);
   assert(h->refcnt > 0);
-  int bzerror;
-  BZ2_bzWrite(&bzerror, h->bz_file, data, data_size);
-
-  if (in_qlog && h->bz_qlog != NULL) {
-    BZ2_bzWrite(&bzerror, h->bz_qlog, data, data_size);
+  h->log->write(data, data_size);
+  if (in_qlog && h->q_log) {
+    h->q_log->write(data, data_size);
   }
   pthread_mutex_unlock(&h->lock);
 }
@@ -324,22 +287,8 @@ void lh_close(LoggerHandle* h) {
   assert(h->refcnt > 0);
   h->refcnt--;
   if (h->refcnt == 0) {
-    if (h->bz_file){
-      int bzerror;
-      BZ2_bzWriteClose(&bzerror, h->bz_file, 0, NULL, NULL);
-      h->bz_file = NULL;
-    }
-    if (h->bz_qlog){
-      int bzerror;
-      BZ2_bzWriteClose(&bzerror, h->bz_qlog, 0, NULL, NULL);
-      h->bz_qlog = NULL;
-    }
-    if (h->qlog_file) {
-      fclose(h->qlog_file);
-      h->qlog_file = NULL;
-    }
-    fclose(h->log_file);
-    h->log_file = NULL;
+    h->log.reset(nullptr);
+    h->q_log.reset(nullptr);
     unlink(h->lock_path);
     pthread_mutex_unlock(&h->lock);
     pthread_mutex_destroy(&h->lock);

selfdrive/loggerd/logger.h

@@ -3,9 +3,11 @@
 #include <stdio.h>
 #include <stdint.h>
 #include <pthread.h>
+#include <memory>
 #include <bzlib.h>
 #include <kj/array.h>
 #include <capnp/serialize.h>
+#include "common/util.h"
 
 #if defined(QCOM) || defined(QCOM2)
 const std::string LOG_ROOT = "/data/media/0/realdata";
@@ -15,18 +17,48 @@ const std::string LOG_ROOT = util::getenv_default("HOME", "/.comma/media/0/reald
 
 #define LOGGER_MAX_HANDLES 16
 
+class BZFile {
+ public:
+  BZFile(const char* path) {
+    file = fopen(path, "wb");
+    assert(file != nullptr);
+    int bzerror;
+    bz_file = BZ2_bzWriteOpen(&bzerror, file, 9, 0, 30);
+    assert(bzerror == BZ_OK);
+  }
+  ~BZFile() {
+    int bzerror;
+    BZ2_bzWriteClose(&bzerror, bz_file, 0, nullptr, nullptr);
+    if (bzerror != BZ_OK) {
+      LOGE("BZ2_bzWriteClose error, bzerror=%d", bzerror);
+    }
+    int err = fclose(file);
+    assert(err == 0);
+  }
+  inline void write(void* data, size_t size) {
+    int bzerror;
+    BZ2_bzWrite(&bzerror, bz_file, data, size);
+    if (bzerror != BZ_OK && !error_logged) {
+      LOGE("BZ2_bzWrite error, bzerror=%d", bzerror);
+      error_logged = true;
+    }
+  }
+  inline void write(kj::ArrayPtr<capnp::byte> array) { write(array.begin(), array.size()); }
+
+ private:
+  bool error_logged = false;
+  FILE* file = nullptr;
+  BZFILE* bz_file = nullptr;
+};
+
 typedef struct LoggerHandle {
   pthread_mutex_t lock;
   int refcnt;
   char segment_path[4096];
   char log_path[4096];
-  char lock_path[4096];
-  FILE* log_file;
-  BZFILE* bz_file;
-
-  FILE* qlog_file;
   char qlog_path[4096];
-  BZFILE* bz_qlog;
+  char lock_path[4096];
+  std::unique_ptr<BZFile> log, q_log;
 } LoggerHandle;
 
 typedef struct LoggerState {