From owner-freebsd-questions  Mon Jul  5 14:23:58 1999
Delivered-To: freebsd-questions@freebsd.org
Received: from plab.ku.dk (plab.ku.dk [130.225.105.65])
	by hub.freebsd.org (Postfix) with ESMTP id 32EDD14D15
	for <freebsd-questions@FreeBSD.ORG>; Mon,  5 Jul 1999 14:23:53 -0700 (PDT)
	(envelope-from tobez@plab.ku.dk)
Received: from lion.plab.ku.dk (lion.plab.ku.dk [130.225.105.49])
	by plab.ku.dk (8.9.1/8.9.1) with ESMTP id XAA21584;
	Mon, 5 Jul 1999 23:23:52 +0200 (CEST)
Received: (from tobez@localhost)
	by lion.plab.ku.dk (8.9.3/8.9.3) id XAA09540;
	Mon, 5 Jul 1999 23:21:39 +0200 (CEST)
	(envelope-from tobez)
Date: Mon, 5 Jul 1999 23:21:39 +0200
From: Anton Berezin <tobez@plab.ku.dk>
To: Dan Langille <junkmale@xtra.co.nz>
Cc: Burke Gallagher <burke@mcs.net>, freebsd-questions@FreeBSD.ORG
Subject: Re: running frequent cron perl scripts
Message-ID: <19990705232139.D8704@lion.plab.ku.dk>
References: <19990705111124.ZBJY282564.mta1-rme@wocker>; <19990705170738.D5735@lion.plab.ku.dk> <19990705203750.BEYX282564.mta1-rme@wocker>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
X-Mailer: Mutt 0.95.4i
In-Reply-To: <19990705203750.BEYX282564.mta1-rme@wocker>; from Dan Langille on Tue, Jul 06, 1999 at 08:34:27AM +1200
Sender: owner-freebsd-questions@FreeBSD.ORG
Precedence: bulk
X-Loop: FreeBSD.ORG

On Tue, Jul 06, 1999 at 08:34:27AM +1200, Dan Langille wrote:
> On 5 Jul 99, at 17:07, Anton Berezin wrote:

>> judging from your use of CGI::Carp in non-cgi environment, you are
>> not very good in Perl, so the possibility that script does something
>> in, uhmm, not the most efficient fashion is far from remote.  :-)
>> Show us more...

> Really?  Can ya tell?  Is it that obvious?  <grin>

Yep, sorry 'bout that. :-)

> Yes, this is my first script.  And it's not even mine.  I obtained it from 
> someone else who wrote it for their ADSL connection.  All I've done is 
> modify it slightly.

That explains something.  Yes, obviously, the original script was used
in the Web setting - it output its results as an HTML document.  You
just don't need CGI and CGI::Carp there.

Although the use of regexes is a bit weird, the program has no loops, so
I can't think of anything which may cuase the slowness you described,
except for the initial loading of the modules and for the internal
working of LWP stuff.  Frankly, Lincoln Stein did a decent job trying to
minimize the loadtime of CGI.pm, but anyway you should immediately
observe certain speedup as soon as you remove CGI and CGI::Carp use.

> #!/usr/bin/perl
> ## Program Name: fetch2.cgi
> ##
> ## Nokia M10 address fetcher
> ##
> ##I hope that this program is useful to you. I make no claim as to this
> ##software's stability or value. It is offered as is. Use at your own risk.
> 
> system("logger -i -t DYNDNS script start");

Umm, is that 486?  Processes are cheap in FreeBSD, but can't you just
tune your cron job description to send Perl's stdout to syslog directly?
This will eliminate one extra process launch (cron will launch a logger
once, anyway).  So let's make it

  print "DYNDNS script start\n";

> $| = 1;

This should not harm - you don't have lots of output anyway.

> use CGI;
> use CGI::Carp (fatalsToBrowser);

Get rid of those.

> require LWP;

Yep, your modem has a password protection.  It's easier to leave this
part as it is now.  Of course, as another poster said, it would probably
be nice in your particular setting to program the fetch with low-level
socket calls, but this becomes a bit of an overhead for *you*, the
programmer, huh?

> ## Set the username and password for the M10
> $user = "MadeHardBy";
> $pass = "Telecom";
> 
> ## Set the URL of the page containing the current IP for the M10
> $URL = "http://192.168.1.254/shell/show+ip+interfaces";
> 
> ## Set the left and right hand side of the Regular Expression that will 
> find
> ## the IP within the page
> $lhs = "inet ";
> $rhs = " netmask 0 peer";

It's probably a matter of style, but most of the Perl programmers I know
just put such literal values (they don't change much, do they?) inside
the actual regex.

> ##  set the following to the name of a program you want run if its a new IP
> $RunIfNew = "/home/dan/dns_update.sh";

Fine, unless you wish to do it in Perl, directly.  It should not be
*that* difficult.

> $IPFilename = "myip.txt";
> $ua = LWP::UserAgent->new;
> $req = HTTP::Request->new(GET,$URL,$h);
> $req->authorization_basic($user,$pass);
> $page = $ua->request($req);
> $page->content() =~ /$lhs(\d*?\.\d*?\.\d*?\.\d*?)$rhs/;

See above.  Personally, I would write it as

   $page->content() =~ /inet (\d*?\.\d*?\.\d*?\.\d*?) netmask 0 peer/;

Also, you dont check for a success of the match.  What if there is no
match?  Also, obviously, either the modem returns the IP as the very
first line, or the HTML it returns does not have "\n" embedded,
otherwise the match will not work.  Add the "s" modifier at the end of
the // operator to avoid this problem.

> $ip = $+;

Strange.  It's valid, but I am pretty sure almost everybody would write

	$ip = $1;

instead.

> ##&page_header;
> &get_OldIP;

Fine.  Almost.  Well, fine.  You can as well just call get_OldIP as you
would in C:

   get_OldIP();

> if ($ip !~ ?^$OldIP$?) {

This beats me.  Why someone needs to match a constant string against
another constant string *backwards*?  Also, the use of ?? is deprecated.
Much better:

  if ($ip ne $OldIP) {

> ##    print "<B>NEW ";
>     &write_OldIP;
> }
> ##print "$ip";
> ##&page_footer;
> if ($ip !~ ?^$OldIP$?) {
>     exec $RunIfNew;
> ##    print "NOTE: IP has changed.\n";
>         system("logger -i -t DYNDNS The IP Address has changed to $ip");
> }

You have two consecutive identical checks.  Why not make just one?
Second, your "system" call will never be reached - that's an "exec" it
does not return unless there was an error.  Make it "system" or
rearrange things or make a call to fork(), just as in C.

     write_OldIP();
     system $RunIfNew;
     print "The IP Address has changed to $ip\n";
  }
   
> 
> system("logger -i -t DYNDNS script stop");

The same comment as at the beginning.

> ## End of program.
> 
> sub get_OldIP{
>     if (open(FILE,"<$IPFilename")) {
>         $OldIP = <FILE>;
>         close FILE;
>     } else {
>         &write_OldIP;
>     }
> }
> 
> sub write_OldIP{
>     if (open(FILE,">$IPFilename")) {
>         print FILE $ip;
>         close FILE;
>     } else {
>         print "<STRONG>Error:</STRONG> couldn't write to file $IPFilename: 
> $!\n";

Would you originally liked this to be mailed to you by cron?  :-)

>     }
> 
> 
> }
> 
> sub page_header{
>   print<<HTML;
>    <HTML><HEAD><TITLE>Nokia M10 IP Address Finder</TITLE></HEAD>
>     <BODY BGCOLOR="#FFFFFF">
>     <CENTER><FONT SIZE=5 FACE=ARIAL>
> HTML
> }
> 
> sub page_footer{
>   print<<HTML;
>   </FONT>
>   </CENTER><P>
>   </BODY></HTML>
> HTML
> }

These two just don't belong here.

Please note, that all those corrections, except maybe the removal of CGI
and CGI::Carp, will  *not* considerably speed things up.  I am almost
sure that most of the time script spends at $ua->request() guts.

But the script is awful.  :-)))
-- 
Anton Berezin <tobez@plab.ku.dk>
The Protein Laboratory, University of Copenhagen


To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-questions" in the body of the message