Hi,
Cryptkiddy wrote (10 Jul 2011 14:39:57 GMT) :
> I would like to contribute a fix in "./config/chroot_local-
> includes/usr/local/bin/tails-start-i2p".
> Now it should be possible to get the actual i2pWebInterface port
> instead of always using the default one (7657).
Thanks a lot.
> I was however wondering what to use to output nonfatal error
> messages. I used warn" "; in this case but am not sure whether that
> is alright?
I think "warn" is ok: can be used for debugging, and won't be
displayed to the end-user.
On the other hand, I'm unsure about what is fatal/non-fatal in the
get_router_port sub; e.g. if the i2p config file cannot be found,
doesn't it indicate something is going wrong very much?
> As this is my first contribution to OSS I'd greatly appreciate any
> feedback. Git-diff is attached.
I'm not sure you really want to hear all of my Perl-ish comments. I'll
anyway share them bellow, in case you want to catch the opportunity to
improve your Perl. If you feel it's too much, I would perfectly
understand, and I'll probably fix the patch myself before merging it
into our Git repository.
In any case, please tell us if you intend to submit an enhanced patch
(possibly with several proposal/comments roundtrips), or if I should
add it to my TODO list for the day I feel like fixing and merging it.
Enough meta-blah, now come the actual comments.
1. Please respect the indentation and formatting style of the code
your modifying. This may sound like nitpicking, but almost every
single free software maintainer you'll contribute patches to will
be slighly annoyed, I believe, if s/he has to do painful work such
as fixing indentation in order to merge your work. In our case,
4-spaces, no tabs, opening brace on the same line as the operator
that requires the braces.
2. Since the get_router_port is called several times, I don't like
that much seeing the config file parsed numerous times, to extract
the same information every time. I'd use the Memoize perl module to
get a cache of already-computed results, or
See my other comments inline bellow.
> diff --git a/config/chroot_local-includes/usr/local/bin/tails-start-i2p
> b/config/chroot_local-includes/usr/local/bin/tails-start-i2p
> index 716d05d..3c372a4 100755
> --- a/config/chroot_local-includes/usr/local/bin/tails-start-i2p
> +++ b/config/chroot_local-includes/usr/local/bin/tails-start-i2p
> @@ -32,9 +32,63 @@ textdomain("tails-i2p-notify-user");
> ### helper subs
> -# TODO: get router port (default 7657) from /etc/i2p/clients.config
> +
> sub get_router_port {
> - return 7657;
> +# gets router port (default 7657) from /etc/i2p/clients.config
> +
> + {
This enclosing scope seems weird to me. The results you want can
probably be achieved in an other way.
> + if(! open(CONFFILE, "/etc/i2p/clients.config"))
Bareword filehandles (CONFFILE) bring misery and suffering. Use
indirect filehandles ("my $CONFFILE", or "my $conffile") instead.
Not using the three-arguments form of open also brings misery and
suffering.
> + {
> + warn "Can not open i2p config file.";
> + last; #goto Errorhandling
> + }
The need to add a comment to explain what this "last" does reveals the
brokeness of the control flow structure (braces-based enclosing scope)
I pointed above. The whole sub should be refactored.
> + my $ClientAppNo = -1;
> + my $string;
> + foreach $string (@client_config)
Better call it $line, rather than $string, since it's really what it
is once you've slurped the whole file into an array variable.
> + {
> + if($string =~
> m/^clientApp\.([0-9]+)\.main=net\.i2p\.router\.web\.RouterConsoleRunner$/)
> + #we are trying to get $NUMBER$ from
>>>clientApp.$NUMBER$.main=net.i2p.router.web.RouterConsoleRunner<<
> + {
> + $ClientAppNo = $1;
> + last;
> + }
> + }
> + if($ClientAppNo == -1)
> + {
> + warn "Check wheather i2p config includes"
> + ." 'clientApp.[0-9].main=net.i2p.router.web.RouterConsoleRunner' .";
> + last; #goto Errorhandling
> + }
Wooh, this is very non-idiomatic. The whole thing should be refactored
as follows, rather than using a special value for $ClientAppNo to
control the execution flow:
if (my $ClientAppNo = ($string =~ REGEXP)) {
...
}
else {
...
}
> + my $regex = '^clientApp\.' . $ClientAppNo . '\.args=([0-9]{1,5})\
> [0-9s\:\,\.\-\ ]+\/webapps\/';
You don't need to escape the ":", ",", "." and space chars inside a
bracketed character class. Doing so just adds backslash to the list of
matching chars, which is apparently not what you want to do.
Also, the dash (-) should be either the first or the last char listed
inside the class.
If the line always contains " ./webapps/", you'd better make it
explicit: if this is the case, the current regexp matches too wide.
> + # 'clientApp.$NUMBER$.args=$PORT$ ::1,127.0.0.1 ./webapps/'
> + # $PORT$ ::1 ./webapps/
> + # $PORT$ ::1,127.0.0.1 -s 7667 ::1,127.0.0.1 ./webapps/
> + #here $NUMBER$ is the one we obtained above and $PORT$ is what we want to get
> + #doesn't match if only https port is given.
> + foreach $string (@client_config)
> + {
> + if ($string =~ m/$regex/)
> + {
> + my $routerPort = $1;
> + return $routerPort;
> + }
> + }
Better use something along these lines:
if (my $routerPort = ($string =~ m/$regex/)) {
return $routerPort;
}
Bye,
--
intrigeri <intrigeri@???>
| GnuPG key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc
| OTR fingerprint @ https://gaffer.ptitcanardnoir.org/intrigeri/otr.asc
| If you must label the absolute, use it's proper name: Temporary.
