Re: [Tails-dev] ***SPAM*** Re: Proposed changes to tails-sta…

Borrar esta mensaxe

Responder a esta mensaxe
Autor: Cryptkiddy
Data:  
Para: intrigeri
CC: tails-dev
Asunto: Re: [Tails-dev] ***SPAM*** Re: Proposed changes to tails-starter-i2p && my first contribution to OSS
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?