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 <patrick.o...@intel.com> 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 <patrick.o...@intel.com> 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 <patrick.o...@intel.com> 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 <patrick.o...@intel.com> 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 <patrick.o...@intel.com> 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 <patrick.o...@intel.com> 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.
diff --git a/src/backends/pbap/PbapSyncSource.cpp b/src/backends/pbap/PbapSyncSource.cpp index 33ba39f..fdafc69 100644 --- a/src/backends/pbap/PbapSyncSource.cpp +++ b/src/backends/pbap/PbapSyncSource.cpp @@ -36,6 +36,7 @@ #include <errno.h> #include <unistd.h> +#include <sys/stat.h> #include <sys/mman.h> #include <syncevo/util.h> @@ -200,7 +201,7 @@ void PbapSession::pullAll(Content &dst) SE_LOG_ERROR(NULL, NULL, "Stat on file failed"); return; } - SE_LOG_DEBUG(NULL, NULL, "Temporary file size is %d", sb.st_size); + SE_LOG_DEBUG(NULL, NULL, "Temporary file size is %ld", (long)sb.st_size); addr = (char*)mmap(NULL, sb.st_size, PROT_READ, MAP_PRIVATE, fd, 0); if (addr == MAP_FAILED) {
_______________________________________________ SyncEvolution mailing list SyncEvolution@syncevolution.org http://lists.syncevolution.org/listinfo/syncevolution