[Mageia-dev] [soft-commits] [6416] Initial commit of Admin Panel.

[Matteo Pasotti]

-----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; + at ISA = qw(Exporter);
> You'd rather use a modern perl idiom: use base qw(Exporter)
> 
> or
> 
> use parent qw(Exporter);
> 
Got it.
>> + at 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-----