Anthony, I've reviewed your XML-RPC patches. It all looks good, so I'm in
favour of putting them in straight away. I'd like to get the second patch in
straight away as well as the first, that is (for those watching at home) to
change xm to using XML-RPC, because I would like to deprecate the old
protocol sooner, rather than later. Making the XML-RPC interface the primary
control path for Xen 3.0.2 will encourage third-parties to code to that rather
than to the s-expression protocol, and that's a good thing.
Here are some specific comments. There's quite a lot here, but nothing that
would prevent both patches going in. We can discuss these things, and if we're
still in disagreement or regard my suggestions as high risk, then we can put
in the two patches as they stand now.
> # HG changeset patch
> # User anthony@xxxxxxxxxxxxxxxxxxxxx
> # Node ID 095ac0d95d9cc154ec8fc3dba1a67f02f79771ac
> # Parent c369d960f96ba4fd7c9c6920cfa60c46a764323c
> Add an XML-RPC interface to Xend.
>
> This introduces a enhanced client library (xmlrpclib2) that supports XML-RPC
> over a domain socket (also working around a bug in Python's handling of NIL).
>
> This also introduces a new server that runs along side of the existing
> S-Expression/HTTP server.
>
> This changeset makes no change to the normal operation of Xend. xm still goes
> through S-Expression/HTTP -- there's just another option for interacting with
> Xend.
>
> Signed-off-by: Anthony Liguori <aliguori@xxxxxxxxxx>
>
> diff -r c369d960f96b -r 095ac0d95d9c tools/python/xen/xend/server/SrvServer.py
> --- a/tools/python/xen/xend/server/SrvServer.py Tue Feb 28 16:45:20 2006
> +++ b/tools/python/xen/xend/server/SrvServer.py Tue Feb 28 22:08:47 2006
> @@ -51,6 +51,7 @@
> from xen.web.SrvDir import SrvDir
>
> from SrvRoot import SrvRoot
> +from XMLRPCServer import XMLRPCServer
>
>
> xroot = XendRoot.instance()
> @@ -113,4 +114,5 @@
> path = xroot.get_xend_unix_path()
> log.info('unix path=' + path)
> servers.add(UnixHttpServer(path=path, root=root))
> + servers.add(XMLRPCServer())
> return servers
It would be prudent to make it possible to turn the XMLRPCServer off, just as we
can turn the other servers off.
> diff -r c369d960f96b -r 095ac0d95d9c tools/python/xen/util/xmlrpclib2.py
> --- /dev/null Tue Feb 28 16:45:20 2006
> +++ b/tools/python/xen/util/xmlrpclib2.py Tue Feb 28 22:08:47 2006
>
> [Snip lots]
>
> +class ServerProxy(xmlrpclib.ServerProxy):
> + def __init__(self, uri, transport=None, encoding=None, verbose=0,
> + allow_none=1):
> + if transport == None:
> + protocol = uri.split(':')[0]
> + if protocol == 'httpu':
> + uri = 'http:' + ':'.join(uri.split(':')[1:])
> + transport = UnixTransport()
How about
(protocol, rest) = uri.split(':', 1)
if protocol == 'httpu':
uri = 'http:' + rest
transport = UnixTransport()
>
> [Snip lots more]
>
> diff -r c369d960f96b -r 095ac0d95d9c
> tools/python/xen/xend/server/XMLRPCServer.py
> --- /dev/null Tue Feb 28 16:45:20 2006
> +++ b/tools/python/xen/xend/server/XMLRPCServer.py Tue Feb 28 22:08:47 2006
> @@ -0,0 +1,97 @@
> +#============================================================================
> +# This library is free software; you can redistribute it and/or
> +# modify it under the terms of version 2.1 of the GNU Lesser General Public
> +# License as published by the Free Software Foundation.
> +#
> +# This library is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +# Lesser General Public License for more details.
> +#
> +# You should have received a copy of the GNU Lesser General Public
> +# License along with this library; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> +#============================================================================
> +# Copyright (C) 2006 Anthony Liguori <aliguori@xxxxxxxxxx>
> +#============================================================================
> +
> +from xen.xend import (XendDomain, XendDomainInfo, XendNode,
> + XendLogging, XendDmesg)
This syntax is only in Python 2.4+, so we have to use
from xen.xend import XendDomain, XendDomainInfo, XendNode, \
XendLogging, XendDmesg
> [Snip]
>
> +def get_log():
> + f = open(XendLogging.getLogFilename(), 'r')
> + ret = f.read()
> + f.close()
> + return ret
This will leak a file descriptor if f.read() throws an exception. We need
f = open(XendLogging.getLogFIlename(), 'r')
try:
return f.read()
finally:
f.close()
> +methods = ['device_create', 'destroyDevice', 'getDeviceSxprs',
> + 'setMemoryTarget', 'setName', 'setVCpuCount', 'shutdown',
> + 'send_sysrq', 'getVCPUInfo', 'waitForDevices']
> +
> +exclude = ['domain_create', 'domain_restore']
> +
> +class XMLRPCServer:
> + def __init__(self):
> + self.ready = False
> +
> + def run(self):
> + self.server = UnixXMLRPCServer("/var/run/xend-xmlrpc.sock")
This filename constant needs to go somewhere else; XendClient is probably the
best place for it. (I've never liked the way that files in xen/xm have to go
poking around in xen/xend for things that they need, but I don't have the
stomach for a major file rearrangement at the moment, so XendClient will have
to do.)
> + # Functions in XendDomainInfo
> + for name in methods:
> + fn = eval("lambda domid, *args: dispatch(domid, '%s',
> args)"%name)
> + self.server.register_function(fn, "xend.domain.%s" %
> name)
> +
> + # Functions in XendDomain
> + inst = XendDomain.instance()
> + for name in dir(inst):
> + fn = getattr(inst, name)
> + if name.startswith("domain_") and callable(fn):
> + if name not in exclude:
> + self.server.register_function(fn,
> "xend.domain.%s" % name[7:])
> +
> + # Functions in XendNode and XendDmesg
> + for type, lst, n in [(XendNode, ['info',
> 'cpu_bvt_slice_set'], 'node'),
> + (XendDmesg, ['info', 'clear'],
> 'node.dmesg')]:
> + inst = type.instance()
> + for name in lst:
> + self.server.register_function(getattr(inst, name),
> + "xend.%s.%s" % (n,
> name))
> +
This is all a bit skanky, and could be easily cleaned up by introducing a
naming convention for XendDomain, XendNode, etc. How about if we prefixed
every function that we wish to expose to the messaging layer with
"public_"? So for example XendDomainInfo.send_sysrq would be named
public_send_sysrq instead. Then, we could use that to guide the
function registration, rather than having exclude lists and inline lists
of callable methods.
This would make your patch a bit more invasive, in that the renaming
would cause it to touch more files, but I don't have a problem with that
-- I think the change is justified.
> [Snip the rest of this patch]
>
> # HG changeset patch
> # User anthony@xxxxxxxxxxxxxxxxxxxxx
> # Node ID 951f0d589164e0cffc785cf23d82118b3e0b0495
> # Parent 095ac0d95d9cc154ec8fc3dba1a67f02f79771ac
> This changeset is a major refactoring of XendClient to use the XML-RPC
> transport for Xend and also to make a few changes to xm so that it knows about
> the new Exception types.
>
> xm-test has been passing with this changeset for the past couple of weeks.
>
> Signed-off-by: Anthony Liguori <aliguori@xxxxxxxxxx>
>
> diff -r 095ac0d95d9c -r 951f0d589164 tools/python/xen/xend/XendClient.py
> --- a/tools/python/xen/xend/XendClient.py Tue Feb 28 22:08:47 2006
> +++ b/tools/python/xen/xend/XendClient.py Tue Feb 28 22:10:14 2006
>
> [Snip lots]
>
> def xend_node_get_dmesg(self):
> - return self.xendGet(self.nodeurl('dmesg'))
> + return self.srv.xend.node.dmesg.info()
What you've done here is slipped the new XML-RPC layer under the existing
XendClient API. I don't think that we need to support that API at all. All
the functions here just turn into stubs onto ServerProxy, and I don't think
that those stubs buy us anything.
What I'd do here is just this:
XendClient.py:
XEND_XMLRPC_UNIX_SOCKET = '/var/run/xend-xmlrpc.sock'
server = ServerProxy('httpu://' + XEND_XMLRPC_UNIX_SOCKET)
and then push _all_ the other code into the client (main.py, create.py, etc).
So these would change from
from xen.xend.XendClient import server
print server.xend_node_get_dmesg()
to
from xen.xend.XendClient import server
print server.xend.node.dmesg.info()
XendClient.py just dissolves into nothingness, and the clients are no more
complicated.
> def xend_domain_sysrq(self, id, key):
> - return self.xendPost(self.domainurl(id),
> - {'op' : 'sysrq',
> - 'key' : key})
> + return self.srv.xend.domain.send_sysrq(self.lookup(id), key)
For calls like this, where we currently look up the domain ID before making the
real call, I would push the lookup to the server; there's no need for two
roundtrips for this kind of call. I think that if you remove the lookup call,
it will just work now in any case, because XMLRPCServer.lookup uses
XendDomain.domain_lookup_by_name_or_id.
> + # FIXME
> + def xend_domain_restore(self, filename):
> + return self.srv.xend.domain.restore(filename)
What does this FIXME mean?
> diff -r 095ac0d95d9c -r 951f0d589164 tools/python/xen/xend/XendDomain.py
> --- a/tools/python/xen/xend/XendDomain.py Tue Feb 28 22:08:47 2006
> +++ b/tools/python/xen/xend/XendDomain.py Tue Feb 28 22:10:14 2006
> @@ -385,7 +385,7 @@
> val = dominfo.destroy()
> else:
> try:
> - val = xc.domain_destroy(domid)
> + val = xc.domain_destroy(int(domid))
> except Exception, ex:
> raise XendError(str(ex))
> return val
I can see why you've done this, but it just led me to wonder why
XendDomain.domain_destroy exists at all. All it's doing is looking up a
domain ID, checking that it's not the privileged domain (in the wrong
order!) calling XendDomainInfo.destroy, and calling xc.domain_destroy in
the case of an exception. We should move the check and the exception
handling into XendDomainInfo.destroy, and then we can dispatch to that
method straight away, without needing XendDomain.domain_destroy at all.
The messaging layer already has the capability to lookup domain IDs --
we should use it.
The reason I noticed is that I have been trying to make it so that
XendDomain and XendDomainInfo only receive arguments of the correct type,
with the casts to integers etc handled by the messaging layer. This
change you've made highlights the fact that there is now nowhere for
that type conversion to take place, because the messaging layer is more
generic, and doesn't understand the calls that it is dispatching.
That's OK -- it's a consequence of the removal of all that code in
SrvDomain -- but it's something to be aware of. If we adopt the naming
convention that I suggest above, then all methods accept only arguments
of the correct type _except_ for those named "public_xyz" -- those
methods must validate and convert their arguments appropriately.
> diff -r 095ac0d95d9c -r 951f0d589164 tools/python/xen/xm/create.py
> --- a/tools/python/xen/xm/create.py Tue Feb 28 22:08:47 2006
> +++ b/tools/python/xen/xm/create.py Tue Feb 28 22:10:14 2006
> @@ -28,11 +28,9 @@
> import time
> import re
>
> -import xen.lowlevel.xc
> -
> from xen.xend import sxp
> from xen.xend import PrettyPrint
> -from xen.xend.XendClient import server, XendError
> +from xen.xend.XendClient import server
> from xen.xend.XendBootloader import bootloader
> from xen.util import blkif
>
> @@ -416,6 +414,8 @@
> def err(msg):
> """Print an error to stderr and exit.
> """
> + import traceback
> + traceback.print_exc()
> print >>sys.stderr, "Error:", msg
> sys.exit(1)
This looks like debug code -- I'm not even sure how this traceback helps
you very much. It should be disabled by default, or removed altogether.
> @@ -810,7 +810,7 @@
> dominfo = server.xend_domain_restore(filename, config)
> else:
> dominfo = server.xend_domain_create(config)
> - except XendError, ex:
> + except Exception, ex:
> import signal
> if vncpid:
> os.kill(vncpid, signal.SIGKILL)
> diff -r 095ac0d95d9c -r 951f0d589164 tools/python/xen/xm/main.py
> --- a/tools/python/xen/xm/main.py Tue Feb 28 22:08:47 2006
> +++ b/tools/python/xen/xm/main.py Tue Feb 28 22:10:14 2006
> @@ -29,8 +29,8 @@
> import socket
> import warnings
> warnings.filterwarnings('ignore', category=FutureWarning)
> -
> -import xen.xend.XendError
> +import xmlrpclib
> +
> import xen.xend.XendProtocol
>
> from xen.xend import PrettyPrint
> @@ -1036,23 +1036,13 @@
> else:
> err("Error connecting to xend: %s." % ex[1])
> sys.exit(1)
> - except xen.xend.XendError.XendError, ex:
> - if len(args) > 0:
> - handle_xend_error(argv[1], args, ex)
> - else:
> - print "Unexpected error:", sys.exc_info()[0]
> - print
> - print "Please report to xen-devel@xxxxxxxxxxxxxxxxxxx"
> - raise
> - except xen.xend.XendProtocol.XendError, ex:
> - if len(args) > 0:
> - handle_xend_error(argv[1], args, ex)
> - else:
> - print "Unexpected error:", sys.exc_info()[0]
> - print
> - print "Please report to xen-devel@xxxxxxxxxxxxxxxxxxx"
> - raise
> except SystemExit:
> + sys.exit(1)
> + except xmlrpclib.Fault, ex:
> +# print "Xend generated an internal fault:"
> +# sys.stderr.write(ex.faultString)
> +# sys.exit(1)
> + print "Error: Internal Xend error"
I expect we can do better than just printing "Internal Xend Error". How
much structure and useful information is there in an xmlrpclib.Fault at
the moment?
> sys.exit(1)
> except:
> print "Unexpected error:", sys.exc_info()[0]
Looks like that's it! Thanks for all your hard work, Anthony, this is
going to make a big difference to Xend's usefulness and maintainability,
and you've done a good job of it. Let's get it into 3.0.2.
Cheers,
Ewan.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home