No dbus::FileDescriptor in DebugDaemonClient

chromeos/dbus/debug_daemon_client.h

 // Copyright (c) 2012 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.
 
 #ifndef CHROMEOS_DBUS_DEBUG_DAEMON_CLIENT_H_
 #define CHROMEOS_DBUS_DEBUG_DAEMON_CLIENT_H_
 
 #include <stdint.h>
 
 #include <map>
 
 #include "base/callback.h"
 #include "base/files/file.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/task_runner.h"
 #include "base/trace_event/tracing_agent.h"
 #include "chromeos/chromeos_export.h"
 #include "chromeos/dbus/dbus_client.h"
-#include "dbus/file_descriptor.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
 namespace chromeos {
 
 // DebugDaemonClient is used to communicate with the debug daemon.
 class CHROMEOS_EXPORT DebugDaemonClient
     : public DBusClient,
       public base::trace_event::TracingAgent {
  public:
   ~DebugDaemonClient() override;
 
   // Called once GetDebugLogs() is complete. Takes one parameter:
   // - succeeded: was the logs stored successfully.
   typedef base::Callback<void(bool succeeded)> GetDebugLogsCallback;
 
-  // Requests to store debug logs into |file| and calls |callback|
+  // Requests to store debug logs into |file_descriptor| and calls |callback|
   // when completed. Debug logs will be stored in the .tgz if
   // |is_compressed| is true, otherwise in logs will be stored in .tar format.
   virtual void DumpDebugLogs(bool is_compressed,
-                             base::File file,
-                             scoped_refptr<base::TaskRunner> task_runner,
+                             int file_descriptor,

[satorux1, 2016/08/23 04:43:37]

How about taking ownership of the file descriptor as base::File? I thought
that'd make ownership clearer.

[hashimoto, 2016/08/23 06:38:50]

Here we want to avoid owning the given FD because there is no need to do it [1]
and it can result in UI jank [2].

1. There is no need to own the FD because appending a FD to a D-Bus message
doesn't require ownership as dbus_message_iter_append_basic() duplicates the FD.

2. The user code (debug_log_writer.cc) passes a FD of a regular file which can
cause UI jank when being closed on the UI thread, instead of a pipe FD, to this
method. From DebugDaemonClient's point of view, there is no way to know if it's
OK to close the given FD on the UI thread.

[satorux1, 2016/08/23 07:02:02]

Makes sense. In that case, could you add a comment that says this function does
not close |file_descriptor|?

[hashimoto, 2016/08/23 07:41:27]

Added a comment about FD duplication.

                              const GetDebugLogsCallback& callback) = 0;
 
   // Called once SetDebugMode() is complete. Takes one parameter:
   // - succeeded: debug mode was changed successfully.
   typedef base::Callback<void(bool succeeded)> SetDebugModeCallback;
 
   // Requests to change debug mode to given |subsystem| and calls
   // |callback| when completed. |subsystem| should be one of the
   // following: "wifi", "ethernet", "cellular" or "none".
   virtual void SetDebugMode(const std::string& subsystem,
@@ skipping 43 matching lines @@
                           const std::string& error_message)>;
 
   // Runs perf (via quipper) with arguments for |duration| (converted to
   // seconds) and returns data collected over the passed |file_descriptor|.
   // |error_callback| is called if there is an error with the DBus call.
   // Note that quipper failures may occur after successfully running the DBus
   // method. Such errors can be detected by |file_descriptor| being closed with
   // no data written.
   virtual void GetPerfOutput(base::TimeDelta duration,
                              const std::vector<std::string>& perf_args,
-                             dbus::ScopedFileDescriptor file_descriptor,
+                             int file_descriptor,

[satorux1, 2016/08/23 04:43:37]

ditto. maybe take ownership?

[hashimoto, 2016/08/23 06:38:50]

Acknowledged.

[satorux1, 2016/08/23 07:02:02]

ditto. a comment about file_descriptor ownership would be helpful here too.

[hashimoto, 2016/08/23 07:41:27]

Done.

                              const DBusMethodErrorCallback& error_callback) = 0;
 
   // Callback type for GetScrubbedLogs(), GetAllLogs() or GetUserLogFiles().
   using GetLogsCallback =
       base::Callback<void(bool succeeded,
                           const std::map<std::string, std::string>& logs)>;
 
   // Gets scrubbed logs from debugd.
   virtual void GetScrubbedLogs(const GetLogsCallback& callback) = 0;
 
@@ skipping 84 matching lines @@
   // Create() should be used instead.
   DebugDaemonClient();
 
  private:
   DISALLOW_COPY_AND_ASSIGN(DebugDaemonClient);
 };
 
 }  // namespace chromeos
 
 #endif  // CHROMEOS_DBUS_DEBUG_DAEMON_CLIENT_H_