[patch] demarshalling messages

[Dafydd Harries]

Ar 29/03/2007 am 14:48, ysgrifennodd Havoc Pennington:
> Hi,
> 
> Dafydd Harries wrote:
> >Ar 26/03/2007 am 21:07, ysgrifennodd Havoc Pennington:
> >>Dafydd Harries wrote:
> >+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

I went for marshaled_data_p.

> 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.

I assume you meant to take the length of tmp after copying the hearder, and
inserting the body at that point.

> >+  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.

Done.

> >+  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?

You're right, writing 0 bytes to a DBusMessageLoader results in an error, so
this check is redundant. I've removed it.

> 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.

Test added; new patch attached.

-- 
Dafydd
-------------- next part --------------
A non-text attachment was scrubbed...
Name: libdbus-marshal-2.diff
Type: text/x-diff
Size: 5567 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/dbus/attachments/20070405/f381d197/libdbus-marshal-2.bin