[f21/master v2] Avoid the possibility of size variables being unset (#1146585)

Anne Mulhern amulhern at redhat.com
Thu Sep 25 19:33:27 UTC 2014





----- Original Message -----
> From: "David Shea" <dshea at redhat.com>
> To: anaconda-patches at lists.fedorahosted.org
> Sent: Thursday, September 25, 2014 3:19:46 PM
> Subject: Re: [f21/master v2] Avoid the possibility of size variables being	unset	(#1146585)
> 
> On 09/25/2014 02:35 PM, Anne Mulhern wrote:
> >
> >
> >
> > ----- Original Message -----
> >> From: "David Shea" <dshea at redhat.com>
> >> To: anaconda-patches at lists.fedorahosted.org
> >> Sent: Thursday, September 25, 2014 1:34:05 PM
> >> Subject: [f21/master v2] Avoid the possibility of size variables being
> >> unset	(#1146585)
> >>
> >> Raise an error if part and logvol sizes and maxsizes cannot be
> >> converted to integers. Treat a missing size in logvol as an error, and a
> >> missing size in part as a request for a default size to be determined by
> >> blivet.
> >> ---
> >>   pyanaconda/kickstart.py | 28 ++++++++++++----------------
> >>   1 file changed, 12 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/pyanaconda/kickstart.py b/pyanaconda/kickstart.py
> >> index fadc501..fe7011a 100644
> >> --- a/pyanaconda/kickstart.py
> >> +++ b/pyanaconda/kickstart.py
> >> @@ -772,11 +772,9 @@ class LogVolData(commands.logvol.F20_LogVolData):
> >>           else:
> >>               try:
> >>                   size = Size("%d MiB" % self.size)
> >> -            except ValueError:
> >> +            except (ValueError, TypeError):
> >>                   raise KickstartValueError(formatErrorMsg(self.lineno,
> >>                           msg="The size \"%s\" is invalid." % self.size))
> >> -            except TypeError:
> >> -                pass
> >>   
> >>           if self.mountpoint == "swap":
> >>               ty = "swap"
> >> @@ -923,11 +921,9 @@ class LogVolData(commands.logvol.F20_LogVolData):
> >>               if self.maxSizeMB:
> >>                   try:
> >>                       maxsize = Size("%d MiB" % self.maxSizeMB)
> >> -                except ValueError:
> >> +                except (ValueError, TypeError):
> >>                       raise
> >>                       KickstartValueError(formatErrorMsg(self.lineno,
> >>                               msg="The maximum size \"%s\" is invalid." %
> >>                               self.maxSizeMB))
> >> -                except TypeError:
> >> -                    pass
> >>               else:
> >>                   maxsize = None
> >>   
> >> @@ -1014,13 +1010,15 @@ class
> >> PartitionData(commands.partition.F18_PartData):
> >>   
> >>           storage.doAutoPart = False
> >>   
> >> -        try:
> >> -            size = Size("%d MiB" % self.size)
> >> -        except ValueError:
> >> -            raise KickstartValueError(formatErrorMsg(self.lineno,
> >> -                    msg=_("The size \"%s\" is invalid.") % self.size))
> >> -        except TypeError:
> >> -            pass
> >> +        if self.size:
> >> +            try:
> >> +                size = Size("%d MiB" % self.size)
> >> +            except (ValueError, TypeError):
> >> +                raise KickstartValueError(formatErrorMsg(self.lineno,
> >> +                        msg=_("The size \"%s\" is invalid.") %
> >> self.size))
> >> +        else:
> >> +            # Have blivet determine a default value
> >> +            size = None
> >>   
> >>           if self.onbiosdisk != "":
> >>               for (disk, biosdisk) in storage.eddDict.iteritems():
> >> @@ -1186,11 +1184,9 @@ class
> >> PartitionData(commands.partition.F18_PartData):
> >>           if self.maxSizeMB:
> >>               try:
> >>                   maxsize = Size("%d MiB" % self.maxSizeMB)
> >> -            except ValueError:
> >> +            except (ValueError, TypeError):
> >>                   raise KickstartValueError(formatErrorMsg(self.lineno,
> >>                           msg=_("The maximum size \"%s\" is invalid.") %
> >>                           self.maxSizeMB))
> >> -            except TypeError:
> >> -                pass
> >>           else:
> >>               maxsize = None
> >>   
> >> --
> >> 2.1.0
> >>
> >> _______________________________________________
> >> anaconda-patches mailing list
> >> anaconda-patches at lists.fedorahosted.org
> >> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> >>
> > The parser in pykickstart defines these values as ints.
> > I think that means that an exception will already have
> > been thrown by parser if they can not be converted to ints.
> > So I don't understand what is the expected situation here.
> 
> Well if that's the case then I guess we can get rid of all of the
> TypeError checks, since they're just there (I think) to catch cases when
> the % operation fails to convert self.size to an int.

Yeah, that's what I think too. This came up recently in connection w/ some other patch.

> >
> > The stack trace just looks to me like somebody said "size" when they meant
> > self.size.
> 
> I think size is correct, since it's supposed to use the version of size
> converted to a Size at the top of the method, but it was possible at the
> top of the method to get a TypeError if self.size was None or an empty
> string or whatever and then not set the size variable.
> 
> 
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> 

Yeah, possibly setting size to 0 if its None right there is the correct thing.

Really is related bz#1146156, then, that code is all new.

- mulhern


More information about the anaconda-patches mailing list