The word "source" implies reading, while in fact access is read/write.
"datastore" avoids that misconception. Writing it in one word emphasizes
that it is single entity.
While renaming, also remove references to explicit --*-property
parameters. The only necessary use today is "--sync-property ?"
and "--datastore-property ?".
--datastore-property was used instead of the short --store-property
because "store" might be mistaken for the verb. It doesn't matter
that it is longer because it doesn't get typed often.
--source-property must remain valid for backward compatility.
As many user-visible instances of "source" as possible got replaced in
text strings by the newer term "datastore". Debug messages were left
unchanged unless some regex happened to match it.
The source code will continue to use the old variable and class names
based on "source".
Various documentation enhancements:
Better explain what local sync is and how it involves two sync
configs. "originating config" gets introduces instead of just
"sync config".
Better explain the relationship between contexts, sync configs,
and source configs ("a sync config can use the datastore configs in
the same context").
An entire section on config properties in the terminology
section. "item" added (Todd Wilson correctly pointed out that it was
missing).
Less focus on conflict resolution, as suggested by Graham Cobb.
Fix examples that became invalid when fixing the password
storage/lookup mechanism for GNOME keyring in 1.4.
The "command line conventions", "Synchronization beyond SyncML" and
"CalDAV and CardDAV" sections were updated. It's possible that the
other sections also contain slightly incorrect usage of the
terminology or are simply out-dated.
Previously, only syncURL=local://@<context name> was allowed and used
the "target-config@context name" config as target side in the local
sync.
Now "local://config-name@context-name" or simply "local://config-name"
are also allowed. "target-config" is still the fallback if only a
context is give.
It also has one more special meaning: "--configure
target-config@google-calendar" will pick the "Google_Calendar"
template automatically because it knows that the intention is to
configure the target side of a local sync. It does not know that when
using some other name for the config, in which case the template (if
needed) must be specified explicitly.
The process name in output from the target side now also includes the
configuration name if it is not the default "target-config".
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.
The information that the sync is freezing is now also handed down to
the transport and all sources. In the case of PBAP caching, the local
transport notifies the child where the PBAP source then uses Bluez
5.15 Transfer1.Suspend/Resume to freeze/thaw the actual OBEX transfer.
If that fails (for example, not implemented because Bluez is too old
or the transfer is still queueing), then the transfer gets cancelled
and the entire sync fails. This is desirable for PBAP caching and
Bluetooth because a failed sync can easily be recovered from (just
start it again) and the overall goal is to free up Bluetooth bandwidth
quickly.
Diagnostic Log and Trace (DLT) manages a sequence of log messages,
with remote controllable level of detail. SyncEvolution optionally
(can be chosen at compile time and again at runtime) uses DLT instead
of its own syncevolution-log.html files.
"username", "proxyUsername" and "databaseUser" used to be simply a
string containing the name of the respective user or (in the case of
the ActiveSync backend) the account ID in gconf.
Now it is also possible to provide credentials (username + password)
indirectly: when any of these properties is set to "id:<config name>",
then the "username/password" properties in that config are used
instead. This is useful in particular with WebDAV, where credentials
had to be repeated several times (target config, in each database when
used as part of SyncML) or when using a service which requires several
configs (Google via SyncML and CalDAV).
For user names which contain colons, the new "user:<user name>" format
must be used. Strings without colons are assumed to be normal user
names.
This commit changes the SyncConfig APIs for this extension. More work
is needed to make the indirect lookup via "id" functional.
"username", "proxyUsername" and "databaseUser" used to be simply a
string containing the name of the respective user or (in the case of
the ActiveSync backend) the account ID in gconf.
Now it is also possible to provide credentials (username + password)
indirectly: when any of these properties is set to "id:<config name>",
then the "username/password" properties in that config are used
instead. This is useful in particular with WebDAV, where credentials
had to be repeated several times (target config, in each database when
used as part of SyncML) or when using a service which requires several
configs (Google via SyncML and CalDAV).
For user names which contain colons, the new "user:<user name>" format
must be used. Strings without colons are assumed to be normal user
names.
This commit changes the SyncConfig APIs for this extension. More work
is needed to make the indirect lookup via "id" functional.
Syncing was slowed down by fowarding all log messages from the local
sync helper to its parent and from the D-Bus helper to
syncevo-dbus-server. Quite often, the log messages then were simply
discarded by the recipient. To speed up syncing, better filter at the
source.
The syncevo-dbus-helper is told which messages are relevant by
forwarding the syncevo-dbus-server "D-Bus log level" command line
setting to the helper process as part of its argv parameters.
The synevo-local-sync helper applies its own log level now also to the
messages sent to the parent. This ensures that messages stored in the
client log also show up in the parent log (unless the parent has more
restrictive settints, which is uncommon) and that INFO/SHOW messages
still reach the user.
The D-Bus method reply may occur when the calling instance of
LocalTransportAgent is already gone. Therefore bind to a weak
pointer instead of this.
Seen when testing against Apple Calendar Server in the nightly
testing. It was a bit uncertain from the logs why it happened,
but it is clear that it can happen and thus the patch makes
sense even though it might not address the real problem.
Following the boost::instrusive_ptr example and making "add_ref =
true" the default in our CXX GLib and GObject wrappers led to some
memory leaks because it didn't enforce thinking about whether the
plain pointer is already owned by us or not.
It is better to use a mandatory enum value, ADD_REF and TRANSFER_REF,
and force explicit construction. Doing that revealed that the
assignment operator was implemented as constructing a CXX instance
with increased ref count and/or that in some places, a real leak was
caused by increasing the ref count unnecessarily.
Running under valgrind gave a false sense of security. Some of the
real leaks only showed up randomly in tests.
Logging must be thread-safe, because the glib log callback may be
called from arbitrary threads. This becomes more important with EDS
3.8, because it shifts the execution of synchronous calls into
threads.
Thread-safe logging will also be required for running the Synthesis
engine multithreaded, to overlap SyncML client communication with
preparing the sources.
To achieve this, the core Logging module protects its global data with
a recursive mutex. A recursive mutes is used because logging calls
themselves may be recursive, so ensuring single-lock semantic would be
hard.
Ref-counted boost pointers are used to track usage of Logger
instances. This allows removal of an instance from the logging stack
while it may still be in use. Destruction then will be delayed until
the last user of the instance drops it. The instance itself must be
prepared to handle this.
The Logging mutex is available to users of the Logging module. Code
which holds the logging mutex should not lock any other mutex, to
avoid deadlocks. The new code is a bit fuzzy on that, because it calls
other modules (glib, Synthesis engine) while holding the mutex. If
that becomes a problem, then the mutex can be unlocked, at the risk of
leading to reordered log messages in different channels (see
ServerLogger).
Making all loggers follow the new rules uses different
approaches.
Loggers like the one in the local transport child which use a parent
logger and an additional ref-counted class like the D-Bus helper keep
a weak reference to the helper and lock it before use. If it is gone
already, the second logging part is skipped. This is the recommended
approach.
In cases where introducing ref-counting for the second class would
have been too intrusive (Server and SessionHelper), a fake
boost::shared_ptr without a destructor is used as an intermediate step
towards the recommended approach. To avoid race conditions while the
instance these fake pointers refer to destructs, an explicit
"remove()" method is necessary which must hold the Logging
mutex. Using the potentially removed pointer must do the same. Such
fake ref-counted Loggers cannot be used as parent logger of other
loggers, because then remove() would not be able to drop the last
reference to the fake boost::shared_ptr.
Loggers with fake boost::shared_ptr must keep a strong reference,
because no-one else does. The goal is to turn this into weak
references eventually.
LogDir must protect concurrent access to m_report and the Synthesis
engine.
The LogRedirectLogger assumes that it is still the active logger while
disabling itself. The remove() callback method will always be invoked
before removing a logger from the stack.
The method became obsolete when introducing fork+exec for local
syncing. Before that, the method was used to remove unsafe loggers in
the child's process. Now exec() does that for us.
Changing the process' name, even temporarily, is not thread-safe
and needs to be avoided. Instead pass the additional parameter
explicitly via the MessageOptions container.
Logger::formatLines() and LogStdout::write() are passed the process
name as "const std::string *" pointer, with Logger::getProcessName()
acting as fallback for NULL. Calling that is delayed as long as possible,
to avoid making the string copy when not necessary.
The previous implementation of formatLines() used a std::string
reference, but only used the content to determine whether to include
the current process name - probably not what was intended, but harmless
because either the empty string or the current name were passed.
The number of parameters for messagev has grown unreasonably, and more
parameters will be needed soon (explicit process name). Better store
the parameters in a struct and pass around a pointer to such a struct
instance. The code becomes shorter and new parameters can be passed
through intermediate functions without modifying them.
Passing an explicit Logger instance to the SE_LOG* macros was hardly
ever used and only made the macros more complex.
The only usage of it was in some backends, which then added a prefix
string automatically. The same effect can be achieved by passing
getDisplayName(). Exception handling no longer needs a Logger instance,
the prefix alone is enough.
The other intended usage, avoiding global variables for logging by
passing a logger known to the caller, was not possible at all.
To make prefix handling more flexible, it is now passed as a "const
std::string *" instead of a "const char *".
Avoid writing config file changes to disk by enabling a new
"ephemeral" mode for syncing via the PIM Manager. This mode is enabled
via the sync mode parameter of the D-Bus API and from there passed
through to the SyncConfig and local sync helper. It cannot be enabled
in the config files (yet?).
In this mode, config file changes are not flushed resp. discarded
directly in the config nodes. This prevents writing to .ini files in
~/.config.
The "synthesis" binfile client files are still written, but they get
redirected into the session directory, which can (and should) be set
to a temp file system and get deleted again quickly.
Data dumps are turned off now in the configs created by the PIM
Manager.
Fetched the code and its history from the 1.3.1 archives at:
http://people.debian.org/~ovek/maemo/http://people.debian.org/~ovek/harmattan/
Merged almost everything, except for Maemo/Harmattan specific build files:
autogen-maemo.sh builddeb buildsrc debian
The following changes were also removed, because they are either local
workarounds or merge artifacts which probably also don't belong into
the Maemo/Harmattan branch:
diff --git a/configure.ac b/configure.ac
index cb66617..2c4403c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -44,7 +44,7 @@ if test "$enable_release_mode" = "yes"; then
AC_DEFINE(SYNCEVOLUTION_STABLE_RELEASE, 1, [binary is meant for end-users])
fi
-AM_INIT_AUTOMAKE([1.11.1 tar-ustar silent-rules subdir-objects -Wno-portability])
+AM_INIT_AUTOMAKE([subdir-objects -Wno-portability])
AM_PROG_CC_C_O
diff --git a/src/backends/webdav/CalDAVSource.cpp b/src/backends/webdav/CalDAVSource.cpp
index decd170..7d338ac 100644
--- a/src/backends/webdav/CalDAVSource.cpp
+++ b/src/backends/webdav/CalDAVSource.cpp
@@ -1282,6 +1282,7 @@ void CalDAVSource::Event::fixIncomingCalendar(icalcomponent *calendar)
// time.
bool ridInUTC = false;
const icaltimezone *zone = NULL;
+ icalcomponent *parent = NULL;
for (icalcomponent *comp = icalcomponent_get_first_component(calendar, ICAL_VEVENT_COMPONENT);
comp;
@@ -1295,6 +1296,7 @@ void CalDAVSource::Event::fixIncomingCalendar(icalcomponent *calendar)
// is parent event? -> remember time zone unless it is UTC
static const struct icaltimetype null = { 0 };
if (!memcmp(&rid, &null, sizeof(null))) {
+ parent = comp;
struct icaltimetype dtstart = icalcomponent_get_dtstart(comp);
if (!icaltime_is_utc(dtstart)) {
zone = icaltime_get_timezone(dtstart);
diff --git a/src/backends/webdav/CalDAVSource.h b/src/backends/webdav/CalDAVSource.h
index 517ac2f..fa7c2ca 100644
--- a/src/backends/webdav/CalDAVSource.h
+++ b/src/backends/webdav/CalDAVSource.h
@@ -45,6 +45,10 @@ class CalDAVSource : public WebDAVSource,
virtual void removeMergedItem(const std::string &luid);
virtual void flushItem(const string &uid);
virtual std::string getSubDescription(const string &uid, const string &subid);
+ virtual void updateSynthesisInfo(SynthesisInfo &info,
+ XMLConfigFragments &fragments) {
+ info.m_backendRule = "HAVE-SYNCEVOLUTION-EXDATE-DETACHED";
+ }
// implementation of SyncSourceLogging callback
virtual std::string getDescription(const string &luid);
Making SySync_ConsolePrintf a real instance inside SyncEvolution leads
to link errors in other configurations. It really has to be extern. Added
a comment to the master branch to make that more obvious:
-extern "C" { // without curly braces, g++ 4.2 thinks the variable is extern
- int (*SySync_ConsolePrintf)(FILE *stream, const char *format, ...);
-}
+// This is just the declaration. The actual function pointer instance
+// is inside libsynthesis, which, for historic purposes, doesn't define
+// it in its header files (yet).
+extern "C" int (*SySync_ConsolePrintf)(FILE *stream, const char *format, ...);
It is possible to sync source A in context @X with source B in the
same context @X. The PIM manager is doing that with one context per
peer and databases that are guaranteed to not conflict with anything
else.
Therefore allow such configs. A better check is left for later, if it
really should be needed at all.
This patch introduces support for true one-way syncing ("caching"):
the local datastore is meant to be an exact copy of the data on the
remote side. The assumption is that no modifications are ever made
locally outside of syncing. This is different from one-way sync modes,
which allows local changes and only temporarily disables sending them
to the remote side.
Another goal of the new mode is to avoid data writes as much as
possible.
This new mode only works on the server side of a sync, where the
engine has enough control over the data flow.
Most of the changes are in libsynthesis. SyncEvolution only needs to
enable the new mode, which is done via an extension of the "sync"
property:
- "local-cache-incremental" will do an incremental sync (if possible)
or a slow sync (otherwise). This is usually the right mode to use,
and thus has "local-cache" as alias.
- "local-cache-slow" will always do a slow sync. Useful for
debugging or after (accidentally) making changes on the server side.
An incremental sync will ignore such changes because they are not
meant to happen and thus leave client and sync out-of-sync!
Both modes are recorded in the sync report of the local side. The
target side is the client and records the normal "two-way" or "slow"
sync modes.
With the current SyncEvolution contact field list, first, middle and
last name are used to find matches during any kind of slow sync. The
organization field is ignored for matching during the initial slow
sync and used in all following ones. That's okay, the difference won't
matter in practice because the initial slow sync in PBAP caching will
be done with no local data. The test achieve the same result in both
cases by keeping the organization set in the reduced data set.
It's also okay to include the property in the comparison, because it
might help to distinguish between "John Doe" in different companies.
It might be worthwhile to add more fields as match criteria, for
example the birthday. Currently they are excluded, probably because
they are not trusted to be supported by SyncML peers. In caching mode
the situation is different, because all our data came from the peer.
The downside is that in cases where matching has to be done all the
time because change detection is not supported (PBAP), including the
birthday as criteria will cause unnecessary contact removed/added
events (and thus disk IO) when a contact was originally created
without birthday locally and then a birthday gets added on the phone.
Testing is done as part of the D-Bus testing framework, because usually
this functionality will be used as part of the D-Bus server and writing
tests in Python is easier.
A new test class "TestLocalCache" contains the new tests. They include
tests for removing extra items during a slow sync (testItemRemoval),
adding new client items under various conditions (testItemAdd*) and
updating/removing an item during incremental syncing
(testItemUpdate/Delete*). Doing these changes during a slow sync could
also be tested (not currently covered).
The tests for removing properties (testPropertyRemoval*) cover
removing almost all contact properties during an initial slow sync, a
second slow sync (which is treated differently in libsynthesis, see
merge=always and merge=slowsync), and an incremental sync.
synccompare on the target side of a local sync was invoked with its
output being redirected via an unreliable socket to the local sync
parent. When the output was large, some of it might have been lost.
Fixed by reading the output locally in the syncevo-local-sync process
with reliable output redirection and sending it to the parent via
D-Bus. Execute() does that when both stdout and stderr are redirected
via LogRedirect, which makes sense also for other stdout output.
Instead of using delays to kill processes at the right time, watch old
and new debug output via D-Bus and then kill the processes. To avoid
race conditions, these processes get delayed at the right point.
Added tests for local sync and command line, covering killing of
all involved processes.
The helper process only detected that the parent failed when
it tried to log something while the parent had already shut down
the D-Bus connection. Even that did not work reliably and differed
between D-Bus libdbus and GIO.
Now logging ignores failures to send the message (done the same way
as in the syncevo-dbus-helper -> syncevo-dbus-server communication).
Parent failures are detected via the ForkExecChild::m_onQuit signal.
They set the "abort" state permanently until the helper process
terminates.
Found while testing some new failure test cases in combination with
D-Bus libdbus.
An asynchronous D-Bus call will invoke the callback even if
the call instance itself was already deleted. Therefore binding
the this pointer of the call instance owner is not safe. This
caused use-after-free errors in local sync (found during testing).
Add a weak pointer to itself to each LocalTransportAgent
instance and use that weak pointers so that the callback
does not crash when it happens to late.
Added a "target side of local sync ready" INFO message to introduce
the output which has the target context in the [INFO] tag. The sync report
from the target side now has the target context embedded in brackets
after the "Changes applied during synchronization" header, to avoid
ambiguities.
Output from the target side of a local sync was passed through stderr
redirection as chunks of text to the frontends. This had several
drawbacks:
- forwarding only happened when the local sync parent was processing
the output redirection, which (due to limitations of the implementation)
only happens when it needs to print something itself
- debug messages were not forwarded
- message boundaries might have been lost
In particular INFO messages about delays on the target side are
relevant while the sync runs and need to be shown immediately.
Now the output is passed through D-Bus, which happens immediately,
preserves message boundaries and is done for all output. The frontend
can decide separately whether it shows debug messages (not currently
supported by the command line tool).
Implementing this required extending the D-Bus API. The
Server.LogOutput signal now has an additional "process name"
parameter. Normally it is empty. For messages originating from the
target side, it carries that extra target context string.
This D-Bus API change is backward compatible. Older clients can still
subscribe to and decode the LogOutput messages, they'll simply ignore
the extra parameter. Newer clients expecting that extra parameter
won't work with an older D-Bus daemon: they'll fail to decode the
D-Bus message.
This revealed that the last error messages in a session was
incorrectly attributed to the syncevo-dbus-server. Might also have
happened with several other error messages. Now everything happening
in the server while working on code related to a session is logged as
coming from that sessions. It's not perfect either (some of the output
could be from unrelated events encountered indirectly while running
that code), but it should be better than before.
The patch changes the handling or errors in the local sync parent
slightly: because it logs real ERROR messages now instead of plain
text, it'll record the child's errors in its own sync report. That's
okay, user's typically shouldn't have to care about where the error
occurred. The D-Bus tests need to be adapted for this, because it
removes the "failure in local sync child" from the sync report.
Another, more internal change is that the syncevo-local-sync helper
does its own output redirection instead of relying on the stderr
handling of the parent. That way libneon debug output ends up in the
log file of the child side (where it belongs) and not in the parent's
log.
When waiting for resending a failed message, the sleeping couldn't be
interrupted when using the D-Bus server. It now uses the SuspendFlags
infrastructure and glib, which detects abort requests sent via D-Bus
in addition to those sent via signals (which already worked earlier).
Somehow syncevo-local-sync got stuck waiting for its parents reply to
a D-Bus method call when the parent had already quit. The expected
error response did not occur in this case.
Fixed by watching whether the parent is around while waiting for the
response. Also got rid of the event loop start/stop in favor of
g_main_context_iteration(). That is more explicit about the reason for
stopping the waiting.
KWallet support was broken: syncevo-dbus-server checked
KDE_FULL_SESSION to determine whether it should use KWallet instead of
GNOME Keyring. That did not work, because the env variable was not set
for D-Bus daemons.
Automatically detecting KDE users is not possible at the
moment. Instead KDE users have to manually set the new "keyring"
global config property to "KDE" (case insensitive) if the
SyncEvolution installation supports both, because GNOME Keyring is the
default to avoid surprises for traditional users. If only KWallet
support is enabled, then this is not necessary.
"GNOME" and "true/false/1/0/yes/no" can also be set. This has the
advantage that keyring usage can be enabled permanently for the
command line in --daemon=no mode; normally keyrings are not used in
that mode because accessing them can bring up UI dialogs.
It also becomes possible to disable keyring usage in syncevo-dbus-server,
something which couldn't be done before.
The --keyring command line option is still supported, as an alias for
"[--sync-property] keyring=<value>". The default value for --keyring
is true, to match the traditional behavior. In contrast to other sync
properties, setting "keyring" does not require an explicit --run
parameter. Again this is done to mirror traditional usage.
Reading a password also (unintentionally) checked all supported
storages while searching for the password. Now it uses exactly
one storage and falls back to asking for the password directly.
The commit itself also cleans up the code a bit (reformatted, fixed
comments). Choosing the right slot in the password signals is done via
a new InitStateTri parameter which contains the "keyring" setting.
Error checking (unsupported keyring string, --keyring=yes and no
keyring enabled) is done in additional slots which run after all the
regular ones.
Parameter parsing for --sync and --keyring were unified. However,
there is the difference that --keyring has an implicit default value
("yes") and never has an additional parameter, in contrast to --sync,
which always is followed by one.
The new CmdlineTest::testKeyring covers different ways of using
--keyring. It relies on actually invoking keyring backends, something
not done by the default SyncContext UI. Therefore
CmdlineSyncClient+KeyringSyncCmdline were moved into libsyncevolution,
to be used by CmdlineTest.
A failure to obtain the password must be communicated back to the caller.
Include as much information in the error message as possible.
Avoid reporting this as a generic transport error in the parent.
"Comparison impossible" was incorrectly printed after a successful
comparison on the target side of local sync.
The reason is that SIGCHLD SIG_IGN prevented reading the synccompare
return code. That code was probably added to prevent zombies when
syncevo-local-sync dies before it reads the return code of its
children.
Removing the SIGCHLD SIG_IGN fixes synccompare output issue. A test
for the sync output will be committed next - the issue was found while
writing the test.
Code for handling zombies still needs to be added to all long-running
process (client-test, syncevo-dbus-server).
All user-facing functions (password handling, reading from
stdin) are now in a dedicated "UserInterface" class, instead of
spreading that between ConfigUserInterface and SyncContext.
SyncContext no longer has the misleading "is a" [Config]UserInterface]
relationship. Instead it "has a" UserInterface instance, which is set
at runtime. Long term the plan is remove the need to subclass
SyncContext. In the local sync that was already possible
(LocalTransportContext->LocalTransportUI).
The guarantee that there always is a usable instance that can be used
without checking for NULL is provided by the getUserInterfaceNonNull()
method, which falls back to a dummy instance if necessary.
Reading data and password from stdin is moved out of the core
libsyncevolution into the syncevolution binary. That way the D-Bus
server and client-test do not accidentally attempt to read from stdin
(has happened when setting up testing incorrectly).
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.
Added debug logging to valgrindcheck.sh, suspend flags, fork/exec
and main() functions. Helps with tracing where signal handlers
are installed and which processes run.
The parent always reported a "remote transport error" when the child
didn't finish as expected. In cases where we know what the problem
was, because the child sent a report, using that status also on the
server side gives more useful information to the user.
For example, the overall sync status is now "unexpected slow sync
(local, status 22000)" after an unexpected slow sync instead of
logging a transport error there.
Shutting down syncevo-local-sync in a timely manner when
aborting is hard: the process might be stuck in a blocking
call which cannot be made to check the abort request (blocking
libneon, activesyncd client library, ...).
The best that can be done is to let the process be killed by the
SIGTERM. To have some trace of that, catch the signal and log the
signal; there's a slight risk that the logging system is in an
inconsistent state, but overall that risk is minor.
Because syncevo-local-sync catches SIGINT, ForkExec::stop() must send
SIGTERM in addition to SIGINT. To suppress redundant and misleading
ERROR messages when the bad child status is handled, the
ForkExecParent remembers that itself asked the child to stop and only
treats unexpected "killed by signal" results as error.
The local transport must call that stop() in its cancel(). It enters
the "canceled" state which prevents all further communication with the
child, in particular waiting for the child sync report; doing that
would produce another redundant error message about "child exited
without sending report".
Calling stop() in the local transport's shutdown() is no longer
possible, because it would kill the child right away. Before it simply
had no effect, because SIGINT was ignored. This points towards an
unsolved problem: how long should the parent wait for the child after
the sync is done? If the child gets stuck hard after sending its last
message, the parent currently waits forever until the user aborts.
In the sync event loop the caller of the transport must recognize
CANCELED as something which might be desired and thus should not be
logged as ERROR. That way the Synthesis engine is called one more time
with STEPCMD_ABORT also in those cases where the transport itself
detected the abort request first.
Having the signal handling code in SyncContext created an unnecessary
dependency of some classes (in particular the transports) on
SyncContext.h. Now the code is in its own SuspendFlags.cpp/h files.
Cleaning up when the caller is done with signal handling is now part
of the utility class (removed automatically when guard instance is
freed).
The signal handlers now push one byte for each caught signal into a
pipe. That byte tells the rest of the code which message it needs to
print, which cannot be done in the signal handlers (because the
logging code is not reentrant and thus not safe to call from a signal
handler).
Compared to the previous solution, this solves several problems:
- no more race condition between setting and printing the message
- the pipe can be watched in a glib event loop, thus removing
the need to poll at regular intervals; polling is still possible
(and necessary) in those transports which do not integrate with
the event loop (CurlTransport) while it can be removed from
others (SoupTransport, OBEXTransport)
A boost::signal is emitted when the global SuspendFlags change.
Automatic connection management is used to disconnect instances which
are managed by boost::shared_ptr. For example, the current transport's
cancel() method is called when the state changes to "aborted".
The early connection phase of the OBEX transport now also can be
aborted (required cleaning up that transport!).
Currently watching for aborts via the event loop only works for real
Unix signals, but not for "abort" flags set in derived SyncContext
instances. The plan is to change that by allowing a "set abort" on
SuspendFlags and thus making
SyncContext::checkForSuspend/checkForAbort() redundant.
The new class is used as follows:
- syncevolution command line without daemon uses it to control
suspend/abort directly
- syncevolution command line as client of syncevo-dbus-server
connects to the state change signal and relays it to the
syncevo-dbus-server session via D-Bus; now all operations
are protected like that, not just syncing
- syncevo-dbus-server installs its own handlers for SIGINT
and SIGTERM and tries to shut down when either of them
is received. SuspendFlags then doesn't activate its own
handler. Instead that handler is invoked by the
syncevo-dbus-server niam() handler, to suspend or abort
a running sync. Once syncs run in a separate process, the
syncevo-dbus-server should request that these processes
suspend or abort before shutting down itself.
- The syncevo-local-sync helper ignores SIGINT after a sync
has started. It would receive that signal when forked by
syncevolution in non-daemon mode and the user presses
CTRL-C. Now the signal is only handled in the parent
process, which suspends as part of its own side of
the SyncML session and aborts by sending a SIGTERM+SIGINT
to syncevo-local-sync. SIGTERM in syncevo-local-sync is
handled by SuspendFlags and is meant to abort whatever
is going on there at the moment (see below).
Aborting long-running operations like import/export or communication
via CardDAV or ActiveSync still needs further work. The backends need
to check the abort state and return early instead of continuing.
Instead of forking and continuing to sync in the forked process
without an explicit exec(), exec() the 'syncevo-local-sync' helper in
the forked process. The syncevo-local-sync helper binary gets
installed into libexec. SYNCEVOLUTION_LIBEXEC_DIR must be set if that
helper is not installed yet, not in the PATH, or an old version would
be found without that env variable ("make" without "make install"
during development!).
Main advantage is the cleaner environment for running the child side
of local sync. Required for getting ActiveSync to work again (GDBus
GIO as used by recent activesyncd client libraries did not work in the
forked process without the exec()).
Full D-Bus communication gets established between parent and child.
The downside is the hard dependency of local sync on the D-Bus
libraries (not the daemon!).
D-Bus communication allowed implementing interactive password requests
from the child side through the parent to the UI using the parent,
something that wasn't implemented before.
The child asks its parent for the password, which in turn
passes the request to its SyncContext. This happens to work
when that SyncContext is a normal instance (reads from stdin,
the "syncevolution --daemon" case) and the syncevo-dbus-server
(sends out an Info Request signal and waits for a response).
The info request and response are handled in the blocking
askPassword() by polling the running mail loop, so the parent should
remain responsive. Overall it is still a pretty difficult setup; it
would be better if askPassword() was asynchronous.
Describing the required password also is sub-optimal: the sync-ui just
asks for a password in its current config (even though that might not
be the config which currently gets synced) and crashes if no config is
currently selected. The command line uses the description derived from
the property and config name, which is a bit technical, but at least
correct.
Syncing uses the child's error string as "first error" in the parent,
too, by logging it anew on the parent side. That puts it into the
parent's sync report ahead of any additional error that it might
encounter while the child shuts down. Also use the child's status when
available instead of a misleading TransportError.
In addition, suppress as many of these errors as possible when we know
already that the child reported an error in its sync report. Not all
"transport errors" are currently avoided that way, but this is at
least a first step.
The -from-client/server sync modes are confusing because the direction
of the data exchange depends on which side acts as SyncML server or
client.
This patch introduces new modes which use -from-local/remote
instead. The statistics and messages also use these variants now. The
old modes are still understood, but are declared as "not recommended"
in the documentation.
As discussed on the mailing list, "source-config" is ambiguous because
the "addressbook/calendar/..." configs are also called "source
configs".
Now the naming is "sync" config (for the config with syncURL=local://,
because it is used for syncing) and "target" config (because it is
used as target in a sync config's syncURL).
Rejected:
"local" config - because the databases are not necessarily local
"source" config - see above
"client" or "server" config - because both sides might use local data
and/or client/server could refer to the role
of the peer or the SyncML client/server model
used internally
Timeouts for the SyncML messages between master and child did not
make sense. In HTTP they are necessary to detect dead peers, but with
permanent pipes between both sides that isn't an issue.
Because the timeouts triggered incorrectly, this patch removes them by
never setting the m_timeoutSeconds member variable. For sending the
child's status report a fixed timeout of 5 minutes (the previous
default) continues to be used, just in case that something goes
horribly wrong (software bug) and sending the report somehow hangs
(which it shouldn't).
There still was a TODO in the code for handling "-" as password value.
No surprise, not having that implemented broke CalDAV sync in
syncevo-dbus-server because it would try to read the password from
stdin (the default in SyncContext).
Probably SyncContext shouldn't provide such an unsafe fallback, but
that's something for another patch.
This patch addresses the immediate problem by moving the
initialization of the SyncContext used by the child process into the
master process and adding the password checking directly afterwards
(LocalTransportAgent::start()). It runs in the main process
(syncevolution or syncevo-dbus-server) and uses the "request password"
method of the main sync context. Passwords are then stored
temporarily, so the same check doesn't have to ask for passwords again
in the child process.
Long term we'll need to rewrite the complete password handling...
The "source-config" peer in the target context used to be required for
local sync. Now it is optional. The reasoning is:
- For WebDAV, copying username/password from the main config may
already be enough to find the server.
- For local file sources, no sync properties ared needed
at all.
In both cases, "source-config" is used if available. That can be useful
to set loglevel separately, for example.
For SyncEvolution<->SyncEvolution synchronization there should be
no need to set the uri property of each source explicitly, because
typically it is the same as the source name.
With this patch, the source name is used as fallback. Because some
code needs to check whether the URI is set, getURI() returns the
setting directly (as before) whereas getURINonEmpty() always returns a
non-empty result with the name as fallback.
Logging calls might end up calling functions which modify errno.
Seen once where logging reported a "pipe error" and the following
exception a "resource temporarily unavailable".
Patrick Ohly
Ove Kåven
Krzesimir Nowak