-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 06/11/2012 13:16, Guillaume Rousse wrote: > information in every single file, whereas a single top-level README > file would be enough. > Hello Guillaume,
GPLv2 says: It is safest to attach them to the start of each source file to most effectively convey the exclusion of warranty; and each file should have at least the "copyright" line and a pointer to where the full notice is found. http://www.gnu.org/licenses/gpl-2.0.html >> + +package Auth; > I'm convinced tough than using a shared top-level namespace, for > instance AdminPanel or Mageia::AdminPanel, would be a better idea > to express the idea than this module is a part of a software, than > a loose comment such as "This file is part of mcc2". package > Mageia::AdminPanel::Auth; > It's just a prototype, it will be "fixed" in future. >> + +require Exporter; +@ISA = qw(Exporter); > You'd rather use a modern perl idiom: use base qw(Exporter) > > or > > use parent qw(Exporter); > Got it. >> +@EXPORT = qw(require_root_capability + >> ask_for_authentication); + +use strict; +use warnings; +use >> diagnostics; > Those pragmas should come first, before package variables > Got it >> +use Data::Dumper; > Unused anywere. Don't load debug-related modules in production > coed. > afaik, apanel is not "in production" so what's your concern?. >> +sub require_root_capability { + return 0 if(!$>); + return >> 1; +} > Perl best practice: use english name for magic variables, for > readability: > > use English qw(-no_match_vars ); return 0 if (!$EUID); > > And your condition could be expressed in a single statement: sub > require_root_capability { return $EUID == 0; } > >> + +sub ask_for_authentication { + my @args = @ARGV; + my >> $command = wrap_command($0); + unshift(@args, $command->[2]); >> + exec { $command->[0] } $command->[1], @args or die ("command >> %s missing", $command->[0]); + die "You must be root to run >> this program" if $>; +} > You're duplicating the condition from previous function here. die > "You must be root to run this program" if > !require_root_capability(); > Don't blame me too much for readability and duplications, please. I was inspired by /usr/lib/libDrakX/common.pm and other modules that are not very readable and that contain similar duplications. Again, this module it's still a prototype and I'm learning new stuff while coding it. I'll work on improving its readability. > Morevoer, you'd better test before executing the command: die "You > must be root to run this program" if !require_root_capability(); > exec { $command->[0] } $command->[1], @args or die ("command %s > missing", $command->[0]); > Same as above, take a look at /usr/lib/libDrakX/common.pm and you'll see that the test is performed after the exec. >> +sub wrap_command { + my $currenv = "env"; + my $wrapper = >> "pkexec"; + my $app = $0; + my $command = [$wrapper, >> $currenv, $app]; + ($command); +} > Perl best practice: use explicit return statement, for better > readability: return ($command); > Again, same as above. > Using temporary variables for constant isn't very useful here, the > whole function would probably be more readable this way: > > sub wrap_command { my ($app) = @_; return (['pkexec', 'env', > $app]); } > > I don't understand the need for list contexte here, tough. > > BTW, your indentation isn't consistent between various files. I worked almost exclusively on the file you have dissected =) and honestly it's not a big issue to me to read source files with different indentations (I mean if they are somehow indented and readable). I'll try to improve my code even paying attention to your precious advices. Thank you. Matteo -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iQEcBAEBAgAGBQJQmS25AAoJED3LowjDDWbNj3MH/jazMliy7g+VmvYYm5EA2t3G nqv4o3+FT01cRp45OXq+Xus7TIg/IEFDAtfsIRfWUWH7sH7J1ooWywzourD4kkj0 XXWw7Wykhhpn2cJ5Sv55KwAr5u+ubt5CC2Tga7sis3hvcsFuMKXOoxV6BtUIUQ4O +IuzybR7GhjM/B+oGiPdSLQyRXEB/LhxWsle+Xs0Ode5dYjLZ3QR4YumVC6YiYsS qU/B9SYd3P09K48zqjWNtxDfwf8QItU5oCZ8p9nFnY3vLkW16FGiG2MFJigrOfjq +Ae+6qH/Q/ZH3iqoZDBGXutr6zUIHhyJmU6ICawR2IIbj6lGdKxTUjqu3aN0H+o= =U5sK -----END PGP SIGNATURE-----