On Tue, 2012-02-14 at 16:02 +0100, Patrick Ohly wrote: 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.

On Wed, 2012-02-15 at 19:03 +0100, Krzesimir Nowak wrote: 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. 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&gt; 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.

On Thu, 2012-02-16 at 10:50 +0100, Patrick Ohly wrote: For example, why did you change this: commit c7a6f28162b5af2d42b41393dc12a16583482312 Author: Krzesimir Nowak <krnowak(a)openismus.com&gt; 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.

On Thu, 2012-02-16 at 16:47 +0100, Chris Kühl 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? commit e746819b058ef40ee72d092be7a2431a2ec36e5a Author: Patrick Ohly <patrick.ohly(a)intel.com&gt; 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.