> On Mon, 2006-09-04 at 10:02 +0100, Steven Smith wrote:
> > > diff -r a2a8f1ed16ea -r 2b360c6b44fa tools/python/xen/xend/image.py
> > > --- a/tools/python/xen/xend/image.py Sat Sep 02 15:22:19 2006 -0400
> > > +++ b/tools/python/xen/xend/image.py Sat Sep 02 15:23:32 2006 -0400
> > > @@ -20,8 +20,10 @@ import os, string
> > > import os, string
> > > import re
> > > import math
> > > +import signal
> > Why?
> Because it's used to kill a process and doing a lazy import of things
> like this is a good way to drive a man crazy ;-)
I'd drop this from this patch, since it's not really required or
particularly useful.
Don't let that stop you from doing a separate cleanup patch,
though. :)
>
> > >
> > > import xen.lowlevel.xc
> > > +import xen.util.auxbin
> > > from xen.xend import sxp
> > > from xen.xend.XendError import VmError
> > > from xen.xend.XendLogging import log
> > > @@ -189,6 +191,68 @@ class LinuxImageHandler(ImageHandler):
> > > cmdline = self.cmdline,
> > > ramdisk = self.ramdisk,
> > > features = self.vm.getFeatures())
> > > +
> > > + def configure(self, imageConfig, deviceConfig):
> > Does this really belong in class LinuxImageHandler?
> Right now, it's only implemented for Linux -- with a proof of concept
> for elsewhere, I could see move it to being generic instead. But right
> now, it's Linux specific
The other PV devices have their own Controller classes
(BlkifController, NetifController, etc.). Why is the framebuffer
special?
> > > + def createDeviceModel(self):
> > Maybe call ImageHandler.createDeviceModel?
> The HVM one doesn't -- perhaps both should although currently the
> comment in the superclass is such that it's not going to define anything
I think that's a bug in the HVM version, personally. I'll have a look
at it later.
> > > @@ -371,7 +435,6 @@ class HVMImageHandler(ImageHandler):
> > >
> > > def destroy(self):
> > > self.unregister_shutdown_watch();
> > > - import signal
> > Why?
> Because we import it once at the top instead of scattering imports all
> over in methods
Again, this really belongs in a separate patch.
> > > +def configure_graphics(config_image, vals):
> > > + """Create the config for graphic consoles.
> > > + """
> > > + args = [ 'vnc', 'vncdisplay', 'vncconsole', 'vncunused',
> > > + 'sdl', 'display', 'xauthority' ]
> > > + for a in args:
> > > + if (vals.__dict__[a]):
> > > + config_image.append([a, vals.__dict__[a]])
> > This looks very wrong. What is it trying to do? Why do these parameters
> > need to be handled differently from the ones in configure_image?
> It's making it so that we have one place to modify the list of graphics
> related arguments instead of keeping one copy in configure_image and one
> copy in configure_hvm. Now, they can both call configure_graphics and
> it's easier to keep things in sync
Your argument would have more force if they actually did both call
configure_graphics.
Steven.
signature.asc
Description: Digital signature
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home