[dbus-cplusplus-devel] API issue in request_name(), race condition?

[Randell J. <rj...@wg...>]

For some reason I'm not receiving email from the list, though I can send
to it...  Please CC me on any reply for now.

Andreas Volz writes:
>Hello Randell,
>
>there was a good reason not to do this. See
>http://dbus.freedesktop.org/doc/api/html/group__DBusBus.html#g4b4903adad0199119c9e49ad18e0cb25
>
>The return value is not a simple int value. It could be some of:
>
>http://dbus.freedesktop.org/doc/api/html/group__DBusShared.html#g3741b483711f0bf115cd39aa7aacd8d2
>
>So simply returning a int would in the next step mean to include some
>dbus headers. This destroys the PIMPL design of dbus-c++. So maybe a
>correct way would be to create a enum with all available states in dbus
>and then use a switch or some table to match the bus to dbus-c++ types.

Well, a few comments: you already allow people to pass in dbus.h #define
values via the undocumented "flags" parameter (one has to read the
source to figure out what values are valid there - DBUS_NAME_FLAG_*).
And "PIMPL" is about having private interfaces not be declared in your
public header files, and hiding implementation, so I don't see a strong
requirement there.  The whole point of dbus-c++ is to wrap dbus, so
exposing dbus enumerations (or our re-enumerations of them) doesn't
bother me.

Second: The underlying return type is 'int'; a change to libdbus (or
alternate implementation) could add return values, so the return should
be 'int', and if you want to avoid needing to include dbus.h you can
mirror those defines in dbus-c++.  If you try to force a conversion to
dbus-c++-specific types and not pass it through, you have to decide what
to do if you get a value you didn't expect:
DBUS_CPLUSPLUS_REQUEST_NAME_REPLY_WTF?  And what does doing all of that
buy you in reality?

Relevant defines:
#define 	DBUS_NAME_FLAG_ALLOW_REPLACEMENT   0x1
#define 	DBUS_NAME_FLAG_REPLACE_EXISTING   0x2
#define 	DBUS_NAME_FLAG_DO_NOT_QUEUE   0x4

#define 	DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER   1
#define 	DBUS_REQUEST_NAME_REPLY_IN_QUEUE   2
#define 	DBUS_REQUEST_NAME_REPLY_EXISTS   3
#define 	DBUS_REQUEST_NAME_REPLY_ALREADY_OWNER   4

Single-instance servers are a major use-case, and need to be supported
easily.  If you want, you could wrap some of the functionality to make
things simpler on the implementer, instead of having to do this:

    DBus::Connection conn = DBus::Connection::SessionBus();
    FooService server(conn);

    // Make sure we never get two instances running!
    try 
    {
        int result;
        if ((result = conn.request_name(FOO_SERVER_NAME,DBUS_NAME_FLAG_DO_NOT_QUEUE)) !=
            DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER)
        {
            syslog(LOG_ERR,"Request_name returned %d",result);
            ecore_shutdown();
            return 1;
        }
    }
    catch(DBus::Error &e) 
    {
        syslog(LOG_ERR,"Service name not available: %s",e.message());
        ecore_shutdown();
        return 1;
    }

Also, while I'm at it, there seems to be a race condition either in
dbus-c++ (or in base dbus protocol, which seems less likely):

The sequence above causes this dbus traffic:
--------------------------------------------
signal sender=org.freedesktop.DBus -> dest=(null destination) path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=NameOwnerChanged
   string ":1.22"
   string ""
   string ":1.22"
method call sender=:1.22 -> dest=org.freedesktop.DBus path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=Hello
signal sender=org.freedesktop.DBus -> dest=(null destination) path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=NameOwnerChanged
   string "com.foo.FooService"
   string ""
   string ":1.22"
method call sender=:1.22 -> dest=org.freedesktop.DBus path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=RequestName
   string "com.foo.FooService"
   uint32 4
method call sender=:1.22 -> dest=org.freedesktop.DBus path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=AddMatch
   string "destination='com.foo.FooService'"
--------------------------------------------

That's fine, except that if someone is watching NameOwnerChanged for a
new com.foo.FooService owner, if they send a method to FooService
immediately it occurs *before* the last AddMatch, and doesn't get
delivered to FooService for 25 seconds (when a retry apparently occurs
in dbus). If the watcher adds a short delay (and thus AddMatch has been
done), the method is delivered to FooService immediately.

I suspect that dbus-c++ should be adding the match earlier, not after
dbus_bus_request_name() returns.

-- 
Randell Jesup, Worldgate (developers of the Ojo videophone)
rj...@wg...