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>
Using templates with a varying number of types allows removing
duplicated code for the different cases.
Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
Building with recent Clang in C++ mode fails when using the non-standard
typeof operator. We can't rely on the new(ish) decltype yet, so use
the Boost implementation instead.
Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
Logging must be thread-safe, because the glib log callback may be
called from arbitrary threads. This becomes more important with EDS
3.8, because it shifts the execution of synchronous calls into
threads.
Thread-safe logging will also be required for running the Synthesis
engine multithreaded, to overlap SyncML client communication with
preparing the sources.
To achieve this, the core Logging module protects its global data with
a recursive mutex. A recursive mutes is used because logging calls
themselves may be recursive, so ensuring single-lock semantic would be
hard.
Ref-counted boost pointers are used to track usage of Logger
instances. This allows removal of an instance from the logging stack
while it may still be in use. Destruction then will be delayed until
the last user of the instance drops it. The instance itself must be
prepared to handle this.
The Logging mutex is available to users of the Logging module. Code
which holds the logging mutex should not lock any other mutex, to
avoid deadlocks. The new code is a bit fuzzy on that, because it calls
other modules (glib, Synthesis engine) while holding the mutex. If
that becomes a problem, then the mutex can be unlocked, at the risk of
leading to reordered log messages in different channels (see
ServerLogger).
Making all loggers follow the new rules uses different
approaches.
Loggers like the one in the local transport child which use a parent
logger and an additional ref-counted class like the D-Bus helper keep
a weak reference to the helper and lock it before use. If it is gone
already, the second logging part is skipped. This is the recommended
approach.
In cases where introducing ref-counting for the second class would
have been too intrusive (Server and SessionHelper), a fake
boost::shared_ptr without a destructor is used as an intermediate step
towards the recommended approach. To avoid race conditions while the
instance these fake pointers refer to destructs, an explicit
"remove()" method is necessary which must hold the Logging
mutex. Using the potentially removed pointer must do the same. Such
fake ref-counted Loggers cannot be used as parent logger of other
loggers, because then remove() would not be able to drop the last
reference to the fake boost::shared_ptr.
Loggers with fake boost::shared_ptr must keep a strong reference,
because no-one else does. The goal is to turn this into weak
references eventually.
LogDir must protect concurrent access to m_report and the Synthesis
engine.
The LogRedirectLogger assumes that it is still the active logger while
disabling itself. The remove() callback method will always be invoked
before removing a logger from the stack.
Changing the process' name, even temporarily, is not thread-safe
and needs to be avoided. Instead pass the additional parameter
explicitly via the MessageOptions container.
Logger::formatLines() and LogStdout::write() are passed the process
name as "const std::string *" pointer, with Logger::getProcessName()
acting as fallback for NULL. Calling that is delayed as long as possible,
to avoid making the string copy when not necessary.
The previous implementation of formatLines() used a std::string
reference, but only used the content to determine whether to include
the current process name - probably not what was intended, but harmless
because either the empty string or the current name were passed.
When libical and libecal were not installed, trying to use the CalDAV
backend for VEVENTs segfaulted because it depends on libical and did
not check properly for it. Only affected syncevolution.org binaries.
The root cause is that libical functions were only looked up in
combination with libecal, when compiled with
--enable-evolution-compatibility. Now they are first checked via
libecal (for old libecal which embedded libical) and separately in
libical itself as fallback.
Instead of using delays to kill processes at the right time, watch old
and new debug output via D-Bus and then kill the processes. To avoid
race conditions, these processes get delayed at the right point.
Added tests for local sync and command line, covering killing of
all involved processes.
Added a "target side of local sync ready" INFO message to introduce
the output which has the target context in the [INFO] tag. The sync report
from the target side now has the target context embedded in brackets
after the "Changes applied during synchronization" header, to avoid
ambiguities.
Output from the target side of a local sync was passed through stderr
redirection as chunks of text to the frontends. This had several
drawbacks:
- forwarding only happened when the local sync parent was processing
the output redirection, which (due to limitations of the implementation)
only happens when it needs to print something itself
- debug messages were not forwarded
- message boundaries might have been lost
In particular INFO messages about delays on the target side are
relevant while the sync runs and need to be shown immediately.
Now the output is passed through D-Bus, which happens immediately,
preserves message boundaries and is done for all output. The frontend
can decide separately whether it shows debug messages (not currently
supported by the command line tool).
Implementing this required extending the D-Bus API. The
Server.LogOutput signal now has an additional "process name"
parameter. Normally it is empty. For messages originating from the
target side, it carries that extra target context string.
This D-Bus API change is backward compatible. Older clients can still
subscribe to and decode the LogOutput messages, they'll simply ignore
the extra parameter. Newer clients expecting that extra parameter
won't work with an older D-Bus daemon: they'll fail to decode the
D-Bus message.
This revealed that the last error messages in a session was
incorrectly attributed to the syncevo-dbus-server. Might also have
happened with several other error messages. Now everything happening
in the server while working on code related to a session is logged as
coming from that sessions. It's not perfect either (some of the output
could be from unrelated events encountered indirectly while running
that code), but it should be better than before.
The patch changes the handling or errors in the local sync parent
slightly: because it logs real ERROR messages now instead of plain
text, it'll record the child's errors in its own sync report. That's
okay, user's typically shouldn't have to care about where the error
occurred. The D-Bus tests need to be adapted for this, because it
removes the "failure in local sync child" from the sync report.
Another, more internal change is that the syncevo-local-sync helper
does its own output redirection instead of relying on the stderr
handling of the parent. That way libneon debug output ends up in the
log file of the child side (where it belongs) and not in the parent's
log.
KWallet support was broken: syncevo-dbus-server checked
KDE_FULL_SESSION to determine whether it should use KWallet instead of
GNOME Keyring. That did not work, because the env variable was not set
for D-Bus daemons.
Automatically detecting KDE users is not possible at the
moment. Instead KDE users have to manually set the new "keyring"
global config property to "KDE" (case insensitive) if the
SyncEvolution installation supports both, because GNOME Keyring is the
default to avoid surprises for traditional users. If only KWallet
support is enabled, then this is not necessary.
"GNOME" and "true/false/1/0/yes/no" can also be set. This has the
advantage that keyring usage can be enabled permanently for the
command line in --daemon=no mode; normally keyrings are not used in
that mode because accessing them can bring up UI dialogs.
It also becomes possible to disable keyring usage in syncevo-dbus-server,
something which couldn't be done before.
The --keyring command line option is still supported, as an alias for
"[--sync-property] keyring=<value>". The default value for --keyring
is true, to match the traditional behavior. In contrast to other sync
properties, setting "keyring" does not require an explicit --run
parameter. Again this is done to mirror traditional usage.
Reading a password also (unintentionally) checked all supported
storages while searching for the password. Now it uses exactly
one storage and falls back to asking for the password directly.
The commit itself also cleans up the code a bit (reformatted, fixed
comments). Choosing the right slot in the password signals is done via
a new InitStateTri parameter which contains the "keyring" setting.
Error checking (unsupported keyring string, --keyring=yes and no
keyring enabled) is done in additional slots which run after all the
regular ones.
Parameter parsing for --sync and --keyring were unified. However,
there is the difference that --keyring has an implicit default value
("yes") and never has an additional parameter, in contrast to --sync,
which always is followed by one.
The new CmdlineTest::testKeyring covers different ways of using
--keyring. It relies on actually invoking keyring backends, something
not done by the default SyncContext UI. Therefore
CmdlineSyncClient+KeyringSyncCmdline were moved into libsyncevolution,
to be used by CmdlineTest.
The user-visible part of this change is that command line output now
uses the same [ERROR/INFO] prefixes like the rest of SyncEvolution,
instead of "Error:". Several messages were split into [ERROR] and
[INFO] parts on seperate lines. Multi-line messages with such a prefix
now have the prefix at the start of each line. Full sentences start
with captital letters.
All usage errors related to the synopsis of the command line now
include the synopsis, without the detailed documentation of all
options. Some of those errors dumped the full documentation, which was
way too much information and pushed the actual synopsis off the
screen. Some other errors did not include usage information at all.
All output still goes to stdout, stderr is not used at all. Should be
changed in a seperate patch, because currently error messages during
operations like "--export -" get mixed with the result of the
operation.
Technically the output handling was simplified. All output is printed
via the logging system, instead of using a mixture of logging and
streaming into std::cout. The advantage is that it will be easier to
redirect all regular output inside the syncevo-dbus-helper to the
parent. In particular, the following code could be removed:
- the somewhat hacky std::streambuf->logging bridge code (CmdlineStreamBuf)
- SyncContext set/getOutput()
- ostream constructor parameters for Cmdline and derived classes
The new code uses SE_LOG_SHOW() to produce output without prefix. Each
call ends at the next line, regardless whether the string ends in a
newline or not. The LoggerStdout was adapted to behave according to
that expectation, and it inserts the line prefix at the start of each
line - probably didn't matter before, because hardly any (no?!)
message had line breaks.
Because of this implicit newline in the logging code, some newlines
become redundant; SE_LOG_SHOW("") is used to insert an empty line
where needed. Calls to the logging system are minimized if possible by
assembling output in buffers first, to reduce overhead and to adhere
to the "one call per message" guideline.
Testing was adapted accordingly. It's a bit stricter now, too, because
it checks the entire error output instead of just the last line. The
previous use of Cmdline ostreams to capture output from the class was
replaced with loggers which hook into the logging system while the
test runs and store the output. Same with SyncContext testing.
Conflicts:
src/dbus/server/cmdline-wrapper.h
The implementation was broken when using GDBus GIO (mismatch between
string and D-Bus object type) and probably also with GDBus libdbus
(missing boolean for GetConfig()).
Rewritten using blocking D-Bus calls, which makes the implementation
also a lot shorter and easier to understand.
It would be good to have automated tests for these features. Not done
yet.
The call operator on DBusClientCall[0123] is now blocking. It will
return result values as single value, std::pair or boost::tuple,
respectively. If an error is encountered either locally or in the
peer, a runtime_error is thrown.
This is an API change. The traditional implementation was to start an
asynchronous call. That is still possible, but has to be requested
explicitly with the new start() method.
This distinction is necessary because C++ cannot guess (easily)
whether the callback parameter is a callback type or another parameter
for the D-Bus peer. It's also a bit cleaner in the source code because
now the call operator really acts like a normal, synchronous call.
The downside of the synchronous calls is that complex return values
have to be copied more often. If that is too expensive, then use the
asynchronous start() + callback approach.
Having the signal handling code in SyncContext created an unnecessary
dependency of some classes (in particular the transports) on
SyncContext.h. Now the code is in its own SuspendFlags.cpp/h files.
Cleaning up when the caller is done with signal handling is now part
of the utility class (removed automatically when guard instance is
freed).
The signal handlers now push one byte for each caught signal into a
pipe. That byte tells the rest of the code which message it needs to
print, which cannot be done in the signal handlers (because the
logging code is not reentrant and thus not safe to call from a signal
handler).
Compared to the previous solution, this solves several problems:
- no more race condition between setting and printing the message
- the pipe can be watched in a glib event loop, thus removing
the need to poll at regular intervals; polling is still possible
(and necessary) in those transports which do not integrate with
the event loop (CurlTransport) while it can be removed from
others (SoupTransport, OBEXTransport)
A boost::signal is emitted when the global SuspendFlags change.
Automatic connection management is used to disconnect instances which
are managed by boost::shared_ptr. For example, the current transport's
cancel() method is called when the state changes to "aborted".
The early connection phase of the OBEX transport now also can be
aborted (required cleaning up that transport!).
Currently watching for aborts via the event loop only works for real
Unix signals, but not for "abort" flags set in derived SyncContext
instances. The plan is to change that by allowing a "set abort" on
SuspendFlags and thus making
SyncContext::checkForSuspend/checkForAbort() redundant.
The new class is used as follows:
- syncevolution command line without daemon uses it to control
suspend/abort directly
- syncevolution command line as client of syncevo-dbus-server
connects to the state change signal and relays it to the
syncevo-dbus-server session via D-Bus; now all operations
are protected like that, not just syncing
- syncevo-dbus-server installs its own handlers for SIGINT
and SIGTERM and tries to shut down when either of them
is received. SuspendFlags then doesn't activate its own
handler. Instead that handler is invoked by the
syncevo-dbus-server niam() handler, to suspend or abort
a running sync. Once syncs run in a separate process, the
syncevo-dbus-server should request that these processes
suspend or abort before shutting down itself.
- The syncevo-local-sync helper ignores SIGINT after a sync
has started. It would receive that signal when forked by
syncevolution in non-daemon mode and the user presses
CTRL-C. Now the signal is only handled in the parent
process, which suspends as part of its own side of
the SyncML session and aborts by sending a SIGTERM+SIGINT
to syncevo-local-sync. SIGTERM in syncevo-local-sync is
handled by SuspendFlags and is meant to abort whatever
is going on there at the moment (see below).
Aborting long-running operations like import/export or communication
via CardDAV or ActiveSync still needs further work. The backends need
to check the abort state and return early instead of continuing.
Instead of forking and continuing to sync in the forked process
without an explicit exec(), exec() the 'syncevo-local-sync' helper in
the forked process. The syncevo-local-sync helper binary gets
installed into libexec. SYNCEVOLUTION_LIBEXEC_DIR must be set if that
helper is not installed yet, not in the PATH, or an old version would
be found without that env variable ("make" without "make install"
during development!).
Main advantage is the cleaner environment for running the child side
of local sync. Required for getting ActiveSync to work again (GDBus
GIO as used by recent activesyncd client libraries did not work in the
forked process without the exec()).
Full D-Bus communication gets established between parent and child.
The downside is the hard dependency of local sync on the D-Bus
libraries (not the daemon!).
D-Bus communication allowed implementing interactive password requests
from the child side through the parent to the UI using the parent,
something that wasn't implemented before.
The child asks its parent for the password, which in turn
passes the request to its SyncContext. This happens to work
when that SyncContext is a normal instance (reads from stdin,
the "syncevolution --daemon" case) and the syncevo-dbus-server
(sends out an Info Request signal and waits for a response).
The info request and response are handled in the blocking
askPassword() by polling the running mail loop, so the parent should
remain responsive. Overall it is still a pretty difficult setup; it
would be better if askPassword() was asynchronous.
Describing the required password also is sub-optimal: the sync-ui just
asks for a password in its current config (even though that might not
be the config which currently gets synced) and crashes if no config is
currently selected. The command line uses the description derived from
the property and config name, which is a bit technical, but at least
correct.
Syncing uses the child's error string as "first error" in the parent,
too, by logging it anew on the parent side. That puts it into the
parent's sync report ahead of any additional error that it might
encounter while the child shuts down. Also use the child's status when
available instead of a misleading TransportError.
In addition, suppress as many of these errors as possible when we know
already that the child reported an error in its sync report. Not all
"transport errors" are currently avoided that way, but this is at
least a first step.
DBusCallObject and friends were not used anywhere. Removed.
DBusObject and DBusRemoteObject used pure virtual methods to let
derived classes provide information about interface, path,
destination, method and the connection. The idea behind that was that
most of these strings are static and thus do not need to be copied.
The downside is that one had to derive from these classes in order to
provide the required information. The same class could not own two
instances of the generic DBusObject to access two different
destinations. It also sprinkled the code for setting up D-Bus
operations into several different places (constructor, class definition).
Now the information is passed to the DBus[Remote]Object constructor
and stored in the classes, which are thus merely containers for the
information and thus easier to use. Users of the classes still derive
from them, to keep the change smaller, although that is no longer
necessary.
Removed plain DBusConnection pointers from the C++ interface, return
DBusConnectionPtr instead. The exception is DBusObject, which still
returns plain pointers owned by the instance (as before) to ease
integration with the underlying D-Bus library. DBUS_CONNECTION_TYPE
is not needed.
This revealed that quite some code incorrectly took another reference
to the connection when assigning to DBusConnectionPtr (assignment
always increases the refcount, only in the constructor is that
optional). As a result the private connections were never
destructed. Apparently there were some global pointers to active
connections, so that this (minor) resource leak didn't show up in
valgrind.
It also showed that the closing of the D-Bus connection never happened
properly, although libdbus requires it. The ref counting mechanism
cannot be used for this because it cannot be checked whether the last
reference is about to be dropped. The slightly hackish solution is to
designate one DBusObject as the "main owner" of the connection. When
that object destructs, it closes the connection. There might still be
some other references; they simply cannot (and shouldn't) send or
receive messages anymore.
Background: Prior to this patch a C++ wrapper around Bluez gdbus, a
convienience wrapper around the low-level libdbus, was used by
syncevo-dbus-server. This patch introduces a second C++ wrapper around
the GIO GDBus implementation. The reason for introducing this second
wrapper is to move away from using the in-tree copy of Bluez gdbus and
towards a more well-maintained dbus implementation. Also, libdbus was
not designed to be thread-safe whereas GIO GDBus was.
The GIO GDBus wrapper retains the same public api as the first
one. This means the consumers of this wrapper (syncevo-dbus-server,
for example) have remained almost completely untouched. The only
exceptions are in the few case where libdbus objects where used
directly by the consuming class.
The choice of which wrapper is determined at configure time. The
option can be explicitly set using the --with-gio-gdbus and
--without-gio-gdbus flags or, if no flag is given, an adequate version
of GIO is search for. If found, the GIO GDBus wrapper is chosen.
The memset/memcpy of the embedded boost::function instances inside the
old ClientTestConfig was causing segfaults at the end of a client-test
run if compiled with optimization.
Therefore this commit turns ClientTestConfig into a proper class
containing members which initialize themselves (Bool wrapper class,
std::string), thus memset is no longer needed and used. Also added the
standard m_ prefix.
m_numItems is gone, was never set by any backend anyway and even
expected to be consistent in one test. Now CLIENT_TEST_NUM_ITEMS is
read by defNumItems() each time it is needed.
Removed "const char *" strings from method parameters. This revealed
that config.itemType (a const char *) was incorrectly passed to
insert() where the boolean "relax" parameter should have been given.
Replaced by "false" (= strict checking) even though the old code
must have run with an implicit "true" (= relaxed checking). Let's see
whether any tests fail now.
clang 2.8 compiles SyncEvolution + Synthesis faster than g++ 4.4.5
(3:40min instead of 4:10min on my laptop) and produces more useful
error reports. This patch fixes the code so that it compiles cleanly
with clang when using "-Wall -Werror -Wno-unknown-pragmas". Note that
clang 2.6 (Debian Squeeze) goes into an infinite recursion compiling
code using gdbus-cxx-bridge.h and dies eventually with a stack
overflow - can't be used.
Changes necessary for clang:
- eptr pointer referencing ambiguous, use *x.get() instead
- boost::intrusive_ptr* must be defined before code using it
- two-phase template checking requires explicitly specifying
members in base classes
- name clashes with plain C structs (DBusServer, DBusWatch) are
an error and need to be avoided (done with namespaces GDBusCXX and
SyncEvo)
- floats cannot be inline constants
- unused methods in local classes are warned about (left() in SyncML.cpp)
SYNCEVOLUTION_DEBUG was already used to disable output redirection.
But messages at level DEBUG were not printed in many command line
operations, with no way to change that because logLevel is a per-peer
config option.
This patch therefore increases the log level to DEBUG when
SYNCEVOLUTION_DEBUG is set.
libecal/ebook implementation based on D-Bus (Evolution >= 2.30)
inevitably will run into D-Bus timeouts as the amount of data
increases.
This patch moves an earlier hack written for Maemo into the core
code and enables it by default, if the EDS backends are active.
It works by intercepting dbus_connection_send_with_reply()
and substituting timeout_milliseconds==-1 ("default timeout
of 25 seconds") with timeout_milliseconds=INT_MAX ("no timeout").
Setting SYNCEVOLUTION_DBUS_TIMEOUT to number of milliseconds
allows controlling the final timeout value.
The main goal of the new D-Bus API is to avoid unnecessary (and incorrect)
updates in Genesis when running non-sync sessions with the command line.
This patch sets the flag accordingly to achieve this.
Note that the modified command line now fails with
org.freedesktop.DBus.Error.UnknownMethod when running against an old
syncevo-dbus-server. There's no fallback to the older StartSession
method. This shouldn't be necessary because the command line is shipped
with the server.
Just in case an INFO message is printed when the method is found to
be missing.
One common mistake among developers is that they recompile SyncEvolution
and then run with the system's D-Bus server. This patch adds an INFO
message about such a version mismatch, but does not abort because such
a combination may be intentional.
It is implemented as part of the attach process: as soon as attaching
was successful, fire of another asynchronous method call to the new
Server.GetVersions() instead of returning from the main loop. Once
that completes, check the version and return.
The previous solution of linking Bluex gdbus statically was not
enough. On Fedora 14, compile errors due to the glib header files
being pulled in indirectly appeared.
This patch does a global search/replace which changes the
"gdbus" (GLib D-Bus) prefix into "bdbus" (Bluez D-Bus). For the
record:
perl -pi 's/g_dbus_/b_dbus_/g; s/GDBus/BDBus/g; s/GDBUS/BDBUS/g' ...
When glib logging is invoked, it check the application name and complains
if not set:
** (process:10130): WARNING **: g_set_application_name not set.
With output redirection we hide this in the command line, but it
showed up in client-test when the system was in a state were another
glib WARNING was triggered.
This patch sets the unlocalized program name instead of using a
localized application name, because we don't have and don't need
localization for these two programs. That seems to satisfy glib.
When running command line arguments in dbus server, it is necessary
to pass environment variables in the command line environment.
The temporary solution is just to collect possible used environment
variables to dbus server. To support it, a new argument of dbus
method 'Session.execute' is added to pass the map of environment
variables.
If there is any error during executing command line arguments in
dbus server, it should catch the exception and set the error code
in the Status. Thus command line can judge the error code and
return the correct value to system.
When passwords in the config are not set, the daemon asks them from
clients like command line. Command line should react and let
user input passwords and then send responses to the daemon.
Command line listens to 'Server.InfoRequest' signal and only handles
password requests from sessions created for command line in the daemon.
If command line dies in the process of inputting passwords, the daemon
always waits for it so expand current timeout mechanism into this
process. Otherwise, the daemon waits permanently.
Convert relative paths into absolute paths before passing arguments
to dbus server because command line and dbus may have different working
directory.
Leave this kind of task in Command line for it knows the meaning of
parameters and convert them if necessary.
Add a new function 'relToAbs' to convert a relative path to a
canonicalized absolute path.
Previously it is supposed the arguments passed to d-bus are valid.
This seems not good as there are always some unpredictable issues.
Throw exceptions when parsing them encoutering errors.
Things like "--sync-property ?" are handled by Cmdline::parse(), producing
output as the parameters are parsed. In this case, running the command line
again in the D-Bus server duplicates the output (once locally, once in the
server).
This patch adds a check for it, similar to the one in Cmdline::run() itself.
Add a 'DBusWatch' to watch daemon and exit the process when the
daemon has gone.
Also find that current 'DBusClientCall' related classes are not
efficient and the code is reduntant. Re-design and implement the
inheritance.
This patch started with the goal of changing the uint level into
a string. The motivation for that was that the numeric constants
would have to be documented (which they weren't) and prevent
us from easily changing the Logger::Level later on.
I did change them once today (see previous commit about reordering the
SHOW level) and really had the problem that a not-restarted
syncevo-dbus-server couldn't talk to a recompiled client.
Using strings avoids that problem.
While changing uint to string I had to make the change twice, once in
an entirely unnecessary new and the pointer definition. Replaced the
shared_ptr with normal members, throughout the code.
Then I wondered about the use of the () operator on these signal
watches. At the point of use it was not obvious what that did. Turns
out that this activates the watch. In contrast to making a remote
function call or emitting a signal, I find the use of the operator
inappropriate for watches. Replaced with an explicit activate()
method.
While looking at the implementation, I found that this operator and
destructors had been duplicated. Instead of fixing it in all copies, I
moved the common code into a SignalWatch<> base template,
parameterized with the type of callback that it deals with.
When the command line tool shut down while it had started a session,
for example because of a fatal local error, shutdown failed with a
"virtual function called - Aborted" error.
The root cause is that RemoteDBusServer installs a shared_ptr to
is RemoteSession in g_session, which remains around after the
server was already destructed. Then during shutdown that global
variable is destructed, which tries to access the m_server reference,
which is already gone.
Two solutions:
- always ensure that g_session is cleared when the server is destructed
- use a weak reference in g_session
For no particular reason this patch uses the weak reference approach.
Perhaps because explicit cleanup code in destructors is so old
fashioned. <shrug>
This patch changes what is shown to the user and how the user
interacts with the command line. Details below.
"--use-daemon [yes/no]" implies that yes/no is optional (square
brackets!), with "yes" being the option that could be expected for a
plain "--use-daemon" parameter. But the implementation always expects
a "yes" or "no" parameter. The original format suggested was
"--use-daemon[=yes/no]"
This patch switches to that format, changes --use-daemon into --daemon
(to be consistent with --keyring) and enables the same format for
--keyring. Although not documented, 0/f/false and 1/t/true are also
accepted. Because the value becomes part of the parameter, m_argc checks
had to be adapted.
The documentation for "--use-daemon" was inserted in the middle of the
"--keyring documentation".
"Parameter not set" has to be available to the Cmdline caller in the
command line too, in addition to true/false. This was done originally
with a string which is empty, "yes" or "no". Using a tri-state
Cmdline::Bool class makes this a bit more explicit and removes the
ambiguity around what useDaemon() returns (values weren't documented).
When running without --daemon and no daemon available, the
first lines of output were:
ERROR: org.<cryptic error>
[INFO] itodo20: inactive
....
=> The command line should fall back *silently* to running in-process.
=> Error messages should be formatted and logged as such, using SE_LOG_ERROR().
Old code might not have done that and we need to preserve that for compatibility
with frontends, but new code should use [ERROR] as output.
=> D-Bus error messages (as in the case above) must have some user (and developer!)
comprehensible explanation what went wrong. The D-Bus error class itself
is no enough.
Although I haven't tested it, I suspect that the code also would have
re-run the operation in-process after the D-Bus server already
executed it and failed.
I rewrote this so that a check for "daemon available" without error messages
is done first before committing to using the daemon. Once that decision is made,
the command line will not fall back to in-process execution.
Rewrote several error messages. Telling a user of a distro's binary to
"re-configure" is misleading (he didn't configure himself).
"can't" => "cannot", punctuation changes. Not sure whether is always an
improvement, comments welcome.
Comment on coding style: I've used "if ()" instead of "if()" because that is
the GNU recommendation.
Why was the output level temporarily increased to DEBUG when using the
daemon? That might have been useful during development, but the final
version should run with INFO level again, like it does in non-daemon
mode. Uncommenting the lines which change the level...
This leads to the question of "which messages should be sent across
D-Bus at all"? Long term clients should decide, but short term we
should reduce the level to INFO and lower to reduce overhead.
Starting a sync of "foo@bar" is possible and leads to a server
which has "foo" as m_configName, which is the unnormalized name
passed to it. The new "configName" property must return the
normalized name, otherwise matching it against other normalized
names will fail.
Normalized config names can be compared literally, no need (and
perhaps wrong later on!) to do this case-insensitive.
Removed redundant it == m_sessions.end() check (must be true after
leaving the while() loop) and added punctuation to the message.
It is debateable whether "--monitor foo" without foo running should
succeed (zero return code) or indicate a failure. Let's go with
what the code currently does: no error indicated in the return code.
Implement cmdline with support of dbus server. To enable cmdline
with dbus server, use the option '--use-daemon yes/no' in case that
you enable dbus service when configuration.
In a typical scenario, a new session is created for the purpose of
execution of arguments. It is scheduled with other sessions but with
a highest priority. Once it becomes active, command line call
'Session.Execute', a newly added method to execute command line
arguments.
The config name of a session should be known for dbus clients like
command line. A new property 'configName' is added in the properties
when calling 'Session.GetConfig'.
CTRL-C handling are processed once executing a real sync to dbus
server. It is mapped to invoke 'Session.Suspend' and 'Session.Abort'.
The meaning of '--enable-dbus-service' is expanded accordingly.
'--status' without server means printing all running session in the
dbus server.
'--monitor' could accept an optional config name. If one is given,
only attach to a session of that config, otherwise print an error.
If none is given, pick the first.
Cmdline and SyncContext standard output should always go
through a variable that can be pointed towards the actual
output channel.
Cmdline already had m_out for that, but it wasn't used
everywhere. Fixed.
SyncContext now has setOutput()/getOutput() and that is
used in SyncContext.cpp. It is not set anywhere yet.
To catch incorrect use of cout or cerr inside SyncEvolution,
the SyncEvo namespace defines its own cout and cerr which cannot
be used like std::cout/cerr, thus triggering compiler
errors. Use "std::cout/cerr" when necessary.
