hi, Find my comments inline.
On Tue, Sep 13, 2011 at 11:50 AM, Yu A Wang <[email protected]> wrote: > Fix BMC#13547 unable to enable/disable 3G after plug in 3G modem again > Fix as samuel's comment to add remove_rfkill to fix missing rfkill > event issue when remove_device, remove all rfkill indexes for a technology > --- > src/technology.c | 20 +++++++++++++++++--- > 1 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/src/technology.c b/src/technology.c > index 96d64af..ab41c82 100644 > --- a/src/technology.c > +++ b/src/technology.c > @@ -707,12 +707,16 @@ void __connman_technology_remove_interface(enum > connman_service_type type, > > technology = technology_find(type); > > - if (technology == NULL || technology->driver == NULL) > + if (technology == NULL) > return; > > + if (technology->driver == NULL) > + goto done; > + > if (technology->driver->remove_interface) > technology->driver->remove_interface(technology, index); > > +done: > technology_put(technology); > } > > ACK for this hunk. however i would do it this way: if (technology == NULL) return; technology_put(technology); if (technology->driver == NULL) return; IMO avoid gotos. @@ -742,6 +746,14 @@ int __connman_technology_add_device(struct > connman_device *device) > return 0; > } > > +static void remove_rfkill(gpointer key, gpointer value, gpointer > user_data) > +{ > + enum connman_service_type *type = user_data; > + > + __connman_technology_remove_rfkill(*(unsigned int *)key, *type); > + > +} > + > int __connman_technology_remove_device(struct connman_device *device) > { > struct connman_technology *technology; > NACK for this HUNK. removing and adding entries in the rfkill table is done by rfkill event loop. can u justify this? @@ -750,6 +762,7 @@ int __connman_technology_remove_device(struct > connman_device *device) > DBG("device %p", device); > > type = __connman_device_get_service_type(device); > + __connman_notifier_disable(type); > __connman_notifier_unregister(type); > > technology = technology_find(type); > NACK for this HUNK. notifier_enable/disable() is for technologies not for devices. hence they should be called from technology_enabled/technology_disabled. if there is a bug then it should be fixed in these functions. > @@ -759,8 +772,9 @@ int __connman_technology_remove_device(struct > connman_device *device) > technology->device_list = g_slist_remove(technology->device_list, > device); > if (technology->device_list == NULL) { > - technology->state = CONNMAN_TECHNOLOGY_STATE_OFFLINE; > - state_changed(technology); > + g_hash_table_foreach(technology->rfkill_list, > remove_rfkill, > + &type); > + technology_put(technology); > } > > return 0; > -- > NACK for this HUNK. again why do u need to explicitly remove the rfkill entries? these are technology wide operations and calling them from a device operation (remove device) is not correct. but i ACK the technology_put() in this HUNK. we need it for correct ref count. > 1.7.2.2 > > Also AFAIK the bug talks about the 3G dongle. if this is a USB dongle, rfkill is not involved in it at all. can you clarify? Cheers, Alok. > _______________________________________________ > connman mailing list > [email protected] > http://lists.connman.net/listinfo/connman > _______________________________________________ connman mailing list [email protected] http://lists.connman.net/listinfo/connman
