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. It should make your life a lot easier, and
ours too. ;) Don't hesitate to split your changes in more than one
commit, and you can use "git log -p --reverse master..HEAD" for a nice
output. See the wiki page [1] on how to optain a synchronized
repository.
[1] http://wiki.debian.org/DebianInstaller/git-svn
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. :)
netcfg definitely needs some love [3] and help here is truly
appreciated.
[2] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=327309
[3] http://bugs.debian.org/cgi-bin/pkgreport.cgi?pkg=netcfg
Third, a (not-so) quick review, no tests done:
> diff -Nurb ../netcfg-1.43/debian/control netcfg-1.43/debian/control
> --- ../netcfg-1.43/debian/control 2008-04-08 19:40:33.000000000 +0800
> +++ netcfg-1.43/debian/control 2008-04-18 22:14:33.000000000 +0800
Please base your patch on the version actually in the SVN repository (or
its mirror): this would prevent such unrelated change to appear in your
diff.
> +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"?
> +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.
Should the future wpasupplicant-udeb be added as a full dependency in
netcfg if we add support for WPA encrypted networks? In that case, this
template should probably be removed. But we might have to deal with
memory/space issues…
> -_Description: Storing network settings...
> +_Description: Storing network settings ...
Reversion from current HEAD.
> 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.
> @@ -184,6 +188,7 @@
> return 1;
> else {
> /* dhcp_pid contains the child's PID */
> +
> signal(SIGCHLD, &dhcp_client_sigchld);
> return 0;
> }
Noise.
> @@ -196,6 +201,11 @@
> return 0;
> }
>
> +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.
> +int reconfigure_wifi(struct debconfclient *client)
As you are refactoring code here, it would probably be better if you
could provide two different patches here: one doing the refactoring, not
adding any new features, the other with the changes required for WPA.
The mixin makes the reviewer harder.
> diff -Nurb ../netcfg-1.43/netcfg.c netcfg-1.43/netcfg.c
> --- ../netcfg-1.43/netcfg.c 2006-09-23 21:24:37.000000000 +0800
> +++ netcfg-1.43/netcfg.c 2008-04-18 22:14:33.000000000 +0800
As far as I can understand, you have two unrelated changes poping it up
here as well:
1. You are adding another step to the state machine (WCONFIG_SECURITY).
2. You are adding supported for WPA.
It might worth two make two different commits here, particularily if you
split the "Open" and "WEP" cases (which might not be a good idea,
actually).
> -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.
> diff -Nurb ../netcfg-1.43/netcfg-static.c netcfg-1.43/netcfg-static.c
> --- ../netcfg-1.43/netcfg-static.c 2006-09-23 21:24:37.000000000 +0800
> +++ netcfg-1.43/netcfg-static.c 2008-04-18 22:14:33.000000000 +0800
Same issue as in netcfg.c.
> 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.
> + di_info ("passphrase == pass %s", passphrase);
Debugging should be removed.
> +int netcfg_wireless_set_security (struct debconfclient *client, char *iface)
> +{
> + (void) client;
> + (void) iface;
> + return 0;
> +}
> +int netcfg_set_passphrase (struct debconfclient *client, char *iface)
> +{
> + (void) client;
> + (void) iface;
> + return 0;
> +}
What are these for? If there is no other solutions than introducing
dead code (which I doubt) please at least leave a comment.
> 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?
> + wpa = (getpid() + 6); /* I *know* this is horrendous, but I am too stupid to work it out */
Mh… What's the problem?
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,
--
Jérémy Bobbio .''`.
lunar@debian.org : :Ⓐ : # apt-get install anarchism
`. `'`
`-
Attachment:
signature.asc
Description: Digital signature