Re: [Xen-devel] [PATCH] Paravirt framebuffer support in xend [3/5]

[Jeremy Katz <katzj@xxxxxxxxxx>]

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

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

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

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

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

Jeremy


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