Index: fedd/federation/emulab_access.py
===================================================================
--- fedd/federation/emulab_access.py	(revision 8cf2b90e2251f7161f682631e113489faf064cc3)
+++ fedd/federation/emulab_access.py	(revision 2f6820c51d846a9fc3a9493c9a12008a72228793)
@@ -10,4 +10,5 @@
 import logging
 import subprocess
+import traceback
 
 from threading import *
@@ -48,7 +49,4 @@
     dynamically.  This implements both direct requests and proxies.
     """
-
-    proxy_RequestAccess= service_caller('RequestAccess')
-    proxy_ReleaseAccess= service_caller('ReleaseAccess')
 
     def __init__(self, config=None, auth=None):
@@ -106,14 +104,4 @@
 
         self.restricted = [ ]
-        self.projects = { }
-        self.keys = { }
-        self.types = { }
-        self.allocation = { }
-        self.state = { 
-            'projects': self.projects,
-            'allocation' : self.allocation,
-            'keys' : self.keys,
-            'types': self.types
-        }
         self.access = { }
         if config.has_option("access", "accessdb"):
@@ -138,6 +126,9 @@
 		    self.make_access_project)
 
-	# read state in the base_class
+	# read_state in the base_class
 	self.state_lock.acquire()
+	for a  in ('allocation', 'projects', 'keys', 'types'):
+	    if a not in self.state:
+		self.state[a] = { }
 	self.allocation = self.state['allocation']
 	self.projects = self.state['projects']
@@ -192,4 +183,10 @@
     @staticmethod
     def make_access_project(str):
+	"""
+	Convert a string of the form (id[:resources:resouces], id, id) into an
+	access_project.  This is called by read_access to convert to local
+	attributes.  It returns a tuple of the form (project, user, user) where
+	users may be names or fedids.
+	"""
         def parse_name(n):
             if n.startswith('fedid:'): return fedid(hexstr=n[len('fedid:'):])
@@ -216,10 +213,16 @@
 
 
+    # RequestAccess support routines
+
     def lookup_access(self, req, fid):
         """
-        Determine the allowed access for this request.  Return the access and
-        which fields are dynamic.
-
-        The fedid is needed to construct the request
+	Look up the local access control information mapped to this fedid and
+	credentials.  In this case it is a (project, create_user, access_user)
+	triple, and a triple of three booleans indicating which, if any will
+	need to be dynamically created.  Finally a list of owners for that
+	allocation is returned.
+
+	lookup_access_base pulls the first triple out, and it is parsed by this
+	routine into the boolean map.  Owners is always the controlling fedid.
         """
         # Return values
@@ -380,4 +383,6 @@
 	return (pname, uname)
 
+    # End of RequestAccess support routines
+
     def RequestAccess(self, req, fid):
         """
@@ -564,6 +569,12 @@
 	    raise service_error(service_error.req, "No such allocation")
 
-
+    # These are subroutines for StartSegment
     def generate_ns2(self, topo, expfn, softdir, connInfo):
+	"""
+	Convert topo into an ns2 file, decorated with appropriate commands for
+	the particular testbed setup.  Convert all requests for software, etc
+	to point at the staged copies on this testbed and add the federation
+	startcommands.
+	"""
 	class dragon_commands:
 	    """
@@ -593,9 +604,11 @@
 			    # XXX: do netmask right
 			    if type =='link':
-				s = ("tb-allow-external ${%s} dragonportal " + \
-					"ip %s vlan %s netmask 255.255.255.0\n") % \
+				s = ("tb-allow-external ${%s} " + \
+					"dragonportal ip %s vlan %s " + \
+					"netmask 255.255.255.0\n") % \
 					(e.name, addr, vlan)
 			    elif type =='lan':
-				s = ("tb-allow-external ${%s} dragonportal " + \
+				s = ("tb-allow-external ${%s} " + \
+					"dragonportal " + \
 					"ip %s vlan %s usurp %s\n") % \
 					(e.name, addr, vlan, subs)
@@ -606,4 +619,7 @@
 
 	class not_dragon:
+	    """
+	    Return true if a node is in the given map of dragon nodes.
+	    """
 	    def __init__(self, map):
 		self.nodes = set(map.keys())
@@ -612,7 +628,9 @@
 		return e.name not in self.nodes
 
-
+	# Main line of generate_ns2
 	t = topo.clone()
 
+	# Create the map of nodes that need direct connections (dragon
+	# connections) from the connInfo
 	dragon_map = { }
 	for i in [ i for i in connInfo if i['type'] == 'transit']:
@@ -752,5 +770,5 @@
 	"""
 	Add a seer node to the given topology, with the startup command passed
-	in.
+	in.  Used by configure seer_services.
 	"""
 	c_node = topdl.Computer(
@@ -810,10 +828,163 @@
 	    self.add_seer_node(topo, 'seer-master', self.seer_master_start)
 
-
+    def retrieve_software(self, topo, certfile, softdir):
+	"""
+	Collect the software that nodes in the topology need loaded and stage
+	it locally.  This implies retrieving it from the experiment_controller
+	and placing it into softdir.  Certfile is used to prove that this node
+	has access to that data (it's the allocation/segment fedid).  Finally
+	local portal and federation software is also copied to the same staging
+	directory for simplicity - all software needed for experiment creation
+	is in softdir.
+	"""
+	sw = set()
+	for e in topo.elements:
+	    for s in getattr(e, 'software', []):
+		sw.add(s.location)
+	for s in sw:
+	    self.log.debug("Retrieving %s" % s)
+	    try:
+		get_url(s, certfile, softdir)
+	    except:
+		t, v, st = sys.exc_info()
+		raise service_error(service_error.internal,
+			"Error retrieving %s: %s" % (s, v))
+
+	# Copy local federation and portal node software to the tempdir
+	for s in (self.federation_software, self.portal_software):
+	    for l, f in s:
+		base = os.path.basename(f)
+		copy_file(f, "%s/%s" % (softdir, base))
+
+
+    def initialize_experiment_info(self, attrs, aid, certfile, tmpdir):
+	"""
+	Gather common configuration files, retrieve or create an experiment
+	name and project name, and return the ssh_key filenames.  Create an
+	allocation log bound to the state log variable as well. 
+	"""
+	configs = set(('hosts', 'ssh_pubkey', 'ssh_secretkey'))
+	ename = None
+	pubkey_base = None
+	secretkey_base = None
+	proj = None
+	user = None
+	alloc_log = None
+
+	for a in attrs:
+	    if a['attribute'] in configs:
+		try:
+		    self.log.debug("Retrieving %s from %s" % \
+			    (a['attribute'], a['value']))
+		    get_url(a['value'], certfile, tmpdir)
+		except:
+		    t, v, st = sys.exc_info()
+		    raise service_error(service_error.internal,
+			    "Error retrieving %s: %s" % (a.get('value', ""), v))
+	    if a['attribute'] == 'ssh_pubkey':
+		pubkey_base = a['value'].rpartition('/')[2]
+	    if a['attribute'] == 'ssh_secretkey':
+		secretkey_base = a['value'].rpartition('/')[2]
+	    if a['attribute'] == 'experiment_name':
+		ename = a['value']
+
+	if not ename:
+	    ename = ""
+	    for i in range(0,5):
+		ename += random.choice(string.ascii_letters)
+	    self.log.warn("No experiment name: picked one randomly: %s" \
+		    % ename)
+
+	if not pubkey_base:
+	    raise service_error(service_error.req, 
+		    "No public key attribute")
+
+	if not secretkey_base:
+	    raise service_error(service_error.req, 
+		    "No secret key attribute")
+
+	self.state_lock.acquire()
+	if aid in self.allocation:
+	    proj = self.allocation[aid].get('project', None)
+	    if not proj: 
+		proj = self.allocation[aid].get('sproject', None)
+	    user = self.allocation[aid].get('user', None)
+	    self.allocation[aid]['experiment'] = ename
+	    self.allocation[aid]['log'] = [ ]
+	    # Create a logger that logs to the experiment's state object as
+	    # well as to the main log file.
+	    alloc_log = logging.getLogger('fedd.access.%s' % ename)
+	    h = logging.StreamHandler(
+		    list_log.list_log(self.allocation[aid]['log']))
+	    # XXX: there should be a global one of these rather than
+	    # repeating the code.
+	    h.setFormatter(logging.Formatter(
+		"%(asctime)s %(name)s %(message)s",
+			'%d %b %y %H:%M:%S'))
+	    alloc_log.addHandler(h)
+	    self.write_state()
+	self.state_lock.release()
+
+	if not proj:
+	    raise service_error(service_error.internal, 
+		    "Can't find project for %s" %aid)
+
+	if not user:
+	    raise service_error(service_error.internal, 
+		    "Can't find creation user for %s" %aid)
+
+	return (ename, proj, user, pubkey_base, secretkey_base, alloc_log)
+
+    def finalize_experiment(self, starter, topo, aid, alloc_id):
+	"""
+	Store key bits of experiment state in the global repository, including
+	the response that may need to be replayed, and return the response.
+	"""
+	# Copy the assigned names into the return topology
+	embedding = [ ]
+	for n in starter.node:
+	    embedding.append({ 
+		'toponame': n,
+		'physname': ["%s%s" %  (starter.node[n], self.domain)],
+		})
+	# Grab the log (this is some anal locking, but better safe than
+	# sorry)
+	self.state_lock.acquire()
+	logv = "".join(self.allocation[aid]['log'])
+	# It's possible that the StartSegment call gets retried (!). 
+	# if the 'started' key is in the allocation, we'll return it rather
+	# than redo the setup.
+	self.allocation[aid]['started'] = { 
+		'allocID': alloc_id,
+		'allocationLog': logv,
+		'segmentdescription': { 
+		    'topdldescription': topo.clone().to_dict()
+		    },
+		'embedding': embedding
+		}
+	retval = copy.copy(self.allocation[aid]['started'])
+	self.write_state()
+	self.state_lock.release()
+	return retval
+
+    def remove_dirs(self, dir):
+	"""
+	Remove the directory tree and all files rooted at dir.  Log any errors,
+	but continue.
+	"""
+	self.log.debug("[removedirs]: removing %s" % dir)
+	try:
+	    for path, dirs, files in os.walk(dir, topdown=False):
+		for f in files:
+		    os.remove(os.path.join(path, f))
+		for d in dirs:
+		    os.rmdir(os.path.join(path, d))
+	    os.rmdir(dir)
+	except EnvironmentError, e:
+	    self.log.error("Error deleting directory tree in %s" % e);
+    
+    # End of StartSegment support routines
 
     def StartSegment(self, req, fid):
-
-	configs = set(('hosts', 'ssh_pubkey', 'ssh_secretkey'))
-
 	err = None  # Any service_error generated after tmpdir is created
 	rv = None   # Return value from segment creation
@@ -821,4 +992,6 @@
 	try:
 	    req = req['StartSegmentRequestBody']
+	    auth_attr = req['allocID']['fedid']
+	    topref = req['segmentdescription']['topdldescription']
 	except KeyError:
 	    raise service_error(server_error.req, "Badly formed request")
@@ -826,5 +999,4 @@
 	connInfo = req.get('connection', [])
 	services = req.get('service', [])
-	auth_attr = req['allocID']['fedid']
 	aid = "%s" % auth_attr
 	attrs = req.get('fedAttr', [])
@@ -845,14 +1017,9 @@
 	# A new request.  Do it.
 
-	if req.has_key('segmentdescription') and \
-		req['segmentdescription'].has_key('topdldescription'):
-	    topo = \
-		topdl.Topology(**req['segmentdescription']['topdldescription'])
+	if topref: topo = topdl.Topology(**topref)
 	else:
 	    raise service_error(service_error.req, 
 		    "Request missing segmentdescription'")
 	
-	master = req.get('master', False)
-
 	certfile = "%s/%s.pem" % (self.certdir, auth_attr)
         try:
@@ -865,95 +1032,13 @@
 	# Try block alllows us to clean up temporary files.
 	try:
-	    sw = set()
-	    for e in topo.elements:
-		for s in getattr(e, 'software', []):
-		    sw.add(s.location)
-	    for s in sw:
-		self.log.debug("Retrieving %s" % s)
-		try:
-		    get_url(s, certfile, softdir)
-		except:
-		    t, v, st = sys.exc_info()
-		    raise service_error(service_error.internal,
-			    "Error retrieving %s: %s" % (s, v))
-
-	    # Copy local federation and portal node software to the tempdir
-	    for s in (self.federation_software, self.portal_software):
-		for l, f in s:
-		    base = os.path.basename(f)
-		    copy_file(f, "%s/%s" % (softdir, base))
-
-	    ename = None
-	    pubkey_base = None
-	    secretkey_base = None
-	    for a in attrs:
-		if a['attribute'] in configs:
-		    try:
-			self.log.debug("Retrieving %s from %s" % \
-				(a['attribute'], a['value']))
-			get_url(a['value'], certfile, tmpdir)
-		    except:
-			t, v, st = sys.exc_info()
-			raise service_error(service_error.internal,
-				"Error retrieving %s: %s" % (s, v))
-		if a['attribute'] == 'ssh_pubkey':
-		    pubkey_base = a['value'].rpartition('/')[2]
-		if a['attribute'] == 'ssh_secretkey':
-		    secretkey_base = a['value'].rpartition('/')[2]
-		if a['attribute'] == 'experiment_name':
-		    ename = a['value']
-
-	    if not ename:
-		ename = ""
-		for i in range(0,5):
-		    ename += random.choice(string.ascii_letters)
-		self.log.warn("No experiment name: picked one randomly: %s" \
-			% ename)
-
-	    if not pubkey_base:
-		raise service_error(service_error.req, 
-			"No public key attribute")
-
-	    if not secretkey_base:
-		raise service_error(service_error.req, 
-			"No secret key attribute")
-
-	    # If the userconf service was imported, collect the configuration
-	    # data.
+	    self.retrieve_software(topo, certfile, softdir)
+	    ename, proj, user, pubkey_base, secretkey_base, alloc_log = \
+		    self.initialize_experiment_info(attrs, aid, 
+			    certfile, tmpdir)
+
+	    # Set up userconf and seer if needed
 	    self.configure_userconf(services)
 	    self.configure_seer_services(services, topo, softdir)
-
-	    proj = None
-	    user = None
-	    self.state_lock.acquire()
-	    if self.allocation.has_key(aid):
-		proj = self.allocation[aid].get('project', None)
-		if not proj: 
-		    proj = self.allocation[aid].get('sproject', None)
-		user = self.allocation[aid].get('user', None)
-		self.allocation[aid]['experiment'] = ename
-		self.allocation[aid]['log'] = [ ]
-		# Create a logger that logs to the experiment's state object as
-		# well as to the main log file.
-		alloc_log = logging.getLogger('fedd.access.%s' % ename)
-		h = logging.StreamHandler(
-			list_log.list_log(self.allocation[aid]['log']))
-		# XXX: there should be a global one of these rather than
-		# repeating the code.
-		h.setFormatter(logging.Formatter(
-		    "%(asctime)s %(name)s %(message)s",
-			    '%d %b %y %H:%M:%S'))
-		alloc_log.addHandler(h)
-		self.write_state()
-	    self.state_lock.release()
-
-	    if not proj:
-		raise service_error(service_error.internal, 
-			"Can't find project for %s" %aid)
-
-	    if not user:
-		raise service_error(service_error.internal, 
-			"Can't find creation user for %s" %aid)
-
+	    # Get and send synch store variables
 	    self.export_store_info(certfile, proj, ename, connInfo)
 	    self.import_store_info(certfile, connInfo)
@@ -962,6 +1047,5 @@
 
 	    self.generate_portal_configs(topo, pubkey_base, 
-		    secretkey_base, tmpdir, master, proj, ename, connInfo, 
-		    services)
+		    secretkey_base, tmpdir, proj, ename, connInfo, services)
 	    self.generate_ns2(topo, expfile, 
 		    "/proj/%s/software/%s/" % (proj, ename), connInfo)
@@ -970,52 +1054,17 @@
 		    debug=self.create_debug, log=alloc_log)
 	    rv = starter(self, ename, proj, user, expfile, tmpdir)
-	    rvtopo = topo.clone()
-
-	    # Copy the assigned names into the return topology
-	    embedding = [ ]
-	    for n in starter.node:
-		embedding.append({ 
-		    'toponame': n,
-		    'physname': ["%s%s" %  (starter.node[n], self.domain)],
-		    })
-
-
 	except service_error, e:
 	    err = e
-	except e:
-	    err = service_error(service_error.internal, str(e))
+	except:
+	    t, v, st = sys.exc_info()
+	    err = service_error(service_error.internal, "%s: %s" % \
+		    (v, traceback.extract_tb(st)))
 
         # Walk up tmpdir, deleting as we go
-	if self.cleanup:
-	    self.log.debug("[StartSegment]: removing %s" % tmpdir)
-	    for path, dirs, files in os.walk(tmpdir, topdown=False):
-		for f in files:
-		    os.remove(os.path.join(path, f))
-		for d in dirs:
-		    os.rmdir(os.path.join(path, d))
-	    os.rmdir(tmpdir)
-	else:
-	    self.log.debug("[StartSegment]: not removing %s" % tmpdir)
+	if self.cleanup: self.remove_dirs(tmpdir)
+	else: self.log.debug("[StartSegment]: not removing %s" % tmpdir)
 
 	if rv:
-	    # Grab the log (this is some anal locking, but better safe than
-	    # sorry)
-	    self.state_lock.acquire()
-	    logv = "".join(self.allocation[aid]['log'])
-	    # It's possible that the StartSegment call gets retried (!). 
-	    # if the 'started' key is in the allocation, we'll return it rather
-	    # than redo the setup.
-	    self.allocation[aid]['started'] = { 
-		    'allocID': req['allocID'],
-		    'allocationLog': logv,
-		    'segmentdescription': { 
-			'topdldescription': rvtopo.to_dict()
-			},
-		    'embedding': embedding
-		    }
-	    retval = copy.copy(self.allocation[aid]['started'])
-	    self.write_state()
-	    self.state_lock.release()
-	    return retval
+	    return self.finalize_experiment(starter, topo, aid, req['allocID'])
 	elif err:
 	    raise service_error(service_error.federant,
@@ -1037,5 +1086,5 @@
 
 	self.state_lock.acquire()
-	if self.allocation.has_key(aid):
+	if aid in self.allocation:
 	    proj = self.allocation[aid].get('project', None)
 	    if not proj: 
