Ewan Mellor wrote:
Anthony, I've reviewed your XML-RPC patches. It all looks good, so I'm in
favour of putting them in straight away.
Thanks!
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.
Okay. So I'm going to take this to mean that we would deprecate
S-Expression/HTTP for 3.0.2 and remove it in 3.0.3? That would be
really nice.
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.
Yeah, this is certainly sane.
+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()
Haven't seen that before. Quite useful :-)
+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
Yeah, thanks for the catch.
[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()
Yeah, again, thanks for the catch :-)
+ 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.)
Okay. I'm fine with that.
+ # 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.
Yes, skanky is very much an understatement. I found XendDomain and
XendDomainInfo particularly nasty to work with considering that they use
different naming conventions too.
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.
Isn't using an underscore a convention for making methods private in
Python? I think, at least, pydoc ignores functions that start with an
underscore.
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.
Yes, that would make things much nicer so I'm happy to do it. I clearly
wanted to avoid making more changes than necessary at first :-)
[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.
Nope. Just compatibility. I know there are people using the XendClient
API. I also know we've never declared that to be a fixed API. I didn't
want to be the one to break it though :-)
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.
Yes, this would be great. If we don't mind breaking XendClient, I'm
happy to do it.
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.
Yeah, lookup() is a bit of a hack. I can go through and audit the
functions to make sure that they all accept either names or IDs.
+ # FIXME
+ def xend_domain_restore(self, filename):
+ return self.srv.xend.domain.restore(filename)
What does this FIXME mean?
Sorry, that shouldn't have been there. I already fixed that one :-)
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.
Ok, sounds sane to me.
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.
Why? If a public_ function expects a domid and gets passed a string, it
ought to throw an exception and the client should get it as an error.
This particular hack was to account for a place in xm where it was
passing a string version of the domain ID. The real problem here was
the xm code. I wanted to avoid modifying that code though to
demonstrate that the XML-RPC stuff could be a drop-in-replacement. Of
course, now that that exercise is complete, there's no reason not to go
in and fix the original source of the problem.
@@ -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.
Yeah, sorry, I missed that.
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?
We can pass whatever we want. xmlrpclib.Fault contain an integer code
and a string message. For now, I'm just always passing a 1 for the
faultCode and sending the Xend traceback as the faultMessage.
I'm not sure how fancy we want to get for the first drop of this code.
Any suggestions?
BTW, it's printing 'Internal Xend Error' because xm-test has a test case
that checks for that specific result :-)
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.
Thanks for looking at the code! I don't think there's that much change
necessary. I should be able to turn around another patch in a couple days.
Regards,
Anthony Liguori
Cheers,
Ewan.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home