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

[Jeremy Katz <katzj@xxxxxxxxxx>]

On Thu, 2005-04-14 at 13:20 +0100, Mike Wray wrote:
> 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@xxxxxxxxxx>

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

Thanks for looking over it.

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

It also runs from xend on the domain reboot case.  Right now only in
non-interactive/quiet mode since finding where the console is at that
point looked treacherous.  See the changes to XendDomainInfo.restart.

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

Also handled, if the console isn't connected it runs with quiet mode
which should just pick the default.

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

I thought I had got all of these (from the xend path), but I'll look
again once I get in to the office.  There is some printing/exiting from
the xm side since if you can't find a kernel there, having xm exit with
an error message gives the user some idea of what's going on.

> - handle errors when calling external scripts

Should be easy enough to add a little bit more here.

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

The problem is that if you don't assume the first device is the boot
disk, you essentially have to add a new configuration directive for
specifying what it is.  I'm fine with adding one, but I was just trying
to minimize the number of config changes needed.  I guess it's
reasonable to add a configuration option and default to the first disk
if it's not there.  I'll also do this once I get into the office.

> > +        print >> sys.stderr, "Bootloader isn't executable"
> > +        sys.exit(1)
> 
> Use logging instead of stderr. Don't call exit, raise a XendError.

Yeah, artifact of code moving around as I wrote it (and not having
logging where it originally was).  Will switch.

> > +    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 problem with using popen is that unless I've missed a way of using
it, you replace stdout/stderr for the process you're exec'ing.  Which
isn't what you want if you're going to run an interactive curses
program.

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

Sure, but the returned sxp will be small so it shouldn't make a big
difference one way or another.  
> > +                    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.

Yes, yes it is :)

> Not keen on '%(fn,)' - why not use '% fn'?

%(fn,) is far more explicit and ensures you don't get bit if something
becomes a tuple when you least expect it.  After hitting problems once
or twice, I've gotten to where I'm very pedantic about always ensuring I
specify format strings in this fashion.

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

Yep.

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

I vaguely remember hitting problems trying to do this, but it could well
have been PEBKAC.  Will try.

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

Is there a way to kick off a full initial boot within xend without using
xm?  I saw only reboots.

Thanks for the review, will make the cleanups suggested this morning and
get out a second pass.

Jeremy


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel