Hi Pekka, The patch looks good to me, I just have some minor comments:
On Fri, Aug 13, 2010 at 09:14:55PM +0300, [email protected] wrote: > From: Pekka Pessi <[email protected]> > > Log more Ofono errors. Your log message should give more details related to the patch subject. > --- > plugins/ofono.c | 207 +++++++++++++++++++----------------------------------- > 1 files changed, 73 insertions(+), 134 deletions(-) > > diff --git a/plugins/ofono.c b/plugins/ofono.c > index 2f67a99..9e06ea8 100644 > --- a/plugins/ofono.c > +++ b/plugins/ofono.c > @@ -3,6 +3,7 @@ > * Connection Manager > * > * Copyright (C) 2007-2010 Intel Corporation. All rights reserved. > + * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies). > * > * This program is free software; you can redistribute it and/or modify > * it under the terms of the GNU General Public License version 2 as > @@ -78,10 +79,11 @@ static void modem_remove(struct connman_device *device) > DBG("device %p", device); > } > > -static void powered_reply(DBusPendingCall *call, void *user_data) > +static void set_ofono_property_reply(DBusPendingCall *call, void *user_data) set_property() would be enough. > { > DBusMessage *reply; > DBusError error; > + char const *name = user_data; > > DBG(""); > > @@ -90,7 +92,8 @@ static void powered_reply(DBusPendingCall *call, void > *user_data) > reply = dbus_pending_call_steal_reply(call); > > if (dbus_set_error_from_message(&error, reply)) { > - connman_error("%s", error.message); > + connman_error("SetProperty(\"%s\"): %s: %s", name, Let's be consistent with the rest of the error messages: connman_error("%s %s", name, error.message); > + error.name, error.message); > dbus_error_free(&error); > } > > @@ -99,31 +102,38 @@ static void powered_reply(DBusPendingCall *call, void > *user_data) > dbus_pending_call_unref(call); > } > > -static int gprs_change_powered(const char *path, dbus_bool_t powered) > +static int set_ofono_property(const char *path, > + const char *interface, > + const char *property, > + int type, > + void *value, > + DBusPendingCallNotifyFunction notify, > + void *user_data, > + DBusFreeFunction free_function) > { > DBusMessage *message; > DBusMessageIter iter; > DBusPendingCall *call; > > - DBG("path %s powered %d", path, powered); > + DBG("path %s %s.%s", path, interface, property); > > if (path == NULL) > return -EINVAL; > > message = dbus_message_new_method_call(OFONO_SERVICE, path, > - OFONO_GPRS_INTERFACE, SET_PROPERTY); > + interface, SET_PROPERTY); > if (message == NULL) > return -ENOMEM; > > dbus_message_set_auto_start(message, FALSE); > > dbus_message_iter_init_append(message, &iter); > - connman_dbus_property_append_basic(&iter, "Powered", > - DBUS_TYPE_BOOLEAN, &powered); > + connman_dbus_property_append_basic(&iter, property, type, value); > > if (dbus_connection_send_with_reply(connection, message, > &call, TIMEOUT) == FALSE) { > - connman_error("Failed to change powered property"); > + connman_error("Failed to change \"%s\" property on %s", Here as well: connman_error("Failed to change %s on %s", > + property, interface); > dbus_message_unref(message); > return -EINVAL; > } > @@ -134,18 +144,32 @@ static int gprs_change_powered(const char *path, > dbus_bool_t powered) > return -EINVAL; > } > > - dbus_pending_call_set_notify(call, powered_reply, (void *)path, NULL); > + if (notify == NULL) { > + notify = set_ofono_property_reply; > + user_data = (void *)property; > + } > + > + dbus_pending_call_set_notify(call, notify, user_data, free_function); > > dbus_message_unref(message); > > return -EINPROGRESS; > } > > +static int gprs_change_powered(const char *path, dbus_bool_t powered) > +{ > + DBG("path %s powered %d", path, powered); > + > + return set_ofono_property(path, OFONO_GPRS_INTERFACE, "Powered", > + DBUS_TYPE_BOOLEAN, &powered, > + NULL, NULL, NULL); > +} > + > static int modem_enable(struct connman_device *device) > { > const char *path = connman_device_get_string(device, "Path"); > > - DBG("device %p, path, %s", device, path); > + DBG("device %p, path %s", device, path); Even better: DBG("device %p path %p", device, path); But in fact I'd rather not see this kind of cosmetic changes being part of this patch. If you want to fix the ofono plugin DBG strings, please send a separate patch. > return gprs_change_powered(path, TRUE); > } > @@ -538,77 +562,23 @@ done: > static int set_network_active(struct connman_network *network, > dbus_bool_t active) > { > - DBusMessage *message; > - DBusPendingCall *call; > - DBusMessageIter iter; > + int error; > > const char *path = connman_network_get_string(network, "Path"); > > DBG("network %p, path %s, active %d", network, path, active); > > - if (path == NULL) > - return -EINVAL; > - > - message = dbus_message_new_method_call(OFONO_SERVICE, path, > - OFONO_PRI_CONTEXT_INTERFACE, SET_PROPERTY); > - if (message == NULL) > - return -ENOMEM; > - > - dbus_message_set_auto_start(message, FALSE); > - > - dbus_message_iter_init_append(message, &iter); > - connman_dbus_property_append_basic(&iter, "Active", > - DBUS_TYPE_BOOLEAN, &active); > - > - if (dbus_connection_send_with_reply(connection, message, > - &call, TIMEOUT * 10) == FALSE) { > - connman_error("Failed to connect service"); > - dbus_message_unref(message); > - return -EINVAL; > - } > - > - if (call == NULL) { > - connman_error("D-Bus connection not available"); > - dbus_message_unref(message); > - return -EINVAL; > - } > - > - connman_network_ref(network); > - > - dbus_pending_call_set_notify(call, set_active_reply, network, NULL); > - > - dbus_message_unref(message); > - > - if (active == TRUE) > - return -EINPROGRESS; > - > - return 0; > -} > - > -static void set_apn_reply(DBusPendingCall *call, void *user_data) > -{ > - DBusMessage *reply; > - DBusError error; > - > - reply = dbus_pending_call_steal_reply(call); > - > - dbus_error_init(&error); > - if (dbus_set_error_from_message(&error, reply)) { > - connman_error("%s", error.message); > + error = set_ofono_property(path, OFONO_PRI_CONTEXT_INTERFACE, > + "Active", DBUS_TYPE_BOOLEAN, &active, > + set_active_reply, network, NULL); > + if (active == FALSE && error == -EINPROGRESS) > + error = 0; > > - dbus_error_free(&error); > - } > - > - dbus_message_unref(reply); > - > - dbus_pending_call_unref(call); > + return error; > } > > static void set_apn(struct connman_network *network) > { > - DBusMessage *message; > - DBusPendingCall *call; > - DBusMessageIter iter; > const char *apn, *path; > > apn = connman_network_get_string(network, "Cellular.APN"); > @@ -621,32 +591,9 @@ static void set_apn(struct connman_network *network) > > DBG("path %s, apn %s", path, apn); > > - message = dbus_message_new_method_call(OFONO_SERVICE, path, > - OFONO_PRI_CONTEXT_INTERFACE, SET_PROPERTY); > - if (message == NULL) > - return; > - > - dbus_message_set_auto_start(message, FALSE); > - > - dbus_message_iter_init_append(message, &iter); > - connman_dbus_property_append_basic(&iter, "AccessPointName", > - DBUS_TYPE_STRING, &apn); > - > - if (dbus_connection_send_with_reply(connection, message, > - &call, TIMEOUT) == FALSE) { > - dbus_message_unref(message); > - return; > - } > - > - if (call == NULL) { > - connman_error("D-Bus connection not available"); > - dbus_message_unref(message); > - return; > - } > - > - dbus_pending_call_set_notify(call, set_apn_reply, NULL, NULL); > - > - dbus_message_unref(message); > + set_ofono_property(path, OFONO_PRI_CONTEXT_INTERFACE, > + "AccessPointName", DBUS_TYPE_STRING, &apn, > + NULL, NULL, NULL); > } > > static int network_connect(struct connman_network *network) > @@ -1068,44 +1015,11 @@ done: > > static int modem_change_powered(const char *path, dbus_bool_t powered) > { > - DBusMessage *message; > - DBusMessageIter iter; > - DBusPendingCall *call; > - > DBG("path %s powered %d", path, powered); > > - if (path == NULL) > - return -EINVAL; > - > - message = dbus_message_new_method_call(OFONO_SERVICE, path, > - OFONO_MODEM_INTERFACE, SET_PROPERTY); > - if (message == NULL) > - return -ENOMEM; > - > - dbus_message_set_auto_start(message, FALSE); > - > - dbus_message_iter_init_append(message, &iter); > - connman_dbus_property_append_basic(&iter, "Powered", > - DBUS_TYPE_BOOLEAN, &powered); > - > - if (dbus_connection_send_with_reply(connection, message, > - &call, TIMEOUT) == FALSE) { > - connman_error("Failed to change powered property"); > - dbus_message_unref(message); > - return -EINVAL; > - } > - > - if (call == NULL) { > - connman_error("D-Bus connection not available"); > - dbus_message_unref(message); > - return -EINVAL; > - } > - > - dbus_pending_call_set_notify(call, powered_reply, NULL, NULL); > - > - dbus_message_unref(message); > - > - return -EINPROGRESS; > + return set_ofono_property(path, OFONO_MODEM_INTERFACE, "Powered", > + DBUS_TYPE_BOOLEAN, &powered, > + NULL, NULL, NULL); > } > > static struct modem_data *add_modem(const char *path) > @@ -1115,6 +1029,8 @@ static struct modem_data *add_modem(const char *path) > if (path == NULL) > return NULL; > > + DBG(""); > + Same here, please remove that from this patch. > modem = g_hash_table_lookup(modem_hash, path); > if (modem != NULL) { > modem->available = TRUE; > @@ -1158,6 +1074,7 @@ static gboolean modem_has_gprs(DBusMessageIter *array) > static void modem_properties_reply(DBusPendingCall *call, void *user_data) > { > DBusMessage *reply; > + DBusError error; > DBusMessageIter array, dict; > const char *path = user_data; > > @@ -1165,6 +1082,16 @@ static void modem_properties_reply(DBusPendingCall > *call, void *user_data) > > reply = dbus_pending_call_steal_reply(call); > > + dbus_error_init(&error); > + > + if (dbus_set_error_from_message(&error, reply)) { > + connman_error("%s.%s(%s): %s: %s", Not consistent with the rest of the code, it could just be: connman_error("%s.%s %s", OFONO_MODEM_INTERFACE, GET_PROPERTIES, error.message); > + OFONO_MODEM_INTERFACE, GET_PROPERTIES, path, > + error.name, error.message); > + dbus_error_free(&error); > + goto done; > + } > + This is a valuable change, but it's mixed with your set_property() code factorizing change. It should come from a separate patch. > if (dbus_message_iter_init(reply, &array) == FALSE) > goto done; > > @@ -1195,7 +1122,6 @@ static void modem_properties_reply(DBusPendingCall > *call, void *user_data) > if (modem_has_gprs(&value) == TRUE) > get_imsi(path); > } > - Please remove that change. > dbus_message_iter_next(&dict); > } > > @@ -1269,6 +1195,8 @@ static void update_modems(DBusMessageIter *array) > { > DBusMessageIter entry; > > + DBG(""); > + > dbus_message_iter_recurse(array, &entry); > > modems_set_unavailable(); > @@ -1293,12 +1221,23 @@ static void update_modems(DBusMessageIter *array) > static void manager_properties_reply(DBusPendingCall *call, void *user_data) > { > DBusMessage *reply; > + DBusError error; > DBusMessageIter array, dict; > > DBG(""); > > reply = dbus_pending_call_steal_reply(call); > > + dbus_error_init(&error); > + > + if (dbus_set_error_from_message(&error, reply)) { > + connman_error("%s.%s(%s): %s: %s", > + OFONO_MANAGER_INTERFACE, GET_PROPERTIES, "/", > + error.name, error.message); > + dbus_error_free(&error); > + goto done; > + } > + Same comments as above. Please have all those error message display part of a separate patch. Cheers, Samuel. > if (dbus_message_iter_init(reply, &array) == FALSE) > goto done; > > -- > 1.7.0.4 > > _______________________________________________ > connman mailing list > [email protected] > http://lists.connman.net/listinfo/connman -- Intel Open Source Technology Centre http://oss.intel.com/ _______________________________________________ connman mailing list [email protected] http://lists.connman.net/listinfo/connman
