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
What about my suggestion on asking for "Open", "WEP", "WPA" as three
different possibilities?
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.
> + 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.
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.
> 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.
> + 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?
> 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.
> 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.
> +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.
> -#endif
> +#endif
> \ No newline at end of file
Please avoid unecessary whitespace changes.
> 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.
> +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.
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.
> +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?
> 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.
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.
Cheers,
--
Jérémy Bobbio .''`.
lunar@debian.org : :Ⓐ : # apt-get install anarchism
`. `'`
`-
Attachment:
signature.asc
Description: Digital signature