Implement classes used for manipulating D-Bus messages.

dbus/message.h

Index: dbus/message.h
diff --git a/dbus/message.h b/dbus/message.h
new file mode 100644
index 0000000000000000000000000000000000000000..22f3937a6016bdff9149747f50d314fb3a34d975
--- /dev/null
+++ b/dbus/message.h
@@ -0,0 +1,341 @@
+// Copyright (c) 2011 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_MESSAGE_H_
+#define DBUS_MESSAGE_H_
+#pragma once
+
+#include <string>
+#include <vector>
+#include <dbus/dbus.h>
+
+#include "base/basictypes.h"
+
+namespace dbus {
+
+class MessageWriter;
+class MessageReader;
+
+// Message is the base class of D-Bus message types. Client code should
+// usually use sub classes such as MethodCall and Response instead.
+//
+// The class name Message is very generic, but there should be no problem
+// as the class is inside 'dbus' namespace. We chose to name this way, as
+// libdbus defines lots of types starting with DBus, such as
+// DBusMessage. We should avoid confusion and conflict with these types.
+class Message {
+ public:
+  // The message type used in D-Bus.
+  // Redefined here so client code doesn't need to use raw D-Bus macros.
+  enum MessageType {
+    MESSAGE_INVALID = DBUS_MESSAGE_TYPE_INVALID,
+    MESSAGE_METHOD_CALL = DBUS_MESSAGE_TYPE_METHOD_CALL,
+    MESSAGE_METHOD_RETURN = DBUS_MESSAGE_TYPE_METHOD_RETURN,
+    MESSAGE_SIGNAL = DBUS_MESSAGE_TYPE_SIGNAL,
+    MESSAGE_ERROR = DBUS_MESSAGE_TYPE_ERROR,
+ };

[stevenjb, 2011/07/27 17:56:26]

I'm not sure that there is anything gained by mapping this enum, since we have
to include dbus/dbus.h in the header anyway. What happens if a new
DBUS_MESSAGE_TYPE is introduced and we cast it to our own enum?

[satorux1, 2011/07/27 21:20:35]

DBUS_MESSAGE_TYPE_INVALID etc. are #define macros. Having an enum type here
makes code a bit more type-safe: 

  MessageType GetMessageType() vs. int GetMessageType().

These message types are defined in the D-Bus protocol so I don't expect a new
message type will be introduced. Even if it's introduced, it won't affect our
code, as long as we are using our MessageType.

I added a comment about it.

[stevenjb, 2011/07/27 21:44:13]

Ah, if these are #defines that makes sense then. Fair enough.

+
+  // The data type used in the D-Bus type system.
+  // Redefined here so client code doesn't need to use raw D-Bus macros.
+  enum DataType {
+    INVALID_DATA = DBUS_TYPE_INVALID,
+    BYTE = DBUS_TYPE_BYTE,
+    BOOL = DBUS_TYPE_BOOLEAN,
+    INT16 = DBUS_TYPE_INT16,
+    UINT16 = DBUS_TYPE_UINT16,
+    INT32 = DBUS_TYPE_INT32,
+    UINT32 = DBUS_TYPE_UINT32,
+    INT64 = DBUS_TYPE_INT64,
+    UINT64 = DBUS_TYPE_UINT64,
+    DOUBLE = DBUS_TYPE_DOUBLE,
+    STRING = DBUS_TYPE_STRING,
+    OBJECT_PATH = DBUS_TYPE_OBJECT_PATH,
+    ARRAY = DBUS_TYPE_ARRAY,
+    STRUCT = DBUS_TYPE_STRUCT,
+    DICT_ENTRY = DBUS_TYPE_DICT_ENTRY,
+    VARIANT = DBUS_TYPE_VARIANT,
+  };

[stevenjb, 2011/07/27 17:56:26]

Same comment as above.

[satorux1, 2011/07/27 21:20:35]

Ditto.

+
+  // Creates a Message. The internal raw message is NULL until it's set
+  // from outside by reset_raw_message().
+  Message();
+  virtual ~Message();
+
+  // Returns the type of the message. Returns MESSAGE_INVALID if
+  // raw_message_ is NULL.
+  MessageType GetMessageType();
+
+  DBusMessage* raw_message() { return raw_message_; }
+
+  // Resets raw_message_ with the given raw message. Takes the ownership
+  // of raw_message. raw_message_ will be unref'ed in the destructor.
+  void reset_raw_message(DBusMessage* raw_message);

[stevenjb, 2011/07/27 17:56:26]

Do we need to expose this publicly, or could we make this protected and require
that sub-classes set raw_message_?

[satorux1, 2011/07/27 21:20:35]

Good point. This is exposed for unit tests and ObjectProxy. ObjectProxy needs to
set the response message returned from the server to Response object. We could
make ObjectProxy a friend class, but I guess it's simpler to expose it.

[stevenjb, 2011/07/27 21:44:13]

I'll have to look at ObjectProxy. Making a test class a friend is standard
practice, but we shouldn't do it for ObjectProxy. Unless it makes sense for
ObejctProxy to use a derived dbus::Message class, leaving this public is fine,
just a little more low level than would be ideal (since it exposes DBusMessage).

+
+  // Returns the string representation of this message. Useful for
+  // debugging.  The returned string is similar to dbus-send's output.
+  std::string ToString();
+
+ private:
+  // Helper function used in ToString().
+  std::string ToStringInternal(const std::string& indent,
+                               MessageReader* reader);
+
+  DBusMessage* raw_message_;
+  DISALLOW_COPY_AND_ASSIGN(Message);
+};
+
+// MessageCall is a type of message used for calling a method via D-Bus.
+class MethodCall : public Message {
+ public:
+  // Creates a method call message for the specified interface name and
+  // the method name.
+  //
+  // For instance, to call "Get" method of DBUS_INTERFACE_INTROSPECTABLE
+  // interface ("org.freedesktop.DBus.Introspectable"), create a method
+  // call like this:
+  //
+  //   MethodCall method_call(DBUS_INTERFACE_INTROSPECTABLE, "Get");
+  //
+  // The constructor creates the internal raw_message_, so the client
+  // doesn't need to set this by reset_raw_message().
+  MethodCall(const std::string& interface_name,
+             const std::string& method_name);
+
+  const std::string& interface_name() { return interface_name_; }
+  const std::string& method_name() { return method_name_; }
+
+ private:
+  friend class ObjectProxy;
+
+  // Sets the service name. This will be handled by the object proxy.
+  void SetServiceName(const std::string& service_name);
+  // Sets the object path. This will be handled by the object proxy.
+  void SetObjectPath(const std::string& object_path);
+
+  std::string interface_name_;
+  std::string method_name_;
+
+  DISALLOW_COPY_AND_ASSIGN(MethodCall);
+};
+
+// Response is a type of message used for receiving a response from a
+// method via D-Bus.
+class Response : public Message {
+ public:
+  // Creates a Response message. The internal raw message is
+  // NULL. ObjectProxy will set it properly once it gets a response from
+  // the server.
+  Response();
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(Response);
+};
+
+// MessageWriter is used to write outgoing messages for calling methods
+// and sending signals.
+class MessageWriter {
+ public:
+  // The data will be written into the given message.
+  MessageWriter(Message* message);
+  ~MessageWriter();
+
+  // Appends a byte to the message.
+  void AppendByte(uint8 value);
+  // The following functions do the same for other basic types.

[stevenjb, 2011/07/27 17:56:26]

This comment is probably not necessary and made me think that Byte was different
from Bool, etc.

[satorux1, 2011/07/27 21:20:35]

Done.

+  void AppendBool(bool value);
+  void AppendInt16(int16 value);
+  void AppendUint16(uint16 value);
+  void AppendInt32(int32 value);
+  void AppendUint32(uint32 value);
+  void AppendInt64(int64 value);
+  void AppendUint64(uint64 value);
+  void AppendDouble(double value);
+  void AppendString(const std::string& value);
+  void AppendObjectPath(const std::string& value);
+
+  // Opens an array. The array contents can be added to the array with
+  // sub_writer. The client code should close the array by CloseContainer(),

[stevenjb, 2011/07/27 17:56:26]

|sub_writer|
s/by/with/

+  // once all contents are added.

[stevenjb, 2011/07/27 17:56:26]

Can multiple arrays use the same sub_writer? If so, does it make sense for
MessageWriter to allocate/free the sub_writer? If not, maybe OpenArray() should
allocate and return sub_writer, freeing it with CloseContainer()?

[satorux1, 2011/07/27 21:20:35]

I think we can do it, but this style is consistent with PopArray(MessageReader*
sub_reader) in MessageReader, so I'd rather keep this way.

+  //
+  // |signature| parameter is used to supply the D-Bus type signature of
+  // the array contents. For instance, if you want an array of strings,
+  // then you pass "s" as the signature.  need to take

[stevenjb, 2011/07/27 17:56:26]

"need to take"?

[satorux1, 2011/07/27 21:20:35]

Done.

+  //
+  // See the spec for details about the type signatures.
+  // http://dbus.freedesktop.org/doc/dbus-specification.html
+  // #message-protocol-signatures
+  //
+  // Ideally, client shouldn't need to supply the signature string, but
+  // the underlying D-Bus library requires us to supply this before
+  // appending contents to array and variant. It's technically possible
+  // for us to design API that doesn't require the signature but it'll

[stevenjb, 2011/07/27 17:56:26]

s/it'll/it will/ (it'll is rarely written as a contraction)

[satorux1, 2011/07/27 21:20:35]

Done.

+  // complicate the implementation so we decided to have the signature
+  // parameter. Hopefully, variants are less used in request messages from
+  // client side than response message from server side, so this should
+  // not be a big issue.
+  //
+  void OpenArray(const std::string& signature, MessageWriter* sub_writer);
+  // Do the same for a variant.
+  void OpenVariant(const std::string& signature, MessageWriter* sub_writer);
+  // Do the same for Struct and dict entry. They don't need the signature.
+  void OpenStruct(MessageWriter* sub_writer);
+  void OpenDictEntry(MessageWriter* sub_writer);
+
+  // Close the container for a array/variant/struct/dict entry.
+  void CloseContainer(MessageWriter* sub_writer);
+
+  // Appends the array of bytes. Arrays of bytes are often used for
+  // exchanging binary blobs hence it's worth having a specialized
+  // function.
+  void AppendArrayOfBytes(const std::vector<uint8>& bytes);

[stevenjb, 2011/07/27 17:56:26]

This might be more flexible if it takes a const uint8* and a length, instead of
a vector. Sometimes binary blobs may not be wrapped up in a vector (and
constructing one might introduce an extra copy).

[satorux1, 2011/07/27 21:20:35]

Good idea. Done.

+
+  // Appends the byte wrapped in a variant data container. Variants are
+  // widely used in D-Bus services so it's worth having a specialized
+  // function. For instance, The third parameter of
+  // "org.freedesktop.DBus.Properties.Set" is a variant.
+  void AppendVariantOfByte(uint8 value);
+  // The following functions do the same for other basic types.

[stevenjb, 2011/07/27 17:56:26]

This comment is probably not necessary and made me think that Byte was different
from Bool, etc.

[satorux1, 2011/07/27 21:20:35]

Done.

+  void AppendVariantOfBool(bool value);
+  void AppendVariantOfInt16(int16 value);
+  void AppendVariantOfUint16(uint16 value);
+  void AppendVariantOfInt32(int32 value);
+  void AppendVariantOfUint32(uint32 value);
+  void AppendVariantOfInt64(int64 value);
+  void AppendVariantOfUint64(uint64 value);
+  void AppendVariantOfDouble(double value);
+  void AppendVariantOfString(const std::string& value);
+  void AppendVariantOfObjectPath(const std::string& value);
+
+ private:
+  // Helper function used to implement AppendByte etc.
+  void AppendBasic(int dbus_type, const void* value);
+
+  // Helper function used to implement AppendVariantOfByte() etc.
+  void AppendVariantOfBasic(int dbus_type, const void* value);
+
+  Message* message_;
+  DBusMessageIter raw_message_iter_;
+  bool container_is_open_;
+
+  DISALLOW_COPY_AND_ASSIGN(MessageWriter);
+};
+
+// MessageReader is used to read incoming messages such as responses for
+// method calls.
+//
+// MessageReader manages an internal iterator to read data. All functions
+// starting with Pop advance the iterator on success.
+//
+// The main design goal of MessageReader and MessageWriter classes is to
+// provide type safe API. In the past, there was a Chrome OS blocker bug,

[stevenjb, 2011/07/27 17:56:26]

s/type safe API/a type safe API

[satorux1, 2011/07/27 21:20:35]

Done.

+// that took days to fix, that would have been prevented if the API was
+// type-safe.
+//
+// For instance, instead of doing something like:
+//
+//   // We shouldn't add '&' to str here, but it compiles with '&' added.
+//   dbus_g_proxy_call(..., G_TYPE_STRING, str, G_TYPE_INVALID, ...)
+//
+// We want to do something like:
+//
+//   writer.AppendString(str);
+//
+// See crosbug.com/7619 for details about the bug in question.

[stevenjb, 2011/07/27 17:56:26]

Comment about blocker bug is probably not necessary; it should be obvious why
type safety is a good thing :)

[satorux1, 2011/07/27 21:20:35]

Done.

+class MessageReader {
+ public:
+  // The data will be read from the given message.
+  MessageReader(Message* message);
+  ~MessageReader();
+
+  // Returns true if the reader has more data to read. The function is
+  // used to iterate contents in a container like:
+  //
+  //   while (reader.HasMore())
+  //     reader.PopString(&value);
+  bool HasMore();

[stevenjb, 2011/07/27 17:56:26]

nit: HasMoreData()

[satorux1, 2011/07/27 21:20:35]

Done.

+
+  // Gets the byte at the current iterator position. On success, advances
+  // the iterator and returns true. It's an error if the actual data type
+  // is not a byte, so false will be returned in this case.
+  bool PopByte(uint8* value);
+  // The following functions do the same for other basic types.

[stevenjb, 2011/07/27 17:56:26]

nit: Don't separate PopBool,etc from PopByte with a comment.

[satorux1, 2011/07/27 21:20:35]

Done.

+  bool PopBool(bool* value);
+  bool PopInt16(int16* value);
+  bool PopUint16(uint16* value);
+  bool PopInt32(int32* value);
+  bool PopUint32(uint32* value);
+  bool PopInt64(int64* value);
+  bool PopUint64(uint64* value);
+  bool PopDouble(double* value);
+  bool PopString(std::string* value);
+  bool PopObjectPath(std::string* value);
+
+  // Sets up the given message reader to read an array at the current
+  // iterator position. On success, advances the iterator and returns
+  // true. It's an error if the actual data type is not an array, so false
+  // will be returned in this case.
+  bool PopArray(MessageReader* sub_reader);
+  // The following functions do the same for other container types.

[stevenjb, 2011/07/27 17:56:26]

nit: Don't separate with a comment.

[satorux1, 2011/07/27 21:20:35]

Done.

+  bool PopStruct(MessageReader* sub_reader);
+  bool PopDictEntry(MessageReader* sub_reader);
+  bool PopVariant(MessageReader* sub_reader);
+
+  // Gets the array of bytes at the current iterator position. On success,
+  // advances the iterator and returns true. Arrays of bytes are often
+  // used for exchanging binary blobs hence it's worth having a
+  // specialized function.

[stevenjb, 2011/07/27 17:56:26]

I think there should be a version of this that takes a uint8** and a length int*
as input and pass ownership of the bytes to the calling function so that it
doesn't force an extra copy. (See comment in C++ file)

[satorux1, 2011/07/27 21:20:35]

Good point. What about adding such a version when it becomes necessary?
vector<uint8> value can be used for C-ish functions like fwrite(v.data(), 1,
v.size(), fp), so I guess that the uint8** version would be rarely needed.

[stevenjb, 2011/07/27 21:44:13]

It's not a question of whether or not vector<> can be used, but whether or not
it should be used. It tends to introduce a lot of extra copying. For not this is
probably fine, just something to consider from a performance perspective.

[satorux1, 2011/07/27 22:12:14]

I'm convinced. Changed it to uint8* + size_t, to make it consistent with
AppendAarrayOfBytes(). 

+  bool PopArrayOfBytes(std::vector<uint8>* bytes);
+
+  // Gets the array of object paths at the current iterator position. On
+  // success, advances the iterator and returns true. Arrays of object
+  // paths are often used to communicate with D-Bus services like
+  // NetworkManager, hence it's worth having a specialized function.
+  bool PopArrayOfObjectPaths(std::vector<std::string>* object_paths);
+
+  // Gets the byte from the variant data container at the current iterator
+  // position. On success, returns true and advances the
+  // iterator. Variants are widely used in D-Bus services so it's worth
+  // having a specialized function. For instance, The return value type of
+  // "org.freedesktop.DBus.Properties.Get" is a variant.
+  bool PopVariantOfByte(uint8* value);
+  // The following functions do the same for other basic types.

[stevenjb, 2011/07/27 17:56:26]

nit: Don't separate with a comment.

[satorux1, 2011/07/27 21:20:35]

Done.

+  bool PopVariantOfBool(bool* value);
+  bool PopVariantOfInt16(int16* value);
+  bool PopVariantOfUint16(uint16* value);
+  bool PopVariantOfInt32(int32* value);
+  bool PopVariantOfUint32(uint32* value);
+  bool PopVariantOfInt64(int64* value);
+  bool PopVariantOfUint64(uint64* value);
+  bool PopVariantOfDouble(double* value);
+  bool PopVariantOfString(std::string* value);
+  bool PopVariantOfObjectPath(std::string* value);
+
+  // Get the data type of the value at the current iterator
+  // position. INVALID_DATA will be returned if the iterator points to the
+  // end of the message.
+  Message::DataType GetDataType();
+
+ private:
+  // Returns true if the data type at the current iterator position
+  // matches the given D-Bus type, such as DBUS_TYPE_BYTE.
+  bool CheckDataType(int dbus_type);
+
+  // Helper function used to implement PopByte() etc.
+  bool PopBasic(int dbus_type, void *value);
+
+  // Helper function used to implement PopArray() etc.
+  bool PopContainer(int dbus_type, MessageReader* sub_reader);
+
+  // Helper function used to implement PopVariantOfByte() etc.
+  bool PopVariantOfBasic(int dbus_type, void* value);
+
+  Message* message_;
+  DBusMessageIter raw_message_iter_;
+
+  DISALLOW_COPY_AND_ASSIGN(MessageReader);
+};
+
+}  // namespace dbus
+
+#endif  // DBUS_MESSAGE_H_