Private PR: Review changes of the final to commits to cove is_str() changes in datapath.py

.github/workflows/other.yml

@@ -49,13 +49,10 @@ jobs:
         if: ${{ matrix.python-version == '2.7' }}
         run: >
           pip install enum future mock pytest-coverage pytest-mock &&
-          pytest
-          --cov=scripts --cov=ocaml/xcp-rrdd
-          scripts/ ocaml/xcp-rrdd -vv -rA
-          --junitxml=.git/pytest${{matrix.python-version}}.xml
+          pytest -vv -rA --cov=ocaml ocaml
           --cov-report term-missing
           --cov-report xml:.git/coverage${{matrix.python-version}}.xml
-          --cov-fail-under 0
+          --cov-fail-under 60
         env:
           PYTHONDEVMODE: yes
 

ocaml/xapi-storage/python/xapi/__init__.py

@@ -31,11 +31,23 @@
 import json
 import argparse
 
-# pylint: disable=invalid-name,redefined-builtin,undefined-variable
-# pyright: reportUndefinedVariable=false
+
+# is_str(): Shortcut to check if a value is an instance of a string type.
+#
+# Replace:
+#     if not isinstance(code, str) and not isinstance(code, unicode):
+# with:
+#     if not is_str(code):
+#
+# This makes for much cleaner code and suits Python3 well too.
 if sys.version_info[0] > 2:
     long = int
-    unicode = str
+    def is_str(x):
+        return isinstance(x, str)  # With Python3, all strings are unicode
+else:
+    def is_str(x):  # pragma: no cover
+        return isinstance(x, (str, unicode))  # pylint: disable=undefined-variable
+
 
 def success(result):
     return {"Status": "Success", "Value": result}
@@ -72,7 +84,7 @@ class XenAPIException(Exception):
 
     def __init__(self, code, params):
         Exception.__init__(self)
-        if not isinstance(code, str) and not isinstance(code, unicode):
+        if not is_str(code):
             raise TypeError("string", repr(code))
         if not isinstance(params, list):
             raise TypeError("list", repr(params))
@@ -124,7 +136,7 @@ def __init__(self, thing, ty, desc):
             "UnmarshalException thing=%s ty=%s desc=%s" % (thing, ty, desc))
 
 
-class TypeError(InternalError):
+class TypeError(InternalError):  # pylint: disable=redefined-builtin
 
     def __init__(self, expected, actual):
         InternalError.__init__(

ocaml/xapi-storage/python/xapi/storage/api/datapath.py

@@ -1,11 +1,23 @@
 from __future__ import print_function
-from xapi import success, Rpc_light_failure, InternalError, UnmarshalException, TypeError, is_long, UnknownMethod
-import xapi
-import sys
-import json
+
 import argparse
-import traceback
+import json
 import logging
+import sys
+import traceback
+
+import xapi
+# pylint: disable=line-too-long,superfluous-parens,unused-argument
+# pylint: disable-next=redefined-builtin  # FIXME: TypeError is a custom class in xapi
+from xapi import (
+    InternalError,
+    Rpc_light_failure,
+    TypeError,
+    UnknownMethod,
+    UnmarshalException,
+    is_str,
+    success,
+)
 
 # pylint: disable=invalid-name,redefined-builtin,undefined-variable
 # pyright: reportUndefinedVariable=false
@@ -15,7 +27,7 @@
 class Unimplemented(Rpc_light_failure):
     def __init__(self, arg_0):
         Rpc_light_failure.__init__(self, "Unimplemented", [ arg_0 ])
-        if not isinstance(arg_0, str) and not isinstance(arg_0, unicode):
+        if not is_str(arg_0):
             raise TypeError("string", repr(arg_0))
         self.arg_0 = arg_0
 class Datapath_server_dispatcher:
@@ -30,12 +42,12 @@ def open(self, args):
         if not('dbg' in args):
             raise UnmarshalException('argument missing', 'dbg', '')
         dbg = args["dbg"]
-        if not isinstance(dbg, str) and not isinstance(dbg, unicode):
+        if not is_str(dbg):
             raise TypeError("string", repr(dbg))
         if not('uri' in args):
             raise UnmarshalException('argument missing', 'uri', '')
         uri = args["uri"]
-        if not isinstance(uri, str) and not isinstance(uri, unicode):
+        if not is_str(uri):
             raise TypeError("string", repr(uri))
         if not('persistent' in args):
             raise UnmarshalException('argument missing', 'persistent', '')
@@ -51,29 +63,29 @@ def attach(self, args):
         if not('dbg' in args):
             raise UnmarshalException('argument missing', 'dbg', '')
         dbg = args["dbg"]
-        if not isinstance(dbg, str) and not isinstance(dbg, unicode):
+        if not is_str(dbg):
             raise TypeError("string", repr(dbg))
         if not('uri' in args):
             raise UnmarshalException('argument missing', 'uri', '')
         uri = args["uri"]
-        if not isinstance(uri, str) and not isinstance(uri, unicode):
+        if not is_str(uri):
             raise TypeError("string", repr(uri))
         if not('domain' in args):
             raise UnmarshalException('argument missing', 'domain', '')
         domain = args["domain"]
-        if not isinstance(domain, str) and not isinstance(domain, unicode):
+        if not is_str(domain):
             raise TypeError("string", repr(domain))
         results = self._impl.attach(dbg, uri, domain)
-        if not isinstance(results['domain_uuid'], str) and not isinstance(results['domain_uuid'], unicode):
+        if not is_str(results['domain_uuid']):
             raise TypeError("string", repr(results['domain_uuid']))
         if results['implementation'][0] == 'Blkback':
-            if not isinstance(results['implementation'][1], str) and not isinstance(results['implementation'][1], unicode):
+            if not is_str(results['implementation'][1]):
                 raise TypeError("string", repr(results['implementation'][1]))
         elif results['implementation'][0] == 'Tapdisk3':
-            if not isinstance(results['implementation'][1], str) and not isinstance(results['implementation'][1], unicode):
+            if not is_str(results['implementation'][1]):
                 raise TypeError("string", repr(results['implementation'][1]))
         elif results['implementation'][0] == 'Qdisk':
-            if not isinstance(results['implementation'][1], str) and not isinstance(results['implementation'][1], unicode):
+            if not is_str(results['implementation'][1]):
                 raise TypeError("string", repr(results['implementation'][1]))
         return results
     def activate(self, args):
@@ -83,17 +95,17 @@ def activate(self, args):
         if not('dbg' in args):
             raise UnmarshalException('argument missing', 'dbg', '')
         dbg = args["dbg"]
-        if not isinstance(dbg, str) and not isinstance(dbg, unicode):
+        if not is_str(dbg):
             raise TypeError("string", repr(dbg))
         if not('uri' in args):
             raise UnmarshalException('argument missing', 'uri', '')
         uri = args["uri"]
-        if not isinstance(uri, str) and not isinstance(uri, unicode):
+        if not is_str(uri):
             raise TypeError("string", repr(uri))
         if not('domain' in args):
             raise UnmarshalException('argument missing', 'domain', '')
         domain = args["domain"]
-        if not isinstance(domain, str) and not isinstance(domain, unicode):
+        if not is_str(domain):
             raise TypeError("string", repr(domain))
         results = self._impl.activate(dbg, uri, domain)
         return results
@@ -104,17 +116,17 @@ def deactivate(self, args):
         if not('dbg' in args):
             raise UnmarshalException('argument missing', 'dbg', '')
         dbg = args["dbg"]
-        if not isinstance(dbg, str) and not isinstance(dbg, unicode):
+        if not is_str(dbg):
             raise TypeError("string", repr(dbg))
         if not('uri' in args):
             raise UnmarshalException('argument missing', 'uri', '')
         uri = args["uri"]
-        if not isinstance(uri, str) and not isinstance(uri, unicode):
+        if not is_str(uri):
             raise TypeError("string", repr(uri))
         if not('domain' in args):
             raise UnmarshalException('argument missing', 'domain', '')
         domain = args["domain"]
-        if not isinstance(domain, str) and not isinstance(domain, unicode):
+        if not is_str(domain):
             raise TypeError("string", repr(domain))
         results = self._impl.deactivate(dbg, uri, domain)
         return results
@@ -125,17 +137,17 @@ def detach(self, args):
         if not('dbg' in args):
             raise UnmarshalException('argument missing', 'dbg', '')
         dbg = args["dbg"]
-        if not isinstance(dbg, str) and not isinstance(dbg, unicode):
+        if not is_str(dbg):
             raise TypeError("string", repr(dbg))
         if not('uri' in args):
             raise UnmarshalException('argument missing', 'uri', '')
         uri = args["uri"]
-        if not isinstance(uri, str) and not isinstance(uri, unicode):
+        if not is_str(uri):
             raise TypeError("string", repr(uri))
         if not('domain' in args):
             raise UnmarshalException('argument missing', 'domain', '')
         domain = args["domain"]
-        if not isinstance(domain, str) and not isinstance(domain, unicode):
+        if not is_str(domain):
             raise TypeError("string", repr(domain))
         results = self._impl.detach(dbg, uri, domain)
         return results
@@ -146,12 +158,12 @@ def close(self, args):
         if not('dbg' in args):
             raise UnmarshalException('argument missing', 'dbg', '')
         dbg = args["dbg"]
-        if not isinstance(dbg, str) and not isinstance(dbg, unicode):
+        if not is_str(dbg):
             raise TypeError("string", repr(dbg))
         if not('uri' in args):
             raise UnmarshalException('argument missing', 'uri', '')
         uri = args["uri"]
-        if not isinstance(uri, str) and not isinstance(uri, unicode):
+        if not is_str(uri):
             raise TypeError("string", repr(uri))
         results = self._impl.close(dbg, uri)
         return results
@@ -193,18 +205,39 @@ def close(self, dbg, uri):
         """Xapi will call the functions here on VM start/shutdown/suspend/resume/migrate. Every function is idempotent. Every function takes a domain parameter which allows the implementation to track how many domains are currently using the volume."""
         raise Unimplemented("Datapath.close")
 class Datapath_test:
-    """Xapi will call the functions here on VM start/shutdown/suspend/resume/migrate. Every function is idempotent. Every function takes a domain parameter which allows the implementation to track how many domains are currently using the volume."""
+    """
+    Xapi will call the functions here on VM start/shutdown/suspend/resume/migrate.
+    Every function is idempotent. Every function takes a domain parameter which allows
+    the implementation to track how many domains are currently using the volume.
+    """
     def __init__(self):
         pass
     def open(self, dbg, uri, persistent):
         """Xapi will call the functions here on VM start/shutdown/suspend/resume/migrate. Every function is idempotent. Every function takes a domain parameter which allows the implementation to track how many domains are currently using the volume."""
         result = {}
         return result
     def attach(self, dbg, uri, domain):
-        """Xapi will call the functions here on VM start/shutdown/suspend/resume/migrate. Every function is idempotent. Every function takes a domain parameter which allows the implementation to track how many domains are currently using the volume."""
-        result = {}
-        result["backend"] = { "domain_uuid": "string", "implementation": None }
+        # type:(str, str, str) -> dict[str, tuple[str, Any] | str]
+        """
+        Return a valid results dictionary to Datapath_server_dispatcher.attach()
+
+        The returned dict must contain the "domain_uuid" key with a string value.
+        The returned dict must contain the "implementation" key with two elements:
+        If the first element is one of "Blkback", "Tapdisk3" or "Qdisk",
+        the second element must be a string. Else, the dispatcher returns an error.
+
+        See Datapath_server_dispatcher.attach() for the implementation details.
+        """
+        # Fixed to not raise an internal error in Datapath_server_dispatcher.attach():
+        result = { "domain_uuid": domain, "implementation": (uri, dbg) }
+        if not domain:  # Provoke an internal error in the dispatcher to cover its code
+            result.pop("domain_uuid")  # by removing the required "domain_uuid" key.
+        if domain == "5":
+            result["domain_uuid"] = 5  # Return an integer to provoke a type error.
+        if dbg == "inject_error" and uri in ["Blkback", "Tapdisk3", "Qdisk"]:
+            result["implementation"] = (uri, False)
         return result
+
     def activate(self, dbg, uri, domain):
         """Xapi will call the functions here on VM start/shutdown/suspend/resume/migrate. Every function is idempotent. Every function takes a domain parameter which allows the implementation to track how many domains are currently using the volume."""
         result = {}

ocaml/xapi-storage/python/xapi/storage/api/plugin.py

@@ -1,11 +1,23 @@
 from __future__ import print_function
-from xapi import success, Rpc_light_failure, InternalError, UnmarshalException, TypeError, is_long, UnknownMethod
-import xapi
-import sys
-import json
+
 import argparse
-import traceback
+import json
 import logging
+import sys
+import traceback
+
+import xapi
+# pylint: disable=line-too-long,superfluous-parens,unused-argument
+# pylint: disable-next=redefined-builtin  # FIXME: TypeError is a custom class in xapi
+from xapi import (
+    InternalError,
+    Rpc_light_failure,
+    TypeError,
+    UnknownMethod,
+    UnmarshalException,
+    is_str,
+    success,
+)
 
 # pylint: disable=invalid-name,redefined-builtin,undefined-variable
 # pyright: reportUndefinedVariable=false
@@ -15,7 +27,7 @@
 class Unimplemented(Rpc_light_failure):
     def __init__(self, arg_0):
         Rpc_light_failure.__init__(self, "Unimplemented", [ arg_0 ])
-        if not isinstance(arg_0, str) and not isinstance(arg_0, unicode):
+        if not is_str(arg_0):
             raise TypeError("string", repr(arg_0))
         self.arg_0 = arg_0
 class Plugin_server_dispatcher:
@@ -30,40 +42,40 @@ def query(self, args):
         if not('dbg' in args):
             raise UnmarshalException('argument missing', 'dbg', '')
         dbg = args["dbg"]
-        if not isinstance(dbg, str) and not isinstance(dbg, unicode):
+        if not is_str(dbg):
             raise TypeError("string", repr(dbg))
         results = self._impl.query(dbg)
-        if not isinstance(results['plugin'], str) and not isinstance(results['plugin'], unicode):
+        if not is_str(results['plugin']):
             raise TypeError("string", repr(results['plugin']))
-        if not isinstance(results['name'], str) and not isinstance(results['name'], unicode):
+        if not is_str(results['name']):
             raise TypeError("string", repr(results['name']))
-        if not isinstance(results['description'], str) and not isinstance(results['description'], unicode):
+        if not is_str(results['description']):
             raise TypeError("string", repr(results['description']))
-        if not isinstance(results['vendor'], str) and not isinstance(results['vendor'], unicode):
+        if not is_str(results['vendor']):
             raise TypeError("string", repr(results['vendor']))
-        if not isinstance(results['copyright'], str) and not isinstance(results['copyright'], unicode):
+        if not is_str(results['copyright']):
             raise TypeError("string", repr(results['copyright']))
-        if not isinstance(results['version'], str) and not isinstance(results['version'], unicode):
+        if not is_str(results['version']):
             raise TypeError("string", repr(results['version']))
-        if not isinstance(results['required_api_version'], str) and not isinstance(results['required_api_version'], unicode):
+        if not is_str(results['required_api_version']):
             raise TypeError("string", repr(results['required_api_version']))
         if not isinstance(results['features'], list):
             raise TypeError("string list", repr(results['features']))
         for tmp_1 in results['features']:
-            if not isinstance(tmp_1, str) and not isinstance(tmp_1, unicode):
+            if not is_str(tmp_1):
                 raise TypeError("string", repr(tmp_1))
         if not isinstance(results['configuration'], dict):
             raise TypeError("(string * string) list", repr(results['configuration']))
         for tmp_2 in results['configuration'].keys():
-            if not isinstance(tmp_2, str) and not isinstance(tmp_2, unicode):
+            if not is_str(tmp_2):
                 raise TypeError("string", repr(tmp_2))
         for tmp_2 in results['configuration'].values():
-            if not isinstance(tmp_2, str) and not isinstance(tmp_2, unicode):
+            if not is_str(tmp_2):
                 raise TypeError("string", repr(tmp_2))
         if not isinstance(results['required_cluster_stack'], list):
             raise TypeError("string list", repr(results['required_cluster_stack']))
         for tmp_3 in results['required_cluster_stack']:
-            if not isinstance(tmp_3, str) and not isinstance(tmp_3, unicode):
+            if not is_str(tmp_3):
                 raise TypeError("string", repr(tmp_3))
         return results
     def ls(self, args):
@@ -73,13 +85,13 @@ def ls(self, args):
         if not('dbg' in args):
             raise UnmarshalException('argument missing', 'dbg', '')
         dbg = args["dbg"]
-        if not isinstance(dbg, str) and not isinstance(dbg, unicode):
+        if not is_str(dbg):
             raise TypeError("string", repr(dbg))
         results = self._impl.ls(dbg)
         if not isinstance(results, list):
             raise TypeError("string list", repr(results))
         for tmp_4 in results:
-            if not isinstance(tmp_4, str) and not isinstance(tmp_4, unicode):
+            if not is_str(tmp_4):
                 raise TypeError("string", repr(tmp_4))
         return results
     def diagnostics(self, args):
@@ -89,10 +101,10 @@ def diagnostics(self, args):
         if not('dbg' in args):
             raise UnmarshalException('argument missing', 'dbg', '')
         dbg = args["dbg"]
-        if not isinstance(dbg, str) and not isinstance(dbg, unicode):
+        if not is_str(dbg):
             raise TypeError("string", repr(dbg))
         results = self._impl.diagnostics(dbg)
-        if not isinstance(results, str) and not isinstance(results, unicode):
+        if not is_str(results):
             raise TypeError("string", repr(results))
         return results
     def _dispatch(self, method, params):