[PATCH 2/5] Make minSize, maxSize consistent and correct.

David Lehman dlehman at redhat.com
Fri Mar 21 13:45:39 UTC 2014


On Fri, 2014-03-21 at 08:15 -0400, Anne Mulhern wrote:
> 
> 
> 
> ----- Original Message -----
> > From: "David Lehman" <dlehman at redhat.com>
> > To: "anaconda patch review" <anaconda-patches at lists.fedorahosted.org>
> > Sent: Thursday, March 20, 2014 6:35:19 PM
> > Subject: Re: [PATCH 2/5] Make minSize, maxSize consistent and correct.
> > 
> > On Thu, 2014-03-20 at 16:36 -0400, Anne Mulhern wrote:
> > > 
> > > 
> > > 
> > > ----- Original Message -----
> > > > From: "David Lehman" <dlehman at redhat.com>
> > > > To: anaconda-patches at lists.fedorahosted.org
> > > > Sent: Thursday, March 20, 2014 1:06:37 PM
> > > > Subject: [PATCH 2/5] Make minSize, maxSize consistent and correct.
> > > > 
> > > > StorageDevice defaults should be dependent on whether the device is
> > > > resizable -- not whether or not the format has known size limits.
> > > > 
> > > > Any minSize or maxSize may be zero, meaning there is no known limit.
> > > > ---
> > > >  blivet/devices.py | 22 ++++++++++++----------
> > > >  1 file changed, 12 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/blivet/devices.py b/blivet/devices.py
> > > > index 47ad3b3..750ce8a 100644
> > > > --- a/blivet/devices.py
> > > > +++ b/blivet/devices.py
> > > > @@ -850,18 +850,12 @@ class StorageDevice(Device):
> > > >      @property
> > > >      def minSize(self):
> > > >          """ The minimum size this device can be. """
> > > > -        if self.format.minSize:
> > > > -            return self.format.minSize
> > > > -        else:
> > > > -            return self.size
> > > > +        return self.format.minSize if self.resizable else
> > > > self.currentSize
> > > >  
> > > >      @property
> > > >      def maxSize(self):
> > > >          """ The maximum size this device can be. """
> > > > -        if self.format.maxSize > self.currentSize:
> > > > -            return self.currentSize
> > > > -        else:
> > > > -            return self.format.maxSize
> > > > +        return self.format.maxSize if self.resizable else
> > > > self.currentSize
> > > >  
> > > >      @property
> > > >      def status(self):
> > > > @@ -1715,7 +1709,11 @@ class PartitionDevice(StorageDevice):
> > > >              if partition.type == parted.PARTITION_FREESPACE:
> > > >                  maxPartSize = self.size +
> > > >                  Size(bytes=partition.getLength(unit="B"))
> > > >  
> > > > -        return min(self.format.maxSize, maxPartSize)
> > > > +        limits = [maxPartSize]
> > > > +        if self.format.maxSize > 0:
> > > > +            limits.append(self.format.maxSize)
> > > > +
> > > > +        return min(limits)
> > > >  
> > > 
> > > This is special handling introduced by the problem that 0 means 0 and "no
> > > max size",
> > 
> > Right. Such handling is sprinkled throughout blivet.
> > 
> > > both. It would be nice to have a special Size value that does the correct
> > > thing.
> > > Here min("no max size", maxPartSize) ought to be maxPartSize.
> > 
> > Right.
> > 
> > > What should max("no max size", maxPartSize) be?
> > 
> > That's a question we have no need to answer, so I'm not terribly worried
> > about it. As of now, any format type that is resizable has a maximum
> > size.
> > 
> 
> If we never have to worry about the right behavior for a max op on
> special "no max size" Size value it becomes a lot easier to design,
> I would think.

A bit easier, yeah. We could remove this pattern from a dozen or so
places. We have bigger fish to fry IMO.

> 
> I was thinking about higher level things before, but the code above
> would have been easier for me to read if it has been:
> 
> return min(self.format.maxSize, maxPartSize) if self.format.maxSize != 0 else maxPartSize

I can hardly believe I'm warming up to that syntax, but I am. I'll go
with that. The pattern I used was taken from a place where there are
three items to consider, so it actually improves clarity and brevity IMO
-- here it does not.

> 
> or something like that.
> 
> - mulhern
> 
> 
> > > 
> > > >      @property
> > > >      def currentSize(self):
> > > > @@ -2641,7 +2639,11 @@ class LVMLogicalVolumeDevice(DMDevice):
> > > >      @property
> > > >      def maxSize(self):
> > > >          """ The maximum size this lv can be. """
> > > > -        return min(self.format.maxSize, self.size + self.vg.freeSpace)
> > > > +        limits = [self.size + self.vg.freeSpace]
> > > > +        if self.format.maxSize > 0:
> > > > +            limits.append(self.format.maxSize)
> > > > +
> > > > +        return min(limits)
> > > >  
> > > 
> > > Ditto.
> > > 
> > > >      @property
> > > >      def vgSpaceUsed(self):
> > > > --
> > > > 1.8.5.3
> > > > 
> > > > _______________________________________________
> > > > anaconda-patches mailing list
> > > > anaconda-patches at lists.fedorahosted.org
> > > > https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> > > > 
> > > 
> > > I believe that maxSize and minSize can be used for both existing and
> > > non-existing devices.
> > 
> > A format's minSize and maxSize can be used for either existing or
> > non-existing devices, but for devices we only use them for resize of
> > existing devices.
> > 
> > > 
> > > That would be valuable information to indicate in method comment, if it is
> > > correct.
> > > 
> > > - mulhern
> > > 
> > > 
> > > 
> > > 
> > > _______________________________________________
> > > anaconda-patches mailing list
> > > anaconda-patches at lists.fedorahosted.org
> > > https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> > 
> > 
> > _______________________________________________
> > anaconda-patches mailing list
> > anaconda-patches at lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> > 
> _______________________________________________
> 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