dbus: add support for passing file descriptors

dbus/file_descriptor.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 DBUS_FILE_DESCRIPTOR_H_
+#define DBUS_FILE_DESCRIPTOR_H_
+#pragma once
+
+namespace dbus {
+
+// FileDescriptor is a type used to encapsulate D-Bus file descriptors
+// and to follow the RAII idiom appropiate for use with message operations
+// where the descriptor might be easily leaked.  Ownership is asserted
+// only when PutValue is used.
+class FileDescriptor {
+ public:
+  // Permit initialization without a value for passing to

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

nit: Permit -> Permits. Please also fix other places.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Comments

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

Not sure which clause you're pointed me at but "Permit" is correct AFAIK.

[satorux1, 2012/03/28 17:30:16]

These comments should be descriptive ("Opens the file") rather than imperative
("Open the file"); the comment describes the function

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

Done.

+  // dbus::MessageReader::PopFileDescriptor to fill in and from int values.
+  FileDescriptor() : value_(-1), owner_(true) {}

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

owner_(true) here means you'll close(-1) later; better to just set
owner_(false), since it's explicitly set to true by PutValue() anyway

+  explicit FileDescriptor(int value) : value_(value), owner_(false) {}

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

Could you remove the default constructor? 

The style guide discourages function overloading, including constructors

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl...

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

This cribs from dbus::ObjectPath. Removing it makes usage more awkward because
every instance must be done with an explicit -1 argument.  I understand the
intent of the style guideline but do we really want it here?

+
+  virtual ~FileDescriptor();
+
+  // Retrieve value as an int.

[keybuk, 2012/03/28 17:57:37]

nit: Retrieves.
Also explain that this does not take ownership.

+  int value() const { return value_; }
+
+  // Set the value and assign ownership.

[keybuk, 2012/03/28 17:57:37]

nit: Sets

+  void PutValue(int value) {

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

Put -> Set? Set is more common for this.

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

Please get together w/ keybuk and decide which way you want me to shave this
yak?

[keybuk, 2012/03/28 17:57:37]

I asked Sam to change this from Set to Put because the object is taking
ownership of the fd you pass in, rather than just setting the value contained
within it.

ie. Put/Take rather than Set/Get

+    value_ = value;
+    owner_ = true;
+  }
+
+  // Take the value and ownership.
+  int TakeValue(void) {

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

nit: (void) -> ()

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

Done.

+    owner_ = false;
+    return value_;

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

I guess owner_ is unnecessary. We can use -1 as non-owner.

  int copy = value_;
  value_ = 1;
  retrn copy;

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

They are not equivalent.  It is useful to preserve the value while releasing
ownership.

[keybuk, 2012/03/28 17:57:37]

It's probably worth explaining in the class comment what ownership implies, and
give the example of the non-ownership case (passing a dbus::FileDescriptor you
don't want closed) and the ownership case (receiving a dbus::FileDescriptor you
do)

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

Done.

+  }

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

nit: we usually have a blank line here.

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

Done.

+ private:
+  int value_;
+  bool owner_;
+
+  DISALLOW_COPY_AND_ASSIGN(FileDescriptor);
+};
+
+}  // namespace dbus
+
+#endif  // DBUS_FILE_DESCRIPTOR_H_