When using GIO, it is possible to avoid the DBusServer listening on a
publicly accessible address. Connection setup becomes more reliable,
too, because the D-Bus server side can detect that a child died
because the connection will be closed.
When using libdbus, the traditional server/listen and client/connect
model is still used. GDBus GIO mimicks the same API to minimize
changes in ForkExec. The API gets simplified to support both
approaches:
- Choosing an address is no longer possible (was only used in the
test example anyway)
- The new connection callback must be specified already when starting
the server.
The GDBus GIO implementation uses SyncEvolution utility code. Strictly
speaking, this creates a circular dependency
libsyncevolution<->libgdbus. This is solved by not linking libgdbus
against libsyncevolution and expecting executables to do that instead.
The GDBus GIO example no longer linked because of that; it gets removed
because it was of little value.
Now that we started using CLOEXEC, we might as well use it for the
permanent file descriptors created for each ForkExec instance.
The remaining warnings are either not valid or refer to code which
is intentionally broken. Suppress these warnings.
SyncEvolution now passes scanning with cppcheck 1.61 without warnings.
The problem is in the C++ D-Bus binding. If the method that gets bound
to D-Bus returns a value, that value is ignored in the signature:
int foo() => no out parameter
It works when the method is declared as having a retval:
void foo (int &result) => integer out parameter
This problem exists for both the libdbus and the GIO D-Bus
bindings. In SyncEvolution it affects methods like GetVersions().
The fix consists of always returning a type for the R return type in
the libdbus bindings. getReturn() returned "" because the type was not
a retval. In the GIO binding, the R type was not handled at all. Also
move some common code into a helper function.
If Boost was installed in a non-default location, it was not found
when compiling the gdbus lib because the necessary compiler flags
were not used for it.
When using GIO D-Bus, sometimes messages created shortly before
shutting down were not sent. Must flush a connection explicitly.
A NOP when using libdbus.
When using libdbus instead of GIO D-Bus (as done by syncevolution.org
binaries and SyncEvolution on Maemo), local sync may have aborted
after 25 seconds when syncing many items with a D-Bus timeout error:
[ERROR] sending message to child failed: org.freedesktop.DBus.Error.NoReply: Did not receive a reply. Possible causes include: the remote application did not send a reply, the message bus security policy blocked the reply, the reply timeout expired, or the network connection was broken.
Reported by Toke Høiland-Jørgensen for Harmattan. Somehow not encountered
elsewhere.
Root cause is the use of the default D-Bus timeout of 25 seconds
instead of disabling the timeout altogether. Fixed by using
DBUS_TIMEOUT_INFINITE and INT_MAX as fallback.
One method was still virtual, from the time when the class had a base
class. Remove the "virtual" keyword because a) it causes a compiler
warning in recent g++ about a class with non-virtual destructor with a
virtual method and b) it makes the generated code unnecessarily
complex.
Avoid the abstract base class and implement the GDBusXX::Watch
class directly. This became necessary because moving the GIO
implementation into the .cpp resulted in strange crashes around
the virtual setCallback() method - but only when optimization
was enabled. Compiler bug?!
Simplifying the class hierarchy is the right step either way. It was
done with separate classes initially to avoid dependencies on
gdbus-cxx-bridge.h in code which just needs to interact with the base
functionality, but it turned out that no code needs that.
In some cases the sender of a D-Bus signal doesn't care whether it can
be delivered (log messages, for example). Such signals can be declared
"optional" with a template parameter, which will suppress throwing
error exceptions.
The API is unchanged. EmitSignal0 is a typedef for the variant with
the traditional semantic (throw errors). To suppress that, use
EmitSignal0Template<true>.
libdbus has a bug where it doesn't notify the caller when a pending
method call failed because the connection went
down (https://bugs.freedesktop.org/show_bug.cgi?id=49728). ForkExec
relied on that to detect loss of connection.
Therefore a different method is needed to alert ForkExec. This commit
finishes the implementation of g_dbus_set_disconnect_function() and
provides a C++ binding for ForkExec.
Writing D-Bus traits involves writing append() and get() methods which
take native types (connection, message, iterator, ...). Code which
calls other dbus_traits can be written without ifdefs when these
native types have aliases which are the same in the GIO and libdbus
implementations.
A signals.connect() to a EmitSignal instance works only after defining
the result_type for boost::bind. Example:
m_configChangedSignal.connect(boost::bind(boost::ref(configChanged)))
Asking for the type of an empty array returns zero instead of
the real type. Because the type was checked, this triggered
an exception during decoding. Now the check accepts zero if,
and only if, the array is empty.
NULL callbacks are always allowed. Checking for those was inconsistent
(no need to call empty(), better use implicit conversion to bool) and
missing for asynchronous method calls.
It usually is not NULL, but for now Connection stuff does not work
in syncevo-dbus-helper and in that situation pending call can be NULL.
This avoids aborts on assertion failure.
Renamed GDBusMessagePtr to DBusMessagePtr. This makes the two
implementations more consistent.
Originally this was done because DBusMessagePtr also become
part of the public API. That's not the case anymore, but the
change still makes sense.
The code which iterates over watches used a freed list entry when the
watch callback removed the watch itself. Fixed by moving to the next
list element before invoking the callback. This still doesn't fix the
issue if the callback removes exactly the next list element.
In libdbus, the variant must be opened as a sub-iterator with the type
of the actual value as parameter. Do that in the boost::variant
visitor, using a template call operator so that we don't need multiple
versions of it for each type in the variant.
The call operator on DBusClientCall[0123] is now blocking. It will
return result values as single value, std::pair or boost::tuple,
respectively. If an error is encountered either locally or in the
peer, a runtime_error is thrown.
This is an API change. The traditional implementation was to start an
asynchronous call. That is still possible, but has to be requested
explicitly with the new start() method.
This distinction is necessary because C++ cannot guess (easily)
whether the callback parameter is a callback type or another parameter
for the D-Bus peer. It's also a bit cleaner in the source code because
now the call operator really acts like a normal, synchronous call.
The downside of the synchronous calls is that complex return values
have to be copied more often. If that is too expensive, then use the
asynchronous start() + callback approach.
When attempting to activate a SignalWatch for one-to-one connections
an error occured because activation attempted to add a match rule
using the AddMatch member of the org.freedsktop.DBus interface.
This patch adds an is_bus_conn argument to the SignalWatch constructor
which is used to forgo adding a match rule to non-bus
connections. is_bus_conn defaults to true so no existing code needs
modification.
Not calling dbus_server_disconnect() kept the socket open. The error
handling for trying different bus unix domain paths was also broke
(didn't reset the DBusError).
Instead of overloading std::pair with additional meaning, use
GDBusCXX::DBusArray when we want to pass byte arrays. Allows defining
"normal" traits for std::pair.
API change, must be done in both D-Bus CXX layers and in the code using it.
- moved common code into base class (prepare/send) reps. .cpp file (CheckError())
- retrieve D-Bus error description from error message and use it as part of
the C++ exception description, in addition to the error name (only error
name was used before)
- add call operators with up to 10 parameters
Added methods and classes for setting up direct D-Bus communication.
Only implemented for the non-GIO GDBus at the moment.
The callback must not throw exceptions because it is invoked from
plain C code.
DBusCallObject and friends were not used anywhere. Removed.
DBusObject and DBusRemoteObject used pure virtual methods to let
derived classes provide information about interface, path,
destination, method and the connection. The idea behind that was that
most of these strings are static and thus do not need to be copied.
The downside is that one had to derive from these classes in order to
provide the required information. The same class could not own two
instances of the generic DBusObject to access two different
destinations. It also sprinkled the code for setting up D-Bus
operations into several different places (constructor, class definition).
Now the information is passed to the DBus[Remote]Object constructor
and stored in the classes, which are thus merely containers for the
information and thus easier to use. Users of the classes still derive
from them, to keep the change smaller, although that is no longer
necessary.
Removed plain DBusConnection pointers from the C++ interface, return
DBusConnectionPtr instead. The exception is DBusObject, which still
returns plain pointers owned by the instance (as before) to ease
integration with the underlying D-Bus library. DBUS_CONNECTION_TYPE
is not needed.
This revealed that quite some code incorrectly took another reference
to the connection when assigning to DBusConnectionPtr (assignment
always increases the refcount, only in the constructor is that
optional). As a result the private connections were never
destructed. Apparently there were some global pointers to active
connections, so that this (minor) resource leak didn't show up in
valgrind.
It also showed that the closing of the D-Bus connection never happened
properly, although libdbus requires it. The ref counting mechanism
cannot be used for this because it cannot be checked whether the last
reference is about to be dropped. The slightly hackish solution is to
designate one DBusObject as the "main owner" of the connection. When
that object destructs, it closes the connection. There might still be
some other references; they simply cannot (and shouldn't) send or
receive messages anymore.
After removing a watch, destroying the glib source once more accessed
the now invalid "watch" pointer in finalize_watch(). Found in D-Bus
server mode after closing one connection.
The GSource was kept running and continued to use the freed
connection. Must use g_source_destroy() instead of g_source_unref() to
remove the source from the main loop in addition to decreasing its
ref count.
free_watch() is the right place to free WatchData, not
finalize_watch(), because the call sequence is free_watch() ->
g_source_destroy(source) -> finalize_watch().
Closing an unshared connection in free_connection() is too late,
because that is the destructor callback which is only called after
the last reference was already dropped. For the same reason the
dbus_connection_unref() there was wrong.
That code was never called in SyncEvolution because all connections
had their reference count increased one time to often when assigning
to DBusConnectionPtr. Fix will be committed separately. The problems
in this commit were found after fixing that problem and introducing
temporary connections in a D-Bus server, because then the faulty code
was called.
clang 2.9 is sensitive to the order of include statements when the
boost:: namespace is used. It happened to work with Bluez GDBus, but failed
with GIO GDBus.
Let's use the global namespace because it works in all case, although
the Boost documentation makes it sound like that the boost:: namespace
should be used.
When a method call caused the interface of the method to be removed,
GDBus used invalid memory after the method returned. It must be
prepared for that and cache the relevant information (the method flags
in this case).
If the watch was never activated, m_tag was never set, which led to an
invalid memory read in the destructor. Also be more careful with using
the connection and better check that it still exists.
dbus_message_iter_get_signature() returns a newly allocated string.
Freeing that now with a smart pointer. SmartPtr.h is not used to
avoid dependencies on the rest of SyncEvolution - just in case that
someone finds this useful.
Background: Prior to this patch a C++ wrapper around Bluez gdbus, a
convienience wrapper around the low-level libdbus, was used by
syncevo-dbus-server. This patch introduces a second C++ wrapper around
the GIO GDBus implementation. The reason for introducing this second
wrapper is to move away from using the in-tree copy of Bluez gdbus and
towards a more well-maintained dbus implementation. Also, libdbus was
not designed to be thread-safe whereas GIO GDBus was.
The GIO GDBus wrapper retains the same public api as the first
one. This means the consumers of this wrapper (syncevo-dbus-server,
for example) have remained almost completely untouched. The only
exceptions are in the few case where libdbus objects where used
directly by the consuming class.
The choice of which wrapper is determined at configure time. The
option can be explicitly set using the --with-gio-gdbus and
--without-gio-gdbus flags or, if no flag is given, an adequate version
of GIO is search for. If found, the GIO GDBus wrapper is chosen.
Recent glib deprecates the direct inclusion of some of its headers,
in favor of including glib.h. Doing that here whenever possible. gi18n.h
still needs to be included directly (otherwise it doesn't compile
on Debian Testing with glib 2.28).
Not sure about gio. I'm keeping the gio/ includes because they
are relevant on platforms where gio was not yet part of glib.
Somehow clang is sensitive to the order of definitions when using
boost::intrusive_ptr. This broke during the D-Bus server reorganization.
Putting the intrusive_ptr_* methods into the global name space instead
of boost works with clang and gcc and avoids the issue.
