From 1fc7a0716a0fd368fa9b94ee926ba14a3e4cba05 Mon Sep 17 00:00:00 2001
From: jbjohnso <jbjohnso@us.ibm.com>
Date: Fri, 18 Apr 2014 17:13:50 -0400
Subject: [PATCH] Address a number of style and other issues

---
 confluent/common/client.py        |  19 ++-
 confluent/common/tlvdata.py       |  10 +-
 confluent/config/attributes.py    |  44 +++----
 confluent/config/configmanager.py | 208 ++++++++++++++++--------------
 confluent/consoleserver.py        |  13 +-
 confluent/interface/console.py    |   1 +
 confluent/pluginapi.py            |  35 +++--
 confluent/sockapi.py              |  37 +++---
 confluent/util.py                 |  10 +-
 9 files changed, 204 insertions(+), 173 deletions(-)

diff --git a/confluent/common/client.py b/confluent/common/client.py
index f3317165..f0d7af1f 100644
--- a/confluent/common/client.py
+++ b/confluent/common/client.py
@@ -20,28 +20,32 @@ import ssl
 import confluent.common.tlvdata as tlvdata
 
 SO_PASSCRED = 16
+
+
 def _parseserver(string):
     if ']:' in string:
         server, port = string[1:].split(']:')
     elif string[0] == '[':
-        server = serverstring[1:-1]
+        server = string[1:-1]
         port = 4001
     elif ':' in string:
         server, port = string.plit(':')
     else:
         server = string
         port = 4001
-    return (server, port)
+    return server, port
+
 
 class Command(object):
 
     def __init__(self, server="/var/run/confluent/api.sock"):
+        self.connection = None
         self.serverloc = server
         if os.path.isabs(server) and os.path.exists(server):
             self._connect_unix()
         else:
             self._connect_tls()
-        banner = tlvdata.recv(self.connection)
+        tlvdata.recv(self.connection)
         authdata = tlvdata.recv(self.connection)
         if authdata['authpassed'] == 1:
             self.authenticated = True
@@ -74,7 +78,8 @@ class Command(object):
 
     def _connect_tls(self):
         server, port = _parseserver(self.serverloc)
-        for res in socket.getaddrinfo(server, port, socket.AF_UNSPEC, socket.SOCK_STREAM):
+        for res in socket.getaddrinfo(server, port, socket.AF_UNSPEC,
+                                      socket.SOCK_STREAM):
             af, socktype, proto, canonname, sa = res
             try:
                 self.connection = socket.socket(af, socktype, proto)
@@ -96,8 +101,9 @@ class Command(object):
         #TODO(jbjohnso): server certificate validation
         self.connection = ssl.wrap_socket(self.connection)
 
+
 def send_request(operation, path, server, parameters=None):
-    '''This function iterates over all the responses
+    """This function iterates over all the responses
     received from the server.
 
     :param operation:  The operation to request, retrieve, update, delete,
@@ -105,7 +111,7 @@ def send_request(operation, path, server, parameters=None):
     :param path: The URI path to the resource to operate on
     :param server: The socket to send data over
     :param parameters:  Parameters if any to send along with the request
-    '''
+    """
     payload = {'operation': operation, 'path': path}
     if parameters is not None:
         payload['parameters'] = parameters
@@ -114,4 +120,3 @@ def send_request(operation, path, server, parameters=None):
     while '_requestdone' not in result:
         yield result
         result = tlvdata.recv(server)
-
diff --git a/confluent/common/tlvdata.py b/confluent/common/tlvdata.py
index 26a67201..0abf0254 100644
--- a/confluent/common/tlvdata.py
+++ b/confluent/common/tlvdata.py
@@ -18,6 +18,7 @@ import confluent.common.tlv as tlv
 import json
 import struct
 
+
 def send(handle, data):
     if isinstance(data, str):
         # plain text, e.g. console data
@@ -31,7 +32,7 @@ def send(handle, data):
         handle.sendall(data)
     elif isinstance(data, dict):  # JSON currently only goes to 4 bytes
         # Some structured message, like what would be seen in http responses
-        sdata = json.dumps(data, separators=(',',':'))
+        sdata = json.dumps(data, separators=(',', ':'))
         tl = len(sdata)
         if tl > 16777215:
             raise Exception("JSON data exceeds protocol limits")
@@ -40,6 +41,7 @@ def send(handle, data):
         handle.sendall(struct.pack("!I", tl))
         handle.sendall(sdata)
 
+
 def recv(handle):
     tl = handle.recv(4)
     if len(tl) == 0:
@@ -49,14 +51,14 @@ def recv(handle):
         raise Exception("Protocol Violation, reserved bit set")
     # 4 byte tlv
     dlen = tl & 16777215  # grab lower 24 bits
-    type = (tl & 2130706432) >> 24  # grab 7 bits from near beginning
+    datatype = (tl & 2130706432) >> 24  # grab 7 bits from near beginning
     data = handle.recv(dlen)
     while len(data) < dlen:
         ndata = handle.recv(dlen - len(data))
         if not ndata:
             raise Exception("Error reading data")
         data += ndata
-    if type == tlv.Types.text:
+    if datatype == tlv.Types.text:
         return data
-    elif type == tlv.Types.json:
+    elif datatype == tlv.Types.json:
         return json.loads(data)
diff --git a/confluent/config/attributes.py b/confluent/config/attributes.py
index 6e24a274..d3f2f4b2 100644
--- a/confluent/config/attributes.py
+++ b/confluent/config/attributes.py
@@ -15,7 +15,6 @@
 # limitations under the License.
 
 
-
 #This defines the attributes of variou classes of things
 
 # 'nic', meant to be a nested structure under node
@@ -140,14 +139,14 @@ node = {
 #    },
 #    'console.port': {
 #        'default': 'auto',
-#        'description': ('Indicate which port to use for text console.  Default'
-#                        'behavior is to auto detect the value appropriate for'
-#                        'the platform.  "Disable" can be used to suppress'
-#                        'serial console configuration')
+#        'description': ('Indicate which port to use for text console. '
+#                        'Default behavior is to auto detect the value '
+#                        'appropriate for the platform.  "Disable" can be used
+#                        'to suppress serial console configuration')
 #    },
     'console.method': {
         'description': ('Indicate the method used to access the console of '
-                       'the managed node.')
+                        'the managed node.')
     },
 #    'virtualization.host': {
 #        'description': ('Hypervisor where this node does/should reside'),
@@ -166,8 +165,8 @@ node = {
 #    },
 #    'virtualization.nicmodel': {
 #        'description': ('The model of NIC adapter to emulate in a virtual'
-#                        'machine.  Defaults to virtio-net for KVM, vmxnet3 for'
-#                        'VMware'),
+#                        'machine.  Defaults to virtio-net for KVM, vmxnet3 '
+#                        'for VMware'),
 #        'appliesto': ['vm'],
 #    },
     'hardwaremanagement.manager': {
@@ -225,30 +224,31 @@ node = {
 #                        'one-way crypted as in /etc/shadow.  For Windows, if '
 #                        'the value is not set or is one-way crypted, the '
 #                        'local '
-#                        'Administrator account will be disabled, requiring AD')
+#                        'Administrator account will be disabled, requiring '
+#                        'AD')
 #    },
     'secret.ipmikg': {
         'description': 'Optional Integrity key for IPMI communication'
     },
 #    'secret.ipmiuser': {
 #        'description': ('The username to use to log into IPMI device related '
-#                        'to the node.  For setting username, default behavior '
-#                        'is to randomize username, for using username if not '
-#                        'set, USERID is assumed'),
+#                        'to the node.  For setting username, default '
+#                        'behavior is to randomize username, for using '
+#                        'username if not set, USERID is assumed'),
 #    },
 #    'secret.ipmipassphrase': {
-#        'description': ('The key to use to authenticate to IPMI device related '
-#                        'to the node.  For setting passphrase, default behavior '
-#                        'is to randomize passphrase and store it here.  If going '
-#                        'to connect over the network and value is not set, '
-#                        'PASSW0RD is attempted')
+#        'description': ('The key to use to authenticate to IPMI device '
+#                        'related to the node.  For setting passphrase, '
+#                        'default behavior is to randomize passphrase and '
+#                        'store it here.  If going to connect over the '
+#                        'network and value is not set, PASSW0RD is attempted')
 #    },
     'secret.hardwaremanagementuser': {
-        'description': ('Username to be set and used by protocols like SSH and '
-                        'HTTP where client provides passphrase over the network.'
-                        'Given the distinct security models betwen this class '
-                        'of protocols and SNMP and IPMI, snmp and ipmi utilize '
-                        'dedicated values.'),
+        'description': ('Username to be set and used by protocols like SSH '
+                        'and HTTP where client provides passphrase over the '
+                        'network. Given the distinct security models betwen '
+                        'this class of protocols and SNMP and IPMI, snmp and '
+                        'ipmi utilize dedicated values.'),
     },
     'secret.hardwaremanagementpassphrase': {
         'description': ('Passphrase to be set and used by protocols like SSH '
diff --git a/confluent/config/configmanager.py b/confluent/config/configmanager.py
index 3543545b..a3f2a91f 100644
--- a/confluent/config/configmanager.py
+++ b/confluent/config/configmanager.py
@@ -56,22 +56,17 @@
 # by passphrase and optionally TPM
 
 
-import Crypto.Protocol.KDF as kdf
+import Crypto.Protocol.KDF as KDF
 from Crypto.Cipher import AES
 from Crypto.Hash import HMAC
 from Crypto.Hash import SHA256
-import array
 import anydbm as dbm
 import ast
-import collections
 import confluent.config.attributes as allattributes
 import confluent.util
 import copy
 import cPickle
 import errno
-import eventlet
-import fcntl
-import math
 import operator
 import os
 import random
@@ -95,15 +90,16 @@ def _mkpath(pathname):
         else:
             raise
 
+
 def _derive_keys(passphrase, salt):
     #implement our specific combination of pbkdf2 transforms to get at
     #key.  We bump the iterations up because we can afford to
     #TODO: WORKERPOOL PBKDF2 is expensive
-    tmpkey = kdf.PBKDF2(passphrase, salt, 32, 50000,
+    tmpkey = KDF.PBKDF2(passphrase, salt, 32, 50000,
                         lambda p, s: HMAC.new(p, s, SHA256).digest())
-    finalkey = kdf.PBKDF2(tmpkey, salt, 32, 50000,
-                        lambda p, s: HMAC.new(p, s, SHA256).digest())
-    return (finalkey[:32],finalkey[32:])
+    finalkey = KDF.PBKDF2(tmpkey, salt, 32, 50000,
+                          lambda p, s: HMAC.new(p, s, SHA256).digest())
+    return finalkey[:32], finalkey[32:]
 
 
 def _get_protected_key(keydict, passphrase):
@@ -153,10 +149,9 @@ def init_masterkey(passphrase=None):
             passphrase=passphrase))
 
 
-
 def decrypt_value(cryptvalue,
-                   key=_masterkey,
-                   integritykey=_masterintegritykey):
+                  key=_masterkey,
+                  integritykey=_masterintegritykey):
     iv, cipherdata, hmac = cryptvalue
     if _masterkey is None or _masterintegritykey is None:
         init_masterkey()
@@ -176,23 +171,23 @@ def decrypt_value(cryptvalue,
 
 
 def crypt_value(value,
-                 key=_masterkey,
-                 integritykey=_masterintegritykey):
+                key=_masterkey,
+                integritykey=_masterintegritykey):
     # encrypt given value
     # PKCS7 is the padding scheme to employ, if no padded needed, pad with 16
     # check HMAC prior to attempting decrypt
     if key is None or integritykey is None:
         init_masterkey()
-        key=_masterkey
-        integritykey=_masterintegritykey
+        key = _masterkey
+        integritykey = _masterintegritykey
     iv = os.urandom(16)
     crypter = AES.new(key, AES.MODE_CBC, iv)
     neededpad = 16 - (len(value) % 16)
     pad = chr(neededpad) * neededpad
-    value = value + pad
+    value += pad
     cryptval = crypter.encrypt(value)
     hmac = HMAC.new(integritykey, cryptval, SHA256).digest()
-    return (iv, cryptval, hmac)
+    return iv, cryptval, hmac
 
 
 def _load_dict_from_dbm(dpath, tdb):
@@ -204,16 +199,18 @@ def _load_dict_from_dbm(dpath, tdb):
                 currdict[elem] = {}
             currdict = currdict[elem]
         for tk in dbe.iterkeys():
-                currdict[tk] = cPickle.loads(dbe[tk])
+            currdict[tk] = cPickle.loads(dbe[tk])
     except dbm.error:
         return
 
+
 def is_tenant(tenant):
     try:
         return tenant in _cfgstore['tenant']
     except:
         return False
 
+
 def get_global(globalname):
     """Get a global variable
 
@@ -225,6 +222,7 @@ def get_global(globalname):
     except:
         return None
 
+
 def set_global(globalname, value):
     """Set a global variable.
 
@@ -240,7 +238,7 @@ def set_global(globalname, value):
             _cfgstore['dirtyglobals'] = set()
         _cfgstore['dirtyglobals'].add(globalname)
     if 'globals' not in _cfgstore:
-        _cfgstore['globals'] = { globalname: value }
+        _cfgstore['globals'] = {globalname: value}
     else:
         _cfgstore['globals'][globalname] = value
     ConfigManager._bg_sync_to_file()
@@ -263,12 +261,13 @@ def _generate_new_id():
     # generate a random id outside the usual ranges used for norml users in
     # /etc/passwd.  Leave an equivalent amount of space near the end disused,
     # just in case
-    id = str(confluent.util.securerandomnumber(65537, 4294901759))
+    uid = str(confluent.util.securerandomnumber(65537, 4294901759))
     if 'idmap' not in _cfgstore['main']:
-        return id
-    while id in _cfgstore['main']['idmap']:
-        id = str(confluent.util.securerandomnumber(65537, 4294901759))
-    return id
+        return uid
+    while uid in _cfgstore['main']['idmap']:
+        uid = str(confluent.util.securerandomnumber(65537, 4294901759))
+    return uid
+
 
 class _ExpressionFormat(string.Formatter):
     # This class is used to extract the literal value from an expression
@@ -297,7 +296,7 @@ class _ExpressionFormat(string.Formatter):
 
     def get_field(self, field_name, args, kwargs):
         parsed = ast.parse(field_name)
-        return (self._handle_ast_node(parsed.body[0].value), field_name)
+        return self._handle_ast_node(parsed.body[0].value), field_name
 
     def _handle_ast_node(self, node):
         if isinstance(node, ast.Num):
@@ -321,13 +320,13 @@ class _ExpressionFormat(string.Formatter):
             mg = re.match(self.posmatch, var)
             if mg:
                 idx = int(mg.group(1))
-                if self._numbers == None:
+                if self._numbers is None:
                     self._numbers = re.findall(self.nummatch, self._nodename)
                 return int(self._numbers[idx - 1])
             else:
                 if var in self._nodeobj:
                     if '_expressionkeys' not in self._nodeobj:
-                        self._nodeobj['_expressionkeys'] = set([key])
+                        self._nodeobj['_expressionkeys'] = set([var])
                     else:
                         self._nodeobj['_expressionkeys'].add(var)
                     val = _decode_attribute(var, self._nodeobj,
@@ -339,7 +338,7 @@ class _ExpressionFormat(string.Formatter):
                 raise Exception("Unsupported operation")
             op = self._supported_ops[optype]
             return op(self._handle_ast_node(node.left),
-               self._handle_ast_node(node.right))
+                      self._handle_ast_node(node.right))
 
 
 def _decode_attribute(attribute, nodeobj, formatter=None, decrypt=False):
@@ -373,6 +372,13 @@ def _decode_attribute(attribute, nodeobj, formatter=None, decrypt=False):
 # tenant context and then modules need not consider the current tenant
 # most of the time as things are automatic
 
+def _addchange(changeset, node, attrname):
+    if node not in changeset:
+        changeset[node] = {attrname: 1}
+    else:
+        changeset[node][attrname] = 1
+
+
 class ConfigManager(object):
     _cfgdir = "/etc/confluent/cfg/"
     _cfgwriter = None
@@ -387,8 +393,8 @@ class ConfigManager(object):
         if tenant is None:
             self.tenant = None
             if 'main' not in _cfgstore:
-                 _cfgstore['main'] = {}
-            self._cfgstore  = _cfgstore['main']
+                _cfgstore['main'] = {}
+            self._cfgstore = _cfgstore['main']
             if 'groups' not in self._cfgstore:
                 self._cfgstore['groups'] = {'everything': {'nodes': set()}}
             if 'nodes' not in self._cfgstore:
@@ -423,7 +429,7 @@ class ConfigManager(object):
         notifierid = random.randint(0, sys.maxint)
         while notifierid in self._notifierids:
             notifierid = random.randint(0, sys.maxint)
-        self._notifierids[notifierid] = { 'attriblist': [] }
+        self._notifierids[notifierid] = {'attriblist': []}
         if self.tenant not in self._attribwatchers:
             self._attribwatchers[self.tenant] = {}
         attribwatchers = self._attribwatchers[self.tenant]
@@ -432,7 +438,7 @@ class ConfigManager(object):
                 attribwatchers[node] = {}
             for attribute in attributes:
                 self._notifierids[notifierid]['attriblist'].append(
-                    (node,attribute))
+                    (node, attribute))
                 if attribute not in attribwatchers[node]:
                     attribwatchers[node][attribute] = {
                         notifierid: callback
@@ -462,7 +468,7 @@ class ConfigManager(object):
             notifierid = random.randint(0, sys.maxint)
         # going to track that this is a nodecollection type watcher,
         # but there is no additional data associated.
-        self.notifierids[notifierid] = set(['nodecollection'])
+        self._notifierids[notifierid] = set(['nodecollection'])
         if self.tenant not in self._nodecollwatchers:
             self._nodecollwatchers[self.tenant] = {}
         self._nodecollwatchers[self.tenant][notifierid] = callback
@@ -478,13 +484,12 @@ class ConfigManager(object):
             for nodeattrib in self._notifierids[watcher]['attriblist']:
                 node, attrib = nodeattrib
                 del attribwatchers[node][attrib][watcher]
-        elif 'nodecollection' in self.notifierids[watcher]:
+        elif 'nodecollection' in self._notifierids[watcher]:
             del self._nodecollwatchers[self.tenant][watcher]
         else:
             raise Exception("Completely not a valid place to be")
         del self._notifierids[watcher]
 
-
     def list_users(self):
         try:
             return self._cfgstore['users'].iterkeys()
@@ -507,7 +512,6 @@ class ConfigManager(object):
         except:
             return None
 
-
     def set_user(self, name, attributemap):
         """Set user attribute(s)
 
@@ -520,24 +524,23 @@ class ConfigManager(object):
             if attribute == 'passphrase':
                 salt = os.urandom(8)
                 #TODO: WORKERPOOL, offload password set to a worker
-                crypted = kdf.PBKDF2(
+                crypted = KDF.PBKDF2(
                     attributemap[attribute], salt, 32, 10000,
                     lambda p, s: HMAC.new(p, s, SHA256).digest()
-                    )
+                )
                 user['cryptpass'] = (salt, crypted)
             else:
                 user[attribute] = attributemap[attribute]
         self._bg_sync_to_file()
 
     def del_user(self, name):
-        changeset = {}
         if name in self._cfgstore['users']:
             _mark_dirtykey('users', name, self.tenant)
             del self._cfgstore['users'][name]
         self._bg_sync_to_file()
 
     def create_user(self, name,
-                    role="Administrator", id=None, displayname=None,
+                    role="Administrator", uid=None, displayname=None,
                     attributemap=None):
         """Create a new user
 
@@ -545,30 +548,30 @@ class ConfigManager(object):
         :param role: The role the user should be considered.  Can be
                      "Administrator" or "Technician", defaults to
                      "Administrator"
-        :param id: Custom identifier number if desired.  Defaults to random.
+        :param uid: Custom identifier number if desired.  Defaults to random.
         :param displayname: Optional long format name for UI consumption
         """
-        if id is None:
-            id = _generate_new_id()
+        if uid is None:
+            uid = _generate_new_id()
         else:
-            if id in _cfgstore['main']['idmap']:
+            if uid in _cfgstore['main']['idmap']:
                 raise Exception("Duplicate id requested")
         if 'users' not in self._cfgstore:
-            self._cfgstore['users'] = { }
+            self._cfgstore['users'] = {}
         name = name.encode('utf-8')
         if name in self._cfgstore['users']:
             raise Exception("Duplicate username requested")
         _mark_dirtykey('users', name, self.tenant)
-        self._cfgstore['users'][name] = {'id': id}
+        self._cfgstore['users'][name] = {'id': uid}
         if displayname is not None:
             self._cfgstore['users'][name]['displayname'] = displayname
         if 'idmap' not in _cfgstore['main']:
             _cfgstore['main']['idmap'] = {}
-        _mark_dirtykey('idmap', id)
-        _cfgstore['main']['idmap'][id] = {
+        _mark_dirtykey('idmap', uid)
+        _cfgstore['main']['idmap'][uid] = {
             'tenant': self.tenant,
             'username': name
-            }
+        }
         if attributemap is not None:
             self.set_user(name, attributemap)
         self._bg_sync_to_file()
@@ -588,9 +591,9 @@ class ConfigManager(object):
         except KeyError:
             return []
 
-    def get_nodegroup_attributes(self, nodegroup, attributes=[]):
+    def get_nodegroup_attributes(self, nodegroup, attributes=()):
         cfgnodeobj = self._cfgstore['groups'][nodegroup]
-        if len(attributes) == 0:
+        if not attributes:
             attributes = cfgnodeobj.iterkeys()
         nodeobj = {}
         for attribute in attributes:
@@ -602,9 +605,9 @@ class ConfigManager(object):
                                                    decrypt=self.decrypt)
         return nodeobj
 
-    def get_node_attributes(self, nodelist, attributes=[]):
+    def get_node_attributes(self, nodelist, attributes=()):
         retdict = {}
-        if isinstance(nodelist,str) or isinstance(nodelist, unicode):
+        if isinstance(nodelist, str) or isinstance(nodelist, unicode):
             nodelist = [nodelist]
         for node in nodelist:
             if node not in self._cfgstore['nodes']:
@@ -635,7 +638,7 @@ class ConfigManager(object):
             return
         for attrib in groupcfg.iterkeys():
             self._do_inheritance(nodecfg, attrib, node, changeset)
-            self._addchange(changeset, node, attrib)
+            _addchange(changeset, node, attrib)
 
     def _node_removed_from_group(self, node, group, changeset):
         try:
@@ -652,18 +655,19 @@ class ConfigManager(object):
                     _mark_dirtykey('nodes', node, self.tenant)
                     del nodecfg[attrib]  # remove invalid inherited data
                     self._do_inheritance(nodecfg, attrib, node, changeset)
-                    self._addchange(changeset, node, attrib)
+                    _addchange(changeset, node, attrib)
             except KeyError:  # inheritedfrom not set, move on
                 pass
 
-    def _do_inheritance(self, nodecfg, attrib, nodename, changeset, srcgroup=None):
+    def _do_inheritance(self, nodecfg, attrib, nodename, changeset,
+                        srcgroup=None):
         # for now, just do single inheritance
         # TODO: concatenating inheritance if requested
         if attrib in ('nodes', 'groups'):
             #not attributes that should be considered here
             return
         if attrib in nodecfg and 'inheritedfrom' not in nodecfg[attrib]:
-            return # already has a non-inherited value set, nothing to do
+            return  # already has a non-inherited value set, nothing to do
         # if the attribute is not set, this will search for a candidate
         # if it is set, but inheritedfrom, search for a replacement, just
         # in case
@@ -716,7 +720,7 @@ class ConfigManager(object):
                 _mark_dirtykey('nodes', node, self.tenant)
                 self._cfgstore['nodes'][node]['groups'].insert(0, group)
             else:
-                continue # next node, this node already in
+                continue  # next node, this node already in
             self._node_added_to_group(node, group, changeset)
 
     def add_group_attributes(self, attribmap):
@@ -728,18 +732,21 @@ class ConfigManager(object):
             if not autocreate and group not in self._cfgstore['groups']:
                 raise ValueError("{0} group does not exist".format(group))
             for attr in attribmap[group].iterkeys():
-                if attr != 'nodes' and (attr not in allattributes.node or
-                        ('type' in allattributes.node[attr] and
-                        not isinstance(attribmap[node][attr],allattributes.node[attr]['type']))):
+                if (attr != 'nodes' and
+                        (attr not in allattributes.node or
+                        ('type' in allattributes.node[attr] and not isinstance(
+                         attribmap[group][attr],
+                         allattributes.node[attr]['type'])))):
                     raise ValueError
                 if attr == 'nodes':
                     if not isinstance(attribmap[group][attr], list):
-                        raise ValueError("nodes attribute on group must be list")
+                        raise ValueError(
+                            "nodes attribute on group must be list")
                     for node in attribmap[group]['nodes']:
                         if node not in self._cfgstore['nodes']:
                             raise ValueError(
                                 "{0} node does not exist to add to {1}".format(
-                                    node,group))
+                                    node, group))
         for group in attribmap.iterkeys():
             group = group.encode('utf-8')
             _mark_dirtykey('groups', group, self.tenant)
@@ -747,12 +754,11 @@ class ConfigManager(object):
                 self._cfgstore['groups'][group] = {'nodes': set()}
             cfgobj = self._cfgstore['groups'][group]
             for attr in attribmap[group].iterkeys():
-                newdict = {}
                 if attr == 'nodes':
                     newdict = set(attribmap[group][attr])
                 elif (isinstance(attribmap[group][attr], str) or
                         isinstance(attribmap[group][attr], unicode)):
-                    newdict = { 'value': attribmap[group][attr] }
+                    newdict = {'value': attribmap[group][attr]}
                 else:
                     newdict = attribmap[group][attr]
                 if 'value' in newdict and attr.startswith("secret."):
@@ -761,13 +767,14 @@ class ConfigManager(object):
                 cfgobj[attr] = newdict
                 if attr == 'nodes':
                     self._sync_nodes_to_group(group=group,
-                        nodes=attribmap[group]['nodes'],
-                        changeset=changeset)
+                                              nodes=attribmap[group]['nodes'],
+                                              changeset=changeset)
                 else:  # update inheritence
                     for node in cfgobj['nodes']:
                         nodecfg = self._cfgstore['nodes'][node]
-                        self._do_inheritance(nodecfg, attr, node, changeset, srcgroup=group)
-                        self._addchange(changeset, node, attr)
+                        self._do_inheritance(nodecfg, attr, node, changeset,
+                                             srcgroup=group)
+                        _addchange(changeset, node, attr)
         self._notif_attribwatchers(changeset)
         self._bg_sync_to_file()
 
@@ -800,12 +807,14 @@ class ConfigManager(object):
                 for notifierid in attribwatchers[attrname].iterkeys():
                     if notifierid in notifdata:
                         if node in notifdata[notifierid]['nodeattrs']:
-                            notifdata[notifierid]['nodeattrs'][node].append(attrname)
+                            notifdata[notifierid]['nodeattrs'][node].append(
+                                attrname)
                         else:
-                            notifdata[notifierid]['nodeattrs'][node] = [attrname]
+                            notifdata[notifierid]['nodeattrs'][node] = [
+                                attrname]
                     else:
                         notifdata[notifierid] = {
-                            'nodeattrs': { node: [attrname] },
+                            'nodeattrs': {node: [attrname]},
                             'callback': attribwatchers[attrname][notifierid]
                         }
         for watcher in notifdata.itervalues():
@@ -855,7 +864,7 @@ class ConfigManager(object):
                     _mark_dirtykey('nodes', node, self.tenant)
                     del nodek[attrib]
                     self._do_inheritance(nodek, attrib, node, changeset)
-                    self._addchange(changeset, node, attrib)
+                    _addchange(changeset, node, attrib)
                 if ('_expressionkeys' in nodek and
                         attrib in nodek['_expressionkeys']):
                     recalcexpressions = True
@@ -888,8 +897,9 @@ class ConfigManager(object):
                 attrval = attribmap[node][attrname]
                 if (attrname not in allattributes.node or
                         ('type' in allattributes.node[attrname] and
-                        not isinstance(
-                            attrval, allattributes.node[attrname]['type']))):
+                         not isinstance(
+                             attrval,
+                             allattributes.node[attrname]['type']))):
                     errstr = "{0} attribute on node {1} is invalid".format(
                         attrname, node)
                     raise ValueError(errstr)
@@ -898,7 +908,7 @@ class ConfigManager(object):
                         if group not in self._cfgstore['groups']:
                             raise ValueError(
                                 "group {0} does not exist".format(group))
-                    if ('everything' in self._cfgstore['groups'] and 
+                    if ('everything' in self._cfgstore['groups'] and
                             'everything' not in attribmap[node]['groups']):
                         attribmap[node]['groups'].append('everything')
         for node in attribmap.iterkeys():
@@ -912,19 +922,19 @@ class ConfigManager(object):
             cfgobj = self._cfgstore['nodes'][node]
             recalcexpressions = False
             for attrname in attribmap[node].iterkeys():
-                newdict = {}
                 if (isinstance(attribmap[node][attrname], str) or
                         isinstance(attribmap[node][attrname], unicode)):
-                    newdict = {'value': attribmap[node][attrname] }
+                    newdict = {'value': attribmap[node][attrname]}
                 else:
                     newdict = attribmap[node][attrname]
                 if 'value' in newdict and attrname.startswith("secret."):
-                    newdict['cryptvalue' ] = crypt_value(newdict['value'])
+                    newdict['cryptvalue'] = crypt_value(newdict['value'])
                     del newdict['value']
                 cfgobj[attrname] = newdict
                 if attrname == 'groups':
                     self._sync_groups_to_node(node=node,
-                    groups=attribmap[node]['groups'], changeset=changeset)
+                                              groups=attribmap[node]['groups'],
+                                              changeset=changeset)
                 if ('_expressionkeys' in cfgobj and
                         attrname in cfgobj['_expressionkeys']):
                     recalcexpressions = True
@@ -935,7 +945,7 @@ class ConfigManager(object):
                                                          formatter=exprmgr)
                 # if any code is watching these attributes, notify
                 # them of the change
-                self._addchange(changeset, node, attrname)
+                _addchange(changeset, node, attrname)
             if recalcexpressions:
                 if exprmgr is None:
                     exprmgr = _ExpressionFormat(cfgobj, node)
@@ -950,12 +960,6 @@ class ConfigManager(object):
         self._bg_sync_to_file()
         #TODO: wait for synchronization to suceed/fail??)
 
-    def _addchange(self, changeset, node, attrname):
-        if node not in changeset:
-            changeset[node] = { attrname: 1 }
-        else:
-            changeset[node][attrname] = 1
-
     @classmethod
     def _read_from_path(cls):
         global _cfgstore
@@ -968,11 +972,14 @@ class ConfigManager(object):
         try:
             for tenant in os.listdir(rootpath + '/tenants/'):
                 _load_dict_from_dbm(
-                    ['main', tenant, 'nodes'], "%s/%s/nodes" % (rootpath,tenant))
+                    ['main', tenant, 'nodes'],
+                    "%s/%s/nodes" % (rootpath, tenant))
                 _load_dict_from_dbm(
-                    ['main', tenant, 'groups'], "%s/%s/groups" % (rootpath,tenant))
+                    ['main', tenant, 'groups'],
+                    "%s/%s/groups" % (rootpath, tenant))
                 _load_dict_from_dbm(
-                    ['main', tenant, 'users'], "%s/%s/users" % (rootpath,tenant))
+                    ['main', tenant, 'users'],
+                    "%s/%s/users" % (rootpath, tenant))
         except OSError:
             pass
 
@@ -992,10 +999,10 @@ class ConfigManager(object):
     def _sync_to_file(cls):
         if 'dirtyglobals' in _cfgstore:
             with _dirtylock:
-                dirtyglobals = copy.deepcopy(_cfgstore['dirtyglobals']);
+                dirtyglobals = copy.deepcopy(_cfgstore['dirtyglobals'])
                 del _cfgstore['dirtyglobals']
             _mkpath(cls._cfgdir)
-            globalf = dbm.open(cls._cfgdir + "/globals", 'c', 384) # 0600
+            globalf = dbm.open(cls._cfgdir + "/globals", 'c', 384)  # 0600
             for globalkey in dirtyglobals:
                 if globalkey in _cfgstore['globals']:
                     globalf[globalkey] = \
@@ -1018,7 +1025,7 @@ class ConfigManager(object):
                     currdict = _cfgstore['tenant'][tenant]
                 for category in dkdict.iterkeys():
                     _mkpath(pathname)
-                    dbf = dbm.open(pathname + category, 'c', 384) # 0600 mode
+                    dbf = dbm.open(pathname + category, 'c', 384)  # 0600 mode
                     for ck in dkdict[category]:
                         if ck not in currdict[category]:
                             if ck in dbf:
@@ -1029,19 +1036,20 @@ class ConfigManager(object):
             cls._writepending = False
             return cls._sync_to_file()
 
-    def _recalculate_expressions(self, cfgobj, formatter, node):
+    def _recalculate_expressions(self, cfgobj, formatter, node, changeset):
         for key in cfgobj.iterkeys():
-            if not isinstance(cfgobj[key],dict):
+            if not isinstance(cfgobj[key], dict):
                 continue
             if 'expression' in cfgobj[key]:
                 cfgobj[key] = _decode_attribute(key, cfgobj,
                                                 formatter=formatter)
-                self._addchange(changeset, node, key)
+                _addchange(changeset, node, key)
             elif ('cryptvalue' not in cfgobj[key] and
                     'value' not in cfgobj[key]):
-                # recurse for nested structures, with some hint tha
+                # recurse for nested structures, with some hint that
                 # it might indeed be a nested structure
-                _recalculate_expressions(cfgobj[key], formatter, node)
+                self._recalculate_expressions(cfgobj[key], formatter, node,
+                                              changeset)
 
 
 try:
diff --git a/confluent/consoleserver.py b/confluent/consoleserver.py
index f640b47b..f0b1131b 100644
--- a/confluent/consoleserver.py
+++ b/confluent/consoleserver.py
@@ -58,6 +58,7 @@ class _ConsoleHandler(object):
         self._attribwatcher = None
         self._console = None
         self.connectionthread = None
+        self.send_break = None
         eventlet.spawn(self._connect)
 
     def _attribschanged(self, **kwargs):
@@ -110,7 +111,7 @@ class _ConsoleHandler(object):
         self._send_rcpts({'connectstate': self.connectstate})
 
     def _got_disconnected(self):
-        self.connecstate = 'unconnected'
+        self.connectstate = 'unconnected'
         self.logger.log(
             logdata='console disconnected', ltype=log.DataTypes.event,
             event=log.Events.consoledisconnect)
@@ -191,9 +192,9 @@ class _ConsoleHandler(object):
             self.shiftin = '0'
         eventdata = 0
         if self.appmodedetected:
-            eventdata = eventdata | 1
+            eventdata |= 1
         if self.shiftin is not None:
-            eventdata = eventdata | 2
+            eventdata |= 2
         self.logger.log(data, eventdata=eventdata)
         self.buffer += data
         #TODO: analyze buffer for registered events, examples:
@@ -235,18 +236,18 @@ class _ConsoleHandler(object):
         #this is one scheme to clear screen, move cursor then clear
         bufidx = self.buffer.rfind('\x1b[H\x1b[J')
         if bufidx >= 0:
-            return (retdata + str(self.buffer[bufidx:]), connstate)
+            return retdata + str(self.buffer[bufidx:]), connstate
         #another scheme is the 2J scheme
         bufidx = self.buffer.rfind('\x1b[2J')
         if bufidx >= 0:
             # there was some sort of clear screen event
             # somewhere in the buffer, replay from that point
             # in hopes that it reproduces the screen
-            return (retdata + str(self.buffer[bufidx:]), connstate)
+            return retdata + str(self.buffer[bufidx:]), connstate
         else:
             #we have no indication of last erase, play back last kibibyte
             #to give some sense of context anyway
-            return (retdata + str(self.buffer[-1024:]), connstate)
+            return retdata + str(self.buffer[-1024:]), connstate
 
     def write(self, data):
         if self.connectstate == 'connected':
diff --git a/confluent/interface/console.py b/confluent/interface/console.py
index 538f57db..4475d4ec 100644
--- a/confluent/interface/console.py
+++ b/confluent/interface/console.py
@@ -14,6 +14,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+
 class ConsoleEvent(object):
     """This represents a number of specific events to be sent between
     consoleserver and console objects.  Disconnect indicates that the console
diff --git a/confluent/pluginapi.py b/confluent/pluginapi.py
index 7dc6c32e..84bbfa24 100644
--- a/confluent/pluginapi.py
+++ b/confluent/pluginapi.py
@@ -80,6 +80,7 @@ rootcollections = ['nodes/', 'groups/', 'users/']
 class PluginRoute(object):
     def __init__(self, routedict):
         self.routeinfo = routedict
+
 # _ prefix indicates internal use (e.g. special console scheme) and should not
 # be enumerated in any collection
 noderesources = {
@@ -139,6 +140,7 @@ def update_user(name, attribmap, configmanager):
     except ValueError:
         raise exc.InvalidArgumentException()
 
+
 def show_user(name, configmanager):
     userobj = configmanager.get_user(name)
     rv = {}
@@ -148,7 +150,8 @@ def show_user(name, configmanager):
             if 'cryptpass' in userobj:
                 rv['passphrase'] = {'cryptvalue': True}
             yield msg.CryptedAttributes(kv={'passphrase': rv['passphrase']},
-                                        desc=attrscheme.user[attr]['description'])
+                                        desc=attrscheme.user[attr][
+                                            'description'])
         else:
             if attr in userobj:
                 rv[attr] = userobj[attr]
@@ -165,7 +168,7 @@ def stripnode(iterablersp, node):
 def iterate_collections(iterable, forcecollection=True):
     for coll in iterable:
         if forcecollection and coll[-1] != '/':
-            coll = coll + '/'
+            coll += '/'
         yield msg.ChildCollection(coll, candelete=True)
 
 
@@ -182,6 +185,7 @@ def delete_user(user, configmanager):
     configmanager.del_user(user)
     yield msg.DeletedResource(user)
 
+
 def delete_nodegroup_collection(collectionpath, configmanager):
     if len(collectionpath) == 2:  # just the nodegroup
         group = collectionpath[-1]
@@ -190,6 +194,7 @@ def delete_nodegroup_collection(collectionpath, configmanager):
     else:
         raise Exception("Not implemented")
 
+
 def delete_node_collection(collectionpath, configmanager):
     if len(collectionpath) == 2:  # just node
         node = collectionpath[-1]
@@ -207,8 +212,9 @@ def enumerate_nodegroup_collection(collectionpath, configmanager):
     collection = nested_lookup(nodegroupresources, collectionpath)
     return iterate_resources(collection)
 
+
 def enumerate_node_collection(collectionpath, configmanager):
-    if collectionpath == ['nodes']:  # it is just '/node/', need a list of nodes
+    if collectionpath == ['nodes']:  # it is just '/node/', need to list nodes
         return iterate_collections(configmanager.list_nodes())
     node = collectionpath[1]
     if not configmanager.is_node(node):
@@ -246,12 +252,12 @@ def enumerate_collections(collections):
 
 
 def handle_path(path, operation, configmanager, inputdata=None):
-    '''Given a full path request, return an object.
+    """Given a full path request, return an object.
 
     The plugins should generally return some sort of iterator.
     An exception is made for console/session, which should return
     a class with connect(), read(), write(bytes), and close()
-    '''
+    """
     iscollection = False
     pathcomponents = path.split('/')
     del pathcomponents[0]  # discard the value from leading /
@@ -261,9 +267,7 @@ def handle_path(path, operation, configmanager, inputdata=None):
     if not pathcomponents:  # root collection list
         return enumerate_collections(rootcollections)
     elif pathcomponents[0] == 'groups':
-        try:
-            group = pathcomponents[1]
-        except IndexError:
+        if len(pathcomponents) < 2:
             if operation == "create":
                 inputdata = msg.InputAttributes(pathcomponents, inputdata)
                 create_group(inputdata.attribs, configmanager)
@@ -272,13 +276,16 @@ def handle_path(path, operation, configmanager, inputdata=None):
             iscollection = True
         if iscollection:
             if operation == "delete":
-                return delete_nodegroup_collection(pathcomponents, configmanager)
+                return delete_nodegroup_collection(pathcomponents,
+                                                   configmanager)
             elif operation == "retrieve":
-                return enumerate_nodegroup_collection(pathcomponents, configmanager)
+                return enumerate_nodegroup_collection(pathcomponents,
+                                                      configmanager)
             else:
                 raise Exception("TODO")
         try:
-            plugroute = nested_lookup(nodegroupresources, pathcomponents[2:]).routeinfo
+            plugroute = nested_lookup(
+                nodegroupresources, pathcomponents[2:]).routeinfo
         except KeyError:
             raise exc.NotFoundException("Invalid element requested")
         inputdata = msg.get_input_message(
@@ -310,6 +317,7 @@ def handle_path(path, operation, configmanager, inputdata=None):
             else:
                 raise Exception("TODO here")
         del pathcomponents[0:2]
+        passvalue = None
         try:
             plugroute = nested_lookup(noderesources, pathcomponents).routeinfo
         except KeyError:
@@ -346,12 +354,13 @@ def handle_path(path, operation, configmanager, inputdata=None):
         # they must only be allowed to see their own user
         try:
             user = pathcomponents[1]
-        except IndexError: # it's just users/
+        except IndexError:  # it's just users/
             if operation == 'create':
                 inputdata = msg.get_input_message(
                     pathcomponents, operation, inputdata)
                 create_user(inputdata.attribs, configmanager)
-            return iterate_collections(configmanager.list_users(), forcecollection=False)
+            return iterate_collections(configmanager.list_users(),
+                                       forcecollection=False)
         if user not in configmanager.list_users():
             raise exc.NotFoundException("Invalid user %s" % user)
         if operation == 'retrieve':
diff --git a/confluent/sockapi.py b/confluent/sockapi.py
index 3421952b..05440959 100644
--- a/confluent/sockapi.py
+++ b/confluent/sockapi.py
@@ -18,29 +18,31 @@
 # This is the socket api layer.
 # It implement unix and tls sockets
 # 
-# TODO: SO_PEERCRED for unix socket
-import confluent.auth as auth
-import confluent.common.tlvdata as tlvdata
-import confluent.consoleserver as consoleserver
-import confluent.config.configmanager as configmanager
-import confluent.exceptions as exc
-import confluent.log as log
-import confluent.messages
-import confluent.pluginapi as pluginapi
-import eventlet.green.socket as socket
-import eventlet.green.ssl as ssl
-import eventlet
-import json
+
 import os
 import pwd
 import stat
 import struct
 import traceback
 
+import eventlet.green.socket as socket
+import eventlet.green.ssl as ssl
+import eventlet
+
+import confluent.auth as auth
+import confluent.common.tlvdata as tlvdata
+import confluent.consoleserver as consoleserver
+import confluent.config.configmanager as configmanager
+import confluent.exceptions as exc
+import confluent.log as log
+import confluent.pluginapi as pluginapi
+
+
 tracelog = None
 auditlog = None
 SO_PEERCRED = 17
 
+
 class ClientConsole(object):
     def __init__(self, client):
         self.client = client
@@ -72,7 +74,7 @@ def sessionhdl(connection, authname, skipauth):
         if authdata is not None:
             cfm = authdata[1]
             authenticated = True
-    tlvdata.send(connection,"Confluent -- v0 --")
+    tlvdata.send(connection, "Confluent -- v0 --")
     while not authenticated:  # prompt for name and passphrase
         tlvdata.send(connection, {'authpassed': 0})
         response = tlvdata.recv(connection)
@@ -209,7 +211,7 @@ def _unixdomainhandler():
              stat.S_IWOTH | stat.S_IROTH | stat.S_IWGRP |
              stat.S_IRGRP | stat.S_IWUSR | stat.S_IRUSR)
     unixsocket.listen(5)
-    while (1):
+    while True:
         cnn, addr = unixsocket.accept()
         creds = cnn.getsockopt(socket.SOL_SOCKET, SO_PEERCRED,
                                 struct.calcsize('3i'))
@@ -234,8 +236,11 @@ def _unixdomainhandler():
         eventlet.spawn_n(sessionhdl, cnn, authname, skipauth)
 
 
-
 class SockApi(object):
+    def __init__(self):
+        self.tlsserver = None
+        self.unixdomainserver = None
+
     def start(self):
         global auditlog
         global tracelog
diff --git a/confluent/util.py b/confluent/util.py
index 7f4cb952..94298bb6 100644
--- a/confluent/util.py
+++ b/confluent/util.py
@@ -26,13 +26,13 @@ def randomstring(length=20):
     :param length: The number of characters to produce, defaults to 20
     """
     chunksize = length / 4
-    if (length % 4 > 0):
+    if length % 4 > 0:
         chunksize += 1
     strval = base64.urlsafe_b64encode(os.urandom(chunksize * 3))
     return strval[0:length-1]
 
 
-def securerandomnumber(min=0, max=4294967295):
+def securerandomnumber(low=0, high=4294967295):
     """Return a random number within requested range
 
     Note that this function will not return smaller than 0 nor larger
@@ -40,11 +40,11 @@ def securerandomnumber(min=0, max=4294967295):
     The python random number facility does not provide charateristics
     appropriate for secure rng, go to os.urandom
 
-    :param min: Smallest number to return (defaults to 0)
-    :param max: largest number to return (defaults to 2^32-1)
+    :param low: Smallest number to return (defaults to 0)
+    :param high: largest number to return (defaults to 2^32-1)
     """
     number = -1
-    while number < min or number > max:
+    while number < low or number > high:
         number = struct.unpack("I", os.urandom(4))[0]
     return number