[pykickstart][rhel7-branch/master][PATCH 1/2] Validate network interface name when parsing the kickstart (#1081982)

Martin Kolman mkolman at redhat.com
Mon Sep 8 17:10:11 UTC 2014


On Mon, 2014-09-08 at 12:06 -0400, Chris Lumens wrote:
> > Resolves: rhbz#1081982
> 
> If this patch resolves this bug, the bug should be reassigned to avoid
> process nightmares.
Done!
> 
> > +        # if it contains '.', vlan id should follow (eg 'ens7.171', 'mydev.171')
> > +        split_name = retval.interfacename.split(".", 1)
> > +        if len(split_name) > 1:
> > +            # 'vlan' can't be followed by a '.'
> > +            if split_name[0] == "vlan":
> > +                raise KickstartValueError(formatErrorMsg(self.lineno,
> > +                    msg=_("for network --interfacename 'vlan' can't be followed by a '.'")))
> 
> I don't really understand this error message.  Could it be made more
> clear?
After studying the related NetworkManager source code, there are two
ways of how to include a vlan id in a network interface name:
* use the vlan prefix, followed by the vlan number:
"vlan123"
* use a prefix followed by a dot ('.') and then the vlan number, with
the condition that the prefix can't equal to "vlan"

So what about:
"When using the <prefix>.<vlan id> interface name notation <prefix>
can't be equal to 'vlan'."

Suggestions for an even more understandable error message are
welcome! :)

> 
> > +            try:
> > +                vlan_id = int(split_name[1])
> > +            except ValueError:
> > +                raise KickstartValueError(formatErrorMsg(self.lineno,
> > +                    msg=_("if network --interfacename contains a '.', valid vlan id should follow")))
> > +
> > +        # if it starts with 'vlan', vlan id should follow ('vlan171')
> > +        if retval.interfacename.startswith("vlan"):
> > +            try:
> > +                vlan_id = int(retval.interfacename.strip("vlan"))
> > +            except ValueError:
> > +                raise KickstartValueError(formatErrorMsg(self.lineno,
> > +                    msg=_("if network --interfacename starts with 'vlan', valid vlan id should follow")))
> 
> I'd like these two error messages to look more like sentences -
> capitalize the If, put a period at the end.
OK
> 
> > +        # check if the vlan id is in range
> > +        if vlan_id is not None:
> > +            if not(min_vlan_id <= vlan_id <= max_vlan_id):
> > +                raise KickstartValueError(formatErrorMsg(self.lineno,
> > +                        msg=_("vlan id out of range")))
> > +
> > +        return retval
> 
> It'd help the user if you said what the valid range is.
Sure! :)
> 
> Also this should probably all be in its own method.  That way if some
> later version of the network command needs to override the parse method,
> it doesn't also need to duplicate all the work you've done here.
Doing just that.
> 
> - Chris
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches




More information about the anaconda-patches mailing list