I just spent a stupid amount of time trying to figure out why IpFire insisted a fixed lease had "invalid fixed ip address" when I could see nothing wrong with the address.
I had to read the source before I realized where the problem was: the same error message is also used when the *next-server* address is bad, and it doesn't accept server name there but wants an IP.
I guess this would not be the most critical thing to fix but at least changing the error message would be nice.
Hi,
On 14.09.2019 15:29, Tapani Tarvainen wrote:
I just spent a stupid amount of time trying to figure out why IpFire insisted a fixed lease had "invalid fixed ip address" when I could see nothing wrong with the address.
I had to read the source before I realized where the problem was: the same error message is also used when the *next-server* address is bad, and it doesn't accept server name there but wants an IP.
To be sure: do you mean this if-clause in 'dhcp.cgi'?
... if ($dhcpsettings{'FIX_NEXTADDR'}) { unless(&General::validip($dhcpsettings{'FIX_NEXTADDR'})) { $errormessage = $Lang::tr{'invalid fixed ip address'}; } } ...
I guess this would not be the most critical thing to fix but at least changing the error message would be nice.
Yep. Should be easy.
Adjusting 'dhcp.cgi' and the 'lang'-string for the errormessage should be sufficient. I just need your confirmation.
Would the errormessage 'invalid ip' => 'Invalid IP Address' be clear enough/sufficient?
Alternative: 'invalid next-server ip' => 'Invalid IP Address for next-server'
Best, Matthias
Hi,
Gesendet: Samstag, 14. September 2019 um 15:55 Uhr Von: "Matthias Fischer" matthias.fischer@ipfire.org An: development@lists.ipfire.org Betreff: Re: Confusing error message
Hi,
On 14.09.2019 15:29, Tapani Tarvainen wrote:
I just spent a stupid amount of time trying to figure out why IpFire insisted a fixed lease had "invalid fixed ip address" when I could see nothing wrong with the address.
I had to read the source before I realized where the problem was: the same error message is also used when the *next-server* address is bad, and it doesn't accept server name there but wants an IP.
To be sure: do you mean this if-clause in 'dhcp.cgi'?
... if ($dhcpsettings{'FIX_NEXTADDR'}) { unless(&General::validip($dhcpsettings{'FIX_NEXTADDR'})) { $errormessage = $Lang::tr{'invalid fixed ip address'}; } } ...
I guess this would not be the most critical thing to fix but at least changing the error message would be nice.
Yep. Should be easy.
Adjusting 'dhcp.cgi' and the 'lang'-string for the errormessage should be sufficient. I just need your confirmation.
Would the errormessage 'invalid ip' => 'Invalid IP Address' be clear enough/sufficient?
Alternative: 'invalid next-server ip' => 'Invalid IP Address for next-server'
I would prefer this version. The semantic "next-server must be a IP address" is expressed more clearly.
Best, Matthias
Best, Bernhard
On Sat, Sep 14, 2019 at 03:55:00PM +0200, Matthias Fischer (matthias.fischer@ipfire.org) wrote:
On 14.09.2019 15:29, Tapani Tarvainen wrote:
I just spent a stupid amount of time trying to figure out why IpFire insisted a fixed lease had "invalid fixed ip address" when I could see nothing wrong with the address.
I had to read the source before I realized where the problem was: the same error message is also used when the *next-server* address is bad, and it doesn't accept server name there but wants an IP.
To be sure: do you mean this if-clause in 'dhcp.cgi'?
... if ($dhcpsettings{'FIX_NEXTADDR'}) { unless(&General::validip($dhcpsettings{'FIX_NEXTADDR'})) { $errormessage = $Lang::tr{'invalid fixed ip address'}; } }
Exactly.
I guess this would not be the most critical thing to fix but at least changing the error message would be nice.
Yep. Should be easy.
Adjusting 'dhcp.cgi' and the 'lang'-string for the errormessage should be sufficient. I just need your confirmation.
Would the errormessage 'invalid ip' => 'Invalid IP Address' be clear enough/sufficient?
Alternative: 'invalid next-server ip' => 'Invalid IP Address for next-server'
The latter is much better, as it makes it instantly clear *where* the error lies.
Thank you,
Hi,
On 14.09.2019 16:56, Tapani Tarvainen wrote:
On Sat, Sep 14, 2019 at 03:55:00PM +0200, Matthias Fischer (matthias.fischer@ipfire.org) wrote:
On 14.09.2019 15:29, Tapani Tarvainen wrote:
I just spent a stupid amount of time trying to figure out why IpFire insisted a fixed lease had "invalid fixed ip address" when I could see nothing wrong with the address.
I had to read the source before I realized where the problem was: the same error message is also used when the *next-server* address is bad, and it doesn't accept server name there but wants an IP.
To be sure: do you mean this if-clause in 'dhcp.cgi'?
... if ($dhcpsettings{'FIX_NEXTADDR'}) { unless(&General::validip($dhcpsettings{'FIX_NEXTADDR'})) { $errormessage = $Lang::tr{'invalid fixed ip address'}; } }
Exactly.
I guess this would not be the most critical thing to fix but at least changing the error message would be nice.
Yep. Should be easy.
Adjusting 'dhcp.cgi' and the 'lang'-string for the errormessage should be sufficient. I just need your confirmation.
Would the errormessage 'invalid ip' => 'Invalid IP Address' be clear enough/sufficient?
Alternative: 'invalid next-server ip' => 'Invalid IP Address for next-server'
The latter is much better, as it makes it instantly clear *where* the error lies.
I tested with the update from above and found another check:
Adding the string "test" (without quotation marks) gives me:
Error messages next-server on GREEN: Invalid IP Address
This is because of this (line ~220):
... if ($dhcpsettings{"NEXT_${itf}"}) { if (!(&General::validip($dhcpsettings{"NEXT_${itf}"}))) { $errormessage = "next-server on ${itf}: " . $Lang::tr{'invalid ip'}; goto ERROR; } ...
Just for your information - we got *two* checks. ;-)
Best, Matthias
On Sep 14, 2019, at 12:31 PM, Matthias Fischer matthias.fischer@ipfire.org wrote:
[snip]
Just for your information - we got *two* checks. ;-)
Belt *and* suspenders!
-
Bernhard Bitsch -
Matthias Fischer -
Tapani Tarvainen -
Tom Rymes