Bug 19567

Created attachment 21990 [details] [review]
Make _dbus_message_set_serial public

Thanks for the patch,

+dbus_message_demarshal_bytes_needed(const char *str, 

Naming this arg "buf" might be clearer than "str"

+  if (!str || len < DBUS_MINIMUM_HEADER_SIZE)
+    return 0;

!str should be a _dbus_return_if_fail() rather than returning an error, that's a programming bug in the calling app. However, it would be nice to allow NULL if len == 0. So _dbus_return_val_if_fail(str != NULL || len == 0, 0) or something like that.

+  DBusString strtmp;

There is some convention for naming this kind of variable in dbus, I forget, but it isn't strtmp 

+  _dbus_string_append_len (&strtmp, str, len);

You can probably use dbus_string_init_const here and avoid making an allocated copy of the passed-in string.

+  if (have_message || validity == DBUS_VALID)

These should always match (if TRUE is returned, validity should be DBUS_VALID), so should check for only one of them and _dbus_assert() or ignore the other.

+  else
+    {
+      return -1; /* broken! */
+    }

Should not use braces on the else{} but omit them on the if() before it.

Does the API need some way to know what was wrong with the message? Should there be an optional "error string" argument or something? I guess I'd lean toward no.

+int          dbus_message_demarshal_bytes_needed (const char *str, int len);

function args each on their own line

   _dbus_assert (message != NULL);
   _dbus_assert (!message->locked);

With set_serial public, these have to become return_if_fail instead of asserts.

+void          dbus_message_set_serial       (DBusMessage   *message, 
+                                             dbus_uint32_t serial)

missing space before "serial" to line up with "message"

+  /* Lock the message, if it isn't already. */
+  _dbus_message_lock (msg);

Hypothetically this is an API break, because previously I guess you could marshal a message and then still send it to a DBusConnection or modify it further.

I don't know if it would really break anyone. Why did you need to make this change?

Just covering the serious issues with the patch (I'll just go ahead and fix the trivial/cosmetic problems when the discussion is finished)...

(In reply to comment #2)

> +  _dbus_string_append_len (&strtmp, str, len);
> 
> You can probably use dbus_string_init_const here and avoid making an allocated
> copy of the passed-in string.

Yup, I ran into some issues with alignment assertions when I used the init_const variant earlier, but now I can't reproduce that anymore (both the wvstreams and the dbus unit tests I wrote work fine with assertions enabled) and I honestly can't think of how such an assertion could be triggered. I'll pin that down to me doing something else wrong.

> Does the API need some way to know what was wrong with the message? Should
> there be an optional "error string" argument or something? I guess I'd lean
> toward no.

I think the error possibilities (not enough data, broken message) are properly encapsulated in the return codes of the function. So I agree, I see no need for an errstring argument.

> +  /* Lock the message, if it isn't already. */
> +  _dbus_message_lock (msg);
> 
> Hypothetically this is an API break, because previously I guess you could
> marshal a message and then still send it to a DBusConnection or modify it
> further.
> 
> I don't know if it would really break anyone. Why did you need to make this
> change?

Basically so that '_dbus_header_update_lengths' gets called (otherwise the marshalled message might not be demarshallable). A quick test shows that replacing _dbus_message_lock (msg) with: 

_dbus_header_update_lengths  (&msg->header, _dbus_string_get_length (&msg->body));

works fine. Would that be more palatable?

(In reply to comment #3)

> > +  /* Lock the message, if it isn't already. */
> > +  _dbus_message_lock (msg);
> > 
> > Hypothetically this is an API break, because previously I guess you could
> > marshal a message and then still send it to a DBusConnection or modify it
> > further.
> > 
> > I don't know if it would really break anyone. Why did you need to make this
> > change?
> 
> Basically so that '_dbus_header_update_lengths' gets called (otherwise the
> marshalled message might not be demarshallable). A quick test shows that
> replacing _dbus_message_lock (msg) with: 
> 
> _dbus_header_update_lengths  (&msg->header, _dbus_string_get_length
> (&msg->body));
> 
> works fine. Would that be more palatable?

Thinking a bit more about this, I guess the reason I made the call to _dbus_message_lock in the first place was that I wanted to be able lock a message before putting it in the outgoing queue in wvdbus to protect against it being modified afterwards, just as you do in dbus itself. Maybe the right thing to do here is make dbus_message_lock a public function as well? 

Making dbus_message_lock() public is appealing to me. It will need API docs in that case.

Created attachment 23481 [details] [review]
Updated patch to add bytes_needed function

Sorry for the delay, I got busy, and then actually did run into some problems with _dbus_string_init_const which required me to figure out dbus's byte alignment requirements and whether the fix should be applied in my application/library or in D-Bus itself (conclusion: application should deliver only byte aligned data to D-Bus).

Anyway, here's my updated patch which should address all the issues brought up. Note it's just one giant patch which does three things (adds dbus_message_demarshal_bytes_needed, makes dbus_message_set_serial public, makes dbus_message_lock public), hope that's ok.

A few minor tweaks, this seems like it should go in though.

+ * D-Bus will set this value for you when marshalling a message.
+ * This function will generally only be required when encapsulating D-Bus
+ * in another protocol.

s/D-Bus/libdbus/ here. Also, libdbus will set the serial when sending the message through DBusConnection, rather than when marshaling. Something like "DBusConnection will automatically set the serial to an appropriate value when the message is sent; this function is only needed when encapsulating messages in another protocol, or otherwise bypassing DBusConnection."

+  _dbus_return_if_fail (dbus_message_get_serial (message) == 0);

For a public API we should probably drop this; the !locked return_if_fail captures the same idea anyway.

+  /* FIXME: using _dbus_string_init_const_len would be faster, but then
+     it wouldn't be 4-byte aligned, so we run into assertion when we
+     try to demarshal some of the header bytes. */
+  _dbus_string_init_const_len (&str, buf, len);

The comment seems to not match the code? (i.e. init_const_len is used...)

Created attachment 24324 [details] [review]
Updated patch to account for HP's comments

Here's an updated patch, incorporating the last round of suggested changes.

Comment on attachment 24324 [details] [review]
Updated patch to account for HP's comments

This patch looks good to me.

Not sure when the next release is planned but makes sense to include this if nobody else has comments.

Applied, thank you!

commit 16a947eedb7f7f2951fff4ebbf301af7776aa8df
Author: William Lachance <wrlach@gmail.com>
Date:   Tue Apr 21 13:51:46 2009 -0400

    Bug 19567 - Make marshaling code usable without DBusConnection
    
    Some projects want to reuse the DBus message format, without
    actually going through a DBusConnection.  This set of changes
    makes a few functions from DBusMessage public, and adds a new
    function to determine the number of bytes needed to demarshal
    a message.
    
    Signed-off-by: Colin Walters <walters@verbum.org>