[PATCH 1/2] Validate network interface name when parsing the kickstart (#1081982)

Anne Mulhern amulhern at redhat.com
Mon Sep 8 21:35:37 UTC 2014





----- Original Message -----
> From: "Martin Kolman" <mkolman at redhat.com>
> To: anaconda-patches at 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 at 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 at 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


More information about the anaconda-patches mailing list