Provide frequency to wpa_supplicant when in adhoc mode (LP: #2020754) by yevmel · Pull Request #363 · canonical/netplan

yevmel · 2023-05-25T22:28:40Z

Description

generated configuration for wpa_supplicant is missing "frequency", when in adhoc mode.

here is the corresponding bug report:
https://bugs.launchpad.net/netplan/+bug/2020754

daniloegea

Thanks so much for your bug report and pull request!

I left a couple of comments inline. The main point is that we probably shouldn't emit the freq_list configuration for ad-hoc interfaces, only the frequency.

Apart from that, it would be great to have some unittests for the new code. They can be implemented in tests/generator/test_wifis.py. You can look for one that checks if the generated wpa_supplicant configuration is correct, copy/paste and make some changes. There are some instruction in our README on how to run the tests (you need to use -Db_coverage=true with meson setup so the code coverage will also be checked)

daniloegea · 2023-05-29T13:34:35Z

examples/adhoc.yaml

@@ -0,0 +1,10 @@
+network:


Perhaps this file could be called wireless_adhoc.yaml? It would be easier to find it if someone is looking for an example of ad-hoc wifi configuration in the examples/ folder.

Also, having an IP address and a default route would be good, people would be able to just copy, paste, make minimal changes to it and have the interface working :)

agree. I will change the example accordingly 👍

daniloegea · 2023-05-29T13:42:28Z

src/networkd.c

                case NETPLAN_WIFI_MODE_ADHOC:
                    g_string_append(s, "  mode=1\n");
+
+                    if (ap->channel) {


This looks fine.

Does it make sense to still have the property freq_list when operating in ad-hoc mode? The documentation says freq_list are frequencies in MHz to allow for selecting the BSS. As an interface in ad hoc mode will not connect to BSSes, this property will probably be ignore (but I'm not sure). If that is the case, maybe we should not emit freq_list for an ad hoc interface. What do you think?

aggree, i should not emit freq_list in adhoc mode. 👍
it was ok for me as a quick fix, because having freq_list does not break thngs in adhoc mode. i will take care of it in the next days.

yevmel · 2023-05-30T07:48:54Z

I Will add the missing unit test in the next days, thank you.

daniloegea · 2023-05-30T08:22:03Z

Thanks, @yevmel
note: the Autopkgtest CI failure is fixed in the main branch, you just need to rebase your work and the test will run again.

yevmel · 2023-05-31T21:56:39Z

Hi @daniloegea , thank you for the feedback.

i added a unit test, which checks the generated wpa_supplicant.conf.

some tests fail for me (before and after my changes). And unfortunately i did not get coverage to work on my local machine, bu i think with the new unit test the coverage should not go down.

daniloegea

Thank you for addressing my comments.

Now that I looked at the whole patch, I believe the fix could be a lot simpler. I left a suggestion in my new inline comment.

The test coverage will probably get back to 100% with another test for band: 5GHz.

After addressing that, I'd like to ask you to rebase and squash some commits so we can keep a clean history.
I believe you just need 3 commits:

the fix itself
the new tests
the new example file

Thanks again!

daniloegea · 2023-06-01T11:04:12Z

src/networkd.c

-                    g_hash_table_foreach(wifi_frequency_5, wifi_append_freq, s);
-                    // overwrite last whitespace with newline
-                    s = g_string_overwrite(s, s->len-1, "\n");
+            if (ap->mode != NETPLAN_WIFI_MODE_ADHOC) {


That works. But now after looking at the whole solution, I think it could be simpler.

Instead of adding a new section to handle the ad-hoc case, you could do something like this:

diff --git a/src/networkd.c b/src/networkd.c index e3ac5c0..05cb151 100644 --- a/src/networkd.c +++ b/src/networkd.c @@ -1164,6 +1164,8 @@ write_wpa_conf(const NetplanNetDefinition* def, const char* rootdir, GError** er NetplanWifiAccessPoint* ap; g_hash_table_iter_init(&iter, def->access_points); while (g_hash_table_iter_next(&iter, NULL, (gpointer) &ap)) { + gchar* freq_config_str = ap->mode == NETPLAN_WIFI_MODE_ADHOC ? "frequency" : "freq_list"; + g_string_append_printf(s, "network={\n ssid=\"%s\"\n", ap->ssid); if (ap->bssid) { g_string_append_printf(s, " bssid=%s\n", ap->bssid); @@ -1176,8 +1178,8 @@ write_wpa_conf(const NetplanNetDefinition* def, const char* rootdir, GError** er if(!wifi_frequency_24) wifi_get_freq24(1); if (ap->channel) { - g_string_append_printf(s, " freq_list=%d\n", wifi_get_freq24(ap->channel)); - } else { + g_string_append_printf(s, " %s=%d\n", freq_config_str, wifi_get_freq24(ap->channel)); + } else if (ap->mode != NETPLAN_WIFI_MODE_ADHOC) { g_string_append_printf(s, " freq_list="); g_hash_table_foreach(wifi_frequency_24, wifi_append_freq, s); // overwrite last whitespace with newline @@ -1188,8 +1190,8 @@ write_wpa_conf(const NetplanNetDefinition* def, const char* rootdir, GError** er if(!wifi_frequency_5) wifi_get_freq5(7); if (ap->channel) { - g_string_append_printf(s, " freq_list=%d\n", wifi_get_freq5(ap->channel)); - } else { + g_string_append_printf(s, " %s=%d\n", freq_config_str, wifi_get_freq5(ap->channel)); + } else if (ap->mode != NETPLAN_WIFI_MODE_ADHOC) { g_string_append_printf(s, " freq_list="); g_hash_table_foreach(wifi_frequency_5, wifi_append_freq, s); // overwrite last whitespace with newline

What do you think?

i like your solution 👍

daniloegea · 2023-06-01T11:08:01Z

tests/generator/test_wifis.py

 mode=adhoc
 '''})

+    def test_wifi_adhoc_wpa(self):


To get full coverage you probably just need to implement the same test for band: 5GHz. Then you could call this test test_wifi_adhoc_wpa_24ghz and the other one ...5ghz.

i added a unit test for 5GHz now too.

daniloegea

Nice! LGTM!

Thank you for your contribution!

yevmel · 2023-06-02T15:34:41Z

@daniloegea thank you very much for your support

daniloegea changed the title ~~provide frequency to wpa_supplicant when in adhoc mode.~~ Provide frequency to wpa_supplicant when in adhoc mode (LP: #2020754) May 29, 2023

daniloegea suggested changes May 29, 2023

View reviewed changes

daniloegea suggested changes Jun 1, 2023

View reviewed changes

yevmel added 3 commits June 1, 2023 22:09

provide frequency to wpa_supplicant when in adhoc mode.

103a42b

added unit test checking generated wpa_supplicant.conf in adhoc mode.

c6fe1a7

added examples for wireless adhoc mode.

b4acb89

yevmel force-pushed the main branch from 34c879c to b4acb89 Compare June 1, 2023 22:12

daniloegea approved these changes Jun 2, 2023

View reviewed changes

daniloegea merged commit 3d15c59 into canonical:main Jun 2, 2023

slyon added documentation Documentation improvements. community This PR has been proposed by somebody outside of the Netplan team and roadmap commitments. labels Oct 12, 2023

Conversation

yevmel commented May 25, 2023

Description

Uh oh!

daniloegea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yevmel commented May 30, 2023

Uh oh!

daniloegea commented May 30, 2023

Uh oh!

yevmel commented May 31, 2023

Uh oh!

daniloegea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniloegea left a comment

Choose a reason for hiding this comment

Uh oh!

yevmel commented Jun 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants