[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [PATCH 7/7] Convert livecd.py to use the storage module where appropriate.



On Fri, 2011-01-07 at 15:26 -0500, Chris Lumens wrote:
> > @@ -125,28 +125,10 @@ class LiveCDCopyBackend(backend.AnacondaBackend):
> >                                 custom_icon="error",
> >                                 custom_buttons=[_("Exit installer")])
> >              sys.exit(0)
> > -        self.rootFsType = isys.readFSType(self.osimg)
> >  
> > -    def _getLiveBlockDevice(self):
> > -        return os.path.normpath(self.osimg)
> > -
> > -    def _getLiveSize(self):
> > -        def parseField(output, field):
> > -            for line in output.split("\n"):
> > -                if line.startswith(field + ":"):
> > -                    return line[len(field) + 1:].strip()
> > -            raise KeyError("Failed to find field '%s' in output" % field)
> > -
> > -        output = subprocess.Popen(['/sbin/dumpe2fs', '-h', self.osimg],
> > -                                  stdout=subprocess.PIPE,
> > -                                  stderr=open('/dev/null', 'w')
> > -                                  ).communicate()[0]
> > -        blkcnt = int(parseField(output, "Block count"))
> > -        blksize = int(parseField(output, "Block size"))
> > -        return blkcnt * blksize
> > -
> > -    def _getLiveSizeMB(self):
> > -        return self._getLiveSize() / 1048576
> > +    @property
> > +    def osimg(self):
> > +        return self.anaconda.storage.liveImage
> >  
> >      def postAction(self, anaconda):
> >          try:
> 
> backend.AnacondaBackend needs to subclass object for this to work.

Weird. I could swear I tested this. Also, wow -- I mistakenly assumed
all of anaconda's classes were new-style.

> 
> > @@ -402,28 +348,11 @@ class LiveCDCopyBackend(backend.AnacondaBackend):
> >  
> >      def getMinimumSizeMB(self, part):
> >          if part == "/":
> > -            return self._getLiveSizeMB()
> > +            return self.osimg.format.size
> >          return 0
> >  
> >      def doBackendSetup(self, anaconda):
> > -        # ensure there's enough space on the rootfs
> > -        # FIXME: really, this should be in the general sanity checking, but
> > -        # trying to weave that in is a little tricky at present.
> > -        ossize = self._getLiveSizeMB()
> > -        slash = anaconda.storage.rootDevice
> > -        if slash.size < ossize:
> > -            rc = anaconda.intf.messageWindow(_("Error"),
> > -                                        _("The root filesystem you created is "
> > -                                          "not large enough for this live "
> > -                                          "image (%.2f MB required).") % ossize,
> > -                                        type = "custom",
> > -                                        custom_icon = "error",
> > -                                        custom_buttons=[_("_Back"),
> > -                                                        _("_Exit installer")])
> > -            if rc == 0:
> > -                return DISPATCH_BACK
> > -            else:
> > -                sys.exit(1)
> > +        pass
> >  
> >      # package/group selection doesn't apply for this backend
> >      def groupExists(self, group):
> 
> You could also convert backend.py:doBackendSetup to just pass instead of
> raise an error, and then get rid of this method entirely.  I'd be fine
> with that.

There are at least eight of these that raise NotImplementedError. I like
the idea of making them pass instead.

> 
> > diff --git a/pyanaconda/storage/__init__.py b/pyanaconda/storage/__init__.py
> > index bf74f88..1531685 100644
> > --- a/pyanaconda/storage/__init__.py
> > +++ b/pyanaconda/storage/__init__.py
> > @@ -616,6 +616,14 @@ class Storage(object):
> >          protected.sort(key=lambda d: d.name)
> >          return protected
> >  
> > +    @property
> > +    def liveImage(self):
> > +        """ The OS image used by live installs. """
> > +        _image = None
> > +        if flags.livecdInstall:
> > +            _image = self.devicetree.getDeviceByPath(self.anaconda.methodstr[9:])
> > +        return _image
> > +
> >      def exceptionDisks(self):
> >          """ Return a list of removable devices to save exceptions to.
> >  
> 
> What's the point behind using the intermediate _image here?  Do you want
> to cache it?

I just prefer a single return statement when it's possible.

> 
> > @@ -1001,20 +1009,25 @@ class Storage(object):
> >  
> >          if (root and
> >              root.size < self.anaconda.backend.getMinimumSizeMB("/")):
> > +            if flags.livecdInstall:
> > +                live = " Live"
> > +            else:
> > +                live = ""
> >              errors.append(_("Your / partition is less than %(min)s "
> >                              "MB which is lower than recommended "
> > -                            "for a normal %(productName)s install.")
> > +                            "for a normal %(productName)s%(live)s install.")
> >                            % {'min': self.anaconda.backend.getMinimumSizeMB("/"),
> > -                             'productName': productName})
> > +                             'productName': productName,
> > +                             'live': live})
> >  
> >          # livecds have to have the rootfs type match up
> >          if (root and
> > -            self.anaconda.backend.rootFsType and
> > -            root.format.type != self.anaconda.backend.rootFsType):
> > +            self.liveImage and
> > +            root.format.type != self.liveImage.format.type):
> >              errors.append(_("Your / partition does not match the "
> >                              "the live image you are installing from.  "
> >                              "It must be formatted as %s.")
> > -                          % (self.anaconda.backend.rootFsType,))
> > +                          % (self.liveImage.format.type,))
> >  
> >          for (mount, size) in checkSizes:
> >              if mount in filesystems and filesystems[mount].size < size:
> 
> I know we talked about this yesterday.  Perhaps now is a good time to
> also force the fstype in the UI so the user can't accidentally choose
> something we're later going to reject.  We might as well get rid of the
> illusion of choice here.

While I like that idea, further thought leads to the unfortunate detail
of rebuilding the fstype combo based on the current value of mountpoint,
which is distasteful at best.

Dave



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]