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

* Re: [PATCH_v7] connman: add plugin in oFono to request/release private network
  2011-06-06  9:17 [PATCH_v7] connman: add plugin in oFono to request/release private network Guillaume Zajac
@ 2011-06-06  6:17 ` Denis Kenzior
  2011-06-08  8:46   ` Guillaume Zajac
  0 siblings, 1 reply; 3+ messages in thread
From: Denis Kenzior @ 2011-06-06  6:17 UTC (permalink / raw)
  To: ofono

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

Hi Guillaume,

On 06/06/2011 04:17 AM, Guillaume Zajac wrote:
> Hi,
> 
> Changelog from v6:
> 	- This patch contains only the ConnMan plugin to request/release
> 	the private network settings.
> 	- remove unused ConnMan interfaces.
> 	- remove error field from pns_req structure.
> 	- directly call release method from plugin when a request is redundant.
> 
> ---
>  Makefile.am       |    3 +
>  plugins/connman.c |  277 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 280 insertions(+), 0 deletions(-)
>  create mode 100644 plugins/connman.c
> 
> diff --git a/Makefile.am b/Makefile.am
> index 0a7b6d5..ab28e2c 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..e6cce3f
> --- /dev/null
> +++ b/plugins/connman.c
> @@ -0,0 +1,277 @@
> +/*
> + *
> + *  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>
> +

Please get into the habit of running 'make distcheck' before submitting
patches.  The above header use is actually wrong.

First you should not use ofono.h here, instead you need to include
<ofono/log.h>, <ofono/dbus.h> and <ofono/plugin.h>.  ofono.h is strictly
for private APIs, which you're not using any of in this plugin.

Secondly, private-network.h should be <ofono/private-network.h> and you
will need to define OFONO_DBUS_API_SUBJECT_TO_CHANGE.  See just about
any plugin for an example.

> +#define CONNMAN_SERVICE			"net.connman"
> +#define CONNMAN_PATH			"/net/connman"
> +
> +#define CONNMAN_MANAGER_INTERFACE	CONNMAN_SERVICE ".Manager"
> +#define CONNMAN_MANAGER_PATH		"/"
> +
> +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;
> +};
> +
> +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;
> +		}
> +	}

Why do you check both pending and dbus_pending_call_get_completed?  You
reset pending to NULL pretty early in request_reply.  So at best
dbus_pending_call_get_completed is redundant, and at worst it will
actually cause bugs.  Think it through fully.

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

You're mixing tabs and spaces for indentation here, please fix this.
You should also follow the indentation rules of item M4.

> +
> +	if (message == NULL)
> +		goto error;
> +
> +	dbus_message_set_no_reply(message, TRUE);
> +	dbus_connection_send(connection, message, NULL);
> +

Err, are you missing something important here?  Like the fd argument?

From connman/doc:
                void ReleasePrivateNetwork(fd) [experimental]

> +error:
> +	if (message)
> +		dbus_message_unref(message);

This check is redundant.  The code would look way nicer if you do:

	dbus_message_unref(message);

error:
	g_hash_table_remove(...);

> +
> +	g_hash_table_remove(requests, &req->uid);
> +}
> +
> +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;
> +
> +	if (req->redundant == TRUE)
> +		goto release;
> +
> +	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 (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 release;
> +	}
> +
> +	req->cb(&pns, req->data);
> +
> +	dbus_message_unref(reply);
> +	dbus_pending_call_unref(call);
> +
> +	return;
> +
> +release:
> +	pns_release(req->uid);
> +error:
> +	if (pns.fd >= 0)
> +		close(pns.fd);
> +
> +	req->cb(NULL, req->data);
> +
> +	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");

Please align these in accordance with item M4 of our coding-style document.

> +
> +	if (message == NULL) {
> +		g_free(req);
> +		return -ENOMEM;
> +	}
> +
> +	if (dbus_connection_send_with_reply(connection,
> +				message, &call, 5000) == FALSE) {

Same here, you can put message on the previous line to make this easier.

> +		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;
> +
> +	dbus_pending_call_set_notify(call, request_reply,
> +							req, NULL);

why are you breaking up this line? It can all fit on the same line.

> +	g_hash_table_insert(requests, &req->uid, req);
> +	dbus_message_unref(message);
> +
> +	return 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;

The id initialization is redundant.  Either initialize it to zero at
declaration or just remember that all statics are initialized to zero
anyway.

> +	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

^ permalink raw reply	[flat|nested] 3+ messages in thread