[master/rhel7-branch][PATCH 3/3] Enable anaconda to use the new rescue mode. (#965985)

Samantha N. Bueno sbueno+anaconda at redhat.com
Thu Jun 18 17:22:11 UTC 2015


On Wed, Jun 17, 2015 at 04:55:19PM -0700, Brian C. Lane wrote:
> On Mon, Jun 15, 2015 at 07:48:03PM -0400, Samantha N. Bueno wrote:
> > +    if flags.rescue_mode:
> > +        from pyanaconda.ui.tui.simpleline import App
> > +        from pyanaconda.rescue import RescueMode
> > +        app = App("Rescue Mode")
> > +        spoke = RescueMode(app, anaconda.ksdata, anaconda.storage)
> > +        app.schedule_screen(spoke)
> > +        app.run()
> > +
> 
> blank line could go.

Gone.
 
> > +    def input(self, args, key):
> > +        """Override any input so we can launch rescue mode."""
> >          try:
> > -            os.symlink('/mnt/runtime/etc/' + f, '/etc/' + f)
> > -        except OSError:
> > +            keyid = int(key) - 1
> > +            if 0 <= keyid < len(self._choices):
> > +                if keyid == 3:
> 
> The indent here is getting pretty deep, I'd either drop the initial
> keyid range check completely or make it return early so you can bump all
> these back one level.

Yeah, I see what you mean. I've moved the code to check values outside
of the try/except block (since that doesn't matter after trying to cast
key to an int) and gotten rid of the range check.
 
> > +                    # quit/reboot
> > +                    d = YesNoDialog(self.app, _(self.app.quit_message))
> > +                    self.app.switch_screen_modal(d)
> > +                    if d.answer:
> > +                        iutil.execWithRedirect("systemctl", ["--no-wall", "reboot"])
> > +                if keyid == 2:
> > +                    # skip to/run shell
> > +                    run_shell()
> > +                else:
> > +                    # user chose 0 (continue/rw-mount) or 1 (ro-mount)
> > +                    # decrypt any luks devices
> > +                    self._unlock_devices()
> > +
> > +                    # this sleep may look pointless, but it seems necessary, in
> > +                    # order for some task to complete; otherwise no existing
> > +                    # installations are discovered. IOW, this is a hack.
> > +                    time.sleep(2)
> > +                    # attempt to find previous installations
> > +                    roots = findExistingInstallations(self.storage.devicetree)
> > +                    if len(roots) == 1:
> > +                        self._root = roots[0]
> > +                    elif len(roots) > 1:
> > +                        # have to prompt user for which root to mount
> > +                        rootspoke = RootSpoke(self.app, self.data, self.storage,
> > +                                    self.payload, self.instclass, roots)
> > +                        self.app.switch_screen_modal(rootspoke)
> > +                        self._root = rootspoke.root
> > +
> > +                    # if only one root detected, or user has chosen which root
> > +                    # to mount, go ahead and do that
> > +                    newspoke = RescueMountSpoke(self.app, self.data,
> > +                                self.storage, self.payload, self.instclass, keyid, self._root)
> > +                    self.app.switch_screen_modal(newspoke)
> > +                    self.close()
> > +        except ValueError:
> > +            # idiots
> >              pass
> 
> Does the ValueError really need to cover the whole block? Or just the
> int(key)?

Heh, yeah, exactly as I commented above, so I've moved some things
around.
 
> > +    def _unlock_devices(self):
> > +        """
> > +            Loop through devices and attempt to unlock any which are detected as
> > +            LUKS devices.
> > +        """
> > +        try_passphrase = None
> 
> "try_passphrase" should be added to the localSkipList in exception.py so that it
> doesn't get dumped with a crash.

Oof, that is a good catch.
 
> > +        for device in self.storage.devices:
> > +            if device.format.type == "luks":
> 
> Here's another place where an early continue could remove a level of
> indentation.

Good idea, and it is definitely much cleaner/easier to read.
 
> > +                skip = False
> > +                unlocked = False
> > +                while not (skip or unlocked):
> > +                    if try_passphrase is None:
> > +                        p = PasswordDialog(self.app, device.name)
> > +                        self.app.switch_screen_modal(p)
> > +                        if p.answer:
> 
> This should also be added to the skiplist. I'm not exactly sure how to
> reference it without experimenting though. Internals for PasswordDialog
> should also be added if they aren't already caught.
> 
> (nevermind, I see from below that p.answer is a property to access
> p._password)
> 
> [snip, snip]
> 
> > +                    # now turn on swap
> > +                    if not self.readOnly:
> > +                        try:
> > +                            self.storage.turnOnSwap()
> > +                        except StorageError:
> > +                            log.error("Error enabling swap.")
> 
> I'm not sure if this makes sense if imageInstall is being used.

Ok, I can add a check for imageInstall and skip this then.
 
> > +        tui.UIScreen.__init__(self, app)
> > +        self._device = device
> > +        self._message = "You must enter your LUKS passphrase to decrypt device %s" % device
> > +        self._password = None
> 
> Good, that'll already be caught by the existing localSkipList.
> 
> > +    @property
> > +    def answer(self):
> > +        """The response can be None (no response) or the password entered."""
> > +        return self._password
> 
> Ah, it's a property. So maybe we don't have to exclude
> > +
> > +    def input(self, args, key):
> > +        if key:
> > +            self._password = key
> > +            self.close()
> > +            return True
> > +        else:
> > +            return False
> 
> Looks good to me, other than those comments.

Ok, thanks very much! I think I will still post a second iteration for
review, in just a bit after I test these changes really quickly.
 
> -- 
> Brian C. Lane | Anaconda Team | IRC: bcl #anaconda | Port Orchard, WA (PST8PDT)
> _______________________________________________
> 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