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 1/4] wps: add new WPS command for connmanctl
(Daniel Wagner)
2. Re: [PATCH v2 2/4] wps: add new WPS API implementation
(Daniel Wagner)
3. Re: [PATCH v2 3/4] wps: add new interface to communicate
wpa_supplicant (Daniel Wagner)
4. Re: [PATCH 4/4] wps: add new WPS API for technology
(Daniel Wagner)
----------------------------------------------------------------------
Message: 1
Date: Tue, 18 Sep 2018 08:18:11 +0200
From: Daniel Wagner <[email protected]>
To: [email protected], 'Marcel Holtmann' <[email protected]>
Cc: "[email protected]" <[email protected]>, [email protected]
Subject: Re: [PATCH v2 1/4] wps: add new WPS command for connmanctl
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Hi Natsuki,
On 08/31/2018 01:10 PM, [email protected] wrote:
> Add new WPS start/cancel command.
> Add WPS success/fail event handler.
>
> Signed-off-by: n-itaya <[email protected]>
> ---
> client/commands.c | 126 +++++++++++++++++++++++++++++++++++++++++
> client/dbus_helpers.c | 2 +-
> doc/agent-api.txt | 6 ++
> doc/technology-api.txt | 43 ++++++++++++++
Send the API changes (agent-api.txt and technology-api.txt) as first
patch and then the changes.
> diff --git a/doc/agent-api.txt b/doc/agent-api.txt
> index e3c1dcde..24b7f762 100644
> --- a/doc/agent-api.txt
> +++ b/doc/agent-api.txt
> @@ -141,6 +141,12 @@ Fields string Name
> In case of a RequestPeerAuthorization, this field will
> be set as mandatory.
>
> + To use this in order to get connected to a given
> + services is deprecated and should not be used. Methods
> + Technology.Start_STA_WPS and Technology.Start_AP_WPS
> + are provided by ConnMan and user should use those two
> + instead.
> +
> string Username
>
> Username for WISPr authentication. This field will be
> diff --git a/doc/technology-api.txt b/doc/technology-api.txt
> index f22e9b29..75c1091d 100644
> --- a/doc/technology-api.txt
> +++ b/doc/technology-api.txt
> @@ -40,6 +40,49 @@ Methods dict GetProperties() [deprecated]
> via the PeersChanged signal from the manager
> interface.
>
> + void Start_AP_WPS(string authentication)
No underscore. Just camelcase.
> +
> + Start a WPS Session when the system is playing AP
> + role (Tethering) in order to allow potential Ex-STAs
> + to get connected by using WPS.
> +
> + The argument indicates the WPS authentication method
> + which can be an empty string, if user wants to use
> + push-button method, or a pin code if user wants to
> + use the pin method.
> +
> + This method is supported only by WiFi technology.
> +
> + Possible Errors: [service].Error.InvalidArguments
> + [service].Error.PermissionDenied
> + [service].Error.NotSupported
> +
> + void Start_STA_WPS(string authentication)
> +
> + Start a WPS Session in STA role in order to be able
> + to get connected with an Ex-AP with WPS capabilities.
> +
> + The argument indicates the WPS authentication method
> + which can be an empty string, if user wants to use
> + push-button method, or a pin code if user wants to
> + use the pin method.
> +
> + This method is supported only by WiFi technology.
> +
> + Possible Errors: [service].Error.InvalidArguments
> + [service].Error.OperationAborted
> + [service].Error.OperationTimeout
> + [service].Error.NotConnected
> + [service].Error.NotSupported
> +
> + void Cancel_WPS()
> +
> + Cancel ongoing WPS session.
> +
> + This method is supported only by WiFi technology.
> +
> + Possible Errors: [service].Error.NotSupported
> +
I still fail to see why these functions need to be global on
technologies. The reason I bring this question up again, this is not
really fitting into the exiting API design. The agent API allows ConnMan
to track the live cycle of the user application. That is we know when to
stop operations or just send to the specific agent a signal instead of a
global one.
Furthermore, we can't really deprecated the existing agent API in the
1.x ConnMan series since it is marked as stable.
@Marcel do you have any input on this API change proposal?
Thanks,
Daniel
------------------------------
Message: 2
Date: Tue, 18 Sep 2018 08:43:59 +0200
From: Daniel Wagner <[email protected]>
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: [PATCH v2 2/4] wps: add new WPS API implementation
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Hi,
On 08/31/2018 01:11 PM, [email protected] wrote:
> This is main part of this patch series.
> Add new WPS start API which does not specify any service.
> Add the handler which receive WPS credential and force save information
> into applicable service.
> Add new status property for display WPS status.
>
> Remove exist WPS API.
Is this a left over? Because I don't think you remove the API. In any
case, you should not remove it because it breaks existing users.
A couple of nitpicks. Overall it looks pretty good.
> +static int peer_disconnect_all(struct wifi_data *wifi)
> +{
> + GSList *list;
> + int result = 0, err = -EALREADY;
> +
> + DBG("");
> +
> + for (list = wifi->peers; list; list = list->next) {
> + struct connman_peer *peer = list->data;
> + GSupplicantPeer *gs_peer =
> + g_supplicant_interface_peer_lookup
> + (wifi->interface, connman_peer_get_identifier(peer));
Only use tabs for indentions. We don't mix tabs and spaces.
> +
> + if (g_supplicant_peer_is_in_a_group(gs_peer)) {
> + err = peer_disconnect(peer);
> + if (err != -EINPROGRESS)
> + result = err;
> + }
> + }
> +
> + return result;
> +}
> +
> @@ -2375,6 +2434,8 @@ static bool handle_wps_completion(GSupplicantInterface
> *interface,
> {
> bool wps;
>
> + DBG("");
> +
The patch adds many DBGs. That is okay for the initial stabilization and
we can leave them in for a while. But we should try to remove them after
a few months of testing because it usually just filling up the log and
it is hard to read anything anymore. But as I said that is something we
can change later.
> wps = connman_network_get_bool(network, "WiFi.UseWPS");
> if (wps) {
> const unsigned char *ssid, *wps_ssid;
> @@ -2486,9 +2547,40 @@ static void interface_state(GSupplicantInterface
> *interface)
> finalize_interface_creation(wifi);
> }
>
> + if (wifi->wps_running) {
> + DBG("WPS running");
> + switch (state) {
> + case G_SUPPLICANT_STATE_SCANNING:
> + if (!wifi->wps_success)
> + connman_technology_wps_state_change_notify
> + (wifi_technology, "scan");
> + return;
> + case G_SUPPLICANT_STATE_ASSOCIATED:
> + connman_technology_wps_state_change_notify
> + (wifi_technology, "processing");
> + return;
> + case G_SUPPLICANT_STATE_DISCONNECTED:
> + if (wifi->wps_success) {
> + if (wps_try_update_services(wifi)) {
> + wps_finish(wifi, 0);
> + } else {
> + wifi->wps_after_scan = true;
> + DBG("Need scan");
> + reset_autoscan(wifi->device);
> + throw_wifi_scan(device,
> + wps_scan_callback);
> + }
Usually the bigger basic block goes first, so that the else basic block
is smaller. In this case it makes it even a bit more natural to read in
the flow. So the 'do something' comes first and the 'end something'
afterwards.
> + }
> + return;
> + default:
> + return;
> + }
> + }
> +
> network = wifi->network;
> - if (!network)
> + if (!network) {
> return;
> + }
Unnecessary change here. No need for adding the brackets.
> --- a/src/connman.h
> +++ b/src/connman.h
> @@ -54,6 +54,8 @@ DBusMessage *__connman_error_operation_aborted(DBusMessage
> *msg);
> DBusMessage *__connman_error_operation_timeout(DBusMessage *msg);
> DBusMessage *__connman_error_invalid_service(DBusMessage *msg);
> DBusMessage *__connman_error_invalid_property(DBusMessage *msg);
> +DBusMessage *__connman_error_pin_rejected(DBusMessage *msg);
> +DBusMessage *__connman_error_pbc_overlap(DBusMessage *msg);
>
> int __connman_manager_init(void);
> void __connman_manager_cleanup(void);
> @@ -665,6 +667,8 @@ int __connman_service_init(void);
> void __connman_service_cleanup(void);
> int __connman_service_load_modifiable(struct connman_service *service);
>
> +void __connman_service_append_struct(struct connman_service *service,
> + DBusMessageIter *iter);
> void __connman_service_list_struct(DBusMessageIter *iter);
>
> int __connman_service_compare(const struct connman_service *a,
> @@ -843,6 +847,7 @@ int __connman_peer_service_unregister(const char *owner,
> int specification_length,
> const unsigned char *query,
> int query_length, int version);
> +enum connman_peer_wps_method __connman_check_wps_method(const char *wpspin);
>
> #include <connman/session.h>
>
> @@ -853,6 +858,7 @@ int __connman_service_iterate_services(service_iterate_cb
> cb, void *user_data);
>
> void __connman_service_mark_dirty();
> void __connman_service_save(struct connman_service *service);
> +void __connman_service_force_save(struct connman_service *service);
Can you factor this and __connman_service_append_struct() out into a
patch of its own? In ConnMan we tend to make smaller steps in changes.
>
> #include <connman/notifier.h>
>
> diff --git a/src/error.c b/src/error.c
> index 4f24ae25..f0b35bdc 100644
> --- a/src/error.c
> +++ b/src/error.c
> @@ -67,6 +67,10 @@ DBusMessage *__connman_error_failed(DBusMessage *msg, int
> errnum)
> return __connman_error_in_progress(msg);
> case ENOKEY:
> return __connman_error_passphrase_required(msg);
> + case EKEYREJECTED:
> + return __connman_error_pin_rejected(msg);
> + case EAGAIN:
> + return __connman_error_pbc_overlap(msg);
> }
>
> return g_dbus_create_error(msg, CONNMAN_ERROR_INTERFACE
> @@ -185,3 +189,15 @@ DBusMessage
> *__connman_error_invalid_property(DBusMessage *msg)
> return g_dbus_create_error(msg, CONNMAN_ERROR_INTERFACE
> ".InvalidProperty", "Invalid property");
> }
> +
> +DBusMessage *__connman_error_pin_rejected(DBusMessage *msg)
> +{
> + return g_dbus_create_error(msg, CONNMAN_ERROR_INTERFACE
> + ".PinRejected", "PIN Rejected");
> +}
> +
> +DBusMessage *__connman_error_pbc_overlap(DBusMessage *msg)
> +{
> + return g_dbus_create_error(msg, CONNMAN_ERROR_INTERFACE
> + ".PbcOverlap", "PBC Overlap");
> +}
> diff --git a/src/peer.c b/src/peer.c
> index 1b9b80e3..70ffeab4 100644
> --- a/src/peer.c
> +++ b/src/peer.c
> @@ -545,8 +545,7 @@ static const char *get_dbus_sender(struct connman_peer
> *peer)
> return dbus_message_get_sender(peer->pending);
> }
>
> -static enum connman_peer_wps_method check_wpspin(struct connman_peer *peer,
> - const char *wpspin)
> +enum connman_peer_wps_method __connman_check_wps_method(const char *wpspin)
> {
> int len, i;
>
> @@ -591,7 +590,7 @@ static void request_authorization_cb(struct connman_peer
> *peer,
> goto out;
> }
>
> - wps_method = check_wpspin(peer, wpspin);
> + wps_method = __connman_check_wps_method(wpspin);
>
> err = peer_driver->connect(peer, wps_method, wpspin);
> if (err == -EINPROGRESS)
> diff --git a/src/service.c b/src/service.c
> index 733c0728..da323d0d 100644
> --- a/src/service.c
> +++ b/src/service.c
> @@ -817,6 +817,15 @@ void __connman_service_save(struct connman_service
> *service)
> service_save(service);
> }
>
> +void __connman_service_force_save(struct connman_service *service)
> +{
> + if (!service)
> + return;
> +
> + service->new_service = false;
> + service_save(service);
> +}
> +
> static enum connman_service_state combine_state(
> enum connman_service_state state_a,
> enum connman_service_state state_b)
> @@ -2577,6 +2586,18 @@ static void append_struct(gpointer value, gpointer
> user_data)
> append_struct_service(iter, append_dict_properties, service);
> }
>
> +void __connman_service_append_struct(struct connman_service *service,
> + DBusMessageIter *iter)
> +{
> + if (!service)
> + return;
> +
> + if (!iter)
> + return;
> +
> + append_struct(service, iter);
> +}
> +
> void __connman_service_list_struct(DBusMessageIter *iter)
> {
> g_list_foreach(service_list, append_struct, iter);
>
Thanks,
Daniel
------------------------------
Message: 3
Date: Tue, 18 Sep 2018 08:50:43 +0200
From: Daniel Wagner <[email protected]>
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: [PATCH v2 3/4] wps: add new interface to communicate
wpa_supplicant
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Hi,
On 08/31/2018 01:14 PM, [email protected] wrote:
> Add new WPS start/cancel interface.
> Add wpa_supplicant wps event callback for WPS state tracing.
> Add wps credential callback to hand over into wifi plugin.
>
> Signed-off-by: n-itaya <[email protected]>
> ---
> gsupplicant/gsupplicant.h | 35 +++
> gsupplicant/supplicant.c | 485 ++++++++++++++++++++++++++++++--------
> 2 files changed, 424 insertions(+), 96 deletions(-)
Just a bunch of nitpicks.
> diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c
> index 5246c80b..2883e03e 100644
> --- a/gsupplicant/supplicant.c
> +++ b/gsupplicant/supplicant.c
> @@ -138,6 +138,24 @@ static struct strvalmap mode_capa_map[] = {
> { }
> };
>
> +static struct strvalmap wps_auth_map[] = {
> + { "open", G_SUPPLICANT_WPS_AUTH_OPEN },
> + { "shared", G_SUPPLICANT_WPS_AUTH_SHARED },
> + { "wpa-psk", G_SUPPLICANT_WPS_AUTH_WPA_PSK },
> + { "wpa-eap", G_SUPPLICANT_WPS_AUTH_WPA_EAP },
> + { "wpa2-eap", G_SUPPLICANT_WPS_AUTH_WPA2_EAP },
> + { "wpa2-psk", G_SUPPLICANT_WPS_AUTH_WPA2_PSK },
> + { }
> +};
Please virtically alignment the G_* definitions and brakets.
> static void remove_group(gpointer data)
> {
> GSupplicantGroup *group = data;
> @@ -863,8 +919,8 @@ static void interface_capability_pairwise(DBusMessageIter
> *iter,
> break;
> }
> }
> -
> static void interface_capability_group(DBusMessageIter *iter, void
> *user_data)
> +
> {
Unnecessary white space change here.
> +
> + memset(&bss, 0, sizeof(struct g_supplicant_bss));
> + memcpy(bss.ssid, interface->wps_cred.ssid,
> + interface->wps_cred.ssid_len);
> + bss.ssid_len = interface->wps_cred.ssid_len;
> + bss.mode = G_SUPPLICANT_MODE_INFRA;
> + bss.security = G_SUPPLICANT_SECURITY_UNKNOWN;
> + if (interface->wps_cred.encrtype &
> + (G_SUPPLICANT_WPS_ENCR_AES | G_SUPPLICANT_WPS_ENCR_TKIP)) {
> + bss.security = G_SUPPLICANT_SECURITY_PSK;
Just use tab indention
> @@ -4995,72 +5278,61 @@ int
> g_supplicant_interface_connect(GSupplicantInterface *interface,
> data->ssid = ssid;
> data->user_data = user_data;
>
> - if (ssid->use_wps) {
> - g_free(interface->wps_cred.key);
> - memset(&interface->wps_cred, 0,
> - sizeof(struct _GSupplicantWpsCredentials));
> + /* By the time there is a request for connect and the network
> + * path is not NULL it means that connman has not removed the
> + * previous network pointer. This can happen in the case AP
> + * deauthenticated client and connman does not remove the
> + * previously connected network pointer. This causes supplicant
> + * to reallocate the memory for struct wpa_ssid again even if it
> + * is the same SSID. This causes memory usage of wpa_supplicnat
> + * to go high. The idea here is that if the previously connected
> + * network is not removed at the time of next connection attempt
> + * check if the network path is not NULL. In case it is non-NULL
> + * first remove the network and then once removal is successful, add
> + * the network.
> + */
While at it, could you change the comment to start with
/*
* By the ...
[...]
* the network.
*/
? It's more consistent with the rest.
>
> - ret = supplicant_dbus_property_set(interface->path,
> - SUPPLICANT_INTERFACE ".Interface.WPS",
> - "ProcessCredentials", DBUS_TYPE_BOOLEAN_AS_STRING,
> - wps_process_credentials, wps_start, data, interface);
> - } else {
> - /* By the time there is a request for connect and the network
> - * path is not NULL it means that connman has not removed the
> - * previous network pointer. This can happen in the case AP
> - * deauthenticated client and connman does not remove the
> - * previously connected network pointer. This causes supplicant
> - * to reallocate the memory for struct wpa_ssid again even if it
> - * is the same SSID. This causes memory usage of wpa_supplicnat
> - * to go high. The idea here is that if the previously connected
> - * network is not removed at the time of next connection attempt
> - * check if the network path is not NULL. In case it is non-NULL
> - * first remove the network and then once removal is
> successful, add
> - * the network.
Thanks,
Daniel
------------------------------
Message: 4
Date: Tue, 18 Sep 2018 09:10:30 +0200
From: Daniel Wagner <[email protected]>
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: [PATCH 4/4] wps: add new WPS API for technology
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Hi,
On 08/31/2018 01:17 PM, [email protected] wrote:
> This implementation is follow the the original patch.
> https://lists.01.org/pipermail/connman/2018-January/022367.html
Please all relevant information directly into the commit message. Any
link will be broken in the future. Also your excellent introduction
summary would be a great additional to the commit messages. In the past
we had very terse commit message which made debugging harder then needed :)
> Signed-off-by: n-itaya <[email protected]>
> ---
> include/technology.h | 20 +++-
> src/technology.c | 233 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 252 insertions(+), 1 deletion(-)
I think this could also be easily added to agent API.
> diff --git a/include/technology.h b/include/technology.h
> index 97db6607..febf6685 100644
> --- a/include/technology.h
> +++ b/include/technology.h
> @@ -34,10 +34,19 @@ extern "C" {
> * @short_description: Functions for handling technology details
> */
>
> +enum connman_technology_wps_mode {
> + CONNMAN_TECHNOLOGY_WPS_STA_MODE = 0,
> + CONNMAN_TECHNOLOGY_WPS_AP_MODE = 1,
> +};
Align virtually
> +
> struct connman_technology;
>
> int connman_technology_tethering_notify(struct connman_technology
> *technology,
> - bool enabled);
> + bool enabled);
> +
Unnecessary white space change
> +void connman_technology_wps_state_change_notify
> +(struct connman_technology *technology, const char *state);
> +
> int connman_technology_set_regdom(const char *alpha2);
> void connman_technology_regdom_notify(struct connman_technology *technology,
> const char *alpha2);
> @@ -46,6 +55,11 @@ bool connman_technology_get_wifi_tethering(const char
> **ssid,
> const char **psk);
> bool connman_technology_is_tethering_allowed(enum connman_service_type
> type);
>
> +void connman_technology_add_wps_offered(struct connman_technology
> *technology,
> + const char *path);
> +void connman_technology_reply_start_sta_wps
> +(struct connman_technology *technology, int error);
> +
> struct connman_technology_driver {
> const char *name;
> enum connman_service_type type;
> @@ -62,6 +76,10 @@ struct connman_technology_driver {
> const char *bridge, bool enabled);
> int (*set_regdom) (struct connman_technology *technology,
> const char *alpha2);
> + int (*start_wps)(struct connman_technology *technology,
> + enum connman_technology_wps_mode mode,
> + const char *wps_pin);
> + int (*cancel_wps)(struct connman_technology *technology);
> };
>
> int connman_technology_driver_register(struct connman_technology_driver
> *driver);
> diff --git a/src/technology.c b/src/technology.c
> index 4c1cbbbb..906a6ec3 100644
> --- a/src/technology.c
> +++ b/src/technology.c
> @@ -74,6 +74,14 @@ struct connman_technology {
> DBusMessage *pending_reply;
> guint pending_timeout;
>
> + /*
> + * Used to handle WPS errors within the two-minute interval.
> + * It is done only for WPS in STA mode, because for AP the
> + * wpa_supplicant does not report any events/errors.
> + */
> + DBusMessage *wps_reply;
> + GSList *wps_offered;
> +
> GSList *scan_pending;
>
> bool rfkill_driven;
> @@ -277,6 +285,15 @@ static int set_tethering(struct connman_technology
> *technology,
> return result;
> }
>
> +void connman_technology_wps_state_change_notify(
> + struct connman_technology *technology,
> + const char *state)
One tab more so that 'struct' is more idented then connman_...()
> +{
> + connman_dbus_property_changed_basic(technology->path,
> + CONNMAN_TECHNOLOGY_INTERFACE, "WpsState",
> + DBUS_TYPE_STRING, &state);
This one sends out to all listener the state change. In the past many
D-Bus clients didn't really filter out global signals they weren't
interested. So they were woken up necessarily.
> +}
> +
> void connman_technology_regdom_notify(struct connman_technology *technology,
> const char *alpha2)
> {
> @@ -576,6 +593,214 @@ static void technology_removed_signal(struct
> connman_technology *technology)
> DBUS_TYPE_INVALID);
> }
>
> +void connman_technology_add_wps_offered(struct connman_technology
> *technology,
> + const char *path)
> +{
> + char *dup_path = g_strdup(path);
> +
> + if (!dup_path)
> + return;
> + technology->wps_offered =
> + g_slist_append(technology->wps_offered, dup_path);
Just tab
> +}
> +
> +static void append_wps_service_structs(DBusMessageIter *iter, void
> *user_data)
> +{
> + struct connman_technology *technology = user_data;
> + GSList *list;
> +
> + for (list = technology->wps_offered; list; list = list->next) {
> + const char *ident = list->data;
> + struct connman_service *service;
> +
> + service = __connman_service_lookup_from_ident(ident);
> + if (!service)
> + continue;
> + __connman_service_append_struct(service, iter);
> + }
> +}
> +
> +static DBusMessage
> +*create_reply_start_sta_wps_success(
> + struct connman_technology *technology,
> + DBusMessage *reply)
> +{
> + DBusMessage *msg;
> +
> + msg = dbus_message_new_method_return(reply);
> + if (!msg)
> + return NULL;
> +
> + __connman_dbus_append_objpath_dict_array(msg,
> + append_wps_service_structs,
> + technology);
> +
> + return msg;
> +}
> +
> +static void free_wps_offered(gpointer data, gpointer user_data)
> +{
> + if (!data)
> + return;
> +
> + g_free(data);
> +}
> +
> +void
> +connman_technology_reply_start_sta_wps(struct connman_technology *technology,
> + int error)
> +{
> + DBusMessage *reply;
> +
> + if (!technology->wps_reply)
> + return;
> +
> + if (error < 0)
> + reply = __connman_error_failed(technology->wps_reply, -error);
> + else {
> + reply = create_reply_start_sta_wps_success(technology,
> + technology->wps_reply);
> + }
Hmm, I would do it this way:
if (!error) {
[...]
} else
[...]
> +
> + g_dbus_send_message(connection, reply);
> +
> + dbus_message_unref(technology->wps_reply);
> + technology->wps_reply = NULL;
> +
> + g_slist_foreach(technology->wps_offered, free_wps_offered, NULL);
> + g_slist_free(technology->wps_offered);
> + technology->wps_offered = NULL;
> +}
> +
> +static int start_wps(struct connman_technology *technology,
> + DBusMessage *msg, enum connman_technology_wps_mode mode)
> +{
> + GSList *tech_drivers;
> + DBusMessageIter iter;
> + enum connman_peer_wps_method wps_method;
> + const char *auth;
> + int err, result = -EOPNOTSUPP;
> +
> + if (technology->type != CONNMAN_SERVICE_TYPE_WIFI)
> + return -EOPNOTSUPP;
> +
> + __sync_synchronize();
> + if (!technology->enabled)
> + return -EACCES;
> +
> + if (!dbus_message_iter_init(msg, &iter))
> + return -EINVAL;
> +
> + if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
> + return -EINVAL;
> +
> + dbus_message_iter_get_basic(&iter, &auth);
> +
> + wps_method = __connman_check_wps_method(auth);
> + if (wps_method == CONNMAN_PEER_WPS_UNKNOWN)
> + return -EINVAL;
> + if (wps_method == CONNMAN_PEER_WPS_PBC)
> + auth = NULL;
> +
> + for (tech_drivers = technology->driver_list; tech_drivers;
> + tech_drivers = g_slist_next(tech_drivers)) {
Just tabs no mixing with spaces.
> + struct connman_technology_driver *driver = tech_drivers->data;
> +
> + if (!driver ||
> + !driver->start_wps ||
> + driver->type != CONNMAN_SERVICE_TYPE_WIFI)
> + continue;
> +
> + err = driver->start_wps(technology, mode, auth);
> +
> + if (result == -EINPROGRESS)
> + continue;
> +
> + if (err == -EINPROGRESS)
> + result = err;
> + }
> +
> + return result;
> +}
> +
> +static DBusMessage *start_ap_wps(DBusConnection *conn, DBusMessage *msg,
> + void *user_data)
> +{
> + struct connman_technology *technology = user_data;
> + int err;
> +
> + /* It is required to enable tethering before starting WPS in AP mode */
> + if (!technology->tethering) {
> + DBG("Error: Tethering is required");
> + return __connman_error_permission_denied(msg);
> + }
> +
> + err = start_wps(technology, msg, CONNMAN_TECHNOLOGY_WPS_AP_MODE);
> + if (err == -EINPROGRESS)
> + return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
> +
> + return __connman_error_failed(msg, -err);
> +}
> +
> +static DBusMessage *start_sta_wps(DBusConnection *conn, DBusMessage *msg,
> + void *user_data)
> +{
> + struct connman_technology *technology = user_data;
> + int err;
> +
> + if (technology->wps_reply)
> + connman_technology_reply_start_sta_wps(technology,
> + -ECONNABORTED);
> +
> + err = start_wps(technology, msg, CONNMAN_TECHNOLOGY_WPS_STA_MODE);
> + if (err == -EINPROGRESS) {
> + technology->wps_reply = dbus_message_ref(msg);
> + return NULL;
> + }
> +
> + return __connman_error_failed(msg, -err);
> +}
> +
> +static DBusMessage *cancel_wps(DBusConnection *conn, DBusMessage *msg,
> + void *user_data)
> +{
> + struct connman_technology *technology = user_data;
> + GSList *tech_drivers;
> + int err = 0, result = -EOPNOTSUPP;
> +
> + if (technology->type != CONNMAN_SERVICE_TYPE_WIFI)
> + return __connman_error_not_supported(msg);
> +
> + __sync_synchronize();
Don't thin we need __synch_synchronize(). This runs all in the same
thread and we don't interact with external process.
> + if (!technology->enabled)
> + return __connman_error_permission_denied(msg);
> +
> + if (technology->wps_reply)
> + connman_technology_reply_start_sta_wps(technology,
> + -ECONNABORTED);
> +
> + for (tech_drivers = technology->driver_list; tech_drivers;
> + tech_drivers = g_slist_next(tech_drivers)) {
> + struct connman_technology_driver *driver = tech_drivers->data;
> +
> + if (!driver || !driver->cancel_wps ||
> + driver->type != CONNMAN_SERVICE_TYPE_WIFI)
> + continue;
> +
> + err = driver->cancel_wps(technology);
> + if (result == -EINPROGRESS)
> + continue;
> +
> + if (err == -EINPROGRESS)
> + result = err;
> + }
This parts is a bit confusing for me. Do we really need the first check
at all? I know we could assign result several times -EINPROGESS but this
is easier to read and I don't think it really matters in performance.
> +
> + if (result == -EINPROGRESS)
> + return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
> +
> + return __connman_error_failed(msg, -result);
> +}
> +
> static DBusMessage *get_properties(DBusConnection *conn,
> DBusMessage *message, void *user_data)
> {
> @@ -1098,6 +1323,14 @@ static const GDBusMethodTable technology_methods[] = {
> GDBUS_ARGS({ "name", "s" }, { "value", "v" }),
> NULL, set_property) },
> { GDBUS_ASYNC_METHOD("Scan", NULL, NULL, scan) },
> + { GDBUS_ASYNC_METHOD("Start_AP_WPS",
> + GDBUS_ARGS({ "authentication", "s" }),
> + NULL, start_ap_wps) },
> + { GDBUS_ASYNC_METHOD("Start_STA_WPS",
> + GDBUS_ARGS({ "authentication", "s" }),
> + NULL, start_sta_wps) },
> + { GDBUS_ASYNC_METHOD("Cancel_WPS",
> + NULL, NULL, cancel_wps) },
> { },
> };
The series is a good shape. The only open point for me is the API thing.
Maybe Marcel chips in as the main API designer.
Thanks,
Daniel
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list
[email protected]
https://lists.01.org/mailman/listinfo/connman
------------------------------
End of connman Digest, Vol 35, Issue 6
**************************************
