Changes from v1: * refactored the validation function out of the parse method * the function returns an error message if it finds an invalid interface name, but None if it does not find an error - like this there is just a single raise statement instead of 4 * error messages read as sentences * hopefully more readable message for the invalid vlan.<vlan id> interface name * out of range error message reports the valid range * tests correctly inherit from F20 tests
Martin Kolman (2): Validate network interface name when parsing the kickstart (#1081982) Add tests for --interfacename validation
pykickstart/commands/network.py | 55 +++++++++++++++++++++++++++++++++++++++++ tests/commands/network.py | 28 +++++++++++++++++++++ 2 files changed, 83 insertions(+)
Resolves: rhbz#1081982 Signed-off-by: Martin Kolman mkolman@redhat.com --- pykickstart/commands/network.py | 55 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)
diff --git a/pykickstart/commands/network.py b/pykickstart/commands/network.py index 7b1341c..44ddb8d 100644 --- a/pykickstart/commands/network.py +++ b/pykickstart/commands/network.py @@ -517,9 +517,64 @@ class RHEL6_Network(F9_Network): op.add_option("--bondopts", dest="bondopts") return op
+def validate_network_interface_name(name): + """Check if the given network interface name is valid, return an error message + if an error is found or None if no errors are found + + :param str name: name to validate + :returns: error message or None if no error is found + :rtype: str or None + """ + # (for reference see the NetworkManager source code: + # NetworkManager/src/settings/plugins/ifcfg-rh/reader.c + # and the make_vlan_setting function) + + min_vlan_id = 0 + max_vlan_id = 4095 + vlan_id = None + + # if it contains '.', vlan id should follow (eg 'ens7.171', 'mydev.171') + split_name = name.split(".", 1) + if len(split_name) > 1: + # 'vlan' can't be followed by a '.' + if split_name[0] == "vlan": + return _("When using the <prefix>.<vlan id> interface name notation, <prefix> can't be equal to 'vlan'.") + + try: + vlan_id = int(split_name[1]) + except ValueError: + return _("If network --interfacename contains a '.', valid vlan id should follow.") + + # if it starts with 'vlan', vlan id should follow ('vlan171') + if name.startswith("vlan"): + try: + vlan_id = int(name.strip("vlan")) + except ValueError: + return _("If network --interfacename starts with 'vlan', valid vlan id should follow.") + + # check if the vlan id is in range + if vlan_id is not None: + if not(min_vlan_id <= vlan_id <= max_vlan_id): + return _("The vlan id out of the %d-%d vlan id range." % (min_vlan_id, max_vlan_id)) + + # network interface name seems to be valid (no error found) + return None + class RHEL7_Network(F20_Network): def _getParser(self): op = F20_Network._getParser(self) op.add_option("--interfacename", dest="interfacename", action="store", default="") return op + + def parse(self, args): + # call the overridden command to do it's job first + retval = F20_Network.parse(self, args) + + # validate the network interface name + error_message = validate_network_interface_name(retval.interfacename) + # something is wrong with the interface name + if error_message is not None: + raise KickstartValueError(formatErrorMsg(self.lineno,msg=error_message)) + + return retval
- # check if the vlan id is in range
- if vlan_id is not None:
if not(min_vlan_id <= vlan_id <= max_vlan_id):
return _("The vlan id out of the %d-%d vlan id range." % (min_vlan_id, max_vlan_id))
Everything looks fine, except this line should be:
return _("The vlan id is out of the %d-%d vlan id range.") % (min_vlan_id, max_vlan_id)
- Chris
----- Original Message -----
From: "Martin Kolman" mkolman@redhat.com To: anaconda-patches@lists.fedorahosted.org Sent: Monday, September 8, 2014 1:49:42 PM Subject: [PATCH 1/2] Validate network interface name when parsing the kickstart (#1081982)
Resolves: rhbz#1081982 Signed-off-by: Martin Kolman mkolman@redhat.com
pykickstart/commands/network.py | 55 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)
diff --git a/pykickstart/commands/network.py b/pykickstart/commands/network.py index 7b1341c..44ddb8d 100644 --- a/pykickstart/commands/network.py +++ b/pykickstart/commands/network.py @@ -517,9 +517,64 @@ class RHEL6_Network(F9_Network): op.add_option("--bondopts", dest="bondopts") return op
+def validate_network_interface_name(name):
- """Check if the given network interface name is valid, return an error
message
- if an error is found or None if no errors are found
Could you give a link here to the specification of the network interface name format or else specify that format here?
- :param str name: name to validate
- :returns: error message or None if no error is found
- :rtype: str or None
This is very pedantic, but None is a value and _NoneType_ is its type.
- """
- # (for reference see the NetworkManager source code:
- # NetworkManager/src/settings/plugins/ifcfg-rh/reader.c
- # and the make_vlan_setting function)
- min_vlan_id = 0
- max_vlan_id = 4095
The above are constants, so should be capitalized.
- vlan_id = None
For the condition below, partition is probably better:
(prefix, sep, vlan_id) = name.partition(".") if sep: if prefix == "vlan": return ... try: vlan_id = int(vlan_id) except ...: ...
That way, you have to do less indexing and you don't have to worry about len and max splits for split() function.
- # if it contains '.', vlan id should follow (eg 'ens7.171', 'mydev.171')
- split_name = name.split(".", 1)
- if len(split_name) > 1:
# 'vlan' can't be followed by a '.'
if split_name[0] == "vlan":
return _("When using the <prefix>.<vlan id> interface name
notation, <prefix> can't be equal to 'vlan'.")
try:
vlan_id = int(split_name[1])
except ValueError:
return _("If network --interfacename contains a '.', valid vlan
id should follow.")
Is vlang.123 an acceptable id? If so, then the block below should be an elif. If not, then the block below should probably be the first block, so that vlang.123 gets thrown out at first check. Right now it gets accepted by first block but rejected by second block.
Should use partition below as well as strip() is not really what you want.
Something like:
(empty, sep, vlan_id) = name.partition("vlan") if empty == "": try: vlan_id = int(vlan_id) except .... ....
That way you avoid accepting "vlan123lvan" by bad coincidence.
- # if it starts with 'vlan', vlan id should follow ('vlan171')
- if name.startswith("vlan"):
try:
vlan_id = int(name.strip("vlan"))
except ValueError:
return _("If network --interfacename starts with 'vlan', valid
vlan id should follow.")
- # check if the vlan id is in range
- if vlan_id is not None:
if not(min_vlan_id <= vlan_id <= max_vlan_id):
return _("The vlan id out of the %d-%d vlan id range." %
(min_vlan_id, max_vlan_id))
- # network interface name seems to be valid (no error found)
- return None
class RHEL7_Network(F20_Network): def _getParser(self): op = F20_Network._getParser(self) op.add_option("--interfacename", dest="interfacename", action="store", default="") return op
- def parse(self, args):
# call the overridden command to do it's job first
retval = F20_Network.parse(self, args)
# validate the network interface name
error_message =
validate_network_interface_name(retval.interfacename)
# something is wrong with the interface name
if error_message is not None:
raise
KickstartValueError(formatErrorMsg(self.lineno,msg=error_message))
return retval
-- 1.9.3
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
Thanks for writing the tests to go along with, they make it a lot easier for me to think about the code.
- mulhern
----- Original Message -----
From: "Anne Mulhern" amulhern@redhat.com To: "anaconda patch review" anaconda-patches@lists.fedorahosted.org Sent: Monday, September 8, 2014 5:35:37 PM Subject: Re: [PATCH 1/2] Validate network interface name when parsing the kickstart (#1081982)
----- Original Message -----
From: "Martin Kolman" mkolman@redhat.com To: anaconda-patches@lists.fedorahosted.org Sent: Monday, September 8, 2014 1:49:42 PM Subject: [PATCH 1/2] Validate network interface name when parsing the kickstart (#1081982)
Resolves: rhbz#1081982 Signed-off-by: Martin Kolman mkolman@redhat.com
pykickstart/commands/network.py | 55 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)
diff --git a/pykickstart/commands/network.py b/pykickstart/commands/network.py index 7b1341c..44ddb8d 100644 --- a/pykickstart/commands/network.py +++ b/pykickstart/commands/network.py @@ -517,9 +517,64 @@ class RHEL6_Network(F9_Network): op.add_option("--bondopts", dest="bondopts") return op
+def validate_network_interface_name(name):
- """Check if the given network interface name is valid, return an error
message
- if an error is found or None if no errors are found
Could you give a link here to the specification of the network interface name format or else specify that format here?
- :param str name: name to validate
- :returns: error message or None if no error is found
- :rtype: str or None
This is very pedantic, but None is a value and _NoneType_ is its type.
- """
- # (for reference see the NetworkManager source code:
- # NetworkManager/src/settings/plugins/ifcfg-rh/reader.c
- # and the make_vlan_setting function)
- min_vlan_id = 0
- max_vlan_id = 4095
The above are constants, so should be capitalized.
- vlan_id = None
For the condition below, partition is probably better:
(prefix, sep, vlan_id) = name.partition(".") if sep: if prefix == "vlan": return ... try: vlan_id = int(vlan_id) except ...: ...
That way, you have to do less indexing and you don't have to worry about len and max splits for split() function.
Except that, whoops, you don't want to set vlan_id in result of partition, so code should actually be something like (with one further simplification):
(vlan, _dot, id_candidate) = name.partition(".") if vlan == "vlan": vlan_id = int(id_candidate) etc.
- # if it contains '.', vlan id should follow (eg 'ens7.171',
'mydev.171')
- split_name = name.split(".", 1)
- if len(split_name) > 1:
# 'vlan' can't be followed by a '.'
if split_name[0] == "vlan":
return _("When using the <prefix>.<vlan id> interface name
notation, <prefix> can't be equal to 'vlan'.")
try:
vlan_id = int(split_name[1])
except ValueError:
return _("If network --interfacename contains a '.', valid
vlan id should follow.")
Is vlang.123 an acceptable id? If so, then the block below should be an elif. If not, then the block below should probably be the first block, so that vlang.123 gets thrown out at first check. Right now it gets accepted by first block but rejected by second block.
Should use partition below as well as strip() is not really what you want.
Something like:
(empty, sep, vlan_id) = name.partition("vlan") if empty == "": try: vlan_id = int(vlan_id) except .... ....
That way you avoid accepting "vlan123lvan" by bad coincidence.
Don't want to set vlan_id in result of partition here, either, so need to make the same change as above.
Also, I was probably carried away by my enthusiasm for partition() here, probably
(vlan, id_candidate) = (name[:4], name[4:]) if vlan == "vlan": try: vlan_id = int(id_candidate) etc.
would be just as good, maybe better.
- # if it starts with 'vlan', vlan id should follow ('vlan171')
- if name.startswith("vlan"):
try:
vlan_id = int(name.strip("vlan"))
except ValueError:
return _("If network --interfacename starts with 'vlan', valid
vlan id should follow.")
- # check if the vlan id is in range
- if vlan_id is not None:
if not(min_vlan_id <= vlan_id <= max_vlan_id):
return _("The vlan id out of the %d-%d vlan id range." %
(min_vlan_id, max_vlan_id))
Also, I think this code will perform incorrectly on the string "meh".
"meh" does not have a '.' so it does not meet first condition and it does not start with "vlan", so it doesn't meet the second condition. This means that vlan_id is None and we just end up returning None, which is wrong.
So, what we need above is something like:
if vlan_id is None: return <some appropriate error message> else: # check if the vlan is is in range etc.
- # network interface name seems to be valid (no error found)
- return None
class RHEL7_Network(F20_Network): def _getParser(self): op = F20_Network._getParser(self) op.add_option("--interfacename", dest="interfacename", action="store", default="") return op
- def parse(self, args):
# call the overridden command to do it's job first
retval = F20_Network.parse(self, args)
# validate the network interface name
error_message =
validate_network_interface_name(retval.interfacename)
# something is wrong with the interface name
if error_message is not None:
raise
KickstartValueError(formatErrorMsg(self.lineno,msg=error_message))
return retval
-- 1.9.3
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
Thanks for writing the tests to go along with, they make it a lot easier for me to think about the code.
- mulhern
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
- mulhern
Related: rhbz#1081982 Signed-off-by: Martin Kolman mkolman@redhat.com --- tests/commands/network.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)
diff --git a/tests/commands/network.py b/tests/commands/network.py index 9643c74..a978a19 100644 --- a/tests/commands/network.py +++ b/tests/commands/network.py @@ -69,5 +69,33 @@ class F20_TestCase(CommandTest): nd2 = self.assert_parse(s) self.assertEquals(value, nd2.teamslaves)
+class RHEL7_TestCase(F20_TestCase): + def runTest(self): + F20_TestCase.runTest(self) + # there needs to be a vlan id after a dot & only one dot is allowed + self.assert_parse_error("network --interfacename=abc.", KickstartValueError) + self.assert_parse_error("network --interfacename=abc.def", KickstartValueError) + self.assert_parse_error("network --interfacename=abc..", KickstartValueError) + self.assert_parse_error("network --interfacename=abc.123.456", KickstartValueError) + # 'vlan' can't be followed by a '.' + self.assert_parse_error("network --interfacename=vlan.123", KickstartValueError) + self.assert_parse_error("network --interfacename=vlan.", KickstartValueError) + self.assert_parse_error("network --interfacename=vlan..", KickstartValueError) + self.assert_parse_error("network --interfacename=vlan.abc", KickstartValueError) + self.assert_parse("network --interfacename=abc.123") + + # if the device name begins with 'vlan', vlan id needs to follow + self.assert_parse_error("network --interfacename=vlan", KickstartValueError) + self.assert_parse_error("network --interfacename=vlanabcd", KickstartValueError) + self.assert_parse("network --interfacename=vlan123") + + # vlan ids go from 0 to 4095 + self.assert_parse("network --interfacename=vlan0") + self.assert_parse("network --interfacename=vlan4095") + self.assert_parse("network --interfacename=abc.0") + self.assert_parse("network --interfacename=abc.4095") + self.assert_parse_error("network --interfacename=vlan9001", KickstartValueError) + self.assert_parse_error("network --interfacename=abc.9001", KickstartValueError) + if __name__ == "__main__": unittest.main()
anaconda-patches@lists.fedorahosted.org