Hi Daniel, On Tue, Aug 02, 2011 at 01:32:17PM +0200, Daniel Wagner wrote: > Hi Samuel, > > On 08/01/2011 11:50 AM, Samuel Ortiz wrote: > > Hi Daniel, > > > > On Sat, Jul 30, 2011 at 03:17:47PM +0200, Daniel Wagner wrote: > >> connman_device_remove_network() might be called with only > >> 1 remaining network available. In this case the service > >> will be destroyed by the hash table remove call, therefore > >> the service pointer is bogus. > >> --- > >> src/device.c | 1 + > >> 1 files changed, 1 insertions(+), 0 deletions(-) > >> > >> diff --git a/src/device.c b/src/device.c > >> index e5bd84d..f0e5b37 100644 > >> --- a/src/device.c > >> +++ b/src/device.c > >> @@ -1119,6 +1119,7 @@ int connman_device_remove_network(struct > >> connman_device *device, > >> identifier = connman_network_get_identifier(network); > >> g_hash_table_remove(device->networks, identifier); > >> > >> + service = __connman_service_lookup_by_identifier(identifier); > > If __connman_service_lookup_from_network() gives us an invalid pointer, then > > we need to fix it. I think having several ways to got from service to > > network > > is just going to be confusing. > > Nope, __connman_service_lookup_from_network() returns us a valid > pointer, but after g_hash_table_remove() has been called the pointer > might be invalid. For example when there is exactly one network for a > service, in this case the pointer is invalid but not NULL. > > The additional lookup makes sure the pointer is updated. Ok, I see.
> > Moreover, I find it difficult to match this diff with the changelog. How is > > the service pointer better with lookup_by_identifier() ? > > I agree, but how do you want to look it up? Of course we could add > something like __connman_service_get_number_of_networks() which is also > something smelly. Any ideas? What about this: diff --git a/src/device.c b/src/device.c index e5bd84d..a3718a9 100644 --- a/src/device.c +++ b/src/device.c @@ -1117,11 +1117,13 @@ int connman_device_remove_network(struct connman_device service = __connman_service_lookup_from_network(network); identifier = connman_network_get_identifier(network); - g_hash_table_remove(device->networks, identifier); + g_hash_table_steal(device->networks, identifier); if (service != NULL) __connman_service_reset_from_networks(service, device->networks) + free_network(network); + return 0; } Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ _______________________________________________ connman mailing list connman@connman.net http://lists.connman.net/listinfo/connman