Re: [RFC PATCH 1/4] Added GPRS context provisioning driver API sources

[Marcel Holtmann <marcel@holtmann.org>]

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

Hi Jukka,

> +#ifndef __OFONO_GPRS_PROVISION_H
> +#define __OFONO_GPRS_PROVISION_H
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include "gprs-context.h"
> +
> +struct ofono_gprs_provisioning_data {
> +	enum ofono_gprs_context_type type;
> +	enum ofono_gprs_proto proto;
> +	gchar *name;
> +	gchar *apn;
> +	gchar *username;
> +	gchar *password;
> +	gchar *message_proxy;
> +	gchar *message_center;
> +};

don't bother with gchar *. Just use char * here. The gchar is
essentially bad idea. We only use it a few rare occasions or when it got
missed in a review.

> +
> +/*
> + * Callback from provisioning plugin.
> + * settings: list of struct ofono_gprs_provisioning_data
> + *
> + * It is responsibility of callback function to free settings-list
> + * settings-list elements must be freed with ofono_gprs_provisioning_data_free()
> + */
> +typedef void (*ofono_gprs_provision_cb_t)(GSList *settings, void *userdata);
> +
> +struct ofono_gprs_provision_driver {
> +	const char *name;
> +	int priority;
> +	void (*get_settings) (struct ofono_modem *modem,
> +				ofono_gprs_provision_cb_t cb,
> +				void *userdata);
> +};

So here is something we need to talk about. The nettime plugin
infrastructure using a "pseudo" atom. So do we wanna do the same here or
do we wanna change the nettime atom to something simple like this.

I like to have consistency here. Aki, Denis, thoughts?

> +struct gprs_provisioning_request {
> +	GSList *current_driver;
> +	struct ofono_modem *modem;
> +	ofono_gprs_provision_cb_t cb;
> +	void *userdata;

The sort of general style is to call this user_data.

> +};
> +
> +static GSList *gprs_provision_drivers = NULL;
> +
> +

We also normally don't do double empty lines.

> +void ofono_gprs_provisioning_data_free(struct ofono_gprs_provisioning_data *data)
> +{
> +	if (data == NULL)
> +		return;
> +
> +	g_free(data->name);
> +	g_free(data->apn);
> +	g_free(data->username);
> +	g_free(data->password);
> +	g_free(data->message_proxy);
> +	g_free(data->message_center);
> +	g_free(data);
> +}
> +
> +
> +static void settings_cb(GSList *settings, void *userdata)
> +{

Same here, please use user_data.

> +	GSList *d;
> +	struct gprs_provisioning_request *req = userdata;

And as another general rule, the variable that have an assignment like
the user_data come first.

> +
> +	if (settings != NULL)
> +		DBG("Provisioning plugin returned settings for %d contexts",
> +			g_slist_length(settings));
> +	else
> +		DBG("Provisioning plugin returned no settings");
> +
> +	d = req->current_driver->next;
> +	if (settings == NULL && d != NULL) {
> +		/* No success from this driver, try next */
> +		const struct ofono_gprs_provision_driver *driver = d->data;
> +		req->current_driver = d;
> +		DBG("Calling provisioning plugin '%s'", driver->name);
> +		driver->get_settings(req->modem, settings_cb, req);
> +		return;
> +	}
> +
> +	req->cb(settings, req->userdata);
> +	g_free(req);
> +}
> +
> +

Same here for the double empty lines. Please fix them all.

> +void ofono_gprs_provision_get_settings(struct ofono_modem *modem,
> +					ofono_gprs_provision_cb_t cb,
> +					void *userdata)
> +{
> +	struct gprs_provisioning_request *req;
> +	const struct ofono_gprs_provision_driver *driver;
> +
> +	if (gprs_provision_drivers == 0)

This needs to be a == NULL.

> +		goto error;
> +
> +	req = g_try_new0(struct gprs_provisioning_request, 1);
> +	if (req == NULL)
> +		goto error;
> +
> +	req->modem = modem;
> +	req->cb = cb;
> +	req->userdata = userdata;
> +	req->current_driver = gprs_provision_drivers;
> +
> +	driver = gprs_provision_drivers->data;
> +	DBG("Calling provisioning plugin '%s'", driver->name);

Normally we put empty lines on top and below the DBG statement. Just to
make the stand out a bit.

Also in this case we might can remove some of the debug statements once
the code has been tested.

> +	driver->get_settings(modem, settings_cb, req);
> +	return;
> +
> +error:
> +	cb(NULL, userdata);
> +}
> +
> +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);
> +
> +	gprs_provision_drivers = g_slist_insert_sorted(gprs_provision_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);
> +
> +	gprs_provision_drivers = g_slist_remove(gprs_provision_drivers, driver);
> +}

Regards

Marcel