----- 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