commit 3afcb23de6e55746fff87f2854e9259d210e9cfc
author Brian Norris <briannorris@chromium.org> Tue Jul 28 16:58:14 2020 -0700
committer Commit Bot <commit-bot@chromium.org> Fri Jul 31 17:37:08 2020 +0000
tree 4035b7269e813a58150bc0763c5beb49f91f7356
parent 410b3f1fb29ce54a2cc9407377a7d0e3451c2785

shill: log early messages

shill_main's OnStartup() sets up logging (stderr for --foreground;
syslog for background (default)).

DaemonTask::Init() constructs most of our singletons (ProcessManager,
Manager, etc.).

There's no reason for ordering one or the other, except to determine
where the logging for Init() goes. Let's let logging for Init() go to
the right place (i.e., background = syslog) by setting up logging before
we go through most other initialization.

While at it, we can simplify the callback hierarchy by inlining
OnStartup() into main(). Note that we still need to construct
ShillDaemon first, because we rely on its embedded AtExitManager for
things like Minjail::GetInstance() / lazy pointer initialization.

This makes things like the following a little more useful, as there are
potentially some manager- or property-related logs that come in the
constructors:

  stop shill; start shill SHILL_LOG_LEVEL=-3 SHILL_LOG_SCOPES="+manager+property+profile"

BUG=none
TEST=see above

Change-Id: I676b3e06f1bfea2d878bf36765fa77e4b5d61c36
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2325076
Tested-by: Brian Norris <briannorris@chromium.org>
Commit-Queue: Brian Norris <briannorris@chromium.org>
Reviewed-by: Alex Khouderchah <akhouderchah@chromium.org>

shill/manager.cc

diff --git a/shill/manager.cc b/shill/manager.cc
index 909396f..6e9d570 100644
--- a/shill/manager.cc
+++ b/shill/manager.cc
@@ -1114,7 +1114,7 @@
 }
 
 void Manager::SetIgnoreUnknownEthernet(bool ignore) {
-  LOG(INFO) << __func__ << "(" << ignore << ")";
+  SLOG(this, 2) << __func__ << "(" << ignore << ")";
   ignore_unknown_ethernet_ = ignore;
 }
 

shill/shill_daemon.cc

diff --git a/shill/shill_daemon.cc b/shill/shill_daemon.cc
index cef55a1..6229db3 100644
--- a/shill/shill_daemon.cc
+++ b/shill/shill_daemon.cc
@@ -11,10 +11,9 @@
 
 namespace shill {
 
-ShillDaemon::ShillDaemon(const base::Closure& startup_callback,
-                         const shill::DaemonTask::Settings& settings,
+ShillDaemon::ShillDaemon(const shill::DaemonTask::Settings& settings,
                          Config* config)
-    : daemon_task_(settings, config), startup_callback_(startup_callback) {}
+    : daemon_task_(settings, config) {}
 
 ShillDaemon::~ShillDaemon() = default;
 
@@ -27,9 +26,6 @@
 
   daemon_task_.Init();
 
-  // Signal that we've acquired all resources.
-  startup_callback_.Run();
-
   return EX_OK;
 }
 

shill/shill_daemon.h

diff --git a/shill/shill_daemon.h b/shill/shill_daemon.h
index a7d1f7c..0a4f20f 100644
--- a/shill/shill_daemon.h
+++ b/shill/shill_daemon.h
@@ -5,7 +5,6 @@
 #ifndef SHILL_SHILL_DAEMON_H_
 #define SHILL_SHILL_DAEMON_H_
 
-#include <base/callback.h>
 #include <base/macros.h>
 #include <brillo/daemons/daemon.h>
 
@@ -20,9 +19,7 @@
 // to DaemonTask, and additionally overrides methods of brillo::Daemon.
 class ShillDaemon : public brillo::Daemon {
  public:
-  ShillDaemon(const base::Closure& startup_callback,
-              const shill::DaemonTask::Settings& settings,
-              Config* config);
+  ShillDaemon(const shill::DaemonTask::Settings& settings, Config* config);
   virtual ~ShillDaemon();
 
  private:
@@ -31,7 +28,6 @@
   void OnShutdown(int* return_code) override;
 
   DaemonTask daemon_task_;
-  base::Closure startup_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(ShillDaemon);
 };

shill/shill_main.cc

diff --git a/shill/shill_main.cc b/shill/shill_main.cc
index 4c27d84..3a4e8c8 100644
--- a/shill/shill_main.cc
+++ b/shill/shill_main.cc
@@ -142,11 +142,6 @@
   }
 }
 
-void OnStartup(const char* daemon_name, base::CommandLine* cl) {
-  SetupLogging(cl->HasSwitch(switches::kForeground), daemon_name);
-  shill::SetLogLevelFromCommandLine(cl);
-}
-
 }  // namespace
 
 int main(int argc, char** argv) {
@@ -225,9 +220,15 @@
 #endif  // DISABLE_DHCPV6
 
   shill::Config config;
+  // Construct the daemon first, so we get our AtExitManager.
+  shill::ShillDaemon daemon(settings, &config);
 
-  shill::ShillDaemon daemon(base::Bind(&OnStartup, argv[0], cl), settings,
-                            &config);
+  // Configure logging before we start anything else, so early log messages go
+  // to a consistent place.
+  SetupLogging(cl->HasSwitch(switches::kForeground), argv[0]);
+  shill::SetLogLevelFromCommandLine(cl);
+
+  // Go for it!
   daemon.Run();
 
   LOG(INFO) << "Process exiting.";