[patch] demarshalling messages

[Havoc Pennington]

Hi,

Dafydd Harries wrote:
> Ar 26/03/2007 am 21:07, ysgrifennodd Havoc Pennington:
>> Dafydd Harries wrote:
>>> +  _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.
> 
> Are you maybe thinking of correct_header_padding? That seems to be called
> whenever the header is modified, though.

I guess I was thinking _dbus_message_get_network_data might do something 
other than just return the data. But looking at it, it does not.

> +dbus_bool_t
> +dbus_message_marshal (DBusMessage  *msg,
> +                      char        **ret,
> +                      int          *len_p)

As a total nitpick I think "buffer_return" or "marshaled_data_p" would 
be a better name than ret

Remember to keep the names the same in the header and .c file or doxygen 
tends to get confused.

> +  if (!_dbus_string_copy (&(msg->header.data), 0, &tmp, 0))
> +    goto fail;
> +
> +  if (!_dbus_string_append_len (&tmp,
> +      _dbus_string_get_data (&(msg->body)),
> +      _dbus_string_get_length (&(msg->body))))
> +    goto fail;

You can use _dbus_string_copy here, as you did for the header.

> +  if (str == NULL)
> +    {
> +      dbus_set_error (error, DBUS_ERROR_INVALID_ARGS,
> +          "String to demarshal must not be null");
> +      return NULL;
> +    }

This check should be a return_if_fail rather than an error, since a null 
string would be a misuse of the API by the app.

> +  if (len == 0)
> +    {
> +      dbus_set_error (error, DBUS_ERROR_INVALID_ARGS,
> +          "Buffer to demarshal must be longer than 0 bytes");
> +      return NULL;
> +    }

This is correctly a DBusError I would say, though, since if you're 
saying "parse this string I got off the network" it may happen to be 
zero-length.

However, I wonder if you need to check for this - won't it already be 
handled as an incomplete message, just as if you only had 1 byte?

Patch is looking good, thanks. One more thing ;-) if you could be sure 
the unit tests at least minimally invoke these functions that might 
prevent some paper bag releases in the future. I think if you just added 
a marshal and demarshal of the "test message" near the end of 
dbus-message-util.c:_dbus_message_test() (right before where it says 
"Load all the sample messages...") and then did a verify_test_message() 
on the demarshaled message, that would be sufficient.

For bonus points, check the demarshal on an empty string and a garbage 
string, and be sure it returns an error for those.

Havoc