So you know Perl; but do you know programming?

[Marza]

I never considered myself a talented coder/programmer/scipter but I know enough to build some useful tools that don't break.....often ;)

As with many shops we have scripts that you "don't touch because they work." One in particular is our Unix account creater that was written by a "Perl Guru" who was paid a great deal of money to automate several funtions.

After taking a class, reading a few books, reading stuff from the Net, and writing a few reports. I was asked to add a "feature" to the account creater.

I reviewd the source and I now want to be a consultant. The guy who wrote this "automation" took a rather heavy handed approach.

The following is a piece of the mess. Basically, you are prompted for a username, the passwd file gets loaded to an array, it checks for duplicates, and it looks for an open userid:

system("clear");
print "CREATE ACCOUNT:\n\n";
$username = &ask("Enter the login id of the new user: ");

#
# Read the local passwd file.
#
open (PWD,"$LOCAL_PASSWD")|| die "can't open $LOCAL_PASSWD\n";
while (<PWD>) {
   push(@local_passwd,$_);
}
close (PWD);

#
# Figure out what uids have been used
#
for ($i=0 ; $i<=$#local_passwd && ($_ = $local_passwd[$i]); $i++) {
   ($uname,$pwd,$uid) = split(/:/);

   push(@user_ids,$uid);
}

foreach $i (200..2000) {
   foreach $uid (@user_ids) {
      $uid_ok=1;
      if ($i eq $uid) {
         $uid_ok=0;
         last;
      }
   }

   if ($uid_ok) {
      $g_user_id = $i;
      last;
   }
}


undef @user_ids;

#
# Read the local password
#
open(PWD,"$LOCAL_PASSWD")|| die "can't open the $LOCAL_PASSWD\n";
while (<PWD>) {
   push(@local_passwd,$_);
}
close (PWD);

#
# Check for duplicate names
#
for ($i=0 ; $i<=$#local_passwd && ($_ = $local_passwd[$i]); $i++) {
   ($uname) = split(/:/);

   if ($uname eq $username) {
      print "DUPLICATE NAME: $username: can't continue\n";
      exit(1);
   }
}
undef @local_passwd;

Like I said, it does the job but in a rather heavy handed way. Guess the guy never heard of hashing.

I suggested a re-write and of course the bosses heart rate went up. So I just got the above example and threw together some code to show a difference.

print $clear_screen;
print "CREATE ACCOUNT:\n\n";
$username = &ask("Enter the login id of the new user: ");
my %account;
my %uid;
#
# Read the local passwd file.
#
open (PWD,"$LOCAL_PASSWD") or die "\n\nUnable to open $LOCAL_PASSWD\n\
+n";
while (<PWD>) {
   chomp;
   my ($accountname, $accountid) = (split/:/)[0,2];
   $account{$accountname} = $accountid;
   $uid{$accountid} = $accountname;
}
close (PWD);

if (exists $account{$username}) {
   die "This account name already exists!\n";
}

#
# Loop through our range and see if we have an available number.
# If a hash entry does not exist, use the missing number.
foreach (200..2000) {
   unless ( exists $uid{$_} ) {
      print "Hey $_ is available!...assigning...\n";
      $g_user_id = $_;
      last;
   }
}

After seeing the reduction, he ok'd the rewrite.

Powerful computers can be a friend as well as an enemy. Sure the script does the job and it isn't even slow, but was the approach right? I don't think so. In this age of "put it in now, we will fix it later" bad code seems to slip in more and more. I should ask for a bonus for rewriting the "experts" scripts. ;)

All in all, I guess this is an example of how you should have people that understand the language and programming in general to interview consultants.

Edited: 04 July 2002, by footpad
Reason: Added <readmore>, per Consideration.

[Zaxo]

I'd call getpwnam $username to check existence, do {1} while getpwuid ++$uid; to find an available uid.

There are corresponding perl functions for dealing with groups.

There is a User::pwent module interface, but its syntactical sugar is fairly thin, imo.

After Compline,
Zaxo

[Marza]

Excellent! There is always a better way to do something. Now I can dump the hashes and one loop! Cool more stuff to read and play! Hmmm I will have to review the perl docs again. Should have saw that one!

Thank you! ++ when I get votes again!

[Aristotle]

Some further points on top of what Zaxo said:

A minor one: habitually limit splits unless you explicitly want "any number" of results. In this case, you're not interested in anything past the 3rd element, so limit it to 4 columns.

And you're doing too much work. If you do not explicitly want the smallest free UID assigned to the new account, you can roll the entire thing into the first loop that reads the passwd:

my $g_user_id = 200;
while (<PWD>) {
   my ($accountname, $accountid) = (split /:/, $_, 4)[0,2];
   die "This account name already exists!\n" if $accountname eq $usern
+ame;
   $g_user_id = $accountid + 1 if $accountid > $g_user_id;
}
die "Ack, too many UIDs assigned.\n" if $g_user_id > 65535;

Note I removed the chomp since the last record on the line is of no interest anyway.

But, yes, your premise is absolutely correct. I believe you will enjoy the following read: Teach Yourself Programming in Ten Years. It pretty much sums up the entire topic.

Makeshifts last the longest.

[Marza]

Hmmmmm. I never thought about waste before and you are right I was only interested in two particular fields. Cool tighter code!

Thanks for the link! That was an interesting article! Hmm can safely say I don't have any "Days" books! ;)

Thank you! ++ When I get some more votes!

[Abigail-II]

Why bother with the rewrite? It wasn't broken, so there wasn't much to gain, not even in running time.

Furthermore, there's a lot more that you could have improved. Why are you parsing a password file yourself? That's why you have getpwent. And what's the point of storing all the existing usernames in a hash? A single call with getpwnam could have told whether a name was free or not.

Finally, this program seems to favour reusing userids over issueing unused ones. Reusing userids is not considered a good thing - a new user might inherit files and other pieces of history from a previous user. It's better to just pass over all the entries, remember the highest issued id, and use the next one available.

And if you really wanted to impress your boss, you would have replaced the program with a one liner:

    exec useradd => @ARGV

Or, if you are on Linux, and really want a question and answer session:

    exec 'adduser'

Of course, I would have replaced it with a 0 liner, and used the already available tools.

Abigail

[Aristotle]

Whether or not to use an own tool over the existing ones is a matter of several factors. Though I have to agree that useradd is the better idea, using solely existing tools is a question of purpose and may or may not be justified in this case. Marza's script may be doing more than we see from the snippet.

F.ex, I wrote a script for a site I worked at that dealt with the details of calling useradd for me; the same script also did a bunch of other things like add entries to a URL rewrite map, an ftpd configuration and several other steps.

Makeshifts last the longest.

[Marza]

Correct. It does a few more things as I told Abigail. They want an all in one script to handle all accounts in one run. Automation of a hetrogenus environment. Hey where is SSO? ;)

[Marza]

Hello Abigail!

The answer is simple. The script is used because we have NIS. Useradd only supports local entries.

The snippet was only an example of doing much more than was needed: 7 loops and three file opens to simply verify a username and to get a uid?

Sure the hashing is overkill. At the time I either didn't know about getpwnam or forgot about it. However, even with the 2 hashes. It reduced the looping to 2 and 1 file open. However, with the suggestions. I don't plan to use the hashes and I am dropping 1 loop.

As to reusing uids? You are correct. However, when an employee leaves, his account is disabled(ie "**" for a password. A list of stuff owned is generated and other workers go through his files and chown what is needed and delete what is not. This eliminates "We forgot what it was for so we should hold onto it." When there are no more files owned, the account gets deleted.

Finally, the script does more then what the snippet shown. Mail entries and mail lists, Meeting Maker account creation(what will be added), and a section for NT creation as well as other stuff like config files, etc.

But I do thank you for the comments!