10:32 p.m.
Hi Patrick,
This week I've been going down the path to get the password request
working. However, to get the session to even request a password we've
got to start a session which is one of the things I've been working
on.
One question I've got is when do we go about activating the sessions.
I'm assuming this can be done immediately. The approach I'm taking now
is to set the session 'active' and emit a status signal as soon as the
helper session's up and ready. Thus, the server creates a session and
it runs immediately.
I've also needed to look at how shutdown is handled in the course of
this. From what I can tell there are 2 events that can cause a
shutdown request: a signal or a file change. Because we are now
dealing with the ideal case that we have no queued sessions & multiple
sessions are running concurrently, it doesn't seem to me that we need
a ShutdownSession any longer. The shutdown session seemed to be used
to get to the head of the queue and prevent any new sessions from
starting. I'm going about removing that and just using the
m_shutdownRequested flag to disallow starting any additional sessions
etc.
Now for the actual infoRequest I'm doing the following. When the
request is made the helper emits a password request signal and waits.
The helper only does one job, so this is fine. The SessionResource in
the server receives this signal and creates an InfoRequest. Now we
can't wait in the server process because we've got possibly multiple
concurrent sessions running. So, I've introduced signal in InfoReq
that lets us know when the response is received. This signal is
connected to a routine in the SessionResource which passes the result
to the session helper who can now carry on its business.
I welcome any input as to whether these are all sound assumptions.
This code will land very soon.
Cheers,
Chris
12:45 a.m.
On Sat, 2012-02-04 at 12:32 +0100, Chris Kühl wrote:
One question I've got is when do we go about activating the
sessions.
I'm assuming this can be done immediately. The approach I'm taking now
is to set the session 'active' and emit a status signal as soon as the
helper session's up and ready. Thus, the server creates a session and
it runs immediately.
The 'active' state indicates that the session is allowed to be used for
operations. It doesn't guarantee that the session will react
immediately. So you could set it to active in the syncevo-dbus-server
side immediately, without waiting for the helper side to be ready. Is
that perhaps simpler?
I've also needed to look at how shutdown is handled in the course
of
this. From what I can tell there are 2 events that can cause a
shutdown request: a signal or a file change. Because we are now
dealing with the ideal case that we have no queued sessions & multiple
sessions are running concurrently,
You cannot guarantee that they are no queued sessions. A new session
might conflict with a running one and thus cannot start immediately.
it doesn't seem to me that we need
a ShutdownSession any longer. The shutdown session seemed to be used
to get to the head of the queue and prevent any new sessions from
starting. I'm going about removing that and just using the
m_shutdownRequested flag to disallow starting any additional sessions
etc.
Do whatever works :-) The goal has to be that queued sessions are
discarded as soon as possible instead of delaying the shutdown.
One open question is whether the syncevo-dbus-server should be allowed
to quit while there is a running sync. In the past it wasn't, because
*itself* was running the sync. Now I think the sync should continue to
run unless it was canceled explicitly itself. For example, if the
syncevo-dbus-server crashes for some reason, it would be nice to
complete the running sync instead of aborting it prematurely.
Now for the actual infoRequest I'm doing the following. When the
request is made the helper emits a password request signal and waits.
The helper only does one job, so this is fine. The SessionResource in
the server receives this signal and creates an InfoRequest. Now we
can't wait in the server process because we've got possibly multiple
concurrent sessions running. So, I've introduced signal in InfoReq
that lets us know when the response is received. This signal is
connected to a routine in the SessionResource which passes the result
to the session helper who can now carry on its business.
Sounds reasonable.
--
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.
1:35 p.m.
On Sat, Feb 4, 2012 at 2:45 PM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
On Sat, 2012-02-04 at 12:32 +0100, Chris Kühl wrote:
> One question I've got is when do we go about activating the sessions.
> I'm assuming this can be done immediately. The approach I'm taking now
> is to set the session 'active' and emit a status signal as soon as the
> helper session's up and ready. Thus, the server creates a session and
> it runs immediately.
The 'active' state indicates that the session is allowed to be used for
operations. It doesn't guarantee that the session will react
immediately. So you could set it to active in the syncevo-dbus-server
side immediately, without waiting for the helper side to be ready. Is
that perhaps simpler?
Yes, as soon as it's add/queued it'll be marked as active in the server process.
> I've also needed to look at how shutdown is handled in the
course of
> this. From what I can tell there are 2 events that can cause a
> shutdown request: a signal or a file change. Because we are now
> dealing with the ideal case that we have no queued sessions & multiple
> sessions are running concurrently,
You cannot guarantee that they are no queued sessions. A new session
might conflict with a running one and thus cannot start immediately.
Yeah, unfortunately things are not ideal. Have you given any further
thought as to how we are going to properly detect conflicting sync
sessions? This points getting close that this is going to need to be
dealt with. I've not looked into this at length yet. Is there no
interface I could use to test that syncs will not conflict?
> it doesn't seem to me that we need
> a ShutdownSession any longer. The shutdown session seemed to be used
> to get to the head of the queue and prevent any new sessions from
> starting. I'm going about removing that and just using the
> m_shutdownRequested flag to disallow starting any additional sessions
> etc.
Do whatever works :-) The goal has to be that queued sessions are
discarded as soon as possible instead of delaying the shutdown.
One open question is whether the syncevo-dbus-server should be allowed
to quit while there is a running sync. In the past it wasn't, because
*itself* was running the sync. Now I think the sync should continue to
run unless it was canceled explicitly itself. For example, if the
syncevo-dbus-server crashes for some reason, it would be nice to
complete the running sync instead of aborting it prematurely.
Ok, I've gone the route of ripping the shutdownSession out and
enabling the sync to run even when the server shuts down. This
actually results in a decent amount of code being removed in my
yet-to-be-tested commit.
Cheers,
Chris
7:04 p.m.
On Tue, 2012-02-07 at 03:35 +0100, Chris Kühl wrote:
Yeah, unfortunately things are not ideal. Have you given any further
thought as to how we are going to properly detect conflicting sync
sessions? This points getting close that this is going to need to be
dealt with. I've not looked into this at length yet. Is there no
interface I could use to test that syncs will not conflict?
Nevertheless, it would be nice to punt this to a later patch/branch,
just so we have some chance to get the current work into master.
--
Murray Cumming
murrayc(a)murrayc.com
www.murrayc.com
www.openismus.com
7:23 p.m.
On Tue, 2012-02-07 at 09:04 +0100, Murray Cumming wrote:
On Tue, 2012-02-07 at 03:35 +0100, Chris Kühl wrote:
> Yeah, unfortunately things are not ideal. Have you given any further
> thought as to how we are going to properly detect conflicting sync
> sessions? This points getting close that this is going to need to be
> dealt with. I've not looked into this at length yet. Is there no
> interface I could use to test that syncs will not conflict?
Nevertheless, it would be nice to punt this to a later patch/branch,
just so we have some chance to get the current work into master.
I agree. Let's keep the current behavior (only one session can run at a
time) and figure out how to detect non-conflicting sessions later. Don't
bite off more than you can swallow :-)
--
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.
10:44 p.m.
On Tue, Feb 7, 2012 at 9:23 AM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
On Tue, 2012-02-07 at 09:04 +0100, Murray Cumming wrote:
> On Tue, 2012-02-07 at 03:35 +0100, Chris Kühl wrote:
> > Yeah, unfortunately things are not ideal. Have you given any further
> > thought as to how we are going to properly detect conflicting sync
> > sessions? This points getting close that this is going to need to be
> > dealt with. I've not looked into this at length yet. Is there no
> > interface I could use to test that syncs will not conflict?
>
> Nevertheless, it would be nice to punt this to a later patch/branch,
> just so we have some chance to get the current work into master.
I agree. Let's keep the current behavior (only one session can run at a
time) and figure out how to detect non-conflicting sessions later. Don't
bite off more than you can swallow :-)
Ok, I'll reimplement the queue then.
I was thinking that with an interface in place I could actually have
the code in-place that would do the concurrent sessions even if now it
would always return that there is a conflict. For testing that
concurrent sessions do actually work with known, non-conflicting
sources, we could introduce an environment variable that would return
that there is no conflict. That way, as soon a solution is found ofr
finding conflicts, is should just work.
Patrick, you were also mentioning that you'd like to rework the
autosync mechanism. Shall I strip out that functionality from the
server?
Cheers,
Chris
6:19 a.m.
On Tue, 2012-02-07 at 12:44 +0100, Chris Kühl wrote:
On Tue, Feb 7, 2012 at 9:23 AM, Patrick Ohly
<patrick.ohly(a)intel.com> wrote:
> On Tue, 2012-02-07 at 09:04 +0100, Murray Cumming wrote:
>> On Tue, 2012-02-07 at 03:35 +0100, Chris Kühl wrote:
>> > Yeah, unfortunately things are not ideal. Have you given any further
>> > thought as to how we are going to properly detect conflicting sync
>> > sessions? This points getting close that this is going to need to be
>> > dealt with. I've not looked into this at length yet. Is there no
>> > interface I could use to test that syncs will not conflict?
>>
>> Nevertheless, it would be nice to punt this to a later patch/branch,
>> just so we have some chance to get the current work into master.
>
> I agree. Let's keep the current behavior (only one session can run at a
> time) and figure out how to detect non-conflicting sessions later. Don't
> bite off more than you can swallow :-)
>
Ok, I'll reimplement the queue then.
I was thinking that with an interface in place I could actually have
the code in-place that would do the concurrent sessions even if now it
would always return that there is a conflict. For testing that
concurrent sessions do actually work with known, non-conflicting
sources, we could introduce an environment variable that would return
that there is no conflict. That way, as soon a solution is found ofr
finding conflicts, is should just work.
That makes sense.
Patrick, you were also mentioning that you'd like to rework the
autosync mechanism. Shall I strip out that functionality from the
server?
Please ifdef out or comment any code which no longer compiles, but keep
it in place. I'll have a look this week.
The core decision making still belongs into the main
syncevo-dbus-server. That's the right place to track when sessions ran
and are meant to run again.
--
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.
8:08 p.m.
On Tue, Feb 7, 2012 at 8:19 PM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
On Tue, 2012-02-07 at 12:44 +0100, Chris Kühl wrote:
> On Tue, Feb 7, 2012 at 9:23 AM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
> > On Tue, 2012-02-07 at 09:04 +0100, Murray Cumming wrote:
> >> On Tue, 2012-02-07 at 03:35 +0100, Chris Kühl wrote:
> >> > Yeah, unfortunately things are not ideal. Have you given any further
> >> > thought as to how we are going to properly detect conflicting sync
> >> > sessions? This points getting close that this is going to need to be
> >> > dealt with. I've not looked into this at length yet. Is there no
> >> > interface I could use to test that syncs will not conflict?
> >>
> >> Nevertheless, it would be nice to punt this to a later patch/branch,
> >> just so we have some chance to get the current work into master.
> >
> > I agree. Let's keep the current behavior (only one session can run at a
> > time) and figure out how to detect non-conflicting sessions later. Don't
> > bite off more than you can swallow :-)
> >
>
> Ok, I'll reimplement the queue then.
>
> I was thinking that with an interface in place I could actually have
> the code in-place that would do the concurrent sessions even if now it
> would always return that there is a conflict. For testing that
> concurrent sessions do actually work with known, non-conflicting
> sources, we could introduce an environment variable that would return
> that there is no conflict. That way, as soon a solution is found ofr
> finding conflicts, is should just work.
That makes sense.
Ok, should have this set up today.
> Patrick, you were also mentioning that you'd like to rework
the
> autosync mechanism. Shall I strip out that functionality from the
> server?
Please ifdef out or comment any code which no longer compiles, but keep
it in place. I'll have a look this week.
Compiles fine so far. Just seemed like you weren't really enthusiastic
about it and it seems that 2 of the 3 scenarios where it's designed to
be used are not implemented. I'll leave it as is.
The core decision making still belongs into the main
syncevo-dbus-server. That's the right place to track when sessions ran
and are meant to run again.
Agreed, and everything should be that way now. The helper process just
waits for commands, syncs and sends status and progress signals to the
server process.
Cheers,
Chris
12:50 a.m.
On Wed, Feb 8, 2012 at 10:08 AM, Chris Kühl <blixtra(a)gmail.com> wrote:
On Tue, Feb 7, 2012 at 8:19 PM, Patrick Ohly
<patrick.ohly(a)intel.com> wrote:
> On Tue, 2012-02-07 at 12:44 +0100, Chris Kühl wrote:
>> On Tue, Feb 7, 2012 at 9:23 AM, Patrick Ohly <patrick.ohly(a)intel.com>
wrote:
>> > On Tue, 2012-02-07 at 09:04 +0100, Murray Cumming wrote:
>> >> On Tue, 2012-02-07 at 03:35 +0100, Chris Kühl wrote:
>> >> > Yeah, unfortunately things are not ideal. Have you given any
further
>> >> > thought as to how we are going to properly detect conflicting sync
>> >> > sessions? This points getting close that this is going to need to
be
>> >> > dealt with. I've not looked into this at length yet. Is there
no
>> >> > interface I could use to test that syncs will not conflict?
>> >>
>> >> Nevertheless, it would be nice to punt this to a later patch/branch,
>> >> just so we have some chance to get the current work into master.
>> >
>> > I agree. Let's keep the current behavior (only one session can run at a
>> > time) and figure out how to detect non-conflicting sessions later.
Don't
>> > bite off more than you can swallow :-)
>> >
>>
>> Ok, I'll reimplement the queue then.
>>
>> I was thinking that with an interface in place I could actually have
>> the code in-place that would do the concurrent sessions even if now it
>> would always return that there is a conflict. For testing that
>> concurrent sessions do actually work with known, non-conflicting
>> sources, we could introduce an environment variable that would return
>> that there is no conflict. That way, as soon a solution is found ofr
>> finding conflicts, is should just work.
>
> That makes sense.
>
Ok, should have this set up today.
So , I've pushed so changes taking us in this direction. Here is the
basic run down of what these changes include.
- We've now got a priority queue of weak pointers implemented with a
std::set container that is initialized with a Compare class for
inserting new Resources based on their priority.
- There is a method (canRunConcurrently()) in Resource which is
checked but currently always returns false, thus forcing only single
session to be run.
- Once the Resource is made active it's placed the list of active
resources, a list of shared pointers to Resources.
- Implemented some of the outstanding issues with Connection (missing
communication between server and helper processes)
- Allow server process to set helper session's active state.
- Krzesimir implemented the missing functionality in the gio-gdbus
wrapper and added code to do blocking calls with minimal extra code
required in consuming classes.
Note that in the above I always refer to Resources instead of Session
or Connection. Previously the Connection would create a session and
place it in the working queue but not activate the public dbus
interface. Because the Connection and the Session it spawns needs to
be in the same process. Thus the Connection is also queued but has a
higher priority than the other resources.
The above mentioned changes compile and are being tested now. I'll get
back to you with the results of that.
Cheers,
Chris
2:02 a.m.
New subject: [SyncEvolution] C++ D-Bus bindings + blocking calls (was: Re: DBus server rework: sync, shutdown, and password request)
On Mon, 2012-02-13 at 14:50 +0100, Chris Kühl wrote:
- Krzesimir implemented the missing functionality in the gio-gdbus
wrapper and added code to do blocking calls with minimal extra code
required in consuming classes.
In addition to adding a block() method which executes the call, wouldn't
it be nicer to also overload the call operator such that if there is no
Callback_t parameter, all results are returned directly (special case
for a single return value) or as tuple (general case)?
The block() call would be more efficient for large return values (avoids
one copying), but for small ones the ease of use certainly would be
worth it.
I've experimented a bit with it and think it can be done.
Is there still a need for the block() variant if there was a "normal"
blocking call operator()?
--
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.
3:21 a.m.
New subject: [SyncEvolution] C++ D-Bus bindings + blocking calls
On Tue, 2012-02-14 at 16:02 +0100, Patrick Ohly wrote:
On Mon, 2012-02-13 at 14:50 +0100, Chris Kühl wrote:
> - Krzesimir implemented the missing functionality in the gio-gdbus
> wrapper and added code to do blocking calls with minimal extra code
> required in consuming classes.
In addition to adding a block() method which executes the call, wouldn't
it be nicer to also overload the call operator such that if there is no
Callback_t parameter, all results are returned directly (special case
for a single return value) or as tuple (general case)?
The block() call would be more efficient for large return values (avoids
one copying), but for small ones the ease of use certainly would be
worth it.
I've experimented a bit with it and think it can be done.
Is there still a need for the block() variant if there was a "normal"
blocking call operator()?
Here's a partial implementation. Not all combinations of number of
parameters and return values are currently spelled out. Is there a
better approach and would it be worthwhile?
diff --git a/src/gdbus/gdbus-cxx-bridge.h b/src/gdbus/gdbus-cxx-bridge.h
index dc217ca..d56be89 100644
--- a/src/gdbus/gdbus-cxx-bridge.h
+++ b/src/gdbus/gdbus-cxx-bridge.h
@@ -81,6 +81,7 @@ static inline void intrusive_ptr_release(DBusServer
*server) { dbus_server_unref
#include <boost/variant.hpp>
#include <boost/variant/get.hpp>
#include <boost/utility.hpp>
+#include <boost/tuple/tuple.hpp>
/* The connection is the only client-exposed type from the C API. To
* keep changes to a minimum while supporting both dbus
@@ -3913,6 +3914,20 @@ protected:
typedef T Callback_t;
+ /**
+ * Blocking method call: execute call and check for error.
+ * Encoding parameters and decoding return values must be done by
+ * caller.
+ */
+ void doCall(DBusMessagePtr &msg, DBusMessagePtr &reply)
+ {
+ DBusErrorCXX error;
+ reply =
dbus_connection_send_with_reply_and_block(DBusClientCall<Callback_t>::m_conn.get(),
msg.get(), -1, &error);
+ if (!reply) {
+ error.throwFailure(DBusClientCall<Callback_t>::m_method);
+ }
+ }
+
void prepare(DBusMessagePtr &msg)
{
// Constructor steals reference, reset() doesn't!
@@ -4225,6 +4240,20 @@ public:
: DBusClientCall<Callback_t>(object, method,
&DBusClientCall0::dbusCallback)
{
}
+
+ // keep call operator from base class visible
+ using DBusClientCall<Callback_t>::operator ();
+
+ // return once the call is done, throws runtime_error if it fails
+ template <class A1>
+ void operator () (const A1 &a1)
+ {
+ DBusMessagePtr msg;
+ prepare(msg);
+ AppendRetvals(msg) << a1;
+ DBusMessagePtr reply;
+ DBusClientCall<Callback_t>::doCall(msg, reply);
+ }
};
/** 1 return value and 0 or more parameters */
@@ -4257,6 +4286,24 @@ public:
: DBusClientCall<Callback_t>(object, method,
&DBusClientCall1::dbusCallback)
{
}
+
+ using DBusClientCall<Callback_t>::operator ();
+
+ template <class A1>
+ R1 operator () (const A1 &a1)
+ {
+ DBusMessagePtr msg;
+ DBusClientCall<Callback_t>::prepare(msg);
+ AppendRetvals(msg) << a1;
+ DBusErrorCXX error;
+ DBusMessagePtr reply =
dbus_connection_send_with_reply_and_block(DBusClientCall<Callback_t>::m_conn.get(),
msg.get(), -1, &error);
+ if (!reply) {
+ error.throwFailure(DBusClientCall<Callback_t>::m_method);
+ }
+ typename dbus_traits<R1>::host_type r;
+ ExtractArgs(DBusClientCall<Callback_t>::m_conn.get(),
reply.get()) >> Get<R1>(r);
+ return r;
+ }
};
/** 2 return value and 0 or more parameters */
@@ -4291,6 +4338,34 @@ public:
: DBusClientCall<Callback_t>(object, method,
&DBusClientCall2::dbusCallback)
{
}
+
+ using DBusClientCall<Callback_t>::operator ();
+
+ boost::tuple<R1, R2> operator () ()
+ {
+ DBusMessagePtr msg;
+ DBusClientCall<Callback_t>::prepare(msg);
+ DBusMessagePtr reply;
+ DBusClientCall<Callback_t>::doCall(msg, reply);
+ boost::tuple<R1, R2> r;
+ boost::get<0>(r);
+ ExtractArgs(DBusClientCall<Callback_t>::m_conn.get(),
reply.get()) >> Get<R1>(boost::get<0>(r)) >>
Get<R2>(boost::get<1>(r));
+ return r;
+ }
+
+ template <class A1>
+ boost::tuple<R1, R2> operator () (const A1 &a1)
+ {
+ DBusMessagePtr msg;
+ DBusClientCall<Callback_t>::prepare(msg);
+ AppendRetvals(msg) << a1;
+ DBusMessagePtr reply;
+ DBusClientCall<Callback_t>::doCall(msg, reply);
+ boost::tuple<R1, R2> r;
+ ExtractArgs(DBusClientCall<Callback_t>::m_conn.get(),
reply.get()) >> Get<R1>(r[0]) >> Get<R2>(r[1]);
+ return r;
+ }
+
};
/** 3 return value and 0 or more parameters */
--
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.
3:53 a.m.
New subject: [SyncEvolution] C++ D-Bus bindings + blocking calls
2012/2/14 Patrick Ohly <patrick.ohly(a)intel.com>:
On Tue, 2012-02-14 at 16:02 +0100, Patrick Ohly wrote:
> On Mon, 2012-02-13 at 14:50 +0100, Chris Kühl wrote:
> > - Krzesimir implemented the missing functionality in the gio-gdbus
> > wrapper and added code to do blocking calls with minimal extra code
> > required in consuming classes.
>
> In addition to adding a block() method which executes the call, wouldn't
> it be nicer to also overload the call operator such that if there is no
> Callback_t parameter, all results are returned directly (special case
> for a single return value) or as tuple (general case)?
>
> The block() call would be more efficient for large return values (avoids
> one copying), but for small ones the ease of use certainly would be
> worth it.
>
> I've experimented a bit with it and think it can be done.
>
> Is there still a need for the block() variant if there was a "normal"
> blocking call operator()?
Here's a partial implementation. Not all combinations of number of
parameters and return values are currently spelled out. Is there a
better approach and would it be worthwhile?
Well, specifiying 44 methods sounds like overkill to me. I do have
some idea, but I don't know if it will work.
Add another template parameter to DBusClientCall:
template <class T, class R>
class DBusClientCall
{
public:
typedef T Callback_t;
typedef R Return_t;
...
};
Add pure virtual method in DBusClientCall:
virtual Return_t demarshal(DBusMessagePtr &msg, DBusConnectionPtr &conn) = 0;
Then example of blocking operator():
Return_t operator () ()
{
// usual body - message creation, preparation, adding parameters...
return sendAndReturn(msg);
}
where sendAndReturn would be:
Return_t sendAndReturn(GDBusMessagePtr &msg)
{
// do synchronous call and throw an exception on error.
GDBusMessagePtr reply(...);
return demarshal(reply, m_conn);
}
DBusClientCall{0,1,2,3} should implement their own demarshal function
and in constructor they should pass return type as a base class second
template parameter:
DBusClientCall2 (const DBusRemoteObject &object, const std::string &method)
: DBusClientCall<Callback_t, boost::tuple<R1, R2> >(object,
method, &DBusClientCall2::dbusCallback)
{
}
This is an idea, so I have no idea if it will work. :) Especially when
Return_t is void.
diff --git a/src/gdbus/gdbus-cxx-bridge.h
b/src/gdbus/gdbus-cxx-bridge.h
index dc217ca..d56be89 100644
--- a/src/gdbus/gdbus-cxx-bridge.h
+++ b/src/gdbus/gdbus-cxx-bridge.h
@@ -81,6 +81,7 @@ static inline void intrusive_ptr_release(DBusServer
*server) { dbus_server_unref
#include <boost/variant.hpp>
#include <boost/variant/get.hpp>
#include <boost/utility.hpp>
+#include <boost/tuple/tuple.hpp>
/* The connection is the only client-exposed type from the C API. To
* keep changes to a minimum while supporting both dbus
@@ -3913,6 +3914,20 @@ protected:
typedef T Callback_t;
+ /**
+ * Blocking method call: execute call and check for error.
+ * Encoding parameters and decoding return values must be done by
+ * caller.
+ */
+ void doCall(DBusMessagePtr &msg, DBusMessagePtr &reply)
+ {
+ DBusErrorCXX error;
+ reply =
dbus_connection_send_with_reply_and_block(DBusClientCall<Callback_t>::m_conn.get(),
msg.get(), -1, &error);
+ if (!reply) {
+ error.throwFailure(DBusClientCall<Callback_t>::m_method);
+ }
+ }
+
void prepare(DBusMessagePtr &msg)
{
// Constructor steals reference, reset() doesn't!
@@ -4225,6 +4240,20 @@ public:
: DBusClientCall<Callback_t>(object, method,
&DBusClientCall0::dbusCallback)
{
}
+
+ // keep call operator from base class visible
+ using DBusClientCall<Callback_t>::operator ();
+
+ // return once the call is done, throws runtime_error if it fails
+ template <class A1>
+ void operator () (const A1 &a1)
+ {
+ DBusMessagePtr msg;
+ prepare(msg);
+ AppendRetvals(msg) << a1;
+ DBusMessagePtr reply;
+ DBusClientCall<Callback_t>::doCall(msg, reply);
+ }
};
/** 1 return value and 0 or more parameters */
@@ -4257,6 +4286,24 @@ public:
: DBusClientCall<Callback_t>(object, method,
&DBusClientCall1::dbusCallback)
{
}
+
+ using DBusClientCall<Callback_t>::operator ();
+
+ template <class A1>
+ R1 operator () (const A1 &a1)
+ {
+ DBusMessagePtr msg;
+ DBusClientCall<Callback_t>::prepare(msg);
+ AppendRetvals(msg) << a1;
+ DBusErrorCXX error;
+ DBusMessagePtr reply =
dbus_connection_send_with_reply_and_block(DBusClientCall<Callback_t>::m_conn.get(),
msg.get(), -1, &error);
+ if (!reply) {
+ error.throwFailure(DBusClientCall<Callback_t>::m_method);
+ }
+ typename dbus_traits<R1>::host_type r;
+ ExtractArgs(DBusClientCall<Callback_t>::m_conn.get(),
reply.get()) >> Get<R1>(r);
+ return r;
+ }
};
/** 2 return value and 0 or more parameters */
@@ -4291,6 +4338,34 @@ public:
: DBusClientCall<Callback_t>(object, method,
&DBusClientCall2::dbusCallback)
{
}
+
+ using DBusClientCall<Callback_t>::operator ();
+
+ boost::tuple<R1, R2> operator () ()
+ {
+ DBusMessagePtr msg;
+ DBusClientCall<Callback_t>::prepare(msg);
+ DBusMessagePtr reply;
+ DBusClientCall<Callback_t>::doCall(msg, reply);
+ boost::tuple<R1, R2> r;
+ boost::get<0>(r);
+ ExtractArgs(DBusClientCall<Callback_t>::m_conn.get(),
reply.get()) >> Get<R1>(boost::get<0>(r)) >>
Get<R2>(boost::get<1>(r));
+ return r;
+ }
+
+ template <class A1>
+ boost::tuple<R1, R2> operator () (const A1 &a1)
+ {
+ DBusMessagePtr msg;
+ DBusClientCall<Callback_t>::prepare(msg);
+ AppendRetvals(msg) << a1;
+ DBusMessagePtr reply;
+ DBusClientCall<Callback_t>::doCall(msg, reply);
+ boost::tuple<R1, R2> r;
+ ExtractArgs(DBusClientCall<Callback_t>::m_conn.get(),
reply.get()) >> Get<R1>(r[0]) >> Get<R2>(r[1]);
+ return r;
+ }
+
};
/** 3 return value and 0 or more parameters */
--
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.
6:34 a.m.
New subject: [SyncEvolution] C++ D-Bus bindings + blocking calls
On Tue, 2012-02-14 at 17:53 +0100, Krzesimir Nowak wrote:
2012/2/14 Patrick Ohly <patrick.ohly(a)intel.com>:
> Here's a partial implementation. Not all combinations of number of
> parameters and return values are currently spelled out. Is there a
> better approach and would it be worthwhile?
Well, specifiying 44 methods sounds like overkill to me. I do have
some idea, but I don't know if it will work.
Add another template parameter to DBusClientCall:
template <class T, class R>
class DBusClientCall
{
public:
typedef T Callback_t;
typedef R Return_t;
...
};
Add pure virtual method in DBusClientCall:
virtual Return_t demarshal(DBusMessagePtr &msg, DBusConnectionPtr &conn) = 0;
Then example of blocking operator():
Return_t operator () ()
{
// usual body - message creation, preparation, adding parameters...
return sendAndReturn(msg);
}
where sendAndReturn would be:
Return_t sendAndReturn(GDBusMessagePtr &msg)
{
// do synchronous call and throw an exception on error.
GDBusMessagePtr reply(...);
return demarshal(reply, m_conn);
}
We could use an empty tuple to handle a void return type. Same for a
single return value. But the result wouldn't be very natural: the empty
tuple could be ignored by the caller, but the single-value very much
should be the return value, not some tuple containing it.
This is an idea, so I have no idea if it will work. :) Especially
when
Return_t is void.
Exactly :-(
So the question remains, is this useful enough to justify the effort
(whether it is some cut-and-paste or further investigations around
achieving this more elegantly)?
--
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.
7:48 p.m.
New subject: [SyncEvolution] C++ D-Bus bindings + blocking calls (was: Re: DBus server rework: sync, shutdown, and password request)
On Tue, 2012-02-14 at 16:02 +0100, Patrick Ohly wrote:
On Mon, 2012-02-13 at 14:50 +0100, Chris Kühl wrote:
> - Krzesimir implemented the missing functionality in the gio-gdbus
> wrapper and added code to do blocking calls with minimal extra code
> required in consuming classes.
In addition to adding a block() method which executes the call, wouldn't
it be nicer to also overload the call operator such that if there is no
Callback_t parameter, all results are returned directly (special case
for a single return value) or as tuple (general case)?
The block() call would be more efficient for large return values (avoids
one copying), but for small ones the ease of use certainly would be
worth it.
I've experimented a bit with it and think it can be done.
... but there are complications.
When the call operator is called with a boost::function as callback, the
C++ compiler picks the operator implementation which has that type as
last parameter, as intended (more specific alternatives are preferred).
But when the result of boost::bind() is passed to the operator, the
compiler ends up treating it like an arbitrary template class and tries
to serialize it in the blocking call version of the call operator.
To solve this, one would have to add more variants of the call operator
- not nice. I'd prefer to change the C++ binding's API and offer a
blocking call operator and an asynchronous start() call which is
equivalent to the current call operator. Seems cleaner to me.
Is there still a need for the block() variant if there was a
"normal"
blocking call operator()?
I looked again at the concurrent-sync-sessions-r1 branch and found that
the direct calling of the block() methods was already removed again in
favor of some generic callback mechanism which stores the return values
in some bound variables.
I still think that a "normal" method call is much more intuitive. If I
don't hear otherwise, then I'll rewrite the top of the branch to use the
approach outlined above.
--
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.
10:39 p.m.
New subject: [SyncEvolution] C++ D-Bus bindings + blocking calls
2012/2/14 Patrick Ohly <patrick.ohly(a)intel.com>:
On Tue, 2012-02-14 at 17:53 +0100, Krzesimir Nowak wrote:
> 2012/2/14 Patrick Ohly <patrick.ohly(a)intel.com>:
> > Here's a partial implementation. Not all combinations of number of
> > parameters and return values are currently spelled out. Is there a
> > better approach and would it be worthwhile?
>
> Well, specifiying 44 methods sounds like overkill to me. I do have
> some idea, but I don't know if it will work.
>
>
> Add another template parameter to DBusClientCall:
>
> template <class T, class R>
> class DBusClientCall
> {
> public:
> typedef T Callback_t;
> typedef R Return_t;
> ...
> };
>
> Add pure virtual method in DBusClientCall:
> virtual Return_t demarshal(DBusMessagePtr &msg, DBusConnectionPtr &conn) =
0;
>
> Then example of blocking operator():
>
>
> Return_t operator () ()
> {
> // usual body - message creation, preparation, adding parameters...
> return sendAndReturn(msg);
> }
>
> where sendAndReturn would be:
>
> Return_t sendAndReturn(GDBusMessagePtr &msg)
> {
> // do synchronous call and throw an exception on error.
> GDBusMessagePtr reply(...);
>
> return demarshal(reply, m_conn);
> }
We could use an empty tuple to handle a void return type. Same for a
single return value. But the result wouldn't be very natural: the empty
tuple could be ignored by the caller, but the single-value very much
should be the return value, not some tuple containing it.
> This is an idea, so I have no idea if it will work. :) Especially when
> Return_t is void.
Exactly :-(
Got different idea about this yesterday at home - maybe we could just
use traits class for DBusClientCall. Traits struct would describe
callback type and return type. For void it would some empty struct
like struct VoidReturn {};
I am now experimenting with it, so I'll see how it works.
So the question remains, is this useful enough to justify the effort
(whether it is some cut-and-paste or further investigations around
achieving this more elegantly)?
--
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.
12:01 a.m.
New subject: [SyncEvolution] C++ D-Bus bindings + blocking calls
On Wed, 2012-02-15 at 12:39 +0100, Krzesimir Nowak wrote:
2012/2/14 Patrick Ohly <patrick.ohly(a)intel.com>:
> We could use an empty tuple to handle a void return type. Same for a
> single return value. But the result wouldn't be very natural: the empty
> tuple could be ignored by the caller, but the single-value very much
> should be the return value, not some tuple containing it.
>
>> This is an idea, so I have no idea if it will work. :) Especially when
>> Return_t is void.
>
> Exactly :-(
>
Got different idea about this yesterday at home - maybe we could just
use traits class for DBusClientCall. Traits struct would describe
callback type and return type. For void it would some empty struct
like struct VoidReturn {};
That should work. It's a pity that the "void" type is treated
differently than other types.
Anyway, I also went ahead and implemented the brute-force approach of
duplicating different operators this morning. See
for-master/dbus-blocking. We can replace that with a more elegant
implementation later.
--
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.
5:03 a.m.
New subject: [SyncEvolution] C++ D-Bus bindings + blocking calls
2012/2/15 Patrick Ohly <patrick.ohly(a)intel.com>:
On Wed, 2012-02-15 at 12:39 +0100, Krzesimir Nowak wrote:
> 2012/2/14 Patrick Ohly <patrick.ohly(a)intel.com>:
> > We could use an empty tuple to handle a void return type. Same for a
> > single return value. But the result wouldn't be very natural: the empty
> > tuple could be ignored by the caller, but the single-value very much
> > should be the return value, not some tuple containing it.
> >
> >> This is an idea, so I have no idea if it will work. :) Especially when
> >> Return_t is void.
> >
> > Exactly :-(
> >
>
> Got different idea about this yesterday at home - maybe we could just
> use traits class for DBusClientCall. Traits struct would describe
> callback type and return type. For void it would some empty struct
> like struct VoidReturn {};
That should work. It's a pity that the "void" type is treated
differently than other types.
Anyway, I also went ahead and implemented the brute-force approach of
duplicating different operators this morning. See
for-master/dbus-blocking. We can replace that with a more elegant
implementation later.
If you are curious then my implementation is in my branch [1] (the
ada8263c3f20a772928889503b92c272535d8ea8 commit). It should be API
compatible with your for-master/dbus-blocking code.
[1]
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
--
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.
7:51 p.m.
New subject: [SyncEvolution] C++ D-Bus bindings + blocking calls
On Wed, 2012-02-15 at 19:03 +0100, Krzesimir Nowak wrote:
2012/2/15 Patrick Ohly <patrick.ohly(a)intel.com>:
> On Wed, 2012-02-15 at 12:39 +0100, Krzesimir Nowak wrote:
>> 2012/2/14 Patrick Ohly <patrick.ohly(a)intel.com>:
>> > We could use an empty tuple to handle a void return type. Same for a
>> > single return value. But the result wouldn't be very natural: the
empty
>> > tuple could be ignored by the caller, but the single-value very much
>> > should be the return value, not some tuple containing it.
>> >
>> >> This is an idea, so I have no idea if it will work. :) Especially when
>> >> Return_t is void.
>> >
>> > Exactly :-(
>> >
>>
>> Got different idea about this yesterday at home - maybe we could just
>> use traits class for DBusClientCall. Traits struct would describe
>> callback type and return type. For void it would some empty struct
>> like struct VoidReturn {};
>
> That should work. It's a pity that the "void" type is treated
> differently than other types.
>
> Anyway, I also went ahead and implemented the brute-force approach of
> duplicating different operators this morning. See
> for-master/dbus-blocking. We can replace that with a more elegant
> implementation later.
If you are curious then my implementation is in my branch [1] (the
ada8263c3f20a772928889503b92c272535d8ea8 commit). It should be API
compatible with your for-master/dbus-blocking code.
Thanks, that looks nicer. I'll try to use it. But of course you are
cheating with that fake void return code ;-)
My goal is to merge all of the D-Bus binding improvements into master as
soon as possible. I've already rewritten some parts of the syncevolution
binary's D-Bus calls to be blocking and it turned out much nicer than
the older code. The PBAP backend would also benefit from proper blocking
calls.
Some quick comments about that branch: please write a bit more verbose
commit messages. Just a single line is often not enough to understand
what the commit is about. Looking at the code helps, but takes a lot
more time. If the code is wrong, that kind of analysis doesn't help
either.
From the HACKING document (not all of that applies to your commits;
I'm
repeating it here in its entirety because some of the guidelines were
not followed in the D-Bus server rewrite):
Here are some guidelines for well-formed commits:
- Avoid unnecessary white space changes in the source code.
- Don't mix unrelated changes in one single commit. Every
single commit should leave the code in a functional state
(compiles, unit tests pass, ...).
- Document new functions and structures with Doxygen comments.
These comments should really add information, not just
repeat the name of the entity. At least paraphrase the names.
- Each commit message should follow the format
<item>: <summary> [(BMC #1234)]
<blank line>
<body>
- <item> here refers to the module, class or functionality modified
in the commit, for example "autotools" for the build scripts,
"EvolutionContactSource" for the EDS contact backend, etc.
- <summary> should be consise enough to describe the patch on
its own.
- The optional bugs.meego.com (BMC) number in round brackets
refers to the issue addressed by the commit. Most changes,
in particular bug fixes, should have such an issue filed
for tracking purposes.
- The body (hard-wrapped, multiple paragraphs for long text)
should include the following pieces of information,
if applicable:
- problem statement (*why* is the change relevant?)
- summary of the solution (*how* is the problem solved?)
- what other solutions were considered and rejected (*context*
for the solution)
- known gaps (in particularly useful during code review,
which will start with the oldest, possibly incomplete patch first)
$ git log -p -n 1 2be5a73838a39d36fabf728cff3f16611476eb09
commit 2be5a73838a39d36fabf728cff3f16611476eb09
Author: Krzesimir Nowak <krnowak(a)openismus.com>
Date: Wed Feb 8 13:31:19 2012 +0100
Put some string constants into variables.
...
diff --git a/src/dbus/server/session-common.h
b/src/dbus/server/session-common.h
index a414047..9f45401 100644
--- a/src/dbus/server/session-common.h
+++ b/src/dbus/server/session-common.h
@@ -78,7 +78,15 @@ namespace SessionCommon
* never runs operations directly after starting, that shouldn't
* be a problem.
*/
- static const int SHUTDOWN_QUIESCENCE_SECONDS = 10;
+ const int SHUTDOWN_QUIESCENCE_SECONDS = 10;
+
+ const char * const SERVICE_NAME = "org.syncevolution";
+ const char * const CONNECTION_PATH =
"/org/syncevolution/Connection";
+ const char * const CONNECTION_IFACE =
"org.syncevolution.Connection";
+ const char * const SESSION_PATH = "/org/syncevolution/Session";
+ const char * const SESSION_IFACE = "org.syncevolution.Session";
+ const char * const SERVER_PATH = "/org/syncevolution/Server";
+ const char * const SERVER_IFACE = "org.syncevolution.Server";
}
I really wish that this was possible, but unfortunately only C++11
allows that. clang 3.0-5 compiles it with a warning:
/tmp/test.h:3:28: warning: in-class initialization of non-static data member accepted as a
C++11 extension
[-Wc++11-extensions]
const char * const FOO = "abc";
g++ 4.6.2 fails with an error:
$ g++ /tmp/test.h -c -o /tmp/test.o
/tmp/test.h:3:30: error: ISO C++ forbids initialization of member ‘FOO’ [-fpermissive]
/tmp/test.h:3:30: error: making ‘FOO’ static [-fpermissive]
/tmp/test.h:3:30: error: invalid in-class initialization of static data member of
non-integral type ‘const char* const’
I don't have a good solution for string constants while sticking to ISO
C++, that's why they are not used much in the SyncEvolution code base.
--
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.
8:03 p.m.
New subject: [SyncEvolution] C++ D-Bus bindings + blocking calls
2012/2/16 Patrick Ohly <patrick.ohly(a)intel.com>:
On Wed, 2012-02-15 at 19:03 +0100, Krzesimir Nowak wrote:
> 2012/2/15 Patrick Ohly <patrick.ohly(a)intel.com>:
> > On Wed, 2012-02-15 at 12:39 +0100, Krzesimir Nowak wrote:
> >> 2012/2/14 Patrick Ohly <patrick.ohly(a)intel.com>:
> >> > We could use an empty tuple to handle a void return type. Same for a
> >> > single return value. But the result wouldn't be very natural: the
empty
> >> > tuple could be ignored by the caller, but the single-value very much
> >> > should be the return value, not some tuple containing it.
> >> >
> >> >> This is an idea, so I have no idea if it will work. :) Especially
when
> >> >> Return_t is void.
> >> >
> >> > Exactly :-(
> >> >
> >>
> >> Got different idea about this yesterday at home - maybe we could just
> >> use traits class for DBusClientCall. Traits struct would describe
> >> callback type and return type. For void it would some empty struct
> >> like struct VoidReturn {};
> >
> > That should work. It's a pity that the "void" type is treated
> > differently than other types.
> >
> > Anyway, I also went ahead and implemented the brute-force approach of
> > duplicating different operators this morning. See
> > for-master/dbus-blocking. We can replace that with a more elegant
> > implementation later.
>
> If you are curious then my implementation is in my branch [1] (the
> ada8263c3f20a772928889503b92c272535d8ea8 commit). It should be API
> compatible with your for-master/dbus-blocking code.
Thanks, that looks nicer. I'll try to use it. But of course you are
cheating with that fake void return code ;-)
My goal is to merge all of the D-Bus binding improvements into master as
soon as possible. I've already rewritten some parts of the syncevolution
binary's D-Bus calls to be blocking and it turned out much nicer than
the older code. The PBAP backend would also benefit from proper blocking
calls.
> [1]
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
Some quick comments about that branch: please write a bit more verbose
commit messages. Just a single line is often not enough to understand
what the commit is about. Looking at the code helps, but takes a lot
more time. If the code is wrong, that kind of analysis doesn't help
either.
From the HACKING document (not all of that applies to your commits; I'm
repeating it here in its entirety because some of the guidelines were
not followed in the D-Bus server rewrite):
Here are some guidelines for well-formed commits:
- Avoid unnecessary white space changes in the source code.
- Don't mix unrelated changes in one single commit. Every
single commit should leave the code in a functional state
(compiles, unit tests pass, ...).
- Document new functions and structures with Doxygen comments.
These comments should really add information, not just
repeat the name of the entity. At least paraphrase the names.
- Each commit message should follow the format
<item>: <summary> [(BMC #1234)]
<blank line>
<body>
- <item> here refers to the module, class or functionality modified
in the commit, for example "autotools" for the build scripts,
"EvolutionContactSource" for the EDS contact backend, etc.
- <summary> should be consise enough to describe the patch on
its own.
- The optional bugs.meego.com (BMC) number in round brackets
refers to the issue addressed by the commit. Most changes,
in particular bug fixes, should have such an issue filed
for tracking purposes.
- The body (hard-wrapped, multiple paragraphs for long text)
should include the following pieces of information,
if applicable:
- problem statement (*why* is the change relevant?)
- summary of the solution (*how* is the problem solved?)
- what other solutions were considered and rejected (*context*
for the solution)
- known gaps (in particularly useful during code review,
which will start with the oldest, possibly incomplete patch first)
Ok, I will try to follow the guidelines in my commits. Also, my cssr1
branch is based on Chris' concurrent-sync-sessions-r1 branch. I should
have put this DBus blocking calls work into separate one based on
master I guess.
$ git log -p -n 1 2be5a73838a39d36fabf728cff3f16611476eb09
commit 2be5a73838a39d36fabf728cff3f16611476eb09
Author: Krzesimir Nowak <krnowak(a)openismus.com>
Date: Wed Feb 8 13:31:19 2012 +0100
Put some string constants into variables.
...
diff --git a/src/dbus/server/session-common.h
b/src/dbus/server/session-common.h
index a414047..9f45401 100644
--- a/src/dbus/server/session-common.h
+++ b/src/dbus/server/session-common.h
@@ -78,7 +78,15 @@ namespace SessionCommon
* never runs operations directly after starting, that shouldn't
* be a problem.
*/
- static const int SHUTDOWN_QUIESCENCE_SECONDS = 10;
+ const int SHUTDOWN_QUIESCENCE_SECONDS = 10;
+
+ const char * const SERVICE_NAME = "org.syncevolution";
+ const char * const CONNECTION_PATH =
"/org/syncevolution/Connection";
+ const char * const CONNECTION_IFACE =
"org.syncevolution.Connection";
+ const char * const SESSION_PATH = "/org/syncevolution/Session";
+ const char * const SESSION_IFACE = "org.syncevolution.Session";
+ const char * const SERVER_PATH = "/org/syncevolution/Server";
+ const char * const SERVER_IFACE = "org.syncevolution.Server";
}
I really wish that this was possible, but unfortunately only C++11
allows that. clang 3.0-5 compiles it with a warning:
/tmp/test.h:3:28: warning: in-class initialization of non-static data member accepted as
a C++11 extension
[-Wc++11-extensions]
const char * const FOO = "abc";
g++ 4.6.2 fails with an error:
$ g++ /tmp/test.h -c -o /tmp/test.o
/tmp/test.h:3:30: error: ISO C++ forbids initialization of member ‘FOO’ [-fpermissive]
/tmp/test.h:3:30: error: making ‘FOO’ static [-fpermissive]
/tmp/test.h:3:30: error: invalid in-class initialization of static data member of
non-integral type ‘const char* const’
I don't have a good solution for string constants while sticking to ISO
C++, that's why they are not used much in the SyncEvolution code base.
This commit should be applied against concurrent-sync-sessions-r1 I
believe - there we have changed SessionCommon from class into
namespace [1], so all my string constants work.
[1]
https://meego.gitorious.org/meego-middleware/syncevolution/commit/7d861f5...
--
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.
8:50 p.m.
New subject: [SyncEvolution] C++ D-Bus bindings + blocking calls
On Thu, 2012-02-16 at 10:03 +0100, Krzesimir Nowak wrote:
2012/2/16 Patrick Ohly <patrick.ohly(a)intel.com>:
> My goal is to merge all of the D-Bus binding improvements into master as
> soon as possible. I've already rewritten some parts of the syncevolution
> binary's D-Bus calls to be blocking and it turned out much nicer than
> the older code. The PBAP backend would also benefit from proper blocking
> calls.
>
>> [1]
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
>
> Some quick comments about that branch: please write a bit more verbose
> commit messages. Just a single line is often not enough to understand
> what the commit is about. Looking at the code helps, but takes a lot
> more time. If the code is wrong, that kind of analysis doesn't help
> either.
>
[...]
Ok, I will try to follow the guidelines in my commits.
Thanks, that'll make my life easier.
Also, my cssr1
branch is based on Chris' concurrent-sync-sessions-r1 branch. I should
have put this DBus blocking calls work into separate one based on
master I guess.
I'm working on that now. Basically I am cherry-picking everything
related to src/gdbus or src/gdbusxx from your cssr1 and my
for-master/dbus-blocking. I'm not squashing anything yet, so hopefully
rebasing cssr1 onto it will work automatically.
Shall we preserve the block() functionality? It might be useful in some
cases (avoid extra copying), but unless it really gets used it's just
dead code. Perhaps preserve it on a branch instead or merging into
master?
> I don't have a good solution for string constants while
sticking to ISO
> C++, that's why they are not used much in the SyncEvolution code base.
This commit should be applied against concurrent-sync-sessions-r1 I
believe - there we have changed SessionCommon from class into
namespace [1], so all my string constants work.
Duh, my bad. You are right of course, in a namespace it is fine. I
missed that in your commit, and then my own test only used them in a
class.
--
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.
9 p.m.
New subject: [SyncEvolution] C++ D-Bus bindings + blocking calls
2012/2/16 Patrick Ohly <patrick.ohly(a)intel.com>:
On Thu, 2012-02-16 at 10:03 +0100, Krzesimir Nowak wrote:
> 2012/2/16 Patrick Ohly <patrick.ohly(a)intel.com>:
> > My goal is to merge all of the D-Bus binding improvements into master as
> > soon as possible. I've already rewritten some parts of the syncevolution
> > binary's D-Bus calls to be blocking and it turned out much nicer than
> > the older code. The PBAP backend would also benefit from proper blocking
> > calls.
> >
> >> [1]
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
> >
> > Some quick comments about that branch: please write a bit more verbose
> > commit messages. Just a single line is often not enough to understand
> > what the commit is about. Looking at the code helps, but takes a lot
> > more time. If the code is wrong, that kind of analysis doesn't help
> > either.
> >
[...]
> Ok, I will try to follow the guidelines in my commits.
Thanks, that'll make my life easier.
> Also, my cssr1
> branch is based on Chris' concurrent-sync-sessions-r1 branch. I should
> have put this DBus blocking calls work into separate one based on
> master I guess.
I'm working on that now. Basically I am cherry-picking everything
related to src/gdbus or src/gdbusxx from your cssr1 and my
for-master/dbus-blocking. I'm not squashing anything yet, so hopefully
rebasing cssr1 onto it will work automatically.
Shall we preserve the block() functionality? It might be useful in some
cases (avoid extra copying), but unless it really gets used it's just
dead code. Perhaps preserve it on a branch instead or merging into
master?
block() functionality is used in resources in
concurrent-sync-sessions-r1 branch. Same with genericCb() and
bindGeneric() in DBusClientCall{0,1,2,3}. I leave that decision to you
- maybe you will find them useful elsewhere - especially for
synchronous calls when all you want to do with return values is to
assign them to some class members. Like in the following code:
std::string error;
call.block(in1, in2, call.bindGeneric(&m_out1, m_out2, &str_error));
if (!error.empty()) { throw Foo; }
> > I don't have a good solution for string constants while
sticking to ISO
> > C++, that's why they are not used much in the SyncEvolution code base.
>
> This commit should be applied against concurrent-sync-sessions-r1 I
> believe - there we have changed SessionCommon from class into
> namespace [1], so all my string constants work.
Duh, my bad. You are right of course, in a namespace it is fine. I
missed that in your commit, and then my own test only used them in a
class.
--
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.
9:41 p.m.
New subject: [SyncEvolution] C++ D-Bus bindings + blocking calls
2012/2/16 Patrick Ohly <patrick.ohly(a)intel.com>:
On Thu, 2012-02-16 at 10:03 +0100, Krzesimir Nowak wrote:
> 2012/2/16 Patrick Ohly <patrick.ohly(a)intel.com>:
> > My goal is to merge all of the D-Bus binding improvements into master as
> > soon as possible. I've already rewritten some parts of the syncevolution
> > binary's D-Bus calls to be blocking and it turned out much nicer than
> > the older code. The PBAP backend would also benefit from proper blocking
> > calls.
> >
> >> [1]
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
> >
> > Some quick comments about that branch: please write a bit more verbose
> > commit messages. Just a single line is often not enough to understand
> > what the commit is about. Looking at the code helps, but takes a lot
> > more time. If the code is wrong, that kind of analysis doesn't help
> > either.
> >
[...]
> Ok, I will try to follow the guidelines in my commits.
Thanks, that'll make my life easier.
> Also, my cssr1
> branch is based on Chris' concurrent-sync-sessions-r1 branch. I should
> have put this DBus blocking calls work into separate one based on
> master I guess.
I'm working on that now. Basically I am cherry-picking everything
related to src/gdbus or src/gdbusxx from your cssr1 and my
for-master/dbus-blocking. I'm not squashing anything yet, so hopefully
rebasing cssr1 onto it will work automatically.
My rebase is here [1]. Maybe it will be useful for you.
[1]
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
Shall we preserve the block() functionality? It might be useful in
some
cases (avoid extra copying), but unless it really gets used it's just
dead code. Perhaps preserve it on a branch instead or merging into
master?
> > I don't have a good solution for string constants while sticking to ISO
> > C++, that's why they are not used much in the SyncEvolution code base.
>
> This commit should be applied against concurrent-sync-sessions-r1 I
> believe - there we have changed SessionCommon from class into
> namespace [1], so all my string constants work.
Duh, my bad. You are right of course, in a namespace it is fine. I
missed that in your commit, and then my own test only used them in a
class.
--
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.
9:57 p.m.
New subject: [SyncEvolution] C++ D-Bus bindings + blocking calls
On Thu, 2012-02-16 at 11:41 +0100, Krzesimir Nowak wrote:
My rebase is here [1]. Maybe it will be useful for you.
Thanks, but I was already done. I found a few issues in the code
(compiler warning, double free), but it should be working now. See
dbus-blocking branch.
I also rebased cssr1 and pushed that. It compiles, but I haven't tried
to run anything. Please use that for all future development.
I'll look at removing the block() functionality next.
--
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.
12:05 a.m.
New subject: [SyncEvolution] C++ D-Bus bindings + blocking calls
On Thu, 2012-02-16 at 10:50 +0100, Patrick Ohly wrote:
On Thu, 2012-02-16 at 10:03 +0100, Krzesimir Nowak wrote:
> 2012/2/16 Patrick Ohly <patrick.ohly(a)intel.com>:
> > My goal is to merge all of the D-Bus binding improvements into master as
> > soon as possible. I've already rewritten some parts of the syncevolution
> > binary's D-Bus calls to be blocking and it turned out much nicer than
> > the older code. The PBAP backend would also benefit from proper blocking
> > calls.
> >
> >> [1]
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
> >
> > Some quick comments about that branch: please write a bit more verbose
> > commit messages. Just a single line is often not enough to understand
> > what the commit is about. Looking at the code helps, but takes a lot
> > more time. If the code is wrong, that kind of analysis doesn't help
> > either.
> >
[...]
> Ok, I will try to follow the guidelines in my commits.
Thanks, that'll make my life easier.
For example, why did you change this:
commit c7a6f28162b5af2d42b41393dc12a16583482312
Author: Krzesimir Nowak <krnowak(a)openismus.com>
Date: Mon Feb 6 16:48:47 2012 +0100
GDBus GIO: don't expose GIO GDBus API in wrapper
In this case - GDBusConnection. Replaced it with DBusConnectionPtr.
diff --git a/src/gdbusxx/gdbus-cxx-bridge.h
b/src/gdbusxx/gdbus-cxx-bridge.h
index 4d60da0..2464b0b 100644
--- a/src/gdbusxx/gdbus-cxx-bridge.h
+++ b/src/gdbusxx/gdbus-cxx-bridge.h
@@ -445,7 +445,7 @@ class DBusObject
}
}
- GDBusConnection *getConnection() const { return m_conn.get(); }
+ DBusConnectionPtr getConnection() const { return m_conn; }
....
Regardless whether we return GDBusConnection or a DBusConnectionPtr, the
fact that there is a GDBusConnection is visible to the user of the API.
On the practical side, now with DBusConnectionPtr, a lot of code has to
make explicit get() calls, which I consider a drawback of that approach.
I stumbled over this because the patch itself is also incomplete. There
are other places where getConnection() still returns a plain pointer.
That in fact breaks compilation of the cssr1 branch in combination with
GDBus libdbus - see my latest push to that branch.
I'm currently leaning towards simply reverting the commit above. Before
I do that I'd like to understand the motivation for it better.
--
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.
12:15 a.m.
New subject: [SyncEvolution] C++ D-Bus bindings + blocking calls
2012/2/16 Patrick Ohly <patrick.ohly(a)intel.com>:
On Thu, 2012-02-16 at 10:50 +0100, Patrick Ohly wrote:
> On Thu, 2012-02-16 at 10:03 +0100, Krzesimir Nowak wrote:
> > 2012/2/16 Patrick Ohly <patrick.ohly(a)intel.com>:
> > > My goal is to merge all of the D-Bus binding improvements into master as
> > > soon as possible. I've already rewritten some parts of the
syncevolution
> > > binary's D-Bus calls to be blocking and it turned out much nicer than
> > > the older code. The PBAP backend would also benefit from proper blocking
> > > calls.
> > >
> > >> [1]
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
> > >
> > > Some quick comments about that branch: please write a bit more verbose
> > > commit messages. Just a single line is often not enough to understand
> > > what the commit is about. Looking at the code helps, but takes a lot
> > > more time. If the code is wrong, that kind of analysis doesn't help
> > > either.
> > >
> [...]
> > Ok, I will try to follow the guidelines in my commits.
>
> Thanks, that'll make my life easier.
For example, why did you change this:
commit c7a6f28162b5af2d42b41393dc12a16583482312
Author: Krzesimir Nowak <krnowak(a)openismus.com>
Date: Mon Feb 6 16:48:47 2012 +0100
GDBus GIO: don't expose GIO GDBus API in wrapper
In this case - GDBusConnection. Replaced it with DBusConnectionPtr.
diff --git a/src/gdbusxx/gdbus-cxx-bridge.h
b/src/gdbusxx/gdbus-cxx-bridge.h
index 4d60da0..2464b0b 100644
--- a/src/gdbusxx/gdbus-cxx-bridge.h
+++ b/src/gdbusxx/gdbus-cxx-bridge.h
@@ -445,7 +445,7 @@ class DBusObject
}
}
- GDBusConnection *getConnection() const { return m_conn.get(); }
+ DBusConnectionPtr getConnection() const { return m_conn; }
....
Regardless whether we return GDBusConnection or a DBusConnectionPtr, the
fact that there is a GDBusConnection is visible to the user of the API.
On the practical side, now with DBusConnectionPtr, a lot of code has to
make explicit get() calls, which I consider a drawback of that approach.
I stumbled over this because the patch itself is also incomplete. There
are other places where getConnection() still returns a plain pointer.
That in fact breaks compilation of the cssr1 branch in combination with
GDBus libdbus - see my latest push to that branch.
I'm currently leaning towards simply reverting the commit above. Before
I do that I'd like to understand the motivation for it better.
--
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.
If I remember correctly then we have dbus_get_connection_fd() function
that gets a connection pointer. In bluez wrapper connection pointer is
marked as DBusConnection and in GIO GDBus wrapper it is
GDBusConnection. But C++ intrusive pointer for them is
DBusConnectionPtr, so I decided then to change getConnection() methods
to return C++ pointer to have single name. I suppose I needed that
when I was updating GIO GDBus wrapper to bluez wrapper state. I guess
we need neither dbus_get_connection_fd() nor getConnection() returning
C++ pointer, so if I were you then I would just get rid of this
commit. Sorry for being vague, but my memory on it already blurred. :)
12:18 a.m.
New subject: [SyncEvolution] C++ D-Bus bindings + blocking calls
On Thu, 2012-02-16 at 14:15 +0100, Krzesimir Nowak wrote:
If I remember correctly then we have dbus_get_connection_fd()
function
that gets a connection pointer. In bluez wrapper connection pointer is
marked as DBusConnection and in GIO GDBus wrapper it is
GDBusConnection. But C++ intrusive pointer for them is
DBusConnectionPtr, so I decided then to change getConnection() methods
to return C++ pointer to have single name. I suppose I needed that
when I was updating GIO GDBus wrapper to bluez wrapper state. I guess
we need neither dbus_get_connection_fd() nor getConnection() returning
C++ pointer, so if I were you then I would just get rid of this
commit.
Right, I'll do. I think dbus_get_connection_fd() also became obsolete.
Sorry for being vague, but my memory on it already blurred. :)
I know exactly what you mean ;-) That's why I write detailed commit
messages, because I know that I cannot rely on my memory.
--
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.
12:29 a.m.
On Mon, 2012-02-13 at 14:50 +0100, Chris Kühl wrote:
So , I've pushed so changes taking us in this direction. Here is
the
basic run down of what these changes include.
- We've now got a priority queue of weak pointers implemented with a
std::set container that is initialized with a Compare class for
inserting new Resources based on their priority.
- There is a method (canRunConcurrently()) in Resource which is
checked but currently always returns false, thus forcing only single
session to be run.
- Once the Resource is made active it's placed the list of active
resources, a list of shared pointers to Resources.
- Implemented some of the outstanding issues with Connection (missing
communication between server and helper processes)
- Allow server process to set helper session's active state.
- Krzesimir implemented the missing functionality in the gio-gdbus
wrapper and added code to do blocking calls with minimal extra code
required in consuming classes.
Note that in the above I always refer to Resources instead of Session
or Connection. Previously the Connection would create a session and
place it in the working queue but not activate the public dbus
interface. Because the Connection and the Session it spawns needs to
be in the same process. Thus the Connection is also queued but has a
higher priority than the other resources.
The above mentioned changes compile and are being tested now. I'll get
back to you with the results of that.
How is that going?
I'm now looking more seriously at the code myself. One thing which
caught my attention is how blocking D-Bus calls are used in
syncevo-dbus-server.
For example, ConnectionResource::process(): it passes the incoming data
on to the helper process with a blocking call. Isn't that a big no-no?
If the helper is busy, won't that D-Bus call completely block all
processing of any other events in the server for considerable periods of
time? Worse, if syncevo-dbus-server is blocked in a call to the helper
and the helper needs something from syncevo-dbus-server (like a
password), then we have a deadlock.
Even if other events do get processed while in a blocking D-Bus call,
the result still won't be very pretty: basically we end up with a
convoluted mixture of recursive calls, instead of a clean "process
events, do something non-blocking, return to event loop" style.
--
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.
12:41 a.m.
On Thu, 2012-02-16 at 14:29 +0100, Patrick Ohly wrote:
On Mon, 2012-02-13 at 14:50 +0100, Chris Kühl wrote:
> The above mentioned changes compile and are being tested now. I'll get
> back to you with the results of that.
How is that going?
I'm now looking more seriously at the code myself. One thing which
caught my attention is how blocking D-Bus calls are used in
syncevo-dbus-server.
For example, ConnectionResource::process(): it passes the incoming data
on to the helper process with a blocking call. Isn't that a big no-no?
... and error handling seems to be missing entirely. If the helper
fails, the class just ignores the error string.
This is going to be a bit better with the new blocking call operator,
because then errors encountered during the execution of the D-Bus call
or returned by the peer will be thrown as runtime exceptions in the
context of the ConnectionResource::process(), leading to a failure of
that call itself.
Speaking of errors returned by the peer:
Return_t sendAndReturn(DBusMessagePtr &msg)
{
DBusErrorCXX error;
// Constructor steals reference, reset() doesn't!
// Therefore use constructor+copy instead of reset().
DBusMessagePtr reply =
DBusMessagePtr(dbus_connection_send_with_reply_and_block(m_conn.get(), msg.get(), -1,
&error));
if (!reply) {
error.throwFailure(m_method);
}
return CallTraits::demarshal(reply, m_conn);
}
static Return_t demarshal(DBusMessagePtr &reply, const DBusConnectionPtr &conn)
{
typename dbus_traits<R1>::host_type r;
ExtractArgs(conn.get(), reply.get()) >> Get<R1>(r);
return r;
}
"reply" may be an error message. That must be checked instead of trying
to extract return values that won't be there.
--
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.
1:18 a.m.
On Thu, 2012-02-16 at 14:41 +0100, Patrick Ohly wrote:
Speaking of errors returned by the peer:
Return_t sendAndReturn(DBusMessagePtr &msg)
{
DBusErrorCXX error;
// Constructor steals reference, reset() doesn't!
// Therefore use constructor+copy instead of reset().
DBusMessagePtr reply =
DBusMessagePtr(dbus_connection_send_with_reply_and_block(m_conn.get(), msg.get(), -1,
&error));
if (!reply) {
error.throwFailure(m_method);
}
return CallTraits::demarshal(reply, m_conn);
}
[...]
"reply" may be an error message. That must be checked
instead of trying
to extract return values that won't be there.
Actually, it is okay for dbus_connection_send_with_reply_and_block(),
which already checks for an error reply.
It's g_dbus_connection_send_message_with_reply_sync() which doesn't do
that for us. Fixed:
Author: Patrick Ohly <patrick.ohly(a)intel.com> 2012-02-16 15:15:30
Committer: Patrick Ohly <patrick.ohly(a)intel.com> 2012-02-16 15:15:30
Parent: 99e9002e1a7d9804490a9cc16f168bebc145402d (GDBus GIO+libdbus:
revert the getConnection() patch)
Branch: cssr1
Follows: syncevolution-1-2-2
Precedes:
GDBus GIO: added missing error handling in blocking call operator
g_dbus_connection_send_message_with_reply_sync(), in contrast to
dbus_connection_send_with_reply_and_block(), doesn't convert a D-Bus
error reply message into a local error. This has to be done by
our own code before trying to extract the return values.
I've pushed it to the cssr1 branch.
--
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.
1:32 a.m.
On Thu, Feb 16, 2012 at 2:29 PM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
On Mon, 2012-02-13 at 14:50 +0100, Chris Kühl wrote:
> So , I've pushed so changes taking us in this direction. Here is the
> basic run down of what these changes include.
>
> - We've now got a priority queue of weak pointers implemented with a
> std::set container that is initialized with a Compare class for
> inserting new Resources based on their priority.
> - There is a method (canRunConcurrently()) in Resource which is
> checked but currently always returns false, thus forcing only single
> session to be run.
> - Once the Resource is made active it's placed the list of active
> resources, a list of shared pointers to Resources.
> - Implemented some of the outstanding issues with Connection (missing
> communication between server and helper processes)
> - Allow server process to set helper session's active state.
> - Krzesimir implemented the missing functionality in the gio-gdbus
> wrapper and added code to do blocking calls with minimal extra code
> required in consuming classes.
>
> Note that in the above I always refer to Resources instead of Session
> or Connection. Previously the Connection would create a session and
> place it in the working queue but not activate the public dbus
> interface. Because the Connection and the Session it spawns needs to
> be in the same process. Thus the Connection is also queued but has a
> higher priority than the other resources.
>
> The above mentioned changes compile and are being tested now. I'll get
> back to you with the results of that.
How is that going?
I'm now looking more seriously at the code myself.
I wouldn't take too close a look at it just yet, I'm still working on
getting some fundamental pieces working, like the main loop dance in
the helper.
One thing which
caught my attention is how blocking D-Bus calls are used in
syncevo-dbus-server.
For example, ConnectionResource::process(): it passes the incoming data
on to the helper process with a blocking call. Isn't that a big no-no?
Yes, in this instance the client is expecting the the Reply signal so
we can definitely do without this blocking call. This is actually how
I wish the entire DBus interface worked; make a request and wait for a
signal that delivers the requested data.
However, blocking calls are necessary in the cases where the client
expects a return value when complete or an error message on failure.
If the helper is busy, won't that D-Bus call completely block
all
processing of any other events in the server for considerable periods of
time? Worse, if syncevo-dbus-server is blocked in a call to the helper
and the helper needs something from syncevo-dbus-server (like a
password), then we have a deadlock.
True, this a problem and would require breaking the DBus interface to
work. The only way I know how to get around this is the make the
public DBus interface asynchronous.
Even if other events do get processed while in a blocking D-Bus call,
the result still won't be very pretty: basically we end up with a
convoluted mixture of recursive calls, instead of a clean "process
events, do something non-blocking, return to event loop" style.
I'll be happy to rework the public DBus interface to get this to work
like this. This, of course, will require that all the clients learn
this new interface and, maybe more painfully, the test suite will need
to be extensively reworked. But it would definitely make the
dbus-server cleaner and some things possible at all.
Cheers,
Chris
2:19 a.m.
On Thu, 2012-02-16 at 15:32 +0100, Chris Kühl wrote:
> One thing which
> caught my attention is how blocking D-Bus calls are used in
> syncevo-dbus-server.
>
> For example, ConnectionResource::process(): it passes the incoming data
> on to the helper process with a blocking call. Isn't that a big no-no?
>
Yes, in this instance the client is expecting the the Reply signal so
we can definitely do without this blocking call. This is actually how
I wish the entire DBus interface worked; make a request and wait for a
signal that delivers the requested data.
But that's already how D-Bus works: send a method call method, get a
reply message back. There's no need to define the D-Bus interface as
"method call without a reply + signal".
> If the helper is busy, won't that D-Bus call completely
block all
> processing of any other events in the server for considerable periods of
> time? Worse, if syncevo-dbus-server is blocked in a call to the helper
> and the helper needs something from syncevo-dbus-server (like a
> password), then we have a deadlock.
True, this a problem and would require breaking the DBus interface to
work. The only way I know how to get around this is the make the
public DBus interface asynchronous.
No, just make the implementation of the D-Bus interface asynchronous.
The C++ bindings support that:
* Asynchronous methods are possible by declaring one parameter as a
* Result pointer and later calling the virtual function provided by
* it. Parameter passing of results is less flexible than that of
* method parameters: the later allows both std::string as well as
* const std::string &, for results only the const reference is
* supported. The Result instance is passed as pointer and then owned
* by the called method.
Have a look at Result1 in src/gdbusxx/test/example.cpp.
The flow of messages then becomes:
1. syncevo-dbus-server receives method call, stores Result pointer
2. syncevo-dbus-server sends method call to helper
3. helper receives method call, replies (asynchronously or
synchronously)
4. syncevo-dbus-server receives reply
5. syncevo-dbus-server relays reply to original caller
I'll see whether I can cook up a patch.
--
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.
2:47 a.m.
On Thu, Feb 16, 2012 at 4:19 PM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
On Thu, 2012-02-16 at 15:32 +0100, Chris Kühl wrote:
> > One thing which
> > caught my attention is how blocking D-Bus calls are used in
> > syncevo-dbus-server.
> >
> > For example, ConnectionResource::process(): it passes the incoming data
> > on to the helper process with a blocking call. Isn't that a big no-no?
> >
>
> Yes, in this instance the client is expecting the the Reply signal so
> we can definitely do without this blocking call. This is actually how
> I wish the entire DBus interface worked; make a request and wait for a
> signal that delivers the requested data.
But that's already how D-Bus works: send a method call method, get a
reply message back. There's no need to define the D-Bus interface as
"method call without a reply + signal".
> > If the helper is busy, won't that D-Bus call completely block all
> > processing of any other events in the server for considerable periods of
> > time? Worse, if syncevo-dbus-server is blocked in a call to the helper
> > and the helper needs something from syncevo-dbus-server (like a
> > password), then we have a deadlock.
>
> True, this a problem and would require breaking the DBus interface to
> work. The only way I know how to get around this is the make the
> public DBus interface asynchronous.
No, just make the implementation of the D-Bus interface asynchronous.
The C++ bindings support that:
* Asynchronous methods are possible by declaring one parameter as a
* Result pointer and later calling the virtual function provided by
* it. Parameter passing of results is less flexible than that of
* method parameters: the later allows both std::string as well as
* const std::string &, for results only the const reference is
* supported. The Result instance is passed as pointer and then owned
* by the called method.
Have a look at Result1 in src/gdbusxx/test/example.cpp.
Ah, that's what I needed. I didn't recall that this was possible
without modifying the DBus wrapper considerably. Looks similar to the
GDbusMethodInvocation functionality.
The flow of messages then becomes:
1. syncevo-dbus-server receives method call, stores Result pointer
2. syncevo-dbus-server sends method call to helper
3. helper receives method call, replies (asynchronously or
synchronously)
4. syncevo-dbus-server receives reply
5. syncevo-dbus-server relays reply to original caller
I'll see whether I can cook up a patch.
No need. I'm now looking at how you use it in LocalTransportAgent.cpp.
That should be enough to get started implementing this.
Cheers,
Chris
4:15 a.m.
On Thu, 2012-02-16 at 16:47 +0100, Chris Kühl wrote:
On Thu, Feb 16, 2012 at 4:19 PM, Patrick Ohly
<patrick.ohly(a)intel.com> wrote:
> The flow of messages then becomes:
> 1. syncevo-dbus-server receives method call, stores Result pointer
> 2. syncevo-dbus-server sends method call to helper
> 3. helper receives method call, replies (asynchronously or
> synchronously)
> 4. syncevo-dbus-server receives reply
> 5. syncevo-dbus-server relays reply to original caller
>
> I'll see whether I can cook up a patch.
>
No need. I'm now looking at how you use it in LocalTransportAgent.cpp.
That should be enough to get started implementing this.
I've already finished an initial draft of the idea. Here's the commit
(applies on top of the code which uses the blocking call operator). Does
this look right?
commit e746819b058ef40ee72d092be7a2431a2ec36e5a
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Thu Feb 16 18:06:11 2012 +0100
syncevo-dbus-server: utility code for relaying D-Bus calls
All D-Bus calls to the syncevo-dbus-helper should be asynchronous,
with pretty much the same reaction each time that call finishes.
The new ProxyCallback template class provides that common
code: return error or result values.
It's a template class because the number of result values differs.
Only the version for zero result values is provided at the moment.
Because some proxy methods need to hook into the result processing,
signals are provided that the methods can connect to. Be careful when
binding these slots to objects which might already be deleted when
syncevo-dbus-helper replies: use the boost::signals2 tracking feature
for that:
boost::shared_ptr<Client> client = ...
boost::shared_ptr<ConnectionResource> myself = ....
ProxyCallback<GDBusCXX::Result0> callback(result);
// cast necessary because Client::detach is ambiguous
callback.m_success->connect(DBusSuccessSignal_t::slot_type(static_cast< void
(Client::*) (Resource *)>(&Client::detach),
client.get(),
myself.get()).track(client).track(myself));
m_connectionProxy->m_close.start(..., callback);
diff --git a/src/dbus/server/connection-resource.cpp
b/src/dbus/server/connection-resource.cpp
index 44974a9..8557403 100644
--- a/src/dbus/server/connection-resource.cpp
+++ b/src/dbus/server/connection-resource.cpp
@@ -21,12 +21,14 @@
#include "client.h"
#include "connection-resource.h"
#include "session-common.h"
+#include "dbus-proxy.h"
#include <synthesis/san.h>
#include <syncevo/TransportAgent.h>
#include <syncevo/SyncContext.h>
#include <boost/lexical_cast.hpp>
+#include <boost/bind.hpp>
using namespace GDBusCXX;
@@ -69,7 +71,9 @@ std::string ConnectionResource::buildDescription(const StringMap
&peer)
void ConnectionResource::process(const Caller_t &caller,
const GDBusCXX::DBusArray<uint8_t> &msg,
- const std::string &msgType)
+ const std::string &msgType,
+ const boost::shared_ptr<GDBusCXX::Result0>
&result)
+
{
boost::shared_ptr<Client> client(m_server.findClient(caller));
if (!client) {
@@ -82,13 +86,15 @@ void ConnectionResource::process(const Caller_t &caller,
throw runtime_error("client does not own connection");
}
- m_connectionProxy->m_process(msg,
- msgType,
- m_peer,
- m_mustAuthenticate);
+ m_connectionProxy->m_process.start(msg,
+ msgType,
+ m_peer,
+ m_mustAuthenticate,
+ MakeProxyCallback(result));
}
-void ConnectionResource::close(const GDBusCXX::Caller_t &caller, bool normal, const
std::string &error)
+void ConnectionResource::close(const GDBusCXX::Caller_t &caller, bool normal, const
std::string &error,
+ const boost::shared_ptr<GDBusCXX::Result0>
&result)
{
SE_LOG_DEBUG(NULL, NULL, "D-Bus client %s closes connection %s %s%s%s",
caller.c_str(),
@@ -102,11 +108,27 @@ void ConnectionResource::close(const GDBusCXX::Caller_t &caller,
bool normal, co
throw runtime_error("unknown client");
}
- m_connectionProxy->m_close(normal,
- error);
+ // Does the client have to own this resource to close it()?
+ // Probably. We also need the smart pointer below.
+ boost::shared_ptr<ConnectionResource> myself =
+ boost::static_pointer_cast<ConnectionResource,
Resource>(client->findResource(this));
+ if (!myself) {
+ throw runtime_error("client does not own connection");
+ }
- // if we get there, then call was successfull.
- client->detach(this);
+ // If the close() calls succeeds, then we remove ourselves from
+ // the client. boost::signals2 tracking ensures that
+ // Client::detach() will not be called with a stale Client or
+ // ConnectionResource pointer.
+ //
+ // static_cast is necessary because detach() is ambiguous.
+ ProxyCallback<GDBusCXX::Result0> callback(result);
+ callback.m_success->connect(DBusSuccessSignal_t::slot_type(static_cast< void
(Client::*) (Resource *)>(&Client::detach),
+ client.get(),
+
myself.get()).track(client).track(myself));
+ m_connectionProxy->m_close.start(normal,
+ error,
+ callback);
}
void ConnectionResource::replyCb(const GDBusCXX::DBusArray<uint8_t> &reply,
const std::string &replyType,
diff --git a/src/dbus/server/connection-resource.h
b/src/dbus/server/connection-resource.h
index 1872851..02d8b0c 100644
--- a/src/dbus/server/connection-resource.h
+++ b/src/dbus/server/connection-resource.h
@@ -75,10 +75,13 @@ class ConnectionResource : public GDBusCXX::DBusObjectHelper,
/** Connection.Process */
void process(const GDBusCXX::Caller_t &caller, const
GDBusCXX::DBusArray<uint8_t> &msg,
- const std::string &msgType);
+ const std::string &msgType,
+ const boost::shared_ptr<GDBusCXX::Result0> &result);
/** Connection.Close */
- void close(const GDBusCXX::Caller_t &caller, bool normal, const std::string
&error);
+ void close(const GDBusCXX::Caller_t &caller, bool normal, const std::string
&error,
+ const boost::shared_ptr<GDBusCXX::Result0> &result);
+
GDBusCXX::EmitSignal0 emitAbort;
bool m_abortSent;
diff --git a/src/dbus/server/dbus-proxy.h b/src/dbus/server/dbus-proxy.h
new file mode 100644
index 0000000..e3fa4f1
--- /dev/null
+++ b/src/dbus/server/dbus-proxy.h
@@ -0,0 +1,111 @@
+/*
+ * Copyright (C) 2012 Intel Corporation
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) version 3.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */
+
+/**
+ * Utility code for relaying D-Bus method calls and signals from
+ * syncevo-dbus-server to syncevo-dbus-helper.
+ */
+
+#ifndef DBUS_PROXY_H
+#define DBUS_PROXY_H
+
+#include <string>
+#include <boost/shared_ptr.hpp>
+#include <boost/signals2.hpp>
+
+#include <gdbus-cxx.h>
+
+#include <syncevo/declarations.h>
+SE_BEGIN_CXX
+
+typedef boost::signals2::signal<void ()> DBusSuccessSignal_t;
+typedef boost::signals2::signal<void (const std::string &)>
DBusFailureSignal_t;
+
+/**
+ * Use this class in syncevo-dbus-server as callback for asynchronous
+ * method calls to syncevo-dbus-helper. Once it gets the reply from
+ * syncevo-dbus-helper (successful or otherwise), it will finish the
+ * pending method call.
+ *
+ * It is possible to hook into the reply processing by connecting
+ * to the signal(s) provided by the class. At the moment, only
+ * a single signal is provided, with the error string as parameter.
+ * More signals could be added as needed.
+ *
+ * The signals have to be shared pointers because ProxyCallback
+ * must be copyable, which boost::signals aren't.
+ *
+ * Exceptions thrown while processing the reply will be logged by
+ * the GDBus C++ bindings, but because they happen inside the main
+ * event loop, they cannot be propagated to the upper layers.
+ *
+ * @param R some kind of GDBusCXX::Result0/1/2/3
+ */
+template <class R> class ProxyCallback
+{
+ public:
+ ProxyCallback(const boost::shared_ptr<R> &result) :
+ m_success(new DBusSuccessSignal_t),
+ m_failure(new DBusFailureSignal_t),
+ m_result(result)
+ {}
+
+ void operator () (const std::string &error);
+
+ /**
+ * Triggered after a successful method call was reported back to
+ * the original caller.
+ */
+ boost::shared_ptr< DBusSuccessSignal_t > m_success;
+
+ /**
+ * Triggered after a method call failure was reported back to the
+ * original caller.
+ */
+ boost::shared_ptr< DBusFailureSignal_t > m_failure;
+
+ private:
+ boost::shared_ptr<R> m_result;
+};
+
+template <class R> void ProxyCallback<R>::operator () (const std::string
&error)
+{
+ if (error.empty()) {
+ // TODO: split error into bus name + description and relay exactly that
+ // (see Chris' code for that)
+
m_result->failed(GDBusCXX::dbus_error("org.syncevolution.gdbuscxx.Exception",
+ error));
+ (*m_failure)(error);
+ } else {
+ m_result->done();
+ (*m_success)();
+ }
+}
+
+/**
+ * utility method for creating a callback of the right type
+ */
+template <class R> class ProxyCallback<R> MakeProxyCallback(const
boost::shared_ptr<R> &result)
+{
+ return ProxyCallback<R>(result);
+}
+
+SE_END_CXX
+
+#endif // RESOURCE_H
diff --git a/src/dbus/server/server.am b/src/dbus/server/server.am
index 1a12274..cd99acd 100644
--- a/src/dbus/server/server.am
+++ b/src/dbus/server/server.am
@@ -27,6 +27,7 @@ src_dbus_server_server_cpp_files = \
src_dbus_server_server_h_files = \
$(src_dbus_server_server_cpp_files:.cpp=.h) \
+ src/dbus/server/dbus-proxy.h \
src/dbus/server/notification-backend-base.h \
src/dbus/server/notification-manager.h \
src/dbus/server/notification-manager-base.h \
--
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.
2:41 a.m.
On Thu, Feb 16, 2012 at 6:15 PM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
On Thu, 2012-02-16 at 16:47 +0100, Chris Kühl wrote:
> On Thu, Feb 16, 2012 at 4:19 PM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
> > The flow of messages then becomes:
> > 1. syncevo-dbus-server receives method call, stores Result pointer
> > 2. syncevo-dbus-server sends method call to helper
> > 3. helper receives method call, replies (asynchronously or
> > synchronously)
> > 4. syncevo-dbus-server receives reply
> > 5. syncevo-dbus-server relays reply to original caller
> >
> > I'll see whether I can cook up a patch.
> >
>
> No need. I'm now looking at how you use it in LocalTransportAgent.cpp.
> That should be enough to get started implementing this.
I've already finished an initial draft of the idea. Here's the commit
(applies on top of the code which uses the blocking call operator). Does
this look right?
Btw, Krzesimir is working on getting these changes integrated.
Cheers,
Chris
3:01 a.m.
On Fri, 2012-02-17 at 16:41 +0100, Chris Kühl wrote:
On Thu, Feb 16, 2012 at 6:15 PM, Patrick Ohly
<patrick.ohly(a)intel.com> wrote:
> I've already finished an initial draft of the idea. Here's the commit
> (applies on top of the code which uses the blocking call operator). Does
> this look right?
>
Btw, Krzesimir is working on getting these changes integrated.
Okay, then I'll let it stand as it is now.
FYI, I finished integrating the GDBus C++ changes into master. A rebased
branch with your code and my partial asynchronous proxy class is in the
cssr1-pohly branch.
--
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.
9:44 p.m.
New subject: [SyncEvolution] DBus server rework: syncevo-dbus-server <-> helper communication
On Thu, 2012-02-16 at 15:32 +0100, Chris Kühl wrote:
> How is that going?
>
> I'm now looking more seriously at the code myself.
I wouldn't take too close a look at it just yet, I'm still working on
getting some fundamental pieces working, like the main loop dance in
the helper.
This is based on an older statement of mine:
"Anything that runs a sync session (i.e. the helper) will have to
continue doing the g_main_loop dance or become multithreaded, because
the libsynthesis APIs are not asynchronous."
As we've seen, for example in the simplified command line client of the
D-Bus server, introducing the blocking method calls considerably
simplified code which is allowed to block.
By definition, anything in a sync session is allowed to block. There are
downsides, the main one being stuck in a long-running operation while
the session is already meant to be aborted, in which case that operation
should also be aborted.
The recently introduced SuspendFlags class is meant to help with that.
Another downside is that requests from syncevo-dbus-server to the helper
via D-Bus will not be processed unless the helper runs the main loop at
least occasionally. Are there any such requests?
So what I am asking now is basically: can we use blocking method calls
from helper to syncevo-dbus-server and would it help?
I think it would help, at least in some cases.
For example, requesting a password can be blocking. In that case it
would be the job of the syncevo-dbus-server to deny the request when the
helper is meant to abort. Same for other blocking requests.
Another alternative would be to include "abort method call"
functionality into our blocking D-Bus method call implementation. For
example, a SIGTERM could be sent to the helper, get caught by
SuspendFlags and then be detected by the generic code which executes a
blocking D-Bus call. That might simplify the logic in the
syncevo-dbus-server (send SIGTERM, wait for helper to terminate).
It would also help the PBAP backend, which now also blocks in a D-Bus
method call to obexd while the session it participates in might already
have been aborted.
It makes the D-Bus bindings a bit more difficult, in particular because
they are meant to be usable without SyncEvolution (not sure whether that
is still useful). I'm just tossing out the idea so that we can consider
its merits.
Chris, you are the one designing the syncevo-dbus-server <-> helper
interaction and know where the pain points are. If changes in the
infrastructure will help, by all means, let's do that.
--
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.
12:15 a.m.
New subject: [SyncEvolution] DBus server rework: syncevo-dbus-server <-> helper communication
2012/2/22 Patrick Ohly <patrick.ohly(a)intel.com>:
On Thu, 2012-02-16 at 15:32 +0100, Chris Kühl wrote:
> > How is that going?
> >
> > I'm now looking more seriously at the code myself.
>
> I wouldn't take too close a look at it just yet, I'm still working on
> getting some fundamental pieces working, like the main loop dance in
> the helper.
This is based on an older statement of mine:
"Anything that runs a sync session (i.e. the helper) will have to
continue doing the g_main_loop dance or become multithreaded, because
the libsynthesis APIs are not asynchronous."
As we've seen, for example in the simplified command line client of the
D-Bus server, introducing the blocking method calls considerably
simplified code which is allowed to block.
Can you point me to this commit? I've not been able to track it down.
Knowing the code, I can imagine the changes that would be made,
though.
By definition, anything in a sync session is allowed to block. There
are
downsides, the main one being stuck in a long-running operation while
the session is already meant to be aborted, in which case that operation
should also be aborted.
The recently introduced SuspendFlags class is meant to help with that.
Another downside is that requests from syncevo-dbus-server to the helper
via D-Bus will not be processed unless the helper runs the main loop at
least occasionally. Are there any such requests?
So what I am asking now is basically: can we use blocking method calls
from helper to syncevo-dbus-server and would it help?
Right now, as you know, the one-to-one DBus interface is on the helper
process side. This makes more sense to me because there is currently
only one instance (password request) where the helper is requesting
information from the server process. In this case, we send a signal
and block until the password response DBus method is invoked. But this
is the single instance that the helper process needs to get
information from the server process. Moving the one-to-one DBus
interface to the server process would introduce many more of these
signal/response methods as the server often requests info from the
helper due to the client calls.
I think it would help, at least in some cases.
For example, requesting a password can be blocking. In that case it
would be the job of the syncevo-dbus-server to deny the request when the
helper is meant to abort. Same for other blocking requests.
And we do block here. It's just doing a g_main_context_iteration loop
while waiting for the server to do the Info Request. But like I said,
this is the only instance where this is needed.
Another alternative would be to include "abort method
call"
functionality into our blocking D-Bus method call implementation. For
example, a SIGTERM could be sent to the helper, get caught by
SuspendFlags and then be detected by the generic code which executes a
blocking D-Bus call. That might simplify the logic in the
syncevo-dbus-server (send SIGTERM, wait for helper to terminate).
It would also help the PBAP backend, which now also blocks in a D-Bus
method call to obexd while the session it participates in might already
have been aborted.
It makes the D-Bus bindings a bit more difficult, in particular because
they are meant to be usable without SyncEvolution (not sure whether that
is still useful). I'm just tossing out the idea so that we can consider
its merits.
Is there actually an instance of this binding be used outside of
SyncEvolution? Maybe moving out into its own project would help with
that.
Chris, you are the one designing the syncevo-dbus-server <-> helper
interaction and know where the pain points are. If changes in the
infrastructure will help, by all means, let's do that.
At this point, the changes required to do the g_main_loop dance are
*far* less than those that are required to move the one-to-one DBus
interface to the server process. As I've said, I think it's actually
better to keep the interface in the helper.
In short, I don't think this would really help.
Cheers,
Chris
12:27 a.m.
New subject: [SyncEvolution] DBus server rework: syncevo-dbus-server <-> helper communication
On Wed, 2012-02-22 at 14:15 +0100, Chris Kühl wrote:
2012/2/22 Patrick Ohly <patrick.ohly(a)intel.com>:
> On Thu, 2012-02-16 at 15:32 +0100, Chris Kühl wrote:
>> > How is that going?
>> >
>> > I'm now looking more seriously at the code myself.
>>
>> I wouldn't take too close a look at it just yet, I'm still working on
>> getting some fundamental pieces working, like the main loop dance in
>> the helper.
>
> This is based on an older statement of mine:
> "Anything that runs a sync session (i.e. the helper) will have to
> continue doing the g_main_loop dance or become multithreaded, because
> the libsynthesis APIs are not asynchronous."
>
> As we've seen, for example in the simplified command line client of the
> D-Bus server, introducing the blocking method calls considerably
> simplified code which is allowed to block.
>
Can you point me to this commit? I've not been able to track it down.
Knowing the code, I can imagine the changes that would be made,
though.
It's fcdd1b265b on the master branch:
command line: fixed --status and --monitor
The implementation was broken when using GDBus GIO (mismatch between
string and D-Bus object type) and probably also with GDBus libdbus
(missing boolean for GetConfig()).
Rewritten using blocking D-Bus calls, which makes the implementation
also a lot shorter and easier to understand.
It would be good to have automated tests for these features. Not done
yet.
> By definition, anything in a sync session is allowed to block.
There are
> downsides, the main one being stuck in a long-running operation while
> the session is already meant to be aborted, in which case that operation
> should also be aborted.
>
> The recently introduced SuspendFlags class is meant to help with that.
>
> Another downside is that requests from syncevo-dbus-server to the helper
> via D-Bus will not be processed unless the helper runs the main loop at
> least occasionally. Are there any such requests?
>
> So what I am asking now is basically: can we use blocking method calls
> from helper to syncevo-dbus-server and would it help?
>
Right now, as you know, the one-to-one DBus interface is on the helper
process side.
I haven't looked at the internal interface defition(s) at all.
This makes more sense to me because there is currently
only one instance (password request) where the helper is requesting
information from the server process. In this case, we send a signal
and block until the password response DBus method is invoked. But this
is the single instance that the helper process needs to get
information from the server process. Moving the one-to-one DBus
interface to the server process would introduce many more of these
signal/response methods as the server often requests info from the
helper due to the client calls.
Makes sense.
> I think it would help, at least in some cases.
>
> For example, requesting a password can be blocking. In that case it
> would be the job of the syncevo-dbus-server to deny the request when the
> helper is meant to abort. Same for other blocking requests.
>
And we do block here. It's just doing a g_main_context_iteration loop
while waiting for the server to do the Info Request. But like I said,
this is the only instance where this is needed.
Does it handle requests to abort here? How are those communicated? Via
signal or D-Bus call?
> Another alternative would be to include "abort method
call"
> functionality into our blocking D-Bus method call implementation. For
> example, a SIGTERM could be sent to the helper, get caught by
> SuspendFlags and then be detected by the generic code which executes a
> blocking D-Bus call. That might simplify the logic in the
> syncevo-dbus-server (send SIGTERM, wait for helper to terminate).
>
> It would also help the PBAP backend, which now also blocks in a D-Bus
> method call to obexd while the session it participates in might already
> have been aborted.
>
> It makes the D-Bus bindings a bit more difficult, in particular because
> they are meant to be usable without SyncEvolution (not sure whether that
> is still useful). I'm just tossing out the idea so that we can consider
> its merits.
Is there actually an instance of this binding be used outside of
SyncEvolution?
Not at the moment. I don't know whether there is an interest either. I
think it is pretty neat, but SyncEvolution is a bit special by trying to
be "pure C++" without introducing too many external dependencies. Qt C++
developers probably wont' care and just use the Qt binding generator.
Maybe moving out into its own project would help with
that.
Perhaps. Right now I just try to avoid making that impossible.
In short, I don't think this would really help.
Fair enough. It's good to talk about it and record these thoughts.
--
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.
2:03 a.m.
New subject: [SyncEvolution] DBus server rework: syncevo-dbus-server <-> helper communication
On Wed, Feb 22, 2012 at 2:27 PM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
On Wed, 2012-02-22 at 14:15 +0100, Chris Kühl wrote:
> 2012/2/22 Patrick Ohly <patrick.ohly(a)intel.com>:
>
> And we do block here. It's just doing a g_main_context_iteration loop
> while waiting for the server to do the Info Request. But like I said,
> this is the only instance where this is needed.
Does it handle requests to abort here? How are those communicated? Via
signal or D-Bus call?
Currently it doesn't handle aborting. I'll pass a shutdownRequested
flag, which is set in niam(), to the Session similar to how it's done
for the Server. The loop and askPassword function will then be exited
when that's true.
In general shutdown occurs if the server calls serverShutdown() and
the session has not started running or if a shutdown signal was
received. There will probably be some refinement needed here, though.
Cheers,
Chris
3861
days inactive
3879
days old
syncevolution@synevolution.org
38 comments
4 participants
participants (4)
-
Chris Kühl -
Krzesimir Nowak -
Murray Cumming -
Patrick Ohly