adding API and tests for IOCTL requests (and bypassing casadm) #920

karolinavelkaja · 2021-08-12T12:42:33Z

Initial/old reviews:

https://github.com/Open-CAS/test-framework/pull/237 (replacing changes from test framework to OCL)
Add IOCTL tests #664

Signed-off-by: Karolina Rogowska karolina.rogowska@intel.com

Signed-off-by: Karolina Rogowska <karolina.rogowska@intel.com>

pep8speaks · 2021-08-12T12:42:37Z

Hello @karolinavelkaja! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-08-12 12:44:45 UTC

robertbaldyga · 2021-08-16T04:10:23Z

test/functional/api/cas/ioctl/cas_structs.py

+class CacheLineSize(Enum):
+    ocf_cache_line_size_4 = ctypes.c_ulonglong(4).value * KiB
+    ocf_cache_line_size_8 = ctypes.c_ulonglong(8).value * KiB
+    ocf_cache_line_size16 = ctypes.c_ulonglong(16).value * KiB
+    ocf_cache_line_size_32 = ctypes.c_ulonglong(32).value * KiB
+    ocf_cache_line_size_64 = ctypes.c_ulonglong(64).value * KiB
+    default = ocf_cache_line_size_4


Why don't we use class Size for proper units and then convert bytes value to ctypes type?

robertbaldyga · 2021-08-16T04:13:15Z

test/functional/api/cas/ioctl/ioctl.py

+IOC_NRBITS = 8
+IOC_TYPEBITS = 8
+IOC_SIZEBITS = 14
+IOC_DIRBITS = 2
+
+IOC_NRMASK = (1 << IOC_NRBITS) - 1                  # 255
+IOC_TYPEMASK = (1 << IOC_TYPEBITS) - 1              # 255
+IOC_SIZEMASK = (1 << IOC_SIZEBITS) - 1              # 16 383
+IOC_DIRMASK = (1 << IOC_DIRBITS) - 1                # 3
+
+IOC_NRSHIFT = 0                                     # 0
+IOC_TYPESHIFT = IOC_NRSHIFT + IOC_NRBITS            # 8
+IOC_SIZESHIFT = IOC_TYPESHIFT + IOC_TYPEBITS        # 16
+IOC_DIRSHIFT = IOC_SIZESHIFT + IOC_SIZEBITS         # 30
+
+IOC_NONE = 0
+IOC_WRITE = 1
+IOC_READ = 2
+
+KCAS_IOCTL_MAGIC = 0xBA                             # 186
+
+IOC_IN = IOC_WRITE << IOC_DIRSHIFT                  # 1 073 741 824
+IOC_OUT = IOC_READ << IOC_DIRSHIFT                  # 2 147 483 648
+IOC_INOUT = (IOC_WRITE | IOC_READ) << IOC_DIRSHIFT  # 3 221 225 472
+IOCSIZE_MASK = IOC_SIZEMASK << IOC_SIZESHIFT        # 1 073 676 288
+IOCSIZE_SHIFT = IOC_SIZESHIFT                       # 16
+
+
+def IOC(dir, type, nr, size):
+    if dir > IOC_DIRMASK:
+        raise OverflowError(f"IO direction value {dir} exceeds {IOC_DIRMASK}")
+    dir <<= IOC_DIRSHIFT
+
+    if type > IOC_TYPEMASK:
+        raise OverflowError(f"IO type value {type} exceeds {IOC_TYPEMASK}")
+    type <<= IOC_TYPESHIFT
+
+    if nr > IOC_NRMASK:
+        raise OverflowError(f"IO command value {nr} exceeds {IOC_NRMASK}")
+    nr <<= IOC_NRSHIFT
+
+    if size > IOC_SIZEMASK:
+        raise OverflowError(f"IO size value {size} exceeds {IOC_SIZEMASK}")
+    size <<= IOC_SIZESHIFT
+
+    return dir | type | nr | size


There are libs for that https://pypi.org/project/ioctl-opt/

robertbaldyga · 2021-08-16T04:23:58Z

test/functional/api/cas/ioctl/ioctl.py

+def send_script_with_dumped_args():
+    if not check_if_directory_exists(temp_dir):
+        create_directory(temp_dir, True)
+
+    TestRun.executor.rsync_to(script_source, script_dest)
+    chmod_numerical(script_dest, 550)
+
+    TestRun.executor.rsync_to(struct_path, struct_path)
+    chmod_numerical(struct_path, 440)


We finally have DUT-side scripting! ❤️
Although it may have been more generalized for use in other places (even as a part of TF).

robertbaldyga · 2021-08-16T04:35:03Z

test/functional/api/cas/ioctl/ioctl.py

+        sleep(2)
+        TestRun.executor.kill_process(pid)


This function should at least take timeout as an argument. Otherwise it's unusable with commands like cache flush, which typically takes much longer than 2 seconds.

robertbaldyga · 2021-08-16T04:38:38Z

test/functional/api/cas/ioctl/ioctl.py

+    if check_if_directory_exists(temp_dir):
+        remove(temp_dir, True, True, True)
+    if os.path.exists(struct_path):
+        os.remove(struct_path)


Virtually all ioctls handled by CAS return information from kernel via ioctl structure, which means we want to have a way to receive this information back from DUT once ioctl is finished.

jfckm · 2021-08-19T11:20:57Z

test/functional/api/cas/ioctl/cas_structs.py

+
+
+class RequestCode(Enum):
+    START_CACHE_CODE = ctypes.c_uint(21).value


What does this construct do? I just checked that type(c_uint(21).value) == int so its equivalent with START_CACHE_CODE = 21 (maybe aside from integer wraparound effects, but that doesn't seem to be needed here)

jfckm · 2021-08-19T11:25:41Z

test/functional/tests/ioctl/common_utils.py

+interrupt_stop = [
+    r"Waiting for cache stop interrupted\. Stop will finish asynchronously\."
+]
+
+interrupt_start = [
+    r"Cache added successfully, but waiting interrupted\. Rollback"
+]
+
+load_and_force = [
+    r"cache\d+: Using \'force\' flag is forbidden for load operation\."
+]


I don't think those messages should be in this file. Maybe more appropriate place would be in api/cas/cli_messages.py?

or keep separate api/cas/ioctl/cli_messages.py

jfckm · 2021-08-19T11:27:11Z

test/functional/tests/ioctl/common_utils.py

+def clear_dmesg():
+    TestRun.executor.run_expect_success('dmesg -C')
+
+
+def check_dmesg(searched_phrase: str):
+    dmesg_out = TestRun.executor.run_expect_success("dmesg").stdout
+    __check_string_msg(dmesg_out, searched_phrase)


Those as well, I think clearing/checking dmesg is more generic operation, not limited only to ioctl tests.

Deixx · 2024-09-03T05:39:20Z

test/functional/api/cas/ioctl/__init__.py

Deixx · 2024-09-03T05:50:44Z

test/functional/api/cas/ioctl/ioctl.py

+    TestRun.executor.rsync_to(script_source, script_dest)
+    chmod_numerical(script_dest, 550)
+
+    TestRun.executor.rsync_to(struct_path, struct_path)
+    chmod_numerical(struct_path, 440)


rsync to be replaced

Deixx · 2024-09-03T05:54:24Z

test/functional/api/cas/ioctl/ioctl.py

+temp_dir = '/tmp/cas'
+struct_path = os.path.join(temp_dir, 'dump_file')
+script_source = os.path.join(f'{os.path.dirname(__file__)}', 'send_ioctl_script.py')
+script_dest = os.path.join(temp_dir, 'send_ioctl_script.py')


struct_path and script_source and local (so os.path is ok), but script_dest is remote (so Linux - posixpath required)
both struct_path (local) and script_dest (remote) use the same temp_dir, which is posix-style

Deixx · 2024-09-03T05:56:55Z

test/functional/tests/ioctl/__init__.py

Deixx · 2024-09-03T05:58:31Z

test/functional/tests/ioctl/common_utils.py

+interrupt_stop = [
+    r"Waiting for cache stop interrupted\. Stop will finish asynchronously\."
+]
+
+interrupt_start = [
+    r"Cache added successfully, but waiting interrupted\. Rollback"
+]
+
+load_and_force = [
+    r"cache\d+: Using \'force\' flag is forbidden for load operation\."
+]


or keep separate api/cas/ioctl/cli_messages.py

Deixx · 2024-09-03T06:01:44Z

test/functional/tests/ioctl/test_ioctl_start.py

+          - Cache started successfully without any errors if 'force' and 'load' flags are not used.
+          - When 'force' and 'load' flags are used cache is not started.


Suggested change

- Cache started successfully without any errors if 'force' and 'load' flags are not used.

- When 'force' and 'load' flags are used cache is not started.

- Cache started successfully without any errors if 'force' and 'load' flags are not used simultaneously.

- When 'force' and 'load' flags are both used cache is not started.

adding API and tests for IOCTL requests (and bypassing casadm)

6bcd126

Signed-off-by: Karolina Rogowska <karolina.rogowska@intel.com>

karolinavelkaja force-pushed the ioctl_requests branch from 5c36023 to 6bcd126 Compare August 12, 2021 12:44

karolinavelkaja mentioned this pull request Aug 12, 2021

Add IOCTL tests #664

Closed

robertbaldyga requested changes Aug 16, 2021

View reviewed changes

jfckm reviewed Aug 19, 2021

View reviewed changes

katlapinka added tests pull labels May 15, 2024

Deixx reviewed Sep 3, 2024

View reviewed changes

katlapinka added the v24.9 label Sep 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding API and tests for IOCTL requests (and bypassing casadm) #920

adding API and tests for IOCTL requests (and bypassing casadm) #920

karolinavelkaja commented Aug 12, 2021

pep8speaks commented Aug 12, 2021 •

edited

Loading

robertbaldyga Aug 16, 2021

robertbaldyga Aug 16, 2021

robertbaldyga Aug 16, 2021

robertbaldyga Aug 16, 2021

robertbaldyga Aug 16, 2021

jfckm Aug 19, 2021

jfckm Aug 19, 2021

Deixx Sep 3, 2024

jfckm Aug 19, 2021

Deixx Sep 3, 2024

Deixx Sep 3, 2024

Deixx Sep 3, 2024

Deixx Sep 3, 2024

Deixx Sep 3, 2024

Deixx Sep 3, 2024



		class RequestCode(Enum):
		START_CACHE_CODE = ctypes.c_uint(21).value

		- Cache started successfully without any errors if 'force' and 'load' flags are not used.
		- When 'force' and 'load' flags are used cache is not started.

adding API and tests for IOCTL requests (and bypassing casadm) #920

Are you sure you want to change the base?

adding API and tests for IOCTL requests (and bypassing casadm) #920

Conversation

karolinavelkaja commented Aug 12, 2021

pep8speaks commented Aug 12, 2021 • edited Loading

Comment last updated at 2021-08-12 12:44:45 UTC

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Aug 12, 2021 •

edited

Loading