When we have attached a name to GDBusConnectionPtr in
dbus_get_bus_connection(), we must copy that name when
passing the connection on.
Otherwise requesting a name via dbus_get_bus_connection()
and later calling dbus_bus_connection_undelay() on a copy
fails to request the name.
Raising exceptions via throwError() looses the original source code
location information. Fixing that by explicitly passing that
information as additional parameter, created with the preprocessor
macro SE_HERE.
Several files included the complex SyncContext.h only because needed
throwError(). A better place for the revised implementation is the
Exception class which used to be in util.h, now Exception.h.
Simplifying the include statements exposed indirect include
dependencies and removed "using namespace std" from the compilation of
some source files which happened to rely on it. We want to get rid of
that name space polution, so fix the code instead of adding it back.
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 signature of the new type is just a 'v' for variant. Recursion is
only supported in combination with std::vector. This is done to keep
implementation simpler. std::vector was chosen over other collections
because it is easier to work with in method calls, the main purpose
of the new type.
The conversion from D-Bus messages accepts anything which can be mapped
into the variant: arrays, tuples, plain types, and of course variants
containing those. This helps with backward compatibility (one can
turn an interface which took a fixed type like std::vector<std::string>
into something with a recursive variant) and with Python, because
Python sends arrays and tuples without converting into variants
when the app uses those simpler types.
One caveat exists with sending an empty list in Python: when using 'v'
as signature in the interface, Python doesn't know how to send the
empty list without assistance by the programmer (as in dbus.Array([],
signature='s'). If possible, avoid the plain 'v' in an interface in
favor of something like 'av' (= std::vector<boost::variant>).
When an asynchronous method call failed with an exception, the code
DBusResult got destructed first before handling the exception and then
sent a generic, uninformative error back to the caller. A test
demonstrating that for the PIM Manager SetPeer() with an invalid UID
be committed separately.
To fix this, the DBusResult must know whether it is really responsible
for replying to the method message. The method call handlers
themselves do not know whether any of their parameters is a
DBusResult, so we have to activate the automatic reply indirectly, via
a guard instance with the same life time as the handler.
When the guard destructs, it can check whether the handler has
replied, because the handler then clears the message pointers. If it
hasn't, the DBusResult becomes the owner of the message. For this to
work, an extra try/catch block is necessary. Otherwise exception
handling would still occur too late, at a time when the guard is gone
and the DBusResult has sent the generic error.
Some other classes had to be adapted to allow for this extra level of
indirection. While doing so, another bug was found.
Retrieving the interface and member used when calling a method
returned the path instead, due to a cut-and-paste error. Fixed. Not
used in practice.
The old code triggered a Klocwork warning about the missing self check
in the copy operator. This was not a real problem because of the
temporary instance from which m_error got taken, but that was not
obvious even to a human.
Better use standard coding practices:
- skip the operation when assigning to self
- reuse the set() operation
- return reference instead of copy (probably a bug!)
The code tried to transfer ownership of the GDBusMethodInfo and
GDBusSignalInfo arrays to the refcounted GDBusInterfaceInfo instance,
but that does not work because g_dbus_interface_info_unref() does not
free those arrays.
Let's simplify this so that all memory, including the GDBusInterfaceInfo
instance, is owned by DBusObjectHelper and kept around as long as the
object is active.
The stack-allocated GDBusInterfaceVTable happened to work in practice,
but the API description does not say whether
g_dbus_connection_register_object() makes a copy, so better keep that
around longer, too.
Finally, clean up checking for "is activated" to use only
m_connId. m_activated was redundant and the method resp. signal
pointers are now always non-NULL.
Exposed as "location" -> (lat, long) in the D-Bus bindings.
Reading, writing and updating are supported. Depends on a folks
release which has the patch which adds FolksLocationDetails.
Default EDS test data now includes GEO. Several peers do not support
it, which gets ignored by synccompare.
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.
The actual implementation is the same as for vector, with D-Bus array
on the D-Bus side. Now the common code is in a template base class and
referenced by the traits for vector, list and deque.
No attempt is made to automatically pick that trait for other classes
which also could be treated like an ordered collection of items.
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.
Previously, strings had to be passed into GDBus as std::string.
Now "const char *" also works. A NULL pointer is allowed and
is turned into an empty string.
DBusObject_t is just an alias for std::string, so functional there's
no change. Adding getObject() which returns a const reference to it
allows the caller to use the value in D-Bus traits where the
difference between a plain string and an object path matters.
Using a DBusClient* to make an actual call does not change the object.
The call is independent of the instance used to make it (see also the
previous commit about handling method results for asynchronous calls
after destroying the call instance). Therefore it makes sense to mark
the methods as const.
Now DBusResult itself tracks whether a reply was sent. If not, it
reports a generic "processing the method call failed" error. If that
error is not good enough for the method implementation, then it must
call failed() itself, as before.
Remove global variables and allow addditional names to be owned via
DBusConnectionPtr::ownName().
g_bus_own_name_on_connection() is given shared access to a ref-counted
OwnNameAsyncData (instead of full ownership) because that way it is
guaranteed that the instance is still around while waiting for name
acquisition in undelay().
Instead of using explicit g_variant_unref() calls, let a smart pointer
class own the instance. This allows to remove code and (more
important) fixes leaks when something throws an exception or the
D-Bus traits for boost::variant return prematurely.
The signal name was not stored, making the actual filtering more
relaxed than it should have been. Worse, the path was redundantly
checked for a complete match even when SIGNAL_FILTER_PATH_PREFIX was
set, thus preventing signal deliver in that case.
Working with obexd 0.47's Transfer object depends on path prefix
filtering of signals to be efficient. This patch adds that support in
the GDBus C++ bindings, together with some documentation on how to
use it.
It also allows relaxing the signal matching by interpreting empty
strings as "match any".
Finally it adds support for passing of sender, path, interface and
member (= signal name) information to the user of the D-Bus
binding. This is necessary because a handler with wildcards for
matching cannot know the actual values unless they are passed to the
callback explicitly. This was not possible at all before for signals
(because extracting that information relied on a message pointer,
which wasn't set while decoding signal messages) and limited to just
the sender in method implementations.
Besides in functionality, GDBus for GIO and for libdbus now also
differ slightly in their API. In order to pass meta data about a
signal into the demarshalling get() methods, they have to be stored in
ExtractArgs first and passed to get() via that struct, which is an
interface change.
Derived code which extends marshalling/demarshalling needs to be
adapted accordingly. Allowing some ifdefs elsewhere seems simpler than
adapting GDBus for libdbus. Those ifdefs can be removed once libdbus
support is no longer needed.
The patch relaxes access control to DBusObject and DBusRemoteObject
because that simplifies the code which works better with std::string.
Traditionally the sender of a signal was not checked. This is probably
related to unsolved questions like "should watching org.foo.bar follow
owner changes", but not doing any checking is just not right
either. Not fixed in this patch.
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 syncevo-dbus-server was started on demand by the D-Bus daemon,
then it registered itself with the daemon before it was ready to
serve requests. Only happened in combination with GIO D-Bus and
thus was not a problem before 1.2.99.x.
One user-visible effect was that the GTK UI did select the default
service when it was started for the first time, because it could not
retrieve that information from syncevo-dbus-server.
The fix consists of delaying the name acquisition. That gives the
caller a chance to register D-Bus objects first, before completing the
connection setup. The semantic change is that
dbus_bus_connection_undelay() must be called on new connections which
have a name (a NOP with libdbus).
This patch tries to minimize code changes. The downside of not
changing the GDBusCXX API more radically is that the bus name must be
attached to DBusConnectionPtr, where it will be copied into each
reference to the connection. Hopefully std::string is smart enough to
share the (small) data in this case. Should be solved cleaner once
libdbus support can be deprecated.
A test for auto-activation and double start of syncevo-dbus-server
is also added.
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.
The previous implementation did not work at all. It relied
on "NameLost" signals with the watched D-Bus name as sender,
but that is not how the signal works. It is sent by the D-Bus
daemon to the old owner of that name if it looses the name.
Instead we need to watch the "NameOwnerChanged" signal and
detect when the peer we are watching is the one who disconnected
from the bus.
Also moved the actual code into the .cpp file because there is
no need to inline all of it.
This problem was found after adding more failure tests to
test-dbus.py. The real-world effect was that syncevo-dbus-server did
not clean up properly when clients disconnected early and that the
command line kept running after syncevo-dbus-server crashed. Did not
affect syncevolution.org binaries, which do not use D-Bus GIO.
The static template method doesn't have to be in the header
file (doesn't depend on template parameters). Moving it into
the .cpp file to minimize compile and executable size.
Runtime work can be minimized by holding the instance
temporarily in a simpler auto pointer before moving it
into the final shared pointer.
Another reason was that compiling with optimization enabled
produced binaries which crashed in that code. However, moving
it did not help.
Testing showed that using the "closed" signal leads to unexpected
error messages, perhaps because it also gets delivered for normal
shutdown. Better use the traditional, known-to-work "monitor method
call" method.
In contrast to the version from GDBus libdbus, GDBus GIO's
dbus_get_bus_connection() with name doesn't fail even if the name
cannot be obtained. That's because it asynchronously asks for the
name without waiting for success.
As a quick-and-dirty solution register a function which kills the
process when the name registration fails. This assumes that the
process will run as a daemon with exactly one name, where failures to
obtain that name are indeed fatal.
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>.
In contrast to the same patch in GDBus libdbus, the GIO version also
includes some refactoring. The common code (storage of DBusObject and
signal name, message send code) is now in a EmitSignalHelper base
class.
Moved common code into the DBusResult base class (sendMsg()) and
throwFailure() function. The revised code there includes the GError
message in the exception, to provide a better explanation than before.
"invalid argument" as error messages provided no information about the
location or reason for the failure. Now the text refers to the
GVariant decoding problem ("g_variant failure") and includes source
code + line (to identify the exact type).
Strictly speaking only GDBus for libdbus really needed the "connection
lost" callback, because the "catch error when pending method call
fails" approach didn't work with it. But GDBus GIO also has a "closed"
signal. Using it is straight-forward and will allow removing the more
complex code in 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)))
All methods called from glib have to catch C++ exceptions. When a
callback let a C++ exception pass, the process was aborted with a
rather mysterious "std::terminate called without pending exception".
All that the D-Bus wrapper can do is abort the program, which is done
here together with logging by calling g_error().
Therefore the real solution is to handle errors inside the C++
callbacks.
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.
