8:43 a.m.
On Thu, 2012-03-15 at 11:26 +0100, Patrick Ohly wrote:
On Thu, 2012-03-15 at 10:34 +0100, Krzesimir Nowak wrote:
> 2012/3/14 Patrick Ohly <patrick.ohly(a)intel.com>:
> > Hello Chris!
> >
> > You added a callback parameter to addResource() (formerly known as
> > enqueue()) and related functions. There's no documentation for that in
> > server.h/cpp. What is it meant to be used for, what kind of constraints
> > does it have, etc.? In other words, please document it for me... ;-)
> >
>
> That is my doing again. Will document it. The callback was added,
> because addResource calls checkQueue() which calls SetActive, which is
> asynchronous. The callback is called when after all SetActive calls
> are finished or, if there was no call to SetActive, at the end of
> checkQueue(). In short - addResource, removeResource are asynchronous
> too, so given callback is called when they are finished.
Instead of just calling the parameter "callback" it would be good to use
a name which is related to the time when the callback is invoked.
The goal of the callback is to return the final result of a D-Bus method
invocation to the D-Bus client, right?
Why does that have to wait for the completion of SetActive? Can't we
tell the D-Bus client that its StartSession() was successful, then later
emit the right "status changed" signal?
The code is here:
// If not we activate the resource and place it in the active resource list.
if (canRun) {
m_activeResources.push_back(waitingResource);
// If this is a session, we set active and emit sessionChanged to
clients.
boost::shared_ptr<SessionResource> session =
boost::dynamic_pointer_cast<SessionResource>(wq_iter->lock());
if (session) {
++(*counter);
session->setActiveAsync(true,
boost::bind(&Server::setActiveCb, this, session, counter, callback));
}
m_waitingResources.erase(wq_iter++);
}
That seems unnecessarily complex to me, but perhaps I am missing something.
More observations/questions.
ConnectionResource and SessionResource share a lot of code: init(),
onQuit(), onFailure(), ... - basically the whole startup of the helper
seems to have been copied. Shouldn't that common logic be in the
Resource base class?
I also wonder about object ownership while the resource is still in the
process of getting constructed.
void SessionResource::createSessionResource(const Callback_t &callback,
Server &server,
const std::string &peerDeviceID,
const std::string &configName,
const std::string &session,
const std::vector<std::string>
&flags)
{
std::auto_ptr<SessionResource> resource(new SessionResource(server,
peerDeviceID,
configName,
session,
flags));
resource->init(callback);
// init did not throw an exception, so we guess that child was
// spawned successfully. thus we release the auto_ptr, so it will
// not delete the resource.
resource.release();
}
"we guess" is not encouraging when it comes to memory ownership ;-} Do
we know for sure? If createSessionResource() transfers owership to
init(), then it should do so explicitly.
Next, SessionResource::init(). It connects the resource to ForkExec
signals, using a plain pointer to the resource, without the
boost::signal2 track mechanism that I had suggested.
Who owns the SessionResource at that point? Who is going to free it when
something goes wrong before (eventually) addResource() is called? As far
as I can tell, the instance will simply leak.
Another situation is when the D-Bus client quits while its requested
session still gets constructed. The Client instance of the client will
get destructed. But the SessionResource was not yet connected to it, so
the construction cannot be aborted by Server.
I still think resources should be added to m_waitingResources before
construction is complete. Activating the object and notifying the caller
can wait until the resource is really usable (= helper ready).
--
Best Regards, Patrick Ohly
The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.