Index: fedd/fedd_experiment_control.py
===================================================================
--- fedd/fedd_experiment_control.py	(revision 11a08b0e9216a4de8d8e50f742da2911382a40e7)
+++ fedd/fedd_experiment_control.py	(revision 0ea11afb184736a45872959bc88e2cb71e328400)
@@ -39,4 +39,10 @@
 
 class fedd_experiment_control_local:
+    """
+    Control of experiments that this system can directly access.
+
+    Includes experiment creation, termination and information dissemination.
+    Thred safe.
+    """
     scripts = ["fed_bootstrap", "federate.sh", "smbmount.FreeBSD.pl",
 	"smbmount.Linux.pl", "make_hosts", "fed-tun.pl", "fed-tun.ucb.pl",
@@ -44,5 +50,12 @@
     
     class thread_pool:
+	"""
+	A class to keep track of a set of threads all invoked for the same
+	task.  Manages the mutual exclusion of the states.
+	"""
 	def __init__(self):
+	    """
+	    Start a pool.
+	    """
 	    self.changed = Condition()
 	    self.started = 0
@@ -50,13 +63,25 @@
 
 	def acquire(self):
+	    """
+	    Get the pool's lock.
+	    """
 	    self.changed.acquire()
 
 	def release(self):
+	    """
+	    Release the pool's lock.
+	    """
 	    self.changed.release()
 
 	def wait(self, timeout = None):
+	    """
+	    Wait for a pool thread to start or stop.
+	    """
 	    self.changed.wait(timeout)
 
 	def start(self):
+	    """
+	    Called by a pool thread to report starting.
+	    """
 	    self.changed.acquire()
 	    self.started += 1
@@ -65,4 +90,7 @@
 
 	def terminate(self):
+	    """
+	    Called by a pool thread to report finishing.
+	    """
 	    self.changed.acquire()
 	    self.terminated += 1
@@ -71,4 +99,7 @@
 
 	def clear(self):
+	    """
+	    Clear all pool data.
+	    """
 	    self.changed.acquire()
 	    self.started = 0
@@ -78,16 +109,25 @@
 
     class pooled_thread(Thread):
+	"""
+	One of a set of threads dedicated to a specific task.  Uses the
+	thread_pool class above for coordination.
+	"""
 	def __init__(self, group=None, target=None, name=None, args=(), 
 		kwargs={}, pdata=None, trace_file=None):
 	    Thread.__init__(self, group, target, name, args, kwargs)
-	    self.rv = None
-	    self.exception = None
-	    self.target=target
-	    self.args = args
-	    self.kwargs = kwargs
-	    self.pdata = pdata
-	    self.trace_file = trace_file
+	    self.rv = None	    # Return value of the ops in this thread
+	    self.exception = None   # Exception that terminated this thread
+	    self.target=target	    # Target function to run on start()
+	    self.args = args	    # Args to pass to target
+	    self.kwargs = kwargs    # Additional kw args
+	    self.pdata = pdata	    # thread_pool for this class
+	    # Logger for this thread
+	    self.log = logging.getLogger("fedd.experiment_control")
 	
 	def run(self):
+	    """
+	    Emulate Thread.run, except add pool data manipulation and error
+	    logging.
+	    """
 	    if self.pdata:
 		self.pdata.start()
@@ -98,17 +138,18 @@
 		except service_error, s:
 		    self.exception = s
-		    if self.trace_file:
-			logging.error("Thread exception: %s %s" % \
-				(s.code_string(), s.desc))
+		    self.log.error("Thread exception: %s %s" % \
+			    (s.code_string(), s.desc))
 		except:
 		    self.exception = sys.exc_info()[1]
-		    if self.trace_file:
-			logging.error(("Unexpected thread exception: %s" +\
-				"Trace %s") % (self.exception,\
-				    traceback.format_exc()))
+		    self.log.error(("Unexpected thread exception: %s" +\
+			    "Trace %s") % (self.exception,\
+				traceback.format_exc()))
 	    if self.pdata:
 		self.pdata.terminate()
 
     def __init__(self, config=None):
+	"""
+	Intialize the various attributes, most from the config object
+	"""
 	self.scripts = fedd_experiment_control_local.scripts
 	self.thread_with_rv = fedd_experiment_control_local.pooled_thread
@@ -121,5 +162,5 @@
 	# Walk through the various relevant certificat specifying config
 	# attributes until the local certificate attributes can be resolved.
-	# The walk is from omst specific to most general specification.
+	# The walk is from most specific to most general specification.
 	for p in ("create_experiment_", "proxy_", ""):
 	    filen = "%scert_file" % p
@@ -200,5 +241,7 @@
 			config.experiment_log)
 
-	# Grab saved state
+	# Grab saved state.  OK to do this w/o locking because it's read only
+	# and only one thread should be in existence that can see self.state at
+	# this point.
 	if self.state_filename:
 	    self.read_state()
@@ -210,5 +253,6 @@
 			"%s/%s not in local script dir" % (self.scripts_dir, s))
 
-	self.soap_handlers = {\
+	# Dispatch tables
+	self.soap_services = {\
 		'Create': make_soap_handler(\
 			CreateRequestMessage.typecode,
@@ -238,5 +282,5 @@
 	}
 
-	self.xmlrpc_handlers = {\
+	self.xmlrpc_services = {\
 		'Create': make_xmlrpc_handler(\
 			getattr(self, "create_experiment"), 
@@ -257,8 +301,8 @@
 
     def get_soap_services(self):
-	return self.soap_handlers
+	return self.soap_services
 
     def get_xmlrpc_services(self):
-	return self.xmlrpc_handlers
+	return self.xmlrpc_services
 
     def copy_file(self, src, dest, size=1024):
@@ -278,4 +322,10 @@
     # Call while holding self.state_lock
     def write_state(self):
+	"""
+	Write a new copy of experiment state after copying the existing state
+	to a backup.
+
+	State format is a simple pickling of the state dictionary.
+	"""
 	if os.access(self.state_filename, os.W_OK):
 	    self.copy_file(self.state_filename, \
@@ -292,4 +342,9 @@
     # Call while holding self.state_lock
     def read_state(self):
+	"""
+	Read a new copy of experiment state.  Old state is overwritten.
+
+	State format is a simple pickling of the state dictionary.
+	"""
 	try:
 	    f = open(self.state_filename, "r")
@@ -303,5 +358,6 @@
     def scp_file(self, file, user, host, dest=""):
 	"""
-	scp a file to the remote host.
+	scp a file to the remote host.  If debug is set the action is only
+	logged.
 	"""
 
@@ -316,4 +372,8 @@
 
     def ssh_cmd(self, user, host, cmd, wname=None):
+	"""
+	Run a remote command on host as user.  If debug is set, the action is
+	only logged.
+	"""
 	sh_str = "%s %s@%s %s" % (self.ssh_exec, user, host, cmd)
 
@@ -326,4 +386,7 @@
 
     def ship_scripts(self, host, user, dest_dir):
+	"""
+	Copy the federation scripts (fedkit) to the a federant.
+	"""
 	if self.ssh_cmd(user, host, "mkdir -p %s" % dest_dir):
 	    for s in self.scripts:
@@ -336,4 +399,7 @@
 
     def ship_configs(self, host, user, src_dir, dest_dir):
+	"""
+	Copy federant-specific configuration files to the federant.
+	"""
 	if not self.ssh_cmd(user, host, "mkdir -p %s" % dest_dir):
 	    return False
@@ -353,19 +419,32 @@
 
     def start_segment(self, tb, eid, tbparams, tmpdir, timeout=0):
+	"""
+	Start a sub-experiment on a federant.
+
+	Get the current state, modify or create as appropriate, ship data and
+	configs and start the experiment.  There are small ordering differences
+	based on the initial state of the sub-experiment.
+	"""
+	# ops node in the federant
 	host = "%s%s" % (tbparams[tb]['host'], tbparams[tb]['domain'])
-	user = tbparams[tb]['user']
-	pid = tbparams[tb]['project']
+	user = tbparams[tb]['user']	# federant user
+	pid = tbparams[tb]['project']	# federant project
 	# XXX
 	base_confs = ( "hosts",)
-	tclfile = "%s.%s.tcl" % (eid, tb)
-	expinfo_exec = "/usr/testbed/bin/expinfo"
+	tclfile = "%s.%s.tcl" % (eid, tb)   # sub-experiment description
+	# command to test experiment state
+	expinfo_exec = "/usr/testbed/bin/expinfo"  
+	# Configuration directories on the remote machine
 	proj_dir = "/proj/%s/exp/%s/tmp" % (pid, eid)
 	tarfiles_dir = "/proj/%s/tarfiles/%s" % (pid, eid)
 	rpms_dir = "/proj/%s/rpms/%s" % (pid, eid)
+	# Regular expressions to parse the expinfo response
 	state_re = re.compile("State:\s+(\w+)")
 	no_exp_re = re.compile("^No\s+such\s+experiment")
-	state = None
+	state = None	# Experiment state parsed from expinfo
+	# The expinfo ssh command
 	cmd = [self.ssh_exec, "%s@%s" % (user, host), expinfo_exec, pid, eid]
 
+	# Get status
 	self.log.debug("[start_segment]: %s"% " ".join(cmd))
 	dev_null = None
@@ -383,4 +462,5 @@
 		if m: state = "none"
 	rv = status.wait()
+
 	# If the experiment is not present the subcommand returns a non-zero
 	# return value.  If we successfully parsed a "none" outcome, ignore the
@@ -506,4 +586,7 @@
 
     def stop_segment(self, tb, eid, tbparams):
+	"""
+	Stop a sub experiment by calling swapexp on the federant
+	"""
 	user = tbparams[tb]['user']
 	host = tbparams[tb]['host']
@@ -541,19 +624,42 @@
 
     def gentopo(self, str):
+	"""
+	Generate the topology dtat structure from the splitter's XML
+	representation of it.
+
+	The topology XML looks like:
+	    <experiment>
+		<nodes>
+		    <node><vname></vname><ips>ip1:ip2</ips></node>
+		</nodes>
+		<lans>
+		    <lan>
+			<vname></vname><vnode></vnode><ip></ip>
+			<bandwidth></bandwidth><member>node:port</member>
+		    </lan>
+		</lans>
+	"""
 	class topo_parse:
+	    """
+	    Parse the topology XML and create the dats structure.
+	    """
 	    def __init__(self):
+		# Typing of the subelements for data conversion
 		self.str_subelements = ('vname', 'vnode', 'ips', 'ip', 'member')
 		self.int_subelements = ( 'bandwidth',)
 		self.float_subelements = ( 'delay',)
+		# The final data structure
 		self.nodes = [ ]
 		self.lans =  [ ]
-		self.element = { }
 		self.topo = { \
 			'node': self.nodes,\
 			'lan' : self.lans,\
 		    }
-		self.chars = ""
+		self.element = { }  # Current element being created
+		self.chars = ""	    # Last text seen
 
 	    def end_element(self, name):
+		# After each sub element the contents is added to the current
+		# element or to the appropriate list.
 		if name == 'node':
 		    self.nodes.append(self.element)
@@ -782,4 +888,9 @@
 	
     class current_testbed:
+	"""
+	Object for collecting the current testbed description.  The testbed
+	description is saved to a file with the local testbed variables
+	subsittuted line by line.
+	"""
 	def __init__(self, eid, tmpdir):
 	    self.begin_testbed = re.compile("^#\s+Begin\s+Testbed\s+\((\w+)\)")
@@ -870,4 +981,8 @@
 
     class allbeds:
+	"""
+	Process the Allbeds section.  Get access to each federant and save the
+	parameters in tbparams
+	"""
 	def __init__(self, get_access):
 	    self.begin_allbeds = re.compile("^#\s+Begin\s+Allbeds")
@@ -1090,5 +1205,12 @@
 
     class shunt_to_file:
+	"""
+	Simple class to write data between two regexps to a file.
+	"""
 	def __init__(self, begin, end, filename):
+	    """
+	    Begin shunting on a match of begin, stop on end, send data to
+	    filename.
+	    """
 	    self.begin = re.compile(begin)
 	    self.end = re.compile(end)
@@ -1098,4 +1220,7 @@
 
 	def __call__(self, line):
+	    """
+	    Call this on each line in the input that may be shunted.
+	    """
 	    if not self.in_shunt:
 		if self.begin.match(line):
@@ -1121,4 +1246,8 @@
 
     class shunt_to_list:
+	"""
+	Same interface as shunt_to_file.  Data collected in self.list, one list
+	element per line.
+	"""
 	def __init__(self, begin, end):
 	    self.begin = re.compile(begin)
@@ -1142,4 +1271,8 @@
 
     class shunt_to_string:
+	"""
+	Same interface as shunt_to_file.  Data collected in self.str, all in
+	one string.
+	"""
 	def __init__(self, begin, end):
 	    self.begin = re.compile(begin)
@@ -1163,4 +1296,13 @@
 
     def create_experiment(self, req, fid):
+	"""
+	The external interface to experiment creation called from the
+	dispatcher.
+
+	Creates a working directory, splits the incoming description using the
+	splitter script and parses out the avrious subsections using the
+	lcasses above.  Once each sub-experiment is created, use pooled threads
+	to instantiate them and start it all up.
+	"""
 	try:
 	    tmpdir = tempfile.mkdtemp(prefix="split-")
@@ -1209,4 +1351,5 @@
 	    while (self.state.has_key(eid)):
 		eid += random.choice(string.ascii_letters)
+	    # To avoid another thread picking this localname
 	    self.state[eid] = "placeholder"
 	    self.state_lock.release()
@@ -1220,4 +1363,5 @@
 		for i in range(0,5):
 		    eid += random.choice(string.ascii_letters)
+	    # To avoid another thread picking this localname
 	    self.state[eid] = "placeholder"
 	    self.state_lock.release()
@@ -1242,7 +1386,9 @@
 	tclparser = Popen(tclcmd, stdout=PIPE)
 
-	allocated = { }
-	started = { }
-
+	allocated = { }	    # Testbeds we can access
+	started = { }	    # Testbeds where a sub-experiment started 
+			    # successfully
+
+	# Objects to parse the splitter output (defined above)
 	parse_current_testbed = self.current_testbed(eid, tmpdir)
 	parse_allbeds = self.allbeds(self.get_access)
@@ -1258,4 +1404,5 @@
 		"^#\s+End\s+rpms")
 
+	# Worling on the split data
 	for line in tclparser.stdout:
 	    line = line.rstrip()
@@ -1278,4 +1425,5 @@
 			"Bad tcl parse? %s" % line)
 
+	# Virtual topology and visualization
 	vtopo = self.gentopo(parse_vtopo.str)
 	if not vtopo:
@@ -1390,4 +1538,5 @@
 		}
 
+	# Insert the experiment into our state and update the disk copy
 	self.state_lock.acquire()
 	self.state[expid] = { 'federant' : [ tbparams[tb]['federant'] \
@@ -1412,4 +1561,7 @@
 
     def get_vtopo(self, req, fid):
+	"""
+	Return the stored virtual topology for this experiment
+	"""
 	rv = None
 
@@ -1442,4 +1594,7 @@
 
     def get_vis(self, req, fid):
+	"""
+	Return the stored visualization for this experiment
+	"""
 	rv = None
 
@@ -1472,4 +1627,7 @@
 
     def get_info(self, req, fid):
+	"""
+	Return all the stored info about this experiment
+	"""
 	rv = None
 
@@ -1504,4 +1662,8 @@
 
     def terminate_experiment(self, req, fid):
+	"""
+	Swap this experiment out on the federants and delete the shared
+	information
+	"""
 	tbparams = { }
 	req = req.get('TerminateRequestBody', None)
@@ -1584,67 +1746,2 @@
 	    self.state_lock.release()
 	    raise service_error(service_error.req, "No saved state")
-
-
-
-
-if __name__ == '__main__':
-    from optparse import OptionParser
-    
-    parser = OptionParser()
-    parser.add_option('-d', '--debug', dest='debug', default=False,
-	    action='store_true', help='print actions rather than take them')
-    parser.add_option('-f', '--file', dest='tcl', help='tcl file to parse')
-    parser.add_option('-m', '--master', dest='master', 
-	    help='testbed label for matster testbd')
-    parser.add_option('-t', '--trace', dest='trace', default=None, 
-	    help='file to print intermediate messages to')
-    parser.add_option('-T', '--trace-stderr', dest='trace', 
-	    action='store_const',const=sys.stderr,
-	    help='file to print intermediate messages to')
-    opts, args  = parser.parse_args()
-
-    trace_file = None
-    if opts.trace:
-	try:
-	    trace_file = open(opts.trace, 'w')
-	except IOError:
-	    print >>sys.stderr, "Can't open trace file"
-
-    if opts.debug:
-	if not trace_file:
-	    trace_file = sys.stderr
-
-    if opts.tcl != None:
-	try:
-	    f = open(opts.tcl, 'r')
-	    content = ''.join(f)
-	    f.close()
-	except IOError, e:
-	    sys.exit("Can't read %s: %s" % (opts.tcl, e))
-    else:
-	sys.exit("Must specify a file name")
-
-    if not opts.master:
-	sys.exit("Must supply master tb label (--master)");
-
-    obj = fedd_create_experiment_local(
-	    debug=opts.debug,
-	    scripts_dir="/users/faber/testbed/federation",
-	    cert_file="./fedd_client.pem", cert_pwd="faber", 
-	    ssh_pubkey_file='/users/faber/.ssh/id_rsa.pub',
-	    trusted_certs="./cacert.pem",
-	    tbmap = { 
-		'deter':'https://users.isi.deterlab.net:23235',
-		'emulab':'https://users.isi.deterlab.net:23236',
-		'ucb':'https://users.isi.deterlab.net:23237',
-		},
-	    trace_file=trace_file
-	) 
-    rv = obj.create_experiment( {\
-	    'experimentdescription' : content, 
-	    'master' : opts.master, 
-	    'user': [ {'userID' : { 'localname' : 'faber' } } ],
-	    },
-	    None)
-
-    print rv
