Re: [PATCH] Guest boot loader support [1/2]

[Mike Wray <mike.wray@hpl.hp.com>]

Jeremy Katz wrote:
> This first patch adds support to xend and xm to run an executable to use
> as a bootloader for passing back the kernel, initrd and kernel arguments
> to use for the domain.
> 
> Signed-off-by: Jeremy Katz <katzj@redhat.com>
> 
> Jeremy

Thanks for the patch. There are some issues that need sorting out before
we can apply it. Headlines here, some more detail in-line below.

- the code only supports booting with the bootloader when called from xm,
   since xm is running the loader. For general use it needs to support configuring
   the bootloader in the domain config and have xend run it.

- need to be able to configure the kernel to boot in the domain config
   when not using a console.

- need to remove calls to exit from xend-called code
- replace prints to stderr with logging

- handle errors when calling external scripts

- it would be better to be able to remove the assumption that the first
   device is the disk to boot from

Feel free to ask if you need any more details or more info.

Mike

> 
> 
> ------------------------------------------------------------------------
> 
> # This is a BitKeeper generated diff -Nru style patch.
> #
> # ChangeSet
> #   2005/04/13 23:01:42-04:00 katzj@bree.local.net 
> #   Add framework for running a bootloader from within xend and xm. To
> #   specify the executable to run, set in your domain config like
> #     bootloader="/usr/bin/pygrub"
> #   
> #   The bootloader will get executed on both domain creation and domain
> #   reboot to get the default kernel/initrd specified in the bootloader
> #   config for the domain.  If you auto-connect the console on domain
> #   creation, then your bootloader will be run in an interactive mode.
> # 
> # BitKeeper/etc/logging_ok
> #   2005/04/13 23:01:42-04:00 katzj@bree.local.net +1 -0
> #   Logging to logging@openlogging.org accepted
> # 
> # tools/python/xen/xend/XendBootloader.py
> #   2005/04/13 23:01:37-04:00 katzj@bree.local.net +79 -0
> # 
> # tools/python/xen/xm/create.py
> #   2005/04/13 23:01:37-04:00 katzj@bree.local.net +29 -2
> #   Run bootloader on domain creation.  If console is being connected,
> #   then run in an interactive mode, else just go with the default.
> # 
> # tools/python/xen/xend/XendDomainInfo.py
> #   2005/04/13 23:01:37-04:00 katzj@bree.local.net +41 -1
> #   Add running of a boot loader from xend on domain reboot.  Runs in
> #   an uninteractive mode to get the default kernel/initrd for the
> #   domain.  Also removes any temporary kernels created by the 
> #   bootloader.
> # 
> # tools/python/xen/xend/XendBootloader.py
> #   2005/04/13 23:01:37-04:00 katzj@bree.local.net +0 -0
> #   BitKeeper file /home/katzj/cvs/xen/xen-pygrub/tools/python/xen/xend/XendBootloader.py
> # 
> diff -Nru a/tools/python/xen/xend/XendBootloader.py b/tools/python/xen/xend/XendBootloader.py
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/tools/python/xen/xend/XendBootloader.py	2005-04-13 23:04:06 -04:00
> @@ -0,0 +1,79 @@
> +#
> +# XendBootloader.py - Framework to run a boot loader for picking the kernel
> +#
> +# Copyright 2005 Red Hat, Inc.
> +# Jeremy Katz <katzj@redhat.com>
> +#
> +# This software may be freely redistributed under the terms of the GNU
> +# general public license.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> +#
> +
> +import os, sys, select
> +import sxp
> +
> +BL_FIFO = "/var/lib/xen/xenbl"
> +
> +def bootloader(blexec, disk, quiet = 0, vcpus = None):
> +    """Run the boot loader executable on the given disk and return a
> +    config image.
> +    @param blexec  Binary to use as the boot loader
> +    @param disk Disk to run the boot loader on."""
> +    
> +    if not os.access(blexec, os.X_OK):
> +        print >> sys.stderr, "Bootloader isn't executable"
> +        sys.exit(1)

Use logging instead of stderr. Don't call exit, raise a XendError.

> +    if not os.access(disk, os.R_OK):
> +        print >> sys.stderr, "Disk isn't accessible"
> +        sys.exit(1)
> +

The code below looks like it's reading data back from the child process -
Python has the popen2 module to do this for you. Maybe use that instead?
Using a constant name for the fifo means only one instance of the code
can run at once - which could be a problem.

The sxpr parser can be called incrementally as you read instead
of building up the whole string.

> +    os.mkfifo(BL_FIFO, 0600)
> +
> +    child = os.fork()
> +    if (not child):
> +        args = [ blexec, "-o", BL_FIFO ]
> +        if quiet:
> +            args.append("-q")
> +        args.append(disk)
> +
> +        try:
> +            os.execvp(args[0], args)
> +        except OSError, e:
> +            print e
> +            pass
> +        os._exit(1)
> +
> +    while 1:
> +        try:
> +            r = os.open(BL_FIFO, os.O_RDONLY)
> +        except OSError, e:
> +            if e.errno == 4:
> +                continue
> +        break
> +    ret = ""
> +    while 1:
> +        select.select([r], [], [])
> +        s = os.read(r, 1024)
> +        ret = ret + s
> +        if len(s) == 0:
> +            break
> +        
> +    (pid, status) = os.waitpid(child, 0)
> +    os.close(r)
> +    os.unlink(BL_FIFO)
> +
> +    pin = sxp.Parser()
> +    pin.input(ret)
> +    pin.input_eof()
> +
> +    config_image = pin.val
> +    if vcpus and sxp.child_value(config_image, "vcpus") is None:
> +        config_image.append(['vcpus', vcpus])
> +
> +    config = [['image', pin.val]]
> +    config.append(['bootloader', blexec])
> +    return config
> +
> diff -Nru a/tools/python/xen/xend/XendDomainInfo.py b/tools/python/xen/xend/XendDomainInfo.py
> --- a/tools/python/xen/xend/XendDomainInfo.py	2005-04-13 23:04:06 -04:00
> +++ b/tools/python/xen/xend/XendDomainInfo.py	2005-04-13 23:04:06 -04:00
> @@ -21,6 +21,7 @@
>  import xen.util.ip
>  from xen.util.ip import _readline, _readlines
>  from xen.xend.server import channel
> +from xen.xend.XendBootloader import bootloader
>  
>  import sxp
>  
> @@ -323,6 +324,7 @@
>          self.image_handler = None
>          self.is_vmx = 0
>          self.vcpus = 1
> +        self.bootloader = None
>  
>      def setdom(self, dom):
>          """Set the domain id.
> @@ -458,6 +460,7 @@
>  
>              self.find_image_handler()
>              self.init_domain()
> +            self.configure_bootloader()
>              self.configure_console()
>              self.configure_backends()
>              self.construct_image()
> @@ -795,7 +798,7 @@
>                        	ramdisk        = ramdisk,
>                        	flags          = flags)
>  	else:
> -        	log.warning('building dom with %d vcpus', self.vcpus)
> +        	log.warning('building dom with %d vcpus' %(self.vcpus,))

There's no need to use % here - log.warning does the formatting for you.


>          	err = buildfn(dom            = dom,
>                 	      	image          = kernel,
>                        	control_evtchn = self.console.getRemotePort(),
> @@ -803,6 +806,14 @@
>                        	ramdisk        = ramdisk,
>                        	flags          = flags,
>                        	vcpus          = self.vcpus)
> +
> +        if self.bootloader:
> +            try:
> +                if kernel: os.unlink(kernel)
> +                if ramdisk: os.unlink(ramdisk)
> +            except OSError, e:
> +                log.warning('unable to unlink kernel/ramdisk: %s' %(e,))
> +       
>          if err != 0:
>              raise VmError('Building domain failed: type=%s dom=%d err=%d'
>                            % (ostype, dom, err))
> @@ -961,6 +972,13 @@
>              maxmem = self.memory
>          xc.domain_setmaxmem(self.dom, maxmem_kb = maxmem * 1024)
>  
> +    def configure_bootloader(self):
> +        """Configure boot loader.
> +        """
> +        bl = sxp.child_value(self.config, "bootloader")
> +        if bl is not None:
> +            self.bootloader = bl
> +
>      def configure_console(self):
>          """Configure the vm console port.
>          """
> @@ -1034,6 +1052,26 @@
>          try:
>              self.restart_check()
>              self.restart_state = STATE_RESTART_BOOTING
> +            # if we're restarting with a bootloader, we need to run it
> +            if self.bootloader:
> +                # FIXME: this assumes the disk is the first device and
> +                # that we're booting from the first disk
> +                blcfg = None
> +                
> +                dev = sxp.child_value(self.config, "device")
> +                if dev:
> +                    log.info("dev is %s" %(dev,))
> +                    disk = sxp.child_value(dev, "uname")
> +                    (type, fn) = disk.split(":")
> +                    if type == "phy" and not fn.startswith("/dev/"):
> +                        fn = "/dev/%s" %(fn,)


There are functions to manipulate the block device name in xend/server/blkif.py.
Probably better to use one of those.
Not keen on '%(fn,)' - why not use '% fn'?

> +                    blcfg = bootloader(self.bootloader, fn, 1, self.vcpus)
> +
> +                if blcfg is None:
> +                    log.warning("Had a boot loader, but can't find the disk")

Shouldn't this be a fatal error?

> +                else:
> +                    s = "(vm %s)" %(sxp.to_string(blcfg[0]),)
> +                    self.config = sxp.merge(sxp.from_string(s), self.config)

If blcfg[0] is already a list, no need to convert to string and back just to append.
Better to do

s = [ sxp.name(self.config) ] + blcfg[0]
sxp.merge(s, self.config)

>              d = self.construct(self.config)
>          finally:
>              self.restart_state = None
> @@ -1163,6 +1201,7 @@
>      if args:
>          cmdline += " " + args
>      ramdisk = sxp.child_value(image, "ramdisk", '')
> +    log.debug("creating linux domain with cmdline: %s" %(cmdline,))
>      vm.create_domain("linux", kernel, ramdisk, cmdline)
>      return vm
>  
> @@ -1377,6 +1416,7 @@
>  add_config_handler('device',     vm_field_ignore)
>  add_config_handler('backend',    vm_field_ignore)
>  add_config_handler('vcpus',      vm_field_ignore)
> +add_config_handler('bootloader', vm_field_ignore)
>  
>  # Register other config handlers.
>  add_config_handler('maxmem',     vm_field_maxmem)
> diff -Nru a/tools/python/xen/xm/create.py b/tools/python/xen/xm/create.py
> --- a/tools/python/xen/xm/create.py	2005-04-13 23:04:06 -04:00
> +++ b/tools/python/xen/xm/create.py	2005-04-13 23:04:06 -04:00
> @@ -10,6 +10,7 @@
>  from xen.xend import sxp
>  from xen.xend import PrettyPrint
>  from xen.xend.XendClient import server, XendError
> +from xen.xend.XendBootloader import bootloader
>  
>  from xen.util import console_client
>  
> @@ -94,6 +95,10 @@
>            fn=set_value, default=None,
>            use="Domain name. Must be unique.")
>  
> +gopts.var('bootloader', val='FILE',
> +          fn=set_value, default=None,
> +          use="Path to bootloader.")
> +
>  gopts.var('kernel', val='FILE',
>            fn=set_value, default=None,
>            use="Path to kernel image.")
> @@ -375,6 +380,23 @@
>      config_devs.append(['device_model', device_model])
>      config_devs.append(['device_config', device_config])
>  
> +def run_bootloader(config, vals):
> +    if not os.access(vals.bootloader, os.X_OK):
> +        print >> sys.stderr, "Bootloader isn't executable"
> +        sys.exit(1)
> +    if len(vals.disk) < 1:
> +        print >> sys.stderr, "No disks configured and boot loader requested"
> +        sys.exit(1)
> +    (uname, dev, mode, backend) = vals.disk[0]
> +    (typ, file) = uname.split(":")
> +    if typ == "phy" and not file.startswith("/dev/"):
> +        file = "/dev/%s" %(file,)
> +
> +    blcfg = bootloader(vals.bootloader, file,
> +                       not vals.console_autoconnect, vals.vcpus)
> +
> +    config.extend(blcfg)

This is a repeat of the reboot code in xend - and initial boot needs
to be in xend too.

> +
>  def make_config(vals):
>      """Create the domain configuration.
>      """
> @@ -396,8 +418,11 @@
>          config.append(['restart', vals.restart])
>      if vals.console:
>          config.append(['console', vals.console])
> -    
> -    configure_image(config, vals)
> +
> +    if vals.bootloader:
> +        run_bootloader(config, vals)
> +    else:
> +        configure_image(config, vals)
>      config_devs = []
>      configure_disks(config_devs, vals)
>      configure_pci(config_devs, vals)
> @@ -405,6 +430,7 @@
>      configure_usb(config_devs, vals)
>      configure_vmx(config_devs, vals)
>      config += config_devs
> +
>      return config
>  
>  def preprocess_disk(opts, vals):
> @@ -588,6 +614,7 @@
>          if not opts.getopt('name') and opts.getopt('defconfig'):
>              opts.setopt('name', os.path.basename(opts.getopt('defconfig')))
>          config = make_config(opts.vals)
> +
>      if opts.vals.dryrun:
>          PrettyPrint.prettyprint(config)
>      else:
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel