RE: [RFC] Xend XML-RPC Refactoring

RE: [RFC] Xend XML-RPC Refactoring

[Ian Pratt]

 
> > We would like to simplify Xend by utilizing more of the standard 
> > Python library and relying on less of our own code.  The 
> most obvious 
> > thing here is the current S-Expression/HTTP RPC interface.  
> We would 
> > like to replace this with XML-RPC using Python's builtin support 
> > (xmlrpclib and SimpleXMLRPCServer).
> > 
> > I've got some initial code and more details on the wiki 
> > (http://wiki.xensource.com/xenwiki/Xend/XML-RPC).  Early 
> estimates are 
> > that this would reduce the code in Xend by about 33% (5k 
> slocs).  We 
> > would also like to standardize this XML-RPC interface so that 
> > third-parties write apps to this interface without worrying about 
> > massive breakage.

Anothony, have you an updated version of this patch? How much testing
has it had?

Should we be looking to get it in prior to 3.0.2? In principle I think
this would be a good thing, but we may have left it too late. I expect
2.6.16 final will be out fairly soon, and then we'll want to be pushing
a release out in the following week or so.

Thanks,
Ian

Re: [RFC] Xend XML-RPC Refactoring

[Anthony Liguori]

[-- Attachment #1: Type: text/plain, Size: 1466 bytes --]

Ian Pratt wrote:
>>> would also like to standardize this XML-RPC interface so that 
>>> third-parties write apps to this interface without worrying about 
>>> massive breakage.
>>>       
>
> Anothony, have you an updated version of this patch? How much testing
> has it had?
>   
Yes, I sent those to xen-devel just the other day.  They are attached to 
this note for your convenience.

I've done a bunch of testing (via xm-test) on x86-32 and limited on x86-64.
> Should we be looking to get it in prior to 3.0.2?
I think the patch that adds an XML-RPC interface to Xend ought to go 
in.  It doesn't change any of the normally executed code paths for xm 
and only listens on a domain socket.  This would give us a chance to 
start supporting XML-RPC in libvirt and to bang on the interface a 
little more.

If 3.0.2 is going out shortly, I'd say hold off on the second patch 
until after it goes out.  There's no real benefit with the second patch 
other than getting more people to exercise the code so it's probably not 
worth taking the risk (although as I said, I've tested it pretty heavily).
>  In principle I think
> this would be a good thing, but we may have left it too late. I expect
> 2.6.16 final will be out fairly soon, and then we'll want to be pushing
> a release out in the following week or so.
>   
I split up the patches specifically with this in mind.  The first one 
should be very low-risk.

Regards,

Anthony Liguori
> Thanks,
> Ian
>   


[-- Attachment #2: xend-xmlrpc-9017.diff --]
[-- Type: text/plain, Size: 9840 bytes --]

# HG changeset patch
# User anthony@rhesis.austin.ibm.com
# 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@us.ibm.com>

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
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
@@ -0,0 +1,109 @@
+#============================================================================
+# 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@us.ibm.com>
+#============================================================================
+
+"""
+An enhanced XML-RPC client/server interface for Python.
+"""
+
+from httplib import HTTPConnection, HTTP
+from xmlrpclib import Transport
+from SimpleXMLRPCServer import SimpleXMLRPCServer, SimpleXMLRPCRequestHandler
+import xmlrpclib, socket, os, traceback
+
+# A new ServerProxy that also supports httpu urls.  An http URL comes in the
+# form:
+#
+# httpu:///absolute/path/to/socket.sock
+#
+# It assumes that the RPC handler is /RPC2.  This problem needs to be improved
+
+class HTTPUnixConnection(HTTPConnection):
+    def connect(self):
+        self.sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
+        self.sock.connect(self.host)
+
+class HTTPUnix(HTTP):
+    _connection_class = HTTPUnixConnection
+
+class UnixTransport(Transport):
+    def request(self, host, handler, request_body, verbose=0):
+        self.__handler = handler
+        return Transport.request(self, host, '/RPC2', request_body, verbose)
+    def make_connection(self, host):
+        return HTTPUnix(self.__handler)
+
+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()
+        xmlrpclib.ServerProxy.__init__(self, uri, transport, encoding,
+                                       verbose, allow_none)
+
+# This is a base XML-RPC server for TCP.  It sets allow_reuse_address to
+# true, and has an improved marshaller that serializes unknown exceptions
+# with full traceback information.
+
+class TCPXMLRPCServer(SimpleXMLRPCServer):
+    allow_reuse_address = True
+
+    def _marshaled_dispatch(self, data, dispatch_method = None):
+        params, method = xmlrpclib.loads(data)
+        try:
+            if dispatch_method is not None:
+                response = dispatch_method(method, params)
+            else:
+                response = self._dispatch(method, params)
+
+            response = (response,)
+            response = xmlrpclib.dumps(response,
+                                       methodresponse=1,
+                                       allow_none=1)
+        except xmlrpclib.Fault, fault:
+            response = xmlrpclib.dumps(exc)
+        except:
+            response = xmlrpclib.dumps(
+                xmlrpclib.Fault(1, traceback.format_exc())
+                )
+
+        return response
+
+# This is a XML-RPC server that sits on a Unix domain socket.
+# It implements proper support for allow_reuse_address by
+# unlink()'ing an existing socket.
+
+class UnixXMLRPCRequestHandler(SimpleXMLRPCRequestHandler):
+    def address_string(self):
+        try:
+            return SimpleXMLRPCRequestHandler.address_string(self)
+        except ValueError, e:
+            return self.client_address[:2]
+
+class UnixXMLRPCServer(TCPXMLRPCServer):
+    address_family = socket.AF_UNIX
+
+    def __init__(self, addr, requestHandler=UnixXMLRPCRequestHandler,
+                 logRequests=1):
+        if self.allow_reuse_address:
+            try:
+                os.unlink(addr)
+            except OSError, exc:
+                pass
+        TCPXMLRPCServer.__init__(self, addr, requestHandler, logRequests)
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@us.ibm.com>
+#============================================================================
+
+from xen.xend import (XendDomain, XendDomainInfo, XendNode,
+                      XendLogging, XendDmesg)
+from xen.util.xmlrpclib2 import UnixXMLRPCServer
+
+def lookup(domid):
+    return XendDomain.instance().domain_lookup_by_name_or_id(domid)
+
+def dispatch(domid, fn, args):
+    info = lookup(domid)
+    return getattr(info, fn)(*args)
+
+def domain(domid):
+    info = lookup(domid)
+    return info.sxpr()
+
+def domains(detail=1):
+    if detail < 1:
+        return XendDomain.instance().list_names()
+    else:
+        domains = XendDomain.instance().list_sorted()
+        return map(lambda dom: dom.sxpr(), domains)
+
+def domain_create(config):
+    info = XendDomain.instance().domain_create(config)
+    return info.sxpr()
+
+def domain_restore(src):
+    info = XendDomain.instance().domain_restore(src)
+    return info.sxpr()    
+
+def get_log():
+    f = open(XendLogging.getLogFilename(), 'r')
+    ret = f.read()
+    f.close()
+    return ret
+
+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")
+
+        # 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))
+
+        # A few special cases
+        self.server.register_function(domain, 'xend.domain')
+        self.server.register_function(domains, 'xend.domains')
+        self.server.register_function(get_log, 'xend.node.log')
+        self.server.register_function(domain_create, 'xend.domain.create')
+        self.server.register_function(domain_restore, 'xend.domain.restore')
+
+        self.server.register_introspection_functions()
+        self.ready = True
+        self.server.serve_forever()

[-- Attachment #3: xend-xmlrpc-9018.diff --]
[-- Type: text/plain, Size: 19947 bytes --]

# HG changeset patch
# User anthony@rhesis.austin.ibm.com
# 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@us.ibm.com>

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
@@ -16,22 +16,8 @@
 # Copyright (C) 2004, 2005 Mike Wray <mike.wray@hp.com>
 #============================================================================
 
-"""Client API for the HTTP interface on xend.
-Callable as a script - see main().
-Supports inet or unix connection to xend.
-
-This API is the 'control-plane' for xend.
-The 'data-plane' is done separately.
-"""
-import os
-import sys
-import types
-
-import sxp
-import PrettyPrint
-from XendProtocol import HttpXendClientProtocol, \
-                         UnixXendClientProtocol, \
-                         XendError
+import types, sxp
+from xen.util.xmlrpclib2 import ServerProxy
 
 def fileof(val):
     """Converter for passing configs or other 'large' data.
@@ -39,377 +25,120 @@
     Assumes a string is a file name and passes its contents.
     """
     if isinstance(val, types.ListType):
-        return sxp.to_string(val)
+        return val
     if isinstance(val, types.StringType):
-        return file(val)
-    if hasattr(val, 'readlines'):
-        return val
-    raise XendError('cannot convert value')
-
-class URL:
-    """A URL.
-    """
-
-    def __init__(self, proto='http', host='localhost', port=None, path='', query=None, frag=None):
-        self.proto = proto
-        self.host = host
-        if port: port = int(port)
-        self.port = port
-        self.path = path
-        self.query = query
-        self.frag = frag
-
-    def url(self):
-        """Get the full URL string including protocol, location and the full path.
-        """
-        return self.proto + '://' + self.location() + self.fullpath()
-
-    def location(self):
-        """Get the location part of the URL, including host and port, if present.
-        """
-        if self.port:
-            return self.host + ':' + str(self.port)
-        else:
-            return self.host
-
-    def fullpath(self):
-        """Get the full path part of the URL, including query and fragment if present.
-        """
-        u = [ self.path ]
-        if self.query:
-            u.append('?')
-            u.append(self.query)
-        if self.frag:
-            u.append('#')
-            u.append(self.frag)
-        return ''.join(u)
-
-    def relative(self, path='', query=None, frag=None):
-        """Create a URL relative to this one.
-        """
-        return URL(proto=self.proto,
-                   host=self.host,
-                   port=self.port,
-                   path=self.path + path,
-                   query=query,
-                   frag=frag)
+        return sxp.from_string(val)
+    raise ValueError('invalid value for config')
 
 class Xend:
     """Client interface to Xend.
     """
 
-    """Default location of the xend server."""
-    SRV_DEFAULT = "localhost:8000"
-
-    """Environment variable to set the location of xend."""
-    SRV_VAR = "XEND"
-
-    """Default path to the xend root on the server."""
-    ROOT_DEFAULT = "/xend/"
-
-    """Environment variable to set the xend root path."""
-    ROOT_VAR = "XEND_ROOT"
-
     def __init__(self, client=None, srv=None, root=None):
-        """Create a xend client interface.
-        If the client protocol is not specified, the default
-        is to use a synchronous protocol.
-
-        @param client:  client protocol to use
-        @param srv:     server host, and optional port (format host:port)
-        @param root:    xend root path on the server
-        """
-        if client is None:
-            client = HttpXendClientProtocol()
-        self.client = client
-        self.bind(srv, root)
-
-    def default_server(self):
-        """Get the default location of the xend server.
-        """
-        return os.getenv(self.SRV_VAR, self.SRV_DEFAULT)
-
-    def default_root(self):
-        """Get the default root path on the xend server.
-        """
-        return os.getenv(self.ROOT_VAR, self.ROOT_DEFAULT)
-
-    def bind(self, srv=None, root=None):
-        """Bind to a given server.
-
-        @param srv:  server location (host:port)
-        @param root: xend root path on the server
-        """
-        if srv is None: srv = self.default_server()
-        if root is None: root = self.default_root()
-        if not root.endswith('/'): root += '/'
-        (host, port) = srv.split(':', 1)
-        self.url = URL(host=host, port=port, path=root)
-
-    def xendGet(self, url, args=None):
-        return self.client.xendGet(url, args)
-
-    def xendPost(self, url, data):
-        return self.client.xendPost(url, data)
-
-    def nodeurl(self, id=''):
-        return self.url.relative('node/' + str(id))
-
-    def domainurl(self, id=''):
-        return self.url.relative('domain/' + str(id))
-
-    def deviceurl(self, id=''):
-        return self.url.relative('device/' + str(id))
-
-    def vneturl(self, id=''):
-        return self.url.relative('vnet/' + str(id))
-
-    def xend(self):
-        return self.xendGet(self.url)
+        self.srv = ServerProxy('httpu:///var/run/xend-xmlrpc.sock')
 
     def xend_node(self):
-        return self.xendGet(self.nodeurl())
+        return self.srv.xend.node.info()
         
-    def xend_node_shutdown(self):
-        return self.xendPost(self.nodeurl(),
-                             {'op'      : 'shutdown'})
-                
-    def xend_node_restart(self):
-        return self.xendPost(self.nodeurl(),
-                             {'op'      : 'reboot'})
-
     def xend_node_get_dmesg(self):
-            return self.xendGet(self.nodeurl('dmesg'))
+        return self.srv.xend.node.dmesg.info()
 
     def xend_node_clear_dmesg(self):
-        return self.xendPost(self.nodeurl('dmesg'),
-                             {'op' : 'clear' } )
+        return self.srv.xend.node.dmesg.clear()
 
     def xend_node_log(self):
-        return self.xendGet(self.nodeurl('log'))
+        return self.srv.xend.node.log()
 
     def xend_node_cpu_bvt_slice_set(self, ctx_allow):
-        return self.xendPost(self.nodeurl(),
-                             {'op'      : 'cpu_bvt_slice_set',
-                              'ctx_allow' : ctx_allow })
+        return self.srv.xend.node.cpu_bvt_slice_set(ctx_allow)
 
     def xend_domains(self):
-        return self.xendGet(self.domainurl())
+        return self.srv.xend.domains(0)
 
     def xend_list_domains(self):
-        return self.xendGet(self.domainurl(), {'detail': '1'})
+        return self.srv.xend.domains(1)
 
     def xend_domain_vcpuinfo(self, dom):
-        return self.xendGet(self.domainurl(dom), {'op': 'vcpuinfo'})
+        return self.srv.xend.domain.getVCPUInfo(dom)
 
     def xend_domain_create(self, conf):
-        return self.xendPost(self.domainurl(),
-                             {'op'      : 'create',
-                              'config'  : fileof(conf) })
+        return self.srv.xend.domain.create(fileof(conf))
 
+    # FIXME
     def xend_domain_restore(self, filename):
-        return self.xendPost(self.domainurl(),
-                             {'op'      : 'restore',
-                              'file'    : filename })
+        return self.srv.xend.domain.restore(filename)
 
     def xend_domain_configure(self, id, conf):
-        return self.xendPost(self.domainurl(id),
-                             {'op'      : 'configure',
-                              'config'  : fileof(conf) })
+        return self.srv.xend.domain.configure(id, fileof(conf))
 
     def xend_domain(self, id):
-        return self.xendGet(self.domainurl(id))
+        return self.srv.xend.domain(id)
 
     def xend_domain_wait_for_devices(self, id):
-        return self.xendPost(self.domainurl(id),
-                             {'op'      : 'wait_for_devices' })
+        try:
+            self.srv.xend.domain.waitForDevices(id)
+            return 0
+        except:
+            return -1
+
+    def lookup(self, id):
+        info = self.srv.xend.domain(id)
+        return int(sxp.child_value(info, 'domid'))
 
     def xend_domain_unpause(self, id):
-        return self.xendPost(self.domainurl(id),
-                             {'op'      : 'unpause' })
+        self.srv.xend.domain.unpause(self.lookup(id))
+        return 0
 
     def xend_domain_pause(self, id):
-        return self.xendPost(self.domainurl(id),
-                             {'op'      : 'pause' })
+        self.srv.xend.domain.pause(self.lookup(id))
+        return 0
 
     def xend_domain_rename(self, id, name):
-        return self.xendPost(self.domainurl(id),
-                             {'op'      : 'rename',
-                              'name'    : name})
+        return self.srv.xend.domain.setName(id, name)
 
     def xend_domain_shutdown(self, id, reason):
-        return self.xendPost(self.domainurl(id),
-                             {'op'      : 'shutdown',
-                              'reason'  : reason})
+        return self.srv.xend.domain.shutdown(id, reason)
 
     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)
 
     def xend_domain_destroy(self, id):
-        return self.xendPost(self.domainurl(id),
-                             {'op'      : 'destroy' })
+        return self.srv.xend.domain.destroy(self.lookup(id))
 
     def xend_domain_save(self, id, filename):
-        return self.xendPost(self.domainurl(id),
-                             {'op'      : 'save',
-                              'file'    : filename })
+        return self.srv.xend.domain.save(self.lookup(id), filename)
 
     def xend_domain_migrate(self, id, dst, live=0, resource=0, port=0):
-        return self.xendPost(self.domainurl(id),
-                             {'op'         : 'migrate',
-                              'destination': dst,
-                              'live'       : live,
-                              'resource'   : resource,
-                              'port'       : port })
+        return self.srv.xend.domain.migrate(self.lookup(id), dst, live, resource, port)
 
     def xend_domain_pincpu(self, id, vcpu, cpumap):
-        return self.xendPost(self.domainurl(id),
-                             {'op'      : 'pincpu',
-                              'vcpu'    : vcpu,
-                              'cpumap'  : str(cpumap) })
+        return self.srv.xend.domain.pincpu(self.lookup(id), vcpu, str(cpumap))
 
     def xend_domain_cpu_bvt_set(self, id, mcuadv, warpback, warpvalue, warpl, warpu):
-        return self.xendPost(self.domainurl(id),
-                             {'op'       : 'cpu_bvt_set',
-                              'mcuadv'   : mcuadv,
-                              'warpback' : warpback,
-                              'warpvalue': warpvalue,
-                              'warpl'    : warpl,
-                              'warpu'    : warpu })
+        return self.srv.xend.domain.cpu_bvt_set(mcuadv, warpback, warpvalue, warpl, warpu)
 
     def xend_domain_cpu_sedf_get(self, id):
-        return self.xendPost(self.domainurl(id),
-                             {'op' : 'cpu_sedf_get'})
+        return self.srv.xend.domain.cpu_sedf_get(self.lookup(id))
 
     def xend_domain_cpu_sedf_set(self, id, period, slice, latency, extratime, weight):
-        return self.xendPost(self.domainurl(id),
-                             {'op'        : 'cpu_sedf_set',
-                              'period'    : period,
-                              'slice'     : slice,
-			      'latency'   : latency,
-			      'extratime' : extratime,
-			      'weight'    : weight })
+        return self.srv.xend.domain.cpu_sedf_set(self.lookup(id), period, slice, latency, extratime, weight)
 
     def xend_domain_maxmem_set(self, id, memory):
-        return self.xendPost(self.domainurl(id),
-                             { 'op'      : 'maxmem_set',
-                               'memory'  : memory })
+        return self.srv.xend.domain.maxmem_set(self.lookup(id), memory)
 
     def xend_domain_mem_target_set(self, id, mem_target):
-        val = self.xendPost(self.domainurl(id),
-                            {'op'        : 'mem_target_set',
-                             'target'    : mem_target })
-        return val
+        return self.srv.xend.domain.setMemoryTarget(id, mem_target)
 
     def xend_domain_set_vcpus(self, dom, vcpus):
-        return self.xendPost(self.domainurl(dom),
-                            {'op'    : 'set_vcpus',
-                             'vcpus' : vcpus })
+        return self.srv.xend.domain.setVCpuCount(self.lookup(dom), vcpus)
 
     def xend_domain_devices(self, id, type):
-        return self.xendPost(self.domainurl(id),
-                             {'op'      : 'devices',
-                              'type'    : type })
+        return self.srv.xend.domain.getDeviceSxprs(id, type)
 
     def xend_domain_device_create(self, id, config):
-        return self.xendPost(self.domainurl(id),
-                             {'op'      : 'device_create',
-                              'config'  : fileof(config) })
-
-    def xend_domain_device_refresh(self, id, type, dev):
-        return self.xendPost(self.domainurl(id),
-                             {'op'      : 'device_refresh',
-                              'type'    : type,
-                              'dev'     : dev })
+        return self.srv.xend.domain.device_create(id, fileof(config))
 
     def xend_domain_device_destroy(self, id, type, dev):
-        return self.xendPost(self.domainurl(id),
-                             {'op'      : 'device_destroy',
-                              'type'    : type,
-                              'dev'     : dev })
+        return self.srv.xend.domain.destroyDevice(id, type, dev)
 
-    def xend_domain_device_configure(self, id, config, dev):
-        return self.xendPost(self.domainurl(id),
-                             {'op'      : 'device_configure',
-                              'dev'     : dev,
-                              'config'  : fileof(config) })
-
-    def xend_vnets(self):
-        return self.xendGet(self.vneturl())
-
-    def xend_vnet_create(self, conf):
-        return self.xendPost(self.vneturl(),
-                             {'op'      : 'create',
-                              'config'  : fileof(conf) })
-
-    def xend_vnet(self, id):
-        return self.xendGet(self.vneturl(id))
-
-    def xend_vnet_delete(self, id):
-        return self.xendPost(self.vneturl(id),
-                              {'op'     : 'delete' })
-
-def getHttpServer(srv=None):
-    """Create and return a xend client.
-    """
-    return Xend(srv=srv, client=HttpXendClientProtocol())
-
-def getUnixServer(srv=None):
-    """Create and return a unix-domain xend client.
-    """
-    return Xend(client=UnixXendClientProtocol(srv))
-
-def xendmain(srv, fn, args, unix=False):
-    if unix:
-        xend = getUnixServer(srv)
-    else:
-        xend = getHttpServer(srv)
-    xend.rc = 0
-    try:
-        v = getattr(xend, fn)(*args)
-        PrettyPrint.prettyprint(v)
-        return 0
-    except XendError, err:
-        print 'ERROR:', err
-        return 1
-
-def main(argv):
-    """Call an API function:
-
-    python XendClient.py fn args...
-
-    The leading 'xend_' on the function can be omitted.
-    Example:
-
-python XendClient.py domains
-    (0 8)
-python XendClient.py domain 0
-    (domain (id 0) (name Domain-0) (memory 128))
-    """
-    from getopt import getopt
-    short_options = 'x:au:d'
-    long_options = ['xend=', 'unix=', 'debug']
-    (options, args) = getopt(argv[1:], short_options, long_options)
-    srv = None
-    unix = 1
-    for k, v in options:
-        if k in ['-x', '--xend']:
-            srv = v
-        elif k in ['-u', '--unix']:
-            unix = int(v)
-    if len(args):
-        fn = args[0]
-        args = args[1:]
-    else:
-        fn = 'xend'
-        args = []
-    if not fn.startswith('xend'):
-        fn = 'xend_' + fn
-    sys.exit(xendmain(srv, fn, args, unix=unix))
-
-if __name__ == "__main__":
-    main(sys.argv)
-else:    
-    server = getUnixServer()
+server = Xend()
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       
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)
 
@@ -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@lists.xensource.com"
-                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@lists.xensource.com"
-                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"
             sys.exit(1)
         except:
             print "Unexpected error:", sys.exc_info()[0]

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

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

Re: [RFC] Xend XML-RPC Refactoring

[Ewan Mellor]

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@rhesis.austin.ibm.com
> # 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@us.ibm.com>
> 
> 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@us.ibm.com>
> +#============================================================================
> +
> +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@rhesis.austin.ibm.com
> # 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@us.ibm.com>
> 
> 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@lists.xensource.com"
> -                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@lists.xensource.com"
> -                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.

Re: [RFC] Xend XML-RPC Refactoring

[Daniel Veillard]

On Sat, Mar 11, 2006 at 08:55:10PM +0000, 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.  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.

  +1 we will try to switch ASAP too at the libvirt level.

> It would be prudent to make it possible to turn the XMLRPCServer off, just as we
> can turn the other servers off.

  Big dilemna :-) 
Currently with http on anybody can manipulate any Xen instance as soon
as one has Xen0 access (I run my libvirt regression test from my normal
user login but except for the convenience it's a bit scary :-). I assume
It will be exactly the same with XML-RPC. I would feel way better if we
could have a policy model which is not all on or all off implemented,
for example associating users with domains, and controlling per user
resource usage. I know that the obvious answer would be "only root should
do that" except that a frustated user needing his VM could then launch
a qemu instance instead and that wouldn't be any better for the server.
IMHO a smarter, more fine grained approach is needed, it will also require
authentication at connection time to the service, and even if not trivial
this should be doable and we use authenticated XML-RPC call all the time
for the Red Hat Network framework, so this has to be possible locally and
remotely.

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

  hum, do we really need to invent a new transport for local access ?
I don't remember seeing 'httpu' anywhere, sounds weird to me

[...]
> >          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?

  Another good question, as we are defining an API to Xend I think
the error handling question will become more and more important. The client
will need more structure and informations about processing error, the 
full stack trace within xend might not be that useful (except for low level
debugging) to the clients, but currently I just extract a string like
  "No such domain test"
which is a bit hard to process correctly even in context. A list of predefined
error code and meaning could help quite a bit along with just the error message.

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

  Yup, thanks Anthony :-) !

Daniel

-- 
Daniel Veillard      | Red Hat http://redhat.com/
veillard@redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/

Re: [RFC] Xend XML-RPC Refactoring

[Ewan Mellor]

On Sat, Mar 11, 2006 at 04:36:57PM -0500, Daniel Veillard wrote:

> > It would be prudent to make it possible to turn the XMLRPCServer off, just as we
> > can turn the other servers off.
> 
>   Big dilemna :-) 
> Currently with http on anybody can manipulate any Xen instance as soon
> as one has Xen0 access (I run my libvirt regression test from my normal
> user login but except for the convenience it's a bit scary :-). I assume
> It will be exactly the same with XML-RPC. I would feel way better if we
> could have a policy model which is not all on or all off implemented,
> for example associating users with domains, and controlling per user
> resource usage. I know that the obvious answer would be "only root should
> do that" except that a frustated user needing his VM could then launch
> a qemu instance instead and that wouldn't be any better for the server.
> IMHO a smarter, more fine grained approach is needed, it will also require
> authentication at connection time to the service, and even if not trivial
> this should be doable and we use authenticated XML-RPC call all the time
> for the Red Hat Network framework, so this has to be possible locally and
> remotely.

Turning the server on and off is just for basic sanity -- the admin gets
to choose which ports they want open so that they don't have to audit
aspects in which they aren't interested.

Of course, a full user / permissions system for the protocol would be a
good idea, but like you say, it's not trivial work.  We could kick that
discussion off if you want, but it's going to need someone to design
the permissions semantics, management of users and roles, etc.  Is
anyone interested in starting this?

At the moment, the XML-RPC is over a Unix domain socket, so only root
can use it anyway (as controlled by the permission on the socket file).

> > > 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()
> 
>   hum, do we really need to invent a new transport for local access ?
> I don't remember seeing 'httpu' anywhere, sounds weird to me

httpu is HTTP over a unix domain socket, as opposed to over TCP.  It's
used elsewhere (though not widely, obviously).  It gives you basic
protection from non-root users (see your complaint above).

> [...]
> > >          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?
> 
>   Another good question, as we are defining an API to Xend I think
> the error handling question will become more and more important. The client
> will need more structure and informations about processing error, the 
> full stack trace within xend might not be that useful (except for low level
> debugging) to the clients, but currently I just extract a string like
>   "No such domain test"
> which is a bit hard to process correctly even in context. A list of predefined
> error code and meaning could help quite a bit along with just the error message.

Sure.  Do you have a list of error codes already (for libvirt, for
example)?

Ewan.

Re: [RFC] Xend XML-RPC Refactoring

[Anthony Liguori]

Ewan Mellor wrote:
> On Sat, Mar 11, 2006 at 04:36:57PM -0500, Daniel Veillard wrote:
>
> Turning the server on and off is just for basic sanity -- the admin gets
> to choose which ports they want open so that they don't have to audit
> aspects in which they aren't interested.
>
> Of course, a full user / permissions system for the protocol would be a
> good idea, but like you say, it's not trivial work.  We could kick that
> discussion off if you want, but it's going to need someone to design
> the permissions semantics, management of users and roles, etc.  Is
> anyone interested in starting this?
>   
We can do basic authentication and require root credentials as a start.  
That's really easy to do but I'm not sure sending the root password as 
clear text really makes things much better :-)

Python, unfortunately, doesn't have server-side SSL bindings so basic 
authentication over SSL is not an option without some custom OpenSSL 
bindings.

Regards,

Anthony Liguori
> At the moment, the XML-RPC is over a Unix domain socket, so only root
> can use it anyway (as controlled by the permission on the socket file).
>
>   
>>>> 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()
>>>       
>>   hum, do we really need to invent a new transport for local access ?
>> I don't remember seeing 'httpu' anywhere, sounds weird to me
>>     
>
> httpu is HTTP over a unix domain socket, as opposed to over TCP.  It's
> used elsewhere (though not widely, obviously).  It gives you basic
> protection from non-root users (see your complaint above).
>
>   
>> [...]
>>     
>>>>          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?
>>>       
>>   Another good question, as we are defining an API to Xend I think
>> the error handling question will become more and more important. The client
>> will need more structure and informations about processing error, the 
>> full stack trace within xend might not be that useful (except for low level
>> debugging) to the clients, but currently I just extract a string like
>>   "No such domain test"
>> which is a bit hard to process correctly even in context. A list of predefined
>> error code and meaning could help quite a bit along with just the error message.
>>     
>
> Sure.  Do you have a list of error codes already (for libvirt, for
> example)?
>
> Ewan.
>   

Re: [RFC] Xend XML-RPC Refactoring

[Daniel Veillard]

On Sat, Mar 11, 2006 at 07:46:35PM -0600, Anthony Liguori wrote:
> Ewan Mellor wrote:
> >On Sat, Mar 11, 2006 at 04:36:57PM -0500, Daniel Veillard wrote:
> >
> >Turning the server on and off is just for basic sanity -- the admin gets
> >to choose which ports they want open so that they don't have to audit
> >aspects in which they aren't interested.
> >
> >Of course, a full user / permissions system for the protocol would be a
> >good idea, but like you say, it's not trivial work.  We could kick that
> >discussion off if you want, but it's going to need someone to design
> >the permissions semantics, management of users and roles, etc.  Is
> >anyone interested in starting this?
> >  
> We can do basic authentication and require root credentials as a start.  
> That's really easy to do but I'm not sure sending the root password as 
> clear text really makes things much better :-)
> 
> Python, unfortunately, doesn't have server-side SSL bindings so basic 
> authentication over SSL is not an option without some custom OpenSSL 
> bindings.

  Hum, I could check how much "custom" is required, I remember doing
that a long time ago but I could ask around :-) if interested.

Daniel

-- 
Daniel Veillard      | Red Hat http://redhat.com/
veillard@redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/

Re: [RFC] Xend XML-RPC Refactoring

[Daniel Veillard]

On Sat, Mar 11, 2006 at 10:20:53PM +0000, Ewan Mellor wrote:
> Of course, a full user / permissions system for the protocol would be a
> good idea, but like you say, it's not trivial work.  We could kick that
> discussion off if you want, but it's going to need someone to design
> the permissions semantics, management of users and roles, etc.  Is
> anyone interested in starting this?

  We need to make sure we can have authentication based on the transport
used, otherwise this need to be added at the RPC level (and hence changes
the API).

> At the moment, the XML-RPC is over a Unix domain socket, so only root
> can use it anyway (as controlled by the permission on the socket file).

  To me that's a big regression. That mean libvirt can't be used anymore 
to just monitor the Xen instance(s) without priviledged access. Monitoring
should not require root priviledge IMHO, ps aux and top are not restricted
to root on Unix. And if you make Xen virtualization a sysadmin only feature
you restrain massively the area of usage IMHO, people will just use
something else (as stated QEmu requires no sysadmin priviledge, and if using
Xen can't be integrated seamlessly in the user workflow, they will switch to
something else).
  Well over (modern) unix socket one can extract the UID of the connecting
client, can we extract that from Python ? (c.f. LOCAL_CREDS/SO_PEERCRED)
If yes then that's a good first step toward local authentication without
messing with https and credentials.

> httpu is HTTP over a unix domain socket, as opposed to over TCP.  It's
> used elsewhere (though not widely, obviously).  It gives you basic
> protection from non-root users (see your complaint above).

  It's not a complaint, I'm raising a problem, which IMHO is different.
Adding core piece of OS machinery without thinking of security and 
authentication aspects from the start is a mistake that had been done on
Unix already, it would be great to not go though the mess again just
because we are eager to push a solution, the subsequent costs could
reduce the benefits from the effort getting there ;-)

> > > 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?
> > 
> >   Another good question, as we are defining an API to Xend I think
> > the error handling question will become more and more important. The client
> > will need more structure and informations about processing error, the 
> > full stack trace within xend might not be that useful (except for low level
> > debugging) to the clients, but currently I just extract a string like
> >   "No such domain test"
> > which is a bit hard to process correctly even in context. A list of predefined
> > error code and meaning could help quite a bit along with just the error message.
> 
> Sure.  Do you have a list of error codes already (for libvirt, for
> example)?

  Yes but for the moment they can't capture what's happening within Xend,
so if the RPC fails they show up just as being from within Xend, with
either a GET or POST information and the string found in the content of
the HTTP answer:
    http://libvirt.org/html/libvirt-virterror.html#virErrorNumber
 associated docs:
    http://libvirt.org/errors.html

Daniel

-- 
Daniel Veillard      | Red Hat http://redhat.com/
veillard@redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/

Re: [RFC] Xend XML-RPC Refactoring

[Anthony Liguori]

On Sun, 12 Mar 2006 04:57:02 -0500, Daniel Veillard wrote:

> On Sat, Mar 11, 2006 at 10:20:53PM +0000, Ewan Mellor wrote:
>> Of course, a full user / permissions system for the protocol would be a
>> good idea, but like you say, it's not trivial work.  We could kick that
>> discussion off if you want, but it's going to need someone to design
>> the permissions semantics, management of users and roles, etc.  Is
>> anyone interested in starting this?
> 
>   We need to make sure we can have authentication based on the transport
> used, otherwise this need to be added at the RPC level (and hence changes
> the API).

Here's my proposal:

We make a simple program that connects to a domain socket and funnels the
stdin/stdout to that socket.

One can then build an XML-RPC transport on top of it via SSH.  The client
side simply invokes ssh and treats it's stdio as it would a normal socket.
 I've used this approach before with great success.

We get PAM authentication for free.  An advanced admin can suid that
executable to delegate privileges.

I still think we ought to have a less privileged socket too but one that's
remotable in a similar manner.
 
>> At the moment, the XML-RPC is over a Unix domain socket, so only root
>> can use it anyway (as controlled by the permission on the socket file).
> 
>   To me that's a big regression. That mean libvirt can't be used anymore
> to just monitor the Xen instance(s) without priviledged access.

The code is there for TCP.  It's just hard coded to use a domain socket
right now.  When I make the change Ewan requested to allow it to be
enabled/disabled I'll make it possible to choose the TCP version.

>   Well over (modern) unix socket one can extract the UID of the
>   connecting
> client, can we extract that from Python ? (c.f. LOCAL_CREDS/SO_PEERCRED)
> If yes then that's a good first step toward local authentication without
> messing with https and credentials.

Yeah, but that's a mess and requires specially constructed packets on the
client side.  I think the ssh tunnel approach is a lot easier.

Regards,

Anthony Liguori

>> httpu is HTTP over a unix domain socket, as opposed to over TCP.  It's
>> used elsewhere (though not widely, obviously).  It gives you basic
>> protection from non-root users (see your complaint above).
> 
>   It's not a complaint, I'm raising a problem, which IMHO is different.
> Adding core piece of OS machinery without thinking of security and
> authentication aspects from the start is a mistake that had been done on
> Unix already, it would be great to not go though the mess again just
> because we are eager to push a solution, the subsequent costs could
> reduce the benefits from the effort getting there ;-)
> 
>> > > 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?
>> > 
>> >   Another good question, as we are defining an API to Xend I think
>> > the error handling question will become more and more important. The
>> > client will need more structure and informations about processing
>> > error, the full stack trace within xend might not be that useful
>> > (except for low level debugging) to the clients, but currently I just
>> > extract a string like
>> >   "No such domain test"
>> > which is a bit hard to process correctly even in context. A list of
>> > predefined error code and meaning could help quite a bit along with
>> > just the error message.
>> 
>> Sure.  Do you have a list of error codes already (for libvirt, for
>> example)?
> 
>   Yes but for the moment they can't capture what's happening within
>   Xend,
> so if the RPC fails they show up just as being from within Xend, with
> either a GET or POST information and the string found in the content of
> the HTTP answer:
>     http://libvirt.org/html/libvirt-virterror.html#virErrorNumber
>  associated docs:
>     http://libvirt.org/errors.html
> 
> Daniel

Re: Re: [RFC] Xend XML-RPC Refactoring

[Daniel Veillard]

On Sun, Mar 12, 2006 at 11:41:58AM -0600, Anthony Liguori wrote:
> On Sun, 12 Mar 2006 04:57:02 -0500, Daniel Veillard wrote:
> 
> > On Sat, Mar 11, 2006 at 10:20:53PM +0000, Ewan Mellor wrote:
> >> Of course, a full user / permissions system for the protocol would be a
> >> good idea, but like you say, it's not trivial work.  We could kick that
> >> discussion off if you want, but it's going to need someone to design
> >> the permissions semantics, management of users and roles, etc.  Is
> >> anyone interested in starting this?
> > 
> >   We need to make sure we can have authentication based on the transport
> > used, otherwise this need to be added at the RPC level (and hence changes
> > the API).
> 
> Here's my proposal:
> 
> We make a simple program that connects to a domain socket and funnels the
> stdin/stdout to that socket.
> 
> One can then build an XML-RPC transport on top of it via SSH.  The client
> side simply invokes ssh and treats it's stdio as it would a normal socket.
>  I've used this approach before with great success.

  You mean you're ready to force 4 context switches to make the RPC because
you don't want to handle authentication at the initialization of the protocol ?
I have a hard time thinking it's a "great" solution. I probably misunderstood
something.

> We get PAM authentication for free.  An advanced admin can suid that
> executable to delegate privileges.
> 
> I still think we ought to have a less privileged socket too but one that's
> remotable in a similar manner.
>  
> >> At the moment, the XML-RPC is over a Unix domain socket, so only root
> >> can use it anyway (as controlled by the permission on the socket file).
> > 
> >   To me that's a big regression. That mean libvirt can't be used anymore
> > to just monitor the Xen instance(s) without priviledged access.
> 
> The code is there for TCP.  It's just hard coded to use a domain socket
> right now.  When I make the change Ewan requested to allow it to be
> enabled/disabled I'll make it possible to choose the TCP version.

  yeah I think I'm lost.
I don't see how you could get to a good solution without separating the
various entry points based on different level of credentials.

> >   Well over (modern) unix socket one can extract the UID of the
> >   connecting
> > client, can we extract that from Python ? (c.f. LOCAL_CREDS/SO_PEERCRED)
> > If yes then that's a good first step toward local authentication without
> > messing with https and credentials.
> 
> Yeah, but that's a mess and requires specially constructed packets on the
> client side.  I think the ssh tunnel approach is a lot easier.

  For writing/running a client ? I'm lost, say I want virsh the shell
based on libvirt to list the running domains how do you get that working ?
virsh is a binary launched from an user shell and linked with libvirt 
what do you think should happen in this scenario ? Where is the ssh 
authentication handled ? when does it take place ? I'm lost. 

Daniel

-- 
Daniel Veillard      | Red Hat http://redhat.com/
veillard@redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/

Re: Re: [RFC] Xend XML-RPC Refactoring

[Anthony Liguori]

On Sun, 12 Mar 2006 13:29:41 -0500, Daniel Veillard wrote:

>> One can then build an XML-RPC transport on top of it via SSH.  The client
>> side simply invokes ssh and treats it's stdio as it would a normal socket.
>>  I've used this approach before with great success.
> 
>   You mean you're ready to force 4 context switches to make the RPC because
> you don't want to handle authentication at the initialization of the protocol ?

The cost of context switching is going to be negligible compared to the
cost of encrypting the traffic (not to mention the cost of byte code
translation within Xend).

> I have a hard time thinking it's a "great" solution. I probably misunderstood
> something.

It's really about where you do policy.  Do you do it within Xend, or do
you do it outside of Xend.  In this case, you would have Xend only
accessible to root, and then you have another application with suid
privileges that can implement a policy.  Those roles are separate and well
defined.

Using ssh is kind of orthogonal to the above separation so you can think
about it independently.  The advantage to using ssh as a transport is that
it totally punts the problem of authentication/authorization and privacy. 
It's easy from an administrative point of view (no new ports need to be
opened up) plus it ties into enterprise-identity systems quite well.

>> The code is there for TCP.  It's just hard coded to use a domain socket
>> right now.  When I make the change Ewan requested to allow it to be
>> enabled/disabled I'll make it possible to choose the TCP version.
> 
>   yeah I think I'm lost.
> I don't see how you could get to a good solution without separating the
> various entry points based on different level of credentials.

Yup, let me be a bit more concrete.

Let's say we have xend and xend-remote.  Xend is the daemon that listens
on a domain socket for XML-RPC.  xend-remote is a command line tool that
is an XML-RPC proxy.  It proxies stdio to a domain socket.

This tool can be setup to be SUID in which case it also reads a policy
file that in the simplest case, is a white list of which calls can be
executed by a particular user/group.

libvirt exec()'s a single instance of ssh with the -S option to set it up
to be in server mode.  This is when authentication occurs.

Then, to execute individual XML-RPC calls, libvirt exec()'s ssh (again
with -S but this time as a client) which then uses the ssh running in
server mode as a proxy.

If you didn't want to go through the hassle of ssh, you could exec()
xend-remote directly (assuming you're only connecting to local host). 
Then you don't need any authentication at all.

The alternative approach to this would be to have clients to Xend pass
SO_CRED stuff directly.  Xend would then have to perform PAM
authentication (every time too since XML-RPC isn't stateful).  If we
wanted privacy, we would have to implement server-side support for SSL. 
There are advantages to this approach but I think it has the disadvantage
of making Xend more complex and having less flexible policy mechanisms.

For instance, using xend-remote, the XenSE guys could implement a much
more sophisticated proxy that was SSID-aware.  If the standard policies we
provide didn't meet an admin's requirements, she could easily implement a
new xend-remote and everything would just work.

>> >   Well over (modern) unix socket one can extract the UID of the
>> >   connecting
>> > client, can we extract that from Python ? (c.f. LOCAL_CREDS/SO_PEERCRED)
>> > If yes then that's a good first step toward local authentication without
>> > messing with https and credentials.
>> 
>> Yeah, but that's a mess and requires specially constructed packets on the
>> client side.  I think the ssh tunnel approach is a lot easier.
> 
>   For writing/running a client ?

Oh, I'm not familiar with SO_PEERCRED.  A quick googling seems to suggest
it's Linux specific.  The mechanism to authentication over domain sockets
that I'm aware of is SCM_CREDS which requires that the client actually
send the credential information (via sendmsg).

> I'm lost, say I want virsh the shell
> based on libvirt to list the running domains how do you get that working ?
> virsh is a binary launched from an user shell and linked with libvirt 
> what do you think should happen in this scenario ? Where is the ssh 
> authentication handled ? when does it take place ? I'm lost. 

I think I covered it above.  If this is localhost management, virsh can
simply exec() xend-remote (once at library initialization) and use that as
a proxy SUID proxy to Xend.

Does this sound sane?  This has been my long term vision for how
things ought to work.  One could actually implement xend-remote pretty
easily right now.  Of course, I'm flexible and open to alternatives.

Regards,

Anthony Liguori

> 
> Daniel

Re: Re: Re: [RFC] Xend XML-RPC Refactoring

[Daniel Veillard]

On Sun, Mar 12, 2006 at 02:28:24PM -0600, Anthony Liguori wrote:
> Does this sound sane?  This has been my long term vision for how
> things ought to work.  One could actually implement xend-remote pretty
> easily right now.  Of course, I'm flexible and open to alternatives.

  It's Sunday, it's late, I think I understand your viewpoint (especially
after the exchange on IRC). Still it decouples completely authentication
and right checking from the API. And I feel like we are trying to create
a solution which may not be adequate.
  I really feel of rights over Xen operations to best reflected by
tokens or capacities to use the old term. For me to create a domain
on a node then you need the capacity for that node, as a result you
get a capacity for that domain. Now once you have the capacity for the
domain you can pause/unpause/save or reduce its resource allocations.
To list domains you don't need a capacity. To shudown/destroy a domain
you need the node or domain capacity. To migrate a domain to a new node
you need both the domain and remote node capacities, etc ...
  So I really think of the authentication and security checkings in 
a very different model a priori than what you are suggesting, maybe 
the model I would like to see is just too complex, or doesn't fit
the tools available. That's probably why using a separate controller
which is unlikely to understand the API and auth at the pure connection
layer feels strange too me. I find that way too coarse, while at the
same time probably expensive to run.
  I certainly need to think more about this, other should probably
tell me how wrong I am too, that should not block going forward with
the current plan anyway :-)

Daniel

-- 
Daniel Veillard      | Red Hat http://redhat.com/
veillard@redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/

Re: [RFC] Xend XML-RPC Refactoring

[Anthony Liguori]

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@rhesis.austin.ibm.com
>> # 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@us.ibm.com>
>>
>> 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@lists.xensource.com"
>> -                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@lists.xensource.com"
>> -                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.
>   

Re: [RFC] Xend XML-RPC Refactoring

[Ewan Mellor]

On Sat, Mar 11, 2006 at 07:44:20PM -0600, Anthony Liguori wrote:

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

Yes, I hope so.  That's assuming that a) no-one objects and b) we get the
config file formats (both the main Xend config file and the xm create config
file) into XML too.  I don't see any point removing the s-expression protocol
support unless we can remove the s-expression parser/generator too, but I
certainly think that we should deprecate it for 3.0.2.

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

A preceding underscore indicates that "from Module import *" should not import
the function, i.e. it's a weak "private" declaration.  There's nothing special
about embedded underscores though.

I chose public_get_name over publicGetName because I thought that, at the
XML-RPC level, it would be conventional to have "xend.domain.get_name" as the
message name, rather than "xend.domain.GetName".  That's personal taste
though, I don't have a strong opinion here.

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

There are?  Who?  Anyone who's using the XendClient API, please speak up!

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

I don't think that there's any user-facing call which should only accept a
domid -- they should all accept either a domain ID or a domain name.  Of
course, xm cannot tell whether it is passing a domain ID or a domain name
(because names may be strings of digits too, and these are typed at the
command line) so I think that any public function must be able to cope with
either case.

That said, if there is some call that accepts only an integer, yes, it ought
to be able to throw an exception.  (I think, actually, that we're talking at
cross purposes, and obliquely agreeing with each other).

Cheers,

Ewan.

Re: [RFC] Xend XML-RPC Refactoring

[Anthony Liguori]

Hi Ewan,

I've made the changes you've requested except where I've described 
below.  After all the regressions complete, I'll push it out to the list 
(sometime tomorrow as long as nothing fails miserably).

Ewan Mellor wrote:
> 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.
>   
I went ahead and started using the public_ naming convention for 
XendDomainInfo.  It was fine for a couple of the methods but it got ugly 
real quick for things like deviceDestroy as it's called in a bunch of 
other places within Xend.

So, I've got two possible solutions for this.  We could just keep the 
white list or we could introduce new public_ methods within 
XendDomainInfo that were just simple wrappers for the underlying 
methods.  It's seems a bit unnatural but less unnatural than peppering 
calls to public_ functions through Xend.  Thoughts?
> 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.
>   
I've gone through and made sure that all XendDomain functions can accept 
either domids or names so that effectively takes care of this hack.  I 
didn't refactor away the domain_destroy function though as it seems 
independent of the XML-RPC stuff.
> 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?
>   
This one is a bit tricky.  We use faultCode to encode particular 
exception types but I'm not sure what that buys us.  I don't think 
encoding OSError as a faultCode (which is what I think the most common 
exception we toss other than XendError) really buys us much right now.  
I think what we need to do is audit the methods in Xend, figure out what 
the common errors are, *catch them within Xend*, and then rethrow them 
as standardized Faults.  Thoughts?

Regards,

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

Re: [RFC] Xend XML-RPC Refactoring

[Ewan Mellor]

On Tue, Mar 14, 2006 at 12:58:48AM -0600, Anthony Liguori wrote:

> Hi Ewan,
> 
> I've made the changes you've requested except where I've described 
> below.  After all the regressions complete, I'll push it out to the list 
> (sometime tomorrow as long as nothing fails miserably).
> 
> Ewan Mellor wrote:
> >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.
> >  
> I went ahead and started using the public_ naming convention for 
> XendDomainInfo.  It was fine for a couple of the methods but it got ugly 
> real quick for things like deviceDestroy as it's called in a bunch of 
> other places within Xend.
> 
> So, I've got two possible solutions for this.  We could just keep the 
> white list or we could introduce new public_ methods within 
> XendDomainInfo that were just simple wrappers for the underlying 
> methods.  It's seems a bit unnatural but less unnatural than peppering 
> calls to public_ functions through Xend.  Thoughts?

Yes, I'd go with the latter.  public_device_destroy does the type
conversion, argument validation (with errors thrown if necessary) and
then just calls deviceDestroy.  All the other calls to deviceDestroy
from within XendDomainInfo itself don't need the type conversion etc, so just
point at deviceDestroy directly.

Ewan.