should request_name org.freedesktop.DBus fail?

[Robert McQueen]

I'd consider this particular case to be a bug in both the documentation
and the library implementation, they are inconsistent in behaviour.

Docs say:
 This function returns a result code. The possible result codes are as
 follows:
  DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER ...
  DBUS_REQUEST_NAME_REPLY_IN_QUEUE ...
  DBUS_REQUEST_NAME_REPLY_EXISTS ...
  DBUS_REQUEST_NAME_REPLY_ALREADY_OWNER ...

The RequestName implementation in bus/service.c does stuff like:
  if (!_dbus_validate_bus_name (service_name, 0,
                                _dbus_string_get_length (service_name)))
    {
      dbus_set_error (error, DBUS_ERROR_INVALID_ARGS,
                      "Requested bus name \"%s\" is not valid",
                      _dbus_string_get_const_data (service_name));

      _dbus_verbose ("Attempt to acquire invalid service name\n");

      goto out;
    }

If the method call from the bus results in an error, -1 is returned.
This is not documented anywhere.

But in request_name there are a load of checks which don't set errors,
and don't return -1 either:
  _dbus_return_val_if_fail (connection != NULL, 0);
  _dbus_return_val_if_fail (name != NULL, 0);
  _dbus_return_val_if_fail (_dbus_check_is_valid_bus_name (name), 0);
  _dbus_return_val_if_error_is_set (error, 0);

Solution 1 (lots of work, not immediately obvious its a win):
 1) Document that -1 will be returned when the error is set.
 2) Set errors in the cases where 0 was previously returned, and return -1.

Solution 2 (application writer gets no idea why method returns 0):
 1) Document that 0 will be returned if arguments are invalid, and that
-1 will be returned when the error is set.

Solution 3 (probably preferable):
 1) Document that 0 will be returned if the arguments are invalid, and
that -1 will be returned when the error is set.
 2) In the case we're doing a check on the bus name, set an error rather
than just returning 0.

Similar behaviour should also be verified in other library functions
which do local checking before invoking the function on the bus. I
really don't have time to do this, I've already spent too long writing
this e-mail. I'll copy/paste this message into a bug report if you wish.

Regards,
Rob

John (J5) Palmieri wrote:
> This sounds like something we want to decide on right now since it
> changes the API.  Returning an error seems like the right thing to do
> here though there may be reasons why we don't.
> 
> On Mon, 2005-11-07 at 12:06 +0000, Robert McQueen wrote:
> 
>>At risk of talking to myself... on a related note, if you request a
>>garbage name, the method returns 0 but does not set an error, because
>>the library uses:
>>  _dbus_return_val_if_fail (_dbus_check_is_valid_bus_name (name), 0);
>>At the start of the function, amongst others.
>>
>>This and others like it should either be made to return an error, or the
>>fact this function can return 0 in unspecific error cases (rather one of
>>the return values in the documentation) made clear in the API
>>documentation and spec.
>>
>>Regards,
>>Rob