From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steven Smith Subject: Re: [PATCH] Paravirt framebuffer support in xend [3/5] Date: Wed, 6 Sep 2006 10:17:23 +0100 Message-ID: <20060906091722.GE3257@cam.ac.uk> References: <1157227117.11059.43.camel@aglarond.local> <20060904090209.GD4812@cam.ac.uk> <1157472717.7571.89.camel@aglarond.local> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2007922440==" Return-path: In-Reply-To: <1157472717.7571.89.camel@aglarond.local> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Mime-version: 1.0 Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Jeremy Katz Cc: xen-devel , Markus Armbruster , sos22@srcf.ucam.org List-Id: xen-devel@lists.xenproject.org --===============2007922440== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="Q8BnQc91gJZX4vDc" Content-Disposition: inline --Q8BnQc91gJZX4vDc Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable > 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. :) >=20 > > > =20 > > > 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 =3D self.cmdline, > > > ramdisk =3D self.ramdisk, > > > features =3D 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): > > > =20 > > > 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 =3D [ '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 paramete= rs > > 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. --Q8BnQc91gJZX4vDc Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.5 (GNU/Linux) iD8DBQFE/pIiO4S8/gLNrjcRAhOfAKC0b793vfBX/KEYlukOnQTODn2GogCgnZ1b EZbcZnFwD7vfIxFXbbFVpIY= =sNRX -----END PGP SIGNATURE----- --Q8BnQc91gJZX4vDc-- --===============2007922440== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel --===============2007922440==--