Provide frequency to wpa_supplicant when in adhoc mode (LP: #2020754)

[yevmel]

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]

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 :)

[yevmel]

agree. I will change the example accordingly 👍

[daniloegea]

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?

[yevmel]

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]

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

[daniloegea]

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]

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:

1. the fix itself
2. the new tests
3. the new example file

Thanks again!

[daniloegea]

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?

[yevmel]

i like your solution 👍

[daniloegea]

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.

[yevmel]

i added a unit test for 5GHz now too.

[daniloegea]

Nice! LGTM!

Thank you for your contribution!

[yevmel]

@daniloegea thank you very much for your support