Re: [PATCH 3/9] lte: add implementation for LTE atom

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Dragos,

On 11/09/2016 11:43 AM, Dragos Tatulea wrote:
> This implementation can only get/set the default APN setting. But
> anything expected for this atom is there:
> * D-Bus interface
> * sync-ing settings to/from file
> * interaction with driver
> ---
>   src/lte.c | 341 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 341 insertions(+)
>   create mode 100644 src/lte.c
>
> diff --git a/src/lte.c b/src/lte.c
> new file mode 100644
> index 0000000..cde0496
> --- /dev/null
> +++ b/src/lte.c
> @@ -0,0 +1,341 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  Copyright (C) 2016  Endocode AG. All rights reserved.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  published by the Free Software Foundation.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <string.h>
> +#include <errno.h>
> +
> +#include <glib.h>
> +#include <gdbus.h>
> +
> +#include "ofono.h"
> +
> +#include "common.h"
> +#include "log.h"
> +#include "gprs-context.h"
> +#include "storage.h"
> +
> +#include "lte.h"
> +
> +#define SETTINGS_STORE "lte"
> +#define SETTINGS_GROUP "Settings"
> +
> +struct ofono_lte {
> +	const struct ofono_lte_driver *driver;
> +	void *driver_data;
> +	struct ofono_atom *atom;
> +	char *imsi;
> +	GKeyFile *settings;
> +	struct ofono_lte_context_config config;
> +};
> +
> +static GSList *g_drivers = NULL;
> +
> +static void lte_load_settings(struct ofono_lte *lte)
> +{
> +	char *apn;
> +
> +	if (lte->imsi == NULL)
> +		return;
> +
> +	lte->settings = storage_open(lte->imsi, SETTINGS_STORE);
> +
> +	if (lte->settings == NULL) {
> +		ofono_error("LTE: Can't open settings file, "
> +				"changes won't be persistent");
> +		return;
> +	}
> +
> +	apn = g_key_file_get_string(lte->settings, SETTINGS_GROUP ,
> +					"DefaultAPN", NULL);
> +	if (apn) {
> +		strcpy(lte->config.apn, apn);
> +
> +		if (lte->driver)
> +			lte->driver->config(lte, &lte->config, NULL, NULL);

We should only register the atom after restoring the settings succeeds. 
  radio-settings.c is a good example of this.

> +	}
> +}
> +
> +static void append_lte_properties(struct ofono_lte *lte,
> +					DBusMessageIter *dict)
> +{
> +	const char *apn = lte->config.apn;
> +
> +	ofono_dbus_dict_append(dict, "DefaultAPN", DBUS_TYPE_STRING, &apn);
> +}
> +
> +static DBusMessage *lte_get_properties(DBusConnection *conn,
> +					DBusMessage *msg, void *data)
> +{
> +	struct ofono_lte *lte = data;
> +	DBusMessage *reply;
> +	DBusMessageIter iter;
> +	DBusMessageIter dict;
> +
> +	reply = dbus_message_new_method_return(msg);
> +	if (reply == NULL)
> +		return NULL;
> +
> +	dbus_message_iter_init_append(reply, &iter);
> +
> +	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
> +					OFONO_PROPERTIES_ARRAY_SIGNATURE,
> +					&dict);
> +	append_lte_properties(lte, &dict);

Why bother with this function?  Can't we just append the properties here?

> +	dbus_message_iter_close_container(&iter, &dict);
> +
> +	return reply;
> +}
> +
> +static DBusMessage *lte_set_default_apn(struct ofono_lte *lte,
> +				DBusConnection *conn, DBusMessage *msg,
> +				const char *apn)
> +{
> +	const char *path = __ofono_atom_get_path(lte->atom);
> +
> +	if (strlen(apn) > OFONO_GPRS_MAX_APN_LENGTH)
> +		return __ofono_error_invalid_format(msg);
> +
> +	if (g_str_equal(apn, lte->config.apn))
> +		return dbus_message_new_method_return(msg);
> +
> +	/* We do care about empty value: it can be used for reset. */
> +	if (is_valid_apn(apn) == FALSE && apn[0] != '\0')
> +		return __ofono_error_invalid_format(msg);
> +
> +	strcpy(lte->config.apn, apn);
> +
> +	if (lte->settings) {
> +		if (strlen(apn) == 0)
> +			/* Clear entry on empty APN. */
> +			g_key_file_remove_key(lte->settings, SETTINGS_GROUP,
> +						"DefaultAPN", NULL);
> +		else
> +			g_key_file_set_string(lte->settings, SETTINGS_GROUP,
> +						"DefaultAPN", lte->config.apn);
> +
> +		storage_sync(lte->imsi, SETTINGS_STORE, lte->settings);
> +	}
> +
> +	if (lte->driver)
> +		lte->driver->config(lte, &lte->config, NULL, NULL);
> +
> +	if (msg)
> +		g_dbus_send_reply(conn, msg, DBUS_TYPE_INVALID);

This setup is a bit unusual from the rest of oFono design where we reply 
to the message & update settings from within the driver callback.  e.g. 
look at cbs.c and how it sets "Topics".

> +
> +	ofono_dbus_signal_property_changed(conn, path,
> +					OFONO_CONNECTION_CONTEXT_INTERFACE,
> +					"DefaultAPN",
> +					DBUS_TYPE_STRING, &apn);
> +
> +	return NULL;
> +
> +}
> +
> +static DBusMessage *lte_set_property(DBusConnection *conn,
> +					DBusMessage *msg, void *data)
> +{
> +	struct ofono_lte *lte = data;
> +	DBusMessageIter iter;
> +	DBusMessageIter var;
> +	const char *property;
> +	const char *str;
> +
> +	if (!dbus_message_iter_init(msg, &iter))
> +		return __ofono_error_invalid_args(msg);
> +
> +	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
> +		return __ofono_error_invalid_args(msg);
> +
> +	dbus_message_iter_get_basic(&iter, &property);
> +	dbus_message_iter_next(&iter);
> +
> +	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_VARIANT)
> +		return __ofono_error_invalid_args(msg);
> +
> +	dbus_message_iter_recurse(&iter, &var);
> +
> +

no double empty lines please

> +	if (!strcmp(property, "DefaultAPN")) {
> +		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
> +			return __ofono_error_invalid_args(msg);
> +
> +		dbus_message_iter_get_basic(&var, &str);
> +
> +		return lte_set_default_apn(lte, conn, msg, str);
> +	}
> +
> +

as above

> +	return __ofono_error_invalid_args(msg);
> +}
> +
> +static const GDBusMethodTable lte_methods[] = {
> +	{ GDBUS_METHOD("GetProperties",
> +			NULL, GDBUS_ARGS({ "properties", "a{sv}" }),
> +			lte_get_properties) },
> +	{ GDBUS_ASYNC_METHOD("SetProperty",
> +			GDBUS_ARGS({ "property", "s" }, { "value", "v" }),
> +			NULL, lte_set_property) },
> +	{ }
> +};
> +
> +static const GDBusSignalTable lte_signals[] = {
> +	{ GDBUS_SIGNAL("PropertyChanged",
> +			GDBUS_ARGS({ "name", "s" }, { "value", "v" })) },
> +	{ }
> +};
> +
> +static void lte_atom_remove(struct ofono_atom *atom)
> +{
> +	struct ofono_lte *lte = __ofono_atom_get_data(atom);
> +
> +	DBG("atom: %p", atom);
> +
> +	if (lte == NULL)
> +		return;
> +
> +	if (lte->settings) {
> +		storage_close(lte->imsi, SETTINGS_STORE, lte->settings, TRUE);
> +
> +		g_free(lte->imsi);
> +		lte->imsi = NULL;
> +		lte->settings = NULL;
> +	}
> +
> +	if (lte->driver && lte->driver->remove)
> +		lte->driver->remove(lte);
> +
> +	g_free(lte);
> +}
> +
> +

Double empty line ;)

> +struct ofono_lte *ofono_lte_create(struct ofono_modem *modem,
> +					const char *driver, void *data)
> +{
> +	struct ofono_lte *lte;
> +	GSList *l;
> +
> +	if (driver == NULL)
> +		return NULL;
> +
> +	lte = g_try_new0(struct ofono_lte, 1);
> +
> +	if (lte == NULL)
> +		return NULL;
> +
> +	lte->atom = __ofono_modem_add_atom(modem, OFONO_ATOM_TYPE_LTE,
> +						lte_atom_remove, lte);
> +
> +	for (l = g_drivers; l; l = l->next) {
> +		const struct ofono_lte_driver *drv = l->data;
> +
> +		if (g_strcmp0(drv->name, driver))
> +			continue;
> +
> +		if (drv->probe(lte, data) < 0)
> +			continue;
> +
> +		lte->driver = drv;
> +		break;
> +	}
> +
> +	DBG("LTE atom created");
> +
> +	return lte;
> +}
> +
> +int ofono_lte_driver_register(const struct ofono_lte_driver *d)
> +{
> +	DBG("driver: %p, name: %s", d, d->name);
> +
> +	if (d->probe == NULL)
> +		return -EINVAL;
> +
> +	g_drivers = g_slist_prepend(g_drivers, (void *) d);
> +
> +	return 0;
> +}
> +
> +void ofono_lte_driver_unregister(const struct ofono_lte_driver *d)
> +{
> +	DBG("driver: %p, name: %s", d, d->name);
> +
> +	g_drivers = g_slist_remove(g_drivers, (void *) d);
> +}
> +
> +static void lte_atom_unregister(struct ofono_atom *atom)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	struct ofono_modem *modem = __ofono_atom_get_modem(atom);
> +	const char *path = __ofono_atom_get_path(atom);
> +
> +	ofono_modem_remove_interface(modem, OFONO_LTE_INTERFACE);
> +	g_dbus_unregister_interface(conn, path, OFONO_LTE_INTERFACE);
> +}
> +
> +void ofono_lte_register(struct ofono_lte *lte)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	struct ofono_modem *modem = __ofono_atom_get_modem(lte->atom);
> +	const char *path = __ofono_atom_get_path(lte->atom);
> +	struct ofono_sim *sim = __ofono_atom_find(OFONO_ATOM_TYPE_SIM, modem);
> +	const char *imsi = ofono_sim_get_imsi(sim);
> +
> +	if (imsi == NULL) {
> +		ofono_error("No sim atom required for registering LTE atom.");	
> +		return;
> +	}
> +
> +	lte->imsi = g_strdup(imsi);
> +
> +	lte_load_settings(lte);
> +
> +	if (!g_dbus_register_interface(conn, path,
> +				OFONO_LTE_INTERFACE,
> +				lte_methods, lte_signals, NULL,
> +				lte, NULL)) {
> +		ofono_error("Could not create %s interface",
> +				OFONO_LTE_INTERFACE);
> +		return;
> +	}
> +
> +	ofono_modem_add_interface(modem, OFONO_LTE_INTERFACE);
> +
> +	__ofono_atom_register(lte->atom, lte_atom_unregister);
> +}
> +
> +void ofono_lte_remove(struct ofono_lte *lte)
> +{
> +	__ofono_atom_free(lte->atom);
> +}
> +
> +void ofono_lte_set_data(struct ofono_lte *lte, void *data)
> +{
> +	lte->driver_data = data;
> +}
> +
> +void *ofono_lte_get_data(struct ofono_lte *lte)
> +{
> +	return lte->driver_data;
> +}
>

Regards,
-Denis