my $verify_port = sub {
    my $port = shift;
    if ($port) {
        if ($port > MAX_PORT or $port < MIN_PORT){
        return TRUE;
        }
    }
    else {
        $moduleInput{-port} = 123;
        return FALSE;
    }
    };
[download]

If the port is outside the valid range, then you return true, if it is inside the allowed port range then you return (I am not sure - if no explicit return value is specified then the one from the last assignment is chosen - so - $port?) and if the port is undefined you set some value and return false.

btw. the '$moduleInput{-port} = 123;' thing is bad style because you're modifying some completely unrelated piece of data. It's a lot better to make it obvious when some value is changed.

# I ~love~ readonly. Primarily because you can write stuff like:
# print "$MIN_TCP_PORT < x < $MAX_TCP_PORT\n";
use Readonly;

Readonly my $DEFAULT_NTP_PORT => 123;
Readonly my $MIN_TCP_PORT     => 1;
Readonly my $MAX_TCP_PORT     => 65535;

# in case of success: return a 'true' value   (1 or a useful value)
# in case of error:   return a 'false' value  (0 or undef)
sub verify_port {
  my $value = shift;

  # user provided input should be viewed as toxic - do not
  # trust until proven to contain a sane value
  if (defined $value and $value =~ /\A (\d+) \z/xms) {
    my $port = $1;

    if ($MIN_TCP_PORT <= $port and $port <= $MAX_TCP_PORT) {
      return 1;  # or TRUE or $port
    }
  }

  return 0;  # or FALSE or undef
}

# get a port value from somewhere, e.g. Getopt::Long
# use a default value if no value has been provided
my $input = $opt_ntp_port // $DEFAULT_NTP_PORT;

# - usage a -
if (verify_port($input)) {
  (do stuff with $input)
}
else {
  (issue warning / bail out)
}

# - usage b -
verify_port($input)
  or die;

# - usage c -
# (possible if verify_port() returns $port)
my $ntp_port = verify_port($input)
  or die 'Invalid ntp port!';
[download]

Usage c would not work if the returned value can be 0. Luckily 0 is not an allowed port for TCP!

Hello Monk::Thomas,

Finally I understood your comments, I was not able to understand that you first need to check if the port number is actually is an Integer:

if (defined $value and $value =~ /\A (\d+) \z/xms)
[download]

Well a few things that I noticed is that you need to modify the second condition from:

if ($MIN_TCP_PORT <= $port and $port <= $MAX_TCP_PORT)

to:

if ($MIN_TCP_PORT <= $port and $port >= $MAX_TCP_PORT)

The port should not be greater than the MAX_TCP_PORT not minor. ;)

Question, why are you assigning the $value to a new value $port = $1. Is there a specific reason?

Last modification that I applied on my updated version, is the port. You are right just by modifying the hash would not be clear to someone else on the future.

So I added a simple or condition when I am creating the socket. Sample of modification:

PeerPort => $moduleInput{-port} || DEFAULT_NTP_PORT, # Default NTP por
+t 123
[download]

Again thank you for your time and effort reviewing my code.

Seeking for Perl wisdom...on the process of learning...not there...yet!

Glad to be of help. =)

Well a few things that I noticed is that you need to modify the second condition...

No you don't. Lets have a look at the code again:

# in case of success: return a 'true' value   (1 or a useful value)
# in case of error:   return a 'false' value  (0 or undef)
sub verify_port {
  my $value = shift;

  # user provided input should be viewed as toxic - do not
  # trust until proven to contain a sane value
  if (defined $value and $value =~ /\A (\d+) \z/xms) {
    my $port = $1;

    if ($MIN_TCP_PORT <= $port and $port <= $MAX_TCP_PORT) {
      return 1;  # or TRUE or $port
    }
  }

  return 0;  # or FALSE or undef
}
[download]

As stated in the function's comment, the function is supposed to return a 'true' value if everything is alright and a 'false' value in case it isn't. (You did it the other way round - return FALSE if the port is fine and TRUE if it isn't.) So what I am doing is to make sure the provided port is defined, an integer and _inside_ the allowed port range. Only then '1' (or TRUE or $port) is returned.

In all other cases (e.g. value is undefined or outside the allowed port range) then the function will return '0' (or FALSE or undef)

Question, why are you assigning the $value to a new value $port = $1. Is there a specific reason?

It is just an additional safeguard to make sure I perform the second check against a validated value. Additionally if you choose to return $port instead of 1 or TRUE then you can use the function to untaint your external / user-provided input.

http://perldoc.perl.org/perlsec.html#Taint-mode
http://www.webreference.com/programming/perl/taint/index.html