Patrick, Thanks for the review and DBus patch. Unfortunately it seems incomplete. When I run my syncpbap branch found here (I incorporated your build fix, that dbus commit, and much of the fyi commit also): http://cgit.collabora.com/git/user/jwhiting/syncevolution.git
I get the following output from syncevolution --print-items target-config@pbap addressbook syncevolution --print-items target-config@pbap addressbook [DEBUG 00:00:00] Mon 2012-08-27 22:44:42 UTC = 16:44 -0600 MDT [DEBUG 00:00:00] ForkExecParent: preparing for child process syncevo-dbus-helper [DEBUG 00:00:00] ForkExecParent: running /usr/local/libexec/syncevo-dbus-helper with D-Bus address unix:abstract=gdbuscxx-1 [DEBUG 00:00:00] ForkExecParent: child process for /usr/local/libexec/syncevo-dbus-helper has pid 9691 [DEBUG 00:00:00] SuspendFlags: (re)activating, currently inactive [DEBUG 00:00:00] SuspendFlags: activating signal handler(s) with fds 9->8 [DEBUG 00:00:00] SuspendFlags: catch SIGINT [DEBUG 00:00:00] SuspendFlags: catch SIGTERM [DEBUG syncevo-dbus-server 00:00:00] Client connected. [DEBUG syncevo-dbus-server 00:00:00] Peer credentials: GCredentials:linux-ucred:pid=9691,uid=1000,gid=1000 [DEBUG syncevo-dbus-server 00:00:00] Negotiated capabilities: unix-fd-passing=1 [DEBUG syncevo-dbus-server 00:00:00] ForkExecParent: child syncevo-dbus-helper has connected [DEBUG 00:00:00] helper has connected [DEBUG 00:00:00] helper (pid 9691) finished setup, waiting for parent connection [DEBUG 00:00:00] connected to parent, run helper [DEBUG 00:00:00] ---> PBAP::createSource() [DEBUG 00:00:00] Using new obex org.bluez.obex.Client [DEBUG 00:00:04] AddMatch failed: GDBus.Error:org.freedesktop.DBus.Error.MatchRuleInvalid: Unknown key "path_namespace" in match rule [DEBUG 00:00:04] execute() helper call completed, org.syncevolution.Server: AddMatch failed: GDBus.Error:org.freedesktop.DBus.Error.MatchRuleInvalid: Unknown key "path_namespace" in match rule [DEBUG 00:00:04] AddMatch failed: GDBus.Error:org.freedesktop.DBus.Error.MatchRuleInvalid: Unknown key "path_namespace" in match rule [ERROR 00:00:04] AddMatch failed: GDBus.Error:org.freedesktop.DBus.Error.MatchRuleInvalid: Unknown key "path_namespace" in match rule [DEBUG 00:00:04] session failed: remember 10500 error [DEBUG 00:00:04] SuspendFlags: deactivating fds 9->8 [DEBUG 00:00:04] SuspendFlags: close m_receiverFD 8 [DEBUG 00:00:04] SuspendFlags: close m_senderFD 9 [DEBUG 00:00:04] SuspendFlags: done with deactivation Do I need to build with some flag set or something so path_namespace will be a known key? thanks, Jeremy On Mon, Aug 27, 2012 at 9:20 AM, Patrick Ohly <[email protected]> wrote: > On Fri, 2012-08-24 at 17:37 +0200, Patrick Ohly wrote: >> On Thu, 2012-08-23 at 16:39 -0600, Jeremy Whiting wrote: >> > Patrick, >> > >> > Thanks for your guidance today. I got my branch uploaded to >> > http://cgit.collabora.com/git/user/jwhiting/syncevolution.git/ it's >> > the syncpbap branch. >> >> Wanted to look at it today, but haven't gotten around to it - sorry. > > I'm not sure whether you wanted comments considering that this is work > in progress, but here they are anyway ;-) > > After looking at your code I see several things which don't look right: > 1. "Completed" is registered for the Client path and interface; it > needs to be registered as "Complete" for the Transfer object. As > expected, when I run your code, it just hangs in the while() > loop waiting for the "Completed" signal. > 2. The code depends on receiving that signal and thus suffers from > the race condition described earlier. > 3. "} else {" is the preferred formatting style. > 4. "content = std::string(addr)" (for a memory-mapped addr) makes > the incorrect assumption that there is a trailing \0 byte. If > the entire file exactly aligns with page boundaries, that > assignment will read past the mapped area, causing a segfault. > 5. Nothing owns the memory-mapped region/file and unmaps/deletes > it. > 6. I had to patch your branch slightly to compile it on 64 bit > Debian Testing. Patch attached. > > The goal of memory mapping was to avoid setting up a std::string in the > first place - instead the intention was to either buffer in a > std::string (old API) or work with a memory-mapped buffer (new API). In > both cases the rest of the code must stop working with std::string as > input and instead work with a memory range. You can use pcrecpp's > StringPiece class for that. > > I'm suggesting pcrecpp because it is already a dependency of > SyncEvolution and I started using it for the PBAP backend in some other > code, see latest update to the pbap branch in upstream gitorious repo > (copied below). > > My main focus is still on using the pbap backend, so if you want I'll > let you continue working on the API change without getting in your way. > However, for using PBAP as intended I had to extend it a bit and fix > bugs that I found, which conflicts with your current branch. > > I also had a look at the necessary changes in GDBus CXX for supporting > path prefix filtering. The change is actually fairly intrusive, once one > takes into account all the consequence like having to pass more > information to signal callbacks. I have pushed the more experimental > code to "pbap-client-api", together with a PBAP backend that was hacked > as a proof-of-concept. > > I hope the GDBus change is useful for you as it stands (in other words, > if it has no bugs - I have done very little testing with it). The other > commit is just FYI, the real solution will be what you are working on. > > commit 9d8318d23161e8754b9e35efdd1d3701817646fa > Author: Patrick Ohly <[email protected]> > Date: Mon Aug 27 17:10:32 2012 +0200 > > PBAP: incomplete support for obexd 0.47 > > Demonstrates how to set up per-session signal callbacks. > Lacks error and result handling. Does not preserve compatibility > with obexd < 0.47. > > commit e2d23ff967483a34c2466437e26a32e180dc6a70 > Author: Patrick Ohly <[email protected]> > Date: Mon Aug 27 16:44:34 2012 +0200 > > GDBus GIO: more flexible SignalWatch > > Working with obexd 0.47's Transfer object depends on path prefix > filtering of signals to be efficient. This patch adds that support in > the GDBus C++ bindings, together with some documentation on how to > use it. > > It also allows relaxing the signal matching by interpreting empty > strings as "match any". > > Finally it adds support for passing of sender, path, interface and > member (= signal name) information to the user of the D-Bus > binding. This is necessary because a handler with wildcards for > matching cannot know the actual values unless they are passed to the > callback explicitly. This was not possible at all before for signals > (because extracting that information relied on a message pointer, > which wasn't set while decoding signal messages) and limited to just > the sender in method implementations. > > Besides in functionality, GDBus for GIO and for libdbus now also > differ slightly in their API. In order to pass meta data about a > signal into the demarshalling get() methods, they have to be stored in > ExtractArgs first and passed to get() via that struct, which is an > interface change. > > Derived code which extends marshalling/demarshalling needs to be > adapted accordingly. Allowing some ifdefs elsewhere seems simpler than > adapting GDBus for libdbus. Those ifdefs can be removed once libdbus > support is no longer needed. > > The patch relaxes access control to DBusObject and DBusRemoteObject > because that simplifies the code which works better with std::string. > > Traditionally the sender of a signal was not checked. This is probably > related to unsolved questions like "should watching org.foo.bar follow > owner changes", but not doing any checking is just not right > either. Not fixed in this patch. > > commit 9dce6488d617d7b31d4998e55e2f55143249b121 > Author: Patrick Ohly <[email protected]> > Date: Mon Aug 27 11:51:14 2012 +0200 > > PBAP: allow configuring format and fields via databaseFormat > > With this patch, the databaseFormat can be used as follows: > > [(2.1|3.0)][:][^]propname,propname,... > > 3.0 = download in vCard 3.0 instead of the default 2.1 > 3.0:^PHOTO = download in vCard 3.0 format, excluding PHOTO > PHOTO = download in vCard 2.1 format, only the PHOTO > > Valid property names are pulled from obexd with ListFilterFields(). > > commit 03d11362e95108f773154849a454aef95bd83968 > Author: Patrick Ohly <[email protected]> > Date: Mon Aug 27 11:49:19 2012 +0200 > > PBAP: fixed dangling reference > > Binding a temporary std::string (the result of getDatabase() in this > case) to a const reference is broken, because the temporary instance > will get deleted before the reference. > > commit e0cf4666e37ae8b35ac913a957a4f2d62fd2b74c > Author: Patrick Ohly <[email protected]> > Date: Mon Aug 27 11:47:03 2012 +0200 > > PBAP: fixed parsing of PullAll result > > Pulling the individual vCard out of the result stream was faulty: it > used the end position as length and thus included data from the next > vCard. > > commit 808b9ca8be4b2033f4205c70746690446f11da93 > Author: Patrick Ohly <[email protected]> > Date: Thu Aug 23 14:04:45 2012 +0200 > > PBAP: don't try to make up stable local IDs > > Local IDs across sessions are only useful when we also have useful > revision strings. For debugging it is easier to just enumerate the > contacts. Would be nice to use the same number as in the PBAP session, > but that information is not currently available via obexd (see "PBAP + > two-step download" on Bluez mailing list). > > As it stands now, the PBAP backend can only be used in slow syncs > where the engine does the matching. Perhaps that's the right way to do > it, instead of adding redundant code to the backends. > > -- > 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 [email protected] http://lists.syncevolution.org/listinfo/syncevolution
