So i'm gonna add the
Tested-by: Timo Eissler timo.eissler@ipfire.org
Tag also, as i'm reviewed the code and tested it.
Am 01.11.2015 um 17:07 schrieb Michael Tremer:
On Sun, 2015-11-01 at 11:10 +0100, Timo Eissler wrote:
Hi,
sorry for the late answer...
Atleast for me this patch works perfectly.
Reviewed-by: Timo Eissler timo.eissler@ipfire.org
You should use the Tested-by: tag than instead of Reviewed-by: because you didn't review the code.
Thank you Alex!
-Michael
Am 01.11.2015 um 06:50 schrieb Alexander Marx:
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. 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" The empty string ("") 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; > close TXT2; 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
-- Timo Eissler Senior Project Engineer / Consultant
Am Zuckerberg 54 D-71640 Ludwigsburg
Tel.: +49 7141 4094003 Mobil.: +49 151 20650311 Email: timo@teissler.de