pyocf: simplify volume open/close API

Make Volumes usable by both pyocf and OCF with clear open/_open split
and clean-up of instance/uuid tracking on C interface only.

Signed-off-by: Jan Musial <jan.musial@intel.com>

tests/functional/pyocf/types/cvolume.py

@@ -58,13 +58,20 @@ class CVolume(OcfInternalVolume):
     def get_c_handle(self):
         return self.cvol.value
 
-    def do_open(self):
+    def open(self):
-        ret = self.lib.ocf_volume_open(self.cvol, c_void_p())
+        ret = super().open()
-        if ret != 0:
+        if ret == 0:
-            raise OcfError("openning composite volume failed", ret)
+            ret = self.lib.ocf_volume_open(self.handle, c_void_p())
+
+        if ret:
+            raise OcfError("opening composite volume failed", ret)
+
+        return ret
+
+    def close(self):
+        self.lib.ocf_volume_close(self.handle)
+        super().close()
 
-    def do_close(self):
-        self.lib.ocf_volume_close(self.cvol)
 
 
 lib = OcfLib.getInstance()

tests/functional/pyocf/types/volume.py

@@ -23,6 +23,7 @@ from ctypes import (
 from hashlib import md5
 import weakref
 from enum import IntEnum
+import warnings
 
 from .io import Io, IoOps, IoDir
 from .queue import Queue
@@ -143,16 +144,25 @@ class Volume:
             try:
                 volume = Volume.get_by_uuid(uuid)
             except:  # noqa E722 TODO:Investigate whether this really should be so broad
-                print("Tried to access unallocated volume {}".format(uuid))
+                warnings.warn("Tried to access unallocated volume {}".format(uuid))
-                print("{}".format(Volume._uuid_))
                 return -1
 
-            return Volume.s_open(ref, volume)
+            ret = volume.open()
+            if not ret:
+                Volume._instances_[ref] = volume
+                volume.handle = ref
+
+            return ret
+
 
         @VolumeOps.CLOSE
         def _close(ref):
             volume = Volume.get_instance(ref)
-            Volume.s_close(volume)
+
+            del Volume._instances_[volume.handle]
+            volume.handle = None
+
+            volume.close()
 
         @VolumeOps.GET_MAX_IO_SIZE
         def _get_max_io_size(ref):
@@ -178,31 +188,19 @@ class Volume:
 
         return Volume._ops_[cls]
 
-    @staticmethod
+    def open(self):
-    def s_open(ref, volume):
+        if self.opened:
-        if volume.opened:
             return -OcfErrorCode.OCF_ERR_NOT_OPEN_EXC
 
-        volume.handle = ref
+        self.opened = True
-        Volume._instances_[ref] = volume
-        volume.opened = True
 
-        ret = volume.do_open()
+        return 0
-        if ret == 0:
-            volume.opened = True
 
-        return ret
+    def close(self):
-
+        if not self.opened:
-    @staticmethod
-    def s_close(volume):
-        if not volume.opened:
             return
 
-        volume.do_close()
+        self.opened = False
-        volume.opened = False
-
-        del Volume._instances_[volume.handle]
-        volume.handle = None
 
     @classmethod
     def get_io_ops(cls):
@@ -230,7 +228,7 @@ class Volume:
     @classmethod
     def get_instance(cls, ref):
         if ref not in cls._instances_:
-            print("tried to access {} but it's gone".format(ref))
+            warnings.warn(f"tried to access volume ref {ref} but it's gone")
             return None
 
         return cls._instances_[ref]
@@ -270,15 +268,6 @@ class Volume:
         self.opened = False
         self.handle = None
 
-    def do_open(self):
-        return 0
-
-    def do_close(self):
-        try:
-            del Volume._instances_[self.handle]
-        except AttributeError:
-            pass
-
     def get_length(self):
         raise NotImplementedError
 
@@ -444,7 +433,6 @@ class ErrorDevice(Volume):
         uuid=None,
     ):
         self.vol = vol
-        self.vol.open()
         super().__init__(uuid)
         self.error_sectors = error_sectors or set()
         self.error_seq_no = error_seq_no or {IoDir.WRITE: -1, IoDir.READ: -1}
@@ -456,6 +444,16 @@ class ErrorDevice(Volume):
     def set_mapping(self, error_sectors: set):
         self.error_sectors = error_sectors
 
+    def open(self):
+        ret = self.vol.open()
+        if ret:
+            return ret
+        return super().open()
+
+    def close(self):
+        super().close()
+        self.vol.close()
+
     def should_forward_io(self, io):
         if not self.armed:
             return True
@@ -540,6 +538,16 @@ class TraceDevice(Volume):
         super().__init__(uuid)
         self.trace_fcn = trace_fcn
 
+    def open(self):
+        ret = self.vol.open()
+        if ret:
+            return ret
+        return super().open()
+
+    def close(self):
+        super().close()
+        self.vol.close()
+
     def _trace(self, io, io_type):
         submit = True
 

tests/functional/pyocf/types/volume_replicated.py

@@ -5,6 +5,7 @@
 
 from threading import Lock
 from .volume import Volume, VOLUME_POISON
+from .shared import OcfErrorCode
 from .io import Io, IoDir
 from ctypes import cast, c_void_p, CFUNCTYPE, c_int, POINTER, memmove, sizeof, pointer
 
@@ -14,19 +15,24 @@ class ReplicatedVolume(Volume):
         super().__init__(uuid)
         self.primary = primary
         self.secondary = secondary
+
+    def open(self):
         ret = self.primary.open()
         if ret:
             raise Exception(f"Couldn't open primary volume. ({ret})")
+            return ret
         ret = self.secondary.open()
         if ret:
             raise Exception(f"Couldn't open secondary volume. ({ret})")
+            return ret
 
-        if secondary.get_max_io_size() < primary.get_max_io_size():
+        if self.secondary.get_max_io_size() < self.primary.get_max_io_size():
             raise Exception("secondary volume max io size too small")
-        if secondary.get_length() < primary.get_length():
+            return -OcfErrorCode.OCF_ERR_INVAL
+        if self.secondary.get_length() < self.primary.get_length():
             raise Exception("secondary volume size too small")
+            return -OcfErrorCode.OCF_ERR_INVAL
 
-    def open(self):
         return super().open()
 
     def close(self):

tests/functional/tests/management/test_metadata_volatile.py

@@ -92,7 +92,7 @@ def test_metadata_volatile_io(pyocf_ctx):
     cache.change_cache_mode(CacheMode.WB)
     core = Core.using_device(core_device, name="test_core")
     cache.add_core(core)
-    vol = CoreVolume(core, open=True)
+    vol = CoreVolume(core)
 
     r = (
         Rio()

tests/functional/tests/management/test_start_stop.py

@@ -153,7 +153,7 @@ def test_start_read_first_and_check_mode(pyocf_ctx, mode: CacheMode, cls: CacheL
     io_from_exported_object(front_vol, queue, test_data.size, Size.from_sector(1).B)
     check_stats_read_after_write(core, mode, cls)
 
-    check_md5_sums(vol, mode)
+    check_md5_sums(front_vol, mode)
 
 
 @pytest.mark.parametrize("cls", CacheLineSize)
@@ -213,7 +213,7 @@ def test_stop(pyocf_ctx, mode: CacheMode, cls: CacheLineSize, with_flush: bool):
 
     cls_no = 10
 
-    run_io_and_cache_data_if_possible(core, mode, cls, cls_no)
+    run_io_and_cache_data_if_possible(front_vol, mode, cls, cls_no)
 
     stats = cache.get_stats()
     assert int(stats["conf"]["dirty"]) == (
@@ -495,23 +495,21 @@ def test_start_stop_noqueue(pyocf_ctx):
     assert not c.results["error"], "Failed to stop cache: {}".format(c.results["error"])
 
 
-def run_io_and_cache_data_if_possible(core, mode, cls, cls_no):
+def run_io_and_cache_data_if_possible(vol, mode, cls, cls_no):
-    front_vol = core.get_front_volume()
+    queue = vol.parent.get_default_queue()
-    bottom_vol = core.get_volume()
-    queue = core.cache.get_default_queue()
 
     test_data = Data(cls_no * cls)
 
     if mode in {CacheMode.WI, CacheMode.WA}:
         logger.info("[STAGE] Write to core device")
-        io_to_core(bottom_vol, queue, test_data, 0)
+        io_to_core(vol.parent.device, queue, test_data, 0)
         logger.info("[STAGE] Read from exported object")
-        io_from_exported_object(front_vol, queue, test_data.size, 0)
+        io_from_exported_object(vol, queue, test_data.size, 0)
     else:
         logger.info("[STAGE] Write to exported object")
-        io_to_core(front_vol, queue, test_data, 0)
+        io_to_core(vol, queue, test_data, 0)
 
-    stats = core.cache.get_stats()
+    stats = vol.parent.cache.get_stats()
     assert stats["usage"]["occupancy"]["value"] == (
         (cls_no * cls / CacheLineSize.LINE_4KiB) if mode != CacheMode.PT else 0
     ), "Occupancy"
@@ -643,7 +641,7 @@ def check_md5_sums(vol: CoreVolume, mode: CacheMode):
         assert (
             vol.parent.device.md5() != vol.md5()
         ), "MD5 check: core device vs exported object without flush"
-        core.cache.flush()
+        vol.parent.cache.flush()
         assert (
             vol.parent.device.md5() == vol.md5()
         ), "MD5 check: core device vs exported object after flush"