Hi,
On 08/31/2018 01:17 PM, Natsuki.Itaya(a)sony.com wrote:
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 <Natsuki.Itaya(a)sony.com>
---
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