[PATCH_v2 0/4] Request private petwork creation to ConnMan

[PATCH_v2 0/4] Request private petwork creation to ConnMan

[Guillaume Zajac]

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

Hi,

This new version contains some fixes of some typos into plugins/connman.c:
	- in dun_emulator_watch() missing some brackets
	- in connman_init(), multiple return and unused variable err.

Guillaume Zajac (4):
  gatppp: Add new contructor to use external fd
  emulator: add routine to request/release private network from ConnMan
  connman: add plugin in oFono to request request/release private
    network
  Makefile: add connman plugin build

 Makefile.am        |    3 +
 gatchat/gatppp.c   |   43 +++++++++-
 gatchat/gatppp.h   |    2 +
 gatchat/ppp.h      |    2 +-
 gatchat/ppp_net.c  |   46 ++++++----
 include/emulator.h |   10 ++
 plugins/connman.c  |  235 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/emulator.c     |   75 +++++++++++++++--
 8 files changed, 387 insertions(+), 29 deletions(-)
 create mode 100644 plugins/connman.c

[PATCH_v2 1/4] gatppp: Add new contructor to use external fd

[Guillaume Zajac]

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

---
 gatchat/gatppp.c  |   43 ++++++++++++++++++++++++++++++++++++++++---
 gatchat/gatppp.h  |    2 ++
 gatchat/ppp.h     |    2 +-
 gatchat/ppp_net.c |   46 ++++++++++++++++++++++++++++------------------
 4 files changed, 71 insertions(+), 22 deletions(-)

diff --git a/gatchat/gatppp.c b/gatchat/gatppp.c
index 993b5ea..d59f69b 100644
--- a/gatchat/gatppp.c
+++ b/gatchat/gatppp.c
@@ -76,6 +76,7 @@ struct _GAtPPP {
 	gpointer debug_data;
 	gboolean sta_pending;
 	guint ppp_dead_source;
+	int fd;
 };
 
 void ppp_debug(GAtPPP *ppp, const char *str)
@@ -288,7 +289,7 @@ void ppp_auth_notify(GAtPPP *ppp, gboolean success)
 void ppp_ipcp_up_notify(GAtPPP *ppp, const char *local, const char *peer,
 					const char *dns1, const char *dns2)
 {
-	ppp->net = ppp_net_new(ppp);
+	ppp->net = ppp_net_new(ppp, ppp->fd);
 
 	if (ppp->net == NULL) {
 		ppp->disconnect_reason = G_AT_PPP_REASON_NET_FAIL;
@@ -296,8 +297,14 @@ void ppp_ipcp_up_notify(GAtPPP *ppp, const char *local, const char *peer,
 		return;
 	}
 
-	if (ppp_net_set_mtu(ppp->net, ppp->mtu) == FALSE)
-		DBG(ppp, "Unable to set MTU");
+	/*
+	 * If we have opened the tun interface locally,
+	 * we have to set a MTU value.
+	 */
+	if (ppp->fd < 0) {
+		if (ppp_net_set_mtu(ppp->net, ppp->mtu) == FALSE)
+			DBG(ppp, "Unable to set MTU");
+	}
 
 	ppp_enter_phase(ppp, PPP_PHASE_LINK_UP);
 
@@ -541,6 +548,9 @@ static GAtPPP *ppp_init_common(GAtHDLC *hdlc, gboolean is_server, guint32 ip)
 
 	ppp->ref_count = 1;
 
+	/* Default value of fd is -1 */
+	ppp->fd = -1;
+
 	/* set options to defaults */
 	ppp->mru = DEFAULT_MRU;
 	ppp->mtu = DEFAULT_MTU;
@@ -629,6 +639,33 @@ GAtPPP *g_at_ppp_server_new_from_io(GAtIO *io, const char *local)
 		return NULL;
 
 	ppp = ppp_init_common(hdlc, TRUE, ip);
+
+	g_at_hdlc_unref(hdlc);
+
+	return ppp;
+}
+
+GAtPPP *g_at_ppp_server_new_from_io_with_fd(GAtIO *io,
+						const char *local, int fd)
+{
+	GAtHDLC *hdlc;
+	GAtPPP *ppp;
+	guint32 ip;
+
+	if (local == NULL)
+		ip = 0;
+	else if (inet_pton(AF_INET, local, &ip) != 1)
+		return NULL;
+
+	hdlc = g_at_hdlc_new_from_io(io);
+	if (hdlc == NULL)
+		return NULL;
+
+	ppp = ppp_init_common(hdlc, TRUE, ip);
+
+	/* Set the fd value returned by ConnMan */
+	ppp->fd = fd;
+
 	g_at_hdlc_unref(hdlc);
 
 	return ppp;
diff --git a/gatchat/gatppp.h b/gatchat/gatppp.h
index fb5de4c..4ed4cde 100644
--- a/gatchat/gatppp.h
+++ b/gatchat/gatppp.h
@@ -54,6 +54,8 @@ GAtPPP *g_at_ppp_new(GIOChannel *modem);
 GAtPPP *g_at_ppp_new_from_io(GAtIO *io);
 GAtPPP *g_at_ppp_server_new(GIOChannel *modem, const char *local);
 GAtPPP *g_at_ppp_server_new_from_io(GAtIO *io, const char *local);
+GAtPPP *g_at_ppp_server_new_from_io_with_fd(GAtIO *io,
+						const char *local, int fd);
 
 void g_at_ppp_open(GAtPPP *ppp);
 void g_at_ppp_set_connect_function(GAtPPP *ppp, GAtPPPConnectFunc callback,
diff --git a/gatchat/ppp.h b/gatchat/ppp.h
index d2786d7..8107820 100644
--- a/gatchat/ppp.h
+++ b/gatchat/ppp.h
@@ -102,7 +102,7 @@ void ppp_chap_free(struct ppp_chap *chap);
 void ppp_chap_process_packet(struct ppp_chap *chap, const guint8 *new_packet);
 
 /* TUN / Network related functions */
-struct ppp_net *ppp_net_new(GAtPPP *ppp);
+struct ppp_net *ppp_net_new(GAtPPP *ppp, int fd);
 const char *ppp_net_get_interface(struct ppp_net *net);
 void ppp_net_process_packet(struct ppp_net *net, const guint8 *packet);
 void ppp_net_free(struct ppp_net *net);
diff --git a/gatchat/ppp_net.c b/gatchat/ppp_net.c
index 1a6cdf7..59c4b5e 100644
--- a/gatchat/ppp_net.c
+++ b/gatchat/ppp_net.c
@@ -123,12 +123,12 @@ const char *ppp_net_get_interface(struct ppp_net *net)
 	return net->if_name;
 }
 
-struct ppp_net *ppp_net_new(GAtPPP *ppp)
+struct ppp_net *ppp_net_new(GAtPPP *ppp, int fd)
 {
 	struct ppp_net *net;
 	GIOChannel *channel = NULL;
 	struct ifreq ifr;
-	int fd, err;
+	int fdesc, err;
 
 	net = g_try_new0(struct ppp_net, 1);
 	if (net == NULL)
@@ -140,23 +140,33 @@ struct ppp_net *ppp_net_new(GAtPPP *ppp)
 		return NULL;
 	}
 
-	/* open a tun interface */
-	fd = open("/dev/net/tun", O_RDWR);
-	if (fd < 0)
-		goto error;
-
-	memset(&ifr, 0, sizeof(ifr));
-	ifr.ifr_flags = IFF_TUN | IFF_NO_PI;
-	strcpy(ifr.ifr_name, "ppp%d");
-
-	err = ioctl(fd, TUNSETIFF, (void *) &ifr);
-	if (err < 0)
-		goto error;
-
-	net->if_name = strdup(ifr.ifr_name);
+	/*
+	 * If the fd value is still the default one,
+	 * open the tun interface and configure it.
+	 */
+	if (fd< 0) {
+		/* open a tun interface */
+		fdesc = open("/dev/net/tun", O_RDWR);
+		if (fdesc < 0)
+			goto error;
+
+		memset(&ifr, 0, sizeof(ifr));
+		ifr.ifr_flags = IFF_TUN | IFF_NO_PI;
+		strcpy(ifr.ifr_name, "ppp%d");
+
+		err = ioctl(fdesc, TUNSETIFF, (void *) &ifr);
+		if (err < 0)
+			goto error;
+
+		net->if_name = strdup(ifr.ifr_name);
+		/* create a channel for reading and writing to this interface */
+		channel = g_io_channel_unix_new(fdesc);
+	} else {
+		net->if_name = strdup("Server ppp");
+		/* create a channel for reading and writing to this interface */
+		channel = g_io_channel_unix_new(fd);
+	}
 
-	/* create a channel for reading and writing to this interface */
-	channel = g_io_channel_unix_new(fd);
 	if (channel == NULL)
 		goto error;
 
-- 
1.7.1

[PATCH_v2 2/4] emulator: add routine to request/release private network from ConnMan

[Guillaume Zajac]

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

---
 include/emulator.h |   10 +++++++
 src/emulator.c     |   75 +++++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 78 insertions(+), 7 deletions(-)

diff --git a/include/emulator.h b/include/emulator.h
index 5cd894b..29abf55 100644
--- a/include/emulator.h
+++ b/include/emulator.h
@@ -105,6 +105,16 @@ enum ofono_emulator_request_type ofono_emulator_request_get_type(
 void ofono_emulator_set_indicator(struct ofono_emulator *em,
 					const char *name, int value);
 
+typedef void (*network_request)(struct ofono_error *, int *,
+				const char **, const char **,
+				const char **, const char **);
+typedef void (*network_release)(void);
+
+void ofono_emulator_add_network_request_cb(struct ofono_emulator *em,
+							network_request cb);
+void ofono_emulator_add_network_release_cb(struct ofono_emulator *em,
+							network_release cb);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/src/emulator.c b/src/emulator.c
index 9b4647b..b5055ff 100644
--- a/src/emulator.c
+++ b/src/emulator.c
@@ -33,10 +33,13 @@
 #include "gatserver.h"
 #include "gatppp.h"
 
-#define DUN_SERVER_ADDRESS     "192.168.1.1"
-#define DUN_PEER_ADDRESS       "192.168.1.2"
-#define DUN_DNS_SERVER_1       "10.10.10.10"
-#define DUN_DNS_SERVER_2       "10.10.10.11"
+struct emulator_network {
+	int fd;
+	const char *server_ip;
+	const char *peer_ip;
+	const char *primary_dns;
+	const char *secondary_dns;
+};
 
 #define RING_TIMEOUT 3
 
@@ -55,6 +58,9 @@ struct ofono_emulator {
 	guint callsetup_source;
 	gboolean clip;
 	gboolean ccwa;
+	struct emulator_network *en;
+	network_request req_en_cb;
+	network_release rel_en_cb;
 };
 
 struct indicator {
@@ -78,6 +84,40 @@ static void emulator_disconnect(gpointer user_data)
 	ofono_emulator_remove(em);
 }
 
+static struct emulator_network *get_emulator_network(struct ofono_emulator *e)
+{
+	struct ofono_error error;
+	struct emulator_network *en;
+
+	en = g_try_new0(struct emulator_network, 1);
+
+	if (en == NULL)
+		return en;
+
+	if (e->req_en_cb)
+		e->req_en_cb(&error, &en->fd, &en->server_ip, &en->peer_ip,
+				&en->primary_dns, &en->secondary_dns);
+	else
+		goto error;
+
+	if (error.type == OFONO_ERROR_TYPE_FAILURE)
+		goto error;
+
+	return en;
+
+error:
+	g_free(en);
+	return NULL;
+}
+
+static void release_emulator_network(struct ofono_emulator *e)
+{
+	if (e->rel_en_cb)
+		e->rel_en_cb();
+	if (e->en)
+		g_free(e->en);
+}
+
 static void ppp_connect(const char *iface, const char *local,
 			const char *remote,
 			const char *dns1, const char *dns2,
@@ -98,6 +138,7 @@ static void ppp_disconnect(GAtPPPDisconnectReason reason, gpointer user_data)
 
 	g_at_ppp_unref(em->ppp);
 	em->ppp = NULL;
+	release_emulator_network(em);
 
 	if (em->server == NULL)
 		return;
@@ -118,14 +159,16 @@ static gboolean setup_ppp(gpointer user_data)
 
 	g_at_server_suspend(em->server);
 
-	em->ppp = g_at_ppp_server_new_from_io(io, DUN_SERVER_ADDRESS);
+	em->ppp = g_at_ppp_server_new_from_io_with_fd(io,
+							em->en->server_ip,
+							em->en->fd);
 	if (em->ppp == NULL) {
 		g_at_server_resume(em->server);
 		return FALSE;
 	}
 
-	g_at_ppp_set_server_info(em->ppp, DUN_PEER_ADDRESS,
-					DUN_DNS_SERVER_1, DUN_DNS_SERVER_2);
+	g_at_ppp_set_server_info(em->ppp, em->en->peer_ip,
+					em->en->primary_dns, em->en->secondary_dns);;
 
 	g_at_ppp_set_credentials(em->ppp, "", "");
 	g_at_ppp_set_debug(em->ppp, emulator_debug, "PPP");
@@ -143,6 +186,9 @@ static gboolean dial_call(struct ofono_emulator *em, const char *dial_str)
 	DBG("dial call %s", dial_str);
 
 	if (c == '*' || c == '#' || c == 'T' || c == 't') {
+		em->en = get_emulator_network(em);
+		if (!em->en)
+			return FALSE;
 		g_at_server_send_intermediate(em->server, "CONNECT");
 		em->source = g_idle_add(setup_ppp, em);
 	}
@@ -927,3 +973,18 @@ void ofono_emulator_set_indicator(struct ofono_emulator *em,
 		break;
 	}
 }
+
+void ofono_emulator_add_network_request_cb(struct ofono_emulator *em,
+							network_request cb)
+{
+	if (em !=NULL)
+		em->req_en_cb = cb;
+}
+
+void ofono_emulator_add_network_release_cb(struct ofono_emulator *em,
+							network_release cb)
+{
+	if (em !=NULL)
+		em->rel_en_cb = cb;
+}
+
-- 
1.7.1

[PATCH_v2 3/4] connman: add plugin in oFono to request request/release private network

[Guillaume Zajac]

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

---
 plugins/connman.c |  235 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 235 insertions(+), 0 deletions(-)
 create mode 100644 plugins/connman.c

diff --git a/plugins/connman.c b/plugins/connman.c
new file mode 100644
index 0000000..8a33d84
--- /dev/null
+++ b/plugins/connman.c
@@ -0,0 +1,235 @@
+/*
+ *
+ *  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 <gdbus.h>
+#include <string.h>
+
+#include <ofono.h>
+#include <emulator.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"
+
+static DBusConnection *connection;
+static guint modemwatch_id;
+static GList *modems;
+
+static void request_pn(struct ofono_error *error, int *out_fd,
+					const char **out_server_ip,
+					const char **out_peer_ip,
+					const char **out_primary_dns,
+					const char **out_secondary_dns)
+{
+	DBusMessageIter array, dict, entry;
+	DBusMessage *message, *reply;
+	DBusError dbus_error;
+
+	DBG("");
+
+	error->type = OFONO_ERROR_TYPE_NO_ERROR;
+
+	message = dbus_message_new_method_call(CONNMAN_SERVICE,
+				     CONNMAN_MANAGER_PATH,
+				     CONNMAN_MANAGER_INTERFACE,
+				     "RequestPrivateNetwork");
+
+	if (message == NULL)
+		goto error;
+
+	dbus_error_init(&dbus_error);
+
+	reply = dbus_connection_send_with_reply_and_block(connection, message,
+								-1, &dbus_error);
+
+	dbus_message_unref(message);
+
+	if (!reply) {
+		if (dbus_error_is_set(&dbus_error)) {
+			g_print("1. %s\n", dbus_error.message);
+			dbus_error_free(&dbus_error);
+		} else {
+			g_print("Request() failed");
+		}
+
+		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, out_fd);
+	g_print("Fildescriptor = %d\n", *out_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);
+
+		g_print("key %s", 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, out_server_ip);
+			g_print(" = %s\n", *out_server_ip);
+
+		} else if (g_str_equal(key, "PeerIPv4")
+				&& type == DBUS_TYPE_STRING) {
+			dbus_message_iter_get_basic(&iter, out_peer_ip);
+			g_print(" = %s\n", *out_peer_ip);
+
+		} else if (g_str_equal(key, "PrimaryDNS")
+				&& type == DBUS_TYPE_STRING) {
+			dbus_message_iter_get_basic(&iter, out_primary_dns);
+			g_print(" = %s\n", *out_primary_dns);
+
+		} else if (g_str_equal(key, "SecondaryDNS")
+				&& type == DBUS_TYPE_STRING) {
+			dbus_message_iter_get_basic(&iter, out_secondary_dns);
+			g_print(" = %s\n", *out_secondary_dns);
+		}
+
+		dbus_message_iter_next(&dict);
+	}
+
+	return;
+error:
+	error->type = OFONO_ERROR_TYPE_FAILURE;
+	return;
+}
+
+static void release_pn(void)
+{
+	DBusMessage *message;
+
+	DBG("");
+
+	message = dbus_message_new_method_call(CONNMAN_SERVICE,
+				     CONNMAN_MANAGER_PATH,
+				     CONNMAN_MANAGER_INTERFACE,
+				     "ReleasePrivateNetwork");
+
+	if (message == NULL)
+		return;
+
+	dbus_connection_send(connection, message, NULL);
+
+	dbus_message_unref(message);
+}
+
+static void dun_emulator_watch(struct ofono_atom *atom,
+				enum ofono_atom_watch_condition cond,
+				void *data)
+{
+	struct ofono_modem *modem = data;
+	struct ofono_emulator *em = __ofono_atom_get_data(atom);
+
+	if (cond == OFONO_ATOM_WATCH_CONDITION_REGISTERED) {
+		modems = g_list_append(modems, modem);
+
+		if (modems->next == NULL) {
+			ofono_emulator_add_network_request_cb(em, request_pn);
+			ofono_emulator_add_network_release_cb(em, release_pn);
+		}
+	} else {
+		modems = g_list_remove(modems, modem);
+	}
+}
+
+static void modem_watch(struct ofono_modem *modem, gboolean added, void *user)
+{
+	DBG("modem: %p, added: %d", modem, added);
+
+	if (added == FALSE)
+		return;
+
+	__ofono_modem_add_atom_watch(modem, OFONO_ATOM_TYPE_EMULATOR_DUN,
+					dun_emulator_watch, modem, NULL);
+}
+
+static void call_modemwatch(struct ofono_modem *modem, void *user)
+{
+	modem_watch(modem, TRUE, user);
+}
+
+static int connman_init(void)
+{
+	DBG("");
+
+	connection = ofono_dbus_get_connection();
+
+	modemwatch_id = __ofono_modemwatch_add(modem_watch, NULL, NULL);
+
+	__ofono_modem_foreach(call_modemwatch, NULL);
+
+	return 0;
+}
+
+static void connman_exit(void)
+{
+	__ofono_modemwatch_remove(modemwatch_id);
+}
+
+OFONO_PLUGIN_DEFINE(connman, "ConnMan plugin", VERSION,
+		OFONO_PLUGIN_PRIORITY_DEFAULT, connman_init, connman_exit)
-- 
1.7.1

[PATCH_v2 4/4] Makefile: add connman plugin build

[Guillaume Zajac]

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

---
 Makefile.am |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 2886b5d..86f5662 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@
-- 
1.7.1

Re: [PATCH_v2 1/4] gatppp: Add new contructor to use external fd

[Denis Kenzior]

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

Hi Guillaume,

On 04/22/2011 07:06 AM, Guillaume Zajac wrote:
> ---
>  gatchat/gatppp.c  |   43 ++++++++++++++++++++++++++++++++++++++++---
>  gatchat/gatppp.h  |    2 ++
>  gatchat/ppp.h     |    2 +-
>  gatchat/ppp_net.c |   46 ++++++++++++++++++++++++++++------------------
>  4 files changed, 71 insertions(+), 22 deletions(-)
> 
> diff --git a/gatchat/gatppp.c b/gatchat/gatppp.c
> index 993b5ea..d59f69b 100644
> --- a/gatchat/gatppp.c
> +++ b/gatchat/gatppp.c
> @@ -76,6 +76,7 @@ struct _GAtPPP {
>  	gpointer debug_data;
>  	gboolean sta_pending;
>  	guint ppp_dead_source;
> +	int fd;
>  };
>  
>  void ppp_debug(GAtPPP *ppp, const char *str)
> @@ -288,7 +289,7 @@ void ppp_auth_notify(GAtPPP *ppp, gboolean success)
>  void ppp_ipcp_up_notify(GAtPPP *ppp, const char *local, const char *peer,
>  					const char *dns1, const char *dns2)
>  {
> -	ppp->net = ppp_net_new(ppp);
> +	ppp->net = ppp_net_new(ppp, ppp->fd);
>  
>  	if (ppp->net == NULL) {
>  		ppp->disconnect_reason = G_AT_PPP_REASON_NET_FAIL;
> @@ -296,8 +297,14 @@ void ppp_ipcp_up_notify(GAtPPP *ppp, const char *local, const char *peer,
>  		return;
>  	}
>  
> -	if (ppp_net_set_mtu(ppp->net, ppp->mtu) == FALSE)
> -		DBG(ppp, "Unable to set MTU");
> +	/*
> +	 * If we have opened the tun interface locally,
> +	 * we have to set a MTU value.
> +	 */

So I don't agree with this.  The MTU is negotiated during LCP phase, so
unless I'm missing something, this value should still be set to the
negotiated one.

> +	if (ppp->fd < 0) {
> +		if (ppp_net_set_mtu(ppp->net, ppp->mtu) == FALSE)
> +			DBG(ppp, "Unable to set MTU");
> +	}
>  
>  	ppp_enter_phase(ppp, PPP_PHASE_LINK_UP);
>  
> @@ -541,6 +548,9 @@ static GAtPPP *ppp_init_common(GAtHDLC *hdlc, gboolean is_server, guint32 ip)
>  
>  	ppp->ref_count = 1;
>  
> +	/* Default value of fd is -1 */
> +	ppp->fd = -1;
> +
>  	/* set options to defaults */
>  	ppp->mru = DEFAULT_MRU;
>  	ppp->mtu = DEFAULT_MTU;
> @@ -629,6 +639,33 @@ GAtPPP *g_at_ppp_server_new_from_io(GAtIO *io, const char *local)
>  		return NULL;
>  
>  	ppp = ppp_init_common(hdlc, TRUE, ip);
> +
> +	g_at_hdlc_unref(hdlc);
> +
> +	return ppp;
> +}
> +
> +GAtPPP *g_at_ppp_server_new_from_io_with_fd(GAtIO *io,
> +						const char *local, int fd)
> +{
> +	GAtHDLC *hdlc;
> +	GAtPPP *ppp;
> +	guint32 ip;
> +
> +	if (local == NULL)
> +		ip = 0;
> +	else if (inet_pton(AF_INET, local, &ip) != 1)
> +		return NULL;
> +
> +	hdlc = g_at_hdlc_new_from_io(io);
> +	if (hdlc == NULL)
> +		return NULL;
> +
> +	ppp = ppp_init_common(hdlc, TRUE, ip);
> +
> +	/* Set the fd value returned by ConnMan */
> +	ppp->fd = fd;
> +
>  	g_at_hdlc_unref(hdlc);
>  
>  	return ppp;
> diff --git a/gatchat/gatppp.h b/gatchat/gatppp.h
> index fb5de4c..4ed4cde 100644
> --- a/gatchat/gatppp.h
> +++ b/gatchat/gatppp.h
> @@ -54,6 +54,8 @@ GAtPPP *g_at_ppp_new(GIOChannel *modem);
>  GAtPPP *g_at_ppp_new_from_io(GAtIO *io);
>  GAtPPP *g_at_ppp_server_new(GIOChannel *modem, const char *local);
>  GAtPPP *g_at_ppp_server_new_from_io(GAtIO *io, const char *local);
> +GAtPPP *g_at_ppp_server_new_from_io_with_fd(GAtIO *io,
> +						const char *local, int fd);

Please name this g_at_ppp_server_new_full()

>  
>  void g_at_ppp_open(GAtPPP *ppp);
>  void g_at_ppp_set_connect_function(GAtPPP *ppp, GAtPPPConnectFunc callback,
> diff --git a/gatchat/ppp.h b/gatchat/ppp.h
> index d2786d7..8107820 100644
> --- a/gatchat/ppp.h
> +++ b/gatchat/ppp.h
> @@ -102,7 +102,7 @@ void ppp_chap_free(struct ppp_chap *chap);
>  void ppp_chap_process_packet(struct ppp_chap *chap, const guint8 *new_packet);
>  
>  /* TUN / Network related functions */
> -struct ppp_net *ppp_net_new(GAtPPP *ppp);
> +struct ppp_net *ppp_net_new(GAtPPP *ppp, int fd);
>  const char *ppp_net_get_interface(struct ppp_net *net);
>  void ppp_net_process_packet(struct ppp_net *net, const guint8 *packet);
>  void ppp_net_free(struct ppp_net *net);
> diff --git a/gatchat/ppp_net.c b/gatchat/ppp_net.c
> index 1a6cdf7..59c4b5e 100644
> --- a/gatchat/ppp_net.c
> +++ b/gatchat/ppp_net.c
> @@ -123,12 +123,12 @@ const char *ppp_net_get_interface(struct ppp_net *net)
>  	return net->if_name;
>  }
>  
> -struct ppp_net *ppp_net_new(GAtPPP *ppp)
> +struct ppp_net *ppp_net_new(GAtPPP *ppp, int fd)
>  {
>  	struct ppp_net *net;
>  	GIOChannel *channel = NULL;
>  	struct ifreq ifr;
> -	int fd, err;
> +	int fdesc, err;
>  
>  	net = g_try_new0(struct ppp_net, 1);
>  	if (net == NULL)
> @@ -140,23 +140,33 @@ struct ppp_net *ppp_net_new(GAtPPP *ppp)
>  		return NULL;
>  	}
>  
> -	/* open a tun interface */
> -	fd = open("/dev/net/tun", O_RDWR);
> -	if (fd < 0)
> -		goto error;
> -
> -	memset(&ifr, 0, sizeof(ifr));
> -	ifr.ifr_flags = IFF_TUN | IFF_NO_PI;
> -	strcpy(ifr.ifr_name, "ppp%d");
> -
> -	err = ioctl(fd, TUNSETIFF, (void *) &ifr);
> -	if (err < 0)
> -		goto error;
> -
> -	net->if_name = strdup(ifr.ifr_name);
> +	/*
> +	 * If the fd value is still the default one,
> +	 * open the tun interface and configure it.
> +	 */
> +	if (fd< 0) {

Missing space before '<'

> +		/* open a tun interface */
> +		fdesc = open("/dev/net/tun", O_RDWR);
> +		if (fdesc < 0)
> +			goto error;
> +
> +		memset(&ifr, 0, sizeof(ifr));
> +		ifr.ifr_flags = IFF_TUN | IFF_NO_PI;
> +		strcpy(ifr.ifr_name, "ppp%d");
> +
> +		err = ioctl(fdesc, TUNSETIFF, (void *) &ifr);
> +		if (err < 0)
> +			goto error;
> +
> +		net->if_name = strdup(ifr.ifr_name);
> +		/* create a channel for reading and writing to this interface */
> +		channel = g_io_channel_unix_new(fdesc);
> +	} else {
> +		net->if_name = strdup("Server ppp");

This looks wrong.  You do actually want to return a proper network
interface here, not make something up.

> +		/* create a channel for reading and writing to this interface */
> +		channel = g_io_channel_unix_new(fd);
> +	}

There's a small problem of symmetry here.  If IPCP is established
correctly, then the fd will eventually be closed by ppp_net.  However,
if we never properly establish IPCP, then fd will not be closed.  What
is actually expected by ConnMan?

>  
> -	/* create a channel for reading and writing to this interface */
> -	channel = g_io_channel_unix_new(fd);
>  	if (channel == NULL)
>  		goto error;
>  

Regards,
-Denis

Re: [PATCH_v2 2/4] emulator: add routine to request/release private network from ConnMan

[Denis Kenzior]

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

Hi Guillaume,

On 04/22/2011 07:06 AM, Guillaume Zajac wrote:
> ---
>  include/emulator.h |   10 +++++++
>  src/emulator.c     |   75 +++++++++++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 78 insertions(+), 7 deletions(-)
> 
> diff --git a/include/emulator.h b/include/emulator.h
> index 5cd894b..29abf55 100644
> --- a/include/emulator.h
> +++ b/include/emulator.h
> @@ -105,6 +105,16 @@ enum ofono_emulator_request_type ofono_emulator_request_get_type(
>  void ofono_emulator_set_indicator(struct ofono_emulator *em,
>  					const char *name, int value);
>  
> +typedef void (*network_request)(struct ofono_error *, int *,
> +				const char **, const char **,
> +				const char **, const char **);
> +typedef void (*network_release)(void);
> +
> +void ofono_emulator_add_network_request_cb(struct ofono_emulator *em,
> +							network_request cb);
> +void ofono_emulator_add_network_release_cb(struct ofono_emulator *em,
> +							network_release cb);
> +

I really don't like this, you're re-inventing the wheel here.  We
already solved this pattern a while ago ;)  I suggest you look at
include/gprs-provision.h and examples/provision.c

>  #ifdef __cplusplus
>  }
>  #endif

Regards,
-Denis

Re: [PATCH_v2 4/4] Makefile: add connman plugin build

[Denis Kenzior]

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

Hi Guillaume,

On 04/22/2011 07:06 AM, Guillaume Zajac wrote:
> ---
>  Makefile.am |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 

Please group this patch with patch 3.  Modifications to the build system
should go together with the patch that introduces a new file.

Regards,
-Denis

Re: [PATCH_v2 1/4] gatppp: Add new contructor to use external fd

[Guillaume Zajac]

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

Hi Denis,

On 27/04/2011 05:20, Denis Kenzior wrote:
> Hi Guillaume,
>
> On 04/22/2011 07:06 AM, Guillaume Zajac wrote:
>> ---
>>   gatchat/gatppp.c  |   43 ++++++++++++++++++++++++++++++++++++++++---
>>   gatchat/gatppp.h  |    2 ++
>>   gatchat/ppp.h     |    2 +-
>>   gatchat/ppp_net.c |   46 ++++++++++++++++++++++++++++------------------
>>   4 files changed, 71 insertions(+), 22 deletions(-)
>>
>> diff --git a/gatchat/gatppp.c b/gatchat/gatppp.c
>> index 993b5ea..d59f69b 100644
>> --- a/gatchat/gatppp.c
>> +++ b/gatchat/gatppp.c
>> @@ -76,6 +76,7 @@ struct _GAtPPP {
>>   	gpointer debug_data;
>>   	gboolean sta_pending;
>>   	guint ppp_dead_source;
>> +	int fd;
>>   };
>>
>>   void ppp_debug(GAtPPP *ppp, const char *str)
>> @@ -288,7 +289,7 @@ void ppp_auth_notify(GAtPPP *ppp, gboolean success)
>>   void ppp_ipcp_up_notify(GAtPPP *ppp, const char *local, const char *peer,
>>   					const char *dns1, const char *dns2)
>>   {
>> -	ppp->net = ppp_net_new(ppp);
>> +	ppp->net = ppp_net_new(ppp, ppp->fd);
>>
>>   	if (ppp->net == NULL) {
>>   		ppp->disconnect_reason = G_AT_PPP_REASON_NET_FAIL;
>> @@ -296,8 +297,14 @@ void ppp_ipcp_up_notify(GAtPPP *ppp, const char *local, const char *peer,
>>   		return;
>>   	}
>>
>> -	if (ppp_net_set_mtu(ppp->net, ppp->mtu) == FALSE)
>> -		DBG(ppp, "Unable to set MTU");
>> +	/*
>> +	 * If we have opened the tun interface locally,
>> +	 * we have to set a MTU value.
>> +	 */
> So I don't agree with this.  The MTU is negotiated during LCP phase, so
> unless I'm missing something, this value should still be set to the
> negotiated one.
>

Right , I thought that as I was setting a MTU value for ConnMan tun 
interface I didn't need to set the MTU value into oFono.
Obviously I was wrong :)

>> +	if (ppp->fd<  0) {
>> +		if (ppp_net_set_mtu(ppp->net, ppp->mtu) == FALSE)
>> +			DBG(ppp, "Unable to set MTU");
>> +	}
>>
>>   	ppp_enter_phase(ppp, PPP_PHASE_LINK_UP);
>>
>> @@ -541,6 +548,9 @@ static GAtPPP *ppp_init_common(GAtHDLC *hdlc, gboolean is_server, guint32 ip)
>>
>>   	ppp->ref_count = 1;
>>
>> +	/* Default value of fd is -1 */
>> +	ppp->fd = -1;
>> +
>>   	/* set options to defaults */
>>   	ppp->mru = DEFAULT_MRU;
>>   	ppp->mtu = DEFAULT_MTU;
>> @@ -629,6 +639,33 @@ GAtPPP *g_at_ppp_server_new_from_io(GAtIO *io, const char *local)
>>   		return NULL;
>>
>>   	ppp = ppp_init_common(hdlc, TRUE, ip);
>> +
>> +	g_at_hdlc_unref(hdlc);
>> +
>> +	return ppp;
>> +}
>> +
>> +GAtPPP *g_at_ppp_server_new_from_io_with_fd(GAtIO *io,
>> +						const char *local, int fd)
>> +{
>> +	GAtHDLC *hdlc;
>> +	GAtPPP *ppp;
>> +	guint32 ip;
>> +
>> +	if (local == NULL)
>> +		ip = 0;
>> +	else if (inet_pton(AF_INET, local,&ip) != 1)
>> +		return NULL;
>> +
>> +	hdlc = g_at_hdlc_new_from_io(io);
>> +	if (hdlc == NULL)
>> +		return NULL;
>> +
>> +	ppp = ppp_init_common(hdlc, TRUE, ip);
>> +
>> +	/* Set the fd value returned by ConnMan */
>> +	ppp->fd = fd;
>> +
>>   	g_at_hdlc_unref(hdlc);
>>
>>   	return ppp;
>> diff --git a/gatchat/gatppp.h b/gatchat/gatppp.h
>> index fb5de4c..4ed4cde 100644
>> --- a/gatchat/gatppp.h
>> +++ b/gatchat/gatppp.h
>> @@ -54,6 +54,8 @@ GAtPPP *g_at_ppp_new(GIOChannel *modem);
>>   GAtPPP *g_at_ppp_new_from_io(GAtIO *io);
>>   GAtPPP *g_at_ppp_server_new(GIOChannel *modem, const char *local);
>>   GAtPPP *g_at_ppp_server_new_from_io(GAtIO *io, const char *local);
>> +GAtPPP *g_at_ppp_server_new_from_io_with_fd(GAtIO *io,
>> +						const char *local, int fd);
> Please name this g_at_ppp_server_new_full()

Ok

>>
>>   void g_at_ppp_open(GAtPPP *ppp);
>>   void g_at_ppp_set_connect_function(GAtPPP *ppp, GAtPPPConnectFunc callback,
>> diff --git a/gatchat/ppp.h b/gatchat/ppp.h
>> index d2786d7..8107820 100644
>> --- a/gatchat/ppp.h
>> +++ b/gatchat/ppp.h
>> @@ -102,7 +102,7 @@ void ppp_chap_free(struct ppp_chap *chap);
>>   void ppp_chap_process_packet(struct ppp_chap *chap, const guint8 *new_packet);
>>
>>   /* TUN / Network related functions */
>> -struct ppp_net *ppp_net_new(GAtPPP *ppp);
>> +struct ppp_net *ppp_net_new(GAtPPP *ppp, int fd);
>>   const char *ppp_net_get_interface(struct ppp_net *net);
>>   void ppp_net_process_packet(struct ppp_net *net, const guint8 *packet);
>>   void ppp_net_free(struct ppp_net *net);
>> diff --git a/gatchat/ppp_net.c b/gatchat/ppp_net.c
>> index 1a6cdf7..59c4b5e 100644
>> --- a/gatchat/ppp_net.c
>> +++ b/gatchat/ppp_net.c
>> @@ -123,12 +123,12 @@ const char *ppp_net_get_interface(struct ppp_net *net)
>>   	return net->if_name;
>>   }
>>
>> -struct ppp_net *ppp_net_new(GAtPPP *ppp)
>> +struct ppp_net *ppp_net_new(GAtPPP *ppp, int fd)
>>   {
>>   	struct ppp_net *net;
>>   	GIOChannel *channel = NULL;
>>   	struct ifreq ifr;
>> -	int fd, err;
>> +	int fdesc, err;
>>
>>   	net = g_try_new0(struct ppp_net, 1);
>>   	if (net == NULL)
>> @@ -140,23 +140,33 @@ struct ppp_net *ppp_net_new(GAtPPP *ppp)
>>   		return NULL;
>>   	}
>>
>> -	/* open a tun interface */
>> -	fd = open("/dev/net/tun", O_RDWR);
>> -	if (fd<  0)
>> -		goto error;
>> -
>> -	memset(&ifr, 0, sizeof(ifr));
>> -	ifr.ifr_flags = IFF_TUN | IFF_NO_PI;
>> -	strcpy(ifr.ifr_name, "ppp%d");
>> -
>> -	err = ioctl(fd, TUNSETIFF, (void *)&ifr);
>> -	if (err<  0)
>> -		goto error;
>> -
>> -	net->if_name = strdup(ifr.ifr_name);
>> +	/*
>> +	 * If the fd value is still the default one,
>> +	 * open the tun interface and configure it.
>> +	 */
>> +	if (fd<  0) {
> Missing space before '<'
>

Ok

>> +		/* open a tun interface */
>> +		fdesc = open("/dev/net/tun", O_RDWR);
>> +		if (fdesc<  0)
>> +			goto error;
>> +
>> +		memset(&ifr, 0, sizeof(ifr));
>> +		ifr.ifr_flags = IFF_TUN | IFF_NO_PI;
>> +		strcpy(ifr.ifr_name, "ppp%d");
>> +
>> +		err = ioctl(fdesc, TUNSETIFF, (void *)&ifr);
>> +		if (err<  0)
>> +			goto error;
>> +
>> +		net->if_name = strdup(ifr.ifr_name);
>> +		/* create a channel for reading and writing to this interface */
>> +		channel = g_io_channel_unix_new(fdesc);
>> +	} else {
>> +		net->if_name = strdup("Server ppp");
> This looks wrong.  You do actually want to return a proper network
> interface here, not make something up.

In fact, only ConnMan needs to know each name of its interfaces,
so do we need to fill this field in when ConnMan creates the interface?

>> +		/* create a channel for reading and writing to this interface */
>> +		channel = g_io_channel_unix_new(fd);
>> +	}
> There's a small problem of symmetry here.  If IPCP is established
> correctly, then the fd will eventually be closed by ppp_net.  However,
> if we never properly establish IPCP, then fd will not be closed.  What
> is actually expected by ConnMan?
>

In the case we don't properly establish the IPCP, ppp_disconnect CB of 
emulator will be called.
Then, we will call the release_private_network DBus method. This method 
will close the fd if it is opened.

>>
>> -	/* create a channel for reading and writing to this interface */
>> -	channel = g_io_channel_unix_new(fd);
>>   	if (channel == NULL)
>>   		goto error;
>>
> Regards,
> -Denis
>

Kind regards,
Guillaume

Re: [PATCH_v2 2/4] emulator: add routine to request/release private network from ConnMan

[Guillaume Zajac]

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

Hi Denis,

On 27/04/2011 05:25, Denis Kenzior wrote:
> Hi Guillaume,
>
> On 04/22/2011 07:06 AM, Guillaume Zajac wrote:
>> ---
>>   include/emulator.h |   10 +++++++
>>   src/emulator.c     |   75 +++++++++++++++++++++++++++++++++++++++++++++++-----
>>   2 files changed, 78 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/emulator.h b/include/emulator.h
>> index 5cd894b..29abf55 100644
>> --- a/include/emulator.h
>> +++ b/include/emulator.h
>> @@ -105,6 +105,16 @@ enum ofono_emulator_request_type ofono_emulator_request_get_type(
>>   void ofono_emulator_set_indicator(struct ofono_emulator *em,
>>   					const char *name, int value);
>>
>> +typedef void (*network_request)(struct ofono_error *, int *,
>> +				const char **, const char **,
>> +				const char **, const char **);
>> +typedef void (*network_release)(void);
>> +
>> +void ofono_emulator_add_network_request_cb(struct ofono_emulator *em,
>> +							network_request cb);
>> +void ofono_emulator_add_network_release_cb(struct ofono_emulator *em,
>> +							network_release cb);
>> +
> I really don't like this, you're re-inventing the wheel here.  We
> already solved this pattern a while ago ;)  I suggest you look at
> include/gprs-provision.h and examples/provision.c

In looking the example, I should create some emulator network drivers to 
get settings and release the network like:

struct emulator_network{
     ...
}

struct emulator_network_provision_driver{
     char *name;
     (void)(*get_settings)(struct emulator_network **out_en);
     (void)(*release)(void);
}

and un/register drivers into emulator.c

>>   #ifdef __cplusplus
>>   }
>>   #endif
> Regards,
> -Denis
>

Kind regards,
Guillaume

Re: [PATCH_v2 1/4] gatppp: Add new contructor to use external fd

[Denis Kenzior]

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

Hi Guillaume,

>>> -    if (ppp_net_set_mtu(ppp->net, ppp->mtu) == FALSE)
>>> -        DBG(ppp, "Unable to set MTU");
>>> +    /*
>>> +     * If we have opened the tun interface locally,
>>> +     * we have to set a MTU value.
>>> +     */
>> So I don't agree with this.  The MTU is negotiated during LCP phase, so
>> unless I'm missing something, this value should still be set to the
>> negotiated one.
>>
> 
> Right , I thought that as I was setting a MTU value for ConnMan tun
> interface I didn't need to set the MTU value into oFono.
> Obviously I was wrong :)
> 

I assume the MTU is set to the default 1500 in Connman?

>>> +        net->if_name = strdup("Server ppp");
>> This looks wrong.  You do actually want to return a proper network
>> interface here, not make something up.
> 
> In fact, only ConnMan needs to know each name of its interfaces,
> so do we need to fill this field in when ConnMan creates the interface?
> 

Remember that we're writing a library, so things must be consistent.
Besides, isn't this is a matter of running a TUNGETIFF ioctl?

>>> +        /* create a channel for reading and writing to this
>>> interface */
>>> +        channel = g_io_channel_unix_new(fd);
>>> +    }
>> There's a small problem of symmetry here.  If IPCP is established
>> correctly, then the fd will eventually be closed by ppp_net.  However,
>> if we never properly establish IPCP, then fd will not be closed.  What
>> is actually expected by ConnMan?
>>
> 
> In the case we don't properly establish the IPCP, ppp_disconnect CB of
> emulator will be called.
> Then, we will call the release_private_network DBus method. This method
> will close the fd if it is opened.
> 

Again, we're writing a library.  We cannot assume some behavior external
to the library is going to save us.  So I really suggest we make closing
of the fd symmetric.

Regards,
-Denis

Re: [PATCH_v2 1/4] gatppp: Add new contructor to use external fd

[Guillaume Zajac]

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

Hi Denis,

On 28/04/2011 16:44, Denis Kenzior wrote:
> Hi Guillaume,
>
>>>> -    if (ppp_net_set_mtu(ppp->net, ppp->mtu) == FALSE)
>>>> -        DBG(ppp, "Unable to set MTU");
>>>> +    /*
>>>> +     * If we have opened the tun interface locally,
>>>> +     * we have to set a MTU value.
>>>> +     */
>>> So I don't agree with this.  The MTU is negotiated during LCP phase, so
>>> unless I'm missing something, this value should still be set to the
>>> negotiated one.
>>>
>> Right , I thought that as I was setting a MTU value for ConnMan tun
>> interface I didn't need to set the MTU value into oFono.
>> Obviously I was wrong :)
>>
> I assume the MTU is set to the default 1500 in Connman?

Exactly

>>>> +        net->if_name = strdup("Server ppp");
>>> This looks wrong.  You do actually want to return a proper network
>>> interface here, not make something up.
>> In fact, only ConnMan needs to know each name of its interfaces,
>> so do we need to fill this field in when ConnMan creates the interface?
>>
> Remember that we're writing a library, so things must be consistent.
> Besides, isn't this is a matter of running a TUNGETIFF ioctl?

Yes with TUNGETIFF I can get the name of the interface created by ConnMan

>>>> +        /* create a channel for reading and writing to this
>>>> interface */
>>>> +        channel = g_io_channel_unix_new(fd);
>>>> +    }
>>> There's a small problem of symmetry here.  If IPCP is established
>>> correctly, then the fd will eventually be closed by ppp_net.  However,
>>> if we never properly establish IPCP, then fd will not be closed.  What
>>> is actually expected by ConnMan?
>>>
>> In the case we don't properly establish the IPCP, ppp_disconnect CB of
>> emulator will be called.
>> Then, we will call the release_private_network DBus method. This method
>> will close the fd if it is opened.
>>
> Again, we're writing a library.  We cannot assume some behavior external
> to the library is going to save us.  So I really suggest we make closing
> of the fd symmetric.
So I will close the fd into g_at_ppp_unref() if it is >=0.
I have also to close it if we fail to create the PPP server.


Kind regards,
Guillaume

Re: [PATCH_v2 2/4] emulator: add routine to request/release private network from ConnMan

[Denis Kenzior]

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

Hi Guillaume,

On 04/28/2011 08:29 AM, Guillaume Zajac wrote:
> Hi Denis,
> 
> On 27/04/2011 05:25, Denis Kenzior wrote:
>> Hi Guillaume,
>>
>> On 04/22/2011 07:06 AM, Guillaume Zajac wrote:
>>> ---
>>>   include/emulator.h |   10 +++++++
>>>   src/emulator.c     |   75
>>> +++++++++++++++++++++++++++++++++++++++++++++++-----
>>>   2 files changed, 78 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/include/emulator.h b/include/emulator.h
>>> index 5cd894b..29abf55 100644
>>> --- a/include/emulator.h
>>> +++ b/include/emulator.h
>>> @@ -105,6 +105,16 @@ enum ofono_emulator_request_type
>>> ofono_emulator_request_get_type(
>>>   void ofono_emulator_set_indicator(struct ofono_emulator *em,
>>>                       const char *name, int value);
>>>
>>> +typedef void (*network_request)(struct ofono_error *, int *,
>>> +                const char **, const char **,
>>> +                const char **, const char **);
>>> +typedef void (*network_release)(void);
>>> +
>>> +void ofono_emulator_add_network_request_cb(struct ofono_emulator *em,
>>> +                            network_request cb);
>>> +void ofono_emulator_add_network_release_cb(struct ofono_emulator *em,
>>> +                            network_release cb);
>>> +
>> I really don't like this, you're re-inventing the wheel here.  We
>> already solved this pattern a while ago ;)  I suggest you look at
>> include/gprs-provision.h and examples/provision.c
> 
> In looking the example, I should create some emulator network drivers to
> get settings and release the network like:
> 
> struct emulator_network{
>     ...
> }

struct private_network_settings or something might be better

> 
> struct emulator_network_provision_driver{
>     char *name;
>     (void)(*get_settings)(struct emulator_network **out_en);

You will probably need to make this async, and allow cancellation, since
there can be multiple emulators requesting private networks
concurrently.  This probably means you need to return a unique
identifier as well.

>     (void)(*release)(void);

And you need to take the unique id as input here.

> }
> 
> and un/register drivers into emulator.c
> 
>>>   #ifdef __cplusplus
>>>   }
>>>   #endif
>> Regards,
>> -Denis
>>
> 
> Kind regards,
> Guillaume

Regards,
-Denis

Re: [PATCH_v2 1/4] gatppp: Add new contructor to use external fd

[Denis Kenzior]

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

Hi Guillaume,

>>>>> +        /* create a channel for reading and writing to this
>>>>> interface */
>>>>> +        channel = g_io_channel_unix_new(fd);
>>>>> +    }
>>>> There's a small problem of symmetry here.  If IPCP is established
>>>> correctly, then the fd will eventually be closed by ppp_net.  However,
>>>> if we never properly establish IPCP, then fd will not be closed.  What
>>>> is actually expected by ConnMan?
>>>>
>>> In the case we don't properly establish the IPCP, ppp_disconnect CB of
>>> emulator will be called.
>>> Then, we will call the release_private_network DBus method. This method
>>> will close the fd if it is opened.
>>>
>> Again, we're writing a library.  We cannot assume some behavior external
>> to the library is going to save us.  So I really suggest we make closing
>> of the fd symmetric.
> So I will close the fd into g_at_ppp_unref() if it is >=0.
> I have also to close it if we fail to create the PPP server.
> 

Right.  Your approach doesn't sound completely correct, but I will let
you figure this out.  Just make sure to close the fd in all circumstances.

Regards,
-Denis

Re: [PATCH_v2 1/4] gatppp: Add new contructor to use external fd

[Denis Kenzior]

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

Hi Guillaume,

On 04/29/2011 08:12 AM, Guillaume Zajac wrote:
> Hi Denis,
> 
> On 28/04/2011 21:41, Denis Kenzior wrote:
>> Hi Guillaume,
>>
>>>>>>> +        /* create a channel for reading and writing to this
>>>>>>> interface */
>>>>>>> +        channel = g_io_channel_unix_new(fd);
>>>>>>> +    }
>>>>>> There's a small problem of symmetry here.  If IPCP is established
>>>>>> correctly, then the fd will eventually be closed by ppp_net. 
>>>>>> However,
>>>>>> if we never properly establish IPCP, then fd will not be closed. 
>>>>>> What
>>>>>> is actually expected by ConnMan?
>>>>>>
>>>>> In the case we don't properly establish the IPCP, ppp_disconnect CB of
>>>>> emulator will be called.
>>>>> Then, we will call the release_private_network DBus method. This
>>>>> method
>>>>> will close the fd if it is opened.
>>>>>
>>>> Again, we're writing a library.  We cannot assume some behavior
>>>> external
>>>> to the library is going to save us.  So I really suggest we make
>>>> closing
>>>> of the fd symmetric.
>>> So I will close the fd into g_at_ppp_unref() if it is>=0.
>>> I have also to close it if we fail to create the PPP server.
>>>
>> Right.  Your approach doesn't sound completely correct, but I will let
>> you figure this out.  Just make sure to close the fd in all
>> circumstances.
> 
> In looking the previous implementation in ppp_net_new(),
> there is just:
> 
> fd = open("/dev/net/tun", ...);
> ...
> net->channel = g_io_channel_unix_new(fd);
> 
> However, the fd is never closed when we do ppp_net_free()
> We just unref the net->channel
> 
> So should we do in ppp_net_free() a:
> fd = g_io_channel_unix_get_fd(net->channel);
> close(fd);
> g_io_channel_unref(net->channel);
> 
> Or maybe I miss something?
> 

You are, namely the call to:

        if (!g_at_util_setup_io(channel, 0))
                goto error;

which sets the close on unref flag on the GIOChannel.

Regards,
-Denis

Re: [PATCH_v2 2/4] emulator: add routine to request/release private network from ConnMan

[Denis Kenzior]

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

Hi Guillaume,

>>> struct emulator_network_provision_driver{
>>>      char *name;
>>>      (void)(*get_settings)(struct emulator_network **out_en);
>> You will probably need to make this async, and allow cancellation, since
>> there can be multiple emulators requesting private networks
>> concurrently.  This probably means you need to return a unique
>> identifier as well.
>>
> 
> Concerning asynchronism and cancellation,
> I don't see the use case here, do you mean:
>     - on a device with multiple modems, we have one DUN emulator per modem
>     - 2 clients request DUN data call on this device, data call will be
> redirected to each modem by ConnMan.
>     - oFono will ask for 2 private network settings, however ConnMan

You can have several modems in the system with DUN connections.  You
might have several DUN connection transports active, e.g. USB & BT, or
you might simply have several clients on the same transport.

> can't handle this case, so we should
>       avoid this case from oFono by cancelling the 1 of the request.
> 

Is ConnMan only handing out 1 IP per application?  If so that is not
enough for oFono...

> how should we priotirize it?
> Could that be a concrete use case?
> 
>>>      (void)(*release)(void);
>> And you need to take the unique id as input here.
> 
> I can store the private_network_settings into ofono_emulator. Then I
> pass it as input of the release function.
> 

Won't work as you might need to cancel a request that is still pending.

Regards,
-Denis

Re: [PATCH_v2 2/4] emulator: add routine to request/release private network from ConnMan

[Guillaume Zajac]

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

Hi Denis,

On 28/04/2011 21:39, Denis Kenzior wrote:
> Hi Guillaume,
>
> On 04/28/2011 08:29 AM, Guillaume Zajac wrote:
>> Hi Denis,
>>
>> On 27/04/2011 05:25, Denis Kenzior wrote:
>>> Hi Guillaume,
>>>
>>> On 04/22/2011 07:06 AM, Guillaume Zajac wrote:
>>>> +typedef void (*network_request)(struct ofono_error *, int *,
>>>> +                const char **, const char **,
>>>> +                const char **, const char **);
>>>> +typedef void (*network_release)(void);
>>>> +
>>>> +void ofono_emulator_add_network_request_cb(struct ofono_emulator *em,
>>>> +                            network_request cb);
>>>> +void ofono_emulator_add_network_release_cb(struct ofono_emulator *em,
>>>> +                            network_release cb);
>>>> +
>>> I really don't like this, you're re-inventing the wheel here.  We
>>> already solved this pattern a while ago ;)  I suggest you look at
>>> include/gprs-provision.h and examples/provision.c
>> In looking the example, I should create some emulator network drivers to
>> get settings and release the network like:
>>
>> struct emulator_network{
>>      ...
>> }
> struct private_network_settings or something might be better
>

Ok

>> struct emulator_network_provision_driver{
>>      char *name;
>>      (void)(*get_settings)(struct emulator_network **out_en);
> You will probably need to make this async, and allow cancellation, since
> there can be multiple emulators requesting private networks
> concurrently.  This probably means you need to return a unique
> identifier as well.
>

Concerning asynchronism and cancellation,
I don't see the use case here, do you mean:
     - on a device with multiple modems, we have one DUN emulator per modem
     - 2 clients request DUN data call on this device, data call will be 
redirected to each modem by ConnMan.
     - oFono will ask for 2 private network settings, however ConnMan 
can't handle this case, so we should
       avoid this case from oFono by cancelling the 1 of the request.

how should we priotirize it?
Could that be a concrete use case?

>>      (void)(*release)(void);
> And you need to take the unique id as input here.

I can store the private_network_settings into ofono_emulator. Then I 
pass it as input of the release function.

>> }
>>
>> and un/register drivers into emulator.c
>>
>>> Regards,
>>> -Denis
>>>
>> Kind regards,
>> Guillaume
> Regards,
> -Denis
>

Kind regards,
Guillaume

Re: [PATCH_v2 1/4] gatppp: Add new contructor to use external fd

[Guillaume Zajac]

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

Hi Denis,

On 28/04/2011 21:41, Denis Kenzior wrote:
> Hi Guillaume,
>
>>>>>> +        /* create a channel for reading and writing to this
>>>>>> interface */
>>>>>> +        channel = g_io_channel_unix_new(fd);
>>>>>> +    }
>>>>> There's a small problem of symmetry here.  If IPCP is established
>>>>> correctly, then the fd will eventually be closed by ppp_net.  However,
>>>>> if we never properly establish IPCP, then fd will not be closed.  What
>>>>> is actually expected by ConnMan?
>>>>>
>>>> In the case we don't properly establish the IPCP, ppp_disconnect CB of
>>>> emulator will be called.
>>>> Then, we will call the release_private_network DBus method. This method
>>>> will close the fd if it is opened.
>>>>
>>> Again, we're writing a library.  We cannot assume some behavior external
>>> to the library is going to save us.  So I really suggest we make closing
>>> of the fd symmetric.
>> So I will close the fd into g_at_ppp_unref() if it is>=0.
>> I have also to close it if we fail to create the PPP server.
>>
> Right.  Your approach doesn't sound completely correct, but I will let
> you figure this out.  Just make sure to close the fd in all circumstances.

In looking the previous implementation in ppp_net_new(),
there is just:

fd = open("/dev/net/tun", ...);
...
net->channel = g_io_channel_unix_new(fd);

However, the fd is never closed when we do ppp_net_free()
We just unref the net->channel

So should we do in ppp_net_free() a:
fd = g_io_channel_unix_get_fd(net->channel);
close(fd);
g_io_channel_unref(net->channel);

Or maybe I miss something?

In the new implementation, in the case we don't establish the IPCP, we 
still have the fd of ConnMan opened.
ppp->net == NULL so in the g_at_ppp_unref() we can do:

if (ppp->net)
         ppp_net_free(ppp->net); /* We close the fd  like above */
else {
         if( fd >= 0)
                 close(ppp->fd); /* We don't have established IPCP, we 
need to close the fd from ConnMan */
         }

Kind regards,
Guillaume

Re: [PATCH_v2 1/4] gatppp: Add new contructor to use external fd

[Guillaume Zajac]

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

Hi again Denis,

On 29/04/2011 15:12, Guillaume Zajac wrote:
> Hi Denis,
>
> On 28/04/2011 21:41, Denis Kenzior wrote:
>> Hi Guillaume,
>>
>>>>>>> +        /* create a channel for reading and writing to this
>>>>>>> interface */
>>>>>>> +        channel = g_io_channel_unix_new(fd);
>>>>>>> +    }
>>>>>> There's a small problem of symmetry here.  If IPCP is established
>>>>>> correctly, then the fd will eventually be closed by ppp_net.  
>>>>>> However,
>>>>>> if we never properly establish IPCP, then fd will not be closed.  
>>>>>> What
>>>>>> is actually expected by ConnMan?
>>>>>>
>>>>> In the case we don't properly establish the IPCP, ppp_disconnect 
>>>>> CB of
>>>>> emulator will be called.
>>>>> Then, we will call the release_private_network DBus method. This 
>>>>> method
>>>>> will close the fd if it is opened.
>>>>>
>>>> Again, we're writing a library.  We cannot assume some behavior 
>>>> external
>>>> to the library is going to save us.  So I really suggest we make 
>>>> closing
>>>> of the fd symmetric.
>>> So I will close the fd into g_at_ppp_unref() if it is>=0.
>>> I have also to close it if we fail to create the PPP server.
>>>
>> Right.  Your approach doesn't sound completely correct, but I will let
>> you figure this out.  Just make sure to close the fd in all 
>> circumstances.
>
> In looking the previous implementation in ppp_net_new(),
> there is just:
>
> fd = open("/dev/net/tun", ...);
> ...
> net->channel = g_io_channel_unix_new(fd);
>
> However, the fd is never closed when we do ppp_net_free()
> We just unref the net->channel
>
> So should we do in ppp_net_free() a:
> fd = g_io_channel_unix_get_fd(net->channel);
> close(fd);
> g_io_channel_unref(net->channel);
>
> Or maybe I miss something?

In fact I missed something :) , the g_io_channel_unref() is closing fd

>
> In the new implementation, in the case we don't establish the IPCP, we 
> still have the fd of ConnMan opened.
> ppp->net == NULL so in the g_at_ppp_unref() we can do:
>
> if (ppp->net)
>         ppp_net_free(ppp->net); /* We close the fd  like above */

We close it during unref.

> else {
>         if( fd >= 0)
>                 close(ppp->fd); /* We don't have established IPCP, we 
> need to close the fd from ConnMan */
>         }
>

We might need also to do:

ppp->fd = -1;

into ppp_ipcp_down_notify() just after ppp_net_free()


Kind regards,
Guillaume

Re: [PATCH_v2 2/4] emulator: add routine to request/release private network from ConnMan

[Guillaume Zajac]

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

Hi Denis,

On 29/04/2011 10:59, Denis Kenzior wrote:
> Hi Guillaume,
>
>>>> struct emulator_network_provision_driver{
>>>>       char *name;
>>>>       (void)(*get_settings)(struct emulator_network **out_en);
>>> You will probably need to make this async, and allow cancellation, since
>>> there can be multiple emulators requesting private networks
>>> concurrently.  This probably means you need to return a unique
>>> identifier as well.
>>>
>> Concerning asynchronism and cancellation,
>> I don't see the use case here, do you mean:
>>      - on a device with multiple modems, we have one DUN emulator per modem
>>      - 2 clients request DUN data call on this device, data call will be
>> redirected to each modem by ConnMan.
>>      - oFono will ask for 2 private network settings, however ConnMan
> You can have several modems in the system with DUN connections.  You
> might have several DUN connection transports active, e.g. USB&  BT, or
> you might simply have several clients on the same transport.
>

Ok

>> can't handle this case, so we should
>>        avoid this case from oFono by cancelling the 1 of the request.
>>
> Is ConnMan only handing out 1 IP per application?  If so that is not
> enough for oFono...
>

This is the case in current private network implementation.
I guess it can be changed.

>> how should we priotirize it?
>> Could that be a concrete use case?
>>
>>>>       (void)(*release)(void);
>>> And you need to take the unique id as input here.
>> I can store the private_network_settings into ofono_emulator. Then I
>> pass it as input of the release function.
>>
> Won't work as you might need to cancel a request that is still pending.

OK I will try to fix this.


Kind regards,
Guillaume