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.
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.
Redirection of stdout was not activated and thus output of
synccompare was not sent via LogOutput messages.
Activating it shows that more work is necessary, although
it works in principle. Problems: line breaks are inserted
into long synccompare output and lines are swallowed.
Also, cout should not be used by the server itself. Instead
use the logging facilities.
The recent patch which moves setTransportCallback() into
createTransportAgent() as incomplete, it didn't adapt
DBusSync::createTransportAgent().
As a reminder, this patch also updates the createTransportAgent()
comments.
Previously, plain strings were used. With DBusObject instead
the C++ bridge code recognizes the special meaning of those
and correctly sends them over D-Bus as object paths.
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.
When incoming data type is 'vCalendar1.0', alarms are converted
into VALARM without 'ACTION' field. This makes Evolution report
'unknown action to be performed'. Add 'DISPLAY' as the 'ACTION'
value if it has not.
requestStart time was used to detect transport timeout, which was only
set inside send(), however it is also possible that the transport
timeout during connect phase (sending a 'Connect' cmd but no response).
The original creating the transport and remembering to set the callback approach:
1) For ObexTransport, the callback need to be set inside
createTransportAgent, because the 'connect' process might timeout.
2) It is error prone to remember setting the callback after
creating each transport.
Now setTransportCallback is integrated inside createTransportAgent.
"MmMmMm" was interpreted as "0M0m0M0m" = 0 seconds. This is not
entirely unreasonable, but perhaps a bit too technical interpretation
of valid input. QA considered it invalid.
So let's be a bit more strict, which has the advantage that more user
typos are found. Now at least one digit must come before each unit
character.
When libnotify was not found, syncevo-dbus-server was silently
compiled without notifications, which happened in the MeeGo
snapshot .rpms due to missing build dependencies.
This patch introduces a --enable/disable-notify switch and aborts if
the feature is on (by default or explicitly requested) and the
development files are missing.
Also document this in README.packagers.
This gets rid of MuxWindow (and the related icon code and images)
for good: Now a specially named GtkToolbar and buttons inside it get
a special treatment from meego gtk theme engine, and end up looking
like Mx window title bar
The meego build will look a bit odd with other gtk themes, though
When the configuration widget is built:
* start a session
* set temporary config
* run CheckSources, show all sources that are usable
This should fix
MB #9961: memo not disabled by default on meego
MB #9303: User can set Memo-URI in First server settings
The move to D-Bus changed the error code for "timezone cannot be
retrieved because it doesn't exist" from
E_CALENDAR_STATUS_OBJECT_NOT_FOUND to
E_CALENDAR_STATUS_INVALID_OBJECT. That latter error code was not
recognized by the code and caused e_cal_check_timezones() to fail with
that error ("Could not retrieve calendar time zone").
This patch fixes the problem by recognizing both the old and the new
error code.
There was an intentional delay of 5 seconds in open() of both calendar
and contacts, followed by a retry of opening the database. This was
meant for the cases where a new database was created ("file://"
prefix) and couldn't be opened just yet, as seen in some older
Evolution releases (and perhaps still relevant today).
This delay hurts the sync-ui when it calls CheckSource() on the
defined, but non-existant "memo" database on MeeGo. This patch
avoids the delay and retry in that case.
The templates weren't really being sorted, my test case just
happened to be correct... This commit fixes sorting and should
make the best template get "automatically" selected.
This is the correct(normal) behaviour in some phones, should be taken
the same as request done. If this happens during sending a 'Get', the
wait() will block forever because it checks 'm_obexReady' or
Transport failed.
Added "--status without config name" comment to help output.
Changed the "No running session(s) just now" into a full sentence,
with a slightly more "positive" twist to it:
"Background sync daemon is idle."
Exceptions thrown inside Cmdline::run() were reported after leaving
and destructing CmdlineWrapper, and thus never reached the client
because that's what CmdlineWrapper does.
Fixed by adding a try/catch block in CmdlineWrapper::run().
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.
In MeeGo, the output is redirected to a log file. We don't want that
(output includes INFO message of syncs and even used to include DEBUG
messages!), so better redirect to /dev/null.
This is relevant when running in the background:
- less D-Bus traffic, in particular no DEBUG messages which
are not printed by the (current) clients
- less work done formatting these messages for stdout which
doesn't go anywhere (/dev/null) or worse, gets recorded
in a log file
Long term, a more intelligent solution would be good:
- for interactive debugging, add command line option
- for D-Bus, let clients choose
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.
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 new SHOW level is not more severe than ERROR. It is more like
INFO, with the only difference being different formatting of the
output.
Making it more severe as in the original code had the effect that the
"level <= ERROR" check in SyncContext.cpp, which detects the first
error message, incorrectly recorded the first SHOW message as the
"first error encountered".
"level == ERROR" would have fixed this, but <= is better in case that
we later rearrange error levels and perhaps a "REALLY_FATAL_ERROR" (not
likely, but who knows...).
On AMD64, iterating over a va_list destroys it. So after the
m_parentLogger.messagev() call "args" is invalid and using it
again in StringPrintfV() caused a segfault.
Need to copy and free the va_list.
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.
