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