Re: [PATCH_v6 5/5] connman: add plugin in oFono to request/release private network

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Guillaume,

On 05/19/2011 04:58 AM, Guillaume Zajac wrote:
> ---
>  Makefile.am       |    3 +
>  plugins/connman.c |  299 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 302 insertions(+), 0 deletions(-)
>  create mode 100644 plugins/connman.c
> 
> diff --git a/Makefile.am b/Makefile.am
> index e1eaf15..ffb85ae 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -339,6 +339,9 @@ builtin_sources += plugins/hfp_ag.c plugins/bluetooth.h
>  builtin_modules += dun_gw
>  builtin_sources += plugins/dun_gw.c plugins/bluetooth.h
>  
> +builtin_modules += connman
> +builtin_sources += plugins/connman.c
> +
>  builtin_sources += $(btio_sources)
>  builtin_cflags += @BLUEZ_CFLAGS@
>  builtin_libadd += @BLUEZ_LIBS@
> diff --git a/plugins/connman.c b/plugins/connman.c
> new file mode 100644
> index 0000000..5f0ad8a
> --- /dev/null
> +++ b/plugins/connman.c
> @@ -0,0 +1,299 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  Copyright (C) 2008-2011  Intel Corporation. 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 <errno.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +
> +#include <gdbus.h>
> +#include <string.h>
> +
> +#include <ofono.h>
> +#include <private-network.h>
> +
> +#define CONNMAN_SERVICE			"net.connman"
> +#define CONNMAN_PATH			"/net/connman"
> +
> +#define CONNMAN_DEBUG_INTERFACE		CONNMAN_SERVICE ".Debug"
> +#define CONNMAN_ERROR_INTERFACE		CONNMAN_SERVICE ".Error"
> +#define CONNMAN_AGENT_INTERFACE		CONNMAN_SERVICE ".Agent"
> +#define CONNMAN_COUNTER_INTERFACE	CONNMAN_SERVICE ".Counter"
> +
> +#define CONNMAN_MANAGER_INTERFACE	CONNMAN_SERVICE ".Manager"
> +#define CONNMAN_MANAGER_PATH		"/"
> +
> +#define CONNMAN_TASK_INTERFACE		CONNMAN_SERVICE ".Task"
> +#define CONNMAN_PROFILE_INTERFACE	CONNMAN_SERVICE ".Profile"
> +#define CONNMAN_SERVICE_INTERFACE	CONNMAN_SERVICE ".Service"
> +#define CONNMAN_PROVIDER_INTERFACE	CONNMAN_SERVICE ".Provider"
> +#define CONNMAN_TECHNOLOGY_INTERFACE	CONNMAN_SERVICE ".Technology"
> +#define CONNMAN_SESSION_INTERFACE	CONNMAN_SERVICE ".Session"
> +#define CONNMAN_NOTIFICATION_INTERFACE	CONNMAN_SERVICE ".Notification"
> +

Why do you bother defining all these interfaces when you only ever use
the MANAGER one?  Please remove the ones you don't need.

> +static DBusConnection *connection;
> +static GHashTable *requests;
> +static unsigned int id;
> +
> +struct pns_req {
> +	int uid;
> +	DBusPendingCall *pending;
> +	ofono_private_network_cb_t *cb;
> +	void *data;
> +	gboolean redundant;
> +	gboolean error;
> +};
> +
> +static void request_reply(DBusPendingCall *call, void *user_data)
> +{
> +	struct pns_req *req = user_data;
> +	struct ofono_private_network_settings pns;
> +	DBusMessageIter array, dict, entry;
> +	DBusMessage *reply;
> +
> +	DBG("");
> +
> +	pns.fd = -1;
> +	pns.server_ip = NULL;
> +	pns.peer_ip = NULL;
> +	pns.primary_dns = NULL;
> +	pns.secondary_dns = NULL;
> +
> +	/* request is no more pending */
> +	req->pending = NULL;
> +
> +	reply = dbus_pending_call_steal_reply(call);
> +	if (!reply)
> +		goto error;
> +
> +	if (dbus_message_get_type(reply) == DBUS_MESSAGE_TYPE_ERROR)
> +		goto error;
> +
> +	if (dbus_message_iter_init(reply, &array) == FALSE)
> +		goto error;
> +
> +	if (dbus_message_iter_get_arg_type(&array) != DBUS_TYPE_UNIX_FD)
> +		goto error;
> +
> +	dbus_message_iter_get_basic(&array, &pns.fd);
> +	DBG("Fildescriptor = %d\n", pns.fd);
> +
> +	dbus_message_iter_next(&array);
> +
> +	if (dbus_message_iter_get_arg_type(&array) != DBUS_TYPE_ARRAY)
> +		goto error;
> +
> +	dbus_message_iter_recurse(&array, &dict);
> +
> +	while (dbus_message_iter_get_arg_type(&dict) == DBUS_TYPE_DICT_ENTRY) {
> +		DBusMessageIter iter;
> +		const char *key;
> +		int type;
> +
> +		dbus_message_iter_recurse(&dict, &entry);
> +
> +		dbus_message_iter_get_basic(&entry, &key);
> +
> +		dbus_message_iter_next(&entry);
> +		dbus_message_iter_recurse(&entry, &iter);
> +
> +		type = dbus_message_iter_get_arg_type(&iter);
> +		if (type != DBUS_TYPE_STRING)
> +			break;
> +
> +		if (g_str_equal(key, "ServerIPv4")
> +				&& type == DBUS_TYPE_STRING)
> +			dbus_message_iter_get_basic(&iter, &pns.server_ip);
> +		else if (g_str_equal(key, "PeerIPv4")
> +				&& type == DBUS_TYPE_STRING)
> +			dbus_message_iter_get_basic(&iter, &pns.peer_ip);
> +		else if (g_str_equal(key, "PrimaryDNS")
> +				&& type == DBUS_TYPE_STRING)
> +			dbus_message_iter_get_basic(&iter, &pns.primary_dns);
> +		else if (g_str_equal(key, "SecondaryDNS")
> +				&& type == DBUS_TYPE_STRING)
> +			dbus_message_iter_get_basic(&iter, &pns.secondary_dns);
> +
> +		dbus_message_iter_next(&dict);
> +	}
> +
> +	if (req->redundant == TRUE)
> +		goto done;

We should be checking this much earlier, before doing all the work of
parsing the returned arguments.  You should also be sending a
ReleasePrivateNetwork call to ConnMan in this case.

> +
> +	if (pns.server_ip == NULL || pns.peer_ip == NULL ||
> +			pns.primary_dns == NULL || pns.secondary_dns == NULL ||
> +			pns.fd < 0) {
> +		ofono_error("Error while reading dictionnary...\n");
> +		goto done;
> +	}
> +
> +	req->cb(req->data, &pns);
> +
> +	dbus_message_unref(reply);
> +	dbus_pending_call_unref(call);
> +
> +	return;
> +
> +error:
> +	req->error = TRUE;
> +done:

Calling this label 'done' is a bit misleading.

> +	if (pns.fd >= 0)
> +		close(pns.fd);
> +
> +	req->cb(req->data, NULL);
> +
> +	if (reply)
> +		dbus_message_unref(reply);
> +
> +	dbus_pending_call_unref(call);
> +}
> +
> +static int pns_request(ofono_private_network_cb_t cb, void *data)
> +{
> +	DBusMessage *message;
> +	DBusPendingCall *call;
> +	struct pns_req *req;
> +
> +	DBG("");
> +
> +	req = g_try_new(struct pns_req, 1);
> +
> +	if (req == NULL)
> +		return -ENOMEM;
> +
> +	message = dbus_message_new_method_call(CONNMAN_SERVICE,
> +				     CONNMAN_MANAGER_PATH,
> +				     CONNMAN_MANAGER_INTERFACE,
> +				     "RequestPrivateNetwork");

Mixing tabs and spaces here, please fix this.

> +
> +	if (message == NULL) {
> +		g_free(req);
> +		return -ENOMEM;
> +	}
> +
> +	if (dbus_connection_send_with_reply(connection,
> +				message, &call, 5000) == FALSE) {
> +		g_free(req);
> +		dbus_message_unref(message);
> +		return -EIO;
> +	}
> +
> +	id++;
> +	req->pending = call;
> +	req->cb = cb;
> +	req->data = data;
> +	req->uid = id;
> +	req->redundant = FALSE;
> +	req->error = FALSE;
> +
> +	dbus_pending_call_set_notify(call, request_reply,
> +							req, NULL);
> +	g_hash_table_insert(requests, &req->uid, req);
> +	dbus_message_unref(message);
> +
> +	return req->uid;
> +}
> +
> +static void pns_release(int uid)
> +{
> +	DBusMessage *message = NULL;
> +	struct pns_req *req;
> +
> +	DBG("");
> +
> +	req = g_hash_table_lookup(requests, &uid);
> +	if (!req)
> +		return;
> +
> +	if (req->pending) {
> +		if (dbus_pending_call_get_completed(req->pending) == FALSE) {
> +			/*
> +			 * We want to cancel the request but we have to wait
> +			 * the response of ConnMan. So we mark request as
> +			 * redundant until we get the response, then we remove
> +			 * it from hash table.
> +			 */
> +			req->redundant = TRUE;
> +			return;
> +		}
> +	}
> +
> +	/*
> +	 * There was an error while reading response or transmitted by response
> +	 * so we don't need to call release DBus method.
> +	 */
> +	if (req->error)
> +		goto error;

Actually I don't think you need this.  We should not call release in
case Request() returned an error.  I fixed this in commit 9ff1b9f.

> +
> +	message = dbus_message_new_method_call(CONNMAN_SERVICE,
> +				     CONNMAN_MANAGER_PATH,
> +				     CONNMAN_MANAGER_INTERFACE,
> +				     "ReleasePrivateNetwork");

Mixing tabs and spaces here, please fix this

> +
> +	if (message == NULL)
> +		goto error;
> +
> +	dbus_message_set_no_reply(message, TRUE);
> +	dbus_connection_send(connection, message, NULL);
> +
> +error:
> +	if (message)
> +		dbus_message_unref(message);
> +
> +	g_hash_table_remove(requests, &req->uid);
> +}
> +
> +static struct ofono_private_network_driver pn_driver = {
> +	.name		= "ConnMan Private Network",
> +	.request	= pns_request,
> +	.release	= pns_release,
> +};
> +
> +static void remove_requests(gpointer user_data)
> +{
> +	struct pns_req *req = user_data;
> +
> +	g_free(req);
> +}
> +
> +static int connman_init(void)
> +{
> +	DBG("");
> +
> +	id = 0;
> +	connection = ofono_dbus_get_connection();
> +	requests = g_hash_table_new_full(g_int_hash, g_int_equal, NULL,
> +						remove_requests);
> +
> +	return ofono_private_network_driver_register(&pn_driver);
> +}
> +
> +static void connman_exit(void)
> +{
> +	g_hash_table_destroy(requests);
> +	ofono_private_network_driver_unregister(&pn_driver);
> +}
> +
> +OFONO_PLUGIN_DEFINE(connman, "ConnMan plugin", VERSION,
> +		OFONO_PLUGIN_PRIORITY_DEFAULT, connman_init, connman_exit)

Regards,
-Denis