Hi Martin,
> +static char *get_ident(const char *path)
> +{
> + char *ident = g_strdup(path);
> + char *re = ident;
> + while (*path != 0) {
> + if (*path++ == '/')
> + *ident = '_';
> +
> + ident++;
> + }
> +
> + return re;
> +}
this one is sooooo ugly. Can we not just use strrchr and look for the /
and then just use that one for now?
> +static void config_network_reply(DBusPendingCall *call, void *user_data)
> +{
> + struct connman_network *network = user_data;
> + DBusMessage *reply;
> + DBusMessageIter array, dict;
> + gboolean internet_type;
> +
> + DBG("network %p", network);
> +
> + reply = dbus_pending_call_steal_reply(call);
> + if (reply == NULL)
> + goto done;
> +
> + if (dbus_message_iter_init(reply, &array) == FALSE)
> + goto done;
> +
> + if (dbus_message_iter_get_arg_type(&array) != DBUS_TYPE_ARRAY)
> + goto done;
> +
> + dbus_message_iter_recurse(&array, &dict);
> +
> + while (dbus_message_iter_get_arg_type(&dict) == DBUS_TYPE_DICT_ENTRY) {
> + DBusMessageIter entry, value;
> + const char *key;
> +
> + dbus_message_iter_recurse(&dict, &entry);
> + dbus_message_iter_get_basic(&entry, &key);
> +
> + dbus_message_iter_next(&entry);
> + dbus_message_iter_recurse(&entry, &value);
> +
> + if (g_str_equal(key, "Name") == TRUE) {
> + const char *name;
> +
> + dbus_message_iter_get_basic(&value, &name);
> + connman_network_set_name(network, name);
> + } else if (g_str_equal(key, "Type") == TRUE) {
> + const char *type;
> +
> + dbus_message_iter_get_basic(&value, &type);
> + if (g_strcmp0(type, "internet") == 0) {
> + internet_type = TRUE;
> +
> + connman_network_set_protocol(network,
> + CONNMAN_NETWORK_PROTOCOL_IP);
> + } else {
> + internet_type = FALSE;
> +
> + connman_network_set_protocol(network,
> + CONNMAN_NETWORK_PROTOCOL_UNKNOWN);
> + }
> + break;
> + }
> +
> + dbus_message_iter_next(&dict);
> + }
Why did you add the "break" there. That is wrong since you don't really
know the order of the properties. It is a dict and there is no implied
order.
> +static gboolean pending_network_is_available(
> + struct connman_network *pending_network)
> +{
> + struct connman_device *device;
> + struct connman_network *network;
> + const char *identifier;
> + char *ident;
> +
> + device = connman_network_get_device(pending_network);
> + /*Modem is removed during waiting for active reply*/
> + if (device == NULL)
> + return FALSE;
Put the comment above the get_device and it is /* Modem ... reply */ to
make my coding style requirements for comments happy ;)
> +
> + identifier = connman_network_get_identifier(pending_network);
> +
> + ident = g_strdup(identifier);
> +
> + connman_network_unref(pending_network);
> +
> + network = connman_device_get_network(device, ident);
> +
> + g_free(ident);
> +
> + /*network is removed during waiting for active reply*/
Same here.
> + dbus_error_init(&error);
> + if (dbus_set_error_from_message(&error, reply)) {
> + int index = connman_network_get_index(network);
> +
> + if (index == -1)
> + connman_network_set_error(network,
> + CONNMAN_NETWORK_ERROR_ASSOCIATE_FAIL);
I think it is better to check for index < 0 and here the index variable
is pointless since you don't use it. So just check the return value of
get_index directly.
> +
> + if (active == TRUE)
> + return -EINPROGRESS;
> + else
> + return 0;
> +}
Please do it like this:
if (active == TRUE)
return -EINPROGRESS;
return 0;
Way easier to read ;)
> +static int network_disconnect(struct connman_network *network)
> +{
> +
> + if (connman_network_get_index(network) < 0)
> + return -ENOTCONN;
Are you selling empty lines ;)
> +static void network_remove(struct connman_network *network)
> +{
> + DBG("network %p", network);
> +}
> +
> +
Empty lines for sale. Get them, they are cheap ;)
> +static struct connman_network_driver network_driver = {
> + .name = "modem_network",
Just call it "network" or "context" or "internet". No need to fiddle the
word modem into it.
> + } else if (g_str_equal(key, "Status") == TRUE) {
> + const char *status;
> +
> + dbus_message_iter_get_basic(&value, &status);
> + /* if (g_strcmp0("roaming", status) == 0)
> + connman_device_set_roaming(device, TRUE);
> + else if(g_strcmp0("register", status) == 0)
> + connman_device_set_roaming(device, FALSE);*/
I prefer if you would just put a FIXME comment in here instead of some
code reference that is not upstream anyway.
> + modem = g_hash_table_lookup(ofono_modems, path);
> + if (modem == NULL)
> + return;
Must have overlooked that. Use modem_hash as variable name for the hash
you store the modems in. Please don't prefix these with ofono_*. That
one is reserved for the core daemon
> +static void mask_unavailable(gpointer key, gpointer value, gpointer
> user_data)
> +{
> + struct ofono_modem *modem = value;
> + modem->available = FALSE;
> +}
And here you need the empty line ;)
> +static void modems_set_unavailable()
> +{
> + g_hash_table_foreach(ofono_modems, mask_unavailable, NULL);
> +}
> +
> +static void cleanup_modem(gpointer key, gpointer value, gpointer user_data)
> +{
> + struct ofono_modem *modem = value;
> + if (modem->available == FALSE)
> + g_hash_table_remove(ofono_modems, key);
> +}
And again. So they just got lost and had to find their way to the right
spot ;)
> +static void modem_remove_device(struct ofono_modem *modem)
> +{
Another one I missed earlier. ofono_* prefix is not for use in the
plugins. Call these struct modem_data.
> + /*
> + if (g_strcmp0(status, "roaming") == 0) {
> + connman_device_set_roaming(modem->device, TRUE);
> + } else if (g_strcmp0(status, "register") == 0) {
> + connman_device_set_roaming(modem->device, FALSE);
> + }*/
Same as above.
> + index = connman_inet_ifindex(interface);
> + if (index >= 0) {
> + connman_network_set_index(
> + pending_network, index);
> +
> + connman_inet_ifup(index);
The ifup handling should be done inside oFono. So please prepare a patch
against oFono that brings the interface up after connected, but before
sending out the settings information.
> + /*Fix me: add static setting*/
> + dbus_message_iter_next(&dict);
> + }
Check how me do comments. And please use FIXME. It is easier to grep for
when trying to find them.
> +
> + if (interface == NULL) {
> + /*deactive, ofono send NULL inteface before deactive signal*/
> + int pending_index = connman_network_get_index(pending_network);
Same as above. Fix spelling and it is oFono.
> + connman_inet_ifdown(pending_index);
oFono needs to do the ifdown, too.
Regards
Marcel
_______________________________________________
connman mailing list
connman@connman.net
http://lists.connman.net/listinfo/connman
