Re: [dbus-cplusplus-devel] API issue in request_name()

[Andreas V. <li...@br...>]

Am Mon, 14 Sep 2009 15:59:46 -0400 schrieb Randell Jesup:

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.

Maybe someone else has a better idea, but that would be my way.

regards
	Andreas

> The request_name() API has an issue:
> 
> Currently it looks like this:
> void Connection::request_name(const char *name, int flags)
> {
> ...
> 	dbus_bus_request_name(_pvt->conn, name, flags,
> e);	//we deliberately don't check return value ...
> }
> 
> It explicitly ignores the return code from dbus_bus_request_name().
> It does pass the flags, such as DBUS_NAME_FLAG_DO_NOT_QUEUE,
> ..._REPLACE_EXISTING, etc
> 
> This is a problem, since if you get an (ignored) response such as
> DBUS_REQUEST_NAME_REPLY_EXISTS, you as the caller to request_name have
> no way to tell if you ended up owning the name.  This makes
> implementing a singleton object problematic.  I may also be missing
> something about how the API is supposed to work (perhaps related to
> the match set up...), but it's not obvious.
> 
> request_name() should export the result so the application can decide
> if it wants to take action.
> 
> // flags are normal dbus_bus_request_name flags, result is normal
> return int Connection::request_name(const char *name, int flags)
> {
> 	InternalError e;
>         int result;
> 
> 	debug_log("%s: registering bus name %s", unique_name(), name);
> 
> 	result = dbus_bus_request_name(_pvt->conn, name, flags, e);
> 
> 	if (e) throw Error(e);
> 
> //	this->remove_match("destination");
> 
> 	if (name)
> 	{
> 		_pvt->names.push_back(name);
> 		std::string match = "destination='" +
> _pvt->names.back() + "'"; add_match(match.c_str());
> 	}
> 
>         return result;
> }
> 
> 
> Patch to implement this and add some docs to request_name():
> 
> Index: include/dbus-c++/connection.h
> ===================================================================
> RCS
> file: /usr/cvs/arm-utils/modules/dbus-cpp/include/dbus-c++/connection.h,v
> retrieving revision 1.1.1.1 diff -u -r1.1.1.1 connection.h
> --- include/dbus-c++/connection.h	28 Aug 2009 22:18:07
> -0000	1.1.1.1 +++ include/dbus-c++/connection.h	14 Sep
> 2009 19:59:50 -0000 @@ -408,7 +408,19 @@
>  	 */
>  	PendingCall send_async( Message& msg, int timeout = -1);
>  
> -	void request_name( const char* name, int flags = 0 );
> +	/*!
> +	 * \brief Requests ownership of the given name.
> +	 *
> +	 * The returned result will be one of be one of the normal
> results of
> +         * dbus_bus_request_name(), such as
> DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER
> +	 * or DBUS_DBUS_REQUEST_NAME_REPLY_IN_QUEUE if successful.
> +         * See <dbus-1.0/dbus-shared.h>.  Errors are thrown.
> +	 * 
> +	 * \param name The name to register as the owner of
> +         * \param flags Flags such as DBUS_NAME_FLAG_REPLACE_EXISTING
> +	 * \throw Error
> +         */
> +	int request_name( const char* name, int flags = 0 );
>      
>      unsigned long sender_unix_uid(const char *sender);
>  
> Index: src/connection.cpp
> ===================================================================
> RCS file: /usr/cvs/arm-utils/modules/dbus-cpp/src/connection.cpp,v
> retrieving revision 1.1.1.1
> diff -u -r1.1.1.1 connection.cpp
> --- src/connection.cpp	28 Aug 2009 22:18:08 -0000
> 1.1.1.1 +++ src/connection.cpp	14 Sep 2009 19:59:50 -0000
> @@ -378,13 +378,15 @@
>  	return PendingCall(new PendingCall::Private(pending));
>  }
>  
> -void Connection::request_name(const char *name, int flags)
> +// flags are normal dbus_bus_request_name flags, result is normal
> return +int Connection::request_name(const char *name, int flags)
>  {
>  	InternalError e;
> +        int result;
>  
>  	debug_log("%s: registering bus name %s", unique_name(),
> name); 
> -	dbus_bus_request_name(_pvt->conn, name, flags,
> e);	//we deliberately don't check return value
> +	result = dbus_bus_request_name(_pvt->conn, name, flags, e);
>  
>  	if (e) throw Error(e);
>  
> @@ -396,6 +398,8 @@
>  		std::string match = "destination='" +
> _pvt->names.back() + "'"; add_match(match.c_str());
>  	}
> +
> +        return result;
>  }
>  
>  unsigned long Connection::sender_unix_uid(const char *sender)
> 
> -- 
> Randell Jesup, Worldgate (developers of the Ojo videophone)
> rj...@wg...
> 
> ------------------------------------------------------------------------------
> Let Crystal Reports handle the reporting - Free Crystal Reports 2008
> 30-Day trial. Simplify your report design, integration and deployment
> - and focus on what you do best, core application coding. Discover
> what's new with Crystal Reports now.  http://p.sf.net/sfu/bobj-july
> _______________________________________________
> dbus-cplusplus-devel mailing list
> dbu...@li...
> https://lists.sourceforge.net/lists/listinfo/dbus-cplusplus-devel
>