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.
--
Peter
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
