Hi Jukka,
I am just making some minor style comments right now.
<snip>
> +struct gprs_provision_request {
> + GSList *drivers; /* Provisioning drivers to be called */
> + struct ofono_modem *modem;
> + ofono_gprs_provision_cb_t cb;
> + void *user_data;
> +};
> +
> +static void settings_cb(GSList *settings, void *user_data);
> +
> +static struct ofono_gprs_provision_context *gprs_provision_context_create(
> + struct ofono_modem *modem,
> + struct ofono_gprs_provision_driver *driver)
> +{
> + struct ofono_gprs_provision_context *context;
> +
> + if (driver->probe == NULL)
> + return NULL;
> +
> + context = g_try_new0(struct ofono_gprs_provision_context, 1);
> +
No empty line here please. Checking for a memory allocation result
should be done right away.
We might have some leftover cases from the earlier days. So bonus points
if you find them and fix them ;)
> + if (context == NULL)
> + return NULL;
> +
> + context->driver = driver;
> + context->modem = modem;
> +
> + if (driver->probe(context) < 0) {
> + g_free(context);
> + return NULL;
> + }
> +
> + return context;
> +}
> +
> +static void clean_active_requests(gpointer data, gpointer user_data)
> +{
> + struct gprs_provision_request *req = data;
> + struct ofono_gprs_provision_context *context = user_data;
> +
> + req->drivers = g_slist_remove(req->drivers, context);
> +}
> +
> +static void context_remove(struct ofono_atom *atom)
> +{
> + struct ofono_gprs_provision_context *context =
> + __ofono_atom_get_data(atom);
> +
> + g_slist_foreach(provision_requests, clean_active_requests, context);
> +
> + if (context->driver->remove)
> + context->driver->remove(context);
> +
> + g_free(context);
> +}
> +
> +void __ofono_gprs_provision_probe_drivers(struct ofono_modem *modem)
> +{
> + struct ofono_gprs_provision_driver *driver;
> + struct ofono_gprs_provision_context *context;
> + GSList *l;
> +
> + for (l = g_drivers; l; l = l->next) {
> + driver = l->data;
> +
> + context = gprs_provision_context_create(modem, driver);
> + if (context == NULL)
> + continue;
> +
> + __ofono_modem_add_atom(modem, OFONO_ATOM_TYPE_GPRS_PROVISION,
> + context_remove, context);
> + }
> +}
> +
> +void ofono_gprs_provision_data_free(struct ofono_gprs_provision_data *data)
> +{
> + if (data == NULL)
> + return;
> +
> + free(data->name);
> + free(data->apn);
> + free(data->username);
> + free(data->password);
> + free(data->message_proxy);
> + free(data->message_center);
Use g_free for all of them please.
> + g_free(data);
> +}
As mentioned in the other reply. This sounds more like an internal API
detail to me.
> + * Calls next driver that has callable get_settings()
> + * Returns TRUE if a driver was called.
> + */
> +static gboolean call_driver_get_settings(struct gprs_provision_request *req,
> + ofono_gprs_provision_cb_t cb)
> +{
> + struct ofono_gprs_provision_context *context;
> +
> + if (req->drivers == NULL)
> + return FALSE;
What is this check for? Wouldn't a
while (req->drivers != NULL) {
}
return FALSE;
just work fine as well.
> +
> + do {
> + context = req->drivers->data;
> + req->drivers = g_slist_delete_link(req->drivers, req->drivers);
> +
> + if (context->driver->get_settings != NULL) {
> + DBG("Calling provisioning plugin '%s'",
> + context->driver->name);
> +
> + provision_requests = g_slist_append(provision_requests,
> + req);
> + context->driver->get_settings(context, cb, req);
> + return TRUE;
> + }
> +
> + } while (req->drivers != NULL);
> +
> + return FALSE;
> +}
> +
> +static void settings_cb(GSList *settings, void *user_data)
> +{
> + struct gprs_provision_request *req = user_data;
> +
> + provision_requests = g_slist_remove(provision_requests, req);
> + if (settings == NULL) {
> + DBG("Provisioning plugin returned no settings");
This sounds more like an ofono_warn message. Especially if you can also
print out the settings driver name.
> + /* No success from this driver, try next */
> + if (call_driver_get_settings(req, settings_cb) == TRUE)
> + return;
> + } else
> + DBG("Provisioning plugin returned settings for %d contexts",
> + g_slist_length(settings));
> +
> + req->cb(settings, req->user_data);
> + g_slist_free(req->drivers);
> + g_free(req);
> +}
> +
> +static void prepend_provision_driver(struct ofono_atom *atom, void *data)
> +{
> + struct ofono_gprs_provision_context *context =
> + __ofono_atom_get_data(atom);
> + GSList **drivers = data;
> +
> + *drivers = g_slist_prepend(*drivers, context);
> +}
> +
> +void __ofono_gprs_provision_get_settings(struct ofono_modem *modem,
> + ofono_gprs_provision_cb_t cb,
> + void *user_data)
> +{
> + struct gprs_provision_request *req;
> +
> + req = g_try_new0(struct gprs_provision_request, 1);
> + if (req == NULL)
> + goto error;
> +
> + req->modem = modem;
> + req->cb = cb;
> + req->user_data = user_data;
> +
> + __ofono_modem_foreach_atom(modem, OFONO_ATOM_TYPE_GPRS_PROVISION,
> + prepend_provision_driver,
> + &req->drivers);
> +
> + if (call_driver_get_settings(req, settings_cb) == TRUE)
> + return;
> +
> + DBG("No callable GPRS provision drivers");
> +
> + g_slist_free(req->drivers);
> +
> +error:
> + g_free(req);
> + cb(NULL, user_data);
> +}
> +
> +static gint compare_priority(gconstpointer a, gconstpointer b)
> +{
> + const struct ofono_gprs_provision_driver *plugin1 = a;
> + const struct ofono_gprs_provision_driver *plugin2 = b;
> +
> + return plugin2->priority - plugin1->priority;
> +}
> +
> +int ofono_gprs_provision_driver_register(
> + const struct ofono_gprs_provision_driver *driver)
> +{
> + DBG("driver: %p name: %s", driver, driver->name);
> +
> + g_drivers = g_slist_insert_sorted(g_drivers, (void *) driver,
> + compare_priority);
> + return 0;
> +}
> +
> +void ofono_gprs_provision_driver_unregister(
> + const struct ofono_gprs_provision_driver *driver)
> +{
> + DBG("driver: %p name: %s", driver, driver->name);
> +
> + g_drivers = g_slist_remove(g_drivers, driver);
> +}
Regards
Marcel
_______________________________________________
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
