Hi Intrigeri,
don't worry about the delay. I think you had all your hands full with GSoC.
Still it does have the disadvantage of me moving to a new town to start
university in a week or so, which means I am cut off from a computer where I
can test the script on a live TAILS system (I do have some other testscripts,
however). So I'm hoping I didn't introduce any stupid but easy to debug bugs
:-(
None the less: Diff as always is attached.
And here are my comments to your comments:
> Had you used Memoize (https://metacpan.org/module/Memoize), which I
> still recommend since it's exactly the right tool for the task at
> hand, you could just:
> return $i2p_default_port;
> ... which expresses much more clearly your intentions I think.
>
You are the boss. I switched it to Memoize.
> It is very non-idiomatic to first read the whole file into a
> multi-line string, then apply a regex on it to extract a sub-string.
>
> A more idiomatic way to do it is:
>
> my $regex =
> '^clientApp\.([0-9]+)\.main=net\.i2p\.router\.web\.RouterConsoleRunner$';
> while ( my $line = <$conffile> ) {
> chomp $line;
> if (($client_app_no) = ($line =~ m/$regex/)) {
> }
The problem I am trying to solve here is that the second Regex actually is(or
can be) appearing one or multiple lines _before_ the first regex in the file,
but I need the regex in the correct order, otherwise I can't compose the
second RegEx.
So I am facing the choice of reading/caching the file completely or reading it
twice from HDD (relying upon another cache grabbing it (Linux kernel, ...).
But that seems undesirable and slower).
+ I could try doing the second RegEx first, but I'd have to store the
potential values in an array and get the right one later. That just seemed
wrong.
+ I could store the whole file in an array of strings instead of a single
string, but I think it will be slower, as the Perl developers probably spent
more time optimizing their code than is being realistic for me. And I have to
put the file in a variable anyway...
+ I could make a completely new function just to return the file content and
use Memoize on it.But that seems like overkill...
Am I missing something here?
> > + $regex = '\nclientApp\.' . $client_app_no . '\.args=([0-9]{1,5})\
> > [0-9s:,. \-]+ \.\/webapps\/[\s]*\n';
>
> Regarding [0-9s:,. \-], I still doubt you really want to include the
> "\" character in the caracter class.
OK. Finally got rid of it ;-) )
> Also, preferably use \s or [[:space:]], or even [[:space:]]+, instead
> of "\ ". It's both clearer to read, and likely to be more robust on
> the long run especially when when parsing files you don't control.
> The "+" I suggest also allows you to remove the space in the character
> class that follows.
changed it to [[:space:]] .
However the space in the following characterclass is still needed, as we might
encounter something like:
"$PORT$ ::1,127.0.0.1 -s 7667 ::1,127.0.0.1 ./webapps/"
^ ^ ^ ^
which has multiple additional spaces inside our character class match
Arguably the [[:space:]] before and after the character class could be dropped
but I thought it adds to clarity.
As thus I did not put a "+" , as the additional spaces will be caught anyway,
and the extra ones are really only for the sake of the reader.
(However, whatever you prefere, shall be done :)
> Regarding "[\s]*", if you mean "possibly followed by space(s)", I
> think "[[:space:]]*" is clearer as it does not require the reader to
> remember if \s is interpreted or not inside a character class.
I have changed it to "[[:space:]]" however according to my tests it can
actually be followed by TABs as well.
(Probably won't. Doesn't seem likely someone mistakenly puts a TAB there)
Cheers,
Cryptkiddy
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..659d122 100755
--- a/config/chroot_local-includes/usr/local/bin/tails-start-i2p
+++ b/config/chroot_local-includes/usr/local/bin/tails-start-i2p
@@ -25,6 +25,8 @@ See
https://tails.boum.org/.
use Desktop::Notify;
use Locale::gettext;
use POSIX;
+use Memoize;
+memoize('get_router_port');
### initialization
setlocale(LC_MESSAGES, "");
@@ -32,9 +34,44 @@ 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 from /etc/i2p/clients.config . If this fails we return I2P default port (7657).
+ my $i2p_default_port = 7657;
+ my $router_port = 0;
+
+
+ my $conffile;
+ if(! open($conffile, "<", "/etc/i2p/clients.config")) {
+ warn "Can not open i2p config file '/etc/i2p/client.config'.".
+ " Using default port ".$i2p_default_port." for I2P webinterface instead.";
+ return $i2p_default_port;
+ }
+ my $client_config = join("", <$conffile>);
+ close($conffile);
+
+ my $client_app_no;
+ my $regex = '\nclientApp\.([0-9]+)\.main=net\.i2p\.router\.web\.RouterConsoleRunner\n';
+ #we are trying to get $NUMBER$ from >>clientApp.$NUMBER$.main=net.i2p.router.web.RouterConsoleRunner<<
+ if(! (($client_app_no) = ($client_config =~ m/$regex/))) {
+ warn "RegEx checking 'clientApp.[0-9].main=net.i2p.router.web.RouterConsoleRunner' didn't match.".
+ "Can not find port in '/etc/i2p/client.config'." .
+ " Using default port ".$i2p_default_port." for I2P webinterface instead.";
+ return $i2p_default_port;
+ }
+
+ $regex = '\nclientApp\.' . $client_app_no . '\.args=([0-9]{1,5})[[:space:]]([0-9s:,.-]|[[:space:]])+[[:space:]]\.\/webapps\/[[:space:]]*\n';
+ # '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.
+ if (! (($router_port) = ($client_config =~ m/$regex/))) {
+ warn "RegEx checking 'clientApp.".$client_app_no.".args= [...]' didn't match.".
+ "Can not find port in '/etc/i2p/client.config'." .
+ " Using default port ".$i2p_default_port." for I2P webinterface instead.";
+ return $i2p_default_port;
+ }
+ return $router_port;
}
# TODO: more perlish way to do below?
