Network chat in Perl

Question

I have written a network chat program as my first major project in Perl. It makes simple use of REGEXP's, modules, sockets, command-line option parsing, and forking and uses these features to produce a fairly simple client-server chat program with a basic nickname system that is capable of handling as many clients at once as IO::Socket::INET is capable of.

I would like to know a few things; firstly, is this good Perl? This is my first major project so I am expecting to have made some rookie mistakes in both programming style and use of features. Secondly, what do you think about the readibility of them? I know Perl is pretty notorious for having nearly imposible to read code, but I tried my best to keep this as readable as possible. Finally, how was my use of sockets and the networking features? This is also my first ever networking project so I'm unsure as to whether I've done it well or if I've made any major faux-pas's.

server.pl

use strict;
use warnings;
use IO::Socket::INET;
use Getopt::Long;

my $MAXLEN = 1024;
my $PORTNO;

GetOptions("port=i" => \$PORTNO);
die "Need port!\n" unless defined $PORTNO;

my $sock = IO::Socket::INET->new(
        LocalPort   => $PORTNO, 
        Proto       => 'udp'
    ) or die "sock: $!";

print "Waiting for users on $PORTNO...\n";

my %clients;
my $msg;

while ($sock->recv($msg, $MAXLEN)) {
    my $ipaddr      = gethostbyaddr($sock->peeraddr, AF_INET);
    my $port        = $sock->peerport;
    my $cur_client  = "$ipaddr:$port";
    my $first_msg   = 0;

    if (not exists $clients{$cur_client}) {
        $clients{$cur_client}->{nick}       = "Guest";
        $clients{$cur_client}->{address}    = $ipaddr;
        $clients{$cur_client}->{port}       = $port; 
        $first_msg                          = 1;
    }

    if ($msg =~ /\/nick (\w+)/) {
        my $prev_nick = $clients{$cur_client}->{nick};
        $clients{$cur_client}->{nick} = $1;
        if ($first_msg) {                   # I feel like this section is a bit hackey, is there a better way of doing this?
            $msg = "[Server] new user: $1";
            $first_msg = 0;
        } else {
            $msg = "[Server] nick change: $prev_nick -> $1 ";
        }
    } else {
        $msg = join "", $clients{$cur_client}->{nick}, ": ", $msg;
    }

    print $msg, " ($cur_client)\n";

    for (keys %clients) {
        close $sock;
        my $sock_send = IO::Socket::INET->new(  # I feel like this should be unnecesary, is there a way of modifying 
            LocalPort   => $PORTNO,             # the existing $sock object instead of creating a new one each time
            Proto       => 'udp',               # I want to send a message?
            PeerAddr    => $clients{$_}->{address},
            PeerPort    => $clients{$_}->{port}
        ) or die "sock: $!";
        $sock_send->send($msg);
        close $sock_send;
    }

    $sock = IO::Socket::INET->new(
        LocalPort   => $PORTNO, 
        Proto       => 'udp'
    ) or die "sock: $!";
} 

client.pl

use strict;
use warnings;
use IO::Socket::INET;
use Getopt::Long;

my ($port, $host);
my $nick = "Guest";
my $MAXLEN  = 1024;

GetOptions( "port=i"    => \$port,
    	    "host=s"	=> \$host,
            "nick=s"    => \$nick);

die "Need Port!\n" unless defined $port;
die "Need host!\n" unless defined $host;

my $sock = IO::Socket::INET->new(
            Proto       => 'udp',
            PeerPort    => $port,
            PeerAddr    => $host
        ) or die "Creating socket: $!\n";

my $child;

if($child = fork) {
    while(1) {
        $sock->recv($_, $MAXLEN) or die "recv: $!\n";
        print "$_\n";
        next;
    }
}

die "fork: $!\n" unless defined $child;

print "Connected as $nick to $host:$port\n";
$sock->send("/nick $nick") or die "send: $!\n";

while(<STDIN>) {
    chomp;
    $sock->send($_) or die "send: $!\n";
}

Note - "client.pl" requires both a host and a port in the form --host=blah --port=blahblah, and "server.pl" requires a only a port (in the same format).

Answer 1

use strict; use warnings;

Excellent!

sub main { ... }

Your code should be wrapped in a main subroutine with the last statement being a call to main() (or main(@ARGV)).

logging

I would at least abstract your logging away into a function, e.g.:

sub loginfo { print @_ , "\n"}
...
loginfo "Waiting for users on $PORTNO";

Then the intent of your print statements are a lot clearer. Moreover, it makes it a lot easier to modify the output format (like auto-adding newlines, adding a timestamp, etc.) or to switch over to a real logging module.

Note that by default the STDOUT file handle is buffered. Think about setting $| = 1 or flushing STDOUT after each write to ensure that your log messages are emitted in a timely fashion. Or, consider logging to STDERR.

close $sock

Closing and re-opening $sock shouldn't be necessary, and by doing so you introduce a race condition between the time the $sock is closed and when it re-opened. Packets received by the OS in that interval will just get dropped (since there is no socket configured to receive them.)

It is much better to leave $sock open for the entire lifetime of the server. Your program will then have this structure:

sub server_loop {
  my $sock = IO::Socket::INET->new(...)
  while (1) {
    my $r = $sock->recv($msg, $MAXLEN);
    if (!defined($r)) {
      die "sock recv error: $!\n";
    }
    # process message
  }
}

Note there is no need to explicitly close $sock. That will be handled automatically when control returns from server_loop.

for (keys %clients)

Avoid using $_ as the loop variable in a foo loop. Always specify a named lexical, e.g.:

for my $client (key %clients) {

}

Also consider iterating over values %clients since you never use $_ except to index into %clients.

$sock_send = ...

You are re-creating the output socket for each message sent. This is actually not such a bad idea since you have decided to use UDP as the message protocol. If you were using TCP you would have to save these open socket handles in the $client record.

Sending a UDP message to a particular address and port is a useful function, so why not factor it out:

sub send_udp_message {
  my ($local_port, $dest_address, $dest_port, $message) = @_;
  my $client = IO::Socket::INET->new(...);
  $client->send($message);
  # again, no need to close $client
}

# ...in the server loop...

Then the logic of the server loop becomes clearer:

  for my $client (values %clients) {
    send_udp_message($PORTNO, $client->{addres}, $client->{port}, $message);
  }

Update

The way to send datagrams to multiple destinations is to use the sendto() version of send.

for $client in (values %client) {
  $sock->send($message, 0, $client->{address}));
}

Notes:

• $sock is the server's socket
• $client->{address} the packed version of the client's address. This is the same as what ->recv() returns.

By using the extended version of the ->send() you can avoid closing and reopening the server's socket.

See:

• perldoc send
• SO: Perl send() UDP packet to multiple active clients

Another option is to use multicast - e.g. IO::Socket::Multicast