Hi, Please inline your patch next time. It makes reviewing simpler On 06.11.18 01:36, Володимир Остап wrote: Argh not again wifi scanning troubles. Somehow this is really hard to get right with wpa_supplicant. Anyway thanks for the bug report including the patch. Though I have a few comments: diff --git a/include/device.h b/include/device.h I am not partucilar fan of adding a bool for this, because it is already hard to figure out which parameters is what. Adding a bool makes it even harder. Why not defining a enum for the forced scan? and here I would change the text to 'scan type {forced|normal}' If it is a forced scan, I guess we should do a active scan instead of a passive. So you need to drop the scan_param and fall through to the passive case. Add a __connman_device_request_full_scan() function and then you don't have to modify this call side. As mention above add a __connman_device_request_full_scan() function and leave the original scan() function as it is. As said, the 'false' is kind of not telling what it does. Havig an enum for this makes it more readable. You rename this function to device_requet_scan() and make it static and call let __connman_device_request_scan() and __connman_device_request_full_scan() call it. Thanks, Daniel

Hi Vasyl and Daniel, Thank you for reviewing my patch. I agree with Vasyl to have a dedicated structure scan_params. I will go this way. I will also prepare changes recommended by Daniel except enum. Enum with 2 types is as bad as bool. ;-) Daniel thank you for pointing me a flaw with PASSIVE scan. It should help me in my work as well. PS. I am busy with my job so do not anticipate quick responses :-( On Fri, Nov 16, 2018 at 12:19 AM Vasyl Vavrychuk < vasyl.vavrychuk(a)globallogic.com&gt; wrote:

Here is modified patch, changes: - added enum to device scan function - changed wifi_scan to fall into ACTIVE scan. Thank you very much Daniel, it helped me with my other bug. - renamed functions PS. There is another patch coming which fixes gsupplicant/supplicant.c interface_scan_params() not to fail wpa_supplicant's scan. ======================================= diff --git a/include/device.h b/include/device.h index 5a3ddc22..a9ffc7a8 100644 --- a/include/device.h +++ b/include/device.h @@ -50,6 +50,17 @@ enum connman_device_type { #define CONNMAN_DEVICE_PRIORITY_DEFAULT 0 #define CONNMAN_DEVICE_PRIORITY_HIGH 100 +struct scan_parameters { + enum connman_service_type type; + const char *ssid; + unsigned int ssid_len; + const char *identity; + const char* passphrase; + const char *security; + bool force_full_scan; + void *user_data; +}; + struct connman_device; struct connman_device *connman_device_create(const char *node, @@ -120,11 +131,8 @@ struct connman_device_driver { void (*remove) (struct connman_device *device); int (*enable) (struct connman_device *device); int (*disable) (struct connman_device *device); - int (*scan)(enum connman_service_type type, - struct connman_device *device, - const char *ssid, unsigned int ssid_len, - const char *identity, const char* passphrase, - const char *security, void *user_data); + int (*scan)(struct connman_device *device, + struct scan_parameters *parameters); void (*stop_scan) (enum connman_service_type type, struct connman_device *device); int (*set_regdom) (struct connman_device *device, diff --git a/plugins/wifi.c b/plugins/wifi.c index e437daeb..b702de16 100644 --- a/plugins/wifi.c +++ b/plugins/wifi.c @@ -1852,11 +1852,8 @@ static int p2p_find(struct connman_device *device) * Note that the hidden scan is only used when connecting to this specific * hidden AP first time. It is not used when system autoconnects to hidden AP. */ -static int wifi_scan(enum connman_service_type type, - struct connman_device *device, - const char *ssid, unsigned int ssid_len, - const char *identity, const char* passphrase, - const char *security, void *user_data) +static int wifi_scan(struct connman_device *device, + struct scan_parameters *parameters) { struct wifi_data *wifi = connman_device_get_data(device); GSupplicantScanParams *scan_params = NULL; @@ -1876,14 +1873,17 @@ static int wifi_scan(enum connman_service_type type, if (wifi->tethering) return -EBUSY; - if (type == CONNMAN_SERVICE_TYPE_P2P) + if (parameters->type == CONNMAN_SERVICE_TYPE_P2P) return p2p_find(device); - DBG("device %p wifi %p hidden ssid %s", device, wifi->interface, ssid); + DBG("device %p wifi %p hidden ssid %s forced full scan %s", device, + wifi->interface, parameters->ssid, parameters->force_full_scan ? + "forced" : "normal"); scanning = connman_device_get_scanning(device, CONNMAN_SERVICE_TYPE_WIFI); - if (!ssid || ssid_len == 0 || ssid_len > 32) { + if (!parameters->ssid || parameters->ssid_len == 0 || + parameters->ssid_len > 32) { if (scanning) return -EALREADY; @@ -1912,8 +1912,8 @@ static int wifi_scan(enum connman_service_type type, return -ENOMEM; } - memcpy(scan_ssid->ssid, ssid, ssid_len); - scan_ssid->ssid_len = ssid_len; + memcpy(scan_ssid->ssid, parameters->ssid, parameters->ssid_len); + scan_ssid->ssid_len = parameters->ssid_len; scan_params->ssids = g_slist_prepend(scan_params->ssids, scan_ssid); scan_params->num_ssids = 1; @@ -1929,12 +1929,12 @@ static int wifi_scan(enum connman_service_type type, wifi->hidden = NULL; } - memcpy(hidden->ssid, ssid, ssid_len); - hidden->ssid_len = ssid_len; - hidden->identity = g_strdup(identity); - hidden->passphrase = g_strdup(passphrase); - hidden->security = g_strdup(security); - hidden->user_data = user_data; + memcpy(hidden->ssid, parameters->ssid, parameters->ssid_len); + hidden->ssid_len = parameters->ssid_len; + hidden->identity = g_strdup(parameters->identity); + hidden->passphrase = g_strdup(parameters->passphrase); + hidden->security = g_strdup(parameters->security); + hidden->user_data = parameters->user_data; wifi->hidden = hidden; if (scanning) { @@ -1947,8 +1947,8 @@ static int wifi_scan(enum connman_service_type type, } } else if (wifi->connected) { g_supplicant_free_scan_params(scan_params); - return wifi_scan_simple(device); - } else { + return wifi_scan_simple(device); + } else if (!parameters->force_full_scan) { ret = get_latest_connections(driver_max_ssids, scan_params); if (ret <= 0) { g_supplicant_free_scan_params(scan_params); diff --git a/src/connman.h b/src/connman.h index c4190fd0..c29de4b2 100644 --- a/src/connman.h +++ b/src/connman.h @@ -566,6 +566,7 @@ void __connman_device_list(DBusMessageIter *iter, void *user_data); enum connman_service_type __connman_device_get_service_type(struct connman_device *device); struct connman_device *__connman_device_find_device(enum connman_service_type type); int __connman_device_request_scan(enum connman_service_type type); +int __connman_device_request_scan_full(enum connman_service_type type); int __connman_device_request_hidden_scan(struct connman_device *device, const char *ssid, unsigned int ssid_len, const char *identity, const char *passphrase, diff --git a/src/device.c b/src/device.c index 5d343ae8..6d9a8374 100644 --- a/src/device.c +++ b/src/device.c @@ -613,10 +613,14 @@ int connman_device_set_powered(struct connman_device *device, for (i = 0; i < MAX_CONNMAN_SERVICE_TYPES; i++) device->scanning[i] = false; - if (device->driver && device->driver->scan) - device->driver->scan(CONNMAN_SERVICE_TYPE_UNKNOWN, device, - NULL, 0, NULL, NULL, NULL, NULL); - + if (device->driver && device->driver->scan) { + struct scan_parameters parameters; + memset(&parameters, 0, sizeof(parameters)); + parameters.type = CONNMAN_SERVICE_TYPE_UNKNOWN; + parameters.force_full_scan = false; + + device->driver->scan(device, &parameters); + } return 0; } @@ -626,7 +630,8 @@ bool connman_device_get_powered(struct connman_device *device) } static int device_scan(enum connman_service_type type, - struct connman_device *device) + struct connman_device *device, + bool force_full_scan) { if (!device->driver || !device->driver->scan) return -EOPNOTSUPP; @@ -634,8 +639,12 @@ static int device_scan(enum connman_service_type type, if (!device->powered) return -ENOLINK; - return device->driver->scan(type, device, NULL, 0, - NULL, NULL, NULL, NULL); + struct scan_parameters parameters; + memset(&parameters, 0, sizeof(parameters)); + parameters.type = type; + parameters.force_full_scan = force_full_scan; + + return device->driver->scan(device, &parameters); } int __connman_device_disconnect(struct connman_device *device) @@ -1080,7 +1089,8 @@ void connman_device_regdom_notify(struct connman_device *device, __connman_technology_notify_regdom_by_device(device, result, alpha2); } -int __connman_device_request_scan(enum connman_service_type type) +static int connman_device_request_scan(enum connman_service_type type, + bool force_full_scan) { bool success = false; int last_err = -ENOSYS; @@ -1108,7 +1118,7 @@ int __connman_device_request_scan(enum connman_service_type type) if (!device_has_service_type(device, type)) continue; - err = device_scan(type, device); + err = device_scan(type, device, force_full_scan); if (err == 0 || err == -EALREADY || err == -EINPROGRESS) { success = true; } else { @@ -1123,6 +1133,16 @@ int __connman_device_request_scan(enum connman_service_type type) return last_err; } +int __connman_device_request_scan(enum connman_service_type type) +{ + return connman_device_request_scan(type, false); +} + +int __connman_device_request_scan_full(enum connman_service_type type) +{ + return connman_device_request_scan(type, true); +} + int __connman_device_request_hidden_scan(struct connman_device *device, const char *ssid, unsigned int ssid_len, const char *identity, const char *passphrase, @@ -1134,9 +1154,17 @@ int __connman_device_request_hidden_scan(struct connman_device *device, !device->driver->scan) return -EINVAL; - return device->driver->scan(CONNMAN_SERVICE_TYPE_UNKNOWN, - device, ssid, ssid_len, identity, - passphrase, security, user_data); + struct scan_parameters parameters; + parameters.type = CONNMAN_SERVICE_TYPE_UNKNOWN; + parameters.ssid = ssid; + parameters.ssid_len = ssid_len; + parameters.identity = identity; + parameters.passphrase = passphrase; + parameters.security = security; + parameters.user_data = user_data; + parameters.force_full_scan = false; + + return device->driver->scan(device, &parameters); } void __connman_device_stop_scan(enum connman_service_type type) diff --git a/src/technology.c b/src/technology.c index 78550f48..4e053fc9 100644 --- a/src/technology.c +++ b/src/technology.c @@ -1087,7 +1087,7 @@ static DBusMessage *scan(DBusConnection *conn, DBusMessage *msg, void *data) technology->scan_pending = g_slist_prepend(technology->scan_pending, msg); - err = __connman_device_request_scan(technology->type); + err = __connman_device_request_scan_full(technology->type); if (err < 0) reply_scan_pending(technology, err); On Fri, Nov 16, 2018 at 12:19 AM Vasyl Vavrychuk < vasyl.vavrychuk(a)globallogic.com&gt; wrote:

--- include/device.h | 17 ++++++++++++----- plugins/wifi.c | 31 +++++++++++++++---------------- src/device.c | 29 +++++++++++++++++++++-------- 3 files changed, 48 insertions(+), 29 deletions(-) diff --git a/include/device.h b/include/device.h index 5a3ddc22..ebc9df00 100644 --- a/include/device.h +++ b/include/device.h @@ -50,6 +50,16 @@ enum connman_device_type { #define CONNMAN_DEVICE_PRIORITY_DEFAULT 0 #define CONNMAN_DEVICE_PRIORITY_HIGH 100 +struct scan_parameters { + enum connman_service_type type; + const char *ssid; + unsigned int ssid_len; + const char *identity; + const char* passphrase; + const char *security; + void *user_data; +}; + struct connman_device; struct connman_device *connman_device_create(const char *node, @@ -120,11 +130,8 @@ struct connman_device_driver { void (*remove) (struct connman_device *device); int (*enable) (struct connman_device *device); int (*disable) (struct connman_device *device); - int (*scan)(enum connman_service_type type, - struct connman_device *device, - const char *ssid, unsigned int ssid_len, - const char *identity, const char* passphrase, - const char *security, void *user_data); + int (*scan)(struct connman_device *device, + struct scan_parameters *parameters); void (*stop_scan) (enum connman_service_type type, struct connman_device *device); int (*set_regdom) (struct connman_device *device, diff --git a/plugins/wifi.c b/plugins/wifi.c index 6fa20312..f41e8ae1 100644 --- a/plugins/wifi.c +++ b/plugins/wifi.c @@ -1853,11 +1853,8 @@ static int p2p_find(struct connman_device *device) * Note that the hidden scan is only used when connecting to this specific * hidden AP first time. It is not used when system autoconnects to hidden AP. */ -static int wifi_scan(enum connman_service_type type, - struct connman_device *device, - const char *ssid, unsigned int ssid_len, - const char *identity, const char* passphrase, - const char *security, void *user_data) +static int wifi_scan(struct connman_device *device, + struct scan_parameters *parameters) { struct wifi_data *wifi = connman_device_get_data(device); GSupplicantScanParams *scan_params = NULL; @@ -1877,14 +1874,16 @@ static int wifi_scan(enum connman_service_type type, if (wifi->tethering) return -EBUSY; - if (type == CONNMAN_SERVICE_TYPE_P2P) + if (parameters->type == CONNMAN_SERVICE_TYPE_P2P) return p2p_find(device); - DBG("device %p wifi %p hidden ssid %s", device, wifi->interface, ssid); + DBG("device %p wifi %p hidden ssid %s", device, wifi->interface, + parameters->ssid); scanning = connman_device_get_scanning(device, CONNMAN_SERVICE_TYPE_WIFI); - if (!ssid || ssid_len == 0 || ssid_len > 32) { + if (!parameters->ssid || parameters->ssid_len == 0 || + parameters->ssid_len > 32) { if (scanning) return -EALREADY; @@ -1913,8 +1912,8 @@ static int wifi_scan(enum connman_service_type type, return -ENOMEM; } - memcpy(scan_ssid->ssid, ssid, ssid_len); - scan_ssid->ssid_len = ssid_len; + memcpy(scan_ssid->ssid, parameters->ssid, parameters->ssid_len); + scan_ssid->ssid_len = parameters->ssid_len; scan_params->ssids = g_slist_prepend(scan_params->ssids, scan_ssid); scan_params->num_ssids = 1; @@ -1930,12 +1929,12 @@ static int wifi_scan(enum connman_service_type type, wifi->hidden = NULL; } - memcpy(hidden->ssid, ssid, ssid_len); - hidden->ssid_len = ssid_len; - hidden->identity = g_strdup(identity); - hidden->passphrase = g_strdup(passphrase); - hidden->security = g_strdup(security); - hidden->user_data = user_data; + memcpy(hidden->ssid, parameters->ssid, parameters->ssid_len); + hidden->ssid_len = parameters->ssid_len; + hidden->identity = g_strdup(parameters->identity); + hidden->passphrase = g_strdup(parameters->passphrase); + hidden->security = g_strdup(parameters->security); + hidden->user_data = parameters->user_data; wifi->hidden = hidden; if (scanning) { diff --git a/src/device.c b/src/device.c index 5d343ae8..f0994035 100644 --- a/src/device.c +++ b/src/device.c @@ -613,10 +613,13 @@ int connman_device_set_powered(struct connman_device *device, for (i = 0; i < MAX_CONNMAN_SERVICE_TYPES; i++) device->scanning[i] = false; - if (device->driver && device->driver->scan) - device->driver->scan(CONNMAN_SERVICE_TYPE_UNKNOWN, device, - NULL, 0, NULL, NULL, NULL, NULL); + if (device->driver && device->driver->scan) { + struct scan_parameters parameters; + memset(&parameters, 0, sizeof(parameters)); + parameters.type = CONNMAN_SERVICE_TYPE_UNKNOWN; + device->driver->scan(device, &parameters); + } return 0; } @@ -634,8 +637,11 @@ static int device_scan(enum connman_service_type type, if (!device->powered) return -ENOLINK; - return device->driver->scan(type, device, NULL, 0, - NULL, NULL, NULL, NULL); + struct scan_parameters parameters; + memset(&parameters, 0, sizeof(parameters)); + parameters.type = type; + + return device->driver->scan(device, &parameters); } int __connman_device_disconnect(struct connman_device *device) @@ -1134,9 +1140,16 @@ int __connman_device_request_hidden_scan(struct connman_device *device, !device->driver->scan) return -EINVAL; - return device->driver->scan(CONNMAN_SERVICE_TYPE_UNKNOWN, - device, ssid, ssid_len, identity, - passphrase, security, user_data); + struct scan_parameters parameters; + parameters.type = CONNMAN_SERVICE_TYPE_UNKNOWN; + parameters.ssid = ssid; + parameters.ssid_len = ssid_len; + parameters.identity = identity; + parameters.passphrase = passphrase; + parameters.security = security; + parameters.user_data = user_data; + + return device->driver->scan(device, &parameters); } void __connman_device_stop_scan(enum connman_service_type type) -- 2.17.1