Re: [PATCHv9 08/16] provision: Add provisioning plugin

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Oleg,

<snip>

> +static int provision_get_settings(const char *mcc, const char *mnc,
> +				const char *spn,
> +				struct ofono_gprs_provision_data **settings,
> +				int *count)
> +{
> +	GSList *l;
> +	GSList *apns;
> +	GError *error = NULL;
> +	int ap_count;
> +	int i;
> +
> +	DBG("Provisioning for MCC %s, MNC %s, SPN '%s'", mcc, mnc, spn);
> +
> +	apns = mbpi_lookup(mcc, mnc, FALSE, &error);
> +	if (apns == NULL) {
> +		if (error != NULL) {
> +			ofono_error("%s", error->message);
> +			g_error_free(error);
> +		}
> +
> +		return -ENOENT;
> +	}
> +
> +	ap_count = g_slist_length(apns);
> +
> +	DBG("Found %d APs", ap_count);
> +
> +	*settings = g_try_malloc_n(ap_count,
> +				sizeof(struct ofono_gprs_provision_data));

Please use g_try_new0 instead of g_try_malloc_n.  The result will be a
bit prettier.

> +	if (*settings == NULL) {
> +		ofono_error("Provisioning failed: %s", g_strerror(errno));
> +
> +		for (l = apns; l; l = l->next)
> +			mbpi_provision_data_free(l->data);
> +
> +		g_slist_free(apns);
> +
> +		return -ENOMEM;
> +	}
> +
> +	*count = ap_count;
> +
> +	for (l = apns, i = 0; l; l = l->next, i++) {
> +		struct ofono_gprs_provision_data *ap = l->data;
> +
> +		DBG("Name: '%s'", ap->name);
> +		DBG("APN: '%s'", ap->apn);
> +		DBG("Username: '%s'", ap->username);
> +		DBG("Password: '%s'", ap->password);
> +
> +		*(*settings + i) = *ap;

Please don't use this particular syntax, we prefer using memcpy for this.

> +
> +		memset(*settings + i, 0,
> +			sizeof(struct ofono_gprs_provision_data));

Do you mean to memset the contents of ap here?  The way I read this code
you're setting an entry in the return array and immediately resetting it
to 0.

> +		mbpi_provision_data_free(ap);

using g_free(ap) might be the easier solution.

> +	}
> +
> +	g_slist_free(apns);
> +
> +	return 0;
> +}
> +
> +static struct ofono_gprs_provision_driver provision_driver = {
> +	.name		= "Provisioning",
> +	.get_settings	= provision_get_settings
> +};
> +
> +static int provision_init(void)
> +{
> +	return ofono_gprs_provision_driver_register(&provision_driver);
> +}
> +
> +static void provision_exit(void)
> +{
> +	ofono_gprs_provision_driver_unregister(&provision_driver);
> +}
> +
> +OFONO_PLUGIN_DEFINE(provision, "Provisioning Plugin", VERSION,
> +			OFONO_PLUGIN_PRIORITY_DEFAULT,
> +			provision_init, provision_exit)

Regards,
-Denis