I uploaded a new version to http://dlehman.fedorapeople.org/bootloader.py that incorporates all of the discussed items except for password encryption and an explanation of boot=. I also didn't address the windows-related workflow question because I don't understand it. Dave 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. > > ... > > > +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. > > > > +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? > > > +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? > > > + 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. > > > + # > > + # 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. > ... > > + # > > + # 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 :/ > > > +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. > > ... > > + @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. > > > + 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. > > > + # 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. > ... > > > 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. > _______________________________________________ Anaconda-devel-list mailing list Anaconda-devel-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/anaconda-devel-list
