semio has asked for the wisdom of the Perl Monks concerning the following question:

fellow monks,

I've constructed a module that consists of several subs I've written while working with nessus nbe data. While writing network reports, these routines have become fairly useful to me in cleaning up and presenting specific data. Currently, the module has subs that will present os versions, service banners, ports and a few others. I plan to add more subs to it as I come across other data I may want to extract. So far, it looks as follows:

package NessusNBE; #arbitrary at the moment use strict; use vars qw/ $VERSION @ISA @EXPORT /; require Exporter; @ISA = qw/ Exporter /; @EXPORT = qw/ nbanners nports nplugin nwebdirs nnfs nos /; $VERSION = '1.0'; sub nbanners { my (@ndata) = @_; my (@banners); foreach my $nbanner (@ndata) { if ( $nbanner =~ /emote(.*)server (banner|type)/ ) { my @result = split ( /\|/, $nbanner ); $result[6] =~ s/^(.*)\:\\n|Solution (.*)$|\\r|\\n//g; push @banners, join "|", $result[2], $result[6]; } } return @banners; } sub nports { my (@ndata) = @_; my (@ports); my $nport = pop (@ndata); foreach my $ndata (@ndata) { my @result = split ( /\|/, $ndata ); if ( $result[4] ) { next; } elsif ( $result[3] =~ /\($nport\// ) { push @ports, join "|", $result[2], $result[3]; } } return @ports; } sub nplugin { my (@ndata) = @_; my (@plugins); my $nplugin = pop (@ndata); foreach my $ndata (@ndata) { my @result = split ( /\|/, $ndata ); if ( !$result[4] ) { next; } elsif ( $result[4] =~ /$nplugin/ ) { push @plugins, join "|", $result[2], $result[3], $result[4 +]; } } return @plugins; } sub nwebdirs { my (@ndata) = @_; my (@webdirs); my $webdirplugin = 11032; foreach my $ndata (@ndata) { my @result = split ( /\|/, $ndata ); if ( !$result[4] ) { next; } elsif ( $result[4] =~ /$webdirplugin/ ) { $result[6] =~ s/^(.*)discovered\:|\\n//g; $result[6] =~ s/The following(.*)authentication/ --auth re +quired/; push @webdirs, join "|", $result[2], $result[3], $result[6 +]; } } return @webdirs; } sub nnfs { my (@ndata) = @_; my (@nfs); my $nfsplugin = 10437; foreach my $ndata (@ndata) { my @result = split ( /\|/, $ndata ); if ( !$result[4] ) { next; } elsif ( $result[4] =~ /$nfsplugin/ ) { $result[6] =~ s/^(.*) \: \\n|\\n\\n(.*)$//g; push @nfs, join "|", $result[2], $result[3], $result[6]; } } return @nfs; } sub nos { my (@nos) = @_; my (@os); foreach my $nos (@nos) { my @result = split ( /\|/, $nos ); if ( $nos =~ m/10336\|Security Note|11268\|Security Note/ ) { if ( $result[4] eq 10336 ) { $result[6] =~ s/(Nmap(.*)running |(\;|\\n))//g; push @os, join "|", $result[2], $result[6]; } elsif ( $result[4] eq 11268 ) { $result[6] =~ s/(Remote OS guess : |\\n\\n(.*)$)//g; push @os, join "|", $result[2], $result[6]; } } } return @os; } 1;
As this is my first attempt at putting together a module, I wanted to post it to see if I'm on the right track and to be made aware of any shortcomings that may be present (at my level, there are probably many ;) ). Would someone mind critiquing this, as a module or otherwise.

cheers, -semio

Replies are listed 'Best First'.
Re: module review
by dragonchild (Archbishop) on May 08, 2003 at 19:30 UTC
    1. Add POD.
    2. Add more POD.
    3. Add still more POD, this time about the expected inputs and outputs.
    4. Make people understand what your magic numbers are. Maybe use constants?
    5. While on that topic, add more comments, documenting assumptions.
    Other than that, it looks great! :-)

    ------
    We are the carpenters and bricklayers of the Information Age.

    Don't go borrowing trouble. For programmers, this means Worry only about what you need to implement.

    Please remember that I'm crufty and crochety. All opinions are purely mine and all code is untested, unless otherwise specified.

Re: module review
by hmerrill (Friar) on May 08, 2003 at 21:47 UTC
    Looks pretty good to me. The one thing that kind of caught my eye was
    @EXPORT = qw/ nbanners nports nplugin nwebdirs nnfs nos /;
    Here's a snippet from 'perldoc perlmodlib':
    · Select what to export. Do NOT export method names! Do NOT export anything else by default without a good reason! Exports pollute the namespace of the module user. If you must export try to use @EXPORT_OK in preference to @EXPORT and avoid short or common names to reduce the risk of name clashes. Generally anything not exported is still accessible from outside the module using the ModuleName::item_name (or "$blessed_ref->method") syntax. By convention you can use a leading underscore on names to indicate informally that they are ’internal’ and not for public use. As a general rule, if the module is trying to be object oriented then export nothing. If it’s just a collection of functions then @EXPORT_OK anything but use @EXPORT with caution.
    There's also a good section in "Programming Perl" 3rd Edition about creating modules and using Exporter - starts on p.302. If you don't have this book, I'd highly recommend getting it - it is *THE* perl reference book. I also remember seeing a section about creating modules in the book "Effective Perl Programming".

    HTH.