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

[Steven Smith <sos22-xen@srcf.ucam.org>]

[-- Attachment #1.1: Type: text/plain, Size: 3181 bytes --]

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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