On Tue, Nov 29, 2011 at 2:31 PM, Patrick Ohly <[email protected]> wrote:
> On Di, 2011-11-29 at 13:20 +0100, Chris Kühl wrote:
>> On Tue, Nov 29, 2011 at 12:12 PM, Patrick Ohly <[email protected]>
>> wrote:
>> > On Mo, 2011-11-28 at 12:24 +0100, Chris Kühl wrote:
>> >> On Wed, Nov 16, 2011 at 2:52 PM, Patrick Ohly <[email protected]>
>> >> wrote:
>> >> > On Mi, 2011-11-16 at 11:58 +0100, Patrick Ohly wrote:
>> >> >> On Mi, 2011-11-16 at 11:10 +0100, Chris Kühl wrote:
>> >> >> > On Mon, Nov 14, 2011 at 4:49 PM, Chris Kühl <[email protected]>
>> >> >> > wrote:
>> >> >> > > On Mon, Nov 14, 2011 at 2:31 PM, Patrick Ohly
>> >> >> > > <[email protected]> wrote:
>> >> >> > > Ok. We should be close to requesting a merge. Only 2 failures to
>> >> >> > > fix
>> >> >> > > before we've reached parity on our machines.
>> >> >> > >
>> >> >> >
>> >> >> > After fixing those remaining issues, we ran the server under valgrind
>> >> >> > and fixed up a few memory leaks we found. I've now created a
>> >> >> > for-master/gio-gdbus branch, squashed the commits and pushed.
>> >> >>
>> >> >> I've started a test run.
>> >> >
>> >> > There seems to be a regression in the D-Bus testing.
>> >> >
>> >> > On Ubuntu Lucid, startup of the syncevo-dbus-server without GIO GDBus
>> >> > fails:
>> >> > http://syncev.meego.com/2011-11-16-10-54_dist/lucid-amd64/6-dbus/output.txt
>> >> >
>> >>
>> >> So, I finally got the issue in Debian Stable and Ubuntu LTS fixed.
>> >> Turned out to be that I'd inadvertently set the server's main
>> >> connection to shared instead of private. I'm not sure why this was
>> >> only causing issues on these distros. That initially sent me searching
>> >> for the issue in all the wrong places.
>> >
>> > lucid-amd64 without GIO GDBus passed the tests now. I had to take the
>> > for-master/gio-gdbus branch temporarily out of testing because the
>> > autoconf changes conflicted with another branch, but now it is back and
>> > was tested.
>> >
>> > However, it segfaults during startup on Debian Testing with GIO GDBus
>> > enabled:
>> > http://syncev.meego.com/latest/testing-amd64-gio/6-dbus/output.txt
>> >
>>
>> Link is broken.
>
> Ah, the "latest" symlink. I need to remember not to use that in email...
>
>> But I've not had that issue with Debian Testing. Hmm?
>> I've got a clean install with only the necessary packages added for
>> building and running syncevolution.
>
> It happens when Bluez is not running.
>
Oh, that's why I didn't hit it.
> Program received signal SIGSEGV, Segmentation fault.
> 0x0000000000597e36 in set (error=0xb7e4c0, this=0x0) at
> /data/runtests/work/sources/syncevolution/src/gdbusxx/gdbus-cxx-bridge.h:164
> warning: Source file is more recent than executable.
> 164 if (m_error) {
> (gdb) where
> #0 0x0000000000597e36 in set (error=0xb7e4c0, this=0x0) at
> /data/runtests/work/sources/syncevolution/src/gdbusxx/gdbus-cxx-bridge.h:164
> #1 GDBusCXX::dbus_get_bus_connection (busType=<optimized out>, name=0x0,
> unshared=<optimized out>, err=0x0) at
> /data/runtests/work/sources/syncevolution/src/gdbusxx/gdbus-cxx-bridge.cpp:68
> #2 0x0000000000574450 in SyncEvo::BluezManager::BluezManager (this=0xb85740,
> server=<optimized out>) at
> /data/runtests/work/sources/syncevolution/src/dbus/server/bluez-manager.cpp:44
> #3 0x0000000000541250 in SyncEvo::Server::Server (this=0x7fffffffd9a0,
> loop=0xb6eab0, shutdownRequested=@0xb46af8, restart=..., conn=<optimized
> out>, duration=600)
> at /data/runtests/work/sources/syncevolution/src/dbus/server/server.cpp:227
> #4 0x000000000052121c in main (argc=1, argv=0x7fffffffdfa8, envp=<optimized
> out>) at
> /data/runtests/work/sources/syncevolution/src/dbus/server/main.cpp:113
>
> GDBusCXX::dbus_get_bus_connection() takes a GDBusCXX::DBusErrorCXX *err
> parameter and doesn't check it for NULL:
>
> 63 } else {
> 64 // This returns a singleton, shared connection object.
> 65 conn = g_bus_get_sync(boost::iequals(busType, "SESSION") ?
> G_BUS_TYPE_SESSION : G_BUS_TYPE_SYSTEM,
> 66 NULL, &error);
> 67 if(error != NULL) {
> 68 err->set(error); <======
> 69 return NULL;
> 70 }
> 71 }
>
> Here's a patch which fixes the issue:
>
> Modified src/gdbusxx/gdbus-cxx-bridge.cpp
> diff --git a/src/gdbusxx/gdbus-cxx-bridge.cpp
> b/src/gdbusxx/gdbus-cxx-bridge.cpp
> index f5194e7..b103509 100644
> --- a/src/gdbusxx/gdbus-cxx-bridge.cpp
> +++ b/src/gdbusxx/gdbus-cxx-bridge.cpp
> @@ -35,7 +35,7 @@ boost::function<void (void)> MethodHandler::m_callback;
> GDBusConnection *dbus_get_bus_connection(const char *busType,
> const char *name,
> bool unshared,
> - DBusErrorCXX *err /* Ignored */)
> + DBusErrorCXX *err)
> {
> GDBusConnection *conn;
> GError* error = NULL;
> @@ -45,7 +45,9 @@ GDBusConnection *dbus_get_bus_connection(const char
> *busType,
> G_BUS_TYPE_SESSION :
> G_BUS_TYPE_SYSTEM,
> NULL, &error);
> if(address == NULL) {
> - err->set(error);
> + if (err) {
> + err->set(error);
> + }
> return NULL;
> }
> // Here we set up a private client connection using the chosen bus'
> address.
> @@ -56,24 +58,24 @@ GDBusConnection *dbus_get_bus_connection(const char
> *busType,
> NULL, NULL, &error);
> g_free(address);
>
> - if(error != NULL) {
> - err->set(error);
> + if(conn == NULL) {
> + if (err) {
> + err->set(error);
> + }
> return NULL;
> }
> } else {
> // This returns a singleton, shared connection object.
> conn = g_bus_get_sync(boost::iequals(busType, "SESSION") ?
> G_BUS_TYPE_SESSION : G_BUS_TYPE_SYSTEM,
> NULL, &error);
> - if(error != NULL) {
> - err->set(error);
> + if(conn == NULL) {
> + if (err) {
> + err->set(error);
> + }
> return NULL;
> }
> }
>
> - if(!conn) {
> - return NULL;
> - }
> -
> if(name) {
> g_bus_own_name_on_connection(conn, name, G_BUS_NAME_OWNER_FLAGS_NONE,
> NULL, NULL, NULL, NULL);
>
>
> Does that look right?
>
Seems fine to me.
> Speaking of DBusErrorCXX, why does it have a separate "message" member?
>
That's only in there to maintain compatibility. I believe the examples
were the only place the message field was used however. We could
easliy change those and get read of this member.
> /**
> * wrapper around GError which initializes
> * the struct automatically, then can be used to
> * throw an exception
> */
> class DBusErrorCXX
> {
> GError *m_error;
> public:
> char *message;
> DBusErrorCXX(GError *error = NULL)
> : m_error(error)
> {
> if (m_error) {
> message = m_error->message;
> }
> }
>
> That "message" pointer is not getting reset when set(NULL) is called,
> leading to a dangling pointer:
True. Shall I fix that or just git rid of the message?
>
> void set(GError *error) {
> if (m_error) {
> g_error_free (m_error);
> }
> m_error = error;
> if (m_error) {
> message = m_error->message;
> }
> }
>
>
Cheers,
Chris
_______________________________________________
SyncEvolution mailing list
[email protected]
http://lists.syncevolution.org/listinfo/syncevolution
