[PATCH 1/5] service: Fix not unreferencing adapter in case of error

[PATCH 1/5] service: Fix not unreferencing adapter in case of error

[Szymon Janc]

If g_dbus_register_interface failed adapter reference was not dropped.
Move getting reference after succesful g_dbus_register_interface call.
---
 plugins/service.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/plugins/service.c b/plugins/service.c
index ba7a693..d70c515 100644
--- a/plugins/service.c
+++ b/plugins/service.c
@@ -509,9 +509,6 @@ static int register_interface(const char *path, struct btd_adapter *adapter)
 	if (serv_adapter == NULL)
 		return -ENOMEM;
 
-	if (adapter != NULL)
-		serv_adapter->adapter = btd_adapter_ref(adapter);
-
 	serv_adapter->pending_list = NULL;
 
 	if (g_dbus_register_interface(btd_get_dbus_connection(),
@@ -520,13 +517,16 @@ static int register_interface(const char *path, struct btd_adapter *adapter)
 						path_unregister) == FALSE) {
 		error("D-Bus failed to register %s interface",
 							SERVICE_INTERFACE);
+
 		g_free(serv_adapter);
 		return -EIO;
 	}
 
 	DBG("Registered interface %s on path %s", SERVICE_INTERFACE, path);
 
-	if (serv_adapter->adapter == NULL)
+	if (adapter != NULL)
+		serv_adapter->adapter = btd_adapter_ref(adapter);
+	else
 		serv_adapter_any = serv_adapter;
 
 	return 0;
-- 
1.7.9.5

[PATCH 2/5] service: Remove not needed initialization

[Szymon Janc]

serv_adapter is allocated with g_try_new0 so there is no need to
explicite initialize its pending_list to NULL.
---
 plugins/service.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/plugins/service.c b/plugins/service.c
index d70c515..af24839 100644
--- a/plugins/service.c
+++ b/plugins/service.c
@@ -509,8 +509,6 @@ static int register_interface(const char *path, struct btd_adapter *adapter)
 	if (serv_adapter == NULL)
 		return -ENOMEM;
 
-	serv_adapter->pending_list = NULL;
-
 	if (g_dbus_register_interface(btd_get_dbus_connection(),
 				path, SERVICE_INTERFACE,
 				service_methods, NULL, NULL, serv_adapter,
-- 
1.7.9.5

[PATCH 3/5] adapter: Call driver remove callback when unregistering driver

[Szymon Janc]

This seems to be what plugins expect as only dbusoob explicite called
its remove function before unregistering adapter drivers.

This results in cleaner shutdown path:

Without patch:
bluetoothd[13563]: src/mgmt.c:mgmt_remove_uuid() index 0
bluetoothd[13563]: src/adapter.c:btd_adapter_unref() 0x606b2a0: ref=4
bluetoothd[13563]: src/adapter.c:btd_adapter_unref() 0x606b2a0: ref=3
bluetoothd[13563]: Stopping SDP server
bluetoothd[13563]: Exit
==13563==
==13563== HEAP SUMMARY:
==13563==     in use at exit: 64,908 bytes in 395 blocks
==13563==   total heap usage: 7,035 allocs, 6,640 frees, 4,432,371 bytes allocated
==13563==
==13563== LEAK SUMMARY:
==13563==    definitely lost: 0 bytes in 0 blocks
==13563==    indirectly lost: 0 bytes in 0 blocks
==13563==      possibly lost: 17,429 bytes in 169 blocks
==13563==    still reachable: 47,479 bytes in 226 blocks
==13563==         suppressed: 0 bytes in 0 blocks

With patch:
bluetoothd[13301]: src/mgmt.c:mgmt_remove_uuid() index 0
bluetoothd[13301]: src/adapter.c:btd_adapter_unref() 0x606b2a0: ref=1
bluetoothd[13301]: src/adapter.c:btd_adapter_unref() 0x606b2a0: ref=0
bluetoothd[13301]: src/adapter.c:adapter_free() 0x606b2a0
bluetoothd[13301]: Stopping SDP server
bluetoothd[13301]: Exit
==13301==
==13301== HEAP SUMMARY:
==13301==     in use at exit: 64,954 bytes in 348 blocks
==13301==   total heap usage: 7,247 allocs, 6,899 frees, 4,625,672 bytes allocated
==13301==
==13301== LEAK SUMMARY:
==13301==    definitely lost: 0 bytes in 0 blocks
==13301==    indirectly lost: 0 bytes in 0 blocks
==13301==      possibly lost: 17,334 bytes in 150 blocks
==13301==    still reachable: 47,620 bytes in 198 blocks
==13301==         suppressed: 0 bytes in 0 blocks
---
 plugins/dbusoob.c |    2 --
 src/adapter.c     |    5 +++++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/plugins/dbusoob.c b/plugins/dbusoob.c
index e58b353..7d9a858 100644
--- a/plugins/dbusoob.c
+++ b/plugins/dbusoob.c
@@ -346,8 +346,6 @@ static void dbusoob_exit(void)
 {
 	DBG("Cleanup dbusoob plugin");
 
-	manager_foreach_adapter((adapter_cb) oob_remove, NULL);
-
 	btd_unregister_adapter_driver(&oob_driver);
 }
 
diff --git a/src/adapter.c b/src/adapter.c
index 0d1dfea..163360f 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -3291,6 +3291,11 @@ int btd_register_adapter_driver(struct btd_adapter_driver *driver)
 
 static void unload_driver(struct btd_adapter *adapter, gpointer data)
 {
+	struct btd_adapter_driver *driver = data;
+
+	if (driver->remove)
+		driver->remove(adapter);
+
 	adapter->drivers = g_slist_remove(adapter->drivers, data);
 }
 
-- 
1.7.9.5

[PATCH 4/5] adaptername: Remove not needed empty remove callback

[Szymon Janc]

Remove callback is called only if it is not NULL so there is no need to
register empty callback function.
---
 plugins/adaptername.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/plugins/adaptername.c b/plugins/adaptername.c
index 353f11c..745238a 100644
--- a/plugins/adaptername.c
+++ b/plugins/adaptername.c
@@ -261,14 +261,10 @@ fail:
 	return FALSE;
 }
 
-static void adaptername_remove(struct btd_adapter *adapter)
-{
-}
-
 static struct btd_adapter_driver adaptername_driver = {
 	.name	= "adaptername",
 	.probe	= adaptername_probe,
-	.remove	= adaptername_remove,
+	.remove	= NULL,
 };
 
 static int adaptername_init(void)
-- 
1.7.9.5

[PATCH 5/5] formfactor: Remove not needed empty remove callback

[Szymon Janc]

Remove callback is called only if it is not NULL so there is no need to
register empty callback function.
---
 plugins/formfactor.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/plugins/formfactor.c b/plugins/formfactor.c
index 0e19ac6..41f9c2a 100644
--- a/plugins/formfactor.c
+++ b/plugins/formfactor.c
@@ -124,14 +124,10 @@ static int formfactor_probe(struct btd_adapter *adapter)
 	return 0;
 }
 
-static void formfactor_remove(struct btd_adapter *adapter)
-{
-}
-
 static struct btd_adapter_driver formfactor_driver = {
 	.name	= "formfactor",
 	.probe	= formfactor_probe,
-	.remove	= formfactor_remove,
+	.remove	= NULL,
 };
 
 static int formfactor_init(void)
-- 
1.7.9.5

Re: [PATCH 4/5] adaptername: Remove not needed empty remove callback

[Anderson Lizardo]

Hi Szymon,

On Mon, Nov 26, 2012 at 5:07 AM, Szymon Janc <szymon.janc@tieto.com> wrote:
>  static struct btd_adapter_driver adaptername_driver = {
>         .name   = "adaptername",
>         .probe  = adaptername_probe,
> -       .remove = adaptername_remove,
> +       .remove = NULL,
>  };

This variable is static, so not explicitly initialized members will be NULL.

Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

Re: [PATCH 4/5] adaptername: Remove not needed empty remove callback

[Szymon Janc]

On Monday 26 of November 2012 12:45:30 Anderson Lizardo wrote:
> Hi Szymon,

Hi Anderson,

> 
> On Mon, Nov 26, 2012 at 5:07 AM, Szymon Janc <szymon.janc@tieto.com> wrote:
> >  static struct btd_adapter_driver adaptername_driver = {
> >         .name   = "adaptername",
> >         .probe  = adaptername_probe,
> > -       .remove = adaptername_remove,
> > +       .remove = NULL,
> >  };
> 
> This variable is static, so not explicitly initialized members will be NULL.

Yes, yet according to code style for bluez statics should be initialized
explicitly, right? Or this does not count for struct members?

-- 
BR
Szymon Janc

Re: [PATCH 4/5] adaptername: Remove not needed empty remove callback

[Johan Hedberg]

Hi Szymon,

On Mon, Nov 26, 2012, Szymon Janc wrote:
> On Monday 26 of November 2012 12:45:30 Anderson Lizardo wrote:
> > Hi Szymon,
> 
> Hi Anderson,
> 
> > 
> > On Mon, Nov 26, 2012 at 5:07 AM, Szymon Janc <szymon.janc@tieto.com> wrote:
> > >  static struct btd_adapter_driver adaptername_driver = {
> > >         .name   = "adaptername",
> > >         .probe  = adaptername_probe,
> > > -       .remove = adaptername_remove,
> > > +       .remove = NULL,
> > >  };
> > 
> > This variable is static, so not explicitly initialized members will be NULL.
> 
> Yes, yet according to code style for bluez statics should be initialized
> explicitly, right? Or this does not count for struct members?

I've left this out for struct members at least in new code so I'd say
that it's not needed.

Johan

Re: [PATCH 4/5] adaptername: Remove not needed empty remove callback

[Anderson Lizardo]

Hi Szymon,

On Mon, Nov 26, 2012 at 6:52 AM, Szymon Janc <szymon.janc@tieto.com> wrote:
> On Monday 26 of November 2012 12:45:30 Anderson Lizardo wrote:
>> Hi Szymon,
>
> Hi Anderson,
>
>>
>> On Mon, Nov 26, 2012 at 5:07 AM, Szymon Janc <szymon.janc@tieto.com> wrote:
>> >  static struct btd_adapter_driver adaptername_driver = {
>> >         .name   = "adaptername",
>> >         .probe  = adaptername_probe,
>> > -       .remove = adaptername_remove,
>> > +       .remove = NULL,
>> >  };
>>
>> This variable is static, so not explicitly initialized members will be NULL.
>
> Yes, yet according to code style for bluez statics should be initialized
> explicitly, right? Or this does not count for struct members?

I forgot to mention that I did run this:

egrep -r '^[[:space:]]*\.[a-z_]+[[:space:]]*=.*NULL' *

with no results, which means no other structs are initializing unused
members with NULL. But you are right that we are still initializing
non-compound static variables explicitly to NULL or 0, as per
convention.

Best Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil