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

Brian C. Lane bcl at redhat.com
Wed Jun 17 23:55:19 UTC 2015


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.

> +    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.

> +                    # 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)?

> +    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.

> +        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.

> +                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.

> +        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.

-- 
Brian C. Lane | Anaconda Team | IRC: bcl #anaconda | Port Orchard, WA (PST8PDT)


More information about the anaconda-patches mailing list