Add more info to iscsi dump info #142
Conversation
Orabug: 34461976 Signed-off-by: Gulam Mohamed <[email protected]>
|
Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application. When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated. If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public. |
oracle-contributor-agreement
bot
added
the
OCA Required
Correct the name of the test function from print_iscsi_sessions() to dump_iscsi_sessions()
There was a problem hiding this comment.
I've left specific feedback below, but two pieces of general feedback regarding the whole patch:
- The second commit in this branch does not have a
Signed-off-by. This is why the bot has marked this with a "needs OCA". I think that second commit probably should just be folded into the first one. So please submit this as a single commit, making sure you have a sign-off in it. - The code here is not formatted according to the guidelines. You can see the pre-commit output here. Please be sure to set up pre-commit in your development environment and use it for future submissions.
Both of these are covered in the developer workflow doc, reach out to me if you need more info.
| # Available Hardware and Software iSCSI Transports | ||
| iscsi_transports = ["bnx2i", | ||
| "cxgb3i", | ||
| "cxgb4i", | ||
| "iser", | ||
| "qedi", | ||
| "bfin dpmc", | ||
| "h1940-bt", | ||
| "tcp"] |
| "username": (escape_ascii_string(iscsi_session.username.string_()) if iscsi_session.username else "<empty>"), | ||
| "password": (escape_ascii_string(iscsi_session.password.string_()) if iscsi_session.password else "********"), | ||
| "username_in": (escape_ascii_string(iscsi_session.username_in.string_()) if iscsi_session.username_in else "<empty>"), | ||
| "password_in": (escape_ascii_string(iscsi_session.password_in.string_()) if iscsi_session.password_in else "********"), |
There was a problem hiding this comment.
So, what's the security implication of this? We should not print and include username/password combinations in corelens reports just because they are there. Do we have a valid debugging use case for including them? I would strongly discourage this unless absolutely necessary.
| iscsi_session_states = ["UNKNOWN", | ||
| "FREE", | ||
| "LOGGED_IN", | ||
| "FAILED", | ||
| "TERMINATE", | ||
| "IN_RECOVERY", | ||
| "RECOVERY_FAILED", | ||
| "LOGGING_OUT" | ||
| ] |
There was a problem hiding this comment.
First: please make all global variables named in ALL_CAPS, since it makes reading the code that uses them much easier. This should apply for the rest of the globals here.
Second: These states appear to be an enumeration from libiscsi.h. For enums, the best thing to do is rely on the kernel's debuginfo to contain the enumeration constants. Sadly, libiscsi.h makes these enumerations anonymous, so it's slightly difficult to get them. Here's an example:
from typing import Dict, Type
from drgn import Program
from drgn.helpers.common import enum_type_to_class
def iscsi_constants(prog: Program) -> Dict[str, Type]:
return {
"ISCSI_STATE": enum_type_to_class(
prog.constant("ISCSI_STATE_FREE").type_,
"IscsiState",
prefix="ISCSI_STATE",
),
...
}You can do this for each enum: choose the most common variant (the one whose name is least likely to change), and then create the Python class with enum_type_to_class().
Once you have the python enum class corresponding to it, it will be quite easy to just lookup the name. For instance:
state = 5 # in reality, this would come from the program
consts = iscsi_constants(prog)
value = consts(state)
print(str(value))That goes for all these lists of constants below - look them up from the program and create them dynamically. That way if any constants change in any kernel version, you'll get the right values.
| for scsi_dev in for_each_scsi_host_device(prog, session.host): | ||
| name = scsi_device_name(prog, scsi_dev) | ||
| print( | ||
| "scsi{} Channel {} Id {} Lun: {}".format( | ||
| session.host.host_no.value_(), | ||
| int(scsi_dev.channel), | ||
| int(scsi_dev.id), | ||
| int(scsi_dev.lun), | ||
| ) | ||
| ) | ||
| sdev_state = enum_type_to_class( | ||
| prog.type("enum scsi_device_state"), "sdev_state" | ||
| ) | ||
| state = enum_name_get( | ||
| sdev_state, | ||
| scsi_dev.sdev_state, | ||
| "UNKNOWN", | ||
| ) | ||
| print(" Attached iscsi disk: {} State: {}".format(name, state)) | ||
|
|
||
| print() | ||
|
|
||
| iscsi_disks_info = get_iscsi_disks_info(scsi_dev, prog) | ||
| print_iscsi_info(iscsi_disks_info) |
There was a problem hiding this comment.
Could this be a table instead? It would probably make it easier to read. See the drgn_tools/table.py.
| sub_headings = ["Interface", | ||
| "CHAP", | ||
| "Timeouts", | ||
| "Negotiated iSCSI params", | ||
| "Attached SCSI devices" | ||
| ] | ||
|
|
||
| def print_iscsi_info(info: Dict[str, Any]) -> None: | ||
| for line in info: | ||
| if "Target IQN" in line: | ||
| print(f"{line}: {info[line]}") | ||
| elif line == "Current Portal" or line == "Persistent Portal": | ||
| print(f"\t{line}: {info[line]}") | ||
| elif any(line in s for s in sub_headings): | ||
| print(f"\t\t"+"*" * (len(line) + 1)) | ||
| print(f"\t\t{line}: {info[line]}") | ||
| print(f"\t\t"+"*" * (len(line) + 1)) | ||
| else: | ||
| print(f"\t\t{line}: {info[line]}") |
There was a problem hiding this comment.
I do like seeing a separation between helpers that get information, and those that print information. That makes it easier for others to write new code based on your helpers. So I do like what you're trying to do here.
But I'm not certain that it's really helping to have a print_iscsi_info() function and a get_iscsi_info() here, for a few reasons.
- The dictionary returned by
get_iscsi_info()is just a bunch of strings. It seems to me that if a developer wanted to get the timeout associated with aniscsi_session, they would want it as a numeric value, not a string that they would need to interpret. So if you're going to make aget_iscsi_info()function, it should return data in a form that a developer would like, not in the form that you want to print it. - The use of blank "header" keys is a confusing design choice, that can easily go out-of-date if somebody updates the
get_iscsi_info()function, but forgets to update the list ofsub_headings.
I think there are two ways to go about this. Either change get_iscsi_info() to just be print_iscsi_info(), so that it directly prints the data without sticking it into a dict first, or else change get_iscsi_info() to return the data in a developer-friendly format (e.g. a dataclass or named tuple) and then format it dircetly in the print_iscsi_info() function.
For an example of the latter, see get_kmem_cache_slub_info() in drgn_tools/slabinfo.py. It returns a named tuple that is later formatted (as part of a table).
| current_address = iscsi_sock.__sk_common.skc_daddr | ||
| current_address = prog.read_u32(current_address.address_of_()) | ||
| current_ip = socket.inet_ntoa(bytes([(current_address) & 0xFF, | ||
| (current_address >> 8) & 0xFF, | ||
| (current_address >> 16) & 0xFF, | ||
| (current_address >> 24) & 0xFF])) | ||
| current_port = iscsi_sock.__sk_common.skc_dport | ||
| current_port = prog.read_u16(current_port.address_of_()) | ||
| current_port = socket.ntohs(current_port) | ||
| local_address = iscsi_sock.__sk_common.skc_rcv_saddr | ||
| local_address = prog.read_u32(local_address.address_of_()) | ||
| local_ip = socket.inet_ntoa(bytes([(local_address) & 0xFF, | ||
| (local_address >> 8) & 0xFF, | ||
| (local_address >> 16) & 0xFF, | ||
| (local_address >> 24) & 0xFF])) |
There was a problem hiding this comment.
For IP addresses, you should use get_ipv4() which is defined in drgn_tools/crash_net.py, see its use there. The kernel always stores the IP addresses in network byte order, so as long as you use that function, you don't need to do byte swapping.
For the port, only do the byte swap if the program is little endian. (which is true for all the architectures we use, but we really should do it correctly). For example, see these lines in the crash_net.py file:
dport = int(skc.skc_dport)
if prog.platform.flags & PlatformFlags.IS_LITTLE_ENDIAN:
dport = socketlib.ntohs(dport)
|
Discussed with Gulam, he will address the legal requirement and open a new PR with all above @brenns10 's feedback addressed. |
Orabug: 34461976