Send connman mailing list submissions to
[email protected]
To subscribe or unsubscribe via the World Wide Web, visit
https://lists.01.org/mailman/listinfo/connman
or, via email, send a message with subject or body 'help' to
[email protected]
You can reach the person managing the list at
[email protected]
When replying, please edit your Subject line so it is more specific
than "Re: Contents of connman digest..."
Today's Topics:
1. Re: [PATCH v2 4/7] iwd: Add Agent support (Patrik Flykt)
2. Re: [PATCH v2 4/7] iwd: Add Agent support (Daniel Wagner)
3. Re: [PATCH v3] service: Block auto connect when connection is
in progress (Patrik Flykt)
4. Re: [PATCH v3] service: Block auto connect when connection is
in progress (Daniel Wagner)
5. Re: [PATCH v4 2/4] service: implement
AlwaysConnectedTechnologies option (Daniel Wagner)
6. Re: Spurious failures starting ConnMan with systemd (Patrik Flykt)
----------------------------------------------------------------------
Message: 1
Date: Mon, 14 Nov 2016 10:56:20 +0200
From: Patrik Flykt <[email protected]>
To: Daniel Wagner <[email protected]>, [email protected]
Cc: Daniel Wagner <[email protected]>
Subject: Re: [PATCH v2 4/7] iwd: Add Agent support
Message-ID: <[email protected]>
Content-Type: text/plain; charset="UTF-8"
On Sat, 2016-11-12 at 22:12 +0100, Daniel Wagner wrote:
> > From: Daniel Wagner <[email protected]>
>
> ---
> ?plugins/iwd.c | 128
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ?1 file changed, 128 insertions(+)
>
> diff --git a/plugins/iwd.c b/plugins/iwd.c
> index d372e9f1ac5c..eb13e6d4b966 100644
> --- a/plugins/iwd.c
> +++ b/plugins/iwd.c
> @@ -34,9 +34,11 @@
> ?
> ?static DBusConnection *connection;
> ?static GDBusClient *client;
> +static GDBusProxy *agent_proxy;
> ?static GHashTable *adapters;
> ?static GHashTable *devices;
> ?static GHashTable *networks;
> +static bool agent_registered;
> ?
> > ?#define IWD_SERVICE "net.connman.iwd"
> > ?#define IWD_PATH "/"
> @@ -45,6 +47,10 @@ static GHashTable *networks;
> > ?#define IWD_DEVICE_INTERFACE "net.connman.iwd.Device"
> > ?#define IWD_NETWORK_INTERFACE "net.connman.iwd.Network"
> ?
> > +#define IWD_AGENT_INTERFACE "net.connman.iwd.Agent"
> > +#define IWD_AGENT_ERROR_INTERFACE "net.connman.iwd.Agent.Error"
> > +#define AGENT_PATH "/net/connman/iwd_agent"
> +
> ?enum iwd_device_state {
> > ? IWD_DEVICE_STATE_UNKNOWN,
> > ? IWD_DEVICE_STATE_CONNECTED,
> @@ -344,12 +350,132 @@ static void create_device(GDBusProxy *proxy)
> > ? device_property_change, NULL);
> ?}
> ?
> +static void unregister_agent();
> +
> +static DBusMessage *agent_release_method(DBusConnection *dbus_conn,
> > + DBusMessage *message, void *user_data)
> +{
> > + unregister_agent();
> > + return g_dbus_create_reply(message, DBUS_TYPE_INVALID);
> +}
> +
> +static DBusMessage *get_reply_on_error(DBusMessage *message, int error)
> +{
> > + return g_dbus_create_error(message,
> > + IWD_AGENT_ERROR_INTERFACE ".Failed", "Invalid parameters");
> +}
> +
> +static DBusMessage *agent_request_passphrase(DBusConnection *dbus_conn,
> > + DBusMessage *message,
> > + void *user_data)
> +{
> > + struct iwd_network *iwdn;
> > + DBusMessageIter iter;
> > + const char *path, *passwd;
> +
> > + DBG("");
> +
> > + dbus_message_iter_init(message, &iter);
> +
> > + if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_OBJECT_PATH)
> > + return get_reply_on_error(message, EINVAL);
> +
> > + dbus_message_iter_get_basic(&iter, &path);
> +
> > + iwdn = g_hash_table_lookup(networks, path);
> > + if (!iwdn)
> > + return get_reply_on_error(message, EINVAL);
> +
> + passwd = "mysecret";
__connman_agent_request_passphrase_input() and so on.
> +
> > + return g_dbus_create_reply(message, DBUS_TYPE_STRING, &passwd,
> > + DBUS_TYPE_INVALID);
> +}
> +
> +static DBusMessage *agent_cancel(DBusConnection *dbus_conn,
> > + DBusMessage *message, void *user_data)
> +{
> > + DBusMessageIter iter;
> > + const char *reason;
> +
> > + dbus_message_iter_init(message, &iter);
> +
> > + if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
> > + return get_reply_on_error(message, EINVAL);
> +
> > + dbus_message_iter_get_basic(&iter, &reason);
> +
> > + DBG("cancel: %s", reason);
> + /* XXX stop connection attempt, which one? */
There seems to be only one iwd agent running at any time.
> +
> > + return g_dbus_create_reply(message, DBUS_TYPE_INVALID);
> +}
> +
> +static const GDBusMethodTable agent_methods[] = {
> > + { GDBUS_METHOD("Release", NULL, NULL, agent_release_method) },
> > + { GDBUS_METHOD("RequestPassphrase",
> > + GDBUS_ARGS({ "path", "o" }),
> > + GDBUS_ARGS({ "passphrase", "s" }),
> > + agent_request_passphrase)},
> > + { GDBUS_METHOD("Cancel",
> > + GDBUS_ARGS({ "reason", "s" }),
> > + NULL, agent_cancel) },
> > + { },
> +};
> +
> +static void agent_register_builder(DBusMessageIter *iter, void *user_data)
> +{
> > + const char *path = AGENT_PATH;
> +
> > + dbus_message_iter_append_basic(iter, DBUS_TYPE_OBJECT_PATH,
> > + &path);
> +}
> +
> ?static void register_agent(GDBusProxy *proxy)
> ?{
> > + if (!g_dbus_proxy_method_call(proxy,
> > + "RegisterAgent",
> > + agent_register_builder,
> > + NULL, NULL, NULL))
> > + return;
> +
> > + agent_proxy = g_dbus_proxy_ref(proxy);
> ?}
> ?
> ?static void unregister_agent()
> ?{
> > + if (!agent_proxy)
> > + return;
> +
> > + g_dbus_proxy_method_call(agent_proxy,
> > + "UnregisterAgent",
> > + agent_register_builder,
> > + NULL, NULL, NULL);
> +
> > + g_dbus_proxy_unref(agent_proxy);
> > + agent_proxy = NULL;
> +}
> +
> +static void iwd_is_present(DBusConnection *conn, void *user_data)
> +{
> > + if (agent_registered)
> > + return;
> +
> > + if (!g_dbus_register_interface(connection, AGENT_PATH,
> > + IWD_AGENT_INTERFACE, agent_methods,
> > + NULL, NULL, NULL, NULL))
> > + return;
> +
> > + agent_registered = true;
> +}
> +
> +static void iwd_is_out(DBusConnection *conn, void *user_data)
> +{
> > + if (agent_registered) {
> > + g_dbus_unregister_interface(connection,
> > + AGENT_PATH, IWD_AGENT_INTERFACE);
> > + agent_registered = false;
> > + }
> ?}
> ?
> ?static void create_network(GDBusProxy *proxy)
> @@ -459,6 +585,8 @@ static int iwd_init(void)
> > ? goto out;
> > ? }
> ?
> > + g_dbus_client_set_connect_watch(client, iwd_is_present, NULL);
> > + g_dbus_client_set_disconnect_watch(client, iwd_is_out, NULL);
> > ? g_dbus_client_set_proxy_handlers(client, object_added, object_removed,
> > ? NULL, NULL);
> ?
Cheers,
Patrik
------------------------------
Message: 2
Date: Mon, 14 Nov 2016 10:33:29 +0100
From: Daniel Wagner <[email protected]>
To: Patrik Flykt <[email protected]>, [email protected]
Cc: Daniel Wagner <[email protected]>
Subject: Re: [PATCH v2 4/7] iwd: Add Agent support
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Hi Patrik,
>>> + if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_OBJECT_PATH)
>>> + return get_reply_on_error(message, EINVAL);
>> +
>>> + dbus_message_iter_get_basic(&iter, &path);
>> +
>>> + iwdn = g_hash_table_lookup(networks, path);
>>> + if (!iwdn)
>>> + return get_reply_on_error(message, EINVAL);
>> +
>> + passwd = "mysecret";
>
> __connman_agent_request_passphrase_input() and so on.
The networking patch adds this part. I added the passwd placeholder
that I can finish the D-Bus part. And I don't have the 'connman network'
bits yet.
>>> + return g_dbus_create_reply(message, DBUS_TYPE_STRING, &passwd,
>>> + DBUS_TYPE_INVALID);
Well, I could just go for an empty reply in this patch here, then the
placeholder wouldn't be needed yet. I am fine with either solution.
>> +static DBusMessage *agent_cancel(DBusConnection *dbus_conn,
>>> + DBusMessage *message, void *user_data)
>> +{
>>> + DBusMessageIter iter;
>>> + const char *reason;
>> +
>>> + dbus_message_iter_init(message, &iter);
>> +
>>> + if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
>>> + return get_reply_on_error(message, EINVAL);
>> +
>>> + dbus_message_iter_get_basic(&iter, &reason);
>> +
>>> + DBG("cancel: %s", reason);
>> + /* XXX stop connection attempt, which one? */
>
> There seems to be only one iwd agent running at any time.
I was wondering if we have to handle it altogether. ConnMan does ask the
user via its own agent for the passphrase before we tell iwd to connect.
So we don't have any pending work to cancel. It looks like
we don't have to don anything here.
cheers,
daniel
------------------------------
Message: 3
Date: Mon, 14 Nov 2016 12:17:49 +0200
From: Patrik Flykt <[email protected]>
To: Saurav Babu <[email protected]>, [email protected]
Cc: [email protected]
Subject: Re: [PATCH v3] service: Block auto connect when connection is
in progress
Message-ID: <[email protected]>
Content-Type: text/plain; charset="UTF-8"
On Thu, 2016-11-10 at 20:30 +0530, Saurav Babu wrote:
> In the following scenario:
> ?1. Device is connected to AP in non hidden mode.
> ?2. Connection is initated to AP in hidden mode.
> connman disconnects old service and tries to connect to new service.
> But
> for connecting hidden service it first scans the hidden AP and when
> network is added for hidden AP then connection is tried. In the
> meantime
> normal AP got disconnected and was tried to autoconnect even before
> hidden AP was scanned successfully.
> This patch blocks auto connect when any service connection is in
> progress and unblocks autoconnection after connection is started
> or hidden wifi scan is completed and hidden AP is not found in the
> scan list.
> ---
>
> v3: Defer auto connect of services when one service is connecting
> ????Allow auto connect of services when service connection fails with
> error
>
>
> ?src/connman.h |??2 ++
> ?src/network.c |??4 ++++
> ?src/service.c | 22 ++++++++++++++++++++--
> ?3 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/src/connman.h b/src/connman.h
> index ce3ef8d..945518b 100644
> --- a/src/connman.h
> +++ b/src/connman.h
> @@ -796,6 +796,8 @@ void __connman_service_notify(struct
> connman_service *service,
> ?int __connman_service_counter_register(const char *counter);
> ?void __connman_service_counter_unregister(const char *counter);
> ?
> +void __connman_service_enable_autoconnect(bool autoconnect);
> +
> ?#include <connman/peer.h>
> ?
> ?int __connman_peer_init(void);
> diff --git a/src/network.c b/src/network.c
> index 2e423bc..e24970d 100644
> --- a/src/network.c
> +++ b/src/network.c
> @@ -1370,6 +1370,8 @@ void connman_network_clear_hidden(void
> *user_data)
> ?
> ? DBG("user_data %p", user_data);
> ?
> + __connman_service_enable_autoconnect(true);
> +
> ? /*
> ? ?* Hidden service does not have a connect timeout so
> ? ?* we do not need to remove it. We can just return
> @@ -1389,6 +1391,8 @@ int connman_network_connect_hidden(struct
> connman_network *network,
> ?
> ? DBG("network %p service %p user_data %p", network, service,
> user_data);
> ?
> + __connman_service_enable_autoconnect(true);
> +
> ? if (!service)
> ? return -EINVAL;
> ?
> diff --git a/src/service.c b/src/service.c
> index f6a76f6..316581a 100644
> --- a/src/service.c
> +++ b/src/service.c
> @@ -48,6 +48,7 @@ static unsigned int autoconnect_timeout = 0;
> ?static unsigned int vpn_autoconnect_timeout = 0;
> ?static struct connman_service *current_default = NULL;
> ?static bool services_dirty = false;
> +static bool allow_autoconnect = true;
> ?
> ?struct connman_stats {
> ? bool valid;
> @@ -3849,6 +3850,9 @@ void __connman_service_auto_connect(enum
> connman_service_connect_reason reason)
> ? if (autoconnect_timeout != 0)
> ? return;
> ?
> + if (!allow_autoconnect)
> + return;
> +
> ? if (!__connman_session_policy_autoconnect(reason))
> ? return;
> ?
> @@ -4025,6 +4029,8 @@ static DBusMessage
> *connect_service(DBusConnection *conn,
> ?
> ? index = __connman_service_get_index(service);
> ?
> + allow_autoconnect = false;
> +
> ? for (list = service_list; list; list = list->next) {
> ? struct connman_service *temp = list->data;
> ?
> @@ -4042,8 +4048,10 @@ static DBusMessage
> *connect_service(DBusConnection *conn,
> ? err = -EINPROGRESS;
> ?
> ? }
> - if (err == -EINPROGRESS)
> + if (err == -EINPROGRESS) {
> + allow_autoconnect = true;
> ? return __connman_error_operation_timeout(msg);
> + }
> ?
> ? service->ignore = false;
> ?
> @@ -4060,8 +4068,10 @@ static DBusMessage
> *connect_service(DBusConnection *conn,
> ? service->pending = NULL;
> ? }
> ?
> - if (err < 0)
> + if (err < 0) {
> + allow_autoconnect = true;
> ? return __connman_error_failed(msg, -err);
> + }
> ?
> ? return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
> ?}
> @@ -5231,6 +5241,9 @@ static void request_input_cb(struct
> connman_service *service,
> ? service_complete(service);
> ? __connman_connection_update_gateway();
> ? }
> +
> + if (!service->hidden)
> + allow_autoconnect = true;
> ?}
> ?
> ?static void downgrade_connected_services(void)
> @@ -5921,6 +5934,11 @@ static void prepare_8021x(struct
> connman_service *service)
> ? service-
> >phase2);
> ?}
> ?
> +void __connman_service_enable_autoconnect(bool autoconnect)
> +{
> + allow_autoconnect = autoconnect;
> +}
> +
> ?static int service_connect(struct connman_service *service)
> ?{
> ? int err;
This enabling and disabling of autoconnect looks a bit complicated.
When the previous service is being disconnected, does the disconnection
go like this:
- src/service.c, 6132, err = service_connect(service);
- src/service.c, 6072, err = __connman_network_connect(service->network);
- src/network.c, 1446, __connman_device_disconnect(network->device);
- src/device.c, 645, __connman_network_disconnect(network);
So in this case it should be the plugin itself that needs to decide
whether and when another network needs to be disconnected? That way it
can keep the network connected until it finds the hidden network it is
looking for?
With that the call to __connman_network_disconnect() could then be
removed, but needs to move into the plugins? Or only look out for
hidden WiFis?
Cheers,
Patrik
------------------------------
Message: 4
Date: Mon, 14 Nov 2016 11:38:28 +0100
From: Daniel Wagner <[email protected]>
To: Patrik Flykt <[email protected]>, Saurav Babu
<[email protected]>, [email protected]
Cc: [email protected]
Subject: Re: [PATCH v3] service: Block auto connect when connection is
in progress
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
> This enabling and disabling of autoconnect looks a bit complicated.
> When the previous service is being disconnected, does the disconnection
> go like this:
> - src/service.c, 6132, err = service_connect(service);
> - src/service.c, 6072, err = __connman_network_connect(service->network);
> - src/network.c, 1446, __connman_device_disconnect(network->device);
> - src/device.c, 645, __connman_network_disconnect(network);
>
> So in this case it should be the plugin itself that needs to decide
> whether and when another network needs to be disconnected? That way it
> can keep the network connected until it finds the hidden network it is
> looking for?
>
> With that the call to __connman_network_disconnect() could then be
> removed, but needs to move into the plugins? Or only look out for
> hidden WiFis?
You bring up a very good point. The plugins or daemon for controlling
the connection start to get smarter. For example, iwd does all the
autoconnect on its own. That means we might have to rethink what happens
on a higher level.
------------------------------
Message: 5
Date: Mon, 14 Nov 2016 13:12:26 +0100
From: Daniel Wagner <[email protected]>
To: Ioan-Adrian Ratiu <[email protected]>, [email protected]
Subject: Re: [PATCH v4 2/4] service: implement
AlwaysConnectedTechnologies option
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252; format=flowed
Hi Ioan-Adrian,
I have missed this last time, sorry about that.
> +static bool always_connect(enum connman_service_type type)
> +{
> + unsigned int *always_connected_techs =
> + connman_setting_get_uint_list("AlwaysConnectedTechnologies");
> + int i;
> +
> + for (i = 0; always_connected_techs && always_connected_techs[i]; i++)
> + if (always_connected_techs[i] == type)
> + return true;
> +
> + return false;
> +}
While this is correct, it stands a bit out. From patch 3:
+static bool autoconnect_already_connecting(struct connman_service *service,
+ bool autoconnecting)
+{
+ /*
+ * If another service is already connecting and this service type has
+ * not been marked as always connecting, stop the connecting procedure.
+ */
+ if (autoconnecting &&
+ !active_sessions[service->type] &&
+ !always_connect(service->type))
+ return true;
+
+ return false;
+}
So we have active_sessions as array and always_connect as function. Both
do the same thing. Return true if for given type we want a connection or
not.
I would propose to change always_connect to be an array like
active_sessions. How often is the AlwaysConnectedTechnologies going to
change during runtime anyway?
cheers,
daniel
------------------------------
Message: 6
Date: Mon, 14 Nov 2016 14:30:01 +0200
From: Patrik Flykt <[email protected]>
To: Daniel Wagner <[email protected]>, Robert Tiemann <[email protected]>,
[email protected]
Subject: Re: Spurious failures starting ConnMan with systemd
Message-ID: <[email protected]>
Content-Type: text/plain; charset="UTF-8"
On Fri, 2016-11-11 at 12:04 +0100, Daniel Wagner wrote:
> Hi
>
> On 11/11/2016 11:05 AM, Robert Tiemann wrote:
> > Now, I think the After=dbus.service line in src/connman.service.in
> > is
> > not enough because After= specifies order, but not a requirement
> > dependency. This what Requires= is there for. I've added the line
> >
> > ????Requires=dbus.service
> >
> > to the service file, reverted my other changes, and tried again.
> > I've
> > been unable to reproduce the problem ever since, so for me the
> > problem
> > seems to be fixed.
>
> I am no systemd expert. I just did a quick read in the documentation
> and?
> it seems it is recommended to have After= and Requires= together:
>
>
> https://www.freedesktop.org/software/systemd/man/systemd.unit.html
>
> """
> Before=, After=
>
> ?????A space-separated list of unit names. Configures ordering?
> dependencies between units. If a unit foo.service contains a setting?
> Before=bar.service and both units are being started, bar.service's?
> start-up is delayed until foo.service is started up. Note that this?
> setting is independent of and orthogonal to the requirement
> dependencies?
> as configured by Requires=. It is a common pattern to include a unit?
> name in both the After= and Requires= option, in which case the unit?
> listed will be started before the unit that is configured with these?
> options.
> """
>
> Care to send a patch which adds Requires=? Please include your
> excellent?error report into the commit message.
Hmm, then again man systemd.service says that "...Service units with
this option configured implicitly gain dependencies on the dbus.socket
unit. This type is the default if BusName= is specified..."
ConnMan defines both Type=dbus and Busname=net.connman, so it should
already have these dependencies. Unless the DefaultDependencies=false
overrides this behavior. Do we have any systemd experts around?
Cheers,
Patrik
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list
[email protected]
https://lists.01.org/mailman/listinfo/connman
------------------------------
End of connman Digest, Vol 13, Issue 21
***************************************
