WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: Mike Wray <mike.wray@xxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] Guest boot loader support [1/2]
From: Jeremy Katz <katzj@xxxxxxxxxx>
Date: Thu, 14 Apr 2005 09:04:16 -0400
Cc: Ian Pratt <m+Ian.Pratt@xxxxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxx
Delivery-date: Thu, 14 Apr 2005 13:06:30 +0000
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <425E5FF4.1070002@xxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <1113448371.5078.50.camel@xxxxxxxxxxxxxx> <425E5FF4.1070002@xxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
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