The Synthesis API does not say explicitly, but in practice all map
items get updated in a tight loop. Rewriting the m_mappingNode (case
insensitive string comparisons) and serialization to disk
(std::ostrstream) consume a significant amount of CPU cycles and cause
extra disk writes.
It is better to allow changes to be cached in memory instead and only
flush to disk at the end. There is no explicit flush API, so we use
SaveAdminData as trigger for flushing instead.
In a refresh-from-remote local sync of 1000 contacts with photo data
using the file backend, this reduces the number of CPU cycles from
4138538276 to 2861476184 (measured with callgrind on x86_64).
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.
For this to work, the memory must be mapped MAP_SHARED (otherwise
writes are not visible outside of the process) and the recipient
of a file descriptor must be able to create a TmpFile for it, which
then can be used to create a memory mapping.
During TestCmdline.testSyncOutput also verify that syncs which don't
change the database also don't update the ~/.config meta data.
The listall() method from testpim.py is used and extended for that
and therefore moved to test-dbus.py.
The sync meta data (sync anchors, client change log) get updated after
a sync even if nothing changed and the existing meta data could be
used again. This can be skipped for local sync, because then
SyncEvolution can ensure that both sides skip updating the meta
data. With a remote SyncML server that is not possible and thus
SyncEvolution has to update its data.
This optimization is only used for local syncs with one source. It is
based on the observation that when the server side calls
SaveAdminData, the client has sent its last message and the sync is
complete. At that point, SyncEvolution can check whether anything has
changed and if not, skip saving the server's admin data and stop the
sync without sending the real reply to the client.
Instead the client gets an empty message with "quitsync" as content
type. Then it takes shortcuts to close down without finalizing the
sync engine, because that would trigger writing of meta data
changes. The server continues its shutdown normally.
This optimization is limited to syncs with a single source, because
the assumption about when aborting is possible is harder to verify
when multiple sources are involved.
During syncs where nothing changes, reseting the database revision and
setting it again at the end of the sync causes undesirable disk
writes. To avoid that, reset the database revision the first time an
item change is done.
Sometimes the new content of the file is exactly the same as the old
one, for example when the tracking node gets reset and
recreated. Detect that for all cases at the lower level by reading the
existing file first and comparing it against the new content.
Synthesis cleanup was supposed to happen between "closing session" and
"session closed" log messages. Because we kept additional references
to the session, this only happened later. Need to drop also references
to shared buffers, because those hold a reference to their session.
This allows signal handlers to affect the operation execution and
result without having to throw an exception, which would get logged as
error and is not desirable when the operation gets skipped
intentionally. Needed for skipping SaveAdminData in the next patch.
The engine gets instantiated twice: once for logging, once for
syncing. The config generated in each case is different because
sources only become active for syncing. Therefore the config change
check unnecessarily triggered during each sync (first getConfigXML
call created one hash, then the second another), causing DevInf to be
sent unnecessarily and always rewriting the .ini file.
The check is only needed when instantiating the engine that is getting
used for syncing. However, the config date must always be valid.
This adds "protocol: CardDAV" as a valid value, with corresponding
changes to the interpretation of some existing properties and some new
ones. The API itself is not changed.
Suspending a CardDAV sync is possible. This freezes the internal
SyncML message exchange, so data exchange with the CardDAV server may
continue for a while after SuspendPeer().
Photo data is always downloaded immediately. The "pbap-sync" flag
in SyncPeerWithFlags() has no effect.
Syncing can be configured to be one-way (local side is read-only
cache) or two-way (local side is read/write). Meta data must be
written either way, to speed up caching or allow two-way syncing. The
most common case (no changes on either side) will have to be optimized
such that existing meta data is not touched and thus no disk writes
occur.
A SuspendPeer() only succeeded while the underlying Bluetooth transfer
was active. Outside of that, Bluez errors caused SyncEvolution to
attempt a cancelation of the transfer and stopped the sync.
When the transfer was still queueing, obexd returns
org.bluez.obex.Error.NotInProgress. This is difficult to handle for
SyncEvolution: it cannot prevent the transfer from starting and has to
let it become active before it can suspend the transfer. Canceling
would lead to difficult to handle error cases (like partially parsed
data) and therefore is not done.
The Bluez team was asked to implement suspending of queued transfers
(see "org.bluez.obex.Transfer1 Suspend/Resume in queued state" on
linux-bluetooth@vger.kernel.org), so this case might not happen
anymore with future Bluez.
When the transfer completes before obexd processes the Suspend(),
org.freedesktop.DBus.Error.UnknownObject gets returned by
obexd. SyncEvolution can ignore errors which occur after the active
transfer completed. In addition, it should prevent starting the next
one. This may be relevant for transfer in chunks, although the sync
engine will also stop asking for data and thus typically no new
transfer gets triggered anyway.
When syncevo-dbus-server was shutting down due to a signal and there
was a sync session running that was triggered via the PIM Manager API,
then a segfault occurred because the manager instance got destroyed
before invoking a Session callback holding a pointer to it.
The fix is to use slot tracking of the weak pointer.
CTRL-C while waiting for the end of a sync causes an interactive
prompt to appear where one can choose been suspend/resume/abort and
continuing to wait. CTRL-C again in the prompt aborts the script.
The help text used single quotes for the JSON example instead of
the required double quotes. Running without --sync-flags was broken
because of trying to parse the empty string as JSON.
When configuring a new sync config, the command line checks whether a
datastore is usable before enabling it. If no datastores were listed
explicitly, only the usable ones get enabled. If unusable datastores
were explicitly listed, the entire configure operation fails.
This check was based on listing databases, which turned out to be too
unspecific for the WebDAV backend: when "database" was set to some URL
which is good enough to list databases, but not a database URL itself,
the sources where configured with that bad URL.
Now a new SyncSource::isUsable() operation is used, which by default
just falls back to calling the existing Operations::m_isEmpty. In
practice, all sources either check their config in open() or the
m_isEmpty operation, so the source is usable if no error is
enountered.
For WebDAV, the usability check is skipped because it would require
contacting a remote server, which is both confusing (why does a local
configure operation need the server?) and could fail even for valid
configs (server temporarily down). The check was incomplete anyway
because listing databases gave a fixed help text response when no
credentials were given. For usability checking that should have
resulted in "not usable" and didn't.
The output during the check was confusing: it always said "listing
databases" without giving a reason why that was done. The intention
was to give some feedback while a potentially expensive operation
ran. Now the isUsable() method itself prints "checking usability" if
(and only if!) such a check is really done.
Sometimes datastores were checked even when they were about to be
configure as "disabled" already. Now checking such datastores is
skipped.
The log prefix seems to be unused (except for some debug message) at
the moment but will be soon, so make sure it gets embedded in the
string sent to the syncevo-dbus-server.
Will become relevant with the next commit, which moves the
source name from being part of the text into the prefix. So far,
the prefix is unused during Cmdline tests.
When deriving from DummySyncSource we need to implement less
pure virtual functions. Only the ones where the behavior
needs to differ from DummySyncSource are needed.
When syncing memos with a peer which also supports iCalendar 2.0 as
data format, the engine will now pick iCalendar 2.0 instead of
converting to/from plain text. The advantage is that some additional
properties like start date and categories can also be synchronized.
The code is a lot simpler, too, because the EDS specific iCalendar 2.0
<-> text conversion code can be removed.
When sending a VEVENT, DESCRIPTION was set to the SUMMARY if empty. This may
have been necessary for some peers, but for notes (= VJOURNAL) we don't know
that (hasn't been used in the past) and don't want to alter the item
unnecessarily, so skip that part and allow empty DESCRIPTION.
When receiving a plain text note, the "text/calendar+plain" type
used to store the first line as summary and the rest as description.
This may be correct in some cases and wrong in others.
The EDS backend implemented a different heuristic: there the first
line is copied into the summary and stays in the description. This
makes a bit more sense (the description alone is always enough to
understand the note). Therefore and to avoid behavioral changes
for EDS users when switching the EDS backend to use text/calendar+plain,
the engine now uses the same approach.
For this to work in all cases, the datatype must always call
MEMO_INCOMING_SCRIPT.
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.
Google has turned off their SyncML server, so the corresponding
"Google Contacts" template became useless and needs to be removed. It
gets replaced by a "Google" template which combines the three
different URLs currently used by Google for CalDAV/CardDAV.
This new template can be used to configure a "target-config@google"
with default calendar and address book database already enabled. The
actual URL of these databases will be determined during the first
sync using them.
The template relies on the WebDAV backend's new capability to search
multiple different entries in the syncURL property for databases. To
avoid listing each calendar twice (once for the legacy URL, once with
the new one) when using basic username/password authentication, the
backend needs a special case for Google and detect that the legacy URL
does not need to be checked.
The syncURL property may contain multiple different space or tab
separated URLs. Previously, the WebDAV backend only used the first one
when scanning for databases. Now it tries all of them.
This will be useful for configuring all Google endpoints in one
template.
Certains domains (like googleapis.com) are aliases. The lookup tools
do not fail with NXDOMAIN in this case, causing the
syncevo-webdav-lookup script to return only an unknown lookup error,
in which case the WebDAV backend will retry for a while.
We need to detect aliases resp. missing SRV information and return a
"no information error".
The asyncError relied on calling
folks_individual_aggregator_remove_individual() incorrectly
with a NULL pointer. folks in Debian Testing just crashes
now, so we can no longer do this.
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".
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.
When doing PBAP caching, we don't want any meta data written because
the next sync would not use it anyway. With the latest libsynthesis
we can configure "/dev/null" as datadir for the client's binfiles and
libsynthesis will avoid writing them.
The PIM manager uses this for PBAP syncing automatically. For testing
it can be enabled by setting the SYNCEVOLUTION_EPHEMERAL env variable.
The recent change which introduced checking of the target session only
worked if the entries were listed in the "right" (= target-config
entry last) order. Don't reply on that, instead check for the one new entry
that we should find and use that.
As pointed out by compiler warnings on recent distros, the write()
result in the SuspendFlags signal handler was ignored. Not sure
whether it really can fail in practice. Handle some
possible errors with retrying.
The Cmdline.cpp unit tests did not check the symlink() result,
causing a warning resp. error depending on the configure options.
Reported by Emiliano Heyns.
This reverts commit c435e937cd.
Commit 7b636720a in libsynthesis fixes an unitialized memory read in
the asynchronous item update code path.
Testing confirms that we can now used batched writes reliably with EDS
(the only backend currently supporting asynchronous writes +
batching), so this change enables it again also for local and
SyncEvolution<->SyncEvolution sync (with asynchronous execution of
contact add/update overlapped with SyncML message exchanges) and other
SyncML syncs (with changes combined into batches and executed at the
end of each message).
Instead of downloading contacts one-by-one with GET, SyncEvolution now
looks at contacts that are most likely going to be needed soon and
gets all of them at once with addressbook-multiget REPORT.
The number of contacts per REPORT is 50 by default, configurable by
setting the SYNCEVOLUTION_CARDDAV_BATCH_SIZE env variable.
This has two advantages:
- It avoids round-trips to the server and thus speeds up a large
download (100 small contacts with individual GETs took 28s on
a fast connection, 3s with two REPORTs).
- It reduces the overall number of requests. Google CardDAV is known
to start issuing intermittent 401 authentication errors when the
number of contacts retrieved via GET is too large. Perhaps this
can be avoided with addressbook-multiget.
This is similar to read-ahead in EDS contacts. However, because we
cannot run Neon requests asynchronously (at least not easily), a
batched read must complete before any contact from it can be
returned to the caller.
Arbitrary sync flags can be passed to SyncPeerWithFlags() with the
new --sync-flags command line parameter. The value of that parameter
must be a JSON formatted hash.
The advantage of this approach over explicit command line parameters
(like --progress-frequency) is that we do not need to add further code
when adding new flags. We can also pass intentionally broken flags
to check the PIM Managers error handling.
The downside is that it is a bit less user-friendly.
With this change, progress frequency and PBAP sync can be set in two
ways, either as part of --sync-flags or with the individual command
line parameters. The latter override --sync-flags.
When the value of a flag has the wrong type, then dumping the value
may provide some hint about what is wrong. Note that values of types
not expected at all by the SyncFlags type already get filtered out
already by our D-Bus binding, in which case the error message shows
no value.
syncevo-webdav-lookup is needed during testing when doing
WebDAV database scans, so build it in "src" like the rest
of the binaries and link to it in the installed test suite.
Google recently enhanced support for RECURRENCE-ID, so SyncEvolution
no longer needs to replace the property when uploading a single
detached event with RECURRENCE-ID. However, several things are still
broken in the server, with no workaround in SyncEvolution:
- Removing individual events gets ignored by the server;
a full "wipe out server data" might work (untested).
- When updating the parent event, all child events also get
updated even though they were included unchanged in the data
sent by SyncEvolution.
- The RECURRENCE-ID of a child event of an all-day recurring event
does not get stored properly.
- The update hack seems to fail for complex meetings: uploading them
once and then deleting them seems to make uploading them again
impossible.
All of these issues were reported to Google and are worked on there,
so perhaps the situation will improve. In the meantime, syncing with
Google CalDAV should better be limited to:
- Downloading a Google calendar in one-way mode.
- Two-way syncing of simple calendars without complex meeting
serieses.
While updating the Google workarounds, the alarm hack (sending a
new event without alarms twice to avoid the automatic server side
alarm) was simplified. Now the new event gets sent only once with a
pseudo-alarm.
The bugs caused by introducing merging of arrays were not detected
by the automated testing. We need to repeat syncing after we have
data and check that nothing changes in the default, no-phone mode,
and we need to check that nothing got changed on the remote side.
In PBAP mode that would have triggered an error, but when using the
file backend we silently modified its data.
Removing the URL ensures that the failure to avoid writes of
incomplete contacts gets covered. Rearranging the properties
also may be relevant (but was handled already).
Adding grouping to the contact datatype broke PBAP caching: when
sending an empty URL, for example, during the sync, the parsed contact
had different field arrays than the locally stored contact, because the
latter was saved without the empty URL.
This caused the field-based comparison to detect a difference even when
the final, reencoded contact wasn't different at all.
To solve this, syncing now uses the same "don't send empty properties"
configuration as local storages. Testing shows that this resolves
the difference for EDS.
A more resilient solution would be to add a check based on the encoded
data, but that's more costly performance wise.
The new "preserve repeating properties during conflict resolution"
feature was only active when using EDS as storage. The relevant
merge script must be applied to all datatypes, not just the EDS
flavor.
The feature was also unintentionally active when running in
caching mode. This caused two problems:
- The cached item was updated even though only the
ordering of repeating properties had been modified during
merging.
- The merged item was sent back to the client side, which
was undesirable (caching is supposed to be one-way) or even
impossible (PBAP is read-only, causing sync failures eith error 20030).
We must check for caching mode and disable merging when it is active.
We also must not tell the engine that we updated the photo property
in the winning item, because then that item would get sent to the
read-only side of the sync.
Perhaps a better solution would be to actually tell the engine
that the remote side is read-only when we activate caching mode.
When handling an update/update conflict (both sides of the sync have an
updated contact) and photo data was moved into a local file by EDS, the engine
merged the file path and the photo data together and thus corrupted the photo.
The engine does not know about the special role of the photo property.
This needs to be handled by the merge script, and that script did not
cover this particular situation. Now the loosing side is cleared,
causing the engine to then copy the winning side over into the loosing
one.
Found by Renato Filho/Canonical when testing SyncEvolution for Ubuntu 14.04.
If enabled via env variables, PullAll transfers will be limited to
a certain numbers contacts at different offsets until all data got
pulled. See README for details.
When transfering in chunks, the enumeration of contacts for the engine
no longer matches the PBAP enumeration. Debug output uses "offset #x"
for PBAP and "ID y" for the engine.
Using a pipe was never fully supported by obexd (blocks
obexd). Transfering in suitably sized chunks (FDO #77272) will be a
more obexd friendly solution with a similar effect (not having to
buffer the entire address book in memory).
Passing 0.1 as delay did not work as intended because it was converted
to an integer value of 0 seconds. Found by gcc 4.9. Must use a one second
delay.
The "func" variable was correctly initialized to NULL if no comparsion
matches, but cppcheck 1.65 warns anyway. Use the more readable
intialization to NULL in the final else clause.
folks_note_field_details_new() takes an additional uid parameter.
Passing NULL is okay, but SyncEvolution wasn't doing that due to
an incorrect function type cast. Found by valgrind only after
a valgrind and tool chain update. Probably we passed a valid
value accidentally before.
Fixed by using a wrapper function.
Ideally the function typecast should get replaced entirely with just casting
the returned pointer.
The parsed number always has a country code, whereas SyncEvolution expected it
to be zero for strings without an explicit country code. This caused a caller
ID lookup of numbers like "089788899" in DE to find only telephone numbers in
the current default country, instead of being more permissive and also finding
"+189788899". The corresponding unit test was broken and checked for the wrong
result. Found while investigating an unrelated test failure when updating
libphonenumber.
EDS handles this differently, by calling ParseAndKeepRawInput() if necessary
(checked by configure) and looking at the source of the country code. Instead
of replicating that logic, let's use EPhoneNumber. This means that EDS has to
be compiled with libphonenumber support, because SyncEvolution can no longer
fall back to using libphonenumber directly.
For testing purposes it is still useful to not depend on X-EVOLUTION-E164.
testLocaledPhone uses this at the moment, because re-generating
X-EVOLUTION-E164 during a locale change seems to be broken at the moment
in the intel-work-3-12 branch.
The test itself has to be updated for the newer libphonenumber (6.1.1 instead
of 5.3.2). The "12345" string it relied upon now gets parsed consistently in
US and DE. Instead we use the "01164 3 331 6005" string (as in libphonenumber
tests) which is treated differently.
The "Fixed compilation error when using libphonenumber from revision
>= 568" patch caused a double free error because SetLogger() owns the
logger instance and, with libphonenumber >= r571 actually frees the
instance.
Old libphonenumber release are compatible with the revised call,
however, they never free the instance.
Empty field filter is supposed to mean "return all supported
fields". This used to work and stopped working with Android phones
after an update to 4.3 (seen on Galaxy S3); now the phone only
returns the mandatory TEL, FN, N fields.
The workaround is to replace the empty filter list with the list of
known and supported properties. This means we only pull data we really
need, but it also means we won't get to see any additional properties
that the phone might support.
The new "signon" provider only depends on lib[g]signon-glib. It uses
gSSO if found, else UOA. Instead of pulling parameters and the
identity via libaccounts-glib, the user of SyncEvolution now has to
ensure that the identity exists and pass all relevant parameters
in the "signon:" username.
This does not have to be user-friendly, so the machine-readable
GVariant text dump format is used to pass all parameters.
gSSO >= 2.0 requires a list of realms to which the identity
applies. We take this list from a new "Realms" setting for the
provider in Accounts.
This is how SyncEvolution does it; it hasn't been checked what
upstream will do around this. In Tizen, libaccounts is not used
and thus a different solution is needed there.
The API of libgsignond >= 2.0 was also changed to be more compatible
with the original libsignon. This allows (and requires) removing
some gSSO ifdefs.
The calendar home set URL on iCloud (the one ending in /calendars/) is
declared as containing calendar data. That was enough for
SyncEvolution to accept it incorrectly as calendar. However, the home
set only contains calendar data indirectly.
We must use the stricter check for leaf collections containing the
right data.
When finding a new URL, we must be prepared to reinitialize the Neon
session with the new host settings. To implement this, candidates are
now full URIs, not just paths on the initial host.
A home set on iCloud contains full URLs, not just paths. We need to
parse the individual entries, which happens to work for paths and URLs
because paths are just special URLS without an explicit host.
iCloud does not have .well-known support on its www.icloud.com
server. To support lookup with a non-icloudd.com email address, we
must do DNS SRV lookup when access to .well-known URLs fails. We do
this without a www prefix on the host first, because that is what happens
to work for icloud.com.
With these changes it becomes possible to do database scans on Apple
iCloud, using syncURL=https://www.icloud.com or
syncURL=https://icloud.com. The former is a bit faster because the
icloud.com redirects to www.icloud.com before we end up doing the DNS
SRV lookup to find the CalDAV resp. CardDAV hosts.
Treat URI with explicit port as equal to an URI where the port number
is implied by the scheme. Add compare() operation similar to
std::string::compare and add full set of compare operators based on it.
501 means "not implemented", in which case resending the same request
is unlikely to succeed. This is relevant for scanning iCloud, because
PROPFIND on http://www.icloud.com returns that.
Apple iCloud servers reject requests unless they contain a User-Agent
header. The exact value doesn't seem to matter. Making the string
configurable might be better, but can still be done later when it
is more certain whether and for what it is needed.
Moved into a separate function and fixed return code handling: due to
missing decoding of the shell scripts return code, all errors were
treated as potentially temporary errors and thus lookup was retried
instead of giving up immediately when there is no DNS SRV entry.
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.
Traditionally, SyncEvolution only modified remote data via syncing.
This is not enough because it does not cover data on the remote side
that SyncEvolution cannot sync.
The new tests solve this by using the command line's import/update
operations which modify data more or less directly.
Now we can test:
- downloading data created remotely
- uploading data via sync, export directly, compare
- simulate conflicts with changes made remotely
The download and upload test include full round-trips, i.e.
the initial transfer plus a change that needs to be synced back.
Because this is highly dependent on the exact data stored by the
peer, all these tests depend on per-peer test data and scripts
for modifying that data. The tests only get enabled if the test
data is found.
The initial test data covers Apple Calendar server, EDS<->EDS
and Google Contacts.
When resolving a merge conflict, repeating properties were taken
wholesale from the winning side (for example, all email addresses). If
a new email address had been added on the loosing side, it got lost.
Arguably it is better to preserve as much data as possible during a
conflict. SyncEvolution now does that in a merge script by checking
which properties in the loosing side do not exist in the winning side
and copying those entries.
Typically only the main value (email address, phone number) is checked
and not the additional meta data (like the type). Otherwise minor
differences (for example, both sides have same email address, but with
different types) would lead to duplicates.
Only addresses are treated differently: for them all attributes
(street, country, city, etc.) are compared, because there is no single
main value.
When copying properties which may have labels, the new entries must be
added such that a unique label can be copied or set. Other properties
can be added at the end of their array.
The modern profile typically doesn't generate empty properties, and therefore
it is useless to create an IMPP, X-ABRELATEDNAMES or X-ABDATE entry with has
an empty value. It just has the effect of creating X-ABLabels which are not
attached to any property.
In principle, CardDAV servers support arbitrary vCard 3.0
data. Extensions can be different and need to be preserved. However,
when multiple different clients or the server's Web UI interpret the
vCards, they need to agree on the semantic of these vCard extensions.
In practice, CardDAV was pushed by Apple and Apple clients are
probably the most common clients of CardDAV services. When the Google
Contacts Web UI creates or edits a contact, Google CardDAV will
send that data using the vCard flavor used by Apple.
Therefore it makes sense to exchange contacts with *all* CardDAV
servers using that format. This format could be made configurable in
SyncEvolution on a case-by-case basis; at the moment, it is
hard-coded.
During syncing, SyncEvolution takes care to translate between the
vCard flavor used internally (based on Evolution) and the CardDAV
vCard flavor. This mapping includes:
X-AIM/JABBER/... <-> IMPP + X-SERVICE-TYPE
Any IMPP property declared as X-SERVICE-TYPE=AIM will get
mapped to X-AIM. Same for others. Some IMPP service types
have no known X- property extension; they are stored in
EDS as IMPP. X- property extensions without a known X-SERVICE-TYPE
(for example, GaduGadu and Groupwise) are stored with
X-SERVICE-TYPE values chosen by SyncEvolution so that
Google CardDAV preserves them (GroupWise with mixed case
got translated by Google into Groupwise, so the latter is used).
Google always sends an X-ABLabel:Other for IMPP. This is ignored
because the service type overrides it.
The value itself also gets transformed during the mapping. IMPP uses
an URI as value, with a chat protocol (like "aim" or "xmpp") and
some protocol specific identifier. For each X- extension the
protocol is determined by the property name and the value is the
protocol specific identifier without URL encoding.
X-SPOUSE/MANAGER/ASSISTANT <-> X-ABRELATEDNAMES + X-ABLabel
The mapping is based on the X-ABLabel property attached to
the X-ABRELATEDNAMES property. This depends on the English
words "Spouse", "Manager", "Assistant" that Google CardDAV
and Apple devices seem to use regardless of the configured
language.
As with IMPP, only the subset of related names which have
a corresponding X- property extension get mapped. The rest
is stored in EDS using the X-ABRELATEDNAMES property.
X-ANNIVERSARY <-> X-ABDATE
Same here, with X-ABLabel:Anniversary as the special case
which gets mapped.
X-ABLabel parameter <-> property
CardDAV vCards have labels attached to arbitrary other properties
(TEL, ADR, X-ABDATE, X-ABRELATEDNAMES, ...) via vCard group tags:
item1.X-ABDATE:2010-01-01
item1.X-ABLabel:Anniversary
The advantage is that property values can contain arbitrary
characters, including line breaks and double quotation marks,
which is not possible in property parameters.
Neither EDS nor KDE (judging from the lack of responses on the
KDE-PIM mailing list) support custom labels. SyncEvolution could
have used grouping as it is done in CardDAV, but grouping is not
used much (not at all?) by the UIs working with the vCards in EDS
and KDE. It seemed easier to use a new X-ABLabel parameter.
Characters which cannot be stored in a parameter get converted
(double space to single space, line break to space, etc.) during
syncing. In practice, these characters don't appear in X-ABLabel
properties anyway because neither Apple nor Google UIs allow entering
them for custom labels.
The "Other" label is used by Google even in case where it adds no
information. For example, all XMPP properties have an associated
X-ABLabel=Other although the Web UI does not provide a means to edit
or show such a label. Editing the text before the value in the UI
changes the X-SERVICE-TYPE parameter value, not the X-ABLabel as for
other fields.
Therefore the "Other" label is ignored by removing it during syncing.
X-EVOLUTION-UI-SLOT (the parameter used in Evolution to determine the
order of properties in the UI) gets stored in CardDAV. The only exception
is Google CardDAV which got confused when an IMPP property had both
X-SERVICE-TYPE and X-EVOLUTION-UI-SLOT parameters set. For Google,
X-EVOLUTION-UI-SLOT is only sent on other properties and thus ordering
of chat information can get lost when syncing with Google.
CardDAV needs to use test data with the new CardDAV vCard flavor.
Most CardDAV servers can store EDS vCards and thus
Client::Source::*::testImport passed (with minor tweaks in
synccompare) when using the default eds_contact.vcf, but
Client::Sync::*::testItems fails when comparing synced data with test
cases when the synced data uses the native format and the test cases
are still the ones from EDS.
A libsynthesis with URLENCODE/DECODE() and sharedfield parameter for
<property> is needed.
The code checking the session URL was never used because the
session did not exist yet during syncing. Adding a contactServer()
call fixes that.
The default WebDAV code in the dead code was redundant. The
default is set before checking for a specific server.
Different backend functions had to ensure that a connection
to the server existed. That work was meant to be done only
once during the existence of the backend instance, but due to
a missing "return" it was potentially (?) repeated multiple
times. Didn't cause any noticable problems.
The fields and their offsets were originally used for Synthesis vCard
extensions (X-CustomLabel-* and X-Synthesis-Ref). SyncEvolution never
used those. The extra fields cause unecessary overhead (logging,
merging, memory even when empty), so better remove them.
testcase files used to be installed in src/testcases.am, using a duplicated
list of files. Better do it in test/test.am with the canonical list of files
and only install generated files in src/testcases.am. This makes it easier
to extend the list in the future.
When doing a recursive scan of the home set, preserve the order of
entries as reported by the server and check the first one first. The
server knows better which entries are more relevant for the user (and
thus should be the default) or may have some other relevant
order. Previously, SyncEvolution replaced that order with sorting by
URL, which led to a predictable, but rather meaningless order.
For example, Google lists the users own calendar first, followed by
the shared calendars sorted alphabetical by their name. Now
SyncEvolution picks the main calendar as default correctly when
scanning from https://www.google.com/calendar/dav/.
Zimbra has a principal URL that also serves as home set. When using it
as start URL, SyncEvolution only looked the URL once, without listing
its content, and thus did not find the databases.
When following the Zimbra principal URL indirectly, SyncEvolution did
check all of the collections there recursively. Unfortunately that
also includes many mail folders, causing the scan to abort after
checking 1000 collections (an internal safe guard).
The solution for both includes tracking what to do with a URL. For the
initial URL, only meta data about the URL itself gets
checked. Recursive scanning is only done for the home set. If that
home set contains many collections, scanning is still slow and may run
into the internal safe guard limit. This cannot be avoided because the
CalDAV spec explicitly states that the home set may contain normal
collections which contain other collections, so a client has to do the
recursive scan.
When looking at a specific calendar, Google CalDAV does not report
what the current principal or the home set is and therefore
SyncEvolution stopped after finding just the initial calendar. Now it
detects the lack of meta information and adds all parents also as
candidates that need to be looked at. The downside of this is that it
doesn't know anything about which parents are relevant, so it ends up
checking https://www.google.com/calendar/ and
https://www.google.com/.
In both cases Basic Auth gets rejected with a temporary redirect to
the Google login page, which is something that SyncEvolution must
ignore immediately during scanning without applying the resend
workaround for "temporary rejection of valid credentials" that can
happen for valid Google CalDAV URLs.
The sorting of sub collections is redundant and can be removed,
because sorting already happens when storing the information for each
path. When scanning Google CalDAV starting at
https://www.google.com/calendar/dav/, the main calendar gets listed by
Google first, but because we reorder by sorting by path, it ends up
last in the SyncEvolution database list and thus is not picked as
default. This needs to be fixed separately by preserving the server's
order.
The previous change to the syncevo-webdav-lookup broke out-of-tree
compilation because it created a symlink to syncevo-webdav-lookup.sh
inside the current directory, which is not where the file resides when
compiling out-of-tree.
Instead of trying to be smart with symlinks (was only done for to avoid
a small file copy), simply copy the file.
syncevo-webdav-lookup.sh uses bash features (pipefail) and thus
must choose /bin/bash as shell explicitly. On systems with dash
as /bin/sh the script failed.
Server::run() still does some initialization (backend loading and file
watching). We need to wait with processing calls until that is done,
otherwise we have a race condition in TestFileNotify.testSession3:
- server starts
- test calls GetVersion()
- server answers as part of sending out log messages or loading backends (?),
sending incomplete information (no backends loaded yet)
- test touches syncevo-dbus-server
- server sets up watch
=> test fails because server missed the file modification and incorrect information returned
by GetVersions()
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.
This takes advantage of the engine setting (new items) and preserving
(existing items) the UID for backends and then generating it as part of
serialization.
The older hacks in create/setResourceName() relied on resource name matching
the UID, which is not guaranteed for items created by us via POST (depending
on the server) and/or other CardDAV clients.
The hacks could be removed; they are simply not used anymore at the moment.
Before, the UID property in a vCard was ignored by the engine.
Backends were responsible for ensuring that the property is
set if required by the underlying storage.
This change moves this into the engine:
- UID is now field. It does not get used for matching
because the engine cannot rely on it being stored
by both sides.
- It gets parsed if present, but only generated if
explicitly enabled (because that is the traditional
behavior).
- It is never shown in the DevInf's CtCap
because the Synthesis engine would always show it
regardless whether a rule enabled the property.
That's because rules normally only get triggered
after exchanging DevInf and thus DevInf has to
be rule-independent. We don't want it shown because
then merging the incoming item during a local sync
would use the incoming UID, even if it is empty.
- Before writing, ensure that UID is set.
When updating an existing item, the Synthesis engine reads
the existing item, preserves the existing UID unless the peer
claims to support UID, and then updates with the existing UID.
This works for local sync (where SyncEvolution never claims
to support UID when talking to the other side). It will break
with peers which have UID in their CtCap although they
rewrite the UID and backends whose underlying storage cannot
handle UID changes during an update (for example, CardDAV).
A backend must still activate the UID property by setting m_backendRule
to LOCALSTORAGE-WITH-UID instead of the default LOCALSTORAGE.
Custom backend rules must include the new sub-rule HAVE-VCARD-UID.
See "[os-libsynthesis] UID + CardDAV" for a discussion of this change.
Long term it would be better to configure profiles without depending
on the rule hack.
The "show" attribute of the property values is not supported
and was silently ignored by the Synthesis engine and thus had
no effect. Remove the useless attributes.
Check whether a collection is read-only by reading ACL properties.
The check is intentionally a bit fuzzy to avoid accidentally marking
a collection as read-only.
All read-only collections are moved to the end of the found databases, to
avoid picking them as default when there are read/write collections. This
needs to be done in both "list all databases" mode (aka --print-databases)
and in "pick default database" mode (aka syncing with a source which
had no explicit 'database' set).
To use a read-only collection, configure it in the 'database'
property.
This is relevant for OwnDrive, aka ownCloud, which has a read-only "birthday"
calendar which was picked as default instead of the real calendar.
The WebDAV backend determines what database to use based on 'database',
'syncURL', and (for DNS SRV discovery) 'databaseUser' respectively
'username'. The new messages help figuring out which settings were
really used.
Read-only database are shown with a new "<read-only>" tag.
There is no explicit read/write flag, so a lack of that tag
is no guarantee that a database is really writable. It depends
on the backend whether it checks for write acccess.
This is relevant for WebDAV database scanning (read-only
databases should not be the default) and could also be used
to enhance automatic setups (for example, do not use two-way
syncing for read-only databases).
Include the name of the file which got modified. This helped track down why
the server sometimes shut down unexpectedly during parallel testing (main
executable was renamed by D-Bus testing).
Not all exceptions thrown while executing a source operation contain
the source name. SyncSource::throwError() does that, but SE_THROW() as
used in helper code does not. It is better to add the source name as
logging prefix. The other advantage is that all lines will have the
prefix, not just the first one.
The SyncSource operations need access to the SyncSource class which
contains them to access the display name of the SyncSource instance.
A positive a side effect of telling the wrappers about the instance at
construction time is that the caller no longer has to pass the source
reference.
This change allows removing the SyncSource::throwError() special
cases, which completes the transition towards having correct source
code location information for all exceptions.
This could happen when using item operations on the command line.
In that case a source without name was created and that empty
name was inserted as prefix before errors encountered while
using the source.
Now empty string is the same as no string.
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.
Additional databases where not found for several
reasons. SyncEvolution ignored all shared calendars
(http://calendarserver.org/ns/shared) and Google marks the additional
calendars that way. The other problem was that the check for leaf
collections (= collections which cannot contain other desired
collections) incorrectly excluded those collections instead of only
preventing listing of their content.
With this change,
https://www.google.com/calendar/dav/?SyncEvolution=Google can be used
as starting point for Google Calendar.
The WebDAV backends contained a hack where the UID inside the data was forced
to be identical to the resource name. This is wrong for items created by us
via POST (because the server may choose a resource name != UID) or by some
other entity (where we have no idea how the resource name got chosen).
This commit removes the hack. Testing must be updated to pass correct data
with the same UID as on the server when updating an item, because the backend
will no longer ensure that and changing the UID of a resource gets rejected by
some servers.
The hack was introduced for peers which do not store the UID (for example, a
vCard or iCalendar 1.0 based SyncML client). A better solution must be found,
probably involving the Synthesis engine and its read/update/write cycle.
PUT has the disadvantage that a client needs to choose a name and then
figure out what the real name on the server is. With Google CardDAV that
requires sending another request and only works because the server happens
to remember the original name (which is not guaranteed!).
POST works for new items without a name and happens to be implemented
by Google such that the response already includes all required
information (new name and revision string).
POST is checked for as described in RFC 5995 once before creating a new
item. Servers which don't support it continue to get a PUT.
The 403 status code must be checked to detect UID conflicts when adding
an item that already exists on the server.
The life time and content of the member was not defined. It got used
in multiple places. It's cleaner to bind the openPropCallback() to
a local Props_t instance.
The "addressbook" source was written for the original iPhone and
Mac OS X. It hasn't been in use for a long time, keeping the code
around without actually compiling it makes no sense anymore.
The "--update <dir name>" operation was supposed to take the
item luids from the file names inside the directory. That part
had not been implemented, turning the operation accidentally
into an "--import".
Also missing was the escaping/unescaping of luids. Now the
same escaping is done as in command line output and command
line parsing to make the luids safe for use as file name.
When aborting, our AbortHandler gets called to close down logging.
This may involve memory allocation, which is unsafe. In FDO #76375, a
deadlock on a libc mutex was seen.
To ensure that the process shuts down anyway, install an alarm and give
the process five seconds to shut down before the SIGALRM signal will kill
it.
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.
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.
g_clear_object() unnecessarily creates a dependency on a more recent glib
version. It probably isn't used correctly here anyway (g_clear_object() checks
for NULL itself, so the code around it doesn't need and should do that), so we
can use the older g_object_unref() instead.
The code is a verbatim copy of the corresponding files from gSSO. The
only difference is that we compile the implementation as C++ code,
because that is what we set compile flags for in libsyncevolution.
This helper class will be used for D-Bus over plain files in
combination with g_dbus_connection_new().
Currently implements the new API by stopping to consume data on the
local side of the sync. The Bluetooth transfer on the remote side
continues to run and isn't even notified about the suspend; need a new
obexd API to also suspend that and then notify the remote side when we
want it to do that.
This adds GetPeerStatus() and "progress" events.
To detect DB changes as they happen, the SyncSource operations are
monitored. Upon entry, a counter gets increased and transmitted
through to the PIM manager in syncevo-dbus-server using extended
SourceProgress structs (only visible internally - public API must not
be extended!). This will count operations which fail and count those
twice which get resumed, so the counts will be too high
occasionally. That is in line with the API definition; because it is
not exact, the API only exposes a "modified" flag.
Progress is reported based on the "item received" Synthesis event and
the total item count. A modified libsynthesis is needed where the
SyncML binfile client on the target side of the local sync actually
sends the total item count (via NumberOfChanges). This cannot be done
yet right at the start of the sync, only the second SyncML message
will have it. That is acceptable, because completion is reached very
quickly anyway for syncs involving only one message.
At the moment, SyncContext::displaySourceProgress() holds back "item
received" events until a different event needs to be emitted. Progress
reporting might get more fine-grained when adding allowing held back
events to be emitted at a fixed rate, every 0.1s. This is not done yet
because it seems to work well enough already.
For testing and demonstration purposes, sync.py gets command line
arguments for setting progress frequency and showing progress either
via listening to signals or polling.
The code was meant to check whether a contact really has contact
data; because the if check was missing, the sync result checking
in testSync failed for test data where one or more contacts had
not photos.
The function now keeps the event loop running while waiting for
completion. That ensures that events get received immediately.
testpim.py now matches the po scanning process because of the _(' text
pattern. We need to exclude it explicitly.
The is new API and flag grant control over the PBAP sync mode. It is
implemented by setting the SYNCEVOLUTION_PBAP_SYNC env variable in the
forked helper process.
The main advantage is that processed data can be discarded
immediately. When using a plain file, the entire address book must be
stored in it.
It also enables suspending a transfer by stopping to read from the
pipe, either via some internal API or simply freezing the
syncevo-local-sync process with SIGSTOP.
The drawback is that obexd does not react well to a full pipe. It
simply gets stuck in a blocking write(); in other words, all obexd
operations get frozen and obexd stops responding on D-Bus.
SyncEvolution was meant to load the syncecal or syncebook shared object
which uses the most recent libraries (libical, libecal/libebook) on
the system and then stop loading further backends. Due to a string
handling bug the check for already backends always found nothing,
leading to multiple conflicting backends loaded on some systems (for
example, those with libical0 and libical1 installed).
If that happened, the backend became unusable.
Nightly testing uses a "Client_Sync_Current" symlink to point
the server to a location where it shall write its per-test logs
without having to reconfigure the server.
When the server-side session runs longer than a specific test, it ends up
using the "Client_Sync_Current" link of the next test. "permission denied"
errors were also seen, although it is less certain whether that had the same
root cause.
To avoid such issues, the sync session now tries to resolve the log dir
path to an absolute path at the start of the session.
The KDE Notes resources store items under a different MIME type than the one
used in AKonadi (see "[Kde-pim] note format"). SyncEvolution use the same type
as Akonadi and thus did not find existing KDE Notes resources.
To support both while KDE and Akonadi transition to the same type,
SyncEvolution now looks for notes resources using both MIME types and accepts
both kinds of items when reading. When writing, SyncEvolution picks the MIME
type that is supported by the resource, which hopefully avoids confusing the
KDE app using the resource (untested).
As a positive side effect, the "database" value used for opening a resource is
now checked more thoroughly. Non-existent resources and the type mismatches
like pointing a "kde-contacts" backend to a calendar resource are now detected
early.
Akonadi resources do not enforce iCalendar 2.0 semantic like
"each VEVENT must have a UID" (see "[Kde-pim] iCalendar semantic").
When receiving an event from a peer which itself does not enforce
that semantic (Funambol, vCalendar 1.0 based phones), then we
need to generate a UID, otherwise KOrganizer will ignore the
imported event.
Akonadi and Akonadi resources do not enforce iCalendar 2.0 semantic
like "UID+RECURRENCE-ID must be unique in a collection". Therefore we
must disable all tests expecting that behavior.
Regular syncing should work okay as long as the peer behaves. A misbehaving
peer will be able to send us invalid sets of items and syncing will not be
able to detect that because the sync engine itself is also agnostic to the
special iCalendar 2.0 semantic.
See "[Kde-pim] iCalendar semantic".
When used as storage in a server, Akonadi got called in a background thread
that gets created to handle slow initialization of sources and preventing
ensuing timeouts in HTTP clients (probably not needed for Akonadi itself,
but may still be useful when combining it with other sources).
Akonadi cannot be used like that, leading to false "Akonadi not running"
errors or (if one got past that check) failing item operations.
This commit avoids the situation by shifting the work back into the main
thread.
This is needed to move one-time operations from AkonadiSyncSource into the
main thread if (and only if) the source is used in a background
thread. Akonadi can't be used in a thread unless the thread was created with
QThread, which is not the case in SyncEvolution.
The basic functionality is the same as the older GRunWhile(). It just has to
be extended so that the action is guaranteed to be called only in the main
thread and there runs only once. This is achieved by turning the action
into a check which immediately returns false.
AkonadiSyncSource cannot include GLibSupport.h due to conflicting usage of
"signal" (a define in Qt, parameter name in glib), therefore these functions
are now made available via util.h.
Writing a test that ensures that libsynthesis gets the right
timezone was hard, because the original problem only occurs
when syncing a vCalendar 1.0 based device with SyncEvolution.
Emulating libsynthesis at least comes close.
libical 1.0 started to return VTIMEZONE definitions with multiple
absolute transition times instead of RRULEs. This causes problems when
exchanging data with peers (see
https://sourceforge.net/p/freeassociation/bugs/95/).
In SyncEvolution, this affected sending an event using New Zealand
time in vCalendar 1.0 format to a phone, because the internal,
out-dated definition of the time zone in libsynthesis was used as
fallback when loading RRULE-based timezone definitions from libical
failed (see "[SyncEvolution] Some events showing wrong time on
phone"). It might also affect exchanging data with CalDAV peers (not
tested).
The workaround is to include the original code from libical from
before the change in SyncEvolution and override
icaltimezone_get_component() with a version that uses the original
timezone loading code. This does not fix cases where other code causes
libical itself to load a timezone, but for libsynthesis it is good
enough because it does the loading early when no other code should
have used libical.
The downside is that now we need to maintain the RRULE heuristics and
ensure that they actually work. Copying libical/src/test/timezones.c
would be useful for that.
Long-term it would be good to enhance libical itself such that it can
return a VTIMEZONE with suitable RRULEs matching a specific event,
point in time or time range.
Check for icaltimezone_get_component() before actually loading libical.
That way our own code uses our own copy of that method from the copied
icaltz-util.c instead of always using the one from libical.
For that we must not overwriting already found functions.
gcc 4.4 complains about "num_types" being used potentially uninitialized.
This probably refers to bailing out before num_types gets set. However,
the use is conditional on "types", which is only set after "num_types",
so this is a false warning.
Prevent the warning anyway for the sake of clean -Wall -Werror compilation.
When EFREAD() does a "goto error", the code leaks the "r_trans"
memory. Found with cppcheck. Adding cleanup code fixes that.
To avoid a false cppcheck warning, the free() call must use the same
variable name as the calloc() assignment. It is a bit more readable
that way, too.
Can be compiled separately when linking against libical. For this
to work it must use header file locations as in libical client
programs.
icaltzutil_get_zone_directory() can come from either libical or
the icatz-util.c copy, depending on DISABLE_ICALTZUTIL_GET_ZONE_DIRECTORY.
We have to use singleton methods to ensure that the global std::string
instances are initialized before use. There was some non-determinism before
because SyncSource loading happens as part of the executable initialization,
too, and can cause a call to EDSAbiWrapperInit().
On some platforms, LDFLAGS is used during configure to set rpath
options. runtests.py must pass the -no-install to the linker
of client-test without overwriting those flags.
This fixes an issue where configure fails to find Akonadi when
test programs do not compile because QString is not found:
checking for Akonadi/Collection... no
configure: error: akonadi.pc not found. Install it to compile with the Akonadi backend enabled.
...
configure:21857: checking Akonadi/Collection presence
configure:21857: g++ -E -I/usr/include/ -I/usr/include//KDE -I/usr/include/qt4 conftest.cpp
In file included from /usr/include//KDE/Akonadi/../../akonadi/collection.h:25:0,
from /usr/include//KDE/Akonadi/Collection:1,
from conftest.cpp:44:
/usr/include/akonadi/entity.h:24:19: fatal error: QString: No such file or directory
#include <QString>
We must use the temporary file that was created for us securily, not
a temp file named after that file. This caused a temp file vulnerability
and the real temporary files were not deleted by the script.
We need the right file suffix for the compiler. "mktemp --suffix"
would be useful, but it not yet available in Ubuntu Lucid. So instead
create a temp dir (which works in Ubuntu Lucid) and create files
inside that.
Disabling the Akonadi test in the backend which used to run
as part of "client-test SyncEvolution" and fail unless Akonadi
was started first. The test is not important enough to justify
running Akonadi.
glib 2.39.0 (aka GNOME 3.10) as found in Ubuntu Trusty introduces
warnings when g_source_remove() is passed an unknown tag. SyncEvolution
did this in two cases: in both, the source callback returned false and thus
caused the source to be removed by the caller. In that case, the explicit
g_source_remove() is redundant and must be avoided.
Such a call is faulty and might accidentally remove a new source with the same
tag (unlikely though, given that tags seem to get assigned incrementally).
The only noticable effect were additional error messages with different
numbers:
[ERROR] GLib: Source ID 9 was not found when attempting to remove it
libical 1.0 broke the ABI, leading to libical.so.1. The only relevant change
for SyncEvolution is the renumbering of ICAL_*_PROPERTY enum values. We can
adapt to that change at runtime, which allows us to compile once with
libical.so.0, then patch executables or use dynamic loading to run with the
more recent libical.so.0 if we add 1 to the known constants. Done without
changing all code via define tricks.
This new mode is enabled for --enable-evolution-compatibility or (limited to
ical) for --enable-evolution-compatibility=ical.
Testing on one platform can only be sped up further by parallelizing
it. Each action started by runtests.py may potentially run in parallel
to other actions, if it either does not need files in the home
directory (like checking out source) or can be run in its own, private
home directory.
The new --home-template parameter specifies the location of a home
directory that runtests.py can copy to create these private home
directory of each test. Each action is run in a fork of the main
runtests.py, so env and working directory changes are confined to that
fork and do not affect other actions.
When --home-template is given, runtests.py will also set up a new home
directory and point to it with HOME,
XDG_CACHE/CONFIG/DATA_HOME. Because test-dbus.py and testpim.py use a
non-standard layout of the XDG dirs without directories hidden by the
leading dot, runtests.py must move the standard directories to conform
with the other scripts' expectation.
testpim.py itself must be more flexible and allow running with a root
for the XDG dirs that is not called "temp-testpim". To allow parallel
tests, GNOME keyrings must be located in XDG_DATA_HOME, which is
supported since gnome-keyring 3.6. On older distros, parallel testing
does not work because gnome-keyring-daemon would always look in the
home directory as specified in /etc/passwd, which we cannot override.
testpim.py must not delete the keyrings when cleaning up the XDG dirs
for a test.
Locking Murphy resources and allocating jobs from GNU make jobserver
gets moved into a separate script which wraps the actual execution of
the action. Some change would have been necessary anyway (we cannot
connect to D-Bus and then fork) and the new approach is cleaner. It
ensures that cut-and-paste of the action command line into a shell
will only run with the necessary Murphy resource locked. Previously,
it might have conflicted with a running test.
As a new feature, test names as passed to resources.py can be mapped
to actual resource names via RESOURCE_<test name> env
variables. Useful for tests with different names which use the same
resources (currently DAViCal for the DAV server tests).
Sometimes, on a more recent Linux distro, the helper is said to terminate
with SIGKILL although we only sent a SIGTERM. Not sure why that happens.
It randomly breaks tests, so let's ignore the unexpected signal. Everything
else seems to work correctly.
When timing out, SyncContext nevertheless sometimes resent a message despite
being close to the final timeout deadline. Relax the math further so that more
time must remain before the deadline when attempting a resend.
The earlier behavior randomly broke some of the resend tests.
When testLocaledPhone was run after testLocale, it used the localed installed
by the earlier test. We must ensure that daemons on the system bus get removed
after each test.
Control flow analysis from clang's own C++ compiler and clang's
scan-tool come to different results: the compiler fails to detect
that the variable will be initialized in all cases and thus requires
a redundant initialization to avoid an "uninitialized memory read"
error with -Wall, while scan-tool complains about the redundant write.
To satisfy both, avoid the initialization when doing static code analysis.
The intermediate "msg" variable caused a warning from clang's scan-build about
setting but not reading it. To get rid of the warning we are now writing into
the final buffer directly.
From scan-build: size argument is greater than the free space in the
destination buffer, for the line with strncat().
This might be a false positive in scan-build, the size looks right
at first glance. To be on the safe side and get rid of the warning,
allocate one one more byte...
clang's scan-tool complains about keeping a local address stored
in a global structure, the argument description. This is not a real
problem because the structure is not going to be used after leaving
main(), but perhaps it is cleaner nevertheless to allocate the
structure on the stack.
Allow marking methods and functions as "does not return". This may
help static code analysis tools to determine that certain code paths
will not be taken.
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.
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.
In this case it is fairly obvious that new_active and old_active
will both be set before use. There is no need for setting old_active
to FALSE. cppcheck warned about this.
cppcheck cannot know that one of the two if checks must succeed
and warns about the case when both are false. This shouldn't
happen, but it doesn't hurt to initialize the const char * pointer
to a default value.
cppcheck complained about "con" not being initialized in the constructor.
In general it is better to fully initialize all members, even if (as in this
case) they are not used outside of a second initialization step (open/close).
This warning pointed to a second potential problem: should SyncEvolution ever
fail to call close() after open(), resources would leak because all allocated
resources get freed in close(). For the sake of defensive programming it is
better to initialize in the constructor, then free in both close() and the
destructor.
That way we can initialize m_accessMode immediately in the constructor,
which keeps cppcheck happy and avoids uninitalized data in the instance
between creation and open(), which is better than before.
cppcheck correctly identified some places where the weaker empty()
call can be used instead of size() to determine whether an STL
data structure contains something.
None of them were really performance critical, but let's fix them
anyway for the sake of clean cppcheck scan results.
cppcheck complains about sub-optimal performance (std::string should be passed
via const reference, not by value). It is not critical here, but for the sake
of getting clean scan results let's fix the issues...
One of the offending methods was never implemented, nor called. Removed.
The Collection class did not initialize all its members. Even if the code
never relied on them being initialized (not checked), it is better to be
safe and predictable, which means initializing all members in the constructor.
Renaming the backends now needs to be done before calling make.
This is used to include two versions of syncecal.so:
- one for EDS >= 3.6 < 3.10
- one for EDS >= 3.10 with the libecal-1.2 soname patched
The second flavor became necessary because EDS 3.10 accidentally
changed the soname. The API and ABI actually is the same, so
we don't need to compile twice.
The package meta data must include additional alternatives for
libecal and libebook when including extra backends. This must
be passed in via EXTRA_BACKENDS_ECAL/EBOOK_REQUIRES; the makefile
does not attempt to determine what those are.
Only the "client-test" executable and its test data were installed so far, in
/usr/bin resp. the doc dir. Now also test-dbus.py and testpim.py plus their
data files get installed, using a new /usr/lib/syncevolution/test directory
for all test files and commands.
"runtests.py --prebuilt" can take one or more .deb files including these
tests, will install them as the "compile" operation and then will use the
installed SyncEvolution, without overriding any paths. This relies on having
suitable rights for "dpkg" and /usr/lib/syncevolution/test; the way how that
is done in the nightly testing is via chroots where the entire file system
is writable by the normal test user.
Normal users without such write access to /usr/lib/syncevolution/test need
to copy that directory into their home directory first.
When enabling auto-sync for a config and then accessing or syncing the
config manually via the command line tool, the server would abort at
the time when the auto-sync was originally scheduled.
The reason is that rescheduling reset the timeout which caused the
rescheduling, only to be cleared when the rescheduling callback
returns. Then when it triggered next, an empty boost::method was
called. The fix is to to track whether the Timeout instance still
refers to the same glib timeout and only clear the instance if that's
still the case.
A unit test will be committed separately.
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 ESourceRegistry singleton used to be handled by the lib, now it
only hosts a pointer to it created elsewhere. To avoid a false
dependency, the lib should not be linked against EDS.
A user reported via email that the Nokia 515 sends
'application/vnd.syncml+wbxml; charset=UTF-8' as type of its messages
this tripped up the syncevo-http-server, leading to:
[ERROR] syncevo-dbus-server: /org/syncevolution/Server: message type 'application/vnd.syncml+wbxml; charset=UTF-8' not supported for starting a sync
We need to strip the '; charset=UTF-8' suffix also when checking for
WBXML.
The earlier fix did not cover the case of compiling SyncEvolution
statically. In that case the *Register.cpp files do get compiled for
all backends. When nothing is enabled, signonRegister.cpp cannot
register any kind of provider.
Source backends can register an inactive source backend to document
that the backend exists and merely was not enabled during compilation;
there is no such support for providers.
When neither gSSO nor UOA were enabled, the provideruoa.so was
still enabled and failed to link because its LDFLAGS were not
set. It shouldn't have been enabled at all.
While we are at it, allow both to be enabled in the build rules,
even though the code doesn't support it (symbol clashes).
Add a new configuration flag (--enable-uoa) to enable building the
signon backend for Ubuntu Online Accounts. The work done for gSSO is
reused and just slightly adapted (using preprocessor conditions) to work
with UOA as well.
Because of an uninitialized memory read introduced for DLT after
1.3.99.5, output from the syncevolution command line tool was not
shown randomly.
Other usages of the second constructor for MessageOptions may also
have been affected.
That this was not caught by nightly testing points towards a problem
which will have to be solve separately: test-dbus.py needs to run
syncevolution under valgrind.
