Over the years, I have had many problems with NTPd stopping to work
without any notice. That always upset me. I finally wrote a perl
script that checks times among various servers and computes a time
offset for each of them. It can either report that, or it can also set
exit code based on whether all these offsets are below a user
specified limit.

To run it, say query-ntp host1 host2 etc

A good practice is to specify localhost as host1, pool.ntp.org as
host2, and whatever other machines you are checking, would follow
after that.

i
################################################## ####################
#!/usr/bin/perl

# See Also:
# http://www.webreference.com/programming/perl/ntp/
# http://www.webreference.com/programming/perl/ntp/2.html

#
# Copyright(C) Igor Chudov, 2008.
# Released under GNU Public License version 3.
#

use strict; use warnings;

use Data:umper;
use Getopt::Long;
use Net::NTP;
use Time::HiRes qw( time );

my $detail = undef;
my $limit = undef;

GetOptions(
"detail!" => \$detail,
"limit=f" => \$limit,
);

my @servers = @ARGV;

my $bad = undef;

foreach my $server (@servers) {
my $t0 = time;
my %h = get_ntp_response( $server );


#Timestamp Name ID When Generated
#------------------------------------------------------------
#Originate Timestamp T1 time request sent by client
#Receive Timestamp T2 time request received by server
#Transmit Timestamp T3 time reply sent by server
#Destination Timestamp T4 time reply received by client
#
#The roundtrip delay d and local clock offset t are defined as
#
#d = (T4 - T1) - (T2 - T3) t = ((T2 - T1) + (T3 - T4)) / 2

my $T1 = $t0; # $h{'Originate Timestamp'};
my $T2 = $h{'Receive Timestamp'};
my $T3 = $h{'Transmit Timestamp'};

my $T4 = time; # From Time::HiRes! Accurate to usec!

#print "T4=$T4\n";
my $d = ($T4 - $T1) - ($T2 - $T3);
my $t = (($T2 - $T1) + ($T3 - $T4)) / 2;
my $duration = $T4-$t0;
my $delta = $T2-$T1 - $duration/2;

if( $limit ) {
if( $delta > $limit || -$delta < -$limit) {
print "Server $server OFF by $delta seconds!!!\n";
$bad = 1;
} else {
print "Server $server OK, delta is $delta seconds!!!\n";
}

} else {
if( $detail ) {
print Dumper( \%h ) . "\nt0=$t0, T4=$T4\ndelay=$d, offset=$t\nelapsed=$duration, delta=$delta\n\n";
} else {
#print "$server delay=$d, offset=$t, delta=$delta, duration=$duration\n";
print sprintf( "%-30s %.5f\n", $server, $t );
}
}
}

exit 1 if $bad;

Ignoramus5390 <ignoramus5390@NOSPAM.5390.invalid> wrote:


> use strict; use warnings;

You might want each on a line of its own.

>
> use Data:umper;
> use Getopt::Long;
> use Net::NTP;
> use Time::HiRes qw( time );
>
> my $detail = undef;
> my $limit = undef;

undef is the default value, I wouldn't assign it explicitly.

> GetOptions(
> "detail!" => \$detail,
> "limit=f" => \$limit,
> );
>
> my @servers = @ARGV;

you might want to do some checking here, and report a usage message.
Also, I recommend to support at least -h / --.

> foreach my $server (@servers) {
> my $t0 = time;
> my %h = get_ntp_response( $server );
>
>
> #Timestamp Name ID When Generated
> #------------------------------------------------------------
> #Originate Timestamp T1 time request sent by client
> #Receive Timestamp T2 time request received by server
> #Transmit Timestamp T3 time reply sent by server
> #Destination Timestamp T4 time reply received by client

If you need to explain what IDs mean it clearly shows that the names of
your variables are not chosen well.


> #
> #The roundtrip delay d and local clock offset t are defined as
> #
> #d = (T4 - T1) - (T2 - T3) t = ((T2 - T1) + (T3 - T4)) / 2
>
> my $T1 = $t0; # $h{'Originate Timestamp'};

^^^^ remove commented out cruft


> my $T2 = $h{'Receive Timestamp'};
> my $T3 = $h{'Transmit Timestamp'};
>
> my $T4 = time; # From Time::HiRes! Accurate to usec!
>
> #print "T4=$T4\n";

don't leave cruft in like that. If you need debugging, use a --debug
option, and use something like:

$debug and print ....

> my $d = ($T4 - $T1) - ($T2 - $T3);

my $roundtrip_delay =

> my $t = (($T2 - $T1) + ($T3 - $T4)) / 2;

my $local_clock_offset = ...

> my $duration = $T4-$t0;
> my $delta = $T2-$T1 - $duration/2;
>
> if( $limit ) {
> if( $delta > $limit || -$delta < -$limit) {
> print "Server $server OFF by $delta seconds!!!\n";
> $bad = 1;
>
> } else {
> print "Server $server OK, delta is $delta seconds!!!\n";
> }
>
> } else {

Note that the else here makes that $detail and $limit are mutual
exclusive. Sounds odd to me. But if that's what you want, instead of
setting $bad to 1, and do a test at the end, you could just have used:

die "Server .... OFF ... \n";

note that die also sets conveniently the exit value...


> if( $detail ) {
> print Dumper( \%h ) . "\nt0=$t0, T4=$T4\ndelay=$d,
> offset=$t\nelapsed=$duration, delta=$delta\n\n";
> } else {
> #print "$server delay=$d, offset=$t, delta=$delta,
> duration=$duration\n"; print sprintf( "%-30s %.5f\n", $server,
> $t );
> }
> }
> }
>
> exit 1 if $bad;


--
John Bokma http://johnbokma.com/

On 2008-01-14, John Bokma <john@castleamber.com> wrote:
> Ignoramus5390 <ignoramus5390@NOSPAM.5390.invalid> wrote:
>
>
>> use strict; use warnings;
>
> You might want each on a line of its own.

ok

>>
>> use Data:umper;
>> use Getopt::Long;
>> use Net::NTP;
>> use Time::HiRes qw( time );
>>
>> my $detail = undef;
>> my $limit = undef;
>
> undef is the default value, I wouldn't assign it explicitly.

I like this style. Makes it very explicit.

>> GetOptions(
>> "detail!" => \$detail,
>> "limit=f" => \$limit,
>> );
>>
>> my @servers = @ARGV;
>
> you might want to do some checking here, and report a usage message.

done

> Also, I recommend to support at least -h / --.

Is there some standard way of doing it?

>> foreach my $server (@servers) {
>> my $t0 = time;
>> my %h = get_ntp_response( $server );
>>
>>
>> #Timestamp Name ID When Generated
>> #------------------------------------------------------------
>> #Originate Timestamp T1 time request sent by client
>> #Receive Timestamp T2 time request received by server
>> #Transmit Timestamp T3 time reply sent by server
>> #Destination Timestamp T4 time reply received by client
>
> If you need to explain what IDs mean it clearly shows that the names of
> your variables are not chosen well.

They come from NTP specification.

>
>> #
>> #The roundtrip delay d and local clock offset t are defined as
>> #
>> #d = (T4 - T1) - (T2 - T3) t = ((T2 - T1) + (T3 - T4)) / 2
>>
>> my $T1 = $t0; # $h{'Originate Timestamp'};
>
> ^^^^ remove commented out cruft
>
>
>> my $T2 = $h{'Receive Timestamp'};
>> my $T3 = $h{'Transmit Timestamp'};
>>
>> my $T4 = time; # From Time::HiRes! Accurate to usec!
>>
>> #print "T4=$T4\n";
>
> don't leave cruft in like that. If you need debugging, use a --debug
> option, and use something like:
>
> $debug and print ....

ok

>> my $d = ($T4 - $T1) - ($T2 - $T3);
>
> my $roundtrip_delay =
>
>> my $t = (($T2 - $T1) + ($T3 - $T4)) / 2;
>
> my $local_clock_offset = ...

changed

>> my $duration = $T4-$t0;
>> my $delta = $T2-$T1 - $duration/2;
>>
>> if( $limit ) {
>> if( $delta > $limit || -$delta < -$limit) {
>> print "Server $server OFF by $delta seconds!!!\n";
>> $bad = 1;
>>
>> } else {
>> print "Server $server OK, delta is $delta seconds!!!\n";
>> }
>>
>> } else {
>
> Note that the else here makes that $detail and $limit are mutual
> exclusive. Sounds odd to me. But if that's what you want, instead of
> setting $bad to 1, and do a test at the end, you could just have used:
>
> die "Server .... OFF ... \n";

I want all bad servers reported, hence delayed dying.

> note that die also sets conveniently the exit value...

Yes, but I want to be a little clear since I want to go to the end.

################################################## ####################

#!/usr/bin/perl

# See Also:
# http://www.webreference.com/programming/perl/ntp/
# http://www.webreference.com/programming/perl/ntp/2.html

#
# Copyright(C) Igor Chudov, 2008.
# Released under GNU Public License version 3.
#

use strict;
use warnings;

use Data:umper;
use Getopt::Long;
use Net::NTP;
use Time::HiRes qw( time );

my $detail = undef;
my $limit = undef;

GetOptions(
"detail!" => \$detail,
"limit=f" => \$limit,
);

die "detail and limit are mutually exclusive" if $detail and $limit;

my @servers = @ARGV;

my $bad = undef;

foreach my $server (@servers) {
my $t0 = time;
my %h = get_ntp_response( $server );


#Timestamp Name ID When Generated
#------------------------------------------------------------
#Originate Timestamp T1 time request sent by client
#Receive Timestamp T2 time request received by server
#Transmit Timestamp T3 time reply sent by server
#Destination Timestamp T4 time reply received by client
#
#The roundtrip delay d and local clock offset t are defined as
#
#d = (T4 - T1) - (T2 - T3) t = ((T2 - T1) + (T3 - T4)) / 2

my $T1 = $t0;
my $T2 = $h{'Receive Timestamp'};
my $T3 = $h{'Transmit Timestamp'};

my $T4 = time; # From Time::HiRes! Accurate to usec!

my $roundtrip_delay = ($T4 - $T1) - ($T2 - $T3);
my $local_clock_offset = (($T2 - $T1) + ($T3 - $T4)) / 2;
my $duration = $T4-$t0;
my $delta = $T2-$T1 - $duration/2;

if( $limit ) {
if( $delta > $limit || -$delta < -$limit) {
print "Server $server OFF by $delta seconds!!!\n";
$bad = 1;
} else {
print "Server $server OK, delta is $delta seconds!!!\n";
}

} else {
if( $detail ) {
print Dumper( \%h ) . "\nt0=$t0, T4=$T4\ndelay=$roundtrip_delay, offset=$local_clock_offset\nelapsed=$duration, delta=$delta\n\n";
} else {
#print "$server delay=$roundtrip_delay, offset=$local_clock_offset, delta=$delta, duration=$duration\n";
print sprintf( "%-30s %.5f\n", $server, $local_clock_offset );
}
}
}

exit 1 if $bad;

Ignoramus5390 <ignoramus5390@NOSPAM.5390.invalid> wrote:

> On 2008-01-14, John Bokma <john@castleamber.com> wrote:

[..]

>> undef is the default value, I wouldn't assign it explicitly.
>
> I like this style. Makes it very explicit.

IMO Perl programmers should know that the default is undef, and hence no
need to assign undef. To confuse matters a bit more (IMO), you set $bad
to undef, yet you assign later on 1 to it (in case of an error), and at
the end of the script you test if $bad .... I try to avoid testing for
undef that way, and I try to avoid to use 1/undef as a true/false. In
short, I would've used something like:

my $server_limit_out_of_range = 0;

:
:
if ....

$server_limit_out_of_range = 1;
:
:
$server_limit_out_of_range and exit 1;

ditto for $detail (which is a flag).

As for limit, I probably would pick a sensible default instead of undef.


>> Also, I recommend to support at least -h / --.
>
> Is there some standard way of doing it?

Perl Best Practices (recommended) recommends:

--usage - one line
-- - --usage line followed by one-line
summary of each option (in my experience
the one-line is to strict)
--version
--man - complete documentation of the program

>> If you need to explain what IDs mean it clearly shows that the names
>> of your variables are not chosen well.
>
> They come from NTP specification.

OK, clear. That's something you've read to write this, but someone who
reads your program does not always have to be familiar with this
documentation, nor has to read it first IMO.

my $roundtrip_delay = ($T4 - $T1) - ($T2 - $T3);

v.s.

my $roundtrip_delay = ( $destination_ts - $originate_ts )
- ( $receive_ts - $transmit_ts );

which is more readable?

>> Note that the else here makes that $detail and $limit are mutual
>> exclusive. Sounds odd to me. But if that's what you want, instead of
>> setting $bad to 1, and do a test at the end, you could just have
>> used:
>>
>> die "Server .... OFF ... \n";
>
> I want all bad servers reported, hence delayed dying.

My bad, I missed the additional }

In that case I probably would have used (warnings go to STDERR):

warn "Server ... \n";
next;
}

to keep it functionally the same. OTOH, I can't see why $limit and
$detail have to be mutually exclusive.


> if( $limit ) {
> if( $delta > $limit || -$delta < -$limit) {

if ( abs( $delta ) > $limit ) {

^ note the extra space, /if/ is not a function

--
John

http://johnbokma.com/mexit/

Ignoramus5390 wrote:

> my @servers = @ARGV;

die "Usage: query-ntp [--detail] [--limit=N] host1 host2 [host3 ...]\n"
unless ($#servers > 1);

--
Toby A Inkster BSc (Hons) ARCS
[Geek of HTML/SQL/Perl/PHP/Python/Apache/Linux]
[OS: Linux 2.6.17.14-mm-desktop-9mdvsmp, up 15 days, 5:47.]

NetSol Cybersquatting
http://tobyinkster.co.uk/blog/2008/0...ybersquatting/

Ignoramus5390 <ignoramus5390@NOSPAM.5390.invalid> writes:

> Over the years, I have had many problems with NTPd stopping to work
> without any notice. That always upset me.

Me too, but I took a different approach.
In root's crontab:

35 * * * * /root/scripts/ntp-check.sh

Content of script:

x=`/etc/rc.d/init.d/ntpd status`
case $x in
*stopped*|*dead*)
status=`/etc/rc.d/init.d/ntpd start 2>&1`
printf "had to start ntpd on `uname -n`, state <$x>\
\nrestart got status\n $status" | mail -s cron root
;;
esac

On 2008-01-14, Dan Espen <daneNO@MORE.mk.SPAMtelcordia.com> wrote:
> Ignoramus5390 <ignoramus5390@NOSPAM.5390.invalid> writes:
>
>> Over the years, I have had many problems with NTPd stopping to work
>> without any notice. That always upset me.
>
> Me too, but I took a different approach.
> In root's crontab:
>
> 35 * * * * /root/scripts/ntp-check.sh
>
> Content of script:
>
> x=`/etc/rc.d/init.d/ntpd status`
> case $x in
> *stopped*|*dead*)
> status=`/etc/rc.d/init.d/ntpd start 2>&1`
> printf "had to start ntpd on `uname -n`, state <$x>\
> \nrestart got status\n $status" | mail -s cron root
> ;;
> esac

Yeah, and what happens when it is alive, but does not talk to any
timeservers?

i

On 2008-01-14, Toby A Inkster <usenet200712@tobyinkster.co.uk> wrote:
> Ignoramus5390 wrote:
>
>> my @servers = @ARGV;
>
> die "Usage: query-ntp [--detail] [--limit=N] host1 host2 [host3 ...]\n"
> unless ($#servers > 1);
>

Thanks. I changed it to:

die "Usage: query-ntp [--detail] [--limit=N] host1 host2 [host3 ...]\n"
unless (@servers);

Thank you

i

Toby A Inkster <usenet200712@tobyinkster.co.uk> wrote:

> Ignoramus5390 wrote:
>
>> my @servers = @ARGV;
>
> die "Usage: query-ntp [--detail] [--limit=N] host1 host2 [host3 ...]\n"
> unless ($#servers > 1);


@servers > 2 or die "......";

(which I read as a precondition for the code that follows: there must be
more than 2 servers, otherwise ... )


It's a bit unclear to me why at least 3 hosts must be given (contradicting
the usage line):

items scalar @servers >2 $#servers > 1
0 0 f -1 f
1 1 f 0 f
2 2 f 1 f
3 3 t 2 t



(additional note:

there is no need for () after the unless (i.e. when used as a
statement modifier)
)



--
John

Arachnids near Coyolillo - part 1
http://johnbokma.com/mexit/2006/05/0...yolillo-1.html

On 2008-01-14, John Bokma <john@castleamber.com> wrote:
> Toby A Inkster <usenet200712@tobyinkster.co.uk> wrote:
>
>> Ignoramus5390 wrote:
>>
>>> my @servers = @ARGV;
>>
>> die "Usage: query-ntp [--detail] [--limit=N] host1 host2 [host3 ...]\n"
>> unless ($#servers > 1);
>
>
> @servers > 2 or die "......";
>
> (which I read as a precondition for the code that follows: there must be
> more than 2 servers, otherwise ... )
>

I am confused about all this. One server supplied on command line is
enough to do useful tests. Just one, not two or three.

ie

query-ntp --limit 1 www.mydomain.com

would test that the clock of mydomain.com is in sync with localhost.

or

query-ntp pool.ntp.org

would compare localhost with atomic clock.

I do not see why there is a need for requiring the user to supply more
than one server.

i

> It's a bit unclear to me why at least 3 hosts must be given (contradicting
> the usage line):
>
> items scalar @servers >2 $#servers > 1
> 0 0 f -1 f
> 1 1 f 0 f
> 2 2 f 1 f
> 3 3 t 2 t
>
>
>
> (additional note:
>
> there is no need for () after the unless (i.e. when used as a
> statement modifier)
> )
>
>
>

Ignoramus5390 <ignoramus5390@NOSPAM.5390.invalid> wrote:

> I am confused about all this. One server supplied on command line is
> enough to do useful tests. Just one, not two or three.

Me too, I would use something like:

@servers
or die "usage: ... query-ntp [--detail] [--limit=N] host [host ... ]

moreover, if you're supporting --usage, you might want to put the message
in a sub:


@servers or show_usage_and_die();

:

sub show_usage {

die ....
}


--
John Bokma http://johnbokma.com/

On 2008-01-14, John Bokma <john@castleamber.com> wrote:
> Ignoramus5390 <ignoramus5390@NOSPAM.5390.invalid> wrote:
>
>> I am confused about all this. One server supplied on command line is
>> enough to do useful tests. Just one, not two or three.
>
> Me too, I would use something like:
>
> @servers
> or die "usage: ... query-ntp [--detail] [--limit=N] host [host ... ]
>
> moreover, if you're supporting --usage, you might want to put the message
> in a sub:
>
>
> @servers or show_usage_and_die();
>
>:
>
> sub show_usage {
>
> die ....
> }
>
>
here's my new version, incorporating usage. It will be my new pattern
to follow:
################################################## ####################
#!/usr/bin/perl

# See Also:
# http://www.webreference.com/programming/perl/ntp/
# http://www.webreference.com/programming/perl/ntp/2.html

#
# Copyright(C) Igor Chudov, 2008.
# Released under GNU Public License version 3.
#

use strict;
use warnings;

use Data:umper;
use Getopt::Long;
use Net::NTP;
use Time::HiRes qw( time );

my $detail = undef;
my $limit = undef;
my $ = undef;

sub usage {
my ($msg) = @_;
print "ERROR: $msg\n\n";
print STDERR "Usage: $0 [--detail] [--limit=N] host {host}\n";
exit ($msg ? 1 : 0);
}

GetOptions(
"detail!" => \$detail,
"limit=f" => \$limit,
"!" => \$,
);

my @servers = @ARGV;

usage if $;
usage "detail and limit are mutually exclusive" if $detail and $limit;
usage "You need to supply one or more servers" unless (@servers);

my $bad = undef;

foreach my $server (@servers) {
my $t0 = time;
my %h = get_ntp_response( $server );


#Timestamp Name ID When Generated
#------------------------------------------------------------
#Originate Timestamp T1 time request sent by client
#Receive Timestamp T2 time request received by server
#Transmit Timestamp T3 time reply sent by server
#Destination Timestamp T4 time reply received by client
#
#The roundtrip delay d and local clock offset t are defined as
#
#d = (T4 - T1) - (T2 - T3) t = ((T2 - T1) + (T3 - T4)) / 2

my $T1 = $t0;
my $T2 = $h{'Receive Timestamp'};
my $T3 = $h{'Transmit Timestamp'};

my $T4 = time; # From Time::HiRes! Accurate to usec!

my $roundtrip_delay = ($T4 - $T1) - ($T2 - $T3);
my $local_clock_offset = (($T2 - $T1) + ($T3 - $T4)) / 2;
my $duration = $T4-$t0;
my $delta = $T2-$T1 - $duration/2;

if( $limit ) {
if( $delta > $limit || -$delta < -$limit) {
print "Server $server OFF by $delta seconds!!!\n";
$bad = 1;
} else {
print "Server $server OK, delta is $delta seconds!!!\n";
}

} else {
if( $detail ) {
print Dumper( \%h ) . "\nt0=$t0, T4=$T4\ndelay=$roundtrip_delay, offset=$local_clock_offset\nelapsed=$duration, delta=$delta\n\n";
} else {
#print "$server delay=$roundtrip_delay, offset=$local_clock_offset, delta=$delta, duration=$duration\n";
print sprintf( "%-30s %.5f\n", $server, $local_clock_offset );
}
}
}

exit 1 if $bad;

Ignoramus5390 <ignoramus5390@NOSPAM.5390.invalid> writes:

> On 2008-01-14, Dan Espen <daneNO@MORE.mk.SPAMtelcordia.com> wrote:
>> Ignoramus5390 <ignoramus5390@NOSPAM.5390.invalid> writes:
>>
>>> Over the years, I have had many problems with NTPd stopping to work
>>> without any notice. That always upset me.
>>
>> Me too, but I took a different approach.
>> In root's crontab:
>>
>> 35 * * * * /root/scripts/ntp-check.sh
>>
>> Content of script:
>>
>> x=`/etc/rc.d/init.d/ntpd status`
>> case $x in
>> *stopped*|*dead*)
>> status=`/etc/rc.d/init.d/ntpd start 2>&1`
>> printf "had to start ntpd on `uname -n`, state <$x>\
>> \nrestart got status\n $status" | mail -s cron root
>> ;;
>> esac
>
> Yeah, and what happens when it is alive, but does not talk to any
> timeservers?

The only time I ever saw it exit,
it lost communications to the time server.
Reading the man page, I think that's what it is supposed to do.

On 14 Jan 2008 18:10:25 GMT,
John Bokma <john@castleamber.com> wrote:
> Ignoramus5390 <ignoramus5390@NOSPAM.5390.invalid> wrote:
>
>> On 2008-01-14, John Bokma <john@castleamber.com> wrote:

>>> If you need to explain what IDs mean it clearly shows that the names
>>> of your variables are not chosen well.
>>
>> They come from NTP specification.
>
> OK, clear. That's something you've read to write this, but someone who
> reads your program does not always have to be familiar with this
> documentation, nor has to read it first IMO.
>
> my $roundtrip_delay = ($T4 - $T1) - ($T2 - $T3);
>
> v.s.
>
> my $roundtrip_delay = ( $destination_ts - $originate_ts )
> - ( $receive_ts - $transmit_ts );
>
> which is more readable?

To me the first one is more readable. While long variable names are good
in many ways, sometimes they hinder the easy parsing of expressions,
because too much wrap and distance gets in the way.

For me it's much easier to see what the expression does int he first
instance than in the second, even if the precise meaning of each
variable is not that clear.

Especially, also, given that these short variables are 'standard'
terminology, I see no problem with them.

Martien
--
|
Martien Verbruggen | I think there is a world market for maybe
| five computers. -- Thomas Watson, chairman of
| IBM, 1943

On 14 Jan 2008 18:10:25 GMT, John Bokma <john@castleamber.com> wrote:

>>> undef is the default value, I wouldn't assign it explicitly.
>>
>> I like this style. Makes it very explicit.
>
>IMO Perl programmers should know that the default is undef, and hence no
>need to assign undef. To confuse matters a bit more (IMO), you set $bad

FWIW, I wholeheartedly second that.


Michele
--
{$_=pack'B8'x25,unpack'A8'x32,$a^=sub{pop^pop}->(map substr
(($a||=join'',map--$|x$_,(unpack'w',unpack'u','G^<R<Y]*YB='
..'KYU;*EVH[.FHF2W+#"\Z*5TI/ER<Z`S(G.DZZ9OX0Z')=~/./g)x2,$_,
256),7,249);s/[^\w,]/ /g;$ \=/^J/?$/:"\r"rint,redo}#JAPH,

On 14 Jan 2008 18:10:25 GMT, John Bokma <john@castleamber.com> wrote:

>Ignoramus5390 <ignoramus5390@NOSPAM.5390.invalid> wrote:
>
>> On 2008-01-14, John Bokma <john@castleamber.com> wrote:
>
>[..]
>
>>> undef is the default value, I wouldn't assign it explicitly.
>>
>> I like this style. Makes it very explicit.
>
>IMO Perl programmers should know that the default is undef, and hence no
>need to assign undef. To confuse matters a bit more (IMO), you set $bad
>to undef, yet you assign later on 1 to it (in case of an error), and at
>the end of the script you test if $bad .... I try to avoid testing for
>undef that way, and I try to avoid to use 1/undef as a true/false. In
>short, I would've used something like:

Yep. Setting it to undef is $bad programming.



>OK, clear. That's something you've read to write this, but someone who
>reads your program does not always have to be familiar with this
>documentation, nor has to read it first IMO.
>
> my $roundtrip_delay = ($T4 - $T1) - ($T2 - $T3);
>
>v.s.
>
> my $roundtrip_delay = ( $destination_ts - $originate_ts )
> - ( $receive_ts - $transmit_ts );
>
>which is more readable?

The second, of course. Both would fail in my classes though because
there's no commenting.

Setting short vars like t1 t2 ... may work in a very short program
where no one is EVER going to follow behind you and maintain said
code. But when does that ever really happen.

--
gburnore at DataBasix dot Com
---------------------------------------------------------------------------
How you look depends on where you go.
---------------------------------------------------------------------------
Gary L. Burnore | ÝÛ³ºÝ³Þ³ºÝ³³Ýۺݳ޳ºÝ³Ý³Þ³ºÝ³ÝÝÛ³
| ÝÛ³ºÝ³Þ³ºÝ³³Ýۺݳ޳ºÝ³Ý³Þ³ºÝ³ÝÝÛ³
Official .sig, Accept no substitutes. | ÝÛ³ºÝ³Þ³ºÝ³³Ýۺݳ޳ºÝ³Ý³Þ³ºÝ³ÝÝÛ³
| ÝÛ 0 1 7 2 3 / Ý³Þ 3 7 4 9 3 0 Û³
Black Helicopter Repair Services, Ltd.| Official Proof of Purchase
================================================== =========================

On Tue, 15 Jan 2008 07:29:06 +1100, Martien Verbruggen
<mgjv@tradingpost.com.au> wrote:

>On 14 Jan 2008 18:10:25 GMT,
> John Bokma <john@castleamber.com> wrote:
>> Ignoramus5390 <ignoramus5390@NOSPAM.5390.invalid> wrote:
>>
>>> On 2008-01-14, John Bokma <john@castleamber.com> wrote:
>
>>>> If you need to explain what IDs mean it clearly shows that the names
>>>> of your variables are not chosen well.
>>>
>>> They come from NTP specification.
>>
>> OK, clear. That's something you've read to write this, but someone who
>> reads your program does not always have to be familiar with this
>> documentation, nor has to read it first IMO.
>>
>> my $roundtrip_delay = ($T4 - $T1) - ($T2 - $T3);
>>
>> v.s.
>>
>> my $roundtrip_delay = ( $destination_ts - $originate_ts )
>> - ( $receive_ts - $transmit_ts );
>>
>> which is more readable?
>
>To me the first one is more readable.

Feh.


>While long variable names are good
>in many ways, sometimes they hinder the easy parsing of expressions,
>because too much wrap and distance gets in the way.

HUH? He used two lines instead of one and that's bad? Please.
>
>For me it's much easier to see what the expression does int he first
>instance than in the second, even if the precise meaning of each
>variable is not that clear.

Really? How do you know what T1 - T4 are? Guess? No, you'd have to
go read them somewhere else. So much for your wrap and distance
getting in the way.

--
gburnore at DataBasix dot Com
---------------------------------------------------------------------------
How you look depends on where you go.
---------------------------------------------------------------------------
Gary L. Burnore | ÝÛ³ºÝ³Þ³ºÝ³³Ýۺݳ޳ºÝ³Ý³Þ³ºÝ³ÝÝÛ³
| ÝÛ³ºÝ³Þ³ºÝ³³Ýۺݳ޳ºÝ³Ý³Þ³ºÝ³ÝÝÛ³
Official .sig, Accept no substitutes. | ÝÛ³ºÝ³Þ³ºÝ³³Ýۺݳ޳ºÝ³Ý³Þ³ºÝ³ÝÝÛ³
| ÝÛ 0 1 7 2 3 / Ý³Þ 3 7 4 9 3 0 Û³
Black Helicopter Repair Services, Ltd.| Official Proof of Purchase
================================================== =========================

On Mon, 14 Jan 2008 13:41:25 -0600, Ignoramus5390
<ignoramus5390@NOSPAM.5390.invalid> wrote:

>On 2008-01-14, John Bokma <john@castleamber.com> wrote:
>> Ignoramus5390 <ignoramus5390@NOSPAM.5390.invalid> wrote:
>>
>>> I am confused about all this. One server supplied on command line is
>>> enough to do useful tests. Just one, not two or three.
>>
>> Me too, I would use something like:
>>
>> @servers
>> or die "usage: ... query-ntp [--detail] [--limit=N] host [host ... ]
>>
>> moreover, if you're supporting --usage, you might want to put the message
>> in a sub:
>>
>>
>> @servers or show_usage_and_die();
>>
>>:
>>
>> sub show_usage {
>>
>> die ....
>> }
>>
>>
>here's my new version, incorporating usage. It will be my new pattern
>to follow:
>################################################# #####################

I assume that the ######## is NOT your first line


>#!/usr/bin/perl
>
># See Also:
># http://www.webreference.com/programming/perl/ntp/
># http://www.webreference.com/programming/perl/ntp/2.html
>
>#
># Copyright(C) Igor Chudov, 2008.
># Released under GNU Public License version 3.
>#
>
>use strict;
>use warnings;
>
>use Data:umper;
>use Getopt::Long;
>use Net::NTP;
>use Time::HiRes qw( time );
>
>my $detail = undef;
>my $limit = undef;
>my $ = undef;
>
>sub usage {
> my ($msg) = @_;
> print "ERROR: $msg\n\n";
> print STDERR "Usage: $0 [--detail] [--limit=N] host {host}\n";
> exit ($msg ? 1 : 0);
>}
>
>GetOptions(
> "detail!" => \$detail,
> "limit=f" => \$limit,
> "!" => \$,
> );
>
>my @servers = @ARGV;
>
>usage if $;
>usage "detail and limit are mutually exclusive" if $detail and $limit;
>usage "You need to supply one or more servers" unless (@servers);
>
>my $bad = undef;
>
>foreach my $server (@servers) {
> my $t0 = time;
> my %h = get_ntp_response( $server );
>
>
> #Timestamp Name ID When Generated
> #------------------------------------------------------------
> #Originate Timestamp T1 time request sent by client
> #Receive Timestamp T2 time request received by server
> #Transmit Timestamp T3 time reply sent by server
> #Destination Timestamp T4 time reply received by client
> #
> #The roundtrip delay d and local clock offset t are defined as
> #
> #d = (T4 - T1) - (T2 - T3) t = ((T2 - T1) + (T3 - T4)) / 2
>
> my $T1 = $t0;
> my $T2 = $h{'Receive Timestamp'};
> my $T3 = $h{'Transmit Timestamp'};
>
> my $T4 = time; # From Time::HiRes! Accurate to usec!
>
> my $roundtrip_delay = ($T4 - $T1) - ($T2 - $T3);
> my $local_clock_offset = (($T2 - $T1) + ($T3 - $T4)) / 2;
> my $duration = $T4-$t0;
> my $delta = $T2-$T1 - $duration/2;
>
> if( $limit ) {
> if( $delta > $limit || -$delta < -$limit) {
> print "Server $server OFF by $delta seconds!!!\n";
> $bad = 1;
> } else {
> print "Server $server OK, delta is $delta seconds!!!\n";
> }
>
> } else {
> if( $detail ) {
> print Dumper( \%h ) . "\nt0=$t0, T4=$T4\ndelay=$roundtrip_delay, offset=$local_clock_offset\nelapsed=$duration, delta=$delta\n\n";
> } else {
> #print "$server delay=$roundtrip_delay, offset=$local_clock_offset, delta=$delta, duration=$duration\n";
> print sprintf( "%-30s %.5f\n", $server, $local_clock_offset );
> }
> }
>}
>
>exit 1 if $bad;

Still don't care for the T1-T4 and t0 and, frankly, missing spaces
between = symbols is most annoying.
--
gburnore at DataBasix dot Com
---------------------------------------------------------------------------
How you look depends on where you go.
---------------------------------------------------------------------------
Gary L. Burnore | ÝÛ³ºÝ³Þ³ºÝ³³Ýۺݳ޳ºÝ³Ý³Þ³ºÝ³ÝÝÛ³
| ÝÛ³ºÝ³Þ³ºÝ³³Ýۺݳ޳ºÝ³Ý³Þ³ºÝ³ÝÝÛ³
Official .sig, Accept no substitutes. | ÝÛ³ºÝ³Þ³ºÝ³³Ýۺݳ޳ºÝ³Ý³Þ³ºÝ³ÝÝÛ³
| ÝÛ 0 1 7 2 3 / Ý³Þ 3 7 4 9 3 0 Û³
Black Helicopter Repair Services, Ltd.| Official Proof of Purchase
================================================== =========================

In article <fmgr5g$7p5$4@blackhelicopter.databasix.com>, Gary L.
Burnore <gburnore@databasix.com> wrote:

> On Mon, 14 Jan 2008 13:41:25 -0600, Ignoramus5390
> <ignoramus5390@NOSPAM.5390.invalid> wrote:
>

> >here's my new version, incorporating usage. It will be my new pattern
> >to follow:
> >################################################# #####################
>

[some code snipped]

> >
> >my $detail = undef;
> >my $limit = undef;
> >my $ = undef;
> >

[more code snipped]

> > if( $detail ) {
> > print Dumper( \%h ) . "\nt0=$t0, T4=$T4\ndelay=$roundtrip_delay,
> > offset=$local_clock_offset\nelapsed=$duration, delta=$delta\n\n";
> > } else {
> > #print "$server delay=$roundtrip_delay, offset=$local_clock_offset,
> > delta=$delta, duration=$duration\n";
> > print sprintf( "%-30s %.5f\n", $server, $local_clock_offset );
> > }
> > }
> >}
> >
> >exit 1 if $bad;
>
> ... frankly, missing spaces
> between = symbols is most annoying.

There are spaces around each '=' in every assignment statement. The
only '=' without surrounding spaces are in print statements, which
appear to be debug statements. I do the same thing in my code --
snuggle up the preceding and succeeding characters in a print statement
-- to see if there is any whitespace in the variables I am printing.
Readability of the output is not the issue; it is done for accuracy of
results.

--
Jim Gibson

Posted Via Usenet.com Premium Usenet Newsgroup Services
----------------------------------------------------------
** SPEED ** RETENTION ** COMPLETION ** ANONYMITY **
----------------------------------------------------------
http://www.usenet.com

On Mon, 14 Jan 2008 15:43:50 -0800, Jim Gibson <jimsgibson@gmail.com>
wrote:

>In article <fmgr5g$7p5$4@blackhelicopter.databasix.com>, Gary L.
>Burnore <gburnore@databasix.com> wrote:


>> ... frankly, missing spaces
>> between = symbols is most annoying.
>
>There are spaces around each '=' in every assignment statement. The
>only '=' without surrounding spaces are in print statements, which
>appear to be debug statements. I do the same thing in my code --
>snuggle up the preceding and succeeding characters in a print statement
>-- to see if there is any whitespace in the variables I am printing.
>Readability of the output is not the issue; it is done for accuracy of
>results.

Fair enough.
--
gburnore at DataBasix dot Com
---------------------------------------------------------------------------
How you look depends on where you go.
---------------------------------------------------------------------------
Gary L. Burnore | ÝÛ³ºÝ³Þ³ºÝ³³Ýۺݳ޳ºÝ³Ý³Þ³ºÝ³ÝÝÛ³
| ÝÛ³ºÝ³Þ³ºÝ³³Ýۺݳ޳ºÝ³Ý³Þ³ºÝ³ÝÝÛ³
Official .sig, Accept no substitutes. | ÝÛ³ºÝ³Þ³ºÝ³³Ýۺݳ޳ºÝ³Ý³Þ³ºÝ³ÝÝÛ³
| ÝÛ 0 1 7 2 3 / Ý³Þ 3 7 4 9 3 0 Û³
Black Helicopter Repair Services, Ltd.| Official Proof of Purchase
================================================== =========================

Jim Gibson <jimsgibson@gmail.com> wrote:

> In article <fmgr5g$7p5$4@blackhelicopter.databasix.com>, Gary L.
> Burnore <gburnore@databasix.com> wrote:

>> > #print "$server delay=$roundtrip_delay,
>> > offset=$local_clock_offset, delta=$delta,
>> > duration=$duration\n"; print sprintf( "%-30s %.5f\n", $server,

>> ... frankly, missing spaces
>> between = symbols is most annoying.
>
> There are spaces around each '=' in every assignment statement. The
> only '=' without surrounding spaces are in print statements, which
> appear to be debug statements. I do the same thing in my code --
> snuggle up the preceding and succeeding characters in a print
> statement -- to see if there is any whitespace in the variables I am
> printing. Readability of the output is not the issue; it is done for
> accuracy of results.

I use either [] (mostly for debugging) or ''. Both have the added
advantage that one can see a spurious newline, e.g.

duration = [10
]

In this case, where numbers are printed, I would use extra spaces but
neither [] nor ''.

anyway, as always YMMV :-)

--
John

http://johnbokma.com/mexit/