mctpd: update get msg type if vdm is registered #134
Conversation
src/mctpd.c
Outdated
| // Allocate extra space for the message types | ||
| resp_len = sizeof(*resp) + type_count; | ||
| // Allocate extra space for the message types. Also two byte for PCIe or IANA VDM | ||
| resp_len = sizeof(*resp) + type_count + sizeof(uint16_t); |
There was a problem hiding this comment.
but there is no uint16_t involved here.
| msg_types[type_count++] = MCTP_TYPE_VENDOR_IANA; | ||
| resp_len++; | ||
| } | ||
| } |
There was a problem hiding this comment.
What if someone has already registered 7e/7f ?
Doing the find (setting the booleans) and message construction (setting msg_types) in the same place is a bit unusual.
If you're taking the approach that 7e/7f do not appear in the supported_message_types array, then I would suggest detecting their presence earlier (ie, iterating supported_vdm_types there), which means you can then allocate exactly the correct size message, then do the message construction later, based on the earlier detection.
There was a problem hiding this comment.
I think 7e or 7f can come in supported_message_types .
Updates the checks
tests/test_mctpd.py
Outdated
| cmd = MCTPControlCommand(True, 0, 0x05, bytes([0x00])) | ||
| rsp = await ep.send_control(mctpd.network.mctp_socket, cmd) | ||
| assert rsp.hex(' ') == '00 05 00 04 00 05 7e 7f' | ||
|
|
There was a problem hiding this comment.
might be good to also test that the type is removed after unregistration, but you could also do that in the original unregistration test.
There was a problem hiding this comment.
Removed the newly added test. Updated VDM test
msnidhin
force-pushed
the
get_msg_type_vdm
branch
from
c9aeda9 to
b65f632
Compare
|
Hi @jk-ozlabs |
|
At a conference at present, will check it out shortly. |
jk-ozlabs
left a comment
jk-ozlabs
left a comment
There was a problem hiding this comment.
There seems to a conflict with registering 0x7e / 0x7f directly here.
I think we may want to prohibit 0x7e/0x7f from appearing in the supported_msg_types array, and hence also being registered through the RegisterTypeSupport dbus method. If a client wants to register a vendor type, we really need the extra information to appear in the Get Vendor Message Types response.
src/mctpd.c
Outdated
| for (i = 0; i < ctx->num_supported_msg_types; i++) { | ||
| pcie_support |= (ctx->supported_msg_types[i].msg_type == | ||
| MCTP_TYPE_VENDOR_PCIE); | ||
| iana_support |= (ctx->supported_msg_types[i].msg_type == | ||
| MCTP_TYPE_VENDOR_IANA); | ||
| } |
There was a problem hiding this comment.
Wouldn't this duplicate the entries for 0x7e/0x7f in the response?
There was a problem hiding this comment.
It should not.. since pcie_support and iana_support will be either true or false at the end
Let me add a unit test for two PCIe VDMs
msnidhin
force-pushed
the
get_msg_type_vdm
branch
2 times, most recently
from
120b5e8 to
7448d19
Compare
If VDM type support is registered using dbus call RegisterVDMTypeSupport then add 7E and 7F based on the format in GetMsgType control command response Signed-off-by: Nidhin MS <nidhin.ms@intel.com>
2 participants
If VDM type support is registered using dbus call RegisterVDMTypeSupport then based
on the format add 7E and 7F in GetMsgType control command response