Re: [dbus-cplusplus-devel] contributing a submission to the project...best way?

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

Am Fri, 20 Nov 2009 09:58:38 -0800 schrieb Freyensee, James P:

> On the examples, I don't think they are important.  I know changes
> were made just to get them to compile so if the examples compile for
> you, then that's good :-).

I didn't try it. I hope you'll give me feedback if it works for you.

> On:
> >connection.h/connection.cpp:
> >extern DXXAPI Dispatcher* default_dispatcher;
> >...and all changes specific to the change that Connection
> >constructors have a new parameter Dispatcher* dispatcher.
> 
> >I've a raw idea what's the reason for it. But could you explain what
> >was the main idea behind this change.
> 
> I cannot give you an exact reason, but I can give you a general
> reason.  We want a safeguard of being able to set up a single default
> dispatch mechanism for all Dbus connections we try to establish in a
> project.  For example, there is this project we have open-sourced for
> a 3rd party:
> 
> https://sourceforge.net/projects/optelephony
> 
> that heavily uses this concept when trying to establish an initial
> session with the 3rd party's product:
> 
> // from https://sourceforge.net/projects/optelpehony
> bool TelSessionClient::initSessionClient(){
> 
>    if(!mDispatcher){
>     mDispatcher = new DBus::Glib::BusDispatcher();
>     DBus::default_dispatcher = mDispatcher;
>     mDispatcher->attach(NULL);
>    }
> 
>    if(!mConnection){
>     try{
>     mConnection = new DBus::Connection(DBus::Connection::SystemBus());
>     }
>     catch (DBus::Error e) {
>      printf("Can not connect to the SystemBus : %s\n",e.message());
>      return 0;
>     }
>     mConnection->set_timeout(120*1000);
>    }
> 
>    if(!cb)
>     cb = new TelCallBack(*mConnection);
> 
>    if(!server)
>     try{
>      server = new TelServerTAPI (*mConnection);
>     }
>     catch (DBus::Error e){
>      return false;
>     }
>    mDispatcher->enter();
>    return true;
> }

I've a little problem with this patch. See here:

before:
Connection Connection::SystemBus()
{
	return Connection(new Private(DBUS_BUS_SYSTEM));
}

after patch:
Connection Connection::SystemBus(Dispatcher* dispatcher )
{
	return Connection(new Private(DBUS_BUS_SYSTEM),dispatcher);
}


But look at Private:

	Private(DBusConnection *, Server::Private * = NULL);

	Private(DBusBusType);

In "Before" the second constructor with DBusBusType was called before.
But now you call a Constructor with (DBusBusType, Dispatcher*) what
doesn't seem to have such a constructor. I didn't compile your sources,
but does this compile? Or do I have some merging problem in Private
here?

> On:
> >generate_adaptor.cpp/generate_proxy.cpp/generator_utils.cpp:
> 
> >What are exactly the reason of these change? Looks for me like some
> >sort of Async method support. Could you spend some more words here?
> 
> that actually did not come from the change we wanted to apply to your
> project.  What looks like has happened is these files used to be in
> xml2cpp.cpp and were separated out at some point.  I believe the
> commit ID on the git tree when this happened was c2b8cb (maybe??).  I
> don't think this is really a part of the libdbus-c++ functionality as
> is a part of a tool to create .cpp code from .xml code, so I don't
> believe this effects us at all.

Ok. Async support is on my TODO list. So I'll later take a look on it.

> On:
> 
> > eventloop.cpp:
> > DefaultMutex::DefaultMutex()
> > {
> 
> >   pthread_mutex_t recmutex = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP;
> >  _mutex = recmutex;
> > //_mutex = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP;
> > //pthread_mutex_init(&_mutex, NULL);
> >}
> 
> We don't seem to use the class DefaultMutex or the methods where the
> DefaultMutex instance variable _mutex_p is used (queue_connection(),
> dispatch_pending()).  And I know we did not comment that code out.  I
> could tell you a possible reason for why
> PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP is there.  It prevents a
> certain deadlock case if PTHREAD_RECURSIVE_MUTEX_INITIALIZER was used
> instead.  If PTHREAD_RECURSIVE_MUTEX_INITIALIZER was used, multiple
> calls of pthread_mutex_lock without calling pthread_mutex_unlock by
> the owner of the mutex could result in dead-lock.  I have no idea if
> the original intent was to be able to use this code recursively, but
> it may be a good idea to leave this alone (well, with the exception
> of removing the commented-out lines)?

I reviewed the code and couldn't see a reason for using the recursive
mutex code here. So I just did this:

	DefaultMutex(bool recursive = false);

DefaultMutex::DefaultMutex(bool recursive)
{
	if (recursive)
	{
		pthread_mutex_t recmutex =
PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP; _mutex = recmutex;
	}
	else
	{
		pthread_mutex_init(&_mutex, NULL);
	}
}

So as default we've the old implementation. If you ever see a problem
with it we may change to the recursive mutex as default. Is this ok for
you? How ever, I commited this.

regards
	Andreas Volz