Re: [PATCH V2] add multiboot support for tboot

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



I have some inline comments.

On Mon, 2011-08-01 at 17:54 +0800, Wei, Gang wrote:
> diff --git a/pyanaconda/bootloader.py b/pyanaconda/bootloader.py
> index 6df0107..dc11f90 100644
> --- a/pyanaconda/bootloader.py
> +++ b/pyanaconda/bootloader.py
> @@ -114,6 +114,25 @@ class LinuxBootLoaderImage(BootLoaderImage):
>              filename = "initramfs-%s.img" % self.version
>          return filename
>  
> +class MultibootLinuxBootLoaderImage(LinuxBootLoaderImage):
> +    def __init__(self, device=None, label=None, short=None,
> version=None,
> +                 multiboot=None, mbargs=None, args=None):
> +        super(MultibootLinuxBootLoaderImage, self).__init__(
> +                                                    device=device,
> label=label,
> +                                                    short=short,
> version=version)
> +        self._multiboot = multiboot      # filename string
> +        self._mbargs = mbargs

It seems like these two attributes above could just be hardcoded in here
if they cannot be changed in any case.

> +       self._args = args
> +
> +    @property
> +    def multiboot(self):
> +        return self._multiboot
> +
> +    def mbargs(self):
> +        return self._mbargs
> +
> +    def args(self):
> +        return self._args
>  
>  class BootLoader(object):
>      """TODO:
> @@ -1123,15 +1142,30 @@ class GRUB(BootLoader):
>                  grub_root = self.grub_device_name(self.stage2_device)
>                  args.update(["ro", "root=%s" %
> image.device.fstabSpec])
>                  args.update(self.boot_args)
> -                stanza = ("title %(label)s (%(version)s)\n"
> -                          "\troot %(grub_root)s\n"


> -                          "\tkernel %(prefix)s/%(kernel)s %(args)s\n"
> -                          "\tinitrd %(prefix)s/%(initrd)s\n"

Just by abstracting the above section you could prevent duplication of
the entire stanza template, eg:

if isinstance(image, MultibootLinuxBootLoaderImage):
  snippet = ("\tkernel %(prefix)s/%(multiboot)s %(mbargs)s\n"
             "\tmodule %(prefix)s/%(kernel)s %(args)s\n"
             "\tmodule %(prefix)s/%(initrd)s\n"
             % {"prefix": self.boot_prefix,
                "multiboot": image.multiboot,
                "mbargs": image.mbargs,
                "kernel": image.kernel, "initrd": image.initrd,
                "args": args})
else:
  snippet = ("\tkernel %(prefix)s/%(kernel)s %(args)s\n"
             "\tinitrd %(prefix)s/%(initrd)s\n"
             % {"prefix": self.boot_prefix,
                "kernel": image.kernel, "initrd": image.initrd,
                "args": args})

and then

stanza = ("title %(label)s (%(version)s)\n"
          "\troot %(grub_root)s\n"
          "%(snippet)s"
          % {"label": image.label, "version": image.version,
	     "snippet": snippet
             "grub_root": grub_root})



> -                          % {"label": image.label, "version":
> image.version,
> -                             "grub_root": grub_root,
> -                             "kernel": image.kernel, "initrd":
> image.initrd,
> -                             "args": args,
> -                             "prefix": self.boot_prefix})
> +                if isinstance(image, MultibootLinuxBootLoaderImage):
> +                    args.update(image.args)
> +                    stanza = ("title %(label)s (%(version)s)\n"
> +                              "\troot %(grub_root)s\n"
> +                              "\tkernel %(prefix)s/%(multiboot)s
> %(mbargs)s\n"
> +                              "\tmodule %(prefix)s/%(kernel)s
> %(args)s\n"
> +                              "\tmodule %(prefix)s/%(initrd)s\n"
> +                              % {"label": image.label, "version":
> image.version,
> +                                 "grub_root": grub_root,
> +                                 "multiboot": image.multiboot,
> +                                 "mbargs": image.mbargs,
> +                                 "kernel": image.kernel, "initrd":
> image.initrd,
> +                                 "args": args,
> +                                 "prefix": self.boot_prefix})
> +                else:
> +                    stanza = ("title %(label)s (%(version)s)\n"
> +                              "\troot %(grub_root)s\n"
> +                              "\tkernel %(prefix)s/%(kernel)s
> %(args)s\n"
> +                              "\tinitrd %(prefix)s/%(initrd)s\n"
> +                              % {"label": image.label, "version":
> image.version,
> +                                 "grub_root": grub_root,
> +                                 "kernel": image.kernel, "initrd":
> image.initrd,
> +                                 "args": args,
> +                                 "prefix": self.boot_prefix})
>              else:
>                  stanza = ("title %(label)s\n"
>                            "\trootnoverify %(grub_root)s\n"
> @@ -1905,6 +1939,8 @@ def writeSysconfigKernel(anaconda,
> default_kernel):
>      f.write("DEFAULTKERNEL=%s\n" % default_kernel)
>      f.close()
>  
> +def is_trusted_boot(anaconda):
> +    return anaconda.backend.ayum.isPackageInstalled(name="tboot")

Perhaps this could be written to an attribute of the BootLoader instance
from inside YumBackend immediately after package installation so we
don't have yum references in the bootloader module. Then you can just
check anaconda.bootloader.trusted_boot in writeBootloader.

>  
>  def writeBootloader(anaconda):
>      """ Write bootloader configuration to disk.
> @@ -1971,9 +2007,18 @@ def writeBootloader(anaconda):
>          used.append(nick)
>          label = "%s-%s" % (base_label, nick)
>          short = "%s-%s" % (base_short, nick)
> -        image =
> LinuxBootLoaderImage(device=anaconda.storage.rootDevice,
> -                                     version=version,
> -                                     label=label, short=short)
> +        if is_trusted_boot(anaconda):
> +            image = MultibootLinuxBootLoaderImage(
> +
> device=anaconda.storage.rootDevice,
> +                                         version=version,
> +                                         label=label, short=short,
> +                                         multiboot="tboot.gz",
> +
> mbargs=["logging=vga,serial,memory"],
> +                                         args=["intel_iommu=on"])

If you're going to hardcode the multiboot and mbargs strings, why not do
it in the MultibootLinuxBootLoaderImage class instead of here?

> +        else:
> +            image =
> LinuxBootLoaderImage(device=anaconda.storage.rootDevice,
> +                                         version=version,
> +                                         label=label, short=short)
>          anaconda.bootloader.add_image(image)
>  
>      # write out /etc/sysconfig/kernel

Dave

> -- 
> 1.7.5.1
> 
> 
> 

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list


[Index of Archives]     [Kickstart]     [Fedora Users]     [Fedora Legacy List]     [Fedora Maintainers]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [Yosemite Photos]     [KDE Users]     [Fedora Tools]
  Powered by Linux