Re^5: RFC: Net::SNTP::Client v1

read() is just an example for the thing I am recommending. Okay, let's take it from the top. Your code:

    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.

This is how I would tackle this:

# 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]

This is a similar behaviour to read() and that's why I mentioned it.

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

Comment on Re^5: RFC: Net::SNTP::Client v1 Select or Download Code

Replies are listed 'Best First'.
Re^6: RFC: Net::SNTP::Client v1 by thanos1983 (Parson) on Jul 02, 2015 at 22:29 UTC
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!	[reply] [d/l] [select]
Re^7: RFC: Net::SNTP::Client v1 by Monk::Thomas (Friar) on Jul 03, 2015 at 08:09 UTC
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	[reply] [d/l]