Re: [PATCH] Release references to technologies held by rfkill during cleanup.
On Tue, 2014-10-21 at 12:31 -0500, David Lechner wrote: Bluetooth, however, never sent this signal, This is caused because there are two plugins for Bluetooth, one for Bluez 4 and the other one for Bluez 5. Both register a connman_technology_driver. So far so good. The problem is caused by technology_get(type) in __connman_technology_add_rfkill(), with more than one technology driver this add function is called multiple times, each time increasing the reference count of the technology by one. It's not that easy to fix, as the technology is created when there is a technology driver present and either an rfkill switch or a connman device is added to the system. While I'm thinking on how to solve this, the UI mopping up after a sudden ConnMan disappearance from D-Bus looks suddenly like a very good and easy solution to the initial problem... Cheers, Patrik ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] Release references to technologies held by rfkill during cleanup.
On 10/23/2014 08:04 AM, Patrik Flykt wrote: This is caused because there are two plugins for Bluetooth, one for Bluez 4 and the other one for Bluez 5. Both register a connman_technology_driver. So far so good. The problem is caused by technology_get(type) in __connman_technology_add_rfkill(), with more than one technology driver this add function is called multiple times, each time increasing the reference count of the technology by one. It's not that easy to fix, as the technology is created when there is a technology driver present and either an rfkill switch or a connman device is added to the system. While I'm thinking on how to solve this, the UI mopping up after a sudden ConnMan disappearance from D-Bus looks suddenly like a very good and easy solution to the initial problem... I see what you mean. I guess the reason my initial patch seemed like it works is because there was only one extra technology_get called. If there were three Bluetooth drivers, then it would not work. It seems like you might be able to have an rfkill_get and rfkill_put so that the first driver that is registered that calls rfkill_get will call __connman_technology_add_rfkill, but subsequent calls to rfkill_get will just return without calling __connman_technology_add_rfkill. When the last driver is unregistered that calls rfkill_put, then it should in turn call __connman_technology_remove_rfkill. Of course, rfkill_process in src/rfkill.c would have to be modified to work with this as well. ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] Release references to technologies held by rfkill during cleanup.
Hi,
On Tue, 2014-10-21 at 12:27 -0500, David Lechner wrote:
---
src/connman.h| 1 +
src/rfkill.c | 2 ++
src/technology.c | 15 +++
3 files changed, 18 insertions(+)
diff --git a/src/connman.h b/src/connman.h
index da01215..bab013b 100644
--- a/src/connman.h
+++ b/src/connman.h
@@ -826,6 +826,7 @@ void __connman_service_save(struct connman_service
*service);
#include connman/notifier.h
+void __connman_technology_cleanup_rfkill(void);
int __connman_technology_init(void);
void __connman_technology_cleanup(void);
diff --git a/src/rfkill.c b/src/rfkill.c
index 960cfea..2f073a9 100644
--- a/src/rfkill.c
+++ b/src/rfkill.c
@@ -232,4 +232,6 @@ void __connman_rfkill_cleanup(void)
g_io_channel_unref(channel);
channel = NULL;
+
+ __connman_technology_cleanup_rfkill();
}
diff --git a/src/technology.c b/src/technology.c
index d80d9e6..5917c59 100644
--- a/src/technology.c
+++ b/src/technology.c
@@ -1776,6 +1776,21 @@ int __connman_technology_remove_rfkill(unsigned
int index,
return 0;
}
+gboolean find_first(gpointer key, gpointer value, gpointer user_data)
+{
+ return TRUE;
+}
+
+void __connman_technology_cleanup_rfkill(void)
+{
+ struct connman_rfkill *rfkill;
+
+ DBG();
+
+ while (rfkill = g_hash_table_find(rfkill_list, find_first, NULL))
+ __connman_technology_remove_rfkill(rfkill-index, rfkill-type);
+}
+
int __connman_technology_init(void)
{
DBG();
This sort of works, but I'd like to fix the problem in src/technology.c,
where the problem manifests itself.
So it seems ConnMan is not doing the technology_put() cleanup in the
correct place. As you noticed this currently happens only in
__connman_technology_remove(), and not on shutdown where only
free_rfkill() is called. So the code starting with the line technology
= technology_find(type); in __connman_technology_remove() should
actually happen in free_rfkill() instead. That way no matter what frees
up an rfkill structure also causes a technology_put() where the last one
sends off a TechnologRemoved signal before destroying the technology
structure.
Cheers,
Patrik
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] Release references to technologies held by rfkill during cleanup.
On 10/22/2014 03:14 AM, Patrik Flykt wrote:
So it seems ConnMan is not doing the technology_put() cleanup in the
correct place. As you noticed this currently happens only in
__connman_technology_remove(), and not on shutdown where only
free_rfkill() is called. So the code starting with the line technology
= technology_find(type); in __connman_technology_remove() should
actually happen in free_rfkill() instead. That way no matter what frees
up an rfkill structure also causes a technology_put() where the last one
sends off a TechnologRemoved signal before destroying the technology
structure.
I tried this suggestion, but it does not fix the problem I was trying to
fix. rfkill_list is destroyed in __connman_technology_cleanup which
happens long after plugins are cleaned up. This means most if not all
technologies have already been destroyed (possibly without being
properly released/removed) before we get to this point.
Based on your other reply though, it sounds like I need to fix my UI. I
did not consider the case of a connman crash, which would be a good
thing to handle. Also, there is the Released DBus signal that I was
overlooking.
Here is the patch of what I did to implement your suggestion.
--- connman-1.26.orig/src/technology.c
+++ connman-1.26/src/technology.c
@@ -364,13 +364,6 @@ bool connman_technology_get_wifi_tetheri
return true;
}
-static void free_rfkill(gpointer data)
-{
-struct connman_rfkill *rfkill = data;
-
-g_free(rfkill);
-}
-
static void technology_load(struct connman_technology *technology)
{
GKeyFile *keyfile;
@@ -1764,18 +1757,26 @@ int __connman_technology_remove_rfkill(u
g_hash_table_remove(rfkill_list, GINT_TO_POINTER(index));
-technology = technology_find(type);
-if (!technology)
-return -ENXIO;
+return 0;
+}
-technology_apply_rfkill_change(technology,
-technology-softblocked, !technology-hardblocked, false);
+static void free_rfkill(gpointer data)
+{
+struct connman_rfkill *rfkill = data;
+struct connman_technology *technology;
-technology_put(technology);
+technology = technology_find(rfkill-type);
+if (technology) {
+technology_apply_rfkill_change(technology,
+technology-softblocked, !technology-hardblocked, false);
-return 0;
+technology_put(technology);
+}
+
+g_free(rfkill);
}
+
int __connman_technology_init(void)
{
DBG();
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH] Release references to technologies held by rfkill during cleanup.
---
src/connman.h| 1 +
src/rfkill.c | 2 ++
src/technology.c | 15 +++
3 files changed, 18 insertions(+)
diff --git a/src/connman.h b/src/connman.h
index da01215..bab013b 100644
--- a/src/connman.h
+++ b/src/connman.h
@@ -826,6 +826,7 @@ void __connman_service_save(struct connman_service
*service);
#include connman/notifier.h
+void __connman_technology_cleanup_rfkill(void);
int __connman_technology_init(void);
void __connman_technology_cleanup(void);
diff --git a/src/rfkill.c b/src/rfkill.c
index 960cfea..2f073a9 100644
--- a/src/rfkill.c
+++ b/src/rfkill.c
@@ -232,4 +232,6 @@ void __connman_rfkill_cleanup(void)
g_io_channel_unref(channel);
channel = NULL;
+
+ __connman_technology_cleanup_rfkill();
}
diff --git a/src/technology.c b/src/technology.c
index d80d9e6..5917c59 100644
--- a/src/technology.c
+++ b/src/technology.c
@@ -1776,6 +1776,21 @@ int __connman_technology_remove_rfkill(unsigned
int index,
return 0;
}
+gboolean find_first(gpointer key, gpointer value, gpointer user_data)
+{
+ return TRUE;
+}
+
+void __connman_technology_cleanup_rfkill(void)
+{
+ struct connman_rfkill *rfkill;
+
+ DBG();
+
+ while (rfkill = g_hash_table_find(rfkill_list, find_first, NULL))
+ __connman_technology_remove_rfkill(rfkill-index, rfkill-type);
+}
+
int __connman_technology_init(void)
{
DBG();
--
1.9.1
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] Release references to technologies held by rfkill during cleanup.
I have written a UI for connman an ran into an bug when restarting connman. When restarting, most technologies send the TechnologyRemoved signal via DBus. Bluetooth, however, never sent this signal, so when connman restarted I had two bluetooth technology entries in my UI. I traced it down to the fact that the rfkill_list in technology.c is static and never releases the technology by calling put_technology when cleaning up. So, this patch removes all rfkills in the list on cleanup so that all of the technologies are removed. ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
