Hi Denis,
On 06/06/2011 08:17, Denis Kenzior wrote:
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.
Thanks for the info, I didn't know about that.
+#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.
I will remove this pending reset into request_reply: I have to let dbus
rountines release the pending call.
I test pending value and dbus_pending_call_get_completed() because it is
possible that the pending call has been completed and is not NULL.
+
+ 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]
It seems there is a typo between the doc and the implemented API, for
the moment ReleasePrivateNetwork() doesn't take any argument.
The PN settings are removed from the hash table using the DBus sender.
I will send a patch to fix the documentation.
+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)
Kind regards,
Guillaume
_______________________________________________
ofono mailing list
[email protected]
http://lists.ofono.org/listinfo/ofono
