-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Jérémy Bobbio wrote: > On Fri, Apr 18, 2008 at 10:16:47PM +0800, Glenn wrote: >> New patch attached regarding Christian Perrier's comments. > > First, I recommend that you think about using git to prepare your > patchset before submitting.
no problem
> Second, thanks for your contribution! Your patch should solve
> #327309 [2] which is a feature I have seen requested a few times
> already. It might get some iterations until your proposed changes are
> clean enough to be integrated, though. :)
I expect it to go through a few iterations ;)
>> +Template: netcfg/wireless_security_type
>> +Type: select
>> +__Choices: Wep/Open, WPA PSK
>> +# :sl2:
>> +_Description: Wireless Network Type for ${iface}:
>> + Chose Wep/Open if the network is open or secured with wep.
>> + Chose WPA if the network is a WPA PSK protected network.
>> +
>
> WEP should be written fully capitalized, as its an accronym for
> Wired Equivalent Privacy.
>
> Why are WEP and "open" grouped together? Wouldn't it make more sense
> for the user to have the choice between "Open", "WEP" and "WPA"?
Will address this in a follow up patch. Will leave as wep/open for the
moment though, as the code already checks if wepkey is null which seems
to work fine.
>
>> +Template: netcfg/wireless_wpa
>> +Type: string
>> +# :sl1:
>> +_Description: WPA passphrase for wireless device ${iface}:
>> + Enter a WPA PSK passphrase.
>
> Mhh… PSK means Pre-Shared Key. How about:
> Enter a passphrase for WPA PSK authentication.
>
> But I might be nitpicking here.
>
>> +Template: netcfg/no_wpa_supplicant
>> +Type: error
>> +# :sl2:
>> +_Description: Wpasupplicant not found
>> + The wpa_supplicant binary was not found on the system.
>> + Chose WEP/Open wireless network, otherwise chose wired network.
Will be removed for next patch. will follow up on this later if its
required.
>
>> diff -Nurb ../netcfg-1.43/dhcp.c netcfg-1.43/dhcp.c
>> --- ../netcfg-1.43/dhcp.c 2008-01-18 20:20:02.000000000 +0900
>> +++ netcfg-1.43/dhcp.c 2008-04-18 22:14:33.000000000 +0800
>> @@ -50,6 +50,9 @@
>> + if (requested_wpa_supplicant)
>> + fprintf(fp, "\twpa-conf
>> /etc/wpa_supplicant/wpa_supplicant.conf\n");
>> + else {
>
> There is no other way than using a global variable for
> requested_wpa_supplicant with the current codebase, is it? They are
> pretty distasteful IMHO, but it's probably the less intrusive change
> currently.
If I move the wpasupp functions into dhcp.c we can do away with this. I
didn't think it would be liked when i did it, but I couldn't think of a
better way. ;) Thoughts?
>>
>> +static int kill_wpa_supplicant(void) /* I would much rather kill wpasupp
>> by pid, but my attempts at using kill() proved buggy */
>> +{
>> + system("killwpa.sh");
>> + return 0;
>> +}
>
> Should be doable with kill(2) instead, yes.
Will look into this again.
>> -typedef enum { NOT_ASKED = 30, GO_BACK } response_t;
>> +typedef enum { NOT_ASKED = 30, GO_BACK, WEPG, WPAG } response_t;
>
> Please use meaningful names. WEPG and WPAG really clear enough on their
> meanings.
no problem
>
>> diff -Nurb ../netcfg-1.43/wireless.c netcfg-1.43/wireless.c
>> --- ../netcfg-1.43/wireless.c 2007-04-10 06:27:06.000000000 +0800
>> +++ netcfg-1.43/wireless.c 2008-04-18 22:14:33.000000000 +0800
>> + debconf_get (client, "netcfg/wireless_security_type");
>> + di_info ("client->value = %s", client->value);
>> +
>> + if (client->value[1] == 'e')
>> + return WEPG;
>> + else
>> + return WPAG;
>
> *cough* Please use Choices-C in netcfg/wireless_security_type and do a
> full string comparison on the return value. This kinds of magic tests
> always come back to haunt you later on.
will do
>
>> + di_info ("passphrase == pass %s", passphrase);
>
> Debugging should be removed.
>
I knew I would miss a couple ;)
>
>> diff -Nurb ../netcfg-1.43/wpa.c netcfg-1.43/wpa.c
>> --- ../netcfg-1.43/wpa.c 1970-01-01 08:00:00.000000000 +0800
>> +++ netcfg-1.43/wpa.c 2008-04-18 22:14:33.000000000 +0800
>> +* Functions shamelessly copied from dhcp.c, if you are looking for comments
>> +* look in that file.
>
> Why not do some refactoring then? Or sharing the code altogether?
If there are no objections, I will add the functions to dhcp.c which
will address the global variable above also
>
>> + wpa = (getpid() + 6); /* I *know* this is horrendous, but I am too
>> stupid to work it out */
>
> Mh… What's the problem?
I thought there must be a better way to get the pid, but as yet I havent
found a solution. This was supposed to be removed. It was from when I
was trying kill()
>
> On the overall, you really need to remove *any* extra changes for diff
> you send, otherwise reviewing is quite painful, and I probably
> overlooked some of your changes due to that.
>
> Cheers,
Thanks to all for comments so far. I will address the template comments
in a seperate patch to come. The attached patch just adds the
reconfigure_wifi() function to dhcp.c It improves readability (i think)
and also fixes an unreported bug where netcfg always returns to
ASK_OPTIONS after reconfiguring wifi, instead of polling/starting dhcp.
It's also paves the way for the wpa functions without turning it into an
even more nested satan machine :)
please CC me as I am not subscribed.
Cheers
Glenn
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFICZiqV8GyuTwyskMRAot9AJ9dkNttLTEcX0qBoCqrusJ3Tyo1TwCfcosn
5TJKjfdH3+js4paDC6UstK0=
=dY5S
-----END PGP SIGNATURE-----
diff --git a/packages/netcfg/dhcp.c b/packages/netcfg/dhcp.c
index d58119c..d8dc17d 100644
--- a/packages/netcfg/dhcp.c
+++ b/packages/netcfg/dhcp.c
@@ -263,6 +263,8 @@ int poll_dhcp_client (struct debconfclient *client)
#define REPLY_DONT_CONFIGURE 3
#define REPLY_RECONFIGURE_WIFI 4
#define REPLY_LOOP_BACK 5
+#define REPLY_CHECK_DHCP 6
+#define REPLY_ASK_OPTIONS 7
int ask_dhcp_options (struct debconfclient *client)
{
@@ -302,8 +304,30 @@ int ask_dhcp_options (struct debconfclient *client)
return REPLY_DONT_CONFIGURE;
}
+int reconfigure_wifi (struct debconfclient *client)
+{
+ enum { ABORT, DONE, ESSID, WEP } wifistate = ESSID;
+ for (;;) {
+ switch (wifistate) {
+ case ESSID:
+ wifistate = ( netcfg_wireless_set_essid(client, interface, "high") == GO_BACK ) ?
+ ABORT : WEP;
+ break;
+ case WEP:
+ wifistate = ( netcfg_wireless_set_wep (client, interface) == GO_BACK ) ?
+ ESSID : DONE;
+ break;
+ case ABORT:
+ return REPLY_ASK_OPTIONS;
+ break;
+ case DONE:
+ return REPLY_CHECK_DHCP;
+ break;
+ }
+ }
+}
+
-/* Here comes another Satan machine. */
int netcfg_activate_dhcp (struct debconfclient *client)
{
char* dhostname = NULL;
@@ -472,37 +496,17 @@ int netcfg_activate_dhcp (struct debconfclient *client)
}
break;
case REPLY_RECONFIGURE_WIFI:
- {
- /* oh god - a NESTED satan machine */
- enum { ABORT, DONE, ESSID, WEP } wifistate = ESSID;
- for (;;) {
- switch (wifistate) {
- case ESSID:
- wifistate = ( netcfg_wireless_set_essid(client, interface, "high") == GO_BACK ) ?
- ABORT : WEP;
- break;
- case WEP:
- wifistate = ( netcfg_wireless_set_wep (client, interface) == GO_BACK ) ?
- ESSID : DONE;
- break;
- case ABORT:
- state = ASK_OPTIONS;
- break;
- case DONE:
- if (dhcp_pid > 0)
- state = POLL;
- else {
- kill_dhcp_client();
- state = START;
- }
- break;
- }
- if (wifistate == DONE || wifistate == ABORT)
- break;
- }
- }
+ if (reconfigure_wifi(client) == REPLY_CHECK_DHCP)
+ if (dhcp_pid > 0)
+ state = POLL;
+ else {
+ kill_dhcp_client();
+ state = START;
+ }
+ else
+ state = ASK_OPTIONS;
break;
- }
+ }
break;
case DHCP_HOSTNAME:
reconfigure-wifi.diff.sig
Description: Binary data
