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

Well, I've been fooling around with this for the last three days... I'd welcome any help. Mail::Sendmail complains saying 'bad or missing from address', but I think the real issue is that it doesn't like being fed a hash in an array.

Please comment on style, efficiency, and whatever else you would like to comment on.

This is a complete rewrite of an existing app I wrote that retrieves email from a pop3 server, and forwards it to a list.

Code included below.
#!f:/program/perl/bin/perl -w #/usr/bin/perl -w # ################ use strict; use Mail::Pop3client; use Mail::Sendmail; #use Data::Dumper; # SMTP server ########################## #my $smtp = 'mail'; my $smtp = "24.0.95.87"; # set $save to 0 to disable saving copies of email to $savedir ########################## my $save = 1; # this is the directory to save copies of the forwarded email into # the save format is YYYY-MM-DD-HH-MM-SS.txt ########################## #my $savedir = "/usr/home/somewhere"; my $savedir = "f:/project/mailfwd/mail"; # turn on verbose. Default 0. ########################## my $verbose = 1; ################################# ################################# # no need to chang anything below # unless you want to. :-) ################################# ######### # checks the args - if it fails, it calls usage and exits ######### my ($username, $host, $pass, $list) = &check_args(); print "Verbose ON\n" if ($verbose); my $version = "Dystrophy Mail Forward v1.03b"; print "$version\n" if ($verbose); &check_mail(); ################# sub check_mail { my $pop = new Mail::POP3Client( USER => "$username", PASSWORD => "$pass", HOST => "$host" ); print "user: $username, pass: $pass, host: $host\n" if ($verbose); my $num_email = $pop->Count(); my @emails = (); if ( $num_email > 0 ) { @emails = &read_list($list); } else { return; } my ($i, @tank, $OK_sender, $body, @head); for ($i = 1; $i <= $num_email; $i++) { $tank[$i]{'smtp'} = $smtp; #set the smtp server $tank[$i]{'X-Mailer'} = $version; my @head = $pop->Head( $i ) ; $tank[$i]{'To'} = (); ($tank[$i]{'From'}, $tank[$i]{'Subject'}) = &parse_head(@head) +; $tank[$i]{'Body'} = $pop->Body( $i ) ; if ($save) { if (-d $savedir && -r $savedir && -w $savedir) { my $path = $savedir."/".&getDate.".txt"; open (FH, ">$path") or print "Could not open $path to +save copies of email: $!\n"; # change normal array separator my $foo = $"; $" = "\n"; print FH "@head"; $" = $foo; # change back print FH $tank[$i]->{'Body'}; close (FH); } else { print "$savedir is not a directory or does not have th +e correct permissions.\nCould not save email $i"; } } ################# # this delete is commented out to make it easier to test the app ################# #$pop->Delete($i) || print STDERR "Could not Delete message nu +m $i: $pop->Message()\n"; #print "Deleted msg $i\n" if ($verbose); } #$pop->Close(); #pop3client complains about this!?! ############### $i = 1; while ( $i <= $num_email ) { #print "From: ".$tank[$i]->{'From'}."\n" if ($verbose); #print "Subject: ".$tank[$i]->{'Subject'}."\n" if ($verbose); my $OK_sender = &check_email($tank[$i]->{'From'}, @emails); if ($OK_sender) { #print $tank[$i]{'From'}."\n"; foreach my $line (@emails) { $tank[$i]{'To'} = "$line" ; my $mail = $tank[$i] ; #print Dumper ($mail, $OK_sender); if ( sendmail $mail ) {} else{print $Mail::Sendmail::error;} exit; print "OK. Log says: $Mail::Sendmail::log \n" if ($v +erbose); } } else { $tank[$i]{'To'} = $tank[$i]->{'From'}; $tank[$i]{'From'} = "Mailer-Daemon <$username\@$host>"; $tank[$i]{'Subject'} = "Returned: Sender Unknown"; my $old_body = $tank[$i]->{'Body'}; $tank[$i]->{'Body'} = "Your email was returned because you + were not found in the list email database. You need to be subscribe +d to send to the list.\n\n>>>> Original Message Attached Below: <<<<\ +n>$old_body"; if ( sendmail $tank[$i] ){} else{print $Mail::Sendmail::error;} print "OK. Log says: $Mail::Sendmail::log \n" if ($verbo +se); } } } ##################### sub check_email { my ($from, @emails) = @_; my $OK = 0; my $address = (); my @array = split (/\s/, $from); foreach my $line (@array) { if ($line =~ /@/) { $line =~ s/^<(.*)>$/$1/ ; foreach (@emails) { $OK = 1 if ($_ =~ /$line/); } } } return $OK; } ############ sub parse_head { my (@head) = @_; my ($from, $subject); foreach (@head) { $from = $1 if (/^From: (.*)/); $subject = $1 if (/^Subject: (.*)/); } return ($from, $subject); } ############ sub check_args { if ($#ARGV ne 3) { &usage; } else { return ( $ARGV[0], $ARGV[1], $ARGV[2], $ARGV[3] ); } } ########## sub getDate { my ($sec,$min,$hour,$mday,$mon,$year,$wday,$yday,$isdst) ; ($sec,$min,$hour,$mday,$mon,$year,$wday,$yday,$isdst) = localtime( +time); $mon++; # fixes Matt's Counter bug if ($min < 10) { $min = "0$min"; } if ($hour < 10) { $hour = "0$hour"; } if ($mday < 10) { $mday = "0$mday"; } if ($mon < 10) { $mon = "0$mon"; } if ($sec < 10) { $sec = "0$sec"; } if ( $year < 1900 ) { $year = $year+1900; } #fix all t +hose y2k bugs return "$year-$mon-$mday-$hour-$min-$sec"; } ############### sub read_list { my ($list) = @_; print "Opening $list\n" if ($verbose); open(LIST,"$list") || die "Can't Open Email List File $list: $!\n" +; my @emails = <LIST>; close (LIST); return @emails; } ############# sub usage { print " mailfwd USERNAME HOST PASSWORD LIST\n\n"; exit; } #####
- dystrophy -

Replies are listed 'Best First'.
Re: mail::sendmail with an array of hashes
by eg (Friar) on Feb 04, 2001 at 13:01 UTC

    The problem is that you're calling sendmail with an hash ref, not a hash.

    if ( sendmail $tank[$i] ){}

    should be

    if ( sendmail %{$tank[$i]} ){}

    It looks like you're using Data::Dumper to debug. Good. Notice how there're braces (curly brackets) around the data when you dump $tank[$i]? That means it's a hash ref. Actually since you're not doing anything with the if, using unless would be cleaner.

    print $Mail::Sendmail::error unless ( sendmail %{$tank[$i]} );

    See perldata and perlsyn.

    p.s. If you try to cut down the amount of code you post to a bare minimum that still exhibits your problem, you'll get much better response here. Plus, by isolating the problem, you give yourself a better chance of fixing it yourself!

      Thanks for the reply. (Boy do I feel sheepish) I was going to post just the bit I was having problems with, but I decided to include it all and ask for input...

      I really appreciate all the monks who took the time to read the code. Thank you for all your input on things like the mime header, Mail::Sendmail Bcc:, $", local and more.

      - dystrophy -
(sacked: improvement- use local) Re: mail::sendmail with an array of hashes
by sacked (Hermit) on Feb 04, 2001 at 16:01 UTC
    This chunk of code:
    # change normal array separator my $foo = $"; $" = "\n"; print FH "@head"; $" = $foo; # change back
    could be replaced with:
    { local $" = "\n"; print FH "@head"; }
    Using local removes the need for a temporary variable, as the saving and restoring of the previous value of the global $" variable is taken care of for you.

    See local, the "Temporary values via local()" and "When to still use local()" sections of perlman:perlsub, and for a more detailed explanation and examples, see brother Dominus's Seven Useful Uses of local.

    -sacked
Re: Mail::Sendmail
by dkubb (Deacon) on Feb 05, 2001 at 11:06 UTC

    One rule I try to code by is to offload all the drugery to modules, so that I focus as much as possible on a higher level. The more you offload to modules, the shorter and clearer your code gets.

    On that note, a few of your subroutines could be replaced by CPAN modules, which will shorten your code and free your focus.

    Before I get into that, one question: Is it possible for you to filter and redirect the mail in real-time as they come in? If so, then you should look into Mail::Audit, which allows you to parse and filter mail pretty easily. Using this method should simplify the process significantly.

    Ok, back to the code. The first thing I would do is look at your getDate() subroutine. Check out Date::Format which will format the date in the exact specification you desire, with much less code to maintain.

    Next, you could replace the check_args() and usage() with Getopt::Declare or a combination of Getopt::Std and Pod::Usage. I prefer Getopt::Declare as it's argueably simpler to use and does what I want.

    You can replace parse_head() with some of the MailTools modules. For example, the output from the Mail::POP3Client methods will be perfect input into Mail::Header and Mail::Internet's new() constructors. From there you can just use method calls to get at the From, To, Subject and Body of the message. No more parsing of the email messages with regexs, it's all done for you, very reliably.

    Also, in your initial loop, you save a copy of the incoming message to a log file, I presume. The way you are opening up the file handle, using >, will clobber the file each time you write to it. My guess is that you need to use >>, which tells open() that you'd like to append to the file, not overwrite it.

    At first glance you're probably thinking "this is alot of stuff to learn". That's true, but I would argue that, at some point, learning CPAN modules is almost as important as learning the core perl commands. At first it seems like alot, but every time you use a new module, the code you got working serves as a reference for next time. After a short period of time you have your own personal reference library and the time to implement similar tasks drops considerably.

    Hope this helps.