patch review comments

[Olivier Andrieu]

Hi,

 Havoc Pennington [Tue, 11 May 2004]:
 > Olivier patch for OwnerChanged
 > ===
 > 
 > On the issue of dbus_message_get_args() not supporting
 > STRING_OR_NIL, indeed I think we need to deal with this. The
 > problem with silently assuming any string is STRING_OR_NIL is that
 > apps would have to manually check != NULL on all strings. My best
 > suggestion is to introduce OR_NIL into type codes?

Yes, but not the typecodes in dbus-protocol.h since this does not
concern the wire protocol. Maybe something like this, in
dbus-message.h :

enum DBusArgType {
  DBUS_ARG_TYPE_INVALID     = DBUS_TYPE_INVALID, 
  DBUS_ARG_TYPE_NIL         = DBUS_TYPE_NIL, 
  DBUS_ARG_TYPE_BYTE        = DBUS_TYPE_BYTE, 
  DBUS_ARG_TYPE_BOOLEAN     = DBUS_TYPE_BOOLEAN, 
  DBUS_ARG_TYPE_INT32       = DBUS_TYPE_INT32, 
  DBUS_ARG_TYPE_UINT32      = DBUS_TYPE_UINT32, 
  DBUS_ARG_TYPE_INT64  	    = DBUS_TYPE_INT64, 
  DBUS_ARG_TYPE_UINT64 	    = DBUS_TYPE_UINT64, 
  DBUS_ARG_TYPE_DOUBLE 	    = DBUS_TYPE_DOUBLE, 
  DBUS_ARG_TYPE_STRING 	    = DBUS_TYPE_STRING, 
  DBUS_ARG_TYPE_CUSTOM 	    = DBUS_TYPE_CUSTOM, 
  DBUS_ARG_TYPE_ARRAY       = DBUS_TYPE_ARRAY, 
  DBUS_ARG_TYPE_DICT        = DBUS_TYPE_DICT, 
  DBUS_ARG_TYPE_OBJECT_PATH = DBUS_TYPE_OBJECT_PATH, 
  DBUS_ARG_TYPE_STRING_OR_NIL 
};


 > There's a comment /* WEIRD */ that makes me nervous about the code
 > there but I don't really know what was considered weird ;-)

It's the various message checks that looked weird to me. In this
function (check_existent_service_auto_activation) there are checks for
ServiceOwnerChanged signals, for error messages coming from
DBUS_INTERFACE_ORG_FREEDESKTOP_DBUS and also for the reply to the
auto-activating method call.

Aren't the checks for errors coming from the bus superfluous ? As I
understand things, ERROR messages can only arrive as replies to
METHOD_CALL messages and a METHOD_CALL can receive at most one reply.
So it would be really weird if the bus was sending, on its own, an
activation error in addition to the method reply !

But since this test is half-disabled (does not go through
_dbus_test_oom_handling) I can't tell for sure.

-- 
   Olivier