Re: [SyncEvolution] syncEvolution configuration structure and global properties

[Chris Kühl]

On Wed, Jun 29, 2011 at 6:22 PM, Patrick Ohly  wrote:
> [Openismus has some problems sending mail to the public list. Sorry for
> only sharing one half of the conversation.]

Oops. Actually I switched my personal address and meant to send this
to the list. :-P Thanks for the redirect.

>
> On Wed, 2011-06-29 at 18:06 +0200, Chris Kühl wrote:
>> Hi,
>>
>> I was wondering if the documentation found here[1] is still true. I
>> ask because when running the code below form test-dbus.py, I do not
>> get a top level.config.ini file.
>
> Yes, the documentation is still valid.
>
>> The "defaultPeer" value lives in default/peers/foo/config.ini, while
>> I'd expect it to be in the .config/syncevolution directory because
>> it's global.
>
> Your expectation is correct. Jussi reported something similar recently:
> https://bugs.meego.com/show_bug.cgi?id=19464
>

Yeah, I seem to be bitten by the same bug. I'll follow up with any
progress there.

>> This is triggered in the test TestMultipleConfigs.testSharing. Anyone
>> see anything obvious?
>
> I'll have a look.
>
> Damn, the broken D-Bus testing integration really is starting to hurt.
>

Seems like a good time to get things fixed then. ;-)

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] D-Bus testing

[Chris Kühl]

On Thu, Jun 30, 2011 at 12:58 AM, Patrick Ohly  wrote:
> On Wed, 2011-06-29 at 13:16 -0700, Patrick Ohly wrote:
>> Hello Chris!
>>
>> One comment on your commit in the test-dbus branch:
>>
>> ---
>> commit da74d2aa1b671c0794cd88a854e137ae93ee102a
>> Author: Chris Kühl 
>> Date:   Tue Jun 28 14:48:56 2011 +0200
>>
>>     test-dbus: Remove check for only one report.
> [...]
>> Which test has the problem with too many reports? Does it use its own
>> XDG root?
>
> I found that the TestSessionAPIsReal tests using the real config
> "dbus_unittest" failed when too many reports where stored. I don't know
> how this was meant to work; one possible solution is to configure that
> config so that at most one report is retained. I've changed test-dbus.py
> accordingly and also removed the mangling of the "database" property:
>
>    """ All unit tests in this class have a dependency on a real sync
>    config named 'dbus_unittest'. That config must have preventSlowSync=0,
>    maxLogDirs=1, username, password set such that syncing succeeds
>    for at least one source. It does not matter which data is synchronized.
>    For example, the following config will work:
>    syncevolution --configure --template  \
>                  username= \
>                  password= \
>                  preventSlowSync=0 \
>                  maxLogDirs=1 \
>                  backend=file \
>                  database=file:///tmp/test_dbus_data \
>                  databaseFormat=text/vcard \
>                  dbus_unittest@test-dbus addressbook
>                  """
>

Thanks, this worked for that issue. I've removed that commit from the
test-dbus branch now.

> This fixes two out of seven failures that I was seeing, after I merged
> Chris' fix for the templates.
>
> Here's a temporary analysis of the failure. I'll continue to look into it.
>
> FAIL: read templates
> Fails if the host has paired Bluetooth devices because they get included in 
> the list.
> Cannot be fixed easily.
>
> FAIL: a second session should not run unless the first one stops
> AssertionError: ['session /org/syncevolution/Session/12604096081309383605 
> idle', 'session /org/syncevolution/Session/2552452311309383604 done'] != 
> ['session /org/syncevolution/Session/12604096081309383605 idle', 'session 
> /org/syncevolution/Session/12604096081309383605 ready', 'session 
> /org/syncevolution/Session/2552452311309383604 done']
> "Ready" before "Done"? Might be harmless, need to check.
>
> FAIL: Executes a real sync with no corresponding config.
> AssertionError: dbus.UInt32(500L) != 10500
> "No such config" should be a local error, not a remote one. Must be fixed in 
> syncevo-dbus-server.

Ok. Good to know. I'm using memotoo for testing and assumed the test
was intolerant to remote configurations, thus my commit to the
test-dbus branch allowing error code 500. I'll remove that commit and
see If I can fix this.

>
> FAIL: set up auto sync, then check that server restarts
> AssertionError: '/tmp/local/libexec/syncevo-dbus-server' != 
> '/home/pohly/work/syncevolution/src/syncevo-dbus-server'
> Caused by a mix-up between paths: as long as syncevo-dbus-server is only once 
> in PATH, this doesn't occur.
>
> FAIL: master must detect a hanging child
> Sync succeeds although it shouldn't - timing changes?
>

Ok, thanks for the insight.

Would it be possible for you to make a step by step list of what you
do to set up your environment for testing? I think this might help us
find why we are getting different results for the "defaultPeer" issue,
for example.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] D-Bus testing

[Chris Kühl]

On Fri, Jul 1, 2011 at 3:57 PM, Patrick Ohly  wrote:
> On Fri, 2011-07-01 at 12:24 +0200, Chris Kühl wrote:
>> Would it be possible for you to make a step by step list of what you
>> do to set up your environment for testing? I think this might help us
>> find why we are getting different results for the "defaultPeer" issue,
>> for example.
>
> The only setup required in the host is the configuration of the
> "dbus_unittest" sync config. I added some comments about that to the
> TestSessionAPIsReal class, which is the only class which should depend
> on it. In particular the shared properties tests should be independent
> of that.
>

Ok, and yes, that configuration change fixed at least one failure. The
additional comments are very welcome.

> Don't run syncevo-http-server. It is not used by D-Bus testing.
>

Yes, this was cleared up in a previous mail.

> Another possible difference might be how the script gets invoked. I
> always run them in the out-of-tree build tree, inside "src". I run with
>  PATH=.:$PATH /test/dbus-test.py
>

If I've cleaned my system-wide directories of the syncevolution files
I find that this doesn't work. I've got to additionally add the
environment variables for the template and xml files. For me
specifically it's...

SYNCEVOLUTION_XML_CONFIG_DIR=/home/chris/checkout/meego/syncevolution/src/syncevo/configs/
SYNCEVOLUTION_TEMPLATE_DIR=/home/chris/checkout/meego/syncevolution/src/templates/
PATH=.:$PATH /home/chris/checkout/meego/syncevolution/test/test-dbus.py
-v


With those things are fine... well, besides the failures we're trying to fix.
.
> This copies /test/dbus-test into ./dbus-test, which is relevant
> for some tests which expect a well-defined xdg root.
>
> Your patch to use different names for the reference files and the local
> copy makes sense. I don't normally notice such things because I never
> run anything other than autogen in my source tree ;-}
>

Hopefully that can avoid a bit of confusion in the future for some poor soul.

> In related news, I updated the test scripts so that D-Bus testing can be
> run as part of the regular nightly testing, in each of the chroots where
> tests are run. I triggered a test run, but network problems broke it
> because the SyncEvolution sources couldn't be updated from gitorious.
> Otherwise I would have been able now to point to
> http://runtests.syncevolution.org and reports which contain D-Bus test
> results.
>

This would be very nice to have. Much less chance it'd fall into such
disrepair if it were run along side the other tests every night.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] D-Bus testing

[Chris Kühl]

On Mon, Jul 4, 2011 at 10:03 PM, Patrick Ohly  wrote:
> On Wed, 2011-06-29 at 15:58 -0700, Patrick Ohly wrote:
>> FAIL: a second session should not run unless the first one stops
>> AssertionError:
>> ['session /org/syncevolution/Session/12604096081309383605 idle',
>> 'session /org/syncevolution/Session/2552452311309383604 done'] !=
>> ['session /org/syncevolution/Session/12604096081309383605 idle',
>> 'session /org/syncevolution/Session/12604096081309383605 ready',
>> 'session /org/syncevolution/Session/2552452311309383604 done']
>> "Ready" before "Done"? Might be harmless, need to check.
>
> Seems to occur only when running all tests. A bit mysterious,
> syncevo-dbus-server should be restarted for each test.
>
> Perhaps this has something to do with the mock net.connman.Manager
> implementation that some of the tests created on the D-Bus session bus.
> I saw some "TypeError: 'NoneType' object is not iterable" errors caused
> by it, because its GetProperties() method doesn't have a final "else"
> clause and just returns without value. Will look further.
>
>> FAIL: Executes a real sync with no corresponding config.
>> AssertionError: dbus.UInt32(500L) != 10500
>> "No such config" should be a local error, not a remote one. Must be
>> fixed in syncevo-dbus-server.
>
> commit cb9b7c3e2b3ac58aec4438019853207521a164f4
> Author: Patrick Ohly 
> Date:   Mon Jul 4 21:05:52 2011 +0200
>
>    error handling: recognize local errors again
>
>    Commit 3f1185, contained in 1.1.99.3, changed
>    SyncContext::throwError() so that it throws a StatusException with
>    STATUS_FATAL. Previously a runtime exception was thrown, which
>    Exception::handle() recorded as a local error.
>
>    This commit fixes that regression by throwing a STATUS_FATAL +
>    LOCAL_STATUS_CODE, which restores the traditional result of
>    throwError().
>
>    Found by test-dbus.py TestDBusSyncError.testSyncNoConfig.
>

Ok, so this is the proper place to do this change. I was doing this in
the server on my branch. I've removed that commit now.

>> FAIL: master must detect a hanging child
>> Sync succeeds although it shouldn't - timing changes?
>
> commit 083984b1cf1e990f45dc05179f2e9bcd2ff47cda
> Author: Patrick Ohly 
> Date:   Mon Jul 4 21:25:23 2011 +0200
>
>    D-Bus testing: removed invalid TestLocalSync.testTimeout
>
>    Commit 1a40a29 (added after 1.1.99.4) removed timeouts in the local
>    transport, reasoning that such timeouts only make sense in unreliable
>    transports and only cause problems (like premature aborts).
>
>    Therefore the TestLocalSync.testTimeout which tested the old behavior
>    became invalid. Removed completely.
>

With these changes, I'm down to only one failure and errors associated
to the global property issue. Also, out of curiosity, I rebased me
dbus-server-reorganization branch onto master and ran the test. It
returns the exact same results as the master branch. So, everything
seems to be alright there thus far. Unfortunately, the "same results"
means that it also has the defaultPeer issue which I'm trying to find
a fix for.

Also, feel free to merge or cherry-pick any remaining commits from the
test-dbus branch at this point.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] D-Bus testing

[Chris Kühl]

On Tue, Jul 5, 2011 at 9:48 AM, Patrick Ohly  wrote:
> On Tue, 2011-07-05 at 09:40 +0200, Chris Kühl wrote:
>> With these changes, I'm down to only one failure
>
> The one about TestDBusSession.testSecondSession, right?

Correct.

> I'm looking at
> it now. As I suspected, the Connman class is to blame. The mock class
> remains active and then messes with the mainloop, which confuses the
> other test.
>
>> Unfortunately, the "same results"
>> means that it also has the defaultPeer issue which I'm trying to find
>> a fix for.
>
> See my other mail about how defaultPeer should be stored (gdb back
> traces, etc.).
>
> Note that you can set
>  debugger = "gdb"
> at the top of test-dbus.py to invoke an interactive debug session on
> syncevo-dbus-server while running a single test.
>

Yes, thanks. I'm aware of the debugger option. Working on it.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] D-Bus server reorganization

[Chris Kühl]

On Tue, Jul 5, 2011 at 3:37 PM, Patrick Ohly  wrote:
> On Fri, 2011-06-24 at 13:11 +0200, Chris Kühl wrote:
>> One question I do have about style is what is the preferred file nameing
>> scheme. Some are in module-class-name form, others in module-ClassName
>> form, and still others in ClassName form. I'll be happy to make my new
>> files reflect the desired scheme.
>
> I forgot to answer this. I don't have a strong opinion about this. I'm
> not a fan of the "one class per file" paradigm. IMHO it leads to a
> needless proliferation of files. I prefer to group related classes in a
> single header and implementation file.
>

There are definitely some classes that can be moved merged with there
larger associated files. AutoTerm is one that comes to mind; it's only
used in DBusServer. Also, some of the utility type classes like TImer
and Timeout can easily be placed in the same file. I'd prefer to keep
non-utility classes that are used in multiple classes in there own
files, though.

> On the other hand, it is more obvious where code is to be found when the
> file name is the same as the class name, once one knows the class name.
>

In my view this is very helpful to get a quick overview of the program.

Of course, this also wins you a few seconds on compile time. After
doing a full compile on both the master and dbus-server-reorganization
branches, I then touched syncevo-dbus-server.cpp on both branches and
reran make from the src directory. It took ~17s to compile on master
and ~7 on d-s-r. A pretty nice time savings.

We have now gone from one extreme of having ~25 classes in one file
stretching > 6000 lines, to the other of having ~40 .h/.cpp files for
those same ~25 classes. I'm happy to merge a few of these but in
general I think keeping the classes in separate files is a good thing.

Shall I merge the obvious candidates?

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] D-Bus server reorganization

[Chris Kühl]

On Tue, Jul 5, 2011 at 5:02 PM, Patrick Ohly  wrote:
> On Tue, 2011-07-05 at 16:49 +0200, Chris Kühl wrote:
>> We have now gone from one extreme of having ~25 classes in one file
>> stretching > 6000 lines, to the other of having ~40 .h/.cpp files for
>> those same ~25 classes. I'm happy to merge a few of these but in
>> general I think keeping the classes in separate files is a good thing.
>>
>> Shall I merge the obvious candidates?
>
> No, don't bother. I'm fine to go along with whatever the developer
> working on a certain subsystem is comfortable with.
>

Alles klar. ;)

Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] D-Bus testing

[Chris Kühl]

On Tue, Jul 5, 2011 at 9:31 PM, Patrick Ohly  wrote:
> On Tue, 2011-07-05 at 12:02 +0200, Chris Kühl wrote:
>> On Tue, Jul 5, 2011 at 9:48 AM, Patrick Ohly  wrote:
>> > On Tue, 2011-07-05 at 09:40 +0200, Chris Kühl wrote:
>> >> With these changes, I'm down to only one failure
>> >
>> > The one about TestDBusSession.testSecondSession, right?
>>
>> Correct.
>
> Fixed, I believe:
>

I can confirm. I've just got this pesky global property error left
now. Wondering if it might be better to trudge ahead with getting the
dbus-server-reorganization branch merged and come back to this issue
later.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] D-Bus testing

[Chris Kühl]

On Wed, Jul 6, 2011 at 3:23 PM, Patrick Ohly  wrote:
> On Wed, 2011-07-06 at 14:58 +0200, Chris Kühl wrote:
>> I can confirm. I've just got this pesky global property error left
>> now. Wondering if it might be better to trudge ahead with getting the
>> dbus-server-reorganization branch merged and come back to this issue
>> later.
>
> I can't merge that branch before releasing 1.2, and I can't release 1.2
> without fixing that global property error first.
>

Ah, I see.

> I know that you want to continue with your real work. But the problem is
> that I simply can't seem to reproduce it myself, so I need help by
> someone who can.
>
> Perhaps we can meet on chat and compare notes about interactive debugger
> sessions?
>

Sure. Let's set up a time to do this. Tomorrow is fine with me... but
we can arrange this off-list.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

[SyncEvolution] dbus-server-reorganization progress

[Chris Kühl]

Hi,

I've been working on getting my dbus-server-reorganization branch on
par with master and I think I've almost achieved that. Currently they
are both seeing 3 failures and 2 errors. I see that the 2 errors are
probably addressed in bug #20990 as they start to appear only after
the recent autosync changes. The 3 failures seem to be due to issues
with the password property in temporary configs. I've pastebin'ed my
output in case it's helpful: http://pastebin.com/kRe0sPBK

I said "almost" on par because I've been having issues when building
with --disable-shared and --enable-static. When trying this I get an
error telling me to add -ldl to the LDFLAGS which I've done and it
does in fact seem to build correctly. However, with this, the backends
don't get registered and I, of course, get tons of problems when
running the tests. This issue exists in the first commit on my branch
which moved the dbus-server over to its own directory. However,
everything does seem to build and work fine with only --enable-static
specified which is what I've been using for running the tests.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] dbus-server-reorganization progress

[Chris Kühl]

>> I said "almost" on par because I've been having issues when building
>> with --disable-shared and --enable-static. When trying this I get an
>> error telling me to add -ldl to the LDFLAGS which I've done and it
>> does in fact seem to build correctly. However, with this, the backends
>> don't get registered and I, of course, get tons of problems when
>> running the tests. This issue exists in the first commit on my branch
>> which moved the dbus-server over to its own directory. However,
>> everything does seem to build and work fine with only --enable-static
>> specified which is what I've been using for running the tests.
>
> Did you notice the rules which add the backend's *Register.cpp files to
> the syncevo-dbus-server?
>
> src/Makefile.am (generated from Makefile-gen.am) contains:
> BACKEND_REGISTRIES = ... backends/.../*Register.cpp
>
> This is then added to syncevo-dbus-server via:
> CORE_SOURCES += $(BACKEND_REGISTRIES)
> syncevo_dbus_server_SOURCES = \
> ...
>        $(CORE_SOURCES)
>

Yes, I've got that in my src/dbus-server/Makefile-gen.am. In fact,
that's why I had to use the "-gen." That appears to give me what I
need in the generated Makefile.am but maybe I'm fumbling something.

> The easiest solution might be to keep linking the main
> syncevo-dbus-server in "src", but with all code compiled in a
> sub-directory into a helper library.
>

I attempted this but had the same results. Here is the patch I tried:
http://pastebin.com/L8BKtY7Z

Does that look like what you had in mind?

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] dbus-server-reorganization progress

[Chris Kühl]

On Fri, Jul 15, 2011 at 4:37 PM, Patrick Ohly  wrote:
> On Do, 2011-07-14 at 16:14 +0200, Chris Kühl wrote:
>> >> I said "almost" on par because I've been having issues when building
>> >> with --disable-shared and --enable-static. When trying this I get an
>> >> error telling me to add -ldl to the LDFLAGS which I've done and it
>> >> does in fact seem to build correctly. However, with this, the backends
>> >> don't get registered and I, of course, get tons of problems when
>> >> running the tests. This issue exists in the first commit on my branch
>> >> which moved the dbus-server over to its own directory. However,
>> >> everything does seem to build and work fine with only --enable-static
>> >> specified which is what I've been using for running the tests.
>> >
>> > Did you notice the rules which add the backend's *Register.cpp files to
>> > the syncevo-dbus-server?
>> >
>> > src/Makefile.am (generated from Makefile-gen.am) contains:
>> > BACKEND_REGISTRIES = ... backends/.../*Register.cpp
>> >
>> > This is then added to syncevo-dbus-server via:
>> > CORE_SOURCES += $(BACKEND_REGISTRIES)
>> > syncevo_dbus_server_SOURCES = \
>> > ...
>> >        $(CORE_SOURCES)
>> >
>>
>> Yes, I've got that in my src/dbus-server/Makefile-gen.am. In fact,
>> that's why I had to use the "-gen." That appears to give me what I
>> need in the generated Makefile.am but maybe I'm fumbling something.
>>
>> > The easiest solution might be to keep linking the main
>> > syncevo-dbus-server in "src", but with all code compiled in a
>> > sub-directory into a helper library.
>> >
>>
>> I attempted this but had the same results. Here is the patch I tried:
>> http://pastebin.com/L8BKtY7Z
>>
>> Does that look like what you had in mind?
>
> No, not quite. I wanted to avoid the need for the -gen.am trick in
> src/dbus-server by linking the syncevo-dbus-server in src, as before.
>

Ok, good. Having a Makefile-gen.am in src/dbus-server just for the
backends always felt wrong to me anyway.

> I checked out your branch and, after I couldn't figure out why autotools
> didn't link the registry files into syncevo-dbus-server, implemented my
> proposal. Have a look at dbus-server-reorganization-pohly.
>
> It starts with a commit which combines all the registry files into an
> utility library. Adding them as source to each executable was just plain
> laziness.
>
> Then the revised "move to dbus-server" patch adds a
> libsyncevodbusserver.la utility library in src/dbus-server which is used
> to link syncevo-dbus-server in src.
>

Unfortunately, I'm still getting the same issue. Here is the first error I get.

==
ERROR: testCredentialsRight (__main__.TestConnection)
TestConnection.testCredentialsRight - send correct credentials
--
Traceback (most recent call last):
  File "/home/chris/checkout/meego/syncevolution/test/test-dbus.py",
line 2258, in testCredentialsRight
self.setupConfig()
  File "/home/chris/checkout/meego/syncevolution/test/test-dbus.py",
line 2151, in setupConfig
self.session.SetConfig(False, False, self.config, utf8_strings=True)
  File "/usr/lib/python2.7/site-packages/dbus/proxies.py", line 140, in __call__
**keywords)
  File "/usr/lib/python2.7/site-packages/dbus/connection.py", line
630, in call_blocking
message, timeout)
DBusException: org.syncevolution.InvalidCall: invalid value 'file' for
property 'backend': 'not one of the valid values (virtual, calendar =
events, addressbook = contacts, todo = tasks, memo = memos = notes)'

I'm assuming this was working for you so I'm not sure what's going on on my end.

Like I said I still like the change so I'll merge it into my branch.

> I was undecided whether I should use hyphens in the library name. The
> other libraries don't use the, so I decided against it. Admittedly makes
> it harder to read.
>
> Speaking of naming, should src/dbus-server rather be src/dbus/server? It
> would be more consistent with "anything related to SyncEvolution D-Bus
> in dbus". Also plays nicer with tab completion of file names in the
> shell. I often ended up with dbus taking me to the "dbus"
> directory instead of completing to "dbus-server".
>

Sure, I'll do this. I think I'll also remove the 'syncevo-' from the
syncevo-dbus-server.* and syncevo-exceptions.* files but leave it on
the executable to be consistent with the other executables.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] dbus-server-reorganization progress

[Chris Kühl]

On Tue, Jul 26, 2011 at 5:39 PM, Patrick Ohly  wrote:
> On Mo, 2011-07-18 at 17:31 +0200, Patrick Ohly wrote:
>> On Mo, 2011-07-18 at 15:30 +0200, Chris Kühl wrote:
>> > On Fri, Jul 15, 2011 at 4:37 PM, Patrick Ohly  
>> > wrote:
>> > > On Do, 2011-07-14 at 16:14 +0200, Chris Kühl wrote:
>> > > No, not quite. I wanted to avoid the need for the -gen.am trick in
>> > > src/dbus-server by linking the syncevo-dbus-server in src, as before.
>> > >
>> >
>> > Ok, good. Having a Makefile-gen.am in src/dbus-server just for the
>> > backends always felt wrong to me anyway.
>> >
>> > > I checked out your branch and, after I couldn't figure out why autotools
>> > > didn't link the registry files into syncevo-dbus-server, implemented my
>> > > proposal. Have a look at dbus-server-reorganization-pohly.
>> > >
>> > > It starts with a commit which combines all the registry files into an
>> > > utility library. Adding them as source to each executable was just plain
>> > > laziness.
>> > >
>> > > Then the revised "move to dbus-server" patch adds a
>> > > libsyncevodbusserver.la utility library in src/dbus-server which is used
>> > > to link syncevo-dbus-server in src.
>> > >
>> >
>> > Unfortunately, I'm still getting the same issue. Here is the first error I 
>> > get.
>> [...]
>> > I'm assuming this was working for you so I'm not sure what's going on on 
>> > my end.
>>
>> I must admit that I pushed my proposal without enough testing. Oops.
>>
>> What's probably happening is this:
>> * the register classes are now all in a .a lib
>> * nothing depends on these object files when linking the executables
>> * they don't get into the executables
>
> That was indeed it.
>
>> *This* and not laziness in defining the rules must have been the reason
>> why previously, these classes were listed as source files of all
>> executables. The resulting object files are then always linked into the
>> executable.
>>
>> I'll check whether there is an easy fix. If not, then we'll have to go
>> back to compiling the sources files multiple times.
>
> ld's --whole-archive option is what we need. The only problem is that
> libtool "helpfully" reorders command line options it so that
> -Wl,--whole-archive -Wl,--no-whole-archive no longer wraps around.
> libsynccommon.la.
>
> Here's a potential solution:
>
> $ git diff
> diff --git a/src/Makefile-gen.am b/src/Makefile-gen.am
> index 11bff8f..c47064a 100644
> --- a/src/Makefile-gen.am
> +++ b/src/Makefile-gen.am
> @@ -132,11 +132,11 @@ endif
>  # SYNCEVOLUTION_LDADD will be replaced with 
> libsyncebook.la/libsyncecal.la/libsyncsqlite.la
>  # if linking statically against them, empty otherwise;
>  # either way this does not lead to a dependency on those libs - done 
> explicitly
> -syncevolution_LDADD = libsyncevocommon.la $(CORE_LDADD) $(KEYRING_LIBS) 
> $(KDE_KWALLET_LIBS)
> +syncevolution_LDADD = $(CORE_LDADD) $(KEYRING_LIBS) $(KDE_KWALLET_LIBS)
>  if COND_DBUS
>  syncevolution_LDADD += gdbus/libgdbussyncevo.la
>  endif
> -syncevolution_LDFLAGS = $(CORE_LD_FLAGS) $(DBUS_LIBS)
> +syncevolution_LDFLAGS = -Wl,--whole-archive -Wl,.libs/libsyncevocommon.a 
> -Wl,--no-whole-archive $(CORE_LD_FLAGS) $(D
>  syncevolution_CXXFLAGS = $(SYNCEVOLUTION_CXXFLAGS) $(CORE_CXXFLAGS) 
> $(KEYRING_CFLAGS) -I$(srcdir)/gdbus $(DBUS_CFLAG
>  syncevolution_DEPENDENCIES = $(EXTRA_LTLIBRARIES) $(CORE_DEP) 
> libsyncevocommon.la # $(SYNTHESIS_DEP)
>
> This is a hack. It completely circumvents the usual libtool logic and
> makes assumptions about libtool internals (.libs dir).
>

I've tried my dbus-server-reorg branch with this change as well as
your dbus-server-reorg-pohly-libsynccommon branch but I still get the
same results as before.

Btw, I've got some basic code in the bluetooth-device-id branch that
extracts the PnPInformation. But we can have that discussion in
Bugzilla.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] dbus-server-reorganization progress

[Chris Kühl]

On Wed, Jul 27, 2011 at 2:52 PM, Patrick Ohly  wrote:
> On Mi, 2011-07-27 at 10:30 +0200, Chris Kühl wrote:
>> On Wed, Jul 27, 2011 at 9:07 AM, Patrick Ohly  wrote:
>> > On Mi, 2011-07-27 at 01:45 +0200, Chris Kühl wrote:
>> >> I've tried my dbus-server-reorg branch with this change as well as
>> >> your dbus-server-reorg-pohly-libsynccommon branch but I still get the
>> >> same results as before.
>> >
>> > Just to be sure, you mean the "no backends available in static
>> > compilation" problem?
>> >
>> > The last commit fixed it for me. Anyway, the little benefit of compiling
>> > the source files less often is not worth the hassle. I'll revert to
>> > including the core source files in all executables.
>> >
>>
>> This is the error I get for the TestConnection.testCredentialsRight for 
>> example,
>> DBusException: org.syncevolution.InvalidCall: invalid value 'file' for
>> property 'backend': 'not one of the valid values (virtual, calendar =
>> events, addressbook = contacts, todo = tasks, memo = memos = notes)'
>
> I forgot that we were talking about the syncevo-dbus-server. My patch
> was only for the syncevolution binary. Because of its ugliness I must
> have shied away from applying the same trick/hack to all binaries -
> sorry for that.
>

Ah, which would have been obvious if I'd looked at the patch more closely. ;)

> I went ahead with reverting this ill-fated liaison with a libtool
> convenience library - not so convenient after all... Exercise for the
> reader: decide whether I meant libtool or its convenience libs.
>
> As part of rebasing the dbus-server-reorganization-pohly branch I did a
> few more things:
>      * include the AutoSyncManager syncDone fix (BMC #21888)
>      * fixed a compile error in the WebDAV backend due to the "Clean up
>        namespace pollution" patch
>      * moved D-Bus server sources into src/dbus/server and squashed
>        with the cleanup fixes for the original renaming to src/server
>      * removed accidental commit and its revert
>
> Regarding the directory renaming we've had a misunderstanding. When I
> said that the "dbus" should hold everything SyncEvolution D-Bus related,
> that was meant to include the server.
>

Ah, thanks for the clarification. However, it appears that you've gone
one level to deep. The source files in your branch are in
src/dbus/server/server/. Once I moved them up one level everything
worked as it should.

> The Notification* classes should also be moved there - not done yet, in
> case that they need further work.
>

Yes, agreed.  Looks out of place there.

> I'll probably have a look at compile times once all of this is in master
> (= directly after 1.2 release). Pushing a lot of files into
> sub-directories limits parallelism in "make -j", which is particularly
> obvious on our shiny new nightly build machine (8 real cores). I also
> saw on my laptop that compiling these *Register.cpp files multiple times
> does consume noticeable time - perhaps there is a better solution after
> all.
>
> Tests are currently running. TestConnection.testCredentialsRight passed
> already, so I am pushing my dbus-server-reorganization-pohly branch.
>

As I said, after moving the files that test passes on my end as well.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] dbus-server-reorganization progress

[Chris Kühl]

On Wed, Jul 27, 2011 at 2:52 PM, Patrick Ohly  wrote:

>
> The Notification* classes should also be moved there - not done yet, in
> case that they need further work.
>

Done. I've also removed the commit changing NULLs to 0s. These and a
fix for the files being in src/dbus/server/server are in my branch.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

[SyncEvolution] Templates and matching scores

[Chris Kühl]

Hi,

I've been working on retrieving the immutable vendor and product id
provided by the Bluetooth Device ID profile (DIP). See my
bluetooth-device-id branch[1] for what's been pushed so far.

Being that we now have a means of getting reliable device information
it seems to me that in the cases where this information was
attainable, a matching score is no longer needed. If the fingerprint
matches the product name from the look-up table exactly (and it will
because we've created the lookup table and the template fingerprints)
we suggest just that one template instead of several. If we didn't get
this product name from the look-up table then everything should behave
as normal because we can't trust the user-modifiable device name
string.

Does this sound reasonable or are there other reasons you'd want more
than one template to choose from even when you're sure which device
you're dealing with?

Cheers,
Chris

[1] 
https://meego.gitorious.org/meego-middleware/syncevolution/commits/bluetooth-device-id
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Templates and matching scores

[Chris Kühl]

On Wed, Aug 3, 2011 at 5:58 PM, Patrick Ohly  wrote:
> On Mi, 2011-08-03 at 17:02 +0200, Chris Kühl wrote:
>> Hi,
>>
>> I've been working on retrieving the immutable vendor and product id
>> provided by the Bluetooth Device ID profile (DIP). See my
>> bluetooth-device-id branch[1] for what's been pushed so far.
>>
>> Being that we now have a means of getting reliable device information
>> it seems to me that in the cases where this information was
>> attainable, a matching score is no longer needed. If the fingerprint
>> matches the product name from the look-up table exactly (and it will
>> because we've created the lookup table and the template fingerprints)
>> we suggest just that one template instead of several. If we didn't get
>> this product name from the look-up table then everything should behave
>> as normal because we can't trust the user-modifiable device name
>> string.
>>
>> Does this sound reasonable or are there other reasons you'd want more
>> than one template to choose from even when you're sure which device
>> you're dealing with?
>
> What about a Sony Ericcson phone which unknown product ID? We have two
> templates for vendor=Sony Ericsson.
>

In this case it would work just as before. If the device supports the
Device ID profile but it's not in our look-up table of known devices
then we don't use it. We need the look-up table to get the product
string.

There are 2 things that have to happen for the matching score to be unnecessary.

1) The device needs to be in the look-up table.
2) The template needs to have a fingerprint that matches the product
string from the look-up table exactly. If we or the user has added it
here then this should be the template to use.

If both are not fulfilled then we do as we've been doing.

> I agree that if the vendor matches exactly one template, preferring that
> template makes sense. I can imagine corner cases where it is not the
> right one (unknown Sony Ericsson phone which behaves like a Nokia), but
> that should be unlikely. So we should take advantage of the more
> reliable information to propose only one template.
>

If there is a Sony Ericsson that acts as a Nokia then the product id
should be in the Nokia template, right? Or we can just copy that
template and rename it which would probably more clear for the user.

With this I'm assuming that fingerprints are only added to templates
once the template is confirmed to be appropriate for that device.

Cheers,
Chris.
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Templates and matching scores

[Chris Kühl]

On Thu, Aug 4, 2011 at 5:26 PM, Patrick Ohly  wrote:
> On Do, 2011-08-04 at 16:29 +0200, Chris Kühl wrote:
>> On Wed, Aug 3, 2011 at 5:58 PM, Patrick Ohly  wrote:
>> > On Mi, 2011-08-03 at 17:02 +0200, Chris Kühl wrote:
>> >> Hi,
>> >>
>> >> I've been working on retrieving the immutable vendor and product id
>> >> provided by the Bluetooth Device ID profile (DIP). See my
>> >> bluetooth-device-id branch[1] for what's been pushed so far.
>> >>
>> >> Being that we now have a means of getting reliable device information
>> >> it seems to me that in the cases where this information was
>> >> attainable, a matching score is no longer needed. If the fingerprint
>> >> matches the product name from the look-up table exactly (and it will
>> >> because we've created the lookup table and the template fingerprints)
>> >> we suggest just that one template instead of several. If we didn't get
>> >> this product name from the look-up table then everything should behave
>> >> as normal because we can't trust the user-modifiable device name
>> >> string.
>> >>
>> >> Does this sound reasonable or are there other reasons you'd want more
>> >> than one template to choose from even when you're sure which device
>> >> you're dealing with?
>> >
>> > What about a Sony Ericcson phone which unknown product ID? We have two
>> > templates for vendor=Sony Ericsson.
>> >
>>
>> In this case it would work just as before. If the device supports the
>> Device ID profile but it's not in our look-up table of known devices
>> then we don't use it. We need the look-up table to get the product
>> string.
>
> I was working based on the assumption that our look-up table of devices
> and the corresponding entries in the templates will be very small.
>

Yes this is small currently, but should grow. And I plan on trying to
crowd source this information once the Desktop Summit is over. Of
course as you state below this doesn't mean we know which template
this should be assigned to.

> Where would we get a full list of product IDs for all Nokia phones ever
> published, for example? And even if we could get such a list, do we add
> all of those names to the Nokia template, without confirmation that it
> really works? As you said, we shouldn't.
>
> So the more realistic outcome is that we have to make decisions based on
> just the vendor name.
>

Right. This is available without support for the Device Id profile, by
using a portion of the mac address.

>> There are 2 things that have to happen for the matching score to be 
>> unnecessary.
>>
>> 1) The device needs to be in the look-up table.
>> 2) The template needs to have a fingerprint that matches the product
>> string from the look-up table exactly. If we or the user has added it
>> here then this should be the template to use.
>>
>> If both are not fulfilled then we do as we've been doing.
>
> Okay. In that case the code needs to use the vendor name from the Device
> ID profile instead of the user-assigned device name in the traditional
> fuzzy search.
>

Ok. Some of the names in the vendor list obtained from the Bluetooth
SIG would not match our fingerprints. For example, Sony Ericsson is
"Ericsson Technology Licensing." SE also has another entry that reads
"Sony Ericsson Mobile Communications." We either need to rename them
to match in the list or in the config templates. I say we change it in
the list.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Templates and matching scores

[Chris Kühl]

On Thu, Aug 4, 2011 at 9:50 PM, Patrick Ohly  wrote:
> On Do, 2011-08-04 at 17:42 +0200, Chris Kühl wrote:
>> On Thu, Aug 4, 2011 at 5:26 PM, Patrick Ohly  wrote:
>> > I was working based on the assumption that our look-up table of devices
>> > and the corresponding entries in the templates will be very small.
>> >
>>
>> Yes this is small currently, but should grow. And I plan on trying to
>> crowd source this information once the Desktop Summit is over. Of
>> course as you state below this doesn't mean we know which template
>> this should be assigned to.
>
> I was trying to grow the list of know and tested devices with the
> http://syncevolution.org/wiki/phone-compatibility-template template page
> and the resulting Wiki query for "phone":
> http://syncevolution.org/taxonomy/term/phone
>
> The result has been somewhat mixed. Directly after the initial
> SyncEvolution 1.0 release, some entries were added, but that stopped
> soon after.
>
> It definitely would be good to advertise that SyncEvolution really works
> with phones and how to report success/failure.

I'll put together a blog post and script or one-liner to make it easy
for people to submit new device id entries.

>
>> > Where would we get a full list of product IDs for all Nokia phones ever
>> > published, for example? And even if we could get such a list, do we add
>> > all of those names to the Nokia template, without confirmation that it
>> > really works? As you said, we shouldn't.
>> >
>> > So the more realistic outcome is that we have to make decisions based on
>> > just the vendor name.
>> >
>>
>> Right. This is available without support for the Device Id profile, by
>> using a portion of the mac address.
>
> Do we have and/or need code for that kind of lookup? Please correct me
> if I'm mistaken, but isn't Device ID profile support wide-spread enough
> that we can rely on it exclusively find the vendor name?
>

This information is available in the hwdata package on most distros.
More specifically it's in the oui.txt file. Not sure if there are
parsing routines for that file. As you mention though, for most of the
phones that we'll be syncing via bluetooth, the PnPInformation from
the Device ID Profile will be available.

>> >> There are 2 things that have to happen for the matching score to be 
>> >> unnecessary.
>> >>
>> >> 1) The device needs to be in the look-up table.
>> >> 2) The template needs to have a fingerprint that matches the product
>> >> string from the look-up table exactly. If we or the user has added it
>> >> here then this should be the template to use.
>> >>
>> >> If both are not fulfilled then we do as we've been doing.
>> >
>> > Okay. In that case the code needs to use the vendor name from the Device
>> > ID profile instead of the user-assigned device name in the traditional
>> > fuzzy search.
>> >
>>
>> Ok. Some of the names in the vendor list obtained from the Bluetooth
>> SIG would not match our fingerprints. For example, Sony Ericsson is
>> "Ericsson Technology Licensing." SE also has another entry that reads
>> "Sony Ericsson Mobile Communications." We either need to rename them
>> to match in the list or in the config templates. I say we change it in
>> the list.
>
> I agree.
>

I've done this for Nokia and Sony Ericsson.

Also, I think I'm pretty much done Bluetooth work. I've commit my
changes to the bluetooth-device-id branch[1] and updated the bug[2].

Cheers,
Chris

[1]: 
https://meego.gitorious.org/meego-middleware/syncevolution/commits/bluetooth-device-id
[2]: https://bugs.meego.com/show_bug.cgi?id=736#c3

> --
> Best Regards, Patrick Ohly
>
> The content of this message is my personal opinion only and although
> I am an employee of Intel, the statements I make here in no way
> represent Intel's position on the issue, nor am I authorized to speak
> on behalf of Intel on this matter.
>
>
>
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

[SyncEvolution] Moving the dbus server to GIO's GDBus

[Chris Kühl]

Hi all,

One of the things I was to look into was whether, and how, I could
port  the syncevo-dbus-server to use GIO's GDBus. Currently
syncevo-dbus-server uses an in-tree copy of a dbus library that also
goes by the name gdbus but, I think, originated in Bluez.

We also currently wrap the Bluez gdbus library in C++ classes and the
intention is to do the same for GIO's GDBus. This has the advantage
that the interface the server uses would remain unchanged.

One  of the requirements that Patrick gave me was that we need to
remain compatible with Debian stable and Ubuntu LTS. However, both of
these have GLib 2.24 packages which is one version before GLib put
GDBus into GIO. We then decided to look into possibly using an in-tree
copy of GIO's GDBus until we able to make GLib >= 2.26 a requirement.
This, however, won't really work since the GDBus that landed in GLib
2.26 makes use of some new features of 2.26.

It seems to me as if we've got 2 choices right now...

1) Postpone this and wait till we can bump the requirement to GLib >=
2.26 to do this work.
2) Go forward with this and use it when ./configure detects an
adequate GLib version.

I'm thinking the second option sounds like the way to go. Let me know
your opinions on this.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Templates and matching scores

[Chris Kühl]

On Mon, Aug 15, 2011 at 3:05 PM, Chris Kühl  wrote:
> On Thu, Aug 4, 2011 at 9:50 PM, Patrick Ohly  wrote:
>> On Do, 2011-08-04 at 17:42 +0200, Chris Kühl wrote:
>>> On Thu, Aug 4, 2011 at 5:26 PM, Patrick Ohly  wrote:
>>> > I was working based on the assumption that our look-up table of devices
>>> > and the corresponding entries in the templates will be very small.
>>> >
>>>
>>> Yes this is small currently, but should grow. And I plan on trying to
>>> crowd source this information once the Desktop Summit is over. Of
>>> course as you state below this doesn't mean we know which template
>>> this should be assigned to.
>>
>> I was trying to grow the list of know and tested devices with the
>> http://syncevolution.org/wiki/phone-compatibility-template template page
>> and the resulting Wiki query for "phone":
>> http://syncevolution.org/taxonomy/term/phone
>>
>> The result has been somewhat mixed. Directly after the initial
>> SyncEvolution 1.0 release, some entries were added, but that stopped
>> soon after.
>>
>> It definitely would be good to advertise that SyncEvolution really works
>> with phones and how to report success/failure.
>
> I'll put together a blog post and script or one-liner to make it easy
> for people to submit new device id entries.
>

I've created a script so that users can easily get and then submit the
information that we need. It's not perfect but it seems to work
alright. Here's the gitorious link:
https://gitorious.org/bluetooth-device-id-inspector

I've also got a blog post ready to go once I get some feedback on the script.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Moving the dbus server to GIO's GDBus

[Chris Kühl]

On Tue, Aug 16, 2011 at 4:32 PM, Patrick Ohly  wrote:
> On Tue, 2011-08-16 at 15:36 +0200, Chris Kühl wrote:
>> It seems to me as if we've got 2 choices right now...
>>
>> 1) Postpone this and wait till we can bump the requirement to GLib >=
>> 2.26 to do this work.
>> 2) Go forward with this and use it when ./configure detects an
>> adequate GLib version.
>>
>> I'm thinking the second option sounds like the way to go. Let me know
>> your opinions on this.
>
> I agree. It'll allow me to continue providing binaries for a wider
> variety of Linux systems on syncevolution.org and make distribution
> packagers happy because they can ignore the in-tree copy of the Bluez
> gdbus.
>

Ok, I'll branch a gio-gdbus branch off my dbus-server-reorg branch.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Templates and matching scores

[Chris Kühl]

On Thu, Aug 18, 2011 at 3:25 PM, Patrick Ohly  wrote:
> On Di, 2011-08-16 at 15:51 +0200, Chris Kühl wrote:
>> I've created a script so that users can easily get and then submit the
>> information that we need. It's not perfect but it seems to work
>> alright. Here's the gitorious link:
>> https://gitorious.org/bluetooth-device-id-inspector
>>
>> I've also got a blog post ready to go once I get some feedback on the script.
>
> When I ran it, I got the following question:
>        What company makes this phone?
>
> There was no other output that would have allowed me to identify which
> of my paired Bluetooth devices the script was talking about. I guess it
> should have printed the user-assigned device name first, plus perhaps
> the MAC address.
>

True. I think I had in mind that only one phone would be queried. I
then wrote the code to walk over all devices, though. I'll change it
to display the device name and maybe the MAC address as well with some
text to explain.

> In the script you volunteer to collect the feedback. So you intend to
> stay around? Great, welcome :-)
>

Well, I'd like to see as many devices supported as possible. So, yeah,
I'm willing to take the time to insert the info in the table, etc.

> Should we encourage users to also try syncing with their device, so that
> we not only get "this device exists" but also "it works with this
> config"?
>
> It shouldn't be required because the hurdle is a lot higher (must
> install SyncEvolution first, may change data on phone).
>

Sure, I could politely point them to the wiki tutorials that Dave worked on.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Templates and matching scores

[Chris Kühl]

On Thu, Aug 18, 2011 at 4:03 PM, Patrick Ohly  wrote:
> On Mo, 2011-08-15 at 15:05 +0200, Chris Kühl wrote:
>> Also, I think I'm pretty much done Bluetooth work. I've commit my
>> changes to the bluetooth-device-id branch[1] and updated the bug[2].
>
> Would it make sense to squash the development history? I don't know how
> many different commits would make sense. Right now I am only looking at
> one diff that includes everything.
>

Absolutely. I've squashed it into 2 commits; one for the server code
and another for the tests. You can find it in the
bluetooth-device-id-merge-prep branch.

> It adds a TODO:
> +                if(hasPnpInfoService(uuids)) {
> +                    // TODO: Get the actual vendor and product ids.
> +                    DBusClientCall1 discoverServices(*this,
> +                                                                  
> "DiscoverServices");
>
> What does that mean? Is there work left to do and is it tracked in 
> bugs.meego.com?

Oop. That's old. Removed now

>
> +    if(pos != std::string::npos)
> +    {
>
> Not my favorite bracket style, but so be it...
>

Think I've changed them all to open on the same line as the if
statement now. [Grumbles about function and if statement braces being
different. ;)]

> +static string SyncEvolutionDataDir()
> +{
> +    string dataDir(DATA_DIR);
> +    const char *envvar = getenv("SYNCEVOLUTION_DATA_DIR");
> +    if (envvar) {
> +        dataDir = envvar;
> +    }
> +    return dataDir;
> +}
>
> This new env variable needs to be documented in README.rst. A better place 
> for this
> function might be in util.h/cpp.
>

Ok I've updated README.rst and moved it to utils.

This is just a copy of SyncEvolutionTemplateDir which is static in
SyncConfig.cpp. I'll make a separate commit to do the same with that,
too.

>
> +static bool getPnpInfoNamesFromValues(const std::string &vendorValue,  
> std::string &vendorName,
> +                                      const std::string &productValue, 
> std::string &productName)
> +{
> +    static GKeyFile *bt_key_vals = NULL;
> +
> +    if(!bt_key_vals) {
> +        bt_key_vals = g_key_file_new();
> +        GError *err = NULL;
> +        string filePath(SyncEvolutionDataDir() + "/bluetooth_products.ini");
>
> The syncevo-dbus-server has a feature where it watches files that make
> up the running process and restarts when any of those change. System
> daemons do that via package scripts, but for daemons started in a
> session that doesn't work. The SyncEvolution mechanism currently works
> for the executable and shared libraries, automatically found from
> scanning /proc/self/maps.
>
> Data files are loaded anew when needed (XML config files, for example).
> Doing that for every getPnpInfoNamesFromValues() might be a bit often.
> Perhaps the result can be cached in the D-Bus session instance instead
> of in a static variable?
>
> Or you could manually add that file to the watch list. See
> DBusServer::run().
>

Ah, yes. I did notice that code but had forgotten about it.

> +    if(vendor)
> +        vendorName = vendor;
> +    else
> +        // We at least need a vendor id match.
> +        return false;
>
> Now *that's* the (non-)bracket style which I once declared unacceptable
> for SyncEvolution ;-) I know, I should replace the reference to the
> Funambol coding style in the HACKING document with some actual rules.
> Please, always use brackets even around single  statements.
>

Sure, I should be done now.

> +        /** callback of 'DiscoverServices' method. The serve detals are 
> retrieved */
> +        void discoverServicesCb(const ServiceDict &serviceDict, const 
> std::string &error);
>
> Typo. Really minor. Consider it a feeble attempt at demonstrating that I
> have read the patch.
>

Thanks. Fixed.

>     for(syncDevIt = m_syncDevices.begin(); syncDevIt != m_syncDevices.end(); 
> ++syncDevIt) {
>         if(boost::equals(syncDevIt->m_deviceId, deviceId)) {
>             device = *syncDevIt;
> +            if(syncDevIt->m_pnpInformation)
> +            {
>
> Switching coding style in the middle of some other code looks weird.
>

What can I say? I like variety. ;) Fixed.

> --- a/test/test-dbus.py
> +++ b/test/test-dbus.py
> @@ -53,6 +53,10 @@ monitor = ["dbus-monitor"]
>  # primarily for XDG files, but also other temporary files
>  xdg_root = "temp-test-dbus"
>  configName = "dbus_unittest"
> +# for bluetooth tests. replace with values from your test device.
> +bt_device_mac = "D4:5D:42:73:E4:6C"
> +bt_device_fingerp

Re: [SyncEvolution] Templates and matching scores

[Chris Kühl]

On Fri, Aug 19, 2011 at 6:10 PM, Chris Kühl  wrote:
> On Thu, Aug 18, 2011 at 4:03 PM, Patrick Ohly  wrote:
>> On Mo, 2011-08-15 at 15:05 +0200, Chris Kühl wrote:
>>
>> +static bool getPnpInfoNamesFromValues(const std::string &vendorValue,  
>> std::string &vendorName,
>> +                                      const std::string &productValue, 
>> std::string &productName)
>> +{
>> +    static GKeyFile *bt_key_vals = NULL;
>> +
>> +    if(!bt_key_vals) {
>> +        bt_key_vals = g_key_file_new();
>> +        GError *err = NULL;
>> +        string filePath(SyncEvolutionDataDir() + "/bluetooth_products.ini");
>>
>> The syncevo-dbus-server has a feature where it watches files that make
>> up the running process and restarts when any of those change. System
>> daemons do that via package scripts, but for daemons started in a
>> session that doesn't work. The SyncEvolution mechanism currently works
>> for the executable and shared libraries, automatically found from
>> scanning /proc/self/maps.
>>
>> Data files are loaded anew when needed (XML config files, for example).
>> Doing that for every getPnpInfoNamesFromValues() might be a bit often.
>> Perhaps the result can be cached in the D-Bus session instance instead
>> of in a static variable?
>>
>> Or you could manually add that file to the watch list. See
>> DBusServer::run().
>>
>
> Ah, yes. I did notice that code but had forgotten about it.

I forgot to add here that I've added a file monitor to the
BluezManager class to take care of this. It's in a separate commit. on
the *merge_prep branch.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Templates and matching scores

[Chris Kühl]

On Mon, Aug 22, 2011 at 3:39 PM, Patrick Ohly  wrote:
> On Mo, 2011-08-22 at 15:32 +0200, Murray Cumming wrote:
>> On Thu, 2011-08-18 at 16:03 +0200, Patrick Ohly wrote:
>> > +static string SyncEvolutionDataDir()
>> > +{
>> > +    string dataDir(DATA_DIR);
>> > +    const char *envvar = getenv("SYNCEVOLUTION_DATA_DIR");
>> > +    if (envvar) {
>> > +        dataDir = envvar;
>> > +    }
>> > +    return dataDir;
>> > +}
>> >
>> > This new env variable needs to be documented in README.rst. A better
>> > place for this
>> > function might be in util.h/cpp.
>>
>> Is there even any need for an environment variable to override the
>> default location? Just for testing?
>
> Primarily for testing. A secondary, less important use case is that some
> users might not have root access on a machine and thus might be forced
> to run pre-compiled binaries in a non-standard location. That works when
> setting the env variables right. The HTTP server HOWTO explained that.
> I'm running SyncEvolution like that for my own use on estamos.de
>
> That reminds me: syncevo-http-server sets all these env variables when
> asked to. SYNCEVOLUTION_DATA_DIR must be added there.
>

Done.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Templates and matching scores

[Chris Kühl]

On Fri, Aug 19, 2011 at 6:10 PM, Chris Kühl  wrote:
> On Thu, Aug 18, 2011 at 4:03 PM, Patrick Ohly  wrote:
>
> What can I say? I like variety. ;) Fixed.
>
>> --- a/test/test-dbus.py
>> +++ b/test/test-dbus.py
>> @@ -53,6 +53,10 @@ monitor = ["dbus-monitor"]
>>  # primarily for XDG files, but also other temporary files
>>  xdg_root = "temp-test-dbus"
>>  configName = "dbus_unittest"
>> +# for bluetooth tests. replace with values from your test device.
>> +bt_device_mac = "D4:5D:42:73:E4:6C"
>> +bt_device_fingerprint = "Nokia 5230"
>> +bt_device_name = "Nokia 5230a"
>>
>> Replace how? By editing the script? I know that I have set a bad
>> precedent by using the same approach for enabling gdb, but that is rare.
>> A better mechanism for the device would be useful - command line
>> parameter or env variable? Or perhaps better simulate a desktop
>> environment with various paired Bluetooth devices by registering a mock
>> org.bluez implementation on the D-Bus session bus and using it in
>> syncevo-dbus-server when a certain env variable is set? See
>> DBUS_TEST_CONNMAN for an example.
>>
>
> Ok, I see. I'll get this changed. Not in right now, though.
>

This is also done now.  I think this is what you intended.

The bluetooth-device-id-merge-prep is ready to be reviewed now.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Templates and matching scores

[Chris Kühl]

2011/8/26 Patrick Ohly :
> On Mi, 2011-08-24 at 14:46 +0200, Chris Kühl wrote:
>> The bluetooth-device-id-merge-prep is ready to be reviewed now.
>
> I've merged the D-Bus server reorganization, dbus-test and
> bluetooth-device-id-merge-prep branches. I fixed some minor problems
> found with -Werror -Wall.

Great!

>
> Attached an updated revision of the bluetooth-device-id-inspector.py
> script. Should we maintain it in SyncEvolution?
>

Makes sense. Should it go in test or should we create a
tools/utilities directory?

> My changes:
> - more information for the user
> - allow the user to skip devices
> - don't include vendor in example product name
> - skip devices which have PNPINFO_UUID information, but are not
>  online at the moment (script used to abort with an exception)

Yeah, I ran into this after committing and never got around to updating. Thanks.

> - write the user provided vendor/model information into the file
>  even if PNPINFO_UUID information isn't supported
>
> The last point avoids the situation where the user is asked to send you
> a file which says "This phone does not support Device ID profile"
> without telling you what phone it is ;-)
>

Ah right. ;)

> Do we want to gather information about that? I think we should for the
> sake of completeness, although I am not sure what to do with it in that
> case.
>

Yes, I agree. It may be of interest to know which phones don't support
this feature, if only to give us some insight into what percentage of
phones do support the device id profile.

> Here's an example run:
>
> Device name: localhost.localdomain-0
> MAC Address: 00:22:43:AF:07:0D
>   Supports SyncML.
>   What company makes this phone? (examples: Nokia, Sony Ericsson), empty to 
> skip:
> Device name: K750i
> MAC Address: 00:0E:07:B1:90:17
>   Supports SyncML.
>   Looking up device information...
>   Failed, skipping device: org.bluez.Error.ConnectionAttemptFailed: Host is 
> down
> Device name: 6310i
> MAC Address: 00:60:57:33:6C:0C
>   What company makes this phone? (examples: Nokia, Sony Ericsson), empty to 
> skip: Nokia
>   What is the model of this phone? (example: N900, K750i), empty to skip: 
> 6310i
> Thanks, please send the file syncevo-phone-info-[6310i].txt to blixtra [at] 
> gmail.com
> Device name: Nokia N70
> MAC Address: 00:17:4B:1D:28:B2
>   Supports SyncML.
>   What company makes this phone? (examples: Nokia, Sony Ericsson), empty to 
> skip: Nokia
>   What is the model of this phone? (example: N900, K750i), empty to skip: N70
> Thanks, please send the file syncevo-phone-info-[N70].txt to blixtra [at] 
> gmail.com
> Device name: Nokia N900
> MAC Address: 80:50:1B:BE:AA:2B
>   Supports SyncML.
>   What company makes this phone? (examples: Nokia, Sony Ericsson), empty to 
> skip: Nokia
>   What is the model of this phone? (example: N900, K750i), empty to skip: N900
> Thanks, please send the file syncevo-phone-info-[N900].txt to blixtra [at] 
> gmail.com
> Device Name: Nokia N97 mini
> MAC Address: A0:4E:04:1E:AD:30
>   Supports SyncML.
>   Looking up device information...
>   What company makes this phone? (examples: Nokia, Sony Ericsson), empty to 
> skip: Nokia
>   What is the model of this phone? (example: N900, K750i), empty to skip: N97 
> mini
> Device name: Nokia 7100s-2
> MAC Address: C8:97:9F:5D:B7:FB
>   Supports SyncML.
>   Looking up device information...
>   Failed, skipping device: org.bluez.Error.ConnectionAttemptFailed: Host is 
> down
>
> Of the various Nokia phones, only some supported the Device ID profile
> and I had only turned on the N97. All resulting files also attached.
>
> When I tried this in combination with the GTK sync-ui, I found that my
> "Nokia N97" (user chosen name) was sometimes shown as "Nokia", sometimes
> as "Nokia N97" in the UI.
>
> Chris, I suspect that I gave you wrong guidance about the use use of
> deviceName/peerName, or we have to extend the semantic. According to the
> D-Bus API docs:
>
>      * deviceName: device template: the device name that the template
>        is for (copied verbatim from that device)
>      * templateName: device template: string identifying the class of
>        devices the templates works for, like "Nokia S40"; meant to be
>        shown to users; optional, fall back to first entry in
>        fingerPrint if not set
>
> Now the syncevo-dbus-server sends deviceName=Nokia, templateName=Nokia
> and peerName=Nokia N97 mini.
>
> I think it should send:
>      * deviceName: the vendor/model information if available, otherwise
>        the user-configurable device name (can't be empty, because
>

Re: [SyncEvolution] Templates and matching scores

[Chris Kühl]

On Mon, Aug 29, 2011 at 12:25 PM, Patrick Ohly  wrote:
> On Mo, 2011-08-29 at 11:50 +0200, Chris Kühl wrote:
>> 2011/8/26 Patrick Ohly :
>> > On Mi, 2011-08-24 at 14:46 +0200, Chris Kühl wrote:
>> >> The bluetooth-device-id-merge-prep is ready to be reviewed now.
>> >
>> > I've merged the D-Bus server reorganization, dbus-test and
>> > bluetooth-device-id-merge-prep branches. I fixed some minor problems
>> > found with -Werror -Wall.
>>
>> Great!
>>
>> >
>> > Attached an updated revision of the bluetooth-device-id-inspector.py
>> > script. Should we maintain it in SyncEvolution?
>> >
>>
>> Makes sense. Should it go in test or should we create a
>> tools/utilities directory?
>
> At some point we should, but then we would also have to move some of the
> other scripts. Let's add it to "test" to keep it simple for the time
> being.
>
>> > Chris, I suspect that I gave you wrong guidance about the use use of
>> > deviceName/peerName, or we have to extend the semantic. According to the
>> > D-Bus API docs:
>> >
>> >      * deviceName: device template: the device name that the template
>> >        is for (copied verbatim from that device)
>> >      * templateName: device template: string identifying the class of
>> >        devices the templates works for, like "Nokia S40"; meant to be
>> >        shown to users; optional, fall back to first entry in
>> >        fingerPrint if not set
>> >
>> > Now the syncevo-dbus-server sends deviceName=Nokia, templateName=Nokia
>> > and peerName=Nokia N97 mini.
>> >
>> > I think it should send:
>> >      * deviceName: the vendor/model information if available, otherwise
>> >        the user-configurable device name (can't be empty, because
>> >        traditionally it wasn't describe as optional)
>>
>> Right, I can make this change. But when you say "vendor/model is
>> available" you mean when the product and not *just* the vendor is in
>> the lookup table, right? That would make sense to me.
>
> IMHO it would make sense also if just the vendor is available. That
> information is not provided to the UI otherwise.
>
> But see my later email: after some more thinking I came to the
> conclusion that "deviceName" should stay as it is (name chosen by the
> user) and the new information should go into "templateName".

Did you mean peerName instead of templateName?

>From the what you are saying here...

Patrick Ohly  wrote:
>> The UI should be adapted to use peerName instead of deviceName, if
>> peerName is available. That way the user would see his chosen name
>> instead of the vendor/model name, which will not be unique in the
>> (unlikely) case that the user has more than one. Not a big deal.
>>
>> We might have the inverse situation, too: multiple different devices all
>> called "My Phone". I think users should (and can) avoid this, so we
>> should continue to display only the chosen name instead of adding the
>> vendor/model information - right?
>
> Given that the UI should only display the chosen name, and expects it in
> "deviceName", perhaps we should keep the traditional D-Bus API semantic
> unchanged and put the Device ID profile information into the "peerName"?
>
> That is a bit backwards (peerName is used for user-configurable strings
> elsewhere), but has the advantage that no changes will be needed in the
> D-Bus clients to get the desired behavior.
>

... I understand the following.

1) In the case that we have no PnPInformation info we have...

deviceName = User-modifiable name
peerName = User-modifiable name

2) In the case that we have PnPInformation info but no product match
in the lookup table  we have...

deviceName = User-modifiable name
peerName = Vendor found in Lookup table

3) In the case that we have PnPInformation info and a product match in
the lookup table we have...

deviceName = User-modifiable name
peerName = Vendor + " " + Product found in Lookup table

Is this right?

Also, the templateName was and is simply the "templateName" field from
the template. The code is...

string TemplateConfig::getTemplateName() {
return m_metaProps["templateName"];
}

Should this be different? We could create a different template for
each class of phone.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Templates and matching scores

[Chris Kühl]

On Mon, Aug 29, 2011 at 3:12 PM, Patrick Ohly  wrote:
> On Mo, 2011-08-29 at 14:03 +0200, Chris Kühl wrote:
>> On Mon, Aug 29, 2011 at 12:25 PM, Patrick Ohly  
>> wrote:
>> > On Mo, 2011-08-29 at 11:50 +0200, Chris Kühl wrote:
>> >> 2011/8/26 Patrick Ohly :
>> > But see my later email: after some more thinking I came to the
>> > conclusion that "deviceName" should stay as it is (name chosen by the
>> > user) and the new information should go into "templateName".
>>
>> Did you mean peerName instead of templateName?
>
> Yes.
>
>> Patrick Ohly  wrote:
>> >> The UI should be adapted to use peerName instead of deviceName, if
>> >> peerName is available. That way the user would see his chosen name
>> >> instead of the vendor/model name, which will not be unique in the
>> >> (unlikely) case that the user has more than one. Not a big deal.
>> >>
>> >> We might have the inverse situation, too: multiple different devices all
>> >> called "My Phone". I think users should (and can) avoid this, so we
>> >> should continue to display only the chosen name instead of adding the
>> >> vendor/model information - right?
>> >
>> > Given that the UI should only display the chosen name, and expects it in
>> > "deviceName", perhaps we should keep the traditional D-Bus API semantic
>> > unchanged and put the Device ID profile information into the "peerName"?
>> >
>> > That is a bit backwards (peerName is used for user-configurable strings
>> > elsewhere), but has the advantage that no changes will be needed in the
>> > D-Bus clients to get the desired behavior.
>> >
>>
>> ... I understand the following.
>>
>> 1) In the case that we have no PnPInformation info we have...
>>
>> deviceName = User-modifiable name
>> peerName = User-modifiable name
>
> Better leave the peerName unset. It's semantic will be "we know for sure
> that this device is a "[ ]".
>
>> 2) In the case that we have PnPInformation info but no product match
>> in the lookup table  we have...
>>
>> deviceName = User-modifiable name
>> peerName = Vendor found in Lookup table
>
> Correct.
>
>> 3) In the case that we have PnPInformation info and a product match in
>> the lookup table we have...
>>
>> deviceName = User-modifiable name
>> peerName = Vendor + " " + Product found in Lookup table
>>
>> Is this right?
>
> Yes.
>
>> Also, the templateName was and is simply the "templateName" field from
>> the template. The code is...
>>
>> string TemplateConfig::getTemplateName() {
>>     return m_metaProps["templateName"];
>> }
>
> Correct.
>
>> Should this be different? We could create a different template for
>> each class of phone.
>
> I don't think anything around templateName needs to be changed.
>

I believe the first patch I've attached takes care of the deviceName
and peerName meanings. The second patch just adds the bluetooth
inspector script to the "test" directory.

Cheers,
Chris
From dec4d34c0b1297b5ed7b39d60df2c58b395eb5b6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Chris=20K=C3=BChl?= 
Date: Mon, 29 Aug 2011 16:12:46 +0200
Subject: [PATCH 1/2] Make deviceName and peerName more well-defined.

Previously the DeviceDescription deviceName was being stored in the
TemplateDescription peerName.

This patch defines devicename in both DeviceDescription &
TemplateDescription as storing the user-modifiable device name, thus
unifiying the meaning. The peerName is now used to hold the value used
as the template fingerprint. This value can be the same as deviceName,
in the case where the device does not support the Device ID
profile. In the case that the Device ID profile is supported, it will
be either the vendor or vendor + model depending on whether the device
model is found in the lookup table.
---
 src/dbus/server/read-operations.cpp |4 ++--
 src/syncevo/SyncConfig.cpp  |   14 +++---
 src/syncevo/SyncConfig.h|   14 --
 3 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/src/dbus/server/read-operations.cpp b/src/dbus/server/read-operations.cpp
index be91541..c6bb6e4 100644
--- a/src/dbus/server/read-operations.cpp
+++ b/src/dbus/server/read-operations.cpp
@@ -45,7 +45,7 @@ void ReadOperations::getConfigs(bool getTemplates, std::vector &con
 std::map numbers;
 BOOST_FOREACH(const boost::shared_ptr peer, list) {
 //if it is not a template for device
-if(peer->m_fin

Re: [SyncEvolution] Templates and matching scores

[Chris Kühl]

On Mon, Aug 29, 2011 at 5:27 PM, Patrick Ohly  wrote:
> On Mo, 2011-08-29 at 15:12 +0200, Patrick Ohly wrote:
>> > 1) In the case that we have no PnPInformation info we have...
>> >
>> > deviceName = User-modifiable name
>> > peerName = User-modifiable name
>>
>> Better leave the peerName unset. It's semantic will be "we know for sure
>> that this device is a "[ ]".
>
> You decided to not implement it like this, did you?

That was an oversight. Attached, you'll find updated patch.

>
> From your patch:
>             // This is the user-modifiable device name. Could be shown in 
> GUIs, for example
>             localConfigs.insert(pair("peerName", 
> peerTemplate->m_peerName));
>
> +        // This can be either the user-modifiable device name, vendor
> +        // name, or product name (vendor + model). This depends on
> +        // whether the device supports the Bluetooth Device ID profile
> +        // and, if so, whether we have the model in the lookup table.
>         std::string m_peerName;
>
> Was it simply easier to implement this way or do you prefer that
> semantic of "peerName" in a template reported by the D-Bus server?
>
> My concern is that if the D-Bus server always reports a value, the D-Bus
> client won't be able to determine whether it has reliable information
> about the manufacturer and model.
>

I agree that there is a need to be able for the client code to know
that they have reliable info or not. Setting peerName to empty is a
good way to do that.

I've also attached a n updated patch for the script. Just adds a
header with copyright and license info.

Cheers,
Chris
From 86e365df27850758fa2480c16e1c298bed172af1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Chris=20K=C3=BChl?= 
Date: Mon, 29 Aug 2011 16:12:46 +0200
Subject: [PATCH 1/2] Make deviceName and peerName more well-defined.

Previously the DeviceDescription deviceName was being stored in the
TemplateDescription peerName.

This patch defines devicename in both DeviceDescription &
TemplateDescription as storing the user-modifiable device name, thus
unifiying the meaning. The peerName is defined to hold the vendor
and/or model information discovered using the Bluetooth Device ID
profile (DIP) or, in the case that DIP is not supported, peerName will
be an empty string. Whether the peerName includes the model of the
device is dependent on whether a matching entry was found in the
product lookup table.
---
 src/dbus/server/read-operations.cpp |7 ---
 src/syncevo/SyncConfig.cpp  |   19 +++
 src/syncevo/SyncConfig.h|   14 --
 3 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/src/dbus/server/read-operations.cpp b/src/dbus/server/read-operations.cpp
index be91541..31d9905 100644
--- a/src/dbus/server/read-operations.cpp
+++ b/src/dbus/server/read-operations.cpp
@@ -45,7 +45,7 @@ void ReadOperations::getConfigs(bool getTemplates, std::vector &con
 std::map numbers;
 BOOST_FOREACH(const boost::shared_ptr peer, list) {
 //if it is not a template for device
-if(peer->m_fingerprint.empty()) {
+if(peer->m_deviceName.empty()) {
 configNames.push_back(peer->m_templateId);
 } else {
 string templName = "Bluetooth_";
@@ -119,8 +119,9 @@ void ReadOperations::getConfig(bool getTemplate,
 score << peerTemplate->m_rank;
 localConfigs.insert(pair("score", score.str()));
 // Actually this fingerprint is transferred by getConfigs, which refers to device name
-localConfigs.insert(pair("deviceName", peerTemplate->m_fingerprint));
-// This is the user-modifiable device name. Could be shown in GUIs, for example
+localConfigs.insert(pair("deviceName", peerTemplate->m_deviceName));
+// This is the reliable device info obtained from the bluetooth
+// device id profile (DIP) or emtpy if DIP not supported.
 localConfigs.insert(pair("peerName", peerTemplate->m_peerName));
 // This is the fingerprint of the template
 localConfigs.insert(pair("fingerPrint", peerTemplate->m_matchedModel));
diff --git a/src/syncevo/SyncConfig.cpp b/src/syncevo/SyncConfig.cpp
index 1ac88b7..3d6b10d 100644
--- a/src/syncevo/SyncConfig.cpp
+++ b/src/syncevo/SyncConfig.cpp
@@ -714,16 +714,19 @@ SyncConfig::TemplateList SyncConfig::matchPeerTemplates(const DeviceList &peers,
 }
 BOOST_FOREACH (const DeviceList::value_type &entry, peers){
 std::string fingerprint(entry.getFingerprint());
-int rank = templateConf.metaMatch (fingerprint, entry.m_matchMode);
+// peerName should be empty if no reliable device info is on hand.
+std::string peerName = entry.m_pnpInformation ? fingerprint : "";
+
+int rank = templateConf.metaMatch (entry.getFingerprint(), entry.m_matchMode);
 if (fuzzyMat

Re: [SyncEvolution] Templates and matching scores

[Chris Kühl]

On Tue, Sep 6, 2011 at 7:01 AM, Patrick Ohly  wrote:
> On Di, 2011-08-30 at 12:33 +0200, Chris Kühl wrote:
>> On Mon, Aug 29, 2011 at 5:27 PM, Patrick Ohly  wrote:
>> > On Mo, 2011-08-29 at 15:12 +0200, Patrick Ohly wrote:
>> >> > 1) In the case that we have no PnPInformation info we have...
>> >> >
>> >> > deviceName = User-modifiable name
>> >> > peerName = User-modifiable name
>> >>
>> >> Better leave the peerName unset. It's semantic will be "we know for sure
>> >> that this device is a "[ ]".
>> >
>> > You decided to not implement it like this, did you?
>>
>> That was an oversight. Attached, you'll find updated patch.
>
> Thanks, merged. When writing the API docs for it I got unhappy about the
> overloading of peerName that I had suggested earlier and ended up
> renaming the property. See attached patch (from master).

Great. On vacation now but glad to see that get in and completed.

>
>> I agree that there is a need to be able for the client code to know
>> that they have reliable info or not. Setting peerName to empty is a
>> good way to do that.
>
> Or better, don't even send it.

True.

>
>> I've also attached a n updated patch for the script. Just adds a
>> header with copyright and license info.
>
> Also merged.

I'll make publish the blog post within the next couple days.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

[SyncEvolution] Patch: user-submitted device ids

[Chris Kühl]

Hi all,

I got about 20 emails in response to my blog post[1] requesting that
folks run the script we wrote to find out whether a phone supports
syncml and extract its Bluetooth device id.

Quite a few submissions were duplicates and a few, which I've included
in the file, seem to give non-conformant information.

I've attached the patch with the corrosponding changes.

Cheers,
Chris

[1] 
http://blixtra.org/blog/2011/09/22/syncevolution-needs-you-or-at-least-your-bluetooth-phones/
From 29451e5b64447741fe3dfbc2d13d01c4ec570d1b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Chris=20K=C3=BChl?= 
Date: Sat, 1 Oct 2011 20:32:15 +0200
Subject: [PATCH] dbus: Added user submitted device ids

---
 src/dbus/server/bluetooth_products.ini |   44 +---
 1 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/src/dbus/server/bluetooth_products.ini b/src/dbus/server/bluetooth_products.ini
index f0141a6..feb1398 100644
--- a/src/dbus/server/bluetooth_products.ini
+++ b/src/dbus/server/bluetooth_products.ini
@@ -123,11 +123,47 @@
 
 # Product lookup table
 #
-# The keys are of the form "VendorID_ProductID", and the value
-# "Vendor Model". The VendorID needs to be used as a prefix because
-# ProductIDs are only unique per vender. Currently we have no list of
-# product IDs from vendors so we add them as we find them.
+# The keys are of the form "VendorID_ProductID", and the value "Vendor
+# Model". The VendorID needs to be used as a prefix because ProductIDs
+# are only unique per vendor. Currently we have no list of product IDs
+# from vendors so we add them as we find them. To this end we've
+# written a script (bluetooth-device-id-inspector.py) located in the
+# top level "test" directory which scans for this info and outputs a
+# file that can be sent to syncEvolution developers for inclusion.
+#
 [Products]
+0x_0xc089=Sony Ericsson W580i
 0x_0xc112=Sony Ericsson W995
+0x0001_0x003c=Nokia 5310
 0x0001_0x0084=Nokia N85
 0x0001_0x00e7=Nokia 5230
+0x0001_0x00ff=Nokia E7
+0x0001_0x0191=Nokia 2323
+
+# Questionable devices
+#
+# The following all have 0x as product ids. The Android devices
+# all seem to do this.
+#
+# SyncML support: False
+# Source: 0x0002
+# Vendor: 0x000a=LG
+# product: 0x=P990
+#
+# SyncML support: False
+# Source: 0x0001
+# Vendor: 0x000f=HTC
+# product: 0x=Desire HD
+#
+# SyncML support: True
+# Source: 0x0002
+# Vendor: 0x000a=Samsung
+# product: 0x=Nexus S
+#
+# Devices known to not support the Bluetooth Device ID profile
+#
+# Nokia N95
+# Nokia N900
+# Nokia E71
+# Nokia N950
+# Siemens S55
-- 
1.7.6.4

___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Patch: user-submitted device ids

[Chris Kühl]

On Mon, Oct 3, 2011 at 4:30 AM, Chris Kühl  wrote:
> Hi all,
>
> I got about 20 emails in response to my blog post[1] requesting that
> folks run the script we wrote to find out whether a phone supports
> syncml and extract its Bluetooth device id.
>
> Quite a few submissions were duplicates and a few, which I've included
> in the file, seem to give non-conformant information.
>
> I've attached the patch with the corrosponding changes.

Actually, I just pushed a branch named device-id-submissions which
I'll be using to add any new submissions.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

[SyncEvolution] Update on work to port syncevo-dbus-server to GIO GDBus

[Chris Kühl]

Hi,

For a while now I've been working on porting the syncevo-dbus-server
to use the GIO dbus implementation, GDBus.  This is a configure time
option that can be explicitly set using the --with-gio-gdbus flag or,
if no flag is given, looks for an adequate version of GIO and, if
found, builds with GIO GDBus.

The work is taking place on the gio-gdbus branch. The code changes in
the dbus/server/ files have only been made to make things work with
both implementations. The GIO GDBus implmentation lives in the
src/gdbusxx directory.

Work is now being done to correct failures that the test-dbus.py
script bring up. Currently there are no more errors but 23 failures.
This is compared to 11 failures when running the test-dbus.py script
on the master branch.

One side issue... Late last week, I ran into an issue when using the
-O0 gcc flag. It was generating invalid code from the
MethodHandler::add method. With the other optimization levels this
doesn't occur. I'm not sure if this should be considered an issue as
it seems to be a known issue[1] in GCC.

Here are a list of the failures that are unique to each dbus implementation.

==
 :: GIO GDBus only::
==
FAIL TestConnection.testCredentialsRight - send correct credentials
FAIL TestConnection.testCredentialsWrong - send invalid credentials
FAIL TestConnection.testKillInactive - block server with client A,
then let client B connect twice
FAIL TestConnection.testStartSync - send a valid initial SyncML message
FAIL TestConnection.testStartSyncTwice - send the same SyncML message
twice, starting two sessions
FAIL TestConnection.testTimeoutSync - start a sync, then wait for
server to detect that we stopped replying
FAIL TestDBusServerTerm.testNoTerm - D-Bus server must stay around during calls
FAIL TestDBusSession.testSecondSession - a second session should not
run unless the first one stops
FAIL testDBusSyncError.testSyncNoConfig - Executes a real sync with no
corresponding config.
FAIL TestLocalSync.testConcurrency - D-Bus server must remain
responsive while sync runs
FAIL TestLocalSync.testSync - run a simple slow sync between local dirs
FAIL TestSessionAPIsDummy.testAutoSyncLocalConfigError - test that
auto-sync is triggered for local sync, fails due to permanent config
error here
FAIL TestSessionAPIsDummy.testAutoSyncLocalSuccess - test that
auto-sync is done successfully for local sync between file backends
FAIL TestSessionAPIsDummy.testAutoSyncNetworkFailure - test that
auto-sync is triggered, fails due to (temporary?!) network error here
FAIL TestSessionAPIsDummy.testGetConfigWithTempConfig -  test the
config is gotten for a new temporary config.
FAIL TestSessionAPIsDummy.testRestoreByRef - restore data before or
after a given session

==
 :: Bluez GDBus only::
==
FAIL TestDBusServerPresence.testPresenceSignal - check Server.Presence signal
FAIL TestDBusServerPresence.testServerCheckPresence - check
Server.CheckPresence()
FAIL TestDBusServerPresence.testSessionCheckPresence - check
Session.CheckPresence()


And here are the failures that both implementations have in common.

==
 :: Common issues::
==
FAIL TestFileNotify.testRestart - set up auto sync, then check that
server restarts
FAIL TestSessionAPIsDummy.testGetConfigWithTempConfig -  test the
config is gotten for a new temporary config.
FAIL TestSessionAPIsDummy.testInteractivePassword -  test the info
request is correctly working for password
FAIL TestSessionAPIsDummy.testUpdateConfigTemp -  test the config is
just temporary updated but no effect in storage.
FAIL TestSessionAPIsReal.testSync - run a real sync with default
server and test status list and progress number
FAIL TestSessionAPIsReal.testSyncSecondSession - ask for a second
session that becomes ready after a real sync
FAIL TestSessionAPIsReal.testSyncStatusAbort -  test status is set
correctly when the session is aborted
FAIL TestSessionAPIsReal.testSyncStatusSuspend -  test status is set
correctly when the session is suspended

Cheers,
Chris

[1] http://blog.flameeyes.eu/2010/06/14/mailbox-does-gcc-miscompile-at-o0
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Update on work to port syncevo-dbus-server to GIO GDBus

[Chris Kühl]

On Mon, Nov 14, 2011 at 2:31 PM, Patrick Ohly  wrote:
> On Mo, 2011-11-14 at 14:10 +0100, Chris Kühl wrote:
>> For a while now I've been working on porting the syncevo-dbus-server
>> to use the GIO dbus implementation, GDBus.  This is a configure time
>> option that can be explicitly set using the --with-gio-gdbus flag or,
>> if no flag is given, looks for an adequate version of GIO and, if
>> found, builds with GIO GDBus.
>>
>> The work is taking place on the gio-gdbus branch. The code changes in
>> the dbus/server/ files have only been made to make things work with
>> both implementations. The GIO GDBus implmentation lives in the
>> src/gdbusxx directory.
>
> Once you have something that you want to be included in an automatic
> test run, push a branch called "for-master/gio-gdbus" and it will be
> included in the next test run.
>
> Currently I start these runs manually, so drop me a note. The test run
> will also not do much with that particular change except for checking
> that it causes no regressions when --with-gio-gdbus is off, so I would
> add a new build platform using Debian Testing where that flag is set,
> then run the D-Bus tests there.
>

Ok. We should be close to requesting a merge. Only 2 failures to fix
before we've reached parity on our machines.

>> Work is now being done to correct failures that the test-dbus.py
>> script bring up. Currently there are no more errors but 23 failures.
>> This is compared to 11 failures when running the test-dbus.py script
>> on the master branch.
>
> None of those errors occur in the nightly testing of the master branch:
> http://syncev.meego.com/2011-11-12-12-51_all/testing-amd64/nightly.html#dbus
>

I was thinking this may be the case.

> Don't worry too much about the (probably) incomplete test setup, better
> focus on the issues that are caused by the GIO GDBus usage. Eventually
> it would be nice to know whether the other failures are really due to
> some missing setup. They might also be genuine bugs which simply do not
> show up on the test platform.
>

Yeah, déjà vu. ;) My goal is to reach parity now.

> What I see occasionally is that the D-Bus server doesn't shut down
> (quickly enough?) after a test has completed.
>

Ok, I'll keep an eye out for this.

>> One side issue... Late last week, I ran into an issue when using the
>> -O0 gcc flag. It was generating invalid code from the
>> MethodHandler::add method. With the other optimization levels this
>> doesn't occur. I'm not sure if this should be considered an issue as
>> it seems to be a known issue[1] in GCC.
>
> -O0 doesn't really have to work, but I don't like mysteries. If there
> was an endless supply of time, then this should be investigated. There
> may be reasons outside of our source for the failure, but perhaps not.
>

Ok, noted.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Update on work to port syncevo-dbus-server to GIO GDBus

[Chris Kühl]

On Mon, Nov 14, 2011 at 4:49 PM, Chris Kühl  wrote:
> On Mon, Nov 14, 2011 at 2:31 PM, Patrick Ohly  wrote:
>> On Mo, 2011-11-14 at 14:10 +0100, Chris Kühl wrote:
>>> For a while now I've been working on porting the syncevo-dbus-server
>>> to use the GIO dbus implementation, GDBus.  This is a configure time
>>> option that can be explicitly set using the --with-gio-gdbus flag or,
>>> if no flag is given, looks for an adequate version of GIO and, if
>>> found, builds with GIO GDBus.
>>>
>>> The work is taking place on the gio-gdbus branch. The code changes in
>>> the dbus/server/ files have only been made to make things work with
>>> both implementations. The GIO GDBus implmentation lives in the
>>> src/gdbusxx directory.
>>
>> Once you have something that you want to be included in an automatic
>> test run, push a branch called "for-master/gio-gdbus" and it will be
>> included in the next test run.
>>
>> Currently I start these runs manually, so drop me a note. The test run
>> will also not do much with that particular change except for checking
>> that it causes no regressions when --with-gio-gdbus is off, so I would
>> add a new build platform using Debian Testing where that flag is set,
>> then run the D-Bus tests there.
>>
>
> Ok. We should be close to requesting a merge. Only 2 failures to fix
> before we've reached parity on our machines.
>

After fixing those remaining issues, we ran the server under valgrind
and fixed up a few memory leaks we found. I've now created a
for-master/gio-gdbus branch, squashed the commits and pushed.

>>> Work is now being done to correct failures that the test-dbus.py
>>> script bring up. Currently there are no more errors but 23 failures.
>>> This is compared to 11 failures when running the test-dbus.py script
>>> on the master branch.
>>
>> None of those errors occur in the nightly testing of the master branch:
>> http://syncev.meego.com/2011-11-12-12-51_all/testing-amd64/nightly.html#dbus
>>
>
> I was thinking this may be the case.
>
>> Don't worry too much about the (probably) incomplete test setup, better
>> focus on the issues that are caused by the GIO GDBus usage. Eventually
>> it would be nice to know whether the other failures are really due to
>> some missing setup. They might also be genuine bugs which simply do not
>> show up on the test platform.
>>
>
> Yeah, déjà vu. ;) My goal is to reach parity now.
>

I currently get 8 failures for each wrapper on my machine and
Krzesimir gets 5 failures. We are both on Fedora 15.

>> What I see occasionally is that the D-Bus server doesn't shut down
>> (quickly enough?) after a test has completed.
>>
>
> Ok, I'll keep an eye out for this.
>

3 of my failures magically went away so this could have been it.

I'll now start to work out the plan for running multiple concurrent sessions.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] syncevo-dbus-server + forking processes

[Chris Kühl]

On Wed, Nov 16, 2011 at 12:22 PM, Patrick Ohly  wrote:
> Hello!
>
> Chris is going to work on running syncs inside a process forked from the
> main syncevo-dbus-server. Before diving into the planning and
> implementation, let me outline the background.
>
> At the moment, syncevo-dbus-server is one process (when ignoring local
> sync for a second) which handles D-Bus calls and syncs at the same time.
> A sync involves operations which can block for a long time, so keeping
> the D-Bus part alive and responsive is challenging. It involves hacks
> like entering and leaving the main event loop, checking it inside
> libsoup, etc.
>
> Local sync already uses a second forked process so that it can run
> client and server engines concurrently.
>
> Getting rid of the current hacks is one motivation for running syncs in
> a forked process. The main process would enter the main loop once and
> never return unless it was asked to shut down.
>
> The other is that (eventually) it'll allow to run a "clean" main process
> which never loads any of the backend modules. That may help to keep the
> permanent memory footprint smaller than it is today. Right now this is
> not possible because merely registering a backend requires loading it.
> Fixing that is outside of the current scope.
>
> Resource leaks inside a sync become less problematic when the process
> running the sync can shut down.
>
> Here are some requirements for "sync in forked process":
>      * should be disabled when running outside of the D-Bus daemon
>        (syncevolution command line in non-daemon mode, client-test
>        testing)
>      * interactive password requests through D-Bus must continue to
>        work
>      * abort/suspend requests via D-Bus must work
>      * the forked process should shut down cleanly, so that "valgrind
>        --follow-child=yes --leak-check=yes" provides sane results (the
>        forked process in a local sync currently fails to do that,
>        because it is forked in a less controlled environment and
>        therefore just calls _exit())
>      * logging:
>              * the main syncevo-dbus-server process should use syslog
>                logging for its own messages, without redirecting
>                stdout/stderr
>              * the forked process should start redirecting
>                stdout/stderr into the normal logging mechanism (so that
>                library error messages are visible when running the
>                syncevolution command line as client of the D-Bus server
>                and, later, they appear in log files) and switch to a
>                session log when the session is ready
>

Thanks for the list of requirements.  I'll take these into
consideration. Some in there that we hadn't talked about before.

> Somewhat related to this work is support for push sync. I suspect that
> this will also use forked processes to monitor remote and local
> databases without interfering with the main process. It would be good if
> the mechanism for forking and communication between process was generic
> and not tied to syncing alone.
>

I'm not really well informed on the plans for push sync other that
what I found in [1], but the idea that I had in mind was for the
forking process to initiate a d-bus peer-to-peer ("one-to-one" in the
d-bus spec) connection by forking the process with the d-bus address
as a command line argument. The client then uses that for
communication. This seems pretty generic to me. The only differences
would be the d-bus interface used. ...but maybe I'm misunderstanding
what you mean.

[1] https://bugs.meego.com/show_bug.cgi?id=23762
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Update on work to port syncevo-dbus-server to GIO GDBus

[Chris Kühl]

On Wed, Nov 16, 2011 at 2:52 PM, Patrick Ohly  wrote:
> On Mi, 2011-11-16 at 11:58 +0100, Patrick Ohly wrote:
>> On Mi, 2011-11-16 at 11:10 +0100, Chris Kühl wrote:
>> > On Mon, Nov 14, 2011 at 4:49 PM, Chris Kühl  wrote:
>> > > On Mon, Nov 14, 2011 at 2:31 PM, Patrick Ohly  
>> > > wrote:
>> > > Ok. We should be close to requesting a merge. Only 2 failures to fix
>> > > before we've reached parity on our machines.
>> > >
>> >
>> > After fixing those remaining issues, we ran the server under valgrind
>> > and fixed up a few memory leaks we found. I've now created a
>> > for-master/gio-gdbus branch, squashed the commits and pushed.
>>
>> I've started a test run.
>
> There seems to be a regression in the D-Bus testing.
>
> On Ubuntu Lucid, startup of the syncevo-dbus-server without GIO GDBus
> fails:
> http://syncev.meego.com/2011-11-16-10-54_dist/lucid-amd64/6-dbus/output.txt
>

This indeed does not look good.

> The same binary runs on Debian Testing, albeit with regressions in
> TestSessionAPIsReal.testSync and
> TestSessionAPIsReal.testSyncSecondSession :
> http://syncev.meego.com/2011-11-16-10-54_dist/prebuilt-amd64/6-dbus/output.txt
>

These were failing for me using master on my fedora 15 system so
haven't been properly debugged in gio-gdbus.

> The nightly.html doesn't properly report the individual D-Bus errors,
> only the overall failure is correctly reported - I will investigate
> that.
>
> For comparison, this used to work in the previous test run:
> http://syncev.meego.com/2011-11-12-12-51_all/lucid-amd64/nightly.html
>
> Linking on Debian Testing fails because some library symbol is used
> without directly linking against that library (the binutils on that
> platform are more demanding and check for this):
> http://syncev.meego.com/2011-11-16-10-54_dist/gcc-4.6-amd64/3-compile/output.txt
>
> It should be fairly easy for me to fix these issues, while it is harder
> for you unless you have access to the right platforms or know what the
> issue is. Just let me know if you want me to lend a hand.
>

I'll setup a virtual machine for debian stable and ubuntu LTS and see
if I can get things working properly with those distros.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Update on work to port syncevo-dbus-server to GIO GDBus

[Chris Kühl]

On Thu, Nov 17, 2011 at 6:14 PM, Chris Kühl  wrote:
> 2011/11/17 Patrick Ohly :
>> On Mi, 2011-11-16 at 15:40 +0100, Chris Kühl wrote:
>>> > The same binary runs on Debian Testing, albeit with regressions in
>>> > TestSessionAPIsReal.testSync and
>>> > TestSessionAPIsReal.testSyncSecondSession :
>>> >
>>> http://syncev.meego.com/2011-11-16-10-54_dist/prebuilt-amd64/6-dbus/output.txt
>>> >
>>>
>>> These were failing for me using master on my fedora 15 system so
>>> haven't been properly debugged in gio-gdbus.
>>
>> It turned that the 101 status in these failures is a remote error sent
>> by the plan44.ch SyncML server, which is the server that these real
>> syncs are meant to be done with. I've switched to Memotoo as peer for
>> the D-Bus tests for now.
>>
>> Lukas, any idea about the reason for the 101 error? I don't see any log
>> created for this session on the server. The error occurs right at the
>> start, as the status returned for the initial SyncHdr.
>>
>> --
>
> Ok, thanks for the update and glad to see that's working. I'm still
> trying to work out what's going on with Ubuntu Lucid. I'm getting the
> same result with Debian Stable as well, though. ...back to bug fixing.
>

I've pushed a commit[1] to the gio-gdbus branch that gets the server
starting up fine. Seems to have been a compiler bug. However, I'm
currently getting the following error when running each test.

ERROR:dbus.proxies:Introspect error on
:1.140:/org/syncevolution/Server: dbus.exceptions.DBusException:
org.freedesktop.DBus.Error.NoReply: Did not receive a reply. Possible
causes include: the remote application did not send a reply, the
message bus security policy blocked the reply, the reply timeout
expired, or the network connection was broken.

Tracking this down now.

Cheers,
Chris

[1] 
https://meego.gitorious.org/meego-middleware/syncevolution/commit/e2d5009ff282e67fde5fba4c74e38c92a5fc7af0/diffs/5baf3ad4c78efe23807aca4cd1550080600da18c
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Update on work to port syncevo-dbus-server to GIO GDBus

[Chris Kühl]

On Wed, Nov 16, 2011 at 2:52 PM, Patrick Ohly  wrote:
> On Mi, 2011-11-16 at 11:58 +0100, Patrick Ohly wrote:
>> On Mi, 2011-11-16 at 11:10 +0100, Chris Kühl wrote:
>> > On Mon, Nov 14, 2011 at 4:49 PM, Chris Kühl  wrote:
>> > > On Mon, Nov 14, 2011 at 2:31 PM, Patrick Ohly  
>> > > wrote:
>> > > Ok. We should be close to requesting a merge. Only 2 failures to fix
>> > > before we've reached parity on our machines.
>> > >
>> >
>> > After fixing those remaining issues, we ran the server under valgrind
>> > and fixed up a few memory leaks we found. I've now created a
>> > for-master/gio-gdbus branch, squashed the commits and pushed.
>>
>> I've started a test run.
>
> There seems to be a regression in the D-Bus testing.
>
> On Ubuntu Lucid, startup of the syncevo-dbus-server without GIO GDBus
> fails:
> http://syncev.meego.com/2011-11-16-10-54_dist/lucid-amd64/6-dbus/output.txt
>

So, I finally got the issue in Debian Stable and Ubuntu LTS fixed.
Turned out to be that I'd inadvertently set the server's main
connection to shared instead of private. I'm not sure why this was
only causing issues on these distros. That initially sent me searching
for the issue in all the wrong places.

Along the way I was also able to fix the --enable-notify flag which
was broken. You can now actually disable notifications.

This will all be in for-master/gio-gdbus within the next hour or so.

> The same binary runs on Debian Testing, albeit with regressions in
> TestSessionAPIsReal.testSync and
> TestSessionAPIsReal.testSyncSecondSession :
> http://syncev.meego.com/2011-11-16-10-54_dist/prebuilt-amd64/6-dbus/output.txt
>
> The nightly.html doesn't properly report the individual D-Bus errors,
> only the overall failure is correctly reported - I will investigate
> that.
>
> For comparison, this used to work in the previous test run:
> http://syncev.meego.com/2011-11-12-12-51_all/lucid-amd64/nightly.html
>
> Linking on Debian Testing fails because some library symbol is used
> without directly linking against that library (the binutils on that
> platform are more demanding and check for this):
> http://syncev.meego.com/2011-11-16-10-54_dist/gcc-4.6-amd64/3-compile/output.txt
>
> It should be fairly easy for me to fix these issues, while it is harder
> for you unless you have access to the right platforms or know what the
> issue is. Just let me know if you want me to lend a hand.
>

Can you try these again, I'm not running into these issues on Debian Testing.

Cheers,
Chris

> --
> Best Regards, Patrick Ohly
>
> The content of this message is my personal opinion only and although
> I am an employee of Intel, the statements I make here in no way
> represent Intel's position on the issue, nor am I authorized to speak
> on behalf of Intel on this matter.
>
>
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] syncevo-dbus-server + forking processes

[Chris Kühl]

On Wed, Nov 16, 2011 at 2:39 PM, Patrick Ohly  wrote:
> On Mi, 2011-11-16 at 13:55 +0100, Chris Kühl wrote:
>> On Wed, Nov 16, 2011 at 12:22 PM, Patrick Ohly  
>> wrote:
>> > Here are some requirements for "sync in forked process":
>> >      * should be disabled when running outside of the D-Bus daemon
>> >        (syncevolution command line in non-daemon mode, client-test
>> >        testing)
>> >      * interactive password requests through D-Bus must continue to
>> >        work
>> >      * abort/suspend requests via D-Bus must work
>> >      * the forked process should shut down cleanly, so that "valgrind
>> >        --follow-child=yes --leak-check=yes" provides sane results (the
>> >        forked process in a local sync currently fails to do that,
>> >        because it is forked in a less controlled environment and
>> >        therefore just calls _exit())
>> >      * logging:
>> >              * the main syncevo-dbus-server process should use syslog
>> >                logging for its own messages, without redirecting
>> >                stdout/stderr
>> >              * the forked process should start redirecting
>> >                stdout/stderr into the normal logging mechanism (so that
>> >                library error messages are visible when running the
>> >                syncevolution command line as client of the D-Bus server
>> >                and, later, they appear in log files) and switch to a
>> >                session log when the session is ready
>> >
>>
>> Thanks for the list of requirements.  I'll take these into
>> consideration. Some in there that we hadn't talked about before.
>
> I know, that's why I'd rather discuss this upfront. Some of these
> requirements are implied by "change must not cause
> regressions" (interactive passwords, abort/suspend), others are open
> questions (startup/shutdown of the forked process, logging).
>
>> > Somewhat related to this work is support for push sync. I suspect that
>> > this will also use forked processes to monitor remote and local
>> > databases without interfering with the main process. It would be good if
>> > the mechanism for forking and communication between process was generic
>> > and not tied to syncing alone.
>> >
>>
>> I'm not really well informed on the plans for push sync other that
>> what I found in [1],
>
> There are no more specific plans yet.
>
>>  but the idea that I had in mind was for the
>> forking process to initiate a d-bus peer-to-peer ("one-to-one" in the
>> d-bus spec) connection by forking the process with the d-bus address
>> as a command line argument. The client then uses that for
>> communication. This seems pretty generic to me. The only differences
>> would be the d-bus interface used. ...but maybe I'm misunderstanding
>> what you mean.
>
> No, that's what I was looking for. When you say "command line argument",
> does that mean that there will be an exec() involved? Would that start
> the same binary or something else?
>

Yes this was going to be my proposal.

> The advantage of the exec() is that the new process is much cleaner. The
> downside is that it is harder to implement (How does the forking process
> find the "right" executable?

I believe the place for the "helper" sync binary would be in the
libexec directory: /usr/libexec/syncevolution/sync-session for
example.

How does injecting valgrind into the new
> binary work?)

Valgrind can follow child processes with --trace-children=yes and
create a log-file per process by inserting %p in the log-file name.

and a bit more expensive (must load all libraries again).
>

This will definitely require a small bit more resources to be used and
take a few microseconds longer to start but these are resources that
will be mostly loaded from memory (and probably cache) after the first
sync session is started and be freed when the process disappears.
Also, the time a fork-exec pair takes over just a fork is
insignificant compared to how long a sync session takes.

> It might be easier to avoid the exec() and instead return to main() of
> the syncevo-dbus-server in the forked process (including freeing of all
> parent process resources, which must be done anyway),

The potential problem I see is that some of the resources we may not
be able to free because they are not under our control: libraries,
etc.

then go down a
> different path as if the binary had just been started.
>

This is how I originally intended to implement this till I started
looking into it a little more and asking around. To me, the potential
problems seem to outweigh the advantages.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] syncevo-dbus-server + forking processes

[Chris Kühl]

On Tue, Nov 29, 2011 at 8:40 AM, Gapps + local IMAP
 wrote:
> On Mo, 2011-11-28 at 18:00 +0100, Chris Kühl wrote:
>> On Wed, Nov 16, 2011 at 2:39 PM, Patrick Ohly  wrote:
>> > No, that's what I was looking for. When you say "command line argument",
>> > does that mean that there will be an exec() involved? Would that start
>> > the same binary or something else?
>> >
>>
>> Yes this was going to be my proposal.
>>
>> > The advantage of the exec() is that the new process is much cleaner. The
>> > downside is that it is harder to implement (How does the forking process
>> > find the "right" executable?
>>
>> I believe the place for the "helper" sync binary would be in the
>> libexec directory: /usr/libexec/syncevolution/sync-session for
>> example.
>
> Please also add an env variable which overrides that default location.
> The nightly testing needs that because it runs with an installation that
> was made with DESTDIR and thus doesn't have the files in the hard-coded
> places.
>
>> > How does injecting valgrind into the new
>> > binary work?)
>>
>> Valgrind can follow child processes with --trace-children=yes and
>> create a log-file per process by inserting %p in the log-file name.
>
> Duh, of course.
>
> [fork + function call from main()]
>
>> This is how I originally intended to implement this till I started
>> looking into it a little more and asking around. To me, the potential
>> problems seem to outweigh the advantages.
>
> Your concerns mirror my own concerns, so agreed, let's do the fork+exec.
>

Ok, I'll proceed in that direction and post a more detailed plan once
I do a little more poking.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Update on work to port syncevo-dbus-server to GIO GDBus

[Chris Kühl]

On Tue, Nov 29, 2011 at 12:12 PM, Patrick Ohly  wrote:
> On Mo, 2011-11-28 at 12:24 +0100, Chris Kühl wrote:
>> On Wed, Nov 16, 2011 at 2:52 PM, Patrick Ohly  wrote:
>> > On Mi, 2011-11-16 at 11:58 +0100, Patrick Ohly wrote:
>> >> On Mi, 2011-11-16 at 11:10 +0100, Chris Kühl wrote:
>> >> > On Mon, Nov 14, 2011 at 4:49 PM, Chris Kühl  wrote:
>> >> > > On Mon, Nov 14, 2011 at 2:31 PM, Patrick Ohly 
>> >> > >  wrote:
>> >> > > Ok. We should be close to requesting a merge. Only 2 failures to fix
>> >> > > before we've reached parity on our machines.
>> >> > >
>> >> >
>> >> > After fixing those remaining issues, we ran the server under valgrind
>> >> > and fixed up a few memory leaks we found. I've now created a
>> >> > for-master/gio-gdbus branch, squashed the commits and pushed.
>> >>
>> >> I've started a test run.
>> >
>> > There seems to be a regression in the D-Bus testing.
>> >
>> > On Ubuntu Lucid, startup of the syncevo-dbus-server without GIO GDBus
>> > fails:
>> > http://syncev.meego.com/2011-11-16-10-54_dist/lucid-amd64/6-dbus/output.txt
>> >
>>
>> So, I finally got the issue in Debian Stable and Ubuntu LTS fixed.
>> Turned out to be that I'd inadvertently set the server's main
>> connection to shared instead of private. I'm not sure why this was
>> only causing issues on these distros. That initially sent me searching
>> for the issue in all the wrong places.
>
> lucid-amd64 without GIO GDBus passed the tests now. I had to take the
> for-master/gio-gdbus branch temporarily out of testing because the
> autoconf changes conflicted with another branch, but now it is back and
> was tested.
>
> However, it segfaults during startup on Debian Testing with GIO GDBus
> enabled:
> http://syncev.meego.com/latest/testing-amd64-gio/6-dbus/output.txt
>

Link is broken. But I've not had that issue with Debian Testing. Hmm?
I've got a clean install with only the necessary packages added for
building and running syncevolution.

> I'm going to have a look and then merge. As long as it doesn't cause
> regressions and isn't enabled by default, I don't mind fixing problems
> in new features on the master branch. Hmm, wait, GIO GDBus is used by
> default if enabled. Let's see...
>

Feel free to change the default to use the bluez code if you feel more
comfortable with that. But I'd assume we will eventually want this to
be the default.

>> Along the way I was also able to fix the --enable-notify flag which
>> was broken. You can now actually disable notifications.
>>
>> This will all be in for-master/gio-gdbus within the next hour or so.
>>
>> > The same binary runs on Debian Testing, albeit with regressions in
>> > TestSessionAPIsReal.testSync and
>> > TestSessionAPIsReal.testSyncSecondSession :
>> > http://syncev.meego.com/2011-11-16-10-54_dist/prebuilt-amd64/6-dbus/output.txt
>> >
>> > The nightly.html doesn't properly report the individual D-Bus errors,
>> > only the overall failure is correctly reported - I will investigate
>> > that.
>> >
>> > For comparison, this used to work in the previous test run:
>> > http://syncev.meego.com/2011-11-12-12-51_all/lucid-amd64/nightly.html
>
> This was the temporary plan44.ch server issue.
>

Yes, I saw that.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Update on work to port syncevo-dbus-server to GIO GDBus

[Chris Kühl]

On Tue, Nov 29, 2011 at 2:31 PM, Patrick Ohly  wrote:
> On Di, 2011-11-29 at 13:20 +0100, Chris Kühl wrote:
>> On Tue, Nov 29, 2011 at 12:12 PM, Patrick Ohly  
>> wrote:
>> > On Mo, 2011-11-28 at 12:24 +0100, Chris Kühl wrote:
>> >> On Wed, Nov 16, 2011 at 2:52 PM, Patrick Ohly  
>> >> wrote:
>> >> > On Mi, 2011-11-16 at 11:58 +0100, Patrick Ohly wrote:
>> >> >> On Mi, 2011-11-16 at 11:10 +0100, Chris Kühl wrote:
>> >> >> > On Mon, Nov 14, 2011 at 4:49 PM, Chris Kühl  
>> >> >> > wrote:
>> >> >> > > On Mon, Nov 14, 2011 at 2:31 PM, Patrick Ohly 
>> >> >> > >  wrote:
>> >> >> > > Ok. We should be close to requesting a merge. Only 2 failures to 
>> >> >> > > fix
>> >> >> > > before we've reached parity on our machines.
>> >> >> > >
>> >> >> >
>> >> >> > After fixing those remaining issues, we ran the server under valgrind
>> >> >> > and fixed up a few memory leaks we found. I've now created a
>> >> >> > for-master/gio-gdbus branch, squashed the commits and pushed.
>> >> >>
>> >> >> I've started a test run.
>> >> >
>> >> > There seems to be a regression in the D-Bus testing.
>> >> >
>> >> > On Ubuntu Lucid, startup of the syncevo-dbus-server without GIO GDBus
>> >> > fails:
>> >> > http://syncev.meego.com/2011-11-16-10-54_dist/lucid-amd64/6-dbus/output.txt
>> >> >
>> >>
>> >> So, I finally got the issue in Debian Stable and Ubuntu LTS fixed.
>> >> Turned out to be that I'd inadvertently set the server's main
>> >> connection to shared instead of private. I'm not sure why this was
>> >> only causing issues on these distros. That initially sent me searching
>> >> for the issue in all the wrong places.
>> >
>> > lucid-amd64 without GIO GDBus passed the tests now. I had to take the
>> > for-master/gio-gdbus branch temporarily out of testing because the
>> > autoconf changes conflicted with another branch, but now it is back and
>> > was tested.
>> >
>> > However, it segfaults during startup on Debian Testing with GIO GDBus
>> > enabled:
>> > http://syncev.meego.com/latest/testing-amd64-gio/6-dbus/output.txt
>> >
>>
>> Link is broken.
>
> Ah, the "latest" symlink. I need to remember not to use that in email...
>
>>  But I've not had that issue with Debian Testing. Hmm?
>> I've got a clean install with only the necessary packages added for
>> building and running syncevolution.
>
> It happens when Bluez is not running.
>

Oh, that's why I didn't hit it.

> Program received signal SIGSEGV, Segmentation fault.
> 0x00597e36 in set (error=0xb7e4c0, this=0x0) at 
> /data/runtests/work/sources/syncevolution/src/gdbusxx/gdbus-cxx-bridge.h:164
> warning: Source file is more recent than executable.
> 164             if (m_error) {
> (gdb) where
> #0  0x00597e36 in set (error=0xb7e4c0, this=0x0) at 
> /data/runtests/work/sources/syncevolution/src/gdbusxx/gdbus-cxx-bridge.h:164
> #1  GDBusCXX::dbus_get_bus_connection (busType=, name=0x0, 
> unshared=, err=0x0) at 
> /data/runtests/work/sources/syncevolution/src/gdbusxx/gdbus-cxx-bridge.cpp:68
> #2  0x00574450 in SyncEvo::BluezManager::BluezManager (this=0xb85740, 
> server=) at 
> /data/runtests/work/sources/syncevolution/src/dbus/server/bluez-manager.cpp:44
> #3  0x00541250 in SyncEvo::Server::Server (this=0x7fffd9a0, 
> loop=0xb6eab0, shutdownRequested=@0xb46af8, restart=..., conn= out>, duration=600)
>    at /data/runtests/work/sources/syncevolution/src/dbus/server/server.cpp:227
> #4  0x0052121c in main (argc=1, argv=0x7fffdfa8, envp= out>) at 
> /data/runtests/work/sources/syncevolution/src/dbus/server/main.cpp:113
>
> GDBusCXX::dbus_get_bus_connection() takes a GDBusCXX::DBusErrorCXX *err
> parameter and doesn't check it for NULL:
>
> 63          } else {
> 64              // This returns a singleton, shared connection object.
> 65              conn = g_bus_get_sync(boost::iequals(busType, "SESSION") ?
> G_BUS_TYPE_SESSION : G_BUS_TYPE_SYSTEM,
> 66                                    NULL, &error);
> 67              if(error != NULL) {
> 68                  err->set(error);   <==
> 69        

Re: [SyncEvolution] Update on work to port syncevo-dbus-server to GIO GDBus

[Chris Kühl]

On Tue, Nov 29, 2011 at 5:56 PM, Patrick Ohly  wrote:
> On Di, 2011-11-29 at 15:09 +0100, Chris Kühl wrote:
>> On Tue, Nov 29, 2011 at 2:31 PM, Patrick Ohly  wrote:
>> > On Di, 2011-11-29 at 13:20 +0100, Chris Kühl wrote:
> [patch with NULL check]
>> > Does that look right?
>> >
>>
>> Seems fine to me.
>
> I've merged your branch and applied the fix.
>
>> > Speaking of DBusErrorCXX, why does it have a separate "message" member?
>> >
>>
>> That's only in there to maintain compatibility. I believe the examples
>> were the only place the message field was used however. We could
>> easliy change those and get read of this member.
>
> Yes, let's do that. I'm working on a patch.

Ok.

>
> Looking closer at compiler output, I saw a warning about this code:
>
> class DBusResult : virtual public Result
> {
> [...]
>    virtual void failed(const dbus_error &error)
>    {
>        GDBusMessage *errMsg;
>        errMsg = g_dbus_message_new_method_error(m_msg.get(), 
> error.dbusName().c_str(),
>                                                 "%s", error.what());
> ===>    if (!g_dbus_connection_send_message(m_conn.get(), m_msg.get(),
>                                            G_DBUS_SEND_MESSAGE_FLAGS_NONE, 
> NULL, NULL)) {
>            throw std::runtime_error(" g_dbus_connection_send_message failed");
>        }
>    }
>
> gdbus-cxx-bridge.h: In member function 'virtual void 
> GDBusCXX::DBusResult::failed(const GDBusCXX::dbus_error&)':
> gdbus-cxx-bridge.h:1778:23: warning: variable 'errMsg' set but not used 
> [-Wunused-but-set-variable]
>
> Is errMsg the message which is meant to be sent above?
>

Yes. Thanks for catching that.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] testing syncevo-dbus-server under valgrind

[Chris Kühl]

On Thu, Dec 1, 2011 at 11:32 AM, Patrick Ohly  wrote:
> Hello!
>
> I've changed the automatic testing so that syncevo-dbus-server is
> started under valgrindcheck.sh by test-dbus.py. This revealed some minor
> memory leaks and broken memory accesses (typically read after free). I
> I've fixed all of that, see master branch.
>

Nice.

> The last remaining issue seems to be a leak in the error path of GIO
> GDBus: when a service is not available,
> g_dbus_connection_new_for_address_sync() leaks some memory.
>
> http://syncev.meego.com/2011-12-01-10-06_syncevolution_dbus_gio-gdbus/gio-gdbus/6-dbus/output.txt
>
> ==10608== 60 (16 direct, 44 indirect) bytes in 1 blocks are definitely lost 
> in loss record 911 of 1,653
> ==10608==    at 0x4C27673: malloc (vg_replace_malloc.c:263)
> ==10608==    by 0x7FABC02: g_malloc (in 
> /lib/x86_64-linux-gnu/libglib-2.0.so.0.3000.2)
> ==10608==    by 0x7FC0976: g_slice_alloc (in 
> /lib/x86_64-linux-gnu/libglib-2.0.so.0.3000.2)
> ==10608==    by 0x7F9045F: g_error_copy (in 
> /lib/x86_64-linux-gnu/libglib-2.0.so.0.3000.2)
> ==10608==    by 0x76700B1: ??? (in 
> /usr/lib/x86_64-linux-gnu/libgio-2.0.so.0.3000.2)
> ==10608==    by 0x761465E: g_initable_new_valist (in 
> /usr/lib/x86_64-linux-gnu/libgio-2.0.so.0.3000.2)
> ==10608==    by 0x7614748: g_initable_new (in 
> /usr/lib/x86_64-linux-gnu/libgio-2.0.so.0.3000.2)
> ==10608==    by 0x766EB1E: g_dbus_connection_new_for_address_sync (in 
> /usr/lib/x86_64-linux-gnu/libgio-2.0.so.0.3000.2)
> ==10608==    by 0x597E08: GDBusCXX::dbus_get_bus_connection(char const*, char 
> const*, bool, GDBusCXX::DBusErrorCXX*) (gdbus-cxx-bridge.cpp:58)
> ==10608==    by 0x583CB8: 
> SyncEvo::ConnmanClient::ConnmanClient(SyncEvo::Server&) 
> (connman-client.cpp:30)
> ==10608==    by 0x541730: SyncEvo::Server::Server(_GMainLoop*, bool&, 
> boost::shared_ptr&, GDBusCXX::DBusConnectionPtr const&, 
> int) (server.cpp:239)
> ==10608==    by 0x52141B: main (main.cpp:113)
>
> ==10608== 60 (16 direct, 44 indirect) bytes in 1 blocks are definitely lost 
> in loss record 912 of 1,653
> ==10608==    at 0x4C27673: malloc (vg_replace_malloc.c:263)
> ==10608==    by 0x7FABC02: g_malloc (in 
> /lib/x86_64-linux-gnu/libglib-2.0.so.0.3000.2)
> ==10608==    by 0x7FC0976: g_slice_alloc (in 
> /lib/x86_64-linux-gnu/libglib-2.0.so.0.3000.2)
> ==10608==    by 0x7F9045F: g_error_copy (in 
> /lib/x86_64-linux-gnu/libglib-2.0.so.0.3000.2)
> ==10608==    by 0x76700B1: ??? (in 
> /usr/lib/x86_64-linux-gnu/libgio-2.0.so.0.3000.2)
> ==10608==    by 0x761465E: g_initable_new_valist (in 
> /usr/lib/x86_64-linux-gnu/libgio-2.0.so.0.3000.2)
> ==10608==    by 0x7614748: g_initable_new (in 
> /usr/lib/x86_64-linux-gnu/libgio-2.0.so.0.3000.2)
> ==10608==    by 0x766EB1E: g_dbus_connection_new_for_address_sync (in 
> /usr/lib/x86_64-linux-gnu/libgio-2.0.so.0.3000.2)
> ==10608==    by 0x597E08: GDBusCXX::dbus_get_bus_connection(char const*, char 
> const*, bool, GDBusCXX::DBusErrorCXX*) (gdbus-cxx-bridge.cpp:58)
> ==10608==    by 0x58A9EE: 
> SyncEvo::NetworkManagerClient::NetworkManagerClient(SyncEvo::Server&) 
> (network-manager-client.cpp:31)
> ==10608==    by 0x541747: SyncEvo::Server::Server(_GMainLoop*, bool&, 
> boost::shared_ptr&, GDBusCXX::DBusConnectionPtr const&, 
> int) (server.cpp:239)
> ==10608==    by 0x52141B: main (main.cpp:113)
>
> Chris, do you agree that this isn't something caused by our code?
>
> If yes, then I'll suppress this particular problem.
>

Just took some time to look at that and I can't see how the syncevo
code could cause this. So, yeah, suppressing this sounds right.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] syncevo-dbus-server + forking processes

[Chris Kühl]

On Wed, Nov 16, 2011 at 12:22 PM, Patrick Ohly  wrote:
> Hello!
>
> Chris is going to work on running syncs inside a process forked from the
> main syncevo-dbus-server. Before diving into the planning and
> implementation, let me outline the background.
>
> At the moment, syncevo-dbus-server is one process (when ignoring local
> sync for a second) which handles D-Bus calls and syncs at the same time.
> A sync involves operations which can block for a long time, so keeping
> the D-Bus part alive and responsive is challenging. It involves hacks
> like entering and leaving the main event loop, checking it inside
> libsoup, etc.
>
> Local sync already uses a second forked process so that it can run
> client and server engines concurrently.
>
> Getting rid of the current hacks is one motivation for running syncs in
> a forked process. The main process would enter the main loop once and
> never return unless it was asked to shut down.
>
> The other is that (eventually) it'll allow to run a "clean" main process
> which never loads any of the backend modules. That may help to keep the
> permanent memory footprint smaller than it is today. Right now this is
> not possible because merely registering a backend requires loading it.
> Fixing that is outside of the current scope.
>
> Resource leaks inside a sync become less problematic when the process
> running the sync can shut down.
>
> Here are some requirements for "sync in forked process":
>      * should be disabled when running outside of the D-Bus daemon
>        (syncevolution command line in non-daemon mode, client-test
>        testing)

This seems pretty straightforward because when in non-daemon mode none
of the src/dbus/server code is touched anyway. I'm hoping to disturb
the code in src/syncevo as little as possible. But I've not touched
src/syncevolution.cpp much so maybe I'm missing something.

>      * interactive password requests through D-Bus must continue to
>        work
>      * abort/suspend requests via D-Bus must work
>      * the forked process should shut down cleanly, so that "valgrind
>        --follow-child=yes --leak-check=yes" provides sane results (the
>        forked process in a local sync currently fails to do that,
>        because it is forked in a less controlled environment and
>        therefore just calls _exit())

Using fork then exec should mostly relieve us of this issue.

>      * logging:
>              * the main syncevo-dbus-server process should use syslog
>                logging for its own messages, without redirecting
>                stdout/stderr
>              * the forked process should start redirecting
>                stdout/stderr into the normal logging mechanism (so that
>                library error messages are visible when running the
>                syncevolution command line as client of the D-Bus server
>                and, later, they appear in log files) and switch to a
>                session log when the session is ready
>

I plan to do this work in 3 steps:

1) The first step, which I've started, is to decouple the session and
server from each other and also modify objects that require both the
session and server objects as needed. Once this is done the server
should function as it does now and all tests should pass in order to
move on to step 2.
2) Move session into separate binary using a one-to-one dbus
connection for communicating between the 2 processes. At this stage
only one session is run at a time and all tests should pass as before.
3) Enable running multiple concurrent sync sessions. At this stage
we'll probably need to add/modify some tests in order to deal with the
new functionality.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] SE_2011-12-02-16-36_dist: lucid-amd64

[Chris Kühl]

On Mon, Dec 5, 2011 at 12:09 PM, Patrick Ohly  wrote:
> Good news!
>
> Automatic integration testing is working and catches problems that are
> hard to detect without the automated tests. From a recent run:
>
> On Fri, 2011-12-02 at 09:18 -0800, syncevo-do-not-re...@meego.com wrote:
>> syncevolution
> [...]
>> origin/for-master/make-notification-file-names-conformant: okay
>
>> Patches:
> [...]
>> =?UTF-8?q?Chris=20K=C3=BChl?= - dbus-server: Make dbus-server
>> notification file names conformant
>
> Note to self: check UTF-8 handling in the scripts.
>
>> prepare
>> Value
>> libsynthesis-fetch-config
>> okay
>> syncevolution-fetch-config
>> okay
>> compile
>> okay
>> dist
>> failed
>> Total passed cases (all: 4)
>> 3
>
>
> "make dist" failed because the renamed files were still referenced in
> the "po" directory. Fixed.

Thanks.

>
> Tip: use "git grep" to find all occurrences of a string in a project.
>

I did, but only in ./src/. oops. ;)

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] syncevo-dbus-server + forking processes

[Chris Kühl]

On Mon, Dec 5, 2011 at 2:12 PM, Patrick Ohly  wrote:
> On Mon, 2011-12-05 at 12:12 +0100, Chris Kühl wrote:
>> On Wed, Nov 16, 2011 at 12:22 PM, Patrick Ohly  
>> wrote:
>> > Here are some requirements for "sync in forked process":
>> >      * should be disabled when running outside of the D-Bus daemon
>> >        (syncevolution command line in non-daemon mode, client-test
>> >        testing)
>>
>> This seems pretty straightforward because when in non-daemon mode none
>> of the src/dbus/server code is touched anyway. I'm hoping to disturb
>> the code in src/syncevo as little as possible. But I've not touched
>> src/syncevolution.cpp much so maybe I'm missing something.
>
> Yes, this should work. libsyncevolution (aka src/syncevo) communicates
> with the front-end via ConfigUserInterface (implemented differently for
> command line and D-Bus server) and several virtual methods in
> SyncContext (display*(), for example displaySyncProgress()).
>
> In particular the password request mechanism is very convoluted. I
> haven't written that part myself and each time I look at it, I need to
> trace through the various methods before I can piece together how they
> interact.
>
> I suspect that this will have to be cleaned up. Perhaps SyncContext
> should be parameterized with instances of ConfigUserIterface and
> SyncUserInterface (new class, would hold the display* functionality)
> instead of deriving from it?
>
>> >      * interactive password requests through D-Bus must continue to
>> >        work
>> >      * abort/suspend requests via D-Bus must work
>> >      * the forked process should shut down cleanly, so that "valgrind
>> >        --follow-child=yes --leak-check=yes" provides sane results (the
>> >        forked process in a local sync currently fails to do that,
>> >        because it is forked in a less controlled environment and
>> >        therefore just calls _exit())
>>
>> Using fork then exec should mostly relieve us of this issue.
>
> Agreed.
>
>> >      * logging:
>> >              * the main syncevo-dbus-server process should use syslog
>> >                logging for its own messages, without redirecting
>> >                stdout/stderr
>> >              * the forked process should start redirecting
>> >                stdout/stderr into the normal logging mechanism (so that
>> >                library error messages are visible when running the
>> >                syncevolution command line as client of the D-Bus server
>> >                and, later, they appear in log files) and switch to a
>> >                session log when the session is ready
>> >
>>
>> I plan to do this work in 3 steps:
>>
>> 1) The first step, which I've started, is to decouple the session and
>> server from each other and also modify objects that require both the
>> session and server objects as needed. Once this is done the server
>> should function as it does now and all tests should pass in order to
>> move on to step 2.
>> 2) Move session into separate binary using a one-to-one dbus
>> connection for communicating between the 2 processes. At this stage
>> only one session is run at a time and all tests should pass as before.
>
> Please try to keep this flexible. I'd like to use the separate binary
> not only for running sessions, but also for monitoring local and remote
> databases for changes. The main D-Bus server then would get a signal
> that a specific config requires a sync run and would kick of that
> operation when suitable.
>

When you say 'flexible' I'm assuming you are referring to both the
means by which a new process is spawned and how the communication
channel (peer-to-peer dbus connection) is set up. I'll try to wrap
this up in a way that you can easily make use of it.

>> 3) Enable running multiple concurrent sync sessions. At this stage
>> we'll probably need to add/modify some tests in order to deal with the
>> new functionality.
>
> Multiple concurrent sync sessions are a can of worms, so careful here.
> The main question is: which sync sessions are allowed to run in
> parallel?
>

Yes, I was wondering how such resource contention should be dealt with.

> For example, syncing against the same local database in parallel is
> asking for (unnecessary) trouble. If the instance controlling sync
> sessions wants to prevent that, how does it identify the "same
> database"? Backend "foo" + database "xxx" might access the same data as
> backend &quo

Re: [SyncEvolution] syncevo-dbus-server + forking processes

[Chris Kühl]

On Mon, Dec 5, 2011 at 2:49 PM, Patrick Ohly  wrote:
> On Mon, 2011-12-05 at 12:12 +0100, Chris Kühl wrote:
>> On Wed, Nov 16, 2011 at 12:22 PM, Patrick Ohly  
>> wrote:
>> >      * logging:
>> >              * the main syncevo-dbus-server process should use syslog
>> >                logging for its own messages, without redirecting
>> >                stdout/stderr
>> >              * the forked process should start redirecting
>> >                stdout/stderr into the normal logging mechanism (so that
>> >                library error messages are visible when running the
>> >                syncevolution command line as client of the D-Bus server
>> >                and, later, they appear in log files) and switch to a
>> >                session log when the session is ready
>> >
>>
>> I plan to do this work in 3 steps:
>>
>> 1) The first step, which I've started, is to decouple the session and
>> server from each other and also modify objects that require both the
>> session and server objects as needed. Once this is done the server
>> should function as it does now and all tests should pass in order to
>> move on to step 2.
>> 2) Move session into separate binary using a one-to-one dbus
>> connection for communicating between the 2 processes. At this stage
>> only one session is run at a time and all tests should pass as before.
>
> One more question: what effect should SIGINT and/or SIGTERM have on the
> syncevo-dbus-server?
>
> I think both should be treated as an urgent request to shut down
> operation. If a sync is running, it should be aborted and only after
> that has worked should the main syncevo-dbus-server process quit.

Yes, cleanly exiting the running sync sessions before the server exits
is essential.

I'll also be handling SIGCHILD in the server to make sure the server
can log a sync session ending unexpectedly and do any cleanup that's
needed.

The
> return code should be 0 in all cases, to distinguish from errors which
> force the daemon to quit.
>

Ok.

> This is slightly different from the current behavior where SIGINT
> requests a suspend and, if repeated quickly, only then forces an abort.
> Users should never have relied on that behavior in the
> syncevo-dbus-server and preserving it is simply not worth the extra
> complication.
>

Ok.

> It should only be preserved in the command line tool, where the user
> also gets immediate feedback on the first CTRL-C ("suspend requested,
> CTRL-C again to abort").
>

Ok, noted.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] syncevo-dbus-server + forking processes

[Chris Kühl]

On Tue, Dec 13, 2011 at 4:04 PM, Patrick Ohly  wrote:
> On Mo, 2011-12-05 at 12:12 +0100, Chris Kühl wrote:
>> I plan to do this work in 3 steps:
>>
>> 1) The first step, which I've started, is to decouple the session and
>> server from each other and also modify objects that require both the
>> session and server objects as needed. Once this is done the server
>> should function as it does now and all tests should pass in order to
>> move on to step 2.
>> 2) Move session into separate binary using a one-to-one dbus
>> connection for communicating between the 2 processes. At this stage
>> only one session is run at a time and all tests should pass as before.
>
> I hate to interrupt your ongoing work, but can we reverse the steps so
> that we first introduce the helper executable and a way to start it,
> including the direct D-Bus communication with it?
>

Ok, I'm looking into this. The original plan was that Step 1 was a
dependency of Step 2 but I've been reconsidering my approach. I'll
look into what you're asking and get back to you soon.

> I discovered end of last week that ActiveSync no longer works:
> apparently fork without exec leaves the forked process in a state where
> GIO gdbus is just blocking the caller without ever reaching activesyncd
> over D-Bus. That confirms that fork+exec is indeed needed.
>

If this is only happening with this combination, maybe it should be
disallowed for a very brief time until this is sorted.

> My plan is to use the helper binary as soon as possible for local sync.
> Thus the desire to have it up and running rather sooner than later,
> independent of refactoring the syncevo-dbus-server to use it.
>
> You mentioned catching SIGCHILD. Please don't do that. It has the
> disadvantage that there can be only one signal handler. Instead rely on
> the specific connection to a helper process to detect premature exits.
> My expectation is that this will be reported as a "peer has
> disconnected" error at the D-Bus level (for open calls) resp. signal
> (for watching the peer).
>

Yes, there is a closed signal for GDBusConnections.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] syncevo-dbus-server + forking processes

[Chris Kühl]

On Wed, Dec 14, 2011 at 2:26 PM, Patrick Ohly  wrote:
> On Mi, 2011-12-14 at 13:27 +0100, Chris Kühl wrote:
>> On Tue, Dec 13, 2011 at 4:04 PM, Patrick Ohly  wrote:
>> > On Mo, 2011-12-05 at 12:12 +0100, Chris Kühl wrote:
>> >> I plan to do this work in 3 steps:
>> >>
>> >> 1) The first step, which I've started, is to decouple the session and
>> >> server from each other and also modify objects that require both the
>> >> session and server objects as needed. Once this is done the server
>> >> should function as it does now and all tests should pass in order to
>> >> move on to step 2.
>> >> 2) Move session into separate binary using a one-to-one dbus
>> >> connection for communicating between the 2 processes. At this stage
>> >> only one session is run at a time and all tests should pass as before.
>> >
>> > I hate to interrupt your ongoing work, but can we reverse the steps so
>> > that we first introduce the helper executable and a way to start it,
>> > including the direct D-Bus communication with it?
>> >
>>
>> Ok, I'm looking into this. The original plan was that Step 1 was a
>> dependency of Step 2 but I've been reconsidering my approach. I'll
>> look into what you're asking and get back to you soon.
>
> One more thought about the various classes in the server and there
> inter-dependencies: would it make sense to introduce signals+slots as a
> way to connect the classes?
>
> Boost.Signals would be the obvious candidate.
>

Well, that wouldn't work for what i was trying to do. I was attempting
to move the entire session along with it's dbus interface into the
helper binary. This was not going very well as the coupling was too
great. I'm now looking at doing the fork  DBusTransportAgent  similar
to how it's done in LocalTransportAgent but with the exec of course.
This looks more likely to work out and makes the helper leaner.

>> > I discovered end of last week that ActiveSync no longer works:
>> > apparently fork without exec leaves the forked process in a state where
>> > GIO gdbus is just blocking the caller without ever reaching activesyncd
>> > over D-Bus. That confirms that fork+exec is indeed needed.
>> >
>>
>> If this is only happening with this combination, maybe it should be
>> disallowed for a very brief time until this is sorted.
>
> It's the usage of GIO gdbus in activesyncd which is causing that, not
> the GIO gdbus usage in SyncEvolution. activesyncd and its client
> libraries switched unconditionally, so now SyncEvolution must adapt.
>

Ah, ok.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] syncevo-dbus-server + forking processes

[Chris Kühl]

On Wed, Dec 14, 2011 at 4:25 PM, Patrick Ohly  wrote:
> On Mi, 2011-12-14 at 15:44 +0100, Chris Kühl wrote:
>> >> Ok, I'm looking into this. The original plan was that Step 1 was a
>> >> dependency of Step 2 but I've been reconsidering my approach. I'll
>> >> look into what you're asking and get back to you soon.
>> >
>> > I can also do that part myself, if you give me some pointers to
>> relevant
>> > technical documentation. I was looking for information on
>> bootstrapping
>> > a D-Bus connection over a pair of connected file descriptors,
>> without
>> > much luck.
>> >
>>
>> Well, in GIO GDBus you can create a connection using a GIOStream using
>> g_dbus_connection_new_sync(). Not sure if that would really work the
>> way you want, though.
>
> Looks promising.
>
> How about the same thing for libdbus?
>

I just looked but don't see anything. If you are referring to setting
up a peer-to-peer dbus connection, that's done using the (G)DBusServer
class. There is an example at [1].

Also, I intend to use g_spawn [2] for the fork and exec'ing. Should
offer enough control and make for less code to maintain.

>> I've no problem doing this, though.
>
> I don't care either way, as long as we can get it done soon, like this
> week ;-)
>

That might be a little tight. Tomorrow is a short day for me; Xmas
party at the Kindergarten my kids attend. So, maybe it's better you
tackle this if you need it that fast.

Cheers,
Chris

[1] http://developer.gnome.org/gio/2.28/GDBusServer.html#GDBusServer.signals
[2] http://developer.gnome.org/glib/2.28/glib-Spawning-Processes.html
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] syncevo-dbus-server + forking processes

[Chris Kühl]

On Fri, Dec 16, 2011 at 3:41 PM, Patrick Ohly  wrote:
> On Mi, 2011-12-14 at 16:52 +0100, Chris Kühl wrote:
>> I just looked but don't see anything. If you are referring to setting
>> up a peer-to-peer dbus connection, that's done using the (G)DBusServer
>> class. There is an example at [1].
>
> I have that same example working based on GDBus + libdbus. What I don't
> like about it is that the connection is based on listen() + connect().
> This makes it harder for the parent to detect that the child died: if
> the parent did the usual trick (create socket pair, fork+exec child,
> close its own side of the socket pair), then a closed socket would
> indicate that the child died. With listen() + connect() it has to check
> for existence of the child periodically. SIGCHILD does not integrate
> into the main loop well, but I see that g_spawn() has a GChildWatch -
> that should do.
>

Yes, looking at the source for the GChildWatch related code it blocks
SIGCHLD. When the mainloop's worker routine is run it checks if any of
the blocked signals are pending and then iterates over the
GChildWatchSources checking if the process has exited. So by checking
the return value of g_spawn_async_with_pipes and setting up the
GChildWatch I don't see a way to miss the childs exit, either.

> While working on this I got unhappy with some aspects of the current
> D-Bus C++ wrapper. The result is the "dbus-cleanup" branch. Chris, what
> do you think about that?

This looks fine. Moving the connection info to DBusObject definitely
makes sense and reduces the code a bit. And, yes, I should have made
better use of DBusConnectionPtr.

Note that your example code closed the
> connection while the rest of SyncEvolution didn't. This might be
> something that still needs to be fixed.
>

It seems that this is dependent on how the connection was created. Let
me take a look at this more closely.

> While testing that with GDBus GIO I noticed that I get segfaults when
> compiling on Debian Testing and --with-gio-dbus. That happens with and
> without my clean up patches - Chris, can you check that? If you cannot
> reproduce it, then please let me know and I will check it. It's a bit
> strange that I haven't seen that in the nightly testing.
>
> The signal callback seems to get an invalid std::string:
>

My testing involved running the test-dbus.py script. I guess this
didn't cover all cases. I'll look into this as well.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

[SyncEvolution] Session dbus api and documentation

[Chris Kühl]

Hi Patrick,

When looking into why the Session has a GetConfigs dbus method, I
noticed that there are several dbus methods which are not documented
in syncevo-session-full.xml. This includes Attach which is actually
mention but not properly documented in that file.

Is this just a case of the docs being out of sync or are some of these
methods not intended to be in the session interface?

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Session dbus api and documentation

[Chris Kühl]

On Mon, Dec 19, 2011 at 12:26 PM, Patrick Ohly  wrote:
> On Mo, 2011-12-19 at 11:02 +0100, Chris Kühl wrote:
>> When looking into why the Session has a GetConfigs dbus method, I
>> noticed that there are several dbus methods which are not documented
>> in syncevo-session-full.xml. This includes Attach which is actually
>> mention but not properly documented in that file.
>
> Good catch.

So what is the reason for GetConfigs in the DBus Session API? It seems
to be identical to Server.GetConfigs.

>
>> Is this just a case of the docs being out of sync or are some of these
>> methods not intended to be in the session interface?
>
> The docs are incomplete. In the patch which introduces the
> implementation, I added some comments to the documentation, but I missed
> completely that the actual documentation for Attach() and Detach() were
> missing.
>
> What is missing is something like:
>
>    
>      
>        
>          Prevents destruction of the session until the client
>          detaches or quits. Implemented with a counter, so each
>          Attach() must be matched by a Detach() to free the session.
>          Meant to be used by clients which want to follow the
>          progress of a session started by the daemon or some other
>          client. StartSession() automatically includes one Attach()
>          call, so typically the client which created a session never
>          calls Attach() and Detach() only once.
>        
>    
>
>    
>      
>        
>          Each Attach() or StartSession() must be matched by one
>          Detach() call, otherwise the session will not be
>          destructed. A client that quits without these calls will be
>          detached automatically.
>        
>      
>    
>
>
> For the record, here's the commit:
>

Ok, thanks.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Session dbus api and documentation

[Chris Kühl]

On Mon, Dec 19, 2011 at 3:24 PM, Patrick Ohly  wrote:
> On Mo, 2011-12-19 at 15:21 +0100, Chris Kühl wrote:
>> On Mon, Dec 19, 2011 at 12:26 PM, Patrick Ohly  
>> wrote:
>> > On Mo, 2011-12-19 at 11:02 +0100, Chris Kühl wrote:
>> >> When looking into why the Session has a GetConfigs dbus method, I
>> >> noticed that there are several dbus methods which are not documented
>> >> in syncevo-session-full.xml. This includes Attach which is actually
>> >> mention but not properly documented in that file.
>> >
>> > Good catch.
>>
>> So what is the reason for GetConfigs in the DBus Session API? It seems
>> to be identical to Server.GetConfigs.
>
> It's just there for the sake of completeness.
>

This DBus method invokes the getConfigs method in ReadOperations. In
turn, this calls Server::getDeviceList which requires BluezManager.
I'd rather not have to create a new BluezManager in the Session sync
process or cause the Session.GetConfigs to trigger a call to the
Server's GetConfigs method (or it's peer-to-peer dbus equivalent). If
it's, in fact, not really needed, is it ok if I drop this from the
Session dbus api ...at least for now?

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] syncevo-dbus-server + forking processes

[Chris Kühl]

On Mon, Dec 19, 2011 at 12:08 PM, Patrick Ohly  wrote:
> On Mo, 2011-12-19 at 10:57 +0100, Chris Kühl wrote:
>> On Fri, Dec 16, 2011 at 3:41 PM, Patrick Ohly  wrote:
>> > Note that your example code closed the
>> > connection while the rest of SyncEvolution didn't. This might be
>> > something that still needs to be fixed.
>> >
>>
>> It seems that this is dependent on how the connection was created. Let
>> me take a look at this more closely.
>
> Private connections need to be closed, shared must not be closed (?).

Yes this is what the libdbus docs tell us.

>
> Note that the libdbus based code uses private connections for
> everything. I think that this was done to avoid some interoperability
> issues with EDS using GIO - but memory is hazy on that. Going forward
> the old-style code should continue to use private connections while the
> new GIO code can be tested with shared connections.
>

I was wondering why these are all private. Now I've at least got a hint.

> So perhaps the parameter for that can be removed and made a static
> choice in the dbus_get_bus_connection() implementations?
>

I'm about half way through test-dbus.py and things are looking fine
using the singleton connection acquired by g_bus_get_sync(). so, if my
interpretation of what you're saying is correct, we can just remove
the unshared (btw, using a negative for a boolean value is confusing.)
option and make the gio gdbus always be a shared, singleton connection
and the libdbus connection always be private. Correct?

Cheers,
Chris

>> > While testing that with GDBus GIO I noticed that I get segfaults when
>> > compiling on Debian Testing and --with-gio-dbus. That happens with and
>> > without my clean up patches - Chris, can you check that? If you cannot
>> > reproduce it, then please let me know and I will check it. It's a bit
>> > strange that I haven't seen that in the nightly testing.
>> >
>> > The signal callback seems to get an invalid std::string:
>> >
>>
>> My testing involved running the test-dbus.py script. I guess this
>> didn't cover all cases. I'll look into this as well.
>
> Right, another test-dbus.py run passed fine this weekend.
>
> So let me be more precise: a "syncevolution --version" invocation
> segfaults.
>

Hmm. I get SyncEvolution 1.2.1+20111218+SE+3f4f71a (pre-release) when
running that on my Debian Testing install. But we have had different
results in the past on Deban Testing so I'm becoming less and less
surprised by this.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Session dbus api and documentation

[Chris Kühl]

On Mon, Dec 19, 2011 at 4:08 PM, Patrick Ohly  wrote:
> On Mo, 2011-12-19 at 15:49 +0100, Chris Kühl wrote:
>> On Mon, Dec 19, 2011 at 3:24 PM, Patrick Ohly  wrote:
>> > On Mo, 2011-12-19 at 15:21 +0100, Chris Kühl wrote:
>> >> On Mon, Dec 19, 2011 at 12:26 PM, Patrick Ohly  
>> >> wrote:
>> >> > On Mo, 2011-12-19 at 11:02 +0100, Chris Kühl wrote:
>> >> >> When looking into why the Session has a GetConfigs dbus method, I
>> >> >> noticed that there are several dbus methods which are not documented
>> >> >> in syncevo-session-full.xml. This includes Attach which is actually
>> >> >> mention but not properly documented in that file.
>> >> >
>> >> > Good catch.
>> >>
>> >> So what is the reason for GetConfigs in the DBus Session API? It seems
>> >> to be identical to Server.GetConfigs.
>> >
>> > It's just there for the sake of completeness.
>> >
>>
>> This DBus method invokes the getConfigs method in ReadOperations. In
>> turn, this calls Server::getDeviceList which requires BluezManager.
>> I'd rather not have to create a new BluezManager in the Session sync
>> process or cause the Session.GetConfigs to trigger a call to the
>> Server's GetConfigs method (or it's peer-to-peer dbus equivalent). If
>> it's, in fact, not really needed, is it ok if I drop this from the
>> Session dbus api ...at least for now?
>
> Oh, now I understand the original question. GetConfigs() is available
> for a session because it is always provided by ReadOperations, right? It
> was never documented as a session method. So in that sense it can be
> considered an implementation detail which can be removed.
>
> But why does the method have to be executed in the session process?
> Isn't the caller talking to the main process who needs to dispatch calls
> through to the background process? What I am aiming at is that the main
> process could execute this particular method as if it had been invoked
> on the server interface.
>

As mentioned in the first of 3 steps outlining my plans for this work,
I'm attempting to break the session apart from the server; decoupling
it completely. This is a pain and has caused me to consider only
running the sync in the separate process several times. However, I
feel it would be much cleaner (but definitely not easier) if the
session, along with its dbus api, lives in its own process. Ideally,
the dbus api shouldn't change for consumers of the api, but if
GetConfigs is not really needed or used then it'd make life easier to
nix it.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] syncevo-dbus-server + forking processes

[Chris Kühl]

On Mon, Dec 19, 2011 at 6:02 PM, Patrick Ohly  wrote:
> On Mo, 2011-12-19 at 16:47 +0100, Chris Kühl wrote:
>> On Mon, Dec 19, 2011 at 12:08 PM, Patrick Ohly  
>> wrote:
>> > So perhaps the parameter for that can be removed and made a static
>> > choice in the dbus_get_bus_connection() implementations?
>> >
>>
>> I'm about half way through test-dbus.py and things are looking fine
>> using the singleton connection acquired by g_bus_get_sync(). so, if my
>> interpretation of what you're saying is correct, we can just remove
>> the unshared (btw, using a negative for a boolean value is confusing.)
>> option and make the gio gdbus always be a shared, singleton connection
>> and the libdbus connection always be private. Correct?
>
> Right.
>
>> > So let me be more precise: a "syncevolution --version" invocation
>> > segfaults.
>> >
>>
>> Hmm. I get SyncEvolution 1.2.1+20111218+SE+3f4f71a (pre-release) when
>> running that on my Debian Testing install. But we have had different
>> results in the past on Deban Testing so I'm becoming less and less
>> surprised by this.
>
> Well, Testing is a moving target ;-) We might also be compiling
> differently, or there is a 32/64 bit difference (I'm using 64 bit).
>
> Anyway, as I am to reproduce it, finding the root cause wasn't that
> difficult:
>
> commit b7b17159d9e89c0452e3df99a35668a468e347d7
> Author: Patrick Ohly 
> Date:   Mon Dec 19 17:57:20 2011 +0100
>
>    GIO GDBus: fixed memory corruption
>
>    g_variant_get() does not work for C++ bool directly because at least
>    on x86_64, C++ bool uses 1 byte while gboolean uses 4. This caused
>    random stack corruption when unpacking four bytes into an address
>    which only had room for one. Caused "syncevolution --version" to segfault
>    when using a corrupted std::string later.
>
> diff --git a/src/gdbusxx/gdbus-cxx-bridge.h b/src/gdbusxx/gdbus-cxx-bridge.h
> index 106b216..568642b 100644
> --- a/src/gdbusxx/gdbus-cxx-bridge.h
> +++ b/src/gdbusxx/gdbus-cxx-bridge.h
> @@ -1170,12 +1170,37 @@ template<> struct dbus_traits :
>     static std::string getReply() { return ""; }
>  };
>
> -template<> struct dbus_traits :
> -    public basic_marshal< bool, VariantTypeBoolean >
> +template<> struct dbus_traits
> +// cannot use basic_marshal because VariantTypeBoolean packs/unpacks
> +// a gboolean, which is not a C++ bool (4 bytes vs 1 on x86_64)
> +// public basic_marshal< bool, VariantTypeBoolean >
>  {
>     static std::string getType() { return "b"; }
>     static std::string getSignature() {return getType(); }
>     static std::string getReply() { return ""; }
> +
> +    typedef bool host_type;
> +    typedef bool arg_type;
> +
> +    static void get(GDBusConnection *conn, GDBusMessage *msg,
> +                    GVariantIter &iter, bool &value)
> +    {
> +        GVariant *var = g_variant_iter_next_value(&iter);
> +        if (var == NULL || !g_variant_type_equal(g_variant_get_type(var), 
> VariantTypeBoolean::getVariantType())) {
> +            throw std::runtime_error("invalid argument");
> +        }
> +        gboolean buffer;
> +        g_variant_get(var, g_variant_get_type_string(var), &buffer);
> +        value = buffer;
> +        g_variant_unref(var);
> +    }
> +
> +    static void append(GVariantBuilder &builder, bool value)
> +    {
> +        const gchar *typeStr = 
> g_variant_type_dup_string(VariantTypeBoolean::getVariantType());
> +        g_variant_builder_add(&builder, typeStr, (gboolean)value);
> +        g_free((gpointer)typeStr);
> +    }
>  };
>

Oops, I recall removing that code.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] syncevo-dbus-server + forking processes

[Chris Kühl]

On Mon, Dec 19, 2011 at 6:02 PM, Patrick Ohly  wrote:
> On Mo, 2011-12-19 at 16:47 +0100, Chris Kühl wrote:
>> On Mon, Dec 19, 2011 at 12:08 PM, Patrick Ohly  
>> wrote:
>> > So perhaps the parameter for that can be removed and made a static
>> > choice in the dbus_get_bus_connection() implementations?
>> >
>>
>> I'm about half way through test-dbus.py and things are looking fine
>> using the singleton connection acquired by g_bus_get_sync(). so, if my
>> interpretation of what you're saying is correct, we can just remove
>> the unshared (btw, using a negative for a boolean value is confusing.)
>> option and make the gio gdbus always be a shared, singleton connection
>> and the libdbus connection always be private. Correct?
>
> Right.
>
>> > So let me be more precise: a "syncevolution --version" invocation
>> > segfaults.
>> >
>>
>> Hmm. I get SyncEvolution 1.2.1+20111218+SE+3f4f71a (pre-release) when
>> running that on my Debian Testing install. But we have had different
>> results in the past on Deban Testing so I'm becoming less and less
>> surprised by this.
>
> Well, Testing is a moving target ;-) We might also be compiling
> differently, or there is a 32/64 bit difference (I'm using 64 bit).
>
> Anyway, as I am to reproduce it, finding the root cause wasn't that
> difficult:
>
> commit b7b17159d9e89c0452e3df99a35668a468e347d7
> Author: Patrick Ohly 
> Date:   Mon Dec 19 17:57:20 2011 +0100
>
>    GIO GDBus: fixed memory corruption
>
>    g_variant_get() does not work for C++ bool directly because at least
>    on x86_64, C++ bool uses 1 byte while gboolean uses 4. This caused
>    random stack corruption when unpacking four bytes into an address
>    which only had room for one. Caused "syncevolution --version" to segfault
>    when using a corrupted std::string later.
>
> diff --git a/src/gdbusxx/gdbus-cxx-bridge.h b/src/gdbusxx/gdbus-cxx-bridge.h
> index 106b216..568642b 100644
> --- a/src/gdbusxx/gdbus-cxx-bridge.h
> +++ b/src/gdbusxx/gdbus-cxx-bridge.h
> @@ -1170,12 +1170,37 @@ template<> struct dbus_traits :
>     static std::string getReply() { return ""; }
>  };
>
> -template<> struct dbus_traits :

Should this not be the following?

template<> struct dbus_traits : public dbus_traits_base

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] syncevo-dbus-server + forking processes

[Chris Kühl]

On Tue, Dec 20, 2011 at 2:23 PM, Patrick Ohly  wrote:
> On Mo, 2011-12-19 at 10:57 +0100, Chris Kühl wrote:
>> On Fri, Dec 16, 2011 at 3:41 PM, Patrick Ohly  wrote:
>> > I have that same example working based on GDBus + libdbus. What I don't
>> > like about it is that the connection is based on listen() + connect().
>> > This makes it harder for the parent to detect that the child died: if
>> > the parent did the usual trick (create socket pair, fork+exec child,
>> > close its own side of the socket pair), then a closed socket would
>> > indicate that the child died. With listen() + connect() it has to check
>> > for existence of the child periodically. SIGCHILD does not integrate
>> > into the main loop well, but I see that g_spawn() has a GChildWatch -
>> > that should do.
>> >
>>
>> Yes, looking at the source for the GChildWatch related code it blocks
>> SIGCHLD. When the mainloop's worker routine is run it checks if any of
>> the blocked signals are pending and then iterates over the
>> GChildWatchSources checking if the process has exited. So by checking
>> the return value of g_spawn_async_with_pipes and setting up the
>> GChildWatch I don't see a way to miss the childs exit, either.
>
> Attached is a first API proposal for the fork/exec helper class and how
> it could be used in a small test program. Does that look sane?
>
> I haven't started implementing any of the new methods, but it should be
> fairly straightforward (famous last words).
>

Overall it looks like it makes sense. A couple questions though.

* Where would you be using g_child_watch here? I'm assuming it would
be set up internally in ForkExecParent and then trigger the OnQuit
signal.
* Should the forkexec argument set opt_fork_exec rather than opt_server.
* Is there a way to avoid passing in the MainLoop?

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] syncevo-dbus-server + forking processes

[Chris Kühl]

On Mon, Jan 9, 2012 at 4:54 PM, Patrick Ohly  wrote:
> On Mi, 2011-12-21 at 15:44 +0100, Patrick Ohly wrote:
>> On Mi, 2011-12-21 at 14:47 +0100, Patrick Ohly wrote:
>> > On Mi, 2011-12-21 at 14:17 +0100, Chris Kühl wrote:
>> > > Overall it looks like it makes sense.
>> >
>> > Good. I already went ahead and have a working implementation now, too.
>>
>> I pushed the complete implementation to the "fork-exec" branch. If (or
>> when ;-) you are working, please have a look at the changes. I'll not
>> rebase that branch anymore, feel free to use it as base of your work.
>
> More changes pushed to "fork-exec":
>
> commit 075003d907a94e821189591bf36bff2b52a0d9ca
> Author: Patrick Ohly 
> Date:   Mon Jan 9 14:40:34 2012 +0100
>
>    fork/exec: added state tracking
>
>    ForkExecParent is now keeping track of the current state:
>            IDLE,           /**< instance constructed, but start() not called 
> yet */
>            STARTING,       /**< start() called */
>            CONNECTED,      /**< child has connected, D-Bus connection 
> established */
>            TERMINATED      /**< child has quit */
>
>    Was needed for local sync and might be useful also elsewhere, thus
>    implemented in the core fork/exec code itself.
>

I've got a commit which adds a getChildPid() method to ForkExecParent
in my branch. I'm using this as a quick and dirty unique idea that the
server and child session share right now.

> commit bf293d0b10e60d3c269a41b4f2b51aea0c54943b
> Author: Patrick Ohly 
> Date:   Mon Jan 9 14:38:49 2012 +0100
>
>    fork/exec: implemented stop() and kill()
>
>    Sending the signals was missing.
>
>> In the meantime I'll start rewriting the local sync to use D-Bus as
>> communication instead of the current fork+pipes. The downside will be
>> that local sync starts to depend on glib and D-Bus, which wasn't the
>> case before. More a theoretic problem, I don't think anyone really
>> needed local sync without also having (and using) the other two.
>
> I pushed my initial local sync with fork+exec to the "fork-local-sync"
> branch.

For my part, I've been moving forward with attempting to place the
session and connection objects into the helper binary. The public dbus
interface still must reside in the server process in order to keep the
path and objects on the same name (org.syncevolution). So, most
session information is relayed from the helper process to the server
process via the p2p dbus connection.

My thinking is that this simplifies the server and would make it
easier to eventually get rid of the g_main_loop manipulation that's
prevalent now. I'm not 100% convinced this will work in the end. Do
you see any show-stopper to this approach?

>
> Chris, do you want me to take a stab at adapting GDBus GIO to the recent
> GDBus libdbus changes (local D-Bus connection, API
> changes/improvements)?
>

In order to avoid distraction, I was intending to do this only once
concurrent session is working with the libdbus implementation.
However, I can see that if you want to go ahead and merge you local
sync code into master that would need to be done first. So, feel free
to do that.

> We also need to finalize the support for launching helper binaries.
> Here's what I do at the moment:
>
> commit 25db81431a7ac5c63ff40548c4b77ee68cfa9c8a
> Author: Patrick Ohly 
> Date:   Mon Jan 9 16:36:28 2012 +0100
>
>    local sync: start 'syncevolution' as helper binary
>
>    Must be done manually with a symlink called 'syncevo-local-sync'.
>    A bit hackish at the moment and incomplete: how do we find the
>    right "syncevolution"? Do we actually install symlinks or copies
>    of the binary?
>
>    Advantage of using 'syncevolution' instead of linking a separate
>    helper: linking during compilation is slow.

On the other hand, if you make changes only in code that the helper
depends on then the server lib doesn't need to be recompiled.

When static linking,
>    the binaries become quite large (contain libsyncevolution).
>

To give this some numbers...

using --disable-shared --enable-static the binary sizes are:
-rwxrwxr-x.  1 chris chris  27M Jan  9 18:16 syncevo-dbus-helper
-rwxrwxr-x.  1 chris chris  32M Jan  9 18:16 syncevo-dbus-server

using --enable-shared --disable-static it's:
-rwxrwxr-x.  1 chris chris 8.7K Jan  9 18:28 syncevo-dbus-helper
-rwxrwxr-x.  1 chris chris 8.6K Jan  9 18:28 syncevo-dbus-server

>    Disadvantage of using 'syncevolution': the D-Bus client code in
>    syncevolution.cpp is dead code when running the helper. Minor issue?!
>

Yes, minor I'd say.

> What d

Re: [SyncEvolution] syncevo-dbus-server + forking processes

[Chris Kühl]

On Tue, Jan 10, 2012 at 8:31 AM, Patrick Ohly  wrote:
> On Mo, 2012-01-09 at 18:59 +0100, Chris Kühl wrote:
>> On Mon, Jan 9, 2012 at 4:54 PM, Patrick Ohly  wrote:
>> > commit bf293d0b10e60d3c269a41b4f2b51aea0c54943b
>> > Author: Patrick Ohly 
>> > Date:   Mon Jan 9 14:38:49 2012 +0100
>> >
>> >    fork/exec: implemented stop() and kill()
>> >
>> >    Sending the signals was missing.
>> >

I've run into an issue when trying to use signals between the child
and parent. When activating the signal it tries to use
dbus_bus_add_match which tries in turn to call AddMatch on the
org.fredesktop.DBus interface which is not available. I've modified
the dbus-client-connection.cpp example to expose this[1] and am
looking into fixing it.

I'm thinking this will not be an issue with the GIO GDBus wrapper.

[1] 
https://meego.gitorious.org/meego-middleware/syncevolution/commit/2b39a0410d5b8f362e3453ede17f980f494e0c10

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] syncevo-dbus-server + forking processes

[Chris Kühl]

On Wed, Jan 11, 2012 at 1:28 PM, Chris Kühl  wrote:
> On Tue, Jan 10, 2012 at 8:31 AM, Patrick Ohly  wrote:
>> On Mo, 2012-01-09 at 18:59 +0100, Chris Kühl wrote:
>>> On Mon, Jan 9, 2012 at 4:54 PM, Patrick Ohly  wrote:
>>> > commit bf293d0b10e60d3c269a41b4f2b51aea0c54943b
>>> > Author: Patrick Ohly 
>>> > Date:   Mon Jan 9 14:38:49 2012 +0100
>>> >
>>> >    fork/exec: implemented stop() and kill()
>>> >
>>> >    Sending the signals was missing.
>>> >
>
> I've run into an issue when trying to use signals between the child
> and parent. When activating the signal it tries to use
> dbus_bus_add_match which tries in turn to call AddMatch on the
> org.fredesktop.DBus interface which is not available. I've modified
> the dbus-client-connection.cpp example to expose this[1] and am
> looking into fixing it.
>
> I'm thinking this will not be an issue with the GIO GDBus wrapper.

This will very much be an issue with GIO GDBus as it uses the same
mechanism. Looking that the souce of libdbus and gdbus leads me to
believe using signals on a non-bus connection doesn't really make
sense. I just use method calls in this case.

Cheers,
Chris

>
> [1] 
> https://meego.gitorious.org/meego-middleware/syncevolution/commit/2b39a0410d5b8f362e3453ede17f980f494e0c10
>
> Cheers,
> Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] syncevo-dbus-server + forking processes

[Chris Kühl]

On Wed, Jan 11, 2012 at 3:08 PM, Patrick Ohly  wrote:
> On Mi, 2012-01-11 at 14:59 +0100, Chris Kühl wrote:
>> This will very much be an issue with GIO GDBus as it uses the same
>> mechanism. Looking that the souce of libdbus and gdbus leads me to
>> believe using signals on a non-bus connection doesn't really make
>> sense. I just use method calls in this case.
>
> Indeed, registering signal subscribers isn't very useful when there is
> always exactly one recipient.
>

I've got SignalWatch activation working with one-to-one connections.
Just have to forgo adding the match rule. See
https://meego.gitorious.org/meego-middleware/syncevolution/commit/7dc817671409e670dfa4819dcb23d8fb2cba5a6c

Cheers,
Chris

> Support for one-way method invocations (= don't care about return value)
> would be almost identical to such a signal. We don't support those at
> the moment, as far as I remember: the caller must use synchronous method
> calls (which wait for a result) or use asynchronous with callback. It
> could use a callback which doesn't do anything.
>
> --
> Best Regards, Patrick Ohly
>
> The content of this message is my personal opinion only and although
> I am an employee of Intel, the statements I make here in no way
> represent Intel's position on the issue, nor am I authorized to speak
> on behalf of Intel on this matter.
>
>
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] syncevo-dbus-server + forking processes

[Chris Kühl]

On Mon, Jan 16, 2012 at 9:50 AM, Patrick Ohly  wrote:
> On So, 2012-01-15 at 12:09 +0100, Chris Kühl wrote:
>> On Wed, Jan 11, 2012 at 3:08 PM, Patrick Ohly  wrote:
>> > On Mi, 2012-01-11 at 14:59 +0100, Chris Kühl wrote:
>> >> This will very much be an issue with GIO GDBus as it uses the same
>> >> mechanism. Looking that the souce of libdbus and gdbus leads me to
>> >> believe using signals on a non-bus connection doesn't really make
>> >> sense. I just use method calls in this case.
>> >
>> > Indeed, registering signal subscribers isn't very useful when there is
>> > always exactly one recipient.
>> >
>>
>> I've got SignalWatch activation working with one-to-one connections.
>> Just have to forgo adding the match rule.
>
> Is that also going to be possible with GDBus GIO? If not, then we will
> have a problem once code depends on signals and we want to switch to
> GDBus GIO.
>

I've not gone and implemented it yet, but I'd say most definitely. I
think it's as simple as not calling
g_dbus_connection_signal_subscribe.

All messages sent from the interface provider are received by the
interface consumer. The signal is simply a message of type
G_DBUS_MESSAGE_TYPE_SIGNAL which is checked in the isMatch method. So
things should work pretty easily.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] syncevo-dbus-server + forking processes

[Chris Kühl]

On Mon, Jan 16, 2012 at 10:17 AM, Chris Kühl  wrote:
> On Mon, Jan 16, 2012 at 9:50 AM, Patrick Ohly  wrote:
>> On So, 2012-01-15 at 12:09 +0100, Chris Kühl wrote:
>>> On Wed, Jan 11, 2012 at 3:08 PM, Patrick Ohly  
>>> wrote:
>>> > On Mi, 2012-01-11 at 14:59 +0100, Chris Kühl wrote:
>>> >> This will very much be an issue with GIO GDBus as it uses the same
>>> >> mechanism. Looking that the souce of libdbus and gdbus leads me to
>>> >> believe using signals on a non-bus connection doesn't really make
>>> >> sense. I just use method calls in this case.
>>> >
>>> > Indeed, registering signal subscribers isn't very useful when there is
>>> > always exactly one recipient.
>>> >
>>>
>>> I've got SignalWatch activation working with one-to-one connections.
>>> Just have to forgo adding the match rule.
>>
>> Is that also going to be possible with GDBus GIO? If not, then we will
>> have a problem once code depends on signals and we want to switch to
>> GDBus GIO.
>>
>
> I've not gone and implemented it yet, but I'd say most definitely. I
> think it's as simple as not calling
> g_dbus_connection_signal_subscribe.
>

Actually it's not quite that easy. You'll have to set up a filter and
fire the callback.  But it's still straight forward.

> All messages sent from the interface provider are received by the
> interface consumer. The signal is simply a message of type
> G_DBUS_MESSAGE_TYPE_SIGNAL which is checked in the isMatch method. So
> things should work pretty easily.
>
> Cheers,
> Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] syncevo-dbus-server + forking processes

[Chris Kühl]

On Tue, Jan 17, 2012 at 4:03 PM, Patrick Ohly  wrote:
> On Mo, 2012-01-16 at 17:19 +0100, Patrick Ohly wrote:
>> After also fixing the handling of asynchronous method
>> implementation in GDBus GIO, local sync works with it.
>
> After fixing some more regressions the D-Bus tests ran well
> (http://syncev.meego.com/2012-01-17-13-27_testing_gio-gdbus_dbus/), so I
> pushed all of the changes for gdbus C++ and fork/exec to the master
> branch.
>
> Chris, this is different from the previous fork/exec branch, so please
> do a "git rebase -i" and pick just your own changes when moving to the
> latest code.
>

Ok, I'll take a look after I've pushed the changes I'm working on now
to get the full one-to-one dbus interface implemented.

Btw, similar to the way the code in syncevolution.cpp makes a dbus
method call then runs a mainloop till the call back has returned, I
had to create such a hack[1] but slightly differently. I've
implemented this by polling the dbusconnection's fd and on any fd
activity, do a context iteration, then seeing if the callback has
finished. Like I said it's a hack and I'll see how much mileage this
gets me but I'm not convinced it's any worse than what's being done in
syncevolution.cpp. Feel free to convince me, though. ;)

Cheers,
Chris

[1] 
https://meego.gitorious.org/meego-middleware/syncevolution/commit/7623d38b047c86b68b1481db7c8620d9352e5d72
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] syncevo-dbus-server + forking processes

[Chris Kühl]

On Tue, Jan 17, 2012 at 5:18 PM, Chris Kühl  wrote:
> On Tue, Jan 17, 2012 at 4:03 PM, Patrick Ohly  wrote:
>> On Mo, 2012-01-16 at 17:19 +0100, Patrick Ohly wrote:
>>> After also fixing the handling of asynchronous method
>>> implementation in GDBus GIO, local sync works with it.
>>
>> After fixing some more regressions the D-Bus tests ran well
>> (http://syncev.meego.com/2012-01-17-13-27_testing_gio-gdbus_dbus/), so I
>> pushed all of the changes for gdbus C++ and fork/exec to the master
>> branch.
>>
>> Chris, this is different from the previous fork/exec branch, so please
>> do a "git rebase -i" and pick just your own changes when moving to the
>> latest code.
>>
>
> Ok, I'll take a look after I've pushed the changes I'm working on now
> to get the full one-to-one dbus interface implemented.

I've renamed my branch to concurrent-sync-sessions and rebased onto
master. I'm now going through and making required changes to get tests
to work.

Cheers,
Chris

>
> Btw, similar to the way the code in syncevolution.cpp makes a dbus
> method call then runs a mainloop till the call back has returned, I
> had to create such a hack[1] but slightly differently. I've
> implemented this by polling the dbusconnection's fd and on any fd
> activity, do a context iteration, then seeing if the callback has
> finished. Like I said it's a hack and I'll see how much mileage this
> gets me but I'm not convinced it's any worse than what's being done in
> syncevolution.cpp. Feel free to convince me, though. ;)
>
> Cheers,
> Chris
>
> [1] 
> https://meego.gitorious.org/meego-middleware/syncevolution/commit/7623d38b047c86b68b1481db7c8620d9352e5d72
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] syncevo-dbus-server + forking processes + signal handling

[Chris Kühl]

On Thu, Jan 19, 2012 at 4:19 PM, Patrick Ohly  wrote:
> On Mi, 2012-01-18 at 16:55 +0100, Chris Kühl wrote:
>> I've renamed my branch to concurrent-sync-sessions and rebased onto
>> master. I'm now going through and making required changes to get tests
>> to work.
>
> Note that I pushed another change onto a new "signal-handling" branch.
> This affects you because syncevo-dbus-server will have to relay
> suspend/abort requests. The helper process will have to deal with
> signals similar to syncevo-local-sync helper in that branch.
>
> Do these changes make sense?
>

Yes, this looks fine.

btw, I noticed when looking through this the the copyright dates for
SuspendFlags.cpp are old.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] syncevo-dbus-server + forking processes + signal handling

[Chris Kühl]

On Sat, Jan 21, 2012 at 9:23 PM, Patrick Ohly  wrote:
> On Fr, 2012-01-20 at 21:21 +0100, Patrick Ohly wrote:
>> On Fr, 2012-01-20 at 10:53 +0100, Chris Kühl wrote:
>> > On Thu, Jan 19, 2012 at 4:19 PM, Patrick Ohly  
>> > wrote:
>> > > On Mi, 2012-01-18 at 16:55 +0100, Chris Kühl wrote:
>> > >> I've renamed my branch to concurrent-sync-sessions and rebased onto
>> > >> master. I'm now going through and making required changes to get tests
>> > >> to work.
>> > >
>> > > Note that I pushed another change onto a new "signal-handling" branch.
>> > > This affects you because syncevo-dbus-server will have to relay
>> > > suspend/abort requests. The helper process will have to deal with
>> > > signals similar to syncevo-local-sync helper in that branch.
>> > >
>> > > Do these changes make sense?
>> > >
>> >
>> > Yes, this looks fine.
>>
>> They are on their way towards master. Running one more test now.
>>
>> After looking into what it would take to have all backends react to
>> abort requests in a timely manner I have come to the conclusion that it
>> will be much simpler to just let the process running them die when
>> receiving a signal. Teaching libs with a synchronous API, like libneon
>> and the activesyncd client libraries, how to watch additional events
>> will be hard.
>>
>> For syncevo-local-sync I have implemented that and intend to merge it
>> soon, see for-master/signal-handling branch.
>>
>> Another patch modifies the syncevo-dbus-server. It's a step towards
>> gettting rid of SyncContext::checkForAbort/Suspend(). This is not
>> absolutely necessary at the moment and probably conflicts with your
>> server rewrite, so I intend to keep it on a branch for now.
>
> After looking at the only regression reported by testing for the
> for-master/fork-local-sync branch it turned out that this final patch
> *is* needed: when not applied, TestLocalSync.testConcurrency fails
> because the Session.Abort() call has no effect while the local transport
> still waits for the child to start. session.cpp sets the SYNC_ABORT
> status and quits the main loop with the goal getting the SyncContext to
> react to the abort. Instead the fork/exec local transport restarts the
> loop and the sync session starts normally.
>
> This is partly a failure in the local transport (it would have to do a
> checkForAbort()), partly in the session (as mentioned in the commit
> message for my latest commit, it allowed a session to go from "aborted"
> to "running").
>
> Either way, with the latest commit applied, the transport gets canceled
> as soon as Abort() is called and the session aborts as intended.
>
> Chris, I hope I'm not disrupting your work too much by merging the
> entire "signal-handling" into master.
>
>

No please go ahead and merge. This fixes I was seeing on the horizon
anyway. I'd also rather have any fixes and changes you've made early
rather than later.

Also, I was reminded by the TestDBusServerTerm.testTermConnection test
that I still needed to implement the Connection interface in the
helper. I've pushed some code to that end although I'm still working
out some kinks in the dbus communication between the server and helper
at the moment.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] syncevo-dbus-server + forking processes + signal handling

[Chris Kühl]

On Mon, Jan 23, 2012 at 12:55 PM, Chris Kühl  wrote:
> On Sat, Jan 21, 2012 at 9:23 PM, Patrick Ohly  wrote:
>> On Fr, 2012-01-20 at 21:21 +0100, Patrick Ohly wrote:
>>> On Fr, 2012-01-20 at 10:53 +0100, Chris Kühl wrote:
>>> > On Thu, Jan 19, 2012 at 4:19 PM, Patrick Ohly  
>>> > wrote:
>>> > > On Mi, 2012-01-18 at 16:55 +0100, Chris Kühl wrote:
>>> > >> I've renamed my branch to concurrent-sync-sessions and rebased onto
>>> > >> master. I'm now going through and making required changes to get tests
>>> > >> to work.
>>> > >
>>> > > Note that I pushed another change onto a new "signal-handling" branch.
>>> > > This affects you because syncevo-dbus-server will have to relay
>>> > > suspend/abort requests. The helper process will have to deal with
>>> > > signals similar to syncevo-local-sync helper in that branch.
>>> > >
>>> > > Do these changes make sense?
>>> > >
>>> >
>>> > Yes, this looks fine.
>>>
>>> They are on their way towards master. Running one more test now.
>>>
>>> After looking into what it would take to have all backends react to
>>> abort requests in a timely manner I have come to the conclusion that it
>>> will be much simpler to just let the process running them die when
>>> receiving a signal. Teaching libs with a synchronous API, like libneon
>>> and the activesyncd client libraries, how to watch additional events
>>> will be hard.
>>>
>>> For syncevo-local-sync I have implemented that and intend to merge it
>>> soon, see for-master/signal-handling branch.
>>>
>>> Another patch modifies the syncevo-dbus-server. It's a step towards
>>> gettting rid of SyncContext::checkForAbort/Suspend(). This is not
>>> absolutely necessary at the moment and probably conflicts with your
>>> server rewrite, so I intend to keep it on a branch for now.
>>
>> After looking at the only regression reported by testing for the
>> for-master/fork-local-sync branch it turned out that this final patch
>> *is* needed: when not applied, TestLocalSync.testConcurrency fails
>> because the Session.Abort() call has no effect while the local transport
>> still waits for the child to start. session.cpp sets the SYNC_ABORT
>> status and quits the main loop with the goal getting the SyncContext to
>> react to the abort. Instead the fork/exec local transport restarts the
>> loop and the sync session starts normally.
>>
>> This is partly a failure in the local transport (it would have to do a
>> checkForAbort()), partly in the session (as mentioned in the commit
>> message for my latest commit, it allowed a session to go from "aborted"
>> to "running").
>>
>> Either way, with the latest commit applied, the transport gets canceled
>> as soon as Abort() is called and the session aborts as intended.
>>
>> Chris, I hope I'm not disrupting your work too much by merging the
>> entire "signal-handling" into master.
>>
>>
>
> No please go ahead and merge. This fixes I was seeing on the horizon
> anyway. I'd also rather have any fixes and changes you've made early
> rather than later.
>
> Also, I was reminded by the TestDBusServerTerm.testTermConnection test
> that I still needed to implement the Connection interface in the
> helper. I've pushed some code to that end although I'm still working
> out some kinks in the dbus communication between the server and helper
> at the moment.
>

This was done yesterday (or 2 days ago now). It my be good to give a
little more detailed update at this point.

The last couple days I've been fixing various issues when running
test-dbus.py. Quickly I found that there was an issue in how the
session's dbus interface was being initialized. The interface was not
immediately available after Session.StartSessionWithFlags returned.
This was because the interface was not being made available until the
onConnect callback was invoked. I've now changed this to spawn the
helper then wait for the helper to indicate that its DBus interface is
ready to be used. Included in this change is a move of all the
ForkExec code in Session and Connection out of the constructors and
into an init() method.

In order to provide the helper binary with additional information I've
created an addEnvVar(name, value) method to the ForkExecParent which
obviously adds an environment variable. I'm using this to indicate
whether the helper should start the

Re: [SyncEvolution] syncevo-dbus-server + forking processes + signal handling

[Chris Kühl]

On Wed, Jan 25, 2012 at 11:11 AM, Patrick Ohly  wrote:
> On Mi, 2012-01-25 at 02:32 +0100, Chris Kühl wrote:
>> This was done yesterday (or 2 days ago now). It my be good to give a
>> little more detailed update at this point.
>
> Thanks, that's very useful.
>
>> With the above changes, the initialization of and communication with
>> the helper binary seems to work well. I'm continuing to work my way
>> through the tests. Right now I'm dealing with getting autosync to work
>> properly. This is requiring some additional communication between the
>> helper and server binaries as well as reworking some code I changed
>> when removing the priority queues in the server.
>
> I wonder whether we should just leave it broken for now. I find the
> whole autosync implementation unintuitive and already planned to rewrite
> it from scratch when adding "sync when changes are detected". The
> problem is that I don't know yet when I will have time for that. If I
> don't find the time soon, it would block merging your work because
> regressions in the master branch are not acceptable.
>

Ok, I'll keep that in mind. Looking at the AutoSync stuff is leading
me to fix other related issues (shutdown) so I'll see how far this
takes me.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

[SyncEvolution] DBus server rework: sync, shutdown, and password request

[Chris Kühl]

Hi Patrick,

This week I've been going down the path to get the password request
working. However, to get the session to even request a password we've
got to start a session which is one of the things I've been working
on.

One question I've got is when do we go about activating the sessions.
I'm assuming this can be done immediately. The approach I'm taking now
is to set the session 'active' and emit a status signal as soon as the
helper session's up and ready. Thus, the server creates a session and
it runs immediately.

I've also needed to look at how shutdown is handled in the course of
this. From what I can tell there are 2 events that can cause a
shutdown request: a signal or a file change. Because we are now
dealing with the ideal case that we have no queued sessions & multiple
sessions are running concurrently, it doesn't seem to me that we need
a ShutdownSession any longer. The shutdown session seemed to be used
to get to the head of the queue and prevent any new sessions from
starting. I'm going about removing that and just using the
m_shutdownRequested flag to disallow starting any additional sessions
etc.

Now for the actual infoRequest I'm doing the following. When the
request is made the helper emits a password request signal and waits.
The helper only does one job, so this is fine. The SessionResource in
the server receives this signal and creates an InfoRequest. Now we
can't wait in the server process because we've got possibly multiple
concurrent sessions running. So, I've introduced signal in InfoReq
that lets us know when the response is received. This signal is
connected to a routine in the SessionResource which passes the result
to the session helper who can now carry on its business.

I welcome any input as to whether these are all sound assumptions.

This code will land very soon.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] DBus server rework: sync, shutdown, and password request

[Chris Kühl]

On Sat, Feb 4, 2012 at 2:45 PM, Patrick Ohly  wrote:
> On Sat, 2012-02-04 at 12:32 +0100, Chris Kühl wrote:
>> One question I've got is when do we go about activating the sessions.
>> I'm assuming this can be done immediately. The approach I'm taking now
>> is to set the session 'active' and emit a status signal as soon as the
>> helper session's up and ready. Thus, the server creates a session and
>> it runs immediately.
>
> The 'active' state indicates that the session is allowed to be used for
> operations. It doesn't guarantee that the session will react
> immediately. So you could set it to active in the syncevo-dbus-server
> side immediately, without waiting for the helper side to be ready. Is
> that perhaps simpler?
>

Yes, as soon as it's add/queued it'll be marked as active in the server process.

>> I've also needed to look at how shutdown is handled in the course of
>> this. From what I can tell there are 2 events that can cause a
>> shutdown request: a signal or a file change. Because we are now
>> dealing with the ideal case that we have no queued sessions & multiple
>> sessions are running concurrently,
>
> You cannot guarantee that they are no queued sessions. A new session
> might conflict with a running one and thus cannot start immediately.
>

Yeah, unfortunately things are not ideal. Have you given any further
thought as to how we are going to properly detect conflicting sync
sessions? This points getting close that this is going to need to be
dealt with. I've not looked into this at length yet. Is there no
interface I could use to test that syncs will not conflict?

>>  it doesn't seem to me that we need
>> a ShutdownSession any longer. The shutdown session seemed to be used
>> to get to the head of the queue and prevent any new sessions from
>> starting. I'm going about removing that and just using the
>> m_shutdownRequested flag to disallow starting any additional sessions
>> etc.
>
> Do whatever works :-) The goal has to be that queued sessions are
> discarded as soon as possible instead of delaying the shutdown.
>
> One open question is whether the syncevo-dbus-server should be allowed
> to quit while there is a running sync. In the past it wasn't, because
> *itself* was running the sync. Now I think the sync should continue to
> run unless it was canceled explicitly itself. For example, if the
> syncevo-dbus-server crashes for some reason, it would be nice to
> complete the running sync instead of aborting it prematurely.
>

Ok, I've gone the route of ripping the shutdownSession out and
enabling the sync to run even when the server shuts down. This
actually results in a decent amount of code being removed in my
yet-to-be-tested commit.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] DBus server rework: sync, shutdown, and password request

[Chris Kühl]

On Tue, Feb 7, 2012 at 9:23 AM, Patrick Ohly  wrote:
> On Tue, 2012-02-07 at 09:04 +0100, Murray Cumming wrote:
>> On Tue, 2012-02-07 at 03:35 +0100, Chris Kühl wrote:
>> > Yeah, unfortunately things are not ideal. Have you given any further
>> > thought as to how we are going to properly detect conflicting sync
>> > sessions? This points getting close that this is going to need to be
>> > dealt with. I've not looked into this at length yet. Is there no
>> > interface I could use to test that syncs will not conflict?
>>
>> Nevertheless, it would be nice to punt this to a later patch/branch,
>> just so we have some chance to get the current work into master.
>
> I agree. Let's keep the current behavior (only one session can run at a
> time) and figure out how to detect non-conflicting sessions later. Don't
> bite off more than you can swallow :-)
>

Ok, I'll reimplement the queue then.

I was thinking that with an interface in place I could actually have
the code in-place that would do the concurrent sessions even if now it
would always return that there is a conflict. For testing that
concurrent sessions do actually work with known, non-conflicting
sources, we could introduce an environment variable that would return
that there is no conflict. That way, as soon a solution is found ofr
finding conflicts, is should just work.

Patrick, you were also mentioning that you'd like to rework the
autosync mechanism. Shall I strip out that functionality from the
server?

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] DBus server rework: sync, shutdown, and password request

[Chris Kühl]

On Tue, Feb 7, 2012 at 8:19 PM, Patrick Ohly  wrote:
> On Tue, 2012-02-07 at 12:44 +0100, Chris Kühl wrote:
>> On Tue, Feb 7, 2012 at 9:23 AM, Patrick Ohly  wrote:
>> > On Tue, 2012-02-07 at 09:04 +0100, Murray Cumming wrote:
>> >> On Tue, 2012-02-07 at 03:35 +0100, Chris Kühl wrote:
>> >> > Yeah, unfortunately things are not ideal. Have you given any further
>> >> > thought as to how we are going to properly detect conflicting sync
>> >> > sessions? This points getting close that this is going to need to be
>> >> > dealt with. I've not looked into this at length yet. Is there no
>> >> > interface I could use to test that syncs will not conflict?
>> >>
>> >> Nevertheless, it would be nice to punt this to a later patch/branch,
>> >> just so we have some chance to get the current work into master.
>> >
>> > I agree. Let's keep the current behavior (only one session can run at a
>> > time) and figure out how to detect non-conflicting sessions later. Don't
>> > bite off more than you can swallow :-)
>> >
>>
>> Ok, I'll reimplement the queue then.
>>
>> I was thinking that with an interface in place I could actually have
>> the code in-place that would do the concurrent sessions even if now it
>> would always return that there is a conflict. For testing that
>> concurrent sessions do actually work with known, non-conflicting
>> sources, we could introduce an environment variable that would return
>> that there is no conflict. That way, as soon a solution is found ofr
>> finding conflicts, is should just work.
>
> That makes sense.
>

Ok, should have this set up today.

>> Patrick, you were also mentioning that you'd like to rework the
>> autosync mechanism. Shall I strip out that functionality from the
>> server?
>
> Please ifdef out or comment any code which no longer compiles, but keep
> it in place. I'll have a look this week.
>

Compiles fine so far. Just seemed like you weren't really enthusiastic
about it and it seems that 2 of the 3 scenarios where it's designed to
be used are not implemented. I'll leave it as is.

> The core decision making still belongs into the main
> syncevo-dbus-server. That's the right place to track when sessions ran
> and are meant to run again.
>

Agreed, and everything should be that way now. The helper process just
waits for commands, syncs and sends status and progress signals to the
server process.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] DBus server rework: sync, shutdown, and password request

[Chris Kühl]

On Wed, Feb 8, 2012 at 10:08 AM, Chris Kühl  wrote:
> On Tue, Feb 7, 2012 at 8:19 PM, Patrick Ohly  wrote:
>> On Tue, 2012-02-07 at 12:44 +0100, Chris Kühl wrote:
>>> On Tue, Feb 7, 2012 at 9:23 AM, Patrick Ohly  wrote:
>>> > On Tue, 2012-02-07 at 09:04 +0100, Murray Cumming wrote:
>>> >> On Tue, 2012-02-07 at 03:35 +0100, Chris Kühl wrote:
>>> >> > Yeah, unfortunately things are not ideal. Have you given any further
>>> >> > thought as to how we are going to properly detect conflicting sync
>>> >> > sessions? This points getting close that this is going to need to be
>>> >> > dealt with. I've not looked into this at length yet. Is there no
>>> >> > interface I could use to test that syncs will not conflict?
>>> >>
>>> >> Nevertheless, it would be nice to punt this to a later patch/branch,
>>> >> just so we have some chance to get the current work into master.
>>> >
>>> > I agree. Let's keep the current behavior (only one session can run at a
>>> > time) and figure out how to detect non-conflicting sessions later. Don't
>>> > bite off more than you can swallow :-)
>>> >
>>>
>>> Ok, I'll reimplement the queue then.
>>>
>>> I was thinking that with an interface in place I could actually have
>>> the code in-place that would do the concurrent sessions even if now it
>>> would always return that there is a conflict. For testing that
>>> concurrent sessions do actually work with known, non-conflicting
>>> sources, we could introduce an environment variable that would return
>>> that there is no conflict. That way, as soon a solution is found ofr
>>> finding conflicts, is should just work.
>>
>> That makes sense.
>>
>
> Ok, should have this set up today.
>

So , I've pushed so changes taking us in this direction. Here is the
basic run down of what these changes include.

- We've now got a priority queue of weak pointers implemented with a
std::set container that is initialized with a Compare class for
inserting new Resources based on their priority.
- There is a method (canRunConcurrently()) in Resource which is
checked but currently always returns false, thus forcing only single
session to be run.
- Once the Resource is made active it's placed the list of active
resources, a list of shared pointers to Resources.
- Implemented some of the outstanding issues with Connection (missing
communication between server and helper processes)
- Allow server process to set helper session's active state.
- Krzesimir implemented the missing functionality in the gio-gdbus
wrapper and added code to do blocking calls with minimal extra code
required in consuming classes.

Note that in the above I always refer to Resources instead of Session
or Connection. Previously the Connection would create a session and
place it in the working queue but not activate the public dbus
interface. Because the Connection and the Session it spawns needs to
be in the same process. Thus the Connection is also queued but has a
higher priority than the other resources.

The above mentioned changes compile and are being tested now. I'll get
back to you with the results of that.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] DBus server rework: sync, shutdown, and password request

[Chris Kühl]

On Thu, Feb 16, 2012 at 2:29 PM, Patrick Ohly  wrote:
> On Mon, 2012-02-13 at 14:50 +0100, Chris Kühl wrote:
>> So , I've pushed so changes taking us in this direction. Here is the
>> basic run down of what these changes include.
>>
>> - We've now got a priority queue of weak pointers implemented with a
>> std::set container that is initialized with a Compare class for
>> inserting new Resources based on their priority.
>> - There is a method (canRunConcurrently()) in Resource which is
>> checked but currently always returns false, thus forcing only single
>> session to be run.
>> - Once the Resource is made active it's placed the list of active
>> resources, a list of shared pointers to Resources.
>> - Implemented some of the outstanding issues with Connection (missing
>> communication between server and helper processes)
>> - Allow server process to set helper session's active state.
>> - Krzesimir implemented the missing functionality in the gio-gdbus
>> wrapper and added code to do blocking calls with minimal extra code
>> required in consuming classes.
>>
>> Note that in the above I always refer to Resources instead of Session
>> or Connection. Previously the Connection would create a session and
>> place it in the working queue but not activate the public dbus
>> interface. Because the Connection and the Session it spawns needs to
>> be in the same process. Thus the Connection is also queued but has a
>> higher priority than the other resources.
>>
>> The above mentioned changes compile and are being tested now. I'll get
>> back to you with the results of that.
>
> How is that going?
>
> I'm now looking more seriously at the code myself.

I wouldn't take too close a look at it just yet, I'm still working on
getting some fundamental pieces working, like the main loop dance in
the helper.

> One thing which
> caught my attention is how blocking D-Bus calls are used in
> syncevo-dbus-server.
>
> For example, ConnectionResource::process(): it passes the incoming data
> on to the helper process with a blocking call. Isn't that a big no-no?
>

Yes, in this instance the client is expecting the the Reply signal so
we can definitely do without this blocking call. This is actually how
I wish the entire DBus interface worked; make a request and wait for a
signal that delivers the requested data.

However, blocking calls are necessary in the cases where the client
expects a return value when complete or an error message on failure.

> If the helper is busy, won't that D-Bus call completely block all
> processing of any other events in the server for considerable periods of
> time? Worse, if syncevo-dbus-server is blocked in a call to the helper
> and the helper needs something from syncevo-dbus-server (like a
> password), then we have a deadlock.

True, this a problem and would require breaking the DBus interface to
work. The only way I know how to get around this is the make the
public DBus interface asynchronous.

>
> Even if other events do get processed while in a blocking D-Bus call,
> the result still won't be very pretty: basically we end up with a
> convoluted mixture of recursive calls, instead of a clean "process
> events, do something non-blocking, return to event loop" style.
>

I'll be happy to rework the public DBus interface to get this to work
like this. This, of course, will require that all the clients learn
this new interface and, maybe more painfully, the test suite will need
to be extensively reworked. But it would definitely make the
dbus-server cleaner and some things possible at all.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] DBus server rework: sync, shutdown, and password request

[Chris Kühl]

On Thu, Feb 16, 2012 at 4:19 PM, Patrick Ohly  wrote:
> On Thu, 2012-02-16 at 15:32 +0100, Chris Kühl wrote:
>> > One thing which
>> > caught my attention is how blocking D-Bus calls are used in
>> > syncevo-dbus-server.
>> >
>> > For example, ConnectionResource::process(): it passes the incoming data
>> > on to the helper process with a blocking call. Isn't that a big no-no?
>> >
>>
>> Yes, in this instance the client is expecting the the Reply signal so
>> we can definitely do without this blocking call. This is actually how
>> I wish the entire DBus interface worked; make a request and wait for a
>> signal that delivers the requested data.
>
> But that's already how D-Bus works: send a method call method, get a
> reply message back. There's no need to define the D-Bus interface as
> "method call without a reply + signal".
>
>> > If the helper is busy, won't that D-Bus call completely block all
>> > processing of any other events in the server for considerable periods of
>> > time? Worse, if syncevo-dbus-server is blocked in a call to the helper
>> > and the helper needs something from syncevo-dbus-server (like a
>> > password), then we have a deadlock.
>>
>> True, this a problem and would require breaking the DBus interface to
>> work. The only way I know how to get around this is the make the
>> public DBus interface asynchronous.
>
> No, just make the implementation of the D-Bus interface asynchronous.
> The C++ bindings support that:
>
>  * Asynchronous methods are possible by declaring one parameter as a
>  * Result pointer and later calling the virtual function provided by
>  * it. Parameter passing of results is less flexible than that of
>  * method parameters: the later allows both std::string as well as
>  * const std::string &, for results only the const reference is
>  * supported. The Result instance is passed as pointer and then owned
>  * by the called method.
>
> Have a look at Result1 in src/gdbusxx/test/example.cpp.
>

Ah, that's what I needed. I didn't recall that this was possible
without modifying the DBus wrapper considerably. Looks similar to the
GDbusMethodInvocation functionality.

> The flow of messages then becomes:
>     1. syncevo-dbus-server receives method call, stores Result pointer
>     2. syncevo-dbus-server sends method call to helper
>     3. helper receives method call, replies (asynchronously or
>        synchronously)
>     4. syncevo-dbus-server receives reply
>     5. syncevo-dbus-server relays reply to original caller
>
> I'll see whether I can cook up a patch.
>

No need. I'm now looking at how you use it in LocalTransportAgent.cpp.
That should be enough to get started implementing this.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] DBus server rework: sync, shutdown, and password request

[Chris Kühl]

On Thu, Feb 16, 2012 at 6:15 PM, Patrick Ohly  wrote:
> On Thu, 2012-02-16 at 16:47 +0100, Chris Kühl wrote:
>> On Thu, Feb 16, 2012 at 4:19 PM, Patrick Ohly  wrote:
>> > The flow of messages then becomes:
>> >     1. syncevo-dbus-server receives method call, stores Result pointer
>> >     2. syncevo-dbus-server sends method call to helper
>> >     3. helper receives method call, replies (asynchronously or
>> >        synchronously)
>> >     4. syncevo-dbus-server receives reply
>> >     5. syncevo-dbus-server relays reply to original caller
>> >
>> > I'll see whether I can cook up a patch.
>> >
>>
>> No need. I'm now looking at how you use it in LocalTransportAgent.cpp.
>> That should be enough to get started implementing this.
>
> I've already finished an initial draft of the idea. Here's the commit
> (applies on top of the code which uses the blocking call operator). Does
> this look right?
>

Btw, Krzesimir is working on getting these changes integrated.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] DBus server rework: syncevo-dbus-server <-> helper communication

[Chris Kühl]

2012/2/22 Patrick Ohly :
> On Thu, 2012-02-16 at 15:32 +0100, Chris Kühl wrote:
>> > How is that going?
>> >
>> > I'm now looking more seriously at the code myself.
>>
>> I wouldn't take too close a look at it just yet, I'm still working on
>> getting some fundamental pieces working, like the main loop dance in
>> the helper.
>
> This is based on an older statement of mine:
> "Anything that runs a sync session (i.e. the helper) will have to
> continue doing the g_main_loop dance or become multithreaded, because
> the libsynthesis APIs are not asynchronous."
>
> As we've seen, for example in the simplified command line client of the
> D-Bus server, introducing the blocking method calls considerably
> simplified code which is allowed to block.
>

Can you point me to this commit? I've not been able to track it down.
Knowing the code, I can imagine the changes that would be made,
though.

> By definition, anything in a sync session is allowed to block. There are
> downsides, the main one being stuck in a long-running operation while
> the session is already meant to be aborted, in which case that operation
> should also be aborted.
>
> The recently introduced SuspendFlags class is meant to help with that.
>
> Another downside is that requests from syncevo-dbus-server to the helper
> via D-Bus will not be processed unless the helper runs the main loop at
> least occasionally. Are there any such requests?
>
> So what I am asking now is basically: can we use blocking method calls
> from helper to syncevo-dbus-server and would it help?
>

Right now, as you know, the one-to-one DBus interface is on the helper
process side. This makes more sense to me because there is currently
only one instance (password request) where the helper is requesting
information from the server process. In this case, we send a signal
and block until the password response DBus method is invoked. But this
is the single instance that the helper process needs to get
information from the server process. Moving the one-to-one DBus
interface to the server process would introduce many more of these
signal/response methods as the server often requests info from the
helper due to the client calls.

> I think it would help, at least in some cases.
>
> For example, requesting a password can be blocking. In that case it
> would be the job of the syncevo-dbus-server to deny the request when the
> helper is meant to abort. Same for other blocking requests.
>

And we do block here. It's just doing a g_main_context_iteration loop
while waiting for the server to do the Info Request. But like I said,
this is the only instance where this is needed.

> Another alternative would be to include "abort method call"
> functionality into our blocking D-Bus method call implementation. For
> example, a SIGTERM could be sent to the helper, get caught by
> SuspendFlags and then be detected by the generic code which executes a
> blocking D-Bus call. That might simplify the logic in the
> syncevo-dbus-server (send SIGTERM, wait for helper to terminate).
>
> It would also help the PBAP backend, which now also blocks in a D-Bus
> method call to obexd while the session it participates in might already
> have been aborted.
>
> It makes the D-Bus bindings a bit more difficult, in particular because
> they are meant to be usable without SyncEvolution (not sure whether that
> is still useful). I'm just tossing out the idea so that we can consider
> its merits.

Is there actually an instance of this binding be used outside of
SyncEvolution? Maybe moving out into its own project would help with
that.

>
> Chris, you are the one designing the syncevo-dbus-server <-> helper
> interaction and know where the pain points are. If changes in the
> infrastructure will help, by all means, let's do that.
>

At this point, the changes required to do the g_main_loop dance are
*far* less than those that are required to move the one-to-one DBus
interface to the server process. As I've said, I think it's actually
better to keep the interface in the helper.

In short, I don't think this would really help.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] DBus server rework: syncevo-dbus-server <-> helper communication

[Chris Kühl]

On Wed, Feb 22, 2012 at 2:27 PM, Patrick Ohly  wrote:
> On Wed, 2012-02-22 at 14:15 +0100, Chris Kühl wrote:
>> 2012/2/22 Patrick Ohly :
>>
>> And we do block here. It's just doing a g_main_context_iteration loop
>> while waiting for the server to do the Info Request. But like I said,
>> this is the only instance where this is needed.
>
> Does it handle requests to abort here? How are those communicated? Via
> signal or D-Bus call?
>

Currently it doesn't handle aborting. I'll pass a shutdownRequested
flag, which is set in niam(), to the Session similar to how it's done
for the Server. The loop and askPassword function will then be exited
when that's true.

In general shutdown occurs if the server calls serverShutdown() and
the session has not started running or if a shutdown signal was
received. There will probably be some refinement needed here, though.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

[SyncEvolution] Concurrent sync sessions update - Almost done

[Chris Kühl]

Hi Patrick,

We've now got the GIO GDBus issues fixed and all the non-autosync &
non-connection tests pass with both DBus wrappers. We are currently
going through the code and making sure code is commented properly,
debugging output is cleaned up and header files are organized more
cleanly. The uncleaned code is in the knowak/for-Chris branch. We
don't suspect to be making anymore logic changes to that, just clean
it up. The clean-up will be finished tomorrow and I'll place it in a
branch named concurrent-sync-sessions-for-review.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Concurrent sync sessions update - Almost done

[Chris Kühl]

On Wed, Mar 7, 2012 at 4:35 PM, Patrick Ohly  wrote:
> On Tue, 2012-03-06 at 17:53 +0100, Patrick Ohly wrote:
>> On Tue, 2012-03-06 at 16:43 +0100, Chris Kühl wrote:
>> > We've now got the GIO GDBus issues fixed and all the non-autosync &
>> > non-connection tests pass with both DBus wrappers. We are currently
>> > going through the code and making sure code is commented properly,
>> > debugging output is cleaned up and header files are organized more
>> > cleanly.
>>
>> Can you rebase it onto for-master/new-master or even
>> for-master/platform-modules?
>
> Please ignore for-master/platform-modules for now, it has a regression
> visible in test-dbus.py. I'm testing a fix:
>

Yeah, we were working on rebasing on to for-master/platform-modules
but were running into issues with the LocalSync and Interactive
password tests. Those tests were passing before we rebased. Taking a
quick look at for-master/platform-modules it seems you are reworking
the password code which would probably break the changes I made to get
interactive passwords working.

Now that you've merged for-master/new-master into master we are
rebasing onto master.

Cheers,
Chris

> diff --git a/src/dbus/server/dbus-sync.cpp b/src/dbus/server/dbus-sync.cpp
> index 0c64b9a..4a86c88 100644
> --- a/src/dbus/server/dbus-sync.cpp
> +++ b/src/dbus/server/dbus-sync.cpp
> @@ -26,7 +26,7 @@ SE_BEGIN_CXX
>
>  DBusSync::DBusSync(const std::string &config,
>                    Session &session) :
> -    SyncContext(config),
> +    SyncContext(config, true),
>     m_session(session)
>  {
>     setUserInterface(this);
>
>
> --
> Best Regards, Patrick Ohly
>
> The content of this message is my personal opinion only and although
> I am an employee of Intel, the statements I make here in no way
> represent Intel's position on the issue, nor am I authorized to speak
> on behalf of Intel on this matter.
>
>
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Concurrent sync sessions update - Almost done

[Chris Kühl]

On Thu, Mar 8, 2012 at 4:43 PM, Patrick Ohly  wrote:
> On Wed, 2012-03-07 at 16:54 +0100, Chris Kühl wrote:
>> Now that you've merged for-master/new-master into master we are
>> rebasing onto master.
>
> How is that going?
>

It's been rebased and we've squashed the changes into about a dozen
commits. The tests seem to give us the same results as before the
rebase. See the concurrent-sync-sessions-rebased-pre-cleanup branch
for the rebased and squashed changes. I'm putting the final touches on
the cleanup know.

> I think I fixed all issues on the for-master/platform-modules branch,
> but don't let that distract you. Once there is a clean patch series for
> current master, I can review and adapt it to the modified password
> handling.
>

ok

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Concurrent sync sessions update - Almost done

[Chris Kühl]

On Thu, Mar 8, 2012 at 5:55 PM, Patrick Ohly  wrote:
> On Thu, 2012-03-08 at 17:18 +0100, Chris Kühl wrote:
>> On Thu, Mar 8, 2012 at 4:43 PM, Patrick Ohly  wrote:
>> > On Wed, 2012-03-07 at 16:54 +0100, Chris Kühl wrote:
>> >> Now that you've merged for-master/new-master into master we are
>> >> rebasing onto master.
>> >
>> > How is that going?
>> >
>>
>> It's been rebased and we've squashed the changes into about a dozen
>> commits. The tests seem to give us the same results as before the
>> rebase. See the concurrent-sync-sessions-rebased-pre-cleanup branch
>> for the rebased and squashed changes. I'm putting the final touches on
>> the cleanup know.
>
> 057a19f18c600fe4158fd06087aed10b44f20c4e
>
> syncevo: Added methods to add and get environment variables to ForkExec
>
>    The parent may need to pass additional environment variables to the
>    child process.
>
>    This commit adds an Add method to the parent process and a Get method
>    for the child process.
>
> +    /*
> +     * Return the value for the environment variable name or and empty
> +     * string is not found or set to ForkExecEnvVarEmpty.
> +     */
> +    std::string getEnvVar(const std::string &name);
>
> Why is this ForkExecEnvVarEmpty necessary?
>

Ah, originally I wanted to use the just the fact that the variable
existed without setting a value but some value needs to be set for env
variables. However, somewhere along the line I changed my mind. I'll
get rid of this.

> Note that there is already a GetEnv() in util.h which returns an empty
> std::string when getenv() returns NULL.
>

Thanks, I wasn't aware. I'll change it to use that.

> +std::string ForkExecChild::getEnvVar(const std::string &name)
> +{
> +    gchar *pValue = getenv(name.c_str());
> +    if(!pValue || boost::iequals(pValue, ForkExecEnvVarEmpty)) {
> +        return std::string();
> +    } else {
> +        return std::string(pValue);
> +
>
> getenv() returns "char *", not "gchar *". I don't know where that makes
> a difference, but if it is guaranteed to be the same, why introduce
> gchar? That implies to me that there might be a difference somewhere.
>

You're right. Moot point now though.

>
> d85da736dec0975962b610e5c3b0b0dc52b612b3
>
> ForkExec.cpp: Fix the build without --enable-dbus-service.
>
>    The .h file's contents are inside an ifdef, so the implementation in
>    .cpp should be too.
>
> --- src/syncevo/ForkExec.cpp 
> ---
> index 0395c33..1f71331 100644
> @@ -18,8 +18,10 @@
>  */
>
>  #include "ForkExec.h"
>
> +#if defined(HAVE_GLIB) && defined(DBUS_SERVICE)
> +
>  SE_BEGIN_CXX
>
>  #if defined(HAVE_GLIB)
>
> @@ -292,4 +294,5 @@ const char *ForkExecChild::getParentDBusAddress()
>  #endif // HAVE_GLIB
>
>  SE_END_CXX
>
> +#endif // HAVE_GLIB
>
> The if and endif don't match, and HAVE_GLIB is tested twice. I suggest
>
> +#if defined(HAVE_GLIB) && defined(DBUS_SERVICE)
> ...
> -#if defined(HAVE_GLIB)
> ...
> -#endif // HAVE_GLIB
> ...
> +#endif // HAVE_GLIB && DBUS_SERVICE
>

Yeah, just saw that too.

> dd0a4ab2d584004095f5f0e7cd30770ae3d952c8
>
>    Add some common DBus callbacks.
>
>    These callbacks will be used once sync sessions run in their own
>    process.
>
>    nullCb:    a function that does nothing.
>    counterCb: a function that calls given callback when given counter
>               drops to zero.
>
>    Example use:
> ...
>
> The documentation should be in comments in the header file, not in the
> commit message. That way it is more readily available when reading the
> source code in the future.
>
> Note that global functions normally start with a a capital letter, to
> distinguish them from member methods, which start with a lower capital.
>

I'll make these changes and squash them into the for-review branch.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Concurrent sync sessions update - Almost done

[Chris Kühl]

On Thu, Mar 8, 2012 at 6:30 PM, Chris Kühl  wrote:
> On Thu, Mar 8, 2012 at 5:55 PM, Patrick Ohly  wrote:
>> On Thu, 2012-03-08 at 17:18 +0100, Chris Kühl wrote:
>>> On Thu, Mar 8, 2012 at 4:43 PM, Patrick Ohly  wrote:
>>> > On Wed, 2012-03-07 at 16:54 +0100, Chris Kühl wrote:
>>> >> Now that you've merged for-master/new-master into master we are
>>> >> rebasing onto master.
>>> >
>>> > How is that going?
>>> >
>>>
>>> It's been rebased and we've squashed the changes into about a dozen
>>> commits. The tests seem to give us the same results as before the
>>> rebase. See the concurrent-sync-sessions-rebased-pre-cleanup branch
>>> for the rebased and squashed changes. I'm putting the final touches on
>>> the cleanup know.
>>
>> 057a19f18c600fe4158fd06087aed10b44f20c4e
>>



>> Note that there is already a GetEnv() in util.h which returns an empty
>> std::string when getenv() returns NULL.
>>
>
> Thanks, I wasn't aware. I'll change it to use that.
>

Hmm, upon inspection I don't actually see this. I only see getEnv()
which returns a char*. I can simply use getenv, though.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Concurrent sync sessions update - Almost done

[Chris Kühl]

On Thu, Mar 8, 2012 at 6:23 PM, Patrick Ohly  wrote:
> On Thu, 2012-03-08 at 17:18 +0100, Chris Kühl wrote:
>> On Thu, Mar 8, 2012 at 4:43 PM, Patrick Ohly  wrote:
>> > On Wed, 2012-03-07 at 16:54 +0100, Chris Kühl wrote:
>> >> Now that you've merged for-master/new-master into master we are
>> >> rebasing onto master.
>> >
>> > How is that going?
>> >
>>
>> It's been rebased and we've squashed the changes into about a dozen
>> commits. The tests seem to give us the same results as before the
>> rebase. See the concurrent-sync-sessions-rebased-pre-cleanup branch
>> for the rebased and squashed changes. I'm putting the final touches on
>> the cleanup know.
>
> 6c9a05a9db72f001d9834d2d24ac589f48fc5798
>
>    dbus-server: Run sync sessions in separate processes
>
> ...
>
>    Sessions are separated into SessionResource and Session classes. A
>    SessionResource instance resides in the server process and serves as a
>    proxy to the Session instance which is in the child process.
>
> This naming seems rather arbitrary to me. Why call it "Resource" and not
> something like "Stub" or "Proxy"?
>

Yeah, I'm not 100% happy with the naming either. They are subclasses
of Resource so it was the obvious choice. Renaming is not a problem
but I'd rather get finished with the more substantive changes needed
to complete this before doing that.

> Or, perhaps even better, don't rename it at all on the server side. Then
> a whole range of diffs goes away:
>

It's just that the Session is actually not in the Server anymore so it
seems a tad misleading to call it that in the Server.

>  src/dbus/server/auto-sync-manager.cpp 
> 
> index 53cdceb..a657f0a 100644
> @@ -17,10 +17,11 @@
>  * 02110-1301  USA
>  */
>
>  #include "auto-sync-manager.h"
> -#include "session.h"
> +#include "session-resource.h"
>  #include "server.h"
> +#include "dbus-callbacks.h"
> ...
>  void AutoSyncManager::startTask()
>  {
>     // get the front task and run a sync
>     // if there has been a session for the front task, do nothing
> -    if(hasTask() && !m_session) {
> +    if(hasTask() && !m_sessionResource) {
>
> But to be fair, I haven't checked whether that makes the diff larger
> elsewhere, for example in the actual implementation (session.h/cpp).
>
> Speaking of those, I'd like to see those moved to a separate directory
> eventually. There are different coding styles in server (strictly
> asynchronous) and in client (may block) and that should be made obvious
> by the location of the file.
>
>

You're speaking of keeping the files that are only used in the helper
in a subdirectory, right?

I'd rather do this towards the end of the process as well? Moving and
renaming files shouldn't be a problem as long as the other pieces are
fine.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Concurrent sync sessions update - Almost done

[Chris Kühl]

On Thu, Mar 8, 2012 at 7:16 PM, Patrick Ohly  wrote:
> On Thu, 2012-03-08 at 18:58 +0100, Chris Kühl wrote:
>> On Thu, Mar 8, 2012 at 6:23 PM, Patrick Ohly  wrote:
>> > On Thu, 2012-03-08 at 17:18 +0100, Chris Kühl wrote:
>> > 6c9a05a9db72f001d9834d2d24ac589f48fc5798
>> >
>> >    dbus-server: Run sync sessions in separate processes
>> >
>> > ...
>> >
>> >    Sessions are separated into SessionResource and Session classes. A
>> >    SessionResource instance resides in the server process and serves as a
>> >    proxy to the Session instance which is in the child process.
>> >
>> > This naming seems rather arbitrary to me. Why call it "Resource" and not
>> > something like "Stub" or "Proxy"?
>> >
>>
>> Yeah, I'm not 100% happy with the naming either. They are subclasses
>> of Resource so it was the obvious choice. Renaming is not a problem
>> but I'd rather get finished with the more substantive changes needed
>> to complete this before doing that.
>
> Everything that minimizes the number of changes that I need to look at
> helps.
>

It actually increased the changed line count. :(

>> > Or, perhaps even better, don't rename it at all on the server side. Then
>> > a whole range of diffs goes away:
>> >
>>
>> It's just that the Session is actually not in the Server anymore so it
>> seems a tad misleading to call it that in the Server.
>
> It's still the implementation of the D-Bus Session API in the server,
> isn't it?
>
> So for the sake of minimizing code churn, Session (in the server) and
> SessionImpl (in the client) might work.
>

I've gone ahead and done the renaming as well as all the other changes
you've requested except for moving the helper files into a
subdirectory. I've pushed them to a new
concurrent-sync-sessions-for-review branch[1].

As mentioned above renaming has increased the changed line count. I've
got a branch with all the changes minus the renaming if you'd rather
have that.

Cheers,
Chris

[1] 
https://meego.gitorious.org/meego-middleware/syncevolution/commits/concurrent-sync-sessions-for-review
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Concurrent sync sessions update - Almost done

[Chris Kühl]

On Fri, Mar 9, 2012 at 8:05 AM, Patrick Ohly  wrote:
> On Fri, 2012-03-09 at 00:52 +0100, Chris Kühl wrote:
>> On Thu, Mar 8, 2012 at 7:16 PM, Patrick Ohly  wrote:
>> > On Thu, 2012-03-08 at 18:58 +0100, Chris Kühl wrote:
>> >> On Thu, Mar 8, 2012 at 6:23 PM, Patrick Ohly  
>> >> wrote:
>> >> > On Thu, 2012-03-08 at 17:18 +0100, Chris Kühl wrote:
>> >> > 6c9a05a9db72f001d9834d2d24ac589f48fc5798
>> >> >
>> >> >    dbus-server: Run sync sessions in separate processes
>> >> >
>> >> > ...
>> >> >
>> >> >    Sessions are separated into SessionResource and Session classes. A
>> >> >    SessionResource instance resides in the server process and serves as 
>> >> > a
>> >> >    proxy to the Session instance which is in the child process.
>> >> >
>> >> > This naming seems rather arbitrary to me. Why call it "Resource" and not
>> >> > something like "Stub" or "Proxy"?
>> >> >
>> >>
>> >> Yeah, I'm not 100% happy with the naming either. They are subclasses
>> >> of Resource so it was the obvious choice. Renaming is not a problem
>> >> but I'd rather get finished with the more substantive changes needed
>> >> to complete this before doing that.
>> >
>> > Everything that minimizes the number of changes that I need to look at
>> > helps.
>> >
>>
>> It actually increased the changed line count. :(
>
> I can see how it might do that. But it did reduce the number of chunks
> from 152 to 130.
>

Yeah, the session and connection files in the helper are quite a bit
larger than those in the server.

>> I've gone ahead and done the renaming as well as all the other changes
>> you've requested except for moving the helper files into a
>> subdirectory. I've pushed them to a new
>> concurrent-sync-sessions-for-review branch[1].
>>
>> As mentioned above renaming has increased the changed line count. I've
>> got a branch with all the changes minus the renaming if you'd rather
>> have that.
>
> Please push it, I'll have a look.
>

Done. It's named concurrent-sync-sessions-for-review-before-rename[1].

Cheers,
chris

[1] 
https://meego.gitorious.org/meego-middleware/syncevolution/commits/concurrent-sync-sessions-for-review-before-rename
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Concurrent sync sessions update - Almost done

[Chris Kühl]

On Fri, Mar 9, 2012 at 9:54 AM, Chris Kühl  wrote:
> On Fri, Mar 9, 2012 at 8:05 AM, Patrick Ohly  wrote:
>> On Fri, 2012-03-09 at 00:52 +0100, Chris Kühl wrote:
>>> On Thu, Mar 8, 2012 at 7:16 PM, Patrick Ohly  wrote:
>>> > On Thu, 2012-03-08 at 18:58 +0100, Chris Kühl wrote:
>>> >> On Thu, Mar 8, 2012 at 6:23 PM, Patrick Ohly  
>>> >> wrote:
>>> >> > On Thu, 2012-03-08 at 17:18 +0100, Chris Kühl wrote:
>>> >> > 6c9a05a9db72f001d9834d2d24ac589f48fc5798
>>> >> >
>>> >> >    dbus-server: Run sync sessions in separate processes
>>> >> >
>>> >> > ...
>>> >> >
>>> >> >    Sessions are separated into SessionResource and Session classes. A
>>> >> >    SessionResource instance resides in the server process and serves 
>>> >> > as a
>>> >> >    proxy to the Session instance which is in the child process.
>>> >> >
>>> >> > This naming seems rather arbitrary to me. Why call it "Resource" and 
>>> >> > not
>>> >> > something like "Stub" or "Proxy"?
>>> >> >
>>> >>
>>> >> Yeah, I'm not 100% happy with the naming either. They are subclasses
>>> >> of Resource so it was the obvious choice. Renaming is not a problem
>>> >> but I'd rather get finished with the more substantive changes needed
>>> >> to complete this before doing that.
>>> >
>>> > Everything that minimizes the number of changes that I need to look at
>>> > helps.
>>> >
>>>
>>> It actually increased the changed line count. :(
>>
>> I can see how it might do that. But it did reduce the number of chunks
>> from 152 to 130.
>>
>
> Yeah, the session and connection files in the helper are quite a bit
> larger than those in the server.
>
>>> I've gone ahead and done the renaming as well as all the other changes
>>> you've requested except for moving the helper files into a
>>> subdirectory. I've pushed them to a new
>>> concurrent-sync-sessions-for-review branch[1].
>>>
>>> As mentioned above renaming has increased the changed line count. I've
>>> got a branch with all the changes minus the renaming if you'd rather
>>> have that.
>>
>> Please push it, I'll have a look.
>>

Have you had a chance to look at this?

Cheers,
Chris

>
> Done. It's named concurrent-sync-sessions-for-review-before-rename[1].
>
> Cheers,
> chris
>
> [1] 
> https://meego.gitorious.org/meego-middleware/syncevolution/commits/concurrent-sync-sessions-for-review-before-rename
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] Concurrent sync sessions update - Almost done

[Chris Kühl]

On Wed, Mar 14, 2012 at 11:28 AM, Patrick Ohly  wrote:
> On Thu, 2012-03-08 at 17:18 +0100, Chris Kühl wrote:
>> It's been rebased and we've squashed the changes into about a dozen
>> commits. The tests seem to give us the same results as before the
>> rebase.
>
> And those are? In other words, which tests are known to fail, and why?
>

I've pushed a branch called css-without-known-test-failures[1] whose
most recent commit comments out all the tests in test/test-dbus.py
that fail. However, there as a bug I introduced during the last
rebase. The fix for that can be found here[2].

All but one of the failures is due to the incomplete AutoSync and
Connection functionality. The restart test has never worked for us on
any branch.

Cheers,
Chris

[1] 
https://meego.gitorious.org/meego-middleware/syncevolution/commit/d88052510b77c8236ced646e77ee0d9303bda126
[2] 
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolution/commit/13896a1df0dfc815037e0749638f03decee507ac
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] shutdown request handling + fork/exec dbus-server

[Chris Kühl]

On Wed, Mar 14, 2012 at 2:26 PM, Patrick Ohly  wrote:
> Hello Chris!
>
> Your patch removes the Server::m_shutdownSession and replaces it with a
> boolean m_shutdownRequested + a timeout that is set once in
> Server::fileModified().
>
> The comment in server.h still refers to m_shutdownSession and explains
> that its purpose was to prevent other sessions from running. How is that
> handled now?
>

That is still the intent but now that you're pointing it out I see
that the implementation is not complete.

> More specifically, consider the following sequence of events:
>     1. files are modified and the quiesence timeout is started
>     2. a client connects, starts a session and syncs
>     3. the timeout triggers, causing the server to shut down
>
> If it works as designed (we don't have tests for it, do we?), then the
> syncevo-dbus-helper will continue to run and complete the session. But
> the client will see an unexpected server disconnect and won't be able to
> monitor progress of the still running session, because the running
> helper isn't connected to any syncevo-dbus-server instance.
>
> It could even be worse: with bad timing, an obsolete syncevo-dbus-helper
> might get started which then fails because it doesn't match the
> installed files.
>
> IMHO adding any kind of resource should be prevented while the
> m_shutdownRequested flag is set. Returning an error message to the
> client would be okay

Yes, and this is how it should work. I forgot to guard against this case.

> Do you agree with this analysis or did I miss something?
>

Yes. That all sounds spot on.

Cheers,
Chris

> Don't bother changing code, I'm just trying to clarify the existing one.
>
> --
> Best Regards, Patrick Ohly
>
> The content of this message is my personal opinion only and although
> I am an employee of Intel, the statements I make here in no way
> represent Intel's position on the issue, nor am I authorized to speak
> on behalf of Intel on this matter.
>
>
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] server class + fork/exec dbus-server

[Chris Kühl]

On Fri, Mar 16, 2012 at 11:54 AM, Patrick Ohly  wrote:
> On Fri, 2012-03-16 at 11:33 +0100, Patrick Ohly wrote:
>> On Thu, 2012-03-15 at 14:43 +0100, Patrick Ohly wrote:
>> > I still think resources should be added to m_waitingResources before
>> > construction is complete. Activating the object and notifying the
>> > caller
>> > can wait until the resource is really usable (= helper ready).
>>
>> And one more observation: running the helper should get delayed until it
>> is really needed, which probably means until the resource is ready to
>> become active.
>>
>> Imagine having 100 pending session requests. An entry in
>> m_waitingResources is cheap enough to handle that (admittedly unlikely)
>> case, but combine that with 100 syncevo-dbus-helper processes and we
>> have a problem.
>
> Speaking of more than one entry in m_waitingResources: does the current
> PriorityCompare really implement a strict weak ordering, as required by
> std::set, the storage used for m_waitingResource?
>
> I don't think it does, because two different resources with the same
> priority will return false regardless which way you compare them, and
> that despite not being equivalent. I bet that as a result, you currently
> cannot store two resources with the same priority in m_waitingResource.
> Even if you could, what would be the relative order? Everything else
> being equal, we want first-come-first-serve.
>

oops, you're right. That should have been a multiset.

Here is a small test I'd written to make sure the ordering was correct.

// constructing multisets
#include 
#include 
using namespace std;

struct classComp {
  bool operator() (const string& lhs, const string& rhs) const
  {return lhs[0]>rhs[0];}
};

int main ()
{
multiset setComp;
multiset::iterator it;

setComp.insert("ace");
setComp.insert("bud");
setComp.insert("bad");
setComp.insert("bla");
setComp.insert("cup");
setComp.insert("boo");

// show content:
for ( it=setComp.begin() ; it != setComp.end(); it++ )
cout << (*it) << endl;

return 0;
}

The ordering works but for some reason, the "multi" prefix didn't make
it into the server.

> Another concern is that the outcome of PriorityCompare depends on being
> able to lock shared pointers. If a resource goes away, then the
> invariants for the set might get violated (comparison no longer returns
> the same results as before). I don't want to find out how std::set deals
> with that...
>

I believe I'm handling the case where the weak pointer goes away
already. If the lock is not obtained for the already-in-queue Resource
AND the to-insert Resource is locked the we move on to the next
comparison. If the to-insert Resource is gone then we stop right
there. I does get inserted but it'll be cleaned up in short time.

> In a nutshell, I think we need to go back to the original std::list and
> its brute-force insertion sort. Agreed?
>

I found it a bit nicer to use the comparison class but if you're not
convinced feel free to revert that.

Cheers,
Chris
___
SyncEvolution mailing list
SyncEvolution@syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution

Re: [SyncEvolution] server class + fork/exec dbus-server

[Chris Kühl]

On Fri, Mar 16, 2012 at 1:52 PM, Patrick Ohly  wrote:
> On Fri, 2012-03-16 at 13:00 +0100, Chris Kühl wrote:
>> On Fri, Mar 16, 2012 at 11:54 AM, Patrick Ohly  
>> wrote:
>> > On Fri, 2012-03-16 at 11:33 +0100, Patrick Ohly wrote:
>> >> On Thu, 2012-03-15 at 14:43 +0100, Patrick Ohly wrote:
>> >> > I still think resources should be added to m_waitingResources before
>> >> > construction is complete. Activating the object and notifying the
>> >> > caller
>> >> > can wait until the resource is really usable (= helper ready).
>> >>
>> >> And one more observation: running the helper should get delayed until it
>> >> is really needed, which probably means until the resource is ready to
>> >> become active.
>> >>
>> >> Imagine having 100 pending session requests. An entry in
>> >> m_waitingResources is cheap enough to handle that (admittedly unlikely)
>> >> case, but combine that with 100 syncevo-dbus-helper processes and we
>> >> have a problem.
>> >
>> > Speaking of more than one entry in m_waitingResources: does the current
>> > PriorityCompare really implement a strict weak ordering, as required by
>> > std::set, the storage used for m_waitingResource?
>> >
>> > I don't think it does, because two different resources with the same
>> > priority will return false regardless which way you compare them, and
>> > that despite not being equivalent. I bet that as a result, you currently
>> > cannot store two resources with the same priority in m_waitingResource.
>> > Even if you could, what would be the relative order? Everything else
>> > being equal, we want first-come-first-serve.
>> >
>>
>> oops, you're right. That should have been a multiset.
>>
>> Here is a small test I'd written to make sure the ordering was correct.
>
> ... but is it guaranteed to be correct, or does it just happen to work
> as intended for your current implementation of std::multiset? I don't
> see anything in the SGI STL documentation that says that a multiset
> preserves the order in which elements are added.
>
> The Linux man page says "Iteration is done in ascending order according
> to the keys." That leaves it undefined what the order is for equal keys.
>
>> > Another concern is that the outcome of PriorityCompare depends on being
>> > able to lock shared pointers. If a resource goes away, then the
>> > invariants for the set might get violated (comparison no longer returns
>> > the same results as before). I don't want to find out how std::set deals
>> > with that...
>> >
>>
>> I believe I'm handling the case where the weak pointer goes away
>> already.
>
> Your code does, but does std::multiset? It's data structure is based on
> the result of the comparison between two elements. If that comparison
> suddenly returns different results, some invariants inside std::multiset
> get violated. All hell might break loose.
>

You're absolutely right. Item 22 in "Effective STL" explains just this
situation. Thanks for catching and explaining that.

>> > In a nutshell, I think we need to go back to the original std::list and
>> > its brute-force insertion sort. Agreed?
>> >
>>
>> I found it a bit nicer to use the comparison class but if you're not
>> convinced feel free to revert that.
>
> It's nicer, but I am not convinced that it is correct.

And you're right.

>
> Another aspect is that m_waitingResources now mixes connections and
> sessions. That wasn't the case before. A connection created a session
> which then got scheduled, instead of being scheduled like one. There is
> a reason for that: suppose there is a running connection for client A.
> The client gives up on that connection and starts anew. Is that new
> connection allowed to start? If not, it will be blocked by the stale
> connection that it is meant to replace.
>

Like I said the Connection code is incomplete and this is because I
wasn't convinced I was taking the right approach. In hindsight I would
have started a connection immediately. Once the session needed to be
created by the Connection the  helper would create a Session object,
set up the one-to-one dbus interface and wait for the Server to create
a SessionResource then place it in the queue. At this point we'd be in
the same situation as if only a Session had been requested.

> So we have a pretty complex logic now: allow multiple
> ConnectionResources to be active, but prevent running multiple sync