[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