Made the plugin output fully json-ified, so I could remove the exception handlers in vmops.py. Cleaned up some pep8 issues that weren't caught in earlier runs.

nova/virt/xenapi/vmops.py

@@ -289,11 +289,7 @@ class VMOps(object):
         found at that path, returns None.
         """
         ret = self._make_xenstore_call('list_records', vm, path)
-        try:
+        return json.loads(ret)
-            return json.loads(ret)
-        except ValueError:
-            # Not a valid JSON value
-            return ret
 
     def read_from_xenstore(self, vm, path):
         """Returns the value stored in the xenstore record for the given VM
@@ -305,14 +301,11 @@ class VMOps(object):
                     {'ignore_missing_path': 'True'})
         except self.XenAPI.Failure, e:
             return None
-        try:
+        ret = json.loads(ret)
-            return json.loads(ret)
+        if ret == "None":
-        except ValueError:
+            # Can't marshall None over RPC calls.
-            # Not a JSON object
+            return None
-            if ret == "None":
+        return ret
-                # Can't marshall None over RPC calls.
-                return None
-            return ret
 
     def write_to_xenstore(self, vm, path, value):
         """Writes the passed value to the xenstore record for the given VM

plugins/xenserver/xenapi/etc/xapi.d/plugins/pluginlib_nova.py

@@ -45,6 +45,7 @@ class PluginError(Exception):
     def __init__(self, *args):
         Exception.__init__(self, *args)
 
+
 class ArgumentError(PluginError):
     """Raised when required arguments are missing, argument values are invalid,
     or incompatible arguments are given.
@@ -67,6 +68,7 @@ def ignore_failure(func, *args, **kwargs):
 
 ARGUMENT_PATTERN = re.compile(r'^[a-zA-Z0-9_:\.\-,]+$')
 
+
 def validate_exists(args, key, default=None):
     """Validates that a string argument to a RPC method call is given, and
     matches the shell-safe regex, with an optional default value in case it
@@ -76,20 +78,24 @@ def validate_exists(args, key, default=None):
     """
     if key in args:
         if len(args[key]) == 0:
-            raise ArgumentError('Argument %r value %r is too short.' % (key, args[key]))
+            raise ArgumentError('Argument %r value %r is too short.' %
+                    (key, args[key]))
         if not ARGUMENT_PATTERN.match(args[key]):
-            raise ArgumentError('Argument %r value %r contains invalid characters.' % (key, args[key]))
+            raise ArgumentError('Argument %r value %r contains invalid '
+                    'characters.' % (key, args[key]))
         if args[key][0] == '-':
-            raise ArgumentError('Argument %r value %r starts with a hyphen.' % (key, args[key]))
+            raise ArgumentError('Argument %r value %r starts with a hyphen.'
+                    % (key, args[key]))
         return args[key]
     elif default is not None:
         return default
     else:
         raise ArgumentError('Argument %s is required.' % key)
 
+
 def validate_bool(args, key, default=None):
-    """Validates that a string argument to a RPC method call is a boolean string,
+    """Validates that a string argument to a RPC method call is a boolean
-    with an optional default value in case it does not exist.
+    string, with an optional default value in case it does not exist.
 
     Returns the python boolean value.
     """
@@ -99,7 +105,9 @@ def validate_bool(args, key, default=None):
     elif value.lower() == 'false':
         return False
     else:
-        raise ArgumentError("Argument %s may not take value %r. Valid values are ['true', 'false']." % (key, value))
+        raise ArgumentError("Argument %s may not take value %r. "
+                "Valid values are ['true', 'false']." % (key, value))
+
 
 def exists(args, key):
     """Validates that a freeform string argument to a RPC method call is given.
@@ -110,6 +118,7 @@ def exists(args, key):
     else:
         raise ArgumentError('Argument %s is required.' % key)
 
+
 def optional(args, key):
     """If the given key is in args, return the corresponding value, otherwise
     return None"""
@@ -122,13 +131,14 @@ def get_this_host(session):
 
 def get_domain_0(session):
     this_host_ref = get_this_host(session)
-    expr = 'field "is_control_domain" = "true" and field "resident_on" = "%s"' % this_host_ref
+    expr = 'field "is_control_domain" = "true" and field "resident_on" = "%s"'
+    expr = expr % this_host_ref
     return session.xenapi.VM.get_all_records_where(expr).keys()[0]
 
 
 def create_vdi(session, sr_ref, name_label, virtual_size, read_only):
     vdi_ref = session.xenapi.VDI.create(
-        { 'name_label': name_label,
+         {'name_label': name_label,
           'name_description': '',
           'SR': sr_ref,
           'virtual_size': str(virtual_size),
@@ -138,7 +148,7 @@ def create_vdi(session, sr_ref, name_label, virtual_size, read_only):
           'xenstore_data': {},
           'other_config': {},
           'sm_config': {},
-          'tags': [] })
+          'tags': []})
     logging.debug('Created VDI %s (%s, %s, %s) on %s.', vdi_ref, name_label,
                   virtual_size, read_only, sr_ref)
     return vdi_ref

plugins/xenserver/xenapi/etc/xapi.d/plugins/xenstore.py

@@ -34,10 +34,17 @@ import pluginlib_nova as pluginlib
 pluginlib.configure_logging("xenstore")
 
 
+def jsonify(fnc):
+    def wrapper(*args, **kwargs):
+        return json.dumps(fnc(*args, **kwargs))
+    return wrapper
+
+
+@jsonify
 def read_record(self, arg_dict):
     """Returns the value stored at the given path for the given dom_id.
-    These must be encoded as key/value pairs in arg_dict. You can 
+    These must be encoded as key/value pairs in arg_dict. You can
-    optinally include a key 'ignore_missing_path'; if this is present 
+    optinally include a key 'ignore_missing_path'; if this is present
     and boolean True, attempting to read a non-existent path will return
     the string 'None' instead of raising an exception.
     """
@@ -46,16 +53,21 @@ def read_record(self, arg_dict):
         return _run_command(cmd).rstrip("\n")
     except pluginlib.PluginError, e:
         if arg_dict.get("ignore_missing_path", False):
-            cmd = "xenstore-exists /local/domain/%(dom_id)s/%(path)s; echo $?" % arg_dict
+            cmd = "xenstore-exists /local/domain/%(dom_id)s/%(path)s; echo $?"
+            cmd = cmd % arg_dict
             ret = _run_command(cmd).strip()
             # If the path exists, the cmd should return "0"
             if ret != "0":
-                # No such path, so ignore the error
+                # No such path, so ignore the error and return the
-                return "None" 
+                # string 'None', since None can't be marshalled
+                # over RPC.
+                return "None"
         # Either we shouldn't ignore path errors, or another
         # error was hit. Re-raise.
         raise
 
+
+@jsonify
 def write_record(self, arg_dict):
     """Writes to xenstore at the specified path. If there is information
     already stored in that location, it is overwritten. As in read_record,
@@ -63,10 +75,13 @@ def write_record(self, arg_dict):
     you must specify a 'value' key, whose value must be a string. Typically,
     you can json-ify more complex values and store the json output.
     """
-    cmd = "xenstore-write /local/domain/%(dom_id)s/%(path)s '%(value)s'" % arg_dict
+    cmd = "xenstore-write /local/domain/%(dom_id)s/%(path)s '%(value)s'"
+    cmd = cmd % arg_dict
     _run_command(cmd)
     return arg_dict["value"]
 
+
+@jsonify
 def list_records(self, arg_dict):
     """Returns all the stored data at or below the given path for the
     given dom_id. The data is returned as a json-ified dict, with the
@@ -80,7 +95,8 @@ def list_records(self, arg_dict):
     except pluginlib.PluginError, e:
         if "No such file or directory" in "%s" % e:
             # Path doesn't exist.
-            return json.dumps({})
+            return {}
+        return str(e)
         raise
     base_path = arg_dict["path"]
     paths = _paths_from_ls(recs)
@@ -96,8 +112,10 @@ def list_records(self, arg_dict):
         except ValueError:
             val = rec
         ret[path] = val
-    return json.dumps(ret)
+    return ret
 
+
+@jsonify
 def delete_record(self, arg_dict):
     """Just like it sounds: it removes the record for the specified
     VM and the specified path from xenstore.
@@ -105,6 +123,7 @@ def delete_record(self, arg_dict):
     cmd = "xenstore-rm /local/domain/%(dom_id)s/%(path)s" % arg_dict
     return _run_command(cmd)
 
+
 def _paths_from_ls(recs):
     """The xenstore-ls command returns a listing that isn't terribly
     useful. This method cleans that up into a dict with each path
@@ -137,13 +156,15 @@ def _paths_from_ls(recs):
         last_nm = barename
     return ret
 
+
 def _run_command(cmd):
     """Abstracts out the basics of issuing system commands. If the command
     returns anything in stderr, a PluginError is raised with that information.
     Otherwise, the output from stdout is returned.
     """
     pipe = subprocess.PIPE
-    proc = subprocess.Popen([cmd], shell=True, stdin=pipe, stdout=pipe, stderr=pipe, close_fds=True)
+    proc = subprocess.Popen([cmd], shell=True, stdin=pipe, stdout=pipe,
+            stderr=pipe, close_fds=True)
     proc.wait()
     err = proc.stderr.read()
     if err: