Hi Samuel,
Thanks for reviewing. Response inline.
On Tue, Mar 1, 2011 at 2:13 AM, Samuel Ortiz <sa...@linux.intel.com> wrote:
> Hi Alok,
>
> On Sat, Feb 19, 2011 at 01:07:34AM +0200, Alok Barsode wrote:
>> From: Alok Barsode <alok.bars...@nokia.com>
>>
>> EnableTechnology used to traverse the whole element tree enabling devices.
>> Instead traverse technology->device_list and enable each device.
> I would like this one and patch #3 to be merged. Now my comments:
Ok. will merge the patches.
>
>> --- a/src/connman.h
>> +++ b/src/connman.h
>> @@ -180,7 +180,6 @@ struct connman_device
>> *__connman_element_get_device(struct connman_element *elem
>>
>> struct connman_device *__connman_element_find_device(enum
>> connman_service_type type);
>> int __connman_element_request_scan(enum connman_service_type type);
>> -int __connman_element_enable_technology(enum connman_service_type type);
>> int __connman_element_disable_technology(enum connman_service_type type);
>>
>> gboolean __connman_element_device_isfiltered(const char *devname);
>> @@ -316,6 +315,8 @@ void __connman_technology_list(DBusMessageIter *iter,
>> void *user_data);
>> int __connman_technology_add_device(struct connman_device *device);
>> int __connman_technology_remove_device(struct connman_device *device);
>> int __connman_technology_enable(enum connman_service_type type);
>> +int technology_enable(enum connman_service_type type);
>> +
> All prototypes in connman.h are prefixed with __connman, which means an API
> available to ConnMan core's code.
Agreed. But we already have a __connman_technology_enable. Can you
suggest a different
name?
>> diff --git a/src/manager.c b/src/manager.c
>> index 0a11f5d..2c7d632 100644
>> --- a/src/manager.c
>> +++ b/src/manager.c
>> @@ -329,7 +329,7 @@ static DBusMessage *enable_technology(DBusConnection
>> *conn,
>> technology_enabled = TRUE;
>> technology_pending = dbus_message_ref(msg);
>>
>> - err = __connman_element_enable_technology(type);
>> + err = technology_enable(type);
> I like the idea of avoiding the element node traversal, btw.
Thanks !
>
>> @@ -674,6 +674,32 @@ int __connman_technology_remove_device(struct
>> connman_device *device)
>> return 0;
>> }
>>
>> +int technology_enable(enum connman_service_type type)
>> +{
>> + GSList *list;
>> + struct connman_technology *technology;
>> +
>> + technology = technology_find(type);
>> + if (technology == NULL)
>> + return -ENXIO;
>> +
>> + if (technology->device_list == NULL)
>> + return -ENODEV;
>> +
>> + for (list = technology->device_list; list; list = list->next) {
>> + struct connman_device *device = list->data;
>> +
>> + /* Tracking error from every device is not possible.
>> + hence ignore the return value */
> 1) My favourite style for multi line comments is:
>
> /*
> * bla bla
> * bla bla
> */
Sure ! will update all multi line comments to follow this.
> 2) You don't need to track every device, but you must track if at least one
> device succeeded or returned -EINPROGRESS. If they all failed, your routine
> should return the appropriate error value, which is not 0 and not
> -EINPROGRESS.
Sounds good to me.will fix this as well.
>
> Cheers,
> Samuel.
Cheers,
Alok.
>
> --
> Intel Open Source Technology Centre
> http://oss.intel.com/
>
_______________________________________________
connman mailing list
connman@connman.net
http://lists.connman.net/listinfo/connman
