This addresses two different warnings from Fedora Rawhide:
/srv/runtests/work/sources/syncevolution/src/syncevo/SyncContext.cpp: In member function 'std::string SyncEvo::XMLFiles::get(SyncEvo::XMLFiles::Category)':
/srv/runtests/work/sources/syncevolution/src/syncevo/SyncContext.cpp:2390:28: error: loop variable 'entry' of type 'const StringPair&' {aka 'const std::pair<std::__cxx11::basic_string<char>, std::__cxx11::basic_string<char> >&'} binds to a temporary constructed from type 'std::pair<const std::__cxx11::basic_string<char>, std::__cxx11::basic_string<char> >' [-Werror=range-loop-construct]
2390 | for (const StringPair &entry: m_files[category]) {
| ^~~~~
/srv/runtests/work/sources/syncevolution/src/syncevo/SyncContext.cpp:2390:28: note: use non-reference type 'const StringPair' {aka 'const std::pair<std::__cxx11::basic_string<char>, std::__cxx11::basic_string<char> >'} to make the copy explicit or 'const std::pair<const std::__cxx11::basic_string<char>, std::__cxx11::basic_string<char> >&' to prevent copying
This fails because StringPair has non-const members. By using "auto",
we get rid of the need to define and pick exactly the right type.
/srv/runtests/work/sources/syncevolution/src/syncevo/SyncConfig.cpp: In member function 'void SyncEvo::SyncConfig::removeSyncSource(const string&)':
/srv/runtests/work/sources/syncevolution/src/syncevo/SyncConfig.cpp:2552:36: error: loop variable 'peer' creates a copy from type 'const string' {aka 'const std::__cxx11::basic_string<char>'} [-Werror=range-loop-construct]
2552 | for (const std::string peer: m_tree->getChildren(m_contextPath + "/peers")) {
| ^~~~
/srv/runtests/work/sources/syncevolution/src/syncevo/SyncConfig.cpp:2552:36: note: use reference type to prevent copying
2552 | for (const std::string peer: m_tree->getChildren(m_contextPath + "/peers")) {
| ^~~~
| &
We could have used "auto" also instead of "std::string", but here it
doesn't save that much typing and is more readable. We just have to
use a reference.
Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
Having to specify the type of an iterator is annoying and does not
really add clarity to the code. With C++11 we can use "auto"
instead and in some cases remove helper typedefs.
Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
NULL is ambiguous (can be integer and pointer) and using it as
terminator for vararg list of pointers without explicit casting to a
pointer was downright incorrect. nullptr fixes that.
Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
We can use std::shared_ptr and std::function instead now.
Lambdas are usually a better alternative to boost/std::bind. The
downside is the need to explicitly specify parameters completely. When
inlining callbacks entirely with lambdas, duplication of that
parameter list can be avoided.
Whenever possible, use std::make_shared to construct objects that are
tracked by std::shared_ptr.
Some objects need a std::weak_ptr during object destruction. For that
we have to use our own implementation of std::enable_shared_from_this,
with a matching creator function. The additional benefit is that we
can get rid of explicit static "create" methods by making that create
function a friend.
Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
boost/foreach.hpp is no longer needed, range-based loops work
the same. With some helpers, even reverse iteration and
boost::make_split_iterator() can be handled the same way.
"auto" makes it possible to avoid explicitly spelling out the
expected type.
Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
So far, only the "username" property was used once identity providers
were involved. The upcoming oauth2 provider uses the "password"
property for the refresh token and needs the ability to store a new
token if the OAuth2 server updates it.
Setting the new value will not always be possible (for example,
when running a command line operation where all properties were
provided on the command line without a permanent config). This still
needs to be handled.
Encoding/decoding of the uint8_t array in D-Bus took a surprisingly
large amount of CPU cycles relative to the rest of the SyncML message
processing. Now the actual data resides in memory-mapped temporary
files and the D-Bus messages only contain offset and size inside these
files. Both sides use memory mapping to read and write directly.
For caching 1000 contacts with photos on a fast laptop, total sync
time roughly drops from 6s to 3s.
To eliminate memory copies, memory handling in libsynthesis or rather,
libsmltk is tweaked such that it allocates the buffer used for SyncML
message data in the shared memory buffer directly. This relies on
knowledge of libsmltk internals, but those shouldn't change and if they
do, SyncEvolution will notice ("unexpected send buffer").
Unusually small values were ignored during syncing. Doing this in the config
access methods instead of the sync session setup ensures that all components
in SyncEvolution use the same value.
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.
When configuring a WebDAV server with username = email address and no
URL (which is possible if the server supports service discovery via
the domain in the email address), then storing the credentials in the
GNOME keyring used to fail with "cannot store password in GNOME
keyring, not enough attributes".
That is because GNOME keyring seemed to get confused when a network
login has no server name and some extra safeguards were added to
SyncEvolution to avoid this.
To store the credentials in the case above, the email address now gets
split into user and domain part and together get used to look up the
password.
Funambol turned of the URL redirect from my.funambol.com to
onemedia.com. The Funambol template now uses the current URL. Users
with existing Funambol configs must updated the syncURL property
manually to https://onemediahub.com/sync
Kudos to Daniel Clement for reporting the change.
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 configuring a local sync on the target side, the code in
SyncConfig::getSyncSourceNodes() created redundant ConfigNode instances. This
was pointed out by cppcheck as redundant variable assignments. The code which
created these additional instances may have had side-effects, but it doesn't
look like these were desired and/or relevant.
The interval is measured from the start of last sync. The rationale for that
is that it makes the time when syncs run independent from the duration of each
sync, which is more predictable.
The implementation already works like that, it just was not clear from the
property description.
The command line tool in --daemon=no mode did not use the GNOME
keyring or KWallet even if the syncevo-dbus-server did, leading
to failing test cases when actually starting to use it by default
there.
Now all components use the same default: use safe password storage if
any was enabled during compilation, don't use if not.
This also makes SyncEvolution work without user intervention on
systems without a password storage.
Figuring out where credentials come from became harder. These debug
messages help. Perhaps they should even be logged as INFO messages
such that normal users can see them?
The real username is only relevant when running a sync. When looking
at a config with a D-Bus client like the GTK UI, the username should
always be "id:<config>", to avoid accidentally removing the
indirection, while the password should be the real one, to allow the
user to edit like he normally would with passwords stored in a
keyring.
To achive this, overriding the username must be suppressed when
resolving as part of the D-Bus config API. While at it, move the
entire "iterate over properties" into a common utility function in
PasswordConfigProperty.
Storing a password with just "user=foo" as lookup attributes is problematic
because it is too unspecific. Different services or configs with the same
user, but different passwords end up overwriting each other's passwords. In
practice, the config with "user=foo" even had the effect of removing the entry
for "user=foo server=bar".
The situation can be avoided by using the remotePeerId as fallback when the
syncURL is empty. There is a (minor?) risk that some configs were stored
in the past without that additional key and now won't be found anymore in the
keyring. Users need to re-set such passwords.
If an attempt is made to store a password with insufficient lookup attributes,
GNOME keyring will now reject the attempt.
When instantiating multiple SyncConfig instances, it is important that
they share filter nodes and the underlying tree, because the nodes
store copies of already retrieved credentials (lookup shall only be
done once!) and the trees represent the current content of the config
(which must be consistent when making changes).
Currently the new code is not thread-safe, but nor are nodes and trees,
so a lot more work would be needed to make this safe. Instead we avoid
concurrency.
SyncConfigTest::normalize() only passed because FileConfigTree accidentally
created the "peers" directory inside the peer. That will change, so don't rely
on that. Instead ensure that the config.ini file of the peers gets written
because it contains something.
The previous approach made FileConfigTree more complex than necessary.
Having an abstract ConfigTree::getRootPath() with undefined behavior
is bad design.
The code was had undesiredable side effects: inside a peer config,
another "peers" directory was created because FileConfigTree didn't
know whether creating that dir was required or not.
Now all of the complexity is in SyncConfig, which arguably knows
better what the tree stands for and how to use
it.
In practice, the methods are always called for a specific SyncConfig.
Passing that allows removing several other parameters and, more
importantly, also grants access to the config and through that other
configs. This will be needed for the indirect credential lookup.
"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.
Passwords are cached after the initial check as temporary property
values. The explicit string members are obsolete and can be removed
together with the code using them.
The real username is only relevant when running a sync. When looking
at a config with a D-Bus client like the GTK UI, the username should
always be "id:<config>", to avoid accidentally removing the
indirection, while the password should be the real one, to allow the
user to edit like he normally would with passwords stored in a
keyring.
To achive this, overriding the username must be suppressed when
resolving as part of the D-Bus config API. While at it, move the
entire "iterate over properties" into a common utility function in
PasswordConfigProperty.
A SHARED_LAYOUT config tree caches config nodes. Allow a second config
to use those same nodes as an already existing config. This will be
useful in combination with indirect password lookup, because then the
credentials can be stored as temporary property values and be reused
when used multiple times in a process (for example, by CardDAV and by
CalDAV).
In practice, the methods are always called for a specific SyncConfig.
Passing that allows removing several other parameters and, more
importantly, also grants access to the config and through that other
configs. This will be needed for the indirect credential lookup.
"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.
Passwords are cached after the initial check as temporary property
values. The explicit string members are obsolete and can be removed
together with the code using them.
The synthesisID value is required for each Synthesis source progress
event, which can be fairly frequent (more than one per item). Instead
of going down to the underlying .ini config node each time, cache the
value in the SyncSourceConfig layer.
HTTP SyncML clients give up after a certain timeout (SyncEvolution
after RetryDuration = 5 minutes by default, Nokia e51 after 15
minutes) when the server fails to respond.
This can happen with SyncEvolution as server when it uses a slow
storage with many items, for example via WebDAV. In the case of slow
session startup, multithreading is now used to run the storage
initializing in parallel to sending regular "keep-alive" SyncML
replies to the client.
By default, these replies are sent every 2 minutes. This can be
configured with another extensions of the SyncMLVersion property:
SyncMLVersion = REQUESTMAXTIME=5m
Other modes do not use multithreading by default, but it can be
enabled by setting REQUESTMAXTIME explicitly. It can be disabled
by setting the time to zero.
The new feature depends on a libsynthesis with multithreading enabled
and glib >= 2.32.0, which is necessary to make SyncEvolution itself
thread-safe. With an older glib, multithreading is disabled, but can
be enabled as a stop-gap measure by setting REQUESTMAXTIME explicitly.
Some peers have problems with meta data (CtCap, old Nokia phones) and
the sync mode extensions required for advertising the restart
capability (Oracle Beehive).
Because the problem occurs when SyncEvolution contacts the peers
before it gets the device information from the peer, dynamic rules
based on the peer identifiers cannot be used. Instead the local config
must already disable these extra features in advance.
The "SyncMLVersion" property gets extended for this. Instead of just
"SyncMLVersion = 1.0" (as before) it now becomes possible to say
"SyncMLVersion = 1.0, noctcap, norestart".
"noctcap" disables sending CtCap. "norestart" disables the sync mode
extensions and thus doing multiple sync cycles in the same session
(used between SyncEvolution instances in some cases to get client and
server into sync in one session).
Both keywords are case-insensitive. There's no error checking for
typos, so beware!
The "SyncMLVersion" property was chosen because it was already in use
for configuring SyncML compatibility aspects and adding a new property
would have been harder.
In the previous attempt (commit a0375e) setting the <syncmodeextensions>
option was only done on the client side, thus breaking multi-cycle
syncing with SyncEvolution as server. On the server side the option
shouldn't be needed (the server never sends the extensions unless
the client did first), but for symmetry and testing reasons it makes
sense to also offer the options there.
This reverts commit 3a71a2bf53 and
commit a0375e0160.
Need to back out the workaround for broken peers because it breaks
SyncEvolution<->SyncEvolution syncing.
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.
Some peers have problems with meta data (CtCap, old Nokia phones) and
the sync mode extensions required for advertising the restart
capability (Oracle Beehive).
Because the problem occurs when SyncEvolution contacts the peers
before it gets the device information from the peer, dynamic rules
based on the peer identifiers cannot be used. Instead the local config
must already disable these extra features in advance.
The "SyncMLVersion" property gets extended for this. Instead of just
"SyncMLVersion = 1.0" (as before) it now becomes possible to say
"SyncMLVersion = 1.0, noctcap, norestart".
"noctcap" disables sending CtCap. "norestart" disables the sync mode
extensions and thus doing multiple sync cycles in the same session
(used between SyncEvolution instances in some cases to get client and
server into sync in one session).
Both keywords are case-insensitive. There's no error checking for
typos, so beware!
The "SyncMLVersion" property was chosen because it was already in use
for configuring SyncML compatibility aspects and adding a new property
would have been harder.
The new InitState template works for both classes and generic types.
The difference is in the kind of base class that it inherits from: the
one for classes "is" an instance of the wrapped type, the other "has"
such an instance. boost::is_class is used to pick the right utility
class, something that had to be done manually previously.
While touching the classes, the explicit initialization with "const
char *" was replace with a more general template constructor.
The main advantage is that other template code, like the D-Bus traits,
no longer needs to distinguish between InitState and
InitStateClass. The InitState implementation did not become
smaller. At least it avoids code duplication (m_wasSet handled in one
place).
Previously, if http_proxy was set, a proxy was used even if
explicitly disabled. This prevented disabling the use of a proxy
which only made sense in some cases, like accessing something
that runs locally. Explicitly telling SyncEvolution to ignore
http_proxy is necessary because it doesn't support no_proxy.
This change became necessary in the new nightly test server,
which depends on http_* env vars for communication with external
servers.
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.
The old explanation made it sound like nothing would get deleted by
default ("If set, ..."). That's not correct, by default only 10
sessions are kept.
Also explain the behavior of deleting intermediate sessions first.
Google does not implement refresh-from-server, therefore using it has
to be made configurable. It is enabled by default for Funambol and
disabled for everything else.
Existing user configs must be updated to use refresh-from-server with
Funambol:
syncevolution --configure enableRefreshSync=1 funambol
Due to the way how properties were handled internally, it wasn't
possible to explicitly set a property to its default value. Instead
the property was unset. For example, explicitly setting database= was
not possible.
This is necessary for client-test and ActiveSync, because client-test
needs to know that the testing is expected to run with the default
databases (something which normally is avoided by overwriting empty
database properties).
Now the "is set" state is tracked explicitly in the config storage and
command line property APIs. Unsetting a property via the command line
could be implemented with an explicit command line option, but is not
supported at the moment.
Tests were extended to cover the new functionality and adapted to the
change behavior for "type" migration: syncFormat was empty
already (because the empty string matched the default), but
forceSyncFormat was unnecessarily set explicitly. Now it is not.
Patrick Ohly
Mateusz Polrola
patrick.ohly@intel.com