Re: [PATCH 3/3 v6] Add WPA support to netcfg
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Jérémy Bobbio wrote:
> Hi!
>
> Here's a review only based on the code, I have not yet done any tests.
> And you might find me nitpicking too much. :D
No problem, happy to get any comments at all :) I would really like some
testing from people, particularly with chips I don't have though.
>
> What about my suggestion on asking for "Open", "WEP", "WPA" as three
> different possibilities?
>
I'm not adverse to doing this, just that it means I need to alter
another function in wireless.c which at the moment isn't strictly
needed. Personally I would prefer to try and get the WPA functionality
working nicely, and then I am certainly looking at digging into some of
the other wireless functions.
> On Fri, Jun 20, 2008 at 04:04:56PM +0800, Glenn Saberton wrote:
>> @@ -306,20 +311,59 @@ int ask_dhcp_options (struct debconfclient *client)
> […]
>> case ESSID:
>> - wifistate = (netcfg_wireless_set_essid(client, interface, "high") == GO_BACK) ?
>> - ABORT : WEP;
>> + if (wpa_supplicant_status == WPA_UNAVAIL)
>> + wifistate = (netcfg_wireless_set_essid(client, interface, "high") == GO_BACK) ?
>> + ABORT : WEP;
>> + else
>> + wifistate = (netcfg_wireless_set_essid(client, interface, "high") == GO_BACK) ?
>> + ABORT : SECURITY;
>> break;
>> +
>
> It might be more readable to inverse the tests here: first on the result
> of set_essid, then on WPA supplicant avaibility.
no problem
>
>> + case SECURITY:
>> + {
>> + int ret;
>> + ret = netcfg_wireless_set_security(client, interface);
>> + if (ret == GO_BACK)
>> + wifistate = ESSID;
>> + else if (ret == REPLY_WPA)
>> + wifistate = WPA;
>> + else
>> + wifistate = WEP;
>> + break;
>> + }
>
> I don't like the braces indentation here. If you want to keep the
> closing brace on the left of the break, please put the opening one on
> the same line as the case.
>
Sure, I don't really mind either way. Will fix in next round.
> I also think that you should rename SECURITY and _security to
> SECURITY_TYPE and _security_type, as done in the debconf template; this
> would be more meaningful.
>
>> case WEP:
>> - wifistate = (netcfg_wireless_set_wep(client, interface) == GO_BACK) ?
>> + if (wpa_supplicant_status == WPA_UNAVAIL)
>> + wifistate = (netcfg_wireless_set_wep(client, interface) == GO_BACK) ?
>> + ESSID : DONE;
>> + else
>> + wifistate = (netcfg_wireless_set_wep(client, interface) == GO_BACK) ?
>> + SECURITY : DONE;
>> + break;
>
> Same thing as for ESSID.
ok
>
>> diff --git a/packages/netcfg/netcfg-static.c b/packages/netcfg/netcfg-static.c
>> index 285f26f..0af7811 100644
>> --- a/packages/netcfg/netcfg-static.c
>> +++ b/packages/netcfg/netcfg-static.c
>> @@ -32,6 +32,7 @@ int main(int argc, char** argv)
>> + else {
>> + wpa_supplicant_installed();
>> + if (wpa_supplicant_status == WPA_UNAVAIL)
>
> I really don't like this. Either turn wpa_supplicant_installed() into
> wpa_supplicant_is_installed() which would return a boolean or rename
> wpa_supplicant_installed() to init_wpa_supplicant_support() and continue
> to modify a global variable.
I suppose this could be made a boolean. After cueing (sic) ;) I don't do
any further checks with it anyway.
>
>> + case WCONFIG_WPA:
>> + if (wpa_supplicant_status == WPA_OK) {
>> + di_exec_shell_log("apt-install wpasupplicant");
>> + wpa_supplicant_status = WPA_CUED;
>> + }
>
> Shouldn't WPA_CUED be WPA_QUEUED?
Can be either way afaik, I don't really mind which.
http://www.thefreedictionary.com/CUED
>
>> diff --git a/packages/netcfg/netcfg.h b/packages/netcfg/netcfg.h
>> index a7bc8a9..986f270 100644
>> --- a/packages/netcfg/netcfg.h
>> +++ b/packages/netcfg/netcfg.h
>> @@ -10,11 +10,16 @@
>> #define DHCLIENT3_CONF "/etc/dhcp3/dhclient.conf"
>> #define DOMAIN_FILE "/tmp/domain_name"
>> #define NTP_SERVER_FILE "/tmp/dhcp-ntp-servers"
>> +#define WPASUPP_CTRL "/var/run/wpa_supplicant"
>> +#define WPAPID "/var/run/wpa_supplicant.pid"
>
> These constant names are not very consistent…
>
>> +#define WPA_MIN 8
>> +#define WPA_MAX 64
>
> Either rename those or put a comment describing the object that they
> actually bound.
>
They are the max and minimum passphrase length. will comment them.
>> diff --git a/packages/netcfg/wireless.c b/packages/netcfg/wireless.c
>> index ed07b8a..56e64fa 100644
>> --- a/packages/netcfg/wireless.c
>> +++ b/packages/netcfg/wireless.c
> […]
>> + debconf_subst (client, "netcfg/wireless_security_type", "iface", iface);
>> + debconf_input (client, "high", "netcfg/wireless_security_type");
>
> No spaces here.
>
>> + debconf_get (client, "netcfg/wireless_security_type");
>
> Same here.
>
>> + if (!strcmp(client->value, "WEP/Open Network"))
>
> Using Choices-C in your template and comparing on that string feels
> more robust to me.
>
I did try choices-c at one stage and had problems with it. After
searching for what exactly the difference was and not finding much other
than a few bug reports on the list I reverted back. Much appreciated if
you could point me to documentation on the difference.
>> +int netcfg_set_passphrase (struct debconfclient *client, char *iface)
>> +{
> […]
>> + debconf_subst(client, "netcfg/invalid_pass", "passphrase", passphrase);
>> + debconf_input(client, "critical", "netcfg/invalid_pass");
> […]
>> +}
>
> Have you tried to enter a wrong passphrase and then a correct one?
>
> I might not have looked carefully enough, but I don't see the previously
> entered passphrase being cleared out on error.
>
This does work, though the saved value needs to be backspaced in debconf
and reset to the new value. The previous value is shown on screen, I'm
not sure whether giving a blank screen on this is a better idea or not.
At the moment the user can see what was entered and check for mistakes.
>> -#endif
>> +#endif
>> \ No newline at end of file
>
> Please avoid unecessary whitespace changes.
Missed that one. Not sure how it got in there..
>
>> diff --git a/packages/netcfg/wpa.c b/packages/netcfg/wpa.c
>> +/*static void wpa_msg_cb(char *msg, size_t len)
>> +{
>> + di_info("%s\n", msg);
>> +} */
>
> Is this for debugging? Using "#if 0" constructs are highly recommended
> when commenting out large section of C codes: they can nest with both
> other "#if 0" and usual C comments.
>
Yep, debugging for unsolicited replies from the daemon. Not required and
will not be in the next patchset.
>> +static int wpa_add_network(void)
> […]
>
> About this and other functions using wpasupplicant control socket: is
> there any way you could factor the common pattern out? Most of these
> functions look similar.
>
Probably for the set_network commands, but the daemon only takes one
request at a time, so even then we would be doing three calls in one
function and checking for errors after each call anyway. I have modeled
this from wpa_cli, but if anyone has suggestions to reduce the number of
functions, I'm certainly open to it.
> I am always getting nervous when I see string manipulation in C.
> For exemple, your code uses strcpy() instead of strncpy(). The former
> really gives me the creeps, and I strongly suggest that you take the
> time to read an article or two about safe string manipulation in C.
>
> If you have already done so and are confident in your code, please
> apologies: I _really_ get the creeps just by seing string manipulation
> in C.
>
the strcpy functions are pretty much for debugging i have hard coded
network id in now, and once I have the few remaining issues sorted, they
will be removed.
>> +int wpa_supplicant_start(struct debconfclient *client, char *iface, char *ssid, char *passphrase)
> […]
>> + }
>> + stop:
>
> Indentation issue.
>
>> + case SUCCESS:
>> + if ((ctrl) == NULL)
>
> Why the extra parenthesis?
oops :)
>
>> diff --git a/packages/netcfg/wpa_ctrl.c b/packages/netcfg/wpa_ctrl.c
>> + * Modifed version of src/common/wpa_ctrl.c from wpa_supplicant, discarding
>> + * all code paths except for CONFIG_CTRL_IFACE_UNIX. Define strlcpy inline,
>> + * it is not provided by GNU libc.
>> + * Copyright (c) 2008, Kel Modderman <kel@otaku42.de>
>> + *
>> + * wpa_supplicant/hostapd control interface library
>> + * Copyright (c) 2004-2007, Jouni Malinen <j@w1.fi>
>
> Your patch should modify debian/copyright as well, then.
I didn't want to get too far in front of myself by modifying changelog. :)
>
>
> On the overall, using the control socket sounds like a good idea. :)
>
> As you have suggested, I wonder how much we should actually delegate to
> wpasupplicant. If the whole code to support Wi-Fi gets simplier, it
> might be a good idea. As wpasupplicant really adds its bytes to the
> initrd, we might need to build two different versions of netcfg, with or
> without Wi-Fi in that case. But that's just random thoughts.
I was also thinking of possibly splitting it. At least removing all
wireless functions from netcfg-static, as afaik it wouldn't be used there?
I am still working on this, and am reasonably happy with its
functionality, apart from once netcfg has been exited.
Since using the ctrl iface, there is no real need to kill the daemon
when we change configuration, but restarting netcfg seems to be a bit of
a problem so far. I hope to have this fixed in the next day or so
though. Would really appreciate any feedback from people with chipsets
that are available in the 25 kernel. No need to actually install, just
get to mirror choice and download release file and we can be confidant
the wpa bits work.
>
> Cheers,
Thanks
Glenn
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
iEYEARECAAYFAkhfpokACgkQV8GyuTwyskMWuQCgoGR0U6aQifmqogyEkLqWx5Pe
u/QAmwfyQPy+U2XP30qF6m0Q37/zP9kG
=Skcu
-----END PGP SIGNATURE-----
Reply to: