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>
Raising exceptions via throwError() looses the original source code
location information. Fixing that by explicitly passing that
information as additional parameter, created with the preprocessor
macro SE_HERE.
Several files included the complex SyncContext.h only because needed
throwError(). A better place for the revised implementation is the
Exception class which used to be in util.h, now Exception.h.
Simplifying the include statements exposed indirect include
dependencies and removed "using namespace std" from the compilation of
some source files which happened to rely on it. We want to get rid of
that name space polution, so fix the code instead of adding it back.
When 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.
Diagnostic Log and Trace (DLT) manages a sequence of log messages,
with remote controllable level of detail. SyncEvolution optionally
(can be chosen at compile time and again at runtime) uses DLT instead
of its own syncevolution-log.html files.
Traditionally, glib messages were caught via LogRedirect.
Later, they got redirected by installing a handler. That change
disabled the message filtering for known harmless error messages
in LogRedirect, which caused messages from folks to show up
in TestCmdline D-Bus tests randomly, depending on the timing.
Reestablish the filtering by checking it in the glog callback.
To minimize changes, keep the error registry in LogRedirect.
It must be thread-safe now.
Logging must be thread-safe, because the glib log callback may be
called from arbitrary threads. This becomes more important with EDS
3.8, because it shifts the execution of synchronous calls into
threads.
Thread-safe logging will also be required for running the Synthesis
engine multithreaded, to overlap SyncML client communication with
preparing the sources.
To achieve this, the core Logging module protects its global data with
a recursive mutex. A recursive mutes is used because logging calls
themselves may be recursive, so ensuring single-lock semantic would be
hard.
Ref-counted boost pointers are used to track usage of Logger
instances. This allows removal of an instance from the logging stack
while it may still be in use. Destruction then will be delayed until
the last user of the instance drops it. The instance itself must be
prepared to handle this.
The Logging mutex is available to users of the Logging module. Code
which holds the logging mutex should not lock any other mutex, to
avoid deadlocks. The new code is a bit fuzzy on that, because it calls
other modules (glib, Synthesis engine) while holding the mutex. If
that becomes a problem, then the mutex can be unlocked, at the risk of
leading to reordered log messages in different channels (see
ServerLogger).
Making all loggers follow the new rules uses different
approaches.
Loggers like the one in the local transport child which use a parent
logger and an additional ref-counted class like the D-Bus helper keep
a weak reference to the helper and lock it before use. If it is gone
already, the second logging part is skipped. This is the recommended
approach.
In cases where introducing ref-counting for the second class would
have been too intrusive (Server and SessionHelper), a fake
boost::shared_ptr without a destructor is used as an intermediate step
towards the recommended approach. To avoid race conditions while the
instance these fake pointers refer to destructs, an explicit
"remove()" method is necessary which must hold the Logging
mutex. Using the potentially removed pointer must do the same. Such
fake ref-counted Loggers cannot be used as parent logger of other
loggers, because then remove() would not be able to drop the last
reference to the fake boost::shared_ptr.
Loggers with fake boost::shared_ptr must keep a strong reference,
because no-one else does. The goal is to turn this into weak
references eventually.
LogDir must protect concurrent access to m_report and the Synthesis
engine.
The LogRedirectLogger assumes that it is still the active logger while
disabling itself. The remove() callback method will always be invoked
before removing a logger from the stack.
The method became obsolete when introducing fork+exec for local
syncing. Before that, the method was used to remove unsafe loggers in
the child's process. Now exec() does that for us.
Changing the process' name, even temporarily, is not thread-safe
and needs to be avoided. Instead pass the additional parameter
explicitly via the MessageOptions container.
Logger::formatLines() and LogStdout::write() are passed the process
name as "const std::string *" pointer, with Logger::getProcessName()
acting as fallback for NULL. Calling that is delayed as long as possible,
to avoid making the string copy when not necessary.
The previous implementation of formatLines() used a std::string
reference, but only used the content to determine whether to include
the current process name - probably not what was intended, but harmless
because either the empty string or the current name were passed.
The number of parameters for messagev has grown unreasonably, and more
parameters will be needed soon (explicit process name). Better store
the parameters in a struct and pass around a pointer to such a struct
instance. The code becomes shorter and new parameters can be passed
through intermediate functions without modifying them.
Passing an explicit Logger instance to the SE_LOG* macros was hardly
ever used and only made the macros more complex.
The only usage of it was in some backends, which then added a prefix
string automatically. The same effect can be achieved by passing
getDisplayName(). Exception handling no longer needs a Logger instance,
the prefix alone is enough.
The other intended usage, avoiding global variables for logging by
passing a logger known to the caller, was not possible at all.
To make prefix handling more flexible, it is now passed as a "const
std::string *" instead of a "const char *".
When realloc() returns NULL, the already allocated buffer remains
available. Previously we discarded (and leaked!) it. Now we track
whether that memory already contains parts of the next log message
and use that when realloc fails. The dgram will then get discarded
without reading the rest.
When reading from a stream, realloc will only be called once to
get the initial buffer. When that fails, we give up.
When processing stdout from syncevo-local-child in
syncevo-dbus-helper, the LogRedirect class was invoked recursively and
tried to print the same stdout data repeatedly until the
syncevo-dbus-helper crashed due to the infinite recurssion.
The output shouldn't have been sent via stdout in the first place
(will be fixed separately), but nor should LogRedirect have crashed.
Fixed by swapping the data into a temporary buffer and thus not using
it again.
Traditionally, the "syncevolution" command line tool mixed its
INFO/ERROR/DEBUG messages into the normal stdout. This has the major
drawback that error messages get lost during operations like
syncevolution --export - @default addressbook | grep "John Doe"
Now anything which not the expected result of the operation is always
sent to stderr. Obviously this includes ERROR messages. INFO and DEBUG
are harder to decide. Because they usually convey meta information
about the running operation, they are also sent to stderr.
This changes the behavior of
syncevolution --run foo eds_event | less
"less" will capture only the following output:
vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
Local data changes to be applied during synchronization:
*** eds_event ***
no changes
Synchronization successful.
Changes applied during synchronization:
+---------------|-----------------------|-----------------------|-CON-+
| | LOCAL | REMOTE | FLI |
| Source | NEW | MOD | DEL | ERR | NEW | MOD | DEL | ERR | CTS |
+---------------+-----+-----+-----+-----+-----+-----+-----+-----+-----+
| eds_event | 0 | 0 | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
| two-way, 0 KB sent by client, 0 KB received |
| item(s) in database backup: 2 before sync, 2 after it |
+---------------+-----+-----+-----+-----+-----+-----+-----+-----+-----+
| start Wed Apr 11 14:34:11 2012, duration 0:03min |
| synchronization completed successfully |
+---------------+-----+-----+-----+-----+-----+-----+-----+-----+-----+
Data modified locally during synchronization:
*** eds_event ***
no changes
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
To get the traditional behavior with bash or sh as shell, use
syncevolution --run foo eds_event 2>&1 | less
Some "normal" output in error messages was changed to INFO and thus
now appears on stderr after the corresponding ERROR message. Empty
lines which were used to separate different parts now also have a tag,
for the sake of consistency with the surrounding output.
The main implementation change is in LogRedirect, which now decides
whether LogStdout shall send the message to stdout or stderr. This has
the desired effect in the "syncevolution" binary, which uses
LogRedirect. Other binaries inherit the same change, although there it
usually doesn't matter.
The Cmdline unit testing mirrors this change in the way how it stores
and checks Cmdline output and also was adapted to the prefix changes.
client-test now uses the same process initialization as all other
SyncEvolution binaries and thus uses the new glib log handler.
Also removes some code duplication.
The glib output redirection test became a bit redundant, but let's
keep it. For that we need to restore the default glib log handler
while that test runs.
Removed all the "using namespace std;" statements from the header
files and made corrosponding changes. Where needed added "using
namespace std;" statement to the implementation files.
When capturing lines from stdout or stderr, strip the trailing newline
before calling message() because that introduces an additional newline
by itself.
The new call is meant to be used after forking: in such a situation,
redirected stdout/stderr output can be read by both the parent and the
child, which leads to a race condition and makes it non-deterministic
where that output will be handled. redoRedirect() closes the child's
connection to the parent's redirection and sets up its own instead.
A nightly test run showed that process() was freeing memory
when called indirectly from restore(). The protection against
recursive processing wasn't working because m_processing wasn't
set by restore(). Added it to that and some other high-level
calls which might end up with a recursive call into process().
The test failed only because the test's own introduction text on
stdout ended up being redirected. Flushing it before running the
test avoids that.
The flushing was added to both the client runner (because some other
tests also accidentally redirected the output, and it should be
made visible right away) and to the "glib" test (because it shouldn't
depend on flushing in the runner).
An additional parameter allows the caller to decide whether
LogRedirect writes into stdout (original behavior) or opens
a specific file. Useful for client-test, which can avoid
one layer of loggers that way.
It was already possible to keep known errors out of stdout with
the SYNCEVOLUTION_SUPPRESS_ERRORS env variable. Now the same is
also possible at runtime via addIgnoreError(). This is useful
for backends which know about some of their internal errors that
they might produce indirectly (tools or libs being called, etc.).
When running client-test, the normal stdout text is copied into
a .log text file named after the currently running test. The output
of the child process should also go there.
This patch achieves that by being more selective about which loggers
in the stack of loggers it removes: LogDir must be removed because it
writes per-process Synthesis .html files, LoggerStdout must remain for
.log. The difference is abstracted away behind a new
Logger::isProcessSafe() method, which tells LocalTransportAgent whether
sharing the logger instance between processes is okay.
The logic of printing all text containing the word "error" to stdout
can clutter up the output with know errors or simply false
positives (text which is not a real error).
Setting SYNCEVOLUTION_SUPPRESS_ERRORS to a newline-separated list
of lines will hide all text containing one of these lines. The text
is still logged.
When an application name is set (as introduced for client-test
by some recent changes), testing against "** (process:" as glib
prefix is insufficient, because "process" will be replaced by
the actual application name.
This was detected by the unit tests. This patch fixes the problem
by relaxing the glib testing (check for "** (" prefix and " **:")
and extends the unit test.
The tests were still expecting the output to appear at level INFO.
They also didn't properly flush the (now) buffered stdout logging.
Added a test which check for this particular feature.
When using streams, the caller of process() expects that once
the function returns, the streams were read until their end,
so that waitpid() will return eventually without deadlocking.
When an error occurs, this is not guaranteed, so we should
better return via an exception. Execute() was already
prepared for that.
As mentioned in the previous commit, SOCK_DGRAM does not allow us
to detect that the write has closed its ends of the socket pair.
This patch switches to using SOCK_STREAM, which gives us that
feature.
pipe() would also have worked, but with SOCK_DGRAM less code needs to
be changed and it is possible to switch back to SOCK_DGRAM (for
experiments) via a define.
The logic of processing data is different when in "streaming" mode:
process() always fills the chunk of default size 1024 with as much
data as possible, then processes it. For stdout this works well,
because line breaks are preserved before feeding into the logging
system.
With stderr there's an increased risk that lines a split, but right
now no code uses streaming mode for stderr, so that doesn't matter
at all.
LogRedirect is intentionally written around lossy UDP communication:
when system() starts a command that produces lot of output, the kernel
drops data instead of deadlocking us (system() command blocked in write,
our process blocked waiting for command).
When we know that a command might produce much output *and* we redirect
output, we need a more intelligent replacement for system(). This patch
introduces Execute() for that purpose.
It reuses much of LogRedirect code. LogRedirect has a new constructor
which creates the pipes and then handles the output like it normally
would, except that the pipes are Unix domain datagram sockets and the
instance itself doesn't modify the global state of the parent (doesn't
register itself as logger, doesn't touch FDs 1 and 2).
Unix domain datagram sockets seemed like a combination of a lossless
connection with data boundaries, which has the advantage that the
existing process() code can be reused. When experimenting with
LogRedirect originally, it was observed that domain sockets did
deadlock when writing more than reading.
The problem with this patch is that Unix domain sockets do not flag
that there is no writer anymore. Therefore LogRedirect::process()
cannot detect when it should return.
syncevo-dbus-server is the first usage of LogRedirect which also
redirects stdout. In turn, the main usage is catching of "synccompare"
output. It turns out that the assumption for stderr so far that writes
are atomic doesn't hold for stdout of synccompare. Lines often are
interrupted in the middle.
In addition, SE_LOG(SHOW) always inserts a newline.
This patch deals with this by treating data received for stdout as a
data stream. It buffers incomplete lines and appends the rest of the
line when the next chunk comes in.
This delays printing such an incomplete line. flush() is introduced
to get that last, incomplete line logged.
For stderr, keeping text in memory instead of printing it as soon
as received is considered undesirable, therefore that code path
is not changed.
Noticed this while investigating a segfault inside the logging system:
the signal handler should not try to log something, because the logging
system itself might be too broken to do something. Better shut down
as quickly as possible.
The "GConf Error: Failed to contact configuration server..." error
message was assigned the Logger::DEV severity and thus never shown to
users. At low loglevel settings it probably also never made it into a
log file, if one was written at all.
This patch raises the severity of all output lines which contain the
word "error". Clearly this only works for non-localized error
messages, but hopefully that's the majority of the relevant error
messages on stderr.
The intention is to get rid of the historic and inconsistent
naming of some classes and their corresponding files:
* EvolutionSyncClient = class derived from Funambol's SyncClient,
* SyncEvolutionConfig = SyncEvolution's config
With the strict 'namespace SyncEvo' and the syncevo/ path prefix for
most header files it is no longer necessary to have "SyncEvolution" or
"Evolution" in the names. This patch thus renames as follows:
EvolutionSyncClient => SyncContext
EvolutionSmartPtr => SmartPtr
SyncEvolutionCmdline => Cmdline
SyncEvolutionConfig => SyncConfig
SyncEvolutionUtil => util
The former EvolutionSyncClient always had a role that went beyond just
running a sync, for example it also provided config access. With the
upcoming server support it also won't be just a client. Thus the new
name "SyncContext".
The 'syncevo/' prefix is used throughout the code now.
removed whenever the prefix made it clear that the file belongs
to SyncEvolution. This helps finding incorrect include paths.
Quotes should be used exclusively for SyncEvolution files which don't
have a specific prefix yet (test.h, config.h) to help identifying
them.
Added syncevo/declarations.h, which has
This is now used for all SyncEvolution source files, except
for the GTK UI, which is written in plain C. In the library
it helps to avoid name clashes.
The reason for using defines instead of spelling out "namespace SyncEvo"
is twofold:
1. if that should ever become necessary, it is easier to
rename the namespace via configure options by changing
the define
2. editors don't indent the whole file content
Patrick Ohly
Chris Kühl
Chen Congwu