[patch] demarshalling messages

[Havoc Pennington]

Hi,

Dafydd Harries wrote:
> The naming I've used is dbus_message_{to,from}_raw.

I think dbus_message_marshal/demarshal or 
dbus_message_to_bytes/from_bytes would be a little nicer, obviously a 
subjective thing.

API documentation should be a little more extensive I think; explaining 
that these are specialized functions that convert to the marshaled form 
of a message as documented in the dbus specification, and maybe in a 
sentence or two explaining when they might be used ("to transmit a dbus 
message stream through another protocol" or something).

> +int
> +dbus_message_to_raw (DBusMessage *msg, char **ret)

Style-wise this should have the args on two separate lines and aligned.

API-wise the dbus convention would be:
dbus_bool_t
dbus_message_to_raw(DBusMessage   *message,
	            char         **marshaled_p,
                     int           *len_p)

where FALSE return means no memory.

> +{
> +  DBusString tmp;
> +  int len;
> +

There should be a _dbus_return_val_if_fail checking all three args are 
not NULL, since they are all mandatory args.

> +  if (!_dbus_string_init (&tmp))
> +    return 0;
> +
> +  _dbus_string_append_len (&tmp,
> +      _dbus_string_get_data (&(msg->header.data)),
> +      _dbus_string_get_length (&(msg->header.data)));

I believe there's a _dbus_string_copy or some other function that would 
allow you to avoid converting to char* and then back to DBusString.
If there isn't we should add one.

> +  _dbus_string_append_len (&tmp,
> +      _dbus_string_get_data (&(msg->body)),
> +      _dbus_string_get_length (&(msg->body)));

I think there's a little more too this, I don't have the code at hand 
but iirc there is some trick with the header padding - there may be a 
_dbus_message_get_stuff_to_send_over_the_wire kind of method that 
handles this? I'm not sure. Just a vague memory, I can look later if you 
don't see anything in the code just before the _dbus_write that writes 
out the message.

> +  len = _dbus_string_get_length (&tmp);
> +
> +  if (!_dbus_string_copy_data (&tmp, ret))

There is a _dbus_string_steal_data that would avoid an extra copy I 
think, or if there isn't we should probably add one.

> +/**
> + * Turn a marshalled DBusMessage into a DBusMessage.
> + *
> + * @param len the length of str
> + * @param str the marshalled DBusMessage
> + * @returns #NULL if not enough memory

or #NULL if a corrupt message

> + */
> +DBusMessage *
> +dbus_message_from_raw (int len, const char *str)

This one I would propose should have a DBusError explaining the problem 
with the message, even if for now the error is always one of "message 
corrupted" or "out of memory" - if there can be two different errors we 
need the DBusError and not just the one return value, and in the future 
we might give a more specific message about how the message is corrupted.

Having the len after the str would be standard for libdbus, instead of 
before it.

> +{
> +  DBusMessageLoader *loader;
> +  DBusString *buffer;
> +  DBusMessage *msg;
> +

Here too _dbus_return_val_if_fail should be added.

> +  loader = _dbus_message_loader_new ();
> +
> +  if (loader == NULL)
> +    return NULL;
> +
> +  _dbus_message_loader_get_buffer (loader, &buffer);
> +  _dbus_string_append_len (buffer, str, len);

This could fail due to OOM right, so would need a check for that.
>  
> +int dbus_message_to_raw (DBusMessage *msg, char **ret);
> +DBusMessage *dbus_message_from_raw (int len, const char *str);

Here also there's some formatting to do, look for the 
"egtk-format-protos" emacs macro if you use emacs

Havoc