Vratislav Podzimek (2): Improve and document network.sanityCheckHostname Check if the given NTP server is a valid hostname (#865869)
pyanaconda/network.py | 37 +++++++++++++++++++++++------- pyanaconda/ui/gui/spokes/datetime_spoke.py | 18 +++++++++++++++ 2 files changed, 47 insertions(+), 8 deletions(-)
The rules for hostname are more strict. Also, it was not easy to use this function because of its return values.
Related: rhbz#865869
Signed-off-by: Vratislav Podzimek vpodzime@redhat.com --- pyanaconda/network.py | 37 +++++++++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 8 deletions(-)
diff --git a/pyanaconda/network.py b/pyanaconda/network.py index 640cd70..1cc735d 100644 --- a/pyanaconda/network.py +++ b/pyanaconda/network.py @@ -35,6 +35,7 @@ import time import dbus import tempfile import simpleconfig +import re from flags import flags from simpleconfig import IfcfgFile from pyanaconda.constants import ROOT_PATH @@ -54,6 +55,10 @@ ipv6ConfFile = "/etc/modprobe.d/ipv6.conf" ifcfgLogFile = "/tmp/ifcfg.log" CONNECTION_TIMEOUT = 45
+# part of a valid hostname between two periods (cannot start nor end with '-') +# for more info about '(?!-)' and '(?<!-)' see 're' module documentation +HOSTNAME_PART_RE = re.compile("(?!-)[A-Z\d-]{1,63}(?<!-)$", re.IGNORECASE) + # Setup special logging for ifcfg NM interface from pyanaconda import anaconda_log logger = logging.getLogger("ifcfg") @@ -75,24 +80,40 @@ class IPMissing(Exception): pass
def sanityCheckHostname(hostname): + """ + Check if the given string is (syntactically) a valid hostname. + + @param hostname: a string to check + @returns: a pair containing boolean value (valid or invalid) and + an error message (if applicable) + @rtype: (bool, str) + + """ + if not hostname: - return None + return (False, _("Hostname cannot be None or an empty string."))
if len(hostname) > 255: - return _("Hostname must be 255 or fewer characters in length.") + return (False, _("Hostname must be 255 or fewer characters in length."))
validStart = string.ascii_letters + string.digits validAll = validStart + ".-"
if hostname[0] not in validStart: - return _("Hostname must start with a valid character in the ranges " - "'a-z', 'A-Z', or '0-9'") + return (False, _("Hostname must start with a valid character in the " + "ranges 'a-z', 'A-Z', or '0-9'"))
- for char in hostname[1:]: - if char not in validAll: - return _("Hostnames can only contain the characters 'a-z', 'A-Z', '0-9', '-', or '.'") + if hostname.endswith("."): + # hostname can end with '.', but the regexp used below would not match + hostname = hostname[:-1]
- return None + if any(not HOSTNAME_PART_RE.match(part) for part in hostname.split(".")): + return (False, _("Hostnames can only contain the characters 'a-z', " + "'A-Z', '0-9', '-', or '.', and parts between periods " + "must contain something and cannot start or end with " + "'-'.")) + + return (True, "")
# Return a list of IP addresses for all active devices. def getIPs():
On Thu, Nov 29, 2012 at 03:30:43PM +0100, Vratislav Podzimek wrote:
The rules for hostname are more strict. Also, it was not easy to use this function because of its return values.
Related: rhbz#865869
Signed-off-by: Vratislav Podzimek vpodzime@redhat.com
pyanaconda/network.py | 37 +++++++++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 8 deletions(-)
- return None
- if any(not HOSTNAME_PART_RE.match(part) for part in hostname.split(".")):
return (False, _("Hostnames can only contain the characters 'a-z', "
"'A-Z', '0-9', '-', or '.', and parts between periods "
"must contain something and cannot start or end with "
"'-'."))
I think 'not all(HOSTNAME_PART_RE...)' would be easier to read. You want all of the parts to match so if they don't all match return an error.
Other than that, looks good.
On Thu, 2012-11-29 at 14:23 -0800, Brian C. Lane wrote:
On Thu, Nov 29, 2012 at 03:30:43PM +0100, Vratislav Podzimek wrote:
The rules for hostname are more strict. Also, it was not easy to use this function because of its return values.
Related: rhbz#865869
Signed-off-by: Vratislav Podzimek vpodzime@redhat.com
pyanaconda/network.py | 37 +++++++++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 8 deletions(-)
- return None
- if any(not HOSTNAME_PART_RE.match(part) for part in hostname.split(".")):
return (False, _("Hostnames can only contain the characters 'a-z', "
"'A-Z', '0-9', '-', or '.', and parts between periods "
"must contain something and cannot start or end with "
"'-'."))
I think 'not all(HOSTNAME_PART_RE...)' would be easier to read. You want all of the parts to match so if they don't all match return an error.
I was deciding between these two versions and ended up with the 'any(not...)'. I'll change it locally before pushing.
Signed-off-by: Vratislav Podzimek vpodzime@redhat.com --- pyanaconda/ui/gui/spokes/datetime_spoke.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/pyanaconda/ui/gui/spokes/datetime_spoke.py b/pyanaconda/ui/gui/spokes/datetime_spoke.py index e44c864..2ac6ee6 100644 --- a/pyanaconda/ui/gui/spokes/datetime_spoke.py +++ b/pyanaconda/ui/gui/spokes/datetime_spoke.py @@ -220,6 +220,19 @@ class NTPconfigDialog(GUIObject): self._threads_counter += 1
def _add_server(self, server): + """ + Checks if a given server is a valid hostname and if yes, adds it + to the list of servers. + + @param server: string containing hostname + + """ + + (valid, error) = network.sanityCheckHostname(server) + if not valid: + log.error("'%s' is not a valid hostname" % server) + return + for row in self._serversStore: if row[0] == server: #do not add duplicate items @@ -251,6 +264,11 @@ class NTPconfigDialog(GUIObject): if not path: return
+ (valid, error) = network.sanityCheckHostname(new_text) + if not valid: + log.error("'%s' is not a valid hostname" % new_text) + return + itr = self._serversStore.get_iter(path)
if self._serversStore[itr][0] == new_text:
On Thu, Nov 29, 2012 at 03:30:44PM +0100, Vratislav Podzimek wrote:
Signed-off-by: Vratislav Podzimek vpodzime@redhat.com
pyanaconda/ui/gui/spokes/datetime_spoke.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/pyanaconda/ui/gui/spokes/datetime_spoke.py b/pyanaconda/ui/gui/spokes/datetime_spoke.py index e44c864..2ac6ee6 100644 --- a/pyanaconda/ui/gui/spokes/datetime_spoke.py +++ b/pyanaconda/ui/gui/spokes/datetime_spoke.py @@ -220,6 +220,19 @@ class NTPconfigDialog(GUIObject): self._threads_counter += 1
def _add_server(self, server):
"""
Checks if a given server is a valid hostname and if yes, adds it
to the list of servers.
@param server: string containing hostname
"""
(valid, error) = network.sanityCheckHostname(server)
if not valid:
log.error("'%s' is not a valid hostname" % server)
return
for row in self._serversStore: if row[0] == server: #do not add duplicate items
@@ -251,6 +264,11 @@ class NTPconfigDialog(GUIObject): if not path: return
(valid, error) = network.sanityCheckHostname(new_text)
if not valid:
log.error("'%s' is not a valid hostname" % new_text)
return
itr = self._serversStore.get_iter(path) if self._serversStore[itr][0] == new_text:
-- 1.7.11.7
You may as well log the error string returned by the function, might be helpful in some cases.
On Thu, 2012-11-29 at 14:25 -0800, Brian C. Lane wrote:
On Thu, Nov 29, 2012 at 03:30:44PM +0100, Vratislav Podzimek wrote:
Signed-off-by: Vratislav Podzimek vpodzime@redhat.com
pyanaconda/ui/gui/spokes/datetime_spoke.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/pyanaconda/ui/gui/spokes/datetime_spoke.py b/pyanaconda/ui/gui/spokes/datetime_spoke.py index e44c864..2ac6ee6 100644 --- a/pyanaconda/ui/gui/spokes/datetime_spoke.py +++ b/pyanaconda/ui/gui/spokes/datetime_spoke.py @@ -220,6 +220,19 @@ class NTPconfigDialog(GUIObject): self._threads_counter += 1
def _add_server(self, server):
"""
Checks if a given server is a valid hostname and if yes, adds it
to the list of servers.
@param server: string containing hostname
"""
(valid, error) = network.sanityCheckHostname(server)
if not valid:
log.error("'%s' is not a valid hostname" % server)
return
for row in self._serversStore: if row[0] == server: #do not add duplicate items
@@ -251,6 +264,11 @@ class NTPconfigDialog(GUIObject): if not path: return
(valid, error) = network.sanityCheckHostname(new_text)
if not valid:
log.error("'%s' is not a valid hostname" % new_text)
return
itr = self._serversStore.get_iter(path) if self._serversStore[itr][0] == new_text:
-- 1.7.11.7
You may as well log the error string returned by the function, might be helpful in some cases.
I was thinking about that too, but the problem is (or isn't it?) that these error strings are translated. I believe they still might be helpful, but I'm not sure we want translated messages in logs.
anaconda-patches@lists.fedorahosted.org