[PATCH 1/4] scan: Improve comment

[PATCH 1/4] scan: Improve comment

[Tim Kourt]

[-- Attachment #1: Type: text/plain, Size: 715 bytes --]

---
 src/scan.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/scan.c b/src/scan.c
index f2332545..4bc95afe 100644
--- a/src/scan.c
+++ b/src/scan.c
@@ -297,8 +297,8 @@ static struct l_genl_msg *scan_build_cmd(struct scan_context *sc,
 	ext_capa = wiphy_get_extended_capabilities(sc->wiphy,
 							NL80211_IFTYPE_STATION);
 	/*
-	 * XXX: If adding IE's here ensure that ordering is not broken for
-	 * probe requests (IEEE-2016 Table 9-33).
+	 * If adding IE's here ensure that ordering is not broken for
+	 * probe requests (IEEE Std 802.11-2016 Table 9-33).
 	 */
 	/* Order 9 - Extended Capabilities */
 	iov[iov_elems].iov_base = (void *) ext_capa;
-- 
2.21.0

[PATCH 2/4] scan: Fix bit checking for interworking

[Tim Kourt]

[-- Attachment #1: Type: text/plain, Size: 606 bytes --]

The checker function will later be changed to match the bit setter.
---
 src/scan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/scan.c b/src/scan.c
index 4bc95afe..f1317686 100644
--- a/src/scan.c
+++ b/src/scan.c
@@ -305,7 +305,7 @@ static struct l_genl_msg *scan_build_cmd(struct scan_context *sc,
 	iov[iov_elems].iov_len = ext_capa[1] + 2;
 	iov_elems++;
 
-	if (util_is_bit_set(ext_capa[3], 7)) {
+	if (util_is_bit_set(ext_capa[2 + 3], 7)) {
 		/* Order 12 - Interworking */
 		interworking[0] = IE_TYPE_INTERWORKING;
 		interworking[1] = 1;
-- 
2.21.0

[PATCH 3/4] scan: Separate IE attr creation into logical block

[Tim Kourt]

[-- Attachment #1: Type: text/plain, Size: 2410 bytes --]

This also introduces the max IE lenght check and exludes the addition
of IEs for the drivers that don't support it.
---
 src/scan.c | 51 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 21 deletions(-)

diff --git a/src/scan.c b/src/scan.c
index f1317686..a3d6e5d8 100644
--- a/src/scan.c
+++ b/src/scan.c
@@ -266,34 +266,15 @@ static void scan_build_attr_scan_frequencies(struct l_genl_msg *msg,
 	l_genl_msg_leave_nested(msg);
 }
 
-static bool scan_mac_address_randomization_is_disabled(void)
-{
-	const struct l_settings *config = iwd_get_config();
-	bool disabled;
-
-	if (!l_settings_get_bool(config, "Scan",
-					"DisableMacAddressRandomization",
-					&disabled))
-		return false;
-
-	return disabled;
-}
-
-static struct l_genl_msg *scan_build_cmd(struct scan_context *sc,
-					bool ignore_flush_flag, bool is_passive,
+static void scan_build_attr_ie(struct l_genl_msg *msg,
+					struct scan_context *sc,
 					const struct scan_parameters *params)
 {
-	struct l_genl_msg *msg;
-	uint32_t flags = 0;
 	struct iovec iov[3];
 	unsigned int iov_elems = 0;
 	const uint8_t *ext_capa;
 	uint8_t interworking[3];
 
-	msg = l_genl_msg_new(NL80211_CMD_TRIGGER_SCAN);
-
-	l_genl_msg_append_attr(msg, NL80211_ATTR_WDEV, 8, &sc->wdev_id);
-
 	ext_capa = wiphy_get_extended_capabilities(sc->wiphy,
 							NL80211_IFTYPE_STATION);
 	/*
@@ -325,6 +306,34 @@ static struct l_genl_msg *scan_build_cmd(struct scan_context *sc,
 	}
 
 	l_genl_msg_append_attrv(msg, NL80211_ATTR_IE, iov, iov_elems);
+}
+
+static bool scan_mac_address_randomization_is_disabled(void)
+{
+	const struct l_settings *config = iwd_get_config();
+	bool disabled;
+
+	if (!l_settings_get_bool(config, "Scan",
+					"DisableMacAddressRandomization",
+					&disabled))
+		return false;
+
+	return disabled;
+}
+
+static struct l_genl_msg *scan_build_cmd(struct scan_context *sc,
+					bool ignore_flush_flag, bool is_passive,
+					const struct scan_parameters *params)
+{
+	struct l_genl_msg *msg;
+	uint32_t flags = 0;
+
+	msg = l_genl_msg_new(NL80211_CMD_TRIGGER_SCAN);
+
+	l_genl_msg_append_attr(msg, NL80211_ATTR_WDEV, 8, &sc->wdev_id);
+
+	if (wiphy_get_max_scan_ie_len(sc->wiphy))
+		scan_build_attr_ie(msg, sc, params);
 
 	if (params->freqs)
 		scan_build_attr_scan_frequencies(msg, params->freqs);
-- 
2.21.0

[PATCH 4/4] scan: Separate cck rates attr creation into logical block

[Tim Kourt]

[-- Attachment #1: Type: text/plain, Size: 3267 bytes --]

---
 src/scan.c | 82 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 42 insertions(+), 40 deletions(-)

diff --git a/src/scan.c b/src/scan.c
index a3d6e5d8..f99ad1e7 100644
--- a/src/scan.c
+++ b/src/scan.c
@@ -308,6 +308,45 @@ static void scan_build_attr_ie(struct l_genl_msg *msg,
 	l_genl_msg_append_attrv(msg, NL80211_ATTR_IE, iov, iov_elems);
 }
 
+static void scan_build_attr_tx_no_cck_rates(struct l_genl_msg *msg,
+							struct scan_context *sc)
+{
+	static const uint8_t b_rates[] = { 2, 4, 11, 22 };
+	uint8_t *scan_rates;
+	const uint8_t *supported;
+	unsigned int num_supported;
+	unsigned int count;
+	unsigned int i;
+
+	l_genl_msg_append_attr(msg, NL80211_ATTR_TX_NO_CCK_RATE, 0, NULL);
+
+	/*
+	 * Assume if we're sending the probe requests at OFDM bit
+	 * rates we don't want to advertise support for 802.11b rates.
+	 */
+	if (L_WARN_ON(!(supported = wiphy_get_supported_rates(sc->wiphy,
+							NL80211_BAND_2GHZ,
+							&num_supported))))
+		return;
+
+	scan_rates = l_malloc(num_supported);
+
+	for (count = 0, i = 0; i < num_supported; i++)
+		if (!memchr(b_rates, supported[i], L_ARRAY_SIZE(b_rates)))
+			scan_rates[count++] = supported[i];
+
+	if (L_WARN_ON(!count)) {
+		l_free(scan_rates);
+		return;
+	}
+
+	l_genl_msg_enter_nested(msg, NL80211_ATTR_SCAN_SUPP_RATES);
+	l_genl_msg_append_attr(msg, NL80211_BAND_2GHZ, count, scan_rates);
+	l_genl_msg_leave_nested(msg);
+
+	l_free(scan_rates);
+}
+
 static bool scan_mac_address_randomization_is_disabled(void)
 {
 	const struct l_settings *config = iwd_get_config();
@@ -338,6 +377,9 @@ static struct l_genl_msg *scan_build_cmd(struct scan_context *sc,
 	if (params->freqs)
 		scan_build_attr_scan_frequencies(msg, params->freqs);
 
+	if (params->no_cck_rates)
+		scan_build_attr_tx_no_cck_rates(msg, sc);
+
 	if (params->flush && !ignore_flush_flag)
 		flags |= NL80211_SCAN_FLAG_FLUSH;
 
@@ -357,46 +399,6 @@ static struct l_genl_msg *scan_build_cmd(struct scan_context *sc,
 	if (flags)
 		l_genl_msg_append_attr(msg, NL80211_ATTR_SCAN_FLAGS, 4, &flags);
 
-	if (params->no_cck_rates) {
-		static const uint8_t b_rates[] = { 2, 4, 11, 22 };
-		uint8_t *scan_rates;
-		const uint8_t *supported;
-		unsigned int num_supported;
-		unsigned int count;
-		unsigned int i;
-
-		l_genl_msg_append_attr(msg, NL80211_ATTR_TX_NO_CCK_RATE, 0,
-					NULL);
-
-		/*
-		 * Assume if we're sending the probe requests at OFDM bit
-		 * rates we don't want to advertise support for 802.11b rates.
-		 */
-		if (L_WARN_ON(!(supported = wiphy_get_supported_rates(sc->wiphy,
-							NL80211_BAND_2GHZ,
-							&num_supported))))
-			goto done;
-
-		scan_rates = l_malloc(num_supported);
-
-		for (count = 0, i = 0; i < num_supported; i++)
-			if (!memchr(b_rates, supported[i],
-						L_ARRAY_SIZE(b_rates)))
-				scan_rates[count++] = supported[i];
-
-		if (L_WARN_ON(!count)) {
-			l_free(scan_rates);
-			goto done;
-		}
-
-		l_genl_msg_enter_nested(msg, NL80211_ATTR_SCAN_SUPP_RATES);
-		l_genl_msg_append_attr(msg, NL80211_BAND_2GHZ,
-							count, scan_rates);
-		l_genl_msg_leave_nested(msg);
-		l_free(scan_rates);
-	}
-
-done:
 	return msg;
 }
 
-- 
2.21.0

Re: [PATCH 4/4] scan: Separate cck rates attr creation into logical block

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 1489 bytes --]

Hi Tim,

> ---
> src/scan.c | 82 ++++++++++++++++++++++++++++--------------------------
> 1 file changed, 42 insertions(+), 40 deletions(-)
> 
> diff --git a/src/scan.c b/src/scan.c
> index a3d6e5d8..f99ad1e7 100644
> --- a/src/scan.c
> +++ b/src/scan.c
> @@ -308,6 +308,45 @@ static void scan_build_attr_ie(struct l_genl_msg *msg,
> 	l_genl_msg_append_attrv(msg, NL80211_ATTR_IE, iov, iov_elems);
> }
> 
> +static void scan_build_attr_tx_no_cck_rates(struct l_genl_msg *msg,
> +							struct scan_context *sc)
> +{
> +	static const uint8_t b_rates[] = { 2, 4, 11, 22 };
> +	uint8_t *scan_rates;
> +	const uint8_t *supported;
> +	unsigned int num_supported;
> +	unsigned int count;
> +	unsigned int i;
> +
> +	l_genl_msg_append_attr(msg, NL80211_ATTR_TX_NO_CCK_RATE, 0, NULL);
> +
> +	/*
> +	 * Assume if we're sending the probe requests at OFDM bit
> +	 * rates we don't want to advertise support for 802.11b rates.
> +	 */
> +	if (L_WARN_ON(!(supported = wiphy_get_supported_rates(sc->wiphy,
> +							NL80211_BAND_2GHZ,
> +							&num_supported))))
> +		return;

these kind of “nested” statements become really hard to read and check if they are correct.

	supported = wiphy_get_supported_rates(sc->wiphy, ..);
	if (L_WARN_ON(!supported))
		return;

I realize that this code is the same as below, but lets improve readability as well. It is fine to submit a cleanup patch to improve readability if we find such places.

Regards

Marcel

Re: [PATCH 4/4] scan: Separate cck rates attr creation into logical block

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1051 bytes --]

Hi Marcel,

>> +	if (L_WARN_ON(!(supported = wiphy_get_supported_rates(sc->wiphy,
>> +							NL80211_BAND_2GHZ,
>> +							&num_supported))))
>> +		return;
> 
> these kind of “nested” statements become really hard to read and check if they are correct.
> 
> 	supported = wiphy_get_supported_rates(sc->wiphy, ..);
> 	if (L_WARN_ON(!supported))
> 		return;
> 
> I realize that this code is the same as below, but lets improve readability as well. It is fine to submit a cleanup patch to improve readability if we find such places.

Actually, I explicitly requested that this be the way it is.  For 
L_WARN_ON, I want the resultant warning to carry as much info as 
possible.  Having a message like "Warning: !supported" show up is pretty 
pointless.  Yes it carries the line number and filename, but it still 
requires an extra step.  And things can change between different 
versions of iwd, etc.

So while the readability is a little worse, I think the hit is worth it 
for cases where L_WARN_ON is used.

Regards,
-Denis

Re: [PATCH 1/4] scan: Improve comment

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 190 bytes --]

Hi Tim,

On 11/8/19 4:03 PM, Tim Kourt wrote:
> ---
>   src/scan.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 

Patches 1-3 applied, thanks.

Regards,
-Denis

Re: [PATCH 4/4] scan: Separate cck rates attr creation into logical block

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 410 bytes --]

Hi Tim,

On 11/8/19 4:04 PM, Tim Kourt wrote:
> ---
>   src/scan.c | 82 ++++++++++++++++++++++++++++--------------------------
>   1 file changed, 42 insertions(+), 40 deletions(-)
> 

<snip>

>   
> +static void scan_build_attr_tx_no_cck_rates(struct l_genl_msg *msg,
> +							struct scan_context *sc)

Given that this appends 2 attributes, I think the naming is misleading.

Regards,
-Denis

Re: [PATCH 4/4] scan: Separate cck rates attr creation into logical block

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 1164 bytes --]

Hi Denis,

>>> +	if (L_WARN_ON(!(supported = wiphy_get_supported_rates(sc->wiphy,
>>> +							NL80211_BAND_2GHZ,
>>> +							&num_supported))))
>>> +		return;
>> these kind of “nested” statements become really hard to read and check if they are correct.
>> 	supported = wiphy_get_supported_rates(sc->wiphy, ..);
>> 	if (L_WARN_ON(!supported))
>> 		return;
>> I realize that this code is the same as below, but lets improve readability as well. It is fine to submit a cleanup patch to improve readability if we find such places.
> 
> Actually, I explicitly requested that this be the way it is.  For L_WARN_ON, I want the resultant warning to carry as much info as possible.  Having a message like "Warning: !supported" show up is pretty pointless.  Yes it carries the line number and filename, but it still requires an extra step.  And things can change between different versions of iwd, etc.
> 
> So while the readability is a little worse, I think the hit is worth it for cases where L_WARN_ON is used.

ok then, fair point. Can we somehow improve these cases with a comment to ease reading and understanding the code.

Regards

Marcel

Re: [PATCH 4/4] scan: Separate cck rates attr creation into logical block

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 377 bytes --]

Hi Marcel,

> 
> ok then, fair point. Can we somehow improve these cases with a comment to ease reading and understanding the code.

So something like:

+                *
+                * wiphy should have parsed the supported rates from the
+                * kernel / driver.  If not, warn and give up

Or do you have something else in mind?

Regards,
-Denis

Re: [PATCH 4/4] scan: Separate cck rates attr creation into logical block

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 757 bytes --]



> On Nov 9, 2019, at 04:33, Denis Kenzior <denkenz@gmail.com> wrote:
> 
> Hi Marcel,
> 
>> ok then, fair point. Can we somehow improve these cases with a comment to ease reading and understanding the code.
> 
> So something like:
> 
> +                *
> +                * wiphy should have parsed the supported rates from the
> +                * kernel / driver.  If not, warn and give up
> 
> Or do you have something else in mind?

In addition I would also do

	* 
	* Keep full statement in L_WARN_ON to include more context when
	* the warning is triggered.

Just as a reminder for later days on why this is done. I am pretty sure otherwise I am going to stumble over it again and ask the same question.

Regards

Marcel

Re: [PATCH 4/4] scan: Separate cck rates attr creation into logical block

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1096 bytes --]

Hi Marcel,

On 11/8/19 9:42 PM, Marcel Holtmann wrote:
> 
> 
>> On Nov 9, 2019, at 04:33, Denis Kenzior <denkenz@gmail.com> wrote:
>>
>> Hi Marcel,
>>
>>> ok then, fair point. Can we somehow improve these cases with a comment to ease reading and understanding the code.
>>
>> So something like:
>>
>> +                *
>> +                * wiphy should have parsed the supported rates from the
>> +                * kernel / driver.  If not, warn and give up
>>
>> Or do you have something else in mind?
> 
> In addition I would also do
> 
> 	*
> 	* Keep full statement in L_WARN_ON to include more context when
> 	* the warning is triggered.
> 
> Just as a reminder for later days on why this is done. I am pretty sure otherwise I am going to stumble over it again and ask the same question.
> 

Right.  We do have a dozen or so of these now and I think the use of 
L_WARN will only keep growing.  Perhaps we should add another section to 
doc/coding-style.txt instead?  Formalize / encourage this as an optional 
best practice going forward.

Regards,
-Denis