[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