[...] >> I do not know how to set the sender mail address when I use the mail >> command. Currently debarchiver fails to set it. > > Doesn't this work? > $mailcmd -s '$package $key' '$to' -- -f '$mailfrom' > > It was in the code before you changed it. That is why I have this check > about from address.
No, I am not able to make it work either with the current version of
debarchiver or from the command line. I get an email whose to header is
malformed and I also get an error from the mailer-daemon.
[...]
> Now to the comments. The comments are below each patch section.
>
> diff --git a/src/debarchiver.pl b/src/debarchiver.pl
> index 2eaa9bd..ba6ef0e 100755
> --- a/src/debarchiver.pl
> +++ b/src/debarchiver.pl
> @@ -72,9 +72,13 @@ use OpaL::read qw(readfile readcommand);
> # 2007-10-09 Ola Lundqvist <o...@inguza.com>
> # Changed force option to ignoredestcheck option.
>
> +my $use_sendmail = 0;
> +my %cmds = ();
> +$cmds{'sendmail'} = 'sendmail';
> +$cmds{'mail'} = 'mail';
> +
> $copycmd = "cp -af";
> $rmcmd = "rm -f";
> -$mailcmd = "mail";
> $movecmd = "mv";
> $vrfycmd = "dscverify";
> $cachedir = "/var/cache/debarchiver";
>
> I think we should rename this %cmds hash to %mailcmds. But also see
> the comments below. I assume order is not defined, right?
As a matter of fact, maybe other system commands such as dscverify,
mv... could be added to the hash.
Then, by a simple call to check_commands, all of them could be checked
and an error could be raise if there is a problem.
The hash does not assume order.
It works this way:
cmds{$mycmd} = $mycmdwithfullpath
if $mycmdwithfullpath is not a full path but only the command name, the
check_commands function will try to find it in a set of common directories.
> @@ -456,9 +460,19 @@ while ($_ = shift @ARGS2) {
> elsif (/^--movecmd$/) {
> $movecmd = shift @ARGS2;
> }
> - elsif (/^--mailcmd$/) {
> - $mailcmd = shift @ARGS2;
> - }
> + elsif (/^--mailcmd$/) {
> + my $usercmd = shift @ARGS2;
> + if ($usercmd =~ m|^(.*\/)+(.*)|) {
> + $cmds{$2} = $usercmd;
> + $usercmd = $2;
> +
> + } else {
> + $cmds{$usercmd} = $usercmd;
> + }
> + &check_commands({$usercmd => ''}, {});
> + $cmds{'mail'} = $cmds{$usercmd};
> + delete $cmds{'sendmail'};
> + }
> elsif (/^--mailfrom$/) {
> $mailfrom = shift @ARGS2;
> }
>
> The second question is about this. What does it do exactly.
> If I would write
> --mailcmd /usr/local/bin/special_mailcommand_opal
>
> Wouldn't that give quite strange results?
If I have not made any mistake, it should only try to find
special_mailcommand_opal in /usr/local/bin/. If it does not exist, it
will try to find it in the set of common directories. If still no luck,
debarchiver should raise an error and stop.
Otherwise:
* the cmds{'mail'} will be overwritten by
/usr/local/bin/special_mailcommand_opal.
* the sendmail reference will be removed from the hash
=> debarchiver will send emails using the same format as the mail comand
for the special_mailcommand_opal command.
> I think it may be better to have a configuration option that
> tell whether it is sendmail format for the mail output or if it
> is the mail format. The automatic detect code is good and can be
> used to set the default.
That sounds better. Something like --mailformat (sendmail|mail) with a
default behavior ?
[...]
> +
> + } else {
> +
> + if (open(M, "| $cmds{'mail'} -s '$subject' '$toAddress'")) {
> + print M $msg . "\n";
> + close(M);
> +
> + } else {
> + pdebug(2, "Could not execute $cmds{'mail'} $!");
> + }
> +
> + }
> +
> + pdebug(5, "Mail exec done.");
> }
>
>
> ###############################################################################
>
>
> Here you missed the set of from address... Apart from that it looks good.
As mentionned above I have not been able to make it work :)
[...]
> +sub check_commands() {
> + my ($include_hr, $exclude_hr) = @_;
> +
> + my @path = qw(
> + /bin
> + /sbin
> + /usr/bin
> + /usr/sbin
> + /usr/local/bin
> + /usr/local/sbin
> + );
> +
> + for my $cmd (keys %cmds) {
>
> Ahh ok here is the reason why this is a hash instead of variables. I see.
> However this way order is not guaranteed... or?
Yes, no order guaranteed.
> +
> + if (keys %$include_hr) {
> + next unless defined $include_hr->{$cmd};
> + }
> + if (keys %$exclude_hr) {
> + next if defined $exclude_hr->{$cmd};
> + }
> +
> + unless (-x $cmds{$cmd}) {
> +
> + my $found = 0;
> + pdebug(3, "$cmd not located/executable at $cmds{$cmd}\n");
>
> This almost always give a warning. Is that intentional?
Should be an info.
> + PATH: for my $dir (@path) {
> + if (-x "${dir}/${cmd}") {
> + $cmds{$cmd} = "${dir}/${cmd}";
> + $found = 1;
> + last PATH;
> + }
> + }
> +
> + if ($found) {
> + pdebug(3,"Found $cmd at $cmds{$cmd}\n");
>
> I think it should be level 4 (info) instead of 3 (warning), right?
You are right.
[...]
Regards,
--
Franck Joncourt
http://debian.org - http://smhteam.info/wiki/
signature.asc
Description: OpenPGP digital signature
