dbus: add support for passing file descriptors

dbus/message.cc

Index: dbus/message.cc
diff --git a/dbus/message.cc b/dbus/message.cc
index 43fb3bb8b593718b6fc01961c1f97a705f66fde8..e9458ac6781dfc6d32e4370b4c4a8c0cb79c4aa6 100644
--- a/dbus/message.cc
+++ b/dbus/message.cc
@@ -9,6 +9,9 @@
 #include "base/basictypes.h"
 #include "base/format_macros.h"
 #include "base/logging.h"
+#if defined(DBUS_TYPE_UNIX_FD)
+#include "base/platform_file.h"
+#endif
 #include "base/stringprintf.h"
 #include "dbus/object_path.h"
 #include "third_party/protobuf/src/google/protobuf/message_lite.h"
@@ -199,6 +202,16 @@ std::string Message::ToStringInternal(const std::string& indent,
         output += ToStringInternal(indent + "  ", &sub_reader);
         break;
       }
+#if defined(DBUS_TYPE_UNIX_FD)

[satorux1, 2012/03/28 00:32:43]

Hmm, there seems to be many #ifdefs based on DBUS_TYPE_UNIX_FD added in this
patch. 

I'd rather like to minimize #ifdefs. What about doing this in message.h:

// DBUS_TYPE_UNIX_FD was added in D-Bus version x.x.x
#if defined(DBUS_TYPE_UNIX_FD)
const bool kDBusTypeUnixFdIsSupported = true;
#else
const bool kDBusTypeUnixFdIsSupported = false;
#define DBUS_TYPE_UNIX_FD      ((int) 'h')
#endif

Then, we can use kDBusTypeUnixFdIsSupported like

  CHECK(kDBusTypeUnixFdIsSupported);

[Sam Leffler, 2012/03/28 17:28:09]

You cannot do this unless you define UNIX_FD in the unsupported case.  There are
4 #ifdefs in this file.  1 is required because UNIX_FD is undefined otherwise. 2
can be handled as you request but result in a bunch of code being compiled in
that will never be used/work.  1 is around the #include of platform_file.h that
I intentionally bracketed to show it was only needed for UNIX_FD support (maybe
a style violation?).

[satorux1, 2012/03/28 17:41:54]

My proposal was to define DBUS_TYPE_UNIX_FD for the unsupported case. Please see
my original comment.



  There are
I'm fine with that. The unused code is fairly small, but #ifdefs make code
harder to maintain. IMHO, the latter is worse in this case.

[Sam Leffler, 2012/03/28 19:25:27]

Done.

+      case UNIX_FD: {
+        FileDescriptor value;
+        if (!reader->PopFileDescriptor(&value))
+          return kBrokenMessage;
+        output += indent + "fd#" +
+                  base::StringPrintf("%u", value.value()) + "\n";
+        break;
+      }
+#endif
       default:
         LOG(FATAL) << "Unknown type: " << type;
     }
@@ -677,6 +690,19 @@ void MessageWriter::AppendVariantOfBasic(int dbus_type, const void* value) {
   CloseContainer(&variant_writer);
 }
 
+void MessageWriter::AppendFileDescriptor(const FileDescriptor& value) {

[satorux1, 2012/03/28 00:32:43]

If we are to go with the idea described above, you can just put

  CHECK(kDBusTypeUnixFdIsSupported);

and get rid of the #ifdef.

+#if defined(DBUS_TYPE_UNIX_FD)
+  base::PlatformFileInfo info;
+  int fd = value.value();
+  bool ok = base::GetPlatformFileInfo(fd, &info);
+  if (!ok || info.is_directory)
+    LOG(FATAL) << "Attempt to pass invalid file descriptor";
+  AppendBasic(DBUS_TYPE_UNIX_FD, &fd);
+#else
+  CHECK(false) << "File descriptor passing not supported";
+#endif
+}
+
 //
 // MessageReader implementation.
 //
@@ -936,4 +962,25 @@ bool MessageReader::PopVariantOfBasic(int dbus_type, void* value) {
   return variant_reader.PopBasic(dbus_type, value);
 }
 
+bool MessageReader::PopFileDescriptor(FileDescriptor* value) {
+#if defined(DBUS_TYPE_UNIX_FD)
+  int fd;
+  const bool success = PopBasic(DBUS_TYPE_UNIX_FD, &fd);
+  if (!success)
+    return false;
+
+  base::PlatformFileInfo info;
+  bool ok = base::GetPlatformFileInfo(fd, &info);
+  if (!ok || info.is_directory) {
+    LOG(FATAL) << "Attempt to receive invalid file descriptor";
+    return false;  // NB: not reached
+  }

[keybuk, 2012/03/27 22:36:35]

doesn't close the file descriptor before returning

also what if base::PlatformFileInfo constructor, base::GetPlatformFileInfo or
LOG() throws an exception?

[Sam Leffler, 2012/03/27 23:20:40]

Not sure where you're referring.  fd is not closed because it's returned via
value->PutValue.  The error case will abort the process because LOG(FATAL) does
that.

[keybuk, 2012/03/28 17:53:12]

it does that now, it might not in future; likewise exception handling...
basically you can leak an fd between :968 and :978 if things change under you,
your code should defend against that

[Sam Leffler, 2012/03/28 19:25:27]

Done.

+  value->PutValue(fd);
+  return true;
+#else
+  CHECK(false) << "File descriptor passing not supported";
+  return false;
+#endif
+}
+
 }  // namespace dbus