On Fri, 2011-04-01 at 17:22 -0400, Peter Jones wrote:
> On 04/01/2011 03:57 PM, David Lehman wrote:
> > +def get_boot_block(device, seek_blocks=0):
> > + block = " " * 512
> > + status = device.status
> > + if not status:
> > + try:
> > + device.setup()
> > + except StorageError:
> > + return block
> > + fd = os.open(device.path, os.O_RDONLY)
> > + if seek_blocks:
> > + os.lseek(fd, seek_blocks * 512, 0)
> > + block = os.read(fd, 512)
> > + os.close(fd)
> > + if not status:
> > + try:
> > + device.teardown(recursive=True)
> > + except StorageError:
> > + pass
> > +
> > + return block
>
> This probably ought to query and use the block size for the device rather than
> hard-code 512, though I'm not 100% sure it'll actually make any difference.
I'll take a look at how to do this. It makes sense. I'd prefer if this
was completely unneeded instead, though.
>
> ...
>
> > +class BootLoaderImage(object):
> > + """ Base class for bootloader images. Suitable for non-linux OS images. """
> > + def __init__(self, device=None, label=None):
> > + self.label = label
> > + self.device = device
>
> I'm really not sure I understand the concept of what an "image" is here.
An image is a boot target, which ends up being a config stanza.
>
>
> > +class LinuxBootLoaderImage(BootLoaderImage):
> > + def __init__(self, device=None, label=None, short=None, version=None):
> > + super(LinuxBootLoaderImage, self).__init__(device=device, label=label)
> > + self.label = label # label string
> > + self.short_label = short # shorter label string
> > + self.device = device # StorageDevice instance
> > + self.version = version # kernel version string
> > + self._kernel = None # filename string
> > + self._initrd = None # filename string
>
> Is it a config stanza then?
Pretty much.
>
> > +class BootLoader(object):
> > + """TODO:
> > + - iSeries bootloader?
> > + - same as pSeries, except optional, I think
> > + - upgrade of non-grub bootloaders
> > + - detection of existing bootloaders
> > + - resolve overlap between Platform checkFoo methods and
> > + _is_valid_target_device and _is_valid_boot_device
> > + - split certain parts of _is_valid_target_device and
> > + _is_valid_boot_device into specific bootloader classes
> > + """
> > + name = "Generic Bootloader"
> > + packages = []
> > + config_file = None
> > + config_file_mode = 0600
> > + can_dual_boot = False
> > + can_update = False
> > + image_label_attr = "label"
> > +
> > + # requirements for bootloader target devices
> > + target_device_types = ["partition"]
> > + target_device_raid_levels = []
> > + target_device_format_types = []
> > + target_device_disklabel_types = ["msdos"]
>
> So the base class is really sort of abstract i386, then, and just doesn't say
> that? Why aren't the x86-ish bits in either an x86-only base class or in GRUB?
I will move the MBR reference, the msdos disklabel reference, and
perhaps the partition reference down into specific classes. I probably
think of non-EFI x86 as more of a "default" than I had realized. I feel
better about BootLoader being more abstract and less "default".
>
> > + def _is_valid_boot_device(self, device, linux=False):
> ...
> > + # FIXME: the windows boot block part belongs in GRUB
> > + if hasattr(device, "partedPartition") and \
> > + (not device.bootable or not device.partedPartition.active) and \
> > + not has_windows_boot_block(device):
> > + return False
>
> Doesn't this mean that this workflow won't work:
>
> 1) Use existing device label with windows installed
> 2) remove all partitions
> 3) create new partitions...
>
> Since at that point we'll still have a NTLDR installed.
Would the NTLDR be in a partition or MBR? If we removed all the
partitions, what good is the NTLDR?
I know nothing about how windows boots, and won't be rushing off anytime
soon to learn about it. Are you saying that with NTLDR in MBR a
partition with vfat and no windows boot block should still be a valid
stage2? If so, I'd be happy to remove that boot block stuff completely.
>
> > + #
> > + # FIPS
> > + #
> > + if flags.cmdline.get("fips") == "1":
> > + self.boot_args.append("boot=%s" % self.stage2_device.fstabSpec)
>
> Wait, what? boot= is for FIPS?
>
> I realize this same code is in booty almost exactly like this, but a comment
> to mention what the hell it's doing might be helpful.
If I knew WTH it was doing, other than satisfying FIPS, I would have
commented to that effect ;-)
> ...
> > + #
> > + # installation
> > + #
> > + def write(self, install_root=""):
> > + """ Write the bootloader configuration and install the bootloader. """
> > + if self.update_only:
> > + self.update(install_root=install_root)
> > + return
> > +
> > + self.write_config(install_root=install_root)
> > + sync()
> > + sync()
> > + sync()
> > + sync()
>
> Three of these are probably unnecessary paranoia held over from booty. But
> We also do need fs checking for e.g. xfs_freeze :/
Proposed fix in two-piece patchset sent this afternoon including "Do
filesystem-specific sync operation after writing configuration."
>
> > +class GRUB(BootLoader):
> ...
> > + boot_device_format_types = ["vfat", "ntfs", "hpfs"]
>
> I was really confused by this for a minute until I figured out that this was
> specifically *not* linux. With that in mind, it seems like I'd assume that
> unprefixed was the common case (e.g. linux) and the non-linux version should
> be the labelled ones. But that's really pretty nitpicky.
I found myself annoyed by this a couple of times, too. I'll look at it
again.
>
> ...
> > + @property
> > + def encrypted_password(self):
> > + import string
> > + import crypt
> > + import random
> > + salt = "$6$"
> > + salt_len = 16
> > + salt_chars = string.letters + string.digits + './'
> > +
> > + rand_gen = random.SystemRandom()
> > + salt += "".join([rand_gen.choice(salt_chars) for i in range(salt_len)])
> > + password = crypt.crypt(self.password, salt)
> > + return password
>
> It almost seems as if password creation should be entirely separate.
Agreed. If I knew where to put it, or really anything about password
encryption, I would have put it somewhere else. I wanted to, but
deferred the effort.
>
> > + def write_device_map(self, install_root=""):
> > + """ Write out a device map containing all supported devices. """
> > + map_path = os.path.normpath(install_root + self.device_map_file)
> > + if os.access(map_path, os.R_OK):
> > + os.rename(map_path, map_path + ".rpmsave")
>
> I'm not entirely convinced there's any point in keeping a .rpmsave here,
> but I guess that's something we can do.
If nothing else, the '.rpmsave' seems a bit inappropriate. This is
another thing I told myself I'd fix later.
>
> > + # make symlink to grub.conf in /etc since that's where configs belong
> > + etc_grub = "%s/etc/%s" % (install_root, self._config_file)
> > + if os.access(etc_grub, os.R_OK):
> > + try:
> > + os.rename(etc_grub, etc_grub + '.rpmsave')
> > + except OSError as e:
> > + log.error("failed to back up %s: %s" % (etc_grub, e))
>
> Keeping a .rpmsave of the symlink seems *really* odd. It's always a symlink.
> Aren't we just going to wind up with 2 symlinks to the same thing? At least
> replace the old symlink with a symlink to the new config file.
Yeah, that's dumb. I'll remove that.
> ...
>
> > anaconda.intf.messageWindow(_("Warning"),
> > - _("No kernel packages were installed on the "
> > - "system. Bootloader configuration "
> > - "will not be changed."))
> > + _("No kernel packages were installed on the system "
> > + "Bootloader configuration will not be changed."))
>
> Minor nit - you've changed the grammar and translation of this message,
> probably not on purpose since the new way makes less sense.
>
You mean by inadvertently removing the period after "system"? I'll
certainly put that back, but I don't know how I changed it otherwise.
Thanks for the feedback.
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
