Move free disk space check to session_manager.

components/arc/arc_bridge_bootstrap.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/arc/arc_bridge_bootstrap.h"
 
 #include <fcntl.h>
 #include <grp.h>
 #include <poll.h>
 #include <unistd.h>
@@ skipping 28 matching lines @@
 
 namespace arc {
 
 namespace {
 
 const base::FilePath::CharType kArcBridgeSocketPath[] =
     FILE_PATH_LITERAL("/var/run/chrome/arc_bridge.sock");
 
 const char kArcBridgeSocketGroup[] = "arc-bridge";
 
-const base::FilePath::CharType kDiskCheckPath[] = "/home";
-
-const int64_t kCriticalDiskFreeBytes = 64 << 20;  // 64MB
+// TODO(hidehiko): Share the constant between Chrome and ChromeOS.

[Daniel Erat, 2016/10/06 14:53:54]

i agree that this constant should be moved to system_api so it can be shared,
but i don't think we want these strings to leak out of SessionManagerClient;
they feel like an implementation detail of the d-bus interface.

could you instead move this constant to session_manager_client.cc and introduce
a new enum in SessionManagerClient? e.g.

  enum class StartArcInstanceResult {
    SUCCESS,  
    LOW_FREE_DISK_SPACE,
    OTHER_ERROR,
  };

[hidehiko, 2016/10/07 05:28:38]

Done.

+constexpr char kArcLowDiskError[] =
+    "org.chromium.SessionManagerInterface.LowFreeDisk";
 
 // This is called when StopArcInstance D-Bus method completes. Since we have the
 // ArcInstanceStopped() callback and are notified if StartArcInstance fails, we
 // don't need to do anything when StopArcInstance completes.
 void DoNothingInstanceStopped(bool) {}
 
 chromeos::SessionManagerClient* GetSessionManagerClient() {
   // If the DBusThreadManager or the SessionManagerClient aren't available,
   // there isn't much we can do. This should only happen when running tests.
   if (!chromeos::DBusThreadManager::IsInitialized() ||
@@ skipping 46 matching lines @@
 // TODO(hidehiko): Refactor more to make this class unittest-able, for at least
 // state-machine part.
 class ArcBridgeBootstrapImpl : public ArcBridgeBootstrap,
                                public chromeos::SessionManagerClient::Observer {
  public:
   // The possible states of the bootstrap connection.  In the normal flow,
   // the state changes in the following sequence:
   //
   // NOT_STARTED
   //   Start() ->
-  // CHECKING_DISK_SPACE
-  //   OnDiskSpaceChecked() ->
   // CREATING_SOCKET
   //   CreateSocket() -> OnSocketCreated() ->
   // STARTING_INSTANCE
   //   -> OnInstanceStarted() ->
   // CONNECTING_MOJO
   //   ConnectMojo() -> OnMojoConnected() ->
   // RUNNING
   //
   // At any state, Stop() can be called. It does not immediately stop the
   // instance, but will eventually stop it.
   // The actual stop will be notified via Observer::OnStopped().
   //
   // When Stop() is called, it makes various behavior based on the current
   // phase.
   //
   // NOT_STARTED:
   //   Do nothing. Immediately transition to the STOPPED state.
-  // CHECKING_DISK_SPACE, CREATING_SOCKET:
-  //   The main task of those phases runs on WorkerPool thread. So, Stop()
-  //   just sets the flag and return. On the main task completion, a callback
+  // CREATING_SOCKET:
+  //   The main task of the phase runs on WorkerPool thread. So, Stop() just
+  //   sets the flag and return. On the main task completion, a callback
   //   will run on the main (practically UI) thread, and the flag is checked
   //   at the beginning of them. This should work under the assumption that
   //   the main tasks do not block indefinitely.
   // STARTING_INSTANCE:
   //   The ARC instance is starting via SessionManager. So, similar to
-  //   CHECKING_DISK_SPACE/CREATING_SOCKET cases, Stop() just sets the flag
-  //   and return. In its callback, it checks if ARC instance is successfully
-  //   started or not. In case of success, a request to stop the ARC instance
-  //   is sent to SessionManager. Its completion will be notified via
-  //   ArcInstanceStopped. Otherwise, it just turns into STOPPED state.
+  //   CREATING_SOCKET case, Stop() just sets the flag and return. In its
+  //   callback, it checks if ARC instance is successfully started or not.
+  //   In case of success, a request to stop the ARC instance is sent to
+  //   SessionManager. Its completion will be notified via ArcInstanceStopped.
+  //   Otherwise, it just turns into STOPPED state.
   // CONNECTING_MOJO:
   //   The main task runs on WorkerPool thread, but it is blocking call.
   //   So, Stop() sends a request to cancel the blocking by closing the pipe
   //   whose read side is also polled. Then, in its callback, similar to
   //   STARTING_INSTANCE, a request to stop the ARC instance is sent to
   //   SessionManager, and ArcInstanceStopped handles remaining procedure.
   // RUNNING:
   //   There is no more callback which runs on normal flow, so Stop() requests
   //   to stop the ARC instance via SessionManager.
   //
@@ skipping 19 matching lines @@
   // Specifically, in STOPPED state, there may be inflight operations or
   // pending callbacks. Though, what they do is just do-nothing conceptually
   // and they can be safely ignored.
   //
   // Note: Order of constants below matters. Please make sure to sort them
   // in chronological order.
   enum class State {
     // ARC is not yet started.
     NOT_STARTED,
 
-    // Checking the disk space.
-    CHECKING_DISK_SPACE,
-
     // An UNIX socket is being created.
     CREATING_SOCKET,
 
     // The request to start the instance has been sent.
     STARTING_INSTANCE,
 
     // The instance has started. Waiting for it to connect to the IPC bridge.
     CONNECTING_MOJO,
 
     // The instance is fully set up.
     RUNNING,
 
     // ARC is terminated.
     STOPPED,
   };
 
   ArcBridgeBootstrapImpl();
   ~ArcBridgeBootstrapImpl() override;
 
   // ArcBridgeBootstrap:
   void Start() override;
   void Stop() override;
 
  private:
-  // Called after getting the device free disk space.
-  void OnFreeDiskSpaceObtained(int64_t disk_free_bytes);
-
   // Creates the UNIX socket on the bootstrap thread and then processes its
   // file descriptor.
   static base::ScopedFD CreateSocket();
   void OnSocketCreated(base::ScopedFD fd);
 
   // DBus callback for StartArcInstance().
-  void OnInstanceStarted(base::ScopedFD socket_fd, bool success);
+  void OnInstanceStarted(base::ScopedFD socket_fd,
+                         bool success,
+                         const std::string& error);
 
   // Synchronously accepts a connection on |socket_fd| and then processes the
   // connected socket's file descriptor.
   static base::ScopedFD ConnectMojo(base::ScopedFD socket_fd,
                                     base::ScopedFD cancel_fd);
   void OnMojoConnected(base::ScopedFD fd);
 
   // Request to stop ARC instance via DBus.
   void StopArcInstance();
 
@@ skipping 40 matching lines @@
   chromeos::SessionManagerClient* client = GetSessionManagerClient();
   if (client == nullptr)
     return;
   client->RemoveObserver(this);
 }
 
 void ArcBridgeBootstrapImpl::Start() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK_EQ(state_, State::NOT_STARTED);
   VLOG(2) << "Starting ARC session.";
-  VLOG(2) << "Checking disk space...";
-  state_ = State::CHECKING_DISK_SPACE;
+  VLOG(2) << "Creating socket...";
 
-  // TODO(crbug.com/628124): Move disk space checking logic to session_manager.
-  base::PostTaskAndReplyWithResult(
-      base::WorkerPool::GetTaskRunner(true).get(), FROM_HERE,
-      base::Bind(&base::SysInfo::AmountOfFreeDiskSpace,
-                 base::FilePath(kDiskCheckPath)),
-      base::Bind(&ArcBridgeBootstrapImpl::OnFreeDiskSpaceObtained,
-                 weak_factory_.GetWeakPtr()));
-}
-
-void ArcBridgeBootstrapImpl::OnFreeDiskSpaceObtained(int64_t disk_free_bytes) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK_EQ(state_, State::CHECKING_DISK_SPACE);
-
-  if (stop_requested_) {
-    VLOG(1) << "Stop() called while checking disk space";
-    OnStopped(ArcBridgeService::StopReason::SHUTDOWN);
-    return;
-  }
-
-  if (disk_free_bytes < 0) {
-    LOG(ERROR) << "ARC: Failed to get free disk space";
-    OnStopped(ArcBridgeService::StopReason::GENERIC_BOOT_FAILURE);
-    return;
-  }
-  if (disk_free_bytes < kCriticalDiskFreeBytes) {
-    LOG(ERROR) << "ARC: The device is too low on disk space to start ARC";
-    OnStopped(ArcBridgeService::StopReason::LOW_DISK_SPACE);
-    return;
-  }
-
-  VLOG(2) << "Disk space check is done. Creating socket...";
   state_ = State::CREATING_SOCKET;
   base::PostTaskAndReplyWithResult(
       base::WorkerPool::GetTaskRunner(true).get(), FROM_HERE,
       base::Bind(&ArcBridgeBootstrapImpl::CreateSocket),
       base::Bind(&ArcBridgeBootstrapImpl::OnSocketCreated,
                  weak_factory_.GetWeakPtr()));
 }
 
 // static
 base::ScopedFD ArcBridgeBootstrapImpl::CreateSocket() {
@@ skipping 62 matching lines @@
   chromeos::SessionManagerClient* session_manager_client =
       chromeos::DBusThreadManager::Get()->GetSessionManagerClient();
   session_manager_client->StartArcInstance(
       cryptohome_id,
       disable_boot_completed_broadcast,
       base::Bind(&ArcBridgeBootstrapImpl::OnInstanceStarted,
                  weak_factory_.GetWeakPtr(), base::Passed(&socket_fd)));
 }
 
 void ArcBridgeBootstrapImpl::OnInstanceStarted(base::ScopedFD socket_fd,
-                                               bool success) {
+                                               bool success,
+                                               const std::string& error) {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (state_ == State::STOPPED) {
     // This is the case that error is notified via DBus before the
     // OnInstanceStarted() callback is invoked. The stopping procedure has
     // been run, so do nothing.
     return;
   }
 
   DCHECK_EQ(state_, State::STARTING_INSTANCE);
 
   if (stop_requested_) {
     if (success) {
       // The ARC instance has started to run. Request to stop.
       StopArcInstance();
       return;
     }
     OnStopped(ArcBridgeService::StopReason::SHUTDOWN);
     return;
   }
 
   if (!success) {
     LOG(ERROR) << "Failed to start ARC instance";
-    OnStopped(ArcBridgeService::StopReason::GENERIC_BOOT_FAILURE);
+    OnStopped(error == kArcLowDiskError
+                  ? ArcBridgeService::StopReason::LOW_DISK_SPACE
+                  : ArcBridgeService::StopReason::GENERIC_BOOT_FAILURE);
     return;
   }
 
   VLOG(2) << "ARC instance is successfully started. Connecting Mojo...";
   state_ = State::CONNECTING_MOJO;
 
   // Prepare a pipe so that AcceptInstanceConnection can be interrupted on
   // Stop().
   base::ScopedFD cancel_fd;
   if (!CreatePipe(&cancel_fd, &accept_cancel_pipe_)) {
@@ skipping 97 matching lines @@
   if (stop_requested_)
     return;
 
   stop_requested_ = true;
   arc_bridge_host_.reset();
   switch (state_) {
     case State::NOT_STARTED:
       OnStopped(ArcBridgeService::StopReason::SHUTDOWN);
       return;
 
-    case State::CHECKING_DISK_SPACE:
     case State::CREATING_SOCKET:
     case State::STARTING_INSTANCE:
       // Before starting the ARC instance, we do nothing here.
       // At some point, a callback will be invoked on UI thread,
       // and stopping procedure will be run there.
       // On Chrome shutdown, it is not the case because the message loop is
       // already stopped here. Practically, it is not a problem because;
-      // - On disk space checking or on socket creating, it is ok to simply
-      // ignore such cases, because we no-longer continue the bootstrap
-      // procedure.
+      // - On socket creating, it is ok to simply ignore such cases,
+      // because we no-longer continue the bootstrap procedure.
       // - On starting instance, the container instance can be leaked.
       // Practically it is not problematic because the session manager will
       // clean it up.
       return;
 
     case State::CONNECTING_MOJO:
       // Mojo connection is being waited on a WorkerPool thread.
       // Request to cancel it. Following stopping procedure will run
       // in its callback.
       DCHECK(accept_cancel_pipe_.get());
@@ skipping 73 matching lines @@
 void ArcBridgeBootstrap::RemoveObserver(Observer* observer) {
   observer_list_.RemoveObserver(observer);
 }
 
 // static
 std::unique_ptr<ArcBridgeBootstrap> ArcBridgeBootstrap::Create() {
   return base::MakeUnique<ArcBridgeBootstrapImpl>();
 }
 
 }  // namespace arc