On Sun, 2015-11-01 at 06:50 +0100, Alexander Marx wrote:
On Sat, 2015-10-31 at 20:21 +0100, Alexander Marx wrote:
Hi,
I think there is a problem with this patch.
On Sat, 2015-10-31 at 07:34 +0100, Alexander Marx wrote:
auth.conf was always written, even if no username/password provided. In this case only the ip or Hostname of the mailserver was written into auth.conf. Now the file is only filled if username/password are filled.
Signed-off-by: Alexander Marx alexander.marx@ipfire.org
html/cgi-bin/mail.cgi | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/html/cgi-bin/mail.cgi b/html/cgi-bin/mail.cgi index be663a6..f5be104 100755 --- a/html/cgi-bin/mail.cgi +++ b/html/cgi-bin/mail.cgi @@ -110,9 +110,12 @@ if ($cgiparams{'ACTION'} eq "$Lang::tr{'save'}"){ #SaveButton on configsite $mail{'SENDER'} = $cgiparams{'txt_mailsender'}; $mail{'RECIPIENT'} = $cgiparams{'txt_recipient'};
$auth{'AUTHNAME'} =$cgiparams{'txt_mailuser'};
$auth{'AUTHPASS'} =$cgiparams{'txt_mailpass'};
$auth{'AUTHHOST'} =$cgiparams{'txt_mailserver'};
if ($cgiparams{'txt_mailuser'} &&$cgiparams{'txt_mailpass'}) {
$auth{'AUTHNAME'} =$cgiparams{'txt_mailuser'};
$auth{'AUTHPASS'} =$cgiparams{'txt_mailpass'};
$auth{'AUTHHOST'} =$cgiparams{'txt_mailserver'};
print TXT1"$auth{'AUTHNAME'}|$auth{'AUTHHOST'}:$auth{'AUTHPASS'}\n";
}My email client made a right mess out of this code above, but maybe you check with the original one... So if the user wants to clear the username and password and then hits save I guess these changes won't be saved. They should though.
Can you confirm this? I think the solution should keep the first three lines that are now in the if-clause before that and also clear auth.conf when the configuration was cleared.
Unfortunately not. With the flow of the code you should see, that auth.conf is always opened for writing (overwrite mode). If a username and password is set, the file is written due to the if clause. Otherwise the file is just closed wich leads to an empty file.
OK.
In my tests, the system did exactly that. I tested a config without user/pass, and clicked "save". Then auth.conf is empty. Next step was to fill username/password and click "save" again. Final step: remove username/pass and again save file -> empty auth.conf.
I don't know what perl is then actually checking when you have a statement like this:
if ($cgiparams{'ABC'}) { ... }
Does it check for a non-empty string? Obviously not. Does it check if that value is defined? Apparently not.
I would like to see more readable code if that is possible though. This is nothing that is connected to this patch alone, it seems to be a general problem with our perl code here. It works, but it is not clear to *everyone* why.
OK i agree with this. Will try to be more exact in future.
This is not necessarily an issue with your patch. It is also a bit related to perl - or that perl-programmers don't *actually* know what their code does.
From http://www.perlmonks.org/?node_id=598879 you will see, that there are several ways of perl ;-) to check if a variable contains something useful. Here is a statement:
Perl tries to make things as easy as possible for you and this is a good example. In most cases, what you actually want is as simple as: if ($value) { # $value contains something useful } else { # $value is "empty" } [download] (Note that the logic is reversed from your original example) What we're doing here is checking the "truth" of $value. $value will be "false" if it contains any of the following values: "undef" (i.e. if it hasn't been given a value) The number 0 The string "0"
That at least should be treated as a valid string. It is unlikely that this is a username or password, but still this is something that needs to be handled.
The empty string ("")
If that was true, you won't be able to clear the form as I mentioned before.
So I guess this must be changed then.
Any other value in $value will evaluate to "true". This is all you need in most cases. There are a couple of "corner
cases" that you sometimes have to consider:
Sometimes 0 is an acceptable value. You account for that by using if ($value or $value == 0). Sonetimes you might want to test that $value contains nothing but whitespace characters. The easiest way to do that is by checking for the presence of non-whitespace characters with if ($value =~ /\S/).
Maybe someone else is able to test this?! Timo seems to be a good tester, he found the bug ;-)
But i will double check and give feedback if i was wrong, but this can take 1-2 days
$dma{'SMARTHOST'} =$cgiparams{'txt_mailserver'}; $dma{'PORT'} = $cgiparams{'txt_mailport'}; @@ -129,7 +132,7 @@ if ($cgiparams{'ACTION'} eq "$Lang::tr{'save'}"){ #SaveButton on configsite print TXT "$k $v\n"; } close TXT;
print TXT1"$auth{'AUTHNAME'}|$auth{'AUTHHOST'}:$auth{'AUTHPASS'}\n";
close TXT1;On a side note: It would probably have helped a bit to name the file handles better. Like AUTHFILE or so instead of numbering them.
}else{
-Michael
-Michael
-Michael