Add iscsi helpers to dump iscsi session information #108

richl9 · 2024-09-30T22:09:43Z

Updated the helper based on feedback from @biger410

biger410

Please reorganize the patch, the second commit should be removed, the 3th to 5th should be merged into one commit. Please create a bug and put the bugdb id in the commit log, also for each commit, please brief in the git commit log what the change is for.

biger410 · 2024-12-03T23:49:36Z

Makefile

@@ -33,4 +33,4 @@ drgn_tools/_version.py:
 .PHONY: rsync
 rsync: drgn_tools/_version.py
 	@if [ -z "$(TARGET)" ]; then echo "error: TARGET unspecified. Either set it in config.mk, or use\nmake TARGET=hostname rsync"; exit 1; fi
-	rsync -avz --exclude "__pycache__" --exclude ".git" --exclude ".mypy_cache" ./drgn_tools $(TARGET):drgn_tools/
+	rsync -avz --exclude "__pycache__" --exclude ".git" --exclude ".mypy_cache" ./drgn_tools $(TARGET)


Please explain what this change is for.

biger410 · 2024-12-03T23:49:41Z

drgn_tools/dm.py

+            if hc.uuid:
+                uuid = hc.uuid.string_().decode()
+
+            yield hc.md, hc.name.string_().decode(), uuid


Please explain what case this uuid can be NULL.

biger410 · 2024-12-03T23:57:13Z

drgn_tools/iscsi.py

+    else:
+        return list_for_each_entry(
+            "struct device", devices, "knode_class.n_node"
+        )


Is this good program practice for python to mix yield and return?

biger410 · 2024-12-03T23:58:23Z

drgn_tools/iscsi.py

+            dev = dev.parent
+
+        if dev.type and dev.type.name.string_().decode() == "scsi_host":
+            yield dev


This doesn't look right, multiple disks can be from same iscsi host, this will return duplcated iscsi host.

biger410 · 2024-12-04T00:08:26Z

drgn_tools/iscsi.py

+        for device_private in list_for_each_entry(
+            "struct device_private", devices, "knode_class.n_node"
+        ):
+            yield device_private.device


In the code path of class_in_private is not null, it never checks anything for iscsi, why will it be iscsi device?

biger410 · 2024-12-04T00:10:49Z

drgn_tools/iscsi.py

@@ -60,7 +65,7 @@ def for_each_iscsi_host_device(prog: Program) -> Iterator[Object]:
            yield dev


-def for_each_iscsi_host(prog: Program) -> Iterator[Object]:
+def for_each_scsi_host(prog: Program) -> Iterator[Object]:


for_each_scsi_host is already defined in scsi.py, just import it.

biger410 · 2024-12-04T00:12:52Z

drgn_tools/iscsi.py


+    return iscsi_sw_tcp_host.session


We should do some precheck to make sure this is sw tcp iscsi, if it's not, then leave a message saying it's not supported, otherwise the script will just crash for other iscsi type.

biger410 · 2024-12-04T00:16:52Z

drgn_tools/iscsi.py

-    for h in for_each_iscsi_host(prog):
-        yield __get_iscsi_session(h)
+    for h in for_each_scsi_host(prog):
+        yield __get_iscsi_session(prog, h)


The system could contain other scsi disk like virtio-scsi, sas or fc disk other than iscsi, here it is iterating scsi host, it will include all other types as well, we should provide a new helper for_each_iscsi_host which used for_each_scsi_host and then just return the host which is iscsi type.

drgn_tools/iscsi.py

biger410 · 2024-12-09T21:50:07Z

drgn_tools/iscsi.py

+    rq = sdev.request_queue
+    dev = container_of(rq.kobj.parent, "struct device", "kobj")
+    return dev.kobj.name.string_().decode()
+


shost_for_each_device and get_dev_name are generic scsi helpers, should be moved to scsi.py, and also maybe rename it to for_each_scsi_host_device and get_scsi_device_name?

biger410 · 2024-12-09T21:52:55Z

drgn_tools/iscsi.py

+    """
+    for h in for_each_iscsi_tcp_host(prog):
+        yield __get_iscsi_sw_tcp_host(prog, h).session
+


As in my following comment, let try to make these helper generic to iscsi drivers, not only to iscsi_tcp driver.

biger410 · 2024-12-09T21:56:22Z

One more comment, please change the commit subject for this patch, probably "Add iscsi helpers to dump iscsi session information"

richl9 · 2024-12-10T23:42:37Z

I noticed that there were test failures reporting a key error for doing 'prog['sesslist']' for ueknext. I tested it locally with 6.11.0-0 as well but didn't observe such error. Maybe could @brenns10 help take a look? Thanks.

biger410 · 2024-12-11T23:20:19Z

drgn_tools/iscsi.py

+    :returns: an iterator of ``struct Scsi_Host *``
+    """
+    for shost in for_each_scsi_host(prog):
+        if "iscsi" in host_module_name(shost).lower():


Not all iscsi hardware driver has iscsi in its name, please use grep iscsi drivers/iscsi find the all iscsi driver module names and create a list for matching.

biger410 · 2024-12-11T23:43:19Z

drgn_tools/iscsi.py

+    """
+    for scsi_dev in for_each_scsi_host_device(prog, shost):
+        name = get_scsi_device_name(prog, scsi_dev)
+        yield scsi_dev, name


This helper seems not necessary to me, it used in print_iscsi_report, you can just use for_each_scsi_host_device and invoke get_scsi_device_name if name is needed.

biger410 · 2024-12-11T23:46:34Z

drgn_tools/iscsi.py

+        print()
+
+
+class IscsiDump(CorelensModule):


Let use Iscsi as the name

biger410 · 2024-12-11T23:49:45Z

drgn_tools/iscsi.py

+def print_iscsi_report(prog: Program) -> None:
+    """
+    Prints a comprehensive report including iscsi table and session stats. More info can be added later if needed.
+    """


Let make the name print_iscsi_sessions and update above comments, since this is only for dumping iscsi sessions. If dump other information is needed, we can add new functions.

Please add below code at the beginning of this function, this will make sure the helper bail out if iscsi is not used when invoking this helper directly from drgn shell while not corelen module.

msg = ensure_debuginfo(prog, ["libiscsi", "scsi_transport_iscsi"]) if msg: print(msg) return

biger410 · 2024-12-11T23:51:37Z

drgn_tools/scsi.py

+    prog: Program, shost: Object
+) -> Iterator[Object]:
+    """
+    Get a list of scsi_devices asscociated with an Scsi_Host


typo, s/asscociated/associated/ , s/an/one

biger410 · 2024-12-11T23:55:50Z

drgn_tools/scsi.py

+) -> Iterator[Object]:
+    """
+    Get a list of scsi_devices asscociated with an Scsi_Host
+    :returns: a iterator of ``struct scsi_device``


struct scsi_device *

biger410 · 2024-12-11T23:58:18Z

drgn_tools/scsi.py

+    )
+
+
+def get_scsi_device_name(prog: Program, sdev: Object) -> str:


Please put module name at the first, scsi_device_name?

drgn_tools/iscsi.py

biger410 · 2024-12-12T00:11:54Z

tests/test_iscsi.py

+    for mod in ISCSI_MODS:
+        km = KernelModule.find(prog, mod)
+        if not km:
+            return


With ensure_debuginfo in the beginning of print_iscsi_report, above loading debuginfo code is not needed.

biger410 · 2024-12-12T00:15:14Z

Also the gitlab CI test failed, please fix it.

drgn_tools/iscsi.py

Orabug: 37362180 Signed-off-by: Richard Li <[email protected]>

biger410

Looks good, thank you.

oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Sep 30, 2024

biger410 reviewed Dec 4, 2024

View reviewed changes

richl9 force-pushed the richard/iscsi branch from d39ab83 to be3ae35 Compare December 6, 2024 21:45

biger410 reviewed Dec 9, 2024

View reviewed changes

richl9 force-pushed the richard/iscsi branch from be3ae35 to 4dfcd2e Compare December 10, 2024 22:12

richl9 changed the title ~~Richard/iscsi~~ Add iscsi helpers to dump iscsi session information Dec 10, 2024

richl9 force-pushed the richard/iscsi branch 3 times, most recently from 3f1f2f6 to e395eb3 Compare December 11, 2024 22:53

biger410 reviewed Dec 12, 2024

View reviewed changes

richl9 force-pushed the richard/iscsi branch from e395eb3 to 40a4d0c Compare December 13, 2024 01:00

biger410 reviewed Dec 17, 2024

View reviewed changes

drgn_tools/iscsi.py Show resolved Hide resolved

richl9 force-pushed the richard/iscsi branch 2 times, most recently from 066e731 to 5828281 Compare December 18, 2024 21:05

Add iscsi helpers to dump iscsi session

85ef297

Orabug: 37362180 Signed-off-by: Richard Li <[email protected]>

richl9 force-pushed the richard/iscsi branch from 5828281 to 85ef297 Compare December 20, 2024 17:43

biger410 approved these changes Jan 3, 2025

View reviewed changes

brenns10 approved these changes Jan 3, 2025

View reviewed changes

brenns10 merged commit 75f7896 into oracle-samples:main Jan 3, 2025
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add iscsi helpers to dump iscsi session information #108

Add iscsi helpers to dump iscsi session information #108

richl9 commented Sep 30, 2024 •

edited

Loading

biger410 left a comment

biger410 Dec 3, 2024

biger410 Dec 3, 2024

biger410 Dec 3, 2024

biger410 Dec 3, 2024

biger410 Dec 4, 2024

biger410 Dec 4, 2024

biger410 Dec 4, 2024

biger410 Dec 4, 2024

biger410 Dec 9, 2024

biger410 Dec 9, 2024

biger410 commented Dec 9, 2024

richl9 commented Dec 10, 2024

biger410 Dec 11, 2024

biger410 Dec 11, 2024

biger410 Dec 11, 2024

biger410 Dec 11, 2024

biger410 Dec 12, 2024

biger410 Dec 11, 2024

biger410 Dec 11, 2024

biger410 Dec 11, 2024

biger410 Dec 12, 2024

biger410 commented Dec 12, 2024

biger410 left a comment

		)


		def get_scsi_device_name(prog: Program, sdev: Object) -> str:

Add iscsi helpers to dump iscsi session information #108

Add iscsi helpers to dump iscsi session information #108

Conversation

richl9 commented Sep 30, 2024 • edited Loading

biger410 left a 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

biger410 commented Dec 9, 2024

richl9 commented Dec 10, 2024

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

biger410 commented Dec 12, 2024

biger410 left a comment

Choose a reason for hiding this comment

richl9 commented Sep 30, 2024 •

edited

Loading