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
