Re: [PATCH 3/8] gprs-provision: add driver API sources

[Marcel Holtmann <marcel@holtmann.org>]

[-- Attachment #1: Type: text/plain, Size: 6251 bytes --]

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