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

Re: [PATCH V2] add multiboot support for tboot



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
> 
> 
> 


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