7:46 p.m.
Hello!
I've picked up work on the PBAP backend [1] again. The "pbap" branch can
be merged with current master without problems and works as it did
before with obexd < 0.47 (see the src/backends/pbap/README):
* "refresh-from-remote" works.
* The entire phone address book, including PHOTOs, is downloaded
in each sync.
There are several todos for the code:
1. Adapt to obexd >= 0.47 and the new D-Bus API in it; the current
code no longer works.
2. Support incremental sync - somehow.
3. Remove overhead for syncing: don't write anything to disc if
nothing changed.
4. First copy contacts without PHOTO data, then add PHOTO data.
I'd like to tackle points 2 and 3 first. For incremental syncing, there
are three options:
A. The PBAP source could create a key for each contact based on
some property (currently done based on N) and a revision string
by hashing the vcard content (not done at the moment). Change
tracking would be done by the TrackingSyncSource. The downside
of this is that the TrackingSyncSource will get confused when
sending contacts sometimes with and sometimes without PHOTO
data.
B. Assemble a custom sync source which does its own change tracking
instead of relying on TrackingSyncSource, and use hashing to
detect changes.
C. Don't try to do change detection in the source at all. Instead
do a slow sync that doesn't write to the phone, let the
Synthesis engine do its normal matching in the first cycle
(without PHOTOs), remove local contacts which were not matched,
then do a second cycle to add PHOTO data.
These options are not mutually exclusive. Option C is needed if syncing
failed, because without it, a "refresh-from-remote" would be needed to
recover. Such a "refresh-from-remote" would wipe out all local data,
even those contacts which were not changed, which is undesirable (want
to reduce disk writes). A normal slow sync would leave deleted contacts
around.
Therefore I'll investigate that option first.
About obexd: compatibility with old obexd needs to be preserved for a
while, because Tizen uses obexd 0.44 and I don't know if or when it can
be updated. It's also convenient (some distros like Debian Testing do
not have 0.47 yet). Note that the new obexd API uses a temporary file to
transfer. Those files should be created in the system temp dir where
they can be garbage collected. Reliably detecting transfer completion
depends on listening to signals in advance - see [2].
In SyncEvolution, listening to D-Bus signals is done with the custom
GDBusCXX::SignalWatch template class. Documentation of that C++ D-Bus
binding is admittedly weak. Grep the source to find examples how it is
used and/or let me know if you want better documentation of the C++
wrapper and I'll add it.
[1] Previous discussions:
http://www.mail-archive.com/syncevolution@syncevolution.org/msg03148.html
http://www.mail-archive.com/syncevolution@syncevolution.org/msg03167.html
[2] http://thread.gmane.org/gmane.linux.bluez.kernel/28481
--
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.
9:31 a.m.
Patrick,
On Fri, Aug 17, 2012 at 5:46 AM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
Hello!
I've picked up work on the PBAP backend [1] again. The "pbap" branch can
be merged with current master without problems and works as it did
before with obexd < 0.47 (see the src/backends/pbap/README):
* "refresh-from-remote" works.
* The entire phone address book, including PHOTOs, is downloaded
in each sync.
There are several todos for the code:
1. Adapt to obexd >= 0.47 and the new D-Bus API in it; the current
code no longer works.
In order to tackle this task, I've gotten the pbap branch, rebased it
on master and gotten it to build by changing all the method() calls to
method.start(). However the guide in the src/backends/pbap/README no
longer works and gives the following message when I run syncevolution
--print-items test-config@pbap:
[ERROR] backend not supported by any of the backend modules (...) or
not correctly configured (backend=select backend databaseFormat=
syncFormat=)
[ERROR] configuration 'test-config@pbap' does not exist
though I did set up test-config@pbap (and it appears in --print-configs) with:
syncevolution --configure syncURL= addressbook/backend=pbap
addressbook/database=obex-bt://$MYMAC target-config@pbap addressbook
I do have a recent libsynthesis installed that it's building against.
Also how would you suggest compatibility with obex 0.47 and obex <
0.47 work? separate branches? separate code paths in the same branch
(maybe with a check to see if org.openobex.client or
org.bluez.obex.client is implemented to know which path to take).
2. Support incremental sync - somehow.
3. Remove overhead for syncing: don't write anything to disc if
nothing changed.
4. First copy contacts without PHOTO data, then add PHOTO data.
I'd like to tackle points 2 and 3 first. For incremental syncing, there
are three options:
A. The PBAP source could create a key for each contact based on
some property (currently done based on N) and a revision string
by hashing the vcard content (not done at the moment). Change
tracking would be done by the TrackingSyncSource. The downside
of this is that the TrackingSyncSource will get confused when
sending contacts sometimes with and sometimes without PHOTO
data.
B. Assemble a custom sync source which does its own change tracking
instead of relying on TrackingSyncSource, and use hashing to
detect changes.
C. Don't try to do change detection in the source at all. Instead
do a slow sync that doesn't write to the phone, let the
Synthesis engine do its normal matching in the first cycle
(without PHOTOs), remove local contacts which were not matched,
then do a second cycle to add PHOTO data.
These options are not mutually exclusive. Option C is needed if syncing
failed, because without it, a "refresh-from-remote" would be needed to
recover. Such a "refresh-from-remote" would wipe out all local data,
even those contacts which were not changed, which is undesirable (want
to reduce disk writes). A normal slow sync would leave deleted contacts
around.
Therefore I'll investigate that option first.
About obexd: compatibility with old obexd needs to be preserved for a
while, because Tizen uses obexd 0.44 and I don't know if or when it can
be updated. It's also convenient (some distros like Debian Testing do
not have 0.47 yet). Note that the new obexd API uses a temporary file to
transfer. Those files should be created in the system temp dir where
they can be garbage collected. Reliably detecting transfer completion
depends on listening to signals in advance - see [2].
In SyncEvolution, listening to D-Bus signals is done with the custom
GDBusCXX::SignalWatch template class. Documentation of that C++ D-Bus
binding is admittedly weak. Grep the source to find examples how it is
used and/or let me know if you want better documentation of the C++
wrapper and I'll add it.
In order to do this I just need to create a SignalWatch on Transfer
and copy the file? or get the data from the Transfer object?
thanks,
Jeremy
[1] Previous discussions:
http://www.mail-archive.com/syncevolution@syncevolution.org/msg03148.html
http://www.mail-archive.com/syncevolution@syncevolution.org/msg03167.html
[2] http://thread.gmane.org/gmane.linux.bluez.kernel/28481
--
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.
2:37 p.m.
On Mon, 2012-08-20 at 19:31 -0600, Jeremy Whiting wrote:
Patrick,
On Fri, Aug 17, 2012 at 5:46 AM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
> Hello!
>
> I've picked up work on the PBAP backend [1] again. The "pbap" branch
can
> be merged with current master without problems and works as it did
> before with obexd < 0.47 (see the src/backends/pbap/README):
> * "refresh-from-remote" works.
> * The entire phone address book, including PHOTOs, is downloaded
> in each sync.
>
> There are several todos for the code:
> 1. Adapt to obexd >= 0.47 and the new D-Bus API in it; the current
> code no longer works.
In order to tackle this task, I've gotten the pbap branch, rebased it
on master and gotten it to build by changing all the method() calls to
method.start(). However the guide in the src/backends/pbap/README no
longer works and gives the following message when I run syncevolution
--print-items test-config@pbap:
^^^^^^^^^^^
Typo - the name of the config is target-config@pbap, if you followed the
README.
though I did set up test-config@pbap (and it appears in
--print-configs) with:
syncevolution --configure syncURL= addressbook/backend=pbap
addressbook/database=obex-bt://$MYMAC target-config@pbap addressbook
^^^^^^^^^^^^^
Also how would you suggest compatibility with obex 0.47 and obex
<
0.47 work? separate branches? separate code paths in the same branch
(maybe with a check to see if org.openobex.client or
org.bluez.obex.client is implemented to know which path to take).
I would do a runtime check. The result of PbapSession::pullAllCb() could
be a pointer into memory (obexd < 0.47) or a memory-mapped file (obexd
= 0.47). Then the rest of the code then needs to be changed to work
with plain char pointers for the buffer instead of the current const
std::string, but would be the same for both versions.
> In SyncEvolution, listening to D-Bus signals is done with the
custom
> GDBusCXX::SignalWatch template class. Documentation of that C++ D-Bus
> binding is admittedly weak. Grep the source to find examples how it is
> used and/or let me know if you want better documentation of the C++
> wrapper and I'll add it.
In order to do this I just need to create a SignalWatch on Transfer
and copy the file? or get the data from the Transfer object?
You'll have to access the file. The Transfer object only gives you meta
information about the file.
You can set up operations in this order:
1. Create temporary file.
2. Create a SignalWatch for all object paths which updates status
if the Transfer's Filename matches the temporary file.
3. Start the transfer.
4. while ("still running") g_main_context_iteration(NULL, true);
5. mmap file
6. parse it
--
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.
3:26 p.m.
Hi Patrick,
On Mon, 2012-08-20 at 19:31 -0600, Jeremy Whiting wrote:
> Patrick,
>
> On Fri, Aug 17, 2012 at 5:46 AM, Patrick Ohly <patrick.ohly(a)intel.com>
wrote:
> > Hello!
> >
> > I've picked up work on the PBAP backend [1] again. The "pbap"
branch
> > can be merged with current master without problems and works as it
> > did before with obexd < 0.47 (see the src/backends/pbap/README):
> > * "refresh-from-remote" works.
> > * The entire phone address book, including PHOTOs, is downloaded
> > in each sync.
> >
> > There are several todos for the code:
> > 1. Adapt to obexd >= 0.47 and the new D-Bus API in it; the current
> > code no longer works.
>
> In order to tackle this task, I've gotten the pbap branch, rebased it
> on master and gotten it to build by changing all the method() calls to
> method.start(). However the guide in the src/backends/pbap/README no
> longer works and gives the following message when I run syncevolution
> --print-items test-config@pbap:
^^^^^^^^^^^
Typo - the name of the config is target-config@pbap, if you followed the
README.
> though I did set up test-config@pbap (and it appears in --print-configs)
with:
> syncevolution --configure syncURL= addressbook/backend=pbap
> addressbook/database=obex-bt://$MYMAC target-config@pbap
addressbook
^^^^^^^^^^^^^
> Also how would you suggest compatibility with obex 0.47 and obex <
> 0.47 work? separate branches? separate code paths in the same branch
> (maybe with a check to see if org.openobex.client or
> org.bluez.obex.client is implemented to know which path to take).
I would do a runtime check. The result of PbapSession::pullAllCb() could be a
pointer into memory (obexd < 0.47) or a memory-mapped file (obexd
>= 0.47). Then the rest of the code then needs to be changed to work
with plain char pointers for the buffer instead of the current const std::string,
but would be the same for both versions.
You have method signature changes in CreateSession(), so I would suggest you do the
runtime check exactly there. You should first try the 0.47-style call and if it fails fall
back to the old signature.
> > In SyncEvolution, listening to D-Bus signals is done with
the custom
> > GDBusCXX::SignalWatch template class. Documentation of that C++
> > D-Bus binding is admittedly weak. Grep the source to find examples
> > how it is used and/or let me know if you want better documentation
> > of the C++ wrapper and I'll add it.
>
> In order to do this I just need to create a SignalWatch on Transfer
> and copy the file? or get the data from the Transfer object?
You'll have to access the file. The Transfer object only gives you meta
information about the file.
You can set up operations in this order:
1. Create temporary file.
Is there any particular reason not to let obex-client create the temporary file?
2. Create a SignalWatch for all object paths which updates
status
A possible optimization here would be using path prefixes, so you can focus on transfers
belonging to your own session.
if the Transfer's Filename matches the temporary file.
I'd rather check the object path instead of the filename (of course you need to start
the transfer first).
3. Start the transfer.
4. while ("still running") g_main_context_iteration(NULL, true);
5. mmap file
6. parse it
Cheers,
Mikel
3:38 p.m.
On Tue, 2012-08-21 at 09:26 +0200, Mikel Astiz wrote:
> You can set up operations in this order:
> 1. Create temporary file.
Is there any particular reason not to let obex-client create the temporary file?
Have you seen my email on the Bluez list about using the obexd client
API correctly?
http://thread.gmane.org/gmane.linux.bluez.kernel/28481
The reason why I am suggesting to use a temporary file under control of
the client is that SyncEvolution has more control about cleaning up in
case of failures.
When obexd chooses a file name, who is responsible for deleting it in
error cases like "client fails to take the file"?
When SyncEvolution chooses the path, it can guarantee that it is
somewhere where either the system (/tmp/) or SyncEvolution itself (some
cache directory, to be implemented) will garbage-collect old obsolete
files.
> 2. Create a SignalWatch for all object paths which updates
status
A possible optimization here would be using path prefixes, so you can
focus on transfers belonging to your own session.
If the obexd API makes such guarantees, then they should be
documented ;-) Right now, client-api.txt only says that the Transfer
object has "Object path [variable prefix]/{transfer0,transfer1,...}"
- there's no indication at all that "variable prefix" is related to the
session path.
> if the Transfer's Filename matches the temporary
file.
I'd rather check the object path instead of the filename (of course you need to start
the transfer first).
And that's where you run into the race condition that I described in my
mail to the Bluez list: by the time that the SignalWatch is set up, the
client might already have missed the signals that it wants to receive.
--
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.
7:31 p.m.
Hi Patrick,
On Tue, 2012-08-21 at 09:26 +0200, Mikel Astiz wrote:
> > You can set up operations in this order:
> > 1. Create temporary file.
>
> Is there any particular reason not to let obex-client create the temporary
file?
Have you seen my email on the Bluez list about using the obexd client API
correctly?
http://thread.gmane.org/gmane.linux.bluez.kernel/28481
The reason why I am suggesting to use a temporary file under control of the
client is that SyncEvolution has more control about cleaning up in case of
failures.
When obexd chooses a file name, who is responsible for deleting it in error
cases like "client fails to take the file"?
When SyncEvolution chooses the path, it can guarantee that it is somewhere
where either the system (/tmp/) or SyncEvolution itself (some cache
directory, to be implemented) will garbage-collect old obsolete files.
Now I understand the problem. Perhaps we should consider modifications in the API as you
suggested in the other thread.
> > 2. Create a SignalWatch for all object paths which
updates
> > status
>
> A possible optimization here would be using path prefixes, so you can
> focus on transfers belonging to your own session.
If the obexd API makes such guarantees, then they should be documented ;-
) Right now, client-api.txt only says that the Transfer
object has "Object path [variable prefix]/{transfer0,transfer1,...}"
- there's no indication at all that "variable prefix" is related to the
session
path.
Indeed, I already sent a patch to fix the documentation.
> > if the Transfer's Filename matches the
temporary file.
>
> I'd rather check the object path instead of the filename (of course you
need to start the transfer first).
And that's where you run into the race condition that I described in my mail
to the Bluez list: by the time that the SignalWatch is set up, the client might
already have missed the signals that it wants to receive.
I still don't get the problem. I was proposing to install the watch with the session
prefix, not the full transfer path. Of course this relies on the undocumented prefix
string in the API. Am I missing something?
Cheers,
Mikel
8:42 p.m.
On Tue, 2012-08-21 at 13:31 +0200, Mikel Astiz wrote:
> On Tue, 2012-08-21 at 09:26 +0200, Mikel Astiz wrote:
> > > if the Transfer's Filename matches the temporary file.
> >
> > I'd rather check the object path instead of the filename (of course you
> need to start the transfer first).
>
> And that's where you run into the race condition that I described in my mail
> to the Bluez list: by the time that the SignalWatch is set up, the client might
> already have missed the signals that it wants to receive.
I still don't get the problem. I was proposing to install the watch
with the session prefix, not the full transfer path.
That wasn't clear from the statement above. The prefix match would work
as you suggested.
But then there still is the problem of actually *using* the prefix
match: as I said on the Bluez list, common D-Bus binding (Python, GIO
D-Bus) do not support matching by prefix (or at least not easily - GIO
apparently allows manually setting up match rules).
In SyncEvolution, the src/gdbusxx C++ binding uses GIO D-Bus and
currently only supports a full object path match. It always passes a
path to g_dbus_connection_signal_subscribe(), even if it is empty. To
allow what I suggested earlier (match any path), that probably needs to
be changed so that an empty std::string is translated into NULL for
g_dbus_connection_signal_subscribe().
--
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.
12:59 a.m.
On Tue, Aug 21, 2012 at 12:37 AM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
On Mon, 2012-08-20 at 19:31 -0600, Jeremy Whiting wrote:
> Patrick,
>
> On Fri, Aug 17, 2012 at 5:46 AM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
> > Hello!
> >
> > I've picked up work on the PBAP backend [1] again. The "pbap"
branch can
> > be merged with current master without problems and works as it did
> > before with obexd < 0.47 (see the src/backends/pbap/README):
> > * "refresh-from-remote" works.
> > * The entire phone address book, including PHOTOs, is downloaded
> > in each sync.
> >
> > There are several todos for the code:
> > 1. Adapt to obexd >= 0.47 and the new D-Bus API in it; the current
> > code no longer works.
>
> In order to tackle this task, I've gotten the pbap branch, rebased it
> on master and gotten it to build by changing all the method() calls to
> method.start(). However the guide in the src/backends/pbap/README no
> longer works and gives the following message when I run syncevolution
> --print-items test-config@pbap:
^^^^^^^^^^^
Typo - the name of the config is target-config@pbap, if you followed the
README.
Of course, my bad. Not sure how I messed that up repeatedly, but it's
sorted out now.
> though I did set up test-config@pbap (and it appears in --print-configs) with:
> syncevolution --configure syncURL= addressbook/backend=pbap
> addressbook/database=obex-bt://$MYMAC target-config@pbap addressbook
^^^^^^^^^^^^^
> Also how would you suggest compatibility with obex 0.47 and obex <
> 0.47 work? separate branches? separate code paths in the same branch
> (maybe with a check to see if org.openobex.client or
> org.bluez.obex.client is implemented to know which path to take).
I would do a runtime check. The result of PbapSession::pullAllCb() could
be a pointer into memory (obexd < 0.47) or a memory-mapped file (obexd
>= 0.47). Then the rest of the code then needs to be changed to work
with plain char pointers for the buffer instead of the current const
std::string, but would be the same for both versions.
Ok, when PbapSession is constructed it creates the m_client which
needs to connect to either org.openobex.client or
org.bluez.obex.client, for a runtime check I could try the new
address, then fall back to the old one if the new one fails, right?
If you had some other kind of runtime check let me know. Then I can
store in the PbapSession class which interface to use and if else all
the code that needs to act differently depending on the obex version.
thanks,
Jeremy
> > In SyncEvolution, listening to D-Bus signals is done with the custom
> > GDBusCXX::SignalWatch template class. Documentation of that C++ D-Bus
> > binding is admittedly weak. Grep the source to find examples how it is
> > used and/or let me know if you want better documentation of the C++
> > wrapper and I'll add it.
>
> In order to do this I just need to create a SignalWatch on Transfer
> and copy the file? or get the data from the Transfer object?
You'll have to access the file. The Transfer object only gives you meta
information about the file.
You can set up operations in this order:
1. Create temporary file.
2. Create a SignalWatch for all object paths which updates status
if the Transfer's Filename matches the temporary file.
3. Start the transfer.
4. while ("still running") g_main_context_iteration(NULL, true);
5. mmap file
6. parse it
--
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.
12:15 p.m.
Patrick, all,
Here's my first stab at adding support for obex 0.47 support to the
pbap backend. It does handle CreateSession and calls PullAll passing
in a temporary file based on the mac address of the phone, however I'm
getting an exception in gdbus-cxx-bridge like this:
[ERROR] unexpected exception caught in dbusCallback(): g_variant
failure ./src/gdbusxx/gdbus-cxx-bridge.h:1393
Trace/breakpoint trap
I tried changing the properties type in pullAllCbNew from Params to
DBusArray in case it was not understanding how to convert a std::map
from a dbus a{sv} but that didn't make a difference. Is there
something I'm obviously missing here? I'll look into creating the
transfer signal watcher tomorrow, but if I can't get PullAll's
callback to get called I'm a bit stuck.
thanks,
Jeremy
On Tue, Aug 21, 2012 at 10:59 AM, Jeremy Whiting <jpwhiting(a)kde.org> wrote:
On Tue, Aug 21, 2012 at 12:37 AM, Patrick Ohly
<patrick.ohly(a)intel.com> wrote:
> On Mon, 2012-08-20 at 19:31 -0600, Jeremy Whiting wrote:
>> Patrick,
>>
>> On Fri, Aug 17, 2012 at 5:46 AM, Patrick Ohly <patrick.ohly(a)intel.com>
wrote:
>> > Hello!
>> >
>> > I've picked up work on the PBAP backend [1] again. The "pbap"
branch can
>> > be merged with current master without problems and works as it did
>> > before with obexd < 0.47 (see the src/backends/pbap/README):
>> > * "refresh-from-remote" works.
>> > * The entire phone address book, including PHOTOs, is downloaded
>> > in each sync.
>> >
>> > There are several todos for the code:
>> > 1. Adapt to obexd >= 0.47 and the new D-Bus API in it; the current
>> > code no longer works.
>>
>> In order to tackle this task, I've gotten the pbap branch, rebased it
>> on master and gotten it to build by changing all the method() calls to
>> method.start(). However the guide in the src/backends/pbap/README no
>> longer works and gives the following message when I run syncevolution
>> --print-items test-config@pbap:
> ^^^^^^^^^^^
> Typo - the name of the config is target-config@pbap, if you followed the
> README.
Of course, my bad. Not sure how I messed that up repeatedly, but it's
sorted out now.
>
>> though I did set up test-config@pbap (and it appears in --print-configs) with:
>> syncevolution --configure syncURL= addressbook/backend=pbap
>> addressbook/database=obex-bt://$MYMAC target-config@pbap addressbook
> ^^^^^^^^^^^^^
>
>> Also how would you suggest compatibility with obex 0.47 and obex <
>> 0.47 work? separate branches? separate code paths in the same branch
>> (maybe with a check to see if org.openobex.client or
>> org.bluez.obex.client is implemented to know which path to take).
>
> I would do a runtime check. The result of PbapSession::pullAllCb() could
> be a pointer into memory (obexd < 0.47) or a memory-mapped file (obexd
>>= 0.47). Then the rest of the code then needs to be changed to work
> with plain char pointers for the buffer instead of the current const
> std::string, but would be the same for both versions.
Ok, when PbapSession is constructed it creates the m_client which
needs to connect to either org.openobex.client or
org.bluez.obex.client, for a runtime check I could try the new
address, then fall back to the old one if the new one fails, right?
If you had some other kind of runtime check let me know. Then I can
store in the PbapSession class which interface to use and if else all
the code that needs to act differently depending on the obex version.
thanks,
Jeremy
>
>> > In SyncEvolution, listening to D-Bus signals is done with the custom
>> > GDBusCXX::SignalWatch template class. Documentation of that C++ D-Bus
>> > binding is admittedly weak. Grep the source to find examples how it is
>> > used and/or let me know if you want better documentation of the C++
>> > wrapper and I'll add it.
>>
>> In order to do this I just need to create a SignalWatch on Transfer
>> and copy the file? or get the data from the Transfer object?
>
> You'll have to access the file. The Transfer object only gives you meta
> information about the file.
>
> You can set up operations in this order:
> 1. Create temporary file.
> 2. Create a SignalWatch for all object paths which updates status
> if the Transfer's Filename matches the temporary file.
> 3. Start the transfer.
> 4. while ("still running") g_main_context_iteration(NULL, true);
> 5. mmap file
> 6. parse it
>
> --
> 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.
>
>
6:05 p.m.
On Tue, 2012-08-21 at 22:15 -0600, Jeremy Whiting wrote:
I'm
getting an exception in gdbus-cxx-bridge like this:
[ERROR] unexpected exception caught in dbusCallback(): g_variant
failure ./src/gdbusxx/gdbus-cxx-bridge.h:1393
Trace/breakpoint trap
The error is probably that a value of type "D-Bus object path" is
expected and something else was sent by obexd. You can check by running
with SYNCEVOLUTION_DEBUG=1 G_DBUS_DEBUG=message.
Both CreateSession and PullAll are defined to return an object path. If
obexd returns a string instead, then that's a bug in obexd.
--
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.
6:18 p.m.
On Tue, 2012-08-21 at 22:15 -0600, Jeremy Whiting wrote:
Here's my first stab at adding support for obex 0.47 support to
the
pbap backend.
About the code: SyncEvolution uses RAII. Please avoid plain pointers in
favor of boost or std smart pointers ("+ GDBusCXX::DBusRemoteObject*
m_client;").
+ std::string filename (std::string("/tmp/") + m_address +
".vcf");
Creating temporary files without race conditions and security problems
is a can of worms. Using mkstemp() or tmpfile() is recommended.
In this case, tmpfile() is of not much help because it only gives us a
FILE pointer and thus file descriptor; the man page doesn't say
explicitly, but the file is probably already unlinked.
For mkstemp() you don't get the file name.
Let's do what obexd does and use g_file_open_tmp().
--
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.
5:43 a.m.
Patrick,
Ok, I've taken your last comments into account, and also have a log
with debugging turned on to try to figure out the gvariant error. As
can be seen in the pbaplog.txt PullAll takes a string filename as
argument, and returns an object and an a{sv} of properties, so where
is the gvariant error in the dbuscallback coming from?
On Tue, Aug 21, 2012 at 12:37 AM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
On Mon, 2012-08-20 at 19:31 -0600, Jeremy Whiting wrote:
> Patrick,
>
> On Fri, Aug 17, 2012 at 5:46 AM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
> > Hello!
> >
> > I've picked up work on the PBAP backend [1] again. The "pbap"
branch can
> > be merged with current master without problems and works as it did
> > before with obexd < 0.47 (see the src/backends/pbap/README):
> > * "refresh-from-remote" works.
> > * The entire phone address book, including PHOTOs, is downloaded
> > in each sync.
> >
> > There are several todos for the code:
> > 1. Adapt to obexd >= 0.47 and the new D-Bus API in it; the current
> > code no longer works.
>
> In order to tackle this task, I've gotten the pbap branch, rebased it
> on master and gotten it to build by changing all the method() calls to
> method.start(). However the guide in the src/backends/pbap/README no
> longer works and gives the following message when I run syncevolution
> --print-items test-config@pbap:
^^^^^^^^^^^
Typo - the name of the config is target-config@pbap, if you followed the
README.
> though I did set up test-config@pbap (and it appears in --print-configs) with:
> syncevolution --configure syncURL= addressbook/backend=pbap
> addressbook/database=obex-bt://$MYMAC target-config@pbap addressbook
^^^^^^^^^^^^^
> Also how would you suggest compatibility with obex 0.47 and obex <
> 0.47 work? separate branches? separate code paths in the same branch
> (maybe with a check to see if org.openobex.client or
> org.bluez.obex.client is implemented to know which path to take).
I would do a runtime check. The result of PbapSession::pullAllCb() could
be a pointer into memory (obexd < 0.47) or a memory-mapped file (obexd
>= 0.47). Then the rest of the code then needs to be changed to work
with plain char pointers for the buffer instead of the current const
std::string, but would be the same for both versions.
> > In SyncEvolution, listening to D-Bus signals is done with the custom
> > GDBusCXX::SignalWatch template class. Documentation of that C++ D-Bus
> > binding is admittedly weak. Grep the source to find examples how it is
> > used and/or let me know if you want better documentation of the C++
> > wrapper and I'll add it.
>
> In order to do this I just need to create a SignalWatch on Transfer
> and copy the file? or get the data from the Transfer object?
You'll have to access the file. The Transfer object only gives you meta
information about the file.
You can set up operations in this order:
1. Create temporary file.
I believe I've got this figured out as in the attached patch.
2. Create a SignalWatch for all object paths which updates
status
if the Transfer's Filename matches the temporary file.
3. Start the transfer.
Start the transfer just means call PullAll, correct?
4. while ("still running")
g_main_context_iteration(NULL, true);
This is a loop watching a value of the Transfer object for completed
or error, right? Why not just store the fd, connect to the transfer
completed and error signals and let the callbacks for those signals do
the next steps?
thanks,
Jeremy
5. mmap file
6. parse it
--
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.
2:31 p.m.
On Wed, 2012-08-22 at 15:43 -0600, Jeremy Whiting wrote:
Ok, I've taken your last comments into account, and also have a
log
with debugging turned on to try to figure out the gvariant error. As
can be seen in the pbaplog.txt PullAll takes a string filename as
argument, and returns an object and an a{sv} of properties, so where
is the gvariant error in the dbuscallback coming from?
I'm out of ideas. The next step will have to be to debug what the
content of that GVariant really is and figure out how the code arrived
at that point.
We know the source file and line, so I would run under gdb and set a
breakpoint. If you don't beat me to it, then I'll try it myself today.
--
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.
11:26 p.m.
On Wed, 2012-08-22 at 15:43 -0600, Jeremy Whiting wrote:
Patrick,
Ok, I've taken your last comments into account, and also have a log
with debugging turned on to try to figure out the gvariant error. As
can be seen in the pbaplog.txt PullAll takes a string filename as
argument, and returns an object and an a{sv} of properties, so where
is the gvariant error in the dbuscallback coming from?
Got it. What it returns is not a sequence of object path followed by the
dict, which would be "oa{sv}". Instead it returns a tuple containing
those two values: "(oa{sv})".
That difference is probably irrelevant in a Python client which will
just convert whatever it gets and then store that in a loosely typed
variable, which happens to be a tuple in both cases.
The demarshal code for GDBusCXX::DBusClientCall is a bit more picky and
expects something which matches exactly what was declared.
The following code works:
GDBusCXX::DBusClientCall1< std::pair<GDBusCXX::DBusObject_t, Params> >
pullall(*m_session, "PullAll");
std::pair<GDBusCXX::DBusObject_t, Params> transfer =
pullall(std::string("/tmp/temp.vcard"));
const GDBusCXX::DBusObject_t &path = transfer.first;
const Params &properties = transfer.second;
SE_LOG_DEBUG(NULL, NULL, "pullall transfer path %s, %ld properties",
path.c_str(), (long)properties.size());
You patch didn't apply against the current pbap branch, so I hacked the
current code myself. Diff is attached.
Note that receiving a dict like that can fail: Params is only defined
such that the content of the boost::variant can be a std::string. If
obexd ever returns a different kind of value in its property dict, than
that value will cause a similar demarshalling error.
Instead we need a boost::variant<std::string, GDBusCXX::DBusObject_t,
int, bool, uint8_t, int16_t, uint16_t, int32_t, uint32_t, int64_t,
uint64_t> as value for the map.
This will require some cut-and-paste work to extend
src/gdbusxx/gdbus-cxx-bridge.h such that "dbus_traits <boost::variant"
is available for up to 11 different types in the boost::variant.
One more comment about your patch: is there a reason for switching to
the asynchronous method call approach? It's harder to read and has
issues that need to be dealt with, like no longer having a valid "this"
pointer when the call completes (which it will do, even if the calling
instance is long gone!).
If you want to go down that road, bind a boost::weak_ptr for the
instance to be on the safe side instead of the plain pointer. Then
attempts to call the bound method will first lock the pointer and do
nothing when that is no longer possible.
--
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.
6:39 a.m.
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. Currently it starts the transfer and does work
(I see vcard data in the file) however as you can probably see my
SignalWatch understanding is not that great. I thought I could watch
the transfer path, but apparently SignalWatch only works on a remote
dbus object, not an object and path. I didn't quite follow what you
were saying earlier about putting a watch on everything, how would
that work (the signal is on the transfer path only, not the whole obex
client object)?
thanks,
Jeremy
On Thu, Aug 23, 2012 at 9:26 AM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
On Wed, 2012-08-22 at 15:43 -0600, Jeremy Whiting wrote:
> Patrick,
>
> Ok, I've taken your last comments into account, and also have a log
> with debugging turned on to try to figure out the gvariant error. As
> can be seen in the pbaplog.txt PullAll takes a string filename as
> argument, and returns an object and an a{sv} of properties, so where
> is the gvariant error in the dbuscallback coming from?
Got it. What it returns is not a sequence of object path followed by the
dict, which would be "oa{sv}". Instead it returns a tuple containing
those two values: "(oa{sv})".
That difference is probably irrelevant in a Python client which will
just convert whatever it gets and then store that in a loosely typed
variable, which happens to be a tuple in both cases.
The demarshal code for GDBusCXX::DBusClientCall is a bit more picky and
expects something which matches exactly what was declared.
The following code works:
GDBusCXX::DBusClientCall1< std::pair<GDBusCXX::DBusObject_t, Params> >
pullall(*m_session, "PullAll");
std::pair<GDBusCXX::DBusObject_t, Params> transfer =
pullall(std::string("/tmp/temp.vcard"));
const GDBusCXX::DBusObject_t &path = transfer.first;
const Params &properties = transfer.second;
SE_LOG_DEBUG(NULL, NULL, "pullall transfer path %s, %ld properties",
path.c_str(), (long)properties.size());
You patch didn't apply against the current pbap branch, so I hacked the
current code myself. Diff is attached.
Note that receiving a dict like that can fail: Params is only defined
such that the content of the boost::variant can be a std::string. If
obexd ever returns a different kind of value in its property dict, than
that value will cause a similar demarshalling error.
Instead we need a boost::variant<std::string, GDBusCXX::DBusObject_t,
int, bool, uint8_t, int16_t, uint16_t, int32_t, uint32_t, int64_t,
uint64_t> as value for the map.
This will require some cut-and-paste work to extend
src/gdbusxx/gdbus-cxx-bridge.h such that "dbus_traits <boost::variant"
is available for up to 11 different types in the boost::variant.
One more comment about your patch: is there a reason for switching to
the asynchronous method call approach? It's harder to read and has
issues that need to be dealt with, like no longer having a valid "this"
pointer when the call completes (which it will do, even if the calling
instance is long gone!).
If you want to go down that road, bind a boost::weak_ptr for the
instance to be on the safe side instead of the plain pointer. Then
attempts to call the bound method will first lock the pointer and do
nothing when that is no longer possible.
--
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.
11:37 p.m.
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.
Currently it starts the transfer and does work
(I see vcard data in the file) however as you can probably see my
SignalWatch understanding is not that great. I thought I could watch
the transfer path, but apparently SignalWatch only works on a remote
dbus object, not an object and path.
SignalWatch takes a DBusRemoteObject, which contains path, interface and
sender (in this case incorrectly called "destination").
I didn't quite follow what you
were saying earlier about putting a watch on everything, how would
that work (the signal is on the transfer path only, not the whole obex
client object)?
The signal uses a path prefix which is the same as that of the session.
Therefore, if there was a way to select a D-Bus prefix match, that
session path could be used to restrict signal processing to signals of
the current session.
But neither GIO D-Bus (at least out of the box) nor the C++ binding for
it support a prefix match. I suggest that you leave your code as it is
for now.
--
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.
11:20 p.m.
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.ohly(a)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.ohly(a)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.ohly(a)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.ohly(a)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.ohly(a)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.ohly(a)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.
6:51 a.m.
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 <patrick.ohly(a)intel.com> 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 <patrick.ohly(a)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.ohly(a)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.ohly(a)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.ohly(a)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.ohly(a)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.ohly(a)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.
2:29 p.m.
On Mon, 2012-08-27 at 16:51 -0600, Jeremy Whiting wrote:
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
[...]
[DEBUG 00:00:04] AddMatch failed:
GDBus.Error:org.freedesktop.DBus.Error.MatchRuleInvalid: Unknown key
"path_namespace" in match rule
[...]
Do I need to build with some flag set or something so path_namespace
will be a known key?
The path_namespace key was implemented in D-Bus >= 1.5.0. What is your
version of the running daemon? What is your distro?
We could simply not use the prefix filter in the match rule and instead
filter on the client side - not very efficient when there are many
unrelated transfers. I'm also concerned about class B removing a match
rule which class A still depends on. To be fair, its unlikely in
SyncEvolution, because the backend runs in a process by itself that
(currently) doesn't do anything other than PBAP.
Either way, some kind of advance version check or error handling with
fallback to match rule without path_namespace would have to be added.
--
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.
5:13 a.m.
Patrick,
Ok, I've updated my syncpbap branch here:
http://cgit.collabora.com/git/user/jwhiting/syncevolution.git but the
completeCb and errorCb both never get called, so it just sits there.
The transfer does happen, as the file is created and vcard data pumped
in, but something is not right with the signalwatch. Is there a way
I can check that the signalwatch is watching the right thing?
I've attached a log of obexd-client when running syncevolution. and
the transfer does get the sub path of
/org/bluez/obex/session0/transfer0 so the signalwatch path should be
matching. Also the complete signal is getting sent according to that
log.
Btw, feel free to offer any feedback on my use of StringPiece, I think
what I have should work, but I've never used it before, so we'll see.
thanks,
Jeremy
On Tue, Aug 28, 2012 at 12:29 AM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
On Mon, 2012-08-27 at 16:51 -0600, Jeremy Whiting wrote:
> 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
[...]
> [DEBUG 00:00:04] AddMatch failed:
> GDBus.Error:org.freedesktop.DBus.Error.MatchRuleInvalid: Unknown key
> "path_namespace" in match rule
[...]
> Do I need to build with some flag set or something so path_namespace
> will be a known key?
The path_namespace key was implemented in D-Bus >= 1.5.0. What is your
version of the running daemon? What is your distro?
We could simply not use the prefix filter in the match rule and instead
filter on the client side - not very efficient when there are many
unrelated transfers. I'm also concerned about class B removing a match
rule which class A still depends on. To be fair, its unlikely in
SyncEvolution, because the backend runs in a process by itself that
(currently) doesn't do anything other than PBAP.
Either way, some kind of advance version check or error handling with
fallback to match rule without path_namespace would have to be added.
--
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.
2:32 p.m.
On Wed, 2012-08-29 at 15:13 -0600, Jeremy Whiting wrote:
Ok, I've updated my syncpbap branch here:
http://cgit.collabora.com/git/user/jwhiting/syncevolution.git but the
completeCb and errorCb both never get called, so it just sits there.
The transfer does happen, as the file is created and vcard data pumped
in, but something is not right with the signalwatch.
Are you now running with a dbus daemon >= 1.5.0? I'm asking because I
don't see any changes in your branch to make the path prefix filtering
work with the older dbus that you were running.
Is there a way
I can check that the signalwatch is watching the right thing?
You can run with SYNCEVOLUTION_DEBUG=1 G_DBUS_DEBUG=message and
--daemon=no --print-items to see the D-Bus messages that SyncEvolution
sends and receives. Watch out for Add/RemoveMatch to see what kind of
filter it installs on the D-Bus daemon.
Additional filtering then happens in SignalFilter::matches() inside
SyncEvolution.
Btw, feel free to offer any feedback on my use of StringPiece, I
think
what I have should work, but I've never used it before, so we'll see.
+ pcrecpp::StringPiece content(addr);
Still assumes that memory-mapped "addr" is 0-terminated (how else would
StringPiece know the length?). Use content(addr, sb.st_size).
+ typedef std::map<std::string, int> CounterMap;
+ CounterMap counterMap;
Any time you instantiate a std::string with data from "content", string
data has to be copied.
It would be better to let the Session object own the memory-mapped
content in a StringPiece, then use StringPiece everywhere else. It can
be used in standard containers.
+ string vcarddata;
Should be StringPiece.
+ pcrecpp::RE re("(BEGIN:VCARD.*END:VCARD)");
I doubt that this will match (doesn't match line breaks) and if it did,
it would match all vcards. Doesn't check that START/END occur at the
start of a line (will therefore fail when it is embedded as comment, for
example).
pcrecpp::RE re("(^BEGIN:VCARD.*?^END:VCARD)",
RE_Options().set_dotall(true));
+ while (re.Consume(&content, &vcarddata)) {
+ typedef std::map<std::string, std::string> VcardMap;
+ VcardMap vcard;
+ vcardParse(vcarddata, 0, vcarddata.length(), vcard);
The whole vcardParse can be removed for now. I left it enabled in "PBAP:
don't try to make up stable local IDs", but there's no need for it at
the moment.
+ VcardMap::const_iterator it = vcard.find("N");
+ if(it != vcard.end() && !it->second.empty()) {
+ const std::string &fn = it->second;
+
+ const std::pair<CounterMap::iterator, bool> &r =
+ counterMap.insert(CounterMap::value_type(fn, 0));
+ if(!r.second) {
+ r.first->second ++;
+ }
+
+ char suffix[8];
+ sprintf(suffix, "%07d", r.first->second);
+
+ std::string id = fn + std::string(suffix);
+ dst[id] = vcarddata;
That reverts "PBAP: don't try to make up stable local IDs".
This parsing code now exists in two variants: once for new API, once for
the old. You can use the StringPiece-based version for both by storing
the std::string in the Session object and then using a StringPiece which
references that in-memory data.
--
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.
5:03 p.m.
On Thu, 2012-08-30 at 08:32 +0200, Patrick Ohly wrote:
+ pcrecpp::RE re("(BEGIN:VCARD.*END:VCARD)");
I doubt that this will match (doesn't match line breaks) and if it did,
it would match all vcards. Doesn't check that START/END occur at the
start of a line (will therefore fail when it is embedded as comment, for
example).
pcrecpp::RE re("(^BEGIN:VCARD.*?^END:VCARD)",
RE_Options().set_dotall(true));
I meant to make this a multiline pattern (needed for the "beginning of
line criteria") but forgot. Should be:
pcrecpp::RE re("(^BEGIN:VCARD.*?^END:VCARD)",
RE_Options().set_dotall(true).
set_multiline(true));
--
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.
3:37 a.m.
On Thu, Aug 30, 2012 at 12:32 AM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
On Wed, 2012-08-29 at 15:13 -0600, Jeremy Whiting wrote:
> Ok, I've updated my syncpbap branch here:
> http://cgit.collabora.com/git/user/jwhiting/syncevolution.git but the
> completeCb and errorCb both never get called, so it just sits there.
> The transfer does happen, as the file is created and vcard data pumped
> in, but something is not right with the signalwatch.
Are you now running with a dbus daemon >= 1.5.0? I'm asking because I
don't see any changes in your branch to make the path prefix filtering
work with the older dbus that you were running.
Yes, I've upgraded dbus to version 1.6.4 now.
> Is there a way
> I can check that the signalwatch is watching the right thing?
You can run with SYNCEVOLUTION_DEBUG=1 G_DBUS_DEBUG=message and
--daemon=no --print-items to see the D-Bus messages that SyncEvolution
sends and receives. Watch out for Add/RemoveMatch to see what kind of
filter it installs on the D-Bus daemon.
Ok, I've attached the log here, it looks to me that we are getting
notified of the Complete (and the Progress signals also) but we don't
seem to be doing anything with them. The completeCb is never called,
as you don't see "obexd transfer" debug messages in the log anywhere.
Is there a way to check if the boost::bind on those callbacks is
working? I don't see any warnings or error messages at runtime
either, but maybe boost supresses those or something?
Additional filtering then happens in SignalFilter::matches() inside
SyncEvolution.
> Btw, feel free to offer any feedback on my use of StringPiece, I think
> what I have should work, but I've never used it before, so we'll see.
+ pcrecpp::StringPiece content(addr);
Still assumes that memory-mapped "addr" is 0-terminated (how else would
StringPiece know the length?). Use content(addr, sb.st_size).
+ typedef std::map<std::string, int> CounterMap;
+ CounterMap counterMap;
Any time you instantiate a std::string with data from "content", string
data has to be copied.
It would be better to let the Session object own the memory-mapped
content in a StringPiece, then use StringPiece everywhere else. It can
be used in standard containers.
Ok, since the vcards no longer need to be parsed to get the id, I
removed the counterMap altogether.
+ string vcarddata;
Should be StringPiece.
If I change that to pcrecpp::StringPiece vcarddata it no longer
builds, since Consume expects it's second argument to be a
std::string.
btw, the boost version here I'm testing with is 1.46.1, that shouldn't
matter, should it?
I believe I've updated according to your other feedback (and force
pushed to my usual syncpbap branch). Let me know if you have any ideas
how I can track down the completeCb never getting called.
thanks,
Jeremy
+ pcrecpp::RE re("(BEGIN:VCARD.*END:VCARD)");
I doubt that this will match (doesn't match line breaks) and if it did,
it would match all vcards. Doesn't check that START/END occur at the
start of a line (will therefore fail when it is embedded as comment, for
example).
pcrecpp::RE re("(^BEGIN:VCARD.*?^END:VCARD)",
RE_Options().set_dotall(true));
+ while (re.Consume(&content, &vcarddata)) {
+ typedef std::map<std::string, std::string> VcardMap;
+ VcardMap vcard;
+ vcardParse(vcarddata, 0, vcarddata.length(), vcard);
The whole vcardParse can be removed for now. I left it enabled in "PBAP:
don't try to make up stable local IDs", but there's no need for it at
the moment.
+ VcardMap::const_iterator it = vcard.find("N");
+ if(it != vcard.end() && !it->second.empty()) {
+ const std::string &fn = it->second;
+
+ const std::pair<CounterMap::iterator, bool> &r =
+ counterMap.insert(CounterMap::value_type(fn, 0));
+ if(!r.second) {
+ r.first->second ++;
+ }
+
+ char suffix[8];
+ sprintf(suffix, "%07d", r.first->second);
+
+ std::string id = fn + std::string(suffix);
+ dst[id] = vcarddata;
That reverts "PBAP: don't try to make up stable local IDs".
This parsing code now exists in two variants: once for new API, once for
the old. You can use the StringPiece-based version for both by storing
the std::string in the Session object and then using a StringPiece which
references that in-memory data.
--
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.
10:59 p.m.
On Thu, 2012-08-30 at 13:37 -0600, Jeremy Whiting wrote:
On Thu, Aug 30, 2012 at 12:32 AM, Patrick Ohly
<patrick.ohly(a)intel.com> wrote:
> On Wed, 2012-08-29 at 15:13 -0600, Jeremy Whiting wrote:
>> Ok, I've updated my syncpbap branch here:
>> http://cgit.collabora.com/git/user/jwhiting/syncevolution.git but the
>> completeCb and errorCb both never get called, so it just sits there.
>> The transfer does happen, as the file is created and vcard data pumped
>> in, but something is not right with the signalwatch.
>
> Are you now running with a dbus daemon >= 1.5.0? I'm asking because I
> don't see any changes in your branch to make the path prefix filtering
> work with the older dbus that you were running.
Yes, I've upgraded dbus to version 1.6.4 now.
Then let's hope that all users with obexd > 0.47 in their distro also
have a recent enough dbus and not bother about adding the fallback code.
Ok, I've attached the log here, it looks to me that we are
getting
notified of the Complete (and the Progress signals also) but we don't
seem to be doing anything with them. The completeCb is never called,
as you don't see "obexd transfer" debug messages in the log anywhere.
Is there a way to check if the boost::bind on those callbacks is
working? I don't see any warnings or error messages at runtime
either, but maybe boost supresses those or something?
It already failed in SignalFilter::matches(). I had a redundant m_path
check in it - sorry for that. I could have sworn that I tested that
version of the code before pushing, but obviously I only tested the
version without that check.
I also noticed another bug and had to fix a compiler warning in your
code. All changes are in jwhiting-pbap:
commit 0e6428cdae8e5c81d13d8d0b9d66c18921f0301c
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Fri Aug 31 16:33:36 2012 +0200
PBAP: fixed compiler warning
%d != long
diff --git a/src/backends/pbap/PbapSyncSource.cpp b/src/backends/pbap/PbapSyncSource.cpp
index 61299f4..b854633 100644
--- a/src/backends/pbap/PbapSyncSource.cpp
+++ b/src/backends/pbap/PbapSyncSource.cpp
@@ -270,7 +270,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",
(long)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) {
commit 24f7d177f411ebb23dfdabbc802459db57aef5bf
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Fri Aug 31 16:31:52 2012 +0200
GDBus GIO: fix SignalFilter
The signal name was not stored, making the actual filtering more
relaxed than it should have been. Worse, the path was redundantly
checked for a complete match even when SIGNAL_FILTER_PATH_PREFIX was
set, thus preventing signal deliver in that case.
diff --git a/src/gdbusxx/gdbus-cxx-bridge.h b/src/gdbusxx/gdbus-cxx-bridge.h
index 40fba39..05c53c7 100644
--- a/src/gdbusxx/gdbus-cxx-bridge.h
+++ b/src/gdbusxx/gdbus-cxx-bridge.h
@@ -4686,6 +4686,7 @@ class SignalFilter : public DBusRemoteObject
const std::string &signal,
Flags flags) :
DBusRemoteObject(conn, path, interface, ""),
+ m_signal(signal),
m_flags(flags)
{}
@@ -4701,7 +4702,6 @@ class SignalFilter : public DBusRemoteObject
bool matches(const ExtractArgs &context) const
{
return
- (m_path.empty() || m_path == context.m_path) &&
(m_interface.empty() || m_interface == context.m_interface) &&
(m_signal.empty() || m_signal == context.m_signal) &&
(m_path.empty() ||
>
> + string vcarddata;
>
> Should be StringPiece.
If I change that to pcrecpp::StringPiece vcarddata it no longer
builds, since Consume expects it's second argument to be a
std::string.
Huh? The second argument is a result variable for the first bracket.
Passing the address of a StringPiece should be fine. Works fine here
with pcre3 8.30 (Debian).
With the fix for GDBus C++, the --print-items operation completes, but
parsing the result fails. I only get the first contact although the
temporary transfer file had more data. At first glance it looks like
Consume() is unhappy once it has leading \n\r in the content StringPiece
(not matched by the regex anywhere)... yes, that's it.
With this change here it works for me:
diff --git a/src/backends/pbap/PbapSyncSource.cpp b/src/backends/pbap/PbapSyncSource.cpp
index b854633..d39dbec 100644
--- a/src/backends/pbap/PbapSyncSource.cpp
+++ b/src/backends/pbap/PbapSyncSource.cpp
@@ -280,13 +280,13 @@ void PbapSession::pullAll(Content &dst)
pcrecpp::StringPiece content(addr, sb.st_size);
- string vcarddata;
+ pcrecpp::StringPiece vcarddata;
int count = 0;
- pcrecpp::RE re("(^BEGIN:VCARD.*?^END:VCARD)",
+ pcrecpp::RE re("[\\r\\n]*(^BEGIN:VCARD.*?^END:VCARD)",
pcrecpp::RE_Options().set_dotall(true).set_multiline(true));
while (re.Consume(&content, &vcarddata)) {
std::string id = StringPrintf("%d", count);
- dst[id] = vcarddata;
+ dst[id] = vcarddata.as_string();
++count;
}
--
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.
12:24 a.m.
Patrick,
Thanks for the fix. Now --print-items is working, though it's only
printing a bunch of id numbers between 0 and 110 (probably how many
contacts I have on my phone at the moment). When I attempt the next
step from the README file I get an error because gnome-keyring is not
running. What does syncevolution need the keyring for? Anyway, now
that it's mostly working I've got a question about usability.
Our usecase is when a phone is paired with the device by bluetooth we
would like to sync the contacts into an eds addressbook named by the
mac address of the phone (or some other unique identifier, but mac
address seems to be fine so far). The steps in the README seem to set
up the sync to put contacts into addressbook, is there a way to
specify which addressbook to sync into I hope?
thanks,
Jeremy
On Fri, Aug 31, 2012 at 8:59 AM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
On Thu, 2012-08-30 at 13:37 -0600, Jeremy Whiting wrote:
> On Thu, Aug 30, 2012 at 12:32 AM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
> > On Wed, 2012-08-29 at 15:13 -0600, Jeremy Whiting wrote:
> >> Ok, I've updated my syncpbap branch here:
> >> http://cgit.collabora.com/git/user/jwhiting/syncevolution.git but the
> >> completeCb and errorCb both never get called, so it just sits there.
> >> The transfer does happen, as the file is created and vcard data pumped
> >> in, but something is not right with the signalwatch.
> >
> > Are you now running with a dbus daemon >= 1.5.0? I'm asking because I
> > don't see any changes in your branch to make the path prefix filtering
> > work with the older dbus that you were running.
>
> Yes, I've upgraded dbus to version 1.6.4 now.
Then let's hope that all users with obexd > 0.47 in their distro also
have a recent enough dbus and not bother about adding the fallback code.
> Ok, I've attached the log here, it looks to me that we are getting
> notified of the Complete (and the Progress signals also) but we don't
> seem to be doing anything with them. The completeCb is never called,
> as you don't see "obexd transfer" debug messages in the log anywhere.
> Is there a way to check if the boost::bind on those callbacks is
> working? I don't see any warnings or error messages at runtime
> either, but maybe boost supresses those or something?
It already failed in SignalFilter::matches(). I had a redundant m_path
check in it - sorry for that. I could have sworn that I tested that
version of the code before pushing, but obviously I only tested the
version without that check.
I also noticed another bug and had to fix a compiler warning in your
code. All changes are in jwhiting-pbap:
commit 0e6428cdae8e5c81d13d8d0b9d66c18921f0301c
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Fri Aug 31 16:33:36 2012 +0200
PBAP: fixed compiler warning
%d != long
diff --git a/src/backends/pbap/PbapSyncSource.cpp b/src/backends/pbap/PbapSyncSource.cpp
index 61299f4..b854633 100644
--- a/src/backends/pbap/PbapSyncSource.cpp
+++ b/src/backends/pbap/PbapSyncSource.cpp
@@ -270,7 +270,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",
(long)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) {
commit 24f7d177f411ebb23dfdabbc802459db57aef5bf
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Fri Aug 31 16:31:52 2012 +0200
GDBus GIO: fix SignalFilter
The signal name was not stored, making the actual filtering more
relaxed than it should have been. Worse, the path was redundantly
checked for a complete match even when SIGNAL_FILTER_PATH_PREFIX was
set, thus preventing signal deliver in that case.
diff --git a/src/gdbusxx/gdbus-cxx-bridge.h b/src/gdbusxx/gdbus-cxx-bridge.h
index 40fba39..05c53c7 100644
--- a/src/gdbusxx/gdbus-cxx-bridge.h
+++ b/src/gdbusxx/gdbus-cxx-bridge.h
@@ -4686,6 +4686,7 @@ class SignalFilter : public DBusRemoteObject
const std::string &signal,
Flags flags) :
DBusRemoteObject(conn, path, interface, ""),
+ m_signal(signal),
m_flags(flags)
{}
@@ -4701,7 +4702,6 @@ class SignalFilter : public DBusRemoteObject
bool matches(const ExtractArgs &context) const
{
return
- (m_path.empty() || m_path == context.m_path) &&
(m_interface.empty() || m_interface == context.m_interface) &&
(m_signal.empty() || m_signal == context.m_signal) &&
(m_path.empty() ||
> >
> > + string vcarddata;
> >
> > Should be StringPiece.
>
> If I change that to pcrecpp::StringPiece vcarddata it no longer
> builds, since Consume expects it's second argument to be a
> std::string.
Huh? The second argument is a result variable for the first bracket.
Passing the address of a StringPiece should be fine. Works fine here
with pcre3 8.30 (Debian).
With the fix for GDBus C++, the --print-items operation completes, but
parsing the result fails. I only get the first contact although the
temporary transfer file had more data. At first glance it looks like
Consume() is unhappy once it has leading \n\r in the content StringPiece
(not matched by the regex anywhere)... yes, that's it.
With this change here it works for me:
diff --git a/src/backends/pbap/PbapSyncSource.cpp b/src/backends/pbap/PbapSyncSource.cpp
index b854633..d39dbec 100644
--- a/src/backends/pbap/PbapSyncSource.cpp
+++ b/src/backends/pbap/PbapSyncSource.cpp
@@ -280,13 +280,13 @@ void PbapSession::pullAll(Content &dst)
pcrecpp::StringPiece content(addr, sb.st_size);
- string vcarddata;
+ pcrecpp::StringPiece vcarddata;
int count = 0;
- pcrecpp::RE re("(^BEGIN:VCARD.*?^END:VCARD)",
+ pcrecpp::RE re("[\\r\\n]*(^BEGIN:VCARD.*?^END:VCARD)",
pcrecpp::RE_Options().set_dotall(true).set_multiline(true));
while (re.Consume(&content, &vcarddata)) {
std::string id = StringPrintf("%d", count);
- dst[id] = vcarddata;
+ dst[id] = vcarddata.as_string();
++count;
}
--
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.
12:25 a.m.
BTW, I forgot to mention, I force pushed my branch to the syncpbap
branch at my usual repo.
Jeremy
On Fri, Aug 31, 2012 at 10:24 AM, Jeremy Whiting <jpwhiting(a)kde.org> wrote:
Patrick,
Thanks for the fix. Now --print-items is working, though it's only
printing a bunch of id numbers between 0 and 110 (probably how many
contacts I have on my phone at the moment). When I attempt the next
step from the README file I get an error because gnome-keyring is not
running. What does syncevolution need the keyring for? Anyway, now
that it's mostly working I've got a question about usability.
Our usecase is when a phone is paired with the device by bluetooth we
would like to sync the contacts into an eds addressbook named by the
mac address of the phone (or some other unique identifier, but mac
address seems to be fine so far). The steps in the README seem to set
up the sync to put contacts into addressbook, is there a way to
specify which addressbook to sync into I hope?
thanks,
Jeremy
On Fri, Aug 31, 2012 at 8:59 AM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
> On Thu, 2012-08-30 at 13:37 -0600, Jeremy Whiting wrote:
>> On Thu, Aug 30, 2012 at 12:32 AM, Patrick Ohly <patrick.ohly(a)intel.com>
wrote:
>> > On Wed, 2012-08-29 at 15:13 -0600, Jeremy Whiting wrote:
>> >> Ok, I've updated my syncpbap branch here:
>> >> http://cgit.collabora.com/git/user/jwhiting/syncevolution.git but the
>> >> completeCb and errorCb both never get called, so it just sits there.
>> >> The transfer does happen, as the file is created and vcard data pumped
>> >> in, but something is not right with the signalwatch.
>> >
>> > Are you now running with a dbus daemon >= 1.5.0? I'm asking because
I
>> > don't see any changes in your branch to make the path prefix filtering
>> > work with the older dbus that you were running.
>>
>> Yes, I've upgraded dbus to version 1.6.4 now.
>
> Then let's hope that all users with obexd > 0.47 in their distro also
> have a recent enough dbus and not bother about adding the fallback code.
>
>> Ok, I've attached the log here, it looks to me that we are getting
>> notified of the Complete (and the Progress signals also) but we don't
>> seem to be doing anything with them. The completeCb is never called,
>> as you don't see "obexd transfer" debug messages in the log
anywhere.
>> Is there a way to check if the boost::bind on those callbacks is
>> working? I don't see any warnings or error messages at runtime
>> either, but maybe boost supresses those or something?
>
> It already failed in SignalFilter::matches(). I had a redundant m_path
> check in it - sorry for that. I could have sworn that I tested that
> version of the code before pushing, but obviously I only tested the
> version without that check.
>
> I also noticed another bug and had to fix a compiler warning in your
> code. All changes are in jwhiting-pbap:
>
> commit 0e6428cdae8e5c81d13d8d0b9d66c18921f0301c
> Author: Patrick Ohly <patrick.ohly(a)intel.com>
> Date: Fri Aug 31 16:33:36 2012 +0200
>
> PBAP: fixed compiler warning
>
> %d != long
>
> diff --git a/src/backends/pbap/PbapSyncSource.cpp
b/src/backends/pbap/PbapSyncSource.cpp
> index 61299f4..b854633 100644
> --- a/src/backends/pbap/PbapSyncSource.cpp
> +++ b/src/backends/pbap/PbapSyncSource.cpp
> @@ -270,7 +270,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",
(long)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) {
>
> commit 24f7d177f411ebb23dfdabbc802459db57aef5bf
> Author: Patrick Ohly <patrick.ohly(a)intel.com>
> Date: Fri Aug 31 16:31:52 2012 +0200
>
> GDBus GIO: fix SignalFilter
>
> The signal name was not stored, making the actual filtering more
> relaxed than it should have been. Worse, the path was redundantly
> checked for a complete match even when SIGNAL_FILTER_PATH_PREFIX was
> set, thus preventing signal deliver in that case.
>
> diff --git a/src/gdbusxx/gdbus-cxx-bridge.h b/src/gdbusxx/gdbus-cxx-bridge.h
> index 40fba39..05c53c7 100644
> --- a/src/gdbusxx/gdbus-cxx-bridge.h
> +++ b/src/gdbusxx/gdbus-cxx-bridge.h
> @@ -4686,6 +4686,7 @@ class SignalFilter : public DBusRemoteObject
> const std::string &signal,
> Flags flags) :
> DBusRemoteObject(conn, path, interface, ""),
> + m_signal(signal),
> m_flags(flags)
> {}
>
> @@ -4701,7 +4702,6 @@ class SignalFilter : public DBusRemoteObject
> bool matches(const ExtractArgs &context) const
> {
> return
> - (m_path.empty() || m_path == context.m_path) &&
> (m_interface.empty() || m_interface == context.m_interface) &&
> (m_signal.empty() || m_signal == context.m_signal) &&
> (m_path.empty() ||
>
>
>> >
>> > + string vcarddata;
>> >
>> > Should be StringPiece.
>>
>> If I change that to pcrecpp::StringPiece vcarddata it no longer
>> builds, since Consume expects it's second argument to be a
>> std::string.
>
> Huh? The second argument is a result variable for the first bracket.
> Passing the address of a StringPiece should be fine. Works fine here
> with pcre3 8.30 (Debian).
>
> With the fix for GDBus C++, the --print-items operation completes, but
> parsing the result fails. I only get the first contact although the
> temporary transfer file had more data. At first glance it looks like
> Consume() is unhappy once it has leading \n\r in the content StringPiece
> (not matched by the regex anywhere)... yes, that's it.
>
> With this change here it works for me:
>
> diff --git a/src/backends/pbap/PbapSyncSource.cpp
b/src/backends/pbap/PbapSyncSource.cpp
> index b854633..d39dbec 100644
> --- a/src/backends/pbap/PbapSyncSource.cpp
> +++ b/src/backends/pbap/PbapSyncSource.cpp
> @@ -280,13 +280,13 @@ void PbapSession::pullAll(Content &dst)
>
> pcrecpp::StringPiece content(addr, sb.st_size);
>
> - string vcarddata;
> + pcrecpp::StringPiece vcarddata;
> int count = 0;
> - pcrecpp::RE re("(^BEGIN:VCARD.*?^END:VCARD)",
> + pcrecpp::RE re("[\\r\\n]*(^BEGIN:VCARD.*?^END:VCARD)",
> pcrecpp::RE_Options().set_dotall(true).set_multiline(true));
> while (re.Consume(&content, &vcarddata)) {
> std::string id = StringPrintf("%d", count);
> - dst[id] = vcarddata;
> + dst[id] = vcarddata.as_string();
>
> ++count;
> }
>
>
> --
> 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.
>
>
2:58 a.m.
On Fri, 2012-08-31 at 10:24 -0600, Jeremy Whiting wrote:
Patrick,
Thanks for the fix. Now --print-items is working, though it's only
printing a bunch of id numbers between 0 and 110 (probably how many
contacts I have on my phone at the moment). When I attempt the next
step from the README file I get an error because gnome-keyring is not
running. What does syncevolution need the keyring for?
The "password" property values are stored in it. You can control the
usage of the keyring with the "keyring" property (disable, use KWallet
instead, ...). Or compile SyncEvolution without GNOME keyring support in
the first place.
Our usecase is when a phone is paired with the device by bluetooth
we
would like to sync the contacts into an eds addressbook named by the
mac address of the phone (or some other unique identifier, but mac
address seems to be fine so far). The steps in the README seem to set
up the sync to put contacts into addressbook, is there a way to
specify which addressbook to sync into I hope?
syncevolution --configure database=<name or UUID> @default addressbook
Creating and deleting that address book is currently out of scope for
SyncEvolution. At some point it was possible to have it create an
address book by using a file:///<file name> URI as database value, but
accessing files like that was removed in EDS a while back.
--
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.
9:41 a.m.
Patrick,
Ok, I am able to create addressbooks in the same way I do for folks
eds backend unit tests by copying an ini file. The last snag I'm
hitting is that syncevo-local-sync is missing in our packages. Is
there some configure line that builds that? I don't see it in
/usr/local/lib/syncevolution in my build from git either.
thanks,
Jeremy
On Fri, Aug 31, 2012 at 12:58 PM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
On Fri, 2012-08-31 at 10:24 -0600, Jeremy Whiting wrote:
> Patrick,
>
> Thanks for the fix. Now --print-items is working, though it's only
> printing a bunch of id numbers between 0 and 110 (probably how many
> contacts I have on my phone at the moment). When I attempt the next
> step from the README file I get an error because gnome-keyring is not
> running. What does syncevolution need the keyring for?
The "password" property values are stored in it. You can control the
usage of the keyring with the "keyring" property (disable, use KWallet
instead, ...). Or compile SyncEvolution without GNOME keyring support in
the first place.
> Our usecase is when a phone is paired with the device by bluetooth we
> would like to sync the contacts into an eds addressbook named by the
> mac address of the phone (or some other unique identifier, but mac
> address seems to be fine so far). The steps in the README seem to set
> up the sync to put contacts into addressbook, is there a way to
> specify which addressbook to sync into I hope?
syncevolution --configure database=<name or UUID> @default addressbook
Creating and deleting that address book is currently out of scope for
SyncEvolution. At some point it was possible to have it create an
address book by using a file:///<file name> URI as database value, but
accessing files like that was removed in EDS a while back.
--
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.
8:58 p.m.
On Fri, 2012-08-31 at 19:41 -0600, Jeremy Whiting wrote:
Patrick,
Ok, I am able to create addressbooks in the same way I do for folks
eds backend unit tests by copying an ini file. The last snag I'm
hitting is that syncevo-local-sync is missing in our packages. Is
there some configure line that builds that? I don't see it in
/usr/local/lib/syncevolution in my build from git either.
It's linked in src/src.am:
# Local sync helper executable. Built unconditionally at the moment,
# thus creating a hard dependency on D-Bus.
libexec_PROGRAMS += src/syncevo-local-sync
The install prefix is /usr/local/libexec.
Are you saying that it doesn't get linked at all in your builds?
--
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.
10:48 p.m.
Ah, it's in /usr/local/libexec, got it. In our debian packages we are
configuring with --libexecdir /usr/lib/syncevolution. It was linked,
but not included in any of our resulting packages. I've fixed that
now and it's working nicely.
thanks,
Jeremy
On Sun, Sep 2, 2012 at 6:58 AM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
On Fri, 2012-08-31 at 19:41 -0600, Jeremy Whiting wrote:
> Patrick,
>
> Ok, I am able to create addressbooks in the same way I do for folks
> eds backend unit tests by copying an ini file. The last snag I'm
> hitting is that syncevo-local-sync is missing in our packages. Is
> there some configure line that builds that? I don't see it in
> /usr/local/lib/syncevolution in my build from git either.
It's linked in src/src.am:
# Local sync helper executable. Built unconditionally at the moment,
# thus creating a hard dependency on D-Bus.
libexec_PROGRAMS += src/syncevo-local-sync
The install prefix is /usr/local/libexec.
Are you saying that it doesn't get linked at all in your builds?
--
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.
4:06 a.m.
Hello Jeremy!
Here my comments for the code that you pushed yesterday.
Your latest code revision still creates string copies instead of just
splitting up the memory mapped file:
commit dcd892e84a630c394f9f3ac4615bed09d8c6bede
Author: Jeremy Whiting <jeremy.whiting(a)collabora.com>
Date: Fri Aug 31 18:18:01 2012 -0600
PBAP: Fix vcard parsing regexp so all vcards are parsed.
diff --git a/src/backends/pbap/PbapSyncSource.cpp
b/src/backends/pbap/PbapSyncSource.cpp
index b854633..d39dbec 100644
--- a/src/backends/pbap/PbapSyncSource.cpp
+++ b/src/backends/pbap/PbapSyncSource.cpp
@@ -280,13 +280,13 @@ void PbapSession::pullAll(Content &dst)
pcrecpp::StringPiece content(addr, sb.st_size);
- string vcarddata;
+ pcrecpp::StringPiece vcarddata;
int count = 0;
- pcrecpp::RE re("(^BEGIN:VCARD.*?^END:VCARD)",
+ pcrecpp::RE re("[\\r\\n]*(^BEGIN:VCARD.*?^END:VCARD)",
pcrecpp::RE_Options().set_dotall(true).set_multiline(true));
while (re.Consume(&content, &vcarddata)) {
std::string id = StringPrintf("%d", count);
- dst[id] = vcarddata;
+ dst[id] = vcarddata.as_string();
^^^^^^^^^^^^^^^^^^^^^^
I still think it would be better to rearrange the class so that dst maps
from id to a StringPiece, with the actual memory owned by the class
(either in the memory mapped area or in a std::string buffer).
The parsing code should be identical for both code paths.
Unmapping the memory mapped file is missing.
The comment for the commit should describe the commit in enough detail
that a reader of the commit log knows how this new functionality works
(key points: dynamic runtime check, tries new API first), and what the
limitations/requirements are (needs D-Bus daemon >= xxx - forgot the
exact number).
It only works as long as obexd only sends strings as property values.
This is not guaranteed. As I said earlier, "Instead we need a
boost::variant<std::string, GDBusCXX::DBusObject_t,
int, bool, uint8_t, int16_t, uint16_t, int32_t, uint32_t, int64_t,
uint64_t> as value for the map".
This should be added to the commit messages and as bug entry in the
issue tracker, otherwise there is a high risk that it'll get into a
release without fixing it. Heck, I probably should insist on it getting
fixed before merging into master :-/
I'd prefer to merge one patch for the PBAP backend. Feel free to squash
your and my commits into 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.
12:40 a.m.
Patrick,
Ok, I've added some changes to my branch and will push shortly. One
remaining issue is the boost::variant for Params. If I add int, or
any of the other plain data types I get errors like this:
ln -s src/backends/webdav/syncevo-webdav-lookup.sh
src/backends/webdav/syncevo-webdav-lookup
/bin/bash ./libtool --tag=CXX --mode=compile g++ -DHAVE_CONFIG_H
-I. -I/home/chaiwala/devel/syncevolution/src -I./src/gdbusxx -I./test
-I/usr/include -pthread -I/usr/include/gnome-keyring-1
-I/usr/include/libxml2 -I/usr/include/libsoup-2.4
-I/usr/include/glib-2.0 -I/usr/lib/i386-linux-gnu/glib-2.0/include
-I/usr/include/nss -I/usr/include/nspr
-I/usr/include/evolution-data-server-3.6 -pthread
-I/usr/include/gnome-keyring-1 -I/usr/include/libxml2
-I/usr/include/libsoup-2.4 -I/usr/include/glib-2.0
-I/usr/lib/i386-linux-gnu/glib-2.0/include -I/usr/include/nss
-I/usr/include/nspr -I/usr/include/evolution-data-server-3.6
-pthread -I/usr/include/nss -I/usr/include/nspr
-I/usr/include/glib-2.0 -I/usr/lib/i386-linux-gnu/glib-2.0/include
-I/usr/include/gnome-keyring-1 -I/usr/include/libxml2
-I/usr/include/libsoup-2.4 -I/usr/include/evolution-data-server-3.6
-I/usr/include/glib-2.0 -I/usr/lib/i386-linux-gnu/glib-2.0/include
-pthread -I/usr/include/glib-2.0
-I/usr/lib/i386-linux-gnu/glib-2.0/include -I/usr/include/glib-2.0
-I/usr/lib/i386-linux-gnu/glib-2.0/include -Wall
-Wno-unknown-pragmas -Wno-deprecated-declarations -pthread
-I/usr/include/glib-2.0 -I/usr/lib/i386-linux-gnu/glib-2.0/include
-g -O2 -MT src/backends/pbap/src_backends_pbap_syncpbap_la-PbapSyncSource.lo
-MD -MP -MF src/backends/pbap/.deps/src_backends_pbap_syncpbap_la-PbapSyncSource.Tpo
-c -o src/backends/pbap/src_backends_pbap_syncpbap_la-PbapSyncSource.lo
`test -f 'src/backends/pbap/PbapSyncSource.cpp' || echo
'./'`src/backends/pbap/PbapSyncSource.cpp
libtool: compile: g++ -DHAVE_CONFIG_H -I.
-I/home/chaiwala/devel/syncevolution/src -I./src/gdbusxx -I./test
-I/usr/include -pthread -I/usr/include/gnome-keyring-1
-I/usr/include/libxml2 -I/usr/include/libsoup-2.4
-I/usr/include/glib-2.0 -I/usr/lib/i386-linux-gnu/glib-2.0/include
-I/usr/include/nss -I/usr/include/nspr
-I/usr/include/evolution-data-server-3.6 -pthread
-I/usr/include/gnome-keyring-1 -I/usr/include/libxml2
-I/usr/include/libsoup-2.4 -I/usr/include/glib-2.0
-I/usr/lib/i386-linux-gnu/glib-2.0/include -I/usr/include/nss
-I/usr/include/nspr -I/usr/include/evolution-data-server-3.6 -pthread
-I/usr/include/nss -I/usr/include/nspr -I/usr/include/glib-2.0
-I/usr/lib/i386-linux-gnu/glib-2.0/include
-I/usr/include/gnome-keyring-1 -I/usr/include/libxml2
-I/usr/include/libsoup-2.4 -I/usr/include/evolution-data-server-3.6
-I/usr/include/glib-2.0 -I/usr/lib/i386-linux-gnu/glib-2.0/include
-pthread -I/usr/include/glib-2.0
-I/usr/lib/i386-linux-gnu/glib-2.0/include -I/usr/include/glib-2.0
-I/usr/lib/i386-linux-gnu/glib-2.0/include -Wall -Wno-unknown-pragmas
-Wno-deprecated-declarations -pthread -I/usr/include/glib-2.0
-I/usr/lib/i386-linux-gnu/glib-2.0/include -g -O2 -MT
src/backends/pbap/src_backends_pbap_syncpbap_la-PbapSyncSource.lo -MD
-MP -MF src/backends/pbap/.deps/src_backends_pbap_syncpbap_la-PbapSyncSource.Tpo
-c src/backends/pbap/PbapSyncSource.cpp -fPIC -DPIC -o
src/backends/pbap/.libs/src_backends_pbap_syncpbap_la-PbapSyncSource.o
In file included from src/backends/pbap/PbapSyncSource.cpp:45:0:
./src/gdbusxx/gdbus-cxx-bridge.h: In static member function 'static
void GDBusCXX::dbus_traits<std::map<K, V, C>
::append(GVariantBuilder&, GDBusCXX::dbus_traits<std::map<K,
V, C>
::arg_type) [with K = std::basic_string<char>, V =
boost::variant<std::basic_string<char>, GDBusCXX::DBusObject_t, int>,
C = std::less<std::basic_string<char> >, GVariantBuilder =
_GVariantBuilder, GDBusCXX::dbus_traits<std::map<K, V, C> >::arg_type
= const std::map<std::basic_string<char>,
boost::variant<std::basic_string<char>, GDBusCXX::DBusObject_t, int>
&, GDBusCXX::dbus_traits<std::map<K, V, C>
>::host_type =
std::map<std::basic_string<char>,
boost::variant<std::basic_string<char>, GDBusCXX::DBusObject_t, int>
]':
./src/gdbusxx/gdbus-cxx-bridge.h:339:9: instantiated
from
'GDBusCXX::AppendRetvals& GDBusCXX::AppendRetvals::operator<<(const
A&) [with A = std::map<std::basic_string<char>,
boost::variant<std::basic_string<char>, GDBusCXX::DBusObject_t, int>
, GDBusCXX::AppendRetvals = GDBusCXX::AppendRetvals]'
./src/gdbusxx/gdbus-cxx-bridge.h:4404:9: instantiated from
'GDBusCXX::DBusClientCall<CallTraits>::Return_t
GDBusCXX::DBusClientCall<CallTraits>::operator()(const A1&, const A2&)
[with A1 = std::basic_string<char>, A2 =
std::map<std::basic_string<char>,
boost::variant<std::basic_string<char>, GDBusCXX::DBusObject_t, int>
, CallTraits = GDBusCXX::Ret1Traits<GDBusCXX::DBusObject_t>,
GDBusCXX::DBusClientCall<CallTraits>::Return_t =
GDBusCXX::DBusObject_t]'
src/backends/pbap/PbapSyncSource.cpp:163:48: instantiated from here
./src/gdbusxx/gdbus-cxx-bridge.h:1718:13: error: 'append' is not a
member of 'GDBusCXX::dbus_traits<boost::variant<std::basic_string<char>,
GDBusCXX::DBusObject_t, int> >'
./src/gdbusxx/gdbus-cxx-bridge.h: In static member function 'static
std::string GDBusCXX::dbus_traits<std::map<K, V, C>
::getContainedType() [with K = std::basic_string<char>, V =
boost::variant<std::basic_string<char>, GDBusCXX::DBusObject_t, int>,
C = std::less<std::basic_string<char> >, std::string =
std::basic_string<char]':
./src/gdbusxx/gdbus-cxx-bridge.h:1716:13: instantiated from 'static
void GDBusCXX::dbus_traits<std::map<K, V, C>
::append(GVariantBuilder&, GDBusCXX::dbus_traits<std::map<K,
V, C>
::arg_type) [with K = std::basic_string<char>, V =
boost::variant<std::basic_string<char>, GDBusCXX::DBusObject_t, int>,
C = std::less<std::basic_string<char> >, GVariantBuilder =
_GVariantBuilder, GDBusCXX::dbus_traits<std::map<K, V, C> >::arg_type
= const std::map<std::basic_string<char>,
boost::variant<std::basic_string<char>, GDBusCXX::DBusObject_t, int>
&, GDBusCXX::dbus_traits<std::map<K, V, C>
>::host_type =
std::map<std::basic_string<char>,
boost::variant<std::basic_string<char>, GDBusCXX::DBusObject_t, int>
]'
./src/gdbusxx/gdbus-cxx-bridge.h:339:9: instantiated
from
'GDBusCXX::AppendRetvals& GDBusCXX::AppendRetvals::operator<<(const
A&) [with A = std::map<std::basic_string<char>,
boost::variant<std::basic_string<char>, GDBusCXX::DBusObject_t, int>
, GDBusCXX::AppendRetvals = GDBusCXX::AppendRetvals]'
./src/gdbusxx/gdbus-cxx-bridge.h:4404:9: instantiated from
'GDBusCXX::DBusClientCall<CallTraits>::Return_t
GDBusCXX::DBusClientCall<CallTraits>::operator()(const A1&, const A2&)
[with A1 = std::basic_string<char>, A2 =
std::map<std::basic_string<char>,
boost::variant<std::basic_string<char>, GDBusCXX::DBusObject_t, int>
, CallTraits = GDBusCXX::Ret1Traits<GDBusCXX::DBusObject_t>,
GDBusCXX::DBusClientCall<CallTraits>::Return_t =
GDBusCXX::DBusObject_t]'
src/backends/pbap/PbapSyncSource.cpp:163:48: instantiated from here
./src/gdbusxx/gdbus-cxx-bridge.h:1673:13: error: 'getType' is not a
member of 'GDBusCXX::dbus_traits<boost::variant<std::basic_string<char>,
GDBusCXX::DBusObject_t, int> >'
./src/gdbusxx/gdbus-cxx-bridge.h: In static member function 'static
void GDBusCXX::dbus_traits<std::map<K, V, C>
::get(GDBusCXX::ExtractArgs&, GVariantIter&,
GDBusCXX::dbus_traits<std::map<K, V, C> >::host_type&) [with K =
std::basic_string<char>, V = boost::variant<std::basic_string<char>,
GDBusCXX::DBusObject_t, int>, C = std::less<std::basic_string<char> >,
GVariantIter = _GVariantIter, GDBusCXX::dbus_traits<std::map<K, V, C>
::host_type = std::map<std::basic_string<char>,
boost::variant<std::basic_string<char>, GDBusCXX::DBusObject_t, int>
]':
./src/gdbusxx/gdbus-cxx-bridge.h:1580:9: instantiated
from 'static
void GDBusCXX::dbus_traits<std::pair<_T1, _T2>
::get(GDBusCXX::ExtractArgs&, GVariantIter&,
GDBusCXX::dbus_traits<std::pair<_T1, _T2> >::host_type&) [with A =
GDBusCXX::DBusObject_t, B = std::map<std::basic_string<char>,
boost::variant<std::basic_string<char>, GDBusCXX::DBusObject_t, int>
, GVariantIter = _GVariantIter,
GDBusCXX::dbus_traits<std::pair<_T1,
_T2> >::host_type =
std::pair<GDBusCXX::DBusObject_t,
std::map<std::basic_string<char>,
boost::variant<std::basic_string<char>, GDBusCXX::DBusObject_t, int> >
]'
./src/gdbusxx/gdbus-cxx-bridge.h:467:9: instantiated
from
'GDBusCXX::ExtractArgs& GDBusCXX::Get<A>::get(GDBusCXX::ExtractArgs&)
const [with A = std::pair<GDBusCXX::DBusObject_t,
std::map<std::basic_string<char>,
boost::variant<std::basic_string<char>, GDBusCXX::DBusObject_t, int> >
]'
./src/gdbusxx/gdbus-cxx-bridge.h:457:27: instantiated
from
'GDBusCXX::ExtractArgs& GDBusCXX::ExtractArgs::operator>>(const A&)
[with A = GDBusCXX::Get<std::pair<GDBusCXX::DBusObject_t,
std::map<std::basic_string<char>,
boost::variant<std::basic_string<char>, GDBusCXX::DBusObject_t, int> >
>, GDBusCXX::ExtractArgs = GDBusCXX::ExtractArgs]'
./src/gdbusxx/gdbus-cxx-bridge.h:4176:9: instantiated from 'static
GDBusCXX::Ret1Traits<R1>::Return_t
GDBusCXX::Ret1Traits<R1>::demarshal(GDBusCXX::DBusMessagePtr&, const
GDBusCXX::DBusConnectionPtr&) [with R1 =
std::pair<GDBusCXX::DBusObject_t, std::map<std::basic_string<char>,
boost::variant<std::basic_string<char>, GDBusCXX::DBusObject_t, int> >
, GDBusCXX::Ret1Traits<R1>::Return_t =
std::pair<GDBusCXX::DBusObject_t, std::map<std::basic_string<char>,
boost::variant<std::basic_string<char>, GDBusCXX::DBusObject_t, int> >
]'
./src/gdbusxx/gdbus-cxx-bridge.h:4351:51: instantiated
from
'GDBusCXX::DBusClientCall<CallTraits>::Return_t
GDBusCXX::DBusClientCall<CallTraits>::sendAndReturn(GDBusCXX::DBusMessagePtr&)
[with CallTraits =
GDBusCXX::Ret1Traits<std::pair<GDBusCXX::DBusObject_t,
std::map<std::basic_string<char>,
boost::variant<std::basic_string<char>, GDBusCXX::DBusObject_t, int> >
>, GDBusCXX::DBusClientCall<CallTraits>::Return_t =
std::pair<GDBusCXX::DBusObject_t, std::map<std::basic_string<char>,
boost::variant<std::basic_string<char>, GDBusCXX::DBusObject_t, int> >
]'
./src/gdbusxx/gdbus-cxx-bridge.h:4387:33: instantiated
from
'GDBusCXX::DBusClientCall<CallTraits>::Return_t
GDBusCXX::DBusClientCall<CallTraits>::operator()(const A1&) [with A1 =
std::basic_string<char>, CallTraits =
GDBusCXX::Ret1Traits<std::pair<GDBusCXX::DBusObject_t,
std::map<std::basic_string<char>,
boost::variant<std::basic_string<char>, GDBusCXX::DBusObject_t, int> >
>, GDBusCXX::DBusClientCall<CallTraits>::Return_t =
std::pair<GDBusCXX::DBusObject_t, std::map<std::basic_string<char>,
boost::variant<std::basic_string<char>, GDBusCXX::DBusObject_t, int> >
]'
src/backends/pbap/PbapSyncSource.cpp:256:88:
instantiated from here
./src/gdbusxx/gdbus-cxx-bridge.h:1702:13: error: 'get' is not a member
of 'GDBusCXX::dbus_traits<boost::variant<std::basic_string<char>,
GDBusCXX::DBusObject_t, int> >'
In file included from src/backends/pbap/PbapSyncSource.cpp:447:0:
./src/gdbusxx/gdbus-cxx-bridge.h: In static member function 'static
std::string GDBusCXX::dbus_traits<std::map<K, V, C>
::getContainedType() [with K = std::basic_string<char>, V =
boost::variant<std::basic_string<char>, GDBusCXX::DBusObject_t, int>,
C = std::less<std::basic_string<char> >, std::string =
std::basic_string<char]':
./src/gdbusxx/gdbus-cxx-bridge.h:1674:5: warning: control reaches end
of non-void function [-Wreturn-type]
make[2]: *** [src/backends/pbap/src_backends_pbap_syncpbap_la-PbapSyncSource.lo]
Error 1
make[2]: Leaving directory `/home/chaiwala/devel/syncevolution'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/home/chaiwala/devel/syncevolution'
make: *** [all] Error 2
One other issue is that you mentioned I need to unmap the file
somehow, when should that happen? I would think only on destruction
since we are using StringPiece's from it until that point, right?
thanks,
Jeremy
On Fri, Sep 21, 2012 at 2:06 PM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
Hello Jeremy!
Here my comments for the code that you pushed yesterday.
Your latest code revision still creates string copies instead of just
splitting up the memory mapped file:
commit dcd892e84a630c394f9f3ac4615bed09d8c6bede
Author: Jeremy Whiting <jeremy.whiting(a)collabora.com>
Date: Fri Aug 31 18:18:01 2012 -0600
PBAP: Fix vcard parsing regexp so all vcards are parsed.
diff --git a/src/backends/pbap/PbapSyncSource.cpp
b/src/backends/pbap/PbapSyncSource.cpp
index b854633..d39dbec 100644
--- a/src/backends/pbap/PbapSyncSource.cpp
+++ b/src/backends/pbap/PbapSyncSource.cpp
@@ -280,13 +280,13 @@ void PbapSession::pullAll(Content &dst)
pcrecpp::StringPiece content(addr, sb.st_size);
- string vcarddata;
+ pcrecpp::StringPiece vcarddata;
int count = 0;
- pcrecpp::RE re("(^BEGIN:VCARD.*?^END:VCARD)",
+ pcrecpp::RE re("[\\r\\n]*(^BEGIN:VCARD.*?^END:VCARD)",
pcrecpp::RE_Options().set_dotall(true).set_multiline(true));
while (re.Consume(&content, &vcarddata)) {
std::string id = StringPrintf("%d", count);
- dst[id] = vcarddata;
+ dst[id] = vcarddata.as_string();
^^^^^^^^^^^^^^^^^^^^^^
I still think it would be better to rearrange the class so that dst maps
from id to a StringPiece, with the actual memory owned by the class
(either in the memory mapped area or in a std::string buffer).
The parsing code should be identical for both code paths.
Unmapping the memory mapped file is missing.
The comment for the commit should describe the commit in enough detail
that a reader of the commit log knows how this new functionality works
(key points: dynamic runtime check, tries new API first), and what the
limitations/requirements are (needs D-Bus daemon >= xxx - forgot the
exact number).
It only works as long as obexd only sends strings as property values.
This is not guaranteed. As I said earlier, "Instead we need a
boost::variant<std::string, GDBusCXX::DBusObject_t,
int, bool, uint8_t, int16_t, uint16_t, int32_t, uint32_t, int64_t,
uint64_t> as value for the map".
This should be added to the commit messages and as bug entry in the
issue tracker, otherwise there is a high risk that it'll get into a
release without fixing it. Heck, I probably should insist on it getting
fixed before merging into master :-/
I'd prefer to merge one patch for the PBAP backend. Feel free to squash
your and my commits into 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.
6:03 p.m.
On Tue, 2012-09-25 at 10:40 -0600, Jeremy Whiting wrote:
Ok, I've added some changes to my branch and will push shortly.
One
remaining issue is the boost::variant for Params. If I add int, or
any of the other plain data types I get errors like this:
[...]
gdbus-cxx-bridge.h must be extended to support boost::variant with more
than two possible types.
However, while looking at that code I found:
/**
* A boost::variant <V> maps to a dbus variant, only care about values of
* type V but will not throw error if type is not matched, this is useful if
* application is interested on only a sub set of possible value types
* in variant.
*/
...
if (!boost::iequals(g_variant_get_type_string(varVar),
dbus_traits<V>::getSignature())) {
//ignore unrecognized sub type in variant
return;
}
In other words, we can keep the PBAP backend code as it is: we will only
store the properties that have strings as values, but as we don't need
anything else (or even the strings) at all, that's okay.
I also found a leak when that return is triggered, but that's a
different story. Will fix it.
One other issue is that you mentioned I need to unmap the file
somehow, when should that happen? I would think only on destruction
since we are using StringPiece's from it until that point, right?
Right. I went ahead and cleaned up the backend, both in the new and in
older code (commit messages below).
The revised code is in the for-master/pbap branch. I'm currently running
a test against that code. Jeremy, shall we merge into master now? I
don't have no objections anymore.
commit 84698cbfed1ae2a53afface7d8f9a832eb3b4780
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Wed Sep 26 11:51:01 2012 +0200
GDBus GIO: use RAII for GVariant
Instead of using explicit g_variant_unref() calls, let a smart pointer
class own the instance. This allows to remove code and (more
important) fixes leaks when something throws an exception or the
D-Bus traits for boost::variant return prematurely.
commit d8dce7acc485388caa0eff080c136da20dccd240
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Wed Sep 26 10:52:33 2012 +0200
PBAP: clean up and bug fixes for new API support
Fallback to old obexd API was broken because creating the
DBusRemoteObject does not verify whether the service really exists and
thus always succeeds. Fixed by checking for existence as part of the
actual CreateSession method call.
The new code needs glib. Include header file and declare dependency in
configure.
The backend must throw errors when something fatal happens. Logging
the error is not enough, because that can't be checked by the
caller. Throwing errors is best done via the utility methods in
SyncSource, because those include the source name in the exception.
Memory handling was broken: nothing owned the memory in the
StringPiece instances, munmap() was missing. Fixed by making
PbabSyncSource the owner of both.
Unified the parsing of the result. The new code based on pcrecpp is
used for both old and new obexd API.
File name and GError allocated by g_file_open_tmp() were leaked. The
file descriptor and the file were leaked in case of aborting via an
exception. Now these resources are owned by a class which will always
clean up when getting destructed.
A failed transfer was not checked for when using the new API. Probably
failed when trying to use the file (because obexd deletes it), but
better show the error message that we got for the failed transfer.
Remove the obsolete vcardParse().
--
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.
3:57 a.m.
On Fri, 2012-09-21 at 22:06 +0200, Patrick Ohly wrote:
I still think it would be better to rearrange the class so that dst
maps
from id to a StringPiece, with the actual memory owned by the class
(either in the memory mapped area or in a std::string buffer).
Your commit which implements that (0f827319) is incomplete: ownership of
the memory referenced by the StringPiece instances is unclear. Because
umapping that memory is missing, it happens to work. I doubt that the
code works for old obex, because the StringPiece is initialized with a
reference to a std::string which then gets deleted.
void PbapSyncSource::listAllItems(RevisionMap_t &revisions)
{
- typedef std::pair<std::string, std::string> Entry;
+ typedef std::pair<std::string, pcrecpp::StringPiece> Entry;
BOOST_FOREACH(const Entry &entry, m_content) {
revisions[entry.first] = "0";
}
A better solution would be to use Content::value_type in BOOST_FOREACH
instead of defining Entry. Not sure why that was done in the first
place.
The parsing code should be identical for both code paths.
Unmapping the memory mapped file is missing.
The comment for the commit should describe the commit in enough detail
that a reader of the commit log knows how this new functionality works
(key points: dynamic runtime check, tries new API first), and what the
limitations/requirements are (needs D-Bus daemon >= xxx - forgot the
exact number).
This all still missing.
It only works as long as obexd only sends strings as property
values.
This is not guaranteed. As I said earlier, "Instead we need a
boost::variant<std::string, GDBusCXX::DBusObject_t,
int, bool, uint8_t, int16_t, uint16_t, int32_t, uint32_t, int64_t,
uint64_t> as value for the map".
I'll have a look at that tomorrow.
--
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.
3044
days inactive
3084
days old
syncevolution@synevolution.org
33 comments
3 participants
participants (3)
-
Jeremy Whiting -
Mikel Astiz -
Patrick Ohly